Skip to content

recomment linked files handler#15105

Merged
koppor merged 5 commits intomainfrom
recommentLInkedFiles
Feb 13, 2026
Merged

recomment linked files handler#15105
koppor merged 5 commits intomainfrom
recommentLInkedFiles

Conversation

@Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Feb 13, 2026

Related issues and pull requests

Fixes #15104

PR Description

Steps to test

Checklist

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

@qodo-free-for-open-source-projects
Copy link
Contributor

Review Summary by Qodo

Uncomment disabled linked files handler test case

🧪 Tests

Grey Divider

Walkthroughs

Description
• Uncommented previously disabled test case in LinkedFileTransferHelperTest
• Restored test for relative file linking to main file directory
Diagram
flowchart LR
  A["Commented Test Case"] -- "Uncommented" --> B["Active Test Case"]
  B -- "Tests" --> C["LinkedFileTransferHelper"]
Loading

Grey Divider

File Changes

1. jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java 🧪 Tests +1/-2

Uncomment disabled test case for file linking

• Removed multi-line comment markers that disabled a test case
• Test case validates file linking relative to main file directory
• Test now executes as part of the parameterized test suite

jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java


Grey Divider

Qodo Logo

@testlens-app
Copy link

testlens-app bot commented Feb 13, 2026

✅ All tests passed ✅

🏷️ Commit: 8e1ffa1
▶️ Tests: 11187 executed
⚪️ Checks: 52/52 completed


Learn more about TestLens at testlens.app.

@qodo-free-for-open-source-projects
Copy link
Contributor

qodo-free-for-open-source-projects bot commented Feb 13, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

✅ 1. Flaky test re-enabled 🐞 Bug ⛯ Reliability
Description
The uncommented parameter set can fail nondeterministically because the helper searches by filename
under the main file directory tree and picks the first match, while the test setup creates multiple
test.pdf candidates under that tree. This can make CI unreliable or fail depending on filesystem
traversal order.
Code

jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java[R122-124]

               Arguments.of(
                       "File in main file directory linked as is",
                       FileTestConfigurationBuilder
Evidence
The re-enabled test config creates test.pdf in two different locations under the *same* main file
dir tree: source uses pdfFileDir("main-file-dir") (so main-file-dir/test.pdf) while target uses
pdfFileDir("main-file-dir/sub-dir") (so main-file-dir/sub-dir/test.pdf). When the linked path
has no parent (e.g., test.pdf), adjustLinkedFilesForTarget skips the deterministic
linkedFile.findIn(targetContext, ...) check and instead walks the target directories recursively
and returns the *first* path whose filename matches, which is ambiguous if multiple matches exist.
Also, for shouldStoreFilesRelativeToBibFile=false, the target directory list is just the global
mainFileDirectory (not the sub-dir), so this recursive walk is exactly over the shared
main-file-dir tree where both candidates exist.

jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java[122-139]
jablib/src/test/java/org/jabref/logic/externalfiles/BibTestConfiguration.java[39-47]
jablib/src/test/java/org/jabref/logic/externalfiles/BibTestConfiguration.java[78-86]
jablib/src/main/java/org/jabref/model/database/BibDatabaseContext.java[180-194]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[90-112]
jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[155-162]

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 previously disabled parameterized test case was re-enabled. With the current setup, it can be flaky because the test creates multiple `test.pdf` files under the same main-file-dir tree, while `LinkedFileTransferHelper` may select an arbitrary match via `Files.walk(...).findFirst()`.
### Issue Context
- Source test setup creates `main-file-dir/test.pdf`.
- Target test setup creates `main-file-dir/sub-dir/test.pdf`.
- For non-bib-relative storage, the file directory list comes from `FilePreferences.getMainFileDirectory()`, i.e., the shared `main-file-dir` root.
- The helper’s recursive search matches by filename only and takes the first hit.
### Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/externalfiles/LinkedFileTransferHelperTest.java[122-139]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[90-112]
- jablib/src/main/java/org/jabref/logic/externalfiles/LinkedFileTransferHelper.java[155-162]

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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@Siedlerchr
Copy link
Member Author

added a commebt

@koppor
Copy link
Member

koppor commented Feb 13, 2026

Very strange that the test worked before...

@koppor koppor added this pull request to the merge queue Feb 13, 2026
@github-actions github-actions bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Feb 13, 2026
Merged via the queue into main with commit 5b88de0 Feb 13, 2026
53 checks passed
@koppor koppor deleted the recommentLInkedFiles branch February 13, 2026 11:45
Siedlerchr added a commit to faneeshh/jabref that referenced this pull request Feb 14, 2026
* upstream/main:
  Refine Automatic Field Editor filtering logic (fixes JabRef#15066) (JabRef#15094)
  Output URL to workflow
  Fix token
  Refine stats message
  Quick fix to reduce load on runners
  "Debug" output for assign-issue-action
  Streamline pr-comment.yml (and remove status: stale label)
  Feature provide insights citation fetcher (JabRef#15093)
  Automatic Grouping By Entry Type (JabRef#15081)
  Minor test fixes for arXiv (JabRef#15100)
  New Crowdin updates (JabRef#15101)
  recomment linked files handler (JabRef#15105)
  Chore(deps): Bump jablib/src/main/resources/csl-styles (JabRef#15087)
  Streamline binaries (JabRef#15085)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix "LinkedFileTransferHelperTest"

2 participants