Warn on schemeless tool URL templates - #159
Conversation
📊 Coverage Report
|
🧪 E2E Test Results✅ 36 passed, 0 failed, 0 skipped
|
… tabs honestly Closes the silent "Opened 0 tabs" path from #157. A template that does not start with http:// or https:// now gets an inline warning under the URL field in ToolEditor, in the same msg/msgWarn style the Hotkeys rows use, with an "add https://" link that prefixes the scheme, trims leading whitespace and leaves the caret after the prefix. In Tray.openTabs, when the main process opened fewer tabs than were offered, the toast reads "Opened N of M tabs; only http and https links can open" instead of a plain success count. Decisions: - The scheme check lives in src/shared/tools.ts as hasWebScheme (case- insensitive, trimmed) so the editor and any future caller agree; the main-process allowlist in open-external.ts is untouched and remains the enforcement point, per the issue. - Saving is still allowed with a schemeless template; the warning informs rather than blocks, matching the Hotkeys reserved-combination pattern. - The tray message is aggregate per click (N of M across all URLs offered), since openExternalUrls only returns a count. Files: src/shared/tools.ts, ToolEditor.tsx, Tray.tsx, plus tests for each.
cb-jeeves
added
the
Jeeves-Review
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Adds a shared hasWebScheme check, an inline warning plus one-click fix in the tool editor, and an honest "Opened N of M tabs" toast. The shape is right and the tests cover the happy paths. One defect in the one-click fix is worth handling before merge. The correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
The one-click fix prepended https:// to whatever the template held, so ftp://host became https://ftp://host and mailto:a@b became https://mailto:a@b, both of which silence the warning while pointing at the wrong host. Strip a leading scheme first. A colon followed by a digit is left alone so a schemeless host:port keeps its port.
|
Responded to review round 1, pushed 0370cf1:
|
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since last round, 0370cf1 makes the one-click fix strip an existing scheme before prefixing https://, with a digit lookahead so a schemeless host:port keeps its port. That resolves the ftp:// and mailto: defect from round 1, and the reply's reasoning for the lookahead holds. The PR is ready apart from one minor placement suggestion below. The correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
The scheme-stripping regex has to agree with hasWebScheme, so it now lives next to it in src/shared/tools.ts as withWebScheme, with a plain unit table there. The editor keeps one test proving the fix button is wired up.
|
Responded to review round 2, pushed 83cb8c9:
|
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 2, 83cb8c9 moves the scheme stripper into src/shared/tools.ts as withWebScheme beside hasWebScheme with its own unit table, exactly as asked; the correctness lens did not run this round, but the helper checks out on every case the tests cover.
The correctness pass did not complete this round.
…url-scheme-warning # Conflicts: # package-lock.json # package.json
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 3 the only change is the merge from main: package-lock.json and the 2.3.0 version bump. The scheme warning, one-click fix and tray toast are unchanged. The correctness lens ran for the first time this PR and found one real gap in code approved last round, so this is not yet ready to merge. Fix that and it is.
The README line added in this PR ("a tool URL must start with http:// or https://") states the same over-broad rule; once the editor exempts a leading {url} token, reword it to say the resolved link must be http or https.
A template that leads with the url token resolves to the captured value,
which the built-in url pattern captures with its http or https scheme and
buildToolUrls substitutes unencoded. The literal-prefix check flagged it
anyway and the one-click fix rewrote it to https://{url}, which resolves to
https://https://... and opens the wrong host. The editor now skips the
warning and the fix button when the template starts with {url}. A pipe
token like {url|domain} keeps the warning since a domain has no scheme.
Reword the README rule to describe the resolved link rather than the
template's literal prefix.
|
Responded to review round 4, pushed 08acb36:
|
…url-scheme-warning # Conflicts: # package-lock.json # package.json
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 4, 08acb36 exempts a template that leads with the url token from the scheme warning and the fix button, with an editor test for {url}, { url }/extra, {url|domain} and x/{url}, and the README line now says the resolved link must be http or https. That closes the round 4 defect. One small gap remains: the exemption spells the token as a regex instead of asking toolTokens, so a url-only pipe token like {url|} still gets the warning and a fix that breaks it. It is minor and the PR is otherwise ready.
The editor exempted a leading url token with its own regex, so {url|} and
{|url}, which toolTokens reads as a url-only token, still warned and the fix
button rewrote them to https://{url|}. needsWebScheme lives beside
hasWebScheme and asks toolTokens for the first token, so the two cannot
drift. The cases move into the shared test table; the editor test keeps one
proving the warning and button stay hidden for {url}.
|
Responded to review round 5, pushed 0744fda:
|
…url-scheme-warning # Conflicts: # package-lock.json # package.json
3 participants
This branch assembles the listed issues for review.
Issues in this consist
Closes #157
What & why
Tool URL templates without an
http:///https://scheme silently opened zero tabs, with no indication to the user why. This adds an inline warning in the tool editor and makes the tray's "opened tabs" toast honest about partial failures.Changes
hasWebSchemetosrc/shared/tools.tsas the single source of truth for the scheme check (case-insensitive, trimmed).ToolEditorshows an inline warning under the URL field when the template lacks a web scheme, with an "add https://" link that prefixes the scheme and preserves cursor position.Tray.openTabsnow reports "Opened N of M tabs; only http and https links can open" when fewer tabs opened than were offered, instead of a misleading plain success count.Review notes
open-external.tsremains the actual enforcement point; the editor warning is informational only and does not block saving.ToolEditor,Tray,tools).main; typecheck and full test suite (87 files, 1065 tests) pass after the merge.