Compress large text changes in ChangeHistory #720
Conversation
WalkthroughThe PR introduces a Changes
Sequence DiagramsequenceDiagram
participant App as App Layer
participant Model as Data Model
participant DB as Database
participant Compress as CompressUtility
App->>Model: Set note body (String)
Model->>Model: Wrap in BodyString
Model->>DB: Store via Converter
DB->>Compress: fromBodyString(BodyString)
alt Body Length > Threshold
Compress->>Compress: Compress with GZIP+Base64
Note over Compress: Prefix "GZ:" marker
else Body Length <= Threshold
Note over Compress: Store as-is
end
Compress->>DB: Store compressed String
DB->>Compress: toBodyString(String)
alt Has "GZ:" Prefix
Compress->>Compress: Decompress Base64+GZIP
else No Prefix
Note over Compress: Use as-is
end
Compress->>Model: Return BodyString(decompressed)
Model->>App: Retrieve body.value for display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key areas requiring attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt (1)
44-44: Remove empty companion object.The empty companion object serves no purpose and triggers a static analysis warning.
Apply this diff:
- companion object {}app/build.gradle.kts (1)
272-272: Consider upgrading zstd-jni to version 1.5.7-4.The
@aarsuffix is correct for Android projects, but the current version1.5.6-7is outdated. The latest stable version is 1.5.7-4 (published July 2, 2025). Update to the latest version for bug fixes and improvements.app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt (1)
46-49: Remove commented-out code.Lines 46-49 contain dead GZIP code that's been replaced by Zstd decompression. Commented-out code should be removed to improve maintainability.
Apply this diff:
Zstd.decompress(result, compressedData) val jsonString = result.toString(Charsets.UTF_8) - // val bis = ByteArrayInputStream(compressedData) - // val jsonString = GZIPInputStream(bis).use { gzipIS -> - // gzipIS.readBytes().toString(Charsets.UTF_8) - // } val jsonObject = JSONObject(jsonString)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
app/build.gradle.kts(1 hunks)app/src/main/java/com/philkes/notallyx/data/imports/evernote/EvernoteImporter.kt(2 hunks)app/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.kt(2 hunks)app/src/main/java/com/philkes/notallyx/data/imports/markdown/MarkdownUtils.kt(0 hunks)app/src/main/java/com/philkes/notallyx/data/imports/txt/PlainTextImporter.kt(2 hunks)app/src/main/java/com/philkes/notallyx/data/model/BaseNote.kt(1 hunks)app/src/main/java/com/philkes/notallyx/data/model/BodyString.kt(1 hunks)app/src/main/java/com/philkes/notallyx/data/model/Converters.kt(1 hunks)app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt(3 hunks)app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt(3 hunks)app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt(2 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt(1 hunks)app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt(4 hunks)app/src/main/java/com/philkes/notallyx/presentation/widget/WidgetFactory.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt(2 hunks)app/src/main/java/com/philkes/notallyx/utils/backup/XmlParserExtensions.kt(2 hunks)app/src/main/java/com/philkes/notallyx/utils/changehistory/ChangeHistory.kt(1 hunks)app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt(3 hunks)app/src/test/java/com/philkes/notallyx/data/imports/markdown/MarkdownUtilsTest.kt(2 hunks)app/src/test/java/com/philkes/notallyx/data/model/MarkdownExportTest.kt(2 hunks)app/src/test/kotlin/com/philkes/notallyx/data/imports/google/GoogleKeepImporterTest.kt(2 hunks)app/src/test/kotlin/com/philkes/notallyx/data/model/ModelExtensionsTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- app/src/main/java/com/philkes/notallyx/data/imports/markdown/MarkdownUtils.kt
🧰 Additional context used
🧬 Code graph analysis (3)
app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.kt (1)
app/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableTextView.kt (1)
extractSearchSnippet(64-77)
app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt (1)
app/src/main/java/com/philkes/notallyx/data/imports/markdown/MarkdownUtils.kt (1)
createMarkdownFromBodyAndSpans(136-191)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (1)
app/src/main/java/com/philkes/notallyx/presentation/view/misc/EditTextWithWatcher.kt (1)
getTextClone(70-72)
🪛 detekt (1.23.8)
app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt
[warning] 44-44: The class or object Companion is empty.
(detekt.empty-blocks.EmptyClassBlock)
🔇 Additional comments (25)
app/src/test/kotlin/com/philkes/notallyx/data/imports/google/GoogleKeepImporterTest.kt (1)
5-5: LGTM! Test helper correctly updated for BodyString wrapper.The test helper now properly wraps the body parameter with
BodyString(body), aligning with the new BaseNote.body type.Also applies to: 250-250
app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt (3)
28-36: LGTM! Correctly retrieves editable text and strips highlight spans.The update logic properly obtains the current text via
getEditableText()and removes highlight spans before applying changes.
49-65: Verify compression threshold and consider PII in debug logs.The compression logic is well-structured, but debug logging at line 54 may expose sensitive note content. Consider removing or sanitizing the log output in production builds.
Additionally, ensure
CompressUtility.COMPRESSION_THRESHOLDis appropriately calibrated for the performance characteristics observed in the related issues (#703, #633, #593, #402).
68-120: No issues found. The SpanRepresentation.isNotUseless() method exists and is properly defined.The method is defined at line 14-16 in
app/src/main/java/com/philkes/notallyx/data/model/SpanRepresentation.ktand returnsBooleanby checking all relevant span properties. The usage at line 114 inEditTextWithHistoryChange.ktis valid.app/src/main/java/com/philkes/notallyx/utils/backup/XmlParserExtensions.kt (1)
4-4: LGTM! Correctly adapted for BodyString wrapper.The XML parser now properly wraps the body string with
BodyString(body)when constructing BaseNote instances.Also applies to: 111-111
app/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.kt (1)
14-14: LGTM! Correctly adapted for BodyString wrapper.The Google Keep importer now properly wraps the parsed body with
BodyString(body)when constructing BaseNote instances.Also applies to: 160-160
app/src/main/java/com/philkes/notallyx/presentation/widget/WidgetFactory.kt (1)
78-81: LGTM! Correctly adapted for BodyString wrapper.The widget now properly accesses the underlying string via
note.body.valuewhile maintaining the existing visibility logic.app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt (1)
82-84: LGTM! Correctly adapted for BodyString wrapper.The notification now properly accesses the underlying body string via
note.body.valuebefore truncating.app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt (1)
389-393: LGTM! Correctly adapted for BodyString wrapper.The share functionality now properly accesses the underlying body string via
note.body.valuefor NOTE types.app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.kt (1)
165-179: LGTM! Correct usage of BodyString wrapper.The changes properly unwrap
baseNote.body.valueto access the underlying String when passing toextractSearchSnippet()andbindNote(), which is consistent with the BodyString refactor introduced in this PR.app/src/main/java/com/philkes/notallyx/utils/changehistory/ChangeHistory.kt (1)
88-93: LGTM! Clean extension for typed change access.The extension function provides a safe, typed way to access the last change with appropriate null handling when undo is unavailable.
app/src/test/java/com/philkes/notallyx/data/imports/markdown/MarkdownUtilsTest.kt (2)
11-32: LGTM! Test updated for BodyString wrapper.The test helper correctly constructs
BaseNotewithBodyString(body), aligning with the new body type.
79-86: Test expectations updated for markdown parsing changes.The expected body now includes title as a header (
# Title\n) and preserves image markdown syntax. Verify these changes match the intended markdown parsing behavior described in the PR objectives.app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt (2)
46-46: Visibility change enables new usage patterns.The
changeHistoryvisibility change fromprivatetointernalallows access from other classes in the module, which appears to be intentional based on the newlastChangeAs()extension usage mentioned in the AI summary.
284-296: Updated to use getEditableText() for text retrieval.The change from
value.text.toString()tovalue.getEditableText().toString()aligns with the EditTextState refactor. Ensure this provides the correct editable text content.app/src/main/java/com/philkes/notallyx/data/imports/evernote/EvernoteImporter.kt (1)
127-162: LGTM! Importer updated for BodyString wrapper.The change properly wraps the parsed body string with
BodyString(body)when constructingBaseNote, consistent with the BodyString refactor across all importers.app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt (3)
106-146: LGTM! JSON deserialization updated for BodyString.Line 137 correctly wraps the deserialized body string with
BodyString(body)when constructing theBaseNotefrom JSON.
180-228: LGTM! HTML generation correctly unwraps BodyString.Line 196 properly accesses
body.valueto get the underlying String before applying spans and converting to HTML.
232-241: LGTM! Markdown generation correctly unwraps BodyString.Line 235 properly accesses
body.valueto pass the underlying String to the markdown creation function.app/src/main/java/com/philkes/notallyx/data/model/BaseNote.kt (1)
11-29: LGTM! Core BodyString wrapper introduced.The change from
StringtoBodyStringfor the body field is the central refactor that enables text compression for large bodies, as described in the PR objectives. The pattern has been consistently applied across the codebase.app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
164-167: No functional impact from disabling cache synchronization.The verification reveals that no active code depends on
Cache.listbeing synchronized. The observer that kept it synchronized is disabled (line 165), and the only code that reads fromCache.listis already commented out (line 231). The only active reference is an explicit clear operation at line 482, which does not depend on automatic synchronization.app/src/main/java/com/philkes/notallyx/data/model/BodyString.kt (1)
1-13: LGTM! Clean wrapper implementation.The value class provides a zero-overhead wrapper for note body text with convenient delegation methods. The use of
@JvmInlineensures no runtime boxing overhead while providing type safety.app/src/test/kotlin/com/philkes/notallyx/data/model/ModelExtensionsTest.kt (1)
108-108: LGTM! Test updated correctly.The test now uses
BodyStringwrapper as expected by the updatedBaseNoteAPI.app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
21-21: LGTM! Backup import updated correctly.The import flow now wraps the body string in
BodyStringwhen constructingBaseNote, consistent with the API change.Also applies to: 323-323
app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt (1)
24-24: Verify compression threshold aligns with PR requirements.The PR description mentions "5 KB" but the code uses
7_000characters. While the comment says "approximately 7KB," this discrepancy should be verified. Note that character count doesn't directly map to byte size—UTF-8 encoding means the actual size depends on character composition.Should the threshold be adjusted to match the PR description, or is the 7000-character threshold intentional?
| // Get state before from last change's after state (if it is EditTextChange | ||
| stateBefore = | ||
| changeHistory.lastChangeAs<EditTextWithHistoryChange>()?.newValue | ||
| ?: EditTextState(getTextClone(), selectionStart) |
There was a problem hiding this comment.
Undo cursor position regressed
When the user moves the caret (no text change) and then types again, we now reuse the previous change’s newValue, so stateBefore keeps the old selection instead of the current selectionStart. Undoing the new edit jumps the cursor back to the prior position, breaking the expected undo behaviour.
Please rebuild stateBefore with the current selection while still reusing the cached text snapshot. For example:
- stateBefore =
- changeHistory.lastChangeAs<EditTextWithHistoryChange>()?.newValue
- ?: EditTextState(getTextClone(), selectionStart)
+ val previous = changeHistory.lastChangeAs<EditTextWithHistoryChange>()?.newValue
+ stateBefore =
+ previous?.let { EditTextState(it.getEditableText(), selectionStart) }
+ ?: EditTextState(getTextClone(), selectionStart)This keeps the clone optimisation but ensures undo restores the caret to the position the user actually had before the change.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt around
lines 344-347, the current code reuses the previous change’s EditTextState
wholesale so stateBefore retains the old selection and causes undo to jump the
caret to a prior position; instead, build stateBefore by reusing the cached text
snapshot from lastChangeAs<EditTextWithHistoryChange>()?.newValue but replace
its selection with the current selectionStart (i.e., construct a new
EditTextState using the last change’s text / clone and the current
selectionStart), and fall back to EditTextState(getTextClone(), selectionStart)
when there is no cached change.
| fun getEditableText(): Editable { | ||
| return when (textContent) { | ||
| is Editable -> textContent | ||
| is ByteArray -> { | ||
| var pair: Pair<String, List<SpanRepresentation>>? | ||
| val time = measureTimeMillis { | ||
| pair = CompressUtility.decompressTextAndSpans(textContent) | ||
| } | ||
| Log.d("COMPRESS", "decompress took: $time ms") | ||
| val pairs = pair as Pair<String, List<SpanRepresentation>> | ||
| pairs.first.applySpans(pairs.second) | ||
| } | ||
| else -> SpannableStringBuilder() | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle decompression failure more safely.
The unsafe cast at line 132 and the empty SpannableStringBuilder fallback at line 135 could mask decompression errors. Consider explicit error handling and logging.
Apply this diff to improve error handling:
fun getEditableText(): Editable {
return when (textContent) {
is Editable -> textContent
is ByteArray -> {
- var pair: Pair<String, List<SpanRepresentation>>?
+ val pair: Pair<String, List<SpanRepresentation>>?
val time = measureTimeMillis {
pair = CompressUtility.decompressTextAndSpans(textContent)
}
Log.d("COMPRESS", "decompress took: $time ms")
- val pairs = pair as Pair<String, List<SpanRepresentation>>
- pairs.first.applySpans(pairs.second)
+ pair?.let { (text, spans) ->
+ text.applySpans(spans)
+ } ?: run {
+ Log.e("COMPRESS", "Decompression failed, returning empty text")
+ SpannableStringBuilder()
+ }
}
else -> SpannableStringBuilder()
}
}📝 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.
| fun getEditableText(): Editable { | |
| return when (textContent) { | |
| is Editable -> textContent | |
| is ByteArray -> { | |
| var pair: Pair<String, List<SpanRepresentation>>? | |
| val time = measureTimeMillis { | |
| pair = CompressUtility.decompressTextAndSpans(textContent) | |
| } | |
| Log.d("COMPRESS", "decompress took: $time ms") | |
| val pairs = pair as Pair<String, List<SpanRepresentation>> | |
| pairs.first.applySpans(pairs.second) | |
| } | |
| else -> SpannableStringBuilder() | |
| } | |
| } | |
| fun getEditableText(): Editable { | |
| return when (textContent) { | |
| is Editable -> textContent | |
| is ByteArray -> { | |
| var pair: Pair<String, List<SpanRepresentation>>? | |
| val time = measureTimeMillis { | |
| pair = CompressUtility.decompressTextAndSpans(textContent) | |
| } | |
| Log.d("COMPRESS", "decompress took: $time ms") | |
| pair?.let { (text, spans) -> | |
| text.applySpans(spans) | |
| } ?: run { | |
| Log.e("COMPRESS", "Decompression failed, returning empty text") | |
| SpannableStringBuilder() | |
| } | |
| } | |
| else -> SpannableStringBuilder() | |
| } | |
| } |
| @@ -0,0 +1,89 @@ | |||
| package com.philkes.notallyx.utils | |||
|
|
|||
| import android.provider.ContactsContract.CommonDataKinds.StructuredName.PREFIX | |||
There was a problem hiding this comment.
Remove unused import.
Line 3 imports PREFIX from ContactsContract.CommonDataKinds.StructuredName, but this constant is never used. The file defines its own PREFIX constant on line 27.
Apply this diff:
-import android.provider.ContactsContract.CommonDataKinds.StructuredName.PREFIX📝 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.
| import android.provider.ContactsContract.CommonDataKinds.StructuredName.PREFIX |
🤖 Prompt for AI Agents
In app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt around line
3, remove the unused import "import
android.provider.ContactsContract.CommonDataKinds.StructuredName.PREFIX" because
the file defines its own PREFIX constant at line 27; simply delete that import
line so there are no conflicting or unused imports left.
|
|
||
| /** | ||
| * Shared compression utilities for large text payloads to decrease memory and storage usage. | ||
| * - For EditText state (text + spans), we store a GZIP-compressed JSON as ByteArray. |
There was a problem hiding this comment.
Correct the misleading comment.
The comment states "GZIP-compressed JSON as ByteArray" but the implementation uses Zstd compression (line 37), not GZIP.
Apply this diff:
- * - For EditText state (text + spans), we store a GZIP-compressed JSON as ByteArray.
+ * - For EditText state (text + spans), we store a Zstd-compressed JSON as ByteArray.📝 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.
| * - For EditText state (text + spans), we store a GZIP-compressed JSON as ByteArray. | |
| * - For EditText state (text + spans), we store a Zstd-compressed JSON as ByteArray. |
🤖 Prompt for AI Agents
In app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt around line
18, the comment incorrectly says "GZIP-compressed JSON as ByteArray" while the
implementation uses Zstd compression; update the comment to accurately reflect
the compression type (e.g., "Zstd-compressed JSON as ByteArray") and adjust any
nearby comments or docs in this file that reference GZIP to Zstd to keep the
source consistent.
Related issues:
This PR tries to improve ChangeHistory performance by using
zstdto compress large text changes (>5KB) in theChangeHistoryinstead of stacking up those huge stringsSummary by CodeRabbit
New Features
Bug Fixes
Refactor