diff --git a/api/README.md b/api/README.md index ab52b302..8a717286 100644 --- a/api/README.md +++ b/api/README.md @@ -43,13 +43,23 @@ podman compose up ```gql query TableMetadataQuery { - metadata(database: {proposal: ""}) + metadata(database: {proposal: ""}) { + runs { + proposal + run + } + variables + tags + timestamp + } } ``` -This returns a JSON snapshot for the proposal: +This returns a `TableMeta` snapshot for the proposal: -- `runs` - sorted list of run numbers in the proposal +- `runs` - server-ordered list of `{proposal, run}` pairs. Run numbers repeat + across the proposals sharing one database, so a run is only identified by + the pair - `variables` - map of variable name to its title and tags (includes the known variables listed below alongside any user-defined ones) - `tags` - map of tag name to its id and the variables it groups @@ -63,6 +73,9 @@ of their cells: ```gql query TableDataQuery($per_page: Int = 10) { runs(database: {proposal: "2956"}, per_page: $per_page) { + database + proposal + run cells { name value @@ -72,8 +85,12 @@ query TableDataQuery($per_page: Int = 10) { } ``` -Each run is returned as a flat list of `Cell` entries (`name`, -`value`, `dtype`). One can pass a list of `names` to select variables: +Every run carries the identity trio `database`, `proposal` and `run`. Select +all three in every document: it is what the client normalizes each run by, and +what keeps two proposals' run 5 apart. + +The cells are a flat list of `Cell` entries (`name`, `value`, `dtype`). One can +pass a list of `names` to select variables: ```gql query TableDataQuery($per_page: Int = 10) { @@ -98,17 +115,43 @@ ones from the proposal's context file: Pagination is controlled with `page` (1-indexed, defaults to `1`) and `per_page` (defaults to `10`). -### 3. Subscribe to latest data +### 3. Subscribe to run updates ```gql -subscription LatestDataSubscription { - latest_data(database: {proposal: ""}, timestamp: ) +subscription RunUpdatesSubscription { + run_updates(database: {proposal: ""}, since: ) { + runs { + database + proposal + run + cells { + name + value + dtype + } + } + metadata { + runs { + proposal + run + } + variables + tags + timestamp + } + timestamp + } } ``` -This returns the following: +Each push is a `RunUpdates`: -- list of (new) runs -- updated metadata +- `runs` - the runs whose cells changed since `since` +- `metadata` - the full `TableMeta`, but only on a push where it materially + changed. It is `null` otherwise, so a tag edit or a new variable arrives + even on a tick that brings no runs +- `timestamp` - the cursor to send as the next `since` -Note that the `timestamp` is in milliseconds since Unix epoch. +Note that both `since` and `timestamp` are in milliseconds since the Unix +epoch. Passing `since: 0` is the cold start: it delivers the next tick's +changed runs, not the proposal's history. diff --git a/api/src/damnit_api/graphql/metadata.py b/api/src/damnit_api/graphql/metadata.py index 0cad4039..cc849a4d 100644 --- a/api/src/damnit_api/graphql/metadata.py +++ b/api/src/damnit_api/graphql/metadata.py @@ -1,4 +1,6 @@ import asyncio +import hashlib +import json from async_lru import alru_cache @@ -11,15 +13,16 @@ async def fetch_metadata(proposal=db.DEFAULT_PROPOSAL): """Fetch the per-proposal metadata snapshot from SQLite. - Returns a dict with `runs`, `variables`, `tags`, and `timestamp`. Result - is TTL-cached; the `latest_data` subscription invalidates this cache when - it observes new data so subsequent reads stay fresh. + Returns a dict with `runs`, `variables`, `tags`, and `timestamp`. `runs` + is a server-ordered list of (proposal, run) pairs (active block first). + Result is TTL-cached; the `run_updates` subscription invalidates this + cache when it observes new data so subsequent reads stay fresh. """ tags, variables, variable_tags, runs, max_timestamp = await asyncio.gather( db.async_all_tags(proposal), db.async_variables(proposal), db.async_variable_tags(proposal), - db.async_column(proposal, table="run_info", name="run"), + db.async_run_identifiers(proposal), db.async_max(proposal, table="run_variables", column="timestamp"), ) @@ -39,9 +42,26 @@ async def fetch_metadata(proposal=db.DEFAULT_PROPOSAL): } tags = create_map([untagged, *tags.values()], key="name") - return { - "runs": sorted(runs or []), + snapshot = { + "runs": runs, "variables": variables, "tags": tags, "timestamp": max_timestamp or 0, } + snapshot["signature"] = _signature(snapshot) + return snapshot + + +def _signature(snapshot) -> str: + """Hash of everything a subscriber would be pushed. + + Computed here so it costs one pass per actual read rather than one per + subscription tick. `timestamp` is left out: it moves whenever any value + changes, which would make every tick look like a metadata change. + """ + payload = json.dumps( + {key: snapshot[key] for key in ("runs", "variables", "tags")}, + sort_keys=True, + default=str, + ) + return hashlib.sha256(payload.encode()).hexdigest() diff --git a/api/src/damnit_api/graphql/queries.py b/api/src/damnit_api/graphql/queries.py index 80c6878c..84cfaa32 100644 --- a/api/src/damnit_api/graphql/queries.py +++ b/api/src/damnit_api/graphql/queries.py @@ -1,5 +1,5 @@ import strawberry -from sqlalchemy import and_, func, select +from sqlalchemy import and_, func, select, tuple_ from strawberry.scalars import JSON from strawberry.types import Info from strawberry.types.nodes import SelectedField @@ -8,8 +8,13 @@ from ..auth.permissions import PROPOSAL_PERMISSIONS from ..metadata.services import _get_proposal_meta, _update_proposal_meta from ..runs.preview import get_preview_data -from ..runs.sqlite import async_table, get_session -from ..runs.types import KNOWN_DTYPES, DamnitRun +from ..runs.sqlite import ( + async_active_proposal, + async_table, + get_session, + order_by_active, +) +from ..runs.types import KNOWN_DTYPES, DamnitRun, TableMeta from .metadata import fetch_metadata from .utils import DatabaseInput, fetch_info @@ -82,27 +87,32 @@ async def fetch_cells(proposal, *, limit, offset, names=None): if table is None: return [] + active = await async_active_proposal(proposal) runs_subquery = ( select(table.c.proposal, table.c.run) .distinct() - .order_by(table.c.run) + .order_by(*order_by_active(table, active)) .limit(limit) .offset(offset) .subquery() ) + page_pairs = select(runs_subquery.c.proposal, runs_subquery.c.run) latest_timestamp_subquery = select( table.c.proposal, table.c.run, table.c.name, func.max(table.c.timestamp).label("latest_timestamp"), - ).where(table.c.run.in_(select(runs_subquery.c.run))) + ).where(tuple_(table.c.proposal, table.c.run).in_(page_pairs)) if names is not None: latest_timestamp_subquery = latest_timestamp_subquery.where( table.c.name.in_(names) ) + # Group by (proposal, run, name): grouping by run alone would let one + # run's max timestamp mix in a colliding run from another proposal, and + # SQLite's bare-column group-by would then pick an arbitrary proposal. latest_timestamp_subquery = latest_timestamp_subquery.group_by( - table.c.run, table.c.name + table.c.proposal, table.c.run, table.c.name ).subquery() # Outer-join from `runs_subquery` so a `names` filter that excludes every @@ -119,7 +129,10 @@ async def fetch_cells(proposal, *, limit, offset, names=None): .select_from(runs_subquery) .outerjoin( latest_timestamp_subquery, - runs_subquery.c.run == latest_timestamp_subquery.c.run, + and_( + runs_subquery.c.proposal == latest_timestamp_subquery.c.proposal, + runs_subquery.c.run == latest_timestamp_subquery.c.run, + ), ) .outerjoin( table, @@ -130,20 +143,22 @@ async def fetch_cells(proposal, *, limit, offset, names=None): table.c.timestamp == latest_timestamp_subquery.c.latest_timestamp, ), ) - .order_by(runs_subquery.c.run) + .order_by(*order_by_active(runs_subquery, active)) ) async with get_session(proposal) as session: result = await session.execute(query) - if not result: - raise ValueError # TODO: Better error handling - return group_by_run(result.mappings().all()) # type: ignore[assignment] def _selected_cell_names(info: Info) -> list[str] | None: """Union the `names` arguments across every `cells` sub-selection. - Returns None if any selection omits the argument (forces a full fetch). + + Returns one of three things: + - ``[]`` if no `cells` field is selected (an identity-only query): fetch no + cells and skip the run_info fetch, since there is nothing to serialize. + - ``None`` if a `cells` selection omits `names`: fetch every cell. + - the sorted union of the requested names otherwise. """ union = set() found = False @@ -156,7 +171,10 @@ def _selected_cell_names(info: Info) -> list[str] | None: if arg is None: return None union.update(arg) - return sorted(union) if found else None + if not found: + # No `cells` selected: caller wants run identities only. + return [] + return sorted(union) def _wants_run_info(names: list[str] | None) -> bool: @@ -199,15 +217,17 @@ async def runs( if not len(cells): return [] - if _wants_run_info(names): - info_rows = await fetch_info( - proposal, runs=[c["run"]["value"] for c in cells] - ) - else: - info_rows = [{} for _ in cells] + pairs = [(c["proposal"]["value"], c["run"]["value"]) for c in cells] + run_info = ( + await fetch_info(proposal, runs=pairs) if _wants_run_info(names) else {} + ) return [ - DamnitRun.from_db({**c, **i}) for c, i in zip(cells, info_rows, strict=True) + DamnitRun.from_db( + {**c, **run_info.get(pair, {})}, + database=database.proposal, + ) + for c, pair in zip(cells, pairs, strict=True) ] @strawberry.field(permission_classes=PROPOSAL_PERMISSIONS) @@ -215,7 +235,7 @@ async def metadata( self, info: Info, database: DatabaseInput, - ) -> JSON: # FIX: # pyright: ignore[reportInvalidTypeForm] + ) -> TableMeta: proposal = database.proposal if not proposal: msg = "Proposal number is required." @@ -225,10 +245,7 @@ async def metadata( await _ensure_damnit_path(info, proposal) snapshot = await fetch_metadata(proposal) - return { - **snapshot, - "timestamp": snapshot["timestamp"] * 1000, # ms for JS - } # pyright: ignore[reportReturnType] + return TableMeta.from_snapshot(snapshot) # Nullable, because a preview asks for many runs in one request, aliasing # this field once per run. A non-null field that raises propagates the null diff --git a/api/src/damnit_api/graphql/subscriptions.py b/api/src/damnit_api/graphql/subscriptions.py index 3e3de997..7722caae 100644 --- a/api/src/damnit_api/graphql/subscriptions.py +++ b/api/src/damnit_api/graphql/subscriptions.py @@ -3,12 +3,10 @@ import strawberry from async_lru import alru_cache -from strawberry.scalars import JSON from ..auth.permissions import PROPOSAL_PERMISSIONS from ..runs.sqlite import async_latest_rows, async_max, async_table -from ..runs.types import DamnitRun, Timestamp -from ..utils import create_map +from ..runs.types import DamnitRun, TableMeta, Timestamp from .metadata import fetch_metadata from .utils import DatabaseInput, LatestData, fetch_info @@ -18,6 +16,27 @@ # newer than what the previous tick already shipped. _last_seen_timestamp: dict[str, float] = {} +# Newest `run_info.added_at` seen per proposal. A run is written to `run_info` +# before its variables are extracted, so it can appear without moving any +# `run_variables` timestamp. This is what notices it on the next tick instead +# of leaving it until the metadata cache expires. +_last_run_added_at: dict[str, float] = {} + + +@strawberry.type +class RunUpdates: + runs: list[DamnitRun] + metadata: TableMeta | None + timestamp: Timestamp + + +async def _new_run_appeared(proposal) -> bool: + added_at = await async_max(proposal, table="run_info", column="added_at") or 0 + if _last_run_added_at.get(proposal) == added_at: + return False + _last_run_added_at[proposal] = added_at + return True + # Per-client cursor is deliberately omitted from the cache key so that # concurrent subscribers coalesce into a single DB read per tick. @@ -39,83 +58,113 @@ async def poll_proposal(proposal): by="timestamp", start_at=_last_seen_timestamp[proposal], ) - if not rows: - return None - - latest_data = LatestData.from_list(rows) - latest_runs = await fetch_info(proposal, runs=list(latest_data.runs.keys())) - latest_runs = create_map(latest_runs, key="run") - - fetch_metadata.cache_invalidate(proposal) + # Re-read the metadata only when something can have changed it: new values, + # or a run that has appeared but has no values yet. A tag edit or a retitled + # variable writes neither, and no cheap query can see one either (`variables` + # and `tags` carry no timestamp), so those ride the cache's own expiry. + if rows or await _new_run_appeared(proposal): + fetch_metadata.cache_invalidate(proposal) metadata = await fetch_metadata(proposal) runs = {} run_timestamps = {} - for run, variables in latest_data.runs.items(): - run_values = { - name: { - "value": data.value, - "summary_type": data.summary_type, - "attributes": data.attributes, - } - for name, data in variables.items() - } - run_values.setdefault("run", {"value": run}) - if run_info := latest_runs.get(run): - run_values.update(run_info) - - runs[run] = DamnitRun.resolve(run_values) - run_timestamps[run] = max(data.timestamp for data in variables.values()) + if rows: + latest_data = LatestData.from_list(rows) + + if latest_data.timestamp is None: + msg = "Latest data has no timestamp." + raise ValueError(msg) + + pairs = list(latest_data.runs.keys()) + info = await fetch_info(proposal, runs=pairs) + + for key, variables in latest_data.runs.items(): + run_proposal, run_number = key + run_values = { + name: { + "value": data.value, + "summary_type": data.summary_type, + "attributes": data.attributes, + } + for name, data in variables.items() + } + run_values.setdefault("proposal", {"value": run_proposal}) + run_values.setdefault("run", {"value": run_number}) - if not runs: - return None + if run_info := info.get(key): + run_values.update(run_info) - if latest_data.timestamp is None: - msg = "Latest data has no timestamp." - raise ValueError(msg) + runs[key] = DamnitRun.from_db(run_values, database=proposal) + run_timestamps[key] = max(data.timestamp for data in variables.values()) - _last_seen_timestamp[proposal] = latest_data.timestamp + _last_seen_timestamp[proposal] = latest_data.timestamp - metadata = { - "runs": sorted(set(metadata["runs"]) | set(runs.keys())), - "variables": metadata["variables"], - "timestamp": latest_data.timestamp * 1000, # ms for JS - } return { "runs": runs, "run_timestamps": run_timestamps, - "max_timestamp": max(run_timestamps.values()), + "max_timestamp": _last_seen_timestamp[proposal], # seconds + # The whole snapshot rides along so a subscriber that needs to push it + # builds `TableMeta` from this exact metadata, rather than re-fetching and + # racing a newer snapshot whose signature no longer matches. Whether this + # is news is the subscriber's call, not ours: this poll is shared by every + # subscriber of the proposal, so a verdict reached here would be delivered + # to whichever one happened to fill the cache window and silently withheld + # from the rest. "metadata": metadata, + "metadata_signature": metadata["signature"], } -def filter_for_client(snapshot, since): - if snapshot is None or not since or snapshot["max_timestamp"] <= since: +def filter_for_client(snapshot, since, metadata=None): + if snapshot is None: return None - runs = { - run: value - for run, value in snapshot["runs"].items() - if snapshot["run_timestamps"][run] > since - } - if not runs: + # A cursorless client gets this tick's changed rows, not a history replay: + # `poll_proposal` already bounds them by the server's high-water mark. Only + # dropping them would be wrong, since a proposal with no runs seeds `since` + # from a zero timestamp and would never advance past it. + runs = [ + run + for key, run in snapshot["runs"].items() + if snapshot["run_timestamps"][key] > (since or 0) + ] + if not runs and metadata is None: return None - return {"runs": runs, "metadata": snapshot["metadata"]} + return RunUpdates( + runs=runs, + metadata=metadata, + timestamp=snapshot["max_timestamp"], + ) @strawberry.type class Subscription: @strawberry.subscription(permission_classes=PROPOSAL_PERMISSIONS) - async def latest_data( + async def run_updates( self, database: DatabaseInput, - timestamp: Timestamp, - ) -> AsyncGenerator[JSON]: # FIX: # pyright: ignore[reportInvalidTypeForm] + since: Timestamp, + ) -> AsyncGenerator[RunUpdates]: + # Which metadata this client has seen, held here rather than per + # proposal: the poll is shared, the delivery is not. The first tick + # always pushes, which also closes the gap between the client's opening + # metadata query and its subscribe. + last_signature = None + while True: await asyncio.sleep(POLLING_INTERVAL) snapshot = await poll_proposal(proposal=database.proposal) - result = filter_for_client(snapshot, timestamp) + + metadata = None + if ( + snapshot is not None + and snapshot["metadata_signature"] != last_signature + ): + last_signature = snapshot["metadata_signature"] + metadata = TableMeta.from_snapshot(snapshot["metadata"]) + + result = filter_for_client(snapshot, since, metadata) if result is not None: - yield result # FIX: # pyright: ignore[reportReturnType] + yield result diff --git a/api/src/damnit_api/graphql/utils.py b/api/src/damnit_api/graphql/utils.py index aa390941..455999ca 100644 --- a/api/src/damnit_api/graphql/utils.py +++ b/api/src/damnit_api/graphql/utils.py @@ -3,7 +3,7 @@ from typing import Any import strawberry -from sqlalchemy import or_, select +from sqlalchemy import select, tuple_ from ..runs.sqlite import async_table, get_session from ..shared.const import DEFAULT_PROPOSAL @@ -35,8 +35,9 @@ def __init__(self): def add(self, data): timestamp = data["timestamp"] - # Bookkeep by runs - run = self.runs[data["run"]] + # Bookkeep by (proposal, run): run numbers collide across proposals in + # one file, so keying by run alone would merge two runs into one. + run = self.runs[data["proposal"], data["run"]] if run[data["name"]].timestamp < timestamp: run[data["name"]] = Data( value=data["value"], @@ -65,15 +66,20 @@ def from_list(cls, sequence): async def fetch_info(proposal, *, runs): + """Fetch `run_info` rows for the given (proposal, run) pairs. + + Returns a mapping keyed by (proposal, run) so callers align rows even when + run numbers collide across proposals in one file. + """ table = await async_table(proposal, name="run_info") if table is None: - return [] - conditions = [table.c.run == run for run in runs] - query = select(table).where(or_(*conditions)).order_by(table.c.run) + return {} + # One `(proposal, run) IN (...)` predicate rather than an OR of per-pair + # ANDs: the unpaginated table asks for up to ALL_RUNS_PAGE_SIZE pairs, and a + # 10000-branch OR builds thousands of expressions SQLite runs as separate + # index probes. Mirrors the tuple IN already used in `fetch_cells`. + query = select(table).where(tuple_(table.c.proposal, table.c.run).in_(runs)) async with get_session(proposal) as session: result = await session.execute(query) - if not result: - raise ValueError # TODO: Better error handling - - return result.mappings().all() + return {(row["proposal"], row["run"]): row for row in result.mappings().all()} diff --git a/api/src/damnit_api/metadata/services.py b/api/src/damnit_api/metadata/services.py index 525c5d7a..de58e68f 100644 --- a/api/src/damnit_api/metadata/services.py +++ b/api/src/damnit_api/metadata/services.py @@ -43,22 +43,10 @@ def _local_proposal_meta(proposal_number: ProposalNumber) -> ProposalMeta: async def _local_proposal_number() -> int | None: - from sqlalchemy import select - - from ..runs.sqlite import async_table, get_session + from ..runs.sqlite import async_active_proposal from ..shared.const import DEFAULT_PROPOSAL - table = await async_table(DEFAULT_PROPOSAL, name="metameta") - if table is None: - return None - - async with get_session(DEFAULT_PROPOSAL) as session: - result = await session.execute( - select(table.c.value).where(table.c.key == "proposal") - ) - value = result.scalar() - - return int(value) if value else None + return await async_active_proposal(DEFAULT_PROPOSAL) async def _fetch_proposal_meta( diff --git a/api/src/damnit_api/runs/sqlite/__init__.py b/api/src/damnit_api/runs/sqlite/__init__.py index 48729c29..47770ff5 100644 --- a/api/src/damnit_api/runs/sqlite/__init__.py +++ b/api/src/damnit_api/runs/sqlite/__init__.py @@ -1,12 +1,14 @@ from ...shared.const import DEFAULT_PROPOSAL from .repository import ( + async_active_proposal, async_all_tags, - async_column, async_latest_rows, async_max, + async_run_identifiers, async_table, async_variable_tags, async_variables, + order_by_active, ) from .session import ( DAMNIT_PATH, @@ -20,14 +22,16 @@ "DAMNIT_PATH", "DEFAULT_PROPOSAL", "DatabaseSessionManager", + "async_active_proposal", "async_all_tags", - "async_column", "async_latest_rows", "async_max", + "async_run_identifiers", "async_table", "async_variable_tags", "async_variables", "get_connection", "get_damnit_path", "get_session", + "order_by_active", ] diff --git a/api/src/damnit_api/runs/sqlite/repository.py b/api/src/damnit_api/runs/sqlite/repository.py index 8116336a..dbb29fa7 100644 --- a/api/src/damnit_api/runs/sqlite/repository.py +++ b/api/src/damnit_api/runs/sqlite/repository.py @@ -5,6 +5,7 @@ from sqlalchemy import ( MetaData, Table, + case, desc, func, select, @@ -15,6 +16,20 @@ from .session import get_connection, get_session +def order_by_active(table, active): + """Order rows with the active proposal's block first, then (proposal, run). + + `active` is the addressing proposal from the file's `metameta`, which is + DAMNIT's own definition of the active proposal. Guest proposals follow, + ordered by (proposal, run). When there is no active proposal the ordering + falls back to plain (proposal, run). + """ + columns = [table.c.proposal, table.c.run] + if active is None: + return columns + return [case((table.c.proposal == active, 0), else_=1), *columns] + + @alru_cache(ttl=300) async def async_table(proposal, name: str = "runs") -> Table | None: async with get_connection(proposal) as conn: @@ -28,6 +43,54 @@ async def async_table(proposal, name: str = "runs") -> Table | None: return None +async def _read_active_proposal(proposal) -> int | None: + table = await async_table(proposal, name="metameta") + if table is None: + return None + selection = select(table.c.value).where(table.c.key == "proposal") + async with get_session(proposal) as session: + result = await session.execute(selection) + value = result.scalar() + if not value: + return None + try: + return int(value) + except ValueError: + # `metameta` is DAMNIT's file, not ours. A value that is not a number + # means the file names no active proposal, which orders rows plainly; + # raising here would instead fail every `runs` query on the file. + return None + + +@alru_cache(ttl=300) +async def async_active_proposal(proposal) -> int | None: + """Return the file's active proposal from `metameta`, or None if absent. + + Cached like `async_table`: the value is fixed for a file's lifetime, and + every `runs` query and every subscription tick asks for it to order rows. + """ + active = await _read_active_proposal(proposal) + if active is None: + # Don't cache misses; the table or the key may appear shortly. This + # drops the cache entry without touching the task still running it. + async_active_proposal.cache_invalidate(proposal) + return active + + +async def async_run_identifiers(proposal) -> list[tuple[int, int]]: + """Return every (proposal, run) pair in `run_info`, server-ordered.""" + table = await async_table(proposal, name="run_info") + if table is None: + return [] + active = await async_active_proposal(proposal) + selection = select(table.c.proposal, table.c.run).order_by( + *order_by_active(table, active) + ) + async with get_session(proposal) as session: + result = await session.execute(selection) + return [(row["proposal"], row["run"]) for row in result.mappings().all()] + + async def async_variables(proposal): variables = await async_table(proposal, name="variables") if variables is None: @@ -70,18 +133,6 @@ async def async_max(proposal, *, table: str, column: str): return result.scalar() -async def async_column(proposal, *, table: str, name: str): - table = await async_table(proposal, name=table) - if table is None: - return [] - selection = select(table.c.get(name)) - - async with get_session(proposal) as session: - result = await session.execute(selection) - - return result.scalars().all() - - async def async_all_tags(proposal): tags_table = await async_table(proposal, name="tags") if tags_table is None: diff --git a/api/src/damnit_api/runs/types.py b/api/src/damnit_api/runs/types.py index 774902b4..ba17581b 100644 --- a/api/src/damnit_api/runs/types.py +++ b/api/src/damnit_api/runs/types.py @@ -1,8 +1,9 @@ import json -from dataclasses import asdict, dataclass +from dataclasses import dataclass from typing import NewType import strawberry +from strawberry.scalars import JSON from .. import get_logger from ..shared.const import DamnitType @@ -36,11 +37,15 @@ class KnownVariable: Any = NewType("Any", object) Timestamp = NewType("Timestamp", float) +# The client works in JS milliseconds; the server works in seconds. Parse +# incoming cursors down to seconds and serialize outgoing ones back to +# milliseconds so both sides stay in their native unit. SCALAR_MAP = { Any: strawberry.scalar(name="Any"), Timestamp: strawberry.scalar( name="Timestamp", parse_value=lambda value: value / 1000, + serialize=lambda value: value * 1000, ), } @@ -88,8 +93,29 @@ class Cell: error: CellError | None = None +def _unwrap(entry): + """Return the bare value whether the record entry is wrapped as + ``{"value": ...}`` (from `fetch_cells`) or a raw scalar (from + `run_info`).""" + if isinstance(entry, dict): + return entry.get("value") + return entry + + +@strawberry.type +class RunId: + proposal: str + run: int + + @strawberry.type class DamnitRun: + # Identity trio: `database` is the addressing handle echoed back, while + # `proposal` and `run` are facts from the row. Runs collide across + # proposals within one file, so all three are needed to key a run. + database: str + proposal: str + run: int _cells: strawberry.Private[list[Cell]] @strawberry.field @@ -97,7 +123,7 @@ def cells(self, names: list[str] | None = None) -> list[Cell]: if names is None: return self._cells requested = set(names) - return [v for v in self._cells if v.name in requested] + return [c for c in self._cells if c.name in requested] @classmethod def _iter_cells(cls, record): @@ -112,26 +138,20 @@ def _iter_cells(cls, record): yield Cell(name=name, value=Any(value), dtype=dtype, error=error) @classmethod - def from_db(cls, record): - return cls(_cells=list(cls._iter_cells(record))) - - @classmethod - def resolve(cls, record): - out: dict[str, object | None] = { - name: None for name, entry in record.items() if entry is None - } - - for v in cls._iter_cells(record): - if v.value is None and v.error is None: - out[v.name] = None - continue - - resolved = {"value": v.value, "dtype": v.dtype.value} - if v.error is not None: - resolved["error"] = asdict(v.error) - - out[v.name] = resolved - return out + 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) + return cls( + database=str(database), + proposal=str(proposal), + run=int(_unwrap(record["run"])), + _cells=list(cls._iter_cells(record)), + ) @staticmethod def known_variables(): @@ -158,3 +178,23 @@ def get_dtype(name, entry): return dtype return DamnitType.STRING + + +@strawberry.type +class TableMeta: + runs: list[RunId] + variables: JSON + tags: JSON + timestamp: Timestamp + + @classmethod + def from_snapshot(cls, snapshot): + return cls( + runs=[ + RunId(proposal=str(proposal), run=int(run)) + for proposal, run in snapshot["runs"] + ], + variables=snapshot["variables"], + tags=snapshot["tags"], + timestamp=snapshot["timestamp"], + ) diff --git a/api/tests/conftest.py b/api/tests/conftest.py index 3f130b3d..43632b55 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -1,12 +1,18 @@ import pytest -from damnit_api.runs.sqlite import DatabaseSessionManager, async_table +from damnit_api.runs.sqlite import ( + DatabaseSessionManager, + async_active_proposal, + async_table, +) @pytest.fixture(autouse=True) def _clear_db_session_manager_registry(): DatabaseSessionManager.registry.clear() async_table.cache_clear() + async_active_proposal.cache_clear() yield DatabaseSessionManager.registry.clear() async_table.cache_clear() + async_active_proposal.cache_clear() diff --git a/api/tests/graphql/conftest.py b/api/tests/graphql/conftest.py index 88334d56..84e98494 100644 --- a/api/tests/graphql/conftest.py +++ b/api/tests/graphql/conftest.py @@ -1,3 +1,5 @@ +from copy import deepcopy + import pytest import strawberry from strawberry.schema.config import StrawberryConfig @@ -13,7 +15,7 @@ EXAMPLE_TAGS, EXAMPLE_VARIABLE_TAGS, EXAMPLE_VARIABLES, - RUNS, + RUN_IDENTIFIERS, ) @@ -22,6 +24,7 @@ def reset_caches(): fetch_metadata.cache_clear() poll_proposal.cache_clear() subscriptions._last_seen_timestamp.clear() + subscriptions._last_run_added_at.clear() return @@ -44,35 +47,38 @@ def bypass_proposal_permission(mocker): _patch_permissions(mocker, authenticated=True, member=True) +# `fetch_metadata` folds tags into variables and variables into tags by +# mutating what it reads. The real db builds those maps afresh from each query, +# so hand out a copy per call; returning the constant itself lets one call's +# edits pile up in the next one's result. +def _fresh(mocker, target, value): + mocker.patch(target, side_effect=lambda *args, **kwargs: deepcopy(value)) + + @pytest.fixture def mocked_metadata_variables(mocker): - mocker.patch( - "damnit_api.graphql.metadata.db.async_variables", - return_value=EXAMPLE_VARIABLES, - ) + _fresh(mocker, "damnit_api.graphql.metadata.db.async_variables", EXAMPLE_VARIABLES) @pytest.fixture def mocked_metadata_all_tags(mocker): - mocker.patch( - "damnit_api.graphql.metadata.db.async_all_tags", - return_value=EXAMPLE_TAGS, - ) + _fresh(mocker, "damnit_api.graphql.metadata.db.async_all_tags", EXAMPLE_TAGS) @pytest.fixture def mocked_metadata_variable_tags(mocker): - mocker.patch( + _fresh( + mocker, "damnit_api.graphql.metadata.db.async_variable_tags", - return_value=EXAMPLE_VARIABLE_TAGS, + EXAMPLE_VARIABLE_TAGS, ) @pytest.fixture -def mocked_metadata_column(mocker): - mocker.patch( - "damnit_api.graphql.metadata.db.async_column", - return_value=RUNS, +def mocked_metadata_run_identifiers(mocker): + return mocker.patch( + "damnit_api.graphql.metadata.db.async_run_identifiers", + return_value=RUN_IDENTIFIERS, ) @@ -96,7 +102,7 @@ def mocked_ensure_damnit_path(mocker): @pytest.fixture def graphql_schema_no_auth( mocked_metadata_variables, - mocked_metadata_column, + mocked_metadata_run_identifiers, mocked_metadata_all_tags, mocked_metadata_variable_tags, ): diff --git a/api/tests/graphql/const.py b/api/tests/graphql/const.py index 507b95c6..ef850057 100644 --- a/api/tests/graphql/const.py +++ b/api/tests/graphql/const.py @@ -5,6 +5,7 @@ PROPOSAL = 900485 RUNS = [348, 349, 350] +RUN_IDENTIFIERS = [(PROPOSAL, run) for run in RUNS] @dataclass(frozen=True, kw_only=True) @@ -87,6 +88,16 @@ def get_values(data): "etof.eTOF_response_width": [7], } +# What `fetch_metadata` makes of the three above: each variable carries the +# names of its tags, not their ids. +EXAMPLE_TAGGED_VARIABLES = { + name: { + **variable, + "tags": [EXAMPLE_TAGS[tag]["name"] for tag in EXAMPLE_VARIABLE_TAGS[name]], + } + for name, variable in EXAMPLE_VARIABLES.items() +} + # ----------------------------------------------------------------------------- # Run variable values for a single run diff --git a/api/tests/graphql/test_models.py b/api/tests/graphql/test_models.py index da7e4ea8..029e2993 100644 --- a/api/tests/graphql/test_models.py +++ b/api/tests/graphql/test_models.py @@ -170,16 +170,24 @@ def test_extract_error_returns_none(attributes): # Test DamnitRun error path -def test_resolve_includes_error_for_failed_variable(): +def test_from_db_includes_error_for_failed_variable(): record = { + "proposal": {"value": 900485}, "run": {"value": 1}, "broken": {"value": None, "attributes": json.dumps(ERROR_ATTRS)}, } - resolved = DamnitRun.resolve(record) + run = DamnitRun.from_db(record, database="900485") - assert "error" not in resolved["run"] - assert resolved["broken"]["value"] is None - assert resolved["broken"]["error"] == { - "message": ERROR_ATTRS["error"], - "cls": "Foo", - } + 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"].error == CellError(message=ERROR_ATTRS["error"], cls="Foo") + + +def test_from_db_populates_identity_trio(): + record = {"proposal": {"value": 900485}, "run": {"value": 348}} + run = DamnitRun.from_db(record, database="900485") + + assert run.database == "900485" + assert run.proposal == "900485" + assert run.run == 348 diff --git a/api/tests/graphql/test_queries.py b/api/tests/graphql/test_queries.py index 02255a43..ef8ad54f 100644 --- a/api/tests/graphql/test_queries.py +++ b/api/tests/graphql/test_queries.py @@ -7,10 +7,10 @@ from .const import ( EXAMPLE_DATA, - EXAMPLE_VARIABLES, + EXAMPLE_TAGGED_VARIABLES, KNOWN_DATA, PROPOSAL, - RUNS, + RUN_IDENTIFIERS, get_values, ) @@ -36,9 +36,11 @@ def mocked_fetch_cells(mocker): @pytest.fixture def mocked_fetch_info(mocker): + # fetch_info returns a mapping keyed by (proposal, run). + info = get_values(KNOWN_DATA) return mocker.patch( "damnit_api.graphql.queries.fetch_info", - return_value=[get_values(KNOWN_DATA)], + return_value={(info["proposal"], info["run"]): info}, ) @@ -70,6 +72,31 @@ async def test_runs_query(graphql_schema, mocked_fetch_cells, mocked_fetch_info) assert mocked_fetch_info.called +@pytest.mark.asyncio +async def test_runs_query_returns_identity_trio( + graphql_schema, mocked_fetch_cells, mocked_fetch_info +): + query = f""" + query {{ + runs(database: {{proposal: "{PROPOSAL}"}}, per_page: 2) {{ + database + proposal + run + }} + }} + """ + result = await graphql_schema.execute(query) + + assert result.errors is None + assert result.data["runs"] == [ + {"database": str(PROPOSAL), "proposal": str(PROPOSAL), "run": 348} + ] + + # Identity-only: no cells to load, so run_info is skipped too. + assert mocked_fetch_cells.call_args.kwargs["names"] == [] + assert not mocked_fetch_info.called + + @pytest.mark.asyncio async def test_runs_query_filters_cells_by_name( graphql_schema, mocked_fetch_cells, mocked_fetch_info @@ -235,6 +262,153 @@ async def test_runs_query_partial_name_match(graphql_schema, real_damnit_db): ] +@pytest_asyncio.fixture +async def two_proposal_db(mocker, tmp_path, request): + """A file holding two proposals that share a run number, wired via + `find_proposal`. The addressing proposal (999999) is the active one in + `metameta`; 888888 is a guest. Yields the addressing proposal id. + + Parametrise indirectly with the `metameta` proposal value to write, or + with None to write no row at all. + """ + proposal = "999999" + active_value = getattr(request, "param", proposal) + guest = 888888 + proposal_root = tmp_path / "proposal" + (proposal_root / DAMNIT_PATH).mkdir(parents=True) + + mocker.patch( + "damnit_api.runs.sqlite.session.find_proposal", + return_value=str(proposal_root), + ) + DatabaseSessionManager.registry.pop(proposal, None) # pyright: ignore[reportAttributeAccessIssue] + + manager = DatabaseSessionManager(proposal) + async with manager.connect() as conn: + await conn.execute( + text( + "CREATE TABLE run_variables (" + " proposal INTEGER NOT NULL," + " run INTEGER NOT NULL," + " name TEXT NOT NULL," + " value BLOB," + " summary_type TEXT," + " attributes BLOB," + " timestamp REAL NOT NULL," + " PRIMARY KEY (proposal, run, name, timestamp)" + ")" + ) + ) + await conn.execute( + text("CREATE TABLE run_info (proposal INTEGER, run INTEGER)") + ) + await conn.execute(text("CREATE TABLE metameta (key TEXT, value TEXT)")) + + variable_rows = [ + # (999999, 1): a superseded and a latest `alpha`. + (int(proposal), 1, "alpha", "a1_old", 1000.0), + (int(proposal), 1, "alpha", "a1", 2000.0), + (int(proposal), 2, "alpha", "a2", 1500.0), + # Guest run collides on run number 1 with its own value. + (guest, 1, "alpha", "guest_a1", 1200.0), + ] + await conn.execute( + text( + "INSERT INTO run_variables" + " (proposal, run, name, value, timestamp)" + " VALUES (:proposal, :run, :name, :value, :timestamp)" + ), + [ + {"proposal": p, "run": r, "name": n, "value": v, "timestamp": t} + for p, r, n, v, t in variable_rows + ], + ) + await conn.execute( + text("INSERT INTO run_info (proposal, run) VALUES (:proposal, :run)"), + [ + {"proposal": int(proposal), "run": 1}, + {"proposal": int(proposal), "run": 2}, + {"proposal": guest, "run": 1}, + ], + ) + if active_value is not None: + await conn.execute( + text("INSERT INTO metameta (key, value) VALUES ('proposal', :value)"), + {"value": active_value}, + ) + + yield proposal + + await manager.close() + DatabaseSessionManager.registry.pop(proposal, None) # pyright: ignore[reportAttributeAccessIssue] + + +@pytest.mark.asyncio +async def test_runs_query_includes_guests_active_block_first( + graphql_schema, two_proposal_db +): + """Guests are included and ordered after the active proposal's block, + and a run number shared across proposals keeps each proposal's value. + """ + proposal = two_proposal_db + query = f""" + query {{ + runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{ + proposal + run + cells(names: ["alpha"]) {{ name value }} + }} + }} + """ + result = await graphql_schema.execute(query) + + assert result.errors is None + runs = result.data["runs"] + + # Active proposal (999999) block first, then the guest (888888). + assert [(r["proposal"], r["run"]) for r in runs] == [ + ("999999", 1), + ("999999", 2), + ("888888", 1), + ] + + alpha = [{v["name"]: v["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"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "two_proposal_db", + # No `proposal` row at all, and a row DAMNIT wrote something unreadable in. + [None, "not-a-number"], + indirect=True, +) +async def test_runs_query_without_an_active_proposal_orders_by_proposal_then_run( + graphql_schema, two_proposal_db +): + """A file whose `metameta` names no readable active proposal has no block + to put first, so runs order by (proposal, run) and the guest leads. + """ + proposal = two_proposal_db + query = f""" + query {{ + runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{ + proposal + run + }} + }} + """ + result = await graphql_schema.execute(query) + + assert result.errors is None + assert [(r["proposal"], r["run"]) for r in result.data["runs"]] == [ + ("888888", 1), + ("999999", 1), + ("999999", 2), + ] + + @pytest.mark.asyncio async def test_runs_query_fetches_run_info_when_metadata_requested( graphql_schema, mocked_fetch_cells, mocked_fetch_info @@ -259,7 +433,12 @@ async def test_runs_query_fetches_run_info_when_metadata_requested( async def test_metadata_query(graphql_schema): query = """ query TableMetadataQuery($proposal: String) { - metadata(database: { proposal: $proposal }) + metadata(database: { proposal: $proposal }) { + runs { proposal run } + variables + tags + timestamp + } } """ result = await graphql_schema.execute( @@ -271,10 +450,12 @@ async def test_metadata_query(graphql_schema): metadata = result.data["metadata"] assert set(metadata.keys()) == {"runs", "variables", "timestamp", "tags"} - assert metadata["runs"] == RUNS + assert metadata["runs"] == [ + {"proposal": str(proposal), "run": run} for proposal, run in RUN_IDENTIFIERS + ] assert metadata["variables"] == { **DamnitRun.known_variables(), - **EXAMPLE_VARIABLES, + **EXAMPLE_TAGGED_VARIABLES, } assert "(Untagged)" in metadata["tags"] assert "eTOF" in metadata["tags"] diff --git a/api/tests/graphql/test_subscriptions.py b/api/tests/graphql/test_subscriptions.py index 9afaad03..9f73a53e 100644 --- a/api/tests/graphql/test_subscriptions.py +++ b/api/tests/graphql/test_subscriptions.py @@ -4,16 +4,19 @@ import pytest -from damnit_api.graphql.subscriptions import POLLING_INTERVAL, filter_for_client -from damnit_api.runs.types import DamnitRun +from damnit_api.graphql.subscriptions import ( + POLLING_INTERVAL, + filter_for_client, + poll_proposal, +) +from damnit_api.runs.types import DamnitRun, TableMeta from damnit_api.shared.const import DamnitType from .const import ( - EXAMPLE_VARIABLES, KNOWN_DATA, NEW_DATA, PROPOSAL, - RUNS, + RUN_IDENTIFIERS, DatabaseVariable, get_values, ) @@ -21,9 +24,27 @@ NEW_RUN = 400 - patched_sleep = patch.object(asyncio, "sleep", return_value=None) +SUBSCRIPTION = """ + subscription RunUpdatesSubscription( + $proposal: String, + $since: Timestamp!) { + run_updates(database: { proposal: $proposal }, since: $since) { + runs { + database + proposal + run + cells { name value dtype } + } + metadata { + runs { proposal run } + } + timestamp + } + } +""" + @pytest.fixture(scope="module") def current_timestamp(): @@ -58,32 +79,54 @@ def mocked_returns(*args, table, **kwargs): ) +@pytest.fixture +def mocked_no_new_rows(mocker): + """The poll finds the `run_variables` table but nothing newer in it.""" + mocker.patch( + "damnit_api.graphql.subscriptions.async_table", + return_value=mocker.sentinel.run_variables_table, + ) + mocker.patch("damnit_api.graphql.subscriptions.async_max", return_value=0) + return mocker.patch( + "damnit_api.graphql.subscriptions.async_latest_rows", + return_value=[], + ) + + @pytest.fixture def mocked_fetch_info(mocker): + # fetch_info returns a mapping keyed by (proposal, run). + info = {**get_values(KNOWN_DATA), "run": NEW_RUN} return mocker.patch( "damnit_api.graphql.subscriptions.fetch_info", - return_value=[{**get_values(KNOWN_DATA), "run": NEW_RUN}], + return_value={(info["proposal"], info["run"]): info}, ) +@pytest.fixture +def mocked_metadata_reads( + mocked_metadata_variables, + mocked_metadata_run_identifiers, + mocked_metadata_all_tags, + mocked_metadata_variable_tags, + mocked_metadata_max, +): + """The sqlite reads behind `fetch_metadata`, for polling the poll directly + instead of through the schema.""" + + @pytest.mark.asyncio -async def test_latest_data( +async def test_run_updates( graphql_schema, current_timestamp, mocked_latest_rows, mocked_fetch_info, ): subscription = await graphql_schema.subscribe( - """ - subscription LatestDataSubscription( - $proposal: String, - $timestamp: Timestamp!) { - latest_data(database: { proposal: $proposal }, timestamp: $timestamp) - } - """, + SUBSCRIPTION, variable_values={ "proposal": str(PROPOSAL), - "timestamp": (current_timestamp - 1) * 1000, # before the new row + "since": (current_timestamp - 1) * 1000, # before the new row }, ) @@ -91,66 +134,54 @@ async def test_latest_data( result = await asyncio.wait_for(anext(subscription), timeout=2) assert not result.errors - data = { + payload = result.data["run_updates"] + assert payload["timestamp"] == current_timestamp * 1000 + + # One changed run, carrying its identity trio and merged variables. + assert len(payload["runs"]) == 1 + run = payload["runs"][0] + assert run["database"] == str(PROPOSAL) + assert run["proposal"] == str(PROPOSAL) + assert run["run"] == NEW_RUN + + expected = { **KNOWN_DATA, **NEW_DATA, - "run": DatabaseVariable( - value=NEW_RUN, - damnit_dtype=DamnitType.NUMBER, - ), + "run": DatabaseVariable(value=NEW_RUN, damnit_dtype=DamnitType.NUMBER), } - - latest_data = result.data["latest_data"] - assert set(latest_data.keys()) == {"runs", "metadata"} - assert latest_data["runs"] == { - NEW_RUN: { - name: { - "value": var.damnit_value, - "dtype": var.damnit_dtype.value, - } - for name, var in data.items() - } + got = { + v["name"]: {"value": v["value"], "dtype": v["dtype"]} for v in run["cells"] } - - metadata = latest_data["metadata"] - assert set(metadata.keys()) == {"runs", "timestamp", "variables"} - assert metadata["runs"] == [*RUNS, NEW_RUN] - assert metadata["timestamp"] == current_timestamp * 1000 - assert metadata["variables"] == { - **DamnitRun.known_variables(), - **EXAMPLE_VARIABLES, + assert got == { + name: {"value": var.damnit_value, "dtype": var.damnit_dtype.value} + for name, var in expected.items() } + + # Nothing has been pushed before, so this push carries the full, + # server-ordered metadata as (proposal, run) pairs. + metadata = payload["metadata"] + assert metadata["runs"] == [ + {"proposal": str(proposal), "run": run_number} + for proposal, run_number in RUN_IDENTIFIERS + ] finally: await subscription.aclose() @pytest.mark.asyncio -async def test_latest_data_with_concurrent_subscriptions( +async def test_run_updates_with_concurrent_subscriptions( graphql_schema, current_timestamp, mocked_latest_rows, mocked_fetch_info, ): - query = """ - subscription LatestDataSubscription( - $proposal: String, - $timestamp: Timestamp!) { - latest_data(database: { proposal: $proposal }, timestamp: $timestamp) - } - """ variables = { "proposal": str(PROPOSAL), - "timestamp": (current_timestamp - 1) * 1000, # before the new row + "since": (current_timestamp - 1) * 1000, # before the new row } - first_sub = await graphql_schema.subscribe( - query, - variable_values=variables, - ) - second_sub = await graphql_schema.subscribe( - query, - variable_values=variables, - ) + first_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) + second_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) try: with patched_sleep: @@ -170,32 +201,19 @@ async def test_latest_data_with_concurrent_subscriptions( @pytest.mark.asyncio -async def test_latest_data_with_nonconcurrent_subscriptions( +async def test_run_updates_with_nonconcurrent_subscriptions( graphql_schema, current_timestamp, mocked_latest_rows, mocked_fetch_info, ): - query = """ - subscription LatestDataSubscription( - $proposal: String, - $timestamp: Timestamp!) { - latest_data(database: { proposal: $proposal }, timestamp: $timestamp) - } - """ variables = { "proposal": str(PROPOSAL), - "timestamp": (current_timestamp - 1) * 1000, # before the new row + "since": (current_timestamp - 1) * 1000, # before the new row } - first_sub = await graphql_schema.subscribe( - query, - variable_values=variables, - ) - second_sub = await graphql_schema.subscribe( - query, - variable_values=variables, - ) + first_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) + second_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) try: with patched_sleep: @@ -215,17 +233,97 @@ async def test_latest_data_with_nonconcurrent_subscriptions( await second_sub.aclose() +@pytest.mark.asyncio +async def test_metadata_reaches_a_subscriber_that_polls_in_a_later_window( + graphql_schema, + current_timestamp, + mocked_latest_rows, + mocked_fetch_info, +): + variables = { + "proposal": str(PROPOSAL), + "since": (current_timestamp - 1) * 1000, # before the new row + } + + first_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) + second_sub = await graphql_schema.subscribe(SUBSCRIPTION, variable_values=variables) + + try: + with patched_sleep: + first = await asyncio.wait_for(anext(first_sub), timeout=2) + + # Subscribers drift apart: each generator's period is a sleep plus its + # own poll, so the second lands in a later cache window and re-runs the + # poll body. The metadata it has never been sent still has to arrive. + await asyncio.sleep(POLLING_INTERVAL * 3) + + with patched_sleep: + second = await asyncio.wait_for(anext(second_sub), timeout=2) + + assert first.data["run_updates"]["metadata"] is not None + assert second.data["run_updates"]["metadata"] is not None + finally: + await first_sub.aclose() + await second_sub.aclose() + + +# ----------------------------------------------------------------------------- +# poll_proposal + + +@pytest.mark.asyncio +async def test_poll_reports_a_signature_when_no_rows_changed( + mocked_no_new_rows, mocked_metadata_reads +): + snapshot = await poll_proposal(proposal=str(PROPOSAL)) + + assert snapshot["runs"] == {} + assert snapshot["metadata_signature"] + + +@pytest.mark.asyncio +async def test_poll_repeats_the_signature_while_metadata_is_unchanged( + mocked_no_new_rows, mocked_metadata_reads +): + first = await poll_proposal(proposal=str(PROPOSAL)) + + poll_proposal.cache_clear() + second = await poll_proposal(proposal=str(PROPOSAL)) + + assert second["metadata_signature"] == first["metadata_signature"] + + +@pytest.mark.asyncio +async def test_poll_stops_rereading_metadata_once_a_proposal_is_idle( + mocked_no_new_rows, mocked_metadata_reads, mocked_metadata_run_identifiers +): + await poll_proposal(proposal=str(PROPOSAL)) + reads = mocked_metadata_run_identifiers.call_count + + poll_proposal.cache_clear() + await poll_proposal(proposal=str(PROPOSAL)) + + # No new rows and no new run, so the tick rides the metadata cache instead + # of scanning every (proposal, run) pair in run_info again. + assert mocked_metadata_run_identifiers.call_count == reads + + # ----------------------------------------------------------------------------- # filter_for_client def _snapshot(run_timestamps): - runs = {run: {"value": run} for run in run_timestamps} + runs = { + (proposal, run): DamnitRun( + database=str(proposal), proposal=str(proposal), run=run, _cells=[] + ) + for proposal, run in run_timestamps + } return { "runs": runs, "run_timestamps": run_timestamps, "max_timestamp": max(run_timestamps.values()), - "metadata": {"runs": list(runs), "variables": {}, "timestamp": 0}, + "metadata_signature": "unchanged", } @@ -233,19 +331,31 @@ def test_filter_for_client_none_snapshot(): assert filter_for_client(None, since=0) is None -def test_filter_for_client_since_zero_returns_none(): - snapshot = _snapshot({1: 100.0, 2: 200.0}) - assert filter_for_client(snapshot, since=0) is None +def test_filter_for_client_cursorless_client_receives_this_tick(): + snapshot = _snapshot({(PROPOSAL, 1): 100.0, (PROPOSAL, 2): 200.0}) + result = filter_for_client(snapshot, since=0) + assert {run.run for run in result.runs} == {1, 2} + + +def test_filter_for_client_delivers_metadata_without_changed_runs(): + snapshot = _snapshot({(PROPOSAL, 1): 100.0}) + metadata = TableMeta( + runs=[], variables={}, tags={}, timestamp=snapshot["max_timestamp"] + ) + + result = filter_for_client(snapshot, since=300.0, metadata=metadata) + assert result.runs == [] + assert result.metadata is metadata def test_filter_for_client_excludes_equal_timestamp(): - snapshot = _snapshot({1: 100.0, 2: 200.0}) + snapshot = _snapshot({(PROPOSAL, 1): 100.0, (PROPOSAL, 2): 200.0}) result = filter_for_client(snapshot, since=100.0) - assert set(result["runs"].keys()) == {2} + assert {run.run for run in result.runs} == {2} def test_filter_for_client_since_above_all_returns_none(): - snapshot = _snapshot({1: 100.0, 2: 200.0}) + snapshot = _snapshot({(PROPOSAL, 1): 100.0, (PROPOSAL, 2): 200.0}) assert filter_for_client(snapshot, since=300.0) is None @@ -254,11 +364,11 @@ def test_filter_for_client_since_above_all_returns_none(): @pytest.mark.asyncio -async def test_latest_data_unauthorized(graphql_schema_no_auth, current_timestamp): +async def test_run_updates_unauthorized(graphql_schema_no_auth, current_timestamp): gen = await graphql_schema_no_auth.subscribe( """ subscription { - latest_data(database: { proposal: "999999" }, timestamp: 0) + run_updates(database: { proposal: "999999" }, since: 0) { timestamp } } """, ) @@ -270,11 +380,13 @@ async def test_latest_data_unauthorized(graphql_schema_no_auth, current_timestam @pytest.mark.asyncio -async def test_latest_data_forbidden(graphql_schema_authenticated_non_member): +async def test_run_updates_forbidden(graphql_schema_authenticated_non_member): gen = await graphql_schema_authenticated_non_member.subscribe( f""" subscription {{ - latest_data(database: {{ proposal: "{PROPOSAL}" }}, timestamp: 0) + run_updates(database: {{ proposal: "{PROPOSAL}" }}, since: 0) {{ + timestamp + }} }} """, ) diff --git a/api/tests/graphql/test_utils.py b/api/tests/graphql/test_utils.py index 049bc880..fee9ef89 100644 --- a/api/tests/graphql/test_utils.py +++ b/api/tests/graphql/test_utils.py @@ -1,11 +1,17 @@ from damnit_api.graphql.utils import LatestData -from .const import EXAMPLE_DATA, NEW_DATA, get_values +from .const import EXAMPLE_DATA, NEW_DATA, PROPOSAL, get_values -def to_row(values, run=1, timestamp=1): +def to_row(values, proposal=PROPOSAL, run=1, timestamp=1): return [ - {"run": run, "name": name, "value": value, "timestamp": timestamp} + { + "proposal": proposal, + "run": run, + "name": name, + "value": value, + "timestamp": timestamp, + } for name, value in values.items() ] @@ -20,8 +26,8 @@ def test_latest_data_update_run(): latest_data = LatestData.from_list(first + second) assert len(latest_data.runs) == 1 - run, variables = next(iter(latest_data.runs.items())) - assert run == 1 + key, variables = next(iter(latest_data.runs.items())) + assert key == (PROPOSAL, 1) updated_values = {**example_values, **new_values} assert variables.keys() == updated_values.keys() @@ -38,16 +44,32 @@ def test_latest_data_multiple_runs(): second = to_row(new_values, run=2) latest_data = LatestData.from_list(first + second) - assert list(latest_data.runs.keys()) == [1, 2] + assert list(latest_data.runs.keys()) == [(PROPOSAL, 1), (PROPOSAL, 2)] - run_1 = latest_data.runs[1] + run_1 = latest_data.runs[PROPOSAL, 1] assert run_1.keys() == example_values.keys() for name, data in run_1.items(): assert data.value == example_values[name] assert data.timestamp == 1 - run_2 = latest_data.runs[2] + run_2 = latest_data.runs[PROPOSAL, 2] assert run_2.keys() == new_values.keys() for name, data in run_2.items(): assert data.value == new_values[name] assert data.timestamp == 1 + + +def test_latest_data_keeps_colliding_run_numbers_apart(): + """The same run number in two proposals stays two runs, not one.""" + guest = 900405 + principal_values = get_values(EXAMPLE_DATA) + guest_values = get_values(NEW_DATA) + + principal = to_row(principal_values, proposal=PROPOSAL, run=5) + other = to_row(guest_values, proposal=guest, run=5) + + latest_data = LatestData.from_list(principal + other) + + assert set(latest_data.runs.keys()) == {(PROPOSAL, 5), (guest, 5)} + assert latest_data.runs[PROPOSAL, 5].keys() == principal_values.keys() + assert latest_data.runs[guest, 5].keys() == guest_values.keys() diff --git a/api/tests/refactor/conftest.py b/api/tests/refactor/conftest.py index 850c2924..af0148da 100644 --- a/api/tests/refactor/conftest.py +++ b/api/tests/refactor/conftest.py @@ -10,8 +10,8 @@ graphql_schema_no_auth, mocked_ensure_damnit_path, mocked_metadata_all_tags, - mocked_metadata_column, mocked_metadata_max, + mocked_metadata_run_identifiers, mocked_metadata_variable_tags, mocked_metadata_variables, reset_caches, diff --git a/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr b/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr index eecfab8a..0c7fd7a1 100644 --- a/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr +++ b/api/tests/refactor/e2e/__snapshots__/test_data_parity.ambr @@ -17,19 +17,58 @@ # name: test_metadata_query_wire_shape_unchanged dict({ 'runs': list([ - 1, - 2, - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, + dict({ + 'proposal': '6996', + 'run': 1, + }), + dict({ + 'proposal': '6996', + 'run': 2, + }), + dict({ + 'proposal': '6996', + 'run': 3, + }), + dict({ + 'proposal': '6996', + 'run': 4, + }), + dict({ + 'proposal': '6996', + 'run': 5, + }), + dict({ + 'proposal': '6996', + 'run': 6, + }), + dict({ + 'proposal': '6996', + 'run': 7, + }), + dict({ + 'proposal': '6996', + 'run': 8, + }), + dict({ + 'proposal': '6996', + 'run': 9, + }), + dict({ + 'proposal': '6996', + 'run': 10, + }), + dict({ + 'proposal': '6996', + 'run': 11, + }), + dict({ + 'proposal': '6996', + 'run': 12, + }), + dict({ + 'proposal': '6996', + 'run': 13, + }), ]), 'tags': dict({ '(Untagged)': dict({ diff --git a/api/tests/refactor/e2e/test_authz_parity.py b/api/tests/refactor/e2e/test_authz_parity.py index be1adbe2..1d6e849d 100644 --- a/api/tests/refactor/e2e/test_authz_parity.py +++ b/api/tests/refactor/e2e/test_authz_parity.py @@ -37,7 +37,13 @@ def runs_query(proposal: int) -> dict: def metadata_query(proposal: int) -> dict: - return {"query": f'query {{ metadata(database: {{ proposal: "{proposal}" }}) }}'} + return { + "query": f""" + query {{ + metadata(database: {{ proposal: "{proposal}" }}) {{ timestamp }} + }} + """ + } async def test_runs_query_forbidden_for_non_member_unchanged(logged_in_client): diff --git a/api/tests/refactor/e2e/test_data_parity.py b/api/tests/refactor/e2e/test_data_parity.py index 5c03bd5a..9b3a67b2 100644 --- a/api/tests/refactor/e2e/test_data_parity.py +++ b/api/tests/refactor/e2e/test_data_parity.py @@ -53,7 +53,18 @@ def runs_query(proposal: int, *, per_page: int, names: list[str]) -> dict: def metadata_query(proposal: int) -> dict: - return {"query": f'query {{ metadata(database: {{ proposal: "{proposal}" }}) }}'} + return { + "query": f""" + query {{ + metadata(database: {{ proposal: "{proposal}" }}) {{ + runs {{ proposal run }} + variables + tags + timestamp + }} + }} + """ + } GET_USER_PROPOSALS_QUERY = """ @@ -100,7 +111,7 @@ async def test_runs_query_wire_shapes_unchanged(logged_in_client, snapshot): runs = payload["data"]["runs"] assert len(runs) == 1 - by_name = {c["name"]: c for c in runs[0]["cells"]} + by_name = {v["name"]: v for v in runs[0]["cells"]} assert set(by_name) == set(names) # Image variables serialize to a base64 PNG data URI, not raw bytes; pin diff --git a/api/tests/refactor/snapshots/schema.graphql b/api/tests/refactor/snapshots/schema.graphql index 762b3b1b..d79ec34d 100644 --- a/api/tests/refactor/snapshots/schema.graphql +++ b/api/tests/refactor/snapshots/schema.graphql @@ -16,6 +16,9 @@ type CellError { } type DamnitRun { + database: String! + proposal: String! + run: Int! cells(names: [String!] = null): [Cell!]! } @@ -69,13 +72,31 @@ type ProposalMeta { type Query { get_user: User! runs(database: DatabaseInput!, page: Int! = 1, per_page: Int! = 10): [DamnitRun!]! - metadata(database: DatabaseInput!): JSON! + metadata(database: DatabaseInput!): TableMeta! extracted_data(database: DatabaseInput!, run: Int!, variable: String!): JSON proposal_metadata(proposal_numbers: [Int!]!): [ProposalMeta!] } +type RunId { + proposal: String! + run: Int! +} + +type RunUpdates { + runs: [DamnitRun!]! + metadata: TableMeta + timestamp: Timestamp! +} + type Subscription { - latest_data(database: DatabaseInput!, timestamp: Timestamp!): JSON! + run_updates(database: DatabaseInput!, since: Timestamp!): RunUpdates! +} + +type TableMeta { + runs: [RunId!]! + variables: JSON! + tags: JSON! + timestamp: Timestamp! } scalar Timestamp diff --git a/api/tests/refactor/test_gql_parity.py b/api/tests/refactor/test_gql_parity.py index ea8836be..0d75aeef 100644 --- a/api/tests/refactor/test_gql_parity.py +++ b/api/tests/refactor/test_gql_parity.py @@ -25,7 +25,7 @@ from ..graphql.const import ( EXAMPLE_DATA, - EXAMPLE_VARIABLES, + EXAMPLE_TAGGED_VARIABLES, KNOWN_DATA, NEW_DATA, PROPOSAL, @@ -106,9 +106,10 @@ def mocked_fetch_cells(mocker): @pytest.fixture def mocked_fetch_info(mocker): + info = get_values(KNOWN_DATA) return mocker.patch( "damnit_api.graphql.queries.fetch_info", - return_value=[get_values(KNOWN_DATA)], + return_value={(info["proposal"], info["run"]): info}, ) @@ -150,7 +151,12 @@ async def test_runs_query_wire_shape_unchanged( async def test_metadata_query_wire_shape_unchanged(graphql_schema): query = """ query($proposal: String) { - metadata(database: { proposal: $proposal }) + metadata(database: { proposal: $proposal }) { + runs { proposal run } + variables + tags + timestamp + } } """ result = await graphql_schema.execute( @@ -163,9 +169,11 @@ async def test_metadata_query_wire_shape_unchanged(graphql_schema): metadata = result.data["metadata"] assert set(metadata.keys()) == {"runs", "variables", "timestamp", "tags"} assert isinstance(metadata["runs"], list) + for identifier in metadata["runs"]: + assert set(identifier.keys()) == {"proposal", "run"} assert metadata["variables"] == { **DamnitRun.known_variables(), - **EXAMPLE_VARIABLES, + **EXAMPLE_TAGGED_VARIABLES, } assert isinstance(metadata["timestamp"], int | float) @@ -205,14 +213,15 @@ def mocked_returns(*args, table, **kwargs): @pytest.fixture def mocked_subscription_fetch_info(mocker): + info = {**get_values(KNOWN_DATA), "run": NEW_RUN} return mocker.patch( "damnit_api.graphql.subscriptions.fetch_info", - return_value=[{**get_values(KNOWN_DATA), "run": NEW_RUN}], + return_value={(info["proposal"], info["run"]): info}, ) @pytest.mark.asyncio -async def test_latest_data_subscription_wire_shape_unchanged( +async def test_run_updates_subscription_wire_shape_unchanged( graphql_schema, current_timestamp, mocked_latest_rows, @@ -222,16 +231,27 @@ async def test_latest_data_subscription_wire_shape_unchanged( """ subscription( $proposal: String, - $timestamp: Timestamp!) { - latest_data( + $since: Timestamp!) { + run_updates( database: { proposal: $proposal }, - timestamp: $timestamp - ) + since: $since + ) { + runs { + database + proposal + run + cells { name value dtype } + } + metadata { + runs { proposal run } + } + timestamp + } } """, variable_values={ "proposal": str(PROPOSAL), - "timestamp": (current_timestamp - 1) * 1000, + "since": (current_timestamp - 1) * 1000, }, ) @@ -239,19 +259,23 @@ async def test_latest_data_subscription_wire_shape_unchanged( result = await asyncio.wait_for(anext(subscription), timeout=2) assert not result.errors - payload = result.data["latest_data"] - assert set(payload.keys()) == {"runs", "metadata"} + payload = result.data["run_updates"] + assert set(payload.keys()) == {"runs", "metadata", "timestamp"} runs = payload["runs"] - assert set(runs.keys()) == {NEW_RUN} - run_data = runs[NEW_RUN] - assert set(run_data.keys()) >= {"n_trains", "run_length", "xgm_intensity"} - for variable in run_data.values(): - assert set(variable.keys()) == {"value", "dtype"} + assert isinstance(runs, list) + run = runs[0] + 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"} metadata = payload["metadata"] - assert set(metadata.keys()) == {"runs", "timestamp", "variables"} - # ms-timestamp, matching the `metadata` query's serialization - assert metadata["timestamp"] == current_timestamp * 1000 + assert set(metadata.keys()) == {"runs"} + for identifier in metadata["runs"]: + assert set(identifier.keys()) == {"proposal", "run"} + + # ms-timestamp cursor, matching the `metadata` query's serialization + assert payload["timestamp"] == current_timestamp * 1000 finally: await subscription.aclose() diff --git a/frontend/README.md b/frontend/README.md index 1eea6429..85588cee 100644 --- a/frontend/README.md +++ b/frontend/README.md @@ -55,45 +55,38 @@ That keeps a file's imports stable when the tree moves, and it is enforced. ### Data flow -Every GraphQL fetch goes through an Apollo hook. What a screen renders _from_ -depends on whether the schema lets Apollo key the data: - -- **Table rows and summary plots** flow from hooks into the `tableData` Redux - slice, which owns the merge. `DamnitRun` has no id: the API models the run - number as a known variable alongside the real ones, so there is nothing for - Apollo to normalize and the merge cannot move into the cache yet. `TablePageLoader` - renders one instance per wanted page, since a component can own only one - watched query, and each dispatches its own rows. -- **Preview plots** render straight from the Apollo cache, with no slice. - `extracted_data` fetches one run per call and has no batch field, so - `usePreviewPlotData` builds a document that aliases the field per run - (`r142: extracted_data(run: 142, ...)`), chunked by run value. One - `PreviewChunkLoader` per chunk fetches cache-and-network, while the parent - watches the whole aliased document cache-only with `returnPartialData`, so the - plot fills in as chunks land. Aliases are erased in the store, so plots that - overlap on a run read the one entry; each chunk still revalidates it. - -Two cache behaviours are worth knowing before you touch a runs document: - -- **A directive is part of the store key.** The lightweight and deferred queries - send identical `runs(...)` arguments, so the deferred write looks certain to - clobber the lightweight one. It does not: the cache holds - `runs({...})@lightweight` and `runs({...})` as two fields. That separation only - holds because `@lightweight` is in the document. -- **Column subsets replace, they do not merge.** Because `DamnitRun` is - unnormalized the runs array is stored inline, and every write replaces it - whole, so two documents asking for different `names` at the same page evict - each other rather than sharing. This is why summary plots fetch `no-cache`: - the slice is the only thing that renders their rows, so a cached copy is never - read, and caching one would only evict the table's. Do not paper over it with - typePolicies against the unkeyed schema; the real fix is a run key on the - backend. - -Both are symptoms of the same missing key. Once the backend gives `DamnitRun` -one and adds a batch preview field, runs normalize in the cache, pagination -becomes one query plus `fetchMore`, the grid and summary plots render from the -cache, and the `tableData` slice goes away. Summary plots go back to -cache-and-network with it. +Every GraphQL fetch goes through an Apollo hook, and the Apollo cache is the +render source. `DamnitRun` carries an identity trio (`database`, `proposal`, +`run`) and the cache keys it on all three, since run numbers collide across +proposals in one file (`typePolicies` in `graphql/type-policies.ts`). + +- **Table rows and summary plots** render from the cache. The row layout is the + server-ordered `metadata.runs`; a cell's value is looked up by the run's + identity. `useTableRuns` owns one watched lightweight query and `fetchMore`s + the pages the user scrolls to; the `Query.runs` field policy (`keyArgs` + `['database']`) dedups every page, deferred fill, and pushed run into one + list. The heavy pass still runs: after a page lands, its blanked heavy values + are refetched (`network-only`, throttled by the priority link) and merge into + the same runs. The `DamnitRun.cells` field policy (`keyArgs: false`) merges + the lightweight, deferred, and pushed cell sets into one bag per run, keyed by + name; a held-back blank (an errorless null) never overwrites a value already + in place. +- **Preview plots** render straight from the Apollo cache. `extracted_data` + fetches one run per call and has no batch field, so `usePreviewPlotData` + builds a document that aliases the field per run (`r142: + extracted_data(run: 142, ...)`), chunked by run value. One + `PreviewChunkLoader` per chunk fetches + cache-and-network, while the parent watches the whole aliased document + cache-only with `returnPartialData`, so the plot fills in as chunks land. + +Liveness comes from the cache. The `run_updates` subscription (`use-proposal`) +writes changed runs into the normalized cache and, when the run list, variables, +or tags change, replaces the `TableMeta` entry; the `since` cursor advances with +each push. Leaving a proposal drops its cached fields and collects the runs they +referenced (`app/store/listeners.ts`), deferred until the departing page's +watchers have unsubscribed so the eviction cannot send them back to the network. +There is no stale-proposal guard: the three-part key makes a departed proposal's +runs unreadable by the next one. ## Installation diff --git a/frontend/e2e/mocks/index.ts b/frontend/e2e/mocks/index.ts index 27fc2dcc..55e05129 100644 --- a/frontend/e2e/mocks/index.ts +++ b/frontend/e2e/mocks/index.ts @@ -59,18 +59,20 @@ export async function mockApi( let contextContent = example.contextFile let contextLastModified = 1_700_000_000 + // variables.proposal arrives as a string, so hold the accessible set as strings. + const accessible = accessibleProposals(example).map(String) + const api: MockApi = { unmockedRequests: [], touchContextFile: (content) => { contextContent = content contextLastModified += 1 }, - pushLatestData: await mockWebSocket(page), + // A push carries the run's identity, so it is delivered for the proposal the + // subscription opened (the first accessible one in a single-proposal example). + pushLatestData: await mockWebSocket(page, accessible[0]), } - // variables.proposal arrives as a string, so hold the accessible set as strings. - const accessible = accessibleProposals(example).map(String) - // One example per test, so the source ignores the requested proposal. A // missing (run, variable) file surfaces as ENOENT; translate it to // MockDataNotFound so the resolver reports it as clean drift. Any other error diff --git a/frontend/e2e/mocks/websocket.ts b/frontend/e2e/mocks/websocket.ts index a75bb376..e3173155 100644 --- a/frontend/e2e/mocks/websocket.ts +++ b/frontend/e2e/mocks/websocket.ts @@ -1,15 +1,23 @@ import type { Page, WebSocketRoute } from '@playwright/test' -import type { Meta, RunData } from '@damnit-frontend/shared/mocks' +import { + shapeCell, + type Meta, + type RunData, +} from '@damnit-frontend/shared/mocks' -// A subscription push: the run-keyed data map plus table metadata. This mirrors -// the backend's latest_data payload, which carries runs and variables but never -// tags (the reducer merges metadata, so the seed's tags survive). The mock -// stamps the timestamp at delivery, like the server, so a caller supplies only -// runs and variables. `runs` is numeric to match the seed and the backend. +// A subscription push, in the test's convenient shape: a run-keyed data map plus +// the table metadata. `deliver` translates it into the backend's `run_updates` +// wire payload (the identity trio on every run, run-identifier pairs, and a +// __typename on each object so Apollo normalizes it). `runs` and `metadata.runs` +// stay numeric to match the seed; `deliver` pairs them with the open proposal. export type LatestData = { runs: Record - metadata: { runs: number[]; variables: Meta['variables'] } + metadata: { + runs: number[] + variables: Meta['variables'] + tags?: Meta['tags'] + } } export type PushLatestData = (data: LatestData) => void @@ -18,14 +26,17 @@ export type PushLatestData = (data: LatestData) => void // play the graphql-transport-ws server by hand: acknowledge the init, remember // the active subscription id, and answer pings. The subscription is never // completed by us, so a test can push repeatedly onto the same id. Returns -// pushLatestData, which delivers a `next` shaped like the backend's latest_data +// pushLatestData, which delivers a `next` shaped like the backend's run_updates // payload so a test can drive a live table update. // // Delivery is not gated on the subscription's `since` cursor: the mock sends // whatever is pushed, regardless of the timestamp the client subscribed with. // The backend owns that cursor filtering and tests it directly, so it is out of // scope here. -export async function mockWebSocket(page: Page): Promise { +export async function mockWebSocket( + page: Page, + proposal: string +): Promise { let socket: WebSocketRoute | undefined let activeId: string | undefined // Pushes made before the app has subscribed (the handshake can lag the grid @@ -39,16 +50,41 @@ export async function mockWebSocket(page: Page): Promise { 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( JSON.stringify({ id: activeId, type: 'next', payload: { data: { - latest_data: { - runs, - metadata: { ...metadata, timestamp: nextTimestamp() }, + run_updates: { + __typename: 'RunUpdates', + runs: Object.entries(runs).map(([run, variables]) => + shapeRun(Number(run), variables) + ), + metadata: { + __typename: 'TableMeta', + runs: metadata.runs.map((run) => ({ + __typename: 'RunId', + proposal, + run, + })), + variables: metadata.variables, + tags: metadata.tags ?? {}, + timestamp, + }, + timestamp, }, }, }, diff --git a/frontend/e2e/tests/app/live-updates/live-updates.spec.ts b/frontend/e2e/tests/app/live-updates/live-updates.spec.ts index 9d5e5670..e1e87093 100644 --- a/frontend/e2e/tests/app/live-updates/live-updates.spec.ts +++ b/frontend/e2e/tests/app/live-updates/live-updates.spec.ts @@ -20,12 +20,12 @@ import { // image-preview.spec. Every updated run sits in the initial vertical fold. test.use({ viewport: { width: 1600, height: 900 } }) -// A subscription push carries the full runs and variables. The reducer merges -// metadata, so the seed's tags survive and the push omits them (as the backend -// does); the mock stamps the timestamp. runs stays numeric to match the seed and -// the backend; a caller overrides it to add a run. +// A subscription push carries the full metadata snapshot: runs, variables, and +// tags. The client replaces its metadata wholesale, exactly as the backend +// sends it, so the push carries tags too. The mock stamps the timestamp; runs +// stays numeric to match the seed, and a caller overrides it to add a run. function fullMetadata(runs: number[]) { - return { runs, variables: XPCS.meta.variables } + return { runs, variables: XPCS.meta.variables, tags: XPCS.meta.tags } } test('a finished run appears as a new row', async ({ page, api, example }) => { diff --git a/frontend/e2e/tests/app/table/cell-detail.spec.ts b/frontend/e2e/tests/app/table/cell-detail.spec.ts index e37c875c..033de3fa 100644 --- a/frontend/e2e/tests/app/table/cell-detail.spec.ts +++ b/frontend/e2e/tests/app/table/cell-detail.spec.ts @@ -29,21 +29,22 @@ test('activating a single cell shows only that variable', async ({ test.describe('errored cell', () => { test.use({ example: xpcsWithErrors }) - test('activating an errored cell opens the sidebar but shows no value', async ({ + test('activating an errored cell shows the failure instead of a value', async ({ page, example, }) => { await openProposal(page, example) - // xgm_intensity failed for run 1, so its cell carries value null. + // xgm_intensity failed for run 1, so its cell carries an error and no value. const errored = ERROR_CELLS[0] await activateCell(page, { col: errored.col, row: ERROR_ROW }) const panel = page.getByRole('complementary') - // The aside still opens for the run. await expect(selectedRunTab(page)).toBeVisible() - // The null value is filtered out, so nothing renders for the cell. - await expect(panel.getByText(titleOf(errored.variable))).toHaveCount(0) + // The cell renders under its own title, with the failure in place of the + // value it never got. + await expect(panel.getByText(titleOf(errored.variable))).toBeVisible() + await expect(panel.getByText(errored.error.message)).toBeVisible() await expect(panel.locator('img')).toHaveCount(0) }) }) diff --git a/frontend/packages/shared/package.json b/frontend/packages/shared/package.json index cd726091..560f6afe 100644 --- a/frontend/packages/shared/package.json +++ b/frontend/packages/shared/package.json @@ -4,6 +4,7 @@ "version": "0.0.0", "type": "module", "exports": { + "./constants": "./src/constants.ts", "./mocks": "./src/mocks/index.ts" }, "scripts": { diff --git a/frontend/packages/shared/src/constants.ts b/frontend/packages/shared/src/constants.ts new file mode 100644 index 00000000..43a68351 --- /dev/null +++ b/frontend/packages/shared/src/constants.ts @@ -0,0 +1,5 @@ +// The dtypes the server's @lightweight directive holds back on the table's +// first pass (mirrors the API's HEAVY_DATA). A null value with one of these is +// a blank still being fetched; a null with any other dtype is a genuinely empty +// cell. Shared so the client and the mock server track the API in lockstep. +export const HEAVY_DTYPES = new Set(['image', 'rgba', 'array']) diff --git a/frontend/packages/shared/src/mocks/index.ts b/frontend/packages/shared/src/mocks/index.ts index 737ca262..7db7aaa2 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, + shapeCell, shapeMetadata, shapeTableData, unmockedOperationError, diff --git a/frontend/packages/shared/src/mocks/resolve.ts b/frontend/packages/shared/src/mocks/resolve.ts index c6041c16..dc47124a 100644 --- a/frontend/packages/shared/src/mocks/resolve.ts +++ b/frontend/packages/shared/src/mocks/resolve.ts @@ -73,9 +73,9 @@ export async function resolveOperation( try { switch (operationName) { - case 'TableMetadataQuery': { + case 'TableMetaQuery': { const { meta } = await source.runs(proposal) - return resolved({ metadata: shapeMetadata(meta) }) + return resolved({ metadata: shapeMetadata(meta, proposal) }) } case 'TableDataQuery': case 'LightweightTableDataQuery': @@ -84,6 +84,7 @@ export async function resolveOperation( const names = variables.names as string[] | null | undefined return resolved( shapeTableData(data, { + proposal, names, lightweight: operationName === 'LightweightTableDataQuery', }) diff --git a/frontend/packages/shared/src/mocks/shape.ts b/frontend/packages/shared/src/mocks/shape.ts index fdb7f653..4db9f538 100644 --- a/frontend/packages/shared/src/mocks/shape.ts +++ b/frontend/packages/shared/src/mocks/shape.ts @@ -1,3 +1,5 @@ +import { HEAVY_DTYPES } from '../constants' + import type { Meta, RunData } from './types' // The app's REST surfaces (auth and context file), as BASE_URL-relative path @@ -5,45 +7,68 @@ import type { Meta, RunData } from './types' // mock drift surfaces immediately instead of as a silently broken page. export const REST_API_PREFIXES = ['oauth/', 'contextfile/'] -export function shapeMetadata(meta: Meta) { +// The metadata snapshot for one proposal. `runs` is a list of (proposal, run) +// pairs, server-ordered; the examples are single-proposal, so every pair takes +// the queried proposal. `__typename` rides on every object so Apollo can +// normalize the runs the same way the real server lets it. +export function shapeMetadata(meta: Meta, proposal: string) { return { + __typename: 'TableMeta', variables: meta.variables, - runs: meta.runs, + runs: meta.runs.map((run) => ({ + __typename: 'RunId', + proposal, + run, + })), timestamp: 0, tags: meta.tags, } } -// What the server's @lightweight directive holds back: the dtypes whose values -// are too big to send with the first pass. It nulls their value and leaves -// dtype and error alone, so the client can see which cells to ask for again. -const HEAVY_DTYPES = ['image', 'rgba', 'array'] +// 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( + name: string, + cell: RunData['variables'][string], + { lightweight = false }: { lightweight?: boolean } = {} +) { + return { + __typename: 'Cell', + name, + value: lightweight && HEAVY_DTYPES.has(cell.dtype) ? null : cell.value, + dtype: cell.dtype, + error: 'error' in cell ? { __typename: 'CellError', ...cell.error } : null, + } +} export function shapeTableData( data: RunData[], - { names, lightweight = false }: ShapeTableDataOptions = {} + { 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, variable]) => ({ - name, - value: - lightweight && HEAVY_DTYPES.includes(variable.dtype) - ? null - : variable.value, - dtype: variable.dtype, - // 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. - error: 'error' in variable ? variable.error : null, - })), + .map(([name, cell]) => shapeCell(name, cell, { lightweight })), })), } } type ShapeTableDataOptions = { + proposal: string names?: string[] | null lightweight?: boolean } diff --git a/frontend/packages/ui/package.json b/frontend/packages/ui/package.json index 44a263d8..b4552f9a 100644 --- a/frontend/packages/ui/package.json +++ b/frontend/packages/ui/package.json @@ -41,6 +41,9 @@ "test:browser": "vitest run --project browser", "test:watch": "vitest" }, + "dependencies": { + "@damnit-frontend/shared": "workspace:*" + }, "peerDependencies": { "@apollo/client": "catalog:graphql", "@glideapps/glide-data-grid": "catalog:components", diff --git a/frontend/packages/ui/src/app/store/actions.ts b/frontend/packages/ui/src/app/store/actions.ts index 2adc26d6..71578591 100644 --- a/frontend/packages/ui/src/app/store/actions.ts +++ b/frontend/packages/ui/src/app/store/actions.ts @@ -5,14 +5,3 @@ import { createAction } from '@reduxjs/toolkit' // (which imports the slices back). export const resetProposal = createAction('app/resetProposal') - -// A subscription push must not write its runs once the user has left the -// proposal, and Apollo defers the websocket unsubscribe by a macrotask, so one -// can still arrive. The guard reads only the current proposal, narrowing to -// that shape rather than importing RootState (which would import the reducer -// back). -type ProposalState = { metadata: { proposal: { value: string } } } - -export function isStaleProposal(state: unknown, proposal: string) { - return (state as ProposalState).metadata.proposal.value !== proposal -} diff --git a/frontend/packages/ui/src/app/store/listeners.ts b/frontend/packages/ui/src/app/store/listeners.ts index 45e6174c..5d46471e 100644 --- a/frontend/packages/ui/src/app/store/listeners.ts +++ b/frontend/packages/ui/src/app/store/listeners.ts @@ -51,6 +51,10 @@ export function registerAppListeners() { ? DELETE : value, }) + // 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() }) ) diff --git a/frontend/packages/ui/src/app/store/reducer.ts b/frontend/packages/ui/src/app/store/reducer.ts index beea79f7..568873d8 100644 --- a/frontend/packages/ui/src/app/store/reducer.ts +++ b/frontend/packages/ui/src/app/store/reducer.ts @@ -4,7 +4,6 @@ import { authApi } from '#src/features/auth/auth.api' import contextFile from '#src/features/context-file/context-file.slice' import { contextfileApi } from '#src/features/context-file/context-file.api' import metadata from '#src/data/metadata/metadata.slice' -import tableData from '#src/data/table/table-data.slice' import dashboard from '#src/features/dashboard/dashboard.slice' import plots from '#src/features/plots/plots.slice' import table from '#src/features/table/table.slice' @@ -15,7 +14,6 @@ const reducer = combineReducers({ plots, metadata, table, - tableData, [authApi.reducerPath]: authApi.reducer, [contextfileApi.reducerPath]: contextfileApi.reducer, }) diff --git a/frontend/packages/ui/src/constants.ts b/frontend/packages/ui/src/constants.ts index 0bb4fe66..88cb35f0 100644 --- a/frontend/packages/ui/src/constants.ts +++ b/frontend/packages/ui/src/constants.ts @@ -1,6 +1,29 @@ /// +import { HEAVY_DTYPES } from '@damnit-frontend/shared/constants' + import { formatUrl } from './utils/helpers' +// The dtypes @lightweight holds back on the table's first pass. Shared with the +// 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({ + value, + error, + dtype, +}: { + value: unknown + error: unknown + dtype: string +}): boolean { + return value == null && error == null && HEAVY_DTYPES.has(dtype) +} + export const CONTACT_EMAIL = 'da@xfel.eu' export const BASE_URL = formatUrl(import.meta.env.VITE_BASE_URL) @@ -22,8 +45,21 @@ export const DTYPES = { timestamp: 'timestamp', } -export const EXCLUDED_VARIABLES = ['proposal', 'added_at'] +export const EXCLUDED_VARIABLES = ['added_at'] // `run` identifies the row rather than describing it, so it is never a column // the user configures, and the run detail panel already shows it in the header. export const NONCONFIGURABLE_VARIABLES = [...EXCLUDED_VARIABLES, 'run'] + +// Real, configurable columns the table leaves out of the default view. They +// stay hidden until the user turns them on in the Variables popover. +export const DEFAULT_HIDDEN_VARIABLES = ['proposal'] + +// A variable the user hasn't touched shows by default, unless it is hidden by +// default, in which case it stays hidden until explicitly turned on. +export function isVariableVisible( + name: string, + visibility: Record +) { + return visibility[name] ?? !DEFAULT_HIDDEN_VARIABLES.includes(name) +} diff --git a/frontend/packages/ui/src/data/table/run-stamps.ts b/frontend/packages/ui/src/data/table/run-stamps.ts new file mode 100644 index 00000000..df2bed12 --- /dev/null +++ b/frontend/packages/ui/src/data/table/run-stamps.ts @@ -0,0 +1,37 @@ +import { makeVar } from '@apollo/client' + +import { runKey } from './table-data.transforms' +import type { RunId } from './table-data.types' + +// Flash stamps for runs delivered by a live push, keyed by run identity. Only +// the subscription push handler writes here: bulk loads and deferred fills are +// not updates, so they never flash. Stamps are performance.now() values +// because glide fades the flash against its own performance.now() frame +// clock; an epoch stamp (Date.now() or a server time) reads as decades in the +// future there and paints the row yellow forever. +export const liveRunStamps = makeVar>(new Map()) + +// How long a stamp can still affect a frame. Glide fades the highlight over +// 500ms and ignores anything older, so this only needs to clear that with room +// to spare; past it a stamp is dead weight the next push would copy again. +const FLASH_DURATION = 1000 + +export function stampLiveRuns(runs: RunId[]): void { + if (runs.length === 0) { + return + } + const now = performance.now() + // Rebuilt rather than copied so the map holds the runs of the last second or + // so, not every run stamped since the tab was opened. That also means a + // proposal's stamps expire on their own, without a teardown hook. + const next = new Map() + for (const [key, stamp] of liveRunStamps()) { + if (now - stamp < FLASH_DURATION) { + next.set(key, stamp) + } + } + for (const run of runs) { + next.set(runKey(run), now) + } + liveRunStamps(next) +} diff --git a/frontend/packages/ui/src/data/table/table-data.constants.ts b/frontend/packages/ui/src/data/table/table-data.constants.ts index 314087b6..662f95b5 100644 --- a/frontend/packages/ui/src/data/table/table-data.constants.ts +++ b/frontend/packages/ui/src/data/table/table-data.constants.ts @@ -3,8 +3,6 @@ export { DEFERRED_TABLE_DATA_QUERY_NAME, } from '#src/graphql/operation-names' -export const LATEST_DATA_FIELD_NAME = 'latest_data' - // The page size for asking the server for every run at once, for the readers // that do not follow the table's pagination: the unpaginated table itself, and // the summary plots that chart a variable across the whole proposal. 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 c2503096..e97a7273 100644 --- a/frontend/packages/ui/src/data/table/table-data.queries.ts +++ b/frontend/packages/ui/src/data/table/table-data.queries.ts @@ -2,11 +2,9 @@ import { gql } from '@apollo/client' import { DEFERRED_TABLE_DATA_QUERY_NAME, - LATEST_DATA_FIELD_NAME, TABLE_DATA_QUERY_NAME, } from './table-data.constants' -import { type DamnitRun } from './table-data.transforms' -import { type TableMetadata } from './table-data.types' +import { type Run, type TableMeta } from './table-data.types' /* * ----------------------------- @@ -14,9 +12,54 @@ import { type TableMetadata } 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. +const CELL_FIELDS = ` + name + value + dtype + error { + message + cls + } +` + +// Every run carries its identity trio (database, proposal, run) so Apollo can +// normalize it: the cache keys a run by all three, since run numbers collide +// across proposals in one file. +const RUN_CELLS = ` + database + proposal + run + cells(names: $names) { + ${CELL_FIELDS} + } +` + +// The same run shape without the `names` argument: every cell the run has. The +// subscription and the run-detail fragment both read the whole merged bag. +const RUN_ALL_CELLS = ` + database + proposal + run + cells { + ${CELL_FIELDS} + } +` + +// Reads one normalized run straight from the cache, for the run-detail aside. +export const RUN_FRAGMENT = gql` + fragment RunEntity on DamnitRun { + ${RUN_ALL_CELLS} + } +` + // The three runs documents differ only in operation name and the @lightweight // directive, but each needs its own name: the priority link throttles by -// operation name, and the mock server resolves by it. +// operation name, and the mock server resolves by it. The directive no longer +// splits the cache entry (the field policy keys `runs` by database alone), so +// lightweight, deferred, and pushed rows all merge into one normalized run. const buildTableDataQuery = ( operationName: string, lightweight: boolean @@ -32,15 +75,7 @@ const buildTableDataQuery = ( page: $page per_page: $per_page ) ${lightweight ? '@lightweight' : ''} { - cells(names: $names) { - name - value - dtype - error { - message - cls - } - } + ${RUN_CELLS} } } ` @@ -61,14 +96,14 @@ export const DEFERRED_TABLE_DATA_QUERY = buildTableDataQuery( ) export type TableDataResult = { - runs: DamnitRun[] + runs: Run[] } // Omitting `names` asks the server for every variable. export type TableDataVariables = { proposal: string - page: number - per_page: number + page?: number + per_page?: number names?: string[] } @@ -78,30 +113,66 @@ export type TableDataVariables = { * ----------------------------- */ -export const TABLE_METADATA_QUERY = gql` - query TableMetadataQuery($proposal: String) { - metadata(database: { proposal: $proposal }) +// The metadata selection the query and the subscription share. Both write the +// same `metadata` cache entry, so they must select identical fields; keeping the +// shape in one place stops them drifting (mirrors CELL_FIELDS). `variables` and +// `tags` are JSON scalars, so they take no sub-selection; only `runs` and +// `timestamp` are typed on the wire. +const META_FIELDS = ` + runs { + proposal + run } + variables + tags + timestamp ` -// `metadata` is an opaque JSON scalar, so nothing here is checked against the -// schema. Runs arrive as numbers; the table keys its rows by string. -export type TableMetadataResult = { - metadata: Omit & { runs: number[] } +export const TABLE_META_QUERY = gql` + query TableMetaQuery($proposal: String) { + metadata(database: { proposal: $proposal }) { + ${META_FIELDS} + } + } +` + +export type TableMetaResult = { + metadata: TableMeta } -export type TableMetadataVariables = { +export type TableMetaVariables = { proposal: string } /* * ----------------------------- - * Latest data + * Run updates (subscription) * ----------------------------- */ -export const LATEST_DATA_SUBSCRIPTION = gql` - subscription LatestRunSubcription($proposal: String, $timestamp: Timestamp!) { - ${LATEST_DATA_FIELD_NAME}(database: { proposal: $proposal }, timestamp: $timestamp) +export const RUN_UPDATES_SUBSCRIPTION = gql` + subscription RunUpdates($proposal: String, $since: Timestamp!) { + run_updates(database: { proposal: $proposal }, since: $since) { + runs { + ${RUN_ALL_CELLS} + } + metadata { + ${META_FIELDS} + } + timestamp + } } ` + +export type RunUpdatesResult = { + run_updates: { + runs: Run[] + metadata: TableMeta | null + timestamp: number + } +} + +export type RunUpdatesVariables = { + proposal: string + since: number +} diff --git a/frontend/packages/ui/src/data/table/table-data.selectors.ts b/frontend/packages/ui/src/data/table/table-data.selectors.ts deleted file mode 100644 index 22a1b4e6..00000000 --- a/frontend/packages/ui/src/data/table/table-data.selectors.ts +++ /dev/null @@ -1,13 +0,0 @@ -import type { RootState } from '#src/app/store/types' -import { createTypedSelector } from '#src/app/store/selectors' -import { EXCLUDED_VARIABLES } from '#src/constants' - -const selectTableData = (state: RootState) => state.tableData - -export const selectVariables = createTypedSelector( - [selectTableData], - (tableData) => - Object.values(tableData.metadata.variables).filter( - (variable) => !EXCLUDED_VARIABLES.includes(variable.name) - ) -) diff --git a/frontend/packages/ui/src/data/table/table-data.slice.ts b/frontend/packages/ui/src/data/table/table-data.slice.ts deleted file mode 100644 index 629c8af5..00000000 --- a/frontend/packages/ui/src/data/table/table-data.slice.ts +++ /dev/null @@ -1,102 +0,0 @@ -/* -This is planned to be deprecated in favor of unified Redux and -Apollo Client store -*/ - -import { createSlice, type PayloadAction } from '@reduxjs/toolkit' - -import { resetProposal } from '#src/app/store/actions' -import { type Maybe } from '#src/types' -import { isEmpty } from '#src/utils/helpers' - -import { isBlanked } from './table-data.transforms' -import { - type TableData, - type TableInfo, - type TableMetadata, -} from './table-data.types' - -interface TableDataState extends TableInfo { - lastUpdate: Record> -} - -const initialState: TableDataState = { - data: {}, - metadata: { variables: {}, runs: [], timestamp: 0, tags: {} }, - lastUpdate: {}, -} - -// Metadata is optional: a page loader has only rows to contribute, while -// useProposal's metadata query and the subscription have both. Runs arrive on -// the wire as numbers, so the payload takes that shape and the reducer is what -// turns them into the strings the table keys its rows by. -type UpdateInfo = { - data: TableData - metadata?: Partial> & { - runs?: (string | number)[] - } - // A push from the subscription, as opposed to a bulk load of rows the table - // asked for. Only a push is news: it is what marks the runs as just-updated - // and what may legitimately clear a value. - live?: boolean -} - -const slice = createSlice({ - name: 'tableData', - initialState, - reducers: { - update: (state, action: PayloadAction) => { - const { data, metadata, live = false } = action.payload - - // Update data - if (!isEmpty(data)) { - const timestamp = performance.now() - const updatedData = { ...state.data } - const updatedTimestamp = { ...state.lastUpdate } - - Object.entries(data).forEach(([run, variables]) => { - const row = { ...(updatedData[run] || { run }) } - - Object.entries(variables).forEach(([name, incoming]) => { - // A bulk load must not clobber a value the deferred pass already - // delivered with one @lightweight held back (see isBlanked): that - // would blank the cell for good, since both passes refetch together - // and the deferred one, having nothing new to report, never runs - // again to repair it. - const heldBack = !live && isBlanked(incoming) - if (heldBack && row[name]?.value != null) { - return - } - row[name] = incoming - }) - - updatedData[run] = row - if (live) { - updatedTimestamp[run] = timestamp - } - }) - - state.data = updatedData - state.lastUpdate = updatedTimestamp - } - - // A subscription push resends runs, variables, and timestamp but never - // tags, so replacing wholesale would drop them and crash the tag-driven - // column visibility. Merge so unsent fields (tags) survive. - if (metadata) { - const { runs, ...rest } = metadata - state.metadata = { - ...state.metadata, - ...rest, - ...(runs ? { runs: runs.map(String) } : {}), - } - } - }, - }, - extraReducers: (builder) => { - builder.addCase(resetProposal, () => initialState) - }, -}) - -export default slice.reducer -export const { update: updateTable } = slice.actions 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 d2aa6b56..57a51adb 100644 --- a/frontend/packages/ui/src/data/table/table-data.transforms.ts +++ b/frontend/packages/ui/src/data/table/table-data.transforms.ts @@ -1,40 +1,26 @@ -import type { - Cell, - CellError, - CellValue, - TableData, - Variable, -} from './table-data.types' +import { isHeavyBlank } from '#src/constants' -type DamnitCell = { - name: string - value: CellValue - dtype: string - error?: CellError | null -} - -export type DamnitRun = { - cells: DamnitCell[] -} +import type { Cell, Run, RunCells, RunId, Variable } from './table-data.types' -// A value the server blanked rather than one that is genuinely absent. The -// @lightweight directive nulls heavy values (images, arrays) so a page's rows -// land fast, which leaves them looking like a cell with no value. The one -// tell is that a real failure carries an error, so the errorless blanks are the -// ones the server is still holding back. -export function isBlanked(cell: Cell | undefined): boolean { - return cell?.value == null && cell?.error == 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. +function isDeferred(cell: Cell): boolean { + return isHeavyBlank({ + value: cell.value, + error: cell.error, + dtype: cell.dtype, + }) } -// The variables worth a second, heavier fetch: the ones whose cells -// @lightweight blanked, in any row. -export function heavyVariableNames(data: TableData): string[] { +// The cells worth a second, heavier fetch: the ones @lightweight held back. +export function heavyCellNames(runs: Run[]): string[] { const names = new Set() - for (const row of Object.values(data)) { - for (const [name, cell] of Object.entries(row)) { - if (isBlanked(cell)) { - names.add(name) + for (const run of runs) { + for (const cell of run.cells) { + if (isDeferred(cell)) { + names.add(cell.name) } } } @@ -42,31 +28,44 @@ export function heavyVariableNames(data: TableData): string[] { return [...names] } -// Turn the GraphQL runs payload into the table's run-keyed row map. Each run is -// keyed by its `run` cell's value; a run with no `run` cell, or a null run -// value, is skipped. A cell's null error collapses to undefined. -export function flattenRuns(runs: DamnitRun[]): TableData { - const table: TableData = {} +// The identity a run is looked up by: (proposal, run). The database is constant +// across a table, so the client keys only on the pair, which is enough to keep +// runs that share a number across proposals apart. +export function runKey({ proposal, run }: RunId): string { + return `${proposal}:${run}` +} - for (const run of runs) { - const runCell = run.cells.find((c) => c.name === 'run') - if (runCell === undefined || runCell.value == null) { - continue - } +// One run's cells keyed by variable name, for O(1) lookup. Shared by the grid +// index and the run-detail aside so both key a run's cells the same way. +export function cellsByName(cells: Cell[]): RunCells { + const byName: RunCells = {} + for (const cell of cells) { + byName[cell.name] = cell + } + return byName +} - const row: Record = {} - for (const cell of run.cells) { - row[cell.name] = { - value: cell.value, - dtype: cell.dtype, - error: cell.error ?? undefined, - } - } +// A live push gives a new `runs` array but reuses the object of every unchanged +// run (Apollo's result caching), so this WeakMap hands back that run's +// already-built cell map and only the changed runs are rebuilt. A proposal can +// hold thousands of runs while a push touches a handful. The reuse is within one +// watched query's re-broadcasts: a separate query over the same runs gets its +// own object refs, since Apollo does not canonicalize across queries, so it +// rebuilds its own entries. Entries drop out when Apollo releases the run object. +const cellsByRun = new WeakMap() - table[String(runCell.value)] = row +// Index the runs payload into a per-identity cell map the grid reads by lookup. +export function indexRunCells(runs: Run[]): Map { + const byIdentity = new Map() + for (const run of runs) { + let cells = cellsByRun.get(run) + if (cells === undefined) { + cells = cellsByName(run.cells) + cellsByRun.set(run, cells) + } + byIdentity.set(runKey(run), cells) } - - return table + return byIdentity } export function getVariableTitle(variable: Variable): string { 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 860b26b8..e9bad4f0 100644 --- a/frontend/packages/ui/src/data/table/table-data.types.ts +++ b/frontend/packages/ui/src/data/table/table-data.types.ts @@ -5,10 +5,12 @@ export type CellError = { cls: string } +// One run's value in one variable. `name` ties the cell to its variable. export type Cell = { + name: string value: CellValue dtype: string - error?: CellError + error?: CellError | null } // A column: what a variable is, independent of any run's value of it. @@ -24,18 +26,31 @@ export type Tag = { variables: string[] } -export type TableData = { - [run: string]: { [variable: string]: Cell } +// A run row. Its identity is the (database, proposal, run) trio, which the cache +// keys on: run numbers collide across proposals in one file, so all three are +// needed to key a run. +export type Run = { + database: string + proposal: string + run: number + cells: Cell[] } -export type TableMetadata = { - variables: Record - runs: string[] - timestamp: number - tags: Record +// The (proposal, run) pair the grid lays out and looks up by. The database is +// constant across a table, so the pair keeps runs that share a number across +// proposals apart. +export type RunId = { + proposal: string + run: number } -export type TableInfo = { - data: TableData - metadata: TableMetadata +// One run's cells keyed by variable name, for O(1) cell lookup. +export type RunCells = Record + +// The table's shape: its columns, row order, tags, and freshness. +export type TableMeta = { + variables: Record + runs: RunId[] + tags: Record + timestamp: number } diff --git a/frontend/packages/ui/src/data/table/use-table-meta.ts b/frontend/packages/ui/src/data/table/use-table-meta.ts new file mode 100644 index 00000000..0f4cd0d2 --- /dev/null +++ b/frontend/packages/ui/src/data/table/use-table-meta.ts @@ -0,0 +1,51 @@ +import { useMemo } from 'react' +import { useQuery } from '@apollo/client/react' + +import { useAppSelector } from '#src/app/store/hooks' +import { EXCLUDED_VARIABLES } from '#src/constants' + +import { + TABLE_META_QUERY, + type TableMetaResult, + type TableMetaVariables, +} from './table-data.queries' +import type { TableMeta, Variable } from './table-data.types' + +const EMPTY_META: TableMeta = { + variables: {}, + runs: [], + tags: {}, + timestamp: 0, +} + +// The server metadata's only home is the Apollo cache. useProposal fetches it +// cache-and-network; every other reader shares that entry cache-first, so the +// run layout, columns, and tags all come from one place. +export function useTableMeta(): TableMeta { + const proposal = useAppSelector((state) => state.metadata.proposal.value) + + const { data } = useQuery( + TABLE_META_QUERY, + { + variables: { proposal }, + skip: !proposal, + fetchPolicy: 'cache-first', + } + ) + + return data?.metadata ?? EMPTY_META +} + +// The variables a user configures, in metadata order: the identity and +// bookkeeping columns are never offered. +export function useTableVariables(): Variable[] { + const { variables } = useTableMeta() + + return useMemo( + () => + Object.values(variables).filter( + (variable) => !EXCLUDED_VARIABLES.includes(variable.name) + ), + [variables] + ) +} diff --git a/frontend/packages/ui/src/data/use-proposal.ts b/frontend/packages/ui/src/data/use-proposal.ts index ef0f9eb2..cbdb88f9 100644 --- a/frontend/packages/ui/src/data/use-proposal.ts +++ b/frontend/packages/ui/src/data/use-proposal.ts @@ -1,56 +1,45 @@ -import { useEffect } from 'react' +import { useEffect, useState } from 'react' import { useQuery, useSubscription } from '@apollo/client/react' -import { updateTable } from '#src/data/table/table-data.slice' import { setProposalNotFound, setProposalSuccess, } from '#src/data/metadata/metadata.slice' -import { LATEST_DATA_FIELD_NAME } from '#src/data/table/table-data.constants' +import { stampLiveRuns } from '#src/data/table/run-stamps' import { - LATEST_DATA_SUBSCRIPTION, - TABLE_METADATA_QUERY, - type TableMetadataResult, - type TableMetadataVariables, + RUN_UPDATES_SUBSCRIPTION, + TABLE_DATA_QUERY, + TABLE_META_QUERY, + type RunUpdatesResult, + type RunUpdatesVariables, + type TableMetaResult, + type TableMetaVariables, } from '#src/data/table/table-data.queries' -import { - useAppDispatch, - useAppSelector, - useAppStore, -} from '#src/app/store/hooks' -import { isStaleProposal } from '#src/app/store/actions' +import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' type UseProposalOptions = { subscribe: boolean } const useProposal = ({ subscribe = true }: UseProposalOptions) => { - // Initialize Redux things const proposal = useAppSelector((state) => state.metadata.proposal) - const { timestamp } = useAppSelector((state) => state.tableData.metadata) const dispatch = useAppDispatch() - const store = useAppStore() - useSubscription(LATEST_DATA_SUBSCRIPTION, { - variables: { proposal: proposal.value, timestamp }, - onData: ({ data }) => { - // A push can arrive after the user left this proposal, since Apollo - // defers the unsubscribe. Drop it so it can't write the departed - // proposal's runs into the shared table slice. - if (isStaleProposal(store.getState(), proposal.value)) { - return - } - const { runs, metadata } = data.data[LATEST_DATA_FIELD_NAME] - dispatch(updateTable({ data: runs, metadata, live: true })) - }, - skip: !subscribe || proposal.loading || proposal.notFound, - }) + // The subscription cursor, seeded once from the metadata snapshot and then + // left alone. The server keeps its own high-water mark per proposal, so it + // already bounds each tick to rows it has not shipped; this cursor only has + // to cover the gap between the snapshot and the subscription opening. + // Advancing it on every push would change a subscription variable, which + // tears the subscription down and re-opens it on every run update. + const [since, setSince] = useState(0) - // Synchronize the server and the client table metadata + // Synchronize the server and the client table metadata. The result lands in + // the Apollo cache, which is the only home the server metadata has; this query + // just drives the proposal's navigation state. const { data: metadataResult, error: metadataError } = useQuery< - TableMetadataResult, - TableMetadataVariables - >(TABLE_METADATA_QUERY, { + TableMetaResult, + TableMetaVariables + >(TABLE_META_QUERY, { variables: { proposal: proposal.value }, skip: !proposal.value, fetchPolicy: 'cache-and-network', @@ -67,10 +56,48 @@ const useProposal = ({ subscribe = true }: UseProposalOptions) => { return } - dispatch(updateTable({ data: {}, metadata })) + setSince((current) => (current === 0 ? metadata.timestamp : current)) dispatch(setProposalSuccess()) }, [metadataResult, metadataError, dispatch]) + useSubscription( + RUN_UPDATES_SUBSCRIPTION, + { + variables: { proposal: proposal.value, since }, + skip: + !subscribe || proposal.loading || proposal.notFound || !proposal.value, + onData: ({ data, client }) => { + const update = data.data?.run_updates + if (!update) { + return + } + + // Changed runs merge into the normalized cache: values update in place + // and a finished run joins the list. A stale push for a proposal the + // user just left writes runs keyed by that proposal, which the current + // table never reads, so no guard is needed. + if (update.runs.length) { + client.cache.writeQuery({ + query: TABLE_DATA_QUERY, + variables: { proposal: proposal.value }, + data: { runs: update.runs }, + }) + stampLiveRuns(update.runs) + } + + // Metadata rides along only when the run list, variables, or tags + // changed. Replace it wholesale; it is the run layout's source. + if (update.metadata) { + client.cache.writeQuery({ + query: TABLE_META_QUERY, + variables: { proposal: proposal.value }, + data: { metadata: update.metadata }, + }) + } + }, + } + ) + return proposal } diff --git a/frontend/packages/ui/src/features/dashboard/run.tsx b/frontend/packages/ui/src/features/dashboard/run.tsx index 2175731a..f529e07b 100644 --- a/frontend/packages/ui/src/features/dashboard/run.tsx +++ b/frontend/packages/ui/src/features/dashboard/run.tsx @@ -1,9 +1,21 @@ import { Image, ScrollArea, Text } from '@mantine/core' +import { useFragment } from '@apollo/client/react' import { selectVariableVisibility } from '#src/features/table/store/selectors' -import { DTYPES, NONCONFIGURABLE_VARIABLES } from '#src/constants' +import { + DTYPES, + NONCONFIGURABLE_VARIABLES, + isVariableVisible, +} from '#src/constants' +import { RUN_FRAGMENT } from '#src/data/table/table-data.queries' +import { cellsByName } from '#src/data/table/table-data.transforms' +import { + type CellError, + type CellValue, + type Run as RunEntity, +} from '#src/data/table/table-data.types' +import { useTableMeta } from '#src/data/table/use-table-meta' import { useAppSelector } from '#src/app/store/hooks' -import { type CellValue } from '#src/data/table/table-data.types' import { formatDate, isEmpty } from '#src/utils/helpers' import classes from './run.module.css' @@ -66,6 +78,16 @@ const renderUnknown = ({ name, label }: RenderProps) => ( ) +const renderError = ({ + name, + label, + error, +}: { + name: string + label: string + error: CellError +}) => + const renderFactory = { [DTYPES.image]: renderImage, [DTYPES.string]: renderString, @@ -75,42 +97,73 @@ const renderFactory = { } const Run = () => { - const tableData = useAppSelector((state) => state.tableData.data) - const { run, variables: selectedVariables } = useAppSelector( - (state) => state.table.selection - ) - const metadataVariables = useAppSelector( - (state) => state.tableData.metadata.variables - ) + const proposal = useAppSelector((state) => state.metadata.proposal.value) + const { + proposal: selectedProposal, + run, + variables: selectedVariables, + } = useAppSelector((state) => state.table.selection) + const { runs, variables: metadataVariables } = useTableMeta() const variableVisibility = useAppSelector(selectVariableVisibility) - if (!run || !tableData[run]) { + // Read the normalized run straight from the cache by its identity trio. The + // selection carries (proposal, run); `database` is constant across the table, + // so those two complete the key the cache normalizes on. + const { data: runEntity, complete } = useFragment({ + fragment: RUN_FRAGMENT, + from: { + __typename: 'DamnitRun', + database: proposal, + proposal: selectedProposal ?? '', + run: run ?? -1, + }, + }) + + // Show the selection only while it is still a row of the current proposal's + // table. Run numbers collide across proposals, so both parts must match; a + // stale selection from a proposal the user has left resolves to nothing. + const inCurrentTable = + run != null && + selectedProposal != null && + runs.some( + (entry) => entry.proposal === selectedProposal && entry.run === run + ) + + if (!inCurrentTable || !complete) { return null } + const cells = cellsByName(runEntity.cells ?? []) const runData = isEmpty(selectedVariables) - ? tableData[run] + ? cells : Object.fromEntries( - Object.entries(tableData[run]).filter(([name]) => + Object.entries(cells).filter(([name]) => selectedVariables.includes(name) ) ) const validRuns = Object.entries(runData).filter( ([name, data]) => - variableVisibility[name] !== false && - data?.value != null && + isVariableVisible(name, variableVisibility) && + (data?.error != null || data?.value != null) && !NONCONFIGURABLE_VARIABLES.includes(name) ) return ( {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. + if (data.error) { + return renderError({ name, label, error: data.error }) + } const render = renderFactory[data.dtype] ?? renderFactory.default return render({ name, - label: metadataVariables[name]?.title || name, - value: data.value, + label, + value: data.value as CellValue, }) })} diff --git a/frontend/packages/ui/src/features/plots/plot-container.tsx b/frontend/packages/ui/src/features/plots/plot-container.tsx index 288cbfd7..20023193 100644 --- a/frontend/packages/ui/src/features/plots/plot-container.tsx +++ b/frontend/packages/ui/src/features/plots/plot-container.tsx @@ -2,131 +2,15 @@ import { useMemo, type PropsWithChildren } from 'react' import { Alert, Code, Image, Skeleton, Stack, Text } from '@mantine/core' import { IconInfoCircle } from '@tabler/icons-react' -import { DTYPES } from '#src/constants' -import { createTypedSelector } from '#src/app/store/selectors' import { useAppSelector } from '#src/app/store/hooks' -import { type Cell, type Variable } from '#src/data/table/table-data.types' +import { useTableMeta } from '#src/data/table/use-table-meta' import { formatRunsSubtitle } from '#src/utils/helpers' -import { type PlotTrace, type PlotMeta, type PlotData } from './plots.types' import Plot from './plot' import PreviewChunkLoader from './preview-chunk-loader' import { useSummaryPlotData } from './use-summary-plot-data' import { usePreviewPlotData } from './use-preview-plot-data' -/* - * ----------------------------- - * Table Data - * ----------------------------- - */ - -const selectTableData = createTypedSelector( - [ - (state) => state.tableData.data, - (state) => state.tableData.metadata, - (_, runs) => runs, - (_, __, variables) => variables, - ], - (tableData, tableMetadata, runs: string[], variables: string[]) => { - const result = variables.reduce< - Record - >((acc, variable) => { - acc[variable] = { - data: [], - metadata: tableMetadata.variables[variable], - } - return acc - }, {}) - - for (const run of runs) { - const varData = variables.map((variable) => tableData[run]?.[variable]) - - if ( - varData.every( - (data): data is Cell => - data != null && - typeof data.value === 'number' && - [DTYPES.number].includes(data.dtype) - ) - ) { - variables.forEach((variable, i) => { - result[variable].data.push(varData[i].value as number) - }) - } - } - - return result - } -) - -type UseTableDataOptions = { - runs: string[] - variables: string[] - enabled: boolean -} - -const useTableData = ( - { runs, variables, enabled }: UseTableDataOptions = { - runs: [], - variables: [], - enabled: true, - } -): PlotData | null => { - const tableData = useAppSelector((state) => - selectTableData(state, runs, variables) - ) - - if (!enabled) { - return null - } - - const trace: PlotTrace = {} - const meta: PlotMeta = { type: 'scatter' } - const [xVar, yVar] = variables - - // A variable dropped from the context file leaves the plot that charts it - // open, with no metadata behind it. Falling back to the name is what the - // titleless case already does. - const xName = tableData[xVar].metadata?.title || xVar - trace.x = { - value: tableData[xVar].data, - name: xName, - } - meta.x = { name: xName } - - const yName = tableData[yVar].metadata?.title || yVar - trace.y = { - value: tableData[yVar].data, - name: yName, - } - meta.y = { name: yName } - - return { traces: [trace], meta } -} - -/* - * ----------------------------- - * Other Selectors - * ----------------------------- - */ - -const selectRuns = createTypedSelector( - [ - (state, plotId) => state.plots.data[plotId], - (state) => state.tableData.metadata.runs, - ], - (plot, runsArr) => { - // Use all runs if `plot.runs` is not defined - if (!plot?.runs) { - return runsArr - } - - // Only get existing runs from `plot.runs` - const runsSet = new Set(runsArr) - return plot.runs.filter((run) => runsSet.has(run)) - } -) - /* * ------------------------------------ * UnableToDisplayAlert Component @@ -157,20 +41,36 @@ type PlotContainerProps = { } const PlotContainer = ({ plotId }: PlotContainerProps) => { - const runs = useAppSelector((state) => selectRuns(state, plotId)) const plot = useAppSelector((state) => state.plots.data[plotId]) const proposal = useAppSelector((state) => state.metadata.proposal.value) + const { runs: allRuns } = useTableMeta() const isSummary = plot.source === 'summary' - const runNumbers = useMemo(() => runs.map(Number), [runs]) - useSummaryPlotData({ - variables: plot.variables, - enabled: isSummary, - }) + // The identity pairs this plot charts, in server order. A plot is pinned by + // run number, which is all the run-selection field can express, so a guest + // proposal sharing a number would match twice and plot two points where the + // user asked for one. Restricting to the proposal being viewed picks the one + // they meant; charting a guest run needs identity all the way down to + // `extracted_data`, which previews do not have yet. + const runIds = useMemo(() => { + if (!plot.runs) { + return allRuns + } + const wanted = new Set(plot.runs) + return allRuns.filter( + (run) => run.proposal === proposal && wanted.has(String(run.run)) + ) + }, [plot.runs, allRuns, proposal]) + + const runNumbers = useMemo(() => runIds.map((run) => run.run), [runIds]) + const runLabels = useMemo( + () => runIds.map((run) => String(run.run)), + [runIds] + ) - const tableData = useTableData({ - runs, + const summaryData = useSummaryPlotData({ + runIds, variables: plot.variables, enabled: isSummary, }) @@ -180,7 +80,7 @@ const PlotContainer = ({ plotId }: PlotContainerProps) => { enabled: !isSummary, }) - const { traces, meta } = tableData ?? previewData + const { traces, meta } = summaryData ?? previewData return ( @@ -195,7 +95,7 @@ const PlotContainer = ({ plotId }: PlotContainerProps) => { {plot.title} - {formatRunsSubtitle(runs)} + {formatRunsSubtitle(runLabels)} {!traces.length ? ( diff --git a/frontend/packages/ui/src/features/plots/plot-dialog.tsx b/frontend/packages/ui/src/features/plots/plot-dialog.tsx index 2769af3c..be9f69d9 100644 --- a/frontend/packages/ui/src/features/plots/plot-dialog.tsx +++ b/frontend/packages/ui/src/features/plots/plot-dialog.tsx @@ -14,8 +14,8 @@ import { useForm } from '@mantine/form' import TextCombobox, { type TextComboboxOptions, } from '#src/components/comboboxes/text-combobox' -import { selectVariables } from '#src/data/table/table-data.selectors' -import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' +import { useTableVariables } from '#src/data/table/use-table-meta' +import { useAppDispatch } from '#src/app/store/hooks' import { type PlotSpec } from '#src/types' import { getVariableTitle } from '#src/data/table/table-data.transforms' @@ -38,7 +38,7 @@ type PlotDialogProps = { const PlotDialog = (props: PlotDialogProps) => { const dispatch = useAppDispatch() - const variables = useAppSelector(selectVariables) + const variables = useTableVariables() const dialogForm = useForm({ mode: 'uncontrolled', diff --git a/frontend/packages/ui/src/features/plots/plots-tab.tsx b/frontend/packages/ui/src/features/plots/plots-tab.tsx index ce4216d0..e503ac65 100644 --- a/frontend/packages/ui/src/features/plots/plots-tab.tsx +++ b/frontend/packages/ui/src/features/plots/plots-tab.tsx @@ -2,6 +2,7 @@ import { Box, Stack, Text } from '@mantine/core' import Tabs from '#src/components/tabs/tabs' import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' +import { useTableMeta } from '#src/data/table/use-table-meta' import { sorted } from '#src/utils/array' import { formatRunsSubtitle, isEmpty } from '#src/utils/helpers' @@ -12,7 +13,8 @@ const PlotsTab = () => { const dispatch = useAppDispatch() const plots = useAppSelector((state) => state.plots) - const runs = useAppSelector((state) => state.tableData.metadata.runs) + const { runs: runIds } = useTableMeta() + const runs = runIds.map((run) => String(run.run)) const contents = Object.fromEntries( Object.entries(plots.data).map(([id, plot]) => { 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 ad7b447c..dd63c2a6 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 @@ -1,32 +1,42 @@ -import { useEffect } from 'react' +import { useMemo } from 'react' import { useQuery } from '@apollo/client/react' -import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' -import { VARIABLES } from '#src/constants' +import { useAppSelector } from '#src/app/store/hooks' +import { DTYPES, VARIABLES } from '#src/constants' import { ALL_RUNS_PAGE_SIZE } from '#src/data/table/table-data.constants' import { TABLE_DATA_QUERY, type TableDataResult, type TableDataVariables, } from '#src/data/table/table-data.queries' -import { updateTable } from '#src/data/table/table-data.slice' -import { flattenRuns } from '#src/data/table/table-data.transforms' +import { 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' + +import type { PlotData, PlotMeta, PlotTrace } from './plots.types' type UseSummaryPlotDataOptions = { + runIds: RunId[] variables: string[] enabled: boolean } -// Fetches the variables a summary plot charts into the table slice, which is -// where the plot reads them back from. Routing through the slice rather than -// rendering from the cache is what keeps summary plots live: a subscription -// push writes the same rows, and the plot redraws with them. +// Summary plots chart a variable across every run in the proposal. The values +// come from the same normalized cache the table fills (`Query.runs` is keyed by +// database alone, so this shares the table's entry), but the paginated table +// only caches the pages scrolled to, so this pulls the full run set itself +// (per_page = ALL_RUNS_PAGE_SIZE, cache-and-network). cache-first would chart +// only the runs already scrolled into cache, so it stays network-backed; the +// merge policy returning a stable list on value-only pushes is what keeps this +// from re-running on every cache write. A run is charted only when every +// variable it plots is a real number. export function useSummaryPlotData({ + runIds, variables, enabled, -}: UseSummaryPlotDataOptions) { - const dispatch = useAppDispatch() +}: UseSummaryPlotDataOptions): PlotData | null { const proposal = useAppSelector((state) => state.metadata.proposal.value) + const { variables: variableMeta } = useTableMeta() const { data } = useQuery( TABLE_DATA_QUERY, @@ -35,23 +45,55 @@ export function useSummaryPlotData({ proposal, page: 1, per_page: ALL_RUNS_PAGE_SIZE, - // `run` keys each row onto the ones the table already holds. + // `run` keys each row; the rest are what the plot charts. names: [VARIABLES.run, ...variables], }, - // The slice is the only thing that renders these rows, so a cached copy - // is never read: caching one would only cost a write, and let this - // query's entry replace the unpaginated table's, which is stored under - // the same arguments. Once a keyed run lets the plot render from the - // cache, this becomes cache-and-network and the slice goes away. - fetchPolicy: 'no-cache', + fetchPolicy: 'cache-and-network', skip: !enabled || !proposal, } ) - useEffect(() => { - if (!data) { - return + return useMemo(() => { + if (!enabled) { + return null } - dispatch(updateTable({ data: flattenRuns(data.runs) })) - }, [data, dispatch]) + + const cells = indexRunCells(data?.runs ?? []) + const series = variables.map(() => [] as number[]) + + for (const id of runIds) { + const row = cells.get(runKey(id)) + const points = variables.map((name) => row?.[name]) + const allNumeric = points.every( + (point) => + point != null && + typeof point.value === 'number' && + point.dtype === DTYPES.number + ) + if (allNumeric) { + points.forEach((point, index) => { + series[index].push(point!.value as number) + }) + } + } + + const [xVar, yVar] = variables + // A variable dropped from the context file leaves the plot that charts it + // open, with no metadata behind it. Falling back to the name is what the + // titleless case already does. + const xName = variableMeta[xVar]?.title || xVar + const yName = variableMeta[yVar]?.title || yVar + + const trace: PlotTrace = { + x: { value: series[0], name: xName }, + y: { value: series[1], name: yName }, + } + const meta: PlotMeta = { + type: 'scatter', + x: { name: xName }, + y: { name: yName }, + } + + return { traces: [trace], meta } + }, [enabled, data, runIds, variables, variableMeta]) } diff --git a/frontend/packages/ui/src/features/table/cells.ts b/frontend/packages/ui/src/features/table/cells.ts index 1a96a0e6..bb0e6d66 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 } from '#src/constants' +import { DTYPES, HEAVY_DTYPES } from '#src/constants' import { type CellError, type CellValue, @@ -289,7 +289,14 @@ export const getCell = ({ dtype, options, }: GetCellOptions): GridCell => { - // If the value is null or undefined, use the loading cell factory. - const factory = value == null ? loadingCell : gridCellFactory[dtype] - return factory(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) } diff --git a/frontend/packages/ui/src/features/table/components/popovers/tags-popover.tsx b/frontend/packages/ui/src/features/table/components/popovers/tags-popover.tsx index 2724527f..d8e656dc 100644 --- a/frontend/packages/ui/src/features/table/components/popovers/tags-popover.tsx +++ b/frontend/packages/ui/src/features/table/components/popovers/tags-popover.tsx @@ -4,6 +4,7 @@ import lodashSize from 'lodash/size' import { Anchor, rem } from '@mantine/core' import { IconEye, IconEyeClosed, IconHash } from '@tabler/icons-react' +import { useTableMeta } from '#src/data/table/use-table-meta' import { ControlButton } from '#src/features/table/components/control-button' import { NONCONFIGURABLE_VARIABLES } from '#src/constants' import { useColumnVisibility } from '#src/features/table/hooks/use-column-visibility' @@ -28,9 +29,7 @@ type TagDetailProps = { } function TagDetail({ name }: TagDetailProps) { - const { variables, tags } = useAppSelector( - (state) => state.tableData.metadata - ) + const { variables, tags } = useTableMeta() const columnVisibility = useColumnVisibility() const items = tags[name].variables @@ -78,7 +77,7 @@ function TagDetail({ name }: TagDetailProps) { export function TagsTable() { const dispatch = useAppDispatch() const selection = useAppSelector(selectTagSelection) - const tags = useAppSelector((state) => state.tableData.metadata.tags) + const { tags } = useTableMeta() const records = useMemo(() => { return Object.keys(tags) diff --git a/frontend/packages/ui/src/features/table/components/popovers/variables-popover.tsx b/frontend/packages/ui/src/features/table/components/popovers/variables-popover.tsx index 9c164e27..7ed607a6 100644 --- a/frontend/packages/ui/src/features/table/components/popovers/variables-popover.tsx +++ b/frontend/packages/ui/src/features/table/components/popovers/variables-popover.tsx @@ -3,6 +3,7 @@ import lodashSize from 'lodash/size' import { Checkbox, rem } from '@mantine/core' import { IconCheck, IconList, IconCircle } from '@tabler/icons-react' +import { useTableMeta } from '#src/data/table/use-table-meta' import { ControlButton } from '#src/features/table/components/control-button' import { useColumnVisibilityFromTags, @@ -23,7 +24,7 @@ type VariableDetailsProps = { } function VariableDetails({ name }: VariableDetailsProps) { - const metadata = useAppSelector((state) => state.tableData.metadata.variables) + const { variables: metadata } = useTableMeta() const tagSelection = useAppSelector(selectTagSelection) const items = metadata[name].tags.map((tagName) => ({ @@ -67,7 +68,7 @@ type VariableRecord = { function VariablesTable() { const dispatch = useAppDispatch() - const metadata = useAppSelector((state) => state.tableData.metadata.variables) + const { variables: metadata } = useTableMeta() const visibilityFromVariables = useColumnVisibilityFromVariables() const visibilityFromTags = useColumnVisibilityFromTags() diff --git a/frontend/packages/ui/src/features/table/hooks/use-column-visibility.ts b/frontend/packages/ui/src/features/table/hooks/use-column-visibility.ts index 2d73119e..72be37e0 100644 --- a/frontend/packages/ui/src/features/table/hooks/use-column-visibility.ts +++ b/frontend/packages/ui/src/features/table/hooks/use-column-visibility.ts @@ -1,7 +1,8 @@ import { useMemo } from 'react' import type { Tag } from '#src/data/table/table-data.types' -import { NONCONFIGURABLE_VARIABLES } from '#src/constants' +import { useTableMeta } from '#src/data/table/use-table-meta' +import { isVariableVisible, NONCONFIGURABLE_VARIABLES } from '#src/constants' import { selectTagSelection, selectVariableVisibility, @@ -15,20 +16,20 @@ type ColumnVisibilityInputs = { tagSelection: Record } -// Columns the user can't hide (proposal, added_at, run) never appear here. +// Columns the user can't hide (added_at, run) never appear here. function configurableVariables(variableNames: string[]) { return variableNames.filter( (name) => !NONCONFIGURABLE_VARIABLES.includes(name) ) } -// A variable is visible unless it was explicitly turned off. +// The visibility map for the configurable columns. function visibilityFromVariables( configurable: string[], visibility: ColumnVisibilityInputs['visibility'] ) { return Object.fromEntries( - configurable.map((name) => [name, visibility[name] !== false]) + configurable.map((name) => [name, isVariableVisible(name, visibility)]) ) } @@ -69,10 +70,7 @@ export function computeColumnVisibility(inputs: ColumnVisibilityInputs) { } function useVariableNames() { - // TODO: Replace with GraphQL useQuery - const variables = useAppSelector( - (state) => state.tableData.metadata.variables - ) + const { variables } = useTableMeta() return useMemo(() => Object.keys(variables), [variables]) } @@ -88,8 +86,7 @@ export function useColumnVisibilityFromVariables() { } export function useColumnVisibilityFromTags() { - // TODO: Replace with GraphQL useQuery - const tags = useAppSelector((state) => state.tableData.metadata.tags) + const { tags } = useTableMeta() const tagSelection = useAppSelector(selectTagSelection) const variableNames = useVariableNames() @@ -106,7 +103,7 @@ export function useColumnVisibilityFromTags() { export function useColumnVisibility() { const variableNames = useVariableNames() const visibility = useAppSelector(selectVariableVisibility) - const tags = useAppSelector((state) => state.tableData.metadata.tags) + const { tags } = useTableMeta() const tagSelection = useAppSelector(selectTagSelection) return useMemo( diff --git a/frontend/packages/ui/src/features/table/pagination.ts b/frontend/packages/ui/src/features/table/pagination.ts index 1f0ab233..b8da3005 100644 --- a/frontend/packages/ui/src/features/table/pagination.ts +++ b/frontend/packages/ui/src/features/table/pagination.ts @@ -1,11 +1,11 @@ import { range } from '@mantine/hooks' -import { isArrayEqual } from '#src/utils/array' - import type { Rectangle } from './types' // The pages to ensure-loaded for a scroll window, padding half a page on each -// side so rows just outside the viewport are ready before they scroll in. +// side so rows just outside the viewport are ready before they scroll in. The +// runs hook fetches each page once and the field policy accumulates them, so a +// page that has scrolled off stays loaded. export function pageRangeForRegion(region: Rectangle, pageSize: number) { const firstPage = Math.max( 0, @@ -16,18 +16,3 @@ export function pageRangeForRegion(region: Rectangle, pageSize: number) { ) return range(firstPage + 1, lastPage + 2) } - -// The pages the table wants loaded: exactly the current window, nothing more. -// Rows keep rendering once scrolled away from because they live in the table -// slice, which only a proposal switch clears, so a page needs a mounted loader -// only while it is on screen. Returns the array unchanged when the window is -// the same, so React bails out of the update rather than rebuilding the loader -// list on every scroll event. -export function pagesForRegion( - current: number[], - region: Rectangle, - pageSize: number -): number[] { - const next = pageRangeForRegion(region, pageSize) - return isArrayEqual(current, next) ? current : next -} diff --git a/frontend/packages/ui/src/features/table/table-page-loader.tsx b/frontend/packages/ui/src/features/table/table-page-loader.tsx deleted file mode 100644 index 3c74846f..00000000 --- a/frontend/packages/ui/src/features/table/table-page-loader.tsx +++ /dev/null @@ -1,83 +0,0 @@ -import { useEffect, useMemo } from 'react' -import { useQuery } from '@apollo/client/react' - -import { useAppDispatch } from '#src/app/store/hooks' -import { VARIABLES } from '#src/constants' -import { - DEFERRED_TABLE_DATA_QUERY, - LIGHTWEIGHT_TABLE_DATA_QUERY, - type TableDataResult, - type TableDataVariables, -} from '#src/data/table/table-data.queries' -import { updateTable } from '#src/data/table/table-data.slice' -import { - flattenRuns, - heavyVariableNames, -} from '#src/data/table/table-data.transforms' - -type TablePageLoaderProps = { - proposal: string - page: number - pageSize: number -} - -// Loads one page of runs into the table slice and renders nothing. The table -// wants a changing number of pages at once and a component can own only one -// watched query, so each page gets its own instance. Rows live in the slice, -// not in this null-rendering loader, so a revisited page has nothing to paint -// from a cached copy. Fetching it fresh (no-cache) is what stops a stale page -// from replaying over the newer values the subscription writes to the slice -// while the page is off screen. -function TablePageLoader({ proposal, page, pageSize }: TablePageLoaderProps) { - const dispatch = useAppDispatch() - - const { data: lightweight } = useQuery( - LIGHTWEIGHT_TABLE_DATA_QUERY, - { - variables: { proposal, page, per_page: pageSize }, - fetchPolicy: 'no-cache', - skip: !proposal, - } - ) - - const rows = useMemo( - () => (lightweight ? flattenRuns(lightweight.runs) : undefined), - [lightweight] - ) - - useEffect(() => { - if (rows === undefined) { - return - } - dispatch(updateTable({ data: rows })) - }, [rows, dispatch]) - - // Ask for the values @lightweight held back, for this page only. `run` comes - // along because it is what keys each row onto the ones already dispatched. - const heavy = useMemo(() => (rows ? heavyVariableNames(rows) : []), [rows]) - - const { data: deferred } = useQuery( - DEFERRED_TABLE_DATA_QUERY, - { - variables: { - proposal, - page, - per_page: pageSize, - names: [VARIABLES.run, ...heavy], - }, - fetchPolicy: 'no-cache', - skip: !heavy.length, - } - ) - - useEffect(() => { - if (!deferred) { - return - } - dispatch(updateTable({ data: flattenRuns(deferred.runs) })) - }, [deferred, dispatch]) - - return null -} - -export default TablePageLoader diff --git a/frontend/packages/ui/src/features/table/table.slice.ts b/frontend/packages/ui/src/features/table/table.slice.ts index c9697508..9c2a38f3 100644 --- a/frontend/packages/ui/src/features/table/table.slice.ts +++ b/frontend/packages/ui/src/features/table/table.slice.ts @@ -16,6 +16,7 @@ type TagSettings = { type TableState = { selection: { + proposal: string | null run: number | null variables: string[] } @@ -28,7 +29,7 @@ type TableState = { } const initialState: TableState = { - selection: { run: null, variables: [] }, + selection: { proposal: null, run: null, variables: [] }, variables: {}, tags: {}, view: { scroll: { x: 0, y: 0 } }, @@ -43,10 +44,13 @@ const slice = createSlice({ state.isActive = action.payload }, selectRun: ({ selection }, action) => { - const { run, variables } = action.payload + const { proposal, run, variables } = action.payload if (selection.run !== run) { selection.run = run } + if (selection.proposal !== proposal) { + selection.proposal = proposal + } if (!isArrayEqual(selection.variables, variables)) { selection.variables = variables } diff --git a/frontend/packages/ui/src/features/table/table.tsx b/frontend/packages/ui/src/features/table/table.tsx index 531f1047..257dfd3d 100644 --- a/frontend/packages/ui/src/features/table/table.tsx +++ b/frontend/packages/ui/src/features/table/table.tsx @@ -13,9 +13,10 @@ import { import { allCells } from '@glideapps/glide-data-grid-cells' import { Group, Stack, useMantineTheme } from '@mantine/core' -import { DTYPES, EXCLUDED_VARIABLES, VARIABLES } from '#src/constants' +import { DTYPES, VARIABLES } from '#src/constants' import { useAppDispatch, useAppSelector } from '#src/app/store/hooks' -import { ALL_RUNS_PAGE_SIZE } from '#src/data/table/table-data.constants' +import { 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' @@ -33,9 +34,8 @@ import { type CellTooltip } from './components/tooltips/table-tooltip' import ContextMenu from './context-menu' import { useTableTooltip } from './hooks/use-table-tooltip' import { useTable } from './hooks/use-table' -import TablePageLoader from './table-page-loader' +import { useTableRuns } from './use-table-runs' import { useContextMenu } from './use-context-menu' -import { usePagination } from './use-pagination' import { useScrollToView } from './use-scroll-to-view' import { plotRequested, selectRun } from './table.slice' @@ -62,20 +62,22 @@ const Table = ({ grid, paginated = true }: TableProps) => { // Initialization: References const tableRef = useRef(null) - // Initialization: Selectors + // Initialization: Data sources (the Apollo cache, via hooks) const proposal = useAppSelector((state) => state.metadata.proposal.value) + const { runs } = useTableMeta() + const tableVariables = useTableVariables() const { - data: tableData, - metadata: tableMetadata, - lastUpdate: tableLastUpdate, - } = useAppSelector((state) => state.tableData) + cellsByKey, + lastUpdatedByKey, + onVisibleRegionChanged: fetchOnScroll, + } = useTableRuns({ + proposal, + paginated, + pageSize: PAGE_SIZE, + }) // Initialization: Hooks const dispatch = useAppDispatch() - const { pages, onVisibleRegionChanged: paginationHandler } = usePagination({ - enabled: paginated, - pageSize: PAGE_SIZE, - }) const { onVisibleRegionChanged: scrollToViewHandler, scrollX, @@ -88,14 +90,10 @@ const Table = ({ grid, paginated = true }: TableProps) => { // Initialization: Memos const tableColumns = useMemo( () => - Object.values(tableMetadata.variables) - .filter( - ({ name }) => - !EXCLUDED_VARIABLES.includes(name) && - columnVisibility[name] !== false - ) + tableVariables + .filter(({ name }) => columnVisibility[name] !== false) .map(({ name, title }) => ({ id: name, title: title || name })), - [tableMetadata.variables, columnVisibility] + [tableVariables, columnVisibility] ) // Error-glyph colors resolved from the live theme; dark mode plugs in here. @@ -112,44 +110,49 @@ const Table = ({ grid, paginated = true }: TableProps) => { [errorColors] ) - // Data: Populate grid + // Data: Populate grid. Row layout is the server-ordered run list; a cell's + // value is looked up by the run's identity from the normalized cache. const getContent = useCallback( ([col, row]: Item) => { - const run = tableMetadata.runs[row] + const identity = runs[row] const variable = tableColumns[col]?.id if (variable === VARIABLES.run) { - return numberCell(run) + return numberCell(identity?.run) } - const rowData = tableData[run] - if (!rowData || !rowData[variable]) { + if (!identity) { return textCell('') } - const cellError = rowData[variable].error - if (cellError) { - return errorCell(cellError) + const key = runKey(identity) + const cell = cellsByKey.get(key)?.[variable] + if (!cell) { + return textCell('') + } + + if (cell.error) { + return errorCell(cell.error) } return getCell({ - value: rowData[variable].value, - dtype: rowData[variable].dtype, - options: { lastUpdated: tableLastUpdate[run] }, + value: cell.value, + dtype: cell.dtype, + options: { lastUpdated: lastUpdatedByKey.get(key) }, }) }, - [tableColumns, tableMetadata.runs, tableData, tableLastUpdate] + [tableColumns, runs, cellsByKey, lastUpdatedByKey] ) // Cell: tooltip. Errored cells show a card; image cells show a preview. const resolveTooltip = useCallback( (col: number, row: number): CellTooltip | undefined => { - const run = tableMetadata.runs[row] + const identity = runs[row] const variable = tableColumns[col]?.id - if (run == null || !variable) { + if (identity == null || !variable) { return undefined } - const item = tableData[run]?.[variable] + const item = cellsByKey.get(runKey(identity))?.[variable] if (!item) { return undefined } @@ -165,7 +168,7 @@ const Table = ({ grid, paginated = true }: TableProps) => { } return undefined }, - [tableColumns, tableMetadata.runs, tableData] + [tableColumns, runs, cellsByKey] ) const { onItemHovered: handleItemHovered, @@ -182,13 +185,16 @@ const Table = ({ grid, paginated = true }: TableProps) => { const handleGridSelectionChange = (newSelection: GridSelection) => { const { columns, rows, current } = newSelection - // Inform that a row has been (de)selected + // Inform that a row has been (de)selected. The proposal rides along: run + // numbers collide across proposals in one table, so the number alone cannot + // identify which run the detail aside should read. const row = rows.last() as number - const run = tableMetadata.runs[row] + const identity = runs[row] dispatch( selectRun({ - run, + proposal: identity?.proposal ?? null, + run: identity?.run ?? null, }) ) @@ -214,11 +220,12 @@ const Table = ({ grid, paginated = true }: TableProps) => { } const handleCellActivated = (cell: Item) => { const [col, row] = cell - const run = tableMetadata.runs[row] + const identity = runs[row] dispatch( selectRun({ - run: run, + proposal: identity?.proposal ?? null, + run: identity?.run ?? null, variables: col == null ? null : [tableColumns[col].id], }) ) @@ -254,7 +261,8 @@ const Table = ({ grid, paginated = true }: TableProps) => { } const column = tableColumns[col]?.id - const rowData = tableData[tableMetadata.runs[row]] + const identity = runs[row] + 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. @@ -263,8 +271,13 @@ const Table = ({ grid, paginated = true }: TableProps) => { const variable = tableColumns[col] const subtitle = `${variable.title}` - const rows = [selectedCell[1], ...selectedRange.map((rect) => rect.y)] - const runs = sorted(rows.map((row) => tableMetadata.runs[row as number])) + const selectedRows = [ + selectedCell[1], + ...selectedRange.map((rect) => rect.y), + ] + const runNumbers = sorted( + selectedRows.map((row) => String(runs[row as number]?.run)) + ) setContextMenu({ localPosition: { x: event.localEventX, y: event.localEventY }, @@ -275,7 +288,11 @@ const Table = ({ grid, paginated = true }: TableProps) => { title: 'Plot: preview', subtitle, onClick: () => - addPreviewPlot({ variable: variable.id, label: subtitle, runs }), + addPreviewPlot({ + variable: variable.id, + label: subtitle, + runs: runNumbers, + }), }, ], }) @@ -387,7 +404,7 @@ const Table = ({ grid, paginated = true }: TableProps) => { } | null>(null) const handleVisibleRegionChange = useCallback( (rect: Rectangle, tx?: number, ty?: number) => { - paginationHandler(rect) + fetchOnScroll(rect) scrollToViewHandler(rect) const previous = lastVisibleRegionRef.current const nextTx = tx ?? 0 @@ -404,27 +421,11 @@ const Table = ({ grid, paginated = true }: TableProps) => { dismissTooltipOnScroll() } }, - [paginationHandler, scrollToViewHandler, dismissTooltipOnScroll] + [fetchOnScroll, scrollToViewHandler, dismissTooltipOnScroll] ) return ( <> - {paginated ? ( - pages.map((page) => ( - - )) - ) : ( - - )} {!tableColumns.length ? null : ( @@ -437,7 +438,7 @@ const Table = ({ grid, paginated = true }: TableProps) => { ref={tableRef} columns={formatColumns(tableColumns)} getCellContent={getContent} - rows={tableMetadata.runs.length} + rows={runs.length} rowSelect="single" rowMarkers="clickable-number" gridSelection={gridSelection} diff --git a/frontend/packages/ui/src/features/table/use-pagination.ts b/frontend/packages/ui/src/features/table/use-pagination.ts deleted file mode 100644 index b204176c..00000000 --- a/frontend/packages/ui/src/features/table/use-pagination.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { useCallback, useState } from 'react' - -import { pagesForRegion } from './pagination' -import type { Rectangle } from './types' - -type UsePaginationOptions = { - enabled?: boolean - pageSize?: number -} - -// Decides which pages the table wants loaded as it scrolls; rendering a -// TablePageLoader per page is what fetches them. Switching proposals remounts -// the whole subtree (see ProposalRoute's key), so the wanted set never needs -// clearing here. -export const usePagination = ({ - enabled = true, - pageSize = 10, -}: UsePaginationOptions = {}) => { - const [pages, setPages] = useState([]) - - const onVisibleRegionChanged = useCallback( - (region: Rectangle) => { - if (!enabled) { - return - } - // The grid reports a zero-size region before it has laid out. That - // measures as page 1 alone, which would drop the window to a single page - // and unmount the loaders for everything actually on screen. - if (region.width === 0 || region.height === 0) { - return - } - setPages((current) => pagesForRegion(current, region, pageSize)) - }, - [enabled, pageSize] - ) - - return { pages, onVisibleRegionChanged } -} diff --git a/frontend/packages/ui/src/features/table/use-table-runs.ts b/frontend/packages/ui/src/features/table/use-table-runs.ts new file mode 100644 index 00000000..c93a4baa --- /dev/null +++ b/frontend/packages/ui/src/features/table/use-table-runs.ts @@ -0,0 +1,200 @@ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useApolloClient, useQuery, useReactiveVar } from '@apollo/client/react' +import { debounce } from 'lodash' + +import { VARIABLES } from '#src/constants' +import { liveRunStamps } from '#src/data/table/run-stamps' +import { ALL_RUNS_PAGE_SIZE } from '#src/data/table/table-data.constants' +import { + DEFERRED_TABLE_DATA_QUERY, + LIGHTWEIGHT_TABLE_DATA_QUERY, + type TableDataResult, + type TableDataVariables, +} from '#src/data/table/table-data.queries' +import { + heavyCellNames, + indexRunCells, +} from '#src/data/table/table-data.transforms' +import type { Run, RunCells } from '#src/data/table/table-data.types' + +import { pageRangeForRegion } from './pagination' +import type { Rectangle } from './types' + +type UseTableRunsOptions = { + proposal: string + paginated: boolean + pageSize: number +} + +type UseTableRuns = { + cellsByKey: Map + lastUpdatedByKey: ReadonlyMap + onVisibleRegionChanged: (region: Rectangle) => void +} + +type Subscription = { unsubscribe: () => void } + +// One watched query is the render source: it fetches page 1, and `fetchMore` +// appends the pages the user scrolls to. The field policy dedups every page, +// pushed run, and deferred fill into one normalized run, so the grid reads a +// run's cells by identity from `cellsByKey`. Switching proposals remounts this +// hook (ProposalWrapper is keyed), so the fetched-page bookkeeping resets with +// it. +export function useTableRuns({ + proposal, + paginated, + pageSize, +}: UseTableRunsOptions): UseTableRuns { + const perPage = paginated ? pageSize : ALL_RUNS_PAGE_SIZE + const client = useApolloClient() + const fetchedPages = useRef(new Set([1])) + const deferredPages = useRef(new Set()) + const deferredSubs = useRef(new Map()) + + const { data, fetchMore } = useQuery( + LIGHTWEIGHT_TABLE_DATA_QUERY, + { + variables: { proposal, page: 1, per_page: perPage }, + fetchPolicy: 'cache-and-network', + skip: !proposal, + } + ) + + const runs = data?.runs + + // Ask for the values @lightweight held back on a page, once. `run` rides along + // so a deferred row normalizes onto the identity the lightweight pass wrote. + // + // Held as a subscription rather than issued through `client.query`, which + // cannot be cancelled: this fetch carries the heavy values, so one still in + // flight when the user leaves the proposal would land after the teardown + // eviction and write those runs, images and all, back into the cache the + // eviction just reclaimed. + const fetchDeferred = useCallback( + (page: number, pageRuns: Run[]) => { + if (deferredPages.current.has(page)) { + return + } + const heavy = heavyCellNames(pageRuns) + if (!heavy.length) { + return + } + deferredPages.current.add(page) + const subscription = client + .watchQuery({ + query: DEFERRED_TABLE_DATA_QUERY, + variables: { + proposal, + page, + per_page: perPage, + names: [VARIABLES.run, ...heavy], + }, + fetchPolicy: 'network-only', + }) + .subscribe({ + next: () => { + // The result is normalized into the cache by the time this fires and + // the watched query repaints from it. Unsubscribe to keep the fetch + // one-shot rather than go on watching. `network-only` never resolves + // synchronously, so `subscription` is always assigned before this + // runs; the self-reference is safe. + deferredSubs.current.delete(page) + subscription.unsubscribe() + }, + error: () => { + // Unmark the page so a later pass can retry it, rather than leaving + // its heavy cells blank for the rest of the session. + deferredPages.current.delete(page) + deferredSubs.current.delete(page) + }, + }) + deferredSubs.current.set(page, subscription) + }, + [client, proposal, perPage] + ) + + useEffect(() => { + const subscriptions = deferredSubs.current + return () => { + for (const subscription of subscriptions.values()) { + subscription.unsubscribe() + } + subscriptions.clear() + } + }, []) + + // Page 1 arrives on the watched query, so its deferred pass rides its result. + useEffect(() => { + if (runs === undefined) { + return + } + fetchDeferred(1, runs) + }, [runs, fetchDeferred]) + + // The grid reports a visible region on every scroll frame, and a page this + // ever sees is fetched and kept forever. Settling first means dragging the + // scrollbar across a large table costs a handful of requests rather than one + // per frame; `maxWait` keeps a continuous slow scroll loading rather than + // waiting for a pause that never comes. Only the settled region is state: + // the fetching itself reads a ref, which a render-built closure may not. + const [settledRegion, setSettledRegion] = useState(null) + const settleRegion = useMemo( + () => debounce(setSettledRegion, 150, { maxWait: 500 }), + [] + ) + + useEffect(() => { + return () => { + settleRegion.cancel() + } + }, [settleRegion]) + + useEffect(() => { + if (settledRegion === null) { + return + } + for (const page of pageRangeForRegion(settledRegion, pageSize)) { + if (fetchedPages.current.has(page)) { + continue + } + // Marked before the request so a later settle does not ask again, and + // unmarked on failure so the page can be retried instead of staying blank + // for the rest of the session. + fetchedPages.current.add(page) + void fetchMore({ variables: { page } }) + .then((result) => { + if (result.data) { + fetchDeferred(page, result.data.runs) + } + }) + .catch(() => { + fetchedPages.current.delete(page) + }) + } + }, [settledRegion, pageSize, fetchMore, fetchDeferred]) + + const onVisibleRegionChanged = useCallback( + (region: Rectangle) => { + if (!paginated) { + return + } + // The grid reports a zero-size region before it has laid out; treat it as + // no window rather than fetching page 1 twice. + if (region.width === 0 || region.height === 0) { + return + } + settleRegion(region) + }, + [paginated, settleRegion] + ) + + const cellsByKey = useMemo(() => indexRunCells(runs ?? []), [runs]) + + // Per-run flash stamps for the grid's update highlight, written where the + // subscription push lands. Loads and deferred fills never stamp, so only a + // live push animates; a run with no stamp reads undefined and glide skips + // the flash entirely. + const lastUpdatedByKey = useReactiveVar(liveRunStamps) + + return { cellsByKey, lastUpdatedByKey, onVisibleRegionChanged } +} diff --git a/frontend/packages/ui/src/graphql/apollo.ts b/frontend/packages/ui/src/graphql/apollo.ts index 77f92c68..b293a9f7 100644 --- a/frontend/packages/ui/src/graphql/apollo.ts +++ b/frontend/packages/ui/src/graphql/apollo.ts @@ -18,6 +18,7 @@ import { BASE_URL, WS_URL } from '#src/constants' import { DEFERRED_TABLE_DATA_QUERY_NAME } from './operation-names' import { createPriorityLink } from './priority-link' +import { typePolicies } from './type-policies' const removeTypenameLink = removeTypenameFromVariables({ except: { @@ -60,7 +61,7 @@ const splitLink = split( from([priorityLink, httpLink]) ) -export const cache = new InMemoryCache() +export const cache = new InMemoryCache({ typePolicies }) 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 new file mode 100644 index 00000000..50157361 --- /dev/null +++ b/frontend/packages/ui/src/graphql/type-policies.ts @@ -0,0 +1,112 @@ +import type { + FieldFunctionOptions, + Reference, + StoreObject, + 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()] +} + +// 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 +// already present, so keeping the field's reference lets Apollo skip a needless +// re-broadcast (and the Map rebuild that rides on it). +function mergeRefsByIdentity( + existing: readonly Reference[] = [], + incoming: readonly Reference[] = [] +): Reference[] { + const seen = new Set(existing.map((ref) => ref.__ref)) + const additions: Reference[] = [] + + for (const ref of incoming) { + if (seen.has(ref.__ref)) { + continue + } + seen.add(ref.__ref) + additions.push(ref) + } + + if (additions.length === 0) { + return existing as Reference[] + } + return [...existing, ...additions] +} + +export const typePolicies: TypePolicies = { + DamnitRun: { + keyFields: ['database', 'proposal', 'run'], + fields: { + cells: { + keyArgs: false, + merge: mergeCellsByName, + }, + }, + }, + Query: { + fields: { + // Paginated runs, one list. Row order is not preserved here: the table + // lays out rows from the server-ordered `metadata.runs` and looks up each + // run's values by identity, so the only thing this list owes is + // membership. + runs: { + keyArgs: ['database'], + merge: mergeRefsByIdentity, + }, + }, + }, +} diff --git a/frontend/packages/ui/tests/data/table/run-stamps.test.ts b/frontend/packages/ui/tests/data/table/run-stamps.test.ts new file mode 100644 index 00000000..b4a75ce0 --- /dev/null +++ b/frontend/packages/ui/tests/data/table/run-stamps.test.ts @@ -0,0 +1,31 @@ +import { afterEach, expect, test, vi } from 'vitest' + +import { liveRunStamps, stampLiveRuns } from '#src/data/table/run-stamps' + +const PROPOSAL = '900405' + +afterEach(() => { + liveRunStamps(new Map()) + vi.restoreAllMocks() +}) + +test('a run stamped by a recent push still flashes on the next one', () => { + stampLiveRuns([{ proposal: PROPOSAL, run: 1 }]) + stampLiveRuns([{ proposal: PROPOSAL, run: 2 }]) + + expect([...liveRunStamps().keys()].sort()).toEqual([ + `${PROPOSAL}:1`, + `${PROPOSAL}:2`, + ]) +}) + +test('a stamp too old to still be fading is dropped on the next push', () => { + stampLiveRuns([{ proposal: PROPOSAL, run: 1 }]) + + const wellPastTheFade = performance.now() + 5000 + vi.spyOn(performance, 'now').mockReturnValue(wellPastTheFade) + stampLiveRuns([{ proposal: PROPOSAL, run: 2 }]) + + // Otherwise every run ever pushed is carried, and copied, forever. + expect([...liveRunStamps().keys()]).toEqual([`${PROPOSAL}:2`]) +}) diff --git a/frontend/packages/ui/tests/data/table/table-data.selectors.test.ts b/frontend/packages/ui/tests/data/table/table-data.selectors.test.ts deleted file mode 100644 index 8e525ce7..00000000 --- a/frontend/packages/ui/tests/data/table/table-data.selectors.test.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { describe, expect, test } from 'vitest' - -import { selectVariables } from '#src/data/table/table-data.selectors' -import type { RootState } from '#src/app/store/reducer' -import type { Variable } from '#src/data/table/table-data.types' - -function stateWithVariables(names: string[]): RootState { - const variables: Record = Object.fromEntries( - names.map((name) => [name, { name, tags: [] }]) - ) - return { tableData: { metadata: { variables } } } as unknown as RootState -} - -describe('selectVariables', () => { - // EXCLUDED_VARIABLES is ['proposal', 'added_at'] only. Note that 'run' is - // NOT excluded here (it is plottable), unlike the column-visibility hook's - // NONCONFIGURABLE_VARIABLES, which also drops 'run'. - test('drops proposal and added_at but keeps run', () => { - const state = stateWithVariables(['proposal', 'added_at', 'run', 'energy']) - expect(selectVariables(state).map((variable) => variable.name)).toEqual([ - 'run', - 'energy', - ]) - }) -}) diff --git a/frontend/packages/ui/tests/data/table/table-data.slice.test.ts b/frontend/packages/ui/tests/data/table/table-data.slice.test.ts deleted file mode 100644 index 948679c8..00000000 --- a/frontend/packages/ui/tests/data/table/table-data.slice.test.ts +++ /dev/null @@ -1,98 +0,0 @@ -import { expect, test } from 'vitest' - -import reducer, { updateTable } from '#src/data/table/table-data.slice' -import type { CellValue, TableData } from '#src/data/table/table-data.types' - -const initial = reducer(undefined, { type: '@@INIT' }) - -const spectrum = (value: CellValue): TableData => ({ - '1': { spectrum: { value, dtype: 'array' } }, -}) - -// What the deferred pass delivers, and what a later lightweight pass sends for -// the same cell: a null with no error, meaning the server held the value back. -const filled = spectrum([1, 2, 3]) -const heldBack = spectrum(null) - -test('a live push marks its runs as just updated', () => { - const state = reducer(initial, updateTable({ data: filled, live: true })) - - expect(state.lastUpdate['1']).toEqual(expect.any(Number)) -}) - -test('a page load does not mark its runs as just updated', () => { - // Only a push is news. Stamping a bulk load would flash the grid's - // live-update highlight over every row the table happens to load. - const state = reducer(initial, updateTable({ data: filled })) - - expect(state.lastUpdate).toEqual({}) -}) - -test('a bulk load does not undo a value already filled in', () => { - const loaded = reducer(initial, updateTable({ data: filled })) - - const state = reducer(loaded, updateTable({ data: heldBack })) - - expect(state.data['1'].spectrum.value).toEqual([1, 2, 3]) -}) - -test('a live push can clear a value', () => { - // A push carries what the server really holds, so its nulls are the truth - // rather than a value being held back for a second fetch. - const loaded = reducer(initial, updateTable({ data: filled })) - - const state = reducer(loaded, updateTable({ data: heldBack, live: true })) - - expect(state.data['1'].spectrum.value).toBeNull() -}) - -test('a held-back value still lands on a cell that has none yet', () => { - // The grid draws a null as a loading skeleton, which is what tells the user - // the deferred pass is still fetching that cell. - const state = reducer(initial, updateTable({ data: heldBack })) - - expect(state.data['1'].spectrum.value).toBeNull() -}) - -test('an errored value replaces one already filled in', () => { - // A blank carrying an error is a real result, not a value held back. - const error = { cls: 'ValueError', message: 'boom' } - const loaded = reducer(initial, updateTable({ data: filled })) - - const errored: TableData = { - '1': { spectrum: { ...spectrum(null)['1'].spectrum, error } }, - } - const state = reducer(loaded, updateTable({ data: errored })) - - expect(state.data['1'].spectrum).toEqual({ - value: null, - dtype: 'array', - error, - }) -}) - -test('runs arriving as numbers become the strings the table keys rows by', () => { - // The subscription and the metadata query both send runs as numbers, while - // the table keys its rows by string. Coercing here is what keeps a plot's - // run list comparable with this one. - const state = reducer( - initial, - updateTable({ data: {}, metadata: { runs: [1, 2, 3] }, live: true }) - ) - - expect(state.metadata.runs).toEqual(['1', '2', '3']) -}) - -test('a metadata push keeps the fields it does not send', () => { - // A push resends runs and variables but never tags, so replacing wholesale - // would drop them and break the tag-driven column visibility. - const tags = { 1: { id: 1, name: 'xpcs', variables: ['spectrum'] } } - const seeded = reducer(initial, updateTable({ data: {}, metadata: { tags } })) - - const state = reducer( - seeded, - updateTable({ data: {}, metadata: { runs: [1] }, live: true }) - ) - - expect(state.metadata.tags).toEqual(tags) -}) 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 b90f1eb1..ace0c43e 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 @@ -1,100 +1,132 @@ import { describe, expect, test } from 'vitest' import { - flattenRuns, - heavyVariableNames, + heavyCellNames, + indexRunCells, + runKey, } from '#src/data/table/table-data.transforms' -import type { CellError, CellValue } from '#src/data/table/table-data.types' +import type { + Cell, + CellError, + CellValue, + Run, +} from '#src/data/table/table-data.types' function cell( name: string, value: CellValue, dtype = 'number', error: CellError | null = null -) { +): Cell { return { name, value, dtype, error } } -describe('flattenRuns', () => { - test('keys each row by its run cell value', () => { - const table = flattenRuns([ - { cells: [cell('run', 5), cell('energy', 1.2)] }, - { cells: [cell('run', 9), cell('energy', 3.4)] }, +function run(proposal: string, number: number, cells: Run['cells']): 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)]), ]) - expect(Object.keys(table)).toEqual(['5', '9']) - expect(table['5'].energy).toEqual({ + expect([...cells.keys()]).toEqual(['900405:5', '900405:9']) + expect(cells.get('900405:5')?.energy).toEqual({ + name: 'energy', value: 1.2, dtype: 'number', - error: undefined, + error: null, }) }) - test('skips a run that has no run variable', () => { - const table = flattenRuns([{ cells: [cell('energy', 1.2)] }]) - expect(table).toEqual({}) - }) + test('keeps runs that share a number across proposals apart', () => { + // 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)]), + ]) - // The guard is `value == null`, so a missing run value is skipped the same - // way whether it arrives as null (as GraphQL delivers it) or undefined. - test('skips a run whose run value is missing', () => { - expect(flattenRuns([{ cells: [cell('run', undefined)] }])).toEqual({}) - expect(flattenRuns([{ cells: [cell('run', null)] }])).toEqual({}) + expect(cells.get('900405:1')?.energy.value).toBe(1.2) + expect(cells.get('900485:1')?.energy.value).toBe(9.9) }) - test('maps a variable to its value, dtype and error', () => { + test('stores each cell by its variable name', () => { const error = { cls: 'ValueError', message: 'boom' } - const table = flattenRuns([ - { cells: [cell('run', 1), cell('x', 2, 'number', error)] }, + const cells = indexRunCells([ + run('900405', 1, [cell('x', 2, 'number', error)]), ]) - expect(table['1'].x).toEqual({ value: 2, dtype: 'number', error }) + expect(cells.get('900405:1')?.x).toEqual({ + name: 'x', + value: 2, + dtype: 'number', + error, + }) }) + test('reuses a run’s cell map while the run object is unchanged', () => { + const runA = run('900405', 1, [cell('energy', 1.2)]) - test('collapses a null error to undefined', () => { - const table = flattenRuns([ - { cells: [cell('run', 1), cell('x', 2, 'number', null)] }, - ]) - expect(table['1'].x.error).toBeUndefined() + // 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. + const first = indexRunCells([runA]).get('900405:1') + const second = indexRunCells([runA]).get('900405:1') + + expect(second).toBe(first) }) + + 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 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 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) + }) +}) + +test('runKey pairs proposal and run into a lookup key', () => { + expect(runKey({ proposal: '900405', run: 143 })).toBe('900405:143') }) -// A heavy value the @lightweight directive held back: the server sends the -// variable with its value nulled out. +// A heavy value the @lightweight directive held back: the server sends the cell +// with its value nulled out. const blanked = (name: string, error: CellError | null = null) => cell(name, null, 'array', error) -describe('heavyVariableNames', () => { +describe('heavyCellNames', () => { test('names the blanked cells worth a second fetch', () => { - const rows = flattenRuns([ - { - cells: [cell('run', 1), cell('energy', 1.2), blanked('spectrum')], - }, + const names = heavyCellNames([ + run('900405', 1, [cell('energy', 1.2), blanked('spectrum')]), ]) - expect(heavyVariableNames(rows)).toEqual(['spectrum']) + expect(names).toEqual(['spectrum']) }) - test('leaves out a variable that failed rather than being held back', () => { - // A blank carrying an error is a variable that genuinely has no value, so + test('leaves out a cell that failed rather than being held back', () => { + // A blank carrying an error is a cell that genuinely has no value, so // fetching it again would only return the same error. - const rows = flattenRuns([ - { - cells: [ - cell('run', 1), - blanked('broken', { cls: 'ValueError', message: 'boom' }), - ], - }, + const names = heavyCellNames([ + run('900405', 1, [ + blanked('broken', { cls: 'ValueError', message: 'boom' }), + ]), ]) - expect(heavyVariableNames(rows)).toEqual([]) + expect(names).toEqual([]) }) - test('names a variable once however many runs blanked it', () => { - const rows = flattenRuns([ - { cells: [cell('run', 1), blanked('spectrum')] }, - { cells: [cell('run', 2), blanked('spectrum')] }, + test('names a cell once however many runs blanked it', () => { + const names = heavyCellNames([ + run('900405', 1, [blanked('spectrum')]), + run('900405', 2, [blanked('spectrum')]), ]) - expect(heavyVariableNames(rows)).toEqual(['spectrum']) + expect(names).toEqual(['spectrum']) }) }) diff --git a/frontend/packages/ui/tests/features/table/cells.test.ts b/frontend/packages/ui/tests/features/table/cells.test.ts index 1b03ce61..08784252 100644 --- a/frontend/packages/ui/tests/features/table/cells.test.ts +++ b/frontend/packages/ui/tests/features/table/cells.test.ts @@ -15,17 +15,20 @@ import { import { DTYPES } from '#src/constants' describe('getCell', () => { - // A missing value (null or undefined) always renders a loading skeleton, - // whatever the declared dtype is. - test('renders a loading cell when the value is missing', () => { - expect( - getCell({ value: undefined, dtype: DTYPES.number, options: {} }).kind - ).toBe(GridCellKind.Loading) + // Only a heavy dtype is ever held back by @lightweight, so only a heavy null + // has a value still on its way. + test('renders a loading cell for a missing heavy value', () => { expect( getCell({ value: undefined, dtype: DTYPES.image, options: {} }).kind ).toBe(GridCellKind.Loading) }) + 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 diff --git a/frontend/packages/ui/tests/features/table/hooks/use-column-visibility.test.ts b/frontend/packages/ui/tests/features/table/hooks/use-column-visibility.test.ts index 3b18f26d..89416fc1 100644 --- a/frontend/packages/ui/tests/features/table/hooks/use-column-visibility.test.ts +++ b/frontend/packages/ui/tests/features/table/hooks/use-column-visibility.test.ts @@ -10,14 +10,34 @@ const tag = (name: string, variables: string[]): Tag => ({ }) describe('computeColumnVisibility', () => { - test('drops the nonconfigurable variables', () => { + test('drops the nonconfigurable variables but keeps proposal', () => { const result = computeColumnVisibility({ variableNames: ['proposal', 'added_at', 'run', 'energy', 'x'], visibility: {}, tags: {}, tagSelection: {}, }) - expect(Object.keys(result)).toEqual(['energy', 'x']) + expect(Object.keys(result)).toEqual(['proposal', 'energy', 'x']) + }) + + test('proposal is hidden by default', () => { + const result = computeColumnVisibility({ + variableNames: ['proposal', 'energy'], + visibility: {}, + tags: {}, + tagSelection: {}, + }) + expect(result).toEqual({ proposal: false, energy: true }) + }) + + test('proposal becomes visible once it is turned on', () => { + const result = computeColumnVisibility({ + variableNames: ['proposal', 'energy'], + visibility: { proposal: true }, + tags: {}, + tagSelection: {}, + }) + expect(result).toEqual({ proposal: true, energy: true }) }) test('a variable is visible unless it is explicitly turned off', () => { diff --git a/frontend/packages/ui/tests/features/table/pagination.test.ts b/frontend/packages/ui/tests/features/table/pagination.test.ts index ffebca4e..b4594463 100644 --- a/frontend/packages/ui/tests/features/table/pagination.test.ts +++ b/frontend/packages/ui/tests/features/table/pagination.test.ts @@ -1,49 +1,19 @@ -import { describe, expect, test } from 'vitest' +import { expect, test } from 'vitest' -import { - pageRangeForRegion, - pagesForRegion, -} from '#src/features/table/pagination' +import { pageRangeForRegion } from '#src/features/table/pagination' const region = (y: number, height: number) => ({ x: 0, y, width: 0, height }) -describe('pageRangeForRegion', () => { - test('returns the padded page window for a scroll region', () => { - // Rows 100-150 live on pages 11-16 (1-based); the window adds a page of - // overscan on each side so rows are loaded before they scroll into view. - expect(pageRangeForRegion(region(100, 50), 10)).toEqual([ - 10, 11, 12, 13, 14, 15, 16, 17, - ]) - }) - - test('pins the first page at the top boundary', () => { - // At the top of the table the upward overscan is clamped: the window - // never asks for a page below page 1. - expect(pageRangeForRegion(region(0, 20), 10)).toEqual([1, 2, 3, 4]) - }) +test('returns the padded page window for a scroll region', () => { + // Rows 100-150 live on pages 11-16 (1-based); the window adds a page of + // overscan on each side so rows are loaded before they scroll into view. + expect(pageRangeForRegion(region(100, 50), 10)).toEqual([ + 10, 11, 12, 13, 14, 15, 16, 17, + ]) }) -describe('pagesForRegion', () => { - test('wants the pages a first scroll window needs', () => { - expect(pagesForRegion([], region(0, 20), 10)).toEqual([1, 2, 3, 4]) - }) - - test('drops the pages scrolled away from', () => { - const top = pagesForRegion([], region(0, 20), 10) - - // Only the window is wanted. The rows left behind keep rendering from the - // table slice, so holding their loaders mounted would buy nothing but a - // watcher per page scrolled past for the life of the proposal. - expect(pagesForRegion(top, region(100, 50), 10)).toEqual([ - 10, 11, 12, 13, 14, 15, 16, 17, - ]) - }) - - test('returns the same array when the window is unchanged', () => { - const pages = pagesForRegion([], region(0, 20), 10) - - // Identity, not just equality: an unchanged reference is what lets React - // bail out instead of rebuilding every loader on each scroll event. - expect(pagesForRegion(pages, region(1, 18), 10)).toBe(pages) - }) +test('pins the first page at the top boundary', () => { + // At the top of the table the upward overscan is clamped: the window never + // asks for a page below page 1. + expect(pageRangeForRegion(region(0, 20), 10)).toEqual([1, 2, 3, 4]) }) diff --git a/frontend/packages/ui/tests/features/table/table-page-loader.test.tsx b/frontend/packages/ui/tests/features/table/table-page-loader.test.tsx deleted file mode 100644 index 55590984..00000000 --- a/frontend/packages/ui/tests/features/table/table-page-loader.test.tsx +++ /dev/null @@ -1,257 +0,0 @@ -import type { PropsWithChildren } from 'react' -import { - ApolloClient, - ApolloLink, - InMemoryCache, - Observable, -} from '@apollo/client' -import { ApolloProvider } from '@apollo/client/react' -import { Provider } from 'react-redux' -import { render } from 'vitest-browser-react' -import { expect, test, vi } from 'vitest' - -import { setupStore, type AppStore } from '#src/app/store/store' -import { updateTable } from '#src/data/table/table-data.slice' -import TablePageLoader from '#src/features/table/table-page-loader' - -const PROPOSAL = '6996' - -// The server blanks heavy values under @lightweight and fills them in on the -// deferred follow-up, so the two operations answer differently. `energy` models -// a scalar the server has since recomputed: it rides along on the lightweight -// pass, which is what makes that response change while the heavy one does not. -// Omitting `names` asks for every variable, exactly as the schema defines it. -const responseFor = ( - operationName: string, - { names, energy }: { names?: string[]; energy: number } -) => { - const lightweight = operationName === 'LightweightTableDataQuery' - const cells = [ - { name: 'run', value: 1, dtype: 'number', error: null }, - { name: 'energy', value: energy, dtype: 'number', error: null }, - { - name: 'spectrum', - value: lightweight ? null : [1, 2, 3], - dtype: 'array', - error: null, - }, - ] - - return { - runs: [ - { - cells: cells.filter( - (cell) => names == null || names.includes(cell.name) - ), - }, - ], - } -} - -type SeenOperation = { name: string; variables: Record } - -// A link that records what was asked and answers only when told to, so a test -// can inspect the gap between a request going out and its rows arriving. -function createNetwork() { - const operations: SeenOperation[] = [] - let pending: Array<() => void> = [] - let energy = 10 - - const link = new ApolloLink((operation) => { - operations.push({ - name: operation.operationName, - variables: operation.variables, - }) - return new Observable((observer) => { - pending.push(() => { - observer.next({ - data: responseFor(operation.operationName, { - names: operation.variables.names as string[] | undefined, - energy, - }), - }) - observer.complete() - }) - }) - }) - - const namesOf = (name: string) => operations.filter((op) => op.name === name) - - return { - link, - operations, - lightweight: () => namesOf('LightweightTableDataQuery'), - deferred: () => namesOf('DeferredTableDataQuery'), - recompute: (value: number) => { - energy = value - }, - deliver: () => { - const ready = pending - pending = [] - ready.forEach((send) => send()) - }, - } -} - -function makeWrapper(store: AppStore, client: ApolloClient) { - return function Providers({ children }: PropsWithChildren) { - return ( - - {children} - - ) - } -} - -const loader = - -const rowsIn = (store: AppStore) => store.getState().tableData.data - -test('a mounted loader dispatches its page rows, then fetches the values the server held back', async () => { - const network = createNetwork() - const store = setupStore() - const client = new ApolloClient({ - cache: new InMemoryCache(), - link: network.link, - }) - - await render(loader, { wrapper: makeWrapper(store, client) }) - - // The lightweight pass asks for the whole page, no names: every variable, - // with the heavy ones blanked. - expect(network.lightweight()).toHaveLength(1) - expect(network.lightweight()[0].variables).toEqual({ - proposal: PROPOSAL, - page: 2, - per_page: 10, - }) - - network.deliver() - - // The blanked cell lands as a null, which is what the grid draws as a loading - // skeleton until the deferred pass fills it in. - await vi.waitFor(() => { - expect(rowsIn(store)['1'].spectrum.value).toBeNull() - }) - - // Learning which values were blanked is what drives the second pass, and it - // asks for `run` too, since that is what keys the rows it fills in. - await vi.waitFor(() => { - expect(network.deferred()).toHaveLength(1) - }) - expect(network.deferred()[0].variables).toEqual({ - proposal: PROPOSAL, - page: 2, - per_page: 10, - names: ['run', 'spectrum'], - }) - - network.deliver() - - await vi.waitFor(() => { - expect(rowsIn(store)['1'].spectrum.value).toEqual([1, 2, 3]) - }) -}) - -test('a loader unmounted before its rows arrive dispatches nothing', async () => { - const network = createNetwork() - const store = setupStore() - const client = new ApolloClient({ - cache: new InMemoryCache(), - link: network.link, - }) - - const screen = await render(loader, { wrapper: makeWrapper(store, client) }) - expect(network.lightweight()).toHaveLength(1) - - // Leaving the proposal unmounts the loader while its page is still in - // flight. The unmounted hook drops the late rows, so they can never refill - // the slice that teardown just reset. - await screen.unmount() - network.deliver() - - await new Promise((resolve) => setTimeout(resolve)) - expect(rowsIn(store)).toEqual({}) -}) - -test('a revalidated page keeps the heavy values the deferred pass filled in', async () => { - const network = createNetwork() - const store = setupStore() - const client = new ApolloClient({ - cache: new InMemoryCache(), - link: network.link, - }) - - // Load the page once, so both passes have run and the heavy value is in place. - const first = await render(loader, { wrapper: makeWrapper(store, client) }) - network.deliver() - await vi.waitFor(() => expect(network.deferred()).toHaveLength(1)) - network.deliver() - await vi.waitFor(() => { - expect(rowsIn(store)['1'].spectrum.value).toEqual([1, 2, 3]) - }) - await first.unmount() - - // Scrolling the page back into view remounts its loader, and the server has - // recomputed a scalar meanwhile. The lightweight pass answers first, carrying - // the new scalar and blanking the heavy cell again. - network.recompute(20) - await render(loader, { wrapper: makeWrapper(store, client) }) - await vi.waitFor(() => expect(network.lightweight()).toHaveLength(2)) - network.deliver() - await vi.waitFor(() => { - expect(rowsIn(store)['1'].energy.value).toBe(20) - }) - - // The held-back guard keeps the heavy value the first pass filled in, so the - // cell never flashes blank while the deferred pass is still on its way. - expect(rowsIn(store)['1'].spectrum.value).toEqual([1, 2, 3]) - - // The deferred pass then refetches and refills it, unchanged. - await vi.waitFor(() => expect(network.deferred()).toHaveLength(2)) - network.deliver() - await vi.waitFor(() => { - expect(rowsIn(store)['1'].spectrum.value).toEqual([1, 2, 3]) - }) -}) - -test('a live value is not rolled back when its page is revisited', async () => { - const network = createNetwork() - const store = setupStore() - const client = new ApolloClient({ - cache: new InMemoryCache(), - link: network.link, - }) - - // Load the page once, then leave it, the way scrolling it off screen does. - const first = await render(loader, { wrapper: makeWrapper(store, client) }) - network.deliver() - await vi.waitFor(() => expect(network.deferred()).toHaveLength(1)) - network.deliver() - await vi.waitFor(() => expect(rowsIn(store)['1'].energy.value).toBe(10)) - await first.unmount() - - // A subscription pushes a newer scalar while the page is off screen. It writes - // only to the slice, so the page's would-be cache entry stays at the old 10. - store.dispatch( - updateTable({ - data: { '1': { energy: { value: 99, dtype: 'number' } } }, - live: true, - }) - ) - expect(rowsIn(store)['1'].energy.value).toBe(99) - - // Scrolling back remounts the loader, which refetches rather than replaying a - // stale page. Nothing is delivered yet, so the live value stands where a - // cached replay would have rolled it back to 10. - network.recompute(99) - await render(loader, { wrapper: makeWrapper(store, client) }) - await vi.waitFor(() => expect(network.lightweight()).toHaveLength(2)) - expect(rowsIn(store)['1'].energy.value).toBe(99) - - // The fresh answer carries the value the server pushed, so it keeps it there. - network.deliver() - await vi.waitFor(() => expect(network.deferred()).toHaveLength(2)) - network.deliver() - expect(rowsIn(store)['1'].energy.value).toBe(99) -}) diff --git a/frontend/packages/ui/tests/features/table/table.slice.test.ts b/frontend/packages/ui/tests/features/table/table.slice.test.ts index 4c2ce444..7076757b 100644 --- a/frontend/packages/ui/tests/features/table/table.slice.test.ts +++ b/frontend/packages/ui/tests/features/table/table.slice.test.ts @@ -8,23 +8,46 @@ import reducer, { } from '#src/features/table/table.slice' describe('selectRun', () => { - test('sets the run and its variables', () => { + test('sets the run, its proposal, and its variables', () => { const state = reducer( undefined, - selectRun({ run: 5, variables: ['a', 'b'] }) + selectRun({ proposal: '900485', run: 5, variables: ['a', 'b'] }) ) + expect(state.selection.proposal).toBe('900485') expect(state.selection.run).toBe(5) expect(state.selection.variables).toEqual(['a', 'b']) }) + // The proposal disambiguates a run number that collides across proposals, so + // the same number under a different proposal is a distinct selection. + test('keeps the proposal that owns the selected run', () => { + // Select the active proposal's run 5. + let state = reducer( + undefined, + selectRun({ proposal: '900485', run: 5, variables: [] }) + ) + expect(state.selection.proposal).toBe('900485') + + // Select a guest proposal's run of the same number. + state = reducer( + state, + selectRun({ proposal: '888888', run: 5, variables: [] }) + ) + expect(state.selection.proposal).toBe('888888') + expect(state.selection.run).toBe(5) + }) + // Referential stability is the contract: a redundant dispatch must keep the // exact same variables array, which is what the isArrayEqual guard buys us. test('keeps the same variables array reference on a redundant dispatch', () => { const first = reducer( undefined, - selectRun({ run: 5, variables: ['a', 'b'] }) + selectRun({ proposal: '900485', run: 5, variables: ['a', 'b'] }) + ) + const second = reducer( + first, + selectRun({ proposal: '900485', run: 5, variables: ['a', 'b'] }) ) - const second = reducer(first, selectRun({ run: 5, variables: ['a', 'b'] })) expect(second.selection.variables).toBe(first.selection.variables) }) }) 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 new file mode 100644 index 00000000..fdb0000a --- /dev/null +++ b/frontend/packages/ui/tests/features/table/use-table-runs.test.tsx @@ -0,0 +1,213 @@ +import { type PropsWithChildren } from 'react' +import { + ApolloClient, + ApolloLink, + InMemoryCache, + Observable, +} from '@apollo/client' +import { ApolloProvider } from '@apollo/client/react' +import { renderHook } from 'vitest-browser-react' +import { afterEach, expect, test, vi } from 'vitest' + +import { + DEFERRED_TABLE_DATA_QUERY_NAME, + TABLE_DATA_QUERY_NAME, +} from '#src/graphql/operation-names' +import { liveRunStamps, stampLiveRuns } from '#src/data/table/run-stamps' +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' + +const PROPOSAL = '900405' +const PAGE_SIZE = 10 +const LIGHTWEIGHT_NAME = `Lightweight${TABLE_DATA_QUERY_NAME}` + +// The stamp store is module state shared across tests: reset it so one test's +// push cannot flash a later test's runs. +afterEach(() => { + liveRunStamps(new Map()) +}) + +// A run whose only cell is a heavy value @lightweight held back, so the hook +// always has a deferred pass to fire for the page. +const runFor = (run: number) => ({ + __typename: 'DamnitRun', + database: PROPOSAL, + proposal: PROPOSAL, + run, + cells: [ + { + __typename: 'Cell', + name: 'spectrum', + value: null, + dtype: 'array', + error: null, + }, + ], +}) + +// Answers the lightweight page reads at once and leaves every deferred read +// hanging, recording which pages started and which were cancelled, so a test +// decides when (and whether) a page's heavy values land. +function createNetwork() { + const started = { lightweight: [] as number[], deferred: [] as number[] } + const cancelled = { deferred: [] as number[] } + + const link = new ApolloLink((operation) => { + const page = (operation.variables.page as number) ?? 1 + if (operation.operationName === LIGHTWEIGHT_NAME) { + started.lightweight.push(page) + return new Observable((observer) => { + observer.next({ data: { runs: [runFor(page)] } }) + observer.complete() + }) + } + started.deferred.push(page) + return new Observable(() => () => { + cancelled.deferred.push(page) + }) + }) + + return { link, started, cancelled } +} + +async function renderTableRuns(network: { link: ApolloLink }) { + const client = new ApolloClient({ + cache: new InMemoryCache({ typePolicies }), + link: ApolloLink.from([ + createPriorityLink({ + maxActive: 1, + queuedOperations: [DEFERRED_TABLE_DATA_QUERY_NAME], + }), + network.link, + ]), + }) + + function Providers({ children }: PropsWithChildren) { + return {children} + } + + const view = await renderHook( + () => + useTableRuns({ + proposal: PROPOSAL, + paginated: true, + pageSize: PAGE_SIZE, + }), + { wrapper: Providers } + ) + return { ...view, client } +} + +test('a scroll sweep fetches the window it settles on, not every window it crossed', async () => { + const pages: number[] = [] + const link = new ApolloLink((operation) => { + if (operation.operationName === LIGHTWEIGHT_NAME) { + const page = (operation.variables.page as number) ?? 1 + pages.push(page) + return new Observable((observer) => { + observer.next({ data: { runs: [runFor(page)] } }) + observer.complete() + }) + } + return new Observable(() => () => {}) + }) + + const { result } = await renderTableRuns({ link }) + await vi.waitFor(() => expect(pages).toContain(1)) + pages.length = 0 + + // Dragging the scrollbar: the grid reports a region on every frame, and each + // one names a different band of pages. + for (let y = 0; y < 500; y += 10) { + result.current.onVisibleRegionChanged({ x: 0, y, width: 100, height: 20 }) + } + + await vi.waitFor(() => expect(pages.length).toBeGreaterThan(0)) + // Fetching per frame would ask for every page the drag passed over, and none + // of those requests can be cancelled once made. + expect(pages.length).toBeLessThan(10) +}) + +test('cancels in-flight deferred fetches when the hook unmounts', async () => { + const network = createNetwork() + const { unmount } = await renderTableRuns(network) + + await vi.waitFor(() => expect(network.started.deferred).toContain(1)) + + // A proposal switch remounts the hook. Its in-flight heavy pages must let go, + // or they land after the teardown eviction and write the departed proposal's + // runs back into the cache. + await unmount() + + await vi.waitFor(() => expect(network.cancelled.deferred).toEqual([1])) +}) + +test('does not flash a run filled by a cache write', async () => { + const network = createNetwork() + const { result, client } = await renderTableRuns(network) + const key = `${PROPOSAL}:1` + + await vi.waitFor(() => + expect(result.current.cellsByKey.get(key)).toBeDefined() + ) + + // Fill run 1's held-back cell straight through the cache, the way the + // deferred pass (or any bulk load) lands. A fill is not an update. + client.cache.writeQuery({ + query: TABLE_DATA_QUERY, + variables: { + proposal: PROPOSAL, + page: 1, + per_page: PAGE_SIZE, + names: ['run', 'spectrum'], + }, + data: { + runs: [ + { + __typename: 'DamnitRun', + database: PROPOSAL, + proposal: PROPOSAL, + run: 1, + cells: [ + { + __typename: 'Cell', + name: 'spectrum', + value: [1, 2, 3], + dtype: 'array', + error: null, + }, + ], + }, + ], + }, + }) + + // The fill landed, and the run still carries no flash stamp. + await vi.waitFor(() => + expect(result.current.cellsByKey.get(key)?.spectrum.value).toEqual([ + 1, 2, 3, + ]) + ) + expect(result.current.lastUpdatedByKey.get(key)).toBeUndefined() +}) + +test('flashes a pushed run in the grid frame clock', async () => { + const network = createNetwork() + const { result } = await renderTableRuns(network) + const key = `${PROPOSAL}:1` + + // Stamp the way the subscription push handler does when a push arrives. + stampLiveRuns([{ proposal: PROPOSAL, run: 1 }]) + + // Glide fades the flash against its own performance.now() frame time, so + // the stamp must sit in that clock: an epoch stamp (Date.now()) reads as + // decades in the future and paints the row yellow forever. + await vi.waitFor(() => + expect(result.current.lastUpdatedByKey.get(key)).toBeGreaterThan(0) + ) + expect(result.current.lastUpdatedByKey.get(key)).toBeLessThanOrEqual( + performance.now() + ) +}) diff --git a/frontend/packages/ui/tests/graphql/priority-link.test.ts b/frontend/packages/ui/tests/graphql/priority-link.test.ts index 1abbd914..27d9dab6 100644 --- a/frontend/packages/ui/tests/graphql/priority-link.test.ts +++ b/frontend/packages/ui/tests/graphql/priority-link.test.ts @@ -11,7 +11,7 @@ const DEFERRED = gql` ` const PROMPT = gql` - query TableMetadataQuery { + query TableMetaQuery { metadata } ` diff --git a/frontend/packages/ui/tests/graphql/type-policies.test.ts b/frontend/packages/ui/tests/graphql/type-policies.test.ts new file mode 100644 index 00000000..4a3021a3 --- /dev/null +++ b/frontend/packages/ui/tests/graphql/type-policies.test.ts @@ -0,0 +1,132 @@ +import { InMemoryCache } from '@apollo/client' +import { beforeEach, expect, test } from 'vitest' + +import { + TABLE_DATA_QUERY, + type TableDataResult, +} from '#src/data/table/table-data.queries' +import { typePolicies } from '#src/graphql/type-policies' + +const PROPOSAL = '900405' + +let cache: InMemoryCache + +beforeEach(() => { + cache = new InMemoryCache({ typePolicies }) +}) + +// A cell as it sits in the cache, under its wire __typename. +type Cell = { + name: string + value: unknown + dtype: string + error: { cls: string; message: string } | null +} + +function cell( + name: string, + value: unknown, + dtype = 'number', + error: Cell['error'] = null +): Cell { + return { __typename: 'Cell', name, value, dtype, error } as Cell +} + +function run(proposal: string, number: number, cells: Cell[]) { + return { + __typename: 'DamnitRun', + database: PROPOSAL, + proposal, + run: number, + cells, + } +} + +function writeRuns(runs: ReturnType[]) { + cache.writeQuery({ + query: TABLE_DATA_QUERY, + variables: { proposal: PROPOSAL, page: 1, per_page: 10 }, + data: { runs }, + }) +} + +function readRuns() { + return cache.readQuery({ + query: TABLE_DATA_QUERY, + variables: { proposal: PROPOSAL, page: 1, per_page: 10 }, + })!.runs +} + +const valueOf = ( + runs: TableDataResult['runs'], + identity: number, + name: string +) => + runs + .find((entry) => entry.run === identity) + ?.cells.find((entry) => entry.name === name)?.value + +const blanked = cell('spectrum', null, 'array') +const filled = cell('spectrum', [1, 2, 3], '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])]) + 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])]) + expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) + expect(valueOf(readRuns(), 1, 'energy')).toBe(10) +}) + +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. + writeRuns([run(PROPOSAL, 1, [blanked])]) + + expect(valueOf(readRuns(), 1, 'spectrum')).toEqual([1, 2, 3]) +}) + +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])]) + + 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. + const error = { cls: 'ValueError', message: 'boom' } + writeRuns([run(PROPOSAL, 1, [filled])]) + + writeRuns([run(PROPOSAL, 1, [cell('spectrum', null, 'array', error)])]) + + const spectrum = readRuns()[0].cells.find((c) => c.name === 'spectrum') + expect(spectrum?.value).toBeNull() + expect(spectrum?.error).toEqual(error) +}) + +test('paginated runs accumulate into one list, deduped by identity', () => { + writeRuns([run(PROPOSAL, 1, [cell('energy', 1)])]) + writeRuns([ + run(PROPOSAL, 1, [cell('energy', 1)]), + run(PROPOSAL, 2, [cell('energy', 2)]), + ]) + + const runs = readRuns() + expect(runs.map((entry) => entry.run)).toEqual([1, 2]) +}) + +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)]), + ]) + + const runs = readRuns() + expect(runs).toHaveLength(2) + expect(cache.identify(runs[0])).not.toBe(cache.identify(runs[1])) +}) diff --git a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx index fd8d4c75..7b692006 100644 --- a/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx +++ b/frontend/packages/ui/tests/redux/proposal-teardown.test.tsx @@ -1,7 +1,7 @@ import type { PropsWithChildren } from 'react' import { useEffect } from 'react' -import { ApolloClient, ApolloLink, Observable } from '@apollo/client' -import { ApolloProvider } from '@apollo/client/react' +import { ApolloClient, ApolloLink, Observable, gql } from '@apollo/client' +import { ApolloProvider, useQuery } from '@apollo/client/react' import { Provider } from 'react-redux' import { render } from 'vitest-browser-react' import { beforeEach, expect, test, vi } from 'vitest' @@ -10,32 +10,53 @@ import { resetProposal } from '#src/app/store/actions' import { useAppDispatch } from '#src/app/store/hooks' import { setupStore, type AppStore } from '#src/app/store/store' import { setProposalPending } from '#src/data/metadata/metadata.slice' -import TablePageLoader from '#src/features/table/table-page-loader' +import { LIGHTWEIGHT_TABLE_DATA_QUERY } from '#src/data/table/table-data.queries' import { cache } from '#src/graphql/apollo' -// The real cache and the real resetProposal listener: what this pins is how the -// listener's eviction lands against watchers that are still on their way out. +// 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, +// and the eviction is late enough that it does not send the departing watcher +// back to the network. -const PAGES = [1, 2, 3] +// The home page's list, keyed by proposal_numbers rather than by a proposal. +const PROPOSAL_LIST = gql` + query ProposalList($proposal_numbers: [Int!]!) { + proposal_metadata(proposal_numbers: $proposal_numbers) { + number + } + } +` -const runsPayload = { - runs: [ - { - cells: [ - { name: 'run', value: 1, dtype: 'number', error: null }, - { name: 'energy', value: 10, dtype: 'number', error: null }, - ], - }, - ], +function runsPayload(proposal: string) { + return { + runs: [ + { + __typename: 'DamnitRun', + database: proposal, + proposal, + run: 1, + cells: [ + { + __typename: 'Cell', + name: 'energy', + value: 10, + dtype: 'number', + error: null, + }, + ], + }, + ], + } } function createNetwork() { const proposals: string[] = [] const link = new ApolloLink((operation) => { - proposals.push(operation.variables.proposal as string) + const proposal = operation.variables.proposal as string + proposals.push(proposal) return new Observable((observer) => { - observer.next({ data: runsPayload }) + observer.next({ data: runsPayload(proposal) }) observer.complete() }) }) @@ -50,6 +71,15 @@ function createNetwork() { } } +// The one watched runs query, the render source the grid reads from. +function RunsWatcher({ proposal }: { proposal: string }) { + useQuery(LIGHTWEIGHT_TABLE_DATA_QUERY, { + variables: { proposal, page: 1, per_page: 10 }, + fetchPolicy: 'cache-and-network', + }) + return null +} + // Mirrors the app's ProposalWrapper: keyed on the proposal, so a switch unmounts // this subtree, and its cleanup is what tells the store the proposal is gone. function ProposalWrapper({ @@ -73,24 +103,69 @@ function tree(store: AppStore, client: ApolloClient, proposal: string) { - {PAGES.map((page) => ( - - ))} + ) } +// 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) { + const snapshot = cache.extract() as Record + return Object.entries(snapshot) + .flatMap(([id, entry]) => (id === 'ROOT_QUERY' ? Object.keys(entry) : [id])) + .filter((key) => key.includes(`"proposal":"${proposal}"`)) +} + beforeEach(async () => { await cache.reset() }) +test('leaving a proposal keeps the shared proposal list', async () => { + const network = createNetwork() + const store = setupStore() + const client = new ApolloClient({ cache, link: network.link }) + + // The home page's proposal list is not scoped to any one proposal, and the + // eviction picks its fields out of ROOT_QUERY by matching the departed + // proposal in their arguments. Nothing else pins that the list survives. + cache.writeQuery({ + query: PROPOSAL_LIST, + variables: { proposal_numbers: [6996] }, + data: { proposal_metadata: [{ __typename: 'ProposalMeta', number: 6996 }] }, + }) + + const screen = await render(tree(store, client, 'A')) + await vi.waitFor(() => expect(cacheEntriesFor('A')).not.toHaveLength(0)) + + await screen.rerender(tree(store, client, 'B')) + + await vi.waitFor(() => expect(cacheEntriesFor('A')).toHaveLength(0)) + expect( + Object.keys(cache.extract().ROOT_QUERY as object).filter((field) => + field.startsWith('proposal_metadata') + ) + ).not.toHaveLength(0) +}) + +test('leaving a proposal drops its cached runs', async () => { + const network = createNetwork() + const store = setupStore() + const client = new ApolloClient({ cache, link: network.link }) + + const screen = await render(tree(store, client, 'A')) + await vi.waitFor(() => expect(cacheEntriesFor('A')).not.toHaveLength(0)) + + await screen.rerender(tree(store, client, 'B')) + + await vi.waitFor(() => { + expect(cacheEntriesFor('A')).toHaveLength(0) + expect(cacheEntriesFor('B')).not.toHaveLength(0) + }) +}) + test('leaving a proposal does not refetch the one just left', async () => { const network = createNetwork() const store = setupStore() @@ -100,10 +175,8 @@ test('leaving a proposal does not refetch the one just left', async () => { await vi.waitFor(() => expect(network.requestsFor('A')).toBeGreaterThan(0)) network.clear() - // Switching proposals unmounts A's subtree. React runs its cleanups parent - // first, so the eviction fires while A's own watchers are still subscribed: - // an unscoped, synchronous evict dirties them and each one refetches a page - // of the proposal the user has already left. + // Switching proposals unmounts A's subtree. The eviction waits for A's + // watcher to unsubscribe, so dropping A's fields dirties nothing still live. await screen.rerender(tree(store, client, 'B')) await new Promise((resolve) => setTimeout(resolve, 250)) diff --git a/frontend/packages/ui/tests/redux/reset.test.ts b/frontend/packages/ui/tests/redux/reset.test.ts index 3dd211a3..eaef9988 100644 --- a/frontend/packages/ui/tests/redux/reset.test.ts +++ b/frontend/packages/ui/tests/redux/reset.test.ts @@ -1,26 +1,16 @@ -import { expect, test, vi } from 'vitest' -import { gql } from '@apollo/client' +import { expect, test } from 'vitest' -import { cache } from '#src/graphql/apollo' import { authApi, type UserInfo } from '#src/features/auth/auth.api' import { selectUserFullName } from '#src/features/auth/auth.slice' import { contextfileApi } from '#src/features/context-file/context-file.api' -import { updateTable } from '#src/data/table/table-data.slice' -import { setProposalPending } from '#src/data/metadata/metadata.slice' import { resetProposal } from '#src/app/store/actions' import type { RootState } from '#src/app/store/reducer' import { setupStore } from '#src/app/store/store' -// Leaving a proposal drops every proposal-scoped cache, in both Apollo and RTK -// Query. Only the session (authApi) and the proposal list (proposal_metadata) -// survive. - -// A real cache, so the eviction below is observable. Importing the real module -// opens a websocket from a Node test. -vi.mock('#src/graphql/apollo', async () => { - const { InMemoryCache } = await import('@apollo/client') - return { cache: new InMemoryCache(), client: {} } -}) +// Leaving a proposal returns every proposal-scoped redux slice to its initial +// state and drops the RTK Query context-file cache. The Apollo cache is left +// warm on purpose (a run is keyed by database, proposal, run), and the session +// (authApi) survives. // upsertQueryData pipes the value through transformResponse, so the fixture // feeds the wire shape. Handing it a UserInfo instead leaves proposals @@ -33,27 +23,6 @@ const user = { proposals_by_year_half: { '202401': [6996] }, } as unknown as UserInfo -async function signedInStoreShowingRun() { - const store = setupStore() - - await store.dispatch( - authApi.util.upsertQueryData('getUserInfo', undefined, user) - ) - store.dispatch( - updateTable({ - data: { '5': { run: { value: 5, dtype: 'number' } } }, - metadata: { - variables: { run: { name: 'run', tags: [] } }, - runs: ['5'], - timestamp: 1, - tags: {}, - }, - }) - ) - - return store -} - // Every slice the store owns except the two RTK Query caches, read off the // store itself so a slice added later is covered without touching this file. const apiPaths: string[] = [authApi.reducerPath, contextfileApi.reducerPath] @@ -76,76 +45,13 @@ test.each(proposalSlices)( } ) -// The dashboard's three root fields alongside the home page's, each with the -// arguments the real queries send, so eviction has to match every variant. -const CACHED_FIELDS = gql` - query Cached($proposal: String) { - runs(database: { proposal: $proposal }) - metadata(database: { proposal: $proposal }) - extracted_data(database: { proposal: $proposal }, run: 1, variable: "image") - proposal_metadata(proposal_numbers: [6996]) { - number - } - } -` - -function cacheProposal(proposal: string) { - cache.writeQuery({ - query: CACHED_FIELDS, - variables: { proposal }, - data: { - runs: { '5': { run: 5 } }, - metadata: { runs: ['5'] }, - extracted_data: { value: 'png' }, - proposal_metadata: [{ __typename: 'ProposalMetadata', number: 6996 }], - }, - }) -} - -const cachedFields = () => - Object.keys(cache.extract().ROOT_QUERY ?? {}).filter( - (key) => key !== '__typename' - ) - -// The eviction is deferred past the departing watchers' unsubscribes, so it -// lands a macrotask later rather than within the dispatch. -async function leaveProposal(proposal: string) { +test('resetProposal keeps the user signed in', async () => { const store = setupStore() - store.dispatch(setProposalPending(proposal)) - cacheProposal(proposal) - - store.dispatch(resetProposal()) - await vi.waitFor(() => - expect(cachedFields()).not.toContain( - `metadata({"database":{"proposal":"${proposal}"}})` - ) + await store.dispatch( + authApi.util.upsertQueryData('getUserInfo', undefined, user) ) -} - -test('resetProposal drops the departed proposal, keeping the proposal list', async () => { - await leaveProposal('6996') - - expect(cachedFields().map((key) => key.split('(')[0])).toEqual([ - 'proposal_metadata', - ]) -}) - -test('resetProposal leaves another proposal cached', async () => { - // What makes the deferred eviction safe: by the time it runs, the proposal - // being opened may already have cached data of its own, and dropping that - // would send it straight back to the network. - cacheProposal('7777') - - await leaveProposal('6996') - - expect(cachedFields()).toContain('metadata({"database":{"proposal":"7777"}})') -}) - -test('resetProposal clears the table but keeps the user signed in', async () => { - const store = await signedInStoreShowingRun() store.dispatch(resetProposal()) - expect(store.getState().tableData.data).toEqual({}) expect(selectUserFullName(store.getState())).toBe('Ada Lovelace') }) diff --git a/frontend/pnpm-lock.yaml b/frontend/pnpm-lock.yaml index 129eb136..f02214c9 100644 --- a/frontend/pnpm-lock.yaml +++ b/frontend/pnpm-lock.yaml @@ -552,6 +552,10 @@ importers: version: 5.9.3 packages/ui: + dependencies: + '@damnit-frontend/shared': + specifier: workspace:* + version: link:../shared devDependencies: '@apollo/client': specifier: catalog:graphql