Skip to content

Fix Wave audio progress listener accumulation - #662

Merged
minimaluminium merged 1 commit into
mainfrom
codex/fix-wave-timeupdate-listener
Oct 14, 2026
Merged

minimaluminium merged 1 commit into
mainfrom
codex/fix-wave-timeupdate-listener

Conversation

@minimaluminium

Copy link
Copy Markdown
Member

Playback accumulated a new listener on every progress event, causing increasing duplicate work.

Credits @angelday’s fix in #505.

Validated with 100 progress events (listener count stays at 1), a Wave build, and GScan.

Avoid registering a new listener on every playback update.

Co-authored-by: József Schaffer <sasvari@gmail.com>
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1ecb7a14-32a7-4ee4-9c69-76fe1e27d43d
📥 Commits

Reviewing files that changed from the base of the PR and between c85d008 and 860532f.

⛔ Files ignored due to path filters: 1
  • packages/wave/assets/built/main.min.js is excluded by !**/*.min.js
📒 Files selected for processing: 1
  • packages/wave/assets/js/main.js

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The playerAudio timeupdate handler now updates the current-time display and progress width directly on each event. It no longer registers a nested timeupdate listener. Duration display behavior is unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 86053

The change removes repeated progress-listener registration. The invalid-duration display issue predates this change, so no actionable merge-blocking risk remains.

### Architecture Summary

Architecture risk: 🔵 Low · up to 86053

The change affects 1 system.

Changed systems: packages/wave

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — packages/wave (library) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in packages/wave/assets/js/main.js: The timeupdate handler now updates current time and progress directly for each event, rather than registering a nested listener to perform those updates. The duration display remains in the outer handler.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description check Passed The description accurately explains the listener accumulation issue, the fix, and the reported validation steps.
Title check Passed The title clearly and concisely identifies the main change: fixing Wave audio progress listener accumulation.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@minimaluminium
minimaluminium merged commit 366237b into main Oct 14, 2026
68 checks passed
@minimaluminium
minimaluminium deleted the codex/fix-wave-timeupdate-listener branch October 14, 2026 08:58
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.

1 participant