Conversation
🦋 Changeset detectedLatest commit: e678523 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // kilocode_change start: Onboarding handlers | ||
| const handleSelectFreeModels = useCallback(() => { | ||
| // Mark onboarding as complete | ||
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) |
There was a problem hiding this comment.
WARNING: "Free models" selection only sets onboarding complete; it may still show the Welcome screen
handleSelectFreeModels posts hasCompletedOnboarding but does not navigate to chat or otherwise affect showWelcome. On the next render, this falls through to showWelcome ? <WelcomeView /> : …, so users who still have showWelcome === true may immediately land on WelcomeView instead of "Start coding immediately".
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
|
|
|
||
| const OnboardingOption: React.FC<OnboardingOptionProps> = ({ title, description, onClick }) => { | ||
| return ( | ||
| <button |
There was a problem hiding this comment.
SUGGESTION: Add an explicit type="button"
Buttons default to type="submit", which can trigger unintended form submissions if this component is ever used within a <form>.
| <button | |
| <button type="button" |
There was a problem hiding this comment.
This doesn't seem to be a risk, since this is not in a form and unlikely to be.
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) | ||
| // Navigate to auth view which will show the device code and handle the OAuth flow | ||
| // The AuthView auto-starts device auth on mount | ||
| switchTab("auth") |
There was a problem hiding this comment.
WARNING: switchTab("auth") won't take effect while onboarding is still rendered
handleSelectPremiumModels sets the tab to auth, but the render short-circuits to <OnboardingView /> while showOnboarding is true. Since hasCompletedOnboarding only flips after the extension posts updated state back, users can momentarily remain on the onboarding screen; and if showWelcome is still true when onboarding completes, they may land on <WelcomeView /> instead of the auth flow.
Consider hiding onboarding immediately (e.g., local state) or making the render conditional prioritize tab === "auth" / tab === "settings" after a selection.
webview-ui/src/App.tsx
Outdated
| // | ||
| // This ensures existing users who upgrade don't see the onboarding screen, | ||
| // while new users who have never used the extension will see it. | ||
| const isExistingUser = (taskHistoryFullLength ?? 0) > 0 |
There was a problem hiding this comment.
maybe we can also set 'hasCompletedOnboarding' if this is true so we can choose this logic at some point in the future, that way this choice doesn't tie in to specific architecture choices
There was a problem hiding this comment.
@markijbema - can you review the new code? Hopefully this is better integrated now.
webview-ui/src/App.tsx
Outdated
| @@ -15,6 +15,7 @@ import ChatView, { ChatViewRef } from "./components/chat/ChatView" | |||
| import HistoryView from "./components/history/HistoryView" | |||
| import SettingsView, { SettingsViewRef } from "./components/settings/SettingsView" | |||
| import WelcomeView from "./components/kilocode/welcome/WelcomeView" // kilocode_change | |||
There was a problem hiding this comment.
we should now never show this right? so why have two? Any reason not to ditch the welcomeview?
There was a problem hiding this comment.
@markijbema - can you take another look? I removed this in 0e8481f
| } | ||
|
|
||
| // kilocode_change start: Show OnboardingView for new users who haven't completed onboarding | ||
| const showOnboarding = hasCompletedOnboarding !== true |
There was a problem hiding this comment.
WARNING: Onboarding may briefly flash for existing users
showOnboarding is derived solely from hasCompletedOnboarding, but existing users upgrading may initially have this unset/false. That means the UI renders <OnboardingView /> until the migration useEffect posts hasCompletedOnboarding and the extension sends updated state back.
If the goal is to never show onboarding to existing users, consider including taskHistoryFullLength (or another persisted signal) directly in the render condition so existing users skip onboarding on the first post-hydration render.
There was a problem hiding this comment.
I don't think this is an actual concern, and I don't see it in my testing.
There was a problem hiding this comment.
Did you test it by completing the onboarding, doing zero tasks, and restarting the extensions (cmd-shift-p, reload webviews) with the extensions open? If the bot is right you'll probably see this screen flash. It sounds at least 70% probable to me
There was a problem hiding this comment.
@markijbema - I've tested. It does not flash, at least for me. AI says the reason why is:
The Flow
-
Initial state (line 297): hasCompletedOnboarding: undefined
-
Before hydration (App.tsx line 349-350):
if (!didHydrateState) {
return null // Nothing renders at all
} -
Hydration occurs (ExtensionStateContext.tsx lines 415-419):
case "state": {
const newState = message.state!
setState((prevState) => mergeExtensionState(prevState, newState))
// ...
setDidHydrateState(true) // Only AFTER state is merged
} -
The extension sends state (ClineProvider.ts line 2524):
hasCompletedOnboarding: this.getGlobalState("hasCompletedOnboarding") -
State merge (line 242): { ...prevRest, ...newRest } - the extension's value overwrites the default undefined.
The key is that setDidHydrateState(true) is called AFTER setState merges the new state. This happens in the same event handler, so React batches these updates.
By the time the component renders for the first time (when didHydrateState becomes true), hasCompletedOnboarding already has the value from the extension's global state.
webview-ui/src/App.tsx
Outdated
| // Mark onboarding as complete | ||
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) | ||
| // The default profile is already set up with a free model, so just close welcome | ||
| // This will trigger a state update that sets showWelcome to false |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| "description": "Sign up and receive $5 in credits" | ||
| }, | ||
| "byok": { | ||
| "title": "Bring my own Key", |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
I prefer Bring your own Key
| // One-time migration: mark existing users as having completed onboarding | ||
| useEffect(() => { | ||
| if (hasCompletedOnboarding !== true && (taskHistoryFullLength ?? 0) > 0) { | ||
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) |
There was a problem hiding this comment.
WARNING: Onboarding migration may repeatedly post state updates
The migration useEffect posts hasCompletedOnboarding whenever hasCompletedOnboarding !== true && (taskHistoryFullLength ?? 0) > 0. If other state changes cause re-renders before the extension posts updated state back, this can send multiple duplicate hasCompletedOnboarding messages (and write global state multiple times).
Consider guarding with a useRef (send-once) or deriving the render condition directly from taskHistoryFullLength so existing users never render onboarding without needing to post a migration message.
There was a problem hiding this comment.
Again don't think this is a concern and have not seen it in my testing.
| // Mark onboarding as complete | ||
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) | ||
| // Navigate to settings with providers section | ||
| switchTab("settings") |
There was a problem hiding this comment.
WARNING: Navigation won’t be visible until onboarding is dismissed
Because App short-circuits to OnboardingView while hasCompletedOnboarding !== true, this switchTab("settings") won’t render until the extension posts updated state. Consider also updating local UI state (optimistically hide onboarding) or deferring navigation until after the state refresh.
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
|
|
||
| const handleSelectPremiumModels = useCallback(() => { | ||
| // Mark onboarding as complete | ||
| vscode.postMessage({ type: "hasCompletedOnboarding", bool: true }) |
There was a problem hiding this comment.
WARNING: Onboarding is marked complete before premium sign-in succeeds
handleSelectPremiumModels posts hasCompletedOnboarding: true immediately, before the device auth flow has actually completed. If the user cancels / fails auth, they'll be treated as "onboarded" and may no longer see the onboarding entry point.
Consider setting completion only after successful authentication (or using a separate "started onboarding" flag).
|
I think this is ready for final review @markijbema. I added some icons too: |
markijbema
left a comment
markijbema
left a comment
There was a problem hiding this comment.
Please try the scenario i mentioned, but tbh I'm not too scared of it; even if it is true it will just mean a quick flash in certain situations on start of the extension.
3 participants
Adds a new welcome screen: