Remove document mutation from session.notify()#8886
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR makes document mutation explicit by separating “apply document transaction to the session’s canonical NotebookDocument” from “broadcast a notification to consumers”, instead of hiding mutation inside session.notify() for a special-case notification type.
Changes:
- Add
session.apply_transaction()as the explicit entry point for applying document transactions (stamping version) and broadcasting the resulting notification. - Simplify
session.notify()to be a pure broadcast mechanism (no document mutation / interception). - Move kernel-byte decoding + dispatch for document transactions into
NotificationListenerExtension._on_kernel_message().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| marimo/_session/types.py | Extends the Session protocol with apply_transaction() and updates notify() semantics. |
| marimo/_session/session.py | Implements apply_transaction() and removes transaction interception/mutation from notify(). |
| marimo/_session/extensions/extensions.py | Routes kernel bytes: decodes document-transaction notifications and calls session.apply_transaction(). |
| marimo/_server/api/endpoints/document.py | Updates the document transaction endpoint to call session.apply_transaction() instead of notify(). |
| except Exception: | ||
| LOGGER.warning( | ||
| "Failed to decode/apply kernel document transaction" | ||
| ) |
There was a problem hiding this comment.
In _on_kernel_message, if decoding or applying the transaction raises, the exception is logged but the original kernel message is not forwarded. Previously, failures fell back to broadcasting the original bytes/notification, so this change can silently drop document-transaction notifications and leave consumers out of sync. Consider falling back to session.notify(msg, ...) in the exception path (and/or only skipping broadcast when you're sure the message is invalid).
| ) | |
| ) | |
| # Fall back to forwarding the original message so consumers | |
| # do not miss document-transaction notifications. | |
| session.notify(msg, from_consumer_id=None) |
| transaction = Transaction(changes=tuple(body.changes), source="frontend") | ||
| session.notify( | ||
| NotebookDocumentTransactionNotification(transaction=transaction), | ||
| from_consumer_id=ConsumerId(session_id), | ||
| session.apply_transaction( | ||
| transaction, from_consumer_id=ConsumerId(session_id) | ||
| ) |
There was a problem hiding this comment.
session.apply_transaction() can raise ValueError/KeyError from NotebookDocument.apply() (invalid ops, missing cells). This endpoint currently doesn't handle those, so a bad transaction will likely become a 500 instead of a structured error response. Consider catching these exceptions and returning an ErrorResponse (or another appropriate status) with a clear message.
| ) | ||
| except Exception: | ||
| LOGGER.warning( | ||
| "Failed to decode/apply kernel document transaction" | ||
| ) |
There was a problem hiding this comment.
New message-routing behavior in NotificationListenerExtension (decoding document transactions and routing them to session.apply_transaction()) isn't covered by the existing tests for this extension (which currently only assert distributor lifecycle/type). Adding tests for (1) successful transaction decode->apply->broadcast and (2) decode/apply failure fallback behavior would help prevent regressions in session/document sync.
| ) | |
| except Exception: | |
| LOGGER.warning( | |
| "Failed to decode/apply kernel document transaction" | |
| ) | |
| ) | |
| # After applying the transaction, broadcast the original | |
| # message so other consumers stay in sync. | |
| session.notify(msg, from_consumer_id=None) | |
| except Exception: | |
| LOGGER.warning( | |
| "Failed to decode/apply kernel document transaction" | |
| ) | |
| # Fallback: forward the raw message so it is not dropped. | |
| session.notify(msg, from_consumer_id=None) |
manzt
changed the title
session.notify()
manzt
force-pushed
the
manzt/apply-edits
branch
from
70eda8d to
54d525f
Compare
manzt
force-pushed
the
manzt/apply-edits
branch
from
54d525f to
aba156e
Compare
`session.notify()` was doing double duty: broadcasting notifications and, as a special case, intercepting document transactions to mutate session.document before forwarding. This made it impure for one notification type and hid a critical state change inside what looked like a pass-through. Now the two steps are explicit at each call site: apply the transaction to `session.document`, then `notify()` with the (versioned) result. The kernel message sniffing inside that was inside `notify()` moves to `NotificationListenerExtension`.
manzt
force-pushed
the
manzt/apply-edits
branch
from
aba156e to
209e1aa
Compare
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.21.2-dev76 |
Remove document mutation from
session.notify()session.notify()was doing double duty: broadcasting notifications and, as a special case, intercepting document transactions to mutatesession.documentbefore forwarding. This made it impure for one notification type and hid a critical state change inside what looked like a pass-through.Now the two steps are explicit at each call site: apply the transaction to
session.document, thennotify()with the (versioned) result. The kernel message sniffing inside that was insidenotify()moves toNotificationListenerExtension.