Fix stale main table search results#16137
Conversation
|
Hey @gabrielSossai! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
github-actions
Bot
added
first contrib
status: changes-required
PR Summary by QodoPrevent stale main table search results from out-of-order searches
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Silent latest-search failure
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| long currentSearchUpdateId = searchUpdateId.incrementAndGet(); | ||
| BackgroundTask.wrap(() -> query.map(searchQuery -> searchContext.search(searchQuery))) | ||
| .onSuccess(results -> { | ||
| if (currentSearchUpdateId != searchUpdateId.get()) { | ||
| return; | ||
| } | ||
|
|
||
| results.ifPresentOrElse(this::setSearchMatches, this::clearSearchMatches); | ||
| FilteredListProxy.refilterListReflection(entriesFiltered); | ||
| }).executeWith(taskExecutor); |
There was a problem hiding this comment.
1. Ui-thread mass search update 🐞 Bug ➹ Performance
updateSearchMatches now applies setSearchMatches/clearSearchMatches inside BackgroundTask.onSuccess, which runs on the JavaFX thread, and these methods iterate all entries and mutate multiple observable properties. On large libraries and frequent search updates (typing), this can cause noticeable UI freezes/jank.
Agent Prompt
### Issue description
`setSearchMatches` / `clearSearchMatches` iterate over all `entriesViewModel` and update multiple JavaFX properties. Because they are now invoked from `onSuccess`, this work runs on the JavaFX thread and can block rendering/interaction for large libraries.
### Issue Context
- `BackgroundTask.onSuccess` is documented to always run on the JavaFX thread.
- The heavy loops are in `setSearchMatches` / `clearSearchMatches`.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[106-147]
### Suggested approach
- Keep `searchContext.search(...)` in the background task.
- In the background thread, compute a compact result (e.g., `Set<BibEntryId> matchedIds` and `Set<BibEntryId> fulltextIds`) and optionally a list of only entries whose state would change.
- In `onSuccess`, apply only the necessary property changes (delta update) and consider chunking updates if still large (e.g., batched `Platform.runLater` updates) to keep frames responsive.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| long currentSearchUpdateId = searchUpdateId.incrementAndGet(); | ||
| BackgroundTask.wrap(() -> query.map(searchQuery -> searchContext.search(searchQuery))) | ||
| .onSuccess(results -> { | ||
| if (currentSearchUpdateId != searchUpdateId.get()) { | ||
| return; | ||
| } | ||
|
|
||
| results.ifPresentOrElse(this::setSearchMatches, this::clearSearchMatches); | ||
| FilteredListProxy.refilterListReflection(entriesFiltered); | ||
| }).executeWith(taskExecutor); |
There was a problem hiding this comment.
2. Silent latest-search failure 🐞 Bug ☼ Reliability
updateSearchMatches does not set an onFailure handler, and under UiTaskExecutor a failure is only surfaced if an exception consumer is configured. With the new searchUpdateId guard, if the latest search task fails, older completed searches are suppressed and the main table can remain stuck showing stale results with no recovery path.
Agent Prompt
### Issue description
The latest-only application logic (`searchUpdateId`) suppresses older results, but `updateSearchMatches` has no `onFailure` handling. If the newest search throws, the UI may keep showing results for a previous query indefinitely.
### Issue Context
In `UiTaskExecutor.getJavaFXTask`, `setOnFailed` is only registered if `BackgroundTask.getOnException()` is non-null; without `onFailure(...)`, failures may not be reported or acted upon.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableDataModel.java[106-116]
- jabgui/src/main/java/org/jabref/gui/util/UiTaskExecutor.java[196-209]
### Suggested fix
- Add `.onFailure(e -> { if (currentSearchUpdateId != searchUpdateId.get()) return; LOGGER.warn("Search failed", e); clearSearchMatches(); FilteredListProxy.refilterListReflection(entriesFiltered); })`.
- Decide the desired UI behavior on failure (clear results vs. keep previous, but then explicitly reflect the failure state); the key is to avoid silently leaving stale results for a different query.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Your code currently does not meet JabRef's code guidelines. We use OpenRewrite to ensure "modern" Java coding practices. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Source Code Tests / OpenRewrite (pull_request)" and click on it. The issues found can be automatically fixed. Please execute the gradle task |
|
Don't fool us. |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #15710. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
3 participants
AI usage
OpenAI Codex (model GPT-5) was used to help inspect the issue, analyze the codebase, discuss implementation alternatives, write and revise the test/fix, check project contribution instructions, and prepare this PR draft. I reviewed, understood, and take ownership of the submitted changes.
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). UseTODOas the issue/PR reference placeholder when no issue is known and the PR is not yet created — 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, it was replaced with the real PR-number link after PR 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)