Feat/chat issue#441
Merged
M3gA-Mind merged 9 commits intotinyhumansai:mainfrom Apr 8, 2026
Merged
Conversation
… flag in handler Consolidate tauriCommands imports and drop redundant mock cast. Handle granted accessibility in focus/visibility callback instead of a follow-up effect. Made-with: Cursor
- Introduced an optional environment variable `OPENHUMAN_DOTENV_PATH` to specify a custom path for dotenv files, enhancing configuration flexibility. - Updated `Cargo.toml` to include the `dotenvy` dependency for improved dotenv file handling. - Enhanced the `.env.example` file with a new comment for the custom dotenv path. - Added data-testid attributes and button types in `SkillDebugModal` and `Skills` components for better testability. - Created new tests for Gmail and Notion third-party skills to ensure proper functionality of sync and debug tools. - Added documentation for memory sync functions to clarify usage patterns and function details.
Contributor
📝 WalkthroughWalkthroughAdds optional dotenv path support for server startup, a dotenvy dependency, QuickJS runtime helpers for backend URL resolution and notifications, multiple UI testability attributes and new frontend tests, extensive memory subsystem documentation, new Rust and JS skill/runtime helpers, and several integration/end-to-end tests for Gmail/Notion skill flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test Runner
participant Server as OpenHuman RPC Server
participant Runtime as Skills Runtime
participant Backend as Staging Backend
participant Memory as MemoryClient
Test->>Server: start RPC server & env setup (incl. dotenv)
Test->>Server: openhuman.skills_start('gmail')
Server->>Runtime: initialize/start gmail skill
Test->>Server: openhuman.skills_rpc('gmail', oauth/complete)
Server->>Runtime: inject OAuth credentials & clientKeyShare
Test->>Server: openhuman.skills_call_tool('gmail','get-profile')
Runtime->>Backend: oauth.fetch (BACKEND_URL resolved)
Backend-->>Runtime: profile response
Runtime-->>Server: tool result (content)
Test->>Server: openhuman.skills_sync('gmail')
Server->>Runtime: trigger sync
Runtime->>Memory: persist docs to skill-gmail namespace
Test->>Memory: poll for persisted documents
Test->>Server: openhuman.skills_stop('gmail')
Server->>Runtime: stop gmail skill
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 8
🧹 Nitpick comments (5)
app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx (1)
160-167: Consider making this test async withwaitForfor robustness.The sync button's click handler (
handleSyncinSkills.tsx) is async. While the current synchronous assertion works becausetriggerSyncis called before the firstawait, usingasync/waitForwould make the test more resilient to implementation changes.🔧 Suggested improvement
- it('Sync control calls skillManager.triggerSync(notion)', () => { + it('Sync control calls skillManager.triggerSync(notion)', async () => { renderWithProviders(<Skills />, { initialEntries: ['/skills'] }); fireEvent.click(screen.getByTestId('skill-sync-button-notion')); - expect(mocks.triggerSync).toHaveBeenCalledTimes(1); - expect(mocks.triggerSync).toHaveBeenCalledWith('notion'); + await waitFor(() => { + expect(mocks.triggerSync).toHaveBeenCalledTimes(1); + }); + expect(mocks.triggerSync).toHaveBeenCalledWith('notion'); });Note: You'll also need to add
waitForto the imports on line 6.🤖 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 160 - 167, The test "Sync control calls skillManager.triggerSync(notion)" should be made async and use waitFor to ensure the async handleSync in Skills.tsx completes before assertions: change the test function to async, import waitFor from testing-library, click the button as before, then wrap the two expect(...) assertions inside an await waitFor(...) block (checking mocks.triggerSync called once and calledWith('notion')), so the test remains robust if triggerSync moves past the first await.app/src/components/skills/SkillDebugModal.tsx (1)
131-137: Consider addingtype="button"to the close button for consistency.The tab and tool buttons now have explicit
type="button", but the modal close button is missing it.🔧 Suggested fix
<button + type="button" onClick={onClose} className="p-1 text-stone-400 hover:text-stone-900 transition-colors rounded-lg hover:bg-stone-100">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillDebugModal.tsx` around lines 131 - 137, The modal close button in the SkillDebugModal component is missing an explicit button type; update the close <button> element (the one rendering the SVG close icon and using onClose) to include type="button" to prevent it from acting as a form submitter and to match the other explicit tab/tool buttons' behavior.app/src/pages/Skills.tsx (1)
306-327: Consider addingtype="button"to the Settings button for consistency.The Sync and Debug buttons now have explicit
type="button", but the Settings button at line 306 is missing it. While not strictly necessary outside a<form>, adding it maintains consistency across all action buttons in this component.🔧 Suggested consistency fix
{/* Settings */} <button + type="button" onClick={e => { e.stopPropagation(); onSetup(); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Skills.tsx` around lines 306 - 327, The Settings button JSX (the <button> with onClick that calls onSetup) is missing an explicit type, so add type="button" to that button element to match the Sync and Debug buttons and avoid it being treated as a submit inside forms; update the button where onClick={e => { e.stopPropagation(); onSetup(); }} is defined to include type="button" so behavior remains unchanged but consistent across the component.app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx (1)
59-73: Consider making this test async withwaitForfor robustness.Similar to the Notion test, the sync handler is async. Using
waitForwould make the assertion more resilient.🔧 Suggested improvement
- it('renders 3rd Party Skills and Gmail with a Sync control when connected', () => { + it('renders 3rd Party Skills and Gmail with a Sync control when connected', async () => { renderWithProviders(<Skills />, { initialEntries: ['/skills'] }); expect(screen.getByRole('heading', { name: '3rd Party Skills' })).toBeInTheDocument(); expect(screen.getByText('Gmail')).toBeInTheDocument(); const syncBtn = screen.getByTestId('skill-sync-button-gmail'); expect(syncBtn).toBeInTheDocument(); expect(syncBtn).toHaveAttribute('aria-label', 'Sync Gmail'); fireEvent.click(syncBtn); - expect(mocks.triggerSync).toHaveBeenCalledTimes(1); - expect(mocks.triggerSync).toHaveBeenCalledWith('gmail'); + await waitFor(() => { + expect(mocks.triggerSync).toHaveBeenCalledTimes(1); + }); + expect(mocks.triggerSync).toHaveBeenCalledWith('gmail'); });Note: You'll also need to add
waitForto the imports on line 5.🤖 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-gmail-sync.test.tsx` around lines 59 - 73, Mark the test case as async and import waitFor from testing-library; after firing the click on the element returned by screen.getByTestId('skill-sync-button-gmail') await waitFor(() => expect(mocks.triggerSync).toHaveBeenCalledTimes(1)) and await waitFor(() => expect(mocks.triggerSync).toHaveBeenCalledWith('gmail')) to handle the async sync handler; keep the existing DOM assertions but move the triggerSync assertions into the waitFor blocks so renderWithProviders, screen, fireEvent and mocks.triggerSync are used as before.src/openhuman/skills/quickjs_libs/bootstrap.js (1)
707-716: Keep the new QuickJS helpers aligned with repo JS conventions.The added paths are still introducing
var, function-expression, and string-concatenation style. Please useconst/let, arrow functions, and template literals for new code even in this legacy file.As per coding guidelines,
Use const by default, let if reassignment is needed, avoid varandPrefer arrow functions over function declarations.Also applies to: 876-876, 935-935, 1171-1171, 1233-1233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/quickjs_libs/bootstrap.js` around lines 707 - 716, Replace legacy var/function/string-concatenation patterns in the QuickJS helpers with modern JS conventions: change the globalThis.__resolveBackendBaseUrl function expression to a const arrow function named __resolveBackendBaseUrl, use const/let instead of var for the raw variable, and use a template literal where any string concatenation exists; apply the same edits to the other helper functions referenced (the helper definitions at the other noted spots) so all new helpers use const/let, arrow functions, and template literals consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 37-38: Clarify in the .env.example comment that
OPENHUMAN_DOTENV_PATH must be provided from the parent environment (exported by
the shell or service manager) because the value is read before any dotenv file
is loaded; mention that setting OPENHUMAN_DOTENV_PATH inside a .env file will
not work for selecting that .env file and reference the commands that read it
(openhuman serve / openhuman run) and the variable name OPENHUMAN_DOTENV_PATH so
maintainers know where to update the text.
In `@docs/memory-sync-functions.md`:
- Around line 13-59: The markdown code fence containing the decision-tree block
is unlabeled and triggers MD040; update the opening fence from ``` to include a
language token (use "text") so it reads ```text; this affects the block that
documents functions like kv_set(), put_doc_light(), put_doc(), ingest_doc(),
graph_upsert(), query_namespace(), recall_namespace(), kv_get(),
list_documents(), clear_namespace(), etc., and will satisfy markdownlint without
changing the content.
- Around line 197-202: Update the docs to clarify that
clear_skill_memory(skill_id, integration_id) performs targeted revocation using
integration_id filtering and does not create per-integration isolated
namespaces; explicitly state that the storage namespace is shared per skill
(e.g., "skill-{skill_id}") and integration_id is used only to filter or target
entries for deletion, not to change namespace isolation, and revise the table
rows for delete_document, kv_delete, clear_skill_memory(skill_id,
integration_id), and clear_namespace so the clear_skill_memory description
reflects targeted revocation/filtering semantics rather than implying
per-integration namespace isolation (also apply the same clarification to the
corresponding description later in the file).
In `@src/core/cli.rs`:
- Around line 72-80: The load_dotenv_for_server function currently treats
OPENHUMAN_DOTENV_PATH="" as set and swallows errors from dotenvy::from_path;
change the logic to treat an empty value as unset (use std::env::var_os or check
that the string is non-empty) and if a path is provided, handle Result from
dotenvy::from_path (log or return the error) instead of discarding it so
failures are visible and you can optionally fall back to dotenvy::dotenv() on
error; apply the same fix to the other occurrence that checks
OPENHUMAN_DOTENV_PATH.
In `@src/openhuman/skills/quickjs_libs/bootstrap.js`:
- Around line 754-763: Replace the current content-logging behavior in the
notify function so it does not persist user-visible text: update the notify
implementation (function name notify in the bootstrap shim) to avoid
concatenating or console.logging the title/body; either make it a no-op or emit
only a generic message like "notification requested" without including title or
body, ensuring the shim still exists to prevent "not a function" runtime errors.
In `@tests/skills_gmail_oauth_proxy_rpc_e2e.rs`:
- Around line 160-162: Remove printing of sensitive secrets and PII from the
test logs: replace direct eprintln! of test_jwt, credential_id, client_key_share
and any live Gmail profile fields with non-sensitive status messages (e.g.,
"JWT_TOKEN present", "CREDENTIAL_ID present", "CLIENT_KEY_SHARE present", or
"GMAIL_PROFILE loaded") in the test function that contains the eprintln! calls
(look for the eprintln! lines referencing test_jwt, credential_id,
client_key_share and the other Gmail profile dumps around the shown ranges);
redact or omit actual values and ensure only presence/boolean/status is logged.
- Around line 178-190: Save the current process-global state before mutating it:
capture original values of HOME, BACKEND_URL, JWT_TOKEN, OPENHUMAN_WORKSPACE,
and VITE_BACKEND_URL (remember whether each was present and its value) and
capture any existing global engine before calling
set_global_engine(engine.clone()). Replace the direct unsafe set_var/remove_var
and set_global_engine calls with a scoped guard that sets the env vars and
global engine, and in its Drop implementation restores each env var to its prior
value (or removes it if it was absent) and restores the previous global engine
(using the existing get/set global engine API or equivalent), ensuring
restoration runs even if the test panics or fails; apply the same pattern to the
other occurrence at lines 324-332.
- Around line 121-123: The E2E test spins up the server with HOME=<tmp> (so the
server uses the default .openhuman workspace) but the verification MemoryClient
polls <tmp>/workspace, causing missed skills_sync writes; fix by making the
client and server point to the same workspace: either set OPENHUMAN_WORKSPACE to
workspace_dir in the test environment before booting the server (ensuring the
server uses workspace_dir), or instantiate MemoryClient with openhuman_dir (the
server's actual workspace) instead of workspace_dir; apply the same change to
the other MemoryClient/workspace_dir usages (the other occurrences of
MemoryClient creation and workspace_dir setup).
---
Nitpick comments:
In `@app/src/components/skills/SkillDebugModal.tsx`:
- Around line 131-137: The modal close button in the SkillDebugModal component
is missing an explicit button type; update the close <button> element (the one
rendering the SVG close icon and using onClose) to include type="button" to
prevent it from acting as a form submitter and to match the other explicit
tab/tool buttons' behavior.
In `@app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx`:
- Around line 59-73: Mark the test case as async and import waitFor from
testing-library; after firing the click on the element returned by
screen.getByTestId('skill-sync-button-gmail') await waitFor(() =>
expect(mocks.triggerSync).toHaveBeenCalledTimes(1)) and await waitFor(() =>
expect(mocks.triggerSync).toHaveBeenCalledWith('gmail')) to handle the async
sync handler; keep the existing DOM assertions but move the triggerSync
assertions into the waitFor blocks so renderWithProviders, screen, fireEvent and
mocks.triggerSync are used as before.
In `@app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx`:
- Around line 160-167: The test "Sync control calls
skillManager.triggerSync(notion)" should be made async and use waitFor to ensure
the async handleSync in Skills.tsx completes before assertions: change the test
function to async, import waitFor from testing-library, click the button as
before, then wrap the two expect(...) assertions inside an await waitFor(...)
block (checking mocks.triggerSync called once and calledWith('notion')), so the
test remains robust if triggerSync moves past the first await.
In `@app/src/pages/Skills.tsx`:
- Around line 306-327: The Settings button JSX (the <button> with onClick that
calls onSetup) is missing an explicit type, so add type="button" to that button
element to match the Sync and Debug buttons and avoid it being treated as a
submit inside forms; update the button where onClick={e => {
e.stopPropagation(); onSetup(); }} is defined to include type="button" so
behavior remains unchanged but consistent across the component.
In `@src/openhuman/skills/quickjs_libs/bootstrap.js`:
- Around line 707-716: Replace legacy var/function/string-concatenation patterns
in the QuickJS helpers with modern JS conventions: change the
globalThis.__resolveBackendBaseUrl function expression to a const arrow function
named __resolveBackendBaseUrl, use const/let instead of var for the raw
variable, and use a template literal where any string concatenation exists;
apply the same edits to the other helper functions referenced (the helper
definitions at the other noted spots) so all new helpers use const/let, arrow
functions, and template literals consistently.
🪄 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: a388b9c9-b034-477b-aa56-89e4f529488e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.env.exampleCargo.tomlapp/src/components/skills/SkillDebugModal.tsxapp/src/pages/Skills.tsxapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsxdocs/memory-sync-functions.mdsrc/core/cli.rssrc/openhuman/skills/quickjs_libs/bootstrap.jstests/skills_gmail_oauth_proxy_rpc_e2e.rs
- Dotenv: treat empty OPENHUMAN_DOTENV_PATH as unset; propagate from_path errors - Document OPENHUMAN_DOTENV_PATH parent-env requirement in .env.example - Memory docs: MD040 fence language; clarify skill namespace vs integration id - QuickJS bootstrap: modern helpers, generic platform.notify log, template URLs - Skills UI: type=button on close/settings; async waitFor in sync tests - Gmail OAuth e2e: workspace env matches MemoryClient; env/engine drop guards; redact secrets from logs - Add replace_global_engine for test teardown Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/skills_gmail_oauth_proxy_rpc_e2e.rs (1)
243-255: Consider making staging reachability check more diagnostic.The staging check panics with a generic message on connection failure. For CI debugging, it might be helpful to also log when staging is unreachable without a clear error (e.g., timeout vs. connection refused).
🔧 Optional: More descriptive skip on staging unreachable
If staging is expected to sometimes be unavailable (e.g., VPN required), consider skipping gracefully instead of panicking:
match health { Ok(resp) => eprintln!( "[gmail-oauth-proxy-e2e] staging /settings -> {}", resp.status() ), - Err(err) => panic!("failed to reach staging backend {backend_url}: {err}"), + Err(err) => { + eprintln!("SKIPPED: staging backend {backend_url} unreachable: {err}"); + return; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills_gmail_oauth_proxy_rpc_e2e.rs` around lines 243 - 255, The staging reachability panic provides only a generic message; update the match on the health check (the reqwest::Client::new().get(.../settings) call and its Result bound to health) to print more diagnostics when Err(err) occurs by logging the full error (err) and classifying common causes using reqwest::Error helpers (e.g., err.is_timeout(), err.is_connect(), err.is_status()) so the message distinguishes timeout vs connection refused; if staging may be intermittently unreachable, replace the panic!("failed...") with a graceful skip path that eprintln!s the diagnostic and returns early from the test (or uses test skip semantics) instead of aborting.app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx (1)
77-82: Prefer arrow function per coding guidelines.The codebase guidelines specify: "Prefer arrow functions over function declarations".
Suggested refactor
-function expectToolCallArgs(toolName: string, args: Record<string, unknown>) { - expect(mocks.callCoreRpc).toHaveBeenCalledWith({ +const expectToolCallArgs = (toolName: string, args: Record<string, unknown>) => { + expect(mocks.callCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skills_call_tool', params: { skill_id: 'notion', tool_name: toolName, arguments: args }, }); -} +};🤖 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 77 - 82, Replace the named function declaration expectToolCallArgs with an arrow function assigned to a const to follow the codebase guideline preferring arrow functions; keep the same signature and body (asserting mocks.callCoreRpc was called with method 'openhuman.skills_call_tool' and params containing skill_id: 'notion', tool_name: toolName, arguments: args) and ensure any references to expectToolCallArgs remain valid (no other changes to call sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx`:
- Around line 77-82: Replace the named function declaration expectToolCallArgs
with an arrow function assigned to a const to follow the codebase guideline
preferring arrow functions; keep the same signature and body (asserting
mocks.callCoreRpc was called with method 'openhuman.skills_call_tool' and params
containing skill_id: 'notion', tool_name: toolName, arguments: args) and ensure
any references to expectToolCallArgs remain valid (no other changes to call
sites).
In `@tests/skills_gmail_oauth_proxy_rpc_e2e.rs`:
- Around line 243-255: The staging reachability panic provides only a generic
message; update the match on the health check (the
reqwest::Client::new().get(.../settings) call and its Result bound to health) to
print more diagnostics when Err(err) occurs by logging the full error (err) and
classifying common causes using reqwest::Error helpers (e.g., err.is_timeout(),
err.is_connect(), err.is_status()) so the message distinguishes timeout vs
connection refused; if staging may be intermittently unreachable, replace the
panic!("failed...") with a graceful skip path that eprintln!s the diagnostic and
returns early from the test (or uses test skip semantics) instead of aborting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c96ad88-53af-44fc-a3d1-89bfb9bb09fc
📒 Files selected for processing (11)
.env.exampleapp/src/components/skills/SkillDebugModal.tsxapp/src/pages/Skills.tsxapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsxdocs/memory-sync-functions.mdsrc/core/cli.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/qjs_engine.rssrc/openhuman/skills/quickjs_libs/bootstrap.jstests/skills_gmail_oauth_proxy_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- src/core/cli.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/pages/Skills.tsx
- app/src/pages/tests/Skills.third-party-gmail-sync.test.tsx
- docs/memory-sync-functions.md
- src/openhuman/skills/quickjs_libs/bootstrap.js
- app/src/components/skills/SkillDebugModal.tsx
This was referenced Apr 10, 2026
Merged
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 modules(Any feature related checklist can go in here)
Impact
Related
Summary by CodeRabbit
New Features
Accessibility & Testing
Documentation
Tests