Add/Update renderString type #55
Conversation
WalkthroughThe PR updates TypeScript declarations in src/types/index.ts: renderBlock, renderPage, and renderSearch now return Promise instead of null; renderString and unmountNode methods are added returning Promise; argument shapes are unchanged. package.json version is bumped from 0.85.7 to 0.86.0. No other public API shapes were modified. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In @src/types/index.ts:
- Around line 274-277: The type for renderString is inconsistent with its
siblings: change the signature of renderString from returning Promise<null> to
returning null (i.e., renderString: (args: { string: string; el: HTMLElement; })
=> null) to match renderBlock, renderPage, and renderSearch unless you
intentionally require async behavior—if async is required, update the other
render* signatures and add a comment documenting that renderString is
intentionally async to reflect the external Roam Alpha API.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/types/index.ts:
- Around line 266-273: The renderSearch type is asynchronous (returns
Promise<null>) but callers may not realize they must await it; update the type
declaration for renderSearch and its public API docs by adding a concise
JSDoc/typed comment on the renderSearch signature indicating it must be awaited
(or chained with .then()) and what the resolved value represents, and consider
changing Promise<null> to Promise<void> for clarity (update any call sites/tests
that assume synchronous behavior). Ensure you reference the renderSearch
function in the types and runtime docs so external consumers know to await its
completion.
- Around line 261-265: The createPage then-callback calls
window.roamAlphaAPI.ui.components.renderPage(...) but does not await its
now-Promise-returning signature; make the then callback async (or use an async
function) and await renderPage (window.roamAlphaAPI.ui.components.renderPage({
uid: parentUid, el, "hide-mentions?": true })) before calling if (autoFocus)
realFocus(); update FormDialog.tsx so renderPage is awaited to ensure
realFocus() runs after rendering completes.
🧹 Nitpick comments (2)
src/types/index.ts (2)
274-277: Consistency achieved; past review concern resolved.The previous review flagged
renderStringas inconsistent because it returnedPromise<null>whilerenderBlock,renderPage, andrenderSearchreturnednull(sync). Now all render methods consistently returnPromise<null>, which resolves that concern.However, consider using
Promise<void>instead ofPromise<null>to align with TypeScript conventions for async operations without meaningful return values.📝 Suggested refactor for idiomatic TypeScript
Apply this pattern to all render methods (renderBlock, renderPage, renderSearch, renderString):
- renderString: (args: { - string: string; - el: HTMLElement; - }) => Promise<null>; + renderString: (args: { + string: string; + el: HTMLElement; + }) => Promise<void>;
278-278: NewunmountNodemethod looks appropriate.The signature is reasonable for an unmount operation. As with the render methods, consider using
Promise<void>instead ofPromise<null>for consistency with TypeScript conventions.📝 Suggested refactor
- unmountNode: (args: { el: HTMLElement }) => Promise<null>; + unmountNode: (args: { el: HTMLElement }) => Promise<void>;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/types/index.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (1)
src/types/index.ts (1)
255-260: Return type is correct; do not change toPromise<void>.The current
Promise<null>return type matches the actual Roam Alpha API behavior, as empirically verified against the live API. Changing this toPromise<void>would create a type mismatch with reality and should not be done.While the shift to async methods (if previously sync) is a breaking change for external consumers, the type definitions are accurate as-is.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Breaking Changes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.