Conversation
📝 WalkthroughWalkthrough
ChangesOpaque-path protocol support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/parse.ts`:
- Around line 207-220: The hasAuthority logic on line 218 only checks for actual
host, auth, or file: protocol, but doesn't preserve URLs with explicit empty
authority sections (like `https:///x`). The parsed object from parseurl() must
contain a property indicating whether an authority section was explicitly
present in the original URL. Update the hasAuthority condition to also check for
this property in the parsed object, so that URLs with explicit `//` separators
(even with empty authority) maintain their structure during the round-trip parse
and stringify operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15738bd2-1b74-4851-9d4e-a58f2bbe7e11
📒 Files selected for processing (3)
src/parse.tstest/parse.test.tstest/utilities.test.ts
| // Only emit the `//` authority separator when there is an authority to | ||
| // separate (a host/auth) or for protocol-relative URLs. Opaque-path | ||
| // protocols such as `mailto:`, `tel:` or `data:` must not gain a spurious | ||
| // `//`, otherwise the parse↔stringify round-trip corrupts the URL. | ||
| let proto = ""; | ||
| if (parsed[protocolRelative]) { | ||
| proto = (parsed.protocol || "") + "//"; | ||
| } else if (parsed.protocol) { | ||
| // `file:` URLs canonically keep their (possibly empty) `//` authority, | ||
| // while opaque-path protocols (`mailto:`, `tel:`, `data:`, ...) without a | ||
| // host/auth must not gain a spurious `//`. | ||
| const hasAuthority = host || auth || parsed.protocol === "file:"; | ||
| proto = hasAuthority ? parsed.protocol + "//" : parsed.protocol; | ||
| } |
There was a problem hiding this comment.
Preserve explicit empty authority during re-stringify.
Line 218 infers authority only from host, auth, or file:. For inputs like https:///x, parseurl() keeps protocol: "https:", host: "", pathname: "/x", but stringifyParsedurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Funjs%2Fufo%2Fpull%2FparseURL%28...)) becomes https:/x (one slash lost). That breaks round-trip structure for URLs that explicitly contained // with an empty authority.
💡 Proposed fix
+const hasURLAuthority = Symbol.for("ufo:hasAuthority");
+
export interface ParsedURL {
protocol?: string;
host?: string;
auth?: string;
href?: string;
pathname: string;
hash: string;
search: string;
[protocolRelative]?: boolean;
+ [hasURLAuthority]?: boolean;
} return {
protocol: protocol.toLowerCase(),
auth: auth ? auth.slice(0, Math.max(0, auth.length - 1)) : "",
host,
pathname,
search,
hash,
[protocolRelative]: !protocol,
+ [hasURLAuthority]: true,
};- const hasAuthority = host || auth || parsed.protocol === "file:";
+ const hasAuthority =
+ parsed[hasURLAuthority] || host || auth || parsed.protocol === "file:";
proto = hasAuthority ? parsed.protocol + "//" : parsed.protocol;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/parse.ts` around lines 207 - 220, The hasAuthority logic on line 218 only
checks for actual host, auth, or file: protocol, but doesn't preserve URLs with
explicit empty authority sections (like `https:///x`). The parsed object from
parseurl() must contain a property indicating whether an authority section was
explicitly present in the original URL. Update the hasAuthority condition to
also check for this property in the parsed object, so that URLs with explicit
`//` separators (even with empty authority) maintain their structure during the
round-trip parse and stringify operations.
Problem
parseurl()silently drops all data for protocols that have an opaque path (no//authority), e.g.mailto:,tel:,urn:. Every field comes back empty and the value can't be reconstructed:The same happens for
tel:+123456789,urn:isbn:..., and any other scheme that uses an opaque path rather than a//hostauthority.hasProtocol()already reports these as having a protocol, but the main parser regex requires//, so the match fails and an all-empty result is returned.Fix
This mirrors the existing handling for the hard-coded
data:/blob:/javascript:/vbscript:schemes (added in #158), but generalises it so any protocol without a//authority preserves its opaque path:parseURLgets a fallback branch (after thehasProtocolcheck) that capturesprotocol:+ the remaining opaque path when it is not followed by//. URLs that do use//(http://,file://, protocol-relative//host) are untouched by the negative lookahead and still flow through the existing host parser.stringifyParsedURLonly emits the//authority separator when there is an authority to separate (ahost/auth), for protocol-relative URLs, or forfile:(which canonically keeps its empty//authority). Opaque-path protocols no longer gain a spurious//. This is consistent with maintainer precedent in fix(parseURL): handledata:andblobprotocols #159 / Unexpected behavior when passing adata:URL into parseurl() #158 / Migrate to nativeURL#208.After the fix:
This complements #352 (which fixes
stringifyParsedURLfor the four hard-coded opaque schemes but leaves theparseURLside broken formailto:/tel:/urn:).Notes
localhost:3000-style inputs are unaffected: with no protocol scheme recognised before, they continue to parse as before. The behaviour here matches the WHATWG URL model, where a scheme with an opaque path keeps that path verbatim.file://round-tripping is preserved exactly (its empty//authority is kept).Tests
Added parse + round-trip cases for
mailto:,tel:andurn:intest/parse.test.tsandtest/utilities.test.ts. Full suite, typecheck and lint all pass (497 tests green).Summary by CodeRabbit