ENG-2186 Grammar › Nodes drill-down and settings navigation primitive - #1375
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
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
| cancelled = true; | ||
| window.clearTimeout(debounceRef.current); | ||
| // Navigating away lands right after a keystroke, and the buffer block is deleted next. | ||
| flushPendingChanges(); |
There was a problem hiding this comment.
Pre-existing bug this PR has to fix rather than inherit: the cleanup used to clearTimeout the pending 250ms write without running it, and the buffer effect then deleted the block carrying that edit. Only a deliberate tab click unmounted this before, so the window was rarely hit. A back button and a breadcrumb are fast, keyboard-adjacent unmount paths landing right after a keystroke, which turns it into a reproducible edit loss. Active only when isNewSettingsStoreEnabled().
Known limit: this recovers an edit Roam has already committed and debounced. A keystroke Roam has not committed yet is still lost.
| * 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.
| --dg-secondary: #5f57c0; | ||
| } | ||
|
|
||
| /* Hand-rolled, not Tailwind: apps/roam has no Tailwind build (tailwind.config.ts sets |
There was a problem hiding this comment.
AGENTS.md says prefer Tailwind, so this needs justifying. apps/roam/tailwind.config.ts sets content: [] and is commented "required for editor support" — there is no Tailwind build for this app, and the built extension.css contains no utilities. Classes only work when Roam's own stylesheet happens to ship them.
Found the hard way: a <h3 className="m-0"> here fell back to host heading margins and opened a ~440px gap. flex-shrink: 0 below is load-bearing for the same class of reason — without it the header is a shrinkable flex item and overlaps the scrolling body.
| // 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
Remove the per-node-type tabs from the settings rail and drill into a node type from Grammar › Nodes instead: node list → node type → Index/Template sub-pages. Navigation state is a path owned by SettingsDialog, exposed through context. Builds on ENG-2189's settingsTabs registry rather than duplicating it, so tab ids and their legacy aliases stay in one place. A saved deep link carrying a node type uid, which used to select that node's own rail tab, now opens the node's page inside Grammar › Nodes. Also lands ENG-2188: NodeConfig's seven inner tabs become one vertical page grouped Identity / Recognition / Creation, reusing ENG-2189's SettingsGroup, with Index and Template as drill-down rows to their own pages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Back and the breadcrumb unmount an editor immediately, so BaseTextPanel and DiscourseNodeColorSetting now run their pending write during cleanup instead of clearing the timer. Both keep their existing validation guard, so an invalid tag, format or shortcut is still not persisted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The embedding block ref is copied out of the template, so the editor is useful here. It is collapsed by default to keep the node page short, and Collapse unmounts its children, so the ephemeral buffer block is only created while open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The collapse toggle and the panel's own Label both read Template. The panel now takes an optional title so the caller can own the header, and the collapsed body is indented to line up under the toggle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Supersedes the per-panel pendingWriteRef added earlier on this branch. That version stored the inner 100ms timer id in the same ref as the outer 250ms one, so unmounting inside that window cleared the inner timer and then ran the pending write, issuing syncToBlock and setter twice. useDeferredWrite keeps one timer and one registry entry per panel, so a commit runs exactly once whether it is triggered by the timer, by unmount, or by a caller flushing before it reads. The number and select panels now go through it too, which is what lets a reader commit them. flushPendingSettingWrites is awaitable: the commits only start a Roam block update, so a caller that read straight after a synchronous flush could still observe the previous value. setBlockProps tracks its in-flight updates so the flush can wait for them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
settingsNavReducer and resolveInitialSettingsPath decide which tab and drill-down a deep link opens, and both fail silently when wrong: the user simply lands somewhere else. The alias map is pinned entry by entry for the same reason, plus a check that every alias target is a real tab. Widens the vitest include so tests can sit next to the settings modules they cover rather than only under src/utils. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resolveInitialSettingsPath rebuilt the list of known tab ids to decide whether an incoming id is a tab or a node type uid. That classification belongs next to the ids themselves, so the passthrough in resolveSettingsTabId and the branch here read from one definition. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review feedback on the Grammar > Nodes drill-down: - Recognition is gone. Index and Format join Identity, Index first, and Specification moves to a Legacy group that holds whatever is mid-migration. Format's "DEPRECATED" note goes with it: Specification is the one in flux. - The Specification toggle reads "Enabled" — its lowercase block text stays the storage key via blockKey — and the query builder only renders while it is on, rather than sitting greyed out underneath. - The back button moves inline with the page title; the breadcrumb beneath already says where it leads, so a second row naming the parent was noise. - The node list page gets the same inset as every other panel, so the add-node input's focus ring is no longer clipped by the scrolling body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review feedback: per-node attribute settings move to the very bottom, after the sync-gated Suggestive mode group. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review choice C: 44px between groups, against 24px of padding inside a row, so a section break cannot be mistaken for a row break. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5c92bfd to
92a4176
Compare
.dg-settings-group was left unclosed when the heading block and this branch's route CSS were merged, so with CSS nesting every later rule — :root, the route container, and the drill-down's scroll body — became a descendant rule of it and stopped matching. Grammar > Nodes lost its scroll and the brand variables. A test now parses the stylesheet and pins those rules at top level, since the bundler accepts the nested form without complaint. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
https://www.loom.com/share/55852dd333654bb08fd8c3c03ba8b797
I decided to merge the work of ENG-2186: Grammar › Nodes drill-down and settings navigation primitive
and ENG-2188: Node type settings are spread across seven inner tabs; put them on one page into 1 PR.
Reviewer brief
Grammar › Nodesdrills node list → node type → Index/Template, with a back button and breadcrumb at each depth. A node type's settings are one vertical page instead of seven inner tabs.EphemeralBlocksPanel.tsxis the riskiest change and the only one touching persistence. Everything else is navigation and layout.settingsTabsregistry instead of adding a second source of tab ids.resolveInitialSettingsPathextendsresolveSettingsTabId: an id that is not a known tab is treated as a node type uid and opens that node insideGrammar › Nodes, which is how the old per-node deep links keep working.**Risk or follow-up:**
- the Index row's description is proposed copy and needs sign-off — ENG-2188 notes Index has no description in code today. - Suggestive mode stays on the node page rather than moving to the Admin panel. That is in ENG-2188's Solution but not its Done When, and it needs its own node selector in Admin. Needs a ticket. - `apps/roam` has no Tailwind build (`tailwind.config.ts` sets `content: []`), so utility classes only apply when Roam's own stylesheet ships them. New layout is therefore hand-rolled CSS. Existing Tailwind in this app is partly inert as a result, `text-neutral-dark` included. Worth its own ticket; not touched here.Verification
pnpm install --frozen-lockfile, thenpnpm ci:validatefrom the repo root: green.pnpm lintinapps/roam: 0 errors. Prettier clean. Production build:built with 0 errors.selectedTabIdopeners still land.$scope-checkagainst the ENG ticket and final diff.Done When:EphemeralBlocksPanelflush fix is a prerequisite, not a drive-by: unmount discarded the pending debounced write and then deleted the block carrying it. A back button makes that a fast, keyboard-adjacent path.Local delegated full review