[feature] Add annotation based graphs to Baml #2173
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aaronvg
reviewed
Jul 19, 2025
| // 24 | | ||
| // 25 | function SummarizeVideo(transcript: string) -> string { | ||
| // | | ||
| // error: Error validating: Type mismatch in if |
There was a problem hiding this comment.
mind explaining what this error message means? If there's a type mismatch i would expect what types are mismatching. Is it that title is string and in our lang we dont test for null like this? All errors should probably go through human + AI review.
|
LGTM 👍 |
Review Summary🔍 Comments beyond diff scope (3)
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| @@ -333,6 +333,7 @@ impl Block { | |||
| is_mutable, | |||
| expr, | |||
| span, | |||
| .. | |||
There was a problem hiding this comment.
correctness: .. in struct pattern matching (lines 336, 362) will ignore unmentioned fields, which can cause silent logic errors if new fields are added to the struct and not handled, leading to missed required data or invariants.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-compiler/src/hir/lowering.rs, line 336, the use of `..` in struct pattern matching can cause silent logic errors if new fields are added to the struct and not handled, leading to missed required data or invariants. Replace `..` with explicit field matching and add a TODO comment to ensure all fields are handled explicitly.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| .. | |
| // TODO: Explicitly match all fields to avoid missing new fields in the future | |
Comment on lines
604
to
614
| // Pick a reasonable span for the default header | ||
| let span_for_default = if let Some(expr) = &block.expr { | ||
| expr.span().clone() | ||
| } else if let Some(first_stmt) = block.stmts.first() { | ||
| first_stmt.span().clone() | ||
| } else { | ||
| // If there is nothing, skip creating a default header | ||
| // as we do not have a meaningful span. | ||
| self.pop_scope(); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
correctness: When both block.expr and block.stmts are empty, the function now returns early, preventing a panic when trying to clone a span; this avoids a crash in empty blocks.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
No action needed. The code now correctly avoids panicking on empty blocks by returning early if both `block.expr` and `block.stmts` are empty. No fix required.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // Pick a reasonable span for the default header | |
| let span_for_default = if let Some(expr) = &block.expr { | |
| expr.span().clone() | |
| } else if let Some(first_stmt) = block.stmts.first() { | |
| first_stmt.span().clone() | |
| } else { | |
| // If there is nothing, skip creating a default header | |
| // as we do not have a meaningful span. | |
| self.pop_scope(); | |
| return; | |
| }; | |
| let span_for_default = if let Some(expr) = &block.expr { | |
| expr.span().clone() | |
| } else if let Some(first_stmt) = block.stmts.first() { | |
| first_stmt.span().clone() | |
| } else { | |
| // If there is nothing, skip creating a default header | |
| // as we do not have a meaningful span. | |
| self.pop_scope(); | |
| return; | |
| }; |
Comment on lines
551
to
562
| Stmt::Expression(expr) => { | ||
| // Plain expression statements – just visit the expression | ||
| self.visit_expression(expr); | ||
| } | ||
| Stmt::Assign(assign_stmt) => { | ||
| // Visit the RHS expression so calls/headers inside are discovered | ||
| self.visit_expression(&assign_stmt.expr); | ||
| } | ||
| Stmt::AssignOp(assign_op_stmt) => { | ||
| // Visit the RHS expression so calls/headers inside are discovered | ||
| self.visit_expression(&assign_op_stmt.expr); | ||
| } |
There was a problem hiding this comment.
performance: visit_expression_block contains a large match on Stmt with repeated logic for visiting expressions in Stmt::Expression, Stmt::Assign, and Stmt::AssignOp, leading to significant code duplication and maintainability burden.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
Refactor engine/baml-lib/ast/src/ast/header_collector.rs lines 551-562 to remove code duplication in the match arms for Stmt::Expression, Stmt::Assign, and Stmt::AssignOp. Each arm only calls self.visit_expression on the relevant expression. Collapse these arms to single-line match arms without repeated comments, improving maintainability.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| Stmt::Expression(expr) => { | |
| // Plain expression statements – just visit the expression | |
| self.visit_expression(expr); | |
| } | |
| Stmt::Assign(assign_stmt) => { | |
| // Visit the RHS expression so calls/headers inside are discovered | |
| self.visit_expression(&assign_stmt.expr); | |
| } | |
| Stmt::AssignOp(assign_op_stmt) => { | |
| // Visit the RHS expression so calls/headers inside are discovered | |
| self.visit_expression(&assign_op_stmt.expr); | |
| } | |
| Stmt::Expression(expr) => self.visit_expression(expr), | |
| Stmt::Assign(assign_stmt) => self.visit_expression(&assign_stmt.expr), | |
| Stmt::AssignOp(assign_op_stmt) => self.visit_expression(&assign_op_stmt.expr), |
Comment on lines
+610
to
+613
| // Visit the final expression (if present) | ||
| if let Some(expr) = &block.expr { | ||
| let expr_id = self.visit_expression(expr); | ||
| self.connect(&block_id, &expr_id, Some("expr")); |
There was a problem hiding this comment.
correctness: self.visit_expression(&block.expr) is called unconditionally, but block.expr may be None, causing a panic at runtime.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/ast/mermaid_debug.rs, lines 610-613, the code previously called `self.visit_expression(&block.expr)` unconditionally, which would panic if `block.expr` is `None`. The fix is to only call `visit_expression` if `block.expr` is `Some`, as shown in the snippet. Please ensure this conditional is present to prevent runtime panics.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // Visit the final expression (if present) | |
| if let Some(expr) = &block.expr { | |
| let expr_id = self.visit_expression(expr); | |
| self.connect(&block_id, &expr_id, Some("expr")); | |
| // Visit the final expression (if present) | |
| if let Some(expr) = &block.expr { | |
| let expr_id = self.visit_expression(expr); | |
| self.connect(&block_id, &expr_id, Some("expr")); | |
| } |
Comment on lines
388
to
393
| Some(ExpressionBlock { | ||
| stmts, | ||
| expr: return_expr, | ||
| expr_headers: headers_since_last_stmt, | ||
| }) | ||
| } |
There was a problem hiding this comment.
performance: The function parse_expr_block (lines 257-393) is over 130 lines long, combining parsing, error handling, and AST construction, making it difficult to maintain and optimize as the grammar evolves.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
Refactor the function `parse_expr_block` in engine/baml-lib/ast/src/parser/parse_expr.rs (lines 257-393). The function is over 130 lines long and combines parsing, error handling, and AST construction, making it difficult to maintain and optimize. Break it into smaller, well-named helper functions for each major responsibility (e.g., token extraction, statement parsing, header tracking, error handling, AST construction) to improve maintainability and enable future performance optimizations.
egol
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5c42b62 in 3 minutes and 20 seconds. Click for details.
- Reviewed
5730lines of code in75files - Skipped
1files when reviewing. - Skipped posting
18draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/test_simple_expr_fn.baml:1
- Draft comment:
The sample Baml function 'SimpleExpr' is clear and concise. No issues noted. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. typescript/packages/playground-common/package.json:65
- Draft comment:
New dependency 'mermaid' (^11.9.0) has been added. Please verify that this version is compatible with the rest of the system and update lock files if needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/atoms-orch-graph.ts:60
- Draft comment:
Consider renaming the 'Position' and 'Dimension' properties (in GroupEntry) to lowerCamelCase (e.g. 'position' and 'dimension') for consistency with typical TypeScript conventions. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/atoms-orch-graph.ts:400
- Draft comment:
The uuid() function uses a simple incremental counter which might lead to collisions on re-renders or remounts. Consider using a more robust unique ID generator if stable IDs are needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/prompt-render-wrapper.tsx:166
- Draft comment:
A new 'mermaid-graph' tab has been added. Please ensure its styling and layout remain consistent with other tabs in the UI. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/test-panel.1/components/MermaidGraphView.tsx:23
- Draft comment:
The mermaid.initialize call is executed on every graph update. If the configuration doesn’t change, consider moving it outside the useEffect to avoid unnecessary reinitializations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is technically correct - the initialization could be moved outside. However, mermaid.initialize() is likely not an expensive operation, and the graph updates probably don't happen frequently. The optimization feels premature without evidence of performance issues. The code works fine as is. I could be wrong about the performance impact - maybe mermaid initialization is actually expensive. The current approach could cause unnecessary work. While valid, we should follow the principle of not optimizing prematurely. The code is readable and works. If performance becomes an issue, profiling would reveal if this is actually a bottleneck. This comment suggests a premature optimization without clear evidence of benefit. The current code is fine and readable as is.
7. engine/baml-lib/ast/src/ast.rs:288
- Draft comment:
There appears to be a missing closing brace for the match block (and/or the function) in the as_template_string_id method. The code currently ends with an underscore arm ("_ => None,") but does not close the match or the function. Please add the necessary closing curly brace(s). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. engine/baml-lib/ast/src/parser/datamodel.pest:298
- Draft comment:
Typographical issue: The code example on this line seems to be missing a closing backtick. The comment currently reads: // - `let z = { let b = 2; [b, 3] } Consider adding a closing backtick after the example for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
9. engine/baml-lib/ast/src/parser/parse_expr.rs:342
- Draft comment:
Typographical error: "Commentend out because we can't have blocks without return" appears to have a misspelling. Consider updating "Commentend" to "Commented". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. engine/baml-lib/baml/tests/validation_files/headers/complex_headers_test.baml:22
- Draft comment:
Typographical consistency: Consider capitalizing the first word in the header. Change "### another header in the statement" to "### Another header in the statement" for consistency with the other headers. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about inconsistency, this is a test file that's being added, and the comment is purely about style. The inconsistency doesn't affect functionality. The headers appear to be part of test cases that might be intentionally testing different header formats. Also, we're told not to comment on pure UI/styling changes. The inconsistency could make the test file harder to read and maintain. Maybe consistent capitalization is important for the testing framework? This is clearly a test file specifically about headers, and the varying formats might be intentional test cases. Even if not, it's a minor styling issue in a test file. Delete this comment as it's purely about style in a test file, and the inconsistency might even be intentional for testing purposes.
11. engine/baml-lib/baml/tests/validation_files/headers/complex_headers_test.baml:32
- Draft comment:
Typographical consistency: Consider capitalizing the first word in the header. Change "## very useful header" to "## Very useful header" for consistency with the other headers. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test file specifically for testing headers in BAML. The inconsistent capitalization might actually be intentional for testing purposes. The comment is about code style rather than functionality. Given this is a test file, enforcing style consistency could actually be counterproductive as it might reduce test coverage of different header formats. I might be wrong about the intention - maybe consistent header formatting is important even in test files. Also, there could be specific BAML style guidelines I'm not aware of. Even if there are style guidelines, this is explicitly a test file for headers, and having variety in the test cases is likely beneficial. The inconsistency appears intentional. The comment should be deleted as it's suggesting a style change in a test file where the inconsistency might be intentional and beneficial for testing purposes.
12. engine/baml-lib/baml/tests/validation_files/headers/complex_workflow.mmd:10
- Draft comment:
Typographical inconsistency: The node label 'GenerateTitle' (line 10) is missing a space compared to 'Generate Title' (line 8). Please verify if this was intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Since this is a test file in a validation directory, it's likely these inconsistencies are intentional for testing purposes. The nodes appear to serve different purposes - one is a display label (n3) and one is a function call node (n4). Also, this is a new file, so any inconsistency was deliberately added this way. I could be wrong about the purpose of the test file. Maybe consistent naming is important for the test to work properly. The file's location in validation_files strongly suggests this is a test case. Test cases often intentionally include edge cases and variations to verify proper handling. Delete this comment as it's pointing out an intentional difference in a test file between display labels and function call nodes.
13. engine/baml-lib/baml/tests/validation_files/headers/complex_workflow.mmd:22
- Draft comment:
Typographical inconsistency: The node label 'GenerateTitle' (line 22) is missing a space compared to 'Generate Title' (line 8). Consider reviewing this for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The different naming appears intentional - n3 is a process step label ("Generate Title") while n4 and n8 appear to be references to an actual function or component name ("GenerateTitle"). This is supported by n4 having a "callNode" class. The current naming actually provides better clarity about the different roles of these nodes. I could be wrong about the intentional distinction - maybe it really is just an inconsistency that should be fixed. Without more context about the system's naming conventions, I can't be 100% certain. The presence of the "callNode" class and the consistent use of "GenerateTitle" for both n4 and n8 strongly suggests this is an intentional pattern, not a mistake. The comment should be deleted as it appears to be suggesting a change that would actually reduce clarity by removing an intentional distinction between process step labels and function references.
14. engine/baml-lib/baml/tests/validation_files/headers/complex_workflow.mmd:17
- Draft comment:
Typographical note: The node label 'SummarizeVideo' (line 17) might be clearer as 'Summarize Video' if that matches the intended naming convention. Please double-check. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This appears to be a purely stylistic suggestion. The name 'SummarizeVideo' follows camelCase convention which is common in code. The name appears twice in the file consistently. This is a test file, and the naming seems intentional. The comment also asks for verification rather than making a strong case for change. The naming convention could be inconsistent with other parts of the codebase that we can't see. Some teams might prefer space-separated names in diagrams for readability. Since this is a test file and the naming is consistent within the file, and the comment asks for verification rather than pointing out a clear issue, it's not a useful comment. Delete this comment as it's purely stylistic, asks for verification, and doesn't point out a clear issue that needs fixing.
15. engine/baml-lib/baml/tests/validation_files/headers/complex_workflow.mmd:23
- Draft comment:
Typographical note: The node label 'SummarizeVideo' (line 23) might be clearer as 'Summarize Video' to maintain consistency with naming conventions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test file in a tests directory. The naming inconsistency seems intentional as part of the test cases. Making purely stylistic suggestions about test files is not valuable. Additionally, this seems like a minor formatting preference rather than a significant issue requiring a code change. The inconsistent naming might actually cause confusion in a real codebase. Maybe maintaining consistent naming conventions is important even in test files? Test files often intentionally include variations to test different scenarios. The inconsistency here appears deliberate and changing it could defeat the purpose of the test. Delete this comment as it's a purely stylistic suggestion on a test file where the inconsistency appears intentional.
16. engine/baml-lib/baml/tests/validation_files/headers/complex_workflow.mmd:24
- Draft comment:
Typographical suggestion: The label 'client 1' (line 24) is lowercase and might be intended as 'Client 1' for consistency with the capitalization used in other nodes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While there is some inconsistency in capitalization, this is a test file and the capitalization difference is minor. The comment doesn't point out a functional issue or code quality problem that would impact the system's behavior. It's purely cosmetic and relatively trivial. Additionally, since this is in a test directory, the standards for cosmetic consistency might be more relaxed. The inconsistent capitalization could make the diagram less professional looking and harder to read. It might indicate a copy-paste error or oversight. While consistency is good, this is a minor styling issue in a test file. The comment doesn't meet the threshold of being an important enough issue to flag. Delete the comment as it addresses a trivial cosmetic issue in a test file and doesn't point out any significant problems that need fixing.
17. engine/baml-lib/baml/tests/validation_files/headers/edge_cases.mmd:127
- Draft comment:
Typo: The header in this line contains the HTML entity"within the title. Please confirm if this was intended, or if a literal double quote is desired. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test file specifically created to test edge cases with headers and special characters. The section this is in is called "VeryLongHeaderTitle" and includes deliberately complex content. The presence of " could be intentional as part of testing HTML entities. Making assumptions about what characters should be used in a test file could be dangerous. I might be overthinking this - maybe the HTML entity really is unintentional and should be replaced with a regular quote for consistency with the other special characters in the string. Given this is explicitly a test file for edge cases, we should be very cautious about suggesting changes to special characters - the HTML entity might be there deliberately to test parser behavior. We should delete this comment as it's making assumptions about the intended format in a test file specifically designed to test edge cases with special characters.
18. engine/baml-lib/baml/tests/validation_files/headers/with_llm_functions.mmd:5
- Draft comment:
Typographical note: The node label 'client 1' is in lowercase while the other nodes use a title case format (e.g., 'GenerateTitle', 'SummarizeVideo', 'CombineOutputs'). Consider revising it to 'Client 1' for consistency, if that aligns with the intended naming convention. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test file in a validation_files directory. The comment is about pure styling/formatting consistency. Our rules explicitly state not to comment on UI or styling changes. While the suggestion might be valid, it's not related to logic or functionality. The inconsistent casing could potentially indicate a semantic difference that's important for the test case. Maybe the lowercase is intentional for testing purposes? Even if the casing is intentional, our rules clearly state not to comment on UI/styling changes. We should assume the author formatted it this way for a reason. The comment should be deleted as it focuses purely on styling/formatting consistency, which our rules explicitly say to ignore.
Workflow ID: wflow_6CMCgg2MEyuo08qo
You can customize 
| // Render calls for the child at the current scope level anchored to its rep id | ||
| self.render_calls_for_header(index, child_root, None, &start_rep_id, 0); | ||
|
|
||
| // Determine the terminal representative in the child scope |
There was a problem hiding this comment.
There’s a duplicate comment here: "Determine the terminal representative in the child scope" is repeated twice. Consider removing one of the redundant lines.
| @@ -117,6 +117,9 @@ pub trait InternalRuntimeInterface { | |||
| ctx: &RuntimeContext, | |||
| ) -> Result<Vec<OrchestratorNode>>; | |||
|
|
|||
| /// send a mermaid graph of the function graph | |||
There was a problem hiding this comment.
Typo/grammar suggestion: Consider updating the documentation comment from "send a mermaid graph of the function graph" to "Sends a Mermaid graph of the function graph" for consistency and proper grammatical tone.
Suggested change
| /// send a mermaid graph of the function graph | |
| /// Sends a Mermaid graph of the function graph |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed edf6023 in 2 minutes and 26 seconds. Click for details.
- Reviewed
3013lines of code in23files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/test-panel/components/MermaidGraphView.tsx:88
- Draft comment:
Use proper CSS comment syntax instead of using '//' comments. CSS does not officially support '//' comments, so replace them with /* … */ to ensure compatibility. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct that CSS doesn't support // comments, this code is within a JavaScript template literal string, not a pure CSS file. The // comments won't cause any issues since they're just commented out code that isn't being used. The comment is being overly pedantic about a non-issue. The reviewer could argue that maintaining consistent comment style throughout the CSS, even in template literals, makes the code more maintainable and cleaner. The code works perfectly fine as is, and these are just temporary commented-out lines. The benefit of changing the comment style is minimal and not worth the developer's time. This comment should be deleted as it's overly pedantic about comment syntax in commented-out code within a template literal, where the syntax doesn't actually matter.
2. typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/test-panel/components/MermaidGraphView.tsx:441
- Draft comment:
The removal of the 'wheel' event listener is using a new anonymous function, which won’t match the original listener. Store the wheelPreventer function in a variable so that the same reference can be used during removal. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_xGbhPRB53ZuYjrBc
You can customize
| --- | ||
| ## How to run | ||
|
|
||
| NOTE: The baml_vis children collapse config will effect these tests and can prevent them from passing |
There was a problem hiding this comment.
Typo: The phrase "will effect these tests" should be corrected to "will affect these tests".
Suggested change
| NOTE: The baml_vis children collapse config will effect these tests and can prevent them from passing | |
| NOTE: The baml_vis children collapse config will affect these tests and can prevent them from passing |
Comment on lines
+248
to
+260
| let mut prev_exits: Option<Vec<String>> = None; | ||
| for hid in items { | ||
| let (entry, exits) = self.build_header(hid, visited_scopes, parent_cluster.clone()); | ||
| if let Some(prev) = prev_exits.take() { | ||
| for e in prev { | ||
| self.graph.edges.push(Edge { | ||
| from: e, | ||
| to: entry.clone(), | ||
| }); | ||
| } | ||
| } | ||
| prev_exits = Some(exits); | ||
| } |
There was a problem hiding this comment.
correctness: build_scope_sequence does not update prev_exits if items is empty, causing possible missing edge connections for empty scopes.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/ast/baml_vis.rs, lines 248-260, the function `build_scope_sequence` does not update `prev_exits` if `items` is empty, which can cause missing edge connections for empty scopes. Please ensure that after the for loop, `prev_exits` is set to `Some(vec![])` if it is still `None`, to maintain correct edge propagation for empty scopes.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| let mut prev_exits: Option<Vec<String>> = None; | |
| for hid in items { | |
| let (entry, exits) = self.build_header(hid, visited_scopes, parent_cluster.clone()); | |
| if let Some(prev) = prev_exits.take() { | |
| for e in prev { | |
| self.graph.edges.push(Edge { | |
| from: e, | |
| to: entry.clone(), | |
| }); | |
| } | |
| } | |
| prev_exits = Some(exits); | |
| } | |
| let mut prev_exits: Option<Vec<String>> = None; | |
| for hid in items { | |
| let (entry, exits) = self.build_header(hid, visited_scopes, parent_cluster.clone()); | |
| if let Some(prev) = prev_exits.take() { | |
| for e in prev { | |
| self.graph.edges.push(Edge { | |
| from: e, | |
| to: entry.clone(), | |
| }); | |
| } | |
| } | |
| prev_exits = Some(exits); | |
| } | |
| // Ensure prev_exits is at least set to an empty vector if items is empty | |
| if prev_exits.is_none() { | |
| prev_exits = Some(vec![]); | |
| } | |
Comment on lines
+589
to
+685
| fn emit( | ||
| out: &mut Vec<String>, | ||
| cluster: Option<&Cluster>, | ||
| children_by_parent: &HashMap<Option<String>, Vec<&Cluster>>, | ||
| nodes_by_cluster: &HashMap<Option<String>, Vec<&Node>>, | ||
| use_fancy: bool, | ||
| indent: usize, | ||
| ) { | ||
| let indent_str = " ".repeat(indent); | ||
| let key = cluster.map(|c| c.id.clone()); | ||
| let key_opt = key.clone(); | ||
| if let Some(c) = cluster { | ||
| out.push(format!( | ||
| "{}subgraph {}[\"{}\"]", | ||
| indent_str, | ||
| c.id, | ||
| escape_label(&c.label) | ||
| )); | ||
| out.push(format!("{} direction LR", indent_str)); | ||
| } | ||
| if let Some(nodes) = nodes_by_cluster.get(&key_opt) { | ||
| for n in nodes { | ||
| match &n.kind { | ||
| NodeKind::Decision(_, _) => { | ||
| out.push(format!( | ||
| "{} {}{{\"{}\"}}", | ||
| indent_str, | ||
| n.id, | ||
| escape_label(&n.label) | ||
| )); | ||
| // Always emit decisionNode class line to match expected output | ||
| out.push(format!("{} class {} decisionNode;", indent_str, n.id)); | ||
| } | ||
| NodeKind::Call { .. } => { | ||
| out.push(format!( | ||
| "{} {}[\"{}\"]", | ||
| indent_str, | ||
| n.id, | ||
| escape_label(&n.label) | ||
| )); | ||
| if use_fancy && SHOW_CALL_NODES { | ||
| out.push(format!("{} class {} callNode;", indent_str, n.id)); | ||
| } | ||
| } | ||
| NodeKind::Header(_, _) => { | ||
| out.push(format!( | ||
| "{} {}[\"{}\"]", | ||
| indent_str, | ||
| n.id, | ||
| escape_label(&n.label) | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if let Some(children) = children_by_parent.get(&key_opt) { | ||
| for ch in children { | ||
| emit( | ||
| out, | ||
| Some(ch), | ||
| children_by_parent, | ||
| nodes_by_cluster, | ||
| use_fancy, | ||
| indent + 2, | ||
| ); | ||
| } | ||
| } | ||
| if cluster.is_some() { | ||
| out.push(format!("{}end", indent_str)); | ||
| } | ||
| } | ||
| emit( | ||
| &mut out, | ||
| None, | ||
| &children_by_parent, | ||
| &nodes_by_cluster, | ||
| use_fancy, | ||
| 0, | ||
| ); | ||
|
|
||
| let mut emitted: HashSet<(String, String)> = HashSet::new(); | ||
| for e in &graph.edges { | ||
| if emitted.insert((e.from.clone(), e.to.clone())) { | ||
| out.push(format!(" {} --> {}", e.from, e.to)); | ||
| } | ||
| } | ||
| // Click lines disabled for tests | ||
| if !span_map.is_empty() { | ||
| if let Ok(json) = serde_json::to_string(span_map) { | ||
| out.push(format!("%%__BAML_SPANMAP__={}", json)); | ||
| } | ||
| for rep_id in span_map.keys() { | ||
| out.push(format!( | ||
| " click {} call bamlMermaidNodeClick() \"Go to source\"", | ||
| rep_id | ||
| )); | ||
| } |
There was a problem hiding this comment.
performance: The emit function in MermaidRenderer recursively allocates and clones strings for every node and cluster, leading to significant CPU and memory usage for deeply nested or large graphs.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/ast/baml_vis.rs, lines 589-685, refactor the `emit` function in `MermaidRenderer` to avoid repeated allocation and cloning of indentation strings. Pass the indentation string as a reference and build it incrementally, rather than allocating a new string with `" ".repeat(indent)` on every recursive call. Update all calls to `emit` accordingly. This will significantly reduce CPU and memory usage for large or deeply nested graphs.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| fn emit( | |
| out: &mut Vec<String>, | |
| cluster: Option<&Cluster>, | |
| children_by_parent: &HashMap<Option<String>, Vec<&Cluster>>, | |
| nodes_by_cluster: &HashMap<Option<String>, Vec<&Node>>, | |
| use_fancy: bool, | |
| indent: usize, | |
| ) { | |
| let indent_str = " ".repeat(indent); | |
| let key = cluster.map(|c| c.id.clone()); | |
| let key_opt = key.clone(); | |
| if let Some(c) = cluster { | |
| out.push(format!( | |
| "{}subgraph {}[\"{}\"]", | |
| indent_str, | |
| c.id, | |
| escape_label(&c.label) | |
| )); | |
| out.push(format!("{} direction LR", indent_str)); | |
| } | |
| if let Some(nodes) = nodes_by_cluster.get(&key_opt) { | |
| for n in nodes { | |
| match &n.kind { | |
| NodeKind::Decision(_, _) => { | |
| out.push(format!( | |
| "{} {}{{\"{}\"}}", | |
| indent_str, | |
| n.id, | |
| escape_label(&n.label) | |
| )); | |
| // Always emit decisionNode class line to match expected output | |
| out.push(format!("{} class {} decisionNode;", indent_str, n.id)); | |
| } | |
| NodeKind::Call { .. } => { | |
| out.push(format!( | |
| "{} {}[\"{}\"]", | |
| indent_str, | |
| n.id, | |
| escape_label(&n.label) | |
| )); | |
| if use_fancy && SHOW_CALL_NODES { | |
| out.push(format!("{} class {} callNode;", indent_str, n.id)); | |
| } | |
| } | |
| NodeKind::Header(_, _) => { | |
| out.push(format!( | |
| "{} {}[\"{}\"]", | |
| indent_str, | |
| n.id, | |
| escape_label(&n.label) | |
| )); | |
| } | |
| } | |
| } | |
| } | |
| if let Some(children) = children_by_parent.get(&key_opt) { | |
| for ch in children { | |
| emit( | |
| out, | |
| Some(ch), | |
| children_by_parent, | |
| nodes_by_cluster, | |
| use_fancy, | |
| indent + 2, | |
| ); | |
| } | |
| } | |
| if cluster.is_some() { | |
| out.push(format!("{}end", indent_str)); | |
| } | |
| } | |
| emit( | |
| &mut out, | |
| None, | |
| &children_by_parent, | |
| &nodes_by_cluster, | |
| use_fancy, | |
| 0, | |
| ); | |
| let mut emitted: HashSet<(String, String)> = HashSet::new(); | |
| for e in &graph.edges { | |
| if emitted.insert((e.from.clone(), e.to.clone())) { | |
| out.push(format!(" {} --> {}", e.from, e.to)); | |
| } | |
| } | |
| // Click lines disabled for tests | |
| if !span_map.is_empty() { | |
| if let Ok(json) = serde_json::to_string(span_map) { | |
| out.push(format!("%%__BAML_SPANMAP__={}", json)); | |
| } | |
| for rep_id in span_map.keys() { | |
| out.push(format!( | |
| " click {} call bamlMermaidNodeClick() \"Go to source\"", | |
| rep_id | |
| )); | |
| } | |
| fn emit( | |
| out: &mut Vec<String>, | |
| cluster: Option<&Cluster>, | |
| children_by_parent: &HashMap<Option<String>, Vec<&Cluster>>, | |
| nodes_by_cluster: &HashMap<Option<String>, Vec<&Node>>, | |
| use_fancy: bool, | |
| indent: usize, | |
| indent_str: &str, | |
| ) { | |
| let next_indent = format!("{} ", indent_str); | |
| ... | |
| if let Some(children) = children_by_parent.get(&key_opt) { | |
| for ch in children { | |
| emit( | |
| out, | |
| Some(ch), | |
| children_by_parent, | |
| nodes_by_cluster, | |
| use_fancy, | |
| indent + 2, | |
| &next_indent, | |
| ); | |
| } | |
| } | |
| ... | |
| } | |
| // Update all calls to emit to pass the indent string by reference, avoiding repeated allocation. | |
Comment on lines
+150
to
+177
| fn precompute(&mut self) { | ||
| for h in &self.index.headers { | ||
| self.by_hid.insert(h.hid, h); | ||
| self.idstr_to_hid.insert(&h.id, h.hid); | ||
| self.scope_root.entry(h.scope).or_insert(h.hid); | ||
| } | ||
| for h in &self.index.headers { | ||
| if let Some(pid) = &h.parent_id { | ||
| if let Some(&ph) = self.idstr_to_hid.get(pid.as_str()) { | ||
| if let Some(parent) = self.by_hid.get(&ph) { | ||
| if parent.scope == h.scope { | ||
| self.md_children.entry(ph).or_default().push(h.hid); | ||
| self.has_md_parent.insert(h.hid); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| for (p, c) in self.index.nested_edges_hid_iter() { | ||
| if let (Some(ph), Some(ch)) = (self.index.get_by_hid(*p), self.index.get_by_hid(*c)) { | ||
| if ph.scope != ch.scope { | ||
| self.nested_children.entry(*p).or_default().push(*c); | ||
| self.nested_targets.insert(*c); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
performance: The precompute method in GraphBuilder repeatedly iterates over all headers for each parent-child relationship, resulting in O(n^2) time for large ASTs.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/ast/baml_vis.rs, lines 150-177, optimize the `precompute` method in `GraphBuilder` to avoid O(n^2) iteration over headers. Build a map from (parent_id, scope) to Hid in a first pass, then use it for efficient lookup in the second pass, reducing the time complexity to O(n).
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| fn precompute(&mut self) { | |
| for h in &self.index.headers { | |
| self.by_hid.insert(h.hid, h); | |
| self.idstr_to_hid.insert(&h.id, h.hid); | |
| self.scope_root.entry(h.scope).or_insert(h.hid); | |
| } | |
| for h in &self.index.headers { | |
| if let Some(pid) = &h.parent_id { | |
| if let Some(&ph) = self.idstr_to_hid.get(pid.as_str()) { | |
| if let Some(parent) = self.by_hid.get(&ph) { | |
| if parent.scope == h.scope { | |
| self.md_children.entry(ph).or_default().push(h.hid); | |
| self.has_md_parent.insert(h.hid); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| for (p, c) in self.index.nested_edges_hid_iter() { | |
| if let (Some(ph), Some(ch)) = (self.index.get_by_hid(*p), self.index.get_by_hid(*c)) { | |
| if ph.scope != ch.scope { | |
| self.nested_children.entry(*p).or_default().push(*c); | |
| self.nested_targets.insert(*c); | |
| } | |
| } | |
| } | |
| } | |
| let mut parent_scope_map: HashMap<(&str, ScopeId), Hid> = HashMap::new(); | |
| for h in &self.index.headers { | |
| parent_scope_map.insert((h.id.as_str(), h.scope), h.hid); | |
| } | |
| for h in &self.index.headers { | |
| if let Some(pid) = &h.parent_id { | |
| if let Some(&ph) = parent_scope_map.get(&(pid.as_str(), h.scope)) { | |
| self.md_children.entry(ph).or_default().push(h.hid); | |
| self.has_md_parent.insert(h.hid); | |
| } | |
| } | |
| } | |
Comment on lines
+263
to
+269
| // Inject CSS override so it takes precedence over Mermaid defaults inside the SVG | ||
| try { | ||
| const styleEl = document.createElement('style'); | ||
| styleEl.setAttribute('data-baml', 'mermaid-css-override'); | ||
| styleEl.textContent = MERMAID_CSS_OVERRIDE; | ||
| svgEl.appendChild(styleEl); | ||
| } catch {} |
There was a problem hiding this comment.
correctness: MERMAID_CSS_OVERRIDE is injected as a <style> child of the SVG, but SVG does not apply CSS from <style> elements inside the SVG in all browsers, causing the override to be ignored and resulting in incorrect diagram appearance.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In typescript/packages/playground-common/src/shared/baml-project-panel/playground-panel/prompt-preview/test-panel/components/MermaidGraphView.tsx, lines 263-269, the code injects a CSS override as a <style> element into the SVG using document.createElement('style'). However, SVGs require <style> elements to be created with the SVG namespace and inserted as the first child for cross-browser compatibility. Please update this block to use document.createElementNS('http://www.w3.org/2000/svg', 'style'), set type='text/css', and insert the style as the first child of the SVG element.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // Inject CSS override so it takes precedence over Mermaid defaults inside the SVG | |
| try { | |
| const styleEl = document.createElement('style'); | |
| styleEl.setAttribute('data-baml', 'mermaid-css-override'); | |
| styleEl.textContent = MERMAID_CSS_OVERRIDE; | |
| svgEl.appendChild(styleEl); | |
| } catch {} | |
| try { | |
| const styleEl = document.createElementNS('http://www.w3.org/2000/svg', 'style'); | |
| styleEl.setAttribute('type', 'text/css'); | |
| styleEl.setAttribute('data-baml', 'mermaid-css-override'); | |
| styleEl.textContent = MERMAID_CSS_OVERRIDE; | |
| svgEl.insertBefore(styleEl, svgEl.firstChild); | |
| } catch {} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Adds annotation-based Mermaid diagram generation and rendering for BAML functions, enhancing visualization capabilities in the BAML engine and web app.
BamlVisDiagramGeneratorandMermaidDiagramGenerator.function_graph()inruntime_interface.rsto generate function graphs.ProjectView.tsxandFileViewer.tsxto integrate Mermaid diagrams into the web app.Header,ExprStmt, andBamlVisDiagramGeneratorto handle annotations and diagram generation.ExpressionBlockandLetStmtto include annotations.parse.rsandparse_expr.rsto handle MDX headers and annotations.MermaidGraphViewcomponent to render diagrams in the web app.mermaidandsvg-pan-zoomdependencies topackage.json.FileViewer.tsxandProjectView.tsxfor better file handling and UI layout.This description was created by
for edf6023. You can customize this summary. It will automatically update as commits are pushed.