fix(desktop): search workspace stores before the global root in known_session_roots - #760
Open
tonyfettes wants to merge 4 commits into
Open
fix(desktop): search workspace stores before the global root in known_session_roots#760tonyfettes wants to merge 4 commits into
tonyfettes wants to merge 4 commits into
Conversation
…usals Archiving resolved its record through the registered-workspace mapping alone, so a conversation whose workspace had just been detached (or whose directory vanished) fell back to the global store and failed with a bare "no durable record for this conversation" — while the sidebar, which had missed the registry change, kept offering both the workspace and the conversation, and every retry failed identically. - archive_session_commit now searches every known store root for the live record (mirroring the archived-twin lookup), names an already-archived record as exactly that, and scopes the not-found refusal to what was actually searched. - EngineError derives Debug: proton logs failed handlers through Repr, which rendered the error as an opaque `<...EngineError: ...>` and swallowed the one string saying what failed. - The frontend answers a definitive archive refusal by re-adopting the workspace registry snapshot and refreshing the archived list, so a page that missed workspace.changed converges instead of staying wedged on rows the host no longer serves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A transcript load for a conversation whose workspace just detached fell
through to the global store and surfaced the engine's raw file error
("@fs.open(): ... No such file or directory"). Check the record's
existence before spending the engine read, and answer with the same
actionable refusals archive now uses: archived records say "unarchive it
first", unreachable ones name the searched scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every icon was raw codicon markup injected via Attrs::inner_html. An innerHTML-bearing element cannot be safely morphed by rabbita's positional child diff: assigning the property silently drops the real children the runtime still tracks, and the next child-list shift that lines a text span up with an icon span crashes the diff (and with it, rendering — the workspace-detach freeze). Transcribe all 33 icons into @svg element builders — path data unchanged, verified verbatim against the previous markup — so no innerHTML remains in the frontend and the hazard is gone regardless of the rabbita version in use. Icons become functions rather than shared constants, so no Props instance is aliased between mounts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…_session_roots Restore the old `session_store_root` priority: when the same session id exists in both a workspace store and the global store, find the workspace record first. The previous ordering (global first, workspaces after) could return the wrong record during archive/unarchive — unlikely to matter in practice (session id entropy makes collisions near-impossible), but correct and costs nothing. Also updates the doc comment to explain the ordering.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 596bc221e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
31
to
+35
| for path in registered_workspaces() { | ||
| roots.push(workspace_session_root(path)) | ||
| } | ||
| if resolved_session_root() is Some(root) { | ||
| roots.push(root) |
There was a problem hiding this comment.
When the same session ID exists in both a workspace and the global store—the collision this ordering is intended to handle—the desktop sidebar can display both rows because each SessionListItem retains its workspace, but ArchiveSession and SessionPayload transmit only the ID. Searching workspace roots first therefore makes Archive or Restore on the global row operate on the workspace record instead; collisions between workspaces similarly select registry order. Preserve and validate the selected row's workspace/store rather than resolving an ambiguous ID by priority.
Useful? React with 👍 / 👎.
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
known_session_rootswas ordering stores as (1) global root, (2) workspace stores — the reverse of the oldsession_store_rootpriority (which searched workspaces first, then fell back to global). This meant that if the same session id existed in both,live_record_rootandarchived_record_rootwould return the global record instead of the workspace record the sidebar row was targeting.This PR swaps the order: workspace stores first, then global root as fallback — matching the old behavior.
Why not a bigger change
The Codex review on #758 suggested passing workspace identity through the frontend message. That would add complexity to the
ArchiveSessionmessage schema and every call site, while the real fix is just reordering one array — the client hint is deliberately not trusted (the comment onlive_record_rootexplains why), so the search order is all you need.Practical impact
Session ID collisions across stores are near-impossible in practice (fingerprint entropy), so this is correctness work, not a bugfix for a user-facing issue. But the old ordering was clearly wrong and costs nothing to correct.
Stacked on #758.