Fix not on fx thread cleanup by Siedlerchr · Pull Request #15835 · JabRef/jabref

Siedlerchr · 2026-05-26T20:26:08Z

Related issues and pull requests

Found while checking out #15833

PR Description

Tip: re-read your description before opening the pull request, then delete this line.

Steps to test

Have an entry with a pdf linked which is not yet in target dir
open bib entry in entry editor
Quality -> cleanup -> File related -> Rename pdf and move files

AI usage

chat gpt codex 5.3 for adding the tests

Checklist

I own the copyright of the code submitted and I license it under the MIT license
If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
I manually tested my changes in running JabRef (always required)
I added JUnit tests for changes (if applicable)
[/] I added screenshots in the PR description (if change is visible to the user)
[/] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
[/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

…ptiosn" This reverts commit 5594f85.

This reverts commit 51bffea.

qodo-free-for-open-source-projects · 2026-05-26T20:26:33Z

Review Summary by Qodo

Fix cleanup operations to use mutation scheduler for entry updates

🐞 Bug fix

Walkthroughs

Description

• Fix cleanup operations not using mutation scheduler for entry updates
• Ensure file I/O operations run on correct thread via scheduler
• Remove unused Optional utility imports and simplify code
• Add tests verifying mutation scheduler is used correctly

Diagram

flowchart LR
  A["Cleanup Operations<br/>MoveFilesCleanup<br/>RenamePdfCleanup"] -->|"Use mutationScheduler<br/>for entry.setFiles()"| B["Thread-safe<br/>Entry Updates"]
  C["File I/O Operations<br/>Background Thread"] -->|"Scheduled via<br/>mutationScheduler"| B
  B -->|"Prevents<br/>FX Thread Exceptions"| D["Fixed Issue #15833"]

File Changes

1. jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java 🐞 Bug fix +8/-10

Use mutation scheduler for entry file updates

• Removed unused Optional and OptionalUtil imports
• Consolidated two cleanup() methods into single method accepting mutationScheduler
• Changed entry.setFiles() call to execute via mutationScheduler to ensure thread safety
• Wrapped result collection in ArrayList and used ifPresent() to add changes

jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java

2. jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java 🐞 Bug fix +9/-10

Use mutation scheduler for entry file updates

• Removed unused Optional and OptionalUtil imports
• Added ArrayList import for result collection
• Consolidated two cleanup() methods into single method accepting mutationScheduler
• Changed entry.setFiles() call to execute via mutationScheduler for thread safety
• Wrapped result collection in ArrayList and used ifPresent() to add changes

jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java

3. jablib/src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java 🧪 Tests +14/-0

Add test for mutation scheduler usage

• Added AtomicBoolean import for test synchronization
• Added new test cleanupMoveFilesUsesMutationSchedulerForEntryUpdate() to verify scheduler is
 invoked
• Test confirms mutation scheduler is called when cleanup modifies entry files

jablib/src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java

View more (2)

4. jablib/src/test/java/org/jabref/logic/cleanup/RenamePdfCleanupTest.java 🧪 Tests +19/-0

Add test for mutation scheduler usage
• Added AtomicBoolean import for test synchronization
• Added new test cleanupRenamePdfUsesMutationSchedulerForEntryUpdate() to verify scheduler is
 invoked
• Test confirms mutation scheduler is called when cleanup renames PDF files
jablib/src/test/java/org/jabref/logic/cleanup/RenamePdfCleanupTest.java

5. CHANGELOG.md 📝 Documentation +1/-0

Document cleanup operations fix

• Added entry documenting fix for cleanup operations causing exceptions
• References issue #15833 regarding Quality > Cleanup > Rename PDF with file directory move

CHANGELOG.md

qodo-free-for-open-source-projects · 2026-05-26T20:26:34Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~Duplicated mutationScheduler change code~~ ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

Two cleanup jobs duplicate the same ArrayList + `mutationScheduler.accept(() ->
entry.setFiles(...))` pattern, increasing maintenance cost and the risk of inconsistent future
fixes. This violates the no-duplication requirement.

Code

jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[R53-56]

Evidence
PR Compliance ID 5 requires avoiding duplicate code blocks when the same logic can be reused. The
same scheduled-mutation snippet (create changes, schedule entry.setFiles(files).ifPresent(...),
return changes) is present in both cleanup implementations.
AGENTS.md: No Code Duplication and No Premature Abstractions: AGENTS.md: No Code Duplication and No Premature Abstractions: AGENTS.md: No Code Duplication and No Premature Abstractions: AGENTS.md: No Code Duplication and No Premature Abstractions
jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[53-56]
jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[59-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`MoveFilesCleanup` and `RenamePdfCleanup` both contain a duplicated snippet that (1) creates a mutable list, (2) schedules an entry mutation via `mutationScheduler`, and (3) returns the list. This should be centralized to avoid duplication and keep behavior consistent.
## Issue Context
Both classes are `CleanupJob` implementations that need to schedule `BibEntry` mutations through `mutationScheduler` while returning `List<FieldChange>`. The duplicated pattern can be replaced by a shared helper (e.g., a reusable utility method or a generic helper in `CleanupJob`) that runs a mutation via the scheduler and returns the produced `List<FieldChange>`.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[53-56]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[59-63]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~FX-thread file serialization~~ ✓ Resolved 🐞 Bug ➹ Performance

Description

MoveFilesCleanup and RenamePdfCleanup call BibEntry#setFiles(files) inside mutationScheduler, which
in the GUI is UiTaskExecutor::runAndWaitInJavaFXThread, so FILE-field serialization runs on the
JavaFX thread. This can cause noticeable FX-thread stalls for entries with many linked files or long
paths, even though only the actual field mutation needs to be scheduled.

Code

jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[R53-56]

Evidence
The GUI cleanup path passes a JavaFX-thread scheduler (run-and-wait) to CleanupWorker jobs, so
anything executed inside mutationScheduler runs on the FX thread. BibEntry#setFiles performs full
FILE-field serialization via FileFieldWriter, which iterates linked files and escapes strings
character-by-character; doing that work in the scheduled runnable can block the FX thread
unnecessarily.
jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[53-56]
jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[59-62]
jabgui/src/main/java/org/jabref/gui/cleanup/CleanupDialogViewModel.java[235-239]
jablib/src/main/java/org/jabref/logic/cleanup/CleanupWorker.java[46-56]
jablib/src/main/java/org/jabref/model/entry/BibEntry.java[1016-1025]
jablib/src/main/java/org/jabref/logic/bibtex/FileFieldWriter.java[15-27]
jablib/src/main/java/org/jabref/logic/bibtex/FileFieldWriter.java[55-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`MoveFilesCleanup` and `RenamePdfCleanup` schedule `entry.setFiles(files)` on `mutationScheduler`. In the GUI cleanup flow, this scheduler is `UiTaskExecutor::runAndWaitInJavaFXThread`, which means `BibEntry#setFiles` (and its `FileFieldWriter.getStringRepresentation(files)` serialization work) runs on the JavaFX thread.
### Issue Context
This PR correctly moved the actual `BibEntry` mutation onto the scheduler thread to avoid “not on FX thread” exceptions. However, `setFiles(...)` does both (1) computation/serialization and (2) the mutation, so scheduling it also schedules the computation.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[39-66]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[36-72]
### Suggested fix approach
1. Keep file I/O and any string construction on the calling (background) thread.
2. Precompute the new FILE field string (e.g., via `FileFieldWriter.getStringRepresentation(files)`) and compare to the current value off-FX.
3. Only schedule the minimal mutation on the FX thread, e.g. `entry.setField(StandardField.FILE, newValue)` (or equivalent), and capture the returned `FieldChange` via an `AtomicReference` if you need to return it.
4. Apply the same pattern in both `MoveFilesCleanup` and `RenamePdfCleanup`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

* upstream/main: (29 commits) Chore(deps): Bump dev.langchain4j:langchain4j-bom in /versions (JabRef#15853) Chore(deps): Bump org.glassfish.jaxb:jaxb-runtime in /versions (JabRef#15854) Chore(deps): Bump com.gradleup.shadow:shadow-gradle-plugin (JabRef#15852) Chore(deps): Bump com.gradleup.shadow:shadow-gradle-plugin (JabRef#15849) Chore(deps): Bump com.autonomousapps:dependency-analysis-gradle-plugin (JabRef#15850) Update dependency org.apache.maven.plugins:maven-surefire-plugin to v3.5.6 (JabRef#15844) Fix reset and import of AiPreferences (JabRef#15843) Fix Comparable Contract Violation in SharedBibEntryData (JabRef#15806) (JabRef#15842) Chore(deps): Bump com.dlsc.gemsfx:gemsfx from 4.0.5 to 4.1.0 in /versions (JabRef#15841) Chore(deps): Bump jablib/src/main/resources/csl-styles (JabRef#15840) New Crowdin updates (JabRef#15839) Add group pseudonymization support (fixes JabRef#14117) (JabRef#15258) Feature parse MeSH terms in PubMed MEDLINE records (JabRef#15529) Fix/non latin author parsed as name prefix (JabRef#15823) Fix not on fx thread cleanup (JabRef#15835) Fix garbled BibEntry Javadoc example (JabRef#15834) Revert "Fix cleanup operationn setFiles not on fx thread causes exceptiosn" Revert "changelog" changelog Fix cleanup operationn setFiles not on fx thread causes exceptiosn ...

Siedlerchr added 2 commits May 26, 2026 22:25

Reapply "Fix cleanup operationn setFiles not on fx thread causes exce…

ef950c5

…ptiosn" This reverts commit 5594f85.

Reapply "changelog"

07d1b94

This reverts commit 51bffea.

addres qodo feedback, move only set field to fx thread

ee6b881

calixtus approved these changes May 26, 2026

View reviewed changes

calixtus added this pull request to the merge queue May 26, 2026

github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label May 26, 2026

Merged via the queue into main with commit fcf1752 May 26, 2026
54 checks passed

calixtus deleted the fixNotOnFxThreadCleanup branch May 26, 2026 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix not on fx thread cleanup#15835

Fix not on fx thread cleanup#15835
calixtus merged 3 commits into
mainfrom
fixNotOnFxThreadCleanup

Siedlerchr commented May 26, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Siedlerchr commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues and pull requests

PR Description

Steps to test

AI usage

Checklist

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Siedlerchr commented May 26, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented May 26, 2026 •

edited

Loading