feat(p4): implement F4 auto-icons and F6 organization workflows#16
Merged
ryota-murakami merged 4 commits intomainfrom Feb 17, 2026
Merged
feat(p4): implement F4 auto-icons and F6 organization workflows#16ryota-murakami merged 4 commits intomainfrom
ryota-murakami merged 4 commits intomainfrom
Conversation
Add F6 sidebar drag-and-drop organization with optimistic PUT /user persistence, inline rename, and full direct context-menu actions. Add F4 auto-icon resolution fallback chain and comprehensive unit/E2E coverage for F4/F6 acceptance criteria.
|
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. 📝 WalkthroughWalkthroughサイドバーに対するドラッグ＆ドロップ、インライン編集、色変更、楽観更新（ロールバック対応）を追加し、ブックマーク追加ダイアログでのURL入力時に自動ファビコン解決（/raindrop/suggest呼び出し＋Googleフォールバック）を実装。関連するユニット／E2Eテストとテスト用APIモックを追加。 Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as LeftSidebar / MainApp
participant DnD as DnDKit (`@dnd-kit`)
participant Org as CollectionOrg (moveRootCollection)
participant API as Server (PUT /user)
User->>UI: コレクションをドラッグ
UI->>DnD: drag start (collection DnD id)
DnD->>UI: update drop indicator / overlay
User->>UI: ドロップ実行
UI->>Org: moveRootCollection(groups, id, targetGroup, index)
Org-->>UI: 新しい groups 配列
UI->>API: PUT /user (toUserGroupPayload)
API-->>UI: success / error
alt success
UI->>UI: 状態確定
else error
UI->>UI: rollback to previous groups
end
sequenceDiagram
participant User
participant Dialog as AddBookmarkDialog
participant Parser as DebouncedParser
participant Favicon as resolveIcon()
participant Suggest as /raindrop/suggest
participant Google as GoogleFaviconCDN
User->>Dialog: URL を入力
Dialog->>Parser: parseUrl() (debounce ~500ms, requestId)
Parser->>Parser: ドメイン抽出 / content-type 判定 / title 補完
Parser->>Favicon: resolveIcon(url)
Favicon->>Suggest: GET /raindrop/suggest?url={url}
alt suggest returns icon
Suggest-->>Favicon: { item.meta.icon }
Favicon-->>Dialog: アイコン URL
else no icon or error
Favicon->>Google: https://www.google.com/s2/favicons?domain={domain}&sz=64
Google-->>Favicon: fallback URL
Favicon-->>Dialog: フォールバック URL
end
Dialog->>Dialog: favicon を描画 (img or domain-initial)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/raindrop/favicon-icon.tsx (1)
62-86:⚠️ Potential issue | 🟠 Major
urlが変更されてもimgErrorがリセットされないバグ。URL A で画像読み込み失敗 →
imgError=true→ URL B に変わってもimgErrorがtrueのまま残り、画像を試行せずイニシャルフォールバックが表示される。🐛 修正案: url 変更時に imgError をリセット
+ import React, { useState, useMemo, useEffect } from 'react' - import React, { useState, useMemo } from 'react'const [imgError, setImgError] = useState(false) + useEffect(() => setImgError(false), [url]) const domain = extractDomain(url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/favicon-icon.tsx` around lines 62 - 86, The bug is that imgError remains true after a failed load and is not reset when url/domain changes; update the component to reset imgError whenever the url (or derived domain) changes by adding a useEffect that watches url or domain and calls setImgError(false); locate the state variables imgError and setImgError and the derived domain/getFaviconUrl logic in favicon-icon.tsx and implement the reset there so a new favicon fetch is attempted for the new url.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/specs/dnd.spec.ts`:
- Around line 45-56: The dragCollectionToTarget helper's DnD sequence (function
dragCollectionToTarget) can be flaky in CI due to timing; add short waits (e.g.
page.waitForTimeout(50)) between the key steps—after awaiting source.hover(),
after page.mouse.down(), and between the two target.hover() calls—so the dnd-kit
reacts reliably to the simulated mouse events and stabilizes CI runs.
In `@src/components/raindrop/add-bookmark-dialog.tsx`:
- Around line 150-164: useParsedFaviconReset only clears parsedFavicon when
autoIcon becomes false but doesn't trigger re-fetch when autoIcon is toggled
back to true; update logic so toggling autoIcon from false→true initiates
resolveIcon for the current URL: modify useParsedFaviconReset (and/or the parent
hook that holds parsedFavicon and parsedUrl) to detect transitions of autoIcon
to true and call the existing resolveIcon function (or dispatch the same action
that resolves the icon), ensuring you reference useParsedFaviconReset,
setParsedFavicon, parsedFavicon, parsedUrl and resolveIcon so the favicon is
re-fetched when autoIcon is turned back on.
- Around line 386-396: The img onError currently mutates the DOM via (e.target
as HTMLImageElement).style.display = 'none'; replace this with a React state
flag: add a faviconError state (e.g., const [faviconError, setFaviconError] =
useState(false)), change the onError to call setFaviconError(true), and update
the conditional render to only show the <img
data-testid="bookmark-favicon-preview" src={parsedFavicon} .../> when
parsedFavicon is truthy and faviconError is false; also reset faviconError to
false in a useEffect that depends on parsedFavicon so the preview reappears when
parsedFavicon changes.
- Around line 239-290: parseUrl is using watch('title') inside a useCallback
which can unintentionally bypass the memoization intent — replace the runtime
form subscription with a snapshot via getValues: destructure getValues from
useForm, change watch('title') to getValues('title') inside parseUrl, and update
the useCallback dependency array to include getValues (remove watch if no longer
used) so parseUrl's memoization semantics are explicit; keep the rest of
parseUrl (parseRequestIdRef, setIsParsing, resolveIcon, setParsedFavicon)
unchanged.
In `@src/components/raindrop/drag-preview.tsx`:
- Around line 30-33: The hardcoded default color '#8b5cf6' used in the
DragPreview JSX should be extracted to a named constant (e.g.
DEFAULT_COLLECTION_COLOR) and referenced instead of the literal; update the
style expression that uses collection.color || '#8b5cf6' to collection.color ||
DEFAULT_COLLECTION_COLOR and define/export the constant either at the top of
src/components/raindrop/drag-preview.tsx or from a shared theme/constants module
so other components can reuse the same default.
In `@src/components/raindrop/favicon-icon.tsx`:
- Around line 62-86: The bug is that imgError remains true after a failed load
and is not reset when url/domain changes; update the component to reset imgError
whenever the url (or derived domain) changes by adding a useEffect that watches
url or domain and calls setImgError(false); locate the state variables imgError
and setImgError and the derived domain/getFaviconUrl logic in favicon-icon.tsx
and implement the reset there so a new favicon fetch is attempted for the new
url.
In `@src/components/raindrop/left-sidebar.tsx`:
- Around line 482-496: The menu currently renders raw hex strings from
COLLECTION_COLOR_OPTIONS (and uses those strings as keys/handler lookups),
causing poor screen-reader output; change COLLECTION_COLOR_OPTIONS to an array
of objects like { value: '#ef4444', label: 'Red' } (or update wherever it’s
defined) and update the mapping in ContextMenuSubContent to use color.value for
key, onClick (colorHandlers.get(color.value)), and style={{ backgroundColor:
color.value }}, while rendering color.label inside ContextMenuItem so screen
readers read a human-friendly name; ensure any other uses of
COLLECTION_COLOR_OPTIONS or colorHandlers refer to the .value/.label shape.
- Around line 854-876: The optimisticGroups state isn't being explicitly cleared
on successful server responses, causing stale optimistic UI to persist until RTK
Query cache updates; update each optimistic handler (handlePersistGroups,
handleDeleteWithOptimistic, handleSubmitInlineRename, handleChangeColor) to call
setOptimisticGroups(null) after the success path completes (after awaiting or in
the .then of the request), so displayedGroups (which falls back via
serializeRootStructure) immediately uses server-backed groups; ensure you call
setOptimisticGroups(null) only on success and keep existing optimistic updates
intact on failure.
- Around line 420-439: Add a ref-based cancel flag in the LeftSidebar component
(e.g., cancelledRef = useRef(false)); set cancelledRef.current = false in
handleStartInlineRename, set cancelledRef.current = true in
onCancelInlineRename, and update the input's onBlur handler to check this flag
(and/or verify editingCollectionId still matches) before calling
handleSubmitRename so blur fired during an Escape-triggered unmount does not
submit an outdated rename; ensure the flag is reset appropriately after
submit/cancel.
- Around line 1-101:
ファイルが大きく可読性が低いため、CollectionRow、SortableRootCollectionRow、GroupSection、SystemCollectionItem
の各コンポーネントを別ファイルに分割してください。具体的には左サイドバーのファイルから各コンポーネント定義（関数コンポーネントとその props
型、関連ユーティリティ・フックのインポート）を切り出してそれぞれ例えば CollectionRow.tsx
等に移動し、元ファイルでは新しいモジュールからインポートするように変更し、既存の依存（getCollectionDndId や
parseCollectionDndId など）や CSS/ユーティリティ参照が正しく解決されることを確認してください。
- Around line 267-322: CollectionRow currently accepts 20+ individual props and
the same long list is threaded through SortableRootCollectionRow, GroupSection,
and LeftSidebar; refactor by grouping related props into cohesive objects (e.g.,
inlineRenameProps: { editingCollectionId, editingName, onStartInlineRename,
onSubmitInlineRename, onCancelInlineRename, onEditingNameChange } and
contextMenuActions or collectionCallbacks: { onSelectCollection,
onToggleCollection, onDeleteCollection, onMoveCollectionToGroup,
onChangeCollectionColor }) or move these into a Context
(InlineRenameContext/CollectionActionsContext) so CollectionRow signature is
simplified; update the component prop type definition, replace usage sites
(SortableRootCollectionRow, GroupSection, LeftSidebar) to pass the new grouped
objects or provide the context provider, and adjust all call sites and tests to
read from the new grouped props or context.
In `@src/hooks/useCollectionsCrud.test.ts`:
- Around line 51-88: The tests only verify that no exceptions are thrown; after
calling updateUserGroups (and optionally renameCollection and recolorCollection)
assert the mock store was updated: call result.current.updateUserGroups(groups)
then check mockStore.user (or the store accessor used in
renderHookWithProviders) contains the new groups array (matching
ids/names/collections), and for renameCollection and recolorCollection assert
the target collection (id '100') in mockStore.user now has the new name 'Renamed
in Sidebar' and color '#3b82f6' respectively so the tests validate state changes
rather than just absence of errors.
In `@src/hooks/useCollectionsCrud.ts`:
- Around line 157-170: The code is using a local type assertion in
recolorCollection to add a color field because CollectionUpdate currently lacks
it; update the CollectionUpdate type in src/store/api/raindropApi.ts to include
color?: string (or appropriate optional/required type) so callers like
recolorCollection can pass { color } without casting, then remove the ad-hoc "as
CollectionUpdate & { color?: string }" in the recolorCollection function and
rely on the updated CollectionUpdate type for type safety.
- Around line 125-138: updateUserGroups currently shows a different error
pattern (it toasts and then throws) compared to other helpers like
createCollection that only toast; make this explicit by adding an inline comment
above the updateUserGroups function noting that the throw is intentional to
force caller-side optimistic rollback and that callers must wrap calls in
try/catch, e.g. reference updateUserGroups and createCollection to explain the
behavioral difference so future readers understand why this function throws in
addition to showing a toast.
In `@src/lib/collection-organization.ts`:
- Around line 45-48: parseCollectionId currently treats an empty string as 0
because Number('') === 0; update parseCollectionId to first trim the input and
return null if the trimmed string is empty or only whitespace, then proceed to
Number(...) and the Number.isFinite check (refer to function parseCollectionId
and variable parsed) so empty/whitespace inputs are not interpreted as the
special 0 collection ID.
- Around line 239-247: The toUserGroupPayload function currently hardcodes
hidden: false causing Group hidden state to be lost; update toUserGroupPayload
so it uses the source group's hidden flag when present (e.g., group.hidden ??
false) instead of always false, leaving the rest of the mapping (title, sort,
collections using parseCollectionId) unchanged; reference Group and
UserGroupPayload types and the toUserGroupPayload function when making the
change so future additions of a hidden property are respected.
🧹 Nitpick comments (9)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts`: - Around line 45-56: The dragCollectionToTarget helper's DnD sequence (function dragCollectionToTarget) can be flaky in CI due to timing; add short waits (e.g. page.waitForTimeout(50)) between the key steps—after awaiting source.hover(), after page.mouse.down(), and between the two target.hover() calls—so the dnd-kit reacts reliably to the simulated mouse events and stabilizes CI runs. In `@src/components/raindrop/add-bookmark-dialog.tsx`: - Around line 386-396: The img onError currently mutates the DOM via (e.target as HTMLImageElement).style.display = 'none'; replace this with a React state flag: add a faviconError state (e.g., const [faviconError, setFaviconError] = useState(false)), change the onError to call setFaviconError(true), and update the conditional render to only show the <img data-testid="bookmark-favicon-preview" src={parsedFavicon} .../> when parsedFavicon is truthy and faviconError is false; also reset faviconError to false in a useEffect that depends on parsedFavicon so the preview reappears when parsedFavicon changes. - Around line 239-290: parseUrl is using watch('title') inside a useCallback which can unintentionally bypass the memoization intent — replace the runtime form subscription with a snapshot via getValues: destructure getValues from useForm, change watch('title') to getValues('title') inside parseUrl, and update the useCallback dependency array to include getValues (remove watch if no longer used) so parseUrl's memoization semantics are explicit; keep the rest of parseUrl (parseRequestIdRef, setIsParsing, resolveIcon, setParsedFavicon) unchanged. In `@src/components/raindrop/drag-preview.tsx`: - Around line 30-33: The hardcoded default color '#8b5cf6' used in the DragPreview JSX should be extracted to a named constant (e.g. DEFAULT_COLLECTION_COLOR) and referenced instead of the literal; update the style expression that uses collection.color || '#8b5cf6' to collection.color || DEFAULT_COLLECTION_COLOR and define/export the constant either at the top of src/components/raindrop/drag-preview.tsx or from a shared theme/constants module so other components can reuse the same default. In `@src/components/raindrop/left-sidebar.tsx`: - Around line 1-101: ファイルが大きく可読性が低いため、CollectionRow、SortableRootCollectionRow、GroupSection、SystemCollectionItem の各コンポーネントを別ファイルに分割してください。具体的には左サイドバーのファイルから各コンポーネント定義（関数コンポーネントとその props 型、関連ユーティリティ・フックのインポート）を切り出してそれぞれ例えば CollectionRow.tsx 等に移動し、元ファイルでは新しいモジュールからインポートするように変更し、既存の依存（getCollectionDndId や parseCollectionDndId など）や CSS/ユーティリティ参照が正しく解決されることを確認してください。 - Around line 267-322: CollectionRow currently accepts 20+ individual props and the same long list is threaded through SortableRootCollectionRow, GroupSection, and LeftSidebar; refactor by grouping related props into cohesive objects (e.g., inlineRenameProps: { editingCollectionId, editingName, onStartInlineRename, onSubmitInlineRename, onCancelInlineRename, onEditingNameChange } and contextMenuActions or collectionCallbacks: { onSelectCollection, onToggleCollection, onDeleteCollection, onMoveCollectionToGroup, onChangeCollectionColor }) or move these into a Context (InlineRenameContext/CollectionActionsContext) so CollectionRow signature is simplified; update the component prop type definition, replace usage sites (SortableRootCollectionRow, GroupSection, LeftSidebar) to pass the new grouped objects or provide the context provider, and adjust all call sites and tests to read from the new grouped props or context. In `@src/hooks/useCollectionsCrud.test.ts`: - Around line 51-88: The tests only verify that no exceptions are thrown; after calling updateUserGroups (and optionally renameCollection and recolorCollection) assert the mock store was updated: call result.current.updateUserGroups(groups) then check mockStore.user (or the store accessor used in renderHookWithProviders) contains the new groups array (matching ids/names/collections), and for renameCollection and recolorCollection assert the target collection (id '100') in mockStore.user now has the new name 'Renamed in Sidebar' and color '#3b82f6' respectively so the tests validate state changes rather than just absence of errors. In `@src/hooks/useCollectionsCrud.ts`: - Around line 125-138: updateUserGroups currently shows a different error pattern (it toasts and then throws) compared to other helpers like createCollection that only toast; make this explicit by adding an inline comment above the updateUserGroups function noting that the throw is intentional to force caller-side optimistic rollback and that callers must wrap calls in try/catch, e.g. reference updateUserGroups and createCollection to explain the behavioral difference so future readers understand why this function throws in addition to showing a toast. In `@src/lib/collection-organization.ts`: - Around line 239-247: The toUserGroupPayload function currently hardcodes hidden: false causing Group hidden state to be lost; update toUserGroupPayload so it uses the source group's hidden flag when present (e.g., group.hidden ?? false) instead of always false, leaving the rest of the mapping (title, sort, collections using parseCollectionId) unchanged; reference Group and UserGroupPayload types and the toUserGroupPayload function when making the change so future additions of a hidden property are respected.src/components/raindrop/drag-preview.tsx (1)
30-33: マジックカラー値の定数化を検討
'#8b5cf6'がハードコードされている。他の箇所でも同じデフォルトカラーを使う可能性があるなら、定数として抽出しておくとメンテナンスしやすい。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/drag-preview.tsx` around lines 30 - 33, The hardcoded default color '#8b5cf6' used in the DragPreview JSX should be extracted to a named constant (e.g. DEFAULT_COLLECTION_COLOR) and referenced instead of the literal; update the style expression that uses collection.color || '#8b5cf6' to collection.color || DEFAULT_COLLECTION_COLOR and define/export the constant either at the top of src/components/raindrop/drag-preview.tsx or from a shared theme/constants module so other components can reuse the same default.src/hooks/useCollectionsCrud.ts (1)
125-138:updateUserGroupsのエラーハンドリングパターン他のメソッド（
createCollection等）は toast のみだが、こちらは toast + throw している。これはオプティミスティックロールバック用と理解できるが、呼び出し側でtry/catchが必要になる点を認識しておくこと。パターンの違いが意図的であることをコメントに明示しておくと良い。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCollectionsCrud.ts` around lines 125 - 138, updateUserGroups currently shows a different error pattern (it toasts and then throws) compared to other helpers like createCollection that only toast; make this explicit by adding an inline comment above the updateUserGroups function noting that the throw is intentional to force caller-side optimistic rollback and that callers must wrap calls in try/catch, e.g. reference updateUserGroups and createCollection to explain the behavioral difference so future readers understand why this function throws in addition to showing a toast.src/hooks/useCollectionsCrud.test.ts (1)
51-88: スモークテストとしては OK、アサーション追加を検討新規テストは MSW ハンドラーとの接続確認として機能しているが、全て「例外が投げられないこと」の暗黙的な検証のみ。既存テストと同じパターンではあるが、
updateUserGroupsのテストではmockStore.userの更新後状態を検証するアサーションがあるとより堅牢になる。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCollectionsCrud.test.ts` around lines 51 - 88, The tests only verify that no exceptions are thrown; after calling updateUserGroups (and optionally renameCollection and recolorCollection) assert the mock store was updated: call result.current.updateUserGroups(groups) then check mockStore.user (or the store accessor used in renderHookWithProviders) contains the new groups array (matching ids/names/collections), and for renameCollection and recolorCollection assert the target collection (id '100') in mockStore.user now has the new name 'Renamed in Sidebar' and color '#3b82f6' respectively so the tests validate state changes rather than just absence of errors.src/lib/collection-organization.ts (1)
239-247:toUserGroupPayloadでhiddenが常にfalse全グループの
hiddenをfalseに固定しているため、グループの非表示状態が永続化されない。現時点で非表示機能が未実装であれば問題ないが、将来Group型にhiddenプロパティが追加された際にここの更新を忘れるリスクがある。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/collection-organization.ts` around lines 239 - 247, The toUserGroupPayload function currently hardcodes hidden: false causing Group hidden state to be lost; update toUserGroupPayload so it uses the source group's hidden flag when present (e.g., group.hidden ?? false) instead of always false, leaving the rest of the mapping (title, sort, collections using parseCollectionId) unchanged; reference Group and UserGroupPayload types and the toUserGroupPayload function when making the change so future additions of a hidden property are respected.e2e/specs/dnd.spec.ts (1)
45-56:dragCollectionToTargetの DnD シミュレーションについて。マウスイベントベースの DnD テストは dnd-kit との組み合わせで動作するが、CI 環境でのタイミングに依存する可能性がある。
hover()間に短い待機（page.waitForTimeout(50)等）を入れると安定性が向上する場合がある。現状 CI で安定しているなら問題なし。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts` around lines 45 - 56, The dragCollectionToTarget helper's DnD sequence (function dragCollectionToTarget) can be flaky in CI due to timing; add short waits (e.g. page.waitForTimeout(50)) between the key steps—after awaiting source.hover(), after page.mouse.down(), and between the two target.hover() calls—so the dnd-kit reacts reliably to the simulated mouse events and stabilizes CI runs.src/components/raindrop/add-bookmark-dialog.tsx (2)
386-396: favicon プレビュー画像のonErrorハンドラ:display: noneでなく状態で制御すべき。Line 393 で DOM を直接操作して
style.display = 'none'にしている。React の宣言的パターンから外れるため、useStateでエラーフラグを管理し、条件レンダリングするほうが堅牢。♻️ 提案 (任意)
状態変数
faviconErrorを追加し:{parsedFavicon && ( + faviconError ? null : <img data-testid="bookmark-favicon-preview" src={parsedFavicon} alt="" className="h-5 w-5 shrink-0 rounded-sm" - onError={(e) => { - ;(e.target as HTMLImageElement).style.display = 'none' - }} + onError={() => setFaviconError(true)} /> )}
parsedFaviconが変更されたらfaviconErrorをリセットする。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/add-bookmark-dialog.tsx` around lines 386 - 396, The img onError currently mutates the DOM via (e.target as HTMLImageElement).style.display = 'none'; replace this with a React state flag: add a faviconError state (e.g., const [faviconError, setFaviconError] = useState(false)), change the onError to call setFaviconError(true), and update the conditional render to only show the <img data-testid="bookmark-favicon-preview" src={parsedFavicon} .../> when parsedFavicon is truthy and faviconError is false; also reset faviconError to false in a useEffect that depends on parsedFavicon so the preview reappears when parsedFavicon changes.
239-290:watchをuseCallbackの依存配列に含めているが、コールバック内部での使用方法に注意。Line 271 で
watch('title')をコールバック内で呼んでいる。react-hook-formのwatchは安定した参照なのでリレンダーのたびにparseUrlが再生成されることはないが、コールバック内部でwatchを使うと、useCallbackのメモ化をすり抜けて常に最新のフォーム値を取得する点は意図的な設計か確認しておきたい。getValues('title')のほうが「コールバック内でスナップショットを取る」意図がより明確になる。♻️ 提案 (任意)
- const currentTitle = watch('title') + const currentTitle = getValues('title')
useFormからgetValuesも destructure する必要あり。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/add-bookmark-dialog.tsx` around lines 239 - 290, parseUrl is using watch('title') inside a useCallback which can unintentionally bypass the memoization intent — replace the runtime form subscription with a snapshot via getValues: destructure getValues from useForm, change watch('title') to getValues('title') inside parseUrl, and update the useCallback dependency array to include getValues (remove watch if no longer used) so parseUrl's memoization semantics are explicit; keep the rest of parseUrl (parseRequestIdRef, setIsParsing, resolveIcon, setParsedFavicon) unchanged.src/components/raindrop/left-sidebar.tsx (2)
1-101: ファイルサイズが大きい (~1300行)。将来的な分割を検討。
CollectionRow,SortableRootCollectionRow,GroupSection,SystemCollectionItemは独立した責務を持つコンポーネントなので、個別ファイルへの抽出が可読性と保守性を向上させる。現時点の PR スコープでは任意。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/left-sidebar.tsx` around lines 1 - 101, ファイルが大きく可読性が低いため、CollectionRow、SortableRootCollectionRow、GroupSection、SystemCollectionItem の各コンポーネントを別ファイルに分割してください。具体的には左サイドバーのファイルから各コンポーネント定義（関数コンポーネントとその props 型、関連ユーティリティ・フックのインポート）を切り出してそれぞれ例えば CollectionRow.tsx 等に移動し、元ファイルでは新しいモジュールからインポートするように変更し、既存の依存（getCollectionDndId や parseCollectionDndId など）や CSS/ユーティリティ参照が正しく解決されることを確認してください。
267-322:CollectionRowの prop 数が非常に多い（20+）。
CollectionRow→SortableRootCollectionRow→GroupSection→LeftSidebarと同一の props が 4 層にわたって受け渡されている。Context API でインライン編集状態やコールバック群をまとめるか、props オブジェクトをグルーピングすることで可読性と保守性が改善する。例:
inlineRenameProps,contextMenuActionsのようなオブジェクトにまとめる。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/left-sidebar.tsx` around lines 267 - 322, CollectionRow currently accepts 20+ individual props and the same long list is threaded through SortableRootCollectionRow, GroupSection, and LeftSidebar; refactor by grouping related props into cohesive objects (e.g., inlineRenameProps: { editingCollectionId, editingName, onStartInlineRename, onSubmitInlineRename, onCancelInlineRename, onEditingNameChange } and contextMenuActions or collectionCallbacks: { onSelectCollection, onToggleCollection, onDeleteCollection, onMoveCollectionToGroup, onChangeCollectionColor }) or move these into a Context (InlineRenameContext/CollectionActionsContext) so CollectionRow signature is simplified; update the component prop type definition, replace usage sites (SortableRootCollectionRow, GroupSection, LeftSidebar) to pass the new grouped objects or provide the context provider, and adjust all call sites and tests to read from the new grouped props or context.
Address auto-icon re-fetch toggle behavior, inline rename cancel/blur race handling, accessible color labels, optimistic state success resets, CollectionUpdate color typing, and empty collection-id parsing edge cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/specs/dnd.spec.ts`:
- Around line 182-216: The artificial 600ms delay inside the route handler (in
the anonymous async route callback that calls page.waitForTimeout(600) before
route.fulfill) is brittle; replace it with a more reliable guard or a larger
timeout. Either wait for a deterministic signal (e.g., use page.waitForResponse
with the PUT to '/api.raindrop.io/rest/v1/user' or another test-controlled
event) before calling route.fulfill, or increase the delay to a safer value
(e.g., 2000ms) and make it configurable so the optimistic-move assertions
(getGroupRootCollectionIds polls) remain stable; update the anonymous route
handler where failedOnce, route.request(), page.waitForTimeout(600), and
route.fulfill are used accordingly.
In `@src/components/raindrop/add-bookmark-dialog.tsx`:
- Around line 142-148: useDebouncedUrlParsing currently calls the async parseUrl
inside setTimeout and discards its Promise, risking unhandled rejections; change
the parseUrl parameter type to (url: string) => Promise<void> and, inside the
timeout callback, invoke parseUrl(url).catch(err => {/* optional logging or noop
*/}) (or use void parseUrl(url).catch(...)) so any rejection is handled; keep
URL_PARSE_DEBOUNCE_MS and the rest of useEffect unchanged.
In `@src/components/raindrop/left-sidebar.tsx`:
- Around line 421-449: The inline edit input lacks an accessibility label: add
an aria-label to the input rendered when isEditing is true (in the same JSX
block that uses editingName, onEditingNameChange, handleSubmitRename,
onCancelInlineRename) so screen readers know what is being edited; for example
set a descriptive label like "Rename collection {collection.name}" (or a
localized variant) via the input's aria-label prop to provide context for
assistive tech.
- Around line 268-538: Too many separate props passed into CollectionRow; group
related props into a smaller props object to improve readability and
maintenance. Create an InlineEditProps type (including editingCollectionId,
editingName, onEditingNameChange, onStartInlineRename, onSubmitInlineRename,
onCancelInlineRename) and accept it as a single prop (e.g., inlineEdit) in
CollectionRow's parameter destructuring; update all usages inside CollectionRow
to reference inlineEdit.editingName etc.; update the parent render and the
recursive <CollectionRow ... /> calls to pass inlineEdit={inlineEdit} instead of
the six separate props; optionally group other handler props similarly (e.g.,
movement/color handlers) following the same pattern and adjust the memo
signature and any external callers to match.
- Around line 1094-1134: The handler handleDragEnd is declared async but
DndContext.onDragEnd expects a synchronous (void) callback, risking unhandled
promise rejections from await handlePersistGroups; change handle to remain
synchronous by removing async and explicitly discard or handle the Promise:
either wrap the persistence call in a try/catch inside handleDragEnd (using a
non-async IIFE if you want to keep await semantics) or call
handlePersistGroups(nextGroups, displayedGroups).catch(err => { process/log the
error }) or prefix with void to intentionally ignore the Promise; ensure you
reference handleDragEnd, DndContext.onDragEnd, and handlePersistGroups when
making the change.
In `@src/hooks/useCollectionsCrud.ts`:
- Around line 125-138: The updateUserGroups callback currently both shows a
toast and throws on failure (in updateUserGroups using putUser), which can lead
to duplicate error toasts when callers like handlePersistGroups in
left-sidebar.tsx also handle errors; remove the toast.error call from
updateUserGroups and let it simply throw or return the failure so callers can
decide how to surface UI errors; update the implementation of updateUserGroups
(and any callers expecting the toast) to rely on caller-side toast handling
(e.g., in handlePersistGroups) and keep putUser usage the same.
🧹 Nitpick comments (4)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts`: - Around line 182-216: The artificial 600ms delay inside the route handler (in the anonymous async route callback that calls page.waitForTimeout(600) before route.fulfill) is brittle; replace it with a more reliable guard or a larger timeout. Either wait for a deterministic signal (e.g., use page.waitForResponse with the PUT to '/api.raindrop.io/rest/v1/user' or another test-controlled event) before calling route.fulfill, or increase the delay to a safer value (e.g., 2000ms) and make it configurable so the optimistic-move assertions (getGroupRootCollectionIds polls) remain stable; update the anonymous route handler where failedOnce, route.request(), page.waitForTimeout(600), and route.fulfill are used accordingly. In `@src/components/raindrop/left-sidebar.tsx`: - Around line 268-538: Too many separate props passed into CollectionRow; group related props into a smaller props object to improve readability and maintenance. Create an InlineEditProps type (including editingCollectionId, editingName, onEditingNameChange, onStartInlineRename, onSubmitInlineRename, onCancelInlineRename) and accept it as a single prop (e.g., inlineEdit) in CollectionRow's parameter destructuring; update all usages inside CollectionRow to reference inlineEdit.editingName etc.; update the parent render and the recursive <CollectionRow ... /> calls to pass inlineEdit={inlineEdit} instead of the six separate props; optionally group other handler props similarly (e.g., movement/color handlers) following the same pattern and adjust the memo signature and any external callers to match. - Around line 1094-1134: The handler handleDragEnd is declared async but DndContext.onDragEnd expects a synchronous (void) callback, risking unhandled promise rejections from await handlePersistGroups; change handle to remain synchronous by removing async and explicitly discard or handle the Promise: either wrap the persistence call in a try/catch inside handleDragEnd (using a non-async IIFE if you want to keep await semantics) or call handlePersistGroups(nextGroups, displayedGroups).catch(err => { process/log the error }) or prefix with void to intentionally ignore the Promise; ensure you reference handleDragEnd, DndContext.onDragEnd, and handlePersistGroups when making the change. In `@src/hooks/useCollectionsCrud.ts`: - Around line 125-138: The updateUserGroups callback currently both shows a toast and throws on failure (in updateUserGroups using putUser), which can lead to duplicate error toasts when callers like handlePersistGroups in left-sidebar.tsx also handle errors; remove the toast.error call from updateUserGroups and let it simply throw or return the failure so callers can decide how to surface UI errors; update the implementation of updateUserGroups (and any callers expecting the toast) to rely on caller-side toast handling (e.g., in handlePersistGroups) and keep putUser usage the same.src/hooks/useCollectionsCrud.ts (1)
125-138:updateUserGroupsは失敗時に toast + throw — 呼び出し側で二重エラーハンドリングに注意。
toast.errorを表示した上でthrowしているため、呼び出し側（left-sidebar.tsxのhandlePersistGroups）のcatchブロックでさらにエラー処理が行われます。left-sidebar.tsxではcatchでロールバックのみ行っており追加の toast は出さないので、現状は問題ありませんが、将来的に呼び出し側でもエラー toast を追加すると二重表示になります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCollectionsCrud.ts` around lines 125 - 138, The updateUserGroups callback currently both shows a toast and throws on failure (in updateUserGroups using putUser), which can lead to duplicate error toasts when callers like handlePersistGroups in left-sidebar.tsx also handle errors; remove the toast.error call from updateUserGroups and let it simply throw or return the failure so callers can decide how to surface UI errors; update the implementation of updateUserGroups (and any callers expecting the toast) to rely on caller-side toast handling (e.g., in handlePersistGroups) and keep putUser usage the same.src/components/raindrop/left-sidebar.tsx (2)
268-538:CollectionRowのプロップ数が非常に多い（約20個）。現状でも動作に問題はありませんが、将来的なメンテナンス性を考えると、コンテキスト（React Context）や props オブジェクトのグルーピングで簡素化できる余地があります。特に
editingCollectionId,editingName,onEditingNameChange,onStartInlineRename,onSubmitInlineRename,onCancelInlineRenameのインライン編集関連プロップはまとめて渡せそうです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/left-sidebar.tsx` around lines 268 - 538, Too many separate props passed into CollectionRow; group related props into a smaller props object to improve readability and maintenance. Create an InlineEditProps type (including editingCollectionId, editingName, onEditingNameChange, onStartInlineRename, onSubmitInlineRename, onCancelInlineRename) and accept it as a single prop (e.g., inlineEdit) in CollectionRow's parameter destructuring; update all usages inside CollectionRow to reference inlineEdit.editingName etc.; update the parent render and the recursive <CollectionRow ... /> calls to pass inlineEdit={inlineEdit} instead of the six separate props; optionally group other handler props similarly (e.g., movement/color handlers) following the same pattern and adjust the memo signature and any external callers to match.
1094-1134:handleDragEndがasyncだが、DndContext.onDragEndは同期ハンドラを想定している。
dnd-kitのonDragEndは(event: DragEndEvent) => void型です。async関数を渡すと返される Promise は無視されます。handlePersistGroups内部でcatchしているため実質的に問題はありませんが、await handlePersistGroups(...)が想定外の例外を投げた場合に unhandled promise rejection になります。
voidで明示的に Promise を捨てるか、内部でtry/catchを追加することを推奨します。♻️ 修正案
- const handleDragEnd = useCallback( - async (event: DragEndEvent) => { + const handleDragEnd = useCallback( + (event: DragEndEvent) => { const activeCollectionId = parseCollectionDndId(event.active.id) const dropTarget = resolveDropIndicator(event.over?.id, displayedGroups) setActiveDragCollectionId(null) setDropIndicator(null) if (!activeCollectionId || !dropTarget) return const sourceLocation = findRootCollectionLocation( displayedGroups, activeCollectionId, ) if (!sourceLocation) return let toIndex = dropTarget.index if ( sourceLocation.groupId === dropTarget.groupId && sourceLocation.index < dropTarget.index ) { toIndex -= 1 } const nextGroups = moveRootCollection( displayedGroups, activeCollectionId, dropTarget.groupId, toIndex, ) if ( serializeRootStructure(displayedGroups) === serializeRootStructure(nextGroups) ) { return } - await handlePersistGroups(nextGroups, displayedGroups) + void handlePersistGroups(nextGroups, displayedGroups) }, [displayedGroups, handlePersistGroups], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/left-sidebar.tsx` around lines 1094 - 1134, The handler handleDragEnd is declared async but DndContext.onDragEnd expects a synchronous (void) callback, risking unhandled promise rejections from await handlePersistGroups; change handle to remain synchronous by removing async and explicitly discard or handle the Promise: either wrap the persistence call in a try/catch inside handleDragEnd (using a non-async IIFE if you want to keep await semantics) or call handlePersistGroups(nextGroups, displayedGroups).catch(err => { process/log the error }) or prefix with void to intentionally ignore the Promise; ensure you reference handleDragEnd, DndContext.onDragEnd, and handlePersistGroups when making the change.e2e/specs/dnd.spec.ts (1)
182-216:page.waitForTimeout(600)は Playwright のベストプラクティスでは非推奨だが、モック応答の遅延シミュレーションとしては許容範囲。
route.fulfill前の人為的遅延としての使用なので、UI ポーリングの代替ではなく意図的なものと理解できます。ただし、CI の負荷次第で 600ms が不十分になる可能性があります。将来的に不安定になった場合は、遅延値を大きくするか、別のシグナル（例: リクエスト完了イベント）でガードすることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts` around lines 182 - 216, The artificial 600ms delay inside the route handler (in the anonymous async route callback that calls page.waitForTimeout(600) before route.fulfill) is brittle; replace it with a more reliable guard or a larger timeout. Either wait for a deterministic signal (e.g., use page.waitForResponse with the PUT to '/api.raindrop.io/rest/v1/user' or another test-controlled event) before calling route.fulfill, or increase the delay to a safer value (e.g., 2000ms) and make it configurable so the optimistic-move assertions (getGroupRootCollectionIds polls) remain stable; update the anonymous route handler where failedOnce, route.request(), page.waitForTimeout(600), and route.fulfill are used accordingly.
Harden e2e startup state normalization by selecting All Bookmarks via sidebar button and asserting stable breadcrumb/search scope before interactions to reduce cross-test persisted-state flakiness.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/specs/dnd.spec.ts`:
- Around line 148-181: The single test 'supports context menu rename, move,
color, and delete actions' chains four independent scenarios and should be split
into separate tests to improve isolation and debuggability: create four tests
(e.g., "context menu rename", "context menu move to group", "context menu change
color", "context menu delete") that each call waitForSidebarReady(page) and use
openCollectionContextMenu(page, '100') as needed; ensure each test resets or
recreates the target collection '100' in a beforeEach or by creating a new
collection at test start so operations on getGroupRootCollectionIds(page,
'group-1'), page.getByTestId('collection-color-100') and
page.getByTestId('sidebar-collection-100') assert only the specific behavior
under test and do not depend on previous steps.
- Around line 9-16: The waitForSidebarReady function currently checks the
Unsorted button only after forcing a click on allBookmarksButton, but it should
also wait for the sidebar navigation to finish like resetToAllBookmarks does;
update waitForSidebarReady to, after await allBookmarksButton.click({ force:
true }), wait for the breadcrumb (the same breadcrumb element or selector used
in resetToAllBookmarks) to be visible (e.g., await
expect(breadcrumbElement).toBeVisible()) before asserting the Unsorted button,
so the test reliably waits for navigation completion; reference the function
name waitForSidebarReady and mirror the breadcrumb wait logic from
resetToAllBookmarks in search.spec.ts.
- Around line 46-57: The dragCollectionToTarget helper performs mouse.down(),
hover(), hover(), mouse.up() with no pauses which can fail in CI because dnd-kit
may require activation delay/distance; update dragCollectionToTarget to
introduce a short explicit delay (e.g., use page.waitForTimeout with ~50-200ms)
after page.mouse.down() and/or before page.mouse.up(), or perform a small
mouse.move with steps to simulate activation distance (use page.mouse.move to a
nearby coordinate then to the target) so the drag is reliably recognized; update
the function references: dragCollectionToTarget, source
(sidebar-collection-{sourceCollectionId}), page.mouse.down, page.mouse.move,
page.waitForTimeout, and page.mouse.up.
In `@e2e/specs/search.spec.ts`:
- Around line 166-174: Extract the repeated "count() > 0 && isVisible() → click"
pattern into a reusable helper (e.g., safeClickIfVisible or clickIfPresent) and
replace the three occurrences that create and use searchEverywhereButton with
calls to that helper; the helper should accept a Locator (or the result of
commandDialog.getByRole) and perform the defensive checks (await locator.count()
> 0 && await locator.first().isVisible()) before clicking the first visible
element, returning a boolean for whether a click occurred so tests can assert
behavior if needed. Ensure callers in search.spec.ts replace the inline logic at
the blocks that reference searchEverywhereButton with the new helper to improve
readability and reduce duplication.
🧹 Nitpick comments (4)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts`: - Around line 148-181: The single test 'supports context menu rename, move, color, and delete actions' chains four independent scenarios and should be split into separate tests to improve isolation and debuggability: create four tests (e.g., "context menu rename", "context menu move to group", "context menu change color", "context menu delete") that each call waitForSidebarReady(page) and use openCollectionContextMenu(page, '100') as needed; ensure each test resets or recreates the target collection '100' in a beforeEach or by creating a new collection at test start so operations on getGroupRootCollectionIds(page, 'group-1'), page.getByTestId('collection-color-100') and page.getByTestId('sidebar-collection-100') assert only the specific behavior under test and do not depend on previous steps. - Around line 9-16: The waitForSidebarReady function currently checks the Unsorted button only after forcing a click on allBookmarksButton, but it should also wait for the sidebar navigation to finish like resetToAllBookmarks does; update waitForSidebarReady to, after await allBookmarksButton.click({ force: true }), wait for the breadcrumb (the same breadcrumb element or selector used in resetToAllBookmarks) to be visible (e.g., await expect(breadcrumbElement).toBeVisible()) before asserting the Unsorted button, so the test reliably waits for navigation completion; reference the function name waitForSidebarReady and mirror the breadcrumb wait logic from resetToAllBookmarks in search.spec.ts. - Around line 46-57: The dragCollectionToTarget helper performs mouse.down(), hover(), hover(), mouse.up() with no pauses which can fail in CI because dnd-kit may require activation delay/distance; update dragCollectionToTarget to introduce a short explicit delay (e.g., use page.waitForTimeout with ~50-200ms) after page.mouse.down() and/or before page.mouse.up(), or perform a small mouse.move with steps to simulate activation distance (use page.mouse.move to a nearby coordinate then to the target) so the drag is reliably recognized; update the function references: dragCollectionToTarget, source (sidebar-collection-{sourceCollectionId}), page.mouse.down, page.mouse.move, page.waitForTimeout, and page.mouse.up. In `@e2e/specs/search.spec.ts`: - Around line 166-174: Extract the repeated "count() > 0 && isVisible() → click" pattern into a reusable helper (e.g., safeClickIfVisible or clickIfPresent) and replace the three occurrences that create and use searchEverywhereButton with calls to that helper; the helper should accept a Locator (or the result of commandDialog.getByRole) and perform the defensive checks (await locator.count() > 0 && await locator.first().isVisible()) before clicking the first visible element, returning a boolean for whether a click occurred so tests can assert behavior if needed. Ensure callers in search.spec.ts replace the inline logic at the blocks that reference searchEverywhereButton with the new helper to improve readability and reduce duplication.e2e/specs/dnd.spec.ts (3)
148-181: 1つのテストで4つの独立したコンテキストメニュー操作を連鎖させている。Rename → Move → Color → Delete を1テストにまとめると、途中で失敗した場合のデバッグが困難になる。特にDelete（Line 178-180）が最後にあるため、前のステップの失敗が原因でも「Delete failed」というエラーメッセージになりうる。
テスト分割を推奨するが、fixtureが毎回新しいアプリを起動するコストを考慮するとトレードオフはある。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts` around lines 148 - 181, The single test 'supports context menu rename, move, color, and delete actions' chains four independent scenarios and should be split into separate tests to improve isolation and debuggability: create four tests (e.g., "context menu rename", "context menu move to group", "context menu change color", "context menu delete") that each call waitForSidebarReady(page) and use openCollectionContextMenu(page, '100') as needed; ensure each test resets or recreates the target collection '100' in a beforeEach or by creating a new collection at test start so operations on getGroupRootCollectionIds(page, 'group-1'), page.getByTestId('collection-color-100') and page.getByTestId('sidebar-collection-100') assert only the specific behavior under test and do not depend on previous steps.
9-16:waitForSidebarReadyでUnsortedボタンの可視性チェックのみでは不十分な可能性。
allBookmarksButton.click({ force: true })の後、ナビゲーション完了の待機がない。search.spec.tsのresetToAllBookmarksではブレッドクラムのtoBeVisibleで遷移完了を確認しているが、こちらにはそれがない。同じパターンに揃えた方がCI安定性が向上する。♻️ ブレッドクラム待機の追加
async function waitForSidebarReady(page: Page) { const allBookmarksButton = page .getByRole('button', { name: 'All Bookmarks' }) .first() await expect(allBookmarksButton).toBeVisible({ timeout: 10_000 }) await allBookmarksButton.click({ force: true }) + await expect( + page.locator('[data-slot="breadcrumb-page"]', { + hasText: 'All Bookmarks', + }), + ).toBeVisible({ timeout: 10_000 }) await expect(page.getByRole('button', { name: 'Unsorted' })).toBeVisible() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts` around lines 9 - 16, The waitForSidebarReady function currently checks the Unsorted button only after forcing a click on allBookmarksButton, but it should also wait for the sidebar navigation to finish like resetToAllBookmarks does; update waitForSidebarReady to, after await allBookmarksButton.click({ force: true }), wait for the breadcrumb (the same breadcrumb element or selector used in resetToAllBookmarks) to be visible (e.g., await expect(breadcrumbElement).toBeVisible()) before asserting the Unsorted button, so the test reliably waits for navigation completion; reference the function name waitForSidebarReady and mirror the breadcrumb wait logic from resetToAllBookmarks in search.spec.ts.
46-57: ドラッグ操作のステップ間にディレイがなく、CIで不安定になる可能性。
mouse.down()→hover()→hover()→mouse.up()の間にディレイがない。dnd-kitのドラッグ認識にはactivation distance/delayが必要な場合があり、CI環境で失敗する可能性がある。♻️ 安定性向上の提案
async function dragCollectionToTarget( page: Page, sourceCollectionId: string, target: Locator, ) { const source = page.getByTestId(`sidebar-collection-${sourceCollectionId}`) await source.hover() await page.mouse.down() + await page.waitForTimeout(150) // dnd-kit activation delay await target.hover() await target.hover() await page.mouse.up() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/dnd.spec.ts` around lines 46 - 57, The dragCollectionToTarget helper performs mouse.down(), hover(), hover(), mouse.up() with no pauses which can fail in CI because dnd-kit may require activation delay/distance; update dragCollectionToTarget to introduce a short explicit delay (e.g., use page.waitForTimeout with ~50-200ms) after page.mouse.down() and/or before page.mouse.up(), or perform a small mouse.move with steps to simulate activation distance (use page.mouse.move to a nearby coordinate then to the target) so the drag is reliably recognized; update the function references: dragCollectionToTarget, source (sidebar-collection-{sourceCollectionId}), page.mouse.down, page.mouse.move, page.waitForTimeout, and page.mouse.up.e2e/specs/search.spec.ts (1)
166-174: 永続化された状態への防御的ハンドリングパターンの重複を検討。この
count() > 0 && isVisible() → clickパターンが複数箇所（Line 86-91、132-137、169-173）で繰り返されている。テストの安定性向上には有効だが、ヘルパーに抽出すると可読性が上がる。♻️ ヘルパー関数の提案
+async function clickIfVisible(locator: Locator) { + if ((await locator.count()) > 0 && (await locator.first().isVisible())) { + await locator.first().click() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/specs/search.spec.ts` around lines 166 - 174, Extract the repeated "count() > 0 && isVisible() → click" pattern into a reusable helper (e.g., safeClickIfVisible or clickIfPresent) and replace the three occurrences that create and use searchEverywhereButton with calls to that helper; the helper should accept a Locator (or the result of commandDialog.getByRole) and perform the defensive checks (await locator.count() > 0 && await locator.first().isVisible()) before clicking the first visible element, returning a boolean for whether a click occurred so tests can assert behavior if needed. Ensure callers in search.spec.ts replace the inline logic at the blocks that reference searchEverywhereButton with the new helper to improve readability and reduce duplication.
Handle debounced async parse promise safely and add accessible aria-labels to inline collection rename inputs.
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
1 participant
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
PUT /uservia new collection-organization helpers and CRUD wiring in the app layerresolveIcon(url)fallback chain (suggest API -> Google favicon -> letter fallback), and add comprehensive unit + E2E coverage for F4/F6 scenariosTest plan
pnpm lintpnpm typecheckpnpm testpnpm buildpnpm knippnpm test:e2epnpm coverage:e2e-specSummary by CodeRabbit
新機能
改善
テスト