[ENG-1547] Relation creation via drag handle (Obsidian)#909
[ENG-1547] Relation creation via drag handle (Obsidian)#909trangdoan982 wants to merge 7 commits intomainfrom
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
trangdoan982
force-pushed
the
eng-1547-implement-relation-creation-via-drag-handles-in-tldraw
branch
from
75c8668 to
0ebe9ce
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
trangdoan982
changed the title
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
-
CI is failing.
-
This PR is ~50% over the line change limit. I understand this might be required (or ideal) in this case, that being said, would you mind creating a Loom video walking through this code and code choices?
The biggest concern I have doing a quick review was the code possibly not being as DRY as possible. I worry we might be duplicating work, specifically in the relation/arrow drag handling logic and potential relations checking.
| onClick={(e) => e.stopPropagation()} | ||
| > | ||
| <div | ||
| style={{ |
| > | ||
| <div | ||
| style={{ | ||
| padding: "4px 8px", |
| key={rt.id} | ||
| onClick={() => handleSelect(rt.id)} | ||
| style={{ | ||
| display: "flex", |
| > | ||
| <span | ||
| style={{ | ||
| width: "8px", |
|
|
||
| for (const relationType of plugin.settings.relationTypes) { | ||
| // Check if there's a discourse relation that matches this pair | ||
| const isValid = plugin.settings.discourseRelations.some( |
There was a problem hiding this comment.
Are we doing this anywhere else in the plugin? Could we DRY this?
| }, [arrow, onDismiss]); | ||
|
|
||
| // Get valid relation types based on source/target node types | ||
| const validRelationTypes = useMemo(() => { |
There was a problem hiding this comment.
Are we doing this anywhere else in the plugin? Could we DRY this?
| const dy = point.y - currentShape.y; | ||
|
|
||
| // Check for a target shape under the cursor | ||
| const target = editor.getShapeAtPoint(point, { |
There was a problem hiding this comment.
Is this work being duplicated in the relationShape/relationUtil?
| export const getRelationDirection = ( | ||
| discourseRelations: DiscourseRelation[], | ||
| relationTypeId: string, | ||
| sourceNodeTypeId: string, | ||
| targetNodeTypeId: string, | ||
| ): { direct: boolean; reverse: boolean } => { |
There was a problem hiding this comment.
🔴 AGENTS.md violation: getRelationDirection has 4 positional params instead of using object destructuring
The AGENTS.md style guide mandates: "Use named parameters (object destructuring) when a function has more than 2 parameters." getRelationDirection takes 4 positional parameters (discourseRelations, relationTypeId, sourceNodeTypeId, targetNodeTypeId) without using object destructuring.
Prompt for agents
In apps/obsidian/src/components/canvas/utils/relationTypeUtils.ts, refactor getRelationDirection (lines 14-41) to use object destructuring for its parameters. Change the signature from:
export const getRelationDirection = (
discourseRelations: DiscourseRelation[],
relationTypeId: string,
sourceNodeTypeId: string,
targetNodeTypeId: string,
): { direct: boolean; reverse: boolean } => {
to:
export const getRelationDirection = ({
discourseRelations,
relationTypeId,
sourceNodeTypeId,
targetNodeTypeId,
}: {
discourseRelations: DiscourseRelation[];
relationTypeId: string;
sourceNodeTypeId: string;
targetNodeTypeId: string;
}): { direct: boolean; reverse: boolean } => {
Then update all call sites:
- apps/obsidian/src/components/canvas/utils/relationTypeUtils.ts lines 55-60 (getValidRelationTypesForNodePair)
- apps/obsidian/src/components/canvas/shapes/DiscourseRelationShape.tsx lines 1102-1107 (updateRelationTextForDirection)
- apps/obsidian/src/components/canvas/shapes/DiscourseRelationShape.tsx lines 1137-1142 (isValidNodeConnection)
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const getValidRelationTypesForNodePair = ( | ||
| settings: RelationTypeSettings, | ||
| sourceNodeTypeId: string, | ||
| targetNodeTypeId: string, | ||
| ): { id: string; label: string; color: string }[] => { |
There was a problem hiding this comment.
🔴 AGENTS.md violation: getValidRelationTypesForNodePair has 3 positional params instead of using object destructuring
The AGENTS.md style guide mandates: "Use named parameters (object destructuring) when a function has more than 2 parameters." getValidRelationTypesForNodePair takes 3 positional parameters (settings, sourceNodeTypeId, targetNodeTypeId) without using object destructuring.
Prompt for agents
In apps/obsidian/src/components/canvas/utils/relationTypeUtils.ts, refactor getValidRelationTypesForNodePair (lines 47-72) to use object destructuring. Change the signature from positional parameters to a single destructured object parameter. Then update the call site at apps/obsidian/src/components/canvas/overlays/RelationTypeDropdown.tsx lines 59-63.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const hasValidRelationTypeForNodePair = ( | ||
| settings: RelationTypeSettings, | ||
| sourceNodeTypeId: string, | ||
| targetNodeTypeId: string, |
There was a problem hiding this comment.
🔴 AGENTS.md violation: hasValidRelationTypeForNodePair has 3 positional params instead of using object destructuring
The AGENTS.md style guide mandates: "Use named parameters (object destructuring) when a function has more than 2 parameters." hasValidRelationTypeForNodePair takes 3 positional parameters (settings, sourceNodeTypeId, targetNodeTypeId) without using object destructuring.
Prompt for agents
In apps/obsidian/src/components/canvas/utils/relationTypeUtils.ts, refactor hasValidRelationTypeForNodePair (lines 77-90) to use object destructuring. Change the signature from positional parameters to a single destructured object parameter. Then update the call site at apps/obsidian/src/components/canvas/overlays/DragHandleOverlay.tsx lines 299-303.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
https://www.loom.com/share/ee0d603ff9db4025a1c8f857f2b01aec
Summary
relations.jsonin-place (preservingid,created,author, etc.)updateRelationType()helper torelationsStore.tsTest plan
relations.jsontypefield updated in-place🤖 Generated with Claude Code