Skip to content

fix: Reset modified state when all changes are undone#1426

Open
dara-abijo-adfa wants to merge 3 commits into
stagefrom
ADFA-2324-undo-clears-file-modified-indicator
Open

fix: Reset modified state when all changes are undone#1426
dara-abijo-adfa wants to merge 3 commits into
stagefrom
ADFA-2324-undo-clears-file-modified-indicator

Conversation

@dara-abijo-adfa

@dara-abijo-adfa dara-abijo-adfa commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Jira ticket - ADFA-2324

When a file is modified, the file name on the tab shows the asterisk (*) sign to indicate there are unsaved changes. When these changes are undone, putting the file in its initial state, the asterisk sign is still present.
This PR fixes that issue, ensuring the asterisk sign is not displayed when all the file changes are undone.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 314a0df4-552f-403e-af90-ba26190eda34

📥 Commits

Reviewing files that changed from the base of the PR and between 14779a3 and 9ab1439.

📒 Files selected for processing (1)
  • editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt

📝 Walkthrough

Release Notes

  • Fixed: File modification indicator (* on the file tab) now correctly clears when undo operations restore the editor content back to the last saved/loaded state
  • Changed: Modified state tracking now uses a stored snapshot of content length + a computed 64-bit FNV-1a hash (instead of String.hashCode()), recalculated after edits to ensure the indicator matches actual unsaved changes
  • Changed: Tab UI updates are now performed only when the modified/unmodified state actually changes, avoiding unnecessary UI refreshes
  • Changed: Global modified status (areFilesModified) is recomputed via hasUnsavedFiles() rather than being forced to true on every document change
  • Improved: Reduced overhead by computing the hash directly from the CharSequence (avoiding text.toString() allocations during modified-state refresh)

Risks / Best-practice notes

  • Hash collision risk ⚠️ Low: Content modification detection still relies on hashing (not cryptographic), though the move to 64-bit FNV-1a and the length check significantly reduce the chance of incorrect modified/unmodified classification for UI state tracking.

Walkthrough

IDEEditor gains two private snapshot fields (content length and hash) captured on markUnmodified(), plus snapshotSavedContent() and refreshModifiedState() methods that recompute isModified on every content change. EditorHandlerActivity.onDocumentChange is updated to read that flag and bidirectionally toggle the * tab prefix, and to derive areFilesModified from hasUnsavedFiles().

Changes

Undo-aware modified indicator

Layer / File(s) Summary
IDEEditor snapshot fields and modified state logic
editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt
Adds savedContentLength and savedContentHash fields; markUnmodified() now calls snapshotSavedContent() to capture the current content; new refreshModifiedState() compares live content length/hash against the snapshot to set isModified; content-change handler switched from markModified() to refreshModifiedState().
EditorHandlerActivity bidirectional tab indicator
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
onDocumentChange derives isModified from getEditorAtIndex(fileIndex)?.isModified, sets editorViewModel.areFilesModified = hasUnsavedFiles(), and conditionally adds or removes the * tab text prefix to match the current modification state.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • jomen-adfa

Poem

🐇 Hop, hop — I undo my tracks,
The asterisk fades, the saved state comes back!
A snapshot was taken when all was at rest,
Length and hash together, a modified test.
No more forever-starred files in the tree —
Undo your edits and pristine you'll be! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing the persistent asterisk indicator when all edits are undone, which is the core issue addressed in both modified files.
Description check ✅ Passed The description clearly explains the issue (asterisk persists after undo) and the fix provided, directly relating to the changeset modifications in both files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ADFA-2324-undo-clears-file-modified-indicator

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt`:
- Around line 145-150: The current implementation stores only savedContentLength
and savedContentHash to detect when edited content returns to its saved state,
creating a data-loss risk from hash collisions. Replace the savedContentHash
variable with a savedContentString variable that stores the actual saved content
as a String. Then in the refreshModifiedState() method (referenced in "Also
applies to: 693-703"), update the logic to perform an explicit content equality
check using .equals() when both the length and hash (computed from the stored
content) match, ensuring that hash collisions do not incorrectly mark files as
unmodified.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 193bcac3-81bc-4b39-be12-d8d6941fc52e

📥 Commits

Reviewing files that changed from the base of the PR and between d73ec0b and 14779a3.

📒 Files selected for processing (2)
  • app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
  • editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt

Comment thread editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt Outdated
Replaces the 32-bit `String.hashCode()` implementation with a custom 64-bit FNV-1a hash algorithm. This addresses a potential hash collision vulnerability where a file could be incorrectly marked as unmodified, leading to data loss.

Additionally, this resolves a performance bottleneck by computing the hash directly on the `CharSequence`, removing the heavy string allocation (`text.toString()`) that was previously happening every time the file modification state refreshed.
@hal-eisen-adfa

Copy link
Copy Markdown
Collaborator

Please be careful about possible collisions with PR #1416

}

private fun computeContentHash(content: CharSequence): Long {
var hash = -3750763034362895579L // FNV_offset_basis

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you extract these values into constants and document them?

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.

3 participants