feat: establish generated v2 foundation#216
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
328004e to
3abbd8d
Compare
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
3abbd8d to
8bd8ace
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328004e708
ℹ️ About Codex in GitHub
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".
| "executor": "nx:run-commands", | ||
| "dependsOn": ["build"], | ||
| "options": { | ||
| "command": "wrangler deploy --config dist/server/wrangler.json" |
There was a problem hiding this comment.
When nx deploy web runs after the configured Vite build, this path is never produced: the web build target declares .output as its output and the only checked-in Wrangler config is apps/web/wrangler.jsonc (the new CI dry-run repeats the same dist/server/wrangler.json path). Cloudflare's current TanStack Start guide shows the generated Worker entry under .output/server/index.mjs and deploys with wrangler deploy, so this command will fail before it can deploy or validate the Worker.
Useful? React with 👍 / 👎.
| status, | ||
| }: GenerationWorkspaceProps) { | ||
| const validation = validateIntermediateDiagram(diagram); | ||
| const scene = renderIntermediateDiagram(diagram); |
There was a problem hiding this comment.
For an invalid intermediate diagram that still satisfies the TypeScript shape, such as an edge whose toId is missing from nodes, validateIntermediateDiagram would populate validation.issues, but this call immediately goes through renderIntermediateDiagram/parseIntermediateDiagram and throws before the error branch in the JSX can render. That makes the workspace crash instead of displaying the validation errors or status="error" UI for bad generated IR.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
2 times, most recently
from
b8a3b31 to
a1bbbac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1bbbacd7c
ℹ️ About Codex in GitHub
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".
| {arrows.map((arrow) => ( | ||
| <line | ||
| key={arrow.id} | ||
| markerEnd="url(#sketchi-arrow)" | ||
| stroke={arrow.strokeColor} |
There was a problem hiding this comment.
For diagrams with edge.label values (the bundled architecture and flowchart fixtures both set relationship labels), the renderer preserves that text on ArrowSceneElement, but this preview loop only emits a <line>. The actual SVG therefore drops the relationship labels even though the IR and rendered scene carried them through.
Useful? React with 👍 / 👎.
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| }, | ||
| "./styles.css": "./dist/style.css" |
There was a problem hiding this comment.
This subpath points consumers at ./dist/style.css, but the UI package build only uses the Vite library entry src/index.ts (which does not import src/styles.css) plus declaration emission, so that CSS asset is not emitted or copied. Any consumer importing @sketchi/diagram-studio-ui/styles.css from the built package, rather than through the web app's source alias, will resolve to a missing file.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
a1bbbac to
9765eaf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9765eafd77
ℹ️ About Codex in GitHub
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".
| name: "SketchiDiagramStudioUi", | ||
| }, | ||
| rollupOptions: { | ||
| external: ["react", "react-dom"], |
There was a problem hiding this comment.
When nx build diagram-studio-ui runs in a clean checkout or before dependency builds in run-many, this Rollup config only externalizes React, so Vite tries to resolve @sketchi/diagram-core and @sketchi/diagram-renderer through their package exports. Those exports point at ./dist/index.js, which does not exist until those packages are built, and this target has no dependsOn for those builds, so the UI build can fail or bundle stale dist artifacts instead of the current source.
Useful? React with 👍 / 👎.
| resolve: { | ||
| tsconfigPaths: true, | ||
| }, |
There was a problem hiding this comment.
When nx test web runs from a clean checkout, the route test imports workspace packages, but resolve.tsconfigPaths is not a Vitest/Vite resolver plugin and this config does not mirror the aliases from apps/web/vite.config.ts. The resolver therefore falls back to the packages' exports entries pointing at dist/..., which the test target does not build first, so the web tests fail unless local dist artifacts happen to be present.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
2 times, most recently
from
52bd7e5 to
e66127b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e66127b394
ℹ️ About Codex in GitHub
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".
| cd "$ROOT" | ||
|
|
||
| bun x ultracite check packages/opencode-excalidraw | ||
| bun run ultracite check packages/opencode-excalidraw |
There was a problem hiding this comment.
This task now invokes bun run ultracite, but bun run --help describes it as running a file or package.json script, and packages/opencode-excalidraw/package.json has no ultracite script (only build/smoke/test/typecheck/test). In environments that execute this mise lint task, it exits with Script not found "ultracite" instead of running the installed Ultracite binary; keep using bun x or add a script.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
e66127b to
c7bdce8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7bdce8b31
ℹ️ About Codex in GitHub
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".
| fromId: edge.fromId, | ||
| height: Math.abs(toCenter.y - fromCenter.y) || 1, | ||
| id: edge.id ?? `edge_${index}`, | ||
| points: [fromCenter, toCenter], |
There was a problem hiding this comment.
For any rendered diagram with edges, this stores each arrow as a center-to-center segment, and the SVG preview renders rectangles after arrows. The marker is placed at the destination node's center, so the target rectangle is painted over the arrowhead and users see relationship lines without visible direction; compute endpoints on the node boundary or render/clip arrows so the marker stays outside the shape.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
c7bdce8 to
ebd9b95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd9b9558a
ℹ️ About Codex in GitHub
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".
| backgroundColor: "transparent", | ||
| fromId: edge.fromId, | ||
| height: Math.abs(toCenter.y - fromCenter.y) || 1, | ||
| id: edge.id ?? `edge_${index}`, |
There was a problem hiding this comment.
For a valid diagram where one edge explicitly uses an ID such as edge_1 and a later edge omits id, this fallback assigns both rendered arrows the same scene ID. The validator only checks duplicates among explicit edge.id values, so this can produce duplicate React keys in DiagramPreview and duplicate scene element IDs for downstream consumers; include a prefix/namespace that cannot collide or track generated IDs against explicit ones.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
ebd9b95 to
03d7261
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03d7261a6f
ℹ️ About Codex in GitHub
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".
| export const GraphStyleSchema = z | ||
| .object({ | ||
| arrowStroke: z.string().optional(), | ||
| fontFamily: z.number().optional(), |
There was a problem hiding this comment.
When generated IR includes a normal CSS font family such as graphOptions.style.fontFamily: "Inter", parseIntermediateDiagram rejects the diagram because this schema requires a number. Font family names are string values, so otherwise valid style hints from diagram generators/fixtures fail validation before they can render.
Useful? React with 👍 / 👎.
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
03d7261 to
2498bcd
Compare
anand-testcompare
force-pushed
the
feat/v2-clean-start-foundation
branch
from
85e832b to
02fa2e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02fa2e202c
ℹ️ About Codex in GitHub
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".
| if ( | ||
| (input.call.name === "diagram_tweak" || | ||
| input.call.name === "diagram_restructure") && | ||
| !sessionId |
There was a problem hiding this comment.
When an MCP client follows the shared tool schema and calls diagram_tweak or diagram_restructure with shareUrl or excalidrawPath, this branch still returns an error because only sessionId or inline excalidraw can populate sessionId. The server exposes those source-scene fields via the catalog, so the generic stdio executor currently advertises valid edit inputs that it cannot execute for share-link or file-based diagrams.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd936d8254
ℹ️ About Codex in GitHub
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".
| ); | ||
| } | ||
|
|
||
| return formatToolResult(result); |
There was a problem hiding this comment.
When an MCP client calls diagram_from_prompt, diagram_tweak, or diagram_restructure with the advertised outputPath/render options, this path returns the raw thread-run response and never invokes exportDiagramToPng, so no pngPath is written or returned even though the tool descriptions/schema promise an optional PNG for these thread-backed tools. This affects clients that rely on the generic MCP server rather than the OpenCode plugin, where the same flows do render after receiving a share link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Pass Excalidraw files into PNG export
When exporting a scene that contains image elements, the .excalidraw file's files map is required for exportToBlob to render those images, but the harness always passes files: null. Since readExcalidrawFile preserves files and exportDiagramToPng advertises rendering from full Excalidraw JSON/files, PNGs for diagrams with embedded images will render missing content even though the input scene is valid.
ℹ️ About Codex in GitHub
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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 992add1cfe
ℹ️ About Codex in GitHub
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".
| "@sketchi/diagram-agent-tools": "workspace:*", | ||
| "@sketchi/diagram-exporter": "workspace:*", |
There was a problem hiding this comment.
When @sketchi-app/opencode-excalidraw is installed from npm, only dependencies/peers are installed, but this change adds runtime imports from @sketchi/diagram-agent-tools and @sketchi/diagram-exporter while keeping them in devDependencies (and both workspace packages are private). The local workspace typecheck/build can resolve them, but the published plugin's dist/index.js will fail to load for users with module-not-found errors unless these imports are bundled or the packages are publishable runtime dependencies.
Useful? React with 👍 / 👎.
This reverts commit 992add1.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ecee2d650
ℹ️ About Codex in GitHub
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".
| @@ -0,0 +1,6 @@ | |||
| export * from "./diagram-catalog"; | |||
There was a problem hiding this comment.
When @sketchi/diagram-core is built with the current tsc target and then consumed through its package export in Node ESM, these extensionless re-exports are emitted unchanged under a type: "module" package. Node does not add .js for relative ESM specifiers, so importing the built package fails before reaching the catalog (for example resolving dist/diagram-catalog instead of dist/diagram-catalog.js); add .js specifiers in the source or switch this package to a bundling/build mode that rewrites them.
Useful? React with 👍 / 👎.
| const excalidrawInputSchema = { | ||
| type: "object", | ||
| description: "Excalidraw JSON blob.", | ||
| additionalProperties: false, |
There was a problem hiding this comment.
When an MCP client validates diagram_tweak, diagram_restructure, diagram_to_png, or diagram_grade input against this schema for an Excalidraw scene containing images, the required top-level files map is rejected because additionalProperties is false and only elements/appState are declared. The executor paths already accept and forward files, so the published schema blocks valid Excalidraw JSON blobs or causes image assets to be stripped by schema-aware clients.
Useful? React with 👍 / 👎.
1 participant
Summary
diagram-corethe canonical IntermediateFormat schema boundary, with the backend reusing that shared schema instead of carrying its own duplicate zod contractdiagram-agent-toolsas the host-neutral diagram tool catalog with MCP-shaped JSON schemas, descriptions, routing hints, and session-awarediagram_to_pnginputdiagram-exporteras the host-neutral Excalidraw share/file parsing, safe output path, and Playwright-backed PNG export packagesketchi-diagramagent config/hints and standalonediagram_to_pngflow to shared packages instead of duplicating tool names, Mermaid guardrails, and PNG export logic@sketchi/mcp-serveras a generic NodeNext MCP stdio adapter over the shared catalog, with a default HTTP executor for Sketchi server-backed generation/edit APIs plus shared PNG exporttest-storybooktarget that verifies indexed static stories?diagram=controls the selected diagram and catalog clicks update the TanStack Router search stateDiagramStatusStripStudio component and a structure guard requiring component file, test, Storybook story, local export, and package exportrun-many -t typecheck,test,buildruns do not race against missingdist/typecheckdeclarationsVerification
pnpm install --frozen-lockfilegit diff --checkbun x ultracite fixNX_DAEMON=false pnpm nx test diagram-exporter --skip-nx-cache(4 files, 14 tests)NX_DAEMON=false pnpm nx typecheck diagram-exporter --skip-nx-cacheNX_DAEMON=false pnpm nx build diagram-exporter --skip-nx-cacheNX_DAEMON=false pnpm nx run diagram-exporter:typecheck-tsgo --skip-nx-cacheNX_DAEMON=false pnpm nx test diagram-agent-tools --skip-nx-cache(1 file, 4 tests)NX_DAEMON=false pnpm nx test mcp-server --skip-nx-cache(2 files, 9 tests)NX_DAEMON=false pnpm nx typecheck mcp-server --skip-nx-cacheNX_DAEMON=false pnpm nx build mcp-server --skip-nx-cacheNX_DAEMON=false pnpm nx run mcp-server:typecheck-tsgo --skip-nx-cachetimeout 2 node packages/mcp-server/dist/stdio.js </dev/nullcd packages/opencode-excalidraw && bun test(32 tests)cd packages/opencode-excalidraw && bun x tsc -p tsconfig.ci.json --noEmit && bun run buildNX_DAEMON=false pnpm nx test web --skip-nx-cache(1 file, 2 tests)NX_DAEMON=false pnpm nx typecheck web --skip-nx-cacheNX_DAEMON=false pnpm nx build web --skip-nx-cacheNX_DAEMON=false pnpm nx run web:typecheck-tsgo --skip-nx-cacherm -rf packages/diagram-core/dist packages/diagram-renderer/dist packages/diagram-studio-ui/dist packages/diagram-agent-tools/dist packages/diagram-exporter/dist packages/mcp-server/dist apps/web/dist tools/sketchi-generators/dist && NX_DAEMON=false pnpm nx run-many -t typecheck,test,build --skip-nx-cache(10 projects)NX_DAEMON=false pnpm nx run-many -t typecheck-tsgo --skip-nx-cache(7 projects)NX_DAEMON=false pnpm nx test-storybook diagram-studio-ui --skip-nx-cacheverified 7 source story files across 11 static Storybook storiespnpm exec wrangler deploy --dry-run --config dist/server/wrangler.jsonNX_DAEMON=false pnpm nx graph --affected --file=.memory/nx-affected-diagram-exporter.htmlNX_DAEMON=false pnpm nx graph --affected --file=.memory/nx-affected-router-search.htmlpnpm devreached Vite ready onhttp://127.0.0.1:5174/;agent-browsersmoke opened/?diagram=flowchart, saw the flowchart prompt, clicked Architecture, and verified the URL changed to/?diagram=architectureNotes
pnpm devfailure on oldmainhappens because that branch stores catalog entries underpackage.json > workspaces.catalog; pnpm expects catalogs inpnpm-workspace.yaml. This v2 branch has the pnpm workspace file andpnpm devstarts cleanly.diagram_graderemains host-owned because it depends on the OpenCode LLM client;diagram_to_pngis now available in the generic MCP default executor through@sketchi/diagram-exporter.dist; this caught and fixed extensionless ESM imports before push.2ecee2d; that work is moving to/home/anandpant/Development/shpitdev/sketchi-v2-lab.2ecee2d.