feat(org-lens): extend shared Valkey cache across Org Lens SSR surface (LFXV2-2356)#970
Conversation
LFXV2-2356) Put a read-through cache in front of every uncached Org Lens server read, reusing the existing CachePort/ValkeyService foundation, with two keying models: - Per-org keys for caller-invariant Snowflake reads (WHERE ACCOUNT_ID = ?), refreshed on the daily dbt batch (1h TTL): overview foundations + involvement, memberships, training, contributions, events, and the People Snowflake tabs. - Per-user keys for FGA/token-filtered reads (30s TTL): the org-wide committee seat drain (now shared once across the Board, Committee, and directory pickers), key contacts, and the org-lens access list. Also migrates the org people directory off its per-instance in-memory Map onto the shared cache, finishing the cross-instance-coherence migration. Caching stays fail-soft (faults degrade to a direct fetch) and fail-closed (unresolved identity bypasses the cache), with shape guards rejecting corrupt entries and successful results only. The account-context fan-out is bounded by a worker pool to protect the Snowflake connection pool on a cold cache. Signed-off-by: Luis Mori Guerra <luismorith@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Valkey-backed caching to all Org Lens Snowflake-reading services. New ChangesValkey Org Lens caching rollout
Sequence Diagram(s)sequenceDiagram
participant Service as Org Lens Service
participant withOrgCache
participant buildOrgCacheKey
participant ValkeyService as valkeyService
participant TypeGuard as accept(value)
participant SnowflakeSvc as Snowflake
Service->>withOrgCache: withOrgCache(accountId, subResource, ttl, fetcher, accept)
withOrgCache->>buildOrgCacheKey: buildOrgCacheKey(accountId, subResource)
alt accountId filter-safe
buildOrgCacheKey-->>withOrgCache: key
else accountId unsafe
buildOrgCacheKey-->>withOrgCache: null
withOrgCache->>SnowflakeSvc: fetcher() [bypass cache]
end
alt valid key
withOrgCache->>ValkeyService: withCache(key, ttl, fetcher, accept)
alt cache hit
ValkeyService->>TypeGuard: accept(cachedValue)
alt accept=true
TypeGuard-->>ValkeyService: true
ValkeyService-->>withOrgCache: cached raw rows
else accept=false
TypeGuard-->>ValkeyService: false [corrupt/legacy]
ValkeyService->>SnowflakeSvc: fetcher()
SnowflakeSvc-->>ValkeyService: raw rows
ValkeyService-->>withOrgCache: raw rows [store in cache]
end
else cache miss
ValkeyService->>SnowflakeSvc: fetcher()
SnowflakeSvc-->>ValkeyService: raw rows
ValkeyService-->>withOrgCache: raw rows [store in cache]
end
end
withOrgCache-->>Service: raw rows
Service->>Service: map raw rows → typed response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing shared Valkey (Redis) read-through cache across the Org Lens SSR/server read surface, adding standardized key namespaces/TTLs and wrapping previously-uncached Snowflake and permission-scoped reads with withOrgCache / withPerUserCache helpers.
Changes:
- Added Org Lens–specific cache namespaces and TTL constants to the shared
VALKEY_CACHEconfiguration. - Introduced server helpers for consistent per-org and per-user cache-key construction and read-through caching.
- Applied caching across Org Lens services (Snowflake-backed reads and per-user permission-filtered reads), including migrating the people directory off in-memory caching.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/constants/valkey-cache.constants.ts | Adds Org Lens cache namespaces and TTL constants for per-org/per-user caching. |
| packages/shared/src/constants/org-selector.constants.ts | Adds a concurrency cap constant for Org Lens account-context cache warmup. |
| apps/lfx-one/src/server/services/valkey.service.ts | Adds key-prefix/key-builder helpers and withOrgCache/withPerUserCache wrappers over ValkeyService.withCache. |
| apps/lfx-one/src/server/services/organization.service.ts | Caches Org Lens account-context reads per accountId with a bounded concurrency worker pool. |
| apps/lfx-one/src/server/services/org-people-trainees.service.ts | Wraps bundled trainees tab Snowflake reads in per-org cache with a shape guard. |
| apps/lfx-one/src/server/services/org-people-key-contacts.service.ts | Adds per-user caching for key contacts using effective username + org UID keying. |
| apps/lfx-one/src/server/services/org-people-event-attendees.service.ts | Wraps bundled event attendees tab Snowflake reads in per-org cache with a shape guard. |
| apps/lfx-one/src/server/services/org-people-directory.service.ts | Migrates the unified directory from in-memory Map caching to shared per-user Valkey caching. |
| apps/lfx-one/src/server/services/org-people-contributors.service.ts | Caches contributors tab Snowflake reads per-org with a time-range subresource suffix. |
| apps/lfx-one/src/server/services/org-lens-training.service.ts | Adds per-org caching for training stats, paged lists, and roster drawers with param signatures. |
| apps/lfx-one/src/server/services/org-lens-people.service.ts | Caches the “All Employees” bundle and per-person detail bundle per org; moves mapping after cache read. |
| apps/lfx-one/src/server/services/org-lens-memberships.service.ts | Caches raw Snowflake membership rows per org and keeps filtering out of cache key. |
| apps/lfx-one/src/server/services/org-lens-foundations.service.ts | Caches foundations/projects response per org with a response shape guard. |
| apps/lfx-one/src/server/services/org-lens-events.service.ts | Adds per-org caching for event list/summary and drawer reads using a param signature. |
| apps/lfx-one/src/server/services/org-lens-board-committee.service.ts | Adds per-user caching for the org-wide seat drain used by multiple Org Lens consumers. |
| apps/lfx-one/src/server/services/org-lens-access.service.ts | Adds per-user caching for access list and principals reads; preserves bypass for post-write refresh. |
| apps/lfx-one/src/server/services/org-involvement.service.ts | Adds per-org caching across involvement metrics endpoints with a generic accountId guard. |
| apps/lfx-one/src/server/services/org-contributions.service.ts | Wraps contributions page Snowflake fan-out (KPIs, lists, options) in per-org caching keyed by query signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-970.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/lfx-one/src/server/services/org-lens-access.service.ts (1)
82-91: 💤 Low valueConsider a stricter shape guard for
OrgAccessUser[].Using
Array.isArrayas the validator accepts any array shape. If a corrupt/legacy entry stores[1, 2, 3]instead ofOrgAccessUser[], it would pass validation and potentially cause runtime issues when consumers access.role, etc.A minimal element check (e.g., verifying the first element has expected fields) would provide stronger protection while remaining lightweight.
♻️ Optional: stricter validator
- Array.isArray + (v: unknown) => Array.isArray(v) && (v.length === 0 || (typeof (v as OrgAccessUser[])[0]?.email === 'string'))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/org-lens-access.service.ts` around lines 82 - 91, The getAccessPrincipals method uses Array.isArray as the shape guard for the withPerUserCache call, which is too permissive and would accept any array shape including corrupt data like [1, 2, 3]. Replace the Array.isArray validator with a stricter shape guard that checks both that the cached value is an array AND that its elements conform to the OrgAccessUser structure (e.g., by verifying the first element has expected fields like email and role). This prevents runtime errors when downstream code tries to access properties on the cached principals.apps/lfx-one/src/server/services/org-contributions.service.ts (1)
459-475: ⚡ Quick winCanonicalize cache-signature inputs to improve hit rate.
Line 460 currently keys on raw
query.searchand raw arrays. Since SQL uses trimmed search and set-likeINsemantics, normalize (trim/case-normalize, de-dup) before signature encoding to avoid avoidable key fragmentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/org-contributions.service.ts` around lines 459 - 475, The contributionsSignature function uses raw query values which can fragment the cache key when logically identical queries differ only in whitespace or array ordering. Modify the function to normalize inputs before signature encoding: trim and case-normalize the query.search field, and deduplicate the query.projects and query.employees arrays (by converting to Set and back to array) before sorting and joining them. This will ensure that semantically identical SQL queries generate the same cache key regardless of input formatting variations.apps/lfx-one/src/server/services/org-lens-training.service.ts (1)
491-516: 💤 Low valueConsider extracting
paramSignatureandisObjectRowArrayto shared utilities.These helpers are duplicated across
org-lens-events.service.ts,org-people-contributors.service.ts, andorg-lens-training.service.ts. While the functions are small and the duplication is harmless, extracting them tovalkey.service.ts(alongsidewithOrgCache) would consolidate the caching utilities in one place.This is minor and can be deferred if preferred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/org-lens-training.service.ts` around lines 491 - 516, The functions paramSignature and isRosterRowArray are duplicated across multiple service files. Extract these utility functions to valkey.service.ts alongside other caching utilities like withOrgCache, then remove the duplicate definitions from each service file and import them from valkey.service.ts instead. This consolidates related utilities in a single location for better maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/src/server/services/org-contributions.service.ts`:
- Around line 477-487: The isContributionsRaw function currently only validates
that each field is an array but does not check the structure of the row objects
within those arrays. Corrupt or legacy cached row objects can pass this
validation and cause errors downstream in mapRepoRow, mapCommitRow, and option
mappers. Add validation logic to check representative row elements from each
array (kpiRows, repoRows, commitRows, projectOptionRows, employeeOptionRows) by
sampling at least one element from each non-empty array and verifying that
required properties exist on those representative row objects before the
function returns true.
In `@apps/lfx-one/src/server/services/organization.service.ts`:
- Around line 927-931: The type guard isAccountContextArray validates only
accountId as a string, but the sort operation at line 923 calls localeCompare on
accountName which requires it to be a string. Enhance the guard in
isAccountContextArray to also validate that accountName is a string property on
each element, ensuring corrupt or legacy cache entries that lack this property
cannot pass the guard and subsequently crash during the sort operation.
- Around line 897-924: The getOrgLensAccountContext method now processes each
account ID individually through the worker pool, which means duplicate IDs in
the input array will produce duplicate rows in the response, whereas the prior
implementation collapsed duplicates naturally through a single IN query.
Deduplicate the accountIds array upfront before building the worker queue to
preserve the prior behavior. This should be done immediately after the initial
length check (after the early return for empty array) by converting the array to
a Set and back to an array, or using another deduplication approach, ensuring
the poolSize and cursor-based iteration operate on the deduplicated list.
---
Nitpick comments:
In `@apps/lfx-one/src/server/services/org-contributions.service.ts`:
- Around line 459-475: The contributionsSignature function uses raw query values
which can fragment the cache key when logically identical queries differ only in
whitespace or array ordering. Modify the function to normalize inputs before
signature encoding: trim and case-normalize the query.search field, and
deduplicate the query.projects and query.employees arrays (by converting to Set
and back to array) before sorting and joining them. This will ensure that
semantically identical SQL queries generate the same cache key regardless of
input formatting variations.
In `@apps/lfx-one/src/server/services/org-lens-access.service.ts`:
- Around line 82-91: The getAccessPrincipals method uses Array.isArray as the
shape guard for the withPerUserCache call, which is too permissive and would
accept any array shape including corrupt data like [1, 2, 3]. Replace the
Array.isArray validator with a stricter shape guard that checks both that the
cached value is an array AND that its elements conform to the OrgAccessUser
structure (e.g., by verifying the first element has expected fields like email
and role). This prevents runtime errors when downstream code tries to access
properties on the cached principals.
In `@apps/lfx-one/src/server/services/org-lens-training.service.ts`:
- Around line 491-516: The functions paramSignature and isRosterRowArray are
duplicated across multiple service files. Extract these utility functions to
valkey.service.ts alongside other caching utilities like withOrgCache, then
remove the duplicate definitions from each service file and import them from
valkey.service.ts instead. This consolidates related utilities in a single
location for better maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1b82ca0-747a-4976-bce0-5f536d940f00
📒 Files selected for processing (18)
apps/lfx-one/src/server/services/org-contributions.service.tsapps/lfx-one/src/server/services/org-involvement.service.tsapps/lfx-one/src/server/services/org-lens-access.service.tsapps/lfx-one/src/server/services/org-lens-board-committee.service.tsapps/lfx-one/src/server/services/org-lens-events.service.tsapps/lfx-one/src/server/services/org-lens-foundations.service.tsapps/lfx-one/src/server/services/org-lens-memberships.service.tsapps/lfx-one/src/server/services/org-lens-people.service.tsapps/lfx-one/src/server/services/org-lens-training.service.tsapps/lfx-one/src/server/services/org-people-contributors.service.tsapps/lfx-one/src/server/services/org-people-directory.service.tsapps/lfx-one/src/server/services/org-people-event-attendees.service.tsapps/lfx-one/src/server/services/org-people-key-contacts.service.tsapps/lfx-one/src/server/services/org-people-trainees.service.tsapps/lfx-one/src/server/services/organization.service.tsapps/lfx-one/src/server/services/valkey.service.tspackages/shared/src/constants/org-selector.constants.tspackages/shared/src/constants/valkey-cache.constants.ts
Address review feedback on the Org Lens cache rollout: - buildPerUserOrgKey now fails closed when the org uid isn't filter-safe, so it can't corrupt the ':'-delimited key (matches buildOrgCacheKey). - getAccessPrincipals and fetchAllOrgSeats accept guards require non-null object elements instead of bare Array.isArray, so corrupt entries miss. - isContributionsRaw samples a representative row per array (non-null object carrying its contract key) before accepting a cache hit. - getOrgLensAccountContext dedupes account ids before fan-out to preserve the prior IN(...) collapse, and isAccountContextArray also validates accountName to protect the localeCompare sort. Signed-off-by: Luis Mori Guerra <luismorith@gmail.com>
Review round recap — cache hardening (commit a44e55f)Addressed all 6 open automated-review threads (Copilot + CodeRabbit), all in the original cache scope. No out-of-scope feedback arrived.
Cache invariants preserved: fail-soft/fail-closed, per-org vs per-user keying, successful-results-only writes, TTLs, the access write-refresh bypass, and the bounded account-context worker pool. Public method signatures unchanged. Quality gate: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/server/services/org-lens-access.service.ts (1)
27-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate principal shape, not just object-ness.
Line 36 still accepts
{}as anOrgAccessUser, so a malformed cached principals entry can be treated as a hit and break later field reads. Reuse the same nested guard forisAccessListResponse.usersas well.Proposed guard tightening
function isAccessListResponse(value: unknown): boolean { const v = value as Partial<OrgAccessListResponse> | null; return ( - !!v && typeof v.orgUid === 'string' && Array.isArray(v.users) && typeof v.summary === 'object' && v.summary !== null && typeof v.canManage === 'boolean' + !!v && + typeof v.orgUid === 'string' && + isPrincipalArray(v.users) && + isAccessSummary(v.summary) && + typeof v.canManage === 'boolean' ); } /** Rejects a corrupt/legacy principals entry whose elements aren't non-null objects (degrades to a miss before the directory merge reads user fields). */ function isPrincipalArray(value: unknown): boolean { - return Array.isArray(value) && value.every((el) => el !== null && typeof el === 'object' && !Array.isArray(el)); + return Array.isArray(value) && value.every(isOrgAccessUser); +} + +function isOrgAccessUser(value: unknown): value is OrgAccessUser { + const user = value as Partial<OrgAccessUser> | null; + return ( + !!user && + typeof user === 'object' && + !Array.isArray(user) && + typeof user.email === 'string' && + typeof user.name === 'string' && + typeof user.initials === 'string' && + (user.avatarUrl === null || typeof user.avatarUrl === 'string') && + (user.jobTitle === null || typeof user.jobTitle === 'string') && + (user.role === 'admin' || user.role === 'viewer') && + typeof user.inviteStatus === 'string' && + typeof user.isPending === 'boolean' + ); +} + +function isAccessSummary(value: unknown): value is OrgAccessSummary { + const summary = value as Partial<OrgAccessSummary> | null; + return ( + !!summary && + typeof summary === 'object' && + !Array.isArray(summary) && + typeof summary.totalUsers === 'number' && + typeof summary.administrators === 'number' && + typeof summary.viewers === 'number' + ); }Also applies to: 95-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/org-lens-access.service.ts` around lines 27 - 37, The isPrincipalArray function currently validates that array elements are non-null objects, but this is insufficient since empty objects `{}` will pass the check while lacking required fields, causing failures during later field access. Enhance the validation in isPrincipalArray to validate the actual shape of principal objects by reusing the same nested guard logic that isAccessListResponse uses for validating users array elements—ensuring that each principal element has the expected properties and structure rather than just checking it is an object. Apply the same validation tightening at the other location mentioned in the comment (line 95) to maintain consistency across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/src/server/services/org-contributions.service.ts`:
- Around line 481-496: The isRowArray function currently only validates the
first element of an array, allowing corrupt rows deeper in the array to pass
validation and crash during later processing. Modify the isRowArray function to
check every element in the array using Array.every() instead of just checking
the first element, ensuring all rows match the expected contract. Additionally,
for the kpiRows validation call (the first isRowArray invocation in the guard),
require validation of specific KPI contract fields to ensure consistent cache
miss behavior when encountering corrupt or legacy row objects.
---
Outside diff comments:
In `@apps/lfx-one/src/server/services/org-lens-access.service.ts`:
- Around line 27-37: The isPrincipalArray function currently validates that
array elements are non-null objects, but this is insufficient since empty
objects `{}` will pass the check while lacking required fields, causing failures
during later field access. Enhance the validation in isPrincipalArray to
validate the actual shape of principal objects by reusing the same nested guard
logic that isAccessListResponse uses for validating users array
elements—ensuring that each principal element has the expected properties and
structure rather than just checking it is an object. Apply the same validation
tightening at the other location mentioned in the comment (line 95) to maintain
consistency across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9370d340-e7d5-40b9-b16f-a73e597cec83
📒 Files selected for processing (5)
apps/lfx-one/src/server/services/org-contributions.service.tsapps/lfx-one/src/server/services/org-lens-access.service.tsapps/lfx-one/src/server/services/org-lens-board-committee.service.tsapps/lfx-one/src/server/services/organization.service.tsapps/lfx-one/src/server/services/valkey.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/lfx-one/src/server/services/org-lens-board-committee.service.ts
- apps/lfx-one/src/server/services/organization.service.ts
- apps/lfx-one/src/server/services/valkey.service.ts
LFXV2-2356) isRowArray now checks every element via every() instead of sampling the first, and the kpiRows guard requires its full KPI contract (PROJECTS_WITH_ACTIVITY, REPOSITORIES, COMMITS) so a single corrupt/legacy row degrades the entry to a cache miss consistently. Signed-off-by: Luis Mori Guerra <luismorith@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR thread resolution — round 2 (follow-up on
|
…che hit (LFXV2-2356) Tighten the Valkey accept-guards for the three Org Lens reads that serve a cached value straight to the client: validate every row/assignment/user element and the numeric stat/summary fields against the wire contract, so a corrupt or legacy entry degrades to a miss (direct fetch) instead of crashing downstream on sources spreading, name.localeCompare, or invalid stat math. Preserves the fail-soft cache contract and public signatures. Signed-off-by: Luis Mori Guerra <luismorith@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Review round 3 — recapRe-fetched the unresolved thread set via GraphQL ( Fixed (commit
Behavior is unchanged on the happy path; only corrupt/legacy cache entries now degrade to a miss (direct fetch), preserving the fail-soft cache contract and all public signatures. Threads left open: none. Quality gate: Self-review ( Pushed: |
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Automated review + code-standards-enforcer audit (report-only). 4 commits by Luis Mori Guerra — all signed off (Signed-off-by: Luis Mori Guerra <luismorith@gmail.com>) ✅. Cursor co-author in 3 of 4 commits — not a violation ✅. No Claude co-author. ✅
🔒 Secrets / critical-constants check
No credentials, tokens, or secrets introduced. All changes are cache-infrastructure wiring: namespace strings, TTL integers, and shape-guard functions. Snowflake queries remain parameterised throughout. ✅
Overview
Routes every uncached Org Lens SSR read through the shared Valkey CachePort, using two keying models:
- Per-org keys (
withOrgCache) for caller-invariant Snowflake reads (WHERE ACCOUNT_ID = ?), 1 h TTL: overview foundations + involvement, memberships, training, contributions, events, and the People Snowflake tabs. - Per-user keys (
withPerUserCache) for FGA/token-filtered reads, 30 s TTL: org-wide committee-seat drain, key contacts, access-list, and the merged people directory.
Also migrates OrgPeopleDirectoryService from a per-instance in-memory Map (limited to a single Express worker) onto the shared Valkey cache (cross-instance coherence). The account-context fan-out is bounded by a 8-worker cursor pool to protect the Snowflake connection pool on a cold start. 18 files, ~1 060 additions.
Correctness — verified ✅
Fail-soft / fail-closed contract preserved:
buildOrgCacheKeyreturnsnullwhenaccountIdis not filter-safe;buildPerUserOrgKeyreturnsnullwhenusernameORorgUidis not filter-safe — both map to a direct-fetch bypass invalkeyService.withCache(null, ...). ✅- Cache faults (Redis error) degrade to the direct fetcher (handled inside
ValkeyService.withCache). ✅
Per-org vs per-user segregation correct throughout:
withOrgCacheused only on pure Snowflake reads that produce identical results for every caller of the sameaccountId. ✅withPerUserCacheused on all FGA/token-gated reads (fetchAllOrgSeats,getKeyContacts,listAccessUsers,getAccessPrincipals,getLive). The key includesgetEffectiveUsername(req)so one caller's permissioned roster cannot be served to another within the TTL. ✅
people-detail:${personKey} cache bypass:
isFilterSafeIdentifier(personKey)checked before constructing the cache key; unsafe person keys fall through torunEmployeeDetailFetchdirectly, preventing:injection into the:-delimited key namespace. ✅
getOrgLensAccountContext cursor-pool fan-out:
cursor++is a synchronous read-then-increment — single-threaded JS ensures no two workers can claim the same index across anawaitboundary. Array pre-allocation (perAccount: ...[] = new Array(uniqueAccountIds.length)) avoids holes when workers finish out-of-order. Deduplication (new Set(accountIds)) preserves the priorIN (...)collapse. ✅
fetchPagedCourseRows signature narrowed:
- Removed the
mapRowcallback; now returns raw{ rows: TRow[], total }. Mapping happens after the cache read, so only serialisable raw rows enter Valkey — correct pattern. ✅
OrgRosterEmployeesResult local interface deleted:
- Was only used inside the training service; replaced cleanly by
OrgRosterEmployeeRow[]plus a standalonemapRosterRowsfunction. No shared-interface regression. ✅
Shape-guard depth is asymmetric by design:
- Client-facing merged responses (
isAllEmployeesResponse,isKeyContactsResponse,isKeyContactsStats,isAccountContextArray) validate every required field and its type. ✅ - Intermediate raw-row arrays (
isRosterRowArray,isObjectRowArray,isTraineesRaw,isEventAttendeesRaw) validate only array-of-objects shape; field-level types are enforced by downstream?? ''/?? 0/?? nullguards in the mapping layer. Acceptable because raw rows are never replayed to the client without transformation.
No cross-tenant key collision:
- Per-org namespace:
{prefix}:org-lens-sf:v1:{accountId}:{subResource}—accountIdis validated filter-safe before inclusion. ✅ - Per-user namespace:
{prefix}:{namespace}:{username}:{orgUid}— bothusernameandorgUidvalidated filter-safe. ✅ - Schema-version suffix (
:v1) on every namespace constant ensures a format change invalidates the entire keyspace without requiring an explicit flush. ✅
Code Standards Audit (~/LFX/code-enforcer-agent.md)
🔴 Critical: none.
🟡 Warning:
- PR body contains
## Test planwith mixed ✅ /[ ]items — violates the CLAUDE.md "No test plans in PRs" rule.
🔵 Info:
- Paged training/certification results include pagination params in the cache key (
paramSignature([searchQuery, level, pageSize, offset, sortField, sortOrder])). Each unique filter combination produces a separate 1 h entry. For orgs with varied pagination patterns and large employee counts, the distinct key count per org could grow; Redis TTL eviction handles this, but there is no explicit per-org key cap. Worth monitoring post-deploy withSCAN … MATCH {prefix}:org-lens-sf:v1:{accountId}:certifications:*. people-detail:{personKey}cached at 1 h. For employees with active governance roles or recent commits, the chevron-expand detail could be up to 1 h stale following a role change. Acceptable given Snowflake's daily dbt batch cadence; just worth documenting in the product runbook.ORG_SEATS_NAMESPACE: 'org-seats:v1'is declared invalkey-cache.constants.tsbut not consumed in this PR (thefetchAllOrgSeatscache inorg-lens-board-committee.service.tsreads from the previously-introduced summary from session context). Either the constant is used in a sibling PR yet to land, or it is pre-declared for the next slice. Worth a follow-up cleanup if unused after merge.
✅ Correctly followed: ORG_LENS_ACCOUNT_CONTEXT_FETCH_CONCURRENCY added to packages/shared/src/constants/org-selector.constants.ts (correct location); all new namespace and TTL constants in packages/shared/src/constants/valkey-cache.constants.ts (correct location); no new interfaces introduced (all existing); no new files → MIT header rule N/A; no effect() calls; no template changes; all commits signed-off; Cursor co-author present (not Claude → not a violation).
Test coverage
No automated tests added. The new helpers (buildOrgCacheKey, buildPerUserOrgKey) and the deep shape guards (isAllEmployeesResponse, isKeyContactsResponse, isAccountContextArray) are strong candidates for unit tests: the key-builder null-returns on unsafe input and the field-level guard branches are both security-relevant and currently exercised only in production paths.
Verdict: PASS (one 🟡 process warning; no code defects)
audigregorie
left a comment
audigregorie
left a comment
There was a problem hiding this comment.
Code Review Summary
Large, carefully-engineered caching extension across the Org Lens server read surface. The fail-soft / fail-closed split is sound, key-safety is enforced (isFilterSafeIdentifier / isFilterSafeUsername + base64url signatures), shape-guards reject corrupt entries, and the account-context fan-out is bounded by a worker pool. Verified the contributions cache signature covers all 11 OrgContributionsQuery fields (no cache-correctness gap) and there are no cache-key collisions across the per-org namespace. No Critical issues.
Verdict: Solid. One Major to confirm (read-after-write staleness on the now-cached access list, with no invalidation on write) plus three Minor notes. Rating 4/5.
What's done well
- Key safety is rigorous: null-key fail-closed on unsafe identifiers, base64url signatures keep
:-delimited keys uncorruptible, andgetEffectiveUsernamescopes per-user keys to the impersonated principal so impersonation can't leak across callers. - Per-org (1h, shared) vs per-user (30s, isolated) split is correctly applied — the seat drain, key contacts, and access list are all token-dependent and keyed per user.
- The account-context refactor (single
IN (...)→ per-account cached reads behind a bounded worker pool, with upfront dedupe) cleanly improves hit rate and protects the Snowflake connection pool.
| return cached.value; | ||
| } | ||
| const username = getEffectiveUsername(req) ?? ''; | ||
| return withPerUserCache( |
There was a problem hiding this comment.
Minor — no server-side memoization when Valkey is unavailable
This service previously kept an unconditional in-memory 30s memo (DIRECTORY_CACHE_*). It now relies entirely on Valkey, so when VALKEY_URL is unset (local dev, and any deploy without Valkey) withPerUserCache degrades to a direct fetch on every call — the heaviest Org Lens operation (4-upstream merge: Snowflake + org-wide seat drain + query-service + member-service) then runs unmemoized per request. Frontend shareReplay only collapses concurrent requests, not sequential ones within the window.
Likely intended per the spec-026 cross-instance direction, but worth confirming the no-Valkey performance hit is acceptable for all targets (esp. local dev).
There was a problem hiding this comment.
Leaving this open as a deliberate-tradeoff confirmation rather than a code change. Dropping the unconditional in-memory memo is intentional with this cache migration — re-adding a process-local memo would partially reintroduce the per-instance staleness/cross-instance inconsistency the shared cache is meant to remove, and contradicts moving the directory off its in-memory Map. When VALKEY_URL is unset (local dev / any Valkey-less deploy) the directory degrades to a direct fetch per request; frontend shareReplay still collapses concurrent requests. Flagging for product/perf confirmation that the no-Valkey hit is acceptable for those targets; happy to revisit if a short-TTL local fallback is wanted.
…guards
- org-lens-access: drop the acting caller's :list/:principals per-user cache
entries after invite/changeRole/removeUser so their own next read reflects
the mutation instead of serving the pre-write list for up to the TTL; add a
best-effort ValkeyService.del + invalidatePerUserCache helper (fail-soft,
no-op on null key/disabled cache). Write-refresh bypass is preserved.
- org-lens-events: replace the shallow object-shape cache guard with contract-
key guards (EVENT_ID for the list, CONTACT_ID for attendees/speakers) so a
poisoned [{}] entry degrades to a miss instead of surfacing undefined ids.
- organization: harden the account-context sort against a null accountName.
Signed-off-by: Luis Mori Guerra <luismorith@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Review-thread round 4 — recapRe-fetched the unresolved set via GraphQL ( Fixed
Left open (non-actionable this round)
Quality gate
|
🧹 Deployment RemovedThe deployment for PR #970 has been removed. |
|
|
||
| const total = result.rows.length > 0 ? result.rows[0].TOTAL_RECORDS : 0; | ||
| const data = result.rows.map((row) => this.mapRowToOrgEvent(row)); | ||
| const total = rows.length > 0 ? rows[0].TOTAL_RECORDS : 0; |
| // Rejects a corrupt/legacy entry (degrade to a miss) before it reaches shapeResponse consumers. | ||
| private static isFoundationsResponse(value: unknown): value is OrgLensFoundationsAndProjectsResponse { | ||
| return ( | ||
| value !== null && | ||
| typeof value === 'object' && | ||
| !Array.isArray(value) && | ||
| typeof (value as OrgLensFoundationsAndProjectsResponse).accountId === 'string' && | ||
| Array.isArray((value as OrgLensFoundationsAndProjectsResponse).rows) | ||
| ); | ||
| } |
| function isTraineesRaw(value: unknown): boolean { | ||
| const v = value as { traineeRows?: unknown; detailRows?: unknown; foundationRows?: unknown; courseRows?: unknown } | null; | ||
| return !!v && Array.isArray(v.traineeRows) && Array.isArray(v.detailRows) && Array.isArray(v.foundationRows) && Array.isArray(v.courseRows); | ||
| } |
| function isEventAttendeesRaw(value: unknown): boolean { | ||
| const v = value as { attendeeRows?: unknown; detailRows?: unknown; foundationRows?: unknown; eventRows?: unknown } | null; | ||
| return !!v && Array.isArray(v.attendeeRows) && Array.isArray(v.detailRows) && Array.isArray(v.foundationRows) && Array.isArray(v.eventRows); | ||
| } |
Summary
Extends the existing shared Valkey cache (the spec-026
CachePort/ValkeyServicefoundation) across the entire Org Lens server read surface that was previously uncached, using two keying models:WHERE ACCOUNT_ID = ?), refreshed on the daily dbt batch (1h TTL): Overview foundations + the 6 involvement metrics + account-context, Memberships (active/expired/discover), Training, Contributions, Events, and the People Snowflake tabs (All Employees, Contributors, Trainees, Event Attendees).Also migrates the org people directory off its per-instance in-memory
Maponto the shared cache, finishing the cross-instance-coherence migration started in spec 026.Design / safety
Test plan
yarn lint:check,yarn check-types(forced tsc on app + shared),yarn format:check— all passui-code-standard-enforcer+ui-code-reviewpasses (fixes applied)VALKEY_URLunset, all pages render via direct fetchVALKEY_URLpoints at ElastiCache)Made with Cursor