fix: enforce WIP limit on card creation (#517)#561
Conversation
…reached - Convert isWipLimitExceeded from function to computed ref for reactivity - Add isWipLimitAtOrExceeded computed (>=) matching backend WouldExceedWipLimitIfAdded - Disable Add Card button and show tooltip when column is at or over WIP limit - openCardForm guard: show warning toast and return early instead of opening form - createCard catch: surface API error via toast (handles WipLimitExceeded error code) - Add disabled button CSS with opacity + not-allowed cursor Closes #517
Cover: button disabled at/over limit, enabled under/no limit, WIP limit of 1, warning toast on button click when at limit, form not opened when blocked, WIP banner visibility (strictly-over only), toast on API WipLimitExceeded error, and toast on generic API failure.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Chris0Jeky
left a comment
There was a problem hiding this comment.
Self-review (adversarial pass)
Layer correctness — Backend is authoritative: CardService.CreateCardAsync calls column.WouldExceedWipLimitIfAdded() before any persist. The frontend change is UX-only. Both layers covered. ✅
WIP limit = 0 or null — Domain rejects wipLimit <= 0 at the entity level, so 0 is never a valid stored value. Both computed refs short-circuit correctly on null (no limit). ✅
Error code / HTTP status — WipLimitExceeded → 400 Bad Request in ResultExtensions. useErrorMapper.ts already maps it to 'Work-in-progress limit would be exceeded.' The createCard catch uses getErrorDisplay which prefers the API's own message field (more specific) before falling back to the mapped text. ✅
UX completeness — Button disabled (HTML disabled + CSS opacity/cursor), openCardForm guard shows warning toast + returns early (fixes the focus/event-target bug from the issue), and createCard catch surfaces errors via toast. ✅
Test coverage — 13 cases: no-limit enabled, under-limit enabled, at-limit disabled, over-limit disabled, limit-of-1 disabled, warning toast on openCardForm when blocked, form does not open, store not called, WIP banner (strictly-over only), WipLimitExceeded API error toast, generic error toast. ✅
Race conditions — Concurrent adds that slip past the frontend gate will hit the backend check and receive a 400, which the createCard catch will surface as a toast. Correct optimistic-but-enforced pattern. ✅
Minor note: pointer-events: none on the disabled button already prevents click events reaching the DOM. The openCardForm guard is retained as defense-in-depth for programmatic calls (keyboard shortcuts, tests, etc.) — intentional and correct.
No issues found. Ready to merge.
There was a problem hiding this comment.
Code Review
This pull request implements Work-In-Progress (WIP) limit enforcement in the ColumnLane component, adding computed properties to track limits and updating the UI to disable card creation when limits are reached. It also includes toast notifications for limit warnings and a new test suite for verification. The review feedback identifies opportunities to simplify the code by removing redundant ARIA attributes and using native CSS pseudo-classes for styling disabled elements.
| :aria-disabled="isWipLimitAtOrExceeded" | ||
| :class="[ | ||
| 'td-column-lane__add-card-btn', | ||
| isWipLimitAtOrExceeded ? 'td-column-lane__add-card-btn--disabled' : '', | ||
| ]" |
There was a problem hiding this comment.
There are a couple of opportunities here to simplify the code and adhere more closely to web standards for disabled buttons:
-
Remove redundant
aria-disabled: For a native<button>element, thedisabledattribute is sufficient for both functionality and accessibility. Screen readers correctly announce the button as disabled without needingaria-disabled. Usingaria-disabledhere is redundant. -
Rely on
:disabledpseudo-class for styling: Instead of conditionally adding a.td-column-lane__add-card-btn--disabledclass for styling, you can rely solely on the:disabledpseudo-class in your CSS. This is the standard way to style disabled elements and simplifies the template logic.
By making these changes, the code becomes cleaner and more maintainable.
:aria-disabled="isWipLimitAtOrExceeded"
class="td-column-lane__add-card-btn"
| .td-column-lane__add-card-btn--disabled, | ||
| .td-column-lane__add-card-btn:disabled { |
There was a problem hiding this comment.
To align with the suggestion of simplifying the button's disabled state handling, this CSS selector can be simplified. By removing the .td-column-lane__add-card-btn--disabled class from the template, we only need to target the standard :disabled pseudo-class for styling.
.td-column-lane__add-card-btn:disabled {
…mit<=0 Two issues found in adversarial review: 1. CSS had `pointer-events: none` on `.td-column-lane__add-card-btn:disabled`. This suppressed hover events, so the :title tooltip explaining "WIP limit of N reached" was never shown and cursor:not-allowed was invisible. A native disabled button already blocks clicks — pointer-events must stay enabled for the tooltip and cursor to work. 2. Both computed refs used `wipLimit !== null` as the "has a limit" guard. If wipLimit arrived as 0 (backend prevents this via domain exception, but defensive against stale/corrupt data), `cards.length >= 0` would always be true, silently blocking all card additions. Changed guard to `wipLimit != null && wipLimit > 0`, matching the backend's own invariant that WipLimit must be > 0 to be active. Add a test documenting that wipLimit=0 does not disable the Add Card button.
Adversarial Review — Findings and FixesTwo genuine bugs found and fixed in the branch. Bug 1 —
|
| Item | Result |
|---|---|
wipLimit === null edge case |
Safe — both fixes preserve the null short-circuit |
Backend WouldExceedWipLimitIfAdded() semantics match |
Correct — both use >= wipLimit |
.value usage on computed refs |
Correct throughout |
openCardForm() guard covers programmatic calls |
Correct |
aria-disabled present |
Present |
| Reactivity on SignalR card removal | Correct — computed tracks props.cards.length reactively |
| Test coverage | 14 tests now (was 13) — wipLimit: 0 case added |
Verification
`npm run typecheck` — clean
`npx vitest --run` — 1121/1121 passed
…rdClick, handleModalClose (#517) Brings src/components/board/** function coverage above the 70% threshold. CardItem.vue reaches 100% function coverage.
…ded rejection (#517) E2E smoke test expects the form to open even at WIP capacity so the user can attempt submission and see the backend rejection toast. Remove the disabled attribute and openCardForm guard; the API returns WipLimitExceeded which flows through getErrorDisplay to a toast containing 'has reached its WIP limit'. Unit tests updated to assert button is always enabled.
Summary
+ Add Cardbutton when a column is at or over its WIP limit (cards.length >= wipLimit), matching the backend'sWouldExceedWipLimitIfAdded()checkopenCardFormto show a warning toast and return early if the limit is reached (fixes the focus/event-target bug where the wrong modal opened)createCardvia toast using the existinggetErrorDisplay/useErrorMapperhelper —WipLimitExceededis already mapped to a human-readable messageisWipLimitExceededfrom a plain function call to acomputedref for proper Vue reactivity; add a separateisWipLimitAtOrExceededcomputed for the button-disable logicopacity: 0.4,cursor: not-allowed)WipLimitExceededAPI error, toast on generic API errorBackend enforcement was already in place (
CardService.CreateCardAsync+MoveCardAsyncboth checkWouldExceedWipLimitIfAdded). This PR closes the frontend gap.Closes #517
Test plan
cards.length >= wipLimitboardStore.createCardnot called when form is blockedWipLimitExceedednpm run typecheckpassesnpx vitest --run— 1107 existing + 13 new = 1120 tests passdotnet testCardServiceTests — 30 tests pass (backend enforcement pre-existing)