Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
0fb7fc7 to
62b0359
Compare
1cdc812 to
b206a69
Compare
62b0359 to
40379a1
Compare
b206a69 to
9f56c90
Compare
40379a1 to
ac5f761
Compare
9f56c90 to
a7867cb
Compare
0cb1b4d to
08241e3
Compare
a7867cb to
464eb1a
Compare
08241e3 to
a671592
Compare
464eb1a to
3cb0f2b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughA new file introduces reusable, type-safe React setting panel components for the Roam application. It provides base implementations for text, boolean, number, select, and multi-text inputs with consistent UI layouts, along with higher-level wrappers for global and personal settings, and a specialized FeatureFlagPanel with optional pre/post-change hooks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx`:
- Around line 171-174: The handler for NumericInput (handleChange) must guard
against valueAsNumber being NaN to avoid storing NaN in settings: update
handleChange to check Number.isNaN(valueAsNumber) and in that case
setValue(undefined) (or another explicit empty sentinel) and call
setter(settingKeys, undefined) (or skip calling setter), otherwise call
setValue(valueAsNumber) and setter(settingKeys, valueAsNumber); reference the
handleChange function, NumericInput, setValue, setter and settingKeys when
making the change.
- Around line 131-147: The event handler handleChange uses
React.ChangeEvent<HTMLInputElement> but BlueprintJS Checkbox passes a
React.FormEvent<HTMLInputElement>; change the signature of handleChange to (e:
React.FormEvent<HTMLInputElement>) and read the value via
e.currentTarget.checked (or cast e.target to HTMLInputElement), update any
related references (handleChange usage and declaration) so setter(settingKeys,
checked) and onChange?.(checked) receive the correct boolean; keep the async
onBeforeChange flow intact and ensure imports/types reference React.FormEvent
where needed.
🧹 Nitpick comments (1)
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx (1)
200-202: Consider guarding against empty options array.If
optionsis an empty array anddefaultValueis not provided,options[0]evaluates toundefined, which could cause unexpected behavior inHTMLSelect.💡 Suggested defensive check
const [value, setValue] = useState( - () => getter(settingKeys) ?? defaultValue ?? options[0], + () => getter(settingKeys) ?? defaultValue ?? options[0] ?? "", );
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
90303ff to
050f93b
Compare
d29fb40 to
0dab0a5
Compare
mdroidian
left a comment
There was a problem hiding this comment.
A few changes to make sure you do before merging.
Also, make sure to format with prettier before merging.
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
941c0af to
34e7587
Compare
050f93b to
9095d0b
Compare
34e7587 to
74fabe6
Compare
9095d0b to
304f5f9
Compare
74fabe6 to
67f9604
Compare
304f5f9 to
a4c9d2c
Compare

https://www.loom.com/share/8ff86c595970423bbf1ec161baf5efa4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.