Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CONTEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ section's line totals, built from intent or from paths alone.
every file keeps its path section. The sidebar says "Grouped by path". _Avoid_: degraded
mode

### Code owners

**Code owner**: A user, team, or email that the base branch's CODEOWNERS names for a
changed file, kept as its token (`@user`, `@org/team`). _Avoid_: owner, when the viewer's
own ownership is meant

**PR reviewer**: An account requested to review a pull request, or whose review GitHub's
merge box lists. A reviewer who only commented isn't one. _Avoid_: reviewer alone, which
names the person reading the diff

### What the reviewer sees

**Sidebar**: gh-prism's sectioned list of changed files, shown in place of the native
Expand Down Expand Up @@ -156,4 +166,6 @@ aren't.
- "Source" names both the background's fetcher of diffs and file contents, and where
intent is read from and cached. Keep it for the fetcher.
- "Pin" also names a diff head's one commit. Say diff pin only for the base and head pair.
- "Reviewer" names the person reading a diff, and in `code-owners.ts` a PR reviewer.
"Viewer" (`ownedByViewer`) is GitHub's word for the person reading the diff.
- The Docs & Comments section's key is `docs`. They are the same section.
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,12 @@ signed-in browser session. It sees only the repositories your account can see.

gh-prism needs access to three hosts and to Chrome's extension storage, and nothing else.

- `https://github.com/*` serves the pull request or commit page, its diff, and file
contents. A content script, which is code that runs inside matching pages, also runs
here, so gh-prism catches GitHub's in-page navigation into a pull request or commit. It
acts only on Files changed and commit pages, and on a pull request's Conversation,
Commits, and Checks tabs, where it reads the diff to split the header's lines-changed
counter by section.
- `https://github.com/*` serves the pull request or commit page, its diff, file contents,
and a pull request's code owners and reviewers. A content script, which is code that
runs inside matching pages, also runs here, so gh-prism catches GitHub's in-page
navigation into a pull request or commit. It acts only on Files changed and commit
pages, and on a pull request's Conversation, Commits, and Checks tabs, where it reads
the diff to split the header's lines-changed counter by section.
- `https://patch-diff.githubusercontent.com/*` is where GitHub redirects diff downloads.
- `https://raw.githubusercontent.com/*` is where GitHub redirects raw-file downloads.
- Extension storage holds the side you put the sidebar on, whether the header's
Expand Down
30 changes: 27 additions & 3 deletions docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ it.*

gh-prism is a Chrome extension built with WXT, a framework for browser extensions. It has
two parts: a service worker that fetches pull request data, and a content script that
reshapes GitHub's Files view. Each section below names the module behind its claims, so
you can read the code next. Terms such as visit, page, intent, and landing are defined in
reshapes GitHub's Files view and reads a pull request's code owners and reviewers itself.
Each section below names the module behind its claims, so you can read the code next.
Terms such as visit, page, intent, and landing are defined in
[`CONTEXT.md`](../CONTEXT.md).

## Overview
Expand Down Expand Up @@ -188,6 +189,29 @@ shape takes a new prefix, so an older entry never validates. The service worker
cache on install, update, and dev reload, because a new build may place files differently,
and the clear also drops the retired `intent.v1` entries.

## Code owners and reviewers

[`utils/github/code-owners.ts`](../utils/github/code-owners.ts) reads who owns each
changed file and who reviews the pull request from the two endpoints GitHub's Files
changed page reads, `/<owner>/<repo>/pull/<n>/page_data/codeowners` and
`/<owner>/<repo>/pull/<n>/page_data/merge_box`. The content script fetches them itself,
same-origin with your session, rather than through the service worker: they need no host
but github.com, and no cache, since a review submitted while the tab is open must show on
the next visit. `index.ts` calls `readCodeOwners` once per visit and hands the promise to
`showVisit` for every page of that visit; a commit page and a pull request's header tabs
read nothing.

The result is each changed file's owners, in CODEOWNERS order, with whether you own the
file, and the `@user` and `@org/team` tokens of the PR reviewers: everyone requested to
review or whose review GitHub's merge box lists, including a team a review was submitted
on behalf of. The merge box leaves out a reviewer who only commented. An owner keeps its
token as written and gets a link only when the token is a valid `@user` or `@org/team`;
gh-prism builds the URL and never takes one from the response. A PR reviewer matches an
owner when their tokens are equal, ignoring case, so a user doesn't match a file owned
only through a team. Either read failing, or either response missing a field it expects,
resolves to `undefined`, never part of the result; a repository without CODEOWNERS
resolves to no owners.

## Section rows

[`entrypoints/content/sidebar/sections-view.ts`](../entrypoints/content/sidebar/sections-view.ts)
Expand Down Expand Up @@ -290,7 +314,7 @@ entrypoints/
header/ diffstat breakout
ui/ shared DOM helpers, loading view, Markdown subset, name reveal, Tailwind tokens
utils/
github/ routes, diff parsing, the PR data port and source
github/ routes, diff parsing, the PR data port and source, code owners and reviewers
intent/ reading, caching, and landing intent
hunk-rules/ the docstring and inline-test rules
sections.ts path rules for the five sections
Expand Down
51 changes: 44 additions & 7 deletions docs/agents/github-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,13 @@ facts about GitHub that held when written, and ends with how to re-check them;
tree link are both `diff-<pathDigest>`.
- **Trust route data only on the visit a full load started.** The embedded script reflects
the last full load only, and only until the first toggle; a soft navigation never
replaces it. gh-prism makes no network requests of its own, so it seeds from route data
only on the visit the full load started, before GitHub's first navigation
(`soft-nav:end` or `popstate`), and only when that data names the visit's pull request;
any visit after a navigation reads headers alone, even the content script's first. A
re-request of the page's path with `accept: application/json` would return the same key
path, fresh (verified from the page's main world only). Read
`user.shouldShowViewedFilesCount` from the same route data as the seed.
replaces it. gh-prism doesn't re-request route data, so it seeds from route data only on
the visit the full load started, before GitHub's first navigation (`soft-nav:end` or
`popstate`), and only when that data names the visit's pull request; any visit after a
navigation reads headers alone, even the content script's first. A re-request of the
page's path with `accept: application/json` would return the same key path, fresh
(verified from the page's main world only). Read `user.shouldShowViewedFilesCount` from
the same route data as the seed.
- **Follow toggles through the header's button, not the tree or `diff_entries`.** Tree
rows carry no Viewed mark, and `diff_entries` batches carry no Viewed field. A header
exists only for a mounted diff, so it gives no coverage, but it tracks every toggle.
Expand Down Expand Up @@ -301,6 +301,43 @@ facts about GitHub that held when written, and ends with how to re-check them;
signed-out data, load the pull request signed out and search its embedded data for a
base SHA.

## Code owners and reviewers

- **Send `X-Requested-With: XMLHttpRequest` to read `page_data/codeowners` and
`page_data/merge_box` as JSON.** Without it, or with `Accept: application/json` alone,
both answer 406 with HTML.
- **`page_data/codeowners` lists every changed path.** `ownershipByPath` maps each path to
`owners`, CODEOWNERS tokens in the rule's order, and `isOwnedByViewer`, true through a
team you belong to. A path no rule matches has `owners: []`. Owners come from the base
branch's CODEOWNERS, not the pull request's. On the code-owners sandbox's 152-file PR #3
it lists all 152 paths, including one behind "Load Diff", with no paging. A repository
without CODEOWNERS answers `isEnabled: false` and an empty `ownershipByPath`. It answers
signed out on a public repository.
- **`page_data/merge_box` needs a signed-in session**, answering 404 signed out even on a
public pull request. `pullRequest.pendingReviewRequests[].reviewer` has `login` and
`type` (`USER` or `TEAM`), and a team's `login` is `org/slug`.
`pullRequest.latestOpinionatedReviews[]` has `author.login`, `onBehalfOf`, and `state`.
- **`latestOpinionatedReviews` leaves out comment-only reviews.** On
[nodejs/node#66895](https://github.com/nodejs/node/pull/66895), two reviewers who only
commented are missing, so gh-prism can't list a reviewer who only commented.
- **`onBehalfOf`'s element shape is unverified.** It was empty on every review checked,
about 460 public pull requests across five repositories; GitHub likely shows it only to
viewers who can see the team. gh-prism reads an element as a team by its `login`, as
`pendingReviewRequests` names one, and skips any element whose `login` isn't `org/slug`.
- **Re-check:** compare

```sh
curl -s -H 'X-Requested-With: XMLHttpRequest' \
https://github.com/excessive-dev/gh-prism-codeowners-sandbox/pull/3/page_data/codeowners \
| jq '.ownershipByPath | length'
```

against
`gh api repos/excessive-dev/gh-prism-codeowners-sandbox/pulls/3 --jq .changed_files`,
and run the same curl without the header for the 406. Signed in with the saved
`storageState`, fetch `merge_box` from a pull request's page and compare
`latestOpinionatedReviews` with `gh api repos/<owner>/<repo>/pulls/<n>/reviews`.

## The narrow layout's overlay

Below 1012px, a pull request's tree toggle opens the file tree as a "Files" overlay.
Expand Down
5 changes: 5 additions & 0 deletions docs/agents/testing-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ To check behavior live, build the extension and drive Playwright's bundled Chrom
CSRF inputs, `csrf_tokens`, `nonce` values, `aliveChannel`, `data-hydro-*` and
`octolytics` attributes, user ids, and JSON fields that identify the viewer. Keep
per-file state such as `diffSummaries[].markedAsViewed`.
- **Cut a `merge_box` capture down to `pullRequest.pendingReviewRequests` and
`pullRequest.latestOpinionatedReviews`, without `avatarUrl` or review `id`.** The rest
carries the viewer's commit emails and signed alive-channel tokens. Where the viewer's
login is data, as in a CODEOWNERS token, replace it with `sandbox-user`. Import a JSON
capture `?raw`, so each test parses its own copy to edit.
- **Wrap the service worker's `fetch` to observe pull request data requests.** The
background script calls the global `fetch` at request time, so wrapping `self.fetch`
through `worker.evaluate` logs each request.
Expand Down
3 changes: 3 additions & 0 deletions entrypoints/content/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { browser } from "wxt/browser";
import type { ContentScriptContext } from "wxt/utils/content-script-context";
import { defineContentScript } from "wxt/utils/define-content-script";
import { storage } from "wxt/utils/storage";
import { readCodeOwners } from "@/utils/github/code-owners";
import { connectPrData } from "@/utils/github/pr-port";
import { readDiffPin, readViewedSeed } from "@/utils/github/pr-ref";
import { storedIntents } from "@/utils/intent/cache";
Expand Down Expand Up @@ -74,6 +75,7 @@ export default defineContentScript({
{
onVisit: (route, visit) => {
const intent = shareIntent((onProgress) => intentFor(route, visit, sources, onProgress));
const codeOwners = readCodeOwners(route.diff, fetch, visit);
// Route data is fresh only until GitHub's first navigation.
const viewed = visitViewed(fullLoad ? readViewedSeed(document, route.diff) : undefined);
fullLoad = false;
Expand All @@ -88,6 +90,7 @@ export default defineContentScript({
intent,
diffstat,
viewed,
codeOwners,
whatsNew: whatsNewForPage?.(),
});
dockSidebar(page, signal, { choice: side, save: saveSide });
Expand Down
14 changes: 11 additions & 3 deletions entrypoints/content/visit/land.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* (`shareIntent`).
*/

import type { PrCodeOwners } from "@/utils/github/code-owners";
import { type LandedSection, landingOf, sectionsOf } from "@/utils/intent/landing";
import type { IntentProgress, IntentResult, PendingIntent } from "@/utils/intent/read";
import { followFileKeys } from "../diffs/file-keys";
Expand Down Expand Up @@ -146,8 +147,9 @@ const orFailed = (read: Promise<IntentResult>): Promise<IntentResult> =>
/**
* Mounts the sidebar, the diff order, the j/k file keys, the diffstat, and the on-screen and Viewed
* trackers that feed the sidebar on one page of a visit, and lands `intent` on them in that order,
* until `signal` aborts. `viewed` is the visit's Viewed state, shared by its pages. `whatsNew`,
* when given, opens the sidebar with its block.
* until `signal` aborts. `viewed` is the visit's Viewed state, shared by its pages. `codeOwners`
* is the visit's code owners and reviewers, which can land after the sidebar draws or not at all.
* `whatsNew`, when given, opens the sidebar with its block.
*/
export function showVisit(
page: FilesPage,
Expand All @@ -157,7 +159,13 @@ export function showVisit(
diffstat,
viewed,
whatsNew,
}: { intent: PendingIntent; diffstat: DiffstatOptions; viewed: VisitViewed; whatsNew?: WhatsNew },
}: {
intent: PendingIntent;
diffstat: DiffstatOptions;
viewed: VisitViewed;
codeOwners?: Promise<PrCodeOwners | undefined>;
whatsNew?: WhatsNew;
},
): void {
const sidebar = showSections(page, signal);
if (whatsNew) sidebar.root.prepend(whatsNewBlock(page.sidebar.ownerDocument, whatsNew));
Expand Down
42 changes: 42 additions & 0 deletions tests/content/index-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { ContentScriptContext } from "wxt/utils/content-script-context";
import contentScript from "../../entrypoints/content";
import { dockSidebar, type Side } from "../../entrypoints/content/sidebar/side";
import { showHeaderTab, showVisit } from "../../entrypoints/content/visit/land";
import { readCodeOwners } from "../../utils/github/code-owners";
import { readFilesRoute } from "../../utils/github/files-route";
import { intentFor } from "../../utils/intent/read";
import { type DiffstatView, whatsNewSeen } from "../../utils/prefs";
Expand All @@ -23,6 +24,10 @@ vi.mock(import("../../utils/intent/read"), async (importOriginal) => ({
...(await importOriginal()),
intentFor: vi.fn(() => new Promise<never>(() => {})),
}));
vi.mock(import("../../utils/github/code-owners"), async (importOriginal) => ({
...(await importOriginal()),
readCodeOwners: vi.fn(() => new Promise<never>(() => {})),
}));
vi.mock(import("../../entrypoints/content/visit/land"), async (importOriginal) => ({
...(await importOriginal()),
showVisit: vi.fn((_page, _signal, { intent }) => void intent(() => {})),
Expand Down Expand Up @@ -195,6 +200,37 @@ describe("content script", () => {
expect(vi.mocked(intentFor).mock.calls[1]?.[0]).toEqual(readFilesRoute("/o/r/pull/2/changes"));
});

it("reads a pull request's code owners once per visit, shared by its pages", async () => {
await run();
const sidebar = byId("pr-file-tree");
sidebar.replaceWith(sidebar.cloneNode(true));
await afterMutations();

expect(readCodeOwners).toHaveBeenCalledOnce();
expect(readCodeOwners).toHaveBeenCalledWith(
readFilesRoute(PR)?.diff,
expect.any(Function),
expect.any(AbortSignal),
);
const read = vi.mocked(readCodeOwners).mock.results[0]?.value;
expect(vi.mocked(showVisit).mock.calls.map(([, , options]) => options.codeOwners)).toEqual([
read,
read,
]);
});

it("reads code owners afresh on the next visit", async () => {
await run();
history.pushState(null, "", "/o/r/pull/2/changes");
document.dispatchEvent(new Event("soft-nav:end"));
await afterMutations();

expect(readCodeOwners).toHaveBeenCalledTimes(2);
expect(vi.mocked(readCodeOwners).mock.calls[1]?.[0]).toEqual(
readFilesRoute("/o/r/pull/2/changes")?.diff,
);
});

it("seeds Viewed state from embedded data on the first visit only", async () => {
history.replaceState(null, "", "/macintacos/gh-prism-sandbox/pull/3/changes");
loadViewedFixture();
Expand Down Expand Up @@ -290,6 +326,12 @@ describe("content script", () => {
);
});

it("reads no code owners", async () => {
await run();

expect(readCodeOwners).not.toHaveBeenCalled();
});

it("shares the Files view's diffstat choice", async () => {
await run();

Expand Down
60 changes: 60 additions & 0 deletions tests/fixtures/codeowners-node.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
{
"isEnabled": true,
"isViewerOneOfMultipleCodeowners": false,
"ownershipByPath": {
"deps/ncrypto/ncrypto.h": {
"isOwnedByViewer": false,
"owners": ["@nodejs/security-wg"],
"ruleLineNumber": 187,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L187"
},
"deps/ncrypto/ncrypto.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/security-wg"],
"ruleLineNumber": 187,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L187"
},
"src/crypto/crypto_util.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"src/crypto/crypto_tls.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"src/crypto/crypto_rsa.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"src/crypto/crypto_hash.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"src/crypto/crypto_context.h": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"src/crypto/crypto_context.cc": {
"isOwnedByViewer": false,
"owners": ["@nodejs/crypto"],
"ruleLineNumber": 63,
"ruleUrl": "/nodejs/node/blob/main/.github/CODEOWNERS#L63"
},
"test/cctest/test_node_crypto.cc": {
"isOwnedByViewer": false,
"owners": [],
"ruleLineNumber": null,
"ruleUrl": null
}
}
}
1 change: 1 addition & 0 deletions tests/fixtures/codeowners-none.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{ "isEnabled": false, "isViewerOneOfMultipleCodeowners": false, "ownershipByPath": {} }
Loading
Loading