integrated webview manager to work with webviewview alongside panel. #41
integrated webview manager to work with webviewview alongside panel. #41
Conversation
…Added functionality to add webview containers (webviewview/webviewpanel) to the manager so it could work with webviewview provider. Fixed syntax errors in rendering html
| context.subscriptions.push( | ||
| vscode.window.registerWebviewViewProvider("codescape.Cityview", provider), | ||
| ); | ||
| // const create = vscode.commands.registerCommand("codescape.createPanel", () => |
| } | ||
|
|
||
| function createPanel(context : vscode.ExtensionContext, store: FileParseStore){ | ||
| // function createPanel(context : vscode.ExtensionContext, javaWatcher : JavaFileWatcher, store: FileParseStore){ |
There was a problem hiding this comment.
Pull request overview
This PR updates the extension’s webview orchestration so a single manager can track and broadcast state to both WebviewPanel instances (commands) and a WebviewView (sidebar provider), aiming to unify multi-view rendering and state sync.
Changes:
- Refactors
WebviewManagerto manage a generic “webview container” (WebviewVieworWebviewPanel) and adds anaddWebview(...)entry point. - Updates extension activation commands to create panels via
createPanel(...)and registers the sidebar view with the manager. - Tweaks file watcher internals (rename java watcher field, guard a Python delete path).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/extension.ts | Switches commands to createPanel, registers sidebar WebviewView with manager, and adjusts/extends webview script logging and rendering code. |
| src/WebviewManager.ts | Adds support for managing both WebviewPanel and WebviewView, and uses shared getWebviewContent(...) to set HTML. |
| src/JavaFileWatcher.ts | Renames the Java watcher field and adds a “no webviews yet” guard in the Python delete handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fileData = msg.payload.files; | ||
| render(); | ||
| } else if (msg.type === 'PARTIAL_STATE' && msg.payload) { | ||
| console.log("PARTIAL STATE CHANGE RECIEVE") | ||
| //create default values because may not exist in payload | ||
| const { changed = [], related = [], removed = [] } = msg.payload; |
There was a problem hiding this comment.
The message handler assigns to and later exports fileData, but fileData is not declared anywhere in this script (earlier comment says state replaces it). This will throw at runtime on the first FULL_STATE/AST_DATA message or export click. Either declare let fileData = [] (and keep it in sync), or migrate these code paths to use state.classes instead of fileData.
| // WebviewView is already ready when provider gives it to us | ||
| if ('onDidChangeVisibility' in container) { | ||
| managedWebview.isReady = true; | ||
| if (this.lastFullState) { | ||
| container.webview.postMessage({ | ||
| type: 'FULL_STATE', | ||
| payload: this.lastFullState, | ||
| }); | ||
| } | ||
| } | ||
| container.webview.onDidReceiveMessage((message) => { |
There was a problem hiding this comment.
addWebview() marks a WebviewView as ready based on 'onDidChangeVisibility' in container and immediately posts FULL_STATE. Messages posted before the webview script runs / registers its window.addEventListener('message', ...) handler can be dropped, which is why the READY handshake exists. Consider treating WebviewView the same as panels: wait for the webview to send { type: 'READY' } before setting isReady and sending FULL_STATE (or buffer via messageQueue and flush on READY).
| // WebviewView is already ready when provider gives it to us | |
| if ('onDidChangeVisibility' in container) { | |
| managedWebview.isReady = true; | |
| if (this.lastFullState) { | |
| container.webview.postMessage({ | |
| type: 'FULL_STATE', | |
| payload: this.lastFullState, | |
| }); | |
| } | |
| } | |
| container.webview.onDidReceiveMessage((message) => { | |
| container.webview.onDidReceiveMessage((message) => { |
There was a problem hiding this comment.
webviewview handshake already completed when provider returns it. Therefore needed way to add a webviewview without needing necessarily requiring handshake.
| import * as vscode from 'vscode'; | ||
| import { ClassInfo } from './parser/javaExtractor'; | ||
| import {getWebviewContent} from './extension' |
There was a problem hiding this comment.
WebviewManager now imports getWebviewContent from ./extension, while extension.ts imports WebviewManager, creating a circular dependency that can make getWebviewContent undefined at runtime (depending on module initialization order). Move getWebviewContent into a separate module (e.g., src/webview/getWebviewContent.ts) that both files import, or keep the HTML builder inside WebviewManager to avoid the cycle.
| disposeAll(): void { | ||
| for (const managed of this.webviews.values()) { | ||
| managed.panel.dispose(); | ||
| for (const id of this.webviews.keys()) { | ||
| let managed = this.webviews.get(id); | ||
| if(managed != null && 'dispose' in managed.container){ | ||
| managed.container.dispose(); | ||
| this.webviews.delete(id); | ||
| } | ||
| this.webviews.clear(); | ||
| } |
There was a problem hiding this comment.
disposeAll() only disposes/deletes entries when the container has a dispose() method; WebviewView instances won't be removed from this.webviews, so getActiveViewCount() will remain non-zero and broadcasts may target disposed views. Consider always deleting map entries, and only calling dispose() when available (also avoid mutating the map while iterating its keys).
| // Registry of rendered buildings for hit detection (hover/click). | ||
| // Each entry is tracked in canvas/world coordinates before zoom. | ||
| const buildingRegistry = []; | ||
| //NOTE: THIS STOPS RENDER FROM RUNNING | ||
| //const buildingRegistry = []; |
There was a problem hiding this comment.
The comment and commented-out buildingRegistry redeclaration should be removed rather than left in-place. The actual issue is that buildingRegistry is already declared earlier (let buildingRegistry = []), so redeclaring it here would cause a runtime/parse error; keeping this commented code and note is misleading and increases maintenance burden.
… fileWatcherFixes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added functionality to add webview containers (webviewview/webviewpanel) to the manager so it could work with webviewview provider. Fixed syntax errors in rendering html.
PS: pls try and run extension to ensure pushed code doesnt break anything in future