scripts/ci-validate-in-docker.ps1 (1)

9-9: Consider Write-Information or Write-Output instead of Write-Host.

Static analysis (PSAvoidUsingWriteHost) flags this. For a developer-facing CI script it's largely harmless, but Write-Output or Write-Information would be more robust across hosts and allow suppression/redirection.

Proposed fix

-Write-Host "==> Starting Linux container (node:22-bookworm) with workspace: $workspace"
+Write-Output "==> Starting Linux container (node:22-bookworm) with workspace: $workspace"

packages/language-server/src/documentation/documentation-provider.ts (1)

44-62: Inconsistent null-handling: parseDeploymentNode and parseDeployedInstance aren't null-checked unlike parseElement.

On line 64, parseElement has an explicit null guard, but parseDeploymentNode (line 46) and parseDeployedInstance (line 50) do not. If those parsers can return null/undefined, the resulting NPE would be silently swallowed by the catch block, returning undefined without clear context. If they can't return null, the parseElement guard may be unnecessary — either way, the inconsistency is worth aligning.

packages/language-server/src/lsp/HoverProvider.ts (1)

15-23: Consider documenting the class and constructor.

While the protected methods now have excellent documentation, the public class and constructor lack JSDoc. Adding class-level documentation would help developers understand the hover provider's role in the language server.

As per coding guidelines, public classes and methods should use JSDoc.

📝 Suggested class and constructor documentation

+/**
+ * Provides hover information for LikeC4 AST nodes in the language server.
+ * Generates markdown-formatted hover content for elements, deployment nodes, and deployed instances,
+ * including summaries, relationships, and links to views.
+ */
 export class LikeC4HoverProvider extends AstNodeHoverProvider {
   private parser: LikeC4ModelParser
   private locator: LikeC4ModelLocator
 
+  /**
+   * Creates a new LikeC4HoverProvider.
+   * `@param` services - The LikeC4 language services container providing model parser and locator.
+   */
   constructor(protected services: LikeC4Services) {

packages/language-server/src/browser/index.ts (1)

17-20: Consider adding JSDoc documentation for this public function.

The startLanguageServer function is exported and serves as a public API entry point for browser-based language server initialization. Adding JSDoc would improve maintainability by documenting the purpose, parameters, and return value. As per coding guidelines, public methods should include JSDoc documentation.

📝 Example JSDoc documentation

+/**
+ * Starts the LikeC4 language server in a browser context.
+ * 
+ * `@param` port - The message port or dedicated worker global scope for LSP communication
+ * `@returns` The initialized language services including shared and LikeC4-specific services
+ */
 export function startLanguageServer(port: MessagePort | DedicatedWorkerGlobalScope): {
   shared: LikeC4SharedServices
   likec4: LikeC4Services
 } {

scripts/ci-validate-in-docker.sh (1)

13-13: Hardcoded pnpm version may drift from packageManager in package.json.

Consider reading the version from package.json's packageManager field to keep them in sync, or add a comment noting the version must match.

e2e/src/likec4-cli-export-drawio.spec.ts (2)

46-54: Dead branch in assertion failure message.

The ternary at Line 51 checks result.exitCode === 1, but this message is only evaluated when the expect fails — which means failedByExitCode is false and result.exitCode !== 1. The true branch ('expected exitCode 1') is unreachable.

Simplify to just the descriptive else-branch message:

Proposed fix

     expect(
       failedByExitCode || failedByStderr,
-      result.exitCode === 1
-        ? 'expected exitCode 1'
-        : `expected exitCode 1 or error on stderr; got exitCode ${result.exitCode}, stderr: ${String(result.stderr).slice(0, 200)}`,
+      `expected exitCode 1 or error on stderr; got exitCode ${result.exitCode}, stderr: ${String(result.stderr).slice(0, 200)}`,
     ).toBe(true)

---

6-6: Global $.nothrow = false is already the default for zx.

zx throws on non-zero exit codes by default, so $.nothrow = false is a no-op. If you intend this as documentation of intent, a comment would be clearer; otherwise it can be removed.

.gitignore (2)

42-48: Broad glob patterns may have unintended side-effects.

*.spec.js and *.test.js (Lines 43-44) are root-level globs that match anywhere in the repo tree. If any package ever ships JS test fixtures or generated test files that should be tracked, they'll be silently ignored. Similarly, packages/*/src/**/*.js (Line 47) blanket-ignores all JS under package sources.

This is likely fine for a TypeScript-first repo, but worth noting the blast radius. Consider scoping these under specific directories if issues arise.

---

121-132: Personal workflow files in shared .gitignore.

Patterns like BEGINNER_*.md, PR-*-review.md, COMMIT_MSG.txt, .commit_msg*.txt, and pr-*-to-origin.md look like individual developer workflow artifacts. These are better placed in each developer's ~/.gitignore_global or .git/info/exclude to keep the shared .gitignore focused on project-level artifacts.

packages/likec4/src/cli/export/drawio/handler.ts (1)

205-214: Sequential per-view export keeps output deterministic — consider parallel export for large workspaces.

The sequential for loop is safe and gives predictable CLI output ordering. If throughput ever becomes an issue with many views, a bounded Promise.all could speed this up, but it's fine as-is for the current scope.

apps/playground/src/components/drawio/useDrawioContextMenuActions.ts (2)

240-272: Simplify conditional spread of onExportError.

Line 251 uses ...(onExportError != null && { onExportError }) — since CollectViewModelsOptions.onExportError is already optional (OnDrawioExportError | undefined), you can always pass it:

✏️ Suggested simplification

     const viewModels = await collectViewModelsForExportAll({
       viewIdsInModel,
       allViewModelsFromState,
       likec4model,
       viewStates,
       getLayoutedModel,
       layoutViews,
-      ...(onExportError != null && { onExportError }),
+      onExportError,
     })

---

200-206: useMemo for allViewModelsFromState depends on viewStates object reference.

If the parent re-creates the viewStates object on every render (e.g., from a state machine snapshot), this memo won't memoize effectively. Ensure the caller stabilizes viewStates via useMemo or a ref, or consider using a shallow-comparison hook if this becomes a performance bottleneck.

apps/playground/src/monaco/MonacoEditor.tsx (1)

80-86: Consider simplifying the conditional spread.

The conditional spread {...(setLayoutedModelApi != null && { setLayoutedModelApi })} works but is harder to read than just passing the prop directly. Since LanguageClientSync already types setLayoutedModelApi as optional, passing undefined is fine.

♻️ Simplified prop passing

       {wrapper && (
         <LanguageClientSync
           config={wrapperConfig}
           wrapper={wrapper}
-          {...(setLayoutedModelApi != null && { setLayoutedModelApi })}
+          setLayoutedModelApi={setLayoutedModelApi}
         />
       )}

apps/playground/src/monaco/LanguageClientSync.tsx (1)

112-144: Consider logging silenced errors in registerLayoutedModelApi for debuggability.

The empty catch blocks at Lines 123 and 136 silently discard errors from FetchLayoutedModel and LayoutView requests. While returning null/skipping is the right fallback, a logger.warn would help diagnose issues in the playground without surfacing them to users.

Optional: add debug-level logging

             try {
               const { model } = await c.sendRequest(FetchLayoutedModel.req, {})
               return model ?? null
-            } catch {
+            } catch (err) {
+              logger.warn(loggable(err))
               return null
             }

               try {
                 const res = await requestLayoutView(c, viewId as ViewId)
                 if (res.result?.diagram) out[viewId] = res.result.diagram
-              } catch {
+              } catch (err) {
+                logger.warn`layout view ${viewId} failed: ${loggable(err)}`
                 // skip failed view
               }

packages/generators/src/drawio/generate-drawio.spec.ts (2)

14-29: Duplicated expectDrawioXmlLoadableInDrawio helper.

This function duplicates the one in packages/likec4/src/drawio-test-utils.ts with slightly different regex patterns. The external version uses named constants (NESTED_ARRAY_IN_GEOMETRY, EDGE_GEOMETRY_SINGLE_ARRAY) while this uses inline regexes. If the validation logic evolves, they could drift apart.

Consider importing from a shared location or at least adding a comment noting the duplication.

---

71-73: vi.fn wrapper is unused for its mocking capabilities.

mockViewModel is wrapped in vi.fn but no spy assertions (e.g. toHaveBeenCalled, toHaveBeenCalledWith) are made on it. A plain function would be simpler.

e2e/tests/drawio-playground.spec.ts (1)

30-40: Inline import type for Download return type.

Minor nit: the inline Promise<import('@playwright/test').Download> could be a top-level type import for clarity, but it's functional as-is.

Proposed fix

-import type { Page } from '@playwright/test'
-import { expect, test } from '@playwright/test'
+import type { Download, Page } from '@playwright/test'
+import { expect, test } from '@playwright/test'

Then on line 34:

-): Promise<import('@playwright/test').Download> {
+): Promise<Download> {

packages/generators/src/drawio/generate-drawio.ts (3)

720-728: Duplicate html=1; in vertex style string.

vertexTextStyle for non-container nodes (line 723) already includes html=1;, and line 728 appends another html=1; at the end of the concatenated style. Draw.io silently deduplicates, so this is functionally harmless, but it adds unnecessary bytes to every vertex cell.

Suggested fix: remove trailing html=1;

   const styleStr =
-    `${vertexTextStyle}${shapeStyle}${colorStyle}${strokeWidthStyle}${containerDashed}${fillOpacityStyle}${navLinkStyle}${likec4Style}html=1;`
+    `${vertexTextStyle}${shapeStyle}${colorStyle}${strokeWidthStyle}${containerDashed}${fillOpacityStyle}${navLinkStyle}${likec4Style}`

Note: verify that the container branch (line 722) also includes html=1; or add it conditionally only when not already present.

---

188-193: getDefaultStrokeWidth ternary returns '' for non-container leaf nodes without explicit border.

return isContainer ? '1' : borderVal ? '1' : ''

When borderVal is undefined and isContainer is false, this returns '', which means no strokeWidth is set on the cell. Draw.io will then use its own default (typically 1). This seems intentional but the semantics are subtle — if the intent is "always 1 unless border=none", consider defaulting to '1' unconditionally (except for 'none').

---

1070-1078: Node sort comparator is not fully transitive.

The sort comparator checks a.id.startsWith(b.id + '.') but doesn't handle all transitive cases. For example, if a.id = "x.y.z" and b.id = "x.y", then a > b. But for two siblings like "x.y" and "x.z" (neither is a prefix of the other), the comparator returns 0. Combined with the parent checks, this generally works because Array.sort doesn't require strict total ordering for a valid grouping — but it could produce different orderings across JS engines for non-parent-related nodes. Since the exported XML is position-based (not order-dependent), this shouldn't cause correctness issues.

packages/generators/src/drawio/parse-drawio.ts (2)

420-444: Self-closing tag detection heuristic relies on last 10 chars.

Line 428: xml.slice(Math.max(tagStart, endOpen - 10), endOpen).trimEnd() checks the last 10 characters before > for a trailing / to detect self-closing tags like <mxCell ... />. This works for typical DrawIO XML but could theoretically fail if there's a very long attribute value ending with / right before > in a non-self-closing tag (e.g., <mxCell value="path/" >). In practice, DrawIO XML is well-formed enough that this is unlikely, but a more robust check would be xml[endOpen - 1] === '/' after trimming whitespace.

---

1707-1818: Round-trip comment parser is correct but has significant code duplication.

The parseDrawioRoundtripComments function has four nearly identical blocks for each comment section (layout, strokeColor, strokeWidth, waypoints) — each repeats the pattern: check start marker → accumulate lines → parse content → check end marker. This could be reduced with a small helper that handles the generic "parse block between markers" pattern.

Since the function works correctly and import is deferred, this is a good candidate for a future cleanup pass.

pnpm-workspace.yaml (1)

19-20: @types/pako is out of alphabetical order in the catalog.

The default catalog groups @-scoped packages first (lines 9–13), sorted alphabetically, then unscoped packages. @types/pako on line 20 should be placed between @types/node and @typescript/native-preview to stay consistent.

Suggested reordering

   '@types/node': ~22.19.10
+  '@types/pako': ^2.0.3
   '@typescript/native-preview': 7.0.0-dev.20260208.1
   ...
   oxlint-tsgolint: 0.11.5
   pako: ^2.1.0
-  '@types/pako': ^2.0.3
   playwright: 1.56.1

Based on learnings: "Configure workspace packages and maintain dependency catalog in pnpm-workspace.yaml for consistency."

.gitignore (2)

39-50: Lines 39-40 are now redundant with the broader glob on Line 49.

packages/*/src/**/*.js (Line 49) already covers both packages/log/src/*.js and packages/diagram/src/**/*.js. Consider removing the specific entries to reduce noise, or keep them if you want them as explicit documentation of known offenders.

Proposed cleanup

-packages/log/src/*.js
-packages/diagram/src/**/*.js
 packages/vscode-preview/src/generated-icons.tsx
 
 # Compiled artifacts (do not commit). Root-level *.spec.js/*.test.js and packages/*/src/**/*.js

---

123-134: Personal workflow files belong in ~/.gitignore_global, not the shared .gitignore.

The comment on Line 123 already acknowledges this. Patterns like PR-*-review.md, COMMIT_MSG.txt, .commit_msg*.txt, BEGINNER_*.md, etc. are developer-specific workflow artifacts. Moving them to each contributor's global gitignore keeps the shared file focused on project build/test artifacts.

e2e/src/likec4-cli-export-drawio.spec.ts (2)

40-42: Add cleanup before creating emptyWorkspaceDir for test isolation.

The skipped test above does rmSync before mkdirSync to ensure a clean slate. This test should do the same — if a previous run (or manual debugging) left stale files in emptyWorkspaceDir, the CLI might behave unexpectedly.

Proposed fix

   async ({ expect }) => {
+    rmSync(emptyWorkspaceDir, { recursive: true, force: true })
     mkdirSync(emptyWorkspaceDir, { recursive: true })

---

43-50: The dual exit-code check is pragmatic but may mask silent CLI regressions.

If the CLI is ever changed to exit 0 without printing an error, this test would incorrectly pass. Consider tightening the fallback condition once the CI tgz quirk is resolved, or adding a tracking comment/issue reference.

e2e/tests/drawio-playground.spec.ts (3)

56-62: exact: false on 'DrawIO' may over-match sibling menu items.

On Line 59, menu.getByText('DrawIO', { exact: false }) will match any item containing "DrawIO", including "Export to DrawIO" and "Export all" (if it were renamed to include "DrawIO"). This assertion doesn't specifically verify that a standalone "DrawIO" parent/header item exists — it just confirms at least one text node contains the substring.

If the intent is to verify a distinct "DrawIO" submenu header, consider using { exact: true } for that specific assertion, while keeping exact: false for the export items which are more uniquely named.

Proposed fix

-    await expect(menu.getByText('DrawIO', { exact: false })).toBeVisible()
+    await expect(menu.getByText('DrawIO', { exact: true })).toBeVisible()

---

64-72: Consider validating the downloaded filename more precisely.

The regex /\.drawio$/ on Line 66 only checks the file extension. A slightly stricter pattern could also confirm the filename contains meaningful characters (e.g., the view name) to catch regressions where the filename becomes empty or generic.

Example

-    expect(download.suggestedFilename()).toMatch(/\.drawio$/)
+    expect(download.suggestedFilename()).toMatch(/^.+\.drawio$/)

---

79-89: Consider reusing MENU_SELECTOR via the imported constant instead of re-calling page.locator directly.

Line 84 creates a locator with page.locator(MENU_SELECTOR).first() inline. This is functionally fine, but it duplicates the pattern used inside openDrawioContextMenu. Since the editor context menu is a different trigger surface, extracting a small helper or at least adding a brief comment would clarify that this is intentionally not the canvas context menu.

Minor nit — no functional issue.

scripts/ci-validate-in-docker.sh (1)

17-17: Consider --frozen-lockfile for reproducible CI validation.

pnpm install --prefer-offline without --frozen-lockfile allows lockfile mutations. For a CI validation script, --frozen-lockfile is the standard practice to catch lockfile drift.

Suggested change

-pnpm install --prefer-offline
+pnpm install --frozen-lockfile --prefer-offline

scripts/ci-validate-in-docker.ps1 (1)

17-17: Piping through bash loses script identity in error traces.

sed ... | bash means set -e failures will reference bash (stdin) rather than the script filename, making debugging harder. Consider writing to a temp file instead:

Alternative approach

-  bash -c "sed 's/\r$//' /workspace/scripts/ci-validate-in-docker.sh | bash"
+  bash -c "tmp=\$(mktemp) && sed 's/\r$//' /workspace/scripts/ci-validate-in-docker.sh > \$tmp && bash \$tmp"

apps/playground/src/components/drawio/useDrawioContextMenuActions.ts (2)

228-230: Consider logging layout-fetch failures in development mode.

The empty catch block silently discards the error. The fallback behavior (using the current diagram) is fine, but a DEV-only warning — like the one at Line 156 — would help during debugging.

♻️ Suggested change

         } catch {
-          // use current diagram if layout fetch fails
+          // use current diagram if layout fetch fails
+          if (import.meta.env.DEV) console.warn('DrawIO export: layout fetch failed for view, using current diagram')
         }

---

245-253: Unnecessary conditional spread for optional property.

onExportError is already optional in CollectViewModelsOptions, so ...(onExportError != null && { onExportError }) can be simplified to just onExportError. Passing undefined for an optional field is harmless and the current pattern adds cognitive overhead.

♻️ Suggested simplification

     const viewModels = await collectViewModelsForExportAll({
       viewIdsInModel,
       allViewModelsFromState,
       likec4model,
       viewStates,
       getLayoutedModel,
       layoutViews,
-      ...(onExportError != null && { onExportError }),
+      onExportError,
     })

packages/likec4/src/cli/export/drawio/handler.ts (2)

96-107: Symlinked subdirectories are silently skipped.

Dirent.isDirectory() returns false for symlinks pointing to directories — only isSymbolicLink() would match them. If a workspace uses symlinked source folders, those .c4/.likec4 files won't be picked up during round-trip reads. This is likely fine for most setups, but worth a note if symlinked workspaces are a supported pattern.

---

269-272: mkdir is called before the empty-views guard.

Line 256 creates the output directory, but Line 269 may throw if there are zero views — leaving behind an empty directory. Consider moving the mkdir call after the empty-views check, or at least after validating that there's work to do.

Suggested reorder

-  // 3) Prepare output dir and view list
-  await mkdir(args.outdir, { recursive: true })
   const viewmodels = [...model.views()]
 
+  // 4) Export: all-in-one file or one file per view
+  if (viewmodels.length === 0) {
+    logger.warn('No views to export')
+    throw new Error(ERR_NO_VIEWS_EXPORTED)
+  }
+
+  await mkdir(args.outdir, { recursive: true })
+
   const exportParams: ExportDrawioParams = {
     ...
   }
-
-  // 4) Export: all-in-one file or one file per view
-  if (viewmodels.length === 0) {
-    logger.warn('No views to export')
-    throw new Error(ERR_NO_VIEWS_EXPORTED)
-  }

apps/playground/src/monaco/LanguageClientSync.tsx (1)

357-367: Minor: redundant .catch on line 361.

requestLayoutViewCallback already wraps its body in try/catch (lines 148-159), so the outer .catch on line 361 will never trigger. Not harmful, but slightly misleading.

packages/generators/src/drawio/generate-drawio.spec.ts (1)

9-30: Consider extracting the shared validation helper to avoid duplication.

expectDrawioXmlLoadableInDrawio is duplicated from packages/likec4/src/drawio-test-utils.ts (as the comment on Line 13 notes). If the validation logic changes, both copies must be updated. Consider moving this into a shared test-utils module (e.g., under packages/generators/src/drawio/__test__/ or a common test-utils package) and importing from both test sites.

packages/generators/src/drawio/generate-drawio.ts (1)

1071-1079: Node sort comparator is not a strict total order.

The comparator returns 0 for most non-related node pairs, which means the sort outcome can vary across JS engines (sort stability differs). This is documented as acceptable on Line 1070 ("Order is not critical for correctness"), so the positional layout still works. Just noting for awareness — if deterministic output order ever becomes important, this comparator would need refinement.

packages/generators/src/drawio/parse-drawio.ts (2)

1700-1810: The TODO about repeated block-parsing pattern is worth addressing.

The four section parsers (layout, strokeColor, strokeWidth, waypoints) at Lines 1724–1807 share the same "find start marker → collect lines → find end marker" structure with only the parsing logic differing. A small helper like parseCommentBlock(lines, i, startMarker, endMarker, parseLine) would reduce ~80 lines of near-identical code.

That said, the current implementation is correct and the try/catch guards are properly placed. This can be deferred.

Sketch of a helper to reduce duplication

+/** Parse a comment block between start/end markers, calling parseLine for each "// " prefixed line. */
+function parseCommentBlock(
+  lines: string[],
+  startIndex: number,
+  endMarker: string,
+  parseLine: (content: string) => void,
+): number {
+  let i = startIndex + 1
+  while (i < lines.length && lines[i]?.trim() !== endMarker) {
+    const ln = lines[i]?.trim()
+    if (ln?.startsWith('// ')) parseLine(ln.slice(3))
+    i += 1
+  }
+  return i + 1
+}

Each section would then become a single parseCommentBlock(...) call with a specialized parseLine callback.

---

547-562: Consider using switch(true) for inferKind.

Per coding guidelines, switch(true) is preferred over if-else chains.

Proposed refactor

 function inferKind(
   style: string | undefined,
   parentCell?: DrawioCell,
 ): 'actor' | 'system' | 'container' | 'component' {
-  if (!style) return parentCell?.style?.toLowerCase().includes('container=1') ? 'component' : 'container'
   const s = style?.toLowerCase() ?? ''
-  if (s.includes('umlactor') || s.includes('shape=person')) return 'actor'
-  if (s.includes('swimlane')) return 'system'
-  if (s.includes('container=1')) return 'system'
-  if (parentCell?.style?.toLowerCase().includes('container=1')) return 'component'
-  return 'container'
+  switch (true) {
+    case !style:
+      return parentCell?.style?.toLowerCase().includes('container=1') ? 'component' : 'container'
+    case s.includes('umlactor') || s.includes('shape=person'):
+      return 'actor'
+    case s.includes('swimlane'):
+    case s.includes('container=1'):
+      return 'system'
+    case !!parentCell?.style?.toLowerCase().includes('container=1'):
+      return 'component'
+    default:
+      return 'container'
+  }
 }

As per coding guidelines, "Favor switch(true) over if-else chains".

packages/likec4/src/drawio-demo-export-import.spec.ts (1)

26-155: Consider sharing workspace loading across tests via beforeAll.

Each test independently calls LikeC4.fromWorkspace(CLOUD_SYSTEM_PATH, ...) and layoutedModel(), which is expensive (hence the 20s timeouts). Since all tests use the same workspace with the same options, hoisting the initialization into a beforeAll block would significantly reduce total test suite time.

Proposed refactor

 describe('DrawIO export/import with cloud-system demo', () => {
+  let drawioXml: string
+  let viewmodels: DrawioViewModelLike[]
+
+  beforeAll(async () => {
+    const likec4 = await LikeC4.fromWorkspace(CLOUD_SYSTEM_PATH, { throwIfInvalid: true })
+    const model = await likec4.layoutedModel()
+    viewmodels = [...model.views()]
+    drawioXml = generateDrawioMulti(viewmodels)
+  }, 30_000)
+
   it(
-    'exports cloud-system to DrawIO and file is loadable in draw.io (no nested Array)',
-    { timeout: 20000 },
-    async () => {
-      const likec4 = await LikeC4.fromWorkspace(CLOUD_SYSTEM_PATH, { throwIfInvalid: true })
-      expect(likec4.hasErrors()).toBe(false)
-      const model = await likec4.layoutedModel()
-      const viewmodels = [...model.views()]
-      expect(viewmodels.length).toBeGreaterThanOrEqual(1)
-      const drawioXml = generateDrawioMulti(viewmodels)
+    'exports cloud-system to DrawIO and file is loadable in draw.io',
+    () => {
       expect(drawioXml).toContain('<?xml version="1.0"')
       expect(drawioXml).toContain('<mxfile ')
       expectDrawioXmlLoadableInDrawio(drawioXml)
     },
   )
   // ... similar simplification for other tests

e2e/src/likec4-cli-export-drawio.spec.ts (1)

44-51: Fragile assertion: accepting exit code 0 as a valid failure outcome.

The fallback branch (failedByStderr) means this test will pass even if the CLI silently succeeds with exit code 0 but happens to log something matching "no LikeC4 sources found" as an informational message. Once the CLI quirk is fixed, consider tightening this to only accept exitCode === 1 so the test doesn't accidentally pass for the wrong reason.

apps/playground/src/components/drawio/useDrawioContextMenuActions.ts (2)

1-2: Consolidate value and type imports from @likec4/core.

Lines 1–2 import from the same module separately. These can be merged into a single import statement.

-import { LikeC4Styles } from '@likec4/core'
-import type { LayoutedLikeC4ModelData } from '@likec4/core'
+import { LikeC4Styles, type LayoutedLikeC4ModelData } from '@likec4/core'

---

249-253: Simplify conditional spread — optional field accepts undefined.

Since CollectViewModelsOptions.onExportError is optional, you can pass it directly without the conditional spread pattern.

-      ...(onExportError != null && { onExportError }),
+      onExportError,

apps/playground/src/monaco/LanguageClientSync.tsx (1)

112-145: Silent failure in layoutViews — consider surfacing partial failures to the caller.

Individual view layout failures are logged but not communicated to the caller. The returned Record will simply be missing entries for failed views, with no way for the consumer (e.g., Draw.io export) to distinguish "view doesn't exist" from "layout failed."

This is acceptable for a first pass, but consider returning a richer result type (e.g., { results: Record<string, DiagramView>; errors: string[] }) in a follow-up so the export UI can report which views failed.

packages/likec4/src/cli/export/drawio/handler.ts (2)

251-261: Inconsistent log level: error vs warn for equally fatal conditions.

Line 252 uses logger.error for an empty model, while line 259 uses logger.warn for no views — yet both throw and abort the export. Since both are hard failures from the user's perspective, consider using the same log level for consistency.

Suggested diff

   if (viewmodels.length === 0) {
-    logger.warn('No views to export')
+    logger.error('No views to export')
     throw new Error(ERR_NO_VIEWS_EXPORTED)
   }

---

137-145: Inconsistent view ID string conversion: String() vs as string.

Line 142 uses String(vm.$view.id) while line 188 uses vm.$view.id as string. For branded string types these produce the same result, but using a single idiom across the file reduces cognitive overhead and prevents divergence if the ID type ever changes.

packages/generators/src/drawio/generate-drawio.ts (2)

86-133: Consider narrowing ExtendedNode via module-level augmentation or a discriminated union rather than unchecked casts.

The accessor object (nodeOptionalFields) centralises the cast, which is good. However, the ExtendedNode type has no overlap guarantee with the actual Node — if a field is renamed upstream (e.g. navigateTo → navigate), these accessors will silently return undefined without a type error. A type predicate or a compile-time check (e.g. satisfies Partial<Node>) would catch drift earlier.

This isn't urgent since the fields are exercised by tests, but worth considering as the surface grows.

---

773-798: Non-null assertions on data.title!, data.titleCellId!, etc. are safe but fragile.

Lines 789–795 use ! because computeNodeCellExportData conditionally sets these fields only when isContainer is true (line 763–768), and the guard at line 785 ensures we only reach this branch for containers. The logic is correct today, but if the conditional spread is ever refactored, these assertions could become runtime errors.

A minor defensive approach:

Suggested improvement

-  const titleCellXml = buildContainerTitleCellXml(
-    data.title!,
-    data.titleCellId!,
-    data.navTo,
-    data.id,
-    data.fontFamily,
-    data.containerTitleFontSizePx!,
-    data.containerTitleColor!,
-  )
+  const titleCellXml = buildContainerTitleCellXml(
+    data.title ?? '',
+    data.titleCellId ?? data.id,
+    data.navTo,
+    data.id,
+    data.fontFamily,
+    data.containerTitleFontSizePx ?? 12,
+    data.containerTitleColor ?? CONTAINER_TITLE_COLOR,
+  )

packages/generators/src/drawio/parse-drawio.ts (1)

1710-1828: Round-trip comment parsing: the TODO about duplication is fair, but acceptable for now.

The four section parsers (layout, strokeColor, strokeWidth, waypoints) repeat the "find start marker → collect lines → find end marker" pattern. A small parseBlock(lines, i, startMarker, endMarker, lineParser) helper would reduce ~80 lines to ~20, but this is a nice-to-have.

packages/likec4/src/drawio-demo-export-import.spec.ts (1)

27-36: The conditional type for viewmodels is fragile — prefer an explicit type or Awaited<...>[].

Line 28–29 uses a conditional type extraction that resolves to never if the layoutedModel() return shape changes. A simpler approach:

Suggested simplification

-  let viewmodels: Awaited<ReturnType<LikeC4['layoutedModel']>> extends { views(): Iterable<infer V> } ? V[]
-    : never
+  let viewmodels: Array<DrawioViewModelLike>

Since generateDrawioMulti accepts Array<DrawioViewModelLike>, typing viewmodels to that interface directly would be more robust and self-documenting. You'd import DrawioViewModelLike from @likec4/generators.

packages/likec4/src/cli/export/drawio/handler.ts (1)

96-116: Consider using switch (true) for the dirent type checks.

Per coding guidelines, switch(true) is preferred over if-else chains.

♻️ Suggested refactor

     for (const e of entries) {
       const full = join(dir, e.name)
-      // Note: symlinks to directories are not followed (isDirectory() is false for symlinks).
-      if (e.isDirectory()) {
-        if (!ROUNDTRIP_IGNORED_DIRS.has(e.name)) await walk(full, depth + 1)
-      } else if (e.isSymbolicLink()) {
-        const st = await stat(full).catch(() => null)
-        if (st?.isFile() && isSourceFile(e.name)) {
-          const content = await readFile(full, 'utf-8').catch(err => {
-            if (logger?.debug) logger.debug(`${k.dim('Roundtrip:')} readFile failed`, { file: full, err })
-            return ''
-          })
-          if (content) chunks.push(content)
-        }
-      } else if (e.isFile() && isSourceFile(e.name)) {
-        const content = await readFile(full, 'utf-8').catch(err => {
-          if (logger?.debug) logger.debug(`${k.dim('Roundtrip:')} readFile failed`, { file: full, err })
-          return ''
-        })
-        if (content) chunks.push(content)
+      switch (true) {
+        case e.isDirectory():
+          if (!ROUNDTRIP_IGNORED_DIRS.has(e.name)) await walk(full, depth + 1)
+          break
+        case e.isSymbolicLink(): {
+          const st = await stat(full).catch(() => null)
+          if (st?.isFile() && isSourceFile(e.name)) {
+            const content = await readFile(full, 'utf-8').catch(err => {
+              if (logger?.debug) logger.debug(`${k.dim('Roundtrip:')} readFile failed`, { file: full, err })
+              return ''
+            })
+            if (content) chunks.push(content)
+          }
+          break
+        }
+        case e.isFile() && isSourceFile(e.name): {
+          const content = await readFile(full, 'utf-8').catch(err => {
+            if (logger?.debug) logger.debug(`${k.dim('Roundtrip:')} readFile failed`, { file: full, err })
+            return ''
+          })
+          if (content) chunks.push(content)
+          break
+        }
       }

As per coding guidelines: "Favor switch(true) over if-else chains".

apps/playground/src/monaco/LanguageClientSync.tsx (1)

128-144: Silent view layout failures may confuse users during export.

The comment on line 128 already acknowledges this, but worth noting: if a subset of views fail to layout, the caller receives a partial Record with no indication of which views were dropped or why. For a Draw.io "Export all" flow, this could produce an incomplete export without any user-visible feedback.

Consider at minimum returning the count of failed views or logging a summary after Promise.all resolves (e.g., logger.warn(\${failedCount}/${viewIds.length} views failed to layout`)`).

💡 Optional: Add a summary log after Promise.all

         await Promise.all(
           viewIds.map(async (viewId) => {
             try {
               const res = await requestLayoutView(c, viewId as ViewId)
               if (res.result?.diagram) out[viewId] = res.result.diagram
             } catch (err) {
               logger.warn(`layout view ${viewId} failed: ${loggable(err)}`)
             }
           }),
         )
+        const failedCount = viewIds.length - Object.keys(out).length
+        if (failedCount > 0) {
+          logger.warn(`${failedCount}/${viewIds.length} views failed to layout`)
+        }
         return out

packages/generators/src/drawio/generate-drawio.ts (2)

260-275: Casting hiContrast and line/labelBg to string assumes theme always provides these fields.

Lines 273, 280, 291–292 use as string casts on theme color values. If the theme contract changes and these become undefined, the cast silently passes "undefined" as a color hex, producing invalid Draw.io output.

Consider adding a fallback:

Safer color resolution

-    font: (elementColors.hiContrast ?? elementColors.stroke) as string,
+    font: String(elementColors.hiContrast ?? elementColors.stroke ?? DEFAULT_NODE_FONT_HEX),

Apply similarly to getEdgeStrokeColor (line 280) and getEdgeLabelColors (lines 291–292).

---

85-96: ExtendedNode relies on unchecked casts — consider a type predicate or runtime guard.

The JSDoc on line 85 even says "Consider type predicate or satisfies Partial to catch upstream field renames." If the upstream Node type renames navigateTo → navigatesTo (for example), all the accessors in nodeOptionalFields would silently return undefined with no compile-time error.

This is low-risk today but worth noting for future maintainability.

apps/playground/src/components/drawio/useDrawioContextMenuActions.ts (2)

241-263: No concurrent-export guard — double-clicks could trigger duplicate downloads.

If the user clicks "Export all views" twice rapidly, two collectViewModelsForExportAll calls run concurrently, potentially producing two file downloads. Consider adding a loading/exporting state to disable the button during export.

Sketch: add an `exporting` guard

+ const [exporting, setExporting] = useState(false)
+
  const handleExportAllViews = useCallback(async () => {
    close()
    if (!likec4model) return
+   if (exporting) return
+   setExporting(true)
+   try {
      // ...existing logic...
+   } finally {
+     setExporting(false)
+   }
  }, [
    close,
+   exporting,
    allViewModelsFromState,
    // ...
  ])

Then expose exporting from the hook for the UI to disable the button.

---

200-207: allViewModelsFromState memo may recompute on every render if viewStates isn't stabilized.

The comment on line 200 warns callers to stabilize viewStates, but if they don't, this useMemo re-runs on every render because viewStates is a Record (likely a new object reference each time). Consider adding a more robust check (e.g., memoizing the filtered/mapped result based on a serialized key or using a ref comparison).

packages/generators/src/drawio/parse-drawio.ts (3)

1709-1817: Consider extracting a shared "parse block between markers" helper.

The TODO on lines 1712–1713 is valid. The four section parsers (layout, strokeColor, strokeWidth, waypoints) each repeat the same pattern: find start marker → collect lines → parse content → find end marker. This adds ~80 lines that could be reduced to ~20 with a helper like:

function parseBlock(
  lines: string[], start: number, endMarker: string, 
  processLine: (rest: string) => void,
): number { ... }

Since import is deferred and this code isn't yet actively exercised, this is a good candidate for the follow-up import PR.

Would you like me to generate the refactored helper, or open an issue to track this for the import PR?

---

1636-1657: Redundant XML parsing when diagrams.length === 1.

Line 1649 delegates to parseDrawioToLikeC4(xml), which internally calls getFirstDiagram(xml) → getAllDiagrams(xml) again, re-parsing the full XML to find diagrams that were already extracted on line 1637. For very large files this is wasteful.

Sketch: pass already-extracted diagram content directly

- if (diagrams.length === 1) return parseDrawioToLikeC4(xml)
+ if (diagrams.length === 1) {
+   const d = diagrams[0]!
+   const cells = parseDrawioXml(d.content)
+   const state = buildSingleDiagramState(cells, d.name)
+   return emitLikeC4SourceFromSingleState(state)
+ }

---

182-195: parseStyle silently discards entries with empty values — verify this is intended.

Line 191: if (k && v) map.set(...) means a style entry like dashed=; (empty value) is silently dropped. If Draw.io ever emits a key with an intentionally empty value to indicate "unset" or "default", it won't be captured. This is likely fine for current usage since meaningful style values are always non-empty, but worth documenting.