Conversation
Co-authored-by: Codex
- Introduced two new snippets: `stac screen` for creating a new Stac screen template and `stac theme` for creating a new Stac theme template. - Updated the extension description and documentation to reflect the new snippets. - Removed the old snippet catalog as it is no longer needed with the new implementation. - Adjusted the snippet completion provider to support the new snippet triggers.
|
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 new stac-vscode VS Code extension under tools/stac-vscode: extension manifest and docs, workspace dev tooling, a Flutter web preview host, asset server, preview orchestration (host process, webview, JSON pipeline), wrap/snippet providers, utilities, tests, ESLint, and license. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Ext as "VS Code Extension\n(PreviewManager)"
participant Asset as "AssetServer"
participant Host as "PreviewHostProcess\n(Flutter host)"
participant Web as "Webview / iframe"
User->>Ext: open preview / edit / save
Ext->>Asset: start(workspaceRoot) -> returns port
Ext->>Host: ensureStarted() -> returns hostUrl/port
Ext->>Web: postMessage(render payload { json, themes, fonts, assetPort })
Web->>Host: forward stac.preview.render (iframe -> host)
Host-->>Web: stac.preview.rendered / stac.preview.error
Web->>Ext: relay rendered / ready / error events
Ext->>User: update status / show messages
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tools/stac-vscode/eslint.config.mjs`:
- Around line 3-27: The second config block (the object containing plugins,
languageOptions, and rules) lacks a files filter so its TypeScript-specific
parser and rules apply to all files; fix by merging the two objects or adding
files: ["**/*.ts"] to that second object so the TypeScript parser and rules
(plugins/@typescript-eslint, languageOptions.parser, and rules like
"@typescript-eslint/naming-convention", curly, eqeqeq, no-throw-literal, semi)
only apply to .ts files.
🧹 Nitpick comments (3)
tools/stac-vscode/.vscodeignore (1)
1-11: Consider excluding test output from the packaged extension.The compiled test files under
out/test/will be included in the VSIX package since onlysrc/**(source) is excluded but not the compiled test output. Adding an exclusion prevents unnecessary bloat.Suggested addition
**/.vscode-test.* +out/test/**tools/stac-vscode/README.md (1)
39-42: Dart version requirement is valid—consider if3.9.2+is the true minimum.Dart 3.9.2 is a legitimate, stable release (August 2025). However, if the extension works with earlier Dart 3.x versions, consider lowering the minimum requirement to improve adoption and compatibility for users on stable versions.
tools/stac-vscode/CHANGELOG.md (1)
5-5: Consider adding a release date to the version heading.Changelogs conventionally include a date next to the version number (e.g.,
## [0.1.0] - 2026-02-13) per Keep a Changelog conventions.
- Added entries to ignore node_modules directory and VS Code extension output files. - Included .eslintcache to prevent tracking of linting cache files.
… add widget removal by extracting child expressions.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@tools/stac-vscode/preview_host/lib/main.dart`:
- Around line 202-209: The _fetchFont function currently calls http.get without
a timeout which can hang and block _loadFonts (which uses Future.wait); wrap the
http.get(Uri.parse(url)) call with a timeout (e.g., .timeout(Duration(seconds:
X))) and catch TimeoutException (and other errors) to throw a clear Exception
like "Timed out loading font from $url" so callers don't wait indefinitely;
update _fetchFont to apply the timeout and convert timeout/errors into
descriptive exceptions so _loadFonts won't hang.
- Line 35: The event listener leak is caused by calling _onMessage.toJS inline
(in web.window.addEventListener and removeEventListener) which creates distinct
JS wrapper functions each time; fix it by creating and storing a single
JS-wrapped callback field (e.g. _onMessageJs) when the listener is first
attached and then reuse that same field for both
web.window.addEventListener(_onMessageJs) and
web.window.removeEventListener(_onMessageJs) (ensure the wrapper is created
once, cleared on dispose() and referenced in the code paths that currently call
_onMessage.toJS).
In `@tools/stac-vscode/src/extension.ts`:
- Around line 128-132: The guard `if (!edit)` is ineffective because
createRemoveWidgetEdit always returns a WorkspaceEdit; replace it with a check
for no-op edits by testing edit.size === 0 (or another WorkspaceEdit emptiness
check) so the early return only triggers when there are no actual edits; update
the code around the variables document/target and the call to
createRemoveWidgetEdit to use this emptiness check instead of truthiness.
- Around line 85-138: The wrapWithStacWidget command disposable
(customDisposable) is never pushed to context.subscriptions causing a leak;
after registering the command via
vscode.commands.registerCommand(COMMANDS.wrapWithStacWidget, ...) add
context.subscriptions.push(customDisposable) (i.e., push the customDisposable
the same way removeDisposable and other disposables are pushed) so the command
is disposed when the extension deactivates.
In `@tools/stac-vscode/src/preview/assetServer.ts`:
- Around line 80-81: The createReadStream call in assetServer.ts can emit an
error if the file is deleted after statSync; attach an 'error' handler to the
stream returned by fs.createReadStream (the variable stream) that handles stream
errors by setting the response status (e.g., 404 or 500 as appropriate), logging
the error, and ending the response (res.end()) to avoid an unhandled exception
and leaking the response; also ensure you remove or ignore further piping after
an error and do not rely solely on stream.pipe(res) without the error handler.
- Around line 89-96: The Promise in AssetServer.start (assetServer.ts) registers
the 'error' handler after calling this.server!.listen, which can miss
synchronous listen errors; move the this.server!.on('error', ...) registration
to before this.server!.listen (and optionally remove the handler once listening
succeeds) and resolve only after the listen callback or 'listening' event fires
so the error handler is always attached before attempting to bind.
- Around line 48-50: The current regex-based safePath is fragile; replace it by
resolving the requested path against workspaceRoot (use path.resolve or
path.join + path.resolve) using url.pathname (ensure you prepend a '.' or strip
leading slashes appropriately), then compute the resolved file path (replace the
existing safePath/filePath logic) and verify the resolved path is inside
workspaceRoot (use path.relative(workspaceRoot, resolvedPath) and deny if it
starts with '..' or use a startsWith check after ensuring both paths end with
the same separator). If the check fails, return a 403/deny instead of serving
the file. Update the code around the safePath and filePath variables (references
to path.normalize, safePath, filePath, workspaceRoot, and url.pathname)
accordingly.
In `@tools/stac-vscode/src/preview/jsonTransformer.ts`:
- Around line 14-28: The early return for matched StacImage objects (the if
checking json.type === 'image' && json.imageType === 'asset' && typeof json.src
=== 'string') does a shallow spread and misses nested image objects; instead,
before returning, recursively transform the remaining properties so nested
fields (e.g., child, error) are processed. Update the branch to build a new
object that sets imageType to 'network' and src to
`http://127.0.0.1:${assetServerPort}/${json.src.replace(/^\//, '')}` but runs
the same transformer over all other property values (recursing into
objects/arrays) rather than returning the shallow ...json as-is. Ensure you
reference the same asset URL construction and preserve other fields while
applying the recursive transform.
In `@tools/stac-vscode/src/preview/previewHostProcess.ts`:
- Around line 97-105: When canBindPort(this.port) is false and
findAvailablePort(...) returns undefined, the code currently continues and will
attempt to spawn Flutter on a busy port; update the block in the preview host
startup (the method using this.port, canBindPort, and findAvailablePort) to
handle the undefined case by writing a clear error/warning to this.outputChannel
(e.g. appendLine indicating no free ports found) and then aborting startup
(throw an Error or return early) instead of continuing with the original port so
the caller doesn't waste time trying to start Flutter on a known-busy port.
In `@tools/stac-vscode/src/preview/previewManager.ts`:
- Around line 408-413: The asset server isn't restarted when the workspace root
changes because AssetServer.start() is a no-op if a server already exists;
modify previewManager to track the last project root used to start the server
(e.g. add a lastAssetRoot field) and before calling
AssetServer.start(projectRoot) compare projectRoot to lastAssetRoot—if
different, call this.assetServer.stop() (or instantiate a new AssetServer) then
start it with the new projectRoot and update lastAssetRoot; reference the
existing this.assetServer, AssetServer.start and projectRoot in the change so
the server resets whenever the workspace root changes.
In `@tools/stac-vscode/src/preview/previewPanel.ts`:
- Around line 395-406: startDelivery currently starts an unbounded setInterval
that reposts latestRenderMessage and only stops when latestRenderMessage becomes
null or hostReady changes; add a safety limit (e.g., maxAttempts or a TTL
timestamp) inside startDelivery to count attempts or compare elapsed time and
call stopDelivery plus surface an error state/notification when exceeded. Update
references: modify startDelivery to initialize an attempts counter (or deadline)
and increment/compare it inside the interval that calls
postRenderToFrame(latestRenderMessage); when the limit is reached clear
deliveryTimer and invoke stopDelivery and a handler to mark the preview as
failed (or show an error to the user) so deliveryTimer cannot run indefinitely.
Ensure deliveryTimer and latestRenderMessage semantics remain unchanged
elsewhere.
In `@tools/stac-vscode/src/test/preview_assets.test.ts`:
- Around line 46-52: The Promise wrapping the http.get call (the one that
creates response via http.get(`http://127.0.0.1:${port}/test.png`, ...`) can
hang if the request errors; update that Promise to attach an .on('error', ...)
handler to the request so the Promise rejects (or resolves with an error status)
when a connection error occurs, and do the same change for the analogous font
test Promise (the other http.get wrapper around the font request). Ensure you
reference the existing variables (port, response) and keep the original data and
end handlers while adding error handling to prevent the test from hanging.
🧹 Nitpick comments (15)
tools/stac-vscode/preview_host/pubspec.yaml (1)
14-15: Consider pinningwebdependency instead ofany.While
publish_to: nonelimits the blast radius,web: anyaccepts any major version — including future breaking releases. A caret constraint (e.g.,^1.1.0) would prevent silent breakage when thewebpackage ships a major bump.tools/stac-vscode/preview_host/lib/main.dart (1)
92-94: Theme parsing errors are silently swallowed.If
StacTheme.fromJsonorstacTheme.parsethrows, the user gets no feedback about why the theme didn't apply. Consider logging the error via_logso it surfaces in the preview output channel.Proposed fix
- } catch (_) { + } catch (e) { + _log('Failed to parse theme: $e'); return null; }tools/stac-vscode/src/test/font_discovery.test.ts (1)
8-8: Consider usingos.tmpdir()instead of__dirnamefor the temp test directory.Using
__dirnameplaces test artifacts inside the compiled output directory, which could be fragile if the output dir is cleaned between runs or used by other tooling.os.tmpdir()+ a unique suffix (e.g., viamkdtempSync) is the conventional approach for test fixtures.tools/stac-vscode/src/preview/utils.ts (1)
14-25:canBindPortmay hang indefinitely if the TCP stack never responds.If
listenneither succeeds nor errors (e.g., on an unusual network configuration), the returned promise will never settle, stallingfindAvailablePort. Consider adding a timeout to guard against this.⏱️ Proposed timeout guard
export function canBindPort(port: number): Promise<boolean> { return new Promise((resolve) => { const server = net.createServer(); + const timeout = setTimeout(() => { + server.close(); + resolve(false); + }, 1000); server.once('error', () => { + clearTimeout(timeout); resolve(false); }); server.listen(port, '127.0.0.1', () => { + clearTimeout(timeout); server.close(() => { resolve(true); }); }); }); }tools/stac-vscode/src/test/remove.test.ts (1)
6-48: DuplicatecreateMockDocumenthelper across test files.This mock is nearly identical to the one in
wrap.test.ts. Consider extracting it into a shared test utility to avoid duplication.Also applies to: 9-50
tools/stac-vscode/src/wrap/removeWidget.ts (4)
4-4: Unused constantMATCH_CHILD_PROP.This regex is defined but never referenced in the file. Remove it to avoid confusion.
🧹 Proposed fix
-const MATCH_CHILD_PROP = /child:\s*/; -
76-76:substris deprecated — usesubstringorsliceinstead.
String.prototype.substris a legacy method. Replace withsubstring(i, i + childKey.length).♻️ Proposed fix
- if (expression.substr(i, childKey.length) === childKey) { + if (expression.substring(i, i + childKey.length) === childKey) {
161-185: Excessive design-note comments in production code.These read as stream-of-consciousness notes from the implementation phase. Consider trimming to a concise summary of the approach.
14-148: Duplicated state machine for comment/string/nesting tracking.The lexer logic (lines 24–62, 106–126) is repeated almost verbatim. Extracting a shared tokenizer or helper would reduce maintenance burden, but the current approach is functional.
tools/stac-vscode/src/wrap/stacWrapCodeActionProvider.ts (1)
75-76: Nit: extra blank lines.Two consecutive blank lines between the custom action block and the remove action block. One blank line is sufficient for readability.
tools/stac-vscode/src/preview/previewPanel.ts (1)
95-104: Type assertion onpostThemesmessage can be avoided.The
as PreviewThemesMessage & { selectedThemeName: string | null }cast suggestsPreviewThemesMessagedoesn't includeselectedThemeName. Consider extending the type definition intypes.tsto include the field, which would eliminate the need for the cast and keep the contract explicit.tools/stac-vscode/src/extension.ts (1)
46-53: Duplicate command-to-preset mapping.This
commandByPresetobject duplicates theWRAP_COMMANDSrecord instacWrapCodeActionProvider.ts(lines 7-14). Consider extracting the mapping toconstants.tsor reusing the one fromstacWrapCodeActionProvider.tsto keep them in sync.tools/stac-vscode/src/wrap/applyWrapEdit.ts (1)
57-68:appendCommaToLastLinemutates input array in-place.The function modifies
lines[last]directly and also returns the array. Current callers always pass freshly constructed arrays, so this is safe today. But the mutate-and-return pattern can be surprising. A brief doc comment noting the mutation would help future callers.tools/stac-vscode/src/preview/previewManager.ts (2)
256-290:asynckeyword is unnecessary on handlers that don'tawait.
handleDidChangeActiveEditorandhandleDidChangeSelectionare declaredasyncbut contain noawaitexpressions. While harmless, theasynckeyword creates a microtask wrapper and can be misleading since the callers already usevoid this.handle...(). Consider removingasyncfrom these methods.Also applies to: 292-337
652-674:resolveProjectRootuses synchronousexistsSyncon each ancestor directory.This is blocking I/O on the extension host thread. For typical local filesystems and shallow directory depths this is fine, but could cause hitches on remote/networked workspaces (e.g., VS Code Remote - SSH). If remote workspace support is a goal, consider an async variant using
vscode.workspace.fs.stat.
…d UI updates - Removed the old VSIX file for the extension. - Added a request ID to manage state updates in the preview host. - Updated the loading behavior to hide the progress indicator when the webview is ready. - Improved theme handling and caching logic. - Enhanced error logging for better debugging during message processing. - Introduced a retry button in the UI for refreshing the preview.
- Introduced a zoom control feature for the mobile frame in the preview panel, allowing users to zoom in and out. - Enhanced the mobile frame's responsiveness by adjusting its width and height based on the editor pane size. - Updated the CSS styles to improve layout and user experience, including new styles for the zoom control buttons and their interactions. - Implemented JavaScript functionality to manage zoom levels and update the display accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tools/stac-vscode/src/preview/buildFallback.ts (1)
56-86: Shell command has no timeout — a hung build process blocks the preview indefinitely.
runShellCommandspawns a process without any timeout. If the build command hangs or takes excessively long, this promise never resolves and blocks the preview pipeline.Proposed fix: add a configurable timeout
function runShellCommand( command: string, cwd: string, outputChannel: vscode.OutputChannel, + timeoutMs = 120_000, ): Promise<ShellCommandResult> { return new Promise((resolve, reject) => { const child = spawn(command, { cwd, shell: true, env: process.env, }); + + const timer = setTimeout(() => { + child.kill(); + outputChannel.appendLine(`[preview] Build command timed out after ${timeoutMs}ms`); + resolve({ exitCode: 124 }); + }, timeoutMs); child.stdout.on('data', (chunk: Buffer | string) => { outputChannel.append(chunk.toString()); }); child.stderr.on('data', (chunk: Buffer | string) => { outputChannel.append(chunk.toString()); }); child.on('error', (error) => { + clearTimeout(timer); reject(error); }); child.on('close', (code) => { + clearTimeout(timer); resolve({ exitCode: code ?? 1, }); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/src/preview/buildFallback.ts` around lines 56 - 86, runShellCommand currently spawns a child process with no timeout so a hung build blocks forever; update runShellCommand to accept a configurable timeout (e.g. timeoutMs: number | undefined) and start a timer after spawn that, when fired, kills the child (child.kill()) and resolves/rejects the Promise with a ShellCommandResult indicating a timeout (non-zero exitCode and a timedOut flag or error), and ensure you clear the timer on normal 'close' and 'error' events to avoid leaks; update callers to pass a sensible default timeout or undefined to preserve behaviour.tools/stac-vscode/src/preview/jsonGeneration.ts (1)
120-151: Shell command has no timeout — consistent withbuildFallback.ts.Same issue as in
buildFallback.ts: therunCommandhelper has no timeout. If the Dart runner hangs, the preview pipeline blocks indefinitely. Consider adding a timeout similar to the one inpreviewManager.ts'srunDartCommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/src/preview/jsonGeneration.ts` around lines 120 - 151, The runCommand function lacks a timeout and can hang; update runCommand to accept or implement a timeout (like previewManager's runDartCommand) so the spawned child is killed and the promise is rejected if the command exceeds the timeout. Specifically, in runCommand (in jsonGeneration.ts) start a timer after spawn that calls child.kill() (or child.kill('SIGKILL') on non-Windows), clears listeners, rejects with a TimeoutError, and ensure the timer is cleared on child 'close'/'error' to avoid leaks; mirror the same timeout behavior and error handling used by previewManager.runDartCommand (and consistent with buildFallback.ts) so callers get a deterministic exit on hangs.tools/stac-vscode/preview_host/lib/main.dart (2)
101-104: Theme cache comparison uses reference equality, which may miss cache hits.
_themeJson == _lastThemeJsoncompares Map references, not contents. If the same theme data arrives in a new Map instance (e.g., from a fresh message), the cache is bypassed and the theme is re-parsed. This isn't incorrect but suboptimal.Consider using
mapEqualsfrompackage:flutter/foundation.dartor caching by theme name instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/preview_host/lib/main.dart` around lines 101 - 104, The cache comparison currently uses reference equality between _themeJson and _lastThemeJson (and _targetPlatform/_lastTargetPlatform) so identical Map contents in new instances miss cache hits; update the check to compare Map contents using mapEquals from package:flutter/foundation.dart (or swap to a name-based cache key) and import that package, i.e., replace the `_themeJson == _lastThemeJson` test with `mapEquals(_themeJson, _lastThemeJson)` (keeping the platform comparison) so `_cachedThemeData` is reused when contents match.
123-125: Silent exception swallowing hides theme parsing errors.The catch block discards the exception without logging. If
StacTheme.fromJsonorstacTheme.parsefails, the user gets no feedback about why theming isn't working.Proposed fix: log the error
} catch (e) { + _log('Failed to build theme: $e'); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/preview_host/lib/main.dart` around lines 123 - 125, The catch block currently swallows exceptions when parsing themes (around StacTheme.fromJson and stacTheme.parse) causing silent failures; update the catch to log the caught exception (including stack trace or message) via the existing logger or console to provide diagnostic information, e.g., capture the exception variable in the catch and call the project's logging facility (or print) with a clear message that theme parsing failed and include the error details and stack trace so users/devs can see why theming didn't apply.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/stac-vscode/preview_host/lib/main.dart`:
- Around line 101-104: The cache comparison currently uses reference equality
between _themeJson and _lastThemeJson (and _targetPlatform/_lastTargetPlatform)
so identical Map contents in new instances miss cache hits; update the check to
compare Map contents using mapEquals from package:flutter/foundation.dart (or
swap to a name-based cache key) and import that package, i.e., replace the
`_themeJson == _lastThemeJson` test with `mapEquals(_themeJson, _lastThemeJson)`
(keeping the platform comparison) so `_cachedThemeData` is reused when contents
match.
- Around line 123-125: The catch block currently swallows exceptions when
parsing themes (around StacTheme.fromJson and stacTheme.parse) causing silent
failures; update the catch to log the caught exception (including stack trace or
message) via the existing logger or console to provide diagnostic information,
e.g., capture the exception variable in the catch and call the project's logging
facility (or print) with a clear message that theme parsing failed and include
the error details and stack trace so users/devs can see why theming didn't
apply.
In `@tools/stac-vscode/src/preview/buildFallback.ts`:
- Around line 56-86: runShellCommand currently spawns a child process with no
timeout so a hung build blocks forever; update runShellCommand to accept a
configurable timeout (e.g. timeoutMs: number | undefined) and start a timer
after spawn that, when fired, kills the child (child.kill()) and
resolves/rejects the Promise with a ShellCommandResult indicating a timeout
(non-zero exitCode and a timedOut flag or error), and ensure you clear the timer
on normal 'close' and 'error' events to avoid leaks; update callers to pass a
sensible default timeout or undefined to preserve behaviour.
In `@tools/stac-vscode/src/preview/jsonGeneration.ts`:
- Around line 120-151: The runCommand function lacks a timeout and can hang;
update runCommand to accept or implement a timeout (like previewManager's
runDartCommand) so the spawned child is killed and the promise is rejected if
the command exceeds the timeout. Specifically, in runCommand (in
jsonGeneration.ts) start a timer after spawn that calls child.kill() (or
child.kill('SIGKILL') on non-Windows), clears listeners, rejects with a
TimeoutError, and ensure the timer is cleared on child 'close'/'error' to avoid
leaks; mirror the same timeout behavior and error handling used by
previewManager.runDartCommand (and consistent with buildFallback.ts) so callers
get a deterministic exit on hangs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a21ed3b-51c4-4025-8dcb-ff4eb9eff084
📒 Files selected for processing (8)
.gitignoretools/stac-vscode/preview_host/lib/main.darttools/stac-vscode/src/extension.tstools/stac-vscode/src/preview/buildFallback.tstools/stac-vscode/src/preview/jsonGeneration.tstools/stac-vscode/src/preview/previewHostProcess.tstools/stac-vscode/src/preview/previewManager.tstools/stac-vscode/src/preview/previewPanel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
| } | ||
| } | ||
|
|
||
| Map<String, dynamic>? _normalize(dynamic raw) { |
There was a problem hiding this comment.
Function name can be better
…n across the STAC ecosystem.
…entation, and example app configurations, while adding new parser and network service tests.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tools/stac-vscode/preview_host/lib/main.dart (1)
279-284: Consider validating parent window existence pattern.The
_postmethod correctly checksparent != nullbefore posting. However,web.window.parentcan return the window itself when there's no parent frame. Consider also checkingparent != web.windowfor robustness in edge cases.💡 Suggested improvement
void _post(Map<String, dynamic> payload) { final parent = web.window.parent; - if (parent != null) { + if (parent != null && parent != web.window) { parent.postMessage(jsonEncode(payload).toJS, '*'.toJS); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/preview_host/lib/main.dart` around lines 279 - 284, The _post method currently only checks parent != null before calling parent.postMessage; update it to also ensure the parent is not the same window (check parent != web.window) to avoid posting to self when no frame exists. Locate the _post function and change the guard to something like if (parent != null && parent != web.window) before calling parent.postMessage(jsonEncode(payload).toJS, '*'.toJS) so postMessage is only invoked on a true parent frame.tools/stac-vscode/src/preview/previewPanel.ts (1)
466-474: Using'*'for postMessage targetOrigin is intentional but worth documenting.The
'*'targetOrigin is necessary because VS Code's webview port-mapping proxies the iframe through an internal origin that differs from the original URL. The inline comment on line 516-518 explains this forpostRenderToFrame, but consider adding a similar brief comment here for consistency.💡 Suggested documentation
if (frame.contentWindow) { try { + // Use '*' because VS Code's webview port-mapping proxies the + // iframe through an internal origin. frame.contentWindow.postMessage( { type: 'stac.preview.setPlatform', platform: platformMap[device] || 'web' }, '*', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/src/preview/previewPanel.ts` around lines 466 - 474, Add a brief inline comment above the postMessage call in previewPanel.ts (around the platformMap/frame.contentWindow block) explaining that using '*' for the targetOrigin is intentional because VS Code's webview port-mapping proxies the iframe through an internal origin differing from the original URL, so a specific origin cannot be used; reference the postMessage call and the 'stac.preview.setPlatform' message to make the intent clear for future readers.tools/stac-vscode/src/preview/previewHostProcess.ts (1)
149-160: Consider clearing startupPromise on process error.If the process emits an
errorevent beforewaitForHostHealthyresolves/rejects,this.startupPromisemay remain set whilethis.processbecomes undefined, potentially causing state inconsistency on the nextensureStarted()call.💡 Potential improvement
The
startupPromiseis already cleared in thefinallyblock ofensureStarted()(lines 63-65), so this is handled. However, theerrorevent handler could log more context about the startup phase:this.process.on('error', (error) => { - this.outputChannel.appendLine(`[preview] Flutter preview host error: ${String(error)}`); + this.outputChannel.appendLine(`[preview] Flutter preview host error during startup: ${String(error)}`); this.process = undefined; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/stac-vscode/src/preview/previewHostProcess.ts` around lines 149 - 160, The process 'error' handler can leave this.startupPromise set if the error happens before waitForHostHealthy resolves; update the error handler (the function attached to this.process.on('error')) to clear this.startupPromise (set to undefined) when an error occurs and enhance the log to include that the error happened during startup (mentioning waitForHostHealthy/ensureStarted) so callers know startup failed; keep existing this.process = undefined behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/stac-vscode/preview_host/lib/main.dart`:
- Around line 279-284: The _post method currently only checks parent != null
before calling parent.postMessage; update it to also ensure the parent is not
the same window (check parent != web.window) to avoid posting to self when no
frame exists. Locate the _post function and change the guard to something like
if (parent != null && parent != web.window) before calling
parent.postMessage(jsonEncode(payload).toJS, '*'.toJS) so postMessage is only
invoked on a true parent frame.
In `@tools/stac-vscode/src/preview/previewHostProcess.ts`:
- Around line 149-160: The process 'error' handler can leave this.startupPromise
set if the error happens before waitForHostHealthy resolves; update the error
handler (the function attached to this.process.on('error')) to clear
this.startupPromise (set to undefined) when an error occurs and enhance the log
to include that the error happened during startup (mentioning
waitForHostHealthy/ensureStarted) so callers know startup failed; keep existing
this.process = undefined behavior intact.
In `@tools/stac-vscode/src/preview/previewPanel.ts`:
- Around line 466-474: Add a brief inline comment above the postMessage call in
previewPanel.ts (around the platformMap/frame.contentWindow block) explaining
that using '*' for the targetOrigin is intentional because VS Code's webview
port-mapping proxies the iframe through an internal origin differing from the
original URL, so a specific origin cannot be used; reference the postMessage
call and the 'stac.preview.setPlatform' message to make the intent clear for
future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cf97ba3-e5e6-40dd-a8f6-f80d0af7a3a1
⛔ Files ignored due to path filters (1)
tools/stac-vscode/preview_host/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
tools/stac-vscode/preview_host/lib/main.darttools/stac-vscode/src/preview/assetServer.tstools/stac-vscode/src/preview/jsonTransformer.tstools/stac-vscode/src/preview/previewHostProcess.tstools/stac-vscode/src/preview/previewManager.tstools/stac-vscode/src/preview/previewPanel.tstools/stac-vscode/src/test/preview_assets.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/stac-vscode/src/preview/assetServer.ts
…ion, and tooling.
… and CLI commands.
…ion, and VS Code extension.
2 participants
Description
This PR introduces the initial implementation of the Stac VS Code extension, enabling developers to build and preview Server-Driven UI screens using the Stac framework directly inside VS Code.
Key features included in this PR:
Live Preview
@StacScreenannotated screens.@StacThemeRefannotations.Wrap Quick Fixes
Adds quick-fix actions (Cmd+.) to wrap widgets with Stac components.
Includes preset wrappers such as:
StacContainerStacPaddingStacCenterStacAlignStacSizedBoxStacExpandedSupports wrapping with any Stac widget using dynamic catalog generation.
Snippets
Adds context-aware snippets:
stac screen— create new Stac screen templatestac theme— create new Stac theme templateSnippets are only shown in Stac DSL contexts.
Type of Change
Summary by CodeRabbit
New Features
Documentation
Tests
Chores