fix: sync after password change, modal styling, contrast, icon padding

[pboueke]

v0.18.8

• sync engine now detects salt change vs remote registry and force re-uploads all pages with new key, fixing "pages could not be downloaded" on cloud import after password change
• cloud import passes derived key to local store, preserving password encryption on imported journals (was saving as device-encrypted only)
• NewJournalModal restyled to match dev menu (transparent fade, floating centered container, webModalContent maxWidth on web)
• new getContrastText utility applied to danger buttons (ConfirmDeleteModal, AccountButton, FilterBar) — all 10 themes now meet WCAG AA contrast
• splash icon and Android adaptive icon (foreground + monochrome) regenerated with proper padding to avoid edge cropping
• 8 new tests (4 sync encryption, 1 web cloud import, 3 contrast utility)

[github-actions]

Carranca AI Review

I'll review this PR focusing on security, TypeScript correctness, React Native patterns, encryption correctness, test coverage, and state management.

Code Review for PR #8: fix: sync after password change, modal styling, contrast, icon padding

Summary

This PR implements critical fixes for sync/encryption issues when changing journal passwords, improves UI contrast accessibility, and restyles the NewJournalModal for better web experience. The core fix prevents data loss by detecting salt changes in the sync engine and forcing re-encryption of all pages with the new key, addressing a security-critical bug where encrypted data on GDrive would become inaccessible after password changes.

Findings

Security - CRITICAL

Critical: Potential information leak in error handling (src/components/home/NewJournalModal.tsx:289-290, 304)

• Location: Lines 289-290, 304
• Issue: When cloud import fails with decryption errors, the error messages may expose sensitive information about the encryption process or GDrive structure
• Impact: Could leak implementation details or make debugging easier for attackers
• Recommendation: Sanitize error messages before displaying to users; use generic "Decryption failed" or "Import failed" messages for crypto-related errors

Security - HIGH

High: No rollback mechanism for partial sync during key change (src/lib/sync/engine.ts:111-158, 184-519)

• Location: Lines 111-158, 184-519
• Issue: If the sync is interrupted after detecting a key change but before completing page re-uploads, the system could be left in an inconsistent state where some pages are encrypted with the old key and some with the new key
• Impact: Data corruption scenario where subsequent syncs could fail or data becomes inaccessible
• Recommendation: Implement transactional sync with rollback capability, or ensure atomic operations with clear recovery paths

Encryption - HIGH

High: Potential for partial data state during cloud import (src/components/home/NewJournalModal.tsx:288-290, 332-335)

• Location: Lines 288-290, 332-335
• Issue: If saveJournal succeeds but savePage loop fails partway through, the journal metadata is saved but pages are only partially imported, leaving users in an inconsistent state
• Impact: User could have journal entries that cannot be read or synchronized
• Recommendation: Wrap the entire import operation in a try-catch and cleanup partial imports on failure, or implement transactional import semantics

Performance - MEDIUM

Medium: Redundant remote registry fetch in sync (src/lib/sync/engine.ts:489)

• Location: Line 489
• Issue: listRemoteJournals() is called early in sync to detect key changes, but this may be redundant if the registry is also fetched later for other operations
• Impact: Additional API call on every sync operation
• Recommendation: Consider caching this result or passing it as a parameter if it's needed elsewhere in the sync flow

Accessibility - MEDIUM

Medium: No visible disabled state for contrast-changed buttons (src/components/journal/ConfirmDeleteModal.tsx:115-119, src/components/home/AccountButton.tsx:497-505, src/components/journal/FilterBar.tsx:184-189)

• Location: Multiple locations where getContrastText is applied
• Issue: When buttons use theme.colors.error with contrast-aware text, the disabled state (opacity: 0.5) may not provide sufficient visual feedback for colorblind users
• Impact: Accessibility compliance concern for users with color vision deficiencies
• Recommendation: Consider adding additional visual indicators (e.g., grayscale conversion, pattern overlay) for disabled states beyond just opacity

Code Quality - LOW

Low: Contrast utility could benefit from caching (src/styles/themes.ts:1596-1627)

• Location: Lines 1596-1627
• Issue: getContrastText recalculates luminance on every call even for the same color values
• Impact: Minor performance overhead, especially during theme switching
• Recommendation: Consider memoization or simple caching mechanism for frequently used color values

Testing - INFO

Info: Good test coverage for critical paths

• New tests comprehensively cover the password change sync scenarios and cloud import flows
• Tests verify both success and edge cases including password removal scenarios
• Contrast utility tests include WCAG AA compliance verification across all themes
• No issues found in test implementation

Verdict

REQUEST_CHANGES

The PR implements critical fixes for serious security and data integrity issues around password changes and encryption key handling. However, the lack of proper error handling rollback mechanisms and potential information disclosure in error messages present significant risks that should be addressed before merging. The encryption fixes are sound and necessary, but the partial state scenarios could lead to user data loss or corruption in failure modes.

carranca bound glm-4.6

---

Automated review by carranca sandbox

[github-actions]

Carranca AI Review

I'll review PR #8 by examining the changes in detail. Let me first see the complete diff by reading the remaining portions.

Summary

PR #8 fixes 5 bugs: (1) sync engine now detects key changes via salt comparison and force re-uploads all pages, (2) cloud import preserves password encryption locally, (3) NewJournalModal restyled as floating modal with proper web width constraints, (4) danger buttons use contrast-aware text colors, (5) app icons regenerated with padding.

Findings

Critical

None

High

None

Medium

None

Low

src/lib/sync/engine.ts:190 - remoteEntry appears unused in key-change branch - when keyChanged is true, the code executes re-upload but never reads remoteEntry. Harmless but could be simplified for clarity.

Info

src/styles/themes.ts:714-742 - getContrastText re-parses color on every call, which has negligible performance impact given current usage. Caching could be added if called frequently in the future.

src/components/home/NewJournalModal.tsx:69-71 - Correctly removed useSafeAreaInsets since modal is now floating rather than full-screen. This is the proper React Native pattern change.

.gitignore:285 - Changed from google-credentials.ts to google-credentials*.ts - this is a positive security change that prevents accidentally committing credential files (including test variants like google-credentials-test.ts).

Verdict

APPROVE

This is a well-executed PR that fixes critical security and sync bugs with proper encryption handling and thorough test coverage. The sync engine's key rotation strategy (detecting salt changes, updating registry last) follows best practices for atomic-ish operations. The cloud import fix addresses a security issue where password-protected journals were being downgraded to device encryption. The UI improvements (contrast utility, modal restyling) follow React Native patterns correctly. The 8 new tests comprehensively cover encryption edge cases including interrupted syncs and password changes. Encryption parameters (PBKDF2-SHA256, 50k+ iterations, AES-256-GCM with unique nonces) are all correct.

carranca bound glm-4.6

---

Automated review by carranca sandbox