ENG-1088 An affordance to delete reified relations #595
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds delete functionality for reified relations in the results view by exporting a query utility, augmenting result items with target UID metadata, adjusting field propagation in discourse context, and implementing a delete handler that queries, removes, and refreshes the results. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 (4)
apps/roam/src/utils/getDiscourseContextResults.ts (1)
61-66: Consider adding explicit default value for consistency.The feature flag check works correctly, but for consistency with the usage in
registerDiscourseDatalogTranslators.ts(line 412), consider adding an explicitfalsedefault value:- if (getSetting("use-reified-relations")) + if (getSetting<boolean>("use-reified-relations", false))This makes the intent clearer and matches the established pattern elsewhere in the codebase.
apps/roam/src/components/results-view/ResultsTable.tsx (3)
177-177: Unrelated change: CellEmbed div simplification.This line simplifies the nested div structure in the
CellEmbedcomponent, but this change appears unrelated to the PR's objective of implementing relation deletion functionality. Consider moving unrelated refactorings to a separate commit or PR for clearer change tracking.
267-275: Consider improving error handling and user feedback.The current implementation refreshes the UI regardless of whether the deletion succeeds or fails. While this might be intentional to show the current state, consider the following improvements:
- Refresh only on success: Only call
onRefresh()when deletion succeeds- User feedback: Provide toast notifications or alerts to inform users of success/failure
Example improvement:
const onDelete = () => { void deleteBlock(r["relation-uid"]) .then(() => { // Show success feedback here if desired onRefresh(); }) .catch((e) => { console.error(e); // Show error feedback to user here }); };
333-341: Consider explicit feature flag check for defensive programming.The Delete button currently relies on the presence of
r["relation-uid"]to determine visibility, which indirectly depends on theuse-reified-relationsfeature flag being enabled upstream. While this works, consider adding an explicit feature flag check for more defensive programming:{getSetting<boolean>("use-reified-relations", false) && r["relation-uid"] && ( <Button minimal icon="delete" className="float-right" title="Delete relation" onClick={onDelete} /> )}This makes the dependency on the feature flag explicit and guards against potential future changes in data structure.
Nitpick: The self-closing Button tag has a redundant
></Button>- either use<Button ... />or<Button ...></Button>, but not both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/results-view/ResultsTable.tsx(3 hunks)apps/roam/src/utils/getDiscourseContextResults.ts(2 hunks)apps/roam/src/utils/registerDiscourseDatalogTranslators.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/results-view/ResultsTable.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/results-view/ResultsTable.tsx
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/results-view/ResultsTable.tsx
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/results-view/ResultsTable.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/registerDiscourseDatalogTranslators.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/components/results-view/ResultsTable.tsx
🧠 Learnings (4)
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
apps/roam/src/utils/registerDiscourseDatalogTranslators.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/roam/src/utils/getDiscourseContextResults.ts
📚 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/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/utils/getDiscourseContextResults.ts
📚 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/roam/src/utils/getDiscourseContextResults.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/getDiscourseContextResults.ts (1)
apps/roam/src/utils/extensionSettings.ts (1)
getSetting(3-11)
🔇 Additional comments (2)
apps/roam/src/utils/registerDiscourseDatalogTranslators.ts (1)
821-821: LGTM! Preserves relation variable for downstream access.The identity mapping for "rel" ensures the relation block variable is carried through the Datalog replacement step, enabling the relation UID to be exposed in query results for deletion functionality. This follows the same pattern as the source and target variable mappings.
apps/roam/src/utils/getDiscourseContextResults.ts (1)
10-10: LGTM! Import correctly added.The import statement properly brings in the
getSettingfunction needed for the feature flag check below.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
RE: cache
Discourse context is currently cached and we have an ignoreCache mechanism. This should be called when we delete a relation. ResultsView is passed onRefresh which is generally where the cache refresh is called, so take a look there.
If we are not adding/fixing that here, please create and link to the linear task in which it is planned on being completed so we can keep track of it.
| text: `node:${conditionUid}-Anchor`, | ||
| }); | ||
| } | ||
| if (getSetting<boolean>("use-reified-relations", false)) |
There was a problem hiding this comment.
Selections are additional columns that are meant to be displayed. Let's move this down to the result mapping on ln 96-99.
There was a problem hiding this comment.
I would love not to use selections, but I do not see another good hook to add to what gets fetch/pulled. By 96-99, the query has been sent, and rel is just not part of the result set. Maybe we need to discuss how to do this properly, it would require quite serious changes.
There was a problem hiding this comment.
What about passing the source, target, and relation type id's and querying for the reified relation block id(s) that need to be deleted.
There was a problem hiding this comment.
That's an extra query for no good reason… but I think it's a good workaround for now.
There was a problem hiding this comment.
status:
To do this, I need the Uid of the focus of the DiscourseContext passed down.
I expected this to be set as parentUid, but actually this prop is ambiguous.
It is indeed the focus in the case of a DiscourseContext result;
But when the results are for a query block, the parentUid is actually the query block's uid, used for calculating settings.
What gets passed down from ResultsView to ResultsTable is taken from the settings:
ResultsView:1337, parentUid={settings.resultNodeUid}
So in the DiscourseContext case, this is empty, and the parentUid is lost.
I am quite sure that replacing that with parentUid is going to create trouble downstream.
I propose actually having two separate props: queryBlockUid and discourseContextFocusUid, or some such.
But that's a big change and I want to confirm.
There was a problem hiding this comment.
We should be able to bypass ResultsView here and just pass the required uid (source of discourse context, whether that be from a page discourse context or the discourse context overlay on a page reference) directly via the `Result` object.
I don't believe we'll need to change something that will affect non-discourse context queries globally.
I'd have to dig into the code to find exactly where, but if we need to pair program on this, let me know.
There was a problem hiding this comment.
I think we should. Probably tomorrow after the dev meeting.
| [ | ||
| { from: source, to: source }, | ||
| { from: target, to: target }, | ||
| { from: "rel", to: "rel" }, |
There was a problem hiding this comment.
If we move the relation-uid from selection to result, this is no longer needed, correct?
Sending necessary data through the result.
maparent
force-pushed
the
eng-1088-an-affordance-to-delete-reified-relations
branch
from
58773e6 to
7ef9d88
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/DiscourseContext.tsx(1 hunks)apps/roam/src/components/results-view/ResultsTable.tsx(5 hunks)apps/roam/src/utils/createReifiedBlock.ts(1 hunks)apps/roam/src/utils/getDiscourseContextResults.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/roam/src/utils/getDiscourseContextResults.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/roam/src/utils/createReifiedBlock.tsapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/utils/createReifiedBlock.tsapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/utils/createReifiedBlock.tsapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/utils/createReifiedBlock.tsapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/createReifiedBlock.tsapps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
Applied to files:
apps/roam/src/utils/createReifiedBlock.ts
📚 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/**/*.{ts,tsx,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/results-view/ResultsTable.tsx
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
apps/roam/src/components/results-view/ResultsTable.tsx
📚 Learning: 2025-10-24T11:44:27.528Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 514
File: apps/roam/src/components/settings/SuggestiveModeSettings.tsx:105-123
Timestamp: 2025-10-24T11:44:27.528Z
Learning: In the discourse-graph codebase (especially in Roam app components using roamjs-components Toast), toast intents are passed as string literals (e.g., "primary", "danger", "success") rather than using the Blueprint Intent enum, as this is the established pattern used in >95% of the codebase.
Applied to files:
apps/roam/src/components/results-view/ResultsTable.tsx
📚 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/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/results-view/ResultsTable.tsxapps/roam/src/components/DiscourseContext.tsx
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.
Applied to files:
apps/roam/src/components/DiscourseContext.tsx
📚 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/roam/src/components/DiscourseContext.tsx
🧬 Code graph analysis (2)
apps/roam/src/components/results-view/ResultsTable.tsx (2)
apps/roam/src/utils/extensionSettings.ts (1)
getSetting(3-11)apps/roam/src/utils/createReifiedBlock.ts (1)
strictQueryForReifiedBlocks(9-42)
apps/roam/src/components/DiscourseContext.tsx (1)
apps/roam/src/utils/types.ts (1)
Result(42-46)
🔇 Additional comments (2)
apps/roam/src/utils/createReifiedBlock.ts (1)
9-11: LGTM: Export enables reified block queries from other modules.The export of
strictQueryForReifiedBlocksis appropriate for enabling the delete functionality in ResultsTable.tsx while maintaining the existing function signature and behavior.apps/roam/src/components/DiscourseContext.tsx (1)
272-272: LGTM: Destructuring adjustment passes through required fields.The change from destructuring
complementandidto including them in the spread preserves these fields in the results passed toResultsView, which are needed by the delete functionality in ResultsTable.tsx.
https://linear.app/discourse-graphs/issue/ENG-1088/an-affordance-to-delete-reified-relations
This implements a "Delete relation" button near each discourse context search result (and in some cases query builder search results.) This is only visible if reified relations are on.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.