Skip to content

Conversation

@juanjsebgarcia
Copy link

@juanjsebgarcia juanjsebgarcia commented Jan 8, 2026

Add the ability to block settings access behind a numerical passcode. This is perfect for protecting my frames from my inquisitve children.

curl http://$IP:53287/settings still works and bypasses the pin code. My kids aren't that smart (yet).

Some notes:

  • Pincode is stored in plaintext, in a non-secure location. I think this is fine.
  • Makefiles make life easy
  • Claude assisted, but has been modified, reviewed and tested

Summary by CodeRabbit

  • New Features

    • Added PIN code protection for settings access, allowing users to secure sensitive settings with a numeric password.
    • Added tamper protection controls to lock or fully disable access to settings.
    • Settings protection can be enabled or disabled from the new Tamper Protection category.
  • Chores

    • Added build automation scripts for app compilation, testing, and deployment.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

A Makefile is added with Android build management targets, and PIN-code-based protection for settings access is implemented via shared preferences, password verification dialogs, and new preference UI elements across MainActivity, SettingsFragment, and the settings XML configuration.

Changes

Cohort / File(s) Summary
Build Infrastructure
Makefile
New file introducing phony targets for Android development: build, build-release, assemble, clean, install, install-debug, install-release, uninstall, test, lint, run, and help (default target). Targets manage Gradle tasks and device installation workflows.
Settings Access Control
app/src/main/java/com/immichframe/immichframe/MainActivity.kt
Modified onSettingsCommand routing to settingsActionFromRpc(). Added password-protected settings flow: settingsAction() checks for saved PIN in SharedPreferences; if present, prompts verification; otherwise opens settings directly. New methods: settingsActionFromRpc() (bypasses password), openSettings(), promptForPasswordVerification(). Added UI imports for dialog and input handling.
Settings Protection Logic
app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt
Introduces settingsPincode SwitchPreferenceCompat with state management and persistence. On enable: shows confirmation dialog and prompts numeric PIN creation. On disable: removes stored PIN. New promptForPasswordCreation() method validates and saves PIN to SharedPreferences. Initializes switch based on stored PIN existence.
Settings UI
app/src/main/res/xml/settings_view.xml
Closed existing Display Options category. Added new Tamper Protection category containing: settingsPincode SwitchPreferenceCompat (PIN protection toggle) and updated settingsLock SwitchPreferenceCompat (full settings lock toggle).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant MainActivity
    participant Dialog as PIN Dialog
    participant SharedPref as SharedPreferences
    participant SettingsActivity

    rect rgb(200, 220, 255)
    note over User,SettingsActivity: PIN Protection - First Access (Settings Enabled)
    User->>MainActivity: onSettingsCommand()
    MainActivity->>SharedPref: Check for saved PIN
    SharedPref-->>MainActivity: PIN exists
    MainActivity->>Dialog: promptForPasswordVerification()
    Dialog->>User: Show numeric PIN input
    User->>Dialog: Enter PIN
    Dialog->>Dialog: Verify PIN
    alt Correct PIN
        Dialog->>SettingsActivity: openSettings()
        SettingsActivity-->>User: Settings Activity launched
    else Incorrect PIN
        Dialog->>User: Show toast error
    end
    end

    rect rgb(200, 255, 220)
    note over User,SettingsActivity: PIN Setup - In Settings Fragment
    User->>SettingsActivity: Enable settingsPincode switch
    SettingsActivity->>Dialog: promptForPasswordCreation()
    Dialog->>User: Show confirmation dialog
    User->>Dialog: Confirm PIN creation
    Dialog->>User: Show numeric PIN input
    User->>Dialog: Enter PIN
    Dialog->>Dialog: Validate non-empty
    Dialog->>SharedPref: Save PIN
    Dialog->>SettingsActivity: Update switch state
    Dialog->>User: Show success toast
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A Makefile for builders so keen,
With targets that manage the Android machine!
Now shields guard the settings with PIN,
No sneaky rabbits can peek from within! 🔐✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add pincode protection to settings' accurately summarizes the main change: adding PIN code-based access control to the settings UI, which is the primary feature introduced across all modified files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @app/src/main/java/com/immichframe/immichframe/MainActivity.kt:
- Around line 729-750: promptForPasswordVerification currently may not reliably
mask input, forces non-cancelable dialogs, and lacks brute-force protection;
update it to use a masked input reliably (ensure EditText uses
passwordTransformationMethod fallback when
InputType.TYPE_NUMBER_VARIATION_PASSWORD doesn't mask), change
setCancelable(false) to setCancelable(true) so users can dismiss the dialog, and
add simple rate-limiting state (e.g. private vars failedPinAttempts and
pinLockoutUntil) checked at the start of promptForPasswordVerification to refuse
attempts while locked and to increment failedPinAttempts on wrong PIN, reset
failedPinAttempts to 0 on success (openSettings), and set pinLockoutUntil to
now+lockoutDuration after N failures to enforce a temporary lockout.

In @app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt:
- Around line 180-209: The PIN creation flow in promptForPasswordCreation lacks
confirmation, minimal length validation, and reliable masking; update
promptForPasswordCreation to enforce a minimum length (e.g., >=4), show a "Next"
action that calls a new promptForPasswordConfirmation(originalPassword,
chkSettingsPincode) which asks the user to re-enter the PIN and only saves to
PreferenceManager.getDefaultSharedPreferences when both entries match; ensure
both EditText instances enable consistent masking (e.g.,
setTransformationMethod(PasswordTransformationMethod.getInstance()) and/or
proper inputType) and add user-facing hints/toasts for "PIN too short" and "PINs
do not match" while unchecking chkSettingsPincode on failure.
🧹 Nitpick comments (2)
Makefile (1)

1-58: Consider addressing static analysis hints for best practices.

The Makefile is functional, but checkmake flags two minor style issues:

  • The help target body exceeds the recommended length (10 lines vs. 5).
  • The conventional all target is missing.

These are optional improvements that enhance maintainability and align with Make conventions.

♻️ Optional improvements

Add an all target and consider shortening the help output:

-.PHONY: build clean install install-debug install-release test help
+.PHONY: all build clean install install-debug install-release test help
+
+# Default: build debug
+all: build

 # Default target
 help:
 	@echo "Available targets:"
-	@echo "  build          - Build debug APK"
-	@echo "  build-release  - Build release APK"
-	@echo "  clean          - Clean build artifacts"
-	@echo "  install        - Install debug APK to connected device"
-	@echo "  install-release- Install release APK to connected device"
-	@echo "  uninstall      - Uninstall app from connected device"
-	@echo "  test           - Run tests"
-	@echo "  lint           - Run lint checks"
-	@echo "  assemble       - Build all variants"
+	@echo "  all, build, build-release, clean, install, install-release,"
+	@echo "  uninstall, test, lint, assemble, run"
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)

718-721: Document the intentional RPC bypass in code comments.

The PR description mentions that RPC access bypasses PIN protection, but this isn't evident from the code. Adding a comment here improves maintainability.

📝 Suggested documentation
 private fun settingsActionFromRpc() {
-    // RPC access bypasses password protection
+    // RPC access bypasses PIN protection by design.
+    // Use case: Remote administration when device is locked or inaccessible.
+    // Note: Secure your RPC endpoint at the network level.
     openSettings()
 }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b39f367 and 2dcc2c9.

📒 Files selected for processing (4)
  • Makefile
  • app/src/main/java/com/immichframe/immichframe/MainActivity.kt
  • app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt
  • app/src/main/res/xml/settings_view.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
  • stopImageTimer (374-377)
🪛 checkmake (0.2.2)
Makefile

[warning] 4-4: Target body for "help" exceeds allowed length of 5 (10).

(maxbodylength)


[warning] 1-1: Missing required phony target "all"

(minphony)

🔇 Additional comments (3)
app/src/main/res/xml/settings_view.xml (1)

32-43: LGTM! Clear separation of tamper protection options.

The new "Tamper Protection" category cleanly groups PIN protection and full settings lock. The updated title for settingsLock ("Fully disable/lock access...") effectively distinguishes it from the PIN option.

app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)

705-716: LGTM! Clean separation of authenticated vs. unauthenticated access.

The logic correctly checks for a saved PIN and routes to verification or direct access accordingly.

app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt (1)

82-111: LGTM! Proper confirmation flow for security-sensitive setting.

The two-stage confirmation (intent verification, then PIN creation) provides good UX for a security feature. The disable path correctly removes the stored PIN.

Comment on lines +729 to +750
private fun promptForPasswordVerification(correctPassword: String) {
val input = EditText(this)
input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

AlertDialog.Builder(this)
.setTitle("Enter Pincode")
.setMessage("Enter your pincode to access settings:")
.setView(input)
.setPositiveButton("OK") { _, _ ->
val enteredPassword = input.text.toString()
if (enteredPassword == correctPassword) {
openSettings()
} else {
Toast.makeText(this, "Incorrect pincode", Toast.LENGTH_SHORT).show()
}
}
.setNegativeButton("Cancel") { dialog, _ ->
dialog.dismiss()
}
.setCancelable(false)
.show()
}
Copy link

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Consider UX and security improvements to PIN verification.

Three concerns:

  1. Input masking: TYPE_NUMBER_VARIATION_PASSWORD may not mask input on all Android versions/devices. Consider testing or adding a fallback.

  2. Dialog cancelability: Line 748 sets setCancelable(false), forcing users to either enter a PIN or click Cancel. This could be frustrating if triggered accidentally. Consider setCancelable(true).

  3. Rate limiting: No protection against brute-force attempts. An attacker could try all 10,000 4-digit PINs rapidly.

🔒 Recommended improvements

For rate limiting, consider adding a simple lockout after failed attempts:

private var failedPinAttempts = 0
private var pinLockoutUntil = 0L

private fun promptForPasswordVerification(correctPassword: String) {
    // Check lockout
    if (System.currentTimeMillis() < pinLockoutUntil) {
        val remainingSec = (pinLockoutUntil - System.currentTimeMillis()) / 1000
        Toast.makeText(this, "Too many attempts. Try again in ${remainingSec}s", Toast.LENGTH_SHORT).show()
        return
    }

    val input = EditText(this)
    input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

    AlertDialog.Builder(this)
        .setTitle("Enter Pincode")
        .setMessage("Enter your pincode to access settings:")
        .setView(input)
        .setPositiveButton("OK") { _, _ ->
            val enteredPassword = input.text.toString()
            if (enteredPassword == correctPassword) {
                failedPinAttempts = 0
                openSettings()
            } else {
                failedPinAttempts++
                if (failedPinAttempts >= 3) {
                    pinLockoutUntil = System.currentTimeMillis() + 30_000 // 30s lockout
                    Toast.makeText(this, "Too many failed attempts. Locked for 30 seconds.", Toast.LENGTH_LONG).show()
                } else {
                    Toast.makeText(this, "Incorrect pincode", Toast.LENGTH_SHORT).show()
                }
            }
        }
        .setNegativeButton("Cancel") { dialog, _ ->
            dialog.dismiss()
        }
        .setCancelable(true)  // Allow dismissal
        .show()
}
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/MainActivity.kt around lines
729 - 750, promptForPasswordVerification currently may not reliably mask input,
forces non-cancelable dialogs, and lacks brute-force protection; update it to
use a masked input reliably (ensure EditText uses passwordTransformationMethod
fallback when InputType.TYPE_NUMBER_VARIATION_PASSWORD doesn't mask), change
setCancelable(false) to setCancelable(true) so users can dismiss the dialog, and
add simple rate-limiting state (e.g. private vars failedPinAttempts and
pinLockoutUntil) checked at the start of promptForPasswordVerification to refuse
attempts while locked and to increment failedPinAttempts on wrong PIN, reset
failedPinAttempts to 0 on success (openSettings), and set pinLockoutUntil to
now+lockoutDuration after N failures to enforce a temporary lockout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these are particular concerns for the use-case of the frame in a residential setting.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juanjsebgarcia Understood! For a residential frame protecting against children, the current implementation is appropriate. Thank you for clarifying the use case.

Comment on lines +180 to +209
private fun promptForPasswordCreation(chkSettingsPincode: SwitchPreferenceCompat?) {
val input = android.widget.EditText(requireContext())
input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

AlertDialog.Builder(requireContext())
.setTitle("Create Pincode")
.setMessage("Enter a pincode to protect settings access:")
.setView(input)
.setPositiveButton("Set") { _, _ ->
val password = input.text.toString()
if (password.isEmpty()) {
Toast.makeText(requireContext(), "Pincode cannot be empty", Toast.LENGTH_SHORT).show()
} else {
// Save the password
val sharedPrefs = PreferenceManager.getDefaultSharedPreferences(requireContext())
sharedPrefs.edit()
.putString("settings_pincode", password)
.apply()

// Update the switch
chkSettingsPincode?.isChecked = true

Toast.makeText(requireContext(), "Pincode protection enabled", Toast.LENGTH_SHORT).show()
}
}
.setNegativeButton("Cancel") { dialog, _ ->
dialog.dismiss()
}
.show()
}
Copy link

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Enhance PIN validation and user experience.

Several improvements would strengthen this feature:

  1. No PIN confirmation: Users should re-enter the PIN to prevent typos (especially since input is masked).

  2. Minimal validation: Line 190 only checks isEmpty(). Consider enforcing a minimum length (e.g., 4 digits) for basic security.

  3. Input masking: TYPE_NUMBER_VARIATION_PASSWORD may not mask input consistently across devices (same concern as in MainActivity).

🔐 Recommended improvements

Add PIN confirmation and minimum length validation:

private fun promptForPasswordCreation(chkSettingsPincode: SwitchPreferenceCompat?) {
    val input = android.widget.EditText(requireContext())
    input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD
    input.hint = "Minimum 4 digits"

    AlertDialog.Builder(requireContext())
        .setTitle("Create Pincode")
        .setMessage("Enter a pincode to protect settings access:")
        .setView(input)
        .setPositiveButton("Next") { _, _ ->
            val password = input.text.toString()
            if (password.length < 4) {
                Toast.makeText(requireContext(), "Pincode must be at least 4 digits", Toast.LENGTH_SHORT).show()
            } else {
                // Prompt for confirmation
                promptForPasswordConfirmation(password, chkSettingsPincode)
            }
        }
        .setNegativeButton("Cancel") { dialog, _ ->
            dialog.dismiss()
        }
        .show()
}

private fun promptForPasswordConfirmation(originalPassword: String, chkSettingsPincode: SwitchPreferenceCompat?) {
    val confirmInput = android.widget.EditText(requireContext())
    confirmInput.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

    AlertDialog.Builder(requireContext())
        .setTitle("Confirm Pincode")
        .setMessage("Re-enter your pincode:")
        .setView(confirmInput)
        .setPositiveButton("Set") { _, _ ->
            val confirmedPassword = confirmInput.text.toString()
            if (confirmedPassword == originalPassword) {
                // Save the password
                val sharedPrefs = PreferenceManager.getDefaultSharedPreferences(requireContext())
                sharedPrefs.edit()
                    .putString("settings_pincode", originalPassword)
                    .apply()

                // Update the switch
                chkSettingsPincode?.isChecked = true

                Toast.makeText(requireContext(), "Pincode protection enabled", Toast.LENGTH_SHORT).show()
            } else {
                Toast.makeText(requireContext(), "Pincodes do not match", Toast.LENGTH_SHORT).show()
                chkSettingsPincode?.isChecked = false
            }
        }
        .setNegativeButton("Cancel") { dialog, _ ->
            dialog.dismiss()
        }
        .show()
}
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt around
lines 180 - 209, The PIN creation flow in promptForPasswordCreation lacks
confirmation, minimal length validation, and reliable masking; update
promptForPasswordCreation to enforce a minimum length (e.g., >=4), show a "Next"
action that calls a new promptForPasswordConfirmation(originalPassword,
chkSettingsPincode) which asks the user to re-enter the PIN and only saves to
PreferenceManager.getDefaultSharedPreferences when both entries match; ensure
both EditText instances enable consistent masking (e.g.,
setTransformationMethod(PasswordTransformationMethod.getInstance()) and/or
proper inputType) and add user-facing hints/toasts for "PIN too short" and "PINs
do not match" while unchecking chkSettingsPincode on failure.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is not designed to be high security, just to keep the innocent out

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juanjsebgarcia, that makes perfect sense! For the use case of keeping children out, the current implementation is appropriate. Thank you for clarifying the design intent.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In @app/src/main/java/com/immichframe/immichframe/Helpers.kt:
- Around line 419-429: applyGammaToBitmap currently documents that it always
recycles the input bitmap but on the size-check path (width * height >
maxPixels) it returns the original bitmap without recycling; fix by returning a
new copy of the bitmap and then recycling the original to preserve the contract:
create a mutable copy of the input (use the bitmap.copy(...) or equivalent to
match Android Bitmap API), call bitmap.recycle() on the original, and return the
copy; update the size-check branch inside applyGammaToBitmap (referencing width,
height, maxPixels) to perform these steps and handle copy failures safely (e.g.,
if copy is null, fall back to returning the original without recycling).

In @app/src/main/java/com/immichframe/immichframe/PowerButtonReturnService.kt:
- Around line 67-74: The code calls
packageManager.getLaunchIntentForPackage(packageName) into returnIntent and then
calls startActivity(returnIntent) without handling a null; update
PowerButtonReturnService to check returnIntent for null after retrieving it
(before calling addFlags/startActivity), and if null log a warning including
packageName and skip startActivity (still call stopSelf or handle cleanup as
appropriate) to avoid the NPE from startActivity when getLaunchIntentForPackage
returns null.

In @app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt:
- Around line 208-250: The promptForPasswordCreation function is missing its
terminating brace, causing a syntax/compilation error; fix it by adding the
missing closing brace '}' immediately after the AlertDialog.show() call to
properly end promptForPasswordCreation (ensure the brace is placed before the
@Suppress("DEPRECATION") annotation), and verify indentation so the subsequent
onDisplayPreferenceDialog override remains outside the function.

In @app/src/main/java/com/immichframe/immichframe/SettingsTimeoutReceiver.kt:
- Around line 24-27: The receiver's code calls
context.packageManager.getLaunchIntentForPackage(...) without null-check and may
crash; update SettingsTimeoutReceiver to handle a null returnIntent (as done in
PowerButtonReturnService) by checking returnIntent for null before calling
addFlags/startActivity and log or silently return if null, ensuring the method
exits gracefully instead of dereferencing a null Intent.
🧹 Nitpick comments (7)
app/src/main/res/layout/preference_seekbar.xml (1)

13-13: Externalize hardcoded strings for internationalization.

The hardcoded text values ("0", "-100", "100", "Reset to Default") should be moved to strings.xml to support localization and follow Android best practices.

Example string resource definitions

Add to res/values/strings.xml:

<string name="seekbar_default_value">0</string>
<string name="seekbar_min_value">-100</string>
<string name="seekbar_max_value">100</string>
<string name="seekbar_reset_button">Reset to Default</string>

Then reference them in the layout:

-        android:text="0"
+        android:text="@string/seekbar_default_value"
-            android:text="-100"
+            android:text="@string/seekbar_min_value"
-            android:text="100"
+            android:text="@string/seekbar_max_value"
-        android:text="Reset to Default"
+        android:text="@string/seekbar_reset_button"

Also applies to: 34-34, 43-43, 55-55

app/src/main/res/xml/settings_view.xml (2)

36-39: Note: Pincode stored in plaintext per PR objectives.

The PR objectives explicitly state that the pincode is stored in plaintext in a non-secure location. This is a known limitation acknowledged by the author. For production use, consider migrating to Android's EncryptedSharedPreferences or the Keystore system for more secure credential storage.

If you'd like to explore more secure storage options in the future, I can provide implementation guidance for EncryptedSharedPreferences.


51-87: Consider adding dependency attributes for better UX.

The individual SeekBarPreference items (brightness, contrast, RGB channels, gamma) could include android:dependency="imageAdjustments" to automatically disable them when the parent "Image Adjustments?" switch is turned off. This provides clearer visual feedback to users about which settings are active.

Example dependency attribute
 <com.immichframe.immichframe.SeekBarPreference
     android:key="image_brightness"
     android:title="Brightness"
     android:defaultValue="0"
+    android:dependency="imageAdjustments"
     app:min="-100"
     app:max="100" />

Apply similarly to the other SeekBarPreference elements.

app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)

531-545: Consider extracting duplicated WebView filter application logic.

The WebView color filter application code (with hardware/software layer fallback) is duplicated between the initialization block (lines 531-545) and the reapplyImageFilters() method (lines 577-589). Extracting this into a private helper method would improve maintainability and reduce the risk of inconsistencies.

♻️ Proposed refactor
private fun applyFilterToWebView() {
    if (!::webView.isInitialized) return
    
    val filter = Helpers.getImageAdjustmentFilter(this)
    if (filter != null) {
        val paint = Paint()
        paint.colorFilter = filter
        try {
            webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint)
        } catch (e: Exception) {
            Log.w("ScreenSaverService", "Hardware layer failed, falling back to software: ${e.message}")
            webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint)
        }
    } else {
        webView.setLayerType(View.LAYER_TYPE_HARDWARE, null)
    }
}

Then replace both occurrences:

-            // Apply color filter to WebView using layer rendering
-            val filter = Helpers.getImageAdjustmentFilter(this)
-            if (filter != null) {
-                val paint = Paint()
-                paint.colorFilter = filter
-                try {
-                    webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint)
-                } catch (e: Exception) {
-                    // Fallback to software layer if hardware fails
-                    Log.w("ScreenSaverService", "Hardware layer failed, falling back to software: ${e.message}")
-                    webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint)
-                }
-            } else {
-                webView.setLayerType(View.LAYER_TYPE_HARDWARE, null)
-            }
+            applyFilterToWebView()

And in reapplyImageFilters():

-        } else if (::webView.isInitialized) {
-            // Reapply filter to WebView (only if initialized)
-            val filter = Helpers.getImageAdjustmentFilter(this)
-            if (filter != null) {
-                val paint = Paint()
-                paint.colorFilter = filter
-                try {
-                    webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint)
-                } catch (e: Exception) {
-                    Log.w("ScreenSaverService", "Hardware layer failed, falling back to software: ${e.message}")
-                    webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint)
-                }
-            } else {
-                webView.setLayerType(View.LAYER_TYPE_HARDWARE, null)
-            }
+        } else {
+            applyFilterToWebView()
         }

Also applies to: 577-589

app/src/main/java/com/immichframe/immichframe/PowerButtonReturnService.kt (1)

31-33: Add receiver export flag for Android 13+ compatibility.

Starting with Android 13 (API 33), registerReceiver requires specifying whether the receiver should be exported. Since this is an internal receiver for system broadcasts, use RECEIVER_NOT_EXPORTED.

Proposed fix
         // Register broadcast receiver for screen off events
         val filter = IntentFilter(Intent.ACTION_SCREEN_OFF)
-        registerReceiver(screenOffReceiver, filter)
+        if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) {
+            registerReceiver(screenOffReceiver, filter, Context.RECEIVER_NOT_EXPORTED)
+        } else {
+            registerReceiver(screenOffReceiver, filter)
+        }
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)

597-612: Consider extracting duplicated WebView filter logic.

The WebView filter application logic (lines 597-612) is duplicated in reapplyImageFilters() (lines 642-656). Consider extracting to a private helper method to reduce duplication and maintenance burden.

Proposed refactor
private fun applyFilterToWebView() {
    val filter = Helpers.getImageAdjustmentFilter(this)
    if (filter != null) {
        val paint = Paint()
        paint.colorFilter = filter
        try {
            webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint)
        } catch (e: Exception) {
            Log.w("MainActivity", "Hardware layer failed, falling back to software: ${e.message}")
            webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint)
        }
    } else {
        webView.setLayerType(View.LAYER_TYPE_HARDWARE, null)
    }
}

Then call applyFilterToWebView() from both locations.

Also applies to: 636-658

app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt (1)

49-55: Use explicit locale for String.format to ensure consistent formatting.

Static analysis correctly identifies implicit locale usage. For display values, use Locale.US or Locale.getDefault() explicitly to ensure consistent decimal formatting across different device locales.

Proposed fix
+import java.util.Locale
+
 // In updateSummary():
     private fun updateSummary() {
         summary = if (key == "image_gamma") {
-            String.format("%.2f", currentValue / 100.0)
+            String.format(Locale.US, "%.2f", currentValue / 100.0)
         } else {
             currentValue.toString()
         }
     }

 // In updateValueText():
     private fun updateValueText(value: Int, isGamma: Boolean) {
         valueText?.text = if (isGamma) {
-            String.format("%.2f", value / 100.0)
+            String.format(Locale.US, "%.2f", value / 100.0)
         } else {
             value.toString()
         }
     }

Apply similar changes to lines 81 and 86 for min/max labels.

Also applies to: 120-126

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dcc2c9 and 46e3cfc.

📒 Files selected for processing (12)
  • app/src/main/AndroidManifest.xml
  • app/src/main/java/com/immichframe/immichframe/Helpers.kt
  • app/src/main/java/com/immichframe/immichframe/MainActivity.kt
  • app/src/main/java/com/immichframe/immichframe/PowerButtonReturnService.kt
  • app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt
  • app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt
  • app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt
  • app/src/main/java/com/immichframe/immichframe/SettingsTimeoutReceiver.kt
  • app/src/main/java/com/immichframe/immichframe/WidgetProvider.kt
  • app/src/main/res/layout/preference_seekbar.xml
  • app/src/main/res/values/attrs.xml
  • app/src/main/res/xml/settings_view.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
  • reapplyImageFilters (636-658)
🪛 detekt (1.23.8)
app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt

[warning] 104-104: This empty block of code can be removed.

(detekt.empty-blocks.EmptyFunctionBlock)


[warning] 105-105: This empty block of code can be removed.

(detekt.empty-blocks.EmptyFunctionBlock)


[warning] 51-51: String.format("%.2f", currentValue / 100.0) uses implicitly default locale for string formatting.

(detekt.potential-bugs.ImplicitDefaultLocale)


[warning] 81-81: String.format("%.1f", preference.min / 100.0) uses implicitly default locale for string formatting.

(detekt.potential-bugs.ImplicitDefaultLocale)


[warning] 86-86: String.format("%.1f", preference.max / 100.0) uses implicitly default locale for string formatting.

(detekt.potential-bugs.ImplicitDefaultLocale)


[warning] 122-122: String.format("%.2f", value / 100.0) uses implicitly default locale for string formatting.

(detekt.potential-bugs.ImplicitDefaultLocale)

🔇 Additional comments (14)
app/src/main/AndroidManifest.xml (2)

86-88: LGTM!

The SettingsTimeoutReceiver is properly declared with android:exported="false", which is appropriate for a broadcast receiver that handles internal scheduled timeouts. The lack of an intent-filter is correct, as this receiver likely receives explicit intents from the alarm manager.


9-9: No action required. The SCHEDULE_EXACT_ALARM permission is already properly handled. The code checks canScheduleExactAlarms() at line 58 of SettingsTimeoutReceiver.kt and gracefully falls back to inexact alarms (setAndAllowWhileIdle()) when the permission is unavailable, ensuring the timeout feature functions correctly across all Android versions.

app/src/main/java/com/immichframe/immichframe/WidgetProvider.kt (1)

93-93: LGTM! Image adjustments correctly integrated into widget update flow.

The call to applyImageAdjustmentsToBitmap is appropriately placed after bitmap quality reduction and before display, ensuring the widget respects user-configured image adjustments.

app/src/main/res/values/attrs.xml (1)

1-7: LGTM! Clean attribute declaration.

The SeekBarPreference styleable declaration is standard and correctly formatted for use with custom preference attributes.

app/src/main/res/xml/settings_view.xml (1)

2-2: LGTM! Namespace declaration required for custom attributes.

The xmlns:app namespace is necessary for the app:min and app:max attributes used by the SeekBarPreference elements.

app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (3)

9-9: LGTM! Import required for layer-based color filtering.

The Paint import supports the new color filter application to WebView and ImageView components.


229-229: LGTM! Filter applied during image transition.

Applying the color filter to imageViewNew ensures that the incoming image respects the user's adjustment settings during the crossfade transition.


555-556: LGTM! Filters reapplied after settings load.

Calling reapplyImageFilters() after successfully loading server settings ensures that any already-displayed content receives the current image adjustment filters.

app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)

774-795: LGTM - PIN verification flow is functional.

The PIN verification dialog implementation works correctly. The PIN is compared in plain text as documented in the PR description. The RPC bypass is intentional per the PR objectives.

app/src/main/java/com/immichframe/immichframe/SettingsTimeoutReceiver.kt (1)

55-91: Well-implemented alarm scheduling with proper API-level handling.

The code correctly handles Android 12+ exact alarm permission checks with fallback to inexact alarms, and uses appropriate alarm types for older API levels.

app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt (1)

239-249: Appropriate handling of deprecated API with suppression.

The use of deprecated setTargetFragment is correctly suppressed with annotation and documented in comments. This is the standard approach for PreferenceDialogFragmentCompat compatibility.

app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt (1)

91-118: LGTM - SeekBar listener implementation is correct.

The empty onStartTrackingTouch and onStopTrackingTouch methods are required by the OnSeekBarChangeListener interface. Only onProgressChanged needs implementation here. The static analysis warning about empty blocks can be safely ignored for this pattern.

app/src/main/java/com/immichframe/immichframe/Helpers.kt (2)

269-286: Thread-safe caching implementation looks good.

The filter cache uses proper synchronization and includes all relevant parameters in the cache key. The early return on cache hit avoids unnecessary filter recreation.


382-409: Clean bitmap lifecycle management with proper recycling.

The function correctly manages bitmap lifecycle: creates new result bitmap, applies filter, recycles input, then applies gamma (which handles its own recycling). The documented contract is maintained.

Comment on lines 419 to 429
private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap {
val width = bitmap.width
val height = bitmap.height

// Safety check: prevent excessive memory allocation
// Max 4000x4000 = 16M pixels = 64MB for ARGB_8888
val maxPixels = 4000 * 4000
if (width * height > maxPixels) {
Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma")
return bitmap
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Inconsistent bitmap recycling contract when size check fails.

The function documents that it always recycles the input bitmap, but when the size check fails (line 426-429), it returns the original bitmap without recycling. This breaks the documented contract and could lead to confusion or memory issues.

Proposed fix - return a copy to maintain contract
         // Safety check: prevent excessive memory allocation
         // Max 4000x4000 = 16M pixels = 64MB for ARGB_8888
         val maxPixels = 4000 * 4000
         if (width * height > maxPixels) {
             Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma")
-            return bitmap
+            // Return copy and recycle original to maintain contract
+            val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.ARGB_8888, false)
+            bitmap.recycle()
+            return copy
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap {
val width = bitmap.width
val height = bitmap.height
// Safety check: prevent excessive memory allocation
// Max 4000x4000 = 16M pixels = 64MB for ARGB_8888
val maxPixels = 4000 * 4000
if (width * height > maxPixels) {
Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma")
return bitmap
}
private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap {
val width = bitmap.width
val height = bitmap.height
// Safety check: prevent excessive memory allocation
// Max 4000x4000 = 16M pixels = 64MB for ARGB_8888
val maxPixels = 4000 * 4000
if (width * height > maxPixels) {
Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma")
// Return copy and recycle original to maintain contract
val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.ARGB_8888, false)
bitmap.recycle()
return copy
}
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/Helpers.kt around lines 419 -
429, applyGammaToBitmap currently documents that it always recycles the input
bitmap but on the size-check path (width * height > maxPixels) it returns the
original bitmap without recycling; fix by returning a new copy of the bitmap and
then recycling the original to preserve the contract: create a mutable copy of
the input (use the bitmap.copy(...) or equivalent to match Android Bitmap API),
call bitmap.recycle() on the original, and return the copy; update the
size-check branch inside applyGammaToBitmap (referencing width, height,
maxPixels) to perform these steps and handle copy failures safely (e.g., if copy
is null, fall back to returning the original without recycling).

Comment on lines 67 to 74
// Launch ImmichFrame and bring it to foreground
Log.d(TAG, "App in background, bringing to foreground via power button")
val returnIntent = packageManager.getLaunchIntentForPackage(packageName)
returnIntent?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
startActivity(returnIntent)

// Stop this service since we're done
stopSelf()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential NPE if launch intent is null.

getLaunchIntentForPackage can return null. While addFlags uses safe call, startActivity(returnIntent) will crash if returnIntent is null.

Proposed fix
         // Launch ImmichFrame and bring it to foreground
         Log.d(TAG, "App in background, bringing to foreground via power button")
         val returnIntent = packageManager.getLaunchIntentForPackage(packageName)
-        returnIntent?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
-        startActivity(returnIntent)
+        returnIntent?.let {
+            it.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
+            startActivity(it)
+        } ?: Log.w(TAG, "Could not get launch intent for package")

         // Stop this service since we're done
         stopSelf()
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/PowerButtonReturnService.kt
around lines 67 - 74, The code calls
packageManager.getLaunchIntentForPackage(packageName) into returnIntent and then
calls startActivity(returnIntent) without handling a null; update
PowerButtonReturnService to check returnIntent for null after retrieving it
(before calling addFlags/startActivity), and if null log a warning including
packageName and skip startActivity (still call stopSelf or handle cleanup as
appropriate) to avoid the NPE from startActivity when getLaunchIntentForPackage
returns null.

Comment on lines 24 to 27
// Launch ImmichFrame and bring it to foreground
val returnIntent = context.packageManager.getLaunchIntentForPackage(context.packageName)
returnIntent?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
context.startActivity(returnIntent)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle null launch intent to prevent potential crash.

Similar to PowerButtonReturnService, getLaunchIntentForPackage can return null. The receiver should handle this gracefully.

Proposed fix
         // Launch ImmichFrame and bring it to foreground
         val returnIntent = context.packageManager.getLaunchIntentForPackage(context.packageName)
-        returnIntent?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
-        context.startActivity(returnIntent)
+        returnIntent?.let {
+            it.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
+            context.startActivity(it)
+        } ?: Log.w(TAG, "Could not get launch intent for package")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Launch ImmichFrame and bring it to foreground
val returnIntent = context.packageManager.getLaunchIntentForPackage(context.packageName)
returnIntent?.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
context.startActivity(returnIntent)
// Launch ImmichFrame and bring it to foreground
val returnIntent = context.packageManager.getLaunchIntentForPackage(context.packageName)
returnIntent?.let {
it.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP)
context.startActivity(it)
} ?: Log.w(TAG, "Could not get launch intent for package")
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/SettingsTimeoutReceiver.kt
around lines 24 - 27, The receiver's code calls
context.packageManager.getLaunchIntentForPackage(...) without null-check and may
crash; update SettingsTimeoutReceiver to handle a null returnIntent (as done in
PowerButtonReturnService) by checking returnIntent for null before calling
addFlags/startActivity and log or silently return if null, ensuring the method
exits gracefully instead of dereferencing a null Intent.

Signed-off-by: juanjsebgarcia <github@juangarcia.co.uk>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @app/src/main/java/com/immichframe/immichframe/MainActivity.kt:
- Around line 729-750: The PIN dialog in promptForPasswordVerification lacks
rate limiting and a max-attempt lockout and also uses a redundant
setCancelable(false); fix by adding an attempt counter and last-failure
timestamp (e.g., in SharedPreferences) keyed to the settings access, check these
values before verifying and refuse/show a locked message when the temporary
lockout is active, increment attempts on each wrong entry, compute an
exponential backoff delay (or fixed lock duration after N failures), persist the
backoff/lockout state and reset attempts on successful auth, disable the OK
button (or ignore input) while the backoff timer is active, log each failed
attempt with timestamps (using Log or existing analytics/logging helpers), and
remove the redundant setCancelable(false) call from the AlertDialog chain in
promptForPasswordVerification.

In @Makefile:
- Line 1: The .PHONY declaration is missing several targets so Make may treat
them as files; update the .PHONY line (the existing .PHONY: build clean install
install-debug install-release test help) to include the additional phony targets
build-release, assemble, uninstall, lint, and run so Make always executes those
rules instead of treating same‑named files as up‑to‑date.
🧹 Nitpick comments (1)
Makefile (1)

4-4: Consider adding an "all" target per Makefile convention.

While not critical, Makefile convention typically includes an all target as the default build target. You could add it after line 3:

📝 Optional enhancement
 # Default target
+all: build
+
 help:
 	@echo "Available targets:"

This would provide a standard "make all" command that builds the debug APK, though keeping help as the default is also reasonable for developer ergonomics.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46e3cfc and e654ebb.

📒 Files selected for processing (4)
  • Makefile
  • app/src/main/java/com/immichframe/immichframe/MainActivity.kt
  • app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt
  • app/src/main/res/xml/settings_view.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
  • stopImageTimer (374-377)
🪛 checkmake (0.2.2)
Makefile

[warning] 4-4: Target body for "help" exceeds allowed length of 5 (10).

(maxbodylength)


[warning] 1-1: Missing required phony target "all"

(minphony)

🔇 Additional comments (11)
Makefile (3)

4-15: LGTM! Help target is well-structured.

The help target provides clear documentation of available commands. The static analysis warning about body length exceeding 5 lines is a false positive—help targets inherently require multiple lines to document all available targets.


17-54: LGTM! Build and install targets are properly structured.

The targets correctly wrap Gradle commands with appropriate dependencies. The explicit build prerequisites for install and install-release targets ensure proper ordering and make dependencies clear to users.


57-58: LGTM! Run target correctly launches the app.

The run target properly depends on install and uses the correct adb command to launch MainActivity.

app/src/main/res/xml/settings_view.xml (1)

32-43: LGTM! Tamper Protection category added correctly.

The XML structure properly closes the Display Options category and introduces the new Tamper Protection preferences with clear titles and appropriate keys that align with the Kotlin implementation.

app/src/main/java/com/immichframe/immichframe/MainActivity.kt (5)

28-28: LGTM! Necessary imports for PIN protection dialog.

The new imports support the numeric password input dialog implementation.

Also applies to: 30-30, 34-34


167-167: RPC bypass of PIN protection is intentional.

The RPC server correctly calls settingsActionFromRpc() which bypasses PIN verification, providing a recovery mechanism if the PIN is forgotten.


705-716: Verify plaintext PIN storage is acceptable for your use case.

The PIN is stored in plaintext in SharedPreferences, which is accessible to users with device access or root privileges. For the stated use case (preventing children from accessing settings), this may be acceptable, but be aware that it provides minimal security against determined attackers.


718-721: LGTM! RPC bypass method is clear and well-commented.

The implementation correctly provides passwordless access via RPC commands, serving as a recovery mechanism.


723-727: LGTM! Good refactoring to extract settings launch logic.

Separating the settings launch into its own method improves code clarity and reusability.

app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt (2)

75-80: LGTM! Switch initialization correctly syncs with stored state.

The implementation properly reflects whether a PIN is currently set by checking SharedPreferences.


82-111: LGTM! Switch change logic correctly handles enable/disable flows.

The implementation properly:

  • Shows confirmation dialog before enabling PIN protection
  • Defers the switch state change until PIN creation succeeds (returns false at line 102)
  • Immediately removes PIN protection when disabled
  • Provides clear warning messages about recovery via RPC

Comment on lines +729 to +750
private fun promptForPasswordVerification(correctPassword: String) {
val input = EditText(this)
input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

AlertDialog.Builder(this)
.setTitle("Enter Pincode")
.setMessage("Enter your pincode to access settings:")
.setView(input)
.setPositiveButton("OK") { _, _ ->
val enteredPassword = input.text.toString()
if (enteredPassword == correctPassword) {
openSettings()
} else {
Toast.makeText(this, "Incorrect pincode", Toast.LENGTH_SHORT).show()
}
}
.setNegativeButton("Cancel") { dialog, _ ->
dialog.dismiss()
}
.setCancelable(false)
.show()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Add rate limiting to prevent brute-force attacks.

The PIN verification dialog has no rate limiting or maximum attempt restrictions. An attacker with physical access could repeatedly attempt PINs (especially if they're short) until finding the correct one.

Consider:

  • Adding a delay after failed attempts (exponential backoff)
  • Implementing a temporary lockout after N failed attempts
  • Logging failed attempts

Additionally, note that setCancelable(false) at line 748 is redundant since a Cancel button is already provided at line 745.

🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/MainActivity.kt around lines
729 - 750, The PIN dialog in promptForPasswordVerification lacks rate limiting
and a max-attempt lockout and also uses a redundant setCancelable(false); fix by
adding an attempt counter and last-failure timestamp (e.g., in
SharedPreferences) keyed to the settings access, check these values before
verifying and refuse/show a locked message when the temporary lockout is active,
increment attempts on each wrong entry, compute an exponential backoff delay (or
fixed lock duration after N failures), persist the backoff/lockout state and
reset attempts on successful auth, disable the OK button (or ignore input) while
the backoff timer is active, log each failed attempt with timestamps (using Log
or existing analytics/logging helpers), and remove the redundant
setCancelable(false) call from the AlertDialog chain in
promptForPasswordVerification.

Comment on lines +180 to +209
private fun promptForPasswordCreation(chkSettingsPincode: SwitchPreferenceCompat?) {
val input = android.widget.EditText(requireContext())
input.inputType = InputType.TYPE_CLASS_NUMBER or InputType.TYPE_NUMBER_VARIATION_PASSWORD

AlertDialog.Builder(requireContext())
.setTitle("Create Pincode")
.setMessage("Enter a pincode to protect settings access:")
.setView(input)
.setPositiveButton("Set") { _, _ ->
val password = input.text.toString()
if (password.isEmpty()) {
Toast.makeText(requireContext(), "Pincode cannot be empty", Toast.LENGTH_SHORT).show()
} else {
// Save the password
val sharedPrefs = PreferenceManager.getDefaultSharedPreferences(requireContext())
sharedPrefs.edit()
.putString("settings_pincode", password)
.apply()

// Update the switch
chkSettingsPincode?.isChecked = true

Toast.makeText(requireContext(), "Pincode protection enabled", Toast.LENGTH_SHORT).show()
}
}
.setNegativeButton("Cancel") { dialog, _ ->
dialog.dismiss()
}
.show()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Enforce minimum PIN length for security.

The PIN creation only validates that it's not empty (line 190), allowing single-digit PINs that provide minimal security. Even against the stated use case (preventing children from changing settings), short PINs are easily guessable.

Recommend enforcing a minimum PIN length of 4-6 digits.

🔒 Proposed fix to enforce minimum PIN length
             .setPositiveButton("Set") { _, _ ->
                 val password = input.text.toString()
-                if (password.isEmpty()) {
-                    Toast.makeText(requireContext(), "Pincode cannot be empty", Toast.LENGTH_SHORT).show()
+                if (password.length < 4) {
+                    Toast.makeText(requireContext(), "Pincode must be at least 4 digits", Toast.LENGTH_SHORT).show()
                 } else {
                     // Save the password
                     val sharedPrefs = PreferenceManager.getDefaultSharedPreferences(requireContext())

@@ -0,0 +1,58 @@
.PHONY: build clean install install-debug install-release test help
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add missing targets to .PHONY declaration.

The .PHONY declaration is incomplete. Several targets defined in the Makefile are missing: build-release, assemble, uninstall, lint, and run. Without declaring these as phony, Make may not execute them correctly if files with matching names exist.

📝 Proposed fix
-.PHONY: build clean install install-debug install-release test help
+.PHONY: help build build-release assemble clean install install-debug install-release uninstall test lint run
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.PHONY: build clean install install-debug install-release test help
.PHONY: help build build-release assemble clean install install-debug install-release uninstall test lint run
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

🤖 Prompt for AI Agents
In @Makefile at line 1, The .PHONY declaration is missing several targets so
Make may treat them as files; update the .PHONY line (the existing .PHONY: build
clean install install-debug install-release test help) to include the additional
phony targets build-release, assemble, uninstall, lint, and run so Make always
executes those rules instead of treating same‑named files as up‑to‑date.

@JW-CH JW-CH requested a review from 3rob3 January 9, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant