Skip to content

refactor(studio): improve player store with zoom and element updates#61

Merged
miguel-heygen merged 1 commit intomainfrom
studio/1-player-store
Mar 28, 2026
Merged

refactor(studio): improve player store with zoom and element updates#61
miguel-heygen merged 1 commit intomainfrom
studio/1-player-store

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented Mar 26, 2026

Summary

  • Add zoom state (zoomMode, pixelsPerSecond) to player store
  • Add timeline element updates (setElements, clearElements)
  • Remove unused/duplicate exports from player barrel file

🤖 Generated with Claude Code

@miguel-heygen miguel-heygen force-pushed the studio/1-player-store branch 2 times, most recently from 2c6db38 to 4baafb7 Compare March 26, 2026 18:44
@miguel-heygen miguel-heygen changed the title refactor(studio): improve player store with zoom, element updates, and tests refactor(studio): improve player store with zoom and element updates Mar 26, 2026
@vanceingalls
Copy link
Copy Markdown
Collaborator

Review: PR #61 — Player Store Refactor

Clean removal of agent tracking state, good zoom/edit-range additions, solid tests.

Issues

[Important] setCurrentTime does not guard against NaN/Infinity. setDuration has if (!Number.isFinite(duration)) return but setCurrentTime does not. If an adapter returns NaN or Infinity, it flows straight into the store.

[Suggestion] updateElement accepts Partial<Pick<...>> but does not validate. Passing { start: NaN } or { duration: -1 } silently corrupts the element. Consider clamping like setDuration does.

What's well done

  • liveTime pub-sub pattern avoids React re-renders during playback — exactly right
  • setPixelsPerSecond clamps to minimum of 10
  • setDuration guards against non-finite values
  • Test coverage is thorough (265 lines) — covers all actions, edge cases, reset semantics
  • reset() intentionally preserves zoom/playbackRate with explicit test documentation

@miguel-heygen miguel-heygen force-pushed the studio/1-player-store branch from 4baafb7 to 7bc8e48 Compare March 26, 2026 20:48
@miguel-heygen
Copy link
Copy Markdown
Collaborator Author

Thanks for the review Vance!

Addressed:

  • setCurrentTime now guards against NaN/Infinity: set({ currentTime: Number.isFinite(time) ? time : 0 })

Not addressed (tracked for follow-up):

  • updateElement validation (clamping negative duration, NaN start) — will add in a follow-up PR with more comprehensive store validation

…d tests

- Add zoom state (zoomMode, pixelsPerSecond) to player store
- Add updateElementDuration, updateElementTrack, updateElement actions
- Remove agent activity tracking (activeEdits, agentId, agentColor)
- Add comprehensive store tests (265 lines)
- Add time utility tests
@miguel-heygen miguel-heygen force-pushed the studio/1-player-store branch from 7bc8e48 to 1442902 Compare March 27, 2026 19:36
Copy link
Copy Markdown
Collaborator Author

miguel-heygen commented Mar 28, 2026

Merge activity

  • Mar 28, 7:59 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Mar 28, 7:59 AM UTC: @miguel-heygen merged this pull request with Graphite.

@miguel-heygen miguel-heygen merged commit 625b513 into main Mar 28, 2026
14 checks passed
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.

2 participants