improvement(tables): announce locks on open instead of a header chip - #5979
Conversation
Drop the lock entry from the table header actions. A locked table now announces itself once on open via an info toast carrying the Lock settings action for admins; the breadcrumb dropdown remains the permanent entry point.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The table page dismisses that notice when you leave the table, when ToastProvider clears route-scoped toasts during render on pathname change instead of in a Reviewed by Cursor Bugbot for commit b505ef6. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThe PR replaces the table-header lock chip with a one-time lock announcement and tightens toast lifecycle handling.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported stale-permission action is dismissed when lock-settings access is lost.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/tables/[tableId]/lock-copy.ts | Updates lock-notice documentation to reflect the new on-open informational announcement. |
| apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx | Removes the header lock action, adds the one-time announcement, and safely dismisses captured actions when access or table context changes. |
| packages/emcn/src/components/toast/toast.tsx | Moves route-scoped toast filtering into the provider render transition so newly mounted route notifications are not swept. |
Reviews (7): Last reviewed commit: "fix(tables): reset the announce latch wh..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The lock announcement fires once per table, so a canAdmin that is still false because permissions have not resolved permanently drops the toast's Lock settings action. Arm the one-shot on the permissions decision rather than on table resolution.
The toast provider clears route-scoped toasts in a pathname effect that runs after child effects, so an announcement fired on a warm-cache navigation was added and removed in the same commit. Opt these toasts out of the sweep and dismiss them on unmount so they still don't trail the user.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f3d8588. Configure here.
A toast's action is captured at creation, so a viewer whose admin access is revoked while the announcement is on screen kept a Lock settings button that opened nothing. Dismiss the notice when the action is no longer available.
|
Addressed the outstanding toast-action lifecycle concern in cad3600. The observation was correct: a toast's Rather than trying to keep a live action in a static toast, the view now drops the notice when the action stops being valid: useEffect(() => {
if (canOpenLockSettings || !blockedToastIdRef.current) return
toast.dismiss(blockedToastIdRef.current)
blockedToastIdRef.current = null
}, [canOpenLockSettings])This pairs with the unmount cleanup added in f3d8588 — the view owns its toasts' lifecycle end-to-end, which is what One direction deliberately not handled: a viewer who gains admin mid-toast keeps the buttonless notice for its remaining duration. That fails safe (the breadcrumb dropdown is the permanent entry point) and re-creating a toast under the user to add a button would be worse than leaving it. |
|
@cursor review |
The dismiss effect treated "has no access" the same as "just lost access", so on a warm-cache mount — where the announcement and the dismiss both run in the mount commit — a non-admin's notice was torn down the moment it was created. Dismiss on the true-to-false transition only.
|
@cursor review |
Switching tables reuses this component rather than remounting it, and the notice is exempt from the provider's route sweep, so a locked table's announcement stayed on screen over the next table — where its action would have opened that table's lock settings instead. Key the cleanup on tableId.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 072f658. Configure here.
…ffects The provider cleared route-scoped toasts from a pathname effect. Effects run child-first, so it also swept toasts the newly rendered route had just raised: any toast added from a child's mount effect — which is what happens whenever that child's data is already cached — was appended and filtered out in the same commit, before it painted. Move the sweep into render, where it runs before children render, so only toasts predating the navigation are cleared. Timers and heights were already reconciled by effects keyed off `toasts`, so this stays a pure state update. Drops the persistAcrossRoutes workaround the table lock notice needed to dodge the bug; its tableId-keyed cleanup stays, covering embedded swaps that change the prop without a route change.
|
Added the underlying The bug. The fix. Move the sweep into render, where it runs before children render, so only toasts predating the navigation are cleared: const [sweptPathname, setSweptPathname] = useState(pathname)
if (pathname !== sweptPathname) {
setSweptPathname(pathname)
setToasts((prev) =>
prev.some((t) => !t.persistAcrossRoutes) ? prev.filter((t) => t.persistAcrossRoutes) : prev
)
}This is React's "adjusting state during render" pattern, and it stays a pure state update — the previous version cleared timers inline, which would be an illegal side effect during render. It doesn't need to: the timer effect already reconciles orphans against Behavior delta: the old effect also ran on mount. That was always a no-op — a freshly mounted provider has an empty stack — so nothing is lost. Knock-on: Net −9 lines across both files. No tests exist for this component, so this is reasoned, not verified — the paths worth exercising by hand are: raise a toast then navigate away (should clear), navigate into a route that toasts from cached data (should now survive), and the permissions provider's |
|
@cursor review |
The tableId cleanup dismissed the notice on departure but left the latch holding that table's id, so returning before another table announced — a quick there-and-back, or a second table that never loaded — found the latch matching and stayed silent. Leaving ends the visit, so clear the latch with it.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b505ef6. Configure here.
1 participant
Summary
The announcement is marked shown as soon as the table resolves, so an admin who just set locks in the panel doesn't immediately get toasted about them.
Type of Change
Testing
Tested manually.
bun run type-check,bun run lint:check, and the full audit suite (boundaries, api-validation:strict, utils, zustand-v5, react-query, client-boundary, bare-icons, icon-paths, realtime-prune, skills:check, agent-stream-docs:check) all pass.Checklist