Changes :#41
Merged
Merged
Conversation
📝 WalkthroughWalkthroughA comprehensive screen state audit introduces six reusable UI state components, a delayed-visibility hook, per-store hydration tracking, persisted-data validation, semantic color theme tokens, development fault injection, and applies these across every major screen to standardize loading, empty, error, and retry behaviors. ChangesScreen State Audit & Resilience Hardening
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/journal-editor/journal-editor-screen.tsx (1)
835-840: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
!wasSavedearly return hides real sync state for existing entries.This change makes the label show
"Save"even when an existing entry is already synced, so users lose accurate state feedback ("Synced","Syncing","Saved locally").Suggested fix
- if (!wasSaved) { + if (!wasSaved && entrySyncStatus !== "synced") { return "Save"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/journal-editor/journal-editor-screen.tsx` around lines 835 - 840, The early return in the button label logic when !wasSaved is true prevents checking the real sync state of existing entries. Reorder the conditional checks so that the entrySyncStatus conditions (checking for "synced", "syncing", etc.) are evaluated before or independently of the !wasSaved check. This ensures that entries with actual sync status ("Synced", "Syncing", "Saved locally") display the correct state feedback instead of defaulting to "Save" label, allowing users to see accurate sync information for existing entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/insights/insights-screen.tsx`:
- Around line 518-520: The loading state logic in the isLoading variable does
not account for which report is actually selected for display. Since the code
uses monthlyReport ?? weeklyReport to determine the displayed report, the
loading state should only reflect the loading status of the selected report, not
both reports independently. Modify the isLoading logic to check only the loading
state of the report that will actually be displayed, so that loading is not
shown when the selected report data is already available even if the unselected
report is still loading.
In `@components/journal-editor/journal-editor-screen.tsx`:
- Around line 303-306: The message property in the save-failure dialog contains
a ternary operator that checks appError.code === "local_save_failed" but both
branches return the identical string, making the condition pointless. Remove the
ternary operator and its condition entirely, replacing it with just the single
string message that should be displayed in both cases.
In `@components/journal-history/journal-calendar-view.tsx`:
- Around line 241-255: The month-level empty state (CalendarEmptyState
component) and the per-day empty state both render when the month has no
entries, creating redundant stacked empty cards. Locate the per-day empty state
block around line 270-273 that renders "Nothing written here yet." and add a
condition to suppress it when calendarMonth.totalEntries === 0, so the
month-level empty state is the only one displayed when there are no entries for
the entire month.
In `@constants/theme.ts`:
- Around line 32-67: Extract the hardcoded hex color values from the
SYNC_STATUS_COLORS object to semantic color constants in the colors object for
consistency. Identify all hardcoded colors in SYNC_STATUS_COLORS (such as the
background and text colors for failed, saved_locally, syncing, and other
statuses) and create corresponding semantic palette entries in the colors object
with meaningful names. Then replace the hardcoded hex values in
SYNC_STATUS_COLORS with references to these new semantic color constants,
similar to how colors.textMuted, colors.offlineSurface, and colors.offlineIcon
are already being used for some statuses.
- Around line 19-30: The CONNECTION_STATE_COLORS object contains hardcoded hex
color values for the online state (background, dot, and text properties) that
are not referenced from the colors palette, breaking consistency with the
offline state which correctly uses colors.offlineSurface and colors.offlineIcon.
Extract the three hardcoded online colors (`#CFF8E6`, `#10B981`, `#047857`) by adding
them to the colors object with semantic names (such as onlineSurface,
onlineIcon, onlineText or similar naming convention), then update the
CONNECTION_STATE_COLORS online object to reference these new colors properties
instead of the hardcoded hex values to match the pattern used for the offline
state.
In `@docs/screen-state-audit.md`:
- Around line 9-26: The Journal History row in the audit table contains an
incorrect copy string in the "First Empty" column that reads "Your journal
begins here" but should match the actual implementation showing "Write your
first entry and it will appear here." Update this cell to reflect the correct
string. Additionally, verify whether the Error, Offline, Refreshing, and Retry
columns for both HomeScreen and JournalHistoryScreen are truly implemented at
the component or parent level (such as error boundaries or sync providers), or
if these states are not yet implemented and should be marked as N/A or pending
in the table; update the table entries accordingly based on your verification
findings.
---
Outside diff comments:
In `@components/journal-editor/journal-editor-screen.tsx`:
- Around line 835-840: The early return in the button label logic when !wasSaved
is true prevents checking the real sync state of existing entries. Reorder the
conditional checks so that the entrySyncStatus conditions (checking for
"synced", "syncing", etc.) are evaluated before or independently of the
!wasSaved check. This ensures that entries with actual sync status ("Synced",
"Syncing", "Saved locally") display the correct state feedback instead of
defaulting to "Save" label, allowing users to see accurate sync information for
existing entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30557bfc-ccd3-4ff0-8c77-3ca2b3d1f95a
📒 Files selected for processing (45)
app/insights/report/[periodType].tsxcomponents/achievements/AchievementsScreen.tsxcomponents/ai-chat/ai-chat-screen.tsxcomponents/connectivity/OfflineNotice.tsxcomponents/errors/FeatureErrorBoundary.tsxcomponents/errors/InlineErrorMessage.tsxcomponents/errors/RootErrorFallback.tsxcomponents/home/home-screen.tsxcomponents/insights/insights-screen.tsxcomponents/insights/report/ReportScreenStates.tsxcomponents/journal-editor/entry-ai-reflection-card.tsxcomponents/journal-editor/journal-editor-screen.tsxcomponents/journal-history/journal-calendar-view.tsxcomponents/journal-history/journal-history-screen.tsxcomponents/legal/legal-document-screen.tsxcomponents/profile/notification-settings-screen.tsxcomponents/profile/profile-screen.tsxcomponents/states/FilteredEmptyState.tsxcomponents/states/InlineRefreshState.tsxcomponents/states/ScreenEmptyState.tsxcomponents/states/ScreenErrorState.tsxcomponents/states/ScreenLoadingState.tsxcomponents/states/StaleDataNotice.tsxcomponents/sync/SyncStatusIndicator.tsxcomponents/sync/SyncStatusRow.tsxcomponents/sync/auto-sync-manager.tsxconstants/theme.tsdocs/screen-state-audit.mdglobal.csshooks/useAIInsightReport.tshooks/useAutoSync.tshooks/useDelayedVisibility.tshooks/useEntryReflection.tslib/account/accountDeletionService.tslib/errors/normalizeAppError.tslib/sync/requestSync.tslib/ui/deriveHistoryViewState.tslib/utils/typeGuards.tsstore/useAIInsightReportStore.tsstore/useChatStore.tsstore/useEntryReflectionStore.tsstore/useSyncStore.tssupabase/functions/delete-account/index.tstypes/appError.tstypes/uiState.ts
💤 Files with no reviewable changes (1)
- hooks/useAutoSync.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
store/journal-store.ts (1)
308-320: 🗄️ Data Integrity & Integration | 🟠 MajorPersisted-data sanitization runs only on version migration, not on same-version rehydration.
normalizePersistedJournalEntriesis wired intomigrateonly. Zustand'spersistinvokesmigrateexclusively when the stored version differs fromversion(3). For a returning user already at version 3, a normal cold launch sets the persistedallEntriesdirectly, andonRehydrateStorageonly callssetActiveUserIdandsetHasHydratedwithout sanitizing.This means duplicate scoped IDs and malformed/invalid-timestamp records are not quarantined once the store reaches the current version. Add normalization to
onRehydrateStorageor introduce amergefunction that sanitizes on every rehydration, ensuring corrupted data does not persist indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/journal-store.ts` around lines 308 - 320, The journal store's data sanitization through normalizePersistedJournalEntries is only applied during the migrate function, which runs only when stored versions differ from the current version. Users already at the current version bypass migration and rehydrate directly, causing the onRehydrateStorage callback to skip sanitization. To fix this, add a call to normalizePersistedJournalEntries in the onRehydrateStorage callback to ensure corrupted entries with duplicate scoped IDs and invalid timestamps are sanitized on every rehydration, not just during version migrations. This ensures the allEntries data is always cleaned up before being used by the store.store/useAchievementStore.ts (1)
173-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: Consider consolidating the local
isRecordwith the shared utility.The local
isRecordfunction at lines 173–177 is identical to the sharedisRecordexported fromlib/utils/typeGuards.ts(already used by other stores in this change cohort). Since the file already imports fromlib/validation/persistedDataValidatorsandlib/storage/createPersistStorage, adding one more import would reduce duplication.This is pre-existing, not introduced by this change, so it's optional.
♻️ Consolidation diff
+import { isRecord } from "`@/lib/utils/typeGuards`"; import { createPersistStorage } from "`@/lib/storage/createPersistStorage`"; import { isNonEmptyString } from "`@/lib/validation/persistedDataValidators`"; // ... rest of file -function isRecord(value: unknown): value is Record<string, unknown> { - return ( - typeof value === "object" && value !== null && !Array.isArray(value) - ); -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/useAchievementStore.ts` around lines 173 - 177, Remove the local isRecord function definition from useAchievementStore.ts and import it from the shared utility file lib/utils/typeGuards.ts instead. Add the import statement to the existing import group from lib modules at the top of the file. This eliminates code duplication since the isRecord type guard is already available and used by other stores in the codebase.store/useAIInsightReportStore.ts (1)
78-82: 📐 Maintainability & Code Quality | 🟠 MajorAdd a
mergefunction to validate and sanitize persisted state on every rehydration for consistency with other stores.The notification preferences, onboarding, and achievements stores all use
mergefunctions to validate persisted data during rehydration. This store uses onlymigrate(which runs only on version changes), leaving corruptedreportsByUserdata vulnerable when the persisted version matches the current version.Add a
mergefunction like the other stores to ensurereportsByUseris validated on every rehydration:Example merge pattern from notification-preferences-store
merge: (persistedState, currentState) => ({ ...currentState, ...getSanitizedNotificationPreferences(persistedState), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/useAIInsightReportStore.ts` around lines 78 - 82, Add a `merge` function to the persist configuration in useAIInsightReportStore (alongside the existing onRehydrateStorage, partialize, and storage options) that validates and sanitizes the persisted reportsByUser data on every rehydration. The merge function should accept persistedState and currentState parameters, combine the current state with sanitized/validated reportsByUser from the persisted state (creating a helper function similar to getSanitizedNotificationPreferences used in other stores), and return the merged result to ensure data integrity regardless of whether a version migration occurs.components/journal-editor/journal-editor-screen.tsx (1)
832-846: 🎯 Functional Correctness | 🟠 MajorFix button label to reflect unsaved local changes when editing a synced entry.
When a user edits an existing synced entry and makes local changes, the button label returns
"Synced"without considering thatwasSavedisfalse. The logic at line 833 checksentrySyncStatus === "synced"first and returns immediately, preventing the code from reaching thewasSavedcheck at line 845. This misleads users into thinking their edits are already saved.Restructure the logic to prioritize unsaved local edits: check
wasSavedbefore returning"Synced", or move the synced check after verifying that no local changes exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/journal-editor/journal-editor-screen.tsx` around lines 832 - 846, The button label logic returns "Synced" immediately when entrySyncStatus equals "synced" without checking if wasSaved is false, which masks unsaved local edits. Restructure the conditional checks to prioritize the wasSaved state by either adding a wasSaved check within the entrySyncStatus === "synced" condition (returning "Synced" only if both entrySyncStatus is synced AND wasSaved is true), or by moving the synced check after the wasSaved check. This ensures that when a user edits a previously synced entry, the button reflects that there are unsaved local changes rather than showing "Synced".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/journal-editor/journal-editor-screen.tsx`:
- Around line 303-304: The hardcoded error message in the message property
around line 303-304 in journal-editor-screen.tsx does not match the normalized
error message returned by the normalizeAppError function called with operation
"local_save_journal_entry" on line 292. Update the hardcoded message string to
end with "before leaving this screen." instead of "before leaving." to ensure
consistency with the normalized error message.
In `@lib/ai/remoteJournalAssistant.ts`:
- Around line 31-40: The fault-injection error throws in
remoteJournalAssistant.ts use generic Error class, while other services like
entryReflectionService.ts and aiInsightReportService.ts use custom error classes
for consistency. Create a RemoteJournalAssistantError custom error class at the
top of the file with a structured error code property, then update the error
throws in the fault-injection blocks (the ones in isFaultEnabled checks for
ai_empty_response and ai_invalid_response) to use this new
RemoteJournalAssistantError class instead of generic Error. Optionally apply the
same pattern to any other error throws in the file for full consistency.
In `@lib/validation/persistedDataValidators.ts`:
- Around line 187-211: The relatedEntryIds property in the return object is
assigned directly without an explicit type cast, while the role and source
properties both use `as` type casts for consistency and clarity. Add an explicit
type cast to relatedEntryIds by changing `relatedEntryIds:
value.relatedEntryIds,` to `relatedEntryIds: value.relatedEntryIds as string[] |
undefined,` to align with the established casting pattern used for role and
source properties and make the type intent explicit.
---
Outside diff comments:
In `@components/journal-editor/journal-editor-screen.tsx`:
- Around line 832-846: The button label logic returns "Synced" immediately when
entrySyncStatus equals "synced" without checking if wasSaved is false, which
masks unsaved local edits. Restructure the conditional checks to prioritize the
wasSaved state by either adding a wasSaved check within the entrySyncStatus ===
"synced" condition (returning "Synced" only if both entrySyncStatus is synced
AND wasSaved is true), or by moving the synced check after the wasSaved check.
This ensures that when a user edits a previously synced entry, the button
reflects that there are unsaved local changes rather than showing "Synced".
In `@store/journal-store.ts`:
- Around line 308-320: The journal store's data sanitization through
normalizePersistedJournalEntries is only applied during the migrate function,
which runs only when stored versions differ from the current version. Users
already at the current version bypass migration and rehydrate directly, causing
the onRehydrateStorage callback to skip sanitization. To fix this, add a call to
normalizePersistedJournalEntries in the onRehydrateStorage callback to ensure
corrupted entries with duplicate scoped IDs and invalid timestamps are sanitized
on every rehydration, not just during version migrations. This ensures the
allEntries data is always cleaned up before being used by the store.
In `@store/useAchievementStore.ts`:
- Around line 173-177: Remove the local isRecord function definition from
useAchievementStore.ts and import it from the shared utility file
lib/utils/typeGuards.ts instead. Add the import statement to the existing import
group from lib modules at the top of the file. This eliminates code duplication
since the isRecord type guard is already available and used by other stores in
the codebase.
In `@store/useAIInsightReportStore.ts`:
- Around line 78-82: Add a `merge` function to the persist configuration in
useAIInsightReportStore (alongside the existing onRehydrateStorage, partialize,
and storage options) that validates and sanitizes the persisted reportsByUser
data on every rehydration. The merge function should accept persistedState and
currentState parameters, combine the current state with sanitized/validated
reportsByUser from the persisted state (creating a helper function similar to
getSanitizedNotificationPreferences used in other stores), and return the merged
result to ensure data integrity regardless of whether a version migration
occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 540a4478-05a5-41c8-ac03-8a697d4d9e45
📒 Files selected for processing (28)
app/(tabs)/journal-editor/index.tsxapp/journal/[id].tsxcomponents/ai-chat/ai-chat-screen.tsxcomponents/insights/insights-screen.tsxcomponents/journal-editor/journal-editor-screen.tsxcomponents/journal-history/journal-calendar-view.tsxconstants/theme.tsdocs/crash-edge-case-test-plan.mddocs/screen-state-audit.mdhooks/useEntryReflection.tslib/ai/entryReflectionService.tslib/ai/remoteJournalAssistant.tslib/dev/faultInjection.tslib/insights/aiInsightReportService.tslib/navigation/routeValidators.tslib/storage/createPersistStorage.tslib/sync/requestSync.tslib/validation/activeUser.tslib/validation/persistedDataValidators.tsstore/journal-store.tsstore/notification-preferences-store.tsstore/onboarding-store.tsstore/useAIInsightReportStore.tsstore/useAchievementStore.tsstore/useChatStore.tsstore/useEntryReflectionStore.tsstore/useSyncStore.tstypes/faultInjection.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@store/journal-store.ts`:
- Around line 311-323: The onRehydrateStorage callback in Zustand 5 receives two
parameters (state and error), but the current implementation only accepts state
and returns early when state is falsy, leaving hasHydrated at false and causing
the loading screen to hang. Update the onRehydrateStorage callback to capture
both the state parameter and the error parameter. When state is falsy
(indicating a storage failure), still call useJournalStore.setState with
hasHydrated set to true to ensure the application can proceed past the loading
state. Keep the existing logic for the success path where state exists and
normalizePersistedJournalEntries is called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c60db5b9-3459-435c-8815-498fe38f3481
📒 Files selected for processing (6)
components/journal-editor/journal-editor-screen.tsxlib/ai/remoteJournalAssistant.tslib/validation/persistedDataValidators.tsstore/journal-store.tsstore/useAIInsightReportStore.tsstore/useAchievementStore.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[+] Implemented code-rabbit's suggested bug fixes and improvements.
[+] Improved the app's error handling and state management for better user experience.
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX