feat(studio): ask for feedback when a render ends, not on a session counter - #3205
Conversation
…ounter The feedback bar fired on a session count, so it interrupted at a moment with no subject, and it emitted nothing when it appeared or was dismissed. That made the collection rate impossible to diagnose: a prompt nobody answers and a prompt that never renders looked identical. The prompt now fires when a render this tab started reaches its outcome, which is the same moment the CLI asks. A failed export and a crash skip the 0-10 score and ask what happened instead, since rating an export you never received is a question with no useful answer. Replaces the 32px inline bar with a card in the existing toast stack, so it no longer pushes the preview up mid-task, and reuses StudioToast's glass treatment rather than adding a second visual language. Reports now carry what is needed to act on them: a breadcrumb trail of the run-up, the render settings and outcome, and how the project was made. Breadcrumbs come from one hook in trackEvent, so every existing and future studio event joins the trail without its own instrumentation. Eligibility lives in one module: once per tab, thirty days after an answer, seven after a dismissal, and never when telemetry is off, because prompting someone whose response would be dropped wastes their attention. VITE_HYPERFRAMES_NO_FEEDBACK=1 still disables it entirely. Breadcrumbs and provenance carry names, enums and counts only. Values come from a fixed allowlist, so comments, paths, stack traces and project titles cannot reach them.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!scaffolded) return; | ||
|
|
||
| try { | ||
| const res = await fetch(`/api/projects/${projectId}/files/${encodeURIComponent(CONFIG_FILE)}`); |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Findings
Overall this is a careful rewrite. Trigger, dedup, telemetry-opt-out, and privacy work are solid. All findings below are discussion-level, none block merge.
1. useRenderQueue trigger has no unit test. packages/studio/src/components/renders/useRenderQueue.ts:~383-413 (the new useEffect over jobs) encodes the core hypothesis of this PR: fire once per terminal outcome, skip cancelled, skip history-loaded jobs (via sessionJobs.current), never re-fire (via promptedJobIds.current). Verified only by manual live testing. A regression here silently breaks the whole premise. A dedicated test — synthetic jobs list transitioning rendering → complete, → failed, → cancelled, plus a history-loaded job whose id is absent from sessionJobs — is cheap insurance. Not blocking, strongly recommended for a follow-up.
2. Stale ids in trackStudioFeedback doc-comment. packages/studio/src/telemetry/events.ts — the new question field JSDoc lists "remove" | "borrow" | "fix" | "detractor" | "failure". Actual ids emitted are remove | workaround | fix | detractor | failure | crash. borrow is fabricated; workaround and crash are missing. Nit, but this is the contract for whoever writes the PostHog insight.
3. Cross-project provenance race. packages/studio/src/components/feedback/projectProvenance.ts uses a module-scope let snapshot keyed by nothing. packages/studio/src/hooks/useFileTree.ts fires void captureProjectProvenance(projectId, ...) on every project load without cancellation. If the user switches projects mid-fetch and renders quickly, feedback attaches the previous project's project_scaffolded / project_authoring_skill / counts. Rare, real. Cheap fix: gate the write with the current projectId (capture at call time, compare on resolve). Module scope is right for surviving the crash unmount; the projectId gate is the missing half.
4. Stranded storage keys from the old bar. Old StudioFeedbackBar wrote hyperframes-studio:feedbackSessionCount, feedbackLastPromptedAt, feedbackSessionCounted. The new trigger uses different keys (feedbackAnsweredAt, feedbackDismissedAt, feedbackAskedThisSession). No cleanup pass — every existing user carries dead bytes forever. Also no migration: someone who dismissed the old bar last week gets a fresh render-end prompt immediately (arguable this is intentional, since the old dismissal wasn't opinion about the new prompt). Consider a one-shot localStorage.removeItem sweep at trigger-module load. Nit.
5. Crash prompt suppressed by an earlier session dismissal — worth a discussion box. In CrashFeedbackPrompt.tsx and feedbackTrigger.ts isEligible, a crash goes through the same askedThisSession + cooldown gate as a render prompt. So a user who dismissed a render_complete ask this session gets no crash prompt when Studio then crashes. Inline rationale is consistency; the counter-argument is that a crash is a categorically higher-signal moment and dismissing "recommend HyperFrames?" is not consent to being asked about crashes. Consider letting reason === "crash" bypass the askedThisSession slot (but still honor answeredAt cooldown). Not blocking; worth naming the decision explicitly in the commit or a code comment.
6. sessionJobs.current and promptedJobIds.current grow unbounded. useRenderQueue.ts never clears these on project switch or across a long-lived tab with hundreds of renders. Small memory tail, real. Cheap sweep on projectId change (existing cleanup effect at bottom of the hook) is the natural spot.
7. role="dialog" without focus management. StudioFeedbackCard.tsx root uses role="dialog" with aria-label. It's a corner toast, not modal — no focus trap, no autofocus on open, root div isn't focusable so Escape only lands once a child owns focus. role="region" or role="complementary" matches reality and avoids the a11y contract violation. Nit.
CI note (non-blocking)
Detect changes reports FAILURE in 3 slots with exit code 1, but downstream shards ran (33 SUCCESS / 66 SKIPPED — the skips are exactly what a working Detect-changes produces). Appears to be a preflight-reporter infra issue, not a code failure. main at dc438311 is green on all checks. Worth spot-checking whether required-check gating trips on it.
Verdict
APPROVE. Trigger discipline (single owner, session cap, per-job dedup, telemetry-opt-out honored, cancelled skipped, history-loaded skipped) is well thought through. Privacy work is genuinely careful (breadcrumb allowlist + length gate, slug-only skill, no titles/paths, tests assert exclusion). The funnel events fix a real observability hole in the old bar. Findings above are quality tail — the untested render-queue effect (1) is the one I'd most like to see landed as a follow-up, and the doc-comment (2) plus provenance-race (3) are worth a five-minute pass. Nothing blocks merge.
— Via
The deletion guard is absolute by design, and the replacement shares too little content with StudioFeedbackBar.tsx for git to pair the two as a rename, so an intentional removal reads as loss. Adds a named allowlist rather than a flag or an env var: a blanket override would be reached for by the branch deleting something by accident, which is the case the guard exists for. An entry has to name the path and say why, so every intentional removal is a line in a diff someone reviews.
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at d195692d.
Read the diff in five dimensions (trigger substitution, removed-check side effects, observability parity, privacy allowlist, wire-in + code quality). Vance's earlier review (approve at same SHA) covers most of what I'd otherwise be flagging inline — I strongly concur on:
useRenderQueueeffect has no unit test (his #1) — this is the crown-jewel seam per your own PR body; a table-driven test overrendering → complete/failed/cancelledtransitions + a history-loaded job is the highest-ROI follow-up.questiondoc-comment drift attelemetry/events.ts:158(his #2) — the doc says"remove" | "borrow" | "fix" | "detractor" | "failure"but the code emitsremove | workaround | fix | detractor | failure | crash;borrowdoesn't exist andworkaround+crashare missing.useFileTree.ts:34provenance race (his #3) —void captureProjectProvenance(projectId, ...)fires without cancellation. Module scope is right for surviving the crash unmount, but a projectId re-check on resolve is the missing half.- Stranded old-bar storage keys (his #4) and
role="dialog"a11y contract (his #7). - His #5 (crash prompt suppressed by an earlier session dismissal in the same tab) is genuinely worth naming as an explicit decision — a crash is a categorically higher-signal moment than a satisfied render, and the rationale for gating them behind the same
askedThisSessioncap deserves aponytail:note inisEligibleso a future reader doesn't quietly loosen it.
I've added inline comments only for the concerns I don't see in Vance's set. Two of them are cross-cutting enough to belong here in the body rather than a specific line:
Cross-cutting concerns
1. PostHog Surveys → plain product event is a silent break for existing dashboards. The old trackStudioFeedback emitted "survey sent" with $survey_id: "studio_experience", $survey_response, $survey_response_2 — the PostHog Surveys product's own event shape. The new code deliberately opts out ("Plain product event, not a PostHog survey response" at events.ts:170-172) and emits studio_feedback. That's a design call I think is right — the old event was Surveys-shaped but the old bar was never a real Survey — but any HeyGen PostHog Insight, Funnel, warehouse export, or the Surveys UI itself that filters on event = "survey sent" OR properties.$survey_id = "studio_experience" OR properties.$survey_response silently stops receiving data on rollout. Given the old bar has been running for months, this is worth doing before merge, not after: grep the analytics / dashboards / Snowflake config for survey sent and studio_experience, either recreate the consumers on studio_feedback or confirm nobody depended on it. If the funnel had five responses over three months (per Vance's read), the practical blast radius may be zero — but the audit is what tells you that, not the assumption.
2. Rating scale silently flips 1-5 → 0-10. Old bar wrote $survey_response: <1-5 int>; new event writes rating: <0-10 int> with an explicit rating_scale: 10 co-property (events.ts:181) — good defensive design, but the co-property is only load-bearing if downstream consumers read it. Any alert like "avg rating < 3", any anomaly detector fit to the old distribution, any dashboard's default numeric filter now means the opposite of what it did last week. The PR body enumerates the wire changes carefully in the "Before / after" table but doesn't call out the scale change specifically. Worth a line in the PR body / commit message so the analytics team can either update their queries or accept the flip explicitly.
Nits (inline)
- Slug check on breadcrumbs is length-only, not shape — inline on
breadcrumbs.ts. - Clock-skew cooldown lockout on future-dated timestamps — inline on
feedbackTrigger.ts. - StrictMode:
markAskedThisSession()fires before the listener actually receives the request — inline onfeedbackTrigger.ts. - Dismiss events carry no
contextwhile submit does — inline onevents.ts. useRenderQueueeffect re-scans on every SSE progress tick (idempotent, just wasteful) — inline._interview_clickdropsrender_idwhile every other event carries it — inline.- Stale mock (body-only because the file isn't in the diff):
packages/studio/src/components/EditorShell.selectionSync.test.tsx:55stillvi.mock("./StudioFeedbackBar", () => ({ StudioFeedbackBar: () => null })).StudioFeedbackBaris deleted in this PR. Vitest tolerates hoisted mocks for modules that are never imported, so this doesn't fail — but it silently rots. Cheap cleanup while the seam is fresh.
Minor PR-body observation
"The card wears StudioToast's glass treatment and joins its stack, so there is no second visual language and no new CSS." — the "no second visual language" claim holds by inspection, but the glass paint (linear-gradient(135deg, rgba(38,38,38,0.55)…) + backdropFilter: blur(16px) saturate(1.6)) is inlined in StudioFeedbackCard.tsx:214-228 as CSS-in-JS, not a shared token/util. Technically new CSS, just co-located. Minor.
What I didn't verify
- Did not run Studio live against the running production bundle to confirm the reported end-to-end payload shape. Trusting Miguel's manual verification.
- Did not audit HeyGen's PostHog dashboards / warehouse exports for consumers of
"survey sent"— that's the ask in concern #1 and is yours to run. - Did not attempt to force a real crash to exercise the crash prompt end-to-end; the component test coverage there is thin (renders the card but doesn't drive submit through the boundary) — flagging as a follow-up rather than a blocker, agreeing with Vance's #1 shape.
Otherwise LGTM on the code. The concerns above are shipping-side (dashboards) and text (PR body scale mention) rather than code changes.
| function detailFor(properties: Record<string, unknown>): string { | ||
| for (const key of DETAIL_KEYS) { | ||
| const value = properties[key]; | ||
| if (typeof value === "string" && value.length > 0 && value.length <= 24) return `:${value}`; |
There was a problem hiding this comment.
🟠 Privacy: "slug" boundary is length-only, not shape.
The PR body promises that "comments, file paths, stack traces and project titles cannot reach [breadcrumbs] even if a future event carries one". The code enforces something narrower — an 8-key allowlist plus value.length <= 24 — which is genuinely strong against the four PII classes named IN their typical shape (long strings, newlines, paths). But it doesn't hold in general against short PII-shaped values under an allowlisted key. Concrete examples that would slip through today:
trackEvent("studio_x", { reason: "/etc/passwd" })— 11 chars, key allowlisted → surfaces asx:/etc/passwdin the breadcrumb.trackEvent("studio_x", { reason: "boss@corp.com" })— 13 chars → surfaces.trackEvent("studio_x", { via: "C:\\Users\\Alice" })— 15 chars → surfaces.
This requires a future developer to actively assign PII to one of the eight discriminator keys, so the practical risk today is a callsite bug rather than a design hole. But two things to consider:
(a) The four PII classes tested (breadcrumbs.test.ts:41-59) are comment, error_message, stack_trace — three keys, none of which are in DETAIL_KEYS to begin with, so their exclusion is trivially true by the key allowlist. projectTitle (the fourth PII class in the PR body enumeration) isn't tested by name. A test that INJECTS a short PII-shaped value under an allowlisted key (e.g. reason: "/etc/passwd") would either lock the guarantee or document that this shape is knowingly permitted.
(b) Cheap tighten: replace the length gate with a slug regex (something like /^[a-z0-9][a-z0-9_-]{0,23}$/i) — matches the authoringSkill regex in projectProvenance.ts:22-23, brings the two mechanisms into parity, and makes the PR body promise literal.
Either resolution is fine (test-only, or tighten the check); the current state (strong promise + weaker check + partial tests) is what worries me. [[feedback_code_vs_stated_contract]].
— Rames D Jusso
| // drop on the floor. Ask only the people we can actually hear. | ||
| if (!browserTelemetryAllowed()) return false; | ||
| if (askedThisSession()) return false; | ||
| if (now - readTimestamp(STORAGE_KEYS.answeredAt) < ANSWERED_COOLDOWN_MS) return false; |
There was a problem hiding this comment.
🟠 Clock-skew cooldown lockout.
if (now - readTimestamp(STORAGE_KEYS.answeredAt) < ANSWERED_COOLDOWN_MS) return false;
if (now - readTimestamp(STORAGE_KEYS.dismissedAt) < DISMISSED_COOLDOWN_MS) return false;Repro: user answers once with system clock set to a future date (accidental clock roll, calendar app + system time desync, timezone travel + wall-clock write, whatever) — writeTimestamp stores a future ms value. Later, back on real wall-clock: now - future is negative, which is always < ANSWERED_COOLDOWN_MS, so the cooldown is treated as still active until real time catches up to the future write. Since ANSWERED_COOLDOWN_MS = 30d, the effective lockout can be arbitrarily long depending on how far the future write landed.
Cheap fix: if (Math.max(0, now - stored) < COOLDOWN) return false; or normalize the write side (writeTimestamp(k, Math.min(now, ...))) or treat future stored values as 0 (never-answered) on read. Any of the three is a one-line change and closes the edge cleanly.
Low probability, but the failure mode is silent — the user just never sees the card again for months, which is exactly the population we DON'T want to lose feedback from.
— Rames D Jusso
| */ | ||
| export function requestStudioFeedback(request: FeedbackRequest): void { | ||
| if (!listener) return; | ||
| if (!isEligible(Date.now())) return; |
There was a problem hiding this comment.
🟠 Mark-before-listener ordering is StrictMode-hostile and slightly wrong-shape even outside Strict.
if (!listener) return;
if (!isEligible(Date.now())) return;
markAskedThisSession();
listener(request);The session slot is claimed before the listener actually delivers to the user. Two failure shapes:
-
React 18 StrictMode double-invoke (dev + any Strict-wrapped test): first mount subscribes,
requestStudioFeedbackfires,markAskedThisSession()writes"1",listener(request)sets card state, StrictMode simulates unmount →unsubscribe()clears the listener, then second mount subscribes fresh. If the next request comes in immediately (e.g. fromuseRenderQueue's re-fired effect),askedThisSession()reads"1"and drops it at:237— the card rendersnull. Prod is unaffected today becausepromptedJobIdson the caller side also dedupes and the request is not re-fired, but any Strict-wrapped integration test on this seam will flake. -
Even in prod, if
listenerthrows (React error in the card's set-state during a subsequent render), the session slot is still marked — the user is effectively banned from feedback for the rest of the tab without ever having seen a card.
Fix shape: mark AFTER the listener returns, or fold the mark into the listener's own "I actually rendered" side effect. Something like:
listener(request);
markAskedThisSession();or, safer:
listener({ ...request, onDelivered: markAskedThisSession });and have StudioFeedbackCard call onDelivered in the effect that actually renders the request. Not blocking, but worth cleaning up while the seam is fresh.
— Rames D Jusso
| }, | ||
| ): void { | ||
| trackEvent("studio_feedback_dismissed", { | ||
| reason: ctx.reason, |
There was a problem hiding this comment.
🟠 Dismiss events carry no context, while submit does.
trackStudioFeedbackDismissed sends reason + render_id + via + had_rating + source. trackStudioFeedback (submit) additionally spreads { ...projectProvenance(), ...ctx.context } (StudioFeedbackCard.tsx:181) — render settings, provenance flattened, breadcrumbs, doctor summary.
Given dismissals will 5-10× outnumber submits (the whole point of the funnel is that most people close the card), the largest cohort in the new instrumentation can only be broken down by reason (three buckets: complete / failed / crash) and via (three buckets: close / escape / timeout). Anyone asking "do dismissals correlate with render-duration percentile? / project scaffolding? / composition count?" needs context on the dismiss event too — otherwise the biggest slice of the funnel is opaque.
Suggested shape:
trackEvent("studio_feedback_dismissed", {
reason: ctx.reason,
render_id: ctx.render_id,
via: ctx.via,
had_rating: ctx.had_rating,
source: "studio",
...projectProvenance(),
...(ctx.context ?? {}),
});Dismiss context has the same privacy shape as submit context (allowlist + length gate), so no new PII risk. Not a blocker — the funnel works without it — but the analytics improvement is disproportionate for two lines.
— Rames D Jusso
| }, | ||
| }); | ||
| } | ||
| }, [jobs]); |
There was a problem hiding this comment.
🟡 Perf nit: the feedback-trigger effect re-scans all jobs on every SSE progress tick.
The useEffect(..., [jobs]) at :387-411 runs on every reference change of jobs. jobs gets a new reference on every progress event (setJobs(prev => ...) at :272-286 for progress + :288-305 for terminal + :293-303 for onerror), so during an active render this iterates the full job list ~10-60x/s to find nothing new (guarded by promptedJobIds).
Idempotent, so no correctness issue — just a hot path that Chrome DevTools' React profiler will notice if the tab is running many concurrent renders. Cheap fix: split into a useEffect gated on a memoized selector like useMemo(() => jobs.filter(j => TERMINAL_STATUSES.has(j.status)), [jobs]), so the effect only fires when at least one job transitions terminal.
Skippable if there's no observed perf issue in practice. Flagging so the option is on the table.
— Rames D Jusso
| export function trackStudioFeedbackInterviewClick(ctx: { reason: string }): void { | ||
| trackEvent("studio_feedback_interview_click", { | ||
| reason: ctx.reason, | ||
| source: "studio", |
There was a problem hiding this comment.
🟡 _interview_click drops render_id while every other event in the funnel carries it.
export function trackStudioFeedbackInterviewClick(ctx: { reason: string }): void {
trackEvent("studio_feedback_interview_click", {
reason: ctx.reason,
source: "studio",
});
}Funnel queries that join shown → submit → interview_click for a given render outcome have to walk via distinct_id for that last hop, which is workable but noisier than the two-hop shown → submit join. Preserving render_id here would let the interview-click be attributed to a specific render outcome cleanly.
Nit — the interview link is a submit-flow tail and the caller already has the context via StudioFeedbackCard's state.
— Rames D Jusso
4 participants
What
Studio's feedback prompt now fires when a render finishes or fails, instead of on a session counter, and the reports it collects carry enough context to act on.
studio_feedback_shown/studio_feedback_dismissed/studio_feedback_interview_click, so the funnel is visibleBefore / after
The old bar's rating numbers are
neutral-600onneutral-900/80, which is why it reads as a disabled row rather than a control.In the running Studio — bottom-right, sharing the toast stack, hovering a chip explains it on the line above the input:
On the crash screen — the prompt the error boundary renders, asking what the user was doing rather than what went wrong, since the stack trace already covers the latter:
neutral-600neutral-400active:scale-[0.97], 150ms ease-outWhy
The old bar fired on a session count, so it interrupted at a moment with no subject: nothing the user had just done, nothing to have an opinion about. It also emitted nothing when it appeared or when it was dismissed, which made the collection rate impossible to diagnose. A prompt nobody answers and a prompt that never renders looked identical from the outside.
Visually it read as a disabled row: 11 buttons at 11px in
neutral-600on a dark strip, with no resting affordance. And appearing mid-task pushed the whole preview stack up, which the old code carried a comment apologising for.Separately, the reports it did collect were not actionable. A comment says what went wrong; it almost never says how to get there.
How
One trigger, one owner.
feedbackTriggerowns eligibility and nothing else does: once per tab, thirty days after an answer, seven after a dismissal, never when telemetry is off (prompting someone whose response we would then drop wastes their attention).VITE_HYPERFRAMES_NO_FEEDBACK=1still disables it entirely.One hook, every failure path. The trigger watches the render job list rather than each of the four places a render can finish (server rejection, unreachable server, SSE terminal event, SSE connection drop), so paths added later are covered without touching the trigger. Renders loaded from disk history never fire it.
Reuses what exists. The card wears
StudioToast's glass treatment and joins its stack, so there is no second visual language and no new CSS. The rating row is native radios, which gives arrow-key navigation, grouping and labels for free.One question each, rotated across users. A corner card that asks three things gets answered by nobody. Each person gets one follow-up with one-tap answers, explained on a reserved line rather than a floating tooltip (the card is 340px in a corner; a bubble above the chips lands on the question, below lands on the input). Detractors are never given a rotated question, because they already have a specific complaint. Every option was checked against the code: an option naming a feature Studio already has would collect taps meaning "I could not find it", which is indistinguishable afterwards from "it does not exist".
Breadcrumbs cost one line. Every studio event already flows through
trackEvent, so recording the trail there needs no new instrumentation and stays correct as events are added.Provenance lives outside React. A crash unmounts the tree, so it is captured when the project loads and read from module scope when the crash prompt renders.
Privacy
Breadcrumbs and provenance carry names, enums and counts only. Values are copied from a fixed allowlist of short keys, and anything longer than a slug is dropped rather than truncated, so comments, file paths, stack traces and project titles cannot reach them even if a future event carries one. Tests assert this.
Where these responses land
Studio feedback goes to PostHog and nowhere else, which is what it did before this change too.
Worth stating because the CLI behaves differently:
hyperframes feedbackalso forwards to the backend feedback endpoint viasubmitFeedback, on top of its PostHog event. Studio has never used that path, before or after this PR, so if you read CLI feedback anywhere other than PostHog, Studio responses will not show up there.Nothing here changes that either way. Whether the two surfaces should share a delivery path is a product question, not a defect in this change, and closing it would need a field on the backend DTO: it is shaped around
cli_version, and Studio reports from a crash or a failed export deliberately carry no rating.Test plan
Unit — 39 new tests: trigger eligibility and cooldowns, the detractor override, rotation, preset shape and the no-brands rule, breadcrumb rolling and privacy, provenance parsing and its failure modes, and the crash boundary rendering the prompt with no rating input.
Live — both render paths driven end to end against a running Studio on a production bundle, with real renders. Every PostHog request was intercepted and dropped, so nothing reached the project. Verified the emitted payload for a finished render, a failed export, the rotated follow-ups, each chip's hint, and the interview link.
Not covered — no live capture of a spontaneous crash. Three attempts to force one failed because Studio's guards held and it kept rendering, so the crash path is verified by component tests rather than by driving it. Touch devices see chip labels without hints, since the hint is revealed on hover and focus.