Skip to content

fix: unblock CI (README formatting, note-geometry scroll clamp, test config isolation)#506

Merged
benvinegar merged 3 commits into
mainfrom
fix/ci-unblock
Jul 3, 2026
Merged

fix: unblock CI (README formatting, note-geometry scroll clamp, test config isolation)#506
benvinegar merged 3 commits into
mainfrom
fix/ci-unblock

Conversation

@benvinegar

Copy link
Copy Markdown
Member

What

Three fixes that unblock CI and local test runs:

  1. README formatting — the Homebrew note from Update hunk installation instructions in README #497/Recommend installation from the main homebrew tap #498 was missing a blank line after the install code fence, so oxfmt --check has been failing on main and short-circuiting the Typecheck + Test steps on every PR since 2026-07-02.

  2. Flaky Windows test root-caused and fixedDiffPane keeps bottom scroll stable when offscreen agent notes are windowed out failed intermittently on Windows (chore(deps): bump the github-actions group with 3 updates #501, fix: quit only on q shortcut #505 runs) with Expected: 48, Received: 40. Not a Windows quirk: note rows rendered only for files near the viewport, a set that decays with rapid-scroll state on a 160ms timer, while file overscan and prefetch keep sections mounted beyond it. A mounted section that skipped its notes painted 8 rows shorter than its note-inclusive layout height, transiently shrinking the scrollbox content height so a bottom-edge over-scroll clamped short. Fix: render note rows for exactly the mounted set — the same set the spacer math assumes — and delete the now-dead viewport-proximity set and its base-layout chain. The regression test waits out the overscan decay window, which reproduced the CI failure deterministically on Linux too.

  3. Test isolation — extends the test(pty): isolate integration tests from ambient user config #494 PTY-harness ambient-config isolation to test/session/ and test/smoke/, which still inherited the developer's real ~/.config/hunk/config.toml and failed locally whenever it changed defaults (agent_notes, line_numbers, hunk_headers). CI was unaffected.

Testing

  • bun run typecheck, bun test (full suite), bun run test:integration (51/51), bun run test:tty-smoke (9/9, now green with a customized user config present), bun run format:check, bun run lint.
  • Regression test verified to fail without the DiffPane fix (identical 48 vs 40 failure as Windows CI) and pass with it.

🤖 Generated with Claude Code

benvinegar and others added 3 commits July 3, 2026 03:28
The Homebrew note merged in #497/#498 was missing a blank line after the
install code fence, so format:check has been failing on main and masking
test results on every PR since.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Note rows previously rendered only for files near the viewport, a set that
decays with rapid-scroll state on a timer, while file overscan and prefetch
keep sections mounted beyond it. A mounted section that skipped its notes
painted shorter than its note-inclusive layout height, transiently shrinking
the scrollbox content height so bottom-edge over-scrolls clamped short. This
was the intermittent Windows CI failure in 'DiffPane keeps bottom scroll
stable when offscreen agent notes are windowed out'; the regression test now
waits out the overscan decay window, which reproduced the failure
deterministically on every platform.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extends the #494 PTY-harness isolation to test/session/ and test/smoke/,
which still inherited the developer's real ~/.config/hunk/config.toml and
failed locally whenever it changed defaults like agent_notes or
line_numbers. CI was unaffected (runners have no user config).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR unblocks CI with three targeted fixes: a README blank-line to satisfy the oxfmt --check formatter, a root-cause fix for a flaky Windows scroll-clamp test, and XDG_CONFIG_HOME isolation extended to test/session/ and test/smoke/.

  • DiffPane scroll fix: replaces the timer-decaying viewport-proximity set with the stable mounted-file-indices set as the source of truth for which sections render agent-note rows. Because sectionGeometry and fileSectionLayouts are always measured with the full note set, a mounted section that omitted its notes painted shorter than its layout height, transiently shrinking the scrollbox and clamping bottom-edge scrolls — this aligns the rendered and measured heights.
  • Test config isolation: a new createTestConfigHome() helper creates an empty temp XDG_CONFIG_HOME injected into every spawned process, preventing the developer's ~/.config/hunk/config.toml from leaking into integration and smoke tests.
  • Regression test: the ui-components.test.tsx extension waits out the RAPID_SCROLL_OVERSCAN_IDLE_MS decay window and re-asserts the bottom scroll hold, making the Windows CI failure reproducible deterministically on Linux.

Confidence Score: 4/5

Safe to merge; all three changes address well-understood, reproducible CI failures with no new logic paths that could regress the rest of the diff view.

The DiffPane refactor is sound — selectedFileId is already folded into mountedFileIndices via buildFileRenderWindow, and showAgentNotes propagates correctly through allAgentNotesByFile. The only loose end is that testConfigHome temp dirs are created at module load and never removed; they are empty and harmless, but the pattern differs from the tempDirs cleanup already present in the same files.

The four test files that call createTestConfigHome() at module level (broker-e2e.test.ts, cli.test.ts, daemon.test.ts, tty.test.ts) and the helper itself — each accumulates one unreleased temp dir per run.

Important Files Changed

Filename Overview
src/ui/components/panes/DiffPane.tsx Moves visibleAgentNotesByFile from viewport-proximity to mounted-window basis, eliminating the transient scroll clamp; also removes now-dead baseEstimatedBodyHeights / baseFileSectionLayouts / visibleViewportFileIds chain.
src/ui/components/ui-components.test.tsx Adds regression step to the scroll-clamp test: waits for rapid-scroll overscan to decay, then re-asserts bottom scroll holds, deterministically reproducing the Windows CI failure.
test/helpers/config-home.ts New helper that creates an isolated XDG_CONFIG_HOME for spawned processes; created at module level in each consumer but never cleaned up.
test/session/broker-e2e.test.ts Injects XDG_CONFIG_HOME into spawned hunk and CLI processes to isolate tests from the developer's ambient config.
test/session/cli.test.ts Same config isolation pattern as broker-e2e: XDG_CONFIG_HOME injected into both spawnHunkSession and runSessionCli helpers.
test/session/daemon.test.ts Config isolation applied to daemon spawning path.
test/smoke/tty.test.ts Config isolation applied to both runTtySmoke and runStdinPagerSmoke helpers.
README.md Adds missing blank line after Homebrew install code fence to fix oxfmt --check failure blocking CI.
.changeset/mounted-note-geometry.md Changeset entry describing the scroll-clamp patch for the DiffPane fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[allAgentNotesByFile\nfull note set per file] --> B[sectionGeometry\nmeasured with full notes]
    B --> C[fileSectionLayouts\nlayout heights]
    C --> D[fileRenderWindow\nmountedFileIndices]
    D --> E{windowing\nenabled?}
    E -- yes --> F[mountedFileIndices\nfrom render window\nincludes selectedFileId\n+ overscan + prefetch]
    E -- no --> G[all file indices]
    F --> H[visibleAgentNotesByFile\nnotes for exactly\nmounted sections]
    G --> H
    A --> H
    H --> I[DiffSection render\nnote rows match\nlayout height]
    I --> J[scrollbox content height\nstable — no clamp]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[allAgentNotesByFile\nfull note set per file] --> B[sectionGeometry\nmeasured with full notes]
    B --> C[fileSectionLayouts\nlayout heights]
    C --> D[fileRenderWindow\nmountedFileIndices]
    D --> E{windowing\nenabled?}
    E -- yes --> F[mountedFileIndices\nfrom render window\nincludes selectedFileId\n+ overscan + prefetch]
    E -- no --> G[all file indices]
    F --> H[visibleAgentNotesByFile\nnotes for exactly\nmounted sections]
    G --> H
    A --> H
    H --> I[DiffSection render\nnote rows match\nlayout height]
    I --> J[scrollbox content height\nstable — no clamp]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
test/helpers/config-home.ts:10-12
**Temp dir created at module load but never cleaned up**

`createTestConfigHome()` is called at the module level in every test file and the resulting directory is never removed — it's not tracked in the `tempDirs` arrays that get cleaned up in `afterEach`. Each test run leaves one extra empty directory per file in the OS temp dir. Because each call is at the top level rather than inside a `beforeAll`, there's no natural hook to delete it. An `afterAll(() => rmSync(testConfigHome, { recursive: true, force: true }))` in each consumer would seal the leak.

Reviews (1): Last reviewed commit: "test: isolate session and tty-smoke test..." | Re-trigger Greptile

Comment on lines +10 to +12
export function createTestConfigHome(prefix = "hunk-test-config-") {
return mkdtempSync(join(tmpdir(), prefix));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Temp dir created at module load but never cleaned up

createTestConfigHome() is called at the module level in every test file and the resulting directory is never removed — it's not tracked in the tempDirs arrays that get cleaned up in afterEach. Each test run leaves one extra empty directory per file in the OS temp dir. Because each call is at the top level rather than inside a beforeAll, there's no natural hook to delete it. An afterAll(() => rmSync(testConfigHome, { recursive: true, force: true })) in each consumer would seal the leak.

Prompt To Fix With AI
This is a comment left during a code review.
Path: test/helpers/config-home.ts
Line: 10-12

Comment:
**Temp dir created at module load but never cleaned up**

`createTestConfigHome()` is called at the module level in every test file and the resulting directory is never removed — it's not tracked in the `tempDirs` arrays that get cleaned up in `afterEach`. Each test run leaves one extra empty directory per file in the OS temp dir. Because each call is at the top level rather than inside a `beforeAll`, there's no natural hook to delete it. An `afterAll(() => rmSync(testConfigHome, { recursive: true, force: true }))` in each consumer would seal the leak.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #509. Went with the afterAll-per-consumer shape you suggested — a central sweep was tried first, but Bun's test runner never emits process.on("exit") (verified with a probe), so the helper now exports cleanupTestConfigHomes() and each of the four consumers registers it via afterAll. Verified /tmp stays clean across the session and tty-smoke suites.

Responded by Claude Code using claude-fable-5.

@benvinegar benvinegar merged commit 675104f into main Jul 3, 2026
9 checks passed
@benvinegar benvinegar deleted the fix/ci-unblock branch July 3, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant