Fix Error when opening a diagram view on a new tab #870
Conversation
gigara
added
the
Checks/Enable UI Tests
WalkthroughAdds an optional isLoading flag to VisualizerLocation and threads it through RPC, state machine, and visualizer guards; introduces a typed LogLevel and a configurable MI.logging.loggingLevel setting; updates logging callsites to use the enum; adjusts state-machine flows and popup timing; minor test and default settings tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as VS Code Extension
participant SM as StateMachine
participant RPC as RPCLayer
participant UI as Visualizer
Ext->>SM: getStateMachine(projectUri, context?)
SM->>SM: merge provided VisualizerLocation (may include isLoading)
SM->>RPC: checkIfMiProject(view?)
RPC-->>SM: detection result + metadata
SM->>SM: set context.isLoading = true
SM->>UI: notify context (isLoading:true)
note right of UI `#f0f7ff`: Visualizer ignores viewReady while isLoading
UI--xSM: skip applying viewReady if initialState.isLoading
SM->>SM: finish detection, set context.isLoading = false
SM->>RPC: update VisualizerLocation
RPC->>UI: send state (isLoading:false / viewReady)
UI->>UI: accept viewReady and render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25-35 minutes Areas to focus:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-05T10:31:47.583ZApplied to files:
🔇 Additional comments (2)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspaces/mi/mi-extension/src/stateMachine.ts (2)
809-824:updateProjectExplorermay inadvertently create state machines with incorrect context.Line 819 calls
getStateMachine(projectRoot, location), which will create a new state machine if one doesn't exist. SinceupdateProjectExploreris a utility function called from multiple places (lines 617, 664, 728), this could initialize state machines with transient or incorrect context (e.g.,isLoading: true).This may be contributing to the error described in the PR where opening a diagram in a new tab triggers an error.
Apply this fix to only retrieve existing state machines without creating new ones:
} else if (projectRoot && !extension.preserveActivity) { location.projectUri = projectRoot; - if (!getStateMachine(projectRoot, location).context().isOldProject) { + // Only check if state machine already exists + if (stateMachines.has(projectRoot) && !getStateMachine(projectRoot).context().isOldProject) { vscode.commands.executeCommand(COMMANDS.REVEAL_ITEM_COMMAND, location); } }Alternatively, add a parameter-less getter:
export const hasStateMachine = (projectUri: string): boolean => { return stateMachines.has(projectUri); };Then use:
if (hasStateMachine(projectRoot) && !getStateMachine(projectRoot).context().isOldProject) { // ... }
681-714: Context parameter silently ignored for cached state machines; default view changed from Welcome to Overview.The
contextparameter is only applied when creating a new state machine. For existing machines retrieved from the cache, the parameter is silently ignored. This creates a subtle bug at line 819 wheregetStateMachine(projectRoot, location)passes context expecting initialization, but if the machine already exists for thatprojectUri, thelocationparameter is discarded without warning.Additionally, the default view changed from
MACHINE_VIEW.Welcome(line 50) toMACHINE_VIEW.Overview(line 698), which may affect initial user experience, especially for new projects.To address this:
- Document the caching behavior if intentional, clarifying that context only applies to new machines.
- Warn when context is ignored for existing machines to catch caller mistakes.
- Re-evaluate whether machines should be reused or if fresh initialization is needed in certain flows (e.g., at line 819).
🧹 Nitpick comments (2)
workspaces/mi/mi-extension/src/stateMachine.ts (2)
24-24: Inconsistent logging approach throughout the file.State entry points use
logDebug(which respects configuration), while runtime detection and operation messages useconsole.log(which bypasses configuration). This means users cannot control the verbosity of detection logs even when logging is set to OFF.Consider replacing all
console.logstatements withlogDebugfor consistency:- console.log("Waiting for LS to be ready " + new Date().toLocaleTimeString()); + logDebug("Waiting for LS to be ready " + new Date().toLocaleTimeString());- console.log("LS is ready " + new Date().toLocaleTimeString()); + logDebug("LS is ready " + new Date().toLocaleTimeString());- console.log("Error occured while waiting for LS to be ready " + new Date().toLocaleTimeString()); + logDebug("Error occured while waiting for LS to be ready " + new Date().toLocaleTimeString());- console.log(`Detecting project in ${projectUri} - ${new Date().toLocaleTimeString()}`); + logDebug(`Detecting project in ${projectUri} - ${new Date().toLocaleTimeString()}`);Apply similar changes to lines 838, 900-901, 904, 925, 930, and 936.
Also applies to: 392-392, 404-404, 406-406, 827-827, 838-838, 900-901, 904-904, 925-925, 930-930, 936-936
58-58: Simplify redundant nullish coalescing.The expression
context.view ?? undefinedis redundant since the default parameter value is alreadyMACHINE_VIEW.Overview(line 826).- src: (context) => checkIfMiProject(context.projectUri!, context.view ?? undefined), + src: (context) => checkIfMiProject(context.projectUri!, context.view),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
workspaces/mi/mi-core/src/state-machine-types.ts(1 hunks)workspaces/mi/mi-extension/package.json(1 hunks)workspaces/mi/mi-extension/src/RPCLayer.ts(1 hunks)workspaces/mi/mi-extension/src/debugger/debugHelper.ts(16 hunks)workspaces/mi/mi-extension/src/debugger/debugger.ts(16 hunks)workspaces/mi/mi-extension/src/debugger/tasks.ts(3 hunks)workspaces/mi/mi-extension/src/stateMachine.ts(25 hunks)workspaces/mi/mi-extension/src/util/logger.ts(2 hunks)workspaces/mi/mi-visualizer/src/Visualizer.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:31:47.583Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 830
File: workspaces/ballerina/ballerina-extension/test/ai/post_proccess/post.test.ts:0-0
Timestamp: 2025-11-05T10:31:47.583Z
Learning: In the workspaces/ballerina/ballerina-extension project, the tsconfig.json uses "rootDirs": ["src", "test"], which allows test files to import from src using shorter relative paths. For example, from test/ai/post_proccess/, the import '../../stateMachine' correctly resolves to src/stateMachine.ts due to this configuration.
Applied to files:
workspaces/mi/mi-extension/src/stateMachine.ts
🧬 Code graph analysis (4)
workspaces/mi/mi-extension/src/debugger/debugger.ts (1)
workspaces/mi/mi-extension/src/util/logger.ts (1)
logDebug(49-61)
workspaces/mi/mi-extension/src/debugger/tasks.ts (1)
workspaces/mi/mi-extension/src/util/logger.ts (1)
logDebug(49-61)
workspaces/mi/mi-extension/src/stateMachine.ts (3)
workspaces/mi/mi-extension/src/util/logger.ts (1)
logDebug(49-61)workspaces/mi/mi-core/src/state-machine-types.ts (1)
VisualizerLocation(252-273)workspaces/mi/mi-extension/src/util/migrationUtils.ts (2)
containsMultiModuleNatureInPomFile(1685-1690)findMultiModuleProjectsInWorkspaceDir(1704-1734)
workspaces/mi/mi-extension/src/debugger/debugHelper.ts (1)
workspaces/mi/mi-extension/src/util/logger.ts (1)
logDebug(49-61)
🔇 Additional comments (13)
workspaces/mi/mi-extension/src/util/logger.ts (1)
23-28: LGTM! Clean enum definition.The LogLevel enum provides a type-safe way to specify logging levels across the extension.
workspaces/mi/mi-extension/src/debugger/tasks.ts (1)
23-23: LGTM! Correct migration to the new logging API.The import and usage of
LogLevel.ERRORare appropriate for the error scenarios in this file.Also applies to: 154-154, 180-180
workspaces/mi/mi-core/src/state-machine-types.ts (1)
272-272: LGTM! Appropriate type extension.Adding the optional
isLoadingproperty enables tracking of loading state in the visualizer location context, which supports the fix for the diagram view error.workspaces/mi/mi-extension/src/RPCLayer.ts (1)
102-102: LGTM! Correct propagation of loading state.The
isLoadingfield is properly passed through from the state machine context to the RPC client, enabling the frontend to access the loading state.workspaces/mi/mi-extension/src/debugger/debugHelper.ts (2)
32-32: LGTM! Correct import update.The import correctly brings in the new
LogLevelenum from the updated logger module.
107-107: LGTM! Consistent migration to the new logging API.All
logDebugcalls correctly useLogLevel.INFOfor success/informational messages andLogLevel.ERRORfor error scenarios. The log level choices are appropriate for each context.Also applies to: 118-118, 129-129, 227-227, 236-236, 429-429, 447-447, 460-460, 522-522, 537-537, 552-552, 584-584, 593-593, 625-625, 659-659
workspaces/mi/mi-visualizer/src/Visualizer.tsx (1)
96-96: LGTM! Correct loading guard implementation.The added condition
&& !initialState.isLoadingprevents visualizer state updates during active loading operations. This addresses the error when opening diagrams in new tabs by ensuring the state machine doesn't transition until loading completes.The logic correctly handles the optional
isLoadingproperty:
- When
isLoadingisundefined(not set),!initialState.isLoadingevaluates totrue, allowing the update- When
isLoadingistrue, the update is blocked- When
isLoadingisfalse, the update is allowedworkspaces/mi/mi-extension/src/debugger/debugger.ts (2)
29-29: LGTM! Correct import update.The import correctly brings in the new
LogLevelenum from the updated logger module.
86-86: LGTM! Consistent migration to the new logging API.All
logDebugcalls correctly use:
LogLevel.ERRORfor error conditions (workspace not found, breakpoint issues, connection errors, etc.)LogLevel.INFOfor informational messages (events received, commands sent, responses)The log level assignments are appropriate for each context.
Also applies to: 136-136, 168-168, 181-181, 190-190, 228-228, 260-260, 277-277, 394-394, 399-399, 404-404, 422-422, 433-433, 453-453, 594-594, 612-612, 647-647
workspaces/mi/mi-extension/src/stateMachine.ts (4)
268-286: Consider adding guards forisLoadingstate.The
isLoadingflag is set totrueon OPEN_VIEW (line 284) and reset tofalsein various completion states (lines 181, 246, 261). However, there are no guards preventing new OPEN_VIEW, REPLACE_VIEW, or NAVIGATE events from being processed whileisLoadingis true.If the intent is to prevent concurrent view transitions, add a guard condition:
OPEN_VIEW: { target: "viewLoading", + cond: (context) => !context.isLoading, actions: assign({Alternatively, if XState's built-in event queuing is sufficient, document this behavior to clarify the design intent.
235-249: LGTM: Loading state properly managed in view transitions.The additions of
dataMapperPropsandisLoading: falseassignments ensure state is properly propagated and loading flags are reset when view finding completes. The target change toviewReady(line 240) correctly reflects the state machine structure.
283-283: LGTM: Previous context tracking enables proper history management.Storing
previousContext(line 283) allows the history service (line 627) to maintain accurate navigation state when opening new views. This is essential for the back/forward navigation feature.
826-915: LGTM: View parameter and scope-based view selection implemented correctly.The addition of the
viewparameter with defaultMACHINE_VIEW.Overview(line 826) aligns with the state machine initialization changes. The scope-based view selection logic (lines 858-873) appropriately determines which view to display for empty projects based on their configured scope.The function correctly returns all necessary properties including
isEnvironmentSetUpfor proper state machine transitions.Note: Console logging consistency is addressed in a separate comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
workspaces/mi/mi-extension/src/util/logger.ts (1)
49-61: Type safety issue: unsafe cast when configuredLevel is undefined.The cast
configuredLevel as LogLevelon line 58 is unsafe. Whenconfig.get()returnsundefined(e.g., if the setting hasn't been explicitly configured yet), the cast is invalid andindexOfwill return-1, breaking the comparison logic.Apply this diff to fix the type safety issue:
export function logDebug(message: string, logLevel: LogLevel = LogLevel.DEBUG): void { const config = vscode.workspace.getConfiguration('MI'); const configuredLevel = config.get<string>('logging.loggingLevel'); if (configuredLevel === 'off') { return; } // Only log if the message's level is >= configured level const levels = [LogLevel.ERROR, LogLevel.WARN, LogLevel.INFO, LogLevel.DEBUG]; - if (levels.indexOf(logLevel) <= levels.indexOf(configuredLevel as LogLevel)) { + const effectiveLevel = (configuredLevel as LogLevel) || LogLevel.ERROR; + if (levels.indexOf(logLevel) <= levels.indexOf(effectiveLevel)) { logWithDebugLevel(message, 'MI Debug', logLevel); } }
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/package.json (1)
166-178: Configuration looks good; minor typo in description.The logging configuration properly includes all log levels including "info", which addresses the previous review concern. The enum values align correctly with the
LogLevelenum inlogger.ts.Minor nitpick: Line 176, "Order" could be lowercase "order" for consistency with typical descriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/mi/mi-extension/package.json(1 hunks)workspaces/mi/mi-extension/src/stateMachinePopup.ts(8 hunks)workspaces/mi/mi-extension/src/test/e2e-playwright-tests/artifactTests/artifact.spec.ts(4 hunks)workspaces/mi/mi-extension/src/util/logger.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:31:47.583Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 830
File: workspaces/ballerina/ballerina-extension/test/ai/post_proccess/post.test.ts:0-0
Timestamp: 2025-11-05T10:31:47.583Z
Learning: In the workspaces/ballerina/ballerina-extension project, the tsconfig.json uses "rootDirs": ["src", "test"], which allows test files to import from src using shorter relative paths. For example, from test/ai/post_proccess/, the import '../../stateMachine' correctly resolves to src/stateMachine.ts due to this configuration.
Applied to files:
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/artifactTests/artifact.spec.ts
🔇 Additional comments (3)
workspaces/mi/mi-extension/src/stateMachinePopup.ts (2)
26-26: LGTM! Debug logging improves observability.The addition of debug logging at state entry points provides valuable observability for tracking state machine transitions, which will help diagnose issues like the one this PR addresses.
Also applies to: 45-45, 63-63, 80-80, 101-101, 113-113, 124-124
224-236: Fix correctly defers OPEN_VIEW and safely handles error states.The state machine confirms that
initializehas two guaranteed transitions:onDone→readyoronError→disabled. Your implementation properly waits for the ready state and sends the event only then.The edge case mentioned (listener remaining attached if initialization fails) is very minor: the listener won't execute when the state transitions to
disabledsince the condition checks for"ready", so the OPEN_VIEW event won't be sent to a disabled state machine. The listener attachment is inert in error scenarios but represents a small memory overhead if initialization repeatedly fails. For better resource hygiene, consider cleaning up the listener ondisabledstate transitions as well, but this is not critical since the current behavior is safe.workspaces/mi/mi-extension/src/util/logger.ts (1)
23-28: LGTM! Well-defined LogLevel enum.The enum provides type-safe log levels that align with the configuration schema.
workspaces/mi/mi-extension/src/test/e2e-playwright-tests/artifactTests/artifact.spec.ts
Show resolved
Hide resolved
gigara
added
Checks/Run MI UI Tests
2 participants
Purpose
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Improvements
Tests