Skip to content

chore(extension): remove dead matchPullRequestTarget from content.js - #8050

Closed
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-extension-dead-match-8023
Closed

chore(extension): remove dead matchPullRequestTarget from content.js#8050
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-extension-dead-match-8023

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Closes matchPullRequestTarget is dead code left behind in loopover-extension/content.js by #7487's issue-page match removal #8023
  • matchPullRequestTarget (apps/loopover-extension/content.js) duplicated matchGitHubPageTarget minus the kind field. fix(extension): drop dead issue-page content-script match #7487 removed issue-page matching (the manifest content-script match is pull/* only) and simplified the if (target) guard, leaving this wrapper stranded — no production call site remains; it was only re-exported through the __loopoverContentInternals test hook.
  • Removed the function and its __loopoverContentInternals entry, and — the part the previous attempt at this issue (fix(extension): remove dead matchPullRequestTarget from content.js #8037) missed and was closed for — updated the root test suite that consumes that hook: test/unit/extension-content.test.ts asserted on internals.matchPullRequestTarget in four places (the issue's "zero test files" note only counted apps/loopover-extension's own missing test/ dir, not this root suite). Those assertions are removed with the symbol; the one case unique to them — a /pull/146/files sub-path still matching — is ported to matchGitHubPageTarget, where the behavior actually lives, so no behavioral coverage is lost.
  • Repo-wide grep matchPullRequestTarget confirms zero remaining references.

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:

  • Deletion-only change (net −11 lines, no new source lines, so no new patch-coverage surface). Ran the exact checks this change can affect: vitest run test/unit/extension-content.test.ts (5 tests green — the suite that consumes the removed test hook), npm run extension:lint and npm run extension:typecheck (the extension checks CI gates validate-code on, which test:ci itself does not run), and the full root npm run typecheck. actionlint/workers/mcp/ui checks are untouched surfaces; CI runs them all.

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.

UI Evidence

Not applicable — dead-code removal with no behavior or visual change (the overlay mounts through matchGitHubPageTarget, which is untouched; the extension has no visible difference to capture).

Notes

  • Why the prior attempt closed, and how this PR resolves it: fix(extension): remove dead matchPullRequestTarget from content.js #8037 removed the symbol but left test/unit/extension-content.test.ts asserting on internals.matchPullRequestTarget, which would throw a TypeError. This PR updates that suite in the same diff and re-anchors the /pull/<n>/files sub-path expectation on matchGitHubPageTarget directly.

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 22, 2026 15:41
@superagent-security

Copy link
Copy Markdown
Contributor

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

…SONbored#8023)

matchPullRequestTarget duplicated matchGitHubPageTarget minus the kind field.
JSONbored#7487 removed issue-page matching and simplified the target guard but left this
wrapper stranded: no production call site remains -- it was only re-exported
through the __loopoverContentInternals test hook.

Unlike the issue's "zero test files" note, the ROOT test suite does consume
that hook: test/unit/extension-content.test.ts asserted on
internals.matchPullRequestTarget in four places (the miss that got the previous
attempt at this issue closed). Those assertions are removed with the symbol;
the one case unique to them -- a JSONbored/pull/146/files sub-path still matching -- is
ported to matchGitHubPageTarget, where the behavior actually lives, so no
behavioral coverage is lost.
@RealDiligent

Copy link
Copy Markdown
Contributor Author

Closing this myself after diagnosing the CI failure — the change is correct (all 5 suite tests green, extension lint/typecheck green, zero remaining matchPullRequestTarget references), but it cannot pass the current scoped-CI pipeline, and neither can any other contributor attempt at #8023:

  • An extension-only diff triggers scoped test selection (vitest --changed=origin/main --coverage.all=false), which selects only test/unit/extension-content.test.ts.
  • That suite loads content.js via readFileSync + node:vm, so it executes nothing under coverage.include (apps/** is not instrumented) — v8 emits an empty coverage/lcov.info.
  • The Verify coverage report exists step then fails all three validate-tests shards with coverage/lcov.info is missing or empty, even though every test passed (see shard 1: Tests 5 passed (5) immediately before the failure).

So #8023 is structurally blocked for contributor PRs until either (a) the empty-lcov guard tolerates a scoped run whose selected tests touch no instrumented source, or (b) extension-only diffs skip the coverage-verify step the way the changes job already gates other harnesses. Happy to reopen/resubmit once either lands — the branch remains available at RealDiligent:fix/critical-issue-extension-dead-match-8023.

@RealDiligent RealDiligent reopened this Jul 22, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 22, 2026
@loopover-orb

loopover-orb Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-22 15:58:23 UTC

2 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This PR removes the dead matchPullRequestTarget wrapper from content.js and correctly updates its sole consumer, the root test suite's __loopoverContentInternals-based test, migrating the one unique assertion (sub-path pull matching) onto matchGitHubPageTarget with the correct kind field added. The diff is well-scoped, traces cleanly to the stated dead-code removal, and the ported test still exercises the real matching behavior rather than fabricating coverage. The grep claim of zero remaining references is consistent with what's shown in the full file content.

Nits — 2 non-blocking

CI checks failing

  • validate
  • validate-tests (2)
  • validate-tests (1)
  • validate-tests (3)
  • validate
  • validate-tests (3)
  • validate-tests (1)
  • validate-tests (2)

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8023
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 378 registered-repo PR(s), 159 merged, 34 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 378 PR(s), 34 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff removes matchPullRequestTarget from content.js and its __loopoverContentInternals export exactly as required, and additionally updates the root test suite that referenced it, preserving the sub-path matching coverage via matchGitHubPageTarget.

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 378 PR(s), 34 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)
after / (scroll)
after / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests (2), validate-tests (1), validate-tests (3), validate, validate-tests (3), validate-tests (1), validate-tests (2))). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

matchPullRequestTarget is dead code left behind in loopover-extension/content.js by #7487's issue-page match removal

1 participant