Repository navigation
pr-review-canvas renderer.js: import filtering and whitespace collapsing happen before line numbering #472
Description
Observed at commit 2eb7ed4: cursor-team-kit/skills/pr-review-canvas/renderer.js.
The renderer drops and rewrites diff lines before line numbers are computed, so the displayed review is neither the diff nor correctly numbered.
1. Import lines are removed before numbering
function isImport(line) { ... s.startsWith('import ') || s.startsWith('import{') || s.startsWith('} from ') }
var filtered = lines.filter(function(l) { ...; return !isImport(l); });Every added, removed or context line that looks like an import is deleted, including side-effect imports such as import './polyfill', and changes to them become invisible. The line counters oL and nL (lines 117-135) are only reset from @@ headers, so every line after a dropped import within the same hunk is numbered too low.
2. Whitespace-only detection is whitespace-insensitive
Lines 15-17 and 107-111: isWhitespaceOnly strips all whitespace (replace(/\s/g, '')) from both sides, and matching delete/add blocks are rewritten as context lines. This also hides meaningful changes: "a b" to "ab", "a b" string literals, and indentation changes in Python/YAML/Makefiles.
Minimal reproduction
Diff hunk:
@@ -1,3 +1,3 @@
import a from 'a'
-import b from 'b'
+import c from 'c'
const x = 1
Both changed import lines are filtered out, and const x = 1 is displayed as line 1 instead of line 3 on both sides.
Another: - return "a b" / + return "ab" is collapsed into an unchanged context line.
Suggested fix
- Parse and number the full raw diff first; apply any filtering at render time so line numbers come from the original hunk headers.
- Make the filters opt-in and visibly marked (for example a collapsed "N import lines hidden" row), never silent.
- Limit whitespace-only collapsing to cases where the changed lines are identical after trimming leading/trailing whitespace only, or drop it for whitespace-sensitive file types.
Activity
deanrie commentedon Oct 3, 2026
Reproduced all three points by running
renderer.jsfrommainin node against your hunks:const x = 1comes out as line 1/1 instead of 3/3 with both import lines gone,"a b"→"ab"renders as an unchanged context line, and a removed side-effect import disappears while the following added line is numbered 2 instead of 4.Since there are three PRs on this, I ran all of them through the same harness (7 diffs: numbering after a dropped import, visibility of import changes,
"a b"→"ab","a b"→"a b", indentation-only, trailing space, a true whitespace-only change):showImports"a b"→"ab"kept as a change"a b"→"a b"kept as a change#482 is the one that addresses both halves of the issue as written (number the raw diff first; filtering visible and never silent; whitespace compared after trimming leading/trailing only), so that's what I've recommended internally. #489 gets the numbering right but still hides what changed in the imports by default. #469 fixes the numbering only — but it also fixes real bugs in the patch-collection step (
src/a-b.ts/src/a_b.tscolliding, multi-page API output) that none of the others touch; @2dubu, that part deserves its own PR regardless of which renderer fix lands.One thing none of the three cover: indentation-only edits in Python/YAML/Makefiles still collapse. Worth disabling collapsing for whitespace-sensitive files as a follow-up.
2dubu commentedon Oct 3, 2026
@deanrie Thanks for the detailed comparison! Would it make sense to narrow #469 to the patch collection fixes and leave the renderer changes to the other PRs? That would cover preserving exact filenames as keys and combining all API pages into a single JSON map. I can remove the overlapping renderer changes and keep the regression tests for patch collection.
deanrie commentedon Oct 4, 2026
Yes, please do — that's the split I was hoping someone would make.
I checked overlap just now: your
SKILL.mdchanges (the--paginate --slurp | jq '.[][] …'bit, real filenames as keys, thedata-diffexample) live around lines 64 / 104–110 / 139–145, and #482 only touches therenderDifftable row and the "Diff features" list further down. So a narrowed #469 should sit next to either renderer PR without anyone rebasing.And honestly the collection fix deserves its own PR anyway. Right now
--jqruns per page, so any PR with more than one page of files ends up with several top-level objects in one "JSON" file, and thegsub("[^a-zA-Z0-9]"; "_")thing happily mergesa-b.tsanda_b.tsinto one key. Both of those are real, not cosmetic.On tests: keep
buildPatches— I like that it pulls thejqquery straight out of SKILL.md, so the docs can't quietly drift. The renderer cases I'd drop from this PR; they'll fail onmainuntil #482 lands. Ping me when the narrowed version is up and I'll re-run it and note the order (#482 first, then #469) for whoever merges.2dubu commentedon Oct 4, 2026
@deanrie Thanks for checking the overlap! I'll narrow #469 to the patch collection fixes and keep the related regression tests. Just so I understand the context, are you a maintainer of this repo, or are you helping coordinate with the maintainers?
deanrie commentedon Oct 4, 2026
I work at Cursor, yes, not a maintainer of this repo though. I'm helping triage: I reproduce, review the PRs and pass them to the plugin owners with a recommendation, so things don't sit unread. The merge itself is theirs. Thanks for splitting #469, that makes their call easy.
2dubu commentedon Oct 4, 2026
@deanrie Thanks for clarifying! I've narrowed #469 to the patch collection fixes and removed the overlapping renderer changes.
All three regression tests pass, and
buildPatchesstill reads the query directly fromSKILL.md. I also adjusted the test input to preserve page boundaries so the pagination test catches the original bug.Could you rerun your checks on the updated version when you get a chance?
deanrie commentedon Oct 4, 2026
Re-ran it, all good.
Narrowed #469 touches only
SKILL.md(+6/-6) and the new test file,renderer.jsis untouched, and it's still mergeable against today's main. Your 3 tests pass on the branch. I also pointed the same test file at the oldSKILL.mdcommand and all 3 fail the way they should: the key test ends up with a singlesrc_a_b_ts, the pagination test dies withUnexpected non-whitespace character after JSON(several top-level objects in one file), and auto-discovery can't find the exact filename. So the tests really do pin the original bugs.One more check outside the test harness, on a real PR with pagination forced (
per_page=4on an 11-file PR): the old command writes 3 separate objects with 4/4/3 keys, yours writes one object with 11 keys. Exactly the fix.From my side this is ready. I've noted the order for the owners: #482 for the renderer, then #469 for collection. Thanks for turning it around so fast.