feat(agent): share sidepanel sessions across agent tabs#462
Open
feat(agent): share sidepanel sessions across agent tabs#462
Conversation
Contributor
Greptile SummaryThis PR wires up a shared sidepanel session registry so that every tab the agent opens in a tool call joins the same conversation as the originating sidepanel, and mirrors the active conversation messages into Key changes:
Two concrete bugs were found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SP as Sidepanel (origin tab)
participant BG as Background SW
participant ST as SharedSidepanelSession<br/>(chrome.storage.local)
participant CV as SharedSidepanelConversation<br/>(chrome.storage.local)
participant CT as Child Tab Sidepanel
SP->>ST: ensureSharedSidepanelSession({ tabId, conversationId })
Note over ST: Creates/updates session entry
SP->>SP: Agent streams messages (tool calls targeting child tabs)
SP->>ST: linkTabToSharedSidepanelSession({ sourceTabId, targetTabId })
SP->>SP: openSidePanel(targetTabId)
SP->>CV: saveSharedSidepanelConversation(conversationId, messages)
CV-->>CT: watch() fires → setMessages(nextMessages)
CT->>ST: watchSharedSidepanelSessionForTab(tabId) fires
CT->>CT: setSharedConversationId(session.conversationId)
CT->>CV: getSharedSidepanelConversation(conversationId)
CT->>CT: Restore messages from local store
BG->>ST: removeTabFromSharedSidepanelSession(tabId) on chrome.tabs.onRemoved
Last reviewed commit: 7391c03 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+35
to
+47
| async function updateSharedSidepanelState( | ||
| updater: (state: SharedSidepanelState) => SharedSidepanelState, | ||
| ): Promise<SharedSidepanelState> { | ||
| const currentState = await sharedSidepanelSessionStorage.getValue() | ||
| const safeState = currentState ?? EMPTY_SHARED_SIDEPANEL_STATE | ||
| const nextState = updater(safeState) | ||
|
|
||
| if (nextState !== safeState) { | ||
| await sharedSidepanelSessionStorage.setValue(nextState) | ||
| } | ||
|
|
||
| return nextState | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition in read-modify-write
updateSharedSidepanelState performs a non-atomic read-modify-write. In a browser extension, ensureSharedSidepanelSession can be called concurrently from the sidepanel context and removeTabFromSharedSidepanelSession can be called simultaneously from the background service worker (on tab close). Both read the same state, compute their independent mutations, and the second write silently clobbers the first — meaning a tab removal or session link can be lost.
For example:
- Background reads state
S, starts computingremoveTab(S, tabId) - Sidepanel reads state
S, starts computingensureSession(S, { tabId, conversationId }) - Background writes
removeTab(S, tabId)— correct - Sidepanel writes
ensureSession(S, ...)— overwrites step 3, the removal is lost
The wxt-dev/storage API supports a getMeta / locking pattern or you can use an optimistic compare-and-swap loop to guard against this. Alternatively, since the background and sidepanel share chrome.storage.local, you can structure writes so each call only appends/patches its own key slice rather than replacing the whole object.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/agent/lib/sidepanel/shared-sidepanel-session.ts
Line: 35-47
Comment:
**Race condition in read-modify-write**
`updateSharedSidepanelState` performs a non-atomic read-modify-write. In a browser extension, `ensureSharedSidepanelSession` can be called concurrently from the sidepanel context and `removeTabFromSharedSidepanelSession` can be called simultaneously from the background service worker (on tab close). Both read the same state, compute their independent mutations, and the second write silently clobbers the first — meaning a tab removal or session link can be lost.
For example:
1. Background reads state `S`, starts computing `removeTab(S, tabId)`
2. Sidepanel reads state `S`, starts computing `ensureSession(S, { tabId, conversationId })`
3. Background writes `removeTab(S, tabId)` — correct
4. Sidepanel writes `ensureSession(S, ...)` — **overwrites step 3**, the removal is lost
The `wxt-dev/storage` API supports a `getMeta` / locking pattern or you can use an optimistic compare-and-swap loop to guard against this. Alternatively, since the background and sidepanel share `chrome.storage.local`, you can structure writes so each call only appends/patches its own key slice rather than replacing the whole object.
How can I resolve this? If you propose a fix, please make it concise.| cancelled = true | ||
| } | ||
| }, [conversationId, status, hasToolCalls, toolTabId]) | ||
| }, [conversationId, hostTabId, status, hasToolCalls, toolTabId, toolTabIds]) |
Contributor
There was a problem hiding this comment.
toolTabIds array causes spurious effect re-runs
toolTabIds is a new array reference on every render (created by extractTabIds), so including it in the useEffect dependency array causes the effect to fire on every render whenever the component re-renders — even if the actual tab IDs haven't changed. During streaming, messages state updates frequently, causing frequent re-renders. Each spurious trigger calls linkTabToSharedSidepanelSession and openSidePanel for every tab ID in a loop, flooding the storage layer and the Chrome API with redundant calls.
Since hasToolCalls (boolean) and toolTabId (the last tab ID) are already in deps, the effect is already correctly gated on meaningful state changes for the activation/deactivation logic. The toolTabIds array is only needed inside the loop — it can be read from a ref to avoid the referential instability:
const toolTabIdsRef = useRef<number[]>([])
toolTabIdsRef.current = toolTabIds // keep it current each render
// in the effect body use toolTabIdsRef.current instead of toolTabIds
// and remove toolTabIds from the deps arrayThis ensures the loop always has the latest tab IDs without making the entire effect re-run every render.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/agent/entrypoints/sidepanel/index/useNotifyActiveTab.tsx
Line: 130
Comment:
**`toolTabIds` array causes spurious effect re-runs**
`toolTabIds` is a new array reference on every render (created by `extractTabIds`), so including it in the `useEffect` dependency array causes the effect to fire on **every render** whenever the component re-renders — even if the actual tab IDs haven't changed. During streaming, `messages` state updates frequently, causing frequent re-renders. Each spurious trigger calls `linkTabToSharedSidepanelSession` and `openSidePanel` for every tab ID in a loop, flooding the storage layer and the Chrome API with redundant calls.
Since `hasToolCalls` (boolean) and `toolTabId` (the last tab ID) are already in deps, the effect is already correctly gated on meaningful state changes for the activation/deactivation logic. The `toolTabIds` array is only needed inside the loop — it can be read from a ref to avoid the referential instability:
```ts
const toolTabIdsRef = useRef<number[]>([])
toolTabIdsRef.current = toolTabIds // keep it current each render
// in the effect body use toolTabIdsRef.current instead of toolTabIds
// and remove toolTabIds from the deps array
```
This ensures the loop always has the latest tab IDs without making the entire effect re-run every render.
How can I resolve this? If you propose a fix, please make it concise.
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
Testing
Notes