add real-time collaborative editing support for devant editor#1975
Open
vinukab wants to merge 397 commits into
Open
add real-time collaborative editing support for devant editor#1975vinukab wants to merge 397 commits into
vinukab wants to merge 397 commits into
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 208 files, which is 58 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (208)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds end-to-end collaborative editing: an Open Collaboration Protocol + Yjs stack, an OCT VS Code extension, and integration into the Ballerina extension — including node locks, cursor/presence types and RPCs, a CollaborationLockManager, remote URI caching/FS bridging, diagnostics mirroring, and UI changes to show locks and remote cursors. ChangesOpen Collaboration protocol & Yjs packages
Ballerina extension integration (collaboration, remote FS, UI)
Sequence Diagram(s)sequenceDiagram
participant User as VSCode User
participant Webview as BI Webview
participant Extension as Ballerina Extension
participant LockMgr as CollaborationLockManager
participant Yjs as Yjs Cluster
participant OCT as Open Collaboration (OCT)
User->>Webview: Click node edit
Webview->>Extension: acquireNodeLock(filePath,nodeId,user)
Extension->>LockMgr: acquireLock(...)
LockMgr->>Yjs: update locks map
Yjs-->>LockMgr: lock update
LockMgr-->>Extension: reply(success)
Extension->>Webview: nodeLockUpdated notification
Webview->>Webview: enable local editing
Webview->>Extension: updateDiagramCursor(x,y,nodeId)
Extension->>OCT: broadcast cursor (awareness)
OCT-->>Extension: awareness change
Extension->>LockMgr: updateCursor(...)
LockMgr-->>Extension: cursors updated
Extension->>Webview: diagramCursorUpdated
Webview->>Webview: render remote cursors
sequenceDiagram
participant RemoteFS as Remote Host (oct://)
participant UriCache as UriCache
participant Extension as Ballerina Extension
participant LSP as Language Server
RemoteFS->>Extension: file changed (watcher)
Extension->>UriCache: cacheRemoteFile(remoteUri)
UriCache-->>Extension: local cached path
Extension->>LSP: didChange(cachedLocalUri, version++)
LSP-->>Extension: analysis results
Extension->>Webview: notify projectContentUpdated (coalesced)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
workspaces/ballerina/bi-diagram/src/components/nodes/ErrorNode/ErrorNodeWidget.tsx (1)
270-318:⚠️ Potential issue | 🟠 MajorRe-check
isLockedinside the menu actions too.Right now locking only prevents opening the menu. If a remote lock arrives while this menu is already open, expand/delete/breakpoint actions still run because none of those callbacks guard against the new lock state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/ErrorNode/ErrorNodeWidget.tsx` around lines 270 - 318, Menu actions currently don't re-check isLocked, so remote locks can still trigger actions; update the action handlers to guard against the locked/readOnly state by early-returning when isLocked (and readOnly where applicable). Specifically, add an isLocked/readOnly check to deleteNode, onAddBreakpoint, onRemoveBreakpoint and to the onClick closures in menuItems (the "expand"/"goToSource"/"delete" handlers) so they no-op if isLocked (and for UI-safe actions also check readOnly), keeping existing setMenuPos/setMenuOpenNodeId behavior unchanged when ignoring the action.workspaces/ballerina/ballerina-extension/src/rpc-managers/visualizer/rpc-manager.ts (1)
303-326:⚠️ Potential issue | 🟠 MajorPropagate the resolved artifact identifier back into state.
This fallback only runs when the identifier-based lookup failed, but the
UPDATE_PROJECT_LOCATIONpayload still sendsidentifier: currentIdentifier. That leaves the state machine with the stale/empty identifier, so the next refresh can still target the wrong node.Proposed fix
if (currentArtifact) { + const resolvedIdentifier = currentArtifact.id ?? currentArtifact.name ?? currentIdentifier; openView(EVENT_TYPE.UPDATE_PROJECT_LOCATION, { documentUri: currentArtifact.path, position: currentArtifact.position, - identifier: currentIdentifier, + identifier: resolvedIdentifier, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/visualizer/rpc-manager.ts` around lines 303 - 326, When the fallback loop finds a matching artifact (currentArtifact), assign the artifact's identifier back into the local state before calling openView so the dispatched UPDATE_PROJECT_LOCATION uses the resolved identifier; specifically set currentIdentifier = currentArtifact.identifier (or the correct identifier field on the found resource) immediately after currentArtifact is set and then call openView(EVENT_TYPE.UPDATE_PROJECT_LOCATION, { documentUri: currentArtifact.path, position: currentArtifact.position, identifier: currentIdentifier });.workspaces/ballerina/bi-diagram/src/components/nodes/IfNode/MatchNodeWidget.tsx (1)
123-142:⚠️ Potential issue | 🟠 MajorKeep
setMenuOpenNodeIdin sync on every menu open/close path.Right-click opening never calls
setMenuOpenNodeId(model.node.id), andhandleOnMenuCloseis not wired into the actual close paths. After an outside click or a menu action, the global open-menu node id can stay stale even though the menu is closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/IfNode/MatchNodeWidget.tsx` around lines 123 - 142, The context-menu open path (handleOnContextMenu) doesn't call setMenuOpenNodeId(model.node.id) and the global state isn't cleared on actual menu close events; update handleOnContextMenu to call setMenuOpenNodeId?.(model.node.id) just like handleOnMenuClick, and ensure the existing handleOnMenuClose (which calls setMenuOpenNodeId?.(undefined)) is passed into the menu component's onClose and any menu-action handlers so the global open-menu id is always cleared on outside click or menu selection; reference handleOnContextMenu, handleOnMenuClick, handleOnMenuClose and setMenuOpenNodeId to locate and wire these changes.workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx (2)
340-344:⚠️ Potential issue | 🟡 MinorMissing
readOnlyandisLockedchecks inhandleOnContextMenu.Unlike other node widgets, this handler has no guards for
readOnlyorisLockedstates. The context menu can be opened even on locked or read-only nodes.Suggested fix
const handleOnContextMenu = (event: React.MouseEvent<HTMLDivElement>) => { event.preventDefault(); + if (readOnly || isLocked) { + return; + } const target = menuButtonElement || event.currentTarget; setMenuPos(getMenuPos(target as HTMLElement)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx` around lines 340 - 344, The context menu handler handleOnContextMenu currently opens the menu unconditionally; add guards to return early when the node is in read-only or locked state by checking the existing readOnly and isLocked flags (the same way other node widgets do) before calling setMenuPos/getMenuPos; use the same variables used in this file (readOnly, isLocked, menuButtonElement, setMenuPos, getMenuPos) so the menu does not open for locked or read-only nodes.
331-338:⚠️ Potential issue | 🟡 MinorMissing
isLockedcheck inhandleOnMenuClick.The handler checks
readOnlybut notisLocked, which is inconsistent with other handlers in this component (handleOnClickat line 292) and other node widgets.Suggested fix
const handleOnMenuClick = (event: React.MouseEvent<HTMLElement | SVGSVGElement>) => { - if (readOnly) { + if (readOnly || isLocked) { return; } const target = menuButtonElement || (event.currentTarget as HTMLElement); setMenuPos(getMenuPos(target)); setMenuOpenNodeId?.(model.node.id); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx` around lines 331 - 338, The handleOnMenuClick handler currently only guards against readOnly but should also return early when the node is locked; update handleOnMenuClick to check both readOnly and isLocked (e.g., if (readOnly || isLocked) return;) before computing target/menu position and calling setMenuOpenNodeId, keeping the rest of the logic that uses menuButtonElement, getMenuPos(target), setMenuPos and setMenuOpenNodeId?.(model.node.id) unchanged.workspaces/ballerina/bi-diagram/src/components/nodes/BaseNode/BaseNodeWidget.tsx (1)
282-298:⚠️ Potential issue | 🟡 MinorMissing
isLockedcheck inhandleOnClick.The
handleOnClickhandler only checksreadOnlybut notisLocked. This is inconsistent with other node widgets and could allow interactions with locked nodes. While the styled component passesreadOnly={readOnly || isLocked}for visual feedback, the click handler itself doesn't block the action.Suggested fix
const handleOnClick = async (event: React.MouseEvent<HTMLDivElement>) => { - if (readOnly) { + if (readOnly || isLocked) { return; } if (event.metaKey) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/BaseNode/BaseNodeWidget.tsx` around lines 282 - 298, The click handler handleOnClick currently only checks readOnly; update it to also block clicks when isLocked is true (i.e. treat the guard as if (readOnly || isLocked) return;). Locate handleOnClick in BaseNodeWidget.tsx and add the isLocked condition before any metaKey handling so locked nodes do not respond to clicks while preserving existing logic that checks event.metaKey and calls openDataMapper, viewFunction, onGoToSource, or onNodeClick.workspaces/ballerina/bi-diagram/src/components/nodes/CommentNode/CommentNodeWidget.tsx (1)
184-193:⚠️ Potential issue | 🟡 MinorMissing
readOnlycheck inhandleOnClick.Unlike other node widgets (e.g., WhileNodeWidget, BaseNodeWidget), the
handleOnClickhandler only checksisLockedbut notreadOnly. This could allow node interactions in read-only mode.Suggested fix
const handleOnClick = (event: React.MouseEvent<HTMLDivElement>) => { - if (isLocked) { + if (readOnly || isLocked) { return; } if (event.metaKey) { onGoToSource(); } else { onNodeClick(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/CommentNode/CommentNodeWidget.tsx` around lines 184 - 193, The click handler handleOnClick currently only guards with isLocked and omits the readOnly check; update handleOnClick to also return early when readOnly is true (i.e., if (isLocked || readOnly) return;), preserving existing behavior for meta-key clicks that call onGoToSource and normal clicks that call onNodeClick; reference handleOnClick, isLocked, readOnly, onGoToSource, and onNodeClick when making the change.workspaces/ballerina/bi-diagram/src/components/NodeLink/NodeLinkWidget.tsx (1)
305-320:⚠️ Potential issue | 🔴 CriticalHonor position locks for the AI prompt action.
The regular add button is disabled when
positionIsLocked, but the prompt button is still clickable andhandleAddPrompt()has no matching guard. That lets users modify a locked insertion point through the prompt flow.Suggested fix
const handleAddPrompt = () => { + if (readOnly || positionIsLocked) { + return; + } if (!onAddNodePrompt) { console.error(">>> NodeLinkWidget: handleAddPrompt: onAddNodePrompt not found"); return; @@ - onClick={isUserAuthenticated ? handleAddPrompt : undefined} + onClick={isUserAuthenticated && !positionIsLocked ? handleAddPrompt : undefined} onMouseEnter={() => setIsPromptButtonHovered(true)} onMouseLeave={() => setIsPromptButtonHovered(false)} css={css` - cursor: ${isUserAuthenticated ? "pointer" : "not-allowed"}; + cursor: ${isUserAuthenticated && !positionIsLocked ? "pointer" : "not-allowed"}; visibility: ${shouldHighlight ? "visible" : "hidden"}; opacity: ${linkOpacity}; `} > + {positionIsLocked && <title>Position locked by another user</title>} {!isUserAuthenticated && <title>You need to be logged into WSO2 Integrator Copilot to access AI features</title>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/NodeLink/NodeLinkWidget.tsx` around lines 305 - 320, The AI prompt button ignores the insertion lock: add the same positionIsLocked guard used by the regular add button so the prompt action cannot run when positionIsLocked is true—update the onClick prop (currently onClick={isUserAuthenticated ? handleAddPrompt : undefined}) to also require !positionIsLocked, adjust cursor/aria/title to reflect the disabled state, and also add an early-return guard inside handleAddPrompt() to no-op if positionIsLocked to prevent programmatic invocation.
🟡 Minor comments (7)
workspaces/ballerina/ballerina-extension/src/features/test-explorer/activator.ts-113-122 (1)
113-122:⚠️ Potential issue | 🟡 MinorPass
workspaceFolder.uridirectly instead of converting throughfsPath.The current code converts the workspace folder's URI to a file path and back to a URI:
Uri.file(workspaceFolder.uri.fsPath). This pattern is inconsistent with the rest of the codebase, which passesUriobjects directly tocheckIsBallerinaWorkspace(),checkIsBallerinaPackage(), andhasMultipleBallerinaPackages().The round-trip through
fsPathforces afile://scheme, preventing support for remote workspaces (e.g.,ssh://,vscode-remote://). Other modules in stateMachine.ts, rpc-managers, and core/extension already passUridirectly and support remote workspaces consistently.Change lines 119–122 to:
const workspaceUri = workspaceFolder.uri; const isBallerinaWorkspace = await checkIsBallerinaWorkspace(workspaceUri); const isBallerinaProject = !isBallerinaWorkspace && await checkIsBallerinaPackage(workspaceUri); const isMultiProjectWorkspace = !isBallerinaWorkspace && !isBallerinaProject && await hasMultipleBallerinaPackages(workspaceUri);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/test-explorer/activator.ts` around lines 113 - 122, The code currently converts workspaceFolder.uri to a file path and back to a Uri (via Uri.file(workspaceFolder.uri.fsPath)), which forces a file:// scheme and breaks remote workspace support; replace that round-trip by using workspaceFolder.uri directly: create a workspaceUri = workspaceFolder.uri and pass it to checkIsBallerinaWorkspace, checkIsBallerinaPackage, and hasMultipleBallerinaPackages so these functions receive the original Uri (do this where workspaceFolder, checkIsBallerinaWorkspace, checkIsBallerinaPackage, and hasMultipleBallerinaPackages are referenced).workspaces/ballerina/bi-diagram/src/components/nodes/CommentNode/CommentNodeWidget.tsx-212-218 (1)
212-218:⚠️ Potential issue | 🟡 MinorMissing
readOnlycheck inhandleOnMenuClick.For consistency with other node widgets,
handleOnMenuClickshould also checkreadOnlybefore allowing the menu to open.Suggested fix
const handleOnMenuClick = (event: React.MouseEvent<HTMLElement | SVGSVGElement>) => { - if (isLocked) { + if (readOnly || isLocked) { return; } setAnchorEl(event.currentTarget); setMenuOpenNodeId?.(model.node.id); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/CommentNode/CommentNodeWidget.tsx` around lines 212 - 218, The menu handler handleOnMenuClick currently only guards with isLocked; add a readOnly check so the menu won't open when the node is read-only—update handleOnMenuClick to return early if readOnly (e.g., check readOnly alongside isLocked) before calling setAnchorEl and setMenuOpenNodeId, referencing the existing handleOnMenuClick function and the readOnly, isLocked, setAnchorEl, setMenuOpenNodeId, and model.node.id symbols.workspaces/ballerina/ballerina-extension/src/utils/modification.ts-235-236 (1)
235-236:⚠️ Potential issue | 🟡 MinorUnclear purpose of arbitrary 100ms delay.
This delay appears to be a workaround for a race condition but lacks documentation explaining why it's needed. Arbitrary delays are brittle and may not be sufficient under heavy load or may unnecessarily slow down operations. Consider documenting the specific race condition being addressed or finding a more robust synchronization mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/modification.ts` around lines 235 - 236, The 100ms setTimeout in modification.ts is an undocumented, brittle race-condition workaround; remove the arbitrary await new Promise(resolve => setTimeout(resolve, 100)) and replace it with a deterministic synchronization mechanism (e.g., await a dedicated Promise or event that signals the resource/operation is ready, or implement a retry loop with exponential backoff and a timeout) inside the same function where the sleep occurs, and add a short comment explaining the exact race condition being handled and why the chosen synchronization method fixes it; ensure you reference the existing promise/event name you create (or the existing ready signal) so callers can rely on the deterministic signal instead of a fixed delay.workspaces/ballerina/ballerina-extension/src/utils/workspace-utils.ts-40-43 (1)
40-43:⚠️ Potential issue | 🟡 MinorFallback to
fsPathmay return unexpected paths for remote URIs.When
uriCacheis unavailable, falling back touri.fsPathfor remote URIs (e.g.,oct://host/path) will return just the path portion without the scheme, which may not be a valid local filesystem path. Consider logging a warning or returningundefinedto signal that translation failed, rather than returning a potentially invalid path.🛡️ Suggested improvement
// Fallback to fsPath (will be the path portion without scheme) + debug(`[WorkspaceUtils] Warning: uriCache unavailable for remote URI: ${uri.toString()}, falling back to fsPath`); return uri.fsPath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/workspace-utils.ts` around lines 40 - 43, The current fallback returns uri.fsPath which can produce invalid local paths for remote schemes; update the logic around the uriCache fallback (the block that currently returns uri.fsPath) to detect non-file schemes (e.g., uri.scheme !== 'file') and instead either return undefined or log a warning via the same logger used in this module to signal translation failure; ensure you reference uriCache and the function that performs URI-to-path conversion so callers can handle undefined rather than receiving an incorrect path.workspaces/ballerina/ballerina-extension/src/utils/modification.ts-112-114 (1)
112-114:⚠️ Potential issue | 🟡 MinorFire-and-forget save may cause race conditions.
The
workspace.applyEdit(edit).then(() => doc.save())pattern doesn't await the save completion. If subsequent code depends on the file being saved, this could cause race conditions. The async version of this function (writeBallerinaFileDidOpen) properly awaits the save.🐛 Suggested fix
Since this function is synchronous, consider making it async or documenting that the save is fire-and-forget:
-export function writeBallerinaFileDidOpenTemp(filePath: string, content: string) { +export async function writeBallerinaFileDidOpenTemp(filePath: string, content: string) { // ... if (doc) { const edit = new WorkspaceEdit(); edit.replace(doc.uri, new Range(new Position(0, 0), doc.lineAt(doc.lineCount - 1).range.end), content.trim()); - workspace.applyEdit(edit).then(() => { - doc.save(); - }); + await workspace.applyEdit(edit); + await doc.save(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/modification.ts` around lines 112 - 114, The current fire-and-forget pattern uses workspace.applyEdit(edit).then(() => doc.save()) which can cause race conditions; update the surrounding function (or create an async variant) to be async and await both operations (await workspace.applyEdit(edit); await doc.save();), or clearly document that the function does not wait, so callers use writeBallerinaFileDidOpen (the async counterpart) when they need a completed save; reference the workspace.applyEdit and doc.save calls and the existing writeBallerinaFileDidOpen function to ensure consistent behavior.workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts-241-253 (1)
241-253:⚠️ Potential issue | 🟡 MinorPotential resource leak: Interval timers are not cleaned up.
The
watchForOCTActivationmethod creates an interval at line 243 that only clears after 30 seconds (line 253), andstartCollaborationWatchercreates a perpetual 2-second interval at line 295 stored inthis.collaborationWatchTimer.Issues:
- If the extension is deactivated,
collaborationWatchTimeris never cleared.- The interval at line 243 could accumulate if
watchForOCTActivationis called multiple times.Consider adding a
dispose()method to clean up resources:♻️ Suggested improvement
public dispose(): void { if (this.collaborationWatchTimer) { clearInterval(this.collaborationWatchTimer); this.collaborationWatchTimer = null; } this.teardownCollaboration(); }And ensure this is called when the extension deactivates.
Also applies to: 293-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts` around lines 241 - 253, The watchForOCTActivation block and startCollaborationWatcher create intervals that are never reliably cleared; add a public dispose(): void on the LockManager (or the class containing watchForOCTActivation/startCollaborationWatcher) that clears this.collaborationWatchTimer and any local interval handles (store the setInterval return from watchForOCTActivation in a field like this.octCheckTimer), sets them to null, and calls teardownCollaboration(); also modify watchForOCTActivation to keep its interval ID in this.octCheckTimer and clear any existing this.octCheckTimer before creating a new one so repeated calls don’t leak timers, and ensure extension deactivation calls the new dispose() method.workspaces/ballerina/ballerina-extension/src/utils/remote-fs/uri-cache.ts-55-82 (1)
55-82:⚠️ Potential issue | 🟡 MinorPotential path injection vulnerability when constructing cache paths.
The
getLocalPathmethod constructs paths using URI components directly:const localPath = path.join(this.cacheDir, uri.scheme, uri.authority || 'default', uri.path);If
uri.pathcontains..segments, this could potentially escape the cache directory. While VS Code URIs are typically well-formed, consider normalizing or validating the path:🛡️ Suggested defensive fix
public getLocalPath(uri: vscode.Uri): string { const uriString = uri.toString(); // Return cached path if exists if (this.uriToLocalPath.has(uriString)) { return this.uriToLocalPath.get(uriString)!; } // Create new local path // Format: <scheme>/<authority>/<path> - const localPath = path.join( + let localPath = path.join( this.cacheDir, uri.scheme, uri.authority || 'default', uri.path ); + + // Ensure the path stays within the cache directory + localPath = path.normalize(localPath); + if (!localPath.startsWith(this.cacheDir)) { + throw new Error(`Invalid URI path: ${uri.toString()}`); + } // Ensure directory exists🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/uri-cache.ts` around lines 55 - 82, The getLocalPath method currently uses uri.path directly which allows ".." or absolute segments to escape cacheDir; sanitize and normalize the URI path before joining: strip any leading path separators, normalize and remove any ".." segments (or collapse to a safe relative path), then build the localPath with path.join(this.cacheDir, uri.scheme, uri.authority || 'default', sanitizedPath) and finally validate the resolved absolute path (e.g., path.resolve(localPath)) starts with the resolved this.cacheDir to prevent directory traversal; keep caching via uriToLocalPath unchanged but only store the validated safe path.
🧹 Nitpick comments (20)
workspaces/ballerina/bi-diagram/src/components/cursorAnchor.ts (1)
6-7: UseDiagramEngineand proper node types instead ofany.The utility functions access methods that are part of the standard diagram model hierarchy. Replace
anywithDiagramEnginefor the engine parameter (which is already used throughout the codebase) and define a minimal interface for the node parameter to match the accessed members (getX,getY,getWidth,getHeight). This aligns with the coding guideline requiring proper typing for TypeScript source files in the bi-diagram package.Also applies to: lines 26-27, 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/cursorAnchor.ts` around lines 6 - 7, Replace untyped `any` node parameters with proper types: change the engine parameter type to DiagramEngine and create a minimal local interface (e.g., INodeLike with getX(): number, getY(): number, getWidth(): number, getHeight(): number) and use it for the `node` parameter in getModelNodeCenter and the other functions mentioned (the functions at the blocks around lines 26-27 and 44-45). Update the function signatures to use DiagramEngine and INodeLike, and update any internal references if needed to satisfy the type checker while keeping behavior unchanged.workspaces/ballerina/ballerina-extension/src/views/ai-panel/activate.ts (1)
36-44: Consider adding a comment to explain the empty catch block.The empty catch block silently suppresses errors when opening the OCT chat container view. If this is intentional (e.g., the view may not exist in certain configurations), a brief comment would clarify the intent for future maintainers.
📝 Suggested improvement
vscode.commands.registerCommand('ballerina.open.oct.chat', async () => { try { await vscode.commands.executeCommand('workbench.view.extension.oct_chat_container'); - } catch { + } catch { + // View container may not exist if OCT extension is not installed } await vscode.commands.executeCommand('oct.chatView.focus'); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/views/ai-panel/activate.ts` around lines 36 - 44, The empty catch in the command handler registered for 'ballerina.open.oct.chat' (the async callback passed to vscode.commands.registerCommand in activate.ts) should include a short comment explaining why errors are intentionally swallowed (e.g., the OCT chat container may not exist in some installations or the fallback to oct.chatView.focus is expected), so future maintainers understand this is deliberate; update the catch block to contain a one-line explanatory comment (or a debug log if preferred) rather than a silent empty block.workspaces/ballerina/ballerina-extension/src/utils/file-utils.ts (1)
438-440: Replace magic number withFileType.Directoryenum for clarity.Using the magic number
2to check for directory type works but reduces readability. VS Code'sFileTypeenum provides a clearer, self-documenting alternative.♻️ Suggested improvement
+import { window, Uri, workspace, ProgressLocation, ConfigurationTarget, MessageItem, Progress, commands, StatusBarAlignment, languages, Range, Selection, ViewColumn, FileType } from "vscode";Then update the check:
const fileUri = Uri.file(filePath); const stat = await workspace.fs.stat(fileUri); - currentFolderPath = (stat.type === 2) ? filePath : path.dirname(filePath); + currentFolderPath = (stat.type === FileType.Directory) ? filePath : path.dirname(filePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/file-utils.ts` around lines 438 - 440, Replace the magic number check in file-utils.ts: when calling workspace.fs.stat(fileUri) and assigning currentFolderPath, use VS Code's FileType enum instead of the literal 2; change the conditional that currently compares stat.type === 2 to use FileType.Directory so the intent is clear (refer to stat, fileUri, currentFolderPath and FileType.Directory).workspaces/ballerina/bi-diagram/src/components/nodes/NodeLockBadge.tsx (1)
21-22: Use the shared design-system icon here instead of a raw emoji.The lock emoji will not inherit the same sizing/theming behavior as the rest of the BI diagram icons, and it renders inconsistently across platforms. Please switch this to the toolkit
Icon+ font icon path used elsewhere in the package.As per coding guidelines, "Use font icons from
@wso2/font-wso2-vscodevia the Icon component from@wso2/ui-toolkit, and center icons using sx prop with explicit fontSize, width, and height."Also applies to: 47-49, 63-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/NodeLockBadge.tsx` around lines 21 - 22, Replace the raw lock emoji in the NodeLockBadge component with the shared design-system Icon: import Icon from "@wso2/ui-toolkit" and use the corresponding font icon class/name from `@wso2/font-wso2-vscode` (the same font icon path used elsewhere in this package) in place of the emoji occurrences (see NodeLockBadge component and the spots around the NodeLock usage). Ensure the Icon is centered and sized consistently by using the sx prop with explicit fontSize, width, and height (matching other BI diagram icons), and remove the emoji characters so the badge uses the toolkit Icon for consistent theming and sizing.workspaces/ballerina/bi-diagram/src/components/nodes/WhileNode/WhileNodeWidget.tsx (1)
301-308: MissingreadOnlycheck inhandleOnContextMenu.Other handlers (e.g.,
handleOnClickat line 255,handleOnMenuClickat line 293) check bothreadOnlyandisLocked, buthandleOnContextMenuonly checksisLocked. For consistency, consider adding thereadOnlycheck.Suggested fix
const handleOnContextMenu = (event: React.MouseEvent<HTMLDivElement>) => { event.preventDefault(); - if (isLocked) { + if (readOnly || isLocked) { return; } const target = menuButtonElement || event.currentTarget; setMenuPos(getMenuPos(target as HTMLElement)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/WhileNode/WhileNodeWidget.tsx` around lines 301 - 308, handleOnContextMenu currently only guards on isLocked; update it to also return early when readOnly is true (same behavior as handleOnClick and handleOnMenuClick). Locate the handleOnContextMenu function and add a check like "if (readOnly || isLocked) return;" before computing target and calling setMenuPos so the context menu is suppressed in read-only mode.workspaces/ballerina/bi-diagram/src/components/nodes/IfNode/IfNodeWidget.tsx (1)
25-26: Potentially unused imports.
Popover,ThemeColors, andTooltipare imported but don't appear to be used in this file. Consider removing them if they're not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/IfNode/IfNodeWidget.tsx` around lines 25 - 26, The import line in IfNodeWidget.tsx currently imports Popover, ThemeColors, and Tooltip which are not used; remove these three symbols from the import statement (the line importing Button, Item, Menu, MenuItem, Popover, ThemeColors, Tooltip from "@wso2/ui-toolkit") and run a quick search for any references to Popover, ThemeColors, or Tooltip in the IfNodeWidget component (and related functions like the exported IfNodeWidget component) to ensure no usages remain; if TypeScript reports unused-import errors after removal, adjust or remove any leftover references accordingly.workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx (2)
53-53: Potentially unused import.The
sizefunction from lodash is imported but doesn't appear to be used in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx` at line 53, The import of size from "lodash" in ApiCallNodeWidget.tsx appears unused; remove the unused import (the line importing size) to clean up the module, or if size was intended for a function (e.g., inside ApiCallNodeWidget or a helper), replace its usage with the correct call site in that function instead of leaving the import unused.
234-235: InconsistentisLockedcomputation and unused variable.
Unlike other node widgets that use
Boolean(model.node.locked && ...), this directly assigns the expression result. Ifmodel.node.lockedis truthy butuserIdcomparison returnsfalse,isLockedcould befalse(correct) but the pattern is inconsistent.
isLockedBySelfis declared but never used.Suggested fix
- const isLocked = model.node.locked && model.node.locked.userId !== currentUserId; - const isLockedBySelf = model.node.locked && model.node.locked.userId === currentUserId; + const isLocked = Boolean(model.node.locked && model.node.locked.userId !== currentUserId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsx` around lines 234 - 235, The isLocked computation should follow the project's Boolean(...) pattern and the unused isLockedBySelf should be removed (or used if intended): replace the current declaration of isLocked with const isLocked = Boolean(model.node.locked && model.node.locked.userId !== currentUserId); and delete the unused const isLockedBySelf = ... from ApiCallNodeWidget (or, if you actually need the self-check elsewhere, keep it but reference it where needed). Ensure you update any logic that relied on the previous raw expression to use the new Boolean result.workspaces/ballerina/bi-diagram/src/components/nodes/BaseNode/BaseNodeWidget.tsx (1)
224-225: Unused variableisLockedBySelf.The variable
isLockedBySelfis declared but never used in this component. Consider removing it to avoid confusion.Suggested fix
const isLocked = Boolean(model.node.locked && model.node.locked.userId !== currentUserId); - const isLockedBySelf = model.node.locked && model.node.locked.userId === currentUserId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/nodes/BaseNode/BaseNodeWidget.tsx` around lines 224 - 225, The variable isLockedBySelf declared as "const isLockedBySelf = model.node.locked && model.node.locked.userId === currentUserId" in BaseNodeWidget is unused; remove this declaration to avoid dead code (or, if the intent was to use it for conditional rendering/behavior, replace references to the current redundant checks with isLockedBySelf so the single boolean is reused). Ensure only the used boolean isLocked (computed as Boolean(model.node.locked && model.node.locked.userId !== currentUserId)) remains or is adjusted accordingly.workspaces/ballerina/ballerina-extension/src/utils/remote-fs/project-info-translator.ts (2)
64-67: Unused private methodgetScheme.This method is defined but never called within the class or exported. Consider removing it to reduce dead code, or use it in
isRemotePathto extract and check schemes more cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/project-info-translator.ts` around lines 64 - 67, The private method getScheme is unused; either remove it to eliminate dead code or reuse it inside isRemotePath to centralize scheme extraction. If removing, delete getScheme and any related imports/comments; if reusing, update isRemotePath to call getScheme(path) and check the returned scheme against remote schemes (e.g., "sftp", "ssh", "http", etc.) instead of inline regex, keeping the method private within the same class (project-info-translator) so behavior remains consistent.
51-56: Brittle detection of local absolute paths.The hardcoded prefixes (
/Users,/home,/var/folders, Windows drive pattern) will miss other valid local paths (e.g.,/opt,/tmp,/var/lib, custom mount points on Linux, or non-standard Windows paths). Consider using a more robust heuristic, such as checking if the path resolves to a file URI viavscode.Uri.file()and inspecting its scheme, or inverting the logic to positively identify remote paths rather than excluding known local prefixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/project-info-translator.ts` around lines 51 - 56, The current remote-path check uses brittle hardcoded local prefixes; replace that logic in the block that reads contextScheme, this.remoteSchemes and path (the same scope as the shown if) with a positive local-file check using vscode.Uri.file(path) — call vscode.Uri.file(path).scheme (after importing vscode) and treat scheme === 'file' as local (return false), otherwise if contextScheme is in this.remoteSchemes return true; remove the hardcoded /Users,/home,/var/folders and Windows drive regex checks so the decision relies on Uri.file() and the existing this.remoteSchemes/contextScheme test.workspaces/ballerina/ballerina-rpc-client/src/BallerinaRpcClient.ts (1)
359-361: Consider adding type constraints tosendRequest.The
requestType: anyparameter loses type safety. Consider using a more specific type or generic constraint if the vscode-messenger library provides one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-rpc-client/src/BallerinaRpcClient.ts` around lines 359 - 361, The sendRequest method currently accepts requestType: any which loses type safety; update the signature of sendRequest<TParams, TResult> to constrain requestType to the messenger library's request descriptor type (e.g., a RequestType/RequestMessage/RequestConstructor generic provided by vscode-messenger) and ensure the call to this.messenger.sendRequest(requestType, HOST_EXTENSION, params) uses that constrained type so TypeScript can validate params and TResult; locate the sendRequest method in BallerinaRpcClient and replace the any with the appropriate messenger-provided generic type (or a local alias) to restore type checking.workspaces/ballerina/bi-diagram/src/components/RemoteCursors.tsx (2)
115-139: Remove or gate debug logging in production.Multiple
console.logandconsole.warnstatements in the render path will pollute the console and may impact performance. Consider removing these or gating them behind a debug flag/environment check.♻️ Suggested approach
+const DEBUG = false; // or import from a config + export function RemoteCursors() { const { remoteCursors, currentUserId, isCollaborationActive, diagramEngine } = useDiagramContext(); if (!isCollaborationActive) { - console.log('[RemoteCursors] Collaboration not active'); + DEBUG && console.log('[RemoteCursors] Collaboration not active'); return null; } if (!remoteCursors || remoteCursors.size === 0) { - console.log('[RemoteCursors] No remote cursors to display'); + DEBUG && console.log('[RemoteCursors] No remote cursors to display'); return null; }Apply similar changes to other
console.logcalls in lines 126, 132, and 139.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/RemoteCursors.tsx` around lines 115 - 139, The render path in RemoteCursors contains several console.log/console.warn calls (checks around isCollaborationActive, remoteCursors, diagramEngine and inside remoteCursors.forEach) that should be removed or gated; update the RemoteCursors component to replace these direct console calls with either a debug flag (e.g. DEBUG_REMOTE_CURSORS or process.env.NODE_ENV !== 'production' guard) or a centralized logger utility and only emit messages when debugging is enabled, and remove/log-gate the final "Filtered cursors to render" and per-entry "Processing cursor entry" statements as well as the diagramEngine warning and initial state logs so production render path has no unconditional console output.
28-41: Add proper typing forengineparameter.The
engineparameter is typed asany, which loses type safety. Consider using theDiagramEnginetype from@projectstorm/react-diagrams-corefor better IDE support and compile-time checks.♻️ Suggested fix
+import { DiagramEngine } from "@projectstorm/react-diagrams-core"; + -function diagramToScreenPosition(engine: any, diagramX: number, diagramY: number): { x: number; y: number } { +function diagramToScreenPosition(engine: DiagramEngine | null, diagramX: number, diagramY: number): { x: number; y: number } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/bi-diagram/src/components/RemoteCursors.tsx` around lines 28 - 41, The diagramToScreenPosition function currently types the engine parameter as any; import the DiagramEngine type from '@projectstorm/react-diagrams-core' and change the signature to accept engine: DiagramEngine | null | undefined (to preserve the existing null-check), so diagramToScreenPosition(engine: DiagramEngine | null | undefined, diagramX: number, diagramY: number): { x: number; y: number } and keep the rest of the logic unchanged; this improves type safety and IDE support when calling engine.getModel() inside the function.workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
139-143: Remove debug logging for production.Multiple
console.logstatements throughout the function will clutter the output. Consider removing them or using the existingdebug()logger from./loggerfor conditional debug output.♻️ Suggested approach
+import { debug } from './logger'; + export async function writeBallerinaFileDidOpen(filePath: string, content: string) { - console.log('[Modification] writeBallerinaFileDidOpen called with filePath:', filePath); + debug('[Modification] writeBallerinaFileDidOpen called with filePath:', filePath); // Check if this is a cached path and get the remote URI const remoteUri = uriCache?.getRemoteUri(filePath); - console.log('[Modification] Remote URI from cache:', remoteUri?.toString()); + debug('[Modification] Remote URI from cache:', remoteUri?.toString());Apply similar changes to other
console.logcalls.Also applies to: 155-158, 187-187, 211-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/modification.ts` around lines 139 - 143, Remove the production console.log statements in writeBallerinaFileDidOpen and replace them with the module debug logger from ./logger (e.g., call debug('[Modification] writeBallerinaFileDidOpen called with filePath: %s', filePath) and debug('[Modification] Remote URI from cache: %s', remoteUri?.toString())). Ensure the debug import is present (import { debug } from './logger') or added if missing, and apply the same replacement for the other console.log occurrences referenced (around lines where filePath/remoteUri and other debug messages appear: the blocks you noted at 155-158, 187, 211) so all temporary console output uses debug() instead of console.log.workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts (2)
297-305: Potential null reference when sending notification.At line 299,
this.messenger.sendNotificationis called, butthis.messengercould be undefined even inside theif (messenger)block because the check is on the parameter, not the instance field. Since assignment happens at line 292, this should be safe, but the code would be clearer with a null assertion or by using the local parameter.However, more importantly, TypeScript's type narrowing doesn't carry over from the parameter to the instance field. Consider using the local
messengerparameter directly:♻️ Suggested improvement
lockManager.onLocksChanged((filePath, locks) => { console.log(`[Lock] Broadcasting lock update for ${filePath} to webviews`); - this.messenger.sendNotification(nodeLockUpdated, { + messenger.sendNotification(nodeLockUpdated, { type: 'webview', webviewType: 'ballerina.visualizer' }, { locks, }); });Apply the same change for the cursor update at lines 310-316.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts` around lines 297 - 305, The callback passed to lockManager.onLocksChanged uses the instance field this.messenger which may be undefined because TypeScript's narrowing on the messenger parameter doesn't apply to the class field; change the callback to use the local messenger parameter (the one checked/assigned earlier) or assert non-null before calling sendNotification, e.g., use messenger.sendNotification(... nodeLockUpdated ...) instead of this.messenger.sendNotification to avoid a potential null reference; apply the same fix for the cursor update callback that calls sendNotification for nodeCursorUpdated so both callbacks use the narrowed local messenger.
2709-2720: Path traversal uses..which may not normalize correctly.The
getRepoRootfunction usespath.join(projectRoot, "..")to traverse up directories. While this works,path.dirname(projectRoot)is more idiomatic for getting the parent directory:♻️ Suggested improvement
export function getRepoRoot(projectRoot: string): string | undefined { // traverse up the directory tree until .git directory is found const gitDir = path.join(projectRoot, ".git"); if (fs.existsSync(gitDir)) { return projectRoot; } // path is root return undefined if (projectRoot === path.parse(projectRoot).root) { return undefined; } - return getRepoRoot(path.join(projectRoot, "..")); + return getRepoRoot(path.dirname(projectRoot)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts` around lines 2709 - 2720, The getRepoRoot function should use path.dirname(projectRoot) to get the parent directory instead of path.join(projectRoot, "..") to avoid non-normalized paths; update the recursive call in getRepoRoot to call getRepoRoot(path.dirname(projectRoot)), keeping the existing base cases (checking fs.existsSync(path.join(projectRoot, ".git")) and comparing projectRoot to path.parse(projectRoot).root) so recursion correctly terminates.workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts (2)
170-170:octApityped asanyreduces type safety.The
octApifield at line 170 is typed asany, andlocksMapat line 146 is alsoany. This reduces TypeScript's ability to catch type errors.Consider creating interface definitions for the OCT API surface you use:
interface OctApi { isActive(): boolean; getCollaborationInstance(): CollaborationInstanceLike | undefined; getSharedDoc?(): Y.Doc; getAwareness?(): Awareness; }Also applies to: 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts` at line 170, The fields octApi and locksMap are typed as any; define a proper OctApi interface (e.g., methods isActive, getCollaborationInstance, optional getSharedDoc, getAwareness) that matches the OCT surface you use and replace the octApi: any declaration with octApi: OctApi | null; also type locksMap appropriately (e.g., Record<string, LockType> or Map<string, LockType>) and update usage in methods like getCollaborationInstance, isActive, and anywhere locksMap is accessed to use the new types so TypeScript can validate calls and properties.
263-283: Global property interception is fragile and may cause issues.The code at lines 263-283 intercepts property setters on
globalThisto detect when OCT sets collaboration instances. This approach:
- May conflict with other extensions modifying these properties
- Uses
configurable: truebut another extension could override- The setter calls
CollaborationLockManager.getInstance()which could cause issues if called during module initializationConsider using OCT's official API events instead of intercepting global properties, or document this as a known workaround for OCT integration limitations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts` around lines 263 - 283, The globalThis property interception via interceptProperties and Object.defineProperty (creating getters/setters that call CollaborationLockManager.getInstance().refreshCollaborationInstance()) is fragile; replace it with OCT's official event/API for collaboration instance changes if available (subscribe to the relevant OCT event and invoke CollaborationLockManager.getInstance().refreshCollaborationInstance() from that handler), and if no API exists, document this workaround clearly and harden the interceptor by avoiding calling getInstance() during module initialization (lazy-init the manager, wrap calls in try/catch, check for existence before invoking refreshCollaborationInstance(), and preserve/restore original property descriptors stored before redefine to minimize conflicts with other extensions).workspaces/ballerina/ballerina-extension/src/utils/remote-fs/uri-cache.ts (1)
236-244: Linear scan for reverse lookup may become a performance bottleneck.The
getRemoteUrimethod iterates through all entries inuriToLocalPathto find a match. For large caches, this O(n) lookup could become slow.Consider maintaining a reverse map (
localPathToUri) if this method is called frequently:private readonly localPathToUri = new Map<string, string>();Update it in
getLocalPathwhen adding new mappings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/uri-cache.ts` around lines 236 - 244, getRemoteUri currently does a linear scan of uriToLocalPath which is O(n); add a reverse map localPathToUri: Map<string,string> and maintain it whenever you add or remove entries so lookups become O(1). Update the code paths that create mappings (e.g., getLocalPath where you call this.uriToLocalPath.set(uriString, localPath)) to also set this.localPathToUri.set(localPath, uriString), and update any removal/clear logic to delete from localPathToUri as well; then change getRemoteUri to check localPathToUri.get(localPath) and return vscode.Uri.parse(uriString) if present. Ensure symbol names referenced: getRemoteUri, getLocalPath, uriToLocalPath, localPathToUri.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1991327b-5182-481d-9739-b23f791bd3ff
⛔ Files ignored due to path filters (2)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (61)
.vscode/launch.jsonworkspaces/ballerina/ballerina-core/src/interfaces/bi.tsworkspaces/ballerina/ballerina-core/src/rpc-types/bi-diagram/interfaces.tsworkspaces/ballerina/ballerina-core/src/rpc-types/bi-diagram/rpc-type.tsworkspaces/ballerina/ballerina-core/src/state-machine-types.tsworkspaces/ballerina/ballerina-extension/package.jsonworkspaces/ballerina/ballerina-extension/src/RPCLayer.tsworkspaces/ballerina/ballerina-extension/src/core/extension.tsworkspaces/ballerina/ballerina-extension/src/extension.tsworkspaces/ballerina/ballerina-extension/src/features/ai/data-mapper/orchestrator.tsworkspaces/ballerina/ballerina-extension/src/features/bi/activator.tsworkspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.tsworkspaces/ballerina/ballerina-extension/src/features/collaboration/oct-helper.tsworkspaces/ballerina/ballerina-extension/src/features/project/cmds/configRun.tsworkspaces/ballerina/ballerina-extension/src/features/test-explorer/activator.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-handler.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/collaboration/rpc-handler.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/common/utils.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/visualizer/rpc-manager.tsworkspaces/ballerina/ballerina-extension/src/stateMachine.tsworkspaces/ballerina/ballerina-extension/src/utils/config.tsworkspaces/ballerina/ballerina-extension/src/utils/file-utils.tsworkspaces/ballerina/ballerina-extension/src/utils/modification.tsworkspaces/ballerina/ballerina-extension/src/utils/project-utils.tsworkspaces/ballerina/ballerina-extension/src/utils/remote-fs/index.tsworkspaces/ballerina/ballerina-extension/src/utils/remote-fs/project-info-translator.tsworkspaces/ballerina/ballerina-extension/src/utils/remote-fs/uri-cache.tsworkspaces/ballerina/ballerina-extension/src/utils/source-utils.tsworkspaces/ballerina/ballerina-extension/src/utils/state-machine-utils.tsworkspaces/ballerina/ballerina-extension/src/utils/workspace-utils.tsworkspaces/ballerina/ballerina-extension/src/views/ai-panel/activate.tsworkspaces/ballerina/ballerina-extension/src/views/persist-layer-diagram/activator.tsworkspaces/ballerina/ballerina-extension/src/views/visualizer/webview.tsworkspaces/ballerina/ballerina-extension/tsconfig.jsonworkspaces/ballerina/ballerina-extension/ui-test/CHANGELOG.mdworkspaces/ballerina/ballerina-low-code-diagram/src/Components/RenderingComponents/ModuleVariable/index.tsxworkspaces/ballerina/ballerina-rpc-client/src/BallerinaRpcClient.tsworkspaces/ballerina/ballerina-rpc-client/src/rpc-clients/bi-diagram/rpc-client.tsworkspaces/ballerina/ballerina-visualizer/src/utils/bi.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsxworkspaces/ballerina/ballerina-visualizer/tsconfig.jsonworkspaces/ballerina/bi-diagram/src/components/Diagram.tsxworkspaces/ballerina/bi-diagram/src/components/DiagramCanvas.tsxworkspaces/ballerina/bi-diagram/src/components/DiagramContext.tsxworkspaces/ballerina/bi-diagram/src/components/NodeLink/NodeLinkWidget.tsxworkspaces/ballerina/bi-diagram/src/components/RemoteCursors.tsxworkspaces/ballerina/bi-diagram/src/components/cursorAnchor.tsworkspaces/ballerina/bi-diagram/src/components/nodes/AgentCallNode/AgentCallNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/ApiCallNode/ApiCallNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/BaseNode/BaseNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/CommentNode/CommentNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/DraftNode/DraftNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/EmptyNode/EmptyNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/ErrorNode/ErrorNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/IfNode/IfNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/IfNode/MatchNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/NodeLockBadge.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/PromptNode/PromptNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/components/nodes/WhileNode/WhileNodeWidget.tsxworkspaces/ballerina/bi-diagram/src/index.tsx
Comment on lines
+645
to
+649
| export interface CollaborationTextSelection { | ||
| filePath: string; | ||
| selectedNodes?: string[]; | ||
| cursor?: WebviewCursorPosition; | ||
| } |
Contributor
There was a problem hiding this comment.
Carry diagramId through selection updates too.
Presence updates are scoped by diagramId, but selection/cursor updates are keyed only by filePath. If two flow diagrams from the same file are open, one diagram's selection state can overwrite the other because this transport cannot distinguish them.
Also applies to: 663-668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-core/src/state-machine-types.ts` around lines
645 - 649, The selection/cursor types must carry diagramId to avoid
cross-diagram collisions: add a diagramId?: string field to the
CollaborationTextSelection interface and to the corresponding selection/cursor
interfaces referenced around lines 663-668, then update any code that builds
selection/cursor transport keys or serializes these objects to include diagramId
(and update callers that construct these objects so they pass the current
diagramId) so presence/scoped updates are distinguished per diagram.
Comment on lines
+537
to
+552
| public isCollaborationActive(): boolean { | ||
| if (this.octApi?.isActive()) { | ||
| // Ensure we have the collaboration instance | ||
| if (!this.isCollaborationMode || !this.collaborationInstance) { | ||
| const instance = this.octApi.getCollaborationInstance(); | ||
| if (instance && instance !== this.collaborationInstance) { | ||
| // Async initialization, but return true since OCT is active | ||
| this.attachToCollaborationInstance(instance).catch(err => { | ||
| console.error('[Collaboration] Failed to attach to OCT instance:', err); | ||
| }); | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| return this.isCollaborationMode && !!this.locksMap; | ||
| } |
Contributor
There was a problem hiding this comment.
Side effect in isCollaborationActive() may cause unexpected behavior.
The isCollaborationActive() method has a side effect: it asynchronously calls attachToCollaborationInstance (line 544) but returns true immediately. This means:
- The method returns a potentially stale state
- A fire-and-forget async operation runs without awaiting
- Callers may get inconsistent results if they call this rapidly
Consider either:
- Making this a pure check (no side effects)
- Returning a Promise and awaiting the attachment
- Moving the attachment logic to a separate method
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/features/collaboration/lock-manager.ts`
around lines 537 - 552, isCollaborationActive currently performs a side-effect
by calling attachToCollaborationInstance when octApi.isActive() but returns true
immediately; change it to be a pure Boolean check (no async calls) by removing
the attachToCollaborationInstance invocation from isCollaborationActive and only
return the computed boolean (use octApi.isActive() || (isCollaborationMode &&
!!locksMap)); move the async attachment logic into a new method (e.g.,
ensureCollaborationAttached or call site init) that explicitly gets the instance
via octApi.getCollaborationInstance() and awaits
attachToCollaborationInstance(instance) or handles errors, leaving
isCollaborationActive solely for synchronous state checks and callers can call
the new ensureCollaborationAttached when they need to initialize attachment.
Comment on lines
+611
to
+617
| // Update model with lock information when locks change | ||
| useEffect(() => { | ||
| if (model) { | ||
| const updatedModel = updateNodeLocks(model, nodeLocks); | ||
| setModel(updatedModel); | ||
| } | ||
| }, [nodeLocks]); |
Contributor
There was a problem hiding this comment.
Reapply lock metadata whenever the flow model is replaced.
getFlowModel() installs a fresh model, but this effect only reacts to nodeLocks. That means any refresh can temporarily drop lock annotations until the next lock event arrives. Derive a lock-enriched model from [model, nodeLocks], or apply updateNodeLocks() before each setModel(...) that replaces the flow model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx`
around lines 611 - 617, The effect that applies updateNodeLocks only depends on
nodeLocks, so when getFlowModel() replaces model the lock annotations can be
lost; modify the logic so the lock-enriched model is derived from both model and
nodeLocks — either change the useEffect dependency to [model, nodeLocks] and
call setModel(updateNodeLocks(model, nodeLocks)) inside it, or ensure every call
site that replaces the flow model (e.g., where getFlowModel() calls
setModel(...)) applies setModel(updateNodeLocks(newModel, nodeLocks)) so
updateNodeLocks always runs when the model is replaced; reference
updateNodeLocks, setModel, getFlowModel, model, and nodeLocks to locate the
places to change.
Comment on lines
+123
to
+125
| const parentId = "id" in node ? node.id : "branch"; | ||
| const anchorKey = `position_${parentId}_${target.line}_${target.offset}`; | ||
| const anchorPosition = getDiagramPositionFromElementCenter(event.currentTarget); |
Contributor
There was a problem hiding this comment.
Use a stable branch identifier in anchorKey.
The "branch" fallback makes different branch insertion points collapse to the same anchor whenever their target line/offset matches. That will cross-wire remote cursor anchoring and lock state between unrelated links.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/bi-diagram/src/components/NodeLink/NodeLinkWidget.tsx`
around lines 123 - 125, The anchorKey uses a brittle fallback ("branch") via
parentId which causes different branches to collapse; in NodeLinkWidget.tsx
update the parentId computation so it uses a stable branch identifier from the
node (for example node.branchId or node.branch?.id if available) instead of the
literal "branch", or if the node API lacks an explicit branch id compute a
deterministic fallback from node-specific properties (e.g.
node.startLine/node.startOffset or a node.path) and then build anchorKey from
that stable identifier (update the assignment to parentId and use it when
forming anchorKey).
Comment on lines
+242
to
245
| currentUserId, | ||
| } = useDiagramContext(); | ||
| const isLocked = Boolean(model.node.locked && model.node.locked.userId !== currentUserId); | ||
|
|
Contributor
There was a problem hiding this comment.
Freeze the prompt editor when lock ownership changes.
These checks only block entering edit mode. If another user acquires the lock while this node is already editable, NPPromptEditor stays writable because it is still gated by !editable, and Save still calls onNodeSave. That bypasses the node lock.
Proposed fix
const handleSave = () => {
+ if (isLocked) {
+ return;
+ }
const clonedNode = cloneDeep(model.node);
clonedNode.properties['prompt'].value = bodyTextTemplate;
clonedNode.codedata.node = "NP_FUNCTION_DEFINITION";
onNodeSave?.(clonedNode);
toggleEditable();
@@
+ const isEditorReadOnly = !editable || isLocked;
+
return (
@@
<NPPromptEditor
@@
- disabled={!editable}
+ disabled={isEditorReadOnly}
/>
@@
{editable && (
<NodeStyles.ButtonGroup>
<Button appearance="secondary" onClick={toggleEditable}>
Cancel
</Button>
- <Button appearance="primary" onClick={handleSave}>
+ <Button appearance="primary" onClick={handleSave} disabled={isLocked}>
Save
</Button>
</NodeStyles.ButtonGroup>
)}Also applies to: 336-351, 438-443, 484-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/bi-diagram/src/components/nodes/PromptNode/PromptNodeWidget.tsx`
around lines 242 - 245, The prompt editor can remain writable after a lock
transfer because we only prevent entering edit mode; update PromptNodeWidget so
lock ownership changes immediately freeze the editor: derive isLocked from
model.node.locked.userId !== currentUserId (already present) and add an effect
that forces editable state to false when isLocked becomes true (reset any local
"editable" or "isEditing" state used by NPPromptEditor), and also guard the save
path (onNodeSave handler) to no-op or return early if isLocked is true; ensure
NPPromptEditor receives the locked/editable prop so it renders read-only when
isLocked is true and references to editable, onNodeSave, and model.node.locked
are the symbols to update.
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/config/rush/.pnpmfile.cjs (1)
32-47:⚠️ Potential issue | 🟠 MajorSync these overrides with the root
package.jsonpnpm.overridesblock.The three bumped/added entries here diverge from the root
package.jsonoverrides shown in the context snippet:
hono: pnpmfile4.12.14vs root4.12.12dompurify: pnpmfile3.4.0vs root3.3.2follow-redirects: pnpmfile1.16.0vs not present in rootWith two sources of truth, PNPM's resolution can differ from what
rush updatepins viapnpm.overrides, producing drift between lockfile and the in-processreadPackagerewriting, and making future security bumps easy to miss in one place. Please mirror these inpackage.jsonpnpm.overrides(or remove them from one location) so both paths agree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/config/rush/.pnpmfile.cjs` around lines 32 - 47, The pnpmfile.cjs overrides for package names hono, dompurify, and follow-redirects diverge from the root package.json pnpm.overrides; update the root package.json pnpm.overrides to match these versions (set "hono": "4.12.14", "dompurify": "3.4.0", and add "follow-redirects": "1.16.0") or remove these entries from .pnpmfile.cjs so there's a single source of truth; ensure the package names ('hono', 'dompurify', 'follow-redirects') in pnpm.overrides exactly match the strings used in .pnpmfile.cjs to avoid resolution drift.
🧹 Nitpick comments (6)
workspaces/ballerina/ballerina-extension/src/utils/remote-fs/diagnostics-bridge.ts (2)
98-113: Re-entrancy flag has limited effect; consider removing or scoping to event handler.
isMirroringInProgressis set insideapplyMirroredDiagnosticsbut only read bymirrorDiagnosticsForChangedUris. BecauseonDidChangeDiagnosticsfires asynchronously, the flag will typically befalseagain by the time the event arrives — the actual guard preventing a feedback loop is theuri.scheme !== 'file'check at line 84 (the mirrored writes target remote URIs). The flag adds mutable state without meaningfully preventing re-entrancy. Either document its intent or rely solely on the scheme filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/diagnostics-bridge.ts` around lines 98 - 113, The isMirroringInProgress flag in applyMirroredDiagnostics should be removed (or at minimum confined to the event handler) because it doesn't prevent re-entrancy; instead, rely on the existing uri.scheme !== 'file' filter in mirrorDiagnosticsForChangedUris/onDidChangeDiagnostics to avoid feedback loops. Remove all reads/writes to isMirroringInProgress (references in applyMirroredDiagnostics and mirrorDiagnosticsForChangedUris), or if you prefer to keep a guard, move a scoped boolean into the onDidChangeDiagnostics handler so its lifetime matches the async event handling; update or remove debug messages that reference the flag accordingly.
40-45: Narrow the scheme filter to only handle OCT remote documents.The listener currently processes all non-
fileschemes (includinggit:,vscode:,output:,untitled:, etc.), triggering aUriCachelookup for each. BecausegetLocalPath()creates cache entries for any URI butgetRemoteUri()only finds mapped documents, schemes outside the cache cause wasted lookups and cache pollution. Filter by the specificoctscheme used for remote collaboration documents.Suggested change
- const remoteDocOpenListener = vscode.workspace.onDidOpenTextDocument((document) => { - if (document.uri.scheme === 'file') { - return; - } - this.syncForRemoteUri(document.uri); - }); + const remoteDocOpenListener = vscode.workspace.onDidOpenTextDocument((document) => { + if (document.uri.scheme !== 'oct') { + return; + } + this.syncForRemoteUri(document.uri); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/diagnostics-bridge.ts` around lines 40 - 45, The onDidOpenTextDocument listener (remoteDocOpenListener) currently calls syncForRemoteUri for any non-file scheme causing unnecessary UriCache lookups; change the guard to only proceed for the OCT remote collaboration scheme (i.e. check document.uri.scheme === 'oct') so only OCT documents trigger syncForRemoteUri, preventing cache pollution from other schemes; update any related logic that assumes non-file implies remote (e.g., usages of getLocalPath/getRemoteUri) to rely on the oct-scheme check when deciding to create or look up cached mappings.workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts (2)
2709-2720: Usepath.dirname(and resolve) instead of recursivepath.join('..').Two concerns with the current implementation:
- For relative inputs (
"foo","./x"),path.join(projectRoot, "..")yields".", then"..", then"../.."… which never equalspath.parse(projectRoot).root(which is""for relative paths). That's a potential unbounded recursion / stack overflow ifprojectRootis ever non-absolute.path.dirnameis the idiomatic way to walk up a path and also stops cleanly at the filesystem root (path.dirname("/") === "/").🛠️ Proposed fix
export function getRepoRoot(projectRoot: string): string | undefined { - // traverse up the directory tree until .git directory is found - const gitDir = path.join(projectRoot, ".git"); - if (fs.existsSync(gitDir)) { - return projectRoot; - } - // path is root return undefined - if (projectRoot === path.parse(projectRoot).root) { - return undefined; - } - return getRepoRoot(path.join(projectRoot, "..")); + let current = path.resolve(projectRoot); + // Iteratively walk up until .git is found or we hit the filesystem root + // eslint-disable-next-line no-constant-condition + while (true) { + if (fs.existsSync(path.join(current, ".git"))) { + return current; + } + const parent = path.dirname(current); + if (parent === current) { + return undefined; + } + current = parent; + } }Iterative form also avoids the per-call stack frame and is easier to reason about.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts` around lines 2709 - 2720, The getRepoRoot function should stop using recursion and path.join('..') and instead resolve the input to an absolute path and iteratively walk up using path.dirname until the filesystem root is reached; update getRepoRoot to call path.resolve(projectRoot) first, then use a while loop that checks fs.existsSync(path.join(currentDir, ".git")) and returns currentDir if found, and breaks/returns undefined when currentDir === path.dirname(currentDir) (or otherwise equals the resolved root), ensuring no unbounded recursion for relative inputs.
299-299: Drop or gate per-event console logs.
console.logfires on every lock/cursor change. In an active collaboration session with multiple users moving cursors, this will flood the extension host console. Consider removing these or routing through a leveled logger behind a debug flag.Also applies to: 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts` at line 299, The per-event console.log call console.log(`[Lock] Broadcasting lock update for ${filePath} to webviews`) (and the similar log at the other location) should be removed or gated behind a debug-level logger; replace direct console.log with a leveled logger call (e.g., this.logger.debug(...) or logger.debug(...)) or wrap the call with a runtime debug flag check (e.g., if (DEBUG) { ... }) so frequent lock/cursor updates do not spam the extension host console. Ensure you use the module/class logger instance used elsewhere in rpc-manager.ts (or add one) and keep the original message text for consistency.workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx (2)
289-302: Drop the[hasDraft]dependency ondebouncedGetFlowModel.
hasDraftis never read inside the callback, but including it in the deps causesuseCallbackto return a new debounced function every time draft state flips. That recreation cancels any in-flight debounce and replaces it with a fresh 150 ms window, so content updates that arrive while a draft is being completed can be dropped or double-fired. Use an empty dep list (matchingdebouncedGetFlowModelForBreakpoints) so the debounce instance is stable.♻️ Suggested change
- const debouncedGetFlowModel = useCallback( - debounce(() => { - getFlowModel(); - }, 150), - [hasDraft] - ); + const debouncedGetFlowModel = useCallback( + debounce(() => { + getFlowModel(); + }, 150), + [] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx` around lines 289 - 302, Remove the unnecessary dependency on hasDraft from the useCallback that creates debouncedGetFlowModel: change the dependency array of the debouncedGetFlowModel useCallback to an empty array (same pattern as debouncedGetFlowModelForBreakpoints) so the debounced function instance for debouncedGetFlowModel remains stable; keep calling getFlowModel inside that debounce as currently implemented and do not alter debouncedGetFlowModelForBreakpoints.
1577-1600: Heartbeat effect re-fires on everynodeLockschange.
sendPresenceUpdateis rebuilt whenevernodeLocks,model?.fileName,currentUserId,currentUserName, orrpcClientchange (its own dep list on line 1561). Because it is in this effect's dep list, each of those changes tears down the interval, runsfireCursorHeartbeat()immediately, and starts a new interval. During active collaborationnodeLockschurns frequently (every peer presence event with lock data), so the "immediate" broadcast fires far more often than the intended 3 s cadence, amplifying OCT traffic.Consider keeping the effect keyed only on
isCollaborationActiveand reading the latestsendPresenceUpdatethrough a ref, so the immediate fire happens once per activation and the interval is stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx` around lines 1577 - 1600, The heartbeat effect currently depends on sendPresenceUpdate causing teardown/restart whenever sendPresenceUpdate is recreated (e.g., on nodeLocks churn); change it to depend only on isCollaborationActive and use a ref to read the latest sendPresenceUpdate inside the effect: create a sendPresenceUpdateRef and update sendPresenceUpdateRef.current whenever sendPresenceUpdate changes, then inside the useEffect for heartbeat call sendPresenceUpdateRef.current(...) from fireCursorHeartbeat and from the immediate fire so the interval remains stable (keep lastBroadcastCursorRef and CURSOR_HEARTBEAT_INTERVAL_MS usage) and still clearInterval on cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts`:
- Line 181: Remove the unused import "uriCache" from the import list in
rpc-manager.ts; locate the import statement that reads "import { uriCache } from
'../../extension';" (near other imports) and delete it so there are no unused
symbols imported from '../../extension'.
- Around line 1063-1086: The code is treating an empty artifacts array as a
failure and rebuilding the entire project structure; instead, only trigger
buildProjectsStructure(...) when updateSourceCode actually fails (throws) or
when the update returns an explicit error flag. Modify the logic around where
artifacts is produced so you no longer enter the fallback on artifacts.length
=== 0 — either (A) change the caller of updateSourceCode to return a richer
result (e.g., {artifacts, error?: Error} or {artifacts, success: boolean}) and
check that error/success before calling buildProjectsStructure, or (B) remove
the artifacts.length === 0 check and only run the rebuild inside the catch block
for exceptions from updateSourceCode; keep the existing use of
StateMachine.context(), resolve({ artifacts: ... }), notifyDeletionUpdate(), and
buildProjectsStructure(...) when a real error occurs.
- Around line 292-319: The constructor currently subscribes to
CollaborationLockManager.getInstance() via onLocksChanged and onCursorsChanged
but drops the returned vscode.Disposable objects, leaking listeners and stale
this.messenger captures; fix by storing those Disposables (e.g., push them into
a private disposables: vscode.Disposable[] on the BiDiagramRpcManager class)
when calling onLocksChanged and onCursorsChanged, change the callbacks to
capture the local messenger parameter (not read optional this.messenger inside
closures), and add a public dispose() method that iterates disposables and calls
dispose() to remove the listeners; ensure any owner that constructs
BiDiagramRpcManager calls dispose() on teardown.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx`:
- Around line 453-582: The presence handler registered in onOctRerenderPresence
closes over helpers that read projectPath at mount, causing stale path checks;
fix by introducing a projectPathRef (e.g., projectPathRef.current updated in a
useEffect when projectPath prop changes) and change the code used inside the
onOctRerenderPresence callback to call a path-normalizer that reads
projectPathRef.current (or compute normalizedCurrentFile directly using
normalizeFilePath with projectPathRef.current), ensuring
pathsMatchWithLegacyFallback and diagramIdsMatchWithLegacyFallback use the
up-to-date project path; alternatively, re-subscribe the
rpcClient.onOctRerenderPresence listener whenever projectPath changes so the
closure captures the latest projectPath.
- Around line 2800-2806: The fallback setTimeout call that invokes
getFlowModel() should store its timer id in a ref (e.g., retryTimerRef) and be
cleared whenever a new retry is scheduled or the component unmounts; update the
code around debouncedGetFlowModel()/getFlowModel() to assign the return of
setTimeout to retryTimerRef.current and call clearTimeout(retryTimerRef.current)
at the start of the function that schedules the retry and inside the
mount/unmount effect that currently manages isMountedRef so only the latest
retry can fire and no timeout calls setState after unmount.
---
Outside diff comments:
In `@common/config/rush/.pnpmfile.cjs`:
- Around line 32-47: The pnpmfile.cjs overrides for package names hono,
dompurify, and follow-redirects diverge from the root package.json
pnpm.overrides; update the root package.json pnpm.overrides to match these
versions (set "hono": "4.12.14", "dompurify": "3.4.0", and add
"follow-redirects": "1.16.0") or remove these entries from .pnpmfile.cjs so
there's a single source of truth; ensure the package names ('hono', 'dompurify',
'follow-redirects') in pnpm.overrides exactly match the strings used in
.pnpmfile.cjs to avoid resolution drift.
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts`:
- Around line 2709-2720: The getRepoRoot function should stop using recursion
and path.join('..') and instead resolve the input to an absolute path and
iteratively walk up using path.dirname until the filesystem root is reached;
update getRepoRoot to call path.resolve(projectRoot) first, then use a while
loop that checks fs.existsSync(path.join(currentDir, ".git")) and returns
currentDir if found, and breaks/returns undefined when currentDir ===
path.dirname(currentDir) (or otherwise equals the resolved root), ensuring no
unbounded recursion for relative inputs.
- Line 299: The per-event console.log call console.log(`[Lock] Broadcasting lock
update for ${filePath} to webviews`) (and the similar log at the other location)
should be removed or gated behind a debug-level logger; replace direct
console.log with a leveled logger call (e.g., this.logger.debug(...) or
logger.debug(...)) or wrap the call with a runtime debug flag check (e.g., if
(DEBUG) { ... }) so frequent lock/cursor updates do not spam the extension host
console. Ensure you use the module/class logger instance used elsewhere in
rpc-manager.ts (or add one) and keep the original message text for consistency.
In
`@workspaces/ballerina/ballerina-extension/src/utils/remote-fs/diagnostics-bridge.ts`:
- Around line 98-113: The isMirroringInProgress flag in applyMirroredDiagnostics
should be removed (or at minimum confined to the event handler) because it
doesn't prevent re-entrancy; instead, rely on the existing uri.scheme !== 'file'
filter in mirrorDiagnosticsForChangedUris/onDidChangeDiagnostics to avoid
feedback loops. Remove all reads/writes to isMirroringInProgress (references in
applyMirroredDiagnostics and mirrorDiagnosticsForChangedUris), or if you prefer
to keep a guard, move a scoped boolean into the onDidChangeDiagnostics handler
so its lifetime matches the async event handling; update or remove debug
messages that reference the flag accordingly.
- Around line 40-45: The onDidOpenTextDocument listener (remoteDocOpenListener)
currently calls syncForRemoteUri for any non-file scheme causing unnecessary
UriCache lookups; change the guard to only proceed for the OCT remote
collaboration scheme (i.e. check document.uri.scheme === 'oct') so only OCT
documents trigger syncForRemoteUri, preventing cache pollution from other
schemes; update any related logic that assumes non-file implies remote (e.g.,
usages of getLocalPath/getRemoteUri) to rely on the oct-scheme check when
deciding to create or look up cached mappings.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx`:
- Around line 289-302: Remove the unnecessary dependency on hasDraft from the
useCallback that creates debouncedGetFlowModel: change the dependency array of
the debouncedGetFlowModel useCallback to an empty array (same pattern as
debouncedGetFlowModelForBreakpoints) so the debounced function instance for
debouncedGetFlowModel remains stable; keep calling getFlowModel inside that
debounce as currently implemented and do not alter
debouncedGetFlowModelForBreakpoints.
- Around line 1577-1600: The heartbeat effect currently depends on
sendPresenceUpdate causing teardown/restart whenever sendPresenceUpdate is
recreated (e.g., on nodeLocks churn); change it to depend only on
isCollaborationActive and use a ref to read the latest sendPresenceUpdate inside
the effect: create a sendPresenceUpdateRef and update
sendPresenceUpdateRef.current whenever sendPresenceUpdate changes, then inside
the useEffect for heartbeat call sendPresenceUpdateRef.current(...) from
fireCursorHeartbeat and from the immediate fire so the interval remains stable
(keep lastBroadcastCursorRef and CURSOR_HEARTBEAT_INTERVAL_MS usage) and still
clearInterval on cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24a7ab54-be4a-47a8-8161-d4431bacb695
⛔ Files ignored due to path filters (1)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
common/config/rush/.pnpmfile.cjsworkspaces/ballerina/ballerina-extension/src/extension.tsworkspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.tsworkspaces/ballerina/ballerina-extension/src/utils/remote-fs/diagnostics-bridge.tsworkspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsxworkspaces/ballerina/bi-diagram/src/components/DiagramCanvas.tsxworkspaces/common-libs/ui-toolkit/src/components/ParamManager/ParamManager.tsxworkspaces/mi/mi-extension/src/visualizer/activate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/ballerina/bi-diagram/src/components/DiagramCanvas.tsx
- workspaces/ballerina/ballerina-extension/src/extension.ts
Comment on lines
+292
to
+319
| constructor(messenger?: Messenger) { | ||
| this.messenger = messenger; | ||
|
|
||
| if (messenger) { | ||
| // Subscribe to lock changes from OCT and broadcast to webviews | ||
| const lockManager = CollaborationLockManager.getInstance(); | ||
| lockManager.onLocksChanged((filePath, locks) => { | ||
| console.log(`[Lock] Broadcasting lock update for ${filePath} to webviews`); | ||
| this.messenger.sendNotification(nodeLockUpdated, { | ||
| type: 'webview', | ||
| webviewType: 'ballerina.visualizer' | ||
| }, { | ||
| locks, | ||
| }); | ||
| }); | ||
|
|
||
| // Subscribe to cursor changes and broadcast to webviews | ||
| lockManager.onCursorsChanged((cursors) => { | ||
| console.log(`[Cursor] Broadcasting cursor update to webviews`); | ||
| this.messenger.sendNotification(diagramCursorUpdated, { | ||
| type: 'webview', | ||
| webviewType: 'ballerina.visualizer' | ||
| }, { | ||
| cursors: Array.from(cursors.values()), | ||
| }); | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Listener leak: disposables from onLocksChanged / onCursorsChanged are discarded.
CollaborationLockManager.getInstance() is a singleton, and both subscription methods return a vscode.Disposable that must be invoked to remove the listener (see lock-manager.ts:715-743). Because the constructor throws away the returned disposables, every new BiDiagramRpcManager (e.g., per webview/session, hot-reload, or tests) will register additional listeners on the singleton forever. This results in:
- duplicate
nodeLockUpdated/diagramCursorUpdatednotifications being broadcast, - stale
this.messengerreferences being invoked after webview/messenger teardown, - an unbounded growth of the listener arrays inside the lock manager.
Capture the disposables and expose a dispose() method (ideally track them so the owner — the RPC handler that new-s the manager — can clean up on deactivation).
🛠️ Proposed fix
// Messenger instance for broadcasting notifications
private messenger?: Messenger;
+ private readonly disposables: vscode.Disposable[] = [];
constructor(messenger?: Messenger) {
this.messenger = messenger;
if (messenger) {
// Subscribe to lock changes from OCT and broadcast to webviews
const lockManager = CollaborationLockManager.getInstance();
- lockManager.onLocksChanged((filePath, locks) => {
- console.log(`[Lock] Broadcasting lock update for ${filePath} to webviews`);
- this.messenger.sendNotification(nodeLockUpdated, {
- type: 'webview',
- webviewType: 'ballerina.visualizer'
- }, {
- locks,
- });
- });
-
- // Subscribe to cursor changes and broadcast to webviews
- lockManager.onCursorsChanged((cursors) => {
- console.log(`[Cursor] Broadcasting cursor update to webviews`);
- this.messenger.sendNotification(diagramCursorUpdated, {
- type: 'webview',
- webviewType: 'ballerina.visualizer'
- }, {
- cursors: Array.from(cursors.values()),
- });
- });
+ this.disposables.push(
+ lockManager.onLocksChanged((filePath, locks) => {
+ messenger.sendNotification(
+ nodeLockUpdated,
+ { type: 'webview', webviewType: 'ballerina.visualizer' },
+ { locks }
+ );
+ })
+ );
+ this.disposables.push(
+ lockManager.onCursorsChanged((cursors) => {
+ messenger.sendNotification(
+ diagramCursorUpdated,
+ { type: 'webview', webviewType: 'ballerina.visualizer' },
+ { cursors: Array.from(cursors.values()) }
+ );
+ })
+ );
}
}
+
+ dispose(): void {
+ while (this.disposables.length) {
+ this.disposables.pop()?.dispose();
+ }
+ }Note this also switches the callbacks to use the captured messenger param (which is already narrowed to Messenger), avoiding the strict-null hazard of reading the optional this.messenger inside the closure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts`
around lines 292 - 319, The constructor currently subscribes to
CollaborationLockManager.getInstance() via onLocksChanged and onCursorsChanged
but drops the returned vscode.Disposable objects, leaking listeners and stale
this.messenger captures; fix by storing those Disposables (e.g., push them into
a private disposables: vscode.Disposable[] on the BiDiagramRpcManager class)
when calling onLocksChanged and onCursorsChanged, change the callbacks to
capture the local messenger parameter (not read optional this.messenger inside
closures), and add a public dispose() method that iterates disposables and calls
dispose() to remove the listeners; ensure any owner that constructs
BiDiagramRpcManager calls dispose() on teardown.
Comment on lines
+1063
to
+1086
| if (!artifacts || artifacts.length === 0) { | ||
| const projectInfo = StateMachine.context().projectInfo; | ||
| if (projectInfo) { | ||
| try { | ||
| const freshStructure = await buildProjectsStructure(projectInfo, StateMachine.langClient(), true); | ||
| const freshArtifacts: typeof artifacts = []; | ||
| for (const project of freshStructure.projects ?? []) { | ||
| for (const list of Object.values(project.directoryMap ?? {})) { | ||
| for (const artifact of list as typeof artifacts) { | ||
| freshArtifacts.push(artifact); | ||
| if (artifact.resources) { | ||
| freshArtifacts.push(...artifact.resources as typeof artifacts); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| resolve({ artifacts: freshArtifacts }); | ||
| notifyDeletionUpdate(); | ||
| return; | ||
| } catch (err) { | ||
| console.error('>>> error refreshing project structure after node deletion', err); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how updateSourceCode signals "no artifacts updated" vs "failure"
rg -nP --type=ts -C3 '^\s*(export\s+)?(async\s+)?function\s+updateSourceCode\b' workspaces/ballerina/ballerina-extension/src/utils/source-utils.ts
rg -nP --type=ts -C2 'return\s+\[\]|return\s+artifacts' workspaces/ballerina/ballerina-extension/src/utils/source-utils.tsRepository: wso2/vscode-extensions
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Get full context around those return statements in source-utils.ts
head -n 145 workspaces/ballerina/ballerina-extension/src/utils/source-utils.ts | tail -n 100Repository: wso2/vscode-extensions
Length of output: 4823
🏁 Script executed:
#!/bin/bash
# Find the calling code in rpc-manager.ts that uses updateSourceCode result
rg -n 'updateSourceCode' workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts -B2 -A5Repository: wso2/vscode-extensions
Length of output: 8212
Consider narrowing the empty-artifacts fallback to actual failures.
The fallback rebuilds the entire project structure (buildProjectsStructure(..., true)) whenever artifacts is falsy or an empty array. updateSourceCode legitimately returns an empty array when there are no modifications to apply (e.g., when edits.length === 0 or modificationRequests is empty after processing), which is a normal success case—not an error. A full project-structure rebuild on every node deletion in these cases is expensive and likely unintended.
The current code cannot use artifacts === undefined as a gate since the function always returns an array. Instead, consider:
- Adding an explicit error flag to the return type to distinguish legitimate empty results from actual failures, or
- Only triggering the rebuild when
updateSourceCodethrows an exception, not when it returns an empty array
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/bi-diagram/rpc-manager.ts`
around lines 1063 - 1086, The code is treating an empty artifacts array as a
failure and rebuilding the entire project structure; instead, only trigger
buildProjectsStructure(...) when updateSourceCode actually fails (throws) or
when the update returns an explicit error flag. Modify the logic around where
artifacts is produced so you no longer enter the fallback on artifacts.length
=== 0 — either (A) change the caller of updateSourceCode to return a richer
result (e.g., {artifacts, error?: Error} or {artifacts, success: boolean}) and
check that error/success before calling buildProjectsStructure, or (B) remove
the artifacts.length === 0 check and only run the rebuild inside the catch block
for exceptions from updateSourceCode; keep the existing use of
StateMachine.context(), resolve({ artifacts: ... }), notifyDeletionUpdate(), and
buildProjectsStructure(...) when a real error occurs.
Comment on lines
+453
to
+582
| try { | ||
| // Listen for text selection updates from OCT (webview collaboration) | ||
| unsubscribeOctSelection = rpcClient.onOctUpdateTextSelection((data: CollaborationTextSelection) => { | ||
| if (data.selectedNodes && data.selectedNodes.length > 0) { | ||
| // Handle remote node selection visualization | ||
| // For example: highlight the nodes selected by remote user | ||
| } | ||
| }); | ||
|
|
||
| // Listen for presence updates from OCT (cursor position, locks, etc.) | ||
| unsubscribeOctPresence = rpcClient.onOctRerenderPresence((data: CollaborationPresenceData) => { | ||
|
|
||
| if (data.cursor) { | ||
| console.log(`[OCT Webview] Cursor details:`, JSON.stringify(data.cursor, null, 2)); | ||
| } | ||
|
|
||
| // Skip updating cursors for the current user | ||
| if (data.peerId === currentUserIdRef.current) { | ||
| console.log('[OCT Webview] Skipping own cursor update (peer ID matches current user)'); | ||
| } else { | ||
| // Only show cursors from peers viewing the same flow diagram | ||
| const normalizedCurrentFile = currentModelFileRef.current | ||
| ? normalizeFilePath(currentModelFileRef.current) | ||
| : undefined; | ||
| const localDiagramId = normalizedCurrentFile | ||
| ? `${normalizedCurrentFile}:${targetRef.current?.startLine?.line ?? ''}` | ||
| : undefined; | ||
| // Strictly render remote cursors only for the same file/diagram. | ||
| const isForCurrentDiagram = (() => { | ||
| if (!normalizedCurrentFile) { | ||
| return false; | ||
| } | ||
|
|
||
| if (data.diagramId && localDiagramId) { | ||
| if (diagramIdsMatchWithLegacyFallback(data.diagramId, localDiagramId)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Fall back to file-level matching when diagram IDs diverge due to | ||
| // transient target range differences across peers. | ||
| if (data.filePath) { | ||
| return pathsMatchWithLegacyFallback(data.filePath, normalizedCurrentFile); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (data.filePath) { | ||
| return pathsMatchWithLegacyFallback(data.filePath, normalizedCurrentFile); | ||
| } | ||
|
|
||
| return false; | ||
| })(); | ||
| console.log(`[OCT Webview] Presence data is for current diagram: ${isForCurrentDiagram}`); | ||
| if (!isForCurrentDiagram) { | ||
| // Peer is viewing a different diagram — remove their cursor if present | ||
| setRemoteCursors((prev) => { | ||
| if (!prev.has(data.peerId)) { | ||
| return prev; | ||
| } | ||
| const updated = new Map(prev); | ||
| updated.delete(data.peerId); | ||
| return updated; | ||
| }); | ||
| } else if (data.cursor) { | ||
| console.log(`[OCT Webview] ✅ Updating cursor for peer ${data.peerId} at position (${data.cursor.x}, ${data.cursor.y})`); | ||
| setRemoteCursors((prev) => { | ||
| const updated = new Map(prev); | ||
| updated.set(data.peerId, { | ||
| user: { | ||
| id: data.peerId, | ||
| name: data.peerName, | ||
| color: data.color, | ||
| }, | ||
| cursor: { | ||
| x: data.cursor.x, | ||
| y: data.cursor.y, | ||
| nodeId: data.cursor.nodeId, | ||
| timestamp: data.cursor.timestamp ?? Date.now(), | ||
| }, | ||
| }); | ||
| console.log(`[OCT Webview] Updated remote cursors map, total peers: ${updated.size}`, Array.from(updated.keys())); | ||
| return updated; | ||
| }); | ||
| } else { | ||
| console.log(`[OCT Webview] Removing remote cursor for peer without cursor data: ${data.peerId}`); | ||
| setRemoteCursors((prev) => { | ||
| if (!prev.has(data.peerId)) { | ||
| return prev; | ||
| } | ||
|
|
||
| const updated = new Map(prev); | ||
| updated.delete(data.peerId); | ||
| return updated; | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Use OCT presence as lock source only when authoritative lock subscription is not available. | ||
| // Always process empty lock arrays as an explicit clear for that peer. | ||
| if (!hasAuthoritativeLockUpdatesRef.current && data.locks !== undefined) { | ||
| const normalizedCurrentFile = currentModelFileRef.current | ||
| ? normalizeFilePath(currentModelFileRef.current) | ||
| : undefined; | ||
| const nextPeerLocks: Record<string, any> = {}; | ||
|
|
||
| data.locks.forEach((lock) => { | ||
| if (normalizedCurrentFile && !pathsMatchWithLegacyFallback(lock.filePath, normalizedCurrentFile)) { | ||
| return; | ||
| } | ||
|
|
||
| nextPeerLocks[lock.nodeId] = { | ||
| userId: lock.userId, | ||
| userName: lock.userName, | ||
| timestamp: lock.timestamp, | ||
| }; | ||
| }); | ||
|
|
||
| octPeerLocksRef.current[data.peerId] = nextPeerLocks; | ||
|
|
||
| const mergedLocks = Object.values(octPeerLocksRef.current).reduce((acc, peerLocks) => { | ||
| return { ...acc, ...peerLocks }; | ||
| }, {} as Record<string, any>); | ||
|
|
||
| setNodeLocks(mergedLocks); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| console.log('[OCT] Collaboration notifications not available:', error); | ||
| } |
Contributor
There was a problem hiding this comment.
onOctRerenderPresence closure can see a stale projectPath.
The OCT presence subscription is registered inside a [rpcClient] effect, so the normalizeFilePath / pathsMatchWithLegacyFallback / diagramIdsMatchWithLegacyFallback helpers captured here are the ones created on the first render. Those helpers read the projectPath prop at closure-capture time (lines 1273-1276), so if projectPath becomes available or changes after mount (a common case when the platform context resolves asynchronously), every subsequent remote presence/lock event will be normalized against the old prop and isForCurrentDiagram / pathsMatchWithLegacyFallback will silently mis-match for the rest of the session.
Capture projectPath in a ref that is updated via an effect and have normalizeFilePath read from the ref, or re-subscribe when projectPath changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx`
around lines 453 - 582, The presence handler registered in onOctRerenderPresence
closes over helpers that read projectPath at mount, causing stale path checks;
fix by introducing a projectPathRef (e.g., projectPathRef.current updated in a
useEffect when projectPath prop changes) and change the code used inside the
onOctRerenderPresence callback to call a path-normalizer that reads
projectPathRef.current (or compute normalizedCurrentFile directly using
normalizeFilePath with projectPathRef.current), ensuring
pathsMatchWithLegacyFallback and diagramIdsMatchWithLegacyFallback use the
up-to-date project path; alternatively, re-subscribe the
rpcClient.onOctRerenderPresence listener whenever projectPath changes so the
closure captures the latest projectPath.
Comment on lines
2800
to
2806
| debouncedGetFlowModel(); | ||
| // Fallback retry: if the LS hasn't reparsed the file within 150ms (cold start can take | ||
| // 400–700ms), the debounced fetch above may return the old model. This retry fires after | ||
| // 800ms total, giving the LS enough time to finish reparsing. The generation counter in | ||
| // getFlowModel ensures any earlier stale response is discarded and the latest wins. | ||
| setTimeout(() => getFlowModel(), 800); | ||
| }; |
Contributor
There was a problem hiding this comment.
setTimeout retry is never cleared.
The 800 ms fallback getFlowModel() retry is not tracked, so if the user closes/navigates the diagram (or model.fileName changes) within the window, the timer still fires and drives an RPC round-trip plus setState on a component that may no longer be mounted. Store the timer id in a ref and clear it on unmount and at the start of the next delete so only the most recent retry survives.
🛠️ Suggested shape
- debouncedGetFlowModel();
- // Fallback retry: ...
- setTimeout(() => getFlowModel(), 800);
+ debouncedGetFlowModel();
+ // Fallback retry: ...
+ if (deleteRetryTimerRef.current) {
+ clearTimeout(deleteRetryTimerRef.current);
+ }
+ deleteRetryTimerRef.current = setTimeout(() => {
+ deleteRetryTimerRef.current = undefined;
+ if (isMountedRef.current) getFlowModel();
+ }, 800);Add a matching clearTimeout in the mount/unmount effect that already manages isMountedRef.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/FlowDiagram/index.tsx`
around lines 2800 - 2806, The fallback setTimeout call that invokes
getFlowModel() should store its timer id in a ref (e.g., retryTimerRef) and be
cleared whenever a new retry is scheduled or the component unmounts; update the
code around debouncedGetFlowModel()/getFlowModel() to assign the return of
setTimeout to retryTimerRef.current and call clearTimeout(retryTimerRef.current)
at the start of the function that schedules the retry and inside the
mount/unmount effect that currently manages isMountedRef so only the latest
retry can fire and no timeout calls setState after unmount.
- Update the connector init system-reminder to reference mode='details'
instead of the removed include_full_descriptions parameter, so the
agent doesn't try to pass a nonexistent flag.
- Parallelize store fetches in lookupConnectorFromCache to avoid
sequential double-latency on cache miss.
- Thread the correct itemType ('connector' vs 'inbound') into
toFallbackStoreItems for the static-DB fallback so inbound entries
aren't labelled as 'Connector' when the connectorType field is
absent on the DB row.
Guard LS operation name comparison against undefined, surface stale connector cache fallbacks via logWarn, and fix the foreach payloadFactory example to embed Synapse expressions directly in <format> per the documented rule.
Tools: thread abort signal through connector_tools and deepwiki_tools; fetchWithTimeout honors external aborts; requestWebApproval races the abort signal; data-mapper skips file writes after abort; readOutputSchema uses non-blocking fs; ripgrep truncation returned as partial success and its sensitive-glob list aligned with the shell sandbox; file_tools denies sensitive paths reached via symlink; connectors.ts enriches inbounds via getInboundInfoFromLS. Types/UI: GetInboundInfoRequest is now a discriminated union; AICodeGenerator observes a dedicated content wrapper and lets user scrolls cancel programmatic suppression; SettingsPanel clears persisted memory state when SHOW_MEMORY_UI is false; AIChatFooter also strips loading bashoutput tags; AIPanel regex simplified and thinking-loading check scoped to attrs. Docs: fix async store/respond ordering, v1/v2 expression mixing, cache identity-header guidance, fault payloadFactory, bracketed attributes in examples, registry hardcode recommendation, and duplicate for-each block.
Update the warning message for conflicting artifacts to prevent UI clutter by truncating overly long strings, thee full detailed error remains accessible in console logs
…licts-dep Detect conflicting integration project dependencies and clean up pom.xml
…ads and resource management
project_tools: remove operations no longer fail when the store/cache has no latestVersion. resolveRemoveVersion tries override, pom-declared, and latest in order and falls back to an empty placeholder, matching the RPC manager which ignores the version on remove. file_tools: grep tool now passes --no-ignore and --no-ignore-vcs so .gitignore-covered paths (target/, build/) stay searchable — consistent with the glob tool and its documented behavior. AIPanel/utils: code-fence regex handles empty blocks. Non-empty bodies still consume the trailing newline via \r?\n; empty bodies fall back to a (?<=\n) lookbehind so the closer can match immediately after the opener's own \n. Also includes pnpm-lock update for @ai-sdk/[email protected].
Memory tool removal: deletes memory_tools.ts, its system-prompt section, storage-path helper, MEMORY_TOOL_NAME, tool-action-mapper case, agent.ts registration block and memoryEnabled plumbing, the clearAgentMemory/openAgentMemoryFolder RPCs (mi-core + mi-rpc-client + handler + manager), and the entire SHOW_MEMORY_UI / isMemoryEnabled surface in MICopilotContext, SettingsPanel, ModelSettingsMenu, and AIChatFooter. CLAUDE.md and AWS_BEDROCK_STATUS are updated to match. Review findings: - call mediator doc now clarifies initAxis2ClientOptions is blocking-only; cache <headersToExcludeInHash> example no longer carries Authorization. - readOutputSchema basename-normalizes operationName and verifies the resolved path is inside outputSchemaDir. - getLocalInboundCatalog accepts both builtin- and inbuilt- inbound-endpoint type strings. - connector_tools prefers action.outputSchemaPath (new readOutputSchemaFile helper), falling back to the connector-level directory only when the operation-specific file is missing. - DeepWiki MCP client creation and tool discovery now race against mainAbortSignal via raceWithAbort; aborts return a distinct DEEPWIKI_ABORTED result. - abort-utils introduces OperationAbortedError + isOperationAbortedError; processItem accepts an AbortSignal, checks at blocking steps, and rethrows aborts out of its generic catch so interrupts don't get swallowed as per-item failures. - Jump-to-latest button uses type="button" + aria-label and is no longer gated on backendRequestTriggered. - file_tools moves the realpath sensitive-path denylist above the !allowOutside branch so read paths can't exfiltrate credentials via symlink.
- Propagate aborts from connector LS, DeepWiki, and manage-connector tools - Add realpath containment to connector schema reads and file-tool writes - Force --with-filename on rg count mode so single-file searches parse - Stamp chatId on agent events so stale events from interrupted runs drop - Normalize connector/inbound artifact ids in tool-action mapper - Consider custom model ids in ModelSettings reset-to-default detection - Replace event.isTrusted scroll heuristic with explicit user-intent listeners - Capture DeepWiki MCP client on late resolution so it is closed on abort - Reject bundled inbound ids in manage_connector regardless of bucket - Correct Send mediator and Script/ForEach V2 docs in mediator reference
Replace --prerelease flag and isPreRelease env var in download-ls.js with a flexible --tag parameter (latest, prerelease, v1.5.0) and BALLERINA_LS_TAG env var. Add --resolve-version mode to emit the concrete release version for use as a cache key. Update CI pipelines to thread ballerinaLsTag through release-vsix and build workflows, and fix the Ballerina LS cache key from static 'ballerina-ls' to version-specific 'ballerina-ls-<resolved-version>'. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- ForEach V2: correct parallel-execution default (true) and keep the verified aggregation attributes note - Script mediator: document MI 4.5+ java.lang class-access sandbox and replace the outdated Thread.sleep claim - connector_tools: rethrow aborts in catalog mode, checkpoint per-op/ per-connection reads in buildLSOperationDetails, and fall back to extractedConnectorPath so readOutputSchemaFile always has a trusted base dir for realpath containment - deepwiki_tools: close late-resolving MCP client when raceWithAbort lost the race so no connection leaks after abort - AIChatFooter: finalizeInterruptionUi takes an origin so backend aborts render the neutral "*[Interrupted]*" marker instead of claiming the user interrupted
…rd-issues Fix clipboard issues in MCP Inspector extension
Sync WSO2 Platform Core MCP fixes to main
Introduce support to override connector dependencies in descriptor.yml
- Change workflow icon from 'function' to 'workflow' - Change activity icon from 'function' to 'task' - Update getDirectoryMapKeyAndIcon() to return correct icons
fix: use correct icons for workflow and activity tree items
…n-stable/mi-1777485258
Automatic Sync: Merge changes from stable/mi to main
Increase heap size to fix build issue
…I diagram testing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (16)
workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.fr.json-2-2 (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect French localization for sign-out and read-only labels
Line 2 mistranslates “Signed out”, and Line 21 is still in English.
Suggested wording
- "Signed out successfully!": "Signature réussie !", + "Signed out successfully!": "Déconnexion réussie !", - "Make Read-Only": "Make Read-Only", + "Make Read-Only": "Passer en lecture seule",Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.fr.json` at line 2, The French translation for the "Signed out successfully!" key is incorrect and another "Read-only" label remains untranslated; update the localization entries so "Signed out successfully!" maps to an appropriate French phrase like "Déconnexion réussie !" or "Vous avez été déconnecté(e)." and replace the English "Read-only" string with "Lecture seule" or "En lecture seule"; locate and edit the keys "Signed out successfully!" and the "Read-only" key in bundle.l10n.fr.json to apply these corrected French values.workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.zh-cn.json-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Chinese translation typos in user-facing messages
Line 12 and Line 39 contain wording typos that read unnaturally in Simplified Chinese and will show directly in the UI.
Suggested wording
- "Stop Collaboration Session": "停止协会话", + "Stop Collaboration Session": "停止协作会话", - "No Open Collaboration Server configured. Please set the server URL in the settings.": "没有配置协作服务器。请先置服务器 URL。", + "No Open Collaboration Server configured. Please set the server URL in the settings.": "未配置协作服务器。请先在设置中配置服务器 URL。",Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.zh-cn.json` at line 12, Replace the unnatural translation value "停止协会话" for the JSON key "Stop Collaboration Session" with the correct, natural Simplified Chinese "停止协作会话"; also find the other occurrence of the same user-facing message (the duplicate entry referenced around line 39) and update its value to "停止协作会话" so both UI strings display the corrected wording.workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.pt-br.json-2-2 (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix PT-BR translation for sign-out status
Line 2 currently translates to “signature/subscription completed,” not “signed out.”
Suggested wording
- "Signed out successfully!": "Assinatura efetuada com sucesso!", + "Signed out successfully!": "Sessão encerrada com sucesso!",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.pt-br.json` at line 2, The Portuguese translation for the JSON key "Signed out successfully!" is incorrect; update the value for that key in bundle.l10n.pt-br.json to a correct PT‑BR phrase such as "Desconectado com sucesso!" (or "Sessão encerrada com sucesso!"), replacing the current "Assinatura efetuada com sucesso!" so the message correctly reflects a sign-out event.workspaces/oct/open-collaboration-vscode/README.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix several typos in the README
Four copy errors in the user-facing docs:
Line Current Should be 5 "and and what changes""and what changes"46 "you aquired""you acquired"52 "the hosts session""the host's session"61 "their respecive""their respective"✏️ Proposed fix
-This way of remote collaboration reduces confusion and maximizes productivity. All participants see what the others are looking at and and what changes they propose in real-time. +This way of remote collaboration reduces confusion and maximizes productivity. All participants see what the others are looking at and what changes they propose in real-time.-1. After you aquired an invite code, click on the share item in the status bar and select +1. After you acquired an invitation code, click on the share item in the status bar and select-4. That's it! After that VSCode will connect to the hosts session +4. That's it! After that VS Code will connect to the host's session-This widget lists all joined users and their respecive cursor colors. +This widget lists all joined users and their respective cursor colors.Also applies to: 46-46, 52-52, 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/README.md` at line 5, Fix the four typos in the README by replacing the exact erroneous phrases: change "and and what changes" to "and what changes", "you aquired" to "you acquired", "the hosts session" to "the host's session" (add the apostrophe), and "their respecive" to "their respective"; update the corresponding occurrences in the README content so the user-facing docs read correctly.workspaces/oct/open-collaboration-vscode/README.md-53-53 (1)
53-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate README to use correct status bar label "Collaborating" instead of "Connected"
Line 53 instructs users to click the
"Connected"item in the status bar, but the actual UI displays"Collaborating". The extension'scollaboration-status-service.tssets the status bar text tovscode.l10n.t('Collaborating')when a user joins a session (StatusBarState.Connected). Update the documentation to match the actual UI label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/README.md` at line 53, Update the README step to match the actual status bar label by replacing the reference to "Connected" with "Collaborating"; the UI text is set in collaboration-status-service.ts under StatusBarState.Connected via vscode.l10n.t('Collaborating'), so update the sentence that says click the "Connected" item to instead say click the "Collaborating" item and select "Close Current Session".workspaces/oct/open-collaboration-vscode/src/utils/uri.ts-75-80 (1)
75-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
vscode.Uri.parsecalled withoutstrict: trueinRoomUri.createandnormalizeServerUri.
RoomUri.parse(line 91) already usesstrict: trueand throws on bad input, but the sibling functions silently accept malformed URLs. A user-entered server URL that is not a valid URI will be silently coerced into a garbagevscode.UribynormalizeServerUri(called fromSettings.getServerUrl), and then forwarded to the server — producing an opaque connection error with no early-exit diagnostic.🛡️ Proposed fix
export function create(roomUri: RoomUri<true>): string { - const uri = vscode.Uri.parse(roomUri.serverUrl); + const uri = vscode.Uri.parse(roomUri.serverUrl, true); return uri.with({ fragment: roomUri.roomId }).toString(true); }export function normalizeServerUri(serverUrl: string): string { - const uri = vscode.Uri.parse(serverUrl); + const uri = vscode.Uri.parse(serverUrl, true); return uri.toString(true); }Callers of
normalizeServerUri(e.g.Settings.getServerUrl) should wrap the call in a try/catch and returnundefinedon parse failure so an invalid configured URL surfaces a clear message rather than a malformed connection attempt.Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/utils/uri.ts` around lines 75 - 80, The create and normalizeServerUri helpers call vscode.Uri.parse without strict:true, so malformed server URLs are silently coerced; update create (RoomUri.create) and normalizeServerUri to call vscode.Uri.parse(..., { strict: true }) so parsing will throw on bad input, and update callers such as Settings.getServerUrl to wrap normalizeServerUri in a try/catch and return undefined (or surface a clear error) when parsing fails; also ensure RoomUri.parse remains strict and that error paths produce a clear diagnostic instead of forwarding a malformed URI to the server.workspaces/oct/open-collaboration-vscode/scripts/translate.ts-58-66 (1)
58-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
inoperator instead of falsy check for key presence detection.
!existingContent[key]treats any falsy translation value (including"") as missing and queues it for re-translation on every run. Use!(key in existingContent)for both checks.🐛 Proposed fix
for (const key of Object.keys(content)) { - if (!existingContent[key]) { + if (!(key in existingContent)) { missing[key] = content[key]; } } for (const key of Object.keys(existingContent)) { - if (!content[key]) { + if (!(key in content)) { extra[key] = existingContent[key]; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/scripts/translate.ts` around lines 58 - 66, The current loops use falsy checks on object lookups which treat empty strings or other falsy translations as missing; in the for-loops that iterate over Object.keys(content) and Object.keys(existingContent) (referencing variables content, existingContent, missing, extra), change the presence checks from !existingContent[key] and !content[key] to using the in operator: use !(key in existingContent) in the first loop and !(key in content) in the second loop so that keys with falsy values are correctly treated as present.workspaces/oct/open-collaboration-vscode/src/chat-webview/src/styles.css-138-140 (1)
138-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStylelint
selector-type-caseerror on::highlight(messageTarget)is a false positive — suppress it.The name defined in the
set()method is used as the parameter of the::highlight()pseudo-element selector. Because highlight names are case-sensitive custom identifiers (not CSS type selectors), the name must match whateverCSS.highlights.set('messageTarget', …)registers in JavaScript — renaming it tomessagetargetwould silently break the highlight. The lint rule should be suppressed for this line.Additionally,
color: blueis a non-theme-aware hardcoded value; replace with a CSS variable.🔧 Proposed fix
+/* stylelint-disable-next-line selector-type-case */ ::highlight(messageTarget) { - color: blue; + color: var(--vscode-editorInfo-foreground, blue); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/src/styles.css` around lines 138 - 140, Suppress the false-positive Stylelint rule on the custom highlight selector and replace the hard-coded color with a theme variable: add a Stylelint suppression for selector-type-case immediately before the ::highlight(messageTarget) rule (so the linter ignores the custom identifier), keep the identifier exactly as used in CSS.highlights.set('messageTarget', …), and change color: blue to a theme-aware CSS variable (e.g., use an existing editor/extension variable like --vscode-editor-foreground or your project’s --oc-... highlight color) to avoid hard-coded colors.workspaces/oct/open-collaboration-vscode/src/chat-webview/src/styles.css-59-69 (1)
59-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded hex colours in message bubbles won't adapt to VS Code themes.
.message.me pre(#007acc) and.message.other pre(#717171) use fixed colours while the rest of the file correctly usesvar(--vscode-button-background, …)andvar(--vscode-foreground). In light or custom themes these bubbles can clash with the editor chrome or have poor contrast.🎨 Proposed fix using VS Code theme variables
.message.me pre { - background: `#007acc`; - color: white; + background: var(--vscode-button-background, `#007acc`); + color: var(--vscode-button-foreground, `#fff`); border-radius: 12px 12px 0 12px; } .message.other pre { - background: `#717171`; - color: white; + background: var(--vscode-input-background, `#717171`); + color: var(--vscode-foreground, `#fff`); border-radius: 12px 12px 12px 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/src/styles.css` around lines 59 - 69, Replace the hardcoded hex colours in the message bubble rules so they respect VS Code themes: update the .message.me pre and .message.other pre selectors to use theme CSS variables (e.g., var(--vscode-button-background, `#007acc`) or another appropriate fallback for the background and var(--vscode-foreground, white) for text) while keeping the existing border-radius values intact; ensure both selectors use theme vars rather than fixed hex values so bubbles adapt to light/dark/custom themes.workspaces/oct/open-collaboration-vscode/src/chat-webview/src/message-input.tsx-31-31 (1)
31-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
NodeJS.Timeoutis a Node.js type — useReturnType<typeof setTimeout>in a browser webview.This component is bundled for the browser (
platform: 'browser'in esbuild). If the webview tsconfig does not include@types/node,NodeJS.Timeoutis an unknown type and compilation fails.ReturnType<typeof setTimeout>resolves to the correct environment type (numberin browsers,NodeJS.Timeoutin Node).🐛 Proposed fix
- const [usersWriting, setUsersWriting] = useState<Record<string, NodeJS.Timeout>>({}); + const [usersWriting, setUsersWriting] = useState<Record<string, ReturnType<typeof setTimeout>>>({});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/src/message-input.tsx` at line 31, The state type uses the Node.js-only type NodeJS.Timeout which breaks browser builds; update the useState declaration for usersWriting (and any related uses of setUsersWriting) to use ReturnType<typeof setTimeout> instead of NodeJS.Timeout so the timeout type resolves correctly in browser webviews; adjust any related type annotations or function signatures that reference NodeJS.Timeout to the same ReturnType<typeof setTimeout>.workspaces/oct/open-collaboration-vscode/src/chat-webview/chat-webview.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo:
'unkown user'→'unknown user'This string is user-visible in the chat window.
✏️ Proposed fix
- const messageObj: ChatMessage = { message, user: user?.name ?? 'unkown user', color: user?.color, isDirect }; + const messageObj: ChatMessage = { message, user: user?.name ?? 'unknown user', color: user?.color, isDirect };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/chat-webview.ts` at line 34, The ChatMessage construction uses a misspelled fallback username ("unkown user"); update the object literal in messageObj within chat-webview.ts so the fallback string is spelled "unknown user" (ensure the change is applied where ChatMessage is created/assigned in the messageObj declaration and search for other occurrences of 'unkown user' to correct them).workspaces/oct/open-collaboration-protocol/src/transport/socket-io-transport.ts-54-56 (1)
54-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
ready.reject()called without a reason — the rejection is opaque.Any
write()caller that catches the rejection fromthis.ready.promisewill receiveundefinedas the error, making it impossible to distinguish a disconnect from a timeout.✏️ Proposed fix
this.socket.on('disconnect', (_reason, _description) => { - this.ready.reject(); + this.ready.reject(new Error('Socket disconnected')); this.ready = new Deferred();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-protocol/src/transport/socket-io-transport.ts` around lines 54 - 56, The rejection of the connection Deferred is currently opaque; change the socket 'disconnect' handler (the this.socket.on('disconnect', (_reason, _description) => { ... }) block) to call this.ready.reject(...) with a descriptive Error or reason (e.g., include _reason and/or _description) before recreating this.ready = new Deferred(); ensure callers of write() awaiting this.ready.promise receive that Error so they can distinguish disconnects from timeouts.workspaces/oct/open-collaboration-vscode/src/extension.ts-180-185 (1)
180-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOCTOU: capture
CollaborationInstance.Currentbeforeawait leave()indeactivate().
CollaborationInstance.Currentis accessed twice.leave()is async and may nullify or replaceCurrentbefore the second access; the subsequentdispose()call could be a no-op or target the wrong instance.🐛 Proposed fix
export async function deactivate(): Promise<void> { - await CollaborationInstance.Current?.leave(); - CollaborationInstance.Current?.dispose(); + const instance = CollaborationInstance.Current; + await instance?.leave(); + instance?.dispose(); await closeSharedEditors(); removeWorkspaceFolders(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/extension.ts` around lines 180 - 185, Capture the current CollaborationInstance once at the start of deactivate(): assign CollaborationInstance.Current to a local const (e.g., current = CollaborationInstance.Current), then call await current?.leave() and current?.dispose() so both operations target the same instance even if CollaborationInstance.Current changes during the async leave; keep the subsequent await closeSharedEditors() and removeWorkspaceFolders() calls as-is.workspaces/oct/open-collaboration-vscode/src/chat-webview/src/webview.tsx-94-114 (1)
94-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a stable key instead of array index.
key={idx}causes React to misidentify DOM nodes when items are removed or reordered. Even in an append-only list, if messages are ever cleared and reloaded, React may reuse stale DOM nodes.♻️ Proposed fix
- <div key={idx} className={`message ${msg.user === 'me' ? 'me' : 'other'}`}> + <div key={`${msg.user}-${msg.timestamp}-${idx}`} className={`message ${msg.user === 'me' ? 'me' : 'other'}`}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/src/webview.tsx` around lines 94 - 114, Replace the unstable key={idx} used in the messages.map render with a stable unique identifier from each message (e.g., use msg.id) so React can correctly track DOM nodes; if messages do not currently include a persistent id, generate and attach one when messages are created (or fall back to a stable composite like `${msg.user}-${msg.timestamp}`) and use that value as the key in the map inside webview.tsx (referencing messages.map, key, msg.id, msg.timestamp, msg.user).workspaces/oct/open-collaboration-vscode/src/collaboration-status-view.ts-44-82 (1)
44-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPending users may be incorrectly rendered as "You" when
ownUserDataresolves toundefined.
peer.peerIdisundefinedfor pending users (Line 104 ingetChildren). IfownUserDataalso resolves toundefined,self?.idisundefined, andundefined === undefinedmakes the pending user pass both the Line 53 "You" tag check and the Line 63elsebranch (contextValue = 'self').🐛 Proposed fix
- if (peer.peerId === self?.id) { + if (peer.peerId !== undefined && peer.peerId === self?.id) { tags.push(vscode.l10n.t('You')); } // ... - if (self?.id !== peer.peerId) { + if (peer.peerId === undefined || self?.id !== peer.peerId) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/collaboration-status-view.ts` around lines 44 - 82, In getTreeItem, pending users with undefined peer.peerId can be misclassified as "You" because self?.id may also be undefined; change the identity checks to first ensure self exists (e.g., check self !== undefined/null) before comparing peer.peerId to self.id and when deciding the self branch for contextValue; update the two places that use self?.id (the tag push that pushes vscode.l10n.t('You') and the outer if that sets contextValue/self icon) to require a defined self (or self.id) so undefined === undefined cannot mark a pending user as "You" or 'self'.workspaces/oct/open-collaboration-vscode/src/collaboration-instance.ts-213-214 (1)
213-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
onDidPendingChangeEmitteris not disposed.The other two
EventEmitters are pushed tothis.toDispose(Lines 294-295), butonDidPendingChangeEmitteris not, so its subscribers are not released when the instance is disposed.🛠️ Suggested fix
this.toDispose.push(this.onDidUsersChangeEmitter); this.toDispose.push(this.onDidDisposeEmitter); + this.toDispose.push(this.onDidPendingChangeEmitter);Also applies to: 287-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/collaboration-instance.ts` around lines 213 - 214, The EventEmitter onDidPendingChangeEmitter is never disposed, so its subscribers leak; add it to the instance disposal collection the same way the other emitters are (push this.onDidPendingChangeEmitter into this.toDispose) or explicitly call this.onDidPendingChangeEmitter.dispose() from the class dispose method; update the constructor or dispose method where the other emitters are registered (referencing onDidPendingChangeEmitter, onDidPendingChange and this.toDispose) so it is cleaned up when the collaboration instance is disposed.
🧹 Nitpick comments (8)
workspaces/oct/open-collaboration-vscode/tsconfig.json (1)
7-7: ⚡ Quick winUpdate TypeScript JSX compiler option to match React 19 toolchain
This project uses React 19 (per package.json) but configures
"jsx": "react"(classic transform). Update to"jsx": "react-jsx"(automatic transform) to align with React 19's modern JSX runtime, which eliminates the need to import React for JSX and enables performance optimizations. React 19's upgrade guide requires the modern transform, and TypeScript's official JSX documentation confirms"react-jsx"is the automatic runtime for React 17+ tooling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/tsconfig.json` at line 7, The tsconfig's JSX compiler option is set to the legacy value "jsx": "react"; update the JSX setting in the TypeScript configuration (the "jsx" property in tsconfig.json) to "react-jsx" so the project uses the automatic React JSX runtime compatible with React 19 (eliminating the need to import React for JSX and enabling modern transforms).workspaces/oct/open-collaboration-vscode/scripts/translate.ts (1)
73-78: ⚡ Quick winAdd placeholder-preservation validation after each translation.
The script doesn't verify that
{N}placeholders from the source string survive the DeepL round-trip. This is the direct root cause of the broken0}/1}strings inbundle.l10n.tr.json(lines 32, 48, 55). A post-translation check and warning would catch these regressions immediately.♻️ Proposed addition after line 77
for (let i = 0; i < missingPairs.length; i++) { existingContent[missingPairs[i][0]] = translations[i].text; + // Validate that all {N} placeholders from the source survive translation. + const placeholderRe = /\{\d+\}/g; + const srcPlaceholders = (missingPairs[i][1].match(placeholderRe) ?? []).sort().join(','); + const dstPlaceholders = (translations[i].text.match(placeholderRe) ?? []).sort().join(','); + if (srcPlaceholders !== dstPlaceholders) { + console.warn(`⚠ Placeholder mismatch for key "${missingPairs[i][0]}" in '${vscodeLang}': expected [${srcPlaceholders}], got [${dstPlaceholders}]`); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/scripts/translate.ts` around lines 73 - 78, After calling translator.translateText for missingPairs, validate that numeric placeholders like {0}, {1}, etc. from the source string (use the original source value via missingPairs/missing mapping) are present in the corresponding translation before writing into existingContent; for each index i, extract source = missing[missingPairs[i][0]] (or use missingPairs[i][1] if that holds the source), compute the set of placeholders in source and in translations[i].text, and if they differ log a warning including the translation key (missingPairs[i][0]), source, and translated text and either (a) skip replacing that key or (b) revert to the source string—choose consistent behavior and document it in the log so broken {N} placeholders aren’t introduced into existingContent. Ensure this check runs immediately inside the for loop that assigns existingContent[missingPairs[i][0]] so no invalid translation is written.workspaces/oct/open-collaboration-vscode/src/extension-web.ts (1)
27-32: ⚡ Quick winAdd
try-finallyto guarantee cleanup indeactivate.If
leave()rejects, execution stops beforedispose(),closeSharedEditors(), andremoveWorkspaceFolders()are called, leaving stale shared editors and workspace folders behind.♻️ Proposed fix
export async function deactivate(): Promise<void> { - await CollaborationInstance.Current?.leave(); - CollaborationInstance.Current?.dispose(); - await closeSharedEditors(); - removeWorkspaceFolders(); + try { + await CollaborationInstance.Current?.leave(); + CollaborationInstance.Current?.dispose(); + } finally { + await closeSharedEditors(); + removeWorkspaceFolders(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/extension-web.ts` around lines 27 - 32, The deactivate function must guarantee cleanup even if CollaborationInstance.Current.leave() rejects: wrap the await CollaborationInstance.Current?.leave() in a try-finally and move CollaborationInstance.Current?.dispose(), await closeSharedEditors(), and removeWorkspaceFolders() into the finally block so they always run; use the existing symbol names (deactivate, CollaborationInstance.Current, leave, dispose, closeSharedEditors, removeWorkspaceFolders) and preserve optional chaining/await semantics (await leave() inside try, then call CollaborationInstance.Current?.dispose(), await closeSharedEditors(), and removeWorkspaceFolders() inside finally).workspaces/oct/open-collaboration-vscode/src/chat-webview/src/message-input.tsx (1)
93-95: ⚡ Quick win
throttle(...)is evaluated on every render insideuseCallback— useuseMemoinstead.
throttle(...)is called as an argument expression on every render;useCallbackreceives a freshly created throttled function each time and discards it whenmessengeris stable. On every keystroke the component re-renders, creating and immediately garbage-collecting a throttle closure.useMemoevaluates its factory only when deps change, avoiding the per-render allocation.♻️ Proposed fix
- const sendWritingNotification = React.useCallback(throttle(() => { - messenger.sendNotification(isWriting, { type: 'extension' }); - }, WRITING_NOTIFICATION_SEND_THROTTLE_MS), [messenger]); + const sendWritingNotification = React.useMemo(() => throttle(() => { + messenger.sendNotification(isWriting, { type: 'extension' }); + }, WRITING_NOTIFICATION_SEND_THROTTLE_MS), [messenger]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/src/message-input.tsx` around lines 93 - 95, The throttled function is being recreated on every render because throttle(...) is called inside useCallback; change this to create the throttled wrapper once with useMemo and then reference that memoized throttled function inside useCallback. Specifically, memoize throttle(() => { messenger.sendNotification(isWriting, { type: 'extension' }) }, WRITING_NOTIFICATION_SEND_THROTTLE_MS) with useMemo (deps: [messenger, isWriting] or only [messenger] if isWriting should be captured at call time), then have sendWritingNotification be a stable callback that calls the memoized throttled function; update references to sendWritingNotification, throttle, useCallback, useMemo, WRITING_NOTIFICATION_SEND_THROTTLE_MS, messenger and isWriting accordingly.workspaces/oct/open-collaboration-vscode/src/extension.ts (1)
99-122: 🏗️ Heavy lift
(instance as any)bypasses the type system — refactors toCollaborationInstancewill silently break this API.Accessing
(instance as any).yjsand(instance as any).yjsAwarenessby name will silently returnundefinedif those private fields are renamed or restructured. The public API surface (getSharedDoc,getAwareness,getClientId) is also typedanyinstead ofY.Doc/Awareness, providing no type safety to consumers.Add typed read-only accessors directly on
CollaborationInstance:// In CollaborationInstance: get sharedDoc(): Y.Doc | undefined { return this.yjs; } get yjsAwarenessInstance(): Awareness | undefined { return this.yjsAwareness; }Then update
extension.ts:getSharedDoc: () => CollaborationInstance.Current?.sharedDoc, getAwareness: () => CollaborationInstance.Current?.yjsAwarenessInstance, getClientId: () => CollaborationInstance.Current?.sharedDoc?.clientID,And tighten the interface return types:
getSharedDoc(): Y.Doc | undefined; getAwareness(): Awareness | undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/extension.ts` around lines 99 - 122, The current getters (getSharedDoc, getAwareness, getClientId) use (instance as any).yjs / yjsAwareness which bypasses types and will silently break if private fields are renamed; add typed read-only accessors on CollaborationInstance (for example: get sharedDoc(): Y.Doc | undefined { return this.yjs; } and get yjsAwarenessInstance(): Awareness | undefined { return this.yjsAwareness; }) so the internal yjs and yjsAwareness private fields are not accessed via any, then update getSharedDoc, getAwareness, getClientId to return CollaborationInstance.Current?.sharedDoc, CollaborationInstance.Current?.yjsAwarenessInstance, and CollaborationInstance.Current?.sharedDoc?.clientID respectively and tighten their signatures to return Y.Doc | undefined and Awareness | undefined (and clientID type) instead of any.workspaces/oct/open-collaboration-vscode/src/collaboration-status-view.ts (1)
29-42: ⚡ Quick winEvent subscription disposables are not tracked — stale listeners may linger across room joins.
onDidUsersChange,onDidPendingChange, andonDidDisposeall return disposables that are silently discarded. IfonConnectionis called for a new room before the previous instance firesonDidDispose, the old listeners remain attached and continue to fireonDidChangeTreeDataEmitter.♻️ Proposed fix
+ private connectionDisposables: vscode.Disposable[] = []; onConnection(instance: CollaborationInstance) { + this.connectionDisposables.forEach(d => d.dispose()); + this.connectionDisposables = []; this.instance = instance; - instance.onDidUsersChange(() => { + this.connectionDisposables.push(instance.onDidUsersChange(() => { this.onDidChangeTreeDataEmitter.fire(); - }); - instance.onDidPendingChange(() => { + })); + this.connectionDisposables.push(instance.onDidPendingChange(() => { this.onDidChangeTreeDataEmitter.fire(); - }); - instance.onDidDispose(() => { + })); + this.connectionDisposables.push(instance.onDidDispose(() => { this.instance = undefined; this.onDidChangeTreeDataEmitter.fire(); - }); + })); this.onDidChangeTreeDataEmitter.fire(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/collaboration-status-view.ts` around lines 29 - 42, onConnection currently discards the disposables returned by instance.onDidUsersChange/onDidPendingChange/onDidDispose causing stale listeners; update CollaborationStatusView to store these disposables (e.g., a property like this.connectionDisposables or reuse this.instanceDisposable) and when onConnection is called first dispose any existing disposables, then assign and push the new disposables returned from instance.onDidUsersChange, instance.onDidPendingChange, and instance.onDidDispose; also ensure the onDidDispose callback disposes the stored disposables and clears the instance reference before firing this.onDidChangeTreeDataEmitter.workspaces/oct/open-collaboration-vscode/src/chat-webview/chat-webview.ts (1)
31-60: ⚡ Quick winPrefer the scoped
collabInstanceover the globalCollaborationInstance.Currentinside room callbacks.Lines 33, 112–114, and 127 all access the global
CollaborationInstance.Currentinside a closure that already holds a reference to the specific instance viacollabInstance. IfCurrentis ever replaced (e.g., the user leaves and immediately rejoins), these callbacks could operate on a different instance than the one they were registered with.♻️ Proposed refactor (relevant lines)
- const user = (await CollaborationInstance.Current?.connectedUsers)?.find(u => u.id === userId); + const user = (await collabInstance.connectedUsers)?.find(u => u.id === userId);- CollaborationInstance.Current?.connection.chat.sendDirectMessage(message.target, message.message); + collabInstance.connection.chat.sendDirectMessage(message.target, message.message);- CollaborationInstance.Current?.connection.chat.sendMessage(message.message); + collabInstance.connection.chat.sendMessage(message.message);- CollaborationInstance.Current?.connection.chat.isWriting(); + collabInstance.connection.chat.isWriting();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/chat-webview/chat-webview.ts` around lines 31 - 60, Inside the room callbacks, don't read from the global CollaborationInstance.Current; use the captured collabInstance instead — e.g., in the onMessage handler replace (await CollaborationInstance.Current?.connectedUsers)?.find(...) with collabInstance.connectedUsers (or await collabInstance.getConnectedUsers() if async) to resolve the sending user's name/color, in the onDidUsersChange callback call a version of getOtherUsers that accepts or uses collabInstance (or inline collabInstance.connectedUsers filtering) instead of relying on this.getOtherUsers() reading the global, and in onIsWriting use collabInstance for any instance-specific data; update getOtherUsers (or add an overload) to accept a CollaborationInstance parameter if needed so all callbacks consistently operate on the collabInstance they were registered with.workspaces/oct/open-collaboration-vscode/src/collaboration-instance.ts (1)
880-900: 💤 Low valueLGTM with one nit on the retry contract.
The retry loop only retries when
vscode.workspace.applyEditresolvesfalse; any thrown error short-circuits toreturn false. That likely matches the intent (don't retry hard failures), but the docstring "retry up to 20 times" reads as if it covers exceptions too. Consider clarifying the comment to match behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/oct/open-collaboration-vscode/src/collaboration-instance.ts` around lines 880 - 900, The docstring for private async applyEdit(changes, edit) is misleading about retries: currently the loop retries up to 20 times only when vscode.workspace.applyEdit(workspaceEdit) resolves to false, while any thrown exception immediately returns false and stops retries; update the comment to explicitly state “retries occur only when applyEdit returns false; thrown errors abort and are not retried” (or alternatively change the catch to only handle transient errors if you want retries on certain exceptions) and reference applyEdit and vscode.workspace.applyEdit in the comment so intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5864f81-4e74-45f5-a319-040b59e04e46
⛔ Files ignored due to path filters (3)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlworkspaces/oct/open-collaboration-vscode/data/oct-logo.pngis excluded by!**/*.pngworkspaces/oct/open-collaboration-vscode/data/oct-view-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (103)
.vscode/launch.jsonrush.jsonworkspaces/ballerina/ballerina-extension/package.jsonworkspaces/oct/open-collaboration-protocol/LICENSEworkspaces/oct/open-collaboration-protocol/README.mdworkspaces/oct/open-collaboration-protocol/package.jsonworkspaces/oct/open-collaboration-protocol/src/configuration.tsworkspaces/oct/open-collaboration-protocol/src/connection-provider.tsworkspaces/oct/open-collaboration-protocol/src/connection.tsworkspaces/oct/open-collaboration-protocol/src/index.tsworkspaces/oct/open-collaboration-protocol/src/messages.tsworkspaces/oct/open-collaboration-protocol/src/messaging/abstract-connection.tsworkspaces/oct/open-collaboration-protocol/src/messaging/compression.tsworkspaces/oct/open-collaboration-protocol/src/messaging/encoding.tsworkspaces/oct/open-collaboration-protocol/src/messaging/encryption.tsworkspaces/oct/open-collaboration-protocol/src/messaging/messages.tsworkspaces/oct/open-collaboration-protocol/src/transport/socket-io-transport.tsworkspaces/oct/open-collaboration-protocol/src/transport/transport.tsworkspaces/oct/open-collaboration-protocol/src/transport/websocket-transport.tsworkspaces/oct/open-collaboration-protocol/src/types.tsworkspaces/oct/open-collaboration-protocol/src/utils/base64.tsworkspaces/oct/open-collaboration-protocol/src/utils/crypto.tsworkspaces/oct/open-collaboration-protocol/src/utils/disposable.tsworkspaces/oct/open-collaboration-protocol/src/utils/errors.tsworkspaces/oct/open-collaboration-protocol/src/utils/event.tsworkspaces/oct/open-collaboration-protocol/src/utils/info.tsworkspaces/oct/open-collaboration-protocol/src/utils/promise.tsworkspaces/oct/open-collaboration-protocol/src/utils/types.tsworkspaces/oct/open-collaboration-protocol/src/utils/version.tsworkspaces/oct/open-collaboration-protocol/tsconfig.jsonworkspaces/oct/open-collaboration-vscode/.gitignoreworkspaces/oct/open-collaboration-vscode/.vscodeignoreworkspaces/oct/open-collaboration-vscode/CHANGELOG.mdworkspaces/oct/open-collaboration-vscode/LICENSEworkspaces/oct/open-collaboration-vscode/README.mdworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.cs.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.de.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.es.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.fr.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.hu.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.it.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.ja.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.ko.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.pl.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.pt-br.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.ru.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.tr.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.zh-cn.jsonworkspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.zh-tw.jsonworkspaces/oct/open-collaboration-vscode/package.jsonworkspaces/oct/open-collaboration-vscode/package.nls.cs.jsonworkspaces/oct/open-collaboration-vscode/package.nls.de.jsonworkspaces/oct/open-collaboration-vscode/package.nls.es.jsonworkspaces/oct/open-collaboration-vscode/package.nls.fr.jsonworkspaces/oct/open-collaboration-vscode/package.nls.hu.jsonworkspaces/oct/open-collaboration-vscode/package.nls.it.jsonworkspaces/oct/open-collaboration-vscode/package.nls.ja.jsonworkspaces/oct/open-collaboration-vscode/package.nls.jsonworkspaces/oct/open-collaboration-vscode/package.nls.ko.jsonworkspaces/oct/open-collaboration-vscode/package.nls.pl.jsonworkspaces/oct/open-collaboration-vscode/package.nls.pt-br.jsonworkspaces/oct/open-collaboration-vscode/package.nls.ru.jsonworkspaces/oct/open-collaboration-vscode/package.nls.tr.jsonworkspaces/oct/open-collaboration-vscode/package.nls.zh-cn.jsonworkspaces/oct/open-collaboration-vscode/package.nls.zh-tw.jsonworkspaces/oct/open-collaboration-vscode/scripts/esbuild.tsworkspaces/oct/open-collaboration-vscode/scripts/translate.tsworkspaces/oct/open-collaboration-vscode/src/chat-webview/chat-webview.tsworkspaces/oct/open-collaboration-vscode/src/chat-webview/messages.tsworkspaces/oct/open-collaboration-vscode/src/chat-webview/src/message-input.tsxworkspaces/oct/open-collaboration-vscode/src/chat-webview/src/styles.cssworkspaces/oct/open-collaboration-vscode/src/chat-webview/src/utils.tsworkspaces/oct/open-collaboration-vscode/src/chat-webview/src/webview.tsxworkspaces/oct/open-collaboration-vscode/src/collaboration-connection-provider.tsworkspaces/oct/open-collaboration-vscode/src/collaboration-file-system.tsworkspaces/oct/open-collaboration-vscode/src/collaboration-instance.tsworkspaces/oct/open-collaboration-vscode/src/collaboration-room-service.tsworkspaces/oct/open-collaboration-vscode/src/collaboration-status-service.tsworkspaces/oct/open-collaboration-vscode/src/collaboration-status-view.tsworkspaces/oct/open-collaboration-vscode/src/commands-list.tsworkspaces/oct/open-collaboration-vscode/src/commands.tsworkspaces/oct/open-collaboration-vscode/src/context-key-service.tsworkspaces/oct/open-collaboration-vscode/src/extension-web.tsworkspaces/oct/open-collaboration-vscode/src/extension.tsworkspaces/oct/open-collaboration-vscode/src/follow-service.tsworkspaces/oct/open-collaboration-vscode/src/inversify.tsworkspaces/oct/open-collaboration-vscode/src/secret-storage.tsworkspaces/oct/open-collaboration-vscode/src/utils/l10n.tsworkspaces/oct/open-collaboration-vscode/src/utils/package.tsworkspaces/oct/open-collaboration-vscode/src/utils/quick-pick.tsworkspaces/oct/open-collaboration-vscode/src/utils/settings.tsworkspaces/oct/open-collaboration-vscode/src/utils/system.tsworkspaces/oct/open-collaboration-vscode/src/utils/uri.tsworkspaces/oct/open-collaboration-vscode/src/utils/workspace.tsworkspaces/oct/open-collaboration-vscode/tsconfig.jsonworkspaces/oct/open-collaboration-yjs/LICENSEworkspaces/oct/open-collaboration-yjs/README.mdworkspaces/oct/open-collaboration-yjs/package.jsonworkspaces/oct/open-collaboration-yjs/src/index.tsworkspaces/oct/open-collaboration-yjs/src/yjs-normalized-text.tsworkspaces/oct/open-collaboration-yjs/src/yjs-provider.tsworkspaces/oct/open-collaboration-yjs/tsconfig.json
✅ Files skipped from review due to trivial changes (43)
- workspaces/oct/open-collaboration-yjs/LICENSE
- workspaces/oct/open-collaboration-vscode/package.nls.pl.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.json
- workspaces/oct/open-collaboration-vscode/CHANGELOG.md
- workspaces/oct/open-collaboration-vscode/LICENSE
- workspaces/oct/open-collaboration-vscode/package.nls.de.json
- workspaces/oct/open-collaboration-vscode/package.nls.zh-tw.json
- workspaces/oct/open-collaboration-yjs/tsconfig.json
- workspaces/oct/open-collaboration-vscode/package.nls.fr.json
- workspaces/oct/open-collaboration-vscode/.vscodeignore
- workspaces/oct/open-collaboration-vscode/package.nls.ru.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.es.json
- workspaces/oct/open-collaboration-vscode/package.nls.pt-br.json
- workspaces/oct/open-collaboration-vscode/package.nls.tr.json
- workspaces/oct/open-collaboration-vscode/package.nls.ko.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.ja.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.pl.json
- workspaces/oct/open-collaboration-vscode/package.nls.es.json
- workspaces/oct/open-collaboration-protocol/LICENSE
- workspaces/oct/open-collaboration-vscode/package.nls.it.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.hu.json
- workspaces/oct/open-collaboration-protocol/src/configuration.ts
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.cs.json
- workspaces/oct/open-collaboration-protocol/src/index.ts
- workspaces/oct/open-collaboration-vscode/package.nls.zh-cn.json
- workspaces/oct/open-collaboration-yjs/src/index.ts
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.zh-tw.json
- workspaces/oct/open-collaboration-vscode/package.nls.json
- workspaces/oct/open-collaboration-vscode/src/utils/package.ts
- workspaces/oct/open-collaboration-vscode/.gitignore
- workspaces/oct/open-collaboration-protocol/src/messaging/encoding.ts
- workspaces/oct/open-collaboration-vscode/package.json
- workspaces/oct/open-collaboration-vscode/package.nls.hu.json
- workspaces/oct/open-collaboration-yjs/package.json
- workspaces/oct/open-collaboration-protocol/src/transport/transport.ts
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.it.json
- workspaces/oct/open-collaboration-vscode/src/commands-list.ts
- workspaces/oct/open-collaboration-vscode/package.nls.ja.json
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.ru.json
- workspaces/oct/open-collaboration-protocol/src/utils/errors.ts
- workspaces/oct/open-collaboration-vscode/l10n/bundle.l10n.de.json
- workspaces/oct/open-collaboration-vscode/package.nls.cs.json
- workspaces/oct/open-collaboration-vscode/src/chat-webview/messages.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .vscode/launch.json
- workspaces/ballerina/ballerina-extension/package.json
Comment on lines
+120
to
+127
| onJoinRequest: handler => this.onRequest(Messages.Peer.Join, handler), | ||
| onInfo: handler => this.onNotification(Messages.Peer.Info, handler), | ||
| onInit: handler => this.onNotification(Messages.Peer.Init, async (origin, response) => { | ||
| this.onDidInit(response); | ||
| handler(origin, response); | ||
| }), | ||
| init: (target, request) => this.sendNotification(Messages.Peer.Init, target, request) | ||
| }; |
Contributor
There was a problem hiding this comment.
Make pending join requesters addressable before sending the response.
Responses for non-server requests are encrypted with getPublicKey(decrypted.origin), but this implementation only knows keys for peers added through onDidJoinRoom/onDidInit. A Messages.Peer.Join requester has not reached either path yet, so accepting the join will throw No public key found for origin ... before the handshake completes. You need a separate source of truth for pending peers' public keys, or a way to seed that map before replying.
Also applies to: 213-223, 226-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workspaces/oct/open-collaboration-protocol/src/connection.ts` around lines
120 - 127, The join-request flow fails because the response encryption uses
getPublicKey(decrypted.origin) but the origin's key isn't yet in the known-peer
map (onDidJoinRoom/onDidInit); update the Messages.Peer.Join handling
(onJoinRequest / the handler passed to this.onRequest for Messages.Peer.Join) so
that it first extracts and stores the requester's public key into a dedicated
pending-peer map (e.g. pendingPublicKeys[origin] or via a new
addPendingPeerPublicKey(origin, key)) before calling the accept/response path
that invokes sendNotification/getPublicKey; similarly seed that pending map in
the corresponding accept/handshake code paths referenced in the comment (regions
around lines 213-223 and 226-243) and make getPublicKey consult
pendingPublicKeys as a fallback so responses to join requests can be encrypted
prior to onDidInit/onDidJoinRoom being called.
Comment on lines
+65
to
+67
| read(cb: (data: Uint8Array) => void): void { | ||
| this.socket.onmessage = event => cb(event.data); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how MessageTransport.read() callback data is consumed upstream
rg -n --type ts -C 4 '\.read\('Repository: wso2/vscode-extensions
Length of output: 5316
🏁 Script executed:
#!/bin/bash
# Find MessageTransport interface definition
rg -n --type ts "interface MessageTransport" -A 10Repository: wso2/vscode-extensions
Length of output: 1475
🏁 Script executed:
#!/bin/bash
# Check for other transport implementations
fd -e ts -path "*/transport/*" | head -20Repository: wso2/vscode-extensions
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Look for the read method signature in the interface
rg -n --type ts "read\s*\(" workspaces/oct/open-collaboration-protocol/src/transport/ -B 2 -A 2Repository: wso2/vscode-extensions
Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Find ConnectionReader type definition
rg -n --type ts "type ConnectionReader\|interface ConnectionReader" -A 3Repository: wso2/vscode-extensions
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Also check imports in the interface file
cat workspaces/oct/open-collaboration-protocol/src/transport/transport.tsRepository: wso2/vscode-extensions
Length of output: 980
🏁 Script executed:
#!/bin/bash
# Check socket-io-transport implementation and its socket type
cat -n workspaces/oct/open-collaboration-protocol/src/transport/socket-io-transport.ts | head -100Repository: wso2/vscode-extensions
Length of output: 4079
event.data is ArrayBuffer, not Uint8Array — transport violates the interface contract.
The MessageTransport interface defines read as ConnectionReader = (cb: (data: Uint8Array) => void) => void. However, with binaryType = 'arraybuffer', the WebSocket API delivers binary frames as ArrayBuffer objects, not Uint8Array. This breaks the contract and forces callers to wrap the data defensively (as seen in abstract-connection.ts line 89). Any code relying on Uint8Array APIs will silently fail at runtime.
🐛 Proposed fix — wrap in a Uint8Array view
read(cb: (data: Uint8Array) => void): void {
- this.socket.onmessage = event => cb(event.data);
+ this.socket.onmessage = event => cb(new Uint8Array(event.data as ArrayBuffer));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| read(cb: (data: Uint8Array) => void): void { | |
| this.socket.onmessage = event => cb(event.data); | |
| } | |
| read(cb: (data: Uint8Array) => void): void { | |
| this.socket.onmessage = event => cb(new Uint8Array(event.data as ArrayBuffer)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@workspaces/oct/open-collaboration-protocol/src/transport/websocket-transport.ts`
around lines 65 - 67, The read implementation assigns socket.onmessage and
passes event.data directly as Uint8Array but WebSocket binaryType='arraybuffer'
yields an ArrayBuffer; update read (the read method on this class using
this.socket.onmessage) to convert event.data to a Uint8Array view before
invoking cb (e.g., wrap ArrayBuffer with new Uint8Array(...); also handle the
case where event.data might already be a Uint8Array or Blob by detecting types)
so the delivered value always matches the MessageTransport/ConnectionReader
signature.
Comment on lines
+60
to
+86
| async generateSymKey() { | ||
| const key = await subtle.generateKey({ | ||
| name: 'AES-CBC', | ||
| length: 256 | ||
| }, true, ['encrypt', 'decrypt']); | ||
| const exportedKey = await subtle.exportKey('raw', key); | ||
| return toBase64(new Uint8Array(exportedKey)); | ||
| }, | ||
| async generateIV() { | ||
| const iv = (crypto.getRandomValues<Uint8Array>)(new Uint8Array(16)); | ||
| return toBase64(iv); | ||
| }, | ||
| async symEncrypt(data: Uint8Array, key: string, iv: string) { | ||
| const cryptoKey = await subtle.importKey('raw', fromBase64(key), 'AES-CBC', false, ['encrypt']); | ||
| const arrayBuffer = await subtle.encrypt({ | ||
| name: 'AES-CBC', | ||
| iv: fromBase64(iv), | ||
| }, cryptoKey, data); | ||
| return new Uint8Array(arrayBuffer); | ||
| }, | ||
| async symDecrypt(data: Uint8Array, key: string, iv: string) { | ||
| const cryptoKey = await subtle.importKey('raw', fromBase64(key), 'AES-CBC', false, ['decrypt']); | ||
| const arrayBuffer = await subtle.decrypt({ | ||
| name: 'AES-CBC', | ||
| iv: fromBase64(iv), | ||
| }, cryptoKey, data); | ||
| return new Uint8Array(arrayBuffer); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the complete crypto.ts file
cat -n workspaces/oct/open-collaboration-protocol/src/utils/crypto.tsRepository: wso2/vscode-extensions
Length of output: 5335
🏁 Script executed:
# Search for usages of symEncrypt and symDecrypt
rg -n "symEncrypt|symDecrypt" --type ts --type tsx workspaces/oct/open-collaboration-protocol/Repository: wso2/vscode-extensions
Length of output: 94
🏁 Script executed:
# Search for usages of symEncrypt and symDecrypt without specifying tsx
rg -n "symEncrypt|symDecrypt" --type ts workspaces/oct/open-collaboration-protocol/Repository: wso2/vscode-extensions
Length of output: 1573
🏁 Script executed:
cat -n workspaces/oct/open-collaboration-protocol/src/messaging/encryption.tsRepository: wso2/vscode-extensions
Length of output: 8423
Replace AES-CBC with an authenticated encryption mode.
The protocol encrypts message bodies with AES-CBC but never authenticates the ciphertext. Message metadata only contains the encrypted symmetric key and IV—no authentication tag. This leaves encrypted traffic malleable: an attacker can alter bytes in the ciphertext and the receiver has no integrity check to detect tampering. Replace with an AEAD mode such as AES-GCM and carry the authentication tag in the message metadata alongside the IV.
Additionally, generateIV() (line 69) uses the global crypto.getRandomValues instead of the injected cryptoModule, which breaks the abstraction and may fail in environments where only the configured module is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workspaces/oct/open-collaboration-protocol/src/utils/crypto.ts` around lines
60 - 86, The code uses AES-CBC without authentication and calls global
crypto.getRandomValues; update generateSymKey to create an 'AES-GCM' 256-bit key
(usages ['encrypt','decrypt']), change generateIV to use the injected
cryptoModule.getRandomValues to produce a 12-byte IV for GCM, and modify
symEncrypt and symDecrypt to use 'AES-GCM' with that IV; ensure symEncrypt
returns both the ciphertext and the authentication tag (or a combined buffer and
expose/tag in metadata) so callers can store the auth tag alongside the IV, and
symDecrypt expects and verifies the auth tag during decryption (use
subtle.encrypt/subtle.decrypt with 'AES-GCM' in importKey/calls and handle
WebCrypto's ciphertext+tag format appropriately).
Comment on lines
+549
to
+556
| await this.applyEdit([], () => { | ||
| const doc = this.findDocument(uri); | ||
| if (!doc) { | ||
| return undefined; | ||
| } | ||
| this.createFullDocumentEdit(doc, textContent); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing return — WorkspaceEdit is constructed and silently discarded.
The arrow callback builds the edit at Line 554 but never returns it. applyEdit then sees workspaceEdit === undefined and short-circuits with return true (Lines 891-893), so no edit is ever applied. The subsequent document.save() (Line 558) persists the stale local content instead of the remote write payload.
The parallel call site at Line 862 is correct (return doc ? this.createFullDocumentEdit(...) : undefined;).
🛠️ Suggested fix
if (textContent !== document.getText()) {
await this.applyEdit([], () => {
const doc = this.findDocument(uri);
if (!doc) {
return undefined;
}
- this.createFullDocumentEdit(doc, textContent);
+ return this.createFullDocumentEdit(doc, textContent);
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workspaces/oct/open-collaboration-vscode/src/collaboration-instance.ts`
around lines 549 - 556, The callback passed to applyEdit builds a WorkspaceEdit
but never returns it, so applyEdit receives undefined and skips applying the
edit; change the arrow callback inside the applyEdit call to return the result
of createFullDocumentEdit when a document is found (i.e., return doc ?
this.createFullDocumentEdit(doc, textContent) : undefined), using the existing
findDocument, createFullDocumentEdit, and applyEdit call sites to locate and
update the callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Goals
Approach
Screen.Recording.2026-04-16.at.12.35.10.mov
Summary by CodeRabbit
New Features
Improvements