Fix RecordTable overflow; expand platform integrations editor - #1354
Conversation
Replace the card list and bottom panel with RecordTable mode=expand, where the expanded row (or orphaned pane for /new) renders the create/edit form. Use createListDetailRoute for selection URLs and replaceLocation after create, delete, and cancel. Document the editor-in-expand exception in the 0010 page inventory.
Give the shared record table a horizontal scroller (and clamp primary cells) so wide package rows no longer clip under the card edge, and register list/detail/new routes so admin platform integrations can expand into the edit form.
📝 WalkthroughWalkthroughAdmin platform integrations now use URL-based list, create, and detail routes. The page uses a searchable ChangesPlatform integrations and RecordTable
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AdminPlatformIntegrationsRoute
participant RecordTable
participant PlatformIntegrationsHandler
Browser->>AdminPlatformIntegrationsRoute: open create or detail route
AdminPlatformIntegrationsRoute->>RecordTable: render filtered integrations
Browser->>AdminPlatformIntegrationsRoute: submit integration form
AdminPlatformIntegrationsRoute->>PlatformIntegrationsHandler: send create or update action
PlatformIntegrationsHandler-->>AdminPlatformIntegrationsRoute: return saved integration
AdminPlatformIntegrationsRoute-->>Browser: navigate to saved integration detail
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
🔎 Preview deployed: https://kody-pr-1354.kody-a99.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5c948a0. Configure here.
| if (!routeData && needsLoad && typeof document !== 'undefined') { | ||
| status = 'loading' | ||
| loadingForHref = currentHref | ||
| handle.queueTask(loadPlatformIntegrations) |
There was a problem hiding this comment.
Search keystrokes refetch list
Medium Severity
Integration search filters with filterApps on the loaded apps list, but each RecordTableSearch update rewrites the URL and needsLoad treats any currentHref !== lastLoadedHref as a full reload. That sets status to loading, hides the table, and POSTs to the JSON API on every keystroke even though the request ignores q.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5c948a0. Configure here.
|
|
||
| function handleEdit(app: AdminPlatformIntegrationApp) { | ||
| editingApp = app | ||
| pendingLogoBase64 = undefined |
There was a problem hiding this comment.
Edit form stale after save
Medium Severity
After a successful edit save the route stays on the detail URL and calls form.reset() while the form key remains edit-${slug}. Uncontrolled fields keep their first-mount defaultValue/defaultChecked, so the UI can show pre-save values even though applyData refreshed apps from the server.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5c948a0. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/worker/client/routes/admin-platform-integrations.tsx (1)
820-836: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated cancel button.
Both branches render the same button with the same handler, the same disabled condition, and the same style. Only the label differs.
♻️ Proposed refactor
- {isEditing ? ( - <button - type="button" - disabled={actionState !== 'idle'} - mix={[on('click', cancelEditor), css(secondaryButtonCss)]} - > - Cancel edit - </button> - ) : ( - <button - type="button" - disabled={actionState !== 'idle'} - mix={[on('click', cancelEditor), css(secondaryButtonCss)]} - > - Cancel - </button> - )} + <button + type="button" + disabled={actionState !== 'idle'} + mix={[on('click', cancelEditor), css(secondaryButtonCss)]} + > + {isEditing ? 'Cancel edit' : 'Cancel'} + </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/client/routes/admin-platform-integrations.tsx` around lines 820 - 836, Collapse the duplicated button in the isEditing conditional into a single shared button, retaining its type, disabled condition, click handler, and styling while conditionally rendering only the label as “Cancel edit” or “Cancel”.packages/worker/client/routes/record-table.node.test.ts (1)
164-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative half of the expand contract.
The test asserts the scroller styles, which now exist for every mode. It does not lock the behavior the change actually introduced for
expand: the scroller must stay uncapped, whilepaneandnonekeep the height cap. A regression that capsexpandwould still pass this test.🧪 Suggested extra assertions
// Remix serializes longhands with a space after the colon. expect(html).toContain('overflow-x: auto') expect(html).toContain('overflow: clip') + // `expand` renders the record inside the scroller, so it must not cap height. + expect(html).not.toContain('max-height') + + const cappedHtml = await renderToString( + jsx(RecordTable, { mode: 'pane', ariaLabel: 'Packages', columns, rows }), + ) + expect(cappedHtml).toContain('max-height') + expect(cappedHtml).toContain('overflow-y: auto') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/client/routes/record-table.node.test.ts` around lines 164 - 177, Strengthen the RecordTable expand-mode test around RecordTable so it verifies the expand scroller remains uncapped, while pane and none retain the height cap. Add assertions that distinguish the mode-specific height styles, preserving the existing horizontal-overflow checks.packages/worker/client/routes/record-table.tsx (1)
267-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an in-cell clamp instead of
maxWidthon this<td>.
primaryCellCssis applied to a table cell, wheremax-widthis not reliably respected in auto table layout and cannot ensure the primary name does not force the table width. ApplyrecordCellClamp(...)to block content inside the primary cell, or use another established inner-content clamp, otherwise this cap provides little sizing constraint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/client/routes/record-table.tsx` around lines 267 - 273, Replace the maxWidth-based sizing in primaryCellCss with the established recordCellClamp(...) applied to block content inside the primary cell. Preserve the existing responsive whiteSpace behavior while ensuring the primary name is constrained within the cell rather than relying on table-cell max-width.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/client/routes/admin-platform-integrations.tsx`:
- Around line 857-869: Key platform-integrations loading state to the
payload-relevant dataHref rather than the full currentHref, so query updates and
list/detail navigation do not refetch or unmount the table. Update the needsLoad
comparisons and the corresponding assignments inside loadPlatformIntegrations,
including the nearby render logic, while preserving normal loading for genuinely
different payloads.
- Around line 434-445: Preserve the active search query across integration
navigation by passing the current search string to the route builders in the
post-create navigation, handleDelete, cancelEditor, and startCreateIntegration.
Reuse the same search value already supplied by the row links with
buildListHref() and buildNewHref(), so q remains intact when leaving or creating
an integration.
---
Nitpick comments:
In `@packages/worker/client/routes/admin-platform-integrations.tsx`:
- Around line 820-836: Collapse the duplicated button in the isEditing
conditional into a single shared button, retaining its type, disabled condition,
click handler, and styling while conditionally rendering only the label as
“Cancel edit” or “Cancel”.
In `@packages/worker/client/routes/record-table.node.test.ts`:
- Around line 164-177: Strengthen the RecordTable expand-mode test around
RecordTable so it verifies the expand scroller remains uncapped, while pane and
none retain the height cap. Add assertions that distinguish the mode-specific
height styles, preserving the existing horizontal-overflow checks.
In `@packages/worker/client/routes/record-table.tsx`:
- Around line 267-273: Replace the maxWidth-based sizing in primaryCellCss with
the established recordCellClamp(...) applied to block content inside the primary
cell. Preserve the existing responsive whiteSpace behavior while ensuring the
primary name is constrained within the cell rather than relying on table-cell
max-width.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a68f3f70-5573-4352-9894-42b0d9b259f9
📒 Files selected for processing (9)
docs/contributing/decisions/0010-account-record-table/page-inventory.mdpackages/worker/client/lazy-route.tsxpackages/worker/client/routes/account-packages.tsxpackages/worker/client/routes/admin-platform-integrations.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/record-table.node.test.tspackages/worker/client/routes/record-table.tsxpackages/worker/src/app/router.tspackages/worker/universal/routes.ts
| function cancelEditor() { | ||
| resetSelectionState() | ||
| replaceLocation(platformIntegrationsRoute.buildListHref()) | ||
| handle.update() | ||
| } | ||
|
|
||
| const primaryButtonCss = getPillButtonCss({ size: 'sm' }) | ||
| const secondaryButtonCss = getGhostButtonCss({ size: 'sm' }) | ||
| const dangerButtonCss = getDangerPillCss({ size: 'sm' }) | ||
| function startCreateIntegration() { | ||
| if (actionState !== 'idle') return | ||
| resetSelectionState() | ||
| replaceLocation(platformIntegrationsRoute.buildNewHref()) | ||
| handle.update() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cancel, create, and post-create navigation discard the active search.
cancelEditor calls buildListHref() and startCreateIntegration calls buildNewHref(), both with no argument, so the q parameter is dropped. handleDelete at Line 428 and the post-create navigation at Line 392 do the same.
The row links at Lines 958-963 do pass the current search through, so the behavior is inconsistent: opening a record keeps the filter, and leaving it clears the filter. createListDetailRoute accepts a search string on all three builders.
🔧 Proposed fix
function cancelEditor() {
+ const search = new URL(readCurrentRouterHref(handle), 'http://localhost')
+ .search
resetSelectionState()
- replaceLocation(platformIntegrationsRoute.buildListHref())
+ replaceLocation(platformIntegrationsRoute.buildListHref(search))
handle.update()
}
function startCreateIntegration() {
if (actionState !== 'idle') return
+ const search = new URL(readCurrentRouterHref(handle), 'http://localhost')
+ .search
resetSelectionState()
- replaceLocation(platformIntegrationsRoute.buildNewHref())
+ replaceLocation(platformIntegrationsRoute.buildNewHref(search))
handle.update()
}Apply the same search argument at Line 392 and Line 428.
📝 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.
| function cancelEditor() { | |
| resetSelectionState() | |
| replaceLocation(platformIntegrationsRoute.buildListHref()) | |
| handle.update() | |
| } | |
| const primaryButtonCss = getPillButtonCss({ size: 'sm' }) | |
| const secondaryButtonCss = getGhostButtonCss({ size: 'sm' }) | |
| const dangerButtonCss = getDangerPillCss({ size: 'sm' }) | |
| function startCreateIntegration() { | |
| if (actionState !== 'idle') return | |
| resetSelectionState() | |
| replaceLocation(platformIntegrationsRoute.buildNewHref()) | |
| handle.update() | |
| } | |
| function cancelEditor() { | |
| const search = new URL(readCurrentRouterHref(handle), 'http://localhost') | |
| .search | |
| resetSelectionState() | |
| replaceLocation(platformIntegrationsRoute.buildListHref(search)) | |
| handle.update() | |
| } | |
| function startCreateIntegration() { | |
| if (actionState !== 'idle') return | |
| const search = new URL(readCurrentRouterHref(handle), 'http://localhost') | |
| .search | |
| resetSelectionState() | |
| replaceLocation(platformIntegrationsRoute.buildNewHref(search)) | |
| handle.update() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/client/routes/admin-platform-integrations.tsx` around lines
434 - 445, Preserve the active search query across integration navigation by
passing the current search string to the route builders in the post-create
navigation, handleDelete, cancelEditor, and startCreateIntegration. Reuse the
same search value already supplied by the row links with buildListHref() and
buildNewHref(), so q remains intact when leaving or creating an integration.
| const needsStaleRefresh = | ||
| consumeStaleNavigationData(currentHref) && !routeData | ||
| const needsLoad = | ||
| (status === 'loading' || | ||
| currentHref !== lastLoadedHref || | ||
| needsStaleRefresh) && | ||
| currentHref !== lastFailedHref && | ||
| loadingForHref !== currentHref | ||
| if (!routeData && needsLoad && typeof document !== 'undefined') { | ||
| status = 'loading' | ||
| loadingForHref = currentHref | ||
| handle.queueTask(loadPlatformIntegrations) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Typing in the search field refetches the list and unmounts the table.
RecordTableSearch.onInput calls replaceLocation on every keystroke. replaceLocation dispatches the router navigate event, so the render function runs again with a new currentHref that carries the updated q.
In that render, needsLoad at Line 859 is true because currentHref !== lastLoadedHref. Line 866 then sets status = 'loading' and Line 868 queues loadPlatformIntegrations. The RecordTable is gated on status === 'ready' at Line 905, so the whole table — including the focused search input — is removed from the DOM on the first keystroke, and the field loses focus. Each keystroke also refetches the full integrations list, although filterApps already filters client-side and the API response does not depend on q.
Key the load gate on the part of the URL the payload actually depends on, not on the full href. The list payload is the same for the list, new, and detail paths and for every value of q.
🐛 Proposed fix: compare a payload-relevant key instead of the full href
+ /** The list payload is identical for every path and query on this screen. */
+ const dataHref = adminPlatformIntegrationsApiPathThen use dataHref where the load gate currently compares hrefs:
const needsStaleRefresh =
consumeStaleNavigationData(currentHref) && !routeData
const needsLoad =
(status === 'loading' ||
- currentHref !== lastLoadedHref ||
+ dataHref !== lastLoadedHref ||
needsStaleRefresh) &&
- currentHref !== lastFailedHref &&
- loadingForHref !== currentHref
+ dataHref !== lastFailedHref &&
+ loadingForHref !== dataHref
if (!routeData && needsLoad && typeof document !== 'undefined') {
status = 'loading'
- loadingForHref = currentHref
+ loadingForHref = dataHref
handle.queueTask(loadPlatformIntegrations)
}Apply the same key to the assignments inside loadPlatformIntegrations (Lines 181-222) and to Lines 854-855.
If you prefer to keep the href-based gate, then keep the previous data on screen during a refetch instead of blanking it: render the RecordTable whenever apps.length > 0 and pass busy for the in-flight state, which is what the busy prop exists for.
Also applies to: 919-930
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/client/routes/admin-platform-integrations.tsx` around lines
857 - 869, Key platform-integrations loading state to the payload-relevant
dataHref rather than the full currentHref, so query updates and list/detail
navigation do not refetch or unmount the table. Update the needsLoad comparisons
and the corresponding assignments inside loadPlatformIntegrations, including the
nearby render logic, while preserving normal loading for genuinely different
payloads.
2 participants
Summary
RecordTableclipping wide rows by giving the table a horizontal scroller (overflow-x: auto), clipping the card withoverflow: clip, and clamping primary cell names so package lists stay readable./admin/platform-integrationsonto the sameRecordTablewithmode="expand", so selecting a row unfolds the create/edit form under that row (create uses/newas an orphaned pane below the table).Test plan
/account/packagesno longer clips Tags/Updated/admin/platform-integrationsexpand → edit form, Create →/newSystem recap — composes existing primitives (low risk)
Mode: recap · Base:
main@28b74f25· Head:5c948a06Classification: composes — shared RecordTable overflow fix and admin UI migration onto existing list/detail routing; no new backend primitives.
Primitives touched
app-uiSystem map
Admin platform integrations and account packages both render through the shared RecordTable; detail selection stays URL-driven.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
RecordTableoverflow: hidden/admin/platform-integrationsSummary by CodeRabbit
New Features
Improvements
Documentation