Add saved views and productivity shortcuts#585
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-Review FindingsItems reviewed
FindingsNo blockers found. Minor observations below:
VerdictShip-ready. The |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Saved Views' feature, enabling users to create and manage reusable filters for cards across all boards through a new Pinia store, updated routing, and a dedicated UI component. Feedback focuses on improving the robustness of the implementation, specifically by addressing potential timezone mismatches in date comparisons, strengthening the validation of data restored from local storage, ensuring static timestamps for default views, and investigating a possible logic error in the overdue card filtering.
| const yesterday = new Date() | ||
| yesterday.setDate(yesterday.getDate() - 1) | ||
| const card = createMockCard({ dueDate: yesterday.toISOString() }) | ||
| const filter = createBaseFilter({ dueDateFilter: 'overdue' }) | ||
| expect(cardMatchesSavedViewFilter(card, filter)).toBe(true) | ||
| }) |
There was a problem hiding this comment.
The test case for should match overdue cards is currently passing, but it appears to be masking a bug in the cardMatchesSavedViewFilter function's 'overdue' logic. Given the current implementation of cardMatchesSavedViewFilter (which incorrectly identifies overdue cards), this test should actually fail. This indicates the test is not correctly asserting the intended behavior for overdue cards.
| const now = new Date() | ||
| const today = new Date(now.getFullYear(), now.getMonth(), now.getDate()) | ||
| const weekFromNow = new Date(today) | ||
| weekFromNow.setDate(weekFromNow.getDate() + 7) |
There was a problem hiding this comment.
The date comparisons for dueDateFilter (e.g., today, weekFromNow) are performed using new Date() which defaults to the client's local timezone. However, card.dueDate is an ISO string, which typically represents UTC. Comparing a local date object with a UTC date object (after parsing card.dueDate into a local Date object) can lead to incorrect filtering results, especially for cards due around midnight or across different timezones. It's best practice to normalize all dates to UTC before comparison to ensure consistent and accurate filtering regardless of the client's timezone.
| .filter( | ||
| (v: unknown): v is SavedView => | ||
| typeof v === 'object' && | ||
| v !== null && | ||
| typeof (v as SavedView).id === 'string' && | ||
| typeof (v as SavedView).name === 'string' && | ||
| typeof (v as SavedView).filter === 'object', | ||
| ) |
There was a problem hiding this comment.
The type validation for SavedView objects restored from localStorage is incomplete. While id, name, and filter (as an object) are checked, the specific properties within filter (e.g., searchText, labelNames, dueDateFilter, showBlockedOnly) are not validated. If localStorage contains malformed SavedViewFilter data, this could lead to runtime errors when the application attempts to access these properties. Consider adding more granular validation for the filter object's properties to ensure data integrity.
| // ── Default starter views ── | ||
|
|
||
| function createDefaultViews(): SavedView[] { | ||
| const now = new Date().toISOString() |
There was a problem hiding this comment.
The createdAt timestamp for default views is generated dynamically using new Date().toISOString(). This means that if the application is reloaded or the store is re-initialized, these 'default' views will have a new createdAt value. For built-in, immutable views, createdAt should ideally be a fixed, static value or omitted if it's only relevant for user-created views. Using a dynamic timestamp for static data can lead to inconsistencies or unexpected behavior if createdAt is used for sorting or other logic where a fixed point in time is expected for defaults.
- Normalize all date comparisons to UTC date-only (toUTCDateOnly helper) to prevent timezone mismatches between local Date and ISO/UTC dueDate - Use static DEFAULT_VIEW_CREATED_AT for built-in views instead of new Date().toISOString() which changes on every reload - Add granular localStorage filter validation via normalizeFilter(), applying safe defaults for missing/invalid properties - Fix overdue filter to reject cards with no dueDate (null guard) - Add new tests: stable timestamps, partial filter restore, missing icon restore, invalid dueDateFilter restore, today-not-overdue edge
- Add explicit null check in localStorage restore filter validation (typeof null === 'object' in JS, so the previous check let null through) - Document that overdue filter does not exclude completed cards since the Card type has no completion status field; callers should pre-filter by column if needed
…imitation - Test that entries with null filter are rejected on localStorage restore - Test that non-string labelNames fall back to default empty array - Test that non-boolean showBlockedOnly falls back to default false - Test that overdue filter matches by date alone regardless of columnId (documents intentional limitation: Card has no completion status)
Gemini Code Review Findings - Fixes AppliedAddressed all four findings from the Gemini code review: 1. CRITICAL: Overdue test / filter logic (Finding 1)
2. HIGH: Timezone mismatches (Finding 2)
3. HIGH: Incomplete localStorage validation (Finding 3)
4. MEDIUM: Static timestamps for default views (Finding 4)
Verification
|
1 participant
Summary
savedViewStore) with localStorage persistence for local-first behaviorSavedViewsViewcomponent with view picker, custom view creation form, and cross-board filtered card results grouped by board/workspace/views,/workspace/views/:viewId) and sidebar navigationCloses #333
Test plan
npm run typecheckpassesnpm run buildpasses