Roam: ENG-738 fix Description field is too small #373
Conversation
…, fix some lint warnings
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds multi-line Description support in NodeConfig by introducing a ValidatedTextareaPanel with Blueprint TextArea, makes the debounced updater generic to handle inputs and textareas, updates state/handlers for Description, adjusts General tab layout to vertical, and ignores Promise results from Roam write calls. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/roam/src/components/settings/NodeConfig.tsx (5)
60-90: ValidatedTextareaPanel: solid extraction; consider using Blueprint’s fill/rows for consistency.Minor UI nit: prefer native Blueprint props for sizing to stay visually consistent with other controls.
Apply this tweak:
<TextArea value={value} onChange={onChange} onBlur={onBlur} placeholder={placeholder} - className="w-full" - style={{ minHeight: 80, resize: "vertical" }} + className="w-full" + fill + rows={4} + style={{ resize: "vertical" }} />
103-124: Debounce timer needs unmount cleanup and value re-sync when uid/initialValue changes.
- Without clearing the pending timeout on unmount, a write can fire after the component is gone.
- Local state never re-syncs if a different node/uid is loaded into the same mounted component.
Both are easy, low-risk fixes.
const isValidRef = useRef(isValid); isValidRef.current = isValid; + // Cleanup pending writes on unmount + useEffect(() => { + return () => window.clearTimeout(debounceRef.current); + }, []); + + // Keep local state in sync when switching nodes/values + useEffect(() => { + setValue(initialValue); + }, [uid, initialValue]);
111-118: Confirm single-child semantics before overwriting only the first child.You read the first child under the config parent (getBasicTreeByParentUid(uid)[0]) and update/create just that node. If more than one child exists (e.g., manual edits), this will ignore others and could surprise users. If the intended model is “exactly one child holds the value,” consider guarding or normalizing.
I can help harden this by pruning extra children or by selecting a child by a recognizable marker.
114-118: Avoid unhandled promise rejections for write calls.Using void ignores the result but does not prevent unhandled rejections if the write fails (network, permissions). Chain a no-op catch (or a logger) to keep the “fire-and-forget” behavior without noisy errors.
- void updateBlock({ uid: existingBlock.uid, text }); + updateBlock({ uid: existingBlock.uid, text }).catch(() => { /* intentionally ignored */ }); @@ - void createBlock({ parentUid: uid, node: { text } }); + createBlock({ parentUid: uid, node: { text } }).catch(() => { /* intentionally ignored */ });
178-183: Blur handlers may write before validation settles (0ms timeout race).handleTagBlur/handleFormatBlur call the hook’s blur first (which schedules an immediate write), then run validate(). Because validation updates isValid via state, the 0ms write can read the previous isValidRef and persist an invalid value.
Reorder to validate first; then trigger the blur write. Low-risk, improves correctness.
const handleTagBlur = useCallback(() => { - handleTagBlurFromHook(); - validate(tagValue, formatValue); + validate(tagValue, formatValue); + handleTagBlurFromHook(); }, [handleTagBlurFromHook, tagValue, formatValue, validate]); const handleFormatBlur = useCallback(() => { - handleFormatBlurFromHook(); - validate(tagValue, formatValue); + validate(tagValue, formatValue); + handleFormatBlurFromHook(); }, [handleFormatBlurFromHook, tagValue, formatValue, validate]);If you prefer to keep the current order, add a tiny timeout to the blur path (e.g., timeout 10–20ms) so the validation render updates isValidRef before the save callback fires.
Also applies to: 187-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/roam/src/components/settings/NodeConfig.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/settings/NodeConfig.tsx
🔇 Additional comments (5)
apps/roam/src/components/settings/NodeConfig.tsx (5)
9-16: Imports look good; BP v3 TextArea addition aligns with our UI stack.Using TextArea from @blueprintjs/core matches the retrieved learning to use BlueprintJS 3 + Tailwind. No issues spotted.
91-97: Good call making useDebouncedRoamUpdater generic.The <T,> disambiguation in TSX is correct; this unlocks reuse for inputs and textareas without any any-casting.
127-133: Typed change handler looks correct.React.ChangeEvent with the generic element keeps consumers type-safe. No action needed.
192-200: Description uses the new textarea updater correctly.Passing true for isValid is fine given no validation rules for Description. Looks good.
265-291: General tab layout and Description UX are improvements.Vertical stack with the textarea first reads well. No functional issues spotted.
| ); | ||
|
|
||
| const useDebouncedRoamUpdater = ( | ||
| const ValidatedTextareaPanel = ({ |
There was a problem hiding this comment.
How is it "validated"? Or why is this called "Validated..."?
mdroidian
deleted the
eng-738-update-general-tab-description-vertical
branch
…, fix some lint warnings (#373)
https://www.loom.com/share/7803fae56fb142e4b65495ef9d2a9be2?sid=2b615b6d-dbe1-426a-a168-f196de29e4d1
Summary by CodeRabbit
New Features
Style