feat(v2-ui): /extractions Perspective grid, review-page retirement (ENG-25) - #234
Merged
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.
Introduced a new document detailing acceptance criteria for the extraction projection viewer, outlining specific tests for grid behavior, data handling, and performance under real data conditions. Updated the existing extraction table design document to reference these criteria, ensuring comprehensive coverage of the testing strategy.
…ep row banding" This reverts commit dfc0e35.
Introduced a new navigation tab for extractions in the index.vue file. Updated the tab change handler to utilize a centralized route mapping for better maintainability. This change enhances the user interface by providing direct access to the extractions section.
Updated the CHANGELOG to correct links and removed the license header and Dockerfile for the frontend, as they are no longer needed. Adjusted workspace references in the dataset API mock and test files to reflect the new naming convention. This cleanup enhances project maintainability.
Medium findings from roborev jobs 249, 250, 251, 255, 259 (256's medium was already moot after the dad64f9 revert). - perspective-bootstrap: `fetch` only rejects on a network-level failure, so an HTTP error status resolved normally and `init_server` stashed the error `Response` — the boot memoized as *resolved* and the failure re-emerged as an unhandled rejection from `worker()`. Both fetches now assert `response.ok`. - useExtractionsViewModel: the in-flight dedupe matched on workspace id alone, so a deselect-then-reselect of the same workspace adopted the superseded promise, issued no request, and left the page on the empty state for a workspace with data. The guard now requires token currency. - demo harness: page errors could not fail the gate (a green "N/N passed" video on an uncaught app exception), and `rm -rf "$OUT"` was unguarded on a documented env knob. Page errors are now fatal (DEMO_ALLOW_PAGE_ERRORS=1 to override) and deletion requires a marker file the script itself creates. - grid-adapter: record why the cell-click chain is retained ahead of its consumer (ENG-32) so the next cleanup pass doesn't remove it a second time. - cellMetaAt: correct a comment that misdescribed row-header metadata as carrying `column_header`. Adds specs for the three gaps these expose: a resolved-but-!ok WASM fetch, the reselect race, and — via a fake `regular_table` in a real shadow root — the banding/linkable classes landing on the first draw. Each was mutation-verified to fail without its fix. 915 unit tests pass, lint clean, `nuxi typecheck` exit 0.
Four parallel audits against HEAD classified doc content as discarded (abandoned/reversed and now contradicted by code), outdated (names code that does not exist), or vacuous (cannot fail). Removals only — no rewrites. Every finding was verified against `file:line` evidence before deletion. Discarded — the reference-review vertical (deleted by 293466a, superseded by the server-enriched projection + /extractions grid): - design spec §7 "Decision 5 — Reference-agnostic ProjectionReviewForm"; its ledger item shipped as GET /v2/projection. The 3 contract gotchas it recorded survive in 2026-07-20 §2, the other 2 in the plan (line 22) and apiErrors.ts. - plan Task 15 (/references page), the ReviewRecordCard + ProjectionReviewForm test blocks, e2e scenarios 2-4 and their seam-C preamble, the 9 dead review.* i18n keys, and the file-structure entries for all of the above. Discarded — the /extractions nav decision, reversed by b8651b0: - both spots forbidding nav wiring (plan Tasks 10/13) and the §5 ledger entry listing it as unbuilt; extractions-nav.spec.ts now gates the opposite. Discarded — the perspective.worker() calling convention, replaced by the memoized initPerspectiveClient (146f5ff) and prohibited in-code: the two prose references plus the __mocks__ and vi.mock blocks encoding it. Vacuous: - AC4's "reference repeats rather than being merged" — guaranteed three times over (required Pydantic field, unconditional adapter line, and Perspective having no cell-merging at all), so nothing could violate it. - AC3's "no assertion is made about cross-schema row correspondence" — a constraint on the test author, not the system; it also contradicted the closing section, which claimed the criterion asserts it. - the e2e scenario-1 "CORS preflight" claim — Nitro's devProxy makes every /api/v2 call same-origin, so no preflight is ever issued. - plan Task 12's e2e listing, whose assertions 9ab8d52 already replaced for being unfalsifiable, and Task 9's chunk-size check that Task 13 never runs. - useExtractionsViewModel's "does not navigate (guard off)" test: its only assertion duplicated grid-adapter.test.ts, and its actual claim was unasserted — flipping the flag or deleting the guard both left it green. 914 unit tests pass, lint clean, `nuxi typecheck` exit 0.
The component shipped as `ExtractionsGrid.client.vue` (the `.client` suffix is load-bearing — it is what makes Nuxt treat it as client-only, which the Perspective WASM boot requires), but the plan still names the pre-rename `ExtractionsGrid.vue`/`.test.ts` throughout. Two `npx vitest run` commands and a `git add` were broken as written. Every path in a runnable command in this plan now resolves at HEAD.
The prune in 50d4d2a over-reached: `notApplicable` was deleted along with the genuinely-dead `review.*` keys, but it is live — `translation/en.js:147`, consumed by `ReviewCellInput.vue:9` via the `ReviewCell.notApplicable` flag. The closing `},` went with it, leaving the code fence on an unterminated object literal. The snippet is now byte-identical to the `review: {…}` block in en.js.
Addresses the remaining open findings from roborev 214 (a review of the
extraction-table plan doc, re-checked against the shipped code).
- latest_responses ordered only by updated_at, which TimestampMixin defaults
to datetime.utcnow, so two users submitting back-to-back could tie and the
winning envelope was whatever order Postgres returned. Load response_id
into DuckDB and append it to the ORDER BY, mirroring what effective_records
already does with record_id.
- Add test_table_fanout_through_the_response_path: every fan-out test seeded
a suggestion, leaving the double-wrapped {q: {"value": [...]}} response
envelope unwrap uncovered.
- Document on build_reference_view that its user-scoping intentionally
differs from build_workspace_view's any-user coalescing.
roborev 268 (review of 0cb1ad7): - Pin explicit distinct `updated_at` on test_latest_submitted_response_any_user_beats_suggestion: it relied on the TimestampMixin default, so both rows tied and the assertion rode the new id tiebreaker rather than the ordering rule it names. - Add test_updated_at_dominates_the_response_id_tiebreaker, where the lower id carries the later timestamp, so the two keys' precedence is pinned. Both keys are now independently mutation-checked. - Document that the tiebreaker buys stability, not latest-ness: on a tie the winner is the greatest response_id, deterministic but arbitrary. roborev 266: the onCellClick spec asserted only the URL, which holds on both branches of the ANNOTATION_CELL_LINKS_ENABLED guard. Stub the window.location href setter and assert it is never written, so flipping the guard fails it. roborev 262: - Fold consoleErrors into the demo's fatal condition behind DEMO_ALLOW_CONSOLE_ERRORS; ExtractionsGrid reports its worst failure via console.error, so collecting these without gating on them read as a signal they weren't. - Clear document.body after each ExtractionsGrid spec; fakeRegularTable leaked shadow hosts into the shared happy-dom document. - Flush the scheduler explicitly after each saveSelectedWorkspace in the reselect spec instead of relying on an incidental microtask drain. - Document that assertOk covers the status, not the bytes: reading the body would close that hole but give up streaming WASM compilation on every boot. roborev 260: - Give each tab its own optional `route` and drop NAVIGATION_TAB_ROUTES: two independent sources of truth let a route exist with no tab, or a tab route nowhere and render a blank panel. - Add pages/index.test.ts covering push-vs-panel without mounting the DI-heavy page; the mapping was pinned only by the separate v2 e2e project. - Scope the e2e heading assertion to the page's own h1 — the breadcrumb trail also carries an "Extractions" crumb. - Drop the untranslated `extractions` blocks from de/es/ja so the `en` fallback renders identically and the gap is visible to translation owners.
roborev 269: - Add the `delete window.location` fallback to the onCellClick spec's teardown and assert the real location is back afterwards. happy-dom 20 exposes `location` as an own configurable accessor, so the existing restore did run (see the review comment) — but the assertion makes a leaked stub fail loudly instead of staying inert until some later spec reads it. - Derive the panel tab ids in pages/index.test.ts from index.vue's own `activeTab.id === '...'` branches instead of hand-copying them. The copy was asymmetric: adding a branch failed the test, but removing or renaming one left it green while that tab rendered an empty panel. - Restore the text assertion the e2e heading check lost when it was scoped to the BEM class, so a broken `extractions.title` lookup fails again. - Gate the demo only on console errors it owns (ExtractionsGrid/grid-adapter). The harness drives `npm run dev`, so devtools and HMR share the `[error]` channel; every one being fatal meant unrelated noise failed the pipeline and the only escape switched off the signal the gate exists for.
The reference-review page that consumed these was removed by 293466a, leaving the whole widget layer with zero consumers. A repo-wide grep for the three component names found exactly one hit outside the subtree: a comment. Deleting the components alone would have left four domain modules orphaned, so they go together. Components: ReviewCellInput.vue, ReviewProvenance.vue, V2TableEditor.vue. Domain: widget-adapters.ts, widget-mapping.ts, SuggestionHint.ts, ReviewCell.ts. Each with its spec. Kept: response-values.ts (live via AnnotationRepository) and the three review use-cases still registered in DI. V2TableEditor was a fourth independent Tabulator boot alongside RenderTable.vue and BaseSimpleTable.vue, re-imported globally-loaded tabulator.min.css, and shipped none of BaseSimpleTable's theme overrides - so it would have rendered off-brand the moment anything used it. v2 has since standardised on Perspective. Also drops the five review.* i18n keys that lose their last consumer (en.js only; de/es/ja never carried them), and amends the keep-list in the extraction-table plan. ENG-32 is updated to say the widgets are a rebuild, not an adaptation - its description previously told the implementer to build on files this commit deletes. Tests: 919 -> 883, zero failures.
Three defects, all shipped together:
1. The color map named --fg-status-active and --fg-status-danger. Neither
exists in assets/css/themes.css - the defined tokens are
--fg-status-{pending,draft,discarded,submitted}. Every entry fell
through to its var() fallback, so every status rendered the same grey.
`published` and `completed` have no token of their own and now share
--fg-status-submitted: both are the terminal "done" state and they
never appear in the same table (schemas renders draft|published,
records renders pending|completed|discarded).
2. The badge rendered the raw server status string. It now translates via
v2Status.*, a new key family covering both vocabularies the badge
spans. Deliberately not v1's recordStatus.*, which is disjoint.
3. V2RecordsTable declared a required workspaceId prop that nothing
referenced, left over from the reference-review page deleted by
293466a. Its call site passed `schema?.workspaceId ?? ''` - and
Schema has no workspaceId field, so it was always ''.
The old spec could not see defect 1: it compared the two *declaration
strings*, which differ, while the rendered colors were identical. The
rewrite asserts the resolved token per status AND that every token named
is defined in all three of themes.css's blocks - the second check is what
makes a nonexistent token visible. Verified by reintroducing the bug:
both assertions fail.
Tests 883 -> 899, lint and typecheck clean.
One component tree instead of two. After the dead review/table subtree went, components/v2/ held only common/, extractions/ and schemas/ - all three move, so the tree is gone entirely. common/V2Empty.vue -> features/global/ common/V2StatusBadge.vue (+test) -> features/global/ extractions/ExtractionsGrid.client.vue (+test) -> features/extractions/ extractions/perspective-bootstrap.ts (+test) -> features/extractions/ schemas/V2RecordsTable.vue -> features/schemas/ global/ is the documented home for components used across pages (docs/structure.md) and V2Empty has 8 usages over 4 pages; extractions/ and schemas/ are new feature roots. All git mv, so history stays followable. No template usage changed: nuxt.config.ts registers components with pathPrefix: false, so every <V2Empty>/<ExtractionsGrid> still resolves by bare name. Three places hard-coded the old path and did need editing - the vitest alias for perspective-bootstrap, the vi.mock on the same specifier, and ExtractionsGrid's own ~/-absolute import. The .client suffix is preserved: it is what keeps Nuxt from SSR-evaluating the WASM boot. Note on the vitest alias: it is currently redundant with ExtractionsGrid.client.test.ts's own vi.mock - no other spec mounts the grid (useExtractionsViewModel.test.ts only mentions it in a comment). It still had to be updated, because a stale alias key is silently inert rather than an error, which is exactly how this would bite later. Verified: 899 tests pass, lint clean, nuxi typecheck clean, npm run build succeeds, playwright --project=v2 --workers=1 is 4/4, and the duplicate basename check stays empty. Browser-checked /schemas, /schemas/[id] and /extractions in light and dark: badges keep their per-status colors and the Perspective grid still boots and paints.
The prefix was only ever a flat-namespace collision guard while a parallel components/v2/ tree existed (see the vertical-slice plan). Inside features/ it reads as leftover scaffolding, and all three target names were free. V2Empty -> Empty (BEM root v2-empty -> empty-state) V2StatusBadge -> StatusBadge V2RecordsTable -> RecordsTable (BEM root v2-records-table -> records-table) Each rename covers the file, the `name:` option, the BEM root class, every template usage, and the stub keys in pages/schemas/index.test.ts - the last of those being the easy one to miss, since Vue does not error on a stub key that matches nothing. Confirmed they are load-bearing here: renaming the Empty stub key back fails the empty-state test rather than silently passing. Kept as its own commit so a rename never has to be untangled from the move. Verified: 899 tests, lint, nuxi typecheck, npm run build, and playwright --project=v2 --workers=1 at 4/4; badge colors byte-identical to the pre-rename run across light/dark/high-contrast.
…eep-list
Jobs 271, 272, 274.
The one that mattered (272, Medium): the badge looks its label up
dynamically, so no literal "v2Status.draft" exists for a grep and the
spec's $t stub never consults a catalog - deleting the whole v2Status
block from en.js left all 899 tests green while every badge rendered a
raw key. This branch already lost the review.* keys that way twice. The
spec now asserts en.v2Status's keys against STATUS_TOKENS.
Also in that spec: the themes.css check counted total `--token:`
occurrences against a hardcoded 3, so three copies inside :root would
have passed, and its "restructure guard" matched block headers by regex -
ambiguous, because themes.css carries a second [data-theme="dark"] and
[data-theme="high-contrast"] block that only set color-scheme. Replaced
with a parser that splits the file into selector/token blocks, keeps
those defining --fg-status-*, pins the selector list and asserts each
token per block by name. The distinctness test compared `var(--token)`
strings - the same gap the previous commit claimed to have closed - and
now compares resolved values per block. __dirname (the only use in any
spec) is now fileURLToPath(import.meta.url).
Every new guard verified by mutation: removing the v2Status block,
dropping --fg-status-draft from the high-contrast block alone, and making
draft equal submitted in :root each fail exactly one intended test.
274: Empty -> EmptyState. `Empty` is single-word, which Vue reserves and
vue/multi-word-component-names would reject - that rule is off only for
Nuxt's single-word pages/layouts, not as a policy for components. It also
matches the empty-state BEM root already introduced. This departs from
the name in the approved plan; flagged rather than silent.
274 also asked for stub-effectiveness assertions. Note that the suggested
findComponent({ name }).exists() does not work: shallowMount auto-stubs
unmatched children while keeping their names, so it passes either way
(verified). The assertions check content only the keyed stubs render, and
both fail when their keys are staled.
271: three documentation findings, addressed in the extraction-table
plan's keep-list rather than as code comments - it now states why the
review use-cases were kept while the widgets were deleted (server
contract vs. a UI to be redesigned), and records that ColumnMeta.review
plus the components/base/inputs/ leaves are deliberate ENG-32 groundwork
rather than dead weight.
902 tests, lint clean, typecheck clean, build clean, v2 e2e 4/4.
JonnyTran
added a commit
that referenced
this pull request
Jul 28, 2026
Columns now come from GET /datasets/{id}/fields, search from v1's
Elasticsearch-backed records/search (authoritative total). Deletes
AnnotationRepository and the three review use-cases, orphaned since #234,
and the rebuild-index button (reindex is a CLI). Also drops the now-dead
gen:api codegen scripts and the two CI gates that diffed against the
deleted v2 OpenAPI snapshot/generated client.
JonnyTran
added a commit
that referenced
this pull request
Aug 5, 2026
* docs(plan): reorder the v2 fold — delete the v2 tree before building the v1 model
models/v2/schemas.py already binds __tablename__ = "schema_versions" on the
shared DatabaseModel metadata, so the new v1 SchemaVersion cannot be declared
while it exists. Tasks 9-11 (deletions) become Tasks 1-3; the build tasks shift
to 4-11; 12-15 are unchanged.
Task 1 now snapshots the v2 tree to a git-ignored workspace dir and tags
v2-pre-fold, because every later task's "Reference: .../v2/..." path is deleted
before that task runs. Deletion boundaries were also redrawn so each task leaves
a green suite: the tests that import contexts/v2 die with api/schemas/v2 and move
into Task 1; SchemaVersionFactory is deleted in Task 3 and re-added in Task 4.
* refactor(server)!: delete the /api/v2 surface
Removes api/v2, api/schemas/v2, SchemaPolicy and the three V2*Policy classes
(they reproduced DatasetPolicy/QuestionPolicy/ResponsePolicy predicate for
predicate). openapi_dump now dumps v1.
Also:
- Retarget tests/integration/conftest.py's async_client override onto api_v1
(it was previously registered on api_v2, so test_rq_groups_workflow.py was
never actually getting the test session for its v1 routes).
- Fix test_rq_groups_workflow.py: its fixtures referenced a non-existent
`async_db` param (should be the root conftest's autouse `db` fixture) and
built Workspace() with title/description kwargs the model no longer has -
both bugs were masked because every test errored at fixture setup before
reaching them. Skip the two tests that hit a separate, pre-existing
SQLite single-writer lock ('database is locked') caused by
create_document_workflow() opening its own AsyncSessionLocal() connection
outside the test's nested transaction - unrelated to this fold, needs a
session-injection seam or different isolation strategy.
- Update test_openapi_dump.py assertions from the v2 schema shape to v1's.
- Add tests/unit/api/test_api_mounts.py pinning that only /api/v1 is mounted.
* fix(server): make test_create_document_workflow_with_rq_groups pass for real
Task 1 review flagged two skipped tests in test_rq_groups_workflow.py as
unaccountable. Root cause (confirmed correct by review): create_document_workflow()
opens its own AsyncSessionLocal() connection, which deadlocks under SQLite's
single-writer lock against the db fixture's nested-transaction connection.
Without touching production code, add a `use_fixture_session_for_workflow` fixture
that patches AsyncSessionLocal at the call site to return the test's own db session
(neutralizing db.close so the shared session survives past create_document_workflow's
`async with` block). This makes test_create_document_workflow_with_rq_groups pass for
real - no longer skipped.
test_concurrent_workflow_processing cannot be fixed the same way: it runs three
create_document_workflow() calls concurrently via asyncio.gather, and a single
AsyncSession cannot be used by overlapping coroutines (confirmed empirically -
sqlalchemy.exc.IllegalStateChangeError: "bind() is already in progress"). Kept
skipped, now citing ENG-37 (test-session architecture: workflows open their own
AsyncSessionLocal) as the tracked follow-up.
* refactor(server)!: delete contexts/v2, validators/v2, and cli/index
The LanceDB engine in index/ is kept untouched; only its v2 glue goes.
Registering it as a SearchEngine implementation is ENG-36. Drops the
no-index-import guard, which is what made v2 review data unsearchable.
* refactor(server)!: delete models/v2
Schema folds into Dataset; V2Record/V2Question/V2Response/V2Suggestion fold
into records/questions/responses/suggestions. Frees the schema_versions
table name for the v1 SchemaVersion that lands next.
* feat(server): fold v2 schema/record columns into v1 models
Adds SchemaVersion (FK datasets), Dataset.current_schema_version_id,
Record.reference, FieldType.column, and Field.__upsertable_columns__.
Replaces the four v2 migrations with one; drops columns_cache and
review_widgets, which the fields table supersedes.
* fix(server): address roborev findings across the v2 fold (jobs 277-282)
Migration history rewrite (HIGH, job 282): the four deleted revisions are live on
origin/develop, so any database migrated before this branch strands alembic_version
at c1510e93882a. Keeps the rewrite per the plan's pre-production constraint but ships
the recovery path — 13da2d87e660's docstring and a new CLAUDE.md section cover rebuild
and stamp-forward, including the Postgres schema_versions name collision. Verified
upgrade -> downgrade -1 -> upgrade round-trips on SQLite.
Table-question suggestion scores (job 280): ports the carve-out deleted with
validators/v2/values.py into SuggestionCreateValidator._validate_score, so a multi-row
table value keeps its single whole-suggestion confidence score instead of 422ing.
Not reachable through v1 schemas yet — SuggestionCreate.value has no table-row variant —
so the tests drive the branch with list[str] and the module says so.
SchemaVersionFactory (job 282/281): object_key dereferenced a SubFactory inside a
LazyAttribute, which sees an un-awaited coroutine. Derived from version only, comment
restored, pinned by a test that actually calls the factory.
Test hygiene: test_api_mounts no longer runs create_server_app (base_url wrapper +
configure_app_statics temp-dir leak) and filters on Mount; the rq-groups test asserts
the commit via a spy (mutation-verified); both conftests pop only their own
dependency_overrides keys so ordering under -p randomly is safe.
Also: passive_deletes on Dataset.schema_versions, ColumnFieldFactory dtype str ->
string (with the same correction applied to every dtype literal left in the plan),
narrowed pytest.raises(Exception), openapi-dump help repointed to v1, index/__init__
documents its ENG-36 parking and the Lance layout break, empty test packages removed.
Plan updated for findings binding later tasks: dtype strings, the dropped
get_s3_client override (Task 7), and the empty-pandera-body 500 (Task 6).
* feat(server): add FieldType.column — indexed, deliberately unvalidated
Column fields declare a Pandera dtype that types the ES mapping without gating
ingestion; no validator collector selects them. Editable columns are reviewed
via a Question bound to them.
* fix(server): guard ColumnFieldSettingsUpdate.nullable against explicit null
nullable is unguarded while ColumnFieldSettings.nullable is non-Optional: a
PATCH body of {"type": "column", "nullable": null} passed validation, then
Field.fill() dict-merged nullable: None into stored settings JSON, breaking
every later parse of that field via Field.settings. Add "nullable" to
__non_nullable_fields__, cover ColumnFieldSettingsUpdate directly (dtype-only
and review-only partial updates, explicit-null rejection for both fields),
and rename a validator test to match its body after Task 5 review.
* feat(server): contexts/schema_versions -- publish a version, derive column fields
Replaces contexts/v2/schemas.publish_version. columns_cache and review_widgets
are gone: the body's columns become Field rows, the widget overlay rides in
Field.settings['review'].
* feat(server): schema-version endpoints on /api/v1
Replaces POST/GET /api/v2/schemas/{id}/versions. GET /schemas/{id}/columns is
dropped: the derived columns are readable from GET /datasets/{id}/fields.
* feat(server): carry record reference through v1 bulk create/upsert and list
Replaces V2Record.reference. Drops the schema_version_id pin (its CASCADE
silently deleted records) and status=discarded (record status is derived from
response distribution; discard is a response status).
Also wires reference through the single-record PATCH /api/v1/records/{id}
path (contexts/records.py::update_record) under the same is_set(...)
semantics, closing a silent no-op gap the new RecordUpdate.reference field
would otherwise leave on that endpoint. Updates the pre-existing full-dict
response assertions across test_records.py, test_datasets.py,
test_list_dataset_records.py, and friends to include the new reference key.
* test(server): prove explicit-null clears record reference on upsert and PATCH
Task 8 review found the omitted-vs-explicit-null distinction for
Record.reference was only inferred by analogy to metadata's is_set
semantics, not proven. Adds the missing null-clears-value case on both
paths: bulk upsert (PUT .../records/bulk) and single-record PATCH.
* feat(server): bind v1 questions to schema columns via settings['columns']
Replaces V2Question.columns and validators/v2/questions.QuestionBindingValidator,
retargeted from SchemaVersion.columns_cache to the dataset's column fields.
* test(server): cover the PATCH /questions column-binding path
The create-path suite (POST /datasets/{id}/questions) never exercised
QuestionColumnBindingValidator via QuestionUpdateValidator, so a regression
in the update call site (or the selectinload(Dataset.fields) eager-load
fix) would go undetected. Add PATCH coverage: valid binding persists
(re-read from DB), invalid binding is rejected and not persisted, and the
scalar arity rule holds on update too.
* feat(server): move the workspace projection onto v1 tables
DuckDB denormalization SQL is unchanged. Adds the schema-backed discriminator
(Dataset.current_schema_version_id IS NOT NULL) so plain annotation datasets
in the same workspace do not leak into the extraction grid.
* test(server): pin the record-status and index side effects v2 omitted
* chore(server): one migration chain, no stale v2 comments
* refactor(frontend)!: repoint the v2 data layer at /api/v1
Columns now come from GET /datasets/{id}/fields, search from v1's
Elasticsearch-backed records/search (authoritative total). Deletes
AnnotationRepository and the three review use-cases, orphaned since #234,
and the rebuild-index button (reindex is a CLI). Also drops the now-dead
gen:api codegen scripts and the two CI gates that diffed against the
deleted v2 OpenAPI snapshot/generated client.
* fix(server): serialize current_schema_version_id on Dataset response
Task 4 of the v2-fold plan added current_schema_version_id to the
Dataset model but no task added it to the v1 Dataset response schema,
so GET /api/v1/datasets/{id} and GET /api/v1/me/datasets never
serialized it — breaking the frontend's schema-backed-dataset filter
on the /schemas page.
DatasetGetterDict needs no new branch: the field name matches the ORM
column exactly, so pydantic's default GetterDict.get(key) -> getattr
fallback already resolves it.
* refactor(frontend)!: fold v2/ into v1/ — one DDD tree
Merges the DI containers, drops every V2 name prefix (V2Record ->
SchemaRecord, V2RecordRepository -> SchemaRecordRepository), and deletes v2/.
* test(e2e): repoint the extraction e2e suite at /api/v1 (steps 1-2 only)
Rewrites seed_v2_e2e.py's calls onto the v1 endpoints per the fold-v2-into-v1
plan's Task 15: dataset CRUD via /api/v1/datasets, publish via POST
.../schema-versions, records via PUT .../records/bulk, suggestions via PUT
/api/v1/records/{id}/suggestions, responses via POST /api/v1/records/{id}/responses.
Drops the v2 :rebuild-index call (v1 indexes on write).
Question creation had to move before the schema-version publish call:
QuestionCreateValidator rejects question creation once a dataset is `ready`,
and publishing a schema version sets `ready` as a side effect. Column
bindings for the `size`/`notes` text questions are therefore added via a
PATCH /api/v1/questions/{id} after publish, once the column Fields the
binding validates against actually exist -- QuestionUpdateValidator, unlike
the create validator, doesn't gate on dataset readiness. This ordering
constraint isn't reflected in the task-15 brief's mapping table; the real v1
validators win.
Renames the Playwright project v2 -> extraction (playwright.config.ts) and
git-mv's e2e/v2 -> e2e/extraction, updating every referrer found by grep:
package.json's e2e:v2* scripts, e2e/extraction/README.md, fixtures.ts and
CLAUDE.md doc comments, the demo/ run script and its READMEs, .gitignore's
seed-output.json pattern, and a comment in pages/index.test.ts.
Steps 3+ (live-stack verification) are explicitly out of scope for this
commit -- local Elasticsearch is wedged at its shard cap and needs clearing
first. Not run: server start, frontend dev server, or Playwright.
* test(e2e): repoint the two remaining /api/v2 waitForResponse matchers
auth-smoke.spec.ts and extractions-grid.spec.ts still asserted against dead
v2 routes (/api/v2/schemas, /api/v2/projection) after the Step 2 rename --
carried along by git mv but never repointed. Both would hang until
Playwright's timeout against a live v1-only server, a guaranteed red
unrelated to the product.
Repointed against the frontend's actual v1 callers (confirmed against the
v1 handlers, read-only):
- SchemaRepository.getSchemas() -> GET /api/v1/me/datasets?workspace_id=...
(extralit_server/api/handlers/v1/datasets/datasets.py:63, list_current_user_datasets).
Matched with a regex instead of .includes() so a "/me/datasets" sub-route
(e.g. .../metrics) can't be mistaken for the list call.
- ProjectionRepository.getWorkspaceProjection() -> GET
/api/v1/me/datasets/projection (.../api/handlers/v1/projection.py:20,
registered ahead of datasets_v1.router specifically so this static path
isn't swallowed by /me/datasets/{dataset_id}).
Also updates the /api/v2 mention in auth-smoke.spec.ts's header comment.
* fix(server): fix final-review Criticals in schema-versions publish/republish
Two Critical defects found by the whole-branch review that every per-task review
structurally missed (they only surface when a real search engine touches the ORM,
and the suite's mock_search_engine never does):
- Critical 1: POST /datasets/{id}/schema-versions raised MissingGreenlet because
dataset.fields was unloaded (or, with a naive eager-load fix, stale) when
create_index iterated it. Fixed with db.refresh(dataset, ["fields"]) between the
commit and the index call. Pinned by a hand-written fake SearchEngine that
actually reads dataset.fields, verified to fail with MissingGreenlet when the
refresh is reverted.
- Critical 2: republishing (a second schema version, or a first schema version on
a dataset already published via PUT /datasets/{id}/publish) always failed
because create_index is not idempotent. Added SearchEngine.index_exists (new
abstract method, shared implementation for both backends) and guard
publish_version's create_index call with it -- an explicit existence check
rather than swallowing a 400, so a genuine mapping error on first create still
surfaces. Also enforces column dtype immutability on republish per product
decision (schemas are immutable, like LanceDB), and adds the dataset.published
webhook notification publish_version was missing.
Also: excludes NULL Record.reference from the workspace projection's count/paging
queries (v1's Record.reference is nullable, unlike v2's), rewords a validator
docstring whose overclaim ("every handler already preloads it") is exactly the
kind of claim that let Critical 1 through review, adds admin/cross-workspace
policy coverage for the schema-versions endpoint, marks demo/seed_demo_workspace.py
as broken against the v1 fold with a hard failure instead of a confusing one, and
files docs/superpowers/plans/2026-07-26-fold-followups.md for what's deliberately
still deferred (ES mapping evolution, field pruning, name collisions, the demo
script repoint, and the remaining ledger minors).
Suite: 1620 passed / 3 failed (same pre-existing baseline) / 68 skipped, run with
--ignore=tests/unit/search_engine (local ES wedged at its shard cap; unaffected --
every new test uses a mock or fake engine, never a live cluster).
* fix(server): refresh all four relationships create_index reads, not just fields
Re-review of the prior fix (a1f77c0) found Critical 1 was only half-fixed: the
endpoint still 500s. `_configure_index_mappings` (search_engine/commons.py)
iterates FOUR relationships -- fields, metadata_properties, vectors_settings,
questions -- but the previous fix only refreshed `dataset.fields`. The handler
still loads dataset with only Dataset.workspace eagerly loaded, so the next
relationship touched (metadata_properties) still raised MissingGreenlet.
Fix: widen the post-commit refresh to
db.refresh(dataset, ["fields", "metadata_properties", "vectors_settings", "questions"]).
Also widened the Critical-1 pinning test: the previous fake engine only read
dataset.fields, which is exactly why it passed against a still-broken endpoint.
The new _RealMappingSearchEngine delegates to the REAL
ElasticSearchEngine._configure_index_mappings (a pure function of the ORM object,
no live cluster needed) instead of hand-picking which relationships to check, so
it automatically covers any relationship that method reads today or in the
future. Mutation-verified: narrowing the refresh back to just ["fields"]
reproduces MissingGreenlet in this test.
Also fixes a Minor the re-review found in the prior fix itself:
publish_version fired the dataset.published webhook unconditionally, so every
republish (version 2..n) re-fired it for an already-ready dataset --
contexts/datasets.py::publish_dataset can only fire once because it's
draft-gated by DatasetPublishValidator. Capture whether the dataset was already
ready before the update and skip the notify if so. Mutation-verified the same
way. New tests at both the context and handler level assert a republish leaves
the webhook queue with exactly one job, from the first publish.
Suite: 1622 passed / 3 failed (same pre-existing baseline) / 68 skipped, run
with --ignore=tests/unit/search_engine (ES still wedged; unaffected -- these
tests use a fake engine backed by the real mapping-construction code, never a
live cluster).
* fix: address open roborev findings across the v2-into-v1 fold
Triaged all 19 open roborev reviews on this branch (jobs 283-301, one per
commit) and closed them. About half their findings were already stale --
fixed by a later commit on the same branch. This lands the six that were
live, unambiguous, and introduced by the fold itself.
Server:
- `publish_version` now emits `DatasetEvent.updated` on a republish. Gating
`published` on the draft -> ready transition (652a0b6) left versions 2..n
emitting nothing at all, so a consumer subscribed to both events learned
about version 1 and never heard that later versions existed.
- Corrected the `index_exists` guard's comment. It said a republish-added
column "is not yet queryable"; the real consequence is deterministic and
worse -- `"dynamic": "strict"` plus `_map_record_fields_to_es` emitting an
entry for every `dataset.fields` row means the next record write is
rejected with `strict_dynamic_mapping_exception` and
`PUT /datasets/{id}/records/bulk` fails outright.
- `_ES_TYPE_BY_COLUMN_DTYPE` gains `"boolean"`. pandas emits `bool` for a
numpy bool column and `boolean` for the nullable extension dtype; only the
former was mapped, so the same logical type took two incompatible ES
mappings depending on which spelling the Pandera body used.
- Unskipped `test_list_dataset_questions` (its `use_table` skip reason no
longer held) and added the webhook assertions that pin the
PUT /publish-then-schema-version flow, the case that distinguishes the
`was_already_ready` guard from a "this is not version 1" alternative.
Frontend:
- Renamed the schema-slice question entity to `SchemaQuestion` /
`SchemaQuestionType` / `SchemaQuestionOption`. The fold left two
`export class Question` and two incompatible `QuestionType` symbols side
by side under `v1/domain/entities/`, both reachable by absolute imports
differing only by directory -- the exact ambiguity the `SchemaRecord`
rename was made to avoid.
- Fixed the e2e auth-smoke matcher, which armed its `waitForResponse` before
`signIn` and so latched onto `DatasetRepository`'s param-less
`/api/v1/me/datasets` call from the post-login landing page rather than the
schemas page's. Dropped a dead `/references/` guard and a stale
"first bearer-token client" rationale.
Two High findings need a product decision and are recorded in the follow-ups
doc rather than fixed: annotators losing read access to `/schemas/{id}` (the
v1 record list/search routes are admin-only where the deleted
`SchemaPolicy.list_records` was member-readable, and there is no `/me/` twin
for the list path), and `publish_version` bypassing `DatasetPublishValidator`
so a `ready` dataset with zero questions is creatable and then permanently
unconfigurable. All surviving findings are carried into
docs/superpowers/plans/2026-07-26-fold-followups.md sections 7-9.
Verified: 52 targeted server tests pass; full server unit suite shows zero
new failures against a clean-HEAD baseline (46 pre-existing failures before
and after, identical sets -- local ES/env gaps and known-failing JWT tests).
Frontend vitest passes for all touched files and `nuxi typecheck` reports no
new errors; 7 perspective-bootstrap failures are an unmet `@perspective-dev/*`
dependency, pre-existing and unrelated.
* refactor(server): publish_version no longer flips dataset.status
`PUT /datasets/{id}/publish` becomes the sole draft -> ready transition and
the sole `create_index` caller, so a schema-backed dataset gets the same
lifecycle, the same `DatasetPublishValidator` checks and the same index
creation path as an annotation one. The working order needs no PATCH-after
dance:
POST /datasets -> draft
POST /datasets/{id}/schema-versions -> columns materialized, still draft
POST /datasets/{id}/questions -> settings["columns"] bound inline
PUT /datasets/{id}/publish -> ready + create_index
PUT /datasets/{id}/records/bulk
Removed as consequences rather than as separate edits:
* the `was_already_ready` webhook branch -- a schema version is a dataset
mutation, so it always emits `updated`; `published` belongs to
publish_dataset alone.
* the `create_index` call and its `index_exists` guard. Under the new
ordering a schema-version publish on a draft would create the index early
and make the subsequent `PUT /publish` fail with
`resource_already_exists_exception`, so removing it was required.
* the four-relationship `db.refresh` and its comment. It existed only to
feed `create_index`; `build_dataset_event` re-selects with its own eager
loads, so the webhook never depended on it.
`_reject_dtype_changes` becomes `_reject_incompatible_columns`: one query,
one pass, three rules before any write -- annotation-field name collision
(followups sec 3), dtype immutability, and no new columns once the dataset
is `ready` (followups sec 1, option (b): the index mapping is
`"dynamic": "strict"` and nothing evolves it, so a column added
post-publish would leave the dataset unwritable at the next record write;
the 422 keeps the failure at the call that caused it).
Corollary: an annotation dataset already published via `PUT /publish`
cannot retroactively become schema-backed, since every column of a first
version is a new column. Pinned by a test rather than left latent.
Also deletes `ColumnFieldSettingsUpdate` and drops `column` from the
`FieldSettingsUpdate` union -- `PATCH /fields/{id}` could change a column's
dtype out of band, contradicting the immutability enforced at publish.
Columns are derived from the Pandera body; republish instead. No production
callers existed. Closes followups sec 9's untested dict-merge path by
removing the path.
Closes followups sec 8 (option (c)), sec 1 (option (b)), sec 3, and sec 9's
ColumnFieldSettingsUpdate item. Net -37 lines of server source while adding
two safety checks.
Verification: 47 targeted tests pass. Full server unit suite shows 118
failures before and after against a stashed clean baseline -- identical
sets, only randomized parametrize ids differ. Seed script compiles; not run
against a live stack.
* fix(server): address CodeRabbit review on PR #236
Five of the ten findings were real. Two were not; see the bottom.
**Serialize schema-version publishing (Critical).** `_next_version_number` now
takes a `SELECT ... FOR UPDATE` on the dataset row before allocating. The
follow-ups doc had recorded this race as harmless -- a loser that rolls back
leaving "an orphaned S3 object" -- and that was wrong. `object_key_for` derives
the key from the version number, so two publishers reading the same max also
`put_object` to the SAME key: the second write overwrites the first's body, and
it can do so after the first publisher's `SchemaVersion` row, carrying a
checksum computed from its own body, has committed. The committed row then
points at content that does not match its checksum, breaking the immutability
the model exists to provide. The unique constraint fires at `db.flush()`, long
after the object was overwritten. Silent corruption of a committed row outranks
a stray object, so this is fixed rather than deferred. SQLite emits no FOR
UPDATE clause (single-writer already serializes), so it is a no-op there.
**Map pandas extension dtypes to typed ES fields.** A column declared
`pd.Int64Dtype()` -- the ordinary way to get a nullable integer column --
reports "Int64", and that spelling survives to_json/from_json into
`Field.settings["dtype"]`. `_ES_TYPE_BY_COLUMN_DTYPE` only had the lowercase
numpy spellings, so every such column fell through to the text fallback and lost
numeric range queries and numeric sort order. Timezone-aware datetimes spell
their zone into the dtype ("datetime64[ns, UTC]") and missed the lookup once per
zone. New `normalize_column_dtype` case-folds and strips the zone before lookup,
which needs no new map entries. This is the same class of bug the earlier
roborev pass fixed for "bool"/"boolean", fixed generally this time.
Note: `nullable=True` alone does NOT change the spelling -- an earlier comment
in this branch implying it does was wrong. The trigger is the extension-dtype
declaration. Both halves are now pinned by tests so they cannot drift.
**Bound the schema-version body** at 1 MiB. It is parsed in-process and then
uploaded whole, so one request sizes both the parse and the object write.
Generous by design (a 500-column schema serializes to well under 100 KiB); a
whole-request limit belongs at the ASGI layer, which does not exist here yet.
**Fail run-demo.sh closed up front.** `seed_demo_workspace.py` still targets the
deleted /api/v2 and exits 1 unconditionally, so the pipeline could never
succeed -- but it first created (and on a rerun deleted) its output directory
before finding out. Now refuses at the top, with the README saying so.
Repointing the seed stays deferred.
**Fix the fold-followups mapping-evolution section.** It stated the 422 contract
in one place and the superseded "option (a), decision pending" analysis in
another, and still described the `index_exists` guard removed in 1db3acc. One
contract now, with the argument for it kept as explicitly historical. Section 5
rewritten as resolved with the corrected damage analysis above.
Markdown lint: MD028 blank-line-in-blockquote across 7 historical-note docs
(the 8th was a false positive: its next line is not a blockquote), MD040 on two
fences. Note this repo runs no markdownlint -- no config, no hook, no CI job.
**Declined.** (1) "Restore concurrent workflow coverage": the premise is wrong.
`test_concurrent_workflow_processing` was not skipped by this branch to hide a
regression -- on develop the whole class errored at fixture setup (it referenced
a nonexistent `async_db` fixture and built Workspace() with removed kwargs), so
it never provided coverage. This branch fixed the fixtures, made one of the two
pass for real, and skipped the other with an accurate root cause. The suggested
fix (per-task sessions) is precisely what the skip reason documents as
insufficient without the ENG-37 seam. (2) review_widgets was not separately
bounded: an item-count cap does not bound bytes and would only look like a
limit.
Verification: 106 targeted tests pass. Same 115 failures before and after across
contexts/search_engine/dataset-handlers/schemas, measured against a stashed
clean baseline -- identical sets, only randomized parametrize ids differ.
* docs(server): scope the version-allocation guarantee to PostgreSQL
The previous commit's claim that SQLite "already serializes" concurrent
version allocation was wrong on both counts, and CodeRabbit was right to
flag it.
SQLAlchemy's SQLite dialect compiles `with_for_update()` away entirely --
verified: the statement renders with FOR UPDATE under the postgresql
dialect and without it under sqlite, silently rather than as an error. And
SQLite's single-writer model does not stand in for the lock, because
pysqlite opens transactions DEFERRED: the read takes no lock at all. Two
concurrent publishes on SQLite therefore still read the same max, derive
the same object_key, and overwrite each other's body; the losing INSERT
then fails on the unique constraint or with `database is locked`, by which
point the object is already corrupt.
This is not a test-only caveat: `sqlite+aiosqlite` is the DEFAULT
database_url (settings.py), so a default deployment runs unprotected. Said
so plainly in both the docstring and fold-followups section 5, which is
retitled FIXED ON POSTGRESQL / OPEN ON SQLITE rather than RESOLVED.
Not implementing a SQLite strategy here. Closing it needs a
dialect-conditional BEGIN IMMEDIATE on the publish transaction, or moving
allocation into the INSERT (`INSERT ... SELECT max(version)+1 ...`) so read
and write are one statement. That is a concurrency-model design question,
deferred rather than guessed at.
Adds the first tests for either half, since the fix shipped untested:
- the dialect asymmetry itself, so the documented gap stays honest and a
future SQLite rendering of FOR UPDATE surfaces as a failure;
- that publish_version issues a locking read at all, guarding the
PostgreSQL half. Mutation-checked: deleting the lock line fails it.
Verification: 114 passed, 25 skipped across tests/unit/contexts and the
schema-version handlers. ruff clean except the pre-existing helpers.py
ASYNC240.
* refactor(tests): simplify test_hub_dataset_exporter and test_hub_dataset
- Removed unused imports and fixtures from test_hub_dataset_exporter.py to streamline the test file.
- Consolidated factory imports in test_hub_dataset_exporter.py for clarity.
- Cleaned up test_hub_dataset.py by removing commented-out code and unnecessary imports, enhancing readability.
- Removed several test cases that were skipped or redundant, focusing on essential functionality.
* test(e2e): enhance search roundtrip spec for filtered search validation
- Updated comments to clarify the behavior of the status filter in the search body.
- Added assertions to ensure that the filtered search correctly honors the status filter, confirming that completed records are displayed while pending records are not.
- Improved the test's handling of empty results to ensure graceful rendering when no records match the filter.
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.extractions-demo.mp4
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.