Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a VS Code auth provider, persistent documentation cache, ZModel preview/save/clear commands with API interaction and caching, a release-notes webview, telemetry signin events, and an LSP request to gather imported ZModel URIs; activation registers these components and new commands/menus. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Claude finished @ymc9's task —— View job PR Review: ZModel Markdown Preview FeatureI'll review this PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds a comprehensive markdown preview feature for ZModel files in VS Code, including authentication, documentation caching, and release notes management. The implementation is well-structured with good separation of concerns. ✅ Strengths
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ide/vscode/src/extension/documentation-cache.ts`:
- Around line 42-50: The cache is being opted into VS Code Settings Sync by
calling extensionContext.globalState.setKeysForSync for keys starting with
DocumentationCache.CACHE_PREFIX; remove that opt-in so generated documentation
remains local-only: delete or disable the call to setKeysForSync in
enableCacheSync (and any other place that performs the same
filtering+setKeysForSync logic) and ensure no code path later registers
DocumentationCache.CACHE_PREFIX keys for sync via
extensionContext.globalState.setKeysForSync; keep the keys stored in globalState
but do not add them to the sync set.
- Around line 56-63: The cache key generation currently strips all whitespace
via model.replace(/\s/g, ''), which conflates distinct inputs; update
generateCacheKey so each model is normalized more safely (e.g., normalize line
endings, collapse consecutive whitespace to a single space, and trim) instead of
removing all spaces — for example use model.replace(/\s+/g, ' ').trim() when
building normalizedModels, then continue to sort, JSON.stringify and hash as
before (keep the createHash('sha512') usage and
DocumentationCache.CACHE_PREFIX).
In `@packages/ide/vscode/src/extension/release-notes-manager.ts`:
- Around line 25-32: The code currently sets the zmodelPreviewReleaseNoteKey as
"shown" unconditionally after calling showReleaseNotes(); change this so the key
is only persisted after showReleaseNotes() successfully loads/displays the HTML
(i.e., return/resolve a success boolean or throw on failure from
showReleaseNotes), and only then call
extensionContext.globalState.update(this.zmodelPreviewReleaseNoteKey, true) and
setKeysForSync(...). Apply the same pattern to the other similar block (lines
~39-68) so both usages of showReleaseNotes/showReleaseNotesIfFirstTime only mark
the announcement shown when the HTML load/display succeeded.
In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts`:
- Around line 177-188: In createSessionFromAccessToken, stop passing raw PII
(claims.email) to telemetry.identify; instead use a non-PII stable identifier
such as claims.sub (or a one-way hash of a stable claim) and fall back to a
generated id when sub is missing. Locate parseJWTClaims and the
telemetry.identify(...) call and replace the argument with claims.sub ||
hashedValue || fallbackId, ensuring you still set the account.label/account.id
fields for the session but never use claims.email directly as the telemetry
identifier.
- Around line 87-93: In logoutAllSessions, don't rely on this._sessions (which
may be empty) — first await this.getSessions() to load persisted sessions, check
the returned sessions array length, then remove each session by awaiting
removeSession for each id (avoid forEach(async ...) which doesn't wait); either
use a for...of loop awaiting this.removeSession(s.id) or await
Promise.all(sessions.map(s => this.removeSession(s.id))). After all removals
complete, call vscode.window.showInformationMessage('Successfully logged out of
ZenStack.').
- Around line 114-119: The redirectUrl constructed in the extension (variables
appName and redirectUrl) is embedded raw into signInUrl which causes its
internal query parameters to be treated as top-level params; encode redirectUrl
before adding it to the sign-in query (e.g., use encodeURIComponent or build
signInUrl with URL/URLSearchParams and set redirect_url to the encoded
redirectUrl) so signInUrl (created from AUTH_URL and used in signInUrl/with({
query: ... })) carries the full nested URL correctly; update the code that
creates signInUrl to use the encoded redirect_url value instead of the raw
redirectUrl.
- Around line 204-210: The JWT payload decoding currently treats the segment as
plain base64; update the logic in zenstack-auth-provider (the code that reads
parts[1] into payload, constructs paddedPayload, uses atob and JSON.parse) to
first convert base64url to base64 (replace '-' with '+' and '_' with '/'), then
add the '=' padding and decode (or use a base64 decoder that accepts base64url),
before JSON.parse the result so tokens with '-' or '_' decode correctly.
In `@packages/ide/vscode/src/extension/zmodel-preview.ts`:
- Around line 193-198: Inside the async callback passed to importedURIs.map,
stop reconstructing the URI with vscode.Uri.file(...) and reading from
workspace.fs; instead use vscode.workspace.openTextDocument(uri) and call
.getText() on the returned TextDocument so remote schemes and unsaved editor
edits are preserved. Update the logic in the importedURIs.map async function
(the block currently using vscode.Uri.file, workspace.fs.readFile,
Buffer.from(...).toString('utf8')) to open the document via
workspace.openTextDocument(uri), obtain the text via document.getText(), trim
it, and return that as the content while keeping the original uri/path
representation (do not lose the URI scheme).
- Around line 120-125: The fetch inside the vscode.window.withProgress call
needs an AbortSignal timeout so it can't hang indefinitely: create an
AbortController via AbortSignal.timeout(timeoutMs) (e.g., 30_000 ms) and pass
its signal to fetch(), handle aborts by catching the thrown
DOMException/AbortError and cleaning up the progress UI/error reporting, and
ensure any downstream code (the response processing in the same async block)
checks for signal/aborted state; update the code around
vscode.window.withProgress and the fetch(...) call to use the controller's
signal and to handle timeout errors gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc8e4652-7c2f-4ceb-9d75-bf9f757ec65d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/ide/vscode/package.jsonpackages/ide/vscode/src/extension/documentation-cache.tspackages/ide/vscode/src/extension/main.tspackages/ide/vscode/src/extension/release-notes-manager.tspackages/ide/vscode/src/extension/vscode-telemetry.tspackages/ide/vscode/src/extension/zenstack-auth-provider.tspackages/ide/vscode/src/extension/zmodel-preview.ts
|
Claude finished @ymc9's task —— View job PR Review: ZModel Markdown Preview FeatureI'll review this PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds a comprehensive markdown preview feature for ZModel files in VS Code, including authentication, documentation caching, and release notes management. The implementation is well-structured with good separation of concerns. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/ide/vscode/src/language-server/main.ts (3)
49-56: Use named constant for diagnostic severity.The magic number
1representsDiagnosticSeverity.Errorin LSP. Using the named constant improves readability and reduces the chance of errors.Proposed fix
+import { DiagnosticSeverity } from 'vscode-languageserver/node.js';if ( doc.parseResult.lexerErrors.length > 0 || doc.parseResult.parserErrors.length > 0 || - doc.diagnostics?.some((e) => e.severity === 1) + doc.diagnostics?.some((e) => e.severity === DiagnosticSeverity.Error) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/language-server/main.ts` around lines 49 - 56, The code in main.ts checks diagnostics with a magic number `1` to represent error severity; replace that literal with the LSP named constant (e.g., DiagnosticSeverity.Error) to improve readability: import DiagnosticSeverity from the language server/LSP module used in this project and update the predicate doc.diagnostics?.some((e) => e.severity === 1) to compare against DiagnosticSeverity.Error; ensure the import (or fully-qualified enum reference) is added near other imports so the symbol is available where the check occurs.
46-57: Consider reusing already-fetched documents instead of re-fetching.Lines 47-48 call
getOrCreateDocumentagain for URIs that were already loaded inimportedDocuments(line 37-39). While functionally correct (likely returns cached documents), this is redundant. Consider reusing the already-fetched document collection.Proposed refactor
+ const allDocuments = [document, ...importedDocuments]; + // build the document together with standard library, plugin modules, and imported documents - await shared.workspace.DocumentBuilder.build([document, ...importedDocuments], { + await shared.workspace.DocumentBuilder.build(allDocuments, { validation: true, }); let hasSyntaxErrors = false; - for (const docUri of [uri, ...importedURIs]) { - const doc = await langiumDocuments.getOrCreateDocument(docUri); + for (const doc of allDocuments) { if ( doc.parseResult.lexerErrors.length > 0 || doc.parseResult.parserErrors.length > 0 || doc.diagnostics?.some((e) => e.severity === 1) ) { hasSyntaxErrors = true; break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/language-server/main.ts` around lines 46 - 57, The loop that sets hasSyntaxErrors re-fetches documents via langiumDocuments.getOrCreateDocument for URIs already loaded into importedDocuments; update the loop to first look up each doc in the existing importedDocuments collection (by matching uri and entries in importedURIs) and only call langiumDocuments.getOrCreateDocument for URIs not present, then perform the same lexer/parser/diagnostic checks on the retrieved document; reference the variables uri, importedURIs, importedDocuments and the function langiumDocuments.getOrCreateDocument and ensure the logic still breaks early when a syntax error is found.
87-89: Use strict equality for string comparison.Line 88 uses loose equality
!=for comparing strings. Prefer strict equality!==for consistency and to avoid type coercion surprises.Proposed fix
return Array.from(uris) - .filter((x) => uriString != x) + .filter((x) => uriString !== x) .map((e) => URI.parse(e));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/language-server/main.ts` around lines 87 - 89, The filter uses loose inequality when comparing uriString to each item (Array.from(uris).filter((x) => uriString != x)), so change the comparison to strict inequality (uriString !== x) to avoid type-coercion issues; keep the rest of the expression (the map to URI.parse) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ide/vscode/src/language-server/main.ts`:
- Around line 59-62: The returned importedURIs must preserve the shape the
client expects (an object with a .path) instead of converting to a plain string;
in the code that builds the return (the importedURIs returned from
eagerLoadAllImports in main.ts), map each URI to an object with a path property
(e.g., importedURIs.map(uri => ({ path: uri.path }))) or else keep the original
parsed-URI plain objects and update the TypeScript assertion, so the client code
at zmodel-preview.ts:195 (which calls vscode.Uri.file(uri.path)) continues to
work; update the returned value where importedURIs is returned to use this
mapped shape.
---
Nitpick comments:
In `@packages/ide/vscode/src/language-server/main.ts`:
- Around line 49-56: The code in main.ts checks diagnostics with a magic number
`1` to represent error severity; replace that literal with the LSP named
constant (e.g., DiagnosticSeverity.Error) to improve readability: import
DiagnosticSeverity from the language server/LSP module used in this project and
update the predicate doc.diagnostics?.some((e) => e.severity === 1) to compare
against DiagnosticSeverity.Error; ensure the import (or fully-qualified enum
reference) is added near other imports so the symbol is available where the
check occurs.
- Around line 46-57: The loop that sets hasSyntaxErrors re-fetches documents via
langiumDocuments.getOrCreateDocument for URIs already loaded into
importedDocuments; update the loop to first look up each doc in the existing
importedDocuments collection (by matching uri and entries in importedURIs) and
only call langiumDocuments.getOrCreateDocument for URIs not present, then
perform the same lexer/parser/diagnostic checks on the retrieved document;
reference the variables uri, importedURIs, importedDocuments and the function
langiumDocuments.getOrCreateDocument and ensure the logic still breaks early
when a syntax error is found.
- Around line 87-89: The filter uses loose inequality when comparing uriString
to each item (Array.from(uris).filter((x) => uriString != x)), so change the
comparison to strict inequality (uriString !== x) to avoid type-coercion issues;
keep the rest of the expression (the map to URI.parse) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fe15bb4-1917-47a1-8819-88a6790f4ef8
📒 Files selected for processing (2)
packages/ide/vscode/src/extension/zenstack-auth-provider.tspackages/ide/vscode/src/language-server/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ide/vscode/src/extension/zenstack-auth-provider.ts
|
Claude finished @ymc9's task —— View job PR Review: ZModel Markdown Preview FeatureI'll review this PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds a comprehensive markdown preview feature for ZModel files in VS Code, including authentication, documentation caching, and release notes management. The implementation is well-structured with good separation of concerns. ✅ Strengths
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ide/vscode/src/extension/release-notes-manager.ts (1)
25-33:⚠️ Potential issue | 🟡 MinorOnly mark the announcement as shown after the HTML loads successfully.
Since
showReleaseNotes()catches errors internally and doesn't propagate failure, the flag on Lines 30-32 will be set even when showing the release notes fails. This prevents users from ever seeing the announcement if the first attempt fails.,
🛡️ Proposed fix: return success status from showReleaseNotes
async showReleaseNotesIfFirstTime(): Promise<void> { // Show release notes if this is the first time activating this version if (!this.extensionContext.globalState.get(this.zmodelPreviewReleaseNoteKey)) { - await this.showReleaseNotes(); - // Update the stored version to prevent showing again - await this.extensionContext.globalState.update(this.zmodelPreviewReleaseNoteKey, true); - // Add this key to sync keys for cross-machine synchronization - this.extensionContext.globalState.setKeysForSync([this.zmodelPreviewReleaseNoteKey]); + const success = await this.showReleaseNotes(); + if (success) { + // Update the stored version to prevent showing again + await this.extensionContext.globalState.update(this.zmodelPreviewReleaseNoteKey, true); + // Add this key to sync keys for cross-machine synchronization + this.extensionContext.globalState.setKeysForSync([this.zmodelPreviewReleaseNoteKey]); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/release-notes-manager.ts` around lines 25 - 33, The current flow in showReleaseNotesIfFirstTime sets the zmodelPreviewReleaseNoteKey flag unconditionally after calling showReleaseNotes(), which can hide the announcement if showReleaseNotes() failed internally; change the call so that showReleaseNotes() returns a success boolean (or rethrow errors) and only call extensionContext.globalState.update(...) and setKeysForSync(...) when that success is true (i.e., after the HTML has loaded successfully), updating showReleaseNotes implementation to resolve true on success and false or throw on failure so showReleaseNotesIfFirstTime can rely on its result.
🧹 Nitpick comments (3)
packages/ide/vscode/src/extension/release-notes-manager.ts (3)
54-57: Consider tracking panel for proper disposal.The webview panel isn't stored, so it can't be disposed when the extension deactivates. Also,
retainContextWhenHidden: truekeeps the webview in memory even when hidden—this may be unnecessary for a static release notes page.♻️ Proposed fix
export class ReleaseNotesManager implements vscode.Disposable { private extensionContext: vscode.ExtensionContext; private readonly zmodelPreviewReleaseNoteKey = 'zmodel-v3-preview-release-note-shown'; + private panel: vscode.WebviewPanel | undefined;In
showReleaseNotes():- const panel = vscode.window.createWebviewPanel( + this.panel = vscode.window.createWebviewPanel( 'ZenstackReleaseNotes', 'ZenStack - New Feature Announcement!', vscode.ViewColumn.One, { enableScripts: true, - retainContextWhenHidden: true, }, ); - panel.webview.html = htmlContent; + this.panel.webview.html = htmlContent; // Optional: Close the panel when user clicks outside or after some time - panel.onDidDispose(() => { - // Panel disposed + this.panel.onDidDispose(() => { + this.panel = undefined; });In
dispose():dispose(): void { - // Any cleanup if needed + this.panel?.dispose(); }Also applies to: 74-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/release-notes-manager.ts` around lines 54 - 57, The webview panel created in showReleaseNotes() isn't stored and retainContextWhenHidden is true; update showReleaseNotes() to assign the created WebviewPanel to a module/class-level variable (e.g., this.panel or releaseNotesPanel) and set retainContextWhenHidden to false for a static release notes page, then implement/augment dispose() to check and call panel.dispose() and null out the reference so the panel is properly cleaned up when the extension deactivates; apply the same pattern to the other panel creation at lines 74-76.
39-68: Return success status to enable conditional flag-setting.The method catches errors and logs them but returns
void, making it impossible for callers to know if the operation succeeded. Return a boolean to allowshowReleaseNotesIfFirstTime()to conditionally persist the "shown" flag.♻️ Proposed fix
- async showReleaseNotes(): Promise<void> { + async showReleaseNotes(): Promise<boolean> { try { // Read the release notes HTML file const releaseNotesPath = vscode.Uri.joinPath( this.extensionContext.extensionUri, 'res/zmodel-v3-preview-release-notes.html', ); const htmlBytes = await vscode.workspace.fs.readFile(releaseNotesPath); const htmlContent = Buffer.from(htmlBytes).toString('utf8'); // Create and show the release notes webview const panel = vscode.window.createWebviewPanel( 'ZenstackReleaseNotes', 'ZenStack - New Feature Announcement!', vscode.ViewColumn.One, { enableScripts: true, retainContextWhenHidden: true, }, ); panel.webview.html = htmlContent; // Optional: Close the panel when user clicks outside or after some time panel.onDidDispose(() => { // Panel disposed }); + return true; } catch (error) { console.error('Error showing release notes:', error); + return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/release-notes-manager.ts` around lines 39 - 68, The showReleaseNotes method currently returns void so callers cannot know if it succeeded; change its signature to async showReleaseNotes(): Promise<boolean>, return true after successfully creating and displaying the webview (e.g., after panel.webview.html = htmlContent) and return false in the catch block (or rethrow if desired), and update callers such as showReleaseNotesIfFirstTime to await the boolean result and only persist the "shown" flag when true; ensure references to this.extensionContext and panel lifecycle remain unchanged.
18-20: Floating promise: async method called without awaiting or error handling.
showReleaseNotesIfFirstTime()is async but called withoutawaitor.catch(). Any unhandled rejection will be silently lost. Consider adding explicit error handling.♻️ Proposed fix
- initialize(): void { - this.showReleaseNotesIfFirstTime(); + initialize(): void { + this.showReleaseNotesIfFirstTime().catch((error) => { + console.error('Failed to show release notes:', error); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/release-notes-manager.ts` around lines 18 - 20, The initialize() method calls the async showReleaseNotesIfFirstTime() without awaiting or handling rejections; update initialize() to either be async and await showReleaseNotesIfFirstTime() or explicitly attach .catch(...) to the returned promise to handle errors (e.g., log via the extension logger or vscode.window.showErrorMessage). Ensure you reference initialize() and showReleaseNotesIfFirstTime() so the change is applied where the call occurs and that any thrown errors are surfaced instead of becoming unhandled rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ide/vscode/src/extension/release-notes-manager.ts`:
- Around line 25-33: The current flow in showReleaseNotesIfFirstTime sets the
zmodelPreviewReleaseNoteKey flag unconditionally after calling
showReleaseNotes(), which can hide the announcement if showReleaseNotes() failed
internally; change the call so that showReleaseNotes() returns a success boolean
(or rethrow errors) and only call extensionContext.globalState.update(...) and
setKeysForSync(...) when that success is true (i.e., after the HTML has loaded
successfully), updating showReleaseNotes implementation to resolve true on
success and false or throw on failure so showReleaseNotesIfFirstTime can rely on
its result.
---
Nitpick comments:
In `@packages/ide/vscode/src/extension/release-notes-manager.ts`:
- Around line 54-57: The webview panel created in showReleaseNotes() isn't
stored and retainContextWhenHidden is true; update showReleaseNotes() to assign
the created WebviewPanel to a module/class-level variable (e.g., this.panel or
releaseNotesPanel) and set retainContextWhenHidden to false for a static release
notes page, then implement/augment dispose() to check and call panel.dispose()
and null out the reference so the panel is properly cleaned up when the
extension deactivates; apply the same pattern to the other panel creation at
lines 74-76.
- Around line 39-68: The showReleaseNotes method currently returns void so
callers cannot know if it succeeded; change its signature to async
showReleaseNotes(): Promise<boolean>, return true after successfully creating
and displaying the webview (e.g., after panel.webview.html = htmlContent) and
return false in the catch block (or rethrow if desired), and update callers such
as showReleaseNotesIfFirstTime to await the boolean result and only persist the
"shown" flag when true; ensure references to this.extensionContext and panel
lifecycle remain unchanged.
- Around line 18-20: The initialize() method calls the async
showReleaseNotesIfFirstTime() without awaiting or handling rejections; update
initialize() to either be async and await showReleaseNotesIfFirstTime() or
explicitly attach .catch(...) to the returned promise to handle errors (e.g.,
log via the extension logger or vscode.window.showErrorMessage). Ensure you
reference initialize() and showReleaseNotesIfFirstTime() so the change is
applied where the call occurs and that any thrown errors are surfaced instead of
becoming unhandled rejections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ee3232e-3430-4f7e-b237-e084228435a0
📒 Files selected for processing (2)
packages/ide/vscode/src/extension/release-notes-manager.tspackages/language/res/zmodel-v3-preview-release-notes.html
✅ Files skipped from review due to trivial changes (1)
- packages/language/res/zmodel-v3-preview-release-notes.html
|
Claude finished @ymc9's task —— View job PR Review: ZModel Markdown Preview FeatureI'll review this PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds a comprehensive markdown preview feature for ZModel files in VS Code, including authentication, documentation caching, and release notes management. The implementation is well-structured with good separation of concerns. ✅ Strengths
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ide/vscode/src/language-server/main.ts (1)
58-61:⚠️ Potential issue | 🔴 CriticalURI serialization will break client code that expects
.pathproperty.The
importedURIsarray contains LangiumURIobjects. When serialized over JSON-RPC, vscode-uri'stoJSON()returns the URI as a string (e.g.,"file:///path/to/file.zmodel"), not an object. The client atzmodel-preview.ts:195callsvscode.Uri.file(uri.path), but strings have no.pathproperty, resulting inundefined.Explicitly map to objects with the shape the client expects:
🐛 Proposed fix
return { hasSyntaxErrors, - importedURIs, + importedURIs: importedURIs.map((uri) => ({ path: uri.path })), };,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/language-server/main.ts` around lines 58 - 61, The returned importedURIs currently contains Langium URI objects which serialize to strings and break client code that expects a .path property (see client use in zmodel-preview.ts:195); change the return to map importedURIs to plain objects with the shape the client expects (e.g., importedURIs.map(u => ({ path: u.path }))) so the JSON-RPC payload includes a .path string; update the return in the function that returns { hasSyntaxErrors, importedURIs } to return the mapped array instead.
🧹 Nitpick comments (1)
packages/ide/vscode/src/language-server/main.ts (1)
87-87: Prefer strict equality (!==) over loose equality (!=).Both operands are strings so this is functionally correct, but strict equality is the idiomatic choice in TypeScript.
- .filter((x) => uriString != x) + .filter((x) => uriString !== x)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/language-server/main.ts` at line 87, The filter callback currently uses loose inequality (uriString != x); change it to strict inequality (uriString !== x) to use TypeScript's idiomatic strict comparison—update the arrow function passed to filter (the expression using uriString and x) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ide/vscode/src/language-server/main.ts`:
- Around line 58-61: The returned importedURIs currently contains Langium URI
objects which serialize to strings and break client code that expects a .path
property (see client use in zmodel-preview.ts:195); change the return to map
importedURIs to plain objects with the shape the client expects (e.g.,
importedURIs.map(u => ({ path: u.path }))) so the JSON-RPC payload includes a
.path string; update the return in the function that returns { hasSyntaxErrors,
importedURIs } to return the mapped array instead.
---
Nitpick comments:
In `@packages/ide/vscode/src/language-server/main.ts`:
- Line 87: The filter callback currently uses loose inequality (uriString != x);
change it to strict inequality (uriString !== x) to use TypeScript's idiomatic
strict comparison—update the arrow function passed to filter (the expression
using uriString and x) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 791c3ce0-bd49-4fc6-9cda-8e55f4767d77
📒 Files selected for processing (1)
packages/ide/vscode/src/language-server/main.ts
|
Claude finished @ymc9's task —— View job PR Review: ZModel Markdown Preview FeatureI'll review this PR focusing on code quality, best practices, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds a comprehensive markdown preview feature for ZModel files in VS Code, including authentication, documentation caching, and release notes management. The implementation is well-structured with good separation of concerns. ✅ Strengths
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/ide/vscode/src/extension/zenstack-auth-provider.ts (2)
183-184:⚠️ Potential issue | 🟠 MajorAvoid using email as telemetry identifier; handle missing email gracefully.
claims.emailis optional, so the non-null assertion (!) will throw if the token lacks an email claim. Additionally, email is PII and shouldn't be used as the telemetry identifier. Per the relevant code snippet, this value is passed directly to Mixpanel as$identified_id. Useclaims.sub(the stable user ID) instead.Suggested fix
const claims = this.parseJWTClaims(accessToken); - telemetry.identify(claims.email!); + if (claims.sub) { + telemetry.identify(claims.sub); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts` around lines 183 - 184, The code calls telemetry.identify(claims.email!) which uses a nullable PII field and force-asserts it; change this to use the stable, non-PII subject claim and handle missing values: retrieve claims via parseJWTClaims(accessToken), use claims.sub as the identifier passed to telemetry.identify, and guard against a missing sub (e.g., only call telemetry.identify if claims.sub is present or fall back to a safe anonymous id) instead of using claims.email with a non-null assertion.
88-95:⚠️ Potential issue | 🟠 MajorLoad persisted sessions before checking, and await all removals.
This function checks
this._sessions.lengthbefore loading from storage. On fresh activation,_sessionsis empty even if persisted sessions exist, causing an early return. Additionally,forEach(async ...)doesn't await the removals, so the success message shows before logout completes.Suggested fix
async logoutAllSessions(): Promise<void> { - if (this._sessions.length === 0) { + const sessions = await this.getSessions(); + if (sessions.length === 0) { return; } - (await this.getSessions()).forEach(async (s) => await this.removeSession(s.id)); + await Promise.all(sessions.map((s) => this.removeSession(s.id))); vscode.window.showInformationMessage('Successfully logged out of ZenStack.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts` around lines 88 - 95, The logoutAllSessions method currently returns early by checking this._sessions.length before loading persisted sessions and uses forEach(async ...) which doesn't await removals; fix by first calling and awaiting this.getSessions() to load persisted sessions (ensure to use its result rather than this._sessions for the emptiness check), then perform removals with Promise.all over mapped calls to this.removeSession(s.id) to await all deletions, and only call vscode.window.showInformationMessage after all removals complete; reference logoutAllSessions, this._sessions, getSessions, removeSession, and vscode.window.showInformationMessage when making the changes.
🧹 Nitpick comments (2)
packages/ide/vscode/src/extension/zenstack-auth-provider.ts (2)
143-148: Timeout timer continues running after successful authentication.The 2-minute timeout timer is never cancelled when authentication succeeds via
handleAuthCallback. While thependingAuthcheck prevents the reject from firing, the timer reference leaks until it eventually fires.Suggested fix - store and clear the timer
+ let timeoutId: NodeJS.Timeout; + // Store the state and resolve function for later use - this.pendingAuth = { resolve, reject, scopes }; + this.pendingAuth = { + resolve: (session) => { + clearTimeout(timeoutId); + resolve(session); + }, + reject: (error) => { + clearTimeout(timeoutId); + reject(error); + }, + scopes, + }; // ... // 2 minutes timeout - setTimeout(() => { + timeoutId = setTimeout(() => { if (this.pendingAuth) { delete this.pendingAuth; } reject(new Error('Timeout')); }, 120000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts` around lines 143 - 148, The timeout created in the authentication flow should be stored and cleared to avoid leaking the timer; modify the code that starts the 2-minute timer (the block that sets setTimeout and references this.pendingAuth) to save the timer id on the pendingAuth object (e.g., this.pendingAuth.timer) and in handleAuthCallback (the method that completes successful authentication) clear that timer via clearTimeout and remove the timer reference when resolving; also ensure any code path that deletes this.pendingAuth clears the timer first so the timeout cannot fire after success.
186-186: Consider usingcrypto.randomUUID()for session ID fallback.
Math.random().toString(36)provides weak uniqueness guarantees. Node.js (and VS Code's runtime) supportscrypto.randomUUID()which generates proper UUIDs.Suggested fix
+import * as crypto from 'crypto'; // ... - id: claims.jti || Math.random().toString(36), + id: claims.jti || crypto.randomUUID(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts` at line 186, Replace the weak fallback Math.random().toString(36) used for the session id in zenstack-auth-provider.ts with a proper UUID from the crypto API: change the id assignment (currently id: claims.jti || Math.random().toString(36)) to use crypto.randomUUID() as the fallback (e.g., id: claims.jti || crypto.randomUUID()); ensure the module has access to crypto (import { randomUUID } from 'crypto' or call globalThis.crypto.randomUUID() depending on runtime/tsconfig) and update any types if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts`:
- Around line 183-184: The code calls telemetry.identify(claims.email!) which
uses a nullable PII field and force-asserts it; change this to use the stable,
non-PII subject claim and handle missing values: retrieve claims via
parseJWTClaims(accessToken), use claims.sub as the identifier passed to
telemetry.identify, and guard against a missing sub (e.g., only call
telemetry.identify if claims.sub is present or fall back to a safe anonymous id)
instead of using claims.email with a non-null assertion.
- Around line 88-95: The logoutAllSessions method currently returns early by
checking this._sessions.length before loading persisted sessions and uses
forEach(async ...) which doesn't await removals; fix by first calling and
awaiting this.getSessions() to load persisted sessions (ensure to use its result
rather than this._sessions for the emptiness check), then perform removals with
Promise.all over mapped calls to this.removeSession(s.id) to await all
deletions, and only call vscode.window.showInformationMessage after all removals
complete; reference logoutAllSessions, this._sessions, getSessions,
removeSession, and vscode.window.showInformationMessage when making the changes.
---
Nitpick comments:
In `@packages/ide/vscode/src/extension/zenstack-auth-provider.ts`:
- Around line 143-148: The timeout created in the authentication flow should be
stored and cleared to avoid leaking the timer; modify the code that starts the
2-minute timer (the block that sets setTimeout and references this.pendingAuth)
to save the timer id on the pendingAuth object (e.g., this.pendingAuth.timer)
and in handleAuthCallback (the method that completes successful authentication)
clear that timer via clearTimeout and remove the timer reference when resolving;
also ensure any code path that deletes this.pendingAuth clears the timer first
so the timeout cannot fire after success.
- Line 186: Replace the weak fallback Math.random().toString(36) used for the
session id in zenstack-auth-provider.ts with a proper UUID from the crypto API:
change the id assignment (currently id: claims.jti ||
Math.random().toString(36)) to use crypto.randomUUID() as the fallback (e.g.,
id: claims.jti || crypto.randomUUID()); ensure the module has access to crypto
(import { randomUUID } from 'crypto' or call globalThis.crypto.randomUUID()
depending on runtime/tsconfig) and update any types if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 004ae9dd-ae01-4667-8c40-5b48fb86245d
📒 Files selected for processing (1)
packages/ide/vscode/src/extension/zenstack-auth-provider.ts
2 participants
Summary by CodeRabbit
New Features
Documentation
Commands