feat: record instrument repository syncs and the instruments that fail them by thomasbeaudry · Pull Request #1832 · DouglasNeuroInformatics/OpenDataCapture

thomasbeaudry · 2026-10-28T19:59:46Z

What

An instrument that failed to compile or load during a repository import or sync was written only to the server log and skipped. Nobody could tell it was missing. For example, CPP_SERIES silently failed because the series referenced editions the instance did not have.

Sync log (API)

Every import and sync writes an InstrumentRepoSyncLog: action, number imported, message (set when the repository could not be read at all), and one failure per instrument with its directory, stage (BUNDLE = did not compile, LOAD = the server refused the bundle) and reason. Reasons include the VM error and each validation issue with its path, not just "Instrument validation failed".
GET /v1/instrument-repos/sync/logs (admin only) and POST /v1/instrument-repos/sync/all. Both have two segments, so a :id route cannot shadow them.
POST /v1/instrument-repos/:id/sync now returns the sync log instead of the repository. The web app is the only consumer.

Sync replaces the repo's instrument list

A sync now sets instrumentIds to what the repository currently provides instead of merging. Nothing is deleted. A dropped edition keeps its sourceRepoId, which is what group visibility reads, so groups can still see and administer old editions.

Admin Instrument Repositories page

Failed column: failed-instrument count from each repository's latest sync, in red when non-zero, and "Sync failed" when the repository could not be read.
Sync All button next to Add Repository.
Sync History table under the repositories, sortable on every column but Messages. Status is Success, Partial success or Failed. Messages list each failed instrument, its stage and why. A per-repository Logs row action shows the same table for one repository.
View dialog: a Source: link to the GitHub repository, and an instrument search.
Long URLs are truncated with the full URL on hover. Before, they ran into the Instruments column, which became visible on the selected row's translucent highlight.

Verification

pnpm lint: 34/34 tasks pass.
pnpm test:coverage: 419 files, 4849 tests, coverage at 100%.
Unit: apps/api/src/instrument-repos/__tests__/instrument-repos.{service,controller}.test.ts cover replace-not-merge, import/sync/download-failure logging, syncAll carrying on past an unreadable repository, and the failure reasons for each error shape. Also apps/web/src/components/InstrumentRepoSyncLogTable/__tests__/, the page test, and the sync, sync-all and sync-logs hook tests.
E2E: new admin instrument repos sync history block in testing/src/specs/admin-instrument-repos.spec.ts. It covers the failed count, history and messages, the Logs dialog, the Source link and search, and Sync All. It also measures that the URL stays inside its cell on a selected row. The API responses are mocked, since the suite cannot reach GitHub. 15/15 pass on chromium.
Exceptions:
- Only the specs I touched were run. The full pnpm test:e2e was not, because port 3750 (the playground) was held by another process and firefox is not installed here.
- Not tried against a real GitHub repository in a browser.

Docs: apps/api/AGENTS.md and .agents/docs/architecture/auth-and-permissions.md now list InstrumentRepoSyncLog among the models no user can be granted. .agents/docs/architecture/instrument-pipeline.md says where a cross-repository series failure now shows up.

Co-Authored-By: Claude Opus 5.5 [email protected]

✅ No longer blocked — on libui 6.19.2, patch removed

The patched dependency is gone. chore: take libui 6.19.2 and drop the data table patch bumps the catalog: entry, deletes patches/@[email protected] and the patchedDependencies block, and drops the paragraph in apps/web/AGENTS.md that pointed at the patch. Nothing here depends on unreleased code any more.

For the record, since this took three upstream PRs:

libui	Shipped in	Why
#129	6.19.0	`ServerDataTableStoreParams` typed `initialState` as `never`, so no column in a server-mode table could be pinned — and `calculateColumnSizing` honours `columnDef.size` only on a pinned column. This is the one the activity log needed.
#130	6.19.1	Taking 6.19.0 gave this repo 26 `TS7006` errors in 12 unrelated files.
#131	6.19.2	6.19.1 still gave 18, in 7 files.

The regression was ours specifically, not a libui bug in general: moving the components off forwardRef left props annotated React.ComponentProps<…>, and a published declaration that names React unqualified resolves only through @types/react's UMD global. This repo deliberately has no @types/react ("@types/react": "-" in the root pnpm.overrides, since a82250d07) and apps/web's react is workspace:react__19.x, whose types are a plain module declaring no global namespace. skipLibCheck hides the unresolved name, so every prop on the affected components silently became any and the error surfaced far away, in our own call sites. #131 fixed it by importing each React type by name, so the emitted declarations are self-contained.

Review feedback addressed

$NumberLike.pipe(…) replaces z.coerce.number() in packages/schemas/src/instrument-repo/instrument-repo.ts — eslint now bans z.coerce.
@CurrentUser('ability') ability: AppAbility is non-optional across all six handlers in InstrumentReposController.
Four violations of main's new rules fixed: vitest/require-top-level-describe (×2), vitest/require-to-throw-message, no-promise-executor-return.

Rebased onto origin/main, not merged — 12 linear commits, no merge commit. Conflicts resolved in four files: SortableHeader.tsx keeps main's SORT_ICONS map with our sortOrder ?? getIsSorted() override, which a server-mode table needs; useSyncInstrumentRepoMutation.ts keeps $InstrumentRepoSyncLog.parse because onSuccess(log) reads log.failures; instrument-repos/index.tsx keeps our tabs/Card page with main's block-bodied accessorFn style; its test file adopts main's describe('Route', …) wrapper around our mock resets.

Activity log table layout

Every cell is one line now. A repository name wrapped onto a second line and made its row taller than the others. The cause is more general than it looks: libui wraps a cell in its truncating span only when the cell content is not a React element, and Tanstack's default cell renderer is a function, which flexRender renders as a component — so no column in any table of ours was ever actually truncated. Every text cell now renders TruncatedCell (one line, ellipsis, full value in the native title), which is what the repositories table already uses for its URL. The status column keeps ColorTagCell, which already clips and carries a title.
Pinned widths: date 150, action 95, status 140, imported 105, failed 85, messages 95 — 670px — and repoName is the only unpinned column, so it absorbs tableWidth − 670.
Deleted the action and failed-count accessorFns, unreachable once each column had a cell.
docs(web): the two load-bearing DataTable behaviours are now recorded in apps/web/AGENTS.md, which had no repo-owned home for them.

Two notes on the widths

The repository column's width cannot be set directly: pinning it too would leave no unpinned column, and calculateColumnSizing then divides the container evenly and ignores every size.
Each pinned size has a floor set by its longest translation, because header cells get no truncating span either. Shrinking past that wraps the header instead of the cell.
Budget against 764px, not 1280. Measured on a live node: that is what the table actually gets at a 1280px viewport once the sidebar and the card's padding are taken out. admin-instrument-repos.spec.ts holds both tables to it.

The three e2e failures from the last review are fixed

All three were reproduced with my commits stashed, so none came from the layout work — but two were on this branch and would have kept CI red, so I fixed them rather than leave them:

admin-instrument-repos.spec.ts should draw no card behind either tab — the card carries shadow-none, which Tailwind v4 compiles to five fully transparent shadow layers rather than the literal none, so toHaveCSS('box-shadow', 'none') could never pass. The assertion now strips the transparent layers and asserts nothing is left.
should fit both tables to the page without a horizontal scroll bar — the repositories table's pinned columns totalled 794px against the real 764px, which also squeezed its name column to 32px. Trimmed to fit (url 370 → 210, instruments 118, failed 88, lastSynced 112) and its name cell no longer wraps. The same measurement showed the activity log's repository column had been left at 19px, which the overflow test could not catch.
record-file-deletion.spec.ts — pre-existing on main, also noted on feat: email bulk remote assignment links #1838. Untouched.

Verification

pnpm lint — 34/34 tasks clean, no eslint --fix rewrites.
pnpm test:coverage — 100% statements / branches / functions / lines; vitest list --filesOnly names all ten of the branch's test files.
pnpm test:e2e — 340 passed / 6 failed on patched 6.16.1. ⚠️ Not re-run since the 6.19.2 bump — lint and test:coverage both ran green on 6.19.2, but e2e needs the dev-server ports, which were occupied on my machine. CI covers it. Two of the six are the pre-existing main failures (record-file-deletion.spec.ts, subject-detail.spec.ts @smoke). The other four (admin-management ×3, auth ×1) pass 50/50 in isolation and passed in two earlier full runs — flaky under fullyParallel on the single shared database, the auth one via the login throttle. Not from this branch.
admin-instrument-repos.spec.ts — 25/25.

🤖 Generated with Claude Code

joshunrau

please fix merge conflicts and run pnpm lint to identify and fix problems. Many new linting rules have been added

joshunrau · 2026-10-30T15:01:11Z

+  findSyncLogs(
+    @Query(new ParseSchemaPipe({ schema: $InstrumentRepoSyncLogsQueryParams }))
+    query: InstrumentRepoSyncLogsQueryParams,
+    @CurrentUser('ability') ability?: AppAbility


I don't think ability can ever be undefined here

Right — fixed. All six handlers in InstrumentReposController now take @CurrentUser('ability') ability: AppAbility rather than an optional. 7a663e6ed

joshunrau · 2026-10-30T15:02:55Z

+
+export type InstrumentRepoSyncLogsQueryParams = z.infer<typeof $InstrumentRepoSyncLogsQueryParams>;
+export const $InstrumentRepoSyncLogsQueryParams = z.object({
+  limit: z.coerce


this is now banned

use $NumberLike

Fixed — z.coerce is gone from the file. 7a663e6ed

Done:

limit: $NumberLike .pipe(z.number().int().positive().max(INSTRUMENT_REPO_SYNC_LOGS_MAX_PAGE_SIZE)) .default(INSTRUMENT_REPO_SYNC_LOGS_PAGE_SIZE), page: $NumberLike.pipe(z.number().int().positive()).default(1),

imported from @douglasneuroinformatics/libjs. 7a663e6ed

…l them An instrument that failed to compile or load during a repository import or sync was only written to the server log and skipped, so nobody knew it was missing. Every import and sync now writes an InstrumentRepoSyncLog with the number of instruments imported and, for each one that failed, its directory, the stage it failed at and why. A repository that could not be read at all is logged too. Sync now replaces a repository's instrument list with what the repository currently provides instead of adding to it. Instruments that drop off the list stay in the database with their provenance tag, so groups can still see and administer them. On the admin Instrument Repositories page: - a Failed column with the latest sync's failed-instrument count - a Sync All button (POST /v1/instrument-repos/sync/all) - a sortable sync history below the table, plus a per-repository Logs action (GET /v1/instrument-repos/sync/logs, admin only) - a Source link and an instrument search in the View dialog - long repository URLs are truncated instead of running into the next column POST /v1/instrument-repos/:id/sync now returns the sync log rather than the repository. Co-Authored-By: Claude Opus 5.5 <[email protected]>

Adds a Clear History button above the sync history, behind a confirmation that the entries are permanently deleted from the database (DELETE /v1/instrument-repos/sync/logs, admin only). Repositories and their instruments are untouched. Sync status is now shown as a coloured dot beside a plain label, as the datahub tags sex at birth, rather than as coloured text. The Failed column of the repository table is plain text, and its Instruments column is renamed Imported. Co-Authored-By: Claude Opus 5.5 <[email protected]>

The table records both imports and syncs, so "Sync History" was wrong: it is now the Activity Log across the heading, the Clear Log button, both dialogs, the toasts and the API reference. The repositories table's Failed cell said "Sync failed" even for a failed import and now says the repository could not be read, and its instrument count keeps the name Instruments so it no longer collides with the activity log's own Imported column. A failure reason is shown as one plain-text line, clipped rather than wrapped, with every reason in full behind a View button: a long esbuild or validation message made a row several times taller than the rest. An empty log is one line of text instead of a DataTable, whose own empty state is a fixed 288px. POST /:id/sync looked its repository up by id alone, so a user granted update InstrumentRepo for one group could sync a repository belonging only to another; it now goes through accessibleQuery as syncAll already did. Why the log's own route is admin-only while the two sync routes are not is recorded on the handler, since the subject carries no group and cannot be granted. The collection is append-only and the page prefetches it on every visit, so it gains a createdAt index, declared in both the schema and DATABASE_INDEXES because nothing else creates one here, and a request returns at most 500 entries. The page's three prefetches now run together rather than as three round trips, and the newest entry per repository is chosen by its timestamp rather than by its position in the response. Co-Authored-By: Claude Opus 5 <[email protected]>

The admin page read every entry on every visit and filtered client side. The collection is append-only and nothing prunes it, so the response grew with the instance's whole history; the 500-entry cap that bounded it also silently hid older entries, and a rarely-synced repository's history could fall off the end of its own dialog. GET /v1/instrument-repos/sync/logs now takes limit, page, repoId and sortOrder and answers with a page, its page count and the total, so each table asks only for the slice it shows and the per-repository dialog asks the server for that repository. A repoId, createdAt index serves it. The repositories table's failed count cannot come from a page, since the newest entry for one repository may be any distance down the history, so GET /v1/instrument-repos/sync/logs/latest answers with the newest entry per repository. It keys under the history's own query key, so the existing invalidations refresh both. Client-side sorting and searching went with it: both would order or filter the ten rows on screen rather than the history, which reads as a table-wide control and is not one. The date column sorts through the server instead, and SortableHeader takes the order the rows were fetched in, because a table in server mode holds no sorting state of its own. Co-Authored-By: Claude Opus 5 <[email protected]>

The assertion read the row Playwright had resolved a moment earlier, which the second query's render can replace: a detached node reports no width at all, so the clipping check failed in CI and nowhere else. The measurement now retries until it lands on an attached node. Co-Authored-By: Claude Opus 5 <[email protected]>

…arate tabs Co-Authored-By: Claude Opus 5.5 <[email protected]>

`ServerDataTableStoreParams` types `initialState` as `never`, and `calculateColumnSizing` honours `columnDef.size` only on a pinned column, so a table that pages server-side could not give one column the room its content needs: every column took an identical share of the width. The patch is type-only -- the store already read `initialState` in both modes and `reset()` deliberately never rebuilds it. It carries DouglasNeuroInformatics/libui#129; delete it and the `patchedDependencies` entry when the catalog moves to the release that includes it. Co-Authored-By: Claude Opus 5 <[email protected]>

…sitory column A repository name wrapped onto a second line, making its row taller than the rest. libui wraps a cell in a truncating span only when the content is not a React element, and Tanstack's default cell renderer is a function that `flexRender` renders as a component -- so no column was ever truncated. Every text cell now renders `TruncatedCell`: one line, an ellipsis, and the full value in the native `title` for a hover that names what was clipped. The seven columns also shared the width evenly, which left a repository called `owner/some-long-name` a seventh of the table. Pinning the other six with explicit sizes leaves the repository column the room they do not use, the way the repositories table above it already works. The `accessorFn`s on the action and failed-count columns are gone: with an explicit `cell` neither was reachable, and both ids come from `id`. Co-Authored-By: Claude Opus 5 <[email protected]>

… one line Asserts on a live node, since the clipping is layout: the cell carries the full name as its title, its content overflows, and it still stands one line tall. Co-Authored-By: Claude Opus 5 <[email protected]>

- `$InstrumentRepoSyncLogsQueryParams` parses `limit` and `page` with `$NumberLike` instead of `z.coerce.number()`, which eslint now bans because coercion turns `''`, `null` and `true` into valid numbers rather than rejecting them. `$AuditLogsQueryParams` already paged this way. - `InstrumentReposController` takes `ability` as `AppAbility` rather than an optional one. Every route here is behind `@RouteAccess`, so a request reaching a handler always carries an ability, and the rest of the controllers in this app type it the same way. - Satisfies the rules main added: a `beforeEach`/`afterEach` sits inside its `describe`, `toThrow` names what it expects, and a promise executor returns nothing. Co-Authored-By: Claude Opus 5 <[email protected]>

Measured on a live node, the table has 764px to work with there: the repositories table's pinned columns alone came to 794, which squeezed its name column to 32px and made the table scroll sideways. Its URL column gives up most of that width, since a URL is truncated with the full value on hover anyway, and the name cell renders `TruncatedCell` rather than wrapping once the column is narrow enough for a long name to need two lines. The activity log's own pinned columns came to 745 of the same 764, leaving its repository column 19px. Each one is trimmed to the width its longest header needs, which gives the repository column back about a hundred pixels at that viewport and proportionally more on a wider screen. The card's invisibility is asserted layer by layer: Tailwind v4 compiles `shadow-none` to transparent shadow layers rather than the keyword `none`, so `toHaveCSS('box-shadow', 'none')` could never have passed. Co-Authored-By: Claude Opus 5 <[email protected]>

Both facts were load-bearing in this change and neither is visible from a call site: cell content that is a React element never reaches `DataTableBody`'s truncating span, which is every ordinary column, and `columnDef.size` applies only to a pinned column. Nothing in the repo stated either, so they were worked out from the vendor source twice. Co-Authored-By: Claude Opus 5 <[email protected]>

6.19.2 carries the type change this branch patched in locally (libui#129), so a server-mode `DataTable` accepts `initialState` and the activity log's columns can keep their own widths from a released dependency. 6.19.0 and 6.19.1 could not be taken: moving off `forwardRef` left published declarations naming `React` unqualified, which resolves only through `@types/react`'s UMD global. This repo has no `@types/react` and vendors React's types as a plain module, so props silently became `any` and surfaced as 26 then 18 `TS7006` errors in unrelated files. Fixed upstream in libui#130 and libui#131. Co-Authored-By: Claude Opus 5 <[email protected]>

thomasbeaudry requested a review from joshunrau as a code owner October 28, 2026 19:59

joshunrau requested changes Oct 30, 2026

View reviewed changes

thomasbeaudry and others added 12 commits October 30, 2026 11:07

feat(web): show instrument repositories and their activity log on sep…

37a0ab8

…arate tabs Co-Authored-By: Claude Opus 5.5 <[email protected]>

thomasbeaudry force-pushed the feat/instrument-repo-sync-log branch from 91ee212 to 96cdfbc Compare October 30, 2026 20:20

Navigation Menu

Repository navigation

feat: record instrument repository syncs and the instruments that fail them - #1832

feat: record instrument repository syncs and the instruments that fail them#1832
thomasbeaudry wants to merge 13 commits into
mainfrom
feat/instrument-repo-sync-log

thomasbeaudry commented Oct 28, 2026 •

edited

Loading

Uh oh!

joshunrau left a comment

Uh oh!

joshunrau Oct 30, 2026

Uh oh!

thomasbeaudry Oct 30, 2026

Uh oh!

joshunrau Oct 30, 2026

Uh oh!

joshunrau Oct 30, 2026

Uh oh!

thomasbeaudry Oct 30, 2026

Uh oh!

thomasbeaudry Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thomasbeaudry commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Verification

✅ No longer blocked — on libui 6.19.2, patch removed

Review feedback addressed

Activity log table layout

Two notes on the widths

The three e2e failures from the last review are fixed

Verification

Uh oh!

joshunrau left a comment

Choose a reason for hiding this comment

Uh oh!

joshunrau Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

thomasbeaudry Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

joshunrau Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

joshunrau Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

thomasbeaudry Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

thomasbeaudry Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomasbeaudry commented Oct 28, 2026 •

edited

Loading