refactor(app): give formatTime a home — a leaf format.js, one fewer host hook (R3a)#895
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR extracts ChangesTime formatter extraction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ost hook (R3a) static/js/format.js (17). One function. Retires the formatTime hook: 12 -> 11. WHY A MODULE FOR ONE FUNCTION. formatTime was a host hook — loops.js and section-practice.js both reached back through the seam for it. It is ALSO, by pure accident of who calls it, inside the dependency closure of the library carve that comes next. Leaving it there would have made loops.js and section-practice.js import the LIBRARY in order to format a timestamp — nonsense, and a cycle waiting to happen. Same rule as the transport carve: a hook is a cycle you agreed to live with; an import is a dependency you actually have. formatTime has a real owner. It just isn't app.js, and it certainly isn't the library. Give it a home and both consumers import it directly. A leaf on purpose. Anything else that turns out to be a shared pure formatter belongs here too; nothing does yet (I checked — formatBadge, _safeImageUrl and _fetchJsonOrThrow have no callers outside the library), so nothing else is here. node 1040/1040, host contract 2/2, ESLint 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c5f85e6 to
e1d9463
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
static/js/juce-audio.js (1)
201-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
window.jucePlayertruthiness guards no longer match what they gate.Two spots still branch on
window.jucePlayereven thoughjucePlayeris now a directly-imported, always-defined singleton object fromtransport.js:
- Line 215:
if (window.jucePlayer) { jucePlayer._dur = dur; ... }— writes to the importedjucePlayer, but the guard checks a different reference (window.jucePlayer).- Line 273:
const pos = (window.jucePlayer ? jucePlayer.currentTime : 0) || 0;— same pattern; if the guard is ever false,possilently falls back to0even thoughjucePlayer.currentTimeis perfectly readable.Under standard ES module evaluation (no top-level
awaitin this graph),window.jucePlayer = jucePlayerin app.js runs synchronously before any of these deferred callbacks can fire, so this isn't currently reachable as a live bug — but it's a leftover from the old host-hook pattern (wherehost.jucePlayer()could genuinely be undefined) that no longer applies now that the binding is a direct import. Worth cleaning up as part of this exact migration rather than leaving a guard that checks the wrong thing.♻️ Proposed cleanup
- if (window.jucePlayer) { - jucePlayer._dur = dur; - jucePlayer._pos = pos; - jucePlayer._pollAt = performance.now(); - } + jucePlayer._dur = dur; + jucePlayer._pos = pos; + jucePlayer._pollAt = performance.now();- const pos = (window.jucePlayer ? jucePlayer.currentTime : 0) || 0; + const pos = jucePlayer.currentTime || 0;Also applies to: 270-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/juce-audio.js` around lines 201 - 219, Remove the stale window.jucePlayer truthiness guards in the deferred playback logic. In the block updating _dur, _pos, and _pollAt, access the imported jucePlayer directly, and in the position calculation use jucePlayer.currentTime directly while preserving the existing || 0 fallback.tests/js/song_event_payload.test.js (1)
132-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
allFrontendSources()is duplicated verbatim intests/js/song_seek.test.js.Same function, same comment block, copy-pasted across two test files. A directory-layout change (e.g. moving
static/jsor adding a nested subfolder) now has to be fixed in both places, and it's easy for the copies to silently drift.Consider extracting this into a small shared test helper (e.g.
tests/js/helpers/frontend-sources.js) that both files import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/js/song_event_payload.test.js` around lines 132 - 149, Extract the duplicated allFrontendSources helper and its source-discovery logic into a shared tests/js helper module, then import and use that shared function in both song_event_payload.test.js and song_seek.test.js. Preserve the existing behavior of reading static/app.js and JavaScript files under static/js.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/js/transport.js`:
- Around line 305-329: The JUCE branch of togglePlay must capture _audioSeekGen
before each asynchronous jucePlayer.pause() or jucePlayer.play() operation, then
return without updating state or emitting song:* events if the generation
changed while awaiting. Preserve the existing pause/play behavior when the
captured generation still matches.
---
Nitpick comments:
In `@static/js/juce-audio.js`:
- Around line 201-219: Remove the stale window.jucePlayer truthiness guards in
the deferred playback logic. In the block updating _dur, _pos, and _pollAt,
access the imported jucePlayer directly, and in the position calculation use
jucePlayer.currentTime directly while preserving the existing || 0 fallback.
In `@tests/js/song_event_payload.test.js`:
- Around line 132-149: Extract the duplicated allFrontendSources helper and its
source-discovery logic into a shared tests/js helper module, then import and use
that shared function in both song_event_payload.test.js and song_seek.test.js.
Preserve the existing behavior of reading static/app.js and JavaScript files
under static/js.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f61774e8-da58-4100-afcd-87a0e39d22da
📒 Files selected for processing (11)
static/app.jsstatic/js/count-in.jsstatic/js/format.jsstatic/js/juce-audio.jsstatic/js/loops.jsstatic/js/section-practice.jsstatic/js/transport.jstests/js/play_button_reroute_guard.test.jstests/js/playback_app_adapter.test.jstests/js/song_event_payload.test.jstests/js/song_seek.test.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
static/js/juce-audio.js (1)
201-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
window.jucePlayertruthiness guards no longer match what they gate.Two spots still branch on
window.jucePlayereven thoughjucePlayeris now a directly-imported, always-defined singleton object fromtransport.js:
- Line 215:
if (window.jucePlayer) { jucePlayer._dur = dur; ... }— writes to the importedjucePlayer, but the guard checks a different reference (window.jucePlayer).- Line 273:
const pos = (window.jucePlayer ? jucePlayer.currentTime : 0) || 0;— same pattern; if the guard is ever false,possilently falls back to0even thoughjucePlayer.currentTimeis perfectly readable.Under standard ES module evaluation (no top-level
awaitin this graph),window.jucePlayer = jucePlayerin app.js runs synchronously before any of these deferred callbacks can fire, so this isn't currently reachable as a live bug — but it's a leftover from the old host-hook pattern (wherehost.jucePlayer()could genuinely be undefined) that no longer applies now that the binding is a direct import. Worth cleaning up as part of this exact migration rather than leaving a guard that checks the wrong thing.♻️ Proposed cleanup
- if (window.jucePlayer) { - jucePlayer._dur = dur; - jucePlayer._pos = pos; - jucePlayer._pollAt = performance.now(); - } + jucePlayer._dur = dur; + jucePlayer._pos = pos; + jucePlayer._pollAt = performance.now();- const pos = (window.jucePlayer ? jucePlayer.currentTime : 0) || 0; + const pos = jucePlayer.currentTime || 0;Also applies to: 270-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/juce-audio.js` around lines 201 - 219, Remove the stale window.jucePlayer truthiness guards in the deferred playback logic. In the block updating _dur, _pos, and _pollAt, access the imported jucePlayer directly, and in the position calculation use jucePlayer.currentTime directly while preserving the existing || 0 fallback.tests/js/song_event_payload.test.js (1)
132-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
allFrontendSources()is duplicated verbatim intests/js/song_seek.test.js.Same function, same comment block, copy-pasted across two test files. A directory-layout change (e.g. moving
static/jsor adding a nested subfolder) now has to be fixed in both places, and it's easy for the copies to silently drift.Consider extracting this into a small shared test helper (e.g.
tests/js/helpers/frontend-sources.js) that both files import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/js/song_event_payload.test.js` around lines 132 - 149, Extract the duplicated allFrontendSources helper and its source-discovery logic into a shared tests/js helper module, then import and use that shared function in both song_event_payload.test.js and song_seek.test.js. Preserve the existing behavior of reading static/app.js and JavaScript files under static/js.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/js/transport.js`:
- Around line 305-329: The JUCE branch of togglePlay must capture _audioSeekGen
before each asynchronous jucePlayer.pause() or jucePlayer.play() operation, then
return without updating state or emitting song:* events if the generation
changed while awaiting. Preserve the existing pause/play behavior when the
captured generation still matches.
---
Nitpick comments:
In `@static/js/juce-audio.js`:
- Around line 201-219: Remove the stale window.jucePlayer truthiness guards in
the deferred playback logic. In the block updating _dur, _pos, and _pollAt,
access the imported jucePlayer directly, and in the position calculation use
jucePlayer.currentTime directly while preserving the existing || 0 fallback.
In `@tests/js/song_event_payload.test.js`:
- Around line 132-149: Extract the duplicated allFrontendSources helper and its
source-discovery logic into a shared tests/js helper module, then import and use
that shared function in both song_event_payload.test.js and song_seek.test.js.
Preserve the existing behavior of reading static/app.js and JavaScript files
under static/js.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f61774e8-da58-4100-afcd-87a0e39d22da
📒 Files selected for processing (11)
static/app.jsstatic/js/count-in.jsstatic/js/format.jsstatic/js/juce-audio.jsstatic/js/loops.jsstatic/js/section-practice.jsstatic/js/transport.jstests/js/play_button_reroute_guard.test.jstests/js/playback_app_adapter.test.jstests/js/song_event_payload.test.jstests/js/song_seek.test.js
🛑 Comments failed to post (1)
static/js/transport.js (1)
305-329: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file and surrounding symbols first ast-grep outline static/js/transport.js --view expanded || true printf '\n--- search for _audioSeekGen / _resetAudioSeekState / jucePlayer ---\n' rg -n "_audioSeekGen|_resetAudioSeekState|jucePlayer|togglePlay|sessionGen|attempt" static/js/transport.js printf '\n--- relevant slices ---\n' sed -n '1,220p' static/js/transport.js printf '\n--- middle slice ---\n' sed -n '220,420p' static/js/transport.jsRepository: got-feedBack/feedBack
Length of output: 20365
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect any tests or callers that might rely on JUCE togglePlay ordering/state rg -n "song:pause|song:play|song:resume|_audioSeekGen|_resetAudioSeekState|togglePlay\\(" static test tests src . || trueRepository: got-feedBack/feedBack
Length of output: 34969
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether JUCE mode state changes can happen during awaits from other paths rg -n "window\\._juceMode|S\\.isPlaying|setPlayButtonState\\(|feedBack\\.isPlaying|emit\\('song:(pause|play|resume)'" static/js/transport.js static/js || trueRepository: got-feedBack/feedBack
Length of output: 8881
Mirror the HTML5 session guard in the JUCE path. Capture
_audioSeekGenbefore theawaitand return early if it changed, so a song switch/teardown can’t let an old JUCE pause/play resolution overwrite the current session’sS.isPlayingorsong:*events.🧰 Tools
🪛 ast-grep (0.44.1)
[error] 314-314: React's useState should not be directly called
Context: setPlayButtonState(false)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.(usestate-direct-usage)
[error] 321-321: React's useState should not be directly called
Context: setPlayButtonState(true)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/transport.js` around lines 305 - 329, The JUCE branch of togglePlay must capture _audioSeekGen before each asynchronous jucePlayer.pause() or jucePlayer.play() operation, then return without updating state or emitting song:* events if the generation changed while awaiting. Preserve the existing pause/play behavior when the captured generation still matches.
1 participant
Why a module for one function
formatTimewas a host hook —loops.jsandsection-practice.jsboth reached back through the seam for it.It is also, by pure accident of who calls it, inside the dependency closure of the library carve that comes next. Leaving it there would have made
loops.jsandsection-practice.jsimport the library in order to format a timestamp — nonsense, and a cycle waiting to happen.Same rule as the transport carve (#894): a hook is a cycle you agreed to live with; an import is a dependency you actually have.
formatTimehas a real owner. It just isn't app.js, and it certainly isn't the library. Give it a home and both consumers import it directly.A leaf on purpose. Anything else that turns out to be a shared pure formatter belongs here too — nothing does yet. I checked:
formatBadge,formatBadgeInline,_safeImageUrland_fetchJsonOrThrowall look general, but none has a caller outside the library, so none of them moved. YAGNI applies to modules too.node 1040/1040 · host contract 2/2 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
M:SSformat, including seconds with leading zeroes.