feat(projects): per-project canvas board (tldraw + SSE)#270
Merged
Conversation
Playwright is not yet scaffolded in this repo; the spec is committed as documented test code. Header comment explains how to wire up Playwright before running. Vitest exclude added so tests/**/*.spec.ts files are skipped by the unit-test runner.
…te + app wiring) The route was reading request.app.state.project_broker, but app.py sets project_event_broker. Both routes (SSE stream + permission toggle) would have raised AttributeError in production. Tests passed because the two fixtures used the same wrong key, masking the bug. Aligns with tinyagentos/routes/projects.py:531 and app.py:410, 777.
📝 WalkthroughWalkthroughAdds a tldraw-based project canvas: frontend components and shapes, a Zustand client store, REST + SSE integration, backend SQL persistence with permission checks, a snapshotter (TLDR + PNG), link unfurling, MCP tools, tests, and dependency updates (Pillow, Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant UI as Canvas UI (tldraw)
participant API as Project API
participant Store as ProjectCanvasStore (DB)
participant Broker as Event Broker
participant SSE as SSE clients
User->>UI: create/update/delete shape
UI->>API: POST/PATCH/DELETE /api/projects/:id/canvas/elements
API->>Store: add/update/delete element
Store-->>API: element result
API->>Broker: publish canvas.event (added/updated/deleted/permission_changed)
Broker->>SSE: deliver event to subscribers
SSE-->>UI: stream data: canvas.* event
UI->>UI: reflect remote changes in editor
sequenceDiagram
participant Broker as Event Broker
participant Snap as CanvasSnapshotter
participant Store as ProjectCanvasStore
participant Renderer as PNG Renderer
participant FS as File System
Broker->>Snap: canvas.* event
Snap->>Snap: mark_dirty(project_id)
Note over Snap: debounce timer elapses
Snap->>Store: list_elements(project_id)
Store-->>Snap: elements list
Snap->>Renderer: render_snapshot_png(elements)
Renderer->>FS: write snapshot.png
FS-->>Renderer: path
Renderer-->>Snap: output path
Snap->>FS: write board.tldr (atomic replace)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 1/3 review remaining, refill in 35 minutes and 28 seconds. Comment |
The canvas PNG renderer imports PIL but Pillow was only listed as a worker extra. CI installs '.[dev]' so the import failed at test collection time on all four Python versions. Move Pillow to main deps.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/projects/test_canvas_integration.py (1)
184-188:⚠️ Potential issue | 🟠 MajorSSE event field assertion uses wrong key.
Line 188 asserts
evt["type"], but the stream payload useskind. This will fail even when the endpoint is correct.✅ Suggested fix
- assert evt["type"] == "canvas.element_added" + assert evt["kind"] == "canvas.element_added"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_canvas_integration.py` around lines 184 - 188, The test currently asserts the SSE event field using evt["type"] which is incorrect because the stream payload uses the "kind" key; update the assertion in the test (where evt is checked in tests/projects/test_canvas_integration.py) to assert evt["kind"] (and adjust any related expectations/strings to match the "kind" value) so the assertion validates the actual payload shape emitted by the endpoint.
🧹 Nitpick comments (5)
tinyagentos/projects/ids.py (1)
4-4: LGTM! Clean addition of canvas element prefix.The addition of
"cve"to the allowed prefixes is straightforward and correctly enables ID generation for canvas elements. The change follows the existing pattern and introduces no breaking changes.Would you like me to verify that test coverage exists for this ID generation module?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/ids.py` at line 4, ID_PREFIXES was updated to include "cve" but there are no tests referenced; add/extend unit tests to assert that the new prefix is accepted and produces valid IDs by exercising the ID generation and validation code (e.g., functions named generate_id, parse_id, or is_valid_id) and by asserting IDs start with "cve" and conform to the existing format; update or add tests to cover both generation and parsing/validation paths to prevent regressions.desktop/src/apps/ProjectsApp/canvas/CanvasView.tsx (1)
7-7: Prefer container-relative sizing over fixed viewport subtraction.Line 7 hard-codes
calc(100vh - 100px), which is brittle if parent layout/header sizes change. Using parent-driven height (100%+minHeight: 0) is usually safer.Optional tweak
- <div style={{ height: "calc(100vh - 100px)", padding: 0 }}> + <div style={{ height: "100%", minHeight: 0, padding: 0 }}> <CanvasBoard projectId={projectId} projectSlug={projectSlug} /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/ProjectsApp/canvas/CanvasView.tsx` at line 7, The inline style on the root container div in CanvasView.tsx uses a brittle fixed viewport subtraction ("calc(100vh - 100px)"); change it to use container-relative sizing by setting height to "100%" and adding minHeight: 0 on that div (and ensure the parent container/layout allows stretching) so the canvas grows to the parent rather than relying on a hard-coded viewport offset.tinyagentos/projects/canvas/unfurl.py (1)
67-70: Avoid silent parser failures.Line 67-70 swallows parser exceptions with no signal. Consider debug logging so bad upstream HTML is diagnosable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/unfurl.py` around lines 67 - 70, The parser call p.feed(html) currently swallows all exceptions; change the except block to capture the exception (e.g., except Exception as e) and emit a debug/error log that includes the exception message and a safe/truncated snippet of html to aid diagnosis (reference p.feed and the html variable in unfurl.py), then allow the function to continue or re-raise based on existing error-handling conventions so parser failures are visible in logs.tinyagentos/projects/canvas/__init__.py (1)
10-10: Sort__all__to satisfy RuffRUF022.Keep exported symbol ordering deterministic to avoid lint noise.
Diff
-__all__ = ["ProjectCanvasStore", "CanvasPermissionError"] +__all__ = ["CanvasPermissionError", "ProjectCanvasStore"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/__init__.py` at line 10, The exported __all__ list is not alphabetically ordered, triggering Ruff RUF022; update the __all__ assignment in tinyagentos/projects/canvas/__init__.py so the symbols are sorted deterministically (place "CanvasPermissionError" before "ProjectCanvasStore") to keep exports stable and satisfy the linter.tests/projects/test_canvas_store.py (1)
126-126: Rename the unused unpacked variable to satisfy RuffRUF059.Use a dummy name for the unused fixture value.
Diff
- cs, ps = store_with_member + cs, _ps = store_with_member🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_canvas_store.py` at line 126, The unpacked fixture value "ps" is unused and triggers Ruff RUF059; change the assignment from "cs, ps = store_with_member" to use a dummy name (e.g., "cs, _ = store_with_member" or "cs, _ps = store_with_member") in the test so the unused value is prefixed with an underscore; ensure you update any further references (if any) to match the new dummy name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/ProjectsApp/canvas/canvas-api.ts`:
- Around line 75-80: The deleteElement function should reject when the DELETE
fails instead of returning false: in deleteElement (canvas-api.ts) check the
fetch response r.ok and if not ok throw an Error (include r.status and/or await
r.text() for details) so callers receive a rejected promise on 403/404;
otherwise return true. Update callers if they expect boolean to handle the
thrown error.
In `@desktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsx`:
- Around line 30-38: The initial HTTP fetch (canvasApi.listElements) can
overwrite live SSE updates because subscribeCanvasStream is started before the
fetch settles; to fix, after the fetch resolves (and after the cancelled check)
verify the current cache state and only call
cacheRef.current.getState().seed(elements) and hydrateEditor(editorRef.current,
elements, projectSlug) if the cache is still empty/unseeded (e.g., check
cacheRef.current.getState().items.length === 0 or an isSeeded flag on the cache
state); keep the subscribeCanvasStream(projectId, cacheRef.current) call as-is
and retain cancelled handling so late fetch responses don’t clobber live updates
from subscribeCanvasStream.
- Around line 116-131: elementToShape currently omits CanvasElement.z_index so
stacking is lost; map z_index into the shape and round-trip it in the inverse
mapping/update handlers too. Add a top-level stacking field (e.g. z or z_index)
on the returned shape object in elementToShape: set it to el.z_index (for images
and non-images alike), and update the corresponding reverse conversion/update
logic (the function that converts editor shapes back to CanvasElement at 141-162
and any payload builders for server updates) to read that shape field and
populate CanvasElement.z_index when sending saves/SSE updates. Ensure the same
symbol name (z_index) is used consistently across mappings and update payloads.
- Around line 116-131: elementToShape currently maps every element with kind
"user_shape" to a generic "geo" type and drops the original tldraw shape, so
shapes stored via pushAdd (which writes kind: "user_shape" +
payload.tldraw_shape) don't round-trip; modify elementToShape to detect el.kind
=== "user_shape" and, when el.payload?.tldraw_shape exists, return the stored
tldraw_shape (using its type and props) merged with the baseProps and the
id/position/rotation fields (and include project_slug for images) instead of
forcing shapeType(el.kind); also update the corresponding inverse/persistence
conversion used by pushAdd to ensure it writes payload.tldraw_shape unchanged so
arrows/drawings retain their original type and props on reload/SSE.
In `@desktop/src/apps/ProjectsApp/ProjectMembers.tsx`:
- Around line 58-73: The handlers calling canvasApi.setPermission and
projectsApi.members.remove should await refresh() and handle async failures to
avoid stale UI: wrap the async work in try/catch inside the onChange and delete
button callbacks, await the API call, then await refresh(), call onChanged()
only after successful refresh, and in the catch log or surface the error (e.g.,
show toast or processLogger) so failures don't silently leave the UI out of
sync; update the anonymous handlers that call canvasApi.setPermission,
projectsApi.members.remove, refresh, and onChanged accordingly.
In `@desktop/tests/canvas-e2e.spec.ts`:
- Around line 21-22: The test currently uses hard-coded project slugs
("e2e-canvas" and "e2e-sse") which can collide across runs; change the project
creation in desktop/tests/canvas-e2e.spec.ts (the create/project setup where
data: { name: "E2E Canvas", slug: "e2e-canvas", ... } and the similar "e2e-sse"
usage) to generate a unique slug at runtime (e.g., append a timestamp or
random/UUID suffix) and use that generated slug for any subsequent lookups or
assertions so tests do not collide on reruns; ensure any places reading the slug
reference the variable rather than the literal string.
In `@tests/projects/test_routes_canvas.py`:
- Around line 185-188: The test currently asserts evt["type"] ==
"canvas.element_added" but the SSE payload uses the key "kind"; update the
assertion to check evt["kind"] == "canvas.element_added" (locate the test
variables data_lines and evt in tests/projects/test_routes_canvas.py) so the SSE
JSON envelope is validated against the correct field.
In `@tinyagentos/projects/canvas/snapshotter.py`:
- Around line 188-197: The export sets a constant index "a1" for every shape,
losing layer/stacking order; update the loop that writes to store (the code that
assigns store[f"shape:{el['id']}"] and its "index" field) to compute a proper
index based on the element's position in elements (or its stored z-order)
instead of the hardcoded "a1" so relative stacking is preserved when importing
snapshots; use the element's list index or its existing order property (e.g., el
index or el["order"]) to generate unique, monotonically increasing indices per
shape.
- Around line 82-107: backfill_active only subscribes existing active projects
at startup so projects that become active later never get a broker loop; fix by
wiring a project-status change handler that calls _ensure_subscribed for
newly-active projects. Specifically, register a listener on the project store
(or the code-path that updates project status) that, when it sees a project
transition to status == "active", calls mark_dirty(project_id) and schedules
_ensure_subscribed(project_id) (use asyncio.create_task if called from a sync
callback) so the _broker_queues/_broker_tasks are created the same way as in
backfill_active/_ensure_subscribed.
In `@tinyagentos/projects/canvas/store.py`:
- Around line 160-177: The reloads use self.get_element(element_id) which can
return an element from a different project; change both the empty-patch fast
path and the post-UPDATE reload to fetch/check by project too (e.g., call
get_element with project_id or verify the returned object's project_id matches)
so you only accept an element that belongs to the given project_id and is not
soft-deleted; if the lookup fails or the project_id mismatches, raise the same
ValueError and avoid calling _publish("canvas.element_updated", ...) or
returning the wrong element.
- Around line 75-89: add_element currently enforces kind and author_kind checks
but never calls _check_edit_permission, so agents can still create canvas
elements; update add_element to call
self._check_edit_permission(project_id=project_id, author_kind=author_kind,
author_id=author_id) (or the existing signature of _check_edit_permission)
before allowing creation, ensuring the permission check runs for both
author_kind "agent" and "user" and raising the same permission error path when
it fails; place the call after validating author_kind and kind but before any DB
insert/return so creation is blocked when edit permission is denied.
In `@tinyagentos/projects/canvas/unfurl.py`:
- Around line 23-31: The _http_get function currently eagerly reads r.text, so
update it to stream the response and enforce content-type and size limits: open
the request with httpx.AsyncClient.stream (or client.get(..., stream=True)),
inspect response.headers for Content-Type and optional Content-Length, reject or
return an error if the type is disallowed or Content-Length exceeds a configured
max, then read at most MAX_BYTES from the stream (using r.aiter_bytes or
r.aiter_raw/read with a byte counter) and decode only those bytes to text (or
return a binary-safe error) so you never materialize arbitrarily large or binary
payloads; keep the function signature _http_get and return an appropriate status
code and truncated/typed body string when limits are hit.
- Around line 113-121: fetch_link_metadata currently fetches arbitrary user URLs
(via _http_get) allowing SSRF; before calling _http_get validate the URL: parse
with urllib.parse to ensure only allowed schemes (https, maybe http if
required), reject non-http(s) schemes, and normalize hostnames; then resolve the
hostname to IP(s) using socket.getaddrinfo and check each IP with the ipaddress
module to block localhost (127.0.0.0/8, ::1), RFC1918 private ranges, link-local
(169.254.0.0/16, fe80::/10) and other non-routable ranges; also enforce a
host/IP allowlist if provided (e.g., ALLOWED_HOSTS/ALLOWED_IPS) and return
_fallback(url) immediately when validation fails; finally only call _http_get
after successful validation. Reference symbols: function fetch_link_metadata,
helper _http_get, and _fallback for where to bail out on invalid targets.
---
Outside diff comments:
In `@tests/projects/test_canvas_integration.py`:
- Around line 184-188: The test currently asserts the SSE event field using
evt["type"] which is incorrect because the stream payload uses the "kind" key;
update the assertion in the test (where evt is checked in
tests/projects/test_canvas_integration.py) to assert evt["kind"] (and adjust any
related expectations/strings to match the "kind" value) so the assertion
validates the actual payload shape emitted by the endpoint.
---
Nitpick comments:
In `@desktop/src/apps/ProjectsApp/canvas/CanvasView.tsx`:
- Line 7: The inline style on the root container div in CanvasView.tsx uses a
brittle fixed viewport subtraction ("calc(100vh - 100px)"); change it to use
container-relative sizing by setting height to "100%" and adding minHeight: 0 on
that div (and ensure the parent container/layout allows stretching) so the
canvas grows to the parent rather than relying on a hard-coded viewport offset.
In `@tests/projects/test_canvas_store.py`:
- Line 126: The unpacked fixture value "ps" is unused and triggers Ruff RUF059;
change the assignment from "cs, ps = store_with_member" to use a dummy name
(e.g., "cs, _ = store_with_member" or "cs, _ps = store_with_member") in the test
so the unused value is prefixed with an underscore; ensure you update any
further references (if any) to match the new dummy name.
In `@tinyagentos/projects/canvas/__init__.py`:
- Line 10: The exported __all__ list is not alphabetically ordered, triggering
Ruff RUF022; update the __all__ assignment in
tinyagentos/projects/canvas/__init__.py so the symbols are sorted
deterministically (place "CanvasPermissionError" before "ProjectCanvasStore") to
keep exports stable and satisfy the linter.
In `@tinyagentos/projects/canvas/unfurl.py`:
- Around line 67-70: The parser call p.feed(html) currently swallows all
exceptions; change the except block to capture the exception (e.g., except
Exception as e) and emit a debug/error log that includes the exception message
and a safe/truncated snippet of html to aid diagnosis (reference p.feed and the
html variable in unfurl.py), then allow the function to continue or re-raise
based on existing error-handling conventions so parser failures are visible in
logs.
In `@tinyagentos/projects/ids.py`:
- Line 4: ID_PREFIXES was updated to include "cve" but there are no tests
referenced; add/extend unit tests to assert that the new prefix is accepted and
produces valid IDs by exercising the ID generation and validation code (e.g.,
functions named generate_id, parse_id, or is_valid_id) and by asserting IDs
start with "cve" and conform to the existing format; update or add tests to
cover both generation and parsing/validation paths to prevent regressions.
🪄 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 Plus
Run ID: d8f34f22-d501-4bb8-9127-722471f9894e
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
desktop/package.jsondesktop/src/apps/ProjectsApp/ProjectMembers.tsxdesktop/src/apps/ProjectsApp/ProjectWorkspace.tsxdesktop/src/apps/ProjectsApp/__tests__/canvas-api.test.tsdesktop/src/apps/ProjectsApp/__tests__/canvas-store.test.tsdesktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsxdesktop/src/apps/ProjectsApp/canvas/CanvasView.tsxdesktop/src/apps/ProjectsApp/canvas/canvas-api.tsdesktop/src/apps/ProjectsApp/canvas/canvas-sse.tsdesktop/src/apps/ProjectsApp/canvas/canvas-store.tsdesktop/src/apps/ProjectsApp/canvas/shapes/ImageShape.tsxdesktop/src/apps/ProjectsApp/canvas/shapes/LinkShape.tsxdesktop/src/apps/ProjectsApp/canvas/shapes/NoteShape.tsxdesktop/src/lib/projects.tsdesktop/tests/canvas-e2e.spec.tsdesktop/vite.config.tstests/projects/test_canvas_integration.pytests/projects/test_canvas_mcp_tools.pytests/projects/test_canvas_render.pytests/projects/test_canvas_snapshotter.pytests/projects/test_canvas_store.pytests/projects/test_canvas_unfurl.pytests/projects/test_routes_canvas.pytinyagentos/app.pytinyagentos/projects/canvas/__init__.pytinyagentos/projects/canvas/mcp_tools.pytinyagentos/projects/canvas/render.pytinyagentos/projects/canvas/snapshotter.pytinyagentos/projects/canvas/store.pytinyagentos/projects/canvas/unfurl.pytinyagentos/projects/ids.pytinyagentos/projects/project_store.pytinyagentos/routes/project_canvas.py
Comment on lines
+75
to
+80
| async deleteElement(projectId: string, elementId: string): Promise<boolean> { | ||
| const r = await fetch( | ||
| `/api/projects/${projectId}/canvas/elements/${elementId}`, | ||
| { method: "DELETE" }, | ||
| ); | ||
| return r.ok; |
There was a problem hiding this comment.
Surface DELETE failures by rejecting the promise.
This currently returns false on 403/404, but the caller only handles rejections. In the current CanvasBoard flow that makes failed deletes look successful and leaves the local board diverged until reload.
Proposed fix
async deleteElement(projectId: string, elementId: string): Promise<boolean> {
const r = await fetch(
`/api/projects/${projectId}/canvas/elements/${elementId}`,
{ method: "DELETE" },
);
- return r.ok;
+ if (!r.ok) {
+ const body = await r.text();
+ throw new Error(`canvas-api ${r.status}: ${body}`);
+ }
+ return true;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/canvas/canvas-api.ts` around lines 75 - 80, The
deleteElement function should reject when the DELETE fails instead of returning
false: in deleteElement (canvas-api.ts) check the fetch response r.ok and if not
ok throw an Error (include r.status and/or await r.text() for details) so
callers receive a rejected promise on 403/404; otherwise return true. Update
callers if they expect boolean to handle the thrown error.
Comment on lines
+30
to
+38
| useEffect(() => { | ||
| let cancelled = false; | ||
| (async () => { | ||
| const elements = await canvasApi.listElements(projectId); | ||
| if (cancelled) return; | ||
| cacheRef.current.getState().seed(elements); | ||
| hydrateEditor(editorRef.current, elements, projectSlug); | ||
| })(); | ||
| const unsub = subscribeCanvasStream(projectId, cacheRef.current); |
There was a problem hiding this comment.
Avoid clobbering live SSE updates with the initial fetch result.
The subscription is opened before listElements() settles, but seed(elements) still replaces the cache when that older HTTP response arrives. If another client edits the board in that window, the later seed rolls the local state back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsx` around lines 30 - 38,
The initial HTTP fetch (canvasApi.listElements) can overwrite live SSE updates
because subscribeCanvasStream is started before the fetch settles; to fix, after
the fetch resolves (and after the cancelled check) verify the current cache
state and only call cacheRef.current.getState().seed(elements) and
hydrateEditor(editorRef.current, elements, projectSlug) if the cache is still
empty/unseeded (e.g., check cacheRef.current.getState().items.length === 0 or an
isSeeded flag on the cache state); keep the subscribeCanvasStream(projectId,
cacheRef.current) call as-is and retain cancelled handling so late fetch
responses don’t clobber live updates from subscribeCanvasStream.
Comment on lines
+116
to
+131
| function elementToShape(el: CanvasElement, projectSlug: string): any { | ||
| const baseProps = { | ||
| w: el.w, h: el.h, | ||
| taos_kind: el.kind, | ||
| taos_payload: el.payload, | ||
| taos_author_id: el.author_id, | ||
| taos_author_kind: el.author_kind, | ||
| }; | ||
| return { | ||
| id: `shape:${el.id}`, | ||
| type: shapeType(el.kind), | ||
| x: el.x, y: el.y, rotation: el.rotation, | ||
| props: el.kind === "image" | ||
| ? { ...baseProps, project_slug: projectSlug } | ||
| : baseProps, | ||
| }; |
There was a problem hiding this comment.
Persist stacking order too.
CanvasElement.z_index never gets mapped into the editor or sent back on updates. Overlapping items will come back in a default order after reload/SSE hydration, so users can lose intentional layering.
Also applies to: 141-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsx` around lines 116 - 131,
elementToShape currently omits CanvasElement.z_index so stacking is lost; map
z_index into the shape and round-trip it in the inverse mapping/update handlers
too. Add a top-level stacking field (e.g. z or z_index) on the returned shape
object in elementToShape: set it to el.z_index (for images and non-images
alike), and update the corresponding reverse conversion/update logic (the
function that converts editor shapes back to CanvasElement at 141-162 and any
payload builders for server updates) to read that shape field and populate
CanvasElement.z_index when sending saves/SSE updates. Ensure the same symbol
name (z_index) is used consistently across mappings and update payloads.
user_shape does not round-trip through persistence.
pushAdd() stores native shapes as kind: "user_shape" plus payload.tldraw_shape, but elementToShape() ignores that record and always recreates a generic "geo" shape. After reload/SSE, arrows, drawings, and other non-custom shapes lose their original type and props.
Also applies to: 141-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/canvas/CanvasBoard.tsx` around lines 116 - 131,
elementToShape currently maps every element with kind "user_shape" to a generic
"geo" type and drops the original tldraw shape, so shapes stored via pushAdd
(which writes kind: "user_shape" + payload.tldraw_shape) don't round-trip;
modify elementToShape to detect el.kind === "user_shape" and, when
el.payload?.tldraw_shape exists, return the stored tldraw_shape (using its type
and props) merged with the baseProps and the id/position/rotation fields (and
include project_slug for images) instead of forcing shapeType(el.kind); also
update the corresponding inverse/persistence conversion used by pushAdd to
ensure it writes payload.tldraw_shape unchanged so arrows/drawings retain their
original type and props on reload/SSE.
Comment on lines
+58
to
+73
| onChange={async (e) => { | ||
| await canvasApi.setPermission(project.id, m.member_id, e.target.checked); | ||
| refresh(); | ||
| onChanged(); | ||
| }} | ||
| /> | ||
| <span className="text-xs">Can edit canvas</span> | ||
| </label> | ||
| )} | ||
| <button | ||
| type="button" | ||
| onClick={async () => { | ||
| await projectsApi.members.remove(project.id, m.member_id); | ||
| refresh(); | ||
| onChanged(); | ||
| }} |
There was a problem hiding this comment.
Handle async failures and await refresh to prevent stale member state.
Both action handlers can reject before refresh runs, and refresh() is currently fire-and-forget. This can leave permission/member UI out of sync.
Diff
<input
type="checkbox"
checked={!!m.can_edit_canvas}
onChange={async (e) => {
- await canvasApi.setPermission(project.id, m.member_id, e.target.checked);
- refresh();
- onChanged();
+ const checked = e.currentTarget.checked;
+ try {
+ await canvasApi.setPermission(project.id, m.member_id, checked);
+ } finally {
+ await refresh();
+ onChanged();
+ }
}}
/>
@@
<button
type="button"
onClick={async () => {
- await projectsApi.members.remove(project.id, m.member_id);
- refresh();
- onChanged();
+ try {
+ await projectsApi.members.remove(project.id, m.member_id);
+ } finally {
+ await refresh();
+ onChanged();
+ }
}}
className="text-xs text-red-400 hover:underline"
aria-label={`Remove ${m.member_id}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/ProjectsApp/ProjectMembers.tsx` around lines 58 - 73, The
handlers calling canvasApi.setPermission and projectsApi.members.remove should
await refresh() and handle async failures to avoid stale UI: wrap the async work
in try/catch inside the onChange and delete button callbacks, await the API
call, then await refresh(), call onChanged() only after successful refresh, and
in the catch log or surface the error (e.g., show toast or processLogger) so
failures don't silently leave the UI out of sync; update the anonymous handlers
that call canvasApi.setPermission, projectsApi.members.remove, refresh, and
onChanged accordingly.
Comment on lines
+21
to
+22
| data: { name: "E2E Canvas", slug: "e2e-canvas", description: "" }, | ||
| }); |
There was a problem hiding this comment.
Avoid hard-coded project slugs in E2E setup.
Line 21 and Line 44 use static slugs (e2e-canvas, e2e-sse), which can collide on reruns and make this suite flaky.
💡 Suggested fix
- const created = await request.post("/api/projects", {
- data: { name: "E2E Canvas", slug: "e2e-canvas", description: "" },
- });
+ const slug = `e2e-canvas-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
+ const created = await request.post("/api/projects", {
+ data: { name: "E2E Canvas", slug, description: "" },
+ });- const created = await request.post("/api/projects", {
- data: { name: "E2E SSE", slug: "e2e-sse", description: "" },
- });
+ const slug = `e2e-sse-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
+ const created = await request.post("/api/projects", {
+ data: { name: "E2E SSE", slug, description: "" },
+ });Also applies to: 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/tests/canvas-e2e.spec.ts` around lines 21 - 22, The test currently
uses hard-coded project slugs ("e2e-canvas" and "e2e-sse") which can collide
across runs; change the project creation in desktop/tests/canvas-e2e.spec.ts
(the create/project setup where data: { name: "E2E Canvas", slug: "e2e-canvas",
... } and the similar "e2e-sse" usage) to generate a unique slug at runtime
(e.g., append a timestamp or random/UUID suffix) and use that generated slug for
any subsequent lookups or assertions so tests do not collide on reruns; ensure
any places reading the slug reference the variable rather than the literal
string.
Comment on lines
+188
to
+197
| for el in elements: | ||
| store[f"shape:{el['id']}"] = { | ||
| "id": f"shape:{el['id']}", | ||
| "typeName": "shape", | ||
| "type": _tldraw_shape_type(el["kind"]), | ||
| "x": el["x"], | ||
| "y": el["y"], | ||
| "rotation": el["rotation"], | ||
| "index": "a1", | ||
| "parentId": "page:page", |
There was a problem hiding this comment.
The .tldr export drops layer ordering.
Every shape is serialized with index: "a1" regardless of its stored order. That means overlapping elements cannot preserve their stacking in exported snapshots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tinyagentos/projects/canvas/snapshotter.py` around lines 188 - 197, The
export sets a constant index "a1" for every shape, losing layer/stacking order;
update the loop that writes to store (the code that assigns
store[f"shape:{el['id']}"] and its "index" field) to compute a proper index
based on the element's position in elements (or its stored z-order) instead of
the hardcoded "a1" so relative stacking is preserved when importing snapshots;
use the element's list index or its existing order property (e.g., el index or
el["order"]) to generate unique, monotonically increasing indices per shape.
Comment on lines
+75
to
+89
| async def add_element( | ||
| self, | ||
| *, | ||
| project_id: str, | ||
| element: dict, | ||
| author_kind: str, | ||
| author_id: str, | ||
| ) -> dict: | ||
| kind = element.get("kind") | ||
| if kind not in _VALID_KINDS: | ||
| raise ValueError(f"invalid kind: {kind}") | ||
| if author_kind == "agent" and kind not in _AGENT_ALLOWED_KINDS: | ||
| raise ValueError(f"agents may not emit kind={kind}") | ||
| if author_kind not in ("user", "agent"): | ||
| raise ValueError(f"invalid author_kind: {author_kind}") |
There was a problem hiding this comment.
Enforce can_edit_canvas on agent creates too.
add_element() validates agent kinds, but it never calls _check_edit_permission(). Right now any agent can add notes/links/images even though update/delete are fail-closed, which breaks the permission model for the new MCP canvas_add_* handlers.
Proposed fix
) -> dict:
kind = element.get("kind")
if kind not in _VALID_KINDS:
raise ValueError(f"invalid kind: {kind}")
if author_kind == "agent" and kind not in _AGENT_ALLOWED_KINDS:
raise ValueError(f"agents may not emit kind={kind}")
if author_kind not in ("user", "agent"):
raise ValueError(f"invalid author_kind: {author_kind}")
+ await self._check_edit_permission(project_id, author_kind, author_id)
eid = element.get("id") or new_id("cve")
now = time.time()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tinyagentos/projects/canvas/store.py` around lines 75 - 89, add_element
currently enforces kind and author_kind checks but never calls
_check_edit_permission, so agents can still create canvas elements; update
add_element to call self._check_edit_permission(project_id=project_id,
author_kind=author_kind, author_id=author_id) (or the existing signature of
_check_edit_permission) before allowing creation, ensuring the permission check
runs for both author_kind "agent" and "user" and raising the same permission
error path when it fails; place the call after validating author_kind and kind
but before any DB insert/return so creation is blocked when edit permission is
denied.
Comment on lines
+23
to
+31
| async def _http_get(url: str) -> tuple[int, str]: | ||
| """Indirection seam for tests.""" | ||
| async with httpx.AsyncClient( | ||
| follow_redirects=True, | ||
| timeout=_TIMEOUT, | ||
| headers={"User-Agent": "taOS-canvas-unfurl/0.1"}, | ||
| ) as client: | ||
| r = await client.get(url) | ||
| return r.status_code, r.text |
There was a problem hiding this comment.
Add response size/content-type limits for unfurl fetches.
Line 23-31 eagerly materializes r.text for any response. Large or binary payloads can increase memory usage and request latency.
Suggested guardrail
async def _http_get(url: str) -> tuple[int, str]:
async with httpx.AsyncClient(
follow_redirects=True,
timeout=_TIMEOUT,
headers={"User-Agent": "taOS-canvas-unfurl/0.1"},
) as client:
- r = await client.get(url)
- return r.status_code, r.text
+ r = await client.get(url)
+ ctype = (r.headers.get("content-type") or "").lower()
+ if "text/html" not in ctype:
+ return r.status_code, ""
+ body = r.text
+ if len(body) > 1_000_000:
+ body = body[:1_000_000]
+ return r.status_code, body🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tinyagentos/projects/canvas/unfurl.py` around lines 23 - 31, The _http_get
function currently eagerly reads r.text, so update it to stream the response and
enforce content-type and size limits: open the request with
httpx.AsyncClient.stream (or client.get(..., stream=True)), inspect
response.headers for Content-Type and optional Content-Length, reject or return
an error if the type is disallowed or Content-Length exceeds a configured max,
then read at most MAX_BYTES from the stream (using r.aiter_bytes or
r.aiter_raw/read with a byte counter) and decode only those bytes to text (or
return a binary-safe error) so you never materialize arbitrarily large or binary
payloads; keep the function signature _http_get and return an appropriate status
code and truncated/typed body string when limits are hit.
Two correctness fixes flagged in PR review: - ProjectCanvasStore.update_element previously filtered the UPDATE by project_id but reloaded the row via get_element(element_id) without scoping, so a caller passing the wrong project_id could read another project's element (and the empty-patch fast path returned it). Add an optional project_id arg to get_element and use it in both reload spots. Regression test added. - unfurl._http_get now resolves the target hostname and rejects private, loopback, link-local, multicast, reserved, and unspecified addresses before any request is made. Redirects are disabled so a 30x to a private host can't bypass the resolution check. Response bodies are capped at 1 MB; the parser only needs <head> metadata.
Owner
Author
|
Triaged the review. Just pushed
On the agent-add permission flag default: that's by spec. The remaining frontend findings (fetch/SSE race, z_index loss on drag end, deleteElement silent failure logging, .tldr stacking on snapshot reload) are real but not correctness-blocking for the initial ship — tracking them as follow-ups. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tinyagentos/projects/canvas/store.py (1)
84-98:⚠️ Potential issue | 🔴 CriticalEnforce
can_edit_canvason agent creates.
add_element()validates kind and author_kind but never calls_check_edit_permission(). Agents can add elements withoutcan_edit_canvas = 1, while updates and deletes are permission-gated. This inconsistency allows unpermitted agents to create canvas elements via MCP tools (canvas_add_note, etc.).🐛 Proposed fix
if author_kind not in ("user", "agent"): raise ValueError(f"invalid author_kind: {author_kind}") + await self._check_edit_permission(project_id, author_kind, author_id) eid = element.get("id") or new_id("cve")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/store.py` around lines 84 - 98, The add_element function currently skips permission checks for agents; before allowing an agent to create an element, call the existing _check_edit_permission helper to enforce can_edit_canvas (e.g., from within add_element when author_kind == "agent"), supplying the project_id and author_id (and author_kind if _check_edit_permission requires it) and abort/raise if the check fails so agent creates are gated the same way as updates/deletes.
🧹 Nitpick comments (5)
tinyagentos/projects/canvas/unfurl.py (1)
55-63: Short-circuit body streaming for non-2xx statuses.Since non-2xx should fall back, you can return immediately after reading the status line and skip streaming up to 1 MB of irrelevant body bytes.
Proposed optimization
async with client.stream("GET", url) as r: + if r.status_code < 200 or r.status_code >= 300: + return r.status_code, "" chunks: list[bytes] = [] total = 0 async for chunk in r.aiter_bytes(): total += len(chunk) chunks.append(chunk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/unfurl.py` around lines 55 - 63, After opening the stream (async with client.stream("GET", url) as r) check the response status immediately (e.g., r.status_code or r.status) and if it is not a 2xx, short-circuit and return/handle the fallback without iterating r.aiter_bytes() so you don't read up to _MAX_BODY_BYTES for error responses; only proceed to accumulate chunks into the chunks list and build body = b"".join(chunks)[:_MAX_BODY_BYTES] when the status is 2xx.tinyagentos/projects/canvas/store.py (2)
45-51: Consider addingstrict=Truetozip()for defensive safety.While cursor description and row lengths should always match in SQLite results, using
strict=Trueguards against silent data truncation if assumptions ever break.♻️ Proposed fix
def _row_to_element(row, description) -> dict: keys = [d[0] for d in description] - e = dict(zip(keys, row)) + e = dict(zip(keys, row, strict=True)) for f in _CANVAS_JSON_FIELDS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/store.py` around lines 45 - 51, Update _row_to_element to use Python's strict zip to ensure the cursor description and row lengths match; specifically, when creating e = dict(zip(keys, row)) replace with a strict zip call (zip(keys, row, strict=True)) so mismatched lengths raise an error instead of silently truncating, and leave the subsequent JSON decoding loop over _CANVAS_JSON_FIELDS unchanged.
174-174: Split multiple statements on one line.Static analysis flags
E702: Multiple statements on one line (semicolon). Split for readability.♻️ Proposed fix
- sets.append("updated_at = ?"); params.append(time.time()) + sets.append("updated_at = ?") + params.append(time.time())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/store.py` at line 174, The line containing two statements joined by a semicolon should be split into separate statements for readability and to satisfy E702: locate the place where sets.append("updated_at = ?"); params.append(time.time()) is used (the update-building logic in the store module) and replace it with two distinct statements: one that calls sets.append("updated_at = ?") and a following line that calls params.append(time.time()), preserving order and functionality.tests/projects/test_canvas_store.py (2)
124-141: Prefix unused variable with underscore.Static analysis flags that
psis unpacked but never used. Use_or_psto indicate intentional discard.♻️ Proposed fix
`@pytest.mark.asyncio` async def test_agent_with_permission_can_update(store_with_member): - cs, ps = store_with_member + cs, _ps = store_with_member await cs._db.execute(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_canvas_store.py` around lines 124 - 141, The test test_agent_with_permission_can_update unpacks store_with_member into cs, ps but never uses ps; rename the unused variable to _ps or _ to silence the static analysis warning (e.g., change "cs, ps = store_with_member" to "cs, _ps = store_with_member" or "cs, _ = store_with_member") so the fixture is still consumed but the linter knows the variable is intentionally discarded.
52-59: Missing test: agentadd_elementpermission enforcement.Once
add_element()is fixed to call_check_edit_permission(), add a test verifying that agents withoutcan_edit_canvascannot create elements. This parallelstest_agent_without_permission_cannot_updateandtest_delete_requires_permission.🧪 Suggested test
`@pytest.mark.asyncio` async def test_agent_without_permission_cannot_add(store_with_member): from tinyagentos.projects.canvas.store import CanvasPermissionError cs, _ = store_with_member with pytest.raises(CanvasPermissionError): await cs.add_element( project_id="p1", element={"kind": "note", "x": 0, "y": 0, "w": 1, "h": 1, "payload": {"text": "a"}}, author_kind="agent", author_id="agent-1", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/projects/test_canvas_store.py` around lines 52 - 59, Add a new async test that mirrors the existing update/delete permission tests to ensure add_element enforces agent edit permissions: use the fixture store_with_member to get cs, import CanvasPermissionError, and call cs.add_element(...) with author_kind="agent" and author_id="agent-1" expecting a CanvasPermissionError; this verifies that add_element calls _check_edit_permission and blocks agents without can_edit_canvas (name the test e.g. test_agent_without_permission_cannot_add).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tinyagentos/projects/canvas/unfurl.py`:
- Around line 156-157: The current check in fetch_link_metadata only falls back
on status >= 400; change the logic so any non-2xx response triggers the fallback
(i.e., treat responses where status < 200 or status >= 300 as failures). Update
the condition around the call that currently does "if status >= 400: return
_fallback(url)" in fetch_link_metadata to call _fallback(url) for non-2xx codes
instead, ensuring 1xx and 3xx are also handled.
---
Duplicate comments:
In `@tinyagentos/projects/canvas/store.py`:
- Around line 84-98: The add_element function currently skips permission checks
for agents; before allowing an agent to create an element, call the existing
_check_edit_permission helper to enforce can_edit_canvas (e.g., from within
add_element when author_kind == "agent"), supplying the project_id and author_id
(and author_kind if _check_edit_permission requires it) and abort/raise if the
check fails so agent creates are gated the same way as updates/deletes.
---
Nitpick comments:
In `@tests/projects/test_canvas_store.py`:
- Around line 124-141: The test test_agent_with_permission_can_update unpacks
store_with_member into cs, ps but never uses ps; rename the unused variable to
_ps or _ to silence the static analysis warning (e.g., change "cs, ps =
store_with_member" to "cs, _ps = store_with_member" or "cs, _ =
store_with_member") so the fixture is still consumed but the linter knows the
variable is intentionally discarded.
- Around line 52-59: Add a new async test that mirrors the existing
update/delete permission tests to ensure add_element enforces agent edit
permissions: use the fixture store_with_member to get cs, import
CanvasPermissionError, and call cs.add_element(...) with author_kind="agent" and
author_id="agent-1" expecting a CanvasPermissionError; this verifies that
add_element calls _check_edit_permission and blocks agents without
can_edit_canvas (name the test e.g. test_agent_without_permission_cannot_add).
In `@tinyagentos/projects/canvas/store.py`:
- Around line 45-51: Update _row_to_element to use Python's strict zip to ensure
the cursor description and row lengths match; specifically, when creating e =
dict(zip(keys, row)) replace with a strict zip call (zip(keys, row,
strict=True)) so mismatched lengths raise an error instead of silently
truncating, and leave the subsequent JSON decoding loop over _CANVAS_JSON_FIELDS
unchanged.
- Line 174: The line containing two statements joined by a semicolon should be
split into separate statements for readability and to satisfy E702: locate the
place where sets.append("updated_at = ?"); params.append(time.time()) is used
(the update-building logic in the store module) and replace it with two distinct
statements: one that calls sets.append("updated_at = ?") and a following line
that calls params.append(time.time()), preserving order and functionality.
In `@tinyagentos/projects/canvas/unfurl.py`:
- Around line 55-63: After opening the stream (async with client.stream("GET",
url) as r) check the response status immediately (e.g., r.status_code or
r.status) and if it is not a 2xx, short-circuit and return/handle the fallback
without iterating r.aiter_bytes() so you don't read up to _MAX_BODY_BYTES for
error responses; only proceed to accumulate chunks into the chunks list and
build body = b"".join(chunks)[:_MAX_BODY_BYTES] when the status is 2xx.
🪄 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 Plus
Run ID: c69987e0-bf0b-4961-bb8c-ca8dd35f5788
📒 Files selected for processing (3)
tests/projects/test_canvas_store.pytinyagentos/projects/canvas/store.pytinyagentos/projects/canvas/unfurl.py
Module contract says "on any failure (timeout, non-2xx, parse error) returns a fallback dict". Previous check only triggered on >=400, so 1xx and 3xx slipped through to the parser. Now that follow_redirects is disabled, 3xx replies are no longer followed and would otherwise be parsed as if they were a real page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/projects/canvas/unfurl.py (1)
101-104: Avoid silent parser failure in metadata extraction.
Line 103-104swallows all parse exceptions withpass, which hides malformed-input patterns and makes production debugging harder. Prefer a debug-level log (still returning fallback-safe metadata behavior).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/projects/canvas/unfurl.py` around lines 101 - 104, The try/except around p.feed(html) currently swallows all exceptions; replace the bare pass with a debug-level log that includes the exception information (e.g., logger.debug("HTML parse failed in p.feed(html)", exc_info=True) or use logging.getLogger(__name__) if no logger exists) and keep the existing fallback-safe metadata return path so behavior is unchanged for callers; update the block surrounding p.feed(html) in unfurl.py accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tinyagentos/projects/canvas/unfurl.py`:
- Around line 101-104: The try/except around p.feed(html) currently swallows all
exceptions; replace the bare pass with a debug-level log that includes the
exception information (e.g., logger.debug("HTML parse failed in p.feed(html)",
exc_info=True) or use logging.getLogger(__name__) if no logger exists) and keep
the existing fallback-safe metadata return path so behavior is unchanged for
callers; update the block surrounding p.feed(html) in unfurl.py accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff354729-ef83-4b3a-ab43-2d48665ebee3
📒 Files selected for processing (1)
tinyagentos/projects/canvas/unfurl.py
Comment on lines
+27
to
+31
| async def _check_ssrf_safe(host: str) -> None: | ||
| """Resolve host and reject private / loopback / link-local addresses.""" | ||
| loop = asyncio.get_running_loop() | ||
| infos = await loop.getaddrinfo(host, None) | ||
| for _family, _type, _proto, _canon, sockaddr in infos: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tinyagentos/projects/canvas/unfurl.py | head -80Repository: jaylfc/tinyagentos
Length of output: 3364
🏁 Script executed:
cat -n tinyagentos/projects/canvas/unfurl.py | sed -n '27,80p'Repository: jaylfc/tinyagentos
Length of output: 2556
🏁 Script executed:
rg -n "httpx\.|AsyncClient|socket|bind|getaddrinfo" tinyagentos/projects/canvas/unfurl.pyRepository: jaylfc/tinyagentos
Length of output: 149
🌐 Web query:
httpx AsyncClient DNS resolution behavior IP pinning
💡 Result:
httpx.AsyncClient performs DNS resolution using Python's standard socket.getaddrinfo on each connection attempt, as it relies on httpcore's AsyncConnectionPool. There is no built-in DNS caching (confirmed in official GitHub discussion as of 2022, no evidence of addition by 2026). This can lead to issues like DNS lookup storms under high concurrency to the same host, causing temporary resolution failures. For IP pinning (forcing connection to a specific IP while using a hostname for Host header and SNI): - Use the IP address directly in the URL, set Host header to the hostname, and for HTTPS, use the "sni_hostname" extension to specify the hostname for SSL verification. - Example (works for sync Client, analogous for AsyncClient): headers = {"Host": "www.encode.io"} extensions = {"sni_hostname": "www.encode.io"} response = client.get("https://185.199.108.153/path", headers=headers, extensions=extensions) - Without the extension, HTTPS fails with certificate mismatch since SSL uses the IP for hostname verification. - No built-in support for custom DNS resolvers or automatic IP pinning from hostname. Custom transports via mounts can be implemented for advanced control, but standard use relies on system DNS. - To get the resolved IP post-request: Access response.extensions["network_stream"].get_extra_info("server_addr") (async stream context) or private stream attributes (not recommended, unstable).
Citations:
- 1: DNS caching? encode/httpx#2211
- 2: https://www.python-httpx.org/advanced/extensions/
- 3: https://stackoverflow.com/questions/74069713/why-does-asyncio-create-task-and-asyncio-ensure-future-behave-differently-when-c
- 4: https://www.python-httpx.org/advanced/transports/
- 5: https://www.python-httpx.org/advanced/proxies/
- 6: https://stackoverflow.com/questions/73934230/get-server-ip-from-response-of-httpx-get
- 7: Is custom name resolution implemented? encode/httpx#2592
DNS-rebinding attack: resolved IP not pinned to connection.
_check_ssrf_safe validates the hostname's DNS answers (lines 27–31) and rejects private addresses, but _http_get then passes the original hostname string to httpx.AsyncClient.stream() (line 55). Since httpx re-resolves the hostname at connection time with no IP caching, an attacker can change DNS between the validation check (line 47) and the actual connection (line 55) to point the hostname to a private IP, bypassing the SSRF guard.
Harden by resolving the hostname in _check_ssrf_safe, returning the validated IP(s), and using the IP address directly in the URL while preserving the hostname via Host header and SNI for HTTPS.
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
Pass B item #2 from the Projects roadmap: per-project tldraw infinite canvas as a persistent ideation surface. Users own elements; agents drop notes/links/images. Live-collaborative via SSE within ~200 ms. Per-agent
can_edit_canvasflag (default OFF, fail-closed).What's in this PR
Backend (Python) — new
tinyagentos/projects/canvas/module:store.py— async aiosqlite store (mirrorsProjectTaskStore) with element CRUD, soft delete, kind validation, permission enforcementsnapshotter.py— debounced.tldrwriter (mirrorsBeadsBridge), per-project asyncio.Lock, atomicos.replaceunfurl.py— HTML/OG-tag scraping for link previews; never raisesrender.py— Pillow PNG renderer for vision agents (low-fidelity)mcp_tools.py— 7 in-process handler functions (canvas_list_elements,canvas_add_note,canvas_add_link,canvas_add_image,canvas_update_element,canvas_delete_element,canvas_get_snapshot_png)Routes (FastAPI) — new
tinyagentos/routes/project_canvas.py:GET/POST /api/projects/{id}/canvas/elementsPATCH/DELETE /api/projects/{id}/canvas/elements/{element_id}GET /api/projects/{id}/canvas/snapshot.png|.tldrPATCH /api/projects/{id}/canvas/permissions/{agent_id}GET /api/projects/{id}/canvas/stream(SSE)Schema — adds
project_canvas_elementstable +project_members.can_edit_canvascolumn. Idempotent ALTER for existing DBs.Frontend (React + tldraw v4) — new
desktop/src/apps/ProjectsApp/canvas/:canvas-api.ts— REST wrappercanvas-store.ts— zustand element cachecanvas-sse.ts— EventSource subscriber dispatching to storeshapes/{Note,Link,Image}Shape.tsx— three custom tldraw shape utilitiesCanvasBoard.tsx—<Tldraw>mount with REST hydration + SSE live updates + push-add/update/delete on user editsCanvasView.tsx+ProjectWorkspace.tsxtab integrationProjectMembers.tsx— per-agentCan edit canvastoggleTest plan
tests/projects/test_canvas_store.py— 11 tests (CRUD, permission, broker publish)tests/projects/test_canvas_unfurl.py— 4 teststests/projects/test_canvas_snapshotter.py— 2 teststests/projects/test_canvas_render.py— 2 teststests/projects/test_routes_canvas.py— 7 tests (raw ASGI for SSE)tests/projects/test_canvas_mcp_tools.py— 4 teststests/projects/test_canvas_integration.py— 2 tests (REST → SSE → snapshot → permission matrix)desktop/src/apps/ProjectsApp/__tests__/canvas-api.test.ts— 3 testsdesktop/src/apps/ProjectsApp/__tests__/canvas-store.test.ts— 3 testsnpm run buildcleanNotes for reviewers
app.state.project_canvas_store(notcanvas_store— that's owned by chat-canvas). All wiring matches.canvas_streamandcanvas_set_permissionuseapp.state.project_event_broker(notproject_broker); fixed mid-review when a regression was caught.deleted_atcolumn;list_elementsfilters them out.can_edit_canvas = 1row inproject_members. Fails closed.desktop/tests/canvas-e2e.spec.tsis documented but not runnable — no Playwright scaffolding in repo yet. Header comment lists the steps to wire it up.ShapeUtil<any>,Rectangle2dfor geometry,editor.run()(wasbatch()),shapeUtilsas<Tldraw>prop (notcreateTLStore). All small, expected.Summary by CodeRabbit
New Features
Chores
Tests