Search and filter tasks through url search params - #37
Merged
Conversation
The header search debounces into ?q= and five filter chips (status, estimate, tags, due date, owner) write their own params, so filtered views are shareable, survive reloads, and never remount the page — search params sit outside the pathname the error boundary keys on. filterInputFromParams is the one validated boundary from URL strings to the typed FilterTaskInput; unknown values are dropped. Filters combine freely and an empty-results component with a clear-filters path renders when nothing matches. MenuPanel graduates from TaskForm to a shared component so the filter chips reuse the app's menu anatomy. Probing the component so the filter chips reuse the app's menu anatomy. Probing the live API first shaped the implementation and the README notes: name matching is a case-sensitive substring, dueDate matches by exact timestamp (satisfied by this app's noon-UTC convention), and ownerId is accepted but ignored server-side — sent as required, documented as observed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe pull request adds URL-backed search and filtering for task pages. It adds filter controls, validates filter parameters, updates task views and empty states, loads owner options, and updates router tests, fixtures, and documentation. ChangesTask filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Header
participant FilterBar
participant URL
participant Dashboard
participant TasksView
User->>Header: Enter search text
Header->>URL: Replace q after debounce
User->>FilterBar: Select task filters
FilterBar->>URL: Replace filter parameters
URL->>Dashboard: Provide search parameters
Dashboard->>TasksView: Pass filter input and active state
TasksView-->>Dashboard: Render filtered or empty task state
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: 3
🧹 Nitpick comments (4)
src/features/tasks/FilterBar.tsx (2)
249-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth branches of the ternary return the same string.
owner !== null ? 'Owner' : 'Owner'always evaluates to'Owner'. Remove the condition.♻️ Proposed refactor
- {ownerName ?? (owner !== null ? 'Owner' : 'Owner')} + {ownerName ?? 'Owner'}🤖 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/FilterBar.tsx` at line 249, In the FilterBar owner label expression, remove the redundant owner !== null ternary and use the constant 'Owner' fallback directly after ownerName.
110-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aria-haspopup="listbox"does not match the rendered markup.
MenuPanelrenders adivthat containsbuttonelements. No element hasrole="listbox"orrole="option". A screen reader announces a listbox and then finds none. The same mismatch repeats on the estimate, tags, and owner triggers.Use
aria-haspopup="menu"and give the panelrole="menu"withrole="menuitem"children, orrole="menuitemcheckbox"for the tag toggles.🤖 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/FilterBar.tsx` around lines 110 - 122, Update the status, estimate, tags, and owner filter triggers from aria-haspopup="listbox" to aria-haspopup="menu", and update MenuPanel plus its button children to use role="menu" and role="menuitem"; use role="menuitemcheckbox" for tag toggle items while preserving their existing behavior.src/features/tasks/filter-params.ts (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a type guard over the
as TaskTag[]cast.
FilterBar.tsxlines 57-59 already uses a predicate for the same parsing. A shared guard removes the cast and keeps both call sites consistent.♻️ Proposed refactor
const tags = params .get('tags') ?.split(',') - .filter((tag) => TAGS.has(tag)) as TaskTag[] | undefined + .filter((tag): tag is TaskTag => TAGS.has(tag))🤖 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/filter-params.ts` around lines 19 - 23, Update the tags parsing in filter-params.ts to replace the as TaskTag[] cast with a type-guard predicate, matching the existing FilterBar.tsx approach. Prefer reusing a shared guard if one exists, and preserve the current filtering and input.tags assignment behavior.src/app/router.test.tsx (1)
24-28: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake both router mocks enforce the GraphQL request contract.
Handle
TasksDocumentexplicitly, assert URL-derived variables in a filter test, and throw for unknown documents in both callbacks. The current fallback returns a task payload for any document and ignores variables.🤖 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 24 - 28, Update both router request mock callbacks to handle TasksDocument explicitly, validate the URL-derived variables in the filter test, and throw for any unrecognized document instead of returning a task payload. Preserve the existing profile and users responses while enforcing the GraphQL request contract in each mock.
🤖 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 `@README.md`:
- Line 116: Resolve the owner-filter behavior described by filterInputFromParams
and TasksView: either locally filter the fetched tasks by ownerId before
rendering, or remove/disable the owner filter and update its documented support
state so it is not presented as functional. Ensure selecting an owner no longer
leaves the task list unchanged while the server ignores ownerId.
In `@src/components/layout/Header.tsx`:
- Around line 13-32: Update the Header search state synchronization around
useSearchParams so external changes to the URL’s q parameter update the local
query value, including removals and browser back/forward navigation. Preserve
the existing debounced setSearchParams behavior for user input while preventing
the synchronization effect from rewriting externally applied values.
In `@src/features/tasks/filter-params.ts`:
- Around line 24-25: Create and export a shared parseDueParam helper in
filter-params.ts that validates both DAY_PATTERN and the calendar round-trip,
then use its result when assigning input.dueDate. In
src/features/tasks/filter-params.ts lines 24-25, replace direct validation with
the helper; in src/features/tasks/FilterBar.tsx lines 23-27, import and apply
the same helper to the due value read at line 60, treating invalid values as no
active due filter for both the chip label and active state.
---
Nitpick comments:
In `@src/app/router.test.tsx`:
- Around line 24-28: Update both router request mock callbacks to handle
TasksDocument explicitly, validate the URL-derived variables in the filter test,
and throw for any unrecognized document instead of returning a task payload.
Preserve the existing profile and users responses while enforcing the GraphQL
request contract in each mock.
In `@src/features/tasks/filter-params.ts`:
- Around line 19-23: Update the tags parsing in filter-params.ts to replace the
as TaskTag[] cast with a type-guard predicate, matching the existing
FilterBar.tsx approach. Prefer reusing a shared guard if one exists, and
preserve the current filtering and input.tags assignment behavior.
In `@src/features/tasks/FilterBar.tsx`:
- Line 249: In the FilterBar owner label expression, remove the redundant owner
!== null ternary and use the constant 'Owner' fallback directly after ownerName.
- Around line 110-122: Update the status, estimate, tags, and owner filter
triggers from aria-haspopup="listbox" to aria-haspopup="menu", and update
MenuPanel plus its button children to use role="menu" and role="menuitem"; use
role="menuitemcheckbox" for tag toggle items while preserving their existing
behavior.
🪄 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: a8f4e1ce-6f5a-4612-a07b-98892f821b8b
📒 Files selected for processing (11)
README.mdsrc/app/router.test.tsxsrc/components/layout/Header.tsxsrc/features/tasks/Dashboard.tsxsrc/features/tasks/FilterBar.tsxsrc/features/tasks/MenuPanel.tsxsrc/features/tasks/MyTask.tsxsrc/features/tasks/TaskForm.tsxsrc/features/tasks/TasksView.tsxsrc/features/tasks/filter-params.tssrc/test/fixtures.ts
The server ignores ownerId, so the owner chip changed nothing — a placebo control. The tasks query now selects creator id and TasksView filters locally when an owner is set, while the param is still sent as the requirement demands. The header search gains a last-pushed ref so external URL changes (clear filters, back/forward) sync into the input instead of being re-pushed by the stale debounce. parseDueParam becomes the single validated accessor for the due param, checking shape and calendar round-trip so junk or rollover dates mean no filter rather than an Invalid Date chip. Removes the stale unchecked search-and-filter checklist line.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
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
Documentation