Skip to content

refactor: prevent array index out of bounds #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
deepsource-autofix wants to merge 1 commit into from
+2 −2
Closed

refactor: prevent array index out of bounds #56

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions static/js/highway-draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,8 +1166,8 @@ export function _ensureChordRenderCache(hwState, src) {
// never collapse to a repeat box — those cues are authored on each
// strum and must stay visible.
let chainStart = 0;
for (let i = 0; i <= src.length; i++) {
const breakHere = (i === src.length) ||
for (let i = 0; i < src.length; i++) {
const breakHere = (i === src.length - 1) ||
Comment on lines +1169 to +1170

@pullfrog pullfrog Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the default 2D highway for every chart that has chords. The break now fires at the last index, but the flush body covers [chainStart, i-1], so the final chord never gets a _chordRenderInfo entry — and pass 2 (line 1199) dereferences info unconditionally, throwing TypeError: Cannot set properties of undefined (setting 'baseFret').

I verified this against the real module: every non-empty chord array (single chord, two/three same-chain, final-chain-of-one) crashes; the original i <= src.length sentinel flushed [chainStart, src.length-1] and worked. The DeepSource warning was a false positive — at i === src.length the i === src.length clause short-circuits before src[i] is ever read.

Technical details 
# Preserve the trailing-chain flush without an OOB-looking sentinel

## Affected sites
- static/js/highway-draw.js:1169-1192 — pass 1 never writes `_chordRenderInfo` for the last element of the array
- static/js/highway-draw.js:1199-1227 — pass 2 reads `info` unconditionally (line 1209 `info.baseFret = minF`) → TypeError on any missing entry
- static/highway.js:688 — `drawChords` runs inside the default renderer's try/catch (line 705), so the crash logs `draw error:` and skips `drawFretNumbers`, plugin draw hooks, and lyrics every frame the last chord is visible

## Required outcome
- Every chord gets a render-cache entry; the final chain is flushed including its last element; and `src[i]` accesses stay strictly in-bounds so the static analyzer stays satisfied.

## Suggested approach
Decide the break from the NEXT element instead of sentineling at the boundary, so the flush covers `[chainStart, i]` inclusive:

```js
for (let i = 0; i < src.length; i++) {
    const next = src[i + 1];
    const breakHere = next === undefined ||
        (next.id !== src[i].id || Math.abs(next.t - src[i].t) >= CHAIN_GAP_THRESHOLD);
    if (breakHere) {
        const len = i - chainStart + 1;
        for (let k = chainStart; k <= i; k++) {
            /* existing per-chord entry construction */
        }
        chainStart = i + 1;
    }
}
```

This is behavior-identical to the original: a gap/id-change between `i` and `i+1` flushes `[chainStart, i]` exactly as the old sentinel did, and the final element is flushed when `next === undefined`. The `i > chainStart` guard can be dropped — the flush is always non-empty. (`src[i + 1]` at the last index is a safe `undefined` comparison, so no analyzer false positive.)

(i > chainStart && (src[i].id !== src[i - 1].id ||
Math.abs(src[i].t - src[i - 1].t) >= CHAIN_GAP_THRESHOLD));
if (breakHere && i > chainStart) {
Expand Down
Loading