Repository navigation
Add live sessions and the Session panel - #4891
TrueDoctor wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
3 issues found across 35 files
Confidence score: 3/5
- In
persist.rs, a guest can treat an undo as successful even though no request was sent when there is no current host. Return a not-ready result or defer undo/redo bookkeeping until a host is available. - In
identity.rs, anonymous users have only 64 possible labels, so peers can appear under the same name in the Session panel. Use a larger or compound deterministic name space. - In
ingest_message_handler.rs, opening a demo through#demo/...can retain the archive’s session room because that path sendsOpeninstead ofOpenDemo. Route demo opens throughOpenDemoor otherwise mark this path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="document/format/src/persist.rs">
<violation number="1" location="document/format/src/persist.rs:450">
P2: A guest can report a retired undo as successful while no request was sent: `request_undo` returns `Ok(())` when there is no current host. Propagate a not-ready result (or defer the editor undo/redo bookkeeping) until a host is available; apply the same handling to the redo request.</violation>
</file>
<file name="editor/src/messages/portfolio/sync/identity.rs">
<violation number="1" location="editor/src/messages/portfolio/sync/identity.rs:12">
P2: This gives anonymous users only 64 possible labels, so distinct peers frequently appear under the same name in the Session panel and cannot be told apart. Use a larger or compound deterministic name space (for example, add a stable short identifier to the animal) so the stand-ins remain distinguishable.</violation>
</file>
<file name="editor/src/messages/portfolio/ingest/ingest_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/ingest/ingest_message_handler.rs:52">
P2: Demos opened through the `#demo/...` URL bypass `fresh_identity` because that loader still sends `Open`, so copies retain the archive's session room. Route all demo opens through `OpenDemo` or otherwise mark this path as a fresh identity.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| replica.broadcast_head_move(moved)?; | ||
| } | ||
| } else if let Some(replica) = &mut self.network { | ||
| replica.request_undo(rev, false)?; |
There was a problem hiding this comment.
P2: A guest can report a retired undo as successful while no request was sent: request_undo returns Ok(()) when there is no current host. Propagate a not-ready result (or defer the editor undo/redo bookkeeping) until a host is available; apply the same handling to the redo request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/format/src/persist.rs, line 450:
<comment>A guest can report a retired undo as successful while no request was sent: `request_undo` returns `Ok(())` when there is no current host. Propagate a not-ready result (or defer the editor undo/redo bookkeeping) until a host is available; apply the same handling to the redo request.</comment>
<file context>
@@ -424,6 +424,58 @@ impl<L: Layout> Gdd<L> {
+ replica.broadcast_head_move(moved)?;
+ }
+ } else if let Some(replica) = &mut self.network {
+ replica.request_undo(rev, false)?;
+ }
+ Ok(())
</file context>
| } | ||
|
|
||
| pub(crate) fn anonymous_name(user: UserId) -> String { | ||
| let index = (scramble(user) >> 40) as usize % ANONYMOUS_ANIMALS.len(); |
There was a problem hiding this comment.
P2: This gives anonymous users only 64 possible labels, so distinct peers frequently appear under the same name in the Session panel and cannot be told apart. Use a larger or compound deterministic name space (for example, add a stable short identifier to the animal) so the stand-ins remain distinguishable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At editor/src/messages/portfolio/sync/identity.rs, line 12:
<comment>This gives anonymous users only 64 possible labels, so distinct peers frequently appear under the same name in the Session panel and cannot be told apart. Use a larger or compound deterministic name space (for example, add a stable short identifier to the animal) so the stand-ins remain distinguishable.</comment>
<file context>
@@ -0,0 +1,81 @@
+}
+
+pub(crate) fn anonymous_name(user: UserId) -> String {
+ let index = (scramble(user) >> 40) as usize % ANONYMOUS_ANIMALS.len();
+ format!("Anonymous {}", ANONYMOUS_ANIMALS[index])
+}
</file context>
| IngestMessage::Ingest { data, action, mime_type, path } => { | ||
| let data_type = DataType::detect(&data, &mime_type, path.as_deref()); | ||
|
|
||
| let fresh_identity = matches!(action, IngestAction::OpenDemo); |
There was a problem hiding this comment.
P2: Demos opened through the #demo/... URL bypass fresh_identity because that loader still sends Open, so copies retain the archive's session room. Route all demo opens through OpenDemo or otherwise mark this path as a fresh identity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At editor/src/messages/portfolio/ingest/ingest_message_handler.rs, line 52:
<comment>Demos opened through the `#demo/...` URL bypass `fresh_identity` because that loader still sends `Open`, so copies retain the archive's session room. Route all demo opens through `OpenDemo` or otherwise mark this path as a fresh identity.</comment>
<file context>
@@ -49,6 +49,7 @@ impl MessageHandler<IngestMessage, IngestMessageContext> for IngestMessageHandle
IngestMessage::Ingest { data, action, mime_type, path } => {
let data_type = DataType::detect(&data, &mime_type, path.as_deref());
+ let fresh_identity = matches!(action, IngestAction::OpenDemo);
let placement = match action {
IngestAction::ResourceInput {
</file context>
269ddd0 to
156721b
Compare
156721b to
9df7d4f
Compare
9df7d4f to
f699efc
Compare
f699efc to
5b39740
Compare
5b39740 to
8d0f030
Compare
8d0f030 to
f43bac2
Compare
f43bac2 to
cef0e5b
Compare
cef0e5b to
837bf98
Compare
837bf98 to
b406d63
Compare
b406d63 to
10303ee
Compare
There was a problem hiding this comment.
2 existing issues remain and no new issues found across 38 files
Confidence score: 3/5
- In
document_history.rs, a dropped-step redo can fail while the legacy stacks have already advanced, leaving history out of sync. Return success only after storage redo succeeds, or restore the note and snapshot. - In
identity.rs, anonymous peers can get the same displayed name and be indistinguishable in the Session panel. Use a larger name space or add a stable anonymous disambiguator while keeping the user ID.
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
| PortfolioMessage::LoadPersistedState { state } => { | ||
| if let Some(layout) = state.workspace_layout { | ||
| self.workspace.panel_layout = layout; | ||
| // Add the Session panel beside the Inspector, unless the user closed it |
There was a problem hiding this comment.
I think this is not what we should do for migrating the state.
I suggest that we separately add a version to it that when incremented just discards the state or something.
or we do popper migrations like with the format.
There was a problem hiding this comment.
Alright, removing the version. It is probably fine to rely on the users to discover the new panel themselves
| document_name: Option<String>, | ||
| document_path: Option<PathBuf>, | ||
| content: Vec<u8>, | ||
| fresh_identity: bool, |
There was a problem hiding this comment.
I think this a weird place to put this
…eps and the panel layout
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Confidence score: 2/5
- In
document_history.rs, a failed storage redo can remove theUndoNotewhile leaving its redo snapshot inlegacy_redo_stack, so history bookkeeping no longer matches. Keep those updates consistent on failure. - In
persist.rs, the equality guard can leave a guest permanently unable to undo if the host disconnects or applies the drop without broadcastingHeadMove. Make the guest able to recover when that response is missing.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/document_history.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_history.rs:185">
P1: A failed storage redo leaves history bookkeeping inconsistent: this branch has already removed the `UndoNote`, while the redo snapshot remains in `legacy_redo_stack`. Storage can also advance before reporting a persistence error, so the non-session fallback may then restore an interface snapshot that disagrees with storage; preserve the note and reconcile partial storage failures before returning false.</violation>
</file>
<file name="document/format/src/persist.rs">
<violation number="1" location="document/format/src/persist.rs:431">
P1: This equality guard can permanently block a guest's undo after the request has no observable response: the host may disconnect and promote the guest, or may apply the drop but fail to broadcast its `HeadMove`. Because `undo_requested` is cleared only by `Gdd::disconnect`, the next undo returns `None` indefinitely until an unrelated head change; clear or invalidate the pending request on role changes and failed/unapplied responses, or add a retry path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| impl WorkspacePanelLayout { | ||
| /// Raised when the default layout gains a panel. | ||
| pub const VERSION: u32 = 1; |
There was a problem hiding this comment.
I still think like this it will become a migration nightmare.
would you be fine with leaving this out for now and then addressing it later and for now relying on the reset workspace done by users?
No description provided.