Add animation path generation and controls#60
Conversation
Summary of ChangesHello @sidhant-sriv, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature that brings dynamic animation capabilities to flowcharts. It enables the automatic generation of potential execution paths through a given code's control flow, and then provides an interactive visual representation of these paths directly within the flowchart viewer. This enhancement aims to improve the understanding of complex code logic by allowing users to 'step through' the flowchart and observe the flow of execution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds animation capabilities to the Visor flowchart extension, allowing users to visualize execution paths through code flowcharts with step-by-step animations. The changes introduce path generation logic, animation controls, and UI elements for managing multiple execution paths.
Key changes:
- Implements an
AnimationPathGeneratorto automatically discover and generate execution paths through flowcharts - Adds animation control buttons and UI for starting/stopping animations and switching between paths
- Extends the webview messaging system to handle animation commands
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/view/BaseFlowchartProvider.ts | Adds animation message types, handlers, UI controls, and JavaScript animation logic in the webview |
| src/logic/utils/AnimationPathGenerator.ts | New file implementing path generation algorithm with support for branches, loops, and exit detection |
| src/logic/EnhancedMermaidGenerator.ts | Integrates path generator and embeds animation metadata as comments in Mermaid output |
| src/ir/ir.ts | Adds AnimationPath interface and optional animationPaths field to FlowchartIR |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| [...pathSteps], | ||
| depth + 1 | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Loop Handling Fails to Explore Multiple Exit Paths
The loop handling in generatePathsFromNode has two issues: the number of simulated iterations is incorrectly tied to the count of outgoing edges (Math.min(outgoingEdges.length, this.MAX_LOOP_ITERATIONS)), leading to inconsistent loop counts. Also, it always follows the first outgoing edge (outgoingEdges[0]) for all iterations, preventing exploration of other potential loop exit paths.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major new feature: animated execution paths for flowcharts. It adds the backend logic to generate these paths from the flowchart's intermediate representation and integrates the animation controls and rendering into the webview. My review focuses on the correctness and robustness of the new path generation algorithm, especially concerning loops and fallback scenarios. I've also identified several areas for improvement in the webview implementation, including theme-aware styling, performance, and UI clarity. There are a few critical issues in the path generation that need to be addressed to ensure animations work correctly for all flowcharts.
| // Prevent cycles | ||
| if (visited.has(nodeId)) { | ||
| console.log(`AnimationPathGenerator: Cycle detected at node ${nodeId}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The current cycle detection logic prevents the correct traversal of loops. The visited.has(nodeId) check will cause the path generation to terminate immediately upon re-entering a node that is part of a loop, meaning loops will not be animated correctly. The logic needs to be adjusted to allow visiting a node multiple times within a path, up to a certain limit, to correctly represent loops. One approach could be to pass a map of node visit counts instead of a Set and check against a maximum visit count.
| const paths = []; | ||
| const pathDescriptions = new Map(); | ||
|
|
||
| const lines = mermaidSource.split('\\n'); |
There was a problem hiding this comment.
The parseAnimationPaths function uses split('\\n') to process the Mermaid source. This will attempt to split the string by a literal backslash followed by the character 'n', which is incorrect. The textContent of the source div will contain actual newline characters (\n), so the split should be done on \n.
| const lines = mermaidSource.split('\\n'); | |
| const lines = mermaidSource.split('\n'); |
| const loopIterations = Math.min(outgoingEdges.length, this.MAX_LOOP_ITERATIONS); | ||
|
|
||
| for (let i = 0; i < loopIterations; i++) { | ||
| const edge = outgoingEdges[0]; // Assume first edge is the loop continuation |
There was a problem hiding this comment.
The assumption that outgoingEdges[0] is always the loop continuation path is risky and may not be true for all control flow structures (e.g., a for loop could have its exit condition check result in a different edge order). This could lead to incorrect animation paths. The logic should be made more robust, for example by analyzing edge properties or labels if available to distinguish between the loop body and loop exit paths.
| private static generateFallbackPath(ir: FlowchartIR): AnimationPath[] { | ||
| // If no paths were generated, create a simple linear path | ||
| const nodes = ir.nodes.map(n => n.id); | ||
| const edges: string[] = []; | ||
|
|
||
| // Create edges between consecutive nodes | ||
| for (let i = 0; i < nodes.length - 1; i++) { | ||
| edges.push(`${nodes[i]}_${nodes[i + 1]}`); | ||
| } | ||
|
|
||
| return [{ | ||
| id: 'path_0', | ||
| nodes, | ||
| edges, | ||
| description: `Linear path: ${nodes.join(' → ')}` | ||
| }]; | ||
| } |
There was a problem hiding this comment.
The fallback path generation is flawed. It assumes a linear execution flow based on the order of nodes in the ir.nodes array, which is not guaranteed to be correct. It also fabricates edge IDs between consecutive nodes, which may not correspond to actual edges in the graph. This can result in a nonsensical or broken fallback animation. A more robust fallback would be to perform a simple depth-first traversal from the entry node without exploring branches, to construct a single, valid path.
| /* Animation styles */ | ||
| .animated-current > rect, | ||
| .animated-current > polygon, | ||
| .animated-current > circle, | ||
| .animated-current > path { | ||
| stroke: #00ff00 !important; | ||
| stroke-width: 5px !important; | ||
| filter: drop-shadow(0 0 10px #00ff00); | ||
| animation: pulse-glow 1s ease-in-out infinite alternate; | ||
| } | ||
|
|
||
| .animated-visited > rect, | ||
| .animated-visited > polygon, | ||
| .animated-visited > circle, | ||
| .animated-visited > path { | ||
| fill: rgba(0, 255, 0, 0.2) !important; | ||
| stroke: #00aa00 !important; | ||
| stroke-width: 2px !important; | ||
| } | ||
|
|
||
| .animated-edge { | ||
| stroke: #00ff00 !important; | ||
| stroke-width: 4px !important; | ||
| filter: drop-shadow(0 0 4px #00ff00); | ||
| } |
There was a problem hiding this comment.
The animation styles use hardcoded colors (e.g., #00ff00, #00aa00). This will not look good on all user themes and may cause accessibility issues due to poor contrast. Please use VS Code theme variables (e.g., var(--vscode-charts-green)) to ensure the animation colors are consistent with the user's environment.
/* Animation styles */
.animated-current > rect,
.animated-current > polygon,
.animated-current > circle,
.animated-current > path {
stroke: var(--vscode-charts-green) !important;
stroke-width: 5px !important;
filter: drop-shadow(0 0 10px var(--vscode-charts-green));
animation: pulse-glow 1s ease-in-out infinite alternate;
}
.animated-visited > rect,
.animated-visited > polygon,
.animated-visited > circle,
.animated-visited > path {
fill: var(--vscode-charts-green) !important;
fill-opacity: 0.2 !important;
stroke: var(--vscode-charts-green) !important;
stroke-width: 2px !important;
}
.animated-edge {
stroke: var(--vscode-charts-green) !important;
stroke-width: 4px !important;
filter: drop-shadow(0 0 4px var(--vscode-charts-green));
}| export interface AnimationPath { | ||
| id: string; | ||
| nodes: string[]; | ||
| edges: string[]; | ||
| description: string; | ||
| } |
There was a problem hiding this comment.
The AnimationPath interface is duplicated here. It has also been added to src/ir/ir.ts in this pull request. To maintain a single source of truth and avoid potential inconsistencies, please remove this local definition and import it from ../../ir/ir instead. You will need to update the import statement on line 1 accordingly.
| public static getPathDescription(path: AnimationPath, maxLength: number = 50): string { | ||
| if (path.description.length <= maxLength) { | ||
| return path.description; | ||
| } | ||
|
|
||
| return path.description.substring(0, maxLength - 3) + '...'; | ||
| } | ||
|
|
||
| /** | ||
| * Check if a path contains loops | ||
| */ | ||
| public static hasLoops(path: AnimationPath): boolean { | ||
| const nodeCounts = new Map<string, number>(); | ||
|
|
||
| for (const nodeId of path.nodes) { | ||
| const count = nodeCounts.get(nodeId) || 0; | ||
| nodeCounts.set(nodeId, count + 1); | ||
| } | ||
|
|
||
| return Array.from(nodeCounts.values()).some(count => count > 1); | ||
| } |
There was a problem hiding this comment.
These methods (getPathDescription and hasLoops) are public but seem to be unused in the current implementation. If they are intended for future use, it might be worth adding a comment indicating that. Otherwise, they should be removed to avoid dead code.
| @keyframes pulse-glow { | ||
| from { | ||
| filter: drop-shadow(0 0 10px #00ff00); | ||
| } | ||
| to { | ||
| filter: drop-shadow(0 0 20px #00ff00); | ||
| } | ||
| } |
There was a problem hiding this comment.
The @keyframes pulse-glow animates the filter property. Animating filter is often not hardware-accelerated and can be performance-intensive, potentially causing jank. For a smoother pulsing effect, consider animating transform: scale() and opacity instead, as these properties are typically handled by the GPU.
| const shortDesc = currentPath.description.length > 15 | ||
| ? currentPath.description.substring(0, 15) + '...' | ||
| : currentPath.description; | ||
| pathSelector.textContent = \`Path \${animationState.currentPathIndex + 1}\`; |
There was a problem hiding this comment.
The updatePathSelector function calculates a shortDesc by truncating the path's description, but this variable is never used. The button text is set to a generic Path {n}. Using the shortDesc for the button's text would make the UI more informative for the user. I've also slightly increased the length before truncation.
const shortDesc = currentPath.description.length > 25
? currentPath.description.substring(0, 25) + '...'
: currentPath.description;
pathSelector.textContent = `Path (${animationState.currentPathIndex + 1}/${animationState.paths.length}) ${shortDesc}`;
No description provided.