ENG-2213 Settings catalog: address every setting by key and location

[trangdoan982]

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

Reviewer brief

• Result: Every setting row now has a stable address and a recorded location. Nothing consumes this yet — ENG-2184 (ENG-2184 Add search to the settings panel #1378, stacked on top) is the first reader. It is split out because it is ~590 lines that are mostly declarative data, and it dominated that diff.

• Review focus — why this is authored rather than derived. Three constraints, worth checking before reading settingsCatalog.ts as a pile of literals:

  • Tabs uses renderActiveTabPanelOnly, so inactive panels are out of the DOM and rows cannot self-register.
  • zodSchema.ts is storage shape — no labels, no descriptions, no locations. Location is a pure UI fact with no runtime source.
  • A key's string is not its label: GLOBAL_KEYS.trigger renders as "Graph-wide default", PERSONAL_KEYS.personalNodeMenuTrigger as "Personal override", and DISCOURSE_NODE_KEYS values are "canvasSettings", "key-image".

• Review focus — the key list is not restated, and coverage is a type error. Entries are keyed by GROUP.member; addresses are built by resolving that id against the settingKeys.ts constants; the object is declared satisfies Record<RowKeyId, AuthoredSetting>, where RowKeyId is every key in settingKeys.ts minus an explicit NonRowKeyId list (22 containers, admin-only flags and nested sub-controls, each with its reason). Adding a key to settingKeys.ts fails to compile until it is entered or exempted.

  Verified by mutation: deleting GLOBAL_KEYS.canvasPageFormat from the catalog, and adding a new key to settingKeys.ts, each fail tsc with TS1360.

• Risk or follow-up:

  • Coverage is enforced in one direction only. Nothing checks that a row actually rendering settingKeys was classified as a row rather than exempted, so exempting a key that is really a visible row would drop it from search silently. The NonRowKeyId list is the part most worth a careful read.
  • No tests. The catalog is reviewed as data, against the rows it claims to cover.
  • Three rows in LeftSidebarPersonalSettings.tsx pass settingKeys={[]} — per-section instances with no stable address. Pre-existing; exempted as LEFT_SIDEBAR_KEYS.children and needs an addressing scheme rather than a one-liner.
  • SettingItemRow's catalog description fallback has no call site yet. It exists so ENG-2187 can migrate rows by deleting props instead of copying text.

Verification

• pnpm ci:validate (check-types + 142 unit tests) and eslint on the changed files — 0 errors, no new warnings on added lines.
• Catalog output checked against the anchors the rows actually stamp: Export/Link type, Query/Hide query metadata, canvasSettings/color, suggestiveRules/embeddingRef, template. These have to match settingAnchor exactly or ENG-2184's jump lands nowhere.
• Node-type expansion checked against a two-node-type stub: one authored entry yields Grammar › Nodes › Claim › Recognition and Grammar › Nodes › Evidence › Recognition, with unique ids.

Scope check

• Ran $scope-check against the ENG ticket and final diff.
• Scope beyond Done When:
  • Descriptions stored on the entry, plus the SettingItemRow fallback. ENG-2184 originally scoped description search out; it was requested during implementation. Storing the text once and letting the row read it back is what stops the row and search drifting apart.
  • PreferencesGeneral raw string → PERSONAL_KEYS.reifiedRelationTriples, for consistency with every other row's addressing.
  • SETTINGS_TAB_META, with Settings.tsx titling its tabs from it. Locations are recorded per tab, so a renamed tab would otherwise drift away from the catalog.

Local delegated full review

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

[linear-code]

ENG-2213

[vercel]

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

1 Skipped Deployment

Project	Deployment	Actions	Updated
discourse-graph	Skipped		Sep 18, 2026 9:08pm UTC

[graphite-app]

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]

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 ↗︎.

[mdroidian]

Non-blocking: the catalog duplicates labels and descriptions already defined in the settings components. For example, “Graph-wide default” and its description appear in both PreferencesGeneral.tsx and settingsCatalog.ts.

Can we explicitly cover consolidating these in ENG-2187 (or elsewhere), so the controls and search read the same labels and descriptions? The current fallback still lets the two copies drift apart.

For example, define the shared metadata once:

export const NODE_TRIGGER_SETTING = {
  settingKeys: [GLOBAL_KEYS.trigger],
  label: "Graph-wide default",
  description: "The trigger to create the node menu.",
  docsLink: ROAM_DOCS.creatingNodes,
};

The control reads it:

<GlobalTextPanel
  title={NODE_TRIGGER_SETTING.label}
  description={withDocsLink(
    NODE_TRIGGER_SETTING.description,
    NODE_TRIGGER_SETTING.docsLink,
  )}
  settingKeys={NODE_TRIGGER_SETTING.settingKeys}
  initialValue={globalSettings[GLOBAL_KEYS.trigger]}
  {...legacyBlocks.trigger}
/>

Search uses the same definition with its navigation metadata:

{
  ...NODE_TRIGGER_SETTING,
  group: "Node trigger",
  keywords: ["trigger", "node menu", "global"],
  path: rootPath(SETTINGS_TAB_IDS.preferencesGeneral),
}

This is illustrative; the existing catalog could hold the shared definitions rather than introducing another metadata source.

I also agree with the Codex request for focused unit tests before merge, though I wouldn’t classify missing coverage alone as P1. Please cover nested addresses, per-node expansion, unique IDs, availability filtering, and exclusion of settings without a path. These are runtime behaviors that type checking won’t verify; there’s no need to snapshot the entire catalog.

[mdroidian]

pre-ENG-2189 isn't very helpful on it's own. What additional context should we add here?

[mdroidian]

grid-cols-[auto_auto] is not included in roam's tailwind.

This is covered in the roam-global-tailwind skill. Could you investigate if claude is using that skill? And if not, ask what we can change so that it does use that skill