Repository navigation
Conversation
Each row gets a menu with the built-in Edit, Copy link and Delete, and the row title links to the item page. A list declares its own Row actions in `ui.listView.rowActions`: `hasAccess` is resolved once per request and the synchronous `isAvailable` per row on the server, so only available keys reach the client and handlers never do. Dispatch is a new `serverAction` operation, `rowAction`, which re-reads the row through the secured context and re-runs both checks before the handler runs. A missing, denied or no-longer-eligible row answers the same "not run" outcome. Outcomes use the list's existing status line. Adds the DropdownMenu primitive (matching #2254's), a "Publish now" Row action in starter-auth with an e2e journey, and moves existing e2e journeys from the removed row Edit link to the row title link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ac0817e The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES. This is posted as a COMMENT because GitHub won't let this identity request changes on its own PR.
The dispatch design is sound. rowAction re-reads the row through the secured context.db and re-runs hasAccess and isAvailable before the handler. A missing, denied, unparseable or ineligible row all return the same notRun object, and a key that was never configured is refused. Some things need fixing before merge. The details are in the inline comments.
Must fix
packages/core/src/context/index.ts~L976: the handler receives the raw wire stringprops.id, not the parsedid. On anint autoincrementlist, the documentedupdate({ where: { id } })gets the string "42" and fails as 'Action failed'. Pass the parsed id, and typeRowActionContext.idfrom the contract rather than asstring.packages/ui/src/components/ListViewClient.tsx~L74:parseRowActionResultuses a type cast (result as {...}), which the repo rules forbid. It also duplicatesparseBulkActionResult, so share one parser.packages/ui/src/components/RowActions.tsx~L113:runhas no catch, so a rejected server action (stale action id after a deploy, or offline) fails silently with no status line.
Should fix
4. packages/core/src/context/index.ts ~L971: the re-check and the handler are not atomic. A double-click or two concurrent admins can both pass isAvailable on a one-shot action. Either run the check and the handler in context.transaction with .forUpdate(), or stop the docs implying that isAvailable guards against stale pages under concurrency.
5. packages/ui/src/components/ListView.tsx ~L139: isAvailable sees computed fields worked out under the list view's own includes, while dispatch's .first() reads the row without them. The menu and dispatch can then disagree: actions are hidden that would run, or shown and then always 'not run'.
6. packages/ui/src/components/ListView.tsx ~L86: errors are handled inconsistently. A throwing hasAccess crashes the list page, a throwing isAvailable is swallowed in the list, and at dispatch a throw returns 'Action failed' instead of the uniform not-run response. The last one weakens the indistinguishability guarantee.
7. packages/ui/src/components/ListViewClient.tsx ~L358: deleting from the row menu only calls refresh. Unlike bulk delete, it does not step back a page, so deleting the last row on the last page leaves an empty 'page 2 of 1'.
8. packages/ui/src/components/ListViewClient.tsx ~L588: the title cell is wrapped in <Link>, which nests anchors when a custom ui.cell renders its own <a>.
Nits: titleColumn sits between renderCell and its TSDoc, so the docblock now attaches to the wrong symbol (L403). resolveVisibleRowActions and serializeRowAction duplicate the bulk-action resolution helpers.
Not verified: none of this was run locally. The e2e journeys (10-row-actions.spec.ts and the edited 02/03/05/07) get their first run in CI, so watch that run. The server/client boundary looked correct in the diff: only key, label, icon, destructive and the per-row available keys cross it.
Generated by Claude Code
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
… Edit link The json-demo and tiptap-demo journeys clicked the per-row Edit link this PR removed; they now open the row through its title link. Dispatch hands the handler the parsed id, typed as the list's own id (`RowActionId<TypeInfo>`), and maps a throwing `hasAccess` or `isAvailable` to the uniform "not run" outcome. The list view resolves Bulk and Row action visibility through one `resolveVisibleActions`, which hides an action whose `hasAccess` throws, and evaluates `isAvailable` over a read with no includes, the shape dispatch re-reads the row in. The client shares one action-result parser, reports a rejected dispatch as not run or not deleted, returns to page 1 after a row delete as a bulk delete does, and puts the title link beside a custom cell rather than around it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (re-review at 985f696; posted as a comment because GitHub refuses request-changes on your own PR)
Security boundary: holds. rowAction dispatch re-reads the row through the secured context.db[listKey].where({ id: { equals } }).first(). These cases all return the same { rowAction: false, error: 'This action was not run' }, and the server tests cover each one:
- an unparseable id
- a denied or missing row (
null) - an ineligible row
- a throwing
hasAccessorisAvailable
The handler now receives the parsed id. Only key, label, icon and destructive cross to the client (serializeRowAction). hasAccess, isAvailable and handler stay on the server.
Earlier threads. I checked each reply against the current code. These nine are fixed and resolved:
- parsed id
- availability read shape
- out-of-range delete
- rejected
serverAction - nested anchors
hasAccessthrow handling- the TSDoc placement
- the parser cast and duplicate
- the
resolveVisibleActionsduplicate
The thread on the re-check and handler not being atomic (r4501284842) stays open for the pending design decision. Inline comment r4501621786 repeats it by mistake: disregard it. I've replied there and resolved that copy.
New findings, posted inline. I'd like these three fixed before merge:
RowActions.tsx:151:disabled={busy}is set in the same batch as the confirm dialog closing. Focus then returns to a disabled trigger and drops to<body>, which contradicts the documented focus return. Only the cancelled path is tested.core/src/context/index.ts:951: nothing enforces thatrowActionskeys are unique, although the type doc says they must be. With duplicate keys,find()silently runs the first handler when the user chose the second.ListView.tsx:133: when the availability read fails, it returns{}. That hides every custom action on every row, including actions with noisAvailablethat never needed the read. Fall back to the actions withoutisAvailableinstead.
Non-blocking:
- Dispatch
passesswallowshasAccessandisAvailablethrows without logging them, unlike ListView (index.ts:960). - A single-row delete now always jumps to page 1. Only the last row on a page needs that (
ListViewClient.tsx:363). - The availability read and the
hasAccessresolution run one after another withresolveListUi. Run them together withPromise.all(ListView.tsx:413).- Cost note: when no visible action declares
isAvailable, the extra read is skipped, which is correct. When one does, it is a second full-column read of the page and re-runs every computed field, including relation-declaring ones (ADR-0081).
- Cost note: when no visible action declares
BUILT_IN_CELLSis a snapshot of the mutable registry taken at module load, so which cells count as built-in depends on load order (CellRenderer.tsx:13).as unknown as AccessContextcasts inListViewRowActions.test.tsxbreak the CLAUDE.md no-casting rule.- The restating comment in
examples/starter-auth/opensaas.config.ts:309would be copied into every app the starter-auth template scaffolds.
packages/ui/src/primitives/dropdown-menu.tsx was intentionally not reviewed for changes.
Generated by Claude Code
- Keep focus on the trigger after a confirmed or running action: guard re-entry in run and mark the trigger aria-disabled instead of disabled - Refuse duplicate ui.listView.rowActions keys when the config is resolved - Fall back to the actions with no isAvailable when the availability read fails - Log a throwing hasAccess or isAvailable at dispatch - Stay on the current page after a row delete unless it emptied a later page - Resolve action access, availability and list UI concurrently - Use an explicit frozen set of built-in cells for the row title link - Drop casts from the ListView Row actions tests and a restating comment Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
Brings in the table refresh (#2259): column picker, density, page sizes, toolbar sort and the phone-card layout. The Row actions trigger now sits in the card's pinned actions cell in place of the old Edit link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
Dispatch now opens context.transaction, takes .forUpdate() on the target row, re-reads it in its own statement after the lock, re-runs hasAccess and isAvailable against that read, and runs the handler with the transaction-bound context. A handler's writes commit atomically with the check and roll back on a throw, and two concurrent dispatches of a one-shot action run the handler once. Silent failure and the redacted handler error are unchanged. Adds an escape-only concurrency test, plus tests that the handler context is transaction-bound, and documents the lock in the RowAction TSDoc, ui.md, context-api.md and the changeset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
| const list = tx.db[props.listKey] | ||
| const locked = await list | ||
| .where({ id: { equals: id } }) | ||
| .forUpdate() |
There was a problem hiding this comment.
Row lock taken before hasAccess is checked. The dispatch opens a transaction and takes .forUpdate() on the row (plus a second read) before re-running hasAccess, which does not depend on the row. A session that hasAccess refuses, but that can query the list, can hand-post { action: 'rowAction', key, id } to the public server action and take FOR UPDATE locks on any row it can read, stalling concurrent writers on those rows, and hasAccess (which may be async and slow) runs while the lock is held. bulkAction checks hasAccess before doing any work. Move the hasAccess check ahead of transaction(...) and keep the same notRun result, so a refused session never locks anything.
Generated by Claude Code
| let rows: Array<Record<string, unknown>> = ids.map((id) => ({ id })) | ||
| if (actions.some((action) => action.isAvailable)) { | ||
| try { | ||
| rows = await read.where({ id: { in: ids } }).all() |
There was a problem hiding this comment.
Every list render pays for a second read of the whole page. When any visible action declares isAvailable, the rows are read again with where({ id: { in: ids } }).all(). That read computes every computed field, and per the costs in CLAUDE.md, a computed field whose needs names a relation fetches every visible child. On a 100-row page that doubles the read, or worse. It also runs when serverAction is absent, where the client never renders custom actions (customActions = onRun ? actions : []). At minimum, skip it when there is no serverAction, and consider reading only the columns the gate needs.
Generated by Claude Code
| ): boolean { | ||
| if (!action.isAvailable) return true | ||
| try { | ||
| return action.isAvailable({ item, session }) |
There was a problem hiding this comment.
The menu and the dispatch evaluate isAvailable differently. Here it is called synchronously and its return value is used as truthy, while core's dispatch (passes in context/index.ts) awaits it. A JS config, or a cast, that supplies an async isAvailable returns a Promise here, which is always truthy, so the action is offered on every row. Dispatch then awaits the real false and answers "This action was not run". The try/catch-to-false wrapper is also written twice, once per package. Export one core helper that both the list view and dispatch call, so the menu and the dispatch can't diverge.
Generated by Claude Code
| } | ||
| } | ||
|
|
||
| const titleColumn = |
There was a problem hiding this comment.
Regression: a list whose displayed columns are all relationships gets no title. On a junction list like Audition { student, production }, displayColumns.find((c) => !relationshipRefs[c]) returns undefined. Such a row then has no row-title-link and no CARD_TITLE_CELL on the phone card layout. Before this PR it fell back to displayColumns[0]. Fall back to the first column, rendering an "Open" link beside it as the custom-cell branch already does, instead of having no title at all.
Generated by Claude Code
| SelectCell, | ||
| TimestampCell, | ||
| CalendarDayCell, | ||
| RelationshipCell, |
There was a problem hiding this comment.
RelationshipCell is not anchor-free, yet it is in the set treated as safe to wrap in a link. rendersBuiltInCell is what decides whether the title cell gets wrapped in <Link data-slot="row-title-link">, but RelationshipCell renders its own <Link>. If the title column is a relationship, the result is nested <a> elements: invalid HTML, and a click target that is unclear. That happens when getLabelFieldName auto-detects a name/title field that is a relationship, since only a configured labelField is checked for that type. Leave RelationshipCell out of this set, or exclude relationship columns from titleColumn in every branch.
Generated by Claude Code
| size="icon" | ||
| data-slot="row-actions-trigger" | ||
| aria-label={`Actions for ${rowLabel}`} | ||
| aria-disabled={busy || undefined} |
There was a problem hiding this comment.
While an action is running, other actions and confirmed deletes are dropped without feedback. aria-disabled does not stop the Radix trigger from opening. While an action runs, the user can open the menu again, choose Delete and confirm it, and run() then returns early on running.current with no status message. The confirmed delete never happens and nothing says so. Either really disable the trigger (disabled={busy}) or report that another action is still running.
Generated by Claude Code
| if (!serverAction) return | ||
| let deleted = 0 | ||
| try { | ||
| const result = await serverAction({ listKey, action: 'bulkDelete', ids: [id] }) |
There was a problem hiding this comment.
Row Delete drops the reason a delete was refused. It goes through bulkDelete, which catches every error and only counts rows. A validate or beforeOperation hook that refuses with a ValidationError ("cannot delete a published post") therefore shows only “X” not deleted. The delete action on the item page shows that message. Silent failure hides access denials, not the application's own validation messages. Consider a single-row dispatch that returns the redacted clientFailure message without a success key.
Generated by Claude Code
| }), | ||
| ) | ||
| } catch { | ||
| setBulkStatus(notRun) |
There was a problem hiding this comment.
A transport failure is reported as "not run", and the page is not refreshed. If serverAction rejects, for example on a network drop or timeout after the server has committed, the status line says "this action was not run", which may be false. The handler returns before router.refresh(), so the row's menu keeps offering an action that may already have run. Report an unknown outcome instead, and refresh in a finally.
Generated by Claude Code
| { fallback, errorPrefix = '' }: { fallback: string; errorPrefix?: string }, | ||
| ): string { | ||
| if (typeof result !== 'object' || result === null || !(key in result)) return fallback | ||
| const ran = |
There was a problem hiding this comment.
Cleanup: ran can be simpler. The key === 'bulkAction' ? 'bulkAction' in result && result.bulkAction : 'rowAction' in result && result.rowAction branch re-checks key in result, which the line above already guaranteed. const ran = Reflect.get(result, key) (or (result as Record<string, unknown>)[key], if a cast is acceptable here) does the same job without the duplicated arm.
Generated by Claude Code
| } | ||
|
|
||
| /** | ||
| * The keys of the Row actions each row offers, keyed by row id. `isAvailable` |
There was a problem hiding this comment.
Convention (CLAUDE.md, "Comments"): "Everything else that feels worth writing is rationale, and rationale does not belong beside code. Put it in an ADR...". This docblock on the internal, non-exported availableRowActionKeys ("...so a computed field sees the same dependencies here as it does when the action runs"), and the one on resolveVisibleActions ("hiding an action is a UX affordance, not the security boundary"), are rationale on internal helpers. The "public API docblocks" exception does not cover them. Move the reasoning to the PR body or an ADR.
Generated by Claude Code
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: APPROVE. I'm posting this as a COMMENT review because GitHub won't let the PR's author approve it.
Reviewed at ac0817e. As settled, I treated two things as fixed: locked dispatch inside context.transaction, and a distinct error for an unknown key.
Earlier threads
All nine unresolved threads from the previous round are fixed at head, and I've resolved them:
- RowActions focus:
aria-disabledplus a re-entry ref. passeslogging.- Availability-read fallback: actions with no
isAvailableare still offered. - Single-row delete: returns to page 1 only when it was the last row on a page past 1.
Promise.allin ListView.builtInCellComponents: now an explicit frozen set.assertUniqueRowActionKeys: runs on the sync path and the plugin path.- Test casts: removed.
- starter-auth comment: deleted.
Locked dispatch (packages/core/src/context/index.ts ~945–1011)
- ADR-0047 ordering holds.
.forUpdate().first()runs first. TheitemthatisAvailableand the handler see comes from a separate statement after the lock, so a racer that waited on the lock sees the committed row. - Silent-failure equivalence holds. An unparseable id, a missing or denied row, a
hasAccessthat returns false or throws, and anisAvailablethat returns false or throws all return the samenotRunconstant. Only an unknown key differs, which is settled. - A throwing handler rolls back. The throw propagates out of the
transactioncallback andclientFailureturns it intoAction failed. That differs fromnotRun, which is correct: the row passed every check. - The handler stays inside the transaction. It receives
engineContextOf(tx), and a nestedcontext.transactioninside it joins the owner (joinNested), so it cannot open a second connection that blocks on our own lock. The handler can only leave the transaction through a context it captured in a closure, and that is the application's choice. - No lock-ordering hazard. The row is locked
FOR UPDATEbefore the handler's own rootupdate(FOR NO KEY UPDATE, which is weaker and on the same transaction). That is the parent-first order ADR-0078 recommends, not the update-then-.forUpdate()shape it warns about.
row-action-race.test.ts
- The test is sound. The barrier sits in the
queryrule, which the lock terminal's scoped read evaluates before it locks, so both racers have read the row before either one locks it. Each racer has its own single-connection pool. - The test is falsifiable. Without the lock, both re-reads see
sentAt === nulland both handlers run. - It skips visibly. It uses
describe.skipIf(escape.kind !== 'postgres')with an[escape-only: …]title, the same pattern ascapacity-gate.test.ts.
Findings
None of these block. The 10 inline threads from this round carry the details:
- Dispatch locks the row before
hasAccessruns (index.ts:977). A session that can read the list but thathasAccessrefuses can still briefly takeFOR UPDATEon rows it can read.hasAccessdoesn't depend on the row, so it could run before the transaction opens. - A second full read of the page runs whenever any action declares
isAvailable, even with noserverAction(ListView.tsx:130). - The menu calls
isAvailablesynchronously while dispatch awaits it (ListView.tsx:107). titleColumnisundefinedwhen every displayed column is a relationship (ListViewClient.tsx:538).- A relationship cell in the title column produces nested anchors (registry.ts:81).
- An overlapping run is dropped without any feedback (RowActions.tsx:155).
- Row delete loses the hook's
ValidationErrormessage (ListViewClient.tsx:486). - A rejected server action reports "not run" and skips the refresh (ListViewClient.tsx:511).
- The
ranternary repeats the same check once per key (ListViewClient.tsx:84). - Internal docblocks on helpers that aren't exported (ListView.tsx:115). This is a style nit: the comment rule's public-API exception doesn't cover them, but they don't change behaviour.
Generated by Claude Code
Implements #2255
Part of #2246
Summary
Each row in the admin list view now has a menu of Row actions, which replaces the old per-row Edit link.
Built-ins: Edit, Copy link, and Delete. Delete asks for confirmation through
ConfirmDialog. The row title (the label column, or the first non-relationship column when the label column is hidden) links to the item page.Config:
ui.listView.rowActions: [{ key, label, icon?, destructive?, hasAccess?, isAvailable?, handler }]. The new core types areRowAction,RowActionContext,RowActionResultandRowActionIcon.iconis a closed union of 16 names, and the UI maps each name to a lucide icon.Server/client boundary:
ListViewresolveshasAccessonce per request and runs the synchronousisAvailableon the server for each row. It sendsrowActions(onlykey/label/icon/destructive) androwActionKeys(the available keys, keyed by row id). Handlers and both checks never reach the client.isAvailablesees the row's own columns, foreign keys and computed fields, and no relations. That is the same shape as the dispatch re-read, so the menu and the dispatch decide on the same data.Dispatch: a new
serverActionoperation,{ listKey, action: 'rowAction', key, id }, returns{ rowAction: true, message? }or{ rowAction: false, error }. It never returnssuccess, so the redirect-on-success wrapper does not intercept it. The steps are:key.hasAccess.idthrough the list's id type.context.db[listKey].where({ id: { equals } }).first().isAvailableon the re-read row.handler({ listKey, id, item, context }).A failure at step 2, 3, 4 (missing or denied) or 5 returns the same object,
{ rowAction: false, error: 'This action was not run' }, so a caller cannot tell a denied row from a missing one (Silent failure). Handler errors go through the existingclientFailureredaction.Delete shows on the existing list-level rule (
canDeleteListplushideDelete). It dispatches a one-idbulkDeleteso it gets{ deleted, total }back rather than a redirectingsuccess. A denied delete reports“<label>” not deletedwith no reason.Outcomes go to the existing status line (
useBulkStatus, persisted in sessionStorage), so they survive therouter.refresh().Keyboard: the menu is the Radix DropdownMenu. Enter, Space or ArrowDown opens it, arrows/Home/End move, and Escape closes it and returns focus to the trigger. A confirmation opens only after the menu has handed focus back (
onCloseAutoFocus), so cancelling the dialog also returns focus to the trigger.starter-auth: adds a
Publish nowRow action, available only whenstatus !== 'published'.Overlap with #2259 (#2254 table refresh)
#2259 also adds
packages/ui/src/primitives/dropdown-menu.tsx. This PR's copy of that file and of theprimitives/index.tsexport block is byte-identical to the one onclaude/issue-2254-table-refresh, so whichever PR merges second should rebase cleanly on those files. This PR still keeps the sticky Actions column cell and only swaps its contents, which leaves the phone-card replacement to #2259.E2E changes
Journeys that clicked the row's
Editlink (02, 03, 05-item-view, 07) now click[data-slot="row-title-link"]. The new10-row-actions.spec.tscovers:Publish now, with the status line reading "Post published", the status cell turning Published, and the action disappearing from the menuDocs
docs/content/reference/ui.md: a new "Row actions" section with an options table, plusDropdownMenuin the primitives list.docs/content/reference/context-api.md: a newrowActionsection.minorfor@opensaas/stack-coreand@opensaas/stack-ui, and the requiredcreate-opensaas-apppatch.Test plan
server-actions.test.ts, against a Test context. It checks that the handler receives listKey/id/re-read item/secured context; thatisAvailableis re-run on the re-read row; that a row that became denied returns exactly the same outcome as a missing row; thathasAccessis re-checked; that an invalid id and an unknown key are refused; and that a handler throw is redacted.ListViewRowActions.test.tsx, a realListViewovercreateTestDatabase. It checks per-row availability fromisAvailable, thatisAvailablesees no relations, and thathasAccessdrops actions and no functions are serialised.RowActions.test.tsx. It checks the keyboard open/move/close and focus return (including after a cancelled confirm), per-row menu contents, dispatch payload and status line, the not-run message, delete confirm with "not deleted", hidden Delete, and Copy link.pnpm build, UI and coretsc, starter-authgenerate+tsc(generated artifacts unchanged)pnpm lint,pnpm format:check,pnpm manypkg fix,check:scaffolder-changeset,check:doc-ts-blocksListUiOptions.test.tsxwhile it booted a database under load, and that file passes on its own.Generated by Claude Code