Repository navigation
fix(resources): complete offline manifests beyond 100 search hits - #437
Conversation
Stop re-slicing bounded Aquifer search results at 100 and report truncated only when provider pagination metadata cannot prove enumeration finished.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #431 requires a complete manifest with accurate resource sizes. The PR implements bounded enumeration, removes the per-configuration 100-item slice, propagates complete, and adds tests for 101… |
Implement or integrate the API size contract required by #431/#273 for manifest assets. Add focused tests that verify accurate sizes for the affected media resources. Keep truncated and failure behavior fail-closed when size data is not a… |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: allowing offline manifests to include more than 100 Aquifer search hits. Out of Scope Changes check The changes support issue #431. Aquifer pagination metadata, shared result-type updates, range-caller migrations, warnings, manifest tests, and lint-suppression updates are required by or support the …Docstring Coverage Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. (1 skipped: 1 … Full details: Out of Scope Changes check
-
Explanation
The changes support issue
#431. Aquifer pagination metadata, shared result-type updates, range-caller migrations, warnings, manifest tests, and lint-suppression updates are required by or support the bounded enumeration change. No unrelated product behavior is demonstrated.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. (1 skipped: 1 unsupported.)
-
Full details: Linked Issues check
-
Explanation
Issue
#431requires a complete manifest with accurate resource sizes. The PR implements bounded enumeration, removes the per-configuration 100-item slice, propagatescomplete, and adds tests for 101 resources and the 20-page ceiling. However,buildManifestItemstill usesasset?.size ?? 0for media items, and the PR states that missing asset sizes remain unresolved under#273. This leaves a direct coding requirement unmet. The Android QA requirement is manual verification and is not used for this coding verdict.Resolution
Implement or integrate the API size contract required by
#431/#273 for manifest assets. Add focused tests that verify accurate sizes for the affected media resources. Keeptruncatedand failure behavior fail-closed when size data is not available.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Reviewed against origin/main @ c8d4cb6 (single commit 931f3e6). Verified locally: the
focused suites pass 61/61, tsc --noEmit, prettier --check, eslint on touched paths are
clean, and knip flags neither new export. Core fix is right — the 100-slice is gone,
complete→truncated wiring is correct, and the ceiling semantics are codified in tests.
A few things below, ordered by weight.
Should address
-
truncatedOpenAPI description overstates hydration —translation-resources.types.ts:315-317.
"False when every matching resource was collected and hydrated" isn't true today:
mapWithConcurrencyskips failedgetResourcecalls, so 1-of-N hydrate failure yields
truncated:falsewith a required item silently missing — the same "partial looks
complete" failure class #431 exists to kill. Deferring strict-fail to #433 is a fine
scope split, but the doc shouldn't claim a guarantee the code doesn't provide. Suggest
softening to "collected" (or noting hydrate failures may still drop items) until #433
lands. -
Range-route callers discard
complete—translation-resources.service.ts:271-282
(searchVerseRangeCollection) and:326-337(getTranslationImagesForRange). Both do
search.data.itemsand dropcomplete, so a >2000-hit verse range still returns a
silently partialitemsarray — no flag, no warn, and unchanged OpenAPI descriptions.
Pre-existing behavior, but the diff introduces the signal specifically so partial sets
can't pass as exhaustive, then drops it in two of three callers. Minimum: a
logger.warnwith collection/range context (also satisfies FA-R8); alternatively a
comment documenting that the exhaustion guard is intentionally manifest-only. -
No server-side signal on
!complete—service.ts:463-465.truncated:truetells
the client, but nothing logs which config hit the 20-page ceiling. If this ever fires in
prod we'll wantlogger.warn({ resourceCollectionCode, bookCode, startChapter, endChapter })here — debugging "some config truncated" with no record will be rough.
Judgement calls / nits
aquifer.client.ts:35—const MAX_SEARCH_PAGES = AQUIFER_MAX_SEARCH_PAGES;is a
leftover alias used once at:253, and the docstring at:242still names
MAX_SEARCH_PAGES. Use the export directly.aquifer.client.ts:260-261—page.data.items.length === 0 ||is subsumed by
< limit.- Provider-contradiction edge —
searchAllResourcesbreakscomplete=trueon a
short/empty page even whenallItems.length < totalItemCount(codified by
aquifer.client.test.ts:294-316: total 500, 1 item → complete). Trusting the page signal
is defensible pagination semantics; worth a comment acknowledging that count-vs-page
disagreement resolves to "complete". - Wasted hydration when
!complete— the manifest hydrates the entire partial set
(up to 2000 items ×getResource) even though the flag means the primary client rejects
the response. Consider skipping hydration when!search.data.completeif no other
consumer needs the partial items. - Serial config loop —
service.ts:447: five independent config searches awaited
serially (~100 sequential upstream calls worst case). Pre-existing; if it's intentional
(Aquifer rate limits) it deserves a comment per FA-R10. - Minor (pre-existing): the manifest path doesn't dedupe hits — the range path does via
dedupeSearchHitsById. Duplicated provider items → duplicateaquiferContentIdentries.
Spec check (#431)
The bounded-enumeration approach satisfies the issue's core asks: no re-slicing, exhaustion
can't pass as complete on the manifest route, and there's focused >100 + ceiling coverage.
Deferred ACs (accurate sizes → #756/#273, strict hydrate → #433, Android QA → post-deploy)
are reasonable splits — just keep #431 open until the mobile verification lands.
|
@kaseywright Thanks for the review — addressed on Conflicts with
|
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Re-reviewed at d14adcd (merge-base now 4bccdc1). Nice round — everything from my last
pass is addressed: truncated doc is honest about hydrate failures, all three
searchAllResources consumers warn on !complete with useful context, the alias/redundant
condition/provider-disagreement items are all resolved, and pulling in #433's 'fail'
policy makes manifest hydration fail-closed (502 on any getResource failure) — which
genuinely closes the "partial looks complete" hole rather than just documenting it.
Re-verified locally: 66/66 focused tests pass, tsc/eslint/prettier clean.
Two things left:
-
New-file FA-L7 suppression.
translation-resources.manifest-completeness.test.ts:25
doesvi.mock('@/db')andeslint-suppressions.jsongained a fresh entry for the new
file. The standards doc says the baselines "only ever shrink" and aren't regenerated to
absorb a new violation. I get the bind — the sibling route-test files carry the same
grandfathered mock (the auth chain's activeOrg read needs it) and FA-L11 forced the
split — but a new file starting life with a suppression is exactly what that rule is
against. Options: mock the seam differently (e.g. the middleware/service layer rather
than@/db), or declare it as a Standards exceptions note in the PR body per the
pre-PR checklist. Net count across files is flat (test.ts 5→4) so this is closer to
bookkeeping than a real regression — but worth a deliberate answer. -
PR body is stale. Still cites base
c8d4cb6(branch now merges4bccdc1), claims
"61 passed" (now 66), and frames #433 as an unmerged dependency ("strict-fail remains
#433") when it has landed and this PR builds on its'fail'mode. A refresh would make
the merge/rebase story clearer for the next reviewer.
Nit, for completeness: the images-range warn (service.ts:354-368) has no assertion —
only the notes-range twin is covered. Symmetric code, low risk.
Mirror the notes-range coverage for the images verse-range page-ceiling warn.
mattrace-gloo
left a comment
mattrace-gloo
left a comment
There was a problem hiding this comment.
Thanks for the re-review at d14adcd — addressed the two remaining items:
-
FA-L7 / new-file suppression — declared under Standards exceptions in the PR body. Same auth-chain
@/dbmock as the sibling route tests; FA-L11 forced the split; netno-restricted-syntaxcount is flat (test.ts5→4). Follow-up noted to mock the middleware/service seam and drop the suppression later. -
PR body refresh — merge-base
4bccdc1, focused suite 67 (was 66; +1 images-range warn), and #433 framed as landed/'fail'mode this PR builds on (not an unmerged dependency).
Nit: added the images-range warn assertion (mirrors notes-range) in 10d26f0.
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
All items resolved — approving.
Verified at 10d26f0: the images-range warn assertion covers the last gap, the FA-L7
new-file @/db mock is declared under Standards exceptions with a seam-mock follow-up,
and the PR body now reflects the real base (4bccdc1), the 67-test count, and #433 as a
landed dependency. Re-ran the focused suites: 67/67 pass.
Spec side (#431): every API-implementable acceptance criterion is met — no re-slicing,
exhaustion can't pass as complete on any consumer (flag on manifest, warns on range
routes), focused >100 + ceiling coverage, and fail-closed hydration via #433's 'fail'
mode. Remaining items are tracked follow-ups (sizes → #756/#273, Android QA → mobile #736),
so #431 stays open until the deployed-revision verification lands.
TLDR
Prepare Offline manifests no longer re-slice a successfully enumerated Aquifer search back to 100 items per configuration. When bounded pagination collects every matching resource (including counts above 100), the response stays
truncated: falsewith full hydration and provider-reported metadata. When the 20-page search ceiling stops collection without provider proof of completeness,truncatedstays true so clients cannot treat a partial catalogue as complete. This enumeration fix does not make missing asset sizes known: the observed underestimate is tracked separately in fluent-mobile#756, with the API size contract under #273.Details
Refs #431
Related mobile acceptance: fluent-mobile#736 (do not close until Android QA on a deployed revision)
origin/main@4bccdc1getPrepareOfflineManifestsettruncatedandslice(0, 100)aftersearchAllResourceshad already paginated (100/page, ≤20 pages).{ items, complete }from bounded Aquifer enumeration; hydrate all collected items; settruncatedonly whencomplete === false.'fail'policy so anygetResourcefailure fails closed (502) rather than silently dropping items.translation-resources(compared file list). #411 only adds Bible-text 404httpStatuson Aquifer errors — no intentional interaction.Type of change
Technical changes
src/lib/services/aquifer/aquifer.client.ts—searchAllResourcesreturns{ items, complete }; exportAQUIFER_MAX_SEARCH_PAGESsrc/lib/services/aquifer/aquifer.types.ts—AquiferResourceSearchEnumerationsrc/domains/translation-resources/translation-resources.service.ts— remove per-config 100 slice; hydrate full enumeration;truncatedfrom!complete; warn on incomplete range/image searchessrc/domains/translation-resources/translation-resources.types.ts— OpenAPItruncateddescription updatedStandards exceptions
vi.mock('@/db')in unit tests):translation-resources.manifest-completeness.test.tscarries a new-file suppression for the same auth-chain activeOrg@/dbmock already grandfathered in sibling route-test files. FA-L11 forced the split out oftranslation-resources.test.ts; netno-restricted-syntaxcount across those files is flat (test.ts5→4, new file +1). Declared here rather than regenerating the baseline to absorb a new violation. Follow-up: mock the middleware/service seam so the new file can drop the suppression.Testing
--configLoader=runner): 67 passed across translation-resources + aquifer.client suites (includes >100 complete / page-ceiling incomplete cases, plus images-rangewarnassertion)prettier --checkon touched files: passeslinton touched domain/aquifer paths: passtsc --noEmit: passnpm run deadcode/ knip: blocked locally —knip@5.88.1not installed in sharednode_modules; stock script invokes barenpx(not run; no install)test:db/test:live: skipped (no isolated DB / no live Aquifer credentials)scripts/check-docs-structure.sh: fails on pre-existingdocs/.DS_Store(unrelated; not modified here)How to verify
./node_modules/.bin/vitest run --configLoader=runner src/domains/translation-resources src/lib/services/aquifer/aquifer.client.test.ts— expect pass including#431>100 complete and page-ceiling incomplete cases.truncated_manifest(e.g. project 12) — expect a non-truncated catalogue with all enumerated metadata; record runtime revision against mobile #736 before closing either issue. Size completeness and unknown-byte presentation remain separate verification under fluent-mobile#756 / Expose Aquifer-backed Translation Resources API for mobile (TN / TQ / Images & Maps) #273.Follow-ups
npxacquisition.manifest-completeness.test.tsby mocking the auth seam instead of@/db.Delivery note: source-identical replacement for #436 solely to remove an automatically added coauthor trailer. The tested source tree is unchanged; no force-push, merge or deployment.
Summary by CodeRabbit