refactor(core): move app state ownership into the Rust core#320
Merged
senamakel merged 13 commits intotinyhumansai:mainfrom Apr 4, 2026
Merged
refactor(core): move app state ownership into the Rust core#320senamakel merged 13 commits intotinyhumansai:mainfrom
senamakel merged 13 commits intotinyhumansai:mainfrom
Conversation
…gement - Replaced UserProvider with CoreStateProvider in App component to centralize state management. - Updated various components to utilize the new CoreStateProvider for accessing session tokens and user data. - Refactored hooks and components to eliminate direct Redux store dependencies, enhancing modularity and maintainability. - Introduced a new core state management structure to improve the handling of user authentication and onboarding states. This refactor aims to simplify state access across the application and improve overall code clarity.
Contributor
|
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 0 minutes and 34 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 (11)
📝 WalkthroughWalkthroughThis PR migrates frontend auth/user/team/local-state flows from Redux to a new CoreState system: adds a CoreStateProvider that polls a core RPC, new core RPC endpoints and Rust controllers for app_state and teams, and updates many components/hooks/services/tests to read/write the core snapshot instead of Redux. Changes
Sequence Diagram(s)sequenceDiagram
participant ReactApp as React App
participant CoreProv as CoreStateProvider
participant RPC as Tauri RPC Server
participant Backend as Backend HTTP
ReactApp->>CoreProv: mount / useCoreState()
CoreProv->>CoreProv: init from getCoreStateSnapshot()
loop poll every 3s
CoreProv->>RPC: callCoreRpc("openhuman.app_state_snapshot")
RPC->>Backend: (optional) GET /telegram/me with session token
Backend-->>RPC: user data / empty
RPC-->>CoreProv: AppStateSnapshot (token, user, teams, localState)
CoreProv->>CoreProv: patch snapshot, set isBootstrapping/isReady
CoreProv-->>ReactApp: context update (snapshot)
end
ReactApp->>CoreProv: update action (e.g., setWallet / setOnboarding)
CoreProv->>RPC: callCoreRpc("openhuman.app_state_update_local_state", ...)
RPC-->>CoreProv: updated stored state
CoreProv->>CoreProv: merge patch into snapshot, notify
ReactApp-->>ReactApp: re-render with new snapshot
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 |
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/team/ops.rs (1)
35-38:⚠️ Potential issue | 🟠 MajorNormalize
api_urlbefore joining request paths.If
effective_api_url()can return something likehttps://host/apiwithout a trailing slash,base.join("teams")resolves to/teamsand silently drops/api. That breaks every team RPC behind a path-prefixed deployment, and the same helper pattern should be fixed insrc/openhuman/app_state/ops.rstoo.#!/bin/bash set -euo pipefail echo "effective_api_url implementation:" rg -n -C4 '\beffective_api_url\b' src echo echo "api_url defaults/examples:" rg -n -C2 'api_url' src app | sed -n '1,120p'Expected result: if any normalized/default API URL ends in a path segment without a trailing
/, the currentUrl::join(...)calls will strip that segment.Also applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/team/ops.rs` around lines 35 - 38, The resolve_base function uses effective_api_url and then calls Url::parse(...). Ensure the base URL is normalized to always end with a trailing slash before any subsequent base.join(...) calls so path segments are not dropped (e.g., if effective_api_url returns "https://host/api" convert it to "https://host/api/"); update resolve_base to append a '/' when the parsed Url has a non-empty path and does not already end with '/', and apply the same normalization wherever you call base.join (including the analogous helper in app_state ops) so join preserves the path prefix.
🧹 Nitpick comments (5)
app/src/utils/__tests__/desktopDeepLinkListener.test.ts (1)
1-5: Test coverage removed; follow-up needed.The deep link listener tests have been skipped rather than updated for the new core-state storage. Per the PR summary, this is noted as follow-up work.
Consider opening a tracking issue to ensure these tests are rewritten to cover the new
getCoreStateSnapshot()andstoreSession()paths.Do you want me to open an issue to track restoring test coverage for the deep link listener with the new core-session storage approach?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/__tests__/desktopDeepLinkListener.test.ts` around lines 1 - 5, The test file desktopDeepLinkListener.test.ts currently skips the legacy Redux wiring; open a tracking issue and update the tests to exercise the new core-session APIs by removing describe.skip and rewriting the test cases to call the desktopDeepLinkListener code paths that now rely on getCoreStateSnapshot() and storeSession() (mocking/stubbing those functions to simulate session state and asserting the listener behavior), and add the issue ID to the test TODO so CI fails if coverage is not restored.app/src/hooks/useUser.ts (1)
12-12: Hardcodederror: nullremoves error visibility.The previous Redux-based implementation likely surfaced errors from the user slice. Hardcoding
error: nullmeans consumers ofuseUser()will never see errors. If this is intentional (errors handled elsewhere), consider documenting it; otherwise, consider exposing error state from the core snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/hooks/useUser.ts` at line 12, The hook useUser currently hardcodes "error: null", which hides any errors from the underlying user state; change the implementation to read and return the error from the core snapshot (e.g., use snapshot.error or snapshot?.error) instead of null so consumers receive real error state, or if hiding errors is intentional add a comment/docs indicating that behavior; update the returned object from useUser (the function named useUser and any variables like snapshot or coreSnapshot) to propagate snapshot error (e.g., error: snapshot?.error ?? null) rather than the fixed null.app/src/pages/onboarding/Onboarding.tsx (1)
31-31: UnuseduseUser()hook call.
useUser()is invoked but its return value is not captured. If this is for triggering side effects (e.g., ensuring user data is loaded), consider adding a comment. Otherwise, this line can be removed.💡 Options
If needed for side effects:
useUser(); // Ensure user context is initializedIf not needed:
- useUser();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/onboarding/Onboarding.tsx` at line 31, The call to useUser() in Onboarding.tsx is unused; either remove the call if it's not needed, or explicitly use its return value (e.g., const user = useUser() or destructure needed fields) if the component depends on the user, or if the call is intentionally only for side effects keep the call but add a comment like “// ensure user context is initialized” so intent is clear; update the useUser() usage accordingly in the Onboarding component.app/src/components/settings/panels/TeamManagementPanel.tsx (1)
25-29: Consider avoiding fetch on every mount when teams array is legitimately empty.The condition
teams.length === 0triggers a refresh on mount, but this will also fire if the user genuinely has no teams (edge case). This is a minor inefficiency rather than a bug, sincerefreshTeams()is idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/TeamManagementPanel.tsx` around lines 25 - 29, The effect in TeamManagementPanel currently calls refreshTeams whenever teams.length === 0 on mount, causing unnecessary fetches for users who truly have no teams; update the component to fetch only once by tracking whether we've already attempted the initial load (e.g., add a ref or state like teamsFetchedRef/teamsInitialized) and change the useEffect to run refreshTeams only if teams.length === 0 AND teamsFetchedRef.current is false, then set teamsFetchedRef.current = true after calling refreshTeams; also ensure you reset/mark fetched true if teams become non-empty so subsequent mounts won't re-fetch unnecessarily.app/src/services/coreStateApi.ts (1)
30-74: Align this new service with the repo’s TS export conventions.The RPC helpers here are exported as function declarations, and
updateCoreLocalStateintroduces another inline object-shape type. Converting the exports toconstarrow functions and extracting the request payload into a namedinterfacewill keep this new shared service surface consistent and easier to reuse. As per coding guidelines, "Prefer arrow functions over function declarations" and "Preferinterfacefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/coreStateApi.ts` around lines 30 - 74, Convert the exported function declarations to exported const arrow functions and extract the inline param object type used by updateCoreLocalState into a named interface; e.g., create interface UpdateCoreLocalStateParams { encryptionKey?: string | null; primaryWalletAddress?: string | null; onboardingTasks?: OnboardingTasks | null } and interface OnboardingTasks { accessibilityPermissionGranted: boolean; localModelConsentGiven: boolean; localModelDownloadStarted: boolean; enabledTools: string[]; connectedSources: string[]; updatedAtMs?: number } then change export function fetchCoreAppSnapshot, updateCoreLocalState, listTeams, getTeamMembers, and getTeamInvites to exported consts using arrow syntax while keeping their existing return types and callCoreRpc usage.
🤖 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/App.tsx`:
- Around line 28-29: CoreStateProvider no longer triggers memory client
initialization; call the exported syncMemoryClientToken() during
CoreStateProvider's mount/initialization effect (or immediately after successful
session refresh in the same effect where storeSessionToken() is invoked) so the
memory RPC client (openhuman.memory_init) is initialized before any memory
operations; update the effect in CoreStateProvider to import and await
syncMemoryClientToken(), handle/ log errors, and ensure storeSessionToken()
still calls storeSession() while syncMemoryClientToken() runs right after or as
part of that flow.
In `@app/src/components/OnboardingOverlay.tsx`:
- Around line 43-46: Replace the synchronous state sync in the OnboardingOverlay
effect by removing the setOnboardingCompleted call and deriving the value
directly from snapshot.onboardingCompleted; specifically, delete the
setOnboardingCompleted usage inside the useEffect and instead read
onboardingCompleted from snapshot.onboardingCompleted (or compute a derived
value) wherever local onboardingCompleted state was used, eliminating the
redundant local state and the dependency on setOnboardingCompleted while keeping
the effect only for side-effects that truly require them.
In `@app/src/components/settings/panels/PrivacyPanel.tsx`:
- Around line 11-15: The handler handleToggleAnalytics flips runtime analytics
before the awaited persistent update, so if setAnalyticsEnabled(newValue) fails
the runtime state (syncAnalyticsConsent) will be wrong; change the order to
await setAnalyticsEnabled(newValue) first and only call
syncAnalyticsConsent(newValue) after the persistent write succeeds, and wrap the
await in try/catch to avoid leaving UI/runtime inconsistent (use
analyticsEnabled, setAnalyticsEnabled, and syncAnalyticsConsent references to
locate the code).
In `@app/src/components/settings/panels/TeamPanel.tsx`:
- Line 13: The hardcoded const isLoading = false in TeamPanel disables the
spinner; replace it by either reading a loading flag from useCoreState() (if
available) or implement local state (e.g., const [isLoading, setIsLoading] =
useState(false)) inside the TeamPanel component and update it around
refreshTeams() (set true before calling refreshTeams(), set false after
success/failure) so the existing spinner render (used in the TeamPanel render)
can activate during the fetch.
In `@app/src/hooks/useConsciousItems.ts`:
- Around line 144-145: The auth token is being captured once at render via
getCoreStateSnapshot() inside useConsciousItems, causing triggerAnalysis to use
a stale token; fix by making the token reactive either by replacing that
snapshot read with the useCoreState() hook (import and call useCoreState()
inside useConsciousItems and pull sessionToken) so authToken updates with state,
or if you must keep snapshot usage, defer reading the latest token inside the
triggerAnalysis callback (call getCoreStateSnapshot().snapshot.sessionToken at
invocation time) so the current sessionToken is used; update references to
authToken in triggerAnalysis accordingly (functions: useConsciousItems,
triggerAnalysis, getCoreStateSnapshot, useCoreState).
In `@app/src/hooks/useIntelligenceSocket.ts`:
- Around line 26-42: The effect must handle logout and token rotation: update
the useEffect that watches connect/isConnected/token so that when token becomes
falsy it calls disconnect (socketService.disconnect via the disconnect callback)
to tear down any authenticated socket, and when token changes while isConnected
is true it forces a reconnect (call socketService.disconnect then
socketService.connect with the new token or invoke the disconnect then connect
callbacks). Use the existing connect and disconnect callbacks (and
isConnected/token) so the effect explicitly disconnects on !token and performs a
disconnect+connect sequence when token changes to a different truthy value while
already connected.
In `@app/src/hooks/useUser.ts`:
- Line 11: The isLoading assignment in useUser uses a redundant ternary that
always yields false; replace the expression `isLoading:
!snapshot.auth.isAuthenticated && !snapshot.sessionToken ? false : false` with a
single deterministic value or correct derived logic—either set `isLoading:
false` if loading is handled elsewhere, or compute `isLoading` from
`snapshot.auth.isAuthenticated` and/or `snapshot.sessionToken` (e.g.,
`!snapshot.auth.isAuthenticated || !snapshot.sessionToken`) to reflect intended
behavior in the useUser hook.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 77-91: The snapshot/teams commits can be overwritten by stale
async polls and don't clear identity-scoped caches when auth changes; update
refresh() and refreshTeams() to use a per-request token (e.g., capture a local
requestId or AbortController at start and compare to a shared currentRequestId)
so commits only apply if the token matches, and when applying a new snapshot in
commitState (inside refresh) detect auth identity changes (compare
snapshot.user?.id with previous snapshot.user?.id or handle null) and clear
identity-scoped fields teams, teamMembersById, and teamInvitesById when the user
became unauthenticated or switched users; likewise gate refreshTeams() commits
with the same token so stale listTeams responses are ignored. Ensure commitState
calls (in refresh and refreshTeams) only mutate the intended keys and perform
the cache clears when identity changes.
In `@app/src/utils/__tests__/authFlow.e2e.test.tsx`:
- Around line 3-4: The skipped test block describe.skip('Auth flow e2e (legacy
Redux bootstrap)') removes coverage for the auth bootstrap; port its assertions
into the core-state bootstrap E2E tests by recreating the same user interaction
assertions in the core-state bootstrap suite (rather than re-enabling the legacy
test). Locate the original assertions inside this file and replicate their
expectations in the core-state bootstrap test file/class that uses core-state
polling (use the same user interaction flows and helpers from the UI layer),
remove the skipped placeholder, and ensure the new tests run as true E2E UI
tests (trigger UI actions in src/ tests and assert resulting auth state via
core-state polling helpers like the core-state bootstrap test utilities).
In `@src/openhuman/app_state/ops.rs`:
- Around line 91-95: The current save_stored_app_state/read-modify-write in
update_local_state is vulnerable to races and partial writes; wrap all access to
the app-state file behind a process-wide mutex/shared store and perform atomic
persistence by writing to a temporary file and renaming into place.
Specifically, modify save_stored_app_state (and the code paths used by
update_local_state and the other affected block) to acquire a global mutex
before reading or writing the file obtained from app_state_path, serialize
StoredAppState to a string, write to a temp file in the same directory, fsync
the temp file and directory if possible, then rename the temp file to the target
path to ensure atomic replace; hold the mutex across the read-modify-write
sequence so two concurrent RPCs cannot clobber each other. Ensure errors from
serialization, temp-file write, fsync, and rename are propagated as before.
- Around line 79-89: The load_stored_app_state function should not return an
error on a malformed or unreadable app-state.json; instead, catch
fs::read_to_string and serde_json::from_str errors, log a warning identifying
the path and error, quarantine the bad file (e.g., rename to
app-state.json.corrupted.<timestamp> via app_state_path result) or remove it,
and then return Ok(StoredAppState::default()); apply the same resilient behavior
to the other parsing site mentioned (the similar serde_json::from_str call
around line 157) so snapshot bootstrap falls back to defaults rather than
failing.
In `@src/openhuman/app_state/schemas.rs`:
- Around line 54-67: The file src/openhuman/app_state/schemas.rs has formatting
issues in the "update_local_state" ControllerSchema arm and in the
handle_snapshot() section; run rustfmt (cargo fmt --all) to reformat the file
and ensure it passes cargo fmt --all -- --check, then re-run cargo check;
specifically reformat the "update_local_state" ControllerSchema construction and
the handle_snapshot() function so spacing, indentation, and trailing commas
match rustfmt style and project guidelines.
In `@src/openhuman/mod.rs`:
- Around line 14-17: Module declarations are out of alphabetical order: move the
line declaring pub mod app_state; to appear before pub mod agent; (so the block
reads about_app, accessibility, app_state, agent) or simply run cargo fmt --all
to automatically reorder and format the module declarations (ensure pub mod
app_state and pub mod agent are in alphabetical order).
In `@src/openhuman/team/schemas.rs`:
- Around line 162-230: The new ControllerSchema arms (e.g., "team_list_teams",
"team_get_team", "team_create_team", "team_update_team", "team_delete_team",
"team_switch_team", "team_leave_team", "team_join_team") are failing rustfmt;
run rustfmt by executing `cargo fmt --all` (or `cargo fmt --all -- --check`
locally to verify) to reformat src/openhuman/team/schemas.rs and then re-run
`cargo check` to ensure no formatting or compilation issues remain before
merging.
- Around line 475-482: The FieldSchema built by optional_string() currently sets
ty: TypeSchema::String but should represent an optional string; update
optional_string() to use ty: TypeSchema::Option(Box::new(TypeSchema::String))
(mirroring optional_u64()) while keeping required: false so the schema matches
UpdateTeamParams.name and other Option<String> fields and ensures
docs/validation reflect the optional type.
---
Outside diff comments:
In `@src/openhuman/team/ops.rs`:
- Around line 35-38: The resolve_base function uses effective_api_url and then
calls Url::parse(...). Ensure the base URL is normalized to always end with a
trailing slash before any subsequent base.join(...) calls so path segments are
not dropped (e.g., if effective_api_url returns "https://host/api" convert it to
"https://host/api/"); update resolve_base to append a '/' when the parsed Url
has a non-empty path and does not already end with '/', and apply the same
normalization wherever you call base.join (including the analogous helper in
app_state ops) so join preserves the path prefix.
---
Nitpick comments:
In `@app/src/components/settings/panels/TeamManagementPanel.tsx`:
- Around line 25-29: The effect in TeamManagementPanel currently calls
refreshTeams whenever teams.length === 0 on mount, causing unnecessary fetches
for users who truly have no teams; update the component to fetch only once by
tracking whether we've already attempted the initial load (e.g., add a ref or
state like teamsFetchedRef/teamsInitialized) and change the useEffect to run
refreshTeams only if teams.length === 0 AND teamsFetchedRef.current is false,
then set teamsFetchedRef.current = true after calling refreshTeams; also ensure
you reset/mark fetched true if teams become non-empty so subsequent mounts won't
re-fetch unnecessarily.
In `@app/src/hooks/useUser.ts`:
- Line 12: The hook useUser currently hardcodes "error: null", which hides any
errors from the underlying user state; change the implementation to read and
return the error from the core snapshot (e.g., use snapshot.error or
snapshot?.error) instead of null so consumers receive real error state, or if
hiding errors is intentional add a comment/docs indicating that behavior; update
the returned object from useUser (the function named useUser and any variables
like snapshot or coreSnapshot) to propagate snapshot error (e.g., error:
snapshot?.error ?? null) rather than the fixed null.
In `@app/src/pages/onboarding/Onboarding.tsx`:
- Line 31: The call to useUser() in Onboarding.tsx is unused; either remove the
call if it's not needed, or explicitly use its return value (e.g., const user =
useUser() or destructure needed fields) if the component depends on the user, or
if the call is intentionally only for side effects keep the call but add a
comment like “// ensure user context is initialized” so intent is clear; update
the useUser() usage accordingly in the Onboarding component.
In `@app/src/services/coreStateApi.ts`:
- Around line 30-74: Convert the exported function declarations to exported
const arrow functions and extract the inline param object type used by
updateCoreLocalState into a named interface; e.g., create interface
UpdateCoreLocalStateParams { encryptionKey?: string | null;
primaryWalletAddress?: string | null; onboardingTasks?: OnboardingTasks | null }
and interface OnboardingTasks { accessibilityPermissionGranted: boolean;
localModelConsentGiven: boolean; localModelDownloadStarted: boolean;
enabledTools: string[]; connectedSources: string[]; updatedAtMs?: number } then
change export function fetchCoreAppSnapshot, updateCoreLocalState, listTeams,
getTeamMembers, and getTeamInvites to exported consts using arrow syntax while
keeping their existing return types and callCoreRpc usage.
In `@app/src/utils/__tests__/desktopDeepLinkListener.test.ts`:
- Around line 1-5: The test file desktopDeepLinkListener.test.ts currently skips
the legacy Redux wiring; open a tracking issue and update the tests to exercise
the new core-session APIs by removing describe.skip and rewriting the test cases
to call the desktopDeepLinkListener code paths that now rely on
getCoreStateSnapshot() and storeSession() (mocking/stubbing those functions to
simulate session state and asserting the listener behavior), and add the issue
ID to the test TODO so CI fails if coverage is not restored.
🪄 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: eecdb781-a469-4663-bf31-e15993a6d8ef
📒 Files selected for processing (47)
app/src/App.tsxapp/src/components/BottomTabBar.tsxapp/src/components/DefaultRedirect.tsxapp/src/components/MiniSidebar.tsxapp/src/components/OnboardingOverlay.tsxapp/src/components/ProtectedRoute.tsxapp/src/components/PublicRoute.tsxapp/src/components/WalletInfoSection.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/panels/BillingPanel.tsxapp/src/components/settings/panels/PrivacyPanel.tsxapp/src/components/settings/panels/RecoveryPhrasePanel.tsxapp/src/components/settings/panels/TeamInvitesPanel.tsxapp/src/components/settings/panels/TeamManagementPanel.tsxapp/src/components/settings/panels/TeamMembersPanel.tsxapp/src/components/settings/panels/TeamPanel.tsxapp/src/hooks/useConsciousItems.tsapp/src/hooks/useIntelligenceSocket.tsapp/src/hooks/useUser.tsapp/src/hooks/useWebhooks.tsapp/src/lib/coreState/store.tsapp/src/lib/skills/manager.tsapp/src/main.tsxapp/src/pages/Intelligence.tsxapp/src/pages/Mnemonic.tsxapp/src/pages/onboarding/Onboarding.tsxapp/src/pages/onboarding/steps/MnemonicStep.tsxapp/src/providers/CoreStateProvider.tsxapp/src/providers/IntelligenceProvider.tsxapp/src/providers/SocketProvider.tsxapp/src/services/analytics.tsapp/src/services/api/teamApi.tsapp/src/services/coreStateApi.tsapp/src/services/daemonHealthService.tsapp/src/services/socketService.tsapp/src/store/index.tsapp/src/store/socketSelectors.tsapp/src/utils/__tests__/authFlow.e2e.test.tsxapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/desktopDeepLinkListener.tssrc/core/all.rssrc/openhuman/app_state/mod.rssrc/openhuman/app_state/ops.rssrc/openhuman/app_state/schemas.rssrc/openhuman/mod.rssrc/openhuman/team/ops.rssrc/openhuman/team/schemas.rs
💤 Files with no reviewable changes (1)
- app/src/providers/IntelligenceProvider.tsx
Comment on lines
+11
to
15
| const handleToggleAnalytics = async () => { | ||
| const newValue = !analyticsEnabled; | ||
| dispatch(setAnalyticsForUser({ userId: user._id, enabled: newValue })); | ||
| syncAnalyticsConsent(newValue); | ||
|
|
||
| // Sync to core config so the Rust process also respects the setting | ||
| if (isTauri()) { | ||
| openhumanUpdateAnalyticsSettings({ enabled: newValue }).catch(() => { | ||
| /* best-effort */ | ||
| }); | ||
| } | ||
| await setAnalyticsEnabled(newValue); | ||
| }; |
Contributor
There was a problem hiding this comment.
Persist the consent change before flipping runtime analytics.
syncAnalyticsConsent(newValue) runs before the awaited core write. If setAnalyticsEnabled(newValue) fails, runtime collection is already changed while the saved setting and UI stay on the old value.
🔒 Persist first, then sync runtime state
const handleToggleAnalytics = async () => {
const newValue = !analyticsEnabled;
- syncAnalyticsConsent(newValue);
- await setAnalyticsEnabled(newValue);
+ await setAnalyticsEnabled(newValue);
+ syncAnalyticsConsent(newValue);
};📝 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
| const handleToggleAnalytics = async () => { | |
| const newValue = !analyticsEnabled; | |
| dispatch(setAnalyticsForUser({ userId: user._id, enabled: newValue })); | |
| syncAnalyticsConsent(newValue); | |
| // Sync to core config so the Rust process also respects the setting | |
| if (isTauri()) { | |
| openhumanUpdateAnalyticsSettings({ enabled: newValue }).catch(() => { | |
| /* best-effort */ | |
| }); | |
| } | |
| await setAnalyticsEnabled(newValue); | |
| }; | |
| const handleToggleAnalytics = async () => { | |
| const newValue = !analyticsEnabled; | |
| await setAnalyticsEnabled(newValue); | |
| syncAnalyticsConsent(newValue); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/PrivacyPanel.tsx` around lines 11 - 15,
The handler handleToggleAnalytics flips runtime analytics before the awaited
persistent update, so if setAnalyticsEnabled(newValue) fails the runtime state
(syncAnalyticsConsent) will be wrong; change the order to await
setAnalyticsEnabled(newValue) first and only call syncAnalyticsConsent(newValue)
after the persistent write succeeds, and wrap the await in try/catch to avoid
leaving UI/runtime inconsistent (use analyticsEnabled, setAnalyticsEnabled, and
syncAnalyticsConsent references to locate the code).
Comment on lines
14
to
17
| pub mod about_app; | ||
| pub mod accessibility; | ||
| pub mod app_state; | ||
| pub mod agent; |
Contributor
There was a problem hiding this comment.
Fix module declaration order to pass cargo fmt.
The pipeline failed because app_state is declared after agent but should come before it alphabetically. Run cargo fmt --all to fix the ordering.
🔧 Proposed fix
pub mod about_app;
pub mod accessibility;
-pub mod app_state;
pub mod agent;
+pub mod app_state;
pub mod approval;📝 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
| pub mod about_app; | |
| pub mod accessibility; | |
| pub mod app_state; | |
| pub mod agent; | |
| pub mod about_app; | |
| pub mod accessibility; | |
| pub mod agent; | |
| pub mod app_state; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/mod.rs` around lines 14 - 17, Module declarations are out of
alphabetical order: move the line declaring pub mod app_state; to appear before
pub mod agent; (so the block reads about_app, accessibility, app_state, agent)
or simply run cargo fmt --all to automatically reorder and format the module
declarations (ensure pub mod app_state and pub mod agent are in alphabetical
order).
- Reintroduced the import of polyfills in main.tsx to ensure compatibility. - Adjusted import statements in PrivacyPanel and SocketProvider for consistency. - Simplified conditional expressions in TeamInvitesPanel and TeamMembersPanel for better readability. - Refactored CoreStateProvider and store.ts to streamline state initialization. - Enhanced formatting in various files for improved code clarity and maintainability.
- Updated OnboardingOverlay tests to utilize mock state management and simplify assertions. - Refactored ProtectedRoute tests to improve readability and ensure consistent use of mock state. - Enhanced teamApi tests to replace direct API calls with core RPC method calls, improving test isolation and clarity. - Adjusted socketSelectors tests to utilize a centralized core state snapshot for better state management during tests.
…e loading logic - Removed unnecessary local state for onboarding completion in OnboardingOverlay, directly utilizing snapshot data. - Enhanced loading logic to prevent unnecessary renders and improve user experience during onboarding. - Updated PrivacyPanel to handle analytics consent persistence with error handling. - Refactored TeamManagementPanel and TeamPanel to improve team data fetching logic and loading states. - Streamlined CoreStateProvider to manage session token synchronization and state updates more effectively.
…ment - Added `tempfile` dependency to Cargo.toml for improved temporary file handling. - Refactored app state loading and saving functions to enhance error handling and ensure data integrity. - Introduced quarantine mechanism for corrupted app state files to prevent application crashes. - Updated URL parsing logic to ensure proper formatting of API URLs. - Adjusted type definitions in schemas for better clarity and consistency.
…d route tests - Consolidated mock state management in OnboardingOverlay and ProtectedRoute tests for improved readability. - Streamlined assertions by reducing unnecessary lines in test setups. - Enhanced consistency in mock return values across tests to ensure clarity and maintainability.
- Updated PublicRoute tests to utilize MemoryRouter for improved routing simulation. - Simplified mock state management by integrating `useCoreState` for user authentication scenarios. - Streamlined test assertions and removed redundant preloaded state configurations for clarity and maintainability.
…nic tests - Simplified mock state management in PublicRoute and Mnemonic tests for improved readability. - Consolidated mock return values to reduce redundancy and enhance clarity in test setups. - Improved consistency in the usage of `useCoreState` across test files.
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/store/index.ts (1)
59-70:⚠️ Potential issue | 🔴 CriticalDon't remove these runtime slices until the remaining consumers are migrated.
RootState = RuntimeRootState & LegacyRootStateonly hides the breakage from TypeScript;configureStorestill stops mountingauth,user,team, andintelligenceat runtime. Consumers that still read those paths — e.g.app/src/providers/UserProvider.tsx:32-33andapp/src/store/authSelectors.ts:3-7in the provided context — will now receiveundefinedand can throw as soon as they dereference the missing slice. Keep the legacy reducers mounted (or stub them with the old shape) until those readers are removed, then delete the shim in the same PR.Also applies to: 88-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/store/index.ts` around lines 59 - 70, The configureStore call removed legacy runtime slices causing consumers to get undefined for auth/user/team/intelligence; restore the legacy reducers (or add stubs with the original state shape) to the reducer map used by configureStore so slices like auth, user, team, and intelligence remain mounted until consumers are migrated; update persistedAiReducer/persistedThreadReducer usage only after ensuring RootState (RuntimeRootState & LegacyRootState) matches the actual mounted reducers and references to authSelectors and UserProvider keep receiving the expected shape.
🧹 Nitpick comments (7)
app/src/services/coreStateApi.ts (1)
20-36: Consider narrowing theauth.usertype.
auth.useris typed asunknown | nullwhilecurrentUseris properly typed asUser | null. If they share the same shape, consolidate toUser | nullfor consistency and better type safety downstream.♻️ Suggested type refinement
interface AppStateSnapshotResult { auth: { isAuthenticated: boolean; userId: string | null; - user: unknown | null; + user: User | null; profileId: string | null; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/coreStateApi.ts` around lines 20 - 36, Update the AppStateSnapshotResult type to tighten auth.user from unknown | null to User | null so it matches currentUser; locate the AppStateSnapshotResult interface and change the auth.user property type to User | null (ensuring any imports or type references for User are available) so downstream code benefits from consistent, stronger typing between auth.user and currentUser.src/openhuman/team/schemas.rs (1)
68-84: Add contract tests for the new team controller IDs.These
namespace/functionpairs are now consumed directly byapp/src/services/api/teamApi.ts, but the Rust tests in this module only pinteam_list_membersand overall list lengths. A typo in any new controller name or input field would still compile and only fail at runtime as an unknown method. A small table-driven test over the new schemas would catch that drift on the Rust side first.Based on learnings: Keep functionality validation in Rust tests for
rust-core.Also applies to: 148-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/team/schemas.rs` around lines 68 - 84, Add a table-driven contract test that verifies each controller schema returned by all_team_controller_schemas() contains the expected namespace/function IDs used by the JS client; iterate the vector produced by all_team_controller_schemas() (and/or call team_schemas() for each expected ID like "team_list_members", "team_list_teams", "team_get_team", "team_create_team", "team_update_team", "team_delete_team", "team_switch_team", "team_leave_team", "team_join_team", "team_create_invite", "team_list_invites", "team_revoke_invite", "team_remove_member", "team_change_member_role") and assert the schema.namespace and schema.function (or equivalent fields on ControllerSchema) match the expected strings so a typo or missing schema will fail at test time; add this test to the same test module that contains all_team_controller_schemas().app/src/providers/CoreStateProvider.tsx (1)
198-219: RPC success/failure responses are not validated before committing state.Both
setAnalyticsEnabledandsetOnboardingCompletedFlagawait the RPC call but don't check the response. Per context snippet 4,setOnboardingCompletedreturnsPromise<boolean>indicating success/failure, andopenhumanUpdateAnalyticsSettingsreturnsCommandResponse<ConfigSnapshot>.If the core returns
falseor an error response (but doesn't throw), the local state is still optimistically updated and may drift from the actual persisted state until the next poll.Consider validating responses:
const setOnboardingCompletedFlag = useCallback( async (value: boolean) => { - await setOnboardingCompleted(value); + const success = await setOnboardingCompleted(value); + if (!success) { + console.warn('[core-state] onboarding flag update rejected by core'); + return; + } commitState(previous => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/providers/CoreStateProvider.tsx` around lines 198 - 219, Both callbacks optimistically commit local state without validating RPC results; update setAnalyticsEnabled and setOnboardingCompletedFlag to inspect the RPC responses from openhumanUpdateAnalyticsSettings (CommandResponse<ConfigSnapshot>) and setOnboardingCompleted (Promise<boolean>) before calling commitState or syncAnalyticsConsent: if the RPC indicates success use commitState as now, if it indicates failure or returns false do not mutate snapshot (or revert) and surface/log the error so UI state stays consistent with core. Reference the functions setAnalyticsEnabled, setOnboardingCompletedFlag, openhumanUpdateAnalyticsSettings, setOnboardingCompleted, commitState and syncAnalyticsConsent when implementing the checks and error handling.src/openhuman/team/ops.rs (3)
263-276:update_teamallows sending an empty update body.If
nameisNoneor whitespace-only, the function sendsPUT /teams/:teamIdwith{}. This may be intentional for future extensibility, but currently the only field isname, so an empty update is a no-op.Consider either returning early if no fields to update, or documenting this as intentional:
pub async fn update_team( config: &Config, team_id: &str, name: Option<&str>, ) -> Result<RpcOutcome<Value>, String> { let team_id = normalize_id(team_id, "teamId")?; let path = build_api_path(&["teams", &team_id])?; let mut body = serde_json::Map::new(); if let Some(name) = name.map(str::trim).filter(|value| !value.is_empty()) { body.insert("name".to_string(), Value::String(name.to_string())); } + // Note: Empty body is allowed for forward compatibility with additional fields. let data = get_authed_value(config, Method::PUT, &path, Some(Value::Object(body))).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/team/ops.rs` around lines 263 - 276, The update_team function currently sends an empty PUT when no updatable fields are provided; modify update_team so that after building the JSON body (the serde_json::Map used before calling get_authed_value) you check if body.is_empty() and return an Err with a clear message (e.g. "no fields to update") instead of calling get_authed_value; reference the update_team function and its use of normalize_id, build_api_path, body (serde_json::Map), get_authed_value and RpcOutcome to locate where to add this early-return check.
243-261: Simplifycreate_teamserialization.The
TeamNameBodystruct is only used increate_teamand serialized viajson!()macro. This is redundant - you can either use the struct directly withserde_json::to_value()or skip the struct entirely:-#[derive(Debug, Serialize)] -struct TeamNameBody<'a> { - name: &'a str, -} - pub async fn create_team(config: &Config, name: &str) -> Result<RpcOutcome<Value>, String> { let trimmed = name.trim(); if trimmed.is_empty() { return Err("name is required".to_string()); } let data = get_authed_value( config, Method::POST, "/teams", - Some(json!(TeamNameBody { name: trimmed })), + Some(json!({ "name": trimmed })), ) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/team/ops.rs` around lines 243 - 261, Remove the unused TeamNameBody struct and simplify create_team by passing a plain JSON object for the request body; replace json!(TeamNameBody { name: trimmed }) with json!({"name": trimmed}) (or alternatively use serde_json::to_value if you prefer a typed value) in the get_authed_value call so create_team, get_authed_value, and RpcOutcome::single_log remain unchanged.
35-44: Extract duplicateresolve_basefunction to shared utility module.The
resolve_basefunction is duplicated identically in bothsrc/openhuman/team/ops.rs(lines 35-44) andsrc/openhuman/app_state/ops.rs(lines 210-219). This DRY violation risks divergent behavior if one is updated without the other.Additionally,
src/openhuman/billing/ops.rsimplements a variant without trailing slash normalization, indicating this function has multiple valid forms across the codebase.Consider creating a shared utility module (e.g.,
crate::openhuman::api_utilsor similar) with both versions, then import and use appropriately in each ops.rs file. Runcargo fmtandcargo checkbefore merging any refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/team/ops.rs` around lines 35 - 44, The duplicated resolve_base function should be extracted to a shared utility module (e.g., crate::openhuman::api_utils) and replaced in both callers; create two exported helpers to cover both behaviors (e.g., resolve_base_with_trailing_slash and resolve_base_without_trailing_normalization) implement the parsing and error mapping once, move the trailing-slash normalization into the _with_trailing_slash variant, update usages in the Team ops and AppState ops to import and call the appropriate helper (replace local resolve_base references), and run cargo fmt and cargo check to verify the refactor.app/src/hooks/useIntelligenceSocket.ts (1)
8-18: Consider adding JSDoc or removing unused no-op stubs.
useIntelligenceSocketreturns no-op functions (sendMessage,sendChatInit,sendTyping) that are never called by consumers (per context,Intelligence.tsxonly usesisConnected). This could confuse future maintainers who might expect these to work.Either:
- Remove the unused stubs if the functionality is permanently deprecated
- Add a comment explaining these are intentional no-ops pending future implementation
export const useIntelligenceSocket = () => { const socketStatus = useAppSelector(selectSocketStatus); + // Note: send* methods are no-ops after Redux intelligence removal. + // Socket messaging is now handled directly by the core. return { isConnected: socketStatus === 'connected', isReady: socketStatus === 'connected', sendMessage: async () => {}, sendChatInit: async () => {}, sendTyping: () => {}, }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/hooks/useIntelligenceSocket.ts` around lines 8 - 18, The hook useIntelligenceSocket currently returns no-op functions (sendMessage, sendChatInit, sendTyping) that are unused; either remove these unused properties from the returned object so callers only receive isConnected/isReady, or add a concise JSDoc above useIntelligenceSocket stating that sendMessage, sendChatInit, and sendTyping are intentionally no-ops/pending implementation; update references inside the hook (sendMessage/sendChatInit/sendTyping) and ensure isConnected/isReady remain unchanged.
🤖 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/__tests__/ProtectedRoute.test.tsx`:
- Around line 22-40: The test currently only verifies ProtectedRoute children
are absent when mockUseCoreState returns isBootstrapping: true; change it to
assert the actual loading UI is rendered by checking for RouteLoadingScreen
(e.g., its stable text or test hook) rather than just absence of "Protected
Content". Update the case in ProtectedRoute.test.tsx to mockUseCoreState({
isBootstrapping: true, snapshot: { sessionToken: null }}) then use renderRoute
and assert screen.getByText or getByTestId for the RouteLoadingScreen output (or
the existing test utility from app/src/test/test-utils.tsx) so the test fails if
the component returns null.
In `@app/src/components/settings/panels/TeamInvitesPanel.tsx`:
- Around line 21-36: Change the invites handling to distinguish "not loaded"
from "loaded empty" by making invites undefined on cache-miss (const invites =
currentTeamId ? teamInvitesById[currentTeamId] : undefined) instead of
defaulting to [], remove the local isLoadingInvites state and its
setIsLoadingInvites calls in the useEffect, and simply call void
refreshTeamInvites(currentTeamId) when currentTeamId changes; this avoids
synchronous setState in the useEffect and eliminates the shared loading race—if
you must show a loading UI, derive it from invites === undefined (or expose a
loading flag from useCoreState) and, if refreshTeamInvites can race, add a
request token/abort handling inside refreshTeamInvites or use a ref-based
requestId check so stale .finally handlers don’t clear state for a newer team.
In `@app/src/lib/coreState/store.ts`:
- Around line 60-85: patchCoreStateSnapshot causes optimistic local changes to
be lost by a stale poll because setCoreStateSnapshot blindly replaces
currentState; fix by adding an optimistic-update timestamp and merging logic:
have patchCoreStateSnapshot (and any caller like manager.ts) write a
lastLocalUpdate timestamp inside currentState.snapshot.localState metadata, then
change setCoreStateSnapshot to compare incoming snapshot timestamps and
merge/preserve any localState fields whose lastLocalUpdate is newer than the
incoming snapshot (or skip applying the poll entirely if its snapshot timestamp
is older than lastLocalUpdate). Update getCoreStateSnapshot usage accordingly so
callers read the merged state; this keeps optimistic updates (e.g.,
localState.primaryWalletAddress) from being overwritten by stale poll responses
while keeping poll-driven full replacements when newer.
---
Outside diff comments:
In `@app/src/store/index.ts`:
- Around line 59-70: The configureStore call removed legacy runtime slices
causing consumers to get undefined for auth/user/team/intelligence; restore the
legacy reducers (or add stubs with the original state shape) to the reducer map
used by configureStore so slices like auth, user, team, and intelligence remain
mounted until consumers are migrated; update
persistedAiReducer/persistedThreadReducer usage only after ensuring RootState
(RuntimeRootState & LegacyRootState) matches the actual mounted reducers and
references to authSelectors and UserProvider keep receiving the expected shape.
---
Nitpick comments:
In `@app/src/hooks/useIntelligenceSocket.ts`:
- Around line 8-18: The hook useIntelligenceSocket currently returns no-op
functions (sendMessage, sendChatInit, sendTyping) that are unused; either remove
these unused properties from the returned object so callers only receive
isConnected/isReady, or add a concise JSDoc above useIntelligenceSocket stating
that sendMessage, sendChatInit, and sendTyping are intentionally no-ops/pending
implementation; update references inside the hook
(sendMessage/sendChatInit/sendTyping) and ensure isConnected/isReady remain
unchanged.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 198-219: Both callbacks optimistically commit local state without
validating RPC results; update setAnalyticsEnabled and
setOnboardingCompletedFlag to inspect the RPC responses from
openhumanUpdateAnalyticsSettings (CommandResponse<ConfigSnapshot>) and
setOnboardingCompleted (Promise<boolean>) before calling commitState or
syncAnalyticsConsent: if the RPC indicates success use commitState as now, if it
indicates failure or returns false do not mutate snapshot (or revert) and
surface/log the error so UI state stays consistent with core. Reference the
functions setAnalyticsEnabled, setOnboardingCompletedFlag,
openhumanUpdateAnalyticsSettings, setOnboardingCompleted, commitState and
syncAnalyticsConsent when implementing the checks and error handling.
In `@app/src/services/coreStateApi.ts`:
- Around line 20-36: Update the AppStateSnapshotResult type to tighten auth.user
from unknown | null to User | null so it matches currentUser; locate the
AppStateSnapshotResult interface and change the auth.user property type to User
| null (ensuring any imports or type references for User are available) so
downstream code benefits from consistent, stronger typing between auth.user and
currentUser.
In `@src/openhuman/team/ops.rs`:
- Around line 263-276: The update_team function currently sends an empty PUT
when no updatable fields are provided; modify update_team so that after building
the JSON body (the serde_json::Map used before calling get_authed_value) you
check if body.is_empty() and return an Err with a clear message (e.g. "no fields
to update") instead of calling get_authed_value; reference the update_team
function and its use of normalize_id, build_api_path, body (serde_json::Map),
get_authed_value and RpcOutcome to locate where to add this early-return check.
- Around line 243-261: Remove the unused TeamNameBody struct and simplify
create_team by passing a plain JSON object for the request body; replace
json!(TeamNameBody { name: trimmed }) with json!({"name": trimmed}) (or
alternatively use serde_json::to_value if you prefer a typed value) in the
get_authed_value call so create_team, get_authed_value, and
RpcOutcome::single_log remain unchanged.
- Around line 35-44: The duplicated resolve_base function should be extracted to
a shared utility module (e.g., crate::openhuman::api_utils) and replaced in both
callers; create two exported helpers to cover both behaviors (e.g.,
resolve_base_with_trailing_slash and
resolve_base_without_trailing_normalization) implement the parsing and error
mapping once, move the trailing-slash normalization into the
_with_trailing_slash variant, update usages in the Team ops and AppState ops to
import and call the appropriate helper (replace local resolve_base references),
and run cargo fmt and cargo check to verify the refactor.
In `@src/openhuman/team/schemas.rs`:
- Around line 68-84: Add a table-driven contract test that verifies each
controller schema returned by all_team_controller_schemas() contains the
expected namespace/function IDs used by the JS client; iterate the vector
produced by all_team_controller_schemas() (and/or call team_schemas() for each
expected ID like "team_list_members", "team_list_teams", "team_get_team",
"team_create_team", "team_update_team", "team_delete_team", "team_switch_team",
"team_leave_team", "team_join_team", "team_create_invite", "team_list_invites",
"team_revoke_invite", "team_remove_member", "team_change_member_role") and
assert the schema.namespace and schema.function (or equivalent fields on
ControllerSchema) match the expected strings so a typo or missing schema will
fail at test time; add this test to the same test module that contains
all_team_controller_schemas().
🪄 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: 1fe97db8-3d8e-4e67-b128-1a71848db443
📒 Files selected for processing (31)
Cargo.tomlapp/src/components/MiniSidebar.tsxapp/src/components/OnboardingOverlay.tsxapp/src/components/__tests__/OnboardingOverlay.test.tsxapp/src/components/__tests__/ProtectedRoute.test.tsxapp/src/components/settings/panels/PrivacyPanel.tsxapp/src/components/settings/panels/TeamInvitesPanel.tsxapp/src/components/settings/panels/TeamManagementPanel.tsxapp/src/components/settings/panels/TeamMembersPanel.tsxapp/src/components/settings/panels/TeamPanel.tsxapp/src/hooks/useConsciousItems.tsapp/src/hooks/useIntelligenceSocket.tsapp/src/hooks/useUser.tsapp/src/lib/coreState/store.tsapp/src/lib/skills/manager.tsapp/src/main.tsxapp/src/pages/Intelligence.tsxapp/src/pages/onboarding/Onboarding.tsxapp/src/providers/CoreStateProvider.tsxapp/src/providers/SocketProvider.tsxapp/src/services/api/__tests__/teamApi.test.tsapp/src/services/api/teamApi.tsapp/src/services/coreStateApi.tsapp/src/services/socketService.tsapp/src/store/__tests__/socketSelectors.test.tsapp/src/store/index.tssrc/openhuman/app_state/ops.rssrc/openhuman/app_state/schemas.rssrc/openhuman/mod.rssrc/openhuman/team/ops.rssrc/openhuman/team/schemas.rs
✅ Files skipped from review due to trivial changes (3)
- app/src/hooks/useConsciousItems.ts
- src/openhuman/app_state/schemas.rs
- app/src/components/settings/panels/TeamMembersPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/components/MiniSidebar.tsx
- app/src/main.tsx
- app/src/components/settings/panels/TeamPanel.tsx
- app/src/components/settings/panels/TeamManagementPanel.tsx
- app/src/pages/Intelligence.tsx
- app/src/providers/SocketProvider.tsx
Comment on lines
+22
to
+40
| it('renders a loading screen while bootstrapping', () => { | ||
| mockUseCoreState.mockReturnValue({ | ||
| isBootstrapping: true, | ||
| snapshot: { sessionToken: null }, | ||
| }); | ||
|
|
||
| renderRoute( | ||
| <Route | ||
| path="/" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <div>Protected Content</div> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| ); | ||
|
|
||
| expect(screen.queryByText('Protected Content')).not.toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the loading UI itself here.
This test still passes if ProtectedRoute returns null. The production component renders RouteLoadingScreen while isBootstrapping is true, so please assert that screen (or its stable test hook) instead of only checking that children are absent.
Based on learnings: Prefer testing behavior over implementation details; use existing helpers from app/src/test/ (test-utils.tsx, shared mock backend) before adding new harness code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/__tests__/ProtectedRoute.test.tsx` around lines 22 - 40,
The test currently only verifies ProtectedRoute children are absent when
mockUseCoreState returns isBootstrapping: true; change it to assert the actual
loading UI is rendered by checking for RouteLoadingScreen (e.g., its stable text
or test hook) rather than just absence of "Protected Content". Update the case
in ProtectedRoute.test.tsx to mockUseCoreState({ isBootstrapping: true,
snapshot: { sessionToken: null }}) then use renderRoute and assert
screen.getByText or getByTestId for the RouteLoadingScreen output (or the
existing test utility from app/src/test/test-utils.tsx) so the test fails if the
component returns null.
Comment on lines
+21
to
+36
| const invites = currentTeamId ? (teamInvitesById[currentTeamId] ?? []) : []; | ||
|
|
||
| const [isGenerating, setIsGenerating] = useState(false); | ||
| const [copiedId, setCopiedId] = useState<string | null>(null); | ||
| const [revokingId, setRevokingId] = useState<string | null>(null); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [isLoadingInvites, setIsLoadingInvites] = useState(false); | ||
|
|
||
| // Confirmation modal state | ||
| const [inviteToRevoke, setInviteToRevoke] = useState<{ id: string; code: string } | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (currentTeamId) dispatch(fetchInvites(currentTeamId)); | ||
| }, [currentTeamId, dispatch]); | ||
| if (!currentTeamId) return; | ||
| setIsLoadingInvites(true); | ||
| void refreshTeamInvites(currentTeamId).finally(() => setIsLoadingInvites(false)); | ||
| }, [currentTeamId, refreshTeamInvites]); |
Contributor
There was a problem hiding this comment.
Keep "not loaded yet" distinct from "loaded empty" for invites.
const invites = ... ?? [] plus isLoadingInvites starting at false means each team change can render the "No invites yet" branch once before the effect runs. The shared flag also races: an older refreshTeamInvites() can still hit .finally() and clear loading for a newer currentTeamId. Treat the cache miss as its own state (undefined vs []) or expose loading from useCoreState instead of toggling local state in the effect.
Based on learnings: In React components, do not perform synchronous setState (or other state-updating calls) directly inside useEffect bodies. The lint rule react-hooks/set-state-in-effect (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/settings/panels/TeamInvitesPanel.tsx` around lines 21 -
36, Change the invites handling to distinguish "not loaded" from "loaded empty"
by making invites undefined on cache-miss (const invites = currentTeamId ?
teamInvitesById[currentTeamId] : undefined) instead of defaulting to [], remove
the local isLoadingInvites state and its setIsLoadingInvites calls in the
useEffect, and simply call void refreshTeamInvites(currentTeamId) when
currentTeamId changes; this avoids synchronous setState in the useEffect and
eliminates the shared loading race—if you must show a loading UI, derive it from
invites === undefined (or expose a loading flag from useCoreState) and, if
refreshTeamInvites can race, add a request token/abort handling inside
refreshTeamInvites or use a ref-based requestId check so stale .finally handlers
don’t clear state for a newer team.
Comment on lines
+60
to
+85
| export function getCoreStateSnapshot(): CoreState { | ||
| return currentState; | ||
| } | ||
|
|
||
| export function setCoreStateSnapshot(next: CoreState): void { | ||
| currentState = next; | ||
| } | ||
|
|
||
| export function patchCoreStateSnapshot(patch: { | ||
| snapshot?: Record<string, unknown> & { localState?: Partial<CoreLocalState> }; | ||
| [key: string]: unknown; | ||
| }): void { | ||
| currentState = { | ||
| ...currentState, | ||
| ...patch, | ||
| snapshot: patch.snapshot | ||
| ? { | ||
| ...currentState.snapshot, | ||
| ...patch.snapshot, | ||
| localState: patch.snapshot.localState | ||
| ? { ...currentState.snapshot.localState, ...patch.snapshot.localState } | ||
| : currentState.snapshot.localState, | ||
| } | ||
| : currentState.snapshot, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all usages of patchCoreStateSnapshot to understand the race window
rg -n 'patchCoreStateSnapshot' --type=ts -C3Repository: tinyhumansai/openhuman
Length of output: 1465
🏁 Script executed:
# Find all usages of setCoreStateSnapshot
rg -n 'setCoreStateSnapshot' --type=ts -B2 -A2Repository: tinyhumansai/openhuman
Length of output: 4147
🏁 Script executed:
# Look for CoreStateProvider and polling mechanism
rg -n 'CoreStateProvider\|setInterval\|polling' --type=tsx --type=ts -B2 -A2Repository: tinyhumansai/openhuman
Length of output: 93
🏁 Script executed:
# Check the full manager.ts context around line 528 to understand the RPC flow
head -n 540 app/src/lib/skills/manager.ts | tail -n 30Repository: tinyhumansai/openhuman
Length of output: 990
🏁 Script executed:
# Get the full CoreStateProvider.tsx to understand polling
cat -n app/src/providers/CoreStateProvider.tsxRepository: tinyhumansai/openhuman
Length of output: 11030
🏁 Script executed:
# Search for setInterval or useEffect with polling
rg -n 'useEffect|setInterval|clearInterval|refreshCoreState|3000|3 ?s' --type=tsx --type=ts app/src/providers/CoreStateProvider.tsx -A5Repository: tinyhumansai/openhuman
Length of output: 93
🏁 Script executed:
# Get complete context of patchCoreStateSnapshot usage in manager.ts
sed -n '515,540p' app/src/lib/skills/manager.tsRepository: tinyhumansai/openhuman
Length of output: 865
🏁 Script executed:
# Check if there are any other usages of patchCoreStateSnapshot
rg 'patchCoreStateSnapshot' --type=ts -lRepository: tinyhumansai/openhuman
Length of output: 126
🏁 Script executed:
# Check the store.ts file fully to understand all functions
cat -n app/src/lib/coreState/store.ts | head -n 90Repository: tinyhumansai/openhuman
Length of output: 2922
Race condition: patchCoreStateSnapshot updates can be lost to polling.
The patchCoreStateSnapshot function applies partial updates to the singleton, but polling in CoreStateProvider runs every 3 seconds, calling fetchCoreAppSnapshot() and then setCoreStateSnapshot() with a complete replacement of currentState. This creates a race condition where:
manager.tscallspatchCoreStateSnapshotto optimistically updatelocalState.primaryWalletAddressafter an RPC write- A polling cycle begins before the RPC persists to the server
- The poll fetches stale data and calls
setCoreStateSnapshot, overwriting the optimistic patch with stale values
The wallet address (and any other patched state) is silently lost until the next poll after the RPC has propagated.
The requestId guard in CoreStateProvider.refresh() only protects against stale POLL results overwriting newer POLL results; it does not prevent local patches from being overwritten by stale poll responses.
Consider either:
- Removing
patchCoreStateSnapshotand callingrefresh()after mutations instead - Having
setCoreStateSnapshotpreserve locally patched fields (requires tracking mutations) - Using optimistic update timestamps to skip stale poll responses
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/coreState/store.ts` around lines 60 - 85, patchCoreStateSnapshot
causes optimistic local changes to be lost by a stale poll because
setCoreStateSnapshot blindly replaces currentState; fix by adding an
optimistic-update timestamp and merging logic: have patchCoreStateSnapshot (and
any caller like manager.ts) write a lastLocalUpdate timestamp inside
currentState.snapshot.localState metadata, then change setCoreStateSnapshot to
compare incoming snapshot timestamps and merge/preserve any localState fields
whose lastLocalUpdate is newer than the incoming snapshot (or skip applying the
poll entirely if its snapshot timestamp is older than lastLocalUpdate). Update
getCoreStateSnapshot usage accordingly so callers read the merged state; this
keeps optimistic updates (e.g., localState.primaryWalletAddress) from being
overwritten by stale poll responses while keeping poll-driven full replacements
when newer.
- Cleaned up import statements in BillingPanel.tsx for better organization. - Enhanced formatting of billing plan descriptions in billingHelpers.ts for consistency. - Improved readability of conditional checks in BillingPanel component.
- Updated references from `/settings` to `/auth/me` in various modules to standardize user authentication flows. - Renamed `parse_settings_response_json` to `parse_api_response_json` for improved clarity in response handling. - Adjusted user ID extraction functions to reflect the new endpoint structure, enhancing maintainability across the codebase. - Updated test cases to align with the new endpoint naming conventions, ensuring consistency in API interactions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/__tests__/PublicRoute.test.tsx (1)
19-83:⚠️ Potential issue | 🟡 MinorAdd the bootstrapping case here.
These cases only cover authenticated vs unauthenticated behavior.
PublicRoutealso rendersRouteLoadingScreenwhileisBootstrappingis true, and this new core-state harness is exactly what needs to exercise that path.Based on learnings: Prefer testing behavior over implementation details; use existing helpers from
app/src/test/(test-utils.tsx, shared mock backend) before adding new harness code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/__tests__/PublicRoute.test.tsx` around lines 19 - 83, Add a new test in PublicRoute.test.tsx that covers the bootstrapping path by using mockUseCoreState to return { isBootstrapping: true, snapshot: { sessionToken: null } } and calling the existing renderRoute helper with a Route that renders <PublicRoute> children; assert that RouteLoadingScreen (or its visible text/role) is shown instead of the children. Reuse the existing test setup/helpers (renderRoute, mockUseCoreState) and assert visible behavior rather than internal implementation details.
♻️ Duplicate comments (2)
app/src/providers/CoreStateProvider.tsx (1)
198-218:⚠️ Potential issue | 🔴 CriticalInvalidate in-flight snapshot polls before local snapshot mutations.
These setters update
snapshotlocally, but they never advancesnapshotRequestIdRef.current. If a poll started earlier is still in flight, its response still passes the stale-check inrefresh()and can overwrite the newer local state. The dangerous case isclearSession(): a slow pre-logout snapshot can restoresessionToken/currentUserright after logout.🔧 Suggested fix
+ const invalidateSnapshotRefreshes = useCallback(() => { + snapshotRequestIdRef.current += 1; + }, []); + const setAnalyticsEnabled = useCallback( async (enabled: boolean) => { + invalidateSnapshotRefreshes(); await openhumanUpdateAnalyticsSettings({ enabled }); commitState(previous => ({ ...previous, snapshot: { ...previous.snapshot, analyticsEnabled: enabled }, })); syncAnalyticsConsent(enabled); }, - [commitState] + [commitState, invalidateSnapshotRefreshes] ); const setOnboardingCompletedFlag = useCallback( async (value: boolean) => { + invalidateSnapshotRefreshes(); await setOnboardingCompleted(value); commitState(previous => ({ ...previous, snapshot: { ...previous.snapshot, onboardingCompleted: value }, })); }, - [commitState] + [commitState, invalidateSnapshotRefreshes] ); @@ - const clearSession = useCallback(async () => { + const clearSession = useCallback(async () => { + invalidateSnapshotRefreshes(); await tauriLogout(); commitState(previous => ({ ...previous, teams: [], teamMembersById: {}, @@ })); memoryTokenRef.current = null; - }, [commitState]); + }, [commitState, invalidateSnapshotRefreshes]);Also applies to: 244-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/providers/CoreStateProvider.tsx` around lines 198 - 218, When mutating the local snapshot in setters like setAnalyticsEnabled and setOnboardingCompletedFlag (and other snapshot-updating setters), increment/invalidate snapshotRequestIdRef.current before calling commitState so any earlier in-flight refresh() responses fail the stale-check; i.e., bump snapshotRequestIdRef.current (or call a helper like advanceSnapshotRequestId()) immediately prior to the local commit and then proceed to call commitState and related side-effects (syncAnalyticsConsent, setOnboardingCompleted) to prevent older poll results from overwriting the new local snapshot (apply same change for clearSession and the other snapshot mutators).app/src/components/__tests__/ProtectedRoute.test.tsx (1)
20-35:⚠️ Potential issue | 🟡 MinorAssert the loading UI itself here.
This still passes if
ProtectedRoutereturnsnull. Since the production branch rendersRouteLoadingScreenwhile bootstrapping, please check that screen's stable text or test hook instead of only asserting thatProtected Contentis absent.Based on learnings: Prefer testing behavior over implementation details; use existing helpers from
app/src/test/(test-utils.tsx, shared mock backend) before adding new harness code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/__tests__/ProtectedRoute.test.tsx` around lines 20 - 35, The test only asserts absence of "Protected Content" which would pass if ProtectedRoute returned null; update the test to assert the actual loading UI is rendered while bootstrapping by checking for RouteLoadingScreen's stable text or test hook instead of just absence; specifically, in ProtectedRoute.test (where mockUseCoreState is set to { isBootstrapping: true }), replace or add an assertion that looks for the loading indicator produced by RouteLoadingScreen (stable text or data-testid) using screen.getByText or getByTestId after renderRoute, and prefer using existing test helpers from app/src/test/test-utils.tsx and the shared mock backend rather than adding new harness code.
🤖 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/providers/CoreStateProvider.tsx`:
- Around line 88-126: The refresh function applies a snapshot only if requestId
matches snapshotRequestIdRef.current, but side effects (syncAnalyticsConsent and
syncMemoryClientToken) still run for discarded snapshots; modify refresh to
check the same requestId after commitState and bail out early if it no longer
matches snapshotRequestIdRef.current so you skip calling syncAnalyticsConsent
and any memoryTokenRef/syncMemoryClientToken logic for stale snapshots;
reference the refresh function, snapshotRequestIdRef, commitState,
syncAnalyticsConsent, syncMemoryClientToken, and memoryTokenRef when making this
change.
- Around line 146-166: Guard team member/invite refreshes against auth/session
changes: in refreshTeamMembers and refreshTeamInvites (which call
getTeamMembers/getTeamInvites and then commitState), capture a stable
auth/session identifier from the current state (e.g., current user id or account
id) before awaiting the network call, and after the await verify the identifier
is unchanged; only call commitState to merge results when the auth/session id
still matches. This prevents a late response from getTeamMembers/getTeamInvites
from overwriting data after a logout or account switch (similar to the
protection used in refreshTeams).
---
Outside diff comments:
In `@app/src/components/__tests__/PublicRoute.test.tsx`:
- Around line 19-83: Add a new test in PublicRoute.test.tsx that covers the
bootstrapping path by using mockUseCoreState to return { isBootstrapping: true,
snapshot: { sessionToken: null } } and calling the existing renderRoute helper
with a Route that renders <PublicRoute> children; assert that RouteLoadingScreen
(or its visible text/role) is shown instead of the children. Reuse the existing
test setup/helpers (renderRoute, mockUseCoreState) and assert visible behavior
rather than internal implementation details.
---
Duplicate comments:
In `@app/src/components/__tests__/ProtectedRoute.test.tsx`:
- Around line 20-35: The test only asserts absence of "Protected Content" which
would pass if ProtectedRoute returned null; update the test to assert the actual
loading UI is rendered while bootstrapping by checking for RouteLoadingScreen's
stable text or test hook instead of just absence; specifically, in
ProtectedRoute.test (where mockUseCoreState is set to { isBootstrapping: true
}), replace or add an assertion that looks for the loading indicator produced by
RouteLoadingScreen (stable text or data-testid) using screen.getByText or
getByTestId after renderRoute, and prefer using existing test helpers from
app/src/test/test-utils.tsx and the shared mock backend rather than adding new
harness code.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 198-218: When mutating the local snapshot in setters like
setAnalyticsEnabled and setOnboardingCompletedFlag (and other snapshot-updating
setters), increment/invalidate snapshotRequestIdRef.current before calling
commitState so any earlier in-flight refresh() responses fail the stale-check;
i.e., bump snapshotRequestIdRef.current (or call a helper like
advanceSnapshotRequestId()) immediately prior to the local commit and then
proceed to call commitState and related side-effects (syncAnalyticsConsent,
setOnboardingCompleted) to prevent older poll results from overwriting the new
local snapshot (apply same change for clearSession and the other snapshot
mutators).
🪄 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: 76cbaf46-716b-4dac-9bb1-a8b35fd0d1a5
📒 Files selected for processing (7)
app/src/components/__tests__/OnboardingOverlay.test.tsxapp/src/components/__tests__/ProtectedRoute.test.tsxapp/src/components/__tests__/PublicRoute.test.tsxapp/src/hooks/useUser.tsapp/src/providers/CoreStateProvider.tsxapp/src/store/__tests__/socketSelectors.test.tsapp/test/Mnemonic.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/store/tests/socketSelectors.test.ts
- app/src/hooks/useUser.ts
- app/src/components/tests/OnboardingOverlay.test.tsx
Comment on lines
+88
to
+126
| const refresh = useCallback(async () => { | ||
| const requestId = ++snapshotRequestIdRef.current; | ||
| const snapshot = normalizeSnapshot(await fetchCoreAppSnapshot()); | ||
| commitState(previous => { | ||
| if (requestId !== snapshotRequestIdRef.current) { | ||
| return previous; | ||
| } | ||
|
|
||
| const previousIdentity = snapshotIdentity(previous.snapshot); | ||
| const nextIdentity = snapshotIdentity(snapshot); | ||
| const shouldClearScopedCaches = | ||
| previousIdentity !== nextIdentity || | ||
| (previous.snapshot.auth.isAuthenticated && !snapshot.auth.isAuthenticated); | ||
|
|
||
| return { | ||
| ...previous, | ||
| isBootstrapping: false, | ||
| isReady: true, | ||
| snapshot, | ||
| teams: shouldClearScopedCaches ? [] : previous.teams, | ||
| teamMembersById: shouldClearScopedCaches ? {} : previous.teamMembersById, | ||
| teamInvitesById: shouldClearScopedCaches ? {} : previous.teamInvitesById, | ||
| }; | ||
| }); | ||
| syncAnalyticsConsent(snapshot.analyticsEnabled); | ||
|
|
||
| if (!snapshot.sessionToken) { | ||
| memoryTokenRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| if (memoryTokenRef.current !== snapshot.sessionToken) { | ||
| try { | ||
| await syncMemoryClientToken(snapshot.sessionToken); | ||
| memoryTokenRef.current = snapshot.sessionToken; | ||
| } catch (error) { | ||
| console.warn('[core-state] memory client sync failed during refresh:', error); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Skip analytics/token side effects when a snapshot was discarded.
commitState() correctly drops stale fetchCoreAppSnapshot() results, but the code below still runs syncAnalyticsConsent() and syncMemoryClientToken() for that stale snapshot. A slow pre-logout/pre-login poll can therefore flip consent or re-sync an old session token even though the snapshot was never applied.
🔧 Suggested fix
const refresh = useCallback(async () => {
const requestId = ++snapshotRequestIdRef.current;
const snapshot = normalizeSnapshot(await fetchCoreAppSnapshot());
+ let applied = false;
commitState(previous => {
if (requestId !== snapshotRequestIdRef.current) {
return previous;
}
+ applied = true;
const previousIdentity = snapshotIdentity(previous.snapshot);
const nextIdentity = snapshotIdentity(snapshot);
const shouldClearScopedCaches =
previousIdentity !== nextIdentity ||
@@
teamInvitesById: shouldClearScopedCaches ? {} : previous.teamInvitesById,
};
});
+ if (!applied) {
+ return;
+ }
+
syncAnalyticsConsent(snapshot.analyticsEnabled);
if (!snapshot.sessionToken) {
memoryTokenRef.current = null;
return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/providers/CoreStateProvider.tsx` around lines 88 - 126, The refresh
function applies a snapshot only if requestId matches
snapshotRequestIdRef.current, but side effects (syncAnalyticsConsent and
syncMemoryClientToken) still run for discarded snapshots; modify refresh to
check the same requestId after commitState and bail out early if it no longer
matches snapshotRequestIdRef.current so you skip calling syncAnalyticsConsent
and any memoryTokenRef/syncMemoryClientToken logic for stale snapshots;
reference the refresh function, snapshotRequestIdRef, commitState,
syncAnalyticsConsent, syncMemoryClientToken, and memoryTokenRef when making this
change.
Comment on lines
+146
to
+166
| const refreshTeamMembers = useCallback( | ||
| async (teamId: string) => { | ||
| const members = await getTeamMembers(teamId); | ||
| commitState(previous => ({ | ||
| ...previous, | ||
| teamMembersById: { ...previous.teamMembersById, [teamId]: members }, | ||
| })); | ||
| }, | ||
| [commitState] | ||
| ); | ||
|
|
||
| const refreshTeamInvites = useCallback( | ||
| async (teamId: string) => { | ||
| const invites = await getTeamInvites(teamId); | ||
| commitState(previous => ({ | ||
| ...previous, | ||
| teamInvitesById: { ...previous.teamInvitesById, [teamId]: invites }, | ||
| })); | ||
| }, | ||
| [commitState] | ||
| ); |
Contributor
There was a problem hiding this comment.
Guard team member/invite refreshes against auth changes.
Unlike refreshTeams(), these loaders always merge the response. If logout or an account switch happens while either request is in flight, the late response repopulates previous-user team data into the new session.
🔒 Suggested fix
const refreshTeamMembers = useCallback(
async (teamId: string) => {
+ const identityAtStart = snapshotIdentity(getCoreStateSnapshot().snapshot);
const members = await getTeamMembers(teamId);
- commitState(previous => ({
- ...previous,
- teamMembersById: { ...previous.teamMembersById, [teamId]: members },
- }));
+ commitState(previous => {
+ if (snapshotIdentity(previous.snapshot) !== identityAtStart) {
+ return previous;
+ }
+ return {
+ ...previous,
+ teamMembersById: { ...previous.teamMembersById, [teamId]: members },
+ };
+ });
},
[commitState]
);
const refreshTeamInvites = useCallback(
async (teamId: string) => {
+ const identityAtStart = snapshotIdentity(getCoreStateSnapshot().snapshot);
const invites = await getTeamInvites(teamId);
- commitState(previous => ({
- ...previous,
- teamInvitesById: { ...previous.teamInvitesById, [teamId]: invites },
- }));
+ commitState(previous => {
+ if (snapshotIdentity(previous.snapshot) !== identityAtStart) {
+ return previous;
+ }
+ return {
+ ...previous,
+ teamInvitesById: { ...previous.teamInvitesById, [teamId]: invites },
+ };
+ });
},
[commitState]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/providers/CoreStateProvider.tsx` around lines 146 - 166, Guard team
member/invite refreshes against auth/session changes: in refreshTeamMembers and
refreshTeamInvites (which call getTeamMembers/getTeamInvites and then
commitState), capture a stable auth/session identifier from the current state
(e.g., current user id or account id) before awaiting the network call, and
after the await verify the identifier is unchanged; only call commitState to
merge results when the auth/session id still matches. This prevents a late
response from getTeamMembers/getTeamInvites from overwriting data after a logout
or account switch (similar to the protection used in refreshTeams).
5 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
app_stateRPC surface and poll it from React viaCoreStateProvider.auth,user,team, andintelligencefrom the live Redux store; keep Redux focused on UI/runtime state such as sockets, threads, accessibility, daemon health, and webhooks.Problem
Solution
openhuman.app_state_*controller set in the Rust core that returns a consolidated snapshot of auth/session, current user, onboarding/analytics flags, and local app state persisted under the workspace.CoreStateProviderplus a small core-state client/store in the frontend, polling the sidecar and exposing refresh/mutation helpers to components.Submission Checklist
yarn typecheckandcargo check --manifest-path Cargo.tomlpass for the refactor; dedicated unit coverage was not added in this cut.app_statesurface.Impact
Related
Summary by CodeRabbit
New Features
Refactor
Tests