ENG-2037 Add tabbed node card context menu to Roam tldraw - #1272
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| const toggleCanvasPresence = async ({ | ||
| relatedUid, | ||
| text, | ||
| }: { | ||
| relatedUid: string; | ||
| text: string; | ||
| }) => { | ||
| setPendingUids((prev) => [...prev, relatedUid]); | ||
| try { | ||
| const existing = nodeShapesByUid.get(relatedUid); | ||
| if (existing) { | ||
| removeFromCanvas(existing); | ||
| } else { | ||
| await addToCanvas({ relatedUid, text }); | ||
| } | ||
| } finally { | ||
| setPendingUids((prev) => prev.filter((u) => u !== relatedUid)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing error handling in toggleCanvasPresence async function. If addToCanvas throws an error (e.g., network failure, missing data), it will be silently swallowed because:
- The try-finally block has no catch clause
- The onClick handler on line 185 uses
voidoperator, discarding the rejected promise
This will leave users confused when add/remove operations fail without feedback.
Fix: Add a catch block to handle errors:
const toggleCanvasPresence = async ({
relatedUid,
text,
}: {
relatedUid: string;
text: string;
}) => {
setPendingUids((prev) => [...prev, relatedUid]);
try {
const existing = nodeShapesByUid.get(relatedUid);
if (existing) {
removeFromCanvas(existing);
} else {
await addToCanvas({ relatedUid, text });
}
} catch (error) {
dispatchToastEvent({
id: "dg-context-tab-toggle-error",
title: "Failed to update canvas",
severity: "error",
});
console.error("Error toggling canvas presence:", error);
} finally {
setPendingUids((prev) => prev.filter((u) => u !== relatedUid));
}
};| const toggleCanvasPresence = async ({ | |
| relatedUid, | |
| text, | |
| }: { | |
| relatedUid: string; | |
| text: string; | |
| }) => { | |
| setPendingUids((prev) => [...prev, relatedUid]); | |
| try { | |
| const existing = nodeShapesByUid.get(relatedUid); | |
| if (existing) { | |
| removeFromCanvas(existing); | |
| } else { | |
| await addToCanvas({ relatedUid, text }); | |
| } | |
| } finally { | |
| setPendingUids((prev) => prev.filter((u) => u !== relatedUid)); | |
| } | |
| }; | |
| const toggleCanvasPresence = async ({ | |
| relatedUid, | |
| text, | |
| }: { | |
| relatedUid: string; | |
| text: string; | |
| }) => { | |
| setPendingUids((prev) => [...prev, relatedUid]); | |
| try { | |
| const existing = nodeShapesByUid.get(relatedUid); | |
| if (existing) { | |
| removeFromCanvas(existing); | |
| } else { | |
| await addToCanvas({ relatedUid, text }); | |
| } | |
| } catch (error) { | |
| dispatchToastEvent({ | |
| id: "dg-context-tab-toggle-error", | |
| title: "Failed to update canvas", | |
| severity: "error", | |
| }); | |
| console.error("Error toggling canvas presence:", error); | |
| } finally { | |
| setPendingUids((prev) => prev.filter((u) => u !== relatedUid)); | |
| } | |
| }; | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| x: shape.x + shape.props.w + NEW_NODE_OFFSET_PX, | ||
| y: shape.y, |
There was a problem hiding this comment.
🟡 Related nodes added from the context list all land in the same spot, hiding each other
Every node added from the relation list is placed at one fixed position next to the selected card (x: shape.x + shape.props.w + NEW_NODE_OFFSET_PX at apps/roam/src/components/canvas/CustomStylePanel.tsx:103-104), so adding a second related node drops it exactly on top of the first one.
Impact: Users who add several related nodes see a single stack of overlapping cards and must drag them apart manually.
Fixed placement offset with no collision or fan-out logic
addToCanvas computes the new shape's position purely from the anchor shape (apps/roam/src/components/canvas/CustomStylePanel.tsx:98-117). It does not consider how many nodes were already added from the panel, nor whether other shapes already occupy that point, so N adds produce N shapes with identical x/y. A simple fix is to offset by the number of already placed siblings (e.g. stagger y by index or search for a free spot before creating).
Prompt for agents
In apps/roam/src/components/canvas/CustomStylePanel.tsx, addToCanvas always positions the newly created discourse node shape at shape.x + shape.props.w + NEW_NODE_OFFSET_PX, shape.y. When a user adds multiple related nodes from the Context tab, they are all created at that same point and visually stack on top of one another. Consider computing a placement that avoids collisions, e.g. stagger vertically based on how many shapes already occupy the target area (editor.getCurrentPageShapes / editor.getShapesAtPoint) or track the number of nodes added from this panel and offset accordingly.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| const NEW_NODE_OFFSET_PX = 80; | ||
|
|
||
| const ContextTabContent = ({ shape }: { shape: DiscourseNodeShape }) => { |
There was a problem hiding this comment.
🟡 New panel code omits the explicit function return types the repo style guide requires
The newly added panel components and helpers are declared without explicit return types (for example const ContextTabContent = ({ shape }: { shape: DiscourseNodeShape }) => { at apps/roam/src/components/canvas/CustomStylePanel.tsx:28), which conflicts with the repository TypeScript rules.
Impact: The code diverges from the project's documented style, making future refactors harder to review.
Rule source and affected declarations
AGENTS.md and STYLE_GUIDE.md both state "Use explicit return types for functions". Affected declarations: ContextTabContent (apps/roam/src/components/canvas/CustomStylePanel.tsx:28), removeFromCanvas (:66), addToCanvas (:74), toggleCanvasPresence (:126), NodeCardPanelContent (:198), and CustomStylePanel (:226). Existing code in the same area follows the rule, e.g. SyncModeMenuSwitchItem returns ReactElement in apps/roam/src/components/canvas/uiOverrides.tsx:85-96.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Overrides tldraw's StylePanel for Roam: selecting a single node card shows a Context tab (default) listing all discourse relations grouped by relation type, with +/− buttons that add or remove the related node on the canvas, and a Styling tab that reuses the stock styling controls. Regular tldraw shapes and multi-selection keep the default style panel. Adding a node also draws its existing relation arrows via createExistingRelations; removing deletes the node shape and its arrows — relation data in Roam is never modified.