feat(activation): record a browser-blocked step in its own durable bucket (#5617)#5629
Merged
Merged
Conversation
…cket (#5617) PR #5615 made a denied notification permission move the activation wizard's alerts step into its blocked state, and closed the resulting signal loss at the EVENT level (`pro-activation-step-blocked`). The durable side stayed collapsed: a blocked step landed in `skippedSteps` with no marker, so anything querying persisted per-account state in Convex after the fact could not tell a browser refusal from a voluntary skip -- which is exactly what sizing the push-denial population and deciding whether re-prompting an account is worth anything both need. Adds a fourth bucket rather than widening an existing one, so behavioral consumers are untouched: - `ActivationStepOutcome` gains `blocked`. It maps to `pending` in the summary (never `failed` -- we never attempted a write), counts as `pending` in the exit event, and still shows the finish-setup chip. A record change, not UX. - `buildActivationOutcomeBuckets` gains `blockedSteps` and now routes every outcome explicitly: its `else` catch-all would have persisted the new outcome as a failed write. - `selectStepEvent('blocked')` returns `stepBlocked`, not `stepSkipped`. - New pure `selectAdvanceOutcome(state)` is the single seam for all three ways a step can be advanced past without a confirm (Continue, Escape/dismiss, never reached). Writing `blocked` on only the Continue arm would have made the bucket a sample of "denied users who clicked Continue". Steps blocked at mount now record identically to those blocked mid-flow. - `recordProActivationOutcome` and the schema accept `blockedSteps`: optional server-side for mixed deploys, validated for disjointness like every other bucket, and written on every snapshot (each one is a full replacement). - `ProActivationFlowDependencies.recordOutcome` now uses the service's own `ProActivationOutcomeSnapshot` instead of a hand-copied structural shape that silently described less the moment a bucket was added. All 11 mutations of the new logic were verified to redden a test. Claude-Session: https://claude.ai/code/session_011wvTSMvKG7hLkJgRezBJFi
…5617) Code-review follow-ups on the durable `blocked` bucket. Three of these were real defects in the first commit, two of them CI-red: - e2e/pro-activation.spec.ts asserted the exact outcome payload in two places (`toEqual` on the snapshot object, and `['skipped','skipped','skipped']` for the denied-permission flow). Both would have gone red. `npm run typecheck` does not cover e2e/ — its tsconfig includes only `src` — so nothing local caught this; the spec now passes (24/24, run against a real browser). - A mid-flow denial was never flushed. `handleConfirm`'s `blocked` arm updated in-memory state but, unlike the `verified` arm, never called `recordProgress()`. A subscriber denied by the browser who then closed the tab stayed recorded as a voluntary skip forever — precisely the cohort this bucket exists to size. The denial is now durable the instant it happens, which required raising MAX_PRO_ACTIVATION_OUTCOME_REVISION to `members.length + 2` (one extra progress write is now reachable). - The durable buckets and the funnel events counted different cohorts: a blocked step recorded as `blocked` but still emitted `stepSkipped`, so the funnel's skip count included denials while `skippedSteps` excluded them. A new `reportAdvanceEvent` seam routes the event off the resolved outcome, so each step emits exactly one: `stepSkipped` for a real skip, `stepBlocked` for a refusal. This also closes a gap that predates #5617 — a step blocked at MOUNT previously emitted only `stepSkipped`, leaving that cohort invisible on the event stream entirely. NOTE: this shifts the `step-skipped` baseline down by the denial cohort. That is the intended correction, not a side effect. Also: - A permanently failing outcome write now reports to Sentry instead of only `console.warn` — an invisible durable-write failure is the exact blind spot #5621 is filed about. - The mutation patches `blockedSteps` straight from the arg rather than coercing to `[]`. Convex removes a field patched as `undefined`, so a client too old to report blocked steps leaves the field ABSENT ("could not report") instead of asserting "none were blocked" — a claim that is false for a client that classified denials as skips. - ProActivationChip re-derived outcomes with its own ternary; it now shares `selectAdvanceOutcome`. Behavior is identical today, which is exactly how the two would have drifted. - Schema rollback note: once rows carry `blockedSteps`, deleting the field fails the Convex deploy. Revert the write path, not the schema line. - Four comments still claimed a blocked step resolves as `skipped`. Tests: +5 (mid-flow flush through the real shell, Sentry on permanent failure, progress-revision persistence, omit-clears-rather-than-strands, revision cap top-of-range). All 15 mutations of the new logic verified to redden a test. Claude-Session: https://claude.ai/code/session_011wvTSMvKG7hLkJgRezBJFi
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
benngee85
pushed a commit
to benngee85/VISTA
that referenced
this pull request
Jul 26, 2026
…cket (koala73#5617) (koala73#5629) * feat(activation): record a browser-blocked step in its own durable bucket (koala73#5617) PR koala73#5615 made a denied notification permission move the activation wizard's alerts step into its blocked state, and closed the resulting signal loss at the EVENT level (`pro-activation-step-blocked`). The durable side stayed collapsed: a blocked step landed in `skippedSteps` with no marker, so anything querying persisted per-account state in Convex after the fact could not tell a browser refusal from a voluntary skip -- which is exactly what sizing the push-denial population and deciding whether re-prompting an account is worth anything both need. Adds a fourth bucket rather than widening an existing one, so behavioral consumers are untouched: - `ActivationStepOutcome` gains `blocked`. It maps to `pending` in the summary (never `failed` -- we never attempted a write), counts as `pending` in the exit event, and still shows the finish-setup chip. A record change, not UX. - `buildActivationOutcomeBuckets` gains `blockedSteps` and now routes every outcome explicitly: its `else` catch-all would have persisted the new outcome as a failed write. - `selectStepEvent('blocked')` returns `stepBlocked`, not `stepSkipped`. - New pure `selectAdvanceOutcome(state)` is the single seam for all three ways a step can be advanced past without a confirm (Continue, Escape/dismiss, never reached). Writing `blocked` on only the Continue arm would have made the bucket a sample of "denied users who clicked Continue". Steps blocked at mount now record identically to those blocked mid-flow. - `recordProActivationOutcome` and the schema accept `blockedSteps`: optional server-side for mixed deploys, validated for disjointness like every other bucket, and written on every snapshot (each one is a full replacement). - `ProActivationFlowDependencies.recordOutcome` now uses the service's own `ProActivationOutcomeSnapshot` instead of a hand-copied structural shape that silently described less the moment a bucket was added. All 11 mutations of the new logic were verified to redden a test. Claude-Session: https://claude.ai/code/session_011wvTSMvKG7hLkJgRezBJFi * fix(activation): close the review gaps on the blocked outcome bucket (koala73#5617) Code-review follow-ups on the durable `blocked` bucket. Three of these were real defects in the first commit, two of them CI-red: - e2e/pro-activation.spec.ts asserted the exact outcome payload in two places (`toEqual` on the snapshot object, and `['skipped','skipped','skipped']` for the denied-permission flow). Both would have gone red. `npm run typecheck` does not cover e2e/ — its tsconfig includes only `src` — so nothing local caught this; the spec now passes (24/24, run against a real browser). - A mid-flow denial was never flushed. `handleConfirm`'s `blocked` arm updated in-memory state but, unlike the `verified` arm, never called `recordProgress()`. A subscriber denied by the browser who then closed the tab stayed recorded as a voluntary skip forever — precisely the cohort this bucket exists to size. The denial is now durable the instant it happens, which required raising MAX_PRO_ACTIVATION_OUTCOME_REVISION to `members.length + 2` (one extra progress write is now reachable). - The durable buckets and the funnel events counted different cohorts: a blocked step recorded as `blocked` but still emitted `stepSkipped`, so the funnel's skip count included denials while `skippedSteps` excluded them. A new `reportAdvanceEvent` seam routes the event off the resolved outcome, so each step emits exactly one: `stepSkipped` for a real skip, `stepBlocked` for a refusal. This also closes a gap that predates koala73#5617 — a step blocked at MOUNT previously emitted only `stepSkipped`, leaving that cohort invisible on the event stream entirely. NOTE: this shifts the `step-skipped` baseline down by the denial cohort. That is the intended correction, not a side effect. Also: - A permanently failing outcome write now reports to Sentry instead of only `console.warn` — an invisible durable-write failure is the exact blind spot koala73#5621 is filed about. - The mutation patches `blockedSteps` straight from the arg rather than coercing to `[]`. Convex removes a field patched as `undefined`, so a client too old to report blocked steps leaves the field ABSENT ("could not report") instead of asserting "none were blocked" — a claim that is false for a client that classified denials as skips. - ProActivationChip re-derived outcomes with its own ternary; it now shares `selectAdvanceOutcome`. Behavior is identical today, which is exactly how the two would have drifted. - Schema rollback note: once rows carry `blockedSteps`, deleting the field fails the Convex deploy. Revert the write path, not the schema line. - Four comments still claimed a blocked step resolves as `skipped`. Tests: +5 (mid-flow flush through the real shell, Sentry on permanent failure, progress-revision persistence, omit-clears-rather-than-strands, revision cap top-of-range). All 15 mutations of the new logic verified to redden a test. Claude-Session: https://claude.ai/code/session_011wvTSMvKG7hLkJgRezBJFi
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
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.
Closes #5617.
The gap
PR #5615 routed a denied notification permission into the activation wizard's blocked state and closed the resulting signal loss at the event level (
pro-activation-step-blocked). The durable side stayed collapsed: a blocked step landed inskippedStepswith no marker, so anything querying persisted per-account state in Convex after the fact could not tell a browser refusal from a voluntary skip.That is what the data is actually for — sizing the push-denial population, and deciding whether re-prompting an account is worth anything (it never is, once permission is
denied).Shape
A fourth bucket rather than a widening of an existing one, so behavioral consumers are untouched:
ActivationStepOutcomegainsblocked. It reads aspendingin the summary (neverfailed— we never attempted a write), counts aspendingin the exit funnel event, and still shows the finish-setup chip. A record change, not a UX change.buildActivationOutcomeBucketsgainsblockedStepsand now routes every outcome explicitly: itselsecatch-all would have persisted the new outcome as a failed write.selectAdvanceOutcome(state)is the single seam for all three ways a step is advanced past without a confirm (Continue, Escape/dismiss, never reached). The issue proposed writingblockedonly on theadvance-skiparm; that would have madeblockedStepsa sample of denied users who clicked Continue. Steps blocked at mount — permission already denied before the wizard opened — record identically.recordProActivationOutcome+ schema acceptblockedSteps, validated for disjointness like every other bucket.Deviations from the issue's proposed shape
Both widen correctness rather than scope; flagging them explicitly:
blocked(the issue scoped this toblockedByConfirm). They are the same denial cohort, and likely its larger half.step-skippedis no longer emitted for a denial. The durable buckets and the funnel were counting different cohorts —skippedStepsexcluded denials while the funnel's skip count included them. Each step now emits exactly one event. This shifts thepro-activation-step-skippedbaseline down by the denial cohort. It also fixes a gap predating this issue: a step blocked at mount previously emitted onlystepSkipped, so that cohort was invisible on the event stream entirely.Rollout ordering (please read before merging)
This is a server-contract change, and Convex deploys through a separate workflow (
convex-deploy.yml) from the Vercel frontend. The mutation arg isv.optional, which covers old-client-to-new-backend. The reverse — new client, not-yet-updated backend — rejects the write for the duration of the skew window. It is best-effort and retried, and now reports to Sentry instead of failing silently, but the Convex deploy should land before the Vercel promotion. Three reviewers independently flagged this; the conservative alternative is to split this into two merges (schema+mutation first, client second) if you'd rather not rely on ordering.Schema rollback is one-way: once rows carry
blockedSteps, deleting the field fails the Convex deploy. Revert the write path, not the schema line (noted in-code).Verification
typecheckclean;biomeclean on all changed files; e2e and convex tsc projects show zero new errors (e2e has 22 pre-existingpage.evaluateresolution errors, unchanged).npm run typecheckdoes not covere2e/, whose tsconfig includes onlysrc.tsx: 16748. Two pre-existing failures unrelated to this change: thedist/dashboard.htmlbuilt-output CSS tests (needvite buildfirst) and a load-sensitivepoolSelectionperf-shape timeout verified to fail identically on a clean tree.elsecatch-all, the mid-flow flush, and both event-routing paths.Review
Reviewed by an 11-reviewer pass (correctness, security, adversarial, api-contract, data-migration, reliability, testing, maintainability, standards + always-on). Security returned no findings. Three real defects were found and fixed in the second commit — two CI-red e2e assertions and an unflushed mid-flow denial. The cross-model pass could not run (the local Codex CLI install is broken).
Adjacent work: #5621 has an in-flight branch touching
recordProActivationOutcome; rebase order may matter.https://claude.ai/code/session_011wvTSMvKG7hLkJgRezBJFi