-
Notifications
You must be signed in to change notification settings - Fork 2
ENG-332 content to concept conversion functions #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
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 📝 WalkthroughWalkthroughA new module was introduced to provide utility functions for converting discourse nodes and relations from Roam Research into a local concept data format suitable for database insertion. The module defines type aliases, helper functions for querying metadata, conversion functions for various discourse structures, and utilities for extracting related concept references. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant conceptConversion
participant RoamAPI
Caller->>conceptConversion: discourseNodeSchemaToLocalConcept(context, node)
conceptConversion->>RoamAPI: getNodeMetadata(node)
RoamAPI-->>conceptConversion: metadata
conceptConversion-->>Caller: LocalConceptDataInput
Caller->>conceptConversion: discourseRelationDataToLocalConcept(context, relationSchemaUid, relationNodes)
conceptConversion->>RoamAPI: getNodeMetadata(relationNodes[role])
RoamAPI-->>conceptConversion: metadata for each node
conceptConversion-->>Caller: LocalConceptDataInput
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/roam/src/utils/conceptConversion.ts (2)
7-8
: Spelling mistake & TODO reference need cleanupThe commented-out import path contains a typo (
@repo/dataase/input_types
) and the TODO itself will become obsolete once the real type is available. Cleaning this up early avoids future search noise.-// When it's merged: import type { LocalConceptDataInput } from '@repo/dataase/input_types'; +// TODO(ENG-332): Replace the local alias with the shared type once +// `@repo/database/input_types` is published.
60-76
: Minor: consider trimming and length-guardingtext
forname
name: text
might exceed expected column limits or contain trailing whitespace. A quick sanitisation step avoids DB errors.- name: text, + name: text.trim().slice(0, 255), // keep within varchar(255)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/conceptConversion.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/roam/src/utils/conceptConversion.ts (2)
packages/database/types.gen.ts (1)
Database
(9-694)apps/roam/src/utils/supabaseContext.ts (1)
SupabaseContext
(9-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (1)
apps/roam/src/utils/conceptConversion.ts (1)
144-150
:.flat()
explodes strings into single characters
Object.values(concept.local_reference_content || {}).flat()
will split a string"abc"
into['a','b','c']
.
Use explicit normalisation to an array first.- const relations = Object.values(concept.local_reference_content || {}).flat(); + const relations = Object.values(concept.local_reference_content || {}) + .flatMap((v) => (Array.isArray(v) ? v : [v]));Likely an incorrect or invalid review comment.
@maparent I'm curious why this is merging into ENG-446-bulk-db-concept-upsert instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do a first pass before reviewing the functionality.
0da9b2d
to
2963277
Compare
d0afedb
to
92d0931
Compare
Because it relies on the on the concept local input types defined in 446. |
7eecff6
to
9375544
Compare
92d0931
to
a8ec1bf
Compare
9375544
to
8377b00
Compare
a8ec1bf
to
42633c9
Compare
actually wait will make minor adjustments |
8377b00
to
1e5c4c6
Compare
added comments as in the other one. |
1e5c4c6
to
6c22171
Compare
6c22171
to
35e3e48
Compare
3e5c7cf
to
92b1032
Compare
@CodeRabbit review function orderConceptsByDependency |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 2806 🏁 Script executed:
Length of output: 7047 @maparent I've found several critical issues in the 🚨 Critical Bugs1. Reversed Key-Value Mapping (Line ~) // BROKEN:
const conceptById: { [key: string]: LocalConceptDataInput } =
Object.fromEntries(concepts.map((c) => [c, c.represented_by_local_id]));
// SHOULD BE:
const conceptById: { [key: string]: LocalConceptDataInput } =
Object.fromEntries(concepts.map((c) => [c.represented_by_local_id!, c])); 2. Wrong Array Method in // BROKEN: unshift() returns array length, not the element
const relatedConceptId = relatedConceptIds.unshift();
// SHOULD BE: shift() removes and returns first element
const relatedConceptId = relatedConceptIds.shift();
|
@CodeRabbit thank you, those were serious concerns indeed. I stayed a bit simpler than what you proposed, because I know I can assume acyclicity. I also gave more context. How does it look now? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 7980 @maparent Great job fixing the critical bugs! 🎉 The key improvements I see: ✅ Fixed Issues
🐛 One Remaining BugThere's still an issue in the main while loop: // BUG: This will fail after first iteration
const first = conceptById[concepts[0].represented_by_local_id!]; After the first iteration, Simple fix: const first = Object.values(conceptById)[0]; // Get any remaining concept 🌟 Love the ImprovementsThe missing dependency tracking is brilliant - it allows callers to verify that all dependencies exist in the database before upserting. The cleaner return type Once you fix that one line, this function will be solid! The assumption of acyclicity simplifies things perfectly for your use case. ✏️ Learnings added
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unused import and update the inputTypes
import, then feel free to merge.
These are the proposed functions to materialize nodes in the concept database.
Summary by CodeRabbit
Also wrote functions to order concepts in order of dependency. Not strictly needed in the current scenario, but could be needed down the road, and adds to the understanding of constraints of the sync function.