Conversation
Review Summary by QodoIntegrate SearchRxiv with SLR feature for query export
WalkthroughsDescription• Add SearchRxiv integration to SLR feature with export functionality • Create SearchRxivExporter to export queries as search-query JSON format • Add "Export search queries" button in Finalize tab with validation • Refactor buildStudy() method for code reuse across save and export • Fix pre-existing compile error in AcademicPagesExporter Diagramflowchart LR
A["SLR Study Definition"] -->|buildStudy| B["Study Object"]
B -->|SearchRxivExporter| C["JSON Files<br/>search-query format"]
C -->|export| D["Output Directory"]
D -->|open browser| E["SearchRxiv Website"]
File Changes1. jabgui/src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java
|
Code Review by Qodo
1. export() passes null repository
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| @Override | ||
| public void export(BibDatabaseContext databaseContext, Path outputFile, List<BibEntry> entries) throws SaveException { | ||
| export(databaseContext, outputFile, entries, List.of(), JournalAbbreviationLoader.loadBuiltInRepository()); | ||
| export(databaseContext, outputFile, entries, List.of(), null); |
There was a problem hiding this comment.
1. export() passes null repository 📘 Rule violation ⛯ Reliability
AcademicPagesExporter.export(...) now passes null for abbreviationRepository despite the class being @NullMarked, which can violate nullability assumptions and lead to runtime NPEs when the repository is used downstream.
Agent Prompt
## Issue description
`AcademicPagesExporter.export(...)` forwards `null` as `abbreviationRepository` even though the code path passes it into `LayoutHelper` without null handling.
## Issue Context
The class is `@NullMarked`, so parameters are non-null by default; passing `null` here can violate null-safety expectations and cause NPEs.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/exporter/AcademicPagesExporter.java[75-78]
- jablib/src/main/java/org/jabref/logic/exporter/AcademicPagesExporter.java[169-179]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This comment has been minimized.
This comment has been minimized.
|
From your singular commit for this feature
Can you point us to the "pre-existing compile error" in AcademicPagesExporter? |
The pre-existing compile error where loadBuiltInRepository() was called with no arguments: loadBuiltInRepository() had been updated to require a DataSource argument, making this call fail to compile: |
The single commit was intentional to keep the PR history clean, during development, the branch accumulated extra commits from merging main to fix a compile error, so to clean up the diff and show only the changes relevant to this issue, i squashed all my commits into one and rebased onto main If this behavior is inconveniencing, tell me and i will try to split those commits |
I did not ask for an LLM copy paste, rather a pointer to where in our main branch does the pre-existing compile error exist. Anyway, we can guess what happened here. Add disclosures of the code, commit etc being generated by AI to the PR description. Usually we don't tolerate misrepresentation but since you have tested this, we'll review the PR at some point. |
I apologize for you, @subhramit, i did not mean to depend on LLM fully, you are right the commits part was by the LLM becuase i faced problems while merging main. but i am always being careful of using ai and try as much as possible to depend on myself and from the documentation, |
LoayTarek5
force-pushed
the
integrate-with-SearchRxiv-12618
branch
from
006fc76 to
3b53d84
Compare
This comment has been minimized.
This comment has been minimized.
github-actions
bot
added
status: changes-required
LoayTarek5
force-pushed
the
integrate-with-SearchRxiv-12618
branch
from
3b53d84 to
aec497b
Compare
This comment has been minimized.
This comment has been minimized.
|
I apologize for the force pushes, i was trying to clean up the commit history , i will avoid force pushing |
✅ All tests passed ✅🏷️ Commit: 91cd952 Learn more about TestLens at testlens.app. |
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
|
Can you address qodo comments please? |
github-actions
bot
added
status: changes-required
LoayTarek5
force-pushed
the
integrate-with-SearchRxiv-12618
branch
2 times, most recently
from
e56addd to
b1ef4fd
Compare
|
I rebased and squashed into a single commit, |
|
We don't like force pushes, because all the comments we made to the code are detached in and partially lost. It adds a lot to our workload, reading your changes again and to do the review partially again. |
…ON format, adhering to the search-query library's specifications.
|
Thanks for answering! Sorry, I misleaded you. By database I actually meant what we have in JabRef for “catalog”. What I mean is that you can (probably) create one json file per 1 question that would search across different catalogs (e.g. arxiv, google scholar). Because the field “database” is a list |
…ON structure for study exports
LoayTarek5
force-pushed
the
integrate-with-SearchRxiv-12618
branch
from
148882f to
ec9f863
Compare
|
@LoayTarek5 kindly stop force pushing changes. |
i know that is the second time😅 happened |
|
Yes this was the second time. And it will be the last time. Again, this causes more work on our side since comments on code get detached and some get lost. If you have problems, ask us. Even a bit of a mess with fixing merge commits afterwards happens all the time. Instead I feel like you are trying to hide something from us. First time force pushing might happen by accident. Second time, being informed by bot message and having us mentioned it (if I remember correctly), was obviously knowingly and willingly against our contribution rules. |
I know that causes more work on your side, and I'm really sorry for that @calixtus. |
There were no conflicting changes between
That is the purpose of a merge commit, to integrate the changes in other files in the feature branch so that history is kept in sync. See if you are merging with rebase enabled or something - simply one merge commit should be present when a merge happens. |
|
Ok @subhramit, Thanks for that, i will learn more, especially when conflicts happen, and of course, you are right i have to ask questions instead of just going on my way |
I searched to verify that, and you are right. json also in JabRef each StudyDatabase entry (IEEE, ACM Portal, Springer, DBLP) resolves to a separate SearchBasedFetcher. I will add an empty database list to the JSON output to fully match the format spec. |
Okay cool, so So this finally answers my question of why there are many separate JSON files |
| private void shareOnSearchRxiv() { | ||
| viewModel.shareOnSearchRxiv( | ||
| pathToStudyDataDirectory, | ||
| preferences.getExternalApplicationsPreferences()); |
There was a problem hiding this comment.
The view model should get and store external applications preferences, so the method shareOnSearchRxiv should accept only one argument
| dialogService.showDirectorySelectionDialog(config).ifPresent(exportDirectory -> { | ||
| try { | ||
| new SearchRxivExporter().export(study, exportDirectory); | ||
| NativeDesktop.openBrowserShowPopup( |
There was a problem hiding this comment.
I'm still not sure if we want to open a website after exporting the search queries to JSON. @koppor @calixtus @subhramit what do you think?
There was a problem hiding this comment.
I think this is partially known behaviour, if I export something, I open the browser to show the result of the export. If it's just Json exportet, I wouldn't show the result, its just technical.
There was a problem hiding this comment.
I couldn’t really find the button “use json search” on the website, so I think it’s better not to show
| LaTeX\ citations=LaTeX citations | ||
| Search\ for\ citations\ in\ LaTeX\ files...=Search for citations in LaTeX files... | ||
| LaTeX\ Citations\ Search\ Results=LaTeX Citations Search Results | ||
| PDF\ file=PDF file |
There was a problem hiding this comment.
Oh, but why are there so many deletions? I think the feature only introduces new strings, or not?
There was a problem hiding this comment.
The deletions in JabRef_en.properties are from a bad merge, not intentional, i will fix it.
|
@InAnYan, Could you please reset the integrate-with-SearchRxiv-12618 branch to main? The branch has accumulated many unrelated commits from previous bad merges. I will then push only my changes cleanly on top (searchrxiv-backup) $ git log upstream/main..integrate-with-SearchRxiv-12618 --oneline (searchrxiv-backup) $ git diff upstream/main --name-only |
|
I have no idea what you are trying to tell us? The branch on upstream looks just fine. If you want to reset your local branch with "many bad commits" use reset on your local branch to return to an earlier stage of your branch. For every emergency question: Look at https://ohshitgit.com/ |
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
Finally, i push it this time without |
…k5/jabref into integrate-with-SearchRxiv-12618
Please also check if you have mode |
Ok, @subhramit, i will check to make sure that everything will go smoothly |
4 participants
Related issues and pull requests
Closes #12618
PR Description
Adds SearchRxiv integration to the SLR feature.
A new "Export search queries (search-query format)" button in the Finalize tab exports study search queries as JSON files following the search-query library standard format , then opens SearchRxiv in the browser for submission.
Steps to test
1- Go to Tools > Start new systematic literature review
2- Fill in all tabs:
Authors and Title: add a title and at least one author
Research Questions: add at least one question
Queries: add a query
Catalogs: enable at least one catalog
Finalize: select a study directory
3- In the Finalize tab, click "Export search queries (search-query format)", then choose an output directory
4- Browser opens to https://www.cabidigitallibrary.org/journal/searchrxiv
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)