Skip to content

fix: Add localStorage quota exceeded error handling#20

Merged
Askath merged 1 commit intomainfrom
claude/issue-12-20250718-1908
Jul 18, 2025
Merged

fix: Add localStorage quota exceeded error handling#20
Askath merged 1 commit intomainfrom
claude/issue-12-20250718-1908

Conversation

@Askath
Copy link
Copy Markdown
Owner

@Askath Askath commented Jul 18, 2025

Fixes #12

Summary

Adds proper error handling for localStorage quota exceeded scenarios and fixes architectural issue where repositories bypassed the storage adapter.

Changes

  • Add StorageQuotaExceededException domain exception
  • Update LocalStorageAdapter to specifically handle QuotaExceededError
  • Refactor all localStorage repositories to use adapter via DI instead of direct calls
  • Fix architectural issue where adapter abstraction was bypassed

Impact

  • Prevents data loss during save operations
  • Provides proper error feedback when storage quota exceeded
  • Maintains consistent state between memory and storage
  • Follows DDD architecture patterns

Generated with Claude Code

- Add StorageQuotaExceededException domain exception
- Update LocalStorageAdapter to handle QuotaExceededError specifically
- Refactor all localStorage repositories to use adapter instead of direct calls
- Fix architectural issue where adapter was bypassed

Resolves #12

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Tara Druffel <Askath@users.noreply.github.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 18, 2025

Claude finished @Askath's task —— View job


Code Review Findings

Excellent DDD Architecture Adherence

This PR properly follows the established DDD patterns:

  • Domain Exception: StorageQuotaExceededException correctly placed in domain layer with proper naming and structure
  • Repository Pattern: All repositories now properly use the storage adapter abstraction instead of direct localStorage calls
  • Dependency Injection: Clean constructor injection pattern maintained across all repositories
  • Layer Separation: Infrastructure layer properly isolated from domain concerns

Code Quality - Very Good

StorageQuotaExceededException (src/app/domain/exceptions/storage-quota-exceeded.exception.ts:1-14):

  • Well-structured domain exception with descriptive default message
  • Optional key parameter provides valuable debugging context
  • Proper error name assignment for stack traces

LocalStorageAdapter (src/app/infrastructure/adapters/local-storage.adapter.ts:37-42):

  • Correctly identifies QuotaExceededError by name (proper cross-browser compatibility)
  • Throws domain exception with context (key parameter)
  • Fallback error handling for other localStorage errors

Repository Refactoring:

  • All three repositories (LocalStorageTimerStateRepository, LocalStorageWorkDayRepository, LocalStorageWorkSessionRepository) consistently use DI pattern
  • Constructor injection properly typed: constructor(private readonly storageAdapter: LocalStorageAdapter)
  • Method calls updated from localStorage.setItem() to this.storageAdapter.setItem()

⚠️ Minor Issues & Suggestions

  1. Error Handling Consistency: In LocalStorageAdapter.setItem() (lines 37-44), there's inconsistent error handling:

    • QuotaExceededError throws domain exception (good)
    • Other errors throw generic Error with key info (could be improved)

    Consider creating additional domain exceptions for other storage failures or a more generic storage exception hierarchy.

  2. Error Logging: The adapter logs errors to console before throwing domain exceptions. While helpful for debugging, consider:

    • Using a proper logging service for production environments
    • Ensuring sensitive data isn't logged (currently only keys are logged, which is appropriate)
  3. Testing Considerations: This change will require updating tests to:

    • Mock the LocalStorageAdapter instead of localStorage directly
    • Test quota exceeded scenarios specifically
    • Verify domain exception propagation through the repository layer

Performance Considerations

  • Positive Impact: No performance regressions introduced
  • DI Overhead: Minimal - Angular's DI container handles singleton instances efficiently
  • Error Handling: Early error detection prevents unnecessary retries and state corruption

Security Assessment

  • No Security Issues: The changes maintain existing security posture
  • Error Information: Only storage keys are exposed in error messages, which is appropriate for debugging
  • No Sensitive Data Exposure: Error handling doesn't leak sensitive application data

⚠️ Test Coverage Gaps

The PR doesn't include tests for the new functionality. Recommended test coverage:

  1. StorageQuotaExceededException: Unit tests for proper error construction
  2. LocalStorageAdapter:
    • Mock localStorage to throw QuotaExceededError and verify domain exception
    • Test other error scenarios and fallback behavior
  3. Repository Integration: Test that repositories properly propagate storage exceptions
  4. End-to-End: Verify error handling flows through the entire application stack

Architectural Benefits

This PR addresses a significant architectural issue:

  • Proper Abstraction: Repositories now correctly use the storage adapter abstraction
  • Consistent Error Handling: Centralized error handling logic in the adapter
  • Domain-Driven: Storage concerns properly encapsulated with domain exceptions
  • Testability: Improved testability through proper dependency injection

Summary

This is a high-quality PR that properly implements localStorage quota handling while fixing an architectural inconsistency. The code follows established DDD patterns, maintains clean separation of concerns, and provides appropriate error handling.

Recommendation: ✅ Approve with minor suggestions

The only significant gap is test coverage, which should be addressed in a follow-up PR to ensure the error handling works as expected in various scenarios.

@Askath Askath merged commit 54c55e8 into main Jul 18, 2025
1 check passed
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.

Bug: LocalStorage quota exceeded error handling missing

1 participant