Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion canvas/server/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,8 @@ export function createAgentServer(options: {
!/^[A-Za-z0-9+/]*={0,2}$/.test(i.data)
)
return send(400, "bad image data");
if (i.page !== undefined && i.page !== true)
return send(400, "bad image page");
}
// The body cap above is the panel's limit in base64; a client that is not the
// panel meets the limit itself here, in the bytes the files come out as.
Expand Down Expand Up @@ -493,15 +495,34 @@ export function createAgentServer(options: {
const id = randomUUID();
fs.mkdirSync(keptOf(id), { recursive: true });
const imagesDir = images.length ? keptOf(id) : "";
// A mockup's picture is kept for the panel, and the agent is pointed at its file,
// `<slug>/<file>.html` in whichever canvases folder holds that slug. A community
// project's is not on this machine, so it keeps the name `sp fetch` finds it by.
const pageOf = (name: string) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Inline the single-use page resolver

This newly added helper has only one call site (pageOf(i.name) in the held mapping), contrary to the repository's explicit rule that one-call helpers be inlined; move this resolution directly into the mapping to avoid the unnecessary jump and follow the mandated structure.

AGENTS.md reference: AGENTS.md:L128-L129

Useful? React with 👍 / 👎.

const [slug, file, ...rest] = name.split("/");
return community === undefined &&
rest.length === 0 &&
SAFE_NAME.test(slug) &&
SAFE_NAME.test(file ?? "")
? path.join(folderOf(boards, examplesDir, slug), file)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Attached mockup points at another project

After a tab switch, pageOf resolves an attached mockup against the project open at send time. The attachment keeps only slug/file.html, so the agent reads another project's board or a nonexistent file.

Learn more

A mockup is attached before its message is sent. attach sends only its slug and file name, while post sends whichever project is open when the message goes out. Resolving this name under the currently open project points to another file after switching projects.

Example: Attach shop/01-home.html in project A, switch to project B, and send "tighten #1". The agent receives B's board if B has the same slug and filename; otherwise it receives a path to a nonexistent board.

Recommended fix: Preserve the board's source project at attachment time and resolve the file from that project, including for queued messages.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2ed1ab8.

: name;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Inline the single-use path helper

pageOf has one call site in the adjacent image mapping. Repository rules ask for single-use helpers to be inlined.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2ed1ab8.

const held: AgentImage[] = images.map(
(i: { n: number; name: string; type: string; data: string }) => ({
(i: {
n: number;
name: string;
type: string;
data: string;
page?: true;
}) => ({
...i,
path: picture(
id,
`image-${i.n}`,
i.type,
Buffer.from(i.data, "base64"),
),
page: i.page && pageOf(i.name),
}),
);
const c = command(
Expand Down
22 changes: 15 additions & 7 deletions canvas/src/ChatPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ const OPEN_KEY = "sp-chat-open";

/**
* What the canvas hands the chat panel when the button is pressed (canvasAttach.tsx): a picture
* to attach to the message, or the reason none was. A board comes over as a picture too. The
* file's own name says which board it is, and the panel shows it under the tile. And the start
* to attach to the message, or the reason none was. A board comes over as a picture too, for its
* tile only: the agent is handed the board's file, which its name says, and the panel shows that
* name under the tile. And the start
* of a message, from the strip's "+" (CanvasStrip.tsx), because a canvas is only ever the
* agent's work, and a folder with no boards in it is not one. And a whole message, sent as it is,
* from the new-project dialog (AppShell.tsx), which starts the agent defining the product.
Expand All @@ -104,7 +105,8 @@ const OPEN_KEY = "sp-chat-open";
export const CANVAS_ATTACH = "sp:canvas-attach";

export type CanvasAttachDetail =
| { kind: "board"; name: string; src: string }
/** `page` for a mockup, whose drawing is only the tile's: the agent is handed its file. */
| { kind: "board"; name: string; src: string; page?: true }
| { kind: "image"; file: File }
| { kind: "error"; message: string }
| { kind: "draft"; text: string }
Expand Down Expand Up @@ -249,6 +251,8 @@ interface Attached {
url: string;
/** A board still being drawn, with no `url` yet, or one whose drawing failed. */
state?: "pending" | "failed";
/** A mockup: the agent gets its file rather than this picture of it (agents.ts). */
page?: true;
}

/**
Expand Down Expand Up @@ -713,6 +717,7 @@ export function ChatPanel(props: {
type: r.file.type,
size: r.file.size,
url: r.url,
page: t.page,
}
: t;
});
Expand Down Expand Up @@ -758,7 +763,7 @@ export function ChatPanel(props: {
* drawing asked of the server, to land in that tile. Asked for again it keeps the tile it has —
* one already there or on its way is only named again, and one that failed is drawn again.
*/
const addBoard = (name: string, src: string) => {
const addBoard = (name: string, src: string, page?: true) => {
let tile = tray.current.find((t) => t.name === name);
if (!tile && tray.current.length >= MAX_IMAGES)
return setSendError(
Expand All @@ -779,6 +784,7 @@ export function ChatPanel(props: {
size: 0,
url: "",
state: "pending",
page,
};
const next = tile;
tray.current = [...tray.current.filter((t) => t.n !== next.n), next].sort(
Expand All @@ -805,7 +811,8 @@ export function ChatPanel(props: {
// What the buttons on a canvas shape hand over (canvasAttach.tsx): a picture, attached and named
// in the sentence, or the reason there is none. A mockup arrives as a picture of itself, called
// by its own path, so pointing at one puts the same tile and the same number in the panel that
// pointing at a picture does — and the path is what the tile is captioned with.
// pointing at a picture does — and the path is what the tile is captioned with, and all the
// agent is given of it.
// No dependency list, so every render leaves a listener holding that render's `addImages` and
// its numbering — a listener that stayed would be attaching to the draft the panel had at mount.
useEffect(() => {
Expand All @@ -819,7 +826,7 @@ export function ChatPanel(props: {
// that is still being read.
if (detail.kind === "board") {
adds.current = adds.current.then(() =>
addBoard(detail.name, detail.src),
addBoard(detail.name, detail.src, detail.page),
);
return;
}
Expand Down Expand Up @@ -937,11 +944,12 @@ export function ChatPanel(props: {
agent,
model,
effort,
images: attached.map(({ n, name, type, url }) => ({
images: attached.map(({ n, name, type, url, page }) => ({
n,
name,
type,
data: url.slice(url.indexOf(",") + 1),
page,
})),
}),
});
Expand Down
22 changes: 22 additions & 0 deletions canvas/src/agents.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,28 @@ describe("AGENTS", () => {
);
});

// A mockup's picture is only the panel's tile: the agent is pointed at its file instead.
it("hands a mockup over as its file, not its picture", () => {
const board = {
n: 2,
name: "shop/01-home.html",
type: "image/png",
data: "CCC",
path: "/tmp/sp-chat-r/2.png",
page: "/proj/canvases/shop/01-home.html",
};
expect(
JSON.parse(def("claude").stdin("tighten #2", "P", [board])).message
.content,
).toEqual([
{ type: "text", text: "[Image #2] /proj/canvases/shop/01-home.html" },
{ type: "text", text: "tighten #2" },
]);
expect(def("codex").stdin("tighten #2", "P", [board])).toBe(
"P\n\n[Image #2] /proj/canvases/shop/01-home.html\n\ntighten #2",
);
});

// A session is what the agent called it on its first turn, and every later turn resumes that.
it("reads the session off the first turn and resumes it on the next", () => {
expect(
Expand Down
23 changes: 15 additions & 8 deletions canvas/src/agents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ export interface AgentImage {
/** Where the server wrote it, for an agent that takes files rather than bytes; gone once
* that agent has exited. */
path: string;
/** A mockup's HTML file, when the picture is only the panel's drawing of one: the agent is
* pointed at the file and not handed the picture, since the file is what it reads and edits. */
page?: string;
}

/** What the composer chose, handed to `args`. An empty string means the CLI decides. */
Expand Down Expand Up @@ -199,13 +202,17 @@ export const AGENTS: AgentDef[] = [
// `[Image #2]` is the marker Claude Code writes itself when a screenshot is pasted into its
// terminal, so the number arrives as something already read rather than a local convention.
stdin: (message, _preamble, images) => {
const blocks = images.flatMap((i) => [
{ type: "text", text: `[Image #${i.n}] ${i.name}` },
{
type: "image",
source: { type: "base64", media_type: i.type, data: i.data },
},
]);
const blocks = images.flatMap((i) =>
i.page
? [{ type: "text", text: `[Image #${i.n}] ${i.page}` }]
: [
{ type: "text", text: `[Image #${i.n}] ${i.name}` },
{
type: "image",
source: { type: "base64", media_type: i.type, data: i.data },
},
],
);
const content = blocks.length
? [...blocks, { type: "text", text: message }]
: message;
Expand Down Expand Up @@ -308,7 +315,7 @@ export const AGENTS: AgentDef[] = [
stdin: (message, preamble, images) =>
[
preamble,
images.map((i) => `[Image #${i.n}] ${i.path}`).join("\n"),
images.map((i) => `[Image #${i.n}] ${i.page ?? i.path}`).join("\n"),
message,
]
.filter(Boolean)
Expand Down
2 changes: 1 addition & 1 deletion canvas/src/canvasAttach.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ async function attach(editor: Editor, target: TLShape) {
`&w=${Math.max(1, Math.round(w * scale))}&h=${Math.max(1, Math.round(h * scale))}`,
window.location.href,
).href;
return dispatchAttach({ kind: "board", name, src });
return dispatchAttach({ kind: "board", name, src, page: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Add an Unreleased note for mockup attachments

The agent now receives a mockup's HTML file instead of its picture. Contributing rules require a line under ## Unreleased for user-visible changes, but this PR adds none.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2ed1ab8.

}
// One of the person's own: whatever it is, the agent gets a picture of it, named by where it
// reads the thing itself (canvasContent.ts).
Expand Down
Loading