feat(frontend): DataTable — the antd-free table the shared settings pages needed - #5886
feat(frontend): DataTable — the antd-free table the shared settings pages needed#5886ardaerzin wants to merge 8 commits into
Conversation
|
@coderabbitai review |
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a shared ChangesSettings DataTable migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
b21dda1 to
d786317
Compare
8a13b53 to
d70dc9f
Compare
d786317 to
6cf00f1
Compare
d70dc9f to
8942568
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/mobile/src/features/settings/SettingsScreen.tsx (1)
148-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the requested tab against visible tabs.
activeaccepts every key inAVAILABLE, but the rail removes tabs wheretab.isHiddenis true. When access hides an available tab, a direct?tab=<key>query still renders its body. Deriveactivefrom the filtered tab list, or use the shared tab resolver used by the desktop settings page.#!/bin/bash set -euo pipefail # Inspect the shared visibility and resolution rules for settings tabs. rg -n -C 6 --glob '*.{ts,tsx}' \ 'getSettingsSidebarTabs|resolveSettingsTab|isHidden' webAlso applies to: 156-158
🧹 Nitpick comments (2)
web/mobile/src/features/settings/SettingsScreen.tsx (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten this code comment.
This new block exceeds one short line. Keep only the constraint that is necessary to explain the read-only host behavior.
As per coding guidelines, “Keep in-code comments to at most one short line.”
Source: Coding guidelines
web/packages/agenta-ui/src/components/ui/data-table.tsx (1)
89-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: wrap the toolbar in a
TooltipProvider.Four consumers (
ApiKeysPage,WebhooksPage,NamedSecretTable,SecretProviderTable) each add a localTooltipProviderfor one reload tooltip. IfDataTableprovides one around the toolbar, those wrappers can be removed. Radix nests providers safely, so hosts that already provide one stay correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b0b39b6a-628b-4782-82b6-4dace024642d
📒 Files selected for processing (9)
web/mobile/package.jsonweb/mobile/src/features/settings/SettingsScreen.tsxweb/packages/agenta-settings-ui/src/ApiKeysPage.tsxweb/packages/agenta-settings-ui/src/projects/ProjectsPage.tsxweb/packages/agenta-settings-ui/src/secrets/NamedSecretTable.tsxweb/packages/agenta-settings-ui/src/secrets/SecretProviderTable.tsxweb/packages/agenta-settings-ui/src/webhooks/WebhooksPage.tsxweb/packages/agenta-ui/src/components/ui/data-table.tsxweb/packages/agenta-ui/src/components/ui/index.ts
| const keys = useApiKeys({ | ||
| workspaceId: "", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the active workspace ID to useApiKeys.
Line 70 always passes an empty workspace ID. useApiKeys clears the list when the ID is empty. The API Keys tab therefore always shows no keys, and Reload does not issue a request.
Proposed fix
const TabBody = ({
tab,
access,
+ workspaceId,
user,
theme,
}: {
tab: SettingsTabKey
access: SettingsAccess
+ workspaceId: string
user: {username?: string | null; email?: string | null} | null
theme: {options: {mode: string; label: string}[]; mode: string; onSelect: (m: string) => void}
}) => {
const keys = useApiKeys({
- workspaceId: "",
+ workspaceId, <TabBody
tab={active}
access={access}
+ workspaceId={workspaceId}
user={user}📝 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 keys = useApiKeys({ | |
| workspaceId: "", | |
| const TabBody = ({ | |
| tab, | |
| access, | |
| workspaceId, | |
| user, | |
| theme, | |
| }: { | |
| tab: SettingsTabKey | |
| access: SettingsAccess | |
| workspaceId: string | |
| user: {username?: string | null; email?: string | null} | null | |
| theme: {options: {mode: string; label: string}[]; mode: string; onSelect: (m: string) => void} | |
| }) => { | |
| const keys = useApiKeys({ | |
| workspaceId, |
| const keys = useApiKeys({ | |
| workspaceId: "", | |
| <TabBody | |
| tab={active} | |
| access={access} | |
| workspaceId={workspaceId} | |
| user={user} |
| case "webhooks": | ||
| return <WebhooksPage /> | ||
| case "projects": | ||
| return <ProjectsPage projects={[]} isLoading={false} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provide project data before rendering ProjectsPage.
Line 107 always supplies an empty list and reports that loading is complete. The Projects tab therefore always renders its empty state. Fetch and pass the project list, or remove this tab until the mobile host supports it.
| <Button | ||
| variant="outline" | ||
| aria-label="Reload secrets" | ||
| disabled={loading} | ||
| onClick={mutate} | ||
| > | ||
| <ArrowClockwise size={14} /> | ||
| </Button> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reload buttons forward the click event to mutate. Both migrated secret tables bind the revalidation function directly to onClick, so React passes a MouseEvent as the first argument. If mutate is the SWR bound mutator, that argument becomes replacement cache data and corrupts the cached secrets list.
web/packages/agenta-settings-ui/src/secrets/NamedSecretTable.tsx#L137-L144: replaceonClick={mutate}withonClick={() => { void mutate() }}.web/packages/agenta-settings-ui/src/secrets/SecretProviderTable.tsx#L208-L215: replaceonClick={mutate}withonClick={() => { void mutate() }}.
📍 Affects 2 files
web/packages/agenta-settings-ui/src/secrets/NamedSecretTable.tsx#L137-L144(this comment)web/packages/agenta-settings-ui/src/secrets/SecretProviderTable.tsx#L208-L215
| : rows.map((record) => ( | ||
| <tr | ||
| key={rowKey(record)} | ||
| onClick={onRowClick ? () => onRowClick(record) : undefined} | ||
| className={clsx( | ||
| "border-0 border-b border-solid border-colorBorderSecondary last:border-b-0 hover:bg-colorFillQuaternary", | ||
| onRowClick && "cursor-pointer", | ||
| )} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add keyboard activation for clickable rows.
The row registers onClick only. Keyboard users cannot activate it, because a <tr> is not focusable and no key handler exists. Add tabIndex, a role, and an onKeyDown handler when onRowClick is set.
♿ Proposed fix
: rows.map((record) => (
<tr
key={rowKey(record)}
onClick={onRowClick ? () => onRowClick(record) : undefined}
+ onKeyDown={
+ onRowClick
+ ? (event) => {
+ if (event.key !== "Enter" && event.key !== " ")
+ return
+ if (event.target !== event.currentTarget) return
+ event.preventDefault()
+ onRowClick(record)
+ }
+ : undefined
+ }
+ tabIndex={onRowClick ? 0 : undefined}
+ role={onRowClick ? "button" : undefined}
className={clsx(📝 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.
| : rows.map((record) => ( | |
| <tr | |
| key={rowKey(record)} | |
| onClick={onRowClick ? () => onRowClick(record) : undefined} | |
| className={clsx( | |
| "border-0 border-b border-solid border-colorBorderSecondary last:border-b-0 hover:bg-colorFillQuaternary", | |
| onRowClick && "cursor-pointer", | |
| )} | |
| > | |
| : rows.map((record) => ( | |
| <tr | |
| key={rowKey(record)} | |
| onClick={onRowClick ? () => onRowClick(record) : undefined} | |
| onKeyDown={ | |
| onRowClick | |
| ? (event) => { | |
| if (event.key !== "Enter" && event.key !== " ") | |
| return | |
| if (event.target !== event.currentTarget) return | |
| event.preventDefault() | |
| onRowClick(record) | |
| } | |
| : undefined | |
| } | |
| tabIndex={onRowClick ? 0 : undefined} | |
| role={onRowClick ? "button" : undefined} | |
| className={clsx( | |
| "border-0 border-b border-solid border-colorBorderSecondary last:border-b-0 hover:bg-colorFillQuaternary", | |
| onRowClick && "cursor-pointer", | |
| )} | |
| > |
…ferences Two defects. Account was gated out: the shared model hides it unless access.isEE, and this app passed CLOSED_SETTINGS_ACCESS, so isEE was false. It now reads the same NEXT_PUBLIC_AGENTA_LICENSE the desktop does. And the screen wired two tabs while settings-ui already had pages for seven. API Keys, LLMs, Secrets, Webhooks and Projects are now wired too. This host passes no create/edit dialogs, so each renders read-only — the lists and their empty states, without the write affordances the desktop supplies through its own modals.
Radix requires a TooltipProvider ancestor. The desktop happens to have one high in its tree, so the bare Tooltip in these four tables only blew up once /m rendered them. Each component now provides its own, which is what a shared component has to do — it cannot assume a host's tree.
…ally needed Correcting a claim I made repeatedly: @agenta/settings-ui was antd-free only at source level. Every extracted table used InfiniteVirtualTableFeatureShell, which imports antd Table, Grid, Pagination, Tabs, Tooltip, Skeleton and Typography — so /m has been pulling antd transitively through @agenta/ui/node_modules, against its own no-antd rule. The eslint ban catches direct imports only. DataTable is the right-sized primitive: settings lists are fully materialized and single-page (that is what useStaticTable means), so they never needed virtualization. A semantic table, token-styled, with toolbar slots, a skeleton state, an empty slot and a Radix row menu. ApiKeysPage moves onto it and no longer reaches @agenta/ui/table. Four pages still do; they follow next. Spinner, Select, EmptyState, Skeleton, Divider, Segmented and Field already exist antd-free — Field covers Form.Item, so no form primitive is needed. Table was the only real gap.
The four remaining settings tables went through InfiniteVirtualTableFeatureShell, which imports antd Table/Grid/Pagination/Tabs. /m has no antd ConfigProvider, so those tables rendered in antd's default light theme — white slabs on a dark page. The eslint antd ban never caught it: it sees direct imports, not what a dependency pulls in. Secrets, LLMs, Webhooks and Projects now use DataTable, so the whole package is antd-free at runtime, not just at source level. DataTable grows the four things those pages actually needed: onRowClick (webhooks open on row click), disabled and hidden on menu items, and a title slot. Hiding every action now drops the menu entirely, and a divider left stranded at either end is trimmed. useStaticTable stays — Tools, Triggers, Organizations and Members still use it in OSS, and its only antd link is an erased `import type`.
The row carried `onClick` and nothing else. A `<tr>` is not focusable and had no key handler, so a keyboard user could not reach a clickable row at all, let alone activate it — on the webhooks table that meant the edit flow was mouse-only. When `onRowClick` is set the row now takes focus, announces as a control and handles Enter and Space (preventing Space's page scroll), with the same focus ring the other primitives use. The handler ignores events that bubbled from a control inside a cell — the row-actions kebab keeps its own keys — mirroring AgentCard and ItemRow. Rows without `onRowClick` are untouched and stay out of the tab order.
…plete Create, rename and delete each need a dialog the host renders. A host that brings none — mobile — still got "New project" and a full kebab, every one of which opened nothing. The page now derives read-only from the absence of all three dialog slots: no primary button, no empty-state button, no row menu. The desktop passes all three, so it is unchanged. Row actions move into a memoized callback so the conditional reads as one expression.
…al projects
Two tabs shipped hardcoded placeholders. API keys passed `workspaceId: ""`, and
`useApiKeys` clears its list and refuses to fetch on a blank workspace, so the tab could
never show a key and Reload issued no request — the screen already receives the route's
`workspace_id`, so it now passes it down. Projects passed `projects={[]}` with
`isLoading={false}`, which rendered "No projects in this workspace yet" to everyone; it
now reads the app's own projects query — the same key and staleTime the drawer switcher
and `useCurrentProject` use, so the tab costs no extra request — scoped to the route's
workspace.
The boundary schema gains `user_role` and `is_default_project`, both optional: it strips
unknown keys, so without them the shared table would have shown a dash for every role and
no default marker.
…arguments Bound directly as the handler, React hands mutate the MouseEvent. It is arity-0 today so the event is swallowed, but it becomes a real defect the moment the hook takes a parameter.
6cf00f1 to
cbe2cc2
Compare
8942568 to
88298de
Compare
1 participant
The settings pages moved into packages in the lane below, but their tables were still antd.
/mcannot take antd, so the tables could not follow.
DataTablein@agenta/uiis the replacement, and the settings tables now render on it on everyhost. Also here:
/msettings shows every tab it has a page for instead of only Preferences, andthe settings tables carry their own
TooltipProviderrather than assuming an ancestor supplies one.Not run in a browser — static gates only (
pnpm lint-fix24/24,tsc --noEmitcleanfor
@agenta/shared,ui,entities,entity-ui,settings-ui,oss,ee,mobile).Stacked on
pkg/settings-spine; review only this lane's diff.