[ENG-869] Key figure #522
[ENG-869] Key figure #522
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds optional key-image support for discourse node types: new boolean field, image extraction/loading utilities, automatic size calculation, and canvas shape updates to render and resize nodes with images. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExistingNodeSearch
participant Utils as Utilities
participant DiscourseNodeShape
User->>ExistingNodeSearch: select file to create node
ExistingNodeSearch->>ExistingNodeSearch: read frontmatter -> fmNodeTypeId
alt fmNodeType enables keyImage
ExistingNodeSearch->>Utils: getFirstImageSrcForFile(file)
Utils-->>ExistingNodeSearch: imageSrc or null
end
ExistingNodeSearch->>Utils: calcDiscourseNodeSize(title, nodeTypeId, imageSrc)
Utils->>Utils: measureNodeText(...)
alt imageSrc present
Utils->>Utils: loadImage(imageSrc) -> image dims
end
Utils-->>ExistingNodeSearch: { w, h }
ExistingNodeSearch->>DiscourseNodeShape: create node with imageSrc, w, h, nodeTypeId
DiscourseNodeShape->>DiscourseNodeShape: render title (+ image if imageSrc)
DiscourseNodeShape->>DiscourseNodeShape: onResize -> resizeBox -> update w/h
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/obsidian/src/components/NodeTypeSettings.tsx (1)
431-455: Boolean-aware validation: avoid string-only .trim() and checks.validateNodeType assumes strings; future required boolean fields will break. Make it type-aware.
- Object.entries(FIELD_CONFIGS).forEach(([key, config]) => { + Object.entries(FIELD_CONFIGS).forEach(([key, config]) => { const field = key as EditableFieldKey; - const value = nodeType[field] as string; + const raw = nodeType[field]; + const value = typeof raw === "string" ? raw : ""; - if (config.required && !value?.trim()) { + if (config.required) { + if (config.type === "boolean") { + if (raw !== true) { + newErrors[field] = `${config.label} is required`; + isValid = false; + return; + } + } else if (!value?.trim()) { + newErrors[field] = `${config.label} is required`; + isValid = false; + return; + } + } - if (config.validate && value) { + if (config.validate && typeof raw === "string" && value) { const { isValid: fieldValid, error } = config.validate( value, nodeType, nodeTypes, );
🧹 Nitpick comments (13)
apps/obsidian/src/components/NodeTypeSettings.tsx (4)
35-35: Type now includes "boolean"; tighten validation typing to avoid future footguns.validate in BaseFieldConfig takes string, and downstream validators/required checks assume strings. It works because boolean fields skip validation, but this is fragile if a boolean ever becomes required. Consider a discriminated union for field configs to encode type at the type level and prevent string-only checks from touching booleans.
131-144: Checkbox a11y: associate with label/description.Wire an accessible name so screen readers announce what this checkbox controls.
Apply:
-const BooleanField = ({ - value, - onChange, -}: { - value: boolean; - onChange: (value: boolean) => void; -}) => ( - <input - type="checkbox" - checked={!!value} - onChange={(e) => onChange((e.target as HTMLInputElement).checked)} - /> -); +const BooleanField = ({ + value, + onChange, + ariaLabel, +}: { + value: boolean; + onChange: (value: boolean) => void; + ariaLabel?: string; +}) => ( + <input + type="checkbox" + checked={!!value} + aria-label={ariaLabel} + onChange={(e) => onChange((e.target as HTMLInputElement).checked)} + /> +);
322-329: Validation gating by typeof is fine; also clear stale errors on boolean edits.Since booleans skip validateField, consider clearing any prior error for this field on boolean change to avoid stale error states after type switches.
- const updatedNodeType = { ...editingNodeType, [field]: value }; + const updatedNodeType = { ...editingNodeType, [field]: value }; if (typeof value === "string") { validateField(field, value, updatedNodeType); } + if (typeof value === "boolean") { + setErrors((prev) => { + const { [field]: _, ...rest } = prev; + return rest; + }); + }
461-488: Narrow the onChange types per control to avoid bivariant function pitfalls.Pass field-type-specific handlers instead of a union-typed handleChange for stronger type safety.
- const value = editingNodeType[fieldConfig.key] as string | boolean; - const error = errors[fieldConfig.key]; - const handleChange = (newValue: string | boolean) => - handleNodeTypeChange(fieldConfig.key, newValue); + const value = editingNodeType[fieldConfig.key] as string | boolean; + const error = errors[fieldConfig.key]; ... - <TemplateField - value={value as string} - error={error} - onChange={handleChange} + <TemplateField + value={value as string} + error={error} + onChange={(v: string) => handleNodeTypeChange(fieldConfig.key, v)} templateConfig={templateConfig} templateFiles={templateFiles} /> ) : fieldConfig.type === "color" ? ( - <ColorField - value={value as string} - error={error} - onChange={handleChange} - /> + <ColorField + value={value as string} + error={error} + onChange={(v: string) => handleNodeTypeChange(fieldConfig.key, v)} + /> ) : fieldConfig.type === "boolean" ? ( - <BooleanField value={value as boolean} onChange={handleChange} /> + <BooleanField + value={value as boolean} + ariaLabel={fieldConfig.label} + onChange={(v: boolean) => handleNodeTypeChange(fieldConfig.key, v)} + /> ) : ( <TextField fieldConfig={fieldConfig} value={value as string} error={error} - onChange={handleChange} + onChange={(v: string) => handleNodeTypeChange(fieldConfig.key, v)} nodeType={editingNodeType} />apps/obsidian/src/utils/loadImage.ts (1)
11-39: Harden image loader: cleanup handlers, SSR guard, optional CORS hint.Prevents leaks and improves robustness in tests/SSR.
export const loadImage = ( url: string, ): Promise<{ width: number; height: number }> => { - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { + if (typeof Image === "undefined") { + reject(new Error("Image API not available in this environment")); + return; + } const img = new Image(); let resolved = false; - const timeoutId = setTimeout(() => { + // Allow natural size without tainting canvas if later used + img.crossOrigin = "anonymous"; + + const timeoutId = setTimeout(() => { if (!resolved) { resolved = true; - reject(new Error("Failed to load image: timeout")); + // Best-effort cleanup + img.onload = null; + img.onerror = null; + img.src = ""; + reject(new Error("Failed to load image: timeout")); } }, 10000); - img.onload = () => { + const cleanup = () => { + img.onload = null; + img.onerror = null; + clearTimeout(timeoutId); + }; + + img.onload = () => { if (!resolved) { resolved = true; - clearTimeout(timeoutId); + cleanup(); resolve({ width: img.naturalWidth, height: img.naturalHeight }); } }; - img.onerror = () => { + img.onerror = () => { if (!resolved) { resolved = true; - clearTimeout(timeoutId); + cleanup(); reject(new Error("Failed to load image")); } }; img.src = url; }); };apps/obsidian/src/utils/measureNodeText.ts (3)
25-45: Keep measurement in sync with component styles.Inline styles risk drifting from the real Tailwind classes. Prefer applying the same className strings to the temp elements so future style tweaks automatically reflect here.
Example:
container.className = "box-border flex flex-col items-start justify-start rounded-md border-2 p-2"; titleEl.className = "m-1 text-base font-semibold"; subtitleEl.className = "m-0 text-sm";Then only set min/max width inline.
27-45: Minimize layout side-effects during measurement.Move it far off-screen to mitigate any accidental hit-testing/overlap in edge cases.
container.style.setProperty("position", "absolute"); container.style.setProperty("visibility", "hidden"); container.style.setProperty("pointer-events", "none"); + container.style.setProperty("left", "-10000px"); + container.style.setProperty("top", "-10000px");
69-73: Return integer dimensions to avoid subpixel jitter.Rounding prevents tiny pixel differences causing layout churn downstream.
- return { - w: rect.width, - h: rect.height, - }; + return { + w: Math.ceil(rect.width), + h: Math.ceil(rect.height), + };apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts (2)
41-61: Normalize target before external/internal resolution.Angle-bracketed markdown targets like https://... won’t pass the external test before normalization. Normalize first, then branch.
- if (match[2]) { - const target = match[2].trim(); - - // External URL - return directly - if (/^https?:\/\//i.test(target)) { - return target; - } + if (match[2]) { + const normalized = normalizeLinkTarget(match[2].trim()); + const target = normalized ?? match[2].trim(); + + // External URL - return directly + if (/^https?:\/\//i.test(target)) { + return target; + } - // Internal path - resolve to vault file - const normalized = normalizeLinkTarget(target); - const tfile = app.metadataCache.getFirstLinkpathDest( - normalized ?? target, + // Internal path - resolve to vault file + const tfile = app.metadataCache.getFirstLinkpathDest( + target, file.path, );
26-80: Consider metadataCache embeds for performance and accuracy.Parsing the whole file may match images inside code blocks. metadataCache.getFileCache(file)?.embeds (and .links) already provide resolved targets; using them avoids full reads and regex pitfalls.
apps/obsidian/src/utils/calcDiscourseNodeSize.ts (1)
70-74: Consider adding type safety for the error.The catch block logs the error but uses a generic type. For better debugging, consider typing the error parameter.
- } catch (error) { + } catch (error: unknown) { // If image fails to load, fall back to text-only dimensions console.warn("calcDiscourseNodeSize: failed to load image", error); return { w, h: textHeight };apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (1)
213-226: Clarify the 1-pixel threshold rationale.The 1px threshold (lines 214-215) prevents unnecessary updates when dimensions are nearly identical. Combined with excluding
wandhfrom the dependencies (line 238-240), this avoids fighting manual resizing.Consider adding a comment near the threshold check explaining that this tolerance allows minor calculation variations while respecting user-initiated resizes.
const { w, h } = await calcDiscourseNodeSize({ title: linkedFile.basename, nodeTypeId: shape.props.nodeTypeId, imageSrc: currentImageSrc, plugin, }); + // Only update dimensions if they differ significantly (>1px) + // This avoids fighting manual resizing and accounts for minor calculation variances if ( Math.abs((shape.props.w || 0) - w) > 1 || Math.abs((shape.props.h || 0) - h) > 1apps/obsidian/src/components/canvas/shapes/nodeConstants.ts (1)
1-32: Consider grouping related constants.The constants are well-named and documented. For better organization, consider grouping them by category (dimensions, spacing, typography, images) with section comments.
/** * Constants for Discourse Node styling and sizing. * These values match the Tailwind classes used in DiscourseNodeShape component. */ +// Dimension constraints export const DEFAULT_NODE_WIDTH = 200; export const MIN_NODE_WIDTH = 160; export const MAX_NODE_WIDTH = 400; -// Padding: p-2 = 0.5rem = 8px on each side = 16px total vertical +// Spacing +// Padding and border: p-2 (8px each side) + border-2 (2px each side) = 20px total export const BASE_PADDING = 20; +export const IMAGE_GAP = 4; -// Font sizes matching Tailwind classes +// Typography export const TITLE_FONT_SIZE = 16; // text-base export const SUBTITLE_FONT_SIZE = 14; // text-sm - -// Line height for text rendering export const LINE_HEIGHT = 1.5; - -// Font family - use system font stack similar to Tailwind export const FONT_FAMILY = 'system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Arial, sans-serif'; -// Maximum height for key images +// Image constraints export const MAX_IMAGE_HEIGHT = 250; - -// Gap between image and text -export const IMAGE_GAP = 4; - -// Base height for nodes without images (estimated) export const BASE_HEIGHT_NO_IMAGE = 100;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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(7 hunks)apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts(1 hunks)apps/obsidian/src/components/canvas/shapes/nodeConstants.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)apps/obsidian/src/utils/calcDiscourseNodeSize.ts(1 hunks)apps/obsidian/src/utils/loadImage.ts(1 hunks)apps/obsidian/src/utils/measureNodeText.ts(1 hunks)
🔇 Additional comments (7)
apps/obsidian/src/types.ts (1)
12-12: Verified: keyImage field addition is correct and safe.All usages employ optional chaining (
nodeType?.keyImage) and falsy checks (!nodeType?.keyImage), which correctly treat missing/undefined as false. Settings merge with Object.assign handles missing fields for backward-compatible loading of existing nodeTypes. NodeTypeSettings config marks the field as non-required boolean with no string mapping.apps/obsidian/src/components/NodeTypeSettings.tsx (1)
119-126: keyImage field implementation is correct—no issues found.Verification confirms:
- Field correctly defined as
type: "boolean", required: false- New node types default to
undefined(equivalent tofalse), as existingDEFAULT_NODE_TYPESdon't include it- No migration needed; the optional field is safe for existing settings
- BooleanField checkbox always returns actual boolean, never a string
- All usage sites safely check with
nodeType?.keyImage, handlingundefinedcorrectly- No string-to-boolean conversion risk exists in the codebase
apps/obsidian/src/components/canvas/ExistingNodeSearch.tsx (2)
63-74: Good defensive programming for image preloading.The try-catch wrapper around image preloading with a clear warning message is a good practice. This ensures that node creation doesn't fail even if the image loading encounters issues.
77-82: Code handles empty string nodeTypeId correctly—no changes needed.The
getNodeTypeByIdfunction uses strict equality comparison (node.id === nodeTypeId) to find matching node types. An empty string""will only match if a node type has an empty ID, which is not a practical scenario. In practice, the function returnsundefined, which is safely handled downstream incalcDiscourseNodeSizevia optional chaining (nodeType?.name) and a default fallback to"". This pattern is consistent across all call sites in the codebase and works as intended.apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (3)
66-74: LGTM: Resize handlers correctly implemented.The resize implementation correctly uses
resizeBoxfrom tldraw and unlocks the aspect ratio to allow flexible resizing. This aligns well with the dynamic sizing behavior based on content.
262-270: Good image rendering implementation.The image element uses appropriate attributes:
lazyloading for performanceasyncdecoding to prevent blockingdraggable="false"for better UXw-fullfor responsive sizingobject-coverfor aspect ratio handling
178-204: The code approach is sound and correctly uses the local variable.The verification confirms the review comment is accurate. The code maintains
currentImageSrcas a local variable that's updated beforecalcDiscourseNodeSizeis called, avoiding any issues with the non-awaitedupdateShapecalls. SincecurrentImageSrcis used for size calculation (notshape.props.imageSrc), the async window is not problematic here.The suggestion to add a clarifying comment explaining why
currentImageSrcis maintained separately is a reasonable best-practice recommendation for code clarity, but the implementation is correct as-is.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/obsidian/src/utils/loadImage.ts (1)
8-10: Add explicit return type annotation.The coding guidelines specify using explicit return types for functions. While TypeScript correctly infers the return type, adding it explicitly improves code clarity and enforces the contract.
Apply this diff:
-export const loadImage = ( - url: string, -): Promise<{ width: number; height: number }> => { +export const loadImage = (url: string): Promise<{ width: number; height: number }> => {apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts (1)
22-25: Add explicit return type annotation.Per the coding guidelines, functions should have explicit return types for better clarity and type safety.
Apply this diff:
-export const getFirstImageSrcForFile = async ( - app: App, - file: TFile, -): Promise<string | null> => { +export const getFirstImageSrcForFile = async (app: App, file: TFile): Promise<string | null> => {apps/obsidian/src/utils/measureNodeText.ts (1)
18-24: Add explicit return type annotation.Following the coding guidelines, add an explicit return type to the function signature for improved clarity.
Apply this diff:
-export const measureNodeText = ({ - title, - subtitle, -}: { - title: string; - subtitle: string; -}): { w: number; h: number } => { +export const measureNodeText = ({ title, subtitle }: { title: string; subtitle: string }): { w: number; h: number } => {apps/obsidian/src/utils/calcDiscourseNodeSize.ts (1)
25-30: Add explicit return type annotation.Per the coding guidelines, add an explicit return type to improve code clarity.
Apply this diff:
-export const calcDiscourseNodeSize = async ({ - title, - nodeTypeId, - imageSrc, - plugin, -}: CalcNodeSizeParams): Promise<{ w: number; h: number }> => { +export const calcDiscourseNodeSize = async ({ title, nodeTypeId, imageSrc, plugin }: CalcNodeSizeParams): Promise<{ w: number; h: number }> => {apps/obsidian/src/components/canvas/shapes/nodeConstants.ts (1)
30-31: Consider deriving from component constants or documenting the rationale.The "estimated" comment suggests this value is somewhat arbitrary. For better maintainability, either calculate it from existing constants (BASE_PADDING, font sizes, line heights) or document why 100px was specifically chosen.
For example, if you want to derive it:
-// Base height for nodes without images (estimated) -export const BASE_HEIGHT_NO_IMAGE = 100; +// Base height for nodes without images +// Calculated as: vertical padding + title + subtitle + gaps +export const BASE_HEIGHT_NO_IMAGE = + BASE_PADDING * 2 + // top and bottom padding + TITLE_FONT_SIZE * LINE_HEIGHT + + SUBTITLE_FONT_SIZE * LINE_HEIGHT + + 8; // additional spacing between elementsOr simply document it better:
-// Base height for nodes without images (estimated) +// Base height for nodes without images - chosen to accommodate title and subtitle with comfortable spacing export const BASE_HEIGHT_NO_IMAGE = 100;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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(7 hunks)apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts(1 hunks)apps/obsidian/src/components/canvas/shapes/nodeConstants.ts(1 hunks)apps/obsidian/src/types.ts(1 hunks)apps/obsidian/src/utils/calcDiscourseNodeSize.ts(1 hunks)apps/obsidian/src/utils/loadImage.ts(1 hunks)apps/obsidian/src/utils/measureNodeText.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Prefertypeoverinterface
Use explicit return types for functions
Avoidanytypes when possible
Files:
apps/obsidian/src/utils/loadImage.tsapps/obsidian/src/components/canvas/ExistingNodeSearch.tsxapps/obsidian/src/utils/calcDiscourseNodeSize.tsapps/obsidian/src/components/canvas/shapes/nodeConstants.tsapps/obsidian/src/components/NodeTypeSettings.tsxapps/obsidian/src/utils/measureNodeText.tsapps/obsidian/src/types.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants
Files:
apps/obsidian/src/utils/loadImage.tsapps/obsidian/src/components/canvas/ExistingNodeSearch.tsxapps/obsidian/src/utils/calcDiscourseNodeSize.tsapps/obsidian/src/components/canvas/shapes/nodeConstants.tsapps/obsidian/src/components/NodeTypeSettings.tsxapps/obsidian/src/utils/measureNodeText.tsapps/obsidian/src/types.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
Use the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for all code in the Obsidian plugin
Files:
apps/obsidian/src/utils/loadImage.tsapps/obsidian/src/components/canvas/ExistingNodeSearch.tsxapps/obsidian/src/utils/calcDiscourseNodeSize.tsapps/obsidian/src/components/canvas/shapes/nodeConstants.tsapps/obsidian/src/components/NodeTypeSettings.tsxapps/obsidian/src/utils/measureNodeText.tsapps/obsidian/src/types.tsapps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsxapps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts
🔇 Additional comments (19)
apps/obsidian/src/types.ts (1)
12-12: LGTM!The optional
keyImagefield is a clean, backward-compatible addition that aligns with the PR's objective to support key images for discourse node types.apps/obsidian/src/utils/loadImage.ts (1)
11-40: LGTM!The promise implementation correctly handles:
- Single-resolution semantics via the
resolvedflag- 10-second timeout for stuck image loads
- Both success and error paths
- Cleanup of the timeout on resolution
apps/obsidian/src/components/canvas/shapes/discourseNodeShapeUtils.ts (1)
29-81: LGTM!The implementation correctly:
- Extracts image references from both Markdown and wiki-style embeds
- Normalizes link targets by handling titles, angle brackets, aliases, and fragments
- Distinguishes between external URLs and internal vault paths
- Validates image file extensions case-insensitively
- Includes error handling with appropriate logging
apps/obsidian/src/utils/measureNodeText.ts (1)
25-73: LGTM!The DOM-based measurement approach correctly:
- Creates a hidden, non-interactive container
- Matches the component's actual styling (padding, borders, flex layout)
- Applies proper width constraints from constants
- Measures using
getBoundingClientRect()- Cleans up the temporary DOM element after measurement
apps/obsidian/src/components/canvas/ExistingNodeSearch.tsx (3)
58-74: LGTM!The image preloading logic correctly:
- Determines node type from frontmatter
- Only preloads when
keyImageis enabled for the node type- Handles errors gracefully with console warnings
- Maintains resilience by not blocking shape creation on image load failure
76-82: LGTM!The dynamic size calculation using
calcDiscourseNodeSizereplaces the previous hardcoded dimensions (200×100) with accurate measurements based on actual content and optional images.
106-106: No issues found. The dependency array change is correct and safe.The
handleSelectcallback accesses bothplugin.app(lines 54, 58, 66) andplugin.settings(line 71). Includingpluginin the dependency array instead of justplugin.appproperly tracks all accessed properties. Sincepluginis a stable prop reference, there are no stale closure or missed re-render risks. This approach is idiomatic React—including the parent object when accessing its properties.apps/obsidian/src/utils/calcDiscourseNodeSize.ts (2)
70-74: LGTM!The error handling correctly falls back to text-only dimensions when image loading fails, ensuring resilience and preventing shape creation failures due to image issues.
51-64: Correct the padding value in your verification note; p-2 is 8px, not 16px as claimed.The review comment contains an inaccurate claim: Tailwind's
p-2class equals 0.5rem or 8px by default, not 16px. The codebase confirms this—no custom spacing theme is configured in either apps/obsidian/tailwind.config.ts or the shared packages/tailwind-config/tailwind.config.ts.However, the core concern about padding mismatch is valid: BASE_PADDING is 20px while the container uses p-2 (8px). This 20px vs 8px discrepancy could affect the
effectiveWidthcalculation, which influences image dimension constraints. The calculation assumes BASE_PADDING represents usable horizontal space, but the container's actual internal padding is only 8px per side.Verify whether this mismatch causes layout issues in practice—specifically, whether images with
w-fullrender correctly when constrained by theeffectiveWidthcalculation.apps/obsidian/src/components/canvas/shapes/DiscourseNodeShape.tsx (4)
66-74: LGTM!The resize implementation correctly:
- Unlocks aspect ratio to allow flexible resizing
- Enables manual resizing via
canResize- Uses tldraw's
resizeBoxutility for proper resize handling
178-231: Consider potential conflicts between manual resizing and auto-sizing.The
useEffectrecalculates and updates node dimensions whendidImageChangeis true (lines 209-230). However, if a user manually resizes the node and then the image changes (e.g., by updating the file content), the manual resize will be overridden.While the comment on line 243 mentions avoiding "fighting manual resizing," the current implementation still updates dimensions when the image changes. Consider whether this behavior is intentional or if you should preserve manual size adjustments after initial image load.
The 1px tolerance check (lines 218-219) helps avoid unnecessary updates, but verify that this behavior aligns with the expected user experience when:
- A user manually resizes a node
- The underlying file's image changes
If manual resizes should be preserved, consider tracking whether the user has manually adjusted the size and skip auto-sizing in that case.
243-256: LGTM!The dependency array correctly includes all external values used within the effect, and the eslint-disable comment appropriately documents why dimensions (
shape.props.wandshape.props.h) are excluded to avoid fighting manual resizing.
267-277: LGTM!The conditional image rendering correctly:
- Only renders when
imageSrcis present- Uses lazy loading and async decoding for performance
- Disables dragging to prevent interference with canvas interactions
- Uses
object-containto preserve aspect ratio within the containerapps/obsidian/src/components/NodeTypeSettings.tsx (4)
119-126: LGTM!The
keyImagefield configuration correctly:
- Uses the
"boolean"type for the checkbox UI- Provides clear label and description
- Sets
required: falseappropriately for an optional feature- Omits validation since boolean fields don't need custom validation
131-143: LGTM!The
BooleanFieldcomponent is correctly implemented as a controlled checkbox with proper type safety. The!!valuecoercion ensures boolean conversion, and the event handler correctly extracts the checked state.
320-332: LGTM!The update correctly:
- Accepts both
stringandbooleanvalues- Only validates string values (since boolean fields don't require validation)
- Maintains type safety through the conditional check
458-499: LGTM!The
renderFieldfunction correctly:
- Handles the union type
string | booleanfor field values- Uses appropriate type assertions for each field type
- Branches correctly between template, color, boolean, and text fields
- Passes properly typed values to each specialized component
apps/obsidian/src/components/canvas/shapes/nodeConstants.ts (2)
1-8: LGTM! Clear documentation and sensible width constraints.The file header clearly explains the purpose, and the width constants provide reasonable bounds for node rendering.
13-28: LGTM! Font and layout constants are well-defined.The font sizes correctly match Tailwind classes, the system font stack is appropriate, and the image-related constants provide reasonable constraints. The IMAGE_GAP of 4px keeps the layout compact, which appears intentional.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I would suggest not using literal values for the measureNodeText, but other than that, LGTM 👍
https://www.loom.com/share/11478ad987614ae4bdee0c0c61504fa8
Old PR comments can be found here: #490
Summary by CodeRabbit