ENG-2183 Build the new setting-item row component by trangdoan982 · Pull Request #1377 · DiscourseGraphs/discourse-graph

trangdoan982 · 2026-08-31T17:51:33Z

Reviewer brief

Result: one shared row renders every panel-based setting as scope badge · label · always-visible description · trailing control. Descriptions leave the hover popover, so their doc links are reachable (ENG-2080).

Review focus.

The seam. SettingTitle was the one helper all five Base*Panels funnelled label and description through, so rewiring it converts ~70 rows with no call-site edits. The risk is that it also changes ~70 rows at once: flag rows now render Switch instead of Checkbox, and text/number/select controls move from below the label to trailing, losing fill.
Why the row uses a raw <label> and flex. Not cosmetic. Measured against live Roam CSS: label.bp3-label .bp3-popover-wrapper is display:block; margin-top:5px at specificity (0,2,1), and .bp3-icon is vertical-align:text-bottom. Inside a Blueprint <Label> the scope badge lands 19.25px out of line; with the raw label plus flex it is 1px. A comment in SettingItemRow.tsx records this — please keep it if you restructure the markup.
Scope is derived, not passed. It comes from the wrapper, since the wrapper already binds the setter that decides who a value is written for. Consequence worth checking: PersonalTextPanel and PersonalNumberPanel accept a setter override, so a caller overriding the setter must also pass scope or the badge will contradict where the value lands. Noted in code.
Tailwind's reach in this app. Utilities do resolve, because Roam itself bundles Tailwind — but on a 14px rem base, and without preflight. So border-b and border set a width that never paints, and this repo's neutral-* theme colours are absent from Roam's build entirely. Both are hand-rolled in settingsStyles.css rather than papered over. This corrects the comment added in ENG-2186 Grammar › Nodes drill-down and settings navigation primitive #1375, which concluded utilities do not apply at all.

Risk or follow-up

No visual verification inside the extension. The dev extension would not load in any tab automation could reach, because Roam needs its directory handle re-granted through a native picker. Alignment, badge fill and row dividers were instead verified by injecting the row's exact markup into live Roam and measuring; the rows as rendered by the extension are unverified. The Checkbox→Switch swap and the loss of fill on NumericInput/HTMLSelect are the parts most worth a human look.
withDocsLink's stopPropagation is deliberately kept. CanvasShortcutSettings still nests a description inside a <label>, so it remains load-bearing.
Two pre-existing issues found, neither introduced nor fixed here: shortcutValue in NodeConfig.tsx is assigned and never used (arrived with ENG-2186 Grammar › Nodes drill-down and settings navigation primitive #1375), and EphemeralBlocksPanel's container uses border border-gray-200, which is inert for the preflight reason above — that box has no visible border today.

Verification

turbo check-types 8/8 workspaces; eslint 0 errors, 515 warnings (unchanged from base); prettier clean; 142/142 roam, 16/16 content-model, 39/39 database unit tests; dev build 0 errors with the new code confirmed present in the bundle.
Measured against live Roam CSS by injecting the row's rendered markup: badge-to-label-text centre offset 1px, versus 19.25px for the Blueprint <Label> structure; badge fill rgb(241,241,241) and 1px solid border painting; row divider 0px on the first row and 1px on subsequent ones.
Not covered: no unit tests. apps/roam's vitest is environment: "node" and includes only src/utils/__tests__, so a render test would need jsdom and RTL added; deferred by agreement. The doc-link fix therefore has no automated guard.
Not verified: the rows as the extension actually renders them (see above).

Loom video

Not recorded.

Scope check

Ran $scope-check against ENG-2183 and the final diff.
Scope beyond Done When:
- Rewiring the five Base*Panels. ENG-2183 excludes migrating call sites, and no call site changes here — the edit is internal to BlockPropSettingPanels.tsx. Included because a row with no consumers cannot be verified at all: this repo has no Storybook. Agreed with @trang while planning.
- Migrating seven bespoke <Label> rows (Personal override, Discourse tool shortcut, Default filters, Page groups, node Color, Specification, Template blocks). Requested by @trang so that every setting row carries a scope badge, which the ticket's Solution asks for but ENG-2187 owned. Not recorded in Linear.
- Deriving scope at the wrapper. This delivers most of ENG-2187's "populate the scope indicator" for the ~70 panel rows, leaving that ticket the remaining bespoke rows. Cheaper here than a second pass over the same files.
- Checkbox → Switch on flag rows, and controls moving to trailing. Both are what the mockups show; neither is stated in Done When.
- Superseding ENG-2186 Grammar › Nodes drill-down and settings navigation primitive #1375's SettingsDrillDownRow. Deleted, and its two callers in NodeConfig.tsx now compose the shared row. Requested by @trang to avoid two drill-down rows landing in the same stack. Its buttonText values were action labels rather than value summaries, so SettingDrillDownSummary's contract documents both uses.
- CanvasShortcutSettings deliberately not migrated. It is a two-column grid where each node contributes two grid cells; converting it would restructure a compact override table into stacked rows. Read as one personal setting with per-node overrides rather than N setting items. Flagged for @trang rather than changed.

Local delegated full review

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

linear-code · 2026-08-31T17:51:37Z

ENG-2183

vercel · 2026-08-31T17:51:40Z

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

Project	Deployment	Actions	Updated
discourse-graph	Ready	Preview	Sep 5, 2026 11:13pm UTC

graphite-app · 2026-08-31T17:51:40Z

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

supabase · 2026-08-31T17:51:41Z

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

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

trangdoan982 · 2026-08-31T17:57:15Z

@@ -1,31 +0,0 @@
-import React from "react";


Deleted, not lost. This row is superseded by SettingItemRow + SettingDrillDownSummary, and its two callers in NodeConfig.tsx (Index, Template) now compose those instead.

Worth knowing while reviewing: its buttonText values were action labels (See all Claim nodes, Edit template) rather than value summaries. Rather than quietly stretch the new prop, SettingDrillDownSummary's summary doc now covers both uses, since a list or a template has no single scalar value to show.

The visible change is the control going from outlined + intent=PRIMARY to minimal with a chevron, plus both rows gaining a nodeType scope badge.

trangdoan982 · 2026-08-31T17:57:16Z

+                    window.clearTimeout(colorWriteTimeoutRef.current);
+                    colorWriteTimeoutRef.current = null;
+                  }
+                  pendingColorRef.current = null;


Conflict-resolution note. This line is #1375's (Flush pending debounced writes when navigating away); this PR restructured the surrounding <Label> into SettingItemRow at the same time, so the two collided here on rebase.

Both are kept deliberately: the row structure is mine, the pendingColorRef reset is #1375's. If you are diffing against that branch, this is the one hunk where the resolution was manual rather than automatic.

The 7 export controls were used by exactly one surface but lived in Settings, so changing an option meant leaving the Export flow, opening Settings, and coming back. Move them into a collapsed "Export options" disclosure at the bottom of the Export dialog's Export tab, and remove the Advanced > Export tab that ENG-2186 parked in Settings for this change to take out. The controls, their storage keys, and the read path in getExportSettings are unchanged, so saved values persist and export behaviour is untouched. The advancedExport tab id goes with it, and the saved "discourse-graph-export" deep link now resolves to the default tab instead of a tab that no longer renders. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The image showed the export options inside the Settings dialog, with the vertical tab list and the pre-taxonomy tab names. Both the Export tab and that chrome are gone, so the screenshot contradicted the prose above it. Removed rather than replaced: the surrounding text already names where the options live, and no fresh capture of the Export dialog is available yet. The frontmatter close-up further down is a crop of the control itself, so it stays accurate and is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…eads The number and select panels in Export options defer their block-prop write behind a short timer, so an option edited a moment before clicking Export was read at its previous value. Awaited rather than fired: the commit only starts the Roam block update, so a synchronous flush would not have been enough on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Review feedback on the Export dialog: the options toggle sits on the left of one row with "Exporting N results" and the discourse-context checkbox on the right, instead of taking a row of its own. The option panels are also seeded from a fresh settings read each time the section opens. Collapse unmounts them while closed, so reading once at dialog mount meant a long-lived dialog could show values that had since changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Review feedback: the checkbox sits right-aligned on the line above; the line below is the Export options toggle on the left and the result count on the right. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Add one shared settings row — scope indicator, label, always-visible description, control — and route every panel-based row through it. Descriptions move out of the hover popover that dismissed before its doc links could be clicked (ENG-2080). The fix is structural: the description renders as a sibling of the label, never a descendant, so a link inside it can no longer toggle the row's control. The rewire happens at SettingTitle, the one helper all five Base*Panels funnelled label and description through, so ~70 call sites are unchanged. Flag rows swap Checkbox for Switch to match the design. Scope is derived from the wrapper rather than passed per call site, because the wrapper already binds the setter that decides who a value is written for. Rows that cannot derive it pass `scope` explicitly. Layout is Tailwind, which resolves because Roam itself bundles Tailwind. Anything depending on Tailwind's preflight is hand-rolled instead: `border-b` and `border` set only a width, and `* { border-style: solid }` is absent, so those borders never paint. This repo's own `neutral-*` theme colours are likewise absent from Roam's build and are mirrored as CSS variables. Supersedes ENG-2186's SettingsDrillDownRow, whose two callers now compose the shared row with a trailing drill-down summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A node type's settings are written to that node type's own page block props, so the whole graph sees them — the agreed badge model has two values, `just you` and `everyone`, and a per-node scope was never one of them. Drops the invented `nodeType` scope and its icon; the nine affected rows now badge as `global`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…verflow Review feedback on the row component: - Separators ran between sibling rows only, so they stopped at every group boundary. Each row now owns its bottom edge. - BaseNumberPanel put `w-24` on the NumericInput, but Blueprint applies that class to the outer ControlGroup while the input keeps its intrinsic width; the box shrank and the input spilled past it (Advanced > Queries, and the same control in Export options). The width now sits on a wrapper and the input fills it. - The Export dialog is too narrow for the trailing layout, so rows take a `compact` presentation there: no scope badge, control below the label, tighter vertical spacing — the shape the options had before this component. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The group heading's 2px rule already frames the group; a 1px hairline after its last row put two rules at one boundary. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Hairlines under rows were inconsistent — present on flat rows, dropped after a group's last row, absent on rows not yet on SettingItemRow — so they told the eye nothing reliable. A line now appears in exactly one place, under a section heading. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

graphite-app · 2026-09-05T23:16:10Z

+        <SettingItemRow
+          label="Specification"
+          description={withDocsLink(
+            `The conditions specified to identify a ${node.text} node.`,
+            ROAM_DOCS.grammarNodes,
+          )}
+          scope="global"
+          controlPlacement="below"
+          control={
+            <DiscourseNodeSpecification
+              node={node}
+              parentUid={specificationUid}
+              parentSetEnabled={(isSpecificationEnabled) => {
+                validate({
+                  tag: tagValue,
+                  format: formatValue,
+                  isSpecificationEnabled,
+                });
+              }}
+            />
+          }
+        />
+        <SettingItemRow
+          label="Index"
+          description={`The saved list of all ${node.text} pages \u2014 which pages appear and which columns show.`}
+          scope="global"
+          control={
+            <SettingDrillDownSummary
+              summary={`See all ${node.text} nodes`}
+              onClick={() => nav.push(nodeConfigSegmentIds.index)}
+            />
+          }
+        />


Duplicate settings detected in the Identity section. Both "Specification" (lines 355-377) and "Index" (lines 378-387) are duplicated - they already appear earlier in the same Identity section (Index at lines 296-303) and Specification also appears in the Legacy section (lines 439-460).

This will cause:

Confusing UI with duplicate identical settings

Potential data inconsistency if users modify different copies

Wasted screen space

// Remove these duplicate entries (lines 355-387) // Keep only one Index in Identity section // Keep only one Specification (either in Identity OR Legacy, not both)

Spotted by Graphite

Is this helpful? React 👍 or 👎 to let us know.

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 8f93eda to 24ac1dc Compare August 31, 2026 17:54

vercel Bot temporarily deployed to Preview August 31, 2026 17:54 Inactive

devin-ai-integration Bot reviewed Aug 31, 2026

View reviewed changes

trangdoan982 commented Aug 31, 2026

View reviewed changes

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 24ac1dc to 173e148 Compare August 31, 2026 18:47

vercel Bot temporarily deployed to Preview August 31, 2026 18:47 Inactive

vercel Bot temporarily deployed to Preview August 31, 2026 19:03 Inactive

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 73ad70b to ba602f6 Compare September 3, 2026 20:30

vercel Bot deployed to Preview September 3, 2026 20:32 View deployment

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from ba602f6 to efae0ef Compare September 4, 2026 21:57

vercel Bot deployed to Preview September 4, 2026 21:58 View deployment

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from efae0ef to 2fae9dd Compare September 4, 2026 22:46

vercel Bot deployed to Preview September 4, 2026 22:46 View deployment

trangdoan982 and others added 4 commits September 5, 2026 18:53

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 2fae9dd to 7b3b18d Compare September 5, 2026 22:55

vercel Bot deployed to Preview September 5, 2026 22:56 View deployment

vercel Bot temporarily deployed to Preview September 5, 2026 22:59 Inactive

trangdoan982 and others added 2 commits September 5, 2026 19:12

trangdoan982 and others added 4 commits September 5, 2026 19:12

ENG-2183 No hairline after a group's last row

1e81108

The group heading's 2px rule already frames the group; a 1px hairline after its last row put two rules at one boundary. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 424f4f4 to f421048 Compare September 5, 2026 23:12

vercel Bot deployed to Preview September 5, 2026 23:13 View deployment

graphite-app Bot reviewed Sep 5, 2026

View reviewed changes

Navigation Menu

Uh oh!

ENG-2183 Build the new setting-item row component - #1377

ENG-2183 Build the new setting-item row component#1377
trangdoan982 wants to merge 10 commits into
eng-2186-grammar-nodes-drill-down-and-settings-navigation-primitivefrom
eng-2183-build-the-new-setting-item-row-component

trangdoan982 commented Aug 31, 2026 •

edited by devin-ai-integration Bot

Loading

Uh oh!

linear-code Bot commented Aug 31, 2026

Uh oh!

vercel Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

graphite-app Bot commented Aug 31, 2026

Uh oh!

supabase Bot commented Aug 31, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

trangdoan982 Aug 31, 2026

Uh oh!

trangdoan982 Aug 31, 2026

Uh oh!

graphite-app Bot Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

trangdoan982 commented Aug 31, 2026 • edited by devin-ai-integration Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer brief

Verification

Loom video

Scope check

Local delegated full review

Uh oh!

linear-code Bot commented Aug 31, 2026

Uh oh!

vercel Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app Bot commented Aug 31, 2026

PR size/scope check

Uh oh!

supabase Bot commented Aug 31, 2026

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

trangdoan982 Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

trangdoan982 Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

trangdoan982 commented Aug 31, 2026 •

edited by devin-ai-integration Bot

Loading

vercel Bot commented Aug 31, 2026 •

edited

Loading