group remember after entry deletion by pgrigolli · Pull Request #15630 · JabRef/jabref

pgrigolli · 2026-04-27T20:22:16Z

Related issues and pull requests

PR Description

When deleting an entry, the active group filter was being reset because
EntriesRemovedEvent listener was not reapplying the group matches after
refreshing the search. Fixed by calling updateGroupMatches() after the refresh.

Steps to test

Open JabRef
Select a existing library (or create an example one)
Delete and entry
Check if the filter is the same as it was before deleting

Checklist

I own the copyright of the code submitted and I license it under the MIT license
I manually tested my changes in running JabRef (always required)
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)

Video Proof:

videoProof15587.mp4

…dinates settings is done

jabref-machine · 2026-04-27T20:28:37Z

Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of - [x] (done), - [ ] (yet to be done) or - [/] (not applicable). Please adhere to our pull request template.

github-actions · 2026-04-28T04:20:21Z

Your pull request conflicts with the target branch.

Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line.

faneeshh · 2026-04-28T07:26:49Z

Looks good to me

pgrigolli · 2026-04-28T13:58:54Z

Hey, thanks for the review, but what is happening with the checks? Is there smth I should do?

faneeshh · 2026-04-28T22:00:21Z

Hey, thanks for the review, but what is happening with the checks? Is there smth I should do?

Those are unrelated to your changes I believe. You can mark the PR ready for review.

qodo-free-for-open-source-projects · 2026-04-29T01:37:46Z

Review Summary by Qodo

(Agentic_describe updated until commit `9bd7428`)

Preserve group filter after entry deletion

🐞 Bug fix

Walkthroughs

Description

• Preserve active group filter when deleting entries
• Store selectedGroupsProperty reference for event listener access
• Reapply group matches after search refresh in EntriesRemovedEvent
• Update CHANGELOG with fix description

Diagram

flowchart LR
  A["Entry Deletion"] --> B["EntriesRemovedEvent"]
  B --> C["Search Refresh"]
  C --> D["updateGroupMatches"]
  D --> E["Group Filter Preserved"]

File Changes

1. jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java 🐞 Bug fix +4/-1

Preserve group filter on entry deletion

• Added selectedGroupsProperty field to store reference for event listener access
• Captured selected groups before background task execution in EntriesRemovedEvent listener
• Called updateGroupMatches() after search refresh to reapply group filter
• Ensures group filter persists after entry deletion

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java

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

Document group filter fix

• Added entry documenting fix for issue #15587
• Describes resolution of group filter reset after entry deletion

CHANGELOG.md

qodo-free-for-open-source-projects · 2026-04-29T01:37:47Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1)

1. Off-thread group selection read 🐞 Bug ☼ Reliability

Description

In the EntriesRemovedEvent handler, selectedGroupsProperty.get() is executed inside
BackgroundTask.call (background thread), which can race with UI updates and lead to inconsistent
group filtering or runtime threading issues. This was introduced by the new
updateGroupMatches(selectedGroupsProperty.get()) call inside the BackgroundTask body.

Code

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[R282-286]

               } else {
                   clearSearchMatches();
               }
+                updateGroupMatches(selectedGroupsProperty.get());
           }).onSuccess(result -> FilteredListProxy.refilterListReflection(entriesFiltered)).executeWith(taskExecutor);

Evidence
MainTableDataModel invokes updateGroupMatches(selectedGroupsProperty.get()) inside the
BackgroundTask.wrap(() -> { ... }) body for EntriesRemovedEvent; BackgroundTask.call is executed on
a background thread by UiTaskExecutor (while callbacks like onSuccess run on the FX thread). Since
selectedGroupsProperty is a JavaFX ListProperty, reading it from the background thread is not
thread-safe and can produce stale/incorrect snapshots or concurrent access issues if the UI updates
the selection concurrently.
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
jabgui/src/main/java/org/jabref/gui/util/UiTaskExecutor.java[108-114]
jabgui/src/main/java/org/jabref/gui/util/UiTaskExecutor.java[186-203]
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[63-88]
Best Practice: JavaFX threading rule

Agent prompt

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

## Issue description
`selectedGroupsProperty.get()` is called inside `BackgroundTask.wrap(() -> { ... })` in the `EntriesRemovedEvent` listener. Because `BackgroundTask.call()` runs on a background thread, this reads a JavaFX `ListProperty` off the FX thread.
### Issue Context
`UiTaskExecutor` executes `BackgroundTask.call()` on a background thread. Accessing JavaFX properties/ObservableLists from that thread can race with UI updates and produce inconsistent results.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
### Suggested fix approach
- Read the selected groups on the FX thread *before* starting the background work, and pass an immutable snapshot into the task (e.g., `List<GroupTreeNode> snapshot = List.copyOf(selectedGroupsProperty.get());`).
- Then update group matches based on that snapshot (either by overloading `updateGroupMatches` to accept `List<GroupTreeNode>` or by inlining the matcher recomputation in the same task without touching `selectedGroupsProperty` off-thread).

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

2. No test for deletion filter 📘 Rule violation ☼ Reliability ⭐ New

Description

The PR changes deletion behavior by reapplying group matches after an EntriesRemovedEvent, but no
regression test is added/updated to cover this behavior. This increases the risk of the bug
returning without detection.

Code

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[R274-287]

        public void listen(EntriesRemovedEvent removedEntriesEvent) {
            // When entries are removed, we need to refresh the search matches
            // to ensure the filtered list is properly updated and doesn't show stale entries
+            ObservableList<GroupTreeNode> selectedGroups = selectedGroupsProperty.get();
            BackgroundTask.wrap(() -> {
                // Re-run the current search to update the filtered results
                if (searchQueryProperty.get().isPresent()) {
                    setSearchMatches(indexManager.search(searchQueryProperty.get().get()));
                } else {
                    clearSearchMatches();
                }
+                updateGroupMatches(selectedGroups);
            }).onSuccess(result -> FilteredListProxy.refilterListReflection(entriesFiltered)).executeWith(taskExecutor);
        }

Evidence
PR Compliance ID 19 requires tests to be added/updated for behavior changes. The PR introduces a
behavioral change in the EntriesRemovedEvent listener (updateGroupMatches(...) is newly invoked
after refreshing search matches), but the existing MainTableDataModelTest does not cover entry
deletion/group-filter retention behavior.
AGENTS.md
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[273-287]
jabgui/src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java[28-64]

Agent prompt

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

## Issue description
Behavior was changed so that after `EntriesRemovedEvent`, group matches are reapplied. This should be covered by a regression test to prevent future regressions.

## Issue Context
The reported bug is: deleting an entry reset the active group filter. The PR fixes this by calling `updateGroupMatches()` after refreshing search results.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[273-287]
- jabgui/src/test/java/org/jabref/gui/maintable/MainTableDataModelTest.java[1-64]

## Suggested direction
- Add a test that sets up a `MainTableDataModel` with a selected group filter, removes an entry (or simulates `EntriesRemovedEvent`), and asserts the filtered list/group match state remains consistent after deletion.

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

3. Redundant refilter/background work 🐞 Bug ➹ Performance

Description

The EntriesRemovedEvent handler schedules a BackgroundTask that refilters on success, and also calls
updateGroupMatches(), which schedules another BackgroundTask that refilters again. This adds extra
work on every deletion and can cause unnecessary UI churn.

Code

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[R285-286]

+                updateGroupMatches(selectedGroupsProperty.get());
           }).onSuccess(result -> FilteredListProxy.refilterListReflection(entriesFiltered)).executeWith(taskExecutor);

Evidence
The EntriesRemovedEvent handler runs a BackgroundTask and refilters the table in its onSuccess.
updateGroupMatches() also creates its own BackgroundTask and refilters in its onSuccess. Calling
updateGroupMatches() from within the deletion refresh path therefore creates two tasks and two
refilter operations for a single delete.
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[167-174]

Agent prompt

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

## Issue description
On entry deletion, the code triggers multiple BackgroundTasks/refilters: one for refreshing search matches and another inside `updateGroupMatches()`. This does redundant work per deletion.
### Issue Context
Both tasks end with `FilteredListProxy.refilterListReflection(entriesFiltered)`.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[167-174]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
### Suggested fix approach
- During `EntriesRemovedEvent`, perform both the search-match refresh and the group-match recomputation within the *same* BackgroundTask call body, and refilter once in the enclosing task’s `onSuccess`.
- Optionally refactor `updateGroupMatches` into a `updateGroupMatchesInternal(...)` that does not schedule its own task, so it can be reused by both the selection listener and the deletion path.

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

Previous review results

Review updated until commit 9bd7428

Results up to commit ba7073c

🐞 Bugs (2) 📘 Rule violations (0)

1. Off-thread group selection read 🐞 Bug ☼ Reliability

Description

In the EntriesRemovedEvent handler, selectedGroupsProperty.get() is executed inside
BackgroundTask.call (background thread), which can race with UI updates and lead to inconsistent
group filtering or runtime threading issues. This was introduced by the new
updateGroupMatches(selectedGroupsProperty.get()) call inside the BackgroundTask body.

Code

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[R282-286]

                } else {
                    clearSearchMatches();
                }
+                updateGroupMatches(selectedGroupsProperty.get());
            }).onSuccess(result -> FilteredListProxy.refilterListReflection(entriesFiltered)).executeWith(taskExecutor);

Evidence
MainTableDataModel invokes updateGroupMatches(selectedGroupsProperty.get()) inside the
BackgroundTask.wrap(() -> { ... }) body for EntriesRemovedEvent; BackgroundTask.call is executed on
a background thread by UiTaskExecutor (while callbacks like onSuccess run on the FX thread). Since
selectedGroupsProperty is a JavaFX ListProperty, reading it from the background thread is not
thread-safe and can produce stale/incorrect snapshots or concurrent access issues if the UI updates
the selection concurrently.
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
jabgui/src/main/java/org/jabref/gui/util/UiTaskExecutor.java[108-114]
jabgui/src/main/java/org/jabref/gui/util/UiTaskExecutor.java[186-203]
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[63-88]
Best Practice: JavaFX threading rule

Agent prompt

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

### Issue description
`selectedGroupsProperty.get()` is called inside `BackgroundTask.wrap(() -> { ... })` in the `EntriesRemovedEvent` listener. Because `BackgroundTask.call()` runs on a background thread, this reads a JavaFX `ListProperty` off the FX thread.

### Issue Context
`UiTaskExecutor` executes `BackgroundTask.call()` on a background thread. Accessing JavaFX properties/ObservableLists from that thread can race with UI updates and produce inconsistent results.

### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]

### Suggested fix approach
- Read the selected groups on the FX thread *before* starting the background work, and pass an immutable snapshot into the task (e.g., `List<GroupTreeNode> snapshot = List.copyOf(selectedGroupsProperty.get());`).
- Then update group matches based on that snapshot (either by overloading `updateGroupMatches` to accept `List<GroupTreeNode>` or by inlining the matcher recomputation in the same task without touching `selectedGroupsProperty` off-thread).

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

2. Redundant refilter/background work 🐞 Bug ➹ Performance

Description

The EntriesRemovedEvent handler schedules a BackgroundTask that refilters on success, and also calls
updateGroupMatches(), which schedules another BackgroundTask that refilters again. This adds extra
work on every deletion and can cause unnecessary UI churn.

Code

jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[R285-286]

+                updateGroupMatches(selectedGroupsProperty.get());
            }).onSuccess(result -> FilteredListProxy.refilterListReflection(entriesFiltered)).executeWith(taskExecutor);

Evidence
The EntriesRemovedEvent handler runs a BackgroundTask and refilters the table in its onSuccess.
updateGroupMatches() also creates its own BackgroundTask and refilters in its onSuccess. Calling
updateGroupMatches() from within the deletion refresh path therefore creates two tasks and two
refilter operations for a single delete.
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]
jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[167-174]

Agent prompt

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

### Issue description
On entry deletion, the code triggers multiple BackgroundTasks/refilters: one for refreshing search matches and another inside `updateGroupMatches()`. This does redundant work per deletion.

### Issue Context
Both tasks end with `FilteredListProxy.refilterListReflection(entriesFiltered)`.

### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[167-174]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[274-286]

### Suggested fix approach
- During `EntriesRemovedEvent`, perform both the search-match refresh and the group-match recomputation within the *same* BackgroundTask call body, and refilter once in the enclosing task’s `onSuccess`.
- Optionally refactor `updateGroupMatches` into a `updateGroupMatchesInternal(...)` that does not schedule its own task, so it can be reused by both the selection listener and the deletion path.

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

calixtus · 2026-05-04T20:26:06Z

Hi, any interest to continue here? Otherwise we'll close

pgrigolli · 2026-05-04T20:32:08Z

Hi, any interest to continue here? Otherwise we'll close

Hey, yes. I will submit a commit that solves that comment today

pgrigolli · 2026-05-04T21:05:43Z

Hey, how can i resolve this conflicts? I didn't change anything in those csl-styles files

Siedlerchr · 2026-05-04T21:09:01Z

https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/trouble-shooting.html#changes-in-jablibsrcmainresourcescsl-styles-are-shown

…r-issue-15587

qodo-free-for-open-source-projects · 2026-05-04T22:37:50Z

Persistent review updated to latest commit 9bd7428

github-actions · 2026-05-04T22:37:59Z

Do not mark a PR as ready-for-review if changes are required.
Address the changes first.

pgrigolli · 2026-05-04T23:20:33Z

Persistent review updated to latest commit 9bd7428

what should I do to solve this? If I understood what it is saying, the bot wants me to write some tests for this fix

Siedlerchr · 2026-05-13T17:44:17Z

The bot is giving you some feedback on critical areas that you should take a closer look. I am not sure if it's possible to write a test but at least take a look at the thing with UI sync

pgrigolli · 2026-05-14T19:08:06Z

The bot is giving you some feedback on critical areas that you should take a closer look. I am not sure if it's possible to write a test but at least take a look at the thing with UI

Looking at what the boot is complaining about, this "Action Required" is about a code that I already changed and was supposed to correct this.

ThiloteE · 2026-06-04T08:04:37Z

The bot is giving you some feedback on critical areas that you should take a closer look. I am not sure if it's possible to write a test but at least take a look at the thing with UI

Looking at what the boot is complaining about, this "Action Required" is about a code that I already changed and was supposed to correct this.

Are you saying the test mentioned by the bot is outdated?

pgrigolli · 2026-06-05T13:26:06Z

The bot is giving you some feedback on critical areas that you should take a closer look. I am not sure if it's possible to write a test but at least take a look at the thing with UI

Looking at what the boot is complaining about, this "Action Required" is about a code that I already changed and was supposed to correct this.

Are you saying the test mentioned by the bot is outdated?

Not the test, but the code itself. If I'm not seeing it wrong, the newest version of this code is not what the bot is complaining there.
The bot is taking in consideration this code: "selectedGroupsProperty.get()"

But seeing this review, I changed it to get this properties before entering the UI thread, removing the possibility to race conditions, as it said in the review.
If you take a look on the files changed tab, the latest commit about this was:

So, if I'm not wrong here, it should consider the version with "selectedGroups" variable (without the .get()), and that should fix this issue.
Please let me know if I'm mistaken here

pgrigolli and others added 11 commits April 10, 2026 16:22

fix visual snap of side pane on startup

06cb696

Fix side pane snap by applying divider position when stage is showing

7ed4ea5

Fix side pane snap by hiding the side pane until the loading and coor…

0c02a92

…dinates settings is done

Changelog

663738a

Merge branch 'main' into fix-for-issue-15394

bf22cc6

Changelog line removal

ace702f

changelog.md description more concise

f832809

Removing dead code

e9e5b5c

Merge branch 'main' into fix-for-issue-15394

5d55b54

Merge branch 'main' into fix-for-issue-15394

b75e056

fix: group remember after entry deletion

4c1a901

github-actions Bot added component: groups status: changes-required Pull requests that are not yet complete labels Apr 27, 2026

github-actions Bot mentioned this pull request Apr 27, 2026

Forgot group selection after entry deletion #15587

Open

2 tasks

faneeshh reviewed Apr 28, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

pgrigolli marked this pull request as ready for review April 29, 2026 01:37

koppor marked this pull request as draft April 29, 2026 01:37

qodo-free-for-open-source-projects Bot reviewed Apr 29, 2026

View reviewed changes

Comment thread jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java

fix: get selectedGroup data before entering UI thread

9e5885d

pgrigolli force-pushed the fix-for-issue-15587 branch from f795c6c to 9e5885d Compare May 4, 2026 21:53

pgrigolli and others added 3 commits May 4, 2026 18:53

Merge branch 'main' into fix-for-issue-15587

0639a2b

Changelog.md

21f9420

Merge remote-tracking branch 'origin/fix-for-issue-15587' into fix-fo…

9bd7428

…r-issue-15587

pgrigolli marked this pull request as ready for review May 4, 2026 22:37

koppor marked this pull request as draft May 4, 2026 22:38

Merge branch 'main' into fix-for-issue-15587

94f94c9

Uh oh!

Conversation

pgrigolli commented Apr 27, 2026

Related issues and pull requests

PR Description

Steps to test

Checklist

Uh oh!

jabref-machine commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

Uh oh!

faneeshh commented Apr 28, 2026

Uh oh!

pgrigolli commented Apr 28, 2026

Uh oh!

faneeshh commented Apr 28, 2026

Uh oh!

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

Uh oh!

Review Summary by Qodo

(Agentic_describe updated until commit 9bd7428)

Walkthroughs

File Changes

Uh oh!

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

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

calixtus commented May 4, 2026

Uh oh!

pgrigolli commented May 4, 2026

Uh oh!

pgrigolli commented May 4, 2026

Uh oh!

Siedlerchr commented May 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

Siedlerchr commented May 13, 2026

Uh oh!

pgrigolli commented May 14, 2026

Uh oh!

ThiloteE commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgrigolli commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

qodo-free-for-open-source-projects Bot commented Apr 29, 2026 •

edited

Loading

(Agentic_describe updated until commit `9bd7428`)

qodo-free-for-open-source-projects Bot commented Apr 29, 2026 •

edited

Loading

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

edited

Loading

pgrigolli commented May 4, 2026 •

edited

Loading

ThiloteE commented Jun 4, 2026 •

edited

Loading