fix(qa): resolve all P0-P2 bugs from QA report #54
Conversation
eslint-plugin-react@7.37.5 (via eslint-config-next) uses the removed context.getFilename() API, causing a crash on ESLint 10.
…/.dark [data-theme='xxx'] and :root/.dark all share specificity (0,1,0). Because themes.css is @imported before :root/.dark in globals.css, the base rules always won the cascade — silently overriding all 14 theme variables. Fix: html[data-theme='xxx'] → specificity (0,1,1), guaranteed to win. Add E2E tests verifying computed CSS variables actually change for every theme (catches this class of specificity bug).
Address 4 P0, 8 P1, 8 P2 bugs across Visual, HIG, Functional, and
Edge Case dimensions identified in qa-summary.md.
Phase 1 - CSS & Layout:
- Replace hardcoded gray/blue colors with theme tokens (login, boards, landing)
- Add prefers-reduced-motion and prefers-contrast media queries
- Add SF Pro font stack + antialiased rendering
- Add github/xl button variants, remove duplicate cn/buttonVariants from landing
- Mobile header stacking, responsive landing grid
- Bump muted-foreground contrast across 7 light themes
Phase 2 - Accessibility:
- 44px tap areas on checkbox, switch, column menu via ::after pseudo-elements
- useReducedMotion() in StatusColumn, MaintenanceClient, CommandPalette, KanbanBoard
Phase 3 - Validation & Business Logic:
- Auth checks on mutation server actions
- Atomic column swap via PostgreSQL RPC (swap_statuslist_positions)
- Comment max length 300→2000 (shared COMMENT_MAX_LENGTH)
- Undo DB sync (batchUpdate after Redux dispatch)
- Zod validation for statusList name/color, grid positions, board name
Phase 4 - UI Enhancement:
- Cascade warning in column delete dialog ("also removes N cards")
Tests updated for new max length, useReducedMotion mock, RPC types.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds theme visual E2E tests; moves theme selectors to Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (Browser)
participant Server as App Server / Actions
participant RPC as Supabase RPC
participant DB as Postgres Table
UI->>Server: request swapStatusListPositions(idA,idB)
Server->>Server: validate ids, auth, input
Server->>RPC: call swap_statuslist_positions(idA,idB)
RPC->>DB: SELECT ... FOR UPDATE (lock rows)
DB-->>RPC: return rows
RPC->>DB: UPDATE swap grid_row/grid_col for both rows
DB-->>RPC: confirm updates
RPC-->>Server: success/void
Server-->>UI: success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 74.65% 74.53% -0.12%
==========================================
Files 118 118
Lines 3764 3821 +57
Branches 1013 1050 +37
==========================================
+ Hits 2810 2848 +38
- Misses 938 951 +13
- Partials 16 22 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/Board/CommentInlineEdit.tsx (1)
68-68:⚠️ Potential issue | 🟡 MinorStale JSDoc: default is 2000, not 300.
The
@param maxLengthdocs say "default: 300" but the actual default is nowCOMMENT_MAX_LENGTH(2000).Fix
- * `@param` maxLength - Character limit (default: 300) + * `@param` maxLength - Character limit (default: COMMENT_MAX_LENGTH, currently 2000)src/components/Board/CommentInlineEdit.stories.tsx (1)
99-127:⚠️ Potential issue | 🟡 Minor
WarningStateandOverLimitstories no longer demonstrate their intended states.With the default
maxLengthnow at 2000:
WarningState(~228 chars) is far below the 90% warning threshold of 1800.OverLimit(~301 chars) is far below 2000.These stories need either explicit
maxLength: 300args (likeTestWarningThreshold/TestOverLimitDisabled) or longerinitialValuestrings to visually match their names.Proposed fix
export const WarningState: Story = { args: { initialValue: 'This is a very long comment that is approaching the character limit. We want to show the warning state when the user types more than 270 characters. The counter should turn orange to indicate that they are close to the limit.', + maxLength: 300, onSave: async (value, options) => {export const OverLimit: Story = { args: { initialValue: 'This comment exceeds the maximum character limit of 300 characters. The counter should turn red and the save button should be disabled. This is to prevent users from submitting comments that are too long for the database to store. The textarea should still allow typing but saving should not be possible.', + maxLength: 300, onSave: async (value, options) => {src/lib/actions/board.ts (1)
351-383:⚠️ Potential issue | 🟡 Minor
batchUpdateStatusListPositionsskips thegridPositionSchemavalidation added to its sibling.
updateStatusListPositionnow validates withgridPositionSchema, but this batch variant accepts the samegridRow/gridColfields without validation. Invalid positions (negative, non-integer) can slip through here.Proposed fix
export async function batchUpdateStatusListPositions( updates: Array<{ id: string; gridRow: number; gridCol: number }>, ): Promise<void> { + // Validate all grid positions (P2-4) + for (const u of updates) { + gridPositionSchema.parse({ gridRow: u.gridRow, gridCol: u.gridCol }) + } + const supabase = await createClient()
🤖 Fix all issues with AI agents
In `@e2e/logged-in/theme-visual-application.spec.ts`:
- Around line 145-156: The uniqueness test loops over THEMES.dark and
THEMES.light but do not assert visibility before clicking, causing flaky
failures; update both loops (the one using THEMES.dark that creates themeButton
via page.locator(...) and the analogous THEMES.light loop) to await
expect(themeButton).toBeVisible() immediately before await themeButton.click(),
then proceed to call getBackgroundVar(page) and store into
backgrounds[theme.id]; this ensures themeButton is rendered before interaction.
In `@src/styles/globals.css`:
- Around line 184-193: The media query for high-contrast only updates :root and
.dark; add matching overrides for all 14 theme selectors so each theme gets
high-contrast values for --muted-foreground and --border. Update the `@media`
(prefers-contrast: more) block to include the light theme selectors
html[data-theme='default'], html[data-theme='sunrise'],
html[data-theme='sandstone'], html[data-theme='mint'], html[data-theme='sky'],
html[data-theme='lavender'], html[data-theme='rose'] with appropriate light
high-contrast oklch values and the dark theme selectors html[data-theme='dark'],
html[data-theme='midnight'], html[data-theme='graphite'],
html[data-theme='forest'], html[data-theme='ocean'], html[data-theme='plum'],
html[data-theme='rust'] with appropriate dark high-contrast oklch values so
--muted-foreground and --border are explicitly overridden for each theme instead
of relying on :root/.dark only.
In `@src/supabase/migrations/20260211_swap_statuslist_positions_rpc.sql`:
- Around line 19-24: The current SELECT ... FOR UPDATE on statuslist using id_a
and id_b can deadlock if two transactions call swap(A,B) and swap(B,A); change
the locking to acquire row locks in a deterministic order by comparing id_a and
id_b and selecting/locking the smaller id first and the larger id second (e.g.,
use a branch on id_a < id_b or a single query that orders ids) so that the rows
read into row_a/col_a and row_b/col_b are always locked in the same sequence;
update references to statuslist, id_a, id_b, row_a, col_a, row_b, col_b and the
FOR UPDATE statements accordingly.
- Around line 20-28: The current checks only validate row_a and row_b and rely
on FOUND which reflects only the last SELECT INTO; update the validation to
ensure both selected rows and columns exist by checking col_a and col_b as well
(or check FOUND after each SELECT INTO). Specifically, after the two SELECT ...
INTO statements that populate row_a, col_a, row_b, col_b, add validation that
row_a IS NOT NULL AND col_a IS NOT NULL AND row_b IS NOT NULL AND col_b IS NOT
NULL (or immediately test FOUND after the first SELECT INTO for id_a and after
the second for id_b) and raise the same exception if any are NULL/missing so the
swap cannot write NULL grid_col values.
🧹 Nitpick comments (9)
src/components/Board/StatusColumn.tsx (1)
118-125: Tap target is ~42px, slightly under the 44px guideline.The button is
h-6 w-6(24px) withafter:-inset-[9px], yielding 24 + 18 = 42px. For consistency with the checkbox/switch (which target exactly 44px), considerafter:-inset-[10px].Proposed fix
- className="hover:bg-accent relative h-6 w-6 p-0 after:absolute after:-inset-[9px] after:content-['']" + className="hover:bg-accent relative h-6 w-6 p-0 after:absolute after:-inset-[10px] after:content-['']"src/app/login/page.tsx (1)
34-54: Consider using the<Button>component with the newgithubvariant.This PR adds a
githubvariant andxlsize to<Button>, which seems tailor-made for this login button. Using a raw<button>with hand-rolled classes duplicates what's already available in the shared component. As per coding guidelines, prefer reusing UI components from/components/ui.Proposed refactor
+import { Button } from '@/components/ui/button' + <form action={signInWithGitHub}> - <button - type="submit" - className="bg-primary text-primary-foreground hover:bg-primary/90 focus:ring-ring flex w-full items-center justify-center gap-3 rounded-lg px-4 py-3 transition-colors focus:ring-2 focus:ring-offset-2 focus:outline-none" - > + <Button type="submit" variant="github" size="xl" className="w-full gap-3"> {/* GitHub Icon */} <svg ...>...</svg> <span className="font-semibold">Sign in with GitHub</span> - </button> + </Button> </form>src/app/page.tsx (1)
23-38: Unnecessary indirection — alias adds no value.
const Button = SharedButtonis a no-op rename. Since the import can be aliased directly, this intermediate binding is dead weight.♻️ Simplify the import
-import { Button as SharedButton } from '@/components/ui/button' +import { Button } from '@/components/ui/button' import { cn } from '@/lib/utils' -// Use shared Button from `@/components/ui/button` (supports github variant + xl size) -const Button = SharedButtonsrc/app/board/[id]/BoardPageClient.tsx (1)
399-408: Extract the card count computation out of JSX.The IIFE inside
AlertDialogDescriptionadds unnecessary cognitive overhead. Compute it before the return statement for clarity.♻️ Move card count to a variable
Compute alongside other derived state (e.g., near line 82):
+ const pendingDeleteCardCount = repoCards.filter( + (c) => c.statusId === statusListDialog.pendingDeleteStatusId, + ).lengthThen simplify the JSX:
"? - {(() => { - const cardCount = repoCards.filter( - (c) => c.statusId === statusListDialog.pendingDeleteStatusId, - ).length - if (cardCount > 0) { - return ` This will also remove ${cardCount} card${cardCount !== 1 ? 's' : ''} in this column.` - } - return '' - })()}{' '} + {pendingDeleteCardCount > 0 && + ` This will also remove ${pendingDeleteCardCount} card${pendingDeleteCardCount !== 1 ? 's' : ''} in this column.`}{' '} This action cannot be undone.As per coding guidelines: "Extract helper functions as pure functions below component definition rather than keeping logic inline."
e2e/logged-in/theme-visual-application.spec.ts (1)
69-69: PreferwaitForor locator assertions overwaitForTimeout.Hard-coded
waitForTimeout(1000)andwaitForTimeout(500)make tests slow and flaky. Consider replacing these with Playwright's built-in auto-waiting, e.g. wait for the theme button to be visible or for a CSS variable to change. The 500ms waits after click could be replaced by asserting the expecteddata-themeattribute (which already has a 5s timeout on line 84).Also applies to: 108-108, 141-141, 169-169
src/components/Board/KanbanBoard.tsx (2)
389-401: Undo DB sync is fire-and-forget — no UI revert on failure.If
batchUpdateStatusListPositionsfails, the Redux state already reflects the undo but the DB retains the pre-undo positions. The toast notification is good, but the user has no way to retry or know the exact consequence. Consider whether a retry or explicit revert is needed, or if this is acceptable given the low failure likelihood.Same pattern applies to the card undo on lines 413-425.
784-793:exitanimation won't fire withoutAnimatePresence.The
exit={{ opacity: 0 }}on line 788 requires the parent to wrap this conditional in<AnimatePresence>. Currently, whenundoMessagebecomesnull, React unmounts the element immediately without the exit transition. This is pre-existing behavior but worth noting if you want the fade-out to actually work.src/lib/actions/board.ts (2)
156-158:parse()throws a rawZodErrorto the client — prefersafeParse()for server actions.
renameBoardActionanddeleteBoardActionusesafeParse()and return structured error objects. Here (and inupdateStatusList,updateStatusListPosition,createBoard),.parse()throws aZodErrorthat will surface as an opaque server error to the caller. Consider aligning onsafeParse()for consistent, user-friendly error handling.
245-268: Auth check added todeleteStatusListbut absent fromupdateStatusList— inconsistent.Both are mutation actions on the same resource. While RLS ultimately enforces ownership, the explicit auth guard pattern should be consistent across all mutations to fail fast with a clear error.
src/supabase/migrations/20260211_swap_statuslist_positions_rpc.sql
Outdated
Show resolved
Hide resolved
src/supabase/migrations/20260211_swap_statuslist_positions_rpc.sql
Outdated
Show resolved
Hide resolved
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
- Add toBeVisible() assertion before clicking theme buttons in uniqueness E2E tests - Add high-contrast overrides for all 14 theme selectors (not just :root/.dark) - Fix deadlock risk in swap RPC with deterministic lock ordering (id_a < id_b) - Add col_a/col_b NULL checks to swap RPC validation
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
2 participants
Summary
qa-summary.md(score 53.5/100 FAIL)Changes by Phase
Phase 1: CSS & Layout (P0-1, P0-2, P1-1, P1-2, P1-3, P1-4, P2-5, P2-6, P2-8)
bg-background,bg-primary)globals.css: SF Pro font stack,prefers-reduced-motion,prefers-contrast: morebutton.tsx:githubvariant +xlsize; landing page deduped (removed local cn/buttonVariants)Phase 2: Accessibility (P0-3, P0-4)
::afterpseudo-elementsuseReducedMotion()in 4 components (StatusColumn, MaintenanceClient, CommandPalette, KanbanBoard)Phase 3: Validation & Business Logic (P1-5, P1-6, P1-7, P1-8, P2-1, P2-3, P2-4)
getUser()) on mutation server actionsswap_statuslist_positions(new migration)COMMENT_MAX_LENGTH), prop-relative warning thresholdbatchUpdateStatusListPositions/batchUpdateRepoCardOrdersafter Redux dispatchPhase 4: UI Enhancement (P2-2)
Tests
useReducedMotionto StatusColumn test mockswap_statuslist_positionsRPC type to Supabase typesSkipped
Test plan
prefers-reduced-motion: reduce→ verify no animationsSummary by CodeRabbit
New Features
Improvements
Tests