Persist search sheet state across close/reopen#5534
Conversation
6474fb0 to
48ab0d6
Compare
Preview deploymentsHost Test Results 1 files 1 suites 2h 58m 43s ⏱️ Results for commit a423790. Realm Server Test Results 1 files ±0 1 suites ±0 12m 22s ⏱️ +19s Results for commit a423790. ± Comparison against earlier commit e42d46b. |
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
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 holds the sheet's state (query text, selected types/realms, sort, view toggle, pagination) plus a snapshot of the last results.@mode-gated subtree is still destroyed on close, so there is no background work while the sheet is closed (the search resource and its realm subscriptions tear down).@persistflag passed only by the search sheet, so the card choosers (which render their ownSearchPanelinstances) keep their own clean, per-invocation state.resetservice, so logout / realm reset clears it. A page reload also clears it (in-memory only, nolocalStorage).resetStatenow runs only on explicit Cancel / Escape — removed from plain close/blur and from result-select, so those keep the search. Reopening a persisted search opens straight to the results view.Files
services/search-sheet-state.ts(new) — the in-memory store + results snapshot, reset-registered.search-sheet/index.gts— state backed by the service; reset only on explicit cancel.card-search/panel.gts— additiveinitialActiveSort/onSortChange/persistargs (choosers unaffected).card-search/panel-content.gts— view + pagination behind@persist; main-results snapshot capture/redisplay.operator-mode/submode-layout.gts— reopen to the results view when a search is persisted.Verification
lint:types,lint:js, andlint:hbspass inpackages/host(with boxel-icons + boxel-ui built).Remaining
🤖 Generated with Claude Code