Repository navigation
chore(review): review:pre lists concrete findings in the change, each at its file and line - #9651
Conversation
…, shared owners, Effects, Changesets and gap rows, merge conflicts
…dings, read useIsomorphicLayoutEffect
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the new findings work (on #8665's head before its fix, the report flags the two document listeners in selectionLock.ts), but two issues need fixing first.
- Item h waives the size finding whenever the description contains any
Stack:line orBatch:row, at any size, or merely mentions "behavior-module PR" (even "not a behavior-module PR"), and its fix text tells authors to add such a line, so #8222 (about 37,000 changed lines againstmain, with aStack:line) gets no size finding (scripts/lib/pre-review-findings.mjs:62-78,docs/contributing/pull-requests.md:210-212). scripts/pre-review.mjs:374-376copieschangedChangesetsfromscripts/release/changeset-coverage.mjs, andchangedLineCountsadds a second diff parser besidereadDiffthat already counts+++/---hunk lines differently, so export and call the first and extendreadDifffor the second.
Smaller points: the shadow-root rule also matches non-walk uses such as packages/core/src/ContextMenu/ContextMenu.tsx:522, the checklist says "findings below" while the findings print above it, and the description's "latest heads" evidence describes heads from before #8190 and #8665 fixed those lines.
[Automated review]
| const SIZE_DECLARATIONS = [ | ||
| /\bbehavior[- ]module (?:pull request|PR)\b/i, | ||
| /^[ \t]*(?:[-*|][ \t]*)?Batch:[ \t]*\S/m, | ||
| /^[ \t]*(?:[-*|][ \t]*)?Stack:[ \t]*\S/m, | ||
| ]; |
There was a problem hiding this comment.
Any Stack: line or Batch: row, or the phrase "behavior-module PR" anywhere (also "This is not a behavior-module PR."), waives the size finding at any size; #8222 (about 37,000 changed lines against main, with a Stack: line) gets none. Drop the Stack: exemption (tell a stacked branch to pass --base set to the branch below), bound the batch exemption, and match the behavior-module declaration as a declaration.
| const changesets = [...present] | ||
| .filter(file => /^\.changeset\/(?!README\.md$)[^/]+\.md$/.test(file)) | ||
| .map(file => ({file, text: readHead(file) ?? ''})); |
There was a problem hiding this comment.
This re-types changedChangesets from scripts/release/changeset-coverage.mjs:367-377 (same regex, deleted-file filter and read). Export it and call it here, so item k and Changeset coverage always agree on which files are Changesets.
| export function changedLineCounts(diffText) { | ||
| const counts = new Map(); | ||
| let file = null; | ||
| let inHunk = false; | ||
| for (const line of diffText.split('\n')) { | ||
| const header = /^diff --git a\/\S+ b\/(\S+)/.exec(line); | ||
| if (header) { | ||
| file = header[1]; | ||
| inHunk = false; | ||
| counts.set(file, 0); | ||
| } else if (line.startsWith('@@')) { | ||
| inHunk = file != null; | ||
| } else if (inHunk && (line[0] === '+' || line[0] === '-')) { | ||
| counts.set(file, counts.get(file) + 1); | ||
| } | ||
| } | ||
| return counts; | ||
| } |
There was a problem hiding this comment.
A second unified-diff parser beside readDiff, which buildReport already runs on the same diff. They already disagree: readDiff skips hunk lines that start with +++ or --- , so a diff adding ++ added line one counts 2 lines here but yields 1 added line there. Extend readDiff with per-file removed counts and count from it.
| - **h. Size (automatic).** More than 2,000 authored lines, without lockfiles, | ||
| snapshots, or generated files, is a finding unless the description declares a | ||
| behavior-module pull request, a `Batch:` row, or a `Stack:` line. |
There was a problem hiding this comment.
This documents the Stack: and unbounded Batch: waivers; update it together with sizeFindings.
…se readDiff and changedChangesets, narrow the shadow-walk rule
There was a problem hiding this comment.
Requesting changes: the shared-path copies and the Stack: exemption from the last round are fixed, but item h still has two problems.
SIZE_DECLARATIONS(scripts/lib/pre-review-findings.mjs:45-48) still clears the size finding for anyBatch:row at any size and for any mention of "behavior-module PR" that is not literally "not a …" (for example "This isn't a behavior-module PR", or this pull request's own description), while review exempts an owner-directed batch only up to 8,000 authored lines and a behavior-module pull request only when the description declares it and names the module, so cap the batch exemption, match the declaration as a declaration, and update the fix text anddocs/contributing/pull-requests.md:210-212to match.CHECKS.h(scripts/pre-review.mjs:64) still says "or a Stack line", and the report prints it right under a size finding whose fix tells a stacked pull request to pass--base, so drop that phrase.
One smaller point: the parent-walk rule still matches any .nodeType === 11, which packages/core/src/Layer/outsidePress.ts:119 uses on purpose for cross-realm nodes that parentAcrossShadow cannot serve.
[Automated review]
| /(?<!\bnot (?:an? )?)\bbehavior[- ]module (?:pull request|PR)\b/i, | ||
| /^[ \t]*(?:[-*|][ \t]*)?Batch:[ \t]*\S/m, |
There was a problem hiding this comment.
Any Batch: row exempts at any size (Batch: none on 20,000 lines gets no finding), and line 46 treats only a literal "not a/an …" as negated, so "This isn't a behavior-module PR" or a description that only mentions the term is exempt. Cap the batch exemption at 8,000 authored lines and match the behavior-module declaration as a declaration.
| e: 'New public hook: a runnable example in a template block that names the hook in exampleFor or alsoExampleFor', | ||
| f: 'Comments: new or changed source comments explain only non-obvious work, in Simplified Technical English', | ||
| g: 'Records: a changed record needs a spec owner to approve its spec content', | ||
| h: `Size: at most ${SIZE_LIMIT.toLocaleString('en-US')} authored lines, or the description declares a behavior module, a Batch row, or a Stack line`, |
There was a problem hiding this comment.
This still names a Stack line as an exemption, and findingChecks prints it right under the size finding whose fix says to pass --base. Drop "or a Stack line".
…declaration as a declaration, drop the nodeType rule
There was a problem hiding this comment.
Looks ready to approve: the item h exemptions now match review (a Batch: row up to 8,000 authored lines, and a behavior-module exemption only for a line that declares it and names the module), and the size checklist line no longer offers a Stack: line. A few optional follow-ups, none blocking:
- Item k tells a
[fix]Changeset beside a translated catalog edit (for examplepackages/core/locales/fr-FR.json) that nothing the package ships changes, while the same report calls that file published package code (scripts/lib/pre-review-findings.mjs:303). generatedPathspassesgit check-attr --source, which needs Git 2.40, without the fallback the merge check has, sopnpm review:pre --head HEADaborts on older Git (scripts/pre-review.mjs:201).
Two smaller points are inline: item j skips useInsertionEffect, and a note one line above a destructuring line counts as missing.
[Automated review]
| item: 'k', | ||
| file, | ||
| line, | ||
| rule: `\`[${category}]\` names ${pkg}, but nothing ${pkg} ships changes`, |
There was a problem hiding this comment.
shippedFiles reads coverage.required, which leaves out shipped files that need no Changeset, such as a translated catalog (packages/core/locales/fr-FR.json). A [fix] beside that edit is told nothing @astryxdesign/core ships changes, while the same report lists the catalog as published package code. Count every path classifyPath marks as shipping, or say the change needs no Changeset instead of claiming nothing ships. (Non-blocking.)
| '-C', | ||
| ROOT, | ||
| 'check-attr', | ||
| ...(source ? [`--source=${source}`] : []), |
There was a problem hiding this comment.
git check-attr --source needs Git 2.40 or newer, and generatedPaths has no try/catch, so pnpm review:pre --head HEAD aborts on older Git. The merge check already reports "not checked" on an error; doing the same here (count without exclusions and say so) keeps the report usable. (Non-blocking.)
| return findings; | ||
| } | ||
|
|
||
| const EFFECT = /\b(?:React\.)?use(?:Isomorphic)?(?:Layout)?Effect\s*\(/; |
There was a problem hiding this comment.
This misses useInsertionEffect, which INV3 covers like any Effect and which the record's own concern pattern (\buse(?:Layout|Insertion)?Effect\b) includes; for example /\b(?:React\.)?use(?:Isomorphic)?(?:Layout|Insertion)?Effect\s*\(/. (Non-blocking.)
| const text = lines[i].trim(); | ||
| if (text === '' || DIRECTIVE.test(text)) continue; | ||
| if (isNote(text)) return true; | ||
| break; |
There was a problem hiding this comment.
The search stops at the first non-comment line, so a note above a destructuring line is missed: on main, the Effect at packages/core/src/hooks/usePager.ts:1413 has its note at :1410-1411 and is reported as having none. Skipping a few plain declarations, or the guide saying the note must sit directly above, would close the gap. (Non-blocking.)
Maintainer impact
pnpm review:prelists the same checklist items for almost every pull request, so a clean run does not predict a clean review. On the heads of #8665, #8190, #9232, #8676, and #9401 that review last blocked or approved (before #8665 and #8190 fixed the lines below), it raised none of the six blocking review findings. Authors fix a duplicated listener or observer only after a review round names it.Intended invariant
The report stays a lookup: the records decide acceptance. Its existing sections and JSON keys are unchanged. The new checks read the local repository (and the description, when one is passed) and write nothing.
Change
review:prenow lists concrete findings in this change, each at its file and line, with the rule, the fix, and its checklist item (new items h to l):linguist-generatedfiles. Above 2,000 it is a finding. A description line that declares a behavior-module pull request and names the module (Behavior module: <name>, or a line that starts "This behavior-module pull request adds …") removes the limit; aBatch:row raises it to 8,000 (--body-file <path>, or--pr <number>throughgh). AStack:line does not exempt: the finding says to pass--basethe branch below, so the count covers only that pull request's lines.ResizeObserver→observeResize(utils/sharedResizeObserver.ts); a document or windowaddEventListener→subscribeInput(utils/inputStream.ts); a module-level warn-once set →useDevWarning(hooks/useDevWarning.ts) orwarnOnce; a local parent walk across shadow roots (assignedSlot, a shadow root'shost) →parentAcrossShadow(utils/flatTree.ts). A test checks that each owner exists.useIsomorphicLayoutEffect) with no note above it or at the top of its body; an Effect that only copies a value into a ref; a DOM node inuseState.changedChangesetsaschangeset-coverage.mjs); a[fix],[feat], or[perf]Changeset for a package where nothing that ships changes; an evidence gap ("gap until built") left in a record row that this change edits together with the code the record governs, or a gap whose text names a file this change edits.git merge-treeagainst the base, with objects written to a temporary store.readDiffnow also counts each path's added and removed lines, and reads a hunk line that starts with---or+++as content, not as a header.docs/contributing/pull-requests.mdgets one checklist line per new check and the two new flags. No Changeset: repository tooling only.Evidence
selectionLock.ts(fix(gestures): a mouse or pen drag a gesture claims keeps the page's text selection (spec:AST-094 FR41) #8665) and the per-sliderResizeObserverinsliderTouchController.ts(feat(Slider): every pointer drags a thumb by its travel, as UISlider does #8190) are now findings at their lines. feat(Slider): every pointer drags a thumb by its travel, as UISlider does #8190's 6,112 authored lines are counted; itsBatch:row keeps it under the 8,000 batch limit. The three remaining blockers are behavior edge cases that need a reviewer. feat(core): a pan follows a trackpad where its owner allows it, never a mouse wheel; a pinch takes a ctrl-wheel (spec:AST-094 FR43) #8676 (behavior-module declaration) and fix(core): a retarget after a late first frame continues from the screen #9401, both approved, report no findings.scripts/and contributor docs change.Scope
Testing
pnpm vitest run --project node scripts/pre-review.test.mjs scripts/lib/pre-review-findings.test.mjs scripts/lib/review-diff.test.mjs scripts/release/changeset-coverage.test.mjs scripts/specs-for-file.test.mjs: 170 tests pass, with a positive and a negative fixture diff for each check, and a temporary repository for the merge check (conflict, clean, no objects written).node scripts/pre-review.mjs --head HEADon this branch: no findings.