refactor: remove QuickJS skills runtime#508
Merged
senamakel merged 6 commits intotinyhumansai:mainfrom Apr 11, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR removes the QuickJS-based "skills" subsystem across frontend and backend: runtime, CLI, JSON-RPC schemas/handlers, runtime bridges, schedulers, registry, many React skill UIs/hooks, Tauri skill commands, related types, and the Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DesktopDeepLinkListener
participant FrontendSkillManager as SkillManager
participant BackendCore as CoreRuntime
rect rgba(200,200,255,0.5)
Browser->>DesktopDeepLinkListener: open oauth deep link (params)
end
Note over DesktopDeepLinkListener,FrontendSkillManager: OLD flow (removed)
DesktopDeepLinkListener->>FrontendSkillManager: extract skillId, clientKey...
FrontendSkillManager->>BackendCore: startSkill(skillId)
BackendCore-->>FrontendSkillManager: started
FrontendSkillManager->>FrontendSkillManager: notifyOAuthComplete(...)
FrontendSkillManager->>Browser: navigate to skill UI
Note over DesktopDeepLinkListener,BackendCore: NEW flow (current)
DesktopDeepLinkListener-->>Browser: dispatch window event oauth:success {integrationId, toolkit}
DesktopDeepLinkListener->>Browser: navigate to `#/skills`
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
src/core/jsonrpc.rs (1)
821-823:⚠️ Potential issue | 🟡 MinorLog message references "skill" subscriber that is no longer registered.
The log lists "skill" among registered subscribers, but
register_skill_cleanup_subscriber()is no longer called in this function. This could cause confusion when debugging.📝 Suggested fix
log::info!( - "[event_bus] webhook, channel, health, skill, composio, restart subscribers + agent native handlers registered" + "[event_bus] webhook, channel, health, composio, restart subscribers + agent native handlers registered" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/jsonrpc.rs` around lines 821 - 823, The log message currently claims "skill" subscribers are registered but register_skill_cleanup_subscriber() is no longer called; either remove "skill" from the log string or reintroduce the missing call to register_skill_cleanup_subscriber() to keep the message accurate—update the logging statement that contains "[event_bus] webhook, channel, health, skill, composio, restart subscribers + agent native handlers registered" to reflect the actual subscribers registered by this function or add a call to register_skill_cleanup_subscriber() where other subscribers are registered.app/src/services/errorReportQueue.ts (1)
48-48:⚠️ Potential issue | 🟡 MinorRemove the unused
'skill'source from thePendingErrorReporttype union.The
'skill'literal is dead code—no code in the codebase callsenqueueErrorwithsource: 'skill'. Only'manual'and'global'sources are actually used. Since the PR removed the QuickJS skills runtime entirely, remove'skill'from the type definition to keep it consistent with the updated documentation and actual usage:source: 'react' | 'global' | 'manual';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/errorReportQueue.ts` at line 48, Update the PendingErrorReport type by removing the unused 'skill' literal from the source union (change source: 'react' | 'global' | 'manual') and then update any related type annotations/usages (e.g., enqueueError signature and its callers) to match the new union so there are no lingering references to 'skill'; search for PendingErrorReport and enqueueError to adjust types and remove dead branches that handle 'skill'.app/src/components/skills/SkillCard.tsx (1)
1-19:⚠️ Potential issue | 🟡 MinorUse
import typeforReactNodeprops.
UnifiedSkillCardPropsuses type-only React symbols (React.ReactNode) but imports React only for runtime values. Add a type import forReactNodeand use it directly to align with repo TypeScript conventions.Suggested change
import { useEffect, useRef, useState } from 'react'; +import type { ReactNode } from 'react'; export interface UnifiedSkillCardProps { - icon: React.ReactNode; + icon: ReactNode; title: string; description: string; @@ secondaryActions?: Array<{ label: string; - icon: React.ReactNode; + icon: ReactNode; onClick: () => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillCard.tsx` around lines 1 - 19, The props use the type React.ReactNode but the file imports React only for runtime hooks; add a type-only import for ReactNode and replace React.ReactNode usages in UnifiedSkillCardProps (and in the secondaryActions.icon type) with the imported ReactNode to follow repo TypeScript conventions; specifically add an import type { ReactNode } from 'react' and update UnifiedSkillCardProps.icon and secondaryActions[].icon to use ReactNode while leaving the existing runtime imports (useEffect, useRef, useState) unchanged.
🧹 Nitpick comments (5)
app/src/components/settings/panels/CronJobsPanel.tsx (2)
36-48: Add namespaced debug checkpoints in the updated cron load path.This changed flow now drives the panel’s primary fetch/refresh behavior but has no debug entry/success/failure checkpoints, which makes tracing harder during app-side + sidecar investigations.
Proposed logging patch
const loadCronSkills = useCallback(async () => { + console.debug('[cron-panel] loadCronSkills:start'); setLoading(true); setCoreError(null); try { await loadCoreCronJobs(); + console.debug('[cron-panel] loadCronSkills:success'); } catch (err) { const message = err instanceof Error ? err.message : String(err); + console.debug('[cron-panel] loadCronSkills:error', { message }); setCoreError(`Failed to load core cron jobs: ${message}`); } finally { + console.debug('[cron-panel] loadCronSkills:done'); setLoading(false); } }, [loadCoreCronJobs]);As per coding guidelines: “Add substantial, development-oriented logs on new/changed flows in TypeScript/React app code; use namespaced debug logs…”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/CronJobsPanel.tsx` around lines 36 - 48, Add namespaced debug checkpoints to the cron load flow in loadCronSkills: log an entry at the start, a success after loadCoreCronJobs completes, and a failure in the catch including the full error; use a consistent namespace like "app:settings:CronJobsPanel:loadCronSkills" and call the logger (or console.debug) before setLoading/setCoreError to aid tracing. Reference the loadCronSkills function and the loadCoreCronJobs call, and ensure the catch logs the error details while still setting setCoreError and finally toggling setLoading(false).
36-52: RenameloadCronSkillsto reflect current behavior.After runtime removal, this callback only loads core scheduler jobs. Renaming will reduce future confusion and keep call sites self-documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/CronJobsPanel.tsx` around lines 36 - 52, The function name loadCronSkills is misleading because it only loads core scheduler jobs; rename it to something explicit (e.g., loadCoreCronJobsOnly or loadCoreCronJobsWrapper) and update all references in this file: change the useCallback declaration name, the dependency array (keep loadCoreCronJobs), and the useEffect invocation (void loadCoreCronJobsOnly()); preserve the same async logic, error handling (setCoreError), and state updates (setLoading) so behavior is unchanged.app/src/utils/desktopDeepLinkListener.ts (1)
249-252: Stale simulation example still referencesskillId.The
__simulateDeepLinkexample comment at line 250 still includesskillId=notion, but the OAuth success handler no longer usesskillId. Consider updating the example to reflect the current expected parameters.- // window.__simulateDeepLink('openhuman://oauth/success?integrationId=69cafd0b103bd070232d3223&skillId=notion') + // window.__simulateDeepLink('openhuman://oauth/success?integrationId=69cafd0b103bd070232d3223&toolkit=notion')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/desktopDeepLinkListener.ts` around lines 249 - 252, Update the stale simulation comment for the test helper: change the sample URL passed to window.__simulateDeepLink so it matches the current OAuth success handler's expected query params (remove the obsolete skillId or replace it with the current param name used by handleDeepLinkUrls, e.g., provider) and ensure the example still shows a realistic integrationId; the helper is the win.__simulateDeepLink assignment and the call into handleDeepLinkUrls([url]).app/src/components/settings/panels/ConnectionsPanel.tsx (2)
85-85: Hardcoded status value creates dead code path.
connectionStatusis hardcoded to'setup_required'and never used dynamically. Since allconnectOptionshavecomingSoon: true, the badge rendering at lines 95-102 (which checksoption.skillId) will never execute. This appears to be placeholder code.Consider either:
- Removing the skill-specific badge logic entirely since it's unreachable
- Adding a comment explaining this is preserved for future use
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/ConnectionsPanel.tsx` at line 85, The code sets connectionStatus = 'setup_required' (type SkillConnectionStatus) and leaves all connectOptions with comingSoon: true, making the skill-specific badge rendering that checks option.skillId unreachable; either remove the dead badge logic that depends on option.skillId and related branches (clean up the conditional rendering around the badge) or keep the rendering but add a clear comment above connectionStatus and the connectOptions explaining this is placeholder preserved for future dynamic status and will be updated when connectOptions are no longer comingSoon; update or remove references to connectionStatus and the badge conditional accordingly (look for connectionStatus, SkillConnectionStatus, connectOptions, and option.skillId to locate the relevant code).
172-175:handleConnectis now a no-op for all current options.With all connection options marked as
comingSoon: true, and theskillIdcheck added at line 174,handleConnecteffectively does nothing for any option. This is acceptable as transitional code, but consider adding a brief comment or TODO to clarify the intended future behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/ConnectionsPanel.tsx` around lines 172 - 175, handleConnect currently returns early for all options because every ConnectOption has comingSoon=true and you also short-circuit when option.skillId is present; add a concise TODO comment inside the handleConnect function explaining that this is intentionally a temporary no-op and what should happen later (e.g., open a connection modal or dispatch a connect action when comingSoon is false or a skillId is provided). Reference the early-return checks (option.comingSoon and option.skillId) in the comment so future maintainers know where to implement the real connect flow (open modal, route, or call connect handler) when options become active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/components/skills/SkillCard.tsx`:
- Around line 1-19: The props use the type React.ReactNode but the file imports
React only for runtime hooks; add a type-only import for ReactNode and replace
React.ReactNode usages in UnifiedSkillCardProps (and in the
secondaryActions.icon type) with the imported ReactNode to follow repo
TypeScript conventions; specifically add an import type { ReactNode } from
'react' and update UnifiedSkillCardProps.icon and secondaryActions[].icon to use
ReactNode while leaving the existing runtime imports (useEffect, useRef,
useState) unchanged.
In `@app/src/services/errorReportQueue.ts`:
- Line 48: Update the PendingErrorReport type by removing the unused 'skill'
literal from the source union (change source: 'react' | 'global' | 'manual') and
then update any related type annotations/usages (e.g., enqueueError signature
and its callers) to match the new union so there are no lingering references to
'skill'; search for PendingErrorReport and enqueueError to adjust types and
remove dead branches that handle 'skill'.
In `@src/core/jsonrpc.rs`:
- Around line 821-823: The log message currently claims "skill" subscribers are
registered but register_skill_cleanup_subscriber() is no longer called; either
remove "skill" from the log string or reintroduce the missing call to
register_skill_cleanup_subscriber() to keep the message accurate—update the
logging statement that contains "[event_bus] webhook, channel, health, skill,
composio, restart subscribers + agent native handlers registered" to reflect the
actual subscribers registered by this function or add a call to
register_skill_cleanup_subscriber() where other subscribers are registered.
---
Nitpick comments:
In `@app/src/components/settings/panels/ConnectionsPanel.tsx`:
- Line 85: The code sets connectionStatus = 'setup_required' (type
SkillConnectionStatus) and leaves all connectOptions with comingSoon: true,
making the skill-specific badge rendering that checks option.skillId
unreachable; either remove the dead badge logic that depends on option.skillId
and related branches (clean up the conditional rendering around the badge) or
keep the rendering but add a clear comment above connectionStatus and the
connectOptions explaining this is placeholder preserved for future dynamic
status and will be updated when connectOptions are no longer comingSoon; update
or remove references to connectionStatus and the badge conditional accordingly
(look for connectionStatus, SkillConnectionStatus, connectOptions, and
option.skillId to locate the relevant code).
- Around line 172-175: handleConnect currently returns early for all options
because every ConnectOption has comingSoon=true and you also short-circuit when
option.skillId is present; add a concise TODO comment inside the handleConnect
function explaining that this is intentionally a temporary no-op and what should
happen later (e.g., open a connection modal or dispatch a connect action when
comingSoon is false or a skillId is provided). Reference the early-return checks
(option.comingSoon and option.skillId) in the comment so future maintainers know
where to implement the real connect flow (open modal, route, or call connect
handler) when options become active.
In `@app/src/components/settings/panels/CronJobsPanel.tsx`:
- Around line 36-48: Add namespaced debug checkpoints to the cron load flow in
loadCronSkills: log an entry at the start, a success after loadCoreCronJobs
completes, and a failure in the catch including the full error; use a consistent
namespace like "app:settings:CronJobsPanel:loadCronSkills" and call the logger
(or console.debug) before setLoading/setCoreError to aid tracing. Reference the
loadCronSkills function and the loadCoreCronJobs call, and ensure the catch logs
the error details while still setting setCoreError and finally toggling
setLoading(false).
- Around line 36-52: The function name loadCronSkills is misleading because it
only loads core scheduler jobs; rename it to something explicit (e.g.,
loadCoreCronJobsOnly or loadCoreCronJobsWrapper) and update all references in
this file: change the useCallback declaration name, the dependency array (keep
loadCoreCronJobs), and the useEffect invocation (void loadCoreCronJobsOnly());
preserve the same async logic, error handling (setCoreError), and state updates
(setLoading) so behavior is unchanged.
In `@app/src/utils/desktopDeepLinkListener.ts`:
- Around line 249-252: Update the stale simulation comment for the test helper:
change the sample URL passed to window.__simulateDeepLink so it matches the
current OAuth success handler's expected query params (remove the obsolete
skillId or replace it with the current param name used by handleDeepLinkUrls,
e.g., provider) and ensure the example still shows a realistic integrationId;
the helper is the win.__simulateDeepLink assignment and the call into
handleDeepLinkUrls([url]).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d90abd-6cb8-4196-b32f-57d59051fd6a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (94)
Cargo.tomlapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/panels/ConnectionsPanel.tsxapp/src/components/settings/panels/CronJobsPanel.tsxapp/src/components/settings/panels/RecoveryPhrasePanel.tsxapp/src/components/settings/panels/cron/RuntimeSkillCronList.tsxapp/src/components/skills/AuthModeSelector.tsxapp/src/components/skills/SetupFormRenderer.tsxapp/src/components/skills/SkillCard.tsxapp/src/components/skills/SkillDebugModal.tsxapp/src/components/skills/SkillManagementPanel.tsxapp/src/components/skills/SkillSetupModal.tsxapp/src/components/skills/SkillSetupWizard.tsxapp/src/components/skills/shared.tsxapp/src/features/autocomplete/useAutocompleteSkillStatus.tsapp/src/features/screen-intelligence/useScreenIntelligenceSkillStatus.tsapp/src/features/voice/useVoiceSkillStatus.tsapp/src/lib/composio/composioApi.tsapp/src/lib/skills/hooks.tsapp/src/lib/skills/index.tsapp/src/lib/skills/manager.tsapp/src/lib/skills/paths.tsapp/src/lib/skills/runtime.tsapp/src/lib/skills/skillEvents.tsapp/src/lib/skills/skillsApi.tsapp/src/lib/skills/sync.tsapp/src/lib/skills/transport.tsapp/src/lib/skills/types.tsapp/src/pages/Mnemonic.tsxapp/src/pages/__tests__/skillsSyncUi.test.tsapp/src/pages/onboarding/steps/SkillsStep.tsxapp/src/pages/skillsSyncUi.tsapp/src/services/errorReportQueue.tsapp/src/services/socketService.tsapp/src/types/skillStatus.tsapp/src/utils/desktopDeepLinkListener.tsapp/src/utils/tauriCommands/index.tsapp/src/utils/tauriCommands/skills.tssrc/core/all.rssrc/core/cli.rssrc/core/jsonrpc.rssrc/core/mod.rssrc/core/skills_cli.rssrc/openhuman/agent/agents/skills_agent/agent.tomlsrc/openhuman/agent/agents/skills_agent/prompt.mdsrc/openhuman/agent/harness/session/builder.rssrc/openhuman/composio/tools.rssrc/openhuman/skills/bridge/mod.rssrc/openhuman/skills/bridge/net.rssrc/openhuman/skills/bus.rssrc/openhuman/skills/cron_scheduler.rssrc/openhuman/skills/manifest.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/paths.rssrc/openhuman/skills/ping_scheduler.rssrc/openhuman/skills/preferences.rssrc/openhuman/skills/qjs_engine.rssrc/openhuman/skills/qjs_skill_instance/event_loop/mod.rssrc/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rssrc/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rssrc/openhuman/skills/qjs_skill_instance/instance.rssrc/openhuman/skills/qjs_skill_instance/js_handlers.rssrc/openhuman/skills/qjs_skill_instance/js_helpers.rssrc/openhuman/skills/qjs_skill_instance/mod.rssrc/openhuman/skills/qjs_skill_instance/types.rssrc/openhuman/skills/quickjs_libs/bootstrap.jssrc/openhuman/skills/quickjs_libs/mod.rssrc/openhuman/skills/quickjs_libs/qjs_ops/mod.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops_core.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops_net.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops_storage.rssrc/openhuman/skills/quickjs_libs/qjs_ops/ops_webhook.rssrc/openhuman/skills/quickjs_libs/qjs_ops/types.rssrc/openhuman/skills/quickjs_libs/storage.rssrc/openhuman/skills/registry_cache.rssrc/openhuman/skills/registry_ops.rssrc/openhuman/skills/registry_types.rssrc/openhuman/skills/schemas.rssrc/openhuman/skills/skill_registry.rssrc/openhuman/skills/types.rssrc/openhuman/skills/utils.rssrc/openhuman/skills/working_memory.rssrc/openhuman/subconscious/situation_report.rssrc/openhuman/tools/impl/agent/mod.rssrc/openhuman/tools/impl/agent/spawn_subagent.rssrc/openhuman/tools/impl/network/mod.rssrc/openhuman/tools/impl/network/skill_bridge.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/orchestrator_tools.rssrc/openhuman/tools/traits.rssrc/openhuman/webhooks/bus.rssrc/openhuman/webhooks/ops.rs
💤 Files with no reviewable changes (64)
- Cargo.toml
- src/core/mod.rs
- app/src/components/settings/panels/RecoveryPhrasePanel.tsx
- app/src/services/socketService.ts
- src/core/cli.rs
- src/openhuman/agent/harness/session/builder.rs
- src/openhuman/skills/quickjs_libs/mod.rs
- src/openhuman/skills/bridge/mod.rs
- app/src/pages/tests/skillsSyncUi.test.ts
- app/src/components/skills/AuthModeSelector.tsx
- src/core/all.rs
- app/src/pages/Mnemonic.tsx
- app/src/components/settings/panels/cron/RuntimeSkillCronList.tsx
- app/src/utils/tauriCommands/index.ts
- app/src/components/skills/SkillSetupModal.tsx
- src/openhuman/skills/cron_scheduler.rs
- src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs
- app/src/lib/skills/transport.ts
- src/core/skills_cli.rs
- app/src/lib/skills/paths.ts
- src/openhuman/skills/quickjs_libs/qjs_ops/ops.rs
- app/src/lib/skills/index.ts
- src/openhuman/skills/qjs_skill_instance/mod.rs
- app/src/lib/skills/skillEvents.ts
- app/src/components/skills/SkillDebugModal.tsx
- src/openhuman/skills/schemas.rs
- app/src/lib/skills/manager.ts
- src/openhuman/skills/qjs_skill_instance/types.rs
- src/openhuman/skills/paths.rs
- app/src/components/skills/shared.tsx
- src/openhuman/skills/quickjs_libs/qjs_ops/mod.rs
- app/src/components/skills/SetupFormRenderer.tsx
- src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs
- src/openhuman/skills/qjs_skill_instance/js_handlers.rs
- src/openhuman/skills/registry_types.rs
- src/openhuman/skills/ping_scheduler.rs
- src/openhuman/skills/quickjs_libs/qjs_ops/ops_net.rs
- app/src/lib/skills/types.ts
- app/src/components/skills/SkillSetupWizard.tsx
- src/openhuman/skills/utils.rs
- app/src/components/skills/SkillManagementPanel.tsx
- app/src/pages/skillsSyncUi.ts
- src/openhuman/skills/qjs_skill_instance/js_helpers.rs
- src/openhuman/skills/quickjs_libs/qjs_ops/types.rs
- app/src/lib/skills/runtime.ts
- src/openhuman/skills/quickjs_libs/qjs_ops/ops_storage.rs
- src/openhuman/skills/quickjs_libs/qjs_ops/ops_webhook.rs
- src/openhuman/skills/registry_cache.rs
- src/openhuman/skills/quickjs_libs/bootstrap.js
- src/openhuman/skills/quickjs_libs/qjs_ops/ops_core.rs
- app/src/lib/skills/skillsApi.ts
- src/openhuman/skills/bridge/net.rs
- src/openhuman/skills/registry_ops.rs
- src/openhuman/skills/manifest.rs
- app/src/utils/tauriCommands/skills.ts
- src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs
- src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs
- src/openhuman/skills/qjs_engine.rs
- app/src/lib/skills/hooks.ts
- src/openhuman/skills/quickjs_libs/storage.rs
- src/openhuman/skills/skill_registry.rs
- src/openhuman/skills/preferences.rs
- app/src/lib/skills/sync.ts
- src/openhuman/skills/qjs_skill_instance/instance.rs
- Removed the 'skill' source option from the error report structure to streamline error reporting. - Refactored the ConnectionsPanel component to simplify connection status badge rendering and improve clarity. - Updated the CronJobsPanel to enhance logging for cron job loading processes. - Adjusted SkillCard component to use a more consistent type for icons. - Deleted outdated end-to-end tests for Gmail and Notion skills, improving test suite maintainability.
…nent - Cleaned up the Conversations component by removing the ESLint disable comment for exhaustive dependencies in the useEffect hook, improving code clarity and maintainability.
- Eliminated an extra line of whitespace in the Conversations component, enhancing code readability and maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/settings/panels/CronJobsPanel.tsx (1)
17-48:⚠️ Potential issue | 🟡 MinorRename the debug namespace to match the core-only path.
Line 17 still uses
loadCronSkills, which is stale after removing runtime skills and makes log grep/correlation noisier.Suggested diff
-const loadCronJobsLog = createDebug('app:settings:CronJobsPanel:loadCronSkills'); +const loadCoreCronJobsLog = createDebug('app:settings:CronJobsPanel:loadCoreCronJobs'); - loadCronJobsLog('start'); + loadCoreCronJobsLog('start'); ... - loadCronJobsLog('success'); + loadCoreCronJobsLog('success'); ... - loadCronJobsLog('failure', err); + loadCoreCronJobsLog('failure', err);As per coding guidelines, "Use grep-friendly log prefixes ([feature], domain name, or JSON-RPC method) in app code for correlation with sidecar and Tauri output".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/CronJobsPanel.tsx` around lines 17 - 48, The debug namespace string assigned to loadCronJobsLog is stale ("app:settings:CronJobsPanel:loadCronSkills"); change it to a grep-friendly name that reflects the core-only path (e.g., "app:settings:CronJobsPanel:loadCoreCronJobs" or similar) and update all usages (loadCronJobsLog(...) calls in loadCoreCronJobsOnly and any other places) so logs correlate with the current CronJobsPanel core job flows; ensure the variable name loadCronJobsLog and the function loadCoreCronJobsOnly remain the same so the change is limited to the namespace string.
🧹 Nitpick comments (2)
app/test/Mnemonic.test.tsx (1)
449-458: Prefer behavior assertions over internal util-call assertions (and align test naming)On Line 513, the test name claims navigation to
/home, but it only assertsmockDeriveAesKeyinvocation. Also, both segments assert internal derivation calls, which are implementation-coupled. Prefer asserting user-visible outcomes/state effects (e.g., successful persistence + navigation), or rename tests so names match what is actually asserted.♻️ Suggested direction
- it('derives the encryption key from the imported phrase and navigates to /home', async () => { + it('stores encryption key after importing a valid recovery phrase', async () => { ... - expect(mockDeriveAesKey).toHaveBeenCalledWith(FIXED_MNEMONIC); + expect(mockSetEncryptionKey).toHaveBeenCalledWith('aes-key-hex'); + // add navigation assertion if this test intends to validate /home redirect });As per coding guidelines "Prefer testing behavior over implementation details in Vitest unit tests".
Also applies to: 513-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/Mnemonic.test.tsx` around lines 449 - 458, The test titled "calls deriveAesKeyFromMnemonic with the generated mnemonic" is asserting an internal util call (mockDeriveAesKey) but the name implies navigation to '/home' — change the spec to assert external behavior instead of implementation details: assert navigation to '/home' (check router history/location or call to navigate) and that the mnemonic/key was persisted (e.g., inspect localStorage or the store) after clicking continueButton(), or alternatively rename the test to accurately state it only verifies deriveAesKeyFromMnemonic was called with FIXED_MNEMONIC; locate the test using identifiers mockDeriveAesKey, continueButton(), and FIXED_MNEMONIC to update assertions or rename accordingly.src/core/jsonrpc.rs (1)
861-898: Make the socket bootstrap idempotent too.
register_domain_subscribersis guarded withOnce, but this block always creates a newSocketManager, replaces the global singleton, and spawns another auto-connect task. Ifbootstrap_skill_runtimeis called twice, different parts of the process can end up bound to different managers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/jsonrpc.rs` around lines 861 - 898, The socket bootstrap unconditionally creates a new SocketManager (SocketManager::new), calls set_global_socket_manager(socket_mgr.clone()), and spawns the auto-connect task, which breaks idempotency; change it to first check whether a global manager already exists (use the corresponding getter for the global socket manager) and only create/set a new SocketManager and spawn the tokio::spawn auto-connect block if none is present. Specifically, in the bootstrap block that defines socket_mgr and calls set_global_socket_manager and tokio::spawn, guard creation/set/spawn behind a "if global not set" check so repeated calls to bootstrap_skill_runtime reuse the existing manager instead of replacing it.
🤖 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/SkillCard.tsx`:
- Around line 1-2: The file currently has two imports from 'react' (useEffect,
useRef, useState) and a separate type-only import for ReactNode which triggers
the no-duplicate-imports lint rule; merge them into a single import by adding
the type inline (e.g. include "type ReactNode") alongside the existing named
imports so only one import statement imports useEffect, useRef, useState and the
type ReactNode from 'react'.
In `@src/core/jsonrpc.rs`:
- Around line 831-835: The match on
crate::openhuman::config::Config::load_or_init().await currently returns early
on error which skips event-bus init, subscriber/native-handler registration and
socket setup while run_server_inner still marks the core as ready; change this
so bootstrap failure aborts startup instead of silently returning: propagate or
map the Config load error out of the caller (or call a controlled shutdown/exit)
so run_server_inner cannot proceed, or alternatively move config-independent
bootstrap steps (event-bus init, subscriber/native-handler registration, and
socket setup) above the Config::load_or_init() branch; update the code paths
around run_server_inner, the socket manager initialization, and the
event-bus/subscriber registration to ensure they either run before calling
load_or_init() or are not executed when load_or_init() fails.
- Around line 492-499: The SSE stream currently uses tokio_stream::once(...)
which ends immediately and causes clients to reconnect; replace that one-shot
stream with a stream that emits the sentinel Event (the
Event::default().event("webhooks_debug").data(...)) and then never terminates so
the connection stays open. Concretely, change the tokio_stream::once(...) usage
to a chained stream that yields the same Ok<Event> sentinel and then a
never-ending pending stream (i.e., chain the single-item stream with a
pending/never stream) before passing it to
Sse::new(...).keep_alive(...).into_response() so the connection remains open
after emitting the runtime_removed event.
---
Outside diff comments:
In `@app/src/components/settings/panels/CronJobsPanel.tsx`:
- Around line 17-48: The debug namespace string assigned to loadCronJobsLog is
stale ("app:settings:CronJobsPanel:loadCronSkills"); change it to a
grep-friendly name that reflects the core-only path (e.g.,
"app:settings:CronJobsPanel:loadCoreCronJobs" or similar) and update all usages
(loadCronJobsLog(...) calls in loadCoreCronJobsOnly and any other places) so
logs correlate with the current CronJobsPanel core job flows; ensure the
variable name loadCronJobsLog and the function loadCoreCronJobsOnly remain the
same so the change is limited to the namespace string.
---
Nitpick comments:
In `@app/test/Mnemonic.test.tsx`:
- Around line 449-458: The test titled "calls deriveAesKeyFromMnemonic with the
generated mnemonic" is asserting an internal util call (mockDeriveAesKey) but
the name implies navigation to '/home' — change the spec to assert external
behavior instead of implementation details: assert navigation to '/home' (check
router history/location or call to navigate) and that the mnemonic/key was
persisted (e.g., inspect localStorage or the store) after clicking
continueButton(), or alternatively rename the test to accurately state it only
verifies deriveAesKeyFromMnemonic was called with FIXED_MNEMONIC; locate the
test using identifiers mockDeriveAesKey, continueButton(), and FIXED_MNEMONIC to
update assertions or rename accordingly.
In `@src/core/jsonrpc.rs`:
- Around line 861-898: The socket bootstrap unconditionally creates a new
SocketManager (SocketManager::new), calls
set_global_socket_manager(socket_mgr.clone()), and spawns the auto-connect task,
which breaks idempotency; change it to first check whether a global manager
already exists (use the corresponding getter for the global socket manager) and
only create/set a new SocketManager and spawn the tokio::spawn auto-connect
block if none is present. Specifically, in the bootstrap block that defines
socket_mgr and calls set_global_socket_manager and tokio::spawn, guard
creation/set/spawn behind a "if global not set" check so repeated calls to
bootstrap_skill_runtime reuse the existing manager instead of replacing it.
🪄 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: a77f812d-518e-4796-b733-0d9396510dbb
📒 Files selected for processing (16)
app/src/components/ErrorReportNotification.tsxapp/src/components/settings/panels/ConnectionsPanel.tsxapp/src/components/settings/panels/CronJobsPanel.tsxapp/src/components/skills/SkillCard.tsxapp/src/pages/Conversations.tsxapp/src/services/errorReportQueue.tsapp/src/utils/desktopDeepLinkListener.tsapp/test/Mnemonic.test.tsxsrc/core/jsonrpc.rstests/json_rpc_e2e.rstests/skills_debug_e2e.rstests/skills_gmail_e2e.rstests/skills_gmail_oauth_proxy_rpc_e2e.rstests/skills_notion_live.rstests/skills_rpc_e2e.rstests/skills_sync_memory_test.rs
💤 Files with no reviewable changes (2)
- app/src/components/ErrorReportNotification.tsx
- app/src/pages/Conversations.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/services/errorReportQueue.ts
- app/src/components/settings/panels/ConnectionsPanel.tsx
Comment on lines
+492
to
499
| let stream = tokio_stream::once(Ok::<Event, std::convert::Infallible>( | ||
| Event::default() | ||
| .event("webhooks_debug") | ||
| .data("{\"event_type\":\"runtime_removed\"}"), | ||
| )); | ||
| Sse::new(stream) | ||
| .keep_alive(KeepAlive::new().interval(std::time::Duration::from_secs(10))) | ||
| .into_response() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the webhook SSE connection open after the sentinel event.
tokio_stream::once(...) finishes immediately after the first frame, so EventSource clients will reconnect on every close. That turns the compatibility shim into a reconnect loop instead of a stable “runtime removed” signal.
💡 One way to keep the existing SSE contract stable
async fn webhook_events_handler() -> Response {
let stream = tokio_stream::once(Ok::<Event, std::convert::Infallible>(
Event::default()
.event("webhooks_debug")
.data("{\"event_type\":\"runtime_removed\"}"),
- ));
+ ))
+ .chain(tokio_stream::pending::<Result<Event, std::convert::Infallible>>());
Sse::new(stream)
.keep_alive(KeepAlive::new().interval(std::time::Duration::from_secs(10)))
.into_response()
}📝 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
| let stream = tokio_stream::once(Ok::<Event, std::convert::Infallible>( | |
| Event::default() | |
| .event("webhooks_debug") | |
| .data("{\"event_type\":\"runtime_removed\"}"), | |
| )); | |
| Sse::new(stream) | |
| .keep_alive(KeepAlive::new().interval(std::time::Duration::from_secs(10))) | |
| .into_response() | |
| let stream = tokio_stream::once(Ok::<Event, std::convert::Infallible>( | |
| Event::default() | |
| .event("webhooks_debug") | |
| .data("{\"event_type\":\"runtime_removed\"}"), | |
| )) | |
| .chain(tokio_stream::pending::<Result<Event, std::convert::Infallible>>()); | |
| Sse::new(stream) | |
| .keep_alive(KeepAlive::new().interval(std::time::Duration::from_secs(10))) | |
| .into_response() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/jsonrpc.rs` around lines 492 - 499, The SSE stream currently uses
tokio_stream::once(...) which ends immediately and causes clients to reconnect;
replace that one-shot stream with a stream that emits the sentinel Event (the
Event::default().event("webhooks_debug").data(...)) and then never terminates so
the connection stays open. Concretely, change the tokio_stream::once(...) usage
to a chained stream that yields the same Ok<Event> sentinel and then a
never-ending pending stream (i.e., chain the single-item stream with a
pending/never stream) before passing it to
Sse::new(...).keep_alive(...).into_response() so the connection remains open
after emitting the runtime_removed event.
Comment on lines
+831
to
835
| let cfg = match crate::openhuman::config::Config::load_or_init().await { | ||
| Ok(cfg) => cfg, | ||
| Err(e) => { | ||
| log::error!("[runtime] Failed to create RuntimeEngine: {e}"); | ||
| log::error!("[runtime] Failed to load config for socket manager: {e}"); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don't silently continue startup after bootstrap config failure.
This early return skips event-bus init, subscriber/native-handler registration, and socket setup, but run_server_inner still announces the core as ready. Either move the config-independent bootstrap above this branch or make bootstrap failure abort startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/jsonrpc.rs` around lines 831 - 835, The match on
crate::openhuman::config::Config::load_or_init().await currently returns early
on error which skips event-bus init, subscriber/native-handler registration and
socket setup while run_server_inner still marks the core as ready; change this
so bootstrap failure aborts startup instead of silently returning: propagate or
map the Config load error out of the caller (or call a controlled shutdown/exit)
so run_server_inner cannot proceed, or alternatively move config-independent
bootstrap steps (event-bus init, subscriber/native-handler registration, and
socket setup) above the Config::load_or_init() branch; update the code paths
around run_server_inner, the socket manager initialization, and the
event-bus/subscriber registration to ensure they either run before calling
load_or_init() or are not executed when load_or_init() fails.
- Combined import statements in the SkillCard component to enhance code readability and maintainability.
4 tasks
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 modulesVerification run:
cargo check --manifest-path Cargo.tomlyarn typecheckformat:check,lint,tsc --noEmit,cargo check --manifest-path app/src-tauri/Cargo.tomlImpact
Related
Summary by CodeRabbit
Removed Features
Simplified Setup
Updated Documentation
Other