From 52f73eebf9c589fc2e78d6d7859e9ce9b7e845ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 14 Aug 2026 10:17:20 +0100 Subject: [PATCH] fix: show shared clarification context (#2641) (#2645) --- .../task/chat/clarification-input-overlay.tsx | 14 ++ apps/web/e2e/pages/session-page.ts | 7 +- apps/web/e2e/tests/chat/clarification.spec.ts | 46 ++++++ .../tests/chat/mobile-clarification.spec.ts | 50 ++++++ .../clarification-context-rendering/plan.md | 142 ++++++++++++++++++ .../task-01-render-clarification-context.md | 98 ++++++++++++ docs/specs/INDEX.md | 1 + docs/specs/ui/clarification-context.md | 49 ++++++ 8 files changed, 406 insertions(+), 1 deletion(-) create mode 100644 docs/plans/clarification-context-rendering/plan.md create mode 100644 docs/plans/clarification-context-rendering/task-01-render-clarification-context.md create mode 100644 docs/specs/ui/clarification-context.md diff --git a/apps/web/components/task/chat/clarification-input-overlay.tsx b/apps/web/components/task/chat/clarification-input-overlay.tsx index 510e709ad9..cd1df8c386 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.tsx @@ -58,6 +58,11 @@ function sortMessagesByQuestionIndex(messages: Message[]): Message[] { }); } +function readSharedContext(message: Message | undefined): string | null { + const context = (message?.metadata as ClarificationRequestMetadata | undefined)?.context; + return context?.trim() ? context : null; +} + function isQuestionAnsweredAt( messages: readonly Message[], answers: Record, @@ -537,6 +542,7 @@ export function ClarificationInputOverlay({ // messages or shrunk bundles never put us out of range. const total = sortedMessages.length; const activeIndex = total === 0 ? 0 : Math.min(rawActiveIndex, total - 1); + const sharedContext = readSharedContext(sortedMessages[0]); useResolveCallback(group.submitState, onResolved); @@ -588,6 +594,14 @@ export function ClarificationInputOverlay({ onSkip={() => void group.skipAll("User skipped")} /> + {sharedContext && ( +
+ {sharedContext} +
+ )} { await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); await expect(session.clarificationOverlay()).toContainText("Which database"); + await expect(session.clarificationContext()).toHaveCount(0); // Single-question bundles still expose option click → instant resolve. await session.clarificationOption("PostgreSQL").click(); @@ -402,6 +403,51 @@ test.describe("Multi-question clarification carousel", () => { await expect(session.clarificationOverlay()).toContainText("Which database"); }); + test("renders shared context once above the active question", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q shared context", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + const context = session.clarificationContext(); + await expect(context).toHaveCount(1); + await expect(context).toHaveText( + "Picking the foundational stack: answer all three so we can move forward.", + ); + await expect(context).toHaveCSS("margin-top", "12px"); + await expect(context).toHaveCSS("padding", "0px"); + await expect(context).toHaveCSS("border-width", "0px"); + await expect(context).toHaveCSS("background-color", "rgba(0, 0, 0, 0)"); + await expect( + session.clarificationQuestionCards().getByTestId("clarification-context"), + ).toHaveCount(0); + + const [contextBox, questionBox] = await Promise.all([ + context.boundingBox(), + session.clarificationQuestionCards().boundingBox(), + ]); + if (!contextBox || !questionBox) { + throw new Error("expected shared context and question card to have bounding boxes"); + } + expect(contextBox.y + contextBox.height).toBeLessThanOrEqual(questionBox.y + 1); + + await session.clarificationStep(1).click(); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "true"); + await expect(context).toHaveCount(1); + await expect(context).toHaveText( + "Picking the foundational stack: answer all three so we can move forward.", + ); + }); + test("answering option auto-advances to next step and marks step as answered", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/chat/mobile-clarification.spec.ts b/apps/web/e2e/tests/chat/mobile-clarification.spec.ts index 409480f2c7..7af632201d 100644 --- a/apps/web/e2e/tests/chat/mobile-clarification.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-clarification.spec.ts @@ -77,4 +77,54 @@ test.describe("Mobile clarification multiline answer", () => { await expect(session.chat).toContainText("second line"); await expect(session.chat).not.toContainText("linesecond line"); }); + + test("shows shared context once above the question on mobile", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationSession( + testPage, + apiClient, + seedData, + "Mobile Clarify Shared Context", + { scenario: "clarification-multi" }, + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + const context = session.clarificationContext(); + await expect(context).toHaveCount(1); + await expect(context).toHaveText( + "Picking the foundational stack: answer all three so we can move forward.", + ); + await expect(context).toHaveCSS("margin-top", "12px"); + await expect(context).toHaveCSS("padding", "0px"); + await expect(context).toHaveCSS("border-width", "0px"); + await expect(context).toHaveCSS("background-color", "rgba(0, 0, 0, 0)"); + await expect( + session.clarificationQuestionCards().getByTestId("clarification-context"), + ).toHaveCount(0); + + const [contextBox, overlayBox] = await Promise.all([ + context.boundingBox(), + session.clarificationOverlay().boundingBox(), + ]); + if (!contextBox || !overlayBox) { + throw new Error("expected mobile shared context and overlay to have bounding boxes"); + } + expect(contextBox.x).toBeGreaterThanOrEqual(overlayBox.x - 1); + expect(contextBox.x + contextBox.width).toBeLessThanOrEqual( + overlayBox.x + overlayBox.width + 1, + ); + expect( + await testPage.evaluate( + () => document.documentElement.scrollWidth <= document.documentElement.clientWidth, + ), + ).toBe(true); + + await session.clarificationStep(1).tap(); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "true"); + await expect(context).toHaveCount(1); + }); }); diff --git a/docs/plans/clarification-context-rendering/plan.md b/docs/plans/clarification-context-rendering/plan.md new file mode 100644 index 0000000000..e0c486f384 --- /dev/null +++ b/docs/plans/clarification-context-rendering/plan.md @@ -0,0 +1,142 @@ +--- +spec: docs/specs/ui/clarification-context.md +created: 2026-08-13 +status: shipped +--- + +# Implementation Plan: Clarification Context Rendering + +## Overview + +Render the shared `ask_user_question_kandev.context` value that already reaches +every clarification message but is currently ignored by the pending overlay. +The confirmed root cause is confined to +`ClarificationInputOverlay`: the MCP handler, clarification service, persisted +message metadata, WebSocket delivery, and frontend metadata type all preserve +the field, while the component renders only question-level fields. + +The repair derives a trimmed bundle-level context from the first sorted message +and renders it once between the overlay header and question carousel. It does +not change backend contracts, persistence, state, answer submission, or +resolved transcript rendering. + +## Backend + +No backend change is planned. The existing path already forwards context from +`apps/backend/internal/mcp/server/handlers.go` through +`apps/backend/internal/mcp/handlers/handlers.go` and persists it in each +clarification message from `apps/backend/internal/backendapp/adapters.go`. + +## Frontend + +### Pending clarification overlay + +- Update `apps/web/components/task/chat/clarification-input-overlay.tsx` to + read the shared context from the first sorted message. +- Trim only for the empty-value decision. Render non-empty content once at the + bundle level, outside `ClarificationCard`, so carousel navigation cannot + duplicate or remove it. +- Present the context as normal text with top spacing and no bordered, padded, + or filled container. +- Preserve authored line breaks and wrap long words inside the overlay. Add a + stable `clarification-context` test ID. +- Do not translate the value. It is agent-authored domain content, not UI copy. + +### E2E page object + +- Add a `clarificationContext()` locator to + `apps/web/e2e/pages/session-page.ts`, scoped to the active clarification + overlay. + +### Mobile design contract + +This is a content-only addition to the existing clarification overlay. The +nearest shipped phone exemplar is +`apps/web/e2e/tests/chat/mobile-clarification.spec.ts`, which exercises the +same shared overlay on the `mobile-chrome` Pixel 5 project. Composition, +navigation, actions, scroll ownership, safe areas, and touch targets remain +unchanged. The context stays above the carousel, wraps within the current +single scroll owner, and must not create document-level horizontal overflow. + +## Tests + +No new pure function or backend behavior is introduced. A browser test is the +smallest faithful regression because it proves the existing MCP-to-message +transport and the missing React rendering boundary together. + +- **What:** shared context is visible exactly once and remains visible across + multi-question carousel navigation. + **File:** `apps/web/e2e/tests/chat/clarification.spec.ts`. + **How:** use the existing `clarification-multi` mock-agent scenario, which + already sends distinctive context, assert one context region before and + after moving to the next question, and prove it is outside the question card. +- **What:** omitted context does not create an empty context region. + **File:** `apps/web/e2e/tests/chat/clarification.spec.ts`. + **How:** extend the existing single-question `clarification` happy path, + whose mock call omits context, with a zero-count assertion. + +## E2E Tests + +- **Scenario:** **GIVEN** a multi-question MCP clarification with shared + context, **WHEN** the task chat renders the pending bundle, **THEN** context + appears exactly once above the carousel and persists across navigation. + **File:** `apps/web/e2e/tests/chat/clarification.spec.ts`. +- **Scenario:** **GIVEN** the same bundle on a phone viewport, **WHEN** the + operator views the pending clarification, **THEN** context remains visible, + contained, and does not add horizontal page overflow. + **File:** `apps/web/e2e/tests/chat/mobile-clarification.spec.ts`. + +## Verification Results + +- RED desktop: `cd apps/web && pnpm e2e:run tests/chat/clarification.spec.ts -- --grep +"shared context"` discovered 1 test and failed because the new context locator + resolved to 0 elements before the renderer change. The managed runner built the + production backend/web assets and completed teardown. +- RED mobile: `cd apps/web && pnpm e2e:run --project mobile-chrome +tests/chat/mobile-clarification.spec.ts -- --grep "shared context"` discovered 1 + test and failed at the same missing-render assertion. The managed runner built + the production assets and completed teardown. +- GREEN desktop: the exact desktop command passed 1 targeted test against a fresh + production build. +- GREEN mobile: the exact mobile command passed 1 targeted test against a fresh + production build. +- `pnpm run typecheck` — passed. +- `pnpm exec eslint --max-warnings 0 components/task/chat/clarification-input-overlay.tsx +e2e/pages/session-page.ts e2e/tests/chat/clarification.spec.ts +e2e/tests/chat/mobile-clarification.spec.ts` — passed. +- `pnpm run i18n:ratchet` — passed with 0 added and 1 modified file clean. +- `git diff --check` — passed. +- PR asset capture: a disposable production-build E2E captured and visually + inspected the desktop and phone states; both screenshots were non-empty and + compressed with `pngquant-bin` for PR media. +- PR fixup: after the requested 15-minute review window, the active-chat page + object scope and independent task verification commands were corrected in + response to valid minor review comments. The focused desktop and mobile E2E + commands passed again against fresh production builds. +- Visual refinement follow-up: the context now has 12px top spacing and normal + text presentation with no padding, border, or background. The updated desktop + and mobile focused tests passed against fresh production builds. + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [x] [Task 01: Render clarification context](task-01-render-clarification-context.md) - done + +The task is sequential because the E2E assertions, page object locator, and UI +change form one Red-Green-Refactor slice. No subagent is planned or authorized. + +## Risks + +- Every message in a bundle currently carries the same context. Reading the + first sorted message preserves the existing contract and avoids duplication; + malformed bundles with inconsistent values remain outside this repair. +- Long agent-authored content can increase overlay height. The existing chat + overlay owns vertical scrolling, while wrapping and the mobile E2E check + protect narrow layouts from horizontal overflow. + +## Out of Scope + +- Backend, MCP schema, WebSocket, or database changes. +- Rendering context in resolved clarification history. +- Refactoring the clarification carousel or answer state. diff --git a/docs/plans/clarification-context-rendering/task-01-render-clarification-context.md b/docs/plans/clarification-context-rendering/task-01-render-clarification-context.md new file mode 100644 index 0000000000..87ace911e5 --- /dev/null +++ b/docs/plans/clarification-context-rendering/task-01-render-clarification-context.md @@ -0,0 +1,98 @@ +--- +id: "01-render-clarification-context" +title: "Render clarification context" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/clarification-context.md" +--- + +# Task 01: Render clarification context + +## Acceptance + +- A non-empty `ask_user_question_kandev.context` value appears exactly once at + the bundle level and remains visible while navigating between questions. +- The context is normal text with top spacing and no border, padding, or filled + container. +- Missing or whitespace-only context renders no empty region; existing answer, + skip, and carousel behavior remains unchanged. +- Desktop and `mobile-chrome` E2E coverage proves visibility, single rendering, + wrapping containment, and no document-level horizontal overflow. + +## Verification + +Bootstrap once if workspace dependencies are absent: + +```bash +cd apps && pnpm install --frozen-lockfile +``` + +Run RED before changing production code, then rerun both commands after the +GREEN implementation and final refactor: + +```bash +(cd apps/web && pnpm e2e:run tests/chat/clarification.spec.ts -- --grep "shared context") +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/chat/mobile-clarification.spec.ts -- --grep "shared context") +``` + +Confirm each focused command discovers and runs one intended test. The managed +runner must build current production assets and report teardown completion. + +## Files likely touched + +- `apps/web/components/task/chat/clarification-input-overlay.tsx` +- `apps/web/e2e/pages/session-page.ts` +- `apps/web/e2e/tests/chat/clarification.spec.ts` +- `apps/web/e2e/tests/chat/mobile-clarification.spec.ts` + +## Dependencies + +None. + +## Parallelism + +Sequential. The browser regression and UI renderer share one behavior boundary +and must preserve the RED-to-GREEN evidence in a single implementation pass. + +## Inputs + +- Spec `What`, `Scenarios`, and `Out of scope`. +- Plan sections `Pending clarification overlay`, `E2E page object`, and + `Mobile design contract`. +- Existing `clarification-multi` mock-agent scenario, which already supplies + the shared context string. +- Existing task and mobile clarification E2E suites and `SessionPage` locators. + +## Output contract + +Report the RED failure, GREEN results, files changed, exact commands and test +counts, managed-runner cleanup, remaining risks, and synchronized task/plan +status. Do not introduce new UI copy or backend contract changes. + +## Results + +- Added `readSharedContext` and rendered the first sorted message's non-empty + context once between the overlay header and active question card. Original + whitespace is preserved for authored line breaks; `whitespace-pre-wrap` and + `break-words` contain long content. +- Added the `SessionPage.clarificationContext()` locator. +- Desktop coverage proves one rendering, exact text, placement above the card, + exclusion from the question card, persistence across carousel navigation, and + no region for the existing context-omitting single-question scenario. +- Mobile coverage proves one rendering, containment within the overlay, no + document-level horizontal overflow, and persistence after touch navigation. +- RED and GREEN results are recorded in `plan.md`; both managed runner commands + built production assets and completed teardown. +- A disposable production-build capture test produced and visually validated + desktop and phone screenshots for the PR; the capture test was removed after + asset generation. +- PR fixup addressed the valid review comments by scoping clarification + locators to the active chat and making the two task commands independent; + both focused E2E commands passed again against fresh production builds. +- Visual refinement follow-up added 12px top spacing and removed the context + container's padding, border, and background; both focused E2E commands passed + again against fresh production builds. +- `pnpm run typecheck`, changed-file ESLint, `pnpm run i18n:ratchet`, and + `git diff --check` passed. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index c00d1a698c..730664b78e 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -177,6 +177,7 @@ Subscription quota tracking and per-agent cheap-model profile routing. | [comment-markdown](ui/comment-markdown.md) | shipped | | [resizable-markdown-tables](ui/resizable-markdown-tables.md) | building | | [transcript-auto-scroll](ui/transcript-auto-scroll.md) | building | +| [clarification-context](ui/clarification-context.md) | shipped | | [empty-turn-notice](ui/empty-turn-notice.md) | shipped | | [acp-shell-command-output](ui/acp-shell-command-output.md) | shipped | | [acp-model-configuration-summary](ui/acp-model-configuration-summary.md) | shipped | diff --git a/docs/specs/ui/clarification-context.md b/docs/specs/ui/clarification-context.md new file mode 100644 index 0000000000..f5bbec111f --- /dev/null +++ b/docs/specs/ui/clarification-context.md @@ -0,0 +1,49 @@ +--- +status: shipped +created: 2026-08-13 +owner: cfl +--- + +# Clarification Shared Context + +## Why + +Agents can provide shared background with `ask_user_question_kandev.context`, +but operators currently see only the individual question fields. Questions +that depend on that background can therefore appear incomplete or confusing. + +## What + +- A pending clarification bundle with non-empty shared context displays that + context once above the active question card. +- Shared context uses normal text presentation with spacing above it. It has no + bordered, padded, or filled container. +- Shared context remains visible while the operator moves between questions in + a multi-question bundle. +- Task chat, Quick Chat, and other surfaces that use the shared clarification + overlay display the same context behavior on desktop and mobile. +- Omitted or whitespace-only context does not create an empty context region. +- The UI preserves line breaks in the agent-authored context and allows long + text to wrap within the clarification surface. + +## Scenarios + +- **GIVEN** an agent submits a multi-question clarification with non-empty + shared context, **WHEN** the pending clarification appears, **THEN** the + operator sees the context exactly once above the active question card. +- **GIVEN** a visible multi-question clarification with shared context, + **WHEN** the operator navigates to another question, **THEN** the same single + context region remains visible. +- **GIVEN** an agent omits context or sends only whitespace, **WHEN** the + pending clarification appears, **THEN** the UI renders no context region and + preserves the existing question layout. +- **GIVEN** a pending clarification with shared context on a phone viewport, + **WHEN** the operator views the question, **THEN** the context is visible, + wraps inside the overlay, and does not introduce horizontal page overflow. + +## Out of scope + +- Changing the `ask_user_question_kandev` schema or backend message metadata. +- Repeating shared context inside every question card. +- Adding shared context to resolved clarification messages in chat history. +- Changing clarification answer, skip, timeout, or carousel behavior.