Review PR

Review PR

Review a change to XYZ/MAPP and report what a careful maintainer would raise. Write the report to the terminal; do not post it to GitHub and do not edit the code under review unless the user asks for fixes afterwards.

Two people use this review. A contributor runs it on their own working changes before pushing, so findings need to be specific enough to act on without further digging. A maintainer runs it while reading someone else's pull request, so the report needs to be readable top to bottom without the repository open alongside it. Write for both: name the file and line for every finding, and say what is wrong rather than only which rule was broken.

Gather The Changes

1. Work out what is under review. A pull request number or URL means gh pr view <n> --repo GEOLYTIX/xyz for the description and linked issues and gh pr diff <n> --repo GEOLYTIX/xyz for the change. Pass --repo explicitly: a fork clone will otherwise resolve to the fork, where the pull request does not exist. No argument means review the working branch, diffed against the branch it targets rather than against main — XYZ maintains major, minor and patch release branches alongside main, and diffing a patch-based branch against main buries the change in unrelated commits. Take the base from gh pr view --json baseRefName when there is a pull request, or from the branch's upstream, and confirm with the user when neither is clear. Add git status for anything uncommitted. Ask which is meant only if the request is genuinely ambiguous — a dirty working tree on a feature branch usually means the uncommitted work is the subject.
2. Read the linked issue in full, not just the pull request description. XYZ issues frequently carry the reproduction, the scope, and the motivation that the pull request summarises in a sentence. Where a linked issue exists it is the yardstick for the scope check below. Plenty of XYZ pull requests reference a Linear ticket such as ENG-295 instead, which you cannot read — when there is no readable linked issue, say so in the report and fall back to the pull request description, and treat the scope check as a judgement rather than a comparison.
3. Read the pull request comments, and search for issues that reference the pull request number. This is where reviewers record the things that never reach the description: a maintainer asking whether the change is in scope, or an issue opened about this change while it was being reviewed. Issue #2955 was filed by a maintainer during the review of #2945 and is the single most useful thing a reviewer of that pull request could read, yet nothing in the pull request itself points to it. gh pr view <n> --comments and a search for the number will both surface this.
4. List every touched file before reading any of them, so the shape of the change is clear before its details are. A change touching one module and its test reads very differently from the same fix scattered over nine.
5. Note which halves of the codebase the change touches, because it decides which checks below apply. apps/mapp/lib/** is the browser library, where the listener and registration check earns its keep. apps/xyz/** is the Node API, where the Vitest coverage check does. A purely server-side change has no listeners to examine, and saying so in one line is the right outcome — not silence, and not a manufactured finding.
6. For a large diff, parallel subagents can help, but split the work by concern rather than by file. One subagent per touched module reads quickly and hides exactly the bugs worth finding: a change that widens a route in router.js and a change to the handler in verify.js are each defensible alone and wrong together. Keep the reading of the diff in the main thread so the report is written with the whole change in view, and have any subagent return findings rather than file contents.

Read Every Touched Method In Full

Read each changed function in its entirety in its post-change state, not just the lines the diff shows. For working changes that is the file on disk. For a pull request you have not checked out, fetch its head first — git fetch upstream pull/<n>/head for an open pull request, or git fetch upstream when it is already merged — and then read with git show FETCH_HEAD:<path> or git show <merge-sha>:<path>. Reading this way rather than checking the branch out leaves whatever the user has in progress untouched, which matters because they may be reviewing someone else's pull request from inside their own unfinished work.

Then find the callers. Searching the bare function name is not enough in MAPP, where modules collect their functions into a methods object and export that as the default, so callers reach a private function through the module name instead — layerStyle.panel(entry), not panel(entry). Search for the exported path as well as the bare name, or you will conclude a function has one caller when it has three.

A diff hunk cannot tell you whether the change is right, only what moved. Reading the whole method and its callers is what surfaces the things reviewers actually miss:

• A guard added at the top of a function that the only caller already performs.
• An early return that now skips cleanup, an event unbind, or a cache write further down.
• A parameter that changed meaning for one caller but not the others.
• A promise that is no longer awaited, or an error path that now returns undefined where callers expect an object.
• A method that was correct for the reported case but is now wrong for a second case it also serves.

When a change touches a route in apps/xyz/router.js, compare the middleware the route carried before against what it carries now. Express routes in XYZ get their request handling from the registration they match, so widening or narrowing a path can silently move a request onto a registration with a different middleware chain — the handler is untouched and still passes its tests, while a parameter it depends on quietly stops being populated. Read both registrations, not just the changed line, and name any middleware the request no longer passes through.

When the change fixes a regression, find the commit that introduced it with git log -L <start>,<end>:<path>. This reframes a review more often than any other single step: it shows whether the change restores the original behaviour or layers a second mechanism on top of the one that broke, which is the difference between a fix and a workaround that leaves the fault in place.

Report on the method as it now stands. Whether the original issue is fixed is necessary but not sufficient — a change that resolves the issue and breaks a sibling path is not ready.

A pre-existing bug you find inside a method the change touches is worth reporting as a [Note], labelled as pre-existing, with a suggestion to open its own issue. It is real, and the reviewer is the person best placed to see it — but holding a pull request for a fault it did not introduce is how good changes stall.

Check Listeners And Repeated Registration

MAPP builds its interfaces by registering callbacks, and those registrations outlive the code that made them. The failure is rarely a crash; it is the same handler firing two or three times because nothing removed the previous one.

For each touched registration, ask whether calling the surrounding function twice leaves one registration or two:

• addEventListener without a matching removeEventListener, an AbortController signal, or { once: true }, on a target that outlives the call — window, document, the mapview, or a layer.
• A push into a persistent array or an assignment into a persistent object, such as layer.showCallbacks, mapview.interactions, or a layer.style entry. These are the easiest to miss because they read as ordinary assignment rather than as subscription.
• OpenLayers interactions, overlays, and sources added to the map without a corresponding removal.
• Anything registered inside a function that runs again on a user action — switching a theme, reloading a layer, reopening a dialog, changing a locale.

Issue #2955 is the shape to look for: a legend method appended to layer.showCallbacks[] each time the theme changed, never removed, so every theme switch added another legend. Nothing errored. The symptom was duplicated behaviour, and only reading the registration alongside its teardown revealed it.

Where a registration is deliberately permanent, say so in the report rather than flagging it — a listener bound once at initialisation on a target that lives for the session is not a leak.