fix: convert WASM byte offsets to code-unit offsets before slicing - #68
Open
technoligest wants to merge 1 commit into
Open
fix: convert WASM byte offsets to code-unit offsets before slicing#68technoligest wants to merge 1 commit into
technoligest wants to merge 1 commit into
Conversation
split_offsets returns UTF-8 byte offsets, but String.prototype.slice
indexes UTF-16 code units. The two coincide only for ASCII, so any
non-ASCII character desynchronizes them:
split_offsets('é cd', { delimiters: ' ', includeDelim: 'none' })
// [[0,2],[3,5]] -> text.slice() yields ["é ", "d"], losing the 'c'
RecursiveChunker, SentenceChunker and SemanticChunker all applied these
offsets directly to strings. Since RecursiveChunker rejoins whitespace
splits with a space, the damage was length-preserving and therefore
silent. An astral character drifts the boundary by 2 bytes, which can
land inside a surrogate pair and emit a lone surrogate - unrepresentable
in UTF-8, so downstream JSON APIs reject the chunk outright.
FastChunker was already correct: it slices the byte array and decodes,
rather than mixing coordinate systems.
Adds toCharOffsets, which walks the string once to map byte offsets to
code-unit offsets and returns ASCII input untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds UTF-8 byte to UTF-16 offset conversion and applies it to recursive, semantic, and sentence chunkers. New tests cover ASCII, accented characters, punctuation, emoji, surrogate pairs, chunk boundaries, and source slicing. ChangesUnicode offset conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Chunker
participant WASM_split_offsets
participant toCharOffsets
participant JavaScript_slice
Chunker->>WASM_split_offsets: request UTF-8 byte offsets
WASM_split_offsets-->>toCharOffsets: return offset pairs
toCharOffsets-->>Chunker: return UTF-16 code-unit pairs
Chunker->>JavaScript_slice: slice source text
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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 `@packages/core/tests/unicode-offsets.test.ts`:
- Line 82: Raise the repository and workspace Node engine lower bounds from 18
to 20, including the relevant declarations in package configuration and
pnpm-workspace.yaml. Update packages/core/tsconfig.json to use a library target
that defines String.prototype.isWellFormed(), then retain the isWellFormed()
test assertion in unicode-offsets.test.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61bca28c-b952-437c-998b-1269a86c2e2b
📒 Files selected for processing (5)
packages/core/src/offsets.tspackages/core/src/recursive.tspackages/core/src/semantic.tspackages/core/src/sentence.tspackages/core/tests/unicode-offsets.test.ts
| const chunks = await chunker.chunk(PARITY_CASES.emoji.wide); | ||
|
|
||
| for (const chunk of chunks) { | ||
| expect(chunk.text.isWellFormed()).toBe(true); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check declared TS lib/target and Node engine requirements
fd tsconfig.json | xargs -I{} sh -c 'echo "== {} =="; cat {}'
fd package.json -d 2 | xargs -I{} sh -c 'echo "== {} =="; jq ".engines" {} 2>/dev/null'
fd '.nvmrc|.tool-versions' -HRepository: feyninc/chonkiejs
Length of output: 2641
🏁 Script executed:
#!/bin/bash
set -e
echo "== package.json files with engines/scripts =="
git ls-files '*package.json' | while read -r f; do
echo "== $f =="
jq '{name, type, engines, scripts, dependencies, devDependencies}' "$f"
done
echo
echo "== workflow node-version declarations =="
if [ -d .github/workflows ]; then
fd -a . .github/workflows -t f | while read -r f; do echo "== $f =="; rg -n "node-version|nodejs|setup-node|vitest|pnpm|npm|yarn|bun" "$f" || true; done
else
echo "no .github/workflows directory"
fi
echo
echo "== unicode-offsets test relevant lines =="
fd unicode-offsets.test.ts -t f | while read -r f; do
echo "== $f =="
sed -n '1,180p' "$f"
done
echo
echo "== TypeScript declarations for isWellFormed in built-in libs =="
fd 'lib.es2023.*.d.ts|lib.es2024.*.d.ts|lib.esnext.*.d.ts' /var 2>/dev/null || trueRepository: feyninc/chonkiejs
Length of output: 9926
🏁 Script executed:
#!/bin/bash -
# Inspect packages/core test config inheritance path and any lockfile engine pins.
set -e
echo "== packages/core/package.json =="
jq '{name, type, engines, scripts, dependencies, devDependencies}' packages/core/package.json
echo
echo "== packages/core/tests/tsconfig.json =="
cat packages/core/tests/tsconfig.json
echo
echo "== packages/core/tsconfig.json =="
cat packages/core/tsconfig.json
echo
echo "== package manager files =="
git ls-files | rg '(^|/)(pnpm-lock.yaml|package-lock.json|yarn.lock|bun.lockb|bun.lock)$' || true
echo
echo "== lockfile engines/node declarations (first 120 matches) =="
for f in $(git ls-files | rg '(^|/)(pnpm-lock.yaml|package-lock.json|yarn.lock|bun.lockb|bun.lock)$' || true); do
echo "== $f =="
rg -n "engines:|node:|node-version|`@chonkiejs/core`|typescript|vitest" "$f" | head -120 || true
done
echo
echo "== TypeScript installed availability =="
node - <<'JS'
try {
const fs = require('fs');
const path = require('path');
for (const root of ['node_modules', process.cwd() + '/node_modules']) {
const file = path.join(root, 'typescript/lib/lib.es2024.string.d.ts');
if (fs.existsSync(file)) {
console.log('types file exists:', file);
const text = fs.readFileSync(file, 'utf8');
console.log(text.includes('isWellFormed') ? 'isWellFormed declaration present' : 'isWellFormed declaration absent');
}
}
console.log('node', process.version);
} catch (e) {
console.log('type check unavailable:', e.message);
}
JSRepository: feyninc/chonkiejs
Length of output: 10387
Raise the test/CI Node lower bound before using String.isWellFormed().
packages/core/tests inherits packages/core/tsconfig.json, which still uses lib: ["ES2021"], so these calls will fail type-checking unless that lib is updated. Add Node 20 to the repo and pnpm-workspace.yaml declarations too; the current workspace/engine config only requires Node 18, but String.prototype.isWellFormed() needs Node 20.
🤖 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 `@packages/core/tests/unicode-offsets.test.ts` at line 82, Raise the repository
and workspace Node engine lower bounds from 18 to 20, including the relevant
declarations in package configuration and pnpm-workspace.yaml. Update
packages/core/tsconfig.json to use a library target that defines
String.prototype.isWellFormed(), then retain the isWellFormed() test assertion
in unicode-offsets.test.ts.
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.
The bug
split_offsetsreturns UTF-8 byte offsets — as its own docstring inchunk.d.tssays — butString.prototype.sliceindexes UTF-16 code units. The two coincide only for ASCII, which is why this hasn't surfaced before. Any non-ASCII character desynchronizes them:éand–drift the boundary by 1 byte, an emoji by 2. Three chunkers apply these offsets straight to a string:recursive.ts— whitespace level and delimiter levelsentence.ts— the single-character-delimiter path (the multi-character defaults go throughsplitByPatternsOffsets, which is correct)semantic.ts— sentence preparationFastChunkeralready does it right — it slices theUint8Arrayand decodes — so the fix here just brings the other three in line.Why it's easy to miss
RecursiveChunkerrejoins whitespace splits with' ', so the corruption is length-preserving: characters are overwritten rather than dropped, and nothing downstream looks wrong. On a real 1246-page document I hit:SentenceChunker's symptom is different — boundaries land mid-word, andendIndexcan run pasttext.length(I sawendIndex69 on a 64-character string).The worst case is astral characters: a drift of 2 can land the boundary inside a surrogate pair. The resulting lone surrogate is unrepresentable in UTF-8, so
JSON.stringifyemits\udXXX, and downstream APIs reject the payload. That's how I found this — a reranking API returned400 … ensure that your input is encoded in valid UTF-8 formaton one chunk in 5,550.The fix
Adds
toCharOffsets(text, byteOffsets), which walks the string once to map UTF-8 byte offsets onto UTF-16 code-unit offsets, and returns ASCII input untouched so the common path stays free. The three chunkers now wrap theirsplit_offsetscalls with it.No changes to the Rust/WASM side — its contract is documented and correct; only the JS consumers were mixing coordinate systems.
Verification
packages/core/tests/unicode-offsets.test.tsadds 19 tests. The 15 integration tests all fail onmainand pass with this change; the 4toCharOffsetsunit tests cover the conversion directly.The chunker tests pair each non-ASCII input with an ASCII input of identical code-unit length (
cafévscafe,🔗 linkvsab link) and assert both produce identical chunk lengths and offsets — which isolates this bug fromRecursiveChunker's existing whitespace-delimiter behaviour.Against that 1246-page document (3.9 MB, 1,244 page segments, 182 emoji plus pervasive en-dashes), chunked at
chunkSize: 1024:The 65 remaining are not encoding-related: replacing every non-ASCII code unit with an ASCII one of the same length reproduces all 65 exactly, so they're the pre-existing whitespace-delimiter drop at the whitespace level (
includeDelim: 'none'discards the delimiter, and the rejoin restores one space). Encoding-specific corruption goes to 0. I left that separate behaviour alone since it's orthogonal to this fix — happy to look at it separately if it's considered a bug.Full suite: no regressions.
tests/code.test.ts(9) and oneinit-allcase fail identically before and after on my machine — they need an optional tree-sitter language pack.Summary by CodeRabbit
Bug Fixes
Tests