Merge Dev into Integration #30
Conversation
Co-authored-by: opencode-agent[bot] <opencode-agent[bot]@users.noreply.github.com> Co-authored-by: rekram1-node <rekram1-node@users.noreply.github.com> Co-authored-by: Aiden Cline <63023139+rekram1-node@users.noreply.github.com>
Co-authored-by: GitHub Action <action@github.com>
…plying provider priority sorting
WalkthroughVersion bumped from 1.0.83 to 1.0.85 across 18 packages. New provider authentication system added with OAuth and API key support. UI enhanced with provider connection dialog, updated prompts, and refined select styling. Sync context extended with provider metadata. New server endpoints for provider management and OAuth flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TUI<br/>(dialog-provider)
participant Auth as ProviderAuth
participant SDK as Opencode SDK
participant Remote as Provider<br/>(e.g., GitHub)
User->>UI: Select "Connect provider"
UI->>UI: Render provider list
User->>UI: Choose provider & method<br/>(OAuth or API key)
alt OAuth Auto Flow
UI->>Auth: authorize(providerID, method)
Auth->>Remote: Initiate OAuth flow
Remote-->>User: Display auth page
User->>Remote: Authenticate & authorize
Remote-->>Auth: OAuth callback
Auth->>SDK: Store credentials
SDK-->>UI: Success
UI->>UI: Transition to DialogModel
else OAuth Code Flow
UI->>Auth: authorize(providerID, method)
Auth-->>UI: Return auth URL + instructions
UI->>UI: Display instructions & code input
User->>User: Complete external auth
User->>UI: Enter authorization code
UI->>Auth: callback(providerID, method, code)
Auth->>SDK: Store credentials
SDK-->>UI: Success
UI->>UI: Transition to DialogModel
else API Key
UI->>UI: Prompt for API key
User->>UI: Enter API key
UI->>Auth: api(providerID, key)
Auth->>SDK: Store API key
SDK-->>UI: Success
UI->>UI: Transition to DialogModel
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@kcrommett: Hello! I'm here to help. What would you like me to do? Here are some options:
Just let me know what you need! ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| onConfirm={async (value) => { | ||
| if (!value) return | ||
| sdk.client.auth.set({ | ||
| path: { | ||
| id: props.providerID, |
There was a problem hiding this comment.
Await auth save before disposing and refreshing
The API-key connect path invokes sdk.client.auth.set(...) without awaiting the request, then immediately disposes the instance and reloads state. If the server rejects the credentials or the call fails, the promise rejection is unhandled and the UI still proceeds as if the provider were connected. Await the auth request and handle errors before tearing down and bootstrapping so connection state accurately reflects whether the key was accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/opencode/src/session/prompt.ts (1)
52-53: Consider a more targeted approach to suppress AI SDK warnings.Setting a global flag via
globalThisaffects the entire runtime and could hide legitimate warnings elsewhere. The@ts-ignoredirective also bypasses type safety.Consider one of these alternatives:
- Configure the AI SDK properly through its configuration API (if available)
- Suppress warnings at specific call sites where needed
- Use a logging filter to exclude these warnings at the logging layer
If a global suppression is truly necessary, document why this broad approach is required and add a proper TypeScript type declaration instead of
@ts-ignore:-// @ts-ignore -globalThis.AI_SDK_LOG_WARNINGS = false +// Suppress AI SDK warnings globally to prevent noise in logs +// See: https://github.com/vercel/ai/blob/main/packages/ai/src/logger/log-warnings.ts +declare global { + var AI_SDK_LOG_WARNINGS: boolean +} +globalThis.AI_SDK_LOG_WARNINGS = falsepackages/opencode/src/cli/cmd/tui/routes/session/index.tsx (1)
1011-1050: Retry status branch is effectively disabledThe condition
when={props.last && status().type !== "idle" && false}will never be true, so the retry/status UI block is dead code. If this is intentional, consider removing or feature‑flagging the block to avoid confusion; if it’s temporary, a short TODO would help.packages/opencode/src/cli/cmd/tui/app.tsx (1)
463-474: Agent tab UI is permanently disabledWrapping the agent tab block in
<Show when={false}>effectively hides it while keeping the code around. If this is a long‑term disable, consider removing it or gating it behind a real feature flag/config value instead of a literalfalseto reduce dead code.packages/opencode/src/cli/cmd/tui/ui/dialog.tsx (1)
6-7: Selection-aware dialog behavior and copy overlay are reasonable; consider aligning checks with AppThe dialog now (a) avoids closing when
renderer.getSelection()is truthy and (b) adds an overlay inDialogProviderthat mirrors the App-level OSC52 + Clipboard copy behavior for selected text inside dialogs. Functionally this is good, but two small follow-ups would make it more robust:
- In
Dialog, you’re checkingrenderer.getSelection()rather thangetSelection()?.getSelectedText()?.length > 0like in App and the new overlay. IfgetSelection()can be non-null even when empty, this might block background clicks from closing the dialog. Aligning the condition with the text-length check would keep behavior consistent.- The OSC52 + copy + toast logic is now duplicated between App and
DialogProvider. If this grows further, extracting a small helper (or hook) to share the behavior would reduce drift.Also applies to: 17-23, 35-37, 132-134, 137-152
packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx (1)
24-41: Dedupe against recent models by ID instead of deep equalityRight now duplicates are filtered with:
filter((x) => !showRecent() || !local.model.recent().find((y) => isDeepEqual(y, x.value)))This assumes elements from
local.model.recent()are deeply equal tox.value(which is{ providerID, modelID }). If recents ever carry extra metadata (timestamps, labels, etc.),isDeepEqualmay stop matching and you’ll show duplicates.Consider comparing by IDs only, e.g.:
filter( (x) => !showRecent() || !local.model.recent().some( (y) => y.providerID === x.value.providerID && y.modelID === x.value.modelID, ), )This is more robust to type evolution.
Also applies to: 86-87
packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx (1)
9-10: Invokedescriptionaccessor instead of passing the function
descriptionis typed as() => JSX.Elementbut is rendered as{props.description}. In typical Solid patterns, you’d call the accessor so the description actually renders as JSX rather than a function value.A safer pattern:
<box gap={1}> {props.description?.()} <textarea ... /> </box>This avoids accidentally rendering a function or
undefinedand matches how other accessors are used.Also applies to: 41-43
packages/opencode/src/cli/cmd/tui/component/dialog-provider.tsx (1)
37-45: Guard for empty auth-method lists and consider minimal error handlingThe overall provider-connect flow (method selection → oauth/api handling → dispose/bootstrap →
DialogModel) looks solid. A couple of small robustness points:
When
sync.data.provider_auth[provider.id]is present but happens to be an empty array,methods.lengthis0,indexremains0, andconst method = methods[index]yieldsundefined, so neither the oauth nor api branches run. A simple early return would make this safer:const methods = sync.data.provider_auth[provider.id] ?? [{ type: "api", label: "API key" }] if (methods.length === 0) returnIn
AutoMethod,CodeMethod, andApiMethod, SDK calls are awaited but not caught. If the client or network throws (rather than returning{ error }), the dialog could get stuck. Wrapping those blocks in atry/catchthat surfaces a minimal error message (or at least clears the dialog) would make the flow more resilient.These are non-blocking but would harden the UX around provider auth.
Also applies to: 61-85, 111-127, 161-178, 205-219
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (1)
801-852: Retry banner and Loader logic are correct but could be factored for clarityThe bottom status bar—
Loaderplus the inline retry countdown and message—is wired correctly tostatus().typeand usesonMount/onCleanupto manage its intervals safely. It’s quite a bit of reactive logic embedded in JSX, though.For readability and easier testing, consider extracting the retry-specific block (the IIFE that defines
retry,message, andseconds) into a small child component (e.g.,<RetryStatus status={status} />). That would keepPrompt’s render tree simpler without changing behavior.Also applies to: 880-913
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
bun.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lockpackages/sdk/js/src/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (36)
packages/console/app/package.json(1 hunks)packages/console/core/package.json(1 hunks)packages/console/function/package.json(1 hunks)packages/console/mail/package.json(1 hunks)packages/desktop/package.json(1 hunks)packages/extensions/zed/extension.toml(2 hunks)packages/function/package.json(1 hunks)packages/opencode/package.json(1 hunks)packages/opencode/src/cli/cmd/tui/app.tsx(3 hunks)packages/opencode/src/cli/cmd/tui/component/border.tsx(1 hunks)packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx(3 hunks)packages/opencode/src/cli/cmd/tui/component/dialog-provider.tsx(1 hunks)packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx(7 hunks)packages/opencode/src/cli/cmd/tui/context/local.tsx(2 hunks)packages/opencode/src/cli/cmd/tui/context/sdk.tsx(0 hunks)packages/opencode/src/cli/cmd/tui/context/sync.tsx(6 hunks)packages/opencode/src/cli/cmd/tui/routes/session/index.tsx(4 hunks)packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx(3 hunks)packages/opencode/src/cli/cmd/tui/ui/dialog-select.tsx(8 hunks)packages/opencode/src/cli/cmd/tui/ui/dialog.tsx(4 hunks)packages/opencode/src/cli/error.ts(1 hunks)packages/opencode/src/project/instance.ts(1 hunks)packages/opencode/src/project/state.ts(1 hunks)packages/opencode/src/provider/auth.ts(1 hunks)packages/opencode/src/provider/provider.ts(0 hunks)packages/opencode/src/provider/transform.ts(1 hunks)packages/opencode/src/server/server.ts(3 hunks)packages/opencode/src/session/prompt.ts(2 hunks)packages/plugin/package.json(1 hunks)packages/plugin/src/index.ts(1 hunks)packages/sdk/js/package.json(1 hunks)packages/slack/package.json(1 hunks)packages/ui/package.json(1 hunks)packages/util/package.json(1 hunks)packages/web/package.json(1 hunks)sdks/vscode/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/opencode/src/cli/cmd/tui/context/sdk.tsx
- packages/opencode/src/provider/provider.ts
🔇 Additional comments (34)
packages/opencode/package.json (1)
3-3: LGTM! Standard version bump.Version updated to 1.0.85 as part of the coordinated monorepo release.
packages/desktop/package.json (1)
3-3: LGTM! Version aligned with monorepo release.Version updated to 1.0.85, consistent with the coordinated release across all packages.
packages/web/package.json (1)
4-4: LGTM! Version synchronized with release.Version updated to 1.0.85 as part of the monorepo-wide release cycle.
sdks/vscode/package.json (1)
5-5: LGTM! VSCode extension version synchronized.Version updated to 1.0.85, aligned with the monorepo release.
packages/sdk/js/package.json (1)
4-4: LGTM! SDK version updated.Version updated to 1.0.85, consistent with the coordinated monorepo release.
packages/plugin/package.json (1)
4-4: LGTM! Plugin version synchronized.Version updated to 1.0.85, aligned with the monorepo-wide release.
packages/opencode/src/project/state.ts (1)
60-60: Disposal tests do not exist - manual verification required.The disposal logic is architecturally sound: disposal tasks are created with captured references before
entries.clear()is called, ensuring async operations complete safely. However, no test files exist forState.dispose()or the State class in general (search returned zero test files in the project).The review explicitly requests verification that this semantic change is covered by tests. Since no tests currently exist for disposal behavior, you should:
- Confirm the semantic change from deleting a specific entry to clearing all entries is intentional
- Determine if test coverage should be added before merging
- Manually validate the disposal logic handles concurrent state access correctly during the disposal window
packages/slack/package.json (1)
3-3: LGTM! Version bump is consistent with the monorepo release.The version update from 1.0.83 to 1.0.85 aligns with the broader version synchronization across packages in this PR.
packages/console/function/package.json (1)
3-3: LGTM! Version synchronized with monorepo release.packages/opencode/src/project/instance.ts (1)
51-55: LGTM! Cache cleanup correctly added to instance disposal.The addition of
cache.delete(Instance.directory)ensures the cache entry is removed when an instance is disposed directly (not throughdisposeAll). In thedisposeAllflow, these individual deletions are redundant with the finalcache.clear(), but they cause no harm.packages/opencode/src/cli/error.ts (1)
14-16: LGTM! Error message is more actionable.The updated message provides clearer remediation steps by instructing users to rename or remove the invalid directory, rather than suggesting alternative usage.
packages/function/package.json (1)
3-3: LGTM! Version bump aligned with monorepo release.packages/console/core/package.json (1)
4-4: LGTM! Version synchronized with monorepo release.packages/util/package.json (1)
3-3: LGTM! Version bump consistent with the monorepo release.packages/console/mail/package.json (1)
3-3: LGTM! Version synchronized with monorepo release.packages/opencode/src/provider/transform.ts (1)
150-154: Verify the pattern matching for gemini-3 models.The condition uses
modelID.includes("gemini-3")which will match any model ID containing "gemini-3" as a substring (e.g., "gemini-30", "my-gemini-3-custom", "gemini-3.5").If you need exact matching for specific model variants, consider a more precise pattern:
-if (providerID === "google" || (providerID === "opencode" && modelID.includes("gemini-3"))) { +if (providerID === "google" || (providerID === "opencode" && /^gemini-3($|\.)/.test(modelID))) {Or if the current substring matching is intentional to cover all gemini-3 variants, please add a comment explaining this behavior.
packages/ui/package.json (1)
3-3: LGTM!Version bump is consistent with the broader PR pattern of updating multiple packages to 1.0.85.
packages/plugin/src/index.ts (2)
29-136: LGTM!The extraction of authentication types into standalone exports (
AuthHookandAuthOuathResult) improves code organization and makes the authentication contract explicit and reusable. The use of discriminated unions for OAuth vs API methods and auto vs code callbacks is a good design pattern.
144-144: LGTM!The refactor to reference the new
AuthHooktype maintains the same functionality while improving type reusability.packages/opencode/src/server/server.ts (2)
310-330: LGTM!The instance disposal endpoint follows the established patterns with proper OpenAPI documentation and straightforward implementation.
1188-1319: LGTM! Provider management endpoints follow established patterns.The new provider authentication and OAuth endpoints are well-structured with:
- Consistent OpenAPI documentation
- Proper request validation using Zod schemas
- Error handling following existing patterns
- Clear endpoint naming and organization
These endpoints integrate with the new
ProviderAuthmodule to enable provider connection workflows.packages/opencode/src/cli/cmd/tui/component/border.tsx (2)
1-13: LGTM!Introducing
EmptyBorderas a reusable constant is a good refactor that reduces duplication and makes it easier to create custom border configurations.
18-18: LGTM!Using the spread operator with
EmptyBorderreduces duplication and maintains the same functionality while making the code more maintainable.packages/extensions/zed/extension.toml (1)
4-4: Release v1.0.85 artifacts verified and available.The v1.0.85 release has been published with all required platform-specific archives present, including darwin-arm64, darwin-x64, linux-arm64, linux-x64, and windows-x64 variants. The version bump and archive URLs are valid.
packages/opencode/src/cli/cmd/tui/context/local.tsx (1)
93-98: No changes needed—the code's return types are consistent.The
themeproperties (secondary,accent,success,warning,primary,error) are all typed asRGBA. Thecolors()memo returns an array of these RGBA values. Therefore, both return statements in thecolor()function returnRGBAobjects: line 95 viaRGBA.fromHex()and line 97 via element access from thecolors()array. There is no type inconsistency.Likely an incorrect or invalid review comment.
packages/opencode/src/cli/cmd/tui/routes/session/index.tsx (3)
603-606: DialogPrompt.show options usage looks consistent with new APISwitching to
DialogPrompt.show(dialog, "Export filename", { value: … })matches the updated signature and preserves the previous default filename behavior.
907-909: UserMessage layout/hover and file badges look correctThe refactor to wrap content in a hoverable inner box, add
SplitBorder.customBorderChars, per‑MIME badges, and username/QUEUED display is logically sound and doesn’t introduce obvious state or event‑handling issues.Also applies to: 910-923, 924-942, 944-953
1314-1317: Guarding.spliton write tool content avoids runtime errorsThe
createMemothat now checkstypeof props.input.content === "string"before calling.split("\n")prevents crashes when non‑string content slips through and keeps downstream consumers (numbers()) safe.packages/opencode/src/cli/cmd/tui/context/sync.tsx (1)
15-16: Provider bootstrap extensions integrate cleanlyAdding
provider_next/provider_authinto the sync store and wiring them intobootstrap(blockingconfig.providers+provider.list, non‑blockingprovider.auth) looks consistent with the existing pattern, including the guarded"partial"status transition and exposingbootstrapon the public API.Also applies to: 33-35, 63-69, 245-285, 287-289, 343-344
packages/opencode/src/cli/cmd/tui/app.tsx (1)
5-5: Provider connect command wiring looks goodImporting
Showand aliasingDialogProvider as DialogProviderListavoids naming clashes, and the new"Connect provider"command cleanly reuses the existingdialog.replace(() => <Dialog… />)pattern.Also applies to: 9-9, 297-304
packages/opencode/src/cli/cmd/tui/ui/dialog-select.tsx (2)
205-207: Current-option state wiring and padding look correct
currentis memoized per option and consistently passed down toOption, and the conditionalpaddingLeft={current() ? 1 : 3}aligns with using a dedicated current marker insideOption. No functional issues stand out here.Also applies to: 220-231
160-189: Layout/padding and keybind/footer rendering changes are coherentThe increased outer padding, tighter scrollbox padding, category header indent, new ◆ marker, added text padding, and the bolded keybind titles all line up visually and semantically with the select’s state (active/current) without affecting behavior.
Also applies to: 197-201, 221-223, 240-248, 274-275, 283-287
packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx (1)
15-21: Connected/showRecent/keybind behavior is consistentUsing
connected()to gateshowRecent(), the “Popular providers” block, and the keybind title (“Connect provider” vs “More providers”) is internally consistent, and the flow intoDialogProviderviadialog.replacelooks correct.Also applies to: 91-103, 106-116
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (1)
52-57: Status-aware highlight and header integration look goodDeriving
statusfromsync.data.session_statusand usinghighlightto drive the prompt border, agent/shell label, and global cursor color ties the visual state cleanly to session/agent state. The shell-mode toggling and agent/model header rendering all look consistent with that design.Also applies to: 542-550, 576-591, 766-778
| "build": "./script/generate-sitemap.ts && vinxi build && ../../opencode/script/schema.ts ./.output/public/config.json", | ||
| "start": "vinxi start", | ||
| "version": "1.0.83" | ||
| "version": "1.0.85" |
There was a problem hiding this comment.
🧩 Analysis chain
Version updated, but placement is unusual.
The version is set to 1.0.85 within the scripts section rather than at the root level of package.json. This is non-standard - typically the version field should be a top-level property. Verify this is intentional for this console app package.
Run the following script to check if this is a pattern used elsewhere or an anomaly:
🏁 Script executed:
#!/bin/bash
# Check for version field placement across package.json files
echo "=== Checking version field locations in package.json files ==="
fd -t f '^package\.json$' -x sh -c 'echo "File: {}"; cat {} | jq -r "if .scripts.version then \" Version in scripts: \" + .scripts.version else \" Version at root: \" + (.version // \"none\") end"'Length of output: 1194
Move version field to root level in packages/console/app/package.json.
Verification confirms this is an anomaly—all other 17 package.json files in the codebase place the version field at the root level. The version field should be moved from the scripts section to the package root to maintain consistency with the established project convention.
🤖 Prompt for AI Agents
In packages/console/app/package.json around line 10, the "version": "1.0.85"
entry is incorrectly located inside the "scripts" section; move that "version"
field out of "scripts" and place it at the top-level of the package.json (same
level as "name", "private" etc.), then remove the version key from the scripts
object so scripts contains only script entries and the package root contains the
version.
| DialogPrompt.show = (dialog: DialogContext, title: string, options?: Omit<DialogPromptProps, "title">) => { | ||
| return new Promise<string | null>((resolve) => { | ||
| dialog.replace( | ||
| () => ( | ||
| <DialogPrompt | ||
| title={title} | ||
| value={value} | ||
| onConfirm={(value) => resolve(value)} | ||
| onCancel={() => resolve(null)} | ||
| /> | ||
| <DialogPrompt title={title} {...options} onConfirm={(value) => resolve(value)} onCancel={() => resolve(null)} /> | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
DialogPrompt.show ignores caller onConfirm/onCancel and never closes the dialog
Two concerns here:
-
Caller handlers are silently dropped
optionsis typed asOmit<DialogPromptProps, "title">, which includesonConfirmandonCancel, but they are never invoked. You overwrite them with:<DialogPrompt title={title} {...options} onConfirm={(value) => resolve(value)} onCancel={() => resolve(null)} />
Any
options.onConfirm/options.onCancelprovided by callers will be ignored, which is surprising given the type. -
The dialog is not dismissed on confirm/cancel
onConfirm/onCancelonly resolve the Promise; they do not calldialog.clear()or otherwise close the dialog. Unless every call site now explicitly closes the dialog afterawait DialogPrompt.show(...), existing usages will leave the prompt onscreen after “confirm”.
If the intent is to keep the old “show-and-close” semantics while allowing extra options, consider:
DialogPrompt.show = (dialog: DialogContext, title: string, options: Omit<DialogPromptProps, "title"> = {}) => {
return new Promise<string | null>((resolve) => {
const handleConfirm = (value: string) => {
options.onConfirm?.(value)
resolve(value)
dialog.clear()
}
const handleCancel = () => {
options.onCancel?.()
resolve(null)
dialog.clear()
}
dialog.replace(
() => (
<DialogPrompt
title={title}
{...options}
onConfirm={handleConfirm}
onCancel={handleCancel}
/>
),
handleCancel,
)
})
}Alternatively, if callers are now responsible for closing the dialog, it’d be less misleading to remove onConfirm/onCancel from the options type to avoid suggesting they’ll be honored.
🏁 Script executed:
#!/bin/bash
# Audit all call sites of DialogPrompt.show to ensure they still behave correctly
# after the semantic change (no auto-close, handlers ignored).
rg -n "DialogPrompt\.show\(" -SLength of output: 148
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the call site context at line 604
cat packages/opencode/src/cli/cmd/tui/routes/session/index.tsx | sed -n '600,620p'
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Find the DialogPrompt implementation
find . -name "dialog-prompt.tsx" -type fLength of output: 56
🏁 Script executed:
#!/bin/bash
# Get the full implementation of DialogPrompt.show and surrounding code
cat packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx | sed -n '50,90p'Length of output: 798
🏁 Script executed:
#!/bin/bash
# Search for dialog.clear() calls to understand dialog lifecycle
rg -n "dialog\.clear\(\)" -A 2 -B 2Length of output: 21311
---
🏁 Script executed:
```shell
#!/bin/bash
# Find DialogPromptProps type definition
cat packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx | head -50
Length of output: 1442
🏁 Script executed:
#!/bin/bash
# Examine broader context around the call at line 604
# to see if dialog.clear() is called after the await
cat packages/opencode/src/cli/cmd/tui/routes/session/index.tsx | sed -n '600,640p'Length of output: 1348
🏁 Script executed:
#!/bin/bash
# Search for any cleanup after DialogPrompt.show() call
rg -n "DialogPrompt\.show\(" -A 20 -B 2Length of output: 2268
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if there are any other usages of DialogPrompt (not .show method)
# to understand if the component is used elsewhere
rg -n "DialogPrompt" packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx -A 5 -B 5 | head -100
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check git log to see if DialogPrompt.show was recently added or modified
cd packages/opencode && git log --oneline -20 --follow -- src/cli/cmd/tui/ui/dialog-prompt.tsx 2>/dev/null || echo "Git history not available"Length of output: 150
DialogPrompt.show has misleading API: handlers are ignored and dialog is not auto-closed
Verification confirms both concerns:
-
Handlers are silently dropped
DialogPromptPropsincludes optionalonConfirmandonCancel, butDialogPrompt.showunconditionally overwrites them (line 70). Any caller-provided handlers are ignored, making the type signature misleading. -
Dialog lacks auto-close semantics
UnlikeDialogConfirmandDialogAlertwhich calldialog.clear()inside their handlers,DialogPrompt.showdoes not. The only call site (line 604) survives because it explicitly callsdialog.clear()at line 629, but this is fragile and inconsistent with the established dialog pattern.
The suggested fix (invoke options handlers before resolving, and call dialog.clear()) aligns the semantics with other dialog components and removes the misleading type contract.
🤖 Prompt for AI Agents
In packages/opencode/src/cli/cmd/tui/ui/dialog-prompt.tsx around lines 66 to 71,
DialogPrompt.show currently overwrites any caller-provided onConfirm/onCancel
handlers and never clears the dialog; modify the implementation so it calls the
provided handlers (if present) before resolving and then calls dialog.clear() to
close the dialog, i.e., invoke options.onConfirm(value) when confirming and
options.onCancel() when cancelling (safely guarded), then call dialog.clear()
and finally resolve the promise with the value (or null).
| export namespace ProviderAuth { | ||
| const state = Instance.state(async () => { | ||
| const methods = pipe( | ||
| await Plugin.list(), | ||
| filter((x) => x.auth?.provider !== undefined), | ||
| map((x) => [x.auth!.provider, x.auth!] as const), | ||
| fromEntries(), | ||
| ) | ||
| return { methods, pending: {} as Record<string, AuthOuathResult> } | ||
| }) | ||
|
|
||
| export const Method = z | ||
| .object({ | ||
| type: z.union([z.literal("oauth"), z.literal("api")]), | ||
| label: z.string(), | ||
| }) | ||
| .meta({ | ||
| ref: "ProviderAuthMethod", | ||
| }) | ||
| export type Method = z.infer<typeof Method> | ||
|
|
||
| export async function methods() { | ||
| const s = await state().then((x) => x.methods) | ||
| return mapValues(s, (x) => | ||
| x.methods.map( | ||
| (y): Method => ({ | ||
| type: y.type, | ||
| label: y.label, | ||
| }), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Harden provider auth against invalid input and clean up pending state
Two areas are fragile here:
- authorize(): unchecked provider/method lookups
auth is obtained as s.methods[input.providerID] and const method = auth.methods[input.method] without guards. For an unknown providerID or an out‑of‑range method index, this will throw a generic TypeError instead of returning a structured error.
Consider validating both lookups and returning a named error (or at least a controlled failure) instead of crashing, e.g.:
export const authorize = fn(
@@
- async (input): Promise<Authorization | undefined> => {
- const auth = await state().then((s) => s.methods[input.providerID])
- const method = auth.methods[input.method]
- if (method.type === "oauth") {
+ async (input): Promise<Authorization | undefined> => {
+ const s = await state()
+ const auth = s.methods[input.providerID]
+ if (!auth) {
+ throw new OauthMissing({ providerID: input.providerID })
+ }
+ const method = auth.methods[input.method]
+ if (!method) {
+ throw new OauthMissing({ providerID: input.providerID })
+ }
+ if (method.type === "oauth") {
const result = await method.authorize()
- await state().then((s) => (s.pending[input.providerID] = result))
+ s.pending[input.providerID] = result
return {
url: result.url,
method: result.method,
instructions: result.instructions,
}
}
},
)- callback(): pending entries are never cleared
After a callback (success or failure), pending[input.providerID] remains set. That allows reusing the same pending entry across multiple callbacks and keeps unnecessary state around.
You can ensure single‑use semantics and avoid stale state by deleting the entry in a finally block:
export const callback = fn(
@@
- async (input) => {
- const match = await state().then((s) => s.pending[input.providerID])
- if (!match) throw new OauthMissing({ providerID: input.providerID })
- let result
-
- if (match.method === "code") {
- if (!input.code) throw new OauthCodeMissing({ providerID: input.providerID })
- result = await match.callback(input.code)
- }
-
- if (match.method === "auto") {
- result = await match.callback()
- }
-
- if (result?.type === "success") {
- if ("key" in result) {
- await Auth.set(input.providerID, {
- type: "api",
- key: result.key,
- })
- }
- if ("refresh" in result) {
- await Auth.set(input.providerID, {
- type: "oauth",
- access: result.access,
- refresh: result.refresh,
- expires: result.expires,
- })
- }
- return
- }
-
- throw new OauthCallbackFailed({})
- },
+ async (input) => {
+ const s = await state()
+ const match = s.pending[input.providerID]
+ if (!match) throw new OauthMissing({ providerID: input.providerID })
+ let result
+
+ try {
+ if (match.method === "code") {
+ if (!input.code) throw new OauthCodeMissing({ providerID: input.providerID })
+ result = await match.callback(input.code)
+ }
+
+ if (match.method === "auto") {
+ result = await match.callback()
+ }
+
+ if (result?.type === "success") {
+ if ("key" in result) {
+ await Auth.set(input.providerID, {
+ type: "api",
+ key: result.key,
+ })
+ }
+ if ("refresh" in result) {
+ await Auth.set(input.providerID, {
+ type: "oauth",
+ access: result.access,
+ refresh: result.refresh,
+ expires: result.expires,
+ })
+ }
+ return
+ }
+
+ throw new OauthCallbackFailed({})
+ } finally {
+ delete s.pending[input.providerID]
+ }
+ },
)These changes make the API more robust against bad inputs and keep the in‑memory auth state bounded and easier to reason about.
Also applies to: 43-53, 54-114, 116-127, 129-143
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.