GUI rebuild: Expo Router app replaces Tauri/Svelte desktop UI#3
Conversation
- Scaffold transplant from jiten donor (Expo Router + Zustand + NativeWind + Clerk LOCAL_MODE) - Add /plans GET/PUT endpoints to metadata-server with 8 tests - Build sidebar + chat + plan editor + threads + graveyard screens - HTTP client, EventSource heartbeat, image picker (web/native) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove Svelte + Tauri implementation now superseded by app/ - Strip @tauri-apps/api and @xterm/* from root package.json - Update root README and CLAUDE.md to describe RN/Expo client - Mark desktop-ui-contract and desktop-shell-phase1-spec docs as superseded Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Migrate stores from Zustand to Jotai (mirror jiten patterns) - Add atomWithStorage-backed themePreference, default dark - Add useThemeEffect + React Navigation ThemeProvider wrap - Add settings screen with theme picker, sidebar footer link - Configure metro to resolve jotai/jotai-optics ESM to CJS on web Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the TUI's information architecture. Fetches /desktop-state from the per-project metadata server (every 2s, refresh-nonce on mutation), groups sessions + services into worktree buckets, renders the tree in both the sidebar and the main panel. Persists selected project across reloads. - Sidebar: project picker ↔ tree view with inline Stop/Resume/Remove - Main panel: same tree as cards, with denser detail (headline, path) - Service detail screen at /(main)/service/[serviceId] - Shared ServiceActions component + status-tone helpers - New api wrappers for agents/services/worktrees CRUD Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Push type ladder: project title 28px / worktree 18px / row 15px / section heading 11px uppercase tracking-widest - Worktree groups now render as real cards with bg-card lift + colored left rail (emerald for main checkout, sky for feature worktrees) - New StatusDot + StatusPill components replace the `●` text glyph; status reads at a glance via color + uppercase pill - Branch shown as a chip with GitBranch icon; ellipsize-middle on long branches and paths so titles never wrap - Bump dark-mode --card / --border / --secondary tokens so card boundaries are actually visible against the bg - Widen sidebar to w-80; add "PROJECT" eyebrow above the project name - Section headings now show counts: "Agents · 3", "Services · 1" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hand-rolled card + icon-button wrappers were inconsistent with the existing shadcn-style primitives at components/ui/. Wire everything through Card / PressableCard / Button so radius, borders, and active states are standardized. - WorktreeGroup outer → Card (sidebar + main panel) - AgentCard → PressableCard - ServiceCard → Card with internal Pressable for body click - ServiceActions: Stop/Resume/Remove buttons → Button variant="outline" size="icon" with compact=h-7w-7 (sidebar) / default h-9w-9 (panel) - Service detail screen: header restructured with eyebrow + ServiceActions cluster, status row + Card body for metadata Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Responsive shell with three breakpoints: persistent sidebar on desktop (≥1024), togglable sidebar on tablet (640–1023), animated drawer + bottom tab bar on mobile (<640). Auth menu supports LOCAL_MODE badge, sign-in redirect, and avatar dropdown with full-window backdrop dismiss. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The instance-registry tests create a tmp git repo via execSync. When yarn test runs under a git hook (pre-push, pre-commit), git sets GIT_DIR in the environment; execSync inherits it, so `cwd: dir` is ignored and test commits leak into the outer repo as empty "init" commits on the current branch. Strip GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/GIT_OBJECT_DIRECTORY/GIT_COMMON_DIR before invoking git in the helper.
resolveRepoRoot (paths.ts) and findMainRepo (worktree.ts) both shell out to `git` with `cwd` set, but inherited GIT_DIR/GIT_WORK_TREE makes git ignore cwd. Tests that pass a temp cwd ended up resolving to the outer repo when yarn test ran under a git hook (pre-push, pre-commit), causing ~30 unrelated multiplexer test failures and cross-test contamination of this repo's .aimux/instances.json. Strip GIT_* env in both call sites. In normal production usage (no GIT_DIR set), behavior is identical.
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy Tauri/Svelte desktop UI with a new Expo Router app (app/) that targets web + iOS + Android, and updates the Node backend to be more robust under inherited git-hook GIT_* environments while adding HTTP plan read/write endpoints to the metadata server.
Changes:
- Remove the legacy desktop client (
desktop-ui/,src-tauri/) and related dependencies/scripts. - Add a new Expo Router client (
app/) with state management, theming, auth scaffolding, and daemon/metadata-server HTTP + SSE integration. - Harden git-root resolution against inherited
GIT_DIR/GIT_WORK_TREEcontamination; add/plans/:sessionIdGET/PUT endpoints and tests.
Reviewed changes
Copilot reviewed 103 out of 113 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Removes desktop/Tauri-related package lock entries. |
| package.json | Removes desktop dev/build scripts and Tauri/Xterm deps. |
| README.md | Updates docs to describe the new app/ client and HTTP surfaces. |
| CLAUDE.md | Updates contributor guidance for the new Expo client workflows. |
| docs/desktop-ui-contract.md | Marks legacy desktop contract as superseded (historical). |
| docs/desktop-shell-phase1-spec.md | Marks legacy desktop shell spec as superseded (historical). |
| src/paths.ts | Strips inherited GIT_* env when resolving repo root via git. |
| src/worktree.ts | Strips inherited GIT_* env for main-repo discovery git calls. |
| src/metadata-server.ts | Adds /plans/:sessionId GET/PUT endpoints with validation and atomic write. |
| src/metadata-server.test.ts | Adds test coverage for the new plans endpoints. |
| src/instance-registry.test.ts | Uses git helper that strips inherited GIT_* env to prevent test contamination. |
| src-tauri/tauri.conf.json | Removes legacy Tauri configuration. |
| src-tauri/gen/schemas/capabilities.json | Removes generated Tauri schema artifact. |
| src-tauri/Cargo.toml | Removes legacy Rust/Tauri crate manifest. |
| src-tauri/capabilities/default.json | Removes legacy Tauri capability configuration. |
| src-tauri/build.rs | Removes legacy Tauri build script. |
| desktop-ui/vite.config.js | Removes legacy Svelte/Vite desktop UI config. |
| desktop-ui/svelte.config.js | Removes legacy Svelte config. |
| desktop-ui/index.html | Removes legacy desktop UI entry HTML. |
| desktop-ui/package.json | Removes legacy desktop UI package manifest. |
| desktop-ui/src/main.js | Removes legacy Svelte bootstrap. |
| desktop-ui/src/styles.css | Removes legacy desktop UI global styles. |
| desktop-ui/src/App.svelte | Removes legacy desktop shell app component. |
| desktop-ui/src/lib/WorkspaceHeader.svelte | Removes legacy desktop header component. |
| desktop-ui/src/lib/TerminalPanel.svelte | Removes legacy embedded terminal panel. |
| desktop-ui/src/lib/terminal-instance.svelte.js | Removes legacy shared Xterm instance wiring. |
| desktop-ui/src/lib/StatusBar.svelte | Removes legacy status/tab bar component. |
| desktop-ui/src/lib/Sidebar.svelte | Removes legacy project sidebar component. |
| desktop-ui/src/lib/sessionSemantic.js | Removes legacy session semantic helpers. |
| desktop-ui/src/lib/SessionPanel.svelte | Removes legacy sessions list panel. |
| desktop-ui/src/lib/PlansPanel.svelte | Removes legacy plan editor panel (Tauri invoke-based). |
| desktop-ui/src/lib/GraveyardPanel.svelte | Removes legacy graveyard UI panel. |
| desktop-ui/src/lib/ActivityPanel.svelte | Removes legacy workflow/activity UI panel. |
| app/package.json | Adds Expo app dependencies and scripts (web/ios/android, EAS build/update, lint/test). |
| app/app.config.js | Expo app configuration (name/ids/updates/plugins). |
| app/babel.config.js | Babel preset config enabling NativeWind JSX transform. |
| app/metro.config.js | Metro config with NativeWind + Jotai web bundling workaround. |
| app/tsconfig.json | TypeScript config extending Expo defaults with strict mode and path alias. |
| app/vitest.config.ts | Vitest config for app-level tests + coverage setup. |
| app/eslint.config.mjs | ESLint configuration for the app (TS + react-hooks + prettier). |
| app/tailwind.config.js | Tailwind/NativeWind config with CSS variable color mapping. |
| app/global.css | Global web CSS (theme vars, scrollbars, navbar backdrop). |
| app/.prettierrc | Prettier formatting config for the app workspace. |
| app/.gitignore | App workspace ignore rules (Expo/native build artifacts, env files, coverage). |
| app/nativewind-env.d.ts | NativeWind type reference shim. |
| app/environment.d.ts | Declares EXPO_PUBLIC_* env typings for TS. |
| app/eas.json | EAS build/submit profiles and channels. |
| app/scripts/build.sh | EAS build wrapper script for TestFlight/Production. |
| app/scripts/update.sh | EAS OTA update wrapper script for channels. |
| app/scripts/version-manager.sh | Version bump/rollback script that updates version + iOS build metadata. |
| app/scripts/check-release-env.js | Release-env consistency checker for EXPO_PUBLIC_* keys and sources. |
| app/lib/version.ts | Generated version metadata consumed by build tooling. |
| app/lib/utils.ts | Utility cn() helper combining clsx + tailwind-merge. |
| app/lib/jotai-storage.ts | SSR-safe JSON storage wrapper for Jotai persistence. |
| app/lib/theme-effect.ts | Theme preference effect bridging NativeWind and web document class. |
| app/lib/status-tone.ts | Shared status→tone mappings and command token helper. |
| app/lib/envContract.js | Canonical list of supported EXPO_PUBLIC_* env keys. |
| app/lib/envRuntime.ts | Centralized raw env getters. |
| app/lib/env.ts | Validated env facade used by app code. |
| app/lib/daemon-url.ts | Resolves daemon base URL and service endpoint base URLs. |
| app/lib/api.ts | Typed HTTP API client for daemon + metadata server endpoints. |
| app/lib/events.ts | Client-side SSE event type taxonomy used by the Expo app. |
| app/lib/heartbeat.ts | EventSourcePolyfill wrapper for metadata-server /events SSE. |
| app/lib/desktop-state.ts | Client-side /desktop-state types + worktree grouping helper. |
| app/lib/auth.tsx | Clerk/local-mode auth provider + hooks and LOCAL_MODE behavior. |
| app/lib/image-picker.d.ts | Platform-independent image picker type contract. |
| app/lib/image-picker.web.ts | Web image picker implementation using <input type=file> + base64 conversion. |
| app/lib/image-picker.native.ts | Native image picker implementation using expo-image-picker. |
| app/stores/ui.ts | Jotai atoms for theme preference + sidebar state. |
| app/stores/projects.ts | Jotai atoms for projects list/selection + reconcile logic. |
| app/stores/desktopState.ts | Jotai atom families for per-project desktop-state and derived worktree groups. |
| app/stores/chat.ts | Jotai atom families and reducers for chat history/pending/output/streaming. |
| app/components/ui/text.tsx | Base Text component with shared typography defaults. |
| app/components/ui/input.tsx | Styled text input component. |
| app/components/ui/button.tsx | Variant/size button component using CVA + NativeWind classes. |
| app/components/ui/card.tsx | Card + pressable card primitives. |
| app/components/ui/badge.tsx | Badge primitive with variants. |
| app/components/ui/separator.tsx | Horizontal/vertical separator component. |
| app/components/ui/segmented-control.tsx | Segmented control component for settings/theme selection. |
| app/components/TopBar.tsx | App chrome top bar with branding and auth menu. |
| app/components/AuthMenu.tsx | Auth menu supporting local-mode badge and Clerk sign-in/out paths. |
| app/components/AppShell.tsx | Responsive layout shell (desktop sidebar, tablet drawer, mobile tab bar). |
| app/components/MobileTabBar.tsx | Bottom tab bar navigation for mobile. |
| app/components/status-dot.tsx | Shared status dot + status pill UI primitives. |
| app/components/service-actions.tsx | Inline service lifecycle action buttons (stop/resume/remove) + refresh kick. |
| app/components/MessageBlock.tsx | Chat message renderer including image parts and delivery-state indicators. |
| app/components/ChatComposer.tsx | Chat input composer with attachment upload + pending message tracking. |
| app/app/index.tsx | Root redirect into the main app group. |
| app/app/_layout.tsx | Root layout wiring: theme provider, auth provider, auth gate, global CSS. |
| app/app/sign-in.tsx | Clerk sign-in screen (incl. verification flow). |
| app/app/sign-up.tsx | Clerk sign-up screen (incl. email verification flow). |
| app/app/(main)/_layout.tsx | Main layout: polling projects + desktop-state and rendering within AppShell. |
| app/app/(main)/index.tsx | Dashboard screen rendering worktree buckets and agent/service cards. |
| app/app/(main)/agent/[sessionId]/chat.tsx | Agent chat screen: history fetch + SSE subscription + composer. |
| app/app/(main)/plans/[sessionId].tsx | Plan editor screen backed by metadata-server /plans endpoints. |
| app/app/(main)/service/[serviceId].tsx | Service detail screen with lifecycle controls. |
| app/app/(main)/threads.tsx | Threads listing screen (list-only for v1). |
| app/app/(main)/graveyard.tsx | Graveyard listing screen (list-only for v1). |
| app/app/(main)/settings.tsx | Settings screen (theme preference selection). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { APP_VERSION } = require("./lib/version.ts"); | ||
|
|
There was a problem hiding this comment.
By design — Expo CLI loads app.config.* via jiti, which transparently handles .ts requires. The current setup works in yarn web/yarn build:testflight. Happy to convert to app.config.ts later if we ever need to invoke this file from plain Node, but it's not blocking today.
| <View className="relative"> | ||
| <Pressable | ||
| className="h-8 w-8 items-center justify-center rounded-full bg-secondary" | ||
| onPress={() => setOpen((v) => !v)} | ||
| > | ||
| <Text className="text-xs font-semibold text-secondary-foreground"> | ||
| {initialsFromUser(user)} | ||
| </Text> | ||
| </Pressable> | ||
|
|
||
| {open ? ( | ||
| <> | ||
| <Pressable | ||
| onPress={close} | ||
| style={[ | ||
| { position: "absolute", top: 0, left: 0, right: 0, bottom: 0, zIndex: 40 }, | ||
| Platform.OS === "web" ? ({ position: "fixed" } as object) : undefined, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Fixed in 535edf1 — wrapped the native menu in <Modal transparent> so the click-away Pressable reaches the screen root. Web keeps the existing position: fixed overlay.
| if ( | ||
| platform === "web" && | ||
| result?.type === "sourceFile" && | ||
| result.filePath.includes("/jotai") && | ||
| result.filePath.endsWith(".mjs") | ||
| ) { | ||
| const cjsPath = result.filePath.replace(/\/esm\/(.+)\.mjs$/, "/$1.js"); |
There was a problem hiding this comment.
Fixed in db9e217 — replaced the /jotai includes check and the .mjs → .js regex with separator-agnostic patterns (/[\\\\/]jotai/, /[\\\\/]esm[\\\\/]…/) and preserve the host separator in the rewritten path so Windows hosts trigger the rewrite too.
| /* Override prefers-reduced-motion — this app is too interactive to function without animations */ | ||
| @media (prefers-reduced-motion: reduce) { | ||
| *, | ||
| *::before, | ||
| *::after { | ||
| animation-duration: revert !important; | ||
| animation-delay: revert !important; | ||
| transition-duration: revert !important; | ||
| transition-delay: revert !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
By design — chat threads, plan editor, and sidebar transitions are load-bearing for orienting between states (without them, layout changes look like full repaints). The intent is to reduce the number of animations over time and respect the OS pref globally rather than carve out per-rule exceptions today. Tracking that as a follow-up.
| // ── Daemon (port 9876) ──────────────────────────────────────────────────── | ||
|
|
||
| export interface DaemonHealth { |
There was a problem hiding this comment.
Looks like this caught a phantom reference — line 52 is inside an error handler and has no port comment. The actual port comment is at line 75 and correctly says port 43190 (matches daemon-url.ts and the README). Nothing to change here.
There was a problem hiding this comment.
Correction to my previous reply: you were right. Fixed in 3c5e155 — bumped the comment to port 43190 to match daemon-url.ts and the README. I was looking at the wrong branch when I marked it as a phantom reference.
| const sessionId = validation.value; | ||
| const planPath = join(getPlansDir(), `${sessionId}.md`); | ||
| try { | ||
| const content = readFileSync(planPath, "utf8"); | ||
| send(res, 200, { ok: true, sessionId, path: planPath, content }); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
Fixed in 4b25dd2 — dropped path from the GET response and updated PlanResponse in app/lib/api.ts to match. The app only reads .content, so the field was unused and just leaked the daemon's local layout.
| const planPath = join(getPlansDir(), `${sessionId}.md`); | ||
| const dir = dirname(planPath); | ||
| mkdirSync(dir, { recursive: true }); | ||
| const tmpPath = join(dir, `.${sessionId}.${randomUUID()}.tmp`); | ||
| writeFileSync(tmpPath, body.content, "utf8"); | ||
| renameSync(tmpPath, planPath); | ||
| send(res, 200, { ok: true, sessionId, path: planPath }); |
There was a problem hiding this comment.
Fixed in 4b25dd2 alongside the GET response — path removed from the PUT body, putPlan return type updated, tests reconstruct the path locally where they still need it.
The click-away Pressable was absolute-positioned inside a relative wrapper, so on native it only covered the small avatar container. Web kept working because position: fixed promoted it to viewport, but native left the menu effectively undismissable by tapping outside. Wrap the native menu in a Modal so the overlay reaches the screen root; web keeps the existing position: fixed overlay.
The Metro resolveRequest hook matched paths with literal "/" separators, so on Windows (backslash-separated filePaths) the .mjs → .js rewrite never fired and Jotai's ESM would still break the web bundle. Match either separator and preserve the host separator in the rewritten CJS path.
GET and PUT /plans returned the on-disk path. The app only reads .content (getPlan callers ignore .path), so the field was unused and leaked the daemon's local directory structure to any HTTP client. Drop it from the server responses and matching client types; tests reconstruct the path locally where they still need it.
The section header said port 9876 but the daemon has used 43190 since the HTTP surface landed (see daemon-url.ts and README). Caught by Copilot on PR #3.
| if (!result.accepted) { | ||
| const msg = result.error ?? "The agent input operation failed."; | ||
| updatePending({ | ||
| sessionId, | ||
| clientMessageId, | ||
| patch: { deliveryState: "failed", deliveryError: msg }, | ||
| }); | ||
| setError(msg); | ||
| return; | ||
| } | ||
| updatePending({ | ||
| sessionId, | ||
| clientMessageId, | ||
| patch: { deliveryState: (result.operation?.state as "submitted") ?? "submitted" }, | ||
| }); |
There was a problem hiding this comment.
Fixed in 1b76463 — the server operation state union is queued|applied|submitted|failed; map anything not failed to the UI's submitted instead of casting.
| const refreshNonce = useAtomValue(desktopStateRefreshNonceAtom); | ||
| const store = useStore(); | ||
| const { getToken } = useAuth(); | ||
| const tokenRef = useRef<string | null>(null); | ||
|
|
||
| // Poll /projects every 2s; reconcile into the projects atom. | ||
| useEffect(() => { | ||
| let cancelled = false; | ||
| let timer: ReturnType<typeof setTimeout> | null = null; | ||
|
|
||
| async function loop() { | ||
| if (cancelled) return; | ||
| try { | ||
| const token = await getToken(); | ||
| if (!cancelled) tokenRef.current = token; | ||
| const projects = await listProjects({ token }); | ||
| if (!cancelled) reconcileProjects(projects); |
There was a problem hiding this comment.
Fixed in 1b76463 — tokenRef removed. The polling loop already had the token in its closure scope; the ref was just dead state.
| const [token, setToken] = useState<string | null>(null); | ||
| const [loadingHistory, setLoadingHistory] = useState(false); | ||
| const scrollRef = useRef<ScrollView>(null); | ||
| const lastEventTsRef = useRef<string | null>(null); |
There was a problem hiding this comment.
Fixed in 1b76463 — lastEventTsRef removed. There's no resume/dedup logic that consumes it; the heartbeat client handles reconnect ordering on its own.
ChatComposer was casting the server operation state ("queued"|"applied"|
"submitted"|"failed") directly into PendingMessage.deliveryState, which
only allows "sending"|"submitted"|"failed". Anything not "failed" is
accepted from the UI's perspective, so map non-failed states to
"submitted" explicitly.
Also removes two write-only refs flagged by review:
- tokenRef in (main)/_layout.tsx (token was held in the polling-loop
closure already)
- lastEventTsRef in chat.tsx (no resume/dedup logic actually read it)
| mkdirSync(dir, { recursive: true }); | ||
| const tmpPath = join(dir, `.${sessionId}.${randomUUID()}.tmp`); | ||
| writeFileSync(tmpPath, body.content, "utf8"); | ||
| renameSync(tmpPath, planPath); | ||
| send(res, 200, { ok: true, sessionId }); |
There was a problem hiding this comment.
Node's fs.renameSync atomically replaces existing regular files on all supported platforms — POSIX has always behaved this way and on Windows Node uses MoveFileExW with MOVEFILE_REPLACE_EXISTING (this has been the documented behavior for many years now). The Node docs explicitly say "In the case that newPath already exists, it will be overwritten". Only an existing directory at newPath would error, and plan paths are always regular files.
| # Update iOS CURRENT_PROJECT_VERSION in pbxproj (same as tealstreet-mobile) | ||
| if [ -f "$PBXPROJ" ]; then | ||
| echo "📱 Updating iOS CURRENT_PROJECT_VERSION → $new_build..." | ||
| sed -i '' "s/CURRENT_PROJECT_VERSION = [0-9]*;/CURRENT_PROJECT_VERSION = $new_build;/g" "$PBXPROJ" | ||
| fi |
There was a problem hiding this comment.
By design — this script is the iOS/EAS build helper and already depends on /usr/libexec/PlistBuddy (macOS-only). The whole script only runs on the developer's macOS box doing yarn build:testflight / yarn build:production, never in Linux CI. BSD sed -i '' is consistent with that scope.
| runtimeVersion: "1.0.0", | ||
| updates: { | ||
| url: "https://u.expo.dev/00000000-0000-0000-0000-000000000000", | ||
| enabled: true, | ||
| checkAutomatically: "ON_LOAD", | ||
| fallbackToCacheTimeout: 30000, | ||
| }, | ||
| plugins: ["expo-router", "expo-updates", "expo-image-picker"], | ||
| experiments: { | ||
| typedRoutes: true, | ||
| }, | ||
| extra: { | ||
| router: {}, | ||
| eas: { | ||
| projectId: "00000000-0000-0000-0000-000000000000", | ||
| }, |
There was a problem hiding this comment.
Fixed in d9483a7 — EAS_PROJECT_ID is now read from env. While it remains the all-zero placeholder, updates.enabled is false and expo-updates is dropped from the plugin list, so dev / web builds don't try to fetch from a bogus URL. Real EAS builds set the env (via eas init or build config) and OTA wiring comes back automatically.
Previously app.config.js shipped a literal all-zero UUID for both extra.eas.projectId and the updates.url path, with updates.enabled: true and expo-updates plugged in. On any launch the runtime would try to hit \`https://u.expo.dev/00000000-…\` and the placeholder projectId would fail various Expo/EAS lookups. Read EAS_PROJECT_ID from env (set by \`eas init\` or build env). When it's still the placeholder, leave updates disabled and drop the expo-updates plugin so the build doesn't try to wire OTA at all. Real EAS builds set the env and get updates back automatically.
| useEffect(() => { | ||
| if (!endpoint) return; | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| const token = await getToken(); | ||
| const data = (await listThreads(endpoint, undefined, { token })) as ThreadSummary[]; | ||
| if (!cancelled) setThreads(Array.isArray(data) ? data : []); | ||
| } catch (err) { | ||
| if (!cancelled) setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Fixed in bedb55e — both effects now clear error at the start of each fetch and on success, and reset threads+error to empty when endpoint goes null (project switch / host offline). No more stale banners across projects or after a recovered failure.
| useEffect(() => { | ||
| if (!endpoint) return; | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| const token = await getToken(); | ||
| const data = (await listGraveyard(endpoint, { token })) as | ||
| | GraveyardEntry[] | ||
| | { entries?: GraveyardEntry[] }; | ||
| if (cancelled) return; | ||
| if (Array.isArray(data)) { | ||
| setEntries(data); | ||
| } else if (Array.isArray(data?.entries)) { | ||
| setEntries(data.entries); | ||
| } else { | ||
| setEntries([]); | ||
| } | ||
| } catch (err) { | ||
| if (!cancelled) setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Fixed in bedb55e — same pattern as the threads screen: clear error on each new fetch and on success, reset entries+error when endpoint is falsy.
Both screens had two issues flagged by review: 1. When the endpoint went null (project switch, host offline), the previous project's data + error banner stayed on screen. 2. A transient fetch failure would set the error banner but no later successful refresh ever cleared it. Reset entries/threads + error to empty when endpoint is falsy, and clear error at the start of each fetch and on success so banners match the current fetch's outcome instead of a historical one.
| if (!isLoaded) return; | ||
| if (isSignedIn && onAuthScreen) { | ||
| router.replace("/"); | ||
| } |
There was a problem hiding this comment.
Line 23 already handles this: else if (!isSignedIn && !LOCAL_MODE && !onPublicScreen) router.replace("/landing") redirects unauthenticated users to the landing page in non-local mode.
| // Strip inherited GIT_* env so the command honors `cwd` even when invoked | ||
| // from a context (e.g. git hook) where the parent set GIT_DIR/GIT_WORK_TREE. | ||
| const env = { ...process.env }; | ||
| delete env.GIT_DIR; | ||
| delete env.GIT_WORK_TREE; | ||
| delete env.GIT_INDEX_FILE; | ||
| delete env.GIT_OBJECT_DIRECTORY; | ||
| delete env.GIT_COMMON_DIR; | ||
| const gitCommonDir = execSync("git rev-parse --git-common-dir", { | ||
| cwd, | ||
| env, | ||
| encoding: "utf-8", |
There was a problem hiding this comment.
By design — paths.ts and worktree.ts have separate GIT_* stripping to avoid circular imports (paths.ts is a low-level module imported by worktree.ts). Consolidating into a shared utility would require a new file that both import, which adds complexity for a 5-line inline pattern.
| if (req.method === "PUT" && url.pathname.startsWith("/plans/")) { | ||
| let raw: string; | ||
| try { | ||
| raw = decodeURIComponent(url.pathname.slice("/plans/".length)); | ||
| } catch { | ||
| send(res, 400, { ok: false, error: "invalid sessionId" }); | ||
| return; |
There was a problem hiding this comment.
Fixed in f679c88 — added CORS headers (Access-Control-Allow-Origin/Methods/Headers) to every response and OPTIONS preflight handler returning 204.
The web client (Metro on :8081) makes cross-origin requests to the metadata server on a different port. PUT /plans triggers a preflight that previously got no response. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
📝 WalkthroughWalkthroughAdds a new Expo Router mobile/web client (app/), typed HTTP + SSE client and heartbeat, Clerk/local auth adapter, Jotai stores, UI components and screens, build/release/versioning tooling, platform image pickers, and metadata-server plan endpoints; removes legacy desktop/Tauri UI and related bundles. ChangesApp + Server (single reviewer narrative)
Sequence Diagram(s): (skipped) Estimated code review effort:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
✅ Actions performedReview triggered.
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/stores/chat.ts (1)
1-112:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMigrate this store from Jotai to Zustand to satisfy repository store contracts.
Line 1 and the rest of this module implement chat state via Jotai atoms, but
app/stores/**is required to use Zustand with distinct domain stores. This should be aligned before merge to avoid architecture drift across the new app state layer.As per coding guidelines
app/stores/**/*.ts:app/stores/must use Zustand for state management with separate stores forprojects(list + selection),chat(per-session history, pending, output, streaming), andui(sidebar).🤖 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 `@app/stores/chat.ts` around lines 1 - 112, This module currently implements chat state with Jotai atoms (chatHistoryFamily, pendingMessagesFamily, outputBufferFamily, streamingFamily, streamTokenFamily, lastErrorFamily and action atoms setHistoryAtom, addPendingAtom, updatePendingAtom, ingestEventAtom); replace it with a Zustand "chat" store that maintains per-session maps for history, pending messages, output buffer, streaming flag and lastError, and exposes methods setHistory(sessionId, messages), addPending(sessionId, pending), updatePending(sessionId, clientMessageId, patch) and ingestEvent(event) that replicate the existing logic (including reconciling pending when setHistory is called and the SSE routing logic in ingestEventAtom). Ensure the new store API matches the repository contracts for app/stores (separate domain stores) and remove Jotai atoms; keep behavior identical for deliveredId filtering, preserving failed pending messages, appending/replacing pending by clientMessageId, and toggling streaming/lastError on "ready"/"alert"/"error" events.
🧹 Nitpick comments (1)
src/worktree.test.ts (1)
66-70: ⚡ Quick winRe-add explicit assertions for sanitized git env in these call checks.
These assertions now validate only
cwd/encoding/stdio, so they no longer verify the new hardening behavior (GIT_*stripping). Add a direct check on the call options’envto prevent silent regressions.Suggested test tightening
expect(execFileSyncMock).toHaveBeenLastCalledWith( "git", ["worktree", "add", target, "-b", "fix-auth"], expect.objectContaining({ cwd: "/repo", encoding: "utf-8", stdio: "pipe" }), ); + const opts1 = execFileSyncMock.mock.calls.at(-1)?.[2] as { env?: Record<string, string | undefined> }; + expect(opts1.env?.GIT_DIR).toBeUndefined(); + expect(opts1.env?.GIT_WORK_TREE).toBeUndefined(); + expect(opts1.env?.GIT_INDEX_FILE).toBeUndefined(); + expect(opts1.env?.GIT_OBJECT_DIRECTORY).toBeUndefined(); + expect(opts1.env?.GIT_COMMON_DIR).toBeUndefined(); @@ expect(execFileSyncMock).toHaveBeenLastCalledWith( "git", ["worktree", "add", target, "fix-auth"], expect.objectContaining({ cwd: "/repo", encoding: "utf-8", stdio: "pipe" }), ); + const opts2 = execFileSyncMock.mock.calls.at(-1)?.[2] as { env?: Record<string, string | undefined> }; + expect(opts2.env?.GIT_DIR).toBeUndefined(); + expect(opts2.env?.GIT_WORK_TREE).toBeUndefined(); + expect(opts2.env?.GIT_INDEX_FILE).toBeUndefined(); + expect(opts2.env?.GIT_OBJECT_DIRECTORY).toBeUndefined(); + expect(opts2.env?.GIT_COMMON_DIR).toBeUndefined();Also applies to: 80-84
🤖 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/worktree.test.ts` around lines 66 - 70, The call-site assertions for execFileSyncMock in src/worktree.test.ts currently only check cwd/encoding/stdio and must be tightened to also assert that the provided options.env has had GIT_* variables stripped; update both call checks (the one around execFileSyncMock in the worktree-add expectation and the similar assertion later at lines ~80-84) to include expect.objectContaining({ env: expect.not.objectContaining({ /* any GIT_* keys */ }) }) or an equivalent assertion that verifies no keys starting with "GIT_" exist on the options.env passed to execFileSyncMock so regressions in environment sanitization are caught.
🤖 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 `@app/app.config.js`:
- Around line 43-49: Replace the hard-coded runtimeVersion string with Expo's
fingerprint policy or derive it from your native build number so OTA updates
won't be applied to incompatible native binaries; specifically, update the
runtimeVersion entry (currently set to "1.0.0") to either use the fingerprint
policy (runtimeVersion: { policy: "fingerprint" }) or set it to the native build
identifier exposed by app/lib/version.ts (use APP_VERSION.buildNumber /
buildNumber) so runtimeVersion reflects native compatibility rather than a fixed
literal.
In `@app/app/`(main)/agent/[sessionId]/chat.tsx:
- Around line 23-24: The code coerces a possibly missing route param into the
string "undefined" by doing String(params.sessionId); update all uses of
useLocalSearchParams and the sessionId variable (where it's used for atom keys
and network calls) to guard against absent params instead of coercing: check
params.sessionId for null/undefined and either throw/redirect or derive a safe
identifier (e.g., use a validated non-empty string or null) before constructing
atom keys or making requests; replace the direct String(...) conversions in the
sessionId assignment and the other occurrences with that guarded logic so you
never propagate the literal "undefined".
- Line 156: The key prop currently builds a string that can become "undefined"
when m.id or m.clientMessageId are missing; change the key expression used in
the message render (the prop referencing m.id, m.clientMessageId and idx) to use
nullish coalescing (??) instead of || so it falls back only when values are null
or undefined, and ensure the final fallback includes idx (e.g., use m.id ??
m.clientMessageId ?? `idx-${idx}`) so React keys are stable and not the literal
"undefined".
In `@app/app/`(main)/graveyard.tsx:
- Line 24: The useEffect hooks that refetch the graveyard currently only depend
on endpoint host/port and therefore can show stale data when the user switches
projects that map to the same endpoint; update the dependency arrays for the
useEffect(s) (the hooks starting with useEffect(() => { ... }) around the
graveyard fetching logic) to also include the project selection identifier used
in the component (e.g., project.id, selectedProject, or projectRef) so the
effect reruns when the selected project changes; locate the
fetch/graveyard-loading logic inside those useEffect hooks and add the
appropriate project key to their dependency arrays (this applies to both
occurrences noted).
In `@app/app/`(main)/plans/[sessionId].tsx:
- Around line 12-13: Don't coerce a potentially missing route param into the
string "undefined"; instead check params.sessionId from useLocalSearchParams and
guard it before calling getPlan/putPlan. Replace the String(params.sessionId)
usage (sessionId) with a presence check: if params.sessionId is null/undefined,
return early or trigger a proper error/redirect, otherwise use the validated
value (or parse it to the expected type) when calling getPlan and putPlan; apply
the same guard to the other occurrences around the blocks indicated (the
sessionId uses at the other locations referenced).
In `@app/app/`(main)/service/[serviceId].tsx:
- Around line 41-42: The code currently coerces params.serviceId into a String
which turns a missing param into the literal "undefined"; update the parsing
around useLocalSearchParams and the serviceId binding so you explicitly check
for params.serviceId (e.g., if params.serviceId is null/undefined, handle it by
throwing, returning a 404/redirect, or using undefined/empty string) instead of
using String(params.serviceId); locate where useLocalSearchParams and the
serviceId constant are used (also where serviceId is consumed later around the
second occurrence) and ensure callers handle a missing id rather than receiving
the string "undefined".
In `@app/app/`(main)/settings.tsx:
- Around line 3-6: The Settings component is wired to Jotai
(useAtom(themePreferenceAtom)) but the app expects the shared ui store to be a
Zustand store; replace the Jotai usage with the Zustand store API: stop
importing useAtom and themePreferenceAtom from "`@/stores/ui`" and instead import
the Zustand hook/selector (e.g. useUiStore or the exported selector/getter and
setter names) and read/write the theme preference via that hook (read current
preference and call the store's setter/action to update it) inside the same
component (settings.tsx) so SegmentedControl uses the Zustand value and its
change handler calls the store's update method (reference the existing symbols
themePreferenceAtom and useAtom in the file to locate the code to change and the
component name Settings/SegmentedControl to wire the new hook).
In `@app/app/`(main)/threads.tsx:
- Around line 39-41: When the fetch in threads.tsx fails the catch currently
only calls setError, leaving previously loaded threads visible; update the catch
block so that when not cancelled you both call setError(err instanceof Error ?
err.message : String(err)) and clear stale data by calling setThreads([]) (or
null if that's the component's empty state), ensuring the UI doesn't show old
threads after a failed refresh; reference the existing cancelled variable and
the setError and setThreads state setters when making the change.
In `@app/components/ChatComposer.tsx`:
- Around line 78-81: The send path currently allows messages to be submitted
while attachments are still uploading; update the send guard in handleSend (and
the alternate send handler around the other send trigger referenced at line 191)
to also check the uploading flag and return early if uploading is true, i.e.,
treat uploading like sending so messages cannot be sent until uploads complete;
locate uses of draft, draftImages, sending and uploading in ChatComposer.tsx
(e.g., function handleSend and the other send entry) and add the uploading check
to their early-return conditions and disable the send UI while uploading.
In `@app/components/MobileTabBar.tsx`:
- Around line 28-31: MobileTabBar's Pressable always calls router.push(route)
causing duplicate Stack entries; change the onPress handler in the Pressable
inside the MobileTabBar component to guard against pressing the already active
tab (check the computed active variable and return early) or use
router.replace(route) instead of router.push(route) so repeated taps don't push
identical routes onto the Stack (update the Pressable onPress logic referencing
the active variable, the Pressable element, and router.push/router.replace).
In `@app/components/service-actions.tsx`:
- Around line 3-12: The component currently uses Jotai-specific useSetAtom and
kickDesktopStateRefreshAtom (useSetAtom, kickDesktopStateRefreshAtom) but the
app stores now follow the Zustand contract; remove the useSetAtom import and
kickDesktopStateRefreshAtom usage and instead import the Zustand desktop store
hook/action (e.g., useDesktopStore or desktopStore) and call the store's refresh
action (e.g., kickRefresh(), incrementRefreshCounter(), or the store method that
triggers a refresh) inside the handlers that currently call the atom setter;
also update the imports to remove Jotai and import the correct Zustand store
symbol and ensure types align with removeService/resumeService/stopService
callers.
In `@app/global.css`:
- Around line 10-20: The CSS block using the `@media` (prefers-reduced-motion:
reduce) rule that targets *, *::before, *::after should not force animations to
run; instead remove or restrict this global override and only re-enable
animations for explicitly essential UI elements (e.g., by creating an opt-in
class like .motion-essential applied to specific components) or honor the system
preference by keeping the rule that sets animation/transition-duration to 0/none
for reduced-motion; update app/global.css to eliminate the blanket revert for
all elements and implement either per-component opt-in (using a
.motion-essential selector) or an in-app toggle that adds such a class when the
user consents.
In `@app/lib/api.ts`:
- Around line 94-100: The function ensureProject currently returns
Promise<unknown>; define and export a concrete response interface (e.g.,
EnsureProjectResponse) representing the JSON shape returned by the daemon for
POST /projects/ensure, change ensureProject signature to return
Promise<EnsureProjectResponse>, and pass that type into callJson's generic so
callers get proper typing; do the same for the other functions noted (around the
438-455 region) by creating and exporting corresponding response interfaces and
using them as the return types and callJson generic parameters (reference the
ensureProject function and the other exported functions in that region when
applying the same pattern).
In `@app/lib/envContract.js`:
- Around line 11-13: The union that builds allKnownEnvKeys omits
buildOnlyEnvKeys so build-only variables aren't included; update the Set
construction for allKnownEnvKeys to include buildOnlyEnvKeys alongside
defaultedRuntimeEnvKeys, optionalRuntimeEnvKeys, and requiredReleaseEnvKeys
(i.e., spread buildOnlyEnvKeys into the new Set used to create allKnownEnvKeys)
so the resulting sorted array contains every known key.
In `@app/lib/heartbeat.ts`:
- Around line 83-95: The onerror handler currently only treats 401/403 as
terminal; update es.onerror to treat any HTTP client error (status >= 400 &&
status < 500) as a non-retriable terminal failure: if such a status is present,
call es.close(), set stopped = true, and invoke onError with a descriptive Error
including the status (preserving the existing 401/403 behavior), so that
non-retriable 4xx responses don't loop reconnecting indefinitely.
In `@app/lib/status-tone.ts`:
- Around line 18-20: firstTokenOf currently splits the input without trimming,
so inputs like " node app.js" produce an empty token; update the firstTokenOf
function to trim the command string first (e.g., call .trim() on the input)
before checking emptiness and before splitting, then return the first element of
the trimmed split; ensure you still handle undefined or all-whitespace inputs by
returning "".
In `@app/scripts/build.sh`:
- Line 47: The build script hardcodes ios in the eas build command, so either
make the script accept a platform argument (e.g., read a PLATFORM env var or a
--platform flag) and pass it into the eas build invocation (replace the
hardcoded "--platform ios" with "--platform \"$PLATFORM\"") defaulting to ios if
unspecified, or provide separate commands for Android (call eas build --platform
android --profile "$EAS_PROFILE" ... using the same EXTRA_ARGS) ; alternatively,
if Android builds are not required, remove the Android section from the
production profile in eas.json so the script and config remain consistent.
In `@app/scripts/check-release-env.js`:
- Around line 14-16: The current logic assigns profile = target === "production"
? "production" : "testflight", which silently maps any unknown arg to
"testflight"; instead validate process.argv[2] (variable target) against an
explicit allowlist and fail fast on unknown values. Replace the ternary with an
explicit check: accept only "production" and "testflight" (set profile
accordingly) and for any other target log an error (console.error or
process.stderr.write) and exit with a non‑zero code (process.exit(1)); update
any downstream use of profile to rely on this validated value.
In `@app/scripts/version-manager.sh`:
- Around line 32-35: The backup only copies VERSION_FILE in create_backups, so
native iOS version changes made by bump-build/set aren’t restored; update
create_backups to also snapshot the iOS native version files (e.g., the
Info.plist(s) and any Xcode project files your bump/set modify) by adding a
list/variable (e.g., IOS_VERSION_FILES) and copying each to .backup
counterparts, and update the rollback/restore function (the rollback or
restore_backups logic referenced around lines ~90-99) to restore those same iOS
backup files alongside lib/version.ts so rollback returns both JS and native iOS
state to the previous values.
In `@src/metadata-server.ts`:
- Around line 1133-1139: In the catch blocks that currently check err?.code ===
"ENOENT" (the one returning 404 and the similar block around the other
occurrence), do not rethrow non-ENOENT errors to be translated into 400; instead
send a 5xx response: if err?.code === "ENOENT" keep send(res, 404, { ok: false,
error: "Plan not found" }) and return; otherwise send(res, 500, { ok: false,
error: "Internal server error", detail: err?.message }) (or a similar 5xx
payload) and return; update both the catch handling around the file read/rename
failures shown and the other catch at the second occurrence so server-side I/O
failures return 5xx rather than falling through.
---
Outside diff comments:
In `@app/stores/chat.ts`:
- Around line 1-112: This module currently implements chat state with Jotai
atoms (chatHistoryFamily, pendingMessagesFamily, outputBufferFamily,
streamingFamily, streamTokenFamily, lastErrorFamily and action atoms
setHistoryAtom, addPendingAtom, updatePendingAtom, ingestEventAtom); replace it
with a Zustand "chat" store that maintains per-session maps for history, pending
messages, output buffer, streaming flag and lastError, and exposes methods
setHistory(sessionId, messages), addPending(sessionId, pending),
updatePending(sessionId, clientMessageId, patch) and ingestEvent(event) that
replicate the existing logic (including reconciling pending when setHistory is
called and the SSE routing logic in ingestEventAtom). Ensure the new store API
matches the repository contracts for app/stores (separate domain stores) and
remove Jotai atoms; keep behavior identical for deliveredId filtering,
preserving failed pending messages, appending/replacing pending by
clientMessageId, and toggling streaming/lastError on "ready"/"alert"/"error"
events.
---
Nitpick comments:
In `@src/worktree.test.ts`:
- Around line 66-70: The call-site assertions for execFileSyncMock in
src/worktree.test.ts currently only check cwd/encoding/stdio and must be
tightened to also assert that the provided options.env has had GIT_* variables
stripped; update both call checks (the one around execFileSyncMock in the
worktree-add expectation and the similar assertion later at lines ~80-84) to
include expect.objectContaining({ env: expect.not.objectContaining({ /* any
GIT_* keys */ }) }) or an equivalent assertion that verifies no keys starting
with "GIT_" exist on the options.env passed to execFileSyncMock so regressions
in environment sanitization are caught.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d16bf367-b40c-4d8e-a689-07821fc47f7d
⛔ Files ignored due to path filters (9)
app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockdesktop-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.locksrc-tauri/Cargo.lockis excluded by!**/*.locksrc-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**src-tauri/icons/icon.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (105)
CLAUDE.mdREADME.mdapp/.gitignoreapp/.prettierrcapp/app.config.jsapp/app/(main)/_layout.tsxapp/app/(main)/agent/[sessionId]/chat.tsxapp/app/(main)/graveyard.tsxapp/app/(main)/index.tsxapp/app/(main)/plans/[sessionId].tsxapp/app/(main)/service/[serviceId].tsxapp/app/(main)/settings.tsxapp/app/(main)/threads.tsxapp/app/_layout.tsxapp/app/index.tsxapp/app/sign-in.tsxapp/app/sign-up.tsxapp/babel.config.jsapp/components/AppShell.tsxapp/components/AuthMenu.tsxapp/components/ChatComposer.tsxapp/components/MessageBlock.tsxapp/components/MobileTabBar.tsxapp/components/ProjectSidebar.tsxapp/components/TopBar.tsxapp/components/service-actions.tsxapp/components/status-dot.tsxapp/components/ui/badge.tsxapp/components/ui/button.tsxapp/components/ui/card.tsxapp/components/ui/input.tsxapp/components/ui/segmented-control.tsxapp/components/ui/separator.tsxapp/components/ui/text.tsxapp/eas.jsonapp/environment.d.tsapp/eslint.config.mjsapp/global.cssapp/lib/api.tsapp/lib/auth.tsxapp/lib/daemon-url.tsapp/lib/desktop-state.tsapp/lib/env.tsapp/lib/envContract.jsapp/lib/envRuntime.tsapp/lib/events.tsapp/lib/heartbeat.tsapp/lib/image-picker.d.tsapp/lib/image-picker.native.tsapp/lib/image-picker.web.tsapp/lib/jotai-storage.tsapp/lib/status-tone.tsapp/lib/theme-effect.tsapp/lib/utils.tsapp/lib/version.tsapp/metro.config.jsapp/nativewind-env.d.tsapp/package.jsonapp/scripts/build.shapp/scripts/check-release-env.jsapp/scripts/update.shapp/scripts/version-manager.shapp/stores/chat.tsapp/stores/desktopState.tsapp/stores/projects.tsapp/stores/ui.tsapp/tailwind.config.jsapp/tsconfig.jsonapp/vitest.config.tsdesktop-ui/index.htmldesktop-ui/package.jsondesktop-ui/src/App.sveltedesktop-ui/src/lib/ActionBar.sveltedesktop-ui/src/lib/ActivityPanel.sveltedesktop-ui/src/lib/GraveyardPanel.sveltedesktop-ui/src/lib/NativeChatPanel.sveltedesktop-ui/src/lib/PlansPanel.sveltedesktop-ui/src/lib/SessionPanel.sveltedesktop-ui/src/lib/Sidebar.sveltedesktop-ui/src/lib/StatusBar.sveltedesktop-ui/src/lib/TerminalPanel.sveltedesktop-ui/src/lib/ThreadsPanel.sveltedesktop-ui/src/lib/WorkspaceHeader.sveltedesktop-ui/src/lib/WorktreePanel.sveltedesktop-ui/src/lib/sessionSemantic.jsdesktop-ui/src/lib/terminal-instance.svelte.jsdesktop-ui/src/main.jsdesktop-ui/src/stores/state.svelte.jsdesktop-ui/src/styles.cssdesktop-ui/svelte.config.jsdesktop-ui/vite.config.jsdocs/desktop-shell-phase1-spec.mddocs/desktop-ui-contract.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/capabilities/default.jsonsrc-tauri/src/main.rssrc-tauri/tauri.conf.jsonsrc/instance-registry.test.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/paths.tssrc/worktree.test.tssrc/worktree.ts
💤 Files with no reviewable changes (28)
- desktop-ui/src/styles.css
- desktop-ui/index.html
- desktop-ui/src/lib/TerminalPanel.svelte
- desktop-ui/vite.config.js
- src-tauri/tauri.conf.json
- desktop-ui/src/lib/ActivityPanel.svelte
- src-tauri/Cargo.toml
- desktop-ui/svelte.config.js
- desktop-ui/src/lib/GraveyardPanel.svelte
- desktop-ui/src/lib/terminal-instance.svelte.js
- desktop-ui/src/main.js
- src-tauri/capabilities/default.json
- desktop-ui/src/lib/StatusBar.svelte
- desktop-ui/src/lib/Sidebar.svelte
- desktop-ui/src/lib/sessionSemantic.js
- desktop-ui/src/App.svelte
- desktop-ui/src/lib/ActionBar.svelte
- desktop-ui/src/lib/SessionPanel.svelte
- package.json
- src-tauri/build.rs
- desktop-ui/src/lib/WorkspaceHeader.svelte
- desktop-ui/src/lib/PlansPanel.svelte
- desktop-ui/src/lib/WorktreePanel.svelte
- desktop-ui/src/stores/state.svelte.js
- desktop-ui/src/lib/ThreadsPanel.svelte
- desktop-ui/package.json
- src-tauri/src/main.rs
- desktop-ui/src/lib/NativeChatPanel.svelte
| res.setHeader("Access-Control-Allow-Origin", "*"); | ||
| res.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, OPTIONS"); | ||
| res.setHeader("Access-Control-Allow-Headers", "Content-Type, Authorization"); |
| function AuthGate({ children }: { children: React.ReactNode }) { | ||
| const { isSignedIn, isLoaded } = useAuth(); | ||
| const segments = useSegments(); | ||
| const router = useRouter(); | ||
| const onAuthScreen = segments[0] === "sign-in" || segments[0] === "sign-up"; | ||
|
|
||
| useEffect(() => { | ||
| if (!isLoaded) return; | ||
| if (isSignedIn && onAuthScreen) { | ||
| router.replace("/"); | ||
| } | ||
| }, [isSignedIn, isLoaded, onAuthScreen, router]); | ||
|
|
||
| if (!isLoaded) return null; | ||
| return <>{children}</>; |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (13)
app/components/MobileTabBar.tsx (1)
28-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid pushing duplicate routes on repeated tab taps.
onPressalways callsrouter.push(route), so tapping the already active tab can add redundant stack entries and degrade back navigation.Suggested change
<Pressable key={route} - onPress={() => router.push(route)} + onPress={() => { + if (active) return; + router.replace(route); + }} className="flex-1 items-center justify-center active:bg-accent/50" >🤖 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 `@app/components/MobileTabBar.tsx` around lines 28 - 31, The Pressable onPress handler in the MobileTabBar component unconditionally calls router.push(route), which can push the current route again; change the handler to first compare the target route with the current route (use the router's current path/route from the routing API your app uses) and only call router.push(route) when different, or use router.replace(route) for idempotent navigation; update the onPress in the Pressable rendering inside MobileTabBar to perform this check (or replace) to avoid duplicate stack entries.app/global.css (1)
10-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor reduced-motion preference instead of globally reverting it.
This block forces animations/transitions back on for all elements even when users request reduced motion, which is an accessibility risk. Remove the global revert (or scope animation opt-in to explicitly essential elements only).
Suggested change
-/* Override prefers-reduced-motion — this app is too interactive to function without animations */ `@media` (prefers-reduced-motion: reduce) { *, *::before, *::after { - animation-duration: revert !important; - animation-delay: revert !important; - transition-duration: revert !important; - transition-delay: revert !important; + animation-duration: 0ms !important; + animation-delay: 0ms !important; + transition-duration: 0ms !important; + transition-delay: 0ms !important; } }🤖 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 `@app/global.css` around lines 10 - 20, The current `@media` (prefers-reduced-motion: reduce) rule that targets *, *::before, *::after and sets animation-duration/animation-delay/transition-duration/transition-delay to revert forces motion back on; remove that global revert and instead honor the preference by disabling non-essential motion: inside `@media` (prefers-reduced-motion: reduce) set animation-duration/animation-delay/transition-duration/transition-delay to 0s (and optionally animation-play-state: paused) for the global selectors, and if some animations are essential, scope re-enablement to a specific class like .motion-essential (use selectors like .motion-essential, .motion-essential * to override the reduced-motion rules) so only explicitly opted-in elements run animations.app/scripts/build.sh (1)
47-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSupport Android target for production builds instead of hardcoding iOS
--platform iospreventsyarn build:productionfrom covering Play builds. Make platform configurable (default iOS) and pass it through toeas build.Proposed fix
set -e CHANNEL="testflight" +PLATFORM="${EAS_PLATFORM:-ios}" EXTRA_ARGS=() @@ -echo "🚀 Starting $CHANNEL_NAME build for iOS..." +echo "🚀 Starting $CHANNEL_NAME build for platform: $PLATFORM..." @@ -eas build --platform ios --profile "$EAS_PROFILE" --auto-submit "${EXTRA_ARGS[@]}" +eas build --platform "$PLATFORM" --profile "$EAS_PROFILE" --auto-submit "${EXTRA_ARGS[@]}"As per coding guidelines,
app/scripts/build.shshould supportyarn build:testflight(iOS default) andyarn build:productionfor App Store / Play production builds.🤖 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 `@app/scripts/build.sh` at line 47, Make the eas build platform configurable instead of hardcoding ios: add a PLATFORM variable with default "ios" (e.g. PLATFORM=${PLATFORM:-ios} or read an optional script arg/env), and replace the hardcoded flag with --platform "$PLATFORM" in the eas build invocation (the line containing eas build --platform ios --profile "$EAS_PROFILE" --auto-submit "${EXTRA_ARGS[@]}"). Ensure callers (yarn build:testflight keeps default ios) and yarn build:production can set PLATFORM=android (or pass an arg) so production builds target Play when desired.app/app/(main)/threads.tsx (1)
39-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale thread data when refresh fails
When fetch fails, old thread rows remain rendered. Clear
threadsin the catch path to avoid showing stale project data.Proposed fix
} catch (err) { - if (!cancelled) setError(err instanceof Error ? err.message : String(err)); + if (!cancelled) { + setThreads([]); + setError(err instanceof Error ? err.message : String(err)); + } }🤖 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 `@app/app/`(main)/threads.tsx around lines 39 - 41, The catch path currently leaves previous thread rows rendered when a fetch fails; update the catch block in the fetch logic (the try/catch around the network call that uses setThreads and setError in threads.tsx) to clear stale data by calling setThreads([]) before setting the error (and only do this when not cancelled), so replace the current catch handling with a branch that, if not cancelled, calls setThreads([]) and then setError(err instanceof Error ? err.message : String(err)).app/app/(main)/service/[serviceId].tsx (1)
41-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse route params defensively to avoid
"undefined"service lookupsCoercing with
String(params.serviceId)can turn a missing param into the literal"undefined", which causes misleading not-found behavior.Proposed fix
- const params = useLocalSearchParams<{ serviceId: string }>(); - const serviceId = String(params.serviceId); + const params = useLocalSearchParams<{ serviceId?: string | string[] }>(); + const serviceId = Array.isArray(params.serviceId) ? params.serviceId[0] : params.serviceId; @@ - const found = useMemo(() => findService(groups, serviceId), [groups, serviceId]); + const found = useMemo( + () => (serviceId ? findService(groups, serviceId) : null), + [groups, serviceId], + );Also applies to: 61-61
🤖 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 `@app/app/`(main)/service/[serviceId].tsx around lines 41 - 42, The route param parsing uses String(params.serviceId) which converts undefined into the literal "undefined"; instead, read params via useLocalSearchParams() then validate presence (e.g., if (!params?.serviceId) return a proper not-found or error response) and only then set const serviceId = params.serviceId; update both occurrences that set serviceId (the lines using useLocalSearchParams and the later occurrence at the other location) to avoid coercing undefined to the string "undefined" and handle the missing param explicitly.app/app/(main)/plans/[sessionId].tsx (1)
12-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid coercing a missing route param into
"undefined"Line 13 can convert an absent
sessionIdinto a truthy string and still hitgetPlan/putPlanwith an invalid identifier.Proposed minimal fix
- const params = useLocalSearchParams<{ sessionId: string }>(); - const sessionId = String(params.sessionId); + const params = useLocalSearchParams<{ sessionId?: string | string[] }>(); + const sessionId = Array.isArray(params.sessionId) ? params.sessionId[0] : params.sessionId; @@ - if (!serviceEndpoint || !sessionId) return; + if (!serviceEndpoint || !sessionId) return; @@ - if (!serviceEndpoint) return; + if (!serviceEndpoint || !sessionId) return; @@ - disabled={saving || !dirty || !serviceEndpoint} + disabled={saving || !dirty || !serviceEndpoint || !sessionId}Also applies to: 27-27, 55-62, 87-87
🤖 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 `@app/app/`(main)/plans/[sessionId].tsx around lines 12 - 13, Don't coerce a possibly-missing route param into the string "undefined"; instead read params.sessionId directly from useLocalSearchParams and guard its presence before using it. Replace the `const sessionId = String(params.sessionId)` pattern with e.g. `const sessionId = params.sessionId` (or `const sessionId = params.sessionId ?? undefined`) and add checks before calling getPlan or putPlan (and any code in the file that currently assumes sessionId exists) to either return/redirect/show an error when sessionId is null/undefined/empty, so getPlan/putPlan only run with a valid non-empty sessionId.app/app.config.js (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded
runtimeVersionwith a native-compatibility strategyUsing a fixed
"1.0.0"can allow incompatible OTA updates across different native builds. Prefer Expo fingerprint policy (or a native build-derived runtime version).Proposed minimal fix
- runtimeVersion: "1.0.0", + runtimeVersion: { policy: "fingerprint" },For Expo EAS Update, is `runtimeVersion: { policy: "fingerprint" }` the recommended way to prevent incompatible OTA updates across native binaries?🤖 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 `@app/app.config.js` at line 43, The hardcoded runtimeVersion ("1.0.0") in app.config.js can allow incompatible OTA updates across native builds; update the runtimeVersion key to use Expo's native-compatibility strategy by replacing runtimeVersion: "1.0.0" with runtimeVersion: { policy: "fingerprint" } (or another native build-derived value) so EAS Update will prevent incompatible OTA installs; locate the runtimeVersion entry in app.config.js and make this replacement while keeping the rest of the config intact.app/lib/heartbeat.ts (1)
83-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat non-retriable 4xx errors as terminal, not transient
Current logic only stops on 401/403. Other 4xx responses can loop reconnect indefinitely.
Proposed minimal fix
- if (status === 401 || status === 403) { + if (typeof status === "number" && status >= 400 && status < 500 && status !== 429) { es.close(); stopped = true; - onError?.(new Error(`heartbeat unauthorized (status ${status})`)); + onError?.(new Error(`heartbeat terminated (status ${status})`)); }🤖 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 `@app/lib/heartbeat.ts` around lines 83 - 95, The onerror handler currently only treats 401/403 as terminal; change the logic in es.onerror to consider any HTTP 4xx (status >= 400 && status < 500) a non-retriable terminal error: if such a status is present, call es.close(), set stopped = true, and invoke onError with a descriptive Error(`heartbeat unauthorized (status ${status})` or similar message that includes the status); keep the existing early return for stopped and leave transient behavior for non-4xx statuses unchanged. Use the existing symbols es.onerror, status, stopped, es.close, and onError to locate and update the code.app/app/(main)/agent/[sessionId]/chat.tsx (2)
23-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t coerce missing route params into the literal
"undefined".Line 24 can propagate
"undefined"into atom keys and API calls whensessionIdis absent (or array-shaped).Suggested fix
- const sessionId = String(params.sessionId); + const sessionId = Array.isArray(params.sessionId) ? params.sessionId[0] : params.sessionId; + if (!sessionId) { + return ( + <View className="flex-1 items-center justify-center bg-background"> + <Text className="text-sm text-muted-foreground">Missing session id.</Text> + </View> + ); + }🤖 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 `@app/app/`(main)/agent/[sessionId]/chat.tsx around lines 23 - 24, The code coerces a possibly missing or array-shaped route param into the literal "undefined" by calling String(params.sessionId); update the logic around useLocalSearchParams and sessionId to validate and normalize the param: explicitly check params.sessionId for undefined/null and for an array case, extract a single string (or bail/throw/redirect) instead of using String(...), and only set sessionId when you have a real string value; reference the useLocalSearchParams call and the sessionId variable to locate where to add the guard and normalization.
156-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish fallback for stable message keys.
Line 156 can still create
"undefined"keys and collisions for messages without ids.Suggested fix
- key={m.id || `${m.clientMessageId}` || `idx-${idx}`} + key={m.id ?? m.clientMessageId ?? `idx-${idx}`}🤖 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 `@app/app/`(main)/agent/[sessionId]/chat.tsx at line 156, The message key expression can still yield "undefined" or collide; replace the current logical-OR chain used where the messages are rendered (the key prop that references m.id, m.clientMessageId and idx) with a nullish-coalescing fallback so you pick the first defined identifier (e.g., use m.id ?? m.clientMessageId ?? `idx-${idx}`) and ensure the chosen value is coerced to a stable string if necessary; update the key prop in the message render where m and idx are in scope.app/scripts/version-manager.sh (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback should restore native iOS version files too.
bump-build/setmutateInfo.plistandproject.pbxproj, but backup/rollback only trackslib/version.ts. Rollback can leave native and JS versions out of sync.Suggested fix
create_backups() { echo "📦 Creating version backup..." cp "$VERSION_FILE" "$VERSION_FILE.backup" + [ -f "$INFO_PLIST" ] && cp "$INFO_PLIST" "$INFO_PLIST.backup" + [ -f "$PBXPROJ" ] && cp "$PBXPROJ" "$PBXPROJ.backup" } rollback_versions() { echo "⏪ Rolling back version..." if [ -f "$VERSION_FILE.backup" ]; then cp "$VERSION_FILE.backup" "$VERSION_FILE" - rm "$VERSION_FILE.backup" + [ -f "$INFO_PLIST.backup" ] && cp "$INFO_PLIST.backup" "$INFO_PLIST" + [ -f "$PBXPROJ.backup" ] && cp "$PBXPROJ.backup" "$PBXPROJ" + rm -f "$VERSION_FILE.backup" "$INFO_PLIST.backup" "$PBXPROJ.backup" echo "✅ Rolled back $VERSION_FILE" else echo "⚠️ No backup found" fi } cleanup_backups() { - rm -f "$VERSION_FILE.backup" + rm -f "$VERSION_FILE.backup" "$INFO_PLIST.backup" "$PBXPROJ.backup" }Also applies to: 90-103
🤖 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 `@app/scripts/version-manager.sh` around lines 32 - 35, create_backups currently only copies VERSION_FILE (lib/version.ts) so native iOS files (Info.plist and project.pbxproj) aren’t backed up; update create_backups to also copy the native files that bump-build/set mutate (e.g., INFO_PLIST_FILE and PBXPROJ_FILE or explicit filenames Info.plist and project.pbxproj) to .backup counterparts, and update the rollback logic to restore those same .backup files alongside VERSION_FILE.backup so JS and native versions remain in sync.app/components/ChatComposer.tsx (1)
78-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock send while attachments are uploading.
Line 81 and Line 191 still permit send during
uploading, so a message can be submitted before selected images finish upload.Suggested fix
async function handleSend() { const trimmed = draft.trim(); if (!trimmed && draftImages.length === 0) return; + if (uploading) return; if (sending) return; @@ <Button label={sending ? "Sending…" : "Send"} onPress={handleSend} - disabled={sending || (!draft.trim() && draftImages.length === 0)} + disabled={sending || uploading || (!draft.trim() && draftImages.length === 0)} />Also applies to: 191-191
🤖 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 `@app/components/ChatComposer.tsx` around lines 78 - 81, handleSend currently allows sending while attachments are still uploading; add a guard to block sends when uploads are in progress. Update the handleSend function (and the other send trigger referenced at the second occurrence) to check the uploading state (e.g., if (uploading) return) before proceeding, and ensure the UI send path that calls the second send handler likewise respects uploading so messages cannot be submitted until all draftImages/uploads finish.src/metadata-server.ts (1)
1133-1139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn 5xx for plan-file I/O failures instead of falling through to 400.
Line 1138 rethrows non-
ENOENT, and Line 1166-Line 1167 can throw during persist. Both currently fall into the outer catch and become400, which misclassifies server-side failures and can break retry behavior.Suggested fix
try { const content = readFileSync(planPath, "utf8"); send(res, 200, { ok: true, sessionId, content }); } catch (err: any) { if (err?.code === "ENOENT") { send(res, 404, { ok: false, error: "Plan not found" }); return; } - throw err; + send(res, 500, { ok: false, error: "Failed to read plan" }); + return; } @@ const dir = dirname(planPath); mkdirSync(dir, { recursive: true }); const tmpPath = join(dir, `.${sessionId}.${randomUUID()}.tmp`); - writeFileSync(tmpPath, body.content, "utf8"); - renameSync(tmpPath, planPath); + try { + writeFileSync(tmpPath, body.content, "utf8"); + renameSync(tmpPath, planPath); + } catch { + send(res, 500, { ok: false, error: "Failed to persist plan" }); + return; + } send(res, 200, { ok: true, sessionId });Also applies to: 1166-1167
🤖 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/metadata-server.ts` around lines 1133 - 1139, Change the error handling so filesystem I/O failures return 5xx instead of falling through to the outer 400 handler: in the try/catch that checks err?.code === "ENOENT" keep ENOENT mapped to send(res, 404, ...), but for any other caught error call send(res, 500, { ok: false, error: "Plan file I/O error" , detail: err.message }) (do not rethrow); apply the same pattern to the code around the persist operation (the block that can throw during persist) so non-ENOENT I/O errors are caught and responded with 5xx instead of propagating to the outer handler.
🤖 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 `@app/app/`(main)/index.tsx:
- Around line 230-235: The useEffect anonymous async IIFE calling getToken may
reject and cause unhandled promise rejections; wrap the await getToken() call in
a try/catch inside the IIFE (or attach .catch) to handle errors, log or surface
the error via your logger/state and avoid calling setToken when cancelled;
specifically update the effect around the getToken invocation in useEffect to
try { const t = await getToken(); if (!cancelled) setToken(t); } catch (err) {
/* log or set error state, optionally setToken(null) */ } so getToken rejections
are handled and no state updates occur after cancelled is true.
In `@app/app/`(main)/service/[serviceId].tsx:
- Around line 50-55: The useEffect's async IIFE calls getToken() without error
handling, risking unhandled promise rejections; wrap the await getToken() call
in a try/catch inside the IIFE, log or set an error state on failure, and only
call setToken(t) if not cancelled (preserve the existing cancelled check);
ensure the existing cleanup still sets cancelled = true so no state updates
occur after unmount.
In `@app/app/sign-in.tsx`:
- Around line 28-39: The sign-in flow currently only handles result.status ===
"complete" and the MFA/client-trust branch (signIn.prepareSecondFactor), leaving
other Clerk statuses unhandled and causing a silent dead-end; update the
handlers around signIn.create / signIn.attemptSecondFactor (the block that
checks result.status and calls setActive, signIn.prepareSecondFactor, and
setNeedsVerification) to include an explicit else branch that stops any loading
state (e.g., setIsLoading(false)), surfaces a user-facing error or info message
including the unexpected result.status (via your existing error/toast state
setter), and logs the full result for debugging so the UI always provides
feedback instead of hanging.
In `@app/components/ChatComposer.tsx`:
- Around line 133-142: The current logic clears the user's draft and attachments
regardless of whether send succeeded because setDraft("") and setDraftImages([])
run even when opState === "failed"; change this so you only clear the
draft/attachments when operation actually succeeded (opState !== "failed" /
deliveryState === "submitted" and ideally operation.state === "succeeded" if
available). In practice, after computing opState/deliveryState and calling
updatePending({ sessionId, clientMessageId, patch: { deliveryState } }),
conditionally call setDraft("") and setDraftImages([]) only when the operation
indicates success (use opState/operation.state or deliveryState to decide) to
avoid dropping user content on failure.
In `@app/components/ProjectSidebar.tsx`:
- Around line 445-450: The async getToken() call inside the useEffect should be
wrapped in a try/catch so rejections are handled; update the IIFE in useEffect
to try { const t = await getToken(); if (!cancelled) setToken(t); } catch (err)
{ if (!cancelled) { setToken(undefined); /* or leave previous state */
console.error('getToken failed', err); } } to avoid unhandled promise rejections
and inconsistent token state — reference the useEffect block, the cancelled
flag, getToken(), and setToken() when making the change.
In `@app/lib/desktop-state.ts`:
- Around line 113-116: The fallback worktree name generation only splits on "/"
so Windows paths like "C:\..." produce incorrect names; update the code that
builds the fallback WorktreeBucket (variable worktreePath -> name) to handle
both POSIX and Windows separators (e.g., use a split with a regex like /[\/\\]/
or use path.basename) so name = worktreePath.split(/[\/\\]/).pop() ??
worktreePath (or call path.basename(worktreePath) and add the appropriate
import) for correct basenames across platforms.
In `@app/scripts/check-release-env.js`:
- Around line 93-100: The current readReleaseEnv() always prefers
envProductionPath over envPath, causing non-production profiles (e.g.,
"testflight") to load the wrong .env; update the selection to be profile-aware:
determine the active release profile (e.g., from process.env.RELEASE_PROFILE or
the same config source used elsewhere), compute a profile-specific env path
(e.g., a profile suffix like envPath + `.${profile}` or a dedicated
profileEnvPath variable), then check for and prefer that profile-specific file
before falling back to envProductionPath or envPath; apply the same
profile-aware replacement where a similar selection occurs (the other block
referenced around the symbol(s) that mirror readReleaseEnv logic).
In `@CLAUDE.md`:
- Line 32: Update the CLAUDE.md documentation to reflect that app/stores/ uses
Jotai rather than Zustand: change the wording on the line describing app/stores/
to say "Jotai stores" and keep the per-store descriptions (projects, chat, ui)
but verify any API references (hooks or atoms) in the text match the Jotai
implementation (e.g., atom names or hook usage) so the docs align with the
actual code.
---
Duplicate comments:
In `@app/app.config.js`:
- Line 43: The hardcoded runtimeVersion ("1.0.0") in app.config.js can allow
incompatible OTA updates across native builds; update the runtimeVersion key to
use Expo's native-compatibility strategy by replacing runtimeVersion: "1.0.0"
with runtimeVersion: { policy: "fingerprint" } (or another native build-derived
value) so EAS Update will prevent incompatible OTA installs; locate the
runtimeVersion entry in app.config.js and make this replacement while keeping
the rest of the config intact.
In `@app/app/`(main)/agent/[sessionId]/chat.tsx:
- Around line 23-24: The code coerces a possibly missing or array-shaped route
param into the literal "undefined" by calling String(params.sessionId); update
the logic around useLocalSearchParams and sessionId to validate and normalize
the param: explicitly check params.sessionId for undefined/null and for an array
case, extract a single string (or bail/throw/redirect) instead of using
String(...), and only set sessionId when you have a real string value; reference
the useLocalSearchParams call and the sessionId variable to locate where to add
the guard and normalization.
- Line 156: The message key expression can still yield "undefined" or collide;
replace the current logical-OR chain used where the messages are rendered (the
key prop that references m.id, m.clientMessageId and idx) with a
nullish-coalescing fallback so you pick the first defined identifier (e.g., use
m.id ?? m.clientMessageId ?? `idx-${idx}`) and ensure the chosen value is
coerced to a stable string if necessary; update the key prop in the message
render where m and idx are in scope.
In `@app/app/`(main)/plans/[sessionId].tsx:
- Around line 12-13: Don't coerce a possibly-missing route param into the string
"undefined"; instead read params.sessionId directly from useLocalSearchParams
and guard its presence before using it. Replace the `const sessionId =
String(params.sessionId)` pattern with e.g. `const sessionId = params.sessionId`
(or `const sessionId = params.sessionId ?? undefined`) and add checks before
calling getPlan or putPlan (and any code in the file that currently assumes
sessionId exists) to either return/redirect/show an error when sessionId is
null/undefined/empty, so getPlan/putPlan only run with a valid non-empty
sessionId.
In `@app/app/`(main)/service/[serviceId].tsx:
- Around line 41-42: The route param parsing uses String(params.serviceId) which
converts undefined into the literal "undefined"; instead, read params via
useLocalSearchParams() then validate presence (e.g., if (!params?.serviceId)
return a proper not-found or error response) and only then set const serviceId =
params.serviceId; update both occurrences that set serviceId (the lines using
useLocalSearchParams and the later occurrence at the other location) to avoid
coercing undefined to the string "undefined" and handle the missing param
explicitly.
In `@app/app/`(main)/threads.tsx:
- Around line 39-41: The catch path currently leaves previous thread rows
rendered when a fetch fails; update the catch block in the fetch logic (the
try/catch around the network call that uses setThreads and setError in
threads.tsx) to clear stale data by calling setThreads([]) before setting the
error (and only do this when not cancelled), so replace the current catch
handling with a branch that, if not cancelled, calls setThreads([]) and then
setError(err instanceof Error ? err.message : String(err)).
In `@app/components/ChatComposer.tsx`:
- Around line 78-81: handleSend currently allows sending while attachments are
still uploading; add a guard to block sends when uploads are in progress. Update
the handleSend function (and the other send trigger referenced at the second
occurrence) to check the uploading state (e.g., if (uploading) return) before
proceeding, and ensure the UI send path that calls the second send handler
likewise respects uploading so messages cannot be submitted until all
draftImages/uploads finish.
In `@app/components/MobileTabBar.tsx`:
- Around line 28-31: The Pressable onPress handler in the MobileTabBar component
unconditionally calls router.push(route), which can push the current route
again; change the handler to first compare the target route with the current
route (use the router's current path/route from the routing API your app uses)
and only call router.push(route) when different, or use router.replace(route)
for idempotent navigation; update the onPress in the Pressable rendering inside
MobileTabBar to perform this check (or replace) to avoid duplicate stack
entries.
In `@app/global.css`:
- Around line 10-20: The current `@media` (prefers-reduced-motion: reduce) rule
that targets *, *::before, *::after and sets
animation-duration/animation-delay/transition-duration/transition-delay to
revert forces motion back on; remove that global revert and instead honor the
preference by disabling non-essential motion: inside `@media`
(prefers-reduced-motion: reduce) set
animation-duration/animation-delay/transition-duration/transition-delay to 0s
(and optionally animation-play-state: paused) for the global selectors, and if
some animations are essential, scope re-enablement to a specific class like
.motion-essential (use selectors like .motion-essential, .motion-essential * to
override the reduced-motion rules) so only explicitly opted-in elements run
animations.
In `@app/lib/heartbeat.ts`:
- Around line 83-95: The onerror handler currently only treats 401/403 as
terminal; change the logic in es.onerror to consider any HTTP 4xx (status >= 400
&& status < 500) a non-retriable terminal error: if such a status is present,
call es.close(), set stopped = true, and invoke onError with a descriptive
Error(`heartbeat unauthorized (status ${status})` or similar message that
includes the status); keep the existing early return for stopped and leave
transient behavior for non-4xx statuses unchanged. Use the existing symbols
es.onerror, status, stopped, es.close, and onError to locate and update the
code.
In `@app/scripts/build.sh`:
- Line 47: Make the eas build platform configurable instead of hardcoding ios:
add a PLATFORM variable with default "ios" (e.g. PLATFORM=${PLATFORM:-ios} or
read an optional script arg/env), and replace the hardcoded flag with --platform
"$PLATFORM" in the eas build invocation (the line containing eas build
--platform ios --profile "$EAS_PROFILE" --auto-submit "${EXTRA_ARGS[@]}").
Ensure callers (yarn build:testflight keeps default ios) and yarn
build:production can set PLATFORM=android (or pass an arg) so production builds
target Play when desired.
In `@app/scripts/version-manager.sh`:
- Around line 32-35: create_backups currently only copies VERSION_FILE
(lib/version.ts) so native iOS files (Info.plist and project.pbxproj) aren’t
backed up; update create_backups to also copy the native files that
bump-build/set mutate (e.g., INFO_PLIST_FILE and PBXPROJ_FILE or explicit
filenames Info.plist and project.pbxproj) to .backup counterparts, and update
the rollback logic to restore those same .backup files alongside
VERSION_FILE.backup so JS and native versions remain in sync.
In `@src/metadata-server.ts`:
- Around line 1133-1139: Change the error handling so filesystem I/O failures
return 5xx instead of falling through to the outer 400 handler: in the try/catch
that checks err?.code === "ENOENT" keep ENOENT mapped to send(res, 404, ...),
but for any other caught error call send(res, 500, { ok: false, error: "Plan
file I/O error" , detail: err.message }) (do not rethrow); apply the same
pattern to the code around the persist operation (the block that can throw
during persist) so non-ENOENT I/O errors are caught and responded with 5xx
instead of propagating to the outer handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17405d12-ca11-402b-b027-09a229bf044c
⛔ Files ignored due to path filters (9)
app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockdesktop-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.locksrc-tauri/Cargo.lockis excluded by!**/*.locksrc-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**src-tauri/icons/icon.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (105)
CLAUDE.mdREADME.mdapp/.gitignoreapp/.prettierrcapp/app.config.jsapp/app/(main)/_layout.tsxapp/app/(main)/agent/[sessionId]/chat.tsxapp/app/(main)/graveyard.tsxapp/app/(main)/index.tsxapp/app/(main)/plans/[sessionId].tsxapp/app/(main)/service/[serviceId].tsxapp/app/(main)/settings.tsxapp/app/(main)/threads.tsxapp/app/_layout.tsxapp/app/index.tsxapp/app/sign-in.tsxapp/app/sign-up.tsxapp/babel.config.jsapp/components/AppShell.tsxapp/components/AuthMenu.tsxapp/components/ChatComposer.tsxapp/components/MessageBlock.tsxapp/components/MobileTabBar.tsxapp/components/ProjectSidebar.tsxapp/components/TopBar.tsxapp/components/service-actions.tsxapp/components/status-dot.tsxapp/components/ui/badge.tsxapp/components/ui/button.tsxapp/components/ui/card.tsxapp/components/ui/input.tsxapp/components/ui/segmented-control.tsxapp/components/ui/separator.tsxapp/components/ui/text.tsxapp/eas.jsonapp/environment.d.tsapp/eslint.config.mjsapp/global.cssapp/lib/api.tsapp/lib/auth.tsxapp/lib/daemon-url.tsapp/lib/desktop-state.tsapp/lib/env.tsapp/lib/envContract.jsapp/lib/envRuntime.tsapp/lib/events.tsapp/lib/heartbeat.tsapp/lib/image-picker.d.tsapp/lib/image-picker.native.tsapp/lib/image-picker.web.tsapp/lib/jotai-storage.tsapp/lib/status-tone.tsapp/lib/theme-effect.tsapp/lib/utils.tsapp/lib/version.tsapp/metro.config.jsapp/nativewind-env.d.tsapp/package.jsonapp/scripts/build.shapp/scripts/check-release-env.jsapp/scripts/update.shapp/scripts/version-manager.shapp/stores/chat.tsapp/stores/desktopState.tsapp/stores/projects.tsapp/stores/ui.tsapp/tailwind.config.jsapp/tsconfig.jsonapp/vitest.config.tsdesktop-ui/index.htmldesktop-ui/package.jsondesktop-ui/src/App.sveltedesktop-ui/src/lib/ActionBar.sveltedesktop-ui/src/lib/ActivityPanel.sveltedesktop-ui/src/lib/GraveyardPanel.sveltedesktop-ui/src/lib/NativeChatPanel.sveltedesktop-ui/src/lib/PlansPanel.sveltedesktop-ui/src/lib/SessionPanel.sveltedesktop-ui/src/lib/Sidebar.sveltedesktop-ui/src/lib/StatusBar.sveltedesktop-ui/src/lib/TerminalPanel.sveltedesktop-ui/src/lib/ThreadsPanel.sveltedesktop-ui/src/lib/WorkspaceHeader.sveltedesktop-ui/src/lib/WorktreePanel.sveltedesktop-ui/src/lib/sessionSemantic.jsdesktop-ui/src/lib/terminal-instance.svelte.jsdesktop-ui/src/main.jsdesktop-ui/src/stores/state.svelte.jsdesktop-ui/src/styles.cssdesktop-ui/svelte.config.jsdesktop-ui/vite.config.jsdocs/desktop-shell-phase1-spec.mddocs/desktop-ui-contract.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/capabilities/default.jsonsrc-tauri/src/main.rssrc-tauri/tauri.conf.jsonsrc/instance-registry.test.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/paths.tssrc/worktree.test.tssrc/worktree.ts
💤 Files with no reviewable changes (28)
- package.json
- desktop-ui/src/lib/WorktreePanel.svelte
- desktop-ui/index.html
- desktop-ui/src/lib/NativeChatPanel.svelte
- desktop-ui/src/main.js
- desktop-ui/src/lib/WorkspaceHeader.svelte
- src-tauri/capabilities/default.json
- desktop-ui/src/lib/sessionSemantic.js
- desktop-ui/package.json
- desktop-ui/src/lib/ActivityPanel.svelte
- desktop-ui/src/stores/state.svelte.js
- desktop-ui/src/lib/terminal-instance.svelte.js
- desktop-ui/src/App.svelte
- desktop-ui/vite.config.js
- desktop-ui/src/lib/ActionBar.svelte
- desktop-ui/src/lib/Sidebar.svelte
- desktop-ui/src/lib/GraveyardPanel.svelte
- desktop-ui/src/lib/ThreadsPanel.svelte
- desktop-ui/svelte.config.js
- desktop-ui/src/lib/StatusBar.svelte
- desktop-ui/src/lib/TerminalPanel.svelte
- src-tauri/tauri.conf.json
- src-tauri/Cargo.toml
- desktop-ui/src/lib/SessionPanel.svelte
- src-tauri/build.rs
- src-tauri/src/main.rs
- desktop-ui/src/styles.css
- desktop-ui/src/lib/PlansPanel.svelte
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (20)
app/lib/envContract.js (1)
11-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
buildOnlyEnvKeysin the contract union.At Line 12,
allKnownEnvKeysskipsbuildOnlyEnvKeys, so future build-only vars won’t be surfaced in the exported complete key list.Proposed fix
const allKnownEnvKeys = Array.from( - new Set([...defaultedRuntimeEnvKeys, ...optionalRuntimeEnvKeys, ...requiredReleaseEnvKeys]), + new Set([ + ...defaultedRuntimeEnvKeys, + ...optionalRuntimeEnvKeys, + ...requiredReleaseEnvKeys, + ...buildOnlyEnvKeys, + ]), ).sort();🤖 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 `@app/lib/envContract.js` around lines 11 - 13, allKnownEnvKeys currently builds its set from defaultedRuntimeEnvKeys, optionalRuntimeEnvKeys and requiredReleaseEnvKeys but omits buildOnlyEnvKeys; update the Set construction used to compute allKnownEnvKeys to include ...buildOnlyEnvKeys alongside the other arrays (keeping the Array.from/new Set/.sort pattern) so build-only variables are included in the exported complete key list; refer to the allKnownEnvKeys constant and the arrays defaultedRuntimeEnvKeys, optionalRuntimeEnvKeys, requiredReleaseEnvKeys and buildOnlyEnvKeys when making the change.app/lib/status-tone.ts (1)
18-20:⚠️ Potential issue | 🟡 MinorTrim before tokenizing command text.
firstTokenOfreturns""for inputs like" node app.js". Trimming first avoids blank tokens on valid commands.🤖 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 `@app/lib/status-tone.ts` around lines 18 - 20, The function firstTokenOf currently splits the raw command string and can return an empty token for inputs with leading whitespace; update firstTokenOf to trim the input (e.g., call .trim() on the command) before tokenizing so leading/trailing spaces are removed, then split and return the first token (still return "" when the trimmed string is empty).app/app/(main)/threads.tsx (1)
39-41:⚠️ Potential issue | 🟠 MajorClear stale thread data when fetch fails.
On Line 40, only
erroris updated. Previously loaded threads remain rendered after a failed refresh, which can show incorrect state for the active project.🤖 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 `@app/app/`(main)/threads.tsx around lines 39 - 41, The catch block currently only calls setError and leaves previously loaded threads visible; update the catch to also clear stale thread state by calling the thread state setter (e.g., setThreads([]) or setThreads(null)) alongside setError when not cancelled, and ensure you still respect the cancelled guard used with setError; reference the existing setError, cancelled flag, and the threads state setter (setThreads) in threads.tsx to locate where to add this change.app/components/MobileTabBar.tsx (1)
28-31:⚠️ Potential issue | 🟠 MajorPrevent duplicate Stack entries from MobileTabBar by guarding active tabs or using router.replace.
app/components/MobileTabBar.tsxcomputes anactivetab indicator, but thePressablestill always callsrouter.push(route)on every tap. Since navigation is configured withexpo-routerStack(no bottom-tabs navigator), repeated taps will keep stacking identical routes and make back navigation noisy. Prefer guarding (if (active) return; router.push(route)) or usingrouter.replace(route)for tab presses.🤖 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 `@app/components/MobileTabBar.tsx` around lines 28 - 31, MobileTabBar currently calls router.push(route) for every Pressable tap which stacks duplicate routes; update the Pressable handler in the MobileTabBar component to first check the computed active boolean (or compare router.pathname to route) and return early if already active, or alternatively call router.replace(route) instead of router.push(route) to avoid creating duplicate Stack entries; modify the onPress for the Pressable (which references route and router) to implement one of these guards.app/lib/desktop-state.ts (1)
113-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle Windows separators in fallback worktree names.
Unknown buckets derived from
C:\...paths won’t produce the expected basename with the current split.Suggested fix
- name: worktreePath.split("/").pop() ?? worktreePath, + name: worktreePath.split(/[\\/]/).pop() ?? worktreePath,🤖 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 `@app/lib/desktop-state.ts` around lines 113 - 116, The fallback WorktreeBucket's name uses worktreePath.split("/").pop() which fails for Windows backslash paths; update the name derivation in the fallback object (WorktreeBucket, variable fallback) to split on both slashes and backslashes (e.g., use a regex split like /[\\/]/ or normalize backslashes to forward slashes) so Windows paths like "C:\foo\bar" produce the correct basename.app/app/sign-in.tsx (1)
28-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit fallback handling for non-
completeClerk statuses.Unhandled sign-in/verification statuses currently fall through with no user feedback.
Suggested fix
if (result.status === "complete") { await setActive({ session: result.createdSessionId }); } else if ( result.status === "needs_second_factor" || (result.status as string) === "needs_client_trust" ) { await signIn.prepareSecondFactor({ strategy: "email_code", }); setNeedsVerification(true); + } else { + setError(`Unsupported sign-in state: ${result.status}`); } @@ if (result.status === "complete") { await setActive({ session: result.createdSessionId }); + } else { + setError(`Verification not complete: ${result.status}`); }Also applies to: 56-58
🤖 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 `@app/app/sign-in.tsx` around lines 28 - 39, The sign-in flow currently handles only "complete" and two specific verification statuses, letting any other Clerk sign-in status silently fall through; update the sign-in handling around result.status (the branch that calls setActive, signIn.prepareSecondFactor, and setNeedsVerification) to include an explicit else fallback that sets an error/feedback state (e.g., setSignInError or reuse existing UI state), logs the unexpected status, and shows a user-friendly message or guidance; apply the same explicit fallback pattern to the later similar block that handles result.status at the other location so all non-`complete` statuses provide feedback instead of doing nothing.app/global.css (1)
10-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect reduced-motion user preference instead of globally reverting it.
This block forces motion for users who explicitly requested reduced motion, which is an accessibility/compliance risk.
Suggested fix
-/* Override prefers-reduced-motion — this app is too interactive to function without animations */ `@media` (prefers-reduced-motion: reduce) { *, *::before, *::after { - animation-duration: revert !important; - animation-delay: revert !important; - transition-duration: revert !important; - transition-delay: revert !important; + animation-duration: 0.01ms !important; + animation-iteration-count: 1 !important; + transition-duration: 0.01ms !important; + transition-delay: 0ms !important; + scroll-behavior: auto !important; } }🤖 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 `@app/global.css` around lines 10 - 20, The current `@media` (prefers-reduced-motion: reduce) block forcibly re-enables animations by using "revert !important", which violates the user's reduced-motion preference; update the rule targeting "`@media` (prefers-reduced-motion: reduce)" and change the declarations for the universal selectors (*, *::before, *::after) to disable motion instead of reverting it (e.g., set animation-duration: 0s !important; animation-delay: 0s !important; transition-duration: 0s !important; transition-delay: 0s !important; and where applicable set animation-play-state: paused or animation: none / transition: none) so the app respects user reduced-motion settings while keeping specificity via the same selectors.app/lib/heartbeat.ts (1)
83-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat non-429 HTTP 4xx as terminal instead of retrying forever.
Current logic only stops on 401/403, so other client errors can loop reconnect indefinitely.
Suggested fix
- if (status === 401 || status === 403) { + if ( + typeof status === "number" && + status >= 400 && + status < 500 && + status !== 429 + ) { es.close(); stopped = true; - onError?.(new Error(`heartbeat unauthorized (status ${status})`)); + onError?.(new Error(`heartbeat terminated (status ${status})`)); }🤖 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 `@app/lib/heartbeat.ts` around lines 83 - 95, Update the ServerEventSource error handler (es.onerror) in heartbeat.ts so that any HTTP 4xx status other than 429 is treated as terminal instead of only 401/403: read the status from the event like the existing code, and if status >= 400 && status < 500 && status !== 429 then call es.close(), set stopped = true, and invoke onError with a descriptive Error (similar to the current `heartbeat unauthorized (status ${status})` message); keep the current behavior for 429 and other non-4xx errors so the polyfill can continue reconnecting.app/components/ProjectSidebar.tsx (1)
445-450:⚠️ Potential issue | 🟠 MajorHandle
getToken()rejection in the effect.
await getToken()is still unhandled here; a rejection can surface as an unhandled promise and leave token state inconsistent.🤖 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 `@app/components/ProjectSidebar.tsx` around lines 445 - 450, The effect that calls getToken() should handle promise rejection to avoid unhandled rejections and inconsistent state: wrap the async call inside a try/catch in the useEffect (the immediately-invoked async function that uses getToken()), catch errors from getToken() and handle them (e.g., log via console or a logger and optionally setToken(null) or leave previous state), and ensure you only call setToken(t) when cancelled is false; keep the cancelled guard and reference the same variables/useEffect closure to prevent state updates after unmount.app/components/ChatComposer.tsx (2)
78-81:⚠️ Potential issue | 🟠 MajorBlock send while uploads are in progress.
Send is still allowed while
uploading === true, so messages can go out before selected attachments finish uploading.Also applies to: 189-191
🤖 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 `@app/components/ChatComposer.tsx` around lines 78 - 81, handleSend currently allows sending while uploads are in progress; add a guard that checks the uploading flag and prevents sending (e.g., if (uploading) return) alongside the existing checks for empty draft, draftImages, and sending in the handleSend function so messages cannot be sent until uploads complete; apply the same guard to the other send path referenced around the second send handler so both send entrypoints check uploading before proceeding.
133-142:⚠️ Potential issue | 🟠 MajorDon’t clear draft/attachments when operation state is failed.
setDraft("")andsetDraftImages([])still run even whenoperation.state === "failed", which drops user content on failed delivery.🤖 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 `@app/components/ChatComposer.tsx` around lines 133 - 142, The draft and attachments are being cleared unconditionally after updating pending state even when result.operation?.state === "failed"; change the logic in the block that computes opState/deliveryState (using result.operation?.state and deliveryState) so that setDraft("") and setDraftImages([]) are only called when the operation succeeded/submitted (i.e., opState !== "failed" or deliveryState === "submitted"); keep updatePending({ sessionId, clientMessageId, patch: { deliveryState } }) as-is so the failed state is recorded, but avoid clearing user input when opState === "failed".app/app.config.js (1)
43-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a compatibility-aware
runtimeVersionpolicy instead of a fixed literal.A fixed
runtimeVersion: "1.0.0"can let OTA updates target incompatible native binaries as native surface changes over time.Suggested fix
- runtimeVersion: "1.0.0", + runtimeVersion: { policy: "fingerprint" },Expo Updates documentation: for app.config.js, what runtimeVersion strategy is recommended to ensure OTA updates only apply to compatible native builds (e.g., fingerprint policy)?🤖 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 `@app/app.config.js` around lines 43 - 49, Replace the fixed string runtimeVersion ("1.0.0") with a compatibility-aware runtimeVersion policy so OTA updates are only delivered to compatible native builds; specifically, change the runtimeVersion entry in the app.config.js updates block to use a policy-based object (e.g., policy: "fingerprint") instead of the literal, leaving the rest of the updates config (url, enabled, checkAutomatically, fallbackToCacheTimeout) untouched and ensuring the symbol runtimeVersion is updated accordingly.app/app/(main)/plans/[sessionId].tsx (1)
12-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
sessionIdfrom route params before using it.
String(params.sessionId)can produce"undefined"and still pass your guards, which then sends invalid IDs togetPlan/putPlan.Suggested fix
- const params = useLocalSearchParams<{ sessionId: string }>(); - const sessionId = String(params.sessionId); + const params = useLocalSearchParams<{ sessionId?: string | string[] }>(); + const sessionIdParam = params.sessionId; + const sessionId = Array.isArray(sessionIdParam) ? sessionIdParam[0] : sessionIdParam; @@ - if (!serviceEndpoint || !sessionId) return; + if (!serviceEndpoint || !sessionId) return; @@ - if (!serviceEndpoint) return; + if (!serviceEndpoint || !sessionId) return;Also applies to: 26-27, 55-62
🤖 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 `@app/app/`(main)/plans/[sessionId].tsx around lines 12 - 13, Normalize and validate the route param before using it: don't rely on String(params.sessionId) which can yield "undefined"; instead check useLocalSearchParams()'s params.sessionId for null/undefined/empty and derive a safe sessionId (e.g. use params.sessionId ?? '' or bail/redirect when missing) before calling functions like getPlan or putPlan and wherever sessionId is used (references: useLocalSearchParams, sessionId, getPlan, putPlan).src/metadata-server.ts (1)
1133-1139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn 5xx for plan file I/O failures instead of falling through to 400.
Non-ENOENT read errors and write/rename failures are server faults, but they currently bubble into the outer
catchand get returned as400.Suggested fix
try { const content = readFileSync(planPath, "utf8"); send(res, 200, { ok: true, sessionId, content }); } catch (err: any) { if (err?.code === "ENOENT") { send(res, 404, { ok: false, error: "Plan not found" }); return; } - throw err; + send(res, 500, { ok: false, error: "Failed to read plan" }); + return; } @@ const planPath = join(getPlansDir(), `${sessionId}.md`); const dir = dirname(planPath); mkdirSync(dir, { recursive: true }); const tmpPath = join(dir, `.${sessionId}.${randomUUID()}.tmp`); - writeFileSync(tmpPath, body.content, "utf8"); - renameSync(tmpPath, planPath); + try { + writeFileSync(tmpPath, body.content, "utf8"); + renameSync(tmpPath, planPath); + } catch { + send(res, 500, { ok: false, error: "Failed to persist plan" }); + return; + } send(res, 200, { ok: true, sessionId });Also applies to: 1166-1167
🤖 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/metadata-server.ts` around lines 1133 - 1139, The catch block handling plan file I/O should treat non-ENOENT errors as server errors instead of rethrowing them; in the catch for the plan read/rename logic (the block that currently checks err?.code === "ENOENT" and calls send(res, 404, ...)), change the behavior so that if err.code !== "ENOENT" you call send(res, 500, { ok: false, error: "Plan file I/O error" , details: err.message }) (or equivalent) rather than throwing, and apply the same change to the similar catch at the other spot mentioned (the catch around the write/rename at the other location).app/app/(main)/agent/[sessionId]/chat.tsx (2)
156-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish coalescing for message keys to avoid
"undefined"key collisions.
||plus template coercion can generate duplicate"undefined"keys when both IDs are absent.Suggested fix
- key={m.id || `${m.clientMessageId}` || `idx-${idx}`} + key={m.id ?? m.clientMessageId ?? `idx-${idx}`}🤖 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 `@app/app/`(main)/agent/[sessionId]/chat.tsx at line 156, The current JSX key uses logical OR and template coercion which can produce duplicate "undefined" keys; update the message key logic in the chat message render (reference symbols: m.id, m.clientMessageId, idx) to use nullish coalescing instead of || and avoid forcing m.clientMessageId into a string when it's undefined—choose m.id if defined, else m.clientMessageId if defined, else a deterministic fallback based on idx.
23-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
sessionIdparam instead of coercing it to"undefined".The current coercion can route invalid IDs into atom keys, history fetches, and heartbeat subscriptions.
Suggested fix
- const params = useLocalSearchParams<{ sessionId: string }>(); - const sessionId = String(params.sessionId); + const params = useLocalSearchParams<{ sessionId?: string | string[] }>(); + const sessionIdParam = params.sessionId; + const sessionId = Array.isArray(sessionIdParam) ? sessionIdParam[0] : sessionIdParam;Also applies to: 58-60, 79-80
🤖 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 `@app/app/`(main)/agent/[sessionId]/chat.tsx around lines 23 - 24, The code currently coerces params.sessionId into a string via String(params.sessionId) which turns undefined into the literal "undefined" and pollutes atom keys/history/heartbeat; instead guard the param: check that useLocalSearchParams().sessionId is not null/undefined before assigning sessionId (e.g. if missing, return null/redirect/render an error) and only use the validated sessionId for atom keys, history fetches and heartbeat subscriptions (update the same guard logic where sessionId is used in the sessionId variable, in the history fetch logic, and in the heartbeat/subscribe code referenced by the other occurrences).app/scripts/check-release-env.js (2)
14-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on invalid release profile args.
Line 15 still maps any unknown arg to
testflight, which can validate the wrong target silently. Validatetargetagainst an allowlist and exit non-zero on invalid values.🤖 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 `@app/scripts/check-release-env.js` around lines 14 - 16, The current mapping silently treats any unknown target as "testflight"; update the logic around the target and profile variables to validate target against an allowlist (e.g., allowedTargets = ['production','testflight']) and if target is not in that list, print an error and exit with non-zero status (process.exit(1)); when valid, set profile to "production" only for "production" and "testflight" for the other allowed value so the script fails fast on invalid args.
93-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse profile-aware env file selection in
readReleaseEnv.Line 94 still prefers
.env.productionwhenever it exists, even whenprofileresolves totestflight. Select env file based onprofileto avoid cross-profile validation drift.Also applies to: 107-109
🤖 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 `@app/scripts/check-release-env.js` around lines 93 - 95, readReleaseEnv currently always prefers envProductionPath over envPath, causing .env.production to win even when profile is "testflight"; change the selection to be profile-aware by first constructing a profile-specific env path (e.g., derive envProfilePath from profile and envPath or env.<profile> equivalent) and use fs.existsSync on that candidate first, then fall back to envProductionPath and finally envPath; apply the same profile-aware selection logic to the other env-file selection block referenced (the second occurrence that currently prefers envProductionPath) so both places use the profile-specific candidate before falling back.app/scripts/version-manager.sh (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback should restore native iOS version files too.
Lines 32-35 and Lines 90-103 only back up/restore
lib/version.ts, butbump-build/setalso mutate iOS version files. IncludeInfo.plistandproject.pbxprojin backup, rollback, and cleanup to keep state consistent.Also applies to: 90-103
🤖 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 `@app/scripts/version-manager.sh` around lines 32 - 35, create_backups currently only copies lib/version.ts (via VERSION_FILE); modify create_backups to also copy iOS files Info.plist and project.pbxproj (e.g., cp "ios/YourApp/Info.plist" "ios/YourApp/Info.plist.backup" and cp "ios/YourApp.xcodeproj/project.pbxproj" "ios/YourApp.xcodeproj/project.pbxproj.backup"), and update the corresponding restore_backups and cleanup_backups functions (the restore/cleanup logic around lines ~90-103) to restore and remove these .backup files alongside the existing lib/version.ts.backup; ensure the same exact filenames used in bump-build/set are backed up/restored so iOS state remains consistent.app/lib/api.ts (1)
94-100: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace
Promise<unknown>responses with exported concrete response types.Line 94 and Lines 438-455 still leak
unknown, which weakens downstream type safety in the shared client contract. Define/export response interfaces and pass them intocallJson<T>for these endpoints.
Based on learnings: "Useapp/lib/api.tsfor typed HTTP client calls to daemon ... and per-project metadata-server routes targeting theserviceEndpoint."Also applies to: 438-455
🤖 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 `@app/lib/api.ts` around lines 94 - 100, The ensureProject function (and other endpoints still returning Promise<unknown>) is leaking an unknown response type; define and export a concrete interface (e.g., EnsureProjectResponse) that matches the daemon response shape, replace Promise<unknown> with Promise<EnsureProjectResponse>, and call callJson<EnsureProjectResponse>(...) inside ensureProject; similarly find the other callJson usages (the block around the other endpoint(s) that currently return Promise<unknown>), create/export appropriate response interfaces for each, and pass them as the generic parameter to callJson so the shared client contract is strongly typed (refer to ensureProject, callJson, getDaemonUrl, and ApiOpts to locate and update the code).
🧹 Nitpick comments (4)
app/eas.json (1)
17-17: ⚡ Quick winPin EAS build images instead of
image: "latest"In
app/eas.json,build.testflight.ios.imageand bothbuild.production.ios.image/build.production.android.imageuselatest; Expo Build recommends a specific pinned image name to keep builds reproducible over time and avoid unexpected CI/toolchain changes.🤖 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 `@app/eas.json` at line 17, Replace the three uses of the unpinned image "latest" in eas.json so builds are reproducible: update build.testflight.ios.image, build.production.ios.image, and build.production.android.image to Expo-recommended pinned image names (e.g., an explicit SDK/runner tag) instead of "latest"; locate those keys in app/eas.json and set each to the specific image string provided by Expo for your SDK/CI (ensure all three keys use appropriate pinned tags).app/lib/image-picker.web.ts (1)
45-49: ⚖️ Poor tradeoffConsider more robust cancellation detection.
The
focusevent listener with a 500mssetTimeoutis a common pattern for detecting when the user closes the file picker without selecting files. However, this approach is fragile:
- The 500ms delay is arbitrary and may not be sufficient in all scenarios (e.g., slow user interaction, browser variations).
- There's a potential race where the timeout fires while the user is still interacting with the picker.
While the current implementation likely works for most cases, consider tracking cancellation more explicitly or documenting the timeout assumption.
Alternative: Explicit cancel tracking
let resolved = false; input.onchange = async () => { resolved = true; // ... rest of handler }; window.addEventListener( "focus", () => { setTimeout(() => { if (!resolved && !input.files?.length) { resolved = true; resolve(null); } }, 500); }, { once: true } );🤖 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 `@app/lib/image-picker.web.ts` around lines 45 - 49, The focus+500ms timeout is fragile; change to explicit cancel tracking by introducing a local flag (e.g., resolved) and set it to true inside the input.onchange handler and any code path that calls resolve; on the window.addEventListener("focus", ...) handler use a short timeout only to check that resolved is still false and input.files is empty, then set resolved = true and call resolve(null). Update references to window.addEventListener, input.onchange, and resolve(null) so the picker cannot double-resolve and cancellation is deterministically tracked.src/instance-registry.test.ts (1)
23-32: ⚡ Quick winConsider using
execFileSyncwith an args array to avoid command injection risk.The current implementation uses string interpolation with
execSync(\git ${args}`), which static analysis correctly flags as a potential command injection vector. While the current call sites only pass hardcoded strings (making this safe in practice), usingexecFileSync` with a proper args array provides defense-in-depth and prevents future refactoring mistakes.🛡️ Safer alternative using execFileSync
function runGit(args: string, dir: string): void { // Strip inherited GIT_* env so commands honor `cwd` even when invoked under a git hook. const env = { ...process.env }; delete env.GIT_DIR; delete env.GIT_WORK_TREE; delete env.GIT_INDEX_FILE; delete env.GIT_OBJECT_DIRECTORY; delete env.GIT_COMMON_DIR; - execSync(`git ${args}`, { cwd: dir, stdio: "pipe", env }); + const argsArray = args.split(/\s+/); + execFileSync("git", argsArray, { cwd: dir, stdio: "pipe", env }); }Note: For more robust parsing (handling quoted args, etc.), consider using a shell-parsing library or restructuring to accept an args array directly:
function runGit(args: string[], dir: string): void { const env = { ...process.env }; delete env.GIT_DIR; delete env.GIT_WORK_TREE; delete env.GIT_INDEX_FILE; delete env.GIT_OBJECT_DIRECTORY; delete env.GIT_COMMON_DIR; execFileSync("git", args, { cwd: dir, stdio: "pipe", env }); } // Call sites: runGit(["init"], dir); runGit(["config", "user.email", "test@example.com"], dir);🤖 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/instance-registry.test.ts` around lines 23 - 32, The runGit function currently builds a shell command via execSync(`git ${args}`) which is a command-injection risk; change runGit to accept an args array (e.g. args: string[]) and call execFileSync("git", args, { cwd: dir, stdio: "pipe", env }) instead of execSync, keep the same env-cleaning logic (delete GIT_* entries) and update all call sites of runGit to pass an array of arguments (e.g. ["init"], ["config","user.email","..."]) so quoting/parsing is handled safely by execFileSync; ensure the function signature and imports are updated accordingly.app/components/ui/button.tsx (1)
59-63: ⚡ Quick winSet explicit accessibility metadata on the base button primitive.
Line 59 creates the core interactive control, but it doesn’t set a default
accessibilityRole/disabled accessibility state. Adding defaults here improves cross-platform screen-reader consistency.Proposed patch
const Button = React.forwardRef<React.ComponentRef<typeof Pressable>, ButtonProps>( ({ className, variant, size, label, children, ...props }, ref) => { return ( <Pressable ref={ref} + accessibilityRole={props.accessibilityRole ?? "button"} + accessibilityState={{ ...(props.accessibilityState ?? {}), disabled: props.disabled }} className={cn(buttonVariants({ variant, size }), props.disabled && "opacity-50", className)} {...props} > {children ?? <Text className={cn(buttonTextVariants({ variant, size }))}>{label}</Text>} </Pressable>🤖 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 `@app/components/ui/button.tsx` around lines 59 - 63, The Pressable base button primitive doesn't set default accessibility metadata; update the Pressable element in app/components/ui/button.tsx (the Pressable that uses ref, buttonVariants, props.disabled, className) to include explicit accessibilityRole="button" and accessibilityState={{ disabled: !!props.disabled }} (and ensure accessible is true if needed) so screen readers correctly perceive it as a button and reflect the disabled state across platforms.
🤖 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 `@app/app/`(main)/_layout.tsx:
- Around line 61-62: The early return when endpoint is falsy leaves per-project
desktop state cached and visible; before returning from the block that checks
"if (!selectedProjectPath || !endpoint) return;" clear the per-project
desktop/worktree state for the current selectedProjectPath (e.g., call the
module/function that resets cached desktop state or dispatch the action that
clears worktrees) and ensure any in-flight effects respect the "cancelled" flag;
update references to selectedProjectPath/endpoint/cancelled in the surrounding
effect to perform the clear and then return.
In `@app/app/sign-up.tsx`:
- Around line 115-120: The current Button's onPress handler uses router.back(),
which can leave users on the wrong screen if Sign Up was opened directly; update
the Button (the component with label "Already have an account? Sign In") to
navigate explicitly to the sign-in route (use router.push or router.replace to
"/sign-in" or your app's sign-in route) instead of calling router.back() so the
action always lands the user on the Sign In page.
In `@app/components/ChatComposer.tsx`:
- Around line 42-45: In handleAttach, wrap the await pickImages() call in a
try/catch so any thrown errors (permissions/cancel) are caught; on catch call
setError(err.message || String(err)) and return early (so the rest of
handleAttach doesn't run), and keep the existing check for picked length;
reference the handleAttach function and the pickImages and setError symbols when
making the change.
In `@app/package.json`:
- Around line 54-56: Update the react-native dependency in package.json: replace
the current "react-native": "0.81.5" entry with a supported React 19–compatible
release (e.g., "0.83.x"); ensure package.json's dependencies include the new
version string and run install to update lockfile, then run the project's
build/test commands to verify compatibility with React 19 and adjust any
RN-specific breaking changes if tests fail.
---
Duplicate comments:
In `@app/app.config.js`:
- Around line 43-49: Replace the fixed string runtimeVersion ("1.0.0") with a
compatibility-aware runtimeVersion policy so OTA updates are only delivered to
compatible native builds; specifically, change the runtimeVersion entry in the
app.config.js updates block to use a policy-based object (e.g., policy:
"fingerprint") instead of the literal, leaving the rest of the updates config
(url, enabled, checkAutomatically, fallbackToCacheTimeout) untouched and
ensuring the symbol runtimeVersion is updated accordingly.
In `@app/app/`(main)/agent/[sessionId]/chat.tsx:
- Line 156: The current JSX key uses logical OR and template coercion which can
produce duplicate "undefined" keys; update the message key logic in the chat
message render (reference symbols: m.id, m.clientMessageId, idx) to use nullish
coalescing instead of || and avoid forcing m.clientMessageId into a string when
it's undefined—choose m.id if defined, else m.clientMessageId if defined, else a
deterministic fallback based on idx.
- Around line 23-24: The code currently coerces params.sessionId into a string
via String(params.sessionId) which turns undefined into the literal "undefined"
and pollutes atom keys/history/heartbeat; instead guard the param: check that
useLocalSearchParams().sessionId is not null/undefined before assigning
sessionId (e.g. if missing, return null/redirect/render an error) and only use
the validated sessionId for atom keys, history fetches and heartbeat
subscriptions (update the same guard logic where sessionId is used in the
sessionId variable, in the history fetch logic, and in the heartbeat/subscribe
code referenced by the other occurrences).
In `@app/app/`(main)/plans/[sessionId].tsx:
- Around line 12-13: Normalize and validate the route param before using it:
don't rely on String(params.sessionId) which can yield "undefined"; instead
check useLocalSearchParams()'s params.sessionId for null/undefined/empty and
derive a safe sessionId (e.g. use params.sessionId ?? '' or bail/redirect when
missing) before calling functions like getPlan or putPlan and wherever sessionId
is used (references: useLocalSearchParams, sessionId, getPlan, putPlan).
In `@app/app/`(main)/threads.tsx:
- Around line 39-41: The catch block currently only calls setError and leaves
previously loaded threads visible; update the catch to also clear stale thread
state by calling the thread state setter (e.g., setThreads([]) or
setThreads(null)) alongside setError when not cancelled, and ensure you still
respect the cancelled guard used with setError; reference the existing setError,
cancelled flag, and the threads state setter (setThreads) in threads.tsx to
locate where to add this change.
In `@app/app/sign-in.tsx`:
- Around line 28-39: The sign-in flow currently handles only "complete" and two
specific verification statuses, letting any other Clerk sign-in status silently
fall through; update the sign-in handling around result.status (the branch that
calls setActive, signIn.prepareSecondFactor, and setNeedsVerification) to
include an explicit else fallback that sets an error/feedback state (e.g.,
setSignInError or reuse existing UI state), logs the unexpected status, and
shows a user-friendly message or guidance; apply the same explicit fallback
pattern to the later similar block that handles result.status at the other
location so all non-`complete` statuses provide feedback instead of doing
nothing.
In `@app/components/ChatComposer.tsx`:
- Around line 78-81: handleSend currently allows sending while uploads are in
progress; add a guard that checks the uploading flag and prevents sending (e.g.,
if (uploading) return) alongside the existing checks for empty draft,
draftImages, and sending in the handleSend function so messages cannot be sent
until uploads complete; apply the same guard to the other send path referenced
around the second send handler so both send entrypoints check uploading before
proceeding.
- Around line 133-142: The draft and attachments are being cleared
unconditionally after updating pending state even when result.operation?.state
=== "failed"; change the logic in the block that computes opState/deliveryState
(using result.operation?.state and deliveryState) so that setDraft("") and
setDraftImages([]) are only called when the operation succeeded/submitted (i.e.,
opState !== "failed" or deliveryState === "submitted"); keep updatePending({
sessionId, clientMessageId, patch: { deliveryState } }) as-is so the failed
state is recorded, but avoid clearing user input when opState === "failed".
In `@app/components/MobileTabBar.tsx`:
- Around line 28-31: MobileTabBar currently calls router.push(route) for every
Pressable tap which stacks duplicate routes; update the Pressable handler in the
MobileTabBar component to first check the computed active boolean (or compare
router.pathname to route) and return early if already active, or alternatively
call router.replace(route) instead of router.push(route) to avoid creating
duplicate Stack entries; modify the onPress for the Pressable (which references
route and router) to implement one of these guards.
In `@app/components/ProjectSidebar.tsx`:
- Around line 445-450: The effect that calls getToken() should handle promise
rejection to avoid unhandled rejections and inconsistent state: wrap the async
call inside a try/catch in the useEffect (the immediately-invoked async function
that uses getToken()), catch errors from getToken() and handle them (e.g., log
via console or a logger and optionally setToken(null) or leave previous state),
and ensure you only call setToken(t) when cancelled is false; keep the cancelled
guard and reference the same variables/useEffect closure to prevent state
updates after unmount.
In `@app/global.css`:
- Around line 10-20: The current `@media` (prefers-reduced-motion: reduce) block
forcibly re-enables animations by using "revert !important", which violates the
user's reduced-motion preference; update the rule targeting "`@media`
(prefers-reduced-motion: reduce)" and change the declarations for the universal
selectors (*, *::before, *::after) to disable motion instead of reverting it
(e.g., set animation-duration: 0s !important; animation-delay: 0s !important;
transition-duration: 0s !important; transition-delay: 0s !important; and where
applicable set animation-play-state: paused or animation: none / transition:
none) so the app respects user reduced-motion settings while keeping specificity
via the same selectors.
In `@app/lib/api.ts`:
- Around line 94-100: The ensureProject function (and other endpoints still
returning Promise<unknown>) is leaking an unknown response type; define and
export a concrete interface (e.g., EnsureProjectResponse) that matches the
daemon response shape, replace Promise<unknown> with
Promise<EnsureProjectResponse>, and call callJson<EnsureProjectResponse>(...)
inside ensureProject; similarly find the other callJson usages (the block around
the other endpoint(s) that currently return Promise<unknown>), create/export
appropriate response interfaces for each, and pass them as the generic parameter
to callJson so the shared client contract is strongly typed (refer to
ensureProject, callJson, getDaemonUrl, and ApiOpts to locate and update the
code).
In `@app/lib/desktop-state.ts`:
- Around line 113-116: The fallback WorktreeBucket's name uses
worktreePath.split("/").pop() which fails for Windows backslash paths; update
the name derivation in the fallback object (WorktreeBucket, variable fallback)
to split on both slashes and backslashes (e.g., use a regex split like /[\\/]/
or normalize backslashes to forward slashes) so Windows paths like "C:\foo\bar"
produce the correct basename.
In `@app/lib/envContract.js`:
- Around line 11-13: allKnownEnvKeys currently builds its set from
defaultedRuntimeEnvKeys, optionalRuntimeEnvKeys and requiredReleaseEnvKeys but
omits buildOnlyEnvKeys; update the Set construction used to compute
allKnownEnvKeys to include ...buildOnlyEnvKeys alongside the other arrays
(keeping the Array.from/new Set/.sort pattern) so build-only variables are
included in the exported complete key list; refer to the allKnownEnvKeys
constant and the arrays defaultedRuntimeEnvKeys, optionalRuntimeEnvKeys,
requiredReleaseEnvKeys and buildOnlyEnvKeys when making the change.
In `@app/lib/heartbeat.ts`:
- Around line 83-95: Update the ServerEventSource error handler (es.onerror) in
heartbeat.ts so that any HTTP 4xx status other than 429 is treated as terminal
instead of only 401/403: read the status from the event like the existing code,
and if status >= 400 && status < 500 && status !== 429 then call es.close(), set
stopped = true, and invoke onError with a descriptive Error (similar to the
current `heartbeat unauthorized (status ${status})` message); keep the current
behavior for 429 and other non-4xx errors so the polyfill can continue
reconnecting.
In `@app/lib/status-tone.ts`:
- Around line 18-20: The function firstTokenOf currently splits the raw command
string and can return an empty token for inputs with leading whitespace; update
firstTokenOf to trim the input (e.g., call .trim() on the command) before
tokenizing so leading/trailing spaces are removed, then split and return the
first token (still return "" when the trimmed string is empty).
In `@app/scripts/check-release-env.js`:
- Around line 14-16: The current mapping silently treats any unknown target as
"testflight"; update the logic around the target and profile variables to
validate target against an allowlist (e.g., allowedTargets =
['production','testflight']) and if target is not in that list, print an error
and exit with non-zero status (process.exit(1)); when valid, set profile to
"production" only for "production" and "testflight" for the other allowed value
so the script fails fast on invalid args.
- Around line 93-95: readReleaseEnv currently always prefers envProductionPath
over envPath, causing .env.production to win even when profile is "testflight";
change the selection to be profile-aware by first constructing a
profile-specific env path (e.g., derive envProfilePath from profile and envPath
or env.<profile> equivalent) and use fs.existsSync on that candidate first, then
fall back to envProductionPath and finally envPath; apply the same profile-aware
selection logic to the other env-file selection block referenced (the second
occurrence that currently prefers envProductionPath) so both places use the
profile-specific candidate before falling back.
In `@app/scripts/version-manager.sh`:
- Around line 32-35: create_backups currently only copies lib/version.ts (via
VERSION_FILE); modify create_backups to also copy iOS files Info.plist and
project.pbxproj (e.g., cp "ios/YourApp/Info.plist"
"ios/YourApp/Info.plist.backup" and cp "ios/YourApp.xcodeproj/project.pbxproj"
"ios/YourApp.xcodeproj/project.pbxproj.backup"), and update the corresponding
restore_backups and cleanup_backups functions (the restore/cleanup logic around
lines ~90-103) to restore and remove these .backup files alongside the existing
lib/version.ts.backup; ensure the same exact filenames used in bump-build/set
are backed up/restored so iOS state remains consistent.
In `@src/metadata-server.ts`:
- Around line 1133-1139: The catch block handling plan file I/O should treat
non-ENOENT errors as server errors instead of rethrowing them; in the catch for
the plan read/rename logic (the block that currently checks err?.code ===
"ENOENT" and calls send(res, 404, ...)), change the behavior so that if err.code
!== "ENOENT" you call send(res, 500, { ok: false, error: "Plan file I/O error" ,
details: err.message }) (or equivalent) rather than throwing, and apply the same
change to the similar catch at the other spot mentioned (the catch around the
write/rename at the other location).
---
Nitpick comments:
In `@app/components/ui/button.tsx`:
- Around line 59-63: The Pressable base button primitive doesn't set default
accessibility metadata; update the Pressable element in
app/components/ui/button.tsx (the Pressable that uses ref, buttonVariants,
props.disabled, className) to include explicit accessibilityRole="button" and
accessibilityState={{ disabled: !!props.disabled }} (and ensure accessible is
true if needed) so screen readers correctly perceive it as a button and reflect
the disabled state across platforms.
In `@app/eas.json`:
- Line 17: Replace the three uses of the unpinned image "latest" in eas.json so
builds are reproducible: update build.testflight.ios.image,
build.production.ios.image, and build.production.android.image to
Expo-recommended pinned image names (e.g., an explicit SDK/runner tag) instead
of "latest"; locate those keys in app/eas.json and set each to the specific
image string provided by Expo for your SDK/CI (ensure all three keys use
appropriate pinned tags).
In `@app/lib/image-picker.web.ts`:
- Around line 45-49: The focus+500ms timeout is fragile; change to explicit
cancel tracking by introducing a local flag (e.g., resolved) and set it to true
inside the input.onchange handler and any code path that calls resolve; on the
window.addEventListener("focus", ...) handler use a short timeout only to check
that resolved is still false and input.files is empty, then set resolved = true
and call resolve(null). Update references to window.addEventListener,
input.onchange, and resolve(null) so the picker cannot double-resolve and
cancellation is deterministically tracked.
In `@src/instance-registry.test.ts`:
- Around line 23-32: The runGit function currently builds a shell command via
execSync(`git ${args}`) which is a command-injection risk; change runGit to
accept an args array (e.g. args: string[]) and call execFileSync("git", args, {
cwd: dir, stdio: "pipe", env }) instead of execSync, keep the same env-cleaning
logic (delete GIT_* entries) and update all call sites of runGit to pass an
array of arguments (e.g. ["init"], ["config","user.email","..."]) so
quoting/parsing is handled safely by execFileSync; ensure the function signature
and imports are updated accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 209efe80-fa7e-4785-b0d5-d92b34aac59a
⛔ Files ignored due to path filters (9)
app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockdesktop-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.locksrc-tauri/Cargo.lockis excluded by!**/*.locksrc-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**src-tauri/icons/icon.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (105)
CLAUDE.mdREADME.mdapp/.gitignoreapp/.prettierrcapp/app.config.jsapp/app/(main)/_layout.tsxapp/app/(main)/agent/[sessionId]/chat.tsxapp/app/(main)/graveyard.tsxapp/app/(main)/index.tsxapp/app/(main)/plans/[sessionId].tsxapp/app/(main)/service/[serviceId].tsxapp/app/(main)/settings.tsxapp/app/(main)/threads.tsxapp/app/_layout.tsxapp/app/index.tsxapp/app/sign-in.tsxapp/app/sign-up.tsxapp/babel.config.jsapp/components/AppShell.tsxapp/components/AuthMenu.tsxapp/components/ChatComposer.tsxapp/components/MessageBlock.tsxapp/components/MobileTabBar.tsxapp/components/ProjectSidebar.tsxapp/components/TopBar.tsxapp/components/service-actions.tsxapp/components/status-dot.tsxapp/components/ui/badge.tsxapp/components/ui/button.tsxapp/components/ui/card.tsxapp/components/ui/input.tsxapp/components/ui/segmented-control.tsxapp/components/ui/separator.tsxapp/components/ui/text.tsxapp/eas.jsonapp/environment.d.tsapp/eslint.config.mjsapp/global.cssapp/lib/api.tsapp/lib/auth.tsxapp/lib/daemon-url.tsapp/lib/desktop-state.tsapp/lib/env.tsapp/lib/envContract.jsapp/lib/envRuntime.tsapp/lib/events.tsapp/lib/heartbeat.tsapp/lib/image-picker.d.tsapp/lib/image-picker.native.tsapp/lib/image-picker.web.tsapp/lib/jotai-storage.tsapp/lib/status-tone.tsapp/lib/theme-effect.tsapp/lib/utils.tsapp/lib/version.tsapp/metro.config.jsapp/nativewind-env.d.tsapp/package.jsonapp/scripts/build.shapp/scripts/check-release-env.jsapp/scripts/update.shapp/scripts/version-manager.shapp/stores/chat.tsapp/stores/desktopState.tsapp/stores/projects.tsapp/stores/ui.tsapp/tailwind.config.jsapp/tsconfig.jsonapp/vitest.config.tsdesktop-ui/index.htmldesktop-ui/package.jsondesktop-ui/src/App.sveltedesktop-ui/src/lib/ActionBar.sveltedesktop-ui/src/lib/ActivityPanel.sveltedesktop-ui/src/lib/GraveyardPanel.sveltedesktop-ui/src/lib/NativeChatPanel.sveltedesktop-ui/src/lib/PlansPanel.sveltedesktop-ui/src/lib/SessionPanel.sveltedesktop-ui/src/lib/Sidebar.sveltedesktop-ui/src/lib/StatusBar.sveltedesktop-ui/src/lib/TerminalPanel.sveltedesktop-ui/src/lib/ThreadsPanel.sveltedesktop-ui/src/lib/WorkspaceHeader.sveltedesktop-ui/src/lib/WorktreePanel.sveltedesktop-ui/src/lib/sessionSemantic.jsdesktop-ui/src/lib/terminal-instance.svelte.jsdesktop-ui/src/main.jsdesktop-ui/src/stores/state.svelte.jsdesktop-ui/src/styles.cssdesktop-ui/svelte.config.jsdesktop-ui/vite.config.jsdocs/desktop-shell-phase1-spec.mddocs/desktop-ui-contract.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/capabilities/default.jsonsrc-tauri/src/main.rssrc-tauri/tauri.conf.jsonsrc/instance-registry.test.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/paths.tssrc/worktree.test.tssrc/worktree.ts
💤 Files with no reviewable changes (28)
- desktop-ui/src/lib/Sidebar.svelte
- desktop-ui/src/lib/terminal-instance.svelte.js
- desktop-ui/index.html
- desktop-ui/src/lib/WorktreePanel.svelte
- desktop-ui/src/App.svelte
- desktop-ui/src/lib/TerminalPanel.svelte
- desktop-ui/src/lib/ThreadsPanel.svelte
- desktop-ui/package.json
- src-tauri/Cargo.toml
- src-tauri/build.rs
- desktop-ui/src/lib/ActionBar.svelte
- src-tauri/tauri.conf.json
- desktop-ui/src/stores/state.svelte.js
- desktop-ui/src/main.js
- desktop-ui/src/lib/NativeChatPanel.svelte
- src-tauri/capabilities/default.json
- desktop-ui/vite.config.js
- desktop-ui/src/lib/GraveyardPanel.svelte
- desktop-ui/src/styles.css
- desktop-ui/src/lib/sessionSemantic.js
- desktop-ui/src/lib/WorkspaceHeader.svelte
- package.json
- desktop-ui/src/lib/StatusBar.svelte
- desktop-ui/src/lib/SessionPanel.svelte
- desktop-ui/src/lib/PlansPanel.svelte
- src-tauri/src/main.rs
- desktop-ui/svelte.config.js
- desktop-ui/src/lib/ActivityPanel.svelte
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app/(main)/plans/[sessionId].tsx (1)
27-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid stuck loading when
sessionIdis absent.Line 28 returns early without clearing
loading, so the screen can remain on the spinner state for a missing/invalid route param.Suggested fix
useEffect(() => { - if (!serviceEndpoint || !sessionId) return; + if (!serviceEndpoint) return; + if (!sessionId) { + setLoading(false); + setError("Missing session id."); + return; + } @@ <Button size="sm" label={saving ? "Saving…" : "Save"} onPress={handleSave} - disabled={saving || !dirty || !serviceEndpoint} + disabled={saving || !dirty || !serviceEndpoint || !sessionId} />Also applies to: 84-89
🤖 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 `@app/app/`(main)/plans/[sessionId].tsx around lines 27 - 29, The early return inside the useEffect that checks if (!serviceEndpoint || !sessionId) leaves the component stuck in loading; update that effect (the useEffect that declares let cancelled = false) to call setLoading(false) before returning (and likewise in the other useEffect block that returns early when sessionId is missing), so both paths clear the loading state; keep the existing cancelled/cleanup logic intact and only add setLoading(false) immediately prior to each early return to ensure the spinner is dismissed when params are absent or invalid.
♻️ Duplicate comments (2)
app/app/sign-in.tsx (1)
58-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-
completeverify results explicitly.Line 58 handles only
"complete". If verification returns another status, the UI exits loading with no actionable feedback.Suggested fix
if (result.status === "complete") { await setActive({ session: result.createdSessionId }); + } else { + setError(`Verification not complete: ${result.status}`); }For `@clerk/clerk-expo` 2.19.x, what SignInResource.status values can signIn.attemptSecondFactor return, and should custom flows handle non-"complete" statuses explicitly?🤖 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 `@app/app/sign-in.tsx` around lines 58 - 60, The current code only handles result.status === "complete" after signIn.attemptSecondFactor and leaves the UI stuck for other statuses; update the flow around setActive and result.status to explicitly handle non-"complete" cases by adding an else branch that clears any loading state, surfaces user-visible feedback (toast/error state) and logs the unexpected status, and branches for other known statuses as needed (e.g., retry prompt or redirect); reference the result variable and the setActive call so you update the logic that currently checks result.status === "complete" to handle other status values explicitly.app/app/(main)/graveyard.tsx (1)
17-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefetch graveyard on project change, not only endpoint tuple.
Line 48 omits project identity from dependencies, so switching projects that resolve to the same host/port can show stale entries from the previous project.
Suggested fix
- }, [endpoint?.host, endpoint?.port, getToken]); + }, [project?.path, endpoint?.host, endpoint?.port, getToken]);🤖 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 `@app/app/`(main)/graveyard.tsx around lines 17 - 48, The effect that fetches graveyard data (useEffect) only depends on endpoint?.host, endpoint?.port and getToken, so switching projects that map to the same host/port leaves stale data; update the dependency array for that useEffect to include the project identifier from the endpoint object (e.g. endpoint.projectId or endpoint.project) so the effect reruns when the project changes, ensuring listGraveyard is recalled and setEntries/setError are refreshed accordingly.
🤖 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.
Outside diff comments:
In `@app/app/`(main)/plans/[sessionId].tsx:
- Around line 27-29: The early return inside the useEffect that checks if
(!serviceEndpoint || !sessionId) leaves the component stuck in loading; update
that effect (the useEffect that declares let cancelled = false) to call
setLoading(false) before returning (and likewise in the other useEffect block
that returns early when sessionId is missing), so both paths clear the loading
state; keep the existing cancelled/cleanup logic intact and only add
setLoading(false) immediately prior to each early return to ensure the spinner
is dismissed when params are absent or invalid.
---
Duplicate comments:
In `@app/app/`(main)/graveyard.tsx:
- Around line 17-48: The effect that fetches graveyard data (useEffect) only
depends on endpoint?.host, endpoint?.port and getToken, so switching projects
that map to the same host/port leaves stale data; update the dependency array
for that useEffect to include the project identifier from the endpoint object
(e.g. endpoint.projectId or endpoint.project) so the effect reruns when the
project changes, ensuring listGraveyard is recalled and setEntries/setError are
refreshed accordingly.
In `@app/app/sign-in.tsx`:
- Around line 58-60: The current code only handles result.status === "complete"
after signIn.attemptSecondFactor and leaves the UI stuck for other statuses;
update the flow around setActive and result.status to explicitly handle
non-"complete" cases by adding an else branch that clears any loading state,
surfaces user-visible feedback (toast/error state) and logs the unexpected
status, and branches for other known statuses as needed (e.g., retry prompt or
redirect); reference the result variable and the setActive call so you update
the logic that currently checks result.status === "complete" to handle other
status values explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0c1d340-81f4-4d6a-b63a-acdc00d0de54
📒 Files selected for processing (27)
CLAUDE.mdapp/app.config.jsapp/app/(main)/_layout.tsxapp/app/(main)/agent/[sessionId]/chat.tsxapp/app/(main)/graveyard.tsxapp/app/(main)/index.tsxapp/app/(main)/plans/[sessionId].tsxapp/app/(main)/service/[serviceId].tsxapp/app/(main)/threads.tsxapp/app/sign-in.tsxapp/app/sign-up.tsxapp/components/ChatComposer.tsxapp/components/MobileTabBar.tsxapp/components/ProjectSidebar.tsxapp/environment.d.tsapp/global.cssapp/lib/api.tsapp/lib/desktop-state.tsapp/lib/envContract.jsapp/lib/heartbeat.tsapp/lib/image-picker.web.tsapp/lib/route-params.tsapp/lib/status-tone.tsapp/scripts/build.shapp/scripts/check-release-env.jsapp/scripts/version-manager.shsrc/metadata-server.ts
💤 Files with no reviewable changes (1)
- app/global.css
✅ Files skipped from review due to trivial changes (3)
- app/lib/route-params.ts
- CLAUDE.md
- app/lib/status-tone.ts
Summary
Replaces the previous Tauri/Svelte desktop client with a single Expo Router codebase that targets web, iOS, and Android from one tree.
Scope
desktop-ui/(Svelte 5 + Vite) andsrc-tauri/(Rust/Tauri) — ~21,700 lines deletedapp/— Expo Router file-based routing, NativeWind, Jotai state, project sidebar + chat/plans/threads surfacesTopBar,AuthMenu(local-mode aware),AppShell,MobileTabBarsrc/paths.tsandsrc/worktree.ts: stripGIT_*env from production git invocations so the daemon resolves the correct repo when invoked from a context (e.g. git hook) that inheritedGIT_DIR/GIT_WORK_TREE. Same fix insrc/instance-registry.test.ts. This patches a cross-test contamination bug where runningyarn testunder pre-push leaked empty "init" commits onto the current branch.What this PR is NOT
Test plan
yarn verifypasses 566/566 standaloneyarn verifypasses 566/566 under pre-push hook env (the cross-contamination case)yarn web(local mode, no auth)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores