WIP

[mdroidian]

---

Summary by CodeRabbit

• New Features

  • Blueprint Widgets plugin with 8 customizable inline widgets: radio buttons, sliders, switches, numeric inputs, tabs, progress bars, icons, and cards.
  • Widget settings UI with configurable access modifiers and per-widget customization.
  • Optional TODO checkbox replacement using Blueprint switches.
  • Tag styling and customization options.

• Documentation

  • Comprehensive Roam Alpha API reference guides for data access, file operations, graph view, UI commands, filters, navigation, and rendering.

[mdroidian]

@coderabbitai full review

[coderabbitai]

Walkthrough

This pull request introduces a comprehensive Blueprint Widgets extension for Roam Research. The changes add a complete widget system including eight widget types (radio, better-slider, switch, numeric-input, tabs, progress, icon, card) with configurable per-instance settings and persistent storage. The implementation includes a React-based observer engine that detects and renders widgets from custom markers within blocks, a settings management system with global and per-widget defaults, styling via CSS, utilities for parsing markers and managing block data, hashtag styling, and a TODO checkbox hijack feature. Supporting documentation, type definitions, and unit tests are also included. The package metadata is updated to reflect the new Blueprint Widgets branding and adds a dependency on @blueprintjs/core.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The PR title 'WIP' is vague and generic, using a non-descriptive abbreviation that conveys minimal meaningful information about the changeset.	Replace 'WIP' with a descriptive title summarizing the main change, such as 'Add Blueprint Widgets plugin with inline UI components and settings management' or 'Implement Blueprint UI widget system for Roam blocks'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

✅ Actions performed

Full review triggered.

[devin-ai-integration]

Devin Review found 1 potential issue.

View 9 additional findings in Devin Review.

[devin-ai-integration]

🔴 Icon :: shorthand parser destructures 2-element array into 3 variables, swapping intent and size

The parseIconParameters function incorrectly parses the name::size shorthand format. When given "icon:search::16", the payload "search::16" is split by :: producing ["search", "16"], but the code destructures this into [icon, intent, size] — a 3-element pattern. Since there are only 2 elements, intent gets "16" and size gets undefined.

Root Cause and Impact

At src/utils/markerParser.ts:77:

const [icon, intent, size] = payload.split("::").map((part) => part.trim());

For input "search::16", split("::") yields ["search", "16"]. Destructuring assigns:

• icon = "search" ✓
• intent = "16" ✗ (should be undefined)
• size = undefined ✗ (should be 16)

The test at src/__tests__/markerParser.test.ts:24-28 expects { icon: "search", intent: undefined, size: 16 } but the code actually produces { icon: "search", intent: "16", size: undefined }.

Impact: All {{icon:name::size}} shorthand markers will have their size value interpreted as an intent string (e.g., "16" as intent), and the icon will render at the default size (14px) instead of the specified size. The intent will also be set to a nonsensical value like "16", which Blueprint will ignore but is still incorrect.

Suggested change

	if (payload.includes("::")) {
	const [icon, intent, size] = payload.split("::").map((part) => part.trim());
	const parsedSize = Number(size);
	return {
	icon: icon || undefined,
	intent: intent || undefined,
	size: Number.isNaN(parsedSize) ? undefined : parsedSize,
	};
	}
	if (payload.includes("::")) {
	const parts = payload.split("::").map((part) => part.trim());
	if (parts.length === 2) {
	const parsedSize = Number(parts[1]);
	return {
	icon: parts[0] || undefined,
	intent: undefined,
	size: Number.isNaN(parsedSize) ? undefined : parsedSize,
	};
	}
	const [icon, intent, size] = parts;
	const parsedSize = Number(size);
	return {
	icon: icon || undefined,
	intent: intent || undefined,
	size: Number.isNaN(parsedSize) ? undefined : parsedSize,
	};
	}

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[coderabbitai]

Actionable comments posted: 63

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@extension.css`:
- Line 1: The file is a minified single-line CSS which makes diffs and edits
impossible; reformat extension.css into a readable, maintainable style sheet
(one selector/rule per line, preserve all rules and order) — for example expand
selectors like .bpui-inline-host, .bpui-widget-row,
.bpui-widget-settings-previews, the media query `@media` (max-width: 720px), etc.,
into multi-line rules so each property is on its own line; after reformatting,
ensure the build pipeline (or repo tooling) is used to minify for production and
consider adding a prettier/stylelint rule to prevent future checked-in minified
CSS.

In `@skills/roam-alphaapi-data/references/data.md`:
- Around line 187-232: The parameter descriptions for addPullWatch and
removePullWatch are missing; replace the {{[[TODO]]}} placeholders for the "pull
pattern" and "entity-id" parameters with clear descriptions: for "pull pattern"
explain it is a Roam-style pull query string (e.g., "[:block/children
:block/string {:block/children ...}]") that specifies which attributes/relations
to fetch and watch, and for "entity-id" explain it is a Roam entity identifier
string (either an entity reference like '[:block/uid \"<uid>\"]' or a page
reference) that scopes the watch to a specific block or page; update both
addPullWatch and removePullWatch parameter docs to include these descriptions
and mark types and required/optional status unchanged.
- Around line 72-73: The phrase "all it's children" in the documentation string
for the example call to window.roamAlphaAPI.data.pull("[:block/string
{:block/children ...}]", "[:block/uid \"xyz\"]") is using the contraction "it's"
incorrectly; change it to the possessive "its" so the line reads "Get the block
string for this block and all its children" to correct the grammar.
- Around line 430-456: The JavaScript example incorrectly calls
window.roamAlphaAPI.updateBlock for a delete operation—replace that call with
window.roamAlphaAPI.deleteBlock using the same {"block":{"uid":"f8cXfDIRn"}}
payload; in the Clojure example rename the function update-block-btn to
delete-block-btn, change the call from block/update to block/delete, and ensure
the button text "delete block" still matches the renamed function and semantics
so the example consistently demonstrates deletion.

In `@skills/roam-alphaapi-data/references/schema.md`:
- Around line 72-73: The schema documentation is missing types/descriptions for
the window fields `collapsed?` and `pinned?`; update their entries to include
the type `__boolean__` and a short description for each (e.g., "`collapsed?`:
__boolean__ — whether the window is collapsed/hidden" and "`pinned?`:
__boolean__ — whether the window is pinned to prevent auto-closing/moving"),
matching the style of the surrounding field docs for consistency.
- Line 89: The line uses LaTeX-style markers $$0$$ and $$n$$ which won't render
in GitHub Markdown; replace the phrase "- Order of the window from $$0$$ to
$$n$$" with plain text or inline code, e.g. "- Order of the window from `0` to
`n`" (target the exact text containing $$0$$ and $$n$$).

In `@skills/roam-alphaapi-files-and-env/references/files-and-env.md`:
- Around line 61-62: The heading "# graph" is missing a blank line before the
list item "- `.graph`"; edit the section containing the heading "# graph" and
insert a single blank line immediately after the heading so the list renders
correctly (i.e., ensure there is an empty line between the "# graph" heading and
the "- `.graph`" content).
- Around line 70-71: The heading "# file" is not followed by a blank line;
update the markdown so there is a single blank line between the heading "# file"
and the list item "- `.file` to comply with proper heading spacing and
formatting (locate the "# file" heading in files-and-env.md and insert an empty
line immediately after it).
- Around line 42-43: The Markdown heading "# platform" needs a blank line after
it to follow Markdown conventions; edit the section containing the "# platform"
heading and insert an empty line between the heading and the following list item
"- `.platform`" so the heading is separated from its content.
- Around line 115-116: The markdown heading "# user" is missing a blank line
before the following list item; edit the section containing the heading "# user"
and insert a single blank line immediately after that heading so the list item
"- `.user`" is separated by an empty line from the heading.
- Around line 154-155: The heading "# constants" is not followed by a blank
line; insert a single empty line between the heading "# constants" and the
following list item "- `.constants`" so the markdown heading is properly
separated from its content (update the block containing the heading "#
constants" and the list item to include one blank line after the heading).
- Around line 136-137: Add a blank line after the heading "# depot" so the
heading is separated from its content; update the block containing the heading
"# depot" and the following list item "- `depot`" to include an empty line
between them (i.e., "# depot" newline newline "- `depot`").
- Around line 14-15: Add a blank line after the Markdown heading "# util" so the
heading and the following list item "- `.util` are separated; locate the heading
string "# util" in the file and insert a single empty line immediately after it
to satisfy markdown formatting and linting rules.

In `@skills/roam-alphaapi-graphview/references/ui-graphview.md`:
- Line 54: The documentation currently contains a TODO placeholder for the
wholeGraph API; replace the TODO with a short but complete description of the
wholeGraph endpoint (purpose, input shape, return shape, examples, and any edge
cases) in the UI docs for the GraphView, or if you want to defer, create and
link an issue to track completing wholeGraph docs and replace the TODO with a
one-line “documented in ISSUE-xxx” reference; search for the symbol wholeGraph
in the GraphView docs to locate where to update.
- Around line 19-21: The description for `callback` is incorrect: it currently
says the function is called "when the user selects the command in the Command
Palette" but this `callback` is invoked when a graph view is loaded; update the
text for `callback` (and the `context` bullet) in ui-graphview.md to state that
the function is called when the graph view is loaded (not a Command Palette
action) and describe what `context` contains in that graph-view load event.

In `@skills/roam-alphaapi-ui-commands-menus/SKILL.md`:
- Line 3: The YAML front-matter description is a single very long line; split it
into a multi-line YAML value (e.g., use the folded style "description: >" or
block style) and break the sentence into shorter lines so the front-matter is
readable; update the SKILL.md description field accordingly so the long
single-line description is replaced with a multi-line YAML string.

In `@skills/roam-alphaapi-ui-filters/references/ui-filters.md`:
- Around line 88-101: The description for `.getSidebarWindowFilters` is
incorrect (copied from `.getPageLinkedRefsFilters`) and the Returns section
contains a stray self-reference; update the human-readable Description for
`.getSidebarWindowFilters` to explain it returns the active include/remove
filters for the right-sidebar window context (given the `window`, `type`, and
`block-uid` parameters) rather than page-linked-ref behavior, and remove the
redundant "`.getSidebarWindowFilters`" line in the Returns section so it only
describes the returned object with "includes" and "removes" lists of page
titles.

In
`@skills/roam-alphaapi-ui-navigation-sidebar/references/ui-navigation-sidebar.md`:
- Around line 399-1556: The document contains multiple full duplications of the
API reference starting at the repeated sections like
`.setBlockFocusAndSelection`, `.mainWindow`, `.leftSidebar`, and
`.rightSidebar`; remove the duplicated block (everything beginning at the second
occurrence of `.setBlockFocusAndSelection` / the repeat starting after line 398)
so only the original single copy (the first occurrence of those sections)
remains; ensure you keep the initial complete reference (the first
`.setBlockFocusAndSelection` through the end of the first `.rightSidebar` block)
and delete the subsequent repeated copies to restore a single canonical
document.

In `@skills/roam-alphaapi-ui-rendering-react/references/ui-rendering-react.md`:
- Around line 297-421: The `.react` documentation block is duplicated (the full
set of component docs for Block, Page, Search, BlockString is repeated); remove
the repeated copy so only one `.react` section remains. Locate the duplicate
block containing the components Block, Page, Search, and BlockString (the same
examples and props repeated) and delete the second occurrence, ensuring you keep
one intact, update surrounding spacing/heading markers if needed, and run a
quick search to confirm no other duplicate component docs remain.
- Line 106: Fix the typo in the docs where the phrase "- default is fale"
appears: change "fale" to "false" so the line reads "- default is false"; update
the occurrence in the UI rendering React reference (look for the exact string "-
default is fale" in ui-rendering-react.md) to correct the boolean default
description.

In `@skills/roam-alphaapi-write-blocks-pages/references/schema.md`:
- Around line 1-84: Both skill directories contain an identical schema.md copy;
extract the shared content into a single canonical schema.md in a shared
references location, remove the duplicate copies, and update both skill
references to include or link to that shared schema file; in the extracted
schema.md fix the incomplete docs for the window fields `collapsed?` and
`pinned?` (document types and defaults) and replace the LaTeX-style $$0$$
notation with plain integers for `order`; after changes, verify both skills
reference the same file (or symlink) so future edits stay centralized.

In `@skills/roam-alphaapi-write-blocks-pages/references/write.md`:
- Around line 316-397: The .page documentation block is duplicated; remove the
second copy (the repeated section describing .page methods such as page.create,
page.fromMarkdown, page.update, page.delete, page.addShortcut, and
page.removeShortcut) so only the original single .page section remains; locate
the repeated verbatim block and delete it to avoid duplicated docs and keep
references to the same method names intact.
- Around line 173-190: The Clojure example incorrectly defines update-block-btn
and calls block/update instead of demonstrating deletion; rename the component
to delete-block-btn and call block/delete with the same payload (e.g., {:block
{:uid "VCuWBrulO"}}) so the example matches the .block.delete documentation and
uses the correct symbol (replace update-block-btn and block/update with
delete-block-btn and block/delete).

In `@src/__tests__/markerParser.test.ts`:
- Around line 1-35: Add unit tests in src/__tests__/markerParser.test.ts to
cover edge cases for parseWidgetMarkers and parseIconParameters: add cases for
empty/malformed markers like "{{}}" and "{{unknown-type}}" and ensure
parseWidgetMarkers returns sensible defaults or skips invalid markers (reference
parseWidgetMarkers), add a test for a block string that contains no valid
markers to assert an empty array result, and add tests for parseIconParameters
handling "icon:" with no parameters (and other malformed icon strings) to verify
it returns undefined/size defaults or throws as expected (reference
parseIconParameters).
- Around line 23-28: The test failure is due to parseIconParameters not handling
the 2-segment shorthand "name::size"; update parseIconParameters to accept both
["name","intent","size"] and ["name","size"] forms: when splitting the payload
(in parseIconParameters) if you get two segments treat the second as size (parse
to number) and set intent = undefined, if three segments use the middle as
intent and parse the last as size; ensure size becomes a number (or undefined if
parse fails) so the test parseIconParameters("icon:search::16") returns { icon:
"search", intent: undefined, size: 16 } and adjust any unit tests if needed.

In `@src/components/BlockWidgetsHost.tsx`:
- Around line 76-134: The callbacks (onSaveSettings, onResetSettings,
onPatchInstance, onPersistValue) currently perform independent read-modify-write
operations (updateLocalInstance then
writeInstanceSettings/writeInstanceValue/upsertInstanceData/resetInstanceSettings)
which can lead to TOCTOU races when multiple widgets on the same blockUid write
concurrently; fix by serializing storage writes per block: introduce a
per-blockUid mutex/queue and acquire it at the start of each callback before
calling
writeInstanceSettings/resetInstanceSettings/upsertInstanceData/writeInstanceValue
(release after the awaited storage call and refresh), so concurrent calls for
the same blockUid are executed sequentially while preserving the existing
optimistic local update via updateLocalInstance.
- Around line 27-29: The effect that blindly calls setLocalData(bpuiData) causes
local optimistic updates to be lost on refresh(); modify the sync to avoid
overwriting in-flight optimistic changes by tracking pending optimistic
operations (e.g., a ref like pendingOptimisticRef) and/or merging instead of
replacing localData: set pendingOptimisticRef.current = true when an optimistic
update starts and false when it finishes, and change the useEffect that
currently runs on bpuiData to only setLocalData(bpuiData) when
!pendingOptimisticRef.current (or perform a shallow/deep merge between bpuiData
and localData to preserve local-only edits); update the optimistic update code
paths to flip the ref so the effect respects in-flight operations.

In `@src/components/common/InlineWidgetShell.tsx`:
- Around line 78-86: The span in the InlineWidgetShell component is a static
container with interactive event handlers but no ARIA role; add role="group" to
the <span> in InlineWidgetShell (the element with className={`bpui-inline-widget
${className}`.trim()}) so assistive tech recognizes it as a non-interactive
container that handles events to prevent edit propagation.
- Line 75: Remove the unnecessary useMemo wrapper around isHovered: replace the
computed constant shouldShowActions that uses useMemo(() => isHovered,
[isHovered]) with a direct reference to the boolean (e.g., const
shouldShowActions = isHovered) inside the InlineWidgetShell component so you
eliminate the no-op memoization and simplify the component logic.
- Around line 49-51: onKeyUp currently always calls setActive(false) which
clears the modifier-gated state even if the Alt key is still held; change
onKeyUp to accept the KeyboardEvent (e) and only call setActive(false) when the
target modifier is no longer active (e.g., if (!e.altKey) or using
e.getModifierState('Alt') === false). Update the onKeyUp signature in
InlineWidgetShell and ensure the keyup handler passed to the element uses the
event parameter so active remains true while the Alt modifier is held.

In `@src/components/common/WidgetSettingsPopover.tsx`:
- Around line 116-120: The onClick handler for WidgetSettingsPopover calls
onReset() but clears local state unconditionally, risking desync if onReset
rejects; update the onClick handler to call onReset() inside a try/catch (or use
onReset().then(...).catch(...)) and only call setDraft({}) and setRawJson("{}")
after onReset resolves successfully, and handle/log errors in the catch so
failures don't wipe the UI state; reference the onReset, setDraft, and
setRawJson calls in the WidgetSettingsPopover onClick handler.
- Around line 55-59: The saveDraft function can leave isSaving true if
onSave(draft) rejects; change saveDraft to mirror saveRawJson by wrapping the
await onSave(draft) in a try/finally so setIsSaving(false) always runs (use the
same try/finally pattern used in saveRawJson), keeping setIsSaving(true) before
the try and preserving the async Promise<void> signature and behavior of onSave.

In `@src/constants/defaults.ts`:
- Line 8: The INTENT_OPTIONS array is currently a plain string[] and can drift
from TagStyleConfig["intent"]; change INTENT_OPTIONS to a readonly tuple by
appending "as const" and then derive a corresponding type (e.g., type Intent =
typeof INTENT_OPTIONS[number]) and use that type for usages such as
WIDGET_SETTING_FIELDS options so mismatches with TagStyleConfig["intent"] are
caught at compile time.

In `@src/core/hashtagStyler.ts`:
- Around line 65-89: styleHashtags currently re-queries and re-processes all tag
elements every observer cycle; update it to skip already-processed elements by
marking candidates after styling and by comparing cached config to avoid
reapplying when unchanged. Inside styleHashtags, after validating with
isHashtagReferenceElement and deriving pageTitle/config (using getTagTitle),
check for a marker (e.g., data-bpui-processed) or the existing bpui-tag class
and a stored JSON config (data-bpui-config) on the candidate; if the marker
exists and the stored config equals the new config, continue/skip. When you do
applyTagClasses/ensureIcon, write the marker and serialized config to the
element (data-bpui-processed="1" and data-bpui-config="...") so subsequent runs
can short-circuit processing unless config differs.
- Around line 39-44: The equality check against expectedClass is brittle;
instead of comparing existing.className to expectedClass, check and manage the
icon-specific class (bp3-icon-${iconName}) using the element's classList: if the
element (existing) does not contain the specific class for iconName, add it, and
avoid overwriting unrelated classes. Update the logic around expectedClass and
existing.className in the styleHashtags/icon-handling block so you use
classList.contains('bp3-icon-${iconName}') and classList.add/remove as needed
rather than replacing the whole className.

In `@src/core/observerEngine.tsx`:
- Line 271: The linter warns because several forEach callbacks use concise arrow
bodies that implicitly return values (e.g., calling
nextHiddenValueChildren.add(uid)); update those callbacks to use block bodies
and explicit statements so they do not return a value. Replace occurrences where
collectHiddenValueChildUids(...).forEach(uid =>
nextHiddenValueChildren.add(uid)) with a block form for the callback (e.g., uid
=> { nextHiddenValueChildren.add(uid); }) and apply the same change for the
other forEach usages at the places flagged (the two calls around lines 342/345
and the calls inside stop()). Keep the same functions/variables
(collectHiddenValueChildUids, nextHiddenValueChildren, and the forEach in
stop()) and only change the arrow callback syntax to a block body.
- Around line 200-212: The async flush() can be re-entered causing race
conditions; add an in-flight guard (e.g., a boolean like isFlushing) and
early-return in flush() to prevent concurrent runs: set isFlushing=true at
start, clear it at the end (finally), and ensure scheduleFlush/markDirty only
schedule when not stopped; reference the flush function and scheduleFlush and
update shared state accesses (blockRecords, hiddenValueChildren, refreshAll,
todoDirty, lastTodoHijackEnabled) so only one flush mutates them at a time.
- Around line 245-303: The loop over targetUids performs sequential awaits of
readBpuiData causing N serial async calls; change it to fetch block data
concurrently with a bounded concurrency limiter: replace the for...of that calls
await readBpuiData with a two-step approach where you (1) build tasks mapping
blockUid -> promise that calls readBpuiData (use Promise.allSettled or a small
concurrency pool like p-limit to avoid thundering), (2) await those promises in
batch and then iterate results to perform the existing logic (checks against
blockRecords, document.body.contains, parseWidgetMarkers,
collectHiddenValueChildUids, hide/restore/unmount, signature comparisons, and
host updates). Keep the same per-block error handling (log and skip) and
maintain use of blockRecords, record.host, record.signature,
nextHiddenValueChildren, parseWidgetMarkers, hideWidgetButtonsForBlock,
restoreWidgetButtonsForBlock, and unmountHost so behavior does not change except
for concurrent reads.
- Around line 62-68: The code in observerEngine.tsx builds expectedWithSpaces
using widgetType.replace("-", " "), which only replaces the first hyphen; update
the creation of expectedWithSpaces to replace all hyphens (e.g., use
widgetType.replaceAll("-", " ") or widgetType.replace(/-/g, " ")) so comparisons
against normalizedButtonText (used in the return expression alongside expected
and startsWith checks) remain correct even if WidgetType values contain multiple
hyphens.
- Around line 310-324: Replace deprecated
ReactDOM.render/ReactDOM.unmountComponentAtNode usage with the React 18+
createRoot API: import { createRoot } from "react-dom/client", create a root via
createRoot(host) and call root.render(<BlockWidgetsHost ... />) instead of
ReactDOM.render, and when unmounting call root.unmount() instead of
ReactDOM.unmountComponentAtNode; store the created root (e.g., in the same
cache/map keyed by blockUid or host used when rendering) so the corresponding
unmount logic (the code path that currently calls
ReactDOM.unmountComponentAtNode) can retrieve and call root.unmount() to clean
up. Ensure this migration is applied for BlockWidgetsHost rendering and its
matching unmount logic as well as similar patterns in todoHijack.

In `@src/core/settings.ts`:
- Line 28: Define a minimal typed interface for the extension API (e.g.,
interface ExtensionAPI { settings: { get(key: string): unknown } }) and replace
the any type on all public functions with this interface; update the signature
of settingValue(extensionAPI: ExtensionAPI, key: string): unknown and any other
exported functions that currently take extensionAPI: any to use ExtensionAPI so
callers get compile-time type safety and IDE hints for settings.get.
- Around line 16-26: The parseJsonWithFallback function currently returns
JSON.parse(raw) as T without validating shape; change parseJsonWithFallback to
validate the parsed value with an isObject-style guard (like the one used in
blockProps.ts) and only return the parsed result when it is a plain object (or
the expected shape), otherwise log a warning and return the fallback; update
callers such as resolveEffectiveSettings to rely on this guard so spreading
globalDefaults won't receive arrays or non-objects.

In `@src/core/todoHijack.tsx`:
- Around line 13-25: The current renderTodoSwitch implementation repeatedly
calls ReactDOM.render on every native change; refactor by creating a
self-contained React component (e.g., TodoSwitchComponent) that holds its own
checked state and subscribes to the underlying native input's "change" event in
useEffect to update that state, and in mountTodoSwitch call ReactDOM.render once
to mount TodoSwitchComponent into host and remove the external change listener;
ensure the component forwards clicks to the original input (call input.click()
on BlueprintSwitch onChange) and cleans up its event listener on unmount.

In `@src/utils/blockProps.ts`:
- Around line 121-143: This is a TOCTOU race: make read→mutate→write atomic per
blockUid by adding a per-block promise-lock (e.g., withWriteLock) and wrapping
the bodies of upsertInstanceData and writeInstanceValue with
withWriteLock(blockUid, async () => { ... }) so reads and subsequent
writeBpuiData calls are serialized; implement withWriteLock as a Map<string,
Promise<any>> chain (prev.then(fn, fn)), ensure the map entry is deleted only
when the chained promise is the same, and keep original function behavior and
return values inside the locked callback.
- Around line 24-33: normalizeBpuiData currently mutates its input (assigning
next._meta) which can alter parsed objects; change it to return a new object
instead of modifying the argument: when value is an object, create and return a
shallow copy of the object with a non-mutating _meta (e.g., {...value, _meta:
{...value._meta, version: BPUI_DATA_VERSION}} or set _meta only when missing) so
callers like parseBpuiConfigChild receive an immutable result; keep the existing
branch that returns a fresh object when value isn't an object.
- Around line 261-271: The code is redundantly calling updateBlockString
immediately after createChildBlock even though createChildBlock already sets the
block string; modify the logic in the valueStorage === "child" branch (which
uses current.valueChildUid, createChildBlock, serializeRawValue,
updateBlockString, blockUid, value) to only call updateBlockString when you are
updating an existing child (i.e., when current.valueChildUid is truthy) and skip
the update when createChildBlock returned a newly created childUid;
alternatively, detect whether childUid came from current.valueChildUid or from
createChildBlock and only call updateBlockString for the former.

In `@src/utils/markerParser.ts`:
- Around line 11-14: isNumericInputAlias currently recognizes the bare space
alias "numeric input" but not seeded forms like "numeric input:42"; update
isNumericInputAlias to also accept strings that start with "numeric input:" (in
addition to "numeric-input:") and then update parseMarkerPayload to handle the
"numeric input:" prefix when extracting the seed/value so both dashed and space
variants (e.g., "numeric-input:42" and "numeric input:42") produce the same
numeric-input widget behavior; reference the isNumericInputAlias and
parseMarkerPayload functions to locate and apply these changes.

In `@src/utils/roam.ts`:
- Around line 46-61: The fallback helpers (pullWithFallback,
updateBlockWithFallback, createBlockWithFallback) currently allow exceptions
from the Roam API to bubble up and crash consumers like pullBlockTree,
pullBlockString, updateBlockString, and createChildBlock; wrap the body of each
helper in a try/catch, call getRoamApi() as before, and on any caught error log
a warning (consistent with runRoamQueryCount style) and return null for
pullWithFallback or void for update/create so callers receive a safe,
non-throwing fallback; ensure the same error-handling pattern is applied to the
API access branches (api.data?.pull vs api.pull and api.data?.update vs
api.update, etc.).
- Around line 147-150: The Math.random fallback that assigns generatedUid can
yield shorter-than-expected UIDs; update the fallback used when
api.util.generateUID is not a function (the generatedUid expression) so it
always produces a fixed-length UID (e.g., 9 chars) by either padding the result
or switching to a stronger generator such as crypto.getRandomValues to build a
9-char base-36 string; keep the conditional using api.util?.generateUID but
replace the Math.random path with a deterministic-length generator to avoid
short/colliding UIDs.

In `@src/widgets/CardWidget.tsx`:
- Around line 12-18: RoamBlockPreview currently reads (window as unknown as {
roamAlphaAPI?: any }).roamAlphaAPI on every render; hoist this lookup to avoid
repeated reads by either declaring a module-scoped constant (e.g., const
RoamBlock = (window as any).roamAlphaAPI?.ui?.react?.Block) or by memoizing
inside the component with useMemo, then have RoamBlockPreview use that stable
RoamBlock reference to render <RoamBlock uid={uid} /> or the fallback div;
update references to the existing RoamBlockPreview and RoamBlock identifiers
accordingly.
- Around line 20-35: The toElevation function currently maps NaN or non-numeric
inputs to Elevation.FOUR; change it to detect invalid numbers (use
Number.isFinite(parsed) or isNaN check on the parsed value) and return the
default elevation (Elevation.ONE) instead of Elevation.FOUR for
undefined/null/non-numeric and out-of-range inputs so it aligns with
WIDGET_DEFAULTS.card; keep the existing exact branch mappings for 0–3 but ensure
the invalid/NaN path returns Elevation.ONE.
- Around line 87-95: The wrapper div can render empty because the current
condition uses !!blockNode.children.length but the map filters out bpui::
children; update the CardWidget render to compute a filteredChildren array
(filtering blockNode.children by !child.string.startsWith("bpui::")), then
conditionally render the <div className="bpui-card-children"> only when
filteredChildren.length > 0 and map over filteredChildren to render
RoamBlockPreview (use child.uid as key).

In `@src/widgets/NumericInputWidget.tsx`:
- Around line 12-15: The helper function toSafeNumber is duplicated (found in
NumericInputWidget.tsx and BetterSliderWidget.tsx); extract it into a shared
utility by creating a new export in a utility module (e.g., export const
toSafeNumber in ~/utils/numbers.ts), delete the duplicate definitions from
NumericInputWidget.tsx and BetterSliderWidget.tsx, and import toSafeNumber from
~/utils/numbers.ts in both files (use the existing function name toSafeNumber to
keep calls unchanged).
- Around line 72-80: The onValueChange async callback in NumericInputWidget (the
handler that calls setValue and await onPersistValue) can produce unhandled
promise rejections; update the handler referenced as onValueChange to either
wrap the await onPersistValue call in try/catch and log errors, or append
.catch(console.error) to the promise so any rejection from onPersistValue is
handled; ensure you keep the existing safe value logic (safe, defaultValue) and
still call setValue(safe) before persisting.
- Line 70: The prop cast "as any" on buttonPosition should be replaced with a
proper typed union so TypeScript validates allowed values; update the
NumericInput usage to pass buttonPosition as a typed value derived from
settings.buttonPosition (coerce/validate it to the union "left" | "right" |
"none" or map settings values to that union) instead of using `as any`, and if
needed add a type for settings.buttonPosition or a small helper that returns
`"left" | "right" | "none"` before passing it to NumericInput to ensure correct
typing.

In `@src/widgets/ProgressWidget.tsx`:
- Around line 14-21: The function parseNamedQueryChild currently compiles a new
RegExp on every call; extract the two regexes into module-level constants (e.g.,
TOTAL_QUERY_PATTERN and PROGRESS_QUERY_PATTERN) keyed to "totalQuery" and
"progressQuery", then have parseNamedQueryChild select the appropriate
precompiled pattern instead of creating one each time; update any references to
pattern within parseNamedQueryChild accordingly so it uses the chosen constant.
- Around line 96-107: The progress bar currently disables animation/stripes
while isRefreshing is true (in const progressBar / ProgressBar props animate and
stripes), so invert the logic: pass animate={isRefreshing} and
stripes={isRefreshing} (rather than !isRefreshing) so the bar shows activity
during refresh; keep the existing value and intent handling (settings.intent)
unchanged and ensure the prop types for ProgressBar accept booleans.

In `@src/widgets/RadioWidget.tsx`:
- Around line 66-84: The async onSelect function (which calls onPatchInstance
and updateBlockString over options) can reject and is currently invoked from a
synchronous onChange without error handling; wrap the body of onSelect in a
try/catch (or add a .catch at the call site) to catch errors from
onPatchInstance and updateBlockString, log or surface the error appropriately,
and ensure any partial updates are handled/avoided; reference the onSelect
function that uses settings.writeSelectionAsTodos, options, ensureTodoSuffix,
stripTodoMarker, and updateBlockString so you can locate and wrap the async
calls with error handling.
- Around line 101-114: The Radio components use option.label for both key and
value which causes collisions when labels duplicate; change the Radio value to
option.uid (e.g., in the Radio elements inside RadioGroup) and keep
label={option.label} for display, then update any selection handling
(onSelect/selectedValue and the logic in writeSelectionAsTodos) to map from the
stored uid back to the option.label when you need the label string; ensure
selectedValue holds the uid and onChange passes the uid to onSelect so selection
is unambiguous.

In `@src/widgets/SwitchWidget.tsx`:
- Around line 56-64: The onChange handler currently sets local state via
setChecked(next) before awaiting onPersistValue, so if persistence fails the UI
remains out of sync; modify the handler in SwitchWidget's onChange to capture
the previous value (const prev = checked), setChecked(next), then await
onPersistValue inside a try/catch and on failure revert with setChecked(prev)
(and surface or log the error as appropriate); keep the same storage logic using
settings.valueStorage and settings.hideValueChild and ensure the catch does not
swallow the error if callers need it.

In `@src/widgets/TabsWidget.tsx`:
- Around line 16-22: The component RoamBlockPreview repeatedly reads
window.roamAlphaAPI on every render; hoist that lookup out of the component or
memoize it so we avoid repeated property traversal — for example, move the
expression (window as unknown as { roamAlphaAPI?: any
}).roamAlphaAPI?.ui?.react?.Block to module scope as a top-level const RoamBlock
(or compute it once with useMemo inside RoamBlockPreview) and then have
RoamBlockPreview simply return <RoamBlock uid={uid} /> when RoamBlock is truthy,
otherwise the fallback div.
- Around line 60-69: The Tabs component's id currently includes selectedTabId
which changes on every tab switch, causing full remounts; update the id to use
only the stable block UID (e.g. use blockNode.uid or a stable prefix like
`tabs-${blockNode.uid}`) so it no longer changes on selection, leaving
selectedTabId, animate, vertical, and onChange (onPatchInstance) logic intact;
locate the Tabs JSX for the component named Tabs and replace
id={`${blockNode.uid}:${selectedTabId}`} with an id derived solely from
blockNode.uid.

In `@src/widgets/types.ts`:
- Around line 1-15: Import the existing ValueStorage type from "~/types/widgets"
and replace the inline union type "props" | "child" in the
WidgetComponentProps.onPersistValue signature with that imported ValueStorage
type; update the file import line to include ValueStorage and change
onPersistValue to (value: unknown, valueStorage: ValueStorage, hideValueChild:
boolean) => Promise<void> so the prop uses the shared type instead of
duplicating the union.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Minified single-line CSS checked into source is unmaintainable.

The entire stylesheet is on one line, making diffs useless and manual editing error-prone. Keep the source formatted (one rule per line) and let the build pipeline minify if needed. Every future change will show the entire file as a single-line diff.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@extension.css` at line 1, The file is a minified single-line CSS which makes
diffs and edits impossible; reformat extension.css into a readable, maintainable
style sheet (one selector/rule per line, preserve all rules and order) — for
example expand selectors like .bpui-inline-host, .bpui-widget-row,
.bpui-widget-settings-previews, the media query `@media` (max-width: 720px), etc.,
into multi-line rules so each property is on its own line; after reformatting,
ensure the build pipeline (or repo tooling) is used to minify for production and
consider adding a prettier/stylelint rule to prevent future checked-in minified
CSS.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Typo: "it's" → "its".

Line 73: "all it's children" should be "all its children" (possessive, not contraction).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/roam-alphaapi-data/references/data.md` around lines 72 - 73, The
phrase "all it's children" in the documentation string for the example call to
window.roamAlphaAPI.data.pull("[:block/string {:block/children ...}]",
"[:block/uid \"xyz\"]") is using the contraction "it's" incorrectly; change it
to the possessive "its" so the line reads "Get the block string for this block
and all its children" to correct the grammar.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

addPullWatch and removePullWatch parameter descriptions are incomplete.

Lines 192, 196, 223, and 227 contain {{[[TODO]]}} placeholders instead of actual descriptions for the pull pattern and entity-id parameters. These should be filled in before merging.

Would you like me to draft descriptions for these parameters based on the usage examples already present in this file?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/roam-alphaapi-data/references/data.md` around lines 187 - 232, The
parameter descriptions for addPullWatch and removePullWatch are missing; replace
the {{[[TODO]]}} placeholders for the "pull pattern" and "entity-id" parameters
with clear descriptions: for "pull pattern" explain it is a Roam-style pull
query string (e.g., "[:block/children :block/string {:block/children ...}]")
that specifies which attributes/relations to fetch and watch, and for
"entity-id" explain it is a Roam entity identifier string (either an entity
reference like '[:block/uid \"<uid>\"]' or a page reference) that scopes the
watch to a specific block or page; update both addPullWatch and removePullWatch
parameter docs to include these descriptions and mark types and
required/optional status unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Copy-paste bug: .delete block example calls .updateBlock instead of .deleteBlock.

The JavaScript example at line 434 calls window.roamAlphaAPI.updateBlock(...) but this section documents the delete operation. The Clojure example similarly names the function update-block-btn (line 445) while the button text says "delete block" (line 454). This will mislead developers.

📝 Proposed fix

                    - [[roam/js]]
                        - ```javascript
                          window
                            .roamAlphaAPI
-                           .updateBlock({"block": 
+                           .deleteBlock({"block": 
                                          {"uid": "f8cXfDIRn"}})
                          ```

And in the Clojure example, rename the function and call accordingly:

-                          (defn update-block-btn [_]
+                          (defn delete-block-btn [_]
                               [:span
                                ...
-                               :on-click (fn [evt] 
-                                           (block/update 
-                                            {:block {:uid "VCuWBrulO"}}))}
+                               :on-click (fn [evt]
+                                           (block/delete
+                                            {:block {:uid "VCuWBrulO"}}))}
                                "delete block"])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/roam-alphaapi-data/references/data.md` around lines 430 - 456, The
JavaScript example incorrectly calls window.roamAlphaAPI.updateBlock for a
delete operation—replace that call with window.roamAlphaAPI.deleteBlock using
the same {"block":{"uid":"f8cXfDIRn"}} payload; in the Clojure example rename
the function update-block-btn to delete-block-btn, change the call from
block/update to block/delete, and ensure the button text "delete block" still
matches the renamed function and semantics so the example consistently
demonstrates deletion.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Incomplete documentation for collapsed? and pinned? fields.

These two window fields lack type annotations and descriptions, unlike every other field in this schema. Add type (likely __boolean__) and a brief description for each.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/roam-alphaapi-data/references/schema.md` around lines 72 - 73, The
schema documentation is missing types/descriptions for the window fields
`collapsed?` and `pinned?`; update their entries to include the type
`__boolean__` and a short description for each (e.g., "`collapsed?`: __boolean__
— whether the window is collapsed/hidden" and "`pinned?`: __boolean__ — whether
the window is pinned to prevent auto-closing/moving"), matching the style of the
surrounding field docs for consistency.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

animate and stripes logic appears inverted — bar is static during refresh.

When isRefreshing is true, both animate and stripes are set to false, making the progress bar look static during loading. The expected UX is the opposite: show stripes/animation while fetching to indicate activity.

🐛 Proposed fix

     <ProgressBar
-      animate={!isRefreshing}
-      stripes={!isRefreshing}
+      animate={isRefreshing}
+      stripes={isRefreshing}
       value={value}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const progressBar = (
	<ProgressBar
	animate={!isRefreshing}
	stripes={!isRefreshing}
	value={value}
	intent={
	`${settings.intent || "none"}` === "none"
	? undefined
	: (`${settings.intent || "none"}` as any)
	}
	/>
	);
	const progressBar = (
	<ProgressBar
	animate={isRefreshing}
	stripes={isRefreshing}
	value={value}
	intent={
	`${settings.intent || "none"}` === "none"
	? undefined
	: (`${settings.intent || "none"}` as any)
	}
	/>
	);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/ProgressWidget.tsx` around lines 96 - 107, The progress bar
currently disables animation/stripes while isRefreshing is true (in const
progressBar / ProgressBar props animate and stripes), so invert the logic: pass
animate={isRefreshing} and stripes={isRefreshing} (rather than !isRefreshing) so
the bar shows activity during refresh; keep the existing value and intent
handling (settings.intent) unchanged and ensure the prop types for ProgressBar
accept booleans.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Unhandled persistence failure leaves local state out of sync.

If onPersistValue rejects, checked will have already been set to next on line 58, but the value won't be persisted. Consider reverting local state on failure.

🛡️ Proposed fix

         onChange={async (event) => {
           const next = (event.target as HTMLInputElement).checked;
           setChecked(next);
-          await onPersistValue(
-            next,
-            `${settings.valueStorage || "props"}` === "child" ? "child" : "props",
-            !!settings.hideValueChild,
-          );
+          try {
+            await onPersistValue(
+              next,
+              `${settings.valueStorage || "props"}` === "child" ? "child" : "props",
+              !!settings.hideValueChild,
+            );
+          } catch {
+            setChecked(!next);
+          }
         }}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	onChange={async (event) => {
	const next = (event.target as HTMLInputElement).checked;
	setChecked(next);
	await onPersistValue(
	next,
	`${settings.valueStorage || "props"}` === "child" ? "child" : "props",
	!!settings.hideValueChild,
	);
	}}
	onChange={async (event) => {
	const next = (event.target as HTMLInputElement).checked;
	setChecked(next);
	try {
	await onPersistValue(
	next,
	`${settings.valueStorage || "props"}` === "child" ? "child" : "props",
	!!settings.hideValueChild,
	);
	} catch {
	setChecked(!next);
	}
	}}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/SwitchWidget.tsx` around lines 56 - 64, The onChange handler
currently sets local state via setChecked(next) before awaiting onPersistValue,
so if persistence fails the UI remains out of sync; modify the handler in
SwitchWidget's onChange to capture the previous value (const prev = checked),
setChecked(next), then await onPersistValue inside a try/catch and on failure
revert with setChecked(prev) (and surface or log the error as appropriate); keep
the same storage logic using settings.valueStorage and settings.hideValueChild
and ensure the catch does not swallow the error if callers need it.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

RoamBlockPreview re-reads window.roamAlphaAPI on every render.

The global lookup could be hoisted to module scope or memoized to avoid repeated property traversal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/TabsWidget.tsx` around lines 16 - 22, The component
RoamBlockPreview repeatedly reads window.roamAlphaAPI on every render; hoist
that lookup out of the component or memoize it so we avoid repeated property
traversal — for example, move the expression (window as unknown as {
roamAlphaAPI?: any }).roamAlphaAPI?.ui?.react?.Block to module scope as a
top-level const RoamBlock (or compute it once with useMemo inside
RoamBlockPreview) and then have RoamBlockPreview simply return <RoamBlock
uid={uid} /> when RoamBlock is truthy, otherwise the fallback div.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Tabs id changes on every tab switch, causing full remount.

The id prop includes selectedTabId (${blockNode.uid}:${selectedTabId}), so it changes whenever the user switches tabs. This forces React/Blueprint to unmount and remount the entire Tabs component on every selection, breaking the animate transition and causing unnecessary DOM thrashing.

Use only the stable block UID for the id:

🐛 Proposed fix

       <Tabs
-        id={`${blockNode.uid}:${selectedTabId}`}
+        id={`bpui-tabs-${blockNode.uid}`}
         selectedTabId={selectedTabId}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	<Tabs
	id={`${blockNode.uid}:${selectedTabId}`}
	selectedTabId={selectedTabId}
	animate={!!settings.animate}
	vertical={!!settings.vertical}
	onChange={async (nextTabId) =>
	onPatchInstance({
	selectedTabId: `${nextTabId}`,
	})
	}
	<Tabs
	id={`bpui-tabs-${blockNode.uid}`}
	selectedTabId={selectedTabId}
	animate={!!settings.animate}
	vertical={!!settings.vertical}
	onChange={async (nextTabId) =>
	onPatchInstance({
	selectedTabId: `${nextTabId}`,
	})
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/TabsWidget.tsx` around lines 60 - 69, The Tabs component's id
currently includes selectedTabId which changes on every tab switch, causing full
remounts; update the id to use only the stable block UID (e.g. use blockNode.uid
or a stable prefix like `tabs-${blockNode.uid}`) so it no longer changes on
selection, leaving selectedTabId, animate, vertical, and onChange
(onPatchInstance) logic intact; locate the Tabs JSX for the component named Tabs
and replace id={`${blockNode.uid}:${selectedTabId}`} with an id derived solely
from blockNode.uid.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Use the ValueStorage type instead of inlining "props" | "child".

Line 13 duplicates the ValueStorage type already exported from ~/types/widgets. Import and use it for consistency.

♻️ Proposed fix

-import type { BlockNode, BpuiInstanceData, ModifierKey, WidgetMarker, WidgetType } from "~/types/widgets";
+import type { BlockNode, BpuiInstanceData, ModifierKey, ValueStorage, WidgetMarker, WidgetType } from "~/types/widgets";

-  onPersistValue: (value: unknown, valueStorage: "props" | "child", hideValueChild: boolean) => Promise<void>;
+  onPersistValue: (value: unknown, valueStorage: ValueStorage, hideValueChild: boolean) => Promise<void>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/types.ts` around lines 1 - 15, Import the existing ValueStorage
type from "~/types/widgets" and replace the inline union type "props" | "child"
in the WidgetComponentProps.onPersistValue signature with that imported
ValueStorage type; update the file import line to include ValueStorage and
change onPersistValue to (value: unknown, valueStorage: ValueStorage,
hideValueChild: boolean) => Promise<void> so the prop uses the shared type
instead of duplicating the union.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Typo: "fale" → "false".

-                                - default is fale
+                                - default is false

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	- default is fale
	- default is false

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/roam-alphaapi-ui-rendering-react/references/ui-rendering-react.md` at
line 106, Fix the typo in the docs where the phrase "- default is fale" appears:
change "fale" to "false" so the line reads "- default is false"; update the
occurrence in the UI rendering React reference (look for the exact string "-
default is fale" in ui-rendering-react.md) to correct the boolean default
description.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

useEffect sync may cause a render flash after every refresh().

When refresh() re-fetches bpuiData and passes it as a new prop, this useEffect resets localData, potentially discarding in-flight optimistic updates. If refresh is called while another async operation is still in progress, the optimistic update from that operation will be lost.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/BlockWidgetsHost.tsx` around lines 27 - 29, The effect that
blindly calls setLocalData(bpuiData) causes local optimistic updates to be lost
on refresh(); modify the sync to avoid overwriting in-flight optimistic changes
by tracking pending optimistic operations (e.g., a ref like
pendingOptimisticRef) and/or merging instead of replacing localData: set
pendingOptimisticRef.current = true when an optimistic update starts and false
when it finishes, and change the useEffect that currently runs on bpuiData to
only setLocalData(bpuiData) when !pendingOptimisticRef.current (or perform a
shallow/deep merge between bpuiData and localData to preserve local-only edits);
update the optimistic update code paths to flip the ref so the effect respects
in-flight operations.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

INTENT_OPTIONS is a plain string[] — consider as const for type safety.

INTENT_OPTIONS is used as options in WIDGET_SETTING_FIELDS (lines 168, 178) and could drift out of sync with the TagStyleConfig["intent"] union type. Adding as const and deriving the type from the union would catch mismatches at compile time.

♻️ Suggested change

-export const INTENT_OPTIONS = ["none", "primary", "success", "warning", "danger"];
+export const INTENT_OPTIONS = ["none", "primary", "success", "warning", "danger"] as const;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export const INTENT_OPTIONS = ["none", "primary", "success", "warning", "danger"];
	export const INTENT_OPTIONS = ["none", "primary", "success", "warning", "danger"] as const;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/constants/defaults.ts` at line 8, The INTENT_OPTIONS array is currently a
plain string[] and can drift from TagStyleConfig["intent"]; change
INTENT_OPTIONS to a readonly tuple by appending "as const" and then derive a
corresponding type (e.g., type Intent = typeof INTENT_OPTIONS[number]) and use
that type for usages such as WIDGET_SETTING_FIELDS options so mismatches with
TagStyleConfig["intent"] are caught at compile time.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

className equality check is order-sensitive and fragile.

If any other code adds a class to the icon element, existing.className !== expectedClass will always be true, causing unnecessary DOM updates on every styleHashtags call. Consider checking for the icon-specific class instead.

Proposed fix

-  const expectedClass = `bp3-icon bp3-icon-${iconName} bpui-tag-icon`;
-  if (existing) {
-    if (existing.className !== expectedClass) {
-      existing.className = expectedClass;
-    }
-    return;
-  }
+  const iconClass = `bp3-icon-${iconName}`;
+  if (existing) {
+    if (!existing.classList.contains(iconClass)) {
+      existing.className = `bp3-icon ${iconClass} bpui-tag-icon`;
+    }
+    return;
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/hashtagStyler.ts` around lines 39 - 44, The equality check against
expectedClass is brittle; instead of comparing existing.className to
expectedClass, check and manage the icon-specific class (bp3-icon-${iconName})
using the element's classList: if the element (existing) does not contain the
specific class for iconName, add it, and avoid overwriting unrelated classes.
Update the logic around expectedClass and existing.className in the
styleHashtags/icon-handling block so you use
classList.contains('bp3-icon-${iconName}') and classList.add/remove as needed
rather than replacing the whole className.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

styleHashtags re-queries and re-processes all tags on every invocation.

Since this runs on every observer cycle, it processes already-styled tags unnecessarily. Consider marking processed elements (e.g., with a data attribute or checking for bpui-tag class) to skip elements whose config hasn't changed, especially on large pages.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/hashtagStyler.ts` around lines 65 - 89, styleHashtags currently
re-queries and re-processes all tag elements every observer cycle; update it to
skip already-processed elements by marking candidates after styling and by
comparing cached config to avoid reapplying when unchanged. Inside
styleHashtags, after validating with isHashtagReferenceElement and deriving
pageTitle/config (using getTagTitle), check for a marker (e.g.,
data-bpui-processed) or the existing bpui-tag class and a stored JSON config
(data-bpui-config) on the candidate; if the marker exists and the stored config
equals the new config, continue/skip. When you do applyTagClasses/ensureIcon,
write the marker and serialized config to the element (data-bpui-processed="1"
and data-bpui-config="...") so subsequent runs can short-circuit processing
unless config differs.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

toSafeNumber is duplicated across widget files.

The identical helper exists in BetterSliderWidget.tsx (lines 12-15). Extract it into a shared utility (e.g., ~/utils/numbers.ts) to avoid drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/NumericInputWidget.tsx` around lines 12 - 15, The helper function
toSafeNumber is duplicated (found in NumericInputWidget.tsx and
BetterSliderWidget.tsx); extract it into a shared utility by creating a new
export in a utility module (e.g., export const toSafeNumber in
~/utils/numbers.ts), delete the duplicate definitions from
NumericInputWidget.tsx and BetterSliderWidget.tsx, and import toSafeNumber from
~/utils/numbers.ts in both files (use the existing function name toSafeNumber to
keep calls unchanged).

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Avoid as any — use a typed union for buttonPosition.

Blueprint v3's NumericInput accepts "left" | "right" | "none" for buttonPosition. The as any cast suppresses type checking entirely.

Proposed fix

-        buttonPosition={`${settings.buttonPosition}` as any}
+        buttonPosition={
+          (["left", "right", "none"].includes(`${settings.buttonPosition}`)
+            ? `${settings.buttonPosition}`
+            : "right") as "left" | "right" | "none"
+        }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	buttonPosition={`${settings.buttonPosition}` as any}
	buttonPosition={
	(["left", "right", "none"].includes(`${settings.buttonPosition}`)
	? `${settings.buttonPosition}`
	: "right") as "left" | "right" | "none"
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/NumericInputWidget.tsx` at line 70, The prop cast "as any" on
buttonPosition should be replaced with a proper typed union so TypeScript
validates allowed values; update the NumericInput usage to pass buttonPosition
as a typed value derived from settings.buttonPosition (coerce/validate it to the
union "left" | "right" | "none" or map settings values to that union) instead of
using `as any`, and if needed add a type for settings.buttonPosition or a small
helper that returns `"left" | "right" | "none"` before passing it to
NumericInput to ensure correct typing.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Async onValueChange — same unhandled rejection concern as RadioWidget.

The async callback returned from onValueChange is not caught. Add .catch(console.error) or wrap in try/catch for resilience.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/NumericInputWidget.tsx` around lines 72 - 80, The onValueChange
async callback in NumericInputWidget (the handler that calls setValue and await
onPersistValue) can produce unhandled promise rejections; update the handler
referenced as onValueChange to either wrap the await onPersistValue call in
try/catch and log errors, or append .catch(console.error) to the promise so any
rejection from onPersistValue is handled; ensure you keep the existing safe
value logic (safe, defaultValue) and still call setValue(safe) before
persisting.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Async onSelect called from synchronous onChange — unhandled rejection if any write fails.

onSelect is async and called at Line 104 without .catch(). If onPatchInstance or any updateBlockString call rejects, it becomes an unhandled promise rejection. Consider wrapping with a try/catch or attaching a .catch() at the call site.

Proposed fix at the call site (line 104)

-        onChange={(event) => onSelect((event.target as HTMLInputElement).value)}
+        onChange={(event) => {
+          onSelect((event.target as HTMLInputElement).value).catch(console.error);
+        }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/RadioWidget.tsx` around lines 66 - 84, The async onSelect
function (which calls onPatchInstance and updateBlockString over options) can
reject and is currently invoked from a synchronous onChange without error
handling; wrap the body of onSelect in a try/catch (or add a .catch at the call
site) to catch errors from onPatchInstance and updateBlockString, log or surface
the error appropriately, and ensure any partial updates are handled/avoided;
reference the onSelect function that uses settings.writeSelectionAsTodos,
options, ensureTodoSuffix, stripTodoMarker, and updateBlockString so you can
locate and wrap the async calls with error handling.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Radio options keyed and valued by label — duplicates will collide.

If two child blocks produce the same stripped label, value={option.label} makes them indistinguishable to the RadioGroup. Selection of one will also mark the other (both get TODO suffix in writeSelectionAsTodos mode). Consider using option.uid as the value and mapping back to the label for display.

Proposed fix

       <RadioGroup
         inline={!!settings.inline}
-        selectedValue={selectedValue}
-        onChange={(event) => onSelect((event.target as HTMLInputElement).value)}
+        selectedValue={
+          options.find((o) => o.label === selectedValue)?.uid || selectedValue
+        }
+        onChange={(event) => {
+          const uid = (event.target as HTMLInputElement).value;
+          const match = options.find((o) => o.uid === uid);
+          if (match) onSelect(match.label);
+        }}
       >
         {options.map((option) => (
           <Radio
             key={option.uid}
-            value={option.label}
+            value={option.uid}
             label={option.label}
             disabled={!!settings.disabled}
             inline={!!settings.inline}
           />
         ))}
       </RadioGroup>

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	<RadioGroup
	inline={!!settings.inline}
	selectedValue={selectedValue}
	onChange={(event) => onSelect((event.target as HTMLInputElement).value)}
	>
	{options.map((option) => (
	<Radio
	key={option.uid}
	value={option.label}
	label={option.label}
	disabled={!!settings.disabled}
	inline={!!settings.inline}
	/>
	))}
	<RadioGroup
	inline={!!settings.inline}
	selectedValue={
	options.find((o) => o.label === selectedValue)?.uid || selectedValue
	}
	onChange={(event) => {
	const uid = (event.target as HTMLInputElement).value;
	const match = options.find((o) => o.uid === uid);
	if (match) onSelect(match.label);
	}}
	>
	{options.map((option) => (
	<Radio
	key={option.uid}
	value={option.uid}
	label={option.label}
	disabled={!!settings.disabled}
	inline={!!settings.inline}
	/>
	))}
	</RadioGroup>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/widgets/RadioWidget.tsx` around lines 101 - 114, The Radio components use
option.label for both key and value which causes collisions when labels
duplicate; change the Radio value to option.uid (e.g., in the Radio elements
inside RadioGroup) and keep label={option.label} for display, then update any
selection handling (onSelect/selectedValue and the logic in
writeSelectionAsTodos) to map from the stored uid back to the option.label when
you need the label string; ensure selectedValue holds the uid and onChange
passes the uid to onSelect so selection is unambiguous.