Skip to content

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

Open
trangdoan982 wants to merge 6 commits into
eng-2183-build-the-new-setting-item-row-componentfrom
eng-2213-settings-catalog-address-every-setting-by-key-and-location
Open

ENG-2213 Settings catalog: address every setting by key and location#1379
trangdoan982 wants to merge 6 commits into
eng-2183-build-the-new-setting-item-row-componentfrom
eng-2213-settings-catalog-address-every-setting-by-key-and-location

Conversation

@trangdoan982

Copy link
Copy Markdown
Member

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

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

ENG-2213

@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated
discourse-graph Ready Ready Preview Sep 8, 2026 1:03am UTC

Request Review

@graphite-app

graphite-app Bot commented Aug 31, 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

@supabase

supabase Bot commented Aug 31, 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 ↗︎.

@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from bc865f3 to 81f750e Compare August 31, 2026 23:08
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 73ad70b to ba602f6 Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 81f750e to fd2b364 Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from fd2b364 to 8f6c3bd Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from ba602f6 to efae0ef Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from efae0ef to 2fae9dd Compare September 4, 2026 22:46
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 8f6c3bd to 0a82e0e Compare September 4, 2026 22:46
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 0a82e0e to 2867339 Compare September 5, 2026 22:55
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 2fae9dd to 7b3b18d Compare September 5, 2026 22:55
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 2867339 to 369c7f8 Compare September 5, 2026 22:59
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 424f4f4 to f421048 Compare September 5, 2026 23:12
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 369c7f8 to c3c5884 Compare September 5, 2026 23:12
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from f421048 to 6e2b2a9 Compare September 7, 2026 22:23
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from c3c5884 to 4c1e173 Compare September 7, 2026 22:23
trangdoan982 and others added 3 commits September 7, 2026 21:01
Records what every setting row is called and where it lives, so a later change
can find a setting without knowing which tab it is on.

Nothing in the codebase held this. `zodSchema.ts` is storage shape — no labels,
no descriptions, no locations — and `settingKeys.ts` holds block keys that
diverge from what a row renders: `GLOBAL_KEYS.trigger` renders as "Graph-wide
default", `PERSONAL_KEYS.personalNodeMenuTrigger` as "Personal override". Nor
can it be collected at runtime: `Tabs` uses `renderActiveTabPanelOnly`, so
inactive panels are out of the DOM and rows cannot self-register. So the label,
group, description, doc link and location are authored, one entry per row.

Everything else is derived rather than restated. Entries are keyed by
`GROUP.member`, addresses are built by resolving that id against the
`settingKeys.ts` constants, and the catalog is declared
`satisfies Record<RowKeyId, AuthoredSetting>`. A key added to `settingKeys.ts`
is then a compile error until it is either given an entry or listed in
`NonRowKeyId` with a reason, so a new setting cannot silently go missing. The
remaining gap is the reverse direction: nothing checks that a row rendering
`settingKeys` was classified as a row rather than exempted.

An entry's `path` is the only field a relocation has to touch. Node-type
settings give `path` as a function of the node uid, so one entry expands to one
result per node type and adding a node type never edits this file.

Six rows rendered without an anchor, so they had no stable address; each now
passes the key constants already in scope at its call site. `SETTINGS_TAB_META`
becomes the single source for tab labels, because a renamed tab would otherwise
drift away from the locations recorded here.

`SettingItemRow` falls back to the catalog for a description when the prop is
omitted, so ENG-2187 can migrate call sites by deleting props rather than by
copying text.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The key was exempted as "persisted UI state and canvas-written values", but
CanvasShortcutSettings renders it as a visible editor under Features > Canvas,
so a real setting was missing from the index: searching "canvas shortcut"
only found the discourse tool shortcut.

Authored as one row rather than one per node type, because the overrides share
a single stored value; the anchor goes on the grid so a jump lands on it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The catalog is what makes search navigable, and its invariants fail quietly:
a duplicate id breaks React keys, a nested setting that drops its parent
segment collides with a same-named top-level key, and an entry pointing at a
tab that no longer exists sends the user nowhere.

Also pins the two rules the export move introduced: settings living outside
the Settings dialog stay out of the index, and stay authored here anyway so
their rows keep reading one description.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982 and others added 3 commits September 7, 2026 21:01
…sTabId

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Index and Format now sit under Identity and Specification under Legacy, so
the breadcrumbs search shows match the page. Format's description loses the
DEPRECATED note along with the row.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@trangdoan982
trangdoan982 force-pushed the eng-2183-build-the-new-setting-item-row-component branch from 6e2b2a9 to a73c878 Compare September 8, 2026 01:02
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 4c1e173 to 1ba0ab8 Compare September 8, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant