Compress large text changes in ChangeHistory#720
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Zstd-based compression for text+spans with a 10_000-byte threshold, refactors EditTextState to store compressed payloads with Changes
Sequence Diagram(s)sequenceDiagram
participant UI as EditText (UI)
participant State as EditTextState
participant Compress as CompressUtility
participant History as ChangeHistory
UI->>State: update(value.getEditableText())
State->>State: extract text & spans, measure size
alt size > COMPRESSION_THRESHOLD
State->>Compress: compressTextAndSpans(text, spans)
Compress-->>State: compressed ByteArray
State-->>History: push(state with compressed payload)
else small payload
State-->>History: push(state with Editable)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Crustack
force-pushed
the
fix/performance
branch
from
9f27809 to
8aeac82
Compare
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt`:
- Around line 89-96: The when-block handling StyleSpan in
EditTextWithHistoryChange (around the code that checks span.style against
Typeface.BOLD and Typeface.ITALIC) currently ignores Typeface.BOLD_ITALIC;
update the logic to detect combined styles (either by checking span.style ==
Typeface.BOLD_ITALIC or by bitwise-testing flags, e.g., (span.style &
Typeface.BOLD) != 0 and (span.style & Typeface.ITALIC) != 0) and set
representation.bold = true and representation.italic = true accordingly so both
attributes are preserved for BOLD_ITALIC spans.
In `@app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt`:
- Around line 38-52: Wrap the body of decompressTextAndSpans in a try/catch that
handles ZstdException, JSONException and a general Exception so corrupted input
doesn't crash the app; on exception log the error (e.g., Log.e or existing
logger) with context mentioning "decompressTextAndSpans" and the caught
exception, and return a safe fallback such as Pair("", emptyList()) instead of
letting the exception bubble up—ensure you still reference the existing symbols
(decompressTextAndSpans, Zstd.decompressedSize/Zstd.decompress, JSONObject,
Converters.jsonToSpans) when updating the method.
♻️ Duplicate comments (1)
app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt (1)
16-16: Correct the misleading comments referencing GZIP.The comments on lines 16, 28, and 37 state "GZIP" but the implementation uses Zstd compression. This was flagged in a previous review.
📝 Suggested fix
/** * 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. + * - For EditText state (text + spans), we store a Zstd-compressed JSON as ByteArray. */- /** Compresses text and spans using GZIP compression into a ByteArray. */ + /** Compresses text and spans using Zstd compression into a ByteArray. */ fun compressTextAndSpans(text: String, spans: List<SpanRepresentation>): ByteArray {- /** Decompresses text and spans that were compressed with GZIP. */ + /** Decompresses text and spans that were compressed with Zstd. */ fun decompressTextAndSpans(compressedData: ByteArray): Pair<String, List<SpanRepresentation>> {Also applies to: 28-28, 37-37
🧹 Nitpick comments (4)
app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt (1)
43-46: Remove commented-out code.This dead code should be removed to improve maintainability.
🧹 Suggested fix
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)app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt (2)
55-58: Remove unnecessary cast.The
extractSpansFromEditablealready returnsList<SpanRepresentation>, so the cast is redundant.🧹 Suggested fix
if (text.length > CompressUtility.COMPRESSION_THRESHOLD) { // Extract spans from the Editable val spans = extractSpansFromEditable(text) // Compress text and spans together - CompressUtility.compressTextAndSpans( - text.toString(), - spans as List<SpanRepresentation>, - ) + CompressUtility.compressTextAndSpans(text.toString(), spans) } else {
41-42: Remove empty companion object.The empty
companion object {}serves no purpose.🧹 Suggested fix
class EditTextState(text: Editable, val cursorPos: Int) { - companion object {} - // Either Editable (for small text) or ByteArray (compressed, for large text and spans)app/src/test/kotlin/com/philkes/notallyx/utils/EditTextCompressionTest.kt (1)
31-45: Avoid hard-coding the compression prefix in tests.
This couples the test to the current wire format. Using equality/size checks keeps the test stable if the prefix or algorithm changes.♻️ Suggested test tweak
- // exact threshold should not be compressed - assertFalse(atThreshold.startsWith("GZ:")) + // exact threshold should not be compressed + assertEquals(base, atThreshold) assertEquals(base, CompressUtility.decompressIfNeeded(atThreshold)) val above = base + "y" val compressed = CompressUtility.compressIfNeeded(above) - // above threshold should be compressed and round-trip correctly - assertTrue(compressed.startsWith("GZ:")) + // above threshold should be compressed and round-trip correctly + assertTrue(compressed.length < above.length) val roundTrip = CompressUtility.decompressIfNeeded(compressed) assertEquals(above, roundTrip)
Crustack
force-pushed
the
fix/performance
branch
from
8aeac82 to
3e16854
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/philkes/notallyx/utils/CompressUtility.kt`:
- Around line 19-20: The comment and the constant COMPRESSION_THRESHOLD in
CompressUtility.kt disagree (comment says ~10KB but PR summary said 5KB); decide
which is correct and make them consistent by either setting
COMPRESSION_THRESHOLD to 5_000 if the intended threshold is 5KB or updating the
inline comment to reflect 10_000 (~10KB) if 10KB is intended; ensure the
constant name COMPRESSION_THRESHOLD and its explanatory comment are updated
together so documentation matches the actual value used by the compression
logic.
🧹 Nitpick comments (2)
app/src/main/java/com/philkes/notallyx/utils/changehistory/ChangeHistory.kt (1)
24-39: Minor simplification possible in eviction logic.The inner
if (changeStack.isNotEmpty())check on line 30 is redundant sincechangeStack.size >= maxSize(wheremaxSize >= 1) guarantees the stack is non-empty.♻️ Optional simplification
fun push(change: Change) { // Drop all redo entries after current pointer popRedos() // If full, evict the oldest entry and shift the pointer accordingly var newStackPointer = stackPointer.value if (changeStack.size >= maxSize) { - if (changeStack.isNotEmpty()) { - changeStack.removeAt(0) - // Shift pointer left because we removed the head - newStackPointer = (newStackPointer - 1).coerceAtLeast(-1) - } + changeStack.removeAt(0) + // Shift pointer left because we removed the head + newStackPointer = (newStackPointer - 1).coerceAtLeast(-1) } changeStack.add(change) stackPointer.value = newStackPointer + 1 Log.d(TAG, "addChange: $change") }app/src/main/java/com/philkes/notallyx/utils/changehistory/EditTextWithHistoryChange.kt (1)
41-43: Remove the empty companion object.It’s unused and can be dropped for clarity.
♻️ Suggested cleanup
class EditTextState(text: Editable, val cursorPos: Int) { - companion object {}
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.
Related issues:
This PR tries to improve ChangeHistory performance by using
zstdto compress large text changes (>10KB) in theChangeHistoryinstead of stacking up those huge stringsSummary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.