fix(filter): create new VersionFilter instead of spreading

[stevekaplan123]

Description

Problem: Scrolling the reader while a translation is open in the resources panel crashes the app.

Fix 1 is the crash. Fixes 2 and 3 are in the same PR on purpose — Fix 2 is the other half of the commit that caused the crash, and Fix 3 is not optional once Fix 2 lands. See Why fixes 2 and 3 belong here for the reasoning.

1st Bug Description

When you open a translation, VersionBlock.js:153 creates a VersionFilter object:

const filter = new VersionFilter("The Koren Jerusalem Bible", "תנ״ך קורן", "en", "Genesis 1:1");
filter.listKey()   // "|The Koren Jerusalem Bible|en"  ✅

When you then scroll to a new segment, updateVersionCat re-pointed that filter at the new ref like this:

const copy = { ...filter, refList: ["Genesis 1:2"] };
copy.versionTitle  // "The Koren Jerusalem Bible"  ✅ data survived
copy.listKey()     // TypeError: copy.listKey is not a function  ❌ method gone

{...obj} copies the object's own fields, but listKey() lives on VersionFilter's prototype, not on the instance — so the spread silently drops it. ConnectionsPanelHeader.js:139 then does key={filter.listKey()} on the next render and throws.

This was introduced in 108b0d1, which replaced an in-place mutation with the spread.

Fix 1 — build a real VersionFilter (ReaderApp.js:1458)

newVersionRecentFilters[filterIndex] = new VersionFilter(
  prevFilter.versionTitle,
  prevFilter.versionTitleInHebrew,
  prevFilter.versionLanguage,
  segmentRef
);

Same data, but it's an actual class instance, so listKey() / toString() / equals() still exist.

Why fixes 2 and 3 belong in this PR

Fix 2 is the other half of the commit that caused Bug 1

108b0d15 (May 2025) was a single refactor with one goal: stop mutating state in ReaderApp.js. Two functions update the same "recent filters" arrays, and the refactor got each one wrong in a different way:

Function	What the refactor did	Result
updateVersionCat	Made a copy, but with {...filter} — the one copy that drops class methods	Bug 1, the crash above
openFilter	Skipped it entirely (git show 108b0d15 never mentions openFilter)	Still mutating state

So Fix 2 isn't an unrelated cleanup we noticed in passing. It's the same defect, in the same file, from the same commit, on the same data structure — one function was converted incorrectly, the other wasn't converted at all. Fixing only the first leaves the refactor half-done in exactly the place that produced the bug.

Fix 3 is forced by Fix 2, and is the reason they can't be split

The wrong key name predates 6f189a50 (Feb 2018), so it sat there for roughly seven years causing no harm. That's because Fix 2's bug was masking it: unshift had already written the correct array into state before setState ran, so setState writing to the wrong key simply didn't matter — the right key was already correct.

Remove the mutation and setState becomes the only thing writing that array. The wrong key stops being cosmetic:

// with Fix 2 but without Fix 3, after tapping Rashi:
this.state.recentFilters      // [Rashi]  ← written, but nobody reads this key
this.state.linkRecentFilters  // []       ← what the panel actually renders

ConnectionsPanel.js:126 then evaluates recentFilters[filterIndex].category with no guard for an empty array, which throws. Fix 2 without Fix 3 would be strictly worse than shipping neither — it trades one crash for another. They have to land together.

If you'd prefer a minimal PR

Fix 1 stands completely on its own and resolves the reported crash. Dropping Fixes 2 and 3 together is a coherent option. Dropping only Fix 3 is not.

Fix 2 — stop mutating state in openFilter (ReaderApp.js:1276)

openFilter grabbed the array straight out of state:

recentFilters = this.state.linkRecentFilters;  // NOT a copy — same array object
recentFilters.unshift(filter);                 // mutates this.state directly

Now it copies first:

recentFilters = [...recentFilters];
staleRecentFilters = [...staleRecentFilters];

Side benefit: two !== checks that were dead now work, because the array is a new object each time. TextList.js:70 compares this.props.recentFilters !== prevProps.recentFilters to decide whether to rebuild its list, and ReaderApp.js:436 uses the same comparison to fire an analytics pageview. Both were always false before.

Fix 3 — write to the state key the app actually reads (ReaderApp.js:1309)

- recentFilters: recentFilters,
+ linkRecentFilters: recentFilters,

The panel renders this.state.linkRecentFilters (ReaderApp.js:2199); nothing reads this.state.recentFilters. Concretely, tapping Rashi used to leave state like:

this.state.recentFilters      // [Rashi]  ← nobody reads this key
this.state.linkRecentFilters  // [Rashi]  ← correct, but only because unshift mutated it

Fix 3 is required by Fix 2. Link filters were working by accident: the old unshift had already mutated linkRecentFilters before setState ran, so the wrong key never mattered. Once Fix 2 removes the mutation, setState is the only thing writing the array — so without Fix 3, this.state.linkRecentFilters stays empty and ConnectionsPanel.js:126 throws on recentFilters[filterIndex].category.

🤖 Generated with Claude Code

[gitvelocity-reviewer]

📊 Code Quality Score: 5/100

Base Score 19 × ESF 0.25 (Micro: 21 lines, 1 file) = 4.75, rounded to 5

Category	Score	Factors
🔭 Scope	3/20	All changes are confined to ReaderApp.js, touching two methods within the connections/filter state management area. One new import (VersionFilter from Filter) is added.
🏗️ Architecture	2/20	No module boundary or dependency structure changes. VersionFilter was already available in Filter.js; the import is added to use the constructor directly rather than object spread.
⚙️ Implementation	5/20	Three fixes: shallow array copies via spread before mutation to avoid in-place state modification, correction of state key from 'recentFilters' to 'linkRecentFilters' in the link-mode setState call, and replacement of object spread reconstruction with new VersionFilter(...) to preserve the prototype chain and class methods listKey(), toString(), and equals().
⚠️ Risk	6/20	The wrong state key ('recentFilters') in the link setState path means link recent filters were silently not being stored under the correct key in production. The VersionFilter constructor argument order (versionTitle, versionTitleInHebrew, versionLanguage, segmentRef) must match the actual Filter.js constructor signature; a mismatch would introduce a new bug. The staleRecentFilters copy is added defensively but may not be strictly necessary.
✅ Quality	2/15	No tests accompany the three bug fixes. Inline comments in the diff explain the rationale for the array copies and the constructor-over-spread choice. No test drives the linkRecentFilters state key assignment or verifies VersionFilter method retention after reconstruction.
🔒 Perf / Security	1/5	No performance or security work is present. The shallow array copies add negligible allocation overhead.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}

[gitvelocity-reviewer]

📊 Code Quality Score: 12/100

31 × 0.4 (Small ESF after breadth bump from Micro base) = 12

Category	Score	Factors
🔭 Scope	7/20	The diff touches 11 files but the substantive logic changes are confined to ReaderApp.js within the connections/filter subsystem. No new endpoints, APIs, or subsystems are introduced.
🏗️ Architecture	5/20	VersionFilter is imported into ReaderApp.js and used to reconstruct filter objects, replacing an object-spread pattern. No new module boundaries, service dependencies, or design patterns are introduced.
⚙️ Implementation	8/20	Two distinct fixes: recentFilters and staleRecentFilters are shallow-copied with spread before mutation to avoid in-place state mutation, and newVersionRecentFilters[filterIndex] is rebuilt via new VersionFilter(prevFilter.versionTitle, prevFilter.versionTitleInHebrew, prevFilter.versionLanguage, segmentRef) to retain class methods that object spread would drop. A state key name bug (recentFilters → linkRecentFilters) is also corrected.
⚠️ Risk	7/20	Changes affect filter selection and version display state in the connections panel. A wrong fix could cause stale filter state or broken version rendering. No data persistence, migrations, or external API calls are involved, and the changes are reversible.
✅ Quality	3/15	No test file accompanies the changes. Inline comments in ReaderApp.js explain the rationale for the copy-before-edit pattern and the new VersionFilter(...) reconstruction, but no automated coverage is added for these paths.
🔒 Perf / Security	1/5	The array spreads are O(n) on small in-memory arrays. No security considerations are present.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}