[codex] Add copy action to conversation error banner#1432
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Mock-LLM E2E Tests10/10 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — Clean, focused feature that does exactly what it says. A copy button that exposes the full error text (including collapsed content) is genuinely useful UX and is implemented cleanly. One real improvement opportunity noted inline.
[IMPROVEMENT OPPORTUNITIES]
- See inline comment on
handleCopyre: missing error handling.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Small, isolated UI addition to a single component. No API surface, routing, or shared state changes. Worst case is a silent clipboard failure, which is not a regression.
VERDICT:
✅ Worth merging — Core logic is sound; the improvement suggestion can be addressed as a follow-up or inline.
KEY INSIGHT:
A shared useCopyToClipboard hook would eliminate duplication between this component and the existing CopyableContentWrapper, but that's a refactor that can come separately.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
Addressed the review feedback in
Local validation passed:
I am re-adding the |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
✅ Mock-LLM E2E Tests10/10 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Elegant, minimal, correct.
The implementation is solid across the board:
- Correctness:
displayTextForLength(the full resolved text, translated if needed) is exactly the right thing to copy — including content hidden by the collapsedline-clamp-3view. That was the whole point of the feature. - Error handling:
handleCopywraps the async clipboard write in a try/catch and surfaces the failure via the existingdisplayErrorToastpath. Good UX in an error banner context. - State management: The two effects (reset after 2 s, reset when message changes) are straightforward and cover the edge cases cleanly.
- Accessibility:
aria-labeltoggles betweenBUTTON$COPY/BUTTON$COPIED; icons arearia-hidden. Correct. - Tests: Both paths (successful write → copied state, rejected write → toast + stays on Copy) are tested with real clipboard spy calls, not just mocks-of-mocks. The
vi.hoisted()pattern for the toast module is the right setup for ESM.afterEachcleanup is correct (restoreAllMocksfor the spy,mockClearfor the hoisted fn). - Evidence: Screenshot in the PR description confirms the UI change is visible and working.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Isolated UI addition to one component. No shared state, no routing, no API changes. Worst case is clipboard API unavailability, which is now handled gracefully with a toast.
VERDICT:
✅ Worth merging — Ready to land as-is.
KEY INSIGHT:
Copying displayTextForLength rather than message ensures i18n-keyed messages are copied in their human-readable translated form, not as raw keys — a detail that's easy to get wrong and is handled correctly here.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
HUMAN:
I have tested and this works

AGENT:
Why
Long conversation error banners can contain details that are difficult to select and copy, especially when the message is collapsed or spans many lines.
Summary
ErrorMessageBanner.Issue Number
N/A
How to Test
npx vitest run __tests__/components/chat/error-message-banner.test.tsxnpm run typechecknpm run build200.Video/Screenshots
See the screenshot in the HUMAN section above.
Type
Notes
The copy action uses the browser Clipboard API and resets its copied indicator after two seconds or when the error text changes.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.28.1-pythonopenhands-automation==1.0.0a109c781b7ba7247bc701cf7946633a18fe2f43e2d7Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-9c781b7Run
All tags pushed for this build
About Multi-Architecture Support
sha-9c781b7) is a multi-arch manifest supporting both amd64 and arm64sha-9c781b7-amd64) are also available if needed