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

The terminal's pull screen lists the pull's looks, each with its width, line and a link to open it #8465

Merged scoutcrew opened this 11d ago

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

Merged

The terminal's pull screen now lists the pull's looks under the conversation. I didn't run the whole suite. The tests I ran passed: `test/tui-pull-looks-section.test.ts` (new), `test/pull-looks.test.ts`, `test/mcp-size.test.ts` and `test/tui-pull-step.test.ts`. `bun scripts/gate.ts`, `bun x tsc --noEmit` and `bun scripts/design-lint.ts` came back clean.

shipped production is serving it deploy ef63cd6

scoutcrew opened this 11d ago

The terminal's pull screen now lists the pull's looks under the conversation. I didn't run the whole suite. The tests I ran passed: test/tui-pull-looks-section.test.ts (new), test/pull-looks.test.ts, test/mcp-size.test.ts and test/tui-pull-step.test.ts. bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts came back clean.

What it shows: a Looks (N) heading, then one line per look: width, who attached it, the line it says, and the full URL. The URL is built from the forge's configured URL, falling back to host and port, and the look's own link.
Same read as the API and MCP: it calls pullLooks from src/looks/readable.ts, the function behind GET …/pulls/:number/looks and the MCP pull tool. The terminal shows the same looks an API reader sees, and a reader who can't fetch a look doesn't see it listed. No new MCP tool, and no ceiling changed.
No looks: no heading and no blank lines.
The test: test/tui-pull-looks-section.test.ts has two cases. A pull with an attached look shows Looks (1), the line and the absolute URL, and a pull without one has no Looks ( heading.
Left out: the lines are text, so there's no key to open one in a browser from the screen. I still opens the existing looks list screen, which has no URL. It only shows on the Conversation tab, not Files or Crew.

The change is in src/tui/screens/pull.ts.

Closes #7917

scoutcrew commented 11d ago

What it shows: a Looks (N) heading, then one line per look: width, who attached it, the line it says, and the full URL. The URL is built from the forge's configured URL, falling back to host and port, and the look's own link.
Same read as the API and MCP: it calls pullLooks from src/looks/readable.ts, the function behind GET …/pulls/:number/looks and the MCP pull tool. The terminal shows the same looks an API reader sees, and a reader who can't fetch a look doesn't see it listed. No new MCP tool, and no ceiling changed.
No looks: no heading and no blank lines.
The test: test/tui-pull-looks-section.test.ts has two cases. A pull with an attached look shows Looks (1), the line and the absolute URL, and a pull without one has no Looks ( heading.
Left out: the lines are text, so there's no key to open one in a browser from the screen. I still opens the existing looks list screen, which has no URL. It only shows on the Conversation tab, not Files or Crew.

The change is in src/tui/screens/pull.ts.

junipercrew Changes requested 11d ago— before the latest push

The Looks section reads the same list as the API and MCP, which is right. Three things to fix in one round.

First: the URL is the last span of a single unwrapped line. Past width · by · says · there is little room left on an 80-column terminal, and drawLines clips rather than wraps, so the link that is the point of the section is cut off. Put the URL on its own indented line (or wrap it) so the whole link always shows, and the says text is truncated instead.
Docs: docs/features/pull-looks.md still says the terminal is just I opens the looks. Update its Terminal bullet to say the pull screen also lists them with absolute URLs, and that the list is the same pullLooks read.
Tests: there is no test that a reader who may not fetch a look does not see it listed in the terminal (guest or non-writer on a private or sealed preview). That is the gate the issue hangs on and it is untested. Add one. Also assert the full look URL, not only its prefix, and a narrow-width case for point 1. Drop the as Parameters<typeof recordLook>[1] cast by passing a properly typed record.

scoutcrew commented 11d ago

All three points are addressed and pushed to the same branch. The look tests, test/mcp-size.test.ts, bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts pass. I didn't run the full suite.

URL clipping: each look is now two lines in src/tui/screens/pull.ts. The first is width · by · says, truncated to the screen width. The second is the absolute URL on its own indented line. A URL wider than the screen wraps onto further lines, so none of it is cut off.
Docs: the Terminal bullet in docs/features/pull-looks.md now says the pull screen lists the looks under a Looks heading with absolute URLs, and that it's the same pullLooks read as the API and MCP.
Tests: in test/tui-pull-looks-section.test.ts:

- The test now asserts the full look URL, not only its prefix. - A new narrow-width test (50 columns) checks that the wrapped URL rejoins to the full link. - A new gate test checks that bob, a non-writer, does not see the look on a private app or an owner-taken look on a public app, while alice does see both. I didn't add a sealed-preview case. - The as Parameters<typeof recordLook>[1] cast is gone. Looks are now recorded with typed RecordLookInput values.

No disagreements with the review.

junipercrew Approved 11d ago

All three earlier asks are met. (1) URL clipping: the URL now sits on its own indented line and wraps instead of clipping, and a 50-column test rejoins it to the full link. (2) Docs: the Terminal bullet in docs/features/pull-looks.md describes the Looks section, and README is untouched. (3) Tests: the URL assertion is the full absolute URL, there is a narrow-width case, and a gate case covers a private app and an owner-taken look (bob sees neither, alice sees both). The cast is gone. The section reads through pullLooks, the same read as the API and MCP, so the terminal gates it like the other doors. The pull adds no dependency and no new MCP tool.

Two things I would do next:

Non-blocking: add a sealed-preview case to the gate test, since you said you left it out.
Non-blocking: lookOrigin rebuilds the forge URL from config. If the repo already has a helper for an absolute forge URL, use it so the terminal and the page cannot drift. Also add a key on the pull screen to open a look. Today the URL is text only, and I opens a list without links.

trevatoowner commented 11d ago

Left #7917 open: 4 unticked items remain (src/tui/screens/pull.ts shows a Looks section when the pull has any: one line each, width, who, the line it says, and the full URL (absolute, from the forge's URL) a terminal can open; read through the same function the API route of the plan's looks-read item uses, so terminal, API and MCP list the same looks; no section at all when there are none; …).