Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR simplifies FuzzySelectInput's locking mechanism by removing public props (onLockedChange, initialUid, initialIsLocked) and deriving lock state internally as Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/roam/src/components/FuzzySelectInput.tsx (2)
71-79:isLockedin the dependency array is a derived value — consider usingvalue?.uidinstead.
isLockedis derived on every render (mode === "create" && Boolean(value?.uid)), so it's not a stable reference. While this works because React compares the primitive boolean value, it would be slightly more explicit and conventional to depend on the underlying values (mode,value?.uid) rather than a derived local variable. This is a minor readability nit — no functional issue.
66-69:valuecould beundefined— spread is safe but the dep array causes frequent re-creation.
valueis typed as optional (value?: T), and{...undefined}is valid JS, so the spread is safe. However, sincevalueis an object, the dependency[value, setValue]means this callback is recreated whenever the parent provides a newvaluereference. If this causes performance concerns, you could narrow the dep to[setValue]and accessvaluevia a ref, but this is fine for now.
|
c6b53e0 quick fixed: keyboard issue to be addressed here: ENG-1316: When selecting node using enter, shouldn't focus on node select dropdown |
|
|
||
| const handleSelect = useCallback( | ||
| (item: T) => { | ||
| if (mode === "create" && item.uid && item.uid !== initialUid) { |
There was a problem hiding this comment.
Oh I think we need another variable to detect if we're creating nodes in canvas.
Removing this and asserting const isLocked = mode === "create" && Boolean(value?.uid); makes it so that every new node creation in Canvas will have isLocked=true by default (because block uid is created first before the flow is finished)
There was a problem hiding this comment.
Nice catch. Re-added isLocked and made it required.
https://www.loom.com/share/0403ecba18dd478995bfb9310a0a6aea
mdroidian
deleted the
eng-1378-automatic-source-appending-for-evd-incorrectly-pre-pends
branch
2 participants
https://www.loom.com/share/12276fa951e14a1d824e8da9e5f6ab7d
Summary
Fixes the EVD create flow so a prefilled Source is visibly locked and treated as an existing node, instead of being silently converted into a “new source” input path.
Problem
In the create-node modal, the referenced Source could appear prefilled but still behave as unlocked. In that state, the input update path could clear the referenced uid, which then caused submit logic to treat it as a new source and re-run source formatting.
Root Cause
FuzzySelectInput kept a separate local isLocked state that could drift from the actual source-of-truth (value.uid), and lock behavior relied on prop-sync logic.
What Changed
Why This Approach
This removes duplicated state and makes lock behavior deterministic from a single source of truth. It is simpler, more React-idiomatic, and avoids stale lock transitions.
Summary by CodeRabbit