ENG-2186 Grammar › Nodes drill-down and settings navigation primitive - #1375
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
ac26028 to
17ad1b7
Compare
| * is not a tab at all is a node type uid from when every node type had its own rail tab; | ||
| * those links now open the node's page inside Grammar > Nodes. | ||
| */ | ||
| export const resolveInitialSettingsPath = ( |
There was a problem hiding this comment.
This is the ENG-2186 "existing deep links still resolve" requirement. resolveSettingsTabId (ENG-2189) maps the renamed tabs; its passthrough comment assumes per-node tabs still exist, keyed by node page uid. This PR removes those tabs, so an id that is not a known tab is now read as a node type uid and opens that node inside Grammar › Nodes instead of selecting a tab that no longer exists.
Deliberately no separate deep-link API: selectedTabId plus this resolution covers it, and ENG-2189 already addresses individual rows via settingAnchor.
| // A deleted node type or stale deep link resolves to nothing; return to the list. | ||
| const isStalePath = Boolean(nodeTypeUid) && !node; | ||
| useEffect(() => { | ||
| if (isStalePath) goToDepth(0); |
There was a problem hiding this comment.
The path outlives this panel — renderActiveTabPanelOnly unmounts and remounts it on every tab switch — so it can point at a node type deleted in the meantime, or arrive stale from a saved link. goToDepth(0) rather than pop() so a depth-2 stale path converges in one dispatch instead of two.
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
bd1b4fe to
592a07c
Compare
592a07c to
5c92bfd
Compare
5c92bfd to
92a4176
Compare
9dba941 to
fd1a59a
Compare
f4058f2 to
9ffa982
Compare
There was a problem hiding this comment.
I’m blocking this on scope and size. ENG-2186 covers the node list and drill-down navigation. The proposal explicitly limits M1 to presentation changes without changing how settings work.
Please remove the settings-write refactor and export flush infrastructure from this PR and track them in a separate ticket.
The persistence issue appears to be pre-existing. If the new Back button makes it easier to encounter, call that out in the separate ticket with the specific failure, reproduction steps, and acceptance criteria. That will make the problem and proposed fix much easier to review.
If any part is needed to prevent a regression introduced by this PR, please identify that dependency explicitly.
0ae32bc to
324529f
Compare
2c9d5c2 to
6aee216
Compare
Replaces the per-node-type rail tabs with a single Grammar › Nodes entry that drills from the node list into a node type's page, with a back button and a breadcrumb. The route state is a path reducer (select-tab / push / pop / truncate) exposed through SettingsNavContext; an incoming id that is not a known tab is treated as a node type uid so the old per-node deep links keep resolving. Also carries the write plumbing the drill-down made necessary. Panels debounce their writes and used to cancel the pending one on unmount; a back button puts an unmount one keystroke from every input, so a commit now runs exactly once by timer, flush, or unmount (useDeferredWrite). Legacy block writes go through an awaitable hook registered with the same in-flight tracker, and every commit re-reads the config tree only after its write resolves, so a reader that flushes first (the export) sees the value it was just given. The node type page itself keeps its inner tabs here; ENG-2188 rebuilds it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Roam ships Tailwind, so the hand-rolled dg-settings-* rules were redundant with the utilities the rest of apps/roam already uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The registry, flushPendingSettingWrites and trackRoamWrite have no consumer at this point in the stack; their only caller is Export, two PRs above. Tracked in ENG-2286, which must land before ENG-2185. useDeferredWrite still commits on unmount rather than cancelling: Back and the breadcrumbs added here are fast unmount paths mid-debounce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverts the deferred-write rework, the template buffer flush and its delete ordering, and the node colour commit-on-unmount. Only the optional title on the template panel stays, which the drill-down needs. This PR is now presentation only. The debounce bug is ENG-2287 and the flush-before-read is ENG-2286. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6aee216 to
d54edbc
Compare
|
@mdroidian i removed the deferred write settings. for this PR all changes are just cosmetic reorganization. I recorded the follow-up ticket: ENG-2286: Settings writes: pending-write registry and flush-before-read |
This branch was successfully deployed
2 participants
Reviewer brief
Grammar › Nodesopens a list of node types. Clicking one opens that node's page in place, with a back button and aGrammar › Nodes › <node>breadcrumb. The node page itself is unchanged here (still the tabbedNodeConfig); ENG-2188 (ENG-2188 Node type settings are spread across seven inner tabs; put them on one page #1420) rebuilds it on top of this PR.["grammar-nodes", nodeTypeUid]) owned by a reducer with four actions:select-tab,push,pop,truncate. BlueprintTabsonly seespath[0]; the Nodes panel reads the rest throughSettingsNavContext. Deep links still work:resolveInitialSettingsPathtreats any id that is not a registered tab as a node type uid and opens it insideGrammar › Nodes. A uid that no longer matches a node falls back to the list.titleon the template panel, which the drill-down needs to render it without a label.Verification
tsc --noEmitand eslint report no errors or warnings forapps/roam. Production build completes with 0 errors.selectedTabIdland on that node's page; typing and then pressing Back immediately keeps the edit.Loom video
https://www.loom.com/share/ffd805d8bbfa47f5ae418d0a85c79906
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: none. The write plumbing has been removed to ENG-2286 and ENG-2287.Local delegated full review