Skip to content

Fix on-demand chat comment surfaces - #1907

Merged
raphaeltm merged 9 commits into
mainfrom
sam/chat-ui-desktop-broken-x7a6h9
Aug 25, 2026
Merged

Fix on-demand chat comment surfaces#1907
raphaeltm merged 9 commits into
mainfrom
sam/chat-ui-desktop-broken-x7a6h9

Conversation

@simple-agent-manager

Copy link
Copy Markdown
Contributor

Summary

  • Restore desktop message comments as an on-demand in-layout rail instead of an always-visible rail or modal side drawer.
  • Wire every desktop comment entry point into that rail: header/dropdown Comments, message-level Comment, selected-text Comment, and existing comment count markers.
  • Move mobile selected-text draft composition into a fixed bottom composer over the normal chat input while preserving mobile whole-message inline comments.
  • Split FloatingHeader out of the main chat component and add a retained UI visual-testing rule for responsive/on-demand surfaces.

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • Additional validation run (if applicable)
    • pnpm build
    • pnpm --filter @simple-agent-manager/web test -- tests/unit/components/project-message-view.test.tsx — 66 tests passed
    • pnpm --filter @simple-agent-manager/web exec playwright test tests/playwright/message-comments-audit.spec.ts --project "Desktop (1280x800)" --project "iPhone SE (375x667)" — 10 tests passed after final accessibility cleanup
  • If this PR changes candidate selection for a sweep/cron/alarm loop (WHERE clause, status set, join, or equivalent), expected candidate volume and worst-case per-candidate cost are stated in the summary or validation notes (see .claude/rules/47-control-loop-io-budget.md)
    • N/A: no sweep/cron/alarm candidate selection changes.

Staging Verification (REQUIRED for all code changes — merge-blocking)

  • Staging deployment greenDeploy Staging workflow triggered manually and passed for this branch
  • Live app verified via Playwright — logged into app.sammy.party (staging) using test credentials and actively tested the application
  • Existing workflows confirmed working — navigated dashboard, projects, and settings; confirmed no regressions in core flows (pages load, data displays, navigation works, no new console errors)
  • New feature/fix verified on staging — the specific changes in this PR work correctly on the live staging environment (describe what was tested below)
  • Infrastructure verification completed — VM provisioned and heartbeat confirmed (required for changes to cloud-init, VM agent, DNS, TLS, scripts/deploy). Write N/A: no infra changes ONLY if the PR does not touch any infrastructure paths.
    • N/A: no cloud-init, VM agent, DNS, TLS, or deployment script changes.
  • Mobile and desktop verification notes added for UI changes

Staging Verification Evidence

  • Deploy Staging run 32876032734 passed for branch sam/chat-ui-desktop-broken-x7a6h9 at SHA bb7288d39, including post-deploy smoke tests. The later branch commit c58c9dd61 only records task evidence and does not change runtime/deployable files.
  • Exact-head live Chromium verifier passed against https://app.sammy.party after staging deploy:
    • Desktop 1280x800: comments rail absent by default; message-level Comment opened the rail; selected-text Comment opened a quoted composer in the rail; header/dropdown Comments reopened the rail.
    • Mobile 375x667: selected-text Comment opened the fixed bottom composer; conversation scroller remained scrollable behind it.
    • No browser console errors observed in the exact-head verifier.
  • Library screenshots:
    • 01M0WZ9XMVJZBFH4K7QET6WR0Fstaging-exacthead-comments-desktop-rail-1280x800.png
    • 01M0WZA1SH67SH4RVWGF9AY2J6staging-exacthead-comments-mobile-bottom-composer-375x667.png
    • Additional local/staging audit screenshots are listed in tasks/active/2026-08-25-on-demand-comment-rail-mobile-overlay.md.

UI Compliance Checklist (Required for UI changes)

  • Mobile-first layout verified
  • Accessibility checks completed
  • Shared UI components used or exception documented
  • Playwright visual audit run locally — mock data scenarios (normal, long text, empty, many items, error, special chars) tested at mobile (375x667) and desktop (1280x800); no horizontal overflow; screenshots in .codex/tmp/playwright-screenshots/ (see .claude/rules/17-ui-visual-testing.md)

Notes:

  • Reused Button, Spinner, CommentComposer, and CommentThreadList for the new rail/mobile surfaces.
  • Desktop rail is an aside with aria-label="Session comments"; draft sections and mobile composer have explicit accessible labels used by unit and Playwright checks.
  • Screenshots reviewed for desktop thread/error/empty states and mobile bottom composer before/after submit.

End-to-End Verification (Required for multi-component changes)

  • Data flow traced from user input to final outcome with code path citations (see .claude/rules/10-e2e-verification.md)
  • Capability test exercises the complete happy path across system boundaries
  • All spec/doc assumptions about existing behavior verified against code (not just "read the code")
  • If any gap exists between automated test coverage and full E2E, manual verification steps documented below

Data Flow Trace

  1. User entry points call comment UI state: selected text and message buttons invoke startComment / onToggleMessageComments in apps/web/src/components/project-message-view/comments/useProjectMessageCommentUi.tsx:39 and apps/web/src/components/project-message-view/comments/useProjectMessageCommentUi.tsx:77.
  2. Desktop breakpoint state opens the parent comments surface through onRequestCommentSurface, and ProjectMessageView chooses docked rail vs mobile drawer with showDockedCommentRail / showMobileCommentsDrawer in apps/web/src/components/project-message-view/index.tsx:326.
  3. The chat layout renders the rail beside the virtualized conversation in apps/web/src/components/project-message-view/index.tsx:570, and renders the mobile selected-text composer over the input at apps/web/src/components/project-message-view/index.tsx:664.
  4. DesktopCommentRail renders loading/error/empty/thread/draft states and submits through CommentComposer in apps/web/src/components/project-message-view/comments/MessageCommentPanels.tsx:127; MobileSelectedTextCommentComposer renders the fixed mobile composer in apps/web/src/components/project-message-view/comments/MessageCommentPanels.tsx:269.
  5. useMessageComments sends create/reply/resolve/reopen/send mutations and reconciles optimistic state in apps/web/src/components/project-message-view/comments/useMessageComments.ts:93.
  6. The API client posts message comments to /api/projects/:projectId/sessions/:sessionId/comments in apps/web/src/lib/api/comments.ts:206.

Untested Gaps

N/A: local unit tests, local Playwright audit, staging smoke tests, and live staging Playwright/Chromium checks cover the changed user flows. The live exact-head check creates real staging comments but does not delete them because message comments do not expose a destructive cleanup path.

Post-Mortem (Required for bug fix PRs)

What broke

On desktop, clicking message-level Comment or selected-text Comment appeared to do nothing. The comment draft state changed, but no visible composer was rendered for the user.

Root cause

PR #1882 introduced message comments with inline panels hidden at lg; PR #1897 moved session comments into the drawer/inbox flow and removed the earlier desktop rail surface. That left desktop comment controls pointing at state whose visible composer only existed in a mobile-only inline panel.

Class of bug

Responsive/on-demand surface visibility regression: state changed successfully, but the only visible UI for that state was hidden at the active breakpoint.

Why it wasn't caught

Coverage did not require desktop tests to click every public comment entry point and assert a visible composer/rail at 1280x800. State-level or mobile-only coverage was not sufficient.

Process fix included in this PR

  • .claude/rules/17-ui-visual-testing.md now requires responsive/on-demand surfaces to be proved visible from every public entry point at the viewport where the action is available.
  • apps/web/tests/playwright/message-comments-audit.spec.ts now clicks the real desktop/mobile comment controls and asserts the resulting rail/composer is visible.
  • apps/web/tests/unit/components/project-message-view.test.tsx now covers header, message-level, selected-text, hidden-by-default rail, and mobile bottom composer behavior.

Post-mortem file

  • tasks/active/2026-08-25-on-demand-comment-rail-mobile-overlay.md
  • .claude/rules/17-ui-visual-testing.md

Specialist Review Evidence (Required for agent-authored PRs)

  • All local reviewers completed and findings addressed before merge
  • If any reviewer did NOT complete: needs-human-review label added and merge deferred to human
    • N/A: all reviewers completed.
Reviewer Status Outcome
ui-ux-specialist PASS On-demand desktop rail and fixed mobile selected-text composer match the requested responsive UX; screenshot review found no overflow/clipping blockers.
test-engineer PASS Unit and Playwright coverage exercise real header, message, selected-text, mobile overlay, voice, loading, error, retry, and thread actions.
constitution-validator PASS New constants are presentational UI geometry/breakpoint values; no new endpoints, secrets, provider IDs, business limits, or deployment config hardcoded.
doc-sync-validator PASS Rule update is scoped to this bug class and references existing code/test behavior without stale file references.
task-completion-validator PASS Completed checklist items are reflected in the diff, validation logs, staging evidence, and uploaded screenshots.

Exceptions (If any)

  • Scope: final branch commit after the exact-head staging run is task-evidence documentation only.
  • Rationale: runtime/deployable files were unchanged after staging run 32876032734; exact deployed app behavior was rechecked live after that run.
  • Expiration: merge of this PR.

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: no external API or third-party documentation needed; change was based on local code, project rules, and staging evidence.

Codebase Impact Analysis

Affected paths:

  • apps/web/src/components/project-message-view/ — chat layout, floating header extraction, comment rail/mobile composer integration.
  • apps/web/src/components/project-message-view/comments/ — responsive comment surface state, desktop rail, mobile selected-text composer, inline suppression for detached selected-text drafts.
  • apps/web/tests/unit/components/project-message-view.test.tsx — unit coverage for desktop/mobile comment entry points.
  • apps/web/tests/playwright/message-comments-audit.spec.ts — visual/behavior audit for desktop rail and mobile bottom composer.
  • .claude/rules/17-ui-visual-testing.md — process guardrail for responsive/on-demand surfaces.

Documentation & Specs

  • Updated .claude/rules/17-ui-visual-testing.md with the retained incident lesson and required testing pattern.
  • Added/updated task evidence in tasks/active/2026-08-25-on-demand-comment-rail-mobile-overlay.md.
  • N/A for public www docs: this is internal chat UI behavior, not a public API/config/self-hosting contract change.

Constitution & Risk Check

Checked Principle XI / no hardcoded values. New fixed values are local UI geometry/breakpoint constants and Tailwind class values tied to responsive layout. No secrets, URLs, provider identifiers, business limits, or deployment-owned configuration were introduced.

@codspeed-hq

codspeed-hq Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing sam/chat-ui-desktop-broken-x7a6h9 (c58c9dd) with main (7c5c2c8)

Open in CodSpeed

@sonarqubecloud

Copy link
Copy Markdown

@raphaeltm
raphaeltm merged commit b5c96ae into main Aug 25, 2026
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant