Skip to content

Add PTY sidebar jump coverage#142

Merged
benvinegar merged 2 commits intomainfrom
pi/tuistory-sidebar-jump-stream
Mar 31, 2026
Merged

Add PTY sidebar jump coverage#142
benvinegar merged 2 commits intomainfrom
pi/tuistory-sidebar-jump-stream

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • add a tuistory PTY integration test for sidebar file selection
  • verify selecting a later sidebar entry jumps the main pane to that file
  • assert the main pane still renders the full review stream instead of collapsing to a single-file view

Testing

  • bun run test:integration
  • bun run typecheck

This PR description was generated by Pi using OpenAI o4-mini

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR adds a PTY integration test that verifies clicking a sidebar file entry scrolls the main review pane to that file without collapsing the diff stream into a single-file view. It introduces createSidebarJumpRepoFixture (a 5-file git repo fixture) in the harness and wires up the test in the existing describe block, both following the established tuistory test patterns.

Key changes:

  • tuistoryHarness.ts: New createSidebarJumpRepoFixture() builds a 5-file git repo (alpha → epsilon) using the shared createGitRepoFixture helper and exports it from the harness return object.
  • tuistory-hunk.integration.ts: New test "sidebar selection jumps the main pane without collapsing the review stream" launches hunk in split mode at 220×12, clicks M delta.ts in the sidebar, and asserts the viewport scrolls to delta.ts content while epsilon.ts still appears in both the sidebar and the main pane stream (confirming the review stream was not collapsed to a single file).
  • One style concern: the waitForSnapshot predicate bundles all three conditions (including the epsilon.ts >= 2 count), meaning a timeout error is produced instead of a clear expect assertion failure if the epsilon check is the failing condition.

Confidence Score: 5/5

Safe to merge — only test files are changed, no production code is affected, and the new test follows existing harness patterns correctly.

All findings are P2 style suggestions. The fixture data, predicate logic, and assertion coverage are all sound; the sole concern is a minor readability/debuggability improvement to the waitForSnapshot predicate.

No files require special attention.

Important Files Changed

Filename Overview
test/integration/tuistory-hunk.integration.ts Adds one new integration test that clicks a sidebar entry and verifies the main pane jumps to that file while keeping the full review stream intact; follows existing patterns well with a minor predicate-complexity style concern.
test/integration/tuistoryHarness.ts Adds createSidebarJumpRepoFixture — a 5-file git repo fixture using the shared createGitRepoFixture helper — and exports it from the harness; straightforward and consistent with existing fixture factories.

Sequence Diagram

sequenceDiagram
    participant Test as Integration Test
    participant Harness as tuistoryHarness
    participant PTY as Bun PTY (tuistory)
    participant Hunk as hunk process

    Test->>Harness: createSidebarJumpRepoFixture()
    Harness-->>Test: { dir } (5-file git repo)

    Test->>Harness: launchHunk({ args: ["diff","--mode","split"], cwd: dir, cols:220, rows:12 })
    Harness->>PTY: launchTerminal(bun run src/main.tsx diff --mode split)
    PTY->>Hunk: spawn in PTY
    Hunk-->>PTY: renders menu bar + sidebar + main pane

    Test->>PTY: session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/)
    PTY-->>Test: initial snapshot (alpha+beta visible, delta not visible)

    Note over Test: assert alphaOnly=true, betaValue=2 visible
    Note over Test: assert deltaOnly=true NOT visible

    Test->>PTY: session.click(/M delta\.ts\s+\+2 -1/)
    PTY->>Hunk: mouse click event on sidebar delta.ts entry
    Hunk-->>PTY: main pane jumps to delta.ts, stream preserved

    Test->>Harness: waitForSnapshot(session, predicate, 5_000)
    loop poll every ~80ms
        Harness->>PTY: session.text({ immediate: true })
        PTY-->>Harness: current snapshot
        Harness->>Harness: check deltaOnly=true AND NOT alphaOnly=true AND epsilon.ts>=2
    end
    Harness-->>Test: jumped snapshot

    Note over Test: assert deltaValue=2, deltaOnly=true visible
    Note over Test: assert alphaOnly=true NOT visible
    Note over Test: assert epsilon.ts appears >=2 times
Loading

Reviews (1): Last reviewed commit: "Add PTY sidebar jump coverage" | Re-trigger Greptile

Comment on lines +175 to +182
const jumped = await harness.waitForSnapshot(
session,
(text) =>
text.includes("deltaOnly = true") &&
!text.includes("alphaOnly = true") &&
harness.countMatches(text, /epsilon\.ts/g) >= 2,
5_000,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Overly complex predicate makes failure messages less useful

The waitForSnapshot predicate bundles all three conditions together, including the epsilon.ts >= 2 count. When waitForSnapshot times out (e.g., if epsilon.ts never appears in the main pane after the jump), the thrown error says "Timed out after 5000ms" and prints the last snapshot — it does not indicate which of the three conditions was not met. The subsequent explicit assertions on lines 184–187 cover conditions 1 and 3 but are guaranteed to pass once the predicate fires (since waitForSnapshot already verified them), making them informative only in the success case.

A more idiomatic pattern (consistent with the simpler predicates used in the other tests in this file, e.g. the navigation and filter tests) would be to narrow the predicate to the key state-transition signal and leave the supporting assertions to expect:

Suggested change
const jumped = await harness.waitForSnapshot(
session,
(text) =>
text.includes("deltaOnly = true") &&
!text.includes("alphaOnly = true") &&
harness.countMatches(text, /epsilon\.ts/g) >= 2,
5_000,
);
const jumped = await harness.waitForSnapshot(
session,
(text) => text.includes("deltaOnly = true") && !text.includes("alphaOnly = true"),
5_000,
);

Then keep all four expect assertions as-is. This way an epsilon.ts count failure produces a clear expect message ("Expected 1 to be >= 2") rather than a generic timeout with a raw snapshot blob.

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!

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.

Good call — I narrowed the waitForSnapshot predicate to the jump signal and left the epsilon.ts count as a regular assertion so a missing follow-on file produces a clearer failure.

This comment was generated by Pi using OpenAI o4-mini

@benvinegar benvinegar merged commit ca1f9e5 into main Mar 31, 2026
3 checks passed
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