fix: Refactor URLService::load() to improve HTTP handling - #362
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesMap provider settings and UI
HTTP request and tile response handling
PNG decoding and tile saving
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MapUI
participant URLService
participant HTTPClient
participant PNGDecoder
participant TileStorage
MapUI->>URLService: request map tile
URLService->>HTTPClient: configure request and timeout
HTTPClient-->>URLService: return bounded PNG response
URLService->>HTTPClient: end request
URLService->>PNGDecoder: decode PNG response
PNGDecoder-->>URLService: return image
URLService->>TileStorage: save decoded tile
TileStorage-->>URLService: return save result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
source/graphics/map/URLService.cpp (1)
145-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider restoring a scope guard for
pngImage.The four exit paths each free
pngImage, and the current code is correct. The manual frees replace the previous RAII guard, so any new early return in this function silently leaks the buffer. A small guard removes the four duplicated calls and keeps the free tied to the scope.♻️ Proposed refactor
Declare the guard right after the allocation succeeds, then delete the individual
lv_free(pngImage)calls at lines 145, 150, 157, and 161.struct LvBufferGuard { uint8_t *ptr; ~LvBufferGuard() { if (ptr) lv_free(ptr); } };LvBufferGuard pngGuard{pngImage};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/graphics/map/URLService.cpp` around lines 145 - 162, Restore an RAII scope guard for the allocated pngImage immediately after successful allocation in the tile-decoding function, using the shown LvBufferGuard pattern and keeping ownership tied to the local scope. Remove the individual lv_free(pngImage) calls from all success and failure exits while preserving their existing return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/graphics/map/URLService.cpp`:
- Around line 91-98: Validate the result of HTTPClient::getStreamPtr() before
entering the read loop. In the surrounding image-fetch function, handle a null
stream by releasing the already-allocated pngImage and returning through the
existing failure path, preventing dereferences of stream when the connection
drops after http.GET().
- Line 96: Update maxIdleSpins and the nearby idleSpins variable in the
tile-read loop to use an unsigned type capable of representing the full
MUI_MAX_IDLE_SPINS configuration range, avoiding truncation for values above 255
while preserving the existing loop behavior.
---
Nitpick comments:
In `@source/graphics/map/URLService.cpp`:
- Around line 145-162: Restore an RAII scope guard for the allocated pngImage
immediately after successful allocation in the tile-decoding function, using the
shown LvBufferGuard pattern and keeping ownership tied to the local scope.
Remove the individual lv_free(pngImage) calls from all success and failure exits
while preserving their existing return behavior.
🪄 Autofix
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 Plus
Run ID: 268cc3c8-3f6d-4cf8-90fb-b9780f7576a0
📒 Files selected for processing (1)
source/graphics/map/URLService.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/graphics/map/TileProvider.cpp`:
- Around line 50-53: Normalize provider selection through the existing
bounds-checked path in both TileProvider::url() overloads, preserving -1 as
unselected instead of converting it to size_t or resetting it to zero; update
source/graphics/map/TileProvider.cpp lines 50-53 and the other url() overload
accordingly. In source/graphics/TFT/TFTView_320x240.cpp lines 2686-2693, only
set the dropdown selection when TileProvider::selectedTemplate() is nonnegative
and below the provider count.
In `@source/graphics/map/URLService.cpp`:
- Around line 92-95: Move the LvFreeGuard for pngImage out of the HTTP scope and
into URLService::load() so it remains active through decoding, saving, and the
function return. Preserve the existing cleanup behavior while ensuring pngImage
is not freed before the later reads.
In `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 2795-2803: Update the provider handling around the URL checks so
each recognized-provider branch hides the other provider’s attribution element:
hide map_attribution_label when showing google_logo_image, and hide
google_logo_image when showing the OpenStreetMap label. Preserve the existing
fallback behavior that hides both elements.
🪄 Autofix
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 Plus
Run ID: 7bf3c9ce-d73d-4b07-bd0e-7d8a7d96406f
⛔ Files ignored due to path filters (2)
generated/ui_320x240/screens.cis excluded by!**/generated/**generated/ui_320x240/screens.his excluded by!**/generated/**
📒 Files selected for processing (7)
include/graphics/map/MapTileSettings.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/map/CURLService.cppsource/graphics/map/MapTileSettings.cppsource/graphics/map/TileProvider.cppsource/graphics/map/URLService.cppstudio/320x240/TFT320x240.eez-project
1 participant
This PR fixes some issues when downloading HTTP map tiles:
Summary by CodeRabbit
Bug Fixes
Improvements