fix(vscode): enforce unfiltered webview typechecks - #12402
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } else { | ||
| const oldFile = local.before | ||
| const newFile = local.after | ||
| if (!oldFile || !newFile) return |
There was a problem hiding this comment.
SUGGESTION: Silent no-op guard skips downstream render-completion work
When metadata is falsy this branch is only reachable for the DiffPair variant, where before/after are required (non-optional) at the type level -- the undefined only shows up because local merges both union members. So this guard should never actually trigger at runtime given valid props, but if it ever does (e.g. a caller bug that supplies neither fileDiff nor before/after), the effect now returns before applyScheme(), patchSeparatorLayout(), setRendered(...), and notifyRendered() run -- silently leaving the diff view in a "stuck" state (e.g. onRendered never firing) with no error surfaced. Consider a dev-time warning/invariant here instead of a bare early return, so a misuse is visible rather than manifesting as a UI that never finishes "rendering".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 00c5da3. The narrowing guard now throws a descriptive invariant error when fileDiff is absent without valid before/after inputs, so malformed props surface immediately instead of silently skipping render-completion work. Direct extension/webview typecheck, lint, and formatting pass.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files changed since previous review)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 00c5da3)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 00c5da3)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file changed since previous review)
Fix these issues in Kilo Cloud Previous review (commit 5cb0382)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Reviewed by claude-sonnet-5 · Input: 42 · Output: 9.2K · Cached: 935.4K Review guidance: REVIEW.md from base branch |
| } else { | ||
| const oldFile = local.before | ||
| const newFile = local.after | ||
| if (!oldFile || !newFile) throw new Error("Diff requires before and after contents when fileDiff is absent") |
There was a problem hiding this comment.
WARNING: Uncaught throw inside createEffect with no ErrorBoundary
This now throws directly inside the render createEffect instead of silently no-oping. That fixes the previous silent-stuck-UI issue, but by this point in the effect container.innerHTML = "" has already run and instance/setCurrent(instance) have already been reassigned, so the throw leaves the DOM cleared with no completed render. More importantly, neither packages/kilo-ui nor the webview usages of Diff wrap this component in a SolidJS ErrorBoundary, so an uncaught error here propagates out of the reactive effect and can crash the surrounding component tree rather than failing just this diff view. Consider catching this at a boundary (or reporting via a callback prop) rather than throwing raw, or wrap Diff usages in an ErrorBoundary.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 39a09ff. Since this branch is only reachable for the DiffPair variant, before and after are guaranteed by DiffProps; the loss of narrowing comes from splitProps over the union. The fix now uses non-null assertions solely to restore that type information. There is no runtime guard, early return, throw, or new error-boundary requirement, and valid rendering behavior remains unchanged. Formatting and whitespace checks pass; fresh CI is running the full typecheck and test suite.
| }) | ||
| } else { | ||
| const oldFile = local.before! | ||
| const newFile = local.after! |
There was a problem hiding this comment.
WARNING: Non-null assertions removed the last runtime guard for malformed Diff props
This replaces the throwing invariant with bare ! assertions and drops the runtime check entirely. The type-level reasoning is sound (local.fileDiff falsy + patchDiff() falsy implies the DiffPair variant, where before/after are required), but it only holds if every caller actually satisfies the discriminated union at runtime. Diff is a shared kilo-ui component consumed across the webview, so a caller bug that supplies neither fileDiff nor before/after is exactly the scenario the original SUGGESTION comment on this line called out.
With the guard gone, that misuse is no longer visible: { ...oldFile, contents, cacheKey } with oldFile undefined silently spreads to nothing (object spread of undefined doesn't throw), producing a oldFile/newFile missing required fields like path, which gets handed to instance.render(...) after container.innerHTML has already been cleared and instance reassigned. That reintroduces a stuck/broken diff view with no error surfaced anywhere — the same failure mode flagged before the throw was added, just without even a console error this time.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Pushing back on this one: the malformed-props behavior is unchanged from main, not introduced by this PR. The base code already executes { ...local.before, ... } / { ...local.after, ... } with no runtime guard. The final patch only assigns those same values to locals with non-null assertions before spreading them, because splitProps loses the DiffProps discriminated-union narrowing. Non-null assertions emit no JavaScript, so the runtime behavior is identical. Adding runtime validation or an error-reporting API would be a separate behavioral change outside this typecheck cleanup. The reviewed commit also passed the complete CI suite, including typecheck and both visual-regression jobs.
chrarnoldus
changed the title
…-typecheck fix(vscode): enforce unfiltered webview typechecks
2 participants
What changed
Run the webview TypeScript compiler directly and remove the custom wrapper that filtered diagnostics. Fix the first-party errors this exposes by:
DiffPairfile-input types lost whensplitPropsprocesses the discriminated unionWhy
The wrapper discarded every diagnostic whose path started with
.., assuming those paths referred to parentnode_modules. In this workspace they also identify source-exporting first-party packages such askilo-uiandui, so genuine compatibility errors in the webview compilation boundary were hidden.The wrapper also inferred success from filtered output instead of propagating the compiler exit status, which could make process failures such as a missing
tscexecutable appear successful. Directtscnow reports all diagnostics and exits normally on compiler failures.The
Diffadjustment is type-only:DiffPropsrequiresbeforeandafterin the pair variant, butsplitPropsloses that narrowing. It does not add runtime guards or change rendering behavior.The final PR head passes the complete CI suite, including typecheck, unit tests, and both visual-regression jobs.