Repository navigation
fix(resources): pass Images caption, attribution, and licenseInfo - #435
Conversation
Images & Maps items omitted optional Aquifer metadata that mobile #594 already maps. Pass through content caption/attribution when present and grouping.licenseInfo like TN/TQ; never fabricate missing fields.
|
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 22 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 |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
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: propagating image caption, attribution, and licenseInfo metadata. Linked Issues check For the scoped Images & Maps work in [ #273], the change adds optionalcaption,attribution, and typedlicenseInfofields toTranslationImageItem. The image builder readscaptionand `attribu…Out of Scope Changes check The diff changes only the image metadata schema, the image asset mapping, related comments, and focused tests. The tests verify the requested [ #273] behavior. The diff does not add manifest behavior, …Full details: Linked Issues check
-
Explanation
For the scoped Images & Maps work in [
#273], the change adds optionalcaption,attribution, and typedlicenseInfofields toTranslationImageItem. The image builder readscaptionandattributionfrom the asset record and readslicenseInfofromdetails.grouping. It omits absent fields and preserves URL, thumbnail, and size behavior. OpenAPI schemas and focused tests cover populated and absent metadata. The remaining#273work is broader than this follow-up and is not required for this scoped assessment.
Full details: Out of Scope Changes check
-
Explanation
The diff changes only the image metadata schema, the image asset mapping, related comments, and focused tests. The tests verify the requested [
#273] behavior. The diff does not add manifest behavior, passage semantics, service modularization, concurrency policy changes, or mobile implementation.
-
✨ Finishing Touches: 💡 1
-
📝 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.
Clean, well-scoped slice. licenseInfo on image items is justified — same grouping.licenseInfo wire TN/TQ/manifest already carry, and mobile's mapper (imagesMaps.ts) prefers explicit attribution then falls back to formatImageLicenseAttribution(licenseInfo), so this feeds both paths. Fixtures are schema-validated, ok/absent paths covered, conditional-spread idiom matches the file. Two nits inline plus one noted risk.
Non-blocking risk (already half-acknowledged in the PR body): caption/attribution are read own-record only — from whichever record wins extractAquiferAsset's first-url DFS (readAssetFromRecord, service.ts:124–156). If production Aquifer puts caption on a parent or sibling record rather than beside url, it's dropped silently and this ships vacuous. Since no live call verified the shape, suggest recording a real image payload (e.g. content id 2788) as a fixture, or a post-deploy check, before mobile #594's API AC is checked off.
Verified independently: lint/typecheck clean, focused license tests 11/11 pass, OpenAPI picks up the fields via the route's response schema, and the "mobile already maps these fields" claim holds — ApiTranslationImageItem already declares all three.
Review drafted with Devin; verified against a local worktree of the PR head.
Align thumbnailUrl OpenAPI with emit behavior, restore the mobile prepareOfflineResourceManifest pointer, and reuse imageSearchHit in license tests. Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(resources): complete offline manifests beyond 100 search hits Stop re-slicing bounded Aquifer search results at 100 and report truncated only when provider pagination metadata cannot prove enumeration finished. * Merge origin/main into mrace/fix/431-complete-resource-manifest-clean Resolve conflicts with #432 fail-closed manifest hydrate and #435 image metadata. Address #437 review: soften truncated OpenAPI copy, warn on incomplete Aquifer searches (manifest + range), and drop the local MAX_SEARCH_PAGES alias / redundant empty-page check. * test(resources): assert images-range warn on incomplete Aquifer search Mirror the notes-range coverage for the images verse-range page-ceiling warn.
TLDR
Narrow follow-up to #273 / historical #274: Images & Maps items now optionally carry
caption,attribution, andlicenseInfowhen Aquifer provides them (samegrouping.licenseInfopath TN/TQ already use).thumbnailUrl/ fullurlbehavior unchanged. Mobile #594 already maps these fields. Does not close broad #273 (other criteria remain).Details
Refs #273
Related: fluent-mobile#594
origin/main@c8d4cb6(QA-aligned tip at branch creation).{ id, title, localizedName, url, thumbnailUrl?, size? }and omitted caption/attribution/licenseInfo.TranslationImageItem+getTranslationImagesForRangenow pass through optional contentcaption/attributionstrings and typedgrouping.licenseInfo; omit when absent; never fabricate.contentis untyped JSON in-repo. Pass-through keyscaption/attributionmatch the mobile optional contract and mock fixtures only — whether production Aquifer populates those keys (e.g. content id 2788) is not verified here (no live API calls). Attribution vialicenseInfois the typed/documented path (mobile derives credit from it).translation-resources.service.tsas fix(resources): reject incomplete offline hydration (#432) #433 only at the images item builder; no change tomapWithConcurrencyfailure policy.Overlap with open PRs
Technical changes
src/domains/translation-resources/translation-resources.types.ts— optionalcaption,attribution,licenseInfoonTranslationImageItemsrc/domains/translation-resources/translation-resources.service.ts— asset own-record pass-through +grouping.licenseInfoon image itemssrc/domains/translation-resources/translation-resources.license.test.ts— mock-labeled populated + absent metadata tests (not live payload proof)Testing
translation-resources+ aquifer client → 45 passed (domain) / related aquifer suite 60 passed earliernpm run lint,format:check,typecheck, fullnpm test→ 1358 passed, 9 skippedknip@5.88.1FA-C1 check → 0 added / 0 stale (stocknpm run deadcodevia barenpx knippulled knip 6 and broke report shape in this worktree; pinned 5.88.1 matches package.json)./scripts/check-docs-structure.sh+npm run build→ passtest:db/test:live(no isolated DB / no Aquifer credentials per task)How to verify
caption,attribution,licenseInfo,thumbnailUrl.npm test -- --run src/domains/translation-resources— expect pass including license/images metadata cases.licenseInfowhen Aquifer grouping has it; confirmcaption/attributiononly if Aquifer content includes those keys — do not assume content id 2788.Follow-ups
contentactually emitscaption/attributionstrings; if onlylicenseInfois populated, mobile attribution still works viaformatImageLicenseAttribution.Made with Cursor
Summary by CodeRabbit