Team collaboration (2/3): web — in-project collaboration + i18n - #5283
Conversation
lefarcen
added
size/XL
|
Could you expand the PR body a bit before pool review picks this up? For an XL UI change, it's still missing the Why / What users will see / Screenshots / Validation pieces, and those will make the collab + read-only flows much faster to scope. Related: part of the same team-collab series as #5277, #5280, #5281, #5282, and #5284. |
Visual regression reviewHead:
0 changed · 0 unchanged · 53 new without baseline · 0 failed New cases without baselines
33 additional new case(s) omitted from this comment. Visual diff is advisory only and does not block merging. |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thanks for pushing this collaboration slice forward. I reviewed the changed web client paths for the presence/read-only/comment-anchor flow, and I found two read-only enforcement gaps that let team viewers still mutate shared project files.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| ) : null} | ||
| <FileWorkspace | ||
| projectId={project.id} | ||
| viewerOnly={projectCollab.viewerOnly} |
There was a problem hiding this comment.
This wires viewerOnly into the workspace shell, but it does not actually protect the open file viewer. FileWorkspace still renders FileViewer without a read-only prop, and FileViewer owns several write paths for already-open files: manual edit save/undo/redo (applyManualEdit, undoManualEdit, redoManualEdit), markdown autosave (saveMarkdownText), markdown image upload (insertImageFiles), and version restore (handleVersionRestored). That means a non-owner can open an existing HTML/Markdown file in a shared project and still persist changes even though the new notice says they cannot edit files. Please propagate the read-only state into FileViewer and gate all file-mutating actions there, while keeping comment-only actions available.
| } | ||
|
|
||
| const fileActions = ( | ||
| const fileActions = viewerOnly ? null : ( |
There was a problem hiding this comment.
Hiding the top create/upload buttons leaves other Design Files write paths active for viewerOnly users. The same component still unconditionally handles drops via onDrop={handleDrop} and calls onUploadFiles(dropped), and the batch bar still renders its delete button whenever selected.size > 0, so a read-only viewer can upload by dragging files into the panel and can delete files after selecting rows. Since this PR defines viewerOnly as withholding create/upload/delete affordances, please make the panel read-only end-to-end: disable/omit drag handlers and copy drop effects when viewerOnly, suppress or disable batch deletion, and ideally hide row mutation menus as well.
|
@lefarcen mrcfps's current blockers are the two read-only enforcement gaps in
|
lefarcen
changed the title
a8cd808 to
6def8f5
Compare
c5aa916 to
fd92a7d
Compare
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thanks for continuing to break the collaboration work into reviewable layers. I reviewed the current web collab client/session/hooks/test changes and found one small source hygiene issue that is worth fixing before this layer becomes a dependency for the next UI slice.
Non-blocking finding
apps/web/src/collab/useCollab.ts, memberKey in useCollab: the new source file contains literal NUL bytes between the template segments on the memberKey line. Evidence from the live PR diff: GitHub/gh pr diff --patch renders this TypeScript file as a GIT binary patch, and locally file apps/web/src/collab/useCollab.ts reports data; grep -nUa locates the NULs on the memberKey line. That makes the file hard to review in GitHub, hides future line-level diffs, and can cause text-oriented repo tooling or search to skip a normal TS source file. Please replace the raw separators with text source that produces the same runtime value, for example \u0000 escapes inside the template literal or a printable/structured key such as JSON.stringify([member.memberId, member.name ?? '', member.role ?? '']).
I could not run the focused web validation locally because this prepared worktree has no node_modules; pnpm --filter @open-design/web typecheck and the focused Vitest command failed before reaching PR code due missing packages like next, react, and vitest.
|
@lefarcen The current head looks down to the one non-blocking cleanup @mrcfps called out in Once that hygiene fix lands, this layer should be in a much cleaner spot for the next pass. |
fd92a7d to
d327f8a
Compare
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thanks for the continued iteration here. I reviewed the current web collaboration client/session/hooks/test slice against the live head and the earlier feedback; the previous read-only blockers are no longer in this scoped diff, but one source hygiene issue is still present on this head.
Non-blocking finding
apps/web/src/collab/useCollab.ts, memberKey in useCollab: the new TypeScript source still contains literal NUL bytes between the template segments on the memberKey line. This is visible from the live PR diff because GitHub/gh pr diff --patch renders apps/web/src/collab/useCollab.ts as a GIT binary patch, and locally file apps/web/src/collab/useCollab.ts reports data; grep -nUa locates the NUL bytes on the memberKey line. That makes the file hard to review in GitHub, hides future line-level diffs, and can cause text-oriented repo tooling/search to skip what should be normal TS source. Please replace the raw separators with text source that preserves the same runtime identity key, for example escaped \u0000 separators in the template literal or a printable/structured key such as JSON.stringify([member.memberId, member.name ?? '', member.role ?? '']).
I could not complete local validation in this prepared worktree because node_modules is missing: pnpm --filter @open-design/web typecheck failed before reaching PR code on missing Next/React/Vitest packages, and the focused Vitest command could not find vitest.
e839441 to
d9b17be
Compare
d327f8a to
245bde8
Compare
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thanks for continuing to iterate on this collaboration client layer. I reviewed the current head's changed web collab files, checked the daemon route/contract shapes against the new client calls, and attempted the focused collab Vitest run locally. The prior read-only blockers are not in this scoped diff, but the source hygiene issue in the current head is still present and worth cleaning up before this layer becomes a dependency for the next UI slice.
Non-blocking finding
apps/web/src/collab/useCollab.ts, memberKey in useCollab: the new TypeScript source still contains literal NUL bytes between the template segments on the memberKey line. Evidence from the live PR diff/current checkout: gh pr diff --patch renders apps/web/src/collab/useCollab.ts as a GIT binary patch, file apps/web/src/collab/useCollab.ts reports data, and grep -nUa locates the NUL bytes on the memberKey line. That makes this normal TS source hard to review in GitHub, hides future line-level diffs, and can cause text-oriented repo tooling/search to skip it. Please replace the raw separators with text source that preserves the same runtime identity key, for example escaped separators in the template literal or a printable structured key such as JSON.stringify([member.memberId, member.name ?? '', member.role ?? '']).
I could not complete the focused local validation in this prepared worktree because node_modules is missing; pnpm --filter @open-design/web exec vitest run apps/web/tests/collab-client.test.ts apps/web/tests/collab-session.test.ts apps/web/tests/collab-presence-bar.test.tsx failed before reaching PR code with Command "vitest" not found.
d9b17be to
5caca3c
Compare
245bde8 to
030ada6
Compare
|
Fixed the source-hygiene finding: |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thanks for continuing to tighten this collaboration client layer. I reviewed the current head against the resolved read-only/source-hygiene feedback and found one small lifecycle issue that is worth addressing, but it should not block the PR.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| const version = typeof body?.publishedVersion === 'number' ? body.publishedVersion : null; | ||
| const syncState = (body?.syncState as ProjectSyncState | undefined) ?? null; | ||
| const ownerMemberId = typeof body?.ownerMemberId === 'string' ? body.ownerMemberId : null; | ||
| this.update({ publishedVersion: version, syncState, ownerMemberId }); |
There was a problem hiding this comment.
pollStatus() and heartbeat() both await network calls and then call this.update(...) without checking whether the client is still running. stop() clears future intervals, but it does not cancel the already-started immediate heartbeat/status requests from start(), and useCollab calls stop() during project/member changes before creating the next client. A slow response from the old project can therefore still invoke the old client's onUpdate and overwrite the hook snapshot after cleanup, briefly showing stale presence/read-only state for the previous project. Please either guard after each await with if (!this.running) return, or give each client an AbortController/generation token so stopped clients cannot publish late snapshots.
2 participants
Why
The client layer for in-project collaboration, on top of the backend contracts
(part 1 of the stack). Kept separate so the collab client primitives review apart
from the larger surface work.
What users will see
Nothing on its own — this is the collaboration client layer the project view and
team surfaces consume: the presence overlay, the collab client/session, the
per-project collab hook (presence + read-only), the workspace-context hook, and
the invite-continuation state machine.
Surface area
Validation
pnpm --filter @open-design/web typecheck