Fix/skill start issue#498
Merged
M3gA-Mind merged 9 commits intotinyhumansai:mainfrom Apr 10, 2026
Merged
Conversation
- Added `overlay/src-tauri/target/` to .gitignore to prevent tracking of build artifacts. - Updated the openhuman package version from 0.52.0 to 0.52.2 in Cargo.lock files for both the main and app/src-tauri directories. - Enhanced entitlements for macOS Hardened Runtime to allow outbound HTTPS calls and server connections. - Refactored registry operations to use rustls explicitly, improving network reliability on macOS.
…s function - Updated the logging statement in the fetch_url_bytes function to enhance readability by formatting the debug message across multiple lines. This change improves clarity in log outputs, making it easier to track the number of bytes fetched from URLs.
…ation - Introduced a managed OAuth auto-advance mechanism to ensure it runs only once per login attempt, improving user experience during authentication. - Updated the SkillSetupWizard to handle skill runtime checks more effectively, ensuring that the skill starts correctly and transitions to the setup phase seamlessly. - Enhanced the useSkillSnapshot hook to provide a synthesized offline snapshot when the skill is not yet running, preventing UI stalls during loading. - Implemented background synchronization after OAuth completion to ensure users see fresh data immediately without blocking the UI. - Added tests to validate the new behavior for skills setup completion and status retrieval without requiring the skill to be started first.
…s_status function - Simplified the retrieval of the `setup_complete` variable by removing unnecessary line breaks, enhancing code readability and maintainability. - This change improves the clarity of the function's logic without altering its functionality.
…om OAuth flow - Updated the success message in the SkillSetupWizard to remove references to background syncing, streamlining user communication. - Removed the initial sync trigger from the SkillManager after OAuth completion, shifting the responsibility for data synchronization to the user interface or cron jobs. - Adjusted comments in the desktopDeepLinkListener to reflect the new sync behavior, clarifying that initial data sync is no longer automatic.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors OAuth completion and skill startup flows to always start skills and poll for a running state, remove fire‑and‑forget post‑OAuth syncs, synthesize offline snapshots on RPC failures, sync QuickJS workspace_dir during install/start, and add Rust and Playwright E2E tests plus shared frontend test mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as SkillSetupWizard (UI)
participant Deep as DesktopDeepLinkListener
participant Manager as SkillManager
participant Engine as RuntimeEngine/Skill
User->>UI: Initiate OAuth / open external URL
Note right of User: Complete OAuth in browser
User->>Deep: Deep link (openhuman://oauth/success)
Deep->>Manager: notifyOAuthComplete(oauthParams)
Deep->>Manager: best-effort startSkill(skillId)
Manager->>Engine: engine.start_skill(skillId)
Engine-->>Manager: start result (success / error)
Manager->>Manager: emitSkillStateChange(skillId) rgba(60,120,180,0.5)
Manager-->>UI: persist/emit setup_complete state
UI->>User: Show "Connected — You can close this window."
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/skills/SkillSetupWizard.tsx`:
- Around line 191-212: Currently the UI sets phase "complete" before running
async setup; instead, keep the wizard non-terminal until
skillManager.startSetup(skillId) resolves: start or fire-and-forget
startSkill(skillId) if you need it concurrently, but await
skillManager.startSetup(skillId) and then if it returns null call setState({
phase: "complete", message: ... }), otherwise call setState({ phase: "step",
step: firstStep }); on any exception from skillManager.startSetup catch it,
console.warn it and call setState({ phase: "error", message: "Setup failed" })
so the wizard doesn't show success when setup failed (refer to startSkill,
skillManager.startSetup, setState and the "complete"/"step"/"error" phase
values).
In `@app/src/lib/skills/hooks.ts`:
- Around line 22-33: The offlineSkillSnapshot helper currently hard-codes
setup_complete:false and resets previous snapshot data; change it to a const
arrow that preserves the last known snapshot values (at minimum carry forward
setup_complete and any non-empty name/tools/state/connection fields) when
available instead of overwriting them, e.g. accept an optional previousSnapshot
parameter or read the cached snapshot used by useSkillSnapshot and merge fields
into the offline fallback; update the declaration to a const arrow named
offlineSkillSnapshot and apply the same preservation/arrow-fn change to the
other occurrence mentioned (lines ~95-100) so transient RPC failures don't flip
the UI.
In `@app/test/e2e/specs/skill-execution-flow.spec.ts`:
- Around line 116-128: The test "skills_set_setup_complete + skills_status
without start (OAuth persistence path)" sets setup_complete to true for the
shared E2E_RUNTIME_SKILL_ID and never clears it; wrap the
callOpenhumanRpc('openhuman.skills_set_setup_complete', { skill_id:
E2E_RUNTIME_SKILL_ID, complete: true }) and subsequent assertions in a
try/finally and in the finally call
callOpenhumanRpc('openhuman.skills_set_setup_complete', { skill_id:
E2E_RUNTIME_SKILL_ID, complete: false }) to restore state; use the same
callOpenhumanRpc helper and E2E_RUNTIME_SKILL_ID constant so the test is
isolated and does not leak setup_complete across specs.
In `@tests/json_rpc_e2e.rs`:
- Around line 1549-1551: The test currently defines a JS helper function
onOAuthComplete and asserts its return value, but the production oauth/complete
runtime handler injects the credential, calls start(), and returns its own
payload without invoking onOAuthComplete; remove the test-only onOAuthComplete
helper and its { ok: true } assertion, and instead call the real oauth/complete
handler (the same pathway that injects the credential and calls start()) and
assert the actual handler response; update the duplicated cases (also around the
1839-1901 range) to validate the real handler result rather than the non-invoked
onOAuthComplete hook.
🪄 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
Run ID: 16d4c9ed-80ee-4906-a5d0-902d96ecff42
📒 Files selected for processing (8)
app/src/components/skills/SkillSetupWizard.tsxapp/src/lib/skills/hooks.tsapp/src/lib/skills/manager.tsapp/src/utils/desktopDeepLinkListener.tsapp/test/e2e/specs/skill-execution-flow.spec.tsapp/test/e2e/specs/skill-oauth.spec.tssrc/openhuman/skills/schemas.rstests/json_rpc_e2e.rs
💤 Files with no reviewable changes (1)
- app/src/lib/skills/manager.ts
- SkillSetupWizard: only show complete after startSetup succeeds; error on failures - hooks: merge prior snapshot into offline fallback; use const arrow for helper - E2E: reset skills_set_setup_complete in finally for isolation - json_rpc_e2e: assert oauth/complete returns start() result; add minimal start() - Skills page tests: mock screen-intelligence/autocomplete/voice hooks (CoreStateProvider) Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/skills/SkillSetupWizard.tsx (1)
69-88:⚠️ Potential issue | 🟠 MajorWait for the skill to be ready before calling
startSetup().
startSkill()does not guarantee the runtime is already setup-ready here. The repo’s own E2E coverage treatsopenhuman.skills_startas asynchronous and allows"initializing"before later interactions, so both of these newstartSkill(skillId) -> startSetup(skillId)sequences can still race and show a false setup error right after auth/OAuth on slower machines. GatestartSetup()on a confirmed running snapshot/status, or centralize this in a helper that waits until startup finishes.🛠️ Suggested direction
+async function waitForSkillRunning(skillId: string): Promise<void> { + const deadline = Date.now() + 10_000; + while (Date.now() < deadline) { + const snapshot = await getSkillSnapshot(skillId); + if (snapshot.status === "running") return; + if (snapshot.status === "error") { + throw new Error(snapshot.error ?? "Skill failed to start"); + } + await new Promise(resolve => setTimeout(resolve, 250)); + } + throw new Error("Timed out waiting for skill to start"); +} + const transitionToSetup = useCallback(async () => { try { await startSkill(skillId); + await waitForSkillRunning(skillId); const firstStep = await skillManager.startSetup(skillId);Also applies to: 187-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillSetupWizard.tsx` around lines 69 - 88, transitionToSetup can race because startSkill(skillId) may return before the runtime is ready, causing skillManager.startSetup(skillId) to fail; fix by gating startSetup on a readiness check: after startSkill(skillId) completes, poll or call a helper like waitForSkillReady(skillId) (or implement one) that queries the runtime/status until it reports running/ready (with timeout/retries), then call skillManager.startSetup(skillId); centralize this logic into a reusable helper (e.g., waitForSkillReady) and use it in transitionToSetup (and the other similar block around lines 187-214) so startSetup is only invoked when the skill is confirmed ready.
🧹 Nitpick comments (1)
app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx (1)
49-86: Extract the shared skill-status mocks into a test helper.These three
vi.mock(...)blocks are effectively the same ones added inapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx. Keeping them duplicated will make these specs drift the next time one of the status-hook return shapes changes. Pulling them behind a shared helper inapp/src/test/would keep both tests aligned from one place.As per coding guidelines, "use existing helpers from
app/src/test/before adding new harness code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx` around lines 49 - 86, Extract the three duplicated vi.mock(...) blocks for useScreenIntelligenceSkillStatus, useAutocompleteSkillStatus, and useVoiceSkillStatus into a single shared test helper (e.g., registerDefaultSkillStatusMocks) under the test helpers and export it; replace the three vi.mock blocks in this test with a single import-and-call of that helper and update the other spec that had the same mocks to import the same helper so both tests use the centralized mock implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/json_rpc_e2e.rs`:
- Around line 1872-1880: Replace the fixed
tokio::time::sleep(Duration::from_millis(400)).await with a readiness poll that
repeatedly calls post_json_rpc for "openhuman.skills_status" (similar to how
"skills_start" was invoked) and uses assert_no_jsonrpc_error to check the
response; parse the returned JSON status field and loop until status ==
"running" or a reasonable timeout is reached (failing the test on timeout). Keep
using the existing helpers post_json_rpc and assert_no_jsonrpc_error and ensure
the poll breaks as soon as "running" is observed to avoid flakiness.
---
Outside diff comments:
In `@app/src/components/skills/SkillSetupWizard.tsx`:
- Around line 69-88: transitionToSetup can race because startSkill(skillId) may
return before the runtime is ready, causing skillManager.startSetup(skillId) to
fail; fix by gating startSetup on a readiness check: after startSkill(skillId)
completes, poll or call a helper like waitForSkillReady(skillId) (or implement
one) that queries the runtime/status until it reports running/ready (with
timeout/retries), then call skillManager.startSetup(skillId); centralize this
logic into a reusable helper (e.g., waitForSkillReady) and use it in
transitionToSetup (and the other similar block around lines 187-214) so
startSetup is only invoked when the skill is confirmed ready.
---
Nitpick comments:
In `@app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx`:
- Around line 49-86: Extract the three duplicated vi.mock(...) blocks for
useScreenIntelligenceSkillStatus, useAutocompleteSkillStatus, and
useVoiceSkillStatus into a single shared test helper (e.g.,
registerDefaultSkillStatusMocks) under the test helpers and export it; replace
the three vi.mock blocks in this test with a single import-and-call of that
helper and update the other spec that had the same mocks to import the same
helper so both tests use the centralized mock implementations.
🪄 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
Run ID: a0d54b26-3094-4bad-94ad-02d07b28e112
📒 Files selected for processing (6)
app/src/components/skills/SkillSetupWizard.tsxapp/src/lib/skills/hooks.tsapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsxapp/test/e2e/specs/skill-execution-flow.spec.tstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- app/test/e2e/specs/skill-execution-flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/lib/skills/hooks.ts
- SkillSetupWizard: waitForSkillRunning after startSkill before startSetup/auth RPC - json_rpc_e2e: poll skills_status until running instead of fixed 400ms sleep - Consolidate Skills page vi.mocks in test/mockDefaultSkillStatusHooks.ts Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/test/mockDefaultSkillStatusHooks.ts (1)
8-17: Optional: Extract a shared offline-status base objectThe repeated status literals are consistent now, but centralizing them would reduce drift and simplify future updates.
♻️ Optional refactor
import { vi } from 'vitest'; +const defaultOfflineStatus = { + connectionStatus: 'offline', + statusDot: 'bg-stone-400', + statusLabel: 'Offline', + statusColor: 'text-stone-500', + ctaLabel: 'Enable', + ctaVariant: 'sage', +}; + vi.mock('../features/screen-intelligence/useScreenIntelligenceSkillStatus', () => ({ useScreenIntelligenceSkillStatus: () => ({ - connectionStatus: 'offline', - statusDot: 'bg-stone-400', - statusLabel: 'Offline', - statusColor: 'text-stone-500', - ctaLabel: 'Enable', - ctaVariant: 'sage', + ...defaultOfflineStatus, allPermissionsGranted: false, platformUnsupported: false, }), })); vi.mock('../features/autocomplete/useAutocompleteSkillStatus', () => ({ useAutocompleteSkillStatus: () => ({ - connectionStatus: 'offline', - statusDot: 'bg-stone-400', - statusLabel: 'Offline', - statusColor: 'text-stone-500', - ctaLabel: 'Enable', - ctaVariant: 'sage', + ...defaultOfflineStatus, platformUnsupported: false, }), })); vi.mock('../features/voice/useVoiceSkillStatus', () => ({ useVoiceSkillStatus: () => ({ - connectionStatus: 'offline', - statusDot: 'bg-stone-400', - statusLabel: 'Offline', - statusColor: 'text-stone-500', - ctaLabel: 'Enable', - ctaVariant: 'sage', + ...defaultOfflineStatus, sttModelMissing: false, voiceStatus: null, serverStatus: null, }), }));Also applies to: 21-29, 33-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/mockDefaultSkillStatusHooks.ts` around lines 8 - 17, Extract a shared constant object (e.g., offlineStatusBase) containing the repeated offline literals (connectionStatus, statusDot, statusLabel, statusColor, ctaLabel, ctaVariant, allPermissionsGranted, platformUnsupported) and replace the inline literal in useScreenIntelligenceSkillStatus with a shallow clone/merge of that constant; do the same for the other hook factories that use the same values (the other occurrences referenced around the same file), so each hook returns { ...offlineStatusBase, <hook-specific overrides if any> } to centralize and avoid drift.app/src/components/skills/SkillSetupWizard.tsx (1)
31-44: Prefer aconstarrow forwaitForSkillRunning.This new helper is module-level production code, so it should follow the repo’s TS style instead of introducing another function declaration.
♻️ Suggested refactor
-async function waitForSkillRunning(skillId: string): Promise<void> { +const waitForSkillRunning = async (skillId: string): Promise<void> => { const deadline = Date.now() + SKILL_RUNNING_WAIT_MS; while (Date.now() < deadline) { const snapshot = await getSkillSnapshot(skillId); if (snapshot.status === "running") return; if (snapshot.status === "error") { throw new Error(snapshot.error ? String(snapshot.error) : "Skill failed to start"); } await new Promise<void>(resolve => { setTimeout(resolve, SKILL_RUNNING_POLL_MS); }); } throw new Error("Timed out waiting for skill to start"); -} +};As per coding guidelines,
**/*.{js,jsx,ts,tsx}: “Use const by default, let if reassignment is needed, avoid var” and “Prefer arrow functions over function declarations.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillSetupWizard.tsx` around lines 31 - 44, The function declaration waitForSkillRunning should be converted to a module-level const arrow per project TS style: replace the `async function waitForSkillRunning(skillId: string): Promise<void> { ... }` declaration with a `const waitForSkillRunning = async (skillId: string): Promise<void> => { ... }` arrow so the behavior remains identical but follows the "use const" and "prefer arrow functions" rules; ensure any consumers still reference the same identifier and that no hoisting-dependent code relies on the old declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/skills/SkillSetupWizard.tsx`:
- Around line 244-250: Do not move the wizard to the "complete" state until
persistence succeeds: call setSetupComplete(skillId, true) and wait for it to
resolve (or handle its rejection) before calling setState({ phase: "complete",
message: "Successfully connected! You can close this window." }); if
setSetupComplete fails, surface an error state/message instead of swallowing the
error (remove the empty .catch(() => {}) and either await the promise or chain
.then/.catch to only set the success state on success and set an error
phase/message on failure).
---
Nitpick comments:
In `@app/src/components/skills/SkillSetupWizard.tsx`:
- Around line 31-44: The function declaration waitForSkillRunning should be
converted to a module-level const arrow per project TS style: replace the `async
function waitForSkillRunning(skillId: string): Promise<void> { ... }`
declaration with a `const waitForSkillRunning = async (skillId: string):
Promise<void> => { ... }` arrow so the behavior remains identical but follows
the "use const" and "prefer arrow functions" rules; ensure any consumers still
reference the same identifier and that no hoisting-dependent code relies on the
old declaration.
In `@app/src/test/mockDefaultSkillStatusHooks.ts`:
- Around line 8-17: Extract a shared constant object (e.g., offlineStatusBase)
containing the repeated offline literals (connectionStatus, statusDot,
statusLabel, statusColor, ctaLabel, ctaVariant, allPermissionsGranted,
platformUnsupported) and replace the inline literal in
useScreenIntelligenceSkillStatus with a shallow clone/merge of that constant; do
the same for the other hook factories that use the same values (the other
occurrences referenced around the same file), so each hook returns {
...offlineStatusBase, <hook-specific overrides if any> } to centralize and avoid
drift.
🪄 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
Run ID: 3f7d557a-3956-4d94-bc3c-a61fb8e9055f
📒 Files selected for processing (5)
app/src/components/skills/SkillSetupWizard.tsxapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsxapp/src/test/mockDefaultSkillStatusHooks.tstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- app/src/pages/tests/Skills.third-party-notion-debug-tools.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/tests/Skills.third-party-gmail-sync.test.tsx
…SkillRunning, mock base - Legacy OAuth: await persistence before complete; error on failure; guard ref + reset on skillId - waitForSkillRunning: const arrow per TS style - mockDefaultSkillStatusHooks: offlineStatusBase spread for shared literals Made-with: Cursor
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.
Summary
Problem
Solution
Submission Checklist
app/) and/orcargo test(core) for logic you add or changeapp/test/e2e, mock backend,tests/json_rpc_e2e.rsas appropriate)//////!(Rust), JSDoc or brief file/module headers (TS) on public APIs and non-obvious modules(Any feature related checklist can go in here)
Impact
Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests