feat(editor): tap tempo for the selected sync point (Shift+B) - #93
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Tap tempo shortcut in Tempo Map mode, with tap-based median BPM estimation, Enter-to-apply and Escape-to-cancel handling, state reset on mode/selection changes, a dedicated test suite, and changelog documentation. ChangesTap Tempo Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant onKeyDown
participant TapTempoHandler
participant TempoMeasure
User->>onKeyDown: Shift+B tap
onKeyDown->>TapTempoHandler: record tap timestamp
TapTempoHandler->>TapTempoHandler: compute median BPM and status
User->>onKeyDown: Enter
onKeyDown->>TapTempoHandler: apply tapped BPM
TapTempoHandler->>TempoMeasure: set measure BPM
User->>onKeyDown: Escape
onKeyDown->>TapTempoHandler: cancel pending tap run
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
screen.js (2)
12824-12830: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePending taps array grows unbounded for the life of a run.
Only the last 9 entries ever feed
_tapTempoBpmPure, but_tapTempo.tapsitself is never trimmed. Held-key auto-repeat (or a very long tapping session) will keep appending indefinitely until reset/apply/cancel.♻️ Proposed fix
_tapTempo.taps.push(now); + if (_tapTempo.taps.length > 9) _tapTempo.taps = _tapTempo.taps.slice(-9); _tapTempo.bpm = _tapTempoBpmPure(_tapTempo.taps);🤖 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 `@screen.js` around lines 12824 - 12830, The tap-tempo state in the _tapTempo update block is letting _tapTempo.taps grow without limit even though _tapTempoBpmPure only uses the latest 9 taps. After pushing the new timestamp, trim the array to keep only the most recent 9 entries (or the maximum needed by _tapTempoBpmPure) while preserving the existing reset behavior tied to S.tempoSel and TAP_TEMPO_RESET_MS.
12803-12865: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the stateful control flow.
tests/tap_tempo.test.jsonly exercises_tapTempoBpmPure. The reset-on-2s-gap logic, staleness expiry on Enter, and the Enter/Escape resolution paths in_editorTapTempoAtSelection/_tapTempoHandleKey(the code that actually drives the single undoable_tempoSetMeasureBpmcall) are untested. These depend on globalSstate,performance.now(), and DOM focus checks, which is why they weren't captured by the existing@pureextraction pattern — worth a lightweight state-mocking test in a follow-up given this is the code path that mutates tempo data.🤖 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 `@screen.js` around lines 12803 - 12865, Add coverage for the stateful tap-tempo flow, not just _tapTempoBpmPure: write tests around _editorTapTempoAtSelection and _tapTempoHandleKey that mock global S state, performance.now(), and the focused-element guard. Verify the 2s reset behavior starts a fresh _tapTempo run after a gap, that Enter rejects stale taps past TAP_TEMPO_STALE_MS, and that Enter/Escape resolve the pending run correctly while exercising the _tempoSetMeasureBpm path. Use the existing tap-tempo symbols (_tapTempo, _editorTapTempoAtSelection, _tapTempoHandleKey, _tempoSetMeasureBpm) so the tests target the actual mutation flow.
🤖 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.
Nitpick comments:
In `@screen.js`:
- Around line 12824-12830: The tap-tempo state in the _tapTempo update block is
letting _tapTempo.taps grow without limit even though _tapTempoBpmPure only uses
the latest 9 taps. After pushing the new timestamp, trim the array to keep only
the most recent 9 entries (or the maximum needed by _tapTempoBpmPure) while
preserving the existing reset behavior tied to S.tempoSel and
TAP_TEMPO_RESET_MS.
- Around line 12803-12865: Add coverage for the stateful tap-tempo flow, not
just _tapTempoBpmPure: write tests around _editorTapTempoAtSelection and
_tapTempoHandleKey that mock global S state, performance.now(), and the
focused-element guard. Verify the 2s reset behavior starts a fresh _tapTempo run
after a gap, that Enter rejects stale taps past TAP_TEMPO_STALE_MS, and that
Enter/Escape resolve the pending run correctly while exercising the
_tempoSetMeasureBpm path. Use the existing tap-tempo symbols (_tapTempo,
_editorTapTempoAtSelection, _tapTempoHandleKey, _tempoSetMeasureBpm) so the
tests target the actual mutation flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22b1e80c-27c3-4c58-8d11-4989a45b5cf1
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/tap_tempo.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/tap_tempo.test.js (1)
23-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
new Function()used to eval regex-extracted source.Static analysis flags dynamic code execution here. In this context the input is the repo's own
screen.js(not user/network input), so it isn't exploitable, but it's brittle: any reformatting of the@pure:tap-tempo/@pure:tap-tempo-applycomment markers or surrounding code in screen.js silently breaks these tests (regexmatchreturningnullis already guarded for the second block but not shown for the first).Consider exporting these pure helpers via
module.exportsin screen.js (guarded by a Node check) andrequire-ing them directly instead of regex+eval — removes the SAST finding and the brittleness in one move.🤖 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/tap_tempo.test.js` around lines 23 - 33, The tests in tap_tempo.test.js are dynamically evaluating regex-extracted screen.js source via new Function(), which is brittle and triggers static analysis. Replace this pattern by exporting the pure helpers from screen.js (for example the _tapTempoBpmPure, _tapTempoStatusReasonPure, and _tapTempoApplyDecisionPure symbols) under a Node-only module.exports path, then import them directly in the test with require instead of parsing and evaling source text. Keep the existing regex-based extraction removed so the test no longer depends on comment marker formatting.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@tests/tap_tempo.test.js`:
- Around line 23-33: The tests in tap_tempo.test.js are dynamically evaluating
regex-extracted screen.js source via new Function(), which is brittle and
triggers static analysis. Replace this pattern by exporting the pure helpers
from screen.js (for example the _tapTempoBpmPure, _tapTempoStatusReasonPure, and
_tapTempoApplyDecisionPure symbols) under a Node-only module.exports path, then
import them directly in the test with require instead of parsing and evaling
source text. Keep the existing regex-based extraction removed so the test no
longer depends on comment marker formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41eb8d38-1f87-46a3-a348-992a13a017b2
📒 Files selected for processing (2)
screen.jstests/tap_tempo.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
Fulfills the planned tempoTapBpm registry command — the fastest way to rough in a tempo map for recordings made without a click. In Tempo Map mode with a sync point selected, tap Shift+B along with the recording: the MEDIAN of the last 8 inter-tap intervals (robust to one flubbed tap, unlike a mean) becomes a live BPM readout in the status bar. Enter applies it to the selected measure via _tempoSetMeasureBpm — one undoable TempoMapCmd — Esc cancels, and a >2 s pause starts a fresh run. Implausible results (outside 20-400 BPM) and non-monotonic clocks are rejected rather than offered; a stale pending run (>15 s) expires instead of applying. The Enter/Escape resolution runs ahead of the shortcut-profile dispatchers so neither profile can steal the keys, and only while a run is pending. Tests: tests/tap_tempo.test.js (7 cases: steady runs, median outlier rejection, last-8-interval window, plausibility bounds, bad clocks, even-count median). node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
A pending tap-tempo run survived leaving Tempo Map mode and changing the selected sync point, so a later Enter applied the STALE measure with no on-screen cue. Clear `_tapTempo` on mode toggle and on sync-point selection change, and re-validate at apply time: refuse (with a status cue) when the run's target no longer matches S.tempoSel via the new pure `_tapTempoApplyDecisionPure` guard. Also signal out-of-range (20–400 BPM) in the tap status instead of an always-"keep tapping" message, via `_tapTempoStatusReasonPure`. Tests: cover the apply-time stale-selection refusal and the out-of-range status reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d7b172b to
bc3bc8b
Compare
2 participants
Summary
Fulfills the previously-planned
tempoTapBpmregistry command (Shift+B in Tempo Map mode, both shortcut profiles) — the fastest way to rough in a tempo map for songs that weren't recorded to a click, before refining sync points by hand._tempoSetMeasureBpmpath), Esc cancels, and pausing >2 s starts a fresh run — a flubbed take is recoverable by waiting a beat and tapping again.Verification
node --check screen.jscleantests/tap_tempo.test.js— 7 cases via the@pure:tap-tempoblock (steady runs, median outlier rejection, last-8 window, plausibility bounds, bad-clock inputs, even-count median)tempoTapBpmflipsplanned→readyin the shortcut panel🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit