From f24ea5ad00d24066c89fb9ac2a629b99f78676c8 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 15:53:47 +0200 Subject: [PATCH 01/14] refactor(api/graphql): make Cell a composite entity with a summary facet --- api/src/damnit_api/graphql/directives.py | 4 +- api/src/damnit_api/runs/types.py | 53 ++++++++++-- api/tests/graphql/test_models.py | 45 +++++++++- api/tests/graphql/test_queries.py | 51 +++++++++-- api/tests/graphql/test_subscriptions.py | 8 +- .../e2e/__snapshots__/test_data_parity.ambr | 84 +++++++++++++------ api/tests/refactor/e2e/test_data_parity.py | 14 +++- api/tests/refactor/snapshots/schema.graphql | 9 +- api/tests/refactor/test_gql_parity.py | 23 +++-- 9 files changed, 235 insertions(+), 56 deletions(-) diff --git a/api/src/damnit_api/graphql/directives.py b/api/src/damnit_api/graphql/directives.py index af59c0b1..f38ac3f8 100644 --- a/api/src/damnit_api/graphql/directives.py +++ b/api/src/damnit_api/graphql/directives.py @@ -19,8 +19,8 @@ def lightweight(field: DirectiveValue[DamnitRun | Cell]): fields = field if isinstance(field, list) else [field] for cell in get_cells(fields): - if cell is not None and cell.dtype in HEAVY_DATA: - cell.value = None + if cell is not None and cell.summary.dtype in HEAVY_DATA: + cell.summary.value = None # Return original field return field diff --git a/api/src/damnit_api/runs/types.py b/api/src/damnit_api/runs/types.py index ba17581b..0b6352b1 100644 --- a/api/src/damnit_api/runs/types.py +++ b/api/src/damnit_api/runs/types.py @@ -86,11 +86,22 @@ def from_attrs(cls, attributes): @strawberry.type -class Cell: - name: str +class CellSummary: value: Any | None dtype: DamnitType - error: CellError | None = None + + +@strawberry.type +class Cell: + # `id` is the cell's global identity, "{database}:{proposal}:{run}:{name}". + # `database` is in it because the same (proposal, run, name) can be served + # through two databases (a guest proposal also opened directly), so it is + # scoped the same way the run's own key is. `error` describes the whole cell: + # a failed variable still carries a summary, with a null value. + id: strawberry.ID + name: str + error: CellError | None + summary: CellSummary def _unwrap(entry): @@ -126,7 +137,7 @@ def cells(self, names: list[str] | None = None) -> list[Cell]: return [c for c in self._cells if c.name in requested] @classmethod - def _iter_cells(cls, record): + def _iter_cells(cls, record, *, database, proposal, run): for name, entry in record.items(): if entry is None: continue @@ -135,7 +146,19 @@ def _iter_cells(cls, record): dtype = cls.get_dtype(name, entry) value, dtype = serialize(entry["value"], dtype=dtype) error = CellError.from_attrs(entry.get("attributes")) - yield Cell(name=name, value=Any(value), dtype=dtype, error=error) + if error is not None: + # A failed cell has no value to render, so its summary type is + # not worth keeping. The client merges a cell's summary without + # being able to see the error alongside it, so a heavy dtype + # here would look like a value @lightweight held back and pin + # whatever the cell held before it failed. + dtype = DamnitType.STRING + yield Cell( + id=strawberry.ID(f"{database}:{proposal}:{run}:{name}"), + name=name, + error=error, + summary=CellSummary(value=Any(value), dtype=dtype), + ) @classmethod def from_db(cls, record, *, database): @@ -146,11 +169,23 @@ def from_db(cls, record, *, database): if proposal is None: msg = "Run record has no proposal." raise ValueError(msg) + database = str(database) + # Cell ids join their parts with ":", so a part carrying one of its own + # would let two different cells share an id and collide in the client's + # cache. Only `database` can: it is the handle the client sent, and a + # path handle is coming. + if ":" in database: + msg = f"Database handle may not contain ':': {database!r}" + raise ValueError(msg) + proposal = str(proposal) + run = int(_unwrap(record["run"])) return cls( - database=str(database), - proposal=str(proposal), - run=int(_unwrap(record["run"])), - _cells=list(cls._iter_cells(record)), + database=database, + proposal=proposal, + run=run, + _cells=list( + cls._iter_cells(record, database=database, proposal=proposal, run=run) + ), ) @staticmethod diff --git a/api/tests/graphql/test_models.py b/api/tests/graphql/test_models.py index 029e2993..ec815536 100644 --- a/api/tests/graphql/test_models.py +++ b/api/tests/graphql/test_models.py @@ -180,10 +180,28 @@ def test_from_db_includes_error_for_failed_variable(): by_name = {v.name: v for v in run.cells()} assert by_name["run"].error is None - assert by_name["broken"].value is None + assert by_name["broken"].summary.value is None assert by_name["broken"].error == CellError(message=ERROR_ATTRS["error"], cls="Foo") +def test_from_db_drops_a_heavy_summary_type_when_the_variable_failed(): + # The client merges a cell's summary without being able to see the error + # beside it, so a heavy dtype on a null value reads as one @lightweight held + # back and pins whatever the cell held before it failed. + record = { + "proposal": {"value": 900485}, + "run": {"value": 1}, + "broken": { + "value": None, + "summary_type": "trendline", + "attributes": json.dumps(ERROR_ATTRS), + }, + } + run = DamnitRun.from_db(record, database="900485") + + assert run.cells(names=["broken"])[0].summary.dtype == DamnitType.STRING + + def test_from_db_populates_identity_trio(): record = {"proposal": {"value": 900485}, "run": {"value": 348}} run = DamnitRun.from_db(record, database="900485") @@ -191,3 +209,28 @@ def test_from_db_populates_identity_trio(): assert run.database == "900485" assert run.proposal == "900485" assert run.run == 348 + + +def test_from_db_scopes_cell_ids_by_database(): + # The same (proposal, run, name) served through two databases (a guest + # proposal also opened directly) must key distinct normalized cells, so the + # second database cannot overwrite the first's cached value. + record = { + "proposal": {"value": 900485}, + "run": {"value": 348}, + "n_trains": {"value": 3641}, + } + guest = DamnitRun.from_db(record, database="900405") + direct = DamnitRun.from_db(record, database="900485") + + assert guest.cells(names=["n_trains"])[0].id == "900405:900485:348:n_trains" + assert direct.cells(names=["n_trains"])[0].id == "900485:900485:348:n_trains" + + +def test_from_db_rejects_a_database_handle_carrying_a_colon(): + # Cell ids join their parts with ":", so a handle carrying one of its own + # would let two different cells share an id and collide in the client cache. + record = {"proposal": {"value": 900485}, "run": {"value": 348}} + + with pytest.raises(ValueError, match="may not contain"): + DamnitRun.from_db(record, database="900405:900485") diff --git a/api/tests/graphql/test_queries.py b/api/tests/graphql/test_queries.py index ef8ad54f..33900b47 100644 --- a/api/tests/graphql/test_queries.py +++ b/api/tests/graphql/test_queries.py @@ -51,7 +51,7 @@ async def test_runs_query(graphql_schema, mocked_fetch_cells, mocked_fetch_info) runs(database: {{proposal: "{PROPOSAL}"}}, per_page: $per_page) {{ cells {{ name - value + summary {{ value }} }} }} }} @@ -72,6 +72,44 @@ async def test_runs_query(graphql_schema, mocked_fetch_cells, mocked_fetch_info) assert mocked_fetch_info.called +@pytest.mark.asyncio +async def test_lightweight_directive_blanks_heavy_values( + graphql_schema, mocker, mocked_fetch_info +): + # A heavy cell (array) next to a scalar, so the directive has both to sort. + record = { + "proposal": {"value": PROPOSAL}, + "run": {"value": 348}, + "n_trains": {"value": 3641, "summary_type": None}, + "spectrum": {"value": [1.0, 2.0, 3.0], "summary_type": "trendline"}, + } + mocker.patch( + "damnit_api.graphql.queries.fetch_cells", + return_value=[record], + ) + + query = f""" + query {{ + runs(database: {{proposal: "{PROPOSAL}"}}, per_page: 2) @lightweight {{ + cells {{ + name + summary {{ value dtype }} + }} + }} + }} + """ + result = await graphql_schema.execute(query) + + assert result.errors is None + + cells = {c["name"]: c["summary"] for c in result.data["runs"][0]["cells"]} + # The heavy value is held back, but its dtype still describes the cell. + assert cells["spectrum"]["value"] is None + assert cells["spectrum"]["dtype"] == "array" + # A scalar is left untouched. + assert cells["n_trains"]["value"] == 3641 + + @pytest.mark.asyncio async def test_runs_query_returns_identity_trio( graphql_schema, mocked_fetch_cells, mocked_fetch_info @@ -245,7 +283,7 @@ async def test_runs_query_partial_name_match(graphql_schema, real_damnit_db): runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{ cells(names: ["alpha", "run"]) {{ name - value + summary {{ value }} }} }} }} @@ -254,7 +292,7 @@ async def test_runs_query_partial_name_match(graphql_schema, real_damnit_db): assert result.errors is None runs = result.data["runs"] - by_run = [{v["name"]: v["value"] for v in r["cells"]} for r in runs] + by_run = [{v["name"]: v["summary"]["value"] for v in r["cells"]} for r in runs] assert by_run == [ {"alpha": "a1", "run": 1}, {"alpha": "a2", "run": 2}, @@ -356,7 +394,7 @@ async def test_runs_query_includes_guests_active_block_first( runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{ proposal run - cells(names: ["alpha"]) {{ name value }} + cells(names: ["alpha"]) {{ name summary {{ value }} }} }} }} """ @@ -372,7 +410,10 @@ async def test_runs_query_includes_guests_active_block_first( ("888888", 1), ] - alpha = [{v["name"]: v["value"] for v in r["cells"]}.get("alpha") for r in runs] + alpha = [ + {v["name"]: v["summary"]["value"] for v in r["cells"]}.get("alpha") + for r in runs + ] # The colliding run 1 keeps each proposal's own latest value. assert alpha == ["a1", "a2", "guest_a1"] diff --git a/api/tests/graphql/test_subscriptions.py b/api/tests/graphql/test_subscriptions.py index 9f73a53e..60deaed6 100644 --- a/api/tests/graphql/test_subscriptions.py +++ b/api/tests/graphql/test_subscriptions.py @@ -35,7 +35,7 @@ database proposal run - cells { name value dtype } + cells { name summary { value dtype } } } metadata { runs { proposal run } @@ -150,7 +150,11 @@ async def test_run_updates( "run": DatabaseVariable(value=NEW_RUN, damnit_dtype=DamnitType.NUMBER), } got = { - v["name"]: {"value": v["value"], "dtype": v["dtype"]} for v in run["cells"] + v["name"]: { + "value": v["summary"]["value"], + "dtype": v["summary"]["dtype"], + } + for v in run["cells"] } assert got == { name: {"value": var.damnit_value, "dtype": var.damnit_dtype.value} diff --git a/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr b/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr index 0c7fd7a1..1f783507 100644 --- a/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr +++ b/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr @@ -228,76 +228,112 @@ # name: test_runs_query_wire_shapes_unchanged dict({ 'added_at': dict({ - 'dtype': 'timestamp', 'error': None, + 'id': '6996:6996:1:added_at', 'name': 'added_at', - 'value': 1717058767016, + 'summary': dict({ + 'dtype': 'timestamp', + 'value': 1717058767016, + }), }), 'n_pulses': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:n_pulses', 'name': 'n_pulses', - 'value': 290.0, + 'summary': dict({ + 'dtype': 'number', + 'value': 290.0, + }), }), 'n_trains': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:n_trains', 'name': 'n_trains', - 'value': 2679, + 'summary': dict({ + 'dtype': 'number', + 'value': 2679, + }), }), 'proposal': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:proposal', 'name': 'proposal', - 'value': 6996, + 'summary': dict({ + 'dtype': 'number', + 'value': 6996, + }), }), 'run': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:run', 'name': 'run', - 'value': 1, + 'summary': dict({ + 'dtype': 'number', + 'value': 1, + }), }), 'sample_type': dict({ - 'dtype': 'string', 'error': None, + 'id': '6996:6996:1:sample_type', 'name': 'sample_type', - 'value': 'Silica nanoparticle', + 'summary': dict({ + 'dtype': 'string', + 'value': 'Silica nanoparticle', + }), }), 'sample_x': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:sample_x', 'name': 'sample_x', - 'value': 1.780738354, + 'summary': dict({ + 'dtype': 'number', + 'value': 1.780738354, + }), }), 'sample_y': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:sample_y', 'name': 'sample_y', - 'value': 21.00022591, + 'summary': dict({ + 'dtype': 'number', + 'value': 21.00022591, + }), }), 'start_time': dict({ - 'dtype': 'timestamp', 'error': None, + 'id': '6996:6996:1:start_time', 'name': 'start_time', - 'value': 1717058356866, + 'summary': dict({ + 'dtype': 'timestamp', + 'value': 1717058356866, + }), }), 'total_transmission': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:total_transmission', 'name': 'total_transmission', - 'value': 0.000114891, + 'summary': dict({ + 'dtype': 'number', + 'value': 0.000114891, + }), }), 'xgm_intensity': dict({ - 'dtype': 'number', 'error': None, + 'id': '6996:6996:1:xgm_intensity', 'name': 'xgm_intensity', - 'value': 470.959655762, + 'summary': dict({ + 'dtype': 'number', + 'value': 470.959655762, + }), }), 'xpcs_g2_plot': dict({ - 'dtype': 'image', 'error': None, + 'id': '6996:6996:1:xpcs_g2_plot', 'name': 'xpcs_g2_plot', - 'value': '', + 'summary': dict({ + 'dtype': 'image', + 'value': '', + }), }), }) # --- diff --git a/api/tests/refactor/e2e/test_data_parity.py b/api/tests/refactor/e2e/test_data_parity.py index 9b3a67b2..08d2f086 100644 --- a/api/tests/refactor/e2e/test_data_parity.py +++ b/api/tests/refactor/e2e/test_data_parity.py @@ -44,7 +44,10 @@ def runs_query(proposal: int, *, per_page: int, names: list[str]) -> dict: query {{ runs(database: {{proposal: "{proposal}"}}, per_page: {per_page}) {{ cells(names: [{names_arg}]) {{ - name value dtype error {{ message cls }} + id + name + error {{ message cls }} + summary {{ value dtype }} }} }} }} @@ -118,10 +121,13 @@ async def test_runs_query_wire_shapes_unchanged(logged_in_client, snapshot): # the prefix directly and replace the value in the snapshot so it isn't # pinning the exact rendered image bytes image = by_name["xpcs_g2_plot"] - assert image["dtype"] == "image" + assert image["summary"]["dtype"] == "image" assert image["error"] is None - assert image["value"].startswith("data:image/png;base64,") - by_name["xpcs_g2_plot"] = {**image, "value": ""} + assert image["summary"]["value"].startswith("data:image/png;base64,") + by_name["xpcs_g2_plot"] = { + **image, + "summary": {**image["summary"], "value": ""}, + } assert _normalize(by_name) == snapshot diff --git a/api/tests/refactor/snapshots/schema.graphql b/api/tests/refactor/snapshots/schema.graphql index d79ec34d..41501c3d 100644 --- a/api/tests/refactor/snapshots/schema.graphql +++ b/api/tests/refactor/snapshots/schema.graphql @@ -4,10 +4,10 @@ directive @lightweight on FIELD scalar Any type Cell { + id: ID! name: String! - value: Any - dtype: DamnitType! error: CellError + summary: CellSummary! } type CellError { @@ -15,6 +15,11 @@ type CellError { cls: String! } +type CellSummary { + value: Any + dtype: DamnitType! +} + type DamnitRun { database: String! proposal: String! diff --git a/api/tests/refactor/test_gql_parity.py b/api/tests/refactor/test_gql_parity.py index 0d75aeef..863c5950 100644 --- a/api/tests/refactor/test_gql_parity.py +++ b/api/tests/refactor/test_gql_parity.py @@ -121,9 +121,10 @@ async def test_runs_query_wire_shape_unchanged( query {{ runs(database: {{proposal: "{PROPOSAL}"}}, per_page: 1) {{ cells {{ + id name - value - dtype + error {{ message cls }} + summary {{ value dtype }} }} }} }} @@ -140,11 +141,17 @@ async def test_runs_query_wire_shape_unchanged( cells = {c["name"]: c for c in runs[0]["cells"]} assert set(cells) >= {"proposal", "run", "n_trains", "start_time"} for cell in cells.values(): - assert set(cell.keys()) == {"name", "value", "dtype"} + assert set(cell.keys()) == {"id", "name", "error", "summary"} + assert set(cell["summary"].keys()) == {"value", "dtype"} + + # The synthesized id is "{database}:{proposal}:{run}:{name}", the Apollo + # cache key; the database handle echoes the proposal it was addressed by. + assert cells["n_trains"]["id"] == f"{PROPOSAL}:{PROPOSAL}:348:n_trains" # `start_time` is a timestamp: the frontend expects milliseconds - assert cells["start_time"]["value"] == KNOWN_DATA["start_time"].damnit_value - assert cells["start_time"]["dtype"] == "timestamp" + start_time = cells["start_time"]["summary"] + assert start_time["value"] == KNOWN_DATA["start_time"].damnit_value + assert start_time["dtype"] == "timestamp" @pytest.mark.asyncio @@ -240,7 +247,7 @@ async def test_run_updates_subscription_wire_shape_unchanged( database proposal run - cells { name value dtype } + cells { id name error { message cls } summary { value dtype } } } metadata { runs { proposal run } @@ -268,7 +275,9 @@ async def test_run_updates_subscription_wire_shape_unchanged( assert set(run.keys()) == {"database", "proposal", "run", "cells"} assert run["run"] == NEW_RUN for cell in run["cells"]: - assert set(cell.keys()) == {"name", "value", "dtype"} + assert set(cell.keys()) == {"id", "name", "error", "summary"} + assert set(cell["summary"].keys()) == {"value", "dtype"} + assert cell["id"] == f"{PROPOSAL}:{PROPOSAL}:{NEW_RUN}:{cell['name']}" metadata = payload["metadata"] assert set(metadata.keys()) == {"runs"} From ef6b539ea97cff921d8ad73e70d04a7c6469cfe8 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 15:54:28 +0200 Subject: [PATCH 02/14] refactor(frontend): read cells through the composite summary facet --- frontend/e2e/mocks/websocket.ts | 18 +-- frontend/packages/shared/src/mocks/index.ts | 2 +- frontend/packages/shared/src/mocks/shape.ts | 85 ++++++++++---- .../ui/src/data/table/table-data.queries.ts | 15 ++- .../src/data/table/table-data.transforms.ts | 4 +- .../ui/src/data/table/table-data.types.ts | 13 ++- .../ui/src/features/dashboard/run.tsx | 7 +- .../features/plots/use-summary-plot-data.ts | 6 +- .../packages/ui/src/features/table/table.tsx | 14 +-- .../packages/ui/src/graphql/type-policies.ts | 107 +++++++++--------- .../data/table/table-data.transforms.test.ts | 40 +++++-- .../features/table/use-table-runs.test.tsx | 25 +++- .../ui/tests/graphql/type-policies.test.ts | 100 +++++++++++++--- .../ui/tests/redux/proposal-teardown.test.tsx | 37 ++++-- frontend/packages/ui/tests/support/cells.ts | 18 +++ 15 files changed, 337 insertions(+), 154 deletions(-) create mode 100644 frontend/packages/ui/tests/support/cells.ts diff --git a/frontend/e2e/mocks/websocket.ts b/frontend/e2e/mocks/websocket.ts index e3173155..2be7e883 100644 --- a/frontend/e2e/mocks/websocket.ts +++ b/frontend/e2e/mocks/websocket.ts @@ -1,7 +1,7 @@ import type { Page, WebSocketRoute } from '@playwright/test' import { - shapeCell, + shapeRun, type Meta, type RunData, } from '@damnit-frontend/shared/mocks' @@ -50,16 +50,6 @@ export async function mockWebSocket( let clock = 1_700_000_000_000 const nextTimestamp = () => (clock += 1000) - const shapeRun = (run: number, variables: RunData['variables']) => ({ - __typename: 'DamnitRun', - database: proposal, - proposal, - run, - cells: Object.entries(variables).map(([name, variable]) => - shapeCell(name, variable) - ), - }) - const deliver = ({ runs, metadata }: LatestData) => { const timestamp = nextTimestamp() socket?.send( @@ -71,7 +61,11 @@ export async function mockWebSocket( run_updates: { __typename: 'RunUpdates', runs: Object.entries(runs).map(([run, variables]) => - shapeRun(Number(run), variables) + shapeRun(variables, { + database: proposal, + proposal, + run: Number(run), + }) ), metadata: { __typename: 'TableMeta', diff --git a/frontend/packages/shared/src/mocks/index.ts b/frontend/packages/shared/src/mocks/index.ts index 7db7aaa2..8721c189 100644 --- a/frontend/packages/shared/src/mocks/index.ts +++ b/frontend/packages/shared/src/mocks/index.ts @@ -1,7 +1,7 @@ export { REST_API_PREFIXES, - shapeCell, shapeMetadata, + shapeRun, shapeTableData, unmockedOperationError, } from './shape' diff --git a/frontend/packages/shared/src/mocks/shape.ts b/frontend/packages/shared/src/mocks/shape.ts index 4db9f538..e78a2a16 100644 --- a/frontend/packages/shared/src/mocks/shape.ts +++ b/frontend/packages/shared/src/mocks/shape.ts @@ -26,22 +26,53 @@ export function shapeMetadata(meta: Meta, proposal: string) { } // One cell's wire object. Shared by the query mock and the subscription mock so -// the `Cell`/`CellError` shape stays identical on both paths. `error` is always -// sent, even absent from the example: the query selects it, so omitting it -// leaves the client's cache read incomplete and every cached replay silently -// refetches. `lightweight` blanks a heavy value the way the real @lightweight -// pass does. -export function shapeCell( +// the composite `Cell` shape stays identical on both paths. The `id` is built +// from the run's identity, so a live push lands on the same normalized cache +// object the table already holds. `error` is always sent, even absent from the +// example: the query selects it, so omitting it leaves the client's cache read +// incomplete and every cached replay silently refetches. `lightweight` blanks a +// heavy value the way the real @lightweight pass does. +function shapeCell( name: string, cell: RunData['variables'][string], - { lightweight = false }: { lightweight?: boolean } = {} + { database, proposal, run, lightweight = false }: ShapeCellOptions ) { return { __typename: 'Cell', + id: `${database}:${proposal}:${run}:${name}`, name, - value: lightweight && HEAVY_DTYPES.has(cell.dtype) ? null : cell.value, - dtype: cell.dtype, error: 'error' in cell ? { __typename: 'CellError', ...cell.error } : null, + summary: { + __typename: 'CellSummary', + value: lightweight && HEAVY_DTYPES.has(cell.dtype) ? null : cell.value, + dtype: cell.dtype, + }, + } +} + +// One run's wire object: the identity trio Apollo keys the run by, plus cells +// whose ids are built from that same trio. Shared by the query mock and the +// subscription mock, so the two cannot disagree on how a run and its cells are +// keyed, and a live push lands on the run the table already holds. +// +// `run` is the logical run number the metadata list and the grid rows use, not +// the physical source run number: in the xpcs example, runs 1 to 6 come from +// source runs 6, 7, 11, 33, 34 and 35. Pass the wrong one and the run and every +// one of its cells key to a row no grid reads. +export function shapeRun( + variables: RunData['variables'], + { database, proposal, run, names, lightweight = false }: ShapeRunOptions +) { + return { + __typename: 'DamnitRun', + database, + proposal, + run, + cells: Object.entries(variables) + .filter(([name]) => names == null || names.includes(name)) + .map(([name, cell]) => + shapeCell(name, cell, { database, proposal, run, lightweight }) + ), } } @@ -50,23 +81,31 @@ export function shapeTableData( { proposal, names, lightweight = false }: ShapeTableDataOptions ) { return { - runs: data.map((run) => ({ - // The identity trio, so Apollo keys the run by (database, proposal, run). - // `database` is the addressing handle the client sent; the examples are - // single-proposal, so a run's own proposal is the queried one too. The - // run is the `run` variable's value (the logical number the metadata run - // list uses), not the physical source run number. - __typename: 'DamnitRun', - database: proposal, - proposal, - run: Number(run.variables.run?.value ?? run.source.run_number), - cells: Object.entries(run.variables) - .filter(([name]) => names == null || names.includes(name)) - .map(([name, cell]) => shapeCell(name, cell, { lightweight })), - })), + runs: data.map((run) => + shapeRun(run.variables, { + // `database` is the addressing handle the client sent; the examples are + // single-proposal, so a run's own proposal is the queried one too. + database: proposal, + proposal, + run: Number(run.variables.run?.value ?? run.source.run_number), + names, + lightweight, + }) + ), } } +type ShapeCellOptions = { + database: string + proposal: string + run: number + lightweight?: boolean +} + +type ShapeRunOptions = ShapeCellOptions & { + names?: string[] | null +} + type ShapeTableDataOptions = { proposal: string names?: string[] | null diff --git a/frontend/packages/ui/src/data/table/table-data.queries.ts b/frontend/packages/ui/src/data/table/table-data.queries.ts index e97a7273..22b339ee 100644 --- a/frontend/packages/ui/src/data/table/table-data.queries.ts +++ b/frontend/packages/ui/src/data/table/table-data.queries.ts @@ -12,17 +12,22 @@ import { type Run, type TableMeta } from './table-data.types' * ----------------------------- */ -// The cell selection every runs document shares. A field added to one copy but -// not the others leaves the documents drifting apart over the same cache entry, -// so the shape lives in one place. +// The cell selection every runs document shares. `id` must ride on it: Cell is +// a normalized entity keyed by `id`, so Apollo throws on a selection that omits +// it, and a field added to one copy but not the others would silently break +// normalization. Keeping the shape in one place stops the three documents from +// drifting. const CELL_FIELDS = ` + id name - value - dtype error { message cls } + summary { + value + dtype + } ` // Every run carries its identity trio (database, proposal, run) so Apollo can diff --git a/frontend/packages/ui/src/data/table/table-data.transforms.ts b/frontend/packages/ui/src/data/table/table-data.transforms.ts index 57a51adb..abbf0eac 100644 --- a/frontend/packages/ui/src/data/table/table-data.transforms.ts +++ b/frontend/packages/ui/src/data/table/table-data.transforms.ts @@ -7,9 +7,9 @@ import type { Cell, Run, RunCells, RunId, Variable } from './table-data.types' // through `isHeavyBlank`, so the two cannot disagree on what is still to come. function isDeferred(cell: Cell): boolean { return isHeavyBlank({ - value: cell.value, + value: cell.summary.value, error: cell.error, - dtype: cell.dtype, + dtype: cell.summary.dtype, }) } diff --git a/frontend/packages/ui/src/data/table/table-data.types.ts b/frontend/packages/ui/src/data/table/table-data.types.ts index e9bad4f0..d043004c 100644 --- a/frontend/packages/ui/src/data/table/table-data.types.ts +++ b/frontend/packages/ui/src/data/table/table-data.types.ts @@ -5,12 +5,19 @@ export type CellError = { cls: string } -// One run's value in one variable. `name` ties the cell to its variable. -export type Cell = { - name: string +// The table-value facet of a cell: the summary the grid renders. +export type CellSummary = { value: CellValue dtype: string +} + +// One run's cell in one variable. `id` ("{database}:{proposal}:{run}:{name}") is +// the Apollo cache key; `name` ties the cell to its variable. +export type Cell = { + id: string + name: string error?: CellError | null + summary: CellSummary } // A column: what a variable is, independent of any run's value of it. diff --git a/frontend/packages/ui/src/features/dashboard/run.tsx b/frontend/packages/ui/src/features/dashboard/run.tsx index f529e07b..6632b3c5 100644 --- a/frontend/packages/ui/src/features/dashboard/run.tsx +++ b/frontend/packages/ui/src/features/dashboard/run.tsx @@ -145,7 +145,7 @@ const Run = () => { const validRuns = Object.entries(runData).filter( ([name, data]) => isVariableVisible(name, variableVisibility) && - (data?.error != null || data?.value != null) && + (data?.error != null || data?.summary.value != null) && !NONCONFIGURABLE_VARIABLES.includes(name) ) @@ -159,11 +159,12 @@ const Run = () => { if (data.error) { return renderError({ name, label, error: data.error }) } - const render = renderFactory[data.dtype] ?? renderFactory.default + const render = + renderFactory[data.summary.dtype] ?? renderFactory.default return render({ name, label, - value: data.value as CellValue, + value: data.summary.value as CellValue, }) })} diff --git a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts index dd63c2a6..784d207f 100644 --- a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts +++ b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts @@ -67,12 +67,12 @@ export function useSummaryPlotData({ const allNumeric = points.every( (point) => point != null && - typeof point.value === 'number' && - point.dtype === DTYPES.number + typeof point.summary.value === 'number' && + point.summary.dtype === DTYPES.number ) if (allNumeric) { points.forEach((point, index) => { - series[index].push(point!.value as number) + series[index].push(point!.summary.value as number) }) } } diff --git a/frontend/packages/ui/src/features/table/table.tsx b/frontend/packages/ui/src/features/table/table.tsx index 257dfd3d..75c47d37 100644 --- a/frontend/packages/ui/src/features/table/table.tsx +++ b/frontend/packages/ui/src/features/table/table.tsx @@ -136,8 +136,8 @@ const Table = ({ grid, paginated = true }: TableProps) => { } return getCell({ - value: cell.value, - dtype: cell.dtype, + value: cell.summary.value, + dtype: cell.summary.dtype, options: { lastUpdated: lastUpdatedByKey.get(key) }, }) }, @@ -160,11 +160,11 @@ const Table = ({ grid, paginated = true }: TableProps) => { return { kind: 'error', error: item.error } } if ( - item.dtype === DTYPES.image && - typeof item.value === 'string' && - item.value + item.summary.dtype === DTYPES.image && + typeof item.summary.value === 'string' && + item.summary.value ) { - return { kind: 'image', src: item.value } + return { kind: 'image', src: item.summary.value } } return undefined }, @@ -267,7 +267,7 @@ const Table = ({ grid, paginated = true }: TableProps) => { // A row whose page has not loaded yet has no data at all, not merely no // value: it has nothing to offer a plot either way. // TODO: Use extracted data type from the database - if (col !== -1 && rowData?.[column]?.value != null) { + if (col !== -1 && rowData?.[column]?.summary.value != null) { const variable = tableColumns[col] const subtitle = `${variable.title}` diff --git a/frontend/packages/ui/src/graphql/type-policies.ts b/frontend/packages/ui/src/graphql/type-policies.ts index 50157361..00441754 100644 --- a/frontend/packages/ui/src/graphql/type-policies.ts +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -7,60 +7,6 @@ import type { import { isHeavyBlank } from '#src/constants' -// A cell as it sits in the cache: an embedded object (Cell is not normalized) -// or, defensively, a reference. -type StoreCell = Reference | StoreObject - -type ReadField = FieldFunctionOptions['readField'] - -// The held-back-blank rule over the cache's own cell representation. Reads -// `isHeavyBlank`, the same rule the table transforms apply to a plain cell, so a -// value @lightweight held back is never mistaken for one DAMNIT genuinely -// cleared, and the merge policy and the deferred-fetch selector stay in step. -function isDeferredCell(cell: StoreCell, readField: ReadField): boolean { - return isHeavyBlank({ - value: readField('value', cell), - error: readField('error', cell), - dtype: readField('dtype', cell)!, - }) -} - -// Merge the lightweight, deferred, and pushed cell sets into one bag per run, -// keyed by name. A held-back value never overwrites a value already in place, so -// a cache-and-network refetch of the lightweight pass cannot blank a heavy value -// the deferred pass filled in. It still lands on a cell that has none yet, -// which is what draws the loading skeleton until the value arrives. -// -// Unlike the phase-1 slice, this cannot tell a live push from a bulk load, so it -// drops the "a live push may clear a value" branch. DAMNIT never un-computes a -// value back to null, so no real push relies on it. -function mergeCellsByName( - existing: readonly StoreCell[] = [], - incoming: readonly StoreCell[] = [], - { readField }: FieldFunctionOptions -): StoreCell[] { - const byName = new Map() - - for (const cell of existing) { - byName.set(readField('name', cell)!, cell) - } - - for (const cell of incoming) { - const name = readField('name', cell)! - const previous = byName.get(name) - const heldBack = - previous != null && - isDeferredCell(cell, readField) && - readField('value', previous) != null - if (heldBack) { - continue - } - byName.set(name, cell) - } - - return [...byName.values()] -} - // Accumulate normalized refs into one list, deduped by Apollo's own cache id // (`__ref`), the identity it already computed from keyFields. When nothing new // arrives this hands back the same array: a value-only push carries only refs @@ -87,16 +33,67 @@ function mergeRefsByIdentity( return [...existing, ...additions] } +// A summary as the cache stores it. Both fields are optional because a write +// only carries what its document selected, not because the schema allows one +// without the other. +type StoredSummary = StoreObject & { + value?: unknown + dtype?: string +} + +// Keep a value the lightweight pass is holding back. Only a heavy dtype is ever +// blanked, so a null arriving over a value of that same dtype is a blank on its +// way to being filled, and the cached value stays. Everything else is DAMNIT's +// own answer and replaces what is there: a null scalar clears a stale number, +// and a retyped variable clears a value that no longer describes it, which is +// what puts the cell back in the deferred pass's queue. +function mergeSummary( + existing: StoredSummary | undefined, + incoming: StoredSummary, + { mergeObjects }: FieldFunctionOptions +): StoredSummary { + if (existing == null) { + return incoming + } + + // A write that selected only `value` carries no dtype; the cached one still + // describes the cell, and merging rather than replacing is what keeps it. + const dtype = incoming.dtype ?? existing.dtype + const heldBackBlank = + dtype != null && + dtype === existing.dtype && + existing.value != null && + isHeavyBlank({ value: incoming.value, error: null, dtype }) + return heldBackBlank ? existing : mergeObjects(existing, incoming) +} + export const typePolicies: TypePolicies = { DamnitRun: { keyFields: ['database', 'proposal', 'run'], fields: { + // Cell refs, one list per run. Cell is a normalized entity, so the + // two-pass table (lightweight blanks, then a heavier deferred fill) writes + // both passes to the same cell object; this list only owes membership. + // Without it, the deferred pass's shorter `cells` array would replace the + // lightweight one and drop cells absent from the second fetch. Value + // protection lives on `CellSummary.value`. cells: { keyArgs: false, - merge: mergeCellsByName, + merge: mergeRefsByIdentity, }, }, }, + Cell: { + keyFields: ['id'], + }, + CellSummary: { + // Merge at the summary level: a merge function only sees the field it + // merges, and this is the only level where `dtype` travels with the value. + // `error` is cell-level, so it cannot be read here; the API drops the + // summary type of a failed cell so that a failure never looks like a blank + // held back. + merge: mergeSummary, + }, Query: { fields: { // Paginated runs, one list. Row order is not preserved here: the table diff --git a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts index ace0c43e..d09b00fb 100644 --- a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts +++ b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts @@ -6,23 +6,39 @@ import { runKey, } from '#src/data/table/table-data.transforms' import type { - Cell, CellError, CellValue, Run, } from '#src/data/table/table-data.types' +type CellInput = { + name: string + value: CellValue + dtype: string + error: CellError | null +} + function cell( name: string, value: CellValue, dtype = 'number', error: CellError | null = null -): Cell { +): CellInput { return { name, value, dtype, error } } -function run(proposal: string, number: number, cells: Run['cells']): Run { - return { database: proposal, proposal, run: number, cells } +function run(proposal: string, number: number, cells: CellInput[]): Run { + return { + database: proposal, + proposal, + run: number, + cells: cells.map((entry) => ({ + id: `${proposal}:${number}:${entry.name}`, + name: entry.name, + error: entry.error, + summary: { value: entry.value, dtype: entry.dtype }, + })), + } } describe('indexRunCells', () => { @@ -34,10 +50,10 @@ describe('indexRunCells', () => { expect([...cells.keys()]).toEqual(['900405:5', '900405:9']) expect(cells.get('900405:5')?.energy).toEqual({ + id: '900405:5:energy', name: 'energy', - value: 1.2, - dtype: 'number', error: null, + summary: { value: 1.2, dtype: 'number' }, }) }) @@ -49,8 +65,8 @@ describe('indexRunCells', () => { run('900485', 1, [cell('energy', 9.9)]), ]) - expect(cells.get('900405:1')?.energy.value).toBe(1.2) - expect(cells.get('900485:1')?.energy.value).toBe(9.9) + expect(cells.get('900405:1')?.energy.summary.value).toBe(1.2) + expect(cells.get('900485:1')?.energy.summary.value).toBe(9.9) }) test('stores each cell by its variable name', () => { @@ -59,10 +75,10 @@ describe('indexRunCells', () => { run('900405', 1, [cell('x', 2, 'number', error)]), ]) expect(cells.get('900405:1')?.x).toEqual({ + id: '900405:1:x', name: 'x', - value: 2, - dtype: 'number', error, + summary: { value: 2, dtype: 'number' }, }) }) test('reuses a run’s cell map while the run object is unchanged', () => { @@ -87,7 +103,7 @@ describe('indexRunCells', () => { expect(second.get('900405:1')).toBe(first.get('900405:1')) expect(second.get('900405:2')).not.toBe(first.get('900405:2')) - expect(second.get('900405:2')?.energy.value).toBe(9.9) + expect(second.get('900405:2')?.energy.summary.value).toBe(9.9) }) }) @@ -96,7 +112,7 @@ test('runKey pairs proposal and run into a lookup key', () => { }) // A heavy value the @lightweight directive held back: the server sends the cell -// with its value nulled out. +// with its summary value nulled out. const blanked = (name: string, error: CellError | null = null) => cell(name, null, 'array', error) diff --git a/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx b/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx index fdb0000a..cae2b1b9 100644 --- a/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx +++ b/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx @@ -18,6 +18,7 @@ import { TABLE_DATA_QUERY } from '#src/data/table/table-data.queries' import { createPriorityLink } from '#src/graphql/priority-link' import { typePolicies } from '#src/graphql/type-policies' import { useTableRuns } from '#src/features/table/use-table-runs' +import { cellId } from '#tests/support/cells' const PROPOSAL = '900405' const PAGE_SIZE = 10 @@ -39,10 +40,15 @@ const runFor = (run: number) => ({ cells: [ { __typename: 'Cell', + id: cellId({ + database: PROPOSAL, + proposal: PROPOSAL, + run, + name: 'spectrum', + }), name: 'spectrum', - value: null, - dtype: 'array', error: null, + summary: { __typename: 'CellSummary', value: null, dtype: 'array' }, }, ], }) @@ -173,10 +179,19 @@ test('does not flash a run filled by a cache write', async () => { cells: [ { __typename: 'Cell', + id: cellId({ + database: PROPOSAL, + proposal: PROPOSAL, + run: 1, + name: 'spectrum', + }), name: 'spectrum', - value: [1, 2, 3], - dtype: 'array', error: null, + summary: { + __typename: 'CellSummary', + value: [1, 2, 3], + dtype: 'array', + }, }, ], }, @@ -186,7 +201,7 @@ test('does not flash a run filled by a cache write', async () => { // The fill landed, and the run still carries no flash stamp. await vi.waitFor(() => - expect(result.current.cellsByKey.get(key)?.spectrum.value).toEqual([ + expect(result.current.cellsByKey.get(key)?.spectrum.summary.value).toEqual([ 1, 2, 3, ]) ) diff --git a/frontend/packages/ui/tests/graphql/type-policies.test.ts b/frontend/packages/ui/tests/graphql/type-policies.test.ts index 4a3021a3..9a32daaf 100644 --- a/frontend/packages/ui/tests/graphql/type-policies.test.ts +++ b/frontend/packages/ui/tests/graphql/type-policies.test.ts @@ -6,6 +6,7 @@ import { type TableDataResult, } from '#src/data/table/table-data.queries' import { typePolicies } from '#src/graphql/type-policies' +import { cellId } from '#tests/support/cells' const PROPOSAL = '900405' @@ -15,30 +16,48 @@ beforeEach(() => { cache = new InMemoryCache({ typePolicies }) }) -// A cell as it sits in the cache, under its wire __typename. -type Cell = { +type CellError = { cls: string; message: string } + +// A cell as the wire sends it, before `run` stamps the normalization id and +// wraps the summary facet. +type CellInput = { name: string value: unknown dtype: string - error: { cls: string; message: string } | null + error: CellError | null } function cell( name: string, value: unknown, dtype = 'number', - error: Cell['error'] = null -): Cell { - return { __typename: 'Cell', name, value, dtype, error } as Cell + error: CellError | null = null +): CellInput { + return { name, value, dtype, error } } -function run(proposal: string, number: number, cells: Cell[]) { +function run(proposal: string, number: number, cells: CellInput[]) { return { __typename: 'DamnitRun', database: PROPOSAL, proposal, run: number, - cells, + cells: cells.map((entry) => ({ + __typename: 'Cell', + id: cellId({ + database: PROPOSAL, + proposal, + run: number, + name: entry.name, + }), + name: entry.name, + error: entry.error, + summary: { + __typename: 'CellSummary', + value: entry.value, + dtype: entry.dtype, + }, + })), } } @@ -64,7 +83,16 @@ const valueOf = ( ) => runs .find((entry) => entry.run === identity) - ?.cells.find((entry) => entry.name === name)?.value + ?.cells.find((entry) => entry.name === name)?.summary.value + +const dtypeOf = ( + runs: TableDataResult['runs'], + identity: number, + name: string +) => + runs + .find((entry) => entry.run === identity) + ?.cells.find((entry) => entry.name === name)?.summary.dtype const blanked = cell('spectrum', null, 'array') const filled = cell('spectrum', [1, 2, 3], 'array') @@ -74,7 +102,9 @@ test('the lightweight, deferred, and pushed cell sets share one run', () => { writeRuns([run(PROPOSAL, 1, [cell('energy', 10), blanked])]) expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() - // The deferred pass fills only the heavy value, keyed onto the same run. + // The deferred pass fills only the heavy value, keyed onto the same run. The + // cells list unions by identity, so `energy` survives even though this pass + // did not carry it. writeRuns([run(PROPOSAL, 1, [cell('run', 1), filled])]) expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) expect(valueOf(readRuns(), 1, 'energy')).toBe(10) @@ -84,12 +114,36 @@ test('a held-back blank does not overwrite a value already in place', () => { writeRuns([run(PROPOSAL, 1, [filled])]) // A cache-and-network refetch of the lightweight pass blanks the heavy value - // again; the merge keeps the value the deferred pass filled in. + // again; the CellSummary.value merge keeps the value the deferred pass filled. writeRuns([run(PROPOSAL, 1, [blanked])]) expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) }) +test('a retyped variable clears the value that no longer describes it', () => { + writeRuns([run(PROPOSAL, 1, [filled])]) + + // A context-file edit retypes `spectrum` from an array to an image, so the + // next lightweight pass blanks it under the new dtype. Keeping the array here + // would pair it with a dtype that cannot draw it, and the cell would never be + // fetched again because it would still look like it had a value. + writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'image')])]) + + expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() + expect(dtypeOf(readRuns(), 1, 'spectrum')).toBe('image') +}) + +test('a null scalar clears the value it had rather than keeping it', () => { + writeRuns([run(PROPOSAL, 1, [cell('energy', 10)])]) + expect(valueOf(readRuns(), 1, 'energy')).toBe(10) + + // Unlike a held-back heavy blank, a null scalar is DAMNIT clearing the value + // for this run, so the merge must let it through instead of keeping the stale + // number. + writeRuns([run(PROPOSAL, 1, [cell('energy', null)])]) + expect(valueOf(readRuns(), 1, 'energy')).toBeNull() +}) + test('a blank still lands on a cell that has no value yet', () => { // The grid draws a null as a loading skeleton until the deferred pass fills it. writeRuns([run(PROPOSAL, 1, [blanked])]) @@ -97,16 +151,20 @@ test('a blank still lands on a cell that has no value yet', () => { expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() }) -test('an errored blank replaces a value already in place', () => { - // A blank carrying an error is a real result, not a value held back. +test('a cell that fails after computing shows the error over its stale value', () => { const error = { cls: 'ValueError', message: 'boom' } writeRuns([run(PROPOSAL, 1, [filled])]) + // The variable errors on a later pass: the summary value is held back (null), + // but the error lands on the cell. The stale value stays cached and unseen, + // since the grid gives the error precedence. writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'array', error)])]) - const spectrum = readRuns()[0].cells.find((c) => c.name === 'spectrum') - expect(spectrum?.value).toBeNull() + const spectrum = readRuns()[0].cells.find( + (entry) => entry.name === 'spectrum' + ) expect(spectrum?.error).toEqual(error) + expect(spectrum?.summary.value).toEqual([1, 2, 3]) }) test('paginated runs accumulate into one list, deduped by identity', () => { @@ -130,3 +188,15 @@ test('runs that share a number across proposals stay separate', () => { expect(runs).toHaveLength(2) expect(cache.identify(runs[0])).not.toBe(cache.identify(runs[1])) }) + +test('cells sharing a name across runs are separate normalized entities', () => { + // The id folds in the run's identity, so one variable's cell in two runs + // never collapses onto a single cache object. + writeRuns([ + run(PROPOSAL, 1, [cell('energy', 1.2)]), + run(PROPOSAL, 2, [cell('energy', 9.9)]), + ]) + + expect(valueOf(readRuns(), 1, 'energy')).toBe(1.2) + expect(valueOf(readRuns(), 2, 'energy')).toBe(9.9) +}) diff --git a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx index 7b692006..e5103098 100644 --- a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx +++ b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx @@ -12,6 +12,7 @@ import { setupStore, type AppStore } from '#src/app/store/store' import { setProposalPending } from '#src/data/metadata/metadata.slice' import { LIGHTWEIGHT_TABLE_DATA_QUERY } from '#src/data/table/table-data.queries' import { cache } from '#src/graphql/apollo' +import { cellId } from '#tests/support/cells' // Leaving a proposal evicts its Apollo entries. The two things that has to get // right, one test each: the departed proposal is actually gone from the cache, @@ -27,21 +28,30 @@ const PROPOSAL_LIST = gql` } ` -function runsPayload(proposal: string) { +// A guest run: addressed through one handle but reporting a proposal of its +// own. The two differ on purpose, because the cell id leads with the handle and +// carries the proposal second, so a fixture that made them equal would let a +// matcher confuse the two and still pass. +function runsPayload(database: string) { + const proposal = `guest-${database}` return { runs: [ { __typename: 'DamnitRun', - database: proposal, + database, proposal, run: 1, cells: [ { __typename: 'Cell', + id: cellId({ database, proposal, run: 1, name: 'energy' }), name: 'energy', - value: 10, - dtype: 'number', error: null, + summary: { + __typename: 'CellSummary', + value: 10, + dtype: 'number', + }, }, ], }, @@ -110,13 +120,24 @@ function tree(store: AppStore, client: ApolloClient, proposal: string) { ) } -// Everything in the cache belonging to a proposal: the ROOT_QUERY fields keyed -// by its number, and the normalized runs those fields point at. -function cacheEntriesFor(proposal: string) { +// Everything in the cache belonging to one database handle: the ROOT_QUERY +// fields whose arguments name it, the runs addressed through it, and those runs' +// cells. All three spell the handle differently, so each needs its own match: a +// query field carries it as an argument, a run as the first of its identity +// trio, and a cell as the first segment of its id. Cells outnumber runs by the +// variable count, so missing them would hide most of a leak. +function cacheEntriesFor(database: string) { const snapshot = cache.extract() as Record + const handleOf = (key: string) => key.match(/^Cell:{"id":"([^:]+):/)?.[1] + return Object.entries(snapshot) .flatMap(([id, entry]) => (id === 'ROOT_QUERY' ? Object.keys(entry) : [id])) - .filter((key) => key.includes(`"proposal":"${proposal}"`)) + .filter( + (key) => + key.includes(`"proposal":"${database}"`) || + key.includes(`"database":"${database}"`) || + handleOf(key) === database + ) } beforeEach(async () => { diff --git a/frontend/packages/ui/tests/support/cells.ts b/frontend/packages/ui/tests/support/cells.ts new file mode 100644 index 00000000..1aa4e98b --- /dev/null +++ b/frontend/packages/ui/tests/support/cells.ts @@ -0,0 +1,18 @@ +// The server's cell id, "{database}:{proposal}:{run}:{name}" (built in +// `DamnitRun._iter_cells`). Apollo keys `Cell` on it, so a fixture that spells +// it differently mints a second entity for the same cell, which is exactly the +// drift these tests exist to catch. `database` leads and is not the proposal: a +// guest run is served through one database and reports another proposal. +export function cellId({ + database, + proposal, + run, + name, +}: { + database: string + proposal: string + run: number + name: string +}): string { + return `${database}:${proposal}:${run}:${name}` +} From 234028182bce6b5343061828382b0785d6b39326 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 15:56:07 +0200 Subject: [PATCH 03/14] fix(frontend/table): tell a genuine null cell from a held-back blank --- frontend/packages/ui/src/constants.ts | 16 ++++---- .../src/data/table/table-data.transforms.ts | 12 +++--- .../packages/ui/src/features/table/cells.ts | 18 ++++---- .../packages/ui/src/graphql/type-policies.ts | 23 +++++------ .../data/table/table-data.transforms.test.ts | 41 ++++++++----------- .../ui/tests/features/table/cells.test.ts | 18 +++++++- 6 files changed, 67 insertions(+), 61 deletions(-) diff --git a/frontend/packages/ui/src/constants.ts b/frontend/packages/ui/src/constants.ts index 88cb35f0..8071b3ba 100644 --- a/frontend/packages/ui/src/constants.ts +++ b/frontend/packages/ui/src/constants.ts @@ -7,21 +7,19 @@ import { formatUrl } from './utils/helpers' // mock server so both track the API's HEAVY_DATA from one place. export { HEAVY_DTYPES } -// An errorless null with a heavy dtype is a value @lightweight held back, not a -// genuine absence: only heavy dtypes are blanked, and a real failure carries an -// error. A null scalar is a cell DAMNIT has no value for. The cache merge policy -// and the table's deferred-fetch selector both decide "still to come" by this -// one rule, over their own cell shapes, so keeping it here stops them drifting. -export function isHeavyBlank({ +// A summary whose value is null under a heavy dtype is one @lightweight held +// back, not a genuine absence: only heavy dtypes are blanked, so a null scalar +// is a cell DAMNIT has no value for. Every caller that decides "still to come" +// reads this one rule over its own cell shape, so keeping it here stops them +// drifting. +export function isHeavySummaryBlank({ value, - error, dtype, }: { value: unknown - error: unknown dtype: string }): boolean { - return value == null && error == null && HEAVY_DTYPES.has(dtype) + return value == null && HEAVY_DTYPES.has(dtype) } export const CONTACT_EMAIL = 'da@xfel.eu' diff --git a/frontend/packages/ui/src/data/table/table-data.transforms.ts b/frontend/packages/ui/src/data/table/table-data.transforms.ts index abbf0eac..99402481 100644 --- a/frontend/packages/ui/src/data/table/table-data.transforms.ts +++ b/frontend/packages/ui/src/data/table/table-data.transforms.ts @@ -1,16 +1,14 @@ -import { isHeavyBlank } from '#src/constants' +import { isHeavySummaryBlank } from '#src/constants' import type { Cell, Run, RunCells, RunId, Variable } from './table-data.types' // A value the server is still holding back on the table's first pass, rather // than one that is genuinely absent. Shares its rule with the cache merge policy -// through `isHeavyBlank`, so the two cannot disagree on what is still to come. +// through `isHeavySummaryBlank`, so the two cannot disagree on what is still to +// come. A cell that failed carries an error, and its blank is the final answer +// rather than a value on its way. function isDeferred(cell: Cell): boolean { - return isHeavyBlank({ - value: cell.summary.value, - error: cell.error, - dtype: cell.summary.dtype, - }) + return cell.error == null && isHeavySummaryBlank(cell.summary) } // The cells worth a second, heavier fetch: the ones @lightweight held back. diff --git a/frontend/packages/ui/src/features/table/cells.ts b/frontend/packages/ui/src/features/table/cells.ts index bb0e6d66..5ed125e3 100644 --- a/frontend/packages/ui/src/features/table/cells.ts +++ b/frontend/packages/ui/src/features/table/cells.ts @@ -12,7 +12,7 @@ import { } from '@glideapps/glide-data-grid' import { type SparklineCellType } from '@glideapps/glide-data-grid-cells' -import { DTYPES, HEAVY_DTYPES } from '#src/constants' +import { DTYPES, isHeavySummaryBlank } from '#src/constants' import { type CellError, type CellValue, @@ -280,7 +280,7 @@ const gridCellFactory = { type GetCellOptions = { value: CellValue - dtype: keyof typeof gridCellFactory + dtype: string options: Partial } @@ -289,14 +289,16 @@ export const getCell = ({ dtype, options, }: GetCellOptions): GridCell => { + // The grid asks for every visible cell on every redraw, so the populated case + // comes first and allocates nothing on its way through. + if (value != null) { + return gridCellFactory[dtype](value, options) + } // A null heavy value is one @lightweight held back, so it draws the loading // skeleton until the deferred fetch fills it. A null scalar is a cell DAMNIT // has no value for and nothing is coming, so it draws as empty rather than // loading forever. - if (value == null) { - return HEAVY_DTYPES.has(String(dtype)) - ? loadingCell(value, options) - : textCell('') - } - return gridCellFactory[dtype](value, options) + return isHeavySummaryBlank({ value, dtype }) + ? loadingCell(value, options) + : textCell('') } diff --git a/frontend/packages/ui/src/graphql/type-policies.ts b/frontend/packages/ui/src/graphql/type-policies.ts index 00441754..b8d9aec0 100644 --- a/frontend/packages/ui/src/graphql/type-policies.ts +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -5,7 +5,7 @@ import type { TypePolicies, } from '@apollo/client' -import { isHeavyBlank } from '#src/constants' +import { isHeavySummaryBlank } from '#src/constants' // Accumulate normalized refs into one list, deduped by Apollo's own cache id // (`__ref`), the identity it already computed from keyFields. When nothing new @@ -63,7 +63,7 @@ function mergeSummary( dtype != null && dtype === existing.dtype && existing.value != null && - isHeavyBlank({ value: incoming.value, error: null, dtype }) + isHeavySummaryBlank({ value: incoming.value, dtype }) return heldBackBlank ? existing : mergeObjects(existing, incoming) } @@ -71,12 +71,9 @@ export const typePolicies: TypePolicies = { DamnitRun: { keyFields: ['database', 'proposal', 'run'], fields: { - // Cell refs, one list per run. Cell is a normalized entity, so the - // two-pass table (lightweight blanks, then a heavier deferred fill) writes - // both passes to the same cell object; this list only owes membership. - // Without it, the deferred pass's shorter `cells` array would replace the - // lightweight one and drop cells absent from the second fetch. Value - // protection lives on `CellSummary.value`. + // Cell refs, one list per run. Both table passes write to the same + // normalized cells, so this list only owes membership: without it the + // deferred pass's shorter array would drop the cells it did not carry. cells: { keyArgs: false, merge: mergeRefsByIdentity, @@ -87,11 +84,11 @@ export const typePolicies: TypePolicies = { keyFields: ['id'], }, CellSummary: { - // Merge at the summary level: a merge function only sees the field it - // merges, and this is the only level where `dtype` travels with the value. - // `error` is cell-level, so it cannot be read here; the API drops the - // summary type of a failed cell so that a failure never looks like a blank - // held back. + // The value guard lives here rather than on `Cell` because Apollo forbids a + // merge function from reading sibling fields, so this is the only level that + // can see `dtype` alongside the value. `error` is cell-level, so it cannot + // be read here; the API drops the summary type of a failed cell so that a + // failure never looks like a blank held back. merge: mergeSummary, }, Query: { diff --git a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts index d09b00fb..45fdb286 100644 --- a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts +++ b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts @@ -6,39 +6,25 @@ import { runKey, } from '#src/data/table/table-data.transforms' import type { + Cell, CellError, CellValue, Run, } from '#src/data/table/table-data.types' -type CellInput = { - name: string - value: CellValue - dtype: string - error: CellError | null -} - +// These transforms key cells by name and never read `id`, so it only has to be +// present, not realistic. function cell( name: string, value: CellValue, dtype = 'number', error: CellError | null = null -): CellInput { - return { name, value, dtype, error } +): Cell { + return { id: name, name, error, summary: { value, dtype } } } -function run(proposal: string, number: number, cells: CellInput[]): Run { - return { - database: proposal, - proposal, - run: number, - cells: cells.map((entry) => ({ - id: `${proposal}:${number}:${entry.name}`, - name: entry.name, - error: entry.error, - summary: { value: entry.value, dtype: entry.dtype }, - })), - } +function run(proposal: string, number: number, cells: Cell[]): Run { + return { database: proposal, proposal, run: number, cells } } describe('indexRunCells', () => { @@ -50,7 +36,7 @@ describe('indexRunCells', () => { expect([...cells.keys()]).toEqual(['900405:5', '900405:9']) expect(cells.get('900405:5')?.energy).toEqual({ - id: '900405:5:energy', + id: 'energy', name: 'energy', error: null, summary: { value: 1.2, dtype: 'number' }, @@ -75,7 +61,7 @@ describe('indexRunCells', () => { run('900405', 1, [cell('x', 2, 'number', error)]), ]) expect(cells.get('900405:1')?.x).toEqual({ - id: '900405:1:x', + id: 'x', name: 'x', error, summary: { value: 2, dtype: 'number' }, @@ -145,4 +131,13 @@ describe('heavyCellNames', () => { expect(names).toEqual(['spectrum']) }) + + test('leaves out a genuinely-empty scalar cell', () => { + // A null scalar (no error, non-heavy dtype) is a deleted-for-this-run + // value, not a held-back heavy blank, so re-fetching it would loop forever. + const empty = cell('note', null, 'string') + const names = heavyCellNames([run('900405', 1, [empty])]) + + expect(names).toEqual([]) + }) }) diff --git a/frontend/packages/ui/tests/features/table/cells.test.ts b/frontend/packages/ui/tests/features/table/cells.test.ts index 08784252..5ea215fb 100644 --- a/frontend/packages/ui/tests/features/table/cells.test.ts +++ b/frontend/packages/ui/tests/features/table/cells.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'vitest' -import { GridCellKind } from '@glideapps/glide-data-grid' +import { GridCellKind, type TextCell } from '@glideapps/glide-data-grid' import { arrayCell, @@ -13,6 +13,7 @@ import { textCell, } from '#src/features/table/cells' import { DTYPES } from '#src/constants' +import type { CellValue } from '#src/data/table/table-data.types' describe('getCell', () => { // Only a heavy dtype is ever held back by @lightweight, so only a heavy null @@ -21,6 +22,21 @@ describe('getCell', () => { expect( getCell({ value: undefined, dtype: DTYPES.image, options: {} }).kind ).toBe(GridCellKind.Loading) + expect( + getCell({ value: undefined, dtype: DTYPES.array, options: {} }).kind + ).toBe(GridCellKind.Loading) + }) + + // A missing scalar is a genuinely empty cell, not a pending fetch, so it + // renders blank instead of spinning forever. + test('renders a blank cell for a genuinely empty scalar', () => { + const cell = getCell({ + value: null as unknown as CellValue, + dtype: DTYPES.number, + options: {}, + }) + expect(cell.kind).toBe(GridCellKind.Text) + expect((cell as TextCell).displayData).toBe('') }) test('renders an empty cell for a missing scalar, not a loading one', () => { From cc3a834b276d2bafde924de008cd62214516f6a8 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 15:56:20 +0200 Subject: [PATCH 04/14] fix(frontend/table): fall back to a text cell for an unknown dtype --- .../ui/src/data/table/table-data.types.ts | 4 +- .../packages/ui/src/features/table/cells.ts | 10 +++-- .../ui/tests/features/table/cells.test.ts | 39 +++++++++++-------- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/frontend/packages/ui/src/data/table/table-data.types.ts b/frontend/packages/ui/src/data/table/table-data.types.ts index d043004c..09df5fd2 100644 --- a/frontend/packages/ui/src/data/table/table-data.types.ts +++ b/frontend/packages/ui/src/data/table/table-data.types.ts @@ -1,4 +1,6 @@ -export type CellValue = string | number | number[] | null | undefined +// `boolean` has no renderer of its own and falls back to text, but the API's +// `Any` scalar does emit one, so leaving it out would only hide it. +export type CellValue = string | number | boolean | number[] | null | undefined export type CellError = { message: string diff --git a/frontend/packages/ui/src/features/table/cells.ts b/frontend/packages/ui/src/features/table/cells.ts index 5ed125e3..7097b8da 100644 --- a/frontend/packages/ui/src/features/table/cells.ts +++ b/frontend/packages/ui/src/features/table/cells.ts @@ -45,7 +45,9 @@ export const textCell = ( value: CellValue, params: Partial = {} ): TextCell => { - const data = value ? String(value) : '' + // Not a truthiness check: this is the fallback for a dtype with no renderer, + // and `false`, `0` and `NaN` are values the grid still has to show. + const data = value == null ? '' : String(value) return { kind: GridCellKind.Text, displayData: data, @@ -290,9 +292,11 @@ export const getCell = ({ options, }: GetCellOptions): GridCell => { // The grid asks for every visible cell on every redraw, so the populated case - // comes first and allocates nothing on its way through. + // comes first and allocates nothing on its way through. A dtype with no + // renderer (e.g. a boolean cell) falls back to text rather than crashing + // every visible cell the grid asks `getContent` for. if (value != null) { - return gridCellFactory[dtype](value, options) + return (gridCellFactory[dtype] ?? textCell)(value, options) } // A null heavy value is one @lightweight held back, so it draws the loading // skeleton until the deferred fetch fills it. A null scalar is a cell DAMNIT diff --git a/frontend/packages/ui/tests/features/table/cells.test.ts b/frontend/packages/ui/tests/features/table/cells.test.ts index 5ea215fb..96cfb684 100644 --- a/frontend/packages/ui/tests/features/table/cells.test.ts +++ b/frontend/packages/ui/tests/features/table/cells.test.ts @@ -13,7 +13,6 @@ import { textCell, } from '#src/features/table/cells' import { DTYPES } from '#src/constants' -import type { CellValue } from '#src/data/table/table-data.types' describe('getCell', () => { // Only a heavy dtype is ever held back by @lightweight, so only a heavy null @@ -29,22 +28,13 @@ describe('getCell', () => { // A missing scalar is a genuinely empty cell, not a pending fetch, so it // renders blank instead of spinning forever. - test('renders a blank cell for a genuinely empty scalar', () => { - const cell = getCell({ - value: null as unknown as CellValue, - dtype: DTYPES.number, - options: {}, - }) + test('renders an empty cell for a missing scalar, not a loading one', () => { + const cell = getCell({ value: null, dtype: DTYPES.number, options: {} }) + expect(cell.kind).toBe(GridCellKind.Text) expect((cell as TextCell).displayData).toBe('') }) - test('renders an empty cell for a missing scalar, not a loading one', () => { - expect( - getCell({ value: undefined, dtype: DTYPES.number, options: {} }).kind - ).toBe(GridCellKind.Text) - }) - test('picks the cell type from the dtype when a value is present', () => { expect( getCell({ value: 3.14159, dtype: DTYPES.number, options: {} }).kind @@ -53,6 +43,15 @@ describe('getCell', () => { getCell({ value: 'hi', dtype: DTYPES.string, options: {} }).kind ).toBe(GridCellKind.Text) }) + + // A dtype with no renderer (a boolean cell has none) must not throw: the grid + // asks getContent for every visible cell, so one would break the whole table. + test('falls back to a text cell for a dtype with no renderer', () => { + const cell = getCell({ value: false, dtype: 'boolean', options: {} }) + + expect(cell.kind).toBe(GridCellKind.Text) + expect((cell as TextCell).displayData).toBe('false') + }) }) describe('numberCell', () => { @@ -124,10 +123,16 @@ describe('textCell', () => { expect(cell.displayData).toBe('hello') }) - test('renders an empty string for the falsy 0', () => { - const cell = textCell(0) - expect(cell.data).toBe('') - expect(cell.displayData).toBe('') + // The unknown-dtype fallback routes through here, so a false or a zero is a + // real value the grid still has to show, not an absent one. + test('stringifies a falsy value rather than blanking it', () => { + expect(textCell(0).displayData).toBe('0') + expect(textCell(false).displayData).toBe('false') + }) + + test('renders an empty string for a missing value', () => { + expect(textCell(null).displayData).toBe('') + expect(textCell(undefined).displayData).toBe('') }) }) From f5c80460606843c2c40bef8046d19106c71b05c4 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 15:56:20 +0200 Subject: [PATCH 05/14] perf(frontend/graphql): raise apollo's read-memo cap for normalized cells --- frontend/packages/ui/src/graphql/apollo.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/frontend/packages/ui/src/graphql/apollo.ts b/frontend/packages/ui/src/graphql/apollo.ts index b293a9f7..35aa9b33 100644 --- a/frontend/packages/ui/src/graphql/apollo.ts +++ b/frontend/packages/ui/src/graphql/apollo.ts @@ -61,7 +61,24 @@ const splitLink = split( from([priorityLink, httpLink]) ) -export const cache = new InMemoryCache({ typePolicies }) +// Apollo memoizes each read per (selection set, object), capped at 50,000 +// entries by default. A cell costs two of those, the `Cell` and its +// `CellSummary`, so a table of a few hundred runs times its variables can fill +// the cap on its own. Past it every cache write re-reads the whole table instead +// of returning the memoized result, which is seconds of main thread per +// paginated page. The budget is shared across documents, and the summary plot +// asks for every run at once, so give it room. +// +// One knob, three caches: `resultCacheMaxSize` is the `max` for +// `executeSelectionSet` (the one reasoned about above), `executeSubSelectedArray` +// and `maybeBroadcastWatch`, whose own defaults are 10,000 and 5,000. Only the +// first is the binding constraint here, because it holds two entries per cell +// while the others hold roughly one per run, so raising all three is headroom +// the other two never reach rather than a budget worth splitting. +export const cache = new InMemoryCache({ + typePolicies, + resultCacheMaxSize: 200_000, +}) export const client = new ApolloClient({ cache, From a094f13cbd5268d672949cfe66e739cfe8a01e6e Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 16:57:07 +0200 Subject: [PATCH 06/14] fix(frontend/table): check the cell error before offering a preview --- frontend/packages/ui/src/features/dashboard/run.tsx | 5 ++--- .../ui/src/features/plots/use-summary-plot-data.ts | 3 +++ frontend/packages/ui/src/features/table/table.tsx | 7 +++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/frontend/packages/ui/src/features/dashboard/run.tsx b/frontend/packages/ui/src/features/dashboard/run.tsx index 6632b3c5..682b376b 100644 --- a/frontend/packages/ui/src/features/dashboard/run.tsx +++ b/frontend/packages/ui/src/features/dashboard/run.tsx @@ -153,9 +153,8 @@ const Run = () => { {validRuns.map(([name, data]) => { const label = metadataVariables[name]?.title || name - // A failed cell keeps its stale value in the cache; the grid gives the - // error precedence, so the aside must too, showing the error rather - // than the stale value. + // The error comes first, the same order the grid and the plots use: a + // cell that failed has nothing worth rendering from its summary. if (data.error) { return renderError({ name, label, error: data.error }) } diff --git a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts index 784d207f..2644ce1b 100644 --- a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts +++ b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts @@ -64,9 +64,12 @@ export function useSummaryPlotData({ for (const id of runIds) { const row = cells.get(runKey(id)) const points = variables.map((name) => row?.[name]) + // A failed cell is out whatever its value reads as, the same rule the + // grid, the aside and the plot context menu all apply. const allNumeric = points.every( (point) => point != null && + point.error == null && typeof point.summary.value === 'number' && point.summary.dtype === DTYPES.number ) diff --git a/frontend/packages/ui/src/features/table/table.tsx b/frontend/packages/ui/src/features/table/table.tsx index 75c47d37..c6de8165 100644 --- a/frontend/packages/ui/src/features/table/table.tsx +++ b/frontend/packages/ui/src/features/table/table.tsx @@ -265,9 +265,12 @@ const Table = ({ grid, paginated = true }: TableProps) => { const rowData = identity && cellsByKey.get(runKey(identity)) // A row whose page has not loaded yet has no data at all, not merely no - // value: it has nothing to offer a plot either way. + // value: it has nothing to offer a plot either way. A failed cell arrives + // with a null value, so it is already out; checking the error keeps this in + // step with the grid and the aside. // TODO: Use extracted data type from the database - if (col !== -1 && rowData?.[column]?.summary.value != null) { + const cell = rowData?.[column] + if (col !== -1 && cell?.error == null && cell?.summary.value != null) { const variable = tableColumns[col] const subtitle = `${variable.title}` From f59aacc902d5537c4917ab38beea740c6d40d0c2 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Mon, 27 Jul 2026 16:57:13 +0200 Subject: [PATCH 07/14] test(frontend/graphql): pin the real wire shape of a failed cell --- .../packages/ui/src/graphql/type-policies.ts | 11 +++---- .../ui/tests/graphql/type-policies.test.ts | 30 +++++++++++++++---- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/frontend/packages/ui/src/graphql/type-policies.ts b/frontend/packages/ui/src/graphql/type-policies.ts index b8d9aec0..5f78fb42 100644 --- a/frontend/packages/ui/src/graphql/type-policies.ts +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -84,11 +84,12 @@ export const typePolicies: TypePolicies = { keyFields: ['id'], }, CellSummary: { - // The value guard lives here rather than on `Cell` because Apollo forbids a - // merge function from reading sibling fields, so this is the only level that - // can see `dtype` alongside the value. `error` is cell-level, so it cannot - // be read here; the API drops the summary type of a failed cell so that a - // failure never looks like a blank held back. + // The value guard lives here rather than on `Cell` because a merge function + // only sees the field it merges, so this is the only level that can see + // `dtype` alongside the value. A failed cell needs no case of its own, and + // could not get one: `error` is cell-level and unreadable from here. The API + // drops the summary type of a failed cell instead, so it arrives as a null + // string rather than a heavy blank and clears the value it had. merge: mergeSummary, }, Query: { diff --git a/frontend/packages/ui/tests/graphql/type-policies.test.ts b/frontend/packages/ui/tests/graphql/type-policies.test.ts index 9a32daaf..b9355e99 100644 --- a/frontend/packages/ui/tests/graphql/type-policies.test.ts +++ b/frontend/packages/ui/tests/graphql/type-policies.test.ts @@ -151,20 +151,38 @@ test('a blank still lands on a cell that has no value yet', () => { expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() }) -test('a cell that fails after computing shows the error over its stale value', () => { +test('a cell that fails after computing clears the value it had', () => { const error = { cls: 'ValueError', message: 'boom' } writeRuns([run(PROPOSAL, 1, [filled])]) - // The variable errors on a later pass: the summary value is held back (null), - // but the error lands on the cell. The stale value stays cached and unseen, - // since the grid gives the error precedence. - writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'array', error)])]) + // DAMNIT stores a failed variable with a null value and no summary type, so + // it comes back as a null string, not a held-back heavy blank: the error + // lands and the array it had goes with it. + writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'string', error)])]) const spectrum = readRuns()[0].cells.find( (entry) => entry.name === 'spectrum' ) expect(spectrum?.error).toEqual(error) - expect(spectrum?.summary.value).toEqual([1, 2, 3]) + expect(spectrum?.summary.value).toBeNull() +}) + +test('an error alone cannot clear a value under a heavy dtype', () => { + writeRuns([run(PROPOSAL, 1, [filled])]) + + // The merge sees only the summary, so the error beside it is invisible here + // and the blank still reads as one @lightweight held back. Nothing on the + // client can close this: what keeps it unreachable is the API dropping the + // summary type of a failed cell (`DamnitRun._iter_cells`), which turns the + // case above into the null string the previous test covers. If that guarantee + // goes, this is the value that gets pinned behind the error. + writeRuns([ + run(PROPOSAL, 1, [ + cell('spectrum', null, 'array', { cls: 'ValueError', message: 'boom' }), + ]), + ]) + + expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) }) test('paginated runs accumulate into one list, deduped by identity', () => { From f9bb33f6b5e887624ba993e1847e71e347a8ccd9 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 02:10:15 +0200 Subject: [PATCH 08/14] fix(frontend/graphql): stop caching the run-updates subscription result --- frontend/packages/ui/src/app/store/listeners.ts | 6 ++++-- frontend/packages/ui/src/data/use-proposal.ts | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/frontend/packages/ui/src/app/store/listeners.ts b/frontend/packages/ui/src/app/store/listeners.ts index 5d46471e..11450e39 100644 --- a/frontend/packages/ui/src/app/store/listeners.ts +++ b/frontend/packages/ui/src/app/store/listeners.ts @@ -54,8 +54,10 @@ export function registerAppListeners() { // This is what reclaims the memory, not a tidy-up. Runs normalize to // top-level `DamnitRun:{...}` entries, so dropping the fields above // only removes the references to them; the entries themselves sit - // there as orphans, images and all, until the collector runs. - cache.gc() + // there as orphans, images and all, until the collector runs. The read + // memo has to go with them: it holds the results read out of those + // entries, so leaving it would keep the payload alive past the sweep. + cache.gc({ resetResultCache: true }) }) ) }, diff --git a/frontend/packages/ui/src/data/use-proposal.ts b/frontend/packages/ui/src/data/use-proposal.ts index cbdb88f9..df29a44f 100644 --- a/frontend/packages/ui/src/data/use-proposal.ts +++ b/frontend/packages/ui/src/data/use-proposal.ts @@ -66,6 +66,12 @@ const useProposal = ({ subscribe = true }: UseProposalOptions) => { variables: { proposal: proposal.value, since }, skip: !subscribe || proposal.loading || proposal.notFound || !proposal.value, + // `onData` below writes every push into the cache itself, so Apollo's own + // write would be the same payload a second time, filed under + // ROOT_SUBSCRIPTION where nothing reads it. That root is never collected, + // so it would also hold every pushed cell (images included) for as long as + // the tab lives. + fetchPolicy: 'no-cache', onData: ({ data, client }) => { const update = data.data?.run_updates if (!update) { From 09d19a4acc51becfc1fd3b0381cf592280aa2bd8 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 03:50:18 +0200 Subject: [PATCH 09/14] refactor(frontend/mocks): share the cell-id builder with the tests --- frontend/packages/shared/src/mocks/index.ts | 1 + frontend/packages/shared/src/mocks/shape.ts | 46 ++++++++++++------- .../features/table/use-table-runs.test.tsx | 42 ++++++----------- .../ui/tests/graphql/type-policies.test.ts | 39 ++++++---------- .../ui/tests/redux/proposal-teardown.test.tsx | 14 +----- frontend/packages/ui/tests/support/cells.ts | 35 ++++++++++---- 6 files changed, 88 insertions(+), 89 deletions(-) diff --git a/frontend/packages/shared/src/mocks/index.ts b/frontend/packages/shared/src/mocks/index.ts index 8721c189..f4141c6d 100644 --- a/frontend/packages/shared/src/mocks/index.ts +++ b/frontend/packages/shared/src/mocks/index.ts @@ -1,5 +1,6 @@ export { REST_API_PREFIXES, + cellId, shapeMetadata, shapeRun, shapeTableData, diff --git a/frontend/packages/shared/src/mocks/shape.ts b/frontend/packages/shared/src/mocks/shape.ts index e78a2a16..e9b957bc 100644 --- a/frontend/packages/shared/src/mocks/shape.ts +++ b/frontend/packages/shared/src/mocks/shape.ts @@ -25,13 +25,20 @@ export function shapeMetadata(meta: Meta, proposal: string) { } } +// The server's cell id, built the same way `DamnitRun._iter_cells` builds it. +// Apollo keys `Cell` on it, so anything that spells it differently mints a +// second entity for the same cell. `database` leads and is not the proposal: a +// guest run is served through one database and reports another proposal. +export function cellId({ database, proposal, run, name }: CellIdParts): string { + return `${database}:${proposal}:${run}:${name}` +} + // One cell's wire object. Shared by the query mock and the subscription mock so -// the composite `Cell` shape stays identical on both paths. The `id` is built -// from the run's identity, so a live push lands on the same normalized cache -// object the table already holds. `error` is always sent, even absent from the -// example: the query selects it, so omitting it leaves the client's cache read -// incomplete and every cached replay silently refetches. `lightweight` blanks a -// heavy value the way the real @lightweight pass does. +// the composite `Cell` shape stays identical on both paths. `error` is always +// sent, even absent from the example: the query selects it, so omitting it +// leaves the client's cache read incomplete and every cached replay silently +// refetches. `lightweight` blanks a heavy value the way the real @lightweight +// pass does. function shapeCell( name: string, cell: RunData['variables'][string], @@ -39,7 +46,7 @@ function shapeCell( ) { return { __typename: 'Cell', - id: `${database}:${proposal}:${run}:${name}`, + id: cellId({ database, proposal, run, name }), name, error: 'error' in cell ? { __typename: 'CellError', ...cell.error } : null, summary: { @@ -54,15 +61,11 @@ function shapeCell( // whose ids are built from that same trio. Shared by the query mock and the // subscription mock, so the two cannot disagree on how a run and its cells are // keyed, and a live push lands on the run the table already holds. -// -// `run` is the logical run number the metadata list and the grid rows use, not -// the physical source run number: in the xpcs example, runs 1 to 6 come from -// source runs 6, 7, 11, 33, 34 and 35. Pass the wrong one and the run and every -// one of its cells key to a row no grid reads. export function shapeRun( variables: RunData['variables'], - { database, proposal, run, names, lightweight = false }: ShapeRunOptions + options: ShapeRunOptions ) { + const { database, proposal, run, names } = options return { __typename: 'DamnitRun', database, @@ -70,9 +73,7 @@ export function shapeRun( run, cells: Object.entries(variables) .filter(([name]) => names == null || names.includes(name)) - .map(([name, cell]) => - shapeCell(name, cell, { database, proposal, run, lightweight }) - ), + .map(([name, cell]) => shapeCell(name, cell, options)), } } @@ -87,6 +88,10 @@ export function shapeTableData( // single-proposal, so a run's own proposal is the queried one too. database: proposal, proposal, + // The logical run number the metadata list and the grid rows use, not + // the physical source run number: in the xpcs example, runs 1 to 6 come + // from source runs 6, 7, 11, 33, 34 and 35. Take the wrong one and the + // run and every one of its cells key to a row no grid reads. run: Number(run.variables.run?.value ?? run.source.run_number), names, lightweight, @@ -95,6 +100,13 @@ export function shapeTableData( } } +type CellIdParts = { + database: string + proposal: string + run: number + name: string +} + type ShapeCellOptions = { database: string proposal: string @@ -102,6 +114,8 @@ type ShapeCellOptions = { lightweight?: boolean } +// A run takes everything a cell takes, because it forwards the bag straight +// through, plus the name filter that only makes sense over a whole run. type ShapeRunOptions = ShapeCellOptions & { names?: string[] | null } diff --git a/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx b/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx index cae2b1b9..2b239fbd 100644 --- a/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx +++ b/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx @@ -18,7 +18,7 @@ import { TABLE_DATA_QUERY } from '#src/data/table/table-data.queries' import { createPriorityLink } from '#src/graphql/priority-link' import { typePolicies } from '#src/graphql/type-policies' import { useTableRuns } from '#src/features/table/use-table-runs' -import { cellId } from '#tests/support/cells' +import { serverCell } from '#tests/support/cells' const PROPOSAL = '900405' const PAGE_SIZE = 10 @@ -38,18 +38,14 @@ const runFor = (run: number) => ({ proposal: PROPOSAL, run, cells: [ - { - __typename: 'Cell', - id: cellId({ - database: PROPOSAL, - proposal: PROPOSAL, - run, - name: 'spectrum', - }), + serverCell({ + database: PROPOSAL, + proposal: PROPOSAL, + run, name: 'spectrum', - error: null, - summary: { __typename: 'CellSummary', value: null, dtype: 'array' }, - }, + value: null, + dtype: 'array', + }), ], }) @@ -177,22 +173,14 @@ test('does not flash a run filled by a cache write', async () => { proposal: PROPOSAL, run: 1, cells: [ - { - __typename: 'Cell', - id: cellId({ - database: PROPOSAL, - proposal: PROPOSAL, - run: 1, - name: 'spectrum', - }), + serverCell({ + database: PROPOSAL, + proposal: PROPOSAL, + run: 1, name: 'spectrum', - error: null, - summary: { - __typename: 'CellSummary', - value: [1, 2, 3], - dtype: 'array', - }, - }, + value: [1, 2, 3], + dtype: 'array', + }), ], }, ], diff --git a/frontend/packages/ui/tests/graphql/type-policies.test.ts b/frontend/packages/ui/tests/graphql/type-policies.test.ts index b9355e99..485dafad 100644 --- a/frontend/packages/ui/tests/graphql/type-policies.test.ts +++ b/frontend/packages/ui/tests/graphql/type-policies.test.ts @@ -6,7 +6,7 @@ import { type TableDataResult, } from '#src/data/table/table-data.queries' import { typePolicies } from '#src/graphql/type-policies' -import { cellId } from '#tests/support/cells' +import { serverCell } from '#tests/support/cells' const PROPOSAL = '900405' @@ -18,8 +18,7 @@ beforeEach(() => { type CellError = { cls: string; message: string } -// A cell as the wire sends it, before `run` stamps the normalization id and -// wraps the summary facet. +// A cell before `run` stamps it with the identity it is keyed by. type CellInput = { name: string value: unknown @@ -42,22 +41,9 @@ function run(proposal: string, number: number, cells: CellInput[]) { database: PROPOSAL, proposal, run: number, - cells: cells.map((entry) => ({ - __typename: 'Cell', - id: cellId({ - database: PROPOSAL, - proposal, - run: number, - name: entry.name, - }), - name: entry.name, - error: entry.error, - summary: { - __typename: 'CellSummary', - value: entry.value, - dtype: entry.dtype, - }, - })), + cells: cells.map((entry) => + serverCell({ database: PROPOSAL, proposal, run: number, ...entry }) + ), } } @@ -76,23 +62,26 @@ function readRuns() { })!.runs } -const valueOf = ( +const summaryOf = ( runs: TableDataResult['runs'], identity: number, name: string ) => runs .find((entry) => entry.run === identity) - ?.cells.find((entry) => entry.name === name)?.summary.value + ?.cells.find((entry) => entry.name === name)?.summary + +const valueOf = ( + runs: TableDataResult['runs'], + identity: number, + name: string +) => summaryOf(runs, identity, name)?.value const dtypeOf = ( runs: TableDataResult['runs'], identity: number, name: string -) => - runs - .find((entry) => entry.run === identity) - ?.cells.find((entry) => entry.name === name)?.summary.dtype +) => summaryOf(runs, identity, name)?.dtype const blanked = cell('spectrum', null, 'array') const filled = cell('spectrum', [1, 2, 3], 'array') diff --git a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx index e5103098..0b79a28f 100644 --- a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx +++ b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx @@ -12,7 +12,7 @@ import { setupStore, type AppStore } from '#src/app/store/store' import { setProposalPending } from '#src/data/metadata/metadata.slice' import { LIGHTWEIGHT_TABLE_DATA_QUERY } from '#src/data/table/table-data.queries' import { cache } from '#src/graphql/apollo' -import { cellId } from '#tests/support/cells' +import { serverCell } from '#tests/support/cells' // Leaving a proposal evicts its Apollo entries. The two things that has to get // right, one test each: the departed proposal is actually gone from the cache, @@ -42,17 +42,7 @@ function runsPayload(database: string) { proposal, run: 1, cells: [ - { - __typename: 'Cell', - id: cellId({ database, proposal, run: 1, name: 'energy' }), - name: 'energy', - error: null, - summary: { - __typename: 'CellSummary', - value: 10, - dtype: 'number', - }, - }, + serverCell({ database, proposal, run: 1, name: 'energy', value: 10 }), ], }, ], diff --git a/frontend/packages/ui/tests/support/cells.ts b/frontend/packages/ui/tests/support/cells.ts index 1aa4e98b..5c400030 100644 --- a/frontend/packages/ui/tests/support/cells.ts +++ b/frontend/packages/ui/tests/support/cells.ts @@ -1,18 +1,35 @@ -// The server's cell id, "{database}:{proposal}:{run}:{name}" (built in -// `DamnitRun._iter_cells`). Apollo keys `Cell` on it, so a fixture that spells -// it differently mints a second entity for the same cell, which is exactly the -// drift these tests exist to catch. `database` leads and is not the proposal: a -// guest run is served through one database and reports another proposal. -export function cellId({ +import { cellId } from '@damnit-frontend/shared/mocks' + +// One cell as the server sends it, built the way the mock server builds it. +// Apollo keys `Cell` on the id, so a fixture that spells the composite shape its +// own way mints a second entity for the same cell, which is exactly the drift +// these tests exist to catch. +export function serverCell({ database, proposal, run, name, -}: { + value, + dtype = 'number', + error = null, +}: ServerCellOptions) { + return { + __typename: 'Cell', + id: cellId({ database, proposal, run, name }), + name, + error, + summary: { __typename: 'CellSummary', value, dtype }, + } +} + +type ServerCellOptions = { database: string proposal: string run: number name: string -}): string { - return `${database}:${proposal}:${run}:${name}` + value: unknown + dtype?: string + error?: CellError | null } + +type CellError = { cls: string; message: string } From 9a57993b67a1e8d903505b4f4db731207abe8bc0 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 03:50:23 +0200 Subject: [PATCH 10/14] refactor(frontend/table): read the failed-cell rule from one predicate --- .../ui/src/data/table/table-data.transforms.ts | 9 +++++++-- frontend/packages/ui/src/features/dashboard/run.tsx | 3 +-- .../ui/src/features/plots/use-summary-plot-data.ts | 11 ++++++----- frontend/packages/ui/src/features/table/table.tsx | 9 +++------ 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/frontend/packages/ui/src/data/table/table-data.transforms.ts b/frontend/packages/ui/src/data/table/table-data.transforms.ts index 99402481..e1af7b82 100644 --- a/frontend/packages/ui/src/data/table/table-data.transforms.ts +++ b/frontend/packages/ui/src/data/table/table-data.transforms.ts @@ -2,11 +2,16 @@ import { isHeavySummaryBlank } from '#src/constants' import type { Cell, Run, RunCells, RunId, Variable } from './table-data.types' +// A cell with something to render. A failed cell is out whatever its summary +// reads as: the error is the result, and every surface shows that instead. +export function hasValue(cell: Cell | undefined): cell is Cell { + return cell != null && cell.error == null && cell.summary.value != null +} + // A value the server is still holding back on the table's first pass, rather // than one that is genuinely absent. Shares its rule with the cache merge policy // through `isHeavySummaryBlank`, so the two cannot disagree on what is still to -// come. A cell that failed carries an error, and its blank is the final answer -// rather than a value on its way. +// come. function isDeferred(cell: Cell): boolean { return cell.error == null && isHeavySummaryBlank(cell.summary) } diff --git a/frontend/packages/ui/src/features/dashboard/run.tsx b/frontend/packages/ui/src/features/dashboard/run.tsx index 682b376b..2903560e 100644 --- a/frontend/packages/ui/src/features/dashboard/run.tsx +++ b/frontend/packages/ui/src/features/dashboard/run.tsx @@ -153,8 +153,7 @@ const Run = () => { {validRuns.map(([name, data]) => { const label = metadataVariables[name]?.title || name - // The error comes first, the same order the grid and the plots use: a - // cell that failed has nothing worth rendering from its summary. + // A cell that failed has nothing worth rendering from its summary. if (data.error) { return renderError({ name, label, error: data.error }) } diff --git a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts index 2644ce1b..a8fd8791 100644 --- a/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts +++ b/frontend/packages/ui/src/features/plots/use-summary-plot-data.ts @@ -9,7 +9,11 @@ import { type TableDataResult, type TableDataVariables, } from '#src/data/table/table-data.queries' -import { indexRunCells, runKey } from '#src/data/table/table-data.transforms' +import { + hasValue, + indexRunCells, + runKey, +} from '#src/data/table/table-data.transforms' import type { RunId } from '#src/data/table/table-data.types' import { useTableMeta } from '#src/data/table/use-table-meta' @@ -64,12 +68,9 @@ export function useSummaryPlotData({ for (const id of runIds) { const row = cells.get(runKey(id)) const points = variables.map((name) => row?.[name]) - // A failed cell is out whatever its value reads as, the same rule the - // grid, the aside and the plot context menu all apply. const allNumeric = points.every( (point) => - point != null && - point.error == null && + hasValue(point) && typeof point.summary.value === 'number' && point.summary.dtype === DTYPES.number ) diff --git a/frontend/packages/ui/src/features/table/table.tsx b/frontend/packages/ui/src/features/table/table.tsx index c6de8165..5f5ec1b2 100644 --- a/frontend/packages/ui/src/features/table/table.tsx +++ b/frontend/packages/ui/src/features/table/table.tsx @@ -15,7 +15,7 @@ import { Group, Stack, useMantineTheme } from '@mantine/core' import { DTYPES, VARIABLES } from '#src/constants' import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' -import { runKey } from '#src/data/table/table-data.transforms' +import { hasValue, runKey } from '#src/data/table/table-data.transforms' import { useTableMeta, useTableVariables } from '#src/data/table/use-table-meta' import { isArrayEqual, sorted } from '#src/utils/array' import { isEmpty } from '#src/utils/helpers' @@ -265,12 +265,9 @@ const Table = ({ grid, paginated = true }: TableProps) => { const rowData = identity && cellsByKey.get(runKey(identity)) // A row whose page has not loaded yet has no data at all, not merely no - // value: it has nothing to offer a plot either way. A failed cell arrives - // with a null value, so it is already out; checking the error keeps this in - // step with the grid and the aside. + // value: it has nothing to offer a plot either way. // TODO: Use extracted data type from the database - const cell = rowData?.[column] - if (col !== -1 && cell?.error == null && cell?.summary.value != null) { + if (col !== -1 && hasValue(rowData ? rowData[column] : undefined)) { const variable = tableColumns[col] const subtitle = `${variable.title}` From e2b88b68d6dbc98194d0588c7ee5066476a21e19 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 03:50:27 +0200 Subject: [PATCH 11/14] refactor(frontend/table): tighten the cell-factory lookup and its null branch --- .../packages/ui/src/features/table/cells.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/frontend/packages/ui/src/features/table/cells.ts b/frontend/packages/ui/src/features/table/cells.ts index 7097b8da..007c5057 100644 --- a/frontend/packages/ui/src/features/table/cells.ts +++ b/frontend/packages/ui/src/features/table/cells.ts @@ -12,7 +12,7 @@ import { } from '@glideapps/glide-data-grid' import { type SparklineCellType } from '@glideapps/glide-data-grid-cells' -import { DTYPES, isHeavySummaryBlank } from '#src/constants' +import { DTYPES, HEAVY_DTYPES } from '#src/constants' import { type CellError, type CellValue, @@ -272,7 +272,11 @@ export const loadingCell = ( } } -const gridCellFactory = { +type CellFactory = (value: CellValue, params: Partial) => GridCell + +// Typed so a lookup reads as possibly-missing: DAMNIT's dtypes are an open set +// (boolean and complex among them) and only these five have a renderer. +const gridCellFactory: Partial> = { [DTYPES.image]: imageCell, [DTYPES.string]: textCell, [DTYPES.number]: numberCell, @@ -291,18 +295,15 @@ export const getCell = ({ dtype, options, }: GetCellOptions): GridCell => { - // The grid asks for every visible cell on every redraw, so the populated case - // comes first and allocates nothing on its way through. A dtype with no - // renderer (e.g. a boolean cell) falls back to text rather than crashing - // every visible cell the grid asks `getContent` for. + // A dtype with no renderer falls back to text rather than throwing on every + // visible cell the grid asks `getContent` for. if (value != null) { return (gridCellFactory[dtype] ?? textCell)(value, options) } // A null heavy value is one @lightweight held back, so it draws the loading // skeleton until the deferred fetch fills it. A null scalar is a cell DAMNIT // has no value for and nothing is coming, so it draws as empty rather than - // loading forever. - return isHeavySummaryBlank({ value, dtype }) - ? loadingCell(value, options) - : textCell('') + // loading forever. This is `isHeavySummaryBlank` with the null already + // established, kept inline because the grid re-asks on every redraw. + return HEAVY_DTYPES.has(dtype) ? loadingCell(value, options) : textCell('') } From 35365e8e2e58b05f7519df023e8deb21fd5623cf Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 03:50:31 +0200 Subject: [PATCH 12/14] refactor(frontend/graphql): simplify the summary merge and its comments --- frontend/packages/ui/src/graphql/apollo.ts | 15 +++++----- .../packages/ui/src/graphql/type-policies.ts | 29 +++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/frontend/packages/ui/src/graphql/apollo.ts b/frontend/packages/ui/src/graphql/apollo.ts index 35aa9b33..3c4d6ad2 100644 --- a/frontend/packages/ui/src/graphql/apollo.ts +++ b/frontend/packages/ui/src/graphql/apollo.ts @@ -67,14 +67,15 @@ const splitLink = split( // the cap on its own. Past it every cache write re-reads the whole table instead // of returning the memoized result, which is seconds of main thread per // paginated page. The budget is shared across documents, and the summary plot -// asks for every run at once, so give it room. +// asks for every run at once, so give it room. This is sized for a few thousand +// runs, not for `ALL_RUNS_PAGE_SIZE` of them. // -// One knob, three caches: `resultCacheMaxSize` is the `max` for -// `executeSelectionSet` (the one reasoned about above), `executeSubSelectedArray` -// and `maybeBroadcastWatch`, whose own defaults are 10,000 and 5,000. Only the -// first is the binding constraint here, because it holds two entries per cell -// while the others hold roughly one per run, so raising all three is headroom -// the other two never reach rather than a budget worth splitting. +// The one knob sets the cap for three caches: `executeSelectionSet` (the one +// reasoned about above), `executeSubSelectedArray` and `maybeBroadcastWatch`, +// whose own defaults are 10,000 and 5,000. Apollo offers no way to raise only +// one, so the other two are raised along with it; the price is that their +// entries now roll over far later, and a saturated budget costs tens of MB of +// bookkeeping. Leaving a proposal resets all three (`registerAppListeners`). export const cache = new InMemoryCache({ typePolicies, resultCacheMaxSize: 200_000, diff --git a/frontend/packages/ui/src/graphql/type-policies.ts b/frontend/packages/ui/src/graphql/type-policies.ts index 5f78fb42..9370e839 100644 --- a/frontend/packages/ui/src/graphql/type-policies.ts +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -33,12 +33,11 @@ function mergeRefsByIdentity( return [...existing, ...additions] } -// A summary as the cache stores it. Both fields are optional because a write -// only carries what its document selected, not because the schema allows one -// without the other. +// A summary as the cache stores it. Every runs document selects the summary +// through the one `CELL_FIELDS` constant, so both fields always arrive. type StoredSummary = StoreObject & { - value?: unknown - dtype?: string + value: unknown + dtype: string } // Keep a value the lightweight pass is holding back. Only a heavy dtype is ever @@ -56,14 +55,10 @@ function mergeSummary( return incoming } - // A write that selected only `value` carries no dtype; the cached one still - // describes the cell, and merging rather than replacing is what keeps it. - const dtype = incoming.dtype ?? existing.dtype const heldBackBlank = - dtype != null && - dtype === existing.dtype && existing.value != null && - isHeavySummaryBlank({ value: incoming.value, dtype }) + incoming.dtype === existing.dtype && + isHeavySummaryBlank(incoming) return heldBackBlank ? existing : mergeObjects(existing, incoming) } @@ -86,10 +81,14 @@ export const typePolicies: TypePolicies = { CellSummary: { // The value guard lives here rather than on `Cell` because a merge function // only sees the field it merges, so this is the only level that can see - // `dtype` alongside the value. A failed cell needs no case of its own, and - // could not get one: `error` is cell-level and unreadable from here. The API - // drops the summary type of a failed cell instead, so it arrives as a null - // string rather than a heavy blank and clears the value it had. + // `dtype` alongside the value. `error` is cell-level and unreadable from + // here, which is why the API drops a failed cell's summary type instead + // (`DamnitRun._iter_cells`). + // + // Declaring any merge here costs one entry in Apollo's `storageTrie` per + // cell, held by a strong Map that `gc`, `evict` and `resetResultCache` all + // leave alone. It is the price of normalizing `Cell`: two documents write + // the same field with different completeness, so something has to arbitrate. merge: mergeSummary, }, Query: { From f09c28dc75397955f96f1ac38d7bef1b86036369 Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 03:50:34 +0200 Subject: [PATCH 13/14] refactor(api/graphql): validate the database handle before reading the record --- api/src/damnit_api/runs/types.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/api/src/damnit_api/runs/types.py b/api/src/damnit_api/runs/types.py index 0b6352b1..b1d16a6c 100644 --- a/api/src/damnit_api/runs/types.py +++ b/api/src/damnit_api/runs/types.py @@ -138,6 +138,7 @@ def cells(self, names: list[str] | None = None) -> list[Cell]: @classmethod def _iter_cells(cls, record, *, database, proposal, run): + prefix = f"{database}:{proposal}:{run}:" for name, entry in record.items(): if entry is None: continue @@ -154,7 +155,7 @@ def _iter_cells(cls, record, *, database, proposal, run): # whatever the cell held before it failed. dtype = DamnitType.STRING yield Cell( - id=strawberry.ID(f"{database}:{proposal}:{run}:{name}"), + id=strawberry.ID(prefix + name), name=name, error=error, summary=CellSummary(value=Any(value), dtype=dtype), @@ -162,13 +163,6 @@ def _iter_cells(cls, record, *, database, proposal, run): @classmethod def from_db(cls, record, *, database): - # Both callers key their rows on (proposal, run), so a record without - # them is a bug upstream. Fail here rather than mint a `"None"` - # proposal that quietly becomes a cache key on the client. - proposal = _unwrap(record["proposal"]) - if proposal is None: - msg = "Run record has no proposal." - raise ValueError(msg) database = str(database) # Cell ids join their parts with ":", so a part carrying one of its own # would let two different cells share an id and collide in the client's @@ -177,6 +171,14 @@ def from_db(cls, record, *, database): if ":" in database: msg = f"Database handle may not contain ':': {database!r}" raise ValueError(msg) + + # Both callers key their rows on (proposal, run), so a record without + # them is a bug upstream. Fail here rather than mint a `"None"` + # proposal that quietly becomes a cache key on the client. + proposal = _unwrap(record["proposal"]) + if proposal is None: + msg = "Run record has no proposal." + raise ValueError(msg) proposal = str(proposal) run = int(_unwrap(record["run"])) return cls( From de5d0bd32b8f3f0e84c75365f942a01daa89930f Mon Sep 17 00:00:00 2001 From: Cammille Carinan Date: Tue, 28 Jul 2026 05:01:00 +0200 Subject: [PATCH 14/14] refactor(frontend/tests): give the cell helpers an options object --- .../data/table/table-data.transforms.test.ts | 44 ++++++++------ .../ui/tests/graphql/type-policies.test.ts | 60 ++++++++++++------- 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts index 45fdb286..54989b93 100644 --- a/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts +++ b/frontend/packages/ui/tests/data/table/table-data.transforms.test.ts @@ -14,15 +14,22 @@ import type { // These transforms key cells by name and never read `id`, so it only has to be // present, not realistic. -function cell( - name: string, - value: CellValue, +function cell({ + name, + value, dtype = 'number', - error: CellError | null = null -): Cell { + error = null, +}: CellOptions): Cell { return { id: name, name, error, summary: { value, dtype } } } +type CellOptions = { + name: string + value: CellValue + dtype?: string + error?: CellError | null +} + function run(proposal: string, number: number, cells: Cell[]): Run { return { database: proposal, proposal, run: number, cells } } @@ -30,8 +37,8 @@ function run(proposal: string, number: number, cells: Cell[]): Run { describe('indexRunCells', () => { test('keys each run by its (proposal, run) identity', () => { const cells = indexRunCells([ - run('900405', 5, [cell('energy', 1.2)]), - run('900405', 9, [cell('energy', 3.4)]), + run('900405', 5, [cell({ name: 'energy', value: 1.2 })]), + run('900405', 9, [cell({ name: 'energy', value: 3.4 })]), ]) expect([...cells.keys()]).toEqual(['900405:5', '900405:9']) @@ -47,8 +54,8 @@ describe('indexRunCells', () => { // The same run number in two proposals is two rows, not one, which is the // whole reason a run is keyed by the pair. const cells = indexRunCells([ - run('900405', 1, [cell('energy', 1.2)]), - run('900485', 1, [cell('energy', 9.9)]), + run('900405', 1, [cell({ name: 'energy', value: 1.2 })]), + run('900485', 1, [cell({ name: 'energy', value: 9.9 })]), ]) expect(cells.get('900405:1')?.energy.summary.value).toBe(1.2) @@ -58,7 +65,7 @@ describe('indexRunCells', () => { test('stores each cell by its variable name', () => { const error = { cls: 'ValueError', message: 'boom' } const cells = indexRunCells([ - run('900405', 1, [cell('x', 2, 'number', error)]), + run('900405', 1, [cell({ name: 'x', value: 2, error })]), ]) expect(cells.get('900405:1')?.x).toEqual({ id: 'x', @@ -68,7 +75,7 @@ describe('indexRunCells', () => { }) }) test('reuses a run’s cell map while the run object is unchanged', () => { - const runA = run('900405', 1, [cell('energy', 1.2)]) + const runA = run('900405', 1, [cell({ name: 'energy', value: 1.2 })]) // A later push hands back a new array but the same unchanged run object, so // its already-built cell map comes back rather than being rebuilt. @@ -79,12 +86,12 @@ describe('indexRunCells', () => { }) test('rebuilds only the run whose object changed', () => { - const runA = run('900405', 1, [cell('energy', 1.2)]) - const runB = run('900405', 2, [cell('energy', 3.4)]) + const runA = run('900405', 1, [cell({ name: 'energy', value: 1.2 })]) + const runB = run('900405', 2, [cell({ name: 'energy', value: 3.4 })]) const first = indexRunCells([runA, runB]) // runB is replaced with a fresh object (its value changed); runA is untouched. - const runBNext = run('900405', 2, [cell('energy', 9.9)]) + const runBNext = run('900405', 2, [cell({ name: 'energy', value: 9.9 })]) const second = indexRunCells([runA, runBNext]) expect(second.get('900405:1')).toBe(first.get('900405:1')) @@ -100,12 +107,15 @@ test('runKey pairs proposal and run into a lookup key', () => { // A heavy value the @lightweight directive held back: the server sends the cell // with its summary value nulled out. const blanked = (name: string, error: CellError | null = null) => - cell(name, null, 'array', error) + cell({ name, value: null, dtype: 'array', error }) describe('heavyCellNames', () => { test('names the blanked cells worth a second fetch', () => { const names = heavyCellNames([ - run('900405', 1, [cell('energy', 1.2), blanked('spectrum')]), + run('900405', 1, [ + cell({ name: 'energy', value: 1.2 }), + blanked('spectrum'), + ]), ]) expect(names).toEqual(['spectrum']) @@ -135,7 +145,7 @@ describe('heavyCellNames', () => { test('leaves out a genuinely-empty scalar cell', () => { // A null scalar (no error, non-heavy dtype) is a deleted-for-this-run // value, not a held-back heavy blank, so re-fetching it would loop forever. - const empty = cell('note', null, 'string') + const empty = cell({ name: 'note', value: null, dtype: 'string' }) const names = heavyCellNames([run('900405', 1, [empty])]) expect(names).toEqual([]) diff --git a/frontend/packages/ui/tests/graphql/type-policies.test.ts b/frontend/packages/ui/tests/graphql/type-policies.test.ts index 485dafad..08d0bf8d 100644 --- a/frontend/packages/ui/tests/graphql/type-policies.test.ts +++ b/frontend/packages/ui/tests/graphql/type-policies.test.ts @@ -26,15 +26,22 @@ type CellInput = { error: CellError | null } -function cell( - name: string, - value: unknown, +function cell({ + name, + value, dtype = 'number', - error: CellError | null = null -): CellInput { + error = null, +}: CellOptions): CellInput { return { name, value, dtype, error } } +type CellOptions = { + name: string + value: unknown + dtype?: string + error?: CellError | null +} + function run(proposal: string, number: number, cells: CellInput[]) { return { __typename: 'DamnitRun', @@ -83,18 +90,18 @@ const dtypeOf = ( name: string ) => summaryOf(runs, identity, name)?.dtype -const blanked = cell('spectrum', null, 'array') -const filled = cell('spectrum', [1, 2, 3], 'array') +const blanked = cell({ name: 'spectrum', value: null, dtype: 'array' }) +const filled = cell({ name: 'spectrum', value: [1, 2, 3], dtype: 'array' }) test('the lightweight, deferred, and pushed cell sets share one run', () => { // The lightweight pass lands the run with its heavy value blanked. - writeRuns([run(PROPOSAL, 1, [cell('energy', 10), blanked])]) + writeRuns([run(PROPOSAL, 1, [cell({ name: 'energy', value: 10 }), blanked])]) expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() // The deferred pass fills only the heavy value, keyed onto the same run. The // cells list unions by identity, so `energy` survives even though this pass // did not carry it. - writeRuns([run(PROPOSAL, 1, [cell('run', 1), filled])]) + writeRuns([run(PROPOSAL, 1, [cell({ name: 'run', value: 1 }), filled])]) expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) expect(valueOf(readRuns(), 1, 'energy')).toBe(10) }) @@ -116,20 +123,22 @@ test('a retyped variable clears the value that no longer describes it', () => { // next lightweight pass blanks it under the new dtype. Keeping the array here // would pair it with a dtype that cannot draw it, and the cell would never be // fetched again because it would still look like it had a value. - writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'image')])]) + writeRuns([ + run(PROPOSAL, 1, [cell({ name: 'spectrum', value: null, dtype: 'image' })]), + ]) expect(valueOf(readRuns(), 1, 'spectrum')).toBeNull() expect(dtypeOf(readRuns(), 1, 'spectrum')).toBe('image') }) test('a null scalar clears the value it had rather than keeping it', () => { - writeRuns([run(PROPOSAL, 1, [cell('energy', 10)])]) + writeRuns([run(PROPOSAL, 1, [cell({ name: 'energy', value: 10 })])]) expect(valueOf(readRuns(), 1, 'energy')).toBe(10) // Unlike a held-back heavy blank, a null scalar is DAMNIT clearing the value // for this run, so the merge must let it through instead of keeping the stale // number. - writeRuns([run(PROPOSAL, 1, [cell('energy', null)])]) + writeRuns([run(PROPOSAL, 1, [cell({ name: 'energy', value: null })])]) expect(valueOf(readRuns(), 1, 'energy')).toBeNull() }) @@ -147,7 +156,11 @@ test('a cell that fails after computing clears the value it had', () => { // DAMNIT stores a failed variable with a null value and no summary type, so // it comes back as a null string, not a held-back heavy blank: the error // lands and the array it had goes with it. - writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'string', error)])]) + writeRuns([ + run(PROPOSAL, 1, [ + cell({ name: 'spectrum', value: null, dtype: 'string', error }), + ]), + ]) const spectrum = readRuns()[0].cells.find( (entry) => entry.name === 'spectrum' @@ -167,7 +180,12 @@ test('an error alone cannot clear a value under a heavy dtype', () => { // goes, this is the value that gets pinned behind the error. writeRuns([ run(PROPOSAL, 1, [ - cell('spectrum', null, 'array', { cls: 'ValueError', message: 'boom' }), + cell({ + name: 'spectrum', + value: null, + dtype: 'array', + error: { cls: 'ValueError', message: 'boom' }, + }), ]), ]) @@ -175,10 +193,10 @@ test('an error alone cannot clear a value under a heavy dtype', () => { }) test('paginated runs accumulate into one list, deduped by identity', () => { - writeRuns([run(PROPOSAL, 1, [cell('energy', 1)])]) + writeRuns([run(PROPOSAL, 1, [cell({ name: 'energy', value: 1 })])]) writeRuns([ - run(PROPOSAL, 1, [cell('energy', 1)]), - run(PROPOSAL, 2, [cell('energy', 2)]), + run(PROPOSAL, 1, [cell({ name: 'energy', value: 1 })]), + run(PROPOSAL, 2, [cell({ name: 'energy', value: 2 })]), ]) const runs = readRuns() @@ -187,8 +205,8 @@ test('paginated runs accumulate into one list, deduped by identity', () => { test('runs that share a number across proposals stay separate', () => { writeRuns([ - run('900405', 1, [cell('energy', 1.2)]), - run('900485', 1, [cell('energy', 9.9)]), + run('900405', 1, [cell({ name: 'energy', value: 1.2 })]), + run('900485', 1, [cell({ name: 'energy', value: 9.9 })]), ]) const runs = readRuns() @@ -200,8 +218,8 @@ test('cells sharing a name across runs are separate normalized entities', () => // The id folds in the run's identity, so one variable's cell in two runs // never collapses onto a single cache object. writeRuns([ - run(PROPOSAL, 1, [cell('energy', 1.2)]), - run(PROPOSAL, 2, [cell('energy', 9.9)]), + run(PROPOSAL, 1, [cell({ name: 'energy', value: 1.2 })]), + run(PROPOSAL, 2, [cell({ name: 'energy', value: 9.9 })]), ]) expect(valueOf(readRuns(), 1, 'energy')).toBe(1.2)