Skip to content

fix(cards): reconcile work-item type editing with older board payloads - #3026

Merged
Chris0Jeky merged 4 commits into
mainfrom
issue-2952/type-selector-legacy-payloads
Sep 11, 2026
Merged

fix(cards): reconcile work-item type editing with older board payloads#3026
Chris0Jeky merged 4 commits into
mainfrom
issue-2952/type-selector-legacy-payloads

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

What

The work-item type selector in the shared card editor gated on
boardStore.currentBoard.canWrite === true. canWrite is optional on the Board
contract, and types/board.ts says why: a payload cached before the field existed
omits it, and surfaces gating on it must treat only an explicit false as
read-only. The strict === true check read that silence as "no", so an otherwise
authorized writer holding an older cached board payload found the control disabled
with no explanation and no way forward.

This replaces the inline gate with useCardTypePermission, which answers the question
with evidence rather than inference:

  • The loaded payload states the permission (canWrite is a boolean, or the board is
    archived) → gate on it, no request. This is every board loaded from the current server,
    so the common path costs nothing.
  • The loaded payload does not state it → read the board back from the server once
    (GET /boards/{id}, the caller-scoped response that carries the server-computed
    CanWrite) and gate on that answer. While the read is open the control stays disabled
    and says it is checking.
  • The read fails → the state stays unknown, nothing is granted, and the editor offers
    an explicit Refresh permission control instead of a silently disabled selector.
  • A fresh payload that still omits the field is a server that predates the field, so the
    documented legacy only-false convention governs it; an archived board is read-only
    whatever the field says.

Viewer, archived board and archived card stay read-only exactly as before and spend no
request. No permission is derived client-side (permissionsStore.canEdit reads
BoardAccess rows, which board owners do not have), and the write itself remains
server-authoritative — this changes only what the UI offers.

CardModal.vue keeps a minimal footprint: the composable call plus the bindings on the
one CardModalForm gate. The assignment, archive-recovery, delete-preview, save and
discard paths landed today by #2977 / #2992 / #3015 / #3011 are untouched.

Follow-up from PR #2949 review comment 3983243109. Closes #2952.

Why not just canWrite !== false

That is the reviewer's minimal suggestion and it does fix the disabled-writer case, but
it grants the control on the absence of evidence in a payload of unknown age. Reading
the current permission from the server keeps the gate evidence-based, which is what the
issue's residual acceptance asks for, and leaves the only-false convention applying where
it belongs: to a fresh answer from a server that has no such field.

Tests

src/tests/composables/useCardTypePermission.spec.ts (new, 11 cases): stated true/false
without a request; missing canWrite → read → granted / denied; fresh payload still
omitting the field → legacy convention; fresh answer says archived → read-only; failed
read → recovery, then granted on the explicit refresh; no automatic retry after a
failure; archived card / unloaded board ask nothing and show no recovery; closed editor
asks nothing; a superseded read never answers another board.

src/tests/components/CardModal.spec.ts (4 new cases): the missing-canWrite regression
(checking → enabled after the authoritative read), the failed-read recovery control and
its retry, archived card, archived board. The existing writer-enabled and viewer-disabled
cases are kept, with the viewer case now also proving no permission request is made.

Checks run

  • npx vitest --run --maxWorkers=2 src/tests/composables/useCardTypePermission.spec.ts src/tests/components/CardModal.spec.ts src/tests/components/CardModalAssignmentSave.spec.ts src/tests/composables/useCardModal.spec.ts src/tests/i18n/catalogs.spec.ts — 169 passed
  • npx vitest --run --maxWorkers=2 on the card-editor hosts: ColumnLane.spec.ts, BoardView.spec.ts, paper/PaperBoardView.spec.ts, paper/PaperCardDetailView.spec.ts, paper/boardMutationCapabilityParity.spec.ts, BoardCanvas.spec.ts — 143 passed
  • npm run typecheck — clean
  • npm run build — clean
  • npx eslint on every touched file — clean
  • node scripts/check-docs-governance.mjs, node scripts/check-doc-links.mjs — pass

NOT verified

  • No full local vitest --run (OOMs on this box without --maxWorkers); the remaining
    suites are CI's to prove.
  • No backend run: no backend, API or contract change in this PR.
  • No browser or Playwright run against a live stack: the "Refresh permission" affordance
    and the checking state are proven at component level only.
  • The server read path is exercised through a mocked boardsApi.getBoard; the real
    GET /boards/{id} response shape is unchanged and untouched here.
  • Spanish and Italian strings are machine-translated, consistent with the rest of those
    catalogs (Seed an i18n translation layer (vue-i18n) with Italian and Spanish locales #1770).

A board payload cached before the optional canWrite field existed omits it, and
the Board contract treats only an explicit false as read-only. The type selector
gated on canWrite === true, so that silence disabled the control for an otherwise
authorized writer with no way forward.

useCardTypePermission reads the board back from the server when the loaded payload
does not state the permission, and gates on that answer. A failed read leaves the
state unknown and the editor offers an explicit Refresh permission control instead
of a silently disabled selector. Viewer, archived board and archived card stay
read-only and cost no request, and no permission is ever derived client-side.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-11T20:29:05.861136Z 8d9cb1c PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d9cb1cf90

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

* field says.
*/
confirmed.value = { boardId, canWrite: board.canWrite !== false && board.isArchived !== true }
} catch {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle 403 as a permission denial

When GET /boards/{id} returns 403—for example, because access was revoked after the cached board loaded—this catch-all converts the definitive denial into permissionUnknown, so the editor misleadingly says the payload omitted permission and offers a futile Refresh loop. Preserve generic recovery for network/5xx failures, but surface 403 as a permission-denied state.

AGENTS.md reference: frontend/AGENTS.md:L6-L6

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triaged once: real, non-blocking (nothing is granted either way; a 403 lands in the unknown state with a recovery control). Tracked as #3030 together with the focus-trap MEDIUM.

checking.value = true
failedBoardId.value = null
try {
const board = await boardsApi.getBoard(boardId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip the permission probe in backend-less demo mode

In backend-less demo mode, buildDemoBoardDetail omits canWrite, so opening any demo card reaches this call even though demo mode explicitly has no server; the shared HTTP client then targets its localhost fallback, retries/logs a network failure, and leaves the demo showing irrelevant permission-recovery UI. Handle demo mode locally or stamp the demo board's read-only capability instead of issuing this request.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed at head 653fc95: demo mode is excluded from the probe (permissionDecides keeps the pre-existing disabled behaviour).

checking.value = true
failedBoardId.value = null
try {
const board = await boardsApi.getBoard(boardId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bound the permission probe before exposing recovery

When the server accepts the connection but never responds, this request has no timeout or abort signal because the shared Axios client defaults to an unlimited timeout. Consequently checking remains true indefinitely, the type selector stays disabled, and the Refresh affordance never appears; pass bounded BoardReadOptions (as the board loader does) so the loading state can reliably transition to the implemented error/recovery state.

AGENTS.md reference: frontend/AGENTS.md:L5-L5

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed at head 653fc95: the probe carries an explicit timeout and skipRetry, and a timed-out read yields the unknown state with recovery, never a grant.

Review round 1 on #3026.

The read now uses the same discipline as every other board read - the board
timeout and skipRetry - so a 503 or a Retry-After cannot hold the control in
'checking' through the shared retry backoff with the recovery affordance hidden
behind it. A superseded or unmounted read is aborted rather than left open.

Demo mode has no server to ask and its board fixtures omit canWrite by
construction, so it keeps the pre-change behaviour instead of offering a refresh
that can never succeed. The retry control stays mounted across the checking and
unknown states so activating it does not drop keyboard focus out of the editor.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review round 1 — independent fresh-context adversarial pass (agent, read-only)

Verdict returned: nothing blocking. No path was found where a viewer, an archived board
or an archived card reaches an enabled selector; no request loop, storm, or permanently
stuck state; the gate is strictly tighter than the canWrite === true one it replaces
(which had no isArchived term at all); and the other lanes' CardModal work is untouched.
Every finding was MEDIUM or LOW. Triage below — fixed in 653fc9590.

Finding Severity Disposition
The permission read was the only board read opting into the shared retry interceptor, with no timeout and no abort — a 503 or a Retry-After could hold the control in "checking" through the whole backoff, with the recovery affordance hidden behind it MEDIUM Fixed. The read now passes { signal, timeout: BOARD_REQUEST_TIMEOUT_MS, skipRetry: true }, the same discipline boardCrudStore uses, and a superseded or unmounted read is aborted rather than left open (onScopeDispose). Two new specs pin the call options and the abort.
Demo mode would fire a live HTTP read (its board fixtures omit canWrite by construction) and land on a permanent unknown state with a Refresh control that can never succeed LOW Fixed. Demo mode keeps the pre-change behaviour — no read, no recovery affordance — with a spec.
Clicking "Refresh permission" unmounted the button that was just activated, dropping keyboard focus out of the editor's tab cycle LOW Fixed. The message swaps inside a persistent role="status" region around a retry control that stays mounted and is disabled while the read is open.
The stale-response spec did not discriminate the generation guard — the board-keyed confirmed value rejects that answer on its own LOW Fixed. Replaced with the case the guard actually exists for: board A → B → A with A's first read still open, where only the generation check stops the oldest answer from speaking for A.
The scope comment justified excluding "card whose board payload is not the loaded one" as an impossibility, when that read could in fact be made LOW Fixed. Reworded as the deliberate scope decision it is.
CardParentField, CardArchiveAction and the assignment field's read-only expression still read an omitted canWrite as "no", so the same editor is internally inconsistent for exactly the user #2952 describes LOW Declined for this PR, and it is the right follow-up: it is a wider behaviour change across three more controls (and the assignment field's own server-authoritative save path), not the type-selector gate this issue scopes. Flagged to the lane coordinator for a tracked follow-up rather than opened here — issue topology is the coordinator's.

Re-verified after the fixes, scoped to the fix diff: useCardTypePermission.spec.ts +
CardModal.spec.ts (67 passed), the card-editor hosts and the i18n catalog guard (118
passed), npm run typecheck, npm run build, npx eslint on the touched files — all clean.

Not verified: the reviewer had no shell, so its retry/demo-mode reasoning was read from
httpRetry.ts and demoData.ts rather than executed; the fixes are proven by the new
specs, not by a live backend.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Coordinator's fresh-context independent review at exact head 653fc95907bf6c75587a9b338d1e6eed96ebb753 against base c565a631a6a821b83e44145f7152caa81e7b8d2b: no CRITICAL/HIGH; one MEDIUM and LOWs tracked. The author's own review round is noted; this is the independent lens for the merge gate, and the PR's round ceiling is reached (law 2d), so findings are tracked rather than fixed here.

Verified by the reviewer from source (read-only):

  • Fail-closed: the only paths to an enabled type control are the server-stamped canWrite on the loaded payload or the response of boardsApi.getBoard; no ownership or "200 means writable" inference, and a 200 with canWrite: false denies.
  • The permissive only-false convention on a fresh payload is unreachable against the shipped server: BoardDetailDto.CanWrite is a non-nullable bool, the Api registers no null-ignoring serializer condition, and the PWA runtime cache never caches /api, so the field is always emitted; the branch matches the documented legacy convention used elsewhere.
  • Stale rejection: generation bumped at the start of each read and checked in both arms, plus a board-keyed result; A to B to A cannot be answered by A's first read.
  • Timeout/abort land in the catch; only a current-generation failure yields the recovery state; nothing rejects unhandled. No store mutation; no request storm (one automatic read per board per instance).
  • cardIsArchived, archived board and demo mode still deny with no read; assignment-save and archive-recovery paths untouched. Locale keys present and consistent in en/es/it.

Tracked as #3030 (law 2c):

Author-run proof at this head: 67 passed on the composable and CardModal specs, 118 on neighbouring suites, typecheck, build, eslint, docs checks. Merging with a merge commit once the hosted checks are green and the aging floor is met.

@Chris0Jeky
Chris0Jeky merged commit 088f1e0 into main Sep 11, 2026
36 checks passed
@Chris0Jeky
Chris0Jeky deleted the issue-2952/type-selector-legacy-payloads branch September 11, 2026 21:04
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Cards] Reconcile work-item type editing with older board payloads

1 participant