Repository navigation
feat(canvas): make Live Canvas live — publish cell changes on a channel (#17020) - #17697
Conversation
…el (#17020) "Live Canvas — agent-driven visual canvas workspace" (#7425) loaded once and received nothing. Measured by call site, not by name: useCanvasWebSocket.ts:15 opened /api/canvas/{id}/ws -- api/canvas.py has five REST routes and no websocket get_canvas_cell_event :325 ZERO callers (one occurrence: its own def) register_canvas_streaming_task ZERO callers unregister_... ZERO callers CanvasView.vue :83 onMounted(loadCanvas), no polling A REGISTRATION IS NOT A CALL. `_format_canvas_cell` sits in MESSAGE_TYPE_FORMATTERS under the key "canvas_cell", so every reader who checked for the name found it, in a dispatch map, and stopped. That is why this change is verified by call sites and why the guard asserts them. CHANNELS, NOT ROUTES. The issue's criterion said to wire the client to "the endpoint that actually carries canvas events"; there was none, and the alternative -- remove it as superseded by the shared bus -- was also false, since the bus had no canvas_cell publisher either. Serving the bespoke socket would have made a third parallel delivery system, which EVENT_STATE_DOCTRINE principle 1 names as a state this codebase has already been in, and principle 5 forbids directly: new event types extend the channel grammar, they do not add an endpoint. The doctrine's anti-goals do give binary and high-volume streams their own sockets -- VNC, audio, PTY, log tails -- and a JSON cell delta is not in that carve-out. So: `canvas` joins _VALID_PREFIXES, `_authorize_canvas_channel` gates it on the same owner check every REST route on the resource applies, and the three routes that write a cell publish canvas_cell after committing. Publishing before the commit would announce a write a rollback discards; publishing inside the persistence step would make delivery its side effect (principle 4). PersistStrategy.NONE deliberately -- the durable fact is the cell row, and a client that was away re-reads it with GET (principle 2). Agent DELTA streaming still has no producer. Nothing writes cell content except those three REST routes, so `seq`/`delta` carry a version and the full content rather than an incremental chunk. What this delivers is every real cell change reaching every subscriber, which is what "does a second connected client see the same thing as the first" asks. An agent streaming path publishes to the same channel when it exists, with no new transport. api/canvas.py was grandfathered at 637 lines in a shrink-only ratchet and the call sites pushed it over, so rich-payload validation is extracted to canvas/rich_payload.py -- split rather than raise, per the rule. The file is now 584 lines, under MAX_LINES, so its KNOWN_LARGE and RATCHET_BASELINE entries are DELETED: the tool's own message says an entry naming a compliant file exempts nothing while looking authoritative. The consumer's message handling is unchanged, conflict branch included. It was written for exactly this event and had simply never been reached. Guard mutation-tested, six ways: removing a publish, publishing before the commit, serving a @router.websocket on the canvas router, dropping the canvas prefix, making the authorizer fail open, and unhooking it from the dispatcher are each CAUGHT. Closes #17020 Refs #7425 Refs #17009
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe backend publishes committed canvas-cell changes through the shared event bus. Canvas channels use owner-based authorization, and the frontend subscribes to those channels instead of opening a dedicated WebSocket. Canvas cells and dashboard widgets now use UUID-based identifiers. ChangesCanvas live updates
Unique frontend identifiers
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Other subscribed canvas views can show stale cell order or rendering after an autosave until they reload. Autosave events also repeat sequence values, though the current frontend does not use them to order updates. These are bounded live-sync issues with a reload workaround. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Authenticated users receive an ownership check before joining a canvas channel. When WebSocket authentication is disabled, however, the shared authorization path can admit a subscriber without applying that check. The exposure depends on deployment configuration and knowledge of a canvas identifier. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue Full details: Out of Scope Changes checkExplanation The canvas event-bus, authorisation, publication, frontend subscription, and related tests support issue
✨ Finishing Touches 💡 1📝 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: eeb52a4b-6186-44a2-b830-42f33669ea4d
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (9)
autobot-backend/api/canvas.pyautobot-backend/api/canvas_channel_authz_17020_test.pyautobot-backend/api/live_events.pyautobot-backend/canvas/events.pyautobot-backend/canvas/rich_payload.pyautobot-backend/live_event_manager.pyautobot-frontend/src/composables/__tests__/useCanvasWebSocket.test.tsautobot-frontend/src/composables/useCanvasWebSocket.tsrepo_tests/canvas_live_channel_17020_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| touched.append(cell) | ||
|
|
||
| await session.commit() | ||
| _log_metric("canvas.autosave.success", canvas_id=str(canvas_id)) | ||
|
|
||
| for cell in touched: | ||
| await _publish_cell(canvas_id, cell) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'get_canvas_cell_event|canvas_cell|\\.seq\\b|seq:.*version' autobot-frontend/src autobot-backend/canvas autobot-backend/api/websockets.py | head -135
sed -n '39,65p' autobot-frontend/src/stores/useCanvasStore.ts
sed -n '85,103p' autobot-backend/canvas/events.pyRepository: mrveiss/AutoBot-AI
Length of output: 4299
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,90p' autobot-frontend/src/composables/useCanvasWebSocket.ts
sed -n '1,90p' autobot-frontend/src/types/canvas.ts
rg -n -C 3 '\bseq\b|upsertStreamCell|CanvasWsMessage' autobot-frontend/src --glob '!**/node_modules/**'
sed -n '315,350p' autobot-backend/api/websockets.py
sed -n '80,106p' autobot-backend/canvas/events.py
sed -n '175,216p' autobot-backend/api/canvas.pyRepository: mrveiss/AutoBot-AI
Length of output: 31528
🏁 Script executed:
#!/bin/bash
set -e
sed -n '35,145p' autobot-frontend/src/stores/useCanvasStore.ts
sed -n '95,145p' autobot-frontend/src/components/canvas/CanvasPanel.vue
rg -n -C 4 'pausedAgentSeq|triggerConflict|resolveConflict|conflict' autobot-frontend/src/stores autobot-frontend/src/components/canvas autobot-frontend/src/composables --glob '!**/node_modules/**'Repository: mrveiss/AutoBot-AI
Length of output: 27299
Increment cell.version before publishing autosave events.
cell_payload maps cell.version to seq and documents it as the sequence for client ordering. Autosave changes the cell but leaves cell.version unchanged, so repeated updates use the same seq.
The current frontend stores seq and passes it to conflict state, but it does not compare or use it to reject events. This is therefore not a major stale-update failure in the current consumer. It still violates the ordering contract and makes sequence-based consumers unreliable.
🐛 Suggested fix
cell.position = item.position
cell.content = item.content
cell.type = item.type
+ cell.version = cell.version + 1
cell.updated_at = now| async with get_async_session_factory()() as session: | ||
| canvas = (await session.execute(select(Canvas).where(Canvas.id == ident))).scalar_one_or_none() | ||
| if canvas is None: | ||
| return False | ||
| return str(canvas.user_id) == user_id |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '232,305p' autobot-backend/api/live_events.py
rg -n 'class Canvas\\b|id =.*UUID|id:.*UUID|_authorize_channel\\(' autobot-backend | head -65Repository: mrveiss/AutoBot-AI
Length of output: 3841
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- Canvas model declarations ---'
rg -n -F 'class Canvas' .
rg -n 'Canvas\.id|id[[:space:]]*=[[:space:]]*.*UUID|UUID\(' autobot-backend --glob '*.py' | head -120
printf '%s\n' '--- authorization callers and subscription dispatch ---'
rg -n '_authorize_channel|subscribe|canvas:' autobot-backend/api/live_events.py
printf '%s\n' '--- database/backend configuration ---'
rg -n 'asyncpg|postgres|postgresql|DATABASE_URL|create_async_engine|dialect' autobot-backend --glob '*.py' --glob '*.env*' --glob '*.toml' --glob '*.yaml' --glob '*.yml' | head -160Repository: mrveiss/AutoBot-AI
Length of output: 36803
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- Canvas model ---'
sed -n '1,75p' autobot-backend/canvas/models.py
printf '%s\n' '--- subscription dispatch ---'
sed -n '330,382p' autobot-backend/api/live_events.py
printf '%s\n' '--- async database URL selection ---'
sed -n '1,75p' autobot-backend/migrations/db_url.py
printf '%s\n' '--- dependency declarations ---'
rg -n 'asyncpg|SQLAlchemy|sqlalchemy' pyproject.toml autobot-backend/pyproject.toml requirements*.txt autobot-backend/requirements*.txt 2>/dev/null | head -80
printf '%s\n' '--- installed library contract sources, if available ---'
python3 - <<'PY'
import importlib.util
for name in ("asyncpg", "sqlalchemy"):
spec = importlib.util.find_spec(name)
print(name, spec.origin if spec else "not installed")
PYRepository: mrveiss/AutoBot-AI
Length of output: 7186
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- runtime session factory ---'
rg -n -C 5 'def get_async_session_factory|get_async_session_factory|create_async_engine|async_url|asyncpg' autobot-backend/user_management autobot-backend --glob '*.py' | head -180
printf '%s\n' '--- exact authorization lines ---'
nl -ba autobot-backend/api/live_events.py | sed -n '238,301p;360,375p'Repository: mrveiss/AutoBot-AI
Length of output: 20182
🌐 Web query:
asyncpg PostgreSQL UUID query argument string expected uuid official documentation
💡 Result:
`asyncpg` expects PostgreSQL `uuid` parameters as Python `uuid.UUID` objects—not plain strings—according to its type-mapping documentation. ([github.com](https://github.com/MagicStack/asyncpg/blob/master/docs/usage.rst?utm_source=openai))
Convert the string before passing it:
```python
from uuid import UUID
await conn.fetchrow(
"SELECT * FROM users WHERE id = $1",
UUID(user_id),
)
```
If you need to pass a string, cast the parameter in SQL so PostgreSQL treats it as a UUID:
```python
await conn.fetchrow(
"SELECT * FROM users WHERE id = $1::uuid",
user_id,
)
```
PostgreSQL accepts UUIDs in standard hyphenated form and several alternatives, but invalid UUID text will still fail to parse. ([postgresql.org](https://www.postgresql.org/docs/current/datatype-uuid.html?utm_source=openai))
Citations:
- 1: https://github.com/MagicStack/asyncpg/blob/master/docs/usage.rst?utm_source=openai
- 2: https://www.postgresql.org/docs/current/datatype-uuid.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
set -u
nl -ba autobot-backend/api/live_events.py | sed -n '1,32p'Repository: mrveiss/AutoBot-AI
Length of output: 2039
🏁 Script executed:
#!/bin/bash
set -u
nl -ba autobot-backend/api/live_events.py | sed -n '33,58p'Repository: mrveiss/AutoBot-AI
Length of output: 1229
Validate the canvas ID before the lookup.
A non-admin authenticated client can submit a non-UUID canvas: channel. Canvas.id is a PostgreSQL UUID column, and asyncpg expects a Python uuid.UUID value. The lookup can therefore raise, and the broad handler writes a full traceback for each request before denying access.
Suggested fix
import asyncio
import json
+import uuid
@@
if not ident:
return False
+ try:
+ canvas_id = uuid.UUID(ident)
+ except ValueError:
+ return False
try:
@@
- canvas = (await session.execute(select(Canvas).where(Canvas.id == ident))).scalar_one_or_none()
+ canvas = (await session.execute(select(Canvas).where(Canvas.id == canvas_id))).scalar_one_or_none()| except Exception: | ||
| logger.warning( | ||
| "canvas_cell publish failed for canvas %s cell %s", | ||
| canvas_id, | ||
| getattr(cell, "id", "?"), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '48,105p' autobot-backend/canvas/events.py
rg -n 'publish failed|exc_info=True|logger.exception' autobot-backend/canvas autobot-backend/api/live_events.py | head -70Repository: mrveiss/AutoBot-AI
Length of output: 3006
Log exception details when publishing fails.
The except block suppresses the publish exception, but logger.warning records only the canvas and cell identifiers. Add exc_info=True so repeated bus failures include their cause and traceback.
Suggested fix
logger.warning(
"canvas_cell publish failed for canvas %s cell %s",
canvas_id,
getattr(cell, "id", "?"),
+ exc_info=True,
)📝 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.
| except Exception: | |
| logger.warning( | |
| "canvas_cell publish failed for canvas %s cell %s", | |
| canvas_id, | |
| getattr(cell, "id", "?"), | |
| ) | |
| except Exception: | |
| logger.warning( | |
| "canvas_cell publish failed for canvas %s cell %s", | |
| canvas_id, | |
| getattr(cell, "id", "?"), | |
| exc_info=True, | |
| ) |
🧰 Tools
🪛 Ruff (0.16.6)
[warning] 77-77: Do not catch blind exception: Exception
(BLE001)
| return get_canvas_cell_event( | ||
| cell_id=str(cell.id), | ||
| seq=getattr(cell, "version", 0), | ||
| delta=getattr(cell, "content", None) or "", | ||
| state=getattr(cell, "state", ""), | ||
| canvas_id=str(canvas_id), | ||
| )["payload"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '35,95p' autobot-frontend/src/stores/useCanvasStore.ts
sed -n '175,217p;298,334p' autobot-backend/api/canvas.py
sed -n '85,105p' autobot-backend/canvas/events.pyRepository: mrveiss/AutoBot-AI
Length of output: 6047
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant diff ---'
git diff --unified=40 290fae38c060e835fd3d8bd6a6bdfdc4a8381272 d7bff5075d724e11e021238dc26d6f7b04c9ff15 -- autobot-backend/canvas/events.py autobot-backend/api/canvas.py autobot-backend/api/websockets.py autobot-frontend/src/composables/useCanvasWebSocket.ts autobot-frontend/src/stores/useCanvasStore.ts autobot-frontend/src/types/canvas.ts
printf '%s\n' '--- event handler and store ---'
sed -n '1,110p' autobot-frontend/src/composables/useCanvasWebSocket.ts
sed -n '1,90p' autobot-frontend/src/stores/useCanvasStore.ts
sed -n '1,90p' autobot-frontend/src/types/canvas.ts
printf '%s\n' '--- canvas event implementations and uses ---'
sed -n '1,180p' autobot-backend/canvas/events.py
rg -n -C 5 'canvas_cell|get_canvas_cell_event|upsertStreamCell|publish_cell|_publish_cell' autobot-backend autobot-frontend --glob '!**/node_modules/**'Repository: mrveiss/AutoBot-AI
Length of output: 42328
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- publisher ---'
sed -n '55,110p' autobot-backend/canvas/events.py
printf '%s\n' '--- live handler ---'
sed -n '1,85p' autobot-frontend/src/composables/useCanvasWebSocket.ts
printf '%s\n' '--- event contract ---'
sed -n '315,355p' autobot-backend/api/websockets.py
printf '%s\n' '--- store and types ---'
sed -n '1,75p' autobot-frontend/src/stores/useCanvasStore.ts
sed -n '35,70p' autobot-frontend/src/types/canvas.ts
printf '%s\n' '--- producer bindings ---'
rg -n -C 8 'get_canvas_cell_event|publish_event\\(|upsertStreamCell|canvas_cell' autobot-backend autobot-frontend --glob '!**/node_modules/**' --glob '!**/*.map'
printf '%s\n' '--- focused tests ---'
rg -n -C 12 'upsertStreamCell|delta|seq|content' autobot-frontend/src/stores autobot-frontend/src/composables/__tests__ autobot-backend/tests --glob '*canvas*' --glob '*websocket*' --glob '*.test.*' --glob '*.py'Repository: mrveiss/AutoBot-AI
Length of output: 10017
Preserve snapshot updates separately from streaming deltas.
Autosave and transition edits publish the complete persisted cell.content as delta. The frontend appends delta to an existing cell. For example, persisted content A can become AA after the live event. seq does not prevent this because the store assigns it without comparing or using it for replacement.
Mark these events as snapshots and replace existing content for snapshot events. Keep append behaviour for the existing streaming-delta contract.
Suggested fix
diff --git a/autobot-backend/canvas/events.py b/autobot-backend/canvas/events.py
@@
- return get_canvas_cell_event(
+ payload = get_canvas_cell_event(
cell_id=str(cell.id),
seq=getattr(cell, "version", 0),
delta=getattr(cell, "content", None) or "",
state=getattr(cell, "state", ""),
canvas_id=str(canvas_id),
)["payload"]
+ payload["updateMode"] = "snapshot"
+ return payload
diff --git a/autobot-frontend/src/types/canvas.ts b/autobot-frontend/src/types/canvas.ts
@@
delta: string
state: CellStreamState
+ updateMode?: 'delta' | 'snapshot'
}
diff --git a/autobot-frontend/src/stores/useCanvasStore.ts b/autobot-frontend/src/stores/useCanvasStore.ts
@@
- function upsertStreamCell(msg: Pick<CanvasWsMessage, 'cellId' | 'seq' | 'delta' | 'state'>) {
+ function upsertStreamCell(msg: Pick<CanvasWsMessage, 'cellId' | 'seq' | 'delta' | 'state' | 'updateMode'>) {
const existing = cells.value.find(c => c.id === msg.cellId)
if (existing) {
- existing.content += msg.delta
+ existing.content = msg.updateMode === 'snapshot'
+ ? msg.delta
+ : existing.content + msg.delta📝 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.
| return get_canvas_cell_event( | |
| cell_id=str(cell.id), | |
| seq=getattr(cell, "version", 0), | |
| delta=getattr(cell, "content", None) or "", | |
| state=getattr(cell, "state", ""), | |
| canvas_id=str(canvas_id), | |
| )["payload"] | |
| payload = get_canvas_cell_event( | |
| cell_id=str(cell.id), | |
| seq=getattr(cell, "version", 0), | |
| delta=getattr(cell, "content", None) or "", | |
| state=getattr(cell, "state", ""), | |
| canvas_id=str(canvas_id), | |
| )["payload"] | |
| payload["updateMode"] = "snapshot" | |
| return payload |
| def test_the_mutating_routes_are_the_ones_this_file_names() -> None: | ||
| """Discovery must agree with the list, so a new writing route fails here.""" | ||
| handlers = route_handlers(_module(_CANVAS_API)) | ||
| writing = {name for name, method in handlers.items() if method in {"put", "post", "patch"}} | ||
| # `export_canvas` is a POST that writes nothing. | ||
| writing.discard("export_canvas") | ||
| assert writing == _MUTATING_ROUTES, ( | ||
| f"the set of writing canvas routes changed: {sorted(writing)} against " | ||
| f"{sorted(_MUTATING_ROUTES)}. A new one must publish too -- add it here " | ||
| f"and give it a publish, rather than widening this set alone" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '31,90p;130,198p' repo_tests/canvas_live_channel_17020_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 5592
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test file outline and relevant references ---'
rg -n "route_handlers|websocket|contrast|synthetic|fixture|REACH|mutating|_MUTATING_ROUTES" repo_tests/canvas_live_channel_17020_test.py repo_tests || true
printf '%s\n' '--- canvas route declarations ---'
rg -n -C 2 "@router|APIRouter|websocket" autobot-backend/api/canvas.py
printf '%s\n' '--- changed-file diff against supplied merge base ---'
git diff --unified=40 290fae38c060e835fd3d8bd6a6bdfdc4a8381272 d7bff5075d724e11e021238dc26d6f7b04c9ff15 -- repo_tests/canvas_live_channel_17020_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 41770
🏁 Script executed:
set -e
rg -n -C 2 "@router|APIRouter|websocket" autobot-backend/api/canvas.py
rg -n -C 2 "route_handlers|websocket|contrast|synthetic|fixture|REACH|_MUTATING_ROUTES" repo_tests/canvas_live_channel_17020_test.py
git diff --unified=20 290fae38c060e835fd3d8bd6a6bdfdc4a8381272 d7bff5075d724e11e021238dc26d6f7b04c9ff15 -- repo_tests/canvas_live_channel_17020_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 13998
Add a reach floor to the WebSocket-route guard.
route_handlers can return an empty mapping when the router name or decorator shape changes. The WebSocket guard then passes because it finds no "websocket" method. Add the five-route reach floor to this guard only. The mutating-route equality guard already rejects empty discovery because _MUTATING_ROUTES contains three handlers.
Suggested fix
def test_the_canvas_router_grows_no_websocket_route() -> None:
"""Principle 5: channels, not routes. The unserved socket must not be served."""
- methods = set(route_handlers(_module(_CANVAS_API)).values())
+ handlers = route_handlers(_module(_CANVAS_API))
+ assert len(handlers) >= 5, (
+ f"only {len(handlers)} canvas routes were discovered; expected at least 5"
+ )
+ methods = set(handlers.values())
assert "websocket" not in methods, (…branch overwrote the cell it protects (#17020) The conflict test added in this PR failed in CI and passed locally. The test was right; `addCell` was wrong. id: `cell-${Date.now()}` Millisecond resolution, and every operation in this store addresses a cell BY id -- `deleteCell`, `moveCell`, `upsertStreamCell`, and the template's `:key`. Two cells created in the same tick shared one. That is what broke the conflict branch specifically. It adds an agent cell and then addresses "the last one": store.addCell('agent') const newId = store.cells[store.cells.length - 1].id store.upsertStreamCell({ ...msg, cellId: newId }) With a duplicate id, `newId` equals the conflicted cell's id, so the delta landed straight back on the cell the user was resolving -- the one thing that branch exists to prevent. WHY IT WAS NEVER CAUGHT BEFORE. That branch had never executed. Making it run for the first time is what #17020 is for, and a latent collision in id generation is precisely the kind of defect only a first execution can reveal. Better here than after merge. Reproduced deterministically with a frozen clock rather than by a fast loop -- a loop only collides when the machine is quick enough, which is why it passed on mine and failed in CI. The three new store tests use `vi.useFakeTimers()`, and one asserts the CONSEQUENCE (`deleteCell` removes the cell asked for, not whichever `find` reaches first) so it stays meaningful if the id scheme changes again. `crypto.randomUUID()` matches the five existing call sites, including the same shape at `useAppStore.ts:195`. FIXED AS A CLASS, NOT AN INSTANCE. `views/CustomDashboard.vue:516` had the identical defect -- `id: \`widget-${Date.now()}\`` -- and dashboards address a widget by id for layout, removal and `:key`. Both are fixed here rather than one fixed and one left for whichever PR next trips over it. A sweep for `id: \`...${Date.now()}\`` across the frontend now returns nothing. Mutation-tested: reverting either id to `Date.now()` is CAUGHT -- by all three store tests and by the CI conflict test.
|
Follow-up commit The defectid: `cell-${Date.now()}` // useCanvasStore.addCellMillisecond resolution, and every operation in that store addresses a cell by id — That is what broke the conflict branch specifically. It adds an agent cell and then addresses "the last one": store.addCell('agent')
const newId = store.cells[store.cells.length - 1].id
store.upsertStreamCell({ ...msg, cellId: newId })With a duplicate id, Why nothing caught it beforeThat branch had never executed. It was written for an event delivered over a socket the backend never served, so it had never been wrong — only unreached. Making it run for the first time is what this PR is for, and a latent collision in id generation is exactly the class of defect that only a first execution can reveal. That is a better argument for this PR existing than the feature is. Reproduced deterministically, not by luckIt passed on my machine and failed in CI, which is the signature of a timing-dependent test. A fast loop only collides when the machine is quick enough, so the three new store tests freeze the clock with
Fixed as a class, not an instance
VerificationMutation-tested. Reverting either id back to Both caught, and the conflict test now reproduces the CI failure locally too — the two One process noteThis went onto this branch rather than a new one. The failing test is in this PR and this PR is what it blocks, so a separate branch would have left #17697 red until a second PR merged — and the standing rule is to fix it in the same PR rather than open a second full CI suite. Flagging it because I was pointed at a separate branch and deliberately did not take that route. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Include autosave position and cell type in canvas_cell updates. · events.py:85-102
autobot-backend/canvas/events.py:85-102
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude autosave position and cell type in
canvas_cellupdates.When
put_canvaschanges a cell,_publish_cellemits onlycellId,seq,delta, andstate.useCanvasWebSocketforwards that payload toupsertStreamCell, which does not update the cell type or its position in the subscribed store. A second canvas view can therefore keep stale order and rendering until it reloads.Extend the shared payload and apply the fields in the store. Use
cellType, becausetypeis reserved for the event envelope.Suggested fix
diff --git a/autobot-backend/api/websockets.py b/autobot-backend/api/websockets.py @@ -def get_canvas_cell_event(cell_id: str, seq: int, delta: str, state: str, canvas_id: Optional[str] = None) -> dict: +def get_canvas_cell_event( + cell_id: str, + seq: int, + delta: str, + state: str, + canvas_id: Optional[str] = None, + position: Optional[int] = None, + cell_type: Optional[str] = None, +) -> dict: @@ "seq": seq, "delta": delta, "state": state, + "position": position, + "cellType": cell_type, }, } diff --git a/autobot-backend/canvas/events.py b/autobot-backend/canvas/events.py @@ delta=getattr(cell, "content", None) or "", state=getattr(cell, "state", ""), canvas_id=str(canvas_id), + position=getattr(cell, "position", 0), + cell_type=getattr(cell, "type", ""), )["payload"] diff --git a/autobot-frontend/src/types/canvas.ts b/autobot-frontend/src/types/canvas.ts @@ export interface CanvasWsMessage { type: 'canvas_cell' cellId: string seq: number delta: string state: CellStreamState + position: number + cellType: string } diff --git a/autobot-frontend/src/stores/useCanvasStore.ts b/autobot-frontend/src/stores/useCanvasStore.ts @@ - function upsertStreamCell(msg: Pick<CanvasWsMessage, 'cellId' | 'seq' | 'delta' | 'state'>) { - const existing = cells.value.find(c => c.id === msg.cellId) + function upsertStreamCell(msg: Pick<CanvasWsMessage, 'cellId' | 'seq' | 'delta' | 'state' | 'position' | 'cellType'>) { + const existingIndex = cells.value.findIndex(c => c.id === msg.cellId) + const existing = existingIndex >= 0 ? cells.value[existingIndex] : undefined if (existing) { existing.content += msg.delta existing.streamState = msg.state existing.seq = msg.seq + existing.contentType = (msg.cellType === 'text' ? 'markdown' : msg.cellType) as CanvasCell['contentType'] + const [cell] = cells.value.splice(existingIndex, 1) + cells.value.splice(Math.max(0, Math.min(msg.position, cells.value.length)), 0, cell) } else { - cells.value.push({ + const cell = { id: msg.cellId, canvasId: canvasId.value ?? '', owner: 'agent', - contentType: 'markdown', + contentType: (msg.cellType === 'text' ? 'markdown' : msg.cellType) as CanvasCell['contentType'], content: msg.delta, streamState: msg.state, seq: msg.seq, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), - }) + } + cells.value.splice(Math.max(0, Math.min(msg.position, cells.value.length)), 0, cell) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 904dd9ec-50a8-49f2-9b5a-f23ee9eab38e
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (2)
autobot-backend/api/canvas.pyautobot-backend/live_event_manager.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
1 participant
Thinking Path
"Live Canvas — agent-driven visual canvas workspace" (#7425) loaded once and received nothing. Measured by call site rather than by name:
A registration is not a call.
_format_canvas_cellsits inMESSAGE_TYPE_FORMATTERSunder the key"canvas_cell", so every reader who searched for the name found it — in a dispatch map — and stopped. That is why this change is verified by call sites, and why the guard asserts call sites rather than presence.Both of the issue's options were unavailable, and the doctrine decided the third. AC1 said to wire the client to "the endpoint that actually carries canvas events": there was none. Its alternative — remove as superseded by the shared bus — was equally false, since the bus had no
canvas_cellpublisher and no other consumer. The approved direction was "a WebSocket route onapi/canvas.pyplus the agent-side publisher", andEVENT_STATE_DOCTRINEforbids the first half of that in two places:Its anti-goals do grant own sockets to binary and high-volume streams — VNC framebuffers, audio, PTY bytes, log tails — and a JSON cell delta is not in that carve-out. So serving the route the client asked for would have built the third transport my own #17643 had just finished documenting. I built the channel instead and did not serve the socket. The guard fails if anyone adds one.
What Changed
live_event_manager.py—canvasjoins_VALID_PREFIXES, and the module docstring's channel list with it.api/live_events.py—_authorize_canvas_channel, gating oncanvas.user_id == caller: the same rule every REST route on the resource applies, asked rather than re-derived, so the socket and the REST surface cannot drift into disagreeing about who may see a canvas. Admins bypass, as on every other prefix. Fails closed on an unknown canvas, an unreadable store, or a raising lookup — the doctrine lists that as a rule with teeth, and it matters here because publishing on a channel makes authorization load-bearing for the first time.api/canvas.py— the three routes that write a cell publishcanvas_cellafter committing. Before the commit would announce a write a rollback discards; inside the persistence step would make delivery its side effect (principle 4). ThePUTupsert collects the cells it actually mutated and publishes each after the single commit.canvas/events.py(new) — the publisher.PersistStrategy.NONEdeliberately: the durable fact is the cell row, and a client that was away re-reads it withGET /api/canvas/{id}, which is what principle 2 asks. It reusesget_canvas_cell_event's payload shape rather than re-specifying one, so the field names the frontend already parses stay the contract. It never raises into its caller — the write has committed by then, and a delivery failure must not turn a successful save into an error.useCanvasWebSocket.ts— subscribes tocanvas:{id}on the shared bus. The message handling is unchanged, conflict branch included — it was written for exactly this event and had simply never been reached. It unsubscribes its own channel on unmount rather than disconnecting a socket other live views share.The file-size rule made me split, not raise
api/canvas.pywas grandfathered at 637 lines in a shrink-only ratchet, and the call sites pushed it to 647. Rich-payload validation — 63 lines, pure, no router or session — moved tocanvas/rich_payload.py. The file is now 584, underMAX_LINES, so itsKNOWN_LARGEandRATCHET_BASELINEentries are deleted, per the tool's own message:So
api/canvas.pycomes off the grandfather list entirely, which is what a shrink-only baseline is for.Verification
Pre-push selected and ran
autobot-backend/api/canvas_channel_authz_17020_test.pyitself — 8 authorization tests, including the non-owner refusal, the unknown canvas, the raising lookup, and the username fallback — and reported[pre-push OK] pytest: all relevant tests pass.Guard mutation-tested six ways, each applied then reverted:
The last two are the ones I would want a reviewer to check by hand anyway: a channel that is authorized by a function nobody calls is the same defect class as the socket nobody served.
What this does NOT deliver
Agent delta streaming still has no producer. Nothing writes cell content except those three REST routes, so
seqcarries the cell's version anddeltacarries the full content rather than an incremental chunk. What lands here is every real cell change reaching every subscriber — which is the doctrine's "does a second connected client see the same thing as the first". An agent streaming path publishes to the same channel when it is built, with no new transport and no client change.The three unreachable streaming helpers (
register_canvas_streaming_task, its unregister pair) are left in place and undeleted — they are the cancellation half of that future producer, and "cleanup means finishing the work".Not claimed: no host verification, no browser observation. Two-tab live behaviour rests on the unit tests, the authz tests, and the guard.
Closes #17020
Refs #7425
Refs #17009
Model Used
Claude Opus 5 (1M context)
Summary by CodeRabbit