feat(FR-3558): generate a build-time search index of pages, tabs, and setting items - #8811
feat(FR-3558): generate a build-time search index of pages, tabs, and setting items#8811nowgnuesLee wants to merge 5 commits into
Conversation
Coverage Report for root-coverage
File CoverageNo changed files found. |
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
dfba7ce to
5501c3e
Compare
8f78dfa to
5bf48be
Compare
There was a problem hiding this comment.
Pull request overview
Adds a source-derived search index for future global palette functionality.
Changes:
- Extracts route, tab, setting, and i18n metadata.
- Adds deterministic generation tests and drift verification.
- Regenerates during development and documents generated-file handling.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
react/scripts/build-search-index.mjs |
Implements index extraction and serialization. |
react/src/generated/searchIndex.json |
Stores the generated index artifact. |
searchIndex.types.ts |
Defines the index’s TypeScript interface. |
searchIndex.test.ts |
Tests coverage, ownership, and determinism. |
scripts/verify.sh |
Adds the index drift gate. |
scripts/dev.mjs |
Regenerates the index at startup. |
react/package.json |
Adds the workspace generator command. |
package.json |
Exposes the root generator command. |
.gitattributes |
Marks the index as generated. |
.prettierignore |
Excludes generated JSON from formatting. |
search-index-conflicts.md |
Documents conflict resolution. |
review-ignored-paths.md |
Excludes generated index content from review. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
5bf48be to
6eab888
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
react/scripts/build-search-index.mjs:370
- Flattening both sides of runtime conditionals makes gated search surfaces unconditional in the generated index. For example,
EnvironmentPage.tsx:68-71exposesregistryonly to superadmins,StatisticsPage.tsx:39-42gatesuser-session-historyon backend support, andUserSettingsPage.tsx:217-258gates setting items on configuration; the emitted tab/setting shapes contain no corresponding guard. The palette can therefore surface hits that render no tab or setting for the current user. Preserve availability metadata for query-time filtering, or otherwise exclude/explicitly register guarded surfaces instead of flattening them unconditionally.
if (ts.isBinaryExpression(x)) {
if (x.operatorToken.kind === ts.SyntaxKind.AmpersandAmpersandToken)
push(x.right);
return;
}
if (ts.isConditionalExpression(x)) {
push(x.whenTrue);
push(x.whenFalse);
agatha197
left a comment
There was a problem hiding this comment.
please resolve conflicts
… setting items The global search palette cannot search other pages' DOM — every route is a React.lazy chunk that is not mounted when the palette opens. So the index is derived from source at build time and stores i18n KEYS, not strings, which makes all 20 locales and language switching free. `react/scripts/build-search-index.mjs` (zero new deps: the react workspace's `typescript` + `vite.transformWithEsbuild`) parses `routes.tsx` with the TS AST, walks each entry's value-import graph at unlimited depth, and collects `t()` / `i18nKey=` keys plus structured tabs (nuqs enum parsers bound to `?tab=` and the tab-like params) and `SettingList` items with their description keys. Edges into other route entries, `MainLayout/**` and `DefaultProviders` are cut, BUI is excluded, plain keys present on >= 10 entries are dropped as chrome, and dynamic template keys are recorded but never emitted so the index holds no key `t()` cannot resolve. Output: 34 entries, 8 `?tab=` pages / 24 keys, 4 setting pages / 54 items, 1,701 body keys, 141 kB / 18 kB gzip, byte-identical across runs. The index lives at `react/src/generated/searchIndex.json`, NOT under `__generated__` as the plan said: relay-compiler deletes every file it does not own in its artifact directory. `.gitattributes`, `.prettierignore` and the review-ignored-paths rule are extended to the new directory instead. `scripts/verify.sh` gains `check_search_index_drift` next to the Relay gate, and `scripts/dev.mjs` refreshes the index once at startup (non-fatal on failure). Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KBDYpkXSSpvZJyLsyCfEdc
…harden the extractor
BLOCKER — the >=10-entry noise filter deleted page-owned vocabulary. The walk
is transitive with no depth limit, so `session.launcher.SharedMemory` reaches 19
entries exactly as `button.Cancel` does, and a count-only filter dropped both.
`session.launcher.{SharedMemory,ClusterMode,Memory}` were absent from the session
launcher, `data.{Foldername,UsageMode,Host}` from Data, `session.SessionName` from
everywhere. Placement is now decided by OWNERSHIP, using the depth `keyMap`
already recorded: an entry owns a key when it declares it within OWNER_DEPTH (4)
import hops of the route component. A key owned by >= NOISE_THRESHOLD entries is
chrome and is dropped everywhere; otherwise a shared key survives only on its
owners. All seven measured keys are back on their pages; `button.*`,
`time.*`, `errorBoundary.*` and `general.ExtendLoginSession` are gone from all.
1701 -> 1940 distinct body keys, 229 count-dropped keys -> 39 chrome keys.
BLOCKER — `hasEntityParam` treated the optional `:id?` in `chat/:id?` as an
entity param, so Chat — a first-class sidebar item — had no entry. Optional
params no longer disqualify a route; it is indexed at the path with the optional
segment omitted (`/project/:projectName/chat`). 34 -> 35 entries.
Also:
- Tab recognizer accepts `value` as a `key` alias, so radio-strip tabs get their
labels: 26/53 -> 41/53 tabs labelled. TAB_OVERRIDES is still required — the
Data page's `statusCategory` is `parseAsString`, so nothing is detectable
statically — and is now asserted against the indexed route set.
- Startup assertions: every configured file path (CUT_*, SHIM_COMPONENT_FILES,
ROUTE_CHROME_COMPONENTS, EXTERNAL_PREFIXES, routes.tsx, en.json) must exist and
every TAB_OVERRIDES key must match an indexed route, or the build throws.
These failed silently on rename before.
- Tests: the route-coverage test compared the extractor to itself; it now checks
VALID_MENU_KEYS against the index with a documented exclusion list
(summary/job aliases, plugin-rendered pipeline) and asserts the exclusions are
not stale. The "drops keys on every page" test was structurally always true;
replaced with direct assertions on the ownership rule.
- Dropped the dead `--check` mode and the `search-index:check` script: verify.sh's
rebuild-then-`git status` gate is the real check, mirroring Relay.
- Dropped the `component` file-path array from the emitted JSON (now --verbose
logging only) and the unused `fileCount`; removed `revisionTab` from
TAB_LIKE_PARAMS (only reachable from a skipped parametrised route).
- `missingFromEnJson` no longer warns on `t('key', 'Fallback')`, which renders;
4 warnings -> 1, and that one is a genuine miss.
- `objectElements` only takes the right operand of `&&`, not of every binary
operator; `dedupeSettings` keys on key+group so a title can repeat per group.
- New rule `.claude/rules/search-index-conflicts.md`: regenerate on conflict,
never hand-merge.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01KBDYpkXSSpvZJyLsyCfEdc
- normalize `rel()` to POSIX separators so the CONFIG path comparisons (cuts, shims, chrome, external prefixes) and emitted IDs hold on Windows - run `search-index` in `build:only` so a production build regenerates the index from the source being built, matching the Relay treatment - drop a diagnostic console.log from searchIndex.test.ts; the values are asserted immediately below Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CsM4qV5xYywwVzbYFPP6gS
Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CsM4qV5xYywwVzbYFPP6gS
…deleted chrome entry main moved the admin session page's `statusCategoryValues` into `helper/sessionStatusBuckets.ts`, and `resolveArrayArg` only looked for a same-file declaration, so `/admin/session` silently lost both status tabs from the index. Follow a named import into its module (two hops, no barrels). main also deleted `FlexActivityIndicator.tsx` (FR-3662), which the CONFIG-integrity assert correctly refused; remove it from `ROUTE_CHROME_COMPONENTS`. Index regenerated. Co-Authored-By: Claude Fable 5.1 <[email protected]>
6eab888 to
23a34ed
Compare
agatha197
left a comment
There was a problem hiding this comment.
Ran the extractor, the vitest file, and a byte-identical rebuild locally: all clean. One latent mismatch in the extractor.
| if (ts.isArrayLiteralExpression(n)) return n; | ||
| if (ts.isCallExpression(n)) { | ||
| const arg = n.arguments[0]; | ||
| if (arg) { |
There was a problem hiding this comment.
The doc comment says [...].filter(...) is unwrapped, but a call with any argument always descends into arguments[0] — for .filter(fn) that is the callback, so the n.expression.expression branch is never reached and the array comes back null (the whole tab/setting list is silently dropped). Check PropertyAccessExpression before the argument, or drop that case from the comment.
Resolves #8807 (FR-3558)
What
react/scripts/build-search-index.mjs(pnpm run search-index) walksroutes.tsx, type-strips each module withvite.transformWithEsbuild, and follows value imports to collect thet('…')keys reachable from every route entry.react/src/generated/searchIndex.json— 34 entries,?tab=deep links for 8 pages / 24 tabs, 4 setting pages / 54 items, 1,701 body keys after dropping keys shared by ≥10 entries (140 kB raw, 18.5 kB gzip).scripts/verify.shgains a "Search index" drift gate next to the Relay one;scripts/dev.mjsruns the build once on start..gitattributes,.prettierignore,.claude/rules/review-ignored-paths.md).Why
The app is an SPA whose other routes are
React.lazychunks — they are not mounted when the palette opens, so their text does not exist in the DOM to search. The index is therefore derived from source at build time, and it stores i18n keys, not strings, so all 20 locales work and switching language needs no re-index.Verification
bash scripts/verify.sh→=== ALL PASS ===searchIndex.test.ts)Review fixes
Applied in
fix(FR-3558): keep page-owned keys, index optional-param routes, and harden the extractor(8f78dfa).Blockers
session.launcher.SharedMemoryreaches 19 entries exactly asbutton.Canceldoes — a count-only filter dropped both. Measured before the fix:session.launcher.{SharedMemory,ClusterMode,Memory}absent from the session launcher,data.{Foldername,UsageMode,Host}absent from Data,session.SessionNameabsent everywhere. Placement now uses the depth already recorded inkeyMap: an entry owns a key when it declares it withinOWNER_DEPTH(4) import hops of the route component. A key owned by ≥NOISE_THRESHOLDentries is chrome and dropped everywhere; any other shared key survives only on its owners; a key owned by nobody survives where it is shallowest. All seven measured keys are back on their pages, andbutton.*,time.*,errorBoundary.*,general.ExtendLoginSessionare gone from all entries.chat/:id?was skipped as parametrised, so Chat — a first-class sidebar item — had no entry. Optional (?-suffixed) params no longer disqualify a route; it is indexed at the path with the optional segment omitted (/project/:projectName/chat).Should-fix
valueas akeyalias, so radio-strip tabs ({ label, value }) get their labels: 26/53 → 41/53 tabs labelled.TAB_OVERRIDESwas re-checked and kept — the Data page'sstatusCategoryisparseAsStringwith labels built by_.map, so nothing is statically detectable (verified: removing the override leaves the page with 0 tabs). It is now guarded by an assertion instead.CUT_PATH_*,EXTERNAL_PREFIXES,SHIM_COMPONENT_FILES,ROUTE_CHROME_COMPONENTS,routes.tsx,en.json) must exist, and everyTAB_OVERRIDESkey must match an indexed route — otherwise the build throws with the stale entry named. All of these previously failed silently on rename.VALID_MENU_KEYSagainst the index, with a documented exclusion list (summary/jobaliases, plugin-renderedpipeline) that is itself asserted not to be stale. "Drops keys on every page" was structurally always true; it is now two tests that assert the ownership rule directly — named page keys present on their pages, named chrome keys absent from all.--checkwas dead (never called, and it ignored--out). Deleted, along withdiffSummaryand the unusedsearch-index:checkscript —verify.sh's rebuild-then-git statusgate is the real check, mirroring Relay.Nits
componentfile-path array is no longer emitted; it is--verbosereport output only. (10) DeadfileCountremoved. (11)revisionTabremoved fromTAB_LIKE_PARAMS— only reachable from a skipped parametrised route.missingFromEnJsonno longer warns ont('key', 'Fallback'), which renders the fallback: 4 warnings → 1, and that one (storageHost.permission.SelectionLimitExceeded, called ast(key, { options })) is a genuine miss.objectElementstakes the right operand only for&&BinaryExpressions, not for??/||/+. (16)dedupeSettingskeys on key+group, so the same title in two groups no longer collapses.Measured effect
/project/:projectName/chatadded)The artifact grows because page-owned vocabulary that was being deleted is now retained; dropping the
componentarray offsets part of it.Deliberately not applied
dev.mjsrewriting the committed index on every dev-server start. Accepted as-is: the build is ~0.7 s, never blocks startup, and a developer who changes a route wants the palette to match without a manual step. The risk it raises is a merge conflict on the regenerated artifact, which is addressed instead by a new.claude/rules/search-index-conflicts.md— on conflict, take either side and re-runpnpm run search-index; never hand-merge.searchIndex.test.ts), but it is derived by walking the filesystem, so it is only reproducible across machines to the extent that the checkout and the resolvedtypescript/viteversions match — both are pinned by the lockfile, andverify.shre-derives it in CI, so a machine-specific build would fail the gate rather than ship.