Prevent storing absolute paths when memory stick mode is enabled#15026
Closed
Nourhan46 wants to merge 6 commits intoJabRef:mainfrom
Closed
Prevent storing absolute paths when memory stick mode is enabled#15026Nourhan46 wants to merge 6 commits intoJabRef:mainfrom
Nourhan46 wants to merge 6 commits intoJabRef:mainfrom
Conversation
github-actions
Bot
added
good first issue
Nourhan46
changed the title
github-actions
Bot
removed
the
status: changes-required
koppor
reviewed
Feb 4, 2026
| // String delimiter | ||
| public static final Character STRINGLIST_DELIMITER = ';'; | ||
|
|
||
| // RECENT_DATABASES |
Author
There was a problem hiding this comment.
I moved it because when I put it in last of variable the check style give me error in order declaration to solve it I moved it with public static final variables then I put comment to declare the change according the remain code
Member
There was a problem hiding this comment.
It hopefully did not. Try again. - REvert to old code - comment is not requierd.
Author
There was a problem hiding this comment.
okay i removed unnecessary comment , i am sorry i just try to make my code similar and use the rules used in code of project , what should i do next can give me a hint
Member
There was a problem hiding this comment.
Educate yourself what "revert to old code" could mean.
koppor
requested changes
Feb 4, 2026
| // String delimiter | ||
| public static final Character STRINGLIST_DELIMITER = ';'; | ||
|
|
||
| public static final String RECENT_DATABASES = "recentDatabases"; |
Member
There was a problem hiding this comment.
Revert too hard for contributor.
Not sure if AI AI AI AI AI AI AI AI
Author
There was a problem hiding this comment.
I am sorry i misunderstood , now i make all the changes required , please give me a feed back ,thank you for your effort
github-actions
Bot
added
status: changes-required
Nourhan46
force-pushed
the
fix-for-issue-3590
branch
from
b64b1e9 to
0c2c953
Compare
github-actions
Bot
added
the
status: changes-required
koppor
requested changes
Feb 4, 2026
| .map(Path::toAbsolutePath) | ||
| .map(Path::toString) | ||
| .toList()); | ||
| Path basePath = Path.of(".").toAbsolutePath(); |
Member
There was a problem hiding this comment.
Variable only used in if branch -> move there
| Path absoultePath = relativePath.toAbsolutePath(); | ||
| preferences.getLastFilesOpenedPreferences().getFileHistory().add(absoultePath); | ||
| List<String> storedList = preferences.getStringList("recentDatabases"); | ||
| assertEquals(relativePath, Path.of(storedList.get(0))); |
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| class JabRefCliPreferencesTest { | ||
| @Test |
Member
There was a problem hiding this comment.
Is it possible to "mirror" this test - and for non-memory stick mode?
Comment on lines
+14
to
+15
| var preferences = new JabRefCliPreferences(); | ||
| preferences.clear(); |
Member
There was a problem hiding this comment.
In JabRef tests, we DO NOT alter existing preferences. But I think, this is not possible in this case?
Then, we need to go for @Disabled, because we do not want to destroy preferences of developers (which use jabref in der day job)
Contributor
|
The requested changes were not addressed for 10 days. Please follow-up in the next 10 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
github-actions
Bot
added
the
status: stale
github-actions
Bot
removed
the
status: stale
🚨 TestLens detected 58 failed tests 🚨Here is what you can do:
Test Summary
🏷️ Commit: 9c3c2a9 Test Failures (first 5 of 58)AuthorTest > addDotIfAbbreviation(String) > [3] "# Lower-case letters\nasdf\na\n# Numbers\n1\n1 23\n" (:jablib:test in Source Code Tests / Unit tests (Windows) - file-based)CSLFormatUtilsTest > ooHTMLTransformFromCitationWithMultipleEntries(String, CitationStyle) (:jablib:test in Source Code Tests / Unit tests (Windows) - file-based)CSLFormatUtilsTest > ooHTMLTransformFromCitationWithSingleEntry(String, CitationStyle) (:jablib:test in Source Code Tests / Unit tests (Windows) - file-based)CSLFormatUtilsTest > ooHTMLTransformFromRawBibliography(String, CitationStyle) (:jablib:test in Source Code Tests / Unit tests (Windows) - file-based)CSLStyleUtilsTest > bibliographicPropertyMatches(boolean, String) > [3] true, "nlm-citation-sequence.csl" (:jablib:test in Source Code Tests / Unit tests (Windows) - file-based)Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
Author
|
i made all changes required , please give me feedback |
Contributor
|
Do not mark a PR as ready-for-review while the CI is running. CI checks need to be completed and passed. |
Contributor
Review Summary by QodoStore relative paths in memory stick mode
WalkthroughsDescription• Store relative paths when memory stick mode enabled • Store absolute paths when memory stick mode disabled • Add JUnit tests for both memory stick mode scenarios Diagramflowchart LR
A["File History"] --> B{"Memory Stick Mode?"}
B -->|"Yes"| C["Relativize Path"]
B -->|"No"| D["Absolute Path"]
C --> E["Store in Preferences"]
D --> E
File Changes1. jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java
|
Contributor
|
Do not request reviews if changes are required. |
Contributor
Code Review by Qodo
1. storeFileHistory uses CWD base
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1974
to
+1979
| if (getInternalPreferences().isMemoryStickMode()) { | ||
| Path basePath = Path.of(".").toAbsolutePath(); | ||
| putStringList(RECENT_DATABASES, history.stream() | ||
| .map(path -> basePath.relativize(path)) | ||
| .map(Path::toString) | ||
| .toList()); |
Contributor
There was a problem hiding this comment.
1. storefilehistory uses cwd base 📎 Requirement gap ✓ Correctness
In memory stick mode, storeFileHistory computes relative paths from the current working directory and unconditionally calls relativize, rather than using the JabRef executable location as the base. This can store incorrect recent-library entries and may throw at runtime (e.g., differing roots/drives), breaking persistence.
Agent Prompt
## Issue description
When memory stick mode is enabled, recent-library paths must be stored relative to the JabRef executable location. The current implementation uses the process working directory as the base and unconditionally calls `relativize`, which can produce incorrect relative paths and can throw for non-relativizable paths (e.g., different roots/drives).
## Issue Context
Compliance requires relative paths to be computed from the JabRef executable directory in memory stick mode, while handling edge cases gracefully.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1974-1979]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+12
to
+13
| @Disabled | ||
| class JabRefCliPreferencesTest { |
Contributor
There was a problem hiding this comment.
2. jabrefclipreferencestest is disabled 📘 Rule violation ⛯ Reliability
The newly added tests are annotated with @Disabled, so they will not run in CI and cannot protect the behavior change from regressions. This violates the requirement that behavioral changes include reliable, CI-friendly tests.
Agent Prompt
## Issue description
The new JUnit tests are disabled and therefore do not validate the behavior change in CI.
## Issue Context
Behavior changes must be covered by active, reliable tests.
## Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/preferences/JabRefCliPreferencesTest.java[12-37]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1974
to
+1984
| if (getInternalPreferences().isMemoryStickMode()) { | ||
| Path basePath = Path.of(".").toAbsolutePath(); | ||
| putStringList(RECENT_DATABASES, history.stream() | ||
| .map(path -> basePath.relativize(path)) | ||
| .map(Path::toString) | ||
| .toList()); | ||
| } else { | ||
| putStringList(RECENT_DATABASES, history.stream() | ||
| .map(Path::toAbsolutePath) | ||
| .map(Path::toString) | ||
| .toList()); |
Contributor
There was a problem hiding this comment.
3. May open duplicate tabs 🐞 Bug ✓ Correctness
Storing relative paths in recent databases makes it more likely that opening a “recent library” uses a relative Path, but OpenDatabaseAction checks for already-open libraries using equals against the (absolute) databasePath, so the same library can be opened twice.
Agent Prompt
### Issue description
With this PR, recent libraries may be loaded/stored as relative paths. `OpenDatabaseAction.openFiles` checks for already-open libraries using `databasePath.equals(file)` (raw input). Since the loader converts `file` to absolute, an already-open absolute path may never equal a relative one, allowing duplicate tabs.
### Issue Context
- Recent database paths are loaded using `Path::of` (no normalization).
- In memory stick mode, recent database paths are persisted as relative strings.
- `OpenDatabaseAction` compares un-normalized input paths but loads absolute paths.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[1967-1986]
- jabgui/src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java[165-199]
- jabgui/src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java[237-243]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
|
Your pull request modified git submodules. Please follow our FAQ on submodules to fix. |
Collaborator
|
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
Contributor
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
Contributor
|
This pull requests was closed without merging. You have been unassigned from the respective issue #3590. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3590
i check if memory stick mode is true then save relative path , if not save absolute path and then create a Junit class to check if stick mode is true what happened and it passed
this is draft pull request i would appreciate any feedback on logic , implementation and my description in pull request
Steps to test
1- go to folder test file JabRefCliPreferencesTest
2- run the test function
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)