feat: add guided onboarding #1621
Merged
Merged
Conversation
- Implement tests for NewThreadPage onboarding functionality to ensure proper rendering and interaction. - Extend PluginsSettings tests to include onboarding state management. - Enhance WelcomePage tests to cover onboarding step transitions and interactions. - Update sessionStore tests to validate onboarding progress and state management during session interactions. - Introduce logic to mark onboarding steps as complete based on user actions and session events.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements guided onboarding end-to-end: Zod contracts/routes and migrations, main-process onboarding engine and IPC, renderer client/composables/resume, GuidedOnboardingOverlay and integrations in Welcome/Settings/Chat, window focus/dev trigger, i18n, tests, and docs. ChangesGuided Onboarding end-to-end
Estimated code review effort Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/pages/NewThreadPage.vue (1)
83-130:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved merge-conflict markers in the template — build blocker, plus a behavioral regression to reconcile.
Lines 83, 105, and 130 still contain raw
<<<<<<< Updated upstream,=======, and>>>>>>> Stashed changesmarkers directly inside the<template>. This is a Vue SFC, so the template compiler will fail to parse the file and the page will not render at all. This must be resolved before merge.When you resolve the conflict, also note that the two branches are not equivalent and the “Stashed changes” block silently regresses the disabled state of the chat input:
- Upstream branch:
:submit-disabled="isAcpWorkdirUnavailable"and:send-disabled="isAcpWorkdirUnavailable || !message.trim()".- Stashed branch (kept):
:submit-disabled="isAcpWorkdirMissing"and:send-disabled="isAcpWorkdirMissing || !message.trim()".
isAcpWorkdirUnavailable(line 316) ismissing || invalid || checking, whereasisAcpWorkdirMissingis only the “missing” case. MeanwhileonSubmit(line 588),onCommandSubmit(line 605), andsubmitText(line 624) still early-return onisAcpWorkdirUnavailable.value. With the stashed predicate, the Send button and Enter-to-submit appear enabled while the workspace path is invalid or still being validated, and the submission silently no-ops — confusing UX.🛠️ Suggested resolution (keep the new guide host, restore `isAcpWorkdirUnavailable`)
<!-- Input area --> - <<<<<<< Updated upstream - <ChatInputBox - ref="chatInputRef" - v-model="message" - :files="attachedFiles" - :session-id="acpDraftSessionId" - :workspace-path="projectStore.selectedProject?.path ?? null" - :is-acp-session="isAcpSelectedAgent" - :submit-disabled="isAcpWorkdirUnavailable" - `@update`:files="onFilesChange" - `@pending-skills-change`="onPendingSkillsChange" - `@command-submit`="onCommandSubmit" - `@submit`="onSubmit" - > - <template `#toolbar`> - <ChatInputToolbar - :send-disabled="isAcpWorkdirUnavailable || !message.trim()" - `@attach`="onAttach" - `@send`="onSubmit" - /> - </template> - </ChatInputBox> - ======= - <div ref="firstChatGuideHostRef" :class="['w-full max-w-4xl flex justify-center']"> + <div ref="firstChatGuideHostRef" class="w-full max-w-4xl flex justify-center"> <ChatInputBox :class="activeChatGuide?.key === 'first-chat' ? 'relative z-30 rounded-2xl' : ''" ref="chatInputRef" v-model="message" :files="attachedFiles" :session-id="acpDraftSessionId" :workspace-path="projectStore.selectedProject?.path ?? null" :is-acp-session="isAcpSelectedAgent" - :submit-disabled="isAcpWorkdirMissing" + :submit-disabled="isAcpWorkdirUnavailable" `@update`:files="onFilesChange" `@pending-skills-change`="onPendingSkillsChange" `@command-submit`="onCommandSubmit" `@submit`="onSubmit" > <template `#toolbar`> <ChatInputToolbar - :send-disabled="isAcpWorkdirMissing || !message.trim()" + :send-disabled="isAcpWorkdirUnavailable || !message.trim()" `@attach`="onAttach" `@send`="onSubmit" /> </template> </ChatInputBox> </div> - >>>>>>> Stashed changes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/pages/NewThreadPage.vue` around lines 83 - 130, Remove the raw merge markers and adopt the "Stashed changes" layout (keep the outer div with ref="firstChatGuideHostRef" and the conditional class on ChatInputBox) but restore the upstream disabled predicate everywhere to avoid the regression: replace any use of isAcpWorkdirMissing with isAcpWorkdirUnavailable in the ChatInputBox props and ChatInputToolbar props, and ensure onSubmit, onCommandSubmit and submitText continue to check isAcpWorkdirUnavailable; then delete the <<<<<<<, =======, >>>>>>> lines so the Vue template parses.
🧹 Nitpick comments (6)
docs/issues/guided-onboarding-first-chat-confirm/spec.md (1)
19-21: ⚡ Quick winConsider varying sentence structure for better readability.
Three successive sentences begin with "No change to," which creates a repetitive pattern. Consider rephrasing for variety.
✍️ Suggested rephrasing
- No change to onboarding completion semantics for the first-chat step. -- No change to chat send behavior. -- No change to the final guide copy. +- Chat send behavior remains unchanged. +- The final guide copy stays as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/issues/guided-onboarding-first-chat-confirm/spec.md` around lines 19 - 21, The three bullet lines in the spec use the repetitive leading phrase "No change to," hurting readability; revise them to vary sentence structure while preserving meaning—e.g., change one to "Onboarding completion semantics remain unchanged for the first-chat step," another to "Chat send behavior will not be modified," and the last to "The final guide copy stays the same," so that the items still convey no functional changes but avoid repetitive phrasing; update the three lines in the document accordingly.test/renderer/stores/sessionStore.test.ts (1)
139-146: 💤 Low valueSimplify redundant ternary operator.
Both branches of the ternary return
'completed' as const, making the conditional unnecessary. This appears to be either a copy-paste artifact or incomplete logic.♻️ Proposed simplification
{ id: 'switch-model', required: true, - status: stepId === 'switch-model' ? ('completed' as const) : ('completed' as const), + status: 'completed' as const, startedAt: 1, completedAt: 2, skippedAt: null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/stores/sessionStore.test.ts` around lines 139 - 146, The ternary setting status in the 'switch-model' step is redundant (both branches return ('completed' as const)); replace "status: stepId === 'switch-model' ? ('completed' as const) : ('completed' as const)" with a single direct assignment "status: ('completed' as const)" in the object for id 'switch-model' (reference the test variable stepId and the object with id 'switch-model' in sessionStore.test.ts).test/renderer/components/WelcomePage.test.ts (1)
41-120: ⚡ Quick winConsider extracting the repeated OnboardingClient mock setup.
The OnboardingClient mock configuration (~115 lines with 8 steps) is duplicated across 5 tests. This creates maintenance overhead: any change to the step structure or state shape requires updates in multiple locations.
Consider extracting a helper factory function that accepts parameters for the specific test scenario (e.g.,
currentStepId,completedSteps) and returns the appropriate mock configuration.♻️ Example refactor approach
const createOnboardingClientMock = (currentStepId: string, options = {}) => ({ createOnboardingClient: vi.fn(() => ({ getState: vi.fn().mockResolvedValue({ version: 4, status: 'active', startedAt: 1, completedAt: null, lastActiveAt: 1, currentStepId, steps: createStepsForCurrentStep(currentStepId, options) }), start: vi.fn().mockResolvedValue({ status: 'active', currentStepId }), setStepStatus: vi.fn().mockResolvedValue({ status: 'active', currentStepId }) })) })Also applies to: 220-299, 392-471, 556-618, 699-778
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/WelcomePage.test.ts` around lines 41 - 120, The tests duplicate a large OnboardingClient mock; extract a reusable factory like createOnboardingClientMock(currentStepId, options) that returns an object implementing createOnboardingClient with getState, start and setStepStatus mocks; update each test to vi.doMock('@api/OnboardingClient', () => createOnboardingClientMock(...)) and implement a helper (and a createStepsForCurrentStep) to produce the steps array based on currentStepId/completedSteps so tests only supply scenario differences while reusing the shared shape and behavior.src/main/routes/onboarding/onboardingRouteSupport.ts (1)
151-250: ⚡ Quick winCollapse the four near-identical
normalize*StepStatehelpers.
normalizeStepState,normalizeLegacyStepState,normalizePreviousStepState, andnormalizeVersion3StepStateare structurally identical: same status validation, same inlinetimestampOrNull, same returned shape (the only deltas are theidtype and whetherrequiredis emitted). HoistingtimestampOrNulland the status validator to module scope and parameterizing one helper would remove ~80 lines, eliminate four redeclarations oftimestampOrNullperreadGuidedOnboardingStatecall, and make future status additions a one-line change. Same pattern repeats for thenextStateBaseconstruction acrossmigratePreviousState/migrateVersion3State/migrateLegacyState/normalizeState.♻️ Sketch
const ONBOARDING_STATUSES = ['pending', 'in_progress', 'completed', 'skipped'] as const type OnboardingStatus = (typeof ONBOARDING_STATUSES)[number] const toTimestampOrNull = (value: unknown): number | null => typeof value === 'number' && Number.isFinite(value) && value >= 0 ? value : null const toOnboardingStatus = (value: unknown, fallback: OnboardingStatus): OnboardingStatus => ONBOARDING_STATUSES.includes(value as OnboardingStatus) ? (value as OnboardingStatus) : fallback const normalizeStepCommon = <TId extends string>( id: TId, stored: Partial<GuidedOnboardingStepState> | undefined, fallbackStatus: OnboardingStatus = 'pending' ) => ({ id, status: toOnboardingStatus(stored?.status, fallbackStatus), startedAt: toTimestampOrNull(stored?.startedAt), completedAt: toTimestampOrNull(stored?.completedAt), skippedAt: toTimestampOrNull(stored?.skippedAt) })The current-version variant just adds
required: isGuidedOnboardingRequiredStepId(id).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/routes/onboarding/onboardingRouteSupport.ts` around lines 151 - 250, Collapse the four near-identical helpers by hoisting the timestamp validator and status validator to module scope (e.g., toTimestampOrNull and ONBOARDING_STATUSES/toOnboardingStatus) and implement a single generic helper like normalizeStepCommon<TId>(id, stored, fallbackStatus?) that returns { id, status, startedAt, completedAt, skippedAt }; then replace normalizeStepState/normalizeLegacyStepState/normalizePreviousStepState/normalizeVersion3StepState with thin wrappers that call normalizeStepCommon (normalizeStepState should add required: isGuidedOnboardingRequiredStepId(id) to the returned object, and normalizePreviousStepState should compute its fallback id for 'open-settings' before calling the common helper). Update any callers to use the new wrappers.src/renderer/src/composables/useGuidedOnboardingStep.ts (1)
78-93: 💤 Low valueClarify intent of resetting
dismissedafter step status changes.Lines 86, 98, and 126 reset
dismissed.value = falseafter completing/skipping/activating steps. This means if a user dismisses the guide and then a step completes programmatically, the guide reappears. Consider whether this is the intended behavior or if it should remain dismissed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/composables/useGuidedOnboardingStep.ts` around lines 78 - 93, The code currently resets dismissed.value = false inside setStepStatus (and similarly in the other step handlers), causing a guide to reappear when steps change; instead, avoid unconditionally flipping dismissed to false—either remove those assignments or add an explicit option/flag (e.g., preserveDismissed: boolean) to setStepStatus/activate/complete handlers so callers can decide when to clear dismissal; update setStepStatus (and the other methods that set dismissed.value) to respect the new flag or to only clear dismissed when transitioning from an initial/never-shown state, and update any callers to pass the appropriate flag.src/renderer/src/lib/onboardingResume.ts (1)
62-93: 💤 Low valueConsider logging storage failures for debugging.
Lines 78-80 and 90-92 silently ignore storage write/clear failures. While graceful failure is appropriate for non-persistent environments, logging these failures would help diagnose issues in development.
Optional logging enhancement
} catch { - // Ignore storage write failures in non-persistent environments. + console.debug('[OnboardingResume] Failed to persist resume intent - likely non-persistent environment') }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/lib/onboardingResume.ts` around lines 62 - 93, The try/catch blocks in persistGuidedOnboardingResumeIntent and clearGuidedOnboardingResumeIntent currently swallow errors; update both catches to log the error for debugging (e.g., catch (err) { console.error('Failed to persist guided onboarding resume intent', { err, input }) } in persistGuidedOnboardingResumeIntent and catch (err) { console.error('Failed to clear guided onboarding resume intent', err) } in clearGuidedOnboardingResumeIntent) so failures are visible during development while still not throwing in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/presenter/windowPresenter/index.ts`:
- Around line 1392-1409: The focusMainWindow method currently returns false when
this.mainWindowId points to a dead window; update it to recover before failing
by attempting to resolve a live window via
BrowserWindow.fromId(this.mainWindowId) and, if that yields null/destroyed, fall
back to BrowserWindow.getAllWindows() (or another preferred selection) to find a
live BrowserWindow, update this.mainWindowId to that window's id, and then
proceed to restore/show/focus it; ensure you still check isDestroyed() and
webContents.isDestroyed() before calling restore/show/focus and only return
false if no live window can be found.
In `@src/main/routes/onboarding/onboardingRouteSupport.ts`:
- Around line 808-844: The code currently honors options.stepId even if that
step is already completed/skipped, which leaves currentStepId pointing at a
terminal step; change the nextStepId resolution so that if the requestedStepId
(from options.stepId / SHARED_GUIDED_ONBOARDING_STEP_IDS) corresponds to a step
in baseState whose status is 'completed' or 'skipped', ignore it and fall back
to findNextPendingStepId(baseState) (or set requestedStepId to undefined) before
building steps and persisting; update the logic around
requestedStepId/nextStepId and the mapping that sets step.status/startedAt so
only non-terminal requested steps are promoted to 'in_progress' and
currentStepId never points at a finished step when activating the onboarding via
persistState.
In `@src/renderer/api/OnboardingClient.ts`:
- Around line 13-16: The methods (getState, start, setStepStatus, complete,
reset) assume bridge.invoke returns a valid shape and directly access
result.state (and other fields); add robust error handling by wrapping each
bridge.invoke call in try/catch, validate the response with simple type guards
(e.g., check result is an object and has the expected keys and types such as
typeof result.state === 'string' or Array.isArray for lists), and either throw a
descriptive Error or return a safe default when the shape is invalid; update the
referenced functions (getState and the start/setStepStatus/complete/reset
methods) to perform these runtime checks and include the route name (e.g.,
onboardingGetStateRoute.name) in error messages for easier debugging.
In `@src/renderer/settings/components/ModelProviderSettingsDetail.vue`:
- Around line 344-366: The watcher on props.provider.id unconditionally sets
activeTab.value = 'connection', which hides the models tab when
props.activeOnboardingStepId remains 'provider-model'; change the watcher in
ModelProviderSettingsDetail.vue so it only sets activeTab to 'connection' when
props.activeOnboardingStepId !== 'provider-model' (or when there is no
onboarding step), but always call initProviderSettings(); reference activeTab,
initProviderSettings, props.provider.id and props.activeOnboardingStepId to
locate and update the watcher logic.
In `@src/renderer/src/composables/useGuidedOnboardingStep.ts`:
- Around line 50-60: The finalization condition in finalizeIfNeeded incorrectly
only checks currentStepId === null but the computed property can also be
undefined; update the condition in finalizeIfNeeded (which accepts
GuidedOnboardingState) to treat both null and undefined as "no current step"
(e.g., use currentStepId == null or explicit checks for null || undefined)
before calling onboardingClient.complete(); keep the existing try/catch that
assigns onboardingState.value and logs stepId on error.
In `@src/renderer/src/i18n/fa-IR/chat.json`:
- Around line 355-368: The onboarding strings under keys
onboarding.agentSwitch.title, onboarding.agentSwitch.description,
onboarding.agentSwitch.caption, onboarding.switchModel.description,
onboarding.switchModel.caption, onboarding.firstChat.description, and
onboarding.firstChat.caption are still in English; replace each English value
with appropriate Persian translations so the fa-IR locale is fully localized
(ensure you preserve JSON string quoting and commas and keep the same key
names).
In `@src/renderer/src/i18n/fr-FR/chat.json`:
- Around line 355-368: The onboarding strings in the fr-FR locale are still in
English; update the values under the "onboarding" object—specifically
onboarding.agentSwitch.title, onboarding.agentSwitch.description,
onboarding.agentSwitch.caption, onboarding.switchModel.description,
onboarding.switchModel.caption, and onboarding.firstChat.description and
onboarding.firstChat.caption—to proper French translations so the French UI
shows consistent copy (replace the English sentences with appropriate French
equivalents).
In `@src/renderer/src/i18n/he-IL/chat.json`:
- Around line 355-368: The onboarding strings under the "onboarding" object
("agentSwitch", "switchModel", "firstChat") in chat.json are still in English;
replace the English values for "title", "description", and "caption" (for
agentSwitch) and the "description" and "caption" values (for switchModel and
firstChat) with their correct Hebrew translations so the he-IL locale is fully
localized; locate the "onboarding" object and update those specific keys' string
values accordingly.
In `@src/renderer/src/i18n/ja-JP/chat.json`:
- Around line 355-369: The onboarding section in the ja-JP locale (keys
onboarding.agentSwitch, onboarding.switchModel, onboarding.firstChat and their
title/description/caption fields) is still in English; replace each English
string with the correct Japanese translations for title, description, and
caption for agentSwitch, switchModel, and firstChat so the onboarding flow is
fully localized (update the values in src/renderer/src/i18n/ja-JP/chat.json
under the onboarding object).
In `@src/renderer/src/i18n/ko-KR/chat.json`:
- Around line 355-369: The onboarding section keys (onboarding.agentSwitch,
onboarding.switchModel, onboarding.firstChat) currently contain English strings;
replace each value with proper Korean translations for title, description, and
caption so the Korean locale displays localized UI text during onboarding—update
the "title", "description", and "caption" fields under agentSwitch, switchModel,
and firstChat with the provided Korean translations (or equivalent accurate
Korean phrasing) while keeping the JSON keys unchanged.
In `@src/renderer/src/i18n/pt-BR/chat.json`:
- Around line 355-369: The onboarding strings under the JSON keys
onboarding.agentSwitch.title, onboarding.agentSwitch.description,
onboarding.agentSwitch.caption, onboarding.switchModel.description,
onboarding.switchModel.caption, and onboarding.firstChat.description/caption are
currently in English; replace each value with the correct Portuguese
translations (e.g., "Switch to DeepChat Agent" → "Mudar para o Agente DeepChat",
"Before chatting, switch away from ACP and return to the built-in DeepChat
flow." → "Antes de conversar, saia do ACP e volte para o fluxo integrado do
DeepChat.", "Click the highlighted agent button in the sidebar to continue." →
"Clique no botão do agente destacado na barra lateral para continuar.", and
similarly translate the switchModel and firstChat description/caption strings)
so the file provides full Portuguese localization for the onboarding flow.
In `@src/renderer/src/i18n/ru-RU/chat.json`:
- Around line 355-369: The onboarding strings under keys
onboarding.agentSwitch.*, onboarding.switchModel.*, and onboarding.firstChat.*
are still in English—replace the English values with appropriate Russian
translations for title, description, and caption to restore localization; update
onboarding.agentSwitch.title, onboarding.agentSwitch.description,
onboarding.agentSwitch.caption, onboarding.switchModel.description,
onboarding.switchModel.caption, onboarding.firstChat.description, and
onboarding.firstChat.caption with the provided Russian text so the ru-RU
chat.json contains Russian guidance for onboarding.
In `@src/renderer/src/pages/WelcomePage.vue`:
- Line 7: The Tailwind z-index utilities z-70 and z-72 used by WelcomePage.vue
("z-70") and the ChatStatusBar popover ("z-72") are not defined; add them by
extending the CSS variables in the `@theme` inline block of
src/renderer/src/assets/style.css (add --z-70: 70; and --z-72: 72;) so the
classes resolve, or alternatively replace usages of z-70 and z-72 with Tailwind
arbitrary values z-[70] and z-[72] in the components to avoid needing custom
utilities.
In `@src/shared/guidedOnboarding.ts`:
- Around line 172-188: resolveCurrentGuidedOnboardingStepId currently only falls
back to a step with status === 'pending' when state.currentStepId is null;
update the fallback to prefer an 'in_progress' step before 'pending' so resume
picks up a partially completed step. In the function
resolveCurrentGuidedOnboardingStepId, when state?.currentStepId is falsy, search
state.steps first for step.status === 'in_progress' and return its id if found,
otherwise fallback to step.status === 'pending' (and then null if neither
exists).
---
Outside diff comments:
In `@src/renderer/src/pages/NewThreadPage.vue`:
- Around line 83-130: Remove the raw merge markers and adopt the "Stashed
changes" layout (keep the outer div with ref="firstChatGuideHostRef" and the
conditional class on ChatInputBox) but restore the upstream disabled predicate
everywhere to avoid the regression: replace any use of isAcpWorkdirMissing with
isAcpWorkdirUnavailable in the ChatInputBox props and ChatInputToolbar props,
and ensure onSubmit, onCommandSubmit and submitText continue to check
isAcpWorkdirUnavailable; then delete the <<<<<<<, =======, >>>>>>> lines so the
Vue template parses.
---
Nitpick comments:
In `@docs/issues/guided-onboarding-first-chat-confirm/spec.md`:
- Around line 19-21: The three bullet lines in the spec use the repetitive
leading phrase "No change to," hurting readability; revise them to vary sentence
structure while preserving meaning—e.g., change one to "Onboarding completion
semantics remain unchanged for the first-chat step," another to "Chat send
behavior will not be modified," and the last to "The final guide copy stays the
same," so that the items still convey no functional changes but avoid repetitive
phrasing; update the three lines in the document accordingly.
In `@src/main/routes/onboarding/onboardingRouteSupport.ts`:
- Around line 151-250: Collapse the four near-identical helpers by hoisting the
timestamp validator and status validator to module scope (e.g.,
toTimestampOrNull and ONBOARDING_STATUSES/toOnboardingStatus) and implement a
single generic helper like normalizeStepCommon<TId>(id, stored, fallbackStatus?)
that returns { id, status, startedAt, completedAt, skippedAt }; then replace
normalizeStepState/normalizeLegacyStepState/normalizePreviousStepState/normalizeVersion3StepState
with thin wrappers that call normalizeStepCommon (normalizeStepState should add
required: isGuidedOnboardingRequiredStepId(id) to the returned object, and
normalizePreviousStepState should compute its fallback id for 'open-settings'
before calling the common helper). Update any callers to use the new wrappers.
In `@src/renderer/src/composables/useGuidedOnboardingStep.ts`:
- Around line 78-93: The code currently resets dismissed.value = false inside
setStepStatus (and similarly in the other step handlers), causing a guide to
reappear when steps change; instead, avoid unconditionally flipping dismissed to
false—either remove those assignments or add an explicit option/flag (e.g.,
preserveDismissed: boolean) to setStepStatus/activate/complete handlers so
callers can decide when to clear dismissal; update setStepStatus (and the other
methods that set dismissed.value) to respect the new flag or to only clear
dismissed when transitioning from an initial/never-shown state, and update any
callers to pass the appropriate flag.
In `@src/renderer/src/lib/onboardingResume.ts`:
- Around line 62-93: The try/catch blocks in persistGuidedOnboardingResumeIntent
and clearGuidedOnboardingResumeIntent currently swallow errors; update both
catches to log the error for debugging (e.g., catch (err) {
console.error('Failed to persist guided onboarding resume intent', { err, input
}) } in persistGuidedOnboardingResumeIntent and catch (err) {
console.error('Failed to clear guided onboarding resume intent', err) } in
clearGuidedOnboardingResumeIntent) so failures are visible during development
while still not throwing in production.
In `@test/renderer/components/WelcomePage.test.ts`:
- Around line 41-120: The tests duplicate a large OnboardingClient mock; extract
a reusable factory like createOnboardingClientMock(currentStepId, options) that
returns an object implementing createOnboardingClient with getState, start and
setStepStatus mocks; update each test to vi.doMock('@api/OnboardingClient', ()
=> createOnboardingClientMock(...)) and implement a helper (and a
createStepsForCurrentStep) to produce the steps array based on
currentStepId/completedSteps so tests only supply scenario differences while
reusing the shared shape and behavior.
In `@test/renderer/stores/sessionStore.test.ts`:
- Around line 139-146: The ternary setting status in the 'switch-model' step is
redundant (both branches return ('completed' as const)); replace "status: stepId
=== 'switch-model' ? ('completed' as const) : ('completed' as const)" with a
single direct assignment "status: ('completed' as const)" in the object for id
'switch-model' (reference the test variable stepId and the object with id
'switch-model' in sessionStore.test.ts).
🪄 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: e008c963-2b75-4588-8a01-4f3c97799d8d
📒 Files selected for processing (81)
docs/issues/guided-onboarding-first-chat-confirm/plan.mddocs/issues/guided-onboarding-first-chat-confirm/spec.mddocs/issues/guided-onboarding-first-chat-confirm/tasks.mdsrc/main/events.tssrc/main/presenter/windowPresenter/index.tssrc/main/routes/index.tssrc/main/routes/onboarding/onboardingRouteSupport.tssrc/renderer/api/OnboardingClient.tssrc/renderer/settings/components/AboutUsSettings.vuesrc/renderer/settings/components/BedrockProviderSettingsDetail.vuesrc/renderer/settings/components/McpSettings.vuesrc/renderer/settings/components/ModelProviderSettings.vuesrc/renderer/settings/components/ModelProviderSettingsDetail.vuesrc/renderer/settings/components/OllamaProviderSettingsDetail.vuesrc/renderer/settings/components/ProviderApiConfig.vuesrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/settings/lib/guidedOnboardingSettings.tssrc/renderer/src/App.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/onboarding/GuidedOnboardingOverlay.vuesrc/renderer/src/components/settings/ModelConfigItem.vuesrc/renderer/src/composables/useAppIpcRuntime.tssrc/renderer/src/composables/useGuidedOnboardingStep.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/about.jsonsrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/welcome.jsonsrc/renderer/src/i18n/en-US/about.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/welcome.jsonsrc/renderer/src/i18n/fa-IR/about.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/welcome.jsonsrc/renderer/src/i18n/fr-FR/about.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/welcome.jsonsrc/renderer/src/i18n/he-IL/about.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/welcome.jsonsrc/renderer/src/i18n/ja-JP/about.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/welcome.jsonsrc/renderer/src/i18n/ko-KR/about.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/welcome.jsonsrc/renderer/src/i18n/pt-BR/about.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/welcome.jsonsrc/renderer/src/i18n/ru-RU/about.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/welcome.jsonsrc/renderer/src/i18n/zh-CN/about.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/welcome.jsonsrc/renderer/src/i18n/zh-HK/about.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/welcome.jsonsrc/renderer/src/i18n/zh-TW/about.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/welcome.jsonsrc/renderer/src/lib/onboardingResume.tssrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/pages/WelcomePage.vuesrc/renderer/src/stores/ui/session.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/onboarding.routes.tssrc/shared/guidedOnboarding.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/routes/onboarding/onboardingRouteSupport.test.tstest/renderer/components/AboutUsSettings.test.tstest/renderer/components/App.startup.test.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/components/McpSettings.test.tstest/renderer/components/ModelProviderSettings.test.tstest/renderer/components/ModelProviderSettingsDetail.test.tstest/renderer/components/NewThreadPage.onboarding.test.tstest/renderer/components/PluginsSettings.test.tstest/renderer/components/WelcomePage.test.tstest/renderer/stores/sessionStore.test.ts
| v-if="showGuideCoachmark" | ||
| data-testid="welcome-guide-coachmark" | ||
| :data-guide-target="coachmarkTargetSurface" | ||
| class="fixed inset-0 z-70" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for explicit zIndex extensions in any Tailwind config and confirm 70 / 72 are defined.
fd -t f -e ts -e js -e cjs -e mjs 'tailwind.config' | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'
# Also surface every place in the repo that uses these utilities, to know the actual layering audience.
rg -nP '\bz-(70|72)\b'Repository: ThinkInAIXYZ/deepchat
Length of output: 381
🏁 Script executed:
#!/bin/bash
# Search for tailwind config with broader patterns
echo "=== Searching for tailwind config files (all patterns) ==="
find . -maxdepth 3 -type f \( -name '*tailwind*' -o -name 'tailwind*' \) ! -path './node_modules/*' ! -path './.git/*'
echo -e "\n=== Checking package.json for tailwind config hints ==="
if [ -f package.json ]; then
cat package.json | grep -A 20 -B 5 'tailwind'
fi
echo -e "\n=== Checking vite/build config references ==="
find . -maxdepth 2 -type f \( -name 'vite.config.*' -o -name 'postcss.config.*' \) ! -path './node_modules/*'
echo -e "\n=== Actual class attribute syntax in the files using z-70/z-72 ==="
echo "WelcomePage.vue line 7:"
sed -n '5,10p' src/renderer/src/pages/WelcomePage.vue
echo -e "\nGuidedOnboardingOverlay.vue line 5:"
sed -n '3,8p' src/renderer/src/components/onboarding/GuidedOnboardingOverlay.vue
echo -e "\nChatStatusBar.vue around line 118:"
sed -n '115,122p' src/renderer/src/components/chat/ChatStatusBar.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 3413
🏁 Script executed:
#!/bin/bash
echo "=== Searching for vite.config files ==="
find . -maxdepth 2 -type f -name 'vite.config.*' ! -path './node_modules/*'
echo -e "\n=== Searching for CSS files with Tailwind `@layer` directives ==="
find ./src -type f \( -name '*.css' -o -name '*.scss' \) | head -20
echo -e "\n=== Check main CSS entry file for Tailwind config ==="
# Common entry points for tailwind CSS in Vue apps
for file in src/main.css src/index.css src/style.css src/renderer/src/main.css src/renderer/src/style.css src/renderer/src/index.css; do
if [ -f "$file" ]; then
echo "=== $file ==="
cat "$file"
fi
done
echo -e "\n=== Search any file for theme.extend.zIndex or z-index config ==="
rg -i 'theme.*extend.*zindex|z.?index.*70|z.?index.*72' --type ts --type js --type cssRepository: ThinkInAIXYZ/deepchat
Length of output: 50379
🏁 Script executed:
#!/bin/bash
echo "=== Content of src/renderer/src/assets/main.css ==="
cat src/renderer/src/assets/main.css
echo -e "\n=== Content of src/renderer/src/assets/style.css ==="
cat src/renderer/src/assets/style.css
echo -e "\n=== Search for vite.config in all directories ==="
find . -name 'vite.config*' ! -path './node_modules/*' 2>/dev/null
echo -e "\n=== Search for postcss.config ==="
find . -name 'postcss.config*' ! -path './node_modules/*' 2>/dev/null
echo -e "\n=== Check if z-70 or z-72 appear in any CSS files ==="
rg 'z-70|z-72' --type cssRepository: ThinkInAIXYZ/deepchat
Length of output: 34745
Fix missing z-70 and z-72 z-index utilities in Tailwind configuration.
The coachmark overlay (z-70) and ChatStatusBar popover (z-72) rely on z-index utilities that are not defined in the project's Tailwind configuration. These classes are not generated and will be silent no-ops, breaking the visual layering of these elements.
Add the missing zIndex extension to src/renderer/src/assets/style.css within the @theme inline block:
--z-70: 70;
--z-72: 72;Or use Tailwind 4's arbitrary value syntax (z-[70], z-[72]) instead of custom utilities.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/pages/WelcomePage.vue` at line 7, The Tailwind z-index
utilities z-70 and z-72 used by WelcomePage.vue ("z-70") and the ChatStatusBar
popover ("z-72") are not defined; add them by extending the CSS variables in the
`@theme` inline block of src/renderer/src/assets/style.css (add --z-70: 70; and
--z-72: 72;) so the classes resolve, or alternatively replace usages of z-70 and
z-72 with Tailwind arbitrary values z-[70] and z-[72] in the components to avoid
needing custom utilities.
Contributor
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)
src/main/routes/onboarding/onboardingRouteSupport.ts (1)
1-984:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Oxfmt violations blocking CI.
The formatting check is failing for this file in CI. Please run the formatter and commit the result before merge.
As per coding guidelines,
src/**/*.{ts,tsx,js,jsx,vue}must use Oxfmt formatting and runpnpm run format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/routes/onboarding/onboardingRouteSupport.ts` around lines 1 - 984, The file fails Oxfmt formatting; run the project formatter and commit changes to fix CI: run the repo formatter (pnpm run format) or your configured Oxfmt command, reformat this file (e.g., the onboarding utilities around functions like createDefaultState, normalizeState, startGuidedOnboarding, and completeGuidedOnboarding), verify the diff only contains formatting changes, and commit/ push the formatted file before merging.
🧹 Nitpick comments (1)
test/renderer/api/OnboardingClient.test.ts (1)
6-8: ⚡ Quick winAvoid hardcoding onboarding schema version in fixture.
Using a literal version here can cause unrelated failures when the shared contract version increments. Import the shared version constant instead.
♻️ Proposed change
import { createOnboardingClient } from '../../../src/renderer/api/OnboardingClient' -import type { GuidedOnboardingState } from '@shared/contracts/routes' +import { + guidedOnboardingVersion, + type GuidedOnboardingState +} from '@shared/contracts/routes' const createState = (): GuidedOnboardingState => ({ - version: 4, + version: guidedOnboardingVersion,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/api/OnboardingClient.test.ts` around lines 6 - 8, The fixture createState function hardcodes the onboarding schema version (version: 4); replace this literal with the shared version constant from the onboarding contract (import the constant and use it as the value) so the test uses the canonical GuidedOnboarding version; update the createState function (and any related GuidedOnboardingState usage) to reference that imported constant instead of the numeric literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/routes/onboarding/onboardingRouteSupport.ts`:
- Around line 1-984: The file fails Oxfmt formatting; run the project formatter
and commit changes to fix CI: run the repo formatter (pnpm run format) or your
configured Oxfmt command, reformat this file (e.g., the onboarding utilities
around functions like createDefaultState, normalizeState, startGuidedOnboarding,
and completeGuidedOnboarding), verify the diff only contains formatting changes,
and commit/ push the formatted file before merging.
---
Nitpick comments:
In `@test/renderer/api/OnboardingClient.test.ts`:
- Around line 6-8: The fixture createState function hardcodes the onboarding
schema version (version: 4); replace this literal with the shared version
constant from the onboarding contract (import the constant and use it as the
value) so the test uses the canonical GuidedOnboarding version; update the
createState function (and any related GuidedOnboardingState usage) to reference
that imported constant instead of the numeric literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3b8bef3-8334-4b08-a840-99cd27b0e2f3
📒 Files selected for processing (18)
docs/issues/pr-1621-ai-review-fixes/plan.mddocs/issues/pr-1621-ai-review-fixes/spec.mddocs/issues/pr-1621-ai-review-fixes/tasks.mdsrc/main/routes/onboarding/onboardingRouteSupport.tssrc/renderer/api/OnboardingClient.tssrc/renderer/settings/components/ModelProviderSettingsDetail.vuesrc/renderer/src/composables/useGuidedOnboardingStep.tssrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/shared/guidedOnboarding.tstest/main/routes/onboarding/onboardingRouteSupport.test.tstest/main/shared/guidedOnboarding.test.tstest/renderer/api/OnboardingClient.test.ts
✅ Files skipped from review due to trivial changes (7)
- docs/issues/pr-1621-ai-review-fixes/plan.md
- src/renderer/src/i18n/fa-IR/chat.json
- src/renderer/src/i18n/ja-JP/chat.json
- src/renderer/src/i18n/fr-FR/chat.json
- src/renderer/src/i18n/ko-KR/chat.json
- src/renderer/src/i18n/he-IL/chat.json
- src/renderer/src/i18n/ru-RU/chat.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/src/i18n/pt-BR/chat.json
- test/main/routes/onboarding/onboardingRouteSupport.test.ts
- src/shared/guidedOnboarding.ts
- src/renderer/settings/components/ModelProviderSettingsDetail.vue
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
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.
20260513_175609.mp4
Summary by CodeRabbit
New Features
Tests