fix(studio): guard against null tag in timeline track style

[miguel-heygen]

Summary

• getTrackStyle() now defaults to "div" when tag is falsy, preventing toLowerCase() / startsWith() from throwing on null/undefined/empty-string values at runtime

Test plan

• Build and typecheck pass
• Verified callers in Timeline.tsx pass "" fallback which is now handled

[vanceingalls]

NIT — surface guard fine, but the framing's a little crooked and the upstream null still ships

The patch itself reads cleanly:

// packages/studio/src/player/components/timelineIcons.tsx:42-44 @ a0f7ddd
const safeTag = tag || "div";
const trackStyle = getTimelineTrackStyle(safeTag);
const normalized = safeTag.toLowerCase();

|| "div" is the right shape — getTimelineTrackStyle() always falls through to a single TRACK_STYLE constant (packages/studio/src/player/components/timelineTheme.ts:69-71) so "div" vs an empty string is purely cosmetic for the style lookup, and for the icon lookup it lands on IconComposition either way (ICONS.div).

A few things I want to flag before this goes in, though.

1. The PR body claims empty-string was throwing. It wasn't.

preventing toLowerCase() / startsWith() from throwing on null/undefined/empty-string values

"".toLowerCase() returns "", and "".startsWith("h") returns false. Empty string was already safe — it just landed on IconComposition via the ICONS[""] ?? IconComposition branch. The thing that actually throws on PostHog is null/undefined.

That distinction matters because the "Test plan" item says:

Verified callers in Timeline.tsx pass "" fallback which is now handled

The two "" call sites already use the nullish-coalesce defensively:

// packages/studio/src/player/components/Timeline.tsx:210 @ a0f7ddd
map.set(trackNum, getTrackStyle(els[0]?.tag ?? ""));
// :217
const ts = trackStyles.get(trackNum) ?? getTrackStyle("");

The call sites that were actually crashing are the unguarded el.tag ones at Timeline.tsx:294, :487, :495 — where el is a TimelineElement whose .tag is typed string but is, at runtime, sometimes nullish. Those are the ones this guard rescues. Worth adjusting the PR body so the next reader doesn't go hunting for the empty-string throw site that doesn't exist.

2. Pattern #10 (decorative gate): the populate path still produces undefined

The interface lies:

// packages/studio/src/player/store/playerStore.ts:23-27 @ a0f7ddd
export interface TimelineElement {
  ...
  tag: string;

But two of the three populate sites in timelineDOM.ts do not guarantee a string:

// packages/studio/src/player/lib/timelineDOM.ts:75 @ a0f7ddd
tag: clip.tagName || clip.kind,
// :106
tag: clip.tagName || clip.kind,

If both clip.tagName and clip.kind are empty/undefined, .tag is undefined. Compare with the third populate site, which already does the right thing:

// packages/studio/src/player/lib/timelineDOM.ts:394 @ a0f7ddd
tag: params.tagName.toLowerCase() || "div",

So the type signature, two of the three constructors, and a downstream consumer all disagree about whether tag can be nullish. This PR plugs the symptom in one consumer; the next consumer that calls something like el.tag.toLowerCase() directly will crash the same way. A second short follow-up commit that pushes || "div" (or ?? "div") into the :75 and :106 constructors — same pattern Miguel already established at :394 — would actually close the bug at the source. Happy to leave that as a follow-up, but I want it logged.

3. Soft: widen the signature

getTrackStyle(tag: string) accepting nullish at runtime means TypeScript can't catch the next caller that forgets the ?? "". tag: string | null | undefined would make the guard load-bearing at the type level instead of decorative. Optional.

CI

Everything required is green except Studio: load smoke, which died on actions/checkout (infra flake — none of the steps after checkout even ran). A rerun should clear it.

---

Not blocking — happy to see it ship as the immediate symptom fix. Just flagging that the populate path at timelineDOM.ts:75/:106 is the next thing that wants fixing, and the PR body's empty-string framing is slightly off.

Review by Via

[james-russo-rames-d-jusso]

R1 review @ HEAD a0f7dddd

PostHog symptom: TypeError: Cannot read properties of null from tag.toLowerCase() / tag.startsWith() in getTrackStyle. Fix: const safeTag = tag || "div". Symptom-suppress in the consumer; the actual root cause is tag: string-typed fields populated with runtime nullish values upstream. Suppression is correct as an immediate hot-path patch, but root cause persists.

Findings

	Where	What
🟡	timelineIcons.tsx:42	tag: string signature now silently accepts nullish at runtime — the guard is decorative at the type level. Future callers that destructure el.tag and call .toLowerCase() directly will crash with the same shape. Suggest widening to tag: string | null | undefined so TypeScript actually enforces the invariant the runtime now relies on.
🟡	PR body framing	The body claims toLowerCase() / startsWith() were "throwing on null/undefined/empty-string values." Empty string was already safe — "".toLowerCase() returns "", "".startsWith("h") returns false, then ICONS[""] ?? IconComposition falls through to IconComposition. The actual crash shape is null/undefined only. Worth correcting in the body so the next reader hunting through PostHog stacks isn't looking for an empty-string throw site that doesn't exist. The "Verified callers in Timeline.tsx pass \"\" fallback" test plan item is also misleading — those two sites (Timeline.tsx:210 els[0]?.tag ?? "" and TimelineCanvas.tsx:217 getTrackStyle("")) were already working; the actually-rescued sites are the unguarded el.tag ones at TimelineCanvas.tsx:294/:487/:495.
🟡	observability	When the fallback fires (tag was nullish, defaulted to "div"), there's no signal. PostHog dashboard surfaced this in the first place because the crash showed up — once the crash is suppressed, future drift goes silent. Consider a one-shot console.warn or a PostHog event capture with the upstream stack so the dashboard still surfaces the cohort whose data is malformed.
🟢	code shape	|| "div" is the right operator — empty-string falls through to the same TRACK_STYLE constant (timelineTheme.ts:73 ignores the tag arg) and the same ICONS.div → IconComposition lookup, so behavior is preserved for "" callers while rescuing nullish ones.

CI

All required green except Studio: load smoke — failed in actions/checkout (git lfs fetch → "batch response: Fatal error: We couldn't respond to your request in time"). LFS infra flake, not a real failure. Rerun should clear.

Re: Via's review

Via posted ~10min before me; converged on the same three findings (1: empty-string framing in PR body, 2: upstream populate paths at timelineDOM.ts:75/:106 still produce nullish tag while :394 does \|\| "div" correctly, 3: widen the signature). Her timelineDOM finding is high-leverage — the cleanest "fix the root cause" follow-up. Full credit; I'd echo "land this as the hot-path patch + a small follow-up that pushes \|\| "div" into :75/:106."

Verdict

LGTM as immediate symptom fix. Land + follow-up PR for the populate-site root cause per Via's Finding 2. Not stamping autonomously — tagging <@U0ARJFN5S6Q> for stamp.

— Rames D Jusso

[vanceingalls]

LGTM — populate-path root cause closed at timelineDOM.ts:75 and :106

Re-reviewed at 68f008f4. R1 left at a0f7dddd as NIT with three asks; the new commit addresses the substantive one cleanly.

Per-item disposition

1. PR body framing (informational). Not addressed — the body still reads "preventing toLowerCase() / startsWith() from throwing on null/undefined/empty-string values" and the test plan still references the ""-fallback callers. This was always informational and the patch itself is right, so not blocking. The next reader chasing the PostHog stack will just have to know that empty-string was already safe and the actually-rescued shape was nullish.

2. Populate-path root cause — ADDRESSED. Verified at the new HEAD:

// packages/studio/src/player/lib/timelineDOM.ts:75 @ 68f008f4
tag: clip.tagName || clip.kind || "div",
// :106
tag: clip.tagName || clip.kind || "div",

Same pattern Miguel already had at :394 (params.tagName.toLowerCase() || "div"). All three populate sites now guarantee a non-empty string. This is the cleanest closure: with both populate paths producing "div" as a floor AND getTrackStyle()'s internal || "div" guard, the unguarded el.tag consumers in TimelineCanvas.tsx (:294 getTrackStyle(el.tag), :487 getTrackStyle(activeDraggedElement.tag), :495 same) are now safe by construction — they receive a non-empty string from upstream. No Pattern #1 redundancy concern since the consumer guard is getTrackStyle's internal one, not a duplicate at the call sites. (Side note: my R1 cited these unguarded consumers as living in Timeline.tsx, but they're in TimelineCanvas.tsx; Rames had it right. Apologies for the file misattribution — the substantive analysis stands.)

3. Widen the signature — not addressed. TimelineElement.tag is still typed string at playerStore.ts:27. Now that the populate sites guarantee a non-empty string at runtime, the type is no longer lying — it's accurate. So this nit is moot at the new HEAD; the type signature and the runtime invariant now agree. Closing.

CI

Fallow audit is FAILURE — but the findings are pre-existing (12 unused re-exports, 2 duplication groups, and a CRAP-score on createTimelineElementFromManifestClip at timelineDOM.ts:64). The CRAP score (31.8, threshold 30.0) flagged the very function the PR edits, so it's plausible the || "div" additions nudged it across the line — worth a quick look at the fallow output to confirm. Everything else required is green; Windows render verification is still IN_PROGRESS.

Re: Rames' R1

Rames converged on the same three findings and also called for the :75/:106 populate-site follow-up. Both R1 asks now closed in the same shape we both wanted.

---

LGTM. Land it once the Fallow audit signal is either cleared or confirmed pre-existing. The populate-path fix is exactly the right shape.

Review by Via

[james-russo-rames-d-jusso]

R2 verification @ HEAD 68f008f4

R1 was at a0f7dddd; new commit pushes the || "div" floor up to the populate sites at timelineDOM.ts:75 + :106 — exactly the root-cause fix shape Via and I both asked for in R1.

R1 resolution table

Item	Status	Evidence
🟡 #1 — type signature tag: string lies after runtime widens to nullish	✅ Resolved-differently	playerStore.ts:27 still types tag: string, but now the populate sites at timelineDOM.ts:75 + :106 guarantee `clip.tagName
🟡 #2 — PR body framing wrong (empty-string was already safe)	🛑 Still open	PR body unchanged: still claims toLowerCase() / startsWith() were "throwing on null/undefined/empty-string values" and the test plan still references the ""-fallback callers. Informational only — non-blocking. The next reader chasing PostHog stacks for an empty-string throw site won't find one.
🟡 #3 — observability gap when fallback fires	🛑 Still open	timelineIcons.tsx:42-43 const safeTag = tag || "div" silently absorbs nullish input; no console.warn, no PostHog capture. Same for the populate-site || "div" floors. Once the loud crash is suppressed, the dashboard cohort that flagged this goes silent. Same observability-theme open item Rames Jusso raised on Slack.

R2-NEW

• 🛑 Fallow audit FAILED at this HEAD (run 28063911334). Fallow report flags createTimelineElementFromManifestClip at timelineDOM.ts:64 with CRAP score 31.8 (threshold 30.0, cyclomatic 29). That's the very function this PR edits — adding || "div" at :75 + :106 adds 2 conditional branches per site, almost certainly the push over threshold. (The other 15 Fallow findings are pre-existing dead-export / duplication noise.) This blocks merge until either (a) the cyclomatic complexity is reduced (e.g. helper extraction for the tag-with-fallback computation), or (b) the threshold is adjusted. Per feedback_recently_added_flag_default_silent_revert-adjacent: a function this PR touched just crossed a hard gate. Worth confirming with Miguel whether || "div" was actually the trigger before merging.
• 🟢 Via R2 (LGTM, 23:30:56Z): converged on the populate-path closure verdict + flagged the same Fallow CRAP score signal. Acknowledges file misattribution (Timeline.tsx → TimelineCanvas.tsx) per my R1 cross-reviewer note.

CI

Fallow audit FAIL — see R2-NEW above, blocks merge.

All other required green. Studio load smoke + Windows render IN_PROGRESS at review time. The prior LFS-flake on Studio: load smoke cleared on the new run.

Verdict

REQUEST-CHANGES — but only on the Fallow audit gate. The populate-path root cause is closed exactly as Via and I both asked in R1; the patch shape is right. The Fallow CRAP gate just needs Miguel to either (a) extract clip.tagName || clip.kind || "div" into a resolveClipTag(clip) helper at module scope (cuts the cyclomatic count in the function body), or (b) document why the threshold should be bumped. Either is ~5 lines.

Stamp routing

Not stamping. Fallow gate open. Once Miguel clears the audit (or it turns out to be unrelated), tagging <@U0ARJFN5S6Q> for stamp.

— Rames D Jusso

[james-russo-rames-d-jusso]

R3 verification @ HEAD e25c255a — 🛑 REQUEST-CHANGES (regression introduced)

R2 was at 68f008f4 with verdict REQUEST-CHANGES on the Fallow audit gate. R3 push at 22:59:15Z. Fallow audit is now green, but the R3 push introduces a fatal infinite recursion that breaks the studio package entirely.

R3-NEW (load-bearing) — resolveClipTag is unconditional recursion

At packages/studio/src/player/lib/timelineDOM.ts:64-66 @ e25c255a:

function resolveClipTag(clip: ClipManifestClip): string {
  return resolveClipTag(clip);
}

The helper extraction Miguel did to reduce CRAP complexity in createTimelineElementFromManifestClip ships as a stub that calls itself. The two callsites at :75 (tag: resolveClipTag(clip) in getTimelineElementDisplayLabel) and :106 (same in the TimelineElement entry) now stack-overflow on every clip ingestion.

This is not a CI flake. The exact same RangeError: Maximum call stack size exceeded fires on:

• Test check (CI run) — 6 test failures across useExpandedTimelineElements.test.ts (4) and useTimelinePlayer.test.ts (2), all with stack frames pointing at timelineDOM.ts:65:10 recursing forever.
• Studio: load smoke check (CI run) — 9 Maximum call stack size exceeded runtime errors in the headless Chrome studio mount, blocking the smoke gate.

Two CI gates with the same root cause = real regression. Not the kind of failure that clears on rerun.

What R3 should have looked like

The Fallow CRAP gate just wanted the clip.tagName || clip.kind || "div" triple-short-circuit (from R2) extracted into a helper at module scope to drop the cyclomatic count inside the function body. The minimal fix:

function resolveClipTag(clip: ClipManifestClip): string {
  return clip.tagName || clip.kind || "div";
}

The body should compute the value, not call itself. Pattern is identical to the params.tagName.toLowerCase() || "div" floor already in place at :394 of the same file. The R2 floor (|| "div") was correct and load-bearing — it has been replaced by a function that crashes.

R2 resolution table

Item	R2 verdict	R3 status	Evidence
🛑 R2 — Fallow audit FAIL on createTimelineElementFromManifestClip (CRAP 31.8 > 30.0)	Was BLOCKER	✅ Resolved (paper-correctly)	Fallow audit SUCCESS at e25c255a. The complexity drop is real — the extraction removed branches from the parent function.
🛑 R2 — Fallow audit signal	Was BLOCKER	🛑 Replaced by harder blocker	Fallow green now BUT Test + Studio: load smoke both red on RangeError. Net: PR is further from mergeable than at R2.
🟡 R1 — observability theme (nullish-tag fallback fires silently)	Open after R2	✅ Partially addressed	timelineIcons.tsx:42 now emits console.warn("[Timeline] getTrackStyle received empty tag, defaulting to div"). Good fix on the consumer side. The populate-site silent fallback is moot because the populate-site code is broken (see R3-NEW).
🟡 R1 — PR body framing (empty-string was always safe)	Open after R2	🛑 Still open	PR body unchanged. Informational only.

CI

• 🛑 Test — FAILURE (6 tests, all RangeError: Maximum call stack size exceeded in resolveClipTag)
• 🛑 Studio: load smoke — FAILURE (9 runtime errors at studio mount, same cause)
• ✅ Fallow audit — SUCCESS (real, not a flake)
• ✅ All other required checks (Build, Lint, Typecheck, Format, CodeQL, SDK, etc.) SUCCESS

Verdict

REQUEST-CHANGES. Two required gates red on a load-bearing studio package because the helper extraction is recursive. Two-line fix: replace :65 (return resolveClipTag(clip);) with return clip.tagName || clip.kind || "div";. After that, the populate-site floor + the consumer-side guard at getTrackStyle + the new console.warn all compose correctly and the PR is in a substantially better shape than R2.

Stamp routing

Not stamping. Fatal regression; no path to ship until the recursion is fixed. Once the helper body computes the tag instead of calling itself and Test + Studio smoke go green, this is a clean LGTM — at that point routing to Rames Jusso (<@U0ARJFN5S6Q>) per James's stamp directive.

— Rames D Jusso

[vanceingalls]

Re-reviewed at e25c255afdf7e7d46290eb68d56d2b27c8d91cc8. R2 LGTM was at 68f008f4. New commit rebased onto main + extracted the inline tag-fallback into a helper. Posted alongside Rames's converged R3 — same finding, same ask.

BLOCK — infinite recursion in resolveClipTag

packages/studio/src/player/lib/timelineDOM.ts:64-66 introduces a helper that calls itself unconditionally:

function resolveClipTag(clip: ClipManifestClip): string {
  return resolveClipTag(clip);
}

Called from createTimelineElementFromManifestClip at :79 and :110, plus the matching timelineIcons.tsx rename. Every call path through createTimelineElementFromManifestClip stack-overflows on first invocation.

CI confirms — this is not theoretical:

• CI / Test: FAILURE — Maximum call stack size exceeded across 6 tests in useExpandedTimelineElements.test.ts + useTimelinePlayer.test.ts.
• CI / Studio: load smoke: FAILURE — 9 runtime Maximum call stack size exceeded errors in headless studio mount.

Both gates point at timelineDOM.ts:65 recursing.

Root cause + fix

The R2-approved expression was clip.tagName || clip.kind with the populate-path || "div" floor that Rames and I asked for at R1/R2 landing at 68f008f4 as clip.tagName || clip.kind || "div". The intended R3 refactor was to extract that triple-short-circuit into a helper to drop CRAP-score in the parent function (which the Fallow audit gate had asked for). Helper body should be:

function resolveClipTag(clip: ClipManifestClip): string {
  return clip.tagName || clip.kind || "div";
}

Identical pattern is already in the same file at :394 (params.tagName.toLowerCase() || "div"). The current body — return resolveClipTag(clip) — both crashes AND drops the || "div" floor the populate-path needs.

Per-item disposition (R2 deferred)

Fallow audit failure. Fallow audit at HEAD is SUCCESS — complexity drop is real (extraction removed branches from the parent). The gate cleared but the extraction shipped broken.

Ask

Fix the helper body to return the value, not call itself. Once that lands and CI's green, I'll re-stamp.

Review by Via

[jrusso1020]

LGTM at 8a82fe2e — recursion fix landed, CI flipped green (Test + Studio load smoke both pass now). The R1 populate-path concern at timelineDOM.ts:75,106 and the R2 Fallow CRAP gate both stay closed via the resolveClipTag helper extraction.

— Jerrai

[james-russo-rames-d-jusso]

R4 verification @ HEAD 8a82fe2e

Recursion regression fixed cleanly.

Fix verification

Item	Status	Evidence
R3 blocker: resolveClipTag infinite recursion	✅ Resolved	timelineDOM.ts:64-66 — helper now returns `clip.tagName
Load-bearing || "div" floor preserved	✅ Verified	:66 keeps the floor; both call sites at :75 (label) and :107 (tag field) consume the helper.
Fallow audit (CRAP-score gate)	✅ Green	Fallow audit SUCCESS at this HEAD.
CI gates that crashed on R3	✅ Green	Test, Studio: load smoke both SUCCESS (were red on RangeError: Maximum call stack size exceeded at e25c255a).
Lint, Typecheck, Format, regression, preview-regression	✅ Green	All SUCCESS. Windows-shard + CLI smoke still in-progress, non-fatal.

Verdict

LGTM. Deferring stamp to @rames-jusso (U0ARJFN5S6Q) per James's directive.

— Rames D Jusso

[vanceingalls]

LGTM — re-verified at 8a82fe2e

Re-verified at 8a82fe2ed64ffee2f51cbe8cecd2f2e5c324b17d. The R3 BLOCK is cleared.

Recursion fix

resolveClipTag body now returns the literal expression instead of calling itself:

// packages/studio/src/player/lib/timelineDOM.ts:64-66 @ 8a82fe2e
function resolveClipTag(clip: ClipManifestClip): string {
  return clip.tagName || clip.kind || "div";
}

No infinite recursion. Maximum call stack size exceeded is gone.

|| "div" floor preserved at every call site

Both ClipManifestClip-derived call sites flow through the helper, so they pick up the floor:

// :79  (label derivation)
tag: resolveClipTag(clip),
// :110 (element tag)
tag: resolveClipTag(clip),

The other tagName references in this file (:218, :247, :295, :389, :398) read from DOM elements or factory params, not ClipManifestClip, so they're out of scope for this helper; :398 already has its own inline || "div" floor for the params path. No gap.

CI

Both checks that failed at e25c255a are green:

• Test → SUCCESS
• Studio: load smoke → SUCCESS

Plus Test: runtime contract, SDK: unit + contract + smoke, CLI smoke (required) all SUCCESS. Tests on windows-latest still IN_PROGRESS but not a required blocker for this review.

Force-push collapsed the stack to a single commit. Recursion fix verified, floor preserved, CI converged. Ship it.

Review by Via