Repository navigation
Conversation
External changes to a library file are merged into the open library without asking; the review dialog only appears for an item that was changed differently in memory and on disk. The autosave preference is relabeled accordingly, since both directions together keep library and file the same. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
A scan overtaken by a newer file change discards its result, and changes are sorted on the FX thread right before they are applied, so no edit can slip in between. Entries whose citation key changed on disk are paired with their in-memory entry, encoding changes count as metadata changes, and switching synchronization on for an open, unmodified library takes effect immediately. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
PR Summary by QodoAutomatically merge external changes into open local libraries
AI Description
Diagram
High-Level Assessment
Files changed (35)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
jabref-machine
added
the
status: changes-required
# Conflicts: # docs/requirements/ux.md
A save, switching synchronization off, or closing the tab now discards the result of a scan still running. Baseline entries found for disk entries are kept under the id of the in-memory entry they came from, and among entries sharing a citation key the one with identical content is preferred. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
jabref-machine
added
status: ready-for-review
| /// Changes the entry in place rather than replacing it, so it keeps its identity: table position, selection, and | ||
| /// an open entry editor stay as they are. |
Sync clients and editors may write the file in several steps; a half-written library parses fine and would look like every later entry had been deleted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt
| public void applyChange(CompoundEdit undoEdit) { | ||
| undoEdit.applyEdit(new UndoableRemoveEntries(databaseContext.getDatabase(), oldEntry)); | ||
| undoEdit.applyEdit(new UndoableInsertEntries(databaseContext.getDatabase(), newEntry)); | ||
| if (!oldEntry.getType().equals(newEntry.getType())) { |
There was a problem hiding this comment.
Also use Objects.equals for null safety?
There was a problem hiding this comment.
🤖 Generated with Claude Code
Done in 5d60bd3.
| LOGGER.error("Error while trying to monitor {}", path, e); | ||
| } | ||
| if (database.getLocation() == DatabaseLocation.LOCAL) { | ||
| preferences.getLibraryPreferences().autoSaveProperty().addListener(synchronizingListener); |
There was a problem hiding this comment.
Is it possible that we add the listener multiple times (e.g. this code get exeucted multiple times)
There was a problem hiding this comment.
🤖 Generated with Claude Code
It runs once per monitor instance (constructor), and a tab replacing its monitor calls unregister() on the old one first, which removes the listener again, so the preference does not accumulate listeners. Added a comment saying so (5d60bd3).
There was a problem hiding this comment.
🤖 Generated with Claude Code
Follow-up on the unregister side: unregister() (called on tab close and when the tab replaces its monitor) removes the listener, but did so only while the library was still local. A library converted to a shared one in the meantime would have kept it. Now removed unconditionally (0d21f34).
| // A group change is always accompanied by the metadata change it is part of, which precedes it in the list | ||
| Side metaDataSide = Side.BOTH; | ||
| for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory)) { | ||
| Side side = switch (change) { |
There was a problem hiding this comment.
I wonder if we should put that into a new method. Together with the for loop, this gets complex quick.
There was a problem hiding this comment.
🤖 Generated with Claude Code
Done in 5d60bd3: the entry-change arm is now classifyEntryChange, returning side and (possibly merged) change; the switch arm is three lines.
github-actions
Bot
added
status: changes-required
| if (generation != scanGeneration) { | ||
| LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); | ||
| return; | ||
| } | ||
| // Sorting the changes on the FX thread right before applying them leaves no window | ||
| // for a user edit to slip in between classification and application | ||
| synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); | ||
| }) |
There was a problem hiding this comment.
big lambda, maybe a new method instead?
There was a problem hiding this comment.
🤖 Generated with Claude Code
Done in 5d60bd3: extracted to onScannedForSynchronization.
Collecting them in a CompoundEdit named after the change lets the warning logged by a failing ChangeSet name the entry, not just the enclosing merge. Undo granularity is unchanged: nested sets are folded into the one step the caller opens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P4cHLmHHsmaTJmBP6JnZat
There was a problem hiding this comment.
There might be scaling problems. Claude says
Details
- find() is a linear scan, called per added entry — on the FX thread
LibraryBaseline.java:246 streams the entire entriesById map for every lookup:
List<Map.Entry<String, BibEntry>> candidates = entriesById.entrySet().stream()
.filter(entry -> entry.getValue().getCitationKey().filter(key::equals).isPresent())
.toList();
...
return byKey.or(() -> entriesById.entrySet().stream()
.filter(entry -> sameContentExceptKey(entry.getValue(), remote))
.findFirst());It's called once per EntryAdd in triage (:110), again per EntryAdd in pairSplitEntries (:196), and again in keepUnresolved (:160). So it's O(A · N) in adds × library size.
The fallback path makes it worse: sameContentExceptKey (:257) copies both field maps into fresh HashMaps on every single comparison, just to drop one key. For 5 000 added entries against a 100 000-entry library that's 5·10⁸ comparisons and 10⁹ throwaway HashMaps.
And this is not on the background thread. scanForChanges (DatabaseChangeMonitor.java:252) deliberately moves triage into onSuccess:
// Sorting the changes on the FX thread right before applying them leaves no window
// for a user edit to slip in between classification and application
synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes));The reasoning for that placement is sound, freezes the UI. Is a large A realistic?Yes: a Dropbox/Nextcloud client replacing r's bulk import, or — worst — a formatting
pass that changes citation keys, which droimilarity threshold and turns them into
add/delete pairs. That last one is exactlyists for, so the code's own worst case is
the one that triggers the quadratic path.
Fix: build the index once per triage inste Map<String, List> by citation key,plus a Map<contentHash, Entry> for the keyless fallback. That turns O(A·N) into O(N + A). And rewrite sameContentExceptKey to compare in place (oring KEY_FIELD, then iterate) instead of copying two maps.
- pairSplitEntries is quadratic in the change LibraryBaseline.java:198:
paired.remove(entryDelete);
paired.set(paired.indexOf(entryAdd), new EntryChange(...)); ArrayList.remove(Object) and indexOf are both O(C), inside a loop over all changes → O(A · C). In the key-change scenario C ≈ 2N, so this is quadratic in lendent of the find() cost.
Fix: one pass building a new list, with ange, DatabaseChange> of replacements and a set of deletes to skip. O(C).
- The baseline deep-copies every entry, a
LibraryBaseline.of (:73) does new BibEntry. The field strings are shared by reference, so the bibliographic data isn't duplicatedblem is the per-object overhead: every copy allocates a Guava EventBus, a ConcurrentHaeMap, fieldsAsWords, latexFreeFields, a MultiKeyMap, a SimpleObjectProperty and a java:106-163). Order of a kilobyte per entry before any content — call it ~100 MB for aong as the tab is open, and briefly doubled
while captureBaseline() builds the replaceopped (DatabaseChangeMonitor.java:302).
That's an estimate from the field list, no
It also re-runs on every save — resetChangWithDisk → captureBaseline (:192), andsynchronizing requires autosave, so that's every 31 s while editing. Autosave at least runs off the FX thread
(AutosaveUiManager is on the autosave execture at :302 is on the FX thread.
Fix: snapshot only what is actually read. needs just the type and the field map — arecord EntrySnapshot(EntryType type, Map<Field, String> fields) would cut the per-entry cost to one map. mergeFields does need real BibEntry objecthComputer, but that's the rare both-sidescase, so build them on demand there.
|
🤖 Generated with Claude Code |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| case MetadataChange _, | ||
| GroupChange _ -> | ||
| updated.keepMetaData(previous); |
There was a problem hiding this comment.
16. Mixed review decisions recur on next scan 🐞 Bug ≡ Correctness
keepUnresolved restores the entire previous metadata baseline when either a metadata change or its accompanying group change is rejected. Because groups occupy a key in that same serialized metadata, accepting one change and rejecting the other also restores the accepted item's old ancestor, so the next scan offers the settled decision for review again.
Agent Prompt
## Issue description
Metadata and groups are separate review choices but share one baseline snapshot. Rejecting either choice resets the accepted choice's ancestor, causing repeated reviews.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java[112-139]
- jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java[319-322]
## Recommended Fix
Preserve or advance the serialized group-tree key independently of other metadata keys according to each review decision. Test accepting a metadata edit while rejecting its accompanying group edit, and the reverse, across a subsequent scan.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
🤖 Generated with Claude Code
Fixed: metadata and groups advance their ancestors separately, per judged item. ff34175
| // Neither key nor content match: an entry deleted in memory may still be the origin, with key and a | ||
| // field changed on disk; then the deletion and the change need a review | ||
| List<String> goneFromMemory = entriesById.keySet().stream().filter(id -> !existsInMemory.test(id)).toList(); | ||
| return closestOf(goneFromMemory, remote).isPresent() ? Side.BOTH : Side.DISK; |
There was a problem hiding this comment.
17. Bulk file additions can freeze the table 🐞 Bug ➹ Performance
Lookup.sideOfAddedEntry scans every baseline entry to rebuild goneFromMemory for each unmatched added disk entry, even when no entry was deleted in memory. A large external import therefore performs work proportional to existing entries times additions inside synchronization's JavaFX success callback, delaying table updates and input.
Agent Prompt
## Issue description
Every new disk entry triggers a full scan of existing baseline entries during triage. Bulk external additions can therefore stall the JavaFX thread.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java[119-154]
- jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java[50-75]
## Recommended Fix
Compute baseline IDs absent from memory once per triage pass and reuse them for added-entry classification; skip closest-match work entirely when that collection is empty. Test or benchmark a large library receiving many new disk entries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
🤖 Generated with Claude Code
Fixed: entries gone from memory are collected once per pass; closest-match skipped when there are none. ff34175
|
Code review by qodo was updated up to the latest commit f1beda8 |
github-actions
Bot
added
the
status: changes-required
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
removed
the
status: changes-required
…chronizer Javadoc cross-links explaining where the three-way ancestor comes from in each case, and a changelog entry for the in-place entry change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
component: shared-database
component: git
component: diff-merge
A merged entry and every reviewed item get the disk version as ancestor, so an unsaved edit is neither reverted by the next file change nor forgotten by a review that did not cover it. Enabling synchronization on a modified library compares it with the file instead of trusting the undo journal. The unchanged mark is not restored over a later edit, and the entries gone from memory are collected once per pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sewhere The review covers the differences to the file only; accepting all of them does not make an unsaved edit outside the review disappear, so neither the clean mark nor a fresh baseline from memory is warranted then. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| keepLocalSettings(local.getMetaData(), metadataChange.getMetaDataDiff().getNewMetaData()); | ||
| metaDataSide = baseline.sideOfMetaData(local.getMetaData(), metadataChange.getMetaDataDiff().getNewMetaData()); | ||
| yield metaDataSide; | ||
| } | ||
| case GroupChange _ -> | ||
| metaDataSide; |
There was a problem hiding this comment.
8. Unrelated setting edits force a review 🐞 Bug ≡ Correctness
ChangeTriage.triage classifies the entire serialized metadata as one item and assigns that result to its accompanying group change. When memory changes one setting, such as encoding, and the file changes only the groups, both changes go to review even though they affect different items.
Agent Prompt
## Issue description
Independent in-memory and on-disk metadata edits are classified together, so unrelated external group changes require review.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java[88-94]
- jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java[246-248]
## Recommended Fix
Compare metadata items independently against their baseline values. Apply disk-only items without replacing memory-only settings, and send only genuinely conflicting items to review; cover a local encoding edit paired with a disk group edit.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (ChangeTriage.matchesFile(scanned, preferences.getCitationKeyPatternPreferences().getKeyPatterns())) { | ||
| captureIfCurrent(generation); | ||
| } else { | ||
| offerReview(generation, scanned); |
There was a problem hiding this comment.
9. Accepting a review can switch auto-merge off 🐞 Bug ≡ Correctness
scanToVerifyMatch sends the raw scanned changes to offerReview without going through ChangeTriage.triage, so keepLocalSettings never copies the in-memory sync setting into the parsed disk metadata. When the user accepts the MetadataChange, MetaData.overwriteWith (which now copies synchronizeWithFile) puts back the file's absent or false value; onSynchronizingChanged(false) then clears the baseline, and the review dialog never showed this difference.
Agent Prompt
## Issue description
The review offered after switching sync on for a modified library applies the file's metadata as-is. That replaces the user's unsaved per-library synchronizeWithFile value and disables synchronization again.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[258-271]
- jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java[205-209]
## Recommended Fix
Make keepLocalSettings accessible (package-private). In offerReview, before notifying listeners, call it for every MetadataChange in the list, passing database.getMetaData() and the change's getMetaDataDiff().getNewMetaData().
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Whether there is an undoable step, i.e. whether the library was edited through the undo manager | ||
| boolean canUndo(); |
There was a problem hiding this comment.
10. New undo method in shared interface is unused 🐞 Bug ⚙ Maintainability
canUndo() was added to the logic-layer UndoManager interface, but no production code calls it through that interface, because the switch-on check now uses libraryTab.isModified(). Every future implementation must still provide it, and its Javadoc suggests the undo stack shows whether the library matches its file, which the monitor's own comment says it does not.
Agent Prompt
## Issue description
`canUndo()` in the logic-layer UndoManager interface has no production caller through that interface.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/undo/UndoManager.java[64-65]
## Recommended Fix
Delete the declaration. GuiUndoManager already declares canUndo and JabRefUndoManager implements it, so existing callers and tests keep working.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
This should not be in the libs undomanager
|
Code review by qodo was updated up to the latest commit 792e502 |
github-actions
Bot
added
the
status: changes-required
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
status: changes-required
|
Stop messing around with undo. I had it fixed. Do not introduce new patterns and do not modify the interfaces. |
| import org.jspecify.annotations.NullMarked; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /// Records an entry's "changed since parsing" flag around an in-place edit, so that undoing the edit also restores |
There was a problem hiding this comment.
This is weird. Marker for changes on a file is the undo stack.
# Conflicts: # docs/requirements/ux.md
|
🤖 Generated with Claude Code Merged Conflicting file:
Generated by Claude Code |
github-actions
Bot
added
status: changes-required
This branch has not been deployed
5 participants
Summary
🤖 With "Automatically merge external changes" enabled (per library, or as the global default in Preferences → General → Saving), changes made to the library file by another program are merged into the open library automatically, including field-level merges of entries also edited in JabRef. The "External changes detected" review is only offered when the same item was changed differently in memory and on disk, or deleted on one side and changed on the other. Accepting an external change to an entry now edits the entry in place, so it keeps its table position, selection, and open entry editor.
Analogies: like honey, external edits now flow in smoothly instead of sticking in a dialog; like chocolate, the merge takes the best of both sides; like the moon, the review still shows up, but only when the two sides truly eclipse each other.
jabref-contrib-policy:4.2:reviewed:okSteps to test
Open a
.bibfile and set Library → Library properties → Saving → "Automatically merge external changes" to On (or enable the global default of the same name in Preferences → General → Saving; both are off by default).Open a
.bibfile in JabRef and edit it in a text editor: change a field of one entry, add a new entry, save.Observe: the table updates within a few seconds, a "Merged 2 change(s) from the library file" notification appears, no review dialog.
Change a field of an entry in JabRef, then change the same field to a different value in the text editor.
Observe: "External changes detected" with "Review changes", offering the usual merge dialog for that entry only.
Delete an entry in JabRef, then change that entry in the text editor: the review is offered; delete it in the text editor instead: nothing is asked.
Before: no such option; every external change was reported for review.
After (Preferences → General → Saving, global defaults):
After (Library properties → Saving, per library):
Related issues and pull requests
Closes #8431
Supersedes #16802, which was based on #11879 without needing it.
Supersedes JabRef#442. Implements the idea of #5669: a baseline of the file decides whether an external change is merged silently or offered for review.
The save side relies on autosave, which is broken on
mainand fixed in #16826. Follow-up in the same stack: #16827 (conflicted copies of sync clients).User documentation: JabRef/user-documentation#672
AI usage
Claude Code (model claude-fable-5-1), AIL4: implementation, tests, and manual GUI test driven by the AI, directed and reviewed by the author.
AI CHECKLIST.md walkthrough
1. Code self-review
Nullability and control flow
== null/!= nullchecks — absence of an item on one side of the three-way comparison is modelled asnull, as in the Git merge rules being reused.Objects.requireNonNull(...).@NullMarked.Optionalconsumed withifPresent/map/orElseThrow.StringUtil.isBlank(...).Exceptions
catch (Exception e).throw new RuntimeException(...).Style and idioms
BibEntryobjects built with withers.BackgroundTask.User-facing text
!; labels do not end with:.Security
Tests
LibraryBaselineTest).@DisplayName, no caught exceptions.2. Verification commands
:jabgui:test --tests org.jabref.gui.collab.*andLocalizationConsistencyTestpass.checkstyleMain checkstyleTest.modernizer.rewriteRunreports no changes.javadoc.markdownlint-cli2on the changed Markdown.3. Documentation
CHANGELOG.mdentry added.docs/requirements/ux.md.4. Pull request
gh pr create --body-file.TODOplaceholder.Checklist
CHANGELOG.mddescribing the change from the user's point of view (if the change is visible to the user)🤖 Generated with Claude Code