Refetch documents by $sequence#916
Conversation
📝 WalkthroughWalkthrough
ChangesRefetch and replay operator-computed documents
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Greptile SummaryThis PR fixes several bugs in the operator-driven document refetch path: switching from
Confidence Score: 5/5Safe to merge; the core logic is correct for all current adapters and the three new e2e tests exercise the precise failure modes the PR addresses. All known adapters handle the one remaining inconsistency (castingAfter called twice on refetched batch documents) idempotently — SQL's castingAfter is an identity function and Mongo's casts are safe to repeat. No data-corrupting path was found. src/Database/Database.php — specifically the castingAfter call in the batch operator loop, which is architecturally inconsistent with the explicit decode guard added in the same PR. Important Files Changed
Reviews (5): Last reviewed commit: "check double decode test. Add refetch in..." | Re-trigger Greptile |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
812-842: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd an explicit limit to the refetch query.
find()falls back to a 25-row limit when noQuery::limit()is present, so bulk calls torefetchDocuments()can return stale pre-operator documents for everything past the first 25. UseQuery::limit(count($sequences))here, and chunk the sequence list if needed for adapter limits.🤖 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 `@src/Database/Database.php` around lines 812 - 842, The refetchDocuments() flow currently calls find() with only a Query::equal('$sequence', $sequences) filter, so it can silently stop at the default 25-result cap. Update the refetch query in Database::refetchDocuments() to include Query::limit(count($sequences)), and if the sequence list can exceed adapter query limits, split the lookup into chunks and merge the results before rebuilding the refetchedMap.
🤖 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.
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 812-842: The refetchDocuments() flow currently calls find() with
only a Query::equal('$sequence', $sequences) filter, so it can silently stop at
the default 25-result cap. Update the refetch query in
Database::refetchDocuments() to include Query::limit(count($sequences)), and if
the sequence list can exceed adapter query limits, split the lookup into chunks
and merge the results before rebuilding the refetchedMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91a5b9c5-ff4c-40bf-97eb-ffa188eb1459
📒 Files selected for processing (1)
src/Database/Database.php
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 `@src/Database/Database.php`:
- Around line 827-833: The refetch logic in refetchDocuments() is still relying
on the default find() limit, so batches larger than 25 can leave later documents
using stale computed/operator values. Update the query passed to
Database::find() to explicitly bound it to the current batch size (or the number
of requested sequence IDs) using Query::limit(), alongside the existing
Query::equal('$sequence', $sequences) and $selections so every document in the
batch is actually refetched.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b45f00f-1ddd-49aa-b7e4-a4c42b4548ca
📒 Files selected for processing (1)
src/Database/Database.php
Summary by CodeRabbit
Bug Fixes
Tests