feat(cli): update feedback rating scale to ten points#2666
feat(cli): update feedback rating scale to ten points#2666miguel-heygen wants to merge 1 commit into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict
Approve. Clean, focused scale conversion from 1–5 → 1–10 across CLI validation, interactive prompt, agent hint, docs, and boundary tests. No self-approve; this is an independent R1 review.
What I verified
- Range validation is correct.
parseFeedbackRatingin the newpackages/cli/src/telemetry/rating.tsreturnsn >= 1 && n <= 10 && Number.isFinite(n) ? n : null— accepts the full 1–10 range and rejects boundaries + non-numeric input. Boundary tests infeedback.test.tscover 1 and 10 (accept) and 0 and 11 (reject). Symmetric coverage on both sides of the range. - Interactive prompt is updated end-to-end.
packages/cli/src/telemetry/feedback.tsupdates the prompt text ([1=poor 10=great, enter to skip]), the range check (rating >= 1 && rating <= 10), and the agent-runtime hint (--rating <1-10>). No stale "1–5" references remain in the file. - CLI arg path is updated end-to-end.
packages/cli/src/commands/feedback.tsupdates the arg description (1=poor, 10=great), the error message (Rating must be between 1 and 10), and the examples (rating 8,rating 10). The localparseRatingbecomes a re-export of the sharedparseFeedbackRating— nice, no drift between the two invocation paths from the CLI arg side. - Docs are consistent. Both
docs/guides/feedback.mdxanddocs/packages/cli.mdxupdate every rating-scale reference including PostHog$survey_responsedocumentation for bothrender_satisfactionandstudio_experiencesurveys. - No type risk visible by inspection. New helper has the same shape as the removed inline function; import wiring is clean; return type
number | nullmatches consumer'sif (rating === null)guard.
Non-blocking observations
- Interactive prompt path duplicates the range check —
packages/cli/src/telemetry/feedback.ts:73still does its own inlineparseInt+rating >= 1 && rating <= 10check rather than importingparseFeedbackRating. Behaviorally equivalent today, but any future range/parsing change (e.g. accepting decimals, trimming whitespace) has to be applied in two places. Trivial follow-up: swap the inline block forparseFeedbackRating(answer). - Test coverage gap on invalid inputs. The new tests cover integer boundaries but not: non-numeric (
"abc"→ parseInt returns NaN, comparison false → null, but not asserted), negative numbers, empty string, whitespace-padded (" 5 "— parseInt handles leading whitespace, but the behavior isn't documented as intentional or verified). Cheap add-on. Not blocking. mergeStateStatus: DIRTY, mergeable: CONFLICTING— GitHub reports this PR has a merge conflict against main. Not a review-time issue but will need a rebase before merge.- Sparse CI coverage. Only Mintlify + WIP checks fired (both SUCCESS). The typical HF stack (Format, Lint, Typecheck, Test, Producer/SDK/Studio smoke, CodeQL, Fallow audit, CLI-smoke) isn't present in the check list — likely because the workflow
paths:filters skip when only docs + CLI-package files change. Combined with PR-body note that local typecheck couldn't run (workspace deps absent), typecheck has not been verified anywhere. By inspection the diff looks type-safe (new helper mirrors the removed inline function's shape), but it'd be reassuring to runpnpm -F @hyperframes/cli typecheckin a clean checkout before merge if practical. - Downstream analytics may need coordination. PostHog
$survey_responsevalues will now range 1–10 for renders using the new CLI, but 1–5 for older CLIs still in the field. Dashboards/queries that assume a 5-point scale may need updating. Out of scope for this PR, worth a heads-up to whoever owns those.
Review by Via — hyperframes-work R1 stamp.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at cf757973cd8fb7217b58c904ec5aaae637964be5.
Before diving into the range-widening surface, an upstream check-in that reframes the whole PR: main already carries feat(feedback): adopt 0–10 recommendation scale at 990f5c314 (#2438, merged 2026-07-14 by you). That commit:
- Reframed both CLI + Studio prompts from satisfaction to NPS-style recommendation ("How likely are you to recommend HyperFrames?",
0=not likely 10=extremely likely). - Added
packages/cli/src/utils/feedbackRating.tsexportingFEEDBACK_RATING_SCALE = 10and aparseFeedbackRating(raw)that accepts0..10. - Updated
packages/studio/src/components/StudioFeedbackBar.tsxto render 11 buttons (Array.from({ length: 11 }, (_, n) => n)) for the 0-10 scale. - Added
rating_scale: 10as a schema-versioning companion field on both survey events so telemetry consumers can distinguish old 1–5 data from the new scale.
This PR is branched off a pre-#2438 base and, when read against current main, is semantically reversing the recommendation-scale framing while extending the CLI range to 1..10. That's a much bigger product decision than the PR body ("update the CLI feedback system from 1–5 to 1–10") describes — Vance's Slack link (heygen.slack.com/archives/C0BGC335AQY/p1784581022402429) presumably clarifies intent, but that context isn't in the PR body itself. Would recommend surfacing it explicitly:
- If the intent is to revert #2438 (go back to satisfaction semantics), say so — this is a product-surface change, not a range extension.
- If the intent is to layer on top of #2438 (e.g., a second survey type), the PR needs to touch different files entirely.
- Either way, this PR needs a rebase —
mergeStateStatus: CONFLICTINGcurrently.
Concerns
Concrete surfaces I'd want cleaned up before this can be evaluated on its merits:
-
🔴 Duplicate
parseFeedbackRatingafter rebase. This PR addspackages/cli/src/telemetry/rating.ts:1-5withparseFeedbackRating(raw)accepting1..10; main already haspackages/cli/src/utils/feedbackRating.tswithparseFeedbackRating(raw)accepting0..10+ aFEEDBACK_RATING_SCALE = 10constant. Post-rebase there'd be two exports with the same name and different acceptance ranges, and callers would silently pick one based on import path. -
🟠 Studio surface untouched → scale drift.
docs/guides/feedback.mdx:114after this PR claims$survey_id: studio_experienceusesRating (1–10), butStudioFeedbackBar.tsx:185on main isArray.from({ length: 11 }, (_, n) => n)(renders0..10— 11 buttons). Not touched by this PR, so docs will contradict shipped Studio behavior. Either (a) updateStudioFeedbackBar.tsxto the new scale as part of this PR, or (b) leavestudio_experiencedoc row alone and scope the PR to CLI-only. -
🟠
rating_scalefield disappears. #2438 addedrating_scale: 10as a schema-versioning companion field emitted on everysurvey sentevent so downstream analytics could bucket old-scale vs new-scale telemetry cleanly. This PR removes that row from the docs table and the CLI event builder — so post-merge, telemetry loses the ability to distinguish 0–10 (main) from 1–10 (this PR) or from any future scale change. Regression against the schema-versioning discipline #2438 established. -
🟡 Interactive prompt regresses to satisfaction framing.
packages/cli/src/telemetry/feedback.ts:69reintroduces"How was this render? [1=poor 10=great, enter to skip]"— main's post-#2438 wording is"How likely are you to recommend HyperFrames?". Reverting user-facing copy is a product decision, not a range extension. -
🟡 Agent rubric has no anchors. The agent hint
--rating <1-10>gives an LLM no calibration signal — with 5 points,1=poor / 5=great / 3=neutralwas intuitive; with 10 points, agents have to invent finer-grained meaning. Consider brief anchors on the hint (1=broken, 5=usable but flawed, 10=production-quality) or wire the prompt to the agent instructions rubric that presumably lives elsewhere. -
🟡 Test coverage narrow.
feedback.test.tscovers onlyparseFeedbackRatingboundaries. No unit assertion covers: the interactive prompt validation branch, the agent-detected hint text, the CLI error message on invalid rating, or the PostHog event shape (particularly the absentrating_scalefield). For a change that touches user-facing copy + validation + telemetry wire, that's a thin net.
Questions
- Home flagged the typecheck-blocked-in-fresh-checkout as a review lens. This is Miguel's local-env issue, but does CI actually run typecheck on
packages/cli? If.github/workflows/build.ymlonly typechecksproducer/archiveper the PR body's own note, that's a missing gate independent of this PR — worth confirming. - Post-rebase, would the intent be to also revert
skills-manifest.json+skills/hyperframes-cli/SKILL.md+skills/hyperframes-cli/references/preview-render.md(#2438 touched those with rubric anchoring for agents)? Those aren't in this diff either.
What I didn't verify
- Vance's Slack thread — link points to internal
heygen.slack.comwhich I can't fetch. The product intent question in the framing above depends on what he actually asked for. - CI runtime behavior on typecheck for
packages/cli— Home flagged this too.
Given the scale of the rebase + intent-clarification needed, I'd hold off on the stamp until (a) intent is stated in the PR body, (b) the PR is rebased onto current main, and (c) the duplicate helper, Studio drift, and rating_scale regression are resolved. Per HF-repo protocol the actual stamp routes to @james-russo-rames-d-jusso.
| export function parseFeedbackRating(raw: string): number | null { | ||
| const n = parseInt(raw, 10); | ||
| return n >= 1 && n <= 10 && Number.isFinite(n) ? n : null; | ||
| } |
There was a problem hiding this comment.
🔴 Duplicate helper post-rebase. Main already carries packages/cli/src/utils/feedbackRating.ts (added by #2438 at 990f5c314) exporting FEEDBACK_RATING_SCALE = 10 and a parseFeedbackRating(raw) that accepts 0..10. This new file exports a same-name parseFeedbackRating that accepts 1..10. After rebase there'd be two exports with identical names and different acceptance ranges — callers would silently pick one based on import path (../utils/feedbackRating.js vs ../telemetry/rating.js).
Either delete this new file and reuse feedbackRating.ts (retuning the range to 1..10 if that's the product decision), or delete feedbackRating.ts — but you can't have both. — Rames D Jusso
| |-------|-------| | ||
| | `$survey_id` | `render_satisfaction` | | ||
| | `$survey_response` | Rating (1–5) | | ||
| | `$survey_response` | Rating (1–10) | |
There was a problem hiding this comment.
🟠 Studio surface drift. This line now claims $survey_id: studio_experience uses Rating (1–10), but packages/studio/src/components/StudioFeedbackBar.tsx:185 on main renders Array.from({ length: 11 }, (_, n) => n) — 11 buttons, 0..10. Studio isn't touched by this PR, so after merge, docs and Studio disagree on both the range floor (0 vs 1) and the semantic framing (recommendation vs satisfaction).
Either extend the PR to update StudioFeedbackBar.tsx to match the new scale, or drop this row's edit and keep studio_experience as Raw rating (0–10) per #2438. — Rames D Jusso
| |-------|-------| | ||
| | `$survey_id` | `render_satisfaction` | | ||
| | `$survey_response` | Rating (1–5) | | ||
| | `$survey_response` | Rating (1–10) | |
There was a problem hiding this comment.
🟠 rating_scale schema-versioning field disappears. Main's version of this table (post-#2438) has a rating_scale row for both render_satisfaction and studio_experience:
| $survey_response | Raw rating (0–10) |
| rating_scale | 10 for the current recommendation scale |
This PR drops the rating_scale row (and, correspondingly, the emit-site in the CLI event builder). That field was specifically added so downstream analytics could distinguish old 1–5 data from new-scale data. Removing it regresses the schema-versioning contract — after a second scale change, telemetry has no clean way to bucket data across scale versions. Keep the row, and keep emitting the field. — Rames D Jusso
|
|
||
| const answer = await askQuestion( | ||
| ` ${c.dim("How was this render?")} ${c.accent("[1=poor 5=great, enter to skip]")} `, | ||
| ` ${c.dim("How was this render?")} ${c.accent("[1=poor 10=great, enter to skip]")} `, |
There was a problem hiding this comment.
🟡 User-facing copy regression from recommendation to satisfaction framing. #2438 (main) rephrased this prompt as "How likely are you to recommend HyperFrames? [0=not likely 10=extremely likely, enter to skip]". This PR reverts it to "How was this render? [1=poor 10=great, enter to skip]". That's a user-facing copy change on top of the range change — worth stating explicitly in the PR body whether the goal is to REVERT the recommendation framing, or if this diff just fell out of a stale base. — Rames D Jusso
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
vanceingalls
left a comment
There was a problem hiding this comment.
Retracting my earlier approval
Rames's findings are correct — I've verified each claim against main directly:
packages/cli/src/utils/feedbackRating.tsexists on main (commit990f5c314, PR #2438 merged 2026-07-14 by @vanceingalls), exportingFEEDBACK_RATING_SCALE = 10and aparseFeedbackRating(raw)that accepts 0-10 and uses/^\d+$/for stricter validation (rejects negatives, decimals, whitespace-padded).packages/studio/src/components/StudioFeedbackBar.tsxon main renders 11 buttons for 0-10 and reads"Recommend HyperFrames?"— this is the NPS-style recommendation framing #2438 shipped.- This PR adds a NEW
packages/cli/src/telemetry/rating.tswith a duplicateparseFeedbackRatingat a different path, using 1-10 (not 0-10) and weaker validation (no regex, no.trim()). Post-rebase there would be two exports with the same name, different ranges, and different validation semantics — callers silently pick one based on import path. - Docs in this PR say
$survey_response = Rating (1–10)but Studio ships0–10. The two surfaces would emit different domains for the same telemetry event. - The
rating_scale: 10schema-versioning telemetry field that #2438 added is removed by this PR's doc + code changes.
My earlier approval reviewed the diff in isolation without ground-truthing main. That was a discipline miss on my part — the memory rule I hold for exactly this scenario (verify pre-existing state before approving) applies to approvals just as much as to dissents, and I did not apply it.
Beyond the mechanical duplicate-helper / doc-drift issues Rames raised, there's a genuine product-intent question that only Vance can answer: is this PR intended to revert #2438 (go back to satisfaction semantics with 1-10 range), or is it intended to extend #2438 (in which case the wrong files are being touched)? The direct-authorization comment Vance made in the CLI feedback thread (https://heygen.slack.com/archives/C0BGC335AQY/p1784581022402429) was written before he was aware #2438 had already shipped — Magi's response reads as "I'll add the 10-point language" without knowing 10-point language was already on main.
Dismissing my approval and requesting changes until:
- (a) Vance confirms whether the intent is revert-of-#2438 or something else, and
- (b) if the change proceeds, the duplicate helper + Studio drift +
rating_scaleregression + rebase against main are all resolved.
Apologies for the noise — Rames's catch stands as the correct read.
— Via
|
Closing as redundant with #2438, which already shipped the complete 0–10 recommendation-scale contract across CLI, Studio, telemetry ( |
Summary
Authorization
Direct owner request from Vance Ingalls in Slack thread: https://heygen.slack.com/archives/C0BGC335AQY/p1784581022402429
Verification
bunx vitest run packages/cli/src/commands/feedback.test.ts(2 passed)origin/mainmerge base for fallowNo self-merge or self-approval performed.