Fix local reset flow and Accessibility permission handling#324
Fix local reset flow and Accessibility permission handling#324senamakel merged 8 commits intotinyhumansai:mainfrom
Conversation
…nd registry_ops - Removed unused imports in `skills_cli.rs` and `registry_ops.rs` to enhance code clarity. - Streamlined import statements for better organization and readability. - Minor adjustments in `engine.rs` to maintain consistency in import usage.
…rd components - Changed the title in MemoryWorkspace to "Memory (EverMind)" for clarity. - Updated text colors in SkillSetupWizard for better visibility and consistency. - Enhanced button styles and loading indicators for improved user experience.
…p data clearing process - Added a new utility function `resetOpenHumanDataAndRestartCore` to reset local OpenHuman data and restart the core process. - Updated `clearAllAppData` in `SettingsHome` to utilize the new reset function, improving the app's data clearing process. - Enhanced error handling during the data reset and session clearing operations to ensure robustness. - Introduced tests for the new reset functionality to validate its behavior and integration.
- Eliminated the screen recording permission check and associated UI elements from the AccessibilityPanel component. - Updated tests to ensure the screen recording option is no longer present in the rendered output, improving clarity and focus on relevant permissions.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes screen-recording from permission requests, adds a core RPC and Tauri command to reset local OpenHuman data and restart the core, updates Settings to invoke that reset before client purges, tweaks UI text/styling, and cleans up unused imports across backend modules. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser Client
participant TauriCmd as Tauri Command<br/>(tauriCommands.ts)
participant RPC as Core RPC<br/>(config_reset_local_data)
participant FileIO as File I/O<br/>(Directory Removal)
participant CoreRestart as Core Restart<br/>(restart_core_process)
Browser->>TauriCmd: resetOpenHumanDataAndRestartCore()
TauriCmd->>RPC: callCoreRpc({ method: 'openhuman.config_reset_local_data' })
RPC->>FileIO: Delete active_workspace.toml & remove dirs
FileIO-->>RPC: removed_paths[] (results)
RPC-->>TauriCmd: RpcOutcome with removed_paths
TauriCmd->>CoreRestart: invoke('restart_core_process')
CoreRestart-->>TauriCmd: restart result
TauriCmd-->>Browser: Promise resolved
Browser->>Browser: Proceed with client storage/cache purge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- Added Husky as a dev dependency in both package.json files to manage Git hooks. - Included "prepare" and "postinstall" scripts in the main package.json to ensure Husky is set up correctly. - Cleaned up the app's package.json by removing the "prepare" script, as it is now handled in the main package.json. - Enhanced logging in tauriCommands.ts for better debugging during the reset process.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-tauri/src/core_process.rs`:
- Around line 211-214: Formatting changes are needed: run rustfmt over the
workspace (cargo fmt --manifest-path ../Cargo.toml --all) to normalize the
formatting around the log::debug calls in core_process.rs (the restart/shutdown
debug statements where log::debug! is invoked, also the similar statement around
the other debug at the noted location), then commit the resulting formatted
changes so cargo fmt --check passes.
In `@app/src/components/settings/panels/AccessibilityPanel.tsx`:
- Line 156: The CI failure is due to formatting checks; run the repository
format tasks and commit the results so the className change in
AccessibilityPanel.tsx (the "mt-1 rounded-lg border ..." JSX attribute) matches
Prettier/format rules. Run the frontend formatting/check commands (e.g. yarn
workspace openhuman-app format or yarn workspace openhuman-app format:fix, then
yarn workspace openhuman-app format:check, plus run eslint and tsc --noEmit) and
commit the updated files; if you also changed Rust code run cargo fmt and cargo
check before committing. Ensure the JSX file AccessibilityPanel.tsx is saved
with the formatter's output and pushed.
🪄 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: 2561c83d-8ddf-47f2-8a6a-dda29382146d
📒 Files selected for processing (17)
app/src-tauri/src/core_process.rsapp/src-tauri/src/lib.rsapp/src/components/intelligence/MemoryWorkspace.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/panels/AccessibilityPanel.tsxapp/src/components/settings/panels/__tests__/AccessibilityPanel.test.tsxapp/src/components/skills/SkillSetupWizard.tsxapp/src/utils/__tests__/tauriCommands.test.tsapp/src/utils/tauriCommands.tssrc/core/skills_cli.rssrc/openhuman/config/ops.rssrc/openhuman/config/schemas.rssrc/openhuman/screen_intelligence/engine.rssrc/openhuman/screen_intelligence/ops.rssrc/openhuman/screen_intelligence/schemas.rssrc/openhuman/skills/registry_ops.rssrc/openhuman/subconscious/engine.rs
💤 Files with no reviewable changes (1)
- src/core/skills_cli.rs
| log::debug!( | ||
| "[core] restart: shutdown complete, checking port {}", | ||
| self.port | ||
| ); |
There was a problem hiding this comment.
Run cargo fmt to fix formatting issues.
The pipeline is failing because cargo fmt --check detected formatting differences. The changes themselves are correct, but please run cargo fmt --manifest-path ../Cargo.toml --all before merging.
Also applies to: 271-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/core_process.rs` around lines 211 - 214, Formatting changes
are needed: run rustfmt over the workspace (cargo fmt --manifest-path
../Cargo.toml --all) to normalize the formatting around the log::debug calls in
core_process.rs (the restart/shutdown debug statements where log::debug! is
invoked, also the similar statement around the other debug at the noted
location), then commit the resulting formatted changes so cargo fmt --check
passes.
| onClick={() => void dispatch(requestAccessibilityPermission('accessibility'))} | ||
| disabled={isRequestingPermissions || isRestartingCore} | ||
| className="rounded-lg border border-primary-500/60 bg-primary-50 px-3 py-2 text-sm text-primary-600 disabled:opacity-50"> | ||
| className="mt-1 rounded-lg border border-primary-500/60 bg-primary-50 px-3 py-2 text-sm text-primary-600 disabled:opacity-50"> |
There was a problem hiding this comment.
CI formatting gate is failing and blocks merge.
The current pipeline reports yarn workspace openhuman-app format:check failing. Please run and commit formatter output before merge (this check includes Prettier and cargo fmt --check).
As per coding guidelines: “Run Prettier, ESLint, and tsc --noEmit before merging changes to app/ code” and “Run cargo fmt and cargo check before merging changes to Rust code.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/AccessibilityPanel.tsx` at line 156, The
CI failure is due to formatting checks; run the repository format tasks and
commit the results so the className change in AccessibilityPanel.tsx (the "mt-1
rounded-lg border ..." JSX attribute) matches Prettier/format rules. Run the
frontend formatting/check commands (e.g. yarn workspace openhuman-app format or
yarn workspace openhuman-app format:fix, then yarn workspace openhuman-app
format:check, plus run eslint and tsc --noEmit) and commit the updated files; if
you also changed Rust code run cargo fmt and cargo check before committing.
Ensure the JSX file AccessibilityPanel.tsx is saved with the formatter's output
and pushed.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/utils/tauriCommands.ts (1)
203-217: Implementation follows established patterns; note partial-reset edge case.The function correctly follows the file's conventions: early
isTauri()guard, debug logging, and letting errors propagate to the caller.One consideration: per the Rust handler in
src/openhuman/config/ops.rs, the reset performs sequentialremove_file/remove_dir_alloperations without transactional rollback. If an error occurs mid-way (e.g., permission denied on the second directory),restartCoreProcess()will not execute, leaving the system in a partially-reset state. The caller (SettingsHome.clearAllAppData) handles this by re-throwing and aborting the client purge, which is reasonable—but users may need to re-run the reset manually to complete cleanup.This is acceptable for a destructive logout flow, but consider documenting this behavior in the JSDoc.
📝 Suggested JSDoc enhancement
+/** + * Delete local OpenHuman config/workspace directories and restart the core process. + * + * This is a destructive operation intended for "clear all app data" flows. + * The backend reset is NOT atomic: if a filesystem error occurs mid-way, + * the core will NOT restart and partial state may remain. Callers should + * re-invoke on failure or instruct the user to retry. + */ export async function resetOpenHumanDataAndRestartCore(): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/tauriCommands.ts` around lines 203 - 217, Add a JSDoc block to resetOpenHumanDataAndRestartCore explaining that the underlying Rust handler performs sequential removals without rollback, so a mid-way error can leave a partially-reset state and will prevent restartCoreProcess() from running; note that errors are propagated to the caller (SettingsHome.clearAllAppData) and that callers may need to re-run the reset manually to complete cleanup. Include the function name resetOpenHumanDataAndRestartCore, mention the Rust handler behavior (sequential remove_file/remove_dir_all) and the restartCoreProcess() restart dependency, and ensure the JSDoc briefly describes expected caller behavior on failure.package.json (1)
20-31: Remove redundant Husky installation hook and consolidate to root only.Running Husky in both
"prepare"and"postinstall"at root is redundant and unnecessary. Husky v9 official guidance recommends using only"prepare": "husky"for development setup. Additionally, Husky is currently declared in both root andapp/package.json, which can cause conflicts in monorepos with centralized Git hooks. Remove"postinstall"and declare Husky only at the monorepo root.Changes needed
--- a/package.json +++ b/package.json @@ - "prepare": "husky", - "postinstall": "husky", + "prepare": "husky",--- a/app/package.json +++ b/app/package.json @@ - "husky": "^9.1.7",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 20 - 31, Remove the redundant "postinstall": "husky" script from the package.json scripts and keep only "prepare": "husky"; also ensure the "husky" devDependency is declared only at the monorepo root (remove the duplicate declaration from app/package.json) so Git hooks are centralized and Husky runs per official v9 guidance.
🤖 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/utils/tauriCommands.ts`:
- Around line 203-217: Add a JSDoc block to resetOpenHumanDataAndRestartCore
explaining that the underlying Rust handler performs sequential removals without
rollback, so a mid-way error can leave a partially-reset state and will prevent
restartCoreProcess() from running; note that errors are propagated to the caller
(SettingsHome.clearAllAppData) and that callers may need to re-run the reset
manually to complete cleanup. Include the function name
resetOpenHumanDataAndRestartCore, mention the Rust handler behavior (sequential
remove_file/remove_dir_all) and the restartCoreProcess() restart dependency, and
ensure the JSDoc briefly describes expected caller behavior on failure.
In `@package.json`:
- Around line 20-31: Remove the redundant "postinstall": "husky" script from the
package.json scripts and keep only "prepare": "husky"; also ensure the "husky"
devDependency is declared only at the monorepo root (remove the duplicate
declaration from app/package.json) so Git hooks are centralized and Husky runs
per official v9 guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85c74c3e-bb5d-456a-b49f-203298210ba4
📒 Files selected for processing (3)
app/package.jsonapp/src/utils/tauriCommands.tspackage.json
- Changed the variable name from `data` to `value` in the test assertions to enhance clarity and better reflect its purpose. - Ensured that the assertions remain functional and maintain the integrity of the test logic.
- Introduced a new function `select_skill_id` to improve skill selection based on environment variables and preferred skills. - Updated the skill ID retrieval process to prioritize user-defined skills while maintaining a fallback to the default skill. - Enhanced documentation to clarify the usage of environment variables for skill testing.
1 participant
Summary
config_reset_local_dataflow and wire destructive logout to clear local OpenHuman data before restarting the sidecarProblem
mainand need to land together with this branchSolution
~/.openhumandirectory when presentrequest_permissions()core path to request only Accessibility and Input Monitoring; explicit per-permission screen recording requests remain available for Screen IntelligenceSubmission Checklist
app/) and/orcargo test(core) for logic you add or changecargo checkcoverage was used instead//////!(Rust), JSDoc or brief file/module headers (TS) on public APIs and non-obvious modulesImpact
Related
Summary by CodeRabbit
New Features
Style
Bug Fixes
Tests / Chores