feat(api): use redirect for kobo sync book images#198
Merged
balazs-szucs merged 3 commits intoMar 27, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (1)booklore-api/src/**/*.java📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2026-03-26T02:20:22.468ZApplied to files:
📚 Learning: 2026-03-25T19:09:09.638ZApplied to files:
📚 Learning: 2026-03-25T21:02:55.427ZApplied to files:
📚 Learning: 2026-03-26T01:46:48.847ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThumbnail endpoints were consolidated into a single handler; the controller now returns 307 Temporary Redirect with a Location built by the KoboServerProxy instead of proxying image bytes; the proxy was simplified to construct CDN URIs and tests were updated accordingly. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as KoboController
participant Proxy as KoboServerProxy
participant CDN as Kobo CDN
rect rgba(100, 200, 100, 0.5)
Note over Client,CDN: Old Flow: Controller proxied CDN image
Client->>Controller: GET /.../thumbnail/...
Controller->>Proxy: proxyExternalUrl(cdnUrl)
Proxy->>CDN: HTTP GET image
CDN-->>Proxy: Image bytes
Proxy-->>Controller: Resource (image bytes)
Controller-->>Client: 200 OK + image bytes
end
rect rgba(100, 150, 200, 0.5)
Note over Client,CDN: New Flow: Controller issues redirect
Client->>Controller: GET /.../thumbnail/...
Controller->>Proxy: getKoboCDNCoverUri(imageId,width,height,isGreyscale)
Proxy-->>Controller: Constructed CDN URI
Controller-->>Client: 307 Temporary Redirect (Location: CDN URI)
Client->>CDN: Follow redirect, GET CDN URI
CDN-->>Client: 200 OK + image bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
8671fbc to
bb76e98
Compare
Contributor
Author
|
This matches us more closely with Komga's implementation, too. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java (1)
246-257: Add a focused controller test for the redirect branch.These assertions lock down URI construction, but the risky part of this PR is
KoboController.getThumbnail(...)'s multi-pattern mapping plus the307/Locationresponse. A smallMockMvcor standalone controller test for at least one legacy path and one simplified path would catch routing regressions without pulling up the full application context.As per coding guidelines,
booklore-api/src/test/**/*.java: Prefer focused unit tests; use@SpringBootTestonly when the Spring context is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java` around lines 246 - 257, Add a focused controller unit test that verifies the redirect branch of KoboController.getThumbnail using MockMvc (standaloneSetup) instead of a full Spring context: instantiate KoboController with a test/double KoboServerProxy (or mock) and call both a legacy-mapped URL and the simplified path, asserting the response status is 307 and the Location header equals the expected URI produced by KoboServerProxy.getKoboCDNCoverUri; ensure the test covers at least one legacy multi-pattern mapping and one simplified mapping to catch routing/regression issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/controller/KoboController.java`:
- Around line 74-75: The OpenAPI annotations on KoboController (the thumbnail
endpoint method, e.g., getBookThumbnail) only document a 200 JPEG response but
the handler can also return a 307 Temporary Redirect with a Location header for
non-"BL-" images; update the `@Operation` summary/description to mention both
direct JPEG body and redirect flows, and replace the single `@ApiResponse` with
multiple `@ApiResponse` entries that include a 200 response (content type
image/jpeg), a 307 response (description "Temporary Redirect" and header
"Location" documented), and any other relevant responses (e.g., 404/400) so
generated docs reflect both modes; apply the same annotation changes to the
other similar handlers referenced in the comment.
---
Nitpick comments:
In
`@booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java`:
- Around line 246-257: Add a focused controller unit test that verifies the
redirect branch of KoboController.getThumbnail using MockMvc (standaloneSetup)
instead of a full Spring context: instantiate KoboController with a test/double
KoboServerProxy (or mock) and call both a legacy-mapped URL and the simplified
path, asserting the response status is 307 and the Location header equals the
expected URI produced by KoboServerProxy.getKoboCDNCoverUri; ensure the test
covers at least one legacy multi-pattern mapping and one simplified mapping to
catch routing/regression issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a6bb1a5-f656-46e2-bd0e-50e8e5aef87b
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/controller/KoboController.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Keep JPA entities on the *Entity suffix
Files:
booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/controller/KoboController.java
booklore-api/src/test/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Prefer focused unit tests; use
@SpringBootTestonly when the Spring context is required
Files:
booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java
🧠 Learnings (3)
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/test/**/*.java : Prefer focused unit tests; use SpringBootTest only when the Spring context is required
Applied to files:
booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java (1)
37-37: Nice extraction of the CDN URI builder.Centralizing the Kobo CDN template here keeps the controller thin, and the explicit encoding handles slash-containing image ids cleanly.
Also applies to: 73-77
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
booklore-api/src/main/java/org/booklore/controller/KoboController.java (1)
75-79:⚠️ Potential issue | 🟡 MinorFinish the redirect OpenAPI contract.
Line 75 still describes only the local-thumbnail flow, and the 307 response on Line 78 still doesn't document the
Locationheader, so generated clients are missing part of the actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/controller/KoboController.java` around lines 75 - 79, Update the OpenAPI contract on the KoboController thumbnail endpoint: expand the `@Operation`(description=...) to mention that a 307 redirect may be returned, and modify the `@ApiResponse` for responseCode="307" to include a documented Location header (use the io.swagger.v3.oas.annotations.headers.Header with name="Location", description like "URL of the thumbnail resource", and a string schema) so generated clients know to read the Location header; adjust the 200 response description if needed to clarify the local-thumbnail vs redirect behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/controller/KoboController.java`:
- Around line 100-103: The current code unconditionally returns an external
redirect using koboServerProxy.getKoboCDNCoverUri(...), which bypasses the
server-side proxy and causes the privacy/behavior regression; change the logic
in KoboController (the method that builds the ResponseEntity with
.location(...)) to check the planned proxy toggle flag and only return the
external TEMPORARY_REDIRECT when the toggle explicitly enables direct CDN
redirects; otherwise (default/fallback) resolve and return the internal proxied
URI from koboServerProxy (use the existing proxy method on koboServerProxy
instead of getKoboCDNCoverUri) so the server-side proxy remains the default
until the toggle is turned on.
---
Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/controller/KoboController.java`:
- Around line 75-79: Update the OpenAPI contract on the KoboController thumbnail
endpoint: expand the `@Operation`(description=...) to mention that a 307 redirect
may be returned, and modify the `@ApiResponse` for responseCode="307" to include a
documented Location header (use the io.swagger.v3.oas.annotations.headers.Header
with name="Location", description like "URL of the thumbnail resource", and a
string schema) so generated clients know to read the Location header; adjust the
200 response description if needed to clarify the local-thumbnail vs redirect
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd192c0b-48ce-439d-bb52-db79d7bd9735
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/controller/KoboController.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Keep JPA entities on the *Entity suffix
Files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
🧠 Learnings (2)
📚 Learning: 2026-03-25T16:46:06.366Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-25T16:46:06.366Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
🔇 Additional comments (1)
booklore-api/src/main/java/org/booklore/controller/KoboController.java (1)
96-98: GoodBL-guard here.This keeps local IDs out of
KoboServerProxy.getKoboCDNCoverUri(...), which would otherwise turn them into broken CDN redirects.
888c8de to
f7bb6d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.java`:
- Line 37: The CDN URL template in KOBO_BOOK_IMAGE_CDN_URL currently builds a
hybrid path that omits the optional quality segment and produces an unadvertised
greyscale shape; update the constant to match the advertised shapes (include a
{Quality} path segment so valid shapes are ".../{Height}/false/image.jpg" and
".../{Height}/{Quality}/{IsGreyscale}/image.jpg"), then update
KoboController.getThumbnail(...) to accept the optional quality path variable
and forward it to wherever the CDN URL is constructed, and ensure
KoboResourcesComponent usage matches the same template names so quality-aware
requests include the quality segment and the no-quality greyscale route uses the
advertised form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32a89c70-98a2-438f-9780-11312bb9ef4b
📒 Files selected for processing (3)
booklore-api/src/main/java/org/booklore/controller/KoboController.javabooklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- booklore-api/src/test/java/org/booklore/service/kobo/KoboServerProxyTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Suite / Backend Tests
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/controller/KoboController.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
📚 Learning: 2026-03-26T02:20:22.468Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 198
File: booklore-api/src/main/java/org/booklore/controller/KoboController.java:100-103
Timestamp: 2026-03-26T02:20:22.468Z
Learning: In `booklore-api/src/main/java/org/booklore/controller/KoboController.java`, the Kobo reader device already makes direct outbound connections to Kobo's CDN/servers for other requests (confirmed by imnotjames via testing). Therefore, returning a `307 TEMPORARY_REDIRECT` to `cdn.kobo.com` for non-`BL-` book cover thumbnails (instead of server-side proxying) does NOT introduce a new privacy or behavioral regression — the Kobo device's IP and User-Agent are already exposed to Kobo through other direct requests.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/controller/KoboController.java
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
booklore-api/src/main/java/org/booklore/service/kobo/KoboServerProxy.javabooklore-api/src/main/java/org/booklore/controller/KoboController.java
📚 Learning: 2026-03-25T21:02:55.427Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 195
File: booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java:15-19
Timestamp: 2026-03-25T21:02:55.427Z
Learning: In `booklore-api/src/main/java/org/booklore/config/logging/RequestLoggingFilterConfig.java` and `booklore-api/src/main/java/org/booklore/config/logging/filter/RequestLoggingFilter.java`, logging all request headers and payloads at DEBUG level is intentional and accepted behavior in this project. The previous `LoggingFilter` also logged all headers. No header redaction or payload scrubbing is required — the DEBUG log level is considered sufficient access control. The new filter is not profile-gated (unlike the old `Profile("dev")` filter) but relies on `logger.isDebugEnabled()` for gating.
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
📚 Learning: 2026-03-26T01:46:48.847Z
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T01:46:48.847Z
Learning: Applies to booklore-api/src/**/*.java : Use MapStruct for entity/DTO mapping
Applied to files:
booklore-api/src/main/java/org/booklore/controller/KoboController.java
the versioned thumbnails are not seemingly called by kobo and are not in other implementations of kobo sync
f7bb6d7 to
51ad113
Compare
Contributor
Yeah, for me looks about the same. Great work as always. I'm on borrowed Kobo right now, but for me it seems to work well also. |
balazs-szucs
approved these changes
Mar 27, 2026
This was referenced Apr 2, 2026
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* refactor(api): simplify routes for kobo thumbnails the versioned thumbnails are not seemingly called by kobo and are not in other implementations of kobo sync * feat(api): use redirect for kobo book images * chore(api): add documentation for thumbnail redirect
zachyale
pushed a commit
to zachyale/grimmory
that referenced
this pull request
Apr 17, 2026
* refactor(api): simplify routes for kobo thumbnails the versioned thumbnails are not seemingly called by kobo and are not in other implementations of kobo sync * feat(api): use redirect for kobo book images * chore(api): add documentation for thumbnail redirect
zachyale
pushed a commit
that referenced
this pull request
Apr 17, 2026
* refactor(api): simplify routes for kobo thumbnails the versioned thumbnails are not seemingly called by kobo and are not in other implementations of kobo sync * feat(api): use redirect for kobo book images * chore(api): add documentation for thumbnail redirect
zachyale
pushed a commit
that referenced
this pull request
Apr 22, 2026
* refactor(api): simplify routes for kobo thumbnails the versioned thumbnails are not seemingly called by kobo and are not in other implementations of kobo sync * feat(api): use redirect for kobo book images * chore(api): add documentation for thumbnail redirect
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
2 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.
Description
Combines the Kobo book image paths to one code path & swaps from the API making a request against kobo to instead telling the kobo reader to do a redirect to get the image. This means less strain on our API & also potentially faster image downloads.
This is clean up to support a toggle to disable the Kobo Proxy - to simplify the code path so that it's easier to disable.
Linked Issue: Related to #193
Changes
Summary by CodeRabbit
Refactor
Documentation
Tests