fix(drive): page lists on the server's has_next signal - #665
Open
ebrahimgamdiwala wants to merge 5 commits into
Open
fix(drive): page lists on the server's has_next signal#665ebrahimgamdiwala wants to merge 5 commits into
has_next signal#665ebrahimgamdiwala wants to merge 5 commits into
Conversation
`get_query_data` dedupes and permission-filters *after* applying
LIMIT/OFFSET, so a full SQL window can come back with fewer rows than
`limit` while rows still remain. The client inferred end-of-list from the
row count, so a single filtered row permanently ended infinite scroll and
left the rest of the folder unreachable — there is no "load more" button.
Worst in Shared, where the dedupe fires on every multi-path share.
The five paginated endpoints now take an opt-in `paginated` flag; with it
`get_query_data` returns `{rows, has_next}`, where `has_next` is measured
from the raw row count *before* the dedupe and the filter run. Callers
that don't page keep the bare list, so folderTree, MoveDialog, Writer's
download helper and `get_attachments` are untouched.
An explicit parameter rather than a response header or a
`frappe.local.response` sibling key because frappe-ui's `frappeRequest`
returns `data.message` and discards both, so no out-of-band channel
reaches the first page, which goes through the resource layer.
Two further fixes the change turned out to need:
- `setCache` replaces the resource's `setData` with one that assigns
`data` raw, and frappe-ui's offline restore calls exactly that with the
persisted response. Left alone it would put `{rows, has_next}` straight
into `resource.data`, where consumers spread it as an array — a
TypeError on the second visit to every paginated list. Both stores are
idb-keyval under the same key, so normalise through `unwrapRows`.
- `useInfiniteScroll` resolves its scroll target once at setup, but
`useScrollContainer` is a module-level registry the shell fills in a
tick later, so on a cold mount it bound to null and its `arrivedState`
never updated again: a first visit loaded page 1 and then never
paginated, however far you scrolled. Binding on the ref's transitions
survives both orders. `fillViewport` covers a page too short to
overflow its container, which can never emit a scroll event — likelier
now that the server can thin a page.
`loadMore` also formats pages through the same `formatRows` the resource
transform uses; it previously called `prettyData` directly and skipped
the dotfile filter, so dotfiles were hidden on page 1 and visible from
page 2 on.
Tests: 6 backend covering the envelope, the raw-count signal and a window
thinned by a deny row; 10 frontend covering the has_next signal, the
late-registering container, the short-page top-up and the cache restore.
The three carrying the regressions were verified failing without their
fix, and the pagination path was verified end-to-end in a browser against
a built site.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Confidence Score: 5/5The PR appears safe to merge. The stale-pagination race is guarded by query epochs, and deterministic fixture names resolve the previous test concern; no blocking failure remains. Reviews (3): Last reviewed commit: "docs(drive): drop internal tracker IDs f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ebrahimgamdiwala
marked this pull request as draft
August 17, 2026 10:03
The first cut of this fix computed `has_next` from `len(raw_sql_rows) ==
limit`, measured before the permission filter. The raw query carries no
permission predicate — access is resolved per row afterwards — so the
signal was derived from rows the caller may not be allowed to see.
That turned an empty page into an existence oracle. A user with read on a
folder but denied a file inside it previously got an empty list, which is
indistinguishable from "nothing here". With the raw count it got:
files(entity_name=folder, limit=1, paginated=True)
=> {'rows': [], 'has_next': True}
`rows` is correctly empty, but `has_next` proves something is there. It
leaks existence only — no name, no content — and needs folder-level read
first, but it is still information about explicitly denied files, and it
was introduced by this branch rather than pre-existing.
The paginated path now walks raw windows until it has collected a full
page of rows the caller can actually see, or the query is exhausted, and
reports `has_next` from that. An empty page can now only mean the query
ran out. Each window asks for just the shortfall, so a page never
overshoots `limit`. Scanning is capped at MAX_SCAN_WINDOWS: without a
bound, a folder of near-entirely denied rows would scan the whole table
running the per-row access check on each. On hitting the cap we report
has_next=True, the safe direction — one extra request rather than hidden
rows.
Because filling a page can consume several windows, `start + limit` is no
longer where the next page begins, so the envelope carries `next_start`
and the client resumes from it. Dedupe state is threaded across windows
too, or a file reachable by two share paths would survive duplicates that
straddle a boundary.
Also addresses review feedback:
- Stale pagination race (P1). Search, sort or refresh resets pagination
while a `loadMore` may still be in flight; the late response appended
rows from the previous query and overwrote the reset cursor, mixing two
result sets and skipping a page of the current one. Requests now carry
a query epoch and are discarded when it moves on. `fillViewport` made
this materially more likely, since it issues a page request off the
back of every refresh. `refreshData` owns clearing `loadingMore`, as a
stale response deliberately leaves it alone and would otherwise wedge
pagination shut.
- Deterministic pagination test fixtures (P2), so a failing run names the
same rows every time.
Tests: 7 backend, including the oracle case — folder readable, every file
in it denied, page must come back empty *and* has_next False. 11
frontend, including a page landing after the query moved on. Both
verified failing without their fix. Re-checked in a browser: a cold visit
fills a full 50-row first page past the denied rows and reaches all 55
visible files, resuming from the server's cursor (start=55, not 50).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ebrahimgamdiwala
marked this pull request as ready for review
August 17, 2026 10:21
The repo allows a longer line than the assertion had been wrapped to, so `ruff format --check` (the gating lint step) rejected it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F5/N1 referenced a personal review-tracker doc, meaningless outside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Drive's list endpoints dedupe and permission-filter after applying
LIMIT/OFFSET, so a full SQL window can return fewer visible rows once filtered. The client inferred "end of list" frompage.length >= PAGE_SIZE, so one filtered row permanently killed infinite scroll — and with no "load more" button, the rest of the folder became unreachable. Worst in Shared, where the dedupe fires on every multi-path share.Fix
The five paginated endpoints take an opt-in
paginatedflag and return{rows, has_next, next_start}. The paginated path walks raw SQL windows until it has a full page of rows the caller can actually see, and deriveshas_nextfrom that — never from the raw count, which would make an empty page advertise the existence of denied files. Filling a page can span several windows, so the response carriesnext_startfor the client to resume from. Non-paging callers keep the bare list, untouched.Three further fixes this turned up:
resource.data, where consumers spread it as an array. The new envelope threw aTypeErroron the second visit to any list.useInfiniteScrollbinds its target once at setup, but the shell registers the scroll container a tick later, so it caughtnulland never re-armed.loadMorewas in flight appended rows from the old query and overwrote the reset cursor. Requests now carry a query epoch.Also:
loadMoreskipped the dotfile filter page 1 applies, so dotfiles appeared from page 2 onward.Testing
7 backend, 11 frontend — each regression case verified failing without its fix. Verified in a browser on a built site: a cold visit fills a full 50-row page past denied rows and reaches all 55 visible files. All Drive backend modules and the full frontend suite pass.