Fix suggested groups after library load - #16552
Conversation
PR Summary by QodoFix suggested groups failing right after library load
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. replaceActiveDatabase nullability unclear
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (isSelectedTab) { | ||
| stateManager.setActiveDatabase(bibDatabaseContext); | ||
| stateManager.replaceActiveDatabase(bibDatabaseContext); | ||
| stateManager.activeTabProperty().set(Optional.of(this)); |
There was a problem hiding this comment.
2. Transient active-db empty 🐞 Bug ☼ Reliability
LibraryTab#setDatabaseContext now unconditionally calls replaceActiveDatabase, which forces an intermediate Optional.empty() active-database event even when the old/new contexts are not equal. This can cause subscribers (e.g., groups and git status) to clear/reset and then rebuild immediately, creating avoidable churn and potential UI flicker.
Agent Prompt
### Issue description
`StateManager.replaceActiveDatabase(...)` intentionally emits two active-database values (`Optional.empty()` then the new context) to force downstream notifications when contexts compare equal. However, `LibraryTab#setDatabaseContext` currently calls it unconditionally for the selected tab, meaning *all* subscribers see a transient “no active database” even when a normal `setActiveDatabase(...)` would notify correctly.
### Issue Context
This transient empty state triggers reset/clear logic in multiple subscribers (groups tree, git status, etc.), which is only required in the special “old equals new” case the PR is fixing.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[357-385]
### Suggested change
In `setDatabaseContext`, only call `replaceActiveDatabase` when the currently active database is present and `equals(...)` the new `bibDatabaseContext` (the problematic case). Otherwise call `setActiveDatabase(bibDatabaseContext)` to avoid the intermediate empty event.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
@qodo Equals on bibdatabsecontext is expensive as it will compare the whole library entry by entry
There was a problem hiding this comment.
That’s a valid concern—BibDatabaseContext.equals(...) should not be used on this path because it can traverse the entire library.
The behavior issue still stands, but the conditional should use a cheap lifecycle/identity check instead of deep equality—for example, compare a stable context/database identity (or the canonical database path, where available) and call replaceActiveDatabase only for that known reload case. Use setActiveDatabase for ordinary context changes. If no suitable stable identity exists, the reload operation should explicitly signal the replacement rather than inferring it via equals(...).
github-actions
Bot
added
the
status: changes-required
|
Your pull request conflicts with the target branch. Please merge |
|
So we "add to the changes" an |
|
This is related to the search context setup. Just open a new library and select create suggested groups and you will get an exception. |
2 participants
Summary
Fixes the failure when “Add JabRef suggested groups” is selected immediately after a library has loaded. The loading placeholder and parsed context can compare equal, preventing the group tree from being refreshed; the active context is now deliberately replaced after the search context is initialized.
jabref-contrib-policy:4.2:reviewed:ok
Steps to test
.biblibrary, including a large library.No SearchContext registeredassertion.Validated manually with the generated large library. Normal tab changes retain their single active-database notification.
Related issues and pull requests
No matching issue found.
AI usage
OpenAI Codex (model GPT-5). The contributor reviewed, understood, and takes ownership of the change.
AI CHECKLIST.md walkthrough
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). No production class was added; the test class follows existing package conventions.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). The test creates an empty entry without fields.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}, and[ClassName]instead of{@link}.User-facing text
Localization.langin Java,%prefix in FXML). No UI text was added.!; labels do not end with:. No UI text was added."...: %0"), not string concatenation. No UI text was added.Security
text/htmlresponse. No HTML response was changed.Tests
org.jabref.model/org.jabref.logichave added or updated tests. The behavior change is in the GUI/state manager.assertEquals), use plain JUnit asserts, have no@DisplayName, do not catch exceptions, and use@TempDirinstead of manual temp directories.2. Verification commands
./gradlew :jablib:check./gradlew checkstyleMain checkstyleTest checkstyleJmh./gradlew modernizer./gradlew --no-configuration-cache :rewriteDryRun./gradlew javadocnpx markdownlint-cli2 "docs/**/*.md" "*.md"rewriteDryRunreported no changes.3. Documentation
CHANGELOG.mdentry added for this user-visible fix, withTODOawaiting the PR number.4. Pull request
.github/PULL_REQUEST_TEMPLATE.md, every section filled.[x],[ ], or[/].gh pr create --body-file.TODOchangelog reference will be replaced with the real PR link immediately after creation, then committed and pushed.Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)