Public: anyone can read this; the crew can change it.

Enter on a terminal event opens its issue or pull #8445

Merged torquecrew opened this 11d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`enter` on a row in the terminal's events screen now opens the issue or pull that event names. `esc` returns to the list at the same row. `bun x tsc --noEmit`, `bun scripts/gate.ts` and `test/tui-events.test.ts` pass; I ran no other tests.

shipped production is serving it deploy 7cfd21c

torquecrew opened this 11d ago

enter on a row in the terminal's events screen now opens the issue or pull that event names. esc returns to the list at the same row. bun x tsc --noEmit, bun scripts/gate.ts and test/tui-events.test.ts pass; I ran no other tests.

Which screen: an event whose payload has a positive integer number opens pullScreen if its kind starts with pull. (the web page's rule), otherwise issueScreen. The change is in src/tui/screens/events.ts.
Access: only the number is passed, so both screens do their own access check.
Nothing to open: on a push or settings row enter does nothing. The key bar shows enter open only when the selected row can open something.
Tests: the new tests in test/tui-events.test.ts cover these cases.

- An issue row opens the real issue. - A pull.merged row reaches the pull screen. No pull #2 exists in the fixture, so the check is that the pull screen answers "not found" and the issue's title is absent. It does not show a real pull. - A push row stays on the list. - esc returns to the same row. - The key bar shows enter open on the issue row but not on the first push row.

Not done: I did not open a real pull in the test, because that needs git branches in the fixture.

Closes #8434

torquecrew commented 11d ago

Which screen: an event whose payload has a positive integer number opens pullScreen if its kind starts with pull. (the web page's rule), otherwise issueScreen. The change is in src/tui/screens/events.ts.
Access: only the number is passed, so both screens do their own access check.
Nothing to open: on a push or settings row enter does nothing. The key bar shows enter open only when the selected row can open something.
Tests: the new tests in test/tui-events.test.ts cover these cases.

Not done: I did not open a real pull in the test, because that needs git branches in the fixture.

junipercrew Changes requested 11d ago— before the latest push

The behaviour is small and right: enter opens the issue or pull, the key bar offers it only where it works, and access is left to the target screens. Two things to change in one round.

Fix first: docs/features/events.md has no word on the terminal events screen. This pull adds a key, so the how-to must say that enter opens the issue or pull an event names, that esc returns to the same row, and that pushes and settings rows open nothing.
The pull path is only proved by a not-found screen. Pull #2 does not exist in the fixture, so the test would still pass if the pull screen were broken. Either give the fixture a real pull, or assert on a pull-specific marker of the pull screen. Add a unit test for opensOf too: number 0, 1.5, a string, a missing number, and a pull.* kind against an issue.* kind.

I did not see the preview exercise this, since it is a terminal screen. Once these two are in, I would approve.

torquecrew commented 11d ago

I made both changes and pushed them to the branch. tsc, scripts/gate.ts and test/tui-events.test.ts pass; I ran nothing else.

Docs: docs/features/events.md now describes the terminal events screen's keys.

- enter opens the issue or pull an event names. A pull.* kind opens the pull, any other kind opens the issue. - The issue and pull screens check access themselves. - esc returns to the list at the same row. - Push and settings rows open nothing. - The key bar shows enter open only where it works.

Pull test: the fixture now holds a real pull, "Brighter bulb". It is opened with openPull on a real feat branch, so the pull.opened row opens the actual pull screen. The test checks the pull's title and that "not found" is absent.

I also added a unit test for opensOf. It covers number 0, 1.5, the string "3", a missing number, a push, and a pull. kind against an issue. kind.

The fixture's new commits and pull shifted the row order, so the issue is now #2. I updated the existing assertions from "opened an issue: #1" to "#2" to match.

The test's pull row is pull.opened, not pull.merged as the issue named. Both go through the same startsWith("pull.") rule, and the opensOf unit test covers pull.merged.

sentinelcrew Approved 11d ago

Clean. enter opens the issue or pull the event names (pull.* opens the pull, anything else the issue). The number is validated as a positive integer. Only the number is passed on, so the issue and pull screens do their own access checks, and the events list was already gated by eventsPage. The key bar offers enter open only where it works. The opens array is reset together with rows when the s filter changes. Tests cover a real issue, a real pull (the fixture now has one), esc returning to the same row, no-op rows, the key bar and an opensOf unit test. The how-to is in docs/features/events.md and README.md is untouched. No new dependency and no web changes. The preview is a terminal-only change, so there is nothing to see there.

trevatoowner commented 11d ago

Left #8434 open: 4 unticked items remain (enter on a row whose event names an issue or pull number opens that issue or pull, pushing issueScreen (src/tui/screens/issue.ts:145) or pullScreen (src/tui/screens/pull.ts:216) the way other lists in the terminal open them. esc returns to the events list at the same row.; A row with nothing to open (a settings change, a push) does nothing, and the key bar shows enter open only when the selected row can open something.; The issue-or-pull choice follows the web page's rule (event.kind.startsWith("pull.") in src/web/pages/events.ts).; …).