[trangdoan982]

Reviewer brief

Stacked on #1553 (ENG-2243), which must merge first.

• Result: Create new in the canvas relation menu opens an "Add relation type" dialog. Submitting a valid label and complement saves the triplet for the dragged node-type pair through ENG-2243's createTripletForPair, shows both toasts, and lists the new type without a refresh.
• Review focus: validateNewRelationType in apps/roam/src/components/canvas/existingRelationTypes.ts. The canvas groups relation types by label, and queries match a name against both label and complement. So the dialog blocks any new label or complement that reuses another type's text, ignoring case. It also rejects built-in query condition names (such as is a), names inherited from Object (such as constructor), and identical label and complement on a same-type pair. Each error says what clashes, and points to Add existing only when that picker can list the type.
• Review focus: getRelationColor in apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx. Arrows paint the color name as a raw CSS color, and the old fallback was white, so every custom relation drew invisibly on light canvases. Named colors (Supports, Opposes, Informs and their complements) now match ignoring case. Every other relation drawn without a palette index is grey, which reads on light and dark canvases. The positional palette is unchanged, and positions past its end now wrap instead of returning undefined. This is shared: the toolbar relation tool, style panel, legacy arrow migration and tool panel all call it. Existing arrows keep their stored color.
• Review focus: apps/roam/src/components/canvas/overlays/RelationTypeDropdown.tsx. The dialog renders in a Blueprint portal outside the menu, so the menu's click-outside and Escape handlers stand down while it's open.
• Risk or follow-up:
  • When saving accepts an exact imported (provisional) triplet instead of writing a new one, "Relation type created" still shows.
  • Collision checks skip triplets whose source or destination is still ?, because those aren't usable anywhere until they're completed in Settings.
  • The dialog doesn't use roamjs-canvas-dialog: another extension's CSS in the test graph recolors that class's first footer button.

The diagram shows the submit path from the menu to the saved triplet.

flowchart LR
  A["RelationTypeDropdown: Create new"] --> B["CreateRelationTypeDialog: Label, Complement, Source → Target"]
  B -- Continue --> C{"validateNewRelationType: required, shown on pair, duplicate, label/complement reuse"}
  C -- error --> B
  C -- ok --> D["DragHandleOverlay.saveRelationTypeForPair(isNew)"]
  D --> E["createTripletForPair (ENG-2243)"]
  E --> F["Toasts: Relation type created, Discourse relation added"]
  F --> G["Menu lists the new type"]

Loading

Verification

Live run in Roam (slot 1 test graph) with pnpm --filter roam playwright:load-extension and a test module. Stored relations were off in the graph, so the module turned them on with "Reactivate without Migration" and turned them off again in cleanup. Cleanup removed all test pages, triplets and stored relations, and a fresh load confirmed nothing remained.

Scenario	Input	Expected	Actual	Pass
Dialog opens on an empty pair	Drag Question → Hypothesis, click Create new	Dialog shows "Question → Hypothesis"	Shown	✅
Typing doesn't trigger canvas shortcuts	Type "draw rect text arrow vhex" in Label	Text in field, tool stays select, no new shapes	As expected	✅
Clicks in the dialog keep the menu	Click inside the dialog body	Menu stays open	Stayed open	✅
Validation	Empty, case-variant duplicate, label with another complement, label or complement reusing another type's text	Inline error, dialog stays open, nothing saved	All 6 errors shown, no writes	✅
Escape	Escape in the dialog	Dialog closes, menu stays	As expected	✅
DES-349 Case 2 end to end	New label and complement on Question → Hypothesis, Enter	Both toasts, type listed without refresh, drawing it saves one stored relation	As expected	✅
Pair that already has types	+ → Create new on Conclusion → Consequence	Existing label rejected, new type created and listed beside it	As expected	✅

A second run at aa536a7 in Chrome on discourse-dev-graph covered the checks added after review. Stored relations were turned on and off again the same way, and all test data was removed.

Scenario	Input	Expected	Actual	Pass
Built-in query condition	IS A / is an instance of	Rejected as a reserved query condition	"is a" is a reserved query condition.	✅
Name inherited from Object	constructor	Rejected as a reserved name	"constructor" is a reserved name.	✅
Case-variant duplicate of an unaccepted imported type	TESTS / Is Tested By	Duplicate error without the Add existing hint	As expected	✅
Label is another type's complement	is tested by (unaccepted), Is Opposed By (accepted)	Error; Add existing hint only for the accepted type	As expected	✅
Placeholder text	?	"Label and complement are required."	As expected	✅
Same text on a same-type pair	eng2244t mirrors / Eng2244t Mirrors on Conclusion → Conclusion	Rejected	As expected	✅
Escape	Escape in the dialog	Dialog closes, menu stays	As expected	✅
Create and draw	eng2244t informs live / eng2244t informed live on Question → Hypothesis	Trimmed type saved, both toasts, listed without refresh, drawing it saves one stored relation	As expected	✅

A third Chrome run at 8998af2 checked relation colors.

Scenario	Input	Expected	Actual	Pass
New custom type	Create new eng2244t colors, then draw it	Grey menu dot and arrow, readable in light and dark mode	grey; label readable on both	✅
Add existing, named label in lowercase	Add existing supports / is supported by, then draw it	Green menu dot and arrow	green	✅
Tool panel	Open the discourse tool panel	Lowercase supports and opposes show named colors	As expected	✅

Scenario	Screenshot
Create new: both toasts, new type listed without refresh
Same label and complement on a same-type pair is rejected

Tests added in apps/roam/src/utils/__tests__/existingRelationTypes.test.ts (validation rules, case-insensitive matching, provisional handling) and apps/roam/src/utils/__tests__/relationTypeDropdown.test.ts (dialog submit, validation error, failed save, click-outside and Escape while open). Rerun:

pnpm -C apps/roam test:unit

Not verified:

• 06898ef restores the positional palette (8998af2 had removed white from it, which shifted every toolbar and tool-panel color). Arrows drawn by the drag-handle menu don't use the palette, so the color rows above still hold. Unit tests cover the restored palette.
• 05ea020 live. It changes which names count as reserved to built-in conditions only, so a deleted relation's name is no longer blocked. That's covered by unit tests; the Chrome run above was at aa536a7.
• Hiding the close button while saving. The save finishes too quickly to observe; the change is a Blueprint isCloseButtonShown prop.
• Accepting an exact imported triplet on its own pair. The test graph has no imported triplet for these node types, so this path is covered by unit tests only.

Loom video

pending

Scope check

• Ran $scope-check against ENG-2244 and the final diff.
• Scope beyond Done When: validation of the new label and complement (required, placeholder, shown on the pair, duplicates, label or complement reuse, built-in query condition and Object names, and identical names on a same-type pair, all case-insensitive, with imported-triplet handling). The fallback color in the shared getRelationColor changes from white to grey, so custom relations drawn by the toolbar tool, style panel and legacy migration are also grey. Named colors match ignoring case. To tell built-in names from relation names, apps/roam/src/utils/conditionToDatalog.ts now records built-in condition keys, and apps/roam/src/utils/registerDiscourseDatalogTranslators.ts flags its fixed conditions with builtIn: true.
• Required now: Create new is the first canvas path that writes a new label. The canvas resolves relation types by label and queries match label and complement, so an unchecked clash can save a relation under the wrong triplet, hide a type from the menu, or replace a built-in query condition. Done When also requires an automatic color, and new and added types were drawn white, which is invisible on a light canvas.
• Anyone affected or consulted: Yes. The rules were set during ENG-2244 planning and review with the author.
• Decision: Not documented. Record it in a Linear comment on ENG-2244 before merge.

Standards check

• Ran $dg-pr-adherence-check against the final diff and PR metadata.

Local delegated full review

• Ran a comprehensive review of the entire final diff in a subagent with a fresh context. Use $dg-delegated-full-review when no other full-review workflow is available.

🤖 Generated with Claude Code

[linear-code]

ENG-2244

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
discourse-graph	Skipped		Oct 17, 2026 6:20pm UTC

[devin-ai-integration]

Devin Review found 4 potential issues.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

[devin-ai-integration]

🟡 Imported pair hides an accepted duplicate

When an exact provisional pair exists, isAcceptedOnSave suppresses every exact-name duplicate, including accepted types on other pairs. Saving accepts the provisional type instead of directing the user to Add existing.

Learn more

The validator permits one exact provisional schema on the selected pair because createTripletForPair accepts that schema on save. But isAcceptedOnSave is a boolean, and the duplicate predicate skips every schema with that text, including accepted schemas on unrelated pairs. The save then accepts a redundant imported schema even though the existing picker offers the accepted type.

Example: An imported provisional supports / is supported by exists on Evidence → Claim, and an accepted identical type exists on Question → Hypothesis. Creating the type on Evidence → Claim passes validation and accepts the imported schema, leaving two accepted versions of the same type.

Recommended fix: Exempt only the exact provisional schema on the selected pair from duplicate detection. Keep accepted matches, and other provisional matches, eligible for the duplicate error.

---

Was this helpful? React with 👍 or 👎 to provide feedback.