fix(site): tighten interface design for various frontend utility functions #18894
Conversation
Parkreiner
changed the title
| const getTemplatesByOrganizationQueryKey = ( | ||
| organization: string, | ||
| options?: GetTemplatesOptions, | ||
| ) => [organization, "templates", options?.deprecated]; | ||
|
|
||
| const templatesByOrganization = ( | ||
| organization: string, | ||
| options: GetTemplatesOptions = {}, | ||
| ) => { | ||
| return { | ||
| queryKey: getTemplatesByOrganizationQueryKey(organization, options), | ||
| queryFn: () => API.getTemplatesByOrganization(organization, options), | ||
| }; | ||
| }; |
| @@ -121,9 +106,11 @@ export const templateExamples = () => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const templateVersionRoot: string = "templateVersion"; | |||
There was a problem hiding this comment.
This ends up being more important for the upcoming PR, but I really, really think that we need to find the time to formalize our entire data layer, and be way more deliberate with our React Query caching
I fully expect our caching to create a super, super hard-to-recreate bug sometime in the future, because it's a bunch of semi-structured global mutable state right now
There was a problem hiding this comment.
Also added an explicit string type annotation so that TypeScript doesn't over-infer the value and treat it as a literal. We shouldn't know what exact combination of characters it is – just what purpose the string serves in the greater codebase as an opaque value
There was a problem hiding this comment.
I am very interested to hear you elaborate on all of this because I am not sure I follow. 😅
| searchParams: URLSearchParams; | ||
| onSearchParamsChange: (newParams: URLSearchParams) => void; |
There was a problem hiding this comment.
Decoupled type signature from React Router. Splitting this up gives us a lot more freedom, which comes up later in this PR
| export type UseFilterResult = Readonly<{ | ||
| query: string; | ||
| values: FilterValues; | ||
| used: boolean; | ||
| update: (newValues: string | FilterValues) => void; | ||
| debounceUpdate: (newValues: string | FilterValues) => void; | ||
| cancelDebounce: () => void; | ||
| }>; |
There was a problem hiding this comment.
I feel like the previous type was super, super risky, because it was derived from a function that didn't have much type-safety
With this, the contract is way more explicit and simple, and there's basically no chance of the TypeScript LSP getting confused and dropping properties for tooltips
| type UsePaginationResult = Readonly<{ | ||
| page: number; | ||
| limit: number; | ||
| offset: number; | ||
| goToPage: (page: number) => void; | ||
| }>; |
There was a problem hiding this comment.
We were missing a return type altogether before
| const abortNavigation = | ||
| page === newPage || !Number.isFinite(page) || !Number.isInteger(page); | ||
| if (abortNavigation) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Another render optimization
There was a problem hiding this comment.
What is this render optimization? Not re-rendering the page if it's the same page?
There was a problem hiding this comment.
So there's two parts:
- The basic input validation from the
Numberstatic methods - The equality check
I could maybe add a comment, but React only has Object.is for change detection, so in general, a lot of render optimizations boil down to "only dispatch a new value (especially a non-primitive value) when you know the contents have changed". Most of the time, you don't have to worry about over-dispatching primitive values (as long as they're done outside a render, React will detect that nothing changed and do a no-op for the state update). But because JS is wacky, there are edge cases with Object.is and numbers, which is why there's the extra === check
There was a problem hiding this comment.
wdym edge cases with Object.is? the only "edge case" I'm aware of is Object.is(0, -0) === false
| const [suggestedName, setSuggestedName] = useState(() => | ||
| generateWorkspaceName(), | ||
| ); | ||
| const [suggestedName, setSuggestedName] = useState(generateWorkspaceName); |
There was a problem hiding this comment.
The extra wrapper function was redundant
| (touched, parameter) => { | ||
| if (autofillByName[parameter.name] !== undefined) { | ||
| touched[parameter.name] = true; | ||
| } | ||
| return touched; | ||
| }, | ||
| {} as Record<string, boolean>, | ||
| {}, |
There was a problem hiding this comment.
Removed type assertion to remove risk of accidentally silencing the compiler
| const [checkedWorkspaceIds, setCheckedWorkspaceIds] = useState( | ||
| new Set<string>(), | ||
| ); |
There was a problem hiding this comment.
This is part of a fix for a super nasty bug that I ran into when updating the batch-update UI
Because we were storing an entire copy of the original workspaces each time we checked off something, that also meant that as the query cache updated its data and triggered re-renders, anything that used the checked workspaces would receive the stale versions of the workspaces
| "delete" | "update" | null | ||
| >(null); | ||
| const [urlSearchParams] = searchParamsResult; | ||
| const [activeBatchAction, setActiveBatchAction] = useState<BatchAction>(); |
There was a problem hiding this comment.
Slightly re-jiggered confirmingBatchAction
| @@ -142,7 +164,18 @@ const WorkspacesPage: FC = () => { | |||
| canCreateTemplate={permissions.createTemplates} | |||
| canChangeVersions={permissions.updateTemplates} | |||
| checkedWorkspaces={checkedWorkspaces} | |||
| onCheckChange={setCheckedWorkspaces} | |||
| onCheckChange={(newWorkspaces) => { | |||
| setCheckedWorkspaceIds((current) => { | |||
There was a problem hiding this comment.
Another render optimization
| onBatchDeleteTransition={() => setActiveBatchAction("delete")} | ||
| onBatchStartTransition={() => batchActions.start(checkedWorkspaces)} | ||
| onBatchStopTransition={() => batchActions.stop(checkedWorkspaces)} | ||
| onBatchUpdateTransition={() => { |
There was a problem hiding this comment.
This change becomes a little more relevant for the other PR, but even if this were the only PR, this would still help the update UI avoid stale state
| type UseBatchActionsResult = Readonly<{ | ||
| isProcessing: boolean; | ||
| start: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>; | ||
| stop: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>; | ||
| delete: (workspaces: readonly Workspace[]) => Promise<WorkspaceBuild[]>; | ||
| updateTemplateVersions: ( | ||
| payload: UpdateAllPayload, | ||
| ) => Promise<WorkspaceBuild[]>; | ||
| favorite: (payload: readonly Workspace[]) => Promise<void>; | ||
| unfavorite: (payload: readonly Workspace[]) => Promise<void>; | ||
| }>; |
There was a problem hiding this comment.
Added explicit return type, which was super important here, because the function's return type was getting inferred as something super complicated and nasty from React Query
| type WorkspaceFilterProps = Readonly<{ | ||
| filter: UseFilterResult; | ||
| error: unknown; | ||
| templateMenu: TemplateFilterMenu; | ||
| statusMenu: StatusFilterMenu; | ||
|
|
||
| userMenu?: UserFilterMenu; | ||
| organizationsMenu?: OrganizationsFilterMenu; | ||
| }>; |
There was a problem hiding this comment.
Decouples the main state from the actual component props
The way I think about it is that props are a hack to get JS function to support named arguments and make it mirror HTML more, so you really shouldn't ever have any direct dependencies on that
| const abortNavigation = | ||
| page === newPage || !Number.isFinite(page) || !Number.isInteger(page); | ||
| if (abortNavigation) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
What is this render optimization? Not re-rendering the page if it's the same page?
| @@ -121,9 +106,11 @@ export const templateExamples = () => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const templateVersionRoot: string = "templateVersion"; | |||
There was a problem hiding this comment.
nit: there seems to be inconsistencies within the code base between using camelCase or UPPER_SNAKE_CASE for file-wide constants so while out of scope for this PR I think deciding on one and sticking to it would be good
There was a problem hiding this comment.
Yeah, that's fair. The JS/TS way seems to be SCREAMING_CASE for constants, while the Go way seems to be to treat them as any other package value
I'm leaning towards switching this to SCREAMING_CASE, but I'll wait for Kayla to chime in
There was a problem hiding this comment.
I prefer to just name them like normal variables personally. an example of why in this context is that for an actual constant key vs a function that returns the key we get...
const aQueryKey = ["a"];
const bQueryKey = (id: string) => ["b"];
({ queryKey: aQueryKey });
({ queryKey: bQueryKey(bId) });a small benefit of this aside from the visual consistency is that if a query key ever needs to go from truly constant to a function the amount of typing required is slightly smaller.
I think SCREAMING_CASE served more of a purpose in the days before LSPs. now it's just so much easier to tell what something is and where it's defined.
| * workspace is in the middle of a transition and will eventually reach a more | ||
| * stable state/status. | ||
| */ | ||
| const ACTIVE_BUILD_STATUSES: readonly WorkspaceStatus[] = [ |
There was a problem hiding this comment.
This becomes exported in the second PR, which is why I put more of a public-facing comment on it
There was a problem hiding this comment.
maybe worth moving to modules/workspaces when you do
| // We want to uncheck the selected workspaces always when the url changes | ||
| // because of filtering or pagination | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: consider refactoring | ||
| useEffect(() => { | ||
| setCheckedWorkspaces([]); | ||
| }, [urlSearchParams]); |
There was a problem hiding this comment.
Forgot to comment on this, but the resetChecked logic above is meant to make sure that if we add more URL logic in the future, it doesn't break our filtering and pagination
We were coarsely syncing based on the entire URL params object. That worked fine for now, but if we were to add any logic on the same page that also dealt with URLs and had nothing to do with the filtering/pagination, it would still cause all the checkboxes to get unchecked on every change
Moving the state syncing to the event handlers for the filter/pagination hooks seemed like the better choice. That way, we're also not coupled to the hooks formatting the URLs a specific way either
WalkthroughThis set of changes refactors how filter and pagination state is managed and propagated across multiple pages and components, especially regarding URL search parameters. It centralizes and synchronizes state management for filters, pagination, and batch actions, improves type safety and naming consistency, and removes some deprecated or redundant code. Components and hooks now generally accept explicit parameters and callbacks instead of React Router tuples, and prop naming is clarified throughout the workspace-related modules. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageComponent
participant useFilter
participant usePagination
User->>PageComponent: Interacts with filter or pagination UI
PageComponent->>useFilter: Passes searchParams, onSearchParamsChange
PageComponent->>usePagination: Passes searchParams, onSearchParamsChange
useFilter-->>PageComponent: Calls onSearchParamsChange with new params
usePagination-->>PageComponent: Calls onSearchParamsChange with new params
PageComponent->>PageComponent: Updates state, resets selections as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
site/src/components/Filter/Filter.tsx (1)
65-78: Technical debt: Direct URLSearchParams mutationThe direct mutation is a pragmatic workaround for existing issues in the codebase. The detailed comment is helpful for understanding why this approach was taken. Consider tracking this as technical debt for future refactoring.
Would you like me to create an issue to track refactoring this mutation pattern and the related code that depends on it?
site/src/pages/WorkspacesPage/WorkspacesPage.tsx (1)
193-212: Pragmatic approach to query invalidation with clear documentation.The prefetch invalidation strategy improves UX by ensuring fresh data before the update modal opens. The comment clearly explains why granular invalidation is necessary given current architectural constraints.
Consider tracking this technical debt - a more unified query invalidation API would simplify this logic in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
site/.storybook/preview.jsx(2 hunks)site/src/api/queries/templates.ts(7 hunks)site/src/components/Filter/Filter.tsx(1 hunks)site/src/hooks/usePagination.ts(1 hunks)site/src/pages/AuditPage/AuditPage.test.tsx(2 hunks)site/src/pages/AuditPage/AuditPage.tsx(1 hunks)site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx(1 hunks)site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx(2 hunks)site/src/pages/TemplatesPage/TemplatesPage.tsx(1 hunks)site/src/pages/UsersPage/UsersPage.tsx(2 hunks)site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx(2 hunks)site/src/pages/WorkspacesPage/WorkspacesPage.tsx(10 hunks)site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx(4 hunks)site/src/pages/WorkspacesPage/WorkspacesPageView.tsx(10 hunks)site/src/pages/WorkspacesPage/batchActions.ts(5 hunks)site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
site/**/*.tsx
📄 CodeRabbit Inference Engine (.cursorrules)
All user-facing frontend code is developed in TypeScript using React and lives in the
site/directory.
Files:
site/src/pages/ConnectionLogPage/ConnectionLogPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsxsite/src/pages/UsersPage/UsersPage.tsxsite/src/pages/AuditPage/AuditPage.test.tsxsite/src/pages/AuditPage/AuditPage.tsxsite/src/pages/TemplatesPage/TemplatesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.test.tsxsite/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsxsite/src/components/Filter/Filter.tsxsite/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.tsx
site/src/**/*.tsx
📄 CodeRabbit Inference Engine (site/CLAUDE.md)
site/src/**/*.tsx: MUI components are deprecated - migrate away from these when encountered
Use shadcn/ui components first - check site/src/components for existing implementations
Emotion CSS is deprecated. Use Tailwind CSS instead.
Responsive design - use Tailwind's responsive prefixes (sm:, md:, lg:, xl:)
Do not usedark:prefix for dark mode
Group related Tailwind classes
Prefer Tailwind utilities over custom CSS when possible
Use Tailwind classes for all new styling
Replace Emotioncssprop with Tailwind classes
Leverage custom color tokens: content-primary, surface-secondary, etc.
Use className with clsx for conditional styling
Don’t call component functions directly; render them via JSX. This keeps Hook rules intact and lets React optimize reconciliation.
After calling a setter you’ll still read the previous state during the same event; updates are queued and batched.
Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Pass a function to useState(initialFn) for lazy initialization—it runs only on the first render.
If the next state is Object.is-equal to the current one, React skips the re-render.
An Effect takes a setup function and optional cleanup; React runs setup after commit, cleanup before the next setup or on unmount.
The dependency array must list every reactive value referenced inside the Effect, and its length must stay constant.
Effects run only on the client, never during server rendering.
Use Effects solely to synchronize with external systems; if you’re not “escaping React,” you probably don’t need one.
Every sibling element in a list needs a stable, unique key prop. Never use array indexes or Math.random(); prefer data-driven IDs.
Keys aren’t passed to children and must not change between renders; if you return multiple nodes per item, use
useRef stores a mutable .current without causing re-renders.
Avoid reading or mutating refs during render; access them in event handlers or Effects ...
Files:
site/src/pages/ConnectionLogPage/ConnectionLogPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsxsite/src/pages/UsersPage/UsersPage.tsxsite/src/pages/AuditPage/AuditPage.test.tsxsite/src/pages/AuditPage/AuditPage.tsxsite/src/pages/TemplatesPage/TemplatesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.test.tsxsite/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsxsite/src/components/Filter/Filter.tsxsite/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.tsx
site/src/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (site/CLAUDE.md)
site/src/**/*.{ts,tsx}: Use ES modules (import/export) syntax, not CommonJS (require)
Destructure imports when possible (eg. import { foo } from 'bar')
Preferfor...ofoverforEachfor iteration
Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Only call Hooks at the top level of a function component or another custom Hook—never in loops, conditions, nested functions, or try / catch.
Only call Hooks from React functions. Regular JS functions, classes, event handlers, useMemo, etc. are off-limits.
Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Don’t call Hooks (including useRef) inside loops, conditions, or map(). Extract a child component instead.
Files:
site/src/pages/ConnectionLogPage/ConnectionLogPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsxsite/src/pages/UsersPage/UsersPage.tsxsite/src/pages/AuditPage/AuditPage.test.tsxsite/src/pages/AuditPage/AuditPage.tsxsite/src/pages/TemplatesPage/TemplatesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.test.tsxsite/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsxsite/src/pages/WorkspacesPage/batchActions.tssite/src/hooks/usePagination.tssite/src/components/Filter/Filter.tsxsite/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsxsite/src/api/queries/templates.tssite/src/pages/WorkspacesPage/WorkspacesPage.tsxsite/src/pages/WorkspacesPage/WorkspacesPageView.tsx
site/src/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursorrules)
React components and pages are organized in the
site/src/directory, with Jest used for testing.
Files:
site/src/pages/AuditPage/AuditPage.test.tsxsite/src/pages/WorkspacesPage/WorkspacesPage.test.tsx
site/**/*.ts
📄 CodeRabbit Inference Engine (.cursorrules)
All user-facing frontend code is developed in TypeScript using React and lives in the
site/directory.
Files:
site/src/pages/WorkspacesPage/batchActions.tssite/src/hooks/usePagination.tssite/src/api/queries/templates.ts
site/src/components/**/*
📄 CodeRabbit Inference Engine (site/CLAUDE.md)
Create custom components only when shadcn alternatives don't exist
Files:
site/src/components/Filter/Filter.tsx
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the `site/` directory.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx (4)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx (10)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Destructure imports when possible (eg. import { foo } from 'bar')
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : If the next state is Object.is-equal to the current one, React skips the re-render.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Use ES modules (import/export) syntax, not CommonJS (require)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use shadcn/ui components first - check site/src/components for existing implementations
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to site/src/**/*.test.{ts,tsx,js,jsx} : React components and pages are organized in the site/src/ directory, with Jest used for testing.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
site/src/pages/UsersPage/UsersPage.tsx (9)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Destructure imports when possible (eg. import { foo } from 'bar')
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Don’t call Hooks (including useRef) inside loops, conditions, or map(). Extract a child component instead.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Don’t call component functions directly; render them via JSX. This keeps Hook rules intact and lets React optimize reconciliation.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
site/.storybook/preview.jsx (1)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
site/src/pages/AuditPage/AuditPage.test.tsx (1)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to site/src/**/*.test.{ts,tsx,js,jsx} : React components and pages are organized in the site/src/ directory, with Jest used for testing.
site/src/pages/AuditPage/AuditPage.tsx (5)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Don’t call Hooks (including useRef) inside loops, conditions, or map(). Extract a child component instead.
site/src/pages/TemplatesPage/TemplatesPage.tsx (6)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Destructure imports when possible (eg. import { foo } from 'bar')
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Use ES modules (import/export) syntax, not CommonJS (require)
site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx (3)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to site/src/**/*.test.{ts,tsx,js,jsx} : React components and pages are organized in the site/src/ directory, with Jest used for testing.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Use ES modules (import/export) syntax, not CommonJS (require)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx (5)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Pass a function to useState(initialFn) for lazy initialization—it runs only on the first render.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Avoid reading or mutating refs during render; access them in event handlers or Effects after commit.
site/src/pages/WorkspacesPage/batchActions.ts (3)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
site/src/hooks/usePagination.ts (4)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : If the next state is Object.is-equal to the current one, React skips the re-render.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Every sibling element in a list needs a stable, unique key prop. Never use array indexes or Math.random(); prefer data-driven IDs.
site/src/components/Filter/Filter.tsx (10)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Don’t call Hooks (including useRef) inside loops, conditions, or map(). Extract a child component instead.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Don’t call component functions directly; render them via JSX. This keeps Hook rules intact and lets React optimize reconciliation.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Only call Hooks from React functions. Regular JS functions, classes, event handlers, useMemo, etc. are off-limits.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Destructure imports when possible (eg. import { foo } from 'bar')
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: ALWAYS use TypeScript LSP tools first when investigating code - don't manually search files
site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx (9)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Replace Emotion css prop with Tailwind classes
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Destructure imports when possible (eg. import { foo } from 'bar')
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Don’t call component functions directly; render them via JSX. This keeps Hook rules intact and lets React optimize reconciliation.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use shadcn/ui components first - check site/src/components for existing implementations
site/src/pages/WorkspacesPage/WorkspacesPage.tsx (15)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Avoid reading or mutating refs during render; access them in event handlers or Effects after commit.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Use ES modules (import/export) syntax, not CommonJS (require)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never pass Hooks around as values or mutate them dynamically. Keep Hook usage static and local to each component.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : After calling a setter you’ll still read the previous state during the same event; updates are queued and batched.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Only call Hooks from React functions. Regular JS functions, classes, event handlers, useMemo, etc. are off-limits.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Don’t call Hooks (including useRef) inside loops, conditions, or map(). Extract a child component instead.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Don’t call component functions directly; render them via JSX. This keeps Hook rules intact and lets React optimize reconciliation.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Only call Hooks at the top level of a function component or another custom Hook—never in loops, conditions, nested functions, or try / catch.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use shadcn/ui components first - check site/src/components for existing implementations
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use Effects solely to synchronize with external systems; if you’re not “escaping React,” you probably don’t need one.
site/src/pages/WorkspacesPage/WorkspacesPageView.tsx (6)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Components and custom Hooks must be pure and idempotent—same inputs → same output; move side-effects to event handlers or Effects.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : After calling a setter you’ll still read the previous state during the same event; updates are queued and batched.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Replace Emotion css prop with Tailwind classes
🧬 Code Graph Analysis (3)
site/src/pages/TemplatesPage/TemplatesPage.tsx (1)
site/src/components/Filter/Filter.tsx (1)
useFilter(49-93)
site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx (1)
site/src/testHelpers/renderHelpers.tsx (1)
renderWithAuth(88-115)
site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx (4)
site/src/components/Filter/Filter.tsx (2)
UseFilterResult(38-45)MenuSkeleton(145-147)site/src/pages/WorkspacesPage/filter/menus.tsx (4)
TemplateFilterMenu(69-69)StatusFilterMenu(130-130)TemplateMenu(76-96)StatusMenu(137-148)site/src/components/Filter/UserFilter.tsx (2)
UserFilterMenu(81-81)UserMenu(89-109)site/src/modules/tableFiltering/options.tsx (2)
OrganizationsFilterMenu(89-91)OrganizationsMenu(98-121)
🔇 Additional comments (44)
site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx (2)
111-111: LGTM! Improved lazy initialization pattern.The change correctly removes the redundant wrapper function. Passing
generateWorkspaceNamedirectly touseStateleverages React's built-in lazy initialization - the function is called only on the first render, which is the intended behavior.
125-132: LGTM! Enhanced type safety with explicit generic typing.The change correctly removes the type assertion and uses explicit generic typing
reduce<Record<string, boolean>>instead. This approach is safer as it avoids potential compiler silencing while maintaining clear type information.site/src/pages/AuditPage/AuditPage.test.tsx (2)
4-4: LGTM! Added type import for better test type safety.The import of
AuditLogsRequestenables explicit typing in the test assertion, improving type safety.
110-115: LGTM! Enhanced test assertion with explicit typing.The change to
toHaveBeenCalledWith<[AuditLogsRequest]>with explicit type parameter improves type safety in the test assertion. Also good to see the migration from the deprecatedtoBeCalledWithto the preferredtoHaveBeenCalledWith.site/src/pages/AuditPage/AuditPage.tsx (1)
36-38: LGTM! Improved hook interface with explicit parameters.The change from
searchParamsResultto separatesearchParamsandonSearchParamsChangeparameters successfully decouples theuseFilterhook from React Router's tuple pattern. This makes the interface more explicit and easier to understand, aligning with the PR's objectives for interface design improvements.site/src/pages/TemplatesPage/TemplatesPage.tsx (2)
17-17: LGTM! Explicit destructuring for clearer parameter handling.The explicit destructuring of
useSearchParams()return value makes the code more readable and prepares for the improveduseFilterinterface.
20-22: LGTM! Consistent interface improvement.The change to separate
searchParamsandonSearchParamsChangeparameters follows the same pattern as other pages in this refactor, successfully decoupling from React Router's tuple pattern while maintaining functionality.site/.storybook/preview.jsx (2)
104-109: LGTM! Good practice centralizing the magic string.The introduction of
queryParametersKeyconstant improves maintainability and reduces the risk of typos. The clear comment explains the reasoning well.
122-123: LGTM! Consistent usage of the centralized constant.The replacement of hardcoded "queries" strings with the
queryParametersKeyconstant maintains consistency and aligns with the maintainability improvements throughout the codebase.site/src/pages/ConnectionLogPage/ConnectionLogPage.tsx (1)
32-34: LGTM! Interface improvement aligns with refactoring goals.The change from passing a
searchParamsResulttuple to explicitsearchParamsandonSearchParamsChangeparameters improves theuseFilterhook interface by decoupling it from React Router internals and making dependencies explicit.site/src/pages/WorkspacesPage/WorkspacesPage.test.tsx (3)
4-4: Good addition for type safety.Adding the
WorkspacesResponsetype import ensures proper typing for mock server responses in tests.
33-36: Improved mock response formatting and type safety.The explicit
WorkspacesResponsetype annotation and formatted JSON structure make the test more readable and type-safe.
41-41: Better semantic testing with findByRole.Using
findByRole("heading", { name: /Create a workspace/ })is more semantically correct thanfindByTextfor testing heading elements.site/src/pages/UsersPage/UsersPage.tsx (3)
42-42: Consistent with refactoring pattern.Destructuring
useSearchParams()into separate variables aligns with the systematic refactor to decouple hooks from React Router tuples.
60-60: Direct parameter passing improves clarity.Passing
searchParamsdirectly topaginatedUsersis clearer than passing it as part of a tuple.
62-63: Explicit interface parameters improve design.The updated
useFiltercall with explicitsearchParamsandonSearchParamsChangeparameters follows the established refactoring pattern and improves the hook's interface design.site/src/pages/WorkspacesPage/WorkspacesPageView.stories.tsx (3)
33-33: Explicit type import improves clarity.Using the explicit
WorkspaceFilterStatetype instead of deriving it fromComponentPropsmakes the type dependencies clearer and more maintainable.
137-137: Consistent type annotation.The explicit
WorkspaceFilterStatetype annotation aligns with the imported type and improves type safety.
170-170: Better semantic naming with filterState.Renaming from
filterPropstofilterStatebetter reflects the semantic meaning of the data and aligns with the updated component interface.Also applies to: 267-267
site/src/api/queries/templates.ts (2)
109-109: Good centralization of query key constant.The
templateVersionRootconstant centralizes all "templateVersion" string literals used in query keys, improving maintainability and reducing magic strings. The explicitstringtype annotation correctly prevents TypeScript from over-inferring the type as a literal.
113-113: Consistent usage of centralized constant.Replacing all "templateVersion" string literals with the
templateVersionRootconstant ensures consistency and makes future changes easier to manage across all template version query keys.Also applies to: 124-124, 143-143, 206-206, 247-247, 254-254, 261-261, 283-283, 303-303
site/src/hooks/usePagination.ts (2)
3-23: Good refactoring to decouple from React RouterThe explicit type definitions and decoupled interface improve type safety and make the hook more reusable. The readonly modifiers on both input and output types prevent accidental mutations.
22-28: Robust page parsing and offset calculationGood defensive programming with proper validation of the page number and safe offset calculation that prevents negative values.
site/src/pages/WorkspacesPage/batchActions.ts (2)
6-26: Excellent type safety improvementsThe explicit return type definition and extracted payload type improve code clarity and maintainability. The consistent use of readonly modifiers prevents accidental mutations.
81-112: Smart solution for TypeScript promise typingGood use of async/await to ensure the mutation functions return
Promise<void>instead ofPromise<void[]>. The explanatory comment helps future maintainers understand the design decision.site/src/pages/WorkspacesPage/WorkspacesPageView.tsx (3)
29-57: Clear and consistent prop namingThe renamed batch action handlers (
onBatchDeleteTransition, etc.) and filter state type make the component's interface more explicit and easier to understand.
90-93: Better boolean variable namingThe rename to
pageNumberIsInvalidfollows boolean naming conventions and makes the code more readable.
119-126: Explicit menu prop passing improves type safetyPassing individual menu props instead of a nested object makes the component's dependencies clearer and enables better TypeScript type checking.
site/src/pages/WorkspacesPage/filter/WorkspacesFilter.tsx (2)
1-88: Good type restructuring for better component interfaceThe separation of
WorkspaceFilterState(for state management) andWorkspaceFilterProps(for component props) with individual menu properties improves type safety and makes the component's dependencies explicit.
90-135: Clean implementation with simplified conditional logicGood use of the
organizationsActivevariable to simplify the conditional rendering logic. The component correctly handles the individual menu props.site/src/components/Filter/Filter.tsx (2)
27-54: Excellent decoupling from React RouterThe explicit type definition and decoupled interface make the hook more reusable and testable. The clear parameter documentation helps with maintainability.
57-63: Good performance optimizationThe early return prevents unnecessary updates when the filter value hasn't changed, reducing re-renders and improving performance.
site/src/pages/WorkspacesPage/WorkspacesPage.tsx (12)
3-3: Good use of type imports and centralized query keys.The addition of
templateVersionRootsupports better query key management, and usingtypeimports for TypeScript types is a best practice that improves build performance.Also applies to: 5-5, 14-14
25-41: Well-structured constants with clear documentation.The
ACTIVE_BUILD_STATUSESconstant properly centralizes the transitional workspace states, and the refresh interval constants are clearly documented. This improves code maintainability.
57-58: Clean type definition for batch actions.The
BatchActionunion type provides type safety for the unified batch action state management.
61-77: Excellent refactoring of workspace selection state.Using a
Set<string>for workspace IDs instead of storing full objects prevents stale data issues and improves performance. The comment clearly explains the rationale for centralizinguseSearchParams.
79-85: Good decoupling of pagination from React Router.The new interface with explicit
searchParamsandonSearchParamsChangecallback improves testability and makes the data flow more explicit. TheresetChecked()call ensures UI consistency.
109-116: Proper coordination of filter, pagination, and selection state.The filter change handler correctly resets both pagination (to page 1) and selections, preventing UI inconsistencies when filters change.
120-120: Correct property access for the refactored filter state.
144-144: Clean unification of batch action state.The single
activeBatchActionstate variable simplifies the logic compared to separate boolean states. DerivingcheckedWorkspacesfrom the ID set ensures fresh data.Also applies to: 150-150, 154-155
167-178: Excellent implementation of the checkbox change handler.The handler properly uses functional updates, includes a smart optimization to avoid unnecessary state updates when selection hasn't changed, and correctly creates new Set instances to maintain immutability.
188-192: Consistent and clear prop naming.The renamed props (
filterState,isRunningBatchAction, batch action handlers) improve API clarity and consistency.
227-234: Consistent batch confirmation dialog implementation.Both confirmation dialogs properly use the unified
activeBatchActionstate and correctly reset it after completion or cancellation.Also applies to: 238-248
257-270: Clean refactoring of the filter hook interface.The explicit
searchParamsandonSearchParamsChangeparameters improve testability and decouple the hook from React Router, consistent with the overall refactoring approach.
| goToPage: (newPage) => { | ||
| const abortNavigation = | ||
| page === newPage || !Number.isFinite(page) || !Number.isInteger(page); | ||
| if (abortNavigation) { | ||
| return; | ||
| } | ||
|
|
||
| const copy = new URLSearchParams(searchParams); | ||
| copy.set("page", page.toString()); | ||
| onSearchParamsChange(copy); | ||
| }, |
There was a problem hiding this comment.
Critical bug: setting wrong page value
The goToPage function has a bug where it sets the current page instead of newPage. Also, the validation should check newPage instead of page.
Apply this fix:
goToPage: (newPage) => {
const abortNavigation =
- page === newPage || !Number.isFinite(page) || !Number.isInteger(page);
+ page === newPage || !Number.isFinite(newPage) || !Number.isInteger(newPage);
if (abortNavigation) {
return;
}
const copy = new URLSearchParams(searchParams);
- copy.set("page", page.toString());
+ copy.set("page", newPage.toString());
onSearchParamsChange(copy);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| goToPage: (newPage) => { | |
| const abortNavigation = | |
| page === newPage || !Number.isFinite(page) || !Number.isInteger(page); | |
| if (abortNavigation) { | |
| return; | |
| } | |
| const copy = new URLSearchParams(searchParams); | |
| copy.set("page", page.toString()); | |
| onSearchParamsChange(copy); | |
| }, | |
| goToPage: (newPage) => { | |
| const abortNavigation = | |
| page === newPage || !Number.isFinite(newPage) || !Number.isInteger(newPage); | |
| if (abortNavigation) { | |
| return; | |
| } | |
| const copy = new URLSearchParams(searchParams); | |
| copy.set("page", newPage.toString()); | |
| onSearchParamsChange(copy); | |
| }, |
🤖 Prompt for AI Agents
In site/src/hooks/usePagination.ts around lines 29 to 39, the goToPage function
incorrectly validates and sets the current page variable instead of the newPage
parameter. To fix this, update the validation conditions to check newPage for
finiteness and integer status, and set the "page" query parameter to
newPage.toString() instead of page.toString() before calling
onSearchParamsChange.
| * ambient types defined in `storybook.d.ts` to provide extra type-safety. | ||
| * Extracting main key to avoid typos. | ||
| */ | ||
| const queryParametersKey = "queries"; |
There was a problem hiding this comment.
actually it looks like this can be a tsx file in Storybook 9. I'm gonna upgrade us.
| @@ -121,9 +106,11 @@ export const templateExamples = () => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const templateVersionRoot: string = "templateVersion"; | |||
There was a problem hiding this comment.
I am very interested to hear you elaborate on all of this because I am not sure I follow. 😅
| const abortNavigation = | ||
| page === newPage || !Number.isFinite(page) || !Number.isInteger(page); | ||
| if (abortNavigation) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
wdym edge cases with Object.is? the only "edge case" I'm aware of is Object.is(0, -0) === false
| @@ -121,9 +106,11 @@ export const templateExamples = () => { | |||
| }; | |||
| }; | |||
|
|
|||
| export const templateVersionRoot: string = "templateVersion"; | |||
There was a problem hiding this comment.
I prefer to just name them like normal variables personally. an example of why in this context is that for an actual constant key vs a function that returns the key we get...
const aQueryKey = ["a"];
const bQueryKey = (id: string) => ["b"];
({ queryKey: aQueryKey });
({ queryKey: bQueryKey(bId) });a small benefit of this aside from the visual consistency is that if a query key ever needs to go from truly constant to a function the amount of typing required is slightly smaller.
I think SCREAMING_CASE served more of a purpose in the days before LSPs. now it's just so much easier to tell what something is and where it's defined.
|
|
||
| /** | ||
| * @todo 2025-07-15 - We have a slightly nasty bug here, where trying to | ||
| * update state the "React way" causes our code to break. |
There was a problem hiding this comment.
| * update state the "React way" causes our code to break. | |
| * update state immutably causes our code to break. |
| const initialTouched = parameters.reduce<Record<string, boolean>>( | ||
| (touched, parameter) => { | ||
| if (autofillByName[parameter.name] !== undefined) { | ||
| touched[parameter.name] = true; | ||
| } | ||
| return touched; | ||
| }, | ||
| {} as Record<string, boolean>, | ||
| {}, | ||
| ); |
There was a problem hiding this comment.
better yet can we just get rid of the reduce here?
const initialTouched = Object.fromEntries(
parameters
.filter((it) => autofillByName[it.name])
.map((it) => [it.name, true]),
);| * workspace is in the middle of a transition and will eventually reach a more | ||
| * stable state/status. | ||
| */ | ||
| const ACTIVE_BUILD_STATUSES: readonly WorkspaceStatus[] = [ |
There was a problem hiding this comment.
maybe worth moving to modules/workspaces when you do
| // Not a great idea to return the promises from the Promise.all calls below | ||
| // because that then gives you a void array, which doesn't make sense with | ||
| // TypeScript's type system. Best to await them, and then have the wrapper | ||
| // mutation function return its own void promise | ||
|
|
||
| const favoriteAllMutation = useMutation({ | ||
| mutationFn: (workspaces: readonly Workspace[]) => { | ||
| return Promise.all( | ||
| mutationFn: async (workspaces: readonly Workspace[]) => { |
There was a problem hiding this comment.
I think I'd rather just specify a : Promise<void> return type to prevent it from getting changed back, and get rid of the comment. it's hopefully clear enough with just that why returning Promise<Array<void>> is weird.
| @@ -77,8 +98,8 @@ export function useBatchActions(options: UseBatchActionsProps) { | |||
| }); | |||
|
|
|||
| const unfavoriteAllMutation = useMutation({ | |||
| mutationFn: (workspaces: readonly Workspace[]) => { | |||
| return Promise.all( | |||
| mutationFn: async (workspaces: readonly Workspace[]) => { | |||
There was a problem hiding this comment.
and then same here
| mutationFn: async (workspaces: readonly Workspace[]) => { | |
| mutationFn: async (workspaces: readonly Workspace[]): Promise<void> => { |
Precursor to #18895
Splitting this off so that the changes are easier to review.
Changes made
withQueryStorybook decoratoruseFilterandusePaginationto have much more strict return types, and decoupled them from React Router at the interface leveluseFilterandusePagination/workspacespageuseEffectcall for syncing URL state to checked workspaces, in favor of explicit state syncsNotes
Summary by CodeRabbit
Refactor
Bug Fixes
Tests