Harden fractional index generation - #29
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds bounded, interval-preserving retry candidates for binary and string keys. It validates generation limits and input lengths, replaces recursive midpoint construction with iterative logic, reduces batch allocations, updates documentation, and makes benchmarks deterministic. ChangesFractional-index retry generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Factory
participant MidpointGenerator
Caller->>Factory: request retry keys
Factory->>Factory: validate count and input lengths
Factory->>MidpointGenerator: subdivide the original interval
MidpointGenerator-->>Factory: return ordered midpoint keys
Factory-->>Caller: return retry candidates
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/lib/fractional-indexing-binary.bench.ts (1)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the
seededRandomhelper between the benchmark files. Both files define byte-identical generators with the same default seed. A single shared test/bench utility guarantees the two suites keep receiving the same workload, which is the stated goal of this change.
packages/core/src/lib/fractional-indexing-binary.bench.ts#L7-L15: importseededRandomfrom a shared bench utility module instead of defining it here.packages/core/src/lib/fractional-indexing-string.bench.ts#L22-L30: import the same sharedseededRandominstead of defining it here.🤖 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/src/lib/fractional-indexing-binary.bench.ts` around lines 7 - 15, Move the byte-identical seededRandom helper into a shared benchmark utility module, then remove the local definitions and import the shared helper in packages/core/src/lib/fractional-indexing-binary.bench.ts (lines 7-15) and packages/core/src/lib/fractional-indexing-string.bench.ts (lines 22-30). Preserve the existing default seed and generator behavior so both benchmarks receive the same workload.packages/core/src/lib/fractional-indexing-binary.ts (1)
262-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated balanced
fillhelper. Both modules now contain the same preallocate-and-fill algorithm, and only the key-generation callback differs. A shared generic helper that takes(lower, upper) => Tkeeps the two implementations from diverging later.
packages/core/src/lib/fractional-indexing-binary.ts#L262-L283: replace the localfillwith a call to the shared helper, passinggenerateKeyBetweenUnsafe.packages/core/src/lib/fractional-indexing-string.ts#L338-L359: replace the localfillwith a call to the same shared helper, passing a closure that applies...args.🤖 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/src/lib/fractional-indexing-binary.ts` around lines 262 - 283, Extract the duplicated balanced preallocate-and-fill algorithm into one shared generic helper accepting a `(lower, upper) => T` key-generation callback. In packages/core/src/lib/fractional-indexing-binary.ts lines 262-283, replace the local fill implementation with the helper using generateKeyBetweenUnsafe; in packages/core/src/lib/fractional-indexing-string.ts lines 338-359, replace its local fill implementation with the same helper using a closure that applies ...args.packages/core/src/factory.ts (1)
596-627: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared internal-error message for retry-key failures.
The
FraciErrorcode is already"INTERNAL_ERROR", but these two throws use"Could not generate retry key"while theINTERNAL_ERRORdocumentation covers generic internal errors and asks users to file an issue. Use the existing internal-error wording here, or add the exact message to the README troubleshooting section if this message is intentional.🤖 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/src/factory.ts` around lines 596 - 627, The retry-key failure paths in generateRetryKey use a message inconsistent with the shared INTERNAL_ERROR wording. Replace both “Could not generate retry key” messages with the existing standard internal-error message, reusing the established symbol or wording rather than adding new documentation.
🤖 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/README.md`:
- Around line 863-867: Update the compaction procedure in the README’s second
step to note the MAX_GENERATED_KEYS limit and instruct batching
generateNKeysBetween calls when rowCount exceeds 1,000,000, while preserving the
same total number and ordering of replacement keys.
---
Nitpick comments:
In `@packages/core/src/factory.ts`:
- Around line 596-627: The retry-key failure paths in generateRetryKey use a
message inconsistent with the shared INTERNAL_ERROR wording. Replace both “Could
not generate retry key” messages with the existing standard internal-error
message, reusing the established symbol or wording rather than adding new
documentation.
In `@packages/core/src/lib/fractional-indexing-binary.bench.ts`:
- Around line 7-15: Move the byte-identical seededRandom helper into a shared
benchmark utility module, then remove the local definitions and import the
shared helper in packages/core/src/lib/fractional-indexing-binary.bench.ts
(lines 7-15) and packages/core/src/lib/fractional-indexing-string.bench.ts
(lines 22-30). Preserve the existing default seed and generator behavior so both
benchmarks receive the same workload.
In `@packages/core/src/lib/fractional-indexing-binary.ts`:
- Around line 262-283: Extract the duplicated balanced preallocate-and-fill
algorithm into one shared generic helper accepting a `(lower, upper) => T`
key-generation callback. In packages/core/src/lib/fractional-indexing-binary.ts
lines 262-283, replace the local fill implementation with the helper using
generateKeyBetweenUnsafe; in packages/core/src/lib/fractional-indexing-string.ts
lines 338-359, replace its local fill implementation with the same helper using
a closure that applies ...args.
🪄 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: 47b1ada6-8671-4379-b18a-0a369a4ea06c
📒 Files selected for processing (12)
.changeset/safe-retries-balance.mdpackages/core/README.mdpackages/core/src/factory.test.tspackages/core/src/factory.tspackages/core/src/lib/decimal-binary.test.tspackages/core/src/lib/decimal-binary.tspackages/core/src/lib/decimal-string.test.tspackages/core/src/lib/decimal-string.tspackages/core/src/lib/fractional-indexing-binary.bench.tspackages/core/src/lib/fractional-indexing-binary.tspackages/core/src/lib/fractional-indexing-string.bench.tspackages/core/src/lib/fractional-indexing-string.ts
1 participant
What changed
Why
The previous conflict suffix strategy could move retry candidates outside the requested interval, especially for bounded and batched generation. Long valid inputs could also exhaust the call stack, and unbounded batch counts could trigger impractical allocations.
Impact
Generation is safer under conflicts and adversarial inputs, while preserving ordered results. A minor changeset is included because generation limits and retry behavior are now explicit API guarantees.
Validation
pnpm exec vp run checkpnpm exec vp run test(34 files, 177 tests)pnpm exec vp run typecheck(core plus supported Drizzle, Prisma, and TypeScript fixtures)Summary by CodeRabbit
New Features
Bug Fixes
Documentation