implement collaboration upgrade proposals - #1856
Conversation
…rable overrides, moderation, layer locks) opengeos#1681
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCollaboration sessions now support identity requirements, invite tokens, locked layers, durable participant permissions, kicking, blocking, and origin/rate-limit controls across shared protocol, relay workers, persistence, and the desktop interface. ChangesCollaboration controls
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
There was a problem hiding this comment.
Pull request overview
This PR upgrades GeoLibre’s live-collaboration feature set end-to-end (shared protocol + relay/DO implementations + desktop UI) by adding invite links, session identity requirements, host moderation controls, layer-lock enforcement, and basic session-creation protections.
Changes:
- Extends the collaboration wire protocol/state to support invites, identity binding, session config, moderation (kick/block), and layer locks.
- Implements these capabilities in both relay backends (Cloudflare Durable Object + Node relay) and persists new session metadata server-side.
- Updates the desktop client/store/UI to send/receive the new messages and expose host controls; adds/adjusts tests and docs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| workers/collab/src/session.ts | Durable Object session logic: new tables (invites/blocks/overrides), join gating, moderation, layer locks, snapshot authorization updates |
| workers/collab/src/index.ts | Worker router: session creation origin allowlist + rate limiting; supports requireIdentity at init |
| workers/collab-node/src/store.ts | SQLite relay store: persists requireIdentity, locked layers, invites, durable overrides, blocked keys |
| workers/collab-node/src/server.ts | Node relay: protocol support for invites/config/moderation/locks; session creation allowlist + rate limiting |
| tests/collab-upgrade.test.ts | New tests covering participant keys, locked-layer diffing, snapshot authorization, and relay flows |
| tests/collab-comment-validate.test.ts | Updates import path for validation helpers |
| packages/core/src/types.ts | Core types: adds identity/invite structures and collaboration state fields |
| packages/core/src/store.ts | Default collaboration state extended for new fields |
| packages/collab-core/src/session.ts | Shared collab logic: participant keys, layer-lock diffing, snapshot authorization updates |
| packages/collab-core/src/protocol.ts | Wire protocol: new message types + new error codes + welcome payload extensions |
| docs/collaboration.md | Protocol documentation updated for new message types and fields |
| apps/geolibre-desktop/src/lib/collab-protocol.ts | Client protocol/types mirrored; adds participantCanEditLayer + new message definitions |
| apps/geolibre-desktop/src/lib/collab-client.ts | Session creation API extended to include requireIdentity |
| apps/geolibre-desktop/src/i18n/locales/en.json | Adds new collaboration UI strings (kick/block/require identity) |
| apps/geolibre-desktop/src/hooks/useCollaboration.ts | Client runtime: handles new server messages and sends new host actions |
| apps/geolibre-desktop/src/components/layout/CollaborationParticipantRow.tsx | Participant row UI: adds kick/block controls + identity badge |
| apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx | Dialog UI: adds require-identity start toggle + host session setting wiring |
Suppressed comments (5)
workers/collab/src/session.ts:501
participantKeyis derived from a temporary{ clientId: "temp" }participant, but block/durable-override writes usegetParticipantKey(target.attachment)(which uses the real, server-assigned clientId for anonymous users). That makes moderation/overrides ineffective for anonymous participants because the keys never match. Use the same generated clientId for both the participantKey lookup and the socket attachment.
const tempParticipant: SessionParticipant = {
clientId: "temp",
displayName: identity ? identity.username : sanitizeDisplayName(message.displayName),
color: sanitizeColor(message.color),
role,
workers/collab/src/session.ts:1061
- Invite tokens are generated with
crypto.randomUUID().slice(0, 16), which both reduces entropy (only ~64 bits of hex) and includes a hyphen, making tokens easier to guess than necessary. Use random bytes for an unguessable token.
const role: CollaborationMode = message.role === "view-only" ? "view-only" : "co-edit";
const token = crypto.randomUUID().slice(0, 16);
const invite: CollabInvite = {
workers/collab-node/src/server.ts:314
participantKeyis computed from a temporary{ clientId: "temp" }participant, but block/durable-override writes usegetParticipantKey(targetPeer.participant)(which uses the real server-assigned clientId for anonymous users). This mismatch makes blocking and durable overrides ineffective for anonymous participants. Use the same generated clientId for both the key lookup andpeer.participant.clientId.
const tempParticipant: SessionParticipant = {
clientId: "temp",
displayName: identity ? identity.username : sanitizeDisplayName(message.displayName),
color: sanitizeColor(message.color),
role: role as CollaborationRole,
workers/collab-node/src/server.ts:446
- Invite tokens are generated with
randomUUID().slice(0, 16), which reduces entropy (~64 bits) and includes a hyphen. Use random bytes for an unguessable token.
const role: CollaborationMode = message.role === "view-only" ? "view-only" : "co-edit";
const token = randomUUID().slice(0, 16);
const invite: CollabInvite = {
apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx:408
- This label is hard-coded English text. Use the existing i18n key (
collaborate.requireIdentityLabel) so the host-side session setting is translated consistently.
onChange={(e) => onSetSessionConfig?.({ requireIdentity: e.target.checked })}
className="h-3.5 w-3.5 rounded border"
/>
Require signed-in account to join
</label>
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx (1)
136-148: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPass the authenticated identity when joining.
api.join()supportsidentityToken, but this call provides no options. The relay rejects every non-host join when the host enablesrequireIdentity. The user then cannot join through this dialog.Obtain the current authenticated identity from the desktop auth flow and pass its verified token as
api.join(..., { identityToken }).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx` around lines 136 - 148, Update handleJoin to obtain the current authenticated identity from the desktop authentication flow and extract its verified identityToken before calling api.join. Pass that token in the join options as api.join(code.trim(), name.trim(), color, { identityToken }), while preserving the existing validation, busy state, and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx`:
- Around line 282-291: Localize all collaboration strings: in
apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx lines 282-291
and 399-408, replace the literal account-requirement label with
t("collaborate.requireIdentityLabel"); in
apps/geolibre-desktop/src/hooks/useCollaboration.ts lines 255-261, add the
English fallback for the kicked-session message to en.json and resolve it
through i18n.t().
In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts`:
- Around line 425-431: Update the server-side collaboration session
authentication around createSession and the relay handler in server.ts to
validate a cryptographically signed, issuer-bound, unexpired identity credential
before allowing requireIdentity sessions. Derive the participant identity
exclusively from verified credential claims, and reject forged or malformed
client-supplied identityToken JSON rather than treating it as authentication.
- Around line 263-266: Update the "error" branch in useCollaboration’s message
handling to detect pendingConnectRef.current, clear it, terminate the failed
connection, set connecting to false, and reject the pending promise with
message.message. Preserve the existing store update and too-large handling, and
leave post-join error behavior unchanged when no connection promise is pending.
In `@docs/collaboration.md`:
- Around line 71-76: Update the collaboration command table entries for
mint-invite and block-participant to describe the relays’ current behavior
accurately, unless the related relay handlers and session logic are changed to
enforce the documented semantics. Do not claim invite roles grant co-edit access
while joining still forces guest, or that blocking prevents anonymous rejoining
when fresh keys are generated.
- Around line 90-94: Update the stale operator note below the collaboration
event table to document the ALLOWED_ORIGINS configuration and the request limit
enforced by isAllowedOrigin and checkRateLimit, replacing the claim that POST
/sessions is unauthenticated, allows every origin, and requires future
restriction work.
In `@packages/collab-core/src/session.ts`:
- Around line 62-72: Update getParticipantKey in
packages/collab-core/src/session.ts#L62-L72 to return null when neither identity
nor inviteToken is present. In workers/collab/src/session.ts#L497-L516, remove
the tempParticipant clientId "temp" and guard isBlockedKey/readDurableOverride
when the key is null. Apply the same null-key guard to store.isBlockedKey and
store.getDurableOverride in workers/collab-node/src/server.ts#L310-L329, and
skip store.saveDurableOverride in workers/collab-node/src/server.ts#L426-L432
when no durable key exists.
- Around line 111-113: Replace the order-sensitive JSON.stringify comparison in
the locked-layer validation with a structural deep-equality check, or
canonicalize both layer objects with deterministically sorted keys before
comparing. Preserve the existing { lockedId, layerName } rejection result only
for genuinely different layer content.
- Around line 210-211: Update toWireParticipant and the related
CollabParticipant and CollaborationParticipant protocol types to emit only
displayName in participant roster and welcome payloads; remove identity and
inviteToken from the wire representation while retaining them server-side, and
use a boolean or redacted field only if signed-in status is required.
- Around line 143-157: Move the existing byteLength > maxBytes validation in the
session validation flow to execute before the locked-layer block that calls
diffLockedLayers. Preserve the current rejection behavior and ensure oversized
inbound snapshots are rejected without performing the locked-layer diff.
In `@tests/collab-upgrade.test.ts`:
- Around line 109-205: Update the end-to-end test around the relay, hostWs, and
guestWs lifecycle to register t.after() cleanup that closes or terminates both
WebSocket clients and awaits relay.close() even when setup, waits, or assertions
fail. Add timeouts and rejection handlers to every WebSocket Promise wait,
including welcome, invite-created, and kick/close waits, so stalled connections
fail promptly and cleanup always runs.
In `@workers/collab-node/src/server.ts`:
- Around line 89-101: Update checkRateLimit to evict expired entries from
rateLimitMap so varying keys cannot grow the map indefinitely, while preserving
the existing per-key window behavior. At the client-key selection near the
request handling call site, use x-forwarded-for only when the relay is
configured behind a trusted proxy; otherwise always use
request.socket.remoteAddress, preventing callers from spoofing keys through the
header.
- Around line 266-269: Annotate the local role variable in the message handling
flow with the existing CollaborationRole type, preserving the current host/guest
conditional values. Then remove the redundant role as CollaborationRole casts at
the later uses around lines 314, 335, and 346.
- Around line 53-87: Update isAllowedOrigin in both server.ts and
collab/src/index.ts so localhost, 127.0.0.1, and *.localhost are allowed only
when an explicit development flag is enabled; otherwise require the configured
allowlist. Keep missing Origin accepted, but document that this is intentional
defense-in-depth behavior and does not authenticate non-browser clients.
In `@workers/collab-node/src/store.ts`:
- Around line 154-196: Add an index for collab_invites(session_id) in the table
schema or migration, and rename the CollabStore method createInvite to
saveInvite, updating every caller—including the useCount persistence path in the
server—while preserving its existing INSERT OR REPLACE behavior.
- Around line 244-260: Wrap the four statements in delete with a database
transaction so session and related rows are removed atomically. Also update
deleteStaleBefore to execute the stale-row scan and all delete calls within one
transaction, preserving the keepSet filtering while avoiding per-row transaction
commits.
- Around line 52-90: The database initialization in the store constructor must
migrate existing collab_sessions tables rather than relying only on CREATE TABLE
IF NOT EXISTS. Add a migration that detects and adds every column required by
current schema and writes, including require_identity, locked_layer_ids, and
rev, while preserving existing data and remaining safe for already-upgraded
databases. Ensure create continues to insert through its current collab_sessions
schema.
In `@workers/collab/src/index.ts`:
- Around line 29-74: Move isAllowedOrigin and checkRateLimit into
`@geolibre/collab-core` and export both helpers. Make isAllowedOrigin receive the
allowlist string as a parameter so each relay can supply its existing source,
then remove the local duplicate implementations and import the shared helpers in
workers/collab/src/index.ts and workers/collab-node/src/server.ts while
preserving current behavior.
- Around line 62-74: Document in or adjacent to checkRateLimit that rateLimitMap
is isolate-local and therefore the configured limit is only a best-effort first
line, not a durable distributed ceiling. Preserve the existing in-memory
behavior, and add an operator-facing note recommending a Cloudflare Rate
Limiting binding or an IP-keyed Durable Object for enforced cross-isolate
limits.
- Around line 114-117: Update the clientKey selection near checkRateLimit to
prefer request.headers.get("CF-Connecting-IP") and fall back to origin, then
"anonymous". Preserve the existing checkRateLimit call and rejection response.
In `@workers/collab/src/session.ts`:
- Around line 472-495: Replace the unsigned JSON parsing in the session identity
flow with verification of a signed JWT using crypto.subtle, validating its
signature, issuer, audience, and expiry before constructing ParticipantIdentity.
Update the corresponding identity handling in the collab-node server as well;
until verification succeeds, identityToken must not affect requireIdentity
admission, displayName, getParticipantKey override lookup, or blocked-key
checks.
- Around line 1069-1070: Update the invite notification in the mint-invite
handler to use broadcastHostOnly instead of send(ws, ...), matching
handleRevokeInvite so every host socket receives invite-created while preserving
the existing invite payload.
- Around line 555-559: Update the initialization around the mode,
lockedLayerIds, and storedRaw destructuring so the storage reads and
readSnapshot() are passed directly to Promise.all without inner awaits, then
apply "co-edit" and [] defaults after destructuring or at their use sites,
matching handleJoin’s pattern.
- Around line 458-470: Update the invite handling in the session join flow
around readInvites and handleMintInvite so a valid invite’s persisted role is
applied as the participant editOverride, preferring any existing durable
override and falling back to inv.role. Remove the ineffective role reassignment,
while preserving invite validation, usage counting, and writing behavior.
- Around line 1059-1065: Update invite creation in the session message handler
to use the full value returned by crypto.randomUUID() without slicing. Validate
message.maxUses before storing it: accept only a finite positive integer, omit
it otherwise, and preserve unlimited-use behavior when no valid limit is
provided.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx`:
- Around line 136-148: Update handleJoin to obtain the current authenticated
identity from the desktop authentication flow and extract its verified
identityToken before calling api.join. Pass that token in the join options as
api.join(code.trim(), name.trim(), color, { identityToken }), while preserving
the existing validation, busy state, and error handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aba50851-d742-4df1-826b-32d0780d7af1
📒 Files selected for processing (17)
apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsxapps/geolibre-desktop/src/components/layout/CollaborationParticipantRow.tsxapps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/collab-client.tsapps/geolibre-desktop/src/lib/collab-protocol.tsdocs/collaboration.mdpackages/collab-core/src/protocol.tspackages/collab-core/src/session.tspackages/core/src/store.tspackages/core/src/types.tstests/collab-comment-validate.test.tstests/collab-upgrade.test.tsworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab/src/index.tsworkers/collab/src/session.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/collab-upgrade.test.ts (1)
135-157: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject each socket wait when the WebSocket emits
error.Each wait handles only success events and timeout. If either WebSocket fails, its unhandled
errorevent can terminate the test process beforet.after()completes. Add anerrorlistener that clears the timer, removes temporary listeners, and rejects the active wait.#!/bin/bash set -euo pipefail # Inspect the installed WebSocket dependency and every event handler in this test. rg -n -C 3 '"ws"|new WebSocket|\.on\("error"|\.once\("error"|timeout' \ package.json package-lock.json workers testsAlso applies to: 162-173, 179-194, 199-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/collab-upgrade.test.ts` around lines 135 - 157, Update each WebSocket wait in the test, including the host connection and the ranges noted in the comment, to handle the socket error event. On success or error, clear the timeout and remove the temporary open/message/error listeners before resolving or rejecting, ensuring socket failures reject the active wait instead of becoming unhandled events.apps/geolibre-desktop/src/hooks/useCollaboration.ts (1)
305-313:⚠️ Potential issue | 🔴 CriticalDo not treat
identityTokenas an authentication assertion.The relay still parses client-provided JSON and trusts
userId,username, andproviderwithout signature, issuer, or expiry validation. A guest can forge a token and satisfyrequireIdentity. Validate a signed credential on the relay and derive identity only from verified claims. Also requireproviderto be a bounded string becauseCollaborationParticipantRowrenders it directly.The relay handler in
workers/collab-node/src/server.tsis the enforcement path for this payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts` around lines 305 - 313, Update the relay enforcement path in the server handler receiving the join payload, not just useCollaboration, so identityToken is accepted only as a cryptographically verified credential with validated signature, issuer, and expiry. Derive userId, username, and provider exclusively from verified claims, reject forged or invalid tokens, and require provider to be a bounded string before constructing CollaborationParticipantRow.apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx (1)
97-101: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWire collaboration credentials and invite management into
CollaborateDialog.The generated link contains only
collab, so it cannot carry an invite role.handleJoinomitsinviteTokenandidentityToken, which causesrequireIdentityjoins to returnidentity-requiredand prevents invite edit permissions. Add an authenticated identity-token source, preserveinviteTokenin the link, pass both options toapi.join, and exposecollaboration.invites,api.mintInvite, andapi.revokeInvitethroughActiveSession.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx` around lines 97 - 101, Update CollaborateDialog’s shareLink to preserve inviteToken, add an authenticated identity-token source, and update handleJoin to pass inviteToken and identityToken to api.join. Extend ActiveSession to expose collaboration.invites plus api.mintInvite and api.revokeInvite so invite management and edit permissions are available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/collab-upgrade.test.ts`:
- Around line 160-195: Extend the guest join assertions after receiving
guestWelcome to locate the invited guest in welcome.participants and assert that
its editOverride is false. Use the participant identity exposed by guestWelcome
or the joined display name, while preserving the existing role assertion.
In `@workers/collab-node/src/server.ts`:
- Around line 315-324: Validate the client-supplied identityToken using the
server’s configured credential verification, including signature and relevant
issuer, audience, and expiry checks, before constructing joiningParticipant.
Build ParticipantIdentity only from verified claims, reject invalid or
unverifiable tokens when requireIdentity applies, and ensure getParticipantKey
receives only the verified identity.
In `@workers/collab/src/session.ts`:
- Around line 711-712: Update handleSetMode to delete every persisted durable
override for the session when the host changes session mode, rather than
clearing only connected attachments. Reuse the existing durable-override
storage/deletion mechanism and ensure disconnected participants’ rows in
collab_durable_overrides are removed before reconnect restoration can occur.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsx`:
- Around line 97-101: Update CollaborateDialog’s shareLink to preserve
inviteToken, add an authenticated identity-token source, and update handleJoin
to pass inviteToken and identityToken to api.join. Extend ActiveSession to
expose collaboration.invites plus api.mintInvite and api.revokeInvite so invite
management and edit permissions are available.
In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts`:
- Around line 305-313: Update the relay enforcement path in the server handler
receiving the join payload, not just useCollaboration, so identityToken is
accepted only as a cryptographically verified credential with validated
signature, issuer, and expiry. Derive userId, username, and provider exclusively
from verified claims, reject forged or invalid tokens, and require provider to
be a bounded string before constructing CollaborationParticipantRow.
In `@tests/collab-upgrade.test.ts`:
- Around line 135-157: Update each WebSocket wait in the test, including the
host connection and the ranges noted in the comment, to handle the socket error
event. On success or error, clear the timeout and remove the temporary
open/message/error listeners before resolving or rejecting, ensuring socket
failures reject the active wait instead of becoming unhandled events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87fb8aee-1518-43c1-8d16-aabbc58c925e
📒 Files selected for processing (10)
apps/geolibre-desktop/src/components/layout/CollaborateDialog.tsxapps/geolibre-desktop/src/components/layout/CollaborationParticipantRow.tsxapps/geolibre-desktop/src/hooks/useCollaboration.tspackages/collab-core/src/protocol.tspackages/collab-core/src/session.tspackages/core/src/types.tstests/collab-upgrade.test.tsworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab/src/session.ts
💤 Files with no reviewable changes (2)
- packages/core/src/types.ts
- packages/collab-core/src/protocol.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/collab-node/src/server.ts (1)
771-775: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not trust
X-Forwarded-Forwithout an explicit trusted-proxy boundary.If the relay is directly reachable, a caller can send a different first
X-Forwarded-Forvalue on every request. This bypasses the per-client limit and creates unbounded distinct rate-limit keys. Userequest.socket.remoteAddressby default. ReadX-Forwarded-Foronly when an explicit trusted-proxy configuration guarantees that the proxy overwrites it.#!/bin/bash set -euo pipefail # Verify whether a trusted proxy boundary exists and overwrites X-Forwarded-For. rg -n -i -C 3 \ 'COLLAB_TRUST_PROXY|trusted proxy|proxy_set_header.*x-forwarded-for|x-forwarded-for' \ . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab-node/src/server.ts` around lines 771 - 775, Update the client IP selection before checkRateLimit to use request.socket.remoteAddress by default, and only honor the first X-Forwarded-For value when an explicit trusted-proxy configuration confirms the proxy overwrites that header. Ensure untrusted callers cannot supply arbitrary rate-limit keys.
♻️ Duplicate comments (1)
workers/collab/src/session.ts (1)
484-500: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftVerify identity credentials before using them for authorization.
Both relays accept arbitrary JSON as
identityToken. A caller can satisfyrequireIdentity, impersonate a user, evade an identity-based block, or select another user's durable permission override. BuildParticipantIdentityonly from a server-verified credential with integrity and expiry checks.
workers/collab/src/session.ts#L484-L500: verify the credential before settingidentityand before the identity-required check.workers/collab-node/src/server.ts#L290-L306: apply the same verification before derivingparticipantKeyand loading overrides or blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 484 - 500, Replace the unverified JSON parsing in the session identity flow with the server-side credential verifier, requiring valid integrity and expiry checks before constructing ParticipantIdentity or evaluating requireIdentity; update workers/collab/src/session.ts lines 484-500 accordingly. Apply the same verification in workers/collab-node/src/server.ts lines 290-306 before deriving participantKey or loading durable permission overrides and blocks, reusing the verified identity data in both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/collaboration.md`:
- Around line 208-215: Update the Operator note describing POST /sessions and
ALLOWED_ORIGINS to characterize isAllowedOrigin as browser-origin filtering or
defense-in-depth only, not authentication or a general server-side access gate.
Preserve the existing explanation that non-browser clients may omit these
headers and remain supported, unless a verifiable credential requirement is
added.
In `@workers/collab/src/index.ts`:
- Around line 64-70: Update the rate-limit cleanup logic in
workers/collab/src/index.ts at lines 64-70 to run sweeps on a scheduled interval
rather than on every request after 5,000 entries, and enforce a bounded maximum
key capacity by evicting entries as needed. Apply the same bounded cleanup
behavior in workers/collab-node/src/server.ts at lines 62-70 to keep relay
implementations consistent; retain expiration-based eviction while preventing
repeated full-map scans and unbounded active-key growth.
---
Outside diff comments:
In `@workers/collab-node/src/server.ts`:
- Around line 771-775: Update the client IP selection before checkRateLimit to
use request.socket.remoteAddress by default, and only honor the first
X-Forwarded-For value when an explicit trusted-proxy configuration confirms the
proxy overwrites that header. Ensure untrusted callers cannot supply arbitrary
rate-limit keys.
---
Duplicate comments:
In `@workers/collab/src/session.ts`:
- Around line 484-500: Replace the unverified JSON parsing in the session
identity flow with the server-side credential verifier, requiring valid
integrity and expiry checks before constructing ParticipantIdentity or
evaluating requireIdentity; update workers/collab/src/session.ts lines 484-500
accordingly. Apply the same verification in workers/collab-node/src/server.ts
lines 290-306 before deriving participantKey or loading durable permission
overrides and blocks, reusing the verified identity data in both paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b65e2959-a941-4fdf-8b1b-42e7f9befe8b
📒 Files selected for processing (6)
docs/collaboration.mdtests/collab-upgrade.test.tsworkers/collab-node/src/server.tsworkers/collab-node/src/store.tsworkers/collab/src/index.tsworkers/collab/src/session.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/collab-node/src/server.ts (1)
292-303: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winConsume an invite only after join authorization succeeds.
This code increments
useCountbefore the identity-required and blocked-participant checks. A caller with a limited-use invite can send an invalid identity token, or use a blocked identity, to exhaust the invite without joining.Defer
store.saveInvite()until after Lines 322-349 succeed.Proposed fix
- let inviteToken: string | undefined = undefined; + let inviteToken: string | undefined; + let invite: CollabInvite | undefined; if (message.inviteToken && typeof message.inviteToken === "string") { const invites = store.getInvites(id); const inv = invites.find((i) => i.token === message.inviteToken && !i.revoked); if (inv && (!inv.maxUses || inv.useCount < inv.maxUses)) { + invite = inv; inviteToken = inv.token; - inv.useCount += 1; - store.saveInvite(id, inv); } } ... if (store.isBlockedKey(id, participantKey)) { // reject } + if (invite) { + invite.useCount += 1; + store.saveInvite(id, invite); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab-node/src/server.ts` around lines 292 - 303, Defer invite consumption in the join flow: in the invite lookup block around inviteToken assignment, record the matched invite without incrementing useCount or calling store.saveInvite. After the identity-required and blocked-participant authorization checks in the join handling flow succeed, increment the invite’s useCount and persist it, ensuring failed authorization attempts do not consume limited-use invites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab-node/test/relay.test.ts`:
- Around line 314-330: Update the test around the existing trustProxy
start/fetch flow to verify rate-limit client-key behavior: send eleven session
requests with distinct X-Forwarded-For values for each server. Assert the
untrusted server returns 429 on the eleventh request because all requests share
the socket address, while the trusted server accepts the distinct
forwarded-address requests without exhausting one bucket.
---
Outside diff comments:
In `@workers/collab-node/src/server.ts`:
- Around line 292-303: Defer invite consumption in the join flow: in the invite
lookup block around inviteToken assignment, record the matched invite without
incrementing useCount or calling store.saveInvite. After the identity-required
and blocked-participant authorization checks in the join handling flow succeed,
increment the invite’s useCount and persist it, ensuring failed authorization
attempts do not consume limited-use invites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58bef95a-accf-4b12-874b-542c58d5251e
📒 Files selected for processing (4)
docs/collaboration.mdworkers/collab-node/src/server.tsworkers/collab-node/test/relay.test.tsworkers/collab/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab-node/test/relay.test.ts`:
- Around line 389-391: Extend the successful-join flow in the relay test after
the existing welcome assertion and authGuest.close() to connect a second
authenticated guest using the same inviteToken, then assert that the relay
rejects this join, verifying the maxUses: 1 invite was consumed.
In `@workers/collab/src/session.ts`:
- Around line 471-477: Only resolve and assign matchedInvite in the invite-token
join logic when role === "guest", preventing host joins that also provide a
valid hostToken from consuming invite uses. Apply this guard in
workers/collab/src/session.ts lines 471-477 and
workers/collab-node/src/server.ts lines 289-295.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71f18f0c-7f2a-44d2-a212-d911f0d7f105
📒 Files selected for processing (3)
workers/collab-node/src/server.tsworkers/collab-node/test/relay.test.tsworkers/collab/src/session.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <option value="view-only">{t("collaborate.modeViewOnly")}</option> | ||
| </Select> | ||
| </div> | ||
| <label className="flex cursor-pointer items-center gap-2 text-xs text-muted-foreground pt-1"> | ||
| <input | ||
| type="checkbox" | ||
| checked={requireIdentity} | ||
| onChange={(e) => setRequireIdentity(e.target.checked)} | ||
| disabled={busy} | ||
| className="h-3.5 w-3.5 rounded border" | ||
| /> | ||
| {(t as (key: string) => string)("collaborate.requireIdentityLabel")} | ||
| </label> |
There was a problem hiding this comment.
Quality/completeness: enabling "Require signed-in account to join" will lock out every guest, since there's no sign-in flow anywhere in the app.
identityToken is only ever produced by hand — searching the frontend, join(...) is called with no options (here and in CommentsPanel.tsx), so the client never actually sends an identityToken. There's no OAuth/sign-in UI, no ?collab= invite-link handling that carries a token, and mintInvite/revokeInvite/setLayerLocks (wired up in useCollaboration.ts) aren't called from anywhere in the UI either.
Net effect as shipped: a host who checks this new box will find that literally no one — including well-intentioned collaborators — can join anymore, since the app has no way to obtain a valid identity token. Same for the invite-link/layer-lock features described in the PR summary: the server/protocol/store support is all there, but there's no way for a user to reach it through the app.
Worth either wiring up minimal UI for these (invite link generation + copy, layer-lock toggles in the layer panel) before merging, or gating the "Require signed-in account" checkbox behind a flag/tooltip noting it's not yet usable, so hosts don't accidentally strand their own session.
Confidence: medium — based on searching the frontend for callers of join, mintInvite, and setLayerLocks and finding none that supply the new options.
There was a problem hiding this comment.
Done in 1757525. The gate can no longer strand a host: with no issuer configured, POST /sessions answers 400 and set-session-config answers a new identity-unavailable error, and the relay advertises identitySupported on /health and in every welcome so the Collaborate dialog hides the "Require signed-in account" checkbox in both the create form and the in-session host controls. GeoLibre ships no issuer, so the option is dark by default. Agreed that mintInvite/revokeInvite/setLayerLocks still have no UI — that is additive rather than a footgun (nothing a host can toggle locks them out), so leaving it for a follow-up rather than growing this PR further.
| @@ -69,15 +169,30 @@ export function authorizeSnapshot( | |||
| return { | |||
| ok: false, | |||
| code: "too-large", | |||
| message: "Project is too large to sync live. Share it via URL instead.", | |||
| message: `Snapshot byte length (${byteLength}) exceeds maximum allowed (${maxBytes}).`, | |||
| }; | |||
| } | |||
| if ( | |||
| participant.role !== "host" && | |||
| lockedLayerIds.length > 0 && | |||
| storedSnapshot && | |||
| inboundSnapshot | |||
| ) { | |||
| const diff = diffLockedLayers(storedSnapshot, inboundSnapshot, lockedLayerIds); | |||
| if (diff) { | |||
| return { | |||
| ok: false, | |||
| code: "layer-locked", | |||
| message: `Layer "${diff.layerName}" is locked by the host and cannot be modified.`, | |||
| }; | |||
| } | |||
| } | |||
| return { ok: true }; | |||
| } | |||
There was a problem hiding this comment.
Two smaller items in this function, grouping them here rather than as separate comments:
-
Perf (low confidence):
diffLockedLayersdoes a full recursive deep-equality walk (isStructurallyEqual) over every locked layer's stored vs. inbound object on every snapshot from a non-host participant. Snapshots can embed full GeoJSON datasets up toMAX_SNAPSHOT_BYTES(10 MB per the comment above). Inworkers/collab-node, this runs synchronously in a single process hosting every session — the existing code already calls out (elsewhere in this file) that a blocking SQLite read on one session adds latency to other sessions in that process, and this deep-diff has the same property. Worth profiling with a large locked layer, or at least capping the comparison (e.g. skip properties known to be immutable/irrelevant) if it turns out to matter. -
Quality (low confidence): the
too-largemessage changed from a user-facing "Project is too large to sync live. Share it via URL instead." to a raw technical string (`Snapshot byte length (${byteLength}) exceeds maximum allowed (${maxBytes}).`, line 172) that gets surfaced verbatim in the Collaborate dialog's error banner. This is a UX regression — exposing byte counts instead of actionable guidance, and it's not translated via i18n like the rest of the UI strings in this PR.
There was a problem hiding this comment.
Item 2 (the too-large message) was already fixed in 5238173, which landed after this review ran — it is back to "Project is too large to sync live. Share it via URL instead." It stays a server-side string, so it is not routed through the client i18n catalogs; translating relay errors would be a separate change across every error code.
Item 1 (the diffLockedLayers deep walk) I am leaving as-is. It only runs for a non-host snapshot when locks are active, and only over layers whose IDs are in lockedLayerIds — not the whole project — and it short-circuits on the first difference. The expensive case is a large locked layer that is unchanged, which is exactly the case the lock exists to police; skipping properties would trade a correctness guarantee for speed on a path that has no measurement behind it yet. Worth revisiting with a profile if a real session shows it.
|
I've completed a thorough review of this PR. Here's the summary comment. Code reviewSecurity
Bugs
Quality
Performance
Not flagged as blocking, but noted inline for context: the origin/rate-limit changes and durable-override/invite persistence logic look correct and are reasonably tested by the new |
… and clean up setParticipantOverride
- Verify `identityToken` cryptographically instead of trusting self-reported JSON. New `verifyIdentityToken`/`signIdentityToken` in `@geolibre/collab-core` check an HMAC-SHA256 signature over the token's payload segment, so a client can no longer mint an account, impersonate a user, evade an identity-keyed block, or wear the roster's verified badge. - Make identity opt-in per deployment via `COLLAB_IDENTITY_SECRET` (Worker secret / env var). With no secret configured every token verifies to null and all joiners are anonymous, rather than the relay accepting whatever it is told. - Stop `requireIdentity` from stranding a host's own session: without an issuer, `POST /sessions` answers 400 and `set-session-config` answers the new `identity-unavailable` error, and the relay reports `identitySupported` on `/health` and in every `welcome` so the Collaborate dialog hides the "require a signed-in account" option entirely instead of offering a gate no guest could pass. - Chain per-peer frame handling in the Node relay, since identity verification made `handleMessage` async and `ws` delivers frames without waiting — a `snapshot` sent right behind a `join` would otherwise overtake it. - Document that `block-participant` is durable only for identity- and invite-based joins: an anonymous guest gets a fresh `clientId` per join and can rejoin by reconnecting. `docs/collaboration.md` no longer claims it bans a participant from rejoining, and `getParticipantKey` says so at the source. - Add `tests/collab-identity.test.ts` (signature, tampering, expiry, malformed input, unconfigured relay) and two relay tests covering a self-asserted token and one signed with the wrong secret.
| * Effective layer edit permission. Host can edit any layer; guests with general | ||
| * edit permission cannot edit layers that are explicitly locked. | ||
| */ | ||
| export function participantCanEditLayer( |
There was a problem hiding this comment.
participantCanEditLayer is exported here but, as far as I can find, never called from any UI component (layer panel, map editing tools, etc.) — only from the relay's authorizeSnapshot server-side and from the new test. That means a guest with edit permission gets no proactive feedback that a layer is locked; they can freely edit it locally, and only discover the lock when their whole-project snapshot push is rejected with a layer-locked error (which just surfaces as a generic error string in useCollaboration.ts's case "error", with no special handling to explain which layer or revert the local edit). Worth wiring this into whatever gates layer editing in the UI so locked layers are visibly read-only rather than silently failing to sync.
Confidence: medium — functionally plausible from the code, but I haven't traced every editing entry point, so there may be a gating mechanism I missed.
There was a problem hiding this comment.
Accurate — participantCanEditLayer is only reached server-side today, so a guest discovers a locked layer when the snapshot push is rejected rather than up front. Leaving it as-is here: gating every layer-editing entry point and rendering locked layers read-only is a UI feature in its own right, and it lands in the same follow-up as the missing invite-link and layer-lock host controls (thread above). The server-side authorization is correct and tested; what is missing is proactive UI feedback, so this is a completeness gap rather than a defect in this PR. Leaving open for @HarshShinde0 to weigh in on scope.
- Correct the docs' "durable across reconnect" claim for `set-participant-mode`. A persisted per-participant override is keyed the same way a block is, so it survives a reconnect only for identity- and invite-based joins; an anonymous guest gets a fresh `clientId`, a fresh participant key, and reverts to the session default. `getParticipantKey` now names the override alongside the block in the same caveat.
| if (role === "guest" && message.inviteToken && typeof message.inviteToken === "string") { | ||
| const invites = store.getInvites(id); | ||
| const inv = invites.find((i) => i.token === message.inviteToken && !i.revoked); | ||
| if (inv && (!inv.maxUses || inv.useCount < inv.maxUses)) { | ||
| inviteToken = inv.token; | ||
| matchedInvite = inv; | ||
| } | ||
| } | ||
|
|
||
| // Signature-checked against the configured issuer secret, so `identity` | ||
| // is only ever non-null for a credential this deployment actually minted. | ||
| // With no secret configured it is null for every token, making every | ||
| // joiner anonymous rather than trusting self-reported claims. | ||
| const identity: ParticipantIdentity | null = await verifyIdentityToken( | ||
| message.identityToken, | ||
| identitySecret, | ||
| ); | ||
|
|
||
| if (persisted.requireIdentity && !identity && role !== "host") { | ||
| send(peer, { | ||
| type: "error", | ||
| code: "identity-required", | ||
| message: "Sign-in required to join this session.", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const socketClientId = randomUUID(); | ||
| const joiningParticipant: SessionParticipant = { | ||
| clientId: socketClientId, | ||
| displayName: identity ? identity.username : sanitizeDisplayName(message.displayName), | ||
| color: sanitizeColor(message.color), | ||
| role, | ||
| identity, | ||
| inviteToken, | ||
| }; | ||
| const participantKey = getParticipantKey(joiningParticipant); | ||
|
|
||
| if (store.isBlockedKey(id, participantKey)) { | ||
| send(peer, { | ||
| type: "error", | ||
| code: "forbidden", | ||
| message: "You have been blocked from this session by the host.", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const durableOverride = store.getDurableOverride(id, participantKey); | ||
| let initialOverride: boolean | undefined = undefined; | ||
| if (durableOverride !== undefined) { | ||
| initialOverride = durableOverride; | ||
| } else if (matchedInvite) { | ||
| initialOverride = matchedInvite.role === "co-edit"; | ||
| } | ||
|
|
||
| if (matchedInvite) { | ||
| matchedInvite.useCount += 1; | ||
| store.saveInvite(id, matchedInvite); | ||
| } |
There was a problem hiding this comment.
Possible TOCTOU race on invite maxUses enforcement. The invite's use-count check (inv.useCount < inv.maxUses, line 312) happens before await verifyIdentityToken(...) (line 322), and the increment/persist (matchedInvite.useCount += 1; store.saveInvite(...), lines 364-366) happens after that await. If two joins for the same maxUses: 1 invite arrive close together (which is realistic when identitySecret is configured, since verifyIdentityToken then does real async Web Crypto work rather than resolving synchronously), both can read useCount: 0 before either writes, and both get granted the invite's role — exceeding maxUses.
Consider re-validating the invite (or using an atomic UPDATE ... WHERE use_count < max_uses in SQLite) at the point of increment rather than trusting the earlier read. The same pattern exists in workers/collab/src/session.ts (~line 484/537).
Confidence: medium — exploitability depends on Node's Web Crypto HMAC implementation actually yielding the event loop, which I couldn't fully verify from static reading, but the check-then-later-write shape across an await is a genuine hazard regardless.
| const [mode, lockedLayerIds, storedRaw] = await Promise.all([ | ||
| (await this.ctx.storage.get<CollaborationMode>("mode")) ?? "co-edit", | ||
| (await this.ctx.storage.get<string[]>("lockedLayerIds")) ?? [], | ||
| this.readSnapshot(), | ||
| ]); |
There was a problem hiding this comment.
Promise.all is used here but two of its three array elements are already awaited before being placed in the array ((await this.ctx.storage.get(...)) ?? "co-edit" and (await this.ctx.storage.get(...)) ?? []). Array literals evaluate left-to-right, so each await blocks evaluation of the next element — this runs the two storage reads sequentially, then starts readSnapshot(), giving no concurrency benefit at all. Since this runs on every snapshot message (the hottest path in the relay), the extra round-trip latency is paid on every edit broadcast.
| const [mode, lockedLayerIds, storedRaw] = await Promise.all([ | |
| (await this.ctx.storage.get<CollaborationMode>("mode")) ?? "co-edit", | |
| (await this.ctx.storage.get<string[]>("lockedLayerIds")) ?? [], | |
| this.readSnapshot(), | |
| ]); | |
| const [mode, lockedLayerIds, storedRaw] = await Promise.all([ | |
| this.ctx.storage.get<CollaborationMode>("mode"), | |
| this.ctx.storage.get<string[]>("lockedLayerIds"), | |
| this.readSnapshot(), | |
| ]); | |
| const resolvedMode = mode ?? "co-edit"; | |
| const resolvedLockedLayerIds = lockedLayerIds ?? []; |
(then use resolvedMode/resolvedLockedLayerIds below, or keep the ?? fallback applied after destructuring instead of inside the array literal).
Confidence: high — this is a straightforward readability/perf defect, not a correctness bug.
| // account" option before it ever tries to create a session. | ||
| return json(response, 200, { ok: true, service: "geolibre-collab", identitySupported }); | ||
| if (url.pathname === "/sessions" && request.method === "POST") { | ||
| const origin = request.headers["origin"]; |
There was a problem hiding this comment.
This relay only reads the Origin header for the isAllowedOrigin check, whereas the parallel Cloudflare Worker implementation (workers/collab/src/index.ts) falls back to Referer when Origin is absent: request.headers.get("Origin") ?? request.headers.get("Referer"). The two relays are documented as implementing identical semantics (docs/collaboration.md's operator note describes a single behavior for "POST /sessions"), so this is a behavioral inconsistency between deployment targets — a request with no Origin header but a browser-set Referer is treated differently depending on which relay backs the deployment.
Confidence: low-medium — minor, and arguably the Node version's stricter behavior (ignoring the more spoofable/less reliable Referer) is fine, but the divergence itself seems unintentional given the shared isAllowedOrigin logic was clearly ported between the two files.
| if (message.snapshot) { | ||
| applyRemoteSnapshot(message.snapshot, true); | ||
| } else if (message.role === "host") { | ||
| // A newly created session has no relay snapshot yet. The store | ||
| // subscription only observes changes made after attach(), so without | ||
| // this seed a project (especially external-plugin layers loaded | ||
| // before starting collaboration) stays invisible to the first guest | ||
| // until the host happens to edit something. | ||
| void sendSnapshot(); | ||
| } | ||
| // Guests follow the host by default. Apply the host's latest presence | ||
| // immediately instead of waiting for their next moveend event. | ||
| if (message.role === "guest" && useAppStore.getState().collaboration.followHost) { | ||
| const host = message.participants.find((participant) => participant.role === "host"); |
There was a problem hiding this comment.
This PR strips a large number (~90 lines across the diff) of pre-existing "why" comments that are unrelated to the collaboration-upgrade feature itself — e.g. the explanation of why a host seeds the snapshot here ("A newly created session has no relay snapshot yet..."), why guests apply the host's presence immediately ("Guests follow the host by default..."), the store.get()-blocks-the-event-loop rationale in the relay's presence fast path, the RETURNING-based revision-read rationale in store.ts, the server-side clientId-assignment rationale in session.ts, etc. None of these were made obsolete by the logic changes in this PR — they still describe current behavior — they were simply deleted.
Per this repo's own guidance (CLAUDE.md: comments should stay when "the WHY is non-obvious... a hidden constraint, a subtle invariant"), these look like exactly the comments worth keeping, and their removal isn't explained by or connected to the feature being added. Worth double-checking this wasn't an unintended side effect of an automated formatting/cleanup pass, since it meaningfully reduces documentation for future maintainers across several files (CollaborateDialog.tsx, useCollaboration.ts, workers/collab-node/src/server.ts, workers/collab-node/src/store.ts, workers/collab/src/session.ts).
Confidence: medium — this is a judgment call on value, but the fact that dozens of unrelated, still-accurate explanatory comments were deleted in a feature PR is objectively verifiable from the diff.
| // Show a localized message; keep the raw error in the console for | ||
| // diagnostics (collab-client throws human-readable English strings). | ||
| console.error("[GeoLibre] Collaboration error", err); | ||
| setError(t("collaborate.connectFailed")); |
There was a problem hiding this comment.
Minor UX note: this PR adds several new join-rejection reasons the relay can send back (identity-required, forbidden for a blocked participant key), each with a specific message string (e.g. "You have been blocked from this session by the host.", "Sign-in required to join this session."). api.join now rejects with new Error(message.message) carrying that specific text (see the new pendingConnectRef handling in useCollaboration.ts), but this catch block discards it in favor of the generic t("collaborate.connectFailed"). A user who gets blocked or hits an identity gate sees the same "connection failed" message as a plain wrong invite code, with no way to tell why.
This pattern predates this PR (the removed comment even called it out as intentional), so it may be out of scope — but the set of distinguishable failure reasons has grown meaningfully with this feature, which makes surfacing err.message (or at least distinguishing "blocked"/"identity-required" specifically) more valuable than it was before.
Confidence: low-medium.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
…s all 18 locales (#1886) * i18n: translate the collaboration moderation and no-CDN strings Fills the 9 keys that landed in en.json with the live collaboration upgrade (#1856) and the GEOLIBRE_NO_EXTERNAL_CDN build flag (#1880) but never reached the other catalogs: - collaborate.kick / block (participant moderation buttons) - collaborate.requireIdentityLabel (session identity checkbox) - collaborate.lockLayer / unlockLayer / layerLocked / layerLockedHint - objectDetection.unavailableNoExternalCdn - segmentEverything.unavailableNoExternalCdn All 18 non-English locales were missing exactly these 9, so every one now reports 100% against the English baseline. Translations follow each catalog's existing vocabulary (host, guest, participant, layer, lock/unlock) and quotation style, and the feature name inside the Segment Everything message reuses that catalog's own segmentEverything.title. * Address CodeRabbit review feedback - tr: lockLayer/unlockLayer now say "konuklar için" instead of dative "konuklara". "bir seyi birine kilitlemek" is a colloquial idiom meaning to dump a task on someone, not to restrict their access, so the dative read as the wrong sense entirely. - es: kick/block take the personal "a" ("Expulsar al participante", "Bloquear al participante"), which Spanish requires for a specific human direct object. - vi: kick is now "Dua nguoi tham gia ra khoi phien". The previous "Loai nguoi tham gia" was ambiguous, since "loai" also reads as the noun "type", so the button could parse as "participant type". - ar: requireIdentityLabel now uses "ishtirat" (stipulating) rather than "talab" (requesting), which understated that sign-in is mandatory. Kept the verbal-noun form the catalog uses for every other option label (tadmin, izhar, istikhdam, as-samah) rather than the suggested finite verb, which would have read as a statement instead of a toggle label.
Adds invite link roles, account requirements, host kick/block controls, layer locks, and rate limits to live collaboration sessions.
All unit tests and relay checks pass cleanly.
Summary by CodeRabbit
New Features
Bug Fixes