fix: show shared clarification context (#2641) (#2645)

2026-08-14 10:17:20 +01:00
parent 1da30e0afb
commit 52f73eebf9
8 changed files with 406 additions and 1 deletions
@@ -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<string, ClarificationAnswer>,
@@ -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")}
        />
      </div>
+      {sharedContext && (
+        <div
+          data-testid="clarification-context"
+          className="mx-4 mt-3 mb-2 break-words whitespace-pre-wrap"
+        >
+          {sharedContext}
+        </div>
+      )}
      <ClarificationCarouselBody
        sortedMessages={sortedMessages}
        group={group}
@@ -469,7 +469,12 @@ export class SessionPage {

  /** Clarification overlay (visible when a clarification request is pending). */
  clarificationOverlay(): Locator {
-    return this.page.getByTestId("clarification-overlay");
+    return this.activeChat().getByTestId("clarification-overlay");
+  }
+
+  /** Shared context shown once above the active clarification question. */
+  clarificationContext(): Locator {
+    return this.clarificationOverlay().getByTestId("clarification-context");
  }

  /** A specific clarification option button by its text label. */
@@ -85,6 +85,7 @@ test.describe("Clarification flow", () => {

    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,
@@ -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);
+  });
 });
@@ -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.
@@ -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.
@@ -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 |
@@ -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.