feat: add paginated fs listing#662
Conversation
|
Warning Review limit reached
Next review available in: 25 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 Walkthrough
✨ 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 |
qiffang
left a comment
There was a problem hiding this comment.
Dev Lead Review — LGTM
Diff reviewed (8 files, +474/-14). Clean keyset pagination implementation.
Architecture: Correct layering Store.ListDirPage → Backend.ReadDirPageCtx → Server.handleList. Original paths untouched for backward compat. limit+1 fetch-then-truncate for has-more detection.
Cursor security: HMAC-SHA256 with tokenSecret, subtle.ConstantTimeCompare, directory binding, version field, DisallowUnknownFields + trailing-data check, validFSListCursorName rejects dangerous names. \x00 field separator prevents boundary confusion.
Test coverage: Datastore keyset (out-of-order inserts, 3 pages + empty + error), handler e2e (compat + pagination + 7 error cases + cross-dir), client SDK mock, scoped token allowlist.
Both adversaries GREEN. Ready for @qiffang merge.
adversary-1 Review — REVISEBlocking issue:
Other reviewed areas look aligned with the agreed pagination contract: old Validation run locally:
Requesting changes until the cookbook coverage is fixed and CI is green. Note: GitHub would not let me submit a formal REQUEST_CHANGES review from this machine because the authenticated account is treated as the PR author, so this is a comment-level REVISE. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a0f84a973
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| q += ` AND fn.name > ?` | ||
| args = append(args, afterName) | ||
| } | ||
| q += ` ORDER BY fn.name` |
There was a problem hiding this comment.
Use a unique cursor key for directory pagination
When a page boundary falls inside entries that share the same listing key, the next page skips the remaining entries because the cursor only stores AfterName and the follow-up query filters with fn.name > ?. This can happen for valid rows such as a file /data/foo and a directory /data/foo/ (distinct paths/path_hashes but the same name), or collation-equivalent names on MySQL; with limit=1, the over-fetched sibling is sliced off, the cursor is foo, and the next query excludes it. Include a unique tie-breaker in both the ORDER BY and cursor predicate, or enforce basename uniqueness before relying on name-only keyset pagination.
Useful? React with 👍 / 👎.
0a0f84a to
ba9e90a
Compare
|
Review verdict: REVISE on The previous cookbook blocker is fixed: Remaining blocker:
Local validation performed:
CI was still pending when checked, but even green CI would not cover the missing cases above. |
ba9e90a to
5852582
Compare
|
Review verdict: REVISE on The missing empty-directory and single-entry coverage is added, and the core pagination code is unchanged. The remaining blocker is in the newly added special-character tests:
Validation performed locally:
GitHub |
5852582 to
d7e5387
Compare
|
Review verdict: GREEN on The previous blockers are resolved:
Local validation performed:
GitHub checks at review time: CodeRabbit passed; |
Summary
limit/cursorpagination forGET /v1/fs/<path>?list=1while preserving the legacy full-list response whenlimitis absent.fn.name, directory-bound opaque cursors, andlimitvalidation in1..10000.list+limit+cursorand add SDKListPage/ListPageCtx.Tests
/usr/local/go/bin/go test -c ./pkg/client ./pkg/server ./pkg/datastore ./pkg/backend/usr/local/go/bin/go vet ./pkg/client ./pkg/server ./pkg/datastore ./pkg/backendgit diff --checkRuntime package tests on this machine are blocked by local testcontainers startup:
rootless Docker not found.