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.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3475f5dcb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| x: shape.x + shape.props.w + NEW_NODE_OFFSET_PX, | ||
| y: shape.y, |
There was a problem hiding this comment.
When the selected node is inside a frame/group or another transformed parent, shape.x/shape.y are local to that parent, but this creates the related node on the current page because no parentId is supplied. In that case the plus button places the new card at the wrong page coordinates instead of next to the selected card; convert the selected shape's bounds to page space or create the new shape in the same parent space.
Useful? React with 👍 / 👎.
1 participant
eng-2037.mp4
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.