diff --git a/CHANGELOG.md b/CHANGELOG.md index 828aea58..6f793707 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to the **GemStone Smalltalk** extension will be documented i ### Added +- **GemStone Search has a refresh button.** Its class, dictionary and global lists are loaded once when the panel opens and then kept up to date by the things that announce themselves — a compile from the IDE, a class removed in the Explorer, a commit or abort. Code created or removed by *executing* it — a `subclass:` or `compileMethod:` evaluated in a workspace, a global assigned in a doit — announces nothing, so it stayed invisible to the search until the next commit. A **⟳** button in the search panel (and in the panel's title bar, plus **GemStone: Refresh GemStone Search** in the Command Palette) reloads everything from the stone and re-runs what you have typed — including re-fetching an open senders/references list, which is just as stale as the rest. ([#517](https://github.com/GemTalk/Jasper/issues/517)) - **A session's stone and gem configuration now has its own panel, opened from the session row.** Seeing what a live session is actually running with — and changing the values that can be changed — was only possible inside the all-in-one GemStone Manager webview, which bundled connection, versions, databases, processes, logs and configuration into one surface; there was no way to look at configuration on its own, and nothing on the session row led to it. A gear on each session row in **Logins & Sessions** (also **GemStone: Session Configuration** in the Command Palette, which uses the active session) opens **Session Configuration** as an editor tab for *that* session, read over that session's own connection rather than whichever session happens to be active — so two sessions can be opened side by side and compared, and a panel closes itself when its session logs out. Each parameter says what you can do with it and why: config-file parameters (`ALL_CAPS`) are read-only because they were fixed before the stone started, a stone parameter is read-only unless you are SystemUser, and values of a kind that cannot be edited in place say so too — with the type and the purpose text from `system.conf` on an ⓘ that pins on click. The stone stays the authority on a change: its verdict comes back in its own words, beside the row that was changed, so a refused or silently-ignored set is visible rather than assumed. A filter box narrows a long report, Refresh re-reads from the session, and **Ping** moved off the session row into the panel — the row is losing buttons rather than gaining them. This is the first piece of the GemStone Manager to be pulled out into a surface of its own, with navigation staying in the sidebar. ([#511](https://github.com/GemTalk/Jasper/issues/511), [#232](https://github.com/GemTalk/Jasper/issues/232)) - **Breakpoints you can set, aim, disable and manage.** Setting a breakpoint used to mean a gutter click, which lands on the leftmost step point of the line — but a Smalltalk line routinely holds several, so the one you wanted was often not the one you got, and there was no way to disable a breakpoint, act on all of them at once, or see what the session actually had. Breakpoints now live in VS Code's own breakpoint list, so the gutter, the per-breakpoint checkbox and the built-in Enable/Disable/Remove All controls all drive GemStone through it. **Toggle Breakpoint at Cursor** (`Shift+F9`) breaks at the step point under the caret rather than the line's first, and the token that will actually break is outlined — grey and faded when disabled, matching the grey the gutter dot goes. A disabled breakpoint is kept in the gem as set-then-disabled, so re-arming it is instant. **Enable All**, **Disable All** and **Remove All Breakpoints** cover every GemStone breakpoint, including ones set outside Jasper. ([#262](https://github.com/GemTalk/Jasper/issues/262)) - **Step points are numbered where they are, without getting in the way.** Step point numbers are drawn as inlay hints — VS Code's own dim, recessive style, suppressed or restyled by its `editor.inlayHints.*` settings like any other — and each number is clickable to toggle a breakpoint at that step point. `gemstone.stepPoints.display` decides when they show: `debugging` (the default, so they appear while a debug session runs and stay out of the way while you are reading or writing code), `always`, or `off`, with **Toggle Step Point Numbers** to flip them. Whatever the setting, hovering a step point reports its number and breakpoint state with links to set, clear, enable or disable it. Numbers are withheld while a buffer has unsaved edits, since the stone's offsets no longer describe the text on screen and a wrong number is worse than none. ([#262](https://github.com/GemTalk/Jasper/issues/262)) @@ -28,6 +29,8 @@ All notable changes to the **GemStone Smalltalk** extension will be documented i ### Fixed +- **GemStone Search now finds the method you actually typed.** Searching for a common selector could miss its most obvious implementor: typing `at:` returned `instVarAt:put:` and `floatAt:put:` from a couple of incidental classes and no `Array>>at:` at all. The search asks the stone for a bounded number of matching selectors to keep searching-as-you-type fast, but the stone collected the *first* ones it walked into rather than the *best* ones — and with over a thousand selectors containing `at:`, the thirty-odd classes that implement `at:` itself sat far past the cut-off and were never sent. The scan now ranks before it truncates — the selector you typed first, then selectors starting with it, then selectors merely containing it — so what a cut-off drops is the least relevant tail. ([#517](https://github.com/GemTalk/Jasper/issues/517)) +- **GemStone Search no longer answers out of the session you just left.** Making another session active left the search panel showing the previous session's results, under the query that produced them, with the previewed source alongside — and because the rows still looked live, opening one fetched a method from the session that was now current. Switching sessions (or logging out of the last one) now clears the panel and rebinds it, and an open Spotter tab is re-pointed at the new session instead of quietly searching the old one. ([#517](https://github.com/GemTalk/Jasper/issues/517)) - **A named breakpoint deleted while Jasper was still asking which class no longer comes back.** A function breakpoint typed as a bare selector has to be pinned to one class, and where several implement it Jasper asks. Deleting the row instead of answering left the picker still open, and answering it then re-created the breakpoint that had just been removed. The breakpoint is checked for still being there before it is converted. ([#262](https://github.com/GemTalk/Jasper/issues/262)) - **Step points are read in one round trip instead of three.** Source, step point offsets and selector ranges were three sequential calls to the stone, made synchronously on the extension host from the hover and the inlay hints — so the first hover on a freshly opened method stalled the editor for all three in a row. They now arrive in one reply, which is also less work for the stone, since it computed the first two to answer the third anyway. ([#262](https://github.com/GemTalk/Jasper/issues/262)) - **A method is named the same way everywhere.** Function-breakpoint messages and log lines wrote `Account class>>reset` while every other method list — Senders, Implementors, References, safe delete — wrote `Account class >> #reset`. They all use the one shared helper now. ([#262](https://github.com/GemTalk/Jasper/issues/262)) diff --git a/client/src/omniSearch/DESIGN.md b/client/src/omniSearch/DESIGN.md index 02132b54..59c2eaf3 100644 --- a/client/src/omniSearch/DESIGN.md +++ b/client/src/omniSearch/DESIGN.md @@ -77,12 +77,43 @@ Global "search anything browsable" for the GemStone IDE — the Jasper answer to | A class removed (Explorer → Remove Class) | `notifyClassRemoved` → `applyChange`, fired **once per class** because the delete takes the subtree | re-fetch per name; the lookup comes back empty and the entry drops | | Dictionary add / remove / rename | `onSymbolListChanged` → `notifySessionSynced` | full `resync` | | Commit / abort / file-in | `notifySessionSynced` | full `resync`, deferred while hidden | - - Everything else — a global created by evaluating code, a class removed by another session — is - only picked up by the next commit/abort `resync`. That is the by-design staleness window. + | The user presses ⟳ / runs `gemstone.search.refresh` | `refresh` | full reload + the references list; deferred only while the docked panel is collapsed | + + Everything else — a global created by evaluating code, a class or method compiled by a workspace + doit, a class removed by another session — announces nothing this panel can listen for, so short of + a commit or abort it is not picked up at all. That staleness window is by design but it has no + upper bound, which is why the **⟳ refresh** exists: it is the user's way to close it on demand, + without inventing a polling scheme or making every doit fire a corpus reload + ([#517](https://github.com/GemTalk/Jasper/issues/517)). It clears any deferred sync rather than + paying for both. Pressing ⟳ in the chrome is never deferred — the click proves someone is looking — + but running the command while the docked panel is COLLAPSED is: its view is disposed, so the reload + would pay three image-wide executes to post results to nobody. That request is remembered + (`refreshPending`) and paid on the next reveal, so the panel you come back to is the fresh one you + asked for. The control lives in the webview chrome (so both surfaces have it) AND in the view's + title bar (so it is discoverable where VS Code users look for a refresh). + + It is a SEPARATE engine call from `resync`, not the same one wired to a button, and the difference + is the pivot. `resync` deliberately leaves a references list alone — a commit is not a request to + disturb what you are reading — but that made the ⟳ look like a dead button for anyone who happened + to have one open: the corpora reloaded silently and the stale senders stayed on screen. So + `refresh` re-asks the stone who references the row the pivot was taken from (keeping whatever + filter is typed into it), and if that row is gone — its method or class deleted — it leaves the + pivot rather than keep showing senders of nothing. - _Methods_: the selector space is too large to preload, so this provider queries the stone **per search term** (debounced, min query length `methodMinQueryLength`), reusing the - `searchSelectors` machinery. + `searchSelectors` machinery. That scan is **bounded and ranked**: it walks every selector of every + class in the symbol list, sorting matches into three tiers — the selector IS the term, STARTS WITH + it, merely CONTAINS it — and returns them in that order, capped per tier. Ranking has to happen on + the SERVER because the cut-off does: the walk visits dictionaries in hash order, so a scan that + stopped at the first `limit` matches answered whichever classes it reached first, and a term as + common as `at:` filled its slice with `instVarAt:put:` and friends while `Array>>at:` — the row the + user wanted — was never sent + ([#517](https://github.com/GemTalk/Jasper/issues/517)). The client re-ranks what it receives with + the configured matcher, but it can only order rows it was given. The walk no longer short-circuits + on a match count (a better-tier hit can be anywhere in the image); the one early exit left is a + FULL exact tier, where nothing later can displace a row. Cost on a 3.6.2 base image: ~27 ms for + `at:`, against ~2 ms for the old early exit — and a full walk was already the price of every + precise term, which never reached the old cutoff at all. - **Explicit-only** categories (`OmniCategory.explicitOnly`) are **excluded from the all-scope fan-out** — they run only when the user scopes to them, so heavyweight work never fires on a plain search: @@ -166,6 +197,38 @@ Global "search anything browsable" for the GemStone IDE — the Jasper answer to the current term. (The origin QuickPick could only express scope with cramped icon title buttons + the title text — the webview tabs are the intended affordance.) +8. **A search belongs to one session, and says so when that changes.** Both hosts hold an engine built + from ONE session's deps — its providers, its activation and its source preview all close over that + session — plus a webview full of rows read out of it. Nothing used to react when the user made + another session active: the docked panel rebuilt its engine only when something else happened to ask + it to (a reveal, a settings change, the next keystroke), and an open Spotter never rebuilt at all. + Until then the panel kept answering, and opening rows, out of a session the user had left + ([#517](https://github.com/GemTalk/Jasper/issues/517)). + + `SessionManager.onDidChangeSelection` now reaches both hosts. Two decisions worth recording: + + - **The webview is wiped, not just the engine.** Dropping the engine is enough for a settings change, + because the rows on screen are still true. Here they are not: they came from the old session, yet + they still look live, and activating one would open a document against the session that is now + current — a wrong answer presented as a right one. So the query, the results, any references pivot + and the preview all go, and the scope returns to All (where the replacement engine starts). + - **The wipe is NOT deferred while hidden, though the rebuild still is.** The `visible` gate exists to + avoid paying for image-wide GCI executes on a background path, and that reasoning still holds for + re-priming. It does not hold for clearing the screen: a reveal cannot un-show stale rows + retroactively, so the cheap part happens immediately and only the expensive part waits. + + The Spotter is **re-pointed in place** rather than closed and reopened: it is an editor tab the user + put there (possibly pinned), and the tab, its pin and its loaded HTML are all session-independent. + `show()` for a different session takes the same path, which removed its old dispose-and-recreate + branch. Logging out of the last session is the same event with nothing to bind to: both hosts reset + and say "Log in to a GemStone session to search" instead of leaving the departed session's rows up — + and both DROP THE ENGINE as well. (The docked panel is always still there to see this; an UNPINNED + Spotter has already disposed itself on focus-out by the time you reach the logout, so only a pinned + one takes this path.) Clearing the screen alone would leave the departed session's primed + corpora (and an `activate` closed over its GCI handle) one keystroke away: the docked host's + `ensureEngine` gate already refused to answer without one, and the Spotter now refuses the same way, + showing the notice instead of searching. A later login rebinds both. + ## Module map (`client/src/omniSearch/`) | File | Responsibility | Stone? | Tested | @@ -200,8 +263,8 @@ New shared query (if needed) lives under `client/src/queries/` per repo conventi - `categories`: which providers are enabled (default: all seven — `classes, methods, dictionaries, globals, source, literals, categories`). - `maxResultsPerCategory`: number (default `20`) — how many rows are **shown** per scope. -- `maxServerScan`: number (default `200`, clamped 20–20 000) — how many matches a scope's - **server-side scan** collects before it stops. A different bound from `maxResultsPerCategory`; see +- `maxServerScan`: number (default `200`, clamped 20–20 000) — the most matches a scope's + **server-side scan** hands back. A different bound from `maxResultsPerCategory`; see "Two different limits bound a result set" below. - `debounceMs`: number (default `120`). - `methodMinQueryLength`: number (default `2`) — min chars before the Methods provider queries the stone. @@ -241,11 +304,15 @@ Behaviour decisions (Eric's review of the first webview cut): ## Two different limits bound a result set The display cap (`maxResultsPerCategory`, raised by Load-more/Load-all) is not the only bound — the -**Methods** scope also has a server-side one. `searchSelectors` short-circuits the moment it has -`limit` matches, and `methodsProvider` clamps that limit to `maxServerScan` (default 200) however high -the display cap goes. So with the default a broad selector term can never yield more than 200 rows, -Load-all included. That ceiling is a **setting** rather than a constant precisely because the honest -answer to "I want more than 200" is "raise the scan, and accept a slower search". +**Methods** scope also has a server-side one. `searchSelectors` yields at most `limit` rows, and +`methodsProvider` clamps that limit to `maxServerScan` (default 200) however high the display cap goes. +So with the default a broad selector term can never yield more than 200 rows, Load-all included. That +ceiling is a **setting** rather than a constant precisely because the honest answer to "I want more than +200" is "raise the scan, and accept a slower search". + +What the ceiling drops is the least relevant tail, not an arbitrary slice: the scan ranks by match tier +before it truncates (decision 3), so the exact and prefix hits survive a cut-off that would once have +discarded them unseen. The two bounds mean different things to the user, so a provider reports when its OWN ceiling was the one that bound it (`OmniTruncationSink`, an optional 4th argument to `OmniProvider.search`, carrying diff --git a/client/src/omniSearch/__tests__/methodsCeiling.integration.test.ts b/client/src/omniSearch/__tests__/methodsCeiling.integration.test.ts index 25ef110a..22124cc1 100644 --- a/client/src/omniSearch/__tests__/methodsCeiling.integration.test.ts +++ b/client/src/omniSearch/__tests__/methodsCeiling.integration.test.ts @@ -2,16 +2,18 @@ // matrix (3.6.2 and 3.7.5). Base-image reflection only — no server plugin — so it runs in both the // bare and plugin CI passes. // -// Regression guard for GemStone Search triage #14. Two halves, both previously untested: -// 1. the generated selector scan really is BOUNDED — `searchSelectors` short-circuits the instant it -// has `limit` matches, so a full slice genuinely means "there are more we never saw"; +// Two halves, both previously untested: +// 1. the generated selector scan really is BOUNDED — `searchSelectors` hands back at most `limit` +// rows, so a full slice genuinely means "there are more we never saw"; // 2. `methodsProvider` turns that into the truncation signal the engine needs, so the footer stops // presenting a cut-off slice as an exact total. // // The unit tests cover the clamp arithmetic with fakes; this one proves the same thing end-to-end -// against real GemStone reflection, where the short-circuit actually happens. It uses a fixture class -// with a unique selector substring and a deliberately tiny cap, so the assertions are exact numbers -// rather than "the base image probably has more than 200 of these". +// against real GemStone reflection, where the cut-off actually happens. It uses a fixture class with a +// unique selector substring and a deliberately tiny cap, so the assertions are exact numbers rather +// than "the base image probably has more than 200 of these". +// +// Which rows survive the cut-off is a separate question, covered by methodsRelevance.integration. import { describe, it, expect, vi } from 'vitest'; vi.mock('vscode', () => import('../../__mocks__/vscode.js')); @@ -35,9 +37,9 @@ describe('methods fetch ceiling (integration)', () => { const session = (): ActiveSession => ({ id: 1, gci, handle }) as unknown as ActiveSession; - const CLS = 'Issue14CeilingDemo'; + const CLS = 'SelectorScanCeilingDemo'; /** Unique enough that only the fixture's own methods can match — keeps the counts exact. */ - const TERM = 'iss14ceil'; + const TERM = 'scanceil'; const FIXTURE_METHODS = 6; // A transient fixture (rolled back by the harness's abort): FIXTURE_METHODS methods whose selectors @@ -59,7 +61,7 @@ describe('methods fetch ceiling (integration)', () => { defineFixture(); const exec = defaultQueryExecutorUsing(session()); - // Asking for fewer than the fixture holds must come back exactly full — the short-circuit. + // Asking for fewer than the fixture holds must come back exactly full — the cut-off. const bounded = searchSelectors(exec, TERM, { limit: 4, ignoreCase: true }); expect(bounded).toHaveLength(4); diff --git a/client/src/omniSearch/__tests__/methodsRelevance.integration.test.ts b/client/src/omniSearch/__tests__/methodsRelevance.integration.test.ts new file mode 100644 index 00000000..3bb96fb8 --- /dev/null +++ b/client/src/omniSearch/__tests__/methodsRelevance.integration.test.ts @@ -0,0 +1,95 @@ +// Integration test for the RELEVANCE of the Methods-scope selector scan against a live stone, over the +// release matrix (3.6.2 and 3.7.5). Base-image reflection only — no server plugin, no fixture — so it +// runs in both the bare and plugin CI passes. +// +// Regression guard for issue #517: typing `at:` returned no `Array>>at:` anywhere in the results. The +// old scan collected the first `limit` selectors CONTAINING the term, walking the symbol list in +// dictionary-hash order, and a term as common as `at:` filled that slice with `instVarAt:put:` / +// `floatAt:put:` from whichever couple of classes the walk happened to reach first. Measured on a +// 3.6.2 base image: 1142 selectors contain `at:` and only 31 ARE `at:`, so the exact implementors — +// the rows anybody typing `at:` wants — sat far past a cutoff of 80 and never reached the client. +// +// The fix ranks on the SERVER (exact selector, then prefix, then substring elsewhere), so the cut-off +// falls on the least relevant tail. That cannot be tested with fakes: it only shows up against an +// image big enough for the scan to give up, walked in an order nobody controls. Hence a live stone, +// asserting on `Array>>at:` — which every GemStone image has, on a class whose name sorts early enough +// that the tie-break puts it on the first page. +import { describe, it, expect, vi } from 'vitest'; +vi.mock('vscode', () => import('../../__mocks__/vscode.js')); + +import { useIntegrationTest } from '../../__tests__/useIntegrationTest'; +import { GciLibrary } from '../../gciLibrary'; +import { searchSelectors } from '../../queries/searchSelectors'; +import { defaultQueryExecutorUsing } from '../../browserQueries'; +import { createMethodsProvider, SERVER_OVERFETCH } from '../providers/methodsProvider'; +import { OMNI_DEFAULTS } from '../omniConfig'; +import { NEVER_CANCELLED, OmniConfig, OmniResult } from '../omniTypes'; +import type { ActiveSession } from '../../sessionManager'; + +// How far down the first page `Array>>at:` is allowed to sit. Nothing in the image fixes the exact +// index — the ranking only promises that `at:` implementors come first, and how many there are (31 on a +// 3.6.2 base image) and how their class names sort are both image facts we do not want to pin. So the +// bound is a readability claim, not a measurement: half of `maxResultsPerCategory` is "near the top of +// the page you are looking at", which is what the bug took away and what the fix has to give back. +const NEAR_TOP_OF_PAGE = OMNI_DEFAULTS.maxResultsPerCategory / 2; + +describe('methods search relevance (integration)', () => { + let gci: GciLibrary; + let handle: unknown; + useIntegrationTest((testContext) => { + gci = testContext.gciLibrary; + handle = testContext.session; + }); + + const session = (): ActiveSession => ({ id: 1, gci, handle }) as unknown as ActiveSession; + const cfg = (over: Partial = {}): OmniConfig => ({ ...OMNI_DEFAULTS, ...over }); + + it('returns the exact implementors of a common selector even though the scan is bounded', () => { + const exec = defaultQueryExecutorUsing(session()); + const limit = OMNI_DEFAULTS.maxResultsPerCategory * SERVER_OVERFETCH; + + const rows = searchSelectors(exec, 'at:', { limit, ignoreCase: true }); + + // The scan really is up against its bound — this is the condition under which the bug appeared. + expect(rows).toHaveLength(limit); + // Yet the exact implementors are what came back, `Array>>at:` among them. + expect(rows.some((r) => r.className === 'Array' && r.selector === 'at:')).toBe(true); + // And they LEAD: everything before the first non-exact row is an `at:` implementor. Assert the + // index was found first — `findIndex` answering -1 would turn the `slice` below into `slice(0, -1)` + // and pass vacuously — and then that the exact tier is more than a single row, since one leading hit + // would also be true of a scan that merely stumbled on `at:` early. + const firstInexact = rows.findIndex((r) => r.selector !== 'at:'); + expect(firstInexact).not.toBe(-1); + expect(firstInexact).toBeGreaterThan(1); + expect(rows.slice(0, firstInexact).every((r) => r.selector === 'at:')).toBe(true); + }); + + it('puts Array>>at: on the first page of the Methods results for the term `at:`', () => { + const provider = createMethodsProvider(1, (term, limit, ignoreCase) => + searchSelectors(defaultQueryExecutorUsing(session()), term, { limit, ignoreCase }), + ); + + const shown = provider.search('at:', cfg(), NEVER_CANCELLED) as OmniResult[]; + const labels = shown.map((r) => r.label); + + // The reported bug, in one assertion. + expect(labels).toContain('Array>>at:'); + // The rows are ranked, so the exact hits fill the top of the page rather than merely appearing + // somewhere in it — see NEAR_TOP_OF_PAGE for why the bound is a readability claim, not a measurement. + expect(labels.indexOf('Array>>at:')).toBeLessThan(NEAR_TOP_OF_PAGE); + expect(labels[0].endsWith('>>at:')).toBe(true); + }); + + it('still finds a selector the term only appears INSIDE, once the better tiers run out', () => { + // The tiers must not become a filter: a term with no exact and no prefix match has to fall through + // to plain substring hits, which is the only thing the old scan did. + const rows = searchSelectors(defaultQueryExecutorUsing(session()), 'VarAt:pu', { + limit: 40, + ignoreCase: true, + }); + + expect(rows.length).toBeGreaterThan(0); + expect(rows.every((r) => r.selector.toLowerCase().includes('varat:pu'))).toBe(true); + expect(rows.some((r) => r.selector === 'instVarAt:put:')).toBe(true); + }); +}); diff --git a/client/src/omniSearch/__tests__/omniEngine.test.ts b/client/src/omniSearch/__tests__/omniEngine.test.ts index 39f787af..25f2926d 100644 --- a/client/src/omniSearch/__tests__/omniEngine.test.ts +++ b/client/src/omniSearch/__tests__/omniEngine.test.ts @@ -933,3 +933,110 @@ describe('createOmniEngine — out-of-order reference and pivot results', () => expect(engine.resultFor(retyped!.rows[0].id)!.label).toBe('Foo'); }); }); + +describe('createOmniEngine — an explicit refresh vs an automatic resync', () => { + /** A provider that reloads on `reprime`, answering a different pool each time it is asked. */ + function reloadingProvider(pools: OmniResult[][]) { + let round = 0; + let pool = pools[0]; + const p: OmniProvider = { + category: CATEGORY_BY_ID.classes, + prime: () => { + pool = pools[Math.min(round, pools.length - 1)]; + }, + reprime: () => { + round += 1; + pool = pools[Math.min(round, pools.length - 1)]; + }, + search: (_q: string, c: OmniConfig) => pool.slice(0, c.maxResultsPerCategory), + }; + return p; + } + + it('reloads the corpora and re-runs the term when nothing is pivoted', async () => { + // Round 2 is what a class created by a workspace doit looks like: the same query, a bigger image. + const provider = reloadingProvider([ + [classResult('Foo')], + [classResult('Foo'), classResult('Foo2')], + ]); + const engine = createOmniEngine({ providers: [provider], config: cfg() }); + await engine.prime(); + const before = await engine.search('foo'); + expect(before!.rows.map((r) => r.label)).toEqual(['Foo']); + + const after = await engine.refresh(); + + expect(after!.rows.map((r) => r.label)).toEqual(['Foo', 'Foo2']); + }); + + it('re-fetches an open references list, instead of looking like a dead button', async () => { + // The bug this pins: `resync` deliberately leaves a pivot alone, so wiring the ⟳ to it made the + // button do nothing visible for anyone reading a senders list — the corpora reloaded silently and + // the stale senders stayed on screen. + const classes = fakeProvider('classes', [classResult('Foo')]); + let senders = [methodResult('A>>useFoo', 'useFoo')]; + const engine = createOmniEngine({ + providers: [classes], + config: cfg(), + resolveReferences: () => ({ title: 'References to Foo', results: senders }), + }); + const search = await engine.search('foo'); + const pivot = await engine.pivot(search!.rows[0].id); + expect(pivot!.rows.map((r) => r.label)).toEqual(['A>>useFoo']); + + // Someone (this session or another) compiles a second sender. + senders = [methodResult('A>>useFoo', 'useFoo'), methodResult('B>>alsoFoo', 'alsoFoo')]; + + const resynced = await engine.resync(); + expect(resynced).toBeNull(); // a commit must not disturb what the user is reading + + const refreshed = await engine.refresh(); + + expect(refreshed!.pivot).toBe(true); + expect(refreshed!.pivotTitle).toBe('References to Foo'); + expect(refreshed!.rows.map((r) => r.label)).toEqual(['A>>useFoo', 'B>>alsoFoo']); + }); + + it('keeps the filter typed into a pivot when it re-fetches', async () => { + const classes = fakeProvider('classes', [classResult('Foo')]); + let senders = [methodResult('A>>useFoo', 'useFoo'), methodResult('B>>alsoFoo', 'alsoFoo')]; + const engine = createOmniEngine({ + providers: [classes], + config: cfg(), + resolveReferences: () => ({ title: 'References to Foo', results: senders }), + }); + const search = await engine.search('foo'); + await engine.pivot(search!.rows[0].id); + const filtered = await engine.search('also'); + expect(filtered!.rows.map((r) => r.label)).toEqual(['B>>alsoFoo']); + + senders = [...senders, methodResult('C>>alsoFooToo', 'alsoFooToo')]; + const refreshed = await engine.refresh(); + + // Re-fetching must not silently widen the list back to every sender — the box still says "also". + expect(refreshed!.rows.map((r) => r.label)).toEqual(['B>>alsoFoo', 'C>>alsoFooToo']); + }); + + it('leaves the pivot when its target is gone, rather than showing senders of nothing', async () => { + const classes = fakeProvider('classes', [classResult('Foo')]); + let refView: ReferenceView | null = { + title: 'References to Foo', + results: [methodResult('A>>useFoo', 'useFoo')], + }; + const engine = createOmniEngine({ + providers: [classes], + config: cfg(), + resolveReferences: () => refView, + }); + const search = await engine.search('foo'); + await engine.pivot(search!.rows[0].id); + + refView = null; // the class or method was deleted out from under the pivot + + const refreshed = await engine.refresh(); + + expect(refreshed!.pivot).toBe(false); + expect(engine.state().pivot).toBe(false); + expect(refreshed!.rows.map((r) => r.label)).toEqual(['Foo']); // the search is back + }); +}); diff --git a/client/src/omniSearch/__tests__/omniRefreshAndReset.test.ts b/client/src/omniSearch/__tests__/omniRefreshAndReset.test.ts new file mode 100644 index 00000000..2912d6a5 --- /dev/null +++ b/client/src/omniSearch/__tests__/omniRefreshAndReset.test.ts @@ -0,0 +1,125 @@ +// @vitest-environment jsdom +/** + * The two webview behaviours issue #517 adds: + * + * - the ⟳ button, which asks the host to reload the cached corpora and re-run the current search, so + * classes and methods created (or removed) by EXECUTING code are picked up without a commit; + * - the `reset` message, which wipes the panel when the session under it changes. Everything on + * screen was read out of the session just left, and a stale row still LOOKS live — activating one + * would open a document against the session that is now current. + * + * Mounts the real chrome from `renderOmniHtml` via the shared harness, so a renamed or dropped control + * fails here rather than passing against markup the extension no longer emits. + */ +import { describe, it, expect, beforeAll, beforeEach } from 'vitest'; +import { loadOmniView, mountOmniView, MountedOmniView } from './omniViewHarness'; + +beforeAll(loadOmniView); + +const CATEGORIES = [ + { id: 'classes', label: 'Classes', explicitOnly: false }, + { id: 'methods', label: 'Methods', explicitOnly: false }, +]; + +function row(id: number, label: string) { + return { + id, + label, + ranges: [], + referenceable: false, + categoryId: 'classes', + categoryLabel: 'Class', + icon: 'symbol-class', + }; +} + +/** A panel mid-work: a typed term, results, a selected row, a references breadcrumb and an error. */ +function busyPanel(): MountedOmniView { + const mounted = mountOmniView({ categories: CATEGORIES, scopeId: null, caseSensitive: false }); + const input = document.getElementById('query') as HTMLInputElement; + input.value = 'Acc'; + mounted.view.onMessage({ + data: { + command: 'results', + rows: [row(0, 'Account'), row(1, 'AccountHolder')], + shownCount: 2, + hasMore: false, + exact: true, + truncations: [], + pivot: true, + pivotTitle: 'Senders of #foo', + pivotHint: 'Esc to go back', + categories: CATEGORIES, + scopeId: 'classes', + caseSensitive: false, + placeholder: 'Search classes…', + }, + }); + mounted.view.onMessage({ data: { command: 'error', message: 'something went wrong' } }); + mounted.posted.length = 0; + return mounted; +} + +describe('GemStone Search webview — the refresh button', () => { + let mounted: MountedOmniView; + beforeEach(() => { + mounted = mountOmniView({ categories: CATEGORIES, scopeId: null, caseSensitive: false }); + }); + + it('asks the host to refresh, and shows the panel as busy until it answers', () => { + (document.getElementById('query') as HTMLInputElement).value = 'at:'; + + (document.getElementById('refresh') as HTMLButtonElement).click(); + + expect(mounted.posted).toEqual([{ command: 'refresh' }]); + expect(document.body.classList.contains('busy')).toBe(true); + }); + + it('keeps the typed term — the point is that same search against the current image', () => { + const input = document.getElementById('query') as HTMLInputElement; + input.value = 'at:'; + + (document.getElementById('refresh') as HTMLButtonElement).click(); + + expect(input.value).toBe('at:'); + // No second query message: the host re-runs the term it already holds. + expect(mounted.posted.filter((m) => m.command === 'query')).toEqual([]); + }); +}); + +describe('GemStone Search webview — resetting on a session switch', () => { + it('clears the query, the rows, the preview, the breadcrumb and the error banner', () => { + const mounted = busyPanel(); + expect(mounted.view.rowCount()).toBe(2); + + mounted.view.onMessage({ data: { command: 'reset' } }); + + expect((document.getElementById('query') as HTMLInputElement).value).toBe(''); + expect(mounted.view.rowCount()).toBe(0); + expect(document.getElementById('results')?.textContent).toBe(''); + expect(document.getElementById('preview')?.textContent).toBe(''); + expect(document.getElementById('breadcrumb')?.textContent).toBe(''); + expect(document.getElementById('error')?.textContent).toBe(''); + expect(document.body.classList.contains('busy')).toBe(false); + }); + + it('puts the scope back to All, which is where the replacement engine starts', () => { + const mounted = busyPanel(); + const activeTab = () => document.querySelector('#tabs .tab.active')?.textContent; + expect(activeTab()).toBe('Classes'); + + mounted.view.onMessage({ data: { command: 'reset' } }); + + expect(activeTab()).toBe('All'); + }); + + it('leaves no references state behind for the next session to inherit', () => { + const mounted = busyPanel(); + const indicator = document.getElementById('refindicator') as HTMLButtonElement; + expect(indicator.style.display).not.toBe('none'); + + mounted.view.onMessage({ data: { command: 'reset' } }); + + expect(indicator.style.display).toBe('none'); + }); +}); diff --git a/client/src/omniSearch/__tests__/omniSearchCommand.test.ts b/client/src/omniSearch/__tests__/omniSearchCommand.test.ts index 492f3ad0..e2a25dee 100644 --- a/client/src/omniSearch/__tests__/omniSearchCommand.test.ts +++ b/client/src/omniSearch/__tests__/omniSearchCommand.test.ts @@ -9,7 +9,8 @@ import * as vscode from 'vscode'; import { __resetConfig, __setConfig } from '../../__mocks__/vscode'; import { logWarning } from '../../gciLog'; import { buildOmniHandlers, registerOmniSearch, revealPanelAfterLogin } from '../omniSearchCommand'; -import { OMNI_VIEW_ID } from '../omniSearchViewProvider'; +import { OMNI_VIEW_ID, OmniSearchViewProvider } from '../omniSearchViewProvider'; +import { OmniSearchPanel } from '../omniSearchPanel'; describe('buildOmniHandlers', () => { beforeEach(() => vi.clearAllMocks()); @@ -198,3 +199,52 @@ describe('registerOmniSearch: when a login reveals the panel', () => { disposable.dispose(); }); }); + +describe('registerOmniSearch: keeping the search bound to the current session', () => { + /** The same stub shape the reveal tests use, minus the reveal plumbing. */ + const registerWith = () => { + let fire: () => void = () => {}; + const sessionManager = { + getSessions: () => [{ id: 1 }, { id: 2 }], + getSelectedSession: () => ({ id: 2 }), + onDidChangeSelection: (listener: () => void) => { + fire = listener; + return { dispose: () => {} }; + }, + }; + const disposable = registerOmniSearch(sessionManager as never); + return { fire, disposable }; + }; + + beforeEach(() => vi.clearAllMocks()); + afterEach(() => __resetConfig()); + + it('tells BOTH hosts when the user makes another session active', () => { + const onView = vi + .spyOn(OmniSearchViewProvider.prototype, 'onSessionSelectionChanged') + .mockResolvedValue(undefined); + const onSpotter = vi + .spyOn(OmniSearchPanel, 'onSessionSelectionChanged') + .mockImplementation(() => {}); + const { fire, disposable } = registerWith(); + + fire(); + + // Either host can be the live one (the `ui` setting decides), and each ignores the call when it has + // nothing open — so both are always told rather than branching on the setting here. + expect(onView).toHaveBeenCalled(); + expect(onSpotter).toHaveBeenCalled(); + onView.mockRestore(); + onSpotter.mockRestore(); + disposable.dispose(); + }); + + it('registers the refresh command alongside the open command', () => { + const { disposable } = registerWith(); + + const registered = vi.mocked(vscode.commands.registerCommand).mock.calls.map((c) => c[0]); + // Contributed as the ⟳ in the panel title bar (package.json view/title) and as a palette entry. + expect(registered).toContain('gemstone.search.refresh'); + disposable.dispose(); + }); +}); diff --git a/client/src/omniSearch/__tests__/omniSearchPanel.test.ts b/client/src/omniSearch/__tests__/omniSearchPanel.test.ts index e465b587..91998490 100644 --- a/client/src/omniSearch/__tests__/omniSearchPanel.test.ts +++ b/client/src/omniSearch/__tests__/omniSearchPanel.test.ts @@ -1,29 +1,45 @@ /** - * OmniSearchPanel.show() session binding. + * OmniSearchPanel session binding. * - * The Spotter is a singleton bound to one GemStone session: its engine (and the providers, activation - * and preview inside it) are built once from that session's `deps` in the constructor. So a second - * `show()` for the SAME session must just refocus the open panel, but a `show()` for a DIFFERENT - * session must REPLACE it — a bare reveal would keep searching and opening against the previous session - * with no sign anything is wrong (the reported two-session bug). + * The Spotter is a singleton bound to one GemStone session: its engine — and the providers, activation + * and preview inside it — is built from that session's `deps`. So a second `show()` for the SAME + * session must just refocus the open panel, while anything that points it at a DIFFERENT session must + * REBIND it: a bare reveal would keep searching and opening against the previous session with no sign + * anything is wrong (the reported two-session bug), and the results left on screen would still look + * live while belonging to a session the user has left. + * + * Two ways in, both covered here: an explicit `show()` for another session, and the user making + * another session active while the Spotter sits there (`onSessionSelectionChanged`). + * + * `OmniSearchPanel.current` is a singleton, so each test opens its own panel and the `afterEach` + * disposes it — `panel.dispose()` is how VS Code closes a tab, and the panel's own `onDidDispose` + * handler is what clears the singleton. Without that the suite's shuffled order would let one test's + * live panel decide what the next one sees. * * createOmniEngine is mocked so `show()`/the constructor need no real session wiring; asserting which - * deps it was last built from is how we prove the live panel is bound to the right session. The whole - * lifecycle runs as one sequential scenario because OmniSearchPanel.current is a singleton that would - * otherwise leak between `it` blocks. + * deps it was last built from is how we prove the live panel is bound to the right session. */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { OmniPanelDeps } from '../omniSearchPanel'; vi.mock('vscode', () => import('../../__mocks__/vscode.js')); -vi.mock('../omniEngine', () => ({ createOmniEngine: vi.fn(() => ({})) })); +vi.mock('../omniEngine', () => ({ + createOmniEngine: vi.fn(() => ({ + prime: vi.fn(async () => {}), + resync: vi.fn(async () => null), + refresh: vi.fn(async () => null), + // Present so the logout test can prove a keystroke never reaches it. + search: vi.fn(async () => null), + })), +})); import * as vscode from 'vscode'; import { OmniSearchPanel } from '../omniSearchPanel'; import { createOmniEngine } from '../omniEngine'; +import { NO_SESSION_MESSAGE } from '../omniSearchShared'; -// Minimal deps: with createOmniEngine mocked, only sessionId (the identity show() compares on) and the -// shape matter; the rest are never touched on the show()/constructor path. +// Minimal deps: with createOmniEngine mocked, only sessionId (the identity the panel compares on) and +// the shape matter; the rest are never touched on the show()/rebind paths. function deps(sessionId: number): OmniPanelDeps { return { sessionId, @@ -35,35 +51,127 @@ function deps(sessionId: number): OmniPanelDeps { } as unknown as OmniPanelDeps; } -describe('OmniSearchPanel.show', () => { - it('refocuses on the same session, but replaces + rebinds the engine on a different one', () => { - const created = vi.mocked(vscode.window.createWebviewPanel); - const engine = vi.mocked(createOmniEngine); +/** Open the Spotter for `sessionId` and hand back the webview panel VS Code was asked to create. */ +function open(sessionId = 1) { + OmniSearchPanel.show(deps(sessionId)); + const created = vi.mocked(vscode.window.createWebviewPanel).mock.results; + return created[created.length - 1].value; +} + +/** The handler the panel registered for webview messages, so a test can play the user typing. */ +function messagesTo(panel: { webview: { onDidReceiveMessage: { mock: { calls: unknown[][] } } } }) { + const calls = panel.webview.onDidReceiveMessage.mock.calls; + return calls[calls.length - 1][0] as (m: unknown) => void; +} - // First open, session 1. - const depsA = deps(1); - OmniSearchPanel.show(depsA); - expect(created).toHaveBeenCalledTimes(1); - const panelA = created.mock.results[0].value; - expect(engine).toHaveBeenLastCalledWith(depsA); +describe('OmniSearchPanel session binding', () => { + let panel: ReturnType | undefined; + + beforeEach(() => { + vi.clearAllMocks(); + panel = undefined; + }); + // Closing the tab is what clears the singleton, so every test starts from no open Spotter. + afterEach(() => panel?.dispose()); + + it('refocuses the open panel when shown again for the same session', () => { + const p = (panel = open(1)); - // Second invocation, SAME session: refocus the open panel, do not open a new one. OmniSearchPanel.show(deps(1)); - expect(created).toHaveBeenCalledTimes(1); // no second panel - expect(panelA.reveal).toHaveBeenCalled(); - expect(panelA.webview.postMessage).toHaveBeenCalledWith({ command: 'focusInput' }); - expect(panelA.dispose).not.toHaveBeenCalled(); - - // Invocation for a DIFFERENT session: tear the old panel down and build a new one bound to it. - // On the buggy code show() would just reveal panelA — no dispose, no new panel, engine still bound - // to session 1 — so all three assertions below pin the fix. + + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1); // no second panel + expect(createOmniEngine).toHaveBeenCalledTimes(1); // no needless re-prime of the same session + expect(p.reveal).toHaveBeenCalled(); + expect(p.webview.postMessage).toHaveBeenCalledWith({ command: 'focusInput' }); + expect(p.dispose).not.toHaveBeenCalled(); + }); + + it('rebinds the open panel when shown for a different session', () => { + const p = (panel = open(1)); + + // On the buggy code show() just revealed the panel with its engine still bound to session 1. const depsB = deps(2); OmniSearchPanel.show(depsB); - // Called, not called-once: dispose() is the panel's own onDidDispose handler - // and also calls panel.dispose(), so disposing re-enters it once and stops on - // the panel's internal already-disposed guard. The second call is a no-op. - expect(panelA.dispose).toHaveBeenCalled(); - expect(created).toHaveBeenCalledTimes(2); - expect(engine).toHaveBeenLastCalledWith(depsB); + + expect(createOmniEngine).toHaveBeenLastCalledWith(depsB); + expect(vscode.window.createWebviewPanel).toHaveBeenCalledTimes(1); // the user's tab survives + expect(p.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + }); + + it('rebinds when the user makes another session active without touching the Spotter', () => { + const p = (panel = open(1)); + p.webview.postMessage.mockClear(); + + const depsC = deps(3); + OmniSearchPanel.onSessionSelectionChanged(() => depsC); + + // So the panel can never answer out of a session that is no longer current. + expect(createOmniEngine).toHaveBeenLastCalledWith(depsC); + expect(p.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + }); + + it('does nothing when the selection lands back on the session it is already bound to', () => { + const p = (panel = open(3)); + p.webview.postMessage.mockClear(); + const builds = vi.mocked(createOmniEngine).mock.calls.length; + + OmniSearchPanel.onSessionSelectionChanged(() => deps(3)); + + // Re-priming three image-wide GCI executes to arrive where we already are, and wiping the results + // to show the same ones again, is exactly what the session-id comparison exists to avoid. + expect(vi.mocked(createOmniEngine).mock.calls.length).toBe(builds); + expect(p.webview.postMessage).not.toHaveBeenCalled(); + }); + + it('drops the engine and says to log in when the last session logs out', () => { + const p = (panel = open(1)); + const built = vi.mocked(createOmniEngine).mock.results; + const engine = built[built.length - 1].value; + p.webview.postMessage.mockClear(); + + OmniSearchPanel.onSessionSelectionChanged(() => null); + + // Only a PINNED Spotter gets here — an unpinned one disposes on focus-out, so clicking away to log + // out closes it first. A pinned tab is one the user asked to keep, so it stays, but it is wiped and + // says so rather than showing the departed session's rows. + expect(p.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + expect(p.webview.postMessage).toHaveBeenCalledWith({ + command: 'error', + message: NO_SESSION_MESSAGE, + }); + + // And the engine goes with them. Wiping the screen alone left it holding the departed session's + // primed corpora, so the next keystroke answered with rows out of a session that is gone — and + // activating one would open a document against its dead GCI handle. + p.webview.postMessage.mockClear(); + messagesTo(p)({ command: 'query', value: 'at:' }); + expect(engine.search).not.toHaveBeenCalled(); + expect(p.webview.postMessage).toHaveBeenCalledWith({ + command: 'error', + message: NO_SESSION_MESSAGE, + }); + }); + + it('re-binds after a logout even when the user logs back into the same session id', () => { + panel = open(1); + OmniSearchPanel.onSessionSelectionChanged(() => null); + const builds = vi.mocked(createOmniEngine).mock.calls.length; + + // Comparing session ids alone would call this a no-op and leave the Spotter engine-less for good. + const again = deps(1); + OmniSearchPanel.onSessionSelectionChanged(() => again); + + expect(vi.mocked(createOmniEngine).mock.calls.length).toBe(builds + 1); + expect(createOmniEngine).toHaveBeenLastCalledWith(again); + }); + + it('builds nothing on a session change when no Spotter is open', () => { + // The docked panel is the default UI, so this hook fires on every session switch with no Spotter in + // sight; resolving deps would build a whole provider set for a panel that does not exist. + const resolve = vi.fn(() => deps(9)); + + OmniSearchPanel.onSessionSelectionChanged(resolve); + + expect(resolve).not.toHaveBeenCalled(); }); }); diff --git a/client/src/omniSearch/__tests__/omniSearchViewProvider.test.ts b/client/src/omniSearch/__tests__/omniSearchViewProvider.test.ts index 319c3a90..53bc30d8 100644 --- a/client/src/omniSearch/__tests__/omniSearchViewProvider.test.ts +++ b/client/src/omniSearch/__tests__/omniSearchViewProvider.test.ts @@ -8,6 +8,7 @@ vi.mock('../omniEngine', () => ({ prime: vi.fn(async () => {}), applyChange: vi.fn(async () => null), resync: vi.fn(async () => null), + refresh: vi.fn(async () => null), search: vi.fn(async () => null), state: () => ({ scopeId: null, caseSensitive: false }), })), @@ -20,7 +21,7 @@ import { REVEAL_DEADLINE_MS, } from '../omniSearchViewProvider'; -function fakeContext(): OmniViewContext { +function fakeContext(sessionId = 1): OmniViewContext { const config = { matchMode: 'fuzzy', caseSensitive: false, @@ -31,9 +32,15 @@ function fakeContext(): OmniViewContext { referencesInPreview: false, }; // The engine is mocked, so only `config` is read here; cast past the unused `OmniPanelDeps` members. - return { deps: { config, onError: vi.fn() }, sessionId: 1 } as unknown as OmniViewContext; + return { deps: { config, onError: vi.fn() }, sessionId } as unknown as OmniViewContext; } +/** The provider registers its webview callback as `void this.onMessage(m)`, so awaiting a message + * returns the moment the handler suspends, not when it finishes. Yield a macrotask to let it run to + * the end — otherwise an in-flight `ready` handler picks up work the test has not posted yet, or the + * assertion runs before the handler's last `postMessage`. */ +const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); + function fakeView(visible: boolean) { const on = { message: (_m: unknown) => Promise.resolve(), visibility: () => {} }; const view = { @@ -161,11 +168,6 @@ describe('GemStone Search docked panel — reacting to image changes', () => { describe('GemStone Search docked panel — a session sync while hidden', () => { beforeEach(() => vi.clearAllMocks()); - // The provider's webview callback is `void this.onMessage(m)`, so awaiting `on.message(...)` returns - // the moment the handler suspends, not when it finishes. Yield a macrotask to let it run to the end — - // otherwise an in-flight `ready` handler picks up a sync this test hasn't posted yet. - const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); - async function openThenHide() { const provider = new OmniSearchViewProvider(vi.fn(async () => fakeContext())); const { view, on } = fakeView(true); @@ -271,3 +273,216 @@ describe('GemStone Search docked panel — reporting whether a reveal landed', ( await expect(landed).resolves.toBe(true); }); }); + +describe('GemStone Search docked panel — switching the active session', () => { + beforeEach(() => vi.clearAllMocks()); + + /** Open the panel on session 1, with a resolver whose session id we can move afterwards. */ + async function openWithSwitchableSession(visible: boolean) { + let sessionId = 1; + const provider = new OmniSearchViewProvider(vi.fn(async () => fakeContext(sessionId))); + const { view, on } = fakeView(visible); + provider.resolveWebviewView(view as never); + await on.message({ command: 'ready' }); + expect(createOmniEngine).toHaveBeenCalledTimes(1); + return { provider, view, on, select: (id: number) => (sessionId = id) }; + } + + it('wipes the webview and rebinds the engine when another session is made active', async () => { + const { provider, view, select } = await openWithSwitchableSession(true); + select(2); + + await provider.onSessionSelectionChanged(); + + // The rows on screen came out of session 1 — leaving them up would show stale results that still + // look live, and activating one would open a document against session 2. + expect(view.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + expect(createOmniEngine).toHaveBeenCalledTimes(2); + }); + + it('does nothing when the selection lands back on the session it is already built for', async () => { + const { provider, view, select } = await openWithSwitchableSession(true); + select(1); // same session — e.g. re-selecting it in the Sessions tree + view.webview.postMessage.mockClear(); + + await provider.onSessionSelectionChanged(); + + // Re-priming would cost three image-wide GCI executes to arrive where we already are. + expect(createOmniEngine).toHaveBeenCalledTimes(1); + expect(view.webview.postMessage).not.toHaveBeenCalledWith({ command: 'reset' }); + }); + + it('wipes a HIDDEN panel too, and leaves the rebuild for the next reveal', async () => { + const { provider, view, on, select } = await openWithSwitchableSession(false); + select(2); + + await provider.onSessionSelectionChanged(); + + // The wipe is not deferred: unlike a stale corpus, stale ROWS are visible the instant the panel is + // revealed, and the reveal cannot un-show them retroactively. + expect(view.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + expect(createOmniEngine).toHaveBeenCalledTimes(1); // the costly part still waits + await on.message({ command: 'ready' }); + expect(createOmniEngine).toHaveBeenCalledTimes(2); + }); + + it('resets and asks for a login when the last session logs out', async () => { + let ctx: OmniViewContext | null = fakeContext(1); + const provider = new OmniSearchViewProvider(vi.fn(async () => ctx)); + const { view, on } = fakeView(true); + provider.resolveWebviewView(view as never); + await on.message({ command: 'ready' }); + ctx = null; + + await provider.onSessionSelectionChanged(); + + expect(view.webview.postMessage).toHaveBeenCalledWith({ command: 'reset' }); + expect(view.webview.postMessage).toHaveBeenCalledWith({ + command: 'error', + message: 'Log in to a GemStone session to search.', + }); + }); +}); + +describe('GemStone Search docked panel — the refresh button', () => { + beforeEach(() => vi.clearAllMocks()); + + async function open(visible = true) { + const provider = new OmniSearchViewProvider(vi.fn(async () => fakeContext())); + const { view, on } = fakeView(visible); + provider.resolveWebviewView(view as never); + void on.message({ command: 'ready' }); + // The handler is fire-and-forget (`void this.onMessage(m)`), so wait for the engine it builds AND + // let the rest of the handler drain — otherwise its own `flushPendingSync` lands mid-test and looks + // like the code under test resyncing. + await vi.waitFor(() => expect(createOmniEngine).toHaveBeenCalled()); + await settle(); + // The LAST engine built, not `results[0]`: `vi.clearAllMocks()` does not empty `mock.results`, so an + // earlier test's engine can still be sitting at index 0. + const results = vi.mocked(createOmniEngine).mock.results; + return { provider, view, on, engine: results[results.length - 1].value }; + } + + it('reloads every cached corpus, so code created by executing it is picked up', async () => { + const { provider, engine } = await open(); + + await provider.refresh(); + + // `refresh`, not `resync`: the two differ over an open references list — see the engine tests. + expect(engine.refresh).toHaveBeenCalled(); + expect(engine.resync).not.toHaveBeenCalled(); + }); + + it('drops the busy indicator when a newer call superseded the refresh', async () => { + const { view, on, engine } = await open(); + engine.refresh.mockResolvedValueOnce(null); + + // The provider's message handler is registered as `void this.onMessage(m)`, so the webview message + // is fire-and-forget — hence waitFor rather than a bare await, here and below. + void on.message({ command: 'refresh' }); + + await vi.waitFor(() => expect(engine.refresh).toHaveBeenCalled()); + await vi.waitFor(() => + expect(view.webview.postMessage).toHaveBeenCalledWith({ command: 'busy', on: false }), + ); + }); + + it('reloads ONCE when a hidden sync was also outstanding', async () => { + const { provider, on, engine } = await open(false); + await provider.onSessionSynced(1); // deferred: the panel is hidden + expect(engine.resync).not.toHaveBeenCalled(); + + void on.message({ command: 'refresh' }); + + // Both want the same rebuild; paying for it twice is two image-wide walks for one click. + await vi.waitFor(() => expect(engine.refresh).toHaveBeenCalledTimes(1)); + expect(engine.resync).not.toHaveBeenCalled(); + }); + + it('builds nothing when the view has never been instantiated', async () => { + // `gemstone.search.refresh` reaches both hosts, so it fires even when the Spotter is the chosen UI + // and this view was never resolved. Priming an engine there would cost three image-wide executes + // for a panel nobody opened. + const resolveContext = vi.fn(async () => fakeContext()); + const provider = new OmniSearchViewProvider(resolveContext); + + await provider.refresh(); + + expect(resolveContext).not.toHaveBeenCalled(); + expect(createOmniEngine).not.toHaveBeenCalled(); + }); + + it('defers the command to the next reveal when the panel is collapsed', async () => { + // Collapsing the panel disposes the view, so reloading now would pay three image-wide executes to + // post results to a webview nobody is looking at — the same bargain every other catch-up path here + // makes. But the request must not be silently dropped, or the panel the user reopens is the stale + // one they just asked to refresh. + const { provider, view, on, engine } = await open(true); + view.visible = false; + + await provider.refresh(); + expect(engine.refresh).not.toHaveBeenCalled(); + + view.visible = true; + on.visibility(); + + await vi.waitFor(() => expect(engine.refresh).toHaveBeenCalledTimes(1)); + expect(engine.resync).not.toHaveBeenCalled(); // the refresh subsumes any deferred sync + }); + + it('still reloads on the webview button while the view reports itself hidden', async () => { + // A message from the webview is proof enough that someone is looking, so the ⟳ inside the chrome + // skips the visibility gate the palette command honours. + const { on, engine } = await open(false); + + void on.message({ command: 'refresh' }); + + await vi.waitFor(() => expect(engine.refresh).toHaveBeenCalledTimes(1)); + }); + + it('takes the spinner off when the reload throws', async () => { + // The palette command and the title-bar button call `refresh()` as a bare `void`, so a rejection — + // resolving senders of a common selector against a busy session, say — used to go unhandled and + // leave the panel faded for good. + const { provider, view, engine } = await open(); + engine.refresh.mockRejectedValueOnce(new Error('session busy')); + + await provider.refresh(); + + expect(view.webview.postMessage).toHaveBeenCalledWith({ + command: 'error', + message: 'session busy', + }); + expect(view.webview.postMessage).toHaveBeenCalledWith({ command: 'busy', on: false }); + }); +}); + +describe('GemStone Search docked panel — reopening the view', () => { + beforeEach(() => vi.clearAllMocks()); + + it('re-sends the config to the fresh webview a reopen creates', async () => { + // Collapsing the panel disposes the view; reopening it hands us a brand-new webview with an empty + // tab row, no case flag and a zero debounce. The engine that outlives it still matches the session, + // so `ensureEngine` has nothing to rebuild — and therefore used to push nothing, leaving the fresh + // webview to limp until the first search happened to refill its chrome. + const provider = new OmniSearchViewProvider(vi.fn(async () => fakeContext())); + const first = fakeView(true); + provider.resolveWebviewView(first.view as never); + void first.on.message({ command: 'ready' }); + await vi.waitFor(() => expect(createOmniEngine).toHaveBeenCalled()); + await settle(); + + const reopened = fakeView(true); + provider.resolveWebviewView(reopened.view as never); + void reopened.on.message({ command: 'ready' }); + + await vi.waitFor(() => + expect( + reopened.view.webview.postMessage.mock.calls.some( + (c) => (c[0] as { command?: string }).command === 'config', + ), + ).toBe(true), + ); + expect(createOmniEngine).toHaveBeenCalledTimes(1); // and without paying for a rebuild + }); +}); diff --git a/client/src/omniSearch/__tests__/omniViewHarness.ts b/client/src/omniSearch/__tests__/omniViewHarness.ts index 532fdb84..2f1f741c 100644 --- a/client/src/omniSearch/__tests__/omniViewHarness.ts +++ b/client/src/omniSearch/__tests__/omniViewHarness.ts @@ -18,8 +18,10 @@ import { renderOmniHtml } from '../omniSearchShared'; /** Every method the webview's `wire()` returns that these tests reach into. */ export interface WiredOmniView { renderResults: (view: unknown) => void; + renderTabs: (categories: unknown, scopeId: string | null) => void; onMessage: (event: { data: unknown }) => void; setActive: (i: number, scroll?: boolean) => void; + rowCount: () => number; previewEnabled: () => boolean; scopeMenuOpen: () => boolean; excludedFromAll: () => string[]; diff --git a/client/src/omniSearch/omniConfig.ts b/client/src/omniSearch/omniConfig.ts index 464f4728..7d7d0a5c 100644 --- a/client/src/omniSearch/omniConfig.ts +++ b/client/src/omniSearch/omniConfig.ts @@ -27,11 +27,11 @@ export const OMNI_DEFAULTS: OmniConfig = { // methods to promote); 1-char selector scans across the whole image are too heavy, so they stay // off. Raise this via settings if per-keystroke method search feels slow on a large stone. methodMinQueryLength: 2, - // How many matches a scope's server-side scan collects before it gives up. The Methods scan walks - // every selector of every class in the symbol list, so it stops early to keep a per-keystroke search - // fast — 200 is a working-set size, not a total. It bounds the RESULTS too: no display cap, Load-all - // included, can reach past it, which is why the footer says so out loud when a scan stops here - // (triage #14). Raise it to see more of a broad term at the cost of a slower search. + // The most rows a scope's server-side scan hands back. The Methods scan walks every selector of every + // class in the symbol list and keeps the best matches by tier, so 200 is a working-set size, not a + // total. It bounds the RESULTS: no display cap, Load-all included, can reach past it, which is why + // the footer says so out loud when a scan is cut off here. Raise it to see more of a broad term at + // the cost of a bigger fetch. maxServerScan: 200, // Try the sticky preview-pane references list by default; flip off to restore the classic pivot. referencesInPreview: true, diff --git a/client/src/omniSearch/omniEngine.ts b/client/src/omniSearch/omniEngine.ts index 08330ba7..74811411 100644 --- a/client/src/omniSearch/omniEngine.ts +++ b/client/src/omniSearch/omniEngine.ts @@ -289,8 +289,14 @@ export interface OmniEngine { * matches the term and its category is in scope); returns null otherwise, leaving the view as-is. */ applyChange(change: OmniCorpusChange): Promise; /** Drop + rebuild every cached corpus (on a session sync — commit/abort), catching changes from - * outside this UI, then re-run the current term. Returns null while a pivot is showing. */ + * outside this UI, then re-run the current term. Returns null while a pivot is showing: a commit is + * not a request to disturb what the user is reading. */ resync(onError?: (message: string) => void): Promise; + /** The USER asked for fresh state (the ⟳ button / `gemstone.search.refresh`). Rebuilds every cached + * corpus like `resync`, but where `resync` leaves a pivot alone this RE-FETCHES it from the stone — + * a references list is exactly as stale as the corpora, and a refresh that visibly did nothing reads + * as a broken button. Returns null only when a newer call superseded this one. */ + refresh(onError?: (message: string) => void): Promise; /** Run the search for a raw field value and return the view (or null if superseded by a newer * call). In the pivot, this filters the loaded reference rows client-side instead. */ search(rawValue: string): Promise; @@ -346,6 +352,13 @@ export function createOmniEngine(deps: OmniEngineDeps): OmniEngine { let excludedFromAll = new Set(config.excludedFromAll); // When non-null, the list shows the references/senders of a result (a "pivot"), not a live search. let pivot: ReferenceView | null = null; + // The row the pivot was taken FROM, kept so an explicit refresh can ask the stone for its references + // again. Without it a refresh could only rebuild the corpora and leave the pivot's rows as they were. + let pivotSource: OmniResult | null = null; + // What is typed in the box while pivoted. In the pivot the field filters the loaded reference rows + // rather than searching, so this is NOT `lastRawValue` (which holds the search to restore on the way + // out) — and a refresh has to re-apply it, or re-fetching would silently widen the list. + let pivotFilter = ''; // The results backing the CURRENT view, indexed by row id. let current: OmniResult[] = []; // The reference rows from the last `referencesFor`, indexed by the preview list's row id. Kept @@ -403,11 +416,26 @@ export function createOmniEngine(deps: OmniEngineDeps): OmniEngine { ); } + /** Drop + reload every cached corpus. A provider that throws just keeps the cache it had — a partial + * reload is better than none, and the error is surfaced rather than swallowed. */ + async function reprimeAll(onError?: (message: string) => void): Promise { + await Promise.all( + providers.map(async (p) => { + try { + await (p.reprime ?? p.prime)?.(NEVER_CANCELLED); + } catch (e: unknown) { + onError?.(e instanceof Error ? e.message : String(e)); + } + }), + ); + } + async function runSearch(rawValue: string): Promise { // In the reference view, typing filters the loaded rows client-side (no provider fan-out); don't // touch `lastRawValue` (it holds the search to restore when the pivot is dismissed). if (pivot) { - current = filterPivot(pivot.results, rawValue.trim()); + pivotFilter = rawValue.trim(); + current = filterPivot(pivot.results, pivotFilter); return pivotView(); } // A new search supersedes any reference load already in flight: otherwise its rows would land in @@ -514,19 +542,32 @@ export function createOmniEngine(deps: OmniEngineDeps): OmniEngine { return null; }, async resync(onError) { - await Promise.all( - providers.map(async (p) => { - try { - await (p.reprime ?? p.prime)?.(NEVER_CANCELLED); - } catch (e: unknown) { - onError?.(e instanceof Error ? e.message : String(e)); - } - }), - ); + await reprimeAll(onError); // Don't disturb a pivot; otherwise re-run the current term against the rebuilt corpora. if (pivot) return null; return runSearch(lastRawValue); }, + async refresh(onError) { + await reprimeAll(onError); + if (!pivot || !pivotSource || !deps.resolveReferences) return runSearch(lastRawValue); + // Re-ask the stone who references the row this pivot was taken from. Superseding in-flight work + // the way `pivot` does, and for the same reason: resolving senders of a common selector is slow, + // and anything the user does meanwhile must win. + const gen = ++generation; + const view = await deps.resolveReferences(pivotSource); + if (gen !== generation) return null; + if (!view) { + // The row is no longer referenceable — its method or class was deleted out from under us. Leave + // the pivot rather than keep showing a list of senders of something that is gone. + pivot = null; + pivotSource = null; + pivotFilter = ''; + return runSearch(lastRawValue); + } + pivot = view; + current = filterPivot(view.results, pivotFilter); + return pivotView(); + }, search: (rawValue) => runSearch(rawValue), async setScope(newScope) { scopeId = newScope; @@ -538,6 +579,8 @@ export function createOmniEngine(deps: OmniEngineDeps): OmniEngine { // filter that was never applied — and the scope then took effect invisibly on the way out, // narrowing a restored search the user never asked to narrow. pivot = null; + pivotSource = null; + pivotFilter = ''; return runSearch(lastRawValue); }, async toggleCase() { @@ -572,12 +615,16 @@ export function createOmniEngine(deps: OmniEngineDeps): OmniEngine { if (gen !== generation) return null; // a newer call superseded this pivot if (!view) return null; // not referenceable — leave the current list as-is pivot = view; + pivotSource = result; + pivotFilter = ''; current = view.results; return pivotView(); }, async exitPivot() { if (!pivot) return null; pivot = null; + pivotSource = null; + pivotFilter = ''; return runSearch(lastRawValue); }, async setExcludedFromAll(ids) { diff --git a/client/src/omniSearch/omniSearchCommand.ts b/client/src/omniSearch/omniSearchCommand.ts index c4046672..4d615109 100644 --- a/client/src/omniSearch/omniSearchCommand.ts +++ b/client/src/omniSearch/omniSearchCommand.ts @@ -42,7 +42,7 @@ import { createLiteralsProvider } from './providers/literalsProvider'; import { createCategoriesProvider } from './providers/categoriesProvider'; import { ReferenceView } from './omniEngine'; import { referenceRequestFor, methodRowsToResults } from './references'; -import { OmniSearchPanel } from './omniSearchPanel'; +import { OmniSearchPanel, OmniPanelDeps } from './omniSearchPanel'; import { OmniSearchViewProvider, OmniViewContext, OMNI_VIEW_ID } from './omniSearchViewProvider'; /** Where a result should open. The docked panel view passes nothing (open in the active group — the @@ -219,9 +219,7 @@ export function buildViewContextResolver( sessionManager: SessionManager, ): () => Promise { return async () => { - const sessions = sessionManager.getSessions(); - const session = - sessionManager.getSelectedSession() ?? (sessions.length === 1 ? sessions[0] : undefined); + const session = currentSession(sessionManager); if (!session) return null; const config = readOmniConfig(vscode.workspace.getConfiguration('gemstone.omniSearch')); return { @@ -248,6 +246,43 @@ export function buildViewContextResolver( }; } +/** The editor-tab Spotter's session-bound deps. Built both when the Spotter is opened and when the + * selected session changes under an open one, so the two paths cannot drift. */ +export function buildSpotterDeps(session: ActiveSession): OmniPanelDeps { + const config = readOmniConfig(vscode.workspace.getConfiguration('gemstone.omniSearch')); + return { + sessionId: session.id, + providers: buildProviders(session, config.enabledCategories), + config, + resolveReferences: resolveReferencesUsing(session), + // When pinned the Spotter stays open, so results open BESIDE it as a regular (non-preview) + // source editor (preserveFocus keeps you in the field for Ctrl+Enter); unpinned it behaves like + // the dialog and opens in the active group (a preview tab is fine — the dialog dismisses). + activate: (result, opts) => + runOmniAction( + result.action, + buildOmniHandlers( + opts.beside + ? { + viewColumn: vscode.ViewColumn.Beside, + preserveFocus: opts.preserveFocus, + preview: false, + } + : undefined, + ), + ), + previewSource: buildPreviewSource(session), + onError: (message) => vscode.window.showErrorMessage(`GemStone Search: ${message}`), + }; +} + +/** The session both hosts search: the selected one, or the sole session when nothing is selected yet. + * Never prompts — it answers what IS current, for code reacting to a change rather than opening a UI. */ +function currentSession(sessionManager: SessionManager): ActiveSession | undefined { + const sessions = sessionManager.getSessions(); + return sessionManager.getSelectedSession() ?? (sessions.length === 1 ? sessions[0] : undefined); +} + export async function runOmniSearch( sessionManager: SessionManager, viewProvider?: OmniSearchViewProvider, @@ -258,34 +293,7 @@ export async function runOmniSearch( if (ui === 'spotter') { const session = await sessionManager.resolveSession(); if (!session) return; - - const config = readOmniConfig(vscode.workspace.getConfiguration('gemstone.omniSearch')); - const providers = buildProviders(session, config.enabledCategories); - - OmniSearchPanel.show({ - sessionId: session.id, - providers, - config, - resolveReferences: resolveReferencesUsing(session), - // When pinned the Spotter stays open, so results open BESIDE it as a regular (non-preview) - // source editor (preserveFocus keeps you in the field for Ctrl+Enter); unpinned it behaves like - // the dialog and opens in the active group (a preview tab is fine — the dialog dismisses). - activate: (result, opts) => - runOmniAction( - result.action, - buildOmniHandlers( - opts.beside - ? { - viewColumn: vscode.ViewColumn.Beside, - preserveFocus: opts.preserveFocus, - preview: false, - } - : undefined, - ), - ), - previewSource: buildPreviewSource(session), - onError: (message) => vscode.window.showErrorMessage(`GemStone Search: ${message}`), - }); + OmniSearchPanel.show(buildSpotterDeps(session)); return; } @@ -388,6 +396,15 @@ export function registerOmniSearch( const nowActive = sessionManager.getSessions().length > 0; syncStatus(); maybeTip(); + // Both hosts hold an engine bound to ONE session, and a webview full of that session's results. + // Re-point (or, with nothing left to search, blank) them so a search never answers out of the + // session the user just left (issue #517). The panel view resolves its own session; the Spotter + // is handed a thunk so nothing is built when no Spotter is open. + void viewProvider.onSessionSelectionChanged(); + OmniSearchPanel.onSessionSelectionChanged(() => { + const session = currentSession(sessionManager); + return session ? buildSpotterDeps(session) : null; + }); if (nowActive && !hadSession) { const ui = vscode.workspace .getConfiguration('gemstone.omniSearch') @@ -418,6 +435,12 @@ export function registerOmniSearch( vscode.commands.registerCommand('gemstone.search', () => runOmniSearch(sessionManager, viewProvider), ), + // The ⟳ in the panel title bar, and a palette entry. Sent to BOTH hosts: only one is in use for a + // given `ui` setting, and each no-ops when it has nothing open, so there is nothing to branch on. + vscode.commands.registerCommand('gemstone.search.refresh', async () => { + await viewProvider.refresh(); + OmniSearchPanel.refresh(); + }), ); // Both class hooks fold the same way — re-fetch just that class name and reconcile the corpus. diff --git a/client/src/omniSearch/omniSearchPanel.ts b/client/src/omniSearch/omniSearchPanel.ts index 78a8ba49..833cfe58 100644 --- a/client/src/omniSearch/omniSearchPanel.ts +++ b/client/src/omniSearch/omniSearchPanel.ts @@ -19,6 +19,7 @@ import { revealTestForResult } from './omniActions'; import { configMessage, dispatchEngineMessage, + NO_SESSION_MESSAGE, renderOmniHtml, resultsMessage, } from './omniSearchShared'; @@ -56,12 +57,17 @@ type OmniInbound = | { command: 'openReference'; refId: number } | { command: 'back' } | { command: 'preview'; id: number } + | { command: 'refresh' } | { command: 'close' }; export class OmniSearchPanel { private static current: OmniSearchPanel | undefined; private readonly panel: vscode.WebviewPanel; - private readonly engine: ReturnType; + // Not readonly, and not always there: `rebindTo` replaces it when the Spotter is pointed at another + // session, and `clearSession` drops it when the last session logs out. Undefined IS the gate that + // stops `onMessage` answering out of a session that no longer exists — the Spotter's equivalent of + // the docked provider's `ensureEngine` check. + private engine: ReturnType | undefined; private disposables: vscode.Disposable[] = []; // Dialog vs. pinned. Unpinned (default) makes the Spotter behave like the Phase-1 QuickPick: it // closes on focus-out and on picking a result. Pinned keeps it open and switches activation to @@ -73,22 +79,20 @@ export class OmniSearchPanel { private hasBeenActive = false; /** Open (or reveal) the Spotter. Only one exists at a time. A second invocation for the SAME - * session just refocuses it; one for a DIFFERENT session replaces it, because a Spotter is bound - * to its session (see below) and can't be re-pointed in place. */ + * session just refocuses it; one for a DIFFERENT session re-points it at that session's deps. */ static show(deps: OmniPanelDeps): void { const existing = OmniSearchPanel.current; if (existing) { - if (existing.deps.sessionId === deps.sessionId) { - // Same session: bring the open Spotter forward and refocus its field. - existing.panel.reveal(existing.panel.viewColumn); - existing.panel.webview.postMessage({ command: 'focusInput' }); - return; + if (existing.deps.sessionId !== deps.sessionId) { + // Different session: the open Spotter's engine — and the providers, activation and preview + // inside it — came from the OLD session's `deps`, so a bare reveal would keep searching and + // opening against the previous session with no sign anything is wrong (the reported + // two-session bug). Re-point it, which also clears the old session's results off the screen. + existing.rebindTo(deps); } - // Different session: the open Spotter's engine (and its providers, activation and preview) was - // built once from the OLD session's `deps` in the constructor and can't be re-pointed here, so a - // bare reveal would keep searching and opening against the previous session with no sign - // anything is wrong. Replace it with a Spotter bound to the new session's deps. - existing.dispose(); + existing.panel.reveal(existing.panel.viewColumn); + existing.panel.webview.postMessage({ command: 'focusInput' }); + return; } const panel = vscode.window.createWebviewPanel( 'gemstoneOmniSearch', @@ -104,7 +108,7 @@ export class OmniSearchPanel { * pinned one — an unpinned Spotter has already closed on focus-out by the time you compile). */ static onClassCompiled(sessionId: number, className: string, dictName?: string): void { const panel = OmniSearchPanel.current; - if (!panel || panel.deps.sessionId !== sessionId) return; + if (!panel?.engine || panel.deps.sessionId !== sessionId) return; void panel.engine .applyChange({ kind: 'class', className, dictName }) .then((view) => view && panel.postView(view)); @@ -113,10 +117,44 @@ export class OmniSearchPanel { /** Rebuild the open Spotter's cached corpora on a session sync (commit/abort), then redraw. */ static onSessionSynced(sessionId: number): void { const panel = OmniSearchPanel.current; - if (!panel || panel.deps.sessionId !== sessionId) return; + if (!panel?.engine || panel.deps.sessionId !== sessionId) return; void panel.engine.resync(panel.deps.onError).then((view) => view && panel.postView(view)); } + /** The user made a different session active. An open Spotter is bound to ONE session — its + * providers, activation and preview all close over it — so it has to be re-pointed at the new one, + * and everything on screen (results, query, pivot, preview) has to go with it: those rows were read + * out of the session just left, yet activating one would open a document against the session that is + * now current (issue #517). + * + * `resolveDeps` is a thunk so nothing is built when no Spotter is open. It answering null means + * there is nothing left to search (the last session logged out). Reaching that branch at all means a + * PINNED Spotter: an unpinned one is a dialog that disposes on focus-out, so clicking away to log out + * closes it first and `current` is already undefined. A pinned tab is one the user asked to keep, so + * it stays — but it is cleared right down to its engine, rather than showing a departed session's + * results. */ + static onSessionSelectionChanged(resolveDeps: () => OmniPanelDeps | null): void { + const panel = OmniSearchPanel.current; + if (!panel) return; // nothing open — don't build providers for a Spotter that isn't there + const deps = resolveDeps(); + if (!deps) { + panel.clearSession(); + return; + } + // The engine, not just the id: a logout drops the engine, and logging back in can hand back the + // SAME session id — comparing ids alone would call that a no-op and leave the Spotter engine-less + // for good. + if (panel.engine && panel.deps.sessionId === deps.sessionId) return; + panel.rebindTo(deps); + } + + /** The user asked for a refresh (⟳ in the panel / `gemstone.search.refresh`): rebuild every cached + * corpus from the stone and re-run the current search, so code created or removed by EXECUTING it + * (a workspace doit, a `compileMethod:`) is picked up without waiting for a commit or abort. */ + static refresh(): void { + void OmniSearchPanel.current?.refresh(); + } + private constructor( panel: vscode.WebviewPanel, private deps: OmniPanelDeps, @@ -171,8 +209,61 @@ export class OmniSearchPanel { this.panel.webview.postMessage({ command: 'pinned', pinned: this.pinned }); } + /** Point this Spotter at another session: a new engine over the new deps, a cleared webview, and a + * fresh prime. Everything else about the panel — the tab, its pin, the loaded HTML and the message + * wiring — is session-independent and stays, so the user keeps the tab they opened. */ + private rebindTo(deps: OmniPanelDeps): void { + this.deps = deps; + this.engine = createOmniEngine(deps); + this.reset(); + this.panel.webview.postMessage(configMessage(deps.config, this.pinned)); + void this.engine.prime(deps.onError); + } + + /** Clear the webview back to an empty search (no query, no results, no pivot, no preview). */ + private reset(): void { + this.panel.webview.postMessage({ command: 'reset' }); + } + + /** The last session went away, with a pinned Spotter still on screen (see onSessionSelectionChanged + * for why it must be pinned to get here). Wiping the screen is not enough: the engine still holds that + * session's primed corpora, and `deps.activate` still closes over its GCI handle, so anything the user + * typed next would answer with rows out of a session that is gone and open documents against it — the + * very failure this hook exists to prevent, just reached through a logout instead of a switch. So the + * engine goes too, and `onMessage` shows the notice until a login rebinds us. */ + private clearSession(): void { + this.engine = undefined; + this.reset(); + this.panel.webview.postMessage({ command: 'error', message: NO_SESSION_MESSAGE }); + } + + /** Reload every cached corpus and re-run the current term. See the static `refresh` for why. */ + private async refresh(): Promise { + const engine = this.engine; + if (!engine) return; // logged out: nothing to reload, and the notice is already up + this.panel.webview.postMessage({ command: 'busy', on: true }); + let view: OmniViewData | null = null; + try { + // `refresh`, not `resync`: an open references list is as stale as the corpora, so it is re-fetched + // rather than left alone the way a commit leaves it. + view = await engine.refresh(this.deps.onError); + } catch (e: unknown) { + // The palette command and the title-bar ⟳ reach `refresh()` with no catch of their own (see the + // static `refresh`), so without this a rejection — `resolveReferences` against a busy session, say + // — would go unhandled and strand the panel faded forever. + const message = e instanceof Error ? e.message : String(e); + this.deps.onError?.(message); + this.panel.webview.postMessage({ command: 'error', message }); + } + // A view takes the spinner off by replacing the results. Without one — superseded by a newer call, + // or the throw above — it has to come off explicitly, or the panel stays faded for good. + if (view) this.postView(view); + else this.panel.webview.postMessage({ command: 'busy', on: false }); + } + /** Send a fresh view to the webview, decorated with the current chrome state. */ private postView(view: OmniViewData): void { + if (!this.engine) return; const st = this.engine.state(); this.panel.webview.postMessage( resultsMessage(view, { @@ -187,8 +278,16 @@ export class OmniSearchPanel { } private async onMessage(m: OmniInbound): Promise { + const engine = this.engine; + if (!engine) { + // Logged out (see clearSession), but the tab is still here, so keystrokes still arrive. Answer + // with the notice rather than searching a departed session's corpora. `close` still means close. + if (m.command === 'close') this.panel.dispose(); + else this.panel.webview.postMessage({ command: 'error', message: NO_SESSION_MESSAGE }); + return; + } try { - const engineOp = dispatchEngineMessage(this.engine, m); + const engineOp = dispatchEngineMessage(engine, m); if (engineOp) { await this.run(() => engineOp); return; @@ -196,7 +295,7 @@ export class OmniSearchPanel { switch (m.command) { case 'ready': { this.panel.webview.postMessage(configMessage(this.deps.config, this.pinned)); - await this.engine.prime(this.deps.onError); + await engine.prime(this.deps.onError); return; } case 'togglePin': @@ -208,7 +307,7 @@ export class OmniSearchPanel { ); return; case 'activate': { - const result = this.engine.resultFor(m.id); + const result = engine.resultFor(m.id); if (!result) return; // Unpinned = dialog: open in the active group and dismiss the Spotter (Phase-1 feel). // Pinned = persistent: open BESIDE, and Ctrl+Enter (side) keeps focus in the field. @@ -222,12 +321,12 @@ export class OmniSearchPanel { case 'revealTest': { // Shift+Enter: go to the result in the Testing view instead of opening it. // The Spotter stays put — you are moving to another view, not dismissing this one. - const result = this.engine.resultFor(m.id); + const result = engine.resultFor(m.id); if (result) await revealTestForResult(result); return; } case 'preview': { - const result = this.engine.resultFor(m.id); + const result = engine.resultFor(m.id); if (!result) return; let source = ''; try { @@ -246,7 +345,7 @@ export class OmniSearchPanel { case 'referencesInline': { // Load a row's senders/references into the sticky preview-pane list (leaves the search list // and its state untouched). `forId` lets the webview drop a stale reply if the row moved on. - const preview = await this.engine.referencesFor(m.id); + const preview = await engine.referencesFor(m.id); if (preview) { this.panel.webview.postMessage({ command: 'refPreview', @@ -260,7 +359,7 @@ export class OmniSearchPanel { } case 'previewReference': { // Source of a single reference row, for the inline (EI Meta-tab style) expand in the list. - const result = this.engine.referenceResultFor(m.refId); + const result = engine.referenceResultFor(m.refId); let source = ''; if (result) { try { @@ -273,13 +372,16 @@ export class OmniSearchPanel { return; } case 'openReference': { - const result = this.engine.referenceResultFor(m.refId); + const result = engine.referenceResultFor(m.refId); if (!result) return; // Opening source from the refs list must NOT dismiss the Spotter — open beside it and keep // focus in the field so the sticky list stays put for the next pick. await this.deps.activate(result, { beside: true, preserveFocus: true }); return; } + case 'refresh': + await this.refresh(); + return; case 'close': this.panel.dispose(); return; diff --git a/client/src/omniSearch/omniSearchShared.ts b/client/src/omniSearch/omniSearchShared.ts index d102942f..5dcf22a8 100644 --- a/client/src/omniSearch/omniSearchShared.ts +++ b/client/src/omniSearch/omniSearchShared.ts @@ -32,6 +32,10 @@ export const REFERENCES_KEY_HINT_HTML = IS_MAC */ export const OMNI_OPEN_KEY_HINT = IS_MAC ? '⌘⇧A' : 'Ctrl+Shift+A'; +/** Shown in both hosts when there is no session to search — on first open before any login, and after + * the last session logs out. Shared so the two hosts cannot drift into saying it differently. */ +export const NO_SESSION_MESSAGE = 'Log in to a GemStone session to search.'; + /** Scope-name lookup for the placeholder. */ const SCOPE_LABEL: Record = { all: 'everything', @@ -514,7 +518,7 @@ export function renderOmniHtml(opts: { showPin: boolean }): string { /* Two controls over what a search COSTS: the preview-pane toggle and the All-scope filter. Kept in their own block (and shaped to match the case chip rather than editing its rule) so this stays an additive hunk. No backticks anywhere in here - this is a template literal. */ - #previewToggle, #scopeFilter, #matchMode { + #previewToggle, #scopeFilter, #matchMode, #refresh { flex: 0 0 auto; padding: 5px 9px; border: 1px solid var(--vscode-input-border, transparent); @@ -531,6 +535,11 @@ export function renderOmniHtml(opts: { showPin: boolean }): string { border-color: var(--vscode-button-background); opacity: 1; } + /* An action, not a mode: it has no on/off state, and while the reload is out to the stone the whole + body carries .busy, which fades it with the rest of the chrome. No backticks in here -- this + stylesheet lives inside a template literal. */ + #refresh { font-size: 1.05em; line-height: 1; } + #refresh:hover { color: var(--vscode-foreground); opacity: 1; } /* The algorithm chip always shows its CURRENT value as text, so it needs no on/off state. */ #matchMode { background: var(--vscode-button-background); @@ -591,6 +600,7 @@ export function renderOmniHtml(opts: { showPin: boolean }): string { + diff --git a/client/src/omniSearch/omniSearchView.js b/client/src/omniSearch/omniSearchView.js index 1f30bf61..8836a18f 100644 --- a/client/src/omniSearch/omniSearchView.js +++ b/client/src/omniSearch/omniSearchView.js @@ -42,6 +42,7 @@ var scopeFilterMenuEl = doc.getElementById('scopeFilterMenu'); var matchModeEl = doc.getElementById('matchMode'); var refIndicatorEl = doc.getElementById('refindicator'); + var refreshEl = doc.getElementById('refresh'); // The last category list + active scope pushed from the host. The host owns scope; we just reflect // what it sent. Kept so the scope hint can name the scopes an All-scope search leaves out (see // updateScopeHint) and so Tab / Shift+Tab can cycle scopes from the field. @@ -634,6 +635,9 @@ case 'busy': setBusy(!!msg.on); break; + case 'reset': + resetView(); + break; case 'preview': // Ignore a stale preview for a row that's no longer active, or one that arrives while the // pane is showing a sticky references list (referencesInPreview mode). @@ -904,6 +908,42 @@ highlightOccurrences(pre, source, inputEl.value.trim(), caseSensitive); } + /** + * Wipe the panel back to an empty search: no query, no results, no references list, no preview, no + * error banner, scope back to All. + * + * Sent by the host as `reset` when the session under the search changes, or the last session logs + * out. Everything on screen was read out of the session just left, and a stale row still LOOKS + * live — activating one would open a document against the session that is now current — so an empty + * panel is the honest state until the new session's engine has primed (issue #517). + */ + function resetView() { + cancelPendingQuery(); + if (previewTimer) { + clearTimeout(previewTimer); + previewTimer = null; + } + inputEl.value = ''; + updateClearVisibility(); + setError(''); + setBreadcrumb('', ''); + refHighlightTerm = ''; + scrollResetPending = true; + // Scope belongs to the engine, and the replacement engine starts at All — so reflect that rather + // than leaving the departed session's tab lit. + renderTabs(lastCategories, null); + // Clears the list, the preview, the footer and the references chip in one pass. + renderResults({ + rows: [], + shownCount: 0, + hasMore: false, + exact: false, + truncations: [], + pivot: false, + }); + setBusy(false); + } + function setBusy(on) { if (on) doc.body.classList.add('busy'); else doc.body.classList.remove('busy'); @@ -1173,6 +1213,18 @@ inputEl.focus(); }); + if (refreshEl) { + // An explicit reload of the cached corpora (classes / dictionaries / globals) plus a re-run of + // whatever is in the field. Deliberately keeps the query: you press this BECAUSE you want the + // same search answered against the current state of the image. The host clears the busy flag by + // sending the fresh results — or, if a newer call superseded this one, a bare `busy: false`. + refreshEl.addEventListener('click', function () { + setBusy(true); + post('refresh'); + inputEl.focus(); + }); + } + if (previewToggleEl) { previewToggleEl.addEventListener('click', function () { setPreviewEnabled(!previewEnabled); diff --git a/client/src/omniSearch/omniSearchViewProvider.ts b/client/src/omniSearch/omniSearchViewProvider.ts index 26520f21..37f7fa89 100644 --- a/client/src/omniSearch/omniSearchViewProvider.ts +++ b/client/src/omniSearch/omniSearchViewProvider.ts @@ -12,13 +12,16 @@ * The cached engine also captures the `gemstone.omniSearch` settings that were live when it was built, * so the command layer calls `onConfigChanged()` (from an `onDidChangeConfiguration` listener) to drop * the engine when those settings change — otherwise a settings edit made while the panel is open would - * be silently ignored until the session changed or the window reloaded. (Rebinding on a live SESSION - * switch — the `sessionMode: "multiple"` case — is deferred to #437.) + * be silently ignored until the session changed or the window reloaded. Switching the SELECTED session + * calls `onSessionSelectionChanged()` for the same reason, and additionally wipes the webview: the + * engine is rebuilt lazily either way, but until it is, everything on screen — query, results, pivot, + * preview — belongs to a session the user has left (issue #517). * - * Both catch-ups are gated on the view being VISIBLE, because the engine outlives a hidden panel and + * Every catch-up is gated on the view being VISIBLE, because the engine outlives a hidden panel and * reloading its corpora costs image-wide synchronous GCI executes. A hidden panel therefore only notes - * that it is out of date (a dropped engine for config, `syncPending` for a commit/abort) and pays for - * it on the next reveal or webview message — never on the hidden path itself. + * that it is out of date — a dropped engine for config, `syncPending` for a commit/abort, + * `refreshPending` for an explicit refresh — and pays for it on the next reveal or webview message, + * never on the hidden path itself. */ import * as vscode from 'vscode'; import { createOmniEngine, OmniEngine, OmniViewData } from './omniEngine'; @@ -28,6 +31,7 @@ import { CommonInbound, configMessage, dispatchEngineMessage, + NO_SESSION_MESSAGE, renderOmniHtml, resultsMessage, } from './omniSearchShared'; @@ -61,6 +65,18 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { // reloaded them yet (see onSessionSynced). Cleared by flushPendingSync, and whenever the engine is // dropped or rebuilt — a fresh engine primes its corpora, so there is nothing left to catch up on. private syncPending = false; + // A `gemstone.search.refresh` that arrived while the panel was collapsed. Same bargain as syncPending + // — don't pay three image-wide GCI executes to redraw a view nobody can see — but a refresh is the + // stronger debt: it also re-fetches an open references list, so when both are outstanding this one + // wins. Cleared by the flush, and whenever the engine is dropped or rebuilt (a fresh engine primes + // every corpus, which is the reload this flag was owed). + private refreshPending = false; + // A newly resolved webview starts with none of the chrome state — no scope tabs, no case flag, no + // debounce. `ensureEngine` pushes the config when it BUILDS an engine, which covers the first open + // but not a reopen: collapsing the panel disposes the view, and the engine that outlives it still + // matches the session, so ensureEngine short-circuits and the fresh webview is never told anything. + // This flag makes the `ready` handler responsible for the push when the engine did not do it. + private webviewNeedsConfig = false; // Fires when the workbench has actually instantiated the view. `focus()` waits on this rather than // inspecting `this.view`, which only reports whether the view has EVER been built (it is set once in // `resolveWebviewView` and never cleared) and so cannot tell a landed reveal from a lost one. @@ -70,6 +86,7 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { resolveWebviewView(view: vscode.WebviewView): void { this.view = view; + this.webviewNeedsConfig = true; // brand-new webview, whether this is the first open or a reopen view.webview.options = { enableScripts: true, localResourceRoots: [] }; view.webview.html = renderOmniHtml({ showPin: false }); view.webview.onDidReceiveMessage((m: PanelInbound) => void this.onMessage(m)); @@ -89,9 +106,92 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { this.deps = undefined; this.builtForSession = undefined; this.syncPending = false; // the replacement engine primes from scratch + this.refreshPending = false; if (this.view?.visible) void this.ensureEngine(); } + /** The user made a different session active (`SessionManager.onDidChangeSelection`). The engine is + * bound to a session, so it has to go — but unlike a config change this also invalidates everything + * the webview is showing: the results, the query that produced them, any references pivot and the + * previewed source were all read out of the session just left. Leaving them on screen is worse than + * showing nothing, because the rows still look live and activating one opens a document against the + * session that is now current. So the webview is reset as well as the engine. + * + * Logging out of the last session lands here too (there is no context to resolve): the panel resets + * and, when visible, says to log in — rather than keeping the departed session's results. + * + * A no-op when the selection lands back on the session we already built for — re-priming three + * image-wide GCI executes to arrive where we already are is exactly what `builtForSession` exists to + * avoid. */ + async onSessionSelectionChanged(): Promise { + const ctx = await this.resolveContext(); + if (this.engine && this.builtForSession !== ctx?.sessionId) { + this.engine = undefined; + this.deps = undefined; + this.builtForSession = undefined; + this.syncPending = false; // a replacement engine primes from scratch + this.refreshPending = false; + this.post({ command: 'reset' }); + } + // Hidden: the next reveal or webview message builds the engine for the now-current session. + if (this.view?.visible) await this.ensureEngine(); + } + + /** The user asked for a refresh (the panel's ⟳ button / `gemstone.search.refresh`): reload every + * cached corpus from the stone and re-run the current search. + * + * This is the only way to pick up work done by EXECUTING code — a class created or removed from a + * workspace, a method compiled by evaluating `compileMethod:`, a new global. Those changes announce + * nothing the panel can listen for, so short of a commit or abort (which do trigger a resync) the + * cached corpora stay stale, and the staleness window has no upper bound. An explicit refresh closes + * it on demand (issue #517). + * + * It clears any pending hidden sync, since a full reload is strictly more than that sync owed. And it + * calls the engine's `refresh` rather than its `resync`, which is what makes it re-fetch an open + * references list instead of leaving it stale (see the engine). */ + async refresh(): Promise { + // Never instantiated: there is nothing on screen to refresh, and building an engine here would pay + // three image-wide GCI executes for a panel the user has not opened (the `gemstone.search.refresh` + // command reaches both hosts, so this fires even when the Spotter is the chosen UI). + if (!this.view) return; + // Collapsed: the view is disposed, so the reload would pay those same three executes to post its + // results to a webview nobody is looking at — the cost every other catch-up path in this file gates + // on `visible` to avoid. The request is not dropped, though: note it and pay on the next reveal, so + // the panel the user comes back to is the fresh one they asked for. + if (!this.view.visible) { + this.refreshPending = true; + return; + } + await this.reload(); + } + + /** The reload itself, with no visibility gate. Reached from `refresh()` once the panel is known to be + * on screen, from the flush when a collapsed panel reopens, and from the webview's own ⟳ button — + * which is proof enough on its own that someone is looking. */ + private async reload(): Promise { + if (!(await this.ensureEngine())) return; + this.syncPending = false; + this.refreshPending = false; + this.post({ command: 'busy', on: true }); + let view: OmniViewData | null = null; + try { + // `refresh`, not `resync`: a references list is exactly as stale as the corpora, so an explicit + // refresh re-fetches it rather than leaving it alone the way a commit does. + view = await this.engine!.refresh(this.deps?.onError); + } catch (e: unknown) { + // `refresh()` is called as a bare `void` from the palette command and the title-bar button, so + // without this a rejection — `resolveReferences` against a busy session, say — would go unhandled + // and strand the panel faded. + const message = e instanceof Error ? e.message : String(e); + this.deps?.onError?.(message); + this.post({ command: 'error', message }); + } + // A view takes the spinner off by replacing the results. Without one — superseded by a newer call, + // or the throw above — it has to come off explicitly, or the panel stays faded for good. + if (view) this.postView(view); + else this.post({ command: 'busy', on: false }); + } + /** A class was compiled locally in `sessionId`: fold it into the live engine's cache (a cheap * single-class lookup, no full reload) and redraw only if it affects the current results. No-op * unless we have an engine built for that same session. */ @@ -116,10 +216,22 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { if (this.view?.visible) await this.flushPendingSync(); } - /** Reveal-time catch-up: make sure the engine matches the current session, then pay for any sync we + /** Reveal-time catch-up: make sure the engine matches the current session, then pay for whatever we * skipped while hidden. */ private async onShown(): Promise { - if (await this.ensureEngine()) await this.flushPendingSync(); + if (await this.ensureEngine()) await this.flushDeferred(); + } + + /** Pay for whatever a hidden panel deferred. A pending refresh subsumes a pending sync — it rebuilds + * every corpus and re-fetches the references list on top — so it wins and the sync is dropped, rather + * than the two paying for the same image-wide walk twice. */ + private async flushDeferred(): Promise { + if (this.refreshPending) { + this.syncPending = false; + await this.reload(); + return; + } + await this.flushPendingSync(); } /** Rebuild the corpora a hidden sync left stale, then redraw the current search. No-op when nothing @@ -187,7 +299,8 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { this.deps = undefined; this.builtForSession = undefined; this.syncPending = false; - this.post({ command: 'error', message: 'Log in to a GemStone session to search.' }); + this.refreshPending = false; + this.post({ command: 'error', message: NO_SESSION_MESSAGE }); return false; } if (this.engine && this.builtForSession === ctx.sessionId) return true; @@ -197,6 +310,7 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { this.syncPending = false; // a brand-new engine primes below, so there is no stale corpus to catch up this.post({ command: 'error', message: '' }); // clear any prior "log in" notice this.post(configMessage(ctx.deps.config, false)); + this.webviewNeedsConfig = false; // just pushed it await this.engine.prime(ctx.deps.onError); return true; } @@ -220,11 +334,24 @@ export class OmniSearchViewProvider implements vscode.WebviewViewProvider { try { if (m.command === 'ready') { await this.ensureEngine(); - await this.flushPendingSync(); + // A reopen reaches here with an engine that ensureEngine had no reason to rebuild, so the + // config it would have pushed never went out. Push it now, or the fresh webview runs with an + // empty tab row and a zero debounce until the first search happens to refill them. + if (this.webviewNeedsConfig && this.deps) { + this.webviewNeedsConfig = false; + this.post(configMessage(this.deps.config, false)); + } + await this.flushDeferred(); this.deliverFocus(); // a focus() that raced the webview load lands the cursor now return; } if (!this.engine && !(await this.ensureEngine())) return; + // Handled BEFORE the deferred-sync flush: a refresh already rebuilds every corpus, so letting the + // flush run first would pay for two full re-primes back to back. + if (m.command === 'refresh') { + await this.reload(); + return; + } // Searching stale corpora would show deleted classes / miss new ones: pay the deferred rebuild. await this.flushPendingSync(); const engine = this.engine!; diff --git a/client/src/omniSearch/omniTypes.ts b/client/src/omniSearch/omniTypes.ts index 519d1aa8..f7cf7ec2 100644 --- a/client/src/omniSearch/omniTypes.ts +++ b/client/src/omniSearch/omniTypes.ts @@ -145,12 +145,12 @@ export const NEVER_CANCELLED: OmniCancel = { isCancelled: false }; * A provider's report that its OWN fetch ceiling — not the display cap — bounded the results, so the * row count is a floor rather than a total. * - * Only providers with a server-side scan bound have one (today: methods, whose scan short-circuits at - * `maxServerScan`); the rest scan exhaustively and cap client-side, so they never report. The + * Only providers with a server-side scan bound have one (today: methods, whose scan hands back at most + * `maxServerScan` rows); the rest scan exhaustively and cap client-side, so they never report. The * engine cannot infer this: the display cap is its own number, and a count below the ceiling proves * nothing once the client-side re-filter has dropped rows. Without the report the footer claims an * exact total at the very moment the results were cut off, and the UI says nothing about the wall the - * user just hit (triage #14). + * user just hit. * * It carries the scope and the number so the UI can name both ("Methods stopped after 200") rather * than show an anonymous warning — and so any provider that gains a ceiling later is covered without @@ -205,8 +205,9 @@ export interface OmniProvider { /** Optional one-time load when the picker opens (load-once providers cache their corpus here). */ prime?(token: OmniCancel): Promise | void; /** Rebuild a cached corpus from scratch (drop + reload). No-op for per-query providers; used on a - * session sync (commit/abort) when changes from outside this UI — including other sessions — may - * have landed. Defaults to `prime` when a provider doesn't override it. */ + * session sync (commit/abort) and on an explicit user refresh, when changes from outside this UI — + * including other sessions, and anything done by executing code — may have landed. Defaults to + * `prime` when a provider doesn't override it. */ reprime?(token: OmniCancel): Promise | void; /** Fold a single known local change into the cached corpus without a full reload. Returns true if * the corpus actually changed (a new or removed name), so the caller can decide whether to diff --git a/client/src/omniSearch/providers/categoriesProvider.ts b/client/src/omniSearch/providers/categoriesProvider.ts index 92143ab4..6d1d287b 100644 --- a/client/src/omniSearch/providers/categoriesProvider.ts +++ b/client/src/omniSearch/providers/categoriesProvider.ts @@ -8,7 +8,8 @@ * `gemstone.explorer.revealCategory`) — a precise category reveal, not just landing in the dictionary. * * Staleness: the category set is derived from classes, so a class compile can introduce a brand-new - * category. Both `reprime` (session sync) and `applyChange` (a class compile) just drop the lazy + * category. Both `reprime` (a session sync or an explicit refresh) and `applyChange` (a class + * compile) just drop the lazy * cache, so the next Categories search re-scans — cheap, since the scan only reruns when that scope * is actually used again. */ diff --git a/client/src/omniSearch/providers/classesProvider.ts b/client/src/omniSearch/providers/classesProvider.ts index 3c2de0f6..46292354 100644 --- a/client/src/omniSearch/providers/classesProvider.ts +++ b/client/src/omniSearch/providers/classesProvider.ts @@ -2,7 +2,8 @@ * Classes provider: enumerate every class once when the picker opens (reusing the same * `getAllClassNames` corpus `Find Class` uses), then match client-side on each keystroke. * - * The cached corpus is kept fresh two ways: a full `reprime` (drop + reload) on a session sync, and a + * The cached corpus is kept fresh two ways: a full `reprime` (drop + reload) on a session sync or an + * explicit user refresh, and a * lightweight `applyChange` that re-fetches just that one class (via `lookupClassEntries`) and * reconciles it, so a change shows up in search without re-enumerating the whole image. The granular * path covers a local compile AND a removal (Explorer → Remove Class, notified per class): the diff --git a/client/src/omniSearch/providers/methodsProvider.ts b/client/src/omniSearch/providers/methodsProvider.ts index 5a410907..edb3bf64 100644 --- a/client/src/omniSearch/providers/methodsProvider.ts +++ b/client/src/omniSearch/providers/methodsProvider.ts @@ -1,8 +1,9 @@ /** * Methods provider: the selector space is too large to preload, so this queries the stone per * search term (the controller debounces, and we skip terms shorter than `methodMinQueryLength` to - * avoid hammering the stone). The server pre-filters by selector substring; we re-rank client-side - * with the configured matcher for a consistent order. + * avoid hammering the stone). The server pre-filters by selector substring and returns its rows + * best-tier-first (exact selector, then prefix, then substring elsewhere — see `searchSelectors`); we + * re-rank client-side with the configured matcher for a consistent order. * * The row label is `Class>>selector` (or `Class class>>selector` for the class side); the match is * computed against the selector and the highlight ranges are shifted into label coordinates. @@ -26,7 +27,8 @@ export type SelectorSearchRunner = ( ) => SelectorSearchResult[]; /** Over-fetch factor: request this many × the displayed cap from the server, so ranking has a - * wider pool to pick the best matches from (see search()). */ + * wider pool to pick the best matches from — a tie-break by class name (below) can only be A→Z over + * the rows it was given (see search()). */ export const SERVER_OVERFETCH = 4; function labelFor(r: SelectorSearchResult): string { return `${r.className}${r.isMeta ? ' class' : ''}>>${r.selector}`; @@ -47,21 +49,22 @@ export function createMethodsProvider( const term = query.trim(); if (term.length < cfg.methodMinQueryLength) return []; - // Fetch a WIDER server slice than we display, then rank + cap client-side. The server scan is - // substring-match in symbol-list order (not by relevance), so a high-quality selector match - // could sit past a tight cutoff and never reach us; over-fetching lets the ranking surface it. + // Fetch a WIDER server slice than we display, then rank + cap client-side: the slice is what the + // A→Z tie-break at the bottom gets to order, so a tight one would show the first few classes the + // scan reached rather than the first few alphabetically. // The `gemstone.omniSearch.maxServerScan` setting bounds the server slice, and so the scan cost, // regardless of maxResultsPerCategory. `readOmniConfig` has already clamped it into 20–20 000, so - // it is read straight. It bounds the RESULTS as well as the cost: `searchSelectors` short-circuits - // the instant it has that many matches, so a broad term can never return more no matter how far - // the display cap is raised — "Load all" included. `search` therefore reports truncation to the - // engine, which would otherwise present a cut-off count as an exact total, with nothing on screen - // saying the scan gave up. + // it is read straight. It bounds the RESULTS as well as the cost: `searchSelectors` yields at most + // that many rows, so a broad term can never return more no matter how far the display cap is + // raised — "Load all" included. `search` therefore reports truncation to the engine, which would + // otherwise present a cut-off count as an exact total, with nothing on screen saying the scan + // stopped short. What the cut-off can no longer do is hide the BEST matches: the server orders + // its rows by match tier, so what a full slice drops is the least relevant tail (issue #517). const ceiling = cfg.maxServerScan; const serverLimit = Math.min(cfg.maxResultsPerCategory * SERVER_OVERFETCH, ceiling); const rows = runSearch(term, serverLimit, !cfg.caseSensitive); - // A FULL slice means the scan short-circuited, so the image almost certainly holds matches we - // never saw: the count is a floor, not a total. Judged on the RAW row count, before the re-filter + // A FULL slice means the scan had rows it could not hand back, so the image almost certainly + // holds matches we never saw: the count is a floor, not a total. Judged on the RAW row count, before the re-filter // below — that drops rows and would mask the fact that we stopped early. When the image happens // to hold exactly `serverLimit` matches this over-reports by claiming "more exist"; telling the // two apart would cost another fetch, so we stay conservative. diff --git a/client/src/queries/__tests__/searchSelectors.test.ts b/client/src/queries/__tests__/searchSelectors.test.ts index 339e395e..1060191b 100644 --- a/client/src/queries/__tests__/searchSelectors.test.ts +++ b/client/src/queries/__tests__/searchSelectors.test.ts @@ -17,18 +17,47 @@ describe('buildSelectorSearchCode', () => { it('folds case on both sides when ignoreCase is true', () => { const code = buildSelectorSearchCode('at', { limit: 10, ignoreCase: true }); expect(code).toContain("'at' asLowercase"); - expect(code).toContain('sel asString asLowercase includesString:'); + expect(code).toContain('text := sel asString asLowercase'); + expect(code).toContain('text includesString: needle'); }); it('compares as-is when ignoreCase is false', () => { const code = buildSelectorSearchCode('at', { limit: 10, ignoreCase: false }); expect(code).not.toContain('asLowercase'); - expect(code).toContain('sel asString includesString:'); + expect(code).toContain('text := sel asString'); }); - it('bounds the scan with an early return at the limit', () => { + it('caps every tier at the limit', () => { const code = buildSelectorSearchCode('at', { limit: 25, ignoreCase: true }); - expect(code).toContain('count >= 25 ifTrue: [^ws contents]'); + expect(code).toContain('exactCount < 25'); + expect(code).toContain('prefixCount < 25'); + expect(code).toContain('otherCount < 25'); + }); + + it('returns the tiers best-first, so a truncated slice keeps the strongest matches', () => { + const code = buildSelectorSearchCode('at', { limit: 10, ignoreCase: true }); + expect(code).toContain('exact contents, prefixed contents, other contents'); + }); + + it('leaves the scan early ONLY when the exact tier is full', () => { + // A full exact tier is the one state where nothing later can change the answer, so it is the only + // safe short-circuit: the old "stop at the first `limit` matches" is what let a common term fill + // its slice with whatever the walk reached first and never reach the exact hit. + const code = buildSelectorSearchCode('at', { limit: 25, ignoreCase: true }); + expect(code).toContain('exactCount >= 25 ifTrue: [^exact contents]'); + expect(code).not.toContain('prefixCount >= 25 ifTrue:'); + expect(code).not.toContain('otherCount >= 25 ifTrue:'); + }); + + it('never compares strings with =, which the Utf8-compiled source makes an error', () => { + // `GciLibrary.execute` compiles our source as Utf8, so a literal is a Utf8 and `sel asString` is a + // String; `String = Utf8` raises ArgumentError 2718 and takes the whole search down with it. The + // tier test therefore uses sizes + includesString: only. Guard against a "clearer" rewrite. + const code = buildSelectorSearchCode('at', { limit: 10, ignoreCase: true }); + expect(code).not.toMatch(/text\s*=\s*needle/); // the exact-tier test + expect(code).not.toMatch(/\)\s*=\s*needle/); // the prefix-tier test + expect(code).toContain('text size = needle size'); + expect(code).toContain('(text copyFrom: 1 to: needle size) includesString: needle'); }); it('honors a non-default environmentId', () => { @@ -74,6 +103,14 @@ describe('parseSelectorSearchResults', () => { }); describe('searchSelectors', () => { + it('hands back at most `limit` rows, keeping the best tiers', () => { + // The server caps each TIER at `limit`, so it can answer up to 3 × limit rows; the rows arrive + // best-tier-first, so the caller's `limit` is applied by keeping the FIRST of them. + const raw = Array.from({ length: 7 }, (_, i) => `Globals\tC${i}\t0\tat:\taccessing\n`).join(''); + const rows = searchSelectors(() => raw, 'at:', { limit: 3, ignoreCase: true }); + expect(rows.map((r) => r.className)).toEqual(['C0', 'C1', 'C2']); + }); + it('runs the built code through the executor and parses the result', () => { const execute = vi.fn(() => 'Globals\tArray\t0\tsize\taccessing\n'); const rows = searchSelectors(execute, 'siz', { limit: 5, ignoreCase: true }); diff --git a/client/src/queries/searchSelectors.ts b/client/src/queries/searchSelectors.ts index fc66198d..1ed000d7 100644 --- a/client/src/queries/searchSelectors.ts +++ b/client/src/queries/searchSelectors.ts @@ -10,12 +10,41 @@ import { escapeString } from './util'; * Result rows are tab-separated `dictNameclassNameisMeta(0|1)selectorcategory`, * matching the shape `methodSearch.ts` already uses, so the client parse is identical. * - * Bounded on purpose: the scan stops (`^ws contents`) as soon as `limit` matches are collected, so - * a common term returns quickly instead of walking every selector in the image. A smarter global - * selector index is a documented follow-up; this bounded scan is the basic implementation. + * RANKED, not first-come. The scan collects matches into three tiers — the selector EQUALS the term, + * the selector STARTS WITH it, the term appears somewhere else in it — and returns them in that + * order, each tier capped at `limit`. That ordering is the whole point: the walk visits the symbol + * list in dictionary-hash order, which has nothing to do with relevance, so a scan that simply + * stopped at the first `limit` matches answered whichever classes it happened to reach first. A + * search for `at:` filled its slice with `instVarAt:put:`, `floatAt:put:` and friends from two + * incidental classes and never reached `Array>>at:` — the one row anybody typing `at:` wants + * (issue #517). Tiering costs nothing extra to compute and makes the cut-off fall on the least + * relevant rows instead of the last-visited ones. + * + * What it costs: the walk now runs to the end of the symbol list instead of quitting at the first + * `limit` matches, because a better-tier match can appear anywhere in it. That was already the price + * of any precise term (a term with fewer than `limit` matches never short-circuited), and measured on + * a 3.6.2 base image (671 behaviours) a full walk for `at:` is ~27 ms against ~2 ms for the old + * early exit. The one early exit still worth taking is a FULL EXACT tier: once `limit` selectors + * equal the term, every row we would return is already an exact match and nothing later can displace + * one, so the scan returns right there. + * + * Memory is bounded to 3 × `limit` rows on the server; `searchSelectors` then hands the caller at + * most `limit` of them, best tier first — so `limit` still means what it always did (the most rows a + * scan yields), and a caller that gets exactly `limit` still cannot tell a complete answer from a cut + * off one, which is why `methodsProvider` reports truncation at that point. * * 3.6.2 discipline: uses `includesString:` (not `includesSubstring:`, which DNUs on 3.6.2) and * `asLowercase` folding; all generated source is ASCII. + * + * NO STRING `=` ANYWHERE, and that is not stylistic. `GciLibrary.execute` compiles our source with + * the `Utf8` class as its string type, so every literal in this code — the needle included — is a + * `Utf8`, while `sel asString` answers a `String`; comparing the two raises + * `ArgumentError 2718, Unicode argument disallowed in String comparison` and the whole search fails. + * `includesString:` is happy across the two classes, so the tier test is expressed with it and with + * integer size comparisons: equal sizes plus a substring hit means the strings are equal, and a hit + * inside the first `needle size` characters means the term is a prefix. (Should a non-ASCII term ever + * make the two classes disagree about `size`, the only consequence is a row landing in a + * neighbouring tier — the tier decides ORDER, never whether a row matches.) */ export interface SelectorSearchResult { dictName: string; @@ -26,7 +55,7 @@ export interface SelectorSearchResult { } export interface SelectorSearchOptions { - /** Max rows to collect before the scan short-circuits. */ + /** Max rows to collect per tier on the server, and the most rows `searchSelectors` returns. */ limit: number; /** When true (the default for omni), fold case on both sides before comparing. */ ignoreCase: boolean; @@ -41,11 +70,18 @@ export function buildSelectorSearchCode(term: string, opts: SelectorSearchOption ? `'${escapeString(term)}' asLowercase` : `'${escapeString(term)}'`; const selText = opts.ignoreCase ? 'sel asString asLowercase' : 'sel asString'; - // A `^ws contents` inside the innermost block is a non-local return from the doit — it exits all - // the loops at once the moment the limit is reached. - return `| ws sl count needle | -ws := WriteStream on: String new. -count := 0. + // `^exact contents` inside the innermost block is a non-local return from the doit — it exits all + // the loops at once the moment the exact tier is full (see the note above on why that is the only + // safe early exit). The tier test runs before the row is built, so the per-row category lookup is + // only paid for a row we are actually keeping. `copyFrom: 1 to: needle size` is always in range: + // the enclosing `includesString:` already proved the term fits inside the selector. + return `| exact prefixed other exactCount prefixCount otherCount sl needle | +exact := WriteStream on: String new. +prefixed := WriteStream on: String new. +other := WriteStream on: String new. +exactCount := 0. +prefixCount := 0. +otherCount := 0. needle := ${needle}. sl := System myUserProfile symbolList. 1 to: sl size do: [:idx | | dict | @@ -54,16 +90,27 @@ sl := System myUserProfile symbolList. v isBehavior ifTrue: [ #(false true) do: [:meta | | cls | cls := meta ifTrue: [v class] ifFalse: [v]. - cls selectors do: [:sel | - (${selText} includesString: needle) ifTrue: [ - ws nextPutAll: dict name; tab; - nextPutAll: k; tab; - nextPutAll: (meta ifTrue: ['1'] ifFalse: ['0']); tab; - nextPutAll: sel asString; tab; - nextPutAll: ((cls categoryOfSelector: sel environmentId: ${envId}) ifNil: ['']); lf. - count := count + 1. - count >= ${limit} ifTrue: [^ws contents]]]]]]]. -ws contents`; + cls selectors do: [:sel | | text ws | + text := ${selText}. + (text includesString: needle) ifTrue: [ + ws := nil. + text size = needle size + ifTrue: [ + exactCount < ${limit} ifTrue: [ws := exact. exactCount := exactCount + 1]] + ifFalse: [ + ((text copyFrom: 1 to: needle size) includesString: needle) + ifTrue: [ + prefixCount < ${limit} ifTrue: [ws := prefixed. prefixCount := prefixCount + 1]] + ifFalse: [ + otherCount < ${limit} ifTrue: [ws := other. otherCount := otherCount + 1]]]. + ws == nil ifFalse: [ + ws nextPutAll: dict name; tab; + nextPutAll: k; tab; + nextPutAll: (meta ifTrue: ['1'] ifFalse: ['0']); tab; + nextPutAll: sel asString; tab; + nextPutAll: ((cls categoryOfSelector: sel environmentId: ${envId}) ifNil: ['']); lf]. + exactCount >= ${limit} ifTrue: [^exact contents]]]]]]]. +exact contents, prefixed contents, other contents`; } export function parseSelectorSearchResults(raw: string): SelectorSearchResult[] { @@ -88,5 +135,9 @@ export function searchSelectors( term: string, opts: SelectorSearchOptions, ): SelectorSearchResult[] { - return parseSelectorSearchResults(execute(buildSelectorSearchCode(term, opts))); + // The server caps each TIER at `limit`, so up to 3 × `limit` rows can come back. Keep the first + // `limit` of them: the rows arrive best tier first, so this drops the least relevant matches rather + // than the last-visited ones, and `limit` keeps its documented meaning for every caller. + const limit = Math.max(1, Math.trunc(opts.limit)); + return parseSelectorSearchResults(execute(buildSelectorSearchCode(term, opts))).slice(0, limit); } diff --git a/package.json b/package.json index 203deac4..8d373806 100644 --- a/package.json +++ b/package.json @@ -236,7 +236,7 @@ "default": 200, "minimum": 20, "maximum": 20000, - "markdownDescription": "How many matches the **Methods** search collects from the stone before it stops looking.\n\nThat search walks every selector of every class in your symbol list, so it gives up early to keep searching-as-you-type fast. This is a limit on the *results*, not just on the time: when a search stops here, no amount of **Load more** or **Load all** can reach the matches beyond it — the footer says `Methods scan capped at 200` when that happens.\n\nRaise it to cast a wider net on broad terms (`add`, `at:`), at the cost of a slower search on every keystroke. Narrowing your search term is usually faster than raising this." + "markdownDescription": "How many matching methods the **Methods** search brings back from the stone.\n\nThat search walks every selector of every class in your symbol list and keeps the best matches — an exact selector first, then ones starting with your term, then ones merely containing it — so what this limit cuts off is the least relevant tail. It is a limit on the *results*: when a search is cut off here, no amount of **Load more** or **Load all** can reach the matches beyond it — the footer says `Methods scan capped at 200` when that happens.\n\nRaise it to cast a wider net on broad terms (`add`, `at:`), at the cost of a bigger fetch on every keystroke. Narrowing your search term is usually faster than raising this." }, "gemstone.omniSearch.referencesInPreview": { "type": "boolean", @@ -1194,6 +1194,12 @@ "category": "GemStone", "icon": "$(search-fuzzy)" }, + { + "command": "gemstone.search.refresh", + "title": "Refresh GemStone Search", + "category": "GemStone", + "icon": "$(refresh)" + }, { "command": "gemstone.findMethodInClass", "title": "Find Method in Class…", @@ -2313,6 +2319,11 @@ } ], "view/title": [ + { + "command": "gemstone.search.refresh", + "when": "view == gemstoneOmniSearchView", + "group": "navigation@0" + }, { "command": "gemstone.breakpoints.enableAll", "when": "view == gemstoneBreakpoints",