fix: key tool-permission resolvers by call id instead of window.resolveTool#169
Merged
jmaxdev merged 2 commits intoTrixtyAI:mainfrom Apr 21, 2026
Merged
Conversation
|
Thanks for the contribution! I'll review it as soon as possible. If you have still changes, please mark this PR as draft and all reviews will be cancelled. Tests reviews will be re-run only when the PR is marked as ready for review. |
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency bug in the desktop AI assistant tool-permission flow where overlapping permission prompts could overwrite a single global resolver (window.resolveTool), leaving earlier tool calls awaiting forever.
Changes:
- Replaced the single global resolver with a ref-backed
Map<callId, resolver>so each in-flight permission prompt is tracked independently. - Updated the Allow/Deny handlers to resolve the currently visible
pendingTool.identry and clean up the corresponding resolver. - Removed the
resolveToolproperty from the globalWindowtype augmentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/desktop/src/addons/builtin.ai-assistant/AiChatComponent.tsx | Stores per-call permission resolvers in a Map and updates the permission dialog button handlers to resolve by call id. |
| apps/desktop/src/api/trixty.ts | Removes resolveTool from the global Window interface extension. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matiaspalmac
added a commit
to matiaspalmac/ide
that referenced
this pull request
Apr 21, 2026
…overrides Addresses review feedback on TrixtyAI#169: - Replace `Math.random().toString(36).substr(2, 9)` with `crypto.randomUUID()` (with a timestamp-plus-random fallback for environments where randomUUID is unavailable). The previous id had narrow entropy and could collide with a still-pending entry in the resolver Map, silently overwriting the earlier resolver and reintroducing the dangling-Promise bug this PR is meant to close. Also drops the deprecated `substr` in favor of `slice`. - In the register path, if the new callId already has a resolver entry (e.g. because the provider reused an id), resolve that prior resolver as denied before replacing it. Pairs with the override-an-older-different-id behavior that was already there. - Rewrote the Allow and Deny button handlers to: 1. Capture the id at click time and early-return when no matching resolver is in the Map (stale or double-click). 2. Clear `pendingTool` only via a functional setter that checks `current.id === clickedId`. A faster follow-up prompt that's already replaced `pendingTool` in state is no longer hidden by a click on the old dialog.
matiaspalmac
force-pushed
the
fix/resolve-tool-map
branch
from
685e50b to
6a3f86a
Compare
…veTool The previous implementation stored the in-flight permission resolver on `window.resolveTool`, which meant two overlapping permission prompts (two chat submissions racing, a tool loop retrying over its own pending prompt, etc.) silently overwrote the first resolver. The first Promise then dangled forever and its `await permissionPromise` never returned, leaking the first tool call's state and stalling the outer send loop. It also polluted the global `Window` type with an implementation detail. Changes: - AiChatComponent keeps a ref-backed Map<callId, resolver>. Each permission prompt registers its own resolver by call id, so the Allow/Deny buttons on the UI look up the resolver for the currently visible `pendingTool.id` rather than a shared global. - If a new prompt arrives while a previous one is still unresolved, the older entry is resolved as denied before the new one takes over the single-slot dialog. That's the safe default: the user never actually saw the older dialog (only the latest renders), so the earlier tool is not approved. - A cleanup effect resolves every remaining entry as denied on unmount so awaiters don't leak past the component's lifetime. - Dropped the global `resolveTool?: (allowed: boolean) => void` declaration from `src/api/trixty.ts`.
…overrides Addresses review feedback on TrixtyAI#169: - Replace `Math.random().toString(36).substr(2, 9)` with `crypto.randomUUID()` (with a timestamp-plus-random fallback for environments where randomUUID is unavailable). The previous id had narrow entropy and could collide with a still-pending entry in the resolver Map, silently overwriting the earlier resolver and reintroducing the dangling-Promise bug this PR is meant to close. Also drops the deprecated `substr` in favor of `slice`. - In the register path, if the new callId already has a resolver entry (e.g. because the provider reused an id), resolve that prior resolver as denied before replacing it. Pairs with the override-an-older-different-id behavior that was already there. - Rewrote the Allow and Deny button handlers to: 1. Capture the id at click time and early-return when no matching resolver is in the Map (stale or double-click). 2. Clear `pendingTool` only via a functional setter that checks `current.id === clickedId`. A faster follow-up prompt that's already replaced `pendingTool` in state is no longer hidden by a click on the old dialog.
jmaxdev
force-pushed
the
fix/resolve-tool-map
branch
from
6a3f86a to
0fbc1d0
Compare
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.
[Fix]: Key tool-permission resolvers by call id instead of
window.resolveToolDescription
Tool-permission prompts stored their
Promiseresolver on a singleglobal —
window.resolveTool— and the UI's Allow/Deny buttons calledinto that global. Because only one slot existed, two overlapping
permission prompts (two chat submissions racing, a tool loop
retrying over its own pending prompt, extension-triggered tool calls
interleaving with user submissions, etc.) silently overwrote the
first resolver. The first Promise then dangled forever, its outer
await permissionPromisenever returned, and the send loop for thatmessage stalled without a visible error. The shape also polluted the
global
Windowtype with an implementation detail of a singlecomponent.
Change
apps/desktop/src/addons/builtin.ai-assistant/AiChatComponent.tsx:window.resolveToolwith a ref-backedMap<callId, resolver>. Each permission prompt registers its ownentry under the current tool call id.
resolver for the currently visible
pendingTool.id, delete theentry, clear
pendingTool, and fire only that resolver.unresolved, the older entry is resolved as denied before the
new one takes over the single-slot dialog. Denying is the
fail-closed default: the user never actually saw the older dialog
(the UI only renders the latest), so the earlier tool was not
approved and should not be treated as such.
remaining entry as denied so awaiters don't leak past the
component's lifetime.
apps/desktop/src/api/trixty.ts:resolveTool?: (allowed: boolean) => voidentry fromthe
declare global { interface Window { … } }block.Trade-offs
single-slot — only the most recent prompt is shown. Stacking
multiple dialogs would be a UX change beyond the scope of this
fix, and the new Map accommodates it later if someone wants to
move in that direction.
visible behavior change vs. "dangles forever". It matches the
defensive default (unseen prompts stay unapproved), aligns with the
cleanup-on-unmount path, and surfaces the race to the calling code
immediately instead of the old silent stall.
and the existing project has no renderer-level test harness to
drive the component. Tracing the four code paths (first register,
override, allow, deny) by hand was enough to verify in isolation.
Verification
pnpm tsc --noEmitacross the desktop app → clean.pnpm eslinton both touched files → 0 findings.window.resolveToolanywhere except the new explanatory comment.
true, Deny withfalse, dialog clears.the visible dialog, clicking Allow/Deny affects only the newer.
outstanding entry with
false, no dangling Promises.Related Issue
Fixes #72
Checklist