Update API models to match latest ImmichFrame server, fix retry logic#56
Update API models to match latest ImmichFrame server, fix retry logic#56dave-palt wants to merge 4 commits into
Conversation
… for issue immichFrame#55 - Sync ServerSettings with latest ClientSettingsDto: remove margin and unattendedMode, add clockDateFormat, showProgressBar, showAlbumName, weatherIconUrl, imagePan fields; make appropriate fields nullable - Make ImageResponse and Weather fields nullable with Kotlin defaults - Add api/Config/Version endpoint to ApiService - Fix getServerSettings() retry logic: 4xx errors (404, 401, 403) now fail immediately with descriptive messages instead of infinite retries - Add null safety to all downstream code in MainActivity, ScreenSaverService, and WidgetProvider
|
Warning Review limit reached
More reviews will be available in 47 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors API models to be nullable/default-tolerant; makes image decoding and portrait-merge flows null-safe across MainActivity, ScreenSaverService, and WidgetProvider; updates photo/date/weather formatting to handle absent server settings; and refactors getServerSettings() to use HTTP-status-derived retryability with centralized failure handling. ChangesAPI Resilience and Null-Safe UI Rendering
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
188-190: ⚡ Quick winPotential race condition: capture
portraitCachein a local variable.Same issue as in
MainActivity.kt-portraitCacheis a mutable class field that can be modified from the Main thread while this coroutine runs onDispatchers.IO.♻️ Proposed fix
val isPortrait = randomBitmap.height > randomBitmap.width if (isPortrait && serverSettings.layout == "splitview") { - if (portraitCache != null && portraitCache!!.randomImageBase64 != null) { - var decodedPortraitImageBitmap = - Helpers.decodeBitmapFromBytes(portraitCache!!.randomImageBase64!!) + val cachedBase64 = portraitCache?.randomImageBase64 + if (cachedBase64 != null) { + var decodedPortraitImageBitmap = + Helpers.decodeBitmapFromBytes(cachedBase64)🤖 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 `@app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt` around lines 188 - 190, The code reads the mutable field portraitCache inside a coroutine on Dispatchers.IO which can race with main-thread updates; capture portraitCache into a local val at the start of the coroutine (e.g. val localPortrait = portraitCache) and then use localPortrait when checking randomImageBase64 and calling Helpers.decodeBitmapFromBytes to ensure a consistent snapshot; update references in ScreenSaverService where portraitCache is accessed inside the IO coroutine so all null-checks and decode calls use the local variable instead of the volatile field.app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
198-200: ⚡ Quick winPotential race condition: capture
portraitCachein a local variable.
portraitCacheis a mutable class field that can be set tonullbyupdateUI()on the Main thread (line 293) while this coroutine runs onDispatchers.IO. The null check at line 198 doesn't guarantee the field remains non-null at line 200, risking an NPE.♻️ Proposed fix to avoid TOCTOU race
val isPortrait = randomBitmap.height > randomBitmap.width if (isPortrait && serverSettings.layout == "splitview") { - if (portraitCache != null && portraitCache!!.randomImageBase64 != null) { - var decodedPortraitImageBitmap = - Helpers.decodeBitmapFromBytes(portraitCache!!.randomImageBase64!!) + val cachedBase64 = portraitCache?.randomImageBase64 + if (cachedBase64 != null) { + var decodedPortraitImageBitmap = + Helpers.decodeBitmapFromBytes(cachedBase64)🤖 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 `@app/src/main/java/com/immichframe/immichframe/MainActivity.kt` around lines 198 - 200, Capture the mutable field portraitCache into a local val at the start of the coroutine before the null check to avoid a TOCTOU race (portraitCache can be nulled by updateUI() on the main thread while this runs on Dispatchers.IO); replace usages of portraitCache!! with the local variable (e.g., val localPortrait = portraitCache; if (localPortrait?.randomImageBase64 != null) { val decodedPortraitImageBitmap = Helpers.decodeBitmapFromBytes(localPortrait.randomImageBase64) ... }) so you never dereference the class field directly and eliminate the forced !! operator.
🤖 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.
Nitpick comments:
In `@app/src/main/java/com/immichframe/immichframe/MainActivity.kt`:
- Around line 198-200: Capture the mutable field portraitCache into a local val
at the start of the coroutine before the null check to avoid a TOCTOU race
(portraitCache can be nulled by updateUI() on the main thread while this runs on
Dispatchers.IO); replace usages of portraitCache!! with the local variable
(e.g., val localPortrait = portraitCache; if (localPortrait?.randomImageBase64
!= null) { val decodedPortraitImageBitmap =
Helpers.decodeBitmapFromBytes(localPortrait.randomImageBase64) ... }) so you
never dereference the class field directly and eliminate the forced !! operator.
In `@app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt`:
- Around line 188-190: The code reads the mutable field portraitCache inside a
coroutine on Dispatchers.IO which can race with main-thread updates; capture
portraitCache into a local val at the start of the coroutine (e.g. val
localPortrait = portraitCache) and then use localPortrait when checking
randomImageBase64 and calling Helpers.decodeBitmapFromBytes to ensure a
consistent snapshot; update references in ScreenSaverService where portraitCache
is accessed inside the IO coroutine so all null-checks and decode calls use the
local variable instead of the volatile field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b4db99d-cea7-46ae-8cdd-b9ffac9259bc
📒 Files selected for processing (4)
app/src/main/java/com/immichframe/immichframe/Helpers.ktapp/src/main/java/com/immichframe/immichframe/MainActivity.ktapp/src/main/java/com/immichframe/immichframe/ScreenSaverService.ktapp/src/main/java/com/immichframe/immichframe/WidgetProvider.kt
Capture mutable portraitCache field into a local val at the start of the splitview branch to avoid racing with main-thread updates from updateUI(). Eliminates forced !! operator dereferences.
| randomBitmap | ||
| ) | ||
| appWidgetManager.updateAppWidget(appWidgetId, views) | ||
| } |
There was a problem hiding this comment.
I considered removing this whole widget. Now that Immich includes one itself I am not sure it is necessary?
There was a problem hiding this comment.
I don't follow the development of immich, so I did not know that there is a built-in widget.
Myself I am using this app without a widget nor immich with an old tablet, if there are people with a similar situation that also use the widget it might be worth keeping I guess?
In general, unless it become a big deal to maintain I don't see a particular reason to remove it, but it's up to you.
There was a problem hiding this comment.
I don't follow the development of immich, so I did not know that there is a built-in widget. Myself I am using this app without a widget nor immich with an old tablet, if there are people with a similar situation that also use the widget it might be worth keeping I guess? In general, unless it become a big deal to maintain I don't see a particular reason to remove it, but it's up to you.
What are you using it for then (out of curiosity)?
edit- nevermind, think you meant you aren't using either this widget or Immich's.
There was a problem hiding this comment.
Yeah, I am using immich in my phone and this app on the tablet, I don't really use widgets in general
| val style: String? = null, | ||
| val baseFontSize: String? = null, | ||
| val showWeatherDescription: Boolean = false, | ||
| val weatherIconUrl: String? = null, |
There was a problem hiding this comment.
Why did you add the 8 new/missing fields if they are not being used? I only included what we actually used.
There was a problem hiding this comment.
I was not aware of the decision of including only the fields we used, parly because I did not think about this a something to consider.
I just updated the pr to discard the unused fields
|
Nice job and nice improvements. This non-webview code has not gotten much attention so thanks! Just made a couple comments. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (2)
279-289:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid rendering dangling
|separators in merged metadata.On Line 279 and Line 287, if only one side is present, the output becomes
" | value"or"value | ".💡 Proposed fix
- val mergedPhotoDate = - if (cachedPhotoDate.isNotEmpty() || responsePhotoDate.isNotEmpty()) { - "$cachedPhotoDate | $responsePhotoDate" - } else { - "" - } + val mergedPhotoDate = listOf(cachedPhotoDate, responsePhotoDate) + .filter { it.isNotEmpty() } + .joinToString(" | ") - val mergedImageLocation = - if (cachedImageLocation.isNotEmpty() || responseImageLocation.isNotEmpty()) { - "$cachedImageLocation | $responseImageLocation" - } else { - "" - } + val mergedImageLocation = listOf(cachedImageLocation, responseImageLocation) + .filter { it.isNotEmpty() } + .joinToString(" | ")🤖 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 `@app/src/main/java/com/immichframe/immichframe/MainActivity.kt` around lines 279 - 289, The mergedPhotoDate and mergedImageLocation logic can produce dangling " | " when one side is empty; change each merge to collect the two parts (cachedPhotoDate and responsePhotoDate for mergedPhotoDate; cachedImageLocation and responseImageLocation for mergedImageLocation), filter out empty strings, and join the remaining parts with " | " (e.g., using a list + filter + joinToString) so you only get "a", "a | b", or "" instead of " | b" or "a | ".
503-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the class
handlerfor retries so callbacks are canceled on destroy.Line 503 creates a new
Handlerper retry, butonDestroy()only clears callbacks from thehandlerfield. Retries can continue after activity teardown.🛠️ Proposed fix
- Handler(Looper.getMainLooper()).postDelayed({ + handler.postDelayed({ attemptFetch() }, retryDelayMillis)🤖 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 `@app/src/main/java/com/immichframe/immichframe/MainActivity.kt` around lines 503 - 505, The retry currently creates a fresh Handler per attempt (Handler(Looper.getMainLooper()).postDelayed) which bypasses the activity-level handler cancellation; change the retry to use the existing handler field (call handler.postDelayed { attemptFetch() } with the same retryDelayMillis) and ensure that handler is initialized on the main looper (e.g., Handler(Looper.getMainLooper())) so onDestroy()'s handler.removeCallbacksAndMessages(null) will cancel pending retries; keep references to the handler and attemptFetch() unchanged so callbacks are properly removed in onDestroy().
🤖 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.
Outside diff comments:
In `@app/src/main/java/com/immichframe/immichframe/MainActivity.kt`:
- Around line 279-289: The mergedPhotoDate and mergedImageLocation logic can
produce dangling " | " when one side is empty; change each merge to collect the
two parts (cachedPhotoDate and responsePhotoDate for mergedPhotoDate;
cachedImageLocation and responseImageLocation for mergedImageLocation), filter
out empty strings, and join the remaining parts with " | " (e.g., using a list +
filter + joinToString) so you only get "a", "a | b", or "" instead of " | b" or
"a | ".
- Around line 503-505: The retry currently creates a fresh Handler per attempt
(Handler(Looper.getMainLooper()).postDelayed) which bypasses the activity-level
handler cancellation; change the retry to use the existing handler field (call
handler.postDelayed { attemptFetch() } with the same retryDelayMillis) and
ensure that handler is initialized on the main looper (e.g.,
Handler(Looper.getMainLooper())) so onDestroy()'s
handler.removeCallbacksAndMessages(null) will cancel pending retries; keep
references to the handler and attemptFetch() unchanged so callbacks are properly
removed in onDestroy().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be85a760-80d8-4bd9-b1d1-31366f140dc4
📒 Files selected for processing (3)
app/src/main/java/com/immichframe/immichframe/Helpers.ktapp/src/main/java/com/immichframe/immichframe/MainActivity.ktapp/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt
💤 Files with no reviewable changes (1)
- app/src/main/java/com/immichframe/immichframe/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt
2 participants
Fixes #55
Changes
Data model updates (
Helpers.kt)ServerSettingswith latest ImmichFrame serverClientSettingsDto:marginandunattendedMode(no longer in server response)clockDateFormat,showProgressBar,showAlbumName,weatherIconUrl,imagePanclockFormat,photoDateFormat,imageLocationFormat,secondaryColor,style,layout,language,baseFontSize)ImageResponsefields nullableWeatherfields nullable with defaultsapi/Config/Versionendpoint toApiServiceFix retry logic (fixes #55)
getServerSettings()in bothMainActivityandScreenSaverService:Null safety
MainActivity,ScreenSaverService, andWidgetProviderto handle nullable fields from the updated modelsSummary by CodeRabbit
New Features
Bug Fixes