Persist search sheet state across close/reopen#5534
Conversation
6474fb0 to
48ab0d6
Compare
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 17m 3s ⏱️ - 1h 48m 52s Results for commit bf76d9c. ± Comparison against earlier commit bf847b7. For more details on these errors, see this check. Realm Server Test Results 1 files ±0 1 suites ±0 13m 23s ⏱️ -1s Results for commit ec140da. ± Comparison against earlier commit bf76d9c. |
Add an in-memory, session-scoped SearchSheetState service that holds the operator-mode search sheet's query, selected types/realms, sort, view, and pagination, plus a snapshot of the last results. The sheet's @mode-gated subtree is still destroyed on close (no background work while closed); on reopen it rehydrates from the service, redisplays the cached results snapshot immediately, and re-runs the query to refresh. Persistence is opt-in via a `@persist` flag passed only by the search sheet, so the card choosers keep their own ephemeral state. The service registers with the reset service, so logout/realm reset clears it; a page reload also clears it (in-memory, no localStorage). resetState now runs only on explicit Cancel/Escape (removed from plain close/blur and result-select), and reopening a persisted search opens straight to the results view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The search sheet's result snapshot and the search resource each wrote tracked state during the render that consumed it, tripping Glimmer's mutate-after-consume assertion and failing host tests as a global error. - Capture the main-results snapshot in a microtask instead of during the modifier's render, and key it to the current wire query so a reopen only redisplays results for the same search (never stale rows under a different term, and an idle sheet never clobbers a snapshot). Extract the snapshot-vs-live decision into a pure, unit-tested helper. - Start SearchEntriesResource's search a microtask after modify so the task's isRunning write lands outside the consuming render — the case that bites when a SearchResults mounts with a query already set (the sheet reopening restored, or a chooser rendering recents). Load tracking stays synchronous via a Deferred so prerender readiness is unaffected. - Update the reopen expectation in the interact-submode acceptance test to the persisted behavior, and add acceptance + unit coverage for persist, Cancel/Escape reset, empty-then-close, and service reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reopening the operator-mode search sheet with an unchanged query now seeds the freshly-mounted search resource from the retained snapshot and skips the fetch, so the prior results show immediately with no re-run and no "Searching…" flash. A one-shot guard keeps the seed from re-applying, so any query change still runs a fresh search. The results-list scroll offset is captured on scroll and restored on reopen (session-only). The seeded resource holds the rows directly, so the display-time resolveMainResults snapshot path is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The search sheet now keeps its query and filters on a plain close (click outside); only an explicit Cancel or Escape clears. Update the two operator-mode UI tests that asserted the old reset-on-blur behavior: - realm filter: selecting a realm then clicking outside and reopening keeps the realm checked. - type filter: a search term plus a type selection survive click-outside, so reopening returns to the results view with both still applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7a7cbc7 to
04c73c6
Compare
The code-mode playground autogenerates a blank instance when it sees a search that has settled with no results. Starting the search resource's task a microtask after modify() (needed so a SearchResults mounting with a query already set doesn't flip isRunning mid-render and trip Glimmer's backtracking assertion) left a window where a just-set query read as settled-with-no-results — so the playground autogenerated "Untitled" instances, breaking the playground / spec / AI-assistant suites. Keep the deferred start, but have isLoading also report loading across that gap: it's true whenever a query is set and no run has completed yet, read from the untracked #previousQuery / hasCompletedFullRun fields so nothing tracked is mutated mid-render. The seed branch marks the run complete synchronously, so a seeded reopen still presents as settled with no flash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4b75873 to
37a7867
Compare
The session-scoped service now owns the search query and the SearchEntriesResource itself, so the results survive a sheet close/reopen without being snapshotted, re-seeded, or revalidated — the resource simply outlives the sheet's subtree, and its realm subscriptions stay live while closed. - Service derives mainQuery from its own inputs (idle when no term, type, or realm), so a type-filtered reopen never runs unfiltered and resetState() collapses the resource to idle. - SearchResults takes a @resource arg instead of @seed/@onSnapshot; PanelContent passes the service resource when persisting and keeps its own query derivation for the card choosers. - An externally-triggered search resets state and remounts the panel via a trigger epoch, so a trigger while the sheet is open re-reads the freshly-reset filter display. - isLoading settles after a failed run (no permanent "Searching…"), and a pending debounce is cancelled on Cancel/Escape. - Delete the snapshot/seed machinery and its tests; add coverage for the query derivation, failed-run settling, and clean triggered search. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Search-panel components no longer know about the search-sheet service. PanelContent is now controlled/uncontrolled via plain args (mainSearchResource, viewId/onViewIdChange, pagination, scrollTop/ onScrollTopChange); the sheet — which owns the service — passes them in. SearchPanel drops the `persist` flag. Replace the bespoke rAF-countdown scroll restore with a reusable persist-scroll-position modifier that re-applies the offset on every geometry change (MutationObserver + ResizeObserver) and only records gesture-driven scrolls, so restore no longer races the sheet's open transition and late-hydrating rows (flash-to-top / stuck-at-top). Adds deterministic integration tests for the modifier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restoring the persisted offset on reopen painted its intermediate states: the list appeared at the top mid-animation, then jumped or drifted as the offset re-applied against still-hydrating rows. The modifier now tags the container `data-scroll-restore-pending` while a non-zero saved offset is in flight, and the search sheet's content hides behind the tag and fades in on removal — the list reveals already in place. The tag comes off when an applied offset survives read-back and the container box has been quiet ~100ms (a stick against the mid-transition tiny viewport is trivial and must not count), on the first user gesture, or after a 1s cap when the offset is unreachable because the list got shorter. Restore runs once per element: recording writes the live offset back into the tracked state feeding the `scrollTop` arg, which re-runs the modifier on every recorded scroll — a re-install is recording-only (no tag, no observers, no re-applies), so the user's own scrolling is never hidden or fought. Close/reopen mounts a new element, so each reopen still restores exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a423790714
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.searchSheetMode = this.searchSheetState.searchKey.trim() | ||
| ? SearchSheetModes.SearchResults | ||
| : SearchSheetModes.SearchPrompt; |
There was a problem hiding this comment.
This reopen gate only checks the saved text, but the new state service also treats selected types and selected realms as active search criteria. For filter-only searches such as Code mode’s “Find instances” (openSearch('', ref)), closing and reopening via the search button sends the sheet to the compact prompt even though mainSearch still has a type-filtered query; compact mode renders only recents, so the saved results are hidden until the user changes the search again.
Useful? React with 👍 / 👎.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review — reactivity & lifetime of the service-owned search resource
This review focused on the two things most likely to bite in this design: the reactivity/lifetime contract of the new session-scoped SearchEntriesResource, and whether "reopen restores the search" is complete across every way a search becomes active (term, URL, type, realm).
Bottom line: the architecture is sound and the reactivity handling is careful and correct. One confirmed gap (filter-only searches don't reopen to their results) is worth closing in this PR, and the PR description's "Approach" section now contradicts the shipped behavior and should be reconciled. No crashers; host + realm CI is green.
What lands right
- Moving the query and the
SearchEntriesResourceinto the session-scoped service is the right call over the earlier snapshot/re-seed approach — the results survive close/reopen because the resource literally outlives the@mode-gated subtree, with no snapshot-vs-live handoff to keep correct. DerivingmainQueryfrom the service's own inputs (not the panel's async-restored filter state) is what makes a type-filtered reopen never transiently run unfiltered. - The deferred task start + two-flag
isLoading(#hasSettledvshasCompletedFullRun) is subtle but correct — details in the inline confirmation onsearch-entries.ts. It's a genuine guard rail worth keeping intact. - The controlled/uncontrolled split on
PanelContent(choosers pass none of the service args and keep their ephemeral local state;SearchResultsbranches once at construction on whether@resourceis present) keeps the blast radius off every other consumer of the search components.
Recommendations
- [should-fix, this PR] Reopen gate misses filter-only searches — see the inline thread on
openSearchSheetToPrompt. Confirms the Codex flag with a verified path ("Find instances" → close → reopen); gate on the service'shasActiveSearchrather thansearchKey.trim(). - [non-blocking, please reconcile] The description's Approach still reads: "there is no background work while the sheet is closed (the search resource and its realm subscriptions tear down)." The shipped service does the opposite, and documents it so in
search-sheet-state.ts: once a search is active, the resource and its realm subscriptions stay live for the whole session, and a plain close does not stop them. BecausemainQueryis always page-capped (page: { size: SECTION_DISPLAY_LIMIT_FOCUSED }), the resource never qualifies for the selective or partial-refresh paths (query.page === undefinedis required for both), so every incremental-index /prerender_htmlevent in any searched realm triggers a full re-run — ongoing while the sheet is closed, until Cancel/Escape or a page reload. That's a defensible, deliberate tradeoff, but it's a real ongoing-cost change from the old "close tears everything down" model, and a plain click-outside close leaving a live re-running search is easy for a user not to realize. Please align the description with the code and confirm the tradeoff is intended. - [non-blocking, coverage] Nothing pins the filter-only reopen from (1): the
mainQueryunit test exercises only term-based derivation (not type-only), and no acceptance test reopens after a type-only "Find instances". A regression test there would lock the fix and the intended results-view reopen.
Adjacent, out of scope: I traced the ScrollToFocusedSection (scroll-to-0 on a restored focusedSection) vs persistScrollPosition (restore saved offset) interaction on reopen — the restore's per-frame re-apply burst outlasts the one-shot scroll-to-0, and the scroll-to-0 fires during restoring === true so it isn't recorded — so no visible conflict. Noting it only so it's on record; nothing to change.
Submitted as a comment — approve / request-changes is the maintainer's call.
Generated by Claude Code
| // Reopen straight to the results view when a search is persisted, so the | ||
| // restored results are shown immediately rather than the compact prompt. | ||
| this.searchSheetMode = this.searchSheetState.searchKey.trim() | ||
| ? SearchSheetModes.SearchResults | ||
| : SearchSheetModes.SearchPrompt; |
There was a problem hiding this comment.
[Claude Code 🤖] Reopen gate misses filter-only searches — confirms the Codex bot's flag on this line, verified against the code path.
The claim. Reopening a filter-only persisted search (a type or realm selected, empty term) sends the sheet to the compact prompt instead of the results view, so the restored results stay hidden until the user changes the search.
The mechanism. This gate keys on this.searchSheetState.searchKey.trim(), but the service treats a search as active when there's a term or a type or a realm — see hasActiveSearch in services/search-sheet-state.ts, which is exactly what drives whether mainQuery (and so the resource) produces results. Concrete path: code mode's "Find instances" calls openSearch('', ref) (detail-panel.gts → searchForInstances) → doExternallyTriggeredSearch('', ref) in search-sheet/index.gts sets searchKey = '' and selectedTypes = [ref]. A plain click-outside close keeps that state; reopening via the search button lands here → ''.trim() is falsy → SearchSheetModes.SearchPrompt. Prompt ⇒ isCompact is true, and in sheet-results.gts the main-query sections render only in the non-compact {{else}} branch — the {{#if @isCompact}} branch shows recents only. The type-filtered rows are live in the resource but never shown.
The fix. Gate on the service's own notion of an active search — it already covers term / URL / type / realm in one place, so the check can't drift the way a term-only re-derivation here already has. Make hasActiveSearch public (drop private on the getter) and:
| // Reopen straight to the results view when a search is persisted, so the | |
| // restored results are shown immediately rather than the compact prompt. | |
| this.searchSheetMode = this.searchSheetState.searchKey.trim() | |
| ? SearchSheetModes.SearchResults | |
| : SearchSheetModes.SearchPrompt; | |
| // Reopen straight to the results view when a search is persisted, so the | |
| // restored results are shown immediately rather than the compact prompt. | |
| this.searchSheetMode = this.searchSheetState.hasActiveSearch | |
| ? SearchSheetModes.SearchResults | |
| : SearchSheetModes.SearchPrompt; |
hasActiveSearch is also true for a URL key (it's non-empty), so the URL-paste reopen — which likewise only renders its section in the results view — keeps working; the existing acceptance test for that stays green.
Scope. Regression in this PR's new reopen behavior; should-fix here (small, and the single-source gate stops the term-vs-active-search drift from recurring). Not a crasher — blocking is your call.
Generated by Claude Code
| // reads the task's `isRunning` (through `isLoading`) during render, so a | ||
| // synchronous `perform()` here — which flips `isRunning` — would mutate a | ||
| // value already consumed in the same computation, tripping Glimmer's | ||
| // backtracking assertion (a `<SearchResults>` mounting with a query already | ||
| // set — a chooser or the playground — hits this). Register the load | ||
| // synchronously (via a Deferred) so a prerender still waits for the search, | ||
| // but defer the task start a microtask so its `isRunning` write lands after | ||
| // the render. `isLoading` below still reports loading across that gap. | ||
| let loaded = new Deferred<void>(); | ||
| this.#trackSearchLoad(loaded.promise); | ||
| void Promise.resolve().then(() => { | ||
| if (isDestroyed(this) || isDestroying(this)) { | ||
| loaded.fulfill(); | ||
| return; | ||
| } | ||
| this.search.perform().then( | ||
| () => loaded.fulfill(), | ||
| () => loaded.fulfill(), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation — nothing to change; a guard rail for the next editor.
This deferred task start is load-bearing and correct, and it's the kind of thing a later "just call perform() directly" simplification would silently break, so it's worth calling out that it's intentional:
- Starting the task a microtask after
modify()(instead of a synchronousperform()) keeps a<SearchResults>that mounts with a query already set — a chooser, the playground, or the sheet reopening restored — from flippingsearch.isRunninginside the render that just consumed it (Glimmer's mutate-after-consume backtracking assertion). - The paired
isLoadingchange (reporting loading across the gap via the untracked#previousQuery/#hasSettled) is what stops a just-set query from reading momentarily as settled-with-no-results — the failure that made the code-mode playground autogenerate a blank "Untitled" instance.
Verified the settle path is right: #hasSettled flips in the task's finally, so a failed run also settles (no permanent "Searching…"), while hasCompletedFullRun stays gated to a successful full run for the partial-refresh precondition — the two-flag split is deliberate. The Deferred keeps the prerender load-tracking synchronous so render readiness is unaffected. Good as is.
Generated by Claude Code
Gate the reopen-to-results decision on the search-sheet state service's own hasActiveSearch (term OR type OR realm) instead of re-deriving "active" from the search term alone. A filter-only search — code mode's "Find instances" sets a type with an empty term — now reopens to its live, type-filtered results rather than the recents-only compact prompt. Cover it with a type-only unit case on hasActiveSearch/mainQuery and an acceptance test that reopens a filter-only search to the results view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
[Claude Code 🤖] Addressed the review feedback in bf847b7:
The confirmation on the deferred task start in |
main renamed ResetService (services/reset.ts) to SessionService (services/session.ts). Update the search-sheet-state service and its unit test to register with SessionService and to trigger a logout reset via notifySessionEnded() instead of the removed ResetService.resetAll(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The reopen gate keys on hasActiveSearch (term OR type OR realm), so a realm-filtered search with no term reopens to the results view, not the compact prompt. Update the integration test's stale expectation to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Resolves CS-12109.
Problem
Closing the operator-mode search sheet discards the entire search — reopening starts from a blank slate. The
@modegate atsearch-sheet/index.gtsdestroys the wholeSearchPanel → PanelContent → SearchResults → SearchEntriesResourcesubtree on close, dropping the query, filters, sort, view, pagination, and results.Approach
A new in-memory, session-scoped
SearchSheetStateservice owns the sheet's inputs (query text, selected types/realms, sort, view toggle, pagination, scroll offset), the derived query (mainQuery), and the oneSearchEntriesResource(mainSearch) itself. Components become pure consumers — nothing is snapshotted or re-seeded.@mode-gated subtree. On reopen the subtree remounts and rehydrates from the service; because the resource never died, the results are already present (no blank flash, no re-run for an unchanged query).mainQueryis derived from the service's own inputs, not the panel's asynchronously-restored filter state, so a type-filtered reopen never transiently runs unfiltered.mainQueryis always page-capped, incremental-index /prerender_htmlevents in searched realms trigger a background re-run while the sheet is closed. This is a deliberate tradeoff (fresh results on reopen) versus the ongoing subscription cost; the state fully clears on Cancel/Escape, logout/realm reset, or a page reload.@persistflag passed only by the search sheet, so the card choosers (which render their ownSearchPanelinstances) keep their own clean, per-invocation state — theirSearchResultsstays component-owned.resetservice, so logout / realm reset clears it; it is in-memory only (nolocalStorage), so a page reload also clears it.resetState()runs only on explicit Cancel / Escape (not on plain close / blur / result-select), and drives the resource to its dormant branch (idle query → unsubscribe every realm → clear rows/meta). Reopening a persisted search opens straight to the results view, gated on the service'shasActiveSearch(term or type or realm) so filter-only searches restore too.Files
services/search-sheet-state.ts(new) — the session-scoped store: persisted inputs, derivedmainQuery, the service-ownedmainSearchresource,resetState(), reset-registered.resources/search-entries.ts— deferred task start (avoids a mutate-after-consume backtracking assertion when a query is already set at mount) + a two-flagisLoadingthat reports loading across the deferred start.search-sheet/index.gts— state backed by the service; filters written into the service; reset only on explicit Cancel/Escape;SearchPanelremounted on an external trigger (e.g. code mode's "Find instances") so it re-reads the freshly-reset filter state.search/panel.gts— additiveinitialActiveSort/onSortChange/persistargs (choosers unaffected).search/panel-content.gts— view + pagination behind@persist; main-resultsSearchResultsconsumes the service's resource.search/search-results.gts— optional@resourcearg (host-only); choosers keep the internally-created resource.operator-mode/submode-layout.gts— reopen to the results view when a search is active.modifiers/persist-scroll-position.ts(new) — restores the results-list scroll offset on reopen (reveal-gated to avoid a mid-transition jump).Verification
lint:types,lint:js, andlint:hbspass inpackages/host. Host acceptance + unit coverage ininteract-submode-test.gtsandsearch-sheet-state-service-test.ts: persist across close/reopen (including a filter-only search), no re-run for an unchanged query, scroll restore, Cancel/Escape resets, empty-query reopen to the compact prompt, in-memory (reset/reload) clears, choosers stay clean.🤖 Generated with Claude Code