Repository navigation
[Refactor] centralize safe log formatter tests, Fixes #429 - #440
Conversation
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
|
495c295 to
2cd741b
Compare
2cd741b to
ae3180a
Compare
c0d9abb to
0085322
Compare
|
Self-review reconciliation: the integration failure with merged #439 is fixed. The pre-fix test reproduced 121 !== NaN; it now checks the observable length without importing the removed constant. Added URL refusal callback coverage for escaped, bounded output and corrected the stale logging comment. Fresh independent five-dimension review: 0 remaining findings. All 12 focused tests, lint, and all 1,252 tests pass on Node 24.18.0. CodeRabbit has no inline findings; review attempts are rate limited. |
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
external-urlduplicated formatter-only coverage throughdescribeRefusedUrl(), even though the sharedsafe-logmodule 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 exercisessafe-logdirectly;external-urlretains URL normalization and refusal-behavior coverage.How to test this
Platforms: any — this is a internal refactor with no visible UI change.
Starting state:
Check out this PR branch (targets
trunk).Install the repository dependencies.
Run
node --test tests/unit/external-url.test.cjs tests/unit/safe-log.test.cjs.Run
npm run lintandnpm test.What must not have happened:
Refused
file:, custom, malformed, and otherwise non-HTTP(S) URLs must still not reachshell.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
Design decisions and alternatives considered
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.Review outcome (required — see AGENTS.md)
0 [fix here] · 0 [follow-up] — no findings across architecture, security, performance, cross-platform, or tests.
npm run lintandnpm testpassed (1,252 tests on Node 24.18.0).Implementation notes
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.