fix(engine): limit the number of snapshots returned when getting latest snapshots since #2550
Conversation
…st snapshots since
|
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
| orderBy: { createdAt: "desc" }, | ||
| take: 50, | ||
| }); | ||
|
|
||
| return snapshots.map(enhanceExecutionSnapshot); | ||
| return snapshots.reverse().map(enhanceExecutionSnapshot); | ||
| } |
There was a problem hiding this comment.
Fix pagination: current desc + reverse drops older snapshots.
With the new take: 50 cap, ordering desc and then reversing means we only ever return the latest 50 snapshots after sinceSnapshot. If more than 50 snapshots were created since then, the earlier ones (closest to sinceSnapshot) never get returned, so clients can’t page through the backlog without data loss. Please keep the query ascending so the first call yields the oldest 50 after the cursor.
- orderBy: { createdAt: "desc" },
- take: 50,
+ orderBy: { createdAt: "asc" },
+ take: 50,
});
- return snapshots.reverse().map(enhanceExecutionSnapshot);
+ return snapshots.map(enhanceExecutionSnapshot);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| orderBy: { createdAt: "desc" }, | |
| take: 50, | |
| }); | |
| return snapshots.map(enhanceExecutionSnapshot); | |
| return snapshots.reverse().map(enhanceExecutionSnapshot); | |
| } | |
| const snapshots = await prisma.executionSnapshot.findMany({ | |
| where: { /* ...existing filters...*/ }, | |
| orderBy: { createdAt: "asc" }, | |
| take: 50, | |
| }); | |
| return snapshots.map(enhanceExecutionSnapshot); | |
| } |
🤖 Prompt for AI Agents
In internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
around lines 219 to 224, the code queries snapshots with orderBy: { createdAt:
"desc" } and then reverses the result, which with take: 50 returns the newest 50
snapshots and drops older ones; change the query to orderBy: { createdAt: "asc"
} and remove the .reverse() so the database returns the oldest 50 snapshots
after sinceSnapshot (allowing correct pagination), then continue to map the
results with enhanceExecutionSnapshot.
No description provided.