Embed Pencil as agentic app with live editor bridge#270
Merged
Conversation
Adds packages/pencil — a Deus AAP that embeds the Pencil editor in an iframe and bridges the bundled @pencil.dev/cli MCP binary to the live canvas. Agents call the binary's full tool surface (batch_design, get_editor_state, snapshot_layout, get_screenshot, …) and every op renders in real time on the user's canvas. Architecture: WebSocket TransportServer accepts the bundled mcp-server binary; SSE + postMessage bridge forwards tool requests to the editor's webappapi IPC handlers. Editor bundle auto-downloads from api.pencil.dev on first launch and caches at ~/.deus/pencil-editor/<version>/. Surface: 4 workspace tools (pencil_list_designs, pencil_get_active, pencil_open, pencil_new) + 13 native editor tools forwarded through. No CLI subprocesses for design work — every action is visible. UI: vanilla TS chrome with Linear-style file switcher, design tokens matched to Deus's dark theme, and a "+ New design" affordance that opens a blank canvas via the binary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Pencil agentic app and integrates it end-to-end: agent manifest and packaging, esbuild build and package metadata, TypeScript runtime modules, a bundled MCP binary proxy, HTTP/MCP router and WebSocket transport, iframe↔host RPC and IPC, CLI integration, editor bundle download and rewrite, filesystem design management, frontend UI (HTML/CSS/JS), backend installed-app prefetch, packaging prune hook, and tests. ChangesPencil AAP Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (4)
packages/pencil/src/lib/transport-server.ts (1)
48-64: ⚡ Quick winUse
ts-patternto decode the transport frame union.This is the protocol discriminated union for the file, but
handleFrame()still narrows it with manual property checks and casts. ConvertingFrameto an explicit union and matching withts-patternplus.exhaustive()would make new frame variants compile-time visible instead of runtime fallthroughs. As per coding guidelines, "Preferts-patternfor discriminated unions. Use.exhaustive()for exhaustiveness checking."Also applies to: 179-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/lib/transport-server.ts` around lines 48 - 64, The Frame union (FlatToolRequest, WrappedToolRequest, and generic frame) is being narrowed with manual checks in handleFrame; replace that with a ts-pattern match to make variants explicit and compile-time checked: adjust the union types to include a clear discriminant (use type: "tool_request" for WrappedToolRequest and e.g. type?: undefined or "flat" for FlatToolRequest) or create an explicit discriminant union, then in handleFrame use match(frame).with(...).run(...) branches for each variant and append .exhaustive() to ensure new variants fail at compile time; apply the same refactor where similar manual narrowing occurs around the 179-190 region so all frame handling uses ts-pattern matching and exhaustive checks (reference symbols: FlatToolRequest, WrappedToolRequest, Frame, handleFrame, ts-pattern, .exhaustive()).packages/pencil/src/lib/designs.ts (1)
104-108: 💤 Low valuePath splitting uses hardcoded
/separator.Line 107 uses
.split("/")which may not work correctly on Windows where paths use backslashes. Consider usingpath.basename()instead.♻️ Proposed fix
+import { basename } from "node:path"; const hash = createHash("sha1").update(penPath).digest("hex").slice(0, 16); -const baseName = - penPath - .split("/") - .pop() - ?.replace(/\.pen$/, "") ?? "design"; +const baseName = basename(penPath, ".pen") || "design";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/lib/designs.ts` around lines 104 - 108, The code that derives baseName from penPath uses a hardcoded split("/") which fails on Windows; update the logic in the baseName computation to use Node's path utilities (import path) and call path.basename on penPath (and strip the ".pen" extension as before) instead of split/pop/replace so it works cross-platform; modify the expression that sets baseName (referencing the baseName variable and penPath) accordingly and ensure the path module is imported.packages/pencil/src/lib/ipc-host.ts (1)
182-187: 💤 Low valueConsider using
getActivePendirectly instead of transforming preview path.Lines 182-187 call
getActivePreview()then strip.preview.pngto get the.penpath. SincegetActivePen()already returns the.penpath directly, this introduces unnecessary indirection and potential inconsistency.♻️ Proposed fix
"get-resource-path": (_payload, ctx) => { - const active = designs.getActivePreview(ctx.storage); - if (!active) return null; - // active is the .preview.png; the .pen file is the sibling. - return active.replace(/\.preview\.png$/, ".pen"); + return designs.getActivePen(ctx.storage); },Apply similar refactoring to
save-resourceandinitializedhandlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/lib/ipc-host.ts` around lines 182 - 187, Replace the transformation of a preview filename with a direct call to the function that already returns the .pen path: in the "get-resource-path" IPC handler, stop calling designs.getActivePreview(ctx.storage) and stripping ".preview.png" and instead call designs.getActivePen(ctx.storage) and return its result; also apply the same refactor to the "save-resource" and "initialized" handlers so they use designs.getActivePen(ctx.storage) consistently rather than deriving the .pen path from the preview path.packages/pencil/src/ui/parent.html (1)
18-39: ⚡ Quick winAdd
aria-labelledbyto the switcher menu for better accessibility.The switcher menu (
role="listbox") should reference the trigger button as its label so screen readers announce context when the menu opens.♿ Proposed fix
-<div class="switcher-menu" id="switcher-menu" role="listbox" hidden></div> +<div class="switcher-menu" id="switcher-menu" role="listbox" aria-labelledby="switcher-trigger" hidden></div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/parent.html` around lines 18 - 39, Add an accessible label reference to the listbox by setting the switcher menu's aria-labelledby to the trigger's id: update the element with id "switcher-menu" (role="listbox") to include aria-labelledby="switcher-trigger" so screen readers announce the "switcher-trigger" button (whose id is "switcher-trigger") as the label for the menu; ensure the referenced id exists ("switcher-trigger") and remains unique.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pencil/skills/pencil/SKILL.md`:
- Around line 37-74: The fenced code examples in SKILL.md (the three blocks that
start with the numbered list beginning "1. get_guidelines(\"general\") ..." and
the two pattern examples "New design from scratch:" and "Edit an existing
design:" / "Switch context:") are missing language identifiers and trigger
markdownlint; add the language identifier "text" to each opening triple-backtick
so they read ```text, preserving the existing content exactly inside the fences
to satisfy the linter.
In `@packages/pencil/src/lib/cli.ts`:
- Around line 129-145: verifyCliKey currently ignores its _key parameter; either
use the provided key when invoking the CLI or rename the function to indicate it
only verifies a persisted key. To fix, update verifyCliKey to inject the given
key into the subprocess environment (use buildCliEnv or the spawnCli env
argument so that spawnCli(["status"], ctx, { env: <env-with-key> }) uses the
passed key) and remove the leading underscore from the parameter, or if intended
to verify a persisted key, rename verifyCliKey to verifyPersistedKey and
document the caller must persist the key first; reference verifyCliKey,
buildCliEnv, and spawnCli to locate the change.
- Around line 218-224: The custom stripAnsi function (stripAnsi) in
packages/pencil/src/lib/cli.ts is incomplete; replace it with the well-tested
strip-ansi package: add strip-ansi as a dependency, import the package in cli.ts
(using the appropriate ES/CJS import style for the repo), and change the
stripAnsi implementation to simply return the result of calling the strip-ansi
function on the input string. Ensure TypeScript typings work (install
`@types/strip-ansi` if needed) and remove the old regex code.
In `@packages/pencil/src/lib/designs.ts`:
- Around line 180-187: findWorkspaceDesigns currently builds preview paths as a
simple sibling (.preview.png) which can be wrong for workspace pens because
previewPathForPen uses content-addressed cache for files outside storage; update
findWorkspaceDesigns to derive preview via the same logic by either accepting a
storage parameter and calling previewPathForPen(full, storage) or refactoring
previewPathForPen into a helper usable by findWorkspaceDesigns, then set the
preview field to that returned path and adjust previewExists by checking that
computed path instead of the sibling path.
In `@packages/pencil/src/lib/editor-bundle.ts`:
- Around line 185-196: The rewriteEditorIndex function must fail-fast if the
injection pattern isn't found: after computing the replacement (using the
existing inject string and html.replace call in rewriteEditorIndex), compare the
result to the original html; if they are identical (no injection occurred) throw
an Error with a clear message (e.g., "Pencil editor injection failed: module
script pattern not found") so consumers notice the broken IPC bridge early. Keep
the existing inject content and only add the check & throw logic around the
result of the replace operation in rewriteEditorIndex.
In `@packages/pencil/src/lib/iframe-rpc.ts`:
- Around line 31-59: The current requestFromIframe broadcasts requests via
broadcastEvent and leaves pending entries global, causing races across multiple
panels and long waits when none are connected; change requestFromIframe to
select a single connected panel/session (e.g., via an existing panels/clients
registry or getConnectedPanel() helper), fail immediately with a clear error if
no panel is available, send the request only to that panel (include
sessionId/targetId in the event or use a targeted send API instead of
broadcastEvent), and store the sessionId/targetId on the pending entry
(PendingRequest) so replies can be matched only to that session in the message
handler (keep the same id generation, timeout handling, and error on timeout).
In `@packages/pencil/src/lib/ipc-host.ts`:
- Around line 217-225: The "save-resource" IPC handler in ipc-host.ts extracts
payload.content without validating its type; add a runtime check in the
"save-resource" handler to ensure payload.content is a string (e.g., if (typeof
content !== "string") throw new TypeError(...)) before computing penPath and
calling fs.promises.writeFile, so malformed payloads are rejected rather than
written; keep the existing logic using designs.getActivePreview(ctx.storage) and
ensure the thrown error provides clear context about the invalid payload.
In `@packages/pencil/src/lib/mcp-binary.ts`:
- Around line 126-131: The current startup loop in mcp-binary.ts returns {
httpPort } even if the MCP child never began listening; modify the logic around
the for-loop that uses isPortOpen(httpPort) so the function only returns after
the port is confirmed open and otherwise throws or rejects an error after the
timeout window (e.g., after 30 attempts). Specifically, in the code that checks
isPortOpen and currently returns { httpPort }, ensure you detect failure to
observe an open port (using the same loop/counter) and raise an Error (or
reject) with a clear message including httpPort so callers know startup failed
instead of getting deferred fetch errors.
In `@packages/pencil/src/lib/router.ts`:
- Around line 323-332: The code temporarily mutates process.env.PENCIL_CLI_KEY
while awaiting verifyCliKey(trimmed, ctx) which can leak the candidate key to
concurrent operations; instead, stop swapping process.env entirely and thread
the candidate key into the verification call and any downstream functions (e.g.,
change verifyCliKey to accept an explicit candidateKey parameter or options
object and use that value for verification), then only set
process.env.PENCIL_CLI_KEY after successful verification/commit; remove
previousEnv/restore logic and any reliance inside verifyCliKey on reading
process.env.PENCIL_CLI_KEY directly.
- Around line 281-301: The diagnostic handler currently returns partial API-key
material via cliKeyPreview (see the resolved variable from
auth.resolveCliKey()), so replace that sensitive field with non-secret
presence/source metadata only: remove any slice/publication of resolved.key and
instead expose booleans or source identifiers (e.g., resolved != null and
resolved.source or resolved.origin) so callers can see whether a key exists and
where it came from without returning any characters of the key; update the
response object in the GET "/diagnostic" block to use the resolved symbol's
non-secret metadata fields instead of cliKeyPreview.
- Around line 432-438: The current startsWith() guard is insecure; instead
resolve and canonicalize paths and perform an exact-or-child check: use
path.resolve/ fs.realpathSync (or their async equivalents) to get
realResolvedTarget from payload.path and realWorkspace / realStorage for
ctx.workspace and ctx.storage, then set safe true if realResolvedTarget ===
realWorkspace or realResolvedTarget === realStorage or
path.relative(realWorkspace, realResolvedTarget) does not start with '..' (same
check for realStorage) — replace the existing safe expression that references
payload.path, ctx.workspace, and ctx.storage with this resolved/canonicalized
boundary check in the router handler where safe is computed.
In `@packages/pencil/src/serve.ts`:
- Around line 82-93: The startup currently swallows failures from
startTransportServer() and startMcpBinary(), causing the process to appear
healthy even when the transport bridge is down; change the error handling so
failures abort startup (or keep readiness unhealthy) by removing the silent
.catch handlers and either rethrow the error or call process.exit(1) when
startTransportServer() rejects, and likewise treat startMcpBinary() rejection as
fatal (replace .catch(() => null) with throwing or exiting) so that binaryInfo
is only used when successful; update the console.error/logging to include error
details from the thrown exception (referencing startTransportServer,
startMcpBinary, and binaryInfo) while ensuring the process exits non‑zero on
failure.
In `@packages/pencil/src/ui/app.ts`:
- Around line 125-133: jsonFetch currently swallows HTTP errors and JSON parse
failures by returning {} or { error: ... }, so callers cannot catch
network/backend failures; update jsonFetch to check res.ok and throw an Error
(including res.status and response text) when !res.ok, and also rethrow (throw
an Error with parse context) when JSON.parse fails instead of returning an error
object; keep the happy path returning parsed JSON as T and preserve handling of
empty bodies if desired by returning {} only when res.ok and body empty—use the
jsonFetch function name to locate and change the behavior.
- Around line 187-189: The iframe messaging currently trusts ev.source and uses
postMessage(..., "*"); update the message handler that receives events (the
"message" event listener) to validate ev.origin against the expected origin
(e.g., derive expectedOrigin from dom.iframe.src or a stored allowedOrigin)
before accepting inbound requests, and when sending replies use a concrete
target origin instead of "*" by calling
dom.iframe.contentWindow.postMessage(reply, expectedOrigin) or
postMessage(reply, ev.origin) only after verifying it matches the allowed
origin; apply the same origin-check + concrete-postMessage change to the other
handler logic covering the block around lines 206-223.
In `@packages/pencil/src/ui/styles.css`:
- Around line 50-51: The CSS font-family variables --font-sans and --font-mono
use unquoted platform font names which triggers stylelint value-keyword-case
errors; update their declarations in packages/pencil/src/ui/styles.css to wrap
BlinkMacSystemFont, SFMono-Regular, and Menlo in quotes (e.g.,
"BlinkMacSystemFont", "SFMono-Regular", "Menlo") so the font-family lists are
properly quoted while preserving the existing order and other family names.
- Around line 129-136: Rules .brand-dot[data-state="live"] and
.brand-dot[data-state="run"] use hardcoded colors (e.g., rgba(52, 199, 89, 0.18)
and white) and must be switched to CSS variables; add new tokens in :root (e.g.,
--success-soft or --accent-soft and --on-surface or equivalent) and replace the
hardcoded values in .brand-dot[data-state="live"] and
.brand-dot[data-state="run"] with those variables, ensuring box-shadow and
background use var(--token) forms; also search the file for the other
occurrences mentioned (lines around 448-454 and 572-574) and update them to use
the same tokens so all colors conform to the repo rule.
---
Nitpick comments:
In `@packages/pencil/src/lib/designs.ts`:
- Around line 104-108: The code that derives baseName from penPath uses a
hardcoded split("/") which fails on Windows; update the logic in the baseName
computation to use Node's path utilities (import path) and call path.basename on
penPath (and strip the ".pen" extension as before) instead of split/pop/replace
so it works cross-platform; modify the expression that sets baseName
(referencing the baseName variable and penPath) accordingly and ensure the path
module is imported.
In `@packages/pencil/src/lib/ipc-host.ts`:
- Around line 182-187: Replace the transformation of a preview filename with a
direct call to the function that already returns the .pen path: in the
"get-resource-path" IPC handler, stop calling
designs.getActivePreview(ctx.storage) and stripping ".preview.png" and instead
call designs.getActivePen(ctx.storage) and return its result; also apply the
same refactor to the "save-resource" and "initialized" handlers so they use
designs.getActivePen(ctx.storage) consistently rather than deriving the .pen
path from the preview path.
In `@packages/pencil/src/lib/transport-server.ts`:
- Around line 48-64: The Frame union (FlatToolRequest, WrappedToolRequest, and
generic frame) is being narrowed with manual checks in handleFrame; replace that
with a ts-pattern match to make variants explicit and compile-time checked:
adjust the union types to include a clear discriminant (use type: "tool_request"
for WrappedToolRequest and e.g. type?: undefined or "flat" for FlatToolRequest)
or create an explicit discriminant union, then in handleFrame use
match(frame).with(...).run(...) branches for each variant and append
.exhaustive() to ensure new variants fail at compile time; apply the same
refactor where similar manual narrowing occurs around the 179-190 region so all
frame handling uses ts-pattern matching and exhaustive checks (reference
symbols: FlatToolRequest, WrappedToolRequest, Frame, handleFrame, ts-pattern,
.exhaustive()).
In `@packages/pencil/src/ui/parent.html`:
- Around line 18-39: Add an accessible label reference to the listbox by setting
the switcher menu's aria-labelledby to the trigger's id: update the element with
id "switcher-menu" (role="listbox") to include
aria-labelledby="switcher-trigger" so screen readers announce the
"switcher-trigger" button (whose id is "switcher-trigger") as the label for the
menu; ensure the referenced id exists ("switcher-trigger") and remains unique.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5226d0c-face-4e60-b2ee-3b752204d82d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
apps/backend/src/config/installed-apps.tspackages/pencil/agentic-app.jsonpackages/pencil/build.tspackages/pencil/package.jsonpackages/pencil/skills/pencil/SKILL.mdpackages/pencil/src/lib/auth.tspackages/pencil/src/lib/cli.tspackages/pencil/src/lib/config.tspackages/pencil/src/lib/designs.tspackages/pencil/src/lib/editor-bundle.tspackages/pencil/src/lib/iframe-rpc.tspackages/pencil/src/lib/ipc-host.tspackages/pencil/src/lib/mcp-binary.tspackages/pencil/src/lib/mcp.tspackages/pencil/src/lib/ops.tspackages/pencil/src/lib/router.tspackages/pencil/src/lib/transport-server.tspackages/pencil/src/lib/types.tspackages/pencil/src/lib/unzipper.d.tspackages/pencil/src/serve.tspackages/pencil/src/ui/app.tspackages/pencil/src/ui/parent.htmlpackages/pencil/src/ui/styles.csspackages/pencil/tsconfig.json
Comment on lines
+218
to
+224
| export function stripAnsi(s: string): string { | ||
| return String(s).replace( | ||
| // eslint-disable-next-line no-control-regex | ||
| /[][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-PRZcf-ntqry=><]/g, | ||
| "" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the complete regex pattern for stripping all ANSI escape sequences from a string?
💡 Result:
The complete regex pattern for stripping all ANSI escape sequences from a string is: /[\u001B\u009B][[#;?](?:[0-9]{1,4}(?:;[0-9]{0,4}))?[0-9A-ORZcf-nqry=><]/g This pattern matches the vast majority of ANSI/VT100 escape sequences, including CSI sequences for colors, styles, cursor movement, and extended codes. Use it with a global replace to empty string, e.g., in JavaScript: string.replace(/[\u001B\u009B][[#;?](?:[0-9]{1,4}(?:;[0-9]{0,4}))?[0-9A-ORZcf-nqry=><]/g, '') [1][2]. For an even more comprehensive pattern that also handles OSC sequences (like hyperlinks and window titles), use the one from the popular ansi-regex library [3][4][2]: /[\u001B\u009B][#;?](?:(?:(?:(?:;[-a-zA-Z\d/#&.:=?%@~_]+)|[a-zA-Z\d]+(?:;[-a-zA-Z\d/#&.:=?%@_]))?\u0007)|(?:(?:\d{1,4}(?:;\d{0,4})*)?[\dA-PR-TZcf-nq-uy=><]))/g Note that simpler patterns like /\u001b[.?m/g only handle SGR color/style codes and miss cursor controls, OSC, etc. [1]. The Python version r'\x1B(?:[@-Z\-_]|[[0-?][ -/]*[@-~])' is also comprehensive for standard sequences [5].
Citations:
- 1: https://stackoverflow.com/questions/25245716/remove-all-ansi-colors-styles-from-strings
- 2: chalk/ansi-regex@b630317
- 3: https://github.com/sindresorhus/ansi-regex
- 4: https://deepwiki.com/chalk/ansi-regex/5.2-pattern-components
- 5: https://stackoverflow.com/questions/14693701/how-can-i-remove-the-ansi-escape-sequences-from-a-string-in-python
🏁 Script executed:
# First, let's check the exact code in the file
cat -n packages/pencil/src/lib/cli.ts | sed -n '215,230p'Repository: zvadaadam/deus-machine
Length of output: 415
🏁 Script executed:
# Check if strip-ansi or ansi-regex are already dependencies
grep -E "(strip-ansi|ansi-regex)" packages/pencil/package.jsonRepository: zvadaadam/deus-machine
Length of output: 48
🏁 Script executed:
# Let's also check the project root for these dependencies
grep -E "(strip-ansi|ansi-regex)" package.json 2>/dev/null || echo "Not found in root"Repository: zvadaadam/deus-machine
Length of output: 84
🌐 Web query:
strip-ansi vs ansi-regex npm package comparison which is better
💡 Result:
strip-ansi is better for most use cases as it provides a complete, high-level solution to strip ANSI escape codes from strings using a simple function call.[1][2] It internally depends on and uses ansi-regex (^6.0.1+), which is a low-level package that only provides the regular expression for matching ANSI codes.[1][3][2] You typically want strip-ansi('string with codes') which returns the cleaned string, rather than manually implementing string.replace(ansiRegex, '').[4][2] Key comparison: - Purpose: strip-ansi removes codes; ansi-regex matches them.[5][2] - Usage: strip-ansi is ready-to-use; ansi-regex requires extra code.[1][5] - Popularity: strip-ansi has ~450M weekly downloads vs ansi-regex's lower direct usage (mostly transitive).[1] - Maintenance: Both by Sindre Sorhus/chalk team, actively updated (strip-ansi v7.2.0 as of 2026).[2][6] - Note: Node.js has built-in stripVTControlCharacters, but strip-ansi offers cross-version consistency and is the basis for Node's implementation.[1][2] Use strip-ansi unless you specifically need the regex (e.g., for custom matching).[2]
Citations:
- 1: https://www.npmjs.com/package/strip-ansi
- 2: https://github.com/chalk/strip-ansi?sponsor=1
- 3: https://registry.npmjs.org/strip-ansi
- 4: https://tessl.io/registry/tessl/npm-strip-ansi/7.1.0
- 5: https://www.npmjs.com/package/ansi-regex
- 6: https://github.com/chalk/ansi-regex/blob/main/package.json
Use strip-ansi package to properly strip all ANSI escape sequences.
The current regex is incomplete and missing the escape character prefix, which means it won't match CSI sequences starting with \u001B or \u009B. It also doesn't handle OSC sequences (e.g., hyperlinks, window titles). Replace the custom implementation with the strip-ansi package, which is well-tested, maintained by the chalk team, and handles all ANSI sequence types comprehensively.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pencil/src/lib/cli.ts` around lines 218 - 224, The custom stripAnsi
function (stripAnsi) in packages/pencil/src/lib/cli.ts is incomplete; replace it
with the well-tested strip-ansi package: add strip-ansi as a dependency, import
the package in cli.ts (using the appropriate ES/CJS import style for the repo),
and change the stripAnsi implementation to simply return the result of calling
the strip-ansi function on the input string. Ensure TypeScript typings work
(install `@types/strip-ansi` if needed) and remove the old regex code.
Owner
Author
There was a problem hiding this comment.
Disagreeing here. The existing regex matches CSI sequences (ESC + parameter + final byte) that pencil status and pencil version actually emit — confirmed by reading the bytes (\x1b\x9b opening, [0-9A-PRZcf-ntqry=><] finals). CodeRabbit's web-search-derived 'complete' regex covers OSC and Fe escapes too, but those don't appear in the input domain we parse. Adding broader matching would risk corrupting non-ANSI text without solving a real bug. Keeping the current regex.
Comment on lines
+82
to
+93
| await startTransportServer().catch((err) => { | ||
| console.error(`[pencil-aap] transport-server failed: ${err.message}`); | ||
| }); | ||
| // Slight delay before spawning so the socket is definitely listening | ||
| // when the binary tries to dial in. | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| const binaryInfo = await startMcpBinary().catch(() => null); | ||
| if (binaryInfo) { | ||
| console.log( | ||
| `[pencil-aap] bundled MCP binary listening on http://127.0.0.1:${binaryInfo.httpPort}/mcp` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Abort startup when the transport bridge is unavailable.
If startTransportServer() or startMcpBinary() fails, the process still comes up and will pass the /health readiness check, but the native editor tool bridge is only partially available. That makes the app look healthy while a core PR objective is broken. This should fail fast, or at least keep readiness unhealthy until the bridge is up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pencil/src/serve.ts` around lines 82 - 93, The startup currently
swallows failures from startTransportServer() and startMcpBinary(), causing the
process to appear healthy even when the transport bridge is down; change the
error handling so failures abort startup (or keep readiness unhealthy) by
removing the silent .catch handlers and either rethrow the error or call
process.exit(1) when startTransportServer() rejects, and likewise treat
startMcpBinary() rejection as fatal (replace .catch(() => null) with throwing or
exiting) so that binaryInfo is only used when successful; update the
console.error/logging to include error details from the thrown exception
(referencing startTransportServer, startMcpBinary, and binaryInfo) while
ensuring the process exits non‑zero on failure.
Owner
Author
There was a problem hiding this comment.
Considered. Going to leave /health reporting OK even if the transport bridge fails: the AAP is still useful in that state (the 4 workspace tools work, and the editor iframe still loads). Failing readiness would prevent users from even pasting a CLI key. Logged a warning at startup if the bridge isn't up; can revisit if we see the partial state cause confusion in the wild.
There was a problem hiding this comment.
@zvadaadam, that's a reasonable tradeoff — keeping the 4 workspace tools and the auth flow functional during a degraded bridge state makes sense, especially since the bridge failure may be transient and the user can still onboard. The warning log at startup is a good middle ground. Thanks for the context!
(ᵔ.ᵔ) ~ curious bunny noted
✏️ Learnings added
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 270
File: packages/pencil/src/serve.ts:98-109
Timestamp: 2026-05-10T17:43:03.672Z
Learning: In `packages/pencil/src/serve.ts`, the `/health` readiness endpoint intentionally returns OK even when `startTransportServer()` or `startMcpBinary()` fails. The design decision is deliberate: the AAP remains useful in degraded state (4 workspace tools work, editor iframe loads, CLI key auth flow unblocked). A warning is logged at startup when the bridge is unavailable. Do not flag this as a bug or suggest failing readiness on bridge failure.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zvadaadam
Repo: zvadaadam/box-ide PR: 177
File: src/app/layouts/hooks/useRepoActions.ts:21-22
Timestamp: 2026-03-12T23:25:58.674Z
Learning: In zvadaadam/box-ide, two TypeScript path aliases coexist: 'shared/*' -> '/shared/' (root-level, cross-package) and '@/shared/*' -> 'src/shared/' (frontend-only). Do not flag mixed usage as inconsistent since they reference different directories and scopes. Ensure imports use the correct alias per intended scope and that tsconfig.json and vite.config.ts are configured accordingly. This applies to all TypeScript files in the repo.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
packages/pencil/src/ui/styles.css (2)
50-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the platform font family names.
BlinkMacSystemFont,SFMono-Regular, andMenloare still unquoted, which is what the reportedvalue-keyword-caseerrors are complaining about.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/styles.css` around lines 50 - 51, The CSS custom properties --font-sans and --font-mono contain unquoted platform font family names causing value-keyword-case lint errors; update the declarations in packages/pencil/src/ui/styles.css to quote BlinkMacSystemFont, SFMono-Regular, and Menlo (i.e., wrap those family names in quotes) so both --font-sans and --font-mono use quoted font-family tokens.
129-136: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace the remaining raw colors with tokens.
These rules still hardcode runtime colors instead of consuming CSS variables, so they can drift from the theme and still violate the repo styling rule.
As per coding guidelines "All colors must use CSS variables/tokens. Never hardcode colors."
Also applies to: 452-454, 586-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/styles.css` around lines 129 - 136, The .brand-dot[data-state="live"] and .brand-dot[data-state="run"] rules use a hardcoded rgba color and a non-token box-shadow color; replace the rgba(52,199,89,0.18) and any other raw colors with the appropriate CSS variables (e.g., use --success-soft or a new --success-ring token) so all colors come from tokens. Update the .brand-dot[data-state="live"] box-shadow to use the token (and add the token to the theme if missing), and ensure .brand-dot[data-state="run"] and the other referenced locations (lines ~452-454 and ~586-588) also reference tokens instead of raw colors. Ensure names of tokens match existing naming conventions like --success, --accent, --accent-soft, or add consistent new tokens such as --success-soft/--success-ring where needed.packages/pencil/src/ui/app.ts (2)
128-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
jsonFetch()reject on HTTP and parse failures.This still turns backend failures into
{}/{ error: ... }, so callers likecheckAuth()andrefreshDesigns()treat transport problems as successful payloads. That can show “not signed in” or wipe the switcher during transient failures instead of taking the error path.Suggested fix
async function jsonFetch<T = unknown>(url: string, init?: RequestInit): Promise<T> { const res = await fetch(url, init); const text = await res.text(); + if (!res.ok) { + throw new Error(text || `HTTP ${res.status}`); + } try { return text.length ? (JSON.parse(text) as T) : ({} as T); - } catch { - return { error: `bad response (${res.status})` } as unknown as T; + } catch { + throw new Error(`Invalid JSON from ${url} (${res.status})`); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 128 - 135, The jsonFetch function currently swallows HTTP and JSON parse failures and returns empty objects, causing callers like checkAuth() and refreshDesigns() to treat failures as success; update jsonFetch(url, init) so that after awaiting fetch it checks res.ok and throws a descriptive Error (include res.status and statusText or body) on non-2xx responses, then attempts JSON.parse(text) and throws a parse Error if parsing fails (include the raw text and status), and only returns the parsed T on success; ensure the function signature Promise<T> remains and that thrown Errors propagate to callers so they can handle transport and parse errors instead of receiving {} or { error: ... }.
190-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin iframe messaging to the expected origin.
Inbound messages are still accepted based only on
ev.source, and outbound replies still usepostMessage(..., "*"). If the iframe navigates to an unexpected origin, that page can drive privileged/ipccalls and receive host responses.Suggested fix
+function editorOrigin(): string { + return new URL(dom.iframe.src, window.location.href).origin; +} + async function handleEditorMessage(msg: IpcMessage): Promise<void> { // Forward to backend. let reply: IpcMessage | null = null; @@ } if (reply && dom.iframe.contentWindow) { - dom.iframe.contentWindow.postMessage(reply, "*"); + dom.iframe.contentWindow.postMessage(reply, editorOrigin()); } } @@ window.addEventListener("message", (ev) => { - if (ev.source !== dom.iframe.contentWindow) return; + if (ev.source !== dom.iframe.contentWindow) return; + if (ev.origin !== editorOrigin()) return; const data = ev.data; @@ function pushToEditor(msg: IpcMessage): void { - if (dom.iframe.contentWindow) dom.iframe.contentWindow.postMessage(msg, "*"); + if (dom.iframe.contentWindow) { + dom.iframe.contentWindow.postMessage(msg, editorOrigin()); + } }Also applies to: 209-226
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 190 - 191, Inbound iframe messages and outbound replies must be constrained to the expected origin: update the message event handler to validate ev.origin === expectedOrigin (and that ev.source === dom.iframe.contentWindow) before accepting messages, and replace dom.iframe.contentWindow.postMessage(reply, "*") with postMessage(reply, expectedOrigin). Use a single well-named constant (e.g., expectedOrigin) for the allowed origin and apply the same origin check when handling messages and when sending replies from dom.iframe to prevent communicating with navigated/untrusted origins.packages/pencil/src/lib/iframe-rpc.ts (1)
31-64:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftScope each iframe RPC to one connected panel.
This still broadcasts every request globally, and replies are still matched only by request id. With two Pencil panels open, both editors can execute the same operation and the first reply wins; with zero panels, callers just burn the full timeout. This needs per-panel targeting plus reply validation against the chosen session.
Also applies to: 92-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/lib/iframe-rpc.ts` around lines 31 - 64, requestFromIframe currently broadcasts every request via broadcastEvent and only matches replies by id, causing cross-panel interference and wasted time when no panel is connected; fix by adding per-panel targeting and reply validation: require or resolve a target session/panel id before sending (e.g., pass targetSessionId into requestFromIframe or obtain currentConnectedPanelId inside the function), include that sessionId on the outgoing message (replace broadcastEvent with a panel-specific send if available or attach sessionId in payload via normalizeIframePayload), store the sessionId on the PendingRequest entry in pending, and update the reply handler to validate incoming IframeReply.sessionId (or reply.session) against the stored pending.sessionId before resolving; also, if no panel/session is connected, immediately reject instead of starting the timeout. Ensure references: requestFromIframe, pending Map, normalizeIframePayload, broadcastEvent and IframeReply are updated accordingly.
🧹 Nitpick comments (1)
apps/backend/src/services/aap/apps.service.ts (1)
162-226: 💤 Low valueConsider adding a timeout to prefetch commands.
The
runPrefetchpromise resolves on child exit or spawn error, but a hung prefetch process would never complete. While this is background work and orphans would be cleaned on next backend restart, a timeout (e.g., 60s) would prevent resource accumulation during long-running sessions.This is low priority since prefetch is fire-and-forget and the comment correctly notes failures shouldn't affect app availability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/services/aap/apps.service.ts` around lines 162 - 226, runPrefetch can hang if the spawned prefetch child never exits; add a configurable timeout (e.g., 60_000 ms) after spawn that, when fired, kills the child, logs a warning including manifest.id and stdout/stderr tails, and calls finish(); store the timer id and clear it inside finish() (and in the spawn error/exit handlers) so it doesn't fire after normal completion; update references in runPrefetch to use this timeout logic around the ChildProcess created by spawn and ensure finish() remains the single resolver for the Promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pencil/src/lib/mcp.ts`:
- Around line 237-249: The code masks failures from listBinaryTools() by using
.catch(() => []) so /mcp can return an incomplete tool list; change this to
surface errors instead: remove the swallow-catch, call listBinaryTools() inside
a try/catch, and on catch return a JSON-RPC error response (use the same
response pattern you use elsewhere—e.g., return err(safeId, { code:
<appropriate_code>, message: error.message })) rather than falling back to an
empty array; reference listBinaryTools, TOOLS, merged and safeId to locate where
to implement the try/catch and error return.
- Around line 208-210: When handling an array batch in the branch where
Array.isArray(req), add two behaviors: if req.length === 0 return a single
JSON-RPC "invalid request" error response instead of [], and if the batch
contains only notifications (i.e. after awaiting Promise.all(req.map((r) =>
handleSingle(r, ctx))) the filtered results array is empty) return null/no
response instead of an empty array. Concretely: detect empty batch up front and
construct/return the standard JSON-RPC invalid request error object; otherwise
await handleSingle for each entry, filter out null responses (notifications),
and if the resulting array has length 0 return null (no response) else return
the array of JsonRpcResponse. Use the existing handleSingle, ctx and
JsonRpcResponse types when implementing.
In `@packages/pencil/src/serve.ts`:
- Around line 196-198: The pathToFileURI function currently uses encodeURI which
leaves characters like '#' and '?' unescaped causing incorrect file URIs; update
pathToFileURI to produce a safe file:// URI by using Node's url.pathToFileURL or
by encoding the path with encodeURIComponent (while preserving leading slash)
before prepending "file://", ensuring filenames with '#', '?' or other reserved
characters are percent-encoded; locate the function named pathToFileURI and
replace the encodeURI-based implementation with a call to
url.pathToFileURL(p).toString() (or the equivalent encodeURIComponent approach)
and add the necessary import for Node's url module.
In `@packages/pencil/src/ui/parent.html`:
- Around line 76-83: The CLI key input (id="signin-key") is currently plain
text; change its type to "password" so the value is masked to treat it as a
secret. Update the input element in parent.html by replacing type="text" with
type="password" (keep other attributes like autocapitalize="off" and
spellcheck="false"), and optionally set a more appropriate autocomplete value
such as "new-password" or "current-password" to reduce accidental autofill.
---
Duplicate comments:
In `@packages/pencil/src/lib/iframe-rpc.ts`:
- Around line 31-64: requestFromIframe currently broadcasts every request via
broadcastEvent and only matches replies by id, causing cross-panel interference
and wasted time when no panel is connected; fix by adding per-panel targeting
and reply validation: require or resolve a target session/panel id before
sending (e.g., pass targetSessionId into requestFromIframe or obtain
currentConnectedPanelId inside the function), include that sessionId on the
outgoing message (replace broadcastEvent with a panel-specific send if available
or attach sessionId in payload via normalizeIframePayload), store the sessionId
on the PendingRequest entry in pending, and update the reply handler to validate
incoming IframeReply.sessionId (or reply.session) against the stored
pending.sessionId before resolving; also, if no panel/session is connected,
immediately reject instead of starting the timeout. Ensure references:
requestFromIframe, pending Map, normalizeIframePayload, broadcastEvent and
IframeReply are updated accordingly.
In `@packages/pencil/src/ui/app.ts`:
- Around line 128-135: The jsonFetch function currently swallows HTTP and JSON
parse failures and returns empty objects, causing callers like checkAuth() and
refreshDesigns() to treat failures as success; update jsonFetch(url, init) so
that after awaiting fetch it checks res.ok and throws a descriptive Error
(include res.status and statusText or body) on non-2xx responses, then attempts
JSON.parse(text) and throws a parse Error if parsing fails (include the raw text
and status), and only returns the parsed T on success; ensure the function
signature Promise<T> remains and that thrown Errors propagate to callers so they
can handle transport and parse errors instead of receiving {} or { error: ... }.
- Around line 190-191: Inbound iframe messages and outbound replies must be
constrained to the expected origin: update the message event handler to validate
ev.origin === expectedOrigin (and that ev.source === dom.iframe.contentWindow)
before accepting messages, and replace
dom.iframe.contentWindow.postMessage(reply, "*") with postMessage(reply,
expectedOrigin). Use a single well-named constant (e.g., expectedOrigin) for the
allowed origin and apply the same origin check when handling messages and when
sending replies from dom.iframe to prevent communicating with
navigated/untrusted origins.
In `@packages/pencil/src/ui/styles.css`:
- Around line 50-51: The CSS custom properties --font-sans and --font-mono
contain unquoted platform font family names causing value-keyword-case lint
errors; update the declarations in packages/pencil/src/ui/styles.css to quote
BlinkMacSystemFont, SFMono-Regular, and Menlo (i.e., wrap those family names in
quotes) so both --font-sans and --font-mono use quoted font-family tokens.
- Around line 129-136: The .brand-dot[data-state="live"] and
.brand-dot[data-state="run"] rules use a hardcoded rgba color and a non-token
box-shadow color; replace the rgba(52,199,89,0.18) and any other raw colors with
the appropriate CSS variables (e.g., use --success-soft or a new --success-ring
token) so all colors come from tokens. Update the .brand-dot[data-state="live"]
box-shadow to use the token (and add the token to the theme if missing), and
ensure .brand-dot[data-state="run"] and the other referenced locations (lines
~452-454 and ~586-588) also reference tokens instead of raw colors. Ensure names
of tokens match existing naming conventions like --success, --accent,
--accent-soft, or add consistent new tokens such as
--success-soft/--success-ring where needed.
---
Nitpick comments:
In `@apps/backend/src/services/aap/apps.service.ts`:
- Around line 162-226: runPrefetch can hang if the spawned prefetch child never
exits; add a configurable timeout (e.g., 60_000 ms) after spawn that, when
fired, kills the child, logs a warning including manifest.id and stdout/stderr
tails, and calls finish(); store the timer id and clear it inside finish() (and
in the spawn error/exit handlers) so it doesn't fire after normal completion;
update references in runPrefetch to use this timeout logic around the
ChildProcess created by spawn and ensure finish() remains the single resolver
for the Promise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b5f381d-b793-4ee8-a5e9-4a305e846a94
📒 Files selected for processing (27)
apps/backend/src/server.tsapps/backend/src/services/aap/apps.service.tsapps/backend/src/services/aap/index.tsapps/backend/src/services/aap/lifecycle.tsapps/backend/test/integration/aap.test.tsapps/backend/test/unit/shared/aap/manifest.test.tselectron-builder.ymlpackage.jsonpackages/pencil/agentic-app.jsonpackages/pencil/build.tspackages/pencil/package.jsonpackages/pencil/src/lib/cli.tspackages/pencil/src/lib/editor-bundle.tspackages/pencil/src/lib/iframe-rpc.tspackages/pencil/src/lib/ipc-host.tspackages/pencil/src/lib/mcp.tspackages/pencil/src/lib/router.tspackages/pencil/src/serve.tspackages/pencil/src/ui/app.tspackages/pencil/src/ui/parent.htmlpackages/pencil/src/ui/styles.csspackages/pencil/test/cli.test.tspackages/pencil/test/iframe-rpc.test.tsscripts/dev.shscripts/prune-pencil-cli-binaries.cjsshared/aap/manifest.tstest/unit/runtime/prune-pencil-cli-binaries.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/pencil/package.json
- packages/pencil/agentic-app.json
- packages/pencil/src/lib/editor-bundle.ts
- packages/pencil/src/lib/router.ts
- packages/pencil/src/lib/cli.ts
- packages/pencil/src/lib/ipc-host.ts
…l-agentic # Conflicts: # electron-builder.yml # package.json # scripts/dev.sh
Two bugs in the + New design flow:
1. pencil_new awaited requestFromIframe("open-document", ...) which
waits up to 60s for an editor reply that never comes (the editor's
handler is sync and doesn't always respond to "new"). Switched to
notifyEditor — fire-and-forget — so the tool returns in <20ms.
2. After success, the switcher still showed "no design" because the
.pen file doesn't exist on disk yet (encrypted format, materializes
on first save). The active path is set but no Design entry matches.
Added a `pendingDesign` state in the panel: when the server emits
active-file with `pending: true`, the trigger renders "<name> · new"
with the rose accent. The pending state clears automatically once
the real file lands on disk (e.g. when the agent's first batch_design
triggers a save).
Tested: pencil_new now returns in 13ms. UI shows the pending design as
the active entry until the real file lands.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pencil/src/ui/app.ts`:
- Around line 233-260: The SSE handlers for "op-start", "op-end", "op-phase",
"ipc-notify", and "ipc-request" call JSON.parse directly and can throw; wrap the
body of each listener in a try-catch around JSON.parse (and subsequent message
handling) so malformed events don't crash the handler, log or console.error the
parse error and return early; specifically update the event listeners that call
JSON.parse and then call showRunningPill, hideRunningPill, set dom.runLabel
(with state.runStartedAt check), and pushToEditor to follow the same try-catch
pattern used by the existing "active-file" handler so errors are caught and the
UI stays stable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4555a8b1-aadf-4152-9416-ba36492d24be
📒 Files selected for processing (2)
packages/pencil/src/lib/mcp.tspackages/pencil/src/ui/app.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pencil/src/lib/mcp.ts
Two UX problems the user flagged in PR #270: 1. Switcher said "no design" while the canvas was full of frames the agent had drawn. Cause: the panel only learned of the active path from pencil_new / pencil_open / save-resource. When the agent went straight to the binary's tools (open_document, batch_design) the server stayed blind. Added a /detect-active route that probes get-editor-state via the bridge and parses documentURI; the panel polls it every 4s (and once 1s after iframe load). The trigger now updates within seconds of any document change in the editor, no matter which path opened it. 2. Default save location was <workspace>/.pencil/designs/<name>.pen — hidden, easy to miss in git. Moved to <workspace>/designs/<name>.pen so the user's agent-generated designs commit naturally as part of their repo. The .pencil dir is still used for AAP-private state (active pointer, editor-session, cli-key, cache). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/pencil/src/ui/app.ts (3)
190-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock iframe messaging to a concrete allowed origin.
Line 191 and Line 226 post to
"*", and Line 209-223 accepts inbound messages without origin validation. That leaves the IPC bridge exposed if the iframe origin ever changes unexpectedly.Suggested fix
+function editorOrigin(): string { + return new URL(dom.iframe.src, window.location.href).origin; +} + window.addEventListener("message", (ev) => { if (ev.source !== dom.iframe.contentWindow) return; + if (ev.origin !== editorOrigin()) return; const data = ev.data; if (!data || typeof data !== "object") return; const msg = data as Partial<IpcMessage>; @@ if (reply && dom.iframe.contentWindow) { - dom.iframe.contentWindow.postMessage(reply, "*"); + dom.iframe.contentWindow.postMessage(reply, editorOrigin()); } } @@ function pushToEditor(msg: IpcMessage): void { - if (dom.iframe.contentWindow) dom.iframe.contentWindow.postMessage(msg, "*"); + if (dom.iframe.contentWindow) dom.iframe.contentWindow.postMessage(msg, editorOrigin()); }Also applies to: 209-223, 225-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 190 - 192, Replace wildcard postMessage and lax inbound handling with an explicit allowed origin check: define a constant allowedOrigin (the concrete iframe origin) and use dom.iframe.contentWindow.postMessage(reply, allowedOrigin) in place of "*" in the places where postMessage is called; also update the incoming window.addEventListener("message", ...) handler to immediately return unless event.origin === allowedOrigin and optionally verify event.source === dom.iframe.contentWindow and event.data structure before acting. Apply these changes for the occurrences that reference dom.iframe.postMessage and the message event handling to lock down the IPC bridge.
233-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard SSE JSON parsing to avoid handler crashes on malformed events.
Lines 233-260 call
JSON.parsedirectly in multiple listeners; one malformed event can throw and skip expected UI updates/relays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 233 - 260, Wrap each SSE handler's JSON.parse call in a try/catch so a malformed event won't throw and break the listener; for the "op-start" and "op-end" handlers (which call showRunningPill and hideRunningPill), the "op-phase" handler (which reads data.phase and updates dom.runLabel when state.runStartedAt !== null), and the "ipc-notify"/"ipc-request" handlers (which call pushToEditor), catch JSON parse errors, log or console.warn the malformed payload with context (e.g., event type and raw data) and return early so normal UI updates and relays continue for future events.
128-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
jsonFetchshould throw on HTTP/parse errors instead of returning pseudo-success payloads.Line 128-135 currently swallows failures, so callers’
catchpaths (e.g., auth/design refresh) won’t run and backend outages can be misclassified as normal app state.Suggested fix
async function jsonFetch<T = unknown>(url: string, init?: RequestInit): Promise<T> { const res = await fetch(url, init); const text = await res.text(); - try { - return text.length ? (JSON.parse(text) as T) : ({} as T); - } catch { - return { error: `bad response (${res.status})` } as unknown as T; - } + if (!res.ok) { + throw new Error(`HTTP ${res.status} from ${url}: ${text || "<empty body>"}`); + } + if (!text.length) return {} as T; + try { + return JSON.parse(text) as T; + } catch (err) { + throw new Error( + `Invalid JSON from ${url}: ${err instanceof Error ? err.message : String(err)}`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 128 - 135, The function jsonFetch currently swallows HTTP and JSON parse errors; change jsonFetch to throw on non-OK responses and on parse failures instead of returning a pseudo-success payload. Specifically, in jsonFetch(url, init) check res.ok and if false read response text and throw a new Error that includes res.status and the response body; likewise, wrap JSON.parse in a try/catch and on parse failure throw an Error that includes the parse error message and the raw response text. Ensure the function still returns the parsed T on success and propagates thrown errors to callers (auth/design refresh paths) so their catch handlers run.
🧹 Nitpick comments (2)
packages/pencil/src/ui/app.ts (2)
334-335: ⚡ Quick winPrefer class-based Tailwind styling over imperative inline style mutations.
Lines 334-335, 353-354, 359-360, and 539 set styles directly in TS. Move these to Tailwind/tokenized classes and toggle classes in JS for consistency and theme safety.
As per coding guidelines: “Use Tailwind CSS v4 for styling” and “Use CSS variables and tokens for all colors — never hardcode color values.”
Also applies to: 353-354, 359-360, 539-539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 334 - 335, Replace inline style mutations with class toggles: remove assignments like dom.auth.style.color = "var(--warn)" and instead add/remove a Tailwind/CSS-tokenized class (e.g., "text-warn" or a utility that maps to --warn) on the dom.auth element; likewise stop setting dataset/state-only for styling and use class changes on dom.signinOverlay (and the other locations at the noted lines) to reflect "signin" state (e.g., toggle a state class like "state-signin"). Update the code paths touching dom.auth, dom.signinOverlay, and the other affected DOM nodes so they call element.classList.add/remove/toggle with the tokenized Tailwind classes that reference CSS variables rather than mutating style properties directly.
102-108: 🏗️ Heavy liftFrontend state/data flow bypasses the repo’s required stack (Zustand + TanStack Query subscription model).
This file manages client state with mutable module globals and periodic polling, instead of the required store/query subscription approach, which will diverge from repo-wide behavior guarantees.
As per coding guidelines: “Use Zustand for UI state management”, “Use TanStack Query v5 for server state management”, and “Frontend subscribes to server state via WebSocket using
useQuerySubscription()withstaleTime: InfinityandrefetchOnWindowFocus: false.”Also applies to: 410-425, 745-751
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pencil/src/ui/app.ts` around lines 102 - 108, The module-level mutable "state" object (signedIn, verifiedEmail, runStartedAt, elapsedTimer, toastTimer) and any polling/timer logic should be removed and replaced with a Zustand store for UI state and TanStack Query v5 subscriptions for server state: create a Zustand store (e.g., useAppStore) to hold UI-only fields (toastTimer, elapsedTimer, etc.) and migrate any reads/writes to that store, and use useQuerySubscription(...) with {staleTime: Infinity, refetchOnWindowFocus: false} to subscribe to server-derived fields (signedIn, verifiedEmail, runStartedAt) instead of polling; also remove any setInterval/setTimeout polling code and wire component effects to subscribe to the store/query updates (update handlers should call store setters or rely on query data), ensuring all places referencing the old "state" const and the periodic polling logic are updated to use useAppStore and useQuerySubscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/pencil/src/ui/app.ts`:
- Around line 190-192: Replace wildcard postMessage and lax inbound handling
with an explicit allowed origin check: define a constant allowedOrigin (the
concrete iframe origin) and use dom.iframe.contentWindow.postMessage(reply,
allowedOrigin) in place of "*" in the places where postMessage is called; also
update the incoming window.addEventListener("message", ...) handler to
immediately return unless event.origin === allowedOrigin and optionally verify
event.source === dom.iframe.contentWindow and event.data structure before
acting. Apply these changes for the occurrences that reference
dom.iframe.postMessage and the message event handling to lock down the IPC
bridge.
- Around line 233-260: Wrap each SSE handler's JSON.parse call in a try/catch so
a malformed event won't throw and break the listener; for the "op-start" and
"op-end" handlers (which call showRunningPill and hideRunningPill), the
"op-phase" handler (which reads data.phase and updates dom.runLabel when
state.runStartedAt !== null), and the "ipc-notify"/"ipc-request" handlers (which
call pushToEditor), catch JSON parse errors, log or console.warn the malformed
payload with context (e.g., event type and raw data) and return early so normal
UI updates and relays continue for future events.
- Around line 128-135: The function jsonFetch currently swallows HTTP and JSON
parse errors; change jsonFetch to throw on non-OK responses and on parse
failures instead of returning a pseudo-success payload. Specifically, in
jsonFetch(url, init) check res.ok and if false read response text and throw a
new Error that includes res.status and the response body; likewise, wrap
JSON.parse in a try/catch and on parse failure throw an Error that includes the
parse error message and the raw response text. Ensure the function still returns
the parsed T on success and propagates thrown errors to callers (auth/design
refresh paths) so their catch handlers run.
---
Nitpick comments:
In `@packages/pencil/src/ui/app.ts`:
- Around line 334-335: Replace inline style mutations with class toggles: remove
assignments like dom.auth.style.color = "var(--warn)" and instead add/remove a
Tailwind/CSS-tokenized class (e.g., "text-warn" or a utility that maps to
--warn) on the dom.auth element; likewise stop setting dataset/state-only for
styling and use class changes on dom.signinOverlay (and the other locations at
the noted lines) to reflect "signin" state (e.g., toggle a state class like
"state-signin"). Update the code paths touching dom.auth, dom.signinOverlay, and
the other affected DOM nodes so they call element.classList.add/remove/toggle
with the tokenized Tailwind classes that reference CSS variables rather than
mutating style properties directly.
- Around line 102-108: The module-level mutable "state" object (signedIn,
verifiedEmail, runStartedAt, elapsedTimer, toastTimer) and any polling/timer
logic should be removed and replaced with a Zustand store for UI state and
TanStack Query v5 subscriptions for server state: create a Zustand store (e.g.,
useAppStore) to hold UI-only fields (toastTimer, elapsedTimer, etc.) and migrate
any reads/writes to that store, and use useQuerySubscription(...) with
{staleTime: Infinity, refetchOnWindowFocus: false} to subscribe to
server-derived fields (signedIn, verifiedEmail, runStartedAt) instead of
polling; also remove any setInterval/setTimeout polling code and wire component
effects to subscribe to the store/query updates (update handlers should call
store setters or rely on query data), ensuring all places referencing the old
"state" const and the periodic polling logic are updated to use useAppStore and
useQuerySubscription.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4681d0ef-57f2-45e4-9d96-236650ef1e4a
📒 Files selected for processing (4)
packages/pencil/src/lib/designs.tspackages/pencil/src/lib/mcp.tspackages/pencil/src/lib/router.tspackages/pencil/src/ui/app.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/pencil/src/lib/mcp.ts
- packages/pencil/src/lib/designs.ts
- packages/pencil/src/lib/router.ts
Security
- /diagnostic: drop cliKeyPreview (was leaking 14 chars). Return
cliKeyPresent + cliKeySource only. (#3214958225)
- /reveal /open-pen: replace startsWith() with resolve+relative
isInside(); sibling-prefix paths like ${workspace}-other/... now
401, not allowed. (#3214958230)
- parent.html sign-in: type="text" → type="password" so the CLI key
isn't shoulder-surfed. (#3215131338)
- app.ts iframe IPC: lock postMessage origin to window.location.origin
for both inbound message validation and outbound replies/pushes. (#3214958238)
Reliability
- mcp-binary.ts: if the binary's HTTP server never comes up in 3s,
kill the child and return null instead of returning a bogus
{ httpPort } that every later fetch hits ECONNREFUSED on. (#3214958220)
- editor-bundle.ts rewriteEditorIndex: throw if the regex no longer
matches the entry script — fail-closed when Pencil ships a new
bundle shape, instead of silently breaking the IPC bridge. (#3214958217)
- mcp.ts tools/list: surface listBinaryTools() failures as a JSON-RPC
-32603 instead of advertising only the 4 local tools. Agents now
see the failure instead of hitting "tool not found" on batch_design. (#3215131335)
- mcp.ts batch handler: empty batch → -32600, notification-only
batch → no response (per JSON-RPC 2.0 §6). (#3215131331)
- app.ts jsonFetch: throw on non-OK / bad JSON instead of returning a
fake { error } that callers ignore. Existing call sites already
wrap in try/catch — they now actually see failures. (#3214958236)
- app.ts SSE handlers: wrap JSON.parse in safeParse; one malformed
event no longer crashes the entire SSE stream handler. (#3215228758)
- ipc-host.ts save-resource: validate payload.content is a string
before writing — prevents silent corruption from a malformed editor
message. (#3214958219)
- designs.ts findWorkspaceDesigns: use previewPathForPen so workspace
files outside <storage>/designs/ get the actual cache path, not a
non-existent sibling .preview.png. (#3214958216)
- serve.ts + ipc-host.ts pathToFileURI: use Node's pathToFileURL —
filenames with #, ?, or other URI-significant chars now round-trip
correctly. (#3215131337)
Polish
- SKILL.md: add `text` language tag to fenced examples to satisfy
markdownlint. (#3214958210)
- styles.css: quote BlinkMacSystemFont, SFMono-Regular, Menlo so
stylelint passes. Replace hardcoded rgba(52,199,89,0.18) /
rgba(255,107,107,0.4) / `white` with --success-soft / --err-soft /
--fg-strong tokens (CLAUDE.md compliance). (#3214958239 #3214958242)
Deferred (with reasoning)
- iframe-rpc multi-panel scoping (#3214958218): heavy lift, hypothetical.
Single panel works fine; revisit when multi-panel is a real use case.
- stripAnsi regex breadth (#3214958212): existing pattern matches the
CSI sequences `pencil status`/`version` actually emit. CodeRabbit's
"complete" web-search regex is broader than the input domain warrants.
Already-fixed-on-branch
- verifyCliKey unused _key (#3214958211): merged work threads `key`
through buildCliEnv override.
- /auth-set process.env mutation (#3214958228): /auth-set already
uses the buildCliEnv override path; no env mutation remains.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Greenlight — Round 1Triaged 21 CodeRabbit comments. Pushed Fixed (16)Security
Reliability
Polish
Deferred (2)
Already-fixed-on-branch (3)
End-to-end smoke verified: empty batch → 400, sibling-prefix → 403, real workspace path → 200, |
zvadaadam
added a commit
that referenced
this pull request
May 31, 2026
Two UX problems the user flagged in PR #270: 1. Switcher said "no design" while the canvas was full of frames the agent had drawn. Cause: the panel only learned of the active path from pencil_new / pencil_open / save-resource. When the agent went straight to the binary's tools (open_document, batch_design) the server stayed blind. Added a /detect-active route that probes get-editor-state via the bridge and parses documentURI; the panel polls it every 4s (and once 1s after iframe load). The trigger now updates within seconds of any document change in the editor, no matter which path opened it. 2. Default save location was <workspace>/.pencil/designs/<name>.pen — hidden, easy to miss in git. Moved to <workspace>/designs/<name>.pen so the user's agent-generated designs commit naturally as part of their repo. The .pencil dir is still used for AAP-private state (active pointer, editor-session, cli-key, cache). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
packages/pencil— a Deus AAP that embeds the Pencil editor in an iframe and bridges the bundled@pencil.dev/cliMCP binary to the live canvas, so agents call Pencil's full tool surface (batch_design,get_editor_state,snapshot_layout,get_screenshot, …) and every op renders in real time.TransportServeraccepts the bundledmcp-serverbinary; SSE +postMessagebridge forwards tool requests to the editor'swebappapiIPC handlers; editor bundle auto-downloads fromapi.pencil.devon first launch and caches at~/.deus/pencil-editor/<version>/.pencil_list_designs,pencil_get_active,pencil_open,pencil_new) + 13 native editor tools forwarded through — no CLI subprocesses for design work, every action visible.installed-apps.ts.Test plan
bun run buildinpackages/pencilsucceeds and producesdist/serve.js+dist/ui/app.js.penloadedtools/listover/mcpreturns 17 tools (4 ours + 13 native)pencil_new→ blank canvas appears in the panel; subsequentbatch_designops render live.penfiles, switching between them does not flash the previous design+ New designform: name → blank canvas opens, agent can design via chat🤖 Generated with Claude Code
Summary by CodeRabbit