fix(sync): restore session permission mode from last message across devices#9
fix(sync): restore session permission mode from last message across devices#9leeroybrun wants to merge 3 commits intomainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughLoads and persists per-session permissionModeUpdatedAts, infers permissionMode from recent message meta using server-aligned timestamps, applies updates when newer than local timestamps, exposes new persistence/time APIs, and centralizes PermissionMode types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MessageStream
participant Storage as SessionStorage
participant Reducer as ReducerState
participant Persistence as LocalPersistence
participant Time as ServerTime
Client->>Storage: applyMessages(messages)
Storage->>Time: (optionally) observeServerTimestamp(message.createdAt)
Storage->>Reducer: request inferred permission meta for messages
Reducer-->>Storage: inferredPermissionMode, inferredAt, latestUsage
Storage->>Storage: compare inferredAt vs session.permissionModeUpdatedAt
alt inferredAt newer
Storage->>Storage: set session.permissionMode & permissionModeUpdatedAt
Storage->>Persistence: saveSessionPermissionModes(...)
Storage->>Persistence: saveSessionPermissionModeUpdatedAts(...)
end
Storage->>Storage: copy latestUsage into session
Note over Storage,Persistence: On init Storage loads session-permission-mode-updated-ats and merges into sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sources/sync/storage.ts (1)
844-858: Consider updating the in-memory cache for consistency.Unlike
applyMessages(line 578), this function doesn't update thesessionPermissionModesin-memory cache after callingsaveSessionPermissionModes. While this has no functional impact (the cache is only used during initial session load), keeping both update paths consistent would improve maintainability.♻️ Suggested fix for consistency
// Persist permission modes (only non-default values to save space) saveSessionPermissionModes(allModes); + sessionPermissionModes = allModes; // No need to rebuild sessionListViewData since permission mode doesn't affect the list display
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sources/sync/storage.tssources/sync/storageTypes.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Runyarn typecheckafter all changes to ensure type safety
Ensure TypeScript strict mode is enabled - all code must be properly typed
Never use Alert module from React Native, always use @sources/modal/index.ts instead
Always apply layout width constraints from @/components/layout to full-screen ScrollViews and content containers for responsive design
Use StyleSheet.create from 'react-native-unistyles' for all styling
Always use function mode in StyleSheet.create when you need theme or runtime access
For Expo Image components, size properties (width, height) must be set as inline styles, not in Unistyles stylesheet, and tintColor must be set directly on the component
Always uset(...)function from @/text for ALL user-visible strings for internationalization
Always use expo-router API, not react-navigation API directly
Use ItemList for most containers for UI, only use custom components or alternatives for custom layouts like chat
Always try to use 'useHappyAction' from @sources/hooks/useHappyAction.ts for async operations instead of handling errors manually
For hotkeys, use 'useGlobalKeyboard' from @sources/hooks/useGlobalKeyboard - do not change it, it only works on Web
Use 'AsyncLock' class for exclusive async locks
Always use 'Avatar' component for displaying avatars
Files:
sources/sync/storageTypes.tssources/sync/storage.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 4 spaces for indentation
Use path alias@/*which maps to./sources/*
Files:
sources/sync/storageTypes.tssources/sync/storage.ts
🧬 Code graph analysis (1)
sources/sync/storage.ts (1)
sources/sync/persistence.ts (1)
saveSessionPermissionModes(187-189)
🔇 Additional comments (5)
sources/sync/storageTypes.ts (1)
73-73: LGTM!The new field correctly supports the coordination mechanism with appropriate typing. The optional
number | nulltype allows distinguishing between "never set" (undefined), "explicitly reset" (null), and "set at timestamp" (number), which aligns with the usage instorage.ts.sources/sync/storage.ts (4)
320-328: LGTM!The session merge logic correctly preserves the local coordination timestamp. Using
?? nullensures consistent initialization when the field doesn't exist yet.
520-532: LGTM!The inference logic correctly identifies the permission mode from the most recent user message. Since
messagesArrayis sorted descending bycreatedAt(line 518), the first matching message will be the newest, allowing early termination withbreak.
540-579: LGTM!The conditional update logic correctly protects local user selections from being overwritten by older message inferences. The timestamp comparison on line 544 ensures that newer message-based modes will still take precedence over local UI toggles, which aligns with the PR objective of "Device A's mode to win" when sending after a local toggle.
830-840: LGTM!Setting
permissionModeUpdatedAtto the current time correctly shields the user's explicit selection from being overwritten by older message-based inferences, while still allowing newer messages from other devices to take precedence.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44190a1dca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/sync/storage.ts (1)
982-1011: Missing cleanup ofpermissionModeUpdatedAtswhen deleting a session.The
deleteSessionfunction cleans updraftsandpermission modesbut does not remove the corresponding entry fromsession-permission-mode-updated-ats. This will cause timestamps for deleted sessions to accumulate in persistence over time.🐛 Proposed fix to clean up timestamps
const modes = loadSessionPermissionModes(); delete modes[sessionId]; saveSessionPermissionModes(modes); + + const updatedAts = loadSessionPermissionModeUpdatedAts(); + delete updatedAts[sessionId]; + saveSessionPermissionModeUpdatedAts(updatedAts); // Rebuild sessionListViewData without the deleted sessionNote: You'll need to import
loadSessionPermissionModeUpdatedAtsandsaveSessionPermissionModeUpdatedAtsif not already available in scope (they are already imported at line 14).
🧹 Nitpick comments (2)
sources/sync/persistence.ts (1)
191-206: Consider adding stricter validation for the parsed data.The current validation
typeof parsed !== 'object'will pass for arrays sincetypeof [] === 'object'. Additionally, the values are not validated to be numbers, which could lead to unexpected behavior if corrupted data is loaded.🔧 Suggested stricter validation
export function loadSessionPermissionModeUpdatedAts(): Record<string, number> { const raw = mmkv.getString('session-permission-mode-updated-ats'); if (raw) { try { const parsed = JSON.parse(raw); - if (!parsed || typeof parsed !== 'object') { + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { return {}; } - return parsed; + // Filter to only valid number values + const result: Record<string, number> = {}; + for (const [key, value] of Object.entries(parsed)) { + if (typeof value === 'number') { + result[key] = value; + } + } + return result; } catch (e) { console.error('Failed to parse session permission mode updated timestamps', e); return {}; } } return {}; }Note: This is a defensive improvement. The existing code follows the same pattern as
loadSessionDraftsand other similar functions in this file, so it's consistent with current conventions.sources/sync/storage.ts (1)
839-869: Correct implementation of user-initiated permission mode updates.Setting
permissionModeUpdatedAttoDate.now()ensures local user selections are protected from older message-based inference until a newer message arrives from another device.The persistence logic (building
allModes/allUpdatedAtsmaps and saving) is duplicated betweenapplyMessages(lines 574-588) andupdateSessionPermissionMode(lines 854-869). Consider extracting to a helper function if this pattern expands further.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sources/sync/persistence.tssources/sync/storage.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Runyarn typecheckafter all changes to ensure type safety
Ensure TypeScript strict mode is enabled - all code must be properly typed
Never use Alert module from React Native, always use @sources/modal/index.ts instead
Always apply layout width constraints from @/components/layout to full-screen ScrollViews and content containers for responsive design
Use StyleSheet.create from 'react-native-unistyles' for all styling
Always use function mode in StyleSheet.create when you need theme or runtime access
For Expo Image components, size properties (width, height) must be set as inline styles, not in Unistyles stylesheet, and tintColor must be set directly on the component
Always uset(...)function from @/text for ALL user-visible strings for internationalization
Always use expo-router API, not react-navigation API directly
Use ItemList for most containers for UI, only use custom components or alternatives for custom layouts like chat
Always try to use 'useHappyAction' from @sources/hooks/useHappyAction.ts for async operations instead of handling errors manually
For hotkeys, use 'useGlobalKeyboard' from @sources/hooks/useGlobalKeyboard - do not change it, it only works on Web
Use 'AsyncLock' class for exclusive async locks
Always use 'Avatar' component for displaying avatars
Files:
sources/sync/storage.tssources/sync/persistence.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 4 spaces for indentation
Use path alias@/*which maps to./sources/*
Files:
sources/sync/storage.tssources/sync/persistence.ts
🧬 Code graph analysis (1)
sources/sync/storage.ts (2)
sources/sync/persistence.ts (3)
loadSessionPermissionModeUpdatedAts(191-206)saveSessionPermissionModes(187-189)saveSessionPermissionModeUpdatedAts(208-210)sources/components/PermissionModeSelector.tsx (1)
PermissionMode(7-7)
🔇 Additional comments (5)
sources/sync/persistence.ts (1)
208-210: LGTM!The save function follows the established pattern used by other persistence functions in this file.
sources/sync/storage.ts (4)
14-14: LGTM!Import additions are correct and consistent with existing patterns.
253-253: LGTM!Initialization follows the existing pattern for
sessionDraftsandsessionPermissionModes.
307-331: LGTM!The merging logic correctly prioritizes existing state > saved persistence > incoming session data. The use of nullish coalescing (
??) forpermissionModeUpdatedAtis appropriate since0could theoretically be a valid timestamp, and the comment clarifies this field is not server-synced.
523-588: Well-implemented permission mode inference logic.The implementation correctly:
- Finds the most recent user message with permission mode metadata (lines 528-535).
- Only applies inference if the message timestamp is newer than the local update timestamp (line 547).
- Persists both modes and timestamps to ensure restart-safe arbitration.
One observation: The persistence loop (lines 576-583) iterates all sessions on each inference write. This is acceptable for typical session counts but worth noting if session volume grows significantly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/sync/storage.ts (1)
1017-1024: Missing cleanup forpermissionModeUpdatedAtsstorage on session deletion.The session deletion cleans up
draftsandpermission modesbut leaves orphanedpermissionModeUpdatedAtentries in persistent storage. This won't cause functional issues but will leak storage over time.Proposed fix
const modes = loadSessionPermissionModes(); delete modes[sessionId]; saveSessionPermissionModes(modes); + + const updatedAts = loadSessionPermissionModeUpdatedAts(); + delete updatedAts[sessionId]; + saveSessionPermissionModeUpdatedAts(updatedAts);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c5b2167 and fecb131cb17439121375d77a3730f9d3d6f35900.
📒 Files selected for processing (2)
sources/sync/storage.tssources/sync/storageTypes.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Runyarn typecheckafter all changes to ensure type safety
Ensure TypeScript strict mode is enabled - all code must be properly typed
Never use Alert module from React Native, always use @sources/modal/index.ts instead
Always apply layout width constraints from @/components/layout to full-screen ScrollViews and content containers for responsive design
Use StyleSheet.create from 'react-native-unistyles' for all styling
Always use function mode in StyleSheet.create when you need theme or runtime access
For Expo Image components, size properties (width, height) must be set as inline styles, not in Unistyles stylesheet, and tintColor must be set directly on the component
Always uset(...)function from @/text for ALL user-visible strings for internationalization
Always use expo-router API, not react-navigation API directly
Use ItemList for most containers for UI, only use custom components or alternatives for custom layouts like chat
Always try to use 'useHappyAction' from @sources/hooks/useHappyAction.ts for async operations instead of handling errors manually
For hotkeys, use 'useGlobalKeyboard' from @sources/hooks/useGlobalKeyboard - do not change it, it only works on Web
Use 'AsyncLock' class for exclusive async locks
Always use 'Avatar' component for displaying avatars
Files:
sources/sync/storage.tssources/sync/storageTypes.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 4 spaces for indentation
Use path alias@/*which maps to./sources/*
Files:
sources/sync/storage.tssources/sync/storageTypes.ts
🔇 Additional comments (6)
sources/sync/storageTypes.ts (2)
24-27: LGTM! Schema extensions align well with the permission sync feature.The new fields correctly extend the metadata schema:
flavoras.nullish()allows flexibility for optional session variantspermissionModeenum values match thePermissionModetype fromPermissionModeSelectorpermissionModeUpdatedAtenables timestamp-based conflict resolution
76-76: LGTM!The
permissionModeUpdatedAtfield correctly mirrors the structure added toMetadataSchemaand supports the local coordination timestamp pattern described in the PR.sources/sync/storage.ts (4)
14-14: LGTM!Import additions for the new persistence functions are correct and complete.
322-356: LGTM! Well-structured permission mode merging with timestamp-based conflict resolution.The merge logic correctly implements the priority chain:
- Existing in-memory state
- Saved from persistence
- Session default
- CLI metadata (only if timestamp is newer)
The timestamp comparison at line 343 properly prevents stale CLI metadata from overwriting more recent local changes.
548-613: LGTM! Clean implementation of permission mode inference from messages.The inference logic correctly:
- Finds the most recent user-text message (leveraging the descending sort order)
- Uses timestamp comparison to prevent older messages from overriding user selections
- Only persists when the mode actually changes
- Updates both
sessionPermissionModesandsessionPermissionModeUpdatedAtsfor consistency
864-894: LGTM! Correct timestamp update on user-initiated permission mode changes.Setting
permissionModeUpdatedAt = Date.now()ensures local user selections are protected from older message-based inference while still allowing newer messages from other devices to take precedence.
leeroybrun
force-pushed
the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
2 times, most recently
from
16dba98 to
e00bbbf
Compare
|
@coderabbitai @codex @gemini-code-assist please review |
|
@leeroybrun, I'll review the changes in this PR right away. ✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to synchronize the permission mode of a session across different devices. The implementation correctly uses an updatedAt timestamp (permissionModeUpdatedAt) to resolve conflicts between local changes and updates from other devices, ensuring the most recent change always wins. New persistence functions for these timestamps are added, and the logic is integrated into session creation, message processing, and manual permission mode updates.
The changes are well-thought-out and address the problem described. I have a couple of suggestions to improve code maintainability by reducing code duplication and centralizing type definitions. Please see my detailed comments.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e00bbbff5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
leeroybrun
force-pushed
the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
from
e00bbbf to
b5ebadc
Compare
|
@codex @coderabbitai @gemini-code-assist please review |
|
@leeroybrun, I'll review the changes in this PR right away. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request effectively implements cross-device synchronization for session permission modes by storing the mode in message metadata and using a server-aligned timestamp to resolve conflicts between local changes and remote updates. The introduction of a centralized PermissionModes.ts constant and a time.ts utility for clock-skew correction are excellent architectural improvements that enhance maintainability and robustness. The core logic for inferring, persisting, and updating the permission mode appears sound. I've left a couple of minor suggestions to improve error handling and code formatting.
leeroybrun
force-pushed
the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
2 times, most recently
from
c80380b to
1a9acb4
Compare
|
@codex @coderabbitai @gemini-code-assist please review the fixes we added/force-pushed to ensure they fixes the issues you reported, and that no new issue exists or was introduced |
|
@leeroybrun, I'll review the force-pushed changes to verify that the reported issues have been fixed and check for any new issues. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism to synchronize session permission modes across devices, addressing several related issues. The core changes involve centralizing the PermissionMode type definition, implementing server-aligned timestamps for consistent ordering, and carefully managing the precedence between user-initiated and inferred permission modes. The refactoring to use a shared PERMISSION_MODES constant and PermissionMode type significantly improves maintainability and type safety throughout the codebase. The addition of permissionModeUpdatedAt is a critical component, ensuring that the most recent permission setting, whether user-defined or inferred from messages, is always respected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9acb4505
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sources/sync/storage.ts
Outdated
| sessions.forEach(session => { | ||
| // Keep a best-effort server time offset for clock-safe local timestamps. | ||
| observeServerTimestamp(session.updatedAt); |
There was a problem hiding this comment.
The new call to observeServerTimestamp(session.updatedAt) uses the session’s last-update time as if it were “current server time”. If a session hasn’t been updated recently (e.g., last message was hours ago), this sets the server offset far into the past, so nowServerMs() (used for permissionModeUpdatedAt) will be earlier than actual recent server messages. In that case, a user toggling permission mode locally right after loading the session can be incorrectly overridden by a newer message that was actually sent before the toggle, because the local timestamp is skewed backward. Consider only updating the offset from timestamps known to be close to “now” (e.g., response headers or realtime message events), or guard against stale updatedAt values.
Useful? React with 👍 / 👎.
leeroybrun
force-pushed
the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
2 times, most recently
from
6e2e747 to
ae1e0db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sources/sync/storage.ts (2)
256-273: Consider adding error handling for persistence failures.If
saveSessionPermissionModesthrows,saveSessionPermissionModeUpdatedAtswon't execute and the in-memory cache won't be updated, potentially leaving the state inconsistent between modes and timestamps.♻️ Suggested improvement
const persistSessionPermissionData = (sessions: Record<string, Session>) => { const allModes: Record<string, PermissionMode> = {}; const allUpdatedAts: Record<string, number> = {}; Object.entries(sessions).forEach(([id, sess]) => { if (sess.permissionMode && sess.permissionMode !== 'default') { allModes[id] = sess.permissionMode; } if (typeof sess.permissionModeUpdatedAt === 'number') { allUpdatedAts[id] = sess.permissionModeUpdatedAt; } }); - saveSessionPermissionModes(allModes); - saveSessionPermissionModeUpdatedAts(allUpdatedAts); - sessionPermissionModes = allModes; - sessionPermissionModeUpdatedAts = allUpdatedAts; + try { + saveSessionPermissionModes(allModes); + saveSessionPermissionModeUpdatedAts(allUpdatedAts); + sessionPermissionModes = allModes; + sessionPermissionModeUpdatedAts = allUpdatedAts; + } catch (e) { + console.error('Failed to persist session permission data:', e); + } };
576-577: Consider validating the inferred permission mode.The type assertion
as PermissionMode | undefinedassumes the stored value is valid. If malformed data exists, this could propagate an invalid mode.♻️ Suggested validation
+import { PERMISSION_MODES } from '@/constants/PermissionModes'; + for (const message of messagesArray) { if (message.kind !== 'user-text') continue; - const mode = message.meta?.permissionMode as PermissionMode | undefined; - if (!mode) continue; + const rawMode = message.meta?.permissionMode; + if (!rawMode || !PERMISSION_MODES.includes(rawMode as any)) continue; + const mode = rawMode as PermissionMode; inferredPermissionMode = mode; inferredPermissionModeAt = message.createdAt; break; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6e2e747f8ec2dcb2a9968d53eb95f70bb3f41cda and ae1e0db13dbb4bc92fa0b412462499b5025f6a00.
📒 Files selected for processing (12)
sources/-session/SessionView.tsxsources/components/PermissionModeSelector.tsxsources/constants/PermissionModes.tssources/sync/apiSocket.tssources/sync/persistence.tssources/sync/settings.tssources/sync/storage.tssources/sync/storageTypes.tssources/sync/sync.tssources/sync/time.tssources/sync/typesMessageMeta.tssources/sync/typesRaw.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- sources/sync/time.ts
- sources/sync/sync.ts
- sources/components/PermissionModeSelector.tsx
- sources/-session/SessionView.tsx
- sources/constants/PermissionModes.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Runyarn typecheckafter all changes to ensure type safety
Ensure TypeScript strict mode is enabled - all code must be properly typed
Never use Alert module from React Native, always use @sources/modal/index.ts instead
Always apply layout width constraints from @/components/layout to full-screen ScrollViews and content containers for responsive design
Use StyleSheet.create from 'react-native-unistyles' for all styling
Always use function mode in StyleSheet.create when you need theme or runtime access
For Expo Image components, size properties (width, height) must be set as inline styles, not in Unistyles stylesheet, and tintColor must be set directly on the component
Always uset(...)function from @/text for ALL user-visible strings for internationalization
Always use expo-router API, not react-navigation API directly
Use ItemList for most containers for UI, only use custom components or alternatives for custom layouts like chat
Always try to use 'useHappyAction' from @sources/hooks/useHappyAction.ts for async operations instead of handling errors manually
For hotkeys, use 'useGlobalKeyboard' from @sources/hooks/useGlobalKeyboard - do not change it, it only works on Web
Use 'AsyncLock' class for exclusive async locks
Always use 'Avatar' component for displaying avatars
Files:
sources/sync/typesRaw.tssources/sync/typesMessageMeta.tssources/sync/settings.tssources/sync/storage.tssources/sync/storageTypes.tssources/sync/apiSocket.tssources/sync/persistence.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 4 spaces for indentation
Use path alias@/*which maps to./sources/*
Files:
sources/sync/typesRaw.tssources/sync/typesMessageMeta.tssources/sync/settings.tssources/sync/storage.tssources/sync/storageTypes.tssources/sync/apiSocket.tssources/sync/persistence.ts
🧬 Code graph analysis (4)
sources/sync/typesRaw.ts (1)
sources/constants/PermissionModes.ts (1)
PERMISSION_MODES(1-9)
sources/sync/typesMessageMeta.ts (1)
sources/constants/PermissionModes.ts (1)
PERMISSION_MODES(1-9)
sources/sync/settings.ts (1)
sources/constants/PermissionModes.ts (1)
PERMISSION_MODES(1-9)
sources/sync/apiSocket.ts (1)
sources/sync/time.ts (1)
observeServerTimestamp(3-8)
🔇 Additional comments (20)
sources/sync/apiSocket.ts (2)
4-4: LGTM!Import added for the server time calibration utility.
184-203: LGTM! Server time calibration via HTTP Date header.This best-effort approach correctly:
- Awaits the fetch before extracting headers
- Validates the parsed timestamp with
Number.isNaN- Silently handles failures to avoid disrupting the request flow
The HTTP
Dateheader is a reliable source for server time alignment, avoiding the pitfall of deriving time from potentially stale resource timestamps.sources/sync/typesRaw.ts (2)
3-3: LGTM!Centralizes permission mode enum source for consistency with the rest of the codebase.
56-56: LGTM! Schema expansion is backward compatible.The permission modes now align with the centralized
PERMISSION_MODESconstant, expanding from 3 to 7 valid values. Existing persisted data with the original modes remains valid.sources/sync/settings.ts (2)
2-2: LGTM!Import aligns with the centralized permission mode definition.
120-120: LGTM!Consistent with the PR's goal of centralizing permission mode definitions. The
AIBackendProfileschema now validates against the same source of truth used across the codebase.sources/sync/persistence.ts (2)
6-6: LGTM!Import path updated to the centralized constants module.
191-217: LGTM! Defensive persistence helpers for permission mode timestamps.The implementation correctly:
- Validates the parsed object structure (non-null, non-array object)
- Filters to only finite numeric values, preventing invalid timestamps from propagating
- Follows the established error-handling pattern used by other persistence functions in this file
sources/sync/typesMessageMeta.ts (2)
2-2: LGTM!Import aligns with the centralized permission mode definition used throughout the PR.
7-7: LGTM!This change is essential for the PR's objective—storing
permissionModein message metadata enables inferring the session's permission mode from the most recent user message when syncing across devices.sources/sync/storageTypes.ts (3)
2-3: LGTM!Clean imports using type-only import for
PermissionModeand regular import for thePERMISSION_MODESconstant array. This separation is correct since the constant is needed at runtime for the Zod schema while the type is only needed for TypeScript compilation.
26-29: LGTM!The schema additions are well-structured:
permissionModeusesz.enum(PERMISSION_MODES)which properly validates against the allowed values.permissionModeUpdatedAtusesz.number()for timestamp tracking.- Both fields are optional, ensuring backward compatibility with existing data.
77-78: LGTM!The Session interface fields correctly mirror the schema additions with appropriate nullable types (
PermissionMode | nullandnumber | null). The inline comments clearly document that these are local-only fields not synced to the server, which is important for understanding the synchronization model.sources/sync/storage.ts (7)
14-15: LGTM!New imports are correctly organized:
- Persistence functions for loading/saving permission mode timestamps
- Type-only import for
PermissionModenowServerMsfor server-aligned timestampsAlso applies to: 24-24
328-377: LGTM!The merge logic correctly prioritizes permission mode sources:
- Existing in-memory state (preserves current session state)
- Saved persistence data (initial load)
- Session data from server
- Default fallback
The metadata-based override (lines 362-368) correctly applies CLI-published permission modes only when the timestamp is newer than local state. This enables seamless handoff between devices while respecting local user selections.
569-581: LGTM!The inference logic correctly scans messages in reverse chronological order (newest first) and extracts the permission mode from the most recent user message. The early
breakensures efficiency by stopping at the first match.
589-624: LGTM!The timestamp comparison logic is sound:
canInferPermissionModegates on the inferred timestamp being newer than the localpermissionModeUpdatedAtshouldWritePermissionModeadds an additional check to avoid no-op updates- Persistence is correctly called only when the mode changes
The inline comments clearly document the timestamp coordination strategy between server timestamps and
nowServerMs().
871-895: LGTM!The implementation correctly:
- Uses the typed
PermissionModeparameter- Stamps
permissionModeUpdatedAtwithnowServerMs()to coordinate with server timestamps- Persists the updated state immediately
The timestamp marking ensures user-initiated mode changes are protected from being overridden by older message-based inference, while still allowing newer messages to take precedence.
1020-1025: LGTM!The cleanup correctly removes
permissionModeUpdatedAtdata when a session is deleted, following the same pattern used for permission modes. Both persistence and in-memory cache are updated to maintain consistency.
120-120: LGTM!Interface signature correctly updated to use the typed
PermissionModeinstead of a string union, maintaining type safety across the codebase.
leeroybrun
force-pushed
the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
from
ae1e0db to
6409cbe
Compare
|
@codex @coderabbitai @gemini-code-assist please review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@leeroybrun, I'll review the force-pushed changes to verify that the reported issues have been resolved and check for any new issues or regressions introduced by the fixes. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request effectively implements cross-device synchronization for session permission modes by storing the mode in message metadata and using timestamps for conflict resolution. The changes are well-structured, introducing a centralized PermissionModes constant, a time synchronization utility, and updating the storage logic to handle the new state. The logic for inferring modes from messages and respecting local user changes via timestamps is sound. I have one suggestion to improve the robustness of data cleanup in the storage layer. Overall, this is a solid implementation of the feature.
| sessionPermissionModes = modes; | ||
|
|
||
| const updatedAts = loadSessionPermissionModeUpdatedAts(); | ||
| delete updatedAts[sessionId]; | ||
| saveSessionPermissionModeUpdatedAts(updatedAts); | ||
| sessionPermissionModeUpdatedAts = updatedAts; |
There was a problem hiding this comment.
The pattern used here to clear persisted data for both modes and updatedAts can be simplified and made more robust. Instead of re-loading from storage, you can modify the in-memory cache variables (sessionPermissionModes and sessionPermissionModeUpdatedAts) directly and then save them. This avoids unnecessary disk reads and is safer against potential race conditions if persistence were to become asynchronous.
Applying this improved pattern would change lines 1022-1030 to something like this:
// In deleteSession...
delete sessionPermissionModes[sessionId];
saveSessionPermissionModes(sessionPermissionModes);
delete sessionPermissionModeUpdatedAts[sessionId];
saveSessionPermissionModeUpdatedAts(sessionPermissionModeUpdatedAts);Since this change spans existing code, I'm providing it as an example rather than a direct suggestion.
feat: support CLAUDE_CONFIG_DIR environment variable
leeroybrun
deleted the
slopus/pr/permission-mode-from-last-message-2026-01-13
branch
1 participant
Summary
Permission mode (yolo/bypassPermissions/etc) is now preserved when switching between devices. The app infers the permission mode from the most recent session's user message metadata, ensuring consistent behavior across mobile and desktop.
How it works:
Fixes
Related
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.