add real-time collaborative editing support for devant editor#1975
add real-time collaborative editing support for devant editor#1975vinukab wants to merge 44 commits intowso2:feature/collaborative-editorfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds collaborative BI-diagram features and remote workspace support: node locking, cursor/presence synchronization via OCT/Yjs, a CollaborationLockManager, remote URI caching, new RPC types/handlers, editor/webview sync changes, and widespread UI updates to render locks/cursors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as VSCode User
participant Ext as Ballerina Extension
participant LockMgr as CollaborationLockManager
participant Yjs as Yjs Shared State
participant WebView as BI Diagram WebView
participant OCT as OCT Integration
User->>WebView: Click node to edit
WebView->>Ext: request acquireNodeLock(filePath,nodeId,user)
Ext->>LockMgr: acquireLock(...)
LockMgr->>Yjs: read/update locks map
alt lock granted
Yjs-->>LockMgr: lock stored
LockMgr-->>Ext: success
Ext->>WebView: nodeLockUpdated notification
WebView->>WebView: enable local editing
else denied
LockMgr-->>Ext: error
Ext->>WebView: show lock conflict
end
WebView->>Ext: updateDiagramCursor(x,y,nodeId)
Ext->>OCT: broadcast cursor (awareness)
OCT-->>Ext: awareness state change
Ext->>LockMgr: updateCursor(...)
LockMgr-->>Ext: cursor state updated
Ext->>WebView: diagramCursorUpdated notification
WebView->>WebView: render remote cursors
sequenceDiagram
participant RemoteFS as Remote Workspace (oct://)
participant Watcher as Extension FileSystemWatcher
participant Cache as UriCache (local temp)
participant LSP as Language Server
participant Ext as Ballerina Extension
RemoteFS->>Watcher: file changed
Watcher->>Cache: cacheRemoteFile(remoteUri)
Cache->>Cache: fetch and store local copy
Cache-->>Watcher: return local cached path
Watcher->>LSP: didChange(cachedLocalUri, version++)
LSP-->>Ext: analysis results
Ext->>Ext: refresh artifacts / notify webview
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
| "args": [ | ||
| "--extensionDevelopmentPath=${workspaceFolder}/workspaces/ballerina/ballerina-extension", | ||
| "--extensionDevelopmentPath=${workspaceFolder}/workspaces/bi/bi-extension", | ||
| "--extensionDevelopmentPath=/Users/vinuka/projects/open-collaboration-tools/packages/open-collaboration-vscode" |
There was a problem hiding this comment.
Replace hard-coded local OCT paths with portable variables.
Line 31 and Line 44 commit a machine-specific /Users/vinuka/... path. This is not portable for the team and exposes a user identifier in repo config.
🔧 Proposed fix
"args": [
"--extensionDevelopmentPath=${workspaceFolder}/workspaces/ballerina/ballerina-extension",
"--extensionDevelopmentPath=${workspaceFolder}/workspaces/bi/bi-extension",
- "--extensionDevelopmentPath=/Users/vinuka/projects/open-collaboration-tools/packages/open-collaboration-vscode"
+ "--extensionDevelopmentPath=${env.OCT_VSCODE_PATH}"
],
@@
"outFiles": [
"${workspaceFolder}/workspaces/ballerina/ballerina-extension/dist/**/*.js",
"${workspaceFolder}/workspaces/bi/bi-extension/out/**/*.js",
- "/Users/vinuka/projects/open-collaboration-tools/packages/open-collaboration-vscode/dist/**/*.js"
+ "${env.OCT_VSCODE_PATH}/dist/**/*.js"
],Set OCT_VSCODE_PATH locally (and document it in onboarding notes).
Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vscode/launch.json at line 31, The launch configuration currently embeds a
machine-specific "--extensionDevelopmentPath=/Users/vinuka/..." value; replace
both occurrences of the literal "--extensionDevelopmentPath=..." argument with a
portable environment-variable reference that reads the path from OCT_VSCODE_PATH
(e.g. use the VS Code variable form that expands env vars like
${env:OCT_VSCODE_PATH}); update the README/onboarding notes to instruct
developers to set OCT_VSCODE_PATH locally and ensure both instances (the two
"--extensionDevelopmentPath=..." entries) are changed consistently.
| export interface CollaborationTextSelection { | ||
| filePath: string; | ||
| selectedNodes?: string[]; | ||
| cursor?: WebviewCursorPosition; | ||
| } |
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.
| 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; | ||
| } |
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.
| // Update model with lock information when locks change | ||
| useEffect(() => { | ||
| if (model) { | ||
| const updatedModel = updateNodeLocks(model, nodeLocks); | ||
| setModel(updatedModel); | ||
| } | ||
| }, [nodeLocks]); |
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.
| const parentId = "id" in node ? node.id : "branch"; | ||
| const anchorKey = `position_${parentId}_${target.line}_${target.offset}`; | ||
| const anchorPosition = getDiagramPositionFromElementCenter(event.currentTarget); |
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).
| currentUserId, | ||
| } = useDiagramContext(); | ||
| const isLocked = Boolean(model.node.locked && model.node.locked.userId !== currentUserId); | ||
|
|
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.
fix vulnarability issues with the libraries
Update create project command for consolidated project support
|
Vinuka Buddhima seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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
| 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()), | ||
| }); | ||
| }); | ||
| } | ||
| } |
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.
| 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); | ||
| } | ||
| } | ||
| } |
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.
| 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); | ||
| } |
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.
| 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); | ||
| }; |
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.
4 participants
Purpose
Goals
Approach
Screen.Recording.2026-04-16.at.12.35.10.mov
Summary by CodeRabbit
New Features
Improvements