feat: Add onboarding form for first timers #6876
Conversation
Deploying windmill with
|
| Latest commit: |
7bc9de2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://63a8aadf.windmill.pages.dev |
| Branch Preview URL: | https://tl-first-timers-onboarding2.windmill.pages.dev |
tristantr
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
|
Claude finished @tristantr's task —— View job Code Review: Add onboarding form for first timers ✅I've completed a comprehensive review of this PR. Overall, this is a solid implementation of a first-time user onboarding feature for cloud-hosted instances. Here's my detailed feedback: ✅ Strengths
🔍 Issues & RecommendationsCritical IssuesNone found. The code is functional and safe. High Priority Issues1. Missing Error Handling in Onboarding Component ( The error handling gracefully continues to workspaces even on failure, but the user experience could be improved: } catch (error) {
console.error('Error submitting onboarding data:', error)
sendUserToast('Failed to save information: ' + (error?.body || error?.message || error), true)
} finally {
isSubmitting = false
goto('/user/workspaces') // ⚠️ Navigates away even on error
}Recommendation: Consider whether failed onboarding submission should block navigation or just log silently. Current behavior might confuse users who see an error toast but are immediately redirected. 2. Unused Variable in Backend ( if let Some((email, hash, super_admin, _first_time_user)) = email_w_h {The Recommendation: Either remove it from the query entirely or document why it's kept (possibly for future use). Medium Priority Issues3. Svelte 5 Compliance - Missing Runes Best Practices The onboarding component uses Svelte 5 runes ( let currentStep = $state(1)
let useCaseText = $state('')
let selectedSource = $state<string | null>(null)
let isSubmitting = $state(false)According to Potential improvement: Consider using const canContinue = $derived(selectedSource !== null && !isSubmitting)4. API Type Safety The onboarding component constructs the request body manually: await UserService.submitOnboardingData({
requestBody: {
email,
customer_id: null,
is_ee_trial: null,
touch_point: touchPoint,
use_case: useCaseText
}
})Recommendation: Verify that the TypeScript types generated from the OpenAPI spec are being used. If 5. Performance - Multiple In const globalUserInfo = await UserService.globalWhoami()
if (globalUserInfo.first_time_user) {
goto('/user/onboarding')
return
}Observation: This adds an extra API call on every login. Consider whether this check could be included in the login response itself to reduce round trips. 6. Missing Input Validation The textarea in step 2 has no length validation: <textarea
bind:value={useCaseText}
class="input mt-1"
rows="8"
></textarea>Recommendation: Add maxlength validation to prevent excessively long submissions: <textarea
bind:value={useCaseText}
maxlength="1000"
...
/>Low Priority / Style Issues7. Console Logging in Production Code
if (!email) {
console.error('No email found in global user info:', globalUserInfo)
throw new Error('User email not found')
}Recommendation: This is fine for debugging, but consider using a proper logging service in production. 8. Hardcoded Success Message
sendUserToast('Information saved successfully')Recommendation: Consider making toast messages consistent across the app or using i18n if internationalization is planned. 9. Button Disabled State Could Be More Explicit
disabled={isSubmitting}
loading={isSubmitting}Recommendation: The "Continue" button should also check if 🔒 Security Review✅ No security issues found:
⚡ Performance ConsiderationsPositive:
Minor concern:
📋 Code Quality & Best PracticesBackend (Rust):
Frontend (Svelte 5):
🎯 Specific Recommendations
🧪 Testing RecommendationsSince this is a user-facing feature, ensure testing covers:
📝 Reproduction Instructions for TestersTo test this feature on a cloud-hosted instance, follow these steps: Testing the onboarding flow:
Testing the skip functionality:
Testing self-hosted behavior:
Verifying data submission:
SummaryThis is a well-implemented feature with clean code and good separation between OSS and EE versions. The main improvements needed are around error handling clarity, removing unused code, and minor optimizations. No blocking issues were found. Recommendation: Approve with minor suggestions ✅ I've also left inline comments on the specific files for the most important issues. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } finally { | ||
| isSubmitting = false | ||
| goto('/user/workspaces') | ||
| } |
There was a problem hiding this comment.
Error Handling Concern: The goto('/user/workspaces') in the finally block means users are redirected even when submission fails. This might be confusing since they'll see an error toast but immediately navigate away.
Suggestion: Consider one of these approaches:
- Only navigate on success (move
gotoinside try block after line 77) - Or make it clear to users that skipping/continuing without saving is acceptable
// Option 1: Navigate only on success
try {
await UserService.submitOnboardingData({ ... })
sendUserToast('Information saved successfully')
goto('/user/workspaces')
} catch (error) {
console.error('Error submitting onboarding data:', error)
sendUserToast('Failed to save information. You can continue anyway.', true)
// Let user decide whether to retry or skip
} finally {
isSubmitting = false
}
| </p> | ||
| <div class="mb-6"> | ||
| <textarea | ||
| bind:value={useCaseText} |
There was a problem hiding this comment.
Input Validation Missing: Consider adding a maxlength attribute to prevent excessively long submissions:
<textarea
bind:value={useCaseText}
class="input mt-1"
rows="8"
maxlength="1000"
></textarea>This provides better UX and prevents potential backend issues with very large payloads.
backend/windmill-api/src/users.rs
Outdated
| .await?; | ||
|
|
||
| if let Some((email, hash, super_admin, first_time_user)) = email_w_h { | ||
| if let Some((email, hash, super_admin, _first_time_user)) = email_w_h { |
There was a problem hiding this comment.
Unused Variable: The _first_time_user field is fetched from the database but not used (prefixed with _). This was previously used for cookie-based onboarding but that code was removed.
Options:
- Remove it from the query entirely to save a tiny bit of performance
- Keep it if there are plans to use it for other purposes
- Add a comment explaining why it's kept
// If removing (option 1):
"SELECT email, password_hash, super_admin FROM password WHERE ..."
// Returns: Option<(String, String, bool)>
// If keeping with documentation (option 2):
// Note: first_time_user fetched but unused - kept for future analytics
if let Some((email, hash, super_admin, _first_time_user)) = email_w_h {
| // Only show onboarding for cloud-hosted instances | ||
| if (isCloudHosted()) { | ||
| try { | ||
| const globalUserInfo = await UserService.globalWhoami() |
There was a problem hiding this comment.
Performance Consideration: This adds an extra globalWhoami() API call on every login for cloud-hosted instances.
Potential optimization: Consider including the first_time_user flag directly in the login response to eliminate this extra round-trip. This would require backend changes to the login endpoint, but could improve login performance slightly.
// Current: Login → Check globalWhoami() → Redirect
// Optimized: Login (returns first_time_user) → RedirectNot critical, but worth considering if login performance becomes a concern.
| #[derive(Deserialize, Debug)] | ||
| #[allow(dead_code)] | ||
| pub struct OnboardingData { | ||
| pub email: String, |
There was a problem hiding this comment.
user info should not come from the frontend to avoid impersonation
You can get the email & username from ApiAuthed instead
There was a problem hiding this comment.
ideally, only use_case and touch_point comes from the front (i.e only what the user actually inputted)
and everything else comes from the backend which cannot be tampered with
| return | ||
| } | ||
| if (firstTime) { |
There was a problem hiding this comment.
Just checked and this value comes from users.rs (is_first_time_setup()).
The function returns true only if the instance has a single user [email protected]:changeme
so this should never happen on cloud
| showPassword = true | ||
| console.error('Could not load logins', e) | ||
| // Only log if it's not a 404 | ||
| if (e?.status !== 404) { |
There was a problem hiding this comment.
No reason to only log 404s, avoid silent errors at all cost
| function selectSource(sourceId: string) { | ||
| selectedSource = sourceId | ||
| // Auto-advance to next step for all selections | ||
| currentStep = 2 |
There was a problem hiding this comment.
currentStep = Math.min(totalStepCount, currentStep + 1)
But the core reason is :
Numbers have no semantic meaning in this case, do not rely on the fact that "1 = source selection"
Maybe one day the steps will change and this is prone to future bugs
| } | ||
| function goToPreviousStep() { | ||
| if (currentStep === 2) { |
There was a problem hiding this comment.
same here, this relies on the fact that total steps count = 2 (avoid magic numbers)
3 participants
No description provided.