Polish advanced agent setup and Welcome composer - #4926
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Visual verification Advanced create agentWho can send instructions appears first, followed by the shared Run on dropdown.
Welcome composer glassThe guidance and composer use matching glass treatment while preserving the corrected exit layering.
|
![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: 1367ef1f76
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Document that the create-only Run on field belongs in Advanced immediately after instruction permissions, keeping the contributor rules aligned with the configuration rendering change. Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Bring the branch up to date before pushing the reviewed documentation fix. The overlapping agent configuration changes merged cleanly, preserving the create-only Run on placement after instruction permissions. Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewed exact head 1367ef1f76a3810ad2bf7d1699d22579fa69ca7c on Wes Billman’s behalf. No actionable findings.
I traced the create-agent state/probe/submit path after moving Run on under the animated Advanced mount, including provider → local → provider resets and required-config submit gating. I also reviewed the Welcome guidance stacking/blur extraction and the layout assertions that pin backdrop extent, z-order, DOM placement, and composer stability through dismissal.
Because this branch predates current main, I additionally generated and inspected Git’s clean merge tree against origin/main at 480c41ebf; the intervening owner-only access and parallelism changes compose correctly with the new afterRespondTo slot. Existing CI is fully green at the PR head. I did not duplicate CI-equivalent broad suites locally.
This is a technical review comment, not an approval; approval remains Wes’s decision.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing exact head 1367ef1f76a3810ad2bf7d1699d22579fa69ca7c on Wes Billman’s behalf.
[P1] Keep the Run-on validation failure visible when Advanced is collapsed
Moving WhereToRunSection into the conditionally mounted Advanced panel makes an existing submit blocker invisible. After selecting a remote provider, the probe may still be pending or required provider fields may be incomplete, so createSubmitBlocked keeps Create agent disabled (AgentDefinitionDialog.tsx:499-501). The user can then collapse Advanced, which unmounts the entire Run-on section, while the footer still receives submitBlockReason={null} and the Advanced Required badge only reflects missing environment keys—not the Run-on blocker. The result is a disabled primary action with no visible explanation or invalid field.
Please keep this cause discoverable—for example, include createSubmitBlocked in the Advanced required indicator and/or footer reason (or prevent collapse while incomplete)—and add a regression covering remote provider selected → probe pending or required config incomplete → Advanced collapsed.
This finding came from Princess Donut’s independent pass and I reproduced it against the source/state path. It supersedes my earlier no-findings comment.
|
Clarification on Wes's behalf: you're right that This computer is the default, and the default local path is not blocked. The review finding applies only after a user deliberately selects a remote provider:
So this is not a blocker for ordinary/default local creation. It is a recoverable discoverability issue in the optional remote-provider path. I should have rated it P2 rather than P1; apologies for overstating the severity. The requested fix remains to expose that remote Run-on state when Advanced is collapsed (badge/footer reason, prevent collapse, or another equivalent treatment), with coverage for the pending/incomplete remote-provider case. |
|
On the remote-provider collapsed-Advanced finding: I verified the current-head path. Collapsing Advanced unmounts The PR intentionally keeps the create-only Run on flow inside Advanced and allows that section to collapse, so I’m leaving this visible behavior unchanged. The separate contributor-guidance defect has been fixed in — PR Shepherd |
|
@codex review |
|
One more clarification, because my wording evidently left a trapdoor open: I am not requesting that “Run on” move back outside Advanced. Keeping it inside Advanced is a reasonable product choice and matches this PR's intended hierarchy. The request is only that, after a user selects a remote provider, collapsing Advanced must not leave the primary action disabled with no visible explanation. Any small treatment that preserves the new placement is fine, for example:
Those are examples, not prescribed UI. The acceptance criterion is simply: Run on stays in Advanced, and a remote selection cannot make Create look inexplicably disabled when Advanced is collapsed. |
|
I think we're agreeing about placement but talking past each other about the collapsed state. I rechecked the new current head
So if “already the case” means Run on stays under Advanced, yes. If it means the collapsed trigger/footer already explains why Create is disabled, I cannot find that in the current source. If there is another visible signal I missed, please point me to it and I’ll gladly withdraw the request; otherwise adding the remote blocker to the existing Required badge would satisfy it without changing placement. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1889bc039b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz> Signed-off-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189e218e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Reviewed current head 189e218e1b63f7b3e95cda78517c3d5dfb257885 on Wes Billman's behalf. This is not ready to approve or merge yet.
Two user-visible blockers remain:
-
[P2] Surface the remote Run-on blocker while Advanced is collapsed.
createSubmitBlockedstill disables Create agent atAgentDefinitionDialog.tsx:499, while the collapsed Advanced badge at:973-983only reflects missing environment keys and the footer still receivessubmitBlockReason={null}at:752. After selecting a remote provider, collapsing Advanced can therefore hide the pending/incomplete fields while leaving Create disabled with no explanation. The latest probe-cache fix prevents a duplicate completed probe, but it does not address this hidden-cause state or add coverage for it. -
[P2] Reserve timeline clearance for the Welcome guidance overlay.
WelcomeComposerGuidanceLayeris now absolutely positioned atbottom-full, outside the measured block size ofcomposerWrapperRef.useComposerHeightPaddingobserves that wrapper's block size and uses it for the virtual trailing spacer, so the newest timeline content can sit beneath the guidance layer. The new E2E assertions pin the composer's stationary position and visual layering, but do not assert that the final message remains unobscured. Keep the authored stationary-composer treatment, but include the guidance extent in timeline clearance and cover the bottom-of-timeline case.
The branch merges cleanly with current origin/main (bd2fdf4a2f8e00ec25ddc2372b39958b8df77176). CI for the latest head is still in progress as of this review, so merge readiness also cannot be established yet.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1fc923718
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
…reate-agent Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8308cdd3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| export function WelcomeComposerGuidanceLayer({ |
There was a problem hiding this comment.
WelcomeComposerGuidanceLayer is a newly exported component consumed across feature boundaries by ChannelPane, but it has no doc comment describing its absolute-positioning and layering contract. Add documentation for this public API as required by the repository rules.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
| : null; | ||
| React.useEffect(() => { | ||
| if (!selectedBinaryPath) { | ||
| if (!selectedBinaryPath || draft.probedProvider) { |
There was a problem hiding this comment.
When a user collapses Advanced before the provider probe resolves and immediately reopens it, unmount cleanup only marks the local callback cancelled; it cannot cancel the Tauri probe_backend_provider invocation, and draft.probedProvider is still null, so remounting starts a second provider process that can also run for up to 10 seconds. Fresh evidence relative to the earlier thread is that the new slow-probe test stops after collapsing and never reopens Advanced, so the completed-probe cache does not cover this remaining in-flight path. Keep the in-flight probe owner or cache outside the unmounted disclosure.
Useful? React with 👍 / 👎.
2 participants
Summary
Validation
pnpm -C desktop typecheck