Skip to content

fix(platform): preserve search privacy and keyboard focus - #4214

Open
yannickmonney wants to merge 1 commit into
mainfrom
fix/scope-global-search-history
Open

yannickmonney wants to merge 1 commit into
mainfrom
fix/scope-global-search-history

Conversation

@yannickmonney

@yannickmonney yannickmonney commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Global and chats-only search history belongs to the organization, signed-in account and palette scope. Collision-safe JSON tuples in a new v2 namespace isolate both the query and selected title. Unresolved identities disable persistence, and ownerless v1 history is ignored without migrating or deleting its bytes. Older builds can still read v1 data after rollback.

An ownership change now resets query, debounce, results and recents inside the shared SearchCommand body. The dialog and its focus scope stay mounted, retaining the original opener. The persistent sidebar search button no longer steals focus behind the open palette: typing and Tab remain inside it, and Escape closes it and returns to the opener. Scope changes within the same owner preserve the query. Exiting animations immediately deactivate the source and pagination. The shared resetKey API documents this ownership boundary; no visible copy or locale key changes.

Rebased onto main e31702b9849afaf653150d4c53a0f93d3b10cb8d; source b2da52e1e1efdc6f999cc39eda349b8ea5047d98. The original three-file privacy commit replays with range equality. This follow-up closes the historical independent review's focus-only finding.

Verification:

  • The new persistent-opener regression fails for organization, account and sign-out changes against the unchanged PR code (11 controls pass / 3 focus failures).
  • Final source: 27 platform tests across six files, 173 shared search/docs controls across twelve files and 6 real-Chromium focus/debounce/close cases pass (206 distinct selected). A different agent independently reran all 6 Chromium cases and accepted the exact source.
  • T3 collaborative-browser observation of the real shared dialog and AppShell with synthetic identities/source confirms cleared state, working typing, trapped Tab, and Escape returning to the surviving opener. This is focus re-verification; the earlier author review already established the complete tenant/account privacy flows in a real local stack.
  • Full platform and shared UI typechecks, configured scoped type-aware lint, format, manual gate, scoped SAST and conflict/whitespace checks pass. React Doctor reports three existing non-component exports, no new error.

Limits: the full local check did not pass: two unchanged marketing-image encoding tests exceeded their existing 5-second limit, cancelling remaining tasks (40/52 successful). The unchanged isolated five-case encoding suite passes at the original limits. Full Knip retains the same seven unused exports as frozen main, tracked by the separate shared CI repair. No new full-stack sign-in/sign-out, phone or screen-reader proof is claimed. All seven native required checks and the merge queue remain required.

Closes #3613

Current-main rebase

Replayed the previously accepted source b2da52e1e1efdc6f999cc39eda349b8ea5047d98 onto main d1373d84cd56972501403f62145ec52e6f65d44a, including the merged shared CI repair in #4625. The accepted feature payload and all current-main changes are preserved in one atomic commit. Configured commit and conflict checks pass; earlier behavioral proof remains recorded above. All seven native required checks and full merge-group validation remain required for this new source.

Maintenance replay: preserves the accepted feature payload on current main 7d178ca. Includes the merged #4649 Knip cleanup and the exact independently accepted one-line shared CLI inventory repair from #4654 (252f0df), which is still pending native merge on main. The fixed suite inventory keeps its discovery and source/compiled phase guards. Existing feature proof is retained; no fresh full-feature/full-workspace test or hosted-green claim. Native required checks remain mandatory.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Independent review verdict: request changes (blocking evidence only)

Reviewed exact head ff0a7f333cfb6f6cbf444be2e933b1e6130f4766 on fix/scope-global-search-history. I found no source-level privacy defect in the reviewed diff:

  • services/platform/app/components/layout/app-sidebar/sidebar-search-command.tsx:164-171 derives the v2 key from a JSON tuple of organization, authenticated user, and scope. JSON encoding prevents delimiter-based collisions, including IDs containing separators.
  • :303 remounts the command on organization/account identity changes. That resets query, debounce/controller state, result titles, and recents; an unresolved account uses no persistence key.
  • The v1 keys are ignored rather than migrated or deleted. That is rollback-compatible and the current build cannot read those ownerless histories. It does leave private v1 data on the device for an older build to read, so a future explicit retention/cleanup decision should address it; I do not treat that as a defect in this compatibility-preserving change.
  • The added jsdom cases cover both scopes, organization/account/sign-out isolation, legacy-key ignoring, same-identity scope separation, and live input/result reset. Existing controls and EN/DE/FR scope labels remain present.

Blocking follow-up

The requested real-UI proof of an organization switch plus sign-out/sign-in was not run in this light slice (no browser, stack, E2E, or container permit). Run that heavy slice before landing and verify that a pending debounce cannot paint or persist the prior identity's query/title. This is an evidence blocker, not a new code finding.

Evidence

  • git diff --check passed for the exact PR diff.
  • JSON tuple collision probe passed for separator-containing IDs.
  • Targeted Vitest could not start because this checkout has no installed workspace dependencies; the attempted resolver reported missing @storybook/addon-vitest/vitest-plugin, @tale/ui/vite/yaml, @vitest/browser-playwright, and vitest/config. I did not install dependencies or broaden the light slice.
  • CI was observed queued/pending at review time; no rerun or cancellation requested.

Please re-request an independent native review after the heavy real-UI evidence is attached. No merge performed. The formal review endpoint also refused request changes because the available GitHub credential is the PR author.

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Real-UI evidence for PR #4214 at ff0a7f333cfb6f6cbf444be2e933b1e6130f4766: changes requested. The privacy fix is proven; one keyboard-focus regression remains.

TALE-540 · agent #3 (4aec6e61), run 49ffc3d8-b451-4d86-b1e0-f6fbd8297c91 · fleet-dispatch 89ba1d5c, heavy permit H1 · author Codex #13 (7155f7ae, run e37b1b60), a distinct agent. This answers TALE-532's evidence-only request (issuecomment-5975862559).

Source and control

  • Head: ff0a7f333, unchanged before posting.
  • Control: main 9b7463b70, the PR's merge base. No file in the palette, search, focus, session or dashboard-route paths changed between it and current main 2aa291e9d, and git merge-tree of the head with 2aa291e9d is clean.
  • Method: one stack at the head. For the control, only sidebar-search-command.tsx was swapped to main's blob (Vite serves it) and the probe re-run. The file was then restored (git status clean) and the probe re-run as head-restored, which matched head.

Stack (isolated, owned by this run, now stopped)

  • Own PostgreSQL 16.15 + pg_search on 127.0.0.1:55378, an S3 double on :59078, and the repo's mock gateway on :4141 behind the e2e-mock provider env from setup.md §1.A.
  • App localhost:3741 → backend :3746.
  • All egress, from the app and Chromium, goes to a dead proxy (127.0.0.1:9). The only console errors are ERR_PROXY_CONNECTION_FAILED for external loads and 401s from pre-login session probes.
  • No model was called (search needs none), and no provider credential was connected.

Data (synthetic)

  • U1 owns Acme Alpha (A) and Beacon Bravo (B). U2 is a member of A only.
  • A has project Falcon Ledger AlphaOnly and chat Falcon payroll thread AlphaOnly.
  • B has Harbor Atlas BravoOnly and Harbor freight thread BravoOnly.

Driving: one Chromium profile (one localStorage) per run, driven through the UI:

  • the log-in form;
  • Ctrl+K, and Enter on the active row;
  • the Chats and Everything chips;
  • the user menu's Organization switcher;
  • browser Back;
  • Log out with its confirmation.
Card item main (control) PR head (head and head-restored)
1. In A, pick Falcon Ledger AlphaOnly (Everything) and Falcon payroll thread AlphaOnly (Chats). Switch to B with the user menu, then press Ctrl+K Leak. B's palette shows Recent searches · falcon · FLA · Falcon Ledger AlphaOnly. Its Chats scope shows falcon · Falcon payroll thread AlphaOnly B shows the empty state (Start typing to search) in both scopes, with focus in the combobox.
Same-identity control: A still replays both entries.
Storage: two tale.platform.search.recentSearches.v2:["<A>","<U1>","everything"|"chats"] keys, none for B
2. Log out (menu, then confirm). Log in as U2 in the same browser, open org A, press Ctrl+K Leak. U2 sees U1's two entries, one in each scope Empty state in both scopes. U1's v2 keys stay in localStorage under U1's identity (plaintext, as the PR states)
3. Open the palette in A with the sidebar Search button and type falcon (262–384 ms). Press browser Back to B's entry 4–25 ms after the last key, inside the 250 ms debounce Leak. falcon stays typed in B. 532 ms after the last key it is sent with orgId=<B> to all five of B's search endpoints (projects/search, tasks/search, chat/threads/search, documents/search-hub, contacts/search), 7 requests in all. Reopening the palette in B lists A's entry Empty input and the empty state. 0 requests carry falcon, and nothing is saved
4a. Focus and empty state in the normal flows The combobox has focus on open, the scope chips keep focus, and the empty state renders Same as main
4b. Focus after the identity change in step 3 Focus stays in the input, Escape closes the palette, and focus returns to Search Regression (below)

Finding (changes requested): keyboard focus after an identity change while the palette is open

If a control that survives the switch opened the palette, the key={historyIdentity} remount leaves focus on that control behind the open modal. The sidebar Search button is such a control.

  • Typing goes nowhere.
  • Escape doesn't close the palette (tried twice).
  • Tab moves to the page's Home link behind the overlay.
  • Only a mouse click into the input recovers.

It reproduced in 5 of 5 runs at the head: head, head-restored, the earlier head-try3, and variants v4 (typed) and v5 (nothing typed). So the remount causes it, not the debounce. It reproduced in 0 of 2 runs on main. Opened with Ctrl+K from <body>, or from the chat composer (which the route replaces), the head behaves correctly (variants v1–v3).

  • Mechanism. The remount unmounts the old Radix Dialog.Content while it is still open. Radix FocusScope runs its unmount auto-focus in a setTimeout(0). So the old instance's onCloseAutoFocus={restoreFocus} (packages/ui/src/components/search/search-command.tsx:228, use-restore-focus.ts) fires after the new instance has focused its input, and moves focus to the opener it captured.

  • jsdom reproduction in the PR's own harness: open with the open-chats button, type, rerender(palette('org-2')), then expect the combobox to have focus and Escape to close.

    • It fails at the head: focus is on BUTTON open-chats.
    • It passes with main's component.
    • The snippet is scripts/jsdom-remount-focus.snippet.test.tsx in the evidence.
    • The PR's identity test only asserts the input and titles, and opens with Ctrl+K from <body>, which hides this.
  • Fix direction (the author's call):

    • keep the dialog mounted across identity changes and reset only the controller state, e.g. a reset key for useSearchCommand, or a key below Dialog.Root;
    • or close the palette on an identity change before remounting.

    Either way, add the test above.

jsdom tests (item 5)

  • The PR's six targeted files at the exact head: 23/23 pass with bunx vitest --run --config vitest.ui.config.ts (Vitest 4.1.11, Node v24.21.0 + Bun 1.4.2, 30.6 s). There was no Zod loader failure.
  • Discrimination: the PR's sidebar-search-command.test.tsx against main's component fails exactly the 6 privacy cases and passes the other 5.
  • I did not re-run the author's 170 shared controls; the PR doesn't touch packages/ui.

Not caused by this PR (no change asked here)

  • routes/dashboard/switching.tsx:139 passes { replace: true } as TanStack history state (the signature is push(path, state, navigateOpts)), so the switching entry is pushed, not replaced. Back after a menu switch re-enters /dashboard/switching?to=<target> and switches to the same org again. I saw it in head-try1, and the code is the same on main. It's worth its own issue.
  • After an org change that replaced the opener (the chat composer), closing the palette drops focus to <body>. This happens identically on main and the head.

Not covered

  • a screen reader;
  • the phone layout;
  • a cross-tab sign-out with the palette open (the same remount path, not driven);
  • visual-aspect-analyzer (this PR adds no new UI).

CI at 03:41Z: 22 success, 10 skipped, 25 pending, 0 failed, so not terminal. I didn't rerun or cancel anything.

Verdict: changes requested, for the 4b keyboard regression only. Items 1–3 and 5 pass at the exact head, and main reproduces every leak, so the privacy fix is proven in the real UI. Re-verification needs only the focus fix and its test. No merge, push or card move.

The evidence (logs, screenshots, scripts, SHA256SUMS) is in the TALE-540 delivery box, /agent/output/8f3183ca-1b55-4505-934c-eb264f902e45/.

@yannickmonney
yannickmonney force-pushed the fix/scope-global-search-history branch from ff0a7f3 to c89749c Compare October 9, 2026 02:40
@yannickmonney
yannickmonney disabled auto-merge October 9, 2026 02:55
@yannickmonney
yannickmonney force-pushed the fix/scope-global-search-history branch from c89749c to b2da52e Compare October 9, 2026 10:36
@yannickmonney yannickmonney changed the title fix(platform): scope search history to organization and account fix(platform): preserve search privacy and keyboard focus Oct 9, 2026
@yannickmonney
yannickmonney force-pushed the fix/scope-global-search-history branch 2 times, most recently from bfcd5d5 to 7f13f12 Compare October 9, 2026 14:59
@yannickmonney
yannickmonney added this pull request to the merge queue Oct 9, 2026
@yannickmonney
yannickmonney added this pull request to the merge queue Oct 9, 2026
@yannickmonney
yannickmonney removed this pull request from the merge queue due to a manual request Oct 9, 2026
@yannickmonney
yannickmonney force-pushed the fix/scope-global-search-history branch from 7f13f12 to a0ef924 Compare October 9, 2026 23:50
@yannickmonney
yannickmonney added this pull request to the merge queue Oct 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 10, 2026
@yannickmonney
yannickmonney added this pull request to the merge queue Oct 10, 2026
@yannickmonney
yannickmonney removed this pull request from the merge queue due to a manual request Oct 10, 2026
@yannickmonney
yannickmonney force-pushed the fix/scope-global-search-history branch from a0ef924 to 316e891 Compare October 10, 2026 09:06

This branch has not been deployed

No deployments
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.

Bug: Global search replays another organization's private query history

1 participant