Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughProgress and export now report both note and attachment counts: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM as BaseNoteModel
participant Export as ContextWrapper.exportAsZip
participant ProgressBus as LiveData/Progress
participant UI as ProgressDialog / Context
User->>VM: trigger exportBackup()
VM->>Export: exportAsZip(...)
Export->>ProgressBus: post BackupProgress(current,total,countSuffix)
ProgressBus->>UI: show/update progress dialog
Export-->>VM: return NotesAndAttachments (notes, attachments)
VM->>UI: application.exportedText(notes, attachments) -> show toast
UI->>User: display completion toast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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 |
Crustack
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/backup/ExportExtensions.kt (1)
131-133:⚠️ Potential issue | 🟡 MinorLog message will now display confusing output due to changed return type.
Since
exportAsZipnow returnsPair<Int, Int>instead ofInt, line 133's log message will print something like"Exported (150, 200) notes"instead of the intended"Exported 150 notes". Consider updating the log to destructure the pair.🐛 Proposed fix for clearer logging
- val exportedNotes = + val (exportedNotes, exportedAttachments) = app.exportAsZip(zipUri, password = preferences.backupPassword.value) - log(TAG, msg = "Exported $exportedNotes notes") + log(TAG, msg = "Exported $exportedNotes notes, $exportedAttachments attachments")🤖 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/backup/ExportExtensions.kt` around lines 131 - 133, exportAsZip now returns a Pair<Int, Int>, so the existing log("Exported $exportedNotes notes") will print the pair instead of the exported count; update the call site in ExportExtensions.kt to destructure the result from exportAsZip (e.g. val (exportedCount, total) = app.exportAsZip(zipUri, password = preferences.backupPassword.value)) and then call log(TAG, msg = "Exported $exportedCount notes") (or include both values in a clearer message) so the logged output shows the actual exported note count rather than the Pair representation.
🧹 Nitpick comments (1)
app/src/main/res/values/strings.xml (1)
278-281: Potential resource name conflict with existing<string name="notes">.Line 277 has
<string name="notes">Notes</string>and lines 278-281 add<plurals name="notes">. While Android allows a string and plurals to share the same name (they're accessed via different methods:getString()vsgetQuantityString()), this can cause confusion during maintenance. Consider renaming the plurals to something likenotes_countor leaving as-is with a comment clarifying the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values/strings.xml` around lines 278 - 281, The plurals entry uses the same resource name "notes" as the existing string resource "notes", which is legal but confusing; rename the plurals resource to a distinct identifier (e.g., change plurals name="notes" to name="notes_count" or similar) and update any calls using getQuantityString(...) to use the new name, or alternatively leave the name and add a clear comment above the plurals to document the deliberate difference between the string resource "notes" and the plurals resource "notes".
🤖 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/UiExtensions.kt`:
- Around line 1192-1193: The exportedText function currently uses a manual
conditional `if (notesAndAttachments.first < 2)` causing wrong grammar for 0
notes; replace that conditional with Android's plural helper by calling
getQuantityString for the note plural (e.g., use
getQuantityString(R.plurals.note, notesAndAttachments.first)) and build the
string using that result (keep attachments using
getQuantityString(R.plurals.attachments, notesAndAttachments.second)); update
the function exportedText to use getQuantityString for the notes count instead
of the `< 2` check.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.kt`:
- Around line 131-133: exportAsZip now returns a Pair<Int, Int>, so the existing
log("Exported $exportedNotes notes") will print the pair instead of the exported
count; update the call site in ExportExtensions.kt to destructure the result
from exportAsZip (e.g. val (exportedCount, total) = app.exportAsZip(zipUri,
password = preferences.backupPassword.value)) and then call log(TAG, msg =
"Exported $exportedCount notes") (or include both values in a clearer message)
so the logged output shows the actual exported note count rather than the Pair
representation.
---
Nitpick comments:
In `@app/src/main/res/values/strings.xml`:
- Around line 278-281: The plurals entry uses the same resource name "notes" as
the existing string resource "notes", which is legal but confusing; rename the
plurals resource to a distinct identifier (e.g., change plurals name="notes" to
name="notes_count" or similar) and update any calls using getQuantityString(...)
to use the new name, or alternatively leave the name and add a clear comment
above the plurals to document the deliberate difference between the string
resource "notes" and the plurals resource "notes".
🪄 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: aa7a01f2-9d6f-44eb-9031-12027d45ea47
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (7)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/view/misc/Progress.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/progress/BackupProgress.ktapp/src/main/java/com/philkes/notallyx/utils/ErrorActivity.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ExportExtensions.ktapp/src/main/res/values/strings.xml
Comment on lines
+1192
to
+1193
| fun Context.exportedText(notesAndAttachments: NotesAndAttachments) = | ||
| "${getString(R.string.exported)} ${notesAndAttachments.first} ${if(notesAndAttachments.first < 2) getString(R.string.note) else getString(R.string.notes)} (${notesAndAttachments.second} ${getQuantityString(R.plurals.attachments, notesAndAttachments.second)})" |
Contributor
There was a problem hiding this comment.
Incorrect pluralization logic for notes count.
The condition notesAndAttachments.first < 2 produces incorrect grammar when there are 0 notes (would display "0 Note" instead of "0 Notes"). For consistency with how attachments are handled, use getQuantityString for notes as well.
🐛 Proposed fix using getQuantityString for notes
fun Context.exportedText(notesAndAttachments: NotesAndAttachments) =
- "${getString(R.string.exported)} ${notesAndAttachments.first} ${if(notesAndAttachments.first < 2) getString(R.string.note) else getString(R.string.notes)} (${notesAndAttachments.second} ${getQuantityString(R.plurals.attachments, notesAndAttachments.second)})"
+ "${getString(R.string.exported)} ${notesAndAttachments.first} ${getQuantityString(R.plurals.notes, notesAndAttachments.first)} (${notesAndAttachments.second} ${getQuantityString(R.plurals.attachments, notesAndAttachments.second)})"🤖 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/UiExtensions.kt` around
lines 1192 - 1193, The exportedText function currently uses a manual conditional
`if (notesAndAttachments.first < 2)` causing wrong grammar for 0 notes; replace
that conditional with Android's plural helper by calling getQuantityString for
the note plural (e.g., use getQuantityString(R.plurals.note,
notesAndAttachments.first)) and build the string using that result (keep
attachments using getQuantityString(R.plurals.attachments,
notesAndAttachments.second)); update the function exportedText to use
getQuantityString for the notes count instead of the `< 2` check.
245983f to
e0dcfe7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #970
Summary by CodeRabbit