Repository navigation
[Remove] test-only external URL predicate - #436
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change removes the test-only Assessment against linked issues
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This cleanup preserves the existing external URL behavior and is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
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 |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
495c295 to
2cd741b
Compare
2cd741b to
ae3180a
Compare
|
Self-review completed: 0 [fix here], 0 [follow-up] across architecture, security, performance, cross-platform, and tests. Fresh independent judgement pass verified the removed predicate has no consumers and normalization coverage remains. Local lint and all 1,251 tests pass on Node 24.18.0. CodeRabbit has no inline findings; its review attempts are rate limited. Required CI must pass before merge. |
## Why `external-url` duplicated formatter-only coverage through `describeRefusedUrl()`, even though the shared `safe-log` module owns the one-line, bounded rendering contract. ## What changes Removes the forwarding export and calls `describeRefused()` directly at the URL refusal boundary. Formatter coverage now exercises `safe-log` directly; `external-url` retains URL normalization and refusal-behavior coverage. ## How to test this Platforms: any — this is a internal refactor with no visible UI change. **Starting state:** 1. Check out this PR branch (targets `trunk`). 2. Install the repository dependencies. 1. Run `node --test tests/unit/external-url.test.cjs tests/unit/safe-log.test.cjs`. 2. Run `npm run lint` and `npm test`. **What must not have happened:** Refused `file:`, custom, malformed, and otherwise non-HTTP(S) URLs must still not reach `shell.openExternal`; refused values must remain one-line and bounded. The focused tests cover this directly, and the full suite covers the integration boundary. ## Risks and limitations No user-visible behavior changed. Parent PR #436 is merged; this PR now targets trunk; the review found 0 [fix here] and 0 [follow-up] findings. ## Related Fixes #429 --- <details> <summary>Design decisions and alternatives considered</summary> The formatter belongs to `safe-log`, where it is shared by URL, site, and editor refusal paths. Keeping a URL-specific wrapper would preserve a second public surface without adding behavior. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up] — no findings across architecture, security, performance, cross-platform, or tests. `npm run lint` and `npm test` passed (1,252 tests on Node 24.18.0). </details> <details> <summary>Implementation notes</summary> PR #436 already merged the test-only external URL predicate removal. Also fixes the integration with #439: the truncation test asserts the observable 121-character output instead of importing the removed constant. Before the fix it failed with 121 !== NaN; after the fix all 12 focused tests pass. The URL refusal callback remains covered for escaping and truncation, and the stale logging comment now names the shared formatter. Fresh independent self-review: no remaining findings. </details>
Trunk removed isAllowedExternalUrl from external-url.js (WordPress#436) and moved describeRefusedUrl to describeRefused in safe-log.js (WordPress#439, WordPress#440) after this branch was opened. The merge was textually clean, so both symbols resolved to undefined: the first uncancelled navigation would have thrown inside will-navigate, and a failed openExternal would have thrown inside its own catch. window-links.js now asks normalizeExternalUrl, and main.js imports describeRefused from safe-log once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123opUnXoxs1CAN7YQ7q8KU
Why
isAllowedExternalUrl()only restated whethernormalizeExternalUrl()returned a value and existed solely for tests. Keeping that duplicate API made the tests assert an implementation wrapper instead of URL normalization behavior.What changes
Remove the test-only predicate from
src/external-url.js. The former tests now assertnormalizeExternalUrl()'s returned normalized URL or refusal directly, while existing opening coverage remains intact.How to test this
Platforms: any — this is a pure URL-normalization module with no platform-specific branch.
Starting state:
node --test tests/unit/external-url.test.cjs.Expected result: the focused suite passes, including parsed-scheme normalization and refused
file:URLs.What must not have happened: invalid or non-HTTP(S) input must not reach the
openExternalrecorder; normalized HTTP(S) URLs must still be the values handed to it.No visible UI surface changed; the focused test command demonstrates this internal cleanup.
Risks and limitations
The removed export was intentionally test-only; a downstream consumer importing it would now fail, but repository-wide search found none. Self-review: 0 findings across the five required dimensions.
Related
Fixes #428.
Design decisions and alternatives considered
The assertions were folded into direct normalization checks rather than deleted wholesale, retaining coverage of the parser-derived protocol and output that callers actually use.
Review outcome (required — see AGENTS.md)
0 [fix here] · 0 [follow-up] — no findings across architecture, security, performance, cross-platform behavior, or tests.
Implementation notes
Validation:
node --test tests/unit/external-url.test.cjs(11 passing),npm run lint, andnpm test(1,258 passing). The unrelated pre-existingpackage-lock.jsonmodification was left uncommitted.