Skip to content

fix: try cache first for mobile notifications#994

Merged
peterchinman merged 2 commits intomainfrom
peter/use-cache
Jan 14, 2026
Merged

fix: try cache first for mobile notifications#994
peterchinman merged 2 commits intomainfrom
peter/use-cache

Conversation

@peterchinman
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 14, 2026

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Jan 14, 2026

Code Review

I found one issue related to CLAUDE.md compliance:

Missing Tests for New Functionality

This PR adds a new exported function getNotificationFromCache() and modifies the behavior of openNotificationFromId() to check the cache first, but no tests are included.

According to CLAUDE.md, Development Notes #4 states:

Testing - Write tests for your changes. When fixing bugs or regressions, identify the issue with a test before fixing it.

Suggested Tests

The existing test file at user-notifications.test.tsx shows that this codebase has an established testing practice for functions in user-notifications.ts.

Consider adding tests for:

  1. getNotificationFromCache():

    • Notification found in cache
    • Notification not found in cache (returns undefined)
    • Multiple pages in cache data
    • Invalid/undefined cache data
  2. openNotificationFromId() cache-first behavior:

    • Opening a notification that exists in cache (should not make network request)
    • Opening a notification not in cache (should fall back to network request)
    • Handling when cached notification fails typing

Related files:

@peterchinman peterchinman requested a review from synoet January 14, 2026 15:53
@peterchinman peterchinman merged commit 328e370 into main Jan 14, 2026
22 checks passed
@peterchinman peterchinman deleted the peter/use-cache branch January 14, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants