Persist todos in localStorage + minimal storage utils (STA-2) #7
Conversation
…d improved caching - Refactor CI workflow to use composite bun-install action (lines 26-41) - Refactor PR quality checks workflow to use composite bun-install action (lines 36-51) - Improve composite action cache key logic by deriving internally and removing guard - Optimize CI Turbo cache key with branch name and lock file hash for better reuse - Fine-tune PR quality checks Turbo cache key using lock file hash instead of commit SHA - Include workflow files in PR quality checks path filter to validate CI changes This reduces code duplication, centralizes maintenance, and improves cache hit rates while ensuring workflow changes are properly validated.
- Updated bun.lock to match package.json dependencies - Resolves frozen lockfile error in CI workflow
- Replace export * with named exports in utils and ui packages - Fix unused variable in create-todo.tsx by prefixing with underscore - Replace any type with proper Todo type in create-todo.tsx - All critical linting errors resolved, only warnings remain
- Remove 'fixed:' from '3xl:fixed:bg-none' -> '3xl:bg-none' - Remove 'fixed:' from '3xl:fixed:max-w-[...]' -> '3xl:max-w-[...]' - Resolves Tailwind build error: 'fixed variant does not exist' - Build now completes successfully
- Add required 'name' prop to FormError components (3 instances) - Remove invalid 'multiline' and 'rows' props from TextField component - All components now match type definitions from @lambdacurry/forms package - todo-app typecheck now passes successfully
- Add individual tsconfig.json files for packages/utils and packages/ui - Remove apps/**/* from root tsconfig.json include to prevent cross-package type conflicts - Each package now only typechecks its own files with proper path mappings - Resolves module resolution errors for ~/components/* imports - All 3 packages now pass typecheck successfully
- Added comprehensive test suites for utils and ui packages - Created vitest configs for proper test environment setup - Fixed FormError components to use explicit name prop for TypeScript compatibility - Tests cover cn utility function, type definitions, and Button component - All new tests pass successfully Co-authored-by: Jake Ruesink <[email protected]>
…848 and resolve conflicts by adopting new Context-based todo store, wrapping app with TodoProvider, and cleaning up removed Zustand store and tests\n\n- Remove legacy Zustand store and tests in favor of Context reducer store from main\n- Fix root to use TodoProvider and import types consistently\n- Align home route with context store hooks\n- Keep README and package updates from both branches where relevant\n\nCo-authored-by: Jake Ruesink <[email protected]>
… import\n\nCo-authored-by: Jake Ruesink <[email protected]>
…mpty function blocks in utils types.test with void expressions\n\nCo-authored-by: Jake Ruesink <[email protected]>
…o tests\n- Clean unused imports and types\n\nCo-authored-by: Jake Ruesink <[email protected]>
…n\nCo-authored-by: Jake Ruesink <[email protected]>
…it returns\n\nCo-authored-by: Jake Ruesink <[email protected]>
…rror body)\n\nCo-authored-by: Jake Ruesink <[email protected]>
… Add vitest.config with jsdom + setupFiles\n- Add setup to import @testing-library/jest-dom/vitest\n- Wire test:ci to use local config\n\nCo-authored-by: Jake Ruesink <[email protected]>
…o-authored-by: Jake Ruesink <[email protected]>
…ef requirement\n\nCo-authored-by: Jake Ruesink <[email protected]>
…to satisfy useHref in remix-hook-form\n\nCo-authored-by: Jake Ruesink <[email protected]>
…vider hydration Co-authored-by: Jake Ruesink <[email protected]>
STA-2 Persist todos in localStorage with minimal utilities (no backend)
Requested by Jake Ruesink Summary Why
Architecture & Integration Plan
Data Flow
Acceptance Criteria
Non-Goals (Out of Scope)
Implementation Steps
Files to Change / Add
Notes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis set of changes introduces robust localStorage persistence for the todo list and filter state in the todo app. It adds a new utility module for safe storage access, modifies the todo context to hydrate and persist state using this utility, and comprehensively tests the persistence logic and storage utilities. No changes are made to the public API of the todo context. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant TodoProvider
participant StorageUtil
Note over TodoProvider: On mount (client-side)
TodoProvider->>StorageUtil: loadFromStorage(key, fallback, validate)
StorageUtil-->>TodoProvider: persistedState or fallback
TodoProvider->>TodoProvider: dispatch({ type: 'HYDRATE', payload: persistedState })
Note over TodoProvider: On todos/filter change (after hydration)
TodoProvider->>StorageUtil: saveToStorage(key, { todos, filter })
StorageUtil-->>TodoProvider: (void)
UI->>TodoProvider: dispatch(action)
TodoProvider->>TodoProvider: update state via reducer
TodoProvider->>StorageUtil: saveToStorage(key, { todos, filter })
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope functional changes detected. All substantive changes are directly related to the objectives of localStorage persistence for todos and filter state as described in STA-2. Possibly related issues
Possibly related PRs
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🔍 Check Suite Status for PR #7
|
…ter context for AddTodo tests\n\nCo-authored-by: Jake Ruesink <[email protected]>
…mpleted test deterministic\n\nCo-authored-by: Jake Ruesink <[email protected]>
…-only usage in tests without data router\n\nCo-authored-by: Jake Ruesink <[email protected]>
… client submit via button click\n\nCo-authored-by: Jake Ruesink <[email protected]>
…olve conflicts; verify bun lint/typecheck/test\n\nCo-authored-by: Jake Ruesink <[email protected]>
…t regex literal in tests to satisfy useTopLevelRegex\n- Replace namespace imports; use named imports where required\n- Update Radix Checkbox imports to named to satisfy noNamespaceImport\n- Remove unused types and fix any types in create-todo\n- Suppress exhaustive-deps where appropriate on storage effect\n- Add explicit button types in tests to satisfy a11y rule\n\nCo-authored-by: Jake Ruesink <[email protected]>
… resolution during typecheck\n\nCo-authored-by: Jake Ruesink <[email protected]>
…hored-by: Jake Ruesink <[email protected]>
… todo completed after toggle)\n\nAlso adjusts CI scripts to pass with empty test suites in packages without tests.\n\nCo-authored-by: Jake Ruesink <[email protected]>
… for 3xl breakpoint\n\nReplaces and with separate utilities.\n\nCo-authored-by: Lambda Curry <[email protected]>
…irectory\n\nThis change ensures that the correct files are sourced from the package, improving compatibility and reducing potential errors in the build process.
…een) Co-authored-by: Jake Ruesink <[email protected]>
…' into codegen/sta-10-testing-add-storage-hydration-tests-utils-todo-context
…eset logic This update introduces stateful mocks for input fields and enhances the test setup for the AddTodo component. It includes a reset mechanism for the test state before each test, ensuring consistent behavior and accurate assertions. Additionally, the render function is modified to use a router context for better integration testing.
This update introduces a new `test:watch` command in the root and individual package configurations, allowing for easier test development. Additionally, the Vitest configuration file is added to support running tests across multiple projects in the monorepo. The test commands in the `todo-app`, `ui`, and `utils` packages are updated to use `vitest run` with options for handling empty test suites.
…' of github.com:lambda-curry/react-router-starter into codegen/sta-10-testing-add-storage-hydration-tests-utils-todo-context
…e handling and cleanup This update refines the todo-context tests by introducing a consistent localStorage polyfill for Node environments, ensuring reliable test execution. It also improves the cleanup logic in the tests to maintain a clean state between runs. Additionally, minor formatting adjustments are made for better readability.
This update simplifies the todo context reducer by consolidating conditional expressions for better readability. Additionally, it enhances the test suite by allowing real localStorage usage in tests, ensuring more accurate persistence behavior during testing. Minor formatting adjustments are also made for consistency.
This update refines the loadFromStorage function by consolidating its signatures for improved clarity and consistency. The changes enhance type safety while maintaining backward compatibility with optional validation logic.
…vert-seed-change-and-fix-tests-to-not-depend
…s storage mocking
…test file exclusions This change adds type definitions for Vitest and Testing Library to the todo-app's TypeScript configuration. Additionally, it removes the exclusion of test files from the build process, allowing for better integration with testing tools.
…torage-hydration-tests-utils-todo-context
…' of github.com:lambda-curry/react-router-starter into codegen/sta-7-major-add-runtime-validation-to-storage-loader
…ime-validation-to-storage-loader
…handler This update modifies the AddTodo component to use the form's onSubmit event for handling submissions, removing the previous client-only submission logic. Additionally, the TodoProvider now includes a hydration mechanism to load state from localStorage on the client side, ensuring proper state management and preventing SSR/CSR mismatches.
…orm submission event This commit adds a comment in the AddTodo component to explain the early form submission handling without a server. It also updates the test cases to use the form's onSubmit event instead of clicking the button, ensuring more accurate simulation of user interactions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (4)
apps/todo-app/app/components/todo-item.tsx (1)
61-67: Accessibility: add labels to icon-only buttonsIcon-only controls need accessible names. Add
aria-labelortitlefor SR users.-<Button size="icon" variant="ghost" type="submit"> +<Button size="icon" variant="ghost" type="submit" aria-label="Save"> <Check className="h-4 w-4" /> </Button> -<Button size="icon" variant="ghost" type="button" onClick={handleCancel}> +<Button size="icon" variant="ghost" type="button" onClick={handleCancel} aria-label="Cancel"> <X className="h-4 w-4" /> </Button> -<Button size="icon" variant="ghost" onClick={handleEdit} className="h-8 w-8"> +<Button size="icon" variant="ghost" onClick={handleEdit} className="h-8 w-8" aria-label="Edit"> <Edit2 className="h-4 w-4" /> </Button> -<Button +<Button size="icon" variant="ghost" onClick={() => onDelete(todo.id)} - className="h-8 w-8 text-destructive hover:text-destructive" + className="h-8 w-8 text-destructive hover:text-destructive" + aria-label="Delete" >Also applies to: 76-86
apps/todo-app/app/routes/create-todo.tsx (1)
110-115: Fix: Side-effect during render; move navigation to useEffectScheduling
setTimeoutin render can fire multiple times and is an anti-pattern. UseuseEffectwith cleanup.- // Handle successful submission - if (fetcher.data?.success) { - setTimeout(() => { - navigate('/'); - }, 2000); - } + // Handle successful submission + useEffect(() => { + if (!fetcher.data?.success) return; + const tid = setTimeout(() => navigate('/'), 2000); + return () => clearTimeout(tid); + }, [fetcher.data?.success, navigate]);Also add:
+ import { useEffect } from 'react';apps/todo-app/app/lib/__tests__/todo-context.test.tsx (1)
8-12: Avoid wholesale override of the globalcryptoobjectReplacing the entire
cryptoimplementation risks breaking APIs other tests might rely on (e.g.subtle). Override just the needed method instead and restore it afterwards.-Object.defineProperty(global, 'crypto', { - value: { randomUUID: () => 'test-uuid' } -}); +const originalUUID = global.crypto?.randomUUID; +Object.defineProperty(global.crypto, 'randomUUID', { + value: () => 'test-uuid', + configurable: true +}); +// …in afterAll +if (originalUUID) Object.defineProperty(global.crypto, 'randomUUID', { value: originalUUID });apps/todo-app/app/lib/todo-context.tsx (1)
56-66: Guard against adding empty todos
ADD_TODOtrims input but still creates a todo even when the result is an empty string. This produces blank entries and wastes a UUID.case 'ADD_TODO': { - const newTodo: Todo = { + const text = action.payload.trim(); + if (!text) return state; // ignore empty submissions + const newTodo: Todo = { - text: action.payload.trim(), + text,
🧹 Nitpick comments (25)
packages/utils/tsconfig.json (1)
11-17: Avoid excluding tests from the main tsconfig (or split configs)Excluding tests here can hinder IDE type-checking and Vitest TS transforms if this tsconfig is picked up. Prefer:
- Keep tests included in tsconfig.json, and
- Create a tsconfig.build.json that excludes tests for build-only tooling.
Option A (simple): include tests by removing the patterns:
"exclude": [ "node_modules", "dist", "build", - "**/*.test.ts", - "**/*.test.tsx" ]Option B (recommended): introduce tsconfig.build.json with the current exclude list and point build tooling to it, while leaving tsconfig.json test-friendly. I can draft that if useful.
apps/todo-app/tsconfig.json (1)
25-30: Exclude coverage folder as well (optional).If you use Vitest coverage, consider excluding the coverage directory to keep TS fast and clean.
"exclude": [ "node_modules", "build", - "dist" + "dist", + "coverage" ]packages/ui/tsconfig.json (1)
11-17: Exclude patterns are fine; consider test type-check strategyExcluding tests from this tsconfig is reasonable for package builds. If you want tests type-checked, consider a separate tsconfig (e.g., tsconfig.test.json) or rely on Vitest’s TS config to catch type errors in tests.
packages/utils/src/cn.test.ts (1)
1-38: Solid coverage; consider a few more edge casesOptional additions to harden behavior:
- Object syntax: cn('a', { b: true, c: false }) => 'a b'
- Nested arrays: cn(['a', ['b', false && 'x']], 'c') => 'a b c'
- Complex Tailwind merges: cn('mt-2 mt-4', 'px-2 px-3', 'hover:text-red-500 text-blue-500') => 'mt-4 px-3 text-blue-500 hover:text-red-500' (verify expected order per tw-merge rules)
packages/utils/package.json (1)
19-22: Test scripts look good; consider a strict CI modeAllowing CI to pass with no tests can mask misconfigurations. Consider adding a strict CI script that fails when no tests are found.
Suggested addition:
"scripts": { "lint": "biome lint .", "format": "biome format --write .", "typecheck": "tsc --noEmit", "test": "vitest run --passWithNoTests", "test:watch": "vitest", - "test:ci": "vitest run --passWithNoTests" + "test:ci": "vitest run --passWithNoTests", + "test:strict": "vitest run" },.github/workflows/ci.yml (1)
33-37: Improve Turbo cache hit rate by removing commit SHA from the cache keyIncluding github.sha makes each cache key unique per commit, reducing exact hits. Prefer a key based on branch and lockfile hash; keep restore-keys as-is.
- key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}-${{ github.sha }} + key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }} restore-keys: | ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}- ${{ runner.os }}-turbo-${{ github.ref_name }}- ${{ runner.os }}-turbo-packages/utils/src/types.test.ts (2)
21-27: Strengthen type-only checks with compile-time assertionsRuntime checks can’t catch invalid union members. Add compile-time guards via expectTypeOf and ts-expect-error.
-import { describe, it, expect } from 'vitest'; +import { describe, it, expect, expectTypeOf } from 'vitest'; @@ it('should accept valid TodoFilter values', () => { const filters: TodoFilter[] = ['all', 'active', 'completed']; filters.forEach(filter => { expect(['all', 'active', 'completed']).toContain(filter); }); + + // Compile-time negative test: invalid value must not assign + // @ts-expect-error - 'invalid' is not a TodoFilter + const badFilter: TodoFilter = 'invalid'; + + // Compile-time shape test + expectTypeOf<TodoFilter>().toEqualTypeOf<'all' | 'active' | 'completed'>(); });
31-40: Prefer satisfies for structural conformance without changing the variable’s typeUsing satisfies checks the shape while keeping the inferred types intact and avoids accidental widening.
- const mockStore: TodoStore = { + const mockStore = { todos: [], filter: 'all', addTodo: (_text: string) => { return; }, toggleTodo: (_id: string) => { return; }, deleteTodo: (_id: string) => { return; }, updateTodo: (_id: string, _text: string) => { return; }, setFilter: (_filter: TodoFilter) => { return; }, clearCompleted: () => { return; } - }; + } satisfies TodoStore;apps/todo-app/test/setup.ts (1)
8-10: Restore mocks after each test to improve isolationAdd vi.restoreAllMocks() to ensure mocked timers/spies don’t leak across tests.
-import { afterEach } from 'vitest'; +import { afterEach, vi } from 'vitest'; @@ afterEach(() => { cleanup(); + vi.restoreAllMocks(); });packages/ui/src/components/ui/button.test.tsx (2)
12-17: Consider asserting class application for variant (more than existence)toBeInTheDocument is fine; add a class assertion to validate styling contract.
const button = screen.getByRole('button', { name: 'Delete' }); expect(button).toBeInTheDocument(); - expect(button).toHaveClass('bg-destructive'); + expect(button).toHaveClass('bg-destructive'); + expect(button).toHaveClass('text-destructive-foreground');
32-41: Assert classes propagate when using asChildEnsure the Slot-based asChild path applies the button classes to the child element.
const link = screen.getByRole('link', { name: 'Link button' }); expect(link).toBeInTheDocument(); expect(link).toHaveAttribute('href', '/test'); + // Verify styling classes are applied via Slot + expect(link.className).toContain('bg-primary'); + expect(link.className).toContain('text-primary-foreground');apps/todo-app/app/components/todo-item.tsx (1)
53-54: Checkbox a11y: provide an accessible nameConsider an
aria-labelor associate a visible label to convey purpose, e.g., “Mark todo complete”.-<Checkbox checked={todo.completed} onCheckedChange={() => onToggle(todo.id)} className="flex-shrink-0" /> +<Checkbox + checked={todo.completed} + onCheckedChange={() => onToggle(todo.id)} + className="flex-shrink-0" + aria-label="Toggle completed" />packages/ui/src/components/ui/button.tsx (1)
38-43: Optional: improve polymorphic typing for asChild usageWhen
asChildis true, the component renders non-button elements, butButtonPropsremainbutton-centric. If you want stricter types, consider a polymorphic pattern so refs/props align with the rendered element. If you’re following the standard shadcn pattern, this is fine to keep.Example direction (not a drop-in diff):
import type { ElementRef, ComponentPropsWithoutRef } from 'react'; // type PolymorphicButtonProps<T extends ElementType = 'button'> = { asChild?: boolean } & ComponentPropsWithoutRef<T> & VariantProps<typeof buttonVariants>; // const Button = forwardRef<ElementRef<T>, PolymorphicButtonProps<T>>(...)apps/todo-app/app/routes/create-todo.tsx (1)
64-64: Nit: Consider logging the caught error in devRenaming to
_erroravoids unused var warnings. Optionally log in non-production to aid debugging.-} catch (_error) { +} catch (_error) { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.error('create-todo action failed:', _error); + }packages/utils/src/__tests__/storage.test.ts (2)
52-56: Nit: Rename test to reflect test-env guardName suggests SSR, but this specifically asserts test-env short-circuiting. Consider renaming to "test env disables storage".
97-111: Missing: Date revival behaviorPersistence layer claims date revival on hydration. Add a test storing an ISO string and assert
instanceof DateafterloadFromStorage.Example:
it('revives ISO date strings to Date instances', () => { process.env.NODE_ENV = 'development'; ensureWindowWithLocalStorage(); const value = { createdAt: new Date().toISOString() }; window.localStorage.setItem(KEY, JSON.stringify(value)); const loaded = loadFromStorage(KEY, { createdAt: '' }); expect(loaded).toEqual({ createdAt: expect.any(Date) }); expect(loaded.createdAt instanceof Date).toBe(true); });packages/ui/src/components/ui/checkbox.tsx (1)
23-23: Preserve Radix displayName for easier debuggingHard-coding
Checkbox.displayName = 'Checkbox'loses the more informative name provided byCheckboxRoot.displayName, which can help identify the underlying Radix primitive in React DevTools. Keeping the original value costs nothing and aids diagnostics.-Checkbox.displayName = 'Checkbox'; +Checkbox.displayName = CheckboxRoot.displayName ?? 'Checkbox';apps/todo-app/app/lib/__tests__/todo-context.test.tsx (1)
88-91: Single-source the storage key constant
STORAGE_KEYis declared here and again inside a later test block. Duplicating literals makes refactors easy to miss—prefer one shared constant.apps/todo-app/app/components/__tests__/add-todo.test.tsx (7)
22-36: Make TextField mock more future-proof by forwarding props and loosening required fieldsAvoid over-constraining the mock. Forward unknown props and make non-essential props optional to better mirror the real component and reduce future breakage.
-interface TextFieldProps { - name: string; - placeholder: string; - className: string; -} +interface TextFieldProps { + name: string; + placeholder?: string; + className?: string; + [key: string]: unknown; +} - TextField: ({ name, placeholder, className }: TextFieldProps) => ( + TextField: ({ name, placeholder, className, ...rest }: TextFieldProps) => ( <input name={name} placeholder={placeholder} className={className} type="text" value={testInputValue} onChange={e => { testInputValue = e.target.value; }} + {...rest} /> ),
38-50: Align Button mock prop types with real component; default type to 'button'Make onClick optional and default the native button type to avoid accidental submits in other tests.
-interface ButtonProps { - children: ReactNode; - onClick: () => void; - type: 'button' | 'submit' | 'reset'; -} +interface ButtonProps { + children?: ReactNode; + onClick?: () => void; + type?: 'button' | 'submit' | 'reset'; +} - Button: ({ children, onClick, type }: ButtonProps) => ( - <button type={type} onClick={onClick}> + Button: ({ children, onClick, type }: ButtonProps) => ( + <button type={type ?? 'button'} onClick={onClick}> {children} </button> )
69-76: reset() comment vs behavior mismatch; dispatch an input event or drop the commentCurrently no custom event is dispatched. Either remove the comment or dispatch an event to better reflect a UI update.
- reset: vi.fn(() => { + reset: vi.fn(() => { testInputValue = ''; - // Force re-render by dispatching a custom event const inputs = document.querySelectorAll('input[name="text"]'); inputs.forEach(input => { (input as HTMLInputElement).value = ''; + // Notify listeners that value changed + input.dispatchEvent(new Event('input', { bubbles: true })); }); }),
113-116: Consider extracting renderWithRouter to a shared test utilThis helper will likely be reused. Moving it to a central test utils module reduces duplication and standardizes router-wrapping across tests.
Happy to draft a small
test-utils.tsxwith this helper and update imports.
143-145: Prefer user-event for interactions over fireEventuser-event simulates user behavior more closely (e.g., typing and submitting), improving test fidelity.
Example:
import userEvent from '@testing-library/user-event'; await userEvent.type(input, 'New todo'); await userEvent.click(button); // relies on type="submit" to submit the formAlso applies to: 157-159, 169-170, 183-184
139-147: Add a call count assertion to guard against double-submitsTighten expectations to prevent regressions that might trigger multiple submissions.
expect(mockOnAdd).toHaveBeenCalledWith('New todo'); +expect(mockOnAdd).toHaveBeenCalledTimes(1);
167-170: Add a whitespace-only negative caseYou cover empty string; add a whitespace-only case to protect trimming behavior from regressions.
it('does not call onAdd with empty text', () => { const mockOnAdd = vi.fn(); renderWithRouter(<AddTodo onAdd={mockOnAdd} />); const button = screen.getByRole('button', { name: ADD_REGEX }); const form = button.closest('form') as HTMLFormElement; fireEvent.submit(form); expect(mockOnAdd).not.toHaveBeenCalled(); }); + + it('does not call onAdd with whitespace-only text', () => { + const mockOnAdd = vi.fn(); + renderWithRouter(<AddTodo onAdd={mockOnAdd} />); + const input = screen.getByPlaceholderText('Add a new todo...'); + const button = screen.getByRole('button', { name: ADD_REGEX }); + const form = button.closest('form') as HTMLFormElement; + fireEvent.change(input, { target: { value: ' ' } }); + fireEvent.submit(form); + expect(mockOnAdd).not.toHaveBeenCalled(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/composite/bun-install/action.yml(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/pr-quality-checks.yml(2 hunks)apps/todo-app/app/components/__tests__/add-todo.test.tsx(1 hunks)apps/todo-app/app/components/add-todo.tsx(2 hunks)apps/todo-app/app/components/todo-filters.tsx(1 hunks)apps/todo-app/app/components/todo-item.tsx(4 hunks)apps/todo-app/app/globals.css(1 hunks)apps/todo-app/app/lib/__tests__/todo-context.test.tsx(4 hunks)apps/todo-app/app/lib/todo-context.tsx(7 hunks)apps/todo-app/app/root.tsx(1 hunks)apps/todo-app/app/routes/create-todo.tsx(4 hunks)apps/todo-app/app/routes/home.tsx(3 hunks)apps/todo-app/package.json(1 hunks)apps/todo-app/react-router.config.ts(0 hunks)apps/todo-app/test/setup.ts(1 hunks)apps/todo-app/tsconfig.json(2 hunks)apps/todo-app/vite.config.ts(0 hunks)apps/todo-app/vitest.config.ts(0 hunks)biome.json(1 hunks)package.json(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/ui/button.test.tsx(1 hunks)packages/ui/src/components/ui/button.tsx(2 hunks)packages/ui/src/components/ui/card.tsx(1 hunks)packages/ui/src/components/ui/checkbox.tsx(1 hunks)packages/ui/src/components/ui/input.tsx(1 hunks)packages/ui/src/index.ts(1 hunks)packages/ui/src/test/setup.ts(1 hunks)packages/ui/test/setup.ts(1 hunks)packages/ui/tsconfig.json(1 hunks)packages/ui/vitest.config.ts(1 hunks)packages/utils/package.json(2 hunks)packages/utils/src/__tests__/storage.test.ts(1 hunks)packages/utils/src/cn.test.ts(1 hunks)packages/utils/src/cn.ts(0 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/storage.ts(1 hunks)packages/utils/src/types.test.ts(1 hunks)packages/utils/src/types.ts(0 hunks)packages/utils/tsconfig.json(1 hunks)packages/utils/vitest.config.ts(1 hunks)tsconfig.json(0 hunks)turbo.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/todo-app/react-router.config.ts
- packages/utils/src/cn.ts
- apps/todo-app/vitest.config.ts
- apps/todo-app/vite.config.ts
- tsconfig.json
- packages/utils/src/types.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-23T12:46:51.780Z
Learnt from: CR
PR: lambda-curry/forms#0
File: .cursor/rules/form-component-patterns.mdc:0-0
Timestamp: 2025-07-23T12:46:51.780Z
Learning: Applies to packages/components/src/remix-hook-form/**/*.tsx : Use the required imports for Remix Hook Form integration, Zod, and base UI components in form-aware components
Applied to files:
apps/todo-app/app/routes/create-todo.tsx
📚 Learning: 2025-07-28T22:12:33.155Z
Learnt from: CR
PR: lambda-curry/email-proxy#0
File: .cursor/rules/forms2.mdc:0-0
Timestamp: 2025-07-28T22:12:33.155Z
Learning: Applies to **/*.tsx : Import form components from `lambdacurry/forms/remix-hook-form`
Applied to files:
apps/todo-app/app/routes/create-todo.tsx
🧬 Code Graph Analysis (3)
apps/todo-app/app/routes/home.tsx (3)
apps/todo-app/app/lib/todo-context.tsx (1)
useTodoStore(170-176)packages/ui/src/components/ui/card.tsx (1)
CardDescription(44-44)packages/ui/src/index.ts (1)
CardDescription(4-4)
packages/ui/src/components/ui/checkbox.tsx (1)
packages/utils/src/cn.ts (1)
cn(4-6)
apps/todo-app/app/components/__tests__/add-todo.test.tsx (2)
packages/ui/src/components/ui/button.tsx (1)
ButtonProps(32-36)apps/todo-app/app/components/add-todo.tsx (1)
AddTodo(18-45)
🔇 Additional comments (33)
apps/todo-app/app/root.tsx (1)
3-3: Nice import cleanupType-only import for MetaFunction keeps runtime bundle clean and avoids unused type import noise. LGTM.
biome.json (1)
28-28: Formatting-only change acknowledgedSingle-line globals array is fine; no behavioral impact. LGTM.
packages/utils/tsconfig.json (1)
4-7: Composite/noEmit flags are safe given no project referencesWe’ve confirmed that the root
tsconfig.jsonhas no"references"entry forpackages/utilsand no bundler configurations (tsup/rollup/vite) reference this package. With"composite": falseand"noEmit": true,tscwon’t emit output nor include this package in TypeScript project references.• If your build strategy relies on an external bundler (e.g., tsup, Rollup, Vite) to compile and emit artifacts for
packages/utils, no changes are needed.
• If you intend to consume this package via TS project references, enable"composite": trueand add it under"references"in the roottsconfig.json.apps/todo-app/tsconfig.json (1)
5-9: Types config looks good for Vitest + JSDOM.Explicitly including vitest/globals, jest-dom, and vite/client is appropriate for the app.
packages/utils/src/storage.ts (1)
5-19: Test guard and SSR safety look solid.Disabling storage in tests by default with an opt-in flag and guarding SSR/private mode errors is clean and robust. Good work.
apps/todo-app/app/components/todo-filters.tsx (1)
39-39: Whitespace cleanup only.No functional changes. LGTM.
packages/ui/src/test/setup.ts (1)
1-3: Correct setup for jest-dom with Vitest.Importing @testing-library/jest-dom/vitest is the recommended approach and provides matcher types. LGTM.
apps/todo-app/app/globals.css (1)
3-3: Scoping@sourcetoforms/distis a good optimizationReduces scan time and avoids picking non-distributed sources. Path looks correct relative to this file in a monorepo layout.
packages/ui/vitest.config.ts (1)
3-10: Solid Vitest config for UI package
jsdom+setupFiles+globals+css: falseis a good default for component tests. No issues spotted.apps/todo-app/package.json (1)
20-21: Switch tovitest runand add watch modeGood change for CI determinism and local DX.
packages/ui/package.json (1)
19-20: Allow tests to pass when none are presentUsing
--passWithNoTestsprevents noisy CI failures on packages without tests. LGTM.apps/todo-app/app/routes/home.tsx (3)
19-19: Destructuring formatting change is fineNo behavioral impact. Hook usage remains the same and safe.
48-48: Inline CardDescription is finePurely stylistic; no change in semantics.
94-94: Inline empty-state text is fineNo logic change; renders identically.
packages/ui/src/components/ui/input.tsx (1)
1-1: Direct React named imports and typed forwardRef look goodConsistent with modern import style; no functional changes. Component remains properly typed and ref-forwarding is intact.
Also applies to: 4-4, 6-6
.github/workflows/ci.yml (1)
26-27: Composite action correctly pins Bun version and caches dependenciesThe
bun-installcomposite already:
- Pins a default Bun version (
1.2.19) via itsbun-versioninput- Uses
oven-sh/setup-bun@v2with that pinned version- Caches the
~/.bundirectory usingactions/cache@v4keyed onbun.lockNo further changes needed here.
vitest.config.ts (1)
7-11: I wasn’t able to find any official mention of defineWorkspace in the Vitest 3.x docs—this feature appears only in the latest v4 release. Since you’re on v3.2.4, switching to defineWorkspace will break your tests. The current glob-based projects syntax is fully supported in 3.x, so you can safely keep it.Next steps:
- Keep the existing
test.projectsarray in your root vitest.config.ts.- Remove the suggestion to use
defineWorkspace, which isn’t available in v3.2.4.Likely an incorrect or invalid review comment.
packages/utils/src/types.test.ts (1)
1-19: LGTM: solid basic runtime checks for Todo typesThe runtime assertions are straightforward and helpful for guarding structural changes.
packages/ui/src/components/ui/button.test.tsx (1)
26-30: Edge case: disabled + asChild behaviorAnchors don’t support disabled. Add a test ensuring disabled isn’t silently forwarded to , and that aria-disabled is handled if needed.
Would you like me to add a test covering:
- renders aria-disabled on the child
- onClick is prevented when aria-disabled
I can draft it based on the underlying component API.
Also applies to: 32-41
.github/workflows/pr-quality-checks.yml (1)
37-39: Good consolidation into a composite actionSwitching to the composite
.github/composite/bun-installsimplifies setup and reduces duplication. Ensure the composite handles Bun install, cache, and install deterministically (including CI vs local nuances).turbo.json (1)
32-33: Nice: include vitest config in Turbo inputsAdding
vitest.config.*totestandtest:ciinputs improves cache correctness for test configuration changes.Also applies to: 43-44
packages/ui/src/components/ui/card.tsx (1)
1-8: Import/style refactor looks goodDirect
forwardRefandHTMLAttributesimports are consistent and reduce the React namespace dependency. Other components’ ref and prop types match the rendered elements.Also applies to: 11-17, 25-31, 32-36, 37-43
apps/todo-app/app/components/todo-item.tsx (1)
32-38: Nice: minimal submit handler with guardThe
onValidhandler avoids unnecessary updates and neatly exits edit mode. Clean and side-effect minimal.packages/ui/src/components/ui/button.tsx (1)
1-1: Good import/type refactorSwitching to named imports (
forwardRef,ButtonHTMLAttributes) keeps consistency with other UI components and reduces React namespace coupling.Also applies to: 33-34
apps/todo-app/app/routes/create-todo.tsx (2)
6-6: LGTM: Added Textarea importImport looks correct alongside other form components.
158-164: LGTM: Switched description to TextareaAppropriate replacement for multiline input; explicit rows prop is clear.
apps/todo-app/app/components/add-todo.tsx (2)
23-29: LGTM: Local submit handler and resetLocal handling and reset keep the UX snappy without server roundtrip.
42-42: LGTM: Explicit FormError targetSpecifying
name="_form"clarifies intent and improves consistency.packages/utils/src/__tests__/storage.test.ts (1)
66-79: LGTM: save/remove round-tripCovers write/read/remove flow in non-test env.
packages/ui/src/index.ts (1)
1-4: No missing UI exports detectedI scanned all imports of
@todo-starter/uiacross the repo (notably inapps/todo-app):
home.tsx,create-todo.tsximportCard, CardHeader, CardContent, CardTitle, CardDescriptiontodo-filters.tsximportsButtontodo-item.tsximportsCheckboxAll of these are explicitly exported in
packages/ui/src/index.ts. You can safely switch from wildcard to named exports without breaking consumers.packages/utils/src/index.ts (1)
1-4: LGTM: Explicit, focused utils exports + storage utilitiesClearer API surface and new storage utilities made available.
apps/todo-app/app/components/__tests__/add-todo.test.tsx (2)
87-105: handleSubmit dual-usage handling looks solidCovers both direct onSubmit binding and higher-order usage. Good guard on trimmed input.
174-186: Trimming behavior test reads wellValidating trimmed value is correct and complements the empty input test.
| with: | ||
| path: ~/.bun | ||
| key: ${{ inputs.cache-key }} | ||
| key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lock') }} |
There was a problem hiding this comment.
Cache key likely wrong: Bun’s lockfile is “bun.lockb”, not “bun.lock”.
Using bun.lock here will cause cache misses. Switch to bun.lockb (or hash both for safety).
- key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lock') }}
+ key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lockb') }}Optional: hash both
- key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lock') }}
+ key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lockb', '**/bun.lock') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lock') }} | |
| key: ${{ runner.os }}-deps-${{ hashFiles('**/bun.lockb') }} |
🤖 Prompt for AI Agents
In .github/composite/bun-install/action.yml at line 22, the cache key is using
'bun.lock' which is incorrect for Bun; it should use 'bun.lockb' instead. Update
the hashFiles argument to reference 'bun.lockb' to prevent cache misses.
Optionally, you can hash both 'bun.lock' and 'bun.lockb' files for safety by
including both patterns in the hashFiles function.
| key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}-${{ github.sha }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}- | ||
| ${{ runner.os }}-turbo-${{ github.ref_name }}- | ||
| ${{ runner.os }}-turbo- |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm bun.lockb presence across the monorepo for cache key effectiveness
Turbo cache keys now hash **/bun.lockb, but the workflow triggers still list bun.lock (Line 12). If the repo uses bun.lockb (Bun’s default), consider updating triggers and verify that lockfiles exist so cache keys are stable.
Run to verify presence and consistency:
🏁 Script executed:
#!/bin/bash
# List Bun lockfiles and their paths
fd -HI --regex 'bun\.lockb?$' | sort
# Show which workflows reference bun.lock vs bun.lockb
rg -n "bun\.lockb?" -A 2 -B 2 .github/workflowsLength of output: 1631
Use bun.lock for Turbo cache keys in workflows
We only have bun.lock in the repo (no bun.lockb), so the current hashFiles('**/bun.lockb') calls will never match anything and the cache key will always fall back to the less specific prefixes. Update both workflows to hash the actual lockfile instead:
• .github/workflows/pr-quality-checks.yml (lines 44–48)
• .github/workflows/ci.yml (lines 33–37)
Suggested diff for each:
- key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}-${{ github.sha }}
+ key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lock') }}-${{ github.sha }}
- restore-keys: |
- ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}-
+ restore-keys: |
+ ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lock') }}-
${{ runner.os }}-turbo-${{ github.ref_name }}-
${{ runner.os }}-turbo-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}-${{ github.sha }} | |
| restore-keys: | | |
| ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lockb') }}- | |
| ${{ runner.os }}-turbo-${{ github.ref_name }}- | |
| ${{ runner.os }}-turbo- | |
| key: ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lock') }}-${{ github.sha }} | |
| restore-keys: | | |
| ${{ runner.os }}-turbo-${{ github.ref_name }}-${{ hashFiles('**/bun.lock') }}- | |
| ${{ runner.os }}-turbo-${{ github.ref_name }}- | |
| ${{ runner.os }}-turbo- |
🤖 Prompt for AI Agents
In .github/workflows/pr-quality-checks.yml around lines 44 to 48, the cache key
uses hashFiles('**/bun.lockb'), but the repo only contains bun.lock, so this
hash will never match. Update the hashFiles argument to '**/bun.lock' in all
occurrences to correctly hash the existing lockfile and improve cache key
specificity.
| text: z.string().min(1, 'Todo text is required').trim() | ||
| }); |
There was a problem hiding this comment.
Fix validation order: trim before min
As written, whitespace-only input passes .min(1) then gets trimmed to empty. Apply .trim() first, then .min(1).
- text: z.string().min(1, 'Todo text is required').trim()
+ text: z.string().trim().min(1, 'Todo text is required')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text: z.string().min(1, 'Todo text is required').trim() | |
| }); | |
| text: z.string().trim().min(1, 'Todo text is required') | |
| }); |
🤖 Prompt for AI Agents
In apps/todo-app/app/components/add-todo.tsx around lines 9 to 10, the
validation chain applies .min(1) before .trim(), allowing whitespace-only input
to pass validation. To fix this, reorder the validation so that .trim() is
called first on the string, followed by .min(1) to ensure the trimmed input has
at least one character.
| todos: state.todos.map(todo => | ||
| todo.id === action.payload.id | ||
| ? { ...todo, text: action.payload.text.trim(), updatedAt: new Date() } | ||
| : todo | ||
| todo.id === action.payload.id ? { ...todo, text: action.payload.text.trim(), updatedAt: new Date() } : todo | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Likewise skip empty text on update
Updating a todo to an empty string leaves a meaningless item in the list.
- todo.id === action.payload.id ? { ...todo, text: action.payload.text.trim(), updatedAt: new Date() } : todo
+ todo.id === action.payload.id
+ ? {
+ ...todo,
+ text: action.payload.text.trim() || todo.text,
+ updatedAt: new Date()
+ }
+ : todo📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| todos: state.todos.map(todo => | |
| todo.id === action.payload.id | |
| ? { ...todo, text: action.payload.text.trim(), updatedAt: new Date() } | |
| : todo | |
| todo.id === action.payload.id ? { ...todo, text: action.payload.text.trim(), updatedAt: new Date() } : todo | |
| ) | |
| todos: state.todos.map(todo => | |
| todo.id === action.payload.id | |
| ? { | |
| ...todo, | |
| text: action.payload.text.trim() || todo.text, | |
| updatedAt: new Date() | |
| } | |
| : todo | |
| ) |
🤖 Prompt for AI Agents
In apps/todo-app/app/lib/todo-context.tsx around lines 83 to 85, the update
logic allows setting a todo's text to an empty string, resulting in meaningless
list items. Modify the update case to first check if the trimmed new text is
non-empty before applying the update; if the text is empty, skip the update and
return the original todo unchanged.
| todo?: { | ||
| id: string; | ||
| title: string; | ||
| description?: string; | ||
| priority: 'low' | 'medium' | 'high'; | ||
| dueDate?: string; | ||
| category: string; | ||
| isUrgent: boolean; | ||
| tags?: string; | ||
| completed: boolean; | ||
| createdAt: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid inline Todo shape; derive from shared type to prevent drift
Use the shared Todo type to keep this aligned with the app store/types. Adjust createdAt as a string if your route returns ISO timestamps.
- const fetcher = useFetcher<{
+ const fetcher = useFetcher<{
success?: boolean;
message?: string;
errors?: Record<string, { message: string }>;
- todo?: {
- id: string;
- title: string;
- description?: string;
- priority: 'low' | 'medium' | 'high';
- dueDate?: string;
- category: string;
- isUrgent: boolean;
- tags?: string;
- completed: boolean;
- createdAt: string;
- };
+ // If Todo.createdAt is Date in shared types, keep it string here:
+ // eslint-disable-next-line @typescript-eslint/consistent-type-definitions
+ todo?: Omit<Todo, 'createdAt'> & { createdAt: string };
}>();Add this import near the top:
import type { Todo } from '@todo-starter/utils';🤖 Prompt for AI Agents
In apps/todo-app/app/routes/create-todo.tsx around lines 79 to 90, replace the
inline Todo type definition with the shared Todo type imported from
'@todo-starter/utils'. Add the import statement for the Todo type near the top
of the file. Adjust the createdAt property type to string if the route returns
ISO timestamp strings, ensuring consistency with the shared type and preventing
type drift.
| function ensureWindowWithLocalStorage() { | ||
| if (typeof window === 'undefined') { | ||
| Object.defineProperty(globalThis, 'window', { | ||
| value: {} as unknown as Window & typeof globalThis, | ||
| configurable: true | ||
| }); | ||
| } | ||
| if (!('localStorage' in window)) { | ||
| const store = new Map<string, string>(); | ||
| Object.defineProperty(window, 'localStorage', { | ||
| value: { | ||
| getItem: (k: string) => store.get(k) ?? null, | ||
| setItem: (k: string, v: string) => { | ||
| store.set(k, v); | ||
| }, | ||
| removeItem: (k: string) => { | ||
| store.delete(k); | ||
| } | ||
| }, | ||
| configurable: true | ||
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add SSR-without-window test coverage
Currently ensureWindowWithLocalStorage always creates window, so we never test SSR path. Add a test where window is undefined to assert fallback behavior in non-test envs.
Example:
it('SSR guard: returns fallback when window is undefined in dev', () => {
process.env.NODE_ENV = 'development';
// simulate SSR
// eslint-disable-next-line @typescript-eslint/no-explicit-any
delete (globalThis as any).window;
const res = loadFromStorage(KEY, { x: 1 });
expect(res).toEqual({ x: 1 });
});🤖 Prompt for AI Agents
In packages/utils/src/__tests__/storage.test.ts around lines 10 to 33, add a new
test case that simulates the SSR environment by deleting globalThis.window and
setting NODE_ENV to development. This test should call loadFromStorage with a
key and default value, then assert that the fallback default value is returned,
ensuring coverage of the SSR path where window is undefined.
| @@ -0,0 +1,53 @@ | |||
| // Minimal localStorage helpers with safe JSON and SSR/test guards | |||
|
|
|||
| export type StorageLike = Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid DOM lib dependency by defining StorageLike inline.
Using Pick<Storage,...> ties the package to the DOM lib. Define a minimal interface to keep utils portable in non-DOM builds.
-export type StorageLike = Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>;
+export interface StorageLike {
+ getItem(key: string): string | null;
+ setItem(key: string, value: string): void;
+ removeItem(key: string): void;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type StorageLike = Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>; | |
| export interface StorageLike { | |
| getItem(key: string): string | null; | |
| setItem(key: string, value: string): void; | |
| removeItem(key: string): void; | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/storage.ts at line 3, the use of Pick<Storage, 'getItem' |
'setItem' | 'removeItem'> creates a dependency on the DOM library. To fix this,
replace the Pick type with an explicit minimal interface that defines only the
getItem, setItem, and removeItem methods with their appropriate signatures. This
change will remove the DOM lib dependency and keep the utils package portable
for non-DOM environments.
| export function loadFromStorage<T>(key: string, fallback: T, validate?: (value: unknown) => value is T): T { | ||
| const storage = getStorage(); | ||
| if (!storage) return fallback; | ||
| try { | ||
| const raw = storage.getItem(key); | ||
| if (!raw) return fallback; | ||
| const parsed = JSON.parse(raw) as unknown; | ||
| if (validate && !validate(parsed)) return fallback; | ||
| return parsed as T; | ||
| } catch { | ||
| return fallback; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing date revival in JSON.parse (breaks Date preservation).
PR objectives call for reviving dates on hydration; current parse uses plain JSON.parse and returns strings. This will turn Date fields into strings and can break logic expecting Date instances.
Apply this change within loadFromStorage (replace parse line):
- const parsed = JSON.parse(raw) as unknown;
+ const parsed = parseJSONWithDates(raw);Add the helper (outside this range; place above loadFromStorage):
// ISO 8601 detection with leniency for milliseconds
const ISO_DATE_RX = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z$/;
function parseJSONWithDates(raw: string): unknown {
return JSON.parse(raw, (_k, v) => {
if (typeof v === 'string') {
// Fast path: strict ISO Zulu; fallback to Date.parse heuristic
if (ISO_DATE_RX.test(v)) return new Date(v);
if (v.includes('T')) {
const t = Date.parse(v);
if (!Number.isNaN(t)) return new Date(t);
}
}
return v;
});
}If you prefer opt-in behavior, we can add an options param (e.g., { reviveDates?: boolean }) instead of always reviving. Happy to provide that refactor.
🤖 Prompt for AI Agents
In packages/utils/src/storage.ts lines 21 to 33, the JSON.parse call in
loadFromStorage does not revive Date objects, causing Date fields to be
deserialized as strings. To fix this, add a helper function parseJSONWithDates
above loadFromStorage that uses JSON.parse with a reviver detecting ISO 8601
date strings and converting them to Date instances. Replace the JSON.parse call
in loadFromStorage with parseJSONWithDates to ensure Date preservation on
hydration.
💡 Verification agent
🧩 Analysis chain
Sanity-check the state key consistency across repo.
PR objectives mention “todo-starter:state:v1” while the summary/code refer to “todo-app/state@v1”. Let’s verify we’re using a single, consistent key.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for state storage keys..."
rg -n --no-heading -S -e 'todo-app/state@v1' -e 'todo-starter:state:v1' -e 'state@v1|state:v1'Length of output: 588
Ensure PR objectives and summary use the correct storage key
Search across the repo shows that all code (and tests) consistently use the key todo-app/state@v1. The PR objectives and summary still reference todo-starter:state:v1, which does not match.
- Update the PR description (objectives/summary) to replace
todo-starter:state:v1withtodo-app/state@v1.
🤖 Prompt for AI Agents
In packages/utils/src/storage.ts lines 21 to 33, the PR description references
the incorrect storage key 'todo-starter:state:v1'. Update the PR objectives and
summary to use the correct key 'todo-app/state@v1' to match the rest of the
codebase and tests.
| export default defineConfig({ | ||
| test: { | ||
| environment: 'jsdom' | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing setup/globals parity with other packages; tests may lack jest-dom matchers
This config omits setupFiles, globals, and css: false. If your tests rely on @testing-library/jest-dom/vitest via ./src/test/setup.ts, those matchers won’t be registered and assertions like toBeInTheDocument will fail. Align with UI’s config:
export default defineConfig({
test: {
- environment: 'jsdom'
+ environment: 'jsdom',
+ setupFiles: ['./src/test/setup.ts'],
+ globals: true,
+ css: false,
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default defineConfig({ | |
| test: { | |
| environment: 'jsdom' | |
| } | |
| }); | |
| export default defineConfig({ | |
| test: { | |
| environment: 'jsdom', | |
| setupFiles: ['./src/test/setup.ts'], | |
| globals: true, | |
| css: false, | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/utils/vitest.config.ts around lines 3 to 7, the test configuration
is missing setupFiles, globals, and css: false settings, which are necessary to
register jest-dom matchers like toBeInTheDocument. Update the config to include
setupFiles pointing to ./src/test/setup.ts, set globals to true, and add css:
false to ensure parity with other packages and proper test matcher registration.
| "test:watch": { | ||
| "dependsOn": ["^build"], | ||
| "inputs": ["$TURBO_DEFAULT$", ".env*", "vitest.config.*"], | ||
| "outputs": [], | ||
| "cache": false, | ||
| "persistent": true | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning test:ci dependencies with test/test:watch
test and test:watch depend on ^build, but test:ci does not. If packages rely on built outputs (common in monorepos), missing ^build could cause flaky CI runs. If intentional for speed, ignore. Otherwise:
"test:ci": {
- "inputs": ["$TURBO_DEFAULT$", ".env*", "vitest.config.*"],
+ "dependsOn": ["^build"],
+ "inputs": ["$TURBO_DEFAULT$", ".env*", "vitest.config.*"],
"outputs": []
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "test:watch": { | |
| "dependsOn": ["^build"], | |
| "inputs": ["$TURBO_DEFAULT$", ".env*", "vitest.config.*"], | |
| "outputs": [], | |
| "cache": false, | |
| "persistent": true | |
| }, | |
| "test:ci": { | |
| "dependsOn": ["^build"], | |
| "inputs": ["$TURBO_DEFAULT$", ".env*", "vitest.config.*"], | |
| "outputs": [] | |
| } |
🤖 Prompt for AI Agents
In turbo.json around lines 35 to 41, the "test:ci" task is missing the "^build"
dependency that "test" and "test:watch" have, which can cause flaky CI runs if
built outputs are required. To fix this, add "dependsOn": ["^build"] to the
"test:ci" task configuration to ensure it runs after the build step, aligning it
with the other test tasks.
This implements STA-2 for Jake Ruesink by persisting todos to localStorage with a minimal, test-safe storage utility and provider hydration.
What:
Integration details:
todo-app/state@v1Relevant files:
Note: Lint/typecheck in the full workspace raises pre-existing warnings/errors unrelated to this change (export-all and some test/button lint checks). The feature compiles in-app, and storage utilities are tree-shakeable and isolated.
Co-authored-by: Jake Ruesink [email protected]
💻 View my work • 👤 Initiated by
Jake Ruesink• About Codegen⛔ Remove Codegen from PR • 🚫 Ban action checks
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Style