ENG-519 Make human-readable labels for node ids - #1271
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| render: (containerEl: HTMLElement, data: unknown) => { | ||
| const nodeTypeId = typeof data === "string" ? data : String(data ?? ""); | ||
| const nodeType = getNodeTypeById(plugin, nodeTypeId); | ||
|
|
||
| const el = containerEl.createSpan({ | ||
| cls: "dg-node-type-id-value", | ||
| text: nodeType?.name ?? nodeTypeId, | ||
| }); | ||
| el.setAttr("title", nodeTypeId); |
There was a problem hiding this comment.
🔴 Node type property shows a meaningless placeholder instead of the type name
The value handed to the property display is converted straight to text (String(data ?? "") at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:27) even though it arrives as a wrapper object rather than plain text, so the property shows [object Object] instead of the readable node type name.
Impact: Every note's node type property in the properties panel can display an unreadable placeholder instead of the human-readable label the feature is meant to add.
Widget render callback receives a property entry object, not the raw value
Obsidian's internal property widget contract (as typed by obsidian-typings) is render(containerEl, data: PropertyEntryData<T>, ctx), where PropertyEntryData is { key, type, value }. The callback here declares data: unknown (which is bivariantly assignable, so the type checker does not catch it) and then treats data as if it were the raw string value. Since the object is not a string, typeof data === "string" is false and String(data) yields "[object Object]"; getNodeTypeById(plugin, "[object Object]") returns undefined, so the fallback text (and the title attribute at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:34) is that placeholder. The fix is to read data.value (guarding for non-string) before looking up the node type.
Prompt for agents
In apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts the widget's render callback assumes its second argument is the raw frontmatter value. Obsidian's property widget API passes a property entry object shaped like { key, type, value }. Because the parameter is declared as `unknown`, TypeScript does not flag the mismatch, and at runtime `String(data)` produces "[object Object]", so the widget shows that placeholder instead of the node type name (and also as the tooltip). Update the render implementation to extract the value field (defensively handling both a raw string and the entry object shape) before calling getNodeTypeById, and consider verifying against obsidian-typings' PropertyEntryData/PropertyRenderContext types instead of `unknown`.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ( | ||
| metadataTypeManager.getAssignedWidget(NODE_TYPE_ID_PROPERTY_KEY) === | ||
| WIDGET_TYPE | ||
| ) { | ||
| void metadataTypeManager.unsetType(NODE_TYPE_ID_PROPERTY_KEY); | ||
| } |
There was a problem hiding this comment.
🟡 Property type assignment is left behind in the vault when the plugin is disabled
The check that decides whether to remove the custom display setting compares a widget object against a plain name (getAssignedWidget(...) === WIDGET_TYPE at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:63-65), which never matches, so the setting is never removed and stays in the vault after the plugin is turned off.
Impact: After disabling or uninstalling the plugin, the node type property keeps pointing at a display style that no longer exists, so it may render oddly until the user fixes it manually.
getAssignedWidget returns the widget object, not the type string
Obsidian's MetadataTypeManager.getAssignedWidget(key) resolves the assigned type name and returns the corresponding entry from registeredTypeWidgets (i.e. a PropertyWidget object); the string type name is available via getAssignedType(key). Comparing the returned object to the string "dg-node-type-id" is always false, so unsetType is never invoked while delete metadataTypeManager.registeredTypeWidgets[WIDGET_TYPE] at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:68 still removes the widget, leaving the persisted assignment (in the vault's types.json, written by setType at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:52) dangling.
| if ( | |
| metadataTypeManager.getAssignedWidget(NODE_TYPE_ID_PROPERTY_KEY) === | |
| WIDGET_TYPE | |
| ) { | |
| void metadataTypeManager.unsetType(NODE_TYPE_ID_PROPERTY_KEY); | |
| } | |
| if (metadataTypeManager.getAssignedType(NODE_TYPE_ID_PROPERTY_KEY) === WIDGET_TYPE) { | |
| void metadataTypeManager.unsetType(NODE_TYPE_ID_PROPERTY_KEY); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const registerNodeTypeIdPropertyWidget = ( | ||
| plugin: DiscourseGraphPlugin, | ||
| ): void => { | ||
| const metadataTypeManager = ( | ||
| plugin.app as unknown as { metadataTypeManager: MetadataTypeManager } | ||
| ).metadataTypeManager; | ||
|
|
||
| metadataTypeManager.registeredTypeWidgets[WIDGET_TYPE] = createWidget(plugin); | ||
| void metadataTypeManager.setType(NODE_TYPE_ID_PROPERTY_KEY, WIDGET_TYPE); | ||
| }; |
There was a problem hiding this comment.
🟡 Plugin can fail to start entirely if the internal properties API is unavailable
The internal properties registry is used without first checking that it exists (metadataTypeManager.registeredTypeWidgets[WIDGET_TYPE] = ... at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:51), so if that unofficial capability is missing the startup step throws and the rest of the plugin never initializes.
Impact: On an Obsidian version without this internal capability, the whole plugin would fail to load instead of just losing the nicer property label.
Unguarded internal API access during onload
registerNodeTypeIdPropertyWidget(this) is called in onload at apps/obsidian/src/index.ts:75 without a try/catch, unlike neighbouring risky initializations (FileChangeListener at apps/obsidian/src/index.ts:86-92, TagNodeHandler at apps/obsidian/src/index.ts:167-173). The cast plugin.app as unknown as { metadataTypeManager: MetadataTypeManager } asserts existence; if app.metadataTypeManager is undefined (or setType/registeredTypeWidgets change shape), a TypeError propagates out of onload, skipping registerCommands, settings tab, and view registrations. This contradicts the module comment at apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts:12-18 claiming graceful degradation. Same unguarded access exists in unregisterNodeTypeIdPropertyWidget.
Prompt for agents
apps/obsidian/src/utils/nodeTypeIdPropertyWidget.ts accesses the unofficial app.metadataTypeManager API via a type assertion and immediately mutates registeredTypeWidgets and calls setType. The module comment promises graceful degradation if Obsidian drops support, but there is no runtime guard, and registerNodeTypeIdPropertyWidget is called unguarded from onload (apps/obsidian/src/index.ts:75), so a missing/renamed API would throw and abort plugin initialization. Add defensive checks (verify the manager and the methods/properties exist, or wrap in try/catch with a console warning) in both register and unregister helpers so the feature silently no-ops.
Was this helpful? React with 👍 or 👎 to provide feedback.
b761a64 to
4956cbc
Compare
1 participant
https://linear.app/discourse-graphs/issue/ENG-519/make-human-readable-labels-for-node-ids