Explain and lock assignment controls after edit permission is revoked (#2982) - #3011
Conversation
A 403 on the assignment PUT was folded into the generic unknown-outcome branch, which told a downgraded editor to refresh and retry. A Viewer's participant read still succeeds and a board canWrite cached from before the downgrade still says writable, so that advice produced a retry loop the server would keep refusing. Classify the save failure (permission / conflict / ineligible / unknown) and make only the permission class sticky: it explains that edit permission was revoked, locks the selector and Save assignments, and is not cleared by a successful read. The draft and the current assignees stay readable. Only the parent's server-derived readOnly input turning writable again, or a different card/session, releases the lock. The existing generation guard still drops a 403 that settles after a newer request.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc46a5c57
ℹ️ 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".
| watch(() => props.readOnly, readOnly => { | ||
| if (!readOnly) { | ||
| saveFailure.value = null | ||
| void load() |
There was a problem hiding this comment.
When the board store still caches canWrite: true across a Viewer downgrade, readOnly remains false when the PUT returns 403; restoring Editor access and even refetching a writable board also leaves that boolean false, so this watcher never fires and permissionLost stays sticky until the card/session is remounted. This is the normal quiet-board path because BoardAccessService.UpdateAccessAsync does not publish a board mutation. Use an authoritative permission-refresh signal rather than requiring a false-to-true readOnly transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triaged once: real, non-blocking (the copy names reopening as a release path, so nothing is misrepresented). Tracked as #3021: drive the lock release from an authoritative permission read.
| if (permissionLost.value) return 'Your edit permission was revoked, so this assignment save was refused. Assignment editing stays locked until your board access is restored — refreshing will not unlock it. Your draft and the current assignees stay readable.' | ||
| if (loadFailed.value) return 'Could not load current participants. Your draft is kept.' |
There was a problem hiding this comment.
If the user clicks Refresh after the assignment PUT returned 403 and either follow-up GET fails—for example because access was then fully revoked or the network failed—loadFailed becomes true while saveFailure remains permission. This ordering always renders the old permission message, including the claim that current assignees remain readable, and suppresses the refresh-failure error while stale assignees remain displayed; show both outcomes or give the failed read precedence.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
|
Fresh-context independent review (round 1) at exact head Verified by the reviewer from source (read-only):
Fix-now (MEDIUM, introduced by this head): folding Non-blocking (law 2c):
|
Review round 2. Folding the permission lock into the single `locked` that disabled the whole fieldset also disabled Clear and Cancel, and the 403 branch additionally set needsRefresh. After a 403 the draft could therefore never be returned to baseline for the life of the mount: dirty stayed true, the host's assignmentDirty kept the card modal's own save and archive disabled, and every close path raised the discard dialog. A refresh used to re-enable Cancel. Split the gate. `busy` is every reason the whole field is non-interactive and drives Clear and Cancel; `locked` is `busy` plus the revoked permission and drives only the selector fieldset and Save assignments. The button row moves out of the selector fieldset so each control names its own condition. The 403 branch no longer sets needsRefresh - a refusal is not a stale-state claim and the permission class carries its own lock - and the refresh affordance is shown for the permission case directly. Copy now states the real release condition (the board reporting write permission again, which the background refetch delivers, or reopening the card) instead of claiming a refresh can never unlock it, and names Clear and Cancel as still working. The product doc says the same and no longer claims settlement restores every control. The refresh-does-not-unlock spec now calls save() through the instance so the `if (locked) return` guard is exercised, rather than clicking a disabled button that test-utils never dispatches. New spec covers 403 then Cancel.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78b11f95f
ℹ️ 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".
| <!-- Outside the selector fieldset on purpose: Clear and Cancel only edit the local | ||
| draft, so they follow `busy` and survive a revoked permission. Save is the write | ||
| and names `locked` itself rather than relying on ancestor propagation. --> | ||
| <div v-if="!readOnly" class="flex gap-3"> |
There was a problem hiding this comment.
When an assignment PUT returns 403 with a dirty draft and a later board refresh correctly changes canWrite to false, CardModal changes this prop to readOnly=true; this v-if then removes both Clear and Cancel even though the new permission message promises they still work. assignmentDirty consequently remains true, disabling the other card save/archive actions and requiring the user to discard or reopen the whole editor. Keep the local draft actions available in the permissionLost state while continuing to lock Save.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in round 2 (e78b11f): draft actions stay rendered and enabled after the board reports read-only; only Save is removed.
Self-review of the round-2 fix: the new cancel test had the same weakness the
reviewer flagged on the save test. Cancel used to be disabled by the ancestor
`<fieldset :disabled="locked">` without carrying its own disabled attribute,
and jsdom does not reflect ancestor disabling onto button.disabled - so both
the attribute check and the click passed against the buggy structure while the
control was dead in a real browser. Assert closest('fieldset[disabled]') is
null for Cancel and Clear, which is what actually regressed.
…ed refresh (#2982) Two Codex P2 threads on this round, both inside the lines the round already owns, both cases of the fix's own copy promising something untrue. 1. The permanently-dirty host had a second route. When the board refetch catches up and correctly reports canWrite:false, CardModal sets readOnly=true, and v-if="!readOnly" removed Clear and Cancel outright - so dirty stuck again and the new message's "Clear and Cancel still work" became false exactly when the user needed them. Discarding a draft is a local action, never a write, so draftLocked now drops readOnly while permissionLost holds; the row renders in that state and Save alone stays hidden, because the board does say read-only. 2. A refresh that failed AFTER the refusal set loadFailed while saveFailure stayed 'permission', and the precedence chain rendered only the permission message - hiding the read failure while still claiming the current assignees were readable, with stale assignees on screen. The two states now render one combined message that reports both and drops that promise. The readOnly-transition thread is declined here, not fixed: unlocking without a remount needs an authoritative permission-refresh signal, which lives outside this field's owned paths. The copy already names reopening the card as the release path, so it is honest about the limit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c420e398
ℹ️ 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".
| * alone stops blocking the draft-side controls; every other reason still does, | ||
| * and Save stays gone because the board says this field is read-only. | ||
| */ | ||
| const draftLocked = computed(() => archived.value || props.disabled || loading.value || saving.value || needsRefresh.value || (props.readOnly && !permissionLost.value)) |
There was a problem hiding this comment.
When a permission-loss 403 is followed by a failed Refresh, load() sets needsRefresh to true and this expression disables both Clear and Cancel. Although the combined alert now reports the read failure, if read access has been revoked or the network remains unavailable, assignmentDirty cannot return to baseline, so CardModal continues blocking other card saves and archive actions until the user discards or reopens the whole editor. Keep the draft-only cancellation controls usable while permissionLost is active.
AGENTS.md reference: frontend/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in round 2 (0c7d319): a failed refresh no longer re-locks Clear/Cancel while permission is lost.
…es (#2982) The busy comment still described it as driving Clear and Cancel, which stopped being true when draftLocked took them over. Replace both comments with one block naming busy (shared base), locked (the write path: selector, Save, save()) and draftLocked (the local draft discard), and why the last one exists.
Round 2 fixes pushedHead is now
MEDIUM regression: the draft could never be dismissed after a 403 — fixedConfirmed, and worse than a disabled button: folding Confirmed in The gate is now split rather than one flag:
The button row moved out of the selector fieldset so each control names its own New spec: 403, then Cancel -> LOW: copy vs. the real release condition — fixed"refreshing will not unlock it" was an absolute claim the code does not make.
LOW: the test re-asserted the attribute instead of the guard — fixedCorrect: test-utils skips the dispatch on a disabled button, so that click await (wrapper.vm as unknown as { save: () => Promise<void> }).save()
expect(cardsApi.replaceAssignments).toHaveBeenCalledTimes(1)
The same weakness was in my own new Cancel test — caught and fixed (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3ad8334a8
ℹ️ 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".
| // The sticky permission message must not swallow a read that failed AFTER it, | ||
| // nor keep promising readable assignees once the refresh stopped confirming them. | ||
| if (permissionLost.value && loadFailed.value) return 'Your edit permission was revoked, so this assignment save was refused, and the latest refresh also failed — the assignees shown may be out of date. The participant selector and Save assignments stay locked until this board reports write permission again or you reopen the card. Your draft is kept; refresh again to confirm the current assignees.' | ||
| if (permissionLost.value) return 'Your edit permission was revoked, so this assignment save was refused. The participant selector and Save assignments stay locked until this board reports write permission again or you reopen the card. Your draft and the current assignees stay readable, and Clear and Cancel still work.' |
There was a problem hiding this comment.
When a manager removes the user's board access entirely between the initial load and the PUT, CardAssignmentService.StageReplaceAsync returns the same 403 as it does for a Viewer downgrade because both fail CanWriteBoardAsync. This branch nevertheless claims that the current assignees remain readable, even though revocation also detaches that user from assignments and subsequent participant/card reads will fail, leaving only stale cached data until Refresh corrects the message. Avoid inferring retained read access from the PUT status alone; use a neutral permission message until a read succeeds or distinguish the state using an authoritative follow-up read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triaged once: real, non-blocking (server rejection is correct; the explanation is wrong for full revocation). Tracked as #3021 together with the quiet-board release path.
Third and last route to the same stranded draft, reported by Codex on the previous commit. A Refresh that fails after the 403 sets needsRefresh, which draftLocked still honoured, so Clear and Cancel died again - and if read access is gone for good or the network stays down, the host is permanently dirty with no exit but discarding or reopening the whole editor. I had papered over this one in the combined alert text rather than fixing it; that was the weaker choice, because the text then promised controls that were disabled. While permissionLost holds, neither readOnly nor needsRefresh blocks the draft-side controls. What still blocks them is only what makes discarding meaningless or unsafe: an archived card, an in-flight read or write, or a host that disabled the field. The combined message can therefore say again that the draft can be cleared or cancelled, and now it is true.
Third route to the stranded draft — fixed (
|
Build correction
So the two earlier comments' "not completed locally" note is superseded: every
Still NOT verified, unchanged: no runtime reproduction against a live backend, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c7d319e49
ℹ️ 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".
| */ | ||
| const busy = computed(() => props.readOnly || archived.value || props.disabled || loading.value || saving.value || needsRefresh.value) | ||
| const locked = computed(() => busy.value || permissionLost.value) | ||
| const draftLocked = computed(() => archived.value || props.disabled || loading.value || saving.value || (!permissionLost.value && (props.readOnly || needsRefresh.value))) |
There was a problem hiding this comment.
When a 403-locked draft is followed by Refresh after another editor archives the card, load() sets archived to true while deliberately preserving selected; this term then disables both Clear and Cancel even though the draft can remain dirty. CardModal uses that dirty state to disable CardArchiveAction, so the user cannot even restore the card and must discard or reopen the whole editor. Keep local draft cancellation available in the permissionLost state when the refreshed card is archived, or reset the draft when installing that archived baseline.
Useful? React with 👍 / 👎.
|
Round 2 (fix round) review at exact head Scoped fresh-context verification of the fix:
Triage of everything raised since round 1 (law 2c), all non-blocking:
Author-run proof at this head: targeted vitest 64 passed, typecheck, build (exit 0), eslint, docs checks. Base has moved since the branch point; the merge state is re-read before merging. |
1 participant
Closes #2982
What
CardAssignmentFieldfolded a 403 on the assignment PUT into the genericunknown-outcome branch: "Could not confirm assignment save. Refresh before
retrying." This change classifies the save failure and makes the forbidden case
its own, sticky state.
saveFailureis now a class (permission/conflict/ineligible/unknown) instead of a pre-rendered string, and the alert copy is derivedfrom it. Non-permission classes behave exactly as before.
permission(403) explains that edit permission was revoked and locks thewrite controls: it feeds
locked, so the participant selector andSave assignments are disabled, and the Save button now names
lockedin itsown
:disabledbinding instead of relying only on ancestor fieldsetpropagation.
the current assignees stay rendered, and "Refresh current assignments" still
works as a read.
Why the lock is sticky
A downgrade to Viewer leaves every read this field makes working. So a
successful participants refresh is not evidence of write permission, and a
board
canWritecached from before the downgrade is not evidence either.Clearing the lock on either signal is what produced the repeated rejected
saves the review comment described
(#2977 (comment)).
permissionis therefore cleared only by:readOnlyinput transitioning back to writable(the field's authoritative permission input), or
A completed
load()clears everything exceptpermission. The existinggeneration guard is untouched, so a 403 that settles after a newer request is
still dropped before it can set the class.
This matches the
permissionvstransientidiom PR #2991 shipped inCardParentField.vue, so the two fields read the same way.Scope
Frontend only:
CardAssignmentField.vue, its spec, and one sentence indocs/product/CARD_ASSIGNMENTS.md.CardModal.vue/useCardModal.tsweredeliberately not touched (owned concurrently by #2969); no change was needed
there.
Checks run
npx vitest --run --maxWorkers=2 src/tests/components/CardAssignmentField.spec.ts- 12 passed(5 new: downgrade-before-save locks with the explanation and a kept draft; a
successful Viewer participants refresh does not unlock; restored parent
permission unlocks and saves; a superseded 403 is ignored; the 400 path stays
refreshable and unlockable, in contrast to the permission path)
npx vitest --run --maxWorkers=2 src/tests/components/CardModal.spec.ts src/tests/components/CardModalAssignmentSave.spec.ts- 49 passednpm run typecheck- cleannpm run build- cleannpx eslinton both touched frontend files - cleannode scripts/check-docs-governance.mjs- passednode scripts/check-doc-links.mjs- passed (695 files, 0 broken links)NOT verified
editor-downgraded-to-Viewer session here, so the 403 is exercised only
through the mocked
cardsApiin the component spec. The issue itself notesthe finding is source-traced and not runtime-reproduced.
card-assignments.spec.tsneeds a running stack).behavior is unchanged and already covered by
CardAssignmentApiTests.repo's proving-check table).
readOnlyback to writable withoutremounting the card is not proven here; if it does not, the card/session
identity reset (reopening the card) remains the release path.