Conversation
WalkthroughAdds a full Roam extension that renders draggable, resizable sticky-note widgets on a full-page overlay. Implements UI and CSS injection, per-note state persisted to Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 122-131: The calls to window.roamAlphaAPI.updateBlock and
window.roamAlphaAPI.deleteBlock (used in the functions returning { titleUid,
titleText } and other places) are currently not awaited or error-handled; modify
those call sites to either await the Promise (making the enclosing function
async if it isn't) or append a .catch(...) to handle and log errors so failures
don't become unhandled rejections and the UI stays consistent; ensure you
consistently handle the Promise for updateBlock and deleteBlock across all
occurrences (e.g., in the functions that create/update/delete notes) and
surface/log any errors via the existing logger or console.
- Around line 632-635: The concise arrow functions passed to
resizeObservers.forEach and blockUnmounts.forEach implicitly return a value;
replace them with block-bodied arrow functions to make the intent explicit
(e.g., change the callbacks used in resizeObservers.forEach((observer) =>
observer.disconnect()) and blockUnmounts.forEach((unmount) => unmount()) to
block bodies that call the methods without returning anything, e.g. (observer)
=> { observer.disconnect(); } and (unmount) => { unmount(); }).
- Around line 560-568: The overlay container created as container (id
"roamjs-sticky-note-container") is missing a z-index, which can cause child
notes (even with NOTE_CLASS z-index) to be painted beneath other fixed elements;
set a high z-index on the container (e.g., container.style.zIndex =
"<high-value>") so it creates a stacking context above Roam UI, and then remove
or limit the z-index rule from the .NOTE_CLASS CSS (keep z-index only for
ordering among notes inside the container).
- Around line 324-333: The onPointerMove handler is persisting layouts on every
pointer move via updateLayout -> setLayouts causing sync localStorage writes and
jank; change behavior so onPointerMove only updates DOM position
(note.style.left/top and in-memory layout values) and move the call to persist
(updateLayout/setLayouts) into onPointerUp (or schedule it once via
requestAnimationFrame) so persistence happens once per drag end; additionally,
wrap the ResizeObserver callback persistence in a debounced function (e.g.,
200–300ms) so setLayouts is not called on every resize event but only after
resizing settles.
🧹 Nitpick comments (3)
src/index.ts (3)
73-78: Datalog query uses string interpolation forPAGE_TITLE— safe here, but fragile if the constant changes.
PAGE_TITLEis a hardcoded constant without special characters, so injection isn't a current risk. However,fetchStickyNoteUids(line 92) andgetPageUid(line 75) both embed the title directly in the query string. IfPAGE_TITLEever contains quotes or special Datalog characters, these queries will break or misbehave. Consider using:in $parameterized inputs as done infetchBlockText(line 103) for consistency.
233-243: Canvas-based text measurement creates a new canvas element on every keystroke.
measureTitleWidthallocates a<canvas>and a 2D context each time it runs. For a title input'sinputevent this is moderately expensive. Consider caching the canvas/context in the closure.Proposed fix
+ const measureCanvas = document.createElement("canvas"); + const measureCtx = measureCanvas.getContext("2d"); const measureTitleWidth = (value: string): number => { const text = value.trim() || "Sticky Note"; - const canvas = document.createElement("canvas"); - const ctx = canvas.getContext("2d"); - if (!ctx) { + if (!measureCtx) { return Math.max(36, text.length * 8); } const computed = window.getComputedStyle(title); - ctx.font = `${computed.fontWeight} ${computed.fontSize} ${computed.fontFamily}`; - return Math.max(36, Math.ceil(ctx.measureText(text).width) + 10); + measureCtx.font = `${computed.fontWeight} ${computed.fontSize} ${computed.fontFamily}`; + return Math.max(36, Math.ceil(measureCtx.measureText(text).width) + 10); };
282-297:MutationObserveron the embedded block container fires on every subtree change to hide the root title.This observer runs
hideEmbeddedRootTitle()on every DOM mutation within the block container (which can be very frequent during editing). The CSS rule at lines 522–524 (.roamjs-sticky-note__embedded-root > .roam-block-container > .rm-block-main { display: none; }) already hides the root block. Consider whether the JS-basedMutationObserverapproach (lines 282–292) is still necessary, or if the CSS rule alone suffices — removing it would eliminate unnecessary DOM thrashing.
…escription and keywords for better clarity
…functionality, and design principles
…ction. Set container z-index to a higher value for better visibility.
…imize performance and maintain clean state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 747-776: The function createStickyNote may persist the parent
block and layout before the child content block is created, leaving dangling
state if the second createBlock throws; wrap the sequence of createBlock calls
and subsequent layout assignment/DOM creation in a try/catch so failures roll
back persisted state: create the parent and child blocks first, then compute and
assign layouts[uid] and call setLayouts and
createStickyNoteElement/container.append; on catch, remove any partial state by
deleting the parent block (call deleteBlock(uid) or your rollback API if
available) and removing layouts[uid] then call setLayouts, and rethrow or
surface the error; ensure focusStickyRenderedUidWithRetries is only called after
successful creation.
🧹 Nitpick comments (2)
src/index.ts (2)
268-293: Pending timers infocusStickyRenderedUidWithRetriesare not cancelled on unload.If the extension unloads while retry timers are still pending, the callbacks will fire on potentially removed DOM elements. Consider returning a cancel function and invoking it during cleanup — or at minimum, guard the callback against a missing container.
Suggested fix: return a cancellation handle
const focusStickyRenderedUidWithRetries = ({ uid, root, }: { uid: string; root: ParentNode; -}): void => { +}): (() => void) => { const timerIds: number[] = []; let focused = false; [60, 140, 280, 520, 900].forEach((delay) => { const timerId = window.setTimeout(() => { if (focused) { return; } focused = focusStickyRenderedUid({ uid, root }); if (focused) { timerIds.forEach((id) => { if (id !== timerId) { window.clearTimeout(id); } }); } }, delay); timerIds.push(timerId); }); + return () => { + timerIds.forEach((id) => window.clearTimeout(id)); + }; };
534-547: Debug utilities attached towindowshould be gated behind a development check.Exposing
roamjsStickyNoteDebugon the global window in production gives users access to internal focus/ID-resolution helpers. Consider gating this behind aprocess.env.NODE_ENV !== "production"check, or removing it entirely if it was only needed during development.
…oper rollback on failure and improve stability.
mdroidian
deleted the
codex/create-roam-research-sticky-note-extension
branch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 780-796: The catch block in createStickyNote currently rethrows
the error after performing rollback (calling window.roamAlphaAPI.deleteBlock,
logRoamMutationError, delete layouts and setLayouts), which can create an
unhandled promise rejection when createStickyNote is used as a command palette
callback; remove the final "throw error" and instead return (or resolve) after
logging/rollback so the promise completes gracefully; ensure the rollback steps
(deleteBlock + logRoamMutationError and setLayouts) remain unchanged and that
createStickyNote returns normally from the catch path.
🧹 Nitpick comments (2)
src/index.ts (2)
295-531:createStickyNoteElementis a 230+ line function — consider extracting sub-concerns.The function handles header construction, title measurement, drag logic, resize observation, block mounting, minimize toggling, and delete with rollback. Each of these could be a small helper, improving readability and testability. Not blocking, but worth considering for future maintainability.
564-705: Large CSS string injected inline — consider moving to a.cssfile.A ~140-line template literal of CSS is harder to lint, syntax-check, and maintain than a dedicated stylesheet. If the build tooling supports it, importing a
.cssasset would give you editor highlighting, auto-complete, and linting for free.
| } catch (error) { | ||
| if (uid) { | ||
| try { | ||
| await window.roamAlphaAPI.deleteBlock({ block: { uid } }); | ||
| } catch (rollbackError) { | ||
| logRoamMutationError({ | ||
| operation: "deleteBlock", | ||
| uid, | ||
| error: rollbackError, | ||
| }); | ||
| } | ||
| delete layouts[uid]; | ||
| setLayouts(layouts); | ||
| } | ||
| console.error("[sticky-note] Failed to create sticky note", error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Re-throwing after rollback may produce an unhandled promise rejection.
createStickyNote is registered as the command palette callback (line 801). If the catch block fires, you log the error, roll back, and then throw error — but the command palette is unlikely to attach a .catch() to the returned promise, so this becomes an unhandled rejection at the top level.
Either remove the throw (the error is already logged and rolled back) or wrap the callback invocation with its own .catch().
Suggested fix
console.error("[sticky-note] Failed to create sticky note", error);
- throw error;
}🤖 Prompt for AI Agents
In `@src/index.ts` around lines 780 - 796, The catch block in createStickyNote
currently rethrows the error after performing rollback (calling
window.roamAlphaAPI.deleteBlock, logRoamMutationError, delete layouts and
setLayouts), which can create an unhandled promise rejection when
createStickyNote is used as a command palette callback; remove the final "throw
error" and instead return (or resolve) after logging/rollback so the promise
completes gracefully; ensure the rollback steps (deleteBlock +
logRoamMutationError and setLayouts) remain unchanged and that createStickyNote
returns normally from the catch path.
1 participant
Motivation
Roam/js/sticky-note.Create Sticky Notecommand and keep layout state across sessions.Description
STORAGE_KEYandStickyNoteLayoutsstored/loaded withgetLayouts/setLayouts.createPageforRoam/js/sticky-noteand create new note blocks withcreateBlock, and enumerate existing notes viawindow.roamAlphaAPI.q.window.roamAlphaAPI.ui.components.renderBlock, supports minimize/delete actions, and updates persisted layout on drag/resize using pointer events and aResizeObserver.addStyle, create a fixed container for notes, and register theCreate Sticky Notecommand in the command palette; include cleanup via the extensionunloadreturn value.Testing
Codex Task
Summary by CodeRabbit
New Features
Documentation
Chores