From ea9bfe1be55f9fab01d768bd378dc35d5b9fc460 Mon Sep 17 00:00:00 2001 From: Binit Varghese Date: Sun, 23 Aug 2026 16:54:53 +0530 Subject: [PATCH 1/4] fix(api): stop GZip buffering the NDJSON answer stream GZipMiddleware (app/main.py) compresses any streaming response whose content type Starlette does not exempt, and it exempts only text/event-stream. Its GZipResponder writes each chunk into a GzipFile without flushing, so deflate held every token until the generator closed and the answer arrived as one block. Measured against the real middleware stack, 30 records yielded 50ms apart, lag = arrival - yield: with GZip, Accept-Encoding: gzip mean 0.795s max 1.593s with GZip, Accept-Encoding: identity mean 0.000s max 0.001s without GZip, Accept-Encoding: gzip mean 0.000s max 0.001s BaseHTTPMiddleware does not buffer, so GZip alone is the cause and the fix is scoped to it. Browsers cannot opt out: Accept-Encoding is a forbidden header name. Confirmed on a live server - content-encoding: identity on POST /api/query/stream. This silently defeated the 15s keepalive here and the 50ms flush throttle in useChatStream.ts. Two tests. No existing test could have caught this: conftest builds its client with httpx ASGITransport, which awaits the app to completion and accumulates the body before constructing the Response, so nothing routed through that fixture can observe streaming at all. The timing test therefore drives the ASGI app directly. It asserts a *decodable* record rather than a non-empty body, because gzip emits a 10-byte header on its first write and a non-empty check passes even when fully buffered. --- app/api/search.py | 17 ++- tests/test_stream_and_history_hardening.py | 150 +++++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/app/api/search.py b/app/api/search.py index fbbd687..060797e 100644 --- a/app/api/search.py +++ b/app/api/search.py @@ -257,7 +257,22 @@ async def stream_results(): with contextlib.suppress(Exception): await agen.aclose() - return StreamingResponse(stream_results(), media_type="application/x-ndjson") + # Content-Encoding opts this stream out of GZipMiddleware (app/main.py), and + # it has to: Starlette exempts only text/event-stream by content type + # (starlette.middleware.gzip.DEFAULT_EXCLUDED_CONTENT_TYPES), and its + # GZipResponder writes each chunk into a GzipFile without flushing, so + # deflate holds every token until the generator closes. Measured on this + # stack: 30 records yielded 50ms apart arrived with mean lag 0.795s / max + # 1.593s, i.e. all at once at the end - which silently defeats the keepalive + # above and the 50ms flush throttle in useChatStream.ts. Browsers cannot opt + # out; Accept-Encoding is a forbidden header name. IdentityResponder passes + # the body through untouched once content-encoding is already set. + # This is per-endpoint: a new NDJSON stream elsewhere needs the same header. + return StreamingResponse( + stream_results(), + media_type="application/x-ndjson", + headers={"Content-Encoding": "identity"}, + ) @router.get("/history") diff --git a/tests/test_stream_and_history_hardening.py b/tests/test_stream_and_history_hardening.py index 100e0a5..d283985 100644 --- a/tests/test_stream_and_history_hardening.py +++ b/tests/test_stream_and_history_hardening.py @@ -2,6 +2,7 @@ import asyncio import json +import os import pytest from httpx import AsyncClient @@ -93,3 +94,152 @@ async def test_injected_token_script_carries_a_non_empty_nonce(client: AsyncClie csp = response.headers["content-security-policy"] assert "'nonce-'" not in csp + + +@pytest.mark.asyncio +async def test_the_ndjson_stream_opts_out_of_gzip(client: AsyncClient, monkeypatch): + """Cheap guard on the header that keeps GZipMiddleware off this route. + + The expensive property - that bytes actually leave early - is asserted by + the test below. This one just pins the mechanism, because it is a single + kwarg in app/api/search.py that a later edit could drop without any content + assertion in this file noticing. + """ + + async def _one_record(*args, **kwargs): + yield {"type": "content", "text": "hello"} + + monkeypatch.setattr("app.search.retrieval.stream_rag", _one_record) + + response = await client.post("/api/query/stream", json={"question": "hi"}) + assert response.status_code == 200 + assert response.headers.get("content-encoding") == "identity" + + +@pytest.mark.asyncio +async def test_first_token_is_sent_before_the_generator_finishes(client, monkeypatch): + """The NDJSON stream must actually stream, not arrive as one block. + + GZipMiddleware (app/main.py) compresses any streaming response whose content + type it does not exempt, and it exempts only text/event-stream. Its + GZipResponder writes each chunk into a GzipFile without flushing, so deflate + holds every token until the generator closes. Content is unaffected - which + is why every other test in this file passed throughout - so only arrival + *order* can catch it. + + Driven against the ASGI app directly rather than through the `client` + fixture, because httpx's ASGITransport cannot express this: it awaits the + app to completion and accumulates the body before constructing the Response + (httpx/_transports/asgi.py:170,185). Nothing routed through that transport + can observe streaming at all. + + Asserted without wall-clock thresholds. The fake provider blocks on + `release` after its first record, and only this test sets `release` - and + only once a *decodable* record has actually been sent. Buffered, no record + is ever sent, `release` is never set, the provider never returns, and + wait_for trips. + + "Decodable" rather than "non-empty" for a reason the negative control + caught: gzip emits its 10-byte header on the first write, so a non-empty + check passes even when the payload is fully buffered. + + Negative control: drop headers={"Content-Encoding": "identity"} from the + StreamingResponse in app/api/search.py and this fails with the buffered + message below. + """ + from app.main import app + + release = asyncio.Event() + first_body = asyncio.Event() + + async def _blocks_until_its_first_record_is_sent(*args, **kwargs): + yield {"type": "content", "text": "first"} + await release.wait() + yield {"type": "content", "text": "second"} + + monkeypatch.setattr("app.search.retrieval.stream_rag", _blocks_until_its_first_record_is_sent) + + payload = json.dumps({"question": "does this stream?"}).encode() + token = os.environ.get("X_LOCAL_ACCESS_TOKEN", "test_token") + scope = { + "type": "http", + "asgi": {"version": "3.0", "spec_version": "2.1"}, + "http_version": "1.1", + "method": "POST", + "scheme": "http", + "path": "/api/query/stream", + "raw_path": b"/api/query/stream", + "query_string": b"", + "root_path": "", + "headers": [ + (b"host", b"test"), + (b"content-type", b"application/json"), + (b"content-length", str(len(payload)).encode()), + (b"x-local-access-token", token.encode()), + # What every browser sends and cannot be told not to send. + (b"accept-encoding", b"gzip, deflate"), + ], + "client": ("127.0.0.1", 1234), + "server": ("testserver", 80), + "state": {}, + } + + sent: list[dict] = [] + body_delivered = False + still_connected = asyncio.Event() + + async def receive(): + # The body once, then block, which is what a live connection does. + # Returning http.request twice trips BaseHTTPMiddleware's wrapped_receive, + # and returning http.disconnect makes the endpoint abandon generation. + nonlocal body_delivered + if not body_delivered: + body_delivered = True + return {"type": "http.request", "body": payload, "more_body": False} + await still_connected.wait() + return {"type": "http.disconnect"} + + streamed = bytearray() + + async def send(message): + sent.append(message) + if message["type"] != "http.response.body": + return + streamed.extend(message.get("body", b"")) + # A non-empty body message is not enough: gzip emits its 10-byte header + # on the first write, so the naive check passes even when fully buffered. + # The bar is a record the client could actually decode. + head, sep, _ = bytes(streamed).partition(b"\n") + if not sep: + return + try: + json.loads(head) + except (UnicodeDecodeError, ValueError): + return + first_body.set() + + task = asyncio.ensure_future(app(scope, receive, send)) + try: + try: + await asyncio.wait_for(first_body.wait(), timeout=10) + except TimeoutError: # pragma: no cover - only on regression + pytest.fail( + "No response body was sent while the provider was still " + "producing: the NDJSON stream is buffered end-to-end." + ) + finally: + release.set() + await asyncio.wait_for(task, timeout=10) + finally: + release.set() + still_connected.set() + task.cancel() + + start = next(m for m in sent if m["type"] == "http.response.start") + headers = {k.decode().lower(): v.decode() for k, v in start["headers"]} + assert headers.get("content-encoding") == "identity" + + body = b"".join(m.get("body", b"") for m in sent if m["type"] == "http.response.body") + records = [json.loads(line) for line in body.decode().splitlines() if line.strip()] + texts = [r.get("text") for r in records if r.get("type") == "content"] + assert texts == ["first", "second"], records From 1bdb97b4ec4e3e23fc0893cfde04ff8ca22d37e8 Mon Sep 17 00:00:00 2001 From: Binit Varghese Date: Sun, 23 Aug 2026 16:55:41 +0530 Subject: [PATCH 2/4] perf(db): drop the duplicate chunks index, index the graph seed lookup Two SQLite defects, kept in one commit because both live in db.py. 1. idx_chunks_text_lookup ON chunks(id, text_preview) duplicated the whole compressed corpus: `id` is the rowid, so the index stored a second full copy of the text column. Nine lines below its declaration, schema.sql already carried a tombstone for idx_chunks_covering, dropped in Phase 9 for exactly this reason - the same defect had been reintroduced under a second name. A/B on 21,584 chunks of 512-char zlib bodies: insert 0.072s -> 0.114s (+57.8%) on disk 4792 KiB -> 9112 KiB (+90.2%) No read benefit on any shape tested, including the FTS-rebuild projection, the only query the planner chose it for: 0.0815s with it against 0.0803s without. The DROP in _migrate is the load-bearing half. executescript(schema.sql) runs before _migrate, so the drop wins regardless of what schema.sql declares - verified by restoring the CREATE and watching the test still pass. Removing the CREATE only stops every startup building the index and immediately dropping it again. 2. The seed step of every graph traversal filtered on json_extract(properties, '$.chunk_id'), which is not sargable, so it scanned kg_nodes and parsed JSON per row - while kg_nodes.chunk_id sat there, written on every insert and indexed by nothing. 64,752 nodes, 20 seeds: json_extract (shipped) 22.54 ms SCAN kg_nodes chunk_id column, no index 7.04 ms SCAN kg_nodes chunk_id column + index 0.06 ms SEARCH USING INDEX All four read sites move to the column; the migration back-fill at db.py:513 keeps reading JSON, as it must. Testing the equivalence found a hole: the existing rebuild migration back-fills chunk_id only when the column is *absent*, so a row whose provenance lived only in the JSON would have vanished from graph seeds. Confirmed divergent - with one such row the old predicate returned it and the column predicate did not. Now back-filled, skipping rows whose chunk is gone, because chunk_id is a FOREIGN KEY and adopting a deleted chunk would fail the migration. test_coverage_booster_gen3 inserted kg_nodes without chunk_id, a row shape production never writes. It now writes the column, and gained the chunk row that the FOREIGN KEY needs - a constraint that was simply unenforced while the column was NULL. --- app/storage/db.py | 61 +++++++++++++++-- app/storage/schema.sql | 11 ++- tests/test_coverage_booster_gen3.py | 26 ++++++-- tests/test_indexing_executor_lifecycle.py | 76 +++++++++++++++++++++ tests/test_retrieval_hardening.py | 81 +++++++++++++++++++++++ 5 files changed, 244 insertions(+), 11 deletions(-) diff --git a/app/storage/db.py b/app/storage/db.py index 56b9e1f..00d8dc2 100644 --- a/app/storage/db.py +++ b/app/storage/db.py @@ -520,6 +520,45 @@ async def _migrate(self, conn: "aiosqlite.Connection") -> None: except Exception as exc: logger.warning("kg_nodes schema migration failed: %s", exc) + # Back-fill any node whose provenance lives only in the properties JSON. + # The rebuild above back-fills the column, but only when the column was + # missing entirely; a database that already had it keeps whatever was + # written, and NULL there is now the difference between a node being + # reachable as a graph seed and being invisible. Verified divergent: with + # one such row, the old json_extract predicate returned it and the column + # predicate did not. Orphans are left NULL deliberately - chunk_id is a + # FOREIGN KEY onto chunks(id) and foreign_keys is ON, so pointing it at a + # deleted chunk would fail the whole migration. + try: + await conn.execute(""" + UPDATE kg_nodes + SET chunk_id = CAST(json_extract(properties, '$.chunk_id') AS INTEGER) + WHERE chunk_id IS NULL + AND json_extract(properties, '$.chunk_id') IS NOT NULL + AND EXISTS ( + SELECT 1 FROM chunks c + WHERE c.id = CAST(json_extract(kg_nodes.properties, '$.chunk_id') AS INTEGER) + ) + """) + await conn.commit() + except Exception as exc: + logger.warning("kg_nodes chunk_id back-fill failed: %s", exc) + + # The seed step of every graph traversal looks nodes up by chunk_id, and + # kg_nodes carried no index at all beyond its id PRIMARY KEY. Measured on + # 64,752 nodes: the shipped json_extract predicate scanned the table and + # parsed JSON per row at 22.54 ms; the column with this index is 0.06 ms. + # Created here rather than beside the CREATE TABLE above because the + # migration block just above may rename and rebuild kg_nodes, which would + # take the index with it. + try: + await conn.execute( + "CREATE INDEX IF NOT EXISTS idx_kg_nodes_chunk_id ON kg_nodes(chunk_id)" + ) + await conn.commit() + except Exception as exc: + logger.warning("Failed to create idx_kg_nodes_chunk_id: %s", exc) + # GraphRAG edges try: await conn.execute(""" @@ -603,6 +642,20 @@ async def _migrate(self, conn: "aiosqlite.Connection") -> None: except Exception as exc: logger.warning("Failed to drop idx_chunks_covering: %s", exc) + # The same defect, reintroduced under a second name: `id` is the rowid, + # so indexing (id, text_preview) kept a second full copy of the + # compressed corpus. Measured +90.2% on disk and +57.8% insert time for + # no read benefit (see the note in schema.sql). Dropped here as well as + # removed from schema.sql, because executescript(schema.sql) above does + # not reconcile away what it no longer declares - an existing database + # would keep the index forever. Space returns via the incremental_vacuum + # already running at startup. + try: + await conn.execute("DROP INDEX IF EXISTS idx_chunks_text_lookup") + await conn.commit() + except Exception as exc: + logger.warning("Failed to drop idx_chunks_text_lookup: %s", exc) + # Phase 9.2: Rebuild chunk_fts with detail=column to save ~40% space try: cur = await conn.execute( @@ -1155,7 +1208,7 @@ def _bfs_cte(placeholders: str) -> str: bfs_nodes(id, depth) AS ( SELECT id, 0 FROM kg_nodes - WHERE json_extract(properties, '$.chunk_id') IN ({placeholders}) + WHERE chunk_id IN ({placeholders}) UNION @@ -1183,10 +1236,10 @@ async def bfs_from_chunks( placeholders = ",".join("?" for _ in chunk_ids) # Only bound placeholders are interpolated; every value is parameterized. projection = """ - SELECT DISTINCT CAST(json_extract(n.properties, '$.chunk_id') AS INTEGER) as chunk_id + SELECT DISTINCT n.chunk_id as chunk_id FROM bfs_nodes b JOIN kg_nodes n ON b.id = n.id - WHERE json_extract(n.properties, '$.chunk_id') IS NOT NULL + WHERE n.chunk_id IS NOT NULL LIMIT ? """ query = self._bfs_cte(placeholders) + projection # nosec B608 @@ -1218,7 +1271,7 @@ async def get_relational_paths( paths(id, path_str, depth, visited) AS ( SELECT id, label || ' ' || id, 0, ',' || id || ',' FROM kg_nodes - WHERE json_extract(properties, '$.chunk_id') IN ({placeholders}) + WHERE chunk_id IN ({placeholders}) UNION ALL diff --git a/app/storage/schema.sql b/app/storage/schema.sql index f95307e..eb91c54 100644 --- a/app/storage/schema.sql +++ b/app/storage/schema.sql @@ -40,7 +40,16 @@ CREATE TABLE IF NOT EXISTS chunks ( -- FK index for efficient joins / cascading deletes CREATE INDEX IF NOT EXISTS idx_chunks_file_id ON chunks(file_id); -CREATE INDEX IF NOT EXISTS idx_chunks_text_lookup ON chunks(id, text_preview); + +-- NOTE: idx_chunks_text_lookup ON chunks(id, text_preview) was dropped for the +-- same reason idx_chunks_covering was (see below), and is dropped from existing +-- databases in db.py _migrate. `id` is the rowid, so indexing (id, text_preview) +-- stored a second full copy of the compressed corpus. Measured on 21,584 chunks +-- of 512-char zlib bodies: +90.2% on disk (4792 -> 9112 KiB) and +57.8% insert +-- time, for no read benefit at all. The planner did pick it, but only for scans +-- that read the same bytes either way - the FTS-rebuild projection +-- `SELECT id, zlib_decompress(text_preview) FROM chunks` measured 0.0815s with +-- it against 0.0803s without. Do not reintroduce it under a third name. -- Chunk embeddings table for storing vector math as binary BLOBs -- This strictly isolates heavy binary data from normal metadata queries diff --git a/tests/test_coverage_booster_gen3.py b/tests/test_coverage_booster_gen3.py index cdae94e..00bbf0d 100644 --- a/tests/test_coverage_booster_gen3.py +++ b/tests/test_coverage_booster_gen3.py @@ -80,19 +80,33 @@ async def test_db_manager_coverage_booster(mock_db): (10, file_id, 0, 20, compressed_text), ) + # A second chunk so the graph below spans two of them. Needed now that + # kg_nodes.chunk_id is populated: the column carries + # FOREIGN KEY (chunk_id) REFERENCES chunks(id), which a NULL column left + # unenforced. Production always has the referent, so the fixture should too. + await db.execute_write( + "INSERT INTO chunks (id, file_id, start_offset, end_offset, text_preview) VALUES (?, ?, ?, ?, ?)", + (11, file_id, 20, 40, compressed_text), + ) + # 5. Test BFS and knowledge graph queries assert await db.bfs_from_chunks([]) == [] assert await db.get_relational_paths([]) == [] - # Insert kg_nodes and kg_edges - # properties contains chunk_id + # Insert kg_nodes and kg_edges. + # chunk_id goes in the column, not only in the properties JSON. This fixture + # used to omit the column and rely on json_extract(properties,'$.chunk_id'), + # which is a row shape production never writes: insert_kg_nodes_bulk fills + # both from the same variable (app/indexing/service.py), and the _migrate + # back-fill populates the column for pre-existing rows. The traversal reads + # the column now, so writing only the JSON tested a path nothing produced. await db.execute_write( - "INSERT INTO kg_nodes (id, label, type, properties) VALUES (?, ?, ?, ?)", - ("node1", "Class", "type_foo", '{"chunk_id": 10}'), + "INSERT INTO kg_nodes (id, label, type, properties, chunk_id) VALUES (?, ?, ?, ?, ?)", + ("node1", "Class", "type_foo", '{"chunk_id": 10}', 10), ) await db.execute_write( - "INSERT INTO kg_nodes (id, label, type, properties) VALUES (?, ?, ?, ?)", - ("node2", "Method", "type_bar", '{"chunk_id": 11}'), + "INSERT INTO kg_nodes (id, label, type, properties, chunk_id) VALUES (?, ?, ?, ?, ?)", + ("node2", "Method", "type_bar", '{"chunk_id": 11}', 11), ) await db.execute_write( "INSERT INTO kg_edges (source, target, relation) VALUES (?, ?, ?)", diff --git a/tests/test_indexing_executor_lifecycle.py b/tests/test_indexing_executor_lifecycle.py index 2ea22fe..062a906 100644 --- a/tests/test_indexing_executor_lifecycle.py +++ b/tests/test_indexing_executor_lifecycle.py @@ -394,3 +394,79 @@ async def test_root_path_is_added_to_an_existing_index(self, tmp_path): # derives a fallback root for exactly these rows. assert [tuple(r) for r in rows] == [("/srv/proj/a.py", "")] assert "idx_files_root_path" in idx + + @pytest.mark.asyncio + async def test_the_redundant_text_lookup_index_is_gone_and_stays_gone(self, tmp_path): + """`idx_chunks_text_lookup` duplicated the whole compressed corpus. + + `chunks.id` is the rowid, so an index on (id, text_preview) is a second + full copy of the text column. Measured on 21,584 chunks: +90.2% on disk + and +57.8% insert time, against no read benefit on any query shape - + including the FTS-rebuild projection, the only one the planner chose it + for. This is the same defect as `idx_chunks_covering`, which was already + dropped for the same reason. + + What this test pins is the DROP in `_migrate`. It is the load-bearing + half: `executescript(schema.sql)` runs *before* `_migrate`, so the drop + wins regardless of what schema.sql declares - verified by restoring the + CREATE line and watching this still pass. Removing the CREATE is + therefore not what makes the behaviour correct; it is what stops every + startup from building the index and immediately dropping it again. + + The reverse control does bite: disable the DROP and the legacy half + fails, because `executescript` never reconciles away a declaration that + schema.sql has stopped making. + """ + import aiosqlite + + from app.storage.db import DatabaseManager + + # Half one: an existing index that already carries it must lose it. + legacy = tmp_path / "legacy.db" + async with aiosqlite.connect(str(legacy)) as conn: + await conn.execute( + "CREATE TABLE chunks (id INTEGER PRIMARY KEY AUTOINCREMENT," + " file_id INTEGER NOT NULL, start_offset INTEGER NOT NULL," + " end_offset INTEGER NOT NULL, text_preview TEXT NOT NULL)" + ) + await conn.execute("CREATE INDEX idx_chunks_text_lookup ON chunks(id, text_preview)") + await conn.commit() + async with conn.execute( + "SELECT name FROM sqlite_master WHERE type='index' AND name=?", + ("idx_chunks_text_lookup",), + ) as cur: + assert await cur.fetchone(), "fixture did not create the index" + + mgr = DatabaseManager(str(legacy)) + try: + await mgr.init_db(schema_path="app/storage/schema.sql") + migrated = { + r[0] + for r in await mgr.execute_query( + "SELECT name FROM sqlite_master WHERE type='index'" + ) + } + finally: + await mgr.close() + + assert "idx_chunks_text_lookup" not in migrated, ( + "an existing database kept the redundant index through init_db" + ) + # The FK index is the one that earns its keep; it must survive. + assert "idx_chunks_file_id" in migrated + + # Half two: a fresh database must never grow it in the first place. + fresh_mgr = DatabaseManager(str(tmp_path / "fresh.db")) + try: + await fresh_mgr.init_db(schema_path="app/storage/schema.sql") + fresh = { + r[0] + for r in await fresh_mgr.execute_query( + "SELECT name FROM sqlite_master WHERE type='index'" + ) + } + finally: + await fresh_mgr.close() + + assert "idx_chunks_text_lookup" not in fresh, "schema.sql still declares it" + assert "idx_chunks_file_id" in fresh diff --git a/tests/test_retrieval_hardening.py b/tests/test_retrieval_hardening.py index 563f6f9..f425526 100644 --- a/tests/test_retrieval_hardening.py +++ b/tests/test_retrieval_hardening.py @@ -897,3 +897,84 @@ async def test_agentic_loop_is_off_by_default(monkeypatch): "q", plan, MagicMock(), MagicMock(), MagicMock(), MagicMock(), 10, None, None, None ) assert got is None and trace is None + + +@pytest.mark.asyncio +async def test_graph_seed_lookup_uses_an_index_rather_than_scanning(real_db): + """The seed step must be an index search, not a full scan of kg_nodes. + + `_bfs_cte` used to select seeds with + `json_extract(properties, '$.chunk_id') IN (...)`, which is not sargable, so + every GRAPH_SEARCH began by scanning kg_nodes and JSON-parsing each row - + while `kg_nodes.chunk_id` sat right there, written on every insert and + indexed by nothing. Measured on 64,752 nodes with 20 seeds: 22.54 ms + scanning against 0.06 ms through the index. + + Asserted against the query plan because that is the property; a behavioural + test passes either way, which is exactly why this survived so long. + """ + chunk_ids = await _seed_cycle(real_db, size=3) + + query = DatabaseManager._bfs_cte("?") + " SELECT COUNT(*) FROM bfs_nodes" # noqa: S608 + plan = [ + row[3] + for row in await real_db.execute_query("EXPLAIN QUERY PLAN " + query, (chunk_ids[0], 3, 3)) + ] + + seed_steps = [step for step in plan if "kg_nodes" in step] + assert seed_steps, plan + assert any("idx_kg_nodes_chunk_id" in step for step in seed_steps), plan + assert not any(step.startswith("SCAN kg_nodes") for step in seed_steps), plan + + +@pytest.mark.asyncio +async def test_nodes_whose_chunk_id_lives_only_in_json_are_backfilled(tmp_path: Path): + """A NULL chunk_id column would silently drop a node from every graph seed. + + The traversal reads `kg_nodes.chunk_id` rather than parsing + `properties`. The existing rebuild migration back-fills that column, but + only when the column is absent altogether - a database that already had it + keeps whatever was written. Verified divergent before this back-fill + existed: with one JSON-only row, the old predicate returned it and the + column predicate did not. + + Orphans stay NULL on purpose: chunk_id is a FOREIGN KEY onto chunks(id) with + foreign_keys ON, so adopting a deleted chunk would fail the migration. + """ + db_file = tmp_path / "backfill.db" + mgr = DatabaseManager(str(db_file)) + await mgr.init_db(schema_path="app/storage/schema.sql") + conn = mgr._get_conn() + await conn.execute("INSERT INTO files (path,size,modified_at,type) VALUES ('f',1,'now','.py')") + await conn.execute( + "INSERT INTO chunks (id,file_id,start_offset,end_offset,text_preview) VALUES (7,1,0,10,?)", + (zlib.compress(b"body"),), + ) + # Provenance in the JSON only, as a pre-existing row could carry it. + await conn.execute( + "INSERT INTO kg_nodes (id,type,label,properties,chunk_id) VALUES (?,?,?,?,?)", + ("orphaned_col", "func", "f", json.dumps({"chunk_id": 7}), None), + ) + # Same shape, but the chunk it names is gone. + await conn.execute( + "INSERT INTO kg_nodes (id,type,label,properties,chunk_id) VALUES (?,?,?,?,?)", + ("dangling", "func", "g", json.dumps({"chunk_id": 999}), None), + ) + await conn.commit() + await mgr.close() + + # Re-open: _migrate runs again and should adopt the recoverable row. + mgr = DatabaseManager(str(db_file)) + try: + await mgr.init_db(schema_path="app/storage/schema.sql") + rows = dict( + (r[0], r[1]) + for r in await mgr.execute_query("SELECT id, chunk_id FROM kg_nodes ORDER BY id") + ) + seeds = await mgr.bfs_from_chunks([7], max_depth=1, limit=5) + finally: + await mgr.close() + + assert rows["orphaned_col"] == 7, "a recoverable node was left unreachable" + assert rows["dangling"] is None, "a node pointing at a deleted chunk must stay NULL" + assert 7 in seeds, "the back-filled node is still not reachable as a graph seed" From 616b9664ddca69cd63db40a591afb03b6ac88da6 Mon Sep 17 00:00:00 2001 From: Binit Varghese Date: Sun, 23 Aug 2026 16:56:07 +0530 Subject: [PATCH 3/4] fix(frontend): one cache key per endpoint, and optimistic writes Three endpoints were each cached under two keys - getFileTree as 'file-tree' and 'files-tree', getLLMPreferences as 'llm-preferences' and 'llm-prefs', getOcrStatus as 'ocr-status' and 'ocr-status-settings'. Each split meant two fetches of one payload and invalidation that never crossed: removing a folder index in Explorer invalidated only its own spelling, so Search kept offering the deleted folder until a reload. cacheKeys.ts now holds one key per endpoint, with a test asserting no two collide. invalidateCache() with no argument ran queryClient.clear(), which *removes* every query rather than marking it stale, so every mounted observer dropped to data: undefined and every page fell back to its cold-start spinner. Four call sites did that, including the one firing when an index run completes - finishing a scan blanked the whole app. The prefix is now required and the clear() path is gone; those four became invalidateCorpusCaches(), which touches only the six corpus-derived keys. An index run cannot change which providers are configured. Two render defects, both from useApi reporting `isLoading || isFetching` with refetchOnWindowFocus enabled: - ExplorerPage replaced the whole tree with a spinner on any background refetch; now guarded on `&& !tree`, the pattern already used correctly in four other places. - SetupPage gated its "Incompatible Storage Detected" warning on !isLoading, and registers focus/visibilitychange listeners that refetch - so a storage-compatibility warning blinked out exactly when the user alt-tabbed back. Both banners now gate on data presence; isDriveConfigSafe already defaults safe while driveInfo is absent. Five writes move to useMutation. useOptimisticMutation carries the ceremony that is easy to get wrong once, let alone five times: cancel in-flight refetches so a stale response cannot land on top of the optimistic value, snapshot, roll back on error, re-sync either way. - remove folder: leaves the tree on click; the button had no pending state at all, so a double-click sent two requests - OCR enabled and cloud-privacy consent checkboxes: both bound `checked` straight to server data and visibly snapped back until the refetch - fallback-chain reorder: shares the provider-settings cache entry - Clear Index and Refresh: no optimistic state to show, but no pending state either until now Repointing the three stale mocks revealed they had been returning undefined and nothing asserted on it. With ocr-status live, OcrSection renders and calls getOcrQueue, which a bare vi.fn() resolves as undefined - hence the mock and act() fixes in SettingsPage.test.tsx. The InsightsPage act() fix is unrelated to this change and pre-existing: 2 warnings at HEAD, 0 with it. --- frontend/src/__tests__/cacheKeys.test.ts | 64 +++++++++ .../components/ExplorerPage.test.tsx | 47 ++++++- .../components/InsightsPage.test.tsx | 7 +- .../__tests__/components/SearchPage.test.tsx | 2 +- .../components/SettingsPage.test.tsx | 48 ++++--- .../__tests__/components/SetupPage.test.tsx | 65 ++++++++- .../__tests__/useOptimisticMutation.test.tsx | 125 ++++++++++++++++++ frontend/src/cacheKeys.ts | 53 ++++++++ frontend/src/components/AppShell.tsx | 5 +- .../src/components/providers/ModelPicker.tsx | 3 +- frontend/src/pages/ExplorerPage.tsx | 67 +++++++--- frontend/src/pages/InsightsPage.tsx | 5 +- frontend/src/pages/LibraryPage.tsx | 83 +++++++----- frontend/src/pages/ProvidersPage.tsx | 42 +++--- frontend/src/pages/SearchPage.tsx | 11 +- frontend/src/pages/SettingsPage.tsx | 80 ++++++----- frontend/src/pages/SetupPage.tsx | 22 ++- frontend/src/providers/ProviderRecipes.tsx | 5 +- frontend/src/useApi.ts | 33 ++++- frontend/src/useOptimisticMutation.ts | 63 +++++++++ 20 files changed, 680 insertions(+), 150 deletions(-) create mode 100644 frontend/src/__tests__/cacheKeys.test.ts create mode 100644 frontend/src/__tests__/useOptimisticMutation.test.tsx create mode 100644 frontend/src/cacheKeys.ts create mode 100644 frontend/src/useOptimisticMutation.ts diff --git a/frontend/src/__tests__/cacheKeys.test.ts b/frontend/src/__tests__/cacheKeys.test.ts new file mode 100644 index 0000000..43d3f2e --- /dev/null +++ b/frontend/src/__tests__/cacheKeys.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +import { CACHE_KEYS, CORPUS_DERIVED_KEYS, launchStatusKey } from '../cacheKeys' + +vi.mock('../queryClient', () => ({ + queryClient: { + invalidateQueries: vi.fn(), + clear: vi.fn(), + }, +})) + +import { queryClient } from '../queryClient' +import { invalidateCache, invalidateCorpusCaches } from '../useApi' + +describe('cache keys', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('maps every endpoint to a distinct key', () => { + // The defect this file exists to prevent: three endpoints were each cached + // under two spellings ('file-tree'/'files-tree', + // 'llm-preferences'/'llm-prefs', 'ocr-status'/'ocr-status-settings'), so the + // same payload was fetched twice and invalidating one never reached the + // other. Two names for one endpoint is the bug; two *values* colliding here + // would be a different one, so assert the values are unique. + const values = Object.values(CACHE_KEYS) + expect(new Set(values).size).toBe(values.length) + }) + + it('builds a distinct launch-status key per provider', () => { + expect(launchStatusKey('ollama')).not.toBe(launchStatusKey('lm_studio')) + }) + + it('invalidates rather than evicting', () => { + // queryClient.clear() *removes* every query (query-core 5.94.5: + // getAll().forEach(q => this.remove(q))), so each mounted observer drops to + // data: undefined and every page falls back to its cold-start spinner. + // invalidateQueries refetches while keeping the previous data on screen. + invalidateCache(CACHE_KEYS.fileTree) + + expect(queryClient.clear).not.toHaveBeenCalled() + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: [CACHE_KEYS.fileTree], + }) + }) + + it('refreshes corpus-derived views without touching configuration state', () => { + invalidateCorpusCaches() + + const invalidated = vi + .mocked(queryClient.invalidateQueries) + .mock.calls.map((call) => (call[0] as { queryKey: string[] }).queryKey[0]) + + expect(new Set(invalidated)).toEqual(new Set(CORPUS_DERIVED_KEYS)) + // An index run cannot change which providers are configured, so blowing + // those away to refresh the file tree is what the old bare + // invalidateCache() did wrong. + expect(invalidated).not.toContain(CACHE_KEYS.providersList) + expect(invalidated).not.toContain(CACHE_KEYS.providerSettings) + expect(invalidated).not.toContain(CACHE_KEYS.llmPreferences) + expect(queryClient.clear).not.toHaveBeenCalled() + }) +}) diff --git a/frontend/src/__tests__/components/ExplorerPage.test.tsx b/frontend/src/__tests__/components/ExplorerPage.test.tsx index 4e0f609..9361cb0 100644 --- a/frontend/src/__tests__/components/ExplorerPage.test.tsx +++ b/frontend/src/__tests__/components/ExplorerPage.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { screen, fireEvent } from '@testing-library/react'; +import { screen, fireEvent, waitFor } from '@testing-library/react'; import { ExplorerPage } from '../../pages/ExplorerPage'; import { renderWithProviders } from '../test-utils'; import { removeFolderIndex } from '../../api'; @@ -11,6 +11,9 @@ import { removeFolderIndex } from '../../api'; // broken tree it produced was never caught here. const ROOT = 'C:/projects/test'; +// Mutable so a test can report a background refetch without re-mocking. +const state = { loading: false }; + vi.mock('../../useApi', () => ({ useApi: vi.fn((_, opts) => { if (opts?.cacheKey === 'file-tree') { @@ -25,7 +28,7 @@ vi.mock('../../useApi', () => ({ total_files: 2, total_size: 3072, }, - loading: false, + loading: state.loading, error: null, refetch: vi.fn(), }; @@ -50,6 +53,7 @@ vi.mock('../../api', () => ({ describe('ExplorerPage Component', () => { beforeEach(() => { vi.clearAllMocks(); + state.loading = false; vi.spyOn(window, 'confirm').mockReturnValue(true); vi.spyOn(window, 'alert').mockImplementation(() => {}); }); @@ -92,6 +96,43 @@ describe('ExplorerPage Component', () => { const del = screen.getAllByTitle('Delete this folder index')[0]; fireEvent.click(del); - expect(removeFolderIndex).toHaveBeenCalledWith([ROOT]); + // Awaited now that the delete goes through useMutation: `mutate` schedules + // the mutationFn rather than entering it synchronously the way the old bare + // `await removeFolderIndex(...)` in the click handler did. + await waitFor(() => expect(removeFolderIndex).toHaveBeenCalledWith([ROOT])); + }); + + it('does not fire a second removal while the first is in flight', async () => { + // The delete button had no pending state at all, so a double-click sent two + // requests for the same folder. + let resolveDelete: (v: unknown) => void = () => {}; + vi.mocked(removeFolderIndex).mockImplementationOnce( + () => new Promise((resolve) => { resolveDelete = resolve; }) as ReturnType, + ); + + renderWithProviders(); + + const del = screen.getAllByTitle('Delete this folder index')[0]; + fireEvent.click(del); + await waitFor(() => expect(removeFolderIndex).toHaveBeenCalledTimes(1)); + + fireEvent.click(del); + expect(removeFolderIndex).toHaveBeenCalledTimes(1); + + resolveDelete({ message: 'ok', chunks_removed: 0 }); + }); + it('keeps the tree on screen while a background refetch is in flight', () => { + // useApi reports `isLoading || isFetching`, so this goes true on every + // background refetch - and with refetchOnWindowFocus enabled, alt-tabbing + // back replaced the entire explorer with a full-panel spinner. The guard is + // `loading && !tree`: spin only when there is nothing to show yet. + state.loading = true; + + renderWithProviders(); + + expect( + screen.getAllByTitle('Delete this folder index').length, + 'the rendered tree was replaced by a spinner during a refetch', + ).toBeGreaterThan(0); }); }); diff --git a/frontend/src/__tests__/components/InsightsPage.test.tsx b/frontend/src/__tests__/components/InsightsPage.test.tsx index f85ebe6..e82b60a 100644 --- a/frontend/src/__tests__/components/InsightsPage.test.tsx +++ b/frontend/src/__tests__/components/InsightsPage.test.tsx @@ -76,12 +76,17 @@ vi.mock('../../api', () => ({ })); describe('InsightsPage Component', () => { - it('renders Insights page title and cards', () => { + it('renders Insights page title and cards', async () => { renderWithProviders(); expect(screen.getByText('Insights')).toBeDefined(); expect(screen.getByText('Total Files')).toBeDefined(); expect(screen.getByText('Indexed Files Size')).toBeDefined(); expect(screen.getByText('Database Size')).toBeDefined(); + + // KnowledgePortrait's getPortrait() promise (mocked to resolve with no + // themes) settles after the assertions above return, so its setState + // lands outside act() unless we wait for the resulting empty-state text. + expect(await screen.findByText('No Portrait Available')).toBeDefined(); }); }); diff --git a/frontend/src/__tests__/components/SearchPage.test.tsx b/frontend/src/__tests__/components/SearchPage.test.tsx index 1ffc68a..abb39e3 100644 --- a/frontend/src/__tests__/components/SearchPage.test.tsx +++ b/frontend/src/__tests__/components/SearchPage.test.tsx @@ -10,7 +10,7 @@ vi.mock('../../useApi', () => ({ if (opts?.cacheKey === 'query-history') { return { data: { history: [] }, loading: false, error: null, refetch: vi.fn() }; } - if (opts?.cacheKey === 'files-tree') { + if (opts?.cacheKey === 'file-tree') { return { data: { folders: {}, total_files: 0, total_size: 0 }, loading: false, error: null, refetch: vi.fn() }; } if (opts?.cacheKey === 'app-config') { diff --git a/frontend/src/__tests__/components/SettingsPage.test.tsx b/frontend/src/__tests__/components/SettingsPage.test.tsx index 4e826a5..02567a7 100644 --- a/frontend/src/__tests__/components/SettingsPage.test.tsx +++ b/frontend/src/__tests__/components/SettingsPage.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { screen, fireEvent, waitFor } from '@testing-library/react'; +import { screen, fireEvent, waitFor, act } from '@testing-library/react'; import { SettingsPage } from '../../pages/SettingsPage'; import { renderWithProviders } from '../test-utils'; import { invalidateCache } from '../../useApi'; @@ -62,10 +62,10 @@ vi.mock('../../useApi', () => ({ if (opts?.cacheKey === 'auth-status') return wrap(mockAuthStatus); if (opts?.cacheKey === 'local-models') return wrap(state.localModels); if (opts?.cacheKey === 'system-info') return wrap(mockSystemInfo); - if (opts?.cacheKey === 'llm-prefs') return wrap(mockLLMPrefs); + if (opts?.cacheKey === 'llm-preferences') return wrap(mockLLMPrefs); if (opts?.cacheKey === 'drive-info') return wrap(mockDriveInfo); if (opts?.cacheKey === 'providers-list') return wrap(mockProviders); - if (opts?.cacheKey === 'ocr-status-settings') return wrap(state.ocrStatus); + if (opts?.cacheKey === 'ocr-status') return wrap(state.ocrStatus); if (opts?.cacheKey === 'ocr-tiers') return wrap(state.ocrTiers); if (opts?.cacheKey === 'ocr-vlm-models') return wrap(state.vlmModels); if (opts?.cacheKey === 'ocr-vlm-selection') return wrap({ selection: null }); @@ -106,7 +106,9 @@ vi.mock('../../api', () => { getVlmSelection: vi.fn(), selectVlmModel: vi.fn(), getOcrInstallState: vi.fn(), - getOcrQueue: vi.fn(), + // OcrSection awaits this and reads .items - a bare vi.fn() resolves + // undefined and sends every test down loadFailed()'s catch branch. + getOcrQueue: vi.fn(() => Promise.resolve({ items: [] })), installOcrTier: vi.fn(), uninstallOcrTier: vi.fn(), cancelOcrInstall: vi.fn(), @@ -117,6 +119,16 @@ vi.mock('../../api', () => { }; }); +// OcrSection's loadFailed() effect calls getOcrQueue() and setStates in the +// resulting microtask. A synchronous test body never yields, so that update +// lands outside act(). Awaiting inside act() flushes it - React keeps the act +// queue installed across the await, so the continuation is covered. +async function renderSettings() { + const result = renderWithProviders(); + await act(async () => {}); + return result; +} + describe('SettingsPage Component', () => { beforeEach(() => { vi.clearAllMocks(); @@ -130,40 +142,40 @@ describe('SettingsPage Component', () => { }; }); - it('renders SettingsPage with settings groups', () => { - renderWithProviders(); + it('renders SettingsPage with settings groups', async () => { + await renderSettings(); expect(screen.getByText('Settings')).toBeDefined(); expect(screen.getByText('Model Selection')).toBeDefined(); }); - it('offers to start an installed local provider that is offline', () => { - renderWithProviders(); + it('offers to start an installed local provider that is offline', async () => { + await renderSettings(); expect(screen.getByRole('button', { name: /Start Ollama/i })).toBeDefined(); expect(screen.getByRole('button', { name: /Start LM Studio/i })).toBeDefined(); }); - it('links to the installer when the provider is not installed', () => { + it('links to the installer when the provider is not installed', async () => { state.launchStatus.ollama = { ...offlineInstalled('ollama'), installed: false, method: null }; - renderWithProviders(); + await renderSettings(); expect(screen.queryByRole('button', { name: /Start Ollama/i })).toBeNull(); const link = screen.getByRole('link', { name: /Install Ollama/i }); expect(link.getAttribute('href')).toBe('https://ollama.com/download'); }); - it('hides the start button once the provider is detected', () => { + it('hides the start button once the provider is detected', async () => { state.localModels.ollama = { detected: true, models: ['llama3:8b'] }; - renderWithProviders(); + await renderSettings(); expect(screen.queryByRole('button', { name: /Start Ollama/i })).toBeNull(); expect(screen.getByText('llama3:8b')).toBeDefined(); }); - it('falls back to the manual hint when launch status is unavailable', () => { + it('falls back to the manual hint when launch status is unavailable', async () => { state.launchStatus = {}; - renderWithProviders(); + await renderSettings(); expect(screen.getByText(/Ensure Ollama is running on localhost:11434/i)).toBeDefined(); }); @@ -178,7 +190,7 @@ describe('SettingsPage Component', () => { elapsed_ms: 2100, }); - renderWithProviders(); + await renderSettings(); fireEvent.click(screen.getByRole('button', { name: /Start Ollama/i })); await waitFor(() => expect(launchProvider).toHaveBeenCalledWith('ollama')); @@ -196,7 +208,7 @@ describe('SettingsPage Component', () => { elapsed_ms: 5, }); - renderWithProviders(); + await renderSettings(); fireEvent.click(screen.getByRole('button', { name: /Start Ollama/i })); expect(await screen.findByText(/doesn't appear to be installed/i)).toBeDefined(); @@ -206,7 +218,7 @@ describe('SettingsPage Component', () => { const { selectOcrTier } = await import('../../api'); vi.mocked(selectOcrTier).mockResolvedValue({ ok: true, tier: 'gpu' }); - renderWithProviders(); + await renderSettings(); // Click GPU tier tab ("High accuracy") fireEvent.click(screen.getByRole('button', { name: /High accuracy/i })); @@ -220,7 +232,7 @@ describe('SettingsPage Component', () => { }); it('renders VLM picker with check again and start buttons when offline', async () => { - renderWithProviders(); + await renderSettings(); // Click Vision model tab ("Your own AI model") fireEvent.click(screen.getByRole('button', { name: /Your own AI model/i })); diff --git a/frontend/src/__tests__/components/SetupPage.test.tsx b/frontend/src/__tests__/components/SetupPage.test.tsx index 5f236f8..b955ec0 100644 --- a/frontend/src/__tests__/components/SetupPage.test.tsx +++ b/frontend/src/__tests__/components/SetupPage.test.tsx @@ -1,20 +1,30 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { screen } from '@testing-library/react'; import { SetupPage } from '../../pages/SetupPage'; import { renderWithProviders } from '../test-utils'; +// Mutable so a test can vary what useApi reports without re-mocking the module. +const state = { + driveInfo: { + is_portable_fs: false, + lancedb_mode: 'local', + mount_path: '/mock/path', + free_bytes: 100000000, + } as Record | undefined, + loading: false, +}; // Mock useApi directly using cacheKey vi.mock('../../useApi', () => ({ useApi: vi.fn((_, opts) => { if (opts?.cacheKey === 'providers-list') { - return { data: [], loading: false, error: null, refetch: vi.fn() }; + return { data: [], loading: state.loading, error: null, refetch: vi.fn() }; } if (opts?.cacheKey === 'local-models') { - return { data: { ollama: { detected: false, models: [] }, lm_studio: { detected: false, models: [] } }, loading: false, error: null, refetch: vi.fn() }; + return { data: { ollama: { detected: false, models: [] }, lm_studio: { detected: false, models: [] } }, loading: state.loading, error: null, refetch: vi.fn() }; } if (opts?.cacheKey === 'drive-info') { - return { data: { is_portable_fs: false, lancedb_mode: 'local', mount_path: '/mock/path', free_bytes: 100000000 }, loading: false, error: null, refetch: vi.fn() }; + return { data: state.driveInfo, loading: state.loading, error: null, refetch: vi.fn() }; } return { data: undefined, loading: false, error: null, refetch: vi.fn() }; @@ -32,6 +42,16 @@ vi.mock('../../api', () => ({ })); describe('SetupPage Component', () => { + beforeEach(() => { + state.driveInfo = { + is_portable_fs: false, + lancedb_mode: 'local', + mount_path: '/mock/path', + free_bytes: 100000000, + }; + state.loading = false; + }); + it('renders Welcome to PMA header and step 1 content', () => { renderWithProviders(); @@ -39,4 +59,41 @@ describe('SetupPage Component', () => { expect(screen.getByText("Your offline-first personal memory assistant. Let's get your intelligence engine connected.")).toBeDefined(); expect(screen.getByText('Cloud Intelligence')).toBeDefined(); }); + + it('keeps the storage warning visible while a background refetch is in flight', () => { + // useApi reports `isLoading || isFetching`, so `loading` goes true on every + // background refetch - and this page registers focus/visibilitychange + // listeners that refetch, so it went true whenever the user alt-tabbed back. + // The banner used to be gated on `!isLoading`, which meant a storage + // *incompatibility warning* silently vanished at exactly that moment. + state.driveInfo = { + is_portable_fs: true, + lancedb_mode: 'local', // portable filesystem without split_brain == unsafe + mount_path: '/mock/path', + free_bytes: 100000000, + }; + + state.loading = false; + const { unmount } = renderWithProviders(); + expect(screen.queryByText('Incompatible Storage Detected')).not.toBeNull(); + unmount(); + + state.loading = true; + renderWithProviders(); + expect( + screen.queryByText('Incompatible Storage Detected'), + 'the warning disappeared during a background refetch', + ).not.toBeNull(); + }); + + it('does not flash the storage warning before drive info has arrived', () => { + // The other half: absent data must not render a warning about data we do + // not have yet. `isDriveConfigSafe` defaults to true while driveInfo is + // undefined, and the banner also requires `driveInfo` to be present. + state.driveInfo = undefined; + state.loading = true; + + renderWithProviders(); + expect(screen.queryByText('Incompatible Storage Detected')).toBeNull(); + }); }); diff --git a/frontend/src/__tests__/useOptimisticMutation.test.tsx b/frontend/src/__tests__/useOptimisticMutation.test.tsx new file mode 100644 index 0000000..533e852 --- /dev/null +++ b/frontend/src/__tests__/useOptimisticMutation.test.tsx @@ -0,0 +1,125 @@ +import React from 'react' +import { describe, it, expect, vi } from 'vitest' +import { renderHook, act, waitFor } from '@testing-library/react' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' + +import { useOptimisticMutation } from '../useOptimisticMutation' + +interface Settings { + enabled: boolean + label: string +} + +const KEY = 'test-settings' + +function setup() { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + client.setQueryData([KEY], { enabled: false, label: 'unchanged' }) + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} + ) + return { client, wrapper } +} + +describe('useOptimisticMutation', () => { + it('shows the new value before the server answers', async () => { + const { client, wrapper } = setup() + let resolve: (v: unknown) => void = () => {} + const mutationFn = vi.fn(() => new Promise((r) => { resolve = r })) + + const { result } = renderHook( + () => + useOptimisticMutation({ + mutationFn, + cacheKey: KEY, + optimistic: (current, enabled) => (current ? { ...current, enabled } : current), + }), + { wrapper }, + ) + + act(() => result.current.mutate(true)) + + // The point of the exercise: the cache reflects the change while the request + // is still in flight, so a control bound to it does not snap back. + await waitFor(() => + expect(client.getQueryData([KEY])?.enabled).toBe(true), + ) + expect(mutationFn).toHaveBeenCalledTimes(1) + + await act(async () => { resolve({ ok: true }) }) + }) + + it('restores the previous value when the write fails', async () => { + const { client, wrapper } = setup() + const onError = vi.fn() + + const { result } = renderHook( + () => + useOptimisticMutation({ + mutationFn: () => Promise.reject(new Error('server said no')), + cacheKey: KEY, + optimistic: (current, enabled) => (current ? { ...current, enabled } : current), + onError, + }), + { wrapper }, + ) + + act(() => result.current.mutate(true)) + + await waitFor(() => expect(onError).toHaveBeenCalled()) + // Rolled back in full, not merely to a default. + expect(client.getQueryData([KEY])).toEqual({ + enabled: false, + label: 'unchanged', + }) + }) + + it('leaves an absent cache entry absent rather than inventing one', async () => { + // The rollback restores `undefined` when there was nothing cached. Writing a + // partial object instead would hand components a half-populated payload. + const { client, wrapper } = setup() + client.removeQueries({ queryKey: [KEY] }) + const onError = vi.fn() + + const { result } = renderHook( + () => + useOptimisticMutation({ + mutationFn: () => Promise.reject(new Error('nope')), + cacheKey: KEY, + optimistic: (current, enabled) => (current ? { ...current, enabled } : current), + onError, + }), + { wrapper }, + ) + + act(() => result.current.mutate(true)) + + await waitFor(() => expect(onError).toHaveBeenCalled()) + expect(client.getQueryData([KEY])).toBeUndefined() + }) + + it('invalidates the entry and its companions once settled', async () => { + const { client, wrapper } = setup() + const invalidate = vi.spyOn(client, 'invalidateQueries') + + const { result } = renderHook( + () => + useOptimisticMutation({ + mutationFn: () => Promise.resolve({ ok: true }), + cacheKey: KEY, + invalidates: ['companion'], + optimistic: (current, enabled) => (current ? { ...current, enabled } : current), + }), + { wrapper }, + ) + + await act(async () => { await result.current.mutateAsync(true) }) + + const keys = invalidate.mock.calls.map((c) => (c[0] as { queryKey: string[] }).queryKey[0]) + expect(keys).toContain(KEY) + expect(keys).toContain('companion') + }) +}) diff --git a/frontend/src/cacheKeys.ts b/frontend/src/cacheKeys.ts new file mode 100644 index 0000000..0f90472 --- /dev/null +++ b/frontend/src/cacheKeys.ts @@ -0,0 +1,53 @@ +/** + * One cache key per backend endpoint. + * + * These were bare string literals spread across pages, and three endpoints ended + * up cached under two keys each: getFileTree as both 'file-tree' and + * 'files-tree', getLLMPreferences as 'llm-preferences' and 'llm-prefs', and + * getOcrStatus as 'ocr-status' and 'ocr-status-settings'. Each split meant two + * fetches of the same payload and, worse, invalidation that never crossed: + * removing a folder index in Explorer invalidated only its own spelling, so the + * Search page kept offering the deleted folder until a reload. + * + * Import from here rather than writing the string, so a typo is a build error + * instead of a second cache. + */ +export const CACHE_KEYS = { + appConfig: 'app-config', + driveInfo: 'drive-info', + fileTree: 'file-tree', + health: 'health', + indexStatus: 'index-status', + insights: 'insights', + llmPreferences: 'llm-preferences', + localModels: 'local-models', + ocrStatus: 'ocr-status', + ocrTiers: 'ocr-tiers', + ocrVlmModels: 'ocr-vlm-models', + ocrVlmSelection: 'ocr-vlm-selection', + providerSettings: 'provider-settings', + providersList: 'providers-list', + queryHistory: 'query-history', + systemInfo: 'system-info', +} as const + +export type CacheKey = (typeof CACHE_KEYS)[keyof typeof CACHE_KEYS] + +/** + * The views derived from the indexed corpus, so they all go stale together when + * indexing runs, the index is cleared, or the user asks for a refresh. + * + * Deliberately excludes provider, model and OCR-tier state: those describe the + * machine's configuration, not its contents, and an index run cannot change them. + */ +export const CORPUS_DERIVED_KEYS: readonly CacheKey[] = [ + CACHE_KEYS.fileTree, + CACHE_KEYS.insights, + CACHE_KEYS.indexStatus, + CACHE_KEYS.health, + CACHE_KEYS.systemInfo, + CACHE_KEYS.ocrStatus, +] + +/** Per-provider launch status. Not a fixed key, so it gets a builder. */ +export const launchStatusKey = (providerId: string) => `launch-status-${providerId}` diff --git a/frontend/src/components/AppShell.tsx b/frontend/src/components/AppShell.tsx index 4593684..2c6c148 100644 --- a/frontend/src/components/AppShell.tsx +++ b/frontend/src/components/AppShell.tsx @@ -5,6 +5,7 @@ import { getAppConfig, getHealth } from '../api' import { useEffect } from 'react' import { useQueryClient } from '@tanstack/react-query' import { useSessionProvider } from '../context/SessionProviderContext' +import { CACHE_KEYS } from '../cacheKeys' const navItems = [ { to: '/library', label: 'Library', icon: BookOpen }, @@ -26,11 +27,11 @@ export function AppShell() { // Poll faster while a sync is in progress so the banner dismisses quickly const { data: health } = useApi(getHealth, { - cacheKey: 'health', + cacheKey: CACHE_KEYS.health, refetchInterval: isSyncing ? 5_000 : 60_000, }) const { data: appConfig } = useApi(getAppConfig, { - cacheKey: 'app-config', + cacheKey: CACHE_KEYS.appConfig, refetchInterval: 60_000 }) diff --git a/frontend/src/components/providers/ModelPicker.tsx b/frontend/src/components/providers/ModelPicker.tsx index f2ceaf8..fc5cc77 100644 --- a/frontend/src/components/providers/ModelPicker.tsx +++ b/frontend/src/components/providers/ModelPicker.tsx @@ -4,12 +4,13 @@ import { getProviders } from '../../api'; import type { ProviderStatus } from '../../api'; import { useSessionProvider } from '../../context/SessionProviderContext'; import { Sparkles, Search, X, Check } from 'lucide-react'; +import { CACHE_KEYS } from '../../cacheKeys' // STATIC_FALLBACK_MODELS removed in favor of dynamic backend discovery and persistent model heaps. // const STATIC_FALLBACK_MODELS: Record = { ... }; export function ModelPicker() { - const { data: providers, refetch: refreshProviders } = useApi(getProviders, { cacheKey: 'providers-list' }); + const { data: providers, refetch: refreshProviders } = useApi(getProviders, { cacheKey: CACHE_KEYS.providersList }); const { sessionModelOverride, setSessionModelOverride } = useSessionProvider(); const [isOpen, setIsOpen] = useState(false); diff --git a/frontend/src/pages/ExplorerPage.tsx b/frontend/src/pages/ExplorerPage.tsx index 6ada5be..dccbf38 100644 --- a/frontend/src/pages/ExplorerPage.tsx +++ b/frontend/src/pages/ExplorerPage.tsx @@ -1,9 +1,11 @@ import { useState, useMemo } from 'react' import { useNavigate } from 'react-router-dom' import { FolderTree, File, Folder, ChevronRight, ChevronDown, Loader2, LayoutGrid, List, Trash2, Search, Download, Bot, ScanText } from 'lucide-react' -import { useApi, invalidateCache } from '../useApi' -import { getFileTree, removeFolderIndex, getOcrStatus, forceOcr, type FileEntry } from '../api' +import { useApi } from '../useApi' +import { getFileTree, removeFolderIndex, getOcrStatus, forceOcr, type FileEntry, type FileTree } from '../api' import { FileTypeTreemap } from '../components/FileTypeTreemap' +import { CACHE_KEYS } from '../cacheKeys' +import { useOptimisticMutation } from '../useOptimisticMutation' function formatSize(bytes: number): string { if (bytes < 1024) return `${bytes} B` @@ -29,13 +31,17 @@ interface FolderNodeProps { readonly onSelect: (file: FileEntry) => void readonly selectedPath: string | null readonly onDeleteFolder: (path: string) => void + readonly deletingPath: string | null } -function FolderNode({ node, depth, onSelect, selectedPath, onDeleteFolder }: FolderNodeProps) { +function FolderNode({ node, depth, onSelect, selectedPath, onDeleteFolder, deletingPath }: FolderNodeProps) { const [open, setOpen] = useState(depth === 0) + const isDeleting = deletingPath === node.fullPath + const handleDelete = (e: React.MouseEvent) => { e.stopPropagation() + if (isDeleting) return if (confirm(`Are you sure you want to remove the index for this folder and all its contents?\n\nPath: ${node.fullPath}`)) { onDeleteFolder(node.fullPath) } @@ -64,10 +70,11 @@ function FolderNode({ node, depth, onSelect, selectedPath, onDeleteFolder }: Fol @@ -84,6 +91,7 @@ function FolderNode({ node, depth, onSelect, selectedPath, onDeleteFolder }: Fol onSelect={onSelect} selectedPath={selectedPath} onDeleteFolder={onDeleteFolder} + deletingPath={deletingPath} /> )) } @@ -122,7 +130,7 @@ function FolderNode({ node, depth, onSelect, selectedPath, onDeleteFolder }: Fol /* ── Main Explorer Page ─────────────────────────────────── */ export function ExplorerPage() { - const { data: tree, loading, refetch } = useApi(getFileTree, { cacheKey: 'file-tree' }) + const { data: tree, loading, refetch } = useApi(getFileTree, { cacheKey: CACHE_KEYS.fileTree }) const [selectedFile, setSelectedFile] = useState(null) const [viewMode, setViewMode] = useState<'tree' | 'treemap'>('tree') const [activeExtension, setActiveExtension] = useState(null) @@ -131,7 +139,7 @@ export function ExplorerPage() { const [ocrMessage, setOcrMessage] = useState('') const navigate = useNavigate() - const { data: ocr } = useApi(getOcrStatus, { cacheKey: 'ocr-status' }) + const { data: ocr } = useApi(getOcrStatus, { cacheKey: CACHE_KEYS.ocrStatus }) const ocrReady = !!ocr?.installed && !!ocr?.enabled const handleForceOcr = async (path: string) => { @@ -174,18 +182,38 @@ export function ExplorerPage() { URL.revokeObjectURL(url) } - const handleDeleteFolder = async (path: string) => { - try { - await removeFolderIndex([path]) - invalidateCache('file-tree') - invalidateCache('insights') - refetch() + // The folder disappears from the tree on click rather than after the round + // trip. Previously this had no pending state at all, so the row stayed fully + // interactive and a double-click sent two removal requests. + const deleteFolder = useOptimisticMutation({ + mutationFn: (path: string) => removeFolderIndex([path]), + cacheKey: CACHE_KEYS.fileTree, + invalidates: [CACHE_KEYS.insights], + optimistic: (current, path) => { + if (!current?.folders?.[path]) return current + const { [path]: removed, ...rest } = current.folders + return { + ...current, + folders: rest, + total_files: Math.max(0, current.total_files - removed.length), + total_size: Math.max(0, current.total_size - removed.reduce((n, f) => n + f.size, 0)), + } + }, + onSuccess: (_data, path) => { if (selectedFile?.path.startsWith(path)) setSelectedFile(null) - alert(`Successfully removed index for: ${path}`) - } catch (e) { + }, + onError: (e) => { alert(`Failed to delete folder index: ${e instanceof Error ? e.message : 'Unknown error'}`) - } + }, + }) + + // Guarded rather than just disabling the button: the treemap has its own + // delete affordance, and useMutation does not dedupe concurrent calls. + const handleDeleteFolder = (path: string) => { + if (deleteFolder.isPending) return + deleteFolder.mutate(path) } + const deletingPath = deleteFolder.isPending ? deleteFolder.variables : null const hierarchicalTree = useMemo(() => { if (!tree?.folders) return null @@ -276,7 +304,11 @@ export function ExplorerPage() { const isEmptyTree = !hierarchicalTree || hierarchicalTree.length === 0; const renderMainContent = () => { - if (loading) { + // `&& !tree` matters: useApi reports `isLoading || isFetching`, so this is + // true on every background refetch too - and with refetchOnWindowFocus on, + // alt-tabbing back replaced the whole explorer with a spinner. Only show it + // when there is genuinely nothing to display yet. + if (loading && !tree) { return (
@@ -304,6 +336,7 @@ export function ExplorerPage() { onSelect={setSelectedFile} selectedPath={selectedFile?.path ?? null} onDeleteFolder={handleDeleteFolder} + deletingPath={deletingPath} /> ))}
diff --git a/frontend/src/pages/InsightsPage.tsx b/frontend/src/pages/InsightsPage.tsx index 19f1699..7056f20 100644 --- a/frontend/src/pages/InsightsPage.tsx +++ b/frontend/src/pages/InsightsPage.tsx @@ -3,6 +3,7 @@ import { BarChart3, PieChart, TrendingUp, FileType, Loader2, Flame, Snowflake, H import { useApi } from '../useApi' import { getInsights, getInsightsByType, getFileTree } from '../api' import { KnowledgePortrait } from '../components/KnowledgePortrait' +import { CACHE_KEYS } from '../cacheKeys' const WebGPUFallback = lazy(() => import('../components/WebGPUFallback').then(m => ({ default: m.WebGPUFallback }))) const FileTypeTreemap = lazy(() => import('../components/FileTypeTreemap').then(m => ({ default: m.FileTypeTreemap }))) @@ -15,8 +16,8 @@ function formatBytes(bytes: number): string { } export function InsightsPage() { - const { data: insights, loading: insightsLoading, error } = useApi(getInsights, { cacheKey: 'insights' }) - const { data: tree, loading: treeLoading } = useApi(getFileTree, { cacheKey: 'file-tree' }) + const { data: insights, loading: insightsLoading, error } = useApi(getInsights, { cacheKey: CACHE_KEYS.insights }) + const { data: tree, loading: treeLoading } = useApi(getFileTree, { cacheKey: CACHE_KEYS.fileTree }) const [typeFilter, setTypeFilter] = useState(null) const [filteredTopFiles, setFilteredTopFiles] = useState<{ path: string; size: number }[]>([]) const [filteredColdFiles, setFilteredColdFiles] = useState<{ path: string; usage_count?: number; size?: number }[]>([]) diff --git a/frontend/src/pages/LibraryPage.tsx b/frontend/src/pages/LibraryPage.tsx index 79849f4..4e27b87 100644 --- a/frontend/src/pages/LibraryPage.tsx +++ b/frontend/src/pages/LibraryPage.tsx @@ -1,6 +1,7 @@ import { useState, useCallback, useEffect } from 'react' +import { useMutation } from '@tanstack/react-query' import { BookOpen, HardDrive, FolderPlus, RefreshCw, Loader2, CheckCircle2, AlertCircle, Play, Trash2, ScanText } from 'lucide-react' -import { useApi, invalidateCache } from '../useApi' +import { useApi, invalidateCorpusCaches } from '../useApi' import { getHealth, getIndexStatus, @@ -16,6 +17,7 @@ import { clearBackendCaches, type IndexStatus, } from '../api' +import { CACHE_KEYS } from '../cacheKeys' export function LibraryPage() { const [folderPath, setFolderPath] = useState('') @@ -26,16 +28,16 @@ export function LibraryPage() { // Pause /index/status polling while local "indexing" is true; SSE drives live progress. const { data: status, refetch: refetchStatus } = useApi(getIndexStatus, { - cacheKey: 'index-status', + cacheKey: CACHE_KEYS.indexStatus, refetchInterval: indexing ? 0 : 10_000, }) - const { data: sysInfo } = useApi(getSystemInfo, { cacheKey: 'system-info' }) - const { data: config } = useApi(getAppConfig, { cacheKey: 'app-config' }) + const { data: sysInfo } = useApi(getSystemInfo, { cacheKey: CACHE_KEYS.systemInfo }) + const { data: config } = useApi(getAppConfig, { cacheKey: CACHE_KEYS.appConfig }) // OCR runs after the index run finishes, so this keeps polling regardless of // indexing state. Without it "indexing complete" is a lie for scanned PDFs. const { data: ocr } = useApi(getOcrStatus, { - cacheKey: 'ocr-status', + cacheKey: CACHE_KEYS.ocrStatus, refetchInterval: 10_000, }) @@ -44,7 +46,7 @@ export function LibraryPage() { // Pause background polling while SSE stream is active const { data: health, refetch: refetchHealth } = useApi(getHealth, { - cacheKey: 'health', + cacheKey: CACHE_KEYS.health, refetchInterval: isRunning ? 0 : 10_000 }) @@ -63,7 +65,7 @@ export function LibraryPage() { setIndexing(false) setCancelling(false) setLiveProgress(null) - invalidateCache() + invalidateCorpusCaches() refetchHealth() refetchStatus() setMessage({ type: 'ok', text: `Indexing complete — ${data.processed_files} files processed` }) @@ -105,19 +107,27 @@ export function LibraryPage() { } }, [isRunning, cancelling]) - const handleClear = useCallback(async () => { - if (isRunning) return - if (!confirm('This will permanently delete ALL indexed data. Continue?')) return - try { - await clearIndex() - invalidateCache() + // Neither of these has optimistic state worth showing - what they need is a + // pending one. Both used to run with no indication at all, so the button + // stayed live and a second click sent a second request. + const clearIndexMutation = useMutation({ + mutationFn: clearIndex, + onSuccess: () => { + invalidateCorpusCaches() refetchHealth() refetchStatus() setMessage({ type: 'ok', text: 'All indexed data cleared' }) - } catch (e) { + }, + onError: (e) => { setMessage({ type: 'err', text: e instanceof Error ? e.message : 'Clear failed' }) - } - }, [refetchHealth, refetchStatus, isRunning]) + }, + }) + + const handleClear = useCallback(() => { + if (isRunning || clearIndexMutation.isPending) return + if (!confirm('This will permanently delete ALL indexed data. Continue?')) return + clearIndexMutation.mutate() + }, [isRunning, clearIndexMutation]) @@ -132,21 +142,26 @@ export function LibraryPage() { } }, [isRunning]) - const handleRefresh = useCallback(async () => { - try { - await clearBackendCaches() - invalidateCache() - refetchHealth() - refetchStatus() - setMessage({ type: 'ok', text: 'Data refreshed successfully' }) - } catch (e) { - console.error('Failed to clear backend caches:', e) - invalidateCache() + const refreshMutation = useMutation({ + mutationFn: clearBackendCaches, + // The local refresh happens either way: a backend cache that refuses to + // clear is not a reason to leave the user looking at stale numbers. + onSettled: (_data, error) => { + if (error) console.error('Failed to clear backend caches:', error) + invalidateCorpusCaches() refetchHealth() refetchStatus() - setMessage({ type: 'ok', text: 'Local data refreshed' }) - } - }, [refetchHealth, refetchStatus]) + setMessage({ + type: 'ok', + text: error ? 'Local data refreshed' : 'Data refreshed successfully', + }) + }, + }) + + const handleRefresh = useCallback(() => { + if (refreshMutation.isPending) return + refreshMutation.mutate() + }, [refreshMutation]) const filesIndexed = status?.files_indexed ?? 0 const chunksIndexed = status?.chunks_indexed ?? 0 @@ -167,7 +182,7 @@ export function LibraryPage() {

- @@ -359,11 +374,13 @@ export function LibraryPage() {
diff --git a/frontend/src/pages/ProvidersPage.tsx b/frontend/src/pages/ProvidersPage.tsx index 95e80f1..ed92186 100644 --- a/frontend/src/pages/ProvidersPage.tsx +++ b/frontend/src/pages/ProvidersPage.tsx @@ -5,6 +5,7 @@ import { setLLMPreferences, getLLMPreferences, getProviderSettings, + type ProviderRoutingSettings, setProviderSettings, } from '../api'; import type { @@ -31,31 +32,43 @@ import { Zap, } from 'lucide-react'; import { useNavigate } from 'react-router-dom'; +import { CACHE_KEYS } from '../cacheKeys' +import { useOptimisticMutation } from '../useOptimisticMutation' export function ProvidersPage() { const navigate = useNavigate(); - const { data: providers, refetch: refetchProviders, loading } = useApi(getProviders, { cacheKey: 'providers-list' }); - const { data: prefs, refetch: refetchPrefs } = useApi(getLLMPreferences, { cacheKey: 'llm-preferences' }); - const { data: routingSettings, refetch: refetchSettings } = useApi(getProviderSettings, { cacheKey: 'provider-settings' }); + const { data: providers, refetch: refetchProviders, loading } = useApi(getProviders, { cacheKey: CACHE_KEYS.providersList }); + const { data: prefs, refetch: refetchPrefs } = useApi(getLLMPreferences, { cacheKey: CACHE_KEYS.llmPreferences }); + const { data: routingSettings, refetch: refetchSettings } = useApi(getProviderSettings, { cacheKey: CACHE_KEYS.providerSettings }); + + // Both of these write provider settings, so they share one cache entry and one + // optimistic path: the list reorders and the checkbox moves on click instead + // of waiting for the round trip and the refetch behind it. + const updateRoutingSettings = useOptimisticMutation< + Partial, + unknown, + ProviderRoutingSettings + >({ + mutationFn: setProviderSettings, + cacheKey: CACHE_KEYS.providerSettings, + optimistic: (current, patch) => (current ? { ...current, ...patch } : current), + }); - const handleMoveFallback = async (index: number, direction: number) => { + const handleMoveFallback = (index: number, direction: number) => { if (!routingSettings) return; const newChain = [...routingSettings.fallback_chain]; const targetIndex = index + direction; if (targetIndex < 0 || targetIndex >= newChain.length) return; - + // Swap const temp = newChain[index]; newChain[index] = newChain[targetIndex]; newChain[targetIndex] = temp; - - await setProviderSettings({ + + updateRoutingSettings.mutate({ provider: routingSettings.provider, - fallback_chain: newChain + fallback_chain: newChain, }); - - invalidateCache('provider-settings'); - refetchSettings(); }; const [selectedId, setSelectedId] = useState('gemini'); @@ -429,10 +442,9 @@ export function ProvidersPage() { { - await setProviderSettings({ cloud_privacy_consent: e.target.checked }); - invalidateCache('provider-settings'); - refetchSettings(); + disabled={updateRoutingSettings.isPending} + onChange={e => { + updateRoutingSettings.mutate({ cloud_privacy_consent: e.target.checked }); }} className="rounded border-amber-500/40" /> diff --git a/frontend/src/pages/SearchPage.tsx b/frontend/src/pages/SearchPage.tsx index aea6eca..5e7982f 100644 --- a/frontend/src/pages/SearchPage.tsx +++ b/frontend/src/pages/SearchPage.tsx @@ -9,6 +9,7 @@ import { MessageBubble } from '../components/chat/MessageBubble'; import { FilterBar } from '../components/chat/FilterBar'; import { ModelPicker } from '../components/providers/ModelPicker'; import { useDreamscapeStore } from '../store/dreamscapeStore'; +import { CACHE_KEYS } from '../cacheKeys' export function SearchPage() { const selectedChunks = useDreamscapeStore(state => state.selectedChunks); @@ -22,10 +23,10 @@ export function SearchPage() { const [selectedFolderTag, setSelectedFolderTag] = useState(''); const [selectedMode, setSelectedMode] = useState(''); - const { data: historyData, refetch: refetchHistory } = useApi(getQueryHistory, { cacheKey: 'query-history' }); + const { data: historyData, refetch: refetchHistory } = useApi(getQueryHistory, { cacheKey: CACHE_KEYS.queryHistory }); const { messages, executeSearch, resetChat: resetChatStream, stopStream } = useChatStream(() => { - invalidateCache('query-history'); + invalidateCache(CACHE_KEYS.queryHistory); refetchHistory(); }); @@ -35,7 +36,7 @@ export function SearchPage() { // already pushes those events. A fixed 15s poll re-fetched the whole tree // forever on a corpus that had not moved. const { data: fileTree, refetch: refetchFileTree } = useApi(getFileTree, { - cacheKey: 'files-tree', + cacheKey: CACHE_KEYS.fileTree, }); useEffect(() => { @@ -45,7 +46,7 @@ export function SearchPage() { // frame - those arrive continuously during a scan. const status = evt?.status ?? ''; if (status !== last && (status === 'completed' || status === 'idle')) { - invalidateCache('files-tree'); + invalidateCache(CACHE_KEYS.fileTree); refetchFileTree(); } last = status; @@ -109,7 +110,7 @@ export function SearchPage() { const doClear = async () => { try { const res = await clearQueryHistory(); - invalidateCache('query-history'); + invalidateCache(CACHE_KEYS.queryHistory); refetchHistory(); resetChatStream(); // The backend reports whether the persistent semantic cache went with diff --git a/frontend/src/pages/SettingsPage.tsx b/frontend/src/pages/SettingsPage.tsx index 016959e..cb7c976 100644 --- a/frontend/src/pages/SettingsPage.tsx +++ b/frontend/src/pages/SettingsPage.tsx @@ -15,6 +15,7 @@ import { getProviderLaunchStatus, launchProvider, getOcrStatus, + type OcrStatus, getOcrTiers, selectOcrTier, getVlmModels, @@ -37,6 +38,8 @@ import { type OcrInstallState, type OcrQueueItem } from '../api' +import { CACHE_KEYS } from '../cacheKeys' +import { useOptimisticMutation } from '../useOptimisticMutation' // STATIC_FALLBACK_MODELS removed in favor of dynamic backend discovery and persistent model heaps. @@ -68,8 +71,8 @@ function StartLocalProviderButton({ providerId, displayName, offlineHint, onStar try { const res = await launchProvider(providerId) if (res.ok) { - invalidateCache('local-models') - invalidateCache('providers-list') + invalidateCache(CACHE_KEYS.localModels) + invalidateCache(CACHE_KEYS.providersList) onStarted() } else { setError(res.message) @@ -231,8 +234,8 @@ function LLMPreferencesSection({ saving: boolean }>) { const navigate = useNavigate() - const { data: llmPrefs } = useApi(getLLMPreferences, { cacheKey: 'llm-prefs' }) - const { data: providers } = useApi(getProviders, { cacheKey: 'providers-list' }) + const { data: llmPrefs } = useApi(getLLMPreferences, { cacheKey: CACHE_KEYS.llmPreferences }) + const { data: providers } = useApi(getProviders, { cacheKey: CACHE_KEYS.providersList }) const [provider, setProvider] = useState('auto') const [selectedModels, setSelectedModels] = useState>({}) @@ -395,9 +398,9 @@ function VlmPicker({ onChanged, setNote, }: Readonly<{ onChanged: () => void; setNote: (s: string) => void }>) { - const { data, loading, refetch } = useApi(getVlmModels, { cacheKey: 'ocr-vlm-models' }) + const { data, loading, refetch } = useApi(getVlmModels, { cacheKey: CACHE_KEYS.ocrVlmModels }) const { data: current, refetch: refetchSelection } = useApi(getVlmSelection, { - cacheKey: 'ocr-vlm-selection', + cacheKey: CACHE_KEYS.ocrVlmSelection, }) const [saving, setSaving] = useState('') const [isChecking, setIsChecking] = useState(false) @@ -412,8 +415,8 @@ function VlmPicker({ return } setNote(`OCR will use ${model}.`) - invalidateCache('ocr-tiers') - invalidateCache('ocr-status-settings') + invalidateCache(CACHE_KEYS.ocrTiers) + invalidateCache(CACHE_KEYS.ocrStatus) refetchSelection() onChanged() } catch (e) { @@ -425,7 +428,7 @@ function VlmPicker({ const handleCheckAgain = async () => { setIsChecking(true) - invalidateCache('ocr-vlm-models') + invalidateCache(CACHE_KEYS.ocrVlmModels) try { await refetch() } finally { @@ -440,8 +443,8 @@ function VlmPicker({ const res = await launchProvider(providerId) if (res.ok) { setNote(`${displayName} started successfully.`) - invalidateCache('ocr-vlm-models') - invalidateCache('local-models') + invalidateCache(CACHE_KEYS.ocrVlmModels) + invalidateCache(CACHE_KEYS.localModels) await refetch() } else { setNote(res.message || `Could not start ${displayName}.`) @@ -589,10 +592,10 @@ function VlmPicker({ function OcrSection() { const { data: ocr, refetch } = useApi(getOcrStatus, { - cacheKey: 'ocr-status-settings', + cacheKey: CACHE_KEYS.ocrStatus, refetchInterval: 10_000, }) - const { data: tierData, refetch: refetchTiers } = useApi(getOcrTiers, { cacheKey: 'ocr-tiers' }) + const { data: tierData, refetch: refetchTiers } = useApi(getOcrTiers, { cacheKey: CACHE_KEYS.ocrTiers }) const tiers = tierData?.tiers ?? [] const [selectedTier, setSelectedTier] = useState('cpu') const selectedTierInfo = tiers.find((t) => t.id === selectedTier) @@ -621,8 +624,8 @@ function OcrSection() { const state = await getOcrInstallState() setInstall(state) if (state.status !== 'running') { - invalidateCache('ocr-tiers') - invalidateCache('ocr-status-settings') + invalidateCache(CACHE_KEYS.ocrTiers) + invalidateCache(CACHE_KEYS.ocrStatus) refetch() refetchTiers() setBusy(false) @@ -667,8 +670,8 @@ function OcrSection() { return } setNote(`OCR switched to ${TIER_COPY[tier]?.short ?? tier}.`) - invalidateCache('ocr-tiers') - invalidateCache('ocr-status-settings') + invalidateCache(CACHE_KEYS.ocrTiers) + invalidateCache(CACHE_KEYS.ocrStatus) refetch() refetchTiers() } catch (e) { @@ -685,8 +688,8 @@ function OcrSection() { await uninstallOcrTier(target) setInstall(null) setNote(`Uninstalled ${TIER_COPY[target]?.short ?? target} tier.`) - invalidateCache('ocr-tiers') - invalidateCache('ocr-status-settings') + invalidateCache(CACHE_KEYS.ocrTiers) + invalidateCache(CACHE_KEYS.ocrStatus) refetch() refetchTiers() } catch (e) { @@ -696,16 +699,25 @@ function OcrSection() { } } - const handleToggle = async (enabled: boolean) => { - try { - const res = await setOcrEnabled(enabled) + // The checkbox reads `checked={!!ocr?.enabled}` straight off server data, so + // before this it visibly snapped back to its old position and stayed there + // until the refetch landed. The optimistic write moves it on click and the + // rollback puts it back only if the server actually refuses. + const toggleOcr = useOptimisticMutation>, OcrStatus>({ + mutationFn: setOcrEnabled, + cacheKey: CACHE_KEYS.ocrStatus, + invalidates: [CACHE_KEYS.ocrTiers], + optimistic: (current, enabled) => (current ? { ...current, enabled } : current), + onSuccess: (res) => { + // A 200 that says "no" is not an error, so it does not roll back. The + // onSettled invalidation refetches and the real value wins; this only + // explains why. if (!res.ok) setNote(`Cannot enable: ${res.error_code}`) - invalidateCache('ocr-tiers') - invalidateCache('ocr-status-settings') - refetch() - refetchTiers() - } catch (e) { setNote(e instanceof Error ? e.message : 'Could not change OCR state.') } - } + }, + onError: (e) => setNote(e instanceof Error ? e.message : 'Could not change OCR state.'), + }) + + const handleToggle = (enabled: boolean) => toggleOcr.mutate(enabled) const handleResume = async () => { try { @@ -1133,10 +1145,10 @@ function ResetSection({ onRestartOnboarding, onFullReset }: Readonly<{ onRestart // ── Main Page Component ────────────────────────────────────────────── export function SettingsPage() { - const { data: localModels, refetch: refetchLocalModels } = useApi(getLocalModels, { cacheKey: 'local-models' }) - const { data: sysInfo } = useApi(getSystemInfo, { cacheKey: 'system-info' }) - const { refetch: refetchPrefs } = useApi(getLLMPreferences, { cacheKey: 'llm-prefs' }) - const { data: driveInfo } = useApi(getDriveInfo, { cacheKey: 'drive-info' }) + const { data: localModels, refetch: refetchLocalModels } = useApi(getLocalModels, { cacheKey: CACHE_KEYS.localModels }) + const { data: sysInfo } = useApi(getSystemInfo, { cacheKey: CACHE_KEYS.systemInfo }) + const { refetch: refetchPrefs } = useApi(getLLMPreferences, { cacheKey: CACHE_KEYS.llmPreferences }) + const { data: driveInfo } = useApi(getDriveInfo, { cacheKey: CACHE_KEYS.driveInfo }) const [message, setMessage] = useState<{ type: 'ok' | 'err'; text: string } | null>(null) const [savingPrefs, setSavingPrefs] = useState(false) @@ -1154,8 +1166,8 @@ export function SettingsPage() { await setProviderDefaultModel(providerId, val).catch(() => {}) } } - invalidateCache('llm-prefs') - invalidateCache('providers-list') + invalidateCache(CACHE_KEYS.llmPreferences) + invalidateCache(CACHE_KEYS.providersList) refetchPrefs() setMessage({ type: 'ok', text: 'LLM preferences saved.' }) } catch (e) { diff --git a/frontend/src/pages/SetupPage.tsx b/frontend/src/pages/SetupPage.tsx index 509046d..b4d17b3 100644 --- a/frontend/src/pages/SetupPage.tsx +++ b/frontend/src/pages/SetupPage.tsx @@ -3,6 +3,7 @@ import { Brain, Shield, ArrowRight, CheckCircle2, ChevronRight, HardDrive, Alert import { useNavigate } from 'react-router-dom' import { useApi } from '../useApi' import { getLocalModels, getDriveInfo, enableSplitBrain, getProviders, setProviderKey } from '../api' +import { CACHE_KEYS } from '../cacheKeys' const PROVIDERS = [ { id: 'gemini', name: 'Google Gemini', icon: '✨' }, @@ -12,7 +13,7 @@ const PROVIDERS = [ ] function ApiKeyInput({ provider }: { provider: typeof PROVIDERS[0] }) { - const { data: providers, refetch } = useApi(getProviders, { cacheKey: 'providers-list' }) + const { data: providers, refetch } = useApi(getProviders, { cacheKey: CACHE_KEYS.providersList }) const pData = providers?.find(p => p.spec.id === provider.id) const [key, setKey] = useState('') const [saving, setSaving] = useState(false) @@ -84,9 +85,9 @@ function ApiKeyInput({ provider }: { provider: typeof PROVIDERS[0] }) { export function SetupPage() { const navigate = useNavigate() - const { data: providers, refetch: refetchProviders, loading: providersLoading } = useApi(getProviders, { cacheKey: 'providers-list' }) - const { data: localModels, loading: localModelsLoading } = useApi(getLocalModels, { cacheKey: 'local-models' }) - const { data: driveInfo, loading: driveLoading } = useApi(getDriveInfo, { cacheKey: 'drive-info' }) + const { data: providers, refetch: refetchProviders, loading: providersLoading } = useApi(getProviders, { cacheKey: CACHE_KEYS.providersList }) + const { data: localModels, loading: localModelsLoading } = useApi(getLocalModels, { cacheKey: CACHE_KEYS.localModels }) + const { data: driveInfo, loading: driveLoading } = useApi(getDriveInfo, { cacheKey: CACHE_KEYS.driveInfo }) const isLoading = providersLoading || localModelsLoading || driveLoading @@ -181,7 +182,14 @@ export function SetupPage() { )} - {!isLoading && !isDriveConfigSafe && driveInfo && !requiresRestart && ( + {/* Gated on data, not on fetching state. `isLoading` is + `isLoading || isFetching`, and the focus/visibilitychange + listeners above refetch on every alt-tab - so `!isLoading` + made this storage-compatibility warning blink out exactly + when the user returned to the page. `driveInfo` presence is + the real guard, and isDriveConfigSafe already defaults to + true while it is absent. */} + {!isDriveConfigSafe && driveInfo && !requiresRestart && (
@@ -230,7 +238,9 @@ export function SetupPage() { )} {/* Drive OK badge — shown when split_brain is active on portable drive */} - {!isLoading && isDriveConfigSafe && driveInfo?.is_portable_fs && ( + {/* Same reasoning as the warning above: `driveInfo?.is_portable_fs` + already requires the data to have arrived. */} + {isDriveConfigSafe && driveInfo?.is_portable_fs && (

diff --git a/frontend/src/providers/ProviderRecipes.tsx b/frontend/src/providers/ProviderRecipes.tsx index c26d7ed..195cd72 100644 --- a/frontend/src/providers/ProviderRecipes.tsx +++ b/frontend/src/providers/ProviderRecipes.tsx @@ -2,6 +2,7 @@ import { Rocket, Zap, Shield, Star, X, Loader2 } from 'lucide-react'; import { useState } from 'react'; import { setProviderSettings, setLLMPreferences, getLLMPreferences } from '../api'; import { invalidateCache } from '../useApi'; +import { CACHE_KEYS } from '../cacheKeys' export function ProviderRecipes({ onRecipeApplied, @@ -32,8 +33,8 @@ export function ProviderRecipes({ [`${defaultModel.provider}_model`]: defaultModel.model, }); - invalidateCache('provider-settings'); - invalidateCache('llm-preferences'); + invalidateCache(CACHE_KEYS.providerSettings); + invalidateCache(CACHE_KEYS.llmPreferences); onRecipeApplied(); } catch (e: any) { setApplyError(e.message || 'Failed to apply recipe'); diff --git a/frontend/src/useApi.ts b/frontend/src/useApi.ts index e08453f..ab32fe4 100644 --- a/frontend/src/useApi.ts +++ b/frontend/src/useApi.ts @@ -1,5 +1,6 @@ import { useQuery } from '@tanstack/react-query' import { queryClient } from './queryClient' +import { CORPUS_DERIVED_KEYS } from './cacheKeys' import { useId } from 'react' interface UseApiOptions { @@ -48,12 +49,32 @@ export function useApi( } } -/** Invalidate cache entries matching a prefix query key */ -export function invalidateCache(prefix?: string) { - if (!prefix) { - queryClient.clear() - return - } +/** + * Invalidate cache entries matching a prefix query key. + * + * The prefix is required. It used to be optional, and calling with no argument + * ran `queryClient.clear()` — which *removes* every query rather than marking it + * stale (`getAll().forEach(q => this.remove(q))` in query-core 5.94.5). Every + * mounted observer therefore dropped to `data: undefined` and every page fell + * back to its cold-start spinner. Four call sites did that, including the one + * that fires when an index run finishes, so completing a scan blanked the whole + * app. Invalidation refetches while keeping the previous data on screen, which + * is what all of them wanted. + */ +export function invalidateCache(prefix: string) { // React Query fuzzy-matches keys, so passing ['prefix'] invalidates any key starting with ['prefix'] queryClient.invalidateQueries({ queryKey: [prefix] }) } + +/** + * Refresh everything derived from the indexed corpus. + * + * The replacement for the bare `invalidateCache()` calls: indexing, clearing and + * a manual refresh all change the same set of views, and naming that set beats + * evicting unrelated caches (providers, API keys, OCR tiers) to reach it. + */ +export function invalidateCorpusCaches() { + for (const key of CORPUS_DERIVED_KEYS) { + invalidateCache(key) + } +} diff --git a/frontend/src/useOptimisticMutation.ts b/frontend/src/useOptimisticMutation.ts new file mode 100644 index 0000000..bdefe0c --- /dev/null +++ b/frontend/src/useOptimisticMutation.ts @@ -0,0 +1,63 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query' + +interface Options { + /** The write itself. */ + mutationFn: (vars: TVars) => Promise + /** + * The cache entry this write changes. Updated immediately, restored if the + * server rejects. Matches `useApi`'s query key, which is `[cacheKey]`. + */ + cacheKey: string + /** Next cached value, given the current one. Return it unchanged to opt out. */ + optimistic: (current: TCache | undefined, vars: TVars) => TCache | undefined + /** Other keys the write invalidates once the server has answered. */ + invalidates?: readonly string[] + onError?: (error: unknown, vars: TVars) => void + onSuccess?: (data: TData, vars: TVars) => void +} + +/** + * A mutation that shows its result before the server confirms it. + * + * Every write in this app used to be a bare `await` followed by a refetch, so a + * control bound to server state (a checkbox reading `checked={!!ocr?.enabled}`, + * a list rendered from `prefs.fallback_chain`) visibly snapped back to the old + * value until the round-trip finished — and destructive actions with no pending + * state could be fired twice. + * + * The ceremony that makes this safe is easy to get wrong once, let alone four + * times, hence one helper: cancel in-flight refetches so a stale response cannot + * land on top of the optimistic value, snapshot for rollback, restore on error, + * and re-sync from the server either way. + */ +export function useOptimisticMutation( + opts: Options, +) { + const client = useQueryClient() + + return useMutation({ + mutationFn: opts.mutationFn, + onMutate: async (vars) => { + // Without this, a refetch already in flight can resolve after the + // optimistic write and overwrite it with the pre-mutation server state. + await client.cancelQueries({ queryKey: [opts.cacheKey] }) + const previous = client.getQueryData([opts.cacheKey]) + client.setQueryData([opts.cacheKey], (current) => + opts.optimistic(current, vars), + ) + return { previous } + }, + onError: (error, vars, context) => { + // Restore exactly what was there, including "nothing". + client.setQueryData([opts.cacheKey], context?.previous) + opts.onError?.(error, vars) + }, + onSuccess: opts.onSuccess, + onSettled: () => { + client.invalidateQueries({ queryKey: [opts.cacheKey] }) + for (const key of opts.invalidates ?? []) { + client.invalidateQueries({ queryKey: [key] }) + } + }, + }) +} From ebda9e73c592ed922a67a0f42858b33a97d1e3ac Mon Sep 17 00:00:00 2001 From: Binit Varghese Date: Mon, 24 Aug 2026 17:42:24 +0530 Subject: [PATCH 4/4] fix(frontend): drop the OCR hook the main merge brought back b89bc10 ("Merge branch 'main' into updates", the PR's Update-branch button) re-added the pre-616b966 getOcrStatus hook alongside the one 616b966 had just rewritten. 616b966 replaced the 'ocr-status' string literal with CACHE_KEYS.ocrStatus; main still carried the literal, and the two landed at non-overlapping offsets, so git merged them clean instead of raising a conflict. The result declared `ocr` twice and esbuild refused the file, taking LibraryPage.test.tsx out of collection with "The symbol ocr has already been declared". Deletes the resurrected block. LibraryPage.tsx is now byte-identical to its 616b966 version. --- frontend/src/pages/LibraryPage.tsx | 7 ------- 1 file changed, 7 deletions(-) diff --git a/frontend/src/pages/LibraryPage.tsx b/frontend/src/pages/LibraryPage.tsx index a9abafa..4e27b87 100644 --- a/frontend/src/pages/LibraryPage.tsx +++ b/frontend/src/pages/LibraryPage.tsx @@ -41,13 +41,6 @@ export function LibraryPage() { refetchInterval: 10_000, }) - // OCR runs after the index run finishes, so this keeps polling regardless of - // indexing state. Without it "indexing complete" is a lie for scanned PDFs. - const { data: ocr } = useApi(getOcrStatus, { - cacheKey: 'ocr-status', - refetchInterval: 10_000, - }) - // Derive running state from BOTH local flag and polled backend status const isRunning = indexing || status?.status === 'running'