perf(platform): simplify TanStack Query#464
Conversation
Greptile OverviewGreptile SummaryThis PR significantly simplifies the TanStack Query integration by removing custom invalidation logic and optimistic updates in favor of Convex's built-in reactivity. Key Changes:
Architecture Improvement: Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/hooks/use-convex-mutation.ts | Simplified to delegate to @convex-dev/react-query instead of manual invalidation |
| services/platform/app/hooks/use-convex-action.ts | Simplified to delegate to @convex-dev/react-query instead of manual invalidation |
| services/platform/app/hooks/use-convex-optimistic-mutation.ts | Removed optimistic mutations file; functionality replaced by Convex native reactivity |
| services/platform/app/hooks/invalidate.ts | Removed manual invalidation helpers; now handled by Convex reactivity |
| services/platform/app/router.tsx | Removed explicit staleTime config to leverage Convex reactivity |
| services/platform/app/features/websites/components/websites-table.tsx | Updated to use UsePaginatedQueryResult instead of static array |
| services/platform/convex/websites/list_websites_paginated.ts | New paginated query helper with smart index selection for filters |
Flowchart
flowchart TB
subgraph Before["Before: Manual Invalidation"]
A1[Component calls mutation hook]
A2[useConvexMutation with invalidates array]
A3[Execute Convex mutation]
A4[onSettled: Manually invalidate queries]
A5[Query cache invalidated]
A6[Component re-fetches data]
A1 --> A2 --> A3 --> A4 --> A5 --> A6
end
subgraph After["After: Convex Reactivity"]
B1[Component calls mutation hook]
B2[useConvexMutation delegates to @convex-dev/react-query]
B3[Execute Convex mutation]
B4[Convex WebSocket pushes updates]
B5[React Query cache auto-updates]
B6[Component re-renders with fresh data]
B1 --> B2 --> B3 --> B4 --> B5 --> B6
end
style Before fill:#ffebee
style After fill:#e8f5e9
Last reviewed commit: 38ea96c
📝 WalkthroughWalkthroughThis pull request removes optimistic mutation handling and explicit cache invalidation from the platform's Convex hooks infrastructure. The refactoring deletes Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/platform/app/features/conversations/hooks/__tests__/mutation-hooks.test.ts (1)
30-34: 🧹 Nitpick | 🔵 TrivialDead mock:
queriesblock is no longer referenced.Since cache invalidation was removed, no test in this file references
api.conversations.queries. Consider removing this leftover to keep the mock minimal.🧹 Suggested cleanup
conversations: { mutations: { closeConversation: 'closeConversation', reopenConversation: 'reopenConversation', markConversationAsRead: 'markConversationAsRead', markConversationAsSpam: 'markConversationAsSpam', }, - queries: { - listConversations: 'listConversations', - }, },services/platform/app/routes/dashboard/$id/_knowledge/websites.tsx (1)
9-12: 🧹 Nitpick | 🔵 Trivial
queryfield in search schema is declared but unused in the component.
searchSchemadefinesquery: z.string().optional()butWebsitesPageonly readssearch.status. Ifqueryis used by the client-side search withinDataTable, it may not need to be in the URL schema. Verify whether this is intentional for URL-persisted search state or leftover from a previous iteration.
🤖 Fix all issues with AI agents
In `@services/platform/app/hooks/__tests__/use-convex-action.test.ts`:
- Around line 56-60: The test incorrectly asserts referential equality of the
mutationFn returned by useConvexAction to mockActionFn even though
useConvexAction sets mutationFn to a wrapper arrow function; update the
assertion to verify behavioral equivalence instead: grab options =
mockUseMutation.mock.calls[0]?.[0], ensure options.mutationFn is a function,
call options.mutationFn with a sample payload and then assert that mockActionFn
was called with that payload (or that the wrapper returns/forwards the result),
referencing useConvexAction, mockActionRef, mockUseMutation and mutationFn to
locate the test.
In `@services/platform/app/hooks/__tests__/use-convex-mutation.test.ts`:
- Around line 56-60: The test currently asserts identity but useConvexMutation
wraps the original mutate (in useConvexMutation.ts) so update the test to assert
delegation: call the returned options.mutationFn with a sample arg and expect
mockMutationFn to have been called with that arg (use the same
mockMutationRef/mockMutationFn used in the test setup), or alternatively remove
the wrapper in useConvexMutation and set mutationFn: mutate directly so the
original identity assertion passes.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/websites.tsx:
- Around line 33-38: The current check returning <WebsitesEmptyState
organizationId={organizationId} /> when paginatedResult.status === 'Exhausted'
&& paginatedResult.results.length === 0 hides filter controls; change the
condition to only render WebsitesEmptyState when there are no active
filters/search params. Detect active filters (e.g., status, search) from your
query/search params or the component's filter state and only return
WebsitesEmptyState if no filters are set; otherwise render the table (or an
empty-table placeholder) with filters still visible so users can clear them.
Update the conditional around WebsitesEmptyState (the paginatedResult.status /
paginatedResult.results.length check) to include a guard like "&&
!hasActiveFilters" where hasActiveFilters is derived from the relevant search
params or props.
In `@services/platform/app/routes/dashboard/`$id/custom-agents/$agentId.tsx:
- Around line 37-48: The loader currently prefetches getCustomAgentByVersion and
getCustomAgentVersions with only { customAgentId } which causes a cache miss
when a version is present in the URL because useCustomAgentByVersion(agentId,
versionNumber) includes the version in its query key; update the loader to
accept loaderDeps (extract the `v` search param into a versionNumber) and pass
that versionNumber into the convexQuery prefetch for
api.custom_agents.queries.getCustomAgentByVersion so the prefetched key matches
the component's key (keep using toId<'customAgents'>(params.agentId) for the
id).
In `@services/platform/app/routes/dashboard/`$id/settings/integrations.tsx:
- Around line 24-30: The loader currently prefetches only
integrations.queries.list; also prefetch the queries used by
useCurrentMemberContext and useSsoProvider so the component hydrates faster. In
the loader function add context.queryClient.prefetchQuery calls for the
underlying Convex queries those hooks use (i.e., call convexQuery with the same
api.* query identifiers that feed useCurrentMemberContext and useSsoProvider and
pass params.id or other needed args), mirroring the existing prefetch pattern
for api.integrations.queries.list so the hooks hit the cache on render.
In `@services/platform/app/routes/dashboard/`$id/settings/organization.tsx:
- Around line 15-21: The loader currently prefetches only the organization
query; add a second prefetch for the member context query so the component's
useCurrentMemberContext data is available. In the loader function, call void
context.queryClient.prefetchQuery(convexQuery(api.members.queries.getCurrentMemberContext,
<same-args-used-by-useCurrentMemberContext>)) alongside the existing
convexQuery(api.organizations.queries.getOrganization, { id: params.id }) call,
passing the same parameters the useCurrentMemberContext hook expects (e.g.,
params.id or no args) to mirror the route loader prefetch pattern.
In `@services/platform/convex/websites/list_websites_paginated.ts`:
- Around line 1-6: The docstring for the function in list_websites_paginated.ts
inaccurately claims remaining filters are applied via .filter(), but the
implementation only uses the compound index for status and never calls
.filter(); update the top comment to remove the reference to applying
`.filter()` and explicitly state that only the primary active filter (status) is
supported via the chosen 2-field compound index (or, if you prefer implementing
support for additional filters, add a `.filter()` pass after the `.paginate()`
call in the function `listWebsitesPaginated` to apply any remaining filters);
reference the function name `listWebsitesPaginated` (or the exported function in
this file) when making the change.
In `@services/platform/convex/websites/queries.ts`:
- Around line 26-35: listWebsitesPaginated is missing a returns validator while
listWebsites uses returns: v.array(websiteValidator); add a returns validator to
listWebsitesPaginated that validates the PaginationResult shape (e.g. returns:
paginationResultValidator(websiteValidator)) so runtime validation is
consistent, or if Convex cannot accept nested/custom validators here, add a
short inline comment on listWebsitesPaginated explaining why the returns clause
was omitted and reference PaginationResult/listWebsitesPaginated to help future
readers locate the rationale.
| it('uses the returned function as mutationFn', () => { | ||
| useConvexAction(mockActionRef); | ||
| const options = mockUseMutation.mock.calls[0]?.[0]; | ||
| // @ts-expect-error -- calling mock onSettled directly for testing | ||
| await options.onSettled?.(); | ||
|
|
||
| expect(callOrder).toEqual(['invalidate', 'invalidate', 'userOnSettled']); | ||
| expect(options).toHaveProperty('mutationFn', mockActionFn); | ||
| }); |
There was a problem hiding this comment.
Test fails: mutationFn is a wrapper, not the raw mock.
The implementation wraps the action in an arrow function — mutationFn: (args) => action(args) — so it will never be referentially equal to mockActionFn. This is confirmed by the pipeline failure.
Assert behavioral equivalence instead of identity:
🐛 Proposed fix
it('uses the returned function as mutationFn', () => {
useConvexAction(mockActionRef);
const options = mockUseMutation.mock.calls[0]?.[0];
- expect(options).toHaveProperty('mutationFn', mockActionFn);
+ expect(options).toHaveProperty('mutationFn');
+ expect(typeof options.mutationFn).toBe('function');
+
+ const testArgs = { foo: 'bar' };
+ options.mutationFn(testArgs);
+ expect(mockActionFn).toHaveBeenCalledWith(testArgs);
});📝 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.
| it('uses the returned function as mutationFn', () => { | |
| useConvexAction(mockActionRef); | |
| const options = mockUseMutation.mock.calls[0]?.[0]; | |
| // @ts-expect-error -- calling mock onSettled directly for testing | |
| await options.onSettled?.(); | |
| expect(callOrder).toEqual(['invalidate', 'invalidate', 'userOnSettled']); | |
| expect(options).toHaveProperty('mutationFn', mockActionFn); | |
| }); | |
| it('uses the returned function as mutationFn', () => { | |
| useConvexAction(mockActionRef); | |
| const options = mockUseMutation.mock.calls[0]?.[0]; | |
| expect(options).toHaveProperty('mutationFn'); | |
| expect(typeof options.mutationFn).toBe('function'); | |
| const testArgs = { foo: 'bar' }; | |
| options.mutationFn(testArgs); | |
| expect(mockActionFn).toHaveBeenCalledWith(testArgs); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Test
[error] 59-59: AssertionError: expected { mutationFn: [Function mutationFn] } to have property "mutationFn" with value [Function Mock]
🪛 GitHub Check: Test
[failure] 59-59: app/hooks/tests/use-convex-action.test.ts > useConvexAction > uses the returned function as mutationFn
AssertionError: expected { mutationFn: [Function mutationFn] } to have property "mutationFn" with value [Function Mock]
- Expected
- Received
- [Function Mock]
- [Function mutationFn]
❯ app/hooks/tests/use-convex-action.test.ts:59:21
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/__tests__/use-convex-action.test.ts` around lines
56 - 60, The test incorrectly asserts referential equality of the mutationFn
returned by useConvexAction to mockActionFn even though useConvexAction sets
mutationFn to a wrapper arrow function; update the assertion to verify
behavioral equivalence instead: grab options =
mockUseMutation.mock.calls[0]?.[0], ensure options.mutationFn is a function,
call options.mutationFn with a sample payload and then assert that mockActionFn
was called with that payload (or that the wrapper returns/forwards the result),
referencing useConvexAction, mockActionRef, mockUseMutation and mutationFn to
locate the test.
| it('uses the returned function as mutationFn', () => { | ||
| useConvexMutation(mockMutationRef); | ||
| const options = mockUseMutation.mock.calls[0]?.[0]; | ||
| // @ts-expect-error -- calling mock onSettled directly for testing | ||
| await options.onSettled?.(); | ||
|
|
||
| expect(callOrder).toEqual(['invalidate', 'invalidate', 'userOnSettled']); | ||
| expect(options).toHaveProperty('mutationFn', mockMutationFn); | ||
| }); |
There was a problem hiding this comment.
Pipeline failure: mutationFn is a wrapper, not the mock itself.
The implementation at use-convex-mutation.ts:20 wraps the mutate function in an arrow: (args) => mutate(args), so it won't be referentially equal to mockMutationFn. The test should verify delegation behavior instead of identity.
🐛 Proposed fix — assert delegation instead of reference equality
it('uses the returned function as mutationFn', () => {
useConvexMutation(mockMutationRef);
const options = mockUseMutation.mock.calls[0]?.[0];
- expect(options).toHaveProperty('mutationFn', mockMutationFn);
+ expect(options).toHaveProperty('mutationFn');
+ const args = { id: '123' };
+ options.mutationFn(args);
+ expect(mockMutationFn).toHaveBeenCalledWith(args);
});Alternatively, if the arrow wrapper in use-convex-mutation.ts is not needed for type reasons, you could simplify the implementation to mutationFn: mutate (passing the function directly), which would make the original test assertion pass:
♻️ Alternative — simplify the implementation
In services/platform/app/hooks/use-convex-mutation.ts:
return useMutation({
- mutationFn: (args: FunctionArgs<Func>) => mutate(args),
+ mutationFn: mutate,
...options,
});📝 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.
| it('uses the returned function as mutationFn', () => { | |
| useConvexMutation(mockMutationRef); | |
| const options = mockUseMutation.mock.calls[0]?.[0]; | |
| // @ts-expect-error -- calling mock onSettled directly for testing | |
| await options.onSettled?.(); | |
| expect(callOrder).toEqual(['invalidate', 'invalidate', 'userOnSettled']); | |
| expect(options).toHaveProperty('mutationFn', mockMutationFn); | |
| }); | |
| it('uses the returned function as mutationFn', () => { | |
| useConvexMutation(mockMutationRef); | |
| const options = mockUseMutation.mock.calls[0]?.[0]; | |
| expect(options).toHaveProperty('mutationFn'); | |
| const args = { id: '123' }; | |
| options.mutationFn(args); | |
| expect(mockMutationFn).toHaveBeenCalledWith(args); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Test
[error] 59-59: AssertionError: expected { mutationFn: [Function mutationFn] } to have property "mutationFn" with value [Function Mock]
🪛 GitHub Check: Test
[failure] 59-59: app/hooks/tests/use-convex-mutation.test.ts > useConvexMutation > uses the returned function as mutationFn
AssertionError: expected { mutationFn: [Function mutationFn] } to have property "mutationFn" with value [Function Mock]
- Expected
- Received
- [Function Mock]
- [Function mutationFn]
❯ app/hooks/tests/use-convex-mutation.test.ts:59:21
🤖 Prompt for AI Agents
In `@services/platform/app/hooks/__tests__/use-convex-mutation.test.ts` around
lines 56 - 60, The test currently asserts identity but useConvexMutation wraps
the original mutate (in useConvexMutation.ts) so update the test to assert
delegation: call the returned options.mutationFn with a sample arg and expect
mockMutationFn to have been called with that arg (use the same
mockMutationRef/mockMutationFn used in the test setup), or alternatively remove
the wrapper in useConvexMutation and set mutationFn: mutate directly so the
original identity assertion passes.
| if ( | ||
| paginatedResult.status === 'Exhausted' && | ||
| paginatedResult.results.length === 0 | ||
| ) { | ||
| return <WebsitesEmptyState organizationId={organizationId} />; | ||
| } |
There was a problem hiding this comment.
Empty state renders even when a filter is active, hiding the filter UI.
When the user has a status filter applied (e.g., ?status=error) and no results match, paginatedResult.status === 'Exhausted' with results.length === 0 triggers WebsitesEmptyState. This replaces the entire table — including the filter controls — so the user can't clear the filter to see all websites. The empty state likely shows a "No websites yet" message with a create CTA, which is misleading when websites exist but don't match the filter.
Consider guarding against this when a search filter is active:
🐛 Proposed fix
if (
paginatedResult.status === 'Exhausted' &&
- paginatedResult.results.length === 0
+ paginatedResult.results.length === 0 &&
+ !search.status
) {
return <WebsitesEmptyState organizationId={organizationId} />;
}📝 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.
| if ( | |
| paginatedResult.status === 'Exhausted' && | |
| paginatedResult.results.length === 0 | |
| ) { | |
| return <WebsitesEmptyState organizationId={organizationId} />; | |
| } | |
| if ( | |
| paginatedResult.status === 'Exhausted' && | |
| paginatedResult.results.length === 0 && | |
| !search.status | |
| ) { | |
| return <WebsitesEmptyState organizationId={organizationId} />; | |
| } |
🤖 Prompt for AI Agents
In `@services/platform/app/routes/dashboard/`$id/_knowledge/websites.tsx around
lines 33 - 38, The current check returning <WebsitesEmptyState
organizationId={organizationId} /> when paginatedResult.status === 'Exhausted'
&& paginatedResult.results.length === 0 hides filter controls; change the
condition to only render WebsitesEmptyState when there are no active
filters/search params. Detect active filters (e.g., status, search) from your
query/search params or the component's filter state and only return
WebsitesEmptyState if no filters are set; otherwise render the table (or an
empty-table placeholder) with filters still visible so users can clear them.
Update the conditional around WebsitesEmptyState (the paginatedResult.status /
paginatedResult.results.length check) to include a guard like "&&
!hasActiveFilters" where hasActiveFilters is derived from the relevant search
params or props.
| loader: ({ context, params }) => { | ||
| void context.queryClient.prefetchQuery( | ||
| convexQuery(api.custom_agents.queries.getCustomAgentByVersion, { | ||
| customAgentId: toId<'customAgents'>(params.agentId), | ||
| }), | ||
| ); | ||
| void context.queryClient.prefetchQuery( | ||
| convexQuery(api.custom_agents.queries.getCustomAgentVersions, { | ||
| customAgentId: toId<'customAgents'>(params.agentId), | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the custom agents query definitions
fd -t f -e ts -e tsx custom_agents | head -20Repository: tale-project/tale
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Search for getCustomAgentByVersion query definition
rg "getCustomAgentByVersion" -t ts -t tsx -A 10 | head -50Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look at the specific file mentioned in the review
cat -n "services/platform/app/routes/dashboard/\$id/custom-agents/\$agentId.tsx" | head -100Repository: tale-project/tale
Length of output: 4417
🏁 Script executed:
#!/bin/bash
# Find custom agents queries definition
fd -type f -name "*.ts" -path "*custom_agents*" | grep -E "(queries|hook)" | head -20Repository: tale-project/tale
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Search for getCustomAgentByVersion in API/query definitions
rg "getCustomAgentByVersion" -A 15 -B 2Repository: tale-project/tale
Length of output: 4419
🏁 Script executed:
#!/bin/bash
# Find useCustomAgentByVersion hook
rg "useCustomAgentByVersion" -t ts -A 15 -B 2Repository: tale-project/tale
Length of output: 5079
🏁 Script executed:
#!/bin/bash
# Check other loaders in the codebase to see if they use search parameter
rg "loader:.*\(\s*\{.*search" -A 5 | head -40Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for TanStack Router loader patterns with search
rg "loader:\s*\(\s*\{" -A 3 | grep -E "(loader:|search)" | head -30Repository: tale-project/tale
Length of output: 1057
🌐 Web query:
TanStack Router loader function parameters search
💡 Result:
In TanStack Router, a route loader is called with one argument: an object that includes (among others) params, deps, context, and location. (tanstack.com)
Loader parameter shape (relevant parts)
params: path params (e.g.$postId) (tanstack.com)deps: whatever yourloaderDepsreturns (defaults to{}) (tanstack.com)location: the current location (includes search info) (tanstack.com)
How to use search params in a loader (recommended)
Use loaderDeps to pick the specific search params your loader depends on, then read them from deps inside loader:
export const Route = createFileRoute('/posts')({
loaderDeps: ({ search: { offset, limit } }) => ({ offset, limit }),
loader: ({ deps: { offset, limit } }) => fetchPosts({ offset, limit }),
})This is preferred because changes in those deps will correctly trigger reloads without invalidating cache for unrelated search params. (tanstack.com)
Citations:
- 1: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
- 2: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
- 3: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
- 4: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
- 5: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
- 6: https://tanstack.com/router/latest/docs/framework/react/guide/data-loading?utm_source=openai
Pass versionNumber to prefetch query using loaderDeps — cache miss when ?v=X is present.
The component queries useCustomAgentByVersion(agentId, versionNumber) with the v search param, but the loader only passes { customAgentId }. When a version is specified in the URL, the prefetched query key won't match the component's query key, causing a cache miss.
TanStack Router loaders don't receive search directly. Use loaderDeps to extract and pass the search param:
+ loaderDeps: ({ search: { v } }) => ({ versionNumber: v }),
- loader: ({ context, params }) => {
+ loader: ({ context, params, deps: { versionNumber } }) => {
void context.queryClient.prefetchQuery(
convexQuery(api.custom_agents.queries.getCustomAgentByVersion, {
customAgentId: toId<'customAgents'>(params.agentId),
+ versionNumber,
}),
);📝 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.
| loader: ({ context, params }) => { | |
| void context.queryClient.prefetchQuery( | |
| convexQuery(api.custom_agents.queries.getCustomAgentByVersion, { | |
| customAgentId: toId<'customAgents'>(params.agentId), | |
| }), | |
| ); | |
| void context.queryClient.prefetchQuery( | |
| convexQuery(api.custom_agents.queries.getCustomAgentVersions, { | |
| customAgentId: toId<'customAgents'>(params.agentId), | |
| }), | |
| ); | |
| }, | |
| loaderDeps: ({ search: { v } }) => ({ versionNumber: v }), | |
| loader: ({ context, params, deps: { versionNumber } }) => { | |
| void context.queryClient.prefetchQuery( | |
| convexQuery(api.custom_agents.queries.getCustomAgentByVersion, { | |
| customAgentId: toId<'customAgents'>(params.agentId), | |
| versionNumber, | |
| }), | |
| ); | |
| void context.queryClient.prefetchQuery( | |
| convexQuery(api.custom_agents.queries.getCustomAgentVersions, { | |
| customAgentId: toId<'customAgents'>(params.agentId), | |
| }), | |
| ); | |
| }, |
🤖 Prompt for AI Agents
In `@services/platform/app/routes/dashboard/`$id/custom-agents/$agentId.tsx around
lines 37 - 48, The loader currently prefetches getCustomAgentByVersion and
getCustomAgentVersions with only { customAgentId } which causes a cache miss
when a version is present in the URL because useCustomAgentByVersion(agentId,
versionNumber) includes the version in its query key; update the loader to
accept loaderDeps (extract the `v` search param into a versionNumber) and pass
that versionNumber into the convexQuery prefetch for
api.custom_agents.queries.getCustomAgentByVersion so the prefetched key matches
the component's key (keep using toId<'customAgents'>(params.agentId) for the
id).
| loader: ({ context, params }) => { | ||
| void context.queryClient.prefetchQuery( | ||
| convexQuery(api.integrations.queries.list, { | ||
| organizationId: params.id, | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider prefetching other queries used by this page.
The loader prefetches integrations.queries.list, but the component also depends on useCurrentMemberContext and useSsoProvider (lines 79-83). If those queries are similarly expensive, prefetching them here would further reduce time-to-content. Otherwise, this is fine as-is.
🤖 Prompt for AI Agents
In `@services/platform/app/routes/dashboard/`$id/settings/integrations.tsx around
lines 24 - 30, The loader currently prefetches only integrations.queries.list;
also prefetch the queries used by useCurrentMemberContext and useSsoProvider so
the component hydrates faster. In the loader function add
context.queryClient.prefetchQuery calls for the underlying Convex queries those
hooks use (i.e., call convexQuery with the same api.* query identifiers that
feed useCurrentMemberContext and useSsoProvider and pass params.id or other
needed args), mirroring the existing prefetch pattern for
api.integrations.queries.list so the hooks hit the cache on render.
| loader: ({ context, params }) => { | ||
| void context.queryClient.prefetchQuery( | ||
| convexQuery(api.organizations.queries.getOrganization, { | ||
| id: params.id, | ||
| }), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider also prefetching the current member context query.
The component depends on both useOrganization and useCurrentMemberContext for its loading state (Line 72), but the loader only prefetches the organization query. Adding a prefetch for the member context query would be consistent with the tone-of-voice route loader pattern, which prefetches all queries consumed by its component.
🤖 Prompt for AI Agents
In `@services/platform/app/routes/dashboard/`$id/settings/organization.tsx around
lines 15 - 21, The loader currently prefetches only the organization query; add
a second prefetch for the member context query so the component's
useCurrentMemberContext data is available. In the loader function, call void
context.queryClient.prefetchQuery(convexQuery(api.members.queries.getCurrentMemberContext,
<same-args-used-by-useCurrentMemberContext>)) alongside the existing
convexQuery(api.organizations.queries.getOrganization, { id: params.id }) call,
passing the same parameters the useCurrentMemberContext hook expects (e.g.,
params.id or no args) to mirror the route loader prefetch pattern.
| /** | ||
| * List websites using Convex native .paginate() for use with usePaginatedQuery. | ||
| * | ||
| * Dispatches to the best 2-field compound index based on the primary active | ||
| * filter, then applies .filter() for any remaining filters. | ||
| */ |
There was a problem hiding this comment.
Docstring mentions .filter() for remaining filters, but the implementation never calls .filter().
Line 5 states the function "applies .filter() for any remaining filters," but no code path invokes .filter(). Currently only status is supported and it's handled via the compound index. Update the docstring to reflect the actual behavior, or implement the remaining-filter path if it's intended.
📝 Suggested docstring fix
/**
* List websites using Convex native .paginate() for use with usePaginatedQuery.
*
* Dispatches to the best 2-field compound index based on the primary active
- * filter, then applies .filter() for any remaining filters.
+ * filter; falls back to the by_organizationId index when no filter is active.
*/🤖 Prompt for AI Agents
In `@services/platform/convex/websites/list_websites_paginated.ts` around lines 1
- 6, The docstring for the function in list_websites_paginated.ts inaccurately
claims remaining filters are applied via .filter(), but the implementation only
uses the compound index for status and never calls .filter(); update the top
comment to remove the reference to applying `.filter()` and explicitly state
that only the primary active filter (status) is supported via the chosen 2-field
compound index (or, if you prefer implementing support for additional filters,
add a `.filter()` pass after the `.paginate()` call in the function
`listWebsitesPaginated` to apply any remaining filters); reference the function
name `listWebsitesPaginated` (or the exported function in this file) when making
the change.
| export const listWebsitesPaginated = queryWithRLS({ | ||
| args: { | ||
| paginationOpts: paginationOptsValidator, | ||
| organizationId: v.string(), | ||
| status: v.optional(v.string()), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| return await listWebsitesPaginatedHelper(ctx, args); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a returns validator for consistency.
listWebsites (Line 12) declares returns: v.array(websiteValidator), but listWebsitesPaginated omits a returns clause. If Convex supports return validators on paginated queries, adding one would improve runtime validation consistency. If not feasible due to the PaginationResult wrapper shape, a brief comment explaining the omission would help future readers.
🤖 Prompt for AI Agents
In `@services/platform/convex/websites/queries.ts` around lines 26 - 35,
listWebsitesPaginated is missing a returns validator while listWebsites uses
returns: v.array(websiteValidator); add a returns validator to
listWebsitesPaginated that validates the PaginationResult shape (e.g. returns:
paginationResultValidator(websiteValidator)) so runtime validation is
consistent, or if Convex cannot accept nested/custom validators here, add a
short inline comment on listWebsitesPaginated explaining why the returns clause
was omitted and reference PaginationResult/listWebsitesPaginated to help future
readers locate the rationale.
1 participant
Summary by CodeRabbit
Release Notes
Refactor
New Features
Tests