feat(ux): lock UI to welcome agent until chat onboarding completes (#883)#892
Conversation
…mansai#883) Add `chat_onboarding_completed` to `AppStateSnapshot` alongside the existing `onboarding_completed` flag, populated from `Config::chat_onboarding_completed`. Serialized as camelCase `chatOnboardingCompleted` — the React layer reads this to decide whether to apply the welcome lockdown (hide bottom nav, thread sidebar, account rail) while the welcome-agent chat flow is still in progress. The debug log gains the flag for diagnostic parity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mansai#883) Extend `json_rpc_app_state_snapshot_returns_runtime_shape` to check both `onboardingCompleted` and the new `chatOnboardingCompleted` key — `write_min_config` sets the TOML flag to `true` to bypass the welcome agent in e2e, so the snapshot must surface `true` to match. Add a companion test `json_rpc_app_state_snapshot_chat_onboarding_defaults_false` that writes a config without the key at all and asserts the serde default surfaces as `false`. This is the fresh-user state that triggers the React welcome lockdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tinyhumansai#883) - Add `chatOnboardingCompleted: boolean` to `AppStateSnapshotResult` (`coreStateApi`) and `CoreAppSnapshot` (`lib/coreState/store`), defaulting to `false` in `emptySnapshot`. - `CoreStateProvider::normalizeSnapshot` copies the field through from the RPC result, and `toSignedOutSnapshot` resets it to `false` so a signed-out user never leaks the previous session's flag. - Export `isWelcomeLocked(snapshot)` — the canonical rule for "the UI should be locked to the welcome conversation." Requires authentication + the React wizard completed + chat onboarding still pending. The auth guard keeps the lock from flickering during the signed-out first paint (OnboardingOverlay handles that path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nsai#883) - `BottomTabBar` returns `null` when `isWelcomeLocked(snapshot)` so the user has no cross-route nav affordance while the welcome agent is still running. - `AppShell` drops the `pb-16` tab-bar spacer when locked (the bar is gone — the spacer would be dead space) and adds a bootstrap-gated `useEffect` that redirects any non-`/chat` pathname back to `/chat` via `navigate(..., { replace: true })`. The redirect skips while `isBootstrapping` is true so we don't fight the router during initial paint, and relies on the `OnboardingOverlay`'s own navigation for the wizard-still-up case (the lock condition requires `onboardingCompleted`, so this effect can't run before the overlay dismisses). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…inyhumansai#883) - `Accounts` (the /chat route) hides the left account rail entirely when locked, force-selects the Agent entry via a guarded `useEffect`, and keeps `<AgentChatPanel/>` as the only visible pane so the user cannot jump to a connected webview or trigger the "Add app" modal. - `Conversations` hides the thread sidebar, chat-header sidebar toggle, and "+ New" button when locked — the user has a single welcome conversation and no affordance to spawn another. - On the `chatOnboardingCompleted` false → true transition (the welcome agent just called `complete_onboarding(action: "complete")` and the 2 s snapshot poll surfaced it), dispatch `createNewThread()` so the user lands in a fresh thread with the orchestrator instead of continuing in the welcome thread. The transition is ref-tracked so the poll cycle cannot re-fire the effect while the flag stays `true`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nyhumansai#883) - New `lib/coreState/__tests__/store.test.ts` covers the four corners of `isWelcomeLocked`: both gates met (locked), chat onboarding done (unlocked), wizard still up (unlocked — overlay owns that gate), signed-out (unlocked — prevents the signed-out first-paint flicker). - Thread the new `chatOnboardingCompleted: false` field through the two existing `makeSnapshot` / `resetCoreStateStore` helpers in `CoreStateProvider.test.tsx` and `socketSelectors.test.ts` so the fixtures type-check against the updated `CoreAppSnapshot` shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a chat-onboarding lock: frontend and backend now expose Changes
Sequence DiagramsequenceDiagram
participant User as User/App
participant Frontend as Frontend App (App.tsx)
participant CoreState as Core State (useCoreState)
participant Backend as Backend Core (RPC Snapshot)
participant UI as UI Components (BottomTabBar, Conversations)
User->>Frontend: App bootstraps after onboarding
Frontend->>CoreState: read snapshot via useCoreState()
CoreState->>Backend: fetch app_state_snapshot RPC
Backend-->>CoreState: return snapshot (chatOnboardingCompleted=false)
CoreState-->>Frontend: snapshot + isWelcomeLocked=true
Frontend->>Frontend: detect welcomeLocked
alt Welcome is locked
Frontend->>Frontend: navigate to /chat (replace:true)
Frontend->>UI: render with welcomeLocked=true
UI-->>User: hide BottomTabBar and sidebars
end
User->>Backend: agent completes onboarding
Backend->>Backend: chat_onboarding_completed = true
Frontend->>CoreState: re-fetch snapshot
Backend-->>CoreState: return chatOnboardingCompleted=true
CoreState-->>Frontend: isWelcomeLocked=false
Frontend->>UI: render unlocked UI
UI-->>User: show BottomTabBar, sidebar, auto-open new thread
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collapse App.tsx JSX attribute onto a single line to match prettier's 100-column rule. CI's yarn format:check flagged this after the local commit slipped through.
…at-lockdown # Conflicts: # app/src/pages/Accounts.tsx
…untime (tinyhumansai#883) pnpm migration (tinyhumansai#886) landed this file with a 4-space indent block (prettier expects 6), an unused `waitForText` import, and a no-useless-escape `\/` in a regex. Pre-push husky blocks our push because `pnpm format:check` and `pnpm lint` fail on this upstream file. Fix it here so PR tinyhumansai#892 can land. Unrelated to welcome-lockdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/json_rpc_e2e.rs (1)
1361-1380: Optional: extract fresh-user config writing into a shared helper.This block duplicates config file setup logic already present in helper utilities; pulling it into one helper will reduce drift and future edit risk.
♻️ Suggested refactor sketch
+fn write_min_config_without_chat_onboarding(openhuman_dir: &Path, api_origin: &str) { + let cfg = format!( + r#"api_url = "{api_origin}" +default_model = "e2e-mock-model" +default_temperature = 0.7 + +[secrets] +encrypt = false +"# + ); + fn write_config_file(config_dir: &Path, cfg: &str) { + std::fs::create_dir_all(config_dir).expect("mkdir openhuman"); + std::fs::write(config_dir.join("config.toml"), cfg).expect("write config"); + } + write_config_file(openhuman_dir, &cfg); + write_config_file(&openhuman_dir.join("users").join("local"), &cfg); +} - - let cfg = format!( ... ); - std::fs::create_dir_all(&openhuman_home).expect("mkdir openhuman"); - std::fs::write(openhuman_home.join("config.toml"), &cfg).expect("write config"); - std::fs::create_dir_all(openhuman_home.join("users").join("local")).expect("mkdir users/local"); - std::fs::write(..., &cfg).expect("write user config"); + write_min_config_without_chat_onboarding(&openhuman_home, &mock_origin);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/json_rpc_e2e.rs` around lines 1361 - 1380, Extract the duplicated config setup into a shared test helper (e.g., fn write_test_config(openhuman_home: &Path, cfg: &str) or fn setup_fresh_user_config(openhuman_home: &Path)) and replace this block that builds cfg and calls std::fs::create_dir_all / std::fs::write with a single call to that helper; ensure the helper performs the same steps (formatting cfg, creating dirs, writing openhuman_home/config.toml and openhuman_home/users/local/config.toml) and reuse it from tests/json_rpc_e2e.rs where cfg and openhuman_home are currently used.app/test/e2e/specs/linux-cef-deb-runtime.spec.ts (1)
161-168: Include failure diagnostics in the health-check failure path.If all RPC probes fail, this throws only the boolean map. Appending
dumpAccessibilityTree()here would make CI failures much faster to debug.Suggested change
const anySuccess = Object.values(results).some(v => v); if (!anySuccess) { + const accessibilityTree = await dumpAccessibilityTree(); throw new Error( - `Expected at least one health check to pass. Results: ${JSON.stringify(results)}` + `Expected at least one health check to pass. Results: ${JSON.stringify(results)}\n` + + `Accessibility tree:\n${accessibilityTree}` ); } expect(anySuccess).toBe(true);Based on learnings: Applies to
app/test/e2e/specs/**/*.spec.ts: Assert both UI outcomes and backend/mock effects when relevant; add failure diagnostics (request logs,dumpAccessibilityTree()) for faster debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts` around lines 161 - 168, When all RPC probes fail (the anySuccess === false branch that currently throws with Results: ${JSON.stringify(results)}), add failure diagnostics by calling dumpAccessibilityTree() and including its output plus any relevant request logs or mock backend traces in the thrown Error message; locate the block that computes anySuccess from results and, before throwing, await dumpAccessibilityTree() (or capture its returned string) and append that diagnostic string and request-log details to the error text so CI failures include both the results map and accessibility/tree/request diagnostics for faster debugging.app/src/App.tsx (1)
92-95: Use the app’s namespaced debug logger instead ofconsole.debug.The redirect trace is useful, but raw console logging does not follow the frontend logging convention and is harder to filter/disable consistently.
As per coding guidelines: "Frontend debug logging should use namespaced
debugcalls with dev-only detail. Log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/App.tsx` around lines 92 - 95, Replace the raw console.debug call with the app’s namespaced debug logger (e.g., the existing debug or logger instance) and emit the same redirect trace using that API; for example call the namespaced logger (like debug('app:welcome-lock') or logger.debug) and pass a message that includes location.pathname and the target "/chat" before calling navigate('/chat', { replace: true }). Ensure the debug call is dev-only and follows the app logging pattern for namespaced debug messages.
🤖 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/pages/Accounts.tsx`:
- Around line 100-104: The component currently only forces selection to AGENT_ID
inside a useEffect (welcomeLocked && activeAccountId !== AGENT_ID ->
dispatch(setActiveAccount(AGENT_ID))), which can let the UI render once with the
stale activeAccountId; instead derive an effective selection in render (e.g.,
effectiveActiveAccountId = welcomeLocked ? AGENT_ID : activeAccountId) and use
that value everywhere in the JSX (webview/provider selection) so the UI
immediately reflects welcomeLocked, and keep the existing useEffect only if you
still want to synchronize Redux state after the paint (or remove the dispatch
there if not needed). Ensure you update references to activeAccountId in the
component to effectiveActiveAccountId and retain AGENT_ID, welcomeLocked,
dispatch, and setActiveAccount as the unique identifiers to locate the logic.
In `@app/src/pages/Conversations.tsx`:
- Around line 867-898: The welcome lockdown is bypassable because
handleSlashCommand still processes "/new" and "/clear"; update
handleSlashCommand to check the welcomeLocked flag (or equivalent state) and
early-return or show a blocked message when welcomeLocked is true, preventing it
from calling handleCreateNewThread or clearing threads; also add the same guard
where handleCreateNewThread might be invoked directly from command parsing so
both "/new" and "/clear" are ignored during lockdown and surface a user-visible
notice instead.
- Around line 215-225: The race occurs because a mount-time loadThreads()
promise can resolve after chatOnboardingCompleted flipped, causing its stale
.then(...) to reselect/create threads and clobber the unlock auto-create; fix by
introducing a bootstrapSequenceRef (or similar monotonic id) that's incremented
before calling loadThreads(), capture that id in the .then handler and bail if
it doesn't match the current bootstrapSequenceRef, and also in the useEffect
that calls handleCreateNewThread() check the same ref/guard to ensure the create
is only performed for the current bootstrap run; update references to
previousChatOnboardingCompletedRef, chatOnboardingCompleted,
handleCreateNewThread and loadThreads to use this sequence guard so stale
promises cannot override the new-thread logic.
---
Nitpick comments:
In `@app/src/App.tsx`:
- Around line 92-95: Replace the raw console.debug call with the app’s
namespaced debug logger (e.g., the existing debug or logger instance) and emit
the same redirect trace using that API; for example call the namespaced logger
(like debug('app:welcome-lock') or logger.debug) and pass a message that
includes location.pathname and the target "/chat" before calling
navigate('/chat', { replace: true }). Ensure the debug call is dev-only and
follows the app logging pattern for namespaced debug messages.
In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts`:
- Around line 161-168: When all RPC probes fail (the anySuccess === false branch
that currently throws with Results: ${JSON.stringify(results)}), add failure
diagnostics by calling dumpAccessibilityTree() and including its output plus any
relevant request logs or mock backend traces in the thrown Error message; locate
the block that computes anySuccess from results and, before throwing, await
dumpAccessibilityTree() (or capture its returned string) and append that
diagnostic string and request-log details to the error text so CI failures
include both the results map and accessibility/tree/request diagnostics for
faster debugging.
In `@tests/json_rpc_e2e.rs`:
- Around line 1361-1380: Extract the duplicated config setup into a shared test
helper (e.g., fn write_test_config(openhuman_home: &Path, cfg: &str) or fn
setup_fresh_user_config(openhuman_home: &Path)) and replace this block that
builds cfg and calls std::fs::create_dir_all / std::fs::write with a single call
to that helper; ensure the helper performs the same steps (formatting cfg,
creating dirs, writing openhuman_home/config.toml and
openhuman_home/users/local/config.toml) and reuse it from tests/json_rpc_e2e.rs
where cfg and openhuman_home are currently used.
🪄 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: 5e30437f-1fa2-44a2-a788-13dac08a6d2a
📒 Files selected for processing (13)
app/src/App.tsxapp/src/components/BottomTabBar.tsxapp/src/lib/coreState/__tests__/store.test.tsapp/src/lib/coreState/store.tsapp/src/pages/Accounts.tsxapp/src/pages/Conversations.tsxapp/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/CoreStateProvider.test.tsxapp/src/services/coreStateApi.tsapp/src/store/__tests__/socketSelectors.test.tsapp/test/e2e/specs/linux-cef-deb-runtime.spec.tssrc/openhuman/app_state/ops.rstests/json_rpc_e2e.rs
…der (tinyhumansai#883) CodeRabbit R1 (minor): the post-paint `useEffect` that dispatched `setActiveAccount(AGENT_ID)` allowed one render with the stale `activeAccountId` after a lock flip. Derive `selectedId = welcomeLocked ? AGENT_ID : (activeAccountId ?? AGENT_ID)` so the first paint already reflects the lock; keep the effect only as a Redux-state sync for other consumers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ock (tinyhumansai#883) Two gaps from CodeRabbit R1 review: 1. **Major** — the mount-time `loadThreads()` promise could resolve after the `chatOnboardingCompleted` false→true transition fired, and its `.then(...)` would re-select the old welcome thread, clobbering the fresh thread opened by the unlock handler. Add a `skipInitialThreadSelectionRef` that the unlock path sets to `true`; the pending `then` bails on it. 2. **Major** — hiding the sidebar + "+ New" button did not prevent the `handleSlashCommand` handler from creating a new thread on `/new` or `/clear`. A locked user could still type `/new` in the composer and escape. Early-return after clearing the input when `welcomeLocked` is true so the slash is consumed (not dispatched to the agent) but no thread is spawned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/pages/Conversations.tsx (1)
193-199:⚠️ Potential issue | 🟠 MajorLockdown can still be bypassed when initial thread list is empty.
Line 198 still calls
handleCreateNewThread()unconditionally. Ifdata.threads.length === 0duringwelcomeLocked, this creates a normal thread and breaks the “stay in welcome thread until complete” contract.Suggested fix
.then(data => { if (cancelled || skipInitialThreadSelectionRef.current) return; if (data.threads.length > 0) { const mostRecent = data.threads[0]; dispatch(setSelectedThread(mostRecent.id)); void dispatch(loadThreadMessages(mostRecent.id)); - } else { + } else if (!welcomeLocked) { void handleCreateNewThread(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Conversations.tsx` around lines 193 - 199, When data.threads is empty we currently call handleCreateNewThread() unconditionally which breaks the welcomeLocked contract; change the conditional so handleCreateNewThread() is only invoked when welcomeLocked is false, and when welcomeLocked is true ensure we keep/select the existing welcome thread (do not create a normal thread)—i.e., wrap the else branch with a check on welcomeLocked and either skip creating a thread or dispatch setSelectedThread(...) to the welcome thread id/state instead; update the logic around handleCreateNewThread, welcomeLocked, and setSelectedThread/loadThreadMessages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/pages/Conversations.tsx`:
- Around line 193-199: When data.threads is empty we currently call
handleCreateNewThread() unconditionally which breaks the welcomeLocked contract;
change the conditional so handleCreateNewThread() is only invoked when
welcomeLocked is false, and when welcomeLocked is true ensure we keep/select the
existing welcome thread (do not create a normal thread)—i.e., wrap the else
branch with a check on welcomeLocked and either skip creating a thread or
dispatch setSelectedThread(...) to the welcome thread id/state instead; update
the logic around handleCreateNewThread, welcomeLocked, and
setSelectedThread/loadThreadMessages accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae2f6b89-75b3-4d5e-8f64-3494a19f2888
📒 Files selected for processing (2)
app/src/pages/Accounts.tsxapp/src/pages/Conversations.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/Accounts.tsx
Summary
complete_onboarding(action: "complete").Config::chat_onboarding_completedflag throughAppStateSnapshotaschatOnboardingCompletedso the frontend can gate UI affordances on it.isWelcomeLocked(snapshot)selector and apply it at four surfaces:BottomTabBar,AppShell(routing redirect + tab-bar spacer),Accounts(left account rail + force-select Agent),Conversations(thread sidebar + "+ New" + hamburger). On unlock, automatically open a fresh thread so the user starts their first "real" conversation with the orchestrator.Problem
The onboarding overlay dismisses to
/chat, but the full app chrome (bottom tab bar, thread sidebar, account rail) is immediately visible and interactive — users can escape the welcome agent before it has run to completion. Thechat_onboarding_completedconfig flag that already tracks "the welcome agent finished" lived only in the Rust core; it was never surfaced to the React layer, so there was no signal to gate UI on.Closes #883.
Solution
Data plumbing
src/openhuman/app_state/ops.rs— addchat_onboarding_completed: booltoAppStateSnapshot, populated fromConfig::chat_onboarding_completedinsnapshot(). Serderename_all = "camelCase"emits it aschatOnboardingCompleted. Debug log gains the field for diagnostic parity.app/src/services/coreStateApi.ts+app/src/lib/coreState/store.ts+app/src/providers/CoreStateProvider.tsx— thread the new field throughAppStateSnapshotResult→CoreAppSnapshot(defaultfalseinemptySnapshot) →normalizeSnapshot/toSignedOutSnapshot.app/src/lib/coreState/store.ts— exportisWelcomeLocked(snapshot) = auth.isAuthenticated && onboardingCompleted && !chatOnboardingCompleted. One canonical rule, so every UI site reads the same condition. The auth guard prevents a signed-out first-paint flicker (OnboardingOverlayalready owns that path).UI lockdown
app/src/components/BottomTabBar.tsx—return nullwhen locked.app/src/App.tsx—AppShelldrops thepb-16tab-bar spacer when locked and adds a bootstrap-gateduseEffectthat redirects any non-/chatpathname back to/chatso hash-bar edits cannot escape the lock.app/src/pages/Accounts.tsx— hide the left account rail entirely (no Agent/account buttons, no "Add app"); a guardeduseEffectforce-selects Agent ifactiveAccountIddrifts.app/src/pages/Conversations.tsx— hide the thread sidebar, the chat-header hamburger, and the "+ New" button. On thechatOnboardingCompletedfalse → truetransition (the welcome agent just succeeded and the 2 s snapshot poll saw it), dispatchcreateNewThread()so the user lands in a fresh thread. Transition is tracked viauseRefso the poll cycle cannot re-fire the effect while the flag staystrue.Tradeoff: the unlock is polled (every 2 s via
CoreStateProvider) rather than pushed, so there is a ≤2 s visible gap betweencomplete_onboardingreturning success and the UI unlocking. A push-based event (newDomainEventvariant + subscriber) is out of scope here; the polling architecture already drives every other reactive field in the snapshot, so this matches the existing pattern and keeps the change additive.Submission Checklist
app/src/lib/coreState/__tests__/store.test.tscovers the four corners ofisWelcomeLocked(locked when both gates met; unlocked when chat onboarding done; unlocked during the wizard; unlocked when signed out). Vitest fixtures inCoreStateProvider.test.tsx+socketSelectors.test.tsupdated for the newchatOnboardingCompletedfield.tests/json_rpc_e2e.rs::json_rpc_app_state_snapshot_returns_runtime_shapeasserts both the newchatOnboardingCompletedkey and the existingonboardingCompleteddefault. Newjson_rpc_app_state_snapshot_chat_onboarding_defaults_falsecovers the fresh-user TOML path (no key → serde default → welcome lock engaged in the frontend).yarn dev:appwith a fresh~/.openhumanthat the wizard → locked/chat→ chat ≥3 exchanges → agentcomplete→ unlock + new thread path works end-to-end. Draft PR; will mark ready once smoke passes.AppStateSnapshot::chat_onboarding_completedpointing back toConfig::chat_onboarding_completedand explaining the lockdown contract; JSDoc onCoreAppSnapshot.chatOnboardingCompletedandisWelcomeLocked.[welcome-lock]prefix on the two newconsole.debuglogs (redirect + unlock transition) so the flow is greppable end-to-end; brief comments at each lockdown gate explaining the!welcomeLockedcondition.Impact
/chatroute is desktop-bound). No mobile/CLI surface.chat_onboarding_completeddefaults tofalsewhen absent fromconfig.toml. The welcome agent is idempotent (per its rustdoc: "re-running it for an already-onboarded user just produces a recognition message"), so the cost is one short conversation — then the flag flips, the UI unlocks, and normal navigation returns. Flagging this explicitly so reviewers know it is intentional.app_state_snapshotpoll, one extra bool field, no new RPC methods, no new DomainEvent variants, no new subscribers.Related
Summary by CodeRabbit
New Features
Tests