Skip to content

Support modifier keys entry drag drop#16286

Open
Eng-YasminKotb wants to merge 7 commits into
JabRef:mainfrom
Eng-YasminKotb:support-modifier-keys-entry-drag-drop
Open

Support modifier keys entry drag drop#16286
Eng-YasminKotb wants to merge 7 commits into
JabRef:mainfrom
Eng-YasminKotb:support-modifier-keys-entry-drag-drop

Conversation

@Eng-YasminKotb

@Eng-YasminKotb Eng-YasminKotb commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Related issues and pull requests

Closes #16109

PR Description

This PR fixes the move operation between libraries by removing the original entries from the source library after all entries have been successfully imported into the target library.

pr.des.mp4

Steps to test

  1. Open two libraries in separate tabs.
  2. Add one or more entries to the source library.
  3. Drag the selected entries from the source library to the target library while holding the Shift modifier key.
  4. Verify that the entries are imported into the target library.
  5. Verify that the original entries are removed from the source library after the move completes.

AI usage

ChatGPT (GPT-5.5)


Checklist

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

@github-actions github-actions Bot added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Jul 18, 2026
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Support modifier keys for entry drag-and-drop between libraries

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Honor COPY vs MOVE modifiers when dragging entries between library tabs.
• On MOVE, remove source entries only after target import fully succeeds.
• Add localized notifications for copy operations.
Diagram

graph TD
  U(["User drag + modifier"]) --> F["FrameDndHandler"] -->|"Transfer mode"| L["LibraryTab.dropEntry"] --> T["TransferInformation"] --> I["InsertUtil"] --> H["ImportHandler"]
  H -->|"Insert entries"| TDB[("Target BibDatabase")]
  H -->|"MOVE & all imported"| SDB[("Source BibDatabase")]

  subgraph Legend
    direction LR
    _u(["User"]) ~~~ _c["Component"] ~~~ _db[("Database")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Delete-on-finish in InsertUtil tracker callback
  • ➕ Centralizes the 'when is the transfer complete' decision in one place (tracker.onFinish).
  • ➕ Avoids per-entry completion checks and reduces risk of multiple/early deletions.
  • ➕ Can handle both success and fallback import paths consistently (e.g., linked-file adjustment failures).
  • ➖ Requires passing richer per-entry results (imported entry mapping) or a callback from ImportHandler.
  • ➖ More refactoring if ImportHandler currently owns all MOVE semantics.
2. Batch import API that returns ImportResult (imported/skipped)
  • ➕ Makes MOVE semantics explicit: delete exactly the entries corresponding to imported results.
  • ➕ Improves testability by returning a deterministic summary instead of relying on side effects.
  • ➖ Larger API change; may ripple through other import call sites.
  • ➖ May require more restructuring of async duplicate resolution flow.

Recommendation: The PR’s approach (propagating transfer mode + source entries via TransferInformation and letting ImportHandler remove originals) is directionally solid because it keeps MOVE semantics close to the import pipeline. During review, pay special attention to completion conditions: the current removal check is performed on the linked-file-adjustment success path; consider ensuring the MOVE deletion condition is evaluated after every successful import mark (including fallback paths) and only once (e.g., in a shared post-import hook or tracker onFinish).

Files changed (5) +132 / -339

Enhancement (3) +77 / -212
LibraryTab.javaPropagate copy/move intent and show correct drop feedback messages +38/-174

Propagate copy/move intent and show correct drop feedback messages

• Updates entry drop handling to accept original entries, entry copies, and a model TransferMode. Chooses localized 'Copied' vs 'Moved' messages and passes enriched TransferInformation into the import flow; remaining changes are mostly formatting/line-wrapping refactors.

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

FrameDndHandler.javaSupport COPY_OR_MOVE tab drops and map JavaFX TransferMode to model +32/-34

Support COPY_OR_MOVE tab drops and map JavaFX TransferMode to model

• Accepts COPY_OR_MOVE when dragging entries over tabs and maps JavaFX TransferMode to org.jabref.model.TransferMode. Passes both original entries and entry copies into LibraryTab.dropEntry so MOVE can later delete originals.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java

TransferInformation.javaCarry source entries for MOVE operations +7/-4

Carry source entries for MOVE operations

• Extends TransferInformation to include the list of source BibEntry objects and adds a convenience constructor preserving the old (context, mode) usage.

jablib/src/main/java/org/jabref/model/TransferInformation.java

Bug fix (1) +53 / -127
ImportHandler.javaComplete MOVE by deleting source entries after all imports succeed +53/-127

Complete MOVE by deleting source entries after all imports succeed

• Extends the import pipeline to remove the original source entries from the source database after the tracker indicates all entries were imported for MOVE transfers. Also adds a BibDatabase dependency and performs broad formatting/streamlining of existing code blocks.

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

Other (1) +2 / -0
JabRef_en.propertiesAdd localization keys for copy transfer notifications +2/-0

Add localization keys for copy transfer notifications

• Adds English strings for 'Copied %0 entry(s) to %1' and its partial/skip variant to match the new copy feedback path.

jablib/src/main/resources/l10n/JabRef_en.properties

@Eng-YasminKotb
Eng-YasminKotb marked this pull request as draft July 18, 2026 19:15
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📜 Skill insights (0)

Grey Divider


Action required

1. Duplicate result ignored 🐞 Bug ≡ Correctness ⭐ New
Description
In ImportHandler.importEntryWithDuplicateCheck, when a duplicate is found and resolved, the code
ignores the Optional<BibEntry> returned from handleDuplicates and imports the original entry
instead. This bypasses merge/keep-right outcomes and can insert the wrong (uncleaned/unmerged) entry
into the target library.
Code

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[R313-319]

+                handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision).thenAccept(duplicateHandledEntry -> {
+                    if (duplicateHandledEntry.isEmpty()) {
+                        tracker.markSkipped();
+                        return;
+                    }
+                    continueImportAfterDuplicateHandling(transferInformation, entry, tracker);
+                }).exceptionally(exception -> {
Evidence
The duplicate branch explicitly computes a cleaned entry (entryToInsert) and obtains a possibly
modified entry from handleDuplicates, but then continues importing entry, discarding the
duplicate-handled result. handleDecisionResult can return a merged entry, so ignoring the returned
Optional breaks correctness.

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[292-327]
jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[365-386]

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

## Issue description
When a duplicate is detected and `handleDuplicates(...)` returns a non-empty `Optional<BibEntry>`, the import flow must continue with that returned entry. The current code continues with the original `entry`, discarding duplicate-resolution results (e.g., merged entry) and the cleaned-up entry variant.

## Issue Context
- `handleDuplicates(...)` may return a merged entry or other decision-dependent result.
- The current callback checks `duplicateHandledEntry.isEmpty()` but then imports `entry` anyway.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[303-327]

## Suggested change
Replace:
- `continueImportAfterDuplicateHandling(transferInformation, entry, tracker);`
With:
- `continueImportAfterDuplicateHandling(transferInformation, duplicateHandledEntry.get(), tracker);`

Also ensure the imported entry is the cleaned/decision-result entry (not the original input).

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


2. MOVE cleanup order-dependent 🐞 Bug ☼ Reliability ⭐ New
Description
Source entry removal for MOVE is executed only inside the linked-file-adjustment onSuccess callback,
while the onFailure callback still imports the entry and increments the imported count without ever
attempting cleanup. Because per-entry imports run concurrently, MOVE cleanup becomes order-dependent
and can be skipped entirely if the last completed entry goes through onFailure.
Code

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[R331-343]

+        BackgroundTask.wrap(() -> adjustLinkedFilesForTargetIfRequired(transferInformation, entryForImport)).onFailure(e -> {
+            LOGGER.error("Error adjusting linked files for target", e);
+            dialogService.notify(Localization.lang("Could not adjust linked files. The entry was imported without linked-file adjustments."));
+            importCleanedEntries(transferInformation, List.of(entryForImport));
+            tracker.markImported(entryForImport);
+        }).onSuccess(adjustedEntry -> {
+            importCleanedEntries(transferInformation, List.of(adjustedEntry));
+            tracker.markImported(adjustedEntry);
+            if (transferInformation != null && transferInformation.transferMode() == org.jabref.model.TransferMode.MOVE && tracker.getImportedCount() == transferInformation.sourceEntries().size()) {
+                BibDatabase sourceDatabase = transferInformation.bibDatabaseContext().getDatabase();
+                List<BibEntry> sourceEntries = transferInformation.sourceEntries();
+                sourceDatabase.removeEntries(sourceEntries);
+            }
Evidence
continueImportAfterDuplicateHandling increments imported count on both success and failure, but only
the success path contains the MOVE cleanup condition. Since the tracker explicitly documents
concurrent imports, the final callback may be a failure and therefore never triggers source removal.
Linked-file transfer performs filesystem move/copy operations, providing a realistic failure mode
for the onFailure branch.

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[330-345]
jabgui/src/main/java/org/jabref/gui/externalfiles/EntryImportHandlerTracker.java[19-75]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]

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

## Issue description
MOVE source-entry cleanup (`sourceDatabase.removeEntries(sourceEntries)`) is performed only in the `onSuccess` branch of linked-file adjustment. If the last processed entry completes via `onFailure`, cleanup never runs even though the failure path still imports the entry and marks it as imported.

## Issue Context
- Imports run concurrently (one task chain per entry).
- `onFailure` calls `importCleanedEntries(...)` and `tracker.markImported(...)` but does not evaluate MOVE cleanup.
- Linked-file adjustment can involve filesystem operations and may fail.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[330-345]
- jabgui/src/main/java/org/jabref/gui/externalfiles/EntryImportHandlerTracker.java[19-75]

## Suggested fix
Implement MOVE cleanup in a completion-safe place that runs exactly once after all entries are processed, regardless of per-entry success/failure ordering. Two viable approaches:
1) Call a shared `maybeFinalizeMove(transferInformation, tracker)` after `tracker.markImported(...)` in BOTH `onSuccess` and `onFailure`, and guard it so it runs once.
2) Move the MOVE finalization to the tracker’s `onFinish` hook (single-shot), checking `transferMode == MOVE` and `importedCount == sourceEntries.size()` before removing source entries.

Ensure the cleanup cannot be skipped due to callback ordering.

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


3. toModelTransferMode lacks null handling ✓ Resolved 📘 Rule violation ☼ Reliability
Description
FrameDndHandler.toModelTransferMode switches on a potentially null javafxTransferMode without
guarding against null, which can throw a NullPointerException during drag-and-drop and break
drop handling. Since JavaFX DragEvent.getTransferMode() may be null, this violates the project’s
nullability expectations and is inconsistent with other DnD code paths.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R129-141]

+    private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {
+        switch (javafxTransferMode) {
+            case COPY -> {
+                return org.jabref.model.TransferMode.COPY;
+            }
+            case MOVE -> {
+                return org.jabref.model.TransferMode.MOVE;
+            }
+            default -> {
+                return org.jabref.model.TransferMode.NONE;
+            }
+        }
+    }
Evidence
PR Compliance ID 8 requires explicit nullability handling and avoidance of unsafe null usage, yet
the new toModelTransferMode implementation performs switch (javafxTransferMode) while being
called with tabDragEvent.getTransferMode(), which can be null. This risk is corroborated by
existing drag-and-drop flows (e.g., in CitationRelationsTab) that explicitly guard
event.getTransferMode() against null, indicating that a null transfer mode is a valid runtime
scenario; therefore, the current switch can dereference null and crash the drop handler.

AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull)
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[115-117]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-141]
jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-650]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-141]
jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-651]

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

## Issue description
`toModelTransferMode(TransferMode javafxTransferMode)` currently performs `switch (javafxTransferMode)` without handling `null`. Because JavaFX `DragEvent.getTransferMode()` may be `null`, this can throw an NPE and abort the drop handler, breaking drag-and-drop and violating the project’s nullability expectations.
## Issue Context
The codebase already treats `event.getTransferMode()` as nullable in other drag-and-drop handlers by explicitly checking it against `null`, indicating that `null` is an expected value in some DnD scenarios. Update `toModelTransferMode` to be null-safe (e.g., return a safe default such as `COPY`, or return `NONE` only if it is handled safely downstream), and optionally add debug/trace logging when the mode is null/unexpected to aid diagnosis.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-141]
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-651]

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


View more (1)
4. MOVE enabled without source update ✓ Resolved 🐞 Bug ≡ Correctness
Description
Tabs now accept COPY_OR_MOVE and the chosen mode is forwarded into the import flow, but the drag
source does not remove entries on MOVE, so a MOVE gesture can leave source entries intact while
linked files may be physically moved. This can break file links in the source library (files moved
away) and creates inconsistent semantics for “move”.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R181-184]

  if (tabDragEvent.getDragboard().hasContent(DragAndDropDataFormats.ENTRIES)) {
-            tabDragEvent.acceptTransferModes(TransferMode.COPY);
+            tabDragEvent.acceptTransferModes(TransferMode.COPY_OR_MOVE);
      tabDragEvent.consume();
  }
Evidence
The PR enables MOVE on tab drag-over and forwards the selected transfer mode to dropEntry.
MainTable initiates drags with COPY_OR_MOVE but the row factory does not register a drag-done
handler that would remove entries on MOVE; meanwhile linked-file transfer uses MOVE to call
Files.move, which can move files away from the source context.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-118]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[170-185]
jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[152-178]
jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[529-552]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]

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

## Issue description
The drop target now allows MOVE and forwards it, but the drag source never performs MOVE completion (removing entries / coordinating file moves). This can move linked files while leaving source entries untouched.
### Issue Context
- Tab drop now accepts `COPY_OR_MOVE` for entries.
- Drop path forwards the selected transfer mode into `TransferInformation`, which drives linked-file copy/move behavior.
- Drag source (MainTable) starts `COPY_OR_MOVE` but does not wire an `onDragDone` handler to remove entries when MOVE completes.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-118]
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[170-185]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[152-178]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[529-552]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]
### Suggested fix (choose one)
1) **Safe short-term**: keep tab drops at COPY only (revert to `acceptTransferModes(TransferMode.COPY)`) until true MOVE semantics (source removal + undo) are implemented.
2) **Full implementation**: add a source-side `onDragDone` handler for MainTable drags that, on successful MOVE, removes the dragged entries from the source database (with undo support) and ensures linked-file movement is only performed when entries are actually moved.
3) **Interim mitigation**: still allow the cursor to show MOVE, but force model transfer mode to COPY for cross-library entry transfer so linked files are not moved away from the source library.

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



Remediation recommended

5. Unrelated reformatting in LibraryTab 📘 Rule violation ⚙ Maintainability ⭐ New
Description
This PR includes multiple whitespace/line-wrapping-only edits (e.g., collapsing multi-line
declarations/constructors into single long lines) that add diff noise and reduce readability without
being required for the behavior change. This violates the requirement to preserve existing
formatting and avoid unrelated reformatting.
Code

jabgui/src/main/java/org/jabref/gui/LibraryTab.java[155]

+    @SuppressWarnings({"FieldCanBeLocal"}) private Subscription dividerPositionSubscription;
Evidence
PR Compliance ID 3 requires keeping formatting changes limited to areas necessary for the
modification. The cited lines show formatting-only edits (collapsed annotations/constructors/records
into single lines) that do not reflect functional changes.

AGENTS.md: Preserve Existing Formatting and Avoid Unrelated Reformatting
jabgui/src/main/java/org/jabref/gui/LibraryTab.java[155-155]
jabgui/src/main/java/org/jabref/gui/LibraryTab.java[179-180]
jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[100-120]

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

## Issue description
The PR includes formatting-only changes (line wrapping/whitespace) unrelated to the functional drag-and-drop/move behavior change.

## Issue Context
Compliance requires minimizing diff noise and preserving existing formatting except where needed for the change.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[155-180]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[101-120]

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


6. Localized message built by concatenation 📘 Rule violation ⚙ Maintainability ⭐ New
Description
A user-facing message is constructed via string concatenation (`Localization.lang(...) + " " +
...`), which prevents proper localization and translator reordering. This violates the
placeholder-based localization requirement.
Code

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[R252-258]

    AutoDetectionImportOutcome createAutoDetectionImportOutcome(Path file, ImportResult importResult) {
        List<BibEntry> importedEntries = importResult.parserResult().getDatabase().getEntries();
        if (importResult.parserResult().hasWarnings()) {
-            String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1",
-                    importResult.format(),
-                    importResult.parserResult().getErrorMessage());
+            String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1", importResult.format(), importResult.parserResult().getErrorMessage());
            if (importedEntries.isEmpty()) {
-                return new AutoDetectionImportOutcome(
-                        List.of(createEmptyEntryWithLink(file)),
-                        false,
-                        Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format())
-                                + " "
-                                + importResult.parserResult().getErrorMessage());
+                return new AutoDetectionImportOutcome(List.of(createEmptyEntryWithLink(file)), false, Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format()) + " " + importResult.parserResult().getErrorMessage());
            }
Evidence
PR Compliance ID 26 requires user-facing messages to be fully localizable using placeholders (no
concatenation). The cited code concatenates a localized prefix with a dynamic error message instead
of using one localized string with placeholders.

jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[252-258]
Best Practice: Learned patterns

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

## Issue description
A user-facing localized message is built using string concatenation instead of a single translation string with placeholders.

## Issue Context
Localization rules require the full message format to be in the translation resource, with dynamic parts provided via placeholders.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[252-258]
- jablib/src/main/resources/l10n/JabRef_en.properties[3037-3045]

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


7. Drop toast ignores mode ✓ Resolved 🐞 Bug ≡ Correctness
Description
LibraryTab.dropEntry always notifies "Moved %0 entry(s)" even when the transfer mode is COPY,
producing incorrect user feedback. With this PR propagating the mode, this becomes user-visible and
misleading.
Code

jabgui/src/main/java/org/jabref/gui/LibraryTab.java[R946-951]

+    public void dropEntry(BibDatabaseContext sourceBibDatabaseContext, List<BibEntry> entriesToAdd, TransferMode mode) {
  addEntriesWithFeedback(
-                new TransferInformation(sourceBibDatabaseContext, TransferMode.NONE), // "NONE", because we don't know the modifiers here and thus cannot say whether the attached file (and entry(s)) should be copied or moved
+                new TransferInformation(sourceBibDatabaseContext, mode),
          entriesToAdd,
          bibDatabaseContext,
          Localization.lang("Moved %0 entry(s) to %1"),
Evidence
The new mode parameter is forwarded into TransferInformation, but the user notification strings
remain "Moved …" regardless of the passed mode.

jabgui/src/main/java/org/jabref/gui/LibraryTab.java[946-956]

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

## Issue description
`LibraryTab.dropEntry` always uses "Moved …" messages even when the operation is COPY.
### Issue Context
The method now receives `TransferMode mode` and passes it into `TransferInformation`, but still emits hard-coded "Moved" notifications.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[946-956]
### Suggested fix
- Choose success/partial message based on `mode`:
- COPY -> "Copied %0 entry(s) to %1" (and partial equivalent)
- MOVE -> "Moved …"
- NONE/unknown -> use neutral wording like "Transferred …" or default to COPY wording.
- Ensure both messages remain localized via `Localization.lang(...)`.

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


View more (1)
8. NONE triggers file move ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new mapping returns model TransferMode.NONE for any unexpected JavaFX mode, but the linked-file
transfer helper treats any non-COPY mode as MOVE and will call Files.move in that case. If NONE ever
reaches this path (e.g., due to an unexpected TransferMode), linked files can be moved instead of
copied.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R129-140]

+    private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {
+        switch (javafxTransferMode) {
+            case COPY -> {
+                return org.jabref.model.TransferMode.COPY;
+            }
+            case MOVE -> {
+                return org.jabref.model.TransferMode.MOVE;
+            }
+            default -> {
+                return org.jabref.model.TransferMode.NONE;
+            }
+        }
Evidence
FrameDndHandler’s default mapping can produce model NONE; LinkedFileTransferHelper’s file operation
uses an else branch that moves whenever the mode is not COPY, so NONE behaves like MOVE there.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-140]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]

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

## Issue description
`toModelTransferMode` defaults to `org.jabref.model.TransferMode.NONE`, but `LinkedFileTransferHelper` currently moves files for any transfer mode other than COPY.
### Issue Context
Even if current entry-tab drops usually negotiate COPY/MOVE, the default branch is a footgun and `TransferMode.NONE` exists in the model and is used in other flows.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-140]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]
### Suggested fix
- Update `LinkedFileTransferHelper` to explicitly branch on the model enum:
- `COPY` -> copy
- `MOVE` -> move
- `NONE` -> do not copy/move (and continue processing remaining files without returning early).
- Optionally also change `toModelTransferMode` default to COPY (safer) or to NONE only if NONE is guaranteed to be non-destructive downstream.

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



Informational

9. Noisy info-level DnD log ✓ Resolved 🐞 Bug ◔ Observability
Description
FrameDndHandler logs the transfer mode at INFO on every tab drop, which can spam logs during normal
usage and reduce signal-to-noise. This is better suited for DEBUG/TRACE or removal after
development.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[78]

+        LOGGER.info("TransferMode = {}", tabDragEvent.getTransferMode());
Evidence
The added INFO log is executed at the start of every onTabDragDropped call.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[77-80]

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

## Issue description
An INFO log is emitted on every tab drop, which is typically too verbose for production logs.
### Issue Context
This is a high-frequency UI event.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[77-80]
### Suggested fix
- Change to `LOGGER.debug(...)` or `LOGGER.trace(...)`, or remove once behavior is verified.

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


Grey Divider

Previous review results

Review updated until commit be69001 ⚖️ Balanced

Results up to commit N/A


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. toModelTransferMode lacks null handling ✓ Resolved 📘 Rule violation ☼ Reliability
Description
FrameDndHandler.toModelTransferMode switches on a potentially null javafxTransferMode without
guarding against null, which can throw a NullPointerException during drag-and-drop and break
drop handling. Since JavaFX DragEvent.getTransferMode() may be null, this violates the project’s
nullability expectations and is inconsistent with other DnD code paths.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R129-141]

+    private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {
+        switch (javafxTransferMode) {
+            case COPY -> {
+                return org.jabref.model.TransferMode.COPY;
+            }
+            case MOVE -> {
+                return org.jabref.model.TransferMode.MOVE;
+            }
+            default -> {
+                return org.jabref.model.TransferMode.NONE;
+            }
+        }
+    }
Evidence
PR Compliance ID 8 requires explicit nullability handling and avoidance of unsafe null usage, yet
the new toModelTransferMode implementation performs switch (javafxTransferMode) while being
called with tabDragEvent.getTransferMode(), which can be null. This risk is corroborated by
existing drag-and-drop flows (e.g., in CitationRelationsTab) that explicitly guard
event.getTransferMode() against null, indicating that a null transfer mode is a valid runtime
scenario; therefore, the current switch can dereference null and crash the drop handler.

AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull): AGENTS.md: Prefer Immutability and Enforce Explicit Nullability with JSpecify (Avoid null and requireNonNull)
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[115-117]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-141]
jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-650]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-141]
jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-651]

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

## Issue description
`toModelTransferMode(TransferMode javafxTransferMode)` currently performs `switch (javafxTransferMode)` without handling `null`. Because JavaFX `DragEvent.getTransferMode()` may be `null`, this can throw an NPE and abort the drop handler, breaking drag-and-drop and violating the project’s nullability expectations.
## Issue Context
The codebase already treats `event.getTransferMode()` as nullable in other drag-and-drop handlers by explicitly checking it against `null`, indicating that `null` is an expected value in some DnD scenarios. Update `toModelTransferMode` to be null-safe (e.g., return a safe default such as `COPY`, or return `NONE` only if it is handled safely downstream), and optionally add debug/trace logging when the mode is null/unexpected to aid diagnosis.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-141]
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[646-651]

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


2. MOVE enabled without source update ✓ Resolved 🐞 Bug ≡ Correctness
Description
Tabs now accept COPY_OR_MOVE and the chosen mode is forwarded into the import flow, but the drag
source does not remove entries on MOVE, so a MOVE gesture can leave source entries intact while
linked files may be physically moved. This can break file links in the source library (files moved
away) and creates inconsistent semantics for “move”.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R181-184]

   if (tabDragEvent.getDragboard().hasContent(DragAndDropDataFormats.ENTRIES)) {
-            tabDragEvent.acceptTransferModes(TransferMode.COPY);
+            tabDragEvent.acceptTransferModes(TransferMode.COPY_OR_MOVE);
       tabDragEvent.consume();
   }
Evidence
The PR enables MOVE on tab drag-over and forwards the selected transfer mode to dropEntry.
MainTable initiates drags with COPY_OR_MOVE but the row factory does not register a drag-done
handler that would remove entries on MOVE; meanwhile linked-file transfer uses MOVE to call
Files.move, which can move files away from the source context.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-118]
jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[170-185]
jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[152-178]
jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[529-552]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]

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

## Issue description
The drop target now allows MOVE and forwards it, but the drag source never performs MOVE completion (removing entries / coordinating file moves). This can move linked files while leaving source entries untouched.
### Issue Context
- Tab drop now accepts `COPY_OR_MOVE` for entries.
- Drop path forwards the selected transfer mode into `TransferInformation`, which drives linked-file copy/move behavior.
- Drag source (MainTable) starts `COPY_OR_MOVE` but does not wire an `onDragDone` handler to remove entries when MOVE completes.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[110-118]
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[170-185]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[152-178]
- jabgui/src/main/java/org/jabref/gui/maintable/MainTable.java[529-552]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]
### Suggested fix (choose one)
1) **Safe short-term**: keep tab drops at COPY only (revert to `acceptTransferModes(TransferMode.COPY)`) until true MOVE semantics (source removal + undo) are implemented.
2) **Full implementation**: add a source-side `onDragDone` handler for MainTable drags that, on successful MOVE, removes the dragged entries from the source database (with undo support) and ensures linked-file movement is only performed when entries are actually moved.
3) **Interim mitigation**: still allow the cursor to show MOVE, but force model transfer mode to COPY for cross-library entry transfer so linked files are not moved away from the source library.

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



Remediation recommended
3. Drop toast ignores mode ✓ Resolved 🐞 Bug ≡ Correctness
Description
LibraryTab.dropEntry always notifies "Moved %0 entry(s)" even when the transfer mode is COPY,
producing incorrect user feedback. With this PR propagating the mode, this becomes user-visible and
misleading.
Code

jabgui/src/main/java/org/jabref/gui/LibraryTab.java[R946-951]

+    public void dropEntry(BibDatabaseContext sourceBibDatabaseContext, List<BibEntry> entriesToAdd, TransferMode mode) {
   addEntriesWithFeedback(
-                new TransferInformation(sourceBibDatabaseContext, TransferMode.NONE), // "NONE", because we don't know the modifiers here and thus cannot say whether the attached file (and entry(s)) should be copied or moved
+                new TransferInformation(sourceBibDatabaseContext, mode),
           entriesToAdd,
           bibDatabaseContext,
           Localization.lang("Moved %0 entry(s) to %1"),
Evidence
The new mode parameter is forwarded into TransferInformation, but the user notification strings
remain "Moved …" regardless of the passed mode.

jabgui/src/main/java/org/jabref/gui/LibraryTab.java[946-956]

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

## Issue description
`LibraryTab.dropEntry` always uses "Moved …" messages even when the operation is COPY.
### Issue Context
The method now receives `TransferMode mode` and passes it into `TransferInformation`, but still emits hard-coded "Moved" notifications.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[946-956]
### Suggested fix
- Choose success/partial message based on `mode`:
- COPY -> "Copied %0 entry(s) to %1" (and partial equivalent)
- MOVE -> "Moved …"
- NONE/unknown -> use neutral wording like "Transferred …" or default to COPY wording.
- Ensure both messages remain localized via `Localization.lang(...)`.

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


4. NONE triggers file move ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new mapping returns model TransferMode.NONE for any unexpected JavaFX mode, but the linked-file
transfer helper treats any non-COPY mode as MOVE and will call Files.move in that case. If NONE ever
reaches this path (e.g., due to an unexpected TransferMode), linked files can be moved instead of
copied.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[R129-140]

+    private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {
+        switch (javafxTransferMode) {
+            case COPY -> {
+                return org.jabref.model.TransferMode.COPY;
+            }
+            case MOVE -> {
+                return org.jabref.model.TransferMode.MOVE;
+            }
+            default -> {
+                return org.jabref.model.TransferMode.NONE;
+            }
+        }
Evidence
FrameDndHandler’s default mapping can produce model NONE; LinkedFileTransferHelper’s file operation
uses an else branch that moves whenever the mode is not COPY, so NONE behaves like MOVE there.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-140]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]

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

## Issue description
`toModelTransferMode` defaults to `org.jabref.model.TransferMode.NONE`, but `LinkedFileTransferHelper` currently moves files for any transfer mode other than COPY.
### Issue Context
Even if current entry-tab drops usually negotiate COPY/MOVE, the default branch is a footgun and `TransferMode.NONE` exists in the model and is used in other flows.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[129-140]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[121-149]
### Suggested fix
- Update `LinkedFileTransferHelper` to explicitly branch on the model enum:
- `COPY` -> copy
- `MOVE` -> move
- `NONE` -> do not copy/move (and continue processing remaining files without returning early).
- Optionally also change `toModelTransferMode` default to COPY (safer) or to NONE only if NONE is guaranteed to be non-destructive downstream.

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



Informational
5. Noisy info-level DnD log ✓ Resolved 🐞 Bug ◔ Observability
Description
FrameDndHandler logs the transfer mode at INFO on every tab drop, which can spam logs during normal
usage and reduce signal-to-noise. This is better suited for DEBUG/TRACE or removal after
development.
Code

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[78]

+        LOGGER.info("TransferMode = {}", tabDragEvent.getTransferMode());
Evidence
The added INFO log is executed at the start of every onTabDragDropped call.

jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[77-80]

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

## Issue description
An INFO log is emitted on every tab drop, which is typically too verbose for production logs.
### Issue Context
This is a high-frequency UI event.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java[77-80]
### Suggested fix
- Change to `LOGGER.debug(...)` or `LOGGER.trace(...)`, or remove once behavior is verified.

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


Qodo Logo

Comment thread jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java
Comment thread jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java
Comment thread jabgui/src/main/java/org/jabref/gui/LibraryTab.java Outdated
Comment thread jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java
Comment thread jabgui/src/main/java/org/jabref/gui/frame/FrameDndHandler.java Outdated
@Eng-YasminKotb

Copy link
Copy Markdown
Contributor Author

my plan for this feature:

  1. make the cursor reflect the right operation (move/ copy ) [done]
  2. Pass TransferMode [need some improvements]
  3. Execute Move [to do]
  4. Test both behaviors [to do]

}
}

private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koppor
Should I move this conversion method to another class (may be utility)?

@github-actions github-actions Bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Jul 19, 2026
}
}

private org.jabref.model.TransferMode toModelTransferMode(TransferMode javafxTransferMode) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid long qualifiers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what about having both javafx.scene.input.TransferMode and org.jabref.model.TransferMode used in FrameDndHandler?

@github-actions github-actions Bot added status: changes-required Pull requests that are not yet complete and removed status: no-bot-comments labels Jul 25, 2026
@github-actions github-actions Bot added status: no-bot-comments and removed status: changes-required Pull requests that are not yet complete labels Jul 25, 2026
GroupTreeNode copiedNode = parent.addSubgroup(groupTreeNodeToCopy.copyNode().getGroup());
// add all entries of a groupTreeNode to the new library.
destinationLibraryTab.dropEntry(stateManager.getActiveDatabase().get(), groupTreeNodeToCopy.getEntriesInGroup(allEntries));
destinationLibraryTab.dropEntry(stateManager.getActiveDatabase().get(), groupTreeNodeToCopy.getEntriesInGroup(allEntries), groupTreeNodeToCopy.getEntriesInGroup(allEntries), org.jabref.model.TransferMode.COPY);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only existing call that passes the same list for both originalEntries and entriesToAdd, is this approach acceptable ?

@Eng-YasminKotb
Eng-YasminKotb marked this pull request as ready for review July 25, 2026 22:31

@SuppressWarnings({"FieldCanBeLocal"})
private Subscription dividerPositionSubscription;
@SuppressWarnings({"FieldCanBeLocal"}) private Subscription dividerPositionSubscription;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Unrelated reformatting in librarytab 📘 Rule violation ⚙ Maintainability

This PR includes multiple whitespace/line-wrapping-only edits (e.g., collapsing multi-line
declarations/constructors into single long lines) that add diff noise and reduce readability without
being required for the behavior change. This violates the requirement to preserve existing
formatting and avoid unrelated reformatting.
Agent Prompt
## Issue description
The PR includes formatting-only changes (line wrapping/whitespace) unrelated to the functional drag-and-drop/move behavior change.

## Issue Context
Compliance requires minimizing diff noise and preserving existing formatting except where needed for the change.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[155-180]
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[101-120]

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

Comment on lines 252 to 258
AutoDetectionImportOutcome createAutoDetectionImportOutcome(Path file, ImportResult importResult) {
List<BibEntry> importedEntries = importResult.parserResult().getDatabase().getEntries();
if (importResult.parserResult().hasWarnings()) {
String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1",
importResult.format(),
importResult.parserResult().getErrorMessage());
String warningMessage = Localization.lang("File was imported as %0, but warnings were reported: %1", importResult.format(), importResult.parserResult().getErrorMessage());
if (importedEntries.isEmpty()) {
return new AutoDetectionImportOutcome(
List.of(createEmptyEntryWithLink(file)),
false,
Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format())
+ " "
+ importResult.parserResult().getErrorMessage());
return new AutoDetectionImportOutcome(List.of(createEmptyEntryWithLink(file)), false, Localization.lang("No importable data was found in %0. An empty entry was created with file link.", importResult.format()) + " " + importResult.parserResult().getErrorMessage());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Localized message built by concatenation 📘 Rule violation ⚙ Maintainability

A user-facing message is constructed via string concatenation (`Localization.lang(...) + " " +
...`), which prevents proper localization and translator reordering. This violates the
placeholder-based localization requirement.
Agent Prompt
## Issue description
A user-facing localized message is built using string concatenation instead of a single translation string with placeholders.

## Issue Context
Localization rules require the full message format to be in the translation resource, with dynamic parts provided via placeholders.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[252-258]
- jablib/src/main/resources/l10n/JabRef_en.properties[3037-3045]

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

Comment on lines +313 to +319
handleDuplicates(entryToInsert, existingDuplicateInLibrary.get(), decision).thenAccept(duplicateHandledEntry -> {
if (duplicateHandledEntry.isEmpty()) {
tracker.markSkipped();
return;
}
continueImportAfterDuplicateHandling(transferInformation, entry, tracker);
}).exceptionally(exception -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Duplicate result ignored 🐞 Bug ≡ Correctness

In ImportHandler.importEntryWithDuplicateCheck, when a duplicate is found and resolved, the code
ignores the Optional<BibEntry> returned from handleDuplicates and imports the original entry
instead. This bypasses merge/keep-right outcomes and can insert the wrong (uncleaned/unmerged) entry
into the target library.
Agent Prompt
## Issue description
When a duplicate is detected and `handleDuplicates(...)` returns a non-empty `Optional<BibEntry>`, the import flow must continue with that returned entry. The current code continues with the original `entry`, discarding duplicate-resolution results (e.g., merged entry) and the cleaned-up entry variant.

## Issue Context
- `handleDuplicates(...)` may return a merged entry or other decision-dependent result.
- The current callback checks `duplicateHandledEntry.isEmpty()` but then imports `entry` anyway.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[303-327]

## Suggested change
Replace:
- `continueImportAfterDuplicateHandling(transferInformation, entry, tracker);`
With:
- `continueImportAfterDuplicateHandling(transferInformation, duplicateHandledEntry.get(), tracker);`

Also ensure the imported entry is the cleaned/decision-result entry (not the original input).

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

Comment on lines +331 to +343
BackgroundTask.wrap(() -> adjustLinkedFilesForTargetIfRequired(transferInformation, entryForImport)).onFailure(e -> {
LOGGER.error("Error adjusting linked files for target", e);
dialogService.notify(Localization.lang("Could not adjust linked files. The entry was imported without linked-file adjustments."));
importCleanedEntries(transferInformation, List.of(entryForImport));
tracker.markImported(entryForImport);
}).onSuccess(adjustedEntry -> {
importCleanedEntries(transferInformation, List.of(adjustedEntry));
tracker.markImported(adjustedEntry);
if (transferInformation != null && transferInformation.transferMode() == org.jabref.model.TransferMode.MOVE && tracker.getImportedCount() == transferInformation.sourceEntries().size()) {
BibDatabase sourceDatabase = transferInformation.bibDatabaseContext().getDatabase();
List<BibEntry> sourceEntries = transferInformation.sourceEntries();
sourceDatabase.removeEntries(sourceEntries);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

4. Move cleanup order-dependent 🐞 Bug ☼ Reliability

Source entry removal for MOVE is executed only inside the linked-file-adjustment onSuccess callback,
while the onFailure callback still imports the entry and increments the imported count without ever
attempting cleanup. Because per-entry imports run concurrently, MOVE cleanup becomes order-dependent
and can be skipped entirely if the last completed entry goes through onFailure.
Agent Prompt
## Issue description
MOVE source-entry cleanup (`sourceDatabase.removeEntries(sourceEntries)`) is performed only in the `onSuccess` branch of linked-file adjustment. If the last processed entry completes via `onFailure`, cleanup never runs even though the failure path still imports the entry and marks it as imported.

## Issue Context
- Imports run concurrently (one task chain per entry).
- `onFailure` calls `importCleanedEntries(...)` and `tracker.markImported(...)` but does not evaluate MOVE cleanup.
- Linked-file adjustment can involve filesystem operations and may fail.

## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java[330-345]
- jabgui/src/main/java/org/jabref/gui/externalfiles/EntryImportHandlerTracker.java[19-75]

## Suggested fix
Implement MOVE cleanup in a completion-safe place that runs exactly once after all entries are processed, regardless of per-entry success/failure ordering. Two viable approaches:
1) Call a shared `maybeFinalizeMove(transferInformation, tracker)` after `tracker.markImported(...)` in BOTH `onSuccess` and `onFailure`, and guard it so it runs once.
2) Move the MOVE finalization to the tracker’s `onFinish` hook (single-shot), checking `transferMode == MOVE` and `importedCount == sourceEntries.size()` before removing source entries.

Ensure the cleanup cannot be skipped due to callback ordering.

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

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit be69001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good first issue An issue intended for project-newcomers. Varies in difficulty. status: no-bot-comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for modifier keys when dragging and dropping an entry.

2 participants