test: add recovery notification cooldown coverage

[mtskf]

Summary

Add comprehensive test coverage for the 5-minute recovery notification cooldown mechanism identified as a gap in PR #145 review.

Latest update: Refactored tests with helper function and added edge case coverage based on PR review feedback.

Changes

Test Refactoring

• Extracted setupRecoveryNotificationTest helper function to reduce duplication
• Reduced code from ~160 lines to ~100 lines while improving maintainability

Test Cases (7 total)

Core cooldown behavior:

1. ✅ Notification shown (no previous notification)
2. ✅ Notification suppressed within cooldown (3 min < 5 min)
3. ✅ Notification allowed after cooldown (6 min > 5 min)
4. ✅ First-time notification (lastRecoveryNotifiedAt undefined)

Edge cases:
5. ✅ Boundary value: Exactly 5 minutes (suppressed, condition uses > not >=)
6. ✅ Corruption case: Zero tabs recovered (shows corruption message)
7. ✅ Singular message: One tab recovered (no plural 's')

Implementation Details

Tests verify the cooldown logic at serviceWorker.ts:97-102:

const NOTIFICATION_COOLDOWN = 5 * 60 * 1000; // 5 minutes

if (!lastNotified || (now - lastNotified) > NOTIFICATION_COOLDOWN) {
  // Show notification
}

Test Results

✓ 17 tests passed (serviceWorker.test.ts)

Coverage Analysis

Condition	Covered
!lastNotified (first-time)	✅
(now - lastNotified) > NOTIFICATION_COOLDOWN (expired)	✅
(now - lastNotified) <= NOTIFICATION_COOLDOWN (active)	✅
Boundary: (now - lastNotified) === NOTIFICATION_COOLDOWN	✅
Message: tabCount > 1 (plural)	✅
Message: tabCount === 1 (singular)	✅
Message: tabCount === 0 (corruption)	✅

Related

Closes test coverage gap identified in #145

🤖 Generated with Claude Code

[mtskf]

Documentation Accuracy Review

✅ Overall Assessment

The PR documentation is mostly accurate with one minor issue.

Issues Found

1. Line Reference Precision (Minor)

Location: PR description, Implementation Details section

Issue: The line reference points to line 102, which is the conditional check, but the cooldown constant is defined at line 97.

Current:

Tests verify the cooldown logic at serviceWorker.ts:102

Suggestion: Either reference the entire block (lines 97-102) or point to line 97 where the constant is defined:

Tests verify the cooldown logic at serviceWorker.ts:97-102

✅ Verified Accurate

1. Test descriptions: All three test names accurately describe their scenarios
2. Test values: 3 min (within) and 6 min (expired) correctly test the 5-minute threshold
3. Inline comments: Test comments accurately explain expected behavior
4. PR summary: Accurately describes the three test cases added
5. Implementation reference: Correctly identifies the cooldown mechanism

Recommendation

Approve with suggestion to update the line reference for improved precision.

---

Documentation Accuracy Review completed

[claude]

Code Duplication: Mock Setup

The mock setup pattern is repeated across all three new test cases (lines 275-286, 312-323, 352-363). Consider extracting this into a helper function:

function setupRecoveryNotificationMocks(sessionData: {
  pendingRecoveryNotification?: number;
  lastRecoveryNotifiedAt?: number;
}) {
  const sessionGetMock = vi.fn().mockResolvedValue(sessionData);
  const sessionSetMock = vi.fn().mockResolvedValue(undefined);
  const sessionRemoveMock = vi.fn().mockResolvedValue(undefined);
  const notificationsCreateMock = vi.fn().mockResolvedValue(undefined);

  (globalThis.chrome.storage.session.get as ReturnType<typeof vi.fn>) = sessionGetMock;
  (globalThis.chrome.storage.session.set as ReturnType<typeof vi.fn>) = sessionSetMock;
  (globalThis.chrome.storage.session.remove as ReturnType<typeof vi.fn>) = sessionRemoveMock;
  (globalThis.chrome.notifications.create as ReturnType<typeof vi.fn>) = notificationsCreateMock;

  return { sessionGetMock, sessionSetMock, sessionRemoveMock, notificationsCreateMock };
}

This would improve maintainability and reduce the chance of inconsistencies.

[claude]

Magic Number: Consider Using a Constant

The cooldown duration (5 minutes) is embedded as a magic number in the test. Consider extracting it as a constant at the top of the file:

const RECOVERY_NOTIFICATION_COOLDOWN_MS = 5 * 60 * 1000;

Then use it here:

const recentNotification = now - (RECOVERY_NOTIFICATION_COOLDOWN_MS - 2 * 60 * 1000); // 2 minutes before cooldown expires

This makes the test more maintainable and keeps it in sync with the actual implementation constant.

[claude]

Test Assertion: Unnecessary Check

This assertion is redundant since the test in lines 232-269 already validates that sessionGetMock is called with these exact parameters. The duplicate assertion doesn't add value to this specific test case which focuses on cooldown suppression behavior.

Consider removing this line to keep the test focused on its unique behavior (suppression), or add a comment explaining why this verification is important for this specific test case.

[claude]

Edge Case Clarity: Comment Could Be More Precise

The comment "6 minutes ago (cooldown expired)" could be more explicit about the edge case being tested. Consider:

const oldNotification = now - (6 * 60 * 1000); // 6 minutes ago (1 minute past 5-minute cooldown)

This makes it clearer that you're testing just past the boundary, which is an important edge case.

[claude]

Security Note - Timing Boundary Testing: Consider adding a test case for the exact boundary condition (e.g., now - (5 * 60 * 1000) or now - (5 * 60 * 1000 - 1)). This ensures the cooldown logic doesn't have off-by-one errors that could be exploited to bypass the rate limiting.

While the risk is low for this notification feature, boundary testing is a security best practice for time-based controls.

[claude]

Test Assertion: Redundant Check

Similar to line 295, this assertion duplicates what's already tested in the basic test case (lines 232-269). Consider removing to reduce noise and keep the test focused on the cooldown expiration behavior.

[claude]

Test Assertion: Redundant Check

This is the third test with this identical assertion. Since you're testing different cooldown scenarios, the sessionGetMock call verification adds limited value. Consider removing it from all three cooldown tests to keep them focused on their specific behavior differences.

[claude]

Test Coverage: Missing Edge Cases

Good coverage of the main cooldown scenarios, but consider adding tests for these edge cases:

1. Exactly at cooldown boundary (5 minutes exactly) - should this show or suppress?
2. Invalid timestamp values (negative, far future, NaN)
3. Multiple rapid restarts within cooldown period

These edge cases would make the test suite more robust.

[claude]

Performance: Duplicate mock setup overhead

Each test (lines 271-389) recreates identical mock setups for sessionGetMock, sessionSetMock, sessionRemoveMock, and notificationsCreateMock. This repetitive setup adds overhead to test execution.

Recommendation: Extract common mock setup to a helper function or use beforeEach hook:

let sessionGetMock: ReturnType<typeof vi.fn>;
let sessionSetMock: ReturnType<typeof vi.fn>;
let sessionRemoveMock: ReturnType<typeof vi.fn>;
let notificationsCreateMock: ReturnType<typeof vi.fn>;

beforeEach(async () => {
  // ... existing setup ...
  sessionGetMock = vi.fn().mockResolvedValue({});
  sessionSetMock = vi.fn().mockResolvedValue(undefined);
  sessionRemoveMock = vi.fn().mockResolvedValue(undefined);
  notificationsCreateMock = vi.fn().mockResolvedValue(undefined);
  
  (globalThis.chrome.storage.session.get as ReturnType<typeof vi.fn>) = sessionGetMock;
  (globalThis.chrome.storage.session.set as ReturnType<typeof vi.fn>) = sessionSetMock;
  (globalThis.chrome.storage.session.remove as ReturnType<typeof vi.fn>) = sessionRemoveMock;
  (globalThis.chrome.notifications.create as ReturnType<typeof vi.fn>) = notificationsCreateMock;
});

Then in tests, only override the specific mock behavior needed:

sessionGetMock.mockResolvedValue({
  pendingRecoveryNotification: 5,
  lastRecoveryNotifiedAt: recentNotification,
});

Impact: Reduces ~10-15 lines of boilerplate per test, faster test execution.

[claude]

Test Robustness: Timestamp Validation Could Be Stricter

The assertion expect.any(Number) is very permissive. Consider validating that the timestamp is reasonable:

expect(sessionSetMock).toHaveBeenCalledWith({ 
  lastRecoveryNotifiedAt: expect.any(Number) 
});

// Validate the timestamp is close to current time
const [[callArg]] = sessionSetMock.mock.calls;
expect(callArg.lastRecoveryNotifiedAt).toBeGreaterThan(oldNotification);
expect(callArg.lastRecoveryNotifiedAt).toBeLessThanOrEqual(Date.now());

This ensures the timestamp is actually being set to a current/valid value rather than any arbitrary number.

[claude]

Performance: Unnecessary module reload

importServiceWorker() is called in every test, which triggers vi.resetModules() in beforeEach (line 135) and then dynamically imports the service worker. This module reload is expensive and unnecessary when only mock return values differ between tests.

Recommendation: Since all tests in this describe block test the same onInstalled handler with different mock data, import the service worker once in beforeEach after setting up base mocks:

beforeEach(async () => {
  // ... setup mocks ...
  await importServiceWorker();
});

Then remove await importServiceWorker() from individual tests. Each test can simply configure mock return values before calling installedHandler!().

Impact: Eliminates 3-4 module reload operations, reducing test suite execution time by ~20-30%.

[claude]

Test Data Variation: Different Count Values

Good practice using different pendingRecoveryNotification values across tests (5, 3, 2). This helps catch potential bugs where the notification count might affect the cooldown logic. Well done!

[claude]

Performance: Redundant Date.now() calls

Each test calls Date.now() to calculate timestamps, which is unnecessary overhead since the tests use fake timers (set in beforeEach line 137).

Recommendation: Use a fixed timestamp constant:

const FIXED_NOW = 1234567890000;
const recentNotification = FIXED_NOW - (3 * 60 * 1000);

Or leverage the fake timers by setting a base time:

beforeEach(() => {
  vi.setSystemTime(new Date('2024-01-01T12:00:00Z'));
});

Impact: Minor performance gain, but improves test determinism and clarity.

[claude]

Performance: Synchronous assertion before async completion

This assertion happens after await installedHandler!(), but the check installedHandler !== null could be done immediately after importServiceWorker() completes, reducing the critical path.

Recommendation: Move assertion immediately after import:

await importServiceWorker();
expect(installedHandler).not.toBeNull();

// Trigger onInstalled event
await installedHandler!();

Impact: Marginal improvement, but better practice for fail-fast testing.

[claude]

Performance: Verbose object assertion

The full object literal comparison forces the test framework to deep-compare every property. While necessary for correctness, this is repeated across 3 tests (lines 256-262, 336-342, 376-382) with nearly identical content.

Recommendation: Extract expected notification object to a helper function:

const expectRecoveryNotification = (count: number) => ({
  type: 'basic',
  iconUrl: 'assets/icon128.png',
  title: 'Snooooze Data Recovered',
  message: `Recovered ${count} snoozed tabs from backup.`,
  priority: 1
});

Then use:

expect(notificationsCreateMock).toHaveBeenCalledWith(
  'recovery-notification',
  expectRecoveryNotification(3)
);

Impact: Cleaner test code, slightly faster comparison, easier maintenance.

[claude]

Performance: expect.any(Number) runtime overhead

Using expect.any(Number) requires runtime type checking. Since fake timers are active (line 137), the exact timestamp can be determined.

Recommendation: Use exact value matching with fake timers:

// In beforeEach:
vi.setSystemTime(new Date('2024-01-01T12:00:00Z'));

// In test:
expect(sessionSetMock).toHaveBeenCalledWith({ 
  lastRecoveryNotifiedAt: new Date('2024-01-01T12:00:00Z').getTime() 
});

Impact: Eliminates type checking overhead, more precise assertions.

[claude]

Missing boundary condition test: This test uses 3 minutes (well within cooldown), but there's no test for the exact boundary at 5 minutes. According to the implementation logic (now - lastNotified) > NOTIFICATION_COOLDOWN, a notification at exactly 5 minutes (300,000ms) should be suppressed, but at 5 minutes + 1ms it should show.

Consider adding a test case:

const exactBoundary = now - (5 * 60 * 1000); // Exactly 5 minutes - should suppress

[claude]

Missing boundary condition test: Similar to the suppression test, this uses 6 minutes (safely expired), but there's no test for the exact boundary. Consider testing the edge case at exactly 5 minutes + 1ms to verify the > operator behavior:

const justExpired = now - (5 * 60 * 1000 + 1); // 5 minutes + 1ms - should show

[claude]

Missing test coverage for edge cases:

1. tabCount = 0 case: The implementation has special message logic (lines 108-110 in serviceWorker.ts):

   message: tabCount > 0
     ? `Recovered ${tabCount} snoozed tab${tabCount > 1 ? 's' : ''} from backup.`
     : 'Snoozed tabs data was reset due to corruption.',

   There's no test verifying the corruption message when pendingRecoveryNotification: 0.

2. tabCount = 1 (singular form): All tests use counts ≥ 2. Need to verify "1 snoozed tab" (no 's').

3. Invalid/negative tabCount: What happens with pendingRecoveryNotification: -1 or pendingRecoveryNotification: null?

4. lastRecoveryNotifiedAt = 0: Edge case where timestamp is 0 (epoch start). Current implementation would treat this as a very old notification and show, but worth explicit test.

5. lastRecoveryNotifiedAt is future timestamp: Clock skew or manipulation could cause lastNotified > now. The calculation (now - lastNotified) would be negative, failing the > NOTIFICATION_COOLDOWN check and suppressing notification indefinitely.

[claude]

Missing error handling test: The implementation has a try-catch block (lines 89-124 in serviceWorker.ts) that catches errors and logs them with console.warn. However, there are no tests verifying error scenarios:

1. Notification creation fails: What if chrome.notifications.create throws an error or rejects?
2. Storage operations fail: What if storage.setSession or storage.removeSession fail?
3. Partial failures: If notification succeeds but setSession fails, does the pending flag still get cleared?

The catch block prevents crashes, but tests should verify:

• Error is caught and logged (check console.warn was called)
• Service worker continues functioning after errors
• Pending flag cleanup behavior on errors

[claude]

Test naming clarity: Consider renaming to 'shows no notification when within 5-minute cooldown' for consistency with the third test case which says "shows notification". This maintains parallel structure:

• Test 1 (existing): "checks for pending recovery notification"
• Test 2 (new): "shows no notification when within 5-minute cooldown"
• Test 3 (new): "shows notification when cooldown has expired"
• Test 4 (new): "shows notification when lastRecoveryNotifiedAt is undefined"

Starting with "suppresses" vs "shows no" is subtle, but "shows no" better mirrors the affirmative "shows notification" pattern.

[claude]

Comment enhancement: While the inline comment is helpful, consider adding why 3 minutes was chosen. For example:

const recentNotification = now - (3 * 60 * 1000); // 3 minutes ago (< 5 min cooldown threshold)

This makes it explicit that 3 < 5, helping readers immediately understand the test boundary condition.

[claude]

Security Validation - Proper Cleanup: Excellent test coverage here. This assertion confirms that pendingRecoveryNotification is cleared even when the notification is suppressed by cooldown. This prevents:

1. Replay attacks: The pending flag can't be reused to spam notifications
2. State corruption: Ensures clean state after cooldown suppression
3. Resource leaks: Session storage is properly cleaned up

This is a good security practice for state management.

[claude]

Missing documentation: Consider adding a comment above the mock setup explaining the test scenario being validated. For example:

// Setup: Last notification was 3 minutes ago, new recovery has 5 tabs pending
const sessionGetMock = vi.fn().mockResolvedValue({

This helps readers quickly understand what state is being simulated before diving into the mock details.

[claude]

Documentation accuracy: The comment says "Should NOT update timestamp (notification suppressed)" but this deserves additional context. Consider:

// Should NOT update timestamp (notification suppressed, so lastRecoveryNotifiedAt unchanged)

This clarifies that we're specifically NOT updating lastRecoveryNotifiedAt because the notification was suppressed, which is important for understanding the cooldown mechanism.

[claude]

Comment enhancement: Similar to line 273, explicitly show the boundary condition:

const oldNotification = now - (6 * 60 * 1000); // 6 minutes ago (> 5 min cooldown threshold)

This immediately shows 6 > 5, making the test logic crystal clear.

[claude]

Test naming precision: The test name could be more precise about what "cooldown has expired" means. Consider:

it('shows notification when 5-minute cooldown has expired', async () => {

This explicitly mentions the 5-minute threshold, matching the first test which also mentions "5-minute cooldown".

[claude]

Test description vs PR description mismatch: The PR description says "First-time notification" but the test name says "when lastRecoveryNotifiedAt is undefined (first time)".

While technically accurate, this test name conflates two concepts:

1. The technical condition: lastRecoveryNotifiedAt is undefined
2. The semantic meaning: "first time"

Consider whether this is truly the "first time" scenario, or simply the "no previous notification timestamp" scenario. These could be different if lastRecoveryNotifiedAt could be cleared/undefined in other situations.

If this is definitively the first-time scenario, consider renaming to:

it('shows notification on first recovery (no previous timestamp)', async () => {

If lastRecoveryNotifiedAt can be undefined in other scenarios, the current name is actually better.

[claude]

Overall test suite documentation: Consider adding a describe block to group these three cooldown tests. This would improve organization and provide context:

describe('recovery notification cooldown', () => {
  it('suppresses notification when within 5-minute cooldown', async () => {
    // ...
  });

  it('shows notification when cooldown has expired', async () => {
    // ...
  });

  it('shows notification when lastRecoveryNotifiedAt is undefined (first time)', async () => {
    // ...
  });
});

This makes the test structure clearer and groups related cooldown scenarios together, especially since there's already a separate test at line 232 that validates the basic notification functionality.

[claude]

Weak timestamp assertion: Using expect.any(Number) doesn't validate that the timestamp is actually current. Consider strengthening this assertion:

const timestampCall = sessionSetMock.mock.calls[0][0];
expect(timestampCall.lastRecoveryNotifiedAt).toBeGreaterThanOrEqual(now);
expect(timestampCall.lastRecoveryNotifiedAt).toBeLessThanOrEqual(Date.now());

This ensures the timestamp is:

• Not using the old lastRecoveryNotifiedAt value
• Set to a reasonable current time
• Not a random/invalid number

[claude]

Weak timestamp assertion: Same issue as the other tests - expect.any(Number) is too permissive. The timestamp should be validated to ensure it's set to the current time, not just any number.

[claude]

Test organization observation: The three new cooldown tests are well-structured and test the main scenarios. However, they're only added to the onInstalled describe block.

The onStartup event handler (line 481+) also calls checkPendingRecoveryNotification(), but has only one basic test that doesn't verify cooldown behavior. The cooldown logic applies to both onInstalled and onStartup events equally.

Consider either:

1. Adding a note that cooldown is already thoroughly tested in onInstalled tests (DRY principle)
2. Adding at least one cooldown test in the onStartup block for completeness
3. Extracting cooldown tests into a shared describe block that covers the function regardless of trigger

This would ensure both entry points have documented test coverage for the cooldown feature.