Conversation
…y. Refactor GiphyPopover component for improved GIF selection and insertion functionality.
… Introduce directional index calculation for arrow key navigation and adjust layout to use flexbox for better responsiveness.
|
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. WalkthroughReplaces the previous inline GiphyPopover with a Dialog-based GiphyOverlay and exposes new public APIs ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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🧪 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: 6
🤖 Fix all issues with AI agents
In `@src/components/GiphyPopover.tsx`:
- Around line 301-332: The Card components inside the conditional renders (the
isLoading block using Spinner, the error block with Button that calls setNonce,
and the empty state checking gifs) include a style with gridColumn: "1 / -1"
which is ignored because the parent container uses display: "flex"; remove the
unused gridColumn entries from the Card style objects (or alternatively convert
the parent container to display: "grid" if you intentionally want grid layout)
so that the Card style is accurate—update the Card elements referenced in the
isLoading, error, and no-results branches accordingly.
- Line 25: The Giphy API key is hardcoded when constructing GiphyFetch in
GiphyPopover (const gf = new GiphyFetch(...)); change this to read the key from
a runtime-config or environment variable (e.g.,
process.env.REACT_APP_GIPHY_API_KEY or a provided config getter) and fail loudly
if not present; update the GiphyPopover code to construct new GiphyFetch(apiKey)
using that injected value and add a clear error/log when the key is missing so
builds/CI and runtime surface the misconfiguration.
- Around line 421-428: openGiphyPicker races when controller is null: it sets
pendingOpen and also schedules a setTimeout that will call
controller.open(context), causing a double-open when the React effect later
consumes pendingOpen; fix by making only one path trigger the open — either
clear pendingOpen inside the timeout before calling controller.open(context) or
skip scheduling the timeout when you set pendingOpen. Update the openGiphyPicker
implementation (and references to controller, pendingOpen, initGiphyOverlay) so
that if you assign pendingOpen = context you do not also leave a timeout that
will call controller.open(context), or ensure the timeout first checks and
clears pendingOpen to avoid duplicate opens.
- Around line 97-119: The insertGif useCallback (insertGif) uses
gif.images.original.url without guarding for missing/empty URLs; update
insertGif to safely resolve a fallback image URL (e.g., check
gif.images.original.url then gif.images.downsized?.url,
gif.images.fixed_height?.url, gif.images.preview_gif?.url, etc.) and use the
first non-empty url for gifMarkdown, or setError/abort insertion if no valid URL
is found; keep the same updateBlock logic but reference the resolvedUrl variable
instead of gif.images.original.url so insertion matches the defensive preview
rendering.
- Around line 202-240: The effect currently fires on every keystroke (dependency
on search) causing many GF API calls; debounce the search input (300–500ms)
before making gf.search/gf.trending calls: create a debouncedSearch (via a small
timeout in this component or a useDebounce hook) and use debouncedSearch in the
effect instead of search, ensure you clear the timeout on cleanup, preserve the
existing requestRef requestId logic and isOpen/nonce checks, and update the
effect dependencies to [isOpen, nonce, debouncedSearch] so only the debounced
value triggers API requests.
In `@src/index.ts`:
- Line 91: The code silently treats a valid cursor position 0 as falsy by using
the || fallback; change the calculation of cursorStart in the handler that uses
textarea?.selectionStart to use nullish coalescing so only null/undefined fall
back to currentValue.length (update the expression that assigns cursorStart
which references textarea?.selectionStart and currentValue to use ?? instead of
|| to preserve a 0 index).
🧹 Nitpick comments (7)
src/components/GiphyPopover.tsx (3)
258-258: PointlessuseMemowrapping a constant.
useMemo(() => 1400, [])is equivalent toconst pickerWidth = 1400. The memo adds overhead with no benefit for a static value.- const pickerWidth = useMemo(() => 1400, []); + const pickerWidth = 1400;
283-285:onKeyPressis deprecated in React.React's
onKeyPresshas been deprecated since React 17 and may be removed in a future version.onKeyDown(which you already use) is the recommended replacement. Since you're only callinge.stopPropagation(), you can safely removeonKeyPressifonKeyDownalready handles propagation.
411-418: No teardown exposed — potential memory leak on extension unload.
initGiphyOverlaymounts a React tree into the DOM but provides no way to unmount it. If the extension can be unloaded/reloaded (common in Roam extensions), the old root stays in the DOM and leaks. Consider returning a cleanup function or exporting adestroyGiphyOverlay.src/index.ts (4)
25-46: Second regex (/gif\s*$) is only reachable with trailing spaces after/gif.The first regex
/\/[^\s/]*$/already matches/gif(and any/word) when there's no trailing whitespace. The second regex/\/gif\s*$/only kicks in when there are trailing spaces specifically after/gif. Other slash fragments with trailing spaces (e.g.,/giphy) won't be caught by either regex.This may be intentional, but a brief comment explaining the two-pass logic would help future readers.
71-80: Fragile type cast onroamAlphaAPI.ui.The inline cast is brittle — if Roam's API shape changes, TypeScript won't catch the breakage. Consider declaring a small augmentation interface or using a runtime check.
Suggested approach
- ( - window.roamAlphaAPI.ui as { - slashCommand?: { - addCommand: (command: { - label: string; - callback: (...args: unknown[]) => void; - }) => void; - }; - } - ).slashCommand?.addCommand({ + const ui = window.roamAlphaAPI.ui as Record<string, unknown>; + const slashCommand = ui?.slashCommand as + | { addCommand: (cmd: { label: string; callback: (...args: unknown[]) => void }) => void } + | undefined; + slashCommand?.addCommand({Or better, declare a module augmentation for
roamAlphaAPItypes.
109-125: Async persistence may double-clean if Roam already applied the textarea change.The flow: (1) mutate
textarea.valuesynchronously, (2) wait for Roam to persist that text, (3) re-run the cleaning function on the persisted text. If Roam has already saved the cleaned text (from thetextarea.valuemutation +inputevent),waitForBlockTextlooking forcurrentValue(the uncleaned text) will time out,getTextByBlockUidwill return the already-cleaned text, and then cleaning runs again on the already-clean string. In that case the second clean is a no-op (no slash fragment found), so it's safe — but theupdateBlockcall is still made unnecessarily.Consider short-circuiting if
finalText === baseText:+ if (finalText === baseText) return; return updateBlock({ uid: targetUid, text: finalText }).catch((e) =>
68-69: No cleanup on extension unload.
runExtensiontypically returns a cleanup/teardown callback. NeitherinitGiphyOverlaynoraddCommandis cleaned up, which means reloading the extension will leak DOM nodes and register duplicate slash commands.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ents, enhancing layout consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 112-124: The fallback uses a stale cursorStart against freshly
fetched baseText which can corrupt the block; when matched is false (after
waitForBlockText/getTextByBlockUid) re-derive a cursor offset from baseText
instead of reusing cursorStart: locate the slash fragment or delimiter in
baseText (e.g. search for the nearest "/" fragment that corresponds to the
portion of currentValue around cursorStart or find the last matching fragment
substring) and compute cursorStartFromBase, then call
removeSlashFragmentFromCursor(baseText, cursorStartFromBase) (fall back to
original cursorStart only if no matching fragment is found); update the branch
that currently calls removeSlashFragmentFromCursor(baseText, cursorStart) to use
this re-derived cursorStartFromBase and keep indexTuple handling unchanged.
- Around line 68-138: The slash command and overlay are never torn down; modify
the runExtension callback to return a cleanup function that unregisters the
slash command and removes the DOM overlay: when calling (window.roamAlphaAPI.ui
as ...).slashCommand?.addCommand(...) capture the returned handle/ID (or the
function it returns) from addCommand and call the corresponding
unregister/remove method inside the cleanup; add a teardown export (e.g.,
teardownGiphyOverlay or cleanupGiphyOverlay) in GiphyPopover.tsx that removes
the overlay DOM and any listeners and import & call it from the cleanup; ensure
the cleanup runs on extension unload by returning it from runExtension so Roam
will invoke it.
🧹 Nitpick comments (3)
src/index.ts (3)
90-90:textarea?.value ||silently treats empty string as missing.Same class of bug that was fixed on line 91 (
??instead of||). If the textarea exists but is empty,||skips togetTextByBlockUid, which happens to return""too — so it's benign today. However, using??would be consistent and future-proof.- const currentValue = textarea?.value || (targetUid ? getTextByBlockUid(targetUid) : ""); + const currentValue = textarea?.value ?? (targetUid ? getTextByBlockUid(targetUid) : "");
25-46: Second regex branch (/gif\s*$) is unreachable for most inputs.The first regex
/\/[^\s/]*$/i(line 29) already matches any trailing slash fragment without spaces (including/gif). The second regex/gif\s*$/i(line 37) only adds coverage for/giffollowed by trailing whitespace. However, a more general pattern like/\/[^\s/]*\s*$/ion the first branch would cover both cases and eliminate the need for a separate/gif-specific fallback.This isn't a bug, but the
/gif-specific regex looks like it was meant to be more general. If other slash command names with trailing spaces need handling, this won't catch them.
71-80: Inline type assertion onroamAlphaAPI.uiis fragile.Casting the entire
roamAlphaAPI.uito an anonymous type to accessslashCommandworks, but it will silently swallow type errors if the API shape changes. Consider augmenting theWindowinterface or using a dedicated type declaration for the Roam API extensions to get compile-time safety.
…GiphyPopover component behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/GiphyPopover.tsx`:
- Around line 110-113: The call to window.roamAlphaAPI.updateBlock inside
insertGif is not error-handled and setIsOpen(false) runs unconditionally; wrap
the updateBlock call in a try/catch within insertGif (or the helper that
performs the update) and only call setIsOpen(false) after a successful await; on
catch, log the error (or surface it to the UI) and avoid closing the dialog so
the user can retry. Ensure you reference the existing insertGif function and the
updateBlock call so the try/catch surrounds that await and controls the
setIsOpen(false) invocation.
- Around line 412-420: The initGiphyOverlay routine creates rootEl and mounts
<GiphyOverlay /> with ReactDOM.render but never unmounts or removes the DOM
node; add a cleanup function (e.g., export const cleanupGiphyOverlay) that calls
ReactDOM.unmountComponentAtNode(rootEl), removes rootEl from document.body, and
sets rootEl = null to allow reinitialization; then update the extension
bootstrap (the runExtension call in src/index.ts) to return that cleanup
function so it is invoked on unload. Ensure references to initGiphyOverlay,
rootEl, GiphyOverlay, and the new cleanup function are used consistently.
🧹 Nitpick comments (2)
src/components/GiphyPopover.tsx (2)
259-259:useMemowrapping a constant is unnecessary.
pickerWidthalways returns1400with an empty dependency array. A plainconstsuffices.- const pickerWidth = useMemo(() => 1400, []); + const pickerWidth = 1400;
284-286: ReplaceonKeyPresswithonKeyDown.
onKeyPressis marked as deprecated in@types/reactwith guidance to useonKeyDowninstead. SinceonKeyDownalready covers all key interactions here, the redundantonKeyPresshandler with juststopPropagationcan be safely removed or consolidated into the existingonKeyDownlogic.
…ing a dedicated vertical grid index calculation for arrow key navigation, improving user experience.
…dback and cleanup teardown function
…gistration and teardown functionality. Enhance GiphyPopover by introducing a dedicated command API for better management of GIF insertion.
…esources. This includes unmounting the component and resetting relevant state variables.
…GIF loading. Update state management to use debouncedSearch instead of immediate search input.
…onsole and display user-friendly message on failure.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| window.setTimeout(async () => { | ||
| const matched = await waitForBlockText({ | ||
| uid: targetUid, | ||
| text: currentValue, | ||
| }); | ||
| const baseText = matched ? currentValue : getTextByBlockUid(targetUid); | ||
| const finalText = indexTuple | ||
| ? removeSlashFragmentFromIndexes(baseText, indexTuple).cleaned | ||
| : removeSlashFragmentFromCursor(baseText, cursorStart).cleaned; | ||
| return updateBlock({ uid: targetUid, text: finalText }).catch((e) => | ||
| console.error("[giphy:/gif] persisted update failed", e) | ||
| ); | ||
| }, 0); |
There was a problem hiding this comment.
🔴 Double-cleaning with stale indexes causes data loss when Roam persists textarea update before waitForBlockText
When the slash command fires with indexes provided, the code immediately updates the textarea to the cleaned value (line 115) and dispatches an input event (line 118). Then in a setTimeout, it calls waitForBlockText waiting for the block text to equal currentValue (the original uncleaned text). If Roam processes the textarea input event and persists the cleaned text before waitForBlockText starts polling, the wait times out (matched = false), and baseText becomes the already-cleaned (shorter) text. Then removeSlashFragmentFromIndexes(baseText, indexTuple) applies the original indexes to the shorter text, removing the wrong characters.
Concrete example showing data loss
Original text: hello /Giphy: Insert GIF world
indexes from Roam: [7, 27]
First clean (line 109-110): from=6, to=27 → cleaned = hello world, insertAt=6
Textarea is updated to hello world at line 115, input event dispatched at line 118.
Roam persists hello world to the block.
setTimeout fires (line 123):
waitForBlockTextpolls forhello /Giphy: Insert GIF worldbut findshello world→ times out,matched = falsebaseText = getTextByBlockUid(targetUid)=hello world(length 11)removeSlashFragmentFromIndexes("hello world", [7, 27]):from = Math.max(0, 6) = 6,to = Math.min(11, 27) = 11cleaned = "hello world".slice(0, 6) + "hello world".slice(11)="hello "—worldis deleted!updateBlockpersists"hello "— data loss
Impact: Text after the slash command insertion point gets silently deleted when Roam processes the textarea update before the polling loop detects the original text.
Prompt for agents
The root cause is that the code tries to clean the slash fragment twice: once immediately via textarea manipulation, and once via the persisted updateBlock. When the textarea update causes Roam to persist the cleaned text before waitForBlockText finds the original text, the second cleaning operates on already-cleaned text with stale indexes, removing wrong characters.
Fix approach: Instead of re-computing the cleaning in the setTimeout, simply persist the already-computed `updatedValue` directly. Replace lines 123-135 in src/index.ts with:
window.setTimeout(() => {
updateBlock({ uid: targetUid, text: updatedValue }).catch((e) =>
console.error("[giphy:/gif] persisted update failed", e)
);
}, 0);
This avoids the race condition entirely by using the already-computed cleaned text rather than trying to re-derive it from potentially stale state.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit