Repository navigation
Conversation
- Opening or cancelling a form reports when entered values could not be cleared, instead of ignoring it. - Failure text comes from one helper, so a CLI failure that answers with an error field shows that reason; a form without variants loads as having none; the workspace picker drops a redundant wrapper. - README: when signed out, Connect and Connect & sync are hidden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const data = parseJson<T>(result.stdout) | ||
| const failure = result.exitCode === 0 ? null : (data?.message ?? firstLine(result.stderr.replace(ANSI, '')) ?? 'failed') | ||
| return { data, failure } | ||
| return { data, failure: result.exitCode === 0 ? null : failureText(result) } |
There was a problem hiding this comment.
[SUGGESTION]: failureText re-parses the same stdout
runJson already parsed result.stdout into data, but failureText(result) parses it again (line 89) on every failed call, and listWorkspaces then parses it once more before calling failureText. Passing the parsed data into the helper would keep the single source of failure text without the repeated JSON.parse.
There was a problem hiding this comment.
Not changing this. The second parse happens only on the failure path (a non-zero exit), and the payload is a small JSON object, so the cost is negligible. Passing parsed data into failureText would widen its signature for every caller, including the sync path, which reads stdout differently, and keeping one helper that takes the raw result is simpler.
| await update($, form, () => null) | ||
| await update($, typed, () => ({})) | ||
| await update($, formMessage, () => note) | ||
| await update($, formMessage, () => [failure && `Could not clear earlier input: ${failure}`, note].filter(Boolean).join(' · ') || null) |
There was a problem hiding this comment.
[SUGGESTION]: Discard warning is lost when the fields read also fails
loadForm overwrites formMessage on failure (line 279), so when the discard above failed and fields fails, this "Could not clear earlier input" warning is replaced and never shown - the exact case this PR aims to surface. Consider setting this message after loadForm, or appending to the existing formMessage instead of overwriting it.
There was a problem hiding this comment.
Fixed in 0df2c6c. loadForm now adds its failure next to the existing formMessage through joinNotes instead of replacing it, so both notes show (Could not clear earlier input: store locked · catalog unavailable). Every loadForm caller sets or clears formMessage first, so notes don't pile up.
| try { | ||
| const { data, failure } = await runJson<Form & { message?: string }>($, ['fields', open.provider, ...(open.variant ? ['--variant', open.variant] : []), '--json']) | ||
| if (data && Array.isArray(data.fields)) await update($, form, () => data) | ||
| if (data && Array.isArray(data.fields)) await update($, form, () => ({ ...data, variants: Array.isArray(data.variants) ? data.variants : [] })) |
There was a problem hiding this comment.
[SUGGESTION]: The new no-variants fallback has no test
The normalisation here prevents fields.variants.length (line 751) from throwing when the CLI omits variants, but the added test only exercises the error field and discard paths. A case whose fields reply omits variants would lock this behaviour in.
There was a problem hiding this comment.
Added in 0df2c6c: a form without variants loads, and a refused connect that answers with an error field shows that reason. Its fields reply has no variants, and the test asserts the form renders. It fails on the pre-fix code.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Incremental review of Previous Review Summary (commit 8ec4eb7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8ec4eb7)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
The core changes are sound: Reviewed by deepseek-v4.1-flash-1017 · Input: 41.4K · Output: 15.7K · Cached: 569.3K |
hkaltai
left a comment
hkaltai
left a comment
There was a problem hiding this comment.
Peer review (lead + Codex Sol 6.1, converged in 1 round): no blocking issues. failureText is a strict superset of the old inline expression, the variants normalisation and picker Box removal are safe, and the README matches the render guards.
Three MINORs are inline.
Nits (consolidated):
integrations.tsx:304-311: when the discard fails on open, the form still opens over the values that weren't cleared.withProgressclears the warning on the next form action, and Connect then uses those values. That's an observation, not a required fix; refusing to open or keeping the warning visible would be stricter.- Scope can't be verified against a ticket: the PR links #12 (a PR) and no Jira or issue. Consider attaching one.
(Approval would go here, but GitHub doesn't allow approving your own PR.)
| await update($, form, () => null) | ||
| await update($, typed, () => ({})) | ||
| await update($, formMessage, () => note) | ||
| await update($, formMessage, () => [failure && `Could not clear earlier input: ${failure}`, note].filter(Boolean).join(' · ') || null) |
There was a problem hiding this comment.
MINOR (bug): the open-time discard warning is overwritten when the fields load fails.
await update($, formMessage, () => [failure && `Could not clear earlier input: ${failure}`, note].filter(Boolean).join(' · ') || null)
await update($, message, () => null)
await loadForm($, open)loadForm's failure branch (else await update($, formMessage, () => failure ?? 'Could not read the credential fields.')) replaces formMessage. So when both discard and fields fail, the user never sees that earlier input could not be cleared. The new test hits exactly this case (discard → store locked, fields → catalog unavailable) and shows only the second.
Suggestion: have loadForm compose onto the existing value, e.g. update($, formMessage, current => [current, failure ?? '…'].filter(Boolean).join(' · ')), or keep the discard warning in its own atom.
There was a problem hiding this comment.
Fixed in 0df2c6c (joinNotes in loadForm), as in the reply above. The test now asserts the combined note on open.
| const data = parseJson<T>(result.stdout) | ||
| const failure = result.exitCode === 0 ? null : (data?.message ?? firstLine(result.stderr.replace(ANSI, '')) ?? 'failed') | ||
| return { data, failure } | ||
| return { data, failure: result.exitCode === 0 ? null : failureText(result) } |
There was a problem hiding this comment.
MINOR (logic): failureText now reads error, but submitForm never reaches it when the CLI returns JSON:
outcome = run.data ?? { status: 'failed', message: run.failure }(integrations.tsx:429). For { "error": "backend unavailable" } with exit 1, run.data is non-null and has no message, so the form shows Jira could not be connected. and drops the reason. The gap predates this PR, but it means the claim that "a CLI failure that answers with an error field shows that reason" doesn't hold for Connect / Update / Keep local. connectFound handles it already (data?.message ?? failure).
Suggestion: outcome = run.data ? { ...run.data, message: run.data.message ?? run.failure } : { status: 'failed', message: run.failure }.
There was a problem hiding this comment.
Fixed in 0df2c6c: submitForm now falls back to run.failure when the JSON has no message. The follow-up review caught that this made a reason-less failure show the bare failed sentinel. 0e13059 maps 'failed' to the friendly <name> could not be connected., the same way listWorkspaces and linkWorkspace do, and adds a test. refresh has its own copy of this logic that ignores error. That copy predates this PR, so it is listed as a follow-up in the description.
| const ui = await $.ui.mount({ ...PANE, surface: 'terminal' }) | ||
| await ui.press({ key: 'jira-add' }) | ||
| await clock.settle() | ||
| expect(await ui.find({ type: 'Text', text: /catalog unavailable/ })).toBeDefined() |
There was a problem hiding this comment.
MINOR (testing): this asserts only the fields error. The open-path discard warning added in openForm has no assertion, and in this fixture it is in fact overwritten (see the comment on openForm).
expect(await ui.find({ type: 'Text', text: /catalog unavailable/ })).toBeDefined()Suggestion: add a case where discard fails and fields succeeds, and assert /Could not clear earlier input: store locked/ after jira-add. Also add a fields reply without variants to cover the loadForm normalisation.
…fused connect shows the CLI's error field Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Follow-up to #12 (merged), addressing its review comments. The plugin's integrations pane:
failureText), so a CLI failure that answers with anerrorfield shows that reason too. The workspace list and link paths compute it once.fieldsoutput has novariantsloads as having none.errorreason, and falls back to " could not be connected." when the CLI gives no reason.Not included: clearing the open form and workspace picker at
session.start. That hook also fires on a plugin reload or worker respawn, so it would close a form you're in the middle of and drop what you typed. New sessions already start with empty state.Test plan
claude plugin validate plugins/altimate-codepasses, andtscis clean.claude plugin test plugins/altimate-code: 160 pass, 0 fail. New tests cover: failed discard reported on open (next to a fields failure) and on cancel; a form without variants; a refused connect with anerrorfield; a refused connect with no reason. Each fails on the code before its fix.tests/*.sh, and pytest with 65 passed.Follow-ups (pre-existing, not changed here)
refreshbuilds its own failure text and ignores anerrorfield (integrations.tsx:106). UsefailureTextthere.failedsentinel can still reach the user fromconnectFound,fields,set,set-secret,open-token-pageanddiscardwhen the CLI gives no reason and stderr is empty.messagefromdiscardwith a non-zero exit is treated as success bydiscardForAction. This hasn't been seen from the CLI.🤖 Generated with Claude Code