Persist onboarding learning times - #315
Conversation
📝 WalkthroughWalkthroughOnboarding learning-time data now uses shared validation and derivation logic, persists authenticated ChangesOnboarding learning-time flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-learning-times # Conflicts: # src/features/auth/dayova-auth-flow.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c23bb8e49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
convex/users.ts (1)
230-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
backfillLegacyLearningTimesre-runs its full scan on everysyncCurrentUsercall until it succeeds — which never happens for permanently-ambiguous legacy data.Since
hasLearningTimesis the only gate, users whose legacy answers are incomplete/ambiguous (or who never had legacy answers) will pay the up-to-6-query scan on every single session sync indefinitely, since there's no persisted "already attempted, unrecoverable" marker.Consider caching the outcome (e.g., a flag/timestamp on the user document, or a short-lived marker) so the scan is attempted at most once (or rate-limited) per user, rather than on every sync call.
Also applies to: 333-345
🤖 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 `@convex/users.ts` around lines 230 - 273, Update backfillLegacyLearningTimes and its syncCurrentUser call path to persist an attempted/unrecoverable backfill marker on the user, and check it before scanning. Set the marker when legacy data is missing or ambiguous, while preserving successful insertion through insertLearningTimesWhenAbsent; ensure permanently invalid users no longer repeat the full query scan on every sync.
🤖 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.
Nitpick comments:
In `@convex/users.ts`:
- Around line 230-273: Update backfillLegacyLearningTimes and its
syncCurrentUser call path to persist an attempted/unrecoverable backfill marker
on the user, and check it before scanning. Set the marker when legacy data is
missing or ambiguous, while preserving successful insertion through
insertLearningTimesWhenAbsent; ensure permanently invalid users no longer repeat
the full query scan on every sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea80d486-3518-43a7-acca-5266e91b282b
📒 Files selected for processing (6)
convex/learningTimeAvailability.test.tsconvex/learningTimeAvailability.tsconvex/users.test.tsconvex/users.tssrc/features/auth/dayova-auth-flow.tsxsrc/features/auth/login-screen.ui.test.tsx
1 participant
Summary
userLearningTimesrecord per onboarding weekday from the selected start time and daily study durationRoot cause
Onboarding stored
studyDays,learningTime, anddailySchoolTimeonly as historicaluserOnboardingAnswers. Settings and learning-plan generation read the separateuserLearningTimestable, so completed onboarding could still leave the learner without scheduling availability.User impact
Onboarding-created learning times now appear immediately in Settings and are consumed by learning-plan scheduling. Existing complete and unambiguous legacy answers are recovered safely, while invalid, incomplete, or ambiguous answers continue through the existing setup flow. Once onboarding, legacy recovery, or a Settings change establishes the authoritative schedule, old answers cannot resurrect times the learner later removes.
Validation
main(f89e8e7) and resolved the onboarding-flow conflict; final effective scope is nine focused filespnpm install --frozen-lockfilewith Node 24.18.0 and pnpm 11.15.1pnpm check— Biome, ESLint, and TypeScript passingpnpm test— 70 Vitest files / 352 tests and 21 Jest suites / 52 rendered UI tests passing, plus all auxiliary script testspnpm format:checkpnpm check:unusedgit diff --check3dc149d— no actionable comments; final hosted incremental review was rate-limited3dc149d…37e4fd7) — 0 issues37e4fd7— passedLinear: DAY-65
Closes #64
UI evidence
day-65-onboarding-message.mp4
Note
I did not re-record it after the latest commits. The existing recording still shows the unchanged onboarding completion UI, while the subsequent changes were backend validation and recovery fixes. It is not evidence from the final commit 37e4fd7.
Summary by CodeRabbit