feat(desktop): add Expo build button to TopBar#1017
feat(desktop): add Expo build button to TopBar#1017danielz1z wants to merge 1 commit intosuperset-sh:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces Expo project detection and an ExpoButton UI component to the TopBar. It adds a new TRPC procedure to detect Expo in package.json, integrates it into the workspaces router, creates a React component managing terminal sessions for running Expo iOS, and updates TopBar to conditionally render the button. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TopBar as TopBar UI
participant ExpoButton as ExpoButton Component
participant TRPC as TRPC Router
participant FS as File System
participant Terminal as Terminal Session
User->>TopBar: Mount TopBar with workspace
TopBar->>ExpoButton: Render with workspaceId, worktreePath
ExpoButton->>TRPC: detectExpo(worktreePath)
TRPC->>FS: readFile(package.json)
FS-->>TRPC: JSON content
TRPC-->>ExpoButton: { hasExpo: true }
ExpoButton-->>User: Display button (Idle state)
User->>ExpoButton: Click button
ExpoButton->>TRPC: createOrAttach(paneId)
TRPC-->>ExpoButton: paneId, tabId
ExpoButton->>ExpoButton: Update state to "starting"
ExpoButton->>TRPC: write(command: "expo start")
TRPC->>Terminal: Send command
Terminal-->>User: Display Expo output
ExpoButton-->>User: Update button (Running state)
ExpoButton->>TRPC: stream(paneId) subscribe
Terminal->>TRPC: PTY exit event
TRPC-->>ExpoButton: Exit notification
ExpoButton->>ExpoButton: Reset state to "idle"
ExpoButton-->>User: Update button (Idle state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/detect-expo.ts`:
- Around line 10-23: The try/catch in the detect-expo logic currently swallows
all errors; update the catch to log the error with context before returning the
fallback. Specifically, in the procedure that calls
secureFs.readFile(input.worktreePath, "package.json") and parses it to compute
hasExpo, change the catch to accept the error (e) and call console.error with a
prefixed message like "[workspaces/detect-expo] failed to read/parse
package.json at <input.worktreePath>:" followed by the error, then continue to
return { hasExpo: false } as the fallback.
In `@apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx`:
- Around line 90-97: The Expo start flow sets setExpoState("starting") but only
flips to "running" in writeMutation.mutate's onSuccess, so on write failure the
UI is stuck; add an onError callback to the writeMutation.mutate call used when
starting Expo (the block that calls setActiveTab(workspaceId, session.tabId),
setExpoState("starting"), writeMutation.mutate({... data:
`\x03\x15${EXPO_COMMAND}\n` ...})) that resets the state (e.g.,
setExpoState("idle")) on error; likewise update the stop flow in handleStop() to
include an onError that resets expo state so the button isn't permanently
disabled after a failed write.
- Around line 74-83: Add a new useEffect inside the ExpoButton component that
resets the session when the workspace context changes: if workspaceId or
worktreePath changes, call setExpoState("idle"), setActivePaneId(null), and set
sessionRef.current = null to avoid reusing stale pane/tab references from the
previous workspace (this prevents handleStart from writing to a stale paneId).
Place the effect alongside the existing effects in ExpoButton.tsx and depend on
[workspaceId, worktreePath] so it runs whenever the workspace changes.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx (1)
1-7: Align component file layout with the component-per-folder convention.
This file would conform better to the repo’s component structure asTopBar/ExpoButton/ExpoButton.tsxwith a barrel export.As per coding guidelines: Use folder structure with one component per file:
ComponentName/ComponentName.tsxwith barrel export inindex.ts.
| try { | ||
| const content = await secureFs.readFile( | ||
| input.worktreePath, | ||
| "package.json", | ||
| ); | ||
| const packageJson = JSON.parse(content); | ||
| const hasExpo = !!( | ||
| packageJson.dependencies?.expo || | ||
| packageJson.devDependencies?.expo | ||
| ); | ||
| return { hasExpo }; | ||
| } catch { | ||
| return { hasExpo: false }; | ||
| } |
There was a problem hiding this comment.
Add contextual error logging instead of silent fallback.
Line 10-23 swallows file/JSON errors and returns false, which makes failures invisible and hard to diagnose. Please log with a prefixed message while keeping the fallback behavior.
🐛 Proposed fix
- } catch {
- return { hasExpo: false };
- }
+ } catch (error) {
+ console.error(
+ "[workspaces/detectExpo] Failed to read or parse package.json",
+ error,
+ );
+ return { hasExpo: false };
+ }As per coding guidelines: Never swallow errors silently; at minimum log them with context; Use prefixed console logging with pattern [domain/operation] message for all logging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const content = await secureFs.readFile( | |
| input.worktreePath, | |
| "package.json", | |
| ); | |
| const packageJson = JSON.parse(content); | |
| const hasExpo = !!( | |
| packageJson.dependencies?.expo || | |
| packageJson.devDependencies?.expo | |
| ); | |
| return { hasExpo }; | |
| } catch { | |
| return { hasExpo: false }; | |
| } | |
| try { | |
| const content = await secureFs.readFile( | |
| input.worktreePath, | |
| "package.json", | |
| ); | |
| const packageJson = JSON.parse(content); | |
| const hasExpo = !!( | |
| packageJson.dependencies?.expo || | |
| packageJson.devDependencies?.expo | |
| ); | |
| return { hasExpo }; | |
| } catch (error) { | |
| console.error( | |
| "[workspaces/detectExpo] Failed to read or parse package.json", | |
| error, | |
| ); | |
| return { hasExpo: false }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/detect-expo.ts`
around lines 10 - 23, The try/catch in the detect-expo logic currently swallows
all errors; update the catch to log the error with context before returning the
fallback. Specifically, in the procedure that calls
secureFs.readFile(input.worktreePath, "package.json") and parses it to compute
hasExpo, change the catch to accept the error (e) and call console.error with a
prefixed message like "[workspaces/detect-expo] failed to read/parse
package.json at <input.worktreePath>:" followed by the error, then continue to
return { hasExpo: false } as the fallback.
| // Reset state if the tracked tab is closed by the user | ||
| useEffect(() => { | ||
| if (!sessionRef.current) return; | ||
| const tabStillExists = tabs.some((t) => t.id === sessionRef.current?.tabId); | ||
| if (!tabStillExists) { | ||
| setExpoState("idle"); | ||
| sessionRef.current = null; | ||
| setActivePaneId(null); | ||
| } | ||
| }, [tabs]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check its content around lines 74-83
cat -n apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx | head -100Repository: superset-sh/superset
Length of output: 4235
🏁 Script executed:
# Continue reading the file to see the rest of the handleStart function and any other relevant code
cat -n apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx | tail -n +100 | head -50Repository: superset-sh/superset
Length of output: 1887
🏁 Script executed:
# Search for other places where workspace changes might be handled or state is cleared
rg "workspaceId|worktreePath" apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx -B2 -A2Repository: superset-sh/superset
Length of output: 1080
🏁 Script executed:
# Check if there's any zustand store managing workspace state or similar cleanup patterns
rg "useEffect.*workspaceId|useEffect.*workspace" apps/desktop/src/renderer -t tsx -A 3 | head -50Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the tabs store to understand how tabs relate to workspaces
cat apps/desktop/src/renderer/stores/tabs/store.tsRepository: superset-sh/superset
Length of output: 31456
🏁 Script executed:
# Check how handleStart uses workspaceId and if the cross-workspace issue is real
rg "setActiveTab" apps/desktop/src/renderer --type ts -B2 -A2 | head -60Repository: superset-sh/superset
Length of output: 5773
🏁 Script executed:
# Verify the current state of the component to see if there are any other workspace-related resets
grep -n "workspaceId\|worktreePath" apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsxRepository: superset-sh/superset
Length of output: 424
🏁 Script executed:
# Check if there's any workspace change detection or cleanup elsewhere in the component
cat -n apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx | tail -60Repository: superset-sh/superset
Length of output: 2325
🏁 Script executed:
# Verify the full list of dependencies in handleStart callback to see if workspaceId changes already trigger recreation
rg "const handleStart" apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx -A 30Repository: superset-sh/superset
Length of output: 1057
Reset the session when the workspace changes to prevent cross‑workspace state reuse.
When a user switches workspaces, sessionRef and activePaneId retain references to the previous workspace's tab and pane. If the user clicks "Start" in the new workspace, handleStart will attempt to write to a stale paneId that may not exist or belong to a different workspace, causing terminal operations to fail unnecessarily.
Add a useEffect that clears the session state whenever workspaceId or worktreePath changes:
Proposed fix
useEffect(() => {
if (!sessionRef.current) return;
const tabStillExists = tabs.some((t) => t.id === sessionRef.current?.tabId);
if (!tabStillExists) {
setExpoState("idle");
sessionRef.current = null;
setActivePaneId(null);
}
}, [tabs]);
+ useEffect(() => {
+ sessionRef.current = null;
+ setActivePaneId(null);
+ setExpoState("idle");
+ }, [workspaceId, worktreePath]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Reset state if the tracked tab is closed by the user | |
| useEffect(() => { | |
| if (!sessionRef.current) return; | |
| const tabStillExists = tabs.some((t) => t.id === sessionRef.current?.tabId); | |
| if (!tabStillExists) { | |
| setExpoState("idle"); | |
| sessionRef.current = null; | |
| setActivePaneId(null); | |
| } | |
| }, [tabs]); | |
| // Reset state if the tracked tab is closed by the user | |
| useEffect(() => { | |
| if (!sessionRef.current) return; | |
| const tabStillExists = tabs.some((t) => t.id === sessionRef.current?.tabId); | |
| if (!tabStillExists) { | |
| setExpoState("idle"); | |
| sessionRef.current = null; | |
| setActivePaneId(null); | |
| } | |
| }, [tabs]); | |
| useEffect(() => { | |
| sessionRef.current = null; | |
| setActivePaneId(null); | |
| setExpoState("idle"); | |
| }, [workspaceId, worktreePath]); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx`
around lines 74 - 83, Add a new useEffect inside the ExpoButton component that
resets the session when the workspace context changes: if workspaceId or
worktreePath changes, call setExpoState("idle"), setActivePaneId(null), and set
sessionRef.current = null to avoid reusing stale pane/tab references from the
previous workspace (this prevents handleStart from writing to a stale paneId).
Place the effect alongside the existing effects in ExpoButton.tsx and depend on
[workspaceId, worktreePath] so it runs whenever the workspace changes.
| if (session) { | ||
| setActiveTab(workspaceId, session.tabId); | ||
| setExpoState("starting"); | ||
| // \x03 = Ctrl+C (kill any running process), \x15 = Ctrl+U (clear partial input) | ||
| writeMutation.mutate( | ||
| { paneId: session.paneId, data: `\x03\x15${EXPO_COMMAND}\n` }, | ||
| { onSuccess: () => setExpoState("running") }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specified lines
if [ -f "apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx" ]; then
echo "=== File content at lines 85-105 ==="
sed -n '85,105p' "apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx"
else
echo "File not found, searching for ExpoButton.tsx"
find . -name "ExpoButton.tsx" -type f
fiRepository: superset-sh/superset
Length of output: 782
🏁 Script executed:
# Check the full context of the component to understand state management
wc -l apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsxRepository: superset-sh/superset
Length of output: 139
🏁 Script executed:
# Search for writeMutation definition to understand how it's used
rg -A 10 -B 5 "writeMutation" apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsxRepository: superset-sh/superset
Length of output: 2422
Reset state on write failure to avoid a stuck "Starting" state.
The current code (line 90-97) sets expoState to "starting" but only updates it to "running" on success. While writeMutation has a global onError handler that shows a toast, this does NOT reset the state. On write failure, expoState remains "starting" and the button stays disabled, requiring a reload to recover. Add an onError callback to reset the state:
Proposed fix
writeMutation.mutate(
{ paneId: session.paneId, data: `\x03\x15${EXPO_COMMAND}\n` },
- { onSuccess: () => setExpoState("running") },
+ {
+ onSuccess: () => setExpoState("running"),
+ onError: () => setExpoState("idle"),
+ },
);Note: handleStop() has the same issue and should also include error handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (session) { | |
| setActiveTab(workspaceId, session.tabId); | |
| setExpoState("starting"); | |
| // \x03 = Ctrl+C (kill any running process), \x15 = Ctrl+U (clear partial input) | |
| writeMutation.mutate( | |
| { paneId: session.paneId, data: `\x03\x15${EXPO_COMMAND}\n` }, | |
| { onSuccess: () => setExpoState("running") }, | |
| ); | |
| if (session) { | |
| setActiveTab(workspaceId, session.tabId); | |
| setExpoState("starting"); | |
| // \x03 = Ctrl+C (kill any running process), \x15 = Ctrl+U (clear partial input) | |
| writeMutation.mutate( | |
| { paneId: session.paneId, data: `\x03\x15${EXPO_COMMAND}\n` }, | |
| { | |
| onSuccess: () => setExpoState("running"), | |
| onError: () => setExpoState("idle"), | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx`
around lines 90 - 97, The Expo start flow sets setExpoState("starting") but only
flips to "running" in writeMutation.mutate's onSuccess, so on write failure the
UI is stuck; add an onError callback to the writeMutation.mutate call used when
starting Expo (the block that calls setActiveTab(workspaceId, session.tabId),
setExpoState("starting"), writeMutation.mutate({... data:
`\x03\x15${EXPO_COMMAND}\n` ...})) that resets the state (e.g.,
setExpoState("idle")) on error; likewise update the stop flow in handleStop() to
include an onError that resets expo state so the button isn't permanently
disabled after a failed write.
1 participant
Summary
npx expo run:ios --devicein a dedicated terminal tabpackage.jsonand hides when not applicableDetails
Detection: New
detectExpotRPC procedure readspackage.jsonfor anexpodependency.Session management: First click creates a terminal tab. Subsequent clicks reuse it, sending Ctrl+C before re-running the command.
Known limitation: The button can't detect when the Expo child process exits while the shell stays alive (e.g. user Ctrl+C's in the terminal directly). This requires OSC 133 shell integration which is out of scope — documented in
EXPO_BUTTON.md.Files changed
apps/desktop/docs/EXPO_BUTTON.md— Feature documentationapps/desktop/src/lib/trpc/routers/workspaces/procedures/detect-expo.ts— New tRPC procedureapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts— Wire up procedureapps/desktop/src/renderer/screens/main/components/TopBar/ExpoButton.tsx— Button componentapps/desktop/src/renderer/screens/main/components/TopBar/index.tsx— Mount in TopBarSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.