Feature/mutations - #36
Merged
Merged
Conversation
Notifications are the app's first custom context — the first value needing tree-wide reach. The provider owns a ten-entry history with read tracking; useNotify serves the mutation call sites and useNotifications serves the header bell. Provider and hooks live in separate files so react-refresh keeps hot reload. Transient toasts were prototyped and deliberately dropped: two presentations of one event prototyped and deliberately dropped: two presentations of one event stream duplicated a function, and failures already surface inline in the dialog that caused them.
…audit fixes the bell, delete-dialog inline alert, and doc updates
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds reusable task forms and CRUD modals, board/list layouts, task list rendering, profile-based task loading, shared display helpers, and a notification provider with a header notification center. ChangesTask management and notifications
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CreateTaskModal
participant TaskForm
participant GraphQLClient
participant NotificationsContext
User->>CreateTaskModal: Submit task form
CreateTaskModal->>TaskForm: Collect validated task fields
CreateTaskModal->>GraphQLClient: Create task mutation
GraphQLClient-->>CreateTaskModal: Return mutation result
CreateTaskModal->>NotificationsContext: Add success or error notification
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/features/tasks/TaskForm.tsx (1)
231-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
aria-haspopupwith the rendered popup roles.The estimate, label, assignee, and status triggers declare
aria-haspopup="listbox", butMenuPanelrenders a plaindivthat containsbuttonelements. Nolistboxoroptionrole exists. The date trigger at Line 416 declaresaria-haspopup="dialog", butDatePickerandDatePickerDialoguserole="group". Screen readers announce a popup type that the user never receives.
TaskCardalready uses the matching pattern:aria-haspopup="menu"withrole="menu"androle="menuitem". Reuse it here.♻️ Proposed change for the estimate trigger and panel
<button type="button" - aria-haspopup="listbox" + aria-haspopup="menu" aria-expanded={openMenu === 'estimate'}Apply the same change to the label, assignee, and status triggers. Then add the matching roles in
MenuPaneland its items:<div + role="menu" className={`absolute top-full left-0 z-10 mt-2 flex flex-col rounded-lg border border-neutral-2 bg-neutral-3 py-2 shadow-drop-large ${className}`} >For the date trigger, either change it to
aria-haspopup="true"or giveDatePickerandDatePickerDialogrole="dialog".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/tasks/TaskForm.tsx` around lines 231 - 232, Align popup accessibility semantics in TaskForm: change the estimate, label, assignee, and status triggers to aria-haspopup="menu", and update MenuPanel plus its button items to use matching menu and menuitem roles, following TaskCard’s pattern. For the date trigger, either use aria-haspopup="true" or update DatePicker and DatePickerDialog to expose role="dialog".src/app/router.test.tsx (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the mock reject unknown documents instead of returning tasks.
The fallback branch returns
{ tasks: ... }for every document that is notProfileDocument.UsersDocument,CreateTaskDocument,UpdateTaskDocument, andDeleteTaskDocumentall resolve to a task list. A future test that opensTaskFormwould receive{ tasks: [...] }for the users query, andusers.data?.userswould beundefined. The assignee menu would render empty and the test would pass for the wrong reason.Map each document explicitly and reject the rest.
♻️ Proposed change to the request mock
- tasksRequestMock().mockImplementation((document: unknown) => - document === ProfileDocument - ? Promise.resolve({ profile: makeFixtureProfile() }) - : Promise.resolve({ tasks: makeFixtureTasks() }), - ) + tasksRequestMock().mockImplementation((document: unknown) => { + if (document === ProfileDocument) return Promise.resolve({ profile: makeFixtureProfile() }) + if (document === TasksDocument) return Promise.resolve({ tasks: makeFixtureTasks() }) + return Promise.reject(new Error('Unmocked GraphQL document')) + })Import
TasksDocumentfrom@/features/tasks/queries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/router.test.tsx` around lines 19 - 23, Update the tasksRequestMock implementation to handle ProfileDocument and TasksDocument explicitly, returning the corresponding profile or task fixtures. Reject any other document, including user and task mutation documents, instead of falling through to the tasks response; import TasksDocument from the tasks queries module.src/test/fixtures.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture pair that exercises the
createdAttie-breaker.
groupTasksByStatusnow falls back toa.createdAt.localeCompare(b.createdAt)when positions match (task-display.ts Line 19). No two fixture tasks share both a status and a position, so the new comparator branch never runs in the tests. Give twoIN_PROGRESStasks the samepositionto cover it.Also applies to: 30-30, 41-41, 52-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/fixtures.ts` at line 19, Update the task fixtures around createdAt and the related entries so two IN_PROGRESS tasks share the same position while retaining distinct createdAt values. Ensure the fixture pair exercises the createdAt tie-breaker in groupTasksByStatus without changing unrelated statuses or ordering expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/NotificationsBell.tsx`:
- Line 20: Remove the aria-haspopup attribute from the notification bell control
in NotificationsBell, since its notification panel is not a menu and does not
provide menu interaction behavior.
In `@src/features/tasks/DeleteTaskDialog.tsx`:
- Around line 40-84: Update the DeleteTaskDialog component’s focus handling to
match TaskForm: focus the “Go back” button on open, trap Tab and Shift+Tab
within the dialog controls, and restore focus to the element that opened the
dialog when dismissing or closing. Reuse the existing TaskForm focus-management
pattern and preserve the current delete/error behavior.
In `@src/features/tasks/TaskCard.tsx`:
- Around line 116-131: Update TaskCard’s dialog handling to store the “Task
options” trigger element in a ref and use a shared closeDialog callback to close
the dialog and restore focus to that trigger after unmount. Wire both
EditTaskModal and DeleteTaskDialog onClose handlers to closeDialog, while
preserving TaskForm’s existing generic cleanup behavior for CreateTaskModal.
In `@src/features/tasks/TaskForm.tsx`:
- Around line 109-121: Update src/features/tasks/TaskForm.tsx lines 109-121 in
the existing keydown effect to clear openMenu and return when a menu is open,
call onClose only when no menu is open, and include openMenu in the
dependencies. Update src/features/tasks/TaskCard.tsx lines 44-54 by adding
Escape handling to the role="menu" element that calls setMenuOpen(false).
Consider sharing a useDismissOnEscape(active, onDismiss) hook between both
sites.
- Around line 343-364: Update the assignee menu rendering around users.data in
TaskForm to handle users.isPending and users.isError explicitly instead of
always falling back to an empty list. Show a short loading message while pending
and an error message with a retry action after failure, using the query’s
existing refetch mechanism; retain the user list rendering for successful
results.
In `@src/features/tasks/TaskList.tsx`:
- Around line 36-83: Update TaskRow to expose task actions by adding an actions
cell or reusing the TaskCard action menu, wired to open EditTaskModal and
DeleteTaskDialog for the current task. Preserve the existing list columns and
ensure the controls are available when TasksView renders the list layout.
---
Nitpick comments:
In `@src/app/router.test.tsx`:
- Around line 19-23: Update the tasksRequestMock implementation to handle
ProfileDocument and TasksDocument explicitly, returning the corresponding
profile or task fixtures. Reject any other document, including user and task
mutation documents, instead of falling through to the tasks response; import
TasksDocument from the tasks queries module.
In `@src/features/tasks/TaskForm.tsx`:
- Around line 231-232: Align popup accessibility semantics in TaskForm: change
the estimate, label, assignee, and status triggers to aria-haspopup="menu", and
update MenuPanel plus its button items to use matching menu and menuitem roles,
following TaskCard’s pattern. For the date trigger, either use
aria-haspopup="true" or update DatePicker and DatePickerDialog to expose
role="dialog".
In `@src/test/fixtures.ts`:
- Line 19: Update the task fixtures around createdAt and the related entries so
two IN_PROGRESS tasks share the same position while retaining distinct createdAt
values. Ensure the fixture pair exercises the createdAt tie-breaker in
groupTasksByStatus without changing unrelated statuses or ordering expectations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 463ced64-6f02-4bf3-aa19-77a9e54a68f7
⛔ Files ignored due to path filters (2)
src/graphql/generated/gql.tsis excluded by!**/generated/**src/graphql/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (25)
README.mdsrc/app/router.test.tsxsrc/components/layout/Header.tsxsrc/components/layout/NotificationsBell.tsxsrc/components/ui/notifications-context.tssrc/components/ui/notifications.tsxsrc/features/tasks/CreateTaskModal.tsxsrc/features/tasks/Dashboard.tsxsrc/features/tasks/DatePicker.tsxsrc/features/tasks/DeleteTaskDialog.tsxsrc/features/tasks/EditTaskModal.tsxsrc/features/tasks/MyTask.tsxsrc/features/tasks/QueryErrorAlert.tsxsrc/features/tasks/TaskCard.tsxsrc/features/tasks/TaskForm.tsxsrc/features/tasks/TaskList.tsxsrc/features/tasks/TasksView.tsxsrc/features/tasks/Toolbar.tsxsrc/features/tasks/icons.tsxsrc/features/tasks/queries.tssrc/features/tasks/task-display.tssrc/features/tasks/types.tssrc/features/tasks/useTasks.tssrc/main.tsxsrc/test/fixtures.ts
💤 Files with no reviewable changes (1)
- src/features/tasks/useTasks.ts
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/tasks/TaskForm.tsx (1)
122-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
positionbefore submission.
min="1"does not validate this custom submission flow. A user can enter0, a negative value, or a decimal value.canSubmitaccepts these values.EditTaskModalthen omits nonpositive values, so the mutation can succeed while leaving the position unchanged.Require a whole number greater than or equal to one when
showPositionis true. Show a position-specific validation error.Proposed fix
+ const positionNumber = Number(position) + const positionIsValid = + !showPosition || (Number.isInteger(positionNumber) && positionNumber >= 1) - const canSubmit = name.trim().length > 0 && estimate !== null && dueDate !== '' + const canSubmit = + name.trim().length > 0 && estimate !== null && dueDate !== '' && positionIsValid ... <input type="number" min="1" + step="1" ... - {canSubmit ? errorMessage : 'An estimate and a due date are required.'} + {canSubmit + ? errorMessage + : !positionIsValid + ? 'Position must be a whole number of at least 1.' + : 'An estimate and a due date are required.'}Also applies to: 381-388, 433-439
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/tasks/TaskForm.tsx` around lines 122 - 130, Update canSubmit and submit in TaskForm so that when showPosition is true, position must be a whole number greater than or equal to 1; reject zero, negative, decimal, and invalid values before calling onSubmit. When this validation fails, enable showPosition-specific validation feedback alongside the existing showValidation handling, and preserve the current behavior when showPosition is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/features/tasks/TaskForm.tsx`:
- Around line 122-130: Update canSubmit and submit in TaskForm so that when
showPosition is true, position must be a whole number greater than or equal to
1; reject zero, negative, decimal, and invalid values before calling onSubmit.
When this validation fails, enable showPosition-specific validation feedback
alongside the existing showValidation handling, and preserve the current
behavior when showPosition is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0d80041-d88b-4c62-9ff0-c0d404e71907
📒 Files selected for processing (8)
README.mdsrc/components/layout/NotificationsBell.tsxsrc/components/ui/use-dialog-focus.tssrc/features/tasks/DeleteTaskDialog.tsxsrc/features/tasks/TaskActions.tsxsrc/features/tasks/TaskCard.tsxsrc/features/tasks/TaskForm.tsxsrc/features/tasks/TaskList.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/layout/NotificationsBell.tsx
- src/features/tasks/DeleteTaskDialog.tsx
- src/features/tasks/TaskList.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/tasks/TaskForm.tsx`:
- Around line 122-125: Update the position validation in the canSubmit flow
using positionNumber so values below the declared minimum are rejected; require
positionNumber to be at least 1 while preserving the existing showPosition and
finite-value checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebe4f584-e650-4125-9f01-6e243004f333
📒 Files selected for processing (2)
src/features/tasks/EditTaskModal.tsxsrc/features/tasks/TaskForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/tasks/EditTaskModal.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes