Polish collapsible Fix with AI error banner - #4351
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff5e858e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This PR extracts the inline No HIGH issues. Two MEDIUM notes below are behavioral/accessibility polish rather than merge blockers. The diff was not truncated, so confidence is good; I did not run the test suites. Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
wwwillchen
removed
the
needs-human:review-issue
wwwillchen
changed the title
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This is a contained renderer-only refactor: the inline Two behavioral issues are worth a look before merge; neither is a hard blocker on its own. The bigger one is that the new collapse and details toggles are component state on a component that stays mounted across errors, so a collapsed banner stays collapsed for the next, unrelated error - hiding the primary "Fix error with AI" recovery action that was previously always visible. The second is the The diff is complete and untruncated, so confidence in the code reading is good. One thing I could not verify from the diff alone: whether the two regenerated Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
081c196 to
9527f46
Compare
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This PR extracts the preview Two non-blocking items are noted below. Nothing here is a merge blocker. Issues Summary
🟢 Low Priority Notes (5 items)
Note on confidence: neither the diff nor the patches were truncated, so the review covers the full change. I could not execute the Playwright suite in this environment, so the two regenerated aria snapshots were checked by reasoning about DOM order and role mapping rather than by running the tests; they look consistent with the new markup (paragraph before controls, Generated by Dyadbot persona-based code review |
- Reset error-banner disclosures when a new error arrives\n- Reuse the shared PreviewError type\n- Add regression coverage for disclosure reset
🤖 Claude Code Review SummaryPR Confidence: 4/5All trusted review threads are resolved and focused tests plus formatting, lint, and type checks pass; confidence is held below 5 because the packaged E2E flow could not be rerun after Electron Forge failed rebuilding Unresolved ThreadsNo unresolved threads Resolved Threads
🤖 Generated by Claude Code |
| } | ||
| aria-expanded={!isBannerCollapsed} | ||
| aria-controls="preview-error-banner-content" | ||
| className="rounded p-1 text-red-500 transition-colors hover:bg-red-100 hover:text-red-700 dark:text-red-400 dark:hover:bg-red-900 dark:hover:text-red-200" |
There was a problem hiding this comment.
🟡 MEDIUM
New banner controls are missing cursor-pointer
The collapse toggle (line 76), the dismiss button (line 89), and the "Show details" toggle (line 100) have no cursor-pointer class, while the "Fix error with AI" button in the same banner explicitly sets it. This project is on Tailwind v4, whose preflight no longer applies cursor: pointer to buttons, and there is no global CSS rule restoring it, so these three new affordances will show the default arrow cursor. Sibling components such as PreviewLoadingScreen's error toggle set cursor-pointer explicitly for exactly this reason, so the banner ends up inconsistent with the rest of the preview panel in a PR whose goal is polish.
💡 Suggestion: Add cursor-pointer to the collapse, dismiss, and Show details button class strings.
| expect(screen.getByRole("button", { name: "Hide details" })).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("resets disclosure state when a different error arrives", () => { |
There was a problem hiding this comment.
🟡 MEDIUM
Remount test asserts React behavior, not the component contract
The "resets disclosure state when a different error arrives" test supplies the key prop itself in both render and rerender, so changing the key remounts the component and resets useState regardless of anything PreviewErrorBanner does. It passes even if the component were rewritten, and it does not cover the piece that can actually regress: the key={${errorMessage.source}:${errorMessage.message}} in PreviewIframe.tsx. If someone drops that key, this test stays green while a stale collapsed banner is shown for a brand-new error.
💡 Suggestion: Render through a small wrapper that mirrors the PreviewIframe call site (passing only the error prop and letting the wrapper own the key), or assert the reset behavior in a test that renders PreviewIframe's banner slot.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This PR extracts the preview error banner into Things that check out:
The two MEDIUM items below are polish/maintainability notes, not merge blockers. Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 815 passed, 22 failed, 7 flaky, 302 skipped Failed Tests🍎 macOS
🪟 WindowsShow all 20 failures
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/concurrent_chat.spec.ts \
e2e-tests/local_agent_auto.spec.ts
|
1 participant
Summary
Make the preview's Fix with AI error banner collapsible while giving the expanded state a calmer, more conventional error-recovery hierarchy.