Conversation
- Introduced OnloadArgs type for better extension settings handling. - Updated getLayouts and setLayouts functions to utilize extension settings API. - Enhanced layout normalization and ensured consistent formatting. - Improved event handling for sticky note dragging, ensuring layout persistence on drag end. - Refactored createStickyNoteElement to streamline layout updates and interactions.
- Updated getPageUid to use the pull method for fetching the page UID, enhancing efficiency. - Simplified the return logic to handle null cases more gracefully.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughLayout persistence was moved from localStorage to the extension settings API. getLayouts now reads and normalizes layouts from extensionSettings (returning an empty object for missing/invalid data). setLayouts writes layouts to extensionSettings. createStickyNoteElement’s public signature was expanded to accept persistLayouts and readLayoutsSetting; initialization was updated to derive layouts via getLayouts({ extensionSettings }) and expose a persistLayouts helper used in place of prior setLayouts calls. Minor formatting and trailing-comma adjustments were applied. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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/index.ts`:
- Around line 478-479: Remove the diagnostic console.log and the auxiliary
readLayoutsSetting parameter from the public API: delete the
console.log("[sticky-note] drag end settings", readLayoutsSetting()) call after
persistLayouts(), remove the readLayoutsSetting parameter from the
createStickyNoteElement function signature and all places that call
createStickyNoteElement (and any intermediate forwarding), and update any
references inside createStickyNoteElement that only existed to support that
debug log; ensure persistLayouts() remains called and no other behavior is
changed.
🧹 Nitpick comments (2)
src/index.ts (2)
82-88: Unsafe cast —normalizeLayoutonly validatesrotation.
JSON.parse(raw) as StickyNoteLayoutstrusts that all layout fields (x,y,width,height,minimized) are present and correctly typed. If stored data is ever partially corrupt (e.g., after a failed write or schema change), notes could render atNaN/undefinedpositions. Consider either validating all fields innormalizeLayoutor adding a guard that drops invalid entries.
467-480: Cancel the pending debounce timer on drag end to avoid a redundant persist.
stopDraggingcallspersistLayouts()immediately (line 478), but a pendingresizePersistTimeoutfromscheduleLayoutPersistence(set duringonPointerMove) may still fire ~250ms later, causing a redundant write.Proposed fix
const stopDragging = (): void => { if (!isDragging) { return; } isDragging = false; + if (resizePersistTimeout) { + window.clearTimeout(resizePersistTimeout); + resizePersistTimeout = null; + } note.classList.remove(NOTE_DRAGGING_CLASS);
…event memory leaks
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }): void => { | ||
| extensionSettings.set(STORAGE_KEY, JSON.stringify(layouts)); | ||
| }; |
There was a problem hiding this comment.
🟡 Unhandled Promise from async extensionSettings.set in setLayouts
The setLayouts function calls extensionSettings.set() without awaiting or catching the returned Promise, causing silent failures and unhandled promise rejections.
Root Cause
The old code used synchronous localStorage.setItem which would throw synchronously on failure (e.g. quota exceeded), but the new code at src/index.ts:101 calls extensionSettings.set(STORAGE_KEY, JSON.stringify(layouts)) which returns Promise<void> (per the type definition at node_modules/roamjs-components/types/native.d.ts:393: set: (k: string, v: unknown) => Promise<void>). Since the return type of setLayouts is void, this Promise is silently discarded.
This means:
- If
extensionSettings.setrejects, the error is completely lost—no logging, no user feedback. - This produces unhandled promise rejections in the runtime, which may be reported as errors in the console or could crash the extension in strict environments.
- Every callsite that invokes
persistLayouts()(lines 451, 482, 513, 558, 781, 805, 830) is affected sincepersistLayoutsat line 751 delegates directly tosetLayouts.
Impact: Layout changes (drag, resize, minimize, delete) may silently fail to persist without any indication to the user, and unhandled rejections are produced.
| }): void => { | |
| extensionSettings.set(STORAGE_KEY, JSON.stringify(layouts)); | |
| }; | |
| }): void => { | |
| void extensionSettings.set(STORAGE_KEY, JSON.stringify(layouts)).catch((err) => { | |
| console.error("[sticky-note] Failed to persist layouts", err); | |
| }); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit