Skip to content

fix: onboarding flow - registry skills, config-based flag, dead code cleanup#177

Merged
senamakel merged 10 commits intotinyhumansai:mainfrom
senamakel:fix/onboarding-flow
Apr 1, 2026
Merged

fix: onboarding flow - registry skills, config-based flag, dead code cleanup#177
senamakel merged 10 commits intotinyhumansai:mainfrom
senamakel:fix/onboarding-flow

Conversation

@senamakel
Copy link
Copy Markdown
Member

@senamakel senamakel commented Apr 1, 2026

Summary

  • Skills step uses registry: Replaced hardcoded skill list (Telegram/Gmail/Notion/Wallet) in onboarding with useAvailableSkills() from the Rust core registry — now shows the same skills as the Skills page
  • Tools step preserved: The tools step (shell, git, files, vision, web, memory, automation) remains unchanged with its hardcoded tool definitions
  • Onboarding flag moved to config: Added onboarding_completed field to Config struct with get/set RPC handlers. Replaced the workspace flag file (skip_onboarding) and Redux-only persist with a single config.toml field as the source of truth
  • User profile fetch fix: Onboarding component now uses useUser() hook to actually fetch the user profile, fixing the bug where user._id was always null during onboarding (preventing the flag from being set)
  • Skip = complete: Both Skip and Complete write onboarding_completed = true to config — no more separate deferred flow
  • Dead code removed: Deleted SetupBanner, selectOnboardingDeferred, setOnboardingDeferredForUser, onboardingDeferredByUser, workspace flag functions, and related test code

Test plan

  • Complete onboarding flow end-to-end — verify onboarding_completed = true written to ~/.openhuman/config.toml
  • Reload app — onboarding should NOT reappear
  • Set onboarding_completed = false in config.toml, reload — onboarding SHOULD reappear
  • Click Skip — verify onboarding dismissed and flag set to true in config
  • Verify Skills step in onboarding matches Skills page content
  • Verify Tools step still shows shell/git/files/vision/web/memory/automation toggles
  • Logout and login — verify onboarding state resets properly

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added back-button navigation throughout the onboarding flow, allowing users to revisit previous steps.
  • User Interface

    • Improved progress indicator visibility with thicker segment bars.
    • Refined onboarding step layouts and updated button styling for better usability.
    • Updated onboarding completion messaging and recovery phrase setup text.
  • Removed Features

    • Removed setup banner that previously appeared on app startup.

senamakel and others added 10 commits April 1, 2026 12:22
…I improvements

- Added back navigation functionality to all onboarding steps (LocalAIStep, ScreenPermissionsStep, SkillsStep, ToolsStep, MnemonicStep) for improved user experience.
- Updated Onboarding component to adjust total steps from 7 to 6.
- Modified ProgressIndicator to reflect the change in total steps.
- Enhanced LocalAIStep UI by removing unnecessary state and improving layout.
- Improved error handling and user feedback in MnemonicStep and ScreenPermissionsStep.
- General UI refinements across onboarding steps for better consistency and usability.
… user experience

- Replaced "Set up later" button with a "Skip" button in the Onboarding component for better clarity.
- Removed unnecessary back navigation buttons from LocalAIStep to streamline the UI.
- Enhanced messaging in ScreenPermissionsStep to clarify data processing and permissions.
- Adjusted button layouts for improved accessibility and consistency across onboarding steps.
…ing steps

- Updated button styles in ScreenPermissionsStep and ToolsStep for consistency and improved user experience.
- Replaced the back navigation button in ToolsStep with a full-width continue button, enhancing layout and accessibility.
…improved consistency

- Adjusted ProgressIndicator component to use 'bg-sage-500' for active steps and increased height for better visibility.
- Streamlined button styles in LocalAIStep and SkillsStep for a more cohesive user experience, including removing unnecessary margins and ensuring full-width buttons where applicable.
- Enhanced ToolsStep to focus on skill installation, updating UI elements and descriptions for clarity and improved user engagement.
…rity

- Changed the title from "Install Skills" to "Connect Skills" to better reflect the action.
- Revised the description to clarify that skills interact with the user's workflow and that all data is processed locally, enhancing user understanding of the feature.
…nd functionality updates

- Integrated useUser hook in Onboarding component for better user state management.
- Updated MnemonicStep title and messaging for clarity, emphasizing the importance of the recovery phrase.
- Streamlined button layout in MnemonicStep and ScreenPermissionsStep for improved accessibility and consistency.
- Refactored SkillsStep to utilize available skills more effectively, enhancing the user experience with clearer skill descriptions and connection statuses.
- Transitioned ToolsStep to focus on enabling tools, updating UI elements and descriptions for better user guidance.
…ompleted config

- Removed Redux state checks for onboarding status and workspace flags.
- Integrated new core config methods to read and write onboarding completion status.
- Updated OnboardingOverlay and Onboarding components to utilize onboarding_completed for flow control.
- Enhanced error handling and user feedback during onboarding state persistence.
The onboarding_completed config field is now the sole source of truth.
Skip and complete both write the same flag. Remove SetupBanner,
onboardingDeferredByUser, selectOnboardingDeferred, and the old
workspace flag file functions (openhumanWorkspaceOnboardingFlagExists/Set).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 1, 2026

📝 Walkthrough

Walkthrough

The PR migrates onboarding state management from Redux-based deferral flags to a persistent config-based completion system. Removes SetupBanner, simplifies OnboardingOverlay with async config reads, adds backward navigation to onboarding steps, and replaces workspace-level onboarding flags with config endpoints for getting/setting completion status.

Changes

Cohort / File(s) Summary
Banner and App cleanup
app/src/App.tsx, app/src/components/SetupBanner.tsx, app/src/components/__tests__/SetupBanner.test.tsx
Removed SetupBanner component entirely and deleted its test suite, eliminating the dismissible session-based onboarding banner and Redux deferred-state interactions.
Onboarding overlay refactor
app/src/components/OnboardingOverlay.tsx
Replaced Redux-based onboarding visibility logic with async config reads via getOnboardingCompleted(); consolidated completion/defer handlers into single handleDone that persists onboardingCompleted to true.
Onboarding flow updates
app/src/pages/onboarding/Onboarding.tsx, app/src/pages/onboarding/steps/LocalAIStep.tsx, app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx, app/src/pages/onboarding/steps/SkillsStep.tsx, app/src/pages/onboarding/steps/ToolsStep.tsx, app/src/pages/onboarding/steps/MnemonicStep.tsx
Reduced steps from 7 to 6; added backward navigation via handleBack() and new onBack prop to step components; changed final completion to use setOnboardingCompleted(true) instead of workspace flag; replaced "Set up later" with fixed-position "Skip" button; reworked skills discovery and step-specific UI flows.
Progress indicator styling
app/src/components/ProgressIndicator.tsx
Updated segment height from h-0.5 to h-1 and changed active-segment range from index < currentStep to index <= currentStep with color change from bg-primary-500 to bg-sage-500.
Redux store cleanup
app/src/store/authSlice.ts, app/src/store/authSelectors.ts, app/src/store/__tests__/authSlice.test.ts, app/src/store/index.ts
Removed onboardingDeferredByUser state field, setOnboardingDeferredForUser action, selectOnboardingDeferred selector, and their persist configuration; eliminated related test assertions.
Frontend Tauri commands
app/src/utils/tauriCommands.ts
Replaced openhumanWorkspaceOnboardingFlagSet/Exists with new getOnboardingCompleted() and setOnboardingCompleted(value) that target config onboarding_completed field; removed DEFAULT_WORKSPACE_ONBOARDING_FLAG constant.
Component integration updates
app/src/components/settings/SettingsHome.tsx, app/src/providers/UserProvider.tsx
Updated imports and calls to use setOnboardingCompleted instead of workspace flag commands; adjusted associated warning messages to reference config onboarding_completed.
Backend config system
src/openhuman/config/ops.rs, src/openhuman/config/schema/types.rs, src/openhuman/config/schemas.rs
Added onboarding_completed: bool field to Config schema with false default; implemented get_onboarding_completed() and set_onboarding_completed(value) RPC handlers; registered new controller endpoints and request handlers.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 From deferred flags to config so sweet,
Onboarding dances with steps complete,
Back-skip-proceed, no banners remain,
State flows like carrots—simple, not plain! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the three main changes: registry-based skills loading, migration to config-based onboarding flag, and removal of dead code (SetupBanner, Redux selectors, workspace flags).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app/src/pages/onboarding/steps/SkillsStep.tsx (1)

112-123: ⚠️ Potential issue | 🟡 Minor

Variable name connectedIds is misleading.

connectedIds contains ALL discovered skill IDs regardless of their connection status. This misalignment propagates through onComplete to Onboarding.tsx where it's stored as connectedSources in Redux.

If the intent is to track "available" or "discovered" skills, consider renaming to availableSkillIds or discoveredSkillIds for clarity.

Suggested rename
   const handleFinish = async () => {
     setError(null);
     setLoading(true);
     try {
-      const connectedIds = sortedSkills.map(s => s.id);
-      await onComplete(connectedIds);
+      const discoveredSkillIds = sortedSkills.map(s => s.id);
+      await onComplete(discoveredSkillIds);
     } catch (e) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/steps/SkillsStep.tsx` around lines 112 - 123, The
variable connectedIds in handleFinish is misnamed because it contains all
discovered skill IDs, not only connected ones; rename connectedIds to a clearer
name (e.g., availableSkillIds or discoveredSkillIds) across the handleFinish
function and where it is passed to onComplete so the value stored as
connectedSources in Redux (in Onboarding.tsx) accurately reflects its intent;
update references to sortedSkills.map(s => s.id), the onComplete parameter
usage, and any downstream usages (connectedSources) to use the new identifier to
avoid confusion.
app/src/pages/onboarding/Onboarding.tsx (1)

103-117: ⚠️ Potential issue | 🟡 Minor

Dual persistence may lead to inconsistent state.

handleMnemonicComplete writes to both Redux (setOnboardedForUser) and core config (setOnboardingCompleted). If the config write fails but Redux succeeds, subsequent sessions will show onboarding again (config is source of truth), but Redux will incorrectly indicate onboarded.

Consider: if config persistence fails, should Redux also not be updated?

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/Onboarding.tsx` around lines 103 - 117, The handler
currently updates Redux (setOnboardedForUser) before persisting the canonical
flag (setOnboardingCompleted), which can leave Redux true while core config
remains false if persistence fails; change handleMnemonicComplete so it first
awaits setOnboardingCompleted(true) and only after that succeeds dispatch
setOnboardedForUser({ userId: user._id, value: true }) and call onComplete; if
setOnboardingCompleted throws, catch the error (capture the error object), log
it via console.warn/processLogger, do not update Redux or call onComplete, and
optionally surface an error to the UI—this ensures the core config remains the
single source of truth and Redux is consistent.
🧹 Nitpick comments (9)
src/openhuman/config/ops.rs (2)

435-451: Consider adding debug logging per coding guidelines.

The coding guidelines recommend "substantial debug logging on new/changed flows" in Rust code.

💡 Suggested logging
 pub async fn get_onboarding_completed() -> Result<RpcOutcome<bool>, String> {
     let config = load_config_with_timeout().await?;
+    log::debug!("[config] get_onboarding_completed: {}", config.onboarding_completed);
     Ok(RpcOutcome::single_log(
         config.onboarding_completed,
         "onboarding_completed read from config",
     ))
 }

 pub async fn set_onboarding_completed(value: bool) -> Result<RpcOutcome<bool>, String> {
+    log::debug!("[config] set_onboarding_completed: {}", value);
     let mut config = load_config_with_timeout().await?;
     config.onboarding_completed = value;
     config.save().await.map_err(|e| e.to_string())?;
     Ok(RpcOutcome::single_log(
         config.onboarding_completed,
         "onboarding_completed saved to config",
     ))
 }

As per coding guidelines: "Add substantial debug logging on new/changed flows using log/tracing at debug or trace level in Rust".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/config/ops.rs` around lines 435 - 451, Add debug/tracing calls
to the onboarding read/write flow: in get_onboarding_completed() log entry to
indicate the function was invoked and the value read (e.g.,
"get_onboarding_completed: read value={}") right after
load_config_with_timeout() succeeds; in set_onboarding_completed(value: bool)
log the incoming value before mutating (e.g., "set_onboarding_completed: setting
value={}"), log the config state after assignment and after successful save
(e.g., "set_onboarding_completed: saved value={}"), and log errors when
config.save().await fails. Use the project logging crate (tracing::debug or
log::debug) and place logs adjacent to the calls in get_onboarding_completed and
set_onboarding_completed so they capture input, result, and error contexts.

443-451: Load-modify-save pattern has theoretical TOCTOU race.

Concurrent calls to set_onboarding_completed could race: both load the same config, modify their copy, and the second save() overwrites the first's changes. For this boolean flag, last-write-wins is acceptable since concurrent callers typically set the same value.

Note: This pattern exists throughout ops.rs (e.g., apply_model_settings). If concurrent config mutations become a concern, consider a file lock or serializing through a single config actor.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/config/ops.rs` around lines 443 - 451, set_onboarding_completed
currently uses a load-modify-save sequence (load_config_with_timeout -> modify
config.onboarding_completed -> config.save) which can race with concurrent
callers; protect this sequence by serializing access: introduce a shared async
mutex (e.g., a static tokio::sync::Mutex or a Config actor) and acquire the lock
at the start of set_onboarding_completed, then call load_config_with_timeout,
modify the config, call config.save, and release the lock; apply the same
pattern to other functions noted (e.g., apply_model_settings) to prevent TOCTOU
overwrites.
app/src/pages/onboarding/steps/LocalAIStep.tsx (1)

22-24: Silent error suppression in fire-and-forget downloads.

The empty .catch(() => {}) blocks silently suppress download errors. Per the comment, the global snackbar tracks progress, so this may be intentional. Consider adding minimal logging for debugging purposes:

💡 Optional: Add error logging
-    void openhumanLocalAiDownload(false).catch(() => {});
-    void openhumanLocalAiDownloadAllAssets(false).catch(() => {});
+    void openhumanLocalAiDownload(false).catch(e => console.debug('[LocalAIStep] download error:', e));
+    void openhumanLocalAiDownloadAllAssets(false).catch(e => console.debug('[LocalAIStep] assets download error:', e));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/steps/LocalAIStep.tsx` around lines 22 - 24, The two
fire-and-forget calls openhumanLocalAiDownload(false) and
openhumanLocalAiDownloadAllAssets(false) currently swallow errors via .catch(()
=> {}) which hides failures; replace the empty catch handlers with minimal error
reporting (e.g., call the global snackbar or console/process logger) so failures
are surfaced for debugging while keeping the calls non-blocking—update the
.catch blocks to forward the thrown error to the existing UI logger (global
snackbar) or console with a clear message mentioning the function name.
app/src/components/OnboardingOverlay.tsx (2)

72-72: Redundant ternary expression.

Both branches produce the same result (!onboardingCompleted), making the ternary a no-op.

Proposed simplification
-  const shouldShow = DEV_FORCE_ONBOARDING ? !onboardingCompleted : !onboardingCompleted;
+  const shouldShow = !onboardingCompleted;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/components/OnboardingOverlay.tsx` at line 72, The const shouldShow
assignment uses a redundant ternary that always evaluates to
!onboardingCompleted; replace the expression with a direct assignment (const
shouldShow = !onboardingCompleted) and remove the unused DEV_FORCE_ONBOARDING
branch; update any nearby comments or dependent logic that referenced
DEV_FORCE_ONBOARDING to avoid confusion and ensure the behavior remains the same
for the onboardingCompleted check.

39-55: Add debug logging for the onboarding config read flow.

Per coding guidelines, new/changed flows should include namespaced debug logs. Consider adding logging for fetching start, result, and any state transitions.

Example logging additions
   useEffect(() => {
     if (!token || !isAuthBootstrapComplete || !userReady) return;

     let mounted = true;
     const check = async () => {
       try {
+        console.debug('[onboarding] Fetching onboarding_completed from config');
         const completed = await getOnboardingCompleted();
-        if (mounted) setOnboardingCompleted(completed);
+        if (mounted) {
+          console.debug('[onboarding] onboarding_completed =', completed);
+          setOnboardingCompleted(completed);
+        }
       } catch {
+        console.debug('[onboarding] Failed to fetch onboarding_completed, defaulting to false');
         if (mounted) setOnboardingCompleted(false);
       }
     };
     void check();

As per coding guidelines: "Add substantial debug logging on new/changed flows using namespaced debug logs in React/app code".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/components/OnboardingOverlay.tsx` around lines 39 - 55, Add
namespaced debug logging to the onboarding config read flow in the useEffect
that depends on token, isAuthBootstrapComplete, and userReady: log when the
check starts (before calling getOnboardingCompleted), log the result returned
from getOnboardingCompleted, log errors in the catch, and log key state
transitions when calling setOnboardingCompleted (including when mounted is
false). Use a consistent namespace (e.g., "OnboardingOverlay") and reference the
existing symbols getOnboardingCompleted, setOnboardingCompleted, the local
mounted flag, and the useEffect dependency variables so logs clearly show start,
success, failure, and state-update attempts.
app/src/pages/onboarding/steps/SkillsStep.tsx (2)

63-63: Unused onBack prop.

The onBack prop is destructured but aliased to _onBack and never used. If back navigation is not needed for this step, consider removing it from the props interface. If it will be used later, a TODO comment would clarify intent.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/steps/SkillsStep.tsx` at line 63, The SkillsStep
component currently destructures onBack as _onBack but never uses it; either
remove onBack from the SkillsStepProps and from the component signature
(SkillsStep) if back navigation isn't needed, or keep the prop and add a short
TODO comment inside SkillsStep explaining intended future use (and rename
_onBack back to onBack to avoid the unused alias) so the linter won't flag an
unused variable. Ensure the change updates the SkillsStepProps interface and any
callers to match.

95-110: Consider tracking skill installation state to avoid redundant downloads.

openSkillSetup calls installSkill(skill.id) unconditionally each time a skill's setup modal opens. While the backend's openhuman.skills_install is idempotent and won't fail on re-calls, it will re-download the manifest and skill files unnecessarily. Add a check to see if the skill is already installed before calling installSkill, or use a state cache to prevent redundant network requests.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/steps/SkillsStep.tsx` around lines 95 - 110,
openSkillSetup is calling installSkill(skill.id) every time which causes
redundant downloads; update openSkillSetup to first check whether the skill is
already installed (e.g., via an existing API call or a local state/cache like
installedSkills or a helper function isSkillInstalled(skill.id)) and only call
installSkill when not installed, still setting setInstalling appropriately
around the install attempt and keeping the finally branch to clear installing;
also ensure existing state updates (setActiveSkillId, setActiveSkillName,
setActiveSkillDescription, setActiveSkillHasSetup, setSetupModalOpen) happen
regardless so the modal opens even when skipping install.
app/src/pages/onboarding/Onboarding.tsx (1)

73-101: Variable name connectedSources is misleading.

Per context snippet from SkillsStep.tsx (line 116), onComplete now receives ALL discovered skill IDs, not just connected/authenticated ones. The parameter name connectedSources and the Redux field connectedSources no longer accurately describe the data.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/pages/onboarding/Onboarding.tsx` around lines 73 - 101, Rename the
misleading variable and Redux field usage for the skills step: update the
parameter name connectedSources in the handleSkillsNext function to reflect that
it contains all discovered skill IDs (e.g., discoveredSkillIds or allSkillIds)
and update the payload key in setOnboardingTasksForUser from connectedSources to
the new name so the stored onboarding tasks accurately represent the data;
ensure any references to connectedSources within handleSkillsNext (including the
setDraft call and the tasks object) are changed to the new identifier and keep
the rest of the logic (userApi.onboardingComplete, handleNext) unchanged.
app/src/utils/tauriCommands.ts (1)

839-861: Implementation is correct; consider adding debug logging.

The RpcOutcome unwrapping logic correctly handles both raw boolean and { result: boolean } envelope shapes. Per coding guidelines, new flows should include namespaced debug logs.

Example debug logging
 export async function getOnboardingCompleted(): Promise<boolean> {
   if (!isTauri()) return false;
+  console.debug('[config] getOnboardingCompleted: invoking RPC');
   const res = await callCoreRpc<boolean | { result: boolean }>({
     method: 'openhuman.config_get_onboarding_completed',
   });
   // RpcOutcome may wrap value in { result, logs } when logs are present
   if (typeof res === 'boolean') return res;
   if (res && typeof res === 'object' && 'result' in res) return res.result;
+  console.debug('[config] getOnboardingCompleted: unexpected response shape, defaulting to false');
   return false;
 }

As per coding guidelines: "Add substantial debug logging on new/changed flows".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/utils/tauriCommands.ts` around lines 839 - 861, Add namespaced debug
logging around the RPC calls in getOnboardingCompleted and
setOnboardingCompleted: log an entry message including the function name and
params (for setOnboardingCompleted), log the raw RPC response (res) immediately
after callCoreRpc returns, and log the final unwrapped boolean or the fallback
path; include an error/debug log when unexpected shapes are returned. Use the
project's existing debug/logger utility with a namespace like
"tauriCommands:getOnboardingCompleted" and
"tauriCommands:setOnboardingCompleted" so traces show which function produced
the logs.
🤖 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/onboarding/Onboarding.tsx`:
- Around line 103-117: The handler currently updates Redux (setOnboardedForUser)
before persisting the canonical flag (setOnboardingCompleted), which can leave
Redux true while core config remains false if persistence fails; change
handleMnemonicComplete so it first awaits setOnboardingCompleted(true) and only
after that succeeds dispatch setOnboardedForUser({ userId: user._id, value: true
}) and call onComplete; if setOnboardingCompleted throws, catch the error
(capture the error object), log it via console.warn/processLogger, do not update
Redux or call onComplete, and optionally surface an error to the UI—this ensures
the core config remains the single source of truth and Redux is consistent.

In `@app/src/pages/onboarding/steps/SkillsStep.tsx`:
- Around line 112-123: The variable connectedIds in handleFinish is misnamed
because it contains all discovered skill IDs, not only connected ones; rename
connectedIds to a clearer name (e.g., availableSkillIds or discoveredSkillIds)
across the handleFinish function and where it is passed to onComplete so the
value stored as connectedSources in Redux (in Onboarding.tsx) accurately
reflects its intent; update references to sortedSkills.map(s => s.id), the
onComplete parameter usage, and any downstream usages (connectedSources) to use
the new identifier to avoid confusion.

---

Nitpick comments:
In `@app/src/components/OnboardingOverlay.tsx`:
- Line 72: The const shouldShow assignment uses a redundant ternary that always
evaluates to !onboardingCompleted; replace the expression with a direct
assignment (const shouldShow = !onboardingCompleted) and remove the unused
DEV_FORCE_ONBOARDING branch; update any nearby comments or dependent logic that
referenced DEV_FORCE_ONBOARDING to avoid confusion and ensure the behavior
remains the same for the onboardingCompleted check.
- Around line 39-55: Add namespaced debug logging to the onboarding config read
flow in the useEffect that depends on token, isAuthBootstrapComplete, and
userReady: log when the check starts (before calling getOnboardingCompleted),
log the result returned from getOnboardingCompleted, log errors in the catch,
and log key state transitions when calling setOnboardingCompleted (including
when mounted is false). Use a consistent namespace (e.g., "OnboardingOverlay")
and reference the existing symbols getOnboardingCompleted,
setOnboardingCompleted, the local mounted flag, and the useEffect dependency
variables so logs clearly show start, success, failure, and state-update
attempts.

In `@app/src/pages/onboarding/Onboarding.tsx`:
- Around line 73-101: Rename the misleading variable and Redux field usage for
the skills step: update the parameter name connectedSources in the
handleSkillsNext function to reflect that it contains all discovered skill IDs
(e.g., discoveredSkillIds or allSkillIds) and update the payload key in
setOnboardingTasksForUser from connectedSources to the new name so the stored
onboarding tasks accurately represent the data; ensure any references to
connectedSources within handleSkillsNext (including the setDraft call and the
tasks object) are changed to the new identifier and keep the rest of the logic
(userApi.onboardingComplete, handleNext) unchanged.

In `@app/src/pages/onboarding/steps/LocalAIStep.tsx`:
- Around line 22-24: The two fire-and-forget calls
openhumanLocalAiDownload(false) and openhumanLocalAiDownloadAllAssets(false)
currently swallow errors via .catch(() => {}) which hides failures; replace the
empty catch handlers with minimal error reporting (e.g., call the global
snackbar or console/process logger) so failures are surfaced for debugging while
keeping the calls non-blocking—update the .catch blocks to forward the thrown
error to the existing UI logger (global snackbar) or console with a clear
message mentioning the function name.

In `@app/src/pages/onboarding/steps/SkillsStep.tsx`:
- Line 63: The SkillsStep component currently destructures onBack as _onBack but
never uses it; either remove onBack from the SkillsStepProps and from the
component signature (SkillsStep) if back navigation isn't needed, or keep the
prop and add a short TODO comment inside SkillsStep explaining intended future
use (and rename _onBack back to onBack to avoid the unused alias) so the linter
won't flag an unused variable. Ensure the change updates the SkillsStepProps
interface and any callers to match.
- Around line 95-110: openSkillSetup is calling installSkill(skill.id) every
time which causes redundant downloads; update openSkillSetup to first check
whether the skill is already installed (e.g., via an existing API call or a
local state/cache like installedSkills or a helper function
isSkillInstalled(skill.id)) and only call installSkill when not installed, still
setting setInstalling appropriately around the install attempt and keeping the
finally branch to clear installing; also ensure existing state updates
(setActiveSkillId, setActiveSkillName, setActiveSkillDescription,
setActiveSkillHasSetup, setSetupModalOpen) happen regardless so the modal opens
even when skipping install.

In `@app/src/utils/tauriCommands.ts`:
- Around line 839-861: Add namespaced debug logging around the RPC calls in
getOnboardingCompleted and setOnboardingCompleted: log an entry message
including the function name and params (for setOnboardingCompleted), log the raw
RPC response (res) immediately after callCoreRpc returns, and log the final
unwrapped boolean or the fallback path; include an error/debug log when
unexpected shapes are returned. Use the project's existing debug/logger utility
with a namespace like "tauriCommands:getOnboardingCompleted" and
"tauriCommands:setOnboardingCompleted" so traces show which function produced
the logs.

In `@src/openhuman/config/ops.rs`:
- Around line 435-451: Add debug/tracing calls to the onboarding read/write
flow: in get_onboarding_completed() log entry to indicate the function was
invoked and the value read (e.g., "get_onboarding_completed: read value={}")
right after load_config_with_timeout() succeeds; in
set_onboarding_completed(value: bool) log the incoming value before mutating
(e.g., "set_onboarding_completed: setting value={}"), log the config state after
assignment and after successful save (e.g., "set_onboarding_completed: saved
value={}"), and log errors when config.save().await fails. Use the project
logging crate (tracing::debug or log::debug) and place logs adjacent to the
calls in get_onboarding_completed and set_onboarding_completed so they capture
input, result, and error contexts.
- Around line 443-451: set_onboarding_completed currently uses a
load-modify-save sequence (load_config_with_timeout -> modify
config.onboarding_completed -> config.save) which can race with concurrent
callers; protect this sequence by serializing access: introduce a shared async
mutex (e.g., a static tokio::sync::Mutex or a Config actor) and acquire the lock
at the start of set_onboarding_completed, then call load_config_with_timeout,
modify the config, call config.save, and release the lock; apply the same
pattern to other functions noted (e.g., apply_model_settings) to prevent TOCTOU
overwrites.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 416b2ed7-c264-48a7-9bd7-81157e03788d

📥 Commits

Reviewing files that changed from the base of the PR and between 88a4647 and 5a6dd16.

📒 Files selected for processing (21)
  • app/src/App.tsx
  • app/src/components/OnboardingOverlay.tsx
  • app/src/components/ProgressIndicator.tsx
  • app/src/components/SetupBanner.tsx
  • app/src/components/__tests__/SetupBanner.test.tsx
  • app/src/components/settings/SettingsHome.tsx
  • app/src/pages/onboarding/Onboarding.tsx
  • app/src/pages/onboarding/steps/LocalAIStep.tsx
  • app/src/pages/onboarding/steps/MnemonicStep.tsx
  • app/src/pages/onboarding/steps/ScreenPermissionsStep.tsx
  • app/src/pages/onboarding/steps/SkillsStep.tsx
  • app/src/pages/onboarding/steps/ToolsStep.tsx
  • app/src/providers/UserProvider.tsx
  • app/src/store/__tests__/authSlice.test.ts
  • app/src/store/authSelectors.ts
  • app/src/store/authSlice.ts
  • app/src/store/index.ts
  • app/src/utils/tauriCommands.ts
  • src/openhuman/config/ops.rs
  • src/openhuman/config/schema/types.rs
  • src/openhuman/config/schemas.rs
💤 Files with no reviewable changes (7)
  • app/src/store/index.ts
  • app/src/store/authSelectors.ts
  • app/src/App.tsx
  • app/src/store/tests/authSlice.test.ts
  • app/src/store/authSlice.ts
  • app/src/components/SetupBanner.tsx
  • app/src/components/tests/SetupBanner.test.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant