[ENG-42] Create setting to define a relationship #99
Conversation
|
@trangdoan982 is attempting to deploy a commit to the Discourse Graphs Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces several new React components and utility functions to manage node types, relationships, and relationship types. A new context provider has been added to share the plugin instance, and the Settings component now uses a tabbed interface to render different settings views. Enhancements include unique ID generation, validation utilities, updated type definitions, and refined CSS for relationship nodes and visualizations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as Settings (Tabbed Interface)
participant P as PluginProvider
participant C as Child Component (Node/Relation Settings)
U->>S: Select a settings tab (e.g., "Node Types")
S->>P: Retrieve plugin context
S->>C: Render the corresponding settings component
C->>C: Process user input & perform validation
C->>P: Save changes via plugin settings
P-->>C: Acknowledge update
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (13)
apps/obsidian/src/utils/generateUid.ts (1)
1-5: Simple and effective unique ID generationThe function provides a good approach for generating unique identifiers by combining a prefix, timestamp, and random string.
For non-critical identification purposes, this implementation is adequate, but consider the following enhancements:
-const generateUid = (prefix = "dg") => { +/** + * Generates a unique identifier with an optional prefix + * @param prefix - The prefix to prepend to the ID (defaults to "dg") + * @returns A string in the format `${prefix}_${timestamp}_${randomString}` + */ +const generateUid = (prefix = "dg"): string => { return `${prefix}_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`; };apps/obsidian/src/components/PluginContext.tsx (1)
16-26: Well-typed provider componentThe PluginProvider component is properly typed and implements a clean pattern for providing the plugin instance to child components.
Consider adding a memo wrapper if this provider is used in performance-sensitive areas:
-export const PluginProvider = ({ +export const PluginProvider = React.memo(({ plugin, children, }: { plugin: DiscourseGraphPlugin; children: ReactNode; -}) => { +}) => { return ( <PluginContext.Provider value={plugin}>{children}</PluginContext.Provider> ); -}; +});apps/obsidian/styles.css (1)
12-18: Smooth transition effects for relationship visualizationThe transition and hover effects provide good visual feedback to users when interacting with relationship elements.
Consider specifying which properties should transition instead of using
allfor better performance:.discourse-relations .relationship-visualization { - transition: all 0.2s ease; + transition: background 0.2s ease, box-shadow 0.2s ease; }apps/obsidian/src/components/NodeTypeSettings.tsx (2)
7-10: Inconsistent import paths.There's an inconsistency in the import paths. Line 9 uses the alias
~/utils/generateUidwhile other utility imports use relative paths.-import generateUid from "~/utils/generateUid"; +import generateUid from "../utils/generateUid";
33-61: Refactor duplicate validation logic.The validation logic for format and name fields contains significant duplication. Consider extracting this into a reusable function.
if (field === "format") { - const { isValid, error } = validateNodeFormat(value, updatedNodeTypes); - if (!isValid) { - setFormatErrors((prev) => ({ - ...prev, - [index]: error || "Invalid format", - })); - } else { - setFormatErrors((prev) => { - const newErrors = { ...prev }; - delete newErrors[index]; - return newErrors; - }); - } + updateErrors(index, validateNodeFormat(value, updatedNodeTypes)); } else if (field === "name") { - const nameValidation = validateNodeName(value, updatedNodeTypes); - if (!nameValidation.isValid) { - setFormatErrors((prev) => ({ - ...prev, - [index]: nameValidation.error || "Invalid name", - })); - } else { - setFormatErrors((prev) => { - const newErrors = { ...prev }; - delete newErrors[index]; - return newErrors; - }); - } + updateErrors(index, validateNodeName(value, updatedNodeTypes)); } + // Add this helper function at the top of your component + const updateErrors = (index: number, validation: { isValid: boolean; error?: string }) => { + if (!validation.isValid) { + setFormatErrors((prev) => ({ + ...prev, + [index]: validation.error || "Invalid input", + })); + } else { + setFormatErrors((prev) => { + const newErrors = { ...prev }; + delete newErrors[index]; + return newErrors; + }); + } + };apps/obsidian/src/components/RelationshipTypeSettings.tsx (2)
45-62: Unnecessary reload of settings after deletion.After deleting a relationship type and saving settings, you're also loading settings. This is unnecessary since you've just saved them and could cause additional flicker or delay.
const handleDeleteRelationType = async (index: number): Promise<void> => { const isUsed = plugin.settings.discourseRelations?.some( (rel) => rel.relationshipTypeId === relationTypes[index]?.id, ); if (isUsed) { new Notice( "Cannot delete this relation type as it is used in one or more relations.", ); return; } const updatedRelationTypes = relationTypes.filter((_, i) => i !== index); setRelationTypes(updatedRelationTypes); plugin.settings.relationTypes = updatedRelationTypes; await plugin.saveSettings(); - await plugin.loadSettings(); };
64-82: Add validation for complement field uniqueness.While labels are checked for uniqueness, the complement field isn't. This could allow duplicates that might cause confusion.
const handleSave = async (): Promise<void> => { for (const relType of relationTypes) { if (!relType.id || !relType.label || !relType.complement) { new Notice("All fields are required for relation types."); return; } } const labels = relationTypes.map((rt) => rt.label); if (new Set(labels).size !== labels.length) { new Notice("Relation type labels must be unique."); return; } + const complements = relationTypes.map((rt) => rt.complement); + if (new Set(complements).size !== complements.length) { + new Notice("Relation type complements must be unique."); + return; + } plugin.settings.relationTypes = relationTypes; await plugin.saveSettings(); setHasUnsavedChanges(false); new Notice("Relation types saved."); };apps/obsidian/src/components/RelationshipSettings.tsx (3)
1-1: Unused import.The
useEffecthook is imported but not used in this component.-import { useState, useEffect } from "react"; +import { useState } from "react";
60-66: Inconsistent save behavior across components.Like in NodeTypeSettings, the delete operation immediately saves changes to settings, while other operations require explicit saving.
const handleDeleteRelation = async (index: number): Promise<void> => { const updatedRelations = discourseRelations.filter((_, i) => i !== index); setDiscourseRelations(updatedRelations); - plugin.settings.discourseRelations = updatedRelations; - await plugin.saveSettings(); - new Notice("Relation deleted"); + setHasUnsavedChanges(true); + new Notice("Relation will be deleted when you save changes"); };
17-25: Consider caching lookup functions.The
findNodeByIdandfindRelationTypeByIdfunctions might be called multiple times during rendering. Consider memoizing these functions withuseMemoor caching their results.+import { useState, useMemo } from "react"; const RelationshipSettings = () => { const plugin = usePlugin(); // ... - const findNodeById = (id: string): DiscourseNode | undefined => { - return plugin.settings.nodeTypes.find((node) => node.id === id); - }; + const nodeMap = useMemo(() => { + const map = new Map<string, DiscourseNode>(); + plugin.settings.nodeTypes.forEach(node => map.set(node.id, node)); + return map; + }, [plugin.settings.nodeTypes]); + + const findNodeById = (id: string): DiscourseNode | undefined => { + return nodeMap.get(id); + }; // Similar for findRelationTypeByIdapps/obsidian/src/utils/validateNodeType.ts (1)
72-100: Add unit tests for each validation scenario.Currently, there's no evidence of test coverage for these validation functions. Implementing tests that cover all the edge cases (e.g., empty format, invalid format without
{content}, duplicate names, etc.) will help ensure code stability and prevent regressions.apps/obsidian/src/components/Settings.tsx (2)
2-2: Remove unused import.
Noticeis not used in this file. Eliminating unused imports tidies up the code and avoids confusion:- import { App, PluginSettingTab, Notice } from "obsidian"; + import { App, PluginSettingTab } from "obsidian";
16-84: Extract inline styling to improve maintainability.Repeated inline styling for tabs can be moved into a dedicated CSS/SASS/SCSS file. This approach centralizes style definitions, making them easier to maintain and update as the codebase grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/obsidian/src/components/NodeTypeSettings.tsx(1 hunks)apps/obsidian/src/components/PluginContext.tsx(1 hunks)apps/obsidian/src/components/RelationshipSettings.tsx(1 hunks)apps/obsidian/src/components/RelationshipTypeSettings.tsx(1 hunks)apps/obsidian/src/components/Settings.tsx(2 hunks)apps/obsidian/src/index.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)apps/obsidian/src/utils/generateUid.ts(1 hunks)apps/obsidian/src/utils/validateNodeType.ts(1 hunks)apps/obsidian/styles.css(1 hunks)
🔇 Additional comments (12)
apps/obsidian/src/index.ts (1)
8-9: Settings structure extended appropriatelyThe addition of
discourseRelationsandrelationTypesarrays to the default settings properly aligns with the PR's objective to define relationships.apps/obsidian/src/components/PluginContext.tsx (2)
1-6: Well-structured React context implementationThe implementation of PluginContext follows React best practices for creating a typed context.
8-14: Good error handling in custom hookThe usePlugin hook correctly throws a descriptive error when used outside of its provider context, which will help developers identify integration issues.
apps/obsidian/styles.css (1)
1-10: Good styling for relationship nodesThe styling for relationship nodes uses appropriate CSS variables for theme compatibility and provides a clean, consistent appearance.
apps/obsidian/src/types.ts (4)
1-7: Good addition of id field to DiscourseNode type.Adding an
idfield to theDiscourseNodetype is a good practice as it provides a stable reference for relationships.
9-13: Well-structured relationship type definition.The
DiscourseRelationTypestructure withlabelandcomplementfields provides a clear way to express bidirectional relationships (e.g., "supports" and "is supported by").
15-19: Clean graph-based relation structure.The
DiscourseRelationtype properly implements a graph-like structure with source, destination, and relationship type references using IDs, which is an appropriate design for modeling relationships.
21-25: Consistent Settings type extension.The Settings type has been appropriately extended to include the new relationship-related arrays, maintaining type safety throughout the application.
apps/obsidian/src/components/NodeTypeSettings.tsx (1)
81-105: Inconsistent save behavior between delete and other operations.The delete operation immediately saves changes to settings, while other operations require clicking "Save Changes". This inconsistency might confuse users.
Consider either:
- Making the delete operation update only the local state without saving, requiring "Save Changes" like other operations, or
- Adding a confirmation dialog before deleting since it's immediately saved
const handleDeleteNodeType = async (index: number): Promise<void> => { const nodeId = nodeTypes[index]?.id; const isUsed = plugin.settings.discourseRelations?.some( (rel) => rel.sourceId === nodeId || rel.destinationId === nodeId, ); if (isUsed) { new Notice( "Cannot delete this node type as it is used in one or more relations.", ); return; } const updatedNodeTypes = nodeTypes.filter((_, i) => i !== index); setNodeTypes(updatedNodeTypes); + setHasUnsavedChanges(true); - plugin.settings.nodeTypes = updatedNodeTypes; - await plugin.saveSettings(); if (formatErrors[index]) { setFormatErrors((prev) => { const newErrors = { ...prev }; delete newErrors[index]; return newErrors; }); } };apps/obsidian/src/components/RelationshipSettings.tsx (1)
192-269: Good relationship visualization.The relationship visualization provides an excellent user experience by showing how node types are connected. This makes it easier for users to understand the relationships they're creating.
apps/obsidian/src/utils/validateNodeType.ts (1)
31-34: Ensure uniqueness logic reflects intended behavior.
validateFormatUniqueness()checks if any two nodes innodeTypesshare the sameformat. If you want to allow a node to preserve its existing format without triggering a duplicate error (e.g., when editing a single node), consider tailoring the uniqueness check to exclude the current node under edit. Otherwise, multiple edits to a single node or comparing across large sets might result in false-positive duplicates.apps/obsidian/src/components/Settings.tsx (1)
111-113: Well-structured context integration.Wrapping the
Settingscomponent inPluginProvidercleanly separates plugin logic from the UI. This promotes modularity and makes the component more testable.
trangdoan982
force-pushed
the
trang/relationship-type-def
branch
from
824dead to
c42d587
Compare
| }; | ||
| const Settings = () => { | ||
| const plugin = usePlugin(); | ||
| const [activeTab, setActiveTab] = useState("nodeTypes"); |
There was a problem hiding this comment.
nit - we can address this later
since we are using the unsaved changes pattern, we should probably also warn before tab switch if unsaved changes
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
2 participants
https://linear.app/discourse-graphs/issue/ENG-42/create-setting-to-define-a-relationship
Node types
Relation Type
Discourse relation definition
Confirm before delete:
