feat(v2-ui): /extractions Perspective grid, review-page retirement#234
Open
JonnyTran wants to merge 24 commits into
Open
feat(v2-ui): /extractions Perspective grid, review-page retirement#234JonnyTran wants to merge 24 commits into
JonnyTran wants to merge 24 commits into
Conversation
…rojection (spec §3.5)
Its only two callers (review-loop.spec.ts, draft-lifecycle.spec.ts) were deleted earlier in this branch, leaving it an unused export. Confirmed no remaining callers before removal.
…le load failures toPerspectiveData() previously fed multi_label_selection/ranking/span cell values (arrays/objects) straight into Perspective, which only infers scalar column types - client.table() could render [object Object] or reject outright. Serialize non-scalar values to a stable JSON string while passing null and genuine scalars through unchanged. That table-construction call also sat outside performLoad's try/catch, so a rejection escaped as an unhandled rejection: loadFailed stayed false while the viewer rendered empty with no explanation. Guard the call and emit a new `load-error` event that the extractions page wires into its existing loadError state cascade.
initPerspective() memoized `ready` unconditionally (`ready ??= ...`), including a REJECTED promise: one failed fetch of the server/viewer WASM (offline blip, a 404 right after a redeploy) permanently bricked every later mount of the extractions grid until a hard page reload. Reset the memo to null when the boot rejects so the next call starts a fresh attempt, while still sharing one in-flight promise across concurrent callers and never re-running after a successful boot.
…d mounts ExtractionsGrid called perspective.worker() directly on every mount. worker() constructs a brand-new Web Worker + WASM server instance each call (confirmed in @perspective-dev/client's worker()/pe() helpers); only Client.terminate() runs the close callback that tears one down, and onBeforeUnmount never called it. Ten visits to /extractions left ten live workers, each with a full WASM heap. Hoist the client into a module-level memo (initPerspectiveClient) alongside the existing WASM-boot memo, mirroring its retry-on-rejection behavior. Exactly one client/worker now lives for the app's session; ExtractionsGrid only drops its local reference on unmount and intentionally never terminates the shared client.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…wer load failures
The datagrid plugin picks `renderTarget = CSS.supports("selector(:host-context(foo))")
? "shadow" : "light"` and, in the shadow case, renders the regular-table and every <td>
inside an attachShadow({mode:"open"}) root. Chromium takes that branch, so the Vue
scoped `:deep()` rules -- which compile to a document-level stylesheet -- could not
reach the cells: banding and the pointer cursor were toggled on every draw and styled
nothing. Inject an id-guarded <style> into the regular-table's own root node instead,
leaving the scoped rules to serve the light-DOM target.
`addStyleListener` only pushes onto regular-table's listener array; it never invokes the
callback nor forces a redraw, and `load()` had already drawn the first frame by the time
we registered. So even once the CSS reached the cells, nothing applied the classes until
a scroll or resize. Apply them once explicitly after registering.
Also: the eject/load/restore/getPlugin catch swallowed every error with no binding, no
log and no emit -- leaving `loadFailed` false with an empty viewer, exactly the silent
failure the load-error contract exists to prevent. Mirror the table-build handler,
including its staleness guard. And release the superseded table on the `!viewer?.load`
early return, the one unwind path still leaking it, now that `table = newTable` has
dropped the last reference to it.
Verified in a real browser: computed cursor `pointer` and the band rule computing to
rgba(0, 0, 0, 0.035) on first paint with zero interaction; style element present once.
`perspective.init_server` returns void, not a promise -- it only stashes what it is handed (@perspective-dev/client/dist/esm/perspective.browser.d.ts:7). So `Promise.all([init_server(fetch(SERVER_WASM)), init_client(fetch(CLIENT_WASM))])` settled on the viewer half alone: a rejected server-WASM fetch never reached the attempt, never triggered the retry-on-rejection reset, surfaced later as an unhandled rejection when worker() awaited the stashed promise, and left `ready` memoized as RESOLVED -- so initPerspective() reported success for a boot that had failed. The retry's headline scenario, a transient fetch blip, was the one case it did not cover. Await both fetches here so either can reject into the attempt, then hand the resolved Responses to init_server/init_client. The specs missed this because their mocks gave init_server a promise-returning signature the real module does not have, so every failure case exercised a path that cannot occur. Type the factories against the real modules and drive the failure specs by rejecting fetch, as production does. Also pin the untested half of the contract: a resolved boot is never re-run.
…ions load `load()`'s null-id early return did not bump `requestToken`, so deselecting the workspace mid-load could not supersede the request already in flight: it still passed its token check and committed the old workspace's projection after the selection was gone, leaving the grid showing a workspace the user was no longer in with no way to clear it. Bump the token and clear the projection. Also add `hasLoaded` so the page can tell "not loaded yet" from "genuinely empty" -- `isLoading` starts false and `load()` only runs after `ensureWorkspaces()` resolves, so every visit briefly rendered the empty state before the spinner -- and drop the `extractions.loading` key, which no caller ever referenced (the page renders BaseLoading, which takes no message).
JSON.stringify does not unconditionally return a string: it returns undefined for a function or symbol, and throws on a BigInt or a circular structure. Either outcome breaks the "every manifest column present on every row, null when absent" contract that toPerspectiveData's doc comment says downstream schema inference depends on -- undefined would leak into a key the contract promises is present, and the throw would escape as a spurious load-error. Server JSON cannot produce these today, but this helper is the one place meant to make that guarantee unconditional.
The seed wrote record fields identical to the annotation values it was meant to prove were coalescing -- fields size="120"/label="control" against a "120" suggestion and a "control" response. A regression that resolved cells from raw record fields instead of coalescing suggestion/response would have passed every positive assertion; only the "intervention" absence check had any power, and it only proved "not the suggestion", never "the response won". Seed distinct field values (999/unset) and assert they are absent, so a field-fallback regression fails loudly. Verified live: 999/unset count 0 while the coalesced 120/control render. Scope the spec's text assertions to the grid's testid rather than unscoped substring matches over the whole document, and validate loadSeed's parsed object so a stale seed-output.json names its own fix instead of timing out on "undefined.notes". The seed's label divergence broke search-roundtrip, which was unmodified: v2 FTS indexes only raw record.fields, never responses. Restore searchability via the unprojected country field rather than weakening the grid's divergence check. Also drop apiToken/apiUrl and the APIRequestContext import, dead since createIsolatedRecord's removal; harden ReviewProvenance's guard to assert against the catalog's structure rather than regexing the whole en.js source (an identically valued pair under any other namespace satisfied it, and en.js already contains a near-miss) and resolve that catalog relative to the spec rather than the runner's cwd.
Member
Author
|
Pushed 5 commits addressing the automated per-commit reviews on this branch (16 jobs, all now closed). Most of their findings were already superseded by later commits here; these are the ones still live at HEAD. The one that matters — row banding and the pointer cursor rendered nothing. Compounding it: Both fixed, and verified in a real browser rather than by unit test (neither is reachable under happy-dom): on first paint with zero interaction, linkable cells compute Also fixed:
Deliberately not changed, flagged instead: the de/es/ja English placeholders (plan-mandated — the open question in the PR description above), the spec §3.5 keep-list flagged as "orphaned" (it's intentional scaffolding for the review drawer), the |
`InternalPage` gave all three of its slots literal string fallbacks ("here is the
header", "here is the page header", "here is the page content"). They were dev
scaffolding, but a slot fallback renders whenever the slot is unfilled -- so every
page that fills only some of the three showed the placeholder as real page copy.
/extractions fills `header` and `page-content` but not `page-header`, and so
rendered "here is the page header" directly above the page title. Found while
recording the extraction-grid demo against the live stack.
Render nothing for an unfilled slot instead. The two pages that do fill
`page-header` (user-settings, dataset settings) are unaffected.
Records the workspace-wide extraction grid against a live backend in headless chromium and composes the recording into an annotated 1080p video with Remotion. It is a demo and a gate: every scene asserts the behaviour it is showing, and a failed assertion exits non-zero and stops the pipeline, so a broken UI cannot be dressed up as a finished video. The composition renders the run's real pass/fail counts, which makes the video a report on the run rather than a claim about it. This is what surfaced the InternalPage slot-placeholder bug fixed in 3f25ea4. The seed is deliberately richer than e2e/v2's minimal fixture -- a table question that fans one reference out to several stacked rows (so banding is visible), a schema with questions but zero records (the coverage map), a human response competing with an agent suggestion, and deliberate holes so absent extractions render blank rather than fabricated. video/ is a separate npm package on purpose: Remotion pulls React 19 and its own toolchain, none of which belong in the Nuxt app's dependency graph. It is excluded from the app's ESLint config for the same reason. Recordings, renders and the generated data module stay untracked -- the composition is a report on one specific run, and committing its data would let stale captions render against a fresh recording.
Member
Author
Demo video:
|
| scene | what it proves |
|---|---|
| 01 sign-in | real bearer token through the actual UI |
| 02 grid | 8 references × 3 schemas denormalized into one flat grid; GET /api/v2/projection → 200 |
| 03 coalesce | agent said cohort, a reviewer submitted cluster-RCT — grid shows the human answer, and cohort appears nowhere |
| 04 gaps | risk_of_bias has questions but zero records: its columns still render, empty. Missing extractions render blank, not fabricated |
| 05 banding | a table question fans one reference out to several stacked rows; banding survives the virtualized redraw after scrolling |
| 06 click | cells resolve back to reference + schema (annotation deep-link still flagged off) |
| 07 swap | switching workspace refetches the projection in place — new columns in, old columns gone |
| 08 empty | an un-extracted workspace gets an explicit empty state, never a blank grid |
The seed (demo/seed_demo_workspace.py) is deliberately richer than e2e/v2's minimal fixture — a realistic malaria systematic review shaped so each of the above has something real to show, including deliberate holes (a missing country, a missing sample size, a reference with no outcomes record).
It caught a real bug
layouts/InternalPage.vue gave all three of its slots literal string fallbacks ("here is the page header", …) left over from dev scaffolding. A slot fallback renders whenever the slot is unfilled, so /extractions — which fills header and page-content but not page-header — rendered "here is the page header" as real copy directly above the page title. Fixed in 3f25ea4, with a regression assertion in the demo driver.
Notes for anyone re-running it
- Perspective's Datagrid renders into a shadow root — a plain
document.querySelectorAll("td")finds nothing. The driver walks every open shadow root instead. - Perspective infers numeric columns, so a digit-only
4812renders as4,812; assertions compare against a separator-stripped copy. demo/video/is a separate npm package on purpose (Remotion pulls React 19 + its own toolchain, which don't belong in the Nuxt dependency graph) and is excluded from the app's ESLint config.- Recordings, renders and the generated
data.tsstay untracked — the composition is a report on one specific run.
📎 Video attached in the next comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2 of the extraction-table plan (
docs/superpowers/plans/2026-07-20-extraction-table.md, Tasks 8–13), stacked on Phase 1 / PR #233 which is already merged todevelop. This is the integration-risk half: the Perspective 4.5.2 WASM wiring, the grid component, the/extractionspage, the review-page deletion, and the replacement e2e gate.Implements spec §3.3 and §3.5 of
docs/superpowers/specs/2026-07-20-extraction-table-design.md.What's here
Perspective 4.5.2 integration (
@perspective-dev/*, all pinned exactly4.5.2; no@finos/*)components/v2/extractions/perspective-bootstrap.ts— module-level memo that initializes the server + viewer WASM engines exactly once and shares oneClient/Workerfor the app's lifetime.nuxt.config.ts—vue.compilerOptions.isCustomElementforperspective-*tags,build.target: esnext, andoptimizeDeps.excludefor the WASM ESM packages.vitest.config.ts+__mocks__/perspective-bootstrap.js— unit tests never touch WASM or custom elements.ExtractionsGrid.client.vue— the<perspective-viewer>wrapper. The.client.vuesuffix keeps Nuxt from ever SSR-evaluating browser-only Web Component / Worker / WASM code. Loads the flat projection rows into a Perspective table, restores a static Datagrid (settings: false, no sort/filter), bands rows by reference via the datagrid'saddStyleListener, and emitscell-clickwith{ cell, reference, schemaId, columnName }./extractionspage —pages/extractions/{index.vue,useExtractionsViewModel.ts}, breadcrumbs, i18n. Standalone route, deliberately not wired into nav orindex.vue.?workspace_id=overrides the selected workspace (deep-load / e2e determinism).Reference-review page retired (spec §3.5) —
ProvenanceandReviewCellwere first extracted verbatim intov2/domain/entities/review/ReviewCell.tsand the kept importers repointed, then the page, its view-model,ProjectionReviewForm,ReviewRecordCard,ReferenceReview.ts,GetReferenceReviewUseCase,ReferenceReviewsStorageand three e2e specs were deleted. The §3.5 keep-list survives intact with its DI registrations.Verification
nuxi typecheck,npm run lint,npm run buildall clean.120from a suggestion,controlbeating a competinginterventionsuggestion). All WASM fetches 200, no console errors./extractions→/schemas/{id}→/extractions) confirmed the shared-client memo survives unmount/remount — the specific risk introduced by the Worker fix below./extractionsroute chunk (211 KB); the entry chunk has zero Perspective references.ANNOTATION_CELL_LINKS_ENABLED === false,V2TableEditor.vueuntouched vs base.Defects found and fixed during review
Worth calling out because unit tests could not have caught most of them — the Perspective lifecycle is unreachable under happy-dom:
multi_label_selection,ranking,spanare arrays/objects) were fed raw into Perspective, which infers scalar column types only —[object Object]at best, aclient.table()rejection at worst. That rejection escaped the guarded path, leaving a blank viewer withloadFailed === falseand no message. Now coerced intoPerspectiveData(scalars andnullpass through unchanged) with aload-errorpath into the page's existing failure state.perspective.worker()ran on every mount, and onlyClient.terminate()terminates the Worker. Client hoisted into the shared bootstrap memo./extractionsfor the whole SPA session — the bootstrap memoized the rejected promise. Now resets on rejection.$ti18n regression: the deletion pass removedreview.response/review.suggestionas "unreferenced", butReviewProvenance.vue— a keep-list file — resolves them via$t(`review.${source}`). Restored, with a regression test that mounts a realcreateI18nfrom the actualen.jscatalog (the repo's$tstub echoes keys, which would have made the test hollow).viewer.eject()before a re-load()instead of an undocumented load-then-delete assumption; the click guard accepting<th>as well as<td>.Notes for reviewers
Three items are flagged rather than decided, because the plan mandates them and I did not want to override it unilaterally:
translation/{en,de,es,ja}.js, so theextractions:block was added to all four with English placeholders in de/es/ja. But no existing v2 block (schemas,review,document,import,errors) appears in de/es/ja at all — this repo's convention is en-only plusfallbackLocale: "en". Review noted the placeholders will silently pass any future "is it translated?" check whereas the fallback leaves the gap visible. Happy to drop the three non-English blocks if you agree.@types/reactdevDependency — works around an upstream packaging bug in@perspective-dev/viewer@4.5.2, whose dist.d.tspoints into its ownsrc/tstree, which imports react types.skipLibCheckcannot help (it skips.d.ts, not the real.tsfile TS falls back to). A narrower ambientdeclare module "react"stub plus a tsconfig paths remap is the tighter alternative.gen:apidrift, pre-existing and environmental. Regenerating now emits"Unprocessable Content"where the committed snapshot says"Unprocessable Entity"— Python 3.13 renamedHTTPStatus(422).phrase. The same mismatch exists at the base commit with no dependency bump on this branch, so the regenerated files were deliberately reverted, not committed, leaving the snapshot matching what other environments produce. Needs a maintainer call.Known coverage gaps, recorded rather than papered over: cell-click plumbing is asserted by nothing until
ANNOTATION_CELL_LINKS_ENABLEDis flipped on; multi-page projections are unit-tested but the e2e seed has a single reference;pages/extractions/index.vuehas no component test. Also pre-existing and not introduced here:save-review-draft-use-case.tsanddiscard-review-use-case.tshave no co-located tests despite the keep-list implying coverage.