refactor(app): carve the tuning-display helpers out of app.js (R3a) - #884
Merged
Conversation
static/js/tuning-display.js (228 lines) — bodies VERBATIM. app.js 9,838 → 9,650.
A LEAF: imports nothing.
Tuning NAME resolution (Drop D / Eb Standard / raw-offset fallback), bass
detection, effective string count, and the target FREQUENCIES + note names the
tuner checks against. Pure functions over a small MIDI/note-name table; the 3
_TUNING_* tables are read nowhere else and move in.
NOT A SLICE — a node-level extract. The span 2309-2535 INTERLEAVES the functions
with the `window.*` / `window.feedBack.*` assignments that publish them, and one
of those is `window.feedBack = window.feedBack || {}` — the BUS BOOTSTRAP, not
tuning code at all. Every ExpressionStatement stays exactly where it was; only the
16 functions and 3 tables move. app.js re-exposes the imported bindings from the
same lines, so the public surface and its ordering are untouched (constitution II
names window.feedBack).
app.js -> { plugin-loader, viz, diagnostics-export, dom, highway-colors,
tuning-display }
HARNESSES — 4 broke, and 3 of them broke in the SAME informative way: they sliced
app.js from `function isBassArrangement(` UP TO the marker
`window.feedBack.parseRawTuningOffsets = parseRawTuningOffsets;` — an end-marker
that (correctly) stayed behind in app.js. The module is now nothing BUT the tuning
helpers, so there is no block to slice: they read it whole and strip `export ` so
the vm sandbox still evaluates it as a script.
tuner_auto_open is SPLIT — its autoplay-gate test still reads app.js, so it keeps
APP_JS and gains TUNING_JS. Retargeting its path wholesale (my first attempt)
silently pointed the autoplay test at the wrong file.
VERIFIED BY DRIVING THE CONTRACT. A/B against origin/main in two browsers, through
the real window surface: displayTuningName -> "E Standard" / "Drop D" /
"Eb Standard", parseRawTuningOffsets('-2,0,0,0,0,0') -> [-2,0,0,0,0,0],
isBassArrangement, effectiveStringCount, displayTuningTargets, and
window.feedBack.displayTuningName / .songTuningContext — IDENTICAL on both, zero
console/page errors either side.
pytest 2396, node 1038/1038, ESLint 0, tailwind-fresh clean, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe tuning helper implementation moves from ChangesTuning display module extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
static/js/tuning-display.js (1)
63-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
typeof parseRawTuningOffsets === 'function'guard.Since
parseRawTuningOffsetsis a function declaration hoisted within this same module, it will always be defined by the timedisplayTuningNameexecutes — thetypeofcheck is dead defensive code that no longer serves a purpose now that both live in one leaf module.🤖 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/tuning-display.js` around lines 63 - 83, Remove the redundant typeof/function guard in displayTuningName and call the hoisted parseRawTuningOffsets function directly when parsing raw tuning offsets. Preserve the existing parsed-length check and Custom Tuning fallback.
🤖 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/tuning-display.js`:
- Around line 16-25: Update _looksLikeRawTuningOffsets to remove the ambiguous
fallback regex and avoid backtracking on long malformed strings. Instead, split
the trimmed input on the supported separators, validate each resulting token as
a signed integer, and preserve the function’s existing boolean behavior for
valid and invalid tuning-offset metadata.
---
Nitpick comments:
In `@static/js/tuning-display.js`:
- Around line 63-83: Remove the redundant typeof/function guard in
displayTuningName and call the hoisted parseRawTuningOffsets function directly
when parsing raw tuning offsets. Preserve the existing parsed-length check and
Custom Tuning fallback.
🪄 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: 8f5370bd-3201-415d-a72d-8ebc912a7290
📒 Files selected for processing (6)
static/app.jsstatic/js/tuning-display.jstests/js/tuner_auto_open.test.jstests/js/tuning_display.test.jstests/js/tuning_targets.test.jstests/js/v3_songs_tuning.test.js
Comment on lines
+16
to
+25
| function _looksLikeRawTuningOffsets(str) { | ||
| if (!str || typeof str !== 'string') return false; | ||
| const s = str.trim(); | ||
| if (!s) return false; | ||
| if (/^-?\d+$/.test(s)) return true; | ||
| if (/^-?\d+(?: -?\d+)+$/.test(s)) return true; | ||
| if (/^-?\d+(?:,-?\d+)+$/.test(s)) return true; | ||
| if (/^-?\d+(-?\d+){2,}$/.test(s)) return true; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== File excerpt ==\n'
nl -ba static/js/tuning-display.js | sed -n '1,120p'
printf '\n== Regex timing probe ==\n'
node <<'JS'
const re = /^-?\d+(-?\d+){2,}$/;
function probe(label, s) {
const t0 = process.hrtime.bigint();
const ok = re.test(s);
const ms = Number(process.hrtime.bigint() - t0) / 1e6;
console.log(label, { len: s.length, ok, ms: ms.toFixed(3) });
}
for (const n of [10, 100, 1_000, 10_000, 100_000, 300_000]) {
probe(`digits+X ${n}`, '1'.repeat(n) + 'X');
}
for (const n of [10, 100, 1_000, 10_000, 100_000, 300_000]) {
probe(`dash-digits+X ${n}`, ('-1'.repeat(n)) + 'X');
}
for (const n of [10, 100, 1_000, 10_000, 100_000, 300_000]) {
probe(`tokens+X ${n}`, Array(n).fill('123').join('-') + 'X');
}
JSRepository: got-feedBack/feedBack
Length of output: 219
🏁 Script executed:
set -euo pipefail
printf 'waiting'Repository: got-feedBack/feedBack
Length of output: 166
🏁 Script executed:
set -euo pipefail
echo '--- static/js/tuning-display.js (1-140) ---'
nl -ba static/js/tuning-display.js | sed -n '1,140p'
echo
echo '--- regex timing probe ---'
node <<'JS'
const re = /^-?\d+(-?\d+){2,}$/;
for (const n of [10, 100, 1_000, 10_000, 100_000]) {
const s = '1'.repeat(n) + 'X';
const t0 = process.hrtime.bigint();
const ok = re.test(s);
const ms = Number(process.hrtime.bigint() - t0) / 1e6;
console.log(JSON.stringify({ n, ok, ms: +ms.toFixed(3) }));
}
JSRepository: got-feedBack/feedBack
Length of output: 243
🏁 Script executed:
set -euo pipefail
echo '--- static/js/tuning-display.js (1-120) ---'
awk 'NR>=1 && NR<=120 { printf "%d:%s\n", NR, $0 }' static/js/tuning-display.js
echo
echo '--- references to _looksLikeRawTuningOffsets / displayTuningName ---'
rg -n "_looksLikeRawTuningOffsets|displayTuningName" static/js/tuning-display.js static/js 2>/dev/null || true
echo
echo '--- regex timing probe ---'
node <<'JS'
const re = /^-?\d+(-?\d+){2,}$/;
function probe(label, s) {
const t0 = process.hrtime.bigint();
const ok = re.test(s);
const ms = Number(process.hrtime.bigint() - t0) / 1e6;
console.log(JSON.stringify({ label, len: s.length, ok, ms: +ms.toFixed(3) }));
}
for (const n of [10, 100, 1_000, 10_000, 50_000, 100_000, 200_000]) {
probe(`digits+X ${n}`, '1'.repeat(n) + 'X');
}
for (const n of [10, 100, 1_000, 10_000, 50_000, 100_000, 200_000]) {
probe(`dashdigits+X ${n}`, ('-1'.repeat(n)) + 'X');
}
for (const n of [10, 100, 1_000, 10_000, 50_000, 100_000, 200_000]) {
probe(`tokens+X ${n}`, Array(n).fill('123').join('-') + 'X');
}
JSRepository: got-feedBack/feedBack
Length of output: 151
🏁 Script executed:
set -euo pipefail
echo 'waiting for the file excerpt and regex probe results'Repository: got-feedBack/feedBack
Length of output: 212
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re, time
path = Path('static/js/tuning-display.js')
text = path.read_text()
lines = text.splitlines()
print('--- static/js/tuning-display.js (1-120) ---')
for i, line in enumerate(lines[:120], 1):
print(f'{i}:{line}')
print('\n--- matching lines ---')
for needle in ('_looksLikeRawTuningOffsets', 'displayTuningName'):
for i, line in enumerate(lines, 1):
if needle in line:
print(f'{i}:{line}')
print('\n--- regex timing probe ---')
re_pat = re.compile(r'^-?\d+(-?\d+){2,}$')
tests = [
('digits+X', lambda n: '1' * n + 'X'),
('dashdigits+X', lambda n: '-1' * n + 'X'),
('tokens+X', lambda n: '-'.join(['123'] * n) + 'X'),
]
for label, make in tests:
for n in [10, 100, 1000, 10_000, 50_000, 100_000]:
s = make(n)
t0 = time.perf_counter()
ok = bool(re_pat.fullmatch(s))
ms = (time.perf_counter() - t0) * 1000
print(f'{label} {n}: ok={ok} ms={ms:.3f}')
PYRepository: got-feedBack/feedBack
Length of output: 151
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re, time, signal, sys
path = Path('static/js/tuning-display.js')
text = path.read_text()
lines = text.splitlines()
print('--- context ---')
for i, line in enumerate(lines, 1):
if 1 <= i <= 60:
print(f'{i}:{line}')
print('\n--- bounded regex probe ---')
pat = re.compile(r'^-?\d+(-?\d+){2,}$')
class Timeout(Exception): pass
def handler(signum, frame):
raise Timeout()
signal.signal(signal.SIGALRM, handler)
tests = [
('digits+X', lambda n: '1' * n + 'X'),
('dashdigits+X', lambda n: '-1' * n + 'X'),
('tokens+X', lambda n: '-'.join(['123'] * n) + 'X'),
]
for label, make in tests:
for n in [1, 2, 3, 4, 5, 6, 8, 10, 12, 14, 16, 18, 20]:
s = make(n)
signal.alarm(1)
try:
t0 = time.perf_counter()
ok = bool(pat.fullmatch(s))
ms = (time.perf_counter() - t0) * 1000
print(f'{label} {n}: ok={ok} ms={ms:.4f}')
except Timeout:
print(f'{label} {n}: TIMEOUT')
break
finally:
signal.alarm(0)
PYRepository: got-feedBack/feedBack
Length of output: 4143
Replace the ambiguous fallback regex in static/js/tuning-display.js:16-25. /-?\d+(-?\d+){2,}$/ backtracks heavily on malformed long digit strings, so this display-only path can stall on adversarial metadata. Split on separators and validate each token instead.
🤖 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/tuning-display.js` around lines 16 - 25, Update
_looksLikeRawTuningOffsets to remove the ambiguous fallback regex and avoid
backtracking on long malformed strings. Instead, split the trimmed input on the
supported separators, validate each resulting token as a signed integer, and
preserve the function’s existing boolean behavior for valid and invalid
tuning-offset metadata.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sixth carve.
static/js/tuning-display.js(228 lines) — bodies verbatim.app.js9,838 → 9,650. A leaf: imports nothing.Tuning name resolution (Drop D / Eb Standard / raw-offset fallback), bass detection, effective string count, and the target frequencies + note names the tuner checks against. Pure functions over a small MIDI/note-name table; the three
_TUNING_*tables are read nowhere else and move in.Not a slice — a node-level extract
The span (2309–2535) interleaves the functions with the
window.*/window.feedBack.*assignments that publish them. And one of those interleaved lines is:Slicing the range would have moved the bus bootstrap into a tuning module. So every
ExpressionStatementstays exactly where it was; only the 16 functions and 3 tables move. app.js re-exposes the imported bindings from the same lines, so the public surface and its ordering are untouched (constitution §II nameswindow.feedBack).The harnesses broke in an informative way
Four broke; three for the same reason. They sliced app.js from
function isBassArrangement(up to the marker:…an end-marker that (correctly) stayed behind in app.js. The module is now nothing but the tuning helpers, so there's no block to slice — they read it whole and strip
exportso the vm sandbox still evaluates it as a script.tuner_auto_openis SPLIT — one of its tests reads app.js for the autoplay gate, which stayed. Retargeting its path wholesale (my first attempt) silently pointed the autoplay test at the wrong file and it failed loudly. It now keepsAPP_JSand gainsTUNING_JS.Verified by driving the contract
A/B against
origin/mainin two browsers, through the realwindowsurface:displayTuningName(null,[0,0,0,0,0,0])E StandarddisplayTuningName(null,[-2,0,0,0,0,0])Drop DdisplayTuningName(null,[-1×6])Eb StandardparseRawTuningOffsets('-2,0,0,0,0,0')[-2,0,0,0,0,0]isBassArrangement/effectiveStringCount/displayTuningTargetswindow.feedBack.displayTuningName/.songTuningContextfunctionpytest 2396 · node 1038/1038 · ESLint 0 · tailwind-fresh clean · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests