Add validation analytics, execution tracking, and founder overview#144
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds session execution lifecycle tracking (started, completed, partially completed, missed, adjusted) with new Convex mutations and schema fields, integrates PostHog analytics via a new hook and identity component capturing validation events across onboarding/entry/plan flows, and adds a founder-only daily validation overview screen with attribution recording. ChangesSession Lifecycle & Validation Analytics
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
FleetAdmiralJakob
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/types/dayEntries.ts (1)
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse shared execution/missed types to prevent enum drift.
DayEntryrepeats literal unions already defined for learning-plan sessions. Importing shared aliases keeps schema/UI contracts synchronized with fewer update points.Proposed refactor
+import type { + SessionExecutionStatus, + MissedReason, +} from "~/features/learning-plans/types"; + export type DayEntry = { id: Id<"dayEntries"> | Id<"learningPlanSessions">; @@ - executionStatus?: - | "notStarted" - | "started" - | "completed" - | "partiallyCompleted" - | "missed" - | "adjusted"; + executionStatus?: SessionExecutionStatus; startedAt?: number; outcomeAt?: number; - missedReason?: - | "no_time" - | "forgot" - | "no_motivation" - | "too_hard" - | "too_big" - | "unclear" - | "other"; + missedReason?: MissedReason; adjustedFromSessionId?: Id<"learningPlanSessions">;🤖 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/types/dayEntries.ts` around lines 15 - 32, The DayEntry type in dayEntries.ts contains inline literal unions for executionStatus and missedReason that duplicate type definitions already present in the learning-plan sessions types. Replace these inline literal unions with imported type aliases from the shared type definitions file (likely the learning-plan sessions type file). Import the execution status type alias and the missed reason type alias, then use them to define the executionStatus and missedReason properties instead of repeating the literal unions inline. This ensures both DayEntry and learning-plan sessions share the same type definitions, preventing enum drift and reducing future maintenance burden.src/app/learning-plans/[planId]/index.tsx (1)
382-382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: other sessions silently no-op while one action is pending.
runSessionActionearly-returns when anypendingSessionIdis set, but only the active card receivesisPending(Line 563), so buttons on other sessions remain visually enabled yet do nothing on tap. Consider disabling all action buttons (or surfacing a global pending state) while any action is in flight.Also applies to: 243-280
🤖 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/app/learning-plans/`[planId]/index.tsx at line 382, The issue is that when a `pendingSessionId` is set, the early return in `runSessionAction` prevents actions from executing, but the `isPending` visual indicator only appears on the active card (referenced at Line 563), leaving buttons on other sessions appearing enabled and clickable while silently doing nothing. To fix this, extend the `isPending` state handling to disable action buttons across all sessions (not just the active card) while any action is pending, or introduce a global pending state indicator that makes it clear to users that an operation is in flight and other interactions are blocked.
🤖 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 `@convex/learningPlans.ts`:
- Around line 1257-1267: The query for learningPlanSessions is returning the
first 50 sessions in ascending sortOrder but needs to find the actual maximum
sortOrder to avoid collisions when a plan has more than 50 sessions. Modify the
query chain by adding `.order("desc")` after the `.withIndex()` call to sort by
sortOrder in descending order, then change `.take(50)` to `.take(1)` to retrieve
only the highest sortOrder. Replace the reduce operation that finds the max
among the 50 lowest values with a simple check for the first result's sortOrder
or fallback to 0 if no sessions exist, then add 1 to get the new sortOrder
value.
In `@convex/validationAnalytics.ts`:
- Around line 84-113: The localDayKey parameter is accepted as an arbitrary
string in both mutations but is relied upon for string comparison operations
that assume YYYY-MM-DD format semantics. Add validation at the beginning of each
mutation handler (in the unnamed first mutation and in markReturnedNextDay) to
verify that localDayKey matches the expected YYYY-MM-DD date format before it is
used in any comparisons or persisted to the database. If the format is invalid,
throw an error to prevent malformed values from corrupting the
lastActivityDayKey and lastReturnDayKey fields.
In `@docs/contexts/integrations/CONTEXT.md`:
- Line 20: Fix the typo in the analytics coverage list where "generalprobe
completions" should be corrected to "general probe completions" with a proper
space between "general" and "probe", or use the exact canonical event label name
if that differs. Locate the text "generalprobe completions" in the analytics
coverage section and update it to use the proper spacing or canonical naming
convention for this event type.
In `@src/app/validation/overview.tsx`:
- Around line 28-29: The csvCell function does not protect against formula
injection vulnerabilities where user-controlled values starting with =, +, -, or
@ characters can be interpreted as formulas by spreadsheet applications. Modify
the csvCell function to detect when the stringified value starts with any of
these dangerous characters and prepend a single quote to force the spreadsheet
application to treat the entire cell as plain text, ensuring the quote escaping
logic remains intact after this check.
In `@src/components/analytics-identity.tsx`:
- Around line 61-82: The dedupe key returnCaptureKeyRef.current is being set
before the markReturnedNextDay promise resolves, which blocks retries if the
mutation fails. Move the assignment of returnCaptureKeyRef.current =
returnCaptureKey from its current position (before the markReturnedNextDay call)
to inside the .then() callback, placing it after the check for result.captured
to ensure the dedupe key is only set when the mutation succeeds.
In `@src/lib/runtime-config.test.ts`:
- Around line 88-89: The test for the typed-env configuration is incomplete. In
addition to asserting that EXPO_PUBLIC_POSTHOG_API_KEY is undefined, add a
corresponding assertion to verify that EXPO_PUBLIC_POSTHOG_HOST is also
undefined. Add an expect statement for env.EXPO_PUBLIC_POSTHOG_HOST right after
the existing EXPO_PUBLIC_POSTHOG_API_KEY assertion to ensure both optional
PostHog configuration keys are properly validated when not provided.
---
Nitpick comments:
In `@src/app/learning-plans/`[planId]/index.tsx:
- Line 382: The issue is that when a `pendingSessionId` is set, the early return
in `runSessionAction` prevents actions from executing, but the `isPending`
visual indicator only appears on the active card (referenced at Line 563),
leaving buttons on other sessions appearing enabled and clickable while silently
doing nothing. To fix this, extend the `isPending` state handling to disable
action buttons across all sessions (not just the active card) while any action
is pending, or introduce a global pending state indicator that makes it clear to
users that an operation is in flight and other interactions are blocked.
In `@src/types/dayEntries.ts`:
- Around line 15-32: The DayEntry type in dayEntries.ts contains inline literal
unions for executionStatus and missedReason that duplicate type definitions
already present in the learning-plan sessions types. Replace these inline
literal unions with imported type aliases from the shared type definitions file
(likely the learning-plan sessions type file). Import the execution status type
alias and the missed reason type alias, then use them to define the
executionStatus and missedReason properties instead of repeating the literal
unions inline. This ensures both DayEntry and learning-plan sessions share the
same type definitions, preventing enum drift and reducing future maintenance
burden.
🪄 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: 98738de1-f987-4270-8257-1ba131e8b304
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.codex/environments/environment.toml.env.exampleapp.config.tsconvex/dayEntries.tsconvex/learningPlans.test.tsconvex/learningPlans.tsconvex/schema.tsconvex/users.tsconvex/validationAnalytics.test.tsconvex/validationAnalytics.tsdocs/contexts/integrations/CONTEXT.mddocs/contexts/platform/CONTEXT.mddocs/contexts/product/CONTEXT.mdpackage.jsonsrc/app/_layout.tsxsrc/app/entry/new.tsxsrc/app/learning-plans/[planId]/generating.tsxsrc/app/learning-plans/[planId]/index.tsxsrc/app/learning-plans/new.tsxsrc/app/validation/overview.tsxsrc/components/analytics-identity.tsxsrc/context/AuthContext.tsxsrc/features/learning-plans/constants.tssrc/features/learning-plans/types.tssrc/lib/analytics-core.tssrc/lib/analytics.tssrc/lib/runtime-config.test.tssrc/lib/runtime-config.tssrc/types/dayEntries.tssrc/types/validation.ts
added 2 commits
June 23, 2026 21:22
# Conflicts: # src/app/learning-plans/new.tsx
Collaborator
Author
PostHog dashboard follow-upI evaluated whether to create the PostHog dashboard directly through the PostHog MCP tools. The tool surface can create dashboards and saved insights, but PostHog's schema currently only shows one of the planned validation events: Recommended path: merge/deploy this PR, run one smoke pass that emits the validation events, then paste the prompt below into the PostHog dashboard agent. Follow-up issue for backend-confirmed analytics transport: #146. |
Merged
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/app/learning-plans/`[planId]/sessions/[sessionId]/index.tsx:
- Around line 846-848: `completeAndLeave` can race with the auto-start path and
call `startSession` twice, which can duplicate `study_slot_started` and conflict
with `recordSessionOutcome`. Add a single shared helper/promise for starting
tracking in `index.tsx` so both the auto-start effect and `completeAndLeave` use
the same in-flight `startSession` call instead of rechecking
`content.session.executionStatus` and firing a second mutation. Update the logic
around `didStartTrackingRef`, `completeAndLeave`, and the auto-start effect to
await the shared guard before proceeding.
🪄 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: 35685912-8d9e-4558-a60d-156b41b747d8
⛔ Files ignored due to path filters (2)
convex/_generated/api.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
app.config.tsconvex/dayEntries.tsconvex/learningPlans.test.tsconvex/learningPlans.tsconvex/learningSessionContent.tsconvex/schema.tsconvex/users.tsdocs/contexts/product/CONTEXT.mdpackage.jsonsrc/app/_layout.tsxsrc/app/entry/new.tsxsrc/app/learning-plans/[planId]/generating.tsxsrc/app/learning-plans/[planId]/sessions/[sessionId]/index.tsxsrc/app/learning-plans/new.tsxsrc/context/AuthContext.tsxsrc/features/learning-plans/replan-recovery.test.tssrc/features/learning-plans/types.ts
✅ Files skipped from review due to trivial changes (1)
- docs/contexts/product/CONTEXT.md
🚧 Files skipped from review as they are similar to previous changes (11)
- package.json
- app.config.ts
- src/app/learning-plans/[planId]/generating.tsx
- src/app/learning-plans/new.tsx
- src/app/entry/new.tsx
- src/app/_layout.tsx
- convex/schema.ts
- convex/dayEntries.ts
- convex/learningPlans.test.ts
- src/context/AuthContext.tsx
- convex/learningPlans.ts
Comment on lines
846
to
+848
| const didEnsureRef = useRef(false); | ||
| const didAutoFinishRef = useRef(false); | ||
| const didStartTrackingRef = useRef(false); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Race: completeAndLeave can double-fire startSession (and duplicate study_slot_started).
The auto-start effect sets didStartTrackingRef.current synchronously but the actual startSession mutation is still in flight when completeAndLeave independently re-checks content.session.executionStatus === "notStarted" (stale until the query resubscribes) and calls startSession again. This can duplicate the study_slot_started analytics event and/or race recordSessionOutcome against a second startSession call, potentially surfacing a spurious completion error to the user.
Consolidate both call sites onto a single guarded/deduplicated helper so completeAndLeave awaits the same in-flight call instead of firing a second one.
🔧 Proposed fix: share a single start-tracking promise
const didStartTrackingRef = useRef(false);
+ const startSessionPromiseRef = useRef<ReturnType<typeof startSession> | null>(
+ null,
+ );
+
+ const ensureSessionStarted = useCallback(() => {
+ if (!sessionId) return Promise.reject(new Error("Missing sessionId"));
+ if (!startSessionPromiseRef.current) {
+ didStartTrackingRef.current = true;
+ startSessionPromiseRef.current = startSession({ sessionId })
+ .then((started) => {
+ void capture(
+ "study_slot_started",
+ definedAnalyticsProperties({
+ ...learningSessionAnalyticsProperties(started),
+ started_at: started.startedAt,
+ }),
+ );
+ return started;
+ })
+ .catch((error) => {
+ didStartTrackingRef.current = false;
+ startSessionPromiseRef.current = null;
+ throw error;
+ });
+ }
+ return startSessionPromiseRef.current;
+ }, [capture, sessionId, startSession]); useEffect(() => {
if (
!sessionId ||
!content ||
content.session.executionStatus !== "notStarted" ||
didStartTrackingRef.current
)
return;
- didStartTrackingRef.current = true;
- void startSession({ sessionId })
- .then((result) => {
- void capture(
- "study_slot_started",
- definedAnalyticsProperties({
- ...learningSessionAnalyticsProperties(result),
- started_at: result.startedAt,
- }),
- );
- })
- .catch((error: unknown) => {
- didStartTrackingRef.current = false;
- logDiagnosticError("Failed to start learning session tracking.", error, {
- source: "learningSession.startSession",
- level: "warn",
- });
- });
- }, [capture, content, sessionId, startSession]);
+ void ensureSessionStarted().catch((error: unknown) => {
+ logDiagnosticError("Failed to start learning session tracking.", error, {
+ source: "learningSession.startSession",
+ level: "warn",
+ });
+ });
+ }, [content, ensureSessionStarted, sessionId]); if (content?.session.executionStatus === "notStarted") {
- const started = await startSession({ sessionId });
- didStartTrackingRef.current = true;
- void capture(
- "study_slot_started",
- definedAnalyticsProperties({
- ...learningSessionAnalyticsProperties(started),
- started_at: started.startedAt,
- }),
- );
+ await ensureSessionStarted();
}Also applies to: 947-974, 1056-1076
🤖 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/app/learning-plans/`[planId]/sessions/[sessionId]/index.tsx around lines
846 - 848, `completeAndLeave` can race with the auto-start path and call
`startSession` twice, which can duplicate `study_slot_started` and conflict with
`recordSessionOutcome`. Add a single shared helper/promise for starting tracking
in `index.tsx` so both the auto-start effect and `completeAndLeave` use the same
in-flight `startSession` call instead of rechecking
`content.session.executionStatus` and firing a second mutation. Update the logic
around `didStartTrackingRef`, `completeAndLeave`, and the auto-start effect to
await the shared guard before proceeding.
Collaborator
Author
CodeRabbit Follow-up AppliedFixed the valid CodeRabbit finding about duplicate startSession calls in the learning-session screen. Files modified:
Commit: c6e7ad9 Validation run locally:
|
14 tasks
This was referenced Jul 13, 2026
Closed
This was referenced Jul 13, 2026
This was referenced Jul 17, 2026
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
2 participants
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.
Summary
Builds the validation-phase analytics system end to end on the current
mainbranch. This supersedes the stale draft analytics foundation in #87 and completes the execution/recovery/founder-observability slices from the validation tracking PRD.The PR adds:
validation_student_codewhen available./validation/overviewwith daily execution rows, Product Signal vs Accountability Signal attribution, and CSV sharing.Issues Resolved
Closes #76
Closes #77
Closes #78
Closes #79
Closes #80
Note for #79/#80: PR #144 lands the backend/session-state foundation. The remaining learner-facing missed/recovery UX and app-side event wiring are tracked as follow-up design work in #155; this PR should not auto-close #155.
Closes #81
Closes #82
Closes #83
Closes #84
How This Maps To The Validation Plan
study_slot_startedis emitted only after a durable Convex transition tostartedsucceeds.completed,partiallyCompleted,missed, oradjusted, with state mirrored into synced day entries.no_time,forgot,no_motivation,too_hard,too_big,unclear,other) and emit both missed/outcome analytics.plan_adjustedwith old/new context.generalprobe_completedwithout creating a separate content system.user_returned_next_dayonce per local day after prior validation activity.Implementation Notes
validationUserStates,validationAttributions). No backfill migration is required.validationStudentCodeis synced from Clerk unsafe metadata into Convex and PostHog as an analytics join key only. It is not used for authorization.users.validationRole === "founder"; the mobile profile sync does not let users grant that role to themselves.EXPO_PUBLIC_POSTHOG_API_KEY, and capture helpers no-op when analytics is not configured.Still Open Work / Follow-Ups
Configure production/EAS PostHog envs (DONE ✅EXPO_PUBLIC_POSTHOG_API_KEY,EXPO_PUBLIC_POSTHOG_HOST) and confirm ingestion in the real PostHog project.validationStudentCodefor students andvalidationRole: "founder"for internal founder/coach accounts.missSession,adjustMissedSession,partiallyCompleted,study_slot_missed,missed_reason_selected, andplan_adjustedinto the app UI./validation/overview.Validation
Local validation run during implementation:
pnpm convex codegenpnpm testpnpm typecheckpnpm lintgit diff --check && git diff --cached --checkCodeRabbit notes:
reviewingfor the full 10-minute window and emitted no new findings before timeout.Out Of Scope
Summary by CodeRabbit
New Features
Bug Fixes
Documentation