refactor(ui): improve skeletons and empty state handling#491
Merged
Conversation
Greptile SummaryThis large refactor (92 files, +2155/−989) introduces reusable UI layout components (
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| services/platform/convex/documents/queries.ts | New approxCountDocuments query is missing auth/authz checks — unlike all other count queries in this PR, it uses plain query with no authentication. |
| services/platform/convex/members/queries.ts | New approxCountMyTeams query has N+1 sequential queries and is missing organization membership check (only verifies authentication, not authorization). |
| services/platform/convex/approvals/queries.ts | New approxCountApprovals query correctly implements auth checks and capped counting pattern. |
| services/platform/convex/conversations/queries.ts | Renamed hasConversations to approxCountConversations with capped count; uses queryWithRLS correctly. |
| services/platform/app/hooks/use-list-page.ts | Added skeletonRows option and changed isInitialLoading for query data sources to use data === undefined, enabling skeleton display during initial load. |
| services/platform/app/components/ui/data-table/data-table.tsx | Added skeletonRows prop (default 10) to control the number of skeleton rows during loading, replacing hardcoded value. |
| services/platform/app/components/ui/layout/page-section.tsx | New reusable layout component combining a section element with a SectionHeader and configurable gap variants. |
| services/platform/app/components/ui/layout/section-header.tsx | New reusable header component with title/description/action support and configurable heading level, size, and weight variants. |
| services/platform/app/components/ui/feedback/empty-placeholder.tsx | New reusable empty state component with optional icon and children, replacing duplicated inline empty state markup. |
| services/platform/app/components/ui/data-display/code-block.tsx | New CodeBlock component with copy-to-clipboard functionality, extracted from webhook section inline code. |
| services/platform/app/routes/dashboard/$id/automations/index.tsx | Access control check is bypassed when count === 0 — non-admin users see empty state with create action instead of AccessDenied. |
| services/platform/app/routes/dashboard/$id/custom-agents/index.tsx | Simplified from loading/empty/table states to count-based rendering. Removed ContentWrapper wrapping which may affect layout. |
| services/platform/app/features/approvals/components/approvals-client.tsx | Enhanced skeleton with better column config (avatar-text, alignment), added approxCount prop for empty state and skeleton row sizing. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Route Loader] -->|ensureQueryData| B[approxCount* Query]
A -->|prefetchQuery| C[list* / paginated Query]
B --> D{count === 0?}
D -->|Yes| E[Empty State Component]
D -->|No / undefined| F{Data loaded?}
F -->|No: data === undefined| G[Skeleton with min count,10 rows]
F -->|Yes| H[Render DataTable / Client]
H --> I{Has more data?}
I -->|Yes| J[Infinite Scroll / Load More]
I -->|No| K[Display All Data]
subgraph Backend approxCount Pattern
B --> L[Auth Check]
L -->|Fail| M[Return 0]
L -->|Pass| N[Iterate with Index]
N --> O{count >= 20?}
O -->|Yes| P[Break + Return 20]
O -->|No + exhausted| Q[Return actual count]
end
Last reviewed commit: abd941d
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+18
to
+26
| export const approxCountDocuments = query({ | ||
| args: { | ||
| organizationId: v.string(), | ||
| }, | ||
| returns: v.number(), | ||
| handler: async (ctx, args) => { | ||
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing authentication/authorization checks
This query uses query instead of queryWithRLS and has no auth checks at all — no getAuthUserIdentity, no getOrganizationMember. Every other approxCount* query in this PR either uses queryWithRLS (which enforces row-level security automatically) or manually checks auth (like approxCountApprovals and approxCountExampleMessages). This means any unauthenticated caller can enumerate document counts for any organization by guessing organizationId values.
Compare with the existing approxCountCustomers (uses queryWithRLS) or approxCountApprovals (manually checks auth). Either wrap this in queryWithRLS or add explicit auth checks like:
Suggested change
| export const approxCountDocuments = query({ | |
| args: { | |
| organizationId: v.string(), | |
| }, | |
| returns: v.number(), | |
| handler: async (ctx, args) => { | |
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | |
| }, | |
| }); | |
| export const approxCountDocuments = query({ | |
| args: { | |
| organizationId: v.string(), | |
| }, | |
| returns: v.number(), | |
| handler: async (ctx, args) => { | |
| const authUser = await getAuthUserIdentity(ctx); | |
| if (!authUser) { | |
| return 0; | |
| } | |
| try { | |
| await getOrganizationMember(ctx, args.organizationId, authUser); | |
| } catch { | |
| return 0; | |
| } | |
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | |
| }, | |
| }); |
Comment on lines
+202
to
+247
| export const approxCountMyTeams = query({ | ||
| args: { | ||
| organizationId: v.string(), | ||
| }, | ||
| returns: v.number(), | ||
| handler: async (ctx, args) => { | ||
| const authUser = await getAuthUserIdentity(ctx); | ||
| if (!authUser) { | ||
| return 0; | ||
| } | ||
|
|
||
| const membershipsResult: BetterAuthFindManyResult<BetterAuthTeamMember> = | ||
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | ||
| model: 'teamMember', | ||
| paginationOpts: { cursor: null, numItems: 100 }, | ||
| where: [{ field: 'userId', operator: 'eq', value: authUser.userId }], | ||
| }); | ||
|
|
||
| if (!membershipsResult || membershipsResult.page.length === 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| let count = 0; | ||
| for (const membership of membershipsResult.page) { | ||
| const teamResult: BetterAuthFindManyResult<BetterAuthTeam> = | ||
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | ||
| model: 'team', | ||
| paginationOpts: { cursor: null, numItems: 1 }, | ||
| where: [ | ||
| { field: '_id', operator: 'eq', value: membership.teamId }, | ||
| { | ||
| field: 'organizationId', | ||
| operator: 'eq', | ||
| value: args.organizationId, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| if (teamResult && teamResult.page.length > 0) { | ||
| count++; | ||
| } | ||
| } | ||
|
|
||
| return count; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing organization membership check
Unlike approxCountApprovals and approxCountExampleMessages (which call getOrganizationMember to verify the user belongs to the org), this query only checks authentication — not authorization. An authenticated user could call this with any organizationId to enumerate team counts in organizations they don't belong to.
Add an org membership check like the other approxCount* queries use:
try {
await getOrganizationMember(ctx, args.organizationId, authUser);
} catch {
return 0;
}
Comment on lines
+67
to
+69
| if (count === 0) { | ||
| return <AutomationsEmptyState organizationId={organizationId} />; | ||
| } |
There was a problem hiding this comment.
Access control check bypassed when count is 0
The count === 0 check on line 67 returns the empty state before the role-based access control check on lines 82-85. This means a user without the admin or developer role will see AutomationsEmptyState (with a "create" action link) instead of the AccessDenied component. The previous code correctly checked access before showing the empty state.
Move the access check before the count check, or at least before the empty state return:
Suggested change
| if (count === 0) { | |
| return <AutomationsEmptyState organizationId={organizationId} />; | |
| } | |
| if (count === 0 && !isMemberLoading) { | |
| return <AutomationsEmptyState organizationId={organizationId} />; | |
| } |
Or better, restructure so access is checked first:
if (isMemberLoading || isAutomationsLoading) { ... }
// check access first
const userRole = (memberContext?.role ?? '').toLowerCase();
if (userRole !== 'admin' && userRole !== 'developer') {
return <AccessDenied ... />;
}
if (count === 0) { return <AutomationsEmptyState ... />; }
Comment on lines
+28
to
40
| function CustomAgentsPage() { | ||
| const { id: organizationId } = Route.useParams(); | ||
| const { agents, isLoading } = useCustomAgents(organizationId); | ||
|
|
||
| const { data: count } = useConvexQuery( | ||
| api.custom_agents.queries.countCustomAgents, | ||
| { organizationId }, | ||
| ); | ||
| const { data: count } = useApproxCustomAgentCount(organizationId); | ||
|
|
||
| if (isLoading) { | ||
| if (count === 0) { | ||
| return ( | ||
| <ContentWrapper className="p-4"> | ||
| <CustomAgentsEmptyState organizationId={organizationId} /> | ||
| </ContentWrapper> | ||
| ); | ||
| } | ||
| return ( | ||
| <ContentWrapper> | ||
| <CustomAgentsTableSkeleton | ||
| organizationId={organizationId} | ||
| rows={Math.min(count ?? 10, 10)} | ||
| /> | ||
| </ContentWrapper> | ||
| ); | ||
| } | ||
| const { agents } = useCustomAgents(organizationId); | ||
|
|
||
| if (agents && agents.length === 0) { | ||
| return ( | ||
| <ContentWrapper className="p-4"> | ||
| <CustomAgentsEmptyState organizationId={organizationId} /> | ||
| </ContentWrapper> | ||
| ); | ||
| if (count === 0) { | ||
| return <CustomAgentsEmptyState organizationId={organizationId} />; | ||
| } | ||
|
|
||
| return ( | ||
| <ContentWrapper> | ||
| <CustomAgentTable | ||
| organizationId={organizationId} | ||
| agents={agents ?? null} | ||
| isLoading={isLoading} | ||
| /> | ||
| </ContentWrapper> | ||
| ); | ||
| return <CustomAgentTable organizationId={organizationId} agents={agents} />; | ||
| } |
There was a problem hiding this comment.
ContentWrapper removed — potential layout regression
The previous code wrapped the page content in <ContentWrapper> / <ContentWrapper className="p-4"> for both the table and empty state. This refactor removes all ContentWrapper usage, which may cause layout inconsistencies with other pages that still use it. If this is intentional (e.g., handled at a higher route level), please confirm — otherwise the table and empty state may render without the expected padding/max-width constraints. Was the removal of ContentWrapper intentional here? The previous code used it for layout consistency.
Comment on lines
+213
to
+243
| const membershipsResult: BetterAuthFindManyResult<BetterAuthTeamMember> = | ||
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | ||
| model: 'teamMember', | ||
| paginationOpts: { cursor: null, numItems: 100 }, | ||
| where: [{ field: 'userId', operator: 'eq', value: authUser.userId }], | ||
| }); | ||
|
|
||
| if (!membershipsResult || membershipsResult.page.length === 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| let count = 0; | ||
| for (const membership of membershipsResult.page) { | ||
| const teamResult: BetterAuthFindManyResult<BetterAuthTeam> = | ||
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | ||
| model: 'team', | ||
| paginationOpts: { cursor: null, numItems: 1 }, | ||
| where: [ | ||
| { field: '_id', operator: 'eq', value: membership.teamId }, | ||
| { | ||
| field: 'organizationId', | ||
| operator: 'eq', | ||
| value: args.organizationId, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| if (teamResult && teamResult.page.length > 0) { | ||
| count++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sequential N+1 queries — consider parallelizing
This function fetches all team memberships, then iterates over each membership sequentially, issuing one findMany query per membership to check if the team belongs to the org. The existing getMyTeams function (line 249) already solves the same problem with Promise.all for parallel execution.
Consider using the same parallel pattern as getMyTeams:
const teamResults = await Promise.all(
membershipsResult.page.map((membership) =>
ctx.runQuery(components.betterAuth.adapter.findMany, {
model: 'team',
paginationOpts: { cursor: null, numItems: 1 },
where: [
{ field: '_id', operator: 'eq', value: membership.teamId },
{ field: 'organizationId', operator: 'eq', value: args.organizationId },
],
}),
),
);
return teamResults.filter((r) => r && r.page.length > 0).length;
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
📝 WalkthroughWalkthroughThis pull request refactors the platform's data loading and skeleton rendering patterns across numerous features. It introduces new UI components (CodeBlock, EmptyPlaceholder, PageSection, SectionHeader, StickySectionHeader) and systematically replaces hardcoded skeleton components with a data-driven approach. The backend Convex queries are renamed to the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
services/platform/app/features/settings/components/settings-navigation.tsx (1)
62-64: 🛠️ Refactor suggestion | 🟠 Major
canChangePasswordnow gates the entire account tab, not just password functionality.const navigationItems = allItems.filter( (item) => canChangePassword || item.labelKey !== 'account', );The prop is named
canChangePassword, but it controls whether the full Account navigation tab is visible. If the account settings page grows to include non-password content (profile details, preferences, notification settings, etc.), settingcanChangePassword=falsewill silently block users from all of it — not just the password section. The prop name also communicates nothing to callers about this side-effect.Consider renaming the prop (and its call-sites) to reflect its actual intent, or, if the filter is truly meant to hide the tab only when there is nothing to show there, drive it off a more semantically accurate prop:
♻️ Suggested rename
interface SettingsNavigationProps { organizationId: string; userRole?: string | null; - canChangePassword?: boolean; + showAccountTab?: boolean; } export function SettingsNavigation({ organizationId, userRole, - canChangePassword = true, + showAccountTab = true, }: SettingsNavigationProps) { ... const navigationItems = allItems.filter( - (item) => canChangePassword || item.labelKey !== 'account', + (item) => showAccountTab || item.labelKey !== 'account', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/settings/components/settings-navigation.tsx` around lines 62 - 64, The filter for navigationItems in settings-navigation.tsx uses the prop canChangePassword to hide the entire Account tab (item.labelKey === 'account') which is misleading; either rename the prop (e.g., canViewAccount or hasAccountTab) across its call-sites to reflect that it controls the whole Account tab, or change the filter to only hide password controls by checking item.id/labelKey for the password sub-item instead and drive tab visibility from a new semantic prop (e.g., hasAccountContent or canViewAccount) so that canChangePassword only gates password-specific functionality; update references to navigationItems, the canChangePassword prop, and any consumers accordingly.services/platform/app/features/approvals/components/approvals-client.tsx (1)
287-326: 🧹 Nitpick | 🔵 TrivialDuplicated empty-state JSX — consider extracting.
The
DataTableEmptyStateblock at lines 287–301 (approxCount === 0) and lines 312–326 (allApprovals.length === 0) render identical markup. Extract a shared helper or variable to reduce duplication.♻️ Proposed refactor
+ const emptyState = ( + <DataTableEmptyState + icon={GitCompare} + title={ + status === 'pending' + ? t('emptyState.pending.title') + : t('emptyState.resolved.title') + } + description={ + status === 'pending' ? t('emptyState.pending.description') : undefined + } + /> + ); + if (approxCount === 0) { - return ( - <DataTableEmptyState - icon={GitCompare} - title={ - status === 'pending' - ? t('emptyState.pending.title') - : t('emptyState.resolved.title') - } - description={ - status === 'pending' ? t('emptyState.pending.description') : undefined - } - /> - ); + return emptyState; } if (paginatedResult.status === 'LoadingFirstPage') { return ( <ApprovalsSkeleton status={status} rows={Math.min(approxCount ?? 10, 10)} /> ); } if (allApprovals.length === 0) { - return ( - <DataTableEmptyState - icon={GitCompare} - title={ - status === 'pending' - ? t('emptyState.pending.title') - : t('emptyState.resolved.title') - } - description={ - status === 'pending' ? t('emptyState.pending.description') : undefined - } - /> - ); + return emptyState; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/approvals/components/approvals-client.tsx` around lines 287 - 326, There are two identical DataTableEmptyState render blocks for the empty conditions (approxCount === 0 and allApprovals.length === 0); extract a shared helper (e.g., renderEmptyState or emptyStateElement) that returns the DataTableEmptyState with icon=GitCompare, title based on status via t('emptyState.*.title'), and conditional description for pending, then replace both duplicated JSX blocks with a single call to that helper; update usages in approvals-client.tsx (referencing approxCount, allApprovals, status, and t) so the logic is unchanged but duplication removed.services/platform/app/routes/dashboard/$id/automations/index.tsx (1)
63-89:⚠️ Potential issue | 🟠 MajorAccess-control check is bypassed when
count === 0.The render order is now:
count === 0→AutomationsEmptyState← new, line 67isMemberLoading || isAutomationsLoading→ Skeleton- Role check (admin/developer) →
AccessDenied← line 83automations.length === 0→AutomationsEmptyStateStep 1 runs before Step 3, so a member without the
adminordeveloperrole who visits an organization with zero automations will seeAutomationsEmptyState— including the "Create with AI" action menu linking to/dashboard/${organizationId}/chat— instead ofAccessDenied. This is a behavioral regression from the pre-PR logic, where empty-state rendering was gated behind the role check.Note that
approxCountAutomationsusesqueryWithRLS, so count may be 0 for a non-privileged user for two distinct reasons: (a) the org truly has no automations, or (b) RLS hides all automations from that user. In both cases the role check should run first.🛡️ Proposed fix — move the role check before the count-based empty-state guard
function AutomationsPage() { const { id: organizationId } = Route.useParams(); const { t } = useT('accessDenied'); const { data: memberContext, isLoading: isMemberLoading } = useCurrentMemberContext(organizationId); const { data: count } = useApproxAutomationCount(organizationId); const { automations, isLoading: isAutomationsLoading } = useAutomations(organizationId); - if (count === 0) { - return <AutomationsEmptyState organizationId={organizationId} />; - } - if (isMemberLoading || isAutomationsLoading) { return ( <ContentWrapper> <AutomationsTableSkeleton organizationId={organizationId} rows={Math.min(count ?? 10, 10)} /> </ContentWrapper> ); } const userRole = (memberContext?.role ?? '').toLowerCase(); if (userRole !== 'admin' && userRole !== 'developer') { return <AccessDenied message={t('automations')} />; } - if (!automations || automations.length === 0) { + if (count === 0 || !automations || automations.length === 0) { return <AutomationsEmptyState organizationId={organizationId} />; }This preserves the snappy skeleton UX (count already available from
ensureQueryData) while ensuring the role gate always fires before any automations UI — including the empty state — is rendered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/routes/dashboard/`$id/automations/index.tsx around lines 63 - 89, The access-control check must run before rendering any automations UI; move the role decision (reading memberContext?.role and checking for 'admin'/'developer') above the count-based empty-state guard so that useApproxAutomationCount/useAutomations can still be used but AutomationsEmptyState is not returned when a non-privileged user visits; ensure the check that returns <AccessDenied message={t('automations')} /> (based on userRole) occurs before the if (count === 0) return <AutomationsEmptyState ... /> and before the final automations.length === 0 empty-state branch.services/platform/app/routes/dashboard/$id/settings/teams.tsx (1)
58-62:⚠️ Potential issue | 🟡 Minor
count === 0check has no guard forundefined— wrong branch on transient refetchWhen
countisundefined(e.g., the query is re-executing after a cache invalidation post-mount),count === 0isfalseand the component falls through to<TeamsTable teams={undefined} />instead of<TeamsEmptyState />. The loader'sensureQueryDatacovers the initial render, but a transient refetch can cause a brief flash of the table before the empty state.Adding a separate loading guard for
countprevents the flash:🛡️ Proposed fix: guard `count` independently
+ const { data: count, isLoading: isCountLoading } = useApproxTeamCount(organizationId); - const { data: count } = useApproxTeamCount(organizationId); if (isMemberLoading) { return ( <TeamsTableSkeleton organizationId={organizationId} rows={Math.min(count ?? 10, 10)} /> ); } if (!memberContext || !memberContext.isAdmin) { return <AccessDenied message={t('teams')} />; } + if (isCountLoading) { + return <TeamsTableSkeleton organizationId={organizationId} rows={10} />; + } if (count === 0) { return <TeamsEmptyState organizationId={organizationId} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/routes/dashboard/`$id/settings/teams.tsx around lines 58 - 62, The component falls through to rendering <TeamsTable teams={teams} /> when count is undefined during a transient refetch; add an explicit guard that returns a loading placeholder (or null) when count === undefined before checking count === 0 so you never render TeamsTable with teams === undefined. Update the conditional flow around the count variable (the block that currently checks `if (count === 0) { return <TeamsEmptyState organizationId={organizationId} />; }`) to first handle `if (count === undefined) { return null /* or a loader */; }` then keep the `if (count === 0)` empty-state branch and finally return `<TeamsTable teams={teams} organizationId={organizationId} />`.services/platform/app/features/tone-of-voice/components/example-messages-table.tsx (1)
145-176: 🧹 Nitpick | 🔵 TrivialOptional: move
actioncomputation after the empty-state early return.
actionis alwaysundefinedwhenexamples.length === 0and is never consumed in the early-return branch. Placing its definition after the guard makes the data-flow clearer.♻️ Suggested reorder
- const action = - examples.length > 0 ? ( - <Button onClick={onAddExample}> - <Plus className="mr-2 size-4" /> - {tTone('exampleMessages.addButton')} - </Button> - ) : undefined; - if (examples.length === 0) { return ( <PageSection ...> <DataTableEmptyState ... /> </PageSection> ); } + const action = ( + <Button onClick={onAddExample}> + <Plus className="mr-2 size-4" /> + {tTone('exampleMessages.addButton')} + </Button> + ); + return ( <PageSection ... action={action} ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/tone-of-voice/components/example-messages-table.tsx` around lines 145 - 176, The local variable action is computed before the early-return but is unused when examples.length === 0; move the action computation (the ternary that references action, examples, and onAddExample) to after the empty-state guard so the early-return branch that renders PageSection/DataTableEmptyState/DataTableActionMenu doesn't compute it unnecessarily and the non-empty rendering can then use action as needed.services/platform/app/routes/dashboard/$id/settings/api-keys.tsx (1)
36-55:⚠️ Potential issue | 🟠 MajorAdd
isLoadingto the skeleton guard to cover the apiKeys loading phase.The race condition is real:
member context(prefetched via Convex) resolves faster than the externalauthClient.apiKey.list()call. During this window:
isMemberLoadingbecomesfalse→ skeleton hidesisLoading(apiKeys) is stilltrue→ empty state guard is skipped (line 51)ApiKeysTablerenders withapiKeys={undefined}While
useListPagegracefully convertsundefinedto[], causingDataTableto display its own empty state instead of the intended skeleton, this creates a UX regression if the previousApiKeysContentcomponent had its own skeleton guard.Update the skeleton guard to cover both loading states:
Proposed fix
- if (isMemberLoading) { + if (isMemberLoading || isLoading) { return <ApiKeysTableSkeleton organizationId={organizationId} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/routes/dashboard/`$id/settings/api-keys.tsx around lines 36 - 55, The skeleton hides too early because only isMemberLoading is checked; update the guard so the component shows ApiKeysTableSkeleton while either member context or api keys are loading by changing the condition that renders ApiKeysTableSkeleton (currently using isMemberLoading) to include isLoading as well (e.g., if (isMemberLoading || isLoading) return <ApiKeysTableSkeleton organizationId={organizationId} />), leaving the subsequent memberContext, hasAccess, and ApiKeysEmptyState/ApiKeysTable logic intact to avoid rendering ApiKeysTable with apiKeys undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/data-display/code-block.stories.tsx`:
- Around line 90-100: The first CodeBlock in the MultipleSections story renders
a broken line break in its children (producing "Content-Type:\n
application/json") while its copyValue is a single-line string; update the
CodeBlock children so the displayed text exactly matches copyValue—e.g., replace
the multi-line JSX text node with a single string literal or a JS expression
('curl -X POST https://api.example.com/webhook -H \'Content-Type:
application/json\'') so the rendered <pre> content and the copyValue for
CodeBlock are identical.
In `@services/platform/app/components/ui/data-display/code-block.test.tsx`:
- Around line 25-39: Add a test in code-block.test.tsx that renders <CodeBlock
copyValue="code"> without copyLabel and assert that the rendered button has a
valid accessible name (e.g., getByRole('button', { name: /copy/i })) or that no
button is rendered; then update the CodeBlock implementation in code-block.tsx
so it no longer renders a button with aria-label={undefined} by either supplying
a sensible default label (e.g., "Copy") when copyValue is present or by making
copyLabel required whenever copyValue is provided (update the component
props/type and the render logic in the CodeBlock component to enforce this).
In `@services/platform/app/components/ui/data-display/code-block.tsx`:
- Around line 45-48: The right-padding for the copy button is applied
unconditionally on the <pre> element (the class string containing "pr-10") while
the wrapper div uses copyValue && 'pr-0' which does nothing; update the
conditional so padding is only added when copyValue is present. Concretely,
modify the wrapper div and/or pre className that use the cn utility: remove the
unconditional "pr-10" from the <pre> and instead apply "pr-10" conditionally
(e.g. cn('bg-muted rounded-md p-3 font-mono text-xs break-all
whitespace-pre-wrap', copyValue && 'pr-10')) or keep "pr-10" on the wrapper by
changing cn('group relative', copyValue && 'pr-0') to cn('group relative',
copyValue && 'pr-10'); use the copyValue prop and the cn calls to ensure padding
is only present when a copy button will be rendered.
- Around line 27-36: The handleCopy's setTimeout handle isn't cleared on
unmount; store the timeout id in a ref (e.g., timerRef) inside the
DataDisplay/CodeBlock component, assign timerRef.current when calling setTimeout
in handleCopy, clear any existing timer before setting a new one, and add a
cleanup useEffect that clears timerRef.current on unmount to avoid leaked timers
and stale state updates; adjust dependencies to include copyValue if needed.
- Around line 49-62: The copy button can end up with no accessible name because
aria-label={copyLabel} may be undefined while the button only contains icon
components (Copy/Check); update the Button rendering in code-block.tsx to ensure
a computable accessible name by using a fallback when copyLabel is absent (e.g.,
compute const label = copyLabel ?? 'Copy' and pass aria-label={label}) or
alternatively change the props type to a discriminated union that requires
copyLabel whenever copyValue is provided; update uses of copyValue/copyLabel,
and ensure handleCopy stays the click handler and the icon components
(Copy/Check) remain present.
In `@services/platform/app/components/ui/feedback/empty-placeholder.tsx`:
- Around line 9-31: The EmptyPlaceholder component currently renders its
children inside a <p> which is invalid if children include block-level
ReactNodes; update the JSX in EmptyPlaceholder to wrap {children} in a <div>
(preserving the existing className "text-muted-foreground text-sm") instead of a
<p> so arbitrary ReactNode content is safe, keep the prop type children:
ReactNode unchanged, and ensure the Icon rendering and outer container
classNames (in EmptyPlaceholder) remain intact.
In `@services/platform/app/components/ui/layout/section-header.test.tsx`:
- Around line 68-74: The size variant tests for SectionHeader only assert the
heading renders but don't verify the size CSS class; update the it.each block
that renders SectionHeader (in section-header.test.tsx) to assert the applied
class for each size ('sm' -> 'text-sm', 'base' -> 'text-base', 'lg' ->
'text-lg') by selecting the heading with screen.getByRole('heading', { level: 2
}) and checking its class (e.g., using toHaveClass or inspecting
element.className) to ensure the correct text-* class is present.
In `@services/platform/app/components/ui/layout/section-header.tsx`:
- Around line 28-40: SectionHeaderProps is currently internal-only which forces
consumers to rely on ComponentProps<typeof SectionHeader>; export the interface
so it can be reused directly by consumers. Update the file to add an export for
SectionHeaderProps (the existing interface) so callers can import
SectionHeaderProps alongside the SectionHeader component; ensure the exported
name matches the interface exactly and that any related types (e.g., references
to titleVariants or VariantProps usage) remain unchanged so the public type
stays consistent with the component props.
In
`@services/platform/app/components/ui/layout/sticky-section-header.stories.tsx`:
- Around line 6-11: Add a new Storybook story named "Scrollable" alongside the
existing meta for StickySectionHeader that demonstrates the sticky top-[49px]
behavior by providing args (e.g., title and description), a decorator that
renders the Story inside a fixed-height, overflow-y-auto container (so the
header can stick while the inner content scrolls), and story parameters/docs
describing its purpose; update the same file's export list so the new Scrollable
story is exported and visible in the Storybook UI.
In `@services/platform/app/components/ui/layout/sticky-section-header.tsx`:
- Around line 23-38: The sticky offsets are hardcoded in the div class
(top-[49px], md:top-[97px]) making StickySectionHeader brittle; replace those
tokens with CSS custom-property references (e.g., top-[var(--nav-height)] and
md:top-[var(--nav-height-md)]) or Tailwind config tokens and update the layout
root to expose those --nav-height variables so the component uses the single
source of truth; update the className in sticky-section-header.tsx and ensure
any top token references are consistent with the nav component that sets the CSS
variables.
- Around line 23-38: The outer wrapper div (the element with ref={ref} and the
cn(...) className) includes a redundant "justify-between" utility; remove
"justify-between" from that class list since SectionHeader (the only child)
already handles its own layout (including justify-between when action is
present) to keep the class list intentional and avoid misleading styles.
In
`@services/platform/app/features/custom-agents/components/custom-agent-table.tsx`:
- Line 56: CustomAgentTable currently calls
useApproxCustomAgentCount(organizationId) even though index.tsx also subscribes
to the same approxCountCustomAgents query; to make the data flow explicit and
avoid duplicate subscriptions in code, add an optional prop (e.g., count?:
number) to the CustomAgentTable component and change the component to use the
passed-in count when provided, otherwise fall back to calling
useApproxCustomAgentCount(organizationId). Update the CustomAgentTable signature
and any call sites in index.tsx to pass the count if available, leaving the
internal hook as a fallback.
In
`@services/platform/app/features/custom-agents/components/custom-agent-webhook-section.tsx`:
- Around line 344-352: The CodeBlock copy buttons inside the usageExamples map
are using the wrong i18n key (copyLabel={t('customAgents.webhook.copyUrl')})
which will announce "Copy URL" for curl examples; update the copyLabel prop on
the CodeBlock instances to use a generic or dedicated copy key (e.g.,
t('common.copy') or t('customAgents.webhook.copyExample')) and add the matching
translation if needed so the aria-label correctly reads "Copy" (or "Copy
example") instead of "Copy URL".
In `@services/platform/app/features/documents/components/documents-client.tsx`:
- Around line 96-97: The documents route loader is missing a preload for the
approxCountDocuments query: add await
context.queryClient.ensureQueryData(['approxCountDocuments', organizationId], ()
=> fetchApproxCountDocuments(organizationId)) inside the loader used for
documents so useApproxDocumentCount(organizationId) does not return undefined
initially; additionally, in the DocumentsClient component where docCount is used
(the useApproxDocumentCount hook and the skeleton row calculation around the
Math.min call), clamp the skeleton rows to at least 1 (e.g., rows = Math.max(1,
Math.min(docCount ?? 10, 10)) or equivalent) so a zero count doesn’t render a
headerless zero-row skeleton.
In
`@services/platform/app/features/documents/components/onedrive-import/onedrive-picker-stage.tsx`:
- Around line 27-53: SourceTabButton is being used as a tab but lacks ARIA tab
semantics; update the SourceTabButton component to include role="tab",
aria-selected={active}, and a focusable tabIndex (e.g., tabIndex={active ? 0 :
-1}) and ensure it supplies an aria-controls id that matches the tab panel; also
add role="tablist" to the wrapping HStack so assistive tech recognizes the
group. Target the SourceTabButton function to add role, aria-selected, tabIndex
and aria-controls attributes and update the HStack wrapping the tabs to include
role="tablist".
In `@services/platform/app/features/products/components/product-table.tsx`:
- Around line 107-111: The emptyState prop is being used for both the "no items"
and "no results due to filters" cases, which can confuse users; update the
product table to let DataTable show a distinct message when filters are active
by either (A) passing the actual hasActiveFilters flag into DataTable (prop name
hasActiveFilters or similar) so it can render its built-in filtered-empty UI, or
(B) supply a separate emptyFilteredState prop or conditional empty state content
when filters are active—locate the emptyState usage in product-table.tsx and the
DataTable component to implement the chosen approach (references: emptyState
prop in product-table.tsx and the DataTable component/prop hasActiveFilters).
In `@services/platform/app/features/settings/teams/hooks/queries.ts`:
- Around line 22-25: Restore the previous null sentinel by returning teams: data
?? null from the useTeams hook (replace the current teams: data return) so the
hook yields Team[] | null like useListTeams did; update the return typing if
necessary and ensure callers relying on falsy checks continue to work with the
restored teams value.
In `@services/platform/app/features/vendors/components/vendors-table.tsx`:
- Around line 39-40: VendorsTable currently calls
useApproxVendorCount(organizationId) internally; instead accept a new prop
skeletonRows (number) so the parent computes the count once (e.g., in
VendorsPage/vendors.tsx) and passes Math.min(count ?? 10, 10) down; remove or
stop using useApproxVendorCount inside VendorsTable (and any duplicate calls at
other locations like lines ~140-141), update VendorsTable’s props/interface to
include skeletonRows, and update VendorsPage/vendors.tsx to compute and pass
skeletonRows when rendering VendorsTable so tests and alternate count sources
can be used without re-querying.
In `@services/platform/app/features/websites/components/websites-table.tsx`:
- Around line 37-38: approximate count resolving to 0 during initial load causes
skeletonRows to be 0 and the table shows a blank area; update WebsitesTable to
treat a zero approximate count as "unknown" while the paginated list is still
loading by using the useApproxWebsiteCount loading/ fetching flag (from
useApproxWebsiteCount) or checking listWebsitesPaginated's loading state and
falling back to a default positive skeleton count (e.g. pageSize or 5) instead
of using count directly; change the logic that computes skeletonRows in
websites-table.tsx so when count === 0 && isLoading (or isFetching) you return
the fallback value, otherwise use the actual count, referencing
useApproxWebsiteCount, listWebsitesPaginated, and the skeletonRows computation.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/tone-of-voice.tsx:
- Around line 96-98: The conditional uses an unreachable fallback "exampleCount
?? 5" when rendering ExampleMessagesSkeleton because the preceding guard
"(exampleCount ?? 0) > 0" guarantees exampleCount is a positive number; replace
the fallback with a non-null assertion or direct reference (e.g., use
Math.min(exampleCount!, 5) or Math.min(exampleCount as number, 5)) so
ExampleMessagesSkeleton receives a clear, type-safe row count and remove the
confusing "?? 5" fallback.
In
`@services/platform/app/routes/dashboard/`$id/custom-agents/$agentId/instructions.tsx:
- Around line 126-136: The Select components are creating redundant
subscriptions by calling form.watch('...') for individual fields; replace those
calls with the already-watched formValues object (e.g., use
formValues.modelPreset instead of form.watch('modelPreset') in the Select value)
and similarly swap form.watch(...) usages for the other fields referenced around
the same block (the other Selects at ~147 and ~155) to avoid duplicate
subscriptions while keeping the existing onValueChange/form.setValue logic
intact.
- Around line 155-161: The CodeBlock rendering under the
form.watch('filePreprocessingEnabled') branch displays
FILE_PREPROCESSING_INSTRUCTIONS but doesn’t pass copyValue, so no copy button
appears; update the CodeBlock call in that branch to include
copyValue={FILE_PREPROCESSING_INSTRUCTIONS} (and optionally copyLabel="Copy
prompt" or a localized label) so users can copy the injected system prompt;
ensure the prop name matches the other webhook CodeBlock usages for consistent
behavior.
In `@services/platform/app/routes/dashboard/`$id/settings/teams.tsx:
- Around line 21-32: The loader currently awaits
context.queryClient.ensureQueryData(convexQuery(api.members.queries.approxCountMyTeams,
{ organizationId: params.id })) which will reject on network/Convex errors and
surface an error boundary; wrap that ensureQueryData call in a try-catch inside
the loader (catch only around the ensureQueryData call for
api.members.queries.approxCountMyTeams), log or ignore the error, and fall back
to a safe default (e.g., leave no data or set a small default count) so the
route continues to render; keep the prefetchQuery for getMyTeams unchanged and
only make the approxCountMyTeams call resilient.
In `@services/platform/convex/documents/queries.ts`:
- Around line 18-26: approxCountDocuments currently skips auth and
org-membership checks; update its handler to call getAuthUserIdentity(ctx) and
then verify membership via getOrganizationMember(ctx.db, auth.userId,
args.organizationId), returning 0 if either check fails, and only call
countItemsInOrg(ctx.db, 'documents', args.organizationId) when the user is
authenticated and a member; reference the symbols approxCountDocuments,
getAuthUserIdentity, getOrganizationMember, and countItemsInOrg when making the
change.
In `@services/platform/convex/members/queries.ts`:
- Around line 224-243: The loop in approxCountMyTeams performs ctx.runQuery
(components.betterAuth.adapter.findMany) sequentially for each membership in
membershipsResult.page causing N+1 blocking latency; change it to mirror
getMyTeams by mapping membershipsResult.page to an array of promises that call
ctx.runQuery(...) for each membership.teamId, await Promise.all on that array,
then compute count by iterating the resolved results (counting those with
page.length > 0); ensure you still use the same query shape (model: 'team',
where checks for _id and organizationId) and replace the sequential increment
logic with counting over the parallel results.
---
Outside diff comments:
In `@services/platform/app/features/approvals/components/approvals-client.tsx`:
- Around line 287-326: There are two identical DataTableEmptyState render blocks
for the empty conditions (approxCount === 0 and allApprovals.length === 0);
extract a shared helper (e.g., renderEmptyState or emptyStateElement) that
returns the DataTableEmptyState with icon=GitCompare, title based on status via
t('emptyState.*.title'), and conditional description for pending, then replace
both duplicated JSX blocks with a single call to that helper; update usages in
approvals-client.tsx (referencing approxCount, allApprovals, status, and t) so
the logic is unchanged but duplication removed.
In `@services/platform/app/features/settings/components/settings-navigation.tsx`:
- Around line 62-64: The filter for navigationItems in settings-navigation.tsx
uses the prop canChangePassword to hide the entire Account tab (item.labelKey
=== 'account') which is misleading; either rename the prop (e.g., canViewAccount
or hasAccountTab) across its call-sites to reflect that it controls the whole
Account tab, or change the filter to only hide password controls by checking
item.id/labelKey for the password sub-item instead and drive tab visibility from
a new semantic prop (e.g., hasAccountContent or canViewAccount) so that
canChangePassword only gates password-specific functionality; update references
to navigationItems, the canChangePassword prop, and any consumers accordingly.
In
`@services/platform/app/features/tone-of-voice/components/example-messages-table.tsx`:
- Around line 145-176: The local variable action is computed before the
early-return but is unused when examples.length === 0; move the action
computation (the ternary that references action, examples, and onAddExample) to
after the empty-state guard so the early-return branch that renders
PageSection/DataTableEmptyState/DataTableActionMenu doesn't compute it
unnecessarily and the non-empty rendering can then use action as needed.
In `@services/platform/app/routes/dashboard/`$id/automations/index.tsx:
- Around line 63-89: The access-control check must run before rendering any
automations UI; move the role decision (reading memberContext?.role and checking
for 'admin'/'developer') above the count-based empty-state guard so that
useApproxAutomationCount/useAutomations can still be used but
AutomationsEmptyState is not returned when a non-privileged user visits; ensure
the check that returns <AccessDenied message={t('automations')} /> (based on
userRole) occurs before the if (count === 0) return <AutomationsEmptyState ...
/> and before the final automations.length === 0 empty-state branch.
In `@services/platform/app/routes/dashboard/`$id/settings/api-keys.tsx:
- Around line 36-55: The skeleton hides too early because only isMemberLoading
is checked; update the guard so the component shows ApiKeysTableSkeleton while
either member context or api keys are loading by changing the condition that
renders ApiKeysTableSkeleton (currently using isMemberLoading) to include
isLoading as well (e.g., if (isMemberLoading || isLoading) return
<ApiKeysTableSkeleton organizationId={organizationId} />), leaving the
subsequent memberContext, hasAccess, and ApiKeysEmptyState/ApiKeysTable logic
intact to avoid rendering ApiKeysTable with apiKeys undefined.
In `@services/platform/app/routes/dashboard/`$id/settings/teams.tsx:
- Around line 58-62: The component falls through to rendering <TeamsTable
teams={teams} /> when count is undefined during a transient refetch; add an
explicit guard that returns a loading placeholder (or null) when count ===
undefined before checking count === 0 so you never render TeamsTable with teams
=== undefined. Update the conditional flow around the count variable (the block
that currently checks `if (count === 0) { return <TeamsEmptyState
organizationId={organizationId} />; }`) to first handle `if (count ===
undefined) { return null /* or a loader */; }` then keep the `if (count === 0)`
empty-state branch and finally return `<TeamsTable teams={teams}
organizationId={organizationId} />`.
Comment on lines
+90
to
+100
| export const MultipleSections: Story = { | ||
| render: () => ( | ||
| <Stack gap={4} className="max-w-lg"> | ||
| <CodeBlock | ||
| label="Request" | ||
| copyValue="curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'" | ||
| copyLabel="Copy request" | ||
| > | ||
| curl -X POST https://api.example.com/webhook -H 'Content-Type: | ||
| application/json' | ||
| </CodeBlock> |
Contributor
There was a problem hiding this comment.
MultipleSections: copied value diverges from displayed children for the first CodeBlock.
The JSX line break at line 99 causes a literal newline + leading spaces to appear in the <pre> output (Content-Type:\n application/json), while copyValue (line 95) is a single-line string. The copy button would produce a string that doesn't match what's visible, undermining the story's demonstration.
🐛 Suggested fix — use a string literal to avoid implicit whitespace
- <CodeBlock
- label="Request"
- copyValue="curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'"
- copyLabel="Copy request"
- >
- curl -X POST https://api.example.com/webhook -H 'Content-Type:
- application/json'
- </CodeBlock>
+ <CodeBlock
+ label="Request"
+ copyValue="curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'"
+ copyLabel="Copy request"
+ >
+ {"curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'"}
+ </CodeBlock>📝 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.
Suggested change
| export const MultipleSections: Story = { | |
| render: () => ( | |
| <Stack gap={4} className="max-w-lg"> | |
| <CodeBlock | |
| label="Request" | |
| copyValue="curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'" | |
| copyLabel="Copy request" | |
| > | |
| curl -X POST https://api.example.com/webhook -H 'Content-Type: | |
| application/json' | |
| </CodeBlock> | |
| export const MultipleSections: Story = { | |
| render: () => ( | |
| <Stack gap={4} className="max-w-lg"> | |
| <CodeBlock | |
| label="Request" | |
| copyValue="curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'" | |
| copyLabel="Copy request" | |
| > | |
| {"curl -X POST https://api.example.com/webhook -H 'Content-Type: application/json'"} | |
| </CodeBlock> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/code-block.stories.tsx`
around lines 90 - 100, The first CodeBlock in the MultipleSections story renders
a broken line break in its children (producing "Content-Type:\n
application/json") while its copyValue is a single-line string; update the
CodeBlock children so the displayed text exactly matches copyValue—e.g., replace
the multi-line JSX text node with a single string literal or a JS expression
('curl -X POST https://api.example.com/webhook -H \'Content-Type:
application/json\'') so the rendered <pre> content and the copyValue for
CodeBlock are identical.
Comment on lines
+25
to
+39
| it('shows copy button when copyValue is provided', () => { | ||
| render( | ||
| <CodeBlock copyValue="code" copyLabel="Copy code"> | ||
| code | ||
| </CodeBlock>, | ||
| ); | ||
| expect( | ||
| screen.getByRole('button', { name: 'Copy code' }), | ||
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('hides copy button when copyValue is not provided', () => { | ||
| render(<CodeBlock>code</CodeBlock>); | ||
| expect(screen.queryByRole('button')).not.toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing test coverage for copyValue without copyLabel — silent accessibility violation
When copyValue is set but copyLabel is omitted, the implementation renders <Button aria-label={undefined}>. This produces a button with no accessible name, which fails the axe button-name rule. The current test suite doesn't cover this combination, so the regression goes undetected.
Add a test and consider enforcing the pairing at the type level:
+ it('passes axe audit when copyValue is provided without copyLabel', async () => {
+ // Should either NOT render the copy button, or provide a default aria-label
+ const { container } = render(
+ <CodeBlock copyValue="curl -X POST">curl -X POST https://example.com</CodeBlock>,
+ );
+ await checkAccessibility(container);
+ });Additionally, in code-block.tsx, consider requiring copyLabel whenever copyValue is provided:
- interface CodeBlockProps {
- copyValue?: string;
- copyLabel?: string;
- // ...
- }
+ type CodeBlockProps = {
+ children: React.ReactNode;
+ label?: string;
+ className?: string;
+ } & (
+ | { copyValue: string; copyLabel: string }
+ | { copyValue?: never; copyLabel?: never }
+ );Also applies to: 49-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/code-block.test.tsx` around
lines 25 - 39, Add a test in code-block.test.tsx that renders <CodeBlock
copyValue="code"> without copyLabel and assert that the rendered button has a
valid accessible name (e.g., getByRole('button', { name: /copy/i })) or that no
button is rendered; then update the CodeBlock implementation in code-block.tsx
so it no longer renders a button with aria-label={undefined} by either supplying
a sensible default label (e.g., "Copy") when copyValue is present or by making
copyLabel required whenever copyValue is provided (update the component
props/type and the render logic in the CodeBlock component to enforce this).
Comment on lines
+27
to
+36
| const handleCopy = useCallback(async () => { | ||
| if (!copyValue) return; | ||
| try { | ||
| await navigator.clipboard.writeText(copyValue); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| } catch { | ||
| // Clipboard API not available | ||
| } | ||
| }, [copyValue]); |
Contributor
There was a problem hiding this comment.
Minor: setTimeout handle not cleared on unmount.
If the component unmounts within the 2-second window after a copy, the timer fires harmlessly (React 18 silently drops the state update), but the timer itself isn't GC'd until it fires. Use a useRef to store the handle and clear it on cleanup.
♻️ Proposed fix
- const [copied, setCopied] = useState(false);
+ const [copied, setCopied] = useState(false);
+ const timeoutRef = useRef<ReturnType<typeof setTimeout>>(undefined);
const handleCopy = useCallback(async () => {
if (!copyValue) return;
try {
await navigator.clipboard.writeText(copyValue);
setCopied(true);
- setTimeout(() => setCopied(false), 2000);
+ clearTimeout(timeoutRef.current);
+ timeoutRef.current = setTimeout(() => setCopied(false), 2000);
} catch {
// Clipboard API not available
}
}, [copyValue]);Also add a cleanup effect:
+ useEffect(() => () => clearTimeout(timeoutRef.current), []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/code-block.tsx` around lines
27 - 36, The handleCopy's setTimeout handle isn't cleared on unmount; store the
timeout id in a ref (e.g., timerRef) inside the DataDisplay/CodeBlock component,
assign timerRef.current when calling setTimeout in handleCopy, clear any
existing timer before setting a new one, and add a cleanup useEffect that clears
timerRef.current on unmount to avoid leaked timers and stale state updates;
adjust dependencies to include copyValue if needed.
Comment on lines
+45
to
+48
| <div className={cn('group relative', copyValue && 'pr-0')}> | ||
| <pre className="bg-muted rounded-md p-3 pr-10 font-mono text-xs break-all whitespace-pre-wrap"> | ||
| {children} | ||
| </pre> |
Contributor
There was a problem hiding this comment.
Layout: pr-10 on <pre> is unconditional; copyValue && 'pr-0' on the wrapper div is a no-op.
pr-10 reserves space for the copy button regardless of whether one is rendered, adding unnecessary right-padding when copyValue is absent. The pr-0 on the wrapper div has no effect because the wrapper carries no right-padding by default.
🐛 Proposed fix
- <div className={cn('group relative', copyValue && 'pr-0')}>
- <pre className="bg-muted rounded-md p-3 pr-10 font-mono text-xs break-all whitespace-pre-wrap">
+ <div className="group relative">
+ <pre className={cn('bg-muted rounded-md p-3 font-mono text-xs break-all whitespace-pre-wrap', copyValue && 'pr-10')}>📝 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.
Suggested change
| <div className={cn('group relative', copyValue && 'pr-0')}> | |
| <pre className="bg-muted rounded-md p-3 pr-10 font-mono text-xs break-all whitespace-pre-wrap"> | |
| {children} | |
| </pre> | |
| <div className="group relative"> | |
| <pre className={cn('bg-muted rounded-md p-3 font-mono text-xs break-all whitespace-pre-wrap', copyValue && 'pr-10')}> | |
| {children} | |
| </pre> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/code-block.tsx` around lines
45 - 48, The right-padding for the copy button is applied unconditionally on the
<pre> element (the class string containing "pr-10") while the wrapper div uses
copyValue && 'pr-0' which does nothing; update the conditional so padding is
only added when copyValue is present. Concretely, modify the wrapper div and/or
pre className that use the cn utility: remove the unconditional "pr-10" from the
<pre> and instead apply "pr-10" conditionally (e.g. cn('bg-muted rounded-md p-3
font-mono text-xs break-all whitespace-pre-wrap', copyValue && 'pr-10')) or keep
"pr-10" on the wrapper by changing cn('group relative', copyValue && 'pr-0') to
cn('group relative', copyValue && 'pr-10'); use the copyValue prop and the cn
calls to ensure padding is only present when a copy button will be rendered.
Comment on lines
+49
to
+62
| {copyValue && ( | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="absolute top-1.5 right-1.5 opacity-0 transition-opacity group-hover:opacity-100" | ||
| onClick={handleCopy} | ||
| aria-label={copyLabel} | ||
| > | ||
| {copied ? ( | ||
| <Check className="size-3.5 text-green-500" /> | ||
| ) : ( | ||
| <Copy className="size-3.5" /> | ||
| )} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Accessibility: copy button may have no accessible name when copyLabel is omitted.
copyLabel is optional, so aria-label={copyLabel} can be undefined. The button's only content is an SVG icon — lucide-react SVGs don't include a <title>, so the button would have no computable accessible name, failing WCAG 2.4.6.
Either make copyLabel required when copyValue is supplied (discriminated union), or add a non-i18n fallback:
🛡️ Option A — discriminated-union props (recommended)
-interface CodeBlockProps {
- children: ReactNode;
- label?: string;
- copyValue?: string;
- copyLabel?: string;
- className?: string;
-}
+type CodeBlockProps = {
+ children: ReactNode;
+ label?: string;
+ className?: string;
+} & (
+ | { copyValue?: never; copyLabel?: never }
+ | { copyValue: string; copyLabel: string }
+);🛡️ Option B — inline fallback (simpler)
- aria-label={copyLabel}
+ aria-label={copyLabel ?? '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.
Suggested change
| {copyValue && ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="absolute top-1.5 right-1.5 opacity-0 transition-opacity group-hover:opacity-100" | |
| onClick={handleCopy} | |
| aria-label={copyLabel} | |
| > | |
| {copied ? ( | |
| <Check className="size-3.5 text-green-500" /> | |
| ) : ( | |
| <Copy className="size-3.5" /> | |
| )} | |
| </Button> | |
| {copyValue && ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="absolute top-1.5 right-1.5 opacity-0 transition-opacity group-hover:opacity-100" | |
| onClick={handleCopy} | |
| aria-label={copyLabel ?? 'Copy'} | |
| > | |
| {copied ? ( | |
| <Check className="size-3.5 text-green-500" /> | |
| ) : ( | |
| <Copy className="size-3.5" /> | |
| )} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/code-block.tsx` around lines
49 - 62, The copy button can end up with no accessible name because
aria-label={copyLabel} may be undefined while the button only contains icon
components (Copy/Check); update the Button rendering in code-block.tsx to ensure
a computable accessible name by using a fallback when copyLabel is absent (e.g.,
compute const label = copyLabel ?? 'Copy' and pass aria-label={label}) or
alternatively change the props type to a discriminated union that requires
copyLabel whenever copyValue is provided; update uses of copyValue/copyLabel,
and ensure handleCopy stays the click handler and the icon components
(Copy/Check) remain present.
Comment on lines
+126
to
+136
| <Select | ||
| options={modelOptions} | ||
| label={t('customAgents.form.modelPreset')} | ||
| value={form.watch('modelPreset')} | ||
| onValueChange={(val) => | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- Select value is constrained to MODEL_PRESET_OPTIONS | ||
| form.setValue('modelPreset', val as ModelPreset) | ||
| } | ||
| required | ||
| disabled={isReadOnly} | ||
| /> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant form.watch() subscriptions — use already-watched formValues instead.
formValues at line 75 is already form.watch() (all fields). Lines 129, 147, and 155 call form.watch('fieldName') again, registering separate subscriptions.
♻️ Proposed change
- value={form.watch('modelPreset')}
+ value={formValues.modelPreset}- <Switch
- checked={form.watch('filePreprocessingEnabled')}
+ <Switch
+ checked={formValues.filePreprocessingEnabled}- {form.watch('filePreprocessingEnabled') && (
+ {formValues.filePreprocessingEnabled && (Also applies to: 147-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/routes/dashboard/`$id/custom-agents/$agentId/instructions.tsx
around lines 126 - 136, The Select components are creating redundant
subscriptions by calling form.watch('...') for individual fields; replace those
calls with the already-watched formValues object (e.g., use
formValues.modelPreset instead of form.watch('modelPreset') in the Select value)
and similarly swap form.watch(...) usages for the other fields referenced around
the same block (the other Selects at ~147 and ~155) to avoid duplicate
subscriptions while keeping the existing onValueChange/form.setValue logic
intact.
Comment on lines
+155
to
+161
| {form.watch('filePreprocessingEnabled') && ( | ||
| <CodeBlock | ||
| label={t('customAgents.form.filePreprocessingInjectedPrompt')} | ||
| > | ||
| {FILE_PREPROCESSING_INSTRUCTIONS} | ||
| </CodeBlock> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CodeBlock for FILE_PREPROCESSING_INSTRUCTIONS has no copyValue — no copy button rendered.
Every CodeBlock in the webhook section passes copyValue to enable copying. Here the injected system prompt is displayed read-only, which is inconsistent given users may want to inspect or reference the exact prompt text. Consider adding copyValue={FILE_PREPROCESSING_INSTRUCTIONS} and a copyLabel if copy-ability is desirable.
♻️ Proposed change
<CodeBlock
label={t('customAgents.form.filePreprocessingInjectedPrompt')}
+ copyValue={FILE_PREPROCESSING_INSTRUCTIONS}
+ copyLabel={t('common.copy')}
>
{FILE_PREPROCESSING_INSTRUCTIONS}
</CodeBlock>📝 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.
Suggested change
| {form.watch('filePreprocessingEnabled') && ( | |
| <CodeBlock | |
| label={t('customAgents.form.filePreprocessingInjectedPrompt')} | |
| > | |
| {FILE_PREPROCESSING_INSTRUCTIONS} | |
| </CodeBlock> | |
| )} | |
| {form.watch('filePreprocessingEnabled') && ( | |
| <CodeBlock | |
| label={t('customAgents.form.filePreprocessingInjectedPrompt')} | |
| copyValue={FILE_PREPROCESSING_INSTRUCTIONS} | |
| copyLabel={t('common.copy')} | |
| > | |
| {FILE_PREPROCESSING_INSTRUCTIONS} | |
| </CodeBlock> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/routes/dashboard/`$id/custom-agents/$agentId/instructions.tsx
around lines 155 - 161, The CodeBlock rendering under the
form.watch('filePreprocessingEnabled') branch displays
FILE_PREPROCESSING_INSTRUCTIONS but doesn’t pass copyValue, so no copy button
appears; update the CodeBlock call in that branch to include
copyValue={FILE_PREPROCESSING_INSTRUCTIONS} (and optionally copyLabel="Copy
prompt" or a localized label) so users can copy the injected system prompt;
ensure the prop name matches the other webhook CodeBlock usages for consistent
behavior.
Comment on lines
+21
to
+32
| loader: async ({ context, params }) => { | ||
| void context.queryClient.prefetchQuery( | ||
| convexQuery(api.members.queries.getMyTeams, { | ||
| organizationId: params.id, | ||
| }), | ||
| ); | ||
| await context.queryClient.ensureQueryData( | ||
| convexQuery(api.members.queries.approxCountMyTeams, { | ||
| organizationId: params.id, | ||
| }), | ||
| ); | ||
| }, |
Contributor
There was a problem hiding this comment.
ensureQueryData rejection on network error kills the entire route for a non-critical query
If approxCountMyTeams fails (transient network error, Convex error), the loader rejects and TanStack Router surfaces an error boundary — even though the count is only used for skeleton sizing. Wrapping with a try-catch and falling back to a default keeps the route functional.
🛡️ Proposed fix: graceful fallback in loader
loader: async ({ context, params }) => {
void context.queryClient.prefetchQuery(
convexQuery(api.members.queries.getMyTeams, {
organizationId: params.id,
}),
);
- await context.queryClient.ensureQueryData(
- convexQuery(api.members.queries.approxCountMyTeams, {
- organizationId: params.id,
- }),
- );
+ try {
+ await context.queryClient.ensureQueryData(
+ convexQuery(api.members.queries.approxCountMyTeams, {
+ organizationId: params.id,
+ }),
+ );
+ } catch {
+ // Non-critical: count is used only for skeleton sizing; fall back to default.
+ }
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id/settings/teams.tsx around lines
21 - 32, The loader currently awaits
context.queryClient.ensureQueryData(convexQuery(api.members.queries.approxCountMyTeams,
{ organizationId: params.id })) which will reject on network/Convex errors and
surface an error boundary; wrap that ensureQueryData call in a try-catch inside
the loader (catch only around the ensureQueryData call for
api.members.queries.approxCountMyTeams), log or ignore the error, and fall back
to a safe default (e.g., leave no data or set a small default count) so the
route continues to render; keep the prefetchQuery for getMyTeams unchanged and
only make the approxCountMyTeams call resilient.
Comment on lines
+18
to
+26
| export const approxCountDocuments = query({ | ||
| args: { | ||
| organizationId: v.string(), | ||
| }, | ||
| returns: v.number(), | ||
| handler: async (ctx, args) => { | ||
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all approxCount* functions in the documents queries file
rg -n "approxCount" services/platform/convex/documents/queries.ts -A 12Repository: tale-project/tale
Length of output: 412
🏁 Script executed:
#!/bin/bash
# Check other queries in the same file for auth pattern
rg -n "export const.*query\(\{" services/platform/convex/documents/queries.ts -A 8 | head -100Repository: tale-project/tale
Length of output: 903
🏁 Script executed:
#!/bin/bash
# Search for getAuthUserIdentity and getOrganizationMember usage in documents/queries.ts
echo "=== Checking auth methods in documents/queries.ts ==="
rg "getAuthUserIdentity|getOrganizationMember" services/platform/convex/documents/queries.ts -B 2 -A 2Repository: tale-project/tale
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Search for all approxCount* functions across the entire codebase
rg "export const approxCount" services/platform/convex/ -A 8Repository: tale-project/tale
Length of output: 7392
approxCountDocuments is missing authentication and organization-membership checks.
Unlike sibling listDocuments and other plain query() handlers in this file (which enforce getAuthUserIdentity and getOrganizationMember), approxCountDocuments directly exposes the count without validation. Similarly, all peer approxCount* queries using plain query() wrappers across the codebase (approxCountExampleMessages, approxCountMyTeams, approxCountApprovals) include explicit auth checks. This allows unauthenticated callers to probe approximate document counts for arbitrary organizations.
Returning 0 for unauthenticated/unauthorized cases is safe here given the frontend fallback (count ?? 10).
Proposed fix
export const approxCountDocuments = query({
args: {
organizationId: v.string(),
},
returns: v.number(),
handler: async (ctx, args) => {
+ const authUser = await getAuthUserIdentity(ctx);
+ if (!authUser) return 0;
+ try {
+ await getOrganizationMember(ctx, args.organizationId, authUser);
+ } catch {
+ return 0;
+ }
return await countItemsInOrg(ctx.db, 'documents', args.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.
Suggested change
| export const approxCountDocuments = query({ | |
| args: { | |
| organizationId: v.string(), | |
| }, | |
| returns: v.number(), | |
| handler: async (ctx, args) => { | |
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | |
| }, | |
| }); | |
| export const approxCountDocuments = query({ | |
| args: { | |
| organizationId: v.string(), | |
| }, | |
| returns: v.number(), | |
| handler: async (ctx, args) => { | |
| const authUser = await getAuthUserIdentity(ctx); | |
| if (!authUser) return 0; | |
| try { | |
| await getOrganizationMember(ctx, args.organizationId, authUser); | |
| } catch { | |
| return 0; | |
| } | |
| return await countItemsInOrg(ctx.db, 'documents', args.organizationId); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/documents/queries.ts` around lines 18 - 26,
approxCountDocuments currently skips auth and org-membership checks; update its
handler to call getAuthUserIdentity(ctx) and then verify membership via
getOrganizationMember(ctx.db, auth.userId, args.organizationId), returning 0 if
either check fails, and only call countItemsInOrg(ctx.db, 'documents',
args.organizationId) when the user is authenticated and a member; reference the
symbols approxCountDocuments, getAuthUserIdentity, getOrganizationMember, and
countItemsInOrg when making the change.
Comment on lines
+224
to
+243
| let count = 0; | ||
| for (const membership of membershipsResult.page) { | ||
| const teamResult: BetterAuthFindManyResult<BetterAuthTeam> = | ||
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | ||
| model: 'team', | ||
| paginationOpts: { cursor: null, numItems: 1 }, | ||
| where: [ | ||
| { field: '_id', operator: 'eq', value: membership.teamId }, | ||
| { | ||
| field: 'organizationId', | ||
| operator: 'eq', | ||
| value: args.organizationId, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| if (teamResult && teamResult.page.length > 0) { | ||
| count++; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Sequential N+1 runQuery calls make the blocking query slower than the non-blocking one
The loop issues one ctx.runQuery per membership sequentially, giving O(n × RTT) latency. getMyTeams performs the identical per-team lookup in parallel via Promise.all. Because the loader awaits approxCountMyTeams (blocking navigation) while getMyTeams is only fire-and-forget prefetched, the slower query ends up on the critical path — inverting the intended optimization.
⚡ Proposed fix: mirror the `getMyTeams` parallel pattern
- let count = 0;
- for (const membership of membershipsResult.page) {
- const teamResult: BetterAuthFindManyResult<BetterAuthTeam> =
- await ctx.runQuery(components.betterAuth.adapter.findMany, {
- model: 'team',
- paginationOpts: { cursor: null, numItems: 1 },
- where: [
- { field: '_id', operator: 'eq', value: membership.teamId },
- {
- field: 'organizationId',
- operator: 'eq',
- value: args.organizationId,
- },
- ],
- });
-
- if (teamResult && teamResult.page.length > 0) {
- count++;
- }
- }
-
- return count;
+ const teamResults = await Promise.all(
+ membershipsResult.page.map((membership) =>
+ ctx.runQuery(components.betterAuth.adapter.findMany, {
+ model: 'team',
+ paginationOpts: { cursor: null, numItems: 1 },
+ where: [
+ { field: '_id', operator: 'eq', value: membership.teamId },
+ {
+ field: 'organizationId',
+ operator: 'eq',
+ value: args.organizationId,
+ },
+ ],
+ }),
+ ),
+ );
+
+ return teamResults.filter((r) => r && r.page.length > 0).length;📝 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.
Suggested change
| let count = 0; | |
| for (const membership of membershipsResult.page) { | |
| const teamResult: BetterAuthFindManyResult<BetterAuthTeam> = | |
| await ctx.runQuery(components.betterAuth.adapter.findMany, { | |
| model: 'team', | |
| paginationOpts: { cursor: null, numItems: 1 }, | |
| where: [ | |
| { field: '_id', operator: 'eq', value: membership.teamId }, | |
| { | |
| field: 'organizationId', | |
| operator: 'eq', | |
| value: args.organizationId, | |
| }, | |
| ], | |
| }); | |
| if (teamResult && teamResult.page.length > 0) { | |
| count++; | |
| } | |
| } | |
| const teamResults = await Promise.all( | |
| membershipsResult.page.map((membership) => | |
| ctx.runQuery(components.betterAuth.adapter.findMany, { | |
| model: 'team', | |
| paginationOpts: { cursor: null, numItems: 1 }, | |
| where: [ | |
| { field: '_id', operator: 'eq', value: membership.teamId }, | |
| { | |
| field: 'organizationId', | |
| operator: 'eq', | |
| value: args.organizationId, | |
| }, | |
| ], | |
| }), | |
| ), | |
| ); | |
| return teamResults.filter((r) => r && r.page.length > 0).length; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/members/queries.ts` around lines 224 - 243, The loop
in approxCountMyTeams performs ctx.runQuery
(components.betterAuth.adapter.findMany) sequentially for each membership in
membershipsResult.page causing N+1 blocking latency; change it to mirror
getMyTeams by mapping membershipsResult.page to an array of promises that call
ctx.runQuery(...) for each membership.teamId, await Promise.all on that array,
then compute count by iterating the resolved results (counting those with
page.length > 0); ensure you still use the same query shape (model: 'team',
where checks for _id and organizationId) and replace the sequential increment
logic with counting over the parallel results.
This was referenced Feb 19, 2026
yannickmonney
added a commit
that referenced
this pull request
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores