feat: 2回目以降のチャットdesignを変更#13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the SessionDetail page from a single-iteration view into a chat-style stacked UI, where each iteration appears as a conversation bubble. It also adds a device_type field to iterations (backend model, migration, API schema, and frontend type), pins the @playwright/mcp version to 0.0.68, and filters noisy MCP log messages from the worker output.
Changes:
- UI redesign: Each iteration rendered as a right-aligned chat bubble with proposals grid below; latest iteration shows live log panel and interactive PR creation; a sticky input bar at the bottom allows chaining additional iterations.
device_typefield: Added to theIterationDB model, Alembic migration, backend schema/router, and frontendIterationtype — used to select proposal card min-width.- Worker updates: Pins
@playwright/mcp@0.0.68in the Dockerfile and both worker scripts, adds--viewport-sizeto the desktop config, and filters "Browser"/"Searching" prefixed text blocks from log output.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/pages/SessionDetail.tsx |
Major refactor: introduces IterationBlock sub-component, chat-style layout, auto-scroll, and sticky continue input |
frontend/src/components/Layout.tsx |
Adds headerExtra state and sticky header with repo/branch info injection support |
frontend/src/hooks/useLayoutContext.ts |
Exposes setHeaderExtra through the outlet context |
frontend/src/services/api.ts |
Adds device_type field to the Iteration interface |
frontend/src/components/ProposalCard.tsx |
Adds readOnly prop; visual distinction for selected past-iteration proposals |
frontend/src/components/StatusBadge.tsx |
Minor size increases (padding and font) |
frontend/src/components/LogPanel.tsx |
Shows tab bar even for a single job |
frontend/src/components/Sidebar.tsx |
Removes border-bottom from header area |
frontend/src/index.css |
Adds responsive breakpoint to hide repo info on mobile |
backend/app/model/session.py |
Adds DeviceType enum and device_type column to Iteration |
backend/migration/versions/d4e5f6g7h8i9_...py |
Alembic migration to add device_type column |
backend/app/schema/session_schema.py |
Adds device_type to IterationResponse |
backend/app/router/sessions.py |
Passes device_type into the iteration response |
backend/app/usecase/session_usecase.py |
Passes device_type to _update_iteration_status_with_retry |
docker/worker.Dockerfile |
Pins @playwright/mcp@0.0.68; changes Chromium install to use bundled version |
docker/worker-analyze.py |
Pins MCP version, adds viewport size, filters noisy log messages |
docker/worker-implement.py |
Pins MCP version, adds viewport size, filters noisy log messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(block, TextBlock): | ||
| emit_log(phase, block.text[:200], detail=block.text) | ||
| text = block.text.strip() | ||
| if text.startswith("Browser") or text.startswith("Searching"): |
There was a problem hiding this comment.
The text filter text.startswith("Searching") may suppress meaningful LLM output that genuinely begins with "Searching", such as "Searching for the relevant component..." or other contextual analysis messages from Claude. Since the elif tool_name in ("Glob", "Grep"): emit_log(...) branch was already removed from _emit_tool_detail, the "Searching: {pattern}" log lines from that branch are already gone. The additional string-prefix filter in the TextBlock loop is overly broad and could silently drop real diagnostic information from Claude's reasoning output.
Consider using a more targeted condition, such as checking for the exact phrases generated by MCP tool output (e.g., checking for "Browser action" patterns using a more specific prefix or regex) instead of the broad startswith("Searching").
| if text.startswith("Browser") or text.startswith("Searching"): | |
| if text.startswith("Browser"): |
| if isinstance(block, TextBlock): | ||
| emit_log(phase, block.text[:200], detail=block.text) | ||
| text = block.text.strip() | ||
| if text.startswith("Browser") or text.startswith("Searching"): |
There was a problem hiding this comment.
Same overly broad startswith("Searching") filter issue as in worker-analyze.py. The filter may suppress meaningful LLM output that starts with "Searching", since any genuine Claude reasoning text beginning with that word will be silently dropped from the log stream.
| if text.startswith("Browser") or text.startswith("Searching"): | |
| if text.startswith("Browser"): |
| text = block.text.strip() | ||
| if text.startswith("Browser") or text.startswith("Searching"): | ||
| continue | ||
| emit_log("implementing", text[:200], detail=block.text) |
There was a problem hiding this comment.
Same overly broad startswith("Searching") filter as in _process_messages. The filter may suppress meaningful Claude output in the implement_changes function as well.
| if text.startswith("Browser") or text.startswith("Searching"): | ||
| continue | ||
| emit_log("analyzing", text[:200], detail=block.text) |
There was a problem hiding this comment.
In the generate_proposals function, the continue statement at line 375 skips both the emit_log call AND the collected_text.append(block.text) call (line 377). This means that if Claude outputs a text block that starts with "Browser" or "Searching" (e.g., "Searching through the codebase, I found: {...json...}"), the proposals JSON embedded in that text block will never be appended to collected_text and will be silently lost, causing _extract_proposals_json to find no JSON and return an empty proposals list.
The filter should only skip the emit_log call, not the collected_text.append. Consider separating the log-suppression from the collection logic.
| if text.startswith("Browser") or text.startswith("Searching"): | |
| continue | |
| emit_log("analyzing", text[:200], detail=block.text) | |
| suppress_log = text.startswith("Browser") or text.startswith("Searching") | |
| if not suppress_log: | |
| emit_log("analyzing", text[:200], detail=block.text) |
| useEffect(() => { | ||
| if (iterationCount > 0) { | ||
| bottomRef.current?.scrollIntoView({ behavior: 'smooth' }) | ||
| } |
There was a problem hiding this comment.
The auto-scroll effect fires whenever iterationCount > 0, which includes the initial page load when a session with existing iterations is opened. This causes the page to automatically scroll to the bottom on every initial load, even for sessions with many iterations where the user may want to start reading from the top. Consider tracking the previous iterationCount (e.g., with a ref initialized to the current count) and only scrolling when the count increases beyond its initial value, rather than on the first render.
| useEffect(() => { | |
| if (iterationCount > 0) { | |
| bottomRef.current?.scrollIntoView({ behavior: 'smooth' }) | |
| } | |
| const prevIterationCountRef = useRef(iterationCount) | |
| useEffect(() => { | |
| const prevIterationCount = prevIterationCountRef.current | |
| if (iterationCount > prevIterationCount) { | |
| bottomRef.current?.scrollIntoView({ behavior: 'smooth' }) | |
| } | |
| prevIterationCountRef.current = iterationCount |
| key={iter.id} | ||
| <div style={{ maxWidth: '1000px', margin: '0 auto', padding: '0px 24px 12px' }}> | ||
| {/* All iterations stacked */} | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '24px', overflow: 'hidden' }}> |
There was a problem hiding this comment.
The overflow: 'hidden' on the iterations container will clip the box-shadow applied to selected ProposalCard components. The ProposalCard renders boxShadow: '0 0 0 2px rgba(59,130,246,0.3)' (or green variant for read-only selected), which extends outside the card's borders. Since overflow: 'hidden' on an ancestor clips any painted content that extends outside the ancestor's bounds, these selection halos will be invisible, making it harder for users to see which proposal is selected.
Remove overflow: 'hidden' from this container, or replace it with overflow: 'visible' (or remove it entirely, as it is the default). If the purpose is to prevent horizontal scrolling, consider using overflow-x: 'hidden' instead.
| <div style={{ display: 'flex', flexDirection: 'column', gap: '24px', overflow: 'hidden' }}> | |
| <div style={{ display: 'flex', flexDirection: 'column', gap: '24px' }}> |
| RUN npm install -g playwright@1.49.0 @playwright/mcp@latest && \ | ||
| # Install Playwright MCP with pinned version and its bundled Chromium | ||
| RUN npm install -g @playwright/mcp@0.0.68 && \ | ||
| cd /usr/lib/node_modules/@playwright/mcp && \ |
There was a problem hiding this comment.
The hardcoded path /usr/lib/node_modules/@playwright/mcp for the cd command assumes that npm install -g always installs to /usr/lib/node_modules. This path is specific to Debian/Ubuntu with NodeSource packages, but may differ in other environments (e.g., Alpine Linux uses /usr/local/lib/node_modules, or if the Node.js prefix changes). If the path is wrong, npx playwright install chromium will fail silently or install Chromium in an unexpected location.
A more reliable approach is to use $(npm root -g)/@playwright/mcp instead of the hardcoded path: cd $(npm root -g)/@playwright/mcp.
| cd /usr/lib/node_modules/@playwright/mcp && \ | |
| cd "$(npm root -g)"/@playwright/mcp && \ |
No description provided.