Fix Android compatibility and add server persistence #1
Conversation
- Added an android package name to allow installation - Removed DynamicColorIOS which prevented from running - Fixed the bottom icons and top bar margin - Save last connected server & auto-connect to it
WalkthroughThis pull request introduces persistent server URL storage via AsyncStorage, simplifies the app's connection initialization flow by removing on-mount auto-connect, adds platform-specific UI adjustments for padding and icon rendering, and includes Android package configuration. Changes
Sequence DiagramsequenceDiagram
participant App as App (Mount)
participant Provider as OpenCodeProvider
participant Storage as AsyncStorage
participant Connection as Connection Logic
participant Result as State Update
App->>Provider: Initialize with context
activate Provider
Note over Provider: Mount effect triggered
Provider->>Storage: Load SERVER_URL_KEY
activate Storage
Storage-->>Provider: Return saved URL (or null)
deactivate Storage
alt Saved URL exists
Provider->>Provider: Set serverUrl from storage
Note over Provider: Generate connectionId
Provider->>Connection: Invoke connect()
activate Connection
Note over Connection: Establish connection<br/>(max 5s timeout)
Connection-->>Provider: Success/Failure
deactivate Connection
Provider->>Provider: Guard: connectionId still valid?
alt Valid & successful
Provider->>Result: Update connected = true
else Stale or failed
Provider->>Result: Connection state unchanged
end
else No saved URL
Provider->>Result: Use default/current serverUrl
end
deactivate Provider
Result-->>App: Redirect based on connected state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/screens/SessionsScreen.tsx (1)
188-189: Duplicate padding logic should be consolidated.This is an exact duplicate of the padding calculation in
SettingsScreen.tsx(lines 27-28). Please refer to the refactoring suggestion in that file to extract this shared logic to a common utility function with a named constant for the magic number60.
🧹 Nitpick comments (6)
app/index.tsx (1)
4-11: Potential flash to connect screen during initial load.The component redirects to
/connectwheneverconnectedis false, which includes during the initialconnectingphase. This may cause a brief flash of the connect screen before auto-connect succeeds from the persisted URL.Consider returning
nullor a loading state whileconnectingis true to prevent the redirect during initial connection attempts:🔎 Suggested improvement
export default function Index() { - const { connected } = useOpenCode(); + const { connected, connecting } = useOpenCode(); + + // Wait for initial connection attempt to complete + if (connecting) { + return null; + } if (connected) { return <Redirect href="/(tabs)/sessions" />; } return <Redirect href="/connect" />; }app/_layout.tsx (1)
9-9: Remove unusedconnectfrom destructuring.The
connectfunction is destructured but never used in this component. It appears to be leftover from the removed auto-connect logic.🔎 Proposed fix
function AuthRedirect({ children }: { children: React.ReactNode }) { - const { connected, connecting, connect } = useOpenCode(); + const { connected, connecting } = useOpenCode();app/(tabs)/_layout.tsx (1)
9-15: Multipleas anytype assertions suppress type checking.The type assertions on
labelColorandtintColorwork around type mismatches with the unstable API. While functional, consider adding a comment explaining why these casts are necessary, so future maintainers understand the workaround.+ // Type casts required due to expo-router/unstable-native-tabs API type definitions + // iOS uses dynamic color objects, Android uses string values const labelColor = Platform.OS === 'ios' ? { color: { dark: 'white', light: 'black' } as any } : isDark ? 'white' : 'black';src/providers/OpenCodeProvider.tsx (3)
132-137: Fire-and-forget AsyncStorage write may silently fail.The
AsyncStorage.setItemcall is not awaited, and errors are only logged. While this is acceptable for non-critical persistence, consider whether failed saves should surface to the user or trigger a retry.For a more robust approach, you could make
setServerUrlasync and propagate errors:🔎 Alternative approach (optional)
- const setServerUrl = useCallback((url: string) => { + const setServerUrl = useCallback(async (url: string) => { setServerUrlState(url); - AsyncStorage.setItem(SERVER_URL_KEY, url).catch(err => { + try { + await AsyncStorage.setItem(SERVER_URL_KEY, url); + } catch (err) { console.error('Failed to save server URL to storage:', err); - }); + } }, []);
233-251: Missing dependencies in useEffect may trigger lint warnings.The empty dependency array
[]is intentional for running once on mount, but ESLint'sreact-hooks/exhaustive-depsrule will flagconnectandserverUrlas missing dependencies.Consider adding an eslint-disable comment to document the intentional behavior:
🔎 Suggested documentation
// Load saved URL and auto-connect on mount useEffect(() => { const init = async () => { try { const savedUrl = await AsyncStorage.getItem(SERVER_URL_KEY); if (savedUrl) { setServerUrlState(savedUrl); // Silently try to connect connect(savedUrl); } else { connect(serverUrl); } } catch (err) { // Fallback to default connect(serverUrl); } }; init(); + // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally run once on mount with initial values }, []);
231-231: Consider addingsetServerUrlto connect dependencies.While
setServerUrlhas a stable identity (empty dependency array), including it in the dependencies makes the code more explicit and consistent with exhaustive-deps rules:- }, [serverUrl]); + }, [serverUrl, setServerUrl]);This is optional since the current code works correctly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app.json(1 hunks)app/(tabs)/_layout.tsx(1 hunks)app/_layout.tsx(2 hunks)app/index.tsx(1 hunks)package.json(1 hunks)src/providers/OpenCodeProvider.tsx(4 hunks)src/screens/SessionsScreen.tsx(2 hunks)src/screens/SettingsScreen.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/screens/SessionsScreen.tsx (1)
src/theme/index.ts (1)
spacing(85-93)
app/(tabs)/_layout.tsx (2)
src/hooks/useTheme.ts (1)
useTheme(5-13)src/components/Icon.tsx (1)
Icon(135-150)
src/screens/SettingsScreen.tsx (1)
src/theme/index.ts (1)
spacing(85-93)
app/index.tsx (1)
src/providers/OpenCodeProvider.tsx (1)
useOpenCode(654-660)
🔇 Additional comments (8)
src/screens/SettingsScreen.tsx (1)
8-8: LGTM! Platform import enables cross-platform compatibility.The Platform API import is correctly used to apply platform-specific padding adjustments for Android compatibility.
src/screens/SessionsScreen.tsx (1)
9-9: LGTM! Platform import enables cross-platform compatibility.The Platform API import is correctly used to apply platform-specific padding adjustments for Android compatibility.
app.json (1)
20-21: LGTM!The Android package name is correctly configured and matches the iOS bundle identifier, ensuring consistent app identification across platforms.
app/_layout.tsx (1)
13-27: LGTM on the redirect logic.The early return when
connectingis true correctly prevents premature redirects during the initial connection attempt. The redirect conditions properly protect authenticated routes.Note: If you remove the unused
connectfrom destructuring per the previous comment, also remove it from the dependency array on line 27.app/(tabs)/_layout.tsx (2)
2-2: Using unstable API from expo-router.The import from
expo-router/unstable-native-tabsindicates this is an experimental API that may have breaking changes. Ensure this is intentional and monitor expo-router releases for stability updates.
28-42: Cross-platform icon implementation looks good.The pattern of providing both
sf(iOS SF Symbols) andsrc(VectorIcon fallback) ensures icons render correctly across platforms. This aligns with the PR objective to replace iOS-only SF Symbols with cross-platform Ionicons.src/providers/OpenCodeProvider.tsx (1)
189-230: Good race condition and timeout handling.The
connectionIdRefpattern correctly guards against stale connection results when multiple connection attempts occur. The 5-second timeout prevents hanging on unresponsive servers.Minor note: The timeout Promise is not cleaned up when the connection succeeds first, but this is negligible since it only holds a reference briefly.
package.json (1)
14-14: AsyncStorage version 2.2.0 is the current stable release.No action needed. This is the latest available version and there are no known security vulnerabilities. The exact version pin ensures reproducibility and is appropriate for Expo SDK 54 compatibility.
| // Extra padding for the floating liquid glass tab bar on iOS/iPad | ||
| const topPadding = Platform.OS === 'ios' ? insets.top + 60 : insets.top + spacing.lg; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated padding logic to a shared constant.
This exact padding calculation is duplicated in SessionsScreen.tsx (lines 188-189). The magic number 60 lacks explanation and should be extracted to a named constant in the theme or a utility module.
🔎 Proposed refactor to eliminate duplication
Add to src/theme/index.ts or create a new src/utils/layout.ts:
import { Platform } from 'react-native';
import { EdgeInsets } from 'react-native-safe-area-context';
import { spacing } from '../theme';
// Height of the floating tab bar on iOS
const IOS_TAB_BAR_OFFSET = 60;
export function getHeaderTopPadding(insets: EdgeInsets): number {
return Platform.OS === 'ios'
? insets.top + IOS_TAB_BAR_OFFSET
: insets.top + spacing.lg;
}Then in both SettingsScreen.tsx and SessionsScreen.tsx:
- // Extra padding for the floating liquid glass tab bar on iOS/iPad
- const topPadding = Platform.OS === 'ios' ? insets.top + 60 : insets.top + spacing.lg;
+ const topPadding = getHeaderTopPadding(insets);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/screens/SettingsScreen.tsx around lines 27-28, the topPadding calculation
duplicates logic found in SessionsScreen and uses the magic number 60; extract
this into a shared constant/function (e.g., add IOS_TAB_BAR_OFFSET = 60 and a
getHeaderTopPadding(insets) util in src/theme/index.ts or src/utils/layout.ts)
and replace the inline expression with a call to that helper in both
SettingsScreen.tsx and SessionsScreen.tsx so the platform check, iOS offset, and
spacing.lg usage are centralized and documented.
1 participant
Summary
DynamicColorIOS, adding standard package naming, and adjusting header margins.Ioniconsfor tab navigation.AsyncStorageand added connection race-condition handling.Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.