Skip to content

feat(cli): update feedback rating scale to ten points#2666

Closed
miguel-heygen wants to merge 1 commit into
mainfrom
fix/feedback-rating-scale-10
Closed

feat(cli): update feedback rating scale to ten points#2666
miguel-heygen wants to merge 1 commit into
mainfrom
fix/feedback-rating-scale-10

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Summary

  • update CLI and interactive feedback prompts to use a 1–10 satisfaction scale
  • validate ratings from 1 through 10 and add boundary tests
  • update CLI and feedback documentation, including agent instructions

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)
  • formatting and lint pre-commit checks passed
  • full typecheck could not run in this checkout because workspace dependencies and Node typings are absent; hook also reported no origin/main merge base for fallow

No self-merge or self-approval performed.

@mintlify

mintlify Bot commented Jul 20, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Jul 20, 2026, 9:06 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Range validation is correct. parseFeedbackRating in the new packages/cli/src/telemetry/rating.ts returns n >= 1 && n <= 10 && Number.isFinite(n) ? n : null — accepts the full 1–10 range and rejects boundaries + non-numeric input. Boundary tests in feedback.test.ts cover 1 and 10 (accept) and 0 and 11 (reject). Symmetric coverage on both sides of the range.
  2. Interactive prompt is updated end-to-end. packages/cli/src/telemetry/feedback.ts updates 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.
  3. CLI arg path is updated end-to-end. packages/cli/src/commands/feedback.ts updates 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 local parseRating becomes a re-export of the shared parseFeedbackRating — nice, no drift between the two invocation paths from the CLI arg side.
  4. Docs are consistent. Both docs/guides/feedback.mdx and docs/packages/cli.mdx update every rating-scale reference including PostHog $survey_response documentation for both render_satisfaction and studio_experience surveys.
  5. No type risk visible by inspection. New helper has the same shape as the removed inline function; import wiring is clean; return type number | null matches consumer's if (rating === null) guard.

Non-blocking observations

  1. Interactive prompt path duplicates the range checkpackages/cli/src/telemetry/feedback.ts:73 still does its own inline parseInt + rating >= 1 && rating <= 10 check rather than importing parseFeedbackRating. 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 for parseFeedbackRating(answer).
  2. 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.
  3. 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.
  4. 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 run pnpm -F @hyperframes/cli typecheck in a clean checkout before merge if practical.
  5. Downstream analytics may need coordination. PostHog $survey_response values 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts exporting FEEDBACK_RATING_SCALE = 10 and a parseFeedbackRating(raw) that accepts 0..10.
  • Updated packages/studio/src/components/StudioFeedbackBar.tsx to render 11 buttons (Array.from({ length: 11 }, (_, n) => n)) for the 0-10 scale.
  • Added rating_scale: 10 as 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: CONFLICTING currently.

Concerns

Concrete surfaces I'd want cleaned up before this can be evaluated on its merits:

  • 🔴 Duplicate parseFeedbackRating after rebase. This PR adds packages/cli/src/telemetry/rating.ts:1-5 with parseFeedbackRating(raw) accepting 1..10; main already has packages/cli/src/utils/feedbackRating.ts with parseFeedbackRating(raw) accepting 0..10 + a FEEDBACK_RATING_SCALE = 10 constant. 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:114 after this PR claims $survey_id: studio_experience uses Rating (1–10), but StudioFeedbackBar.tsx:185 on main is Array.from({ length: 11 }, (_, n) => n) (renders 0..10 — 11 buttons). Not touched by this PR, so docs will contradict shipped Studio behavior. Either (a) update StudioFeedbackBar.tsx to the new scale as part of this PR, or (b) leave studio_experience doc row alone and scope the PR to CLI-only.

  • 🟠 rating_scale field disappears. #2438 added rating_scale: 10 as a schema-versioning companion field emitted on every survey sent event 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:69 reintroduces "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=neutral was 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.ts covers only parseFeedbackRating boundaries. 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 absent rating_scale field). 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.yml only typechecks producer/archive per 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.com which 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.

Review by 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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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

Comment thread docs/guides/feedback.mdx
|-------|-------|
| `$survey_id` | `render_satisfaction` |
| `$survey_response` | Rating (1–5) |
| `$survey_response` | Rating (1–10) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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

Comment thread docs/guides/feedback.mdx
|-------|-------|
| `$survey_id` | `render_satisfaction` |
| `$survey_response` | Rating (1–5) |
| `$survey_response` | Rating (1–10) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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]")} `,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

@mintlify

mintlify Bot commented Jul 20, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟡 Building Jul 20, 2026, 9:05 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retracting my earlier approval

Rames's findings are correct — I've verified each claim against main directly:

  1. packages/cli/src/utils/feedbackRating.ts exists on main (commit 990f5c314, PR #2438 merged 2026-07-14 by @vanceingalls), exporting FEEDBACK_RATING_SCALE = 10 and a parseFeedbackRating(raw) that accepts 0-10 and uses /^\d+$/ for stricter validation (rejects negatives, decimals, whitespace-padded).
  2. packages/studio/src/components/StudioFeedbackBar.tsx on main renders 11 buttons for 0-10 and reads "Recommend HyperFrames?" — this is the NPS-style recommendation framing #2438 shipped.
  3. This PR adds a NEW packages/cli/src/telemetry/rating.ts with a duplicate parseFeedbackRating at 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.
  4. Docs in this PR say $survey_response = Rating (1–10) but Studio ships 0–10. The two surfaces would emit different domains for the same telemetry event.
  5. The rating_scale: 10 schema-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_scale regression + rebase against main are all resolved.

Apologies for the noise — Rames's catch stands as the correct read.

— Via

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Closing as redundant with #2438, which already shipped the complete 0–10 recommendation-scale contract across CLI, Studio, telemetry (rating_scale: 10), docs, skills, and tests. The linked 5/10 report confirms the current scale is active. This stale-base PR would instead regress the contract to 1–10 satisfaction semantics and drop schema versioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants