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..b1d16a6c 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,8 @@ 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): + prefix = f"{database}:{proposal}:{run}:" for name, entry in record.items(): if entry is None: continue @@ -135,10 +147,31 @@ 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(prefix + name), + name=name, + error=error, + summary=CellSummary(value=Any(value), dtype=dtype), + ) @classmethod def from_db(cls, record, *, database): + 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) + # 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. @@ -146,11 +179,15 @@ def from_db(cls, record, *, database): if proposal is None: msg = "Run record has no proposal." 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"} 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..f4141c6d 100644 --- a/frontend/packages/shared/src/mocks/index.ts +++ b/frontend/packages/shared/src/mocks/index.ts @@ -1,7 +1,8 @@ export { REST_API_PREFIXES, - shapeCell, + cellId, 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..e9b957bc 100644 --- a/frontend/packages/shared/src/mocks/shape.ts +++ b/frontend/packages/shared/src/mocks/shape.ts @@ -25,23 +25,55 @@ 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 `Cell`/`CellError` shape stays identical on both paths. `error` is always +// 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. -export function shapeCell( +function shapeCell( name: string, cell: RunData['variables'][string], - { lightweight = false }: { lightweight?: boolean } = {} + { database, proposal, run, lightweight = false }: ShapeCellOptions ) { return { __typename: 'Cell', + id: cellId({ 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. +export function shapeRun( + variables: RunData['variables'], + options: ShapeRunOptions +) { + const { database, proposal, run, names } = options + return { + __typename: 'DamnitRun', + database, + proposal, + run, + cells: Object.entries(variables) + .filter(([name]) => names == null || names.includes(name)) + .map(([name, cell]) => shapeCell(name, cell, options)), } } @@ -50,23 +82,44 @@ 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, + // 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, + }) + ), } } +type CellIdParts = { + database: string + proposal: string + run: number + name: string +} + +type ShapeCellOptions = { + database: string + proposal: string + run: number + 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 +} + type ShapeTableDataOptions = { proposal: string names?: string[] | null 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/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.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..e1af7b82 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,19 @@ -import { isHeavyBlank } from '#src/constants' +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 `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. function isDeferred(cell: Cell): boolean { - return isHeavyBlank({ - value: cell.value, - error: cell.error, - dtype: cell.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/data/table/table-data.types.ts b/frontend/packages/ui/src/data/table/table-data.types.ts index e9bad4f0..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,16 +1,25 @@ -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 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/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) { diff --git a/frontend/packages/ui/src/features/dashboard/run.tsx b/frontend/packages/ui/src/features/dashboard/run.tsx index f529e07b..2903560e 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) ) @@ -153,17 +153,16 @@ 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. + // A cell that failed has nothing worth rendering from its summary. 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..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' @@ -66,13 +70,13 @@ export function useSummaryPlotData({ const points = variables.map((name) => row?.[name]) const allNumeric = points.every( (point) => - point != null && - typeof point.value === 'number' && - point.dtype === DTYPES.number + hasValue(point) && + 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/cells.ts b/frontend/packages/ui/src/features/table/cells.ts index bb0e6d66..007c5057 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, @@ -270,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, @@ -280,7 +286,7 @@ const gridCellFactory = { type GetCellOptions = { value: CellValue - dtype: keyof typeof gridCellFactory + dtype: string options: Partial } @@ -289,14 +295,15 @@ export const getCell = ({ dtype, options, }: GetCellOptions): GridCell => { + // 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. - if (value == null) { - return HEAVY_DTYPES.has(String(dtype)) - ? loadingCell(value, options) - : textCell('') - } - return gridCellFactory[dtype](value, options) + // 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('') } diff --git a/frontend/packages/ui/src/features/table/table.tsx b/frontend/packages/ui/src/features/table/table.tsx index 257dfd3d..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' @@ -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 && hasValue(rowData ? rowData[column] : undefined)) { const variable = tableColumns[col] const subtitle = `${variable.title}` diff --git a/frontend/packages/ui/src/graphql/apollo.ts b/frontend/packages/ui/src/graphql/apollo.ts index b293a9f7..3c4d6ad2 100644 --- a/frontend/packages/ui/src/graphql/apollo.ts +++ b/frontend/packages/ui/src/graphql/apollo.ts @@ -61,7 +61,25 @@ 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. This is sized for a few thousand +// runs, not for `ALL_RUNS_PAGE_SIZE` of them. +// +// 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, +}) export const client = new ApolloClient({ cache, diff --git a/frontend/packages/ui/src/graphql/type-policies.ts b/frontend/packages/ui/src/graphql/type-policies.ts index 50157361..9370e839 100644 --- a/frontend/packages/ui/src/graphql/type-policies.ts +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -5,61 +5,7 @@ import type { TypePolicies, } from '@apollo/client' -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()] -} +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 @@ -87,16 +33,64 @@ function mergeRefsByIdentity( return [...existing, ...additions] } +// 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 +} + +// 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 + } + + const heldBackBlank = + existing.value != null && + incoming.dtype === existing.dtype && + isHeavySummaryBlank(incoming) + return heldBackBlank ? existing : mergeObjects(existing, incoming) +} + export const typePolicies: TypePolicies = { DamnitRun: { keyFields: ['database', 'proposal', 'run'], fields: { + // 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: mergeCellsByName, + merge: mergeRefsByIdentity, }, }, }, + Cell: { + keyFields: ['id'], + }, + 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. `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: { 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..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 @@ -12,32 +12,41 @@ import type { Run, } from '#src/data/table/table-data.types' -function cell( - name: string, - value: CellValue, +// These transforms key cells by name and never read `id`, so it only has to be +// present, not realistic. +function cell({ + name, + value, dtype = 'number', - error: CellError | null = null -): Cell { - return { name, value, dtype, error } + error = null, +}: CellOptions): Cell { + return { id: name, name, error, summary: { value, dtype } } } -function run(proposal: string, number: number, cells: Run['cells']): Run { +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 } } 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']) expect(cells.get('900405:5')?.energy).toEqual({ + id: 'energy', name: 'energy', - value: 1.2, - dtype: 'number', error: null, + summary: { value: 1.2, dtype: 'number' }, }) }) @@ -45,28 +54,28 @@ 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.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', () => { 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', 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', () => { - 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. @@ -77,17 +86,17 @@ 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')) 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,14 +105,17 @@ 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) + 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']) @@ -129,4 +141,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({ name: 'note', value: null, dtype: '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..96cfb684 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, @@ -21,12 +21,18 @@ 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 an empty cell for a missing scalar, not a loading one', () => { - expect( - getCell({ value: undefined, dtype: DTYPES.number, options: {} }).kind - ).toBe(GridCellKind.Text) + const cell = getCell({ value: null, dtype: DTYPES.number, options: {} }) + + expect(cell.kind).toBe(GridCellKind.Text) + expect((cell as TextCell).displayData).toBe('') }) test('picks the cell type from the dtype when a value is present', () => { @@ -37,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', () => { @@ -108,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('') }) }) 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..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,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 { serverCell } from '#tests/support/cells' const PROPOSAL = '900405' const PAGE_SIZE = 10 @@ -37,13 +38,14 @@ const runFor = (run: number) => ({ proposal: PROPOSAL, run, cells: [ - { - __typename: 'Cell', + serverCell({ + database: PROPOSAL, + proposal: PROPOSAL, + run, name: 'spectrum', value: null, dtype: 'array', - error: null, - }, + }), ], }) @@ -171,13 +173,14 @@ test('does not flash a run filled by a cache write', async () => { proposal: PROPOSAL, run: 1, cells: [ - { - __typename: 'Cell', + serverCell({ + database: PROPOSAL, + proposal: PROPOSAL, + run: 1, name: 'spectrum', value: [1, 2, 3], dtype: 'array', - error: null, - }, + }), ], }, ], @@ -186,7 +189,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..08d0bf8d 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 { serverCell } from '#tests/support/cells' const PROPOSAL = '900405' @@ -15,30 +16,41 @@ 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 before `run` stamps it with the identity it is keyed by. +type CellInput = { name: string value: unknown dtype: string - error: { cls: string; message: string } | null + error: CellError | null } -function cell( - name: string, - value: unknown, +function cell({ + name, + value, dtype = 'number', - error: Cell['error'] = null -): Cell { - return { __typename: 'Cell', name, value, dtype, error } as Cell + 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: Cell[]) { +function run(proposal: string, number: number, cells: CellInput[]) { return { __typename: 'DamnitRun', database: PROPOSAL, proposal, run: number, - cells, + cells: cells.map((entry) => + serverCell({ database: PROPOSAL, proposal, run: number, ...entry }) + ), } } @@ -57,25 +69,39 @@ 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)?.value + ?.cells.find((entry) => entry.name === name)?.summary -const blanked = cell('spectrum', null, 'array') -const filled = cell('spectrum', [1, 2, 3], 'array') +const valueOf = ( + runs: TableDataResult['runs'], + identity: number, + name: string +) => summaryOf(runs, identity, name)?.value + +const dtypeOf = ( + runs: TableDataResult['runs'], + identity: number, + name: string +) => summaryOf(runs, identity, name)?.dtype + +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. - writeRuns([run(PROPOSAL, 1, [cell('run', 1), filled])]) + // 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({ name: 'run', value: 1 }), filled])]) expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) expect(valueOf(readRuns(), 1, 'energy')).toBe(10) }) @@ -84,12 +110,38 @@ 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({ 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({ 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({ name: 'energy', value: 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,23 +149,54 @@ 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 clears the value it had', () => { const error = { cls: 'ValueError', message: 'boom' } writeRuns([run(PROPOSAL, 1, [filled])]) - 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({ name: 'spectrum', value: null, dtype: 'string', 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).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({ + name: 'spectrum', + value: null, + dtype: 'array', + error: { cls: 'ValueError', message: 'boom' }, + }), + ]), + ]) + + expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) }) 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() @@ -122,11 +205,23 @@ 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() 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({ name: 'energy', value: 1.2 })]), + run(PROPOSAL, 2, [cell({ name: 'energy', value: 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..0b79a28f 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 { 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, @@ -27,22 +28,21 @@ 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', - name: 'energy', - value: 10, - dtype: 'number', - error: null, - }, + serverCell({ database, proposal, run: 1, name: 'energy', value: 10 }), ], }, ], @@ -110,13 +110,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..5c400030 --- /dev/null +++ b/frontend/packages/ui/tests/support/cells.ts @@ -0,0 +1,35 @@ +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 + value: unknown + dtype?: string + error?: CellError | null +} + +type CellError = { cls: string; message: string }