refactor: remove -client suffix from components and extract table configs#526
Conversation
…fig hooks - Rename all *-client.tsx components to drop the -client suffix - Extract use-documents-table-config and use-audit-log-table-config hooks - Update all import references in route files accordingly
yannickmonney
changed the title
Greptile SummaryThis PR performs a clean refactoring by removing the Key Changes:
Benefits:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/features/documents/hooks/use-documents-table-config.tsx | New hook extracted from documents-table.tsx with table column configuration logic |
| services/platform/app/features/settings/audit-logs/hooks/use-audit-log-table-config.tsx | New hook extracted from audit-log-table.tsx with table column configuration logic |
| services/platform/app/features/documents/components/documents-table.tsx | Simplified by extracting column configuration to use-documents-table-config hook |
| services/platform/app/features/settings/audit-logs/components/audit-log-table.tsx | Simplified by extracting column configuration to use-audit-log-table-config hook |
| services/platform/app/features/tone-of-voice/components/tone-of-voice-form.tsx | Renamed from tone-of-voice-form-client.tsx and integrated skeleton components from route file |
| services/platform/app/routes/dashboard/$id/_knowledge/tone-of-voice.tsx | Simplified by moving skeleton components to ToneOfVoiceForm and updating imports |
Last reviewed commit: 6f5f6d9
📝 WalkthroughWalkthroughThis PR systematically refactors UI components across the platform by removing the "Client" suffix from component names (e.g., Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
services/platform/app/features/documents/components/documents-table.tsx (1)
36-36: 🧹 Nitpick | 🔵 Trivial
PAGE_SIZEis duplicated with the hook'spageSize: 20.This constant on line 36 and the hook's return value on line 254 of
use-documents-table-config.tsxare the same value (20). If you adopt the hook as the single source of truth (per the comment above), this constant can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/documents/components/documents-table.tsx` at line 36, Remove the duplicated PAGE_SIZE constant and use the hook's single source of truth instead: delete PAGE_SIZE from documents-table.tsx and update the component to consume the pageSize provided by the hook returned value (the pageSize property from the useDocumentsTableConfig hook in use-documents-table-config.tsx), or import a named export from that module if it exposes the default page size; ensure all usages in documents-table.tsx reference the hook's pageSize rather than the removed constant.services/platform/app/routes/dashboard/$id/settings/logs.tsx (1)
31-51:⚠️ Potential issue | 🟡 MinorSkeleton may not match the actual table's sticky layout, causing CLS.
AuditLogTablerenders withstickyLayoutenabled (line 105 ofaudit-log-table.tsx), butLogsSkeletondoesn't passstickyLayouttoDataTableSkeleton. Since the skeleton's purpose is to prevent CLS during loading, the layout should match. The hook already providesstickyLayout: true:♻️ Suggested change
function LogsSkeleton() { - const { columns } = useAuditLogTableConfig(); + const { columns, stickyLayout } = useAuditLogTableConfig(); return ( <Stack gap={4}> ... <Card title={<Skeleton className="h-6 w-32" />}> <DataTableSkeleton rows={10} columns={columns} showHeader noFirstColumnAvatar + stickyLayout={stickyLayout} /> </Card> </Stack> ); }🤖 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/logs.tsx around lines 31 - 51, LogsSkeleton currently only reads columns from useAuditLogTableConfig but doesn't pass the stickyLayout flag used by AuditLogTable, causing a layout mismatch and CLS; update LogsSkeleton to also destructure stickyLayout from useAuditLogTableConfig and pass that stickyLayout prop into DataTableSkeleton (maintain existing props like rows, showHeader, noFirstColumnAvatar) so the skeleton matches the real table's stickyLayout behavior.services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx (2)
78-228: 🧹 Nitpick | 🔵 TrivialConsider extracting shared column definitions to eliminate duplication.
The
'approval','event', and'action'column definitions — including the newmetaadditions — are duplicated verbatim betweenpendingColumnsandresolvedColumns. Keeping them in sync requires touching two places every time a property changes (as demonstrated by the three identicalmetaadditions in this PR).Extracting them as a shared factory or constant (outside the hook or memoised once) would eliminate the drift risk.
♻️ Sketch of the refactor
+ const sharedColumns = useMemo( + (): ColumnDef<ApprovalItem>[] => [ + { + id: 'approval', + header: t('columns.approvalRecipient'), + size: 256, + meta: { skeleton: { type: 'avatar-text' as const } }, + cell: ({ row }) => (/* … */), + }, + { + id: 'event', + header: t('columns.event'), + size: 256, + meta: { skeleton: { type: 'avatar-text' as const } }, + cell: ({ row }) => (/* … */), + }, + { + id: 'action', + header: t('columns.action'), + size: 256, + meta: { skeleton: { type: 'avatar-text' as const } }, + cell: ({ row }) => (/* … */), + }, + ], + [t], + ); const pendingColumns = useMemo( (): ColumnDef<ApprovalItem>[] => [ - { id: 'approval', … }, - { id: 'event', … }, - { id: 'action', … }, + ...sharedColumns, { id: 'confidence', … }, { id: 'actions', … }, ], - [t, getApprovalTypeLabel, getCustomerLabel, getConfidencePercent, approving, rejecting, onApprove, onReject], + [sharedColumns, getApprovalTypeLabel, getCustomerLabel, getConfidencePercent, approving, rejecting, onApprove, onReject], ); const resolvedColumns = useMemo( (): ColumnDef<ApprovalItem>[] => [ - { id: 'approval', … }, - { id: 'event', … }, - { id: 'action', … }, + ...sharedColumns, { id: 'reviewer', … }, { id: 'reviewedAt', … }, { id: 'status', … }, ], - [t, getCustomerLabel, formatDate], + [sharedColumns, getCustomerLabel, formatDate], );Note: the
'approval'cell inresolvedColumnshardcodest('types.recommendProduct')while thependingColumnsversion callsgetApprovalTypeLabel(row.original.resourceType)— resolve this divergence before sharing.Also applies to: 230-352
🤖 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/use-approval-columns.tsx` around lines 78 - 228, The three column defs ('approval', 'event', 'action') are duplicated between pendingColumns and resolvedColumns; extract them into a shared factory or const (e.g., createSharedColumnsFactory or SHARED_APPROVAL_EVENT_ACTION_COLUMNS) outside the hook and reuse in both pendingColumns and resolvedColumns, returning ColumnDef<ApprovalItem>[] fragments to splice into each memo; ensure the 'approval' cell divergence is resolved by parameterizing the factory with a label resolver (pass getApprovalTypeLabel for pending and a function that returns t('types.recommendProduct') for resolved) and update the hook deps to include any passed functions (getApprovalTypeLabel, t, onApprove/onReject if actions are included).
302-314:⚠️ Potential issue | 🟡 Minor
resolvedColumns:'reviewer'and'status'columns are missing skeleton metadata.Every other column in
resolvedColumnsreceived at least askeletonentry, but:
'reviewer'(lines 302–314) has nometaat all.'status'(lines 331–349) gets onlyheaderLabel; the analogous'actions'column inpendingColumnsalso getsskeleton: { type: 'action' as const }.If the skeleton renderer iterates all columns to render placeholders during the initial load, these two columns will render nothing (or fall back to a default) while neighbors show skeletons, producing an inconsistent loading state.
🛠️ Proposed additions
{ id: 'reviewer', header: t('columns.reviewer'), + meta: { skeleton: { type: 'text' as const } }, cell: ({ row }) => { … }, },{ id: 'status', header: () => (…), size: 100, - meta: { headerLabel: t('columns.approved') }, + meta: { + headerLabel: t('columns.approved'), + skeleton: { type: 'action' as const }, + }, cell: ({ row }) => (…), },Also applies to: 331-349
🤖 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/use-approval-columns.tsx` around lines 302 - 314, resolvedColumns is missing skeleton metadata for the 'reviewer' and 'status' column objects, causing inconsistent loading placeholders; update the resolver to add a meta.skeleton to each column object (the 'reviewer' column object where id === 'reviewer' and the 'status' column object where id === 'status') — for example add meta: { skeleton: { type: 'text' as const } } to reviewer and meta: { skeleton: { type: 'action' as const } } to status (matching how pendingColumns' 'actions' defines skeleton) so the skeleton renderer sees consistent placeholder info for these columns.services/platform/app/routes/dashboard/$id/settings/organization.tsx (1)
62-70: 🧹 Nitpick | 🔵 TrivialDuplicated
DataTableSkeletoncolumn config — consider extracting a shared constant.The columns array at lines 62–70 here is identical to the one in
organization-settings.tsx(lines 173–182). A change to any column (header key, size, action) requires updating both places.This PR already extracts table configuration into hooks (
use-documents-table-config,use-audit-log-table-config). Applying the same pattern here would make it consistent:♻️ Suggested extraction
In a new file
services/platform/app/features/settings/organization/components/member-table-columns.ts(or inline in the component):+export const MEMBER_TABLE_COLUMNS = [ + { header: 'headers.member', size: 348 }, + { header: 'headers.role', size: 200 }, + { header: 'headers.joined', align: 'right' as const }, + { isAction: true, size: 140 }, +] as const;Then in both
organization.tsxandorganization-settings.tsx:-columns={[ - { header: tTables('headers.member'), size: 348 }, - { header: tTables('headers.role'), size: 200 }, - { header: tTables('headers.joined'), align: 'right' }, - { isAction: true, size: 140 }, -]} +columns={MEMBER_TABLE_COLUMNS.map((col) => + 'header' in col ? { ...col, header: tTables(col.header) } : col, +)}🤖 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/organization.tsx around lines 62 - 70, The DataTableSkeleton columns array is duplicated between the organization component and organization-settings; extract the shared columns config into a single exported constant (e.g., MEMBER_TABLE_COLUMNS) in a new module (suggested name: member-table-columns.ts), preserve any typings used by DataTableSkeleton, then import and use MEMBER_TABLE_COLUMNS in both the DataTableSkeleton instances (referenced here as DataTableSkeleton in organization.tsx and the one in organization-settings.tsx) so updates are made in one place.
🤖 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/features/approvals/components/approvals.tsx`:
- Line 54: The skeleton's column selection is inverted versus the main Approvals
component—update the conditional in the skeleton (the code that currently uses
status === 'resolved' ? resolvedColumns : pendingColumns) to match the main
component's logic by using status === 'pending' ? pendingColumns :
resolvedColumns so undefined falls back to resolvedColumns; reference the status
prop/variable and the resolvedColumns/pendingColumns identifiers (and the
useApprovalColumns delegation) when making this change.
- Around line 44-49: ApprovalsSkeleton is passing inline no-op callbacks
(onApprove/onReject) into useApprovalColumns which lists them in its useMemo
dependencies, causing the memo to re-run every render; replace the inline () =>
{} no-ops with stable references (either module-level constants like
NOOP_APPROVE/NOOP_REJECT or memoized callbacks via useCallback) and pass those
stable functions into useApprovalColumns so pendingColumns and resolvedColumns
are memoized correctly.
In
`@services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx`:
- Line 103: The product-list columns ('event' and 'action') are using meta: {
skeleton: { type: 'avatar-text' } } which produces an avatar+text placeholder
that doesn't match their actual layout (a label plus ProductListCell); update
the skeleton.type for those columns in use-approval-columns (useApprovalColumns)
to a more appropriate skeleton such as 'text' (or a single-line/label skeleton
used elsewhere) instead of 'avatar-text', leaving the 'approval' column's
avatar-text unchanged; locate the 'event' and 'action' column definitions in
useApprovalColumns and change their meta.skeleton.type accordingly.
In `@services/platform/app/features/conversations/components/conversations.tsx`:
- Line 27: The imported component ConversationsClientSkeleton still uses the old
"-client-" name; rename the file conversations-client-skeleton.tsx to
conversations-skeleton.tsx, change the exported component name inside that file
from ConversationsClientSkeleton to ConversationsSkeleton, and update this
import in conversations.tsx to import { ConversationsSkeleton } from
'./conversations-skeleton'; also search and update any other references to
ConversationsClientSkeleton to the new ConversationsSkeleton.
In `@services/platform/app/features/documents/components/documents-table.tsx`:
- Around line 203-207: The component duplicates values that the
useDocumentsTableConfig hook already returns: destructure stickyLayout,
pageSize, and searchPlaceholder from useDocumentsTableConfig alongside columns
(e.g. const { columns, stickyLayout, pageSize, searchPlaceholder } =
useDocumentsTableConfig(...)), then replace the hardcoded PAGE_SIZE constant
usage in useListPage with the returned pageSize, replace the hardcoded
stickyLayout and tDocuments('searchPlaceholder') usages with the returned
stickyLayout and searchPlaceholder, and remove the now-unused PAGE_SIZE constant
to keep the hook as the single source of truth.
In
`@services/platform/app/features/documents/hooks/use-documents-table-config.tsx`:
- Around line 90-143: The source column cell renderer duplicates
OneDrive/SharePoint branches; refactor the JSX in the cell (inside
use-documents-table-config.tsx) by extracting a small helper that inspects
row.original.sourceProvider and sourceMode and returns {icon, title, synced}
(use OneDriveIcon, SharePointIcon, Monitor and tDocuments keys), then render a
single unified block that conditionally adds the RefreshCw when synced is true;
update the cell to use this helper instead of repeating almost-identical markup.
- Around line 166-189: The overflow count can be incorrect because tag IDs
missing from teamMap are rendered as null but still counted via tags.length; in
the cell renderer (cell: ({ row }) => { ... }) first derive a filteredTags array
by mapping row.original.teamTags to resolved team names via teamMap and
filtering out unresolved IDs (or undefined names), then use
filteredTags.slice(0,2) to render badges and compute the overflow as Math.max(0,
filteredTags.length - 2) for the "+N" badge; keep the early returns for folders
and isLoadingTeams unchanged and ensure you key badges by the original tag ID or
resolved name consistently.
In `@services/platform/app/features/settings/account/components/account-form.tsx`:
- Around line 26-29: AccountFormProps currently includes an unused
organizationId field; remove organizationId from the AccountFormProps interface
and any places that pass it through (e.g., the AccountForm component props and
the route that supplies it), and ensure AccountForm, ChangePasswordForm, and
SetPasswordForm do not expect or receive organizationId anywhere—update their
prop types/usages accordingly so no unused prop is declared or forwarded.
In
`@services/platform/app/features/settings/audit-logs/components/audit-log-table.tsx`:
- Around line 37-38: The hook useAuditLogTableConfig currently returns
stickyLayout but the component only destructures columns and pageSize; update
the destructuring to include stickyLayout (e.g., const { columns, stickyLayout,
pageSize } = useAuditLogTableConfig()) and pass that stickyLayout value into the
DataTable component instead of the hardcoded prop on DataTable (remove the bare
hardcoded stickyLayout and use the hook-provided variable).
In
`@services/platform/app/features/settings/audit-logs/hooks/use-audit-log-table-config.tsx`:
- Around line 26-42: The timestamp column (accessorKey: 'timestamp') has a
right-aligned header and a TableDateCell using alignRight but its meta only
contains headerLabel, so DataTableSkeleton can't detect alignment; update the
column's meta to include align: 'right' (e.g., meta: { headerLabel:
t('logs.audit.columns.timestamp'), align: 'right' }) so both skeleton header and
skeleton cell render right-aligned consistently when using DataTableSkeleton.
- Around line 93-113: Add a badge skeleton meta to the columns that render Badge
components: locate the column objects with accessorKey 'status' and 'category'
in use-audit-log-table-config (the column definitions that return a <Badge>) and
add meta: { skeleton: { type: 'badge' } } to each column object so
DataTableSkeleton will use the pill-shaped badge skeleton instead of the default
text skeleton.
In
`@services/platform/app/features/tone-of-voice/components/tone-of-voice-form.tsx`:
- Around line 255-267: The conditional rendering for the skeleton is confusing
because it relies on exampleCount truthiness; update the ternary condition in
the ToneOfVoiceForm component so it explicitly checks for a defined count (use
exampleCount != null && exampleCount > 0) before rendering
ExampleSectionSkeleton, leaving the isLoading branch and the
ExampleMessagesTable (examples, onAddExample, onViewExample, onEditExample,
onDeleteExample) behavior unchanged.
---
Outside diff comments:
In
`@services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx`:
- Around line 78-228: The three column defs ('approval', 'event', 'action') are
duplicated between pendingColumns and resolvedColumns; extract them into a
shared factory or const (e.g., createSharedColumnsFactory or
SHARED_APPROVAL_EVENT_ACTION_COLUMNS) outside the hook and reuse in both
pendingColumns and resolvedColumns, returning ColumnDef<ApprovalItem>[]
fragments to splice into each memo; ensure the 'approval' cell divergence is
resolved by parameterizing the factory with a label resolver (pass
getApprovalTypeLabel for pending and a function that returns
t('types.recommendProduct') for resolved) and update the hook deps to include
any passed functions (getApprovalTypeLabel, t, onApprove/onReject if actions are
included).
- Around line 302-314: resolvedColumns is missing skeleton metadata for the
'reviewer' and 'status' column objects, causing inconsistent loading
placeholders; update the resolver to add a meta.skeleton to each column object
(the 'reviewer' column object where id === 'reviewer' and the 'status' column
object where id === 'status') — for example add meta: { skeleton: { type: 'text'
as const } } to reviewer and meta: { skeleton: { type: 'action' as const } } to
status (matching how pendingColumns' 'actions' defines skeleton) so the skeleton
renderer sees consistent placeholder info for these columns.
In `@services/platform/app/features/documents/components/documents-table.tsx`:
- Line 36: Remove the duplicated PAGE_SIZE constant and use the hook's single
source of truth instead: delete PAGE_SIZE from documents-table.tsx and update
the component to consume the pageSize provided by the hook returned value (the
pageSize property from the useDocumentsTableConfig hook in
use-documents-table-config.tsx), or import a named export from that module if it
exposes the default page size; ensure all usages in documents-table.tsx
reference the hook's pageSize rather than the removed constant.
In `@services/platform/app/routes/dashboard/`$id/settings/logs.tsx:
- Around line 31-51: LogsSkeleton currently only reads columns from
useAuditLogTableConfig but doesn't pass the stickyLayout flag used by
AuditLogTable, causing a layout mismatch and CLS; update LogsSkeleton to also
destructure stickyLayout from useAuditLogTableConfig and pass that stickyLayout
prop into DataTableSkeleton (maintain existing props like rows, showHeader,
noFirstColumnAvatar) so the skeleton matches the real table's stickyLayout
behavior.
In `@services/platform/app/routes/dashboard/`$id/settings/organization.tsx:
- Around line 62-70: The DataTableSkeleton columns array is duplicated between
the organization component and organization-settings; extract the shared columns
config into a single exported constant (e.g., MEMBER_TABLE_COLUMNS) in a new
module (suggested name: member-table-columns.ts), preserve any typings used by
DataTableSkeleton, then import and use MEMBER_TABLE_COLUMNS in both the
DataTableSkeleton instances (referenced here as DataTableSkeleton in
organization.tsx and the one in organization-settings.tsx) so updates are made
in one place.
| const { pendingColumns, resolvedColumns } = useApprovalColumns({ | ||
| approving: null, | ||
| rejecting: null, | ||
| onApprove: () => {}, | ||
| onReject: () => {}, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inline no-ops defeat useMemo inside useApprovalColumns.
onApprove: () => {} and onReject: () => {} are recreated on every render of ApprovalsSkeleton. Because useApprovalColumns lists these callbacks in its useMemo dependency array, the memoised column arrays are recomputed on every render. Correctness is unaffected (skeletons don't invoke the handlers), but the memoisation is completely wasted for the skeleton case.
Stabilise the references with module-level constants or useCallback:
♻️ Proposed fix — stable no-op refs
+const noop = () => {};
+
function ApprovalsSkeleton({
status,
rows,
}: {
status?: 'pending' | 'resolved';
rows?: number;
}) {
const { pendingColumns, resolvedColumns } = useApprovalColumns({
approving: null,
rejecting: null,
- onApprove: () => {},
- onReject: () => {},
+ onApprove: noop,
+ onReject: noop,
});📝 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.
| const { pendingColumns, resolvedColumns } = useApprovalColumns({ | |
| approving: null, | |
| rejecting: null, | |
| onApprove: () => {}, | |
| onReject: () => {}, | |
| }); | |
| const noop = () => {}; | |
| function ApprovalsSkeleton({ | |
| status, | |
| rows, | |
| }: { | |
| status?: 'pending' | 'resolved'; | |
| rows?: number; | |
| }) { | |
| const { pendingColumns, resolvedColumns } = useApprovalColumns({ | |
| approving: null, | |
| rejecting: null, | |
| onApprove: noop, | |
| onReject: noop, | |
| }); |
🤖 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.tsx` around
lines 44 - 49, ApprovalsSkeleton is passing inline no-op callbacks
(onApprove/onReject) into useApprovalColumns which lists them in its useMemo
dependencies, causing the memo to re-run every render; replace the inline () =>
{} no-ops with stable references (either module-level constants like
NOOP_APPROVE/NOOP_REJECT or memoized callbacks via useCallback) and pass those
stable functions into useApprovalColumns so pendingColumns and resolvedColumns
are memoized correctly.
| <DataTableSkeleton | ||
| rows={rows} | ||
| columns={columns} | ||
| columns={status === 'resolved' ? resolvedColumns : pendingColumns} |
There was a problem hiding this comment.
Inconsistent undefined-status fallback between skeleton and main component.
Line 54 (skeleton): status === 'resolved' ? resolvedColumns : pendingColumns → undefined falls back to pendingColumns.
Line 270 (main): status === 'pending' ? pendingColumns : resolvedColumns → undefined falls back to resolvedColumns.
This inconsistency is new (introduced by the skeleton now delegating to useApprovalColumns). In practice it's harmless—status is always 'pending' | 'resolved' from the route—but aligning the two expressions makes the intent explicit and guards against future callers that omit status.
♻️ Proposed fix — align skeleton expression with main component
- columns={status === 'resolved' ? resolvedColumns : pendingColumns}
+ columns={status === 'pending' ? pendingColumns : resolvedColumns}Also applies to: 270-270
🤖 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.tsx` at line
54, The skeleton's column selection is inverted versus the main Approvals
component—update the conditional in the skeleton (the code that currently uses
status === 'resolved' ? resolvedColumns : pendingColumns) to match the main
component's logic by using status === 'pending' ? pendingColumns :
resolvedColumns so undefined falls back to resolvedColumns; reference the status
prop/variable and the resolvedColumns/pendingColumns identifiers (and the
useApprovalColumns delegation) when making this change.
| id: 'event', | ||
| header: t('columns.event'), | ||
| size: 256, | ||
| meta: { skeleton: { type: 'avatar-text' as const } }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the skeleton renderer to understand what 'avatar-text' produces
rg -n "avatar-text" --type tsx -A 5 -B 2Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Also check for the ProductListCell component to see what it renders
fd "ProductListCell" --type tsxRepository: tale-project/tale
Length of output: 231
🏁 Script executed:
# Find the skeleton type definitions or renderer
rg -n "skeleton.*type" --type tsx | head -30Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Examine the specific file to understand the column definitions better
cat -n services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx | sed -n '100,110p'Repository: tale-project/tale
Length of output: 559
🏁 Script executed:
# Search for avatar-text skeleton type usage across tsx files
rg "avatar-text" -A 5 -B 2Repository: tale-project/tale
Length of output: 9801
🏁 Script executed:
# Find ProductListCell component
fd "ProductListCell"Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Search for skeleton renderer or type definitions
rg "skeleton.*type" -A 3 -B 1 | head -50Repository: tale-project/tale
Length of output: 4924
🏁 Script executed:
# Check what content the 'event' column actually renders
cat -n services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx | sed -n '100,115p'Repository: tale-project/tale
Length of output: 763
🏁 Script executed:
# Also check the 'action' column (line 128)
cat -n services/platform/app/features/approvals/components/approvals/use-approval-columns.tsx | sed -n '125,140p'Repository: tale-project/tale
Length of output: 771
Replace 'avatar-text' skeleton type on product-list columns with a more appropriate type.
The 'event' and 'action' columns render a text label followed by ProductListCell (a product list), not an avatar + text layout. The 'avatar-text' skeleton type is designed for avatar + two text lines and will produce a visually mismatched loading placeholder. The 'approval' column correctly uses this type since it displays an icon/avatar with text, but the product-list columns need a different skeleton type that matches their actual layout.
🤖 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/use-approval-columns.tsx`
at line 103, The product-list columns ('event' and 'action') are using meta: {
skeleton: { type: 'avatar-text' } } which produces an avatar+text placeholder
that doesn't match their actual layout (a label plus ProductListCell); update
the skeleton.type for those columns in use-approval-columns (useApprovalColumns)
to a more appropriate skeleton such as 'text' (or a single-line/label skeleton
used elsewhere) instead of 'avatar-text', leaving the 'approval' column's
avatar-text unchanged; locate the 'event' and 'action' column definitions in
useApprovalColumns and change their meta.skeleton.type accordingly.
| @@ -27,21 +27,21 @@ import { ConversationPanel } from './conversation-panel'; | |||
| import { ConversationsClientSkeleton } from './conversations-client-skeleton'; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Search for ConversationsClientSkeleton and conversations-client-skeleton references
rg "ConversationsClientSkeleton|conversations-client-skeleton" --type tsx --type tsRepository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Check for similar skeleton files in the conversations directory to understand naming pattern
fd "skeleton" services/platform/app/features/conversations/ 2>/dev/null || echo "Path may not exist"Repository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Look for any conversations skeleton files more broadly
fd -e tsx -e ts | rg "skeleton" | grep -i conversationRepository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Check the git history or current state of conversations.tsx to see what's imported
cat services/platform/app/features/conversations/components/conversations.tsx 2>/dev/null | head -40Repository: tale-project/tale
Length of output: 1667
🏁 Script executed:
# Check what conversations-client-skeleton.tsx exports
cat services/platform/app/features/conversations/components/conversations-client-skeleton.tsx | tail -20Repository: tale-project/tale
Length of output: 670
🏁 Script executed:
# Search for the export of ConversationsClientSkeleton
rg "export.*ConversationsClientSkeleton" services/platform/app/features/conversations/Repository: tale-project/tale
Length of output: 196
🏁 Script executed:
# Look for other skeleton files in the same directory and their naming
ls -la services/platform/app/features/conversations/components/ | grep skeletonRepository: tale-project/tale
Length of output: 144
🏁 Script executed:
# Check if other components were renamed from *Client to drop the Client suffix in this PR
rg "ConversationsClient" services/platform/app/features/conversations/components/ --max-count 3Repository: tale-project/tale
Length of output: 768
🏁 Script executed:
# Check what main component was renamed (ConversationsClient -> Conversations)
rg "export function Conversations\(" services/platform/app/features/conversations/components/conversations.tsxRepository: tale-project/tale
Length of output: 92
🏁 Script executed:
# Find other skeleton files in the conversations feature to see if they also retain -client
find services/platform/app/features/conversations -name "*skeleton*"Repository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Check if there are other -client files that were renamed in this PR
find services/platform/app/features/conversations/components -name "*client*"Repository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Look for the original ConversationsClient component - was it renamed or removed?
rg "ConversationsClient" services/platform/app/features/conversations/components/ | grep -v skeleton | grep -v ".tsx:.*import"Repository: tale-project/tale
Length of output: 215
🏁 Script executed:
# Check the git log for PR 397 or related commits about this rename
git log --oneline --all | grep -i "client\|rename" | head -20Repository: tale-project/tale
Length of output: 692
🏁 Script executed:
# Search for any other *-client-skeleton files in the app directory
find services/platform/app -name "*client-skeleton*"Repository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Check the full directory structure of conversations/components to see all files
ls -la services/platform/app/features/conversations/components/Repository: tale-project/tale
Length of output: 1344
🏁 Script executed:
# Look for git diff or status to see what was actually changed in this PR
git show --stat --name-only 2>/dev/null | head -50 || git statusRepository: tale-project/tale
Length of output: 1537
🏁 Script executed:
# Check the exact commit files to see if skeleton was intentionally excluded
git show 6f5f6d9 --name-status | grep -i skeletonRepository: tale-project/tale
Length of output: 781
🏁 Script executed:
# Look at the full commit to see what files were actually renamed
git show 6f5f6d9 --name-status | grep "conversations.*skeleton\|conversations.*client"Repository: tale-project/tale
Length of output: 152
🏁 Script executed:
# Check if the skeleton file appears in the rename list
git show 6f5f6d9 --name-status | head -100 | grep -i "conversations"Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Look for actual rename operations (R status) in the commit
git show 6f5f6d9 --name-status | grep "^R"Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Check if conversations-client-skeleton existed before this commit
git log --all --full-history -S "conversations-client-skeleton" --oneline | head -10Repository: tale-project/tale
Length of output: 147
🏁 Script executed:
# See what files were actually touched in the conversations feature
git show 6f5f6d9 --name-status | grep "conversations"Repository: tale-project/tale
Length of output: 5104
🏁 Script executed:
# Check if there's a rename from something to conversations-client-skeleton
git show 6f5f6d9 | grep -A 5 -B 5 "conversations-client-skeleton" | head -30Repository: tale-project/tale
Length of output: 1263
🏁 Script executed:
# Check if there are other *-client-skeleton files in the app directory
find services/platform/app -name "*-client-skeleton*" -type fRepository: tale-project/tale
Length of output: 150
🏁 Script executed:
# Check commit details to see if there were any intentional exclusions documented
git show 6f5f6d9 --pretty=format:"%B" | head -20Repository: tale-project/tale
Length of output: 763
🏁 Script executed:
# Verify the skeleton file content to confirm it's not just a passthrough
head -20 services/platform/app/features/conversations/components/conversations-client-skeleton.tsxRepository: tale-project/tale
Length of output: 711
ConversationsClientSkeleton retains the -client- naming, inconsistent with the PR's refactoring goal.
The skeleton file (conversations-client-skeleton.tsx) and its export were not renamed to drop the -client suffix, despite this being the stated objective of the refactor. Other skeleton files added in this same commit (e.g., automations-table-skeleton.tsx, api-keys-table-skeleton.tsx, teams-table-skeleton.tsx) follow the pattern of dropping the -client prefix. Rename the skeleton file to conversations-skeleton.tsx and update the export to ConversationsSkeleton for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/conversations/components/conversations.tsx` at
line 27, The imported component ConversationsClientSkeleton still uses the old
"-client-" name; rename the file conversations-client-skeleton.tsx to
conversations-skeleton.tsx, change the exported component name inside that file
from ConversationsClientSkeleton to ConversationsSkeleton, and update this
import in conversations.tsx to import { ConversationsSkeleton } from
'./conversations-skeleton'; also search and update any other references to
ConversationsClientSkeleton to the new ConversationsSkeleton.
| const { columns } = useDocumentsTableConfig({ | ||
| onDocumentClick: handleDocumentClick, | ||
| isLoadingTeams, | ||
| teamMap, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hook's stickyLayout, pageSize, and searchPlaceholder are returned but unused in the main table.
The hook returns stickyLayout, pageSize, and searchPlaceholder, but the component duplicates these values: PAGE_SIZE constant on line 36, hardcoded stickyLayout on line 257, and tDocuments('searchPlaceholder') on line 232. Consider destructuring and using them from the hook to keep it the single source of truth.
♻️ Suggested change
- const { columns } = useDocumentsTableConfig({
+ const { columns, stickyLayout, pageSize, searchPlaceholder } = useDocumentsTableConfig({
onDocumentClick: handleDocumentClick,
isLoadingTeams,
teamMap,
});Then use pageSize in the useListPage call and stickyLayout/searchPlaceholder where they're currently hardcoded, and remove the PAGE_SIZE constant.
📝 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.
| const { columns } = useDocumentsTableConfig({ | |
| onDocumentClick: handleDocumentClick, | |
| isLoadingTeams, | |
| teamMap, | |
| }); | |
| const { columns, stickyLayout, pageSize, searchPlaceholder } = useDocumentsTableConfig({ | |
| onDocumentClick: handleDocumentClick, | |
| isLoadingTeams, | |
| teamMap, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/documents/components/documents-table.tsx`
around lines 203 - 207, The component duplicates values that the
useDocumentsTableConfig hook already returns: destructure stickyLayout,
pageSize, and searchPlaceholder from useDocumentsTableConfig alongside columns
(e.g. const { columns, stickyLayout, pageSize, searchPlaceholder } =
useDocumentsTableConfig(...)), then replace the hardcoded PAGE_SIZE constant
usage in useListPage with the returned pageSize, replace the hardcoded
stickyLayout and tDocuments('searchPlaceholder') usages with the returned
stickyLayout and searchPlaceholder, and remove the now-unused PAGE_SIZE constant
to keep the hook as the single source of truth.
| interface AccountFormProps { | ||
| organizationId: string; | ||
| memberContext: MemberContext | null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
organizationId is a dead prop — consider removing it.
AccountFormProps declares organizationId: string, but AccountForm never destructures or forwards it. Neither ChangePasswordForm nor SetPasswordForm receives it. This is a pre-existing gap, but now that the interface is cleanly renamed it's a good opportunity to clean up the unused prop.
♻️ Proposed cleanup
interface AccountFormProps {
- organizationId: string;
memberContext: MemberContext | null;
}And in the route file remove the corresponding prop:
<AccountForm
- organizationId={organizationId}
memberContext={memberContext}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/features/settings/account/components/account-form.tsx`
around lines 26 - 29, AccountFormProps currently includes an unused
organizationId field; remove organizationId from the AccountFormProps interface
and any places that pass it through (e.g., the AccountForm component props and
the route that supplies it), and ensure AccountForm, ChangePasswordForm, and
SetPasswordForm do not expect or receive organizationId anywhere—update their
prop types/usages accordingly so no unused prop is declared or forwarded.
| const { columns, pageSize } = useAuditLogTableConfig(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
stickyLayout from hook is unused; hardcoded on line 105 instead.
The hook returns { columns, stickyLayout: true, pageSize: 30 } but only columns and pageSize are destructured here, while stickyLayout is hardcoded as a bare prop on <DataTable> at line 105. For consistency with the centralized-config pattern, consider consuming it from the hook:
♻️ Suggested change
- const { columns, pageSize } = useAuditLogTableConfig();
+ const { columns, pageSize, stickyLayout } = useAuditLogTableConfig();Then at line 105:
- stickyLayout
+ stickyLayout={stickyLayout}📝 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.
| const { columns, pageSize } = useAuditLogTableConfig(); | |
| const { columns, pageSize, stickyLayout } = useAuditLogTableConfig(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/audit-logs/components/audit-log-table.tsx`
around lines 37 - 38, The hook useAuditLogTableConfig currently returns
stickyLayout but the component only destructures columns and pageSize; update
the destructuring to include stickyLayout (e.g., const { columns, stickyLayout,
pageSize } = useAuditLogTableConfig()) and pass that stickyLayout value into the
DataTable component instead of the hardcoded prop on DataTable (remove the bare
hardcoded stickyLayout and use the hook-provided variable).
| { | ||
| accessorKey: 'timestamp', | ||
| header: () => ( | ||
| <span className="block w-full text-right"> | ||
| {t('logs.audit.columns.timestamp')} | ||
| </span> | ||
| ), | ||
| size: 140, | ||
| meta: { headerLabel: t('logs.audit.columns.timestamp') }, | ||
| cell: ({ row }) => ( | ||
| <TableDateCell | ||
| date={row.original.timestamp} | ||
| preset="relative" | ||
| alignRight | ||
| /> | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Timestamp column missing align: 'right' in meta — skeleton cells won't be right-aligned.
The timestamp column header is rendered right-aligned via a function header (<span className="block w-full text-right">), and the actual cell uses alignRight. However, the meta object only has headerLabel — it doesn't include align: 'right'. When DataTableSkeleton normalizes this column, it won't detect the right alignment, so both the skeleton header and skeleton cell content will render left-aligned, causing a visual shift when loading completes.
🐛 Proposed fix
{
accessorKey: 'timestamp',
header: () => (
<span className="block w-full text-right">
{t('logs.audit.columns.timestamp')}
</span>
),
size: 140,
- meta: { headerLabel: t('logs.audit.columns.timestamp') },
+ meta: { headerLabel: t('logs.audit.columns.timestamp'), align: 'right' as const },
cell: ({ row }) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/audit-logs/hooks/use-audit-log-table-config.tsx`
around lines 26 - 42, The timestamp column (accessorKey: 'timestamp') has a
right-aligned header and a TableDateCell using alignRight but its meta only
contains headerLabel, so DataTableSkeleton can't detect alignment; update the
column's meta to include align: 'right' (e.g., meta: { headerLabel:
t('logs.audit.columns.timestamp'), align: 'right' }) so both skeleton header and
skeleton cell render right-aligned consistently when using DataTableSkeleton.
| { | ||
| accessorKey: 'status', | ||
| header: t('logs.audit.columns.status'), | ||
| cell: ({ row }) => { | ||
| const status = row.original.status; | ||
| return ( | ||
| <Badge | ||
| variant={ | ||
| status === 'success' | ||
| ? 'green' | ||
| : status === 'denied' | ||
| ? 'yellow' | ||
| : 'destructive' | ||
| } | ||
| > | ||
| {status} | ||
| </Badge> | ||
| ); | ||
| }, | ||
| size: 100, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding meta: { skeleton: { type: 'badge' } } to the category and status columns.
The category and status columns render <Badge> components, but they don't declare a skeleton type in their meta. When rendered in DataTableSkeleton, they'll get the default text skeleton instead of a pill-shaped badge skeleton (h-5 w-20 rounded-full). This is a minor visual fidelity concern for the loading state.
♻️ Suggested change for category (line 83–92)
{
accessorKey: 'category',
header: t('logs.audit.columns.category'),
cell: ({ row }) => (
<Badge variant="outline" className="capitalize">
{row.original.category}
</Badge>
),
size: 100,
+ meta: { skeleton: { type: 'badge' } },
},♻️ Suggested change for status (line 93–113)
{
accessorKey: 'status',
header: t('logs.audit.columns.status'),
cell: ({ row }) => {
...
},
size: 100,
+ meta: { skeleton: { type: 'badge' } },
},📝 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.
| { | |
| accessorKey: 'status', | |
| header: t('logs.audit.columns.status'), | |
| cell: ({ row }) => { | |
| const status = row.original.status; | |
| return ( | |
| <Badge | |
| variant={ | |
| status === 'success' | |
| ? 'green' | |
| : status === 'denied' | |
| ? 'yellow' | |
| : 'destructive' | |
| } | |
| > | |
| {status} | |
| </Badge> | |
| ); | |
| }, | |
| size: 100, | |
| }, | |
| { | |
| accessorKey: 'status', | |
| header: t('logs.audit.columns.status'), | |
| cell: ({ row }) => { | |
| const status = row.original.status; | |
| return ( | |
| <Badge | |
| variant={ | |
| status === 'success' | |
| ? 'green' | |
| : status === 'denied' | |
| ? 'yellow' | |
| : 'destructive' | |
| } | |
| > | |
| {status} | |
| </Badge> | |
| ); | |
| }, | |
| size: 100, | |
| meta: { skeleton: { type: 'badge' } }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/audit-logs/hooks/use-audit-log-table-config.tsx`
around lines 93 - 113, Add a badge skeleton meta to the columns that render
Badge components: locate the column objects with accessorKey 'status' and
'category' in use-audit-log-table-config (the column definitions that return a
<Badge>) and add meta: { skeleton: { type: 'badge' } } to each column object so
DataTableSkeleton will use the pill-shaped badge skeleton instead of the default
text skeleton.
| {isLoading ? ( | ||
| exampleCount && exampleCount > 0 ? ( | ||
| <ExampleSectionSkeleton rows={Math.min(exampleCount, 5)} /> | ||
| ) : null | ||
| ) : ( | ||
| <ExampleMessagesTable | ||
| examples={examples} | ||
| onAddExample={() => setIsAddDialogOpen(true)} | ||
| onViewExample={handleViewExample} | ||
| onEditExample={handleEditExample} | ||
| onDeleteExample={handleDeleteExample} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Conditional skeleton logic is sound, but relies on exampleCount truthiness in a non-obvious way.
The expression exampleCount && exampleCount > 0 ? <Skeleton> : null works correctly as a ternary condition because 0 and undefined are both falsy. However, it reads as though exampleCount (a number) could leak into the JSX tree. Since it's only used as the condition of ? : (not rendered directly via &&), there's no rendering bug — but consider simplifying to exampleCount != null && exampleCount > 0 for clarity.
🤖 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/tone-of-voice-form.tsx`
around lines 255 - 267, The conditional rendering for the skeleton is confusing
because it relies on exampleCount truthiness; update the ternary condition in
the ToneOfVoiceForm component so it explicitly checks for a defined count (use
exampleCount != null && exampleCount > 0) before rendering
ExampleSectionSkeleton, leaving the isLoading branch and the
ExampleMessagesTable (examples, onAddExample, onViewExample, onEditExample,
onDeleteExample) behavior unchanged.
1 participant
Summary by CodeRabbit
New Features
Refactor