Repository navigation
[Remove] unused Git and logging exports - #439
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 37 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 (2)
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 |
Collaborator
Author
|
@coderabbitai review |
|
Collaborator
Author
|
@coderabbitai review |
|
juanmaguitar
force-pushed
the
juanmaguitar/cleanup-431
branch
2 times, most recently
from
September 11, 2026 04:00
63d32e4 to
53719df
Compare
juanmaguitar
force-pushed
the
juanmaguitar/cleanup-431
branch
from
September 11, 2026 04:06
53719df to
9ac538c
Compare
juanmaguitar
added a commit
that referenced
this pull request
Sep 11, 2026
## 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>
juanmaguitar
added a commit
to amitraj2203/contributor-toolkit
that referenced
this pull request
Sep 11, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
git-progress.cjsandsafe-log.jsexposed implementation constants that no caller imports. Keeping their public API limited to consumed helpers prevents consumers from depending on internal details without changing behavior.What changes
Removes
PROGRESS_LINE,CONTROL_CHARACTERS, andMAX_DESCRIPTION_LENGTHfrom their respective CommonJS export objects. The constants remain module-private and the existing exported behavior, includingparseProgressLines, is unchanged.How to test this
No user-visible surface changed; this is an API-surface cleanup, so manual app testing is not applicable.
Starting state: A checkout with dependencies installed.
npm run lint; it exits successfully.npm test; all tests pass, includingtests/unit/git-progress.test.cjsand logging refusal behavior exercised by the suite.What must not have happened: Git progress parsing and bounded, one-line refusal logging must continue to behave identically; no production consumer may need the removed internal exports.
Platforms: any — this only narrows JavaScript module exports.
Risks and limitations
Low risk: this is a deliberate breaking change only for unsupported, unconsumed exports. No user-facing behavior changed and no manual UI path applies.
Related
Fixes #431
Design decisions and alternatives considered
Kept the constants in their original modules rather than deleting or inlining them, preserving implementation behavior and readability. Retained
parseProgressLinesbecause its dedicated unit test imports it.Review outcome (required — see AGENTS.md)
0 [fix here] · 0 [follow-up] — none found. The review checked architecture, security, performance, cross-platform behavior, and test coverage against the two-file diff; no invariants are affected.
Implementation notes
Validation:
node --test tests/unit/git-progress.test.cjs,npm run lint, andnpm test(1,258 passing). The worktree has a pre-existing unrelatedpackage-lock.jsonmodification, excluded from this commit and PR.