#47: link existing covers ↔ existing markings from the detail pages

[reese8272]

What

Michael's ask: "relate an existing cover to one or more existing markings, and relate an existing marking to one or more covers. Currently you can only add entirely new covers and they're assigned to the one marking."

• RecordDetail (marking page): a Link Existing Cover button next to Submit New Cover opens a dialog accepting a cover ID or code (42 / C-42), verifies it via getCoverById, POSTs the existing cover-markings endpoint, and refreshes the Associated Covers list in place. Duplicate/validation errors surface in the dialog.
• CoverDetail: a Link Existing Marking button on the Associated Markings card, same flow via getMarkingById + createCoverMarking.
• Linking is one-at-a-time but repeatable, so "one or more" is covered; bulk multi-select can layer on later if editors want it.
• ID parsing extracted to lib/recordLinking.ts with unit tests.

Gating decision

Both buttons render only for editor/admin (isStaff). The endpoint is IsEditorOrAdminWrite, so a contributor-visible button would just 403 on submit — frontend now matches the backend's permission. If we want contributors linking covers, that's a backend permission change to make deliberately, not a UI default.

Zero backend changes

CoverMarkingViewSet + createCoverMarking already shipped; this is UI plumbing only. Ported by hand from the pre-refactor reese/cover-workflow-improvements branch — the image-move half of that branch lands separately as #48.

Tests

jest 53/53 (12 suites, +4 new for ID parsing), tsc --noEmit clean, eslint clean, vite build clean.

🤖 Generated with Claude Code

[github-actions]

Code review overview

• Syntax Errors - Score - 10/10
• Code Smells - Score - 7/10
• Bugs - Score - 7/10
• Security Vulnerabilities - Score - 8/10

---

Syntax Errors

No issues found.

---

Code Smells

1. Duplicated handler logic (CoverDetail.tsx and RecordDetail.tsx)

• File: CoverDetail.tsx ~line 506, RecordDetail.tsx ~line 725
• The handleLinkExistingMarking and handleLinkExistingCover functions are nearly identical: parse input → validate → fetch record → create junction → toast → reload list → reset state. This pattern is copy-pasted in full.
• Suggested fix: Extract a shared useLinkRecord hook (or a generic handleLinkRecord utility) that accepts the parse function, fetch function, create function, and reload callback. This follows DRY and reduces the surface area for future divergence.

2. Inline error-shape cast repeated in two files

• File: CoverDetail.tsx ~line 548, RecordDetail.tsx ~line 757
• err as { response?: { data?: { detail?: string; non_field_errors?: string[] } } } is duplicated verbatim. If the API error shape ever changes, it must be updated in multiple places.
• Suggested fix: Define a shared ApiError type (or a small extractApiError(err: unknown): string | null helper) in a shared utils/errors file.

3. Four parallel state variables per feature

• File: CoverDetail.tsx ~line 161-164, RecordDetail.tsx ~line 306-309
• linkMarkingOpen, linkMarkingInput, linkMarkingBusy, linkMarkingError (and the cover-side equivalents) bloat the component state. The component already has many useState calls.
• Suggested fix: Group into a single useReducer or a small custom hook so the dialog lifecycle is self-contained.

4. Missing blank line before goCoverView in RecordDetail.tsx

• File: RecordDetail.tsx ~line 770
• The handleLinkExistingCover function closes without a blank line before the next function declaration (goCoverView), breaking the consistent spacing style used throughout the file.
• Suggested fix: Add a blank line between the two function declarations.

---

Bugs

1. setLinkMarkingBusy(false) not called in finally if early-return path is taken

• File: CoverDetail.tsx ~line 518-522
• When getMarkingById returns a falsy value the function calls setLinkMarkingError(...) and returns inside the try block, before the finally block runs. Wait — finally does still run after a return inside try, so setLinkMarkingBusy(false) is correctly called. However, the return exits the handler while linkMarkingBusy is still true for a brief moment; because finally runs synchronously, this is actually fine. No bug here in isolation — but the same pattern in RecordDetail.tsx is identical.

2. Stale markingId used to reselect the active link after reload

• File: CoverDetail.tsx ~line 536-539
• After creating the junction row, the code uses the outer-scope markingId (from URL params / query) to find which link to treat as "current". If markingId is null, it silently falls back to linksResult.links[0]. This could silently switch the page context to a different marking if the cover has multiple markings and the user arrived without a markingId query param.
• Suggested fix: Document the fallback explicitly, or guard the dialog behind a check that markingId is set, and/or reload the page/navigate to the newly active marking.

3. parseCoverIdInput strips any leading C/c regardless of position context

• File: recordLinking.ts ~line 9
• The regex /^[Cc]-?/ strips a leading c (with optional hyphen), so "c42" → "42" → 42. This is tested and works. However, "c-42" (lowercase with hyphen) would also be accepted even though the comment only mentions "C-42" and "c42" as valid forms. This is a minor scope ambiguity rather than a crash bug, but it may accept forms that the backend doesn't recognise, leading to a confusing lookup.
• Suggested fix: Clarify in the JSDoc (or a test case) whether "c-42" is intentionally supported.

4. No test coverage for parseMarkingIdInput("0") (negative ID boundary)

• File: recordLinking.test.ts ~line 25-29
• parseCoverIdInput is tested with "0" and "-3", but parseMarkingIdInput has no equivalent boundary tests. The implementation handles it correctly via value > 0, but the gap in tests means a future refactor could introduce a regression undetected.
• Suggested fix: Add expect(parseMarkingIdInput("0")).toBeNull() and expect(parseMarkingIdInput("-5")).toBeNull().

---

Security Vulnerabilities

1. API error detail displayed directly in the UI

• File: CoverDetail.tsx ~line 549-551, RecordDetail.tsx ~line 758-760
• The string from ax.response?.data?.detail or non_field_errors[0] is rendered as text content inside a <p> tag (not as dangerouslySetInnerHTML), so XSS is not a risk here in React. However, a malicious or misconfigured backend could surface internal stack traces or sensitive path/model information directly to editor-role users through this message.
• Suggested fix: Consider capping the displayed message length and/or only showing the detail string when it passes a safe-string check (no stack-trace patterns), falling back to the generic message otherwise.

2. Client-side staff check is the only UI gate for "Link" buttons

• File: CoverDetail.tsx ~line 800, RecordDetail.tsx ~line 1140
• The diff note says the endpoint is IsEditorOrAdminWrite-gated on the server, which is correct. The client-side isStaff guard is purely cosmetic; the API call itself is protected. This is the right architecture, but worth confirming in review that createCoverMarking will reliably receive a 403 for non-staff callers so the absence of the button in the UI can't be bypassed via the browser console.
• Suggested fix: No code change needed if server auth is confirmed; add a comment cross-referencing the server permission class for future maintainers.