fix(file-browser): keep recursive paginated entries reachable (#345) - #357
fix(file-browser): keep recursive paginated entries reachable (#345)#357kimbotai1337 wants to merge 2 commits into
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements a multi-part fix for recursive directory-tree pagination (issue ChangesFile-browser recursive pagination fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9736fc265d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 (1)
server/lib/config.ts (1)
82-84: ⚡ Quick winConsider validating FILE_BROWSER_MAX_TREE_ENTRIES and warning on invalid values.
The parsing logic will silently coerce invalid values (e.g.,
"abc","0") to1without feedback. While safe, this could confuse users attempting to configure the limit for testing scenarios described in the plan document.✨ Suggested validation in `validateConfig()`
Add a validation block in the
validateConfig()function (after line 312):// ── FILE_BROWSER_MAX_TREE_ENTRIES validation ─────────────────────────── if (process.env.FILE_BROWSER_MAX_TREE_ENTRIES) { const parsed = Number(process.env.FILE_BROWSER_MAX_TREE_ENTRIES); if (Number.isNaN(parsed) || parsed < 1) { console.warn( `[config] ⚠ FILE_BROWSER_MAX_TREE_ENTRIES="${process.env.FILE_BROWSER_MAX_TREE_ENTRIES}" ` + `is invalid — using minimum value of 1` ); } }🤖 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 `@server/lib/config.ts` around lines 82 - 84, The FILE_BROWSER_MAX_TREE_ENTRIES parsing silently coerces invalid inputs; add validation in validateConfig() to detect when process.env.FILE_BROWSER_MAX_TREE_ENTRIES is present but not a positive number and emit a console.warn (or processLogger.warn) showing the original value and that a minimum of 1 (or the fallback) will be used; locate the config export that sets fileBrowserMaxTreeEntries and add the suggested Number(parsing) + Number.isNaN / < 1 check after that in validateConfig(), logging a clear warning message including the raw environment string.
🤖 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
`@docs/residual-review-findings/fix-issue-345-file-browser-recursive-cursor.md`:
- Line 96: The docs currently include a machine-local path
`/tmp/compound-engineering/ce-code-review/20260522-091008-3574e918/` which is
inaccessible to other reviewers; replace that hardcoded `/tmp/...` entry in the
document with a reproducible reference (for example: a CI artifact URL, the
attached PR artifact name, or a repo-relative path under
docs/artifacts/<artifact-name>), and update any surrounding text to instruct
readers how to retrieve the artifact (e.g., “See CI artifact: <url>” or “See
artifact: reviews/20260522-3574e918.json”); ensure the symbol/string
`/tmp/compound-engineering/ce-code-review/...` is removed or replaced throughout
the file.
---
Nitpick comments:
In `@server/lib/config.ts`:
- Around line 82-84: The FILE_BROWSER_MAX_TREE_ENTRIES parsing silently coerces
invalid inputs; add validation in validateConfig() to detect when
process.env.FILE_BROWSER_MAX_TREE_ENTRIES is present but not a positive number
and emit a console.warn (or processLogger.warn) showing the original value and
that a minimum of 1 (or the fallback) will be used; locate the config export
that sets fileBrowserMaxTreeEntries and add the suggested Number(parsing) +
Number.isNaN / < 1 check after that in validateConfig(), logging a clear warning
message including the raw environment string.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bea9b3d2-f9f5-4cf9-80c9-18b41e1b866e
📒 Files selected for processing (6)
docs/plans/2026-05-22-001-fix-file-browser-recursive-pagination-plan.mddocs/residual-review-findings/fix-issue-345-file-browser-recursive-cursor.mdserver/lib/config.tsserver/routes/file-browser.test.tsserver/routes/file-browser.tssrc/features/file-browser/types.ts
…rhashimoto#345) CodeRabbit flagged three coupled issues in listDirectoryPage during review of daggerhashimoto#339, surfaced as issue daggerhashimoto#345: 1. nextCursor advances past items the shared budget never materialized. 2. Shared budget mutation across siblings + recursion strands later siblings on the same page so a follow-up paginated request can't reach them. 3. Child directory listings reuse the parent limit with no per-child nextCursor surfaced, so nested overflow is invisible to the caller. Refactor listDirectoryPage to: - Compute nextCursor at the return statement from items actually consumed (dirsConsumed + filesConsumed), not from the slice length. - Gate file-listing on full dir-loop completion at this level. If a sibling subtree exhausts the budget, files at this level are skipped so the cursor still points at the first unconsumed sibling. - Surface child truncation on the directory TreeEntry via two new optional fields, childrenTruncated and childrenNextCursor. Omitted from the JSON when not meaningful. Frontend TreeEntry kept in sync. - Add an explicit childLimit option (defaulting to options.limit) as the documented hook for future per-child paging tuning. - Filter non-file/non-directory Dirents (symlinks, sockets, FIFOs) at shouldIncludeDirent so totalEntries and the consumed count stay in agreement under the new cursor math. The shared MAX_TREE_RESPONSE_ENTRIES cap remains in force; this change makes its interaction with pagination correct. Added an env override (FILE_BROWSER_MAX_TREE_ENTRIES) so the regression tests can trigger budget exhaustion against tiny fixtures. Regression tests cover the three scenarios called out in the issue (flat-dir-at-budget, deep-tree-across-budget, page-boundary-at-dir/file) plus child-truncation surfacing, schema cleanliness when no truncation occurs, depth=1 no-recursion semantics, and the rapid-supersede leak this PR was originally cut to address.
9736fc2 to
02185e2
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02185e2ce6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…hashimoto#357) Codex round 1 flagged two related issues in the env-var parse: - A non-numeric value like FILE_BROWSER_MAX_TREE_ENTRIES=abc was being coerced via `Number(...) || 0` then clamped with `Math.max(1, ...)` to 1, silently dropping every tree response to a single entry instead of falling back to the documented 5,000 default. - A fractional value like 2.5 was accepted as-is, letting filesConsumed become fractional and producing fractional nextCursor values that parseTreeCursor then floors, drifting paginated windows. Replace the inline ternary with a small parsePositiveIntEnv helper that requires a finite positive integer and falls back to the default otherwise. Added regression tests covering unset / non-numeric / fractional / zero / negative / valid-override.
|
Round 1 - pushed P1 fixed (3):
Regression tests added in Validation:
I argue with the rabbits so you can ship @coderabbitai full review |
|
The 🐇 ✅ ✅ Actions performedFull review triggered. |
|
Bot cycle complete. No remaining P0/P1 findings after 1 round on this branch. Ready for human review. speaks bot, ships code |
2 participants
Closes #345.
Summary
CodeRabbit's review of #339 flagged three coupled issues in
listDirectoryPagethat, in combination, return paginated tree responses where some entries are silently dropped and unreachable via any subsequentcursor. The fix lands all three, plus regression tests against tiny budget fixtures so the failure modes are exercised directly.The three issues, paraphrased from the CR review on #339:
nextCursorwas computed fromstart + pageItems.lengthbefore the recursive descent and file-listing loop. If the shared budget exhausted during recursion, dropped items had no way back.budgetwas mutated across siblings and the parent/child boundary. A deep subtree could burn the budget under siblingdirN, after whichdirN+1, ...,dirMwere silently skipped butnextCursorhad already advanced past them.limitand only theentriesof the recursive call were retained. Per-childtruncatedandnextCursorwere discarded, so nested overflow was invisible to the caller.What changed
nextCursoris computed at the return statement fromstart + dirsConsumed + filesConsumed.dirsConsumedis the count of dirs whose iteration completed (after the recursive call returned);filesConsumedisallowedFileCountwhen the dir loop fully drained, else0.dirsConsumed === directories.length. If any sibling subtree exhausted the shared budget, files at this level are skipped and the cursor still points at the first unconsumed sibling.TreeEntrygains two optional fields on directory entries:childrenTruncated?: booleanandchildrenNextCursor?: string. Both omitted from the JSON when not meaningful (verified vianot.toHaveProperty). Forward-compatible with existing consumers; frontendTreeEntryupdated to match.listDirectoryPageaccepts an optionalchildLimitin its options bag (defaults tooptions.limit). No behavior change today; documented hook for future per-child paging tuning.shouldIncludeDirentnow filters non-file/non-directory Dirents (symlinks, sockets, FIFOs). Without this, the newdirsConsumed + filesConsumedcount could undercountpageItems, pointnextCursorat already-emitted items, and emit duplicates on the next page. The old code silently dropped these via the dirs/files split anyway; filtering at the gate keepstotalEntriesand the consumed count consistent.MAX_TREE_RESPONSE_ENTRIESis now overridable viaconfig.fileBrowserMaxTreeEntries/FILE_BROWSER_MAX_TREE_ENTRIES. Default still 5,000. Lets the regression tests trigger budget exhaustion against tiny fixtures.Test plan
npx vitest run server/routes/file-browser.test.ts- 65/65 pass, including 8 new tests targeting the three bugs from the issue (deep-tree-across-budget, leaf-level budget, page-boundary, empty-dir, child-truncation surfacing, child schema cleanliness when not truncated, depth=1 no-recursion, plus the original supersede regression).npx vitest run- full suite 2113/2113 pass.npx eslint server/routes/file-browser.ts server/routes/file-browser.test.ts server/lib/config.ts src/features/file-browser/types.ts- clean.npx tsc -p config/tsconfig.server.json --noEmit- clean.Follow-ups noted but not landed here
These were surfaced during review but deliberately not auto-applied; tracked here for visibility, suitable for a follow-up PR or issue.
fs.readdirthrowing on a subdirectory during recursive descent. Implementation handles it (the catch block returns an emptyDirectoryListinganddirsConsumedstill increments) but there's no test exercising it.cursorpasttotalEntriesto exercise theMath.minclamping branch.childrenNextCursoris defined but not value-correct, and does not assert non-overlap between the first child page and the resumed page.PageResponsetype that mirrors the route'sDirectoryListing. If the route shape changes, tests stay green while silently asserting the wrong shape. Cleanup: exportTreeEntryandDirectoryListingfrom the route and import them in the test.truncated: true, the parent renderschildren: []pluschildrenTruncated: true. Clients that ignorechildrenTruncatedcannot distinguish this from a genuinely empty directory. Settingchildren: nullin this corner case would make the API self-disambiguate.nextCursorsemantics changed (bug fix, but technically observable). Clients caching cursor values across a server restart may land on a different page boundary on the next request. Worth a changelog line.Notes
next, notmaster. The code being fixed (listDirectoryPage) ships in PR perf: bound file browser tree rendering #339, which is onnextbut not yet onmaster.next-targeted PRs. Triggered@coderabbitai full reviewmanually after open.