fix: invalidation issues - Don't merge this till we merge rest of the deploy PRs#4000
fix: invalidation issues - Don't merge this till we merge rest of the deploy PRs#4000ogzhanolguncu wants to merge 1 commit intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors the collections layer from static singletons to factory-created instances, updates reset/cleanup semantics, adds unique indexes, and standardizes error handling. Sidebar hooks now pass dependency arrays to useLiveQuery. Workspace creation invalidates memberships and resets collections. Team switcher adds a guard to avoid redundant workspace changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as Dashboard UI
participant TRPC as trpc.createWorkspace
participant Utils as trpcUtils.user.listMemberships
participant Coll as collections.reset()
U->>UI: Submit "Create workspace"
UI->>TRPC: mutateAsync(payload)
TRPC-->>UI: onSuccess(newWorkspace)
UI->>Utils: invalidate()
UI->>Coll: reset()
activate Coll
note over Coll: Cleanup per-project and global collections<br/>Re-instantiate global collections from factories<br/>Preload collections
Coll-->>UI: resolved
deactivate Coll
UI-->>U: UI reflects new memberships and data
sequenceDiagram
autonumber
participant Reset as reset()
participant Clean as cleanupAll()
participant Global as GLOBAL_COLLECTION_FACTORIES
participant Public as collection (public object)
Reset->>Clean: run cleanup helper
Clean-->>Reset: per-project & global cleaned
Reset->>Global: instantiate each factory
Global-->>Reset: new instances
Reset->>Public: replace collection entries
Reset-->>Reset: sequentially preload all
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx (1)
29-31: Fix potential crash when data is undefined during initial loaddata can be undefined while loading (projects hook guards for this). Accessing data.length would throw.
Apply this diff to guard consistently:
- if (data.length === 0) { - return []; - } + if (!data?.length) { + return []; + }Also applies to: 33-39
🧹 Nitpick comments (9)
apps/dashboard/components/navigation/sidebar/team-switcher.tsx (2)
86-93: Guard against concurrent switches and handle rejected promisesDouble‑clicks can fire multiple mutateAsync calls; also, unhandled rejections can spam the console. Add a loading guard and wrap in try/catch (onError already toasts).
- const handleWorkspaceSwitch = async (targetOrgId: string) => { + const handleWorkspaceSwitch = async (targetOrgId: string) => { // Prevent switch if already on the target workspace if (targetOrgId === currentOrgMembership?.organization.id) { return; } - - await changeWorkspace.mutateAsync(targetOrgId); + if (changeWorkspace.isLoading) return; + try { + await changeWorkspace.mutateAsync(targetOrgId); + } catch { + // handled by onError + } };
153-172: Disable the current/active item and while switching (UX + safety)Prevent clicks on the active workspace and during an in‑flight switch. If DropdownMenuItem supports disabled, wire it up.
- <DropdownMenuItem + <DropdownMenuItem key={membership.id} className="flex items-center justify-between" - onClick={() => handleWorkspaceSwitch(membership.organization.id)} + disabled={ + membership.organization.id === currentOrgMembership?.organization.id || + changeWorkspace.isLoading + } + onClick={() => handleWorkspaceSwitch(membership.organization.id)} >apps/dashboard/lib/collections/deploy/projects.ts (1)
41-67: Broaden error map to common tRPC codes (auth/rate limit)Add UNAUTHORIZED and TOO_MANY_REQUESTS for clearer UX. Optional: centralize this map for reuse across collections.
FORBIDDEN: { message: "Permission Denied", description: "You don't have permission to create projects in this workspace.", }, + UNAUTHORIZED: { + message: "Sign-in required", + description: "Your session expired. Please sign in and try again.", + }, BAD_REQUEST: { message: "Invalid Configuration", description: "Please check your project settings.", }, + TOO_MANY_REQUESTS: { + message: "Rate limit exceeded", + description: "Please wait a moment and try again.", + },apps/dashboard/lib/collections/ratelimit/namespaces.ts (2)
26-33: Validate and trim name on createAvoid manual null/empty checks; trim and enforce bounds with zod so whitespace‑only names don’t slip through and uniqueness behaves predictably.
- onInsert: async ({ transaction }) => { - const { changes: newNamespace } = transaction.mutations[0]; - if (!newNamespace.name) { - throw new Error("Namespace name is required"); - } - const mutation = trpcClient.ratelimit.namespace.create.mutate({ - name: newNamespace.name, - }); + onInsert: async ({ transaction }) => { + const { changes: newNamespace } = transaction.mutations[0]; + const { name } = z.object({ name: z.string().trim().min(1).max(50) }).parse({ + name: newNamespace.name, + }); + const mutation = trpcClient.ratelimit.namespace.create.mutate({ name });
47-51: Also sanitize on updateTrim and validate on rename to keep invariants consistent.
- const mutation = trpcClient.ratelimit.namespace.update.name.mutate({ - namespaceId: original.id, - name: modified.name, - }); + const { name } = z.object({ name: z.string().trim().min(1).max(50) }).parse({ + name: modified.name, + }); + const mutation = trpcClient.ratelimit.namespace.update.name.mutate({ + namespaceId: original.id, + name, + });apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx (1)
41-46: Simplify “Requests” active state logicThe OR condition duplicates isExactlyRatelimitRoot.
- active: isExactlyRatelimitRoot || (currentNamespaceActive && !segments.at(2)), + active: currentNamespaceActive && segments.length === 2,apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (1)
36-45: Guard against duplicate child items if baseNavItems already contains dynamic entriesIf callers ever pass previously “enhanced” items back as baseNavItems, appending again will duplicate children. Consider replacing only the dynamic portion or ensuring callers pass a stable, static base.
apps/dashboard/lib/collections/index.ts (2)
101-115: Serialize reset to avoid overlapping cleanups and instance churnConcurrent calls to reset() could interleave cleanup/assign/preload and produce stale subscriptions. Serialize resets with an in‑flight promise.
Apply within reset():
-export async function reset() { - // This is GC cleanup only useful for better memory management - await collectionManager.cleanupAll(); - // Without these components still subscribed to old collections, so create new instances for each reset. Mostly used when switching workspaces - Object.assign( - collection, - Object.fromEntries( - Object.entries(GLOBAL_COLLECTION_FACTORIES).map(([key, factory]) => [key, factory()]), - ), - ); - // Preload all collections, please keep this sequential. Otherwise UI acts weird. react-query already takes care of batching. - for (const c of Object.values(collection)) { - await c.preload(); - } -} +export async function reset() { + if (resetInFlight) return resetInFlight; + resetInFlight = (async () => { + await collectionManager.cleanupAll(); + Object.assign( + collection, + Object.fromEntries( + Object.entries(GLOBAL_COLLECTION_FACTORIES).map(([key, factory]) => [key, factory()]), + ), + ); + for (const c of Object.values(collection)) { + await c.preload(); + } + })().finally(() => { + resetInFlight = null; + }); + return resetInFlight; +}Add near module top:
let resetInFlight: Promise<void> | null = null;
73-87: Minor: clarify cleanup sequencing and intentProject cleanups start immediately (promises created), globals are cleaned sequentially, then you await projectPromises. Add a short comment so future refactors don’t inadvertently flip the order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/dashboard/app/new/hooks/use-workspace-step.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/team-switcher.tsx(2 hunks)apps/dashboard/lib/collections/deploy/projects.ts(1 hunks)apps/dashboard/lib/collections/index.ts(3 hunks)apps/dashboard/lib/collections/ratelimit/namespaces.ts(1 hunks)apps/dashboard/lib/collections/ratelimit/overrides.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-25T13:46:08.303Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:1-1
Timestamp: 2025-08-25T13:46:08.303Z
Learning: The NamespaceListDateTime component in apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx is intentionally designed to use the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than the namespace list hook, as clarified by ogzhanolguncu. This coupling is by design, not an architectural issue.
Applied to files:
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx
📚 Learning: 2025-08-25T13:46:34.441Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx:4-4
Timestamp: 2025-08-25T13:46:34.441Z
Learning: The namespace list refresh component (apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx) intentionally uses the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than a namespace-specific hook. This cross-coupling between namespace list components and overview hooks is an architectural design decision.
Applied to files:
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx
📚 Learning: 2025-08-25T12:56:59.310Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/lib/trpc/routers/ratelimit/query-namespaces/index.ts:59-66
Timestamp: 2025-08-25T12:56:59.310Z
Learning: In the ratelimit namespace query system (apps/dashboard/lib/trpc/routers/ratelimit/query-namespaces/index.ts), the nameQuery filter is designed as an array for future extensibility to support multiple filters, but currently only the first filter (index 0) is processed. This is intentional future-proofing.
Applied to files:
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsxapps/dashboard/lib/collections/ratelimit/namespaces.ts
🧬 Code graph analysis (7)
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (1)
apps/dashboard/lib/collections/index.ts (1)
collection(93-99)
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx (1)
apps/dashboard/lib/collections/index.ts (1)
collection(93-99)
apps/dashboard/app/new/hooks/use-workspace-step.tsx (1)
apps/dashboard/lib/collections/index.ts (1)
reset(101-115)
apps/dashboard/lib/collections/deploy/projects.ts (1)
apps/dashboard/lib/collections/client.ts (1)
trpcClient(12-25)
apps/dashboard/lib/collections/index.ts (6)
apps/dashboard/lib/collections/deploy/environments.ts (1)
createEnvironmentsCollection(15-30)apps/dashboard/lib/collections/deploy/domains.ts (1)
createDomainsCollection(16-31)apps/dashboard/lib/collections/deploy/deployments.ts (1)
createDeploymentsCollection(41-56)apps/dashboard/lib/collections/deploy/projects.ts (1)
createProjectsCollection(69-111)apps/dashboard/lib/collections/ratelimit/namespaces.ts (1)
createRatelimitNamespacesCollection(15-82)apps/dashboard/lib/collections/ratelimit/overrides.ts (1)
createRatelimitOverridesCollection(18-81)
apps/dashboard/lib/collections/ratelimit/overrides.ts (3)
apps/dashboard/lib/collections/index.ts (1)
collection(93-99)apps/dashboard/lib/collections/client.ts (1)
trpcClient(12-25)internal/ui/src/components/toaster.tsx (1)
toast(29-29)
apps/dashboard/lib/collections/ratelimit/namespaces.ts (3)
apps/dashboard/lib/collections/index.ts (1)
collection(93-99)apps/dashboard/lib/collections/client.ts (1)
trpcClient(12-25)internal/ui/src/components/toaster.tsx (1)
toast(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (8)
apps/dashboard/lib/collections/ratelimit/overrides.ts (1)
73-78: Confirm uniqueness semantics (case sensitivity, server parity)The in‑memory unique index enforces client‑side uniqueness for (namespaceId, identifier). Confirm the server enforces the same rule (and the same case‑sensitivity) to avoid client/server divergence.
apps/dashboard/lib/collections/deploy/projects.ts (1)
69-111: Factory migration looks goodInstantiation, query options, parsing, and toast flow are consistent with the new collections pattern.
apps/dashboard/lib/collections/ratelimit/namespaces.ts (1)
74-79: Uniqueness expectationsIf names should be unique case‑insensitively, normalize (e.g., toLowerCase) before insert/update or enforce at the server. Otherwise “Foo” and “foo” will be distinct locally.
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-ratelimit-navigation.tsx (2)
18-25: Good: adding deps to useLiveQuery matches the reset/invalidations planUsing [collection.ratelimitNamespaces] ensures re‑runs after workspace reset. This aligns with the factory-based collections.
Please confirm other hooks subscribe similarly so they re-run after reset.
21-23: Verify ordering by id is semantically correctDescending by namespace.id assumes ids are time‑sortable (e.g., ULID/KSUID). If not, prefer createdAt.
apps/dashboard/components/navigation/sidebar/app-sidebar/hooks/use-projects-navigation.tsx (2)
11-15: Good: dependency array ensures reruns after resetPassing [collection.projects] addresses the invalidation/rerender gap on workspace changes.
12-14: Confirm id-based ordering is intendedOrdering by project.id desc relies on ids being monotonic. If not guaranteed, switch to createdAt.
apps/dashboard/lib/collections/index.ts (1)
24-28: Solid move to factory-built global collections with in-place mutation on resetFactory map + in-place Object.assign preserves the collection object identity while swapping instances—good for hooks depending on property identity.
Also applies to: 92-99
ogzhanolguncu
changed the title
|
Closing this in favor of @perkinsjr 's PR. Note for my future-self: We can use the factory structure in the future. It removes most of the boilerplate and makes it easy to add new collections |
1 participant
What does this PR do?
This PR:
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit