Mobile#1113
Open
Ashraf-Ali-aa wants to merge 12 commits into
Open
Conversation
|
Too many files changed for review. ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds an Expo mobile workspace with pairing, WebSocket session chat, offline queueing, markdown rendering, app shell screens, mobile tooling and docs, conditional CI checks, and desktop WebSocket tests for mobile token authentication and parameterized token routes. ChangesMobile app workspace
Desktop WebSocket auth tests
Sequence Diagram(s)sequenceDiagram
participant User
participant PairScreen
participant SecureStore
participant useMaestroWebSocket
participant SessionsContext
participant useSessionChat
User->>PairScreen: scan QR or submit manual code
PairScreen->>SecureStore: store credentials
PairScreen->>useMaestroWebSocket: connect()
useMaestroWebSocket->>SessionsContext: emit authenticated and session events
SessionsContext->>useSessionChat: deliver output/state/tool/input events
useSessionChat->>User: render chat messages and streaming state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 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 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.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (28)
apps/mobile/src/app/(settings)/_layout.tsx-43-55 (1)
43-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegal/help menu actions are currently non-functional.
Lines 43-55 render policy/support items without any action, so users cannot actually open these destinations. Please wire each item to its target (e.g., external URL or in-app route), especially for policy entries.
🤖 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 `@apps/mobile/src/app/`(settings)/_layout.tsx around lines 43 - 55, The Stack.Toolbar.MenuAction components for Acceptable Use Policy, Consumer Terms, Privacy Policy, and Help & Support are rendered without any onPress handlers or navigation logic. Add onPress handlers to each of these menu action items that navigate to the appropriate destinations, whether that be external URLs (using linking APIs) for policy documents or in-app routes for help content. Ensure each menu item triggers the correct navigation behavior when tapped by the user.apps/mobile/src/utils/use-system-background-color.ts-6-9 (1)
6-9:⚠️ Potential issue | 🟠 MajorGuard background color before invoking System UI API.
The double-cast bypasses type safety and allows
useCSSVariable()to returnundefinedor numeric values, which gets coerced to a string. Whileundefinedis technically valid (clears the background), invalid color strings will cause unhandled Promise rejections during theme transitions. Validate the value before calling the async function.Suggested fix
export function useSystemBackgroundColor() { const color = useCSSVariable('--app-background'); useEffect(() => { - SystemUI.setBackgroundColorAsync(color as unknown as string); + if (typeof color !== 'string' || color.length === 0) return; + void SystemUI.setBackgroundColorAsync(color); }, [color]); }🤖 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 `@apps/mobile/src/utils/use-system-background-color.ts` around lines 6 - 9, The code in the useEffect hook uses a double-cast (as unknown as string) to bypass type safety when passing the color value to SystemUI.setBackgroundColorAsync(), which allows invalid values like undefined or numeric coercions to reach the async function and cause unhandled Promise rejections. Remove the type coercion and add validation to guard the SystemUI.setBackgroundColorAsync call by checking that the color value returned from useCSSVariable is a valid non-empty string before invoking the async function.apps/mobile/src/app/(settings)/profile.tsx-35-69 (1)
35-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire the profile action buttons to handlers (or disable them until implemented).
Line 35, Line 59, and Line 67 render tappable actions with no
onPress, so all primary actions are no-ops on a reachable settings screen.Suggested fix
export default function ProfileScreen() { const [fullName, setFullName] = useState('Evan Bacon'); const [nickname, setNickname] = useState('Evan'); const [preferences, setPreferences] = useState("I'm a creator and software developer."); + const handleUpdateProfile = () => { + // TODO: wire profile update mutation + }; + const handleSavePreferences = () => { + // TODO: wire preferences save mutation + }; + const handleDeleteAccount = () => { + // TODO: show confirm dialog + destructive delete flow + }; return ( @@ - <Pressable className="bg-foreground rounded-xl mt-6 py-3.5 items-center active:opacity-80 border-continuous"> + <Pressable + onPress={handleUpdateProfile} + className="bg-foreground rounded-xl mt-6 py-3.5 items-center active:opacity-80 border-continuous" + > @@ - <Pressable className="bg-muted rounded-xl mt-4 py-3.5 items-center active:opacity-80 border-continuous"> + <Pressable + onPress={handleSavePreferences} + className="bg-muted rounded-xl mt-4 py-3.5 items-center active:opacity-80 border-continuous" + > @@ - <Pressable className="flex-row items-center gap-2 active:opacity-60"> + <Pressable onPress={handleDeleteAccount} className="flex-row items-center gap-2 active:opacity-60">🤖 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 `@apps/mobile/src/app/`(settings)/profile.tsx around lines 35 - 69, The three Pressable buttons in the profile screen (the "Update Profile" button, the "Save Preferences" button, and the "Delete account" button) are missing onPress event handlers, making them non-functional. Add onPress handlers to each Pressable component that reference their corresponding handler functions, or alternatively disable these buttons by adding the disabled prop and setting their opacity/styling to indicate a disabled state until the handlers are implemented. Ensure all interactive elements on this reachable settings screen are either fully wired to their functionality or clearly marked as disabled.apps/mobile/src/components/markdown/chat-markdown.tsx-185-188 (1)
185-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent fallback link open after custom
onPress.This expression calls
Linking.openURLwheneverextras.onPressreturnsvoid, so links can open twice and wiki links can bypass the toast-only path.💡 Proposed fix
- const onPress = () => extras?.onPress?.(node.url) ?? Linking.openURL(node.url); + const onPress = () => { + if (!node.url) return; + if (extras?.onPress) { + extras.onPress(node.url); + return; + } + Linking.openURL(node.url); + };🤖 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 `@apps/mobile/src/components/markdown/chat-markdown.tsx` around lines 185 - 188, The onPress arrow function in the link handler uses the nullish coalescing operator which causes Linking.openURL to be called whenever extras.onPress returns undefined (void), resulting in duplicate link opens and wiki links bypassing the custom handler. Replace the conditional logic in the onPress function to check if extras?.onPress exists as a function before calling it, and only fall back to Linking.openURL if the custom handler is not provided, rather than relying on the return value of the custom handler to determine whether to invoke the fallback.apps/mobile/src/components/chat/prompt-input.web.tsx-16-26 (1)
16-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PromptInputActionchildren are parsed but never rendered.
actionsis built inPromptInput, but the footer inPromptInputBodyuses hardcoded controls and ignores those parsed children. That drops caller-provided action buttons.Proposed patch
-import { Children, type ReactNode, isValidElement } from 'react'; +import { Children, cloneElement, type ReactNode, isValidElement } from 'react'; @@ export function PromptInput({ children }: { children: ReactNode }) { @@ return ( @@ - <View className="flex w-full flex-col rounded-2xl border border-border/30 bg-card/70 shadow-composer transition-shadow duration-300 focus-within:shadow-composer-focus"> - {body} + <View className="flex w-full flex-col rounded-2xl border border-border/30 bg-card/70 shadow-composer transition-shadow duration-300 focus-within:shadow-composer-focus"> + {isValidElement(body) ? cloneElement(body, { actions }) : body} </View> </View> ); } @@ -export function PromptInputBody({ children }: { children: ReactNode }) { +export function PromptInputBody({ + children, + actions = [], +}: { + children: ReactNode; + actions?: ReactNode[]; +}) { @@ - <View className="flex flex-row items-center gap-1"> - {/* Attachments button */} - <Pressable className="flex h-7 w-7 items-center justify-center rounded-lg border border-border/40 transition-colors hover:bg-accent"> - <Paperclip size={14} className="text-muted-foreground" /> - </Pressable> - {/* Model selector mock */} - <Pressable className="flex h-7 flex-row items-center gap-1.5 rounded-lg px-2 transition-colors hover:bg-accent"> - <Text className="text-[12px] text-muted-foreground">Opus</Text> - </Pressable> - </View> + <View className="flex flex-row items-center gap-1"> + {actions.length > 0 ? ( + actions + ) : ( + <> + <Pressable className="flex h-7 w-7 items-center justify-center rounded-lg border border-border/40 transition-colors hover:bg-accent"> + <Paperclip size={14} className="text-muted-foreground" /> + </Pressable> + <Pressable className="flex h-7 flex-row items-center gap-1.5 rounded-lg px-2 transition-colors hover:bg-accent"> + <Text className="text-[12px] text-muted-foreground">Opus</Text> + </Pressable> + </> + )} + </View>Also applies to: 82-93
🤖 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 `@apps/mobile/src/components/chat/prompt-input.web.tsx` around lines 16 - 26, The `actions` array is being parsed from children in the `PromptInput` component but is never passed to or rendered by the footer/action controls section. Locate where the footer controls are rendered (likely in the `PromptInputBody` component or around lines 82-93) and modify it to accept the `actions` array as a prop or through context. Replace the hardcoded action button controls in that footer rendering section with code that renders the items from the `actions` array, ensuring that caller-provided `PromptInputAction` children are actually displayed instead of being discarded.apps/mobile/src/components/blur-raw.tsx-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorReplace internal import with the public
expo-blurAPI.
expo-blur/build/NativeBlurModuleis not a supported public API in Expo SDK 56. ImportBlurViewdirectly from theexpo-blurpackage instead:import { BlurView } from 'expo-blur'. Internal paths underbuild/are private implementation details and are subject to change without notice.🤖 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 `@apps/mobile/src/components/blur-raw.tsx` at line 6, Replace the import statement in blur-raw.tsx that currently imports NativeBlurView from the private internal path 'expo-blur/build/NativeBlurModule' with the public API by importing BlurView directly from 'expo-blur' instead. Update any references to NativeBlurView throughout the file to use BlurView to ensure the component works with the supported public API.apps/mobile/src/components/chat/prompt-input.tsx-174-186 (1)
174-186: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace function-form
styleonPressablewithclassName+ static style.This uses the forbidden
style={({ pressed }) => ...}pattern in mobile code. Move pressed styling toactive:utilities and keepstylestatic.Proposed patch
<Pressable - style={({ pressed }) => ({ - width: 34, - height: 34, - borderRadius: 17, - borderCurve: 'continuous', - justifyContent: 'center', - alignItems: 'center', - opacity: pressed ? 0.7 : 1, - margin: 5, - backgroundColor: disabled ? undefined : accentColor, - })} - className={disabled ? 'bg-secondary' : undefined} + style={{ + width: 34, + height: 34, + borderRadius: 17, + borderCurve: 'continuous', + justifyContent: 'center', + alignItems: 'center', + margin: 5, + backgroundColor: disabled ? undefined : accentColor, + }} + className={cn('active:opacity-70', disabled && 'bg-secondary')} onPress={onSend} disabled={disabled} >As per coding guidelines, “Do NOT use the function form of
styleonPressablecomponents… useclassNamewith theactive:modifier.”🤖 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 `@apps/mobile/src/components/chat/prompt-input.tsx` around lines 174 - 186, The Pressable component in prompt-input.tsx uses the forbidden function-form style pattern with the pressed parameter. Convert this to use static styling instead by moving all the current style properties to a static style object, and then use className with the active: Tailwind modifier to handle the opacity change when pressed. Specifically, remove the function form style={({ pressed }) => ...} pattern, make style a regular object with all the static properties, and add the active:opacity-70 utility class to the className to replace the conditional opacity styling based on the pressed state.Source: Coding guidelines
apps/mobile/src/components/markdown/render-rules.tsx-145-147 (1)
145-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the link fallback so URLs are actually opened.
onPresscurrently returns a fallback function instead of invoking it, so links withoutextras.onPresswon’t open.Proposed fix
- const onPress = () => extras?.onPress(node.url) || (() => Linking.openURL(node.url)); + const onPress = () => { + if (extras?.onPress) { + extras.onPress(node.url); + return; + } + Linking.openURL(node.url); + };🤖 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 `@apps/mobile/src/components/markdown/render-rules.tsx` around lines 145 - 147, The onPress function in the link render rule is returning a function instead of invoking it as a fallback. When extras?.onPress does not exist or is falsy, the current code returns a function definition (() => Linking.openURL(node.url)) rather than calling Linking.openURL(node.url) directly. Fix the onPress function to ensure that when extras?.onPress is not available, Linking.openURL(node.url) is actually executed, not returned as a function reference.apps/mobile/src/components/touchable-glass.tsx-82-86 (1)
82-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double-calling
onPressOutin fallback tap success path.
onTouchEndSuccesscallsonPressOut, andTouchableWithoutFeedback.onPressOutalso calls it viaonTouchEnd, so successful taps can invokeonPressOuttwice.Proposed fix
const onTouchEndSuccess = () => { setPressed(false); onPress?.(); - onPressOut?.(); };Also applies to: 90-95
🤖 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 `@apps/mobile/src/components/touchable-glass.tsx` around lines 82 - 86, The onTouchEndSuccess function is calling onPressOut?.() explicitly, but since this is a fallback handler for onTouchEnd which is already handled by TouchableWithoutFeedback.onPressOut, the callback gets invoked twice for successful taps. Remove the explicit onPressOut?.() call from the onTouchEndSuccess function, allowing the onPressOut callback to be handled only once through the standard TouchableWithoutFeedback callback chain. Apply the same fix to any similar functions in the range of lines 90-95 that may have the same issue.apps/mobile/src/components/touchable-glass.tsx-49-60 (1)
49-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback path drops touch-target props used by consumers.
PromptInputActionpasseshitSlopintoTouchableGlass(apps/mobile/src/components/chat/prompt-input.tsx, Line 63-77), but fallback strips props intoAnimated.Viewand never forwards touch props toTouchableWithoutFeedback. On non-liquid-glass devices, this shrinks the actionable hit target.Proposed fix
function TouchableGlassFallback({ onPress, onPressIn, onPressOut, ref, disabled, children, style, className, ...rest }: TouchableGlassProps) { @@ - const safeViewProps = viewProps as ViewOnlyProps; + const safeViewProps = viewProps as ViewOnlyProps & { hitSlop?: ViewProps['hitSlop'] }; + const { hitSlop, ...viewOnlyProps } = safeViewProps; @@ return ( <TouchableWithoutFeedback className="contents" onPress={onTouchEndSuccess} onPressIn={onTouchBegin} onPressOut={onTouchEnd} disabled={disabled} + hitSlop={hitSlop} > <Animated.View ref={ref} className={className} - {...safeViewProps} + {...viewOnlyProps}Also applies to: 90-101
🤖 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 `@apps/mobile/src/components/touchable-glass.tsx` around lines 49 - 60, The TouchableGlassFallback function accepts touch-target props like hitSlop from consumers but fails to forward them to the TouchableWithoutFeedback component, causing the actionable hit target to shrink on non-liquid-glass devices. Extract touch-target props (such as hitSlop and other touch-related properties) from the rest parameter and explicitly pass them to the TouchableWithoutFeedback component to ensure touch targets are properly sized when the fallback path is used.apps/mobile/src/components/drawer-content.tsx-255-261 (1)
255-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent double navigation when opening Settings on Android.
This handler calls both
onNavigateandonOpenModalon Android, causing two route actions for a single press.Proposed fix
onPress={() => { - if (process.env.EXPO_OS === 'android') { - onNavigate('/(settings)/settings'); - } - onOpenModal('/(settings)/settings'); + if (process.env.EXPO_OS === 'android') { + onNavigate('/(settings)/settings'); + return; + } + onOpenModal('/(settings)/settings'); }}🤖 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 `@apps/mobile/src/components/drawer-content.tsx` around lines 255 - 261, The Settings button handler is calling both onNavigate and onOpenModal on Android, which causes double navigation. Restructure the conditional logic so that only one of these functions is executed per platform. On Android, call only onNavigate with '/(settings)/settings', and for other platforms, call only onOpenModal with the same route. Use an if-else statement to ensure mutually exclusive execution of these navigation functions.apps/mobile/src/components/tw.tsx-29-46 (1)
29-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback glass view drops incoming view props.
FallbackAppleGlassViewignores...rest, so when liquid glass is unavailable, callers lose non-style props (accessibility/layout/handlers), creating behavior drift versus the primary implementation.Proposed fix
const FallbackAppleGlassView = ({ fallbackTint, fallbackIntensity, children, style, className, ...rest }: FallbackAppleGlassViewProps) => { + const { + animatedProps, + glassEffectStyle, + tintColor, + isInteractive, + colorScheme, + ...viewProps + } = rest as Record<string, unknown>; return ( <BlurView className={className} style={[{ overflow: 'hidden' }, StyleSheet.flatten(style) as ViewStyle]} tint={fallbackTint} intensity={fallbackIntensity} + {...(viewProps as Record<string, unknown>)} > {children as React.ReactNode} </BlurView> ); };🤖 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 `@apps/mobile/src/components/tw.tsx` around lines 29 - 46, The FallbackAppleGlassView component extracts the rest props in its destructuring but never passes them to the BlurView component, causing accessibility attributes, layout props, and event handlers to be dropped. Spread the ...rest props onto the BlurView component alongside the existing className, style, tint, and intensity props to ensure all incoming props are properly forwarded and the fallback implementation maintains consistent behavior with the primary glass view.apps/mobile/src/app/attachments.tsx-117-122 (1)
117-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCamera/Photos actions are unintentionally disabled on Android.
At Line 117-122,
onPressis gated byIS_IOS, so Android users see buttons that do nothing. Since the handlers already useexpo-image-pickerpermission APIs, this should be wired for supported non-iOS targets too (or visually disabled/hidden).Suggested patch
- <AttachmentButton icon={Camera} label="Camera" onPress={IS_IOS ? openCamera : undefined} /> + <AttachmentButton icon={Camera} label="Camera" onPress={openCamera} /> <AttachmentButton icon={ImageIcon} label="Photos" - onPress={IS_IOS ? openPhotos : undefined} + onPress={openPhotos} />🤖 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 `@apps/mobile/src/app/attachments.tsx` around lines 117 - 122, The AttachmentButton components for Camera and Photos have their onPress handlers conditionally gated by IS_IOS, which disables these buttons on Android. Since the openCamera and openPhotos handlers already support Android via expo-image-picker permission APIs, remove or modify the IS_IOS conditional checks on both the Camera button's onPress and the ImageIcon Photos button's onPress to enable functionality on Android as well. Update the conditional logic to pass the handlers for all supported platforms, not just iOS..github/workflows/ci.yml-35-55 (1)
35-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
permissionsfor the new jobs.Both new jobs currently rely on default workflow token permissions. Add explicit minimal scopes (for these jobs, typically
contents: read) to reduce blast radius.🤖 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 @.github/workflows/ci.yml around lines 35 - 55, Add explicit least-privilege permissions to both the changes and mobile-checks jobs in the workflow. For each job, add a permissions section with minimal required scopes (typically contents: read for jobs that only need to read repository contents). This should be added at the job level under the job definition, before other configuration like runs-on or needs. This restricts the default workflow token permissions and reduces security risk if the workflow or its steps are compromised.Source: Linters/SAST tools
apps/mobile/shims/config.ts-62-62 (1)
62-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncode the token path segment before constructing the WebSocket URL.
Line 62 inserts
config.tokenraw into the URL path. If token format ever includes reserved URL characters, connection/auth routing can fail. Encode this segment likesessionIdis encoded.Suggested patch
- let url = `ws://${config.host}:${config.port}/${config.token}/ws`; + let url = `ws://${config.host}:${config.port}/${encodeURIComponent(config.token)}/ws`;🤖 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 `@apps/mobile/shims/config.ts` at line 62, The WebSocket URL construction in the line assigning the `ws://` URL is inserting the `config.token` directly into the path without URL encoding. If the token contains reserved URL characters, this will cause connection failures. Apply URL encoding to the `config.token` value using the same encoding method (likely encodeURIComponent) that is used elsewhere in the file for encoding path segments like `sessionId`, and place the encoded token into the URL path..github/workflows/ci.yml-40-42 (1)
40-42:⚠️ Potential issue | 🟠 MajorPin Actions by commit SHA and disable checkout credential persistence.
Lines 40-42 and 59-60 use mutable tag refs (
@v6,@v3) instead of commit SHAs. Tags can be updated by repository maintainers to point to different (potentially malicious) code. Pin to the full 40-character commit SHA instead (optionally with a version comment for readability).Additionally, both
actions/checkoutsteps should setpersist-credentials: falseunless authentication is needed for subsequent steps. Currently, the GITHUB_TOKEN is persisted in.git/configby default, making it accessible to all subsequent steps—a risk that doesn't apply here since neither step requires credential access.Example fix for lines 40-42:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 with: persist-credentials: false - uses: dorny/paths-filter@512585a4c4beff36070b4b2da4cc2c589c235a27 # v3🤖 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 @.github/workflows/ci.yml around lines 40 - 42, Replace the mutable tag references for actions/checkout@v6 and dorny/paths-filter@v3 with their full 40-character commit SHA equivalents (you can find these by checking the releases or commits of each action's repository) and add a version comment for readability. Additionally, add a with block to both the actions/checkout and dorny/paths-filter steps to set persist-credentials: false, which prevents the GITHUB_TOKEN from being persisted in the git config and reduces unnecessary credential exposure for steps that don't require authentication.Source: Linters/SAST tools
apps/mobile/.agents/skills/building-native-ui/references/storage.md-39-42 (1)
39-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
JSON.parsebefore returning cached values.A malformed or migrated entry will throw here and crash every caller of
useStorageduring render. Fall back todefaultValuewhen parsing fails.♻️ Proposed fix
get<T>(key: string, defaultValue: T): T { const value = localStorage.getItem(key); - return value ? JSON.parse(value) : defaultValue; + if (!value) return defaultValue; + try { + return JSON.parse(value); + } catch { + return defaultValue; + } }🤖 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 `@apps/mobile/.agents/skills/building-native-ui/references/storage.md` around lines 39 - 42, The `get` method lacks error handling around the `JSON.parse` call, which will throw and crash the caller if the stored value is malformed or corrupted. Wrap the `JSON.parse(value)` statement in a try-catch block and return the `defaultValue` when parsing fails, ensuring graceful fallback behavior instead of propagating the error up the call stack.apps/mobile/src/app/pair.tsx-61-68 (1)
61-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeout/abort handling to the redeem request.
The redeem
fetchhas no timeout. On stalled networks this can leave pairing stuck in the processing state for a long period and block retry flow.Proposed fix
async function redeemPairingCode(payload: PairCodePayload): Promise<RedemptionResult> { const deviceName = getDeviceName(); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); - const response = await fetch(`http://${payload.host}:${payload.port}/api/mobile-pairing/redeem`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - code: payload.code, - deviceName, - }), - }); + let response: Response; + try { + response = await fetch(`http://${payload.host}:${payload.port}/api/mobile-pairing/redeem`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + code: payload.code, + deviceName, + }), + signal: controller.signal, + }); + } finally { + clearTimeout(timeout); + }Also applies to: 167-168
🤖 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 `@apps/mobile/src/app/pair.tsx` around lines 61 - 68, The fetch request to the mobile pairing redeem endpoint lacks timeout handling, which can cause the pairing flow to hang indefinitely on stalled networks. Add an AbortController to implement timeout functionality, create a timeout that aborts the request after a reasonable duration (such as 30 seconds), pass the controller's signal to the fetch options, and wrap the fetch call in a try-catch block to handle AbortError exceptions appropriately so users can retry the pairing. Apply this same pattern to the other fetch call mentioned at lines 167-168.apps/mobile/src/app/pair.tsx-75-84 (1)
75-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate redeem response fields before persisting credentials.
Only
tokenis checked. IfdeviceIdis missing or non-string, invalid credentials are stored and downstream logic receives a brokenpairingId.Proposed fix
const result = await response.json(); - if (!result.token) { - throw new Error('No token in response'); + if ( + !result || + typeof result !== 'object' || + typeof result.token !== 'string' || + result.token.length === 0 || + typeof result.deviceId !== 'string' || + result.deviceId.length === 0 + ) { + throw new Error('Invalid redeem response payload'); }Also applies to: 140-146
🤖 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 `@apps/mobile/src/app/pair.tsx` around lines 75 - 84, The response validation in the pair.tsx file only checks for the presence of the token field but does not validate that deviceId exists or is a string. Add validation for the deviceId field to ensure it is present and has the correct type before returning the credentials object containing token, deviceId, and deviceName. This validation should be added immediately after the existing token check and before the return statement. Apply the same validation pattern to the second occurrence of this code (around lines 140-146).apps/mobile/src/lib/credentials.ts-35-40 (1)
35-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate parsed credential shape before returning it.
JSON.parse(...) as MaestroCredentialstrusts unvalidated storage data. A malformed object can pass through and break downstream URL/socket behavior instead of cleanly returningnull.Proposed fix
export interface MaestroCredentials { host: string; port: number; token: string; pairingId: string; deviceName: string; } + +function isValidCredentials(value: unknown): value is MaestroCredentials { + if (!value || typeof value !== 'object') return false; + const v = value as Record<string, unknown>; + return ( + typeof v.host === 'string' && + v.host.length > 0 && + typeof v.port === 'number' && + Number.isInteger(v.port) && + v.port >= 1 && + v.port <= 65535 && + typeof v.token === 'string' && + v.token.length > 0 && + typeof v.pairingId === 'string' && + v.pairingId.length > 0 && + typeof v.deviceName === 'string' && + v.deviceName.length > 0 + ); +} export async function getCredentials(): Promise<MaestroCredentials | null> { try { const stored = await SecureStore.getItemAsync(CREDENTIALS_KEY); if (!stored) return null; - return JSON.parse(stored) as MaestroCredentials; + const parsed: unknown = JSON.parse(stored); + return isValidCredentials(parsed) ? parsed : null; } catch { return null; } }🤖 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 `@apps/mobile/src/lib/credentials.ts` around lines 35 - 40, The getCredentials function parses stored JSON data and casts it to MaestroCredentials using `as`, but this provides no runtime validation of the actual data structure. If the stored data is malformed or missing required fields, it will still be returned and could break downstream code. Add validation logic after the JSON.parse call to verify the parsed object matches the MaestroCredentials shape (check for required properties/fields), and return null if validation fails instead of returning potentially invalid data.apps/mobile/src/hooks/useMaestroConnection.ts-74-84 (1)
74-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear reconnect state on terminal disconnects.
isReconnectingis only reset onauthenticated, so a failed reconnect that lands back indisconnectedcan leave the status pill stuck on “Reconnecting...” indefinitely. Reset the flag on the terminal disconnected path too.🔧 Suggested fix
onConnectionChange: (state: WebSocketState) => { if (state === 'authenticated') { setIsReconnecting(false); // Check for stale buffer on reconnect if (hasStaleBufferRef.current) { hasStaleBufferRef.current = false; onStaleBufferDiscarded?.(); } + } else if (state === 'disconnected') { + setIsReconnecting(false); } handlers?.onConnectionChange?.(state); },🤖 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 `@apps/mobile/src/hooks/useMaestroConnection.ts` around lines 74 - 84, The isReconnecting state is only being reset to false when the connection state becomes 'authenticated', but if a reconnection fails and returns to 'disconnected', the flag remains true causing the UI to display a stuck "Reconnecting..." status. In the onConnectionChange handler within useMaestroConnection, add an additional check for when state equals 'disconnected' (or other terminal disconnected states) and reset isReconnecting to false in that branch as well, similar to how it's reset for the 'authenticated' state.apps/mobile/metro.config.js-51-53 (1)
51-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize resolver origin paths before mobile/web-hooks checks.
Lines 51-53 assume
/separators; Windows paths use\, so shim routing may silently not apply.Suggested fix
- const origin = context.originModulePath || ''; - const inMobileOrWebHooks = origin.includes('apps/mobile/') || origin.includes('src/web/hooks/'); + const origin = (context.originModulePath || '').replace(/\\/g, '/'); + const inMobileOrWebHooks = + origin.includes('/apps/mobile/') || origin.includes('/src/web/hooks/');🤖 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 `@apps/mobile/metro.config.js` around lines 51 - 53, The path checks on `context.originModulePath` assume forward slash separators, which causes the mobile and web-hooks routing to silently fail on Windows systems that use backslashes. Normalize the `origin` variable by converting all backslashes to forward slashes before performing the `origin.includes()` checks for 'apps/mobile/' and 'src/web/hooks/' in the `inMobileOrWebHooks` assignment, ensuring cross-platform compatibility.apps/mobile/src/lib/useMaestroWebSocket.ts-350-352 (1)
350-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
buildWebSocketUrl()failures inside the existing connection try/catch.Line 351 awaits
buildWebSocketUrl()before thetryblock; if it rejects, the error bypasses your connection error handling and escapes as an unhandled async rejection.Suggested fix
- // Build URL without a specific sessionId - session is selected after connection - const url = await buildWebSocketUrl(); - - if (!url) { - // No credentials available - need pairing - setError('No credentials - please pair with Maestro desktop'); - handlersRef.current?.onError?.('No credentials - please pair with Maestro desktop'); - setState('disconnected'); - handlersRef.current?.onConnectionChange?.('disconnected'); - return; - } - - try { + try { + // Build URL without a specific sessionId - session is selected after connection + const url = await buildWebSocketUrl(); + if (!url) { + setError('No credentials - please pair with Maestro desktop'); + handlersRef.current?.onError?.('No credentials - please pair with Maestro desktop'); + setState('disconnected'); + handlersRef.current?.onConnectionChange?.('disconnected'); + return; + } const ws = new WebSocket(url); wsRef.current = ws;Also applies to: 362-395
🤖 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 `@apps/mobile/src/lib/useMaestroWebSocket.ts` around lines 350 - 352, The buildWebSocketUrl() call on line 351 is located before the try block, which means if it rejects, the error will not be caught by the existing connection error handling and will escape as an unhandled async rejection. Move the buildWebSocketUrl() invocation inside the try block along with the other connection logic (the WebSocket constructor and related setup code) so that any failures from building the URL are properly handled by the existing error handling mechanism.apps/mobile/src/hooks/useSessionChat.ts-153-153 (1)
153-153: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace inline random message IDs with shared ID utility calls.
These lines create new local ID-generation logic; use the repo-standard generator to keep behavior consistent and avoid duplicate implementations.
Suggested fix
import * as Haptics from 'expo-haptics'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { generateUUID } from '`@maestro/shared/uuid`'; @@ - const messageId = `assistant-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const messageId = `assistant-${generateUUID()}`; @@ - const messageId = `user-desktop-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const messageId = `user-desktop-${generateUUID()}`; @@ - const userMessageId = `user-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const userMessageId = `user-${generateUUID()}`;As per coding guidelines, "Use
generateId()fromsrc/renderer/utils/ids.tsorgenerateUUID()fromsrc/shared/uuid.tsfor ID generation. Do not create duplicate implementations."Also applies to: 228-228, 304-304
🤖 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 `@apps/mobile/src/hooks/useSessionChat.ts` at line 153, The messageId assignment on lines 153, 228, and 304 in the useSessionChat hook uses inline random ID generation with Date.now() and Math.random(). Replace this inline implementation with a call to either generateId() from src/renderer/utils/ids.ts or generateUUID() from src/shared/uuid.ts to maintain consistency with the codebase standard. Update all three occurrences to use the same shared utility function instead of creating duplicate ID-generation logic.Source: Coding guidelines
apps/mobile/src/global.css-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorAdd
ignoreAtRulesconfiguration to stylelint for Tailwind/Uniwind directives.The stylelint config at
.stylelintrc.jsonis missing the allowlist for@variant(lines 11, 28) and@theme(line 46) directives. Add this rule to suppress unknown at-rule warnings:Required configuration update
"at-rule-no-unknown": [true, { "ignoreAtRules": ["theme", "variant"] }]Add this to the
rulesobject in.stylelintrc.json.🤖 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 `@apps/mobile/src/global.css` at line 11, The stylelint configuration is missing the allowlist for custom at-rules used by Tailwind/Uniwind. In the `.stylelintrc.json` file, add the `at-rule-no-unknown` rule to the rules object with the `ignoreAtRules` option set to allow both "theme" and "variant" directives. This will suppress the unknown at-rule warnings that are currently being triggered by the `@variant` and `@theme` directives in the CSS files.Source: Linters/SAST tools
apps/mobile/src/lib/useMaestroWebSocket.ts-451-451 (1)
451-451: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse shared ID utilities for request IDs instead of inline random construction.
Line 451 builds a new ad-hoc ID implementation; this should use the shared ID generator utility.
Suggested fix
import { useState, useEffect, useCallback, useRef } from 'react'; import { buildWebSocketUrl } from '../../shims/config'; import type { Theme } from '`@maestro/shared/theme-types`'; +import { generateUUID } from '`@maestro/shared/uuid`'; @@ - const requestId = `history-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; + const requestId = `history-${generateUUID()}`;As per coding guidelines, "Use
generateId()fromsrc/renderer/utils/ids.tsorgenerateUUID()fromsrc/shared/uuid.tsfor ID generation. Do not create duplicate implementations."🤖 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 `@apps/mobile/src/lib/useMaestroWebSocket.ts` at line 451, Replace the inline ad-hoc request ID construction on line 451 in the useMaestroWebSocket hook with a call to the shared ID generator utility. Remove the manual implementation using Date.now() and Math.random(), and instead import and use either generateId() from src/renderer/utils/ids.ts or generateUUID() from src/shared/uuid.ts. Update the requestId assignment to use the imported utility function, maintaining the history prefix if needed for context identification.Source: Coding guidelines
apps/mobile/src/streaming/reconcileStreamingMessage.ts-95-96 (1)
95-96: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse shared ID utilities for new message IDs.
Assistant/user IDs are generated manually from timestamps. Please switch to
generateId()/generateUUID()to follow repository standards and avoid ad-hoc ID generation drift.Suggested change
+import { generateUUID } from '`@maestro/shared/uuid`'; ... - const messageId = `assistant-${action.timestamp}`; + const messageId = `assistant-${generateUUID()}`; ... - const messageId = `user-${action.timestamp}`; + const messageId = `user-${generateUUID()}`;As per coding guidelines, use
generateId()fromsrc/renderer/utils/ids.tsorgenerateUUID()fromsrc/shared/uuid.tsfor ID generation and avoid duplicate implementations.Also applies to: 185-186
🤖 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 `@apps/mobile/src/streaming/reconcileStreamingMessage.ts` around lines 95 - 96, The message ID generation at the location containing `assistant-${action.timestamp}` is using manual timestamp concatenation instead of the repository's standard ID utilities. Replace this manual ID generation with a call to `generateId()` from `src/renderer/utils/ids.ts` or `generateUUID()` from `src/shared/uuid.ts`, and add the appropriate import statement. Apply the same fix to similar manual ID generation patterns elsewhere in the file that also use timestamp-based concatenation.Source: Coding guidelines
apps/mobile/src/streaming/reconcileStreamingMessage.ts-148-171 (1)
148-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SESSION_STATE_CHANGEcan leave a stale in-flight placeholder.At Line 150, commit requires a truthy
streamingBuffer. If the first AI chunk is'', Line 168-171 runs and only flipsisGenerating, leavingstreamingMessageIdand the empty placeholder message behind.Proposed fix
case 'SESSION_STATE_CHANGE': { if (action.state === 'idle' || action.state === 'ready') { - // Commit streaming buffer to final message - if (state.streamingMessageId && state.streamingBuffer) { - const finalContent = state.streamingBuffer; - const msgId = state.streamingMessageId; - - const updated = state.messages.map((m) => - m.id === msgId ? { ...m, content: finalContent } : m - ); - - return { - ...state, - messages: updated, - streamingBuffer: '', - streamingMessageId: null, - isGenerating: false, - }; - } + if (state.streamingMessageId) { + const msgId = state.streamingMessageId; + const updated = + state.streamingBuffer.length > 0 + ? state.messages.map((m) => + m.id === msgId ? { ...m, content: state.streamingBuffer } : m + ) + : state.messages.filter((m) => m.id !== msgId); + + return { + ...state, + messages: updated, + streamingBuffer: '', + streamingMessageId: null, + isGenerating: false, + }; + } // No buffer to commit, just stop generating return {🤖 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 `@apps/mobile/src/streaming/reconcileStreamingMessage.ts` around lines 148 - 171, When SESSION_STATE_CHANGE action has state 'idle' or 'ready', the code only commits the streaming buffer if it is truthy. If the first AI chunk is an empty string, the condition fails and falls through to the "No buffer to commit, just stop generating" return statement which only sets isGenerating to false. This leaves streamingMessageId and the placeholder message as stale data. In the fallback return statement that handles the case where there is no buffer to commit, also reset streamingMessageId to null and streamingBuffer to an empty string to properly clean up the stale placeholder message, matching the cleanup done in the successful commit case above.
🧹 Nitpick comments (5)
apps/mobile/src/app/chats.tsx (1)
13-17: ⚡ Quick winUse the shared formatter instead of a local time-ago helper.
formatTimeAgoduplicates shared formatting logic; please switch this to the shared formatter utility to keep behavior consistent across clients.As per coding guidelines,
**/*.{ts,tsx,js,jsx}should useformatSize(),formatNumber(),formatTokens(),formatTokensCompact(),estimateTokenCount(),formatElapsedTime(),formatElapsedTimeColon(),formatRelativeTime(), orformatCost()fromsrc/shared/formatters.tsfor formatting operations.🤖 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 `@apps/mobile/src/app/chats.tsx` around lines 13 - 17, The local formatTimeAgo function in chats.tsx duplicates shared formatting logic that already exists in src/shared/formatters.ts. Remove the formatTimeAgo function definition and replace its usage with the appropriate shared formatter utility. Check src/shared/formatters.ts for the correct formatter to use (likely formatRelativeTime, formatElapsedTime, or similar) that provides the time-ago formatting behavior, then update the call site to use the shared formatter instead.Source: Coding guidelines
package.json (1)
27-27: ⚡ Quick winAvoid hardcoding a specific iOS simulator in the root dev script.
Line 27 ties local workflows to one simulator name, which commonly fails on machines that don’t have that exact runtime/device. Prefer default simulator selection or an env-driven device override.
🤖 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 `@package.json` at line 27, The dev:mobile:ios script in package.json hardcodes the device selection to "iPhone 16 Pro", which causes failures on machines without that specific simulator. Remove the --device 'iPhone 16 Pro' parameter from the expo run:ios command to allow Expo to select a default simulator, or alternatively make the device name configurable through an environment variable that users can override locally without modifying the script itself.apps/mobile/src/pairing/__tests__/parseQrPayload.test.ts (1)
214-222: ⚡ Quick winAdd a regression case for
maestro://pairing?...payloads.Current invalid-scheme tests don’t cover prefix-collision inputs like
maestro://pairing?..., which should be rejected. Adding that case will lock in the intended strict format.🤖 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 `@apps/mobile/src/pairing/__tests__/parseQrPayload.test.ts` around lines 214 - 222, Add a regression test case in the "invalid URL schemes" describe block to verify that parseQrPayload correctly rejects payloads using maestro://pairing instead of the valid maestro://pair format. Insert a new test case after the existing "returns null for maestro:// without pair path" test that calls parseQrPayload with maestro://pairing?host=localhost&port=8080&code=X and asserts it returns null. This ensures the strict format validation for the pair path segment is locked in against future regressions.apps/mobile/src/streaming/__tests__/streamingReconciliation.test.ts (1)
31-103: ⚡ Quick winAdd a regression for empty first
SESSION_OUTPUTchunks.There’s no coverage for
data: ''followed bySESSION_STATE_CHANGEtoidle/ready. That edge case should assert the placeholder is cleared and streaming flags are reset.Suggested test case
+it('clears placeholder when first AI chunk is empty and session becomes idle', () => { + let state = createInitialState(); + state = streamingReducer(state, { + type: 'SESSION_OUTPUT', + data: '', + source: 'ai', + timestamp: 1234, + }); + + state = streamingReducer(state, { + type: 'SESSION_STATE_CHANGE', + state: 'idle', + }); + + expect(state.streamingMessageId).toBeNull(); + expect(state.streamingBuffer).toBe(''); + expect(state.isGenerating).toBe(false); +});Also applies to: 202-280
🤖 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 `@apps/mobile/src/streaming/__tests__/streamingReconciliation.test.ts` around lines 31 - 103, Add a new test case within the SESSION_OUTPUT events describe block (or create a new describe block for SESSION_STATE_CHANGE events) that covers the edge case where a SESSION_OUTPUT action with empty data (data: '') is followed by a SESSION_STATE_CHANGE action with state transitioning to idle or ready. The test should verify that after these actions, the placeholder message in the messages array is properly cleared (content should not remain), the streamingBuffer is empty, the streamingMessageId is null, and isGenerating is false.apps/mobile/src/__tests__/offlineQueueReplay.test.ts (1)
44-357: 🏗️ Heavy liftThese tests are disconnected from the production queue implementation.
This suite mostly validates local arrays/state transitions, so regressions in the real queue hook can still pass. Please anchor key cases to
useMaestroOfflineQueue/shared queue logic via public API-level tests.🤖 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 `@apps/mobile/src/__tests__/offlineQueueReplay.test.ts` around lines 44 - 357, The test suite is testing isolated arrays and mock state transitions rather than the actual queue implementation from useMaestroOfflineQueue, which means production regressions could pass these tests. Refactor the test cases to use the real useMaestroOfflineQueue hook and its public API instead of directly manipulating arrays. For key test cases like those in queue ordering, storage persistence, queue capacity, retry handling, and command removal sections, replace the direct array manipulation with actual calls to the queue hook's methods and verify the behavior through the hook's public interface. This ensures the tests catch real regressions in the actual queue logic rather than just validating basic array operations.
Implement core pairing infrastructure with short-lived codes (6-char base32, 5-minute expiry) and long-lived hashed tokens (90-day TTL) for mobile device authentication. Includes device CRUD operations and periodic code cleanup.
Expose mobile pairing operations to renderer via window.maestro.mobilePairing namespace. Handlers integrate with WebServer to include host/port in generated pairing codes for QR display.
Extend WebSocket route to authenticate both browser security tokens and mobile device tokens. Add public pairing code redemption endpoint with rate limiting. Track mobile client metadata for connection management.
Implement MobileDevicesSection component with QR code generation for device pairing, real-time countdown, paired device list, and revocation. Integrated into General settings tab with full search support.
Replace direct localStorage calls with injected StorageAdapter interface. Enables the same hook to work with AsyncStorage in React Native. Add async initialization and factory function for web localStorage adapter.
Update WebSocket route tests for wildcard routing pattern. Add comprehensive tests for mobile token validation, auth rejection, and client ID prefixes.
Add conditional mobile-checks CI job that runs TypeScript, ESLint, Jest, and expo-doctor only when mobile or shared code changes. Add development scripts for running the Expo app. Exclude apps/ from root ESLint config.
Initialize apps/mobile with Expo managed workflow. Configure Metro for monorepo symlinks to shared code. Include Jest testing infrastructure and standalone ESLint config.
The pre-push validate hook ran prettier over generated CocoaPods, .expo, and native build output under apps/mobile, failing the format check on files that are not meant to be hand-formatted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/__tests__/main/mobile-pairing.test.ts (1)
113-121: ⚡ Quick winHarden timer cleanup to prevent cross-test clock leakage
If an assertion throws before
vi.useRealTimers(), later tests can run under fake time and fail non-deterministically. Add a global cleanup hook (afterEach) to always restore timers.Suggested patch
import { describe, it, expect, beforeEach, afterAll, vi } from 'vitest'; @@ afterAll(() => { rmSync(tmpRoot, { recursive: true, force: true }); }); + +afterEach(() => { + vi.useRealTimers(); + vi.clearAllTimers(); +});Also applies to: 180-190, 244-253
🤖 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 `@src/__tests__/main/mobile-pairing.test.ts` around lines 113 - 121, The test `rejects expired codes` uses vi.useFakeTimers() and vi.useRealTimers() manually, but if an assertion throws before vi.useRealTimers() is called, the fake timers remain active and will cause subsequent tests to run under fake time and fail non-deterministically. Add a global afterEach cleanup hook at the appropriate scope (test file or describe block) that unconditionally calls vi.useRealTimers() to guarantee timer restoration regardless of assertion outcomes. This same pattern should be applied to the other affected tests at lines 180-190 and 244-253.
🤖 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 @.github/workflows/ci.yml:
- Line 40: The checkout steps at lines 40 and 59 are leaving write-capable
tokens in the git config which is a security risk. Add the parameter
persist-credentials: false to both instances of actions/checkout@v6 to disable
credential persistence and prevent the token from remaining in git config after
the job execution completes.
- Around line 40-41: Replace the movable version tags with immutable commit SHAs
for all GitHub Actions to strengthen supply-chain integrity. For the
actions/checkout@v6 on line 40 and dorny/paths-filter@v3 on line 41, change the
`@v*` tag to the full commit SHA of the respective action version. The same
change applies to any other actions referenced on lines 59-60. Update each uses
statement to reference the specific commit hash instead of the version tag
format.
- Around line 35-54: Add explicit least-privilege permissions to the workflow to
restrict token access to only what is necessary. At the workflow level (after
the `on:` configuration), add a `permissions` section that restricts access to
read-only operations. For the `changes` job which uses `actions/checkout@v6` and
`dorny/paths-filter@v3`, and the `mobile-checks` job which only needs to read
outputs from the changes job, explicitly set `permissions: contents: read` to
limit token scope to content read access only, removing reliance on default
broader permissions.
In `@apps/mobile/.agents/skills/building-native-ui/references/route-structure.md`:
- Line 18: Add language identifiers to all fenced code blocks in the
route-structure.md file that are missing them. Specifically, modify the opening
fenced code block markers at lines 18, 30, 69, 95, 112, and 155 by adding a
language identifier (such as `text`) immediately after the opening triple
backticks. For example, change the opening marker from ``` to ```text to satisfy
the markdownlint MD040 rule.
In `@apps/mobile/.agents/skills/building-native-ui/references/tabs.md`:
- Line 305: The fenced code block in the route-structure section is missing a
language identifier, which violates the MD040 markdown linting rule. Locate the
opening triple backtick (```) that corresponds to the closing triple backtick on
line 305 and add an appropriate language identifier immediately after the
opening backticks (such as "route-structure" or another relevant language label)
to specify the language of the code block content.
In `@apps/mobile/.agents/skills/building-native-ui/SKILL.md`:
- Around line 34-40: Update the Expo Go guidance in lines 34-40 of the SKILL.md
file to reflect that this app requires a custom Expo development build and will
not work in Expo Go. Replace the current instructions that recommend trying Expo
Go first with a clear statement that custom builds are mandatory for this app,
and then provide the correct setup steps that skip Expo Go and go directly to
custom build commands like npx expo run:ios or npx expo run:android.
- Line 14: The fenced code blocks at line 14 and line 249 in the SKILL.md file
are missing language identifiers, which violates markdown lint rule MD040. Add
the appropriate language identifier (such as javascript, typescript, json, bash,
etc.) to each opening fence marker (the triple backticks) to specify what
language the code block contains. For example, change ``` to ```javascript or
```typescript depending on the content of each code block.
In `@apps/mobile/src/components/chat/prompt-input.web.tsx`:
- Around line 16-26: The actions array is being populated with PromptInputAction
children in the PromptInput component but these actions are never rendered in
the component's return statement, making them have no effect. After collecting
the actions and body by filtering children using Children.forEach, ensure the
actions array is included in the component's JSX output alongside the body.
Render the actions array elements in an appropriate location within the
component's return statement to make PromptInputAction children functional.
- Around line 110-127: The onKeyPress handler in the TextInput component calls
onSend() directly without validating input state, unlike the button submit
logic. Add validation checks before calling onSend() in the keyboard handler to
ensure the input is not blank and generation is not in progress, mirroring the
checks that exist in the button's submit handler.
---
Nitpick comments:
In `@src/__tests__/main/mobile-pairing.test.ts`:
- Around line 113-121: The test `rejects expired codes` uses vi.useFakeTimers()
and vi.useRealTimers() manually, but if an assertion throws before
vi.useRealTimers() is called, the fake timers remain active and will cause
subsequent tests to run under fake time and fail non-deterministically. Add a
global afterEach cleanup hook at the appropriate scope (test file or describe
block) that unconditionally calls vi.useRealTimers() to guarantee timer
restoration regardless of assertion outcomes. This same pattern should be
applied to the other affected tests at lines 180-190 and 244-253.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cf5fa57-54b2-47a5-9117-6ae429305559
⛔ Files ignored due to path filters (4)
apps/mobile/assets/images/splash-icon.pngis excluded by!**/*.pngapps/mobile/bun.lockis excluded by!**/*.lockapps/mobile/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (147)
.github/workflows/ci.yml.prettierignoreapps/mobile/.agents/skills/building-native-ui/SKILL.mdapps/mobile/.agents/skills/building-native-ui/references/animations.mdapps/mobile/.agents/skills/building-native-ui/references/controls.mdapps/mobile/.agents/skills/building-native-ui/references/form-sheet.mdapps/mobile/.agents/skills/building-native-ui/references/gradients.mdapps/mobile/.agents/skills/building-native-ui/references/icons.mdapps/mobile/.agents/skills/building-native-ui/references/media.mdapps/mobile/.agents/skills/building-native-ui/references/route-structure.mdapps/mobile/.agents/skills/building-native-ui/references/search.mdapps/mobile/.agents/skills/building-native-ui/references/storage.mdapps/mobile/.agents/skills/building-native-ui/references/tabs.mdapps/mobile/.agents/skills/building-native-ui/references/toolbar-and-headers.mdapps/mobile/.agents/skills/building-native-ui/references/visual-effects.mdapps/mobile/.agents/skills/building-native-ui/references/webgpu-three.mdapps/mobile/.agents/skills/building-native-ui/references/zoom-transitions.mdapps/mobile/.agents/skills/uniwind/SKILL.mdapps/mobile/.claude/launch.jsonapps/mobile/.claude/settings.jsonapps/mobile/.claude/skills/building-native-uiapps/mobile/.claude/skills/uniwindapps/mobile/.eas/workflows/deploy.ymlapps/mobile/.eas/workflows/preview-web.ymlapps/mobile/.env.exampleapps/mobile/.gitattributesapps/mobile/.gitignoreapps/mobile/AGENTS.mdapps/mobile/CLAUDE.mdapps/mobile/README.mdapps/mobile/app.jsonapps/mobile/eslint.config.jsapps/mobile/jest.config.jsapps/mobile/jest.setup.tsapps/mobile/metro.config.jsapps/mobile/package.jsonapps/mobile/scripts/setup-symlinks.jsapps/mobile/shims/config.tsapps/mobile/shims/logger.tsapps/mobile/skills-lock.jsonapps/mobile/src/__tests__/offlineQueueReplay.test.tsapps/mobile/src/__tests__/sessionAddedRouting.test.tsxapps/mobile/src/__tests__/smoke.test.tsapps/mobile/src/app/(settings)/_layout.tsxapps/mobile/src/app/(settings)/capabilities.tsxapps/mobile/src/app/(settings)/profile.tsxapps/mobile/src/app/(settings)/settings.tsxapps/mobile/src/app/__tests__/chatScreensUseRealAgent.test.tsapps/mobile/src/app/_layout.tsxapps/mobile/src/app/_layout.web.tsxapps/mobile/src/app/attachments.tsxapps/mobile/src/app/chats.tsxapps/mobile/src/app/index.tsxapps/mobile/src/app/model-picker.tsxapps/mobile/src/app/pair.tsxapps/mobile/src/app/session/[sessionId].tsxapps/mobile/src/components/AITabStrip.tsxapps/mobile/src/components/ConnectionStatusPill.tsxapps/mobile/src/components/blur-raw.tsxapps/mobile/src/components/blur-raw.web.tsxapps/mobile/src/components/chat/__tests__/streaming-store.test.tsapps/mobile/src/components/chat/chat-context.tsxapps/mobile/src/components/chat/conversation.tsxapps/mobile/src/components/chat/conversation.web.tsxapps/mobile/src/components/chat/index.tsapps/mobile/src/components/chat/message.tsxapps/mobile/src/components/chat/message.web.tsxapps/mobile/src/components/chat/prompt-input.tsxapps/mobile/src/components/chat/prompt-input.web.tsxapps/mobile/src/components/chat/streaming-message.tsxapps/mobile/src/components/chat/streaming-store.tsapps/mobile/src/components/chat/types.tsapps/mobile/src/components/drawer-content.tsxapps/mobile/src/components/drawer-layout.tsxapps/mobile/src/components/grabber.android.tsxapps/mobile/src/components/grabber.tsxapps/mobile/src/components/icon.tsxapps/mobile/src/components/main-header.android.tsxapps/mobile/src/components/main-header.fallback.tsxapps/mobile/src/components/main-header.ios.tsxapps/mobile/src/components/main-header.swiftui.tsxapps/mobile/src/components/main-header.tsxapps/mobile/src/components/markdown/ast-renderer.tsapps/mobile/src/components/markdown/chat-markdown.tsxapps/mobile/src/components/markdown/code-block.tsxapps/mobile/src/components/markdown/index.tsapps/mobile/src/components/markdown/markdown.tsxapps/mobile/src/components/markdown/render-rules.tsxapps/mobile/src/components/markdown/types.tsapps/mobile/src/components/markdown/utils.tsapps/mobile/src/components/model-context.tsxapps/mobile/src/components/sidebar.tsxapps/mobile/src/components/sidebar.web.tsxapps/mobile/src/components/symbol-image.tsxapps/mobile/src/components/touchable-glass.tsxapps/mobile/src/components/tw.tsxapps/mobile/src/global.cssapps/mobile/src/hooks/__tests__/useMaestroConnection.test.tsapps/mobile/src/hooks/__tests__/useSessionChat.test.tsapps/mobile/src/hooks/useMaestroConnection.tsapps/mobile/src/hooks/useMaestroOfflineQueue.tsapps/mobile/src/hooks/usePairingCheck.tsapps/mobile/src/hooks/useSessionChat.tsapps/mobile/src/lib/SessionsContext.tsxapps/mobile/src/lib/ToastContext.tsxapps/mobile/src/lib/__tests__/messageRouting.test.tsapps/mobile/src/lib/credentials.tsapps/mobile/src/lib/useMaestroWebSocket.tsapps/mobile/src/pairing/__tests__/parseQrPayload.test.tsapps/mobile/src/pairing/parseQrPayload.tsapps/mobile/src/sf.cssapps/mobile/src/storage/__tests__/asyncStorageAdapter.test.tsapps/mobile/src/storage/asyncStorageAdapter.tsapps/mobile/src/streaming/__tests__/streamingReconciliation.test.tsapps/mobile/src/streaming/index.tsapps/mobile/src/streaming/reconcileStreamingMessage.tsapps/mobile/src/theme/AccentContext.tsxapps/mobile/src/utils/mock-chats.tsapps/mobile/src/utils/tailwind.tsapps/mobile/src/utils/use-system-background-color.tsapps/mobile/tsconfig.jsonapps/mobile/uniwind-types.d.tseslint.config.mjspackage.jsonsrc/__tests__/main/mobile-pairing.test.tssrc/__tests__/main/web-server/routes/wsRoute.test.tssrc/__tests__/web/hooks/useOfflineQueue.test.tssrc/__tests__/web/mobile/App.test.tsxsrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/mobile-pairing.tssrc/main/mobile-pairing/index.tssrc/main/preload/index.tssrc/main/preload/mobilePairing.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/routes/index.tssrc/main/web-server/routes/mobilePairingRoutes.tssrc/main/web-server/routes/wsRoute.tssrc/main/web-server/types.tssrc/renderer/components/Settings/MobileDevicesSection.tsxsrc/renderer/components/Settings/searchableSettings.tssrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/global.d.tssrc/web/hooks/__tests__/useOfflineQueue.test.tssrc/web/hooks/useOfflineQueue.tssrc/web/mobile/App.tsx
✅ Files skipped from review due to trivial changes (23)
- apps/mobile/skills-lock.json
- apps/mobile/src/components/main-header.android.tsx
- apps/mobile/.claude/skills/uniwind
- .prettierignore
- apps/mobile/.claude/settings.json
- apps/mobile/CLAUDE.md
- apps/mobile/app.json
- apps/mobile/.claude/launch.json
- apps/mobile/src/components/chat/index.ts
- apps/mobile/.gitignore
- apps/mobile/.env.example
- apps/mobile/uniwind-types.d.ts
- apps/mobile/.claude/skills/building-native-ui
- apps/mobile/src/components/main-header.ios.tsx
- apps/mobile/.agents/skills/building-native-ui/references/zoom-transitions.md
- apps/mobile/src/utils/tailwind.ts
- apps/mobile/.agents/skills/building-native-ui/references/form-sheet.md
- apps/mobile/README.md
- apps/mobile/.agents/skills/building-native-ui/references/storage.md
- apps/mobile/.agents/skills/building-native-ui/references/icons.md
- apps/mobile/.agents/skills/building-native-ui/references/gradients.md
- eslint.config.mjs
- apps/mobile/.agents/skills/building-native-ui/references/search.md
🚧 Files skipped from review as they are similar to previous changes (89)
- apps/mobile/src/app/tests/chatScreensUseRealAgent.test.ts
- apps/mobile/jest.setup.ts
- apps/mobile/src/components/blur-raw.tsx
- apps/mobile/jest.config.js
- package.json
- apps/mobile/src/components/sidebar.tsx
- apps/mobile/src/components/blur-raw.web.tsx
- apps/mobile/src/storage/asyncStorageAdapter.ts
- apps/mobile/src/components/main-header.tsx
- apps/mobile/src/utils/mock-chats.ts
- apps/mobile/scripts/setup-symlinks.js
- apps/mobile/src/components/chat/types.ts
- apps/mobile/src/components/chat/tests/streaming-store.test.ts
- apps/mobile/src/components/grabber.tsx
- apps/mobile/src/app/_layout.web.tsx
- apps/mobile/src/streaming/index.ts
- apps/mobile/src/components/model-context.tsx
- apps/mobile/src/utils/use-system-background-color.ts
- apps/mobile/src/components/markdown/index.ts
- apps/mobile/src/storage/tests/asyncStorageAdapter.test.ts
- apps/mobile/src/components/grabber.android.tsx
- apps/mobile/tsconfig.json
- apps/mobile/.gitattributes
- apps/mobile/src/app/(settings)/profile.tsx
- apps/mobile/src/tests/smoke.test.ts
- apps/mobile/.agents/skills/building-native-ui/references/animations.md
- apps/mobile/shims/logger.ts
- apps/mobile/shims/config.ts
- apps/mobile/src/components/chat/streaming-store.ts
- apps/mobile/src/components/chat/streaming-message.tsx
- apps/mobile/src/app/(settings)/capabilities.tsx
- apps/mobile/src/app/attachments.tsx
- apps/mobile/src/components/chat/chat-context.tsx
- apps/mobile/src/lib/tests/messageRouting.test.ts
- apps/mobile/src/hooks/useMaestroOfflineQueue.ts
- apps/mobile/.eas/workflows/preview-web.yml
- apps/mobile/src/components/chat/conversation.web.tsx
- apps/mobile/src/components/chat/message.web.tsx
- apps/mobile/src/app/(settings)/settings.tsx
- apps/mobile/src/hooks/usePairingCheck.ts
- apps/mobile/src/components/drawer-layout.tsx
- apps/mobile/src/components/markdown/code-block.tsx
- apps/mobile/src/hooks/tests/useMaestroConnection.test.ts
- apps/mobile/src/components/main-header.swiftui.tsx
- apps/mobile/.eas/workflows/deploy.yml
- apps/mobile/src/components/main-header.fallback.tsx
- apps/mobile/src/components/ConnectionStatusPill.tsx
- apps/mobile/src/hooks/tests/useSessionChat.test.ts
- apps/mobile/.agents/skills/building-native-ui/references/visual-effects.md
- apps/mobile/metro.config.js
- apps/mobile/src/pairing/parseQrPayload.ts
- apps/mobile/src/components/markdown/utils.ts
- apps/mobile/package.json
- apps/mobile/src/components/icon.tsx
- apps/mobile/src/lib/credentials.ts
- apps/mobile/src/components/markdown/markdown.tsx
- apps/mobile/src/app/index.tsx
- apps/mobile/src/components/touchable-glass.tsx
- apps/mobile/src/app/(settings)/_layout.tsx
- apps/mobile/src/components/symbol-image.tsx
- apps/mobile/eslint.config.js
- apps/mobile/src/app/model-picker.tsx
- apps/mobile/src/streaming/tests/streamingReconciliation.test.ts
- apps/mobile/src/components/drawer-content.tsx
- apps/mobile/.agents/skills/building-native-ui/references/controls.md
- apps/mobile/src/components/markdown/chat-markdown.tsx
- apps/mobile/src/components/chat/prompt-input.tsx
- apps/mobile/src/components/sidebar.web.tsx
- apps/mobile/src/app/chats.tsx
- apps/mobile/src/theme/AccentContext.tsx
- apps/mobile/src/components/markdown/ast-renderer.ts
- apps/mobile/src/components/AITabStrip.tsx
- apps/mobile/src/components/chat/message.tsx
- apps/mobile/src/tests/offlineQueueReplay.test.ts
- apps/mobile/src/components/tw.tsx
- apps/mobile/src/components/markdown/types.ts
- apps/mobile/src/lib/ToastContext.tsx
- apps/mobile/src/components/markdown/render-rules.tsx
- apps/mobile/src/app/_layout.tsx
- apps/mobile/src/hooks/useMaestroConnection.ts
- apps/mobile/src/components/chat/conversation.tsx
- apps/mobile/src/lib/useMaestroWebSocket.ts
- apps/mobile/src/hooks/useSessionChat.ts
- apps/mobile/src/tests/sessionAddedRouting.test.tsx
- apps/mobile/src/app/pair.tsx
- apps/mobile/src/streaming/reconcileStreamingMessage.ts
- apps/mobile/src/app/session/[sessionId].tsx
- src/tests/main/web-server/routes/wsRoute.test.ts
- apps/mobile/src/lib/SessionsContext.tsx
- ci.yml: add top-level least-privilege permissions and persist-credentials: false on checkouts - skills/building-native-ui: label fenced code blocks with `text` to satisfy MD040 - skills/building-native-ui: rewrite "Running the App" to require a custom dev build (per apps/mobile/CLAUDE.md), since Expo Go does not support this app's native modules - prompt-input.web.tsx: forward PromptInputAction children into PromptInputBody so they actually render - prompt-input.web.tsx: gate Enter-to-send by input.trim() and isGenerating, matching the submit button Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace floating @v* tags in .github/workflows/ci.yml with full commit SHAs (with version comments) so the workflow does not silently shift under us if an upstream tag is moved. - actions/checkout@v6 -> df4cb1c (v6.0.3) - actions/setup-node@v6 -> 48b55a0 (v6.4.0) - dorny/paths-filter@v3 -> d1c1ffe (v3.0.3) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Review: Mobile companion appRead through the backend integration, the native app, and how it lines up with Maestro's model. The security foundation is solid and the desktop-side wiring is idiomatic, so the concerns below are mostly strategic rather than safety blockers. Scoping this as an "M2, chat companion" milestone, it's a reasonable foundation. What holds up
1. It's a chat-only remote, and "New session" is stubbedThe app sends 6 of ~100 WS message types. Creating new agents from mobile isn't needed (that's a desktop task), but creating a new session is, and it's a stub: "New session feature is not available in M2" ( Also: the model picker is cosmetic and Claude-only. Hardcoded Anthropic names ( 2. It forks the desktop-like web client instead of sharing it
More importantly, Two loose ends worth resolving: 3. Remote access: the cloudflared tunnel already exists - pairing just doesn't use itMaestro already ships a working built-in cloudflared tunnel: The mobile gap is just that pairing ignores it: the QR is built from
Smaller items
Bottom lineSolid, safe foundation. Before merge I'd want the two lockfiles resolved, the dead-code questions (streaming reducer, |
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
2 participants
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.
Summary by CodeRabbit