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.
| // 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
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>
f4058f2 to
9ffa982
Compare
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
| segmentsOf, | ||
| type SettingsNavAction, | ||
| type SettingsPath, | ||
| } from "../utils/settingsNavigation"; |
| @@ -0,0 +1,69 @@ | |||
| import React from "react"; | |||
| import { Button } from "@blueprintjs/core"; | |||
| import { buildBreadcrumbTrail } from "../utils/settingsNavigation"; | |||
| --dg-secondary: #5f57c0; | ||
| } | ||
|
|
||
| /** Hand-rolled: apps/roam has no Tailwind build, so only utilities Roam ships resolve. */ |
There was a problem hiding this comment.
"apps/roam has no Tailwind build"?
Please use tailwind.
2 participants
Reviewer brief
Result: the rail no longer grows one tab per node type.
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.Review focus: the route. Navigation state is a path array (
["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.Review focus: the write plumbing.
pendingSettingWrites.ts,useDeferredWrite,useLegacyBlockSync, the write tracker insetBlockProps.ts, and theEphemeralBlocksPanelflush are here because the back button made two existing bugs easy to hit:A commit now runs exactly once, whether the timer fires, the panel unmounts, or a caller flushes. It awaits the legacy block write before it refreshes the config tree. ENG-2185 uses the flush so an export reads the setting the user just changed.
Risk: panels no longer cancel writes on unmount, so closing the dialog right after typing writes the value instead of dropping it. That is the intended behaviour, but it is a change from before.
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: the write plumbing above. It is a prerequisite, not a drive-by: the back button turns the existing cancel-on-unmount bug into reproducible data loss.Local delegated full review