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
1 participant
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.