-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
.pr_agent_accepted_suggestions
| PR 15988 (2026-06-16) |
[correctness] Stale monitor after Save As
Stale monitor after Save As
SaveDatabaseAction.save(Path) always calls LibraryTab.resumeChangeMonitor() in finally, but during saveAs(Path, ...) the BibDatabaseContext path is updated only after save(...) returns, so the monitor can be recreated for the old path and later cannot be unregistered. This leaves a file watcher for the previous file active, which can trigger incorrect “external changes” notifications and leak listeners.SaveDatabaseAction.save(Path, ...) calls libraryTab.resumeChangeMonitor() unconditionally in finally. In the saveAs(Path, ...) flow, save(file, mode) returns before context.setDatabasePath(file) runs, so resumeChangeMonitor() can recreate a DatabaseChangeMonitor registered for the old path. After the path changes, libraryTab.resetChangeMonitor() attempts to unregister using the new path, which fails to remove the old-path listener because DatabaseChangeMonitor.unregister() consults database.getDatabasePath() at unregister time.
This results in a stale listener still registered on the old file, potentially causing spurious “external changes detected” notifications and leaking listeners.
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[208-248]
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[821-841]
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[42-68]
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[136-138]
- jablib/src/main/java/org/jabref/model/database/BibDatabaseContext.java[108-117]
Implement one of the following robust fixes (prefer A):
A) Make DatabaseChangeMonitor unregister path-stable
- Store the monitored path at construction time (e.g.,
private final Optional<Path> monitoredPath;initialized fromdatabase.getDatabasePath()). - Use
monitoredPathfor both registration andunregister()removal. - This ensures
unregister()removes the correct listener even ifBibDatabaseContext.setDatabasePath(...)is called later. B) Conditionally resume monitor insave(Path, ...) - Capture the original path before suspending.
- In
finally, only resume if the save failed ORtargetPath.equals(originalPath). - This prevents re-registering on the old path in the
saveAscase where the caller will reset the monitor after updating the context path. Add/adjust a unit test coveringsaveAs(...)from an already-saved library to ensure no stale old-path listener remains (can be done by verifyingFileUpdateMonitor.removeListener(oldPath, ...)is called, using a test double/mocked FileUpdateMonitor).
[maintainability] `resumeChangeMonitor` uses `isPresent()`
`resumeChangeMonitor` uses `isPresent()`
`resumeChangeMonitor()` uses an `Optional.isPresent()` conditional, which is discouraged in favor of `ifPresent(...)`/functional Optional APIs. This reduces code clarity and consistency with the project's Optional-handling standards.resumeChangeMonitor() branches on bibDatabaseContext.getDatabasePath().isPresent() instead of using idiomatic Optional APIs.
The compliance checklist prefers ifPresent/ifPresentOrElse over isPresent-driven branching for clearer intent.
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[837-840]
| PR 15984 (2026-06-15) |
[correctness] `Could not auto-detect...` missing l10n
`Could not auto-detect...` missing l10n
`ImportHandler` introduces a new user-facing message via `Localization.lang(...)` but there is no corresponding entry in `jablib/src/main/resources/l10n/JabRef_en.properties`, which can break localization consistency checks and leaves translations missing. Add the key to the English base properties file (and keep using placeholder-based formatting if variables are added later).A new Localization.lang("Could not auto-detect file format. An empty entry was created with file link.") string was added, but the corresponding localization key is missing from JabRef_en.properties.
JabRef requires that user-facing strings used via Localization.lang(...) exist in the English base localization file so that localization consistency checks pass and translations can be provided.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[233-233]
- jablib/src/main/resources/l10n/JabRef_en.properties[2549-2556]
[performance] Ungated drop auto-detection
Ungated drop auto-detection
ImportHandler now calls ImportFormatReader.importWithAutoDetection for every dropped file that is neither PDF nor .bib, which will iterate through many importers and can repeatedly read/parse arbitrary files, slowing drag-and-drop compared to the previous constant-time “create empty entry + link file” fallback. This is a new performance/reliability risk for large or frequently-dropped non-bibliography files.ImportHandler#importFilesInBackground runs ImportFormatReader.importWithAutoDetection(file) for any non-PDF/non-.bib dropped file. Auto-detection loops through many importers and may attempt parsing for each, creating avoidable work for drops that should remain “attach/link file”.
ImportFormatReader#importWithAutoDetection(Path) iterates over all registered importers and checks isRecognizedFormat(...) before attempting importDatabase(...). The base Importer#isRecognizedFormat(...) defaults to true, meaning some importers will be tried even when unsure, which increases the amount of work performed during drag-and-drop.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[207-235]
- jablib/src/main/java/org/jabref/logic/importer/ImportFormatReader.java[150-275]
- jablib/src/main/java/org/jabref/logic/importer/Importer.java[33-62]
Before calling importWithAutoDetection(file), add a cheap gate such as:
- Only attempt auto-detection when the file extension matches one of the extensions supported by any importer’s
getFileType()(excludingpdf/bib, already handled), or - Only attempt when extension is in a curated set of known “library/bibliography” formats.
If the file doesn’t pass the gate, keep the prior behavior:
createEmptyEntryWithLink(file).
[correctness] Auto-import ignores warnings
Auto-import ignores warnings
When auto-detection yields entries, ImportHandler always marks the result as success and does not surface `ParserResult` warnings or the detected format, so partial/dirty imports can be silently treated as successful. This reduces correctness of user feedback compared to the `.bib` import path which explicitly checks for warnings.In the new auto-detection import path, the UI result is considered successful solely based on importedEntries.isEmpty(). Any ParserResult warnings (or the detected format) are ignored, which can mislead users when an import produced entries but also warnings.
ParserResult explicitly supports warnings via hasWarnings() / getErrorMessage(). The .bib drag-drop path already uses hasWarnings() to set success and the message, but the auto-detect path does not.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[207-235]
- jablib/src/main/java/org/jabref/logic/importer/ParserResult.java[105-143]
After importWithAutoDetection(file):
- Check
importResult.parserResult().hasWarnings(). - If warnings exist, either:
- mark the result as unsuccessful and show
parserResult.getErrorMessage(), or - mark success but include a warning message (and ideally the detected
importResult.format()). - Prefer a non-BibTeX-specific message for the empty-entry case (since this branch handles many formats).
| PR 15977 (2026-06-13) |
[correctness] PAT persist skipped in setAll
PAT persist skipped in setAll
GitPreferences#setAll updates patProperty before rememberPatProperty; since JabRefCliPreferences only writes PAT changes to the keyring when rememberPat is already true, a bulk update that flips rememberPat to true and updates the PAT can leave the keyring out of sync. This can happen in importPreferences via getGitPreferences().setAll(getGitPreferencesFromBackingStore(getGitPreferences())) when rememberPat becomes true but the PAT comes from the defaults fallback (not the keyring).GitPreferences#setAll assigns pat before rememberPat. In JabRefCliPreferences, the patProperty listener persists the PAT only if rememberPat is already enabled, so setAll can skip persisting a new PAT when it enables persistence and updates the PAT in the same operation.
-
JabRefCliPreferenceslistens togitPreferences.patProperty()and callssetGitHubPat(newVal), butsetGitHubPatis guarded byrememberPatProperty().get(). -
importPreferencesperforms bulk updates usingsetAll(...), so this ordering matters for keeping the in-memory GitPreferences and the keyring consistent.
- jablib/src/main/java/org/jabref/logic/git/preferences/GitPreferences.java[41-46]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2357-2374]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2379-2388]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2412-2426]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[812-848]
Reorder setAll so rememberPat is set before pat (or alternatively, when rememberPat flips to true, explicitly persist the current PAT).
| PR 15973 (2026-06-12) |
[reliability] `jabkit.md` uses fixed /tmp
`jabkit.md` uses fixed /tmp
The added native-image CLI smoke tests write outputs to hardcoded `/tmp/...` paths, which can cause flaky, non-deterministic failures when tests are rerun, run concurrently, or when `/tmp` is not writable/clean. This violates the requirement that tests remain deterministic and reliable, and can also fail when existing output files trigger non-overwrite behavior unless `--force` is used.The native-image CLI smoke test writes test artifacts to fixed filenames under /tmp, which can make the test flaky due to collisions across reruns or concurrent executions, and can also fail if /tmp is not writable/clean. Additionally, pseudonymize may exit non-zero when the output already exists unless -f/--force is supplied, so hardcoded paths can create spurious failures.
This PR adds two new clitest commands for preferences export and pseudonymize that currently write outputs to fixed /tmp/... locations (e.g., jabkit-prefs-smoke.xml, origin.pseudo.bib, origin.pseudo.csv). The pseudonymize implementation performs an overwrite check (via Pseudonymize.fileOverwriteCheck(...)) and refuses to overwrite existing output unless --force is set.
- jabkit/src/test/nativeimage/jabkit.md[11-14]
[reliability] Pseudonymize test misses failures
Pseudonymize test misses failures
In the native-image smoke test, `pseudonymize` output is piped to `grep` without checking the `jabkit` process exit code, so the test can pass even when `pseudonymize` exits non-zero after printing its start message. This can mask real native-image regressions in CI.The smoke test validates pseudonymize by grepping its stdout, but does not assert the command's exit status. Because Pseudonymize prints the "Pseudonymizing library ..." message before later failure points, the grep can succeed even when the command fails.
org.jabref.toolkit.commands.Pseudonymize prints the message before fileOverwriteCheck(...) and returns 2 on overwrite-check failures.
- jabkit/src/test/nativeimage/jabkit.md[11-14]
Split the pipeline to both assert exit code and still validate output, e.g.:
| PR 15968 (2026-06-11) |
[correctness] Broken 'Fixes.' fragment
Broken 'Fixes.' fragment
The Science Direct fetcher entry was changed to "... downloaded. Fixes. [#5860]", which is grammatically broken and reads as an incomplete sentence. This is a content regression in CHANGELOG.md introduced by the punctuation normalization.A CHANGELOG bullet now reads ... could not be downloaded. Fixes. [#5860](...), which is an incomplete/incorrect phrase.
The goal is to keep dot-space-link-style (i.e., the final issue/PR link is preceded by . ) without breaking sentence meaning.
- CHANGELOG.md[1762-1762]
Reword to something like:
-
- We fixed an issue with the Science Direct fetcher where PDFs could not be downloaded. [#5860](...)(so the link is still preceded by., but the strayFixes.is removed).
[correctness] Dangling 'for.' before link
Dangling 'for.' before link
The `monthfiled` entry now ends with "... resolve BibTeX-Strings for. [#13375]", leaving a dangling preposition and altering the intended meaning. This is a content regression in CHANGELOG.md introduced by the punctuation normalization.A CHANGELOG bullet now ends with ... resolve BibTeX-Strings for. [#13375](...), which reads as an incomplete phrase.
Keep compliance with dot-space-link-style while restoring correct wording.
- CHANGELOG.md[333-333]
Rewrite to something like:
-
- We added the fieldmonthfiledto the default list of fields to resolve BibTeX-Strings. [#13375](...)(removing the danglingfor).
| PR 15959 (2026-06-10) |
[reliability] Test misses exit code
Test misses exit code
The native-image smoke test for `jabkit check integrity` pipes output into `grep`, so it never directly asserts `jabkit`’s exit code (0/1 vs error codes 2/3). This can let regressions in exit-code behavior slip through as long as the expected stdout line still appears.The clitest command for check integrity is part of a pipeline, so the test only checks grep’s exit status and output, not the actual jabkit process exit code.
CheckIntegrity uses specific exit codes to distinguish “findings present” (1) from “success/no findings” (0) and real errors (2/3). The smoke test should assert this behavior explicitly.
- jabkit/src/test/nativeimage/jabkit.md[4-7]
Rewrite the integrity test to capture and assert the jabkit exit code before checking output, e.g.:
| PR 15948 (2026-06-10) |
[maintainability] CHANGELOG.md has `#TODO` link
CHANGELOG.md has `#TODO` link
The new changelog entry includes a placeholder `[#TODO](...)` PR link and highly implementation-specific internal identifiers, making it inaccurate and not end-user focused. This violates the changelog requirements for user-visible, merge-ready release notes and would ship a broken link.The added CHANGELOG entry contains a placeholder [#TODO](https://github.com/JabRef/jabref/pull/TODO) PR link (which would be a broken link in a release) and uses internal implementation identifiers (CLEAN_UP_ISSN, NormalizeIssn) rather than end-user-oriented language.
Changelog entries are user-facing release notes and must be end-user focused and properly maintained (per the compliance rule); placeholders like #TODO should not be merged. Replace the placeholder with the actual PR number/link (or remove the link if it is not known yet) and rephrase the entry to describe the user-visible change without internal identifiers.
- CHANGELOG.md[38-41]
| PR 15946 (2026-06-09) |
[correctness] `legislation` uses `pages` key
`legislation` uses `pages` key
The new `legislation` mapping uses the CSL key `pages`, but `ZoteroCitationData.ItemData.getFieldValue(...)` only recognizes `page`, so the pages value resolves to an empty string and `StandardField.PAGES` is never populated. This silently drops page ranges and produces incorrect output for that type mapping.CSLItemTypeDefinitions maps legislation pages using the CSL key "pages", but ZoteroCitationData.ItemData#getFieldValue only supports "page". Because field population is driven by iterating mappings and calling getFieldValue(mappingKey), StandardField.PAGES is never set for legislation items and page ranges are silently dropped.
Field mappings are applied by iterating CSLItemTypeDefinitions.getFieldMappings(type) and passing each mapping key into itemData.getFieldValue(key) (as done by the parser), so any mismatch between mapping keys and getFieldValue cases results in empty strings and skipped fields.
- jablib/src/main/java/org/jabref/logic/openoffice/CSLItemTypeDefinitions.java[196-200]
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationData.java[63-99]
[maintainability] `Location` field name nonstandard
`Location` field name nonstandard
The new `ItemData` field is named `Location` (capitalized), which deviates from standard Java field naming conventions and reduces clarity at call sites. This increases maintenance risk and makes the code look like it references a type rather than a field.A newly introduced field in ItemData is named Location (capital L). This is inconsistent with Java naming conventions for fields and harms readability.
The field is used in getFieldValue for event-place/publisher-place.
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationData.java[45-48]
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationData.java[73-75]
[correctness] Mapped fields never populated
Mapped fields never populated
CSLItemTypeDefinitions declares mappings for CSL fields like "note" (COMMON_FIELDS) and "section" (hearing) that ZoteroCitationData.ItemData does not deserialize or expose via getFieldValue. Those mapped BibTeX fields can never be imported into the resulting BibEntry even when present in the CSL JSON.Some CSL field mappings are currently dead/unreachable because ZoteroCitationData.ItemData doesn’t model them and getFieldValue doesn’t return them.
Concrete examples in the current PR:
-
COMMON_FIELDSmaps"note"→StandardField.NOTE, butItemDatahas nonotefield and nogetFieldValue("note")case. -
hearingmaps"section"→StandardField.ORGANIZATION, butItemDatahas nosectionfield and nogetFieldValue("section")case.
ZoteroCitationMarkParser now populates fields exclusively via CSLItemTypeDefinitions.getFieldMappings(type) + itemData.getFieldValue(mappingKey); therefore any mapping key not supported by ItemData will always result in a blank value and be skipped.
- jablib/src/main/java/org/jabref/logic/openoffice/CSLItemTypeDefinitions.java[80-86]
- jablib/src/main/java/org/jabref/logic/openoffice/CSLItemTypeDefinitions.java[178-184]
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationData.java[16-99]
Choose one:
- Implement the missing CSL fields end-to-end:
- Add
String note = "";(andString section = "";if you keep that mapping) toItemData. - Add corresponding
getFieldValueswitch cases (e.g.,case "note" -> note;,case "section" -> section;).
- If these fields are intentionally not supported yet, remove/disable their mappings in
CSLItemTypeDefinitionsuntil the JSON model supports them (to avoid misleading future maintenance).
| PR 15938 (2026-06-09) |
[maintainability] Hardcoded border color
Hardcoded border color
The new `.bordered` CSS class hardcodes `-fx-border-color: black`, which is not theme-aware and can look wrong in dark/custom themes compared to the rest of the theme that uses variables (e.g., `-fx-outer-border`, `-jr-separator`). This undermines the intent of moving styling into theme CSS with consistent, overridable tokens.The .bordered class introduced in jabref-theme.css hardcodes black as border color. This is inconsistent with the rest of JabRef’s theming approach (which relies heavily on CSS variables like -fx-outer-border / -jr-*) and may reduce contrast or visual consistency in dark/custom themes.
JabRef already defines theme variables and uses them widely for borders/backgrounds. A generic utility class like .bordered should follow the same convention.
- jabgui/src/main/resources/org/jabref/gui/jabref-theme.css[70-86]
Change .bordered to use a theme token, e.g.:
-
-fx-border-color: -fx-outer-border;(matches existing usage) or -
-fx-border-color: -jr-separator;/ another existing-jr-*border token if that better matches the design system. Keep-fx-border-width: 1;unchanged unless a token exists for width.
[maintainability] Misnamed padding class
Misnamed padding class
The new CSS class name `.padding-left-medium` contradicts the padding it applies (`-fx-padding: 0 15 0 0`), which is used elsewhere in the theme file as “right padding” based on the same 4-value padding convention. This naming mismatch makes the class easy to misuse and increases the chance of layout mistakes when reused..padding-left-medium is defined with -fx-padding: 0 15 0 0, which (by the same convention used elsewhere in JabRef CSS) represents right padding, not left padding. The class name is therefore misleading.
The class is already referenced from NewEntry.fxml for the idJumpLink hyperlink, so the name will be encountered and likely reused.
- jabgui/src/main/resources/org/jabref/gui/jabref-theme.css[83-85]
- jabgui/src/main/resources/org/jabref/gui/newentry/NewEntry.fxml[113-119]
Pick one:
- If the intent is right padding (matches the current values), rename to something like
.padding-right-medium(or.padding-right-15) and updateNewEntry.fxmlaccordingly. - If the intent is left padding, keep the name but change the values to
-fx-padding: 0 0 0 15;and verify the UI still looks correct.
| PR 15937 (2026-06-09) |
[reliability] Missing 404 escaping test
Missing 404 escaping test
The PR changes the HTML entry-preview 404 path to escape user-controlled path parameters, but no test was added to assert the 404 HTML response reflects `id`/`entryId` in escaped form. This leaves the XSS fix vulnerable to regression without CI coverage.The XSS fix escapes id/entryId in the NotFoundException message for the text/html entry preview, but there is no regression test covering the 404/error response body.
There is already a test asserting field values in the success HTML body are escaped, but the error-path reflection was the reported vulnerability.
- jabsrv/src/main/java/org/jabref/http/server/resources/EntryResource.java[105-109]
- jabsrv/src/test/java/org/jabref/http/server/EntryResourceTest.java[25-38]
[security] Unescaped library-id error
Unescaped library-id error
EntryResource.getHTMLRepresentation escapes `id`/`entryId` only in the “entry not found” branch, but `getDatabaseContext(id)` can throw a NotFoundException whose message concatenates the raw (user-controlled) library id. That leaves an unescaped error-message path reachable from the same `text/html` endpoint, so user input can still be reflected in an error response.getHTMLRepresentation now escapes the path params when the citation key is missing, but the same request can still throw a NotFoundException from ServerUtils.getBibDatabaseContext that embeds the raw id in the exception message. Since the HTML endpoint is explicitly treating exception messages as part of a text/html response, this is an unescaped reflection path that should be closed too.
-
getHTMLRepresentationcallsgetDatabaseContext(id)before checking the entry list. -
getDatabaseContextdelegates toServerUtils.getBibDatabaseContext, which throwsnew NotFoundException("No library with id " + id + " found"). -
GlobalExceptionMapperreturnsWebApplicationExceptionresponses without any additional escaping/sanitization.
- jabsrv/src/main/java/org/jabref/http/server/resources/EntryResource.java[102-110]
- jabsrv/src/main/java/org/jabref/http/server/resources/EntryResource.java[192-195]
- jabsrv/src/main/java/org/jabref/http/server/services/ServerUtils.java[60-79]
- jabsrv/src/main/java/org/jabref/http/dto/GlobalExceptionMapper.java[16-21]
Apply HTML-escaping to the id in the library-not-found exception message too (either by escaping in ServerUtils.getBibDatabaseContext when constructing the message, or by catching that NotFoundException in EntryResource.getHTMLRepresentation and rethrowing with an escaped message for the HTML representation).
| PR 15921 (2026-06-08) |
[reliability] `prefs.get(..., null)` in migration
`prefs.get(..., null)` in migration
The new migration code passes `null` as the default value to `prefs.get(...)` to test presence, which violates the project’s preference to avoid passing/using `null` when alternatives exist. This can be replaced with `prefs.hasKey(...)` and/or `prefs.getAsOptional(...)` to keep nullability explicit and safer.PreferencesMigrations.upgradeImportFileAndDirePatterns and migrateFileImportPattern call prefs.get(..., null) which passes null as an argument. The compliance checklist requires avoiding passing/using null where reasonable.
JabRefCliPreferences provides non-null alternatives such as hasKey(String) and getAsOptional(String).
- jabgui/src/main/java/org/jabref/migrations/PreferencesMigrations.java[205-231]
| PR 15916 (2026-06-07) |
[maintainability] Typo in test method name
Typo in test method name
The newly added test method name `unmigatedFetcherThrowsOnRawQuery` contains a spelling error (`unmigated`), which reduces readability and makes the test harder to search for consistently. This violates the requirement for meaningful names without typos and should be corrected since it’s newly introduced.The newly added test method name is misspelled: unmigatedFetcherThrowsOnRawQuery should be renamed to unmigratedFetcherThrowsOnRawQuery to meet the requirement for correctly spelled, meaningful method names.
Correct spelling improves readability, maintainability, and searchability of tests, and this is a minor issue that is easy to fix as part of the same PR (including updating any references if present).
- jablib/src/test/java/org/jabref/logic/importer/SearchBasedFetcherTest.java[101-106]
| PR 15914 (2026-06-07) |
[maintainability] Missing requirement for hover preview
Missing requirement for hover preview
This PR adds a new user-facing feature (citation preview tooltip on hover) but does not add a corresponding OpenFastTrace requirement under `docs/requirements/`. This breaks requirements tracing expectations for new features and reduces long-term maintainability/auditability.A new feature (citation preview tooltip on hover in the Entry Editor's "Citations" tab) was introduced without adding a corresponding OpenFastTrace requirement in docs/requirements/<area>.md.
The PR adds hover-based citation preview behavior in the GUI and documents it in the changelog, which indicates this is a user-visible feature that should be traceable via an OpenFastTrace requirement.
- docs/requirements/entry-editor.md[4-15]
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[583-589]
- CHANGELOG.md[14-14]
[performance] Eager `orElse(new BibDatabaseContext())`
Eager `orElse(new BibDatabaseContext())`
The new hover handler uses `stateManager.getActiveDatabase().orElse(new BibDatabaseContext())`, which eagerly allocates a `BibDatabaseContext` on every hover even when an active database is present. This is non-idiomatic Optional usage on a hot UI path and can add unnecessary work/GC pressure when moving across many items.The hover handler currently calls stateManager.getActiveDatabase().orElse(new BibDatabaseContext()), which eagerly allocates a new BibDatabaseContext every time the handler runs, even when the Optional is present, creating unnecessary allocations on a frequent onMouseEntered UI path.
This code executes on onMouseEntered for each rendered citation item, making it a hot path where repeated allocations can contribute to GC pressure/jank. The compliance checklist (PR Compliance ID 8) requires fluent Optional handling and avoiding orElse(default) fallbacks that are never (or almost never) needed.
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[594-598]
[maintainability] Duplicated PreviewViewer tooltip setup
Duplicated PreviewViewer tooltip setup
The PR adds new tooltip initialization logic (creating `PreviewViewer`, calling `resizeForTooltipContent()`, and listening for `Worker.State.SUCCEEDED` to resize the tooltip) that duplicates the existing preview-tooltip pattern already implemented elsewhere. This increases maintenance cost and risks divergence of tooltip behavior across the UI.Tooltip/preview initialization logic is duplicated instead of reusing a shared component.
There is already an existing preview-tooltip implementation (MainTableTooltip) that sets up a PreviewViewer for tooltips and performs sizeToScene() on successful web-engine load. The PR reintroduces a very similar setup inside CitationRelationsTab.
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[180-188]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTableTooltip.java[16-33]
[reliability] Stale preview race
Stale preview race
Hovering triggers asynchronous preview generation for each entered item; if the mouse moves quickly between items, earlier background tasks can finish later and overwrite the tooltip with a preview for the wrong entry. This can show incorrect citation previews and wastes background work even when the tooltip never ends up being shown.CitationRelationsTab updates a shared PreviewViewer on onMouseEntered, which triggers async preview generation. PreviewViewer.update() applies results without verifying the entry is still current, so out-of-order task completion can display stale content.
PreviewViewer.update() captures currentEntry but calls setPreviewText(previewText) unconditionally in onSuccess/onFailure. It only guards against staleness in the cover-download refresh path.
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[594-600]
- jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[200-221]
Option A (recommended):
- In
PreviewViewer.update(), introduce a monotonically increasing request id (or capture bothentryanddatabaseContext) and inonSuccess/onFailureonly apply results if the captured values still equal the currentthis.entryandthis.databaseContext. Option B (mitigation): - Move the
setLayout/setDatabaseContext/setEntrycalls fromonMouseEnteredto a tooltip lifecycle hook (e.g.,previewTooltip.setOnShowing(...)) and/or debounce, to reduce the number of background tasks started while the user is just moving the cursor.
| PR 15913 (2026-06-07) |
[maintainability] `ExportService` prints hardcoded text
`ExportService` prints hardcoded text
`ExportService` and the new Picocli options (`--output`, `--output-format`) emit hardcoded, non-localized English user-facing text, and `ExportService` also constructs a localized warning via string concatenation. This prevents translation and breaks placeholder-based localization formatting required for CLI output and help text.ExportService prints user-facing messages that are not localized and builds localized output by concatenating strings, and the Picocli option help texts for --output and --output-format are hardcoded in English.
Compliance requires all user-facing CLI text (including Picocli help/usage output) to go through Localization.lang(...) and to avoid constructing localized messages by concatenating strings; instead, define a single localized message with placeholders and store the English defaults in jablib/src/main/resources/l10n/JabRef_en.properties.
- jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[127-129]
- jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[142-146]
- jabkit/src/main/java/org/jabref/toolkit/commands/GetCitedWorks.java[37-48]
- jabkit/src/main/java/org/jabref/toolkit/commands/GetCitingWorks.java[37-48]
- jablib/src/main/resources/l10n/JabRef_en.properties[328-334]
[maintainability] `ExportServiceTest` catches exceptions
`ExportServiceTest` catches exceptions
The new test catches `ExportServiceException` via a manual try/catch block instead of using JUnit’s `assertThrows`. This violates the project’s test conventions and can reduce clarity of test intent.A unit test catches exceptions with a try/catch instead of using assertThrows.
Project test conventions require not catching exceptions in tests; JUnit should handle them and assertions should be expressed via assertThrows.
- jabkit/src/test/java/org/jabref/toolkit/service/ExportServiceTest.java[58-72]
[correctness] Check ignores findings code
Check ignores findings code
`Check.checkConsistency()` and `Check.checkIntegrity()` ignore the `int` returned by `CheckConsistency.execute(...)` / `CheckIntegrity.execute(...)` and always return `ExitCode.OK` when no exception is thrown, so `jabkit check FILE` can exit 0 even when findings exist (which should exit 1).Check.checkConsistency() and Check.checkIntegrity() currently discard the return value of the underlying execute(...) methods and always return ExitCode.OK on success.
This breaks the CLI contract that jabkit check <file> should exit with 1 when findings exist (and 0 only when clean).
Both CheckConsistency.execute(...) and CheckIntegrity.execute(...) still return int exit codes indicating findings vs. clean; the wrapper methods must propagate those values.
- jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[61-79]
[correctness] Export logs break porcelain
Export logs break porcelain
`ExportService.tryExportWithExporter(...)` always prints an "Exporting …" line, which (a) produces duplicate output because `exportParserResultToFile(...)` already prints, and (b) emits status text even when callers request `--porcelain` output.ExportService.tryExportWithExporter(...) prints to stdout unconditionally. This causes:
- Duplicate "Exporting" messages (one in
exportParserResultToFile, one intryExportWithExporter). -
--porcelainoutput to be polluted with status text (e.g.,ConvertpassessharedOptions.porcelain, but it is not respected).
Porcelain is documented as "script-friendly output" and should avoid non-data status lines.
- jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[173-211]
- jabkit/src/main/java/org/jabref/toolkit/commands/Convert.java[53-64]
[correctness] BibTeX exporter ignores entries
BibTeX exporter ignores entries
The custom `bibtexExporter` ignores the `entries` parameter and always saves the full `BibDatabaseContext`, so any call site that exports a subset via `exportBibDatabaseContextToFile(context, subset, ...)` can accidentally export more entries than requested.The custom BibTeX exporter implementation ignores the entries list passed to Exporter.export(...). This violates the exporter contract (export the provided subset) and can lead to exporting unintended entries when a BibDatabaseContext contains more entries than the subset list.
The Exporter API explicitly passes List<BibEntry> entries to allow subset exporting.
- jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[61-67]
- jabkit/src/main/java/org/jabref/toolkit/service/ExportService.java[151-171]
| PR 15908 (2026-06-05) |
[correctness] Wrong setAll update order
Wrong setAll update order
ImporterPreferences#setAll updates apiKeys before persistCustomKeys, but JabRefCliPreferences stores/clears OS keyring entries on apiKeys changes based on the current persist flag, so reset/import can leave stale keys in the keyring or store keys when persistence is being turned off. This can break the expected semantics of Preferences reset/import for custom API keys.ImporterPreferences#setAll currently calls setApiKeys(...) before updating persistCustomKeys. Because JabRefCliPreferences attaches an InvalidationListener to ImporterPreferences#getApiKeys(), modifying the set triggers storeFetcherKeys(...), which decides whether to write/delete keyring entries based on shouldPersistCustomKeys(). With the current order, storeFetcherKeys(...) can run using the previous persist flag value during preference reset/import.
-
clear()andimportPreferences()callgetImporterPreferences().setAll(...). -
getApiKeys()changes trigger keyring store/clear.
- jablib/src/main/java/org/jabref/logic/importer/ImporterPreferences.java[118-130]
Suggested fix: In
setAll, setpersistCustomKeysbefore callingsetApiKeys(...)(matching the ordering already documented/used inWebSearchTabViewModel.storeSettings).
[correctness] Custom importers not loaded
Custom importers not loaded
JabRefCliPreferences#getCustomImportFormats always returns defaults because it checks for the exact key "customImportFormat", but the data is stored under numbered keys ("customImportFormat0", "customImportFormat1", …). This causes saved custom importers to be ignored after restart/import and effectively loses user configuration.getCustomImportFormats checks hasKey(IMPORTER_CUSTOM_FORMAT) (i.e., customImportFormat) and returns defaults if absent. However, the series is stored under customImportFormat0, customImportFormat1, ... so the guard prevents loading custom importers.
-
hasKeychecks only an exact key in the backing store. -
getSeries(prefix)readsprefix + N. -
storeCustomImportFormatswritesprefix + i.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2132-2154]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2157-2162]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[589-591]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[781-789]
Replace the guard with a check that matches the storage format, e.g.:
-
if (!hasKey(IMPORTER_CUSTOM_FORMAT + "0")) return defaults;or - remove the
hasKeyguard and instead do: List<String> series = getSeries(IMPORTER_CUSTOM_FORMAT);if (series.isEmpty()) return defaults;- parse
series.
[correctness] Catalog defaults not applied
Catalog defaults not applied
JabRefCliPreferences#getImporterPreferencesFromBackingStore ignores the provided defaults for catalogs by calling getStringList(IMPORTER_CATALOGS) without falling back to defaults.getCatalogs() when the preference is absent/blank. On fresh profiles or incomplete preference imports, importer catalogs become empty, disabling web search defaults in the GUI (JabRefGuiPreferences extends JabRefCliPreferences).getImporterPreferencesFromBackingStore(ImporterPreferences defaults) currently loads catalogs using getStringList(IMPORTER_CATALOGS) without applying the passed-in defaults when the key is missing/blank, resulting in an empty catalog list on first run or when imported preferences omit the key.
-
getImporterPreferences()passesImporterPreferences.getDefault()intogetImporterPreferencesFromBackingStore(...). -
getStringList(key)usesget(key)internally; if the key is absent, it can yield an empty list. - JabRef GUI preferences inherit this behavior because
JabRefGuiPreferences extends JabRefCliPreferences.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2095-2130]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[593-595]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[675-690]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[78-79]
In getImporterPreferencesFromBackingStore:
- Compute catalogs with a fallback:
List<String> catalogs = getStringList(IMPORTER_CATALOGS);if (catalogs.isEmpty()) { catalogs = defaults.getCatalogs(); }- pass
catalogsinto theImporterPreferencesconstructor. Alternative: introduce a helpergetStringList(key, List<String> defaultList)that usesget(key, convertListToString(defaultList)).
| PR 15899 (2026-06-03) |
[correctness] Artifact permissions too restrictive
Artifact permissions too restrictive
The workflow sets `permissions` to only `contents: read` but uses `actions/upload-artifact` and `actions/download-artifact`, which can fail when the Actions permission scope isn’t granted. This can prevent the smoke-test job from receiving the built binary and make the workflow non-functional.The workflow restricts GITHUB_TOKEN permissions to contents: read, but it uploads and downloads artifacts. Artifact APIs may require actions permission, so the workflow can fail at artifact upload/download.
Other workflows in this repository that upload artifacts grant actions: write, indicating the repo expects explicit actions permissions when artifacts are used.
- .github/workflows/jabkit-native-smoke-test.yml[6-8]
Update the workflow permissions to include at least actions: write (or actions: read + job-level overrides for upload steps), e.g.:
[observability] Smoke test hides stderr
Smoke test hides stderr
The clitest smoke test redirects stderr from `jabkit --help` to `/dev/null`, which removes the most useful diagnostics if the native binary fails to start or prints errors on stderr. This makes CI failures harder to debug.The smoke-test command discards stderr (2>/dev/null). When the binary fails to initialize, stderr often contains the actionable failure reason; discarding it makes CI failures opaque.
JabKit prints user-facing error messages to stderr in some failure paths.
- jabkit/src/nativeTest/clitest/jabkit.md[1-3]
Remove the stderr redirection, or capture it to a file and print it on failure. Example:
| PR 15898 (2026-06-03) |
[correctness] Git option bypass
Git option bypass
The guard only triggers when the raw command contains the literal substring "git commit", so valid commit invocations with git global options in between (e.g., `git -c ... commit ...`) are not checked and will bypass the deny. This leaves a realistic path for the same malformed here-string commit to still be created.commit-msg-heredoc-guard.py uses a raw substring check ("git commit" in command) to decide whether to inspect a command. This misses real git commit invocations when git global options (e.g., -c, -C, --no-pager) appear between git and commit, causing false negatives.
This repo already has a PreToolUse hook (git-rebase-guard.py) that tokenizes the command and locates the git subcommand while skipping global options.
- .claude/hooks/commit-msg-heredoc-guard.py[37-45]
- .claude/hooks/git-rebase-guard.py[18-34]
[reliability] Quote-unaware regex deny
Quote-unaware regex deny
The opener regex is applied to the raw command string, so it can match `@'`/`@"` sequences that occur inside quoted arguments (e.g., an `echo "git commit -m @'..."` or a commit message that literally mentions PowerShell here-strings) and deny even though bash will treat them as literal text. This makes the hook prone to false positives compared to the repo’s other hook which tokenizes to avoid matching inside quoted strings.The here-string detection runs regex over the full raw command, without understanding shell quoting. As a result, @' / @" inside quoted literals can still match and cause a deny, even though they are not PowerShell here-string syntax in that context.
git-rebase-guard.py explicitly uses shlex.split(..., posix=True) so matches inside quoted strings/heredoc bodies are not flagged.
- .claude/hooks/commit-msg-heredoc-guard.py[40-45]
- .claude/hooks/git-rebase-guard.py[9-12]
- .claude/hooks/git-rebase-guard.py[98-103]
| PR 15896 (2026-06-03) |
[reliability] `inputs.gradleCommand` undefined in action
`inputs.gradleCommand` undefined in action
The composite GitHub Action `.github/actions/package` no longer defines the `gradleCommand` input but still references `inputs.gradleCommand` in the Smoke test (and ShadowJar) steps, which can result in an empty/invalid command and deterministic workflow failures. This undermines consistent, configured Gradle invocation in CI.The composite action .github/actions/package/action.yml removed the gradleCommand input, but some steps still reference ${{ inputs.gradleCommand }}, which can evaluate to an empty string and produce an invalid Gradle command, breaking CI (including the binaries workflow).
The gradleCommand input was removed to standardize on a single Gradle invocation, and .github/workflows/binaries.yml no longer passes gradleCommand into the action. The action already hardcodes a Gradle invocation for the jpackage step, so the remaining ${{ inputs.gradleCommand }} usages should be updated to match that standardized approach (or the input should be reintroduced if standardization is not desired).
- .github/actions/package/action.yml[4-40]
- .github/actions/package/action.yml[71-82]
- .github/actions/package/action.yml[129-139]
| PR 15895 (2026-06-03) |
[correctness] Staged/untracked changes ignored
Staged/untracked changes ignored
The new check-formatting composite action is documented as failing on “uncommitted changes”, but the underlying script treats an empty `git diff` (unstaged only) as a clean tree. This means staged changes or untracked files can exist without failing CI, undermining the intent of the formatting/OpenRewrite gate.check-formatting relies on .github/format-diff-annotations.sh, which currently uses plain git diff and exits 0 when that output is empty. This only detects unstaged modifications, so staged changes and untracked files can still be present while CI reports “clean”.
The action’s description and usage implies it gates on “uncommitted changes”, so the check should cover at least:
- unstaged changes
- staged changes
- (optionally) untracked files
- .github/format-diff-annotations.sh[21-26]
- .github/actions/check-formatting/action.yml[34-43]
- .github/actions/check-formatting/action.yml[1-5]
[maintainability] Misleading annotation comment
Misleading annotation comment
The script header comment still says the message appends only `lines -`, but flush() now emits `line N` for single-line hunks. This mismatch can confuse future maintenance/debugging of the annotation output format.The header comment describing the appended range format is now inaccurate after changing the output to use line N for single-line hunks.
Keeping this comment correct matters because it documents the externally-visible annotation message format that developers see in CI.
- .github/format-diff-annotations.sh[9-14]
- .github/format-diff-annotations.sh[64-75]
| PR 15880 (2026-06-02) |
[correctness] Wrong file on deletions
Wrong file on deletions
The script only updates `file` on `+++ b/...`, so diffs that use `+++ /dev/null` (file deletions) will keep the previous file value and emit annotations against the wrong path. This can make CI annotations misleading/incorrect, especially in the OpenRewrite job that also uses this script..github/format-diff-annotations.sh sets file only when matching +++ b/.... For deletion diffs (+++ /dev/null), file is never updated, so subsequent ::error file=... annotations can be emitted for the previous file.
This script is used in CI to annotate formatting and OpenRewrite diffs. OpenRewrite can produce diffs including file deletions, so +++ /dev/null is a realistic case to support.
- Update parsing to reliably set
filefor every file-level diff, including deletions (e.g., parsediff --git a/... b/...and/or handle+++ /dev/null). - file/path focus:
- .github/format-diff-annotations.sh[61-64]
[correctness] Add-only hunks mis-anchored
Add-only hunks mis-anchored
The new hunk walker tracks only old-side line numbers and no longer parses `+newStart,+newCount`, so hunks with `oldCount=0` (pure insertions / new-file-style hunks) collapse to a single old-side line (or line 1 when oldStart=0). This breaks the “annotate only changed lines” behavior for insert-only changes by pointing at an incorrect range.The script only parses the old-side start ($2) and maintains only an oldLine counter. For addition-only hunks (@@ -X,0 +Y,N @@), annotations cannot be anchored to the actual changed new-side lines and instead collapse to a single old-side line number.
CI git diff output can include hunks where oldCount=0 (pure insertions). In those cases, the annotation should use the new-side range (newStart..newStart+N-1) or otherwise provide a meaningful range that corresponds to committed code developers can edit.
- Parse both sides from the hunk header (
-oldStart,oldCountand+newStart,newCount). - Maintain both
oldLineandnewLinecounters while scanning hunk body. - When a run contains no old-side consumption (or
oldCount==0), anchor the annotation on the new-side range. - file/path focus:
- .github/format-diff-annotations.sh[65-82]
[correctness] Quoted paths not parsed
Quoted paths not parsed
The awk parser only matches unquoted `--- a/...` and `+++ b/...` headers, so when `git diff` emits quoted headers (e.g., `--- "a/path with spaces"`) the script may keep a stale `file` value (or empty) and emit `::error` annotations against the wrong path. This makes CI annotations misleading/unactionable for changed files whose names require quoting (spaces/special characters)..github/format-diff-annotations.sh extracts the file path only from unquoted diff headers (--- a/... and +++ b/...). When git diff quotes paths (commonly for filenames containing spaces), those regexes don’t match, so fileRaw/file won’t be updated and subsequent annotations can point at the previous file or have an empty file=.
Concrete example of headers that won’t match today:
--- "a/config/Eclipse Code Style.epf"+++ "b/config/Eclipse Code Style.epf"
This repository contains tracked files with spaces in their path, so this is not purely theoretical.
- .github/format-diff-annotations.sh[64-71]
- Add additional header matchers for quoted paths, e.g.:
-
^--- "a/… strip leading--- "a/and trailing" -
^\+\+\+ "b/… strip leading+++ "b/and trailing"(Optionally handle backslash-escaped quotes within the name if you want to be extra robust.)
- Consider resetting
fileRaw/fileon^diff --gitto avoid accidentally reusing the previous file if a header isn’t recognized. - Optionally, make
flush()a no-op iffileis empty to avoid emitting invalid annotations.
| PR 15876 (2026-06-01) |
[maintainability] Ambiguous modules.properties location
Ambiguous modules.properties location
The new step says to add a mapping to `modules.properties` but then instructs to edit `gradle/modules.properties`, and also links to an upstream `modules.properties` file; this mixes three different identifiers and can send contributors to the wrong place. This is likely to cause wasted time or incorrect documentation-driven changes when resolving module mapping issues.The ordered-list item introduces modules.properties without a path, but the instructions refer to gradle/modules.properties and also mention an upstream modules.properties in the Gradlex project. This is ambiguous and can mislead contributors about where they should add the mapping.
- The repository contains the local mapping file at
gradle/modules.properties. - The docs also link to the upstream Gradlex mapping file; this should be clearly labeled as upstream.
- docs/code-howtos/dependency-management.md[78-85]
- Update the list item label to explicitly mention the local path, e.g.
Add missing mapping to \\gradle/modules.properties\``. - Reword the “Furthermore…” sentence to explicitly call out that the link is to the upstream Gradlex mapping file (and optionally change the link text to remove the
...placeholder-like segment).
| PR 15874 (2026-06-01) |
[reliability] Empty cleanup jobs crash
Empty cleanup jobs crash
`JabRefCliPreferences#getCleanupPreferencesFromBackingStore` can throw when `CLEANUP_JOBS` exists but is blank/empty, because it calls `EnumSet.copyOf(...)` on an empty collected set. This can break preferences loading (and potentially app startup) after users disable all cleanup steps, since the key can remain present with an empty value.getCleanupPreferencesFromBackingStore builds an EnumSet via EnumSet.copyOf(collectedSet) when hasKey(CLEANUP_JOBS) is true. If the stored list is empty/blank, convertStringToList returns an empty list, leading to an empty Set and an exception from EnumSet.copyOf(...).
This is not prevented by the current hasKey(CLEANUP_JOBS) guard, because hasKey returns true even when the stored value is an empty string.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1814-1833]
- In
getCleanupPreferencesFromBackingStore, after parsing the list, if it is empty, returnEnumSet.noneOf(CleanupStep.class)(or fall back to defaults) instead of callingEnumSet.copyOf. - Optionally also adjust the active-jobs listener: if
cleanupPreferences.getActiveJobs()is empty,remove(CLEANUP_JOBS)instead of writing an empty string, to keephasKeyfalse for the empty case.
[correctness] Empty focused path persisted
Empty focused path persisted
`LastFilesOpenedPreferences.getDefault()` uses `Path.of("")` to represent “no last focused file”, but `JabRefCliPreferences` persists any non-null focused path via `toAbsolutePath()`, turning the empty path into the current working directory. Because `clear()` now resets last-opened prefs via `setAll(getDefault())`, a reset can incorrectly write the current directory as `LAST_FOCUSED` and also load it when the key is missing.LastFilesOpenedPreferences uses an empty Path as a sentinel for “no last focused file”. Persistence treats any non-null Path as valid and stores toAbsolutePath(), which converts the empty path to the current working directory; after this PR, clear() calls setAll(LastFilesOpenedPreferences.getDefault()), so reset/clear can persist an unintended focused path.
The persistence layer already uses null as the signal to remove LAST_FOCUSED, so using an empty Path defeats that contract.
- jablib/src/main/java/org/jabref/logic/preferences/LastFilesOpenedPreferences.java[23-35]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[841-879]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1869-1894]
- Change
LastFilesOpenedPreferences.getDefault()to usenullforlastFocusedFile. - In
getLastFilesOpenedPreferencesFromBackingStore, only create aPathwhenhasKey(LAST_FOCUSED)and the stored string is non-blank; otherwise setlastFocusedFiletonull. - In the
lastFocusedFilePropertylistener, treat blank/empty paths the same asnull(remove the key) to avoid persisting a sentinel value.
| PR 15873 (2026-06-01) |
[security] Whitespace collapses PR number
Whitespace collapses PR number
The `Read pr_number.txt` step deletes *all* whitespace from the artifact (`tr -d '[:space:]'`), so malformed content like `12 3` becomes `123` and passes the numeric regex check. This can cause the workflow to target/comment on the wrong PR while still treating the artifact as valid.The workflow currently uses tr -d '[:space:]' to remove all whitespace from pr_number.txt before validating it. This can change the PR number by concatenating digits across spaces/tabs/newlines, allowing malformed content to become a different numeric value and still pass validation.
The artifact is produced as a single-line PR number (with a trailing newline from echo). The validation should therefore accept only a single line containing digits (optionally ending with \r\n), and reject any additional characters/lines/whitespace rather than deleting them.
- .github/workflows/pr-comment.yml[64-80]
- Read exactly the first line (
IFS= read -r PR_NUMBER < "$ARTIFACT_FILE"). - Strip only a trailing carriage return (
PR_NUMBER="${PR_NUMBER%$'\r'}"). - Reject if the file has more than one line (e.g.,
tail -n +2 "$ARTIFACT_FILE" | grep -q .→ invalid). - Validate with a strict digits-only check (e.g.,
[[ "$PR_NUMBER" =~ ^[0-9]+$ ]]). - Do not remove internal whitespace (no
tr -d '[:space:]').
| PR 15871 (2026-06-01) |
[correctness] Unescaped annotation properties
Unescaped annotation properties
The script emits `::error file=...,line=...,endLine=...` using raw `file` values from `git diff`, so paths containing characters that must be escaped for GitHub Actions workflow-command properties (e.g., `:`, `,`, `%`, newline) can produce malformed annotations or wrong parsing. The repo already contains a dedicated escape utility for exactly this annotation format, so this script is inconsistent with established behavior and can silently drop annotations in edge cases..github/format-diff-annotations.sh prints GitHub Actions workflow commands (::error ...) but does not escape the file property (and generally any property/data field). GitHub Actions requires percent-encoding for special characters in both the message data and in key=value properties; otherwise annotations can be parsed incorrectly or ignored.
The codebase already has an established escaping implementation (GitHubActionsEscape.property/data). The bash script should apply the same escaping rules when printing file=... (and any other dynamic property/data fields).
Existing Java-based annotation writers escape both properties and data explicitly, including : and , for properties.
- .github/format-diff-annotations.sh[19-30]
- jablib/src/main/java/org/jabref/logic/util/GitHubActionsEscape.java[15-25]
- Implement an AWK function equivalent to
GitHubActionsEscape.property(escape%,\r,\n, then additionally escape:and,). - Apply it to
filebefore printing. - (Optional) Also escape the message data portion if it ever becomes dynamic in the future.
[correctness] Wrong range for pure additions
Wrong range for pure additions
For pure-addition hunks (`oldCount==0`), the script still uses the *old* hunk range (`-oldStart,oldCount`) and can emit `line=0,endLine=0` (e.g., new files or insertions at the start), producing a meaningless “lines 0-0” message and an annotation that cannot reliably attach to source. For `oldCount==0`, the annotation should use the `+newStart,newCount` range (and clamp to 1-based lines).When a diff hunk has oldCount==0 (pure addition / new file), the script currently anchors the annotation using oldStart, which can be 0 and does not represent real lines in the committed file. This can yield line=0,endLine=0 and a confusing message.
The hunk header contains both old and new ranges: @@ -oldStart,oldCount +newStart,newCount @@. If oldCount==0, there are no old lines to annotate, so the correct anchor is the new range.
- .github/format-diff-annotations.sh[21-29]
- Parse
$3(+newStart,newCount) as well as$2. - If
oldCount > 0, keep current behavior (use old range). - Else (pure addition), set
start=newStartandend=newStart+newCount-1. - Ensure
start >= 1andend >= startbefore emitting the annotation.
[observability] Misleading OpenRewrite annotations
Misleading OpenRewrite annotations
The annotation title/text is hardcoded to “Formatting / Run the formatter”, but the workflow also uses this script to fail the OpenRewrite job when it leaves a dirty working tree. When OpenRewrite causes semantic changes or adds/removes files, the annotations will incorrectly tell contributors to “run the formatter” instead of indicating OpenRewrite output needs to be committed/fixed.The script always emits title=Formatting and a message telling users to run the formatter, but it is also invoked from the openrewrite job where the root cause may be OpenRewrite changes rather than formatting.
In .github/workflows/tests-code.yml, the OpenRewrite job runs :rewriteRun, then runs the formatter, then calls .github/format-diff-annotations.sh. Any remaining diff at that point is not necessarily a formatting issue.
- .github/workflows/tests-code.yml[108-129]
- .github/format-diff-annotations.sh[1-33]
- Add optional parameters or env vars to
.github/format-diff-annotations.sh(e.g.,ANNOTATION_TITLE/ANNOTATION_MESSAGE_PREFIX, or a first arg likeformat|openrewrite). - In the OpenRewrite job, set the title/message to something like
OpenRewrite/Uncommitted changes after OpenRewrite; run :rewriteRun and commit results. - Keep the current “Formatting” messaging for the format jobs.
| PR 15870 (2026-06-01) |
[correctness] Immutable exporters list
Immutable exporters list
ExportPreferences#getDefault passes List.of() into FXCollections.observableList, producing an ObservableList backed by an immutable list. Later calls to ExportPreferences#setCustomExporters/setAll will attempt to mutate that list (setAll), causing UnsupportedOperationException and breaking the Custom Exporters preferences flow (and other preference reset/import flows when no custom exporters are stored).ExportPreferences stores customExporters using FXCollections.observableList(customExporters). The default constructor passes List.of(), so the resulting ObservableList is backed by an immutable list. Any later mutation via setAll(...) / setCustomExporters(...) can throw UnsupportedOperationException.
This manifests in GUI when saving the “Custom Exporters” preferences tab because CustomExporterTabViewModel#storeSettings calls preferences.getExportPreferences().setCustomExporters(...), which calls customExporters.setAll(...).
It also affects normal startup/import/reset flows when no custom exporters are stored, because the backing-store loader returns defaults for an empty series.
Ensure customExporters is always backed by a mutable list, e.g.:
- In
ExportPreferencesconstructor, useFXCollections.observableArrayList(customExporters)orFXCollections.observableList(new ArrayList<>(customExporters)). - Keep
setCustomExporters/setAllas-is afterward.
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[33-50]
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[56-61]
- jablib/src/main/java/org/jabref/logic/exporter/ExportPreferences.java[99-105]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1682-1688]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1771-1779]
[maintainability] `get(0)` replaces `getFirst()`
`get(0)` replaces `getFirst()`
The updated export save-order persistence uses `List.get(0)` instead of the modern `SequencedCollection#getFirst()` idiom, regressing from a more modern Java style. This diverges from the project’s stated preference for modern Java 25+ APIs/idioms.Modern Java provides SequencedCollection#getFirst() for readability and consistency; the PR changed first-element access to get(0).
This code previously used getFirst() and the compliance checklist explicitly prefers modern Java 25+ idioms.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1694-1699]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1727-1731]
[maintainability] `keyWordDelimiterProperty` Javadoc wrong
`keyWordDelimiterProperty` Javadoc wrong
The updated parameter comment references `BibEntryProperties#keyWordDelimiterProperty`, which does not exist (and no longer matches the renamed `keywordSeparator` concept). This creates confusing and misleading documentation for maintainers.A Javadoc/parameter comment references a non-existent or outdated API name, making the documentation misleading.
The code now uses keywordSeparator, but the Javadoc still points to BibEntryProperties#keyWordDelimiterProperty.
- jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyPatternPreferences.java[46-46]
| PR 15868 (2026-05-31) |
[security] CLI path check bypass
CLI path check bypass
`isLibraryPathAccessAllowed()` returns `true` for `JabRefSrvStateManager` (stand-alone HTTP server), which makes the new `librarypath` validation effectively a no-op in that mode. As a result, any existing local file path can be opened via `librarypath` without being served or user-approved, defeating the intended hardening.In CAYWResource.isLibraryPathAccessAllowed, stand-alone server mode (srvStateManager instanceof JabRefSrvStateManager) is currently treated as automatically allowed. This bypasses the new guard in getBibDatabaseContext and allows arbitrary librarypath access.
JabRefSrvStateManager is the stand-alone HTTP server state manager (no GUI prompt possible). The new hardening should restrict librarypath to libraries that are already being served (via FilesToServe / open databases), otherwise reject.
- jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java[227-275]
- jabsrv/src/main/java/org/jabref/http/JabRefSrvStateManager.java[21-29]
- jabsrv/src/main/java/org/jabref/http/server/services/ServerUtils.java[55-90]
- Change
isLibraryPathAccessAllowedso that forJabRefSrvStateManagerit returnsfalse(or otherwise never grants access to unserved paths). - Ensure the
GraphicsEnvironment.isHeadless()rejection logic is actually reachable for headless/stand-alone mode. - (Optional) Adjust
INVALID_LIBRARY_PATH_ERRORtext to reflect “not served / not permitted” if you keep a separate allow path in GUI mode.
| PR 15867 (2026-05-31) |
[reliability] Date parsing can crash
Date parsing can crash
ZoteroCitationMarkParser#setDate and helpers call List.getFirst() and Integer.parseInt() on unvalidated CSL "date-parts" contents, so empty inner arrays or non-numeric parts can throw NoSuchElementException/NumberFormatException. These exceptions are not caught by parse() (it only catches JsonParseException), so parsing can fail hard and bubble up to callers.ZoteroCitationMarkParser#setDate assumes issued.date-parts is well-formed and numeric. It uses getFirst() and Integer.parseInt() without validating inner list contents, which can throw runtime exceptions that are not caught by parse().
This parser is intended to consume Zotero-provided CSL JSON embedded in a reference mark name. Real-world data can be incomplete or partially missing (e.g., "date-parts":[[]]) or contain unexpected tokens; the parser should degrade gracefully (e.g., omit date fields) rather than throw.
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[39-50]
- jablib/src/main/java/org/jabref/logic/openoffice/ZoteroCitationMarkParser.java[107-164]
- In
setDate(...): - Guard against
issuedData == null. - After
issuedData.datePartsnon-empty, also validate the first inner list is non-null and non-empty before callinggetFirst(). - Replace direct
Integer.parseInt(...)calls with safe parsing (e.g., try/catchNumberFormatExceptionand return/skip setting month/day), so malformed values don’t crash parsing. - Consider broadening the
parse(...)catch to includeRuntimeException(or at leastNoSuchElementException/NumberFormatException) and returnList.of()after logging at debug level. - Add tests for edge cases such as
"date-parts":[[]]and non-numeric date parts to prevent regressions.
| PR 15863 (2026-05-30) |
[correctness] `See what's new` typo
`See what's new` typo
The new Russian translation contains a spelling error (`Узнайть`) which will be shown to users in the UI. This violates the UI text style guideline to keep spelling consistent and professional.The Russian UI translation for See what's new contains a spelling typo (Узнайть).
This string is user-facing and should follow UI text style guidelines (correct spelling).
- jablib/src/main/resources/l10n/JabRef_ru.properties[1665-1665]
[correctness] `Move URL...` missing quote
`Move URL...` missing quote
The new Russian translation has unbalanced quotes around the field name `note` for the string `Move URL in 'note' field to 'URL' field`, which makes the Cleanup checkbox label render with malformed quoting and look unpolished to users. This violates the UI text style guideline (PR Compliance ID 20) requiring consistent, correct punctuation in user-facing UI text.The Russian translation for Move URL in 'note' field to 'URL' field is missing the closing ' after note, causing unbalanced quotes and a malformed label shown to users.
This is user-facing UI text and must have correct punctuation/quoting to comply with PR Compliance ID 20 (consistent, correct UI text). The key is used as a UI label in the Cleanup entries panel (FXML), and JabRef’s localization performs %0/%1 placeholder substitution but does not interpret quotes specially, so this is purely a visible string defect.
- jablib/src/main/resources/l10n/JabRef_ru.properties[1214-1214]
| PR 15862 (2026-05-30) |
[correctness] getDefaultPath override blocked
getDefaultPath override blocked
JabRefCliPreferences#getDefaultPath is now private, so JabRefGuiPreferences#getDefaultPath annotated with @Override no longer overrides and the project should not compile. Even if the annotation is removed, GUI-mode defaults (e.g., LAST_USED_DIRECTORY and FilePreferences defaults) will fall back to "/" instead of NativeDesktop’s default directory.JabRefCliPreferences#getDefaultPath() was changed from an overridable method to private, which prevents JabRefGuiPreferences from overriding it (and makes its existing @Override invalid). This breaks GUI-mode default directory behavior (and likely compilation).
JabRefGuiPreferences expects to override getDefaultPath() to use NativeDesktop.get().getDefaultFileChooserDirectory(), but JabRefCliPreferences now defines a private method returning /.
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[952-954]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[1261-1265]
[reliability] Reset crashes clearing truststore
Reset crashes clearing truststore
`clear()` calls `clearTruststoreFromCustomCertificates()`, which constructs a `TrustStoreManager` using `defaults.get(SSL_TRUSTSTORE_PATH).toString()`. The PR removed initialization of `SSL_TRUSTSTORE_PATH` in the `defaults` map, so reset can throw a `NullPointerException` and abort.Preferences reset can crash with an NPE because clearTruststoreFromCustomCertificates() still reads the truststore path from defaults, but the PR removed the defaults.put(SSL_TRUSTSTORE_PATH, ...) initialization.
clear() always calls clearTruststoreFromCustomCertificates(). That method should not depend on a defaults entry that may not exist; it should use getSSLPreferences().getTruststorePath() (with a safe fallback) or SSLPreferences.getDefault().getTruststorePath().
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[749-752]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[488-596]
| PR 15838 (2026-05-27) |
[correctness] `csv.begin.layout` missing `Chapter` header
`csv.begin.layout` missing `Chapter` header
The new CSV header row omits standard BibTeX fields such as `chapter` and `annote`, so the exported CSV cannot include headers for all standard fields as required. This makes the export incomplete and inconsistent with the stated objective.The CSV header template does not include headers for all standard BibTeX fields (e.g., chapter, annote). This violates the requirement that csv.begin.layout emits a complete header row for all standard fields.
StandardField includes standard BibTeX fields like ANNOTE and CHAPTER, but the current csv.begin.layout header line does not include corresponding columns.
- jablib/src/main/resources/resource/layout/csv/csv.begin.layout[1-1]
- jablib/src/main/resources/resource/layout/csv/csv.layout[1-1]
[reliability] `GenericCsvExportFormatTest` lacks key test
`GenericCsvExportFormatTest` lacks key test
The added tests do not verify the full required header set and do not assert citation key export behavior, so they don’t validate the key requirements of the new CSV exporter. This reduces regression protection for the new export format.The new CSV exporter tests only check that the header contains a handful of substrings and never assert citation key output behavior. The compliance requirement expects automated tests to validate headers (for all standard fields) and citation key behavior.
performExportContainsAllStandardFieldHeaders currently asserts only a few header fragments, and the tests never set a citation key (e.g., via withCitationKey(...)) to validate how it is exported.
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[46-62]
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[93-111]
[correctness] Unescaped quotes in CSV
Unescaped quotes in CSV
The new `csv.layout` wraps most fields in double quotes but does not escape embedded `"` characters for those fields, producing malformed CSV when values contain quotes. This can break spreadsheet import and can also violate the template README’s “one row per entry” promise when fields contain line breaks.The new generic CSV template quotes values (e.g., "\author") but does not apply CSV escaping for embedded double quotes (and does not normalize newlines), so exported CSV can be malformed or span multiple lines per entry.
CSV requires embedded " inside quoted fields to be escaped as "". JabRef already has ReplaceWithEscapedDoubleQuotes for exactly this purpose, but the new template only applies it to \citationkey.
- jablib/src/main/resources/resource/layout/csv/csv.layout[1-1]
- jablib/src/main/resources/resource/layout/csv/README[1-4]
- jablib/src/main/java/org/jabref/logic/layout/format/ReplaceWithEscapedDoubleQuotes.java[5-18]
- Wrap each exported field in a formatter chain that at least applies
ReplaceWithEscapedDoubleQuotes(and ideally also replaces\nwith a space to keep one row per entry), e.g.: "\format[ReplaceWithEscapedDoubleQuotes]{\author}"- or
"\format[Replace(\n, ),ReplaceWithEscapedDoubleQuotes]{\abstract}" - Consider also quoting/escaping the citation key consistently (currently it is escaped but not quoted).
- Add/extend a unit test to include a field value containing a double quote (and optionally a newline) and assert the output is valid (e.g., contains doubled quotes).
[reliability] Missing exporter test lock
Missing exporter test lock
`GenericCsvExportFormatTest` lacks the `@Execution(SAME_THREAD)` / `@ResourceLock("exporter")` guards used by other exporter tests, so it may run concurrently and become flaky. This is especially risky because `TemplateExporter` mutates global `Number.serialExportNumber` during export.GenericCsvExportFormatTest can run in parallel with other exporter tests, but exporter code mutates global static state, which can lead to non-deterministic failures.
TemplateExporter sets and increments Number.serialExportNumber (a public static int) during export. Existing exporter tests (e.g., CsvExportFormatTest) protect against this by forcing same-thread execution and applying a shared ResourceLock("exporter").
- jablib/src/test/java/org/jabref/logic/exporter/GenericCsvExportFormatTest.java[18-33]
- jablib/src/main/java/org/jabref/logic/exporter/TemplateExporter.java[240-246]
- jablib/src/main/java/org/jabref/logic/layout/format/Number.java[7-19]
- jablib/src/test/java/org/jabref/logic/exporter/CsvExportFormatTest.java[31-33]
- Add the same annotations used by
CsvExportFormatTest: @Execution(ExecutionMode.SAME_THREAD)@ResourceLock("exporter")- and the required imports.
- (Optional) Add an
@AfterEachcleanup like the existing test class, for consistency.
| PR 15837 (2026-05-26) |
[correctness] Duplicate extension on invalid path
Duplicate extension on invalid path
When Path.of(fileName) throws InvalidPathException in getBaseName, getBaseName returns the full filename (including extension), but getValidFileName then appends the extracted extension again, producing results like "name.pdf.pdf".getValidFileName now always rebuilds the filename as cleanedBase + '.' + extension. If getBaseName hit InvalidPathException it returns the original fileName (which can still contain the extension), leading to duplicated extensions (<cleanedNameIncludingExt>.<ext>).
-
getFileExtensionwas updated to tolerateInvalidPathExceptionvia a fallback, so the invalid-path flow is explicitly supported. -
getBaseNamestill returns the rawfileNameonInvalidPathException, which is not a "base name". - The newly added test only asserts non-empty output for an invalid-path-like input, so it would not catch
.pdf.pdf.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[118-131]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[178-198]
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[409-413]
In getBaseName(String) catch block, compute a best-effort base name instead of returning the full input. For example:
- Use
FilenameUtils.getBaseName(FilenameUtils.getName(fileName.trim()))(matching the strategy used ingetFileExtension).
Update getValidFileNameDoesNotThrowForInvalidPathCharacters (or add a new test) to assert the output does not contain a duplicated extension (e.g., does not end with .pdf.pdf).
[reliability] Unhandled InvalidPathException
Unhandled InvalidPathException
FileUtil.getValidFileName now unconditionally calls getFileExtension(fileName), but getFileExtension uses Path.of(fileName.trim()) without handling InvalidPathException, so getValidFileName can now throw at runtime for inputs it is supposed to clean. This is a regression because getBaseName explicitly tolerates InvalidPathException and getValidFileName is used in file-export/rename flows where the input can originate from user/library data.FileUtil.getValidFileName(String) now always evaluates the extension via getFileExtension(fileName). getFileExtension(String) calls Path.of(fileName.trim()) without catching InvalidPathException, which means getValidFileName can throw for filenames containing characters invalid for Path parsing on the current OS (exactly the kind of inputs this method exists to sanitize).
-
getBaseName(String)already catchesInvalidPathExceptionand falls back to the raw string, implying invalid path strings are expected/possible. -
getValidFileName(String)is used in export and linked-file flows, so an exception here can break rename/export operations.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[76-97]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[112-125]
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[172-188]
- Make
getFileExtension(String)resilient by catchingInvalidPathExceptionand extracting the “name” portion withoutPath.of(e.g.,FilenameUtils.getName(trimmed)), then proceed withFilenameUtils.getExtension(...). - This keeps extension extraction consistent for all callers.
- Alternatively (less ideal), wrap the
getFileExtension(fileName)call insidegetValidFileNamewith try/catch and fall back to a string-based extension parse on failure. - Add a unit test that passes a name containing characters that would be invalid for
Path.ofon Windows (e.g.,"a:b.pdf") and assertgetValidFileNamereturns a sanitized value without throwing.
[maintainability] `Stream.generate` for repeat string
`Stream.generate` for repeat string
The new test builds long strings using `Stream.generate(...).limit(...).collect(Collectors.joining())` where the modern and clearer Java API `"1".repeat(n)` would be appropriate. This introduces an outdated pattern in newly added code and reduces readability.The new test constructs repeated-character strings using Stream.generate(...).collect(Collectors.joining()), which is less idiomatic than String#repeat(int) on modern Java.
The project encourages using modern Java (25+) idioms where applicable to improve readability/maintainability.
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[401-408]
[maintainability] Misaligned chain indentation
Misaligned chain indentation
The newly added test uses deeply indented chained-call formatting that deviates from the surrounding method-chaining style in the same test class. This introduces unnecessary formatting inconsistency in modified areas.The new test’s method-chain indentation is inconsistent with the existing formatting in FileUtilTest, creating a noisy/less consistent style.
This PR should preserve established formatting conventions and avoid unnecessary reformatting/stylistic divergence.
- jablib/src/test/java/org/jabref/logic/util/io/FileUtilTest.java[401-408]
[correctness] Negative truncation length
Negative truncation length
When truncating, the computed substring length can become negative if the extension length is very large, causing StringIndexOutOfBoundsException in getValidFileName. The new full-filename length check makes this branch reachable even when the base name is short but the extension is extremely long.getValidFileName computes MAXIMUM_FILE_NAME_LENGTH - (extensionLength + 1) and uses that directly as the substring end index. If the extension is extremely long, this value can be negative and substring(0, negative) throws.
This is an edge case (very long extensions are unusual), but the failure mode is a hard exception in a core utility method.
- jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java[172-188]
- Guard the computed max base length:
int maxBaseLen = MAXIMUM_FILE_NAME_LENGTH - extension.map(s -> s.length() + 1).orElse(0);- If
maxBaseLen <= 0, avoid callingsubstringwith a negative index. - Simple safe fallback: return
fullCleanedName.substring(0, MAXIMUM_FILE_NAME_LENGTH)whenmaxBaseLen <= 0. - (Alternative: truncate the extension to fit, but any safe non-throwing behavior is better than an exception.)
- Add a unit test with an extension length > 254 to assert no exception and the result length is <= 255.
| PR 15835 (2026-05-26) |
[maintainability] Duplicated `mutationScheduler` change code
Duplicated `mutationScheduler` change code
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.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.
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>.
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[53-56]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[59-63]
[performance] FX-thread file serialization
FX-thread file serialization
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.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.
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.
- jablib/src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java[39-66]
- jablib/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java[36-72]
- Keep file I/O and any string construction on the calling (background) thread.
- Precompute the new FILE field string (e.g., via
FileFieldWriter.getStringRepresentation(files)) and compare to the current value off-FX. - Only schedule the minimal mutation on the FX thread, e.g.
entry.setField(StandardField.FILE, newValue)(or equivalent), and capture the returnedFieldChangevia anAtomicReferenceif you need to return it. - Apply the same pattern in both
MoveFilesCleanupandRenamePdfCleanup.
| PR 15832 (2026-05-26) |
[reliability] Heylogs failure masked
Heylogs failure masked
The CHANGELOG lint step forces success with `|| true` and only fails based on counting `::error` lines in `heylogs.txt`, so if jbang/heylogs fails without emitting GitHub Actions `::error` annotations to stdout (e.g., stderr-only failure), CI will pass without validating CHANGELOG.md.The workflow masks the exit code of the heylogs invocation (|| true) and decides pass/fail only by grepping ::error lines in heylogs.txt. If heylogs/jbang fails without producing those annotations on stdout (common for runtime failures which go to stderr), error_count becomes 0 and the step passes, resulting in false-green CI.
You still want to ignore the specific heylogs rule all-h2-contain-a-version because of the Unreleased section, but you should not ignore unexpected command failures.
- .github/workflows/tests-code.yml[188-200]
- Capture the pipeline exit code (via
PIPESTATUS) and fail on unexpected non-zero exit codes, while still allowing the lint-error path to be handled by parsing::errorlines. - Also consider capturing stderr into
heylogs.txt(2>&1) so runtime failures are visible and can be surfaced. Example pattern:
| PR 15830 (2026-05-26) |
[correctness] Git options bypass guard
Git options bypass guard
`find_violation()` assumes the git subcommand is the token immediately after `git`, so any global git option placed there (e.g., `git -C … rebase`) will bypass the rebase/force-push blocking entirely. This undermines the PR’s core goal of reliably forbidding rebase/force-push for AI agents.The hook assumes tokens[i+1] is the git subcommand, which fails when git global options appear between git and the subcommand.
Git invocations can legally include global options before the subcommand; the hook should identify the actual subcommand before deciding whether to deny.
- .claude/hooks/git-rebase-guard.py[18-53]
- When you encounter a
gittoken, scan forward past recognized global options (and their arguments where applicable, e.g.-C <dir>,-c <name>=<value>,--git-dir=<path>,--work-tree=<path>) until the first non-option token, and treat that as the subcommand. - Then apply the existing
rebase/pull/pushchecks against that subcommand and the remaining args.
[correctness] Pull -r bypass allowed
Pull -r bypass allowed
The hook only blocks `git pull` when it sees `--rebase`/`--rebase=…`, so other rebase-enabling pull variants (e.g., `-r`) will not be denied. This allows rebase-based pulls to proceed despite the stated policy intent.The git pull check only detects --rebase and --rebase=... tokens, missing other rebase-enabling variants.
To reliably forbid rebase-based pulls, the hook should treat common short/variant flags as equivalent to --rebase.
- .claude/hooks/git-rebase-guard.py[35-41]
- Extend the predicate to also match
-r. - Consider matching any token that starts with
--rebase(e.g.,--rebase-merges) if the policy is “no rebase at all.”
[security] Force push refspec bypass
Force push refspec bypass
The hook blocks force-push only when `--force`/`--force-with-lease`/`-f` is present, but force-updates can also be expressed via `+`-prefixed refspecs that won’t be detected. This leaves a direct path to perform a forbidden force push without triggering the guard.The push guard only checks for force flags and ignores refspec arguments, so +-prefixed refspec force-updates are not blocked.
To enforce “no force push,” the hook must also inspect non-flag arguments to git push.
- .claude/hooks/git-rebase-guard.py[43-52]
- After identifying
git push, scan remaining args for refspec tokens beginning with+(e.g.,+HEAD:branch,+refs/heads/x:refs/heads/y) and deny if any are found. - Optionally also include other force-like flags (if relevant to your policy) alongside the existing set.
[maintainability] Docs/hook pull mismatch
Docs/hook pull mismatch
AGENTS.md says “Do not use bare `git pull`… Use the explicit `fetch` + `merge` pair,” but the hook does not deny plain `git pull`. This creates inconsistent guidance vs. enforcement for AI agents.Documentation forbids bare git pull, but the enforcement hook only blocks git pull when rebase is explicitly requested.
This mismatch can confuse agents (and humans) about what will be blocked vs. allowed.
- AGENTS.md[440-452]
- .claude/hooks/git-rebase-guard.py[35-41]
Choose one:
- Enforce the doc: deny any
git pullinvocation (regardless of flags) and instruct to usefetch+merge. - Or adjust AGENTS.md wording to clarify the hook only blocks explicit rebase pulls, if that’s the intended enforcement scope.
| PR 15827 (2026-05-25) |
[correctness] `withMainFileDirectory` sets wrong property
`withMainFileDirectory` sets wrong property
`FilePreferences.withMainFileDirectory` incorrectly assigns the provided path to `workingDirectory` (and even uses a misleading parameter name), so callers attempting to reset/import the main file directory do not actually update it and may instead overwrite the working directory setting. This breaks expected preference reset behavior (including the GUI reset path) and makes the API naming inconsistent and misleading.FilePreferences.withMainFileDirectory(...) currently updates workingDirectory instead of mainFileDirectory, and its parameter name (lastUsedDirectory) is misleading. As a result, callers (notably preference reset via JabRefCliPreferences.clear() used by the GUI reset path) silently fail to reset the main file directory and may unintentionally overwrite the working directory.
- The method is intended (by name and usage) to set the main file directory, but it assigns the provided
Pathto the wrong field. -
JabRefCliPreferences.clear()usesFilePreferences.getDefault().withMainFileDirectory(getDefaultPath())to reset the main file directory. - In GUI mode,
getDefaultPath()is overridden to the default file chooser directory, so this bug directly breaks the expected GUI preferences reset behavior. - The compliance checklist expectation of small, focused, naming-consistent code is violated because the method name/purpose and implementation disagree.
- jablib/src/main/java/org/jabref/logic/FilePreferences.java[384-387]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[991-996]
- jabgui/src/main/java/org/jabref/gui/preferences/JabRefGuiPreferences.java[1224-1228]
[reliability] Tests mock old return
Tests mock old return
FilePreferences.getMainFileDirectory() now returns Path, but some tests still mock it as Optional, which will fail compilation and block the build.The API change from Optional<Path> getMainFileDirectory() to Path getMainFileDirectory() was not propagated to all tests; remaining mocks returning Optional.of(...) no longer match the method signature.
Two test files still return Optional<Path> from getMainFileDirectory(), which should now return Path.
- jablib/src/main/java/org/jabref/logic/FilePreferences.java[142-152]
- jabgui/src/test/java/org/jabref/gui/desktop/os/BibDatbaseContextTest.java[27-36]
- jabgui/src/test/java/org/jabref/gui/fieldeditors/contextmenu/ContextMenuFactoryTest.java[58-65]
- Replace
thenReturn(Optional.of(path))withthenReturn(path). - Remove now-unused
Optionalimports/usage in those tests.
[correctness] Blank directory becomes CWD
Blank directory becomes CWD
LinkedFilesTabViewModel stores the main file directory via Path.of(text) and the validator does not reject blank input, so clearing the field can produce an empty Path that resolves relative to the current working directory and is treated as valid.The linked-files preferences UI converts the text field to a Path using Path.of(...) without handling blank input. An empty string yields an "empty" path; the validator only checks Files.exists/isDirectory and therefore can accept the current working directory when the field is cleared.
This behavior is new/impactful because storeSettings() now parses the path immediately (instead of storing a raw string), and the validator currently has no explicit blank-string guard.
- jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java[64-80]
- jabgui/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTabViewModel.java[116-120]
- In the validator: if the value is
null/blank, return a validation error (or normalize to the desired default). - In
storeSettings(): guard against blank input; either - normalize blank to the default main directory (e.g., preferences default), or
- prevent saving and rely on validation to block closing the dialog.
- Add a regression test covering blank input behavior.
| PR 15823 (2026-05-24) |
[maintainability] `AuthorListParser` formatting-only changes
`AuthorListParser` formatting-only changes
Several changes in `AuthorListParser` appear to be formatting-only (notably unusually wide indentation in `Set.of(...)` declarations and comment/Javadoc reflow) and unrelated to the functional fix, creating noisy diffs and reducing readability. This violates the requirement to avoid reformatting and keep changes minimal and consistent with the existing style.AuthorListParser.java includes formatting-only changes (especially unusually wide indentation in Set.of(...) declarations and comment/Javadoc reflow) that are not required for the behavioral fix, creating unnecessary diff noise and deviating from the existing formatting style.
The PR’s functional change is in token case detection, but multiple unrelated formatting edits were introduced, including re-indentation of the AVOID_TERMS_IN_LOWER_CASE and TEX_NAMES Set.of(...) constants and reflow of comments/Javadoc into long single lines. These should be reverted or adjusted back to the standard continuation indentation used across the codebase to reduce churn and improve readability.
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[23-26]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[36-39]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[111-116]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[200-201]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[420-426]
[maintainability] `CHANGELOG` mentions `namePrefix`
`CHANGELOG` mentions `namePrefix`
The new changelog bullet is written with internal implementation terms (e.g., `namePrefix`, `familyName`) and starts with lowercase “we,” which makes it less end-user focused and inconsistent with the existing `- We ...` capitalization/style used in surrounding entries. This violates the changelog requirement for user-oriented language and consistent formatting in release notes.The added CHANGELOG.md entry is not end-user oriented (it uses internal implementation terminology like namePrefix/familyName) and it breaks the existing bullet style by starting with lowercase “we” instead of matching the surrounding - We ... capitalization.
This is user-facing release-note text in the ### Fixed section; surrounding bullets consistently start with - We fixed ... and avoid internal code-level field names. The current entry uses lowercase we and references internal terms (namePrefix, familyName), making it inconsistent with the established changelog style and less readable for end users.
- CHANGELOG.md[31-38]
- CHANGELOG.md[37-37]
[maintainability] Misleading tokenCase logic
Misleading tokenCase logic
`getToken()` now sets `tokenCase` true for any letter that is not in Unicode category `LOWERCASE_LETTER`, which no longer matches the in-file documentation (“true if upper-case token”) and makes the condition redundant/unclear. This increases the risk of future incorrect changes because `tokenCase` drives von/last-name splitting decisions.tokenCase is documented as “true if upper-case token, false if lower-case”, but the new condition effectively treats any non-lowercase letter (including caseless scripts) as true. The current expression is also redundant: under the existing Character.isLetter(c) guard, Character.getType(c) != Character.LOWERCASE_LETTER already covers upper-case and Han, making isUpperCase/HAN checks unnecessary.
tokenCase is later used to decide whether a token starts the “von part” (!tokenCase) and where the last name begins (tokenCase), so readability and correct semantics matter.
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[54-57]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[467-475]
- jablib/src/main/java/org/jabref/logic/importer/AuthorListParser.java[230-276]
- Replace the assignment with something explicit and self-documenting, e.g.:
tokenCase = !Character.isLowerCase(c);- or keep
getTypebut simplify to:tokenCase = Character.getType(c) != Character.LOWERCASE_LETTER; - Update the field comment to match the new semantics, e.g. “true if token does not start with a lowercase letter (uppercase/caseless)”.
- (Optional) add a small comment explaining the caseless-script rationale so the intent is preserved.
| PR 15810 (2026-05-22) |
[maintainability] Blank line before `req~`
Blank line before `req~`
In `docs/requirements/slr.md`, each requirement heading is separated from its OpenFastTrace `req~...` identifier line by a blank line, violating the repository’s required requirements authoring format. This can impair automated requirement extraction/tracing and reduce traceability consistency.The OpenFastTrace/requirements authoring convention requires the requirement identifier line (e.g., req~...~1) to appear immediately under the Markdown heading with no blank line in between. In docs/requirements/slr.md, an empty line is placed between each ## ... heading and its req~... identifier, which can break or confuse automated requirement extraction and tracing.
PR Compliance ID 33 and the repo’s requirements guide explicitly state that IDs must be directly below headings with no empty line. Other requirement documents under docs/requirements/ follow this pattern, and CI runs gradle traceRequirements, so consistent formatting is required for tooling to parse requirements reliably.
- docs/requirements/slr.md[19-41]
[reliability] Markdown trailing whitespace
Markdown trailing whitespace
`docs/requirements/slr.md` contains a trailing space at the end of a list item, which violates markdownlint MD009 and can fail the CI Markdown job. This will block merging even though the change is only documentation.A trailing space at end-of-line violates markdownlint (MD009).
The CI workflow runs markdownlint over docs/**/*.md, and .markdownlint.yml keeps default rules enabled.
- docs/requirements/slr.md[13-13]
| PR 15790 (2026-05-20) |
[maintainability] Optional `parsed.get()` after check
Optional `parsed.get()` after check
`addPlainCitation` checks `parsed.isEmpty()` and then calls `parsed.get()`, which is the non-idiomatic and risk-prone Optional control-flow pattern disallowed by the checklist. This can regress safety/readability and is explicitly called out as a failure case.EntriesResource.addPlainCitation(...) uses Optional.isEmpty() followed by Optional.get(), which is discouraged and listed as a compliance failure case.
The code currently does:
if (parsed.isEmpty()) { throw ... }- later
parsed.get()
- jabsrv/src/main/java/org/jabref/http/server/resources/EntriesResource.java[114-126]
[maintainability] Optional `get()` in `findDoi`
Optional `get()` in `findDoi`
`findDoi` uses `parsedDoi.isEmpty()` followed by `parsedDoi.get()`, which is the Optional anti-pattern prohibited by the checklist. This reduces readability and can enable unsafe refactors later.RuleBasedPlainCitationParser.findDoi(...) uses Optional.isEmpty() then Optional.get(), which is a prohibited Optional control-flow pattern.
Current code:
- gets
Optional<DOI> parsedDoi = DOI.findInText(input) - checks
parsedDoi.isEmpty() - then reads the value via
parsedDoi.get()
- jablib/src/main/java/org/jabref/logic/importer/plaincitation/RuleBasedPlainCitationParser.java[114-121]
[reliability] `LibraryQueryRequest` nullability mismatch
`LibraryQueryRequest` nullability mismatch
`LibraryQueryRequest` is `@NullMarked` and declares `queries` as non-null, but the canonical constructor treats `queries` as possibly null (`queries != null ? ...`). This breaks the null-safety contract and should be expressed with explicit nullability annotations or input normalization outside the non-null API surface.Under @NullMarked, record components are non-null by default, but LibraryQueryRequest's constructor treats queries as nullable and normalizes it. This creates a nullability contract mismatch.
The constructor currently does queries = queries != null ? queries : List.of(); even though the record component type is List<String> (non-null under @NullMarked).
- jabsrv/src/main/java/org/jabref/http/dto/LibraryQueryRequest.java[17-20]
[correctness] Empty match entryId
Empty match entryId
LibrariesResource.runQuery emits entryId as entry.getCitationKey().orElse(""), so entries without a citation key become indistinguishable (""), breaking the endpoint’s stated identification-by-key contract.POST /libraries/query returns LibraryQueryMatch.entryId as an empty string when an entry has no citation key. This makes results ambiguous (collisions) and violates the DTO’s documented intent (identify by citation key).
-
LibraryQueryMatchis documented as identifying a match by "library and citation key". - Citation keys are optional in the model; therefore, returning
""is a real runtime state.
- jabsrv/src/main/java/org/jabref/http/server/resources/LibrariesResource.java[88-94]
- jabsrv/src/main/java/org/jabref/http/dto/LibraryQueryMatch.java[5-8]
Choose one:
- Change
LibraryQueryMatchto carry a stable internal identifier (e.g.,BibEntry.getId()) and optionally addcitationKeyas a separate field. - If the API must be citation-key based, filter out entries without citation keys (or generate keys before responding) and document that behavior.
| PR 15789 (2026-05-20) |
[maintainability] Changelog entry lacks PR link
Changelog entry lacks PR link
The new CHANGELOG bullet for the `github-actions` output format does not include an issue/PR reference link, unlike surrounding entries. This reduces traceability and consistency in user-facing documentation.The newly added changelog entry is missing the required issue/PR reference, reducing traceability and making the changelog inconsistent.
In CHANGELOG.md, entries should reference an issue (#NUM) or link the PR implementing the change.
- CHANGELOG.md[20-20]
| PR 15780 (2026-05-19) |
[reliability] Skip import on adjust error
Skip import on adjust error
If `adjustLinkedFilesForTargetIfRequired` fails, the `onFailure` path marks the entry as skipped and never calls `importCleanedEntries`, so the entry is not imported at all even though only linked-file adjustment failed. This can drop entries on common filesystem/path issues during transfer (e.g., invalid paths or unexpected runtime exceptions in file operations).ImportHandler#importEntryWithDuplicateCheck runs linked-file adjustment in a background task. If that task fails, it marks the entry as skipped and does not import it, causing silent entry loss when adjustment fails.
adjustLinkedFilesForTargetIfRequired calls LinkedFileTransferHelper.adjustLinkedFilesForTarget, which performs path and filesystem operations. While many IO failures are handled internally, uncaught runtime exceptions (e.g., invalid path strings) can still happen and currently abort the entry import.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[286-296]
In the .onFailure(...) of the linked-file adjustment task:
- still proceed with
importCleanedEntries(transferInformation, List.of(entryForImport))(or the best available unadjusted entry) - call
tracker.markImported(...)(or mark as imported-with-warning) - optionally show a user-visible warning/notification instead of only logging
[reliability] Dialog lock likely ineffective
Dialog lock likely ineffective
`getDuplicateDecision` uses `synchronized` to prevent multiple duplicate resolver dialogs, but all `BackgroundTask` success callbacks are invoked on the JavaFX thread, so the lock does not serialize same-thread dialog requests. Because the dialog is shown via `Dialog.showAndWait()`, which runs a nested event loop, other queued FX callbacks can still reach `getDuplicateDecision` while a dialog is open, allowing additional dialogs to open before the first decision is made.The PR adds synchronized (duplicateDecisionLock) around dialogService.showCustomDialogAndWait(dialog) to prevent multiple dialogs. However, duplicate handling runs on the JavaFX thread, and showCustomDialogAndWait calls Dialog.showAndWait() which runs a nested event loop; additional FX callbacks can still fire and re-enter getDuplicateDecision.
Multiple entries are imported concurrently (importEntriesWithDuplicateCheck starts tasks in a loop). Several entries can discover duplicates and attempt to resolve them while the first dialog is still open.
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[339-362]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[502-520]
Pick one approach:
- Batch-serialize duplicate resolution: process entries sequentially for the duplicate-resolution phase (chain background tasks) so only one dialog can be requested at a time.
- Queue dialog requests: maintain a single in-flight decision request and queue subsequent duplicate checks until the dialog completes, then resume with the chosen decision.
- If keeping a lock, replace it with an explicit non-reentrant “dialog in progress” gate + queue;
synchronizedalone does not prevent re-entry on the FX thread.
| PR 15779 (2026-05-19) |
[correctness] Crossref KEY inheritance broken
Crossref KEY inheritance broken
BibEntry#getSourceField now treats StandardField.KEY as a forbidden field, so BibEntry#getResolvedFieldOrAlias(StandardField.KEY, db) will no longer inherit "key" from the crossref parent. This contradicts the existing CrossrefTest expectations that StandardField.KEY is inherited via same-name inheritance, likely breaking crossref resolution behavior and tests.BibEntry#getSourceField was changed to return Optional.empty() for StandardField.KEY. This disables crossref inheritance for the BibTeX key field.
The repo’s crossref tests indicate StandardField.KEY is expected to be inherited (same-name inheritance). With the new forbidden check, resolving KEY via getResolvedFieldOrAlias(..., db) will return empty, causing behavior change and likely test failures.
-
InternalField.KEY_FIELDis the citation key;StandardField.KEYis a normal BibTeX field (used for sorting in some styles). Even if it’s “rare”, the codebase currently expects it to participate in crossref inheritance.
- jablib/src/main/java/org/jabref/model/entry/BibEntry.java[188-198]
Remove StandardField.KEY from the forbidden-fields list in getSourceField (or, if the intended behavior is to stop inheriting it, update the crossref behavior/tests accordingly and document the breaking change).
| PR 15775 (2026-05-19) |
[maintainability] PR template instructs emoji praise
PR template instructs emoji praise
The PR template now instructs adding a praise sentence ending with a `⭐` emoji, which is unprofessional and inconsistent for contributor-facing documentation. This can reduce the seriousness and clarity of PR descriptions.The PR template includes an instruction to add a sentence praising JabRef and end it with a ⭐ emoji. This is not professional guidance for contributor documentation and may encourage low-signal PR descriptions.
This change is in .github/PULL_REQUEST_TEMPLATE.md under the ### PR Description guidance.
- .github/PULL_REQUEST_TEMPLATE.md[32-33]
| PR 15773 (2026-05-19) |
[correctness] Wrong `JabRef_en.properties` path
Wrong `JabRef_en.properties` path
The new “Removing an obsolete key” section in `docs/code-howtos/localization.md` tells contributors to edit `src/main/resources/l10n/JabRef_en.properties`, but the repository’s actual English localization bundle is `jablib/src/main/resources/l10n/JabRef_en.properties`. Following the documentation as written can lead to editing/creating the wrong file, conflicting with the localization workflow and potentially leaving `LocalizationConsistencyTest` failing.The “Removing an obsolete key” instructions in docs/code-howtos/localization.md currently direct contributors to edit src/main/resources/l10n/JabRef_en.properties, but the correct English localization file in this repository is jablib/src/main/resources/l10n/JabRef_en.properties. Update the documentation so contributors modify the correct file path and don’t accidentally edit/create a non-existent file or leave localization tests failing.
- The localization workflow (PR Compliance ID 19) requires key additions/removals to be applied to
jablib/src/main/resources/l10n/JabRef_en.properties. - The document already references the correct
jablib/.../JabRef_en.propertiespath earlier, but the newly added “Removing an obsolete key” step usessrc/main/resources/..., creating an internal inconsistency.
- docs/code-howtos/localization.md[57-69]
[maintainability] Conflicting test class name
Conflicting test class name
`localization.md` now tells readers to run `org.jabref.logic.l10n.LocalizationConsistencyTest` in the new removal flow, but the existing add-key flow still references `org.jabref.logic.LocalizationConsistencyTest` (wrong package). This inconsistency can send contributors to a non-existent test class and block the workflow the doc is trying to describe.docs/code-howtos/localization.md contains conflicting instructions for which test to run:
- “Adding a new key” references
org.jabref.logic.LocalizationConsistencyTest(incorrect). - The new “Removing an obsolete key” section references
org.jabref.logic.l10n.LocalizationConsistencyTest(correct). This inconsistency is confusing and can cause contributors to try to run a test class that doesn’t exist.
The actual test class is in package org.jabref.logic.l10n.
- docs/code-howtos/localization.md[55-67]
- jablib/src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java[1-5]
| PR 15762 (2026-05-18) |
[reliability] Null selected tab deref
Null selected tab deref
LibraryTab#setDatabaseContext calls selectedItemProperty().get().equals(this), which can throw NullPointerException when the TabPane has no selected item. This can abort library loading and leave the StateManager in an inconsistent state.LibraryTab#setDatabaseContext uses tabPane.getSelectionModel().selectedItemProperty().get().equals(this) which will throw if the selected item is null.
Other code already accounts for TabPane having no selected tab (getSelectedItem() == null), so null is a real state and should be handled here as well.
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[372-382]
Replace the dereference with a null-safe comparison, e.g.:
- Home
- General Information
- Development
- Please go to our devdocs at https://devdocs.jabref.org
- Completed "Google Summer of Code" (GSoC) projects
- GSoC 2025 ‐ Git Support for JabRef
- GSoC 2025 - LSP
- GSoC 2025 - Walkthrough and Welcome Tab
- GSoC 2024 ‐ Improved CSL Support (and more LibreOffice‐JabRef integration enhancements)
- GSoC 2024 - Lucene Search Backend Integration
- GSoC 2024 ‐ AI‐Powered Summarization and “Interaction” with Academic Papers
- GSoC 2022 — Implement a Three Way Merge UI for merging BibTeX entries
- GSoC 2021 - Improve pdf support in JabRef
- GSoC 2021 - Microsoft Word Integration
- GSoc 2019 - Bidirectional Integration — Paper Writing — LaTeX and JabRef 5.0
- GSoC Archive
- Release
- JabCon Archive