Conversation
WalkthroughAdds dedicated client and host entry points and exports, updates build configuration to emit multiple entries, consolidates public re-exports, narrows a key public type, removes a client HOC, adjusts logger typing and naming, and relocates the disallowedLogKeys definition and re-exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client Component
participant WP as WebviewProvider
participant VH as VS Code Host
participant HP as WebviewApiProvider
C->>WP: useWebviewApi().callApi(method, args)
note right of WP: New/Changed: callApi return type annotation<br/>(implementation resolves a promise)
WP->>VH: postMessage({ request })
VH->>HP: dispatch(request)
HP-->>VH: response / error
VH-->>WP: message({ response })
WP-->>C: resolve/reject
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 separates the library into distinct entry points for client and host by updating the build config and exports, reorganizes module entry files, refines type handling in the client API hooks, and cleans up logger initialization and related constants. Class diagram for updated ClientCalls type and useWebviewApiclassDiagram
class ClientCalls {
<<type>>
+ [key: string]: (...args: any[]) => PromiseLike<any>
}
class useWebviewApi {
+ useWebviewApi<T extends ClientCalls>(): WebviewContextValue<T>
+ createCtxKey<T extends ClientCalls>(contextKey: string): CtxKey<T>
}
useWebviewApi --> ClientCalls
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 significantly refactors the library's architecture by separating client and host-side code into distinct modules with their own entry points. This change aims to improve modularity, reduce bundle sizes for consumers by allowing them to import only the necessary components, and enhance the clarity of the codebase by clearly delineating responsibilities between client and host environments. The build configuration has been updated to support this new structure, and several type definitions have been refined for improved accuracy.
Highlights
- Separate Entry Points: Introduced distinct entry points for client-side (
./client) and host-side (./host) code inpackage.jsonandvite.config.ts, allowing for more granular imports and potentially smaller bundles. - New Client and Host Modules: Created
src/lib/client.tsandsrc/lib/host.tsto consolidate and export client-specific and host-specific functionalities, respectively. - Type Refinements: Modified the
callApireturn type inWebviewProvider.tsxfor better type inference and refined theClientCallstype insrc/lib/types.tsto only accept string keys. - Code Cleanup and Relocation: Removed the
withWebviewApi.tsxhigher-order component and relocateddisallowedLogKeysfromsrc/lib/index.tstosrc/lib/host/logger.ts.
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:
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
|
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 |
![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:
- Double-check the Vite config’s removal of the custom fileName logic—ensure the bundled host and client files are named and placed as expected across both ES and CJS outputs.
- The ClientCalls type was tightened from allowing string|symbol keys to only string keys—verify there are no existing code paths or contexts relying on symbol‐keyed call definitions.
- The disallowedLogKeys array has been moved and re-exported in host/logger.ts—consider centralizing this constant in one module to avoid duplication or drift as its usage grows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check the Vite config’s removal of the custom fileName logic—ensure the bundled host and client files are named and placed as expected across both ES and CJS outputs.
- The ClientCalls type was tightened from allowing string|symbol keys to only string keys—verify there are no existing code paths or contexts relying on symbol‐keyed call definitions.
- The disallowedLogKeys array has been moved and re-exported in host/logger.ts—consider centralizing this constant in one module to avoid duplication or drift as its usage grows.
## Individual Comments
### Comment 1
<location> `src/lib/client/WebviewProvider.tsx:54` </location>
<code_context>
const callApi = <K extends keyof T = keyof T>(
key: K,
...params: Parameters<T[K]>
- ): Promise<ReturnType<T[K]>> => {
+ ): ReturnType<T[K]> => {
const id = generateId('req');
const deferred = new DeferredPromise<Awaited<ReturnType<T[K]>>>(key as string);
</code_context>
<issue_to_address>
**issue:** Changing return type from Promise<ReturnType<T[K]>> to ReturnType<T[K]> may cause type mismatches.
Ensure that all callApi usages expect a promise, as the implementation still returns one despite the updated return type.
</issue_to_address>
### Comment 2
<location> `src/lib/host/BaseWebviewViewProvider.ts:28` </location>
<code_context>
private readonly extensionUri: vscode.Uri,
private readonly apiProvider?: WebviewApiProvider<HostCalls>
) {
- this.logger = getLogger(providerId.split('.')[1]);
+ this.logger = getLogger(providerId.split('.')[1] ?? providerId);
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Fallback to providerId if split fails may mask errors.
Validating providerId or logging a warning when the format is unexpected would help catch formatting errors instead of silently falling back.
```suggestion
const providerIdParts = providerId.split('.');
if (providerIdParts.length < 2) {
console.warn(`[BaseWebviewViewProvider] Unexpected providerId format: "${providerId}". Expected at least one '.'`);
this.logger = getLogger(providerId);
} else {
this.logger = getLogger(providerIdParts[1]);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const callApi = <K extends keyof T = keyof T>( | ||
| key: K, | ||
| ...params: Parameters<T[K]> | ||
| ): Promise<ReturnType<T[K]>> => { |
There was a problem hiding this comment.
issue: Changing return type from Promise<ReturnType<T[K]>> to ReturnType<T[K]> may cause type mismatches.
Ensure that all callApi usages expect a promise, as the implementation still returns one despite the updated return type.
| private readonly extensionUri: vscode.Uri, | ||
| private readonly apiProvider?: WebviewApiProvider<HostCalls> | ||
| ) { | ||
| this.logger = getLogger(providerId.split('.')[1]); |
There was a problem hiding this comment.
suggestion (bug_risk): Fallback to providerId if split fails may mask errors.
Validating providerId or logging a warning when the format is unexpected would help catch formatting errors instead of silently falling back.
| this.logger = getLogger(providerId.split('.')[1]); | |
| const providerIdParts = providerId.split('.'); | |
| if (providerIdParts.length < 2) { | |
| console.warn(`[BaseWebviewViewProvider] Unexpected providerId format: "${providerId}". Expected at least one '.'`); | |
| this.logger = getLogger(providerId); | |
| } else { | |
| this.logger = getLogger(providerIdParts[1]); | |
| } |
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the library to have separate entry points for the client and host environments, which is a great enhancement for modularity. The changes to package.json and vite.config.ts correctly set up the new build structure. I also appreciate the improvements in type safety, such as refining the ClientCalls type and making the return type of callApi more accurate. However, I've found one critical issue: the withWebviewApi Higher-Order Component was removed, but its export statement remains in src/lib/index.ts, which will cause the build to fail. Please address this to ensure the project remains buildable.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 5a1ae38 in 2 minutes and 13 seconds. Click for details.
- Reviewed
219lines of code in11files - Skipped
0files when reviewing. - Skipped posting
7draft 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. package.json:25
- Draft comment:
New entry points for './host' and './client' have been added. Ensure these exports are compatible with your bundler and consumers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure compatibility with bundlers and consumers, which falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.
2. src/lib/client.ts:9
- Draft comment:
Missing newline at end-of-file; consider adding one to comply with POSIX standards. - 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% While technically correct about POSIX standards, this is a very minor issue that would likely be handled by editor settings or linting rules if it was important to the project. It doesn't affect functionality and is more of a style preference. According to our rules, we should not make purely informative comments or point out obvious/unimportant issues. The missing newline could potentially cause issues with some tools or in certain environments. It is a widely accepted standard. While true, if this was important to the project, it would be enforced by their tooling. This kind of minor style issue doesn't warrant a manual review comment. Delete this comment as it points out a minor style issue that doesn't require manual intervention and would be better handled by automated tooling if important to the project.
3. src/lib/client/WebviewProvider.tsx:79
- Draft comment:
The cast 'as ReturnType<T[K]>' may hide potential type mismatches. Verify that the deferred.promise type aligns with the expected promise type. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the alignment of types, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code. It is more of a cautionary note rather than a constructive comment.
4. src/lib/client/useWebviewApi.ts:24
- Draft comment:
Good use of Object.freeze to enforce immutability for the context key. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/lib/host/BaseWebviewViewProvider.ts:28
- Draft comment:
Ensure that 'providerId' always contains a '.' as expected; otherwise, splitting it might yield undefined. Consider adding validation for providerId format. - Reason this comment was not posted:
Comment was on unchanged code.
6. src/lib/types.ts:6
- Draft comment:
Changing 'ClientCalls' to accept only string keys (was string | symbol) may be breaking if symbol keys were used elsewhere. Confirm that symbol keys aren’t needed. - 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 type guard isViewApiRequest already enforces string keys with typeof message.key === 'string'. This means the code was already assuming string-only keys. The type change appears to be aligning the type definition with existing runtime behavior. The comment is speculative and asks for confirmation without strong evidence that symbol keys were ever used. I could be wrong if there are other files in the codebase that use symbol keys with ClientCalls that I can't see. The type guard might not be the only usage pattern. However, the presence of the type guard strongly suggests this was the intended usage pattern. If symbol keys were meant to be supported, the type guard would have been written differently. Delete the comment. The type change appears to be fixing a mismatch between types and runtime behavior, and the comment is speculative without evidence.
7. vite.config.ts:18
- Draft comment:
Multiple entry points (index, host, client) are now defined. Ensure output filenames and module boundaries are as intended; if module separation is required, consider setting preserveModules to true. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests ensuring that output filenames and module boundaries are as intended, which violates the rule against asking the PR author to ensure behavior is intended. However, it also provides a specific suggestion about settingpreserveModulesto true if module separation is required, which is a valid code suggestion.
Workflow ID: wflow_zeW9A6VdYdzvvat0
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/host/logger.ts (1)
18-21: structuredClone can throw before try/catch — make logging never throw.If
structuredClonefails (e.g., non-cloneables), the logger will throw. Logging must be fail-safe.Apply:
- const clone = structuredClone(dictionary) as Record<string, unknown>; - - try { + try { + const clone = structuredClone(dictionary) as Record<string, unknown>; for (const key in clone) { const value = clone[key]; if (disallowedLogKeys.includes(key)) { delete clone[key]; continue; } if (Array.isArray(value)) { clone[key] = value.map((item) => removePromptsFromData(item)) as unknown; } else if (typeof value === 'object' && value !== null) { clone[key] = removePromptsFromData(value as Record<string, unknown>) as unknown; } } } catch (error) { console.error('Error processing log data:', error); return {} as T; }Optional micro: use a local
Setfor lookups without changing the exported API.+ const redactSet = new Set(disallowedLogKeys); - if (disallowedLogKeys.includes(key)) { + if (redactSet.has(key)) {
🧹 Nitpick comments (7)
src/lib/client/WebviewProvider.tsx (1)
69-77: Handle postMessage Thenable rejections (and false).
vscode.postMessagereturns a Thenable. Sync try/catch won’t catch async failures; alsofalseshould be treated as delivery failure.Apply:
- try { - vscodeApi.postMessage(request); - } catch (error) { + try { + const maybeThenable = vscodeApi.postMessage(request); + if (maybeThenable && typeof (maybeThenable as any).then === 'function') { + (maybeThenable as PromiseLike<boolean>).then( + (ok) => { + if (ok === false) { + const err = new Error(`Host did not accept message ${String(key)}`); + const d = pendingRequests.current.get(id); + if (d) { + d.clearTimeout(); + pendingRequests.current.delete(id); + d.reject(err); + } + } + }, + (error) => { + const d = pendingRequests.current.get(id); + if (d) { + d.clearTimeout(); + pendingRequests.current.delete(id); + d.reject(error instanceof Error ? error : new Error(String(error))); + } + } + ); + } + } catch (error) { console.error(`Failed to send API request ${key as string}:`, error); deferred.clearTimeout(); pendingRequests.current.delete(id); deferred.reject(error instanceof Error ? error : new Error(String(error))); }src/lib/host/logger.ts (1)
4-5: Expose redaction keys here — consider freezing and documenting.Export looks good. Optionally make it immutable to avoid accidental mutation and clarify intent.
-export const disallowedLogKeys = ['password', 'secret', 'token', 'apiKey', 'apiSecret', 'content']; +export const disallowedLogKeys = Object.freeze( + ['password', 'secret', 'token', 'apiKey', 'apiSecret', 'content'] as const +);package.json (1)
1-95: CI depcheck false-positive for 'vscode'.
vscodeis provided by the VS Code runtime; it should be extern’d (it is) and ignored by depcheck. Don’t add it as a dependency (the npm ‘vscode’ package is not meant for runtime here).Update the CI depcheck step to ignore it:
- npx depcheck --ignores="@types/*" + npx depcheck --ignores="vscode,@types/*"Alternatively, add a depcheck config:
"devDependencies": { ... }, + "depcheck": { + "ignoreMatches": ["vscode", "@types/*"] + }src/lib/host/BaseWebviewViewProvider.ts (1)
28-29: Tag derivation: prefer last segment to handle multi-dot IDs.Using only index 1 can mislabel tags if there are more than two segments.
- this.logger = getLogger(providerId.split('.')[1] ?? providerId); + this.logger = getLogger(providerId.split('.').pop() ?? providerId);src/lib/client.ts (1)
9-9: Fix Prettier violation: add a trailing newline.CI is failing with "prettier/prettier: Insert ⏎".
Apply this diff:
-export type { WebviewLayout } from './types'; +export type { WebviewLayout } from './types'; +src/lib/host.ts (2)
8-8: Fix Prettier violation: add a trailing newline.Matches the CI error on this file.
Apply this diff:
-export type { WebviewLayout } from './types'; +export type { WebviewLayout } from './types'; +
2-2: MakedisallowedLogKeysimmutable to consumers.Since it’s part of the public host API, expose it as a
ReadonlyArray<string>or a frozenSet<string>to prevent mutation.In
src/lib/host/logger.ts, consider:export const disallowedLogKeys = Object.freeze<string[]>([/* ... */]) as readonly string[]; // or export const disallowedLogKeys = new Set<string>([/* ... */]) as ReadonlySet<string>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
package.json(1 hunks)src/lib/client.ts(1 hunks)src/lib/client/WebviewProvider.tsx(2 hunks)src/lib/client/useWebviewApi.ts(2 hunks)src/lib/client/withWebviewApi.tsx(0 hunks)src/lib/host.ts(1 hunks)src/lib/host/BaseWebviewViewProvider.ts(2 hunks)src/lib/host/logger.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/types.ts(1 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/lib/client/withWebviewApi.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib/types.ts (2)
src/lib/client.ts (1)
ClientCalls(7-7)src/lib/index.ts (1)
ClientCalls(13-13)
src/lib/client/useWebviewApi.ts (1)
src/lib/types.ts (1)
CtxKey(10-14)
src/lib/host/BaseWebviewViewProvider.ts (3)
src/lib/host/ILogger.ts (1)
ILogger(6-12)src/lib/types/ipcReducer.ts (2)
ActionDelegate(72-76)WebviewKey(3-3)src/lib/host/logger.ts (1)
getLogger(96-106)
src/lib/host/logger.ts (2)
src/lib/host.ts (1)
disallowedLogKeys(2-2)src/lib/index.ts (1)
disallowedLogKeys(10-10)
🪛 GitHub Actions: CI
src/lib/host/BaseWebviewViewProvider.ts
[error] 1-1: depcheck reported missing dependency 'vscode' for this file (not listed in package.json). Command: 'npx depcheck --ignores="@types/*"'. Exit code: 255.
src/lib/client.ts
[error] 9-9: prettier/prettier: Insert ⏎.
🪛 GitHub Check: Lint, Type Check, and Test
src/lib/host.ts
[failure] 8-8:
Insert ⏎
src/lib/client.ts
[failure] 9-9:
Insert ⏎
⏰ 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 (12)
src/lib/client/WebviewProvider.tsx (1)
54-54: Return type fix aligns with ClientCalls PromiseLike.Good change: returning
ReturnType<T[K]>(PromiseLike) avoids double-wrapping. The cast toReturnType<T[K]>is safe sincePromisesatisfiesPromiseLike.Please confirm no callers relied on the previous double-promise (e.g.,
await (await api.foo())). A quick repo grep for nestedawaiton API calls would suffice.Also applies to: 79-79
src/lib/types.ts (1)
6-6: String-only keys for ClientCalls match runtime guarantees.This lines up with the guards (
typeof message.key === 'string') and structured clone constraints. Mark as a (minor) breaking type change and note in release notes.Confirm no internal/public APIs used symbol keys for client calls.
src/lib/client/useWebviewApi.ts (2)
2-3: Import source refactor is clean.Switching to local types keeps entry points clean and avoids circular barrels.
24-24: Freezing context keys is a good defensive move.Prevents accidental mutation and preserves WeakMap identity guarantees.
vite.config.ts (1)
18-22: Multi-entry lib config LGTM.Matches new package exports. No need for custom fileName; Vite will emit per-entry names.
Double-check d.ts emission creates
host.d.tsandclient.d.tsat dist root withvite-plugin-dts rollupTypes: true.src/lib/host/BaseWebviewViewProvider.ts (3)
12-12: ILogger typing import is correct.Strongly typing the logger property tightens contracts without runtime impact.
21-21: Explicit logger type is an improvement.Keeps the class interface clear and helps callers.
1-1: CI: depcheck missing 'vscode'.Root cause and fixes summarized in package.json comment; nothing to change in this file.
src/lib/index.ts (1)
10-10: Re-exporting disallowedLogKeys from host/logger is consistent.Keeps the single source of truth in host/logger.
package.json (1)
25-33: Build artifacts missing — run build and re-verify exportsLocal verification failed: dist/ not found and require('./dist/host.cjs') raised MODULE_NOT_FOUND.
Snippet to verify remains (package.json exports):
"./host": { "types": "./dist/host.d.ts", "import": "./dist/host.js", "require": "./dist/host.cjs" }, "./client": { "types": "./dist/client.d.ts", "import": "./dist/client.js", "require": "./dist/client.cjs"Run these and paste the output:
npm run build ls -1 dist | rg -n '^(host|client)\.(js|cjs|d\.ts)$' node -e "require('./dist/host.cjs'); import('./dist/client.js').then(()=>console.log('ok'))"src/lib/client.ts (1)
1-5: Approved — client public surface verifiedAll symbols re-exported by src/lib/client.ts exist in their source files and are exported; no missing exports detected.
src/lib/host.ts (1)
1-4: Host entry point shape LGTM; package.json exports present — verify bundler emits host/client artifacts.
- package.json includes "./host" and "./client" with types/import/require pointing at dist/host.{d.ts,js,cjs} and dist/client.{d.ts,js} (package.json ~lines 25–32).
- No matching Vite/Rollup lib entry found in the quick check; confirm your build (vite/rollup/tsup/etc.) actually produces dist/host.{js,d.ts} and dist/client.{js,d.ts} or add the bundler entries.
1 participant
Summary by Sourcery
Split the library into separate client and host entry points, update build configuration and package exports accordingly, and refine related type definitions and module exports.
New Features:
clientandhostin both build config and package exportsclient.tsandhost.tsmodules to simplify imports for each entry pointEnhancements:
index,host,client) and remove custom filename logicdisallowedLogKeysfrom the host logger moduleuseWebviewApiandWebviewProviderClientCallstype to use string keys only and adjust host logger type annotationsSummary by CodeRabbit