Scope CEF data to OpenHuman user profiles#917
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds per-user CEF profile management: new backend module for resolving/overriding cache paths, queuing and draining pending per-user cache purges across launches, Tauri commands to schedule purges and restart the app, frontend wiring to queue purges on logout/identity changes, and a tauri-cef submodule update. Changes
Sequence DiagramssequenceDiagram
actor User
participant Frontend as SettingsHome
participant TauriCmds as Tauri Commands
participant Backend as cef_profile
participant FS as TOML/FS
participant AppRuntime as CEF/App Runtime
User->>Frontend: Click "Clear App Data"
Frontend->>Frontend: Read user_id from core state
Frontend->>TauriCmds: scheduleCefProfilePurge(user_id)
TauriCmds->>Backend: schedule_cef_profile_purge(user_id)
Backend->>FS: Load/update pending_cef_purge.toml (dedupe)
TauriCmds-->>Frontend: Return queue id
Frontend->>Frontend: Clear session, redux, browser storage
Frontend->>TauriCmds: restartApp()
TauriCmds->>AppRuntime: restart_app
AppRuntime->>Backend: prepare_process_cache_path() on startup
Backend->>FS: Drain pending_cef_purge.toml -> delete queued dirs (safe canonical checks)
Backend->>FS: Read active_user.toml (or default) and ensure cache dir exists
Backend->>AppRuntime: Set OPENHUMAN_CEF_CACHE_PATH
AppRuntime-->>User: App restarts with configured per-user CEF cache
sequenceDiagram
actor User
participant CoreState as CoreStateProvider
participant API as storeSession
participant TauriCmds as Tauri Commands
participant CEFProfile as cef_profile
participant App as App Runtime
User->>CoreState: Log in / switch user
CoreState->>CoreState: Snapshot prev identity
CoreState->>API: storeSession(token)
API-->>CoreState: Session stored
CoreState->>CoreState: refresh + refreshTeams
CoreState->>CoreState: Snapshot next identity
alt Identity changed
CoreState->>TauriCmds: restartApp()
TauriCmds->>CEFProfile: prepare_process_cache_path()
CEFProfile->>App: Set OPENHUMAN_CEF_CACHE_PATH
App-->>User: Restart into new user's profile
else No change
CoreState-->>User: Continue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🤖 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/cef_profile.rs`:
- Around line 60-66: user_id is used directly in path joins which allows
absolute paths or traversal like ".." to escape the intended profile root; add
validation/sanitization before using it in user_openhuman_dir and
cache_dir_for_user: implement a helper (e.g., sanitize_user_id or
validate_user_id) that rejects any input containing path separators, components
like "." or "..", or leading separators (and optionally enforces an allowed
charset), return a Result or normalized String, and call that helper from
user_openhuman_dir and cache_dir_for_user so only verified safe IDs are joined
into the filesystem path.
- Around line 68-70: The pending CEF purge marker is currently placed inside
default_openhuman_dir (via pending_purge_marker_path and
PENDING_PURGE_STATE_FILE) which gets deleted by reset_local_data_for_paths;
change pending_purge_marker_path to return a location outside the directory
being wiped (e.g., a sibling or parent location such as
default_openhuman_dir.parent().join(PENDING_PURGE_STATE_FILE) or another
app-specific config dir) so reads/writes continue to use the same symbol and the
file survives reset_local_data_for_paths; update all callers that read/write the
marker to use pending_purge_marker_path and run/update any tests that rely on
the marker location.
- Around line 164-197: The code currently removes the purge marker
unconditionally after iterating state.paths so transient std::fs::remove_dir_all
errors become permanent; change the logic in the loop that iterates state.paths
to record whether any removal failed (e.g., a boolean like had_failures) and
only call std::fs::remove_file(&marker_path) when no failures occurred; on any
Err from remove_dir_all keep the path in state.paths (or leave state unchanged)
so the marker remains and the failed entries will be retried on next run, and
log clearly when skipping marker removal due to had_failures; update the code
around remove_dir_all, state.paths and marker_path to implement this conditional
removal.
In `@app/src-tauri/src/lib.rs`:
- Around line 780-786: When cef_profile::prepare_process_cache_path() returns
Err, do not clear cef_profile::CEF_CACHE_PATH_ENV or let the runtime fall back
to the shared cache; instead fail closed by propagating or terminating startup.
Replace the current Err arm (which logs a warn and calls std::env::remove_var)
with code that logs an error (including the error details) and then aborts
startup (e.g., propagate the Err from the surrounding function or call
std::process::exit(1)) so the application does not continue with the
global/shared CEF cache.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 393-398: The restart log is leaking raw user identifiers
(previousIdentity and nextIdentity); modify the console.debug call in
CoreStateProvider (the block that checks nextIdentity !== previousIdentity and
calls restartApp) to avoid emitting raw PII by either redacting or hashing those
values before logging (e.g., replace with a stable hash or masked string like
****xxxx), and include only non-sensitive context in the log message so the
restart event remains visible without exposing full IDs.
In `@app/src/utils/tauriCommands/core.ts`:
- Around line 82-85: The debug is logging raw PII (userId) in core.ts; change
the console.debug in the scheduleCefProfilePurge call to avoid printing the full
userId—either remove the userId field entirely or replace it with a
non-sensitive indicator (e.g., "userId: redacted" or a truncated/masked form)
and keep the invoke('schedule_cef_profile_purge', { userId: userId ?? null })
behavior unchanged so the backend still receives the id while the renderer log
does not contain full PII.
🪄 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: 45e9109f-0844-4f8d-96d2-1f821fe62df3
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/src-tauri/Cargo.tomlapp/src-tauri/src/cef_preflight.rsapp/src-tauri/src/cef_profile.rsapp/src-tauri/src/lib.rsapp/src-tauri/vendor/tauri-cefapp/src/components/settings/SettingsHome.tsxapp/src/providers/CoreStateProvider.tsxapp/src/utils/tauriCommands/core.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src-tauri/src/cef_profile.rs (1)
211-215:⚠️ Potential issue | 🟠 MajorRedact
user_idfrom the CEF profile logs.These logs currently write the full logical user identifier, and this module explicitly accepts email-shaped IDs. Please log a boolean, a short hash, or omit the identifier entirely.
As per coding guidelines, "Never log secrets, raw JWTs, API keys, credentials, or full PII; redact or omit sensitive fields."
Also applies to: 239-243
🧹 Nitpick comments (1)
app/src/utils/tauriCommands/core.ts (1)
65-85: Add co-located tests for the new Tauri command wrappers.These branches now control app restart and destructive purge scheduling, but there’s no coverage for the non-Tauri short-circuit or the
userId ?? nullpayload shape. A smallcore.test.tswould make this safer to evolve.As per coding guidelines, "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top" and "Co-locate unit tests as
*.test.tsor*.test.tsxunderapp/src/**; use Vitest withapp/test/vitest.config.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/tauriCommands/core.ts` around lines 65 - 85, Add a co-located Vitest file (core.test.ts) to cover the Tauri command wrappers restartApp and scheduleCefProfilePurge: mock isTauri() and invoke() to test both short-circuit behavior when isTauri() is false (restartApp should not call invoke and scheduleCefProfilePurge should return null) and the Tauri path when isTauri() is true (restartApp should call invoke('restart_app') and scheduleCefProfilePurge should call invoke('schedule_cef_profile_purge', { userId: null }) for undefined, { userId: null } for null, and { userId: '<id>' } for a string); also assert scheduleCefProfilePurge returns the invoke result string and verify console debug calls if desired. Ensure tests live under app/src/** and use the project's Vitest config.
🤖 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/cef_profile.rs`:
- Around line 257-280: The purge loop is using raw_path ->
PathBuf::from(raw_path) and directly calling std::fs::remove_dir_all, which
risks deleting arbitrary directories from a corrupted TOML marker; before
calling remove_dir_all in the loop that iterates state.paths (and in functions
handling queued purge), canonicalize the target (std::fs::canonicalize) and
validate it is a descendant of the known app root (e.g., the computed
default_openhuman_dir/users/*/cef pattern) — verify the canonical path
starts_with the canonicalized default_openhuman_dir/users and optionally
ends_with "cef" or matches a persisted user ID-derived path; if validation
fails, log a warning and push the raw_path into remaining instead of deleting.
Ensure the check is applied where remove_dir_all is invoked (the match branch
around std::fs::remove_dir_all(&target)) and consider switching to deriving
purge paths from persisted user IDs rather than raw TOML strings.
- Around line 307-342: Add unit tests in the existing tests module that exercise
the purge-queue sibling-marker migration and the retry-on-failure drain path:
create temp dirs and seed the PURGE_QUEUE_FILE and SIBLING_MARKER_FILE with
representative contents, call migrate_sibling_marker(...) to verify the sibling
marker is migrated/removed and queue entries are adjusted, and call
drain_purge_queue(...) (or drain_purge_queue_with_retry if present) to assert
successful removal of queued items and that failed removals are retried
(simulate failure by making a file/entry non-removable or using the existing
retry hook), using helper functions like enqueue_purge(...) and
read_purge_queue(...) to observe state changes; place these tests alongside the
existing tests and use the same tempfile/tempdir patterns as read_active_user_id
and cache_dir_for_user.
---
Nitpick comments:
In `@app/src/utils/tauriCommands/core.ts`:
- Around line 65-85: Add a co-located Vitest file (core.test.ts) to cover the
Tauri command wrappers restartApp and scheduleCefProfilePurge: mock isTauri()
and invoke() to test both short-circuit behavior when isTauri() is false
(restartApp should not call invoke and scheduleCefProfilePurge should return
null) and the Tauri path when isTauri() is true (restartApp should call
invoke('restart_app') and scheduleCefProfilePurge should call
invoke('schedule_cef_profile_purge', { userId: null }) for undefined, { userId:
null } for null, and { userId: '<id>' } for a string); also assert
scheduleCefProfilePurge returns the invoke result string and verify console
debug calls if desired. Ensure tests live under app/src/** and use the project's
Vitest config.
🪄 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: 4c60d8fa-66e7-4671-a357-e6752f9f0650
📒 Files selected for processing (4)
app/src-tauri/src/cef_profile.rsapp/src-tauri/src/lib.rsapp/src/providers/CoreStateProvider.tsxapp/src/utils/tauriCommands/core.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/providers/CoreStateProvider.tsx
- Introduced unit tests for `restartApp` and `scheduleCefProfilePurge` functions in `core.test.ts`, ensuring correct behavior based on Tauri environment. - Implemented a safety check in `cef_profile.rs` to validate purge paths, preventing unauthorized directory deletions and enhancing security. - Added tests to verify the correct handling of trusted and untrusted paths during the purge process.
- Simplified the construction of file paths in tests by removing unnecessary line breaks. - Consolidated the initialization of `PendingCefPurgeState` for improved readability.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/utils/tauriCommands/core.test.ts (1)
24-33: Prefer automatic spy cleanup to avoid leakage on failed assertions.If an assertion fails before
debug.mockRestore(), the spy can leak into subsequent tests. Use centralized cleanup (afterEach(() => vi.restoreAllMocks())) to make this robust and remove duplication.♻️ Suggested cleanup
-import { beforeEach, describe, expect, type Mock, test, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, type Mock, test, vi } from 'vitest'; describe('tauriCommands/core', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + const mockIsTauri = isTauri as Mock; const mockInvoke = invoke as Mock; @@ - debug.mockRestore(); @@ - debug.mockRestore();Also applies to: 44-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/tauriCommands/core.test.ts` around lines 24 - 33, The test manually restores the console.debug spy (debug.mockRestore()) which can leak if an assertion fails; instead add centralized cleanup by calling vi.restoreAllMocks() in an afterEach block (e.g., add afterEach(() => vi.restoreAllMocks()) at the test-suite top level), and remove the per-test debug.mockRestore() calls in the tests that spy on console.debug (the tests invoking restartApp and assertions against mockInvoke and debug). Ensure the tests still create the spy with vi.spyOn(console, 'debug') and assert expect(debug).toHaveBeenCalledWith(...), but rely on afterEach to clean up.
🤖 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/core.test.ts`:
- Around line 24-33: The test manually restores the console.debug spy
(debug.mockRestore()) which can leak if an assertion fails; instead add
centralized cleanup by calling vi.restoreAllMocks() in an afterEach block (e.g.,
add afterEach(() => vi.restoreAllMocks()) at the test-suite top level), and
remove the per-test debug.mockRestore() calls in the tests that spy on
console.debug (the tests invoking restartApp and assertions against mockInvoke
and debug). Ensure the tests still create the spy with vi.spyOn(console,
'debug') and assert expect(debug).toHaveBeenCalledWith(...), but rely on
afterEach to clean up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5852174-9030-4795-ba08-224a98b55df7
📒 Files selected for processing (2)
app/src-tauri/src/cef_profile.rsapp/src/utils/tauriCommands/core.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src-tauri/src/cef_profile.rs
… in `cef_profile.rs` - Eliminated a test case that was specific to Unix systems, which was no longer necessary for the current test suite. - This cleanup improves the maintainability of the test code.
This was referenced May 1, 2026
This was referenced May 11, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
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
.openhuman/users/<id>/cefwith alocalpre-login fallbacktauri-cefruntime to honor anOPENHUMAN_CEF_CACHE_PATHoverride before CEF bootsProblem
Solution
.openhuman/users/<id>/cefand configures that path before the runtime starts.tauri-runtime-cefsubmodule to respectOPENHUMAN_CEF_CACHE_PATH; see Allow overriding the CEF cache path tauri-cef#9.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 modulesImpact
Related
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests