fix: order and limit the DAV search so "Recent" is actually recent - #105
Merged
Conversation
buildOfficeMimeSearch() sent neither <d:orderby> nor <d:limit>. Without
an explicit limit the server applies its own default of 100 rows, and
without an ordering the query runs with no ORDER BY at all — so the
client received an arbitrary 100-row slice in database order and then
sorted that slice by mtime. For anyone with more than ~100 office
files, "Recent" could omit a file edited minutes ago, silently: the
list still looked plausible, just not the newest files. It also made
MAX_DISPLAY_FILES (200) and the "Show all in Files" affordance
unreachable, since the result set could never exceed 100.
Both knobs are already supported by the DAV search grammar, so this is
a request-side fix: order by {DAV:}getlastmodified descending, and
request SEARCH_RESULT_LIMIT (500) rows explicitly, well above
MAX_DISPLAY_FILES since one search feeds all four categories.
getAllOfficeFiles() now returns { nodes, truncated }, and "Show all in
Files" also appears when the search itself was capped, so the page
doesn't imply it's showing everything when it isn't. Existing specs
for officeFiles.ts and OfficeOverview.vue are updated to the new
return shape, and both files gain regression tests: the SEARCH body
carries orderby/limit and reports truncation correctly, and the "Show
all" button appears on a truncated result even when the fetched set is
well under MAX_DISPLAY_FILES.
Co-authored-by: Frank Karlitschek <karlitschek@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
truncated was computed as results.length >= SEARCH_RESULT_LIMIT, so a user with exactly 500 matching files (no more) and one with 500+ files capped at the limit both came back as a 500-row response — the flag couldn't tell them apart, and the first case saw an unnecessary "Show all in Files" button. Request one extra row past the limit instead: a 501st row coming back is unambiguous proof more files exist, while capping at exactly 500 with nothing beyond it now returns 500 or fewer. Trim the extra row before returning nodes so it's never rendered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
- src/App.spec.ts:16 mocked getAllOfficeFiles to a bare [], the pre-this-PR shape — harmless today only because getTemplates() also resolves to [] here, so the mock's return value is never consumed, but it's the exact gap AGENTS.md's spec-update rule exists to catch before it bites a future test. - OfficeFilesResult.truncated's JSDoc still described "returned a full page ... were not fetched", true before the off-by-one fix but not after: the disambiguating row now is fetched, then trimmed. - OfficeOverview.spec.ts referenced officeFiles.ts's old cachedNodes variable name, renamed to cachedResult in the first commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
hasMoreFiles ORs in the global truncated flag, which describes the
shared search across all four categories, not this category
specifically — so "No {category} found" and "Show all in Files" can
render at once. Before this PR that combination was impossible
(hasMoreFiles was purely a count on the already-empty filtered list).
Not a bug: a sparse category is exactly the one most likely to have
real matches sitting past a global cutoff, so surfacing the escape
hatch here is the conservative-correct call. Pin it as a test so it
reads as an intentional case, not an untested gap.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Contributor
Author
|
/compile |
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.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
3 participants
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.
Summary
Replaces #100, rebased onto current
mainand with tests fixed and added.buildOfficeMimeSearch()sent neither<d:orderby>nor<d:limit>, so the server returned an arbitrary 100-row slice in database order that was then sorted by mtime client-side. For anyone with more than ~100 office files, "Recent" could silently omit a file edited minutes ago. It also madeMAX_DISPLAY_FILES(200) and the "Show all in Files" affordance unreachable, since the result set could never exceed 100.{DAV:}getlastmodifieddescending and requestsSEARCH_RESULT_LIMIT(500) rows explicitly — both already supported by the DAV search grammar, so this is a request-side fix only.getAllOfficeFiles()now returns{ nodes, truncated }; "Show all in Files" also appears when the server search itself was capped, not just when the client-side render cap is hit.officeFiles.spec.tsandOfficeOverview.spec.tsto the new return shape (every mock/assertion built on the old bare-array shape), and adds regression tests: the SEARCH body carriesorderby/limitand reports truncation correctly, and "Show all in Files" appears on a truncated result even when the fetched set is well underMAX_DISPLAY_FILES.Diagnosis and the original fix are Frank Karlitschek's (#100), credited via
Co-authored-by:on the commit. This PR carries that fix forward with the target files unchanged (no drift since #100 branched), plus the test and process fixes needed to land it.Test plan
npm run test:unit— 79/79 passing (Node 24 via.nvmrc; Node 25 has an unrelated knownlocalStorage/jsdom issue, see README)npm run lint,npm run stylelint— cleannpm run build— succeeds;js/cssintentionally not committed, needs/compileofficeFiles.spec.ts), "Show all in Files" on truncated-but-small result (OfficeOverview.spec.ts)Still needs
/compileforjs/css.