Add entry from url - #16435
Conversation
GenericUrlBasedFetcher wraps an arbitrary URL into a @misc BibEntry, populating url, a best-effort scraped page title (falling back to the URL itself if the fetch fails), and urldate. Part of JabRef#15411.
Wires GenericUrlBasedFetcher into a new tab, placed after "Enter Identifier", following the same async fetch -> duplicate-check -> import pattern used by the Interpret Citations and Specify BibTeX tabs. Fixes JabRef#15411.
isURL() matched the raw string against the URL pattern before trimming, so a pasted URL with a leading/trailing space (e.g. from a clipboard paste) was incorrectly reported as invalid, even though URLUtil.create() right after it already trims. Trim once up front, matching create()'s behavior. This also fixes the same latent issue in the library-table paste handler and the URL field in the entry editor, which share isURL().
github-actions
Bot
added
good first issue
PR Summary by QodoAdd “Enter URL” tab to create @misc entries from pasted links
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
github-actions
Bot
added
the
status: changes-required
- fetchTitle now skips non-http(s) URLs (e.g. ftp://) before calling Jsoup, which only supports http/https and threw an uncaught IllegalArgumentException for anything else, aborting entry creation entirely. Added a regression test for this. - Add an explicit 30s connect timeout, matching the existing DoiResolution.java convention, so a stalled host can't hang the background task indefinitely. - Annotate UrlBasedFetcher and GenericUrlBasedFetcher with @NullMarked, per AGENTS.md's rule for new classes. - Drop the string concatenation in the one Localization.lang(...) call that's new to this PR; left the identical pre-existing "An unknown error has occurred..." message (shared verbatim with two other tabs in the same file) untouched to avoid making three copies inconsistent.
aadb485 to
d867d56
Compare
Per Qodo review: Platform.runLater(() -> urlText.requestFocus()) could fire after the user switches away from the Enter URL tab, stealing focus back to a field that's no longer visible. Re-check tabEnterUrl.isSelected() inside the callback before requesting focus. Scoped to only this new tab -- the three sibling tabs (switchLookupIdentifier, switchInterpretCitations, switchSpecifyBibtex) have the identical unguarded pattern already, pre-existing and out of this PR's scope.
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
|
Code review by qodo was updated up to the latest commit b2458fa |
- Move ENTER_URL to the end of NewEntryDialogTab. JabRefGuiPreferences persists the last-used tab as the ordinal index into values(); inserting it in the middle shifted every later constant's ordinal, silently remapping existing users' stored tab preference. The tab's visual position (right after Enter Identifier) is unaffected -- that comes from NewEntry.fxml's element order, not the enum's declaration order. - Capture the expected urldate before calling performSearch in GenericUrlBasedFetcherTest, not after. The production code also calls LocalDate.now() internally during its (real, network-bound) title fetch, so asserting with a post-call LocalDate.now() risked a midnight rollover making the test flaky. - Add req~fetchers.generic-url~1 (docs/requirements/fetchers.md) with a matching [impl->...] tag, closing an OpenFastTrace coverage gap for this new feature. Verified via ./gradlew traceRequirements.
github-actions
Bot
added
status: changes-required
|
/review |
|
Code review by qodo was updated up to the latest commit 6fa64f8 |
| // Appended after the pre-existing constants (rather than grouped next to ENTER_IDENTIFIER, which is where it | ||
| // appears in NewEntry.fxml's tab order -- unrelated to this enum's declaration order) because | ||
| // JabRefGuiPreferences persists the last-used tab as the ordinal index into values(). Inserting a constant in | ||
| // the middle would silently shift every later constant's ordinal, remapping existing users' stored preference | ||
| // to the wrong tab. | ||
| ENTER_URL, |
| String trimmedUrl = url.trim(); | ||
|
|
||
| // Check if the URL has a protocol (http://, https://, ftp://) | ||
| if (!URL_PATTERN.matcher(url).matches()) { | ||
| if (!URL_PATTERN.matcher(trimmedUrl).matches()) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| create(url); | ||
| create(trimmedUrl); |
There was a problem hiding this comment.
bad naming, why not make it inline
There was a problem hiding this comment.
I've reverted the helper change to avoid changing 3 existing callers of isURL(unrelated to this issue).
|
The requested changes were not addressed for 3 days. Please follow-up in the next 7 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
- Revert URLUtil.isURL() trimming: widening it to accept surrounding whitespace created a validate/consume mismatch for existing callers that pass the raw string to strict URI consumers. Trimming is now scoped to the Enter URL tab's own validator; the fetcher already trims before use. - Log only scheme and host of a failing URL, since userinfo and query parameters can embed credentials or access tokens. - Remove the enum ordering comment from NewEntryDialogTab (ENTER_URL stays declared last; the last-used tab is persisted by ordinal).
|
/review |
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
|
@LoayTarek5 @pluto-han I've tried to address qodo comments, and reverted the isURL helper change. |
|
Code review by qodo was updated up to the latest commit 38156be |
Ok, i will review it by today or tomorrow at the latest |
| private final GenericUrlBasedFetcher fetcher = new GenericUrlBasedFetcher(); | ||
|
|
||
| @Test | ||
| void performSearchWithValidUrlReturnsMiscEntryWithTitleAndUrldate() throws FetcherException { |
There was a problem hiding this comment.
This test can not fail for the reason it exists, bec GenericUrlBasedFetcher sets the title to fetchTitle.orElse(trimmedUrl), and the URL is always non blank
There was a problem hiding this comment.
yup, fixed. Now the URL-as-title fallback no longer passes as a scraped title.
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @FetcherTest | ||
| class GenericUrlBasedFetcherTest { |
There was a problem hiding this comment.
The invalid URL, blank input and getName tests touch no network but are excluded from the default test task, so tag the four network tests instead
There was a problem hiding this comment.
moved to method level for the 3 tests, 1 test is now offline.
|
|
||
| private class WorkerEnterUrl extends Task<Optional<List<BibEntry>>> { | ||
| @Override | ||
| protected Optional<List<BibEntry>> call() throws FetcherException { |
There was a problem hiding this comment.
i think the WorkerEnterUrl.call() and result.isEmpty() below, these guards can not trigger, bec performSearch always return List.of(entry), so entries.isEmpty() is never true, and the blank, invalid recheck duplicates urlTextValidator, maybe call() can return List directly, right?
There was a problem hiding this comment.
yes call can return list directly.
| validator.configureValidation(viewModel.duplicateDoiValidatorStatus(), textInput); | ||
| } | ||
|
|
||
| private void initializeEnterUrl() { |
There was a problem hiding this comment.
i try to think about that, but uncnditional clipboard prefill means the tab opens showing a red "You must provide a valid URL." for ordinary clipboard text,right? try to see the identifier tab guards this at NewEntryView
There was a problem hiding this comment.
Fixed, now clipboard invalid urls won't prefill and instead the tab would remain empty.
|
|
||
| /// Reduces a user-provided URL to its scheme and host for logging, since the rest of the URL (userinfo, path, | ||
| /// query, fragment) commonly carries credentials or access tokens that must not end up in application logs. | ||
| private static String sanitizeUrlForLogging(String url) { |
There was a problem hiding this comment.
try to see FetcherException already handles this, so getLocalizedMessage runs getRedactedUrl which strips userinfo and API key query params, use a URL-carrying constructor instead
There was a problem hiding this comment.
Done, dropped the sanitizer. using redacted url and localized message now.
| /// Best-effort fetch of the target page's `<title>`. A failure here (unreachable host, timeout, no | ||
| /// title tag, ...) should never prevent the entry from being created, so this swallows the error and lets the | ||
| /// caller fall back to using the URL itself as the title. | ||
| /// | ||
| /// Only attempted for `http`/`https` URLs — [Jsoup.connect] only supports those schemes and throws | ||
| /// `IllegalArgumentException` for anything else (e.g. `ftp://`, which [URLUtil.isURL] otherwise accepts). | ||
| private Optional<String> fetchTitle(String url) { | ||
| String scheme = url.toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
Jsoup.connect throws IllegalArgumentException for non-http schemes, but jsoup throws MalformedURLException, which is an IOException and already caught below, right
There was a problem hiding this comment.
yes, the guard was redundant. fixed.
| urlTextValidator = new FunctionBasedValidator<>( | ||
| urlText, | ||
| input -> input != null && URLUtil.isURL(input.trim()), | ||
| ValidationMessage.error(Localization.lang("You must specify a valid URL."))); |
There was a problem hiding this comment.
i think it is never rendered, only the FXML string shows
There was a problem hiding this comment.
Yes, the validator message was never displayed. It now reuses the FXML label's string.
| /// Looks for bibliographic information for the resource located at the given URL. | ||
| /// | ||
| /// @param url a string containing the URL of the resource | ||
| /// @return a list of [BibEntry], one entry per hit found for the URL | ||
| List<BibEntry> performSearch(String url) throws FetcherException; | ||
| } |
There was a problem hiding this comment.
the javadoc describes the method but not what implementations must guarantee
There was a problem hiding this comment.
Included in javadoc : implementations own validation (throw FetcherException for unhandled URLs), must degrade gracefully on optional-data failures, and return a possibly-empty but never-null list.
| /// added, and (best-effort) the target page's title. | ||
| @NullMarked | ||
| // [impl->req~fetchers.generic-url~1] |
There was a problem hiding this comment.
this sits between @NullMarked and the class declaration?
github-actions
Bot
added
status: changes-required
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
I have questions also, @koppor
1- The URL tab ignores identifiers, so if you paste somthing like https://dl.acm.org/doi/epdf/10.1145/... into the new tab, you get a plain @misc entry with just the link and the page title, and paste the same link into the Enter Identifier tab next to it, and you get a full entry with authors, journal and year.
2- is it should @misc or @online? i see the issue asks for @misc, but in NewEntryView file says Online is the type for web sites, and this PR also sets urldate, which is a biblatex field, so i suggest @online looks like the better fit for a plain web link, right?
I think because @online is only for BibLaTeX |
- Document the UrlBasedFetcher contract (multi-hit, validation ownership, graceful degradation on optional data) - Remove the redundant non-http scheme guard: jsoup rejects such URLs with a MalformedURLException before opening a connection, which the existing IOException handling already covers - Strengthen the live-scrape test so the URL-as-title fallback no longer passes as a scraped title; tag only the three tests that touch the network as @FetcherTest so the rest run per-PR - Simplify WorkerEnterUrl to Task<List<BibEntry>>; a single empty-result check remains in onSucceeded per the interface contract - Use FetcherException's URL redaction for log, dialog, and the invalid-URL message instead of a hand-rolled sanitizer - Only prefill the Enter URL field when the clipboard holds a URL, mirroring the Enter Identifier tab's guard - Drop the never-rendered duplicate "valid URL" localisation string
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
|
@LoayTarek5 I've done the required changes. code is a lot cleaner now. |
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
LGTM, great work @dvinit
LoayTarek5
added
status: awaiting-second-review
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWwniM7qqFkpfnS4wo1x3u
4 participants
Related issues and pull requests
Closes #15411
PR Description
Until now, JabRef could create a new entry from a DOI, ISBN, or arXiv ID — but not from a plain web link. This PR adds a new "Enter URL" tab to the New Entry dialog: paste any link, and JabRef creates an entry for it automatically, using the linked page's title when it can be reached, and recording the date it was added.
While testing this, we also found and fixed a small existing bug: pasting a link with an extra space at the start or end (which easily happens when copying a URL from a browser) was incorrectly rejected as invalid.
User documentation to follow in a separate PR.
Steps to test
Cmd/Ctrl+N, or Library menu → "Add entry using...").https://gi-radar.de/397-coding-unterstuetzung-im-lauf-der-zeit/, and click "Create".not a url) — the Create button should stay disabled and show an error.AI usage
Claude Code was used as a helper for exploring the codebase, resolving git issues and suggesting style fixes. The core feature plan and implementation for this task was done manually.
AI CHECKLIST.md walkthrough
Updated after automated (Qodo) review: 4 of its 6 findings were fixed (
@NullMarked, the non-http title-fetch bug, an explicit timeout, and oneLocalization.langconcatenation) and folded into the existing commits; 2 were deliberately left as-is with reasoning given in review-thread replies. This walkthrough reflects the current, post-fix state of the code.1. Code self-review
Nullability and control flow
== null/!= nullchecks —GenericUrlBasedFetcher.performSearchoriginally had one (url == null ? null : url.trim()); removed it to match the sibling fetchers in this same package (RfcFetcher,DoiFetcher,IssnFetcher), none of which null-check theirStringparameter — they let it propagate naturally.NewEntryView.switchEnterUrl()'sif (urlText != null)is unchanged from the file's existing convention (every siblingswitchXxx()method has the identical guard) — not a new pattern.Objects.requireNonNull(...)@NullMarked— added to bothUrlBasedFetcherandGenericUrlBasedFetcherin response to automated review. I'd originally left this out reasoning it mirrors the unannotated siblingIdBasedFetcher, butAGENTS.mdstates the rule unconditionally for new classes, with no carve-out for matching an unannotated sibling — so I fixed it rather than keep the exception.IdBasedFetcheritself is pre-existing code, untouched by this PR.Optionalconsumed withmap/orElse—fetchTitle(...).orElse(trimmedUrl)uses a real, meaningful fallback value, not a discarded one.StringUtil.isBlank(...)— used inWorkerEnterUrl.call().Exceptions
catch (Exception e)— onlyIOException(title fetch) andFetcherException/its subtypes.throw new RuntimeException(...)/IllegalStateException(...)Style and idioms
BibEntryobjects built with withers (.withField(...))List.of(entry))Pattern— no new regex introducedorg.jabref.logic.util.BackgroundTask— usesjavafx.concurrent.Taskvia the sharedtaskExecutorinstead, matching this exact file's existing convention for every sibling tab (WorkerLookupId,WorkerInterpretCitations,WorkerSpecifyBibtexall extendTask, notBackgroundTask) — followed the established in-file pattern rather than introducing a different one for only this tab.///) — fixed two spots that initially used old-style{@link}/{@code}tags in my own new files (UrlBasedFetcher.java,GenericUrlBasedFetcher.java); now use[ClassName]/backtick style throughout. The doc comment added later onfetchTitle(explaining the http/https restriction) follows the same style (`ftp://`,[Jsoup.connect],[URLUtil.isURL]) — confirmed it resolves cleanly with zero javadoc warnings. (Pre-existing{@link}usages elsewhere inURLUtil.javaand a plain//comment inNewEntryView.javapredate this PR and are untouched.)User-facing text
Localization.langin Java,%prefix in FXML)!; no trailing:on labels. Tab title "Enter URL" is Title Case, matching every existing sibling tab title ("Enter Identifier", "Interpret Citations", "Specify Bib(La)TeX") — a UI-navigation-label convention, not body copy."...: %0"), not concatenationSecurity
Tests
org.jabref.logicbehavior changes have tests —GenericUrlBasedFetcherTest(7 tests: valid URL, unreachable-URL fallback, whitespace trimming, non-http scheme skips title fetch instead of throwing, invalid/blank input,getName()) and 3 new whitespace cases inURLUtilTest. The dialog-wiring changes live inorg.jabref.gui, outside this rule's stated scope (same situation as prior GUI-layer PRs) — verified manually instead (see Steps to test).assertEquals/assertTrue/assertThrows, plain JUnit, no@DisplayName, don't catch exceptions, no manual temp dirs2. Verification commands
./gradlew :jablib:fetcherTest --tests GenericUrlBasedFetcherTest— 7/7 pass (includes a live network call to the issue's own test URL, plus a regression test for theftp://scheme bug found in review)./gradlew :jablib:test --tests URLUtilTest— 62/62 pass (includes the new whitespace cases)./gradlew :jablib:test --tests LocalizationConsistencyTest— passes with the 10 new keys./gradlew :jablib:checkstyleMain :jablib:checkstyleTest :jabgui:checkstyleMain :jabgui:checkstyleTest— clean./gradlew modernizer— clean./gradlew --no-configuration-cache :rewriteDryRun— "Applying recipes would make no changes" (one pre-existing, unrelated parse warning on a file I didn't touch)./gradlew :jablib:javadoc :jabgui:javadoc— builds successfully; zero warnings on any file this PR touches (100 warnings total, all on pre-existing files elsewhere)npx markdownlint-cli2 "CHANGELOG.md"— 0 issues./gradlew check(full aggregate, all modules) — not run; ran the targeted equivalents above instead given the time cost of the full suite. Happy to run it if a maintainer wants that explicit confirmation.formatcheck (IntelliJ-based formatter, runs via Docker in CI) — initially failed on indentation in the newJsoup.connect(...)method chain, since none of the local commands above cover that specific formatter (Docker daemon wasn't available locally to run it myself,ghcr.io/leventebajczi/intellij-formatfromCHECKLIST.md's own fallback instructions). Fixed to match CI's reported diff exactly and confirmed passing in a subsequent CI run. Flagging this honestly as a real gap in my local verification coverage, not something the commands above would have caught.3. Documentation
CHANGELOG.mdentry added under### Added, linked to Add entry using URL #15411docs/requirements/<area>.md— not added. The new tab's text field reuses the existing documented requirementreq~textinput.clipboard.autofocus~1(docs/requirements/dialog-with-text-input.md), tagged accordingly ([impl->req~textinput.clipboard.autofocus~1]inNewEntryView.initializeEnterUrl()). The more specificreq~newentry.clipboard.autofocus~1(auto-switch-tab-on-clipboard-detection,docs/requirements/focus.md) deliberately does not apply — auto-switching to this tab on clipboard content was explicitly decided against, to keep this PR scoped. No existing doc enumerates "which tabs exist" as a trackable requirement, so there's no natural place to add "Enter URL" as a new entry; open to adding one if a maintainer wants it.docs/— checked; nothing underdocs/references the New Entry dialog's tab set that would go stale.4. Pull request
.github/PULL_REQUEST_TEMPLATE.md, every legitimate section filled[x]/[ ]/[/]gh pr create --body-file <file>TODOplaceholder — not applicable, the real issue link (Add entry using URL #15411) was used from the startChecklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)