[ENG-1081] Add ability to remove relations on Discourse Context Panel #570
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughChanges add a delete relationship feature to the Obsidian RelationshipSection component with a new delete button and deleteRelationship callback, while refactoring turbo.json task dependencies from "with" to "dependsOn" for roam#dev and website#dev tasks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 (3)
apps/obsidian/src/components/RelationshipSection.tsx (3)
340-427: AlignuseEffectdependencies withloadCurrentRelationshipsand avoid lint warningsThe new
loadCurrentRelationshipsuseCallbacklooks good, butuseEffectabove now depends on it (direct call + insideonMetadataChange) without listing it in the dependency array. This will violatereact-hooks/exhaustive-depsand is slightly indirect.You can make the relationship explicit and a bit clearer by moving the callback above the effect and depending on it directly:
- useEffect(() => { - loadCurrentRelationships(); - const onMetadataChange = (file: TFile) => { - if (file && file.path === activeFile.path) { - loadCurrentRelationships(); - } - }; - plugin.app.metadataCache.on("changed", onMetadataChange); - return () => { - plugin.app.metadataCache.off( - "changed", - onMetadataChange as (...data: unknown[]) => unknown, - ); - }; - }, [activeFile, plugin]); - - const loadCurrentRelationships = useCallback(async () => { + const loadCurrentRelationships = useCallback(async () => { const fileCache = plugin.app.metadataCache.getFileCache(activeFile); if (!fileCache?.frontmatter) return; @@ - setGroupedRelationships(Array.from(tempRelationships.values())); - }, [activeFile, plugin]); + setGroupedRelationships(Array.from(tempRelationships.values())); + }, [activeFile, plugin]); + + useEffect(() => { + loadCurrentRelationships(); + + const onMetadataChange = (file: TFile) => { + if (file && file.path === activeFile.path) { + loadCurrentRelationships(); + } + }; + + plugin.app.metadataCache.on("changed", onMetadataChange); + + return () => { + plugin.app.metadataCache.off( + "changed", + onMetadataChange as (...data: unknown[]) => unknown, + ); + }; + }, [activeFile, plugin, loadCurrentRelationships]);Behavior stays the same, but the hook dependencies become explicit and lint-friendly.
428-491: Delete logic is sound; consider minor cleanups and explicit typing (optional)The
deleteRelationshipimplementation correctly:
- Locates the
relationTypeby id.- Removes the appropriate wiki link from both
activeFileandlinkedFilefrontmatter, handling scalar vs array values and deleting the key when empty.- Shows clear success/error notices and refreshes state via
loadCurrentRelationships().Two small, optional refinements you might consider:
- Parallelize frontmatter updates to reduce latency a bit:
- // Remove link from active file - await removeLinkFromFrontmatter( - activeFile, - linkedFile.name, - relationTypeId, - ); - - // Remove reverse link from linked file - await removeLinkFromFrontmatter( - linkedFile, - activeFile.name, - relationTypeId, - ); + await Promise.all([ + removeLinkFromFrontmatter(activeFile, linkedFile.name, relationTypeId), + removeLinkFromFrontmatter(linkedFile, activeFile.name, relationTypeId), + ]);
- Add an explicit return type for clarity with TS guidelines, e.g.:
const deleteRelationship = useCallback( async (linkedFile: TFile, relationTypeId: string): Promise<void> => { // ... }, [/* ... */], );Not required for correctness, but these keep the implementation a bit tighter and closer to the stated TS conventions.
513-536: New delete button works; consider small accessibility and icon tweaksThe new inline delete button per relationship (
×with a tooltip) is functional and styled consistently with the rest of the panel.To align more closely with accessibility and Obsidian icon conventions, you could optionally:
- Add an
aria-labelfor screen readers (title alone isn’t always sufficient).- Swap the raw
"×"for a Lucide or Obsidian icon, per the Obsidian style guidance.For example:
- <button - className="!text-muted hover:!text-error cursor-pointer border-0 !bg-transparent p-1 text-sm" - onClick={(e) => { - e.preventDefault(); - deleteRelationship(file, group.relationTypeOptions.id); - }} - title="Delete relationship" - > - × - </button> + <button + className="!text-muted hover:!text-error cursor-pointer border-0 !bg-transparent p-1 text-sm" + onClick={(e) => { + e.preventDefault(); + deleteRelationship(file, group.relationTypeOptions.id); + }} + title="Delete relationship" + aria-label="Delete relationship" + > + × + </button>Functionally you’re good; this is just polish.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/obsidian/src/components/RelationshipSection.tsx(4 hunks)turbo.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/components/RelationshipSection.tsx
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/components/RelationshipSection.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
turbo.json
🔇 Additional comments (1)
turbo.json (1)
63-68: Switch todependsOnis consistent with the rest of the Turbo configUsing
"dependsOn": ["@repo/database#dev"]for bothroam#devandwebsite#devmatches how other tasks (e.g.,build,lint,deploy) express dependencies in this file and keeps the task graph explicit. No issues spotted with this change.
https://www.loom.com/share/c30a96008bff4252ac98c556d0815fe1
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.