Skip to content

fix VerseRef.verseNum setter mis-port - #60

Merged
irahopkinson merged 1 commit into
mainfrom
fix-versenum-setter
Aug 10, 2026
Merged

fix VerseRef.verseNum setter mis-port#60
irahopkinson merged 1 commit into
mainfrom
fix-versenum-setter

Conversation

@irahopkinson

@irahopkinson irahopkinson commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

The verseNum setter assigned the backing field and nothing else, still carrying
its ToDo placeholder:

set verseNum(value: number) {
  // ToDo: replace or remove this placeholder
  this._verseNum = value;
}

The C# VerseRef.VerseNum
it ports does two more things:

set
{
    if (value < 0)
        throw new VerseRefException("VerseNum can not be negative");
    verseNum = (short)value;
    verse = null;          // <- clears the range/segment string
}

So the TS accepted negative verse numbers, and assigning verseNum left any
range or segment string in place:

const vref = new VerseRef('LUK', '3', '4b-5a');
vref.verseNum = 9;
vref.verse;        // '4b-5a' (stale) — now '9'
vref.hasMultiple;  // true (stale)    — now false

This mirrors #58, which fixed the sibling chapterNum setter the same way.
this._verse = undefined is used rather than null, per the repo convention of
preferring undefined for missing values, and the C# (short) cast is not
replicated.

Tests

Worked TDD: tests written first, confirmed red (3 failures — no throw on
negative, '4b-5a' and '4b' both stale), then fixed.

The C# test that actually covers the range-clearing is CopyVerseFrom — it sets
VerseNum = 9 on a LUK 3:4b-6a source, then asserts the copied Verse is
"9" rather than "4b-6a". It can't be ported yet: copyVerseFrom isn't
implemented in this port. Nothing else in VerseRefTests.cs assigns VerseNum
on a ranged ref, so the cases go in the existing
describe('Extra (TS-only tests)') > describe('Property setters') block
alongside the chapterNum ones:

  • negative throws VerseRefException
  • zero is allowed (boundary guard)
  • assigning verseNum clears a range ('4b-5a''9', hasMultiple false)
  • assigning verseNum clears a segment ('4b''9')

BuildVerseRefByProps already exercised verseNum = 0/15/17 and still passes —
those are plain numbers with no verse string to clear.

Also removes the now-fixed set verseNum bullet from CLAUDE.md's "Known porting
gaps".

⚠️ This is a consumer-visible behaviour change

Unlike chapterNum — which recursed into itself and blew the stack on every
assignment, so nothing could have depended on it — this setter works today.
Clearing _verse changes what the verse getter and hasMultiple return for
code that currently functions: anything that assigns verseNum to a ref holding
a range or segment and then reads verse will now get the new number instead of
the old range string.

That is the correct, C#-faithful behaviour, and the old value was stale — but it
is a break, not a pure bugfix in the semver sense. The downstream survey below
bounds the actual blast radius.

Downstream usage survey

Surveyed for real reachability of both setters across the paranext org (38
repos), eten-tech-foundation/scripture-editors,
and sillsdev/scripture-forge-platform-extensions.

No consumer reaches either setter.

Every .verseNum = / .chapterNum = assignment found is on a plain object, not
a VerseRef instance — SerializedVerseRef is an interface (src/verse-ref.ts:10),
so those are data properties with no accessor:

Repo Site Receiver type
paranext-core platform-scripture-editor.utils.ts:198,300 SerializedVerseRef
paranext-core inventory.web-view.tsx:97–104 SerializedVerseRef
paranext-core checks-side-panel.web-view.tsx:167–168 SerializedVerseRef
paranext-core usj-reader-writer.ts:3085–3162 inline {bookId, chapterNum, verseNum}
paratext-bible-internal-extensions send-receive.web-view.tsx:260 plain scr ref object
scripture-editors demos/…/usj-reader-writer.ts plain object (vendored copy)
scripture-editors packages/scribe/…/useModifiedMarkersForMenu.ts:45–47 commented out
scripture-forge-platform-extensions imports only Canon, SerializedVerseRef

The VerseRef class is barely used downstream. Only three TypeScript
new VerseRef(...) sites exist across all three targets, and each is a read-only
chained expression that never stores the instance:

  • paranext-core extensions/src/quick-verse/src/main.ts:191new VerseRef(selector).toJSON()
  • paranext-core lib/platform-bible-utils/src/scripture/scripture-util.ts:325,336.BBBCCC / .BBBCCCVVV
  • scripture-editors demos/platform/lib/platform-bible-utils/src/scripture-util.ts:225,236 — same two

VerseRef.fromJSON has zero uses, and there are no : VerseRef annotations or
as VerseRef casts anywhere. With no VerseRef instance ever held in a
variable, there is nothing for the setter to be invoked on. The same holds for
chapterNum, which also retroactively confirms #58 was safe.

(The many other new VerseRef hits in paranext-core are C# under c-sharp/
and c-sharp-tests/, using libpalaso's own class — unrelated to this package.)

Method: local clones grepped for assignment patterns, plus GitHub code search
across all three targets. Code search indexes default-branch HEAD, so it
reflects current main regardless of clone freshness; the commits the local
clones were behind by were separately diffed and introduce no new assignments
and no new new VerseRef. Blind spot: any private repo not visible to the
token used.

Bearing on the version bump

All four consumers declare "@sillsdev/scripture": "^2.0.5" (paranext-core
package.json and extensions/package.json, scripture-editors
packages/platform/package.json, scripture-forge-platform-extensions
package.json).

So a 3.0.0 would not propagate to any of them until someone widens the range
by hand, whereas a 2.x release is picked up on the next install — and per the
survey would be a no-op for all four. The practical risk to known consumers is
zero either way; a major would only be a conservative signal to unknown ones.
No version bump is included here — that call is left to the maintainer.

Verification

Rebased onto main at 2.0.6 (#61, #62) and re-verified from a clean npm ci
against the updated lockfile, since dependencies changed underneath this branch:

npm run lint && npm run prettier:ci && npm run build && npx vitest run
  • lint — clean at --max-warnings 0
  • prettier:ci — no files differ
  • buildtsc clean, dist/ regenerated with no unexpected diff
  • vitest run — 47 passed (3 files)
  • npx tsc -p tsconfig.test.json --noEmit — clean (CI doesn't run this; test
    files are otherwise unchecked)

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.68%. Comparing base (33883ee) to head (89f5f67).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   83.93%   84.68%   +0.74%     
==========================================
  Files           4        4              
  Lines         330      333       +3     
  Branches       77       80       +3     
==========================================
+ Hits          277      282       +5     
+ Misses         33       30       -3     
- Partials       20       21       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The setter assigned the backing field and nothing else, marked with a `ToDo`
placeholder. It was missing both of the other things the C# `VerseRef.VerseNum`
setter does:

    if (value < 0)
        throw new VerseRefException("VerseNum can not be negative");
    verseNum = (short)value;
    verse = null;

So negative verse numbers were accepted, and assigning `verseNum` left the
range/segment string in place. Setting `verseNum = 9` on `LUK 3:4b-5a` left a
stale `4b-5a` in the `verse` getter and `hasMultiple` still `true`.

Uses `this._verse = undefined` rather than `null`, per the repo convention of
preferring `undefined` for missing values. The C# `(short)` cast is not
replicated.

The C# test that covers the range-clearing is `CopyVerseFrom`, which sets
`VerseNum = 9` on a `LUK 3:4b-6a` source and then asserts the copied `Verse` is
`"9"`. That test cannot be ported yet — `copyVerseFrom` is not implemented in
this port — so the cases are added as TS-only tests instead: the negative
guard, the zero boundary, and the clearing of both a range and a segment.

`BuildVerseRefByProps` already exercised `verseNum = 0/15/17` and still passes;
those are plain numbers with no verse string to clear.

Behaviour change for consumers: unlike the sibling `chapterNum` setter fixed in
#58, this setter worked before, so code reading `verse`/`hasMultiple` after
assigning `verseNum` will see different results.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@irahopkinson
irahopkinson merged commit fb9b8e7 into main Aug 10, 2026
3 checks passed
@irahopkinson
irahopkinson deleted the fix-versenum-setter branch August 10, 2026 04:28
irahopkinson added a commit to eten-tech-foundation/scripture-editors that referenced this pull request Aug 25, 2026
Routine minor bump, not a security update: the advisory-ID set is
unchanged (GHSA-g2r8-wvmj-jf5w, GHSA-vp3h-ghgh-jr7g before and after),
so this neither closes nor opens a finding. No overrides needed.

Skipping 2.0.6 means this picks up two `VerseRef` setter repairs at
once. Neither changes our behaviour today — we hold no `VerseRef`
instance to assign to — but both were armed traps that would have
fired the first time we reached for that API, so taking the bump
disarms them ahead of the need rather than after it.

`chapterNum` (2.0.6, sillsdev/scripture#58) assigned the property to
itself behind a `ToDo` placeholder and recursed infinitely. There was
no guard in front of it, so every input threw `RangeError: Maximum call
stack size exceeded` — `= 5`, `= 0`, `= -1` alike. The property was
completely unusable.

`verseNum` (2.1.0, sillsdev/scripture#60) is the same class of
mis-port, and is flagged upstream as a potential breaking change
because it did work. It assigned the backing field and nothing else,
where the C# it ports also clears the verse string and rejects
negatives. So `new VerseRef('LUK','3','4b-5a')` then `verseNum = 9`
left `verse` reading '4b-5a' and `hasMultiple` true, describing the old
reference while `verseNum` described the new one.

Of the two, `verseNum` was the more dangerous to us: the recursion
fails loud on first use, while the stale string fails silent and flows
onward into anything reading `verse` or `toString()`.

We reach neither today. Both are instance setters on the `VerseRef`
class, and outside the vendored `demos/platform/lib` copy this repo
never touches the class — only `SerializedVerseRef`, an interface whose
`verseNum` is a plain data property with no setter. The two
`new VerseRef(...)` sites in platform-bible-utils' scripture-util.ts are
read-only expressions that never store the instance, and every
`verseNum`/`chapterNum` assignment in the tree targets a plain object or
a local number. The upstream release notes survey this repo by name and
reach the same conclusion.

2.1.0 declares no dependencies, so the lockfile diff is confined to the
three specifiers and the single package entry.

Verified with `nx run-many -t build test lint typecheck --skip-nx-cache`
across all 10 projects: green, 1793 tests passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
irahopkinson added a commit to eten-tech-foundation/scripture-editors that referenced this pull request Aug 27, 2026
Routine minor bump, not a security update: the advisory-ID set is
unchanged (GHSA-g2r8-wvmj-jf5w, GHSA-vp3h-ghgh-jr7g before and after),
so this neither closes nor opens a finding. No overrides needed.

Skipping 2.0.6 means this picks up two `VerseRef` setter repairs at
once. Neither changes our behaviour today — we hold no `VerseRef`
instance to assign to — but both were armed traps that would have
fired the first time we reached for that API, so taking the bump
disarms them ahead of the need rather than after it.

`chapterNum` (2.0.6, sillsdev/scripture#58) assigned the property to
itself behind a `ToDo` placeholder and recursed infinitely. There was
no guard in front of it, so every input threw `RangeError: Maximum call
stack size exceeded` — `= 5`, `= 0`, `= -1` alike. The property was
completely unusable.

`verseNum` (2.1.0, sillsdev/scripture#60) is the same class of
mis-port, and is flagged upstream as a potential breaking change
because it did work. It assigned the backing field and nothing else,
where the C# it ports also clears the verse string and rejects
negatives. So `new VerseRef('LUK','3','4b-5a')` then `verseNum = 9`
left `verse` reading '4b-5a' and `hasMultiple` true, describing the old
reference while `verseNum` described the new one.

Of the two, `verseNum` was the more dangerous to us: the recursion
fails loud on first use, while the stale string fails silent and flows
onward into anything reading `verse` or `toString()`.

We reach neither today. Both are instance setters on the `VerseRef`
class, and outside the vendored `demos/platform/lib` copy this repo
never touches the class — only `SerializedVerseRef`, an interface whose
`verseNum` is a plain data property with no setter. The two
`new VerseRef(...)` sites in platform-bible-utils' scripture-util.ts are
read-only expressions that never store the instance, and every
`verseNum`/`chapterNum` assignment in the tree targets a plain object or
a local number. The upstream release notes survey this repo by name and
reach the same conclusion.

2.1.0 declares no dependencies, so the lockfile diff is confined to the
three specifiers and the single package entry.

Verified with `nx run-many -t build test lint typecheck --skip-nx-cache`
across all 10 projects: green, 1793 tests passed.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant