chore: fix open SonarCloud maintainability issues#5
Merged
Conversation
- src/cli.ts: flip savedTo branch to drop negated condition (S7735) - src/sandbox.ts: extract resolveAllowedRoot helper + early-return default path to drop buildAllowedRoots cognitive complexity (S3776) - src/sandbox.ts: extract walkToExtantAncestor + isContainedIn helpers to drop checkPath cognitive complexity (S3776) - src/sandbox.ts: hoist roots list into rootsList const to remove nested template literal in the not-contained error (S4624) Public exports (buildAllowedRoots, checkPath, CheckResult) keep identical signatures and error-message wording. All existing tests pass; tsc build succeeds.
Followup to the Sonar cleanup commit: re-inline two helpers whose extraction was net cosmetic, drop a redundant ternary, and remove two comments that didn't pull weight. Public API and behavior unchanged; Sonar rules still satisfied. Changes: - Inline resolveAllowedRoot back into buildAllowedRoots: the early- return for the default path alone drops S3776 complexity (17 to ~9 by Sonar's counting), so the helper was redundant. - Inline isContainedIn back into checkPath's for-loop: with the redundant 'rel === ""' check gone, the body is two lines — inlining doesn't push checkPath above the 15-point threshold (still ~6 points with walkToExtantAncestor still extracted). - Collapse 'reattached' ternary to join(resolvedAncestor, ...walked.trailing). path.join(x) returns x when no extra segments are passed, so the trailing.length === 0 guard was dead code. - Drop the 'Sequential (not Promise.all)' meta-comment (explains an alternative not taken). - Drop the walkToExtantAncestor header comment (function name + return type already document it). Combined PR diff vs main: +59 / -55 = +4 LOC net.
|
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
1 participant
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.
Summary
vasylenko_markfetch, OPEN+CONFIRMED filter) without changing any public behavior.src/cli.ts: the-osave path branch is flipped fromif (savedTo !== undefined)toif (savedTo === undefined)so the early branch handles the stdout case. Callers see identical output — markdown to stdout when no save path,Saved N bytes to <path>confirmation otherwise. Removestypescript:S7735.src/sandbox.ts—buildAllowedRoots: the default-path case now returns early instead of wrapping the validation loop in anif (raw != null && raw !== "")block. The per-entry validation loop runs at one less nesting level. Public signature and the three error messages (every entry must be an absolute path,not a directory,could not resolve) are byte-for-byte identical, so existing regex assertions intests/sandbox.test.tsstill match. Cognitive complexity drops from 17 to ~9.src/sandbox.ts—checkPath: the while/try/catch ancestor-walk loop is extracted into a privatewalkToExtantAncestorhelper. The main body is now a linear pipeline: walk → realpath → fold → contain-check → format error. Also removes a redundantif (rel === "") return okinside the per-root loop — the subsequent!rel.startsWith("..") && !isAbsolute(rel)already accepts emptyrel. TheCheckResultdiscriminated union and the function signature are unchanged. Cognitive complexity drops from 20 to under the 15 threshold.src/sandbox.ts— the nested template literal in the final error formatter is hoisted into aconst rootsList = roots.map((r) => \'${r}'`).join(", ")above the return. Removestypescript:S4624`.src/sandbox.ts—reattachedis simplified fromtrailing.length === 0 ? x : join(x, ...trailing)tojoin(resolvedAncestor, ...walked.trailing)sincepath.join(x)with no extra segments returnsxunchanged.Why
SonarCloud flags these four issues on every scan against
mainand they block a clean maintainability rating for the project. The refactor is mechanical — branch flips, one extracted helper, one hoisted expression — and preserves both the public API and every existing test assertion.npm testreports 63 passed / 2 skipped (the 2 are the existing win32-only case-fold tests) / 0 failed, andnpm run buildexits 0. LOC delta is +59 / −55 = +4 net, almost all of it thewalkToExtantAncestorhelper signature.SonarCloud filter URL: https://sonarcloud.io/project/issues?impactSoftwareQualities=MAINTAINABILITY&issueStatuses=OPEN%2CCONFIRMED&id=vasylenko_markfetch
The branch is two commits — initial refactor and a followup that trimmed two helper extractions that turned out to be net cosmetic. Squash-merge is fine if a single commit on
mainis preferred.