Skip to content

chore(studio): add anonymous usage events#1765

Merged
miguel-heygen merged 1 commit into
mainfrom
eg-08-chores
Jun 27, 2026
Merged

chore(studio): add anonymous usage events#1765
miguel-heygen merged 1 commit into
mainfrom
eg-08-chores

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Studio chores: telemetry, copy, log cleanup (8/8)

  • anonymous usage events for keyframe / group / shortcut actions
  • timeline keyframe button reads "Add keyframe (K)" with the shortcut hint
  • remove debug logging

Top of the stack.

@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 6038fd68. Three commits: anonymous usage events for keyframe/group/shortcut + manual double-click detection on preview + drill-in escape, timeline-toolbar shortcut hint copy, and debug-log cleanup. studioTelemetry.ts already exists on main (added in #1643) — this PR is "extend coverage", not "introduce telemetry", so the consent/PII shape isn't net-new for this review.

🟠 Concern — telemetry coverage gap (failure-path / wrong-handler)

  • The canonical "Add keyframe" path doesn't fire a telemetry event. useGsapSelectionHandlers.ts:201-215 adds trackStudioEvent("keyframe", { action: "add", property }) to handleGsapAddKeyframe — but the toolbar K-button + useEnableKeyframes flow goes through handleGsapAddKeyframeBatch (useGsapSelectionHandlers.ts:217-225, called from useEnableKeyframes.ts:229-232), which has no telemetry. Per the feedback_observability_pr_failure_path_coverage lens this is the same class of bug: the PR's stated goal ("anonymous usage events for keyframe / group / shortcut actions") will dramatically under-count the most-common add-keyframe path — the K shortcut. Suggested fix: mirror the same trackStudioEvent("keyframe", { action: "add", property }) (or action: "add_batch" with the property count) at the top of handleGsapAddKeyframeBatch.

🟡 Concerns

  • Stray IIFE wrap in domEditingGroups.ts:30-40. The resolveGroupCapture body got wrapped in const result = ((): GroupCapture => { ... })(); return result; with zero behavior change. Reads like a leftover from inserting a console.log(result) mid-function during debugging, then removing the log without unwinding the IIFE. Worth inverting back to early returns — same code, 3 fewer lines, no indirection for the reader. Doesn't belong in a "remove debug logging" commit but ends up there.

  • Toolbar copy regression in the "none" state. TimelineToolbar.tsx:141 — the keyframeState === "none" tooltip changed from "Enable keyframes" to "Add keyframe (K)". Per useKeyframeToggle at TimelineToolbar.tsx:37-69, this state fires when there's no kfAnim for the selection — clicking initializes the keyframe track (via useEnableKeyframes), it doesn't add to an existing one. The previous "Enable keyframes" was accurate; the new copy describes the wrong action for users hitting this state first. Suggest "Enable keyframes (K)" to preserve the verb while still exposing the shortcut.

🟢 Questions

  • Group-action telemetry fires before the await. useDomEditSession.ts:309 and :320 fire trackStudioEvent before void groupSelection(members) / void ungroupSelection(sel) resolve. This is consistent with the "track intent, not success" pattern elsewhere in the file — but worth a sanity check: is a downstream failure (e.g. ungroup against a deeply-nested DOM that the GSAP strip can't handle) common enough that counting these as successes would mislead the dashboard? Acceptable either way; flagging because the existing useEditorSave.ts / useDomEditCommits.ts events DO distinguish success/failure.

  • keyboard_shortcut action coverage asymmetry. useAppHotkeys.ts:195-200copy only fires when handleCopy() returns true (correct: nothing was copied otherwise). But paste (:204) and cut (:211) fire unconditionally / on intent, which over-counts. Probably fine for usage analytics; flagging because the "what fraction of paste events resulted in a no-op" question can't be answered from this shape.

🔵 Nits

  • GestureRecordControl.tsx:46-47 em-dashes () became double-dashes (--) — almost certainly a stray Graphite restack / line-ending artifact. Other commits in this stack use . Quick revert.

  • keyboard_shortcut properties are stable strings, fine for PostHog cardinality. The keyframe.property field on handleGsapAddKeyframe is also bounded (CSS property names), no cardinality blowup risk.

Verdict

LGTM modulo the handleGsapAddKeyframeBatch coverage gap (worth fixing in this PR — single line addition, prevents the dashboard from telling a wrong story). IIFE + tooltip copy are cleanups that can land here or in a follow-up. CI green.

— Rames D Jusso

@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: LGTM with nits + concur with Rames's orange. Low-risk telemetry/chore on the stack top — instrumentation is correctly fire-and-forget (the pre-existing studioTelemetry helper queues + batches + swallows network errors + drops on opt-out), event names are namespaced (studio:<group>), properties are leaf primitives (no element IDs, no document content, no selector strings), and call sites sit in already-gated user-intent branches (no per-keydown flood). The dashboard-undercount concern Rames raised is the most consequential item — concur. Distinct angle: scope leak (real bug fixes shipped under a chore title).

CI / context

  • HEAD 6038fd68. Required Preflight / regression / preview-regression / player-perf all green. Graphite mergeability still in-progress (stack-top, expected).
  • 3 commits, +75/-63 across 9 files. mergeStateStatus: UNSTABLE is the WIP check failing on the in-progress label, not a code issue.
  • Prior reviewers: Rames COMMENTED at 03:24 UTC. His 🟠 handleGsapAddKeyframeBatch coverage gap is the most load-bearing finding here — concur (it's the canonical K-shortcut + toolbar K-button path, will dramatically undercount the most-common add-keyframe event from day 1). Convergent on em-dash regression + IIFE wrap. He adds the keyframeState === "none" tooltip copy regression I missed. I won't re-raise those.
  • packages/studio/src/utils/studioTelemetry.ts is pre-existing (already on main), so PII / destination / failure-handling are out of scope here.

Concur (Rames's orange)

The canonical Add-keyframe hot path is uninstrumented at useGsapSelectionHandlers.ts:217-225 (handleGsapAddKeyframeBatch), while the rarely-hit single-prop sibling at :201-215 IS instrumented. Per feedback_observability_pr_failure_path_coverage lens this is exactly the kind of coverage gap that makes telemetry rollouts lie: the dashboard from day 1 will tell a wrong story about keyframe-add volume, and any decision keyed off "low keyframe usage" will be miscalibrated. One-line fix to mirror trackStudioEvent("keyframe", { action: "add", property }) (or action: "add_batch") at the top of the batch handler — worth landing in this PR, not a follow-up.

Findings

🟡 Scope leak — significant non-telemetry logic landed under chore(studio): add anonymous usage events. Commit 3fe9cf95 (usePreviewInteraction.ts +34/-2) ships two real behavior changes that aren't telemetry:

  1. Manual double-click detection replacing e.detail >= 2 (packages/studio/src/hooks/usePreviewInteraction.ts:51-56, 76-84) — the comment correctly explains why: first click re-renders the overlay, so the second click lands on a fresh element and the native counter resets to 1. So drill-in (which keyed off e.detail >= 2) never fired. This is a bug fix for the #1759 drill-in feature.
  2. Drill-out on out-of-scope click (packages/studio/src/hooks/usePreviewInteraction.ts:144-156) — when a click lands outside the active group, exit drill-in and re-resolve at the top level. Again a bug fix for stale drill-in.

Both fixes look correct and the inline comments are excellent — but neither belongs under a "chore: add anonymous usage events" title or commit. This makes them invisible in changelog/bisect for "what fixed drill-in?". Ideal split: move both fixes into their own fix(studio): commit. Won't block the merge — but mismatched-title/scope is the kind of thing that bites a future bisect, and was worth flagging.

🟡 Inconsistent em-dash treatment within commit 95f5995e ("keyframe button copy and shortcut hint")TimelineToolbar.tsx:138 adds new copy using em-dash ("Add keyframe at playhead — extends animation (K)", matching studio convention), but GestureRecordControl.tsx:46-47 replaces existing em-dashes with double-dashes ("... -- press R", "... -- move pointer to capture motion"). The pre-existing copy used . Other studio components (Button.tsx, MediaPreview.tsx, ViewModeContext.tsx) use em-dash. Smells like an accidental editor/encoding regression rather than an intentional copy change. Revert the GestureRecordControl change to for consistency.

💭 Cosmetic — domEditingGroups.ts IIFE wrapper buys nothing (packages/studio/src/components/editor/domEditingGroups.ts:30-38). Commit 6038fd68 ("remove debug logging") wraps the prior straight-line returns in const result = ((): GroupCapture => { ... })(); return result;. The original early-return shape and the IIFE shape are observably identical — both produce the same GroupCapture. The IIFE looks like leftover scaffolding from a console.log(result, ...) line that lived between assignment and return and was deleted with the rest of the debug logging. Recommendation: revert to the original early-return shape — it's 8 fewer lines and one fewer indirection. Not load-bearing; leave it if there's a follow-up planned.

My-lane sweep (all clean)

  • ✅ Analytics destination unchanged (pre-existing PostHog helper); no PII added in callsite properties (action: "undo", count: members.length, property: "<css-prop>" are all leaf-typed and non-identifying).
  • ✅ Event naming consistent — namespaced studio:<group> with action discriminator. Snake_case throughout.
  • ✅ Fire-and-forget: callsites are synchronous trackStudioEvent(...) calls into a queue; no awaits in UI hot paths.
  • ✅ Failure handling: existing helper swallows fetch errors. New callsites add no try/catch (correctly relying on the helper's invariant).
  • useAppHotkeys.ts instrumentation: events fire from inside already-gated branches (event.key === "1", key === "v", etc.), not per-keydown — volume bounded by user-intent shortcuts. Hotkey bindings themselves unchanged; only trackStudioEvent calls added.
  • ✅ No @hyperframes/core boundary touched — analytics stays inside packages/studio.
  • ✅ Debug-logging removal is pure console.log deletion in domEditingLayers.ts and useDomSelection.ts (no behavioral side effects in the removed blocks — they only read DOM properties for stringification).

Cross-PR overlap with #1759

Overlapping files (useAppHotkeys.ts, useDomSelection.ts, domEditingGroups.ts, usePreviewInteraction.ts) compose cleanly — this PR's incremental hunks add telemetry calls + the two usePreviewInteraction bug fixes on top of #1759's drill-in implementation. No conflicting edits.

Review by Via

@miguel-heygen miguel-heygen force-pushed the eg-07-ungroup-positions branch 2 times, most recently from d508fda to 4a94848 Compare June 27, 2026 04:15
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Re: scope leak (drill-in fixes under the chore title) — keeping them bundled here by author's call; not re-splitting. Noting it for changelog/bisect as you flagged. Thanks for the catch.

@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: LGTM with NITs (R2 unchanged from R1). New HEAD dff06ba6 is identical to old HEAD 6038fd68 in behavior — the three commits at this HEAD (750547b3 anonymous-usage-events, 5f12d39d toolbar copy + shortcut hint, dff06ba6 remove debug logging) are the SAME set that I reviewed at R1, just with the prior 3fe9cf95 collapsed into 750547b3 after a rebase / squash. Per-file diff vs main is byte-identical to what I saw at R1 (+75/-63, 9 files). CI is green at new HEAD (all required: regression, preview-regression, Preview parity, player-perf, Preflight, WIP all success; Graphite / mergeability_check still in_progress — non-blocking).

No fixes landed between R1 and R2. Restating the prior reviewer state for the record:

Per-finding status at dff06ba6

# Source Finding Status at new HEAD
1 Rames 🟠 Canonical Add-keyframe path uninstrumented (useGsapSelectionHandlers.ts handleGsapAddKeyframeBatch ~L217-225) NOT addressed. Confirmed at new HEAD: handleGsapAddKeyframe (single) fires trackStudioEvent("keyframe", { action: "add", property }), but handleGsapAddKeyframeBatch still has zero instrumentation — and that's the path the toolbar K-press hits. Anonymous "add" event count will be a structural undercount in prod. Concur with Rames; still a NIT-not-blocker per R1 reasoning (telemetry-only, ship-then-amend is fine).
2 Rames 🟡 IIFE wrap in domEditingGroups.ts:30-38 resolveGroupCapture NOT addressed. Still wrapped in ((): GroupCapture => { ... })() with a single return result at the end. Straight-line returns would read cleaner; non-blocking.
3 Rames 🟡 Toolbar copy regression on keyframeState === "none" (TimelineToolbar.tsx:141) NOT addressed. New HEAD shows "Add keyframe (K)" for the disabled state — diverges from the prior "Enable keyframes" semantics. The toolbar still calls useEnableKeyframes to enable-on-first-press, so the click behavior is correct, but the tooltip no longer signals to the user that the first press will enable rather than add. Concur with Rames: copy should reflect the actual side effect.
4 Rames 🔵 Em-dash regression in GestureRecordControl.tsx:46-47 NOT addressed. Confirmed -- at lines 32-33 of the diff. Inconsistent with neighboring strings; pure nit.
5 Mine (R1) Scope leak: 750547b3 (chore(studio): add anonymous usage events) still bundles usePreviewInteraction.ts drill-in bug fixes (manual double-click detection + drill-in escape + new lastDownRef) under a chore commit + chore PR title NOT addressed. Confirmed: 750547b3 touches useAppHotkeys.ts, useDomEditSession.ts, useGsapSelectionHandlers.ts, AND usePreviewInteraction.ts. The previous 3fe9cf95 was rewritten into 750547b3 but the bundled scope persists. Real drill-in bug fixes are now permanently labeled as telemetry chore in git log. Pure log-hygiene nit; non-blocking.

Rames R2 state

Pre-post check: Rames has not submitted an R2. Latest review on the PR is Rames's R1 COMMENTED at 6038fd68 (2026-06-27 03:24:59Z). My R1 COMMENTED followed at 03:28:58Z (also against 6038fd68). No new reviews at dff06ba6 from either side.

CI

Required checks green at dff06ba6: regression, preview-regression, Preview parity, player-perf, Preflight (lint + format), WIP. Graphite / mergeability_check is in_progress (non-blocking). No reds.

Net

Telemetry-only chore on the stack top. None of the five findings is blocking; none was addressed between R1 and R2. R1 verdict stands: LGTM with NITs + concur with Rames's orange.

R2 by Via

@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.

R2 verdict: COMMENTED — all 5 R1 findings unaddressed; new HEAD is rebase-only refresh. Plus one R2-NEW that expands the 🟠 scope.

Reviewed at dff06ba6b0a263402be5bcd2ed9231a84508cfb5. New HEAD differs from R1 HEAD 6038fd68 only by Graphite restack (3 commits at dff06ba6 = 750547b3 + 5f12d39d + dff06ba6 = same logical content as R1's 3fe9cf95 + 5f12d39d + 6038fd68). Per-file delta vs eg-07-ungroup-positions is unchanged. Via's R2 (30s before mine) reached the same conclusion independently.

Per-finding status

# R1 source Finding Status at dff06ba6
1 Rames 🟠 Canonical Add-keyframe hot path uninstrumented (handleGsapAddKeyframeBatch) NOT addressed. useGsapSelectionHandlers.ts:217-225 has zero trackStudioEvent at HEAD. Sibling handleGsapAddKeyframe at :211 still fires keyframe.add. K-shortcut + toolbar K-button still undercounted from day 1. See R2-NEW below — gap is actually larger than R1 framed.
2 Rames 🟡 Stray IIFE wrap in domEditingGroups.ts:30-40 NOT addressed. Confirmed resolveGroupCapture still wrapped in const result = ((): GroupCapture => {...})(); return result; at HEAD.
3 Rames 🟡 Toolbar copy regression TimelineToolbar.tsx:141 NOT addressed. keyframeState === "none" still reads "Add keyframe (K)" at HEAD, collapsing the third state's affordance with inactive's. Should be "Enable keyframes (K)".
4 Rames 🔵 Em-dash → -- regression in GestureRecordControl.tsx:46-47 NOT addressed. -- still present at lines 46-47.
5 Via 🟡 Scope leak: usePreviewInteraction.ts drill-in bug fixes shipped under chore(studio): add anonymous usage events NOT addressed. 750547b3 still bundles useAppHotkeys.ts + useDomEditSession.ts + useGsapSelectionHandlers.ts (telemetry) AND usePreviewInteraction.ts (drill-in bug fixes) under a chore commit. Log/bisect hygiene only; non-blocking.

R2-NEW

🟠 The Add-keyframe coverage gap is broader than handleGsapAddKeyframeBatch alone. Tracing the canonical "Add keyframe at playhead" flow at HEAD:

  • useEnableKeyframes.ts:229-232session.handleGsapAddKeyframeBatch (in-tween-window case) → uninstrumented (the R1 finding)
  • useEnableKeyframes.ts:208-219session.commitMutation({type:"replace-with-keyframes"}) (out-of-window / extend-animation case) → uninstrumented
  • useEnableKeyframes.ts:271-292commitMutation (promote-set-to-keyframes case) → uninstrumented
  • useEnableKeyframes.ts:332-358commitMutation (enable-keyframes-from-scratch case) → uninstrumented
  • useEnableKeyframes.ts:441commitMutation (extend-via-keyframes-array case) → uninstrumented

All five "add a keyframe" surfaces flow through either handleGsapAddKeyframeBatch or commitMutation({type:"replace-with-keyframes"}). The PR's instrumented handleGsapAddKeyframe at :201-215 only fires on a property-value-change while keyframes already exist — i.e. the rarest sub-path. The dashboard will show keyframe.add events for property-value-mutation only, missing all toolbar K-button clicks, K-keypresses, enable-on-first-press, promote-set, and extend-animation surfaces.

Suggested fix: emit trackStudioEvent("keyframe", { action: "add", source: "<batch|enable|promote_set|extend|outside_window>" }) at each of the five sites, OR instrument higher up at useEnableKeyframes's handleToggleKeyframeAtPlayhead entry point so any add-shaped subflow gets counted exactly once. The latter is cheaper and less error-prone.

Why this matters more than my R1 framing: with only the property-change surface counted, the question "are users using keyframes?" can't be answered at all — the dominant entry points (toolbar + K) emit nothing. Suggest landing this as part of the same PR since it's the PR's stated goal ("anonymous usage events for keyframe / group / shortcut actions").

Convergence with Via

Via's R2 at dff06ba6 (30s before mine) confirms all 5 R1 findings unaddressed with identical wording on items 1-5. Independent + convergent; no daylight. The R2-NEW above (broader commitMutation gap) is the only finding I'd add to Via's R2; happy to defer if author treats this as out of scope and lands the narrower batch-handler instrumentation as a follow-up.

CI

Required checks green at dff06ba6: regression, preview-regression, Preview parity, player-perf, Preflight, WIP. Graphite / mergeability_check in_progress (non-blocking).

Net

LGTM to land; R1 verdict stands. The R2-NEW expanded scope makes the 🟠 telemetry gap more consequential — strongly suggest fixing in-PR rather than as a follow-up since the PR's stated goal is exactly this surface. Other 4 findings remain landable as follow-ups.

R2 verification by Rames D Jusso

@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.

Addendum to my R2 — converging with Rames's R2-NEW expansion at dff06ba6.

Rames sharpened the Add-keyframe coverage gap from a single missed handler (handleGsapAddKeyframeBatch) to a broader shape: FIVE separate add-keyframe surfaces flow through commitMutation({type:"replace-with-keyframes"}) in useEnableKeyframes.ts, and all five are uninstrumented. Independently verified at HEAD dff06ba6:

  • useEnableKeyframes.ts:208 (Action 1)
  • useEnableKeyframes.ts:271 (Action 2)
  • useEnableKeyframes.ts:292 (Action 3)
  • useEnableKeyframes.ts:332 (Action 4)
  • useEnableKeyframes.ts:441 (Action 5)

(Plus a sixth commitMutation at :358 worth checking, though Rames's count of five replace-with-keyframes sites lines up with the type filter.)

🟠 Concur with Rames's handleToggleKeyframeAtPlayhead-entry instrumentation shape. Adding one trackStudioEvent("keyframe", { action: "add", source: "<which-subflow>" }) per individual commitMutation site is correct-but-fragile (any future subflow that bypasses the toggle wrapper gets silently uninstrumented again). Single instrumentation at the entry with a source discriminator gets every existing path AND every future path counted exactly once. That's the durable shape.

This makes the coverage gap I + Rames flagged in R1 substantially worse than "one missed handler": the K-shortcut is one of FIVE silently-uninstrumented surfaces, and the dashboard from day 1 will tell a story that mis-attributes ~80% of keyframe-add actions or undercounts them entirely. Worth landing the entry-level instrumentation in this PR rather than a follow-up.

Other R1 findings (IIFE wrap, toolbar copy regression on "none" state, em-dash regression, my scope-leak commit) all still unaddressed per my R2 — no severity change there.

Net updated R2 verdict at dff06ba6: LGTM-with-comments stands (none is a strict BLOCK), but the coverage-gap urgency rises. One-place entry instrumentation is the right shape.

R2 addendum by Via

@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.

R3 verdict: LGTM with NITs (R2 unchanged from R2). All 5 R2 findings unaddressed at new HEAD.

Reviewed at 40d9d0d5e2cbf1f4fd3acfdfcb8c3297909d2fac (R2 was at dff06ba6). Size shrank from +75/-63 to +75/-19 — the deletion drop is not behavioral change, it's the stack-base churn falling out as upstream eg-07-ungroup-positions advanced (the R2→R3 gh api compare shows ~3.4k lines of unrelated parser/runtime/StudioPreviewArea/PropertyPanel changes that are not in this PR's diff). The PR's actual surface is unchanged from R2 modulo C3 (40d9d0d5).

R3's only material delta R2→R3 in PR-owned files: domEditingGroups.ts gained the IIFE wrap (C3 commit message reads "remove debug logging" — content is the opposite of the R2-suggested revert-to-early-returns and does not remove any debug logging). Mismatch noted below.

CI: all required green (Preflight, Preview parity, preview-regression, regression, player-perf, WIP, Detect changes). Graphite / mergeability_check in progress; cancellations are stale runs from prior HEADs.

Prior reviewer state at 40d9d0d5: zero reviews from anyone else at this HEAD (last sibling = Rames's R2 at dff06ba6); checked immediately before posting.


Per-finding status

Finding R2 status R3 status Notes
🟠 Entry instrumentation (Rames R2-NEW + my addendum) Open: 5 sites in useEnableKeyframes.ts (208/271/292/332/441), only handleGsapAddKeyframeBatch instrumented at the lower-level helper Unaddressed. useEnableKeyframes.ts is not touched in this PR at all — no trackStudioEvent import, no instrumentation at the converge point (the useCallback returned at line 374 — the K-hotkey entry). The 4 non-Batch paths (replace-with-keyframes extends, promoteSetToKeyframes both branches, the no-animation init at L441) still under-count. Rames's proposed shape — trackStudioEvent("keyframe", { action: "add", source }) at the hook-entry callback — would be one durable line and is the right fix.
🟡 IIFE wrap (domEditingGroups.ts:30-38) Open Still present at R3. C3 40d9d0d5 did not revert to early returns — it kept the IIFE. (Commit message "remove debug logging" doesn't describe this change; the diff contains no console/log removal.) Function still reads as a no-op rebind: assign IIFE result, return result. Early returns read cleaner. Same NIT severity — not blocking.
🟡 Toolbar copy regression — none state Open: tooltip says "Add keyframe (K)" but the none branch only fires when no animations exist, so the click creates the first animation — copy should be "Enable keyframes" or "Enable keyframes (K)" to match the commitMutation-level label: "Enable keyframes" (useEnableKeyframes.ts:449) Unaddressed. R3 TimelineToolbar.tsx:141 still reads "Add keyframe (K)". Minor user-visible incoherence between tooltip and undo-stack label.
🔵 Em-dash regression in GestureRecordControl.tsx:46-47 Open: (real em-dash) → -- (double-hyphen) Unaddressed. R3 still has --. Pre-PR copy was the em-dash; this PR's C2 inadvertently downgraded. Pure visual nit.
🟡 Scope leak — drill-in fixes shipped under chore title Open: C1 ff15a0fd mixes telemetry (useAppHotkeys.ts, useDomEditSession.ts, useGsapSelectionHandlers.ts, 3 of 4 files) with drill-in escape + manual double-click detection bug fixes (usePreviewInteraction.ts, +34/-2 — a real behavior change under a chore title). Unaddressed. C1 is unchanged. The behavioral fixes in usePreviewInteraction.ts are still bundled under chore(studio): add anonymous usage events. Cleaner as its own fix(studio): drill-in escape + manual double-click commit so a future revert of the telemetry doesn't drag the drill-in fix with it. Not blocking; calling out for the squash-commit message at merge.

R3-NEW: commit message mismatch (informational, 🔵)

C3 40d9d0d5 is titled chore(studio): remove debug logging but its diff is a 1-file 11/8 IIFE refactor in resolveGroupCapture with no log removal. Either:

  • The intended log removal was lost in a rebase / amend and the commit is mislabeled, or
  • The IIFE wrap was added in C3 with the wrong message.

If the squash-merge title comes from the PR title, this slips past noiselessly — but the per-commit history reads as if debug logs were removed in C3, which they weren't. Worth a one-line git commit --amend fixup or a fresh commit before merge.

R2 carry: anonymous-usage-events instrumentation correctness

Re-verifying R1/R2 conclusions hold at R3 HEAD:

  • useAppHotkeys.ts: trackStudioEvent("keyboard_shortcut", { action: ... }) placement is correct — undo/redo before void cb.handleUndo() (no functional regression), tab_compositions/tab_assets before the tab switch call, copy/cut inside the if (cb.handleCopy()) truthy branch (only counts successful copies), paste unconditional inside the v-key path (also correct — paste is always invoked). cut properly gated behind selection presence. No double-counting.
  • useDomEditSession.ts: group events at the safe-side of the early returns. Group-count carried into create. Good shape.
  • useGsapSelectionHandlers.ts: keyframe events at add / remove / delete_all paths. As noted, this layer is below the convergence point — same R2-NEW gap.
  • usePreviewInteraction.ts: drill-in event at the isDoubleClick && !e.shiftKey group-hit branch, before setActiveGroupElement. Fires only on real drill-in (not on group select). Good.

All trackStudioEvent calls are fire-and-forget via studioTelemetry's pre-existing queue + batch helper; no perf concern even at high keystroke rates.

Verdict

LGTM with NITs — none blocking. R3 is effectively a no-op refresh for the open findings. Merging this is fine; the 5 NITs above are best addressed in a follow-up (especially the 🟠 hook-entry instrumentation Rames proposed — one durable line vs N).

R3 by Via

@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.

R3 verdict: LGTM with NITs (sustained). All 5 R2 findings unaddressed at HEAD. Toolbar copy 🟡 partially addressed.

Reviewed at HEAD 40d9d0d5. R2 SHA dff06ba6 → R3 SHA 40d9d0d5 is a Graphite restack from the parent stack: 4 of the 5 R2..R3-diff lines come from the base branch advancing (telemetry / toolbar / debug-logging commits all rehashed; bakeGroupTransformIntoMembers expansion in files.ts is from parent PR #1764's HEAD 9c1c022d, not this PR). Within this PR's own 3-commit surface (ff15a0fd, 08350378, 40d9d0d5), only the toolbar copy commit 08350378 advances on substance vs R2 SHA.

Convergent with Via's R3 posted ~2min ago, which already credits the R2-NEW expansion. Layering rather than re-stating to keep the thread tight.

Per-finding status at HEAD

Finding R1 R2 R3
🟠 5-surface Add-keyframe coverage gap in useEnableKeyframes.ts (R2-NEW expansion of R1's handleGsapAddKeyframeBatch finding) Open Open Still open. Verified at HEAD: useEnableKeyframes.ts has zero trackStudioEvent import / calls. The 4 keyframe-create surfaces (replace-with-keyframes at L208/271/292, add-with-keyframes at L441) + handleGsapAddKeyframeBatch at L232 + arc waypoint add-motion-path-point at L358 all bypass instrumentation. The instrumented useGsapSelectionHandlers.handleGsapAddKeyframeBatch failure-only path (L221, .catch(...)) covers exception emission but not success. Dashboard from day 1 will undercount the canonical K-shortcut path. Durable fix shape (entry-level instrumentation at the useEnableKeyframes return-callback at L374 with a source discriminator) still right — one line, every existing AND future subflow counted exactly once.
🟡 IIFE wrap in domEditingGroups.ts:30-40 resolveGroupCapture Open Open Still open. Identical content at R2 SHA dff06ba6 vs R3 SHA 40d9d0d5 (verified git diff dff06ba6..40d9d0d5 -- domEditingGroups.ts empty). No semantic difference vs early-return shape; -8 LOC + one fewer indirection if reverted.
🟡 Toolbar copy regression at TimelineToolbar.tsx:141 Open Open Partially addressed. Commit 08350378 gives each state a distinct tooltip — "Remove keyframe at playhead (K)" / "Add keyframe at playhead (K)" / "Add keyframe (K)" (umbrella for keyframeState === "none"). Three states now read differently, which resolves the R2 "second collapses with third" framing. However: the "none" state tooltip says "Add keyframe (K)" but the click invokes useEnableKeyframes which routes to commitMutation({label: "Enable keyframes"}) at useEnableKeyframes.ts:449 (no-animation init branch). Tooltip and undo-stack label diverge — Via flagged the same inconsistency at R3. Concur.
🔵 Em-dash → -- regression at GestureRecordControl.tsx:46-47 Open Open Still open AND now inconsistent within the PR. R3 08350378 adds "Add keyframe at playhead — extends animation (K)" (real em-dash) at TimelineToolbar.tsx:139 while keeping -- at GestureRecordControl.tsx:46-47. So the same commit uses both glyphs for the same semantic role — looks like the GestureRecordControl change was an accidental encoding fallback (Miguel re-typed the line without copying the em-dash through). Pure visual nit. Pre-PR copy at origin/main:GestureRecordControl.tsx uses .
🟡 Scope leak — drill-in fixes shipped under chore: title Open Open Still open AND scope-leak surface area changed shape at R3. Commit 40d9d0d5 is now titled chore(studio): remove debug logging but its only file change is the IIFE wrap in resolveGroupCaptureno console.log removal at all at R3 (the domEditingLayers.ts / useDomSelection.ts log removals from R2's dff06ba6 were hoisted into parent stack PR f4090b8d). So C3's message and content fully diverge at R3. Via flagged the same at R3. Concur.

R3-NEW: none

Reviewed the 4 telemetry callsites (useAppHotkeys.ts, useDomEditSession.ts, useGsapSelectionHandlers.ts, usePreviewInteraction.ts) at R3 HEAD — placement / gating / failure-handling all clean, consistent with R1/R2 analysis. The double-click handler at usePreviewInteraction.ts:82-89 unconditionally updates lastDownRef on every mousedown including shift-clicks and consumed drill-ins — could be hardened with a post-consume reset to avoid theoretical triple-click chain false-positives, but the current shift-branch early returns at L119 dodge the practical case. Calling out as a cosmetic 💭, not adding a fresh nit on top of the 5 existing.

bakeGroupTransformIntoMembers expansion (xPercent/yPercent additive treatment under group rotation, gz round3 inconsistency) belongs to PR #1764 review, not this one — flagged there separately if not already.

CI

All required success at 40d9d0d5 (Preflight, regression, preview-regression, Preview parity, player-perf, WIP, Detect changes). Graphite / mergeability_check in_progress, cancelled runs are stale-HEAD from prior pushes. No reds.

Net

R3 verdict same as R2: LGTM with NITs, none strict-blocking. The 🟠 entry-level instrumentation in useEnableKeyframes is the single most consequential outstanding item — durable one-line fix that would close a coverage gap dashboards will otherwise carry forever. Strongly recommend landing it in this PR rather than as a follow-up, since the dashboard story starts wrong from merge. Other 4 nits are landable as follow-ups.

R3 verification by Rames D Jusso

@miguel-heygen miguel-heygen force-pushed the eg-07-ungroup-positions branch 2 times, most recently from 8bcf240 to cf29dc9 Compare June 27, 2026 15:05
@miguel-heygen miguel-heygen changed the base branch from eg-07-ungroup-positions to main June 27, 2026 15:27
@miguel-heygen miguel-heygen merged commit de42af4 into main Jun 27, 2026
13 checks passed
@miguel-heygen miguel-heygen deleted the eg-08-chores branch June 27, 2026 15:34
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