Skip to content

test(stories): add a story of the whole chat - #127

Open
pratikbuilds wants to merge 3 commits into
conversation/14-conversation-threadfrom
conversation/15-whole-chat-story
Open

pratikbuilds wants to merge 3 commits into
conversation/14-conversation-threadfrom
conversation/15-whole-chat-story

Conversation

@pratikbuilds

Copy link
Copy Markdown
Collaborator

Compose the new chat family from the shared samples into one story, in both scroll policies: a conversation with agent steps, approvals and generated images, a prompt input with its toolbar and attachments, the conversation map and history window, and a panel with a thread tab. A browser spec drives the story to check that sending settles, approvals keep keyboard focus after they fold, and attachments move into the sent message.

Stacked on #126: review only this commit. Part of #94.

Notes for review

  • e2e/full-chat.spec.ts drives the built Storybook story (the :61000 server the axe sweep already uses) rather than an e2e/fixtures.tsx route: fixtures can't import stories/, and a fixture copy would test a duplicate of the composition. It depends on the story ids.
  • After a keyboard approval decision folds the call, the story moves focus to the run's ActivityTrigger (or the batch's next waiting call); asserted under settle and stick.
  • Left out: the failed-send sample (covered in Chat / Message), MessageActionMenu and PromptInputContext.

Verification

  • This commit passes bun run typecheck and bun run lint on its own (the stack is checked commit by commit).
  • At the top of the stack: bun run check, bun run build, bun run dep-guard, bun run contrast-test and bun run test:e2e (Playwright specs and the Storybook axe sweep in light and dark) pass.

Closes #109

@corbits-triage

corbits-triage Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Triage: Pass

No changes needed from you. A maintainer will review this pull request.

Stacked on #126: only this pull request's own changes were checked.

Notes:

  • added code matches a code pattern: stories/chat/full-chat.stories.tsx:1140 Invented fallback `?? []`; handle the missing value instead., stories/chat/full-chat.stories.tsx:900 Nested ternary; use if statements or a lookup.
  • added code matches a code pattern: stories/chat/full-chat.stories.tsx:252 Invented fallback (an empty string, || [] or || 0); handle the missing value instead.

@TheGreatAxios TheGreatAxios left a comment

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.

Automated multi-agent review: independent review + second-agent validation; only confirmed items listed. The story itself is genuinely strong; the e2e spec under it has one blocking structural error.

  • blocking (verified against Ladle 5.1.1 itself — package contents + routing source) e2e/full-chat.spec.ts:9-11 — the spec drives Storybook URLs (/iframe.html?id=…&viewMode=story&globals=theme:light, waits for body.sb-show-main) at a Ladle server. Ladle ships exactly one HTML file (lib/app/index.html, no iframe.html), ignores id/viewMode/globals params, has no sb-show-main class ever, and its own comment in playwright.config.ts calls it "the Ladle stories build" — so all five tests fail at the first goto, permanently, in CI. Fix: use the sibling spec's real Ladle scheme — page.goto(\${STORIES}/?story=chat-full-chat--${story}&mode=preview&theme=light`)and wait for[data-storyloaded](the story idschat-full-chat--settle/--stickare already in correct Ladle format) — then rerunbun run build && bun run test:e2e`.
  • should-fix (plan gap) stories/chat/full-chat.stories.tsx:473-486 — the Sources family never composes: the story's case "sources" branch is dead code because the only sample carrying a sources part (shortChat) is never mounted, while task 15's objective is "prove every family composes". Give one mounted answer a sources part.
  • minor (process) no e2e/fixtures.tsx entry, while the plan requires one entry per scenario and every sibling spec follows that contract. Defensible here (the composition lives in the story and the axe sweep hits the same server) — but say so in the PR body rather than silently dropping it.
  • minor (flake) e2e/full-chat.spec.ts:133-141 — Stop is clicked on a live label that exists for a single 120 ms replay frame, from inside a page.waitForFunction(..., { polling: "raf" }) side effect; one missed frame = permanent false condition = 30 s timeout despite a clean settle. Poll read-only (expect.poll on the trigger text) and click via locator.
  • nits — onOpen={(image, opener) => …} misnames an index (the prop hands over (index, opener); rename to index, :470); approved tool outputs are hardcoded "Done" (:~308) — vary or derive them; attached files are never openable (viewerItemsOf only maps image kinds and message-tile AttachmentPreview gets no onOpen — story-coverage follow-up); MODELS/MODES/attachment refusal copy is duplicated from prompt-input.stories.tsx/attachments.stories.tsx and already diverged (2 vs 3 models; MAX_FILES vs literal 20) — hoist to samples/.

@pratikbuilds
pratikbuilds force-pushed the conversation/14-conversation-thread branch from b402e73 to 5f8dcec Compare October 27, 2026 13:54
@pratikbuilds
pratikbuilds force-pushed the conversation/15-whole-chat-story branch from 4f86f25 to 163203b Compare October 27, 2026 13:54
@pratikbuilds

pratikbuilds commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Here's what happened to each point.

On the blocking item

This repo runs Storybook 10, not Ladle (see package.json), so the spec's iframe.html?id=… URLs are correct. They pass in CI and locally.

Fixed

1. The Sources component never appeared in the whole-chat story (should-fix)

  • The fix: the story now includes the short sample, whose answer cites two sources, so the accessibility sweep covers them.
  • Test: "an answer's sources show as links under it". It failed before the fix.

2. The Stop test was flaky (minor)

  • The fix: the test now uses Playwright's page clock. It pauses the clock once the story has loaded, steps the replay forward one frame at a time, then clicks Stop normally. There are no more clicks from inside the page.
  • Result: with the CPU slowed 4–10x, the old test failed 14 of 60 runs, and the new one passed 110 of 110.

3. Nits

  • The image argument is now named index.
  • An approved call's output now comes from its name instead of always being "Done".

Not changed, and why

  • No e2e fixture entry: the whole composition lives in the story, and the accessibility sweep runs against that same page.
  • Moving the duplicated models and modes into samples/: that would edit earlier PRs' stories.
  • Making attached files openable in the story: a follow-up.

Also in this PR

A refactor(stories) commit stops the story writing a ref during render.

Commits: test(stories): compose sources in the full chat; steady the Stop test, refactor(stories): stop a closed thread's answer without a render ref

@pratikbuilds
pratikbuilds force-pushed the conversation/14-conversation-thread branch from 5f8dcec to c8ba54f Compare October 27, 2026 14:13
@pratikbuilds
pratikbuilds force-pushed the conversation/15-whole-chat-story branch from 163203b to 92e2515 Compare October 27, 2026 14:13

@TheGreatAxios TheGreatAxios left a comment

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.

Review · Request changes

Adds a story of the whole chat. Stop and unmount do clear the replay timer in the story. The e2e does not prove that.

Findings

  • e2e/full-chat.spec.ts:164 — Stop can pass while the next frame is still scheduled. playTurn emits the running frame and queues the next one 120ms later (play-turn.ts:183). stopAnswer applies stopped() synchronously (full-chat.stories.tsx:1194-1205), so "1 failed", data-status="sent", and the tool's error are already true before page.clock.resume(). Resume unpauses and does not advance that 120ms, and data-status="sent" is also the natural last frame. Dropping stop.current?.() and keeping the state update still goes green if the expects finish inside the step. After Stop, runFor several REPLAY_STEP_MS and assert the tool is still error.

  • e2e/full-chat.spec.ts:191 — The "4 replies" expect does not wait for the streamed answer. send appends the user reply and playTurn emits the empty streaming frame in the same turn (full-chat.stories.tsx:988-996, play-turn.ts:185). The parent count is threadReplies.length, so it becomes 4 before any timer. A playTurn that emits once and returns would still pass. Wait for the reply text before asserting the count.

Compose the new chat family from the shared samples into one story, in
both scroll policies: a conversation with agent steps, approvals and
generated images, a prompt input with its toolbar and attachments, the
conversation map and history window, and a panel with a thread tab. A
browser spec drives the story to check that sending settles, approvals
keep keyboard focus after they fold, and attachments move into the sent
message.
No mounted message carried a sources part, so the Sources family
never rendered in the whole-chat story. Start the recent stretch with
the short-chat sample, whose answer cites two sources, and check them
in the browser spec.

The Stop test had to hit a call that runs for one 120 ms replay frame,
clicking from inside a raf-polled waitForFunction. Under CPU load it
missed the frame, or clicked as the settled frame landed. Hold the
page clock once the story has loaded and step the replay a frame at a
time, so the running frame stays put while Stop is pressed.

Also name the GeneratedImages onOpen index for what it is, and derive
an approved call's output from its name instead of a fixed "Done".
The full-chat ThreadTab kept its stop function in a ref written during
render, so unmounting could call the latest one. Make the function
stable with useCallback instead and return it from the effect.
@pratikbuilds
pratikbuilds force-pushed the conversation/15-whole-chat-story branch from 92e2515 to 7c44aa4 Compare October 28, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants