Keep external change detection active while saving - #16613
Open
Siedlerchr wants to merge 2 commits into
Open
Conversation
Siedlerchr
marked this pull request as ready for review
August 20, 2026 18:51
Contributor
PR Summary by QodoKeep external change detection active during library saves
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Contributor
Code Review by Qodo
1. SaveAs scan path race
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+143
to
+144
| // File watchers can report JabRef's own write or an external change while saving. Scanning once after the | ||
| // save finishes avoids reacting to an incomplete file and still detects an external write that occurred then. |
Contributor
There was a problem hiding this comment.
1. scanforchanges comment not /// 📘 Rule violation ⚙ Maintainability
A new multi-line comment in scanForChanges() uses // instead of the required Markdown Javadoc /// style. This diverges from the mandated documentation convention and increases style inconsistency in changed code.
Agent Prompt
## Issue description
New multi-line documentation-style comments must use Markdown Javadoc (`///`) and Markdown syntax.
## Issue Context
`scanForChanges()` introduces a two-line explanatory comment using `//`, which violates the project’s required multi-line comment convention.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[141-145]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+134
to
+138
| public void resumeChangeDetection() { | ||
| synchronized (database) { | ||
| changeDetectionSuspended = false; | ||
| scanForChanges(); | ||
| } |
Contributor
There was a problem hiding this comment.
2. Saveas scan path race 🐞 Bug ≡ Correctness
DatabaseChangeMonitor.resumeChangeDetection() schedules a scan that consults BibDatabaseContext.getDatabasePath() at execution time, but SaveDatabaseAction.saveAs(...) updates the context path only after save(file, mode) returns. This can scan the old/empty path during Save As, causing missed detection on the newly saved file or spurious notifications from scanning the previous file.
Agent Prompt
## Issue description
`resumeChangeDetection()` triggers `scanForChanges()` which eventually reads `database.getDatabasePath()` inside the background task. During `SaveDatabaseAction.saveAs(...)`, the new database path is only set *after* `save(file, mode)` (which resumes change detection in its `finally`). This creates a race where the post-save scan may run against the old (or empty) path.
## Issue Context
- `SaveDatabaseAction.saveAs(...)` calls `save(file, mode)` and only sets `context.setDatabasePath(file)` after a successful return.
- `save(Path, ...)` always calls `libraryTab.resumeChangeDetection()` in `finally`.
- `resumeChangeDetection()` immediately schedules a scan, and `ChangeScanner.scanForChanges()` uses `database.getDatabasePath()`.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[154-166]
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[212-252]
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[134-154]
- jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java[36-59]
## Suggested approach
1. Add an API that allows scanning the just-saved path explicitly (e.g., `DatabaseChangeMonitor.scanForChanges(Path fileToCompare)` or `resumeChangeDetection(Path savedPath)`), using `ChangeScanner.getDatabaseChanges(savedPath)` instead of relying on `database.getDatabasePath()`.
2. In `SaveDatabaseAction.save(Path targetPath, ...)`, call the new resume-and-scan method with `targetPath` in the `finally`.
3. Keep the existing no-arg resume for non-save callers (if any), but ensure Save As uses the path-aware version to remove the race.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
|
|
||
| libraryTab.suspendChangeMonitor(); | ||
| libraryTab.suspendChangeDetection(); |
Contributor
There was a problem hiding this comment.
3. Watcher thread can block 🐞 Bug ☼ Reliability
Because change detection is now suspended (not unregistered) during saves, filesystem callbacks still invoke DatabaseChangeMonitor.fileUpdated() on the WatchService thread and block on synchronized (database) while the save holds the same lock. This can stall DefaultFileUpdateMonitor’s single watcher loop, delaying or losing other filesystem events (including for other monitored files).
Agent Prompt
## Issue description
The WatchService thread calls `FileUpdateListener.fileUpdated()` inline. With this PR, the listener remains registered during saves and `fileUpdated()` synchronizes on the `BibDatabaseContext` object; the save path holds the same lock for the duration of writing. This blocks the WatchService thread, preventing timely processing of further filesystem events.
## Issue Context
- `DefaultFileUpdateMonitor.notifyAboutChange` directly calls listeners on the watcher thread.
- `DatabaseChangeMonitor.fileUpdated()` synchronizes on `database` before checking the suspended flag.
- `SaveDatabaseAction.saveDatabase(...)` synchronizes on the same `BibDatabaseContext` while writing.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java[39-67]
- jabgui/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java[82-84]
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[119-132]
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[255-290]
## Suggested approach
1. Make `changeDetectionSuspended` readable without taking the database monitor (e.g., `volatile` or `AtomicBoolean`).
2. In `fileUpdated()`, check the suspended flag *before* attempting any locking; if suspended, return immediately (or set a lightweight `pendingUpdate` flag) without blocking the WatchService thread.
3. If you still need mutual exclusion with saving, move any required locking into the background task (or use a separate lock dedicated to change-detection state) so the watcher thread never blocks on long-running saves.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+134
to
+138
| public void resumeChangeDetection() { | ||
| synchronized (database) { | ||
| changeDetectionSuspended = false; | ||
| scanForChanges(); | ||
| } |
Contributor
There was a problem hiding this comment.
4. Duplicate post-save scans 🐞 Bug ➹ Performance
resumeChangeDetection() always schedules a scan and queued filesystem events that were blocked during the save can run after resume and schedule additional scans, leading to redundant parsing and potentially duplicated external-change notifications. This is a performance regression on large libraries and can create noisy UX (multiple identical notifications).
Agent Prompt
## Issue description
After a save, `resumeChangeDetection()` schedules a scan unconditionally. Because watcher callbacks may be blocked during the save and delivered after resume, `fileUpdated()` can schedule an additional scan immediately after the resume-triggered scan is queued.
## Issue Context
- `resumeChangeDetection()` calls `scanForChanges()` every time.
- `fileUpdated()` calls `scanForChanges()` whenever not suspended.
- Save holds the same lock `fileUpdated()` uses, so events can be delayed until after resume.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[119-154]
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[225-252]
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[260-289]
## Suggested approach
1. Add a small coalescing mechanism in `DatabaseChangeMonitor`, e.g.:
- `AtomicBoolean scanScheduled`
- `AtomicBoolean pendingUpdateWhileSuspended`
2. In `fileUpdated()`:
- if suspended: set `pendingUpdateWhileSuspended=true` and return
- else: `scheduleScanIfNotScheduled()`
3. In `resumeChangeDetection()`:
- clear suspended
- only schedule a scan if `pendingUpdateWhileSuspended` was true (or if the save path explicitly requests one), and clear the pending flag
4. Clear `scanScheduled` in both `onSuccess` and `onFailure` callbacks so subsequent real updates can schedule scans.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Summary
JabRef now keeps its external-change watcher registered while a library is saved. Filesystem events received during the save are deferred, then the saved library is scanned once afterwards so persistent external changes can still be offered for conflict resolution.
Analogies
The monitor is like honey: it stays connected rather than being removed while the jar is opened. It waits like chocolate cooling before judging the final shape of a save. Once the save finishes, it checks the resulting file like the moon reflecting what is actually on disk.
This draft requires contributor review before adding the policy compliance tag required for a ready-for-review pull request.
Steps to test
Related issues and pull requests
No confident matching issue identified.
AI usage
Codex (GPT-5) assisted with implementation and tests from the contributor-requested design. AIL4: AI produced the implementation from a human-provided basic idea. The contributor must review, understand, and take full ownership before marking this PR ready for review.
AI CHECKLIST.md walkthrough
Code checklist
1. Code self-review
Nullability and control flow
== null/!= nullchecks — JSpecify annotations (@NullMarked,@Nullable,@NonNull) used instead.Objects.requireNonNull(...)— nullability expressed via JSpecify annotations.@NullMarked(org.jspecify.annotations.NullMarked).Optionalconsumed withifPresent/ifPresentOrElse/map/orElseThrow— neverorElse(unusedValue)nor anisPresent()+get()block.StringUtil.isBlank(...)used instead ofs == null || s.isBlank().Exceptions
catch (Exception e)— only specific exceptions are caught.throw new RuntimeException(...)/IllegalStateException(...)— these tear down the whole application.LOGGER.info("...", e)), not concatenated into the message string.Style and idioms
BibEntryobjects built with withers (withField, notsetField).List.of()/Map.of()/Set.of(),Path.of(),SequencedCollection/SequencedSet, text blocks.Pattern.compile(...)constant, notString.matches(...).org.jabref.logic.util.BackgroundTask, notnew Thread().///) uses Markdown syntax, not JavaDoc inline tags:`code`instead of{@code},[ClassName]instead of{@link}.User-facing text
Localization.langin Java,%prefix in FXML).!; labels do not end with:."...: %0"), not string concatenation.Security
text/htmlresponse — including exception/error messages, not just the success body (XSS).Tests
org.jabref.model/org.jabref.logichave added or updated tests.assertEquals), use plain JUnit asserts (not AssertJ), have no@DisplayName, do not catch exceptions (let them propagate so JUnit reports setup/teardown failures directly), and use@TempDirinstead of manual temp directories.2. Verification commands
./gradlew :jablib:check(or./gradlew checkfor all modules)../gradlew checkstyleMain checkstyleTest checkstyleJmh../gradlew modernizer../gradlew --no-configuration-cache :rewriteDryRunreports no changes (run./gradlew rewriteRunto fix)../gradlew javadoc.npx markdownlint-cli2 "docs/**/*.md" "*.md"(only if Markdown changed).rewriteRun:docker run -v $(pwd):/github/workspace ghcr.io/leventebajczi/intellij-format:master "*.java" "" ".idea/codeStyles/Project.xml".3. Documentation
CHANGELOG.mdentry added if the change is visible to the user (end-user wording, no extra blank lines). Link the issue if one exists; link the PR only when no issue exists. UseTODOas the placeholder when neither is known yet — never a fake number.TODO(nocloses/fixesfor merely-similar issues).docs/requirements/<area>.mdif the change is a new feature or significant bug fix (skip for refactors, minor fixes, and internal changes).docs/updated if behavior or architecture changed.4. Pull request
.github/PULL_REQUEST_TEMPLATE.md, every section filled.[x],[ ], or[/].gh pr create --body-file <file>(not--body).CHANGELOG.mdused aTODOplaceholder (no issue confidently identified yet — an existing issue link always stays), it was replaced with the real PR-number link after PR creation, then committed and pushed. If an issue is identified or created later, the link is switched to the issue.Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)