Add series instrument creation and remote assignment flow by thomasbeaudry · Pull Request #1392 · DouglasNeuroInformatics/OpenDataCapture

thomasbeaudry · 2026-07-06T04:51:40Z

Includes the create-series-instrument feature (bundler, schemas, renderer, web hooks/routes, e2e coverage) and a fix for the group-save 500: filter deleted instrument ids server-side before the relation set, and prune them from the cached group after an instrument delete.

Something I noticed is that the datahub doesn't track whether an insturment is administered standalone or within a series. It may be useful to track this because I could imagine the results of a form could vary depending on the other forms they take at the same time? Also the datahub records the form date, but no the form adminsitration time.

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

Includes the create-series-instrument feature (bundler, schemas, renderer, web hooks/routes, e2e coverage) and a fix for the group-save 500: filter deleted instrument ids server-side before the relation set, and prune them from the cached group after an instrument delete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The ./constants subpath export had its `types` condition pointing at the source `./src/constants.ts` instead of the built declaration. A source .ts is not a valid target for the `types` condition, so a fresh CI build failed to resolve `@opendatacapture/runtime-core` exports, cascading into module-not-found and implicit-any errors in the instrument-bundler fixtures. Local builds passed only because a stale runtime/v1/dist masked it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gdevenyi · 2026-07-06T18:59:46Z

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

We need to review the "metadata" model of instruments in general to make sure we're capturing all we should capture.

Copilot

Pull request overview

Adds a “series instrument” feature that lets group managers assemble an ordered set of existing scalar instruments into a new SERIES instrument, preview it in the web UI, assign it to groups (including expanding underlying scalar IDs), and delete series instruments when they have no collected records. Also hardens group updates against stale/deleted instrument IDs to prevent 500s.

Changes:

Add schemas + API endpoints/service logic for creating SERIES instruments server-side (bundled/validated like normal instruments) and deleting instruments with record-count protection.
Update web group-manage UI to create/preview/delete series instruments and expand selected series IDs into underlying scalar IDs on save.
Prevent group-save failures by filtering deleted/nonexistent instrument IDs server-side and pruning deleted IDs client-side.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/schemas/src/instrument/instrument.base.ts	Extends instrument info schema (seriesItems/internal.name) and adds create-series DTO schema.
packages/react-core/src/components/InstrumentRenderer/SeriesInstrumentRenderer.tsx	Threads submit button label into series form rendering.
packages/react-core/src/components/InstrumentRenderer/ScalarInstrumentRenderer.tsx	Threads submit button label into scalar form rendering.
packages/react-core/src/components/InstrumentRenderer/InstrumentRenderer.tsx	Adds submitButtonLabel prop to top-level renderer API.
packages/react-core/src/components/FormContent/FormContent.tsx	Adds submitButtonLabel prop and forwards to libui Form.
apps/web/src/store/types.ts	Adds auth-slice API for pruning deleted instrument IDs.
apps/web/src/store/slices/auth.slice.ts	Implements pruneDeletedInstrument to keep cached group state consistent.
apps/web/src/routes/_app/group/manage.tsx	Adds series create/preview/delete UI and expands series selections to include scalar IDs.
apps/web/src/hooks/useDeleteInstrumentMutation.ts	New mutation hook to delete instruments + invalidate instrument-info + prune cache.
apps/web/src/hooks/useCreateSeriesInstrumentMutation.ts	New mutation hook to create series instruments with duplicate-confirmation flow.
apps/api/src/instruments/instruments.service.ts	Implements createSeries, deleteById, seriesItems projection in findInfo, and series-id logic update.
apps/api/src/instruments/instruments.controller.ts	Adds POST /instruments/series and DELETE /instruments/:id endpoints.
apps/api/src/instruments/dto/create-series-instrument.dto.ts	Adds validated DTO for create-series input.
apps/api/src/instruments/tests/instruments.service.spec.ts	Adds unit tests for createSeries/id generation/deleteById behavior.
apps/api/src/groups/groups.service.ts	Filters nonexistent instrument IDs before setting group instrument relations.
apps/api/src/groups/tests/groups.service.spec.ts	Adds test for dropping deleted instrument IDs before relation set.
apps/api/src/auth/ability.factory.ts	Grants group managers create/delete abilities for instruments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thomasbeaudry · 2026-07-06T20:53:34Z

        ability.can('read', 'Instrument');
+        // Group managers may assemble series instruments on the fly and remove ones they created
+        // (the service enforces that an instrument with collected records cannot be deleted).
+        ability.can('create', 'Instrument');
+        ability.can('delete', 'Instrument');


Addressed on the current branch: the delete ability is now scoped — ability.can('delete', 'Instrument', { sourceRepoId: null }) (ability.factory.ts:33). accessibleQuery therefore no longer returns {}, so a group manager can only delete non-repo (manually-uploaded) instruments; repo-sourced instruments shared across groups are excluded. The service-side record-count guard remains as defense-in-depth.

Why do group managers need to be able to delete instruments in the first place? This contradicts the original design of the platform.

Addressed in the latest push. The rationale for keeping it: a series instrument isn't a shared platform asset — it's an on-the-fly bundle of existing instruments that a group manager assembles, and records are only ever collected against the constituent scalars, never the series itself. So deleting one never orphans data or touches a real instrument. The ability is now scoped — ability.can('delete', 'Instrument', { sourceRepoId: null }) — so repo-sourced instruments can never be deleted, and the service additionally rejects deletion of anything that isn't a series (unit-tested). If you'd still rather group managers not have this at all, I can drop the delete path entirely.

thomasbeaudry · 2026-07-06T20:53:35Z

+    const instrument = await this.instrumentModel.findFirst({
+      where: { AND: [accessibleQuery(ability, 'delete', 'Instrument')], id }
+    });
+    if (!instrument) {
+      throw new NotFoundException(`Failed to find instrument with ID: ${id}`);
+    }
+    const recordCount = await this.instrumentRecordModel.count({ where: { instrumentId: id } });


deleteById gates on accessibleQuery(ability, 'delete', 'Instrument'), and the delete ability now carries { sourceRepoId: null } (ability.factory.ts:33), so the generated Prisma where already excludes repo-sourced instruments — a repo instrument yields no match and returns NotFound. Happy to also add an explicit sourceRepoId === null assertion in the service as belt-and-suspenders (independent of the ability config) if you'd like it guaranteed at the service layer too.

the ability should be scoped to only series instruments and this should be explicitly tested.

Done in the latest push — deleteById now rejects anything that isn't a series (isSeriesInstrument check → ForbiddenException), with an explicit unit test ('refuses to delete a non-series (scalar) instrument'). One caveat on scoping the ability to series: kind isn't a database column (it lives inside the evaluated bundle), so the CASL/Prisma layer can only scope on real fields — the ability is scoped to { sourceRepoId: null } and the series-only rule is enforced (and tested) at the service layer.

thomasbeaudry · 2026-07-06T20:53:37Z

+        internal: instrument.internal ?? null,
        kind: instrument.kind,
+        seriesItems: instrument.seriesItems,
        source,


Addressed: source is now derived from repoId, not the repo name — const source = repoId ? { kind: 'repo', name: repoName ?? 'Unknown' } : { kind: 'manual' } (manage.tsx:876). A legacy repo instrument with a null name is therefore still treated as repo-sourced (so it doesn't get the manual/delete affordance), with an "Unknown" fallback label.

Following up — correcting my earlier reply: the committed code was still deriving provenance from the repo name. Now fixed: source is keyed on the repo id, with a localized 'Unknown repository' fallback label, so a legacy repo instrument with a null name is still treated as repo-sourced and never gets the manual/delete affordance.

joshunrau · 2026-07-06T20:11:45Z

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

We need to review the "metadata" model of instruments in general to make sure we're capturing all we should capture.

The data hub does not track anything. The data hub is just displaying the results from the database. We record both the exact time that an instrument was completed as well as the mode of administration (remote, in-person, retrospective). Both of these should be available when you download the results as a CSV.

I agree that we should display the mode of capture. I did not add exact time to the table to avoid cluttering it. If you would prefer, we could add something to display the time, I just didn't see the use case.

gdevenyi · 2026-07-06T20:36:22Z

If you would prefer, we could add something to display the time, I just didn't see the use case.

I'd think I'd like to discuss how we can make customiziable columns in the viewer so that data can be available if wanted. Seperate issue to this (and to the broader metadata audit)

thomasbeaudry · 2026-07-06T20:41:31Z

I'd think I'd like to discuss how we can make customiziable columns in the viewer so that data can be available if wanted. Seperate issue to this (and to the broader metadata audit)

Yes i was thinking the exact same thing.

@joshunrau also I downloaded the happiness questionaire csv and I don't see the mode of administration:

Also, is the series that was used to record the data being recorded in the DB? This would be linked to this PR if we want ot track it.

joshunrau

this is terrible, it needs major changes

joshunrau · 2026-07-07T18:43:31Z

        ability.can('read', 'Instrument');
+        // Group managers may assemble series instruments on the fly and remove ones they created
+        // (the service enforces that an instrument with collected records cannot be deleted).
+        ability.can('create', 'Instrument');
+        ability.can('delete', 'Instrument');


Why do group managers need to be able to delete instruments in the first place? This contradicts the original design of the platform.

joshunrau · 2026-07-07T22:34:40Z

+  bundle: vi.fn(() => Promise.resolve('__BUNDLE__'))
+}));
+
+// A multilingual series instance as returned by `find`, containing two forms.


add proper type annotation

@joshunrau a series instrument isn't really an instrument, it's a bundled list of instruments. I see no harm in letting the people who create them delete them, They could even belong to speccific groups instead of every user as well

Done — existingSeries is now typed as WithID<SeriesInstrument> (no as any), which also forced the fixture to carry the full details/license/tags shape.

joshunrau · 2026-07-07T22:58:47Z

+import type { ScalarInstrumentInternal } from '@opendatacapture/runtime-core';
+import { $CreateSeriesInstrumentData } from '@opendatacapture/schemas/instrument';
+
+@ValidationSchema($CreateSeriesInstrumentData)


no need for dto anymore. just import the $CreateSeriesInstrumentData any use that as annotation. make sure not to use type-only import

Done — the DTO class is deleted. $CreateSeriesInstrumentData is exported as both the schema value and its inferred type, and the controller uses it directly as the @Body() annotation (value import, not type-only) — same pattern as $SelfUpdateUserData in the users controller.

joshunrau · 2026-07-07T23:00:52Z

+  instructions?: string;
+  items: ScalarInstrumentInternal[];
+  title: string;
+}


Why are you not using the actual series instrument definition? Right now, you are missing the multilingual aspects and potential other things. Use the same data structure as the instrument type, omitting things that are not stored/computed

Reworked — the request schema is now composed from the master instrument schemas: details picked from $InstrumentDetails (multilingual title + optional description), clientDetails.instructions from $ClientInstrumentDetails, plus language: $InstrumentLanguage. Only fields that are stored verbatim are accepted; everything computed server-side (id, content, kind) is omitted. The generated definition is explicitly typed Omit<SeriesInstrument, 'id'> so any drift from the instrument type fails to compile.

joshunrau · 2026-07-07T23:02:06Z

+    const instrument = await this.instrumentModel.findFirst({
+      where: { AND: [accessibleQuery(ability, 'delete', 'Instrument')], id }
+    });
+    if (!instrument) {
+      throw new NotFoundException(`Failed to find instrument with ID: ${id}`);
+    }
+    const recordCount = await this.instrumentRecordModel.count({ where: { instrumentId: id } });


the ability should be scoped to only series instruments and this should be explicitly tested.

joshunrau · 2026-07-07T23:12:13Z

    logout: () => {
      window.location.reload();
+    },
+    pruneDeletedInstrument: (instrumentId) => {


this is a terrible design. I understand why it might be necessary, but consider how you can change implementation details to make a clean approach

Removed. The underlying need (don't re-send a deleted id on the next group save) is handled locally in the manage route: a deletedIds set in component state, filtered at submit time. No global store involvement.

joshunrau · 2026-07-07T23:12:52Z

  /** @deprecated */
  options?: InterpretOptions;
  subject?: SubjectDisplayInfo;
+  submitButtonLabel?: string;


Changed — submitButtonLabel is now a LocalizedText ({ [L in Language]?: string }) resolved with t() inside FormContent, so the shared component can no longer be handed a pre-translated single-language string.

joshunrau · 2026-07-07T23:12:57Z

  NavigationBlocker?: NavigationBlockerComponent;
  onSubmit: InstrumentSubmitHandler;
  subject?: SubjectDisplayInfo;
+  submitButtonLabel?: string;


Same fix as on ScalarInstrumentRenderer — the prop is LocalizedText on all three renderers and translated at the leaf with t().

joshunrau · 2026-07-07T23:14:24Z

+  // The ordered list of scalar instruments (by name + edition) that make up the series.
+  items: z.array($ScalarInstrumentInternal).min(2),
+  // The unique display name entered by the group manager.
+  title: z.string().min(1)


this should be your source of truth and should come based on the master schema

Done — $CreateSeriesInstrumentData is now composed from the master schemas ($InstrumentDetails, $ClientInstrumentDetails, $InstrumentLanguage, $ScalarInstrumentInternal) rather than redeclaring its own shapes.

joshunrau · 2026-07-07T23:15:33Z

+      })
+      .array()
+      .optional(),
    sourceRepo: z


we should be using a discriminated union here for series vs scalar

Done — InstrumentInfo is now a discriminated union on kind: scalar variants carry a required internal, the series variant carries required seriesItems. findInfo builds the proper variant and consumers narrow on kind instead of optional-chaining.

Backend - Derive $CreateSeriesInstrumentData from the master instrument details/clientDetails schemas (multilingual title/description/instructions) and take language from the client instead of hardcoding en/fr and duplicating text. - Remove the create-series DTO; the controller annotates the body with the $CreateSeriesInstrumentData schema value directly. - Share a discriminated-union result contract (CreateSeriesInstrumentResult) between the service, controller and web hook (outcome: 'created' | 'duplicate'). - Make InstrumentInfo a discriminated union over kind (scalar carries `internal`, series carries `seriesItems`); build the correct variant in findInfo and resolve series items from the full scalar set even under a kind filter. - deleteById: restrict deletion to series instruments (series never collect records) and drop the record-count guard; scope the group-manager delete ability to non-repo instruments. Add an explicit test that deleting a non-series is refused. - Handle multilingual titles throughout (uniqueness search now spans all instruments, duplicate detection returns the stored title); rename getFormSetKey -> getSeriesItemSetKey and give the generated definition an explicit SeriesInstrument type. Frontend - Rewrite useCreateSeriesInstrumentMutation to reuse the shared request/result types. - Remove pruneDeletedInstrument from the auth store; track deleted ids as local manage state and filter them from the save payload. - Fix instrument provenance to derive from the repo id (not name) so legacy repo instruments are never treated as manually-uploaded. - Make the shared submitButtonLabel prop a localizable value resolved with t(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

thomasbeaudry · 2026-07-10T05:30:15Z

+ * server-side. Exported as both a value (the schema) and a type so it can annotate a controller body
+ * without a dedicated DTO class.
+ */
+type $CreateSeriesInstrumentData = z.infer<typeof $CreateSeriesInstrumentData>;


This compiles as written — type $CreateSeriesInstrumentData and const $CreateSeriesInstrumentData share a name via TypeScript declaration merging, and the export block at the bottom of the file exports that name in both its type and value meanings, so import { $CreateSeriesInstrumentData } works in either position (the monorepo tsc and CI lint-and-test pass on this code). The type/const same-name pattern is intentional here: the API controller imports it as a value so the zod schema itself becomes the body's design:paramtypes metadata, which libnest's ValidationPipe recognizes (metatype instanceof z.ZodType) — giving runtime validation without a DTO class, while the same identifier annotates the static type.

thomasbeaudry · 2026-07-10T05:30:16Z

+  const handleCreate = async () => {
+    const items = buildItems();
+    if (items.length < 2) {
+      return;
+    }
+    const result = await createMutation.mutateAsync(buildPayload(items));
+    if (result.outcome === 'duplicate') {
+      // The existing title comes back in its stored form (a plain string, or multilingual object).
+      setDuplicateOf(typeof result.existingTitle === 'string' ? result.existingTitle : t(result.existingTitle));
+      return;
+    }
+    onClose();
+  };


Partly agreed. The intended UX is actually preserved on failure: onClose() sits after the await, so a rejection leaves the dialog open, and the mutation's react-query onError already raises the error notification. What does escape is the rejected promise itself from the void handleCreate() call site (console noise / unhandled-rejection event, no user-facing impact). A try/catch swallow at the handler level would silence that — worth adding as a small hardening pass alongside the two sibling handlers flagged below.

thomasbeaudry · 2026-07-10T05:30:17Z

+  const handleConfirmDuplicate = async () => {
+    const items = buildItems();
+    if (items.length < 2) {
+      return;
+    }
+    await createMutation.mutateAsync({ ...buildPayload(items), confirmDuplicate: true });
+    onClose();
+  };


Same assessment as handleCreate above: the dialog already stays open on failure (state updates only run after the await) and the mutation's onError notification fires, so retry is possible — the genuine defect is limited to the rejection escaping the void call site uncaught. Same one-line try/catch hardening applies.

thomasbeaudry · 2026-07-10T05:30:18Z

+  const handleDelete = async () => {
+    await deleteMutation.mutateAsync({ id: item.id });
+    onDeleted(item.id);
+    onClose();
+  };


Confirmed the success-path ordering is already correct: onDeleted()/local-state updates run only after await deleteMutation.mutateAsync(...) resolves, and useDeleteInstrumentMutation.onError shows the failure notification — so state is not corrupted on failure. As with the create handlers, the remaining issue is only the uncaught rejection from the void handleDelete() call site; same try/catch hardening applies to all three handlers.

thomasbeaudry · 2026-07-10T05:30:19Z

+  @Post('series')
+  @RouteAccess({ action: 'create', subject: 'Instrument' })
+  createSeries(


Accurate observation, and worth a maintainer decision. create Instrument for GROUP_MANAGER is required by design — creating series instruments on the fly is a core group-manager flow (and the upcoming bulk remote-assignment feature depends on it) — but it does also authorize POST /v1/instruments (arbitrary bundle upload), which the web UI never exposes to group managers. If managers should be restricted to series creation only, the cleanest split is a dedicated subject/action for the upload endpoint (e.g. require manage on Instrument there) while leaving create for POST /instruments/series. Flagging as a follow-up rather than changing it in this PR, since tightening it affects existing GROUP_MANAGER API behavior beyond the series feature.

gdevenyi

Review: Add series instrument creation and remote assignment flow

Comprehensive pass over c2478b9. Overall the rework in the last commit is a big improvement — the schemas are clean (discriminated unions on kind and outcome, $CreateSeriesInstrumentData composed from the master schemas, explicit Omit<SeriesInstrument,'id'> typing), the service-layer guards are sound, test coverage for the new service paths is solid, and all user-facing strings are localized. CI (lint-and-test, CodeQL, Analyze) is green. Posting as Comment rather than Approve because of the permission expansion and the still-open design sign-off from @joshunrau.

🔴 Needs decision before merge

1. GROUP_MANAGER now has unscoped create on Instrument — this also unlocks the regular scalar-upload endpoint. apps/api/src/auth/ability.factory.ts:34
On main, GROUP_MANAGER only had read on Instrument. This PR adds ability.can('create', 'Instrument') (unscoped). That's required so the series route passes @RouteAccess({ action: 'create' }), but POST /instruments (the regular scalar instrument-upload endpoint, instruments.controller.ts:26) is gated by the exact same action/subject. Net effect: group managers can now upload arbitrary scalar instrument bundles (code executed in the virtualization sandbox) platform-wide — previously admin-only — and those instruments are visible to everyone. This looks incidental to the series feature. If series-only is intended, consider a dedicated subject/action for series creation (or otherwise keep the regular create admin-only) so the ability doesn't over-grant.

2. Group-manager delete on Instrument — still needs @joshunrau's explicit sign-off. apps/api/src/auth/ability.factory.ts:35
@joshunrau's earlier review (changes requested) explicitly questioned why group managers can delete instruments at all ("contradicts the original design"). The latest push scopes it to { sourceRepoId: null } and the service layer enforces series-only with a test (instruments.service.spec.ts:261) — good defense-in-depth. But two things to confirm:

The ability itself still permits delete on any manually-uploaded scalar (sourceRepoId: null); only the service-level isSeriesInstrument check (instruments.service.ts:166) prevents it. If that service check is ever refactored away, manual scalars become deletable by managers. The CASL layer can't narrow to series because kind isn't a DB column — acknowledged — so please get explicit maintainer buy-in that this trade-off is acceptable (or drop the delete path and have admins delete series).
This is a genuine design disagreement that appears resolved-in-code but not yet approved. Want @joshunrau to ack before this merges.

🟡 Medium

3. Duplicate-items detection is ability-scoped, while title uniqueness is global — inconsistent. instruments.service.ts:399
isInstrumentTitleTaken correctly searches the full instrument set (find({}), no ability), but findSeriesTitleWithSameItems calls this.find({ kind: 'SERIES' }, { ability }). So a manager won't be warned about an identical series built by someone in another group (different title → passes both checks → near-duplicate series created). Either make the duplicate-set check global too (consistent with the title check) or document why it's intentionally scoped.

4. Creating/deleting a series discards pending instrument toggles in the manage form. apps/web/src/routes/_app/group/manage.tsx:602
Both new flows invalidate ['instrument-info'] (hooks at useCreateSeriesInstrumentMutation.ts:38 and useDeleteInstrumentMutation.ts:27). The refetch recomputes data → new initialSelectedIds reference → the (pre-existing) useEffect at manage.tsx:602 resets selectedIds. If a user has toggled several instruments and then creates or deletes a series, those pending checkbox changes are silently lost. Pre-existing pattern, but newly triggerable. Consider not invalidating until the dialog closes, or guarding the resync while there are unsaved edits.

5. DELETE /instruments/:id is series-only in disguise. instruments.controller.ts:43
The service throws ForbiddenException for any non-series instrument — including for admins. The Swagger summary "Delete Instrument" implies a general delete. Either route it as /instruments/series/:id or note the series-only contract in the summary/description so callers aren't surprised that it 403s on scalars.

🟢 Minor / nits

6. pnpm-lock.yaml — 8640 lines of formatting-only churn, zero package.json changes. The diff reduces to "no syntactic changes", i.e. a reformat from a different pnpm patch version with no dependency movement. Regenerate against the repo's pinned pnpm so the PR diff stays reviewable and we avoid future merge conflicts.

7. Type-naming convention: $CreateSeriesInstrumentData is $-prefixed as a type. packages/schemas/src/instrument/instrument.base.ts:307
The sibling right above it follows CreateInstrumentData (type, no $) / $CreateInstrumentData (value). The new one reuses the $ name for both type and value. Works, but inconsistent — consider naming the inferred type CreateSeriesInstrumentData.

8. Cost of full-set scans. instruments.service.ts:468 / :394 / :262
Each createSeries triggers find({}) (all instruments, full virtualization) for the title check, plus find({ kind: 'SERIES' }) for the duplicate check; findInfo now does an extra full find({}) whenever the result contains series (:262). Fine at current scale — flagging for awareness if the instrument count grows.

9. Missing regression tests for two specifically-discussed fixes.

findInfo seriesItems resolution under kind=SERIES (the Copilot-flagged bug fixed in :262) — no test.
No ability.factory test asserting group managers can't delete repo instruments (the scoping rationale). The service-level series-only refusal is covered; the ability scoping itself isn't.

10. PR description says "e2e coverage" but the diff only adds the two unit specs under apps/api/.../__tests__/ — no testing/ e2e files. Minor description mismatch.

What's done well

Schema layer is now a proper single source of truth: InstrumentInfo discriminated on kind, CreateSeriesInstrumentResult discriminated on outcome, request schema composed from $InstrumentDetails/$ClientInstrumentDetails/$InstrumentLanguage/$ScalarInstrumentInternal.
Service tests are meaningful: duplicate-confirm-without-create, case-insensitive title uniqueness, series-only delete refusal, id-generation includes title, and the assertion that instrumentRecordModel.count is never called (instruments.service.spec.ts:289).
submitButtonLabel correctly threaded as LocalizedText and resolved with t() at the leaf (FormContent.tsx:51) — fixes the "french" concern.
Group-save 500 fix is minimal and well-scoped (groups.service.ts:88-95), with a test for stale ids.
Discriminated-union consumers narrow on kind instead of optional-chaining (InstrumentCard.tsx:51, instrument-repos/index.tsx:321, manage.tsx:901).

A series' identity encodes the exact ordered composition of its items (A,B,C differs from B,C,A), so records collected through a series must preserve which series drove the collection. Adds a nullable seriesInstrumentId relation to InstrumentRecord and populates it end-to-end: - prisma: InstrumentRecord.seriesInstrumentId (named relations to Instrument) - schemas: seriesInstrumentId on $CreateInstrumentRecordData / $InstrumentRecord - api: create() validates the reference is a SERIES instrument and connects it; the gateway synchronizer tags remote records with the assignment's series id - react-core: SeriesInstrumentRenderer includes seriesInstrumentId in its submit context; the web render route forwards it when creating records Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

# Conflicts: # apps/api/src/gateway/gateway.synchronizer.ts

thomasbeaudry · 2026-07-10T05:14:25Z

Merged origin/main into this branch (0369722a4) — brings in the incomplete-series gateway fix, the series terminate function, and the synchronizer error-logging improvements. One conflict in gateway.synchronizer.ts was resolved by keeping main's restructured decrypt/parse/create error handling and re-applying the seriesInstrumentId metadata tagging from fe5c7b6be inside it. Typecheck, full lint (35/35), and api tests (106/106) all pass post-merge.

thomasbeaudry requested a review from joshunrau as a code owner July 6, 2026 04:51

gdevenyi requested a review from Copilot July 6, 2026 18:59

Copilot started reviewing on behalf of gdevenyi July 6, 2026 18:59 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

joshunrau requested changes Jul 7, 2026

View reviewed changes

gdevenyi requested a review from Copilot July 8, 2026 20:29

Copilot started reviewing on behalf of gdevenyi July 8, 2026 20:29 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

gdevenyi reviewed Jul 8, 2026

View reviewed changes

thomasbeaudry and others added 2 commits July 10, 2026 00:51

Merge remote-tracking branch 'origin/main' into createSeriesInstrument

0369722

# Conflicts: # apps/api/src/gateway/gateway.synchronizer.ts

Fix group-scoped series instrument management

87ce6b5

Uh oh!

Conversation

thomasbeaudry commented Jul 6, 2026

Uh oh!

gdevenyi commented Jul 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshunrau commented Jul 6, 2026

Uh oh!

gdevenyi commented Jul 6, 2026

Uh oh!

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

Uh oh!

Uh oh!

joshunrau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

thomasbeaudry commented Jul 6, 2026 •

edited

Loading