Skip to content

feat: implement robust localStorage persistence with error handling#5

Open
quantbitrealmSimon wants to merge 1 commit intomohitagarwal24:mainfrom
quantbitrealmSimon:feat/local-storage-persistence
Open

feat: implement robust localStorage persistence with error handling#5
quantbitrealmSimon wants to merge 1 commit intomohitagarwal24:mainfrom
quantbitrealmSimon:feat/local-storage-persistence

Conversation

@quantbitrealmSimon
Copy link
Copy Markdown

@quantbitrealmSimon quantbitrealmSimon commented Feb 28, 2026

Summary

This PR implements localStorage persistence for the todo application, ensuring tasks are saved and restored across browser sessions.

Changes

Core Features

  • ✅ Tasks are automatically saved to localStorage when added, completed, or deleted
  • ✅ Tasks are restored from localStorage on page load
  • ✅ Data persists across browser refreshes and reopening

Error Handling (Production-Ready)

  • JSON Parsing Errors: Gracefully handles corrupted localStorage data by clearing it and logging the error
  • QuotaExceededError: Handles storage full scenarios with appropriate console warnings
  • Type Safety: Validates that stored data is an array before setting state

Code Quality

  • Added comprehensive JSDoc documentation for the App component
  • Included test suite covering:
    • Saving/loading todos
    • Handling corrupted JSON
    • Handling empty storage
    • Handling quota exceeded errors

Testing

The build passes successfully:

vite v5.4.21 building for production...
✓ 36 modules transformed.
✓ built in 396ms

Checklist

  • Tasks persist after browser refresh
  • Tasks persist after closing and reopening browser
  • Error handling for corrupted data
  • Error handling for storage quota exceeded
  • Code is documented with JSDoc
  • Build passes successfully

Fixes #1
/claim #1

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of todo persistence with robust load/save error handling and automatic recovery from corrupted storage.
  • Tests

    • Added comprehensive test suite covering persistence, corrupted data handling, empty storage, and quota-exceeded scenarios.
    • Added test setup and test configuration for running UI tests.
  • Chores

    • Added test scripts and testing tooling to the project.
  • Documentation

    • Added descriptive comment block for the main app component.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 28, 2026

📝 Walkthrough

Walkthrough

Adds robust localStorage persistence for todos: on mount the app safely loads and validates JSON from localStorage (clears corrupted data), and on todos updates it safely saves to localStorage with specific handling for quota errors and general failures.

Changes

Cohort / File(s) Summary
App + persistence
src/App.jsx
Wraps localStorage load/save in try/catch, validates parsed data before setting state, clears corrupted data, and distinguishes QuotaExceededError when saving. Adds JSDoc block for the component.
Tests & test setup
src/App.test.jsx, src/test-setup.js
Adds Vitest test suite covering save/load, corrupted JSON handling, empty storage, and quota-exceeded simulation. Adds test setup that provides a mocked window.localStorage.
Test runner config
vitest.config.js
New Vitest configuration: jsdom environment, global APIs enabled, setup file wired, React plugin, and coverage reporters configured.
Package scripts / deps
package.json
Adds test and test:ui scripts and devDependencies vitest and jsdom for running the new tests.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code with tiny paws and care,
I tucked your todos in a cozy lair,
If JSON frays or quota makes a fuss,
I tidy, log, and never make a muss. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: implementing robust localStorage persistence with error handling, which matches the core functionality added throughout the PR.
Linked Issues check ✅ Passed The PR implements all technical requirements from issue #1: localStorage integration with JSON serialization, persistence on add/complete/delete, retrieval on page load, consistency across refreshes, and robust error handling for corrupted data and quota exceeded scenarios.
Out of Scope Changes check ✅ Passed All changes are directly scoped to localStorage persistence: App.jsx implements core logic, test files add comprehensive test coverage, package.json adds testing dependencies, and vitest.config.js configures the test environment—all aligned with issue #1 requirements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/App.jsx (2)

30-30: The length > 0 check is unnecessary.

An empty array [] is valid stored data. While this doesn't cause a bug (initial state is already []), the check is semantically misleading—it treats an empty saved list as invalid. Simply checking Array.isArray(parsedTodos) would be clearer and more consistent.

♻️ Suggested simplification
-        if (Array.isArray(parsedTodos) && parsedTodos.length > 0) {
+        if (Array.isArray(parsedTodos)) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.jsx` at line 30, The condition that checks saved todos uses
"Array.isArray(parsedTodos) && parsedTodos.length > 0", which incorrectly treats
an empty stored array as invalid; update the check to only use
Array.isArray(parsedTodos) so valid empty arrays are accepted (locate the
conditional around parsedTodos in App.jsx, likely inside the effect or
load-from-storage logic) and remove the unnecessary "parsedTodos.length > 0"
clause.

6-6: Consider lazy initialization to avoid overwriting localStorage on mount.

The current pattern has a subtle timing issue: the save effect runs on initial mount with todos = [] before the load effect's setTodos triggers a re-render. This briefly overwrites localStorage with an empty array, creating a small window for data loss if the user refreshes.

The idiomatic React pattern is to use lazy initialization in useState, which reads localStorage synchronously before the first render:

♻️ Suggested refactor using lazy initialization
 function App() {
-  const [todos, setTodos] = useState([])
+  const [todos, setTodos] = useState(() => {
+    try {
+      const storedTodos = localStorage.getItem("todos")
+      if (storedTodos) {
+        const parsedTodos = JSON.parse(storedTodos)
+        if (Array.isArray(parsedTodos)) {
+          return parsedTodos
+        }
+      }
+    } catch (error) {
+      console.error("Error loading todos from localStorage:", error)
+      localStorage.removeItem("todos")
+    }
+    return []
+  })

Then remove the load effect (lines 24-39) entirely, keeping only the save effect.

Also applies to: 24-52

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.jsx` at line 6, The initial useState call (const [todos, setTodos] =
useState([])) causes an empty array to be saved before the async load effect
runs; change to lazy initialization by passing a function to useState that reads
and parses localStorage (fallback to []), remove the separate load effect that
calls setTodos (the load logic moves into the lazy initializer), and keep the
existing save effect that watches todos to persist changes; ensure you still
JSON.parse safely and handle missing/malformed data in the initializer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/App.jsx`:
- Line 30: The condition that checks saved todos uses
"Array.isArray(parsedTodos) && parsedTodos.length > 0", which incorrectly treats
an empty stored array as invalid; update the check to only use
Array.isArray(parsedTodos) so valid empty arrays are accepted (locate the
conditional around parsedTodos in App.jsx, likely inside the effect or
load-from-storage logic) and remove the unnecessary "parsedTodos.length > 0"
clause.
- Line 6: The initial useState call (const [todos, setTodos] = useState([]))
causes an empty array to be saved before the async load effect runs; change to
lazy initialization by passing a function to useState that reads and parses
localStorage (fallback to []), remove the separate load effect that calls
setTodos (the load logic moves into the lazy initializer), and keep the existing
save effect that watches todos to persist changes; ensure you still JSON.parse
safely and handle missing/malformed data in the initializer.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13fc303 and c83228c.

📒 Files selected for processing (1)
  • src/App.jsx

- Implement localStorage persistence for todos with comprehensive error handling
- Add try-catch for JSON parsing to handle corrupted data
- Handle QuotaExceededError when storage is full
- Add JSDoc documentation for App component
- Add test suite for localStorage functionality
- Update package.json with test scripts

Fixes mohitagarwal24#1
/claim mohitagarwal24#1
@quantbitrealmSimon quantbitrealmSimon force-pushed the feat/local-storage-persistence branch from c83228c to e81aa9f Compare February 28, 2026 05:45
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
src/App.jsx (1)

39-41: Unnecessary length > 0 check prevents restoring empty todo lists.

If a user deletes all their todos, [] is saved. On reload, this condition prevents loading it (though the end result is the same). More importantly, it's semantically inconsistent—an empty array is valid persisted state.

Suggested change
-        if (Array.isArray(parsedTodos) && parsedTodos.length > 0) {
+        if (Array.isArray(parsedTodos)) {
           setTodos(parsedTodos)
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.jsx` around lines 39 - 41, The check guarding restoration is too
strict: in App.jsx the condition "if (Array.isArray(parsedTodos) &&
parsedTodos.length > 0)" prevents setting an empty persisted todo list; change
the guard to only verify the type (use Array.isArray(parsedTodos)) and call
setTodos(parsedTodos) whenever parsedTodos is an array so empty arrays are
restored correctly; update the conditional around parsedTodos and setTodos
accordingly.
src/test-setup.js (1)

8-21: Consider resetting the store between tests to prevent state leakage.

The store object persists across tests unless localStorage.clear() is called. Consider exporting the mock or adding a beforeEach hook to reset store = {} automatically. Currently tests must remember to call localStorage.clear().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test-setup.js` around lines 8 - 21, The in-memory store used by the
localStorageMock (store) persists across tests causing state leakage; modify
src/test-setup.js to reset store automatically by either exporting
localStorageMock and exposing a reset function (e.g., resetStore() that sets
store = {}) or adding a beforeEach hook in the test setup file that calls store
= {} (or invokes localStorageMock.clear()) so that getItem, setItem, removeItem
operate on a fresh store for each test; reference the symbols store,
localStorageMock, clear, getItem, setItem, and removeItem when implementing the
reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package.json`:
- Around line 11-12: The package.json scripts include "test:ui": "vitest --ui"
but the `@vitest/ui` package is not listed, causing the command to fail; update
package.json by adding `@vitest/ui` to devDependencies (e.g., under
"devDependencies": { "@vitest/ui": "<latest-compatible-version>" }) and run your
package manager to install (npm/yarn/pnpm install) so the vitest --ui command
can locate the UI package.

In `@src/App.jsx`:
- Line 15: The mount race happens because useEffect that saves `todos` runs
before the async load sets state, so the initial `todos = []` overwrites
localStorage; fix by using useState's lazy initializer to synchronously read and
parse saved todos from localStorage when the App component initializes (replace
the current `useState([])` with a lazy init that reads the same key and falls
back to []), then remove or simplify the separate load effect that calls
`setTodos` (keep only the save effect that writes when `todos` changes). Ensure
you reference the same state variables (`todos`, `setTodos`) and effects in
App.jsx so loading happens synchronously at initialization and prevents the
save-effect race.

In `@src/App.test.jsx`:
- Around line 76-78: The test spies on Storage.prototype.setItem which doesn't
affect the replacement localStorage mock from test-setup.js; update the test to
spy/mock the mock object's setItem directly (e.g., target the localStorageMock
or window.localStorage.setItem used in tests instead of
Storage.prototype.setItem) so the thrown error is actually triggered; replace
the vi.spyOn(Storage.prototype, 'setItem') usage with a spy/mock on the mock's
setItem (the same reference created in test-setup.js) and ensure the mock
implementation throws the intended error.
- Around line 11-92: The tests in src/App.test.jsx are exercising localStorage
directly instead of exercising App's behavior; update the tests to render the
App component (use render, screen, fireEvent from `@testing-library/react`) and
assert that on mount App displays items from localStorage, that adding a todo
via the UI causes localStorage to be updated, and that corrupted localStorage
data does not crash App; also fix the QuotaExceededError test by removing the
ineffective spy on Storage.prototype.setItem (which is bypassed by the
test-setup mock) and instead spy/mock the actual mocked localStorage.setItem
used in test-setup or restore the real window.localStorage for that test so the
quota-exceeded path in App's save logic is exercised when calling App's save
handler (e.g., triggers from the component add/save action).

In `@src/test-setup.js`:
- Around line 10-11: The mock getItem implementation on localStorageMock
incorrectly uses || null which converts stored empty strings to null; update the
getItem function (localStorageMock.getItem) to use nullish coalescing (?? null)
so that empty string values are returned as "" while undefined becomes null,
matching real localStorage behavior.

---

Nitpick comments:
In `@src/App.jsx`:
- Around line 39-41: The check guarding restoration is too strict: in App.jsx
the condition "if (Array.isArray(parsedTodos) && parsedTodos.length > 0)"
prevents setting an empty persisted todo list; change the guard to only verify
the type (use Array.isArray(parsedTodos)) and call setTodos(parsedTodos)
whenever parsedTodos is an array so empty arrays are restored correctly; update
the conditional around parsedTodos and setTodos accordingly.

In `@src/test-setup.js`:
- Around line 8-21: The in-memory store used by the localStorageMock (store)
persists across tests causing state leakage; modify src/test-setup.js to reset
store automatically by either exporting localStorageMock and exposing a reset
function (e.g., resetStore() that sets store = {}) or adding a beforeEach hook
in the test setup file that calls store = {} (or invokes
localStorageMock.clear()) so that getItem, setItem, removeItem operate on a
fresh store for each test; reference the symbols store, localStorageMock, clear,
getItem, setItem, and removeItem when implementing the reset.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c83228c and e81aa9f.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • package.json
  • src/App.jsx
  • src/App.test.jsx
  • src/test-setup.js
  • vitest.config.js

Comment thread package.json
Comment on lines +11 to +12
"test": "vitest",
"test:ui": "vitest --ui"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

vitest --ui @vitest/ui separate package requirement

💡 Result:

Yes—vitest --ui requires installing the UI as a separate (optional) package: @vitest/ui. Install it (typically as a dev dependency) and then run Vitest with the --ui flag. [1]

  • Install: npm i -D @vitest/ui`` [1]
  • Run: vitest --ui [1]
  • (--ui is just the CLI flag that enables the UI mode.) [2]

Sources: Vitest UI guide [1], Vitest CLI docs [2].


Add missing @vitest/ui dependency for test:ui script.

The test:ui script uses vitest --ui, which requires the @vitest/ui package to be installed separately. Without it, the command will fail.

Proposed fix
     "tailwindcss": "^3.4.6",
     "vite": "^5.3.4",
-    "vitest": "^2.0.4"
+    "vitest": "^2.0.4",
+    "@vitest/ui": "^2.0.4"
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` around lines 11 - 12, The package.json scripts include
"test:ui": "vitest --ui" but the `@vitest/ui` package is not listed, causing the
command to fail; update package.json by adding `@vitest/ui` to devDependencies
(e.g., under "devDependencies": { "@vitest/ui": "<latest-compatible-version>" })
and run your package manager to install (npm/yarn/pnpm install) so the vitest
--ui command can locate the UI package.

Comment thread src/App.jsx
* @returns {JSX.Element} The rendered Todo app
*/
function App() {
const [todos, setTodos] = useState([])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Race condition: save effect may overwrite localStorage before load completes.

On initial mount, both effects run. The save effect executes with todos = [] before the load effect's setTodos update takes effect, briefly writing [] to localStorage. While the correct data is restored on re-render, a browser crash in that window could cause data loss.

Use lazy initialization in useState to load from localStorage synchronously, eliminating the race condition:

Proposed fix using lazy initialization
-  const [todos, setTodos] = useState([])
+  const [todos, setTodos] = useState(() => {
+    try {
+      const storedTodos = localStorage.getItem("todos")
+      if (storedTodos) {
+        const parsedTodos = JSON.parse(storedTodos)
+        if (Array.isArray(parsedTodos)) {
+          return parsedTodos
+        }
+      }
+    } catch (error) {
+      console.error("Failed to load todos from localStorage:", error)
+      localStorage.removeItem("todos")
+    }
+    return []
+  })

   // ... other functions ...

-  // Load todos from localStorage on mount with error handling
-  useEffect(() => {
-    try {
-      const storedTodos = localStorage.getItem("todos")
-      if (storedTodos) {
-        const parsedTodos = JSON.parse(storedTodos)
-        if (Array.isArray(parsedTodos) && parsedTodos.length > 0) {
-          setTodos(parsedTodos)
-        }
-      }
-    } catch (error) {
-      console.error("Failed to load todos from localStorage:", error)
-      // Clear corrupted data
-      localStorage.removeItem("todos")
-    }
-  }, [])
-
   // Save todos to localStorage whenever they change with error handling
   useEffect(() => {

Also applies to: 34-47, 51-61

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.jsx` at line 15, The mount race happens because useEffect that saves
`todos` runs before the async load sets state, so the initial `todos = []`
overwrites localStorage; fix by using useState's lazy initializer to
synchronously read and parse saved todos from localStorage when the App
component initializes (replace the current `useState([])` with a lazy init that
reads the same key and falls back to []), then remove or simplify the separate
load effect that calls `setTodos` (keep only the save effect that writes when
`todos` changes). Ensure you reference the same state variables (`todos`,
`setTodos`) and effects in App.jsx so loading happens synchronously at
initialization and prevents the save-effect race.

Comment thread src/App.test.jsx
Comment on lines +11 to +92
describe('localStorage Persistence', () => {
beforeEach(() => {
// Clear localStorage before each test
localStorage.clear()
vi.clearAllMocks()
})

it('should save todos to localStorage', () => {
const todos = [{ id: 1, todo: 'Test task', completed: false }]
localStorage.setItem('todos', JSON.stringify(todos))

const stored = JSON.parse(localStorage.getItem('todos'))
expect(stored).toEqual(todos)
})

it('should load todos from localStorage', () => {
const todos = [
{ id: 1, todo: 'Task 1', completed: false },
{ id: 2, todo: 'Task 2', completed: true }
]
localStorage.setItem('todos', JSON.stringify(todos))

const stored = JSON.parse(localStorage.getItem('todos'))
expect(stored).toHaveLength(2)
expect(stored[0].todo).toBe('Task 1')
})

it('should handle corrupted localStorage data gracefully', () => {
// Simulate corrupted data
localStorage.setItem('todos', 'invalid json{{')

const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})

expect(() => {
const stored = localStorage.getItem('todos')
if (stored) {
try {
JSON.parse(stored)
} catch (e) {
console.error('Failed to load todos from localStorage:', e)
localStorage.removeItem('todos')
}
}
}).not.toThrow()

expect(consoleSpy).toHaveBeenCalledWith(
'Failed to load todos from localStorage:',
expect.any(SyntaxError)
)

consoleSpy.mockRestore()
})

it('should handle empty localStorage', () => {
const stored = localStorage.getItem('todos')
expect(stored).toBeNull()
})

it('should handle QuotaExceededError', () => {
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})

// Simulate quota exceeded
const error = new Error('Quota exceeded')
error.name = 'QuotaExceededError'

vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
throw error
})

try {
localStorage.setItem('todos', JSON.stringify([{ id: 1 }]))
} catch (e) {
if (e.name === 'QuotaExceededError') {
console.error('localStorage quota exceeded. Unable to save todos.')
}
}

expect(consoleSpy).toHaveBeenCalledWith('localStorage quota exceeded. Unable to save todos.')

consoleSpy.mockRestore()
})
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files --cached | head -20

Repository: mohitagarwal24/to-do

Length of output: 414


🏁 Script executed:

cat -n package.json | head -50

Repository: mohitagarwal24/to-do

Length of output: 1133


🏁 Script executed:

cat test-setup.js 2>/dev/null || echo "File not found"

Repository: mohitagarwal24/to-do

Length of output: 78


🏁 Script executed:

fd test-setup --type f

Repository: mohitagarwal24/to-do

Length of output: 81


🏁 Script executed:

cat -n src/App.test.jsx | head -100

Repository: mohitagarwal24/to-do

Length of output: 3432


🏁 Script executed:

cat -n src/App.jsx 2>/dev/null | head -100

Repository: mohitagarwal24/to-do

Length of output: 3293


🏁 Script executed:

cat -n vite.config.js 2>/dev/null || echo "vite.config.js not found"

Repository: mohitagarwal24/to-do

Length of output: 275


🏁 Script executed:

cat -n vitest.config.js 2>/dev/null || echo "vitest.config.js not found"

Repository: mohitagarwal24/to-do

Length of output: 604


🏁 Script executed:

rg "setupFiles|environment" vite.config.js vitest.config.js 2>/dev/null || echo "No setup config found"

Repository: mohitagarwal24/to-do

Length of output: 227


🏁 Script executed:

fd "*.config.js" --type f

Repository: mohitagarwal24/to-do

Length of output: 354


🏁 Script executed:

rg "jsdom" . --type json

Repository: mohitagarwal24/to-do

Length of output: 371


🏁 Script executed:

cat -n src/main.jsx 2>/dev/null || head -20

Repository: mohitagarwal24/to-do

Length of output: 368


🏁 Script executed:

cat -n src/test-setup.js

Repository: mohitagarwal24/to-do

Length of output: 729


🏁 Script executed:

rg "localStorage|Storage" src/ --type js

Repository: mohitagarwal24/to-do

Length of output: 2697


Tests don't actually test the App component's localStorage integration.

These tests verify localStorage API behavior in isolation but don't render App or trigger its effects. For example:

  • "should save todos" manually calls localStorage.setItem, not App's save logic
  • "should load todos" doesn't verify App reads and displays the data
  • "corrupted data" test manually replicates App's error handling instead of testing App

Additionally, the QuotaExceededError test is broken: it spies on Storage.prototype.setItem (line 76), but the test setup replaces window.localStorage with a plain mock object (src/test-setup.js, lines 10-21), making the spy ineffective. The mocked localStorage.setItem will never trigger the spy.

This provides false confidence. Consider using @testing-library/react to render App and verify:

  1. On mount, todos from localStorage are displayed
  2. Adding a todo updates localStorage
  3. Corrupted data doesn't crash the app
Example integration test approach
import { render, screen, fireEvent } from '@testing-library/react'
import App from './App'

describe('App localStorage integration', () => {
  beforeEach(() => {
    localStorage.clear()
    vi.clearAllMocks()
  })

  it('loads todos from localStorage on mount', () => {
    const todos = [{ id: 1, todo: 'Test task', completed: false }]
    localStorage.setItem('todos', JSON.stringify(todos))
    
    render(<App />)
    
    expect(screen.getByText('Test task')).toBeInTheDocument()
  })

  it('saves todos to localStorage when adding', async () => {
    render(<App />)
    
    // Add a todo via UI interaction
    // ...
    
    const stored = JSON.parse(localStorage.getItem('todos'))
    expect(stored).toHaveLength(1)
  })
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.test.jsx` around lines 11 - 92, The tests in src/App.test.jsx are
exercising localStorage directly instead of exercising App's behavior; update
the tests to render the App component (use render, screen, fireEvent from
`@testing-library/react`) and assert that on mount App displays items from
localStorage, that adding a todo via the UI causes localStorage to be updated,
and that corrupted localStorage data does not crash App; also fix the
QuotaExceededError test by removing the ineffective spy on
Storage.prototype.setItem (which is bypassed by the test-setup mock) and instead
spy/mock the actual mocked localStorage.setItem used in test-setup or restore
the real window.localStorage for that test so the quota-exceeded path in App's
save logic is exercised when calling App's save handler (e.g., triggers from the
component add/save action).

Comment thread src/App.test.jsx
Comment on lines +76 to +78
vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
throw error
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Mock conflict: Storage.prototype.setItem spy won't intercept the mocked localStorage.

test-setup.js replaces window.localStorage with a plain object whose setItem is a separate vi.fn(). Spying on Storage.prototype.setItem has no effect on this mock—the spy throws, but localStorageMock.setItem is still called instead, so the error never occurs.

Proposed fix: spy on the mock's setItem directly
   it('should handle QuotaExceededError', () => {
     const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
     
     // Simulate quota exceeded
     const error = new Error('Quota exceeded')
     error.name = 'QuotaExceededError'
     
-    vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
+    vi.spyOn(window.localStorage, 'setItem').mockImplementation(() => {
       throw error
     })
📝 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.

Suggested change
vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
throw error
})
vi.spyOn(window.localStorage, 'setItem').mockImplementation(() => {
throw error
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/App.test.jsx` around lines 76 - 78, The test spies on
Storage.prototype.setItem which doesn't affect the replacement localStorage mock
from test-setup.js; update the test to spy/mock the mock object's setItem
directly (e.g., target the localStorageMock or window.localStorage.setItem used
in tests instead of Storage.prototype.setItem) so the thrown error is actually
triggered; replace the vi.spyOn(Storage.prototype, 'setItem') usage with a
spy/mock on the mock's setItem (the same reference created in test-setup.js) and
ensure the mock implementation throws the intended error.

Comment thread src/test-setup.js
Comment on lines +10 to +11
const localStorageMock = {
getItem: vi.fn((key) => store[key] || null),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Mock returns null for empty string values stored in localStorage.

Using || null means if an empty string "" is stored, getItem returns null instead of "". Real localStorage.getItem returns the empty string. Use nullish coalescing to match browser behavior.

Proposed fix
 const localStorageMock = {
-  getItem: vi.fn((key) => store[key] || null),
+  getItem: vi.fn((key) => store[key] ?? null),
📝 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.

Suggested change
const localStorageMock = {
getItem: vi.fn((key) => store[key] || null),
const localStorageMock = {
getItem: vi.fn((key) => store[key] ?? null),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test-setup.js` around lines 10 - 11, The mock getItem implementation on
localStorageMock incorrectly uses || null which converts stored empty strings to
null; update the getItem function (localStorageMock.getItem) to use nullish
coalescing (?? null) so that empty string values are returned as "" while
undefined becomes null, matching real localStorage behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local Storage Persistence

2 participants