Skip to content

fix(ui): show pending tool prompts#367

Closed
pascalandr wants to merge 3 commits into
NeuralNomadsAI:devfrom
Pagecran:fix/show-pending-tool-asks
Closed

fix(ui): show pending tool prompts#367
pascalandr wants to merge 3 commits into
NeuralNomadsAI:devfrom
Pagecran:fix/show-pending-tool-asks

Conversation

@pascalandr
Copy link
Copy Markdown
Contributor

@pascalandr pascalandr commented Apr 25, 2026

Fixes #290

Summary

  • keep tool calls expanded while a permission or question is attached, even before active interruption reconciliation catches up
  • use the same pending-interruption rule for diagnostics expansion and manual toggle guards
  • make permission asks more reliably visible in the message stream when tool calls are normally collapsed

Testing

  • npm run typecheck --workspace @codenomad/ui
  • npm run build --workspace @codenomad/ui

@pascalandr
Copy link
Copy Markdown
Contributor Author

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented Apr 25, 2026

Gatekeeper review of the latest PR state.

I understood the goal here as: keep tool prompts visible while a permission or question is pending, especially when tool calls are normally collapsed, and make the expansion/collapse rules consistent.

  1. Blocking: pending interruptions can still be hidden from the message stream when tools are collapsed.
    packages/ui/src/components/tool-call.tsx:660-720 now forces the individual tool call open for any pending permission/question, but packages/ui/src/components/message-timeline.tsx:751-771 still decides whether the entire tool segment is hidden based only on hasActivePermission.

That means the new expanded() logic never gets a chance to help in the cases this PR is trying to cover:

  • a permission is pending but not yet marked active during reconciliation lag
  • a question is pending at all

In both cases, with tools collapsed, the segment can still remain fully hidden from the timeline. So the PR does not fully deliver the stated visibility fix yet.

Suggested direction: move the timeline visibility rule to the same pending-interruption predicate used in tool-call.tsx, instead of keying only off permissionState?.active.

  1. Medium: the generic pending banner still ignores pending questions, so the UI can show a misleading "waiting to run" state next to an actual question prompt.
    packages/ui/src/components/tool-call.tsx:514-518 and :550-554 suppress the pending banner only when pendingPermission() exists. After this change, a pending question also forces expansion, but those branches still render the generic spinner/banner.

That creates inconsistent UX: the tool is simultaneously shown as "waiting to run" while also actively asking the user a question. This should use the same shared pending-interruption predicate, or be split into interruption-specific messaging.

Verification:

  • npm run typecheck --workspace @codenomad/ui could not complete in this environment because workspace dependencies are missing (solid-js, zod, etc.).
  • npm run build --workspace @codenomad/ui could not complete in this environment because vite is not installed here.

Non-blocking note: packages/ui/src/components/tool-call.tsx is already ~942 lines, which is above the source-file warning threshold and makes this kind of state-visibility drift easier to introduce.

--
Yours,
CodeNomadBot

@pascalandr
Copy link
Copy Markdown
Contributor Author

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented Apr 25, 2026

Updated gatekeeper review for the latest PR state (51173f2).

I understood the intent as unchanged: make pending permission/question prompts reliably visible even while interruption reconciliation is still catching up, and keep the expansion/collapse behavior consistent.

  1. Blocking: the timeline visibility fix still misses unreconciled pending interruptions.
    packages/ui/src/components/message-timeline.tsx:754-782 now checks getPermissionState(messageId, partId) and getQuestionState(messageId, partId) and keeps the tool segment visible when either is present. That fixes the part-resolved case, but it still does not cover the exact lag window this PR description calls out.

Why: unresolved interruptions are stored before they have a resolved partId.

  • packages/ui/src/stores/message-v2/bridge.ts:169-183 and :222-236 upsert permission/question entries even when partId is still missing.
  • packages/ui/src/stores/message-v2/instance-store.ts:907-916 keys those unresolved entries by entry.permission.id / request id, not by the tool partId.
  • message-timeline.tsx only probes by segment.messageId + partId, so during that unreconciled window it still won’t see the pending entry and can still hide the tool segment.

That means the latest change still falls short of the PR’s stated guarantee for "even before active interruption reconciliation catches up".

Suggested direction: use a shared helper that can answer "does this tool segment have any pending interruption?" from the same data sources as tool-call.tsx, including unresolved queue entries matched by message/call identity, not just resolved byMessage[messageId][partId] lookups.

What improved since the previous review:

  • The earlier question-banner inconsistency in tool-call.tsx is fixed. The generic pending banner is now suppressed for both pending permissions and pending questions.
  • The timeline now correctly handles resolved pending questions in addition to permissions.

Verification:

  • npm run typecheck --workspace @codenomad/ui still cannot complete in this environment because workspace dependencies are missing (solid-js, zod, etc.).
  • npm run build --workspace @codenomad/ui still cannot complete in this environment because vite is not installed here.

Non-blocking note: packages/ui/src/components/tool-call.tsx remains ~942 lines, above the warning threshold.

--
Yours,
CodeNomadBot

@pascalandr
Copy link
Copy Markdown
Contributor Author

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot
Copy link
Copy Markdown
Contributor

codenomadbot Bot commented Apr 25, 2026

Updated gatekeeper review for the latest PR state (9ee9ed2).

I understood the intent as unchanged: make pending permission/question prompts reliably visible even before interruption reconciliation catches up, while keeping the collapse/expansion rules aligned.

No blocking findings in the latest revision.

What changed that resolves the previous blocker:

  • packages/ui/src/stores/message-v2/instance-store.ts:976-1077 now centralizes tool interruption lookup via getToolInterruptionState(...).
  • That helper checks both the direct per-part state and unresolved queued entries by messageId + tool call id, which closes the lag window where a pending interruption existed before partId reconciliation.
  • packages/ui/src/components/message-timeline.tsx:752-779 now uses that shared store helper, so hidden/collapsed tool segments stay visible whenever the tool has any pending interruption.
  • packages/ui/src/components/tool-call.tsx:588-911 also consumes the same interruption source, which is a better implementation than keeping the matching logic split across component and timeline layers.

Residual risks / non-blocking notes:

  • I do not see targeted automated coverage for the specific regression this PR is fixing: unresolved queued permission/question entries that later reconcile onto a tool part. The implementation looks sound, but this path is subtle enough that a focused store or UI test would be valuable.
  • packages/ui/src/components/tool-call.tsx is still ~942 lines, above the source-file warning threshold.

Verification:

  • npm run typecheck --workspace @codenomad/ui still cannot complete in this environment because workspace dependencies are missing (solid-js, zod, etc.).
  • npm run build --workspace @codenomad/ui still cannot complete in this environment because vite is not installed here.

Gatekeeper verdict: no current code-quality blockers found in the latest PR state.

--
Yours,
CodeNomadBot

@shantur
Copy link
Copy Markdown
Collaborator

shantur commented Apr 26, 2026

@pascalandr - Were you able to reproduce this issue at your end?

@pascalandr
Copy link
Copy Markdown
Contributor Author

@pascalandr - Were you able to reproduce this issue at your end?

Yes

@pascalandr
Copy link
Copy Markdown
Contributor Author

pascalandr commented Apr 26, 2026

Thinking about it more, recently I often run into permission-related weirdness, including in yolo mode, and a lot of symptoms can end up looking broken. So I think this needs more investigation, and the root cause is probably more complex than the small band-aid in this PR.

@shantur
Copy link
Copy Markdown
Collaborator

shantur commented Apr 26, 2026

Thinking about it more, recently I often run into permission-related weirdness, including in yolo mode, and a lot of symptoms can end up looking broken. So I think this needs more investigation, and the root cause is probably more complex than the small band-aid in this PR.

Shall we close this then and wait for the time when we know exactly what's happening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: sometime missing display ask

2 participants