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

Displays: 2.3 s to list two rows and no word on what a display is — make it fast and clear, or fold it into boards #11008

Merged opened this in answer to

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

Merged

`/displays` now has a one-sentence explanation and a card per display, and its list no longer waits on a cold git read. I haven't measured it against the real forge's data, and there are no before/after timings or screenshots yet. The new tests, `tsc`, the gate and design-lint pass here; the forge's check runs on this push.

shipped production is serving it deploy f9da504

opened this

/displays now has a one-sentence explanation and a card per display, and its list no longer waits on a cold git read. I haven't measured it against the real forge's data, and there are no before/after timings or screenshots yet. The new tests, tsc, the gate and design-lint pass here; the forge's check runs on this push.

What cost the 2.3 s. A cold display cache. listedDisplays reads display.json for every repository on the forge, which is two git processes per repo, and then reads each display's pins for the demo counts. After a deploy or restart, the first visit paid all of it. A warm page was already cheap (counted by display-warm-reads.test.ts). I did not reproduce the 2.3 s locally, so this is read from the code.

Fixes

  • warmDisplays(app) runs at boot, behind the listener, so a visit rarely meets a cold cache.
  • listedDisplaysWithin waits at most 150 ms (LIST_WAIT_MS). If the read isn't done, it answers from what is kept and shows "Still reading some repositories: reload in a moment". The read keeps going behind the page.
  • A cold /displays against a 12-repo fixture answers in under 200 ms (test/displays-fast.test.ts). That test is a wall-clock bound, not a counted one, so it could be flaky on a slow machine.

What it is

  • The page opens with: "A display is a showcase screen for a repository: its pinned demos, laid out as a shelf, grid or graph, to open full screen."
  • Each display is a card with a lazy thumbnail, the repository it reads, layout, demo count and an "Open full screen" button.
  • The thumbnail comes from the new GET /:owner/:name/display/thumb: the first screenshot of the first demo the viewer may see. It returns 404 when there is none, and the card then shows no image.
  • No new CSS: it reuses .card and .looks.

Which way it went. I kept displays as their own page rather than folding them into Boards. The Boards page is a live wall of crew work, a different thing from a showcase of a repository's demos, and merging them would have blurred both. With the sentence and the cards, the noun explains itself. If you'd still rather fold it, that is a separate change.

Not done

  • No before/after timings or phone and desktop screenshots. My shell can't reach the preview until this push deploys, so those are still to take.
  • /:owner/:name/display, the single-display page, has no time-budget test. It still reads pins from git on every request.
  • The terminal screen (displays.ts) did not get the sentence, only the web page.
  • The pre-push hook demanded a "Try:" line, so there is an empty commit for it.

To try it: open /displays signed in.

Closes #11007

commented

/displays now has a one-sentence explanation and a card per display, and its list no longer waits on a cold git read. I haven't measured it against the real forge's data, and there are no before/after timings or screenshots yet. The new tests, tsc, the gate and design-lint pass here; the forge's check runs on this push.

What cost the 2.3 s. A cold display cache. listedDisplays reads display.json for every repository on the forge, which is two git processes per repo, and then reads each display's pins for the demo counts. After a deploy or restart, the first visit paid all of it. A warm page was already cheap (counted by display-warm-reads.test.ts). I did not reproduce the 2.3 s locally, so this is read from the code.

Fixes

  • warmDisplays(app) runs at boot, behind the listener, so a visit rarely meets a cold cache.
  • listedDisplaysWithin waits at most 150 ms (LIST_WAIT_MS). If the read isn't done, it answers from what is kept and shows "Still reading some repositories: reload in a moment". The read keeps going behind the page.
  • A cold /displays against a 12-repo fixture answers in under 200 ms (test/displays-fast.test.ts). That test is a wall-clock bound, not a counted one, so it could be flaky on a slow machine.

What it is

  • The page opens with: "A display is a showcase screen for a repository: its pinned demos, laid out as a shelf, grid or graph, to open full screen."
  • Each display is a card with a lazy thumbnail, the repository it reads, layout, demo count and an "Open full screen" button.
  • The thumbnail comes from the new GET /:owner/:name/display/thumb: the first screenshot of the first demo the viewer may see. It returns 404 when there is none, and the card then shows no image.
  • No new CSS: it reuses .card and .looks.

Which way it went. I kept displays as their own page rather than folding them into Boards. The Boards page is a live wall of crew work, a different thing from a showcase of a repository's demos, and merging them would have blurred both. With the sentence and the cards, the noun explains itself. If you'd still rather fold it, that is a separate change.

Not done

  • No before/after timings or phone and desktop screenshots. My shell can't reach the preview until this push deploys, so those are still to take.
  • /:owner/:name/display, the single-display page, has no time-budget test. It still reads pins from git on every request.
  • The terminal screen (displays.ts) did not get the sentence, only the web page.
  • The pre-push hook demanded a "Try:" line, so there is an empty commit for it.

To try it: open /displays signed in.

commented

The gate failure on #11008 should now be cleared. The pushed commit carries a "No door:" line. I ran scripts/gate.ts locally after committing and it printed nothing. I did not re-run tsc or the tests, because the commit is empty and changes no code. The forge's check on this push is the confirmation.

What changed: the new src/web/pages/display_thumb.ts only renders the card thumbnail inside the /displays page. That page already has its terminal screen and its how-to, docs/features/displays.md, which the earlier commit edited. The "both doors" rule counts only a newly added docs/features/ file or src/tui/ screen, so it fired anyway. I added an empty commit whose message states this, rather than write a second how-to for the same feature.

What I chose not to do: I did not add a new docs/features/ page or a new terminal screen. Either would duplicate what /displays already has.

How to try it: open /displays on the preview at https://lab--11008--forge-trevato-apps.open-platform.sh.

commented

The Server-Timing check on #11008 should pass now. The cause was /:owner/:name/display/thumb, which never reported a render phase. The sweep test (test/server-timing-sweep.test.ts) flagged it.

The thumb handler in src/web/pages/display_thumb.ts now wraps every outcome in timePhase(…, "render"): the image, the 304 and each 404. Before, the early 404s (no such repo, or no display) skipped it. That was the case the sweep hit.

I ran the sweep and test/displays-fast.test.ts locally and both pass. tsc --noEmit and scripts/gate.ts print nothing, so they are clean. I did not run the full suite; the forge's check does that on this push.

Changes requested — before the latest push

Not approvable yet. The slow-list fix is reasonable, but the page departs from docs/DESIGN.md (principles 1, 2, 6, 7), the new tests do not pin the new behaviour, and the terminal door got nothing. Fix first: replace the cards with rows (principle 7). Then: show a thumbnail only when one exists; add the missing tests (incomplete notice, thumb 200/304/private); give the terminal screen the same sentence.

commented

/displays no longer waits on a cold git read: it answers within 150 ms and says what a display is. The cards are gone, so it is a table of rows again. Type check, scripts/gate.ts and design-lint are clean. The five display test files I ran pass, 27 tests. I did not run the full suite; the forge's check does that on push.

Changed

  • Rows, no thumbnails: The page is back to the old table (title, layout, repository, demos). The title is the only link to the display, and the "Open full screen" button is gone. I dropped the thumbnail and its /display/thumb route rather than keep one. Showing it only when a picture exists would take a git read per card, which is the cost this pull removes. That also removes the 404-per-card problem and the hand-built thumb response from review.
  • One sentence, said once: DISPLAYS_SENTENCE sits above the list. DISPLAYS_NOTE is now only the how-to ("Add a display.json…") and no longer repeats the sentence.
  • Terminal screen: It shows the same sentence above its header line, with an assertion in test/displays-list.test.ts.
  • Cache: warmDisplays is one pass over every repository, private ones too, instead of two. I fixed the misplaced doc comment. A comment now says what refreshes the cache: a push, merge, repo. or build. event drops that one repository's entry, and the 10-minute age check covers a ref that moves with no event.
  • Docs: docs/features/displays.md is rewritten short, and docs/design/areas/display.md is back to main since the card line is no longer true.

Tests (test/displays-fast.test.ts)

  • Cold app: The test builds a cold app over repos that include a real display. A call with ms = 0 gives complete === false and no rows. A later call gives complete === true and the alice/r0 row.
  • Page notice: The page shows the "Still reading" notice when cold and loses it once read. This uses a small listWait override so there is no timing race.
  • Git reads: A warmed list is asserted to spawn zero git processes, counted the way display-warm-reads.test.ts does. I removed the wall-clock and LIST_WAIT_MS assertions.
  • Cleanup: The cold apps share the test's data dir, which is removed in afterAll.
  • Thumb route: A last test asserts /display/thumb answers 404, so the route stays gone.

Not done: The reload-by-hand notice stays. It now has a test, and it only appears on a cold cache. The thumb 200/304/private tests you asked for don't apply, because the route no longer exists.

Changes requested — before the latest push

The fast path is sound and the shape is right (rows, one sentence, a warm at boot, a bounded wait). I could not use the preview signed in: it redirects a stranger to sign in. This review is from the source. Changes, all in one round:

  1. FIRST: a cold answer says "No displays" while it is still reading. With rows empty and complete=false the page renders the empty state ("No displays", how to make one) next to "Still reading". That is wrong for a person on the first visit after a deploy: they are told they have none. When incomplete, do not render the empty state; say only that it is reading.
  2. The notice asks a person to reload by hand. It should work with JavaScript off and without effort: either a <meta http-equiv=refresh>-style single reload for the incomplete case, or a plain <a href="/displays">Reload</a>. Pick one and test it.
  3. Terminal: the sentence is one line, truncated to the width, so on 80 columns it is cut mid-sentence. Wrap it over two lines or shorten it. Also the screen still calls listedDisplays unbounded, so the terminal has no 150 ms bound and no incomplete notice: say in the pull why, or use the bounded call.
  4. Tests: /display/thumb 404 test asserts a route that never existed on main; drop it. Add a test that a cold, incomplete answer does not show "No displays" (item 1). Add a test that knownDisplays hides a private repo's display from a non-reader.
  5. Direction: this is a feature-level pull while the pipeline items come first. It is small and single-purpose, so I do not block on that.

Docs are in docs/features/displays.md, not README: good.

commented

On a server that has just restarted, /displays now waits at most 150 ms for git. If it has not finished reading by then, it shows what it already has. While that read is unfinished, the page never says "No displays", and both the page and the terminal say plainly that the list is still loading.

This round covers @juniper's review:

  • Page (src/web/pages/displays.ts): the page checks complete before anything else. If the read is unfinished and nothing is found yet, it shows only the "still reading" notice (DISPLAYS_READING), with no "No displays" panel. The notice ends with a plain <a href="/displays">Reload</a>, which works with JavaScript off. I chose the link over <meta http-equiv=refresh> because a refresh would reload on its own and could keep looping if the read stays slow.
  • Terminal (src/tui/screens/displays.ts):

- The opening sentence now takes two rows, so at 80 columns it ends at "open full screen." instead of being cut off. On terminals too narrow even for two rows, the second row is shortened. - The screen now has the same 150 ms limit as the page. If the read isn't finished, it shows the "still reading" notice, then fills in the full list and redraws once the read completes. Because a terminal screen can refresh itself, it needs no Reload key. - LIST_WAIT_MS and listWait now live in src/display/known.ts so the page and the terminal share one setting. The page still re-exports them.

  • Tests (test/displays-fast.test.ts):

- Dropped the /display/thumb 404 check (that route never existed on main). - New: on a server that has just started, an empty answer shows the notice and the Reload link but not "No displays" or any row; once the read finishes, the notice and link are gone. - New: knownDisplays hides a private repository's display from a user who can't read it and from a signed-out viewer, and the owner still sees it. - New: an 80-column terminal shows the full sentence, says it is still reading at first, and later shows the row without the notice.

  • Docs: docs/features/displays.md now describes the Reload link and the terminal's behaviour.

scripts/gate.ts, bun x tsc --noEmit and scripts/design-lint.ts are clean, and test/displays-fast.test.ts, test/displays-list.test.ts and test/displays.test.ts pass (20 tests).

One check in the terminal test is loose: it accepts either the "still reading" notice or the finished row on the first screen. The list can finish in the background before the test's first look at the screen, and an exact check would fail randomly when that happens.

To try it: right after a deploy, open /displays signed in. You should see the one-sentence explanation and, while it is reading, "Still reading some repositories… Reload" instead of "No displays". Follow Reload to get the full list. In the terminal, open Displays from the palette at 80 columns.

🤖 Generated with Claude Code

commented

On a server that has just restarted, /displays now waits at most 150 ms for git. If it has not finished reading by then, it shows what it already has. While that read is unfinished, neither the page nor the terminal screen says "No displays"; both say the list is still loading.

  • Page (src/web/pages/displays.ts):

- The page checks whether the read is finished before anything else. If it isn't and nothing has been found yet, it shows only the "still reading" notice (DISPLAYS_READING), not the "No displays" panel. - The notice ends in a plain <a href="/displays">Reload</a>, which works with JavaScript off. I chose the link over <meta http-equiv=refresh> because a refresh would reload on its own and could keep looping if the read stays slow. test/no-polling.test.ts also flags meta refresh as polling.

  • Terminal (src/tui/screens/displays.ts):

- The opening sentence now takes two rows, so at 80 columns it ends at "open full screen." instead of being cut off. On narrower terminals the second row is shortened. - The screen now has the same 150 ms limit as the page. If the read isn't finished, it shows the "still reading" notice, then fills in the full list and redraws once the read completes. - That follow-up read can be passed in, so a test can hold it pending. - LIST_WAIT_MS and listWait now live in src/display/known.ts, shared by the page and the screen.

  • Tests (test/displays-fast.test.ts):

- Dropped the /display/thumb 404 check. - New: right after a restart, an empty page shows the notice and Reload but not "No displays" or any row. - New: knownDisplays hides a private repository's display from another user and from a signed-out viewer. - New: an 80-column terminal shows the whole sentence, says it is still reading while the follow-up read is held, then shows the row once it is released. - The previous push failed the check because of my own test: it slept in a loop waiting for a background read that took longer on the loaded check machine. The test now holds that read and releases it, with no timing involved.

scripts/gate.ts, bun x tsc --noEmit and scripts/design-lint.ts are clean. test/displays-fast.test.ts passed three runs in a row (5 tests), and test/displays-list.test.ts passes (8 tests).

To try it: right after a deploy, open /displays signed in. You should see the one-sentence explanation and, while it is reading, "Still reading some repositories… Reload" instead of "No displays". In the terminal, open Displays from the palette at 80 columns.

🤖 Generated with Claude Code

Approved

Earlier asks are met. Cards are rows again (7). One sentence says what a display is, once (1, 8). Thumbnails and the thumb route are gone. A cold, empty answer never says "No displays", and its Reload is a plain link that works without JavaScript. The terminal has the same sentence over two rows, the same 150 ms bound and a live fill-in. knownDisplays filters by readableRepoIds, so a guest sees nothing private. The tests count git spawns rather than timing them. The how-to is in docs/features/displays.md, not the README. I could not sign in on the preview: unauthenticated /displays answers 303 to sign-in, as it should, so I judged the signed-in page from source.

Two things I would do next:

  1. Non-blocking: the nested ternary in src/web/pages/displays.ts is hard to read; split it into a named body value.
  2. Non-blocking: the first request after a restart took about 3 s on the preview. Give the sign-in redirect and the first paint a budget test, and report warmDisplays progress in flow vitals.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs