Skip to content

chore(studio): remove all console.* calls from studio package#1691

Open
miguel-heygen wants to merge 1 commit into
mainfrom
chore/studio-remove-console-logs
Open

chore(studio): remove all console.* calls from studio package#1691
miguel-heygen wants to merge 1 commit into
mainfrom
chore/studio-remove-console-logs

Conversation

@miguel-heygen

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

Copy link
Copy Markdown
Collaborator

Summary

Remove all console.debug, console.log, console.warn, and console.info calls from the studio package. These were a mix of leftover debug logs and error-swallowing warns that should use proper error boundaries or telemetry instead.

  • 18 files, 24 insertions, 83 deletions
  • Unused catch parameters renamed or removed
  • editDebugLog function body emptied (signature preserved for callers)

Test plan

  • Studio loads without console errors
  • Existing functionality unaffected (drag, resize, rotate, gesture recording)

@miguel-heygen miguel-heygen force-pushed the chore/studio-remove-console-logs branch from 7ab746b to f53eaa4 Compare June 24, 2026 05:22

@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: NIT (with a small ask)

Chore is mostly clean — the strip-and-empty-catch pattern is fine in the cross-origin / fire-and-forget call sites, and editLog's signature is preserved so all 4 callers (useGsapAwareEditing.ts, useRazorSplit.ts, useGsapScriptCommits.ts, useGestureCommit.ts) keep compiling. A few cleanup nits worth addressing before merge:

1) Empty if blocks left behind (dead branches)

The console.* was the only statement inside an if, so removing it leaves the condition computing for nothing. Lint will flag these and they're harmless but noisy:

  • packages/studio/src/components/editor/snapTargetCollection.ts:101-104if (elements.length >= MAX_SNAP_TARGETS) {} (kept the cap check, dropped the body).
  • packages/studio/src/player/components/Player.tsx:255-256if (lastUnloaded) {} inside the poll's settle branch.
  • packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts:294-295 — the 5s readiness-timeout guard: if (!settled) {}.
  • packages/studio/src/hooks/useGestureRecording.ts:385-396 — the catch {} keeps its 3-line comment promising "Surface in dev so a dead preview isn't silent" and an if (isDevBuild()) {} body. The comment now lies (Pattern #6) — either drop both the comment and the dev guard, or restore the warn behind a non-console channel.

Easy fix: just delete each empty if (...) {} plus the now-stale comment above it.

2) Dangling // eslint-disable-next-line no-console directives

Two pragmas no longer cover any console.* call and will trip eslint's no-unused-disable-directives once that's enabled:

  • packages/studio/src/telemetry/client.ts:132 — sits directly above the closing } of showNoticeOnce().
  • packages/studio/src/utils/sourcePatcher.ts:247 — sits directly above the closing } of the duplicate-match guard.

3) showNoticeOnce() is now a behavior change, not a strip

packages/studio/src/telemetry/client.ts:129-133 — the function still calls markNoticeShown() so the "one-time" latch fires, but never actually shows the notice. Users no longer get the "anonymous studio usage analytics enabled — here's how to disable it" disclosure on first run. That's a disclosure / consent surface, not noisy debug output. Either delete the whole function + its call site, or replace the console.info with the intended dialog/banner so the latch still has meaning. Worth a quick word with whoever owns telemetry UX before merging this in its current shape.

4) Lint enforcement — what stops the next console.log from sneaking back?

.oxlintrc.json only enables correctness + react/typescript plugins; there's no no-console rule, and I don't see one being added in this PR. Without that, the cleanup decays back to the pre-PR state on the next "I'll just console.log this real quick" commit. Either add "no-console": "error" (with a narrow allowlist for whichever telemetry/disclosure surface lands as the replacement for #3) in a follow-up, or note it as a TODO in the PR body.

Non-issues / verified

  • editLog signature preserved; underscored params are correct; 4 callers across hooks/ still typecheck. Comment in the new body even leaves a "Restore with:" recipe — nice.
  • The catch {}-without-binding swallow is fine in gsapDragPositionCommit.ts, useDomEditTextCommits.ts, useTimelinePlayer.ts, useTimelineSyncCallbacks.ts, timelineIframeHelpers.ts, optimisticUpdate.ts — these are all best-effort cross-origin postMessage / debounced auto-saves where the prior warns were already noise.
  • useDomEditCommits.ts already routes failure into trackError(...) telemetry; dropping the warn is pure noise reduction.
  • lefthook.yml switch from oxfmt --check to autofix-and-restage is unrelated-but-fine (matches the PR body's reasoning); doesn't affect runtime behavior.

CI

In flight at read time: Build, Lint, Typecheck, Test, Studio load smoke, CodeQL, Fallow audit all IN_PROGRESS; non-required checks green. Worth landing #1's dead-ifs before lint completes — they may trip an no-empty style rule if one is on.


Happy to stamp once the empty ifs and stale pragmas are cleaned up; the showNoticeOnce change is the one item that genuinely deserves a call with the telemetry owner before merge.

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

R1 @ f53eaa4 — Rames D Jusso review

Reviewing the studio console.* purge. Scope is narrower than the title claims, and there are syntactic-stub leftovers from the mechanical removal.

🛑 Blockers

  • packages/studio/src/hooks/useGestureRecording.ts:388-392 — empty if (isDevBuild()) {} block after the warn body was deleted. The comment above ("Surface in dev so a dead preview isn't silent; r.runtime is nulled below so this warns at most once per gesture") advertises a dev-surfaced signal that is now a no-op. Either delete the gate entirely (with the comment) or replace with a structured trace (PostHog/useStudioTelemetry-shape). As written, the comment lies about behavior.

  • packages/studio/src/components/editor/snapTargetCollection.ts:103-104 — empty if (elements.length >= MAX_SNAP_TARGETS) {} block. Either delete the conditional or hook a metric (snap-target-cap-reached counter is exactly the kind of soft-regression signal worth keeping). Right now the cap-reached path silently drops elements from snap alignment with zero diagnostic surface; that's a UX regression-detection blind spot, not just a stylistic dangle.

  • packages/studio/src/player/hooks/useTimelineSyncCallbacks.ts:294-296 — empty if (!settled) {} inside the 5s readiness probe's timeout. Same shape — runtime didn't signal readiness within 5s is a real failure mode worth a metric. The diff also kills the explanatory [useTimelinePlayer] Runtime did not signal readiness within 5s warn without a replacement.

  • packages/studio/src/player/components/Player.tsx:256-258 — empty if (lastUnloaded) {} after the asset-load 10s timeout. Replace with a studioTelemetry event for "asset-loading-overlay timed out" so SRE/oncall can see it on the cohort dashboard. Asset-loading timeouts at the 10s ceiling are the exact "infra hiccup" signal users describe but can't reproduce.

  • packages/studio/src/telemetry/client.ts:130-133showNoticeOnce() still calls markNoticeShown() and eslint-disable-next-line no-console but the body is gone. Function name + side effect mismatch: it marks the notice shown without showing anything. Either delete the function and its callers, or restore a console.info (this one is opt-in observability for end users, not a debug log). Leaving the stub means users never see the "telemetry enabled, opt out via localStorage" notice that was the function's whole purpose, while still consuming the hasShownNotice budget.

  • packages/studio/src/utils/sourcePatcher.ts:246-248 — duplicate-id detection still has eslint-disable-next-line no-console but no console call, and the JSDoc above says "Don't silently patch the first while leaving the other stale — surface it." After this PR, it silently patches and stays stale. Either restore the warn (it's an invariant violation worth the noise) or route through studioTelemetry.

⚠️ Concerns

  • Scope-vs-title mismatch: title + body claim "all console.* calls". Reality — only console.log / console.warn / console.debug / console.info are removed; console.error remains in 14+ studio files (StudioErrorBoundary.tsx:20, SlideshowPanel.tsx:103,226, useFileManager.ts:255,277,295,318,339, useTimelineEditing.ts:124,175,287, useTimelineClipDrag.ts:473,514, useGestureCommit.ts:263, playbackAdapter.ts:185, timelineIcons.tsx:42). The timelineIcons.tsx:42 one is actually console.warn, missed entirely. Either widen the diff or rename PR to chore(studio): remove debug console.log/warn/debug/info. The current framing leaves reviewers thinking the studio is console-free post-merge; it isn't.

  • No no-console ESLint rule: a chore-PR that doesn't enforce the chore reverts the moment another agent generates studio code with console.warn(…). The 14 surviving console.error calls would also be a useful canary for "what we explicitly chose to keep" if encoded as no-restricted-syntax allowlist. Worth landing a rule ("no-console": ["error", { allow: ["error"] }]) in this same PR — the diff that proves the rule passes is right here.

  • packages/studio/src/utils/editDebugLog.ts:6-9editLog() is now a no-op stub with 5 callers (gesture commit, razor split, gsap commits, manual drag x3). Comment says "Restore with: console.log(...)" but a real fix is routing to PostHog with a __hfDebug-gated console echo, otherwise the entire edit-pipeline trail (the function's stated purpose, per the docstring) is permanently dark. As-is it's an empty interface that pollutes callsites with dead args.

  • Suppressed-path observability (memory: dashboard-derived-fix observability silence): the useDomEditCommits.ts:224 removal nuked the "Element not found in source" warn — but the immediately-preceding studioTelemetry.captureEvent({ event_kind: "dom_edit_target_not_found", … }) does carry the signal. ✅ That removal is justified. The optimistic-update console.warn("[optimistic] Mutation failed, rolled back:", error) removal at utils/optimisticUpdate.ts:14 is NOT — there is no replacement telemetry, and a rolled-back mutation is the exact thing oncall needs visibility on. Route through studioTelemetry.captureEvent({ event_kind: "optimistic_rollback", error }).

  • packages/studio/src/captions/hooks/useCaptionSync.ts:81 — auto-save failures now .catch(() => {}) with no telemetry. Caption auto-save is a data-loss path; users will hit "where's my caption edit?" and we'll have no signal. Route through telemetry.

  • lefthook.yml:9-12 — out-of-scope. Hook now auto-formats AND re-stages on commit. Behavioral change: a bunx oxfmt --check failure used to abort the commit; now the commit silently mutates {staged_files} and proceeds. Generally fine, but the previous --check was load-bearing for amend-after-hook snapshots — the new comment acknowledges that but the auto-re-stage shape can race with git commit --amend workflows that mutate the index mid-hook. Worth a separate PR with explicit testing of amend flow.

🟡 Questions

  • Why was packages/producer/tests/distributed/css-var-fonts/output/output.mp4 regenerated? Binary diff in a console-cleanup chore PR reads like accidental fixture overwrite. If intentional, mention; if not, revert.

🟢 Looks good

  • BlockParamsPanel.tsx _compositionPath rename — clean, explicit unused-param convention.
  • useDomEditTextCommits.ts consolidation of try/catch shapes — clean, telemetry path remains via studioTelemetry.captureEvent in the upstream call sites.
  • gsapDragPositionCommit.ts:165-169[gsap-drag] start-value read failed warn → silent restore-to-identity is fine; the fallback shape itself is observable as "drag commit produced no delta".

— Rames D Jusso

Stamp routing: @rames Jusso once findings addressed (or punted with rationale).

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