Conversation
…n only be accessed via the unlock method.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a "Hidden" folder: new Folder.HIDDEN, HiddenFragment guarded by biometric/PIN prompt, ViewModel observable for hidden notes, UI/menu/action updates to move/toggle hidden state, LockUtils support for hidden flow, plus drawable, navigation, and string resources. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant HiddenFragment as HiddenFragment
participant LockUtils as LockUtils
participant Biometric as BiometricManager
participant DAO as BaseNoteDao
participant UI as NotesUI
User->>HiddenFragment: navigate to Hidden
HiddenFragment->>UI: hide() (binding.root = INVISIBLE)
HiddenFragment->>LockUtils: showBiometricOrPinPromptHidden(title, onSuccess, onFailure)
LockUtils->>Biometric: attempt biometric / check canAuthenticate
alt Biometric available & success
Biometric-->>LockUtils: success
LockUtils->>HiddenFragment: onSuccess()
HiddenFragment->>DAO: request hiddenNotes (getFrom Folder.HIDDEN)
DAO-->>HiddenFragment: return notes
HiddenFragment->>UI: show() (binding.root = VISIBLE) + display notes
else Biometric unavailable -> fallback to PIN or immediate success
LockUtils->>HiddenFragment: onSuccess()
HiddenFragment->>DAO: request hiddenNotes
DAO-->>HiddenFragment: return notes
HiddenFragment->>UI: show()
else Auth failure
Biometric-->>LockUtils: failure/error
LockUtils->>HiddenFragment: onFailure(error?)
HiddenFragment->>User: navigateUp() (deny access)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt`:
- Around line 25-29: The fragment currently hides its root in onStart() then
calls showBiometricOrPinPromptHidden(this, ..., onSuccess = { show() }) with an
empty failure lambda, which leaves the UI blank on cancel/error; update the
failure lambda (and the equivalent launcher-null path in LockUtils functions) to
restore a visible state or navigate away: e.g., call show() to reveal a
retryable locked UI or call
requireActivity().onBackPressedDispatcher.onBackPressed() / navigateUp to leave
the fragment, and optionally display a toast or locked-state view; locate
handlers in HiddenFragment.onStart, the hide()/show() calls, and the
biometric/pin launch code in LockUtils to implement the change.
- Around line 21-22: In getObservable() of HiddenFragment avoid force-unwrapping
model.hiddenNotes; instead return a safe non-null LiveData by checking
model.hiddenNotes and returning a fallback LiveData when it's null (e.g. a new
MutableLiveData<List<Item>>(emptyList())) so a cold open won't crash; reference
model.hiddenNotes and getObservable()/HiddenFragment to locate the change and
ensure BaseNoteModel.hiddenNotes initialization race is handled by providing the
safe fallback.
- Around line 31-36: The show()/hide() methods dereference binding with
binding!! which can crash if showBiometricOrPinPromptHidden() completes after
onDestroyView(); change both to safely access the view (e.g., replace binding!!
with binding? and set visibility only when non-null: binding?.root?.visibility =
VISIBLE / INVISIBLE) or guard with binding?.root?.let { it.visibility = ... }
and optionally check fragment state (isAdded or viewLifecycleOwner.lifecycle)
before touching the view to avoid post-destroy access.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/SearchFragment.kt`:
- Line 35: The fragment references R.id.Hidden (used in
SearchFragment.check(R.id.Hidden) / folder selection) but the ChipGroup in
fragment_notes.xml lacks a Chip with id "@+id/Hidden"; add a new Chip with
android:id="@+id/Hidden" (use the same style as the other filter chips, e.g.,
style="@style/FilterChip", wrap_content sizes, and text="@string/hidden") inside
the existing ChipGroup alongside Notes, Deleted, and Archived so the
check(R.id.Hidden) call can find and select the chip and enable filtering by the
Hidden folder.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt`:
- Around line 563-566: The Hidden menu case in MainActivity sets
baseModel.currentLabel to the Unlabeled sentinel CURRENT_LABEL_NONE, making
Hidden indistinguishable from Unlabeled; change the assignment in the
R.id.Hidden branch (the block that also calls
binding.NavigationView.setCheckedItem(destination.id)) to use a distinct
sentinel such as CURRENT_LABEL_EMPTY (or create a dedicated
CURRENT_LABEL_HIDDEN) instead of CURRENT_LABEL_NONE so menu rebuilds and Search
setup can differentiate Hidden from Unlabeled.
In `@app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt`:
- Around line 124-128: The PIN fallback path calls promptPinAuthentication with
a null activityResultLauncher so the PIN confirmation intent is launched but its
result is never delivered and onSuccess is never invoked; fix by either (A)
requiring a non-null ActivityResultLauncher: change promptPinAuthentication
signature to accept a non-null ActivityResultLauncher and update the caller here
to pass HiddenFragment’s launcher so the result callback can invoke onSuccess,
or (B) for API 21-22 implement the legacy flow: call
fragment.startActivityForResult for the KEYGUARD intent and handle the result in
the fragment’s onActivityResult to call onSuccess on success, or (C) if
intended, explicitly short-circuit and call onSuccess when the device is
unsecured (matching the biometric unsecured branch) — choose one approach and
update promptPinAuthentication, its callers, and HiddenFragment result handling
accordingly.
In `@app/src/main/res/values/strings.xml`:
- Around line 389-390: The strings.xml entry for <string name="unhidden"> is
incorrectly set to the same value as <string name="hidden">, causing toggle
actions in MainActivity.kt and Preference.kt to both display "Hidden"; update
the <string name="unhidden"> resource to a distinct English label (e.g.,
"Unhidden" or "Show") so the hide vs unhide actions display correctly, then
rebuild to verify MainActivity.kt and Preference.kt use the corrected resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2864abd7-2575-4759-b24f-07c92a9bcbbc
📒 Files selected for processing (14)
app/src/main/java/com/philkes/notallyx/data/model/Folder.ktapp/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/SearchFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.ktapp/src/main/java/com/philkes/notallyx/utils/security/LockUtils.ktapp/src/main/res/drawable/hidden.xmlapp/src/main/res/drawable/unhidden.xmlapp/src/main/res/navigation/navigation.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values/strings.xml
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/SearchFragment.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt (1)
209-248:⚠️ Potential issue | 🟠 MajorNullable launcher silently fails to authenticate.
When
activityResultLauncherisnulland the device is secured (lines 224-228, 240-244), the intent is not launched butonFailureis also not called—the function returns silently. This enables the security bypass inshowBiometricOrPinPromptHiddenwhereonSuccessis called immediately after.Consider calling
onFailure(null)when the launcher is null but intent creation succeeds, to signal that authentication couldn't be performed:Proposed fix
if (intent != null) { - activityResultLauncher?.launch(intent) + if (activityResultLauncher != null) { + activityResultLauncher.launch(intent) + } else { + onFailure.invoke(null) + } } else { onFailure.invoke(null) }Apply this pattern to both occurrences (lines 224-228 and 240-244).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt` around lines 209 - 248, In promptPinAuthentication, handle the case where activityResultLauncher is null by invoking onFailure(null) whenever an intent is created but cannot be launched; update both branches (the Build.VERSION.SDK_INT >= Build.VERSION_CODES.M path and the else path that checks isKeyguardSecure) so that after creating the intent you call activityResultLauncher?.launch(intent) if non-null, otherwise call onFailure(null) instead of returning silently—this prevents showBiometricOrPinPromptHidden from treating a null launcher as a successful authentication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt`:
- Around line 125-137: The fallback branch in LockUtils.kt must not reference
android.hardware.biometrics.BiometricManager on API 21-22 and must not call
onSuccess.invoke() before authentication completes; replace the
BIOMETRIC_SUCCESS check with a KeyguardManager.isKeyguardSecure() check for API
<23 (use
fragment.requireContext().getSystemService(KeyguardManager::class.java)) and
only proceed to authentication if the device is secure. Ensure
promptPinAuthentication is invoked with a non-null ActivityResultLauncher (or
add an ActivityResultLauncher parameter to the function) and remove the
immediate onSuccess.invoke() call; instead call onSuccess only after the
PIN/password result handler confirms successful authentication (handle result in
HiddenFragment or via the passed launcher).
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt`:
- Around line 209-248: In promptPinAuthentication, handle the case where
activityResultLauncher is null by invoking onFailure(null) whenever an intent is
created but cannot be launched; update both branches (the Build.VERSION.SDK_INT
>= Build.VERSION_CODES.M path and the else path that checks isKeyguardSecure) so
that after creating the intent you call activityResultLauncher?.launch(intent)
if non-null, otherwise call onFailure(null) instead of returning silently—this
prevents showBiometricOrPinPromptHidden from treating a null launcher as a
successful authentication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53c618c4-dfd8-4a18-b854-3d29fa3d1e39
📒 Files selected for processing (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt
| else -> { | ||
| if ( | ||
| fragment.requireContext().canAuthenticateWithBiometrics() != | ||
| android.hardware.biometrics.BiometricManager.BIOMETRIC_SUCCESS | ||
| ) { | ||
| onSuccess.invoke() | ||
| return | ||
| } | ||
| // API 21-22: No biometric support, fallback to PIN/Password | ||
| promptPinAuthentication(fragment.requireContext(), null, titleResId, onFailure) | ||
| onSuccess.invoke() | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Crash on API 21-22 and security bypass.
Two severe issues in this fallback branch:
-
Line 128 - NoClassDefFoundError crash:
android.hardware.biometrics.BiometricManagerwas added in API 29, but thiselsebranch executes on API < 23 (21-22). Referencing this class will crash the app. -
Lines 134-135 - Security bypass:
promptPinAuthenticationis called with anulllauncher (so the PIN intent is never actually launched), and thenonSuccess.invoke()is called immediately—granting access without any authentication.
Proposed fix
For API 21-22, use KeyguardManager.isKeyguardSecure to check device security (it's available since API 16), and either require a non-null ActivityResultLauncher to properly handle the PIN result, or allow access if the device has no lock:
else -> {
- if (
- fragment.requireContext().canAuthenticateWithBiometrics() !=
- android.hardware.biometrics.BiometricManager.BIOMETRIC_SUCCESS
- ) {
+ val keyguardManager = ContextCompat.getSystemService(
+ fragment.requireContext(),
+ KeyguardManager::class.java
+ )
+ if (keyguardManager?.isKeyguardSecure != true) {
+ // Device has no lock screen - allow access
onSuccess.invoke()
return
}
- // API 21-22: No biometric support, fallback to PIN/Password
- promptPinAuthentication(fragment.requireContext(), null, titleResId, onFailure)
- onSuccess.invoke()
+ // Device is secured but we can't properly handle PIN auth without a launcher
+ // Either require an ActivityResultLauncher parameter, or deny access
+ onFailure.invoke(null)
}Alternatively, add an ActivityResultLauncher parameter to this function and handle the PIN authentication result properly in HiddenFragment.
📝 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.
| else -> { | |
| if ( | |
| fragment.requireContext().canAuthenticateWithBiometrics() != | |
| android.hardware.biometrics.BiometricManager.BIOMETRIC_SUCCESS | |
| ) { | |
| onSuccess.invoke() | |
| return | |
| } | |
| // API 21-22: No biometric support, fallback to PIN/Password | |
| promptPinAuthentication(fragment.requireContext(), null, titleResId, onFailure) | |
| onSuccess.invoke() | |
| } | |
| } | |
| else -> { | |
| val keyguardManager = ContextCompat.getSystemService( | |
| fragment.requireContext(), | |
| KeyguardManager::class.java | |
| ) | |
| if (keyguardManager?.isKeyguardSecure != true) { | |
| // Device has no lock screen - allow access | |
| onSuccess.invoke() | |
| return | |
| } | |
| // Device is secured but we can't properly handle PIN auth without a launcher | |
| // Either require an ActivityResultLauncher parameter, or deny access | |
| onFailure.invoke(null) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/philkes/notallyx/utils/security/LockUtils.kt` around
lines 125 - 137, The fallback branch in LockUtils.kt must not reference
android.hardware.biometrics.BiometricManager on API 21-22 and must not call
onSuccess.invoke() before authentication completes; replace the
BIOMETRIC_SUCCESS check with a KeyguardManager.isKeyguardSecure() check for API
<23 (use
fragment.requireContext().getSystemService(KeyguardManager::class.java)) and
only proceed to authentication if the device is secure. Ensure
promptPinAuthentication is invoked with a non-null ActivityResultLauncher (or
add an ActivityResultLauncher parameter to the function) and remove the
immediate onSuccess.invoke() call; instead call onSuccess only after the
PIN/password result handler confirms successful authentication (handle result in
HiddenFragment or via the passed launcher).
|
Hi there, thanks for working on a hidden notes folder feature 🙂 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt`:
- Around line 26-29: The root view is being hidden too late (hide() is called in
onStart), causing a brief pre-auth flash because adapters/observers in
NotallyFragment initialize in onViewCreated; move the initial hide to the very
start of onViewCreated (or set the root's initial visibility in its layout) so
the UI is hidden before adapters bind, then keep the existing logic in
showBiometricOrPinPromptHidden to reveal the view after successful auth; update
references around onViewCreated in HiddenFragment (and/or the NotallyFragment
layout) to ensure the root is initially GONE/INVISIBLE and only toggled by
hide() and the post-auth reveal path.
- Around line 29-34: The showBiometricOrPinPromptHidden flow incorrectly invokes
onSuccess before confirming authentication in multiple branches; update
LockUtils.showBiometricOrPinPromptHidden so that (1) the canAuthenticate failure
path (currently calling onSuccess when Android M+ has no authenticator) instead
triggers onFailure or starts a verified fallback, (2) the pre-M
biometric-unavailable branch does not call onSuccess but routes to the
PIN/password flow or onFailure, and (3) the promptPinAuthentication path waits
for the actual PIN/password verification result instead of calling onSuccess
immediately — refactor promptPinAuthentication to accept a completion callback
(success/failure) and only call onSuccess when that callback reports success,
using the same verified-callback pattern for biometric and PIN paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bec3169a-0c04-4395-9a1b-a462c94d8b7c
📒 Files selected for processing (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt
| override fun onStart() { | ||
| super.onStart() | ||
| hide() | ||
| showBiometricOrPinPromptHidden( |
There was a problem hiding this comment.
Hide the view earlier to avoid pre-auth flash of hidden content.
hide() runs in onStart (Line 28), but observers/adapters are initialized in onViewCreated of NotallyFragment, so hidden notes can briefly render before auth. Hide the root at the beginning of onViewCreated (or via initial layout state) and reveal only after successful auth.
Suggested patch
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
- super.onViewCreated(view, savedInstanceState)
+ view.visibility = INVISIBLE
model.folder.value = Folder.HIDDEN
+ super.onViewCreated(view, savedInstanceState)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt`
around lines 26 - 29, The root view is being hidden too late (hide() is called
in onStart), causing a brief pre-auth flash because adapters/observers in
NotallyFragment initialize in onViewCreated; move the initial hide to the very
start of onViewCreated (or set the root's initial visibility in its layout) so
the UI is hidden before adapters bind, then keep the existing logic in
showBiometricOrPinPromptHidden to reveal the view after successful auth; update
references around onViewCreated in HiddenFragment (and/or the NotallyFragment
layout) to ensure the root is initially GONE/INVISIBLE and only toggled by
hide() and the post-auth reveal path.
| showBiometricOrPinPromptHidden( | ||
| this, | ||
| R.string.hidden_lock_title, | ||
| onSuccess = { show() }, | ||
| onFailure = { findNavController().popBackStack() }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify bypass-prone paths in LockUtils helper used by HiddenFragment.
# Expected: Identify branches where onSuccess.invoke() executes without successful authentication callback.
fd -i "LockUtils.kt" --exec nl -ba {} | sed -n '60,150p'
echo "----"
rg -n --type=kt -C3 'showBiometricOrPinPromptHidden|canAuthenticate\(|promptPinAuthentication\(|onSuccess\.invoke\('Repository: Crustack/NotallyX
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Locate the key files
echo "=== Finding LockUtils.kt ==="
fd "LockUtils.kt" -t f
echo "=== Finding HiddenFragment.kt ==="
fd "HiddenFragment.kt" -t f
echo "=== Finding NotallyFragment.kt ==="
fd "NotallyFragment.kt" -t fRepository: Crustack/NotallyX
Length of output: 412
🏁 Script executed:
#!/bin/bash
# Once we locate the files, examine the LockUtils.kt showBiometricOrPinPromptHidden function
# Search for the function and surrounding code
echo "=== Searching for showBiometricOrPinPromptHidden function ==="
rg -n "showBiometricOrPinPromptHidden" --type-add 'kotlin:*.kt' -t kotlin -A 80Repository: Crustack/NotallyX
Length of output: 13097
🏁 Script executed:
#!/bin/bash
# Search for canAuthenticate and promptPinAuthentication patterns
echo "=== Searching for authentication helper invocations ==="
rg -n "canAuthenticate|promptPinAuthentication|onSuccess\.invoke" --type-add 'kotlin:*.kt' -t kotlin -B 2 -A 2Repository: Crustack/NotallyX
Length of output: 10380
Hidden-note unlock bypassed on multiple authentication paths.
In showBiometricOrPinPromptHidden (LockUtils.kt:60–138), onSuccess is invoked without confirmed authentication in three scenarios:
- Line 76: Android M+ with no available authenticator (canAuthenticate fails)
- Line 130: Pre-M with no biometric capability
- Line 135: Pre-M with available PIN/password prompt—onSuccess is called immediately after
promptPinAuthentication()launches, not after the user authenticates
Line 135 is the most critical: the PIN/password prompt displays but onSuccess executes unconditionally before the user completes authentication, allowing hidden notes to be exposed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/HiddenFragment.kt`
around lines 29 - 34, The showBiometricOrPinPromptHidden flow incorrectly
invokes onSuccess before confirming authentication in multiple branches; update
LockUtils.showBiometricOrPinPromptHidden so that (1) the canAuthenticate failure
path (currently calling onSuccess when Android M+ has no authenticator) instead
triggers onFailure or starts a verified fallback, (2) the pre-M
biometric-unavailable branch does not call onSuccess but routes to the
PIN/password flow or onFailure, and (3) the promptPinAuthentication path waits
for the actual PIN/password verification result instead of calling onSuccess
immediately — refactor promptPinAuthentication to accept a completion callback
(success/failure) and only call onSuccess when that callback reports success,
using the same verified-callback pattern for biometric and PIN paths.
Relates to #612, #635, #771
If the screen lock is enabled, they ca…n only be accessed via the unlock method.
Summary by CodeRabbit
New Features
UI / Localization