test: add focused coverage for review metrics#133
Conversation
Greptile SummaryThis PR adds focused regression coverage for three distinct subsystems — Key changes:
Findings:
Confidence Score: 5/5Safe to merge — test-only changes with a minimal, non-breaking export addition in production code. All findings are P2 style suggestions. The single production-code change (adding test/fixtures/diff-helpers.ts — hardcoded Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[test/fixtures/diff-helpers.ts] -->|createDiffFile / createHeaderOnlyDiffFile| B[test/section-heights.test.ts]
A -->|createDiffFile| C[test/loaders-ordering.test.ts]
B -->|import| D[measureDiffSectionMetrics\nsrc/ui/lib/sectionHeights.ts]
C -->|import| E[orderDiffFiles\nsrc/core/loaders.ts]
F[test/hunk-scroll.test.ts] -->|import| G[computeHunkRevealScrollTop\nsrc/ui/lib/hunkScroll.ts]
E -.->|export added| H[src/core/loaders.ts]
Reviews (1): Last reviewed commit: "test: add focused coverage for review me..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| patch: "", | ||
| path, | ||
| previousPath, | ||
| stats: { additions: 1, deletions: 1 }, |
There was a problem hiding this comment.
stats don't reflect actual content
The default before/after pair changes two lines (alpha 1→10 and gamma 3→30), so the real stats are { additions: 2, deletions: 2 }. Hardcoding { additions: 1, deletions: 1 } for all fixture files regardless of content could mislead future test authors who reach for createDiffFile in a test that does assert on stats.
Consider computing stats from the diff metadata, or at least accepting them as an override parameter:
stats: { additions: metadata.hunks.reduce((s, h) => s + h.additionLines, 0), deletions: metadata.hunks.reduce((s, h) => s + h.deletionLines, 0) },
| test("returns a one-row placeholder for files with no visible hunks", () => { | ||
| const file = createDiffFile({ | ||
| after: "const stable = true;\n", | ||
| before: "const stable = true;\n", | ||
| id: "empty", | ||
| path: "empty.ts", |
There was a problem hiding this comment.
expect(file.metadata.hunks).toHaveLength(0) is a setup invariant that documents why the branch under test is taken — it reads more clearly before the measureDiffSectionMetrics call. As written, a reader has to work backwards to understand the test's premise.
| test("returns a one-row placeholder for files with no visible hunks", () => { | |
| const file = createDiffFile({ | |
| after: "const stable = true;\n", | |
| before: "const stable = true;\n", | |
| id: "empty", | |
| path: "empty.ts", | |
| expect(file.metadata.hunks).toHaveLength(0); | |
| const metrics = measureDiffSectionMetrics(file, "split", true, theme); | |
| expect(metrics.bodyHeight).toBe(1); | |
| expect(metrics.hunkBounds.size).toBe(0); | |
| expect(metrics.rowMetrics).toEqual([]); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
measureDiffSectionMetrics()including notes, wrapping, empty files, and header-only hunkscomputeHunkRevealScrollTop()tests for fitting, oversized, clamped, and zero-viewport casespreviousPathTesting
bun run typecheckbun testThis PR description was generated by Pi using OpenAI GPT-5