Hermes/refboard-ayon
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(annotations): address code review findings (1-8, 10)

Browse Source 
1. toggleVote wrapped in transaction (race condition fix)
2. FeedbackPanel fetch calls now surface errors via onError/toast
3. Extracted resolveBoard/hasCollectionRole to shared board-access.js
4. AnnotationStore uses monotonic version counter for snapshots
5. PinOverlay uses object pool instead of destroy/recreate on refresh
6. canvasObjects prop memoized with useMemo
7. PinOverlay store subscription cleaned up on unmount
8. Comment content capped at 5000 chars (backend validation)
10. anchor_type validated to 'object' or 'point'
This commit is contained in:
Hiren Kangad
2026-03-10 21:16:08 +05:30
parent 303124f518
commit f7a39a1726
9 changed files with 176 additions and 133 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/db.js
+5 -1
View File
@@ -534,7 +534,7 @@ function getVotesByBoard(boardId) {
return db.prepare('SELECT * FROM object_votes WHERE board_id = ?').all(boardId);
}
function toggleVote(boardId, objectId, userId) {
const _toggleVoteTx = db.transaction((boardId, objectId, userId) => {
const existing = db.prepare(
'SELECT 1 FROM object_votes WHERE board_id = ? AND object_id = ? AND user_id = ?'
).get(boardId, objectId, userId);
@@ -548,6 +548,10 @@ function toggleVote(boardId, objectId, userId) {
.run(boardId, objectId, userId);
return true; // vote added
}
});
function toggleVote(boardId, objectId, userId) {
return _toggleVoteTx(boardId, objectId, userId);
}
// ---------------------
backend/routes/board-access.js
+27
View File
@@ -0,0 +1,27 @@
const { getBoard, getCollection, getCollectionMember } = require('../db');
function hasCollectionRole(member, minRole) {
if (!member) return false;
const hierarchy = { owner: 3, editor: 2, viewer: 1 };
return (hierarchy[member.role] || 0) >= (hierarchy[minRole] || 0);
}
function resolveBoard(req, res, minRole = 'viewer') {
const board = getBoard(req.params.boardId);
if (!board) { res.status(404).json({ error: 'Board not found' }); return null; }
const collection = getCollection(board.collection_id);
if (!collection) { res.status(404).json({ error: 'Collection not found' }); return null; }
const member = getCollectionMember(board.collection_id, req.user.id);
if (minRole === 'viewer' && collection.is_public) {
return { board, collection, member: member || { role: 'viewer' } };
}
if (!hasCollectionRole(member, minRole)) {
res.status(403).json({ error: `${minRole} access required` });
return null;
}
return { board, collection, member };
}
module.exports = { hasCollectionRole, resolveBoard };
backend/routes/threads.js
+14 -26
View File
@@ -2,9 +2,6 @@ const { Router } = require('express');
const { v4: uuidv4 } = require('uuid');
const { authMiddleware } = require('../auth');
const {
getBoard,
getCollection,
getCollectionMember,
getThreadsByBoard,
getThread,
createThread,
@@ -18,34 +15,13 @@ const {
incrementThreadCommentCount,
decrementThreadCommentCount,
} = require('../db');
const { hasCollectionRole, resolveBoard } = require('./board-access');
const router = Router();
router.use(authMiddleware);
function hasCollectionRole(member, minRole) {
if (!member) return false;
const hierarchy = { owner: 3, editor: 2, viewer: 1 };
return (hierarchy[member.role] || 0) >= (hierarchy[minRole] || 0);
}
function resolveBoard(req, res, minRole = 'viewer') {
const board = getBoard(req.params.boardId);
if (!board) { res.status(404).json({ error: 'Board not found' }); return null; }
const collection = getCollection(board.collection_id);
if (!collection) { res.status(404).json({ error: 'Collection not found' }); return null; }
const member = getCollectionMember(board.collection_id, req.user.id);
if (minRole === 'viewer' && collection.is_public) {
return { board, collection, member: member || { role: 'viewer' } };
}
if (!hasCollectionRole(member, minRole)) {
res.status(403).json({ error: `${minRole} access required` });
return null;
}
return { board, collection, member };
}
const MAX_COMMENT_LENGTH = 5000;
// GET /api/boards/:boardId/threads — all threads + comments for board
router.get('/:boardId/threads', (req, res) => {
@@ -85,6 +61,12 @@ router.post('/:boardId/threads', (req, res) => {
if (!object_id || !content || !content.trim()) {
return res.status(400).json({ error: 'object_id and content are required' });
}
if (content.length > MAX_COMMENT_LENGTH) {
return res.status(400).json({ error: `Content too long (max ${MAX_COMMENT_LENGTH} chars)` });
}
if (anchor_type && !['object', 'point'].includes(anchor_type)) {
return res.status(400).json({ error: 'anchor_type must be "object" or "point"' });
}
const threadId = uuidv4();
const commentId = uuidv4();
@@ -200,6 +182,9 @@ router.post('/:boardId/threads/:threadId/comments', (req, res) => {
if (!content || !content.trim()) {
return res.status(400).json({ error: 'content is required' });
}
if (content.length > MAX_COMMENT_LENGTH) {
return res.status(400).json({ error: `Content too long (max ${MAX_COMMENT_LENGTH} chars)` });
}
const thread = getThread(req.params.threadId);
if (!thread || thread.board_id !== req.params.boardId) {
@@ -246,6 +231,9 @@ router.put('/:boardId/threads/:threadId/comments/:commentId', (req, res) => {
if (!content || !content.trim()) {
return res.status(400).json({ error: 'content is required' });
}
if (content.length > MAX_COMMENT_LENGTH) {
return res.status(400).json({ error: `Content too long (max ${MAX_COMMENT_LENGTH} chars)` });
}
const comment = getComment(req.params.commentId);
if (!comment || comment.thread_id !== req.params.threadId) {
backend/routes/votes.js
+2 -31
View File
@@ -1,41 +1,12 @@
const { Router } = require('express');
const { authMiddleware } = require('../auth');
const {
getBoard,
getCollection,
getCollectionMember,
getVotesByBoard,
toggleVote,
} = require('../db');
const { getVotesByBoard, toggleVote } = require('../db');
const { resolveBoard } = require('./board-access');
const router = Router();
router.use(authMiddleware);
function hasCollectionRole(member, minRole) {
if (!member) return false;
const hierarchy = { owner: 3, editor: 2, viewer: 1 };
return (hierarchy[member.role] || 0) >= (hierarchy[minRole] || 0);
}
function resolveBoard(req, res, minRole = 'viewer') {
const board = getBoard(req.params.boardId);
if (!board) { res.status(404).json({ error: 'Board not found' }); return null; }
const collection = getCollection(board.collection_id);
if (!collection) { res.status(404).json({ error: 'Collection not found' }); return null; }
const member = getCollectionMember(board.collection_id, req.user.id);
if (minRole === 'viewer' && collection.is_public) {
return { board, collection, member: member || { role: 'viewer' } };
}
if (!hasCollectionRole(member, minRole)) {
res.status(403).json({ error: `${minRole} access required` });
return null;
}
return { board, collection, member };
}
// GET /api/boards/:boardId/votes — all votes for board
router.get('/:boardId/votes', (req, res) => {
try {
frontend/src/canvas/PinOverlay.ts
+36 -25
View File
@@ -9,7 +9,8 @@ const PIN_COLOR_RESOLVED = 0x666666;
export class PinOverlay extends Container {
private _pins = new Map<string, Graphics>();
private _voteBadges: Graphics[] = [];
private _voteBadges = new Map<string, Graphics>();
private _pool: Graphics[] = [];
private _viewport: Viewport;
private _scene: SceneManager;
private _store: AnnotationStore;
@@ -21,16 +22,30 @@ export class PinOverlay extends Container {
this._store = store;
}
private _acquire(): Graphics {
const gfx = this._pool.pop() || new Graphics();
gfx.clear();
gfx.removeChildren();
gfx.visible = true;
gfx.eventMode = 'none';
gfx.cursor = 'default';
return gfx;
}
private _release(gfx: Graphics) {
gfx.visible = false;
this._pool.push(gfx);
}
/** Call on every viewport moved/zoomed event and on store change */
refresh(showResolved = false) {
const scale = 1 / this._viewport.scale.x; // scale-independent size
const scale = 1 / this._viewport.scale.x;
// Remove old pins + badges
for (const gfx of this._pins.values()) gfx.destroy();
for (const gfx of this._voteBadges) gfx.destroy();
// Return current pins and badges to pool
for (const gfx of this._pins.values()) this._release(gfx);
for (const gfx of this._voteBadges.values()) this._release(gfx);
this._pins.clear();
this._voteBadges = [];
this.removeChildren();
this._voteBadges.clear();
// ── Thread pins ──
for (const thread of this._store.threads.values()) {
@@ -46,12 +61,11 @@ export class PinOverlay extends Container {
wx = bounds.x + thread.pin_x * bounds.width;
wy = bounds.y + thread.pin_y * bounds.height;
} else {
// Object-level: top-right corner
wx = bounds.x + bounds.width;
wy = bounds.y;
}
const gfx = new Graphics();
const gfx = this._acquire();
const r = PIN_RADIUS * scale;
const color = thread.status === 'open' ? PIN_COLOR_OPEN : PIN_COLOR_RESOLVED;
@@ -59,27 +73,21 @@ export class PinOverlay extends Container {
gfx.fill({ color, alpha: 0.9 });
gfx.stroke({ color: 0xffffff, width: 1.5 * scale, alpha: 0.8 });
gfx.position.set(wx, wy);
gfx.eventMode = 'static';
gfx.cursor = 'pointer';
(gfx as any)._threadId = thread.id;
// Count label
if (thread.comment_count > 1) {
const label = new Text({
text: String(thread.comment_count),
style: new TextStyle({
fontSize: 9 * scale,
fill: '#ffffff',
fontWeight: 'bold',
}),
style: new TextStyle({ fontSize: 9 * scale, fill: '#ffffff', fontWeight: 'bold' }),
});
label.anchor.set(0.5);
gfx.addChild(label);
}
gfx.eventMode = 'static';
gfx.cursor = 'pointer';
(gfx as any)._threadId = thread.id;
if (!gfx.parent) this.addChild(gfx);
this._pins.set(thread.id, gfx);
this.addChild(gfx);
}
// ── Vote badges ──
@@ -92,7 +100,7 @@ export class PinOverlay extends Container {
const wx = bounds.x + bounds.width;
const wy = bounds.y + bounds.height;
const gfx = new Graphics();
const gfx = this._acquire();
const pw = 24 * scale;
const ph = 16 * scale;
const pr = 4 * scale;
@@ -106,8 +114,9 @@ export class PinOverlay extends Container {
});
label.anchor.set(0.5);
gfx.addChild(label);
this.addChild(gfx);
this._voteBadges.push(gfx);
if (!gfx.parent) this.addChild(gfx);
this._voteBadges.set(objectId, gfx);
}
}
@@ -125,9 +134,11 @@ export class PinOverlay extends Container {
override destroy(options?: any) {
for (const gfx of this._pins.values()) gfx.destroy();
for (const gfx of this._voteBadges) gfx.destroy();
for (const gfx of this._voteBadges.values()) gfx.destroy();
for (const gfx of this._pool) gfx.destroy();
this._pins.clear();
this._voteBadges = [];
this._voteBadges.clear();
this._pool = [];
super.destroy(options);
}
}
frontend/src/components/FeedbackPanel.tsx
+37 -11
View File
@@ -9,6 +9,7 @@ interface FeedbackPanelProps {
token: string;
canvasObjects: Map<string, { id: string; name?: string; type: string }>;
onJumpToObject?: (objectId: string) => void;
onError?: (msg: string) => void;
}
export default function FeedbackPanel({
@@ -19,6 +20,7 @@ export default function FeedbackPanel({
token,
canvasObjects,
onJumpToObject,
onError,
}: FeedbackPanelProps) {
const [collapsed, setCollapsed] = useState(false);
const [expandedThreadId, setExpandedThreadId] = useState<string | null>(null);
@@ -30,7 +32,7 @@ export default function FeedbackPanel({
// Subscribe to store changes
const _version = useSyncExternalStore(
(cb) => annotationStore.subscribe(cb),
() => annotationStore.threads.size + annotationStore.votes.size,
() => annotationStore.version,
);
const allThreads = Array.from(annotationStore.threads.values());
@@ -54,42 +56,65 @@ export default function FeedbackPanel({
// ── API helpers ──
const apiFetch = useCallback(async (url: string, init?: RequestInit) => {
try {
const res = await fetch(url, init);
if (!res.ok) {
const body = await res.json().catch(() => ({}));
throw new Error(body.error || `Request failed (${res.status})`);
}
return res;
} catch (err: any) {
onError?.(err.message || 'Request failed');
throw err;
}
}, [onError]);
const postReply = useCallback(async (threadId: string) => {
if (!replyText.trim()) return;
await fetch(`/api/boards/${boardId}/threads/${threadId}/comments`, {
try {
await apiFetch(`/api/boards/${boardId}/threads/${threadId}/comments`, {
method: 'POST',
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
body: JSON.stringify({ content: replyText.trim() }),
});
setReplyText('');
}, [boardId, token, replyText]);
} catch {}
}, [boardId, token, replyText, apiFetch]);
const resolveThread = useCallback(async (threadId: string, status: string) => {
await fetch(`/api/boards/${boardId}/threads/${threadId}`, {
try {
await apiFetch(`/api/boards/${boardId}/threads/${threadId}`, {
method: 'PATCH',
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
body: JSON.stringify({ status }),
});
}, [boardId, token]);
} catch {}
}, [boardId, token, apiFetch]);
const deleteComment = useCallback(async (threadId: string, commentId: string) => {
await fetch(`/api/boards/${boardId}/threads/${threadId}/comments/${commentId}`, {
try {
await apiFetch(`/api/boards/${boardId}/threads/${threadId}/comments/${commentId}`, {
method: 'DELETE',
headers: { Authorization: `Bearer ${token}` },
});
}, [boardId, token]);
} catch {}
}, [boardId, token, apiFetch]);
const toggleVote = useCallback(async (objectId: string) => {
await fetch(`/api/boards/${boardId}/votes`, {
try {
await apiFetch(`/api/boards/${boardId}/votes`, {
method: 'POST',
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
body: JSON.stringify({ object_id: objectId }),
});
}, [boardId, token]);
} catch {}
}, [boardId, token, apiFetch]);
const createThread = useCallback(async () => {
if (!newCommentText.trim() || !selectedObjectId) return;
await fetch(`/api/boards/${boardId}/threads`, {
try {
await apiFetch(`/api/boards/${boardId}/threads`, {
method: 'POST',
headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
body: JSON.stringify({
@@ -99,7 +124,8 @@ export default function FeedbackPanel({
}),
});
setNewCommentText('');
}, [boardId, token, newCommentText, selectedObjectId]);
} catch {}
}, [boardId, token, newCommentText, selectedObjectId, apiFetch]);
// ── Collapsed state ──
frontend/src/hooks/useCanvasSetup.ts
+10 -4
View File
@@ -355,13 +355,18 @@ export function useCanvasSetup(deps: CanvasSetupDeps) {
pinOverlayRef.current = pinOverlay;
// Refresh overlay on viewport move
viewport.on('moved', () => {
const onViewportMoved = () => {
if (pinOverlay.visible) pinOverlay.refresh();
});
// Refresh on store change
annotationStoreRef.current!.subscribe(() => {
};
viewport.on('moved', onViewportMoved);
// Refresh on store change (save unsub for cleanup)
const unsubPinOverlay = annotationStoreRef.current!.subscribe(() => {
if (pinOverlay.visible) pinOverlay.refresh();
});
(pinOverlay as any)._cleanup = () => {
viewport.off('moved', onViewportMoved);
unsubPinOverlay();
};
}
// Setup drag/drop and paste
@@ -394,6 +399,7 @@ export function useCanvasSetup(deps: CanvasSetupDeps) {
dropCleanupRef.current?.();
pasteCleanupRef.current?.();
if (pinOverlayRef.current) {
(pinOverlayRef.current as any)._cleanup?.();
pinOverlayRef.current.destroy();
pinOverlayRef.current = null;
}
frontend/src/pages/Editor.tsx
+15 -10
View File
@@ -162,6 +162,19 @@ export default function Editor({ isPublicView }: EditorProps) {
uploadManager, onCanvasChange, showToast, setOnlineUsers, setSelectedLayerIds,
});
// Build canvas objects map for FeedbackPanel (memoized on object count changes)
const canvasObjectMap = React.useMemo(() => {
const scene = canvasRef.current?.getScene();
const map = new Map<string, { id: string; name?: string; type: string }>();
if (scene) {
for (const item of scene.items.values()) {
map.set(item.id, { id: item.id, name: item.data.name, type: item.data.type });
}
}
return map;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [objectCount]);
// Toggle pin overlay visibility with review mode
useEffect(() => {
if (pinOverlay) {
@@ -663,16 +676,8 @@ export default function Editor({ isPublicView }: EditorProps) {
userId={user.id}
boardId={resolvedBoardId}
token={localStorage.getItem('token') || ''}
canvasObjects={(() => {
const scene = canvasRef.current?.getScene();
const map = new Map<string, { id: string; name?: string; type: string }>();
if (scene) {
for (const item of scene.items.values()) {
map.set(item.id, { id: item.id, name: item.data.name, type: item.data.type });
}
}
return map;
})()}
canvasObjects={canvasObjectMap}
onError={(msg) => showToast(msg)}
onJumpToObject={(objectId) => {
const scene = canvasRef.current?.getScene();
const vp = canvasRef.current?.getViewport();
frontend/src/stores/annotationStore.ts
+5
View File
@@ -42,6 +42,10 @@ export class AnnotationStore {
votes = new Map<string, Set<string>>();
private _listeners = new Set<Listener>();
private _version = 0;
/** Monotonic version counter for useSyncExternalStore snapshots */
get version(): number { return this._version; }
subscribe(fn: Listener): () => void {
this._listeners.add(fn);
@@ -49,6 +53,7 @@ export class AnnotationStore {
}
private _notify() {
this._version++;
for (const fn of this._listeners) fn();
}