[ENG-1036] Fix the relation color not picking by forcing colors to strictly match Tldraw palette #557
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR introduces a centralized color management system for relation types, replacing hardcoded hex colors with semantic color names. It adds a ColorPicker UI component, defines Tldraw color constants and types, updates the DiscourseRelationType schema, and propagates color values through arrow creation and canvas rendering with fallback defaults. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant RelationshipTypeSettings
participant ColorPicker
participant Canvas
participant relationUtils
User->>RelationshipTypeSettings: Edit relation type
RelationshipTypeSettings->>ColorPicker: Render with current color
User->>ColorPicker: Select color from palette
ColorPicker->>RelationshipTypeSettings: onChange(TldrawColorName)
RelationshipTypeSettings->>RelationshipTypeSettings: Update state (color as TldrawColorName)
RelationshipTypeSettings->>Canvas: Save relation type config
Canvas->>DiscourseRelationTool: Create arrow for relation
DiscourseRelationTool->>Canvas: Set shape color (relationType?.color ?? DEFAULT_TLDRAW_COLOR)
Canvas->>relationUtils: Render arrow shape
relationUtils->>relationUtils: Validate color against theme
relationUtils->>relationUtils: Resolve colorHex (theme lookup or DEFAULT_TLDRAW_COLOR)
relationUtils->>Canvas: Render with resolved colorHex (stroke & fill)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/obsidian/src/components/canvas/utils/relationUtils.tsx (1)
1889-1892: Consider including color in changeIndex memo dependencies.The
changeIndexmemo currently depends only onshape, which should capture color changes since color is a shape property. However, consider explicitly including color-related values in the dependency array for clarity and to ensure Safari-specific re-rendering works correctly when colors change.Consider this for explicitness:
const changeIndex = React.useMemo<number>(() => { return editor.environment.isSafari ? (globalRenderIndex += 1) : 0; - }, [shape]); + }, [shape, colorName]);Though the current implementation should work since
shapechanges when its color changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/obsidian/src/components/RelationshipTypeSettings.tsx(6 hunks)apps/obsidian/src/components/canvas/DiscourseRelationTool.ts(2 hunks)apps/obsidian/src/components/canvas/overlays/RelationPanel.tsx(2 hunks)apps/obsidian/src/components/canvas/utils/relationUtils.tsx(5 hunks)apps/obsidian/src/constants.ts(1 hunks)apps/obsidian/src/types.ts(2 hunks)apps/obsidian/src/utils/tldrawColors.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-17T23:42:29.279Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:95-104
Timestamp: 2025-06-17T23:42:29.279Z
Learning: In the DiscourseGraphs/discourse-graph codebase, DiscourseRelation type properties are either string or Triple[], and the STANDARD_ROLES filter in conceptConversion.ts excludes Triple[] properties, so only string values remain after filtering.
Applied to files:
apps/obsidian/src/types.tsapps/obsidian/src/components/canvas/DiscourseRelationTool.tsapps/obsidian/src/components/canvas/overlays/RelationPanel.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/obsidian/src/types.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/obsidian/src/types.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/obsidian/src/types.ts
🔇 Additional comments (11)
apps/obsidian/src/components/canvas/DiscourseRelationTool.ts (1)
254-254: LGTM! Clean fallback pattern for relation color.The color assignment correctly uses the relation type's color when available, falling back to
DEFAULT_TLDRAW_COLORotherwise. This ensures every relation shape has a valid color from the Tldraw palette.apps/obsidian/src/components/canvas/overlays/RelationPanel.tsx (1)
381-381: LGTM! Consistent color fallback pattern.The color assignment mirrors the approach in DiscourseRelationTool.ts, ensuring consistency across different relation creation paths.
apps/obsidian/src/utils/tldrawColors.ts (2)
1-60: Well-structured color management system.The centralized color utilities provide:
- Type-safe color name constraints via
TldrawColorName- Human-readable labels for UI display
- A default fallback color
- Hex value mappings for rendering
This design enables consistent color handling across the application.
46-60: No issues found – the COLOR_PALETTE hex values appear correct.The palette in
apps/obsidian/src/utils/tldrawColors.tsis identical to the verified COLOR_PALETTE inapps/roam/src/components/canvas/DiscourseNodeUtil.tsx, which is explicitly sourced from@tldraw/editor/editor.css. Both files maintain consistent color definitions across the codebase, and the values are properly mapped to tldraw's standard color names.apps/obsidian/src/components/canvas/utils/relationUtils.tsx (1)
1882-1887: Good color validation with theme fallback.The color validation logic ensures that
shape.props.coloris a valid key in the theme. If invalid, it falls back toDEFAULT_TLDRAW_COLOR. This prevents runtime errors when rendering shapes with invalid color values.apps/obsidian/src/components/RelationshipTypeSettings.tsx (4)
21-109: Well-implemented ColorPicker component with accessibility considerations.The custom ColorPicker provides:
- Click-outside closing behavior via refs and event listeners
- Proper cleanup in useEffect return
- Contrast-aware text coloring for accessibility
- Visual color swatches for easy identification
- Integration with TldrawColorName type system
The implementation is clean and follows React best practices.
133-137: Good type-safe handling of color field updates.The conditional logic properly narrows the type for color updates, ensuring type safety when assigning
TldrawColorNamevalues. This prevents type errors while maintaining flexibility for other string fields.
193-198: Color validation added to save logic.The validation now checks that the
colorfield is populated, treating it as a required field alongsideid,label, andcomplement. This ensures data integrity.
14-14: No issues found—the utility is correctly implemented and imported.The
getContrastColorfunction exists atapps/obsidian/src/utils/colorUtils.tsand is properly exported. The import inRelationshipTypeSettings.tsxline 14 is valid and the function is actively used in the component for text color accessibility calculations. No action required.apps/obsidian/src/constants.ts (1)
31-44: Color mappings verified—changes are correct.All semantic color names map to the exact hex values that were previously hardcoded:
"green"→#099268✓"red"→#e03131✓"grey"→#adb5bd✓The refactoring from hex strings to semantic color names preserves visual appearance and aligns correctly with the Tldraw palette defined in
COLOR_PALETTE.apps/obsidian/src/types.ts (1)
2-2: Add type validation and document color compatibility.The type change from
stringtoTldrawColorNameimproves type safety, and all default values are already compatible semantic colors. However, the change lacks runtime validation when loading settings.Key findings:
- DEFAULT_RELATION_TYPES already use valid TldrawColorName values ("green", "red", "grey")
- Settings load with
Object.assignusing unsafeas TldrawColorNamecast (no validation)- RelationshipTypeSettings allows custom relation type creation, all constrained to semantic colors
- No migration logic or validation guards
The practical risk is low since:
- Default values are compatible
- UI constrains colors to semantic names only
- No evidence users created non-semantic color values
However, recommend adding a validation step in
loadSettings()to ensure color values match TldrawColorName and log/fall back to defaults for invalid values. This prevents silent failures if old data exists.
|
next action: waiting for review |
Summary by CodeRabbit
New Features
Improvements