Repository navigation
feat(studio): track completed preview editing gestures - #4916
Conversation
Edit accuracy: accurate 1216 (base branch 1216), smooth 1136 of thoseThe gate passes. Quarantined, measured but not gated (1)
|
1874690 to
dcfe83b
Compare
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Changes requested at 944744efa6329705eb228b78e848b76f2a745275 for the stated no-user-content telemetry contract.
New usage events include the user-authored project name in their serialized payload. trackPreviewFeatureUsed (packages/studio/src/utils/previewFeatureUsage.ts:28-30) calls trackStudioEvent, which adds getSessionProperties() to every event (studioTelemetry.ts:44-64,69-78) before drainBatch() forwards those properties to PostHog (studioTelemetry.ts:86-93,107-112). Its url_hash is the hash route with #project/ removed and the query stripped—not an anonymous route kind. Project routing encodes the user-supplied name into that hash (projectRouting.ts:45-56), and the existing route test proves "Launch #1? v2" becomes #project/Launch%20%231%3F%20v2 (projectRouting.test.ts:70-86). A completed preview move in that project therefore sends a new studio:feature_used event with url_hash: "Launch%20%231%3F%20v2", a reversibly encoded project name, alongside feature/surface/method. The same inherited property reaches new keyframe and design-input events. The privacy opt-out stops collection when disabled; it does not remove this content when enabled.
The shared session property predates this PR, but these newly introduced event types inherit it, contrary to the PR's "Send feature, surface and method only" claim and the requested no-user-content payload. Please remove or anonymize project names for these events (or explicitly settle a policy exception), and test the serialized batch using a named project rather than asserting only the caller's filtered properties. I found no other verified code blocker in the gesture/write-result paths. At review time the active 20-shard edit-accuracy and Windows reruns were still in progress; this finding is independent of CI.
— Review by tai (pr-review)
|
@terencecho Replaced the deterministic route fingerprint at |
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Changes requested at 07e88fdb10412742e968dca09307d133e9261111 on the remaining no-user-content telemetry contract.
The raw route was removed, but every Studio event now sends a deterministic fingerprint of the authored project name. packages/studio/src/utils/studioTelemetry.ts:51-57 computes an unsalted, 32-bit FNV-1a hash over location.hash before ?; :59-77,83-106 adds that url_route_id to every queued event and serializes it in both transports. The route includes the URL-encoded project name. For the test fixture Launch #1? v2, the client hashes #project/Launch%20%231%3F%20v2 into 7aef12cc; anyone with a candidate project name can compute the same value offline and match it to events. This is a stable, low-entropy pseudonym, not anonymization of user-authored content. The same issue affects non-project hashes categorized as other, which the helper hashes without checking their contents. The new serialized-batch tests (studioTelemetry.test.ts:90-151) assert the raw and URL-encoded name are absent, but explicitly accept the derived eight-hex token, so they do not pin the stated privacy invariant.
The finite url_hash route kind fixes the original plaintext leak, and I found no separate gesture/write-path regression in this two-commit delta. Please omit the deterministic name-derived url_route_id, use a random per-session/per-tab route-equality token if the mid-session comparison needs one, or obtain an explicit privacy-policy exception for a stable cross-session project fingerprint. Test the complete serialized fetch and beacon batches against the derived-token case, not just the raw name.
Verdict: REQUEST CHANGES
Reasoning: The new event sink no longer exposes the project name directly, but the replacement still emits a reproducible identifier derived from that name; one approval would be merge-enabling before this contract is resolved.
— Review by tai (pr-review)
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Approved at 5995525dc47a792c645d407c542ca7b34a4e7521 after re-reviewing the three-commit privacy fix against my prior changes request.
The unsalted FNV project-name fingerprint is gone. studioRouteId now maps each route (without its query) to four CSPRNG bytes; route strings are keys only in sessionStorage/the in-memory map, while the queued studio:* event's route properties carry only the finite kind and random eight-hex token. Both fetch and beacon serialize that same event shape. The serialized-batch tests reject the original project's raw and URL-encoded name and its old deterministic token, and exercise route equality across query changes and a reload. I found no remaining blocker in this delta against the route-envelope privacy contract.
The Studio test, lint, typecheck, and preflight checks passed when I inspected them; edit-accuracy and Windows shards were still pending, so this review does not claim the entire CI run is green.
— Review by tai (pr-review)
2 participants
What
Track completed preview editing gestures through
studio:feature_used. Add missing successful keyframe actions throughstudio:keyframeand custom inspector inputs throughstudio:design_input. Replace user-authored route text in the shared Studio telemetry envelope with a route kind and stable equality ID.Why
Preview gestures and several keyframe write paths bypass existing usage events. A resolved save promise alone does not establish that source changed.
Related work
No linked issue. Existing inspector, keyboard, clipboard, and history events retain their own event names.
How
Carry the writer outcome through geometry commits. Collect grouped GSAP writes into one gesture outcome, then enqueue one usage event. Usage-specific properties are feature, surface and method. Keyframe events include an action and, for the existing single-property add path, a property name. Batch writers return an ordered change receipt for each mutation, so a changed update cannot credit a no-op add. Recording retains its original selection and animation set through the write facade.
Every
studio:*event inheritsurl_hashasproject,homeorother.url_route_idis now a random eight-hex token generated withcrypto.getRandomValues, reused through a route-to-ID map in sessionStorage. The map stays local; neither transport sends route names or selection queries. Independent tabs receive separate tokens. A tab opened from Studio can inherit its opener's sessionStorage tokens, preserving equality within the same session.Saved insight compatibility was checked through the read-only paginated API. Read 404s: hashchange (door 2) vs mid-session (door 3) compares the loaded route against later events and will use
url_route_idfor equality. Top diverging targets (triage) currently displaysurl_hashas a sample and will display the route kind. Their query updates are planned after merge.Paths below are relative to
packages/studio/src/.movecomponents/editor/useDomEditOverlayGestures.ts:357,494resizecomponents/editor/useDomEditOverlayGestures.ts:541rotatecomponents/editor/useDomEditOverlayGestures.ts:447cropcomponents/editor/DomEditCropHandles.tsx:239nudgecomponents/editor/useDomEditNudge.ts:177multi_selectcomponents/editor/marqueeCommit.ts:145,hooks/usePreviewInteraction.ts:173z_ordercomponents/editor/useDomEditZOrder.ts:177text_edithooks/useDomEditTextCommits.ts:281gesture_recordinghooks/useGestureCommit.ts:355motion_pathcomponents/editor/motionPathCommit.ts:23,components/editor/MotionPathOverlay.tsxsnappingcomponents/editor/SnapToolbar.tsx:40,57gridcomponents/editor/SnapToolbar.tsx:45,62grid_spacingcomponents/editor/SnapToolbar.tsx:27snap_to_gridcomponents/editor/SnapToolbar.tsx:226rulercomponents/editor/SnapToolbar.tsx:122safe_marginscomponents/editor/SnapToolbar.tsx:122studio:keyframeactions:add,convert,remove_all,reset. Legacy single/batch writers count changed results; SDK operations compare committed source. Replacement operations that insert a stop carry explicit add metadata. Timeline delete-all counts one gesture across its animation writes.Missing inspector controls covered: FX parameters, effect bypass, add/remove/reorder effects, apply/remove presets, preset amount, add/edit/remove EQ and automation, native carve controls, levelling enable/remove, and multi-selection group/hide buttons. These use
studio:design_input, notstudio:feature_used.Source inventory found no separate preview align/distribute action or custom guide-drag writer.
Test plan
Remote Linux: the gesture implementation passed 624 tests across 34 files at
8dfe0571, followed by exact-head affected-hook tests and real canvas/keyframe captures at later heads. Recording target, consumer attribution, producer receipt and nudge mutations each failed their witness test, then passed after restoration. The random-ID follow-up passed 47 telemetry, routing and client tests ate8a63efb. Restoring FNV deliberately failed both complete serialized-batch privacy tests and the independent-tab test; restoring random IDs passed all 12 telemetry tests. At5995525d, all 47 tests passed again, both typechecks, formatting of 83 touched paths, lint, Fallow audit and production builds passed. The deliberate FNV mutation failed the same three privacy tests, then all 12 telemetry tests passed after restoration. Fresh browser captures show one move and one add event with the same random route ID.Size
The patch spans gesture callers and their shared save-result plumbing. Group move was extracted to retain the production file-size limit. The random route-ID implementation adds no dependency. Storage decoding and tuple validation are separate from token creation, addressing the Fallow complexity finding without a suppression. Independent source review and all three simplification reviews found no blockers at
5995525d. Final runtime proof passed. Review and simplification marks are recorded at this head. CI is in progress.Before
On main (
d5b56426), the repository edit-accuracy fixture completes the canvas drag and a keyframe add, but neither matching usage event appears in the flushed telemetry batch. Telemetry is enabled and a startup batch is observed.After
At
5995525d, the same fixture drag persists and emits exactly onestudio:feature_usedwithfeature=move,surface=preview,method=drag. A keyframe add persists and emits exactly onestudio:keyframewithaction=add. Both actual request panels showurl_hash=projectand the same random eight-hex route ID. Requests are fulfilled locally, so validation does not send analytics. The complete serialized-batch regressions separately check the entire envelope for raw names, encoded names, selection queries and the old deterministic fixture fingerprint.