Conversation
WalkthroughRemoves silencing fallbacks in CI/publish workflows and consolidates NPM publishing. Refactors client/host webview IPC: introduces a new WebviewProvider, stricter runtime type guards, renamed/updated types, and revised logging types and sanitization. Adjusts host registration and message handling signatures. Adds utilities (generateId, getErrorMessage) and disallowedLogKeys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as React Component
participant WP as WebviewProvider
participant VS as vscodeApi (webview)
participant Host as Extension Host
note over WP: Builds proxy API + listeners<br/>Maintains pending requests (DeferredPromise)
UI->>WP: api.someMethod(params)
activate WP
WP->>WP: generateId("req")
WP->>VS: postMessage({type:"request", id, key:"someMethod", params, context})
Note right of VS: Asynchronous message to host
VS-->>Host: message
deactivate WP
Host->>Host: handle request, compute result or error
Host-->>VS: postMessage({type:"response" | "error", id, value})
VS-->>WP: message
activate WP
alt response
WP->>WP: resolve pending[id]
else error
WP->>WP: reject pending[id]
end
WP-->>UI: Promise resolves/rejects
deactivate WP
Note over WP,Host: Events
Host-->>VS: {type:"event", key, value}
VS-->>WP: message
WP->>WP: notify listeners for key
sequenceDiagram
autonumber
participant Ext as Extension
participant WAP as WebviewApiProvider
participant View as vscode.WebviewView
Ext->>WAP: registerView(viewId, View)
WAP->>WAP: if duplicate -> warn/return
WAP->>WAP: context = {viewId, viewType: View.viewType, sessionId: generateId("session")}
WAP->>View: onDidDispose(register cleanup)
Note over WAP: Dispatch loop on host -> webview
Ext->>WAP: dispatch({type, ...})
loop for each connected view
WAP->>View.webview: postMessage(message)
alt postMessage fails
WAP->>WAP: mark failed
end
end
WAP->>WAP: remove failed views (if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis PR restructures and streamlines the Webview client and host integration by extracting utilities, tightening type safety, and improving logging, while also simplifying CI/CD workflows for release and testing. ER diagram for updated WebviewContextData and related typeserDiagram
WebviewContextData {
string layout
string extensionUri
string logoUris
string title
string description
string version
string homepage
string repository
string author
string license
string publisher
string theme
string color
string background
string accent
string icon
string banner
string screenshot
string documentation
string changelog
string support
string feedback
string privacy
string terms
string analytics
string telemetry
string errorReporting
string crashReporting
string update
string install
string uninstall
string activate
string deactivate
string reload
string reset
string migrate
string backup
string restore
string import
string export
string sync
string settings
string preferences
string configuration
string environment
string platform
string os
string arch
string cpu
string memory
string disk
string network
string proxy
string firewall
string vpn
string cloud
string server
string database
string cache
string queue
string storage
string bucket
string container
string registry
string secret
string token
string apiKey
string apiSecret
string password
string content
}
WebviewContextData ||--o| VsCodeApi : uses
Class diagram for updated Logger and logging typesclassDiagram
class LoggerImpl {
+debug(message: string, data?: Record<string, unknown>)
+info(message: string, data?: Record<string, unknown>)
+warn(message: string, data?: Record<string, unknown>)
+error(message: string, data?: Record<string, unknown>)
+dispose()
-log(level: LogLevel, message: string, data?: Record<string, unknown>)
-outputChannel: vscode.OutputChannel
}
class Logger {
+debug(message: string, data?)
+info(message: string, data?)
+warn(message: string, data?)
+error(message: string, data?)
+dispose()
}
class getLogger {
+debug(message: string, data?)
+info(message: string, data?)
+warn(message: string, data?)
+error(message: string, data?)
+dispose()
}
class LogMessage {
type: 'log'
level: LogLevel
message: string
data?: Record<string, unknown>
}
LoggerImpl <|-- Logger
LoggerImpl <|-- getLogger
LogMessage --> LogLevel
Class diagram for updated WebviewApiProvider and BaseWebviewViewProviderclassDiagram
class WebviewApiProvider {
+connectedViews: Map<WebviewKey, ConnectedView>
+registerView(id: WebviewKey, view: vscode.WebviewView)
+sendEvent(key, event)
+dispose()
-logger
-disposables: vscode.Disposable[]
}
class ConnectedView {
view: vscode.WebviewView
context: RequestContext
}
class BaseWebviewViewProvider {
+_view?: vscode.WebviewView
+logger
+webviewDisposable?: vscode.Disposable
+webviewActionDelegate: ActionDelegate<A>
+generateWebviewHtml(webview, extensionUri, context: WebviewContextData): string
+handleMessage(message: unknown, webview: vscode.Webview): Promise<void>
+handleLogMessage(message: LogMessage): void
+onWebviewDispose()
}
WebviewApiProvider --> ConnectedView
BaseWebviewViewProvider --> LogMessage
BaseWebviewViewProvider --> WebviewContextData
BaseWebviewViewProvider --> vscode.WebviewView
Class diagram for updated type guards and utility functionsclassDiagram
class isViewApiRequest {
+isViewApiRequest(message: unknown): message is ViewApiRequest
}
class isViewApiResponse {
+isViewApiResponse(message: unknown): message is ViewApiResponse
}
class isViewApiError {
+isViewApiError(message: unknown): message is ViewApiError
}
class isViewApiEvent {
+isViewApiEvent(message: unknown): message is ViewApiEvent
}
class isLogMessage {
+isLogMessage(value: unknown): value is LogMessage
}
class generateId {
+generateId(prefix: string): string
}
class getErrorMessage {
+getErrorMessage(error: unknown): string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @hbmartin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request undertakes a significant refactoring of the webview API client, aiming to improve the structure, type safety, and maintainability of the communication between the extension host and webviews. It centralizes common logic, strengthens type checks for message passing, and enhances logging capabilities to better manage sensitive data and error reporting.
Highlights
- Webview API Client Refactoring: The core webview API client logic has been extracted from
WebviewContext.tsxinto new, dedicated files (WebviewProvider.tsx,hooks.tsx,types.ts). This significantly improves modularity, separation of concerns, and maintainability of the webview communication layer. - Enhanced Type Safety and Error Handling: Type guards for API messages (
isViewApiRequest,isViewApiResponse, etc.) have been made more robust with explicitunknowntype checks andinoperator usage. TheVsCodeApiinterface now usesunknownfor message types, and error handling in theWebviewApiProviderhas been standardized using a newgetErrorMessageutility. - Improved Logging Mechanisms: A new
disallowedLogKeysarray has been introduced to filter sensitive information from logs. TheremovePromptsFromDatafunction now recursively cleans log data, handling arrays and objects more effectively, and the logger gracefully handles unserializable data. Log message handling from webviews is now centralized inBaseWebviewViewProvider. - ESLint Configuration Updates: The ESLint configuration has been updated to use
projectService: trueforparserOptionsand to disable theunicorn/require-post-message-target-originrule, streamlining the linting process. - New Utility Functions: New utility functions
generateId(for unique ID generation with prefixes) andgetErrorMessage(for consistent error message extraction) have been added tosrc/lib/utils.ts, promoting code reuse and consistency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The updated GitHub publish workflow removes the manual dispatch inputs and associated steps—confirm this change matches your intended release process or reintroduce any missing manual release paths.
- The isViewApi* type guards contain large blocks of repeated property checks—consider extracting a shared helper to reduce duplication and improve maintainability.
- DeferredPromise defines a timeoutHandle but never initializes it—either implement timeout logic for unanswered requests or remove the unused field to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated GitHub publish workflow removes the manual dispatch inputs and associated steps—confirm this change matches your intended release process or reintroduce any missing manual release paths.
- The isViewApi* type guards contain large blocks of repeated property checks—consider extracting a shared helper to reduce duplication and improve maintainability.
- DeferredPromise defines a timeoutHandle but never initializes it—either implement timeout logic for unanswered requests or remove the unused field to avoid confusion.
## Individual Comments
### Comment 1
<location> `src/lib/host/logger.ts:5` </location>
<code_context>
-function removePromptsFromData<T extends Record<any, any>>(
- dictionary: T | undefined | null
-): T | undefined {
+function removePromptsFromData<T>(dictionary: T | undefined | null): T | undefined {
if (dictionary === null || dictionary === undefined) {
return undefined;
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing manual recursion and cloning with JSON.stringify and a replacer function to simplify log data filtering.
```markdown
You can collapse all of that recursion + cloning into a single `JSON.stringify` + replacer-run, and avoid any manual traversals/try–catches. For example, drop `removePromptsFromData` entirely and inline:
```ts
// helper replacer that drops any disallowed key
const replacer = (key: string, value: unknown) =>
disallowedLogKeys.includes(key) ? undefined : value;
// in your log method:
private static log(level: LogLevel, message: string, data?: unknown) {
const timestamp = new Date().toISOString().split('T')[1];
const levelStr = LogLevel[level] ?? 'UNKNOWN';
const header = `[${timestamp}] [${levelStr}] ${message}`;
if (!data) {
return this.outputChannel.appendLine(header);
}
// stringify with replacer to drop bad keys:
let body: string;
try {
body = JSON.stringify(data, replacer);
} catch {
body = '"<unserializable data>"';
}
this.outputChannel.appendLine(`${header} : ${body}`);
}
```
This:
- Eliminates `structuredClone` & manual recursion entirely
- Automatically prunes any key in `disallowedLogKeys`
- Handles arrays/objects/scalars in one shot via the built-in JSON machinery
- Keeps exactly the same output format and error-fallback behavior
```
</issue_to_address>
### Comment 2
<location> `src/lib/client/WebviewProvider.tsx:78` </location>
<code_context>
- /**
- * Create typed API object using Proxy for dynamic method access
- */
- const api = new Proxy({} as WebviewContextValue['api'], {
- get: (_, key: string) => {
- return (...args: any[]) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the Proxy-based API with a factory function that statically maps each method to callApi for improved simplicity and analyzability.
Consider dropping the Proxy in favor of a small factory that maps each known key to `callApi`. This is still fully typed but much simpler and statically analyzable. For example:
```ts
// 1) Define a typed helper to build your API surface:
type ApiFromClient<C> = {
[K in keyof C]: (...args: Parameters<C[K]>) => ReturnType<C[K]>;
};
function createApi<C extends Record<string, any>>(
callApi: <K extends keyof C>(key: K, ...args: Parameters<C[K]>) => ReturnType<C[K]>
): ApiFromClient<C> {
const api = {} as ApiFromClient<C>;
(Object.keys(api) as Array<keyof C>).forEach((key) => {
api[key] = ((...args: any[]) => callApi(key, ...args)) as any;
});
return api;
}
// 2) Replace your Proxy with a one-liner:
const api = useMemo(
() => createApi<ClientCalls>(callApi),
[callApi]
);
```
If you prefer to list methods explicitly, you can even do:
```ts
const api: ApiFromClient<ClientCalls> = {
getData: (...p) => callApi('getData', ...p),
updateItem: (...p) => callApi('updateItem', ...p),
// …etc for each key in ClientCalls
};
```
Both approaches eliminate the runtime indirection of a Proxy while preserving full type safety.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the webview client-side code. The separation of concerns into different files (WebviewProvider, hooks, types, utils) greatly improves the code structure and maintainability. The extensive work on improving TypeScript type safety by replacing any with unknown, strengthening type guards, and using branded types is commendable. The enhancements to the logging system, especially the recursive data sanitization, are a great addition for security and debugging.
I've found one high-severity issue related to incorrect disposable handling that should be addressed. I've also included a medium-severity suggestion to further improve type safety by replacing more instances of any with unknown. Overall, this is a high-quality contribution that modernizes and hardens the codebase.
| const pendingRequests = useRef<Map<string, DeferredPromise<any>>>(new Map()); | ||
| const listeners = useRef<Map<keyof HostCalls, Set<(...args: any[]) => void>>>(new Map()); |
There was a problem hiding this comment.
To improve type safety and avoid disabling no-explicit-any for the whole file, consider replacing any with unknown. For instance, DeferredPromise<any> can be DeferredPromise<unknown>, and (...args: any[]) can be (...args: unknown[]). This makes the code more robust by requiring explicit type checks or assertions before using these values.
| const pendingRequests = useRef<Map<string, DeferredPromise<any>>>(new Map()); | |
| const listeners = useRef<Map<keyof HostCalls, Set<(...args: any[]) => void>>>(new Map()); | |
| const pendingRequests = useRef<Map<string, DeferredPromise<unknown>>>(new Map()); | |
| const listeners = useRef<Map<keyof HostCalls, Set<(...args: unknown[]) => void>>>(new Map()); |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to d369b55 in 3 minutes and 7 seconds. Click for details.
- Reviewed
1433lines of code in17files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yml:37
- Draft comment:
The fallback echo for test failures was removed. Without it, missing or misconfigured tests might fail silently. Confirm this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The original fallback was specifically for when no tests were configured. Now that it's removed, the command will fail naturally if tests fail, which is actually better - we want the CI to fail if tests fail. The 2>/dev/null just suppresses stderr output but doesn't affect the exit code. The comment's suggestion to echo "Tests failed" wouldn't add any value since GitHub Actions already shows when a step fails. Maybe there's a specific reason they had the fallback that I'm not aware of. The change could break existing workflows that depend on the command never failing. The original fallback specifically mentioned "No tests configured yet" which suggests it was temporary. In a CI pipeline, we actually want test failures to fail the build, so removing the fallback is likely an intentional improvement. The comment should be deleted. Removing the fallback is likely intentional and beneficial, as it ensures test failures properly fail the CI run.
2. .github/workflows/publish.yml:7
- Draft comment:
Manual trigger inputs and release PR creation steps were removed. Confirm that relying solely on release events is sufficient for your workflow. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm if relying solely on release events is sufficient for their workflow. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior is intended.
3. eslint.config.js:129
- Draft comment:
Disabling the 'unicorn/require-post-message-target-origin' rule could reduce security if your app handles cross-origin messaging. Consider the security implications. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. src/lib/client/WebviewProvider.tsx:80
- Draft comment:
The dynamic proxy uses a broad type assertion when calling 'callApi'. Ensure that all ClientCalls methods are handled correctly to avoid runtime type mismatches. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all ClientCalls methods are handled correctly, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. src/lib/host/BaseWebviewViewProvider.ts:29
- Draft comment:
Using 'providerId.split('.')[1]' assumes the providerId contains a dot. If the format changes, the logger tag might be undefined. Consider a safer extraction method. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/lib/host/WebviewLogger.ts:33
- Draft comment:
The methods here use 'Record<any, any>' for the data parameter, but ILogger expects 'Record<string, unknown>'. Updating this to maintain consistency would improve type safety. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_sCSWmU7fwiWX1Bao
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/host/WebviewLogger.ts (1)
33-40: Fix no-floating-promises on postMessage.ESLint config enforces @typescript-eslint/no-floating-promises. Explicitly discard the Thenable.
- this.vscode.postMessage({ + void this.vscode.postMessage({ type: 'log', level: LogLevel.DEBUG, message: `[${this.tag}] ${message}`, data, } satisfies LogMessage);Repeat for info/warn/error below.
src/lib/host/WebviewApiProvider.ts (1)
34-36: Asynchronous failure handling doesn’t prune views; remove on error immediately.Failures pushed inside
.then(..., onRejected)are processed after the prune block runs, so failed views won’t be removed. Also,failedViewsshould beWebviewKey[]if kept. Prefer removing on failure inside the handler and in the sync catch.- // Track views that fail to receive messages - const failedViews: string[] = []; - - // Send to all connected views - for (const [viewId, connectedView] of this.connectedViews.entries()) { + // Send to all connected views + for (const [viewId, connectedView] of this.connectedViews.entries()) { // eslint-disable-next-line sonarjs/no-try-promise try { // Wrap postMessage in try-catch to handle synchronous exceptions const postPromise = connectedView.view.webview.postMessage(event); // Handle async failures - postPromise.then( + void postPromise.then( () => { // Message sent successfully }, (error: unknown) => { this.logger.error( - `Failed to send event ${key} to view ${connectedView.context.viewType}:${viewId}: ${getErrorMessage(error)}` + `Failed to send event ${key} to view ${connectedView.context.viewType}:${viewId}: ${getErrorMessage(error)}` ); - - // Mark view for removal - failedViews.push(viewId); + // Remove failed view immediately + if (this.connectedViews.delete(viewId)) { + this.logger.warn( + `Removed failed webview ${connectedView.context.viewType}:${viewId} from connectedViews` + ); + this.logger.info( + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `Remaining connected webviews: ${this.connectedViews.size}` + ); + } } ); } catch (error) { // Handle synchronous exceptions from postMessage - this.logger.error( - `Exception while sending event ${key} to view ${connectedView.context.viewType}:${viewId}: ${String(error)}` - ); - - // Mark view for removal - failedViews.push(viewId); + this.logger.error( + `Exception while sending event ${key} to view ${connectedView.context.viewType}:${viewId}: ${getErrorMessage(error)}` + ); + if (this.connectedViews.delete(viewId)) { + this.logger.warn( + `Removed failed webview ${connectedView.context.viewType}:${viewId} from connectedViews` + ); + this.logger.info( + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `Remaining connected webviews: ${this.connectedViews.size}` + ); + } } - } - - // Prune failed views after iteration to avoid modifying collection during iteration - if (failedViews.length > 0) { - for (const viewId of failedViews) { - const connectedView = this.connectedViews.get(viewId); - if (connectedView) { - this.logger.warn( - `Removing failed webview ${connectedView.context.viewType}:${viewId} from connectedViews` - ); - - // Only remove from connected views - let webviews handle their own disposal lifecycle - this.connectedViews.delete(viewId); - } - } - - this.logger.info( - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - `Removed ${failedViews.length} failed webview(s) from connectedViews. Remaining: ${this.connectedViews.size}` - ); - } + }Also applies to: 38-67, 69-87
🧹 Nitpick comments (26)
.github/workflows/ci.yml (1)
37-37: Don’t hide test failures: drop stderr redirection.
2>/dev/nullsuppresses stack traces and diagnostics, making failures hard to debug in CI.Apply:
- run: npm test -- --run 2>/dev/null + run: npm test -- --run.github/workflows/publish.yml (4)
39-39: Same here: don’t suppress test stderr.Apply:
- run: npm test -- --run 2>/dev/null + run: npm test -- --run
74-86: Enable npm provenance or drop unused OIDC permission.You grant
id-token: writebut don’t use it. Either add provenance or remove the permission.Apply one:
- name: Publish to NPM if: github.event_name == 'release' run: | @@ echo "Publishing with tag: $TAG" - npm publish --access public --tag $TAG + npm publish --access public --tag $TAG --provenance env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}Or, if not using provenance:
-permissions: - id-token: write - contents: read +permissions: + contents: read
65-67: Pin npm major instead of floating “latest”.Using the absolute latest can introduce surprise changes. Prefer a major pin (e.g., v10) for stability.
Example:
- - name: Upgrade npm to latest - run: npm install -g npm@latest + - name: Upgrade npm + run: npm install -g npm@^10
75-75: Nit: redundant condition.
if: github.event_name == 'release'is redundant since the workflow only runs onrelease.published. Safe to keep; can be removed for clarity.eslint.config.js (1)
129-129: Scope down unicorn/require-post-message-target-origin override.Global disable can hide real issues for window.postMessage elsewhere. Limit the override to VS Code webview modules that use vscode.postMessage (no targetOrigin).
- 'unicorn/require-post-message-target-origin': 'off', + // Default: keep on; override per webview files below. + 'unicorn/require-post-message-target-origin': 'error',Add a targeted block:
+ { + files: ['src/lib/**/Webview*.ts*', 'src/lib/**/useLogger.ts'], + rules: { + 'unicorn/require-post-message-target-origin': 'off', + }, + },src/lib/utils.ts (2)
4-7: Prefer crypto.randomUUID for lower collision risk.Use crypto when available; keep current fallback for older runtimes.
export function generateId(prefix: string): string { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions, sonarjs/pseudo-random, code-complete/no-magic-numbers-except-zero-one - return `${prefix}_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`; + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + // Use crypto when available; fallback to time + Math.random + // @ts-expect-error Node 18+/modern browsers expose crypto.randomUUID + if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + return `${prefix}_${crypto.randomUUID()}`; + } + // eslint-disable-next-line sonarjs/pseudo-random, code-complete/no-magic-numbers-except-zero-one + return `${prefix}_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`; }
8-10: Cross‑realm safety for Error checks.instanceof Error can fail across realms. Also handle objects with a message field.
-export function getErrorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} +export function getErrorMessage(error: unknown): string { + if (typeof error === 'object' && error !== null && 'message' in error) { + return String((error as { message: unknown }).message); + } + return String(error); +}src/lib/host/WebviewLogger.ts (2)
11-24: Validate level range in the guard.Reject out-of-range numeric levels to avoid unexpected host handling.
if (!('level' in value) || typeof value.level !== 'number') { return false; } + if (value.level < LogLevel.DEBUG || value.level > LogLevel.ERROR) { + return false; + }
33-67: Unify data param type with ILogger.Method signatures still use Record<any, any>. Align with ILogger’s Record<string, unknown>.
- debug(message: string, data?: Record<any, any>): void { + debug(message: string, data?: Record<string, unknown>): void { ... - info(message: string, data?: Record<any, any>): void { + info(message: string, data?: Record<string, unknown>): void { ... - warn(message: string, data?: Record<any, any>): void { + warn(message: string, data?: Record<string, unknown>): void { ... - error(message: string, data?: Record<any, any>): void { + error(message: string, data?: Record<string, unknown>): void {src/lib/client/useLogger.ts (1)
25-33: Skip logging undefined data to reduce noise.Avoid printing an extra “undefined” argument.
- debug: (message: string, data?: Record<string, unknown>) => - console.debug(`[${tag}] ${message}`, data), + debug: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.debug(`[${tag}] ${message}`) : console.debug(`[${tag}] ${message}`, data), - info: (message: string, data?: Record<string, unknown>) => - console.info(`[${tag}] ${message}`, data), + info: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.info(`[${tag}] ${message}`) : console.info(`[${tag}] ${message}`, data), - warn: (message: string, data?: Record<string, unknown>) => - console.warn(`[${tag}] ${message}`, data), + warn: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.warn(`[${tag}] ${message}`) : console.warn(`[${tag}] ${message}`, data), - error: (message: string, data?: Record<string, unknown>) => - console.error(`[${tag}] ${message}`, data), + error: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.error(`[${tag}] ${message}`) : console.error(`[${tag}] ${message}`, data),src/lib/types/ipcReducer.ts (2)
9-11: Replace explicitanywithunknownand drop the disables.These conditional-type positions don’t need
any. Usingunknowntightens typing and lets you remove the eslint-disable comments.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - [K in keyof T]: T[K] extends (...args: any[]) => any ? K : never; + [K in keyof T]: T[K] extends (...args: unknown[]) => unknown ? K : never;- // eslint-disable-next-line @typescript-eslint/no-explicit-any - readonly params: T[K] extends (...a: infer A) => any ? Readonly<A> : never; + readonly params: T[K] extends (...a: infer A) => unknown ? Readonly<A> : never;- [K in FnKeys<A>]: A[K] extends (...args: any) => infer R + [K in FnKeys<A>]: A[K] extends (...args: unknown[]) => infer RAlso applies to: 29-31, 41-46
48-66: Type guard improvements LGTM; tiny null-check simplification optional.Accepting
unknownand refining is correct. You can slightly simplify the null checks:- message !== null && - message !== undefined && + message != null &&src/lib/host/logger.ts (1)
71-84: Stringify fallback is good; consider truncation to keep logs sane.Optional: cap serialized payload length (e.g., 10–20KB) to avoid huge log writes.
src/lib/client/hooks.tsx (1)
20-32: Avoid relying onComponent.nameforviewType.Names can be minified/empty in prod builds. Allow an explicit
viewTypeoverride.-export function withWebviewApi<P extends object>(Component: ComponentType<P>): ComponentType<P> { +export function withWebviewApi<P extends object>( + Component: ComponentType<P>, + viewTypeOverride?: string +): ComponentType<P> { const WrappedComponent: FC<P> = (props) => { return ( - <WebviewProvider viewType={Component.name}> + <WebviewProvider viewType={viewTypeOverride ?? Component.displayName ?? Component.name}> <Component {...props} /> </WebviewProvider> ); };src/lib/client/WebviewProvider.tsx (5)
18-20: Remove unused eslint-disable (CI warning).The directive is unnecessary;
acquireVsCodeApi()is typed.-// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const vscodeApi = acquireVsCodeApi();
110-112: Type themessagelocal explicitly to quiet unsafe-assignment lint.- const message = event.data; + const message: unknown = event.data;
136-141: Handle unknownerrorsafely in listener callbacks.Avoid passing unknown directly to
console. Convert to string.- console.error('Error in event listener for %s:', message.key, error); + console.error(`Error in event listener for ${String(message.key)}: ${error instanceof Error ? error.message : String(error)}`);
1-15: Consolidate utils import — use getErrorMessage for error logsgetErrorMessage is available in src/lib/utils.ts; import it into src/lib/client/WebviewProvider.tsx and use it for error logs to keep formatting consistent.
163-173: Add a default timeout to DeferredPromise (e.g. 30s)types.ts defines timeoutHandle and clearTimeout but no timeout is set; set a default timeout in the DeferredPromise implementation (src/lib/client/types.ts) that rejects with a clear error and marksSettled on timeout, and ensure clearTimeout() is invoked on resolve/reject to avoid leaking pending requests if the host never responds.
src/lib/client/types.ts (1)
15-27: Clear timeouts on settle and widen resolve type.Prevent stray timers and allow PromiseLike resolution.
export class DeferredPromise<T> { - promise: Promise<T>; - resolve!: (value: T) => void; + readonly promise: Promise<T>; + resolve!: (value: T | PromiseLike<T>) => void; reject!: (reason?: unknown) => void; timeoutHandle?: ReturnType<typeof setTimeout>; private settled = false; constructor(readonly key: string) { this.promise = new Promise<T>((resolve, reject) => { - this.resolve = (value: T) => { + this.resolve = (value: T | PromiseLike<T>) => { if (!this.settled) { this.settled = true; - resolve(value); + this.clearTimeout(); + resolve(value); } }; this.reject = (reason?: unknown) => { if (!this.settled) { this.settled = true; - reject(reason instanceof Error ? reason : new Error(String(reason))); + this.clearTimeout(); + reject( + reason instanceof Error + ? reason + : new Error(typeof reason === 'string' ? reason : JSON.stringify(reason)) + ); } }; }); } /** * Clear the timeout handle if it exists */ clearTimeout(): void { if (this.timeoutHandle) { clearTimeout(this.timeoutHandle); this.timeoutHandle = undefined; } } /** * Mark this deferred as settled to prevent further resolve/reject calls */ markSettled(): void { this.settled = true; + this.clearTimeout(); } }Also applies to: 33-38, 43-45
src/lib/host/WebviewApiProvider.ts (1)
61-62: Use consistent error formatting helper.Use
getErrorMessage(error)here too (you already import it).- this.logger.error( - `Exception while sending event ${key} to view ${connectedView.context.viewType}:${viewId}: ${String(error)}` - ); + this.logger.error( + `Exception while sending event ${key} to view ${connectedView.context.viewType}:${viewId}: ${getErrorMessage(error)}` + );src/lib/host/BaseWebviewViewProvider.ts (4)
29-30: Avoid assumingproviderIdhas a dot.
providerId.split('.')[1]can beundefined. Provide a fallback.- this.logger = getLogger(providerId.split('.')[1]); + this.logger = getLogger((providerId.split('.')[1] ?? (providerId as unknown as string)));
88-93: Don’t dispose theonDidDisposedisposable from within its own callback.It’s redundant and risks confusing call order. Just dispose the message listener and run cleanup.
- this.webviewDisposable = webviewView.onDidDispose(() => { + this.webviewDisposable = webviewView.onDidDispose(() => { messageListener.dispose(); this.onWebviewDispose(); - this.webviewDisposable?.dispose(); });
66-83: Guard delegate execution and log errors.Uncaught errors from
delegateFnwill surface as unhandled rejections. Catch, log, and optionally report back.- const patch = await delegateFn(...message.params); + let patch: unknown; + try { + patch = await delegateFn(...message.params); + } catch (err) { + this.logger.error(`Action handler failed for ${String(message.key)}: ${String(err)}`); + return; + }
96-103: Optional: fire-and-forget safety.If your linter flags floating promises, prefix with
void.- this._view?.webview.postMessage({ + void this._view?.webview.postMessage({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(1 hunks).github/workflows/publish.yml(3 hunks)eslint.config.js(2 hunks)src/lib/client/WebviewContext.tsx(2 hunks)src/lib/client/WebviewProvider.tsx(1 hunks)src/lib/client/hooks.tsx(1 hunks)src/lib/client/types.ts(1 hunks)src/lib/client/useLogger.ts(2 hunks)src/lib/host/BaseWebviewViewProvider.ts(5 hunks)src/lib/host/ILogger.ts(1 hunks)src/lib/host/WebviewApiProvider.ts(8 hunks)src/lib/host/WebviewLogger.ts(1 hunks)src/lib/host/logger.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/types.ts(2 hunks)src/lib/types/ipcReducer.ts(4 hunks)src/lib/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/lib/utils.ts (2)
src/lib/host/WebviewLogger.ts (1)
error(60-67)src/lib/host/logger.ts (1)
error(63-65)
src/lib/client/WebviewProvider.tsx (4)
src/lib/types.ts (8)
VsCodeApi(140-144)HostCalls(3-3)RequestContext(9-14)ClientCalls(4-4)ViewApiRequest(19-25)isViewApiResponse(79-89)isViewApiError(94-106)isViewApiEvent(111-123)src/lib/client/types.ts (1)
DeferredPromise(6-46)src/lib/utils.ts (1)
generateId(4-7)src/lib/client/WebviewContext.tsx (2)
WebviewContextValue(7-15)WebviewContext(17-17)
src/lib/client/hooks.tsx (2)
src/lib/client/WebviewContext.tsx (2)
WebviewContextValue(7-15)WebviewContext(17-17)src/lib/client/WebviewProvider.tsx (1)
WebviewProvider(29-184)
src/lib/host/logger.ts (3)
src/lib/index.ts (1)
disallowedLogKeys(1-1)src/lib/host/WebviewLogger.ts (1)
error(60-67)src/lib/host/ILogger.ts (1)
ILogger(6-12)
src/lib/host/WebviewApiProvider.ts (3)
src/lib/types/ipcReducer.ts (1)
WebviewKey(3-3)src/lib/utils.ts (2)
getErrorMessage(8-10)generateId(4-7)src/lib/types.ts (1)
RequestContext(9-14)
src/lib/host/BaseWebviewViewProvider.ts (3)
src/lib/types.ts (1)
WebviewContextData(127-137)src/lib/host/WebviewLogger.ts (2)
isLogMessage(11-25)LogMessage(4-9)src/lib/types/ipcReducer.ts (1)
isMyActionMessage(48-66)
🪛 GitHub Actions: CI
src/lib/client/WebviewProvider.tsx
[warning] 18-18: Command: eslint .; Unused eslint-disable directive (no problems were reported from '@typescript-eslint/no-unsafe-assignment')
[error] 78-78: Command: eslint .; Unsafe assignment of an error typed value (@typescript-eslint/no-unsafe-assignment)
src/lib/host/BaseWebviewViewProvider.ts
[error] 1-1: depcheck detected missing dependency 'vscode' required by ./src/lib/host/BaseWebviewViewProvider.ts. The step 'npx depcheck --ignores "@types/*"' failed with exit code 255.
🪛 GitHub Check: Lint, Type Check, and Test
src/lib/client/types.ts
[failure] 1-1:
'VsCodeApi' is defined but never used
[failure] 1-1:
Remove this unused import of 'VsCodeApi'
[failure] 1-1:
'HostCalls' is defined but never used
[failure] 1-1:
Remove this unused import of 'HostCalls'
[failure] 1-1:
'ClientCalls' is defined but never used
[failure] 1-1:
Remove this unused import of 'ClientCalls'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (11)
src/lib/host/ILogger.ts (2)
7-12: LGTM: tighter log data typing.Changing data to Record<string, unknown> improves safety and aligns with sanitization.
15-18: LGTM: explicit LogLevel enum entries.Enum remains stable; callers sending numeric levels across the boundary are compatible.
src/lib/host/WebviewLogger.ts (1)
4-9: LGTM: exported LogMessage and stricter data type.Public surface is clearer and safer.
src/lib/types.ts (5)
79-88: LGTM: response guard is precise and permissive.Only enforces required fields; optional value is not forced. Good.
94-105: LGTM: error guard enforces stringified value.Consistent with getErrorMessage usage elsewhere.
111-122: LGTM: event guard shape is reasonable.Array check for value matches Parameters<...>.
127-137: LGTM: WebviewContextData rename and shape.API surface is clearer and backwards-compatible when re-exported properly.
141-144: LGTM: VsCodeApi methods now unknown‑typed.Safer boundary; callers must assert types intentionally.
eslint.config.js (1)
57-59: projectService: tooling looks compatible — no action requiredpackage.json contains eslint@^9.33.0 and typescript-eslint@^8.43.0; typescript-eslint v8 supports ESLint v9 and exposes the parser/plugin via the typescript-eslint meta-package, so projectService: true is supported. (typescripteslint.com)
Confirm eslint.config.js still sets projectService and that your config imports/uses the typescript-eslint package or sets parser to '@typescript-eslint/parser'; remove any older @typescript-eslint v7 packages if present.
File: eslint.config.js
Lines: 57-59projectService: true, tsconfigRootDir: import.meta.dirname, },src/lib/client/WebviewContext.tsx (1)
7-15: Context surface looks solid.Typing aligns with the new provider/hook setup. Using
undefineddefault pairs well with the throwing hook.Also applies to: 17-17
src/lib/host/logger.ts (1)
2-2: No circular import — OK to importdisallowedLogKeysfrom the barrel. Verified:Loggeris only exported in src/lib/host/logger.ts and the barrel (src/lib) does not re‑exportLogger, so theimport { disallowedLogKeys } from '..'does not introduce a cycle.
| @@ -0,0 +1,46 @@ | |||
| import type { ClientCalls, HostCalls, VsCodeApi } from '../types'; | |||
There was a problem hiding this comment.
Remove unused imports to fix CI lint failures.
ClientCalls, HostCalls, and VsCodeApi are not used; lint is failing. Drop the import.
-import type { ClientCalls, HostCalls, VsCodeApi } from '../types';
+// (intentionally empty) – no imports required📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { ClientCalls, HostCalls, VsCodeApi } from '../types'; | |
| // (intentionally empty) – no imports required |
🧰 Tools
🪛 GitHub Check: Lint, Type Check, and Test
[failure] 1-1:
'VsCodeApi' is defined but never used
[failure] 1-1:
Remove this unused import of 'VsCodeApi'
[failure] 1-1:
'HostCalls' is defined but never used
[failure] 1-1:
Remove this unused import of 'HostCalls'
[failure] 1-1:
'ClientCalls' is defined but never used
[failure] 1-1:
Remove this unused import of 'ClientCalls'
🤖 Prompt for AI Agents
In src/lib/client/types.ts around line 1, the import line brings in ClientCalls,
HostCalls, and VsCodeApi which are unused and causing CI lint failures; remove
those unused specifiers from the import (or delete the entire import line if
nothing else is needed) so the file no longer imports unused types, then run the
linter/CI to verify the error is resolved.
| @@ -1,5 +1,5 @@ | |||
| import * as vscode from 'vscode'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
Fix CI depcheck: add vscode as a devDependency or ignore it.
CI fails: depcheck reports missing dependency 'vscode' for this import. Add "vscode": "^<engine-matched>" to devDependencies (match engines.vscode) or update depcheck config to ignore 'vscode'.
Proposed package.json change:
{
"devDependencies": {
+ "vscode": "^1.93.0"
},
"scripts": {
- "depcheck": "npx depcheck --ignores \"@types/*\""
+ "depcheck": "npx depcheck --ignores \"@types/*,vscode\""
}
}If you prefer only one, pick either adding the devDependency (recommended) or ignoring it.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Root package.json (first 400 lines) ----"
if [ -f package.json ]; then
sed -n '1,400p' package.json || true
else
echo "no root package.json"
fi
echo "---- All package.json files (paths) ----"
find . -type f -name package.json -not -path '*/node_modules/*' -print || true
echo "---- Search for 'vscode' string in package.json files ----"
rg -n --hidden --no-ignore -g '!**/node_modules/**' '"vscode"' || true
echo "---- Search for depcheck occurrences ----"
rg -n --hidden --no-ignore -g '!**/node_modules/**' 'depcheck' || true
echo "---- Search for depcheck config files ----"
ls -la .depcheckrc .depcheckrc.js .depcheckrc.json 2>/dev/null || true
echo "---- Search for 'vscode' imports in source files ----"
rg -n --hidden --no-ignore -g '!**/node_modules/**' "from ['\"]vscode['\"]|require\\(['\"]vscode['\"]\\)|import \\* as vscode" -S || trueLength of output: 3269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- .github/workflows ----"
if [ -d .github/workflows ]; then
ls -la .github/workflows || true
else
echo "no .github/workflows"
fi
echo "---- grep for 'depcheck' ----"
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=.git -F "depcheck" . || true
echo "---- grep for 'npx depcheck' ----"
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=.git -F "npx depcheck" . || true
echo "---- grep for 'vscode' imports/usages ----"
grep -RIn --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=.git -E "from ['\"]vscode['\"]|require\\(['\"]vscode['\"]\\)|import \\* as vscode|\"@types/vscode\"|engines\\.vscode" . || true
echo "---- show src/lib/host/BaseWebviewViewProvider.ts (if exists) ----"
if [ -f src/lib/host/BaseWebviewViewProvider.ts ]; then
sed -n '1,240p' src/lib/host/BaseWebviewViewProvider.ts || true
else
echo "src/lib/host/BaseWebviewViewProvider.ts not found"
fi
echo "---- package.json engines/devDependencies/scripts snippet ----"
if [ -f package.json ]; then
python - <<'PY'
import json,sys
with open('package.json') as f:
j=json.load(f)
out={}
for k in ('engines','devDependencies','scripts'):
if k in j: out[k]=j[k]
print(json.dumps(out,indent=2))
PY
else
echo "no package.json"
fi
echo "---- check for .depcheckrc files ----"
ls -la .depcheckrc .depcheckrc.js .depcheckrc.json 2>/dev/null || trueLength of output: 6947
Fix CI depcheck: add vscode to devDependencies or ignore it in CI
CI (.github/workflows/ci.yml) runs depcheck and flags imports of vscode (e.g. src/lib/host/BaseWebviewViewProvider.ts) because package.json only has @types/vscode. Either:
- Add
"vscode"to devDependencies in package.json (recommended). - Or update CI to ignore it: change the depcheck run to
npx depcheck --ignores="@types/*,vscode"in .github/workflows/ci.yml.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: depcheck detected missing dependency 'vscode' required by ./src/lib/host/BaseWebviewViewProvider.ts. The step 'npx depcheck --ignores "@types/*"' failed with exit code 255.
🤖 Prompt for AI Agents
In src/lib/host/BaseWebviewViewProvider.ts line 1, CI depcheck is flagging the
import of 'vscode' because package.json only contains @types/vscode; either add
"vscode" to devDependencies in package.json (recommended) so depcheck recognizes
the import, or modify .github/workflows/ci.yml to run depcheck with an ignore
for vscode (e.g. npx depcheck --ignores="@types/*,vscode") to suppress the
warning — choose one approach and update the corresponding file accordingly.
| registerView(id: WebviewKey, view: vscode.WebviewView): void { | ||
| if (this.connectedViews.has(id) && view === this.connectedViews.get(id)?.view) { | ||
| this.logger.error(`Webview ${id} already registered`); | ||
| return; | ||
| } | ||
|
|
||
| const context: RequestContext = { | ||
| viewId: id, | ||
| viewType: viewType, | ||
| viewType: view.viewType, | ||
| timestamp: Date.now(), | ||
| sessionId: `session-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, | ||
| sessionId: generateId('session'), | ||
| }; | ||
|
|
||
| this.connectedViews.set(id, { view, context }); | ||
| this.logger.info(`Registered webview: ${viewType}:${id}`); | ||
| this.logger.info(`Registered webview: ${view.viewType}:${id}`); |
There was a problem hiding this comment.
Duplicate registration semantics can drop a newly registered view.
If a different view registers with the same id, the old onDidDispose callback will later delete the new mapping. Guard or replace safely.
- registerView(id: WebviewKey, view: vscode.WebviewView): void {
- if (this.connectedViews.has(id) && view === this.connectedViews.get(id)?.view) {
- this.logger.error(`Webview ${id} already registered`);
- return;
- }
+ registerView(id: WebviewKey, view: vscode.WebviewView): void {
+ const existing = this.connectedViews.get(id);
+ if (existing?.view === view) {
+ this.logger.warn(`Webview ${existing.context.viewType}:${id} already registered; ignoring`);
+ return;
+ }
+ if (existing && existing.view !== view) {
+ this.logger.warn(
+ `Replacing existing webview for id ${id} (${existing.context.viewType} -> ${view.viewType})`
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| registerView(id: WebviewKey, view: vscode.WebviewView): void { | |
| if (this.connectedViews.has(id) && view === this.connectedViews.get(id)?.view) { | |
| this.logger.error(`Webview ${id} already registered`); | |
| return; | |
| } | |
| const context: RequestContext = { | |
| viewId: id, | |
| viewType: viewType, | |
| viewType: view.viewType, | |
| timestamp: Date.now(), | |
| sessionId: `session-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, | |
| sessionId: generateId('session'), | |
| }; | |
| this.connectedViews.set(id, { view, context }); | |
| this.logger.info(`Registered webview: ${viewType}:${id}`); | |
| this.logger.info(`Registered webview: ${view.viewType}:${id}`); | |
| registerView(id: WebviewKey, view: vscode.WebviewView): void { | |
| const existing = this.connectedViews.get(id); | |
| if (existing?.view === view) { | |
| this.logger.warn(`Webview ${existing.context.viewType}:${id} already registered; ignoring`); | |
| return; | |
| } | |
| if (existing && existing.view !== view) { | |
| this.logger.warn( | |
| `Replacing existing webview for id ${id} (${existing.context.viewType} -> ${view.viewType})` | |
| ); | |
| } | |
| const context: RequestContext = { | |
| viewId: id, | |
| viewType: view.viewType, | |
| timestamp: Date.now(), | |
| sessionId: generateId('session'), | |
| }; | |
| this.connectedViews.set(id, { view, context }); | |
| this.logger.info(`Registered webview: ${view.viewType}:${id}`); |
🤖 Prompt for AI Agents
In src/lib/host/WebviewApiProvider.ts around lines 93 to 107, the current
duplicate-registration logic returns only when the exact same view object is
re-registered but allows a new view with the same id to overwrite the map while
the old view's onDidDispose callback still references and can later delete the
new mapping; fix by safely replacing the existing registration: if
connectedViews.has(id) and the existing.view !== view, first remove or dispose
the old view's onDidDispose listener (or make that listener check view identity
before deleting), then set the new mapping and attach the new onDidDispose;
alternatively store the disposable for the dispose callback in the map and call
disposable.dispose() before replacing so the old callback cannot delete the new
entry.
| view.onDidDispose(() => { | ||
| this.connectedViews.delete(id); | ||
| this.logger.info(`Unregistered webview: ${viewType}:${id}`); | ||
| this.logger.info(`Unregistered webview: ${view.viewType}:${id}`); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ignore stale dispose events after re-registration.
Ensure an old view’s dispose doesn’t remove a newer mapping for the same id.
- view.onDidDispose(() => {
- this.connectedViews.delete(id);
- this.logger.info(`Unregistered webview: ${view.viewType}:${id}`);
- });
+ view.onDidDispose(() => {
+ const current = this.connectedViews.get(id);
+ if (current?.view === view) {
+ this.connectedViews.delete(id);
+ this.logger.info(`Unregistered webview: ${view.viewType}:${id}`);
+ } else {
+ this.logger.debug(`Ignored stale dispose for ${view.viewType}:${id}`);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| view.onDidDispose(() => { | |
| this.connectedViews.delete(id); | |
| this.logger.info(`Unregistered webview: ${viewType}:${id}`); | |
| this.logger.info(`Unregistered webview: ${view.viewType}:${id}`); | |
| }); | |
| view.onDidDispose(() => { | |
| const current = this.connectedViews.get(id); | |
| if (current?.view === view) { | |
| this.connectedViews.delete(id); | |
| this.logger.info(`Unregistered webview: ${view.viewType}:${id}`); | |
| } else { | |
| this.logger.debug(`Ignored stale dispose for ${view.viewType}:${id}`); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/lib/host/WebviewApiProvider.ts around lines 110 to 113, the dispose
handler currently unconditionally deletes this.connectedViews.delete(id) which
allows a stale dispose to remove a newer view registered under the same id;
change the handler to first check that the map still references the same view
instance (e.g. this.connectedViews.get(id) === view) and only then delete and
log the unregistration, otherwise ignore the dispose event.
1 participant
Summary by Sourcery
Refactor the client-side webview API into modular components, strengthen type safety, enhance host-side logging, and streamline CI/CD workflows
Enhancements:
Build:
Chores:
Important
Refactor webview context and provider, enhance logging and API handling, and update configurations for improved maintainability and functionality.
WebviewContext.tsxto only include context creation.WebviewProviderlogic toWebviewProvider.tsx.hooks.tsxforuseWebviewApiandwithWebviewApi.ILoggerinterface inILogger.tsto useRecord<string, unknown>for data.WebviewLogger.tsto useLogMessageinterface and improve type safety.logger.tsto sanitize log data and handle serialization errors.WebviewApiProvider.tsto improve event handling and view registration.generateIdfromutils.tsfor consistent ID generation.eslint.config.jsto include new rules and plugins.publish.yml.disallowedLogKeysinindex.tsfor sensitive data filtering.This description was created by
for d369b55. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Refactor
Chores
Style