Skip to content

ENG-2188 Node type settings are spread across seven inner tabs; put them on one page - #1420

Merged
trangdoan982 merged 3 commits into
roam-setting-overhaulfrom
eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put
Sep 28, 2026
Merged

ENG-2188 Node type settings are spread across seven inner tabs; put them on one page#1420
trangdoan982 merged 3 commits into
roam-setting-overhaulfrom
eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put

Conversation

@trangdoan982

@trangdoan982 trangdoan982 commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Reviewer brief

  • Result: a node type's settings are one vertical page instead of seven inner tabs. Groups, in order: Identity (Index, Description, Tag, Color, Format), Creation (Shortcut, Template), Canvas, Legacy (Specification), Suggestive mode (shown only when sync is on), Attributes. Index and Template are drill-down rows. Each opens its own sub-page with a Grammar › Nodes › <node> › Index breadcrumb.
  • Review focus: NodeConfig.tsx is a re-layout. No setting changes its key, default, or write path. SettingsDrillDownRow renders a row that pushes a segment on the ENG-2186 route. NodeIndexPage and NodeTemplatePage wrap the existing panels for the two sub-pages.
  • Review focus: the Specification row. The toggle now reads Enabled. Its block text stays lowercase enabled through blockKey, so storage is unchanged. The query builder renders only while the toggle is on.
  • Review focus: group order differs from the ticket. The ticket lists Identity / Recognition / Creation / Index. Review dropped Recognition, moved Index and Format into Identity, moved Specification into a Legacy group for settings mid-migration, and moved Attributes last. Format's DEPRECATED — use Specification note went with it, because Specification is now the setting in flux.
  • Follow-up: Suggestive mode stays on the node page. The ticket's Solution suggests moving it to the Admin panel, but it needs its own node selector there, so that move needs its own ticket. The Index row's description is proposed copy and needs sign-off.

Verification

  • tsc --noEmit and eslint report no errors or warnings for apps/roam. The production build completes with 0 errors.
  • Checked in Roam: Grammar › Nodes → node type → Index and Template. Back and the breadcrumb work at each depth. Toggling Enabled shows and hides the query builder. Legacy and Attributes are the last two groups.

Loom video

https://www.loom.com/share/f76e4399d7784cf2a85613fd91301e78

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When: group names and order follow the manager's review instead of the ticket text (see the review focus above). The ticket needs updating to match.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context.

🤖 Generated with Claude Code

Devin Review

@linear-code

linear-code Bot commented Sep 8, 2026

Copy link
Copy Markdown

ENG-2188

@vercel

vercel Bot commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 28, 2026 2:32pm UTC

Request Review

@supabase

supabase Bot commented Sep 8, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

devin-ai-integration[bot]
devin-ai-integration Bot reviewed Sep 8, 2026
View reviewed changes

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Devin Review: 2 flags

Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

@graphite-app

graphite-app Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from 64d614b to d59d299 Compare September 8, 2026 21:22
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from d59d299 to 72e962e Compare September 8, 2026 21:54
@trangdoan982 trangdoan982 mentioned this pull request Sep 9, 2026
Merged
2 tasks done
@trangdoan982
trangdoan982 removed this pull request from stack #1423 September 9, 2026 19:16
@trangdoan982
trangdoan982 added this pull request to stack #1426 September 9, 2026 19:30
mdroidian
mdroidian reviewed Sep 11, 2026
View reviewed changes
Comment thread apps/roam/src/styles/settingsStyles.css Outdated
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from 72e962e to 2a87379 Compare September 17, 2026 17:21
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from e7cc56d to 1d0700c Compare September 17, 2026 19:59
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from 1d0700c to b752e3c Compare September 17, 2026 20:18
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from b752e3c to 0f9a0be Compare September 17, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from 0f9a0be to 43404d1 Compare September 18, 2026 20:39
mdroidian
mdroidian approved these changes Sep 22, 2026
View reviewed changes
Comment thread apps/roam/src/components/settings/components/SettingsDrillDownRow.tsx
Comment thread apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from 43404d1 to bceae82 Compare September 22, 2026 20:54
Base automatically changed from eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitive to roam-setting-overhaul September 28, 2026 14:32
trangdoan982 and others added 3 commits September 28, 2026 10:32
@trangdoan982 @claude
ENG-2188 Node type settings are spread across seven inner tabs; put t…
aef3580 
…hem on one page

A node type's settings become one vertical page: Identity (Index first, then
Description, Tag, Color, Format), Creation, Canvas, Legacy (Specification),
the sync-gated Suggestive mode, and Attributes last. Index and Template are
drill-down rows on the ENG-2186 primitive, each with its own sub-page.

Specification's toggle reads "Enabled" (its lowercase block text stays the
storage key via blockKey) and the query builder renders only while it is on.
Format loses its DEPRECATED note: Specification is the one in migration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@trangdoan982 @claude
ENG-2188 Style the node config page container with Tailwind
d74929c 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@trangdoan982
reduce gap
0c1f856
@trangdoan982
trangdoan982 force-pushed the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch from bceae82 to 0c1f856 Compare September 28, 2026 14:32
@trangdoan982
trangdoan982 merged commit 379c39c into roam-setting-overhaul Sep 28, 2026
15 of 16 checks passed
@trangdoan982
trangdoan982 deleted the eng-2188-node-type-settings-are-spread-across-seven-inner-tabs-put branch September 28, 2026 14:52

This branch was previously deployed

1 inactive deployment
Preview — 0c1f8563 Deployed Sep 28, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments
@mdroidian mdroidian mdroidian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@trangdoan982 @mdroidian