Integrate Onboarding with Server configurations #34
Conversation
Deploying webui with
|
| Latest commit: |
a1286fe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8c9302d9.webui-9fh.pages.dev |
| Branch Preview URL: | https://issues.webui-9fh.pages.dev |
Code Review Summary✨ This pull request introduces significant architectural improvements, especially in state management and configuration handling. The 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
🔍 Code Review💡 1. **components/WalletForm.vue** (Lines 160-165) - REFACTORThe Suggested Code: Current Code: onMounted(() => {
if (!isEditing.value && (!form.value.currency || form.value.currency.trim() === '')) {
form.value.currency = defaultCurrency.value;
}
});💡 2. **components/settings/SettingsWallets.vue** (Lines 25-29) - BUGThe 'Default Group' section uses hardcoded options ('Personal', 'Family', 'Work') and saves the selected group as a string value. However, the Suggested Code: Current Code: <select v-if="isEditMode" v-model="group" class="form-select">
<option value="Personal">Personal</option>
<option value="Family">Family</option>
<option value="Work">Work</option>
</select>💡 3. **components/settings/SettingsWallets.vue** (Line 91) - IMPROVEMENTCurrently, only the Suggested Code: Current Code: if (walletId.value) {
await configurationsApi.update(CONFIGURATION_KEYS.WALLET, {
value: walletId.value,
type: 'string'
});
// Refresh shared configurations so other parts of the app reflect the change immediately
await sharedData.loadConfigurations(true);
}💡 4. **composables/useSharedData.ts** (Lines 208-210) - BUGThe Suggested Code: Current Code: const groupId = typeof configured === 'object' ? configured.id : configured;
const group = groups.value.find((g) => g.id === parseInt(groupId));
if (group) return group;💡 5. **composables/useSharedData.ts** (Lines 264-265) - CLARITYThe comment Suggested Code: Current Code: // REMOVED: Auto-initialization - now controlled by data manager
// ensureInit();💡 6. **pages/onboarding.vue** (Lines 406-408) - BUGThe Suggested Code: Current Code: if (!selectedLanguage.value) return;
showSuccess(
'Language Set!',
`Language changed to ${availableLanguages.find((l) => l.code === selectedLanguage.value)?.name}.`
);
nextStep();💡 7. **pages/onboarding.vue** (Lines 411-420) - BUGSimilar to the language selection, this Suggested Code: Current Code: if (!isWalletCurrencySetupValid.value) return;
let message = 'Settings saved successfully!';
if (walletChoice.value === 'rename-default') {
message = `Default wallet renamed to "${newWalletName.value}" and currency set to ${selectedCurrency.value}.`;
} else if (walletChoice.value === 'create-new') {
message = `New wallet "${newWalletName.value}" created with ${newWalletCurrency.value} currency.`;
} else {
message = `Default wallet configured with ${selectedCurrency.value} currency.`;
}
showSuccess('Setup Complete!', message);
nextStep();💡 8. **pages/onboarding.vue** (Line 322) - BUGThe Suggested Code: Current Code: const transactionCount = computed(() => 0);💡 9. **utils/transactionMapper.ts** (Lines 28-31) - CLARITYThe parameters Suggested Code: Current Code: toFrontend(
api: ApiTransaction,
parties: Party[] = [],
_categories: Category[] = [],
wallets: Wallet[] = [],
_groups: GroupLite[] = []
): FrontendTransaction {💡 10. **utils/transactionMapper.ts** (Lines 194-196) - CLARITYSimilar to the single Suggested Code: Current Code: toFrontendBatch(
apiTransactions: ApiTransaction[],
parties: Party[],
categories: Category[],
wallets: Wallet[],
groups: GroupLite[]
): FrontendTransaction[] {
return apiTransactions.map((api) => this.toFrontend(api, parties, categories, wallets, groups));
},Verdict: APPROVE Posted as a comment because posting a review failed. |
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
Looks okay, minor nitpick.
I am yet to actually tests though.
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
So on-boarding starts after sign up. That must be the first the user should see. If the user is already signed up, you need to check if they completed on-boarding if they did not you have to show it.
Some of the post/put to configurations return 422 Unprocessable Content.
This flow does not show a user the onboarding form after register.
Test cases:
- New users see onboarding form
- Logged-in user with on-boarding complete from sees onboarding form
- If user skips onboarding we should mark it as complete
|
Some guidelines...
In Fix needed: Strip key from payload body before sending PUT request.
Fix needed: Update middleware to fetch onboarding status and redirect accordingly.
const skipStep = () => {
nextStep();
};Fix needed: If user skips to the final step or navigates away, mark onboarding-complete in the API. Test using these
Summary The branch needs fixes for the configuration API payload and onboarding flow logic before merge. Todos
|
Issues resolved:
|
|
|
||
| // Check onboarding status | ||
| // Force reload configurations to avoid stale cache across sessions | ||
| const configurations = await sharedData.loadConfigurations(true); | ||
|
|
||
| // Explicitly check for the onboarding_complete key | ||
| // If it's missing or false, we consider onboarding incomplete | ||
| const isOnboardingComplete = !!configurations?.[CONFIGURATION_KEYS.ONBOARDING_COMPLETE]; | ||
|
|
||
| // If onboarding is NOT complete | ||
| if (!isOnboardingComplete) { | ||
| // And we are NOT already on the onboarding page | ||
| if (to.path !== '/onboarding') { | ||
| return navigateTo('/onboarding'); | ||
| } | ||
| // If we ARE on the onboarding page, allow access (do nothing) | ||
| return; | ||
| } | ||
|
|
||
| // If onboarding IS complete | ||
| if (isOnboardingComplete) { | ||
| // And we try to go to onboarding page | ||
| if (to.path === '/onboarding') { | ||
| // Redirect to dashboard | ||
| return navigateTo('/dashboard'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks good but too many unnecessary comments, all of those are obvious right?
| return; | ||
| } | ||
| // Login succeeded – now check onboarding status (errors here should not affect login error) |
There was a problem hiding this comment.
| // Login succeeded – now check onboarding status (errors here should not affect login error) |
| const handleSubmit = async () => { | ||
| loading.value = true; | ||
| loginError.value = ''; | ||
| // First attempt login |
There was a problem hiding this comment.
| // First attempt login |
| await login(form.value); | ||
| router.push('/dashboard'); | ||
| } catch { | ||
| // Login failed – show error and stop further processing |
There was a problem hiding this comment.
| // Login failed – show error and stop further processing |
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
Signed-off-by: nfebe <fenn25.fn@gmail.com>
Add dynamic key to NuxtLayout based on route.meta.layout to ensure proper DOM cleanup when switching between layouts (e.g., onboarding to dashboard). Signed-off-by: nfebe <fenn25.fn@gmail.com>
- Switch Reports, Settings, AI Insights to dashboard layout for consistency - Remove auto-selection of default wallet in useStatistics composable - Use currentStatistics from composable in reports page - Change default item indicator color from primary to success Signed-off-by: nfebe <fenn25.fn@gmail.com>
General summary
Align wallet creation with backend expectations and improve onboarding completion flow.
Auto-fill wallet & currency from the app’s default when not selected.
Minor UI/UX and styling tweaks across settings, tables, and forms pointing to the use of default wallet.
issue addressed: #24
Regarding issue/31: Concerning auth.token security => #31 (comment)
Screencast.From.2025-11-18.13-29-11.mp4