feat: support escaped quoted edge labels in DSL#28
Conversation
📝 WalkthroughWalkthroughThe DSL parser now supports edge labels with both single and double quote syntax. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds robust parsing for quoted (single/double) edge labels in the DSL, preserving escapes and rejecting ambiguous labeled-edge arrow forms, with accompanying tests and documentation updates.
Changes:
- Extend DSL tokenizer/parser to support both single- and double-quoted edge labels (with escape preservation) and enforce the strict
-> "label" ->/-> 'label' ->form. - Add unit and integration coverage for escaped quotes/backslashes and for invalid/ambiguous labeled-edge syntax.
- Update README with examples and CLI-focused guidance for label escaping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/parser/dsl-parser.ts |
Adds quoted-label parsing (single/double) and stricter labeled-edge syntax validation with clearer error reporting. |
tests/unit/parser/dsl-parser.test.ts |
Adds unit tests covering new quoting/escaping behavior and malformed labeled-edge rejections. |
tests/integration/exporter/edge-labels.test.ts |
New end-to-end tests verifying labels bind to arrows and single-quoted labels work through generation. |
README.md |
Documents single-quoted labels and shell escaping guidance for CLI usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const escaped = input[i + 1]; | ||
| if (escaped === quote || escaped === '\\') { | ||
| value += escaped; | ||
| } else { | ||
| value += escaped; | ||
| } |
There was a problem hiding this comment.
parseQuotedLabel has a redundant conditional: both branches append escaped, so the if (escaped === quote || escaped === "\\") block is dead code. Simplify to a single append (or adjust logic if you intended different behavior for non-quote/non-backslash escapes).
| const arrow = file.elements.find((element: any) => element.type === 'arrow'); | ||
| const text = file.elements.find( | ||
| (element: any) => element.type === 'text' && element.containerId === arrow?.id | ||
| ); | ||
|
|
||
| expect(arrow).toBeTruthy(); | ||
| expect(arrow.boundElements).toEqual([{ id: text.id, type: 'text' }]); | ||
| expect(text).toBeTruthy(); |
There was a problem hiding this comment.
The test uses text.id before asserting text is defined. If the label binding breaks, this will throw a TypeError and hide the actual failure. Assert text exists before dereferencing (and consider finding text only after verifying arrow).
| describe('edge label integration', () => { | ||
| it('binds escaped edge labels to arrows in generated Excalidraw output', async () => { | ||
| const dsl = String.raw`[API] -> "GET /users?name=\"pp\" \\ path" -> [Client]`; |
There was a problem hiding this comment.
These integration tests call createFlowchartFromDSL (which runs ELK layout) but don't set an explicit timeout, while other exporter integration tests use 30s. Consider adding a timeout to avoid CI flakiness on slower runners.
![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
🧹 Nitpick comments (1)
tests/integration/exporter/edge-labels.test.ts (1)
10-19: Assert the lookup results before dereferencing them.
text.id,text.text, andarrow.idare read before the test has conclusively proven those elements exist, so a regression here can surface as a TypeError instead of a clear assertion failure.✅ Suggested cleanup
const arrow = file.elements.find((element: any) => element.type === 'arrow'); - const text = file.elements.find( - (element: any) => element.type === 'text' && element.containerId === arrow?.id - ); - - expect(arrow).toBeTruthy(); - expect(arrow.boundElements).toEqual([{ id: text.id, type: 'text' }]); - expect(text).toBeTruthy(); - expect(text.text).toBe('GET /users?name="pp" \\ path'); - expect(text.containerId).toBe(arrow.id); + expect(arrow).toBeTruthy(); + + const text = file.elements.find( + (element: any) => element.type === 'text' && element.containerId === arrow!.id + ); + expect(text).toBeTruthy(); + + expect(arrow!.boundElements).toEqual([{ id: text!.id, type: 'text' }]); + expect(text!.text).toBe('GET /users?name="pp" \\ path'); + expect(text!.containerId).toBe(arrow!.id); }); it('supports single-quoted edge labels end to end', async () => { const raw = await createFlowchartFromDSL(String.raw`[Decision] -> 'team\'s choice' -> [Next]`); const file = JSON.parse(raw); const text = file.elements.find((element: any) => element.type === 'text' && element.containerId); - expect(text.text).toBe("team's choice"); + expect(text).toBeTruthy(); + expect(text!.text).toBe("team's choice"); });Also applies to: 26-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/exporter/edge-labels.test.ts` around lines 10 - 19, The test currently dereferences text.id, text.text and arrow.id before ensuring those objects exist; update the assertions in tests/integration/exporter/edge-labels.test.ts to first assert arrow and text are truthy (e.g., expect(arrow).toBeTruthy(); expect(text).toBeTruthy();) before accessing arrow.id or text.id/text.text, and then perform the deeper checks (boundElements, containerId, and text content) so any missing element yields a clear assertion failure instead of a TypeError; apply the same change to the other occurrences around lines 26-27.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 102-107: The fenced DSL block containing "(Start) -> [Enter
Credentials] -> {Valid?} ..." is missing a language tag which trips markdownlint
MD040; update that fenced code block in README.md by adding the "excalidraw"
language tag (i.e., change the opening ``` to ```excalidraw) so the snippet
(including lines with {Valid?} -> "yes" -> [Dashboard] and [API] -> "GET
/users?name=\"pp\" \\ cache" -> [Client]) is syntax-highlighted and consistent
with the other DSL examples.
In `@src/parser/dsl-parser.ts`:
- Around line 750-770: The parser currently accepts labeled-edge syntax even
when there is no source node (e.g., `-> "x" -> [B]`) because the branch that
handles arrow + label sets pendingLabel/pendingDashed and advances i without
verifying a source; update the branch in the function handling tokens (the code
referencing token, nextToken, trailingArrow, targetNodeToken, pendingLabel,
pendingDashed, i) to verify a valid source (e.g., that lastNode is not null or
the previous token was a node/image) before setting pendingLabel; if no source
exists, throw a descriptive Error (similar to the other labeled-edge errors)
instead of silently accepting, and add a regression test for the `-> "x" -> [B]`
case.
---
Nitpick comments:
In `@tests/integration/exporter/edge-labels.test.ts`:
- Around line 10-19: The test currently dereferences text.id, text.text and
arrow.id before ensuring those objects exist; update the assertions in
tests/integration/exporter/edge-labels.test.ts to first assert arrow and text
are truthy (e.g., expect(arrow).toBeTruthy(); expect(text).toBeTruthy();) before
accessing arrow.id or text.id/text.text, and then perform the deeper checks
(boundElements, containerId, and text content) so any missing element yields a
clear assertion failure instead of a TypeError; apply the same change to the
other occurrences around lines 26-27.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8bb0437-5d2f-41a6-a709-9e0e8d5d6fc6
📒 Files selected for processing (4)
README.mdsrc/parser/dsl-parser.tstests/integration/exporter/edge-labels.test.tstests/unit/parser/dsl-parser.test.ts
| ``` | ||
| (Start) -> [Enter Credentials] -> {Valid?} | ||
| {Valid?} -> "yes" -> [Dashboard] -> (End) | ||
| {Valid?} -> "no" -> [Show Error] -> [Enter Credentials] | ||
| {Valid?} -> 'no' -> [Show Error] -> [Enter Credentials] | ||
| [API] -> "GET /users?name=\"pp\" \\ cache" -> [Client] | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this DSL example fence.
This block trips markdownlint MD040 and loses syntax highlighting; excalidraw would match the other DSL snippets in this README.
📝 Suggested change
-```
+```excalidraw
(Start) -> [Enter Credentials] -> {Valid?}
{Valid?} -> "yes" -> [Dashboard] -> (End)
{Valid?} -> 'no' -> [Show Error] -> [Enter Credentials]
[API] -> "GET /users?name=\"pp\" \\ cache" -> [Client]
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 102 - 107, The fenced DSL block containing "(Start)
-> [Enter Credentials] -> {Valid?} ..." is missing a language tag which trips
markdownlint MD040; update that fenced code block in README.md by adding the
"excalidraw" language tag (i.e., change the opening ``` to ```excalidraw) so the
snippet (including lines with {Valid?} -> "yes" -> [Dashboard] and [API] -> "GET
/users?name=\"pp\" \\ cache" -> [Client]) is syntax-highlighted and consistent
with the other DSL examples.
| if (token.type === 'arrow') { | ||
| const nextToken = tokens[i + 1]; | ||
| if (nextToken?.type === 'label') { | ||
| const trailingArrow = tokens[i + 2]; | ||
| const targetNodeToken = tokens[i + 3]; | ||
|
|
||
| if (token.value !== '->' || trailingArrow?.type !== 'arrow' || trailingArrow.value !== '->') { | ||
| const rawLabel = nextToken.raw ?? JSON.stringify(nextToken.value); | ||
| throw new Error( | ||
| `Invalid labeled edge syntax around ${rawLabel}: expected [A] -> ${rawLabel} -> [B], got ${formatArrowToken(token)} ${rawLabel} ${formatArrowToken(trailingArrow)}` | ||
| ); | ||
| } | ||
|
|
||
| if (!targetNodeToken || (targetNodeToken.type !== 'node' && targetNodeToken.type !== 'image')) { | ||
| const rawLabel = nextToken.raw ?? JSON.stringify(nextToken.value); | ||
| throw new Error(`Invalid labeled edge syntax around ${rawLabel}: missing target node after label`); | ||
| } | ||
|
|
||
| pendingDashed = false; | ||
| pendingLabel = nextToken.value; | ||
| i += 3; |
There was a problem hiding this comment.
Reject labeled edges that have no source node.
-> "x" -> [B] currently passes this branch, sets pendingLabel, and then gets silently discarded because lastNode is still null. The parser should fail fast here instead of accepting malformed DSL. A regression test for that exact input would be worth adding with the fix.
🐛 Suggested fix
if (token.type === 'arrow') {
const nextToken = tokens[i + 1];
if (nextToken?.type === 'label') {
+ if (!lastNode) {
+ const rawLabel = nextToken.raw ?? JSON.stringify(nextToken.value);
+ throw new Error(`Invalid labeled edge syntax around ${rawLabel}: missing source node before label`);
+ }
+
const trailingArrow = tokens[i + 2];
const targetNodeToken = tokens[i + 3];
if (token.value !== '->' || trailingArrow?.type !== 'arrow' || trailingArrow.value !== '->') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parser/dsl-parser.ts` around lines 750 - 770, The parser currently
accepts labeled-edge syntax even when there is no source node (e.g., `-> "x" ->
[B]`) because the branch that handles arrow + label sets
pendingLabel/pendingDashed and advances i without verifying a source; update the
branch in the function handling tokens (the code referencing token, nextToken,
trailingArrow, targetNodeToken, pendingLabel, pendingDashed, i) to verify a
valid source (e.g., that lastNode is not null or the previous token was a
node/image) before setting pendingLabel; if no source exists, throw a
descriptive Error (similar to the other labeled-edge errors) instead of silently
accepting, and add a regression test for the `-> "x" -> [B]` case.
Summary
--> "x" ->instead of silently guessingTesting
node dist/cli.js parse tests/tmp/edge-label-parse.dslnode dist/cli.js create --inline "[API] -> \"GET /users?name=\\\"pp\\\" \\\\ cache\" -> [Client]" -o tests/tmp/edge-label-demo.excalidrawNotes
npm run test:runstill has pre-existing failures intests/integration/exporter/cli-export.test.tsbecause that suite assumes a builtdist/cli.jsin a way this repo currently doesn't guarantee during Vitest runs. The new parser/unit/integration coverage for this feature passes.Summary by CodeRabbit
New Features
Documentation
Tests