Skip to content

refactor(review): consolidate 3 duplicate helper implementations - #4677

Merged
JSONbored merged 1 commit into
mainfrom
fix/lower-priority-duplicate-cleanup-4611
Jul 10, 2026
Merged

refactor(review): consolidate 3 duplicate helper implementations#4677
JSONbored merged 1 commit into
mainfrom
fix/lower-priority-duplicate-cleanup-4611

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Consolidates the 3 lower-priority duplicate clusters from the review-stack architecture audit's duplication dimension, each independently confirmed against current main before fixing:
    1. isCommentLine (5 bodies in review-enrichment/src/analyzers/): unsafe-any.ts, complexity.ts, and floating-promise.ts carried a byte-identical isCommentLine. Extracted it as a new isBasicCommentLine export in diff-lines.ts (the existing home for shared diff-line predicates, alongside isDiffFileHeaderLine), and pointed those 3 files at the import. hardcoded-url.ts and a11y-regression.ts keep their own isCommentLine — each now composes the shared base plus its deliberate per-language extra (#/<!-- for shell/Python/HTML, <!--/import/from for JSX) instead of re-deriving the whole regex, documented as an override.
    2. authoritativeContributorRepoStats (4 copies): confirmed all 4 still functionally identical (only type-spelling/formatting differed). Exported the one in services/decision-pack.ts and replaced the local copies in api/routes.ts, mcp/server.ts, and queue/processors.ts with an import.
    3. hasUnsafeWildcardCount (3rd private copy): the guarded host/engine pair (src/signals/change-guardrail.tspackages/gittensory-engine/src/signals/change-guardrail.ts) is untouched. packages/gittensory-engine/src/scoring/preview.ts had a 3rd, non-exported copy invisible to the engine-parity drift check by construction — it now imports hasUnsafeWildcardCount from its own package's signals/change-guardrail.ts instead of redefining it.
  • No behavior change in any of the three. Each site's runtime output is identical before/after.

Fixes #4611

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This is a pure relocate-and-import refactor with no new branches or behavior changes, so validation was scoped to what the diff can actually affect rather than the full battery: npm run typecheck (full, clean), npm --workspace @jsonbored/gittensory-engine run build (clean), npm run rees:test (review-enrichment's own build + node:test suite, 1224/1224 passing), npm run test:engine-parity (15/15, confirms the guarded host/engine change-guardrail.ts pair is untouched), npm run test --workspace @jsonbored/gittensory-engine (the engine package's own standalone suite, 349/349), and targeted vitest run against every test file that exercises a changed line (decision-pack.test.ts, api.test.ts, the queue.test.ts contributor-evidence tests, scoring.test.ts) with --coverage scoped to the changed files — decision-pack.ts at 98.96%/96.76% (line/branch) with authoritativeContributorRepoStats fully exercised, preview.ts at 98.73%/99.56% with hasUnsafeWildcardCount fully exercised on both branches, and the processors.ts call site directly confirmed executed via the lcov report. actionlint/test:workers/build:mcp/test:mcp-pack/ui:*/npm audit are not exercised by this diff (no workflow, workers-pool, MCP-package-build, UI, or dependency changes) and are left to CI. Rebased onto fresh main immediately before pushing (main had moved, including an unrelated review-enrichment dedupe PR and a large queue/processors.ts change) and re-ran the full validation pass above against the rebased tree with identical results.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Not applicable: no auth/cookie/CORS/GitHub App/Cloudflare/session changes, no API/OpenAPI/MCP request/response shape changes, and no UI/docs changes in this PR.

UI Evidence

Not applicable — no visible UI, frontend, docs, or extension changes.

Notes

  • Each of the 3 clusters was independently re-verified against current main (not just the issue text) before fixing, since line numbers had drifted (e.g. authoritativeContributorRepoStats in queue/processors.ts had moved by ~340 lines).

Three independent, low-risk duplicate cleanups from the review-stack
architecture audit's duplication dimension:

- isCommentLine: extracted the byte-identical body shared by unsafe-any.ts,
  complexity.ts, and floating-promise.ts into a new isBasicCommentLine
  export in review-enrichment's diff-lines.ts. hardcoded-url.ts and
  a11y-regression.ts keep their own isCommentLine override, now composing
  the shared base plus their deliberate per-language extras (shell/Python
  #, HTML <!--, JSX import/from) instead of re-deriving the whole regex.
- authoritativeContributorRepoStats: exported the single implementation
  from services/decision-pack.ts and pointed api/routes.ts, mcp/server.ts,
  and queue/processors.ts at it instead of each carrying its own copy.
- hasUnsafeWildcardCount: packages/gittensory-engine/src/scoring/preview.ts
  now imports the wildcard-safety check from its own package's
  signals/change-guardrail.ts instead of re-deriving a third hand-
  maintained copy; the guarded host/engine change-guardrail.ts pair is
  untouched.

No behavior change. Validated with typecheck, the engine package's own
build and test suite, the engine-parity contract test, review-enrichment's
node:test suite, and targeted vitest runs (coverage scoped to the changed
files) against the decision-pack, api, mcp, queue, and scoring tests.
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 1cee567 Commit Preview URL

Branch Preview URL
Jul 10 2026, 11:31 AM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 10, 2026
@JSONbored
JSONbored merged commit 776f308 into main Jul 10, 2026
9 checks passed
@JSONbored
JSONbored deleted the fix/lower-priority-duplicate-cleanup-4611 branch July 10, 2026 11:42
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.14%. Comparing base (e984eef) to head (1cee567).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4677   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files         435      435           
  Lines       38566    38550   -16     
  Branches    14060    14054    -6     
=======================================
- Hits        36308    36293   -15     
  Misses       1599     1599           
+ Partials      659      658    -1     
Files with missing lines Coverage Δ
packages/gittensory-engine/src/scoring/preview.ts 100.00% <ø> (ø)
src/api/routes.ts 94.26% <ø> (-0.01%) ⬇️
src/mcp/server.ts 95.58% <ø> (-0.02%) ⬇️
src/queue/processors.ts 95.31% <ø> (+0.02%) ⬆️
src/services/decision-pack.ts 97.30% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lower-priority duplicate cleanup batch

1 participant