feat(canvas): local documents and offline-style document scripts (APP-037) - #162
Merged
Merged
Conversation
Replace SQLite canvas_board persistence with file-backed documents under ~/.atmos/canvas, add Documents UI, durable document scripts (offline-style), CLI docs/script/exec verbs, and agent skill references for on-demand loading.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughCanvas persistence changes from a database-backed default board to filesystem-backed ChangesCanvas document platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| crates/core-service/src/service/canvas.rs | Adds the file-backed canvas document service with validation, serialized writes, unique temp files, and locked rename/delete paths. |
| apps/web/src/features/canvas/hooks/use-canvas-board.ts | Owns document loading, saving, active document preferences, pin targets, and delete replacement behavior. |
| apps/web/src/features/canvas/lib/canvas-agent-bus.ts | Moves script source reads, script writes, and exec behind the bridge command gate while keeping status-only reads available. |
| apps/web/src/features/canvas/components/CanvasView.tsx | Adds document controls, script host wiring, autosave behavior, and identity-based Tldraw remounting. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/web/src/features/canvas/hooks/use-canvas-board.ts:440-442
**Replacement Reuses Store**
Deleting the current document can still reopen a replacement with the same file name, so Tldraw can keep the old editor store mounted. `createNewDocument()` picks the first free `Untitled*.atmos.tldr`; after deleting the current `Untitled.atmos.tldr` or `Untitled-N.atmos.tldr`, that same name is free again and can be returned here. Since `boardIdentity` is derived from `fileName`, the Tldraw key does not change, and the deleted document's shapes can stay visible and later be saved into the new empty file. Bump the remount key or use a per-open instance key when replacing the current document after delete.
Reviews (23): Last reviewed commit: "fix(canvas): preserve pointer behavior i..." | Re-trigger Greptile
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Old web/desktop bundles still GET/PUT the removed default-board endpoint and hard-fail on 404. Map those calls onto Default.atmos.tldr so Canvas loads while clients catch up to the documents API.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/infra/src/db/migration/m20260718_000032_drop_canvas_board.rs">
<violation number="1" location="crates/infra/src/db/migration/m20260718_000032_drop_canvas_board.rs:24">
P2: Rolling back this migration restores `canvas_board` without its unique `slug` constraint, so duplicate slugs can be written before re-applying the original migration (whose `if_not_exists` table creation then skips index creation). Recreate `idx-canvas_board-slug` in `down()` after creating the table.</violation>
</file>
<file name="apps/api/src/api/dto.rs">
<violation number="1" location="apps/api/src/api/dto.rs:42">
P3: Programmatic `AtmosCanvasScriptPayload::default()` produces an empty entry instead of the documented `main.js`, so a caller that adds files to the default payload receives a validation error on save. Use a manual `Default` implementation aligned with the serde default.</violation>
</file>
<file name="apps/web/src/features/canvas/lib/document-script-session.ts">
<violation number="1" location="apps/web/src/features/canvas/lib/document-script-session.ts:44">
P2: `script-put` never returns when a document script implements a long-lived async loop (a supported use of `signal`). Start the script without awaiting its completion, while retaining error/status reporting for its eventual rejection.</violation>
</file>
<file name="crates/core-service/src/service/canvas.rs">
<violation number="1" location="crates/core-service/src/service/canvas.rs:137">
P2: Documents listing follows symlinks outside the canvas root, exposing target metadata and showing entries that `read_document` subsequently rejects. Resolve each entry through the same containment check before creating its list item.</violation>
</file>
<file name="apps/cli/src/commands/canvas.rs">
<violation number="1" location="apps/cli/src/commands/canvas.rs:455">
P3: Canvas REST commands now maintain a second HTTP/envelope client with behavior that already differs from `request_json`; a 2xx API failure can be printed as success. Reuse `api_client::request_json` so all CLI REST commands share auth, errors, and envelope semantics.</violation>
</file>
<file name="apps/api/src/api/canvas/mod.rs">
<violation number="1" location="apps/api/src/api/canvas/mod.rs:19">
P1: With LAN-without-token enabled, a host on the private network can overwrite or delete any canvas document. Route document mutations through the destructive loopback-or-token guard (rename/duplicate should be covered too).</violation>
</file>
<file name="specs/APP/APP-037_canvas-local-documents/TECH.md">
<violation number="1" location="specs/APP/APP-037_canvas-local-documents/TECH.md:185">
P2: The `AtmosCanvasFile` JSON envelope and field table in TECH.md are missing the `script` field, despite the scope summary explicitly stating that document scripts (`script` on `AtmosCanvasFile`) ship as part of this PR. Reading this section in isolation would give implementers the wrong data model. Add `script` to the JSON example and its field table, or at minimum add a note deferring the field definition to a separate section if the script internals are documented elsewhere.</violation>
</file>
<file name="apps/web/src/features/canvas/__tests__/use-canvas-board.test.ts">
<violation number="1" location="apps/web/src/features/canvas/__tests__/use-canvas-board.test.ts:92">
P2: The test "hydrates canvas terminal shapes with createCanvasSnapshot" only asserts `snapshot?.document` is truthy, but never inspects the terminal shape in the result — the test name claims to verify shape hydration but the assertion wouldn't catch missing shapes, missing fields, or broken normalization of fields like `lastAttachedAt` and `sourceTerminalTabId`.
Consider restoring concrete shape-property assertions (e.g., checking `snapshot?.document.store["shape:terminal"].props.lastAttachedAt` and `sourceTerminalTabId`) so the test actually guards the hydration logic it describes.</violation>
</file>
<file name="specs/APP/APP-037_canvas-local-documents/PRD.md">
<violation number="1" location="specs/APP/APP-037_canvas-local-documents/PRD.md:10">
P3: Cross-spec references should use relative path links per AGENTS.md convention (e.g., `../APP-014_canvas/PRD.md`) instead of bare bold text. Makes navigation harder and breaks from the pattern used by APP-015 and APP-027 PRDs.</violation>
</file>
<file name="apps/api/src/api/canvas/agent.rs">
<violation number="1" location="apps/api/src/api/canvas/agent.rs:286">
P2: The `status` function now reads the canvas directory and lists all documents, but the `dir` field always serializes as an empty string on error (`unwrap_or_else(|_| String::new())`). If `canvas_dir()` fails (e.g. home dir missing), the response silently omits the error — callers may assume a valid empty directory instead of a misconfiguration.</violation>
</file>
<file name="skills/atmos-canvas-agent/references/documents.md">
<violation number="1" location="skills/atmos-canvas-agent/references/documents.md:68">
P2: The "Minimal new file" example shows `doc-sanitize` but leaves the `doc-put` step as a bare comment with no CLI syntax. An agent following this needs to cross-reference the CLI table and guess the `--file` and `--from` arguments. Show the actual command with the sanitized filename so the step is directly actionable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "/documents/{file_name}", | ||
| get(handlers::get_document) | ||
| .put(handlers::put_document) | ||
| .delete(handlers::delete_document), |
Contributor
There was a problem hiding this comment.
P1: With LAN-without-token enabled, a host on the private network can overwrite or delete any canvas document. Route document mutations through the destructive loopback-or-token guard (rename/duplicate should be covered too).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/api/canvas/mod.rs, line 19:
<comment>With LAN-without-token enabled, a host on the private network can overwrite or delete any canvas document. Route document mutations through the destructive loopback-or-token guard (rename/duplicate should be covered too).</comment>
<file context>
@@ -10,9 +10,21 @@ use crate::app_state::AppState;
+ "/documents/{file_name}",
+ get(handlers::get_document)
+ .put(handlers::put_document)
+ .delete(handlers::delete_document),
+ )
+ .route(
</file context>
| } | ||
|
|
||
| manager | ||
| .create_table( |
Contributor
There was a problem hiding this comment.
P2: Rolling back this migration restores canvas_board without its unique slug constraint, so duplicate slugs can be written before re-applying the original migration (whose if_not_exists table creation then skips index creation). Recreate idx-canvas_board-slug in down() after creating the table.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/infra/src/db/migration/m20260718_000032_drop_canvas_board.rs, line 24:
<comment>Rolling back this migration restores `canvas_board` without its unique `slug` constraint, so duplicate slugs can be written before re-applying the original migration (whose `if_not_exists` table creation then skips index creation). Recreate `idx-canvas_board-slug` in `down()` after creating the table.</comment>
<file context>
@@ -0,0 +1,48 @@
+ }
+
+ manager
+ .create_table(
+ Table::create()
+ .table(Alias::new("canvas_board"))
</file context>
| "schema": "atmos-canvas-file.1", | ||
| "title": "Ops Desk", | ||
| "tldrawDocument": { }, | ||
| "session": { "version": 0, "isGridMode": true } |
Contributor
There was a problem hiding this comment.
P2: The AtmosCanvasFile JSON envelope and field table in TECH.md are missing the script field, despite the scope summary explicitly stating that document scripts (script on AtmosCanvasFile) ship as part of this PR. Reading this section in isolation would give implementers the wrong data model. Add script to the JSON example and its field table, or at minimum add a note deferring the field definition to a separate section if the script internals are documented elsewhere.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At specs/APP/APP-037_canvas-local-documents/TECH.md, line 185:
<comment>The `AtmosCanvasFile` JSON envelope and field table in TECH.md are missing the `script` field, despite the scope summary explicitly stating that document scripts (`script` on `AtmosCanvasFile`) ship as part of this PR. Reading this section in isolation would give implementers the wrong data model. Add `script` to the JSON example and its field table, or at minimum add a note deferring the field definition to a separate section if the script internals are documented elsewhere.</comment>
<file context>
@@ -0,0 +1,304 @@
+ "schema": "atmos-canvas-file.1",
+ "title": "Ops Desk",
+ "tldrawDocument": { },
+ "session": { "version": 0, "isGridMode": true }
+}
+```
</file context>
| pub maximized_terminal_id: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, Default, Clone)] |
Contributor
There was a problem hiding this comment.
P3: Programmatic AtmosCanvasScriptPayload::default() produces an empty entry instead of the documented main.js, so a caller that adds files to the default payload receives a validation error on save. Use a manual Default implementation aligned with the serde default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/api/dto.rs, line 42:
<comment>Programmatic `AtmosCanvasScriptPayload::default()` produces an empty entry instead of the documented `main.js`, so a caller that adds files to the default payload receives a validation error on save. Use a manual `Default` implementation aligned with the serde default.</comment>
<file context>
@@ -39,16 +39,57 @@ pub struct TerminalLayoutResponse {
pub maximized_terminal_id: Option<String>,
}
+#[derive(Debug, Serialize, Deserialize, Default, Clone)]
+pub struct AtmosCanvasScriptPayload {
+ #[serde(default = "default_script_entry")]
</file context>
| @@ -0,0 +1,111 @@ | |||
| # PRD · APP-037: Canvas Local Documents | |||
Contributor
There was a problem hiding this comment.
P3: Cross-spec references should use relative path links per AGENTS.md convention (e.g., ../APP-014_canvas/PRD.md) instead of bare bold text. Makes navigation harder and breaks from the pattern used by APP-015 and APP-027 PRDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At specs/APP/APP-037_canvas-local-documents/PRD.md, line 10:
<comment>Cross-spec references should use relative path links per AGENTS.md convention (e.g., `../APP-014_canvas/PRD.md`) instead of bare bold text. Makes navigation harder and breaks from the pattern used by APP-015 and APP-027 PRDs.</comment>
<file context>
@@ -0,0 +1,111 @@
+- **Problem**: Canvas stores one anonymous board in SQLite (`document_json`). Users cannot name boards, keep several working surfaces, or treat Canvas like a normal local document.
+- **Why now**: No production users yet — clean break is allowed. Local-first Atmos should own canvas boards as files under a single managed directory. This unblocks multi-board workflows and later file-oriented features (export, scripts, agent “which doc is open”).
+- **Related specs**:
+ - Supersedes **APP-014** persistence model (M9 single default board in DB) for storage only; Canvas overlay, terminal cards, and tools remain.
+ - Does not replace **APP-015** agent draw verbs; may extend status with active document identity (Nice).
+ - Compatible with **APP-027** widgets: widget shapes live **inside** the document file like any other shapes.
</file context>
Suggested change
| # PRD · APP-037: Canvas Local Documents | |
| - Supersedes [**APP-014**](../APP-014_canvas/PRD.md) persistence model (M9 single default board in DB) for storage only; Canvas overlay, terminal cards, and tools remain. |
| .await | ||
| } | ||
|
|
||
| async fn canvas_http_json( |
Contributor
There was a problem hiding this comment.
P3: Canvas REST commands now maintain a second HTTP/envelope client with behavior that already differs from request_json; a 2xx API failure can be printed as success. Reuse api_client::request_json so all CLI REST commands share auth, errors, and envelope semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/cli/src/commands/canvas.rs, line 455:
<comment>Canvas REST commands now maintain a second HTTP/envelope client with behavior that already differs from `request_json`; a 2xx API failure can be printed as success. Reuse `api_client::request_json` so all CLI REST commands share auth, errors, and envelope semantics.</comment>
<file context>
@@ -218,26 +344,153 @@ fn skill_dir() -> Result<Value, String> {
+ .await
+}
+
+async fn canvas_http_json(
+ api: &ApiClientArgs,
+ method: reqwest::Method,
</file context>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
apps/cli/src/commands/canvas.rs-470-478 (1)
470-478: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the response status before parsing JSON. Non-JSON 401/403/5xx bodies currently fail at
resp.json()and skip auth hints or HTTP diagnostics. Read the body as text, handle unsuccessful statuses first, then parse JSON on success.🤖 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/cli/src/commands/canvas.rs` around lines 470 - 478, Update the request response handling around `status_code` and `resp.json::<Value>()` to read the body as text first, handle unsuccessful HTTP statuses—including auth hints and diagnostics—before JSON parsing, and only parse the text as JSON for successful responses. Preserve the existing endpoint context and error propagation in the surrounding request flow.apps/cli/src/commands/canvas.rs-143-150 (1)
143-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject conflicting
--code/--fileinputs.ScriptPutArgsandExecArgsboth accept the two sources today, and the handler silently prefers--code. Add Clap conflicts so invalid input fails before execution.🤖 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/cli/src/commands/canvas.rs` around lines 143 - 150, Update the Clap definitions for ScriptPutArgs and ExecArgs to mark their code and file options as mutually exclusive, so providing both is rejected during argument parsing before the command handler runs. Keep the existing requirement that at least one source is supplied and leave CanvasCommand::Exec execution behavior unchanged.crates/core-service/src/service/canvas.rs-24-36 (1)
24-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
AtmosCanvasScript::default()valid or removeDefault.derive(Default)leavesentryempty, which violatesvalidate_atmos_canvas_script; implementDefaultmanually withmain.js, or drop the trait if this constructor isn’t intended.🤖 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 `@crates/core-service/src/service/canvas.rs` around lines 24 - 36, Update AtmosCanvasScript’s Default implementation so default() initializes entry through default_script_entry() (main.js) while retaining an empty files map, ensuring the result passes validate_atmos_canvas_script; replace the derived Default with a manual implementation rather than removing the trait.apps/web/src/features/canvas/hooks/use-canvas-board.ts-32-39 (1)
32-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize the new document titles and error messages.
Strings such as
"Untitled","Default", and the load/save/open errors are user-visible. Use the existing canvas translation namespace and add corresponding keys to every web locale.As per coding guidelines, “Avoid hardcoded user-facing labels, titles, tooltips, empty states, and error text; use existing i18n lookup patterns.”
Also applies to: 158-160, 173-175, 230-244, 260-261, 289-300, 322-323
🤖 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/web/src/features/canvas/hooks/use-canvas-board.ts` around lines 32 - 39, Replace hardcoded user-visible titles and load/save/open error messages in createDefaultDocument and the referenced canvas-board flows with lookups from the existing canvas translation namespace. Add matching translation keys and localized values to every web locale, preserving the current fallback and error behavior while removing literal strings such as “Untitled” and “Default.”Source: Coding guidelines
apps/web/src/features/canvas/lib/canvas-agent-bus.ts-197-235 (1)
197-235: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize the new command errors.
Route
exec requires…, the unregistered-session error, andscript-put requires…throughcanvasBusT, as these messages are returned to users through CLI/agent results.As per coding guidelines, “Avoid hardcoded user-facing labels, titles, tooltips, empty states, and error text; use existing i18n lookup patterns.”
🤖 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/web/src/features/canvas/lib/canvas-agent-bus.ts` around lines 197 - 235, Route the user-facing validation and session errors in the command handler through canvasBusT: the “exec requires args.code”, “Document script session is not registered”, and “script-put requires args.files or args.code...” messages. Preserve their existing error codes and retry flags while replacing hardcoded text with the established i18n lookup pattern.Source: Coding guidelines
apps/web/src/features/canvas/components/CanvasView.tsx-1373-1375 (1)
1373-1375: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the saved timestamp when the active document changes.
lastSavedAtsurvives open/new operations, so another document can display the previous document's save time. Reset it onboardIdentitychanges or initialize it from the selected document's metadata.🤖 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/web/src/features/canvas/components/CanvasView.tsx` around lines 1373 - 1375, Update the state or derived value around lastSavedAt and boardIdentity so the saved timestamp is reset whenever the active document changes. Ensure the selected document’s save metadata is used when available, without retaining the previous document’s timestamp during open or new operations.apps/web/src/features/canvas/components/CanvasDocumentScriptStatus.tsx-27-61 (1)
27-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAnnounce dynamic script status to assistive technology.
Add
role="status"/aria-live="polite"to the running state androle="alert"to the error state; otherwise these asynchronous updates are only visual.🤖 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/web/src/features/canvas/components/CanvasDocumentScriptStatus.tsx` around lines 27 - 61, Update the running-state container in CanvasDocumentScriptStatus to include role="status" and aria-live="polite", and update the error-state container to include role="alert". Preserve the existing content, styling, and state-specific rendering.apps/web/src/features/canvas/lib/canvas-agent-bus.ts-219-238 (1)
219-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject malformed script bundles instead of coercing them.
String(...)converts objects and numbers into script text, and the selected entry need not exist infiles. Require a non-empty string entry, string file names/contents, andfiles[entry]before replacing the current script.🤖 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/web/src/features/canvas/lib/canvas-agent-bus.ts` around lines 219 - 238, The script-put validation around session.setScript must reject malformed bundles instead of coercing values. Require entry to be a non-empty string, require files to be an object whose file names and contents are strings, and verify files[entry] exists before calling setScript; preserve the existing code fallback only when its entry is valid.
🧹 Nitpick comments (2)
apps/web/src/api/query/api-operation-inventory.ts (1)
529-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider changing the classification to
"query".
canvasBoardLoadcorresponds to aGETrequest to/api/canvas/documents/:file_name. Should its classification be"query"rather than"mutation", matching other read operations in the inventory?♻️ Proposed refactor
{ domain: "canvas", operation: "canvasBoardLoad", transport: "rest", - classification: "mutation", + classification: "query", legacyOwner: "use-canvas-board + /api/canvas/documents", phase: "extended", status: "complete", rationale: "APP-037: file-backed documents via REST list/get under ~/.atmos/canvas.", },🤖 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/web/src/api/query/api-operation-inventory.ts` around lines 529 - 535, Update the inventory entry for canvasBoardLoad to classify this read-only GET operation as "query" instead of "mutation", preserving its existing transport, ownership, phase, status, and rationale metadata.specs/APP/APP-037_canvas-local-documents/PRD.md (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a diagram for the document switch flow.
As per coding guidelines,
PRD.mdshould add diagrams when they clarify user-visible flows or states. The "dirty guard on switch" (M10) introduces a multi-step user interaction (Save, Discard, Cancel). Consider adding a simple Mermaid state diagram to visually clarify this flow for the engineering team.🤖 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 `@specs/APP/APP-037_canvas-local-documents/PRD.md` around lines 54 - 55, Update the M10 “Dirty guard on switch” section in PRD.md by adding a concise Mermaid state diagram that shows switching with unsaved changes and the Save, Discard, and Cancel outcomes. Keep the diagram focused on the user-visible document-switch flow and preserve the existing M10 and M11 requirements.Source: Coding guidelines
🤖 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 `@apps/api/src/api/canvas/handlers.rs`:
- Around line 16-20: Remove the decode_name helper and stop applying
urlencoding::decode to path parameters. Use the file_name value supplied by
Axum’s Path extractor directly at every handler call site, preserving legitimate
percent sequences in filenames.
- Around line 31-40: Move all synchronous canvas file-system operations off the
Tokio reactor using tokio::task::spawn_blocking: in
apps/api/src/api/canvas/handlers.rs lines 31-40 wrap list_documents; lines 42-70
wrap read_document and absolute_path; lines 72-92 wrap write_document; lines
94-103 wrap delete_document; lines 105-117 wrap rename_document; lines 119-131
wrap duplicate_document. Also update apps/api/src/api/canvas/agent.rs lines
280-314 to wrap list_documents. Preserve existing error propagation and response
behavior when joining each blocking task.
In `@apps/web/src/app-shell/CenterStage.tsx`:
- Line 910: Update the pin cleanup flow around loadPinTargetDocument and the
related lines 929-930 to persist a pinKey-to-fileName ownership association when
the pin is created or registered. During cleanup, resolve the owning fileName
from that association instead of relying on the currently active document, then
remove the terminal shape and clear bookkeeping in the owning document; preserve
existing behavior when no owner association is available.
In `@apps/web/src/features/canvas/__tests__/use-canvas-board.test.ts`:
- Around line 19-24: Update both exact object assertions for
parseAtmosCanvasFile() in the canvas board tests to include the returned script
property with a null value. Preserve the existing expected fields and values.
In `@apps/web/src/features/canvas/components/CanvasDocumentsControl.tsx`:
- Around line 6-14: Update the imports in CanvasDocumentsControl to load Button,
Input, Popover, PopoverContent, and PopoverTrigger from their prescribed
`@workspace/ui/components/ui/`* modules instead of the `@workspace/ui` barrel; keep
cn and toastManager imported from the barrel.
- Around line 466-473: Update the rename flow in CanvasDocumentsControl’s
onClick handler so renaming the active dirty document preserves the live editor
snapshot, including script, and its dirty state instead of reloading a stale
document through onRefreshList. Save the live snapshot before or as part of the
rename, or update only the filename/title while retaining the complete document
state.
- Around line 381-513: Replace the fixed overlay implementations for the
pending, saveAsOpen, renameTarget, and deleteTarget flows in
CanvasDocumentsControl with the shared workspace Dialog primitive from
`@workspace/ui/components/ui`. Preserve each modal’s existing content, actions,
state transitions, and loading/disabled behavior while using Dialog’s
trigger/content structure to provide dialog semantics, focus
trapping/restoration, and Escape handling.
In `@apps/web/src/features/canvas/components/CanvasView.tsx`:
- Around line 750-752: Update the boardIdentity-switch effect and the related
session debounce logic to cancel or flush any pending timeout for the previous
document before replacing pendingSessionRef. Reset the associated dirty refs
during the switch, ensuring an old timeout cannot write the new document’s
session under the previous boardIdentity; apply the same behavior to the logic
around the additional referenced effect.
- Around line 713-748: Make the document script lifecycle effect around
getDocumentScriptHost the sole owner of script execution: remove the direct
applyDocumentScriptBundle call from setScript in registerDocumentScriptSession,
while preserving state updates and dirty marking so document?.script changes
trigger the existing effect exactly once.
- Around line 754-780: The persistEditorSnapshot callback must not return while
documentSaveInFlightRef is set. Queue or await the active save, then re-enter
the save flow to capture a fresh editor snapshot and persist it before
dirty-switch handling continues; preserve the existing saveAs/saveDocument
behavior and in-flight flag cleanup.
In `@apps/web/src/features/canvas/hooks/use-canvas-board.ts`:
- Around line 165-170: Make whole-document mutations revision-aware to prevent
silent last-writer-wins updates: in
apps/web/src/features/canvas/hooks/use-canvas-board.ts lines 165-170, update
savePinTargetDocument to use an ETag/revision conditional write; in
apps/web/src/features/canvas/hooks/use-canvas-board.ts lines 240-259, include
the loaded revision in normal saves and surface conflicts; in
apps/web/src/features/terminal/hooks/use-terminal-grid-canvas-pins.ts lines
147-194 and apps/web/src/app-shell/CenterStage.tsx lines 924-927, retry
conflicts by reloading the document and reapplying the pin or cleanup mutation.
- Around line 303-317: Update renameDocument so renaming the active document
preserves its existing dirty state and script content instead of reconstructing
it through applyLoaded. When fileName matches targetFileName, retain the current
document’s unsaved metadata while updating the renamed file name and title,
ensuring the next save cannot overwrite the durable script.
In `@apps/web/src/features/canvas/lib/document-script-helpers.ts`:
- Around line 119-141: Update onShapeTranslate to check options?.signal?.aborted
before creating the editor.store.listen subscription; when already aborted,
return a no-op cleanup without registering the listener. Preserve the existing
abort listener and unsubscribe behavior for active signals.
In `@apps/web/src/features/canvas/lib/document-script-host.ts`:
- Around line 168-178: Update the document-script execution flow around the
dynamic import and run invocation so scripts are never auto-executed in the
privileged application realm. Before importing or calling the module, require
explicit trust derived from the document file content; untrusted or modified
content must not run automatically. Prefer moving execution into a Worker or
isolated frame with only capability-limited editor RPC, and apply the same
protection to the additional execution path noted in the comment.
- Around line 130-159: Replace the order-dependent module rewriting in the
document script host’s bundle-loading flow around the data-URL/blob-URL
construction and entry generation. Resolve the complete module graph before
rewriting any module so chained relative imports point to dependencies’ final
URLs, and ensure side-effect imports such as import "./setup.js" are handled;
alternatively, explicitly restrict this path to single-file scripts.
In `@crates/core-service/src/service/canvas.rs`:
- Around line 3-6: Refactor CanvasDocumentService so it only validates requests
and orchestrates document workflows, while an injected infrastructure repository
owns all filesystem CRUD and metadata access currently using std::fs, Path,
PathBuf, and SystemTime. Define the small repository in core-engine and route
every local-document persistence operation through it, preserving existing
service behavior and validation.
- Around line 410-435: The validate_atmos_canvas_script function currently
checks files against a trimmed entry while preserving the untrimmed persisted
value. Reject entries with surrounding whitespace by validating the original
script.entry and requiring it to equal its trimmed form before checking
files.contains_key; alternatively, normalize script.entry before persistence so
the stored entry matches the file key.
- Around line 385-388: Update the stem length handling around stem.truncate in
the canvas service to clamp MAX_STEM_LEN to the nearest preceding UTF-8
character boundary before truncating. Preserve the existing trimming behavior
and ensure multi-byte Unicode names cannot panic.
---
Minor comments:
In `@apps/cli/src/commands/canvas.rs`:
- Around line 470-478: Update the request response handling around `status_code`
and `resp.json::<Value>()` to read the body as text first, handle unsuccessful
HTTP statuses—including auth hints and diagnostics—before JSON parsing, and only
parse the text as JSON for successful responses. Preserve the existing endpoint
context and error propagation in the surrounding request flow.
- Around line 143-150: Update the Clap definitions for ScriptPutArgs and
ExecArgs to mark their code and file options as mutually exclusive, so providing
both is rejected during argument parsing before the command handler runs. Keep
the existing requirement that at least one source is supplied and leave
CanvasCommand::Exec execution behavior unchanged.
In `@apps/web/src/features/canvas/components/CanvasDocumentScriptStatus.tsx`:
- Around line 27-61: Update the running-state container in
CanvasDocumentScriptStatus to include role="status" and aria-live="polite", and
update the error-state container to include role="alert". Preserve the existing
content, styling, and state-specific rendering.
In `@apps/web/src/features/canvas/components/CanvasView.tsx`:
- Around line 1373-1375: Update the state or derived value around lastSavedAt
and boardIdentity so the saved timestamp is reset whenever the active document
changes. Ensure the selected document’s save metadata is used when available,
without retaining the previous document’s timestamp during open or new
operations.
In `@apps/web/src/features/canvas/hooks/use-canvas-board.ts`:
- Around line 32-39: Replace hardcoded user-visible titles and load/save/open
error messages in createDefaultDocument and the referenced canvas-board flows
with lookups from the existing canvas translation namespace. Add matching
translation keys and localized values to every web locale, preserving the
current fallback and error behavior while removing literal strings such as
“Untitled” and “Default.”
In `@apps/web/src/features/canvas/lib/canvas-agent-bus.ts`:
- Around line 197-235: Route the user-facing validation and session errors in
the command handler through canvasBusT: the “exec requires args.code”, “Document
script session is not registered”, and “script-put requires args.files or
args.code...” messages. Preserve their existing error codes and retry flags
while replacing hardcoded text with the established i18n lookup pattern.
- Around line 219-238: The script-put validation around session.setScript must
reject malformed bundles instead of coercing values. Require entry to be a
non-empty string, require files to be an object whose file names and contents
are strings, and verify files[entry] exists before calling setScript; preserve
the existing code fallback only when its entry is valid.
In `@crates/core-service/src/service/canvas.rs`:
- Around line 24-36: Update AtmosCanvasScript’s Default implementation so
default() initializes entry through default_script_entry() (main.js) while
retaining an empty files map, ensuring the result passes
validate_atmos_canvas_script; replace the derived Default with a manual
implementation rather than removing the trait.
---
Nitpick comments:
In `@apps/web/src/api/query/api-operation-inventory.ts`:
- Around line 529-535: Update the inventory entry for canvasBoardLoad to
classify this read-only GET operation as "query" instead of "mutation",
preserving its existing transport, ownership, phase, status, and rationale
metadata.
In `@specs/APP/APP-037_canvas-local-documents/PRD.md`:
- Around line 54-55: Update the M10 “Dirty guard on switch” section in PRD.md by
adding a concise Mermaid state diagram that shows switching with unsaved changes
and the Save, Discard, and Cancel outcomes. Keep the diagram focused on the
user-visible document-switch flow and preserve the existing M10 and M11
requirements.
🪄 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: 9231d019-c7a3-49f2-8882-dabcb269785a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
apps/api/src/api/canvas/agent.rsapps/api/src/api/canvas/handlers.rsapps/api/src/api/canvas/mod.rsapps/api/src/api/dto.rsapps/api/src/api/ws/message.rsapps/api/src/api/ws/message/fs.rsapps/api/src/api/ws/router/mod.rsapps/api/src/app_state.rsapps/api/src/main.rsapps/cli/Cargo.tomlapps/cli/src/commands/canvas.rsapps/web/messages/en.jsonapps/web/messages/zh.jsonapps/web/src/api/query/api-operation-inventory.tsapps/web/src/api/rest-api.tsapps/web/src/api/ws-api-types.tsapps/web/src/api/ws-api.tsapps/web/src/app-shell/CenterStage.tsxapps/web/src/features/canvas/__tests__/use-canvas-board.test.tsapps/web/src/features/canvas/components/CanvasDocumentScriptStatus.tsxapps/web/src/features/canvas/components/CanvasDocumentsControl.tsxapps/web/src/features/canvas/components/CanvasView.tsxapps/web/src/features/canvas/hooks/use-canvas-agent-bridge.tsapps/web/src/features/canvas/hooks/use-canvas-board.tsapps/web/src/features/canvas/lib/canvas-agent-bus.tsapps/web/src/features/canvas/lib/canvas-document-prefs.tsapps/web/src/features/canvas/lib/document-script-helpers.tsapps/web/src/features/canvas/lib/document-script-host.tsapps/web/src/features/canvas/lib/document-script-session.tsapps/web/src/features/connection/hooks/use-websocket.tsapps/web/src/features/terminal/hooks/use-terminal-grid-canvas-pins.tscrates/core-service/src/lib.rscrates/core-service/src/service/canvas.rscrates/core-service/src/service/canvas_agent_relay.rscrates/infra/src/db/entities/canvas_board.rscrates/infra/src/db/entities/mod.rscrates/infra/src/db/migration/m20260718_000032_drop_canvas_board.rscrates/infra/src/db/migration/mod.rscrates/infra/src/db/repo/canvas_board_repo.rscrates/infra/src/db/repo/mod.rsskills/atmos-canvas-agent/SKILL.mdskills/atmos-canvas-agent/references/command-reference.mdskills/atmos-canvas-agent/references/document-scripts.mdskills/atmos-canvas-agent/references/documents.mdskills/system-skills-manifest.jsonspecs/APP/APP-037_canvas-local-documents/BRAINSTORM.mdspecs/APP/APP-037_canvas-local-documents/PRD.mdspecs/APP/APP-037_canvas-local-documents/TECH.mdspecs/APP/APP-037_canvas-local-documents/TEST.mdspecs/README.md
💤 Files with no reviewable changes (6)
- crates/infra/src/db/entities/mod.rs
- crates/infra/src/db/entities/canvas_board.rs
- crates/infra/src/db/repo/canvas_board_repo.rs
- crates/infra/src/db/repo/mod.rs
- apps/api/src/api/ws/message.rs
- apps/web/src/api/ws-api.ts
Comment on lines
+31
to
+40
| pub async fn list_documents( | ||
| State(state): State<AppState>, | ||
| ) -> ApiResult<Json<ApiResponse<CanvasDocumentListResponse>>> { | ||
| let dir = state.canvas_service.canvas_dir()?; | ||
| let items = state.canvas_service.list_documents()?; | ||
| Ok(Json(ApiResponse::success(CanvasDocumentListResponse { | ||
| dir: dir.display().to_string(), | ||
| items: items.into_iter().map(item_dto).collect(), | ||
| }))) | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Blocking file I/O operations on the Tokio async reactor thread.
These handlers invoke synchronous file system methods from canvas_service (like list_documents, read_document, and write_document) directly within an async fn. This blocks the Tokio worker thread, violating the best practice against blocking calls on request threads and potentially stalling the API service under load. Wrap these synchronous operations in tokio::task::spawn_blocking to safely execute them off the async reactor.
apps/api/src/api/canvas/handlers.rs#L31-L40: wrapstate.canvas_service.list_documents()inspawn_blocking.apps/api/src/api/canvas/handlers.rs#L42-L70: wrapstate.canvas_service.read_document()andabsolute_path()inspawn_blocking.apps/api/src/api/canvas/handlers.rs#L72-L92: wrapstate.canvas_service.write_document()inspawn_blocking.apps/api/src/api/canvas/handlers.rs#L94-L103: wrapstate.canvas_service.delete_document()inspawn_blocking.apps/api/src/api/canvas/handlers.rs#L105-L117: wrapstate.canvas_service.rename_document()inspawn_blocking.apps/api/src/api/canvas/handlers.rs#L119-L131: wrapstate.canvas_service.duplicate_document()inspawn_blocking.apps/api/src/api/canvas/agent.rs#L280-L314: wrapstate.canvas_service.list_documents()inspawn_blocking.
📍 Affects 2 files
apps/api/src/api/canvas/handlers.rs#L31-L40(this comment)apps/api/src/api/canvas/handlers.rs#L42-L70apps/api/src/api/canvas/handlers.rs#L72-L92apps/api/src/api/canvas/handlers.rs#L94-L103apps/api/src/api/canvas/handlers.rs#L105-L117apps/api/src/api/canvas/handlers.rs#L119-L131apps/api/src/api/canvas/agent.rs#L280-L314
🤖 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/api/src/api/canvas/handlers.rs` around lines 31 - 40, Move all
synchronous canvas file-system operations off the Tokio reactor using
tokio::task::spawn_blocking: in apps/api/src/api/canvas/handlers.rs lines 31-40
wrap list_documents; lines 42-70 wrap read_document and absolute_path; lines
72-92 wrap write_document; lines 94-103 wrap delete_document; lines 105-117 wrap
rename_document; lines 119-131 wrap duplicate_document. Also update
apps/api/src/api/canvas/agent.rs lines 280-314 to wrap list_documents. Preserve
existing error propagation and response behavior when joining each blocking
task.
| const document = board.document_json | ||
| ? parseBoardDocument(board.document_json) | ||
| : createDefaultDocument(); | ||
| const { fileName, document } = await loadPinTargetDocument(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Clean up the document that owns the pin, not the currently active document.
After a document switch, loadPinTargetDocument() targets the new active file, leaving the terminal shape in its original file and clearing bookkeeping under the wrong filename. Persist a pinKey → fileName association and use that owner during cleanup.
Also applies to: 929-930
🤖 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/web/src/app-shell/CenterStage.tsx` at line 910, Update the pin cleanup
flow around loadPinTargetDocument and the related lines 929-930 to persist a
pinKey-to-fileName ownership association when the pin is created or registered.
During cleanup, resolve the owning fileName from that association instead of
relying on the currently active document, then remove the terminal shape and
clear bookkeeping in the owning document; preserve existing behavior when no
owner association is available.
Comment on lines
+6
to
+14
| import { | ||
| Button, | ||
| Input, | ||
| Popover, | ||
| PopoverContent, | ||
| PopoverTrigger, | ||
| cn, | ||
| toastManager, | ||
| } from "@workspace/ui"; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import atomic UI components from their prescribed module paths.
Move Button, Input, and Popover components from the root barrel to their @workspace/ui/components/ui/* modules.
As per coding guidelines, “Use @workspace/ui/components/ui/* for atomic UI components.”
🤖 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/web/src/features/canvas/components/CanvasDocumentsControl.tsx` around
lines 6 - 14, Update the imports in CanvasDocumentsControl to load Button,
Input, Popover, PopoverContent, and PopoverTrigger from their prescribed
`@workspace/ui/components/ui/`* modules instead of the `@workspace/ui` barrel; keep
cn and toastManager imported from the barrel.
Source: Coding guidelines
Comment on lines
+168
to
+178
| const mod = (await import(/* webpackIgnore: true */ entryUrl)) as ScriptModule; | ||
| if (signal.aborted) return; | ||
|
|
||
| const run = mod.default; | ||
| if (typeof run !== "function") { | ||
| throw new Error( | ||
| "Document script must default-export a function: export default function ({ editor, helpers, signal }) {}", | ||
| ); | ||
| } | ||
|
|
||
| await run({ editor, helpers, signal }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not execute document code in the application's privileged realm.
Blob imports and AsyncFunction retain access to window, storage, authenticated APIs, and Desktop bridges. A copied or edited .atmos.tldr file can therefore execute arbitrary application-origin code when opened.
At minimum, require explicit trust keyed to file content before auto-run. Prefer a Worker or isolated frame exposing only capability-limited editor RPC.
Also applies to: 229-239
🤖 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/web/src/features/canvas/lib/document-script-host.ts` around lines 168 -
178, Update the document-script execution flow around the dynamic import and run
invocation so scripts are never auto-executed in the privileged application
realm. Before importing or calling the module, require explicit trust derived
from the document file content; untrusted or modified content must not run
automatically. Prefer moving execution into a Worker or isolated frame with only
capability-limited editor RPC, and apply the same protection to the additional
execution path noted in the comment.
Comment on lines
+3
to
+6
| use std::env; | ||
| use std::fs; | ||
| use std::path::{Component, Path, PathBuf}; | ||
| use std::time::SystemTime; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move filesystem persistence behind an infra repository.
Assumption: local document storage is L1 persistence. CanvasDocumentService currently owns all std::fs operations instead of orchestrating an injected repository. Keep validation and document workflows here, but place filesystem CRUD and metadata access behind a small infra repository.
As per coding guidelines, services must orchestrate L1/L2 components, and “Do not put technical implementation details in the service layer — use core-engine.”
Also applies to: 68-317, 458-493
🤖 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 `@crates/core-service/src/service/canvas.rs` around lines 3 - 6, Refactor
CanvasDocumentService so it only validates requests and orchestrates document
workflows, while an injected infrastructure repository owns all filesystem CRUD
and metadata access currently using std::fs, Path, PathBuf, and SystemTime.
Define the small repository in core-engine and route every local-document
persistence operation through it, preserving existing service behavior and
validation.
Source: Coding guidelines
Comment on lines
+410
to
+435
| fn validate_atmos_canvas_script(script: &AtmosCanvasScript) -> Result<()> { | ||
| let entry = script.entry.trim(); | ||
| if entry.is_empty() { | ||
| return Err(ServiceError::Validation( | ||
| "Canvas script entry must not be empty".into(), | ||
| )); | ||
| } | ||
| if entry.contains("..") || entry.starts_with('/') || entry.contains('\\') { | ||
| return Err(ServiceError::Validation( | ||
| "Canvas script entry must be a relative path without ..".into(), | ||
| )); | ||
| } | ||
| if script.files.is_empty() { | ||
| return Err(ServiceError::Validation( | ||
| "Canvas script files must not be empty when script is set".into(), | ||
| )); | ||
| } | ||
| if script.files.len() > MAX_SCRIPT_FILES { | ||
| return Err(ServiceError::Validation(format!( | ||
| "Canvas script allows at most {MAX_SCRIPT_FILES} files" | ||
| ))); | ||
| } | ||
| if !script.files.contains_key(entry) { | ||
| return Err(ServiceError::Validation(format!( | ||
| "Canvas script entry `{entry}` is missing from files" | ||
| ))); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the stored script entry, not a trimmed surrogate.
entry: " main.js " with a main.js file passes validation, but the persisted entry is not a key in files. Reject surrounding whitespace or normalize the payload before saving.
let entry = script.entry.trim();
if entry.is_empty() {
return Err(ServiceError::Validation(
"Canvas script entry must not be empty".into(),
));
}
+if script.entry.as_str() != entry {
+ return Err(ServiceError::Validation(
+ "Canvas script entry must not contain surrounding whitespace".into(),
+ ));
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn validate_atmos_canvas_script(script: &AtmosCanvasScript) -> Result<()> { | |
| let entry = script.entry.trim(); | |
| if entry.is_empty() { | |
| return Err(ServiceError::Validation( | |
| "Canvas script entry must not be empty".into(), | |
| )); | |
| } | |
| if entry.contains("..") || entry.starts_with('/') || entry.contains('\\') { | |
| return Err(ServiceError::Validation( | |
| "Canvas script entry must be a relative path without ..".into(), | |
| )); | |
| } | |
| if script.files.is_empty() { | |
| return Err(ServiceError::Validation( | |
| "Canvas script files must not be empty when script is set".into(), | |
| )); | |
| } | |
| if script.files.len() > MAX_SCRIPT_FILES { | |
| return Err(ServiceError::Validation(format!( | |
| "Canvas script allows at most {MAX_SCRIPT_FILES} files" | |
| ))); | |
| } | |
| if !script.files.contains_key(entry) { | |
| return Err(ServiceError::Validation(format!( | |
| "Canvas script entry `{entry}` is missing from files" | |
| ))); | |
| fn validate_atmos_canvas_script(script: &AtmosCanvasScript) -> Result<()> { | |
| let entry = script.entry.trim(); | |
| if entry.is_empty() { | |
| return Err(ServiceError::Validation( | |
| "Canvas script entry must not be empty".into(), | |
| )); | |
| } | |
| if script.entry.as_str() != entry { | |
| return Err(ServiceError::Validation( | |
| "Canvas script entry must not contain surrounding whitespace".into(), | |
| )); | |
| } | |
| if entry.contains("..") || entry.starts_with('/') || entry.contains('\\') { | |
| return Err(ServiceError::Validation( | |
| "Canvas script entry must be a relative path without ..".into(), | |
| )); | |
| } | |
| if script.files.is_empty() { | |
| return Err(ServiceError::Validation( | |
| "Canvas script files must not be empty when script is set".into(), | |
| )); | |
| } | |
| if script.files.len() > MAX_SCRIPT_FILES { | |
| return Err(ServiceError::Validation(format!( | |
| "Canvas script allows at most {MAX_SCRIPT_FILES} files" | |
| ))); | |
| } | |
| if !script.files.contains_key(entry) { | |
| return Err(ServiceError::Validation(format!( | |
| "Canvas script entry `{entry}` is missing from files" | |
| ))); |
🤖 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 `@crates/core-service/src/service/canvas.rs` around lines 410 - 435, The
validate_atmos_canvas_script function currently checks files against a trimmed
entry while preserving the untrimmed persisted value. Reject entries with
surrounding whitespace by validating the original script.entry and requiring it
to equal its trimmed form before checking files.contains_key; alternatively,
normalize script.entry before persistence so the stored entry matches the file
key.
Avoid reloading Tldraw when saving the same open file; use portal Dialog for Save As/rename/delete and DropdownMenu for row actions so menus are not clipped.
Include document-scripts.1 and exec.1 in bridge registration, and document capability checks in the canvas agent skill so agents skip stale clients.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Reject non-overwrite writes when a document file already exists, require explicit overwrite for Save/pin updates, and remove path/hint copy from the Documents popover.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Contributor
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/features/canvas/components/CanvasView.tsx">
<violation number="1" location="apps/web/src/features/canvas/components/CanvasView.tsx:400">
P2: When the user clicks New while already on an untitled document, the `canvasRenderKey` does not change: `boardIdentity` stays `"untitled"`, `fileName` stays `null`, and `tldrawRemountKey` is not bumped. Because `document != null` remains `true` (switching from one non-null document to another), the `useMemo` never recomputes, so React keeps the existing editor store and the user sees stale shapes instead of a blank canvas.
Consider bumping `tldrawRemountKey` inside `newDocument()`, or including a monotonic counter in the key so that each New action produces a distinct render key.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ].join(":"); | ||
| }, [board?.guid, board?.updated_at, connectionBootstrapReady, document, tldrawRemountKey]); | ||
| return [boardIdentity, fileName ?? "untitled", tldrawRemountKey].join(":"); | ||
| }, [boardIdentity, connectionBootstrapReady, document != null, fileName, tldrawRemountKey]); |
Contributor
There was a problem hiding this comment.
P2: When the user clicks New while already on an untitled document, the canvasRenderKey does not change: boardIdentity stays "untitled", fileName stays null, and tldrawRemountKey is not bumped. Because document != null remains true (switching from one non-null document to another), the useMemo never recomputes, so React keeps the existing editor store and the user sees stale shapes instead of a blank canvas.
Consider bumping tldrawRemountKey inside newDocument(), or including a monotonic counter in the key so that each New action produces a distinct render key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/features/canvas/components/CanvasView.tsx, line 400:
<comment>When the user clicks New while already on an untitled document, the `canvasRenderKey` does not change: `boardIdentity` stays `"untitled"`, `fileName` stays `null`, and `tldrawRemountKey` is not bumped. Because `document != null` remains `true` (switching from one non-null document to another), the `useMemo` never recomputes, so React keeps the existing editor store and the user sees stale shapes instead of a blank canvas.
Consider bumping `tldrawRemountKey` inside `newDocument()`, or including a monotonic counter in the key so that each New action produces a distinct render key.</comment>
<file context>
@@ -384,15 +386,18 @@ export const CanvasView: React.FC = () => {
return [boardIdentity, fileName ?? "untitled", tldrawRemountKey].join(":");
- }, [boardIdentity, connectionBootstrapReady, document, fileName, tldrawRemountKey]);
+ }, [boardIdentity, connectionBootstrapReady, document != null, fileName, tldrawRemountKey]);
if (previousCanvasRenderKeyRef.current !== canvasRenderKey) {
</file context>
Create Untitled/Untitled-N files on first open and New; drop Save/Save As from the documents popover; show last-saved time under titles; toolbar shows autosave status only.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Last-saved time only appears under each document title in the Documents popover; background autosave remains without a Save button.
Previous commit accidentally emptied CanvasView.tsx; restore the file with the Save button removed and autosave status no longer in the toolbar.
Paint hover/active across the full list item including the ··· control so it no longer looks like a separate chip on a partial highlight.
…ipts
import(`./helper.js`) was left as a relative specifier and failed against
blob URLs. Match backtick dynamic imports when the path has no ${} and cover
with unit tests.
Focus/follow no longer hardcodes z=1. Compute zoom so the union bounds fit in the viewport (zoom out when needed), but never zoom in past 100% so small single widgets are not over-magnified.
next-intl rejects {{...}} as MALFORMED_ARGUMENT. Use ICU single-quoted
literals so the JSON example {"API_KEY":"..."} renders as plain text.
Stop falling back to localStorage for pin/cleanup targets. A fresh tab with no session value uses Default.atmos.tldr instead of another tab’s last-opened board. Cold-start open still uses last-opened via readActiveCanvasDocumentFileName.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Contributor
There was a problem hiding this comment.
2 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/features/canvas/lib/document-script-host.ts">
<violation number="1" location="apps/web/src/features/canvas/lib/document-script-host.ts:305">
P2: Literal template imports with `$` in a sibling filename are not rewritten, so they resolve relative to the blob entry and fail instead of loading the bundled script. Permit `$` in the match and retain the existing `${` interpolation guard.</violation>
</file>
<file name="apps/web/src/features/canvas/hooks/use-canvas-board.ts">
<violation number="1" location="apps/web/src/features/canvas/hooks/use-canvas-board.ts:146">
P2: `loadPinTargetDocument` now reads only from sessionStorage via `readTabActiveCanvasDocumentFileName()` to avoid leaking this tab's pin target into another tab's last-opened. But it still writes via `writeActiveCanvasDocumentFileName()`, which writes to both sessionStorage and localStorage — undermining the tab isolation the read change was meant to achieve.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| // import('./x') / import("./x") / import(`./x`) — literal relative only | ||
| // (no ${…} interpolation; those stay raw and will fail at load if used). | ||
| const dynMatch = rest.match(/^import\s*\(\s*(['"`])(\.(?:[^'"`$\\]|\\.)*)\1\s*\)/); |
Contributor
There was a problem hiding this comment.
P2: Literal template imports with $ in a sibling filename are not rewritten, so they resolve relative to the blob entry and fail instead of loading the bundled script. Permit $ in the match and retain the existing ${ interpolation guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/features/canvas/lib/document-script-host.ts, line 305:
<comment>Literal template imports with `$` in a sibling filename are not rewritten, so they resolve relative to the blob entry and fail instead of loading the bundled script. Permit `$` in the match and retain the existing `${` interpolation guard.</comment>
<file context>
@@ -300,14 +300,19 @@ export function rewriteRelativeImports(
- const dynMatch = rest.match(/^import\s*\(\s*(['"])(\.[^'"]+)\1\s*\)/);
+ // import('./x') / import("./x") / import(`./x`) — literal relative only
+ // (no ${…} interpolation; those stay raw and will fail at load if used).
+ const dynMatch = rest.match(/^import\s*\(\s*(['"`])(\.(?:[^'"`$\\]|\\.)*)\1\s*\)/);
if (dynMatch && isKeywordBoundary(code, i, "import")) {
- const replaced = replaceSpec(dynMatch[1]!, dynMatch[2]!);
</file context>
Suggested change
| const dynMatch = rest.match(/^import\s*\(\s*(['"`])(\.(?:[^'"`$\\]|\\.)*)\1\s*\)/); | |
| const dynMatch = rest.match(/^import\s*\(\s*(['"`])(\.(?:[^'"`\\]|\\.)*)\1\s*\)/); |
| fileName: string; | ||
| document: CanvasBoardDocument; | ||
| }> { | ||
| const preferred = readTabActiveCanvasDocumentFileName(); |
Contributor
There was a problem hiding this comment.
P2: loadPinTargetDocument now reads only from sessionStorage via readTabActiveCanvasDocumentFileName() to avoid leaking this tab's pin target into another tab's last-opened. But it still writes via writeActiveCanvasDocumentFileName(), which writes to both sessionStorage and localStorage — undermining the tab isolation the read change was meant to achieve.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/features/canvas/hooks/use-canvas-board.ts, line 146:
<comment>`loadPinTargetDocument` now reads only from sessionStorage via `readTabActiveCanvasDocumentFileName()` to avoid leaking this tab's pin target into another tab's last-opened. But it still writes via `writeActiveCanvasDocumentFileName()`, which writes to both sessionStorage and localStorage — undermining the tab isolation the read change was meant to achieve.</comment>
<file context>
@@ -135,13 +136,14 @@ export function toAtmosCanvasFile(document: CanvasBoardDocument): AtmosCanvasFil
document: CanvasBoardDocument;
}> {
- const preferred = readActiveCanvasDocumentFileName();
+ const preferred = readTabActiveCanvasDocumentFileName();
const candidates = preferred
? [preferred, DEFAULT_PIN_DOCUMENT_FILE]
</file context>
Each canvas Agent ACP Chat card gets its own instanceKey for session storage so documents/widgets no longer share one chat. ACP session ids are written into shape props and thus into .atmos.tldr on autosave for restore after reload.
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/features/agent/components/AgentChatPanel.tsx">
<violation number="1" location="apps/web/src/features/agent/components/AgentChatPanel.tsx:102">
P1: Queued prompts are not isolated between canvas agent-chat widgets: every widget in a workspace reads the same queue and can dispatch its head into its own session. Include `instanceKey` in queue/draft storage and enqueue routing, or prevent widget panels from consuming shared queued prompts.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| historyListActive: showsWideHistoryLayout, | ||
| contextOverride, | ||
| transformPrompt, | ||
| instanceKey, |
Contributor
There was a problem hiding this comment.
P1: Queued prompts are not isolated between canvas agent-chat widgets: every widget in a workspace reads the same queue and can dispatch its head into its own session. Include instanceKey in queue/draft storage and enqueue routing, or prevent widget panels from consuming shared queued prompts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/features/agent/components/AgentChatPanel.tsx, line 102:
<comment>Queued prompts are not isolated between canvas agent-chat widgets: every widget in a workspace reads the same queue and can dispatch its head into its own session. Include `instanceKey` in queue/draft storage and enqueue routing, or prevent widget panels from consuming shared queued prompts.</comment>
<file context>
@@ -92,6 +99,9 @@ export function AgentChatPanel({
historyListActive: showsWideHistoryLayout,
contextOverride,
transformPrompt,
+ instanceKey,
+ initialSessionBinding,
+ onSessionBindingChange,
</file context>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 19 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/src/features/canvas/lib/canvas-shape-focus.ts (1)
64-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.wand.hover.widthand.heightfor TldrawBoxconsistency.While Tldraw's
Boxcurrently provideswidthandheightgetters, its fundamental properties arewandh. Using.wand.his safer against future Tldraw API changes and aligns with howbounds.wandbounds.hare accessed throughout the rest of this file.
apps/web/src/features/canvas/lib/canvas-shape-focus.ts#L64-L66: Replacescreen.widthandscreen.heightwithscreen.wandscreen.hwhen calculating available space.apps/web/src/features/canvas/lib/canvas-shape-focus.ts#L97-L99: Replacescreen.widthandscreen.heightwithscreen.wandscreen.hwhen calculating page dimensions.🛠️ Proposed refactor for consistency
For
fitZoomForPageBounds(Lines 64-66):const screen = editor.getViewportScreenBounds(); - const availW = Math.max(1, screen.width - insetPx * 2); - const availH = Math.max(1, screen.height - insetPx * 2); + const availW = Math.max(1, screen.w - insetPx * 2); + const availH = Math.max(1, screen.h - insetPx * 2);For
centerCameraOnPageBounds(Lines 97-99):const screen = editor.getViewportScreenBounds(); - const pageW = screen.width / zoom; - const pageH = screen.height / zoom; + const pageW = screen.w / zoom; + const pageH = screen.h / zoom;🤖 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/web/src/features/canvas/lib/canvas-shape-focus.ts` around lines 64 - 66, The Tldraw Box dimensions use noncanonical properties in both viewport calculations. In apps/web/src/features/canvas/lib/canvas-shape-focus.ts#L64-L66 within fitZoomForPageBounds and `#L97-L99` within centerCameraOnPageBounds, replace screen.width and screen.height with screen.w and screen.h; both sites require the same direct change.apps/web/src/features/canvas/lib/canvas-agent-bus.ts (1)
191-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate dynamic imports.
You can import both
getDocumentScriptSessionandgetLiveDocumentScriptStatusin a single statement to avoid repeating the dynamic import. This keeps the code cleaner.
apps/web/src/features/canvas/lib/canvas-agent-bus.ts#L191-L198: Consolidate dynamic imports in thescript_getcommand handler.apps/web/src/features/canvas/lib/canvas-agent-bus.ts#L209-L254: Consolidate dynamic imports in thescript_putcommand handler.♻️ Proposed refactor
if (command === "script_get" || command === "script-get") { - const { getDocumentScriptSession } = await import("./document-script-session"); + const { getDocumentScriptSession, getLiveDocumentScriptStatus } = await import("./document-script-session"); const session = getDocumentScriptSession(); return ok({ script: session?.getScript() ?? null, - status: (await import("./document-script-session")).getLiveDocumentScriptStatus(), + status: getLiveDocumentScriptStatus(), }); }if (command === "script_put" || command === "script-put") { const args = input.args ?? {}; - const session = (await import("./document-script-session")).getDocumentScriptSession(); + const { getDocumentScriptSession, getLiveDocumentScriptStatus } = await import("./document-script-session"); + const session = getDocumentScriptSession(); if (!session) { return fail("EDITOR_NOT_READY", "Document script session is not registered", true); } if (args.clear === true || args.script === null) { await session.setScript(null); return ok({ cleared: true, - status: (await import("./document-script-session")).getLiveDocumentScriptStatus(), + status: getLiveDocumentScriptStatus(), }); } // ... await session.setScript({ entry, files }); return ok({ entry, files: Object.keys(files), - status: (await import("./document-script-session")).getLiveDocumentScriptStatus(), + status: getLiveDocumentScriptStatus(), }); }🤖 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/web/src/features/canvas/lib/canvas-agent-bus.ts` around lines 191 - 198, Consolidate the dynamic imports in the script_get handler at apps/web/src/features/canvas/lib/canvas-agent-bus.ts:191-198 by importing getDocumentScriptSession and getLiveDocumentScriptStatus together and reusing both bindings. Apply the same consolidation in the script_put handler at apps/web/src/features/canvas/lib/canvas-agent-bus.ts:209-254, without changing handler behavior.apps/web/src/features/agent/hooks/use-agent-chat-session-types.ts (1)
26-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor duplication between
AgentChatSessionBindingand theonSessionBindingChangecallback type.Both describe the same
{ acpSessionId, registryId, sessionCwd }triple; the callback re-declares it inline instead of reusingAgentChatSessionBinding. ConsiderRequired<AgentChatSessionBinding>(or aligning optionality) to keep the two in sync as fields are added.♻️ Proposed refactor
- onSessionBindingChange?: (binding: { - acpSessionId: string | null; - registryId: string | null; - sessionCwd: string | null; - }) => void; + onSessionBindingChange?: (binding: Required<AgentChatSessionBinding>) => void;🤖 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/web/src/features/agent/hooks/use-agent-chat-session-types.ts` around lines 26 - 59, Update UseAgentChatSessionOptions.onSessionBindingChange to reuse AgentChatSessionBinding instead of redeclaring the acpSessionId, registryId, and sessionCwd fields inline; align optionality with the callback’s current contract, using Required<AgentChatSessionBinding> if all fields must remain required.apps/web/src/features/agent/hooks/use-agent-chat-session.ts (1)
466-492: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid persisting canvas-only session bindings indefinitely.
writeAgentLastSession(contextKey, ...)runs for every connected canvas widget, but the only clears are tied to create-new-session, logout, or resume failure. Since thesecontextKeys are instance-scoped and deleted widgets never clear them,lastSessionByContextcan keep growing. Skip this write forinstanceKey-backed canvas sessions, or clear the key when the widget is removed.🤖 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/web/src/features/agent/hooks/use-agent-chat-session.ts` around lines 466 - 492, Update the useEffect that calls writeAgentLastSession so instanceKey-backed canvas sessions do not persist session bindings indefinitely; skip the write (and corresponding binding-change notification if appropriate) when the session is canvas-only, while preserving persistence for non-canvas sessions.
🤖 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 `@apps/web/src/features/workspace/components/NotePanel.tsx`:
- Around line 52-69: Update the saveNote error path in handleSave to display an
error toast to the user when saving fails, while retaining the existing
console.error logging and cleanup behavior in finally. Reuse the component’s
established toast mechanism rather than introducing a new notification pattern.
- Around line 40-44: Replace the prop-synchronization useEffect in NotePanel
with render-time state adjustment: when !isEditing && !isSaving and draftNote
differs from note ?? '', update draftNote during rendering while preserving the
current draft otherwise. Remove the effect and its dependency array, and ensure
the adjustment avoids redundant setDraftNote calls.
---
Nitpick comments:
In `@apps/web/src/features/agent/hooks/use-agent-chat-session-types.ts`:
- Around line 26-59: Update UseAgentChatSessionOptions.onSessionBindingChange to
reuse AgentChatSessionBinding instead of redeclaring the acpSessionId,
registryId, and sessionCwd fields inline; align optionality with the callback’s
current contract, using Required<AgentChatSessionBinding> if all fields must
remain required.
In `@apps/web/src/features/agent/hooks/use-agent-chat-session.ts`:
- Around line 466-492: Update the useEffect that calls writeAgentLastSession so
instanceKey-backed canvas sessions do not persist session bindings indefinitely;
skip the write (and corresponding binding-change notification if appropriate)
when the session is canvas-only, while preserving persistence for non-canvas
sessions.
In `@apps/web/src/features/canvas/lib/canvas-agent-bus.ts`:
- Around line 191-198: Consolidate the dynamic imports in the script_get handler
at apps/web/src/features/canvas/lib/canvas-agent-bus.ts:191-198 by importing
getDocumentScriptSession and getLiveDocumentScriptStatus together and reusing
both bindings. Apply the same consolidation in the script_put handler at
apps/web/src/features/canvas/lib/canvas-agent-bus.ts:209-254, without changing
handler behavior.
In `@apps/web/src/features/canvas/lib/canvas-shape-focus.ts`:
- Around line 64-66: The Tldraw Box dimensions use noncanonical properties in
both viewport calculations. In
apps/web/src/features/canvas/lib/canvas-shape-focus.ts#L64-L66 within
fitZoomForPageBounds and `#L97-L99` within centerCameraOnPageBounds, replace
screen.width and screen.height with screen.w and screen.h; both sites require
the same direct change.
🪄 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: 1b0195de-4b63-4ae6-ae74-4cc9e59590ab
📒 Files selected for processing (33)
apps/web/messages/en.jsonapps/web/messages/zh.jsonapps/web/src/app-shell/header-workspace-widgets.tsxapps/web/src/app-shell/state/agent-prompt-queue.test.tsapps/web/src/app-shell/state/use-dialog-store.tsapps/web/src/features/agent/components/AgentChatPanel.tsxapps/web/src/features/agent/components/AgentPromptComposer.tsxapps/web/src/features/agent/hooks/use-agent-chat-session-types.tsapps/web/src/features/agent/hooks/use-agent-chat-session.tsapps/web/src/features/agent/hooks/use-agent-chat-submit-handler.tsapps/web/src/features/agent/lib/agent/active-composer.tsapps/web/src/features/canvas/__tests__/canvas-agent-activity.test.tsapps/web/src/features/canvas/__tests__/canvas-shape-focus.test.tsapps/web/src/features/canvas/__tests__/canvas-widget-shape.test.tsapps/web/src/features/canvas/__tests__/document-script-host.test.tsapps/web/src/features/canvas/components/CanvasAgentOverlay.tsxapps/web/src/features/canvas/components/widgets/CanvasAgentChatWidget.tsxapps/web/src/features/canvas/hooks/use-add-atmos-widget.tsapps/web/src/features/canvas/hooks/use-canvas-agent-bridge.tsapps/web/src/features/canvas/hooks/use-canvas-board.tsapps/web/src/features/canvas/lib/canvas-agent-activity.tsapps/web/src/features/canvas/lib/canvas-agent-bus.tsapps/web/src/features/canvas/lib/canvas-document-prefs.tsapps/web/src/features/canvas/lib/canvas-shape-focus.tsapps/web/src/features/canvas/lib/canvas-widget-shape.tsapps/web/src/features/canvas/lib/document-script-host.tsapps/web/src/features/settings/components/HeaderLayoutSettingsSection.tsxapps/web/src/features/settings/store/layout-settings-store.tsapps/web/src/features/workspace/components/NotePanel.tsxapps/web/src/features/workspace/components/OverviewTab.tsxcrates/core-service/src/service/canvas.rsskills/atmos-canvas-agent/SKILL.mdskills/atmos-canvas-agent/references/document-scripts.md
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/atmos-canvas-agent/references/document-scripts.md
- apps/web/src/features/canvas/lib/document-script-host.ts
- skills/atmos-canvas-agent/SKILL.md
- crates/core-service/src/service/canvas.rs
Comment on lines
+40
to
+44
| useEffect(() => { | ||
| if (!isEditing && !isSaving) { | ||
| setDraftNote(note ?? ''); | ||
| } | ||
| }, [isEditing, isSaving, note]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fix CI lint warning: avoid setState inside useEffect for prop-driven sync.
Pipeline flags react-hooks/set-state-in-effect here — calling setDraftNote synchronously in the effect body causes an extra cascading render whenever note/isEditing/isSaving change. Use the render-time "adjusting state" pattern instead.
🔧 Proposed fix using render-time state adjustment
- const [draftNote, setDraftNote] = useState(note ?? '');
+ const [draftNote, setDraftNote] = useState(note ?? '');
+ const [syncedNote, setSyncedNote] = useState(note);
const [isSaving, setIsSaving] = useState(false);
const saveRef = useRef(false);
- useEffect(() => {
- if (!isEditing && !isSaving) {
- setDraftNote(note ?? '');
- }
- }, [isEditing, isSaving, note]);
+ if (note !== syncedNote && !isEditing && !isSaving) {
+ setSyncedNote(note);
+ setDraftNote(note ?? '');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!isEditing && !isSaving) { | |
| setDraftNote(note ?? ''); | |
| } | |
| }, [isEditing, isSaving, note]); | |
| const [draftNote, setDraftNote] = useState(note ?? ''); | |
| const [syncedNote, setSyncedNote] = useState(note); | |
| const [isSaving, setIsSaving] = useState(false); | |
| const saveRef = useRef(false); | |
| if (note !== syncedNote && !isEditing && !isSaving) { | |
| setSyncedNote(note); | |
| setDraftNote(note ?? ''); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI - Web / 2_Lint.txt
[warning] 42-42: react-hooks/set-state-in-effect: Avoid calling setState() directly within an effect. Calling setState synchronously within an effect body causes cascading renders. (https://react.dev/learn/you-might-not-need-an-effect).
🪛 GitHub Actions: CI - Web / Lint
[warning] 42-44: react-hooks/set-state-in-effect: Avoid calling setState() directly within an effect. (setDraftNote(note ?? ''))
🤖 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/web/src/features/workspace/components/NotePanel.tsx` around lines 40 -
44, Replace the prop-synchronization useEffect in NotePanel with render-time
state adjustment: when !isEditing && !isSaving and draftNote differs from note
?? '', update draftNote during rendering while preserving the current draft
otherwise. Remove the effect and its dependency array, and ensure the adjustment
avoids redundant setDraftNote calls.
Source: Pipeline failures
Comment on lines
+52
to
+69
| const handleSave = useCallback(async () => { | ||
| if (!effectivePath || saveRef.current) return; | ||
| if (draftNote === (note ?? '')) { | ||
| setIsEditing(false); | ||
| return; | ||
| } | ||
| saveRef.current = true; | ||
| setIsSaving(true); | ||
| try { | ||
| await saveNote(effectivePath, draftNote); | ||
| setIsEditing(false); | ||
| } catch (error) { | ||
| console.error('Failed to save note', error); | ||
| } finally { | ||
| saveRef.current = false; | ||
| setIsSaving(false); | ||
| } | ||
| }, [draftNote, effectivePath, note, saveNote]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Surface save failures to the user.
On saveNote rejection, only console.error runs — no toast or inline error is shown, so a failed save looks identical to nothing happening. Per coding guidelines, toasts should be used for error cases like this.
As per coding guidelines: "reserve toasts for errors, background work, cross-context outcomes, or unclear fallbacks."
🔧 Proposed fix adding error toast
} catch (error) {
console.error('Failed to save note', error);
+ toastManager.add({
+ title: t('note.saveFailedTitle'),
+ description: error instanceof Error ? error.message : String(error),
+ type: 'error',
+ });
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleSave = useCallback(async () => { | |
| if (!effectivePath || saveRef.current) return; | |
| if (draftNote === (note ?? '')) { | |
| setIsEditing(false); | |
| return; | |
| } | |
| saveRef.current = true; | |
| setIsSaving(true); | |
| try { | |
| await saveNote(effectivePath, draftNote); | |
| setIsEditing(false); | |
| } catch (error) { | |
| console.error('Failed to save note', error); | |
| } finally { | |
| saveRef.current = false; | |
| setIsSaving(false); | |
| } | |
| }, [draftNote, effectivePath, note, saveNote]); | |
| const handleSave = useCallback(async () => { | |
| if (!effectivePath || saveRef.current) return; | |
| if (draftNote === (note ?? '')) { | |
| setIsEditing(false); | |
| return; | |
| } | |
| saveRef.current = true; | |
| setIsSaving(true); | |
| try { | |
| await saveNote(effectivePath, draftNote); | |
| setIsEditing(false); | |
| } catch (error) { | |
| console.error('Failed to save note', error); | |
| toastManager.add({ | |
| title: t('note.saveFailedTitle'), | |
| description: error instanceof Error ? error.message : String(error), | |
| type: 'error', | |
| }); | |
| } finally { | |
| saveRef.current = false; | |
| setIsSaving(false); | |
| } | |
| }, [draftNote, effectivePath, note, saveNote]); |
🤖 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/web/src/features/workspace/components/NotePanel.tsx` around lines 52 -
69, Update the saveNote error path in handleSave to display an error toast to
the user when saving fails, while retaining the existing console.error logging
and cleanup behavior in finally. Reuse the component’s established toast
mechanism rather than introducing a new notification pattern.
Source: Coding guidelines
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/features/workspace/components/NotePanel.tsx">
<violation number="1" location="apps/web/src/features/workspace/components/NotePanel.tsx:115">
P2: A rejected save renders two missing translation keys, so the conflict notice/reload action shows fallback keys and emits next-intl missing-message errors. Add `note.conflict` and `note.reload` to every locale used by the app.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| <div className={cn('flex min-h-0 flex-1 flex-col overflow-hidden p-4', contentClassName)}> | ||
| {hasConflict ? ( | ||
| <div className="mb-3 flex shrink-0 items-center justify-between gap-3 rounded-md border border-warning/30 bg-warning/10 px-3 py-2 text-xs text-foreground"> | ||
| <span>{t('note.conflict')}</span> |
Contributor
There was a problem hiding this comment.
P2: A rejected save renders two missing translation keys, so the conflict notice/reload action shows fallback keys and emits next-intl missing-message errors. Add note.conflict and note.reload to every locale used by the app.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/features/workspace/components/NotePanel.tsx, line 115:
<comment>A rejected save renders two missing translation keys, so the conflict notice/reload action shows fallback keys and emits next-intl missing-message errors. Add `note.conflict` and `note.reload` to every locale used by the app.</comment>
<file context>
@@ -90,6 +110,19 @@ export const NotePanel: React.FC<NotePanelProps> = ({
<div className={cn('flex min-h-0 flex-1 flex-col overflow-hidden p-4', contentClassName)}>
+ {hasConflict ? (
+ <div className="mb-3 flex shrink-0 items-center justify-between gap-3 rounded-md border border-warning/30 bg-warning/10 px-3 py-2 text-xs text-foreground">
+ <span>{t('note.conflict')}</span>
+ <Button
+ variant="ghost"
</file context>
E2E report (expired)This report has been superseded by a newer CI - E2E run.
|
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment on lines
+440
to
+442
| const created = await canvasApi.createNewDocument(); | ||
| const res = await canvasApi.getDocument(created.item.file_name); | ||
| applyLoaded(res.file_name, parseAtmosCanvasFile(res.body)); |
Contributor
There was a problem hiding this comment.
Deleting the current document can still reopen a replacement with the same file name, so Tldraw can keep the old editor store mounted. createNewDocument() picks the first free Untitled*.atmos.tldr; after deleting the current Untitled.atmos.tldr or Untitled-N.atmos.tldr, that same name is free again and can be returned here. Since boardIdentity is derived from fileName, the Tldraw key does not change, and the deleted document's shapes can stay visible and later be saved into the new empty file. Bump the remount key or use a per-open instance key when replacing the current document after delete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/features/canvas/hooks/use-canvas-board.ts
Line: 440-442
Comment:
**Replacement Reuses Store**
Deleting the current document can still reopen a replacement with the same file name, so Tldraw can keep the old editor store mounted. `createNewDocument()` picks the first free `Untitled*.atmos.tldr`; after deleting the current `Untitled.atmos.tldr` or `Untitled-N.atmos.tldr`, that same name is free again and can be returned here. Since `boardIdentity` is derived from `fileName`, the Tldraw key does not change, and the deleted document's shapes can stay visible and later be saved into the new empty file. Bump the remount key or use a per-open instance key when replacing the current document after delete.
How can I resolve this? If you propose a fix, please make it concise.
E2E report: ✅ Passed0 passed · 0 failed · 0 flaky · 0 skipped · 1m 50s · 100% pass rate Run
Overview
All selected E2E suites passed. |
7 of 18 tasks
This branch was successfully deployed
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
1 participant
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
Implements APP-037: Canvas Local Documents and expands Canvas toward tldraw-offline-style programmable boards for agents:
~/.atmos/canvas/*.atmos.tldr(JSON envelopeatmos-canvas-file.1); drops SQLitecanvas_board/document_json(no migration of old rows).scriptfield on the document;DocumentScriptHostrunsexport default function ({ editor, helpers, signal })on open; status chip for running/error.docs/doc-*,script-get|put|status|clear,exec;statusincludesdocuments+active_documentfrom bridge.SKILL.md+ on-demandreferences/(diagrams vs scripts vs documents).Related Issue
Implements specs in
specs/APP/APP-037_canvas-local-documents/(no GitHub issue number).Type of Change
Validation
just lintjust testjust fmtScoped checks run during development:
cargo test -p core-service canvas(document service + rename/delete/duplicate + script payload validation)cargo check -p api -p atmosbun testforapps/web/src/features/canvas/__tests__/use-canvas-board.test.tsatmos canvas --helplists newdocs/doc-*/script-*/execverbsFull monorepo
just lint/just test/just fmtnot run in this session (large suite); recommended in CI.Checklist
Notes for reviewers
Default.atmos.tldr.script-putruns immediately; user must Save for disk persistence in.atmos.tldr.config.jsShapeUtil/Overlay not in this PR.Summary by cubic
Moves Canvas to local, file-backed documents under ~/.atmos/canvas/*.atmos.tldr with autosave and a simple Documents UI, plus durable document scripts with safe, isolated keyboard input and preserved pointer behavior. Adds REST/CLI support and agent integration; isolates agent‑chat widgets per instance, persists their ACP session binding, adds a workspace Note summary, and prevents stale note overwrites (APP-037).
New Features
Migration
canvas_boardremoved (table dropped); WScanvas_get_default_board/canvas_update_default_boardand REST/api/canvas/defaultremoved.script-putruns immediately; changes persist via autosave in the open document.Written for commit 5cf7a69. Summary will update on new commits.
Summary by CodeRabbit
execoption.atmos canvascommands for document and script management, plus on-demand command reference docs.