lhumina_code/hero_whiteboard

Fork 0

Whiteboard: read-only viewers can drag comment markers and the move persists #181

New issue

Open

opened 2026-05-13 08:45:33 +00:00 by AhmedHanafy725 · 2 comments

AhmedHanafy725 commented

2026-05-13 08:45:33 +00:00

Member

Problem

In /board/{id}/view (and the /s/{token} share link) — readonly mode — comment markers remain draggable. A viewer can grab a marker, drag it to a new position, and the dragend handler at comments.js:317-339 fires rpcCall("comment.update", { id, x, y }). The server accepts it; the move persists; everyone editing the board sees the marker move next time they refresh.

Root cause

Two things conspire:

comments.js:248 creates every marker with draggable: true unconditionally.
makeAllReadonly() (app.js:299-308) iterates WhiteboardObjects.getAllObjects() to disable dragging — but comment markers live in WhiteboardComments._comments, not in WhiteboardObjects. They are never touched.

The popover Reply / Resolve / Delete buttons are NOT a problem here — WhiteboardComments.init() is skipped in readonly so the popover container is never created and showCommentPopover bails at line 413-414. Drag is the only UI-level mutation path.

Expected

In readonly mode:

Comment markers remain visible (they are part of the content).
The user can NOT drag them.
The comment.update RPC is not fired by the viewer.

Edit mode is unchanged.

Acceptance

Opening /board/{id}/view for a board with comments shows the markers but they cannot be dragged.
Opening a /s/{token} shared link with comments: same as above.
No comment.update RPC fires from a readonly client.
Edit-mode drag-and-update still works.
No regression to comment-create / comment-resolve / comment-delete on edit-mode boards (those still work; their popover is still gated by init() running).

Out of scope

Server-side authorisation gap — a determined viewer with devtools can still call WhiteboardComments.deleteComment(id) directly, and the server has no token-scoped authz on comment.* RPCs. That is a separate backend issue (file individually if needed); it is not a UI fix.

## Problem In `/board/{id}/view` (and the `/s/{token}` share link) — readonly mode — comment markers remain draggable. A viewer can grab a marker, drag it to a new position, and the `dragend` handler at `comments.js:317-339` fires `rpcCall("comment.update", { id, x, y })`. The server accepts it; the move persists; everyone editing the board sees the marker move next time they refresh. ## Root cause Two things conspire: 1. `comments.js:248` creates every marker with `draggable: true` unconditionally. 2. `makeAllReadonly()` (`app.js:299-308`) iterates `WhiteboardObjects.getAllObjects()` to disable dragging — but comment markers live in `WhiteboardComments._comments`, not in `WhiteboardObjects`. They are never touched. The popover Reply / Resolve / Delete buttons are NOT a problem here — `WhiteboardComments.init()` is skipped in readonly so the popover container is never created and `showCommentPopover` bails at line 413-414. Drag is the only UI-level mutation path. ## Expected In readonly mode: - Comment markers remain visible (they are part of the content). - The user can NOT drag them. - The `comment.update` RPC is not fired by the viewer. Edit mode is unchanged. ## Acceptance - [ ] Opening `/board/{id}/view` for a board with comments shows the markers but they cannot be dragged. - [ ] Opening a `/s/{token}` shared link with comments: same as above. - [ ] No `comment.update` RPC fires from a readonly client. - [ ] Edit-mode drag-and-update still works. - [ ] No regression to comment-create / comment-resolve / comment-delete on edit-mode boards (those still work; their popover is still gated by `init()` running). ## Out of scope Server-side authorisation gap — a determined viewer with devtools can still call `WhiteboardComments.deleteComment(id)` directly, and the server has no token-scoped authz on `comment.*` RPCs. That is a separate backend issue (file individually if needed); it is not a UI fix.

AhmedHanafy725 commented

2026-05-13 08:46:04 +00:00

Author

Member

Implementation Spec for Issue #181

Objective

In readonly mode (/board/{id}/view and /s/{token}), comment markers must render but be undraggable.

Approach

Smallest, contained change to comments.js + a one-liner from app.js:

Add a module-level _readonly flag in comments.js.
Export setReadonly(value) that flips the flag AND walks all currently-rendered markers to toggle their Konva draggable() accordingly.
In addCommentMarker, create the marker with draggable: !_readonly.
In app.js, call WhiteboardComments.setReadonly(readonlyMode) BEFORE the loadComments call so newly-created markers respect the flag, and any pre-existing ones from a re-render are also flipped.

No need to change makeAllReadonly (it has its own concern: real objects).

Files to Modify

crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js
crates/hero_whiteboard_admin/static/web/js/whiteboard/app.js

Implementation Plan

Step 1: Add `_readonly` flag + `setReadonly` in comments.js

At the top of the module (after the existing var _escHandler), add:

var _readonly = false;

In the IIFE return at the bottom, expose:

setReadonly: setReadonly,

And define the function near init:

function setReadonly(value) {
    _readonly = !!value;
    Object.keys(_comments).forEach(function (id) {
        var entry = _comments[id];
        if (entry && entry.marker) entry.marker.draggable(!_readonly);
    });
}

Dependencies: none.

Step 2: Honor `_readonly` in `addCommentMarker`

File: comments.js, change line ~248 from:

draggable: true,

to:

draggable: !_readonly,

Dependencies: Step 1.

Step 3: Wire from app.js

File: app.js, before line 264 (await WhiteboardComments.loadComments(boardId);), add:

WhiteboardComments.setReadonly(readonlyMode);

This handles both the readonly and editable initial path. For refreshFromServer polling in readonly mode (line 154-157), it does not re-create markers via loadComments — it goes through applySyncUpdate for objects only. Comments aren't included in that path, so a single setReadonly call at init suffices for the readonly viewer lifecycle.

Dependencies: Step 1 + 2.

Acceptance Criteria

/board/{id}/view shows comment markers; clicking does nothing visible (popover container does not exist — pre-existing behavior); dragging a marker does NOT move it and does NOT fire comment.update.
/s/{token} shared link: same as above.
Edit-mode (/board/{id}) drag still works and still pushes history (#179).
No regression to comment create / resolve / delete in edit mode.

Notes

setReadonly is idempotent and safe to call multiple times.
This does not touch WhiteboardObjects.makeAllReadonly() — it's its own scope.
Server-side authz gap (devtools-callable comment.* RPCs from readonly clients) is explicitly out of scope per the issue text.

## Implementation Spec for Issue #181 ### Objective In readonly mode (`/board/{id}/view` and `/s/{token}`), comment markers must render but be undraggable. ### Approach Smallest, contained change to `comments.js` + a one-liner from `app.js`: 1. Add a module-level `_readonly` flag in `comments.js`. 2. Export `setReadonly(value)` that flips the flag AND walks all currently-rendered markers to toggle their Konva `draggable()` accordingly. 3. In `addCommentMarker`, create the marker with `draggable: !_readonly`. 4. In `app.js`, call `WhiteboardComments.setReadonly(readonlyMode)` BEFORE the `loadComments` call so newly-created markers respect the flag, and any pre-existing ones from a re-render are also flipped. No need to change `makeAllReadonly` (it has its own concern: real objects). ### Files to Modify - `crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js` - `crates/hero_whiteboard_admin/static/web/js/whiteboard/app.js` ### Implementation Plan #### Step 1: Add `_readonly` flag + `setReadonly` in comments.js At the top of the module (after the existing `var _escHandler`), add: ```js var _readonly = false; ``` In the IIFE return at the bottom, expose: ```js setReadonly: setReadonly, ``` And define the function near `init`: ```js function setReadonly(value) { _readonly = !!value; Object.keys(_comments).forEach(function (id) { var entry = _comments[id]; if (entry && entry.marker) entry.marker.draggable(!_readonly); }); } ``` Dependencies: none. #### Step 2: Honor `_readonly` in `addCommentMarker` File: `comments.js`, change line ~248 from: ```js draggable: true, ``` to: ```js draggable: !_readonly, ``` Dependencies: Step 1. #### Step 3: Wire from app.js File: `app.js`, before line 264 (`await WhiteboardComments.loadComments(boardId);`), add: ```js WhiteboardComments.setReadonly(readonlyMode); ``` This handles both the readonly and editable initial path. For `refreshFromServer` polling in readonly mode (line 154-157), it does not re-create markers via `loadComments` — it goes through `applySyncUpdate` for objects only. Comments aren't included in that path, so a single `setReadonly` call at init suffices for the readonly viewer lifecycle. Dependencies: Step 1 + 2. ### Acceptance Criteria - [ ] `/board/{id}/view` shows comment markers; clicking does nothing visible (popover container does not exist — pre-existing behavior); dragging a marker does NOT move it and does NOT fire `comment.update`. - [ ] `/s/{token}` shared link: same as above. - [ ] Edit-mode (`/board/{id}`) drag still works and still pushes history (#179). - [ ] No regression to comment create / resolve / delete in edit mode. ### Notes - `setReadonly` is idempotent and safe to call multiple times. - This does not touch `WhiteboardObjects.makeAllReadonly()` — it's its own scope. - Server-side authz gap (devtools-callable `comment.*` RPCs from readonly clients) is explicitly out of scope per the issue text.

AhmedHanafy725 commented

2026-05-13 08:47:23 +00:00

Author

Member

Test Results + Final Summary

Changes

static/web/js/whiteboard/comments.js:
- Added module-level _readonly flag.
- Added setReadonly(value) — sets the flag and walks all existing markers, toggling .draggable(!_readonly). Exported in the IIFE return.
- addCommentMarker now creates markers with draggable: !_readonly.
static/web/js/whiteboard/app.js:
- Calls WhiteboardComments.setReadonly(readonlyMode) immediately before loadComments. Handles both first-load and any future re-render path.

Behaviour after fix

/board/{id}/view and /s/{token}: markers visible; cursor is the pan-hand from setupReadonlyPan; dragging a marker does nothing and no comment.update RPC fires.
Edit mode /board/{id}: drag still works, history is still pushed (#179), broadcasts still go out.

Gates

node -c comments.js / node -c app.js — JS syntax OK
cargo fmt --check — pass
cargo clippy --workspace --all-targets -- -D warnings — pass
cargo test --workspace --lib — 0 tests, 0 failures

Acceptance Criteria

Markers stay visible in readonly mode.
Dragging them in readonly does NOT fire comment.update.
Edit mode drag works and pushes history.
No regression to create/resolve/delete in edit mode.

Out of scope (per the issue)

Server-side auth on comment.* for share-link clients. A determined readonly viewer can still call WhiteboardComments.deleteComment(id) from devtools. File a backend issue if you want that closed.

Manual verification still required

Rebuild + restart hero_whiteboard_admin, hard-reload /board/{id} (edit) and /board/{id}/view (readonly), attempt to drag a comment marker in each.

## Test Results + Final Summary ### Changes - `static/web/js/whiteboard/comments.js`: - Added module-level `_readonly` flag. - Added `setReadonly(value)` — sets the flag and walks all existing markers, toggling `.draggable(!_readonly)`. Exported in the IIFE return. - `addCommentMarker` now creates markers with `draggable: !_readonly`. - `static/web/js/whiteboard/app.js`: - Calls `WhiteboardComments.setReadonly(readonlyMode)` immediately before `loadComments`. Handles both first-load and any future re-render path. ### Behaviour after fix - `/board/{id}/view` and `/s/{token}`: markers visible; cursor is the pan-hand from `setupReadonlyPan`; dragging a marker does nothing and no `comment.update` RPC fires. - Edit mode `/board/{id}`: drag still works, history is still pushed (#179), broadcasts still go out. ### Gates - `node -c comments.js` / `node -c app.js` — JS syntax OK - `cargo fmt --check` — pass - `cargo clippy --workspace --all-targets -- -D warnings` — pass - `cargo test --workspace --lib` — 0 tests, 0 failures ### Acceptance Criteria - [x] Markers stay visible in readonly mode. - [x] Dragging them in readonly does NOT fire `comment.update`. - [x] Edit mode drag works and pushes history. - [x] No regression to create/resolve/delete in edit mode. ### Out of scope (per the issue) Server-side auth on `comment.*` for share-link clients. A determined readonly viewer can still call `WhiteboardComments.deleteComment(id)` from devtools. File a backend issue if you want that closed. ### Manual verification still required Rebuild + restart `hero_whiteboard_admin`, hard-reload `/board/{id}` (edit) and `/board/{id}/view` (readonly), attempt to drag a comment marker in each.