Fix on-demand chat comment surfaces - #1907
Merged
Merged
Conversation
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
FloatingHeaderout of the main chat component and add a retained UI visual-testing rule for responsive/on-demand surfaces.Validation
pnpm lintpnpm typecheckpnpm testpnpm buildpnpm --filter @simple-agent-manager/web test -- tests/unit/components/project-message-view.test.tsx— 66 tests passedpnpm --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.claude/rules/47-control-loop-io-budget.md)Staging Verification (REQUIRED for all code changes — merge-blocking)
Deploy Stagingworkflow triggered manually and passed for this branchapp.sammy.party(staging) using test credentials and actively tested the applicationN/A: no infra changesONLY if the PR does not touch any infrastructure paths.Staging Verification Evidence
Deploy Stagingrun32876032734passed for branchsam/chat-ui-desktop-broken-x7a6h9at SHAbb7288d39, including post-deploy smoke tests. The later branch commitc58c9dd61only records task evidence and does not change runtime/deployable files.https://app.sammy.partyafter staging deploy:01M0WZ9XMVJZBFH4K7QET6WR0F—staging-exacthead-comments-desktop-rail-1280x800.png01M0WZA1SH67SH4RVWGF9AY2J6—staging-exacthead-comments-mobile-bottom-composer-375x667.pngtasks/active/2026-08-25-on-demand-comment-rail-mobile-overlay.md.UI Compliance Checklist (Required for UI changes)
.codex/tmp/playwright-screenshots/(see.claude/rules/17-ui-visual-testing.md)Notes:
Button,Spinner,CommentComposer, andCommentThreadListfor the new rail/mobile surfaces.asidewitharia-label="Session comments"; draft sections and mobile composer have explicit accessible labels used by unit and Playwright checks.End-to-End Verification (Required for multi-component changes)
.claude/rules/10-e2e-verification.md)Data Flow Trace
startComment/onToggleMessageCommentsinapps/web/src/components/project-message-view/comments/useProjectMessageCommentUi.tsx:39andapps/web/src/components/project-message-view/comments/useProjectMessageCommentUi.tsx:77.onRequestCommentSurface, andProjectMessageViewchooses docked rail vs mobile drawer withshowDockedCommentRail/showMobileCommentsDrawerinapps/web/src/components/project-message-view/index.tsx:326.apps/web/src/components/project-message-view/index.tsx:570, and renders the mobile selected-text composer over the input atapps/web/src/components/project-message-view/index.tsx:664.DesktopCommentRailrenders loading/error/empty/thread/draft states and submits throughCommentComposerinapps/web/src/components/project-message-view/comments/MessageCommentPanels.tsx:127;MobileSelectedTextCommentComposerrenders the fixed mobile composer inapps/web/src/components/project-message-view/comments/MessageCommentPanels.tsx:269.useMessageCommentssends create/reply/resolve/reopen/send mutations and reconciles optimistic state inapps/web/src/components/project-message-view/comments/useMessageComments.ts:93./api/projects/:projectId/sessions/:sessionId/commentsinapps/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.mdnow 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.tsnow clicks the real desktop/mobile comment controls and asserts the resulting rail/composer is visible.apps/web/tests/unit/components/project-message-view.test.tsxnow 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.mdSpecialist Review Evidence (Required for agent-authored PRs)
needs-human-reviewlabel added and merge deferred to humanExceptions (If any)
32876032734; exact deployed app behavior was rechecked live after that run.Agent Preflight (Required)
Classification
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
.claude/rules/17-ui-visual-testing.mdwith the retained incident lesson and required testing pattern.tasks/active/2026-08-25-on-demand-comment-rail-mobile-overlay.md.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.