Team collaboration (3/3): web — team workspace surfaces - #5284
Conversation
lefarcen
added
size/XXL
|
Quick body follow-up: the surface list is clear, but before pool review scopes this cleanly could you add the missing Why, What users will see, and Validation sections? Since UI is checked here, please also add screenshots (entry-point or before/after captures for the new team/community surfaces) so reviewers can evaluate the user-visible changes without rebuilding locally. |
3 similar comments
|
Quick body follow-up: the surface list is clear, but before pool review scopes this cleanly could you add the missing Why, What users will see, and Validation sections? Since UI is checked here, please also add screenshots (entry-point or before/after captures for the new team/community surfaces) so reviewers can evaluate the user-visible changes without rebuilding locally. |
|
Quick body follow-up: the surface list is clear, but before pool review scopes this cleanly could you add the missing Why, What users will see, and Validation sections? Since UI is checked here, please also add screenshots (entry-point or before/after captures for the new team/community surfaces) so reviewers can evaluate the user-visible changes without rebuilding locally. |
|
Quick body follow-up: the surface list is clear, but before pool review scopes this cleanly could you add the missing Why, What users will see, and Validation sections? Since UI is checked here, please also add screenshots (entry-point or before/after captures for the new team/community surfaces) so reviewers can evaluate the user-visible changes without rebuilding locally. |
Visual regression reviewHead:
0 changed · 0 unchanged · 47 new without baseline · 1 failed Capture or diff failures
New cases without baselines
27 additional new case(s) omitted from this comment. Visual diff is advisory only and does not block merging. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 2 blocking issues on this head.
apps/web/public/community-templates/open-design-landing.png: the new asset is 2,058,917 bytes, which trips the repository's 1,048,576-byte blob guard (Static gate is red for this exact file). As a result the PR cannot merge as-is. Please move this image to repository-assets R2 or re-encode/compress it under the threshold, then rerun the check.
I left one inline comment for the broken template remix action.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| } else if (route.kind === 'collab-demo') { | ||
| appMain = <CollabDemoView projectId={route.projectId} />; | ||
| } else if (route.kind === 'community') { | ||
| appMain = <CommunityView onRemixTemplate={() => navigate({ kind: 'home', view: 'home' })} />; |
There was a problem hiding this comment.
CommunityView calls onRemixTemplate(template.id) for every non-prompt card, but the callback wired here ignores that argument and always navigates to the plain home route. That means every visible "Remix" action drops the selected template and lands on a generic page instead of opening a remix flow for the chosen artifact. Please thread the templateId into a template-aware destination here (for example, open the new-project template tab with that id preselected) rather than discarding it.
|
@lefarcen nettee's current-head review already covers the blocking items here: the oversized 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
lefarcen
changed the title
c5aa916 to
fd92a7d
Compare
864dada to
095b877
Compare
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 3 blocking issues on this head.
apps/web/public/community-templates/open-design-landing.png is still 2,058,917 bytes, so Static gate is red on this exact file and the PR cannot merge until the asset is moved to repository-assets R2 or recompressed under the 1,048,576-byte limit.
I left two inline comments for the remaining functional blockers in the community remix flow and the shared-project read-only gate.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/collab/useProjectCollab.ts RIGHT line 104
sharedReadOnly only falls back to read-only for context?.role === 'member'. Because useCollab starts with ownerMemberId: null, any admin/owner who opens someone else's shared project will keep edit affordances until the owner id arrives from /collab/status (and the same thing happens if that payload is briefly missing ownerMemberId). That breaks the single-writer/read-only contract this PR is introducing. Please treat a shared project as read-only until the owner id explicitly matches the current member, or preload the owner id before rendering editable controls.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| /> | ||
| ) : null} | ||
| {view === 'community' ? ( | ||
| <CommunityView onRemixTemplate={() => changeView('home')} /> |
There was a problem hiding this comment.
CommunityView calls onRemixTemplate(template.id) for every non-prompt card, but this callback still ignores that templateId and just sends the user back to home. That means the new gallery's primary action drops the selected template instead of opening a template-specific remix/create flow, so users still cannot remix any community template into a project. Please thread the selected templateId into a real destination here (and keep the duplicate App.tsx branch in sync), then add a test that asserts the chosen template id reaches that flow.
|
Current-head follow-up: @nettee's latest review is still the blocker here, and it now covers three items on this head — the oversized Once those are fixed on this head, this should be ready for another pass. |
fd92a7d to
d327f8a
Compare
095b877 to
8a11f2d
Compare
|
Addressing the review:
|
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 2 blocking issues on this head.
The current branch still cannot merge because the new community gallery ships an oversized asset that fails Static gate, and the primary "Remix" CTA still discards the selected template id instead of opening a template-specific flow.
| }; | ||
|
|
||
| const TEMPLATE_PREVIEW_SRC: Record<string, string> = { | ||
| 'electric-studio': '/community-templates/open-design-landing.png', |
There was a problem hiding this comment.
/community-templates/open-design-landing.png is the 2,058,917-byte blob that is still tripping the repository's 1,048,576-byte asset guard, so Static gate stays red and this PR cannot merge as-is. Because this new mapping is what pulls that file into the shipped gallery, the branch remains blocked until the image is moved to repository-assets R2 or re-encoded under the size limit and these references are updated to point at the smaller asset.
| /> | ||
| ) : null} | ||
| {view === 'community' ? ( | ||
| <CommunityView onRemixTemplate={() => changeView('home')} /> |
There was a problem hiding this comment.
CommunityView calls onRemixTemplate(template.id) for every non-prompt card, but this callback still ignores that templateId and always routes back to plain home. That means every visible "Remix" action drops the user's selected template instead of opening a template-specific create/remix flow, so the new gallery's primary CTA is still broken. Please thread the selected id into the actual destination here, and keep the duplicate /community branch in App.tsx in sync so both entry points preserve the chosen template.
d327f8a to
245bde8
Compare
8a11f2d to
dcc6137
Compare
|
Pushed fix:
Rebased onto the latest |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 2 blocking issues on this head. I also tried to rerun the targeted web tests in this worktree, but dependencies are not installed here (vitest is missing), so I could not verify them locally.
| /> | ||
| ) : null} | ||
| {view === 'community' ? ( | ||
| <CommunityView onRemixTemplate={() => changeView('home')} /> |
There was a problem hiding this comment.
CommunityView calls onRemixTemplate(template.id) for every non-prompt card, but this callback still discards the templateId and always routes to plain home. That leaves the new gallery's primary "Remix" CTA broken on the current head: users select a specific template, click Remix in the card or preview modal, and land on a generic page with no template context. Please thread the selected id into a real template-specific create/remix destination here, keep the duplicate /community branch in App.tsx in sync, and add a regression test that proves the chosen template id reaches that flow.
| } | ||
|
|
||
| const fileActions = ( | ||
| const fileActions = viewerOnly ? null : ( |
There was a problem hiding this comment.
viewerOnly is introduced here as the read-only gate for shared projects, but this branch only hides the top-bar create/upload buttons and the empty-state CTAs. The same panel still keeps the body drop zone active (onDrop={handleDrop}), still renders selection plus the batch delete bar, and still exposes the row menu/rename path below, so a shared-project viewer can still hit upload/delete/rename affordances in a surface that is supposed to be single-writer read-only. Please make viewerOnly suppress every mutation entry point in DesignFilesPanel (drop upload, selection/delete, row menu/rename, project actions, etc.) and add a focused panel test that asserts those controls are absent in read-only mode.
245bde8 to
030ada6
Compare
dcc6137 to
986f298
Compare
|
Pushed fixes for the read-only and remix findings. Fixed — shared-project read-only fails closed. Fixed — Fixed — the community "Remix" CTA threads the template. It no longer discards the id and navigates to a generic page. It now seeds the Home composer with a template-specific starting prompt and hands the user into Home (both the standalone Oversized asset — already resolved. The 2 MB |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 2 blocking issues in the new shared-project read-only path.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/src/collab/useProjectCollab.ts RIGHT line 105
This still fails open before the first /collab/status poll completes. collab.syncState starts as null, so shared is false here and viewerOnly stays false for every writable team workspace until the async status request lands. A non-owner who deep-links into an already-shared project can therefore see editable UI for the initial render window, which breaks the "fail closed" guarantee described in the comment above. Please either seed the hook with known share state from the project payload, or pessimistically treat team workspaces as read-only until the first status response proves local_only or confirms the current member as the owner.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
| {projectTypeLabel ? ( | ||
| <span className="meta" data-testid="project-meta">{projectTypeLabel}</span> | ||
| ) : null} | ||
| {projectCollab.enabled ? ( |
There was a problem hiding this comment.
viewerOnly is now threaded into ChatPane and FileWorkspace, but this same header block still leaves two mutation controls live: the project title remains contentEditable (onBlur still calls handleProjectRename), and the DesignSystemPicker below still calls handleChangeDesignSystemId. That means a member viewing someone else’s shared project can still rename the project or swap its design system even though the rest of the workspace is read-only. Please gate these header controls on projectCollab.viewerOnly as well (for example, render plain text instead of an editable title and disable or hide the picker), then add a focused regression test for the shared-project header state.
|
Current-head follow-up: @nettee's latest review is still the blocker here, and it is now narrowed to the shared-project read-only path — Once those two are addressed on this head, this should be ready for another pass. |
986f298 to
0868aea
Compare
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found 1 remaining blocking issue in the shared-project read-only path. I also tried to rerun the targeted web tests here, but this worktree does not have dependencies installed (vitest is missing), so I could not verify them locally.
| {projectTypeLabel ? ( | ||
| <span className="meta" data-testid="project-meta">{projectTypeLabel}</span> | ||
| ) : null} | ||
| {projectCollab.enabled ? ( |
There was a problem hiding this comment.
projectCollab.viewerOnly still does not gate the header mutations in this block. The project title above remains contentEditable and still calls handleProjectRename on blur, and the DesignSystemPicker below still forwards onChange={handleChangeDesignSystemId}. That means a member viewing someone else’s shared project can still rename the project or swap its design system even though the rest of the workspace has been switched into read-only mode. Please make this header fail closed too: when viewerOnly is true, render plain text instead of an editable title and disable or hide the design-system picker, then add a focused regression test that exercises the shared-project header state.
|
Current-head follow-up: @nettee's latest review is now narrowed to one blocker in the shared-project read-only path — Once that header is gated on |
3 participants
Why
The team-edition UI, on top of the backend (part 1) and the collab client layer
(part 2). This is where the collaboration and team surfaces become visible.
What users will see
Community and recents in the sidebar); a lean local rail otherwise. Gated on the
workspace being a team, not on the coding-agent choice — a BYOK workspace keeps
every team feature.
workspace freezes edits with a recovery banner. Comments on shared projects.
the cloud web console — the local client links out; it does not embed those views.
Surface area
Settings workspace region, community
Validation
pnpm --filter @open-design/web typecheck(full web) +pnpm guard