-
Notifications
You must be signed in to change notification settings - Fork 3
[ENG-869] Key figure #490
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
[ENG-869] Key figure #490
Conversation
* cleaned * sm * address PR comments
* data persistence to the file * error handling * address PR comments * address some PR comments * address other PR comments * address PR comments
* current state * works now * clean up * address PR comments * address PR reviews * cleanup * fix styling issues * address PR comments
* current state * works now * clean up * address PR comments * address PR reviews * fix styling issues * latest progress * update new shape * shape defined * address PR comments * sm address PR review * current progress * reorg * address other PR comments * clean * simplify flow * address PR comments
* eng-604: create node flow * pwd
* eng-658-add-existing-nodes-flow * address PR comments * small changes
https://www.loom.com/share/3641f2a642714b0d849262344e8c6ee5?sid=0614c657-e541-4bfd-92df-9b1aa60945b6 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Added a Relations overlay on the canvas that shows a “Relations” button when a discourse node is selected. - Introduced a Relations panel to view and manage relations for the selected node, including adding or removing links, with clear loading/error states. - Overlay appears above the canvas without disrupting existing tools. - Chores - Consolidated relation-type lookup into shared utilities and updated imports. No user-facing changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
* add color setting * address PR reviews * address PR commens
* eng-812 : Update database cli tools: supabase, vercel, cucumber. * newer cucumber constrains node
* cleaned * sm * address PR comments
* data persistence to the file * error handling * address PR comments * address some PR comments * address other PR comments * address PR comments
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds optional key-image support for discourse nodes: introduces a boolean node-type setting (keyImage), a helper to extract the first image from a note, preloads and passes imageSrc into DiscourseNodeShape, and adjusts rendering and sizing when an image is present. Extends field rendering to support boolean types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Search as ExistingNodeSearch
participant Utils as discourseNodeShapeUtils
participant Shape as DiscourseNodeShape
User->>Search: Select file
Search->>Search: Read frontmatter → nodeType
alt nodeType.keyImage enabled
Search->>Utils: getFirstImageSrcForFile(app, file)
Utils-->>Search: imageSrc or null
Search->>Shape: Create node with props { imageSrc? }
else
Search->>Shape: Create node without imageSrc
end
Shape->>Shape: useEffect on nodeType/keyImage
alt imageSrc available
Shape->>Shape: Compute height (title/subtitle/image/padding)
Shape->>Shape: Render with <img .../>
else
Shape->>Shape: Render without image
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
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/obsidian/src/components/NodeTypeSettings.tsx (1)
263-273: Type-aware validation: future-proof boolean fieldsGood gating in handleNodeTypeChange. However, validateNodeType assumes strings and uses trim; a required boolean field would break.
Consider updating validateNodeType to branch on fieldConfig.type:
// Inside validateNodeType Object.entries(FIELD_CONFIGS).forEach(([key, config]) => { const field = key as EditableFieldKey; const raw = nodeType[field]; if (config.type === "boolean") { // For required booleans, ensure value is true or explicitly present if (config.required && raw !== true) { newErrors[field] = `${config.label} is required`; isValid = false; } return; } const value = (raw as string) ?? ""; if (config.required && !value.trim()) { newErrors[field] = `${config.label} is required`; isValid = false; return; } if (config.validate && value) { const { isValid: fieldValid, error } = config.validate(value, nodeType, nodeTypes); if (!fieldValid) { newErrors[field] = error || `Invalid ${config.label.toLowerCase()}`; isValid = false; } } });apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
221-230: Effect dependencies: consider narrowing for fewer re-rendersDepending on shape.props (object) causes the effect to run on any prop change. Consider narrowing to specific props: src, imageSrc, h, and nodeType?.keyImage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/obsidian/src/components/NodeTypeSettings.tsx(5 hunks)apps/obsidian/src/components/canvas/ExistingNodeSearch.tsx(3 hunks)apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx(6 hunks)apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)
🔇 Additional comments (3)
apps/obsidian/src/types.ts (1)
11-11: Addition of keyImage flag looks goodOptional boolean is safe and aligns with downstream usage.
apps/obsidian/src/components/NodeTypeSettings.tsx (1)
90-103: BooleanField: OKCheckbox implementation is fine and integrates cleanly with FieldWrapper.
apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
49-50: Props/API additions look goodimageSrc prop and defaults are consistent with TL validators.
Also applies to: 59-60, 29-30
33ea6a8 to
d698c41
Compare
mdroidian
left a comment
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.
Good start 👍. A few issues and bugs
| const paddingY = 2 * 8; // p-2 = 0.5rem = 8px | ||
| const titleHeight = 20; // approx | ||
| const subtitleHeight = 16; // approx | ||
| const maxImageHeight = 160; | ||
| const baseHeight = 100; | ||
| const initialHeight = preloadedImageSrc |
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.
DRY (this is used elsewhere), make this into a function
| title: file.basename, | ||
| nodeTypeId: getFrontmatterForFile(plugin.app, file)?.nodeTypeId, | ||
| nodeTypeId: fmNodeTypeId ?? "", | ||
| ...(preloadedImageSrc ? { imageSrc: preloadedImageSrc } : {}), |
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.
nit: Any particular reason for the spread for a single value? Something like this would be much cleaner:
imageSrc: preloadedImageSrc ?? "",

https://www.loom.com/share/dad6f94dbbf54d9693562f53a4dffe38
Summary by CodeRabbit