Conversation
Preview:
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const onResolvedRef = useRef(onResolved) | ||
| onResolvedRef.current = onResolved | ||
|
|
||
| return useCallback(async (actionId: number, decision: ActionDecision) => { | ||
| return useCallback(async function resolveAction(actionId: number, decision: ActionDecision) { |
There was a problem hiding this comment.
🔍 Hook style diverges from convention
The edited hook remains a function declaration despite the repository's named-arrow default. Review whether recursion warrants this departure.
Was this helpful? React with 👍 or 👎 to provide feedback.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| console.error(`Failed to ${decision} action:`, error) | ||
| toasts.add({ title: `Failed to ${decision} action`, variant: 'error' }) | ||
| const diagnostics = actionErrorDiagnostics(error, { actionId, decision }) | ||
| const toastId = toasts.add({ |
There was a problem hiding this comment.
P2: Clear stale failure toasts when resolution starts elsewhere
This persistent toast is closed only by its own Try again callback. After a failure, retrying from the still-visible action card can succeed while this toast remains indefinitely; clicking its retry afterward targets an already-resolved action and creates another misleading persistent error. Repeated card failures also stack distinct timeout: 0 toasts. Track one toast per action and close/replace it whenever any new resolution attempt starts (and when the action is observed resolved).
|
Submitted 1 actionable inline finding. |
|
|
||
| return useCallback(async function resolveAction(actionId: number, decision: ActionDecision) { | ||
| if (currentOverseer.current !== overseer) return | ||
| closeFailureToast.current?.() |
There was a problem hiding this comment.
P2: Keep failure toasts for other pending actions
closeFailureToast is shared across every action handled by this hook, so starting a resolution for action B immediately dismisses action A’s persistent error toast here, even if A is still pending. For example, after A fails, clicking Approve on B removes A’s only “Try again” and “Copy error details” controls; concurrent failures also overwrite each other in the catch block. Track the failure toast by action ID and close only the toast for the action being retried/resolved.
There was a problem hiding this comment.
I think that's okay. If you moved on the approve action B and so lost the toast for the failing action A, you can just reapprove action A and you'll get the toast again.
|
Submitted one actionable inline finding: starting another action dismisses the first action’s failure toast, removing its retry and copy-details controls while that action is still pending. |
There was a problem hiding this comment.
🔍 Contribution exceeds the external-review bar
This outside contribution adds 129 lines of retry, reconnection, toast-lifecycle, and concurrency logic. Repository policy limits such contributions to small, trivially verified changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| return useCallback(async function resolveAction(actionId: number, decision: ActionDecision) { | ||
| if (currentOverseer.current !== overseer) return | ||
| closeFailureToast.current?.() |
There was a problem hiding this comment.
🟡 Unrelated actions dismiss recovery toast
Starting any action through resolveAction dismisses the previous failed action's recovery toast, even when their IDs differ. The first failure then loses both retry and diagnostic controls.
Learn more
Each hook instance owns one closeFailureToast slot, but its callers can resolve several pending actions independently. A failure stores its recovery controls in that single slot. Any later action closes the slot before checking whether that action is related to the failure. Concurrent failures also replace one another when they settle, so only the last failure remains recoverable.
Example: Action 5 fails and displays retry and diagnostic controls. The user then approves action 6. Starting action 6 closes action 5's toast, although action 5 remains pending and unresolved.
Recommended fix: Track failure toasts by action ID, or keep independent failure toasts. Close only the toast for the action being retried or resolved, and clean up every tracked toast when the overseer changes or the hook unmounts.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
What does this change?
Currently, the "Failed to approve action" leaves the user unable to do anything. The real error is hidden in the browser console. This PR adds a "Copy error details" and a "Try again" button.
The error schema looks like this:
{ "operation": "approve-action", "actionId": 5, "error": { "type": "Error", "message": "Home Assistant WS command failed (not_found): Service cover.press not found.", "stack": "evaluateImpl@http://localhost:3000/node_modules/.vite/deps/capnweb.js?v=f70653e6:1560:75\nevaluateWithDepth@http://localhost:3000/node_modules/.vite/deps/capnweb.js?v=f70653e6:1470:28\nevaluate@http://localhost:3000/node_modules/.vite/deps/capnweb.js?v=f70653e6:1465:17\nreadLoop@http://localhost:3000/node_modules/.vite/deps/capnweb.js?v=f70653e6:2369:69\n" } }Why is this obviously correct and trivially verifiable?
It's a small change that would help users act upon seeing this error, either by retrying or by copying the error and reporting it to the devs, or by pasting it back in the chat and letting the agent understand what went wrong.
Checklist
Checking every item does not guarantee acceptance. Maintainers determine whether
a pull request meets the contribution policy.