feat: add shared workspaces view#991
Conversation
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
from
77214b6 to
67f3cee
Compare
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
from
67f3cee to
5073b43
Compare
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
2 times, most recently
from
710aebd to
6072dba
Compare
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
from
6072dba to
0c47e82
Compare
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 3 | Last posted: Round 3, 19 findings (2 P2, 12 P3, 5 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedCRF-2 (P2, deploymentManager.ts:281) - Token verification race window and silent token drop on failure
Law analysisEffective LOC: 1094 (+1094 -207). Head SHA: 0c47e82. Verdict: Don't split. Enforcement: Advisory. Round logRound 1Panel. 2 P2, 7 P3, 5 Nit. 2 body-only (not posted inline). Reviewed against bea4059..0c47e82. Round 2Churn guard: PROCEED (overrode BLOCKED; CRF-8 and CRF-15 were addressed in code but had no reply because they were folded into the review body). 14 fixed, 1 contested (CRF-2). CRF-2 closed by panel (8/8). 3 new P3 findings. Reviewed against 4ae51e1..19084ea. Round 3Churn guard: PROCEED. CRF-17, CRF-18, CRF-19 all addressed (07fbfa5). No open findings remain. Reviewed against 4ae51e1..07fbfa5. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The session-state refactoring is well-designed. The SessionStore + WorkspaceSessionState interface decouples DeploymentManager from WorkspaceProvider cleanly. The revision-based snapshot is the right mechanism for stale-fetch detection, and WORKSPACE_QUERY_CONFIG replaces scattered conditionals with a single source of truth. Test density is strong at 54.4%, and the concurrency tests (session change mid-request, sign-out during pending fetch, hidden-then-visible transitions) verify real race scenarios.
Severity breakdown: 2 P2, 7 P3, 5 Nit.
The two P2s both stem from the auth listener's token-rotation path, which changed from a lightweight setCredentials() swap to a full verifyAndUpdateSession -> setDeployment -> signIn() cascade. On success, this causes tree flicker and 4 extra API calls per rotation (CRF-1). On failure, the new token is silently dropped, leaving the client holding expiring credentials (CRF-2). Both are fixable without changing the overall architecture.
Process notes: Two of the three commits have empty bodies. The feature commit should explain why shared:true includes current-user workspaces and why client-side filtering exists; that is non-obvious and only lives in the PR description currently. The PR description's collapsible "Implementation plan" section references APIs that were removed in subsequent commits (#authedUser, getCurrentUserId(), getAuthStateVersion(), filterWorkspaces callback).
"If
#verifyCredentialsthrows, thecatchblock logs a warning and returns. Neither the old nor the new token is applied to the client." --Mafuuu, on why verify-before-apply needs a fallback
src/workspace/workspacesProvider.ts:178
P3 [CRF-8] Agent metadata watchers can leak if dispose() races an in-flight fetch() that creates watchers.
dispose() calls clearState() which disposes and clears agentWatchers. If dispose() runs while fetch() is between createAgentMetadataWatcher calls:
dispose()disposes all existing watchers and clears the map.createAgentMetadataWatchercompletes and adds a new watcher to the now-empty map.setWorkspaceschecksthis.disposedand returns early.- The new watcher is never disposed; its EventSource connection leaks.
This only affects WorkspaceQuery.Mine (the only query with showMetadata: true) and requires a narrow timing window. Fix: check this.disposed before creating each new watcher inside the loop.
(Chopper)
🤖
src/workspace/workspacesProvider.ts:259
Nit [CRF-15] _onDidChangeTreeData EventEmitter is never disposed in dispose(). The disposed flag prevents new fires, so this is a resource accounting gap rather than a functional bug. Add this._onDidChangeTreeData.dispose() to dispose() for completeness. (Chopper)
🤖
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
2 times, most recently
from
175c550 to
19084ea
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 16 R1 findings addressed. CRF-2 (contested) is closed by the panel (8/8 accept). The verify-before-apply approach with suspension recovery is sound; every reviewer independently traced the three-branch recovery in verifyAndUpdateSession and confirmed the tests cover all race interleavings.
The R2 delta is clean: CRF-1 fix (same-user rotation skips revision bump) eliminates the tree rebuild, CRF-3 fix (bounded loop with disposed check) closes the unbounded recursion, and CRF-8 fix (post-await disposed guard on watcher creation) prevents the leak. Comment trimming, naming, and documentation fixes are all retained.
3 new P3 findings from this round, all in the watcher-creation section of fetch() and the retry cap.
"The verify-before-apply pattern prevents unverified tokens from reaching the live client. The suspension-recovery path handles the main race scenario." --Meruem, closing CRF-2
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 19 findings across 3 rounds are resolved. CRF-17 (MAX_FETCH_ATTEMPTS test), CRF-18 (revision re-check after watcher creation), and CRF-19 (dispose-during-watcher-creation test) are all addressed in 07fbfa5. The sessionChangedSince helper shared between the mid-request check and the watcher-creation guard is a clean factoring.
Final state: 571 production lines, 779 test lines (57.7% test density), 1667 tests passing. The session-state architecture is sound, the race conditions are covered, and the shared-workspace filtering works correctly.
"The revision-based snapshot comparison is a well-chosen mechanism for stale-fetch detection." --Knov, R1
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
from
07fbfa5 to
9c46b7a
Compare
Add a Shared Workspaces tree view alongside My and All Workspaces, with search and manual refresh. The view queries `shared:true`, which on the server matches any workspace with non-empty user or group ACLs. That set also includes workspaces the signed-in user owns and shared out, so the extension filters those out client-side by `owner_id` to leave only workspaces shared with the user. `shared_with_user:me` is not used: the server does not implement `me` there, and direct-user filtering would miss group shares.
Replace the WorkspaceProvider filter callback and ad-hoc state-version getter with a small WorkspaceSessionState abstraction. Providers now read a point-in-time snapshot (kind, revision, userId) and subscribe to changes, instead of being handed a filter function and a version getter. This puts current-user knowledge and stale-fetch detection behind one interface: DeploymentManager exposes session state in one place, and the provider compares snapshot revisions to discard responses from a session that changed mid-request.
- collapse per-query branches into a readonly WORKSPACE_QUERY_CONFIG table - drop the unreachable re-entry branch in the workspace refresh loop - remove test-only getCurrentUserId; tests read getSnapshot via a helper - extract the repeated tree-view wiring in extension.ts into a helper - drive metadata tests through the real watcher over MockEventStream - move shared test doubles (session, workspaces client, flush) into testHelpers - replace describe-scoped state + beforeEach/afterEach with a setup() helper
verifyAndUpdateSession keeps verify-before-apply: it verifies the rotated token before touching the live client, rotates credentials in place when the user is unchanged (no revision bump or tree rebuild), and recovers a session that was suspended mid-verify instead of getting stuck signed out (CRF-1, CRF-2). Also: - bound fetch() retries with a per-attempt disposed check, and dispose a metadata watcher created while dispose() races the fetch (CRF-3, CRF-8) - dispose the tree-data EventEmitter on dispose() (CRF-15) - rename clearSideEffects to clearAuthState (CRF-12) - document the session snapshot revision contract (CRF-13) - drop the dead getAxiosInstance mock helper and restore an unrelated blank line (CRF-14, CRF-16) - trim comments that restated the code (CRF-9) - add tests for dispose lifecycle, verify failure, signed-out startup, and stale-fetch recovery (CRF-4 through CRF-7)
- re-check the session revision after the watcher-creation await so a session change mid-await cannot overwrite the cleared tree with a stale session's workspaces (CRF-18) - add tests for the MAX_FETCH_ATTEMPTS exhaustion path and the dispose-during-watcher-creation guard (CRF-17, CRF-19)
EhabY
force-pushed
the
feat/shared-workspaces-review-fixes
branch
from
9c46b7a to
400b4d2
Compare
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
Gave it a run and it looks good to me! Mostly just architectural comments below, and I think the retry is not necessary?
| return this.#data; | ||
| } | ||
|
|
||
| /** Lean projection for consumers that only track auth status and revision. */ |
There was a problem hiding this comment.
Why do we make this lean copy instead of just passing back the session data? Seems like more memory for no benefit?
The PR description says this is to not "leak stale users" but I am not sure I understand what this means. If a user signs in/out, and workspace tree has stored this snapshot, it will now be stale. Even if the tree stores #data directly it would still be stale, since we set a new object rather than modifying it, it looks like?
And if the workspace tree is not storing the snapshot but rather calls getSnapshot() each time, this is still equivalent to just calling current() in terms of freshness.
| * Consumers that only need auth status (like the workspace tree) take the lean | ||
| * WorkspaceSessionState projection instead of the full session. |
There was a problem hiding this comment.
Nit: I think it is a bit of an anti-pattern to describe what should call this code because external changes may make the comment out of date ("find references" is better for this anyway).
Ideally it explains what functions should be call in what scenario, like Prefer getSnapshot() over current() if you only need the auth status because (reason here).
| * another login, dispose), so callers don't need a race guard. | ||
| */ | ||
| public async verifyAndApplyDeployment( | ||
| public async verifyAndApplySession( |
There was a problem hiding this comment.
I like the name change. I even wonder if we should just have SessionManager instead of DeploymentManager.
| private readonly deploymentTelemetry: DeploymentTelemetry; | ||
|
|
||
| #deployment: Deployment | null = null; | ||
| readonly #sessionStore = new SessionStore(); |
There was a problem hiding this comment.
It looks like all we do is expose the session store's methods directly on the deployment manager anyway? Why not just put the methods directly on it? Is there an advantage of a session store abstraction separate from the manager if it is never used in an abstract way?
| return this.#sessionStore.current.deployment; | ||
| } | ||
|
|
||
| public getSnapshot(): WorkspaceSessionSnapshot { |
There was a problem hiding this comment.
getCurrentAuthSession() maybe? deploymentManager.getSnapshot() feels non-obvious to me what it would return.
| * `revision` increments on every sign-in or sign-out. Consumers snapshot the | ||
| * revision before an async call and compare afterward to detect that the | ||
| * session changed while the call was in flight. |
There was a problem hiding this comment.
If we just grabbed the session data we could compare the object and have no need of an extra revision property I believe.
Although, I feel like, if possible, a better path than checking the session after a call is to immediately abort any calls whenever the session changes (via onDidChange). But idk how easy this is, probably would have to support it in the api we import from coder I imagine.
| } | ||
|
|
||
| // Bounds fetch() retries when the session keeps changing mid-request. | ||
| export const MAX_FETCH_ATTEMPTS = 3; |
There was a problem hiding this comment.
Is there a downside to unbounding this? If the session changes, we really should start a new request, even if it is the fourth time.
| this.config = WORKSPACE_QUERY_CONFIG[getWorkspacesQuery]; | ||
| this.sessionChangeDisposable = this.sessionState.onDidChange(() => { | ||
| this.clear(); | ||
| void this.fetchAndRefresh(); |
There was a problem hiding this comment.
Oh wait we do fetch every time it changes. This means the retry is not necessary right? Would we not potentially end up with two requests in parallel? This new one here, and potentially a retry if one was already in flight.
| // finally getting workspaces for the new one. | ||
| return this.fetch(); | ||
| } | ||
| // Session changed mid-request; this result is stale, so retry. |
There was a problem hiding this comment.
Yeah seems like here we should just return early or abort the request, since a new request will be tried anyway.
| // `shared:true` also returns workspaces we own and shared out; drop them | ||
| // to leave only those shared with us. |
There was a problem hiding this comment.
Nit: I think this comment would make more sense on excludeOwn: true above where we set it for the shared workspace filter.
2 participants
Summary
Shared Workspacestree view with search and manual refresh actions.shared:true, then filter out workspaces owned by the signed-in user insideWorkspaceProviderusing the session snapshot'suserId.DeploymentManager/SessionStoreand expose a lean snapshot (kind,revision,userId) so concurrent auth or session changes do not leak stale users into shared-workspace filtering or render stale fetches.Testing
pnpm format:checkpnpm typecheckpnpm lintpnpm test:extensionImplementation plan and decision log
origin/mainand resolved theDeploymentManagerconflict while preserving current main's recovery and concurrency guards.coder/codershared-workspace semantics:shared:truematches any workspace with non-empty user/group ACLs, including current-user-owned workspaces that were shared out, so the extension must filter outworkspace.owner_id === currentUserIdclient-side.shared_with_user:mebecause server-sidemesupport is not implemented there and direct-user filtering misses group shares.All Workspaces, instead of polling likeMy Workspaces.WorkspaceProviderfree of current-user knowledge by driving per-view behavior from aWORKSPACE_QUERY_CONFIGtable and reading the session through a leanWorkspaceSessionState.getSnapshot()projection.SessionStoreowns the session and bumps arevisionon every sign-in/out; the provider snapshotsrevisionanduserIdbefore each fetch and compares afterward to drop stale responses and to filtershared:trueresults byowner_id.Generated with Coder Agents on behalf of @EhabY.