fix: preserve yolo/permission mode across remote switch; validate persisted values; fix Ionicons, QR scanning, MCP bridge on macOS Tauri; add health/relay/spawn-mode tests#144
Open
ahundt wants to merge 9 commits intohappier-dev:devfrom
Conversation
…QR scanner) Previous behavior: - App crashed on launch: "invalid provider settings… codexMcpResumeInstallSpec is missing from settingsShape" due to orphaned references after mcp_resume deprecation (plugin.ts:53, installablesRegistry.ts:74) - Ionicons rendered as empty boxes on web/Tauri because _layout.tsx:578-588 bypasses expo-font on web via injectWebFontFaces(), but fontMap only included FontAwesome.font, not Ionicons.font - QR scanner was disabled on desktop web because QrCodeScannerView.tsx:119 required isWebMobileLikeQrScannerHost() (viewport ≤500px + mobile UA), blocking any desktop with a camera - codexBackendMode default changed 'mcp' → 'acp' but tests still expected 'mcp' - Kokoro TTS worker tests crashed on ENOENT for never-committed vendor artifact - TaskView t() mock didn't handle parameterized translation functions What changed: - plugin.ts: remove orphaned codexMcpResumeInstallSpec UI field - installablesRegistry.ts: remove type union hack, point codex-mcp-resume installable to codexAcpInstallSpec (mcp_resume routes through ACP now) - _layout.tsx: add Ionicons to import + ...Ionicons.font to fontMap so injectWebFontFaces() generates the @font-face CSS rule on web - QrCodeScannerView.tsx: simplify canUseCamera to check isWebQrScannerSupported() (navigator.mediaDevices.getUserMedia) instead of requiring phone-sized viewport — any device with camera API works - useConnectAccount.ts: same simplification for canUseScanner - restore/index.tsx: same simplification for showScannerFirst - synthesizeKokoroWav.spec.ts: graceful skip when kokoroTtsWorker.js absent - TaskView.test.tsx: fix t() mock to handle subject param - 6 test files updated to match new behavior + defaults Why: The Tauri macOS desktop app is a first-class target. These regressions accumulated from (1) codex mcp_resume deprecation leaving orphaned references, (2) custom web font loading path not including Ionicons, (3) QR scanner being unnecessarily restricted to phone-sized viewports when the camera API check alone is sufficient. Files affected: - apps/ui/sources/agents/providers/codex/settings/plugin.ts - apps/ui/sources/capabilities/installablesRegistry.ts (+test) - apps/ui/sources/app/_layout.tsx - apps/ui/sources/components/qr/QrCodeScannerView.tsx (+test) - apps/ui/sources/hooks/auth/useConnectAccount.ts (+test) - apps/ui/sources/app/(app)/restore/index.tsx (+test) - apps/ui/sources/voice/kokoro/runtime/synthesizeKokoroWav.spec.ts - apps/ui/sources/components/tools/renderers/workflow/TaskView.test.tsx - apps/ui/sources/sync/domains/settings/settings.spec.ts - apps/ui/sources/sync/domains/settings/settings.providerPlugins.test.ts - apps/ui/sources/__tests__/routes/_layout.init.spec.tsx - apps/ui/sources/__tests__/app/machine/machineDetails.capabilitiesRequestStability.test.ts Testable: yarn workspace @happier-dev/app test → 4662 passed, 0 failed
session.ts:54-59: Add spawnPermissionMode field (PermissionMode, default 'default'). Stores the permission mode from initial session creation (CLI --dangerously-skip-permissions or mobile spawn picker). Set once, never overwritten by per-message or metadata updates. Used as a floor when constructing local spawn args so the launch intent survives remote control transfers. session.test.ts: Add test verifying spawnPermissionMode is independent of lastPermissionMode — setLastPermissionMode does not clobber it.
Captures the original permission mode intent (from CLI args or daemon spawn RPC) into spawnPermissionMode at session creation. This value represents the launch constraint and is set before the metadata/fallback seeding of lastPermissionMode, ensuring both fields are populated from the same source.
…local spawn args When constructing CLI arguments for a local Claude Code resume after a remote-to-local switch, use spawnPermissionMode as a floor: if lastPermissionMode has been clobbered to 'default' by remote per-message modes but the session was originally started with a non-default mode (e.g. yolo via --dangerously-skip-permissions), preserve the launch intent so the local Claude process respects it. Root cause: handleModeChange() in permissionHandler.ts calls session.setLastPermissionMode() on every remote message. If the mobile app sends messages with permissionMode: 'default' (the picker default), lastPermissionMode gets overwritten from 'yolo' to 'default'. On local resume, the CLI spawns without --permission-mode bypassPermissions. The fix reads session.spawnPermissionMode (set once at session creation, never overwritten) and uses it when lastPermissionMode would produce a less permissive spawn than originally intended.
…-across-remote-switch
loadSessionPermissionModes() was calling JSON.parse() without validating each entry against isPermissionMode(). Unknown/stale/corrupted values from old app versions would silently pass through as PermissionMode. Now mirrors the validation pattern used by loadSessionPermissionModeUpdatedAts(): iterate parsed object entries and skip any value that fails isPermissionMode(value). Also adds ensureTauriMcpDevCapability() to prepareTauriSidecar.mjs for generating the mcp-dev.json capability file needed during Tauri builds, with corresponding tests.
… relay tests
Previous behavior:
- enableMonitoring.integration.spec.ts had one test (service name only); harness was created inside each it() so a second test would fail with "Database client is already initialized" due to the singleton Proxy in storage/prisma.ts
- No test verified the full { status, timestamp, service } response shape on /health
- No test verified the 503 path when db.$queryRaw throws
- No test verified serverFeaturesClient returns { status: 'error' } when the relay is completely unreachable (ECONNREFUSED / Network request failed on all fetches including the /health probe)
What changed:
- enableMonitoring.integration.spec.ts: move harness creation to beforeAll/afterAll so the DB singleton is initialized once; add test asserting status:'ok', timestamp (valid ISO string), service:'happier-server'
- enableMonitoring.spec.ts (new): unit test using vi.mock('@/storage/db') to make $queryRaw reject; asserts statusCode 503, body.status:'error', body.service:'happier-server', body.error:'Database connectivity failed'. vi.spyOn on the proxy target was not viable since $queryRaw is not an own property of the Proxy({}) target.
- serverFeaturesClient.test.ts: add test that overrides globalThis.fetch to throw TypeError('Network request failed') for all URLs, then calls getServerFeaturesSnapshot({ force:true, timeoutMs:100 }); asserts result.status === 'error' — covers the Tauri first-launch "Choose Relay / can't reach relay" screen
Why: The /health endpoint is the reachability probe polled by serverReachabilitySupervisorPool; untested edge cases (missing shape fields, DB failure, fully offline relay) are the exact failure modes that produce the "Server not supported" and "can't reach relay" errors seen on first Tauri launch.
Files affected:
- apps/server/sources/app/api/utils/enableMonitoring.integration.spec.ts: shared harness + shape test
- apps/server/sources/app/api/utils/enableMonitoring.spec.ts: new unit test for 503 DB failure
- apps/ui/sources/sync/api/capabilities/serverFeaturesClient.test.ts: offline relay error test
Add prisma/migrations/ to the server gitignore so auto-generated migration directories (e.g. from pglite test harness) are not tracked without needing to enumerate specific timestamps.
…olo-mode-across-remote-switch # Conflicts: # apps/ui/sources/__tests__/app/machine/machineDetails.capabilitiesRequestStability.test.ts # apps/ui/sources/__tests__/routes/(app)/restore/index.webDesktop.spec.tsx # apps/ui/sources/agents/providers/codex/settings/plugin.ts # apps/ui/sources/capabilities/installablesRegistry.test.ts # apps/ui/sources/capabilities/installablesRegistry.ts # apps/ui/sources/components/qr/QrCodeScannerView.test.tsx # apps/ui/sources/components/tools/renderers/workflow/SubAgentView.test.tsx # apps/ui/sources/hooks/auth/useConnectAccount.scannerLifecycle.test.tsx # apps/ui/sources/sync/domains/settings/settings.providerPlugins.test.ts # apps/ui/sources/sync/domains/settings/settings.spec.ts
WalkthroughThe PR introduces session spawn permission mode tracking in the CLI to preserve initial permissions, simplifies web QR scanning heuristics, adds Ionicons font loading support, enhances test harness setup for server monitoring, creates MCP dev capability provisioning for Tauri, and improves validation and error handling across various modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
apps/server/sources/app/api/utils/enableMonitoring.integration.spec.ts (2)
24-26: Add brief justifications for theas anycasts.Same pattern as the unit test file—add a one-line comment explaining the Fastify type bridge.
Suggested fix
it('reports service as happier-server in /health responses', async () => { - const app = Fastify(); + // Fastify generic satisfies enableMonitoring's app param at runtime + const app = Fastify(); try { - enableMonitoring(app as any); + enableMonitoring(app as any); // see comment aboveit('returns full response body shape { status, timestamp, service } when database is healthy', async () => { - const app = Fastify(); + // Fastify generic satisfies enableMonitoring's app param at runtime + const app = Fastify(); try { - enableMonitoring(app as any); + enableMonitoring(app as any); // see comment aboveAlso applies to: 39-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/sources/app/api/utils/enableMonitoring.integration.spec.ts` around lines 24 - 26, Add a one-line comment explaining each Fastify type cast to `any` used to bridge the test harness to the production-type signature: next to the `Fastify()` instantiation and the `enableMonitoring(app as any)` calls (and the similar cast at lines 39-41), add a short comment like “// cast to any to satisfy enableMonitoring’s Fastify type in test harness” so reviewers know these are intentional test-only type bridges.
49-50: Consider usingtoBeTypeOfandnot.toBeNaNfor clearer intent.The current assertion works but Vitest provides more expressive matchers.
Optional refactor
- expect(typeof body.timestamp).toBe('string'); - expect(Number.isNaN(new Date(body.timestamp!).getTime())).toBe(false); + expect(body.timestamp).toBeTypeOf('string'); + expect(new Date(body.timestamp!).getTime()).not.toBeNaN();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/sources/app/api/utils/enableMonitoring.integration.spec.ts` around lines 49 - 50, Replace the two assertions on body.timestamp with Vitest expressive matchers: use expect(body.timestamp).toBeTypeOf('string') instead of typeof check, and use expect(new Date(body.timestamp!).getTime()).not.toBeNaN() instead of Number.isNaN(...).toBe(false); update the assertions around body.timestamp accordingly (the symbols to change are the existing expects for body.timestamp and Number.isNaN(new Date(body.timestamp!).getTime())).apps/server/sources/app/api/utils/enableMonitoring.spec.ts (1)
15-15: Add a brief justification for theas anycast.Per coding guidelines, broad
as anycasts require a one-line justification. This cast bridges Fastify's generic type to the expected app parameter type.Suggested fix
- const app = Fastify({ logger: false }) as any; + // Fastify generic satisfies enableMonitoring's app param at runtime + const app = Fastify({ logger: false }) as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/sources/app/api/utils/enableMonitoring.spec.ts` at line 15, The test uses a broad cast "as any" for the Fastify instance (const app = Fastify({ logger: false }) as any) without justification; add a one-line inline comment explaining the reason for the cast (e.g., "cast to any to satisfy test helper's app type because Fastify's generic types differ from the expected App type in tests") placed either at the end of the statement or immediately above it so the purpose is documented per coding guidelines.apps/ui/sources/components/qr/QrCodeScannerView.tsx (1)
109-118: Remove stale viewport dependencies fromcanUseCamera.
width/heightare no longer part of camera eligibility, soLine 118now triggers unnecessary recomputation on every resize.♻️ Suggested cleanup
-import { ActivityIndicator, AppState, Linking, Platform, Pressable, useWindowDimensions, View } from 'react-native'; +import { ActivityIndicator, AppState, Linking, Platform, Pressable, View } from 'react-native'; ... - const { width, height } = useWindowDimensions(); ... const canUseCamera = React.useMemo(() => { if (isRunningOnMac()) return false; if (Platform.OS !== 'web') return true; return isWebQrScannerSupported(); - }, [height, width]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/sources/components/qr/QrCodeScannerView.tsx` around lines 109 - 118, The useMemo for canUseCamera is recomputing on every resize because it depends on width and height from useWindowDimensions; remove the stale viewport dependencies by eliminating width/height from the dependency array (or stop calling useWindowDimensions if unused) and change the useMemo dependencies to only those that affect camera eligibility (e.g., Platform.OS, isRunningOnMac, isWebQrScannerSupported). Update references to width/height so there are no unused variables (they may be removed from the file) and ensure canUseCamera remains defined as the memoized value based solely on platform/mac/support checks.apps/ui/sources/hooks/auth/useConnectAccount.ts (1)
21-22: Drop stale dimension dependencies fromconnectAccount.After switching to capability-based web checks,
width/heightno longer affect scanner eligibility, so this callback is being recreated unnecessarily on viewport changes.♻️ Suggested cleanup
-import { Platform, useWindowDimensions } from 'react-native'; +import { Platform } from 'react-native'; ... - const { width, height } = useWindowDimensions(); const [isLoading, setIsLoading] = React.useState(false); ... - }, [height, width]); + }, []);Also applies to: 55-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/sources/hooks/auth/useConnectAccount.ts` around lines 21 - 22, The connectAccount callback in useConnectAccount.ts is being needlessly recreated on viewport changes because it depends on width and height from useWindowDimensions(); remove width and height from connectAccount's closure and dependency array so the callback no longer includes those stale dimension values. Locate the connectAccount function and its useEffect/useCallback dependency array and drop width and height, and apply the same removal to the other dimension-dependent callback referenced around lines 55-62 (remove width/height from its closure and dependency array as well). Ensure no remaining code inside these callbacks reads width or height after the change.apps/cli/src/backends/claude/session.ts (1)
105-110: Enforce the “set once” invariant in code, not only in comments.
spawnPermissionModeis documented as immutable after creation, but it is currently public/mutable. Consider constructor initialization +readonly(or a guarded one-time setter) to prevent accidental reassignment later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/backends/claude/session.ts` around lines 105 - 110, spawnPermissionMode is documented as immutable but is currently public and mutable; make it truly set-once by initializing it via the session constructor and marking it readonly (or implement a guarded one-time setter) so it cannot be reassigned later. Locate the class/constructor that defines spawnPermissionMode (type PermissionMode) in session.ts, remove the public mutable field initialization, accept the initial PermissionMode as a constructor parameter and assign it once to a readonly spawnPermissionMode property, or alternatively add a private backing field plus a setOnceSpawnPermissionMode(value: PermissionMode) method that throws if called more than once to enforce the invariant.apps/ui/scripts/prepareTauriSidecar.mjs (1)
140-147: Pass explicit context to the injected MCP capability helper.Calling
ensureTauriMcpDevCapabilityImpl()with no args makes the DI contract fragile for injected implementations that destructure inputs.Small DI-contract hardening
- await ensureTauriMcpDevCapabilityImpl(); + await ensureTauriMcpDevCapabilityImpl({ + srcTauriDir: join(uiDir, 'src-tauri'), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/scripts/prepareTauriSidecar.mjs` around lines 140 - 147, The injected MCP capability helper ensureTauriMcpDevCapabilityImpl is being invoked with no arguments making DI fragile for implementations that destructure inputs; update the call to pass an explicit context object (e.g., include uiDir, bootstrapDir, env and any other relevant locals used by ensureTauriMcpDevCapability) so ensureTauriMcpDevCapabilityImpl({ uiDir, bootstrapDir, env }) is called instead of ensureTauriMcpDevCapabilityImpl() to harden the DI contract.apps/ui/scripts/prepareTauriSidecar.test.mjs (1)
90-110: Strengthen capability assertions to enforce exact permissions.These tests currently allow unnoticed extra permissions. For a security-sensitive capability file, prefer exact array checks in both tests.
Suggested test tightening
assert.equal(capability.identifier, 'mcp-dev'); assert.deepEqual(capability.windows, ['main']); - assert.ok(capability.permissions.includes('mcp-bridge:default')); + assert.deepEqual(capability.permissions, ['mcp-bridge:default']); @@ const capability = JSON.parse(await readFile(join(srcTauriDir, 'capabilities', 'mcp-dev.json'), 'utf8')); assert.equal(capability.identifier, 'mcp-dev'); + assert.deepEqual(capability.windows, ['main']); + assert.deepEqual(capability.permissions, ['mcp-bridge:default']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/scripts/prepareTauriSidecar.test.mjs` around lines 90 - 110, The tests for ensureTauriMcpDevCapability allow extra permissions because they use .includes; update both tests ("ensureTauriMcpDevCapability writes mcp-dev.json with correct content" and "ensureTauriMcpDevCapability is idempotent when run multiple times") to assert the permissions array exactly matches the expected array rather than contains it — replace the assert.ok(capability.permissions.includes('mcp-bridge:default')) with an exact array comparison (e.g., assert.deepEqual(capability.permissions, ['mcp-bridge:default'])) so the capability file cannot silently include extra permissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/.gitignore`:
- Line 16: The .gitignore entry "prisma/migrations/" is preventing tracked
Prisma migration files from being present in containers/CI; remove that ignore
(or add an explicit negation like "!prisma/migrations/") so repository migration
files are available to runtime scripts; ensure this change makes migrations
visible to the scripts referenced (run-server.sh, migrate.light.deploy.ts,
migrate.sqlite.deploy.ts) and CI (tests.yml) so prisma migrate deploy can run
successfully.
In `@apps/ui/sources/sync/api/capabilities/serverFeaturesClient.test.ts`:
- Around line 481-486: The test currently calls
resetServerFeaturesClientForTests() but doesn't reset the reachability
supervisor state, which can leak across tests; before calling
getServerFeaturesSnapshot({ force: true, timeoutMs: 100 }) add a call to the
test helper that clears reachability supervisors (e.g.,
resetReachabilitySupervisorsForTests or the equivalent reset function exposed by
serverFeaturesClient or its reachability module) so the reachability gate is in
a known offline state for this test; ensure the reset call occurs after
importing serverFeaturesClient and before the getServerFeaturesSnapshot
invocation.
In `@apps/ui/sources/voice/kokoro/runtime/synthesizeKokoroWav.spec.ts`:
- Around line 17-19: The tests currently return early when the vendored worker
file is absent (via fs.existsSync(workerPath)) which yields silent-pass behavior
and duplicate guard logic; change this to a deterministic guard: replace the
bare returns with an explicit test.skip or fail (e.g., throw or assert.fail)
with a clear message when workerPath is missing, and centralize the
existence/read logic into a single helper (e.g., loadKokoroWorker or
getWorkerSource) used by both tests so the file-read (workerPath, fs.existsSync,
fs.readFileSync, workerSource) is only implemented once and the test outcome is
explicit when the artifact is not present.
---
Nitpick comments:
In `@apps/cli/src/backends/claude/session.ts`:
- Around line 105-110: spawnPermissionMode is documented as immutable but is
currently public and mutable; make it truly set-once by initializing it via the
session constructor and marking it readonly (or implement a guarded one-time
setter) so it cannot be reassigned later. Locate the class/constructor that
defines spawnPermissionMode (type PermissionMode) in session.ts, remove the
public mutable field initialization, accept the initial PermissionMode as a
constructor parameter and assign it once to a readonly spawnPermissionMode
property, or alternatively add a private backing field plus a
setOnceSpawnPermissionMode(value: PermissionMode) method that throws if called
more than once to enforce the invariant.
In `@apps/server/sources/app/api/utils/enableMonitoring.integration.spec.ts`:
- Around line 24-26: Add a one-line comment explaining each Fastify type cast to
`any` used to bridge the test harness to the production-type signature: next to
the `Fastify()` instantiation and the `enableMonitoring(app as any)` calls (and
the similar cast at lines 39-41), add a short comment like “// cast to any to
satisfy enableMonitoring’s Fastify type in test harness” so reviewers know these
are intentional test-only type bridges.
- Around line 49-50: Replace the two assertions on body.timestamp with Vitest
expressive matchers: use expect(body.timestamp).toBeTypeOf('string') instead of
typeof check, and use expect(new Date(body.timestamp!).getTime()).not.toBeNaN()
instead of Number.isNaN(...).toBe(false); update the assertions around
body.timestamp accordingly (the symbols to change are the existing expects for
body.timestamp and Number.isNaN(new Date(body.timestamp!).getTime())).
In `@apps/server/sources/app/api/utils/enableMonitoring.spec.ts`:
- Line 15: The test uses a broad cast "as any" for the Fastify instance (const
app = Fastify({ logger: false }) as any) without justification; add a one-line
inline comment explaining the reason for the cast (e.g., "cast to any to satisfy
test helper's app type because Fastify's generic types differ from the expected
App type in tests") placed either at the end of the statement or immediately
above it so the purpose is documented per coding guidelines.
In `@apps/ui/scripts/prepareTauriSidecar.mjs`:
- Around line 140-147: The injected MCP capability helper
ensureTauriMcpDevCapabilityImpl is being invoked with no arguments making DI
fragile for implementations that destructure inputs; update the call to pass an
explicit context object (e.g., include uiDir, bootstrapDir, env and any other
relevant locals used by ensureTauriMcpDevCapability) so
ensureTauriMcpDevCapabilityImpl({ uiDir, bootstrapDir, env }) is called instead
of ensureTauriMcpDevCapabilityImpl() to harden the DI contract.
In `@apps/ui/scripts/prepareTauriSidecar.test.mjs`:
- Around line 90-110: The tests for ensureTauriMcpDevCapability allow extra
permissions because they use .includes; update both tests
("ensureTauriMcpDevCapability writes mcp-dev.json with correct content" and
"ensureTauriMcpDevCapability is idempotent when run multiple times") to assert
the permissions array exactly matches the expected array rather than contains it
— replace the assert.ok(capability.permissions.includes('mcp-bridge:default'))
with an exact array comparison (e.g., assert.deepEqual(capability.permissions,
['mcp-bridge:default'])) so the capability file cannot silently include extra
permissions.
In `@apps/ui/sources/components/qr/QrCodeScannerView.tsx`:
- Around line 109-118: The useMemo for canUseCamera is recomputing on every
resize because it depends on width and height from useWindowDimensions; remove
the stale viewport dependencies by eliminating width/height from the dependency
array (or stop calling useWindowDimensions if unused) and change the useMemo
dependencies to only those that affect camera eligibility (e.g., Platform.OS,
isRunningOnMac, isWebQrScannerSupported). Update references to width/height so
there are no unused variables (they may be removed from the file) and ensure
canUseCamera remains defined as the memoized value based solely on
platform/mac/support checks.
In `@apps/ui/sources/hooks/auth/useConnectAccount.ts`:
- Around line 21-22: The connectAccount callback in useConnectAccount.ts is
being needlessly recreated on viewport changes because it depends on width and
height from useWindowDimensions(); remove width and height from connectAccount's
closure and dependency array so the callback no longer includes those stale
dimension values. Locate the connectAccount function and its
useEffect/useCallback dependency array and drop width and height, and apply the
same removal to the other dimension-dependent callback referenced around lines
55-62 (remove width/height from its closure and dependency array as well).
Ensure no remaining code inside these callbacks reads width or height after the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ef2cf05-c565-4af6-bcdb-4955c6ff2cc4
📒 Files selected for processing (18)
apps/cli/src/backends/claude/claudeLocalLauncher.tsapps/cli/src/backends/claude/loop.tsapps/cli/src/backends/claude/session.test.tsapps/cli/src/backends/claude/session.tsapps/server/.gitignoreapps/server/sources/app/api/utils/enableMonitoring.integration.spec.tsapps/server/sources/app/api/utils/enableMonitoring.spec.tsapps/ui/scripts/prepareTauriSidecar.mjsapps/ui/scripts/prepareTauriSidecar.test.mjsapps/ui/sources/__tests__/routes/(app)/restore/index.webDesktop.spec.tsxapps/ui/sources/__tests__/routes/_layout.init.spec.tsxapps/ui/sources/app/(app)/restore/index.tsxapps/ui/sources/app/_layout.tsxapps/ui/sources/components/qr/QrCodeScannerView.tsxapps/ui/sources/hooks/auth/useConnectAccount.tsapps/ui/sources/sync/api/capabilities/serverFeaturesClient.test.tsapps/ui/sources/sync/domains/state/persistence.tsapps/ui/sources/voice/kokoro/runtime/synthesizeKokoroWav.spec.ts
|
|
||
| generated/ | ||
|
|
||
| prisma/migrations/ |
There was a problem hiding this comment.
Do not ignore tracked Prisma migrations (breaks deploy paths).
Line 16 ignores prisma/migrations/, but server startup/CI scripts run prisma migrate deploy and require repository migration files to exist. This can break container boot and test setup.
Suggested fix
-prisma/migrations/Cross-file evidence: apps/server/scripts/run-server.sh (runtime deploy), apps/server/scripts/migrate.light.deploy.ts, apps/server/scripts/migrate.sqlite.deploy.ts, and .github/workflows/tests.yml all invoke deploy migrations.
📝 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.
Suggested change
| prisma/migrations/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/.gitignore` at line 16, The .gitignore entry "prisma/migrations/"
is preventing tracked Prisma migration files from being present in
containers/CI; remove that ignore (or add an explicit negation like
"!prisma/migrations/") so repository migration files are available to runtime
scripts; ensure this change makes migrations visible to the scripts referenced
(run-server.sh, migrate.light.deploy.ts, migrate.sqlite.deploy.ts) and CI
(tests.yml) so prisma migrate deploy can run successfully.
Comment on lines
+481
to
+486
| const { getServerFeaturesSnapshot, resetServerFeaturesClientForTests } = await import('./serverFeaturesClient'); | ||
| resetServerFeaturesClientForTests(); | ||
|
|
||
| // timeoutMs=100 allows the reachability gate to time out quickly in the test. | ||
| const result = await getServerFeaturesSnapshot({ force: true, timeoutMs: 100 }); | ||
| expect(result.status).toBe('error'); |
There was a problem hiding this comment.
Reset reachability supervisors here to keep this test isolated.
resetServerFeaturesClientForTests() clears only the features cache; reachability supervisor state can leak across tests and make this case order-dependent. Please reset supervisors before asserting offline behavior.
Suggested fix
const { getServerFeaturesSnapshot, resetServerFeaturesClientForTests } = await import('./serverFeaturesClient');
+ const { resetServerReachabilitySupervisors } = await import('@/sync/runtime/connectivity/serverReachabilitySupervisorPool');
resetServerFeaturesClientForTests();
+ await resetServerReachabilitySupervisors();
// timeoutMs=100 allows the reachability gate to time out quickly in the test.
const result = await getServerFeaturesSnapshot({ force: true, timeoutMs: 100 });Based on learnings: Tests must be deterministic and isolated with no shared global state and no ordering reliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/sources/sync/api/capabilities/serverFeaturesClient.test.ts` around
lines 481 - 486, The test currently calls resetServerFeaturesClientForTests()
but doesn't reset the reachability supervisor state, which can leak across
tests; before calling getServerFeaturesSnapshot({ force: true, timeoutMs: 100 })
add a call to the test helper that clears reachability supervisors (e.g.,
resetReachabilitySupervisorsForTests or the equivalent reset function exposed by
serverFeaturesClient or its reachability module) so the reachability gate is in
a known offline state for this test; ensure the reset call occurs after
importing serverFeaturesClient and before the getServerFeaturesSnapshot
invocation.
Comment on lines
+17
to
+19
| const workerPath = new URL('../../../../public/vendor/kokoro/kokoroTtsWorker.js', import.meta.url); | ||
| if (!fs.existsSync(workerPath)) return; // vendored artifact not yet built | ||
| const workerSource = fs.readFileSync(workerPath, 'utf8'); |
There was a problem hiding this comment.
Avoid silent-pass test behavior when the vendored worker file is missing.
Line 18 and Line 25 return early, so both tests can pass with zero assertions depending on local artifact presence. This hides regressions. Also, the same load/guard logic is duplicated.
Proposed fix
describe('synthesizeKokoroWav (web)', () => {
+ const readVendoredWorkerSource = (): string => {
+ const workerPath = new URL('../../../../public/vendor/kokoro/kokoroTtsWorker.js', import.meta.url);
+ if (!fs.existsSync(workerPath)) {
+ throw new Error('Missing vendored kokoroTtsWorker.js test artifact');
+ }
+ return fs.readFileSync(workerPath, 'utf8');
+ };
+
beforeEach(() => {
vi.resetModules();
(globalThis as any).Worker = undefined;
(globalThis as any).window = { location: { href: 'http://example.test/' } };
});
@@
it('closes the TextSplitterStream so streaming requests complete', () => {
- const workerPath = new URL('../../../../public/vendor/kokoro/kokoroTtsWorker.js', import.meta.url);
- if (!fs.existsSync(workerPath)) return; // vendored artifact not yet built
- const workerSource = fs.readFileSync(workerPath, 'utf8');
+ const workerSource = readVendoredWorkerSource();
expect(workerSource).toContain('splitter.close');
});
@@
it('supports kokoro-js stream chunk audio shapes', () => {
- const workerPath = new URL('../../../../public/vendor/kokoro/kokoroTtsWorker.js', import.meta.url);
- if (!fs.existsSync(workerPath)) return; // vendored artifact not yet built
- const workerSource = fs.readFileSync(workerPath, 'utf8');
+ const workerSource = readVendoredWorkerSource();
expect(workerSource).toContain('audioObj?.audio');
expect(workerSource).toContain('audioObj?.sampling_rate');
});Based on learnings: “Tests must be deterministic and isolated with no shared global state and no ordering reliance.”
Also applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/sources/voice/kokoro/runtime/synthesizeKokoroWav.spec.ts` around
lines 17 - 19, The tests currently return early when the vendored worker file is
absent (via fs.existsSync(workerPath)) which yields silent-pass behavior and
duplicate guard logic; change this to a deterministic guard: replace the bare
returns with an explicit test.skip or fail (e.g., throw or assert.fail) with a
clear message when workerPath is missing, and centralize the existence/read
logic into a single helper (e.g., loadKokoroWorker or getWorkerSource) used by
both tests so the file-read (workerPath, fs.existsSync, fs.readFileSync,
workerSource) is only implemented once and the test outcome is explicit when the
artifact is not present.
Contributor
Greptile SummaryThis PR delivers several independent but related improvements: the core fix preserves non-default permission modes (e.g. yolo) across remote↔local session switches by introducing a dedicated On the Tauri desktop side: Ionicons fonts are included in the web font injection map, Key observations:
Confidence Score: 5/5Safe to merge; all remaining findings are P2 cleanup suggestions with no impact on correctness or runtime behavior. All core logic changes (spawnPermissionMode floor, persistence validation, MCP capability generation, QR scanner gating, font fix) are correct and well-tested. The only open findings are stale [height, width] dependency arrays in two components — purely cosmetic, causing unnecessary re-renders on resize but no incorrect behavior. apps/ui/sources/hooks/auth/useConnectAccount.ts and apps/ui/sources/components/qr/QrCodeScannerView.tsx have leftover useWindowDimensions subscriptions and stale memo/callback dep arrays that can be cleaned up.
|
| Filename | Overview |
|---|---|
| apps/cli/src/backends/claude/claudeLocalLauncher.ts | Adds spawnPermissionMode floor logic so a yolo/non-default launch intent is preserved when lastPermissionMode has drifted back to 'default' via remote messages. Logic is correct and well-commented. |
| apps/cli/src/backends/claude/session.ts | Adds public spawnPermissionMode: PermissionMode = 'default' field, deliberately separate from lastPermissionMode so remote metadata updates cannot overwrite the initial launch intent. |
| apps/ui/sources/sync/domains/state/persistence.ts | Validates the parsed MMKV blob in loadSessionPermissionModes — rejects non-objects, arrays, and per-entry values that don't pass isPermissionMode(). Mirrors the already-existing validation pattern used in loadSessionPermissionModeUpdatedAts. |
| apps/ui/sources/hooks/auth/useConnectAccount.ts | Replaces isWebMobileLikeQrScannerHost with isWebQrScannerSupported, but useWindowDimensions() subscription and [height, width] in the connectAccount dep array are now dead code, causing unnecessary re-renders on resize. |
| apps/ui/sources/components/qr/QrCodeScannerView.tsx | Removes dimension-based scanner guard, but [height, width] dep array in canUseCamera useMemo is now stale — triggers needless recomputation on window resize without affecting the result. |
| apps/ui/scripts/prepareTauriSidecar.mjs | Adds ensureTauriMcpDevCapability which generates capabilities/mcp-dev.json idempotently during Tauri sidecar preparation. Safe: default parameter references uiDir which is evaluated at call time, after full module initialization. |
| apps/server/sources/app/api/utils/enableMonitoring.spec.ts | New unit test using a mocked db.$queryRaw to verify the 503 error path when the database is unreachable. Uses vi.mock + dynamic await import() correctly for this pattern. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI (loop.ts)
participant Session as Session
participant Launcher as claudeLocalLauncher
participant Remote as Remote Messages
CLI->>Session: new Session(...)
CLI->>Session: spawnPermissionMode = opts.permissionMode ?? 'default'
CLI->>Session: lastPermissionMode = opts.permissionMode ?? 'default'
Remote->>Session: setLastPermissionMode('default', ts)
Note over Session: lastPermissionMode drifts to 'default'
Note over Session: spawnPermissionMode unchanged ('yolo')
Launcher->>Session: read lastPermissionMode → 'default'
Launcher->>Session: read spawnPermissionMode → 'yolo'
Launcher->>Launcher: effectivePermissionMode = spawnPermissionMode ('yolo')
Launcher->>Launcher: upsertClaudePermissionModeArgs(claudeArgs, { permissionMode: 'yolo' })
Launcher->>CLI: spawn Claude with --permission-mode yolo
Comments Outside Diff (1)
-
apps/ui/sources/hooks/auth/useConnectAccount.ts, line 21-62 (link)Dead
useWindowDimensionssubscription and stale dependency arrayAfter removing the
isWebMobileLikeQrScannerHostcall, neitherwidthnorheightis consumed anywhere in this hook. TheuseWindowDimensions()call on line 21 now subscribes the component to every window-resize event unnecessarily, and[height, width]in theconnectAccountdep array causes the callback to be recreated on each resize even though the result is always the same.Removing the now-dead destructuring cleans this up — remove line 21 and simplify the dependency array on the callback to
[].
Reviews (1): Last reviewed commit: "Merge branch 'fix/tauri-macos-launch-reg..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
114
to
118
| const canUseCamera = React.useMemo(() => { | ||
| if (isRunningOnMac()) return false; | ||
| if (Platform.OS !== 'web') return true; | ||
| if (!isWebQrScannerSupported()) return false; | ||
| return isWebMobileLikeQrScannerHost({ width, height }); | ||
| return isWebQrScannerSupported(); | ||
| }, [height, width]); |
Contributor
There was a problem hiding this comment.
useMemo dependency array after removing dimension-based check
isRunningOnMac(), Platform.OS, and isWebQrScannerSupported() are all constant for the lifetime of the component, so canUseCamera never changes on window resize. The [height, width] dependency array is now stale — it causes the memo to recompute on every window resize even though neither value is used in the computation.
Suggested change
| const canUseCamera = React.useMemo(() => { | |
| if (isRunningOnMac()) return false; | |
| if (Platform.OS !== 'web') return true; | |
| if (!isWebQrScannerSupported()) return false; | |
| return isWebMobileLikeQrScannerHost({ width, height }); | |
| return isWebQrScannerSupported(); | |
| }, [height, width]); | |
| const canUseCamera = React.useMemo(() => { | |
| if (isRunningOnMac()) return false; | |
| if (Platform.OS !== 'web') return true; | |
| return isWebQrScannerSupported(); | |
| }, []); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes yolo mode (and other non-default permission modes) stick reliably across the full session lifecycle, including when a session moves between remote and local execution. It also makes the system more resilient to corrupted or unexpected values in persisted permission mode storage.
What users get
Yolo mode stays on when you set it
Before this change, starting a session with
--dangerously-skip-permissionsworked fine until the session received messages from a remote source. Those messages could silently reset the permission mode back todefault, so the next time Claude spawned locally it would run without yolo mode, silently and with no indication anything had changed. This was especially confusing because everything looked normal from the outside.Now the CLI records the permission mode chosen at launch time in a dedicated
spawnPermissionModefield. That value is never touched by incoming remote messages or metadata updates. When building the local spawn command, the launcher treatsspawnPermissionModeas a floor: if the tracked per-message mode has drifted back todefaultbut the session was launched with something stronger, the launch intent wins.Garbled permission mode storage no longer causes silent failures
The UI was loading persisted permission modes with a bare
JSON.parseand trusting whatever came back. A corrupted or unexpected blob could silently inject invalid values into the permission mode map, which would then be forwarded to the CLI and cause confusing behavior downstream.Now the loader validates the shape of the parsed value and filters each entry through
isPermissionMode()before accepting it, so only recognized permission mode strings survive. Anything else is quietly discarded rather than propagated.macOS desktop app
The Tauri desktop build now auto-generates the MCP bridge capability file (
mcp-dev.json) during sidecar preparation, so the MCP bridge plugin is properly authorized in dev and debug builds. Previously this file had to exist manually or the MCP bridge would silently fail to load. The generation is idempotent, so repeated builds stay clean.The permission mode fixes above also matter directly for desktop sessions: a user who launches a local Claude session from the desktop app with permissions elevated will no longer have those silently stripped when the session receives remote messages.
Icons and QR scanning are also fixed. Ionicons now render correctly in the Tauri webview:
Ionicons.fontis included in the_layout.tsxfont map so the custom web font injection generates the required@font-facerule. QR code scanning is enabled on the desktop: the scanner and connect-account hook now bypass the mobile-viewport check when running inside Tauri, so users can scan a QR code to link their account and use the restore flow from the desktop app. The restore screen defaults to showing the camera scanner when the camera API is available, matching the expected flow on Tauri macOS.Confidence in these fixes
The health endpoint and offline relay detection both lacked tests, making it hard to trust changes in those areas. Both are now covered: the
/healthendpoint is verified to report correctly when the database is healthy and to return a clear error response when the database connection fails. The server features client is verified to report an error state when the relay is completely unreachable.The CLI session spawn mode behavior is also covered, giving the floor logic a regression guard so future changes can't quietly break it.
Housekeeping
The server
.gitignorenow ignores theprisma/migrations/directory, which was showing up as untracked noise from pglite test runs.Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores