Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

packages/language-server/package.json (1)

115-115: Consider simplifying with fs.rmSync.

The inline script works but is quite dense. A simpler cross-platform alternative that also handles nested contents:

♻️ Suggested simplification

-    "pregenerate": "node -e \"const fs=require('fs');const p=require('path');const d='src/generated';if(fs.existsSync(d)){fs.readdirSync(d).forEach(f=>{try{fs.unlinkSync(p.join(d,f))}catch(e){}})}\"",
+    "pregenerate": "node -e \"const fs=require('fs');const d='src/generated';if(fs.existsSync(d)){fs.rmSync(d,{recursive:true,force:true});fs.mkdirSync(d,{recursive:true})}\"",

This handles subdirectories too (current unlinkSync silently fails on directories), is shorter, and achieves the same cross-platform goal. fs.rmSync with recursive + force is available since Node 14.14.

packages/diagram/vite.config.ts (1)

126-129: execSync will throw on non-zero exit — consider wrapping with try/catch for consistency.

The err guard on line 121 gracefully skips on failure, but if pnpm panda ship itself exits non-zero, the unhandled exception will crash the build. If a graceful skip is intended here too, wrap in try/catch. Also, cwd: process.cwd() is the default for execSync and can be dropped.

♻️ Suggested fix

       this.info('shipping panda')
-      execSync('pnpm panda ship --outfile ./panda.buildinfo.json', {
-        stdio: 'inherit',
-        cwd: process.cwd(),
-      })
+      try {
+        execSync('pnpm panda ship --outfile ./panda.buildinfo.json', {
+          stdio: 'inherit',
+        })
+      } catch (e) {
+        this.warn('panda ship failed, skipped')
+      }

packages/icons/scripts/generate.mjs (1)

187-199: Consider reusing indexFiles at line 218 to avoid a duplicate crawl and ensure consistent filtering.

Lines 187–192 crawl **/index.ts with a node_modules filter, but line 218 performs the same crawl again without the filter. You could reuse indexFiles in the for loop at line 218 to avoid the redundant filesystem crawl and ensure node_modules paths are consistently excluded.

♻️ Proposed refactor

-for (const fname of new fdir().glob('**/index.ts').withBasePath().crawl().sync()) {
+for (const fname of indexFiles) {

apps/playground/src/state/playground-machine.ts (1)

394-409: Persist-before-assign means the newly added file isn't immediately saved.

The 'persist to storage' action runs before assign, so it writes the previous context (without the new file). If the user closes the tab before another persist trigger fires, the added file is lost. This matches the existing workspace.changeActiveFile pattern (Line 386), so it's consistent — but worth noting.

Consider swapping the order or adding a second persist after the assign if reliability of the import flow matters:

Suggested reorder

         'workspace.addFile': {
           actions: [
-            'persist to storage',
             assign({
               files: ({ event, context }) => ({
                 ...context.files,
                 [event.filename]: event.content,
               }),
               originalFiles: ({ event, context }) => ({
                 ...context.originalFiles,
                 [event.filename]: event.content,
               }),
               activeFilename: ({ event }) => event.filename,
             }),
+            'persist to storage',
           ],
         },

apps/playground/src/routes/w.$workspaceId/route.tsx (1)

2-3: Merge imports from the same module.

Both imports come from $components/drawio/DrawioContextMenuProvider and can be combined into a single import statement.

♻️ Proposed fix

-import { DrawioContextMenuProvider } from '$components/drawio/DrawioContextMenuProvider'
-import { useDrawioContextMenu } from '$components/drawio/DrawioContextMenuProvider'
+import { DrawioContextMenuProvider, useDrawioContextMenu } from '$components/drawio/DrawioContextMenuProvider'

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

49-55: Consider adding a test for truly invalid XML input.

The "empty XML" test covers a minimal but structurally unusual input (no <diagram>/<mxGraphModel> wrapper). It would be useful to also test behavior when given completely invalid XML (e.g. "not xml at all") to document whether parseDrawioToLikeC4 throws or returns a fallback.

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

48-52: mockViewModel wraps vi.fn but the mock is never asserted on.

Since you never check call counts or arguments, a plain function would be cleaner. Using vi.fn here implies the mock is tracked for assertions, which could confuse future readers.

♻️ Suggested simplification

-const mockViewModel = vi.fn(function ($view: ProcessedView<aux.Unknown>) {
-  return {
-    $view,
-  } as unknown as LikeC4ViewModel<aux.Unknown, LayoutedView<aux.Unknown>>
-})
+function mockViewModel($view: ProcessedView<aux.Unknown>) {
+  return {
+    $view,
+  } as unknown as LikeC4ViewModel<aux.Unknown, LayoutedView<aux.Unknown>>
+}

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

28-32: Static analysis: regex built from variable input (ReDoS surface).

While name is always a hardcoded string from internal callers, the RegExp constructor with interpolated input is flagged by ast-grep as a potential ReDoS vector (CWE-1333). Since the callers are all internal and pass safe attribute names, the actual risk is negligible, but escaping the input would be a defensive improvement.

♻️ Defensive fix

 function getAttr(attrs: string, name: string): string | undefined {
-  const re = new RegExp(`${name}="([^"]*)"`, 'i')
+  const escapedName = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+  const re = new RegExp(`${escapedName}="([^"]*)"`, 'i')
   const m = attrs.match(re)
   return m ? m[1] : undefined
 }

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

121-129: Non-transitive sort comparator.

The comparator returns 0 for many unrelated pairs, which means the sort result may vary across engines. For parent-before-child ordering, consider a topological sort or at minimum ensure transitivity. In practice this likely works because the input is already partially ordered, but it's fragile.

apps/playground/src/components/drawio/DrawioContextMenu.tsx (1)

76-82: Fragile viewmodel shim with type assertion.

The ad-hoc object { $view: diagram, get $styles() { ... } } cast via as Parameters<typeof generateDrawio>[0] will silently break if generateDrawio's signature evolves (e.g., if it starts accessing .bounds or other LikeC4ViewModel members). Consider creating a shared helper or factory function that produces a properly typed viewmodel shim, reusable across both this component and the Provider.

apps/playground/src/components/drawio/DrawioContextMenuProvider.tsx (1)

121-130: Potential stale closure in window event handlers.

handleImport and handleExport are in the dependency array, so the effect re-subscribes when they change. However, handleExport captures diagram and likec4model from the snapshot — if these change frequently, the event listeners will be torn down and re-added on each render cycle. This is functionally correct but creates some churn. A useRef-based approach for the handlers would avoid repeated listener registration.