refactor: decompose NoteModal + extract useEditableUrl hook (#151, #152)#158
Merged
ryota-murakami merged 3 commits intomainfrom Feb 25, 2026
Merged
refactor: decompose NoteModal + extract useEditableUrl hook (#151, #152)#158ryota-murakami merged 3 commits intomainfrom
ryota-murakami merged 3 commits intomainfrom
Conversation
…152) NoteModal (363→130 lines): - Extract useNoteModalDraft hook for draft persistence - Extract NoteSection for note editing UI - Extract LinkManager for link CRUD with undo support EditableUrlItem (26→1 hook in component): - Extract useEditableUrl hook encapsulating all editing lifecycle - Component becomes pure render with single hook call
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracts note editing and link management from NoteModal into NoteSection and LinkManager, adds useNoteModalDraft hook for draft/persistence, and moves EditableUrlItem logic into a new useEditableUrl hook; NoteModal now composes the new pieces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NoteSection
participant LinkManager
participant useNoteModalDraft
participant Redux
participant Server
participant Toast
User->>NoteSection: edit note
NoteSection->>useNoteModalDraft: handleNoteChange(payload)
User->>LinkManager: add/edit/delete link
LinkManager->>useNoteModalDraft: handleLinksChange(links)
useNoteModalDraft->>Redux: persist draft
User->>NoteSection: click Save
NoteSection->>useNoteModalDraft: handleSave()
useNoteModalDraft->>Server: onSave({note, links})
Server-->>useNoteModalDraft: success / error
useNoteModalDraft->>Redux: clear draft (on success)
useNoteModalDraft->>Toast: show success / error
Toast-->>User: notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
- Coverage 69.09% 69.06% -0.03%
==========================================
Files 149 153 +4
Lines 4607 4636 +29
Branches 1269 1274 +5
==========================================
+ Hits 3183 3202 +19
- Misses 1401 1411 +10
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/hooks/board/useNoteModalDraft.ts (3)
204-206:handleClosewrapping is redundant.This
useCallbackjust forwards toonClosewith no added logic. You could passonClosedirectly to consumers and drop the wrapper.♻️ Simplify
- const handleClose = useCallback(() => { - onClose() - }, [onClose]) + // Use onClose directly in the return objectAnd in the return:
- handleClose, + handleClose: onClose,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useNoteModalDraft.ts` around lines 204 - 206, The handleClose useCallback is redundant because it only forwards to onClose; remove the handleClose wrapper in useNoteModalDraft and pass the original onClose directly to consumers (e.g., where handleClose was returned/used). Update any references to handleClose to use onClose (in the hook's return value and any internal usages) and delete the handleClose declaration to eliminate unnecessary indirection.
213-215:isFormValidrunsisValidUrlon every render for all links.This is computed inline (not memoized). For a small number of links it's negligible, but if
isValidUrldoes regex or URL parsing, wrapping inuseMemowith[links]as a dependency would be more consistent with the memoizedcharCountabove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useNoteModalDraft.ts` around lines 213 - 215, The isFormValid boolean is recomputed on every render by calling isValidUrl for each link; wrap this computation in a useMemo to avoid unnecessary work by memoizing the result with links as the dependency array. Replace the inline const isFormValid = links.every(...) with a const isFormValid = useMemo(() => links.every(link => !link.url || isValidUrl(link.url).valid), [links]) so it only re-evaluates when links changes, similar to the existing charCount memoization.
141-171: Stale closure risk betweenhandleNoteChangeandhandleLinksChange.
handleNoteChangecaptureslinksandhandleLinksChangecapturesnote. If both are called in rapid succession within the same render, the Redux draft dispatch may use a stale snapshot of the other value. For a draft-persistence mechanism this is low-risk (the next keystroke corrects it), but worth noting.A common fix is to use a ref mirror for the "other" value, or consolidate into a single update function. Given the low severity, this is fine to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useNoteModalDraft.ts` around lines 141 - 171, handleNoteChange captures links and handleLinksChange captures note, risking stale values when both fire quickly; fix by introducing refs (e.g., noteRef and linksRef) that are kept in sync inside setNote and setLinks setters, then read from those refs when calling dispatch(updateDraftNote(...)) in both handleNoteChange and handleLinksChange (or alternatively consolidate into a single updateDraft function that accepts both content and links and is always called with current ref values) so dispatch always uses the latest note and links.src/hooks/ui/useEditableUrl.ts (1)
190-247:handleSavetreatsonUrlChangeas synchronous — verify this assumption holds.The save flow calls
onUrlChange(trimmedValue)(line 216) then immediately sets state (lines 219-222). If a consumer ever makesonUrlChangeasynchronous, this will break. The current callers inLinkManagerare synchronous state updates, so this is fine today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/ui/useEditableUrl.ts` around lines 190 - 247, handleSave currently assumes onUrlChange is synchronous; make it robust to async handlers by treating onUrlChange(trimmedValue) as possibly returning a Promise: convert handleSave to an async function, await the result of onUrlChange and wrap it in try/catch to handle rejections (setting error/announcement and clearing isSaving/isSavingRef appropriately), keep isMountedRef checks before updating state, and preserve the success timeout logic; update the dependency array to include any new identifiers you reference (e.g., isMountedRef) and ensure debouncedValidate.cancel and restoreFocus behavior remains unchanged.src/components/Modals/LinkManager.tsx (1)
56-70: Preset loading usesuseState + useEffect—use()with Suspense is preferred per project conventions.Per the project's CLAUDE.md rules, React 19.2 patterns (
use(), Suspense) are preferred overuseState + useEffectfor data fetching. That said, this is a non-blocking side load that silently falls back, so restructuring may not be worth it for this refactoring PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modals/LinkManager.tsx` around lines 56 - 70, Replace the useState/useEffect pattern that fetches presets with React 19.2's use() + Suspense: remove the mounted flag and the getUserPresets().then(...) effect; import use from React and call const userPresets = try { use(getUserPresets()) } catch (e) { undefined } (or wrap the use() call in a try/catch) and render using userPresets ?? builtInPresets so the component falls back silently on error; update references from setUserPresets to use the userPresets local variable and ensure the component is rendered inside a Suspense boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modals/LinkManager.tsx`:
- Around line 164-184: The list key uses the array index
(key={`${link.type}-${index}`}) which can make React reuse wrong EditableUrlItem
instances after operations like handleUndoDelete; change to a stable unique key
by adding an id to each ProjectLink and use that id for the key (or generate a
stable uuid when creating a new link in the code paths that add/splice links
such as handleRemoveUrl, handleUndoDelete, and wherever new links are appended),
and update any construction of links so new items always include that id; keep
EditableUrlItem props (autoEdit, forceExitEdit, onUrlChange/onTypeChange) intact
so component instances remain properly matched.
- Around line 122-134: handleRemoveUrl doesn't adjust editingUrlIndex when an
earlier link is removed, causing the wrong item to be treated as edited; update
handleRemoveUrl to compute and set a corrected editing index: if editingUrlIndex
is not null and the removed index is less than editingUrlIndex, call
setEditingUrlIndex(editingUrlIndex - 1); if the removed index equals
editingUrlIndex keep the existing behavior (set null); then proceed to remove
the link and call onLinksChange with newLinks so forceExitEdit will target the
correct item.
In `@src/hooks/ui/useEditableUrl.ts`:
- Around line 84-92: The UseEditableUrlParams interface currently includes a
disabled property that is never used by the hook; remove disabled from the
UseEditableUrlParams definition and any related types so the hook signature
(useEditableUrl) no longer accepts or suggests the disabled param, and update
any call sites or tests that pass disabled into useEditableUrl to instead handle
disabled at the component/JSX level.
---
Nitpick comments:
In `@src/components/Modals/LinkManager.tsx`:
- Around line 56-70: Replace the useState/useEffect pattern that fetches presets
with React 19.2's use() + Suspense: remove the mounted flag and the
getUserPresets().then(...) effect; import use from React and call const
userPresets = try { use(getUserPresets()) } catch (e) { undefined } (or wrap the
use() call in a try/catch) and render using userPresets ?? builtInPresets so the
component falls back silently on error; update references from setUserPresets to
use the userPresets local variable and ensure the component is rendered inside a
Suspense boundary.
In `@src/hooks/board/useNoteModalDraft.ts`:
- Around line 204-206: The handleClose useCallback is redundant because it only
forwards to onClose; remove the handleClose wrapper in useNoteModalDraft and
pass the original onClose directly to consumers (e.g., where handleClose was
returned/used). Update any references to handleClose to use onClose (in the
hook's return value and any internal usages) and delete the handleClose
declaration to eliminate unnecessary indirection.
- Around line 213-215: The isFormValid boolean is recomputed on every render by
calling isValidUrl for each link; wrap this computation in a useMemo to avoid
unnecessary work by memoizing the result with links as the dependency array.
Replace the inline const isFormValid = links.every(...) with a const isFormValid
= useMemo(() => links.every(link => !link.url || isValidUrl(link.url).valid),
[links]) so it only re-evaluates when links changes, similar to the existing
charCount memoization.
- Around line 141-171: handleNoteChange captures links and handleLinksChange
captures note, risking stale values when both fire quickly; fix by introducing
refs (e.g., noteRef and linksRef) that are kept in sync inside setNote and
setLinks setters, then read from those refs when calling
dispatch(updateDraftNote(...)) in both handleNoteChange and handleLinksChange
(or alternatively consolidate into a single updateDraft function that accepts
both content and links and is always called with current ref values) so dispatch
always uses the latest note and links.
In `@src/hooks/ui/useEditableUrl.ts`:
- Around line 190-247: handleSave currently assumes onUrlChange is synchronous;
make it robust to async handlers by treating onUrlChange(trimmedValue) as
possibly returning a Promise: convert handleSave to an async function, await the
result of onUrlChange and wrap it in try/catch to handle rejections (setting
error/announcement and clearing isSaving/isSavingRef appropriately), keep
isMountedRef checks before updating state, and preserve the success timeout
logic; update the dependency array to include any new identifiers you reference
(e.g., isMountedRef) and ensure debouncedValidate.cancel and restoreFocus
behavior remains unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Modals/LinkManager.tsxsrc/components/Modals/NoteModal.tsxsrc/components/Modals/NoteSection.tsxsrc/components/ui/editable-url-item.tsxsrc/hooks/board/index.tssrc/hooks/board/useNoteModalDraft.tssrc/hooks/ui/useEditableUrl.ts
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/Modals/LinkManager.tsx (2)
164-184: Index-based list key after undo-insert can cause React to reuse wrong component instances.
key={\${link.type}-${index}`}will mismatch afterhandleUndoDeletesplices a link back at a prior position. Consider adding a stableidtoProjectLink` if this causes visible UI glitches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modals/LinkManager.tsx` around lines 164 - 184, The list key for EditableUrlItem uses an unstable index-based key (`key={`${link.type}-${index}`}`) which can cause React to reuse wrong component instances after handleUndoDelete modifies the array; update the ProjectLink items to include and persist a stable unique identifier (e.g., id) and change the key to use that stable id (for example reference the link.id in the key for the EditableUrlItem inside LinkManager.tsx) so that adding/splicing via handleRemoveUrl and handleUndoDelete preserves component identity.
122-134:⚠️ Potential issue | 🟠 Major
editingUrlIndexbecomes stale when deleting a link before the currently-edited item.Deleting index
iwherei < editingUrlIndexshifts all subsequent items down by one, buteditingUrlIndexis not adjusted. This causesforceExitEdit(line 179-181) to target the wrong item.🐛 Proposed fix
const handleRemoveUrl = useCallback( (index: number) => { const linkToDelete = links[index] if (!linkToDelete) return setDeletedLink({ link: linkToDelete, index }) const newLinks = links.filter((_, i) => i !== index) if (editingUrlIndex === index) { setEditingUrlIndex(null) + } else if (editingUrlIndex !== null && index < editingUrlIndex) { + setEditingUrlIndex(editingUrlIndex - 1) } onLinksChange(newLinks) }, [editingUrlIndex, links, onLinksChange], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modals/LinkManager.tsx` around lines 122 - 134, When removing a link in handleRemoveUrl, adjust editingUrlIndex when the deleted index is before the currently edited item: if linkToDelete is found and editingUrlIndex !== null, setEditingUrlIndex(null) when index === editingUrlIndex, otherwise if index < editingUrlIndex call setEditingUrlIndex(editingUrlIndex - 1) so the edited item stays in sync after filtering; perform this update (alongside setDeletedLink and before onLinksChange) to avoid forceExitEdit referencing the wrong item. Reference: handleRemoveUrl, editingUrlIndex, setEditingUrlIndex, setDeletedLink, onLinksChange, links.src/hooks/ui/useEditableUrl.ts (1)
84-92: Unuseddisabledparameter still present.
disabledis declared inUseEditableUrlParams(line 20) but never destructured or used in the hook. The component handles it directly in JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/ui/useEditableUrl.ts` around lines 84 - 92, Remove the unused "disabled" parameter from the type and hook API: delete "disabled" from the UseEditableUrlParams interface definition and from any related type exports, and ensure useEditableUrl's signature no longer expects or references "disabled" (so the hook's parameter list remains { link, onUrlChange, onDelete, onUndoDelete, autoEdit = false, onEditStart, forceExitEdit = false }). Also update any call sites or tests that pass "disabled" into useEditableUrl to stop passing it, since the component handles disabled in JSX.
🧹 Nitpick comments (3)
src/hooks/ui/useEditableUrl.ts (1)
359-362:handleSavePointerDownsetsisCancellingRef— naming is misleading.This handler fires on the save button's
pointerDown, but it setsisCancellingRef = true, which tellshandleBlurto skip its auto-save. The mechanism is correct (prevents double-save from blur + click), but the ref name suggests cancellation when it's really "suppress blur-triggered save."A name like
isSuppressingBlurSaveReforskipBlurSaveRefwould better convey intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/ui/useEditableUrl.ts` around lines 359 - 362, The handler handleSavePointerDown currently sets isCancellingRef.current = true which is misleading; rename the ref to something like skipBlurSaveRef or isSuppressingBlurSaveRef and update all references (including where handleBlur checks it) so the intent — suppressing the blur-triggered auto-save when the save button's pointerDown fires — is clear; change the ref declaration name, its .current assignments in handleSavePointerDown, and the conditional check in handleBlur (and any other usages) to the new identifier and update any related comments to match.src/components/Modals/LinkManager.tsx (1)
56-70: User presets fetch could use the React 19use()+ Suspense pattern.Currently loads presets via
useEffect+setState. The project guidelines prefer React 19.2 patterns (useAPI, Suspense). This is a good candidate for a future refactor but not blocking. Based on learnings, the project prefers React 19.2 APIs likeuseoveruseState + useEffectfor data fetching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modals/LinkManager.tsx` around lines 56 - 70, The useEffect + setState pattern in LinkManager (useEffect, getUserPresets, setUserPresets) should be refactored to React 19's data fetching pattern using the use() hook and Suspense: replace the effect-based fetch with a resource created outside the component (or a server/loader function) that returns the preset promise, call use(getUserPresetsPromise) inside LinkManager to read presets synchronously, and wrap LinkManager (or the presets-consuming subtree) with a Suspense boundary to show a fallback while loading; ensure error handling is moved into an error boundary or the promise rejection is handled by the provider so the old silent-fail behavior is preserved.src/hooks/board/useNoteModalDraft.ts (1)
141-171: Stale closure risk betweenhandleNoteChangeandhandleLinksChange.
handleNoteChangecaptureslinks(line 156) andhandleLinksChangecapturesnote(line 170). Each dispatchesupdateDraftNotewith the other's value from the closure. If rapid edits overlap, one dispatch could overwrite the other's latest value in Redux.This is unlikely in practice (users edit note or links, not both simultaneously), but a
useReffor the latest values would eliminate the risk entirely.♻️ Proposed ref-based fix
+ const noteRef = useRef(note) + noteRef.current = note + const linksRef = useRef(links) + linksRef.current = links + const handleNoteChange = useCallback( (value: string) => { try { const slateValue = parseSlateValue(value) const textLength = getSlateTextLength(slateValue) if (textLength <= NOTE_MAX_LENGTH) { setNote(value) - dispatch(updateDraftNote({ cardId, content: value, links })) + dispatch(updateDraftNote({ cardId, content: value, links: linksRef.current })) } } catch { setNote(value) - dispatch(updateDraftNote({ cardId, content: value, links })) + dispatch(updateDraftNote({ cardId, content: value, links: linksRef.current })) } }, - [cardId, dispatch, links], + [cardId, dispatch], ) const handleLinksChange = useCallback( (newLinks: ProjectLink[]) => { setLinks(newLinks) - dispatch(updateDraftNote({ cardId, content: note, links: newLinks })) + dispatch(updateDraftNote({ cardId, content: noteRef.current, links: newLinks })) }, - [cardId, dispatch, note], + [cardId, dispatch], )This also stabilizes callback identity, preventing
LinkManagerre-renders on every note keystroke.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useNoteModalDraft.ts` around lines 141 - 171, handleNoteChange and handleLinksChange capture stale closures for links and note respectively causing race overwrites when dispatching updateDraftNote; fix by adding refs (e.g., latestNoteRef and latestLinksRef) that are updated whenever setNote or setLinks are called, then have both handlers read from those refs when dispatching updateDraftNote, and remove note/links from the handlers' dependency arrays so handleNoteChange and handleLinksChange remain stable (keep cardId and dispatch in deps); ensure you update the refs in the places that call setNote/setLinks so the refs always reflect latest values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modals/NoteSection.tsx`:
- Line 46: The Label's htmlFor="note" doesn't match any input; update the
NoteSection so the label either is not using htmlFor or is linked via
aria-labelledby: remove htmlFor="note" from the Label in NoteSection (or change
it to id="note-label"), then add aria-labelledby="note-label" to the PlateEditor
root/container (or to the element that PlateEditor renders) so the label and
editor are properly associated; reference the Label component and the
PlateEditor usage in NoteSection when making the change.
---
Duplicate comments:
In `@src/components/Modals/LinkManager.tsx`:
- Around line 164-184: The list key for EditableUrlItem uses an unstable
index-based key (`key={`${link.type}-${index}`}`) which can cause React to reuse
wrong component instances after handleUndoDelete modifies the array; update the
ProjectLink items to include and persist a stable unique identifier (e.g., id)
and change the key to use that stable id (for example reference the link.id in
the key for the EditableUrlItem inside LinkManager.tsx) so that adding/splicing
via handleRemoveUrl and handleUndoDelete preserves component identity.
- Around line 122-134: When removing a link in handleRemoveUrl, adjust
editingUrlIndex when the deleted index is before the currently edited item: if
linkToDelete is found and editingUrlIndex !== null, setEditingUrlIndex(null)
when index === editingUrlIndex, otherwise if index < editingUrlIndex call
setEditingUrlIndex(editingUrlIndex - 1) so the edited item stays in sync after
filtering; perform this update (alongside setDeletedLink and before
onLinksChange) to avoid forceExitEdit referencing the wrong item. Reference:
handleRemoveUrl, editingUrlIndex, setEditingUrlIndex, setDeletedLink,
onLinksChange, links.
In `@src/hooks/ui/useEditableUrl.ts`:
- Around line 84-92: Remove the unused "disabled" parameter from the type and
hook API: delete "disabled" from the UseEditableUrlParams interface definition
and from any related type exports, and ensure useEditableUrl's signature no
longer expects or references "disabled" (so the hook's parameter list remains {
link, onUrlChange, onDelete, onUndoDelete, autoEdit = false, onEditStart,
forceExitEdit = false }). Also update any call sites or tests that pass
"disabled" into useEditableUrl to stop passing it, since the component handles
disabled in JSX.
---
Nitpick comments:
In `@src/components/Modals/LinkManager.tsx`:
- Around line 56-70: The useEffect + setState pattern in LinkManager (useEffect,
getUserPresets, setUserPresets) should be refactored to React 19's data fetching
pattern using the use() hook and Suspense: replace the effect-based fetch with a
resource created outside the component (or a server/loader function) that
returns the preset promise, call use(getUserPresetsPromise) inside LinkManager
to read presets synchronously, and wrap LinkManager (or the presets-consuming
subtree) with a Suspense boundary to show a fallback while loading; ensure error
handling is moved into an error boundary or the promise rejection is handled by
the provider so the old silent-fail behavior is preserved.
In `@src/hooks/board/useNoteModalDraft.ts`:
- Around line 141-171: handleNoteChange and handleLinksChange capture stale
closures for links and note respectively causing race overwrites when
dispatching updateDraftNote; fix by adding refs (e.g., latestNoteRef and
latestLinksRef) that are updated whenever setNote or setLinks are called, then
have both handlers read from those refs when dispatching updateDraftNote, and
remove note/links from the handlers' dependency arrays so handleNoteChange and
handleLinksChange remain stable (keep cardId and dispatch in deps); ensure you
update the refs in the places that call setNote/setLinks so the refs always
reflect latest values.
In `@src/hooks/ui/useEditableUrl.ts`:
- Around line 359-362: The handler handleSavePointerDown currently sets
isCancellingRef.current = true which is misleading; rename the ref to something
like skipBlurSaveRef or isSuppressingBlurSaveRef and update all references
(including where handleBlur checks it) so the intent — suppressing the
blur-triggered auto-save when the save button's pointerDown fires — is clear;
change the ref declaration name, its .current assignments in
handleSavePointerDown, and the conditional check in handleBlur (and any other
usages) to the new identifier and update any related comments to match.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Modals/LinkManager.tsxsrc/components/Modals/NoteModal.tsxsrc/components/Modals/NoteSection.tsxsrc/components/ui/editable-url-item.tsxsrc/hooks/board/index.tssrc/hooks/board/useNoteModalDraft.tssrc/hooks/ui/useEditableUrl.ts
- Fix stale editingUrlIndex after link deletion in LinkManager - Remove unused disabled param from useEditableUrl interface - Replace invalid htmlFor="note" with id="note-label" in NoteSection
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/hooks/ui/useEditableUrl.ts (1)
7-7: Consider decoupling hook types from component modules.
useEditableUrlimportsProjectLinkfrom a UI component file, which inverts dependency direction (hooks → components). MovingProjectLinkto a shared types module would keep boundaries cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/ui/useEditableUrl.ts` at line 7, The hook useEditableUrl currently imports the ProjectLink type from the UI component editable-url-item which inverts dependencies; extract ProjectLink into a shared types module (e.g., types/project.ts or types/ui.ts), export the type there, and update useEditableUrl to import ProjectLink from that new shared module; also update the component file editable-url-item to import ProjectLink from the shared types module and run a quick search to update any other imports referencing the old location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modals/LinkManager.tsx`:
- Around line 75-77: handleAddUrl appends a blank link but never updates
editingUrlIndex, so the newly added item immediately gets forceExitEdit; after
adding the new link (in handleAddUrl and the other add-link handler around the
second occurrence), call setEditingUrlIndex(links.length) (or the equivalent
state setter) so the new item's index becomes the active editingUrlIndex, and
ensure you use the current links array when computing the new index before
calling onLinksChange.
- Around line 141-148: The undo handler (handleUndoDelete) reinserts a deleted
link but doesn't update editingUrlIndex, so if the restored item is inserted
before the currently edited item the edit index becomes stale; modify
handleUndoDelete to after computing index (const { link, index } = deletedLink)
and before calling setDeletedLink(null) / onLinksChange(newLinks) check if
editingUrlIndex is not null/undefined and index <= editingUrlIndex then call
setEditingUrlIndex(prev => prev + 1) (or set to index-adjusted value) to shift
the edit index right by one; ensure you reference the state setters
editingUrlIndex and setEditingUrlIndex in the useCallback deps list so the
closure sees the latest value.
In `@src/hooks/ui/useEditableUrl.ts`:
- Around line 271-276: The save path in useEditableUrl can bypass in-flight
debounced validation because it only checks the mutable error state; modify the
block where trimmedEdit is compared to trimmedLink (and
debouncedValidate.cancel() is called) to run the underlying validation
synchronously for trimmedEdit before setting isSavingRef.current or calling
onUrlChange: invoke the same validator used by debouncedValidate (e.g.,
validateUrl or the validation helper inside this hook) and if it returns an
error, set the error state and abort (do not call onUrlChange); only proceed to
set isSavingRef.current = true, cancel debouncedValidate, and call
onUrlChange(trimmedEdit) when the synchronous validation passes and
isMountedRef.current remains true.
---
Nitpick comments:
In `@src/hooks/ui/useEditableUrl.ts`:
- Line 7: The hook useEditableUrl currently imports the ProjectLink type from
the UI component editable-url-item which inverts dependencies; extract
ProjectLink into a shared types module (e.g., types/project.ts or types/ui.ts),
export the type there, and update useEditableUrl to import ProjectLink from that
new shared module; also update the component file editable-url-item to import
ProjectLink from the shared types module and run a quick search to update any
other imports referencing the old location.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Modals/LinkManager.tsxsrc/components/Modals/NoteSection.tsxsrc/components/ui/editable-url-item.tsxsrc/hooks/ui/useEditableUrl.ts
- Set editingUrlIndex when adding new URL to prevent forceExitEdit conflict - Adjust editingUrlIndex on undo reinsert to prevent stale index - Add synchronous validation in force-exit path to prevent invalid URL save
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Issue #151 — NoteModal Decomposition
src/hooks/board/useNoteModalDraft.tssrc/components/Modals/NoteSection.tsxsrc/components/Modals/LinkManager.tsxsrc/components/Modals/NoteModal.tsxIssue #152 — EditableUrlItem Hook Extraction
src/hooks/ui/useEditableUrl.tssrc/components/ui/editable-url-item.tsxTest plan
Closes #151, closes #152
Summary by CodeRabbit
New Features
Quality / UX