feat: make infinite scroll at search result - #11617
Conversation
…tasks, research) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cumulation util Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tSearchInfiniteKey Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reset to resetKey Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…selection in SearchPageBase Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cumulated append Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…umulated bulk-delete in SearchPageBase Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ll, remove numbered pager Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r/retry contract (1.6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ection after bulk delete Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…SearchPageBase contract Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-disabled, sentinel load) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(5.2 audit, no gaps) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….3 (deferred to user) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
…select, optional searchPager, chunk-size doc) - PrivateLegacyPages: explicitly deselect after bulk delete / convert, since SearchPageBase now resets selection on resetKey change (not on every refetch) - SearchPageBase: make searchPager an optional prop; SearchPage no longer passes null - spec: correct chunk-size wording (showPageLimitationL default is 50, 20 is fallback) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
レビュー: PR #11617
|
| 観点 | 対応セクション |
|---|---|
| dead code | §3 |
| 重複コード | §3 |
| spec と実装の乖離 | §4 |
| セキュリティ(重点) | §5 |
| パフォーマンス | §2 P2-2〜P2-4 / §6 |
| テスト(既存 skill 適用) | §7 |
| その他 | §2 P3 |
1. 概要
/_search(フルページ検索結果)を offset/limit の番号ページャから infinite scroll へ置き換える PR。
- 新規フック
useSWRINFxSearchと純粋なキー関数getSearchInfiniteKeyを追加(既存useSWRxSearchは温存 = 6 消費者を守る) - 累積合成の純粋関数
mergeInfiniteSearchResultを新設 SearchPageBaseにresetKey(必須)とinfiniteScroll(任意)を追加し、リセット契機を[pages]→[resetKey]へ変更PrivateLegacyPagesは番号ページャのまま非回帰対応
設計方針は非常に良い。 共有資産を壊さないための resetKey データ駆動化、純粋関数抽出、SWR キー名前空間分離(/search/infinite)は steering(coding-style)にも合致している。
ただし サーバ実装との契約の食い違いに起因する P1 バグが 1 件、既存機能のサイレントな回帰が 1 件あり、これらはマージ前に対応が必要。
2. 重大度順の指摘
🔴 P1-1. 「終端判定」がサーバ実装と食い違い、結果が途中で打ち切られ・スピナーが永久に回る
該当: apps/app/src/stores/search.tsx:143 / apps/app/src/features/search/client/util/infinite-search-result.ts:39
// getSearchInfiniteKey
if (previousPageData != null && previousPageData.data.length < chunkSize) {
return null; // 末尾到達とみなして停止
}サーバ側 apps/app/src/server/service/search.ts:794 の formatSearchResult は最後にこうしている:
result.data = pages.filter(nonNullable); // Mongo に存在しないページを除外
result.meta = searchResult.meta; // total/hitsCount は ES の生値のままつまり data.length ≤ meta.hitsCount であり、ES インデックスに残っているが Mongo 側で取得できないページがあると data.length < chunkSize になる(GROWI では ES インデックスのドリフトは珍しくない)。
このとき起きること:
getSearchInfiniteKeyがnullを返し、まだ結果が残っているのに追加取得が完全に止まる- 一方
isReachingEnd = loadedCount >= totalは48 < 200でfalseのまま InfiniteScrollはisReachingEnd === falseなので ローディングスピナーを永久に表示(setSizeは増えるが key が null なので何も起きない)
結果として「途中で結果が切れ、スピナーが回り続ける」という分かりにくい壊れ方をする。ページャ時代は各ページ独立取得だったのでこの問題は表面化しなかった。
修正案: 停止判定と isReachingEnd の両方を、フィルタ前の値である meta.hitsCount に揃える。
// getSearchInfiniteKey
if (previousPageData != null && previousPageData.meta.hitsCount < chunkSize) {
return null;
}
// mergeInfiniteSearchResult
const fetchedCount = data.reduce((acc, r) => acc + r.meta.hitsCount, 0);
const isLastChunkPartial = (data.at(-1)?.meta.hitsCount ?? 0) < chunkSize;
// ...
isReachingEnd: fetchedCount >= total || isLastChunkPartial,pages / loadedCount は表示用として data.flatMap のままで問題ない。offset は pageIndex * chunkSize で ES の from と一致するため、フィルタによるズレは発生しない。
🔴 P1-2. mutateSearching() が infinite scroll の結果に効かなくなっている(サイレント回帰)
該当: apps/app/src/stores/search.tsx:51-53
export const mutateSearching = async (): Promise<void[]> => {
return mutate((key) => Array.isArray(key) && key[0] === '/search');
};apps/app/src/features/search/client/components/SearchPage/SearchResultList.tsx:120-160 は、結果リスト内のドロップダウンからページを複製 / リネーム / 削除したときに mutateSearching() を呼んで一覧を更新している。
新フックのキー先頭は '/search/infinite' なので、この述語に一致しない。その結果、SearchPage では「リストアイテムのメニューからページを削除 / リネーム / 複製しても一覧が更新されない」という回帰が発生する。
一括削除は deleteCompletedHandler で明示 mutate しているため無影響 = この抜けが見つかりにくい。
修正案: key[0] === '/search' || key[0] === '/search/infinite'(あるいは startsWith('/search'))へ広げるか、infinite 用の mutate を別途 export して SearchResultList から呼ぶ。合わせて回帰テストを 1 本追加すること。
🟠 P2-1. limit(チャンクサイズ)と nqName が SWR key に含まれていない
該当: apps/app/src/stores/search.tsx:116
// offset (derived per page) and limit (= chunkSize) are excluded on purpose.しかし fetcher は chunkSize と nqName をクロージャで参照してレスポンス内容を変えている。SWR の鉄則「fetcher が読む値はすべて key に入れる」に反しており、
showPageLimitationLが途中で変わる / 別の消費者が別のlimitで同じフックを使う → 同一 key に異なる件数のレスポンスがキャッシュされるnqNameを渡す消費者が現れると、名前付きクエリ違いの結果を取り違える
現状 SearchPage が唯一の消費者かつ showPageLimitationLAtom は atom<number>(50)(apps/app/src/states/server-configurations/server-configurations.ts:59)で実質固定なので顕在化しないが、潜在バグとして残すべきではない。chunkSize と nqName を key に追加すること(offset を除くのは正しい判断)。
なお同じ理由で apps/app/src/features/search/client/components/SearchPage/SearchPage.tsx:96 の showPageLimitationL ?? INITIAL_PAGIONG_SIZE の ?? は、atom が number 非 nullable なので到達不能。requirements 3.1 の「未取得時 20 フォールバック」は現行 atom 定義では成立しない。
🟠 P2-2. useSWRxPageInfoForList が累積リスト全体を毎回再取得する(O(n²) + URL 長超過リスク)
該当: apps/app/src/features/search/client/components/SearchPage/SearchResultList.tsx:45-59
const pageIdsWithNoSnippet = pages.filter(...).map((page) => page.data._id);
const { data: idToPageInfo } = useSWRxPageInfoForList(pageIdsWithNoSnippet, ...);apps/app/src/stores/page-listing.tsx:154 の useSWRxPageInfoForList は key が ['/page-listing/info', pageIds, ...] で、pageIds は GET のクエリパラメータとして送られる。
infinite scroll では pages が append のたびに伸びるため:
- 追記ごとに「累積 ID 全件」を載せた新リクエストが発生(差分ではない)→ スクロール全体で O(n²) の転送・DB 参照
- ID は 24 文字。数百件累積すると クエリ文字列が 10KB を超え、nginx の
large_client_header_buffers(既定 8k)等で 414/431 で失敗し得る。失敗するとスニペット・ブックマーク数が表示されなくなる useSWRImmutableなので中間状態の配列キーがすべてキャッシュに残る(メモリ)
本 PR のスコープ外に見えるが、infinite scroll 化によって初めて現実的な問題になる。少なくとも「新規追加分の ID だけを問い合わせる」への変更、または既知の制約として issue 化を推奨。
🟠 P2-3. 仮想化なしの累積描画
SearchResultList は累積 pages をそのまま <ul> に展開する。showPageLimitationL 既定 50 で数千件を最後までスクロールすると DOM ノードが無制限に増え、PageListItemL は重い(ドロップダウン・チェックボックス・スニペット)ため終盤でスクロールがもたつく。
design.md の Non-Goals にも仮想化の記述がないので、既知の制約として明記するか後続タスク化すること。
🟠 P2-4. ES の max_result_window 到達で「再試行しても必ず失敗する」状態になる
apps/app/src/server/util/apiPaginate.js:28 は limit + offset > 10000 で例外を投げ、apps/app/src/client/util/apiv1-client.ts:31 の apiRequest が throw → swr.error → エラー + 再試行 UI が出る。しかし再試行しても同じ offset なので永久に失敗する。
ページャ時代は「201 ページ目を明示的に押す」必要があったが、infinite scroll ではただ下にスクロールし続けるだけで到達する。isReachingEnd に loadedCount + chunkSize > 10000 の条件を OR して、素直に終端扱いする方が親切。
🟡 P3. その他の実装上の指摘
| # | 箇所 | 指摘 |
|---|---|---|
| P3-1 | SearchPageBase.tsx:209-219 |
コメントは「resetKey が唯一のトリガー」と書いてあるのに、依存配列に onSelectedPagesByCheckboxesChanged が入っている。消費者がハンドラをインライン関数で渡すと毎レンダーで選択が全解除される(現状は両消費者とも useCallback([]) なので顕在化せず)。ハンドラを useRef に退避して依存から外すのが安全。 |
| P3-2 | SearchPage.tsx:203-208 |
swr.setSize(1); swr.mutate(); は両方が再検証を起こすためリクエストが 2 回飛ぶ。setSize は Promise を返すので await swr.setSize(1); await swr.mutate(); にするとコメントどおりの順序保証も得られる。 |
| P3-3 | SearchPage.tsx:140-149 |
searchInvokedHandler の swr.setSize(1) は、キーワード / 条件が変わる場合は無意味(useSWRInfinite の size は infinite key 単位で保持されるため、新キーでは自動的に 1 から始まる)。同一条件での再検索時のみ効く。コメント「累積を破棄し先頭から再読込」は誤解を招くので実態に合わせて修正を。 |
| P3-4 | SearchPage.tsx の onRetry / deleteCompletedHandler / searchInvokedHandler |
deps に swr オブジェクトを置いている。useSWRInfinite の戻り値は毎レンダー新しいオブジェクトなので useCallback が実質無効化される。const { setSize, mutate } = swr; と分割代入して安定参照を deps に置くこと。連鎖して searchControl の useMemo も毎レンダー再計算され、append のたびに SearchControl 全体が再レンダーする。 |
| P3-5 | SearchPageBase.tsx:312-359 |
JSX 内の即時実行関数(IIFE)はネストが深く読みづらい。const searchResultListNode = ... をコンポーネント本体(早期 return より下)に上げ、分岐は {infiniteScroll != null ? <InfiniteScroll…> : <>…</>} の三項で十分。 |
| P3-6 | SearchPage.tsx:320 |
pages={merged.pages} は常に配列なので、SearchPageBase.tsx:299 の pages == null ローディング分岐が SearchPage 経路では到達不能になった。初回ロード時のスピナーが「画面中央の大きいもの」→「リスト末尾の小さいもの」に変わる(意図的なら OK だが design には記述なし)。 |
| P3-7 | PrivateLegacyPages.tsx:586 付近 |
useSWRxSearch は keepPreviousData: true なので、ページ送りで resetKey が変わった瞬間に選択 Set はクリアされる一方、旧データの行がまだ描画されたままでチェックが視覚的に残る短い窓ができる(旧実装は [pages] 依存だったので同期していた)。resetKey 変更時に searchResultListRef.current?.deselectAll() も呼ぶと解消。 |
| P3-8 | SearchPageBase.tsx:334 |
エラー表示が t('Error') だけでは「追加読み込みに失敗した」ことが伝わらない。search_result.failed_to_load_more のような専用キーの追加を推奨。 |
3. Dead code / 重複
Dead code
| # | 箇所 | 内容 |
|---|---|---|
| D-1 | SearchPage.tsx:96 |
showPageLimitationL ?? INITIAL_PAGIONG_SIZE の ?? — atom が非 nullable のため到達不能(P2-1 参照) |
| D-2 | SearchPage.spec.tsx |
vi.mock('~/client/components/PaginationWrapper', ...) — SearchPageBase をスタブ化しており SearchPage は PaginationWrapper を import すらしないので完全に未使用。data-testid="pagination-wrapper" を誰も query していない |
| D-3 | SearchPage.spec.tsx |
useSWRxSearch モック — コメントに「RED フェーズ用に残した」と明記された TDD の足場がそのまま残存。削除すること |
| D-4 | infinite-search-result.ts |
MergedSearchResult.loadedCount — 本番コードからの参照はなくテストのみが読んでいる(P1-1 の修正で fetchedCount を導入するなら整理の好機) |
重複
| # | 箇所 | 内容 |
|---|---|---|
| R-1 | SearchPage.tsx:35 / stores/search.tsx:113 |
INITIAL_PAGIONG_SIZE = 20 と DEFAULT_SEARCH_CHUNK_SIZE = 20 の二重定義。design.md が「チャンクサイズ定数を共有化」と明記しているのに実装で分裂している。片方に寄せること |
| R-2 | SearchPage.tsx:203 / PrivateLegacyPages.tsx:380,428 |
削除完了ハンドラの「deselectAll() + onSelectedPagesByCheckboxesChanged(0, 0)」がほぼ同形で 3 箇所。SearchPageBase の imperative handle に resetSelection() を生やせば 1 箇所に寄せられる(deselectAll は既にあるので、親への通知だけが重複) |
4. spec と実装の乖離
| # | spec の記述 | 実装 | 判定 |
|---|---|---|---|
| S-1 | design.md「stores/search.tsx … チャンクサイズ定数を共有化」 | 2 箇所に重複定義(R-1) | ❌ 乖離 |
| S-2 | requirements 3.1「クライアントで当該設定が未取得の場合のみ 20 件にフォールバック」 | atom が number 既定 50 のためフォールバック経路が存在しない(D-1) |
❌ 乖離。要件文の修正 or atom の型見直しが必要 |
| S-3 | design「isReachingEnd … 累積件数 >= meta.total で明快に判定できる」 |
サーバの post-filter を考慮していない(P1-1) | ❌ 前提誤り |
| S-4 | requirements 1.6「以降のスクロール操作で再試行できる状態を維持」 | 明示的な Retry ボタン | |
| S-5 | tasks.md 親タスク - [ ] 1. - [ ] 2. … |
子タスクは全て [x] |
|
| S-6 | tasks.md 5.3「実機スモーク」 | 未チェック(manual-pending) | |
| S-7 | design「Out of Boundary: InfiniteScroll コンポーネント内部 — 改変しない」 |
改変なし。テストのみ新規追加 | ✅ 問題なし(むしろ良い) |
その他、design で規定された getSearchInfiniteKey / mergeInfiniteSearchResult / SearchPageBaseInfiniteProps のシグネチャ、プレビュー初期選択の 2 段構成、append 再通知 effect はすべて design どおりに実装されている。設計トレーサビリティ自体は良好。
5. セキュリティ
デリケートな箇所との指定を受けて重点的に確認したが、新規の脆弱性は見つからなかった。根拠は以下。
✅ 問題なしと判断した点
| 観点 | 根拠 |
|---|---|
| 攻撃面の増加ゼロ | 新規エンドポイントなし。既存 apiv1 /search を同じパラメータ(q/nq/limit/offset/sort/order)で叩くだけ。クエリ生成も既存 createSearchQuery を再利用 |
| 認可はサーバ側で維持 | server/routes/search.ts:143-151 の userGroups 解決と ES クエリのアクセス制御、formatSearchResult の findPageListByIds による可視性フィルタ、serializeUserSecurely()、スニペットの filterXss.process() はすべて不変 |
| 一括削除の対象は表示中ページに限定 | SearchPageBase.tsx:412 は pages.filter(p => selectedIds.has(p.data._id))。選択 Set に古い ID が残っていても現在のリストに存在しないページは削除対象に入らない(over-delete しない安全側)。削除実行自体はサーバ側で再認可される |
limit はユーザー入力ではない |
admin 設定 showPageLimitationL 由来。加えてサーバ側で LIMIT_MAX = 1000 と limit + offset ≤ 10000 の二重ガードあり。offset も pageIndex * chunkSize で単調増加のみ |
| XSS | スニペット / ハイライトパスのサニタイズはサーバ側 filterXss のまま。クライアントの新規描画箇所(エラー + Retry ボタン)は静的テキストのみ |
⚠️ 脆弱性ではないが認識しておくべき点
- 「全選択」の意味変化: 対象が「表示中 20 件」から「累積 500 件」へ静かに変わる。indeterminate → チェックで一気に数百ページが削除対象になり得るが、削除確認モーダルが対象一覧を提示するため許容範囲。requirements 4.1 で承認済みの仕様でもある
- 監査ログの増加:
server/routes/search.ts:198-207は/search1 リクエストにつきACTION_SEARCH_PAGEの Activity を 1 件書き込む。スクロールでチャンクを読むたびに記録されるため、Activity コレクションの増加ペースが上がる(ページャ時代と回数自体は同等だが、スクロールは無意識に発生する分だけ増える)。攻撃ではなく運用影響として認識しておく価値がある
6. パフォーマンス
深刻度順に P2-2(useSWRxPageInfoForList の O(n²) + URL 長)> P2-3(仮想化なし)> P3-4(swr を deps に置いたことによる再レンダー連鎖)。詳細は §2 参照。
良い点
revalidateFirstPage: falseの指定により append 時に先頭チャンクを無駄に再検証しない設計。RecentChangesのパターンとも整合しているmergeInfiniteSearchResultをuseMemo(..., [swr.data])で包んでおり、append ごとの O(n) 合成のみで済んでいる
追加の推奨
useSWRxSearchが明示しているrevalidateOnFocus: falseが新フックには無い。revalidateFirstPage: falseによりネットワーク実害はほぼ無いはずだが、意図の明示として付けておくことを推奨
7. テストレビュー(essential-test-design / essential-test-patterns 適用)
対象 5 ファイル・60 テストすべてグリーンを確認済み。総じて契約ベースで書かれた良いテストが多い。
✅ 高く評価できる点
infinite-search-result.spec.ts— 純粋関数の境界(未満 / 一致 / 0 件 / 未取得)を出力値で検証。教科書的InfiniteScroll.spec.tsx— IntersectionObserver をスタブして「交差したら次チャンクを要求」「終端では要求しない」「取得中は要求しない」という観測可能な契約を検証。expect(updater(1)).toBe(2)で「+1 であること」まで見ているのが良いmock<SWRInfiniteResponse<...>>({...})の使用(InfiniteScroll.spec,search.spec,infinite-search-result.spec)— testing rule の型安全モック方針に準拠
❌ 改善が必要な点
T-1. 型アサーションの混在(testing rule 違反 / Tier 1)
// SearchPage.spec.tsx / SearchPageBase.spec.tsx
const createChunk = (ids: string[]): IFormattedSearchResult => ({ ... }) as unknown as IFormattedSearchResult;
const createPage = (id: string): IPageWithSearchMeta => ({ ... }) as unknown as IPageWithSearchMeta;同じ PR 内の infinite-search-result.spec.ts / search.spec.ts では正しく mock<IPageHasId>({ _id: id }) を使っているのに、コンポーネント spec 側だけ as unknown as T になっている。削除コスト ≤ 0 の Tier 1 に該当するので mock<T>() へ統一すること。
T-2. 実現不可能な状態を作って通しているテスト
SearchPageBase.spec.tsx「re-notifies full selection (checked) when the appended pages are also selected」:
searchResultListSpy.lastProps?.onCheckboxChanged?.(true, 'c'); // 'c' はまだロードされていない
searchResultListSpy.lastProps?.onCheckboxChanged?.(true, 'd'); // 同上まだ描画されていない行のチェックボックスイベントを発火させている。実運用では追記された行は必ず未選択(Req 4.3)なので、append 後は常に indeterminate になり、この checked ケースは発生しない。
「Arrange が Assert のために存在する」アンチパターンであり、Req 4.6 の実際の契約を守っていない。削除するか、selectAll() → append → selectAll() のような到達可能なシナリオに書き直すべき。
T-3. SearchPage.spec の実装結合度が高い
SearchPageBase / SearchControl / OperateAllControl / stores/search / states/search / server-configurations / i18n など 10 個のモジュールをモックし、スタブに渡された props を searchPageBaseSpy.lastProps で覗いてアサートしている。
SearchPage が実質「配線コンポーネント」なのである程度は妥当だが、以下は改善余地がある:
expect(typeof props?.infiniteScroll?.isReachingEnd).toBe('boolean')は何の回帰も検出しない(常に true)。削除するか具体値を検証するsearchPagerの falsy チェックより「番号ページャが DOM に出ない」を検証する方が Req 2.1 の契約に忠実(PaginationWrapperのモックが用意されているのに使われていない = まさにこの意図の残骸に見える。D-2 参照)setSize(1)/mutate()の呼び出し回数アサートは実装詳細寄りだが、SWR がこのコンポーネントの「外部境界」なので許容範囲と判断
T-4. カバレッジの穴
- P1-1 のシナリオ(
data.length < hitsCount)を検証するテストが皆無。mergeInfiniteSearchResult/getSearchInfiniteKeyのテストはすべてdata.length === hitsCountを前提にしたフィクスチャ。修正の際は「ES が満杯チャンクを返したが post-filter で減った」ケースを必ず追加すること mutateSearching()が infinite key に効くかの回帰テストなし(P1-2)useSWRINFxSearch本体(fetcher が正しいlimit/offsetで/searchを叩くか)は未検証。純粋関数だけをテストする design の方針どおりではあるが、chunkSizeが key に無い問題(P2-1)はここでしか捕まえられない
8. 総括
| 観点 | 評価 | コメント |
|---|---|---|
| 設計・方針 | ⭐️⭐️⭐️⭐️⭐️ | 共有資産の非破壊拡張、純粋関数抽出、キー名前空間分離すべて適切 |
| 実装品質 | ⭐️⭐️⭐️ | P1 が 2 件。useCallback の deps に SWR オブジェクトを置くなど詰めの甘さ |
| テスト | ⭐️⭐️⭐️⭐️ | 契約ベースで良質。型アサーション混在と実現不可シナリオ 1 件、P1 領域の穴 |
| セキュリティ | ⭐️⭐️⭐️⭐️⭐️ | 新規リスクなし。認可・サニタイズ・上限はすべてサーバ側で維持 |
| spec 整合 | ⭐️⭐️⭐️⭐️ | 概ね忠実。定数共有化とフォールバック仕様の 2 点が乖離 |
マージ前に対応を推奨
- P1-1 終端判定を
meta.hitsCountベースへ変更 + 回帰テスト追加 - P1-2
mutateSearching()を/search/infiniteにも効かせる + 回帰テスト追加 - S-6 tasks 5.3 の実機スモーク(P1-1 はここで発見されるはずの不具合)
フォローアップで対応可
- P2-1 SWR key に
chunkSize/nqNameを含める - P2-2
useSWRxPageInfoForListの差分取得化(または issue 化) - P2-3 仮想化の要否を判断(または既知の制約として design に明記)
- P2-4
max_result_window到達時の終端扱い - P3-1〜P3-8
- D-1〜D-4 の dead code 削除、R-1 / R-2 の重複解消
- T-1 型アサーションの
mock<T>()統一、T-2 の書き直し、T-3 の無意味なアサート削除
…ection, mutateSearching) - P1-1: judge end-of-results by meta.hitsCount (pre-MongoDB-filter), not data.length. When the ES index has drifted (docs dropped by the server), data.length stays below chunkSize/total and would truncate results and spin the loader forever. getSearchInfiniteKey + mergeInfiniteSearchResult now use hitsCount; add regression tests for the drift case. - P1-2: broaden mutateSearching to match both '/search' and '/search/infinite' keys so item-level page operations (delete/rename/duplicate) in SearchResultList refresh the infinite list; add a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- P2-1: carry chunk size (limit) and nqName in the infinite-scroll SWR key so different values never share a cache entry; add revalidateOnFocus:false. getSearchInfiniteKey now reads chunkSize from configurations (4 params). - R-1: consolidate the chunk-size constant into DEFAULT_SEARCH_CHUNK_SIZE in stores/search.tsx, shared by SearchPage (was a duplicated INITIAL_PAGIONG_SIZE). - S-2: correct requirements/design wording (showPageLimitationL default is 50; the 20 fallback is a defensive floor unreachable with the current config). - D-3: drop the dead RED-phase useSWRxSearch mock from SearchPage.spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- P3-4: destructure stable setSize/mutate/data/error from the useSWRInfinite response so SearchPage callbacks keep stable identities and memoized consumers (searchControl) are not recomputed on every append. - P2-4: stop infinite scroll before the Elasticsearch max_result_window (10000) so scrolling to the cap ends cleanly instead of failing every retry; add regression tests (at and just below the boundary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…8/S-5 - P3-1: hold the selection-change handler in a ref so SearchPageBase's reset and append-follow effects no longer depend on it (an inline handler from a consumer would otherwise re-run and clear the selection every render). - P3-7: also deselect the rendered rows on a resetKey change, so the legacy keepPreviousData path does not briefly show stale checkmarks. - P3-8: use a dedicated 'search_result.failed_to_load_more' message (added to 5 locales) for the additional-load error instead of the generic 'Error'. - S-5: check off the completed parent tasks (1-4) in tasks.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- T-1: replace 'as unknown as' fixtures with type-safe mock<T>() in the SearchPage and SearchPageBase specs (createPage/createChunk/createFullChunks). - T-2: rewrite the 'checked after append' test to a reachable scenario — select the appended rows AFTER they are loaded, instead of firing checkbox events for pages that are not yet in the list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o O(n^2) as Non-Goals (P2-2/P2-3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- D-2: remove the unused PaginationWrapper mock from SearchPage.spec. - T-3: assert the concrete isReachingEnd value (false) instead of just its type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cit retry (S-4) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- C: stop infinite scroll when a chunk returns fewer hits than the chunk size, not only when fetchedCount>=total. Prevents a permanent spinner when ES over-counts total (track_total_hits estimate / concurrent deletes) since getSearchInfiniteKey stops but isReachingEnd stayed false. mergeInfiniteSearchResult now takes chunkSize; add an over-count regression test (RED-verified). - B: re-add mutate() in searchInvokedHandler so re-running the identical keyword/conditions refetches instead of serving the cached first chunk. - F: compute the max_result_window guard from the floored chunkSize so a non-positive showPageLimitationL (which '??' does not catch) can't disable it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
再レビュー: PR #11617
|
| 前回指摘 | 状況 | 備考 |
|---|---|---|
P1-1 終端判定を meta.hitsCount ベースへ |
✅ 対応済み | getSearchInfiniteKey / mergeInfiniteSearchResult 双方を修正。fetchedCount >= total || lastChunkPartial に加え、コメントで「なぜ post-filter の data.length を使えないか」まで明記。ドリフト用テストも追加 |
P1-2 mutateSearching() が infinite に効かない |
❌ 未解決 | 述語は '/search/infinite' も拾うよう広げたが、SWR 仕様上 no-op のまま(→ A-1) |
P2-1 SWR key に limit / nqName |
✅ 対応済み | ISearchInfiniteConfigurations に limit / nqName を取り込み、fetcher も fixedConfigurations 経由に統一。revalidateOnFocus: false も追加 |
| P2-2 / P2-3 page-info の O(n²) / 非仮想化 | ✅ 対応済み | design.md の Non-Goals に明記(承知の上の後続対応) |
P2-4 max_result_window 到達 |
✅ 対応済み | reachedResultWindowLimit = (loadedChunks + 1) * chunkSize > ES_MAX_RESULT_WINDOW。境界式は apiPaginate.js:28 の limit + offset > 10000 と一致 |
| P3-1 リセット effect の余計な deps | ✅ 対応済み | onSelectedChangedRef に退避し deps から除去 |
P3-2 / P3-3 setSize/mutate の順序・無意味な setSize(1) |
✅ 実害なしを確認 | setSize(1) は同期でサイズを書くため後続 mutate() は 1 リクエストで済む。searchInvokedHandler も setSize(1) + mutate() になり同一条件再検索が正しく効くようになった |
| P3-4 deps に SWR オブジェクト | 分割代入は入ったが狙いの再レンダー抑止は未達(→ A-5) | |
| P3-5 JSX 内 IIFE | スタイルのみ。優先度低 | |
| P3-6 ローディング分岐の到達不能 | 挙動としては許容範囲 | |
| P3-7 legacy のチェック残り | ✅ 対応済み | リセット effect で searchResultListRef.current?.deselectAll() を呼ぶよう修正 |
| P3-8 エラー文言 | ✅ 対応済み | |
| D-1 / D-2 / D-3 dead code | ✅ 対応済み | |
D-4 loadedCount 未使用 |
❌ 未対応 | → A-6 |
| R-1 チャンクサイズ定数の重複 | ✅ 対応済み | DEFAULT_SEARCH_CHUNK_SIZE を export して単一ソース化 |
| R-2 削除完了ハンドラの 3 重複 | ❌ 未対応 | しかも挙動が乖離した(→ A-7) |
| S-1〜S-5 spec 乖離 | ✅ 対応済み | requirements 1.6 も明示 Retry に合わせて更新済み |
| S-6 実機スモーク | 下記 A-2 / A-3 は実機スモークで発見されるはずの不具合 | |
| T-1 型アサーション | ✅ 対応済み | mock<T>() に統一 |
| T-2 実現不可能シナリオ | ✅ 対応済み | 到達可能なシナリオへ書き換え |
| T-3 無意味なアサート | ✅ 対応済み |
2. 今回の指摘(重大度順)
🔴 A-1. mutateSearching() は infinite キャッシュに対して依然 no-op(P1-2 未解決)
該当: apps/app/src/stores/search.tsx:51-60
return mutate(
(key) =>
Array.isArray(key) &&
(key[0] === '/search' || key[0] === '/search/infinite'),
);述語自体は正しくなったが、swr 2.3.3 のフィルタ mutate の実装により機能しない。実ソースで確認した内容:
// swr/dist/_internal/config-context-client-v7VOFo66.mjs:243-251
const matchedKeys = [];
for (const key of cache.keys()) {
if (// Skip the special useSWRInfinite and useSWRSubscription keys.
!/^\$(inf|sub)\$/.test(key) && keyFilter(cache.get(key)._k)) {
matchedKeys.push(key);
}
}$inf$...キー(useSWRInfiniteが実際に購読しているキー)は正規表現で明示的に除外される- ページ単位キー(
['/search/infinite', keyword, offset, config])は_k付きでキャッシュに存在する(swr/dist/infinite/index.mjs:215のsetSWRCache({ data, _k: pageArg }))ため述語にはマッチする - しかし
mutateSearching()はデータ引数なしで呼ぶためargs.length < 3→startRevalidate()に入る。そこで参照するEVENT_REVALIDATORS[pageKey]は、ページ単位キーを購読しているマウント済みuseSWRが存在しない(infinite フックはinfiniteKeyのみ購読、infinite/index.mjs:119)ので undefined → 現在値を返すだけで何も起きない
⇒ 行メニューからの 削除 / リネーム / 複製で一覧が更新されない回帰が残存している。
修正案: 同リポジトリ内に既存パターンがある(apps/app/src/stores/page-listing.tsx:86-94)。
import { unstable_serialize } from 'swr/infinite';
// getKey を共有できる形に切り出したうえで
mutate(unstable_serialize(getKey));明示キー指定なら mutateByKey(_key) に直行し $inf$ スキップを通らず、infinite フックの revalidator が登録済みなので正しく再検証される。
なお
useSWRxSearch(legacy 経路)に対しては従来どおり効くため、SearchPage だけが壊れている状態。一括削除はdeleteCompletedHandlerで明示 mutate しているので、この抜けは非常に見つけにくい。
🔴 A-2. legacy ページャでプレビューが「前のページの先頭」に固定される(新規回帰)
該当: apps/app/src/features/search/client/components/SearchPage/SearchPageBase.tsx:189-201
useEffect(() => {
if (pages == null || pages.length === 0) { setSelectedPageWithMeta(undefined); return; }
if (appliedPreviewResetKeyRef.current === resetKey) return; // ← append 判定
appliedPreviewResetKeyRef.current = resetKey;
setSelectedPageWithMeta(pages[0]);
}, [pages, resetKey]);PrivateLegacyPages が使う useSWRxSearch は keepPreviousData: true。番号ページャで 2 ページ目へ移ると:
resetKey(keyword|offset|limit)が先に変わるが、pagesは まだ 1 ページ目のデータ- この commit で effect が走り、
appliedPreviewResetKeyRef.current = 新 resetKeyを立てつつ 旧ページのpages[0]をプレビューに設定 - 本来の 2 ページ目データが到着した commit では
appliedPreviewResetKeyRef.current === resetKeyなので スキップ
⇒ ページ送りしても右ペインが前ページの先頭記事のまま。旧実装([pages] 依存)では正しく追従していたため legacy の非回帰要件(design「Out of Boundary: 現状の挙動を維持」)に違反。
修正案: 適用済みフラグを「resetKey」ではなく「resetKey + そのとき採用した先頭ページ ID」で持つ、あるいは pages の参照が resetKey 変更後に実際に更新されたかを判定する。最小修正としては、フラグ更新を「新しい pages が到着した(=前回適用時の配列参照と異なる)」条件と組み合わせる。
🟠 A-3. 一括削除後、削除済みページが右ペインに残る(新規回帰)
該当: apps/app/src/features/search/client/components/SearchPage/SearchPage.tsx:228-233
const deleteCompletedHandler = useCallback(() => {
setSize(1);
mutate();
searchPageBaseRef.current?.deselectAll();
setSelectedCount(0);
}, [setSize, mutate]);削除後は resetKey が変わらないため、SearchPageBase のプレビュー系 effect (a)(b) はどちらも発火しない((b) は appliedPreviewResetKeyRef.current === resetKey でスキップ)。結果 selectedPageWithMeta は削除済みページを指したままで、右ペインにその内容が残り続ける。
旧実装は [pages] 依存だったため refetch 後に pages[0] へ復帰していた。リセット契機を resetKey へ移した副作用であり、design の「削除後は先頭のチャンクから取得し直し、一覧を再表示する」(Req 5.3)の趣旨にも反する。
PrivateLegacyPages の削除 / convert 完了ハンドラも同様。
修正案: SearchPageBase の imperative handle に resetPreview() を追加して削除完了時に呼ぶか、deleteCompletedHandler 側で resetKey を強制的に変える(世代カウンタを resetKey に混ぜる)。後者は選択クリアと一括で表現できるため、A-7 の重複解消とも相性が良い。
🟠 A-4. Retry が読み込み済み全チャンクを再取得する
該当: apps/app/src/features/search/client/components/SearchPage/SearchPage.tsx:143-145
const onRetry = useCallback(() => { mutate(); }, [mutate]);swr 2.3.3 の infinite 実装では、引数なしの mutate() は内部コンテキストに _i: true を立て(infinite/index.mjs:265)、それが forceRevalidateAll として shouldFetchPage を無条件に true にする(同 :174, :201)。parallel 未指定なので 逐次 await で回る。
⇒ 40 チャンク読み込んだ状態で 1 チャンク失敗して Retry すると、41 回の逐次リクエストが発生し、そのぶん ACTION_SEARCH_PAGE の Activity も 41 件記録される。revalidateFirstPage: false で得た省コストを打ち消す。
修正案: 失敗ページのみを再取得したい場合は、setSize(size)(同値セット)や失敗ページキーへの個別 mutate を使う。少なくとも「Retry は全チャンク再取得になる」ことをコメントで明示すること。
🟠 A-5. searchControl の useMemo は結局毎レンダー再計算(P3-4 未達)
該当: apps/app/src/features/search/client/components/SearchPage/SearchPage.tsx:237-241 / SearchPageBase.tsx:399
P3-4 の対応として const { data, error, setSize, mutate } = swr; の分割代入が入ったが、
// SearchPageBase.tsx:399 — メモ化されていないクロージャを返す
export const usePageDeleteModalForBulkDeletion = (...) => {
...
return () => { ... }; // ← 毎レンダー新しい関数
};deleteAllButtonClickedHandler が毎レンダー新しい関数 → deps に持つ collapseContents の useMemo が無効化 → それを deps に持つ searchControl の useMemo も無効化。
⇒ append のたびに SearchControl のサブツリー全体が再レンダーするという、P3-4 で潰したかった問題がそのまま残っている。
修正案: usePageDeleteModalForBulkDeletion の戻り値を useCallback で包む(pages / ref / onDeleted を deps に)。共有フックなので PrivateLegacyPages にも同時に効く。
🟡 A-6. MergedSearchResult.loadedCount が本番未使用(D-4 未対応)
該当: apps/app/src/features/search/client/util/infinite-search-result.ts:8, 39, 64
本番コードからの参照はテストのみ。加えて post-filter の値であり、P1-1 の修正コメントが「終端判定にこれを使ってはいけない」と明記しているフィールドが公開 API に残っている状態。将来の利用者が loadedCount >= total を書いて P1-1 を再発させるリスクがあるため、削除するか、用途を「表示用件数」に限定する旨をコメントで明示すること。
🟡 A-7. 削除完了処理の 3 重複(R-2 未対応)+挙動が乖離
該当: SearchPage.tsx:228 / PrivateLegacyPages.tsx:396-397, 432-433
// PrivateLegacyPages(2 箇所)
searchPageBaseRef.current?.deselectAll();
selectedPagesByCheckboxesChangedHandler(0, 0); // ← selectAllControlRef.deselect() が走る
// SearchPage
searchPageBaseRef.current?.deselectAll();
setSelectedCount(0); // ← selectAllControlRef.deselect() は走らない重複が残っているだけでなく、SearchPage だけ selectAllControlRef.current.deselect() を呼んでいない。OperateAllControl は ref 経由の非制御コンポーネントなので、削除前に checked / indeterminate だったヘッダのチェックボックスが削除後もその見た目のまま残る(内部 Set は空)。Req 4.2 / 4.6 の表示契約と食い違う。
修正案: SearchPage も selectedPagesByCheckboxesChangedHandler(0, 0) を呼ぶ形へ統一し、3 箇所を SearchPageBase 側の resetSelection()(A-3 の resetPreview() と合わせて 1 メソッド)に集約する。
3. Dead code / 重複(今回時点)
| # | 種別 | 箇所 | 内容 |
|---|---|---|---|
| A-6 | dead | infinite-search-result.ts:8 |
loadedCount が本番未使用 |
| A-7 | 重複 | SearchPage.tsx:228 / PrivateLegacyPages.tsx:396,432 |
削除完了処理が 3 箇所(かつ挙動乖離) |
| — | 重複 | PrivateLegacyPages.tsx:361 |
instance.deselectAll() の 3 箇所目。A-7 の集約対象に含めてよい |
初回指摘の D-1 / D-2 / D-3 / R-1 は解消済み。新規の dead code は検出されなかった。
4. spec と実装の乖離(今回時点)
| # | 内容 | 判定 |
|---|---|---|
| S-1〜S-5 | 初回指摘分 | ✅ すべて解消(定数共有化、requirements 3.1 の記述修正、requirements 1.6 の明示 Retry 化、tasks 記帳) |
| S-8 | design「Out of Boundary: PrivateLegacyPages の番号ページャ — 現状の挙動を維持(非回帰のみ担保)」に対し、A-2 でプレビューが回帰している |
❌ 乖離 |
| S-9 | design「削除完了時: setSize(1) + mutate() + deselectAll」にプレビューのリセットが規定されていないため、A-3 が仕様の穴として通ってしまった |
|
| S-6 | tasks 5.3 実機スモーク |
5. セキュリティ
前回同様、新規の脆弱性は見つからなかった。今回の差分でも攻撃面は増えていない。
| 観点 | 確認結果 |
|---|---|
| 攻撃面 | 新規エンドポイント・新規リクエストパラメータなし。useSWRINFxSearch は既存 apiv1 /search を同じパラメータで呼ぶのみ |
| 認可 | server/routes/search.ts:143-151 の userGroups 解決、formatSearchResult の findPageListByIds による可視性フィルタ、serializeUserSecurely()、スニペットの filterXss.process() — すべてサーバ側で不変 |
| 一括削除の範囲 | SearchPageBase.tsx:412 の pages.filter(p => selectedIds.has(p.data._id)) は変更なし。選択 Set に古い ID が残っても現在のリストに存在しないページは対象外(over-delete しない安全側)。削除実行自体はサーバ側で再認可 |
limit の入力源 |
admin 設定 showPageLimitationL 由来でユーザー入力ではない。サーバ側に LIMIT_MAX = 1000 と limit + offset <= 10000 の二重ガード。今回追加された reachedResultWindowLimit はクライアント側の追加ガードであり、サーバ側ガードを緩めてはいない |
| key に載る情報 | nqName / limit を SWR key に追加したが、キーはブラウザ内メモリキャッシュのみ。永続化・送信はされない |
| XSS | 新規描画箇所(エラー + Retry ボタン)は静的テキストのみ |
運用上の注意(脆弱性ではない)
- A-4 により Retry 1 回で最大 41 件の
ACTION_SEARCH_PAGEActivity が記録される。前回指摘した「スクロールごとの Activity 増加」に加え、エラー時にさらに増幅する。監査ログのボリューム設計に影響し得るので認識しておくこと - 「全選択」の対象が「表示中」から「累積読み込み済み」に変わる件は Req 4.1 で承認済み仕様。削除確認モーダルが対象一覧を提示するため許容範囲
6. パフォーマンス
| # | 内容 | 状況 |
|---|---|---|
| A-4 | Retry が全チャンク逐次再取得 | 🟠 要対応 |
| A-5 | append ごとに SearchControl 全体が再レンダー | 🟠 要対応(P3-4 の狙い未達) |
| P2-2 | useSWRxPageInfoForList の O(n²) + クエリ文字列長 |
✅ design.md の Non-Goals に明記(後続対応) |
| P2-3 | 非仮想化描画による DOM 肥大 | ✅ 同上 |
良い点
revalidateFirstPage: false+revalidateOnFocus: falseで append 時の無駄な再検証を抑止(useSWRxSearchとの一貫性も取れた)mergeInfiniteSearchResultはuseMemo(..., [swrData, chunkSize])済みで、append ごとの O(n) 合成のみreachedResultWindowLimitにより、深いスクロールでサーバエラーを踏む前に終端扱いできるようになった
7. テストレビュー(essential-test-design / essential-test-patterns 適用)
67 テストすべてグリーン(前回 60 → +7)。前回指摘の T-1 / T-2 / T-3 はいずれも解消済み。
✅ 改善が確認できた点
- T-1:
as unknown as Tが全廃されmock<T>()に統一 - T-2: 「未描画行のチェックボックスを発火させる」実現不可能シナリオが、到達可能なシナリオへ書き換え済み
- T-3:
expect(typeof ...).toBe('boolean')のような回帰を検出しないアサートが削除済み - 新規: P1-1(ES ドリフト時に
data.length < hitsCountになるケース)の境界テストがinfinite-search-result.spec.ts/search.spec.tsの双方に追加されている。指摘の本質を正しく押さえた良い追加
❌ 残る指摘
T-4. showPageLimitationLAtom のモックが本番と乖離
該当: apps/app/src/features/search/client/components/SearchPage/SearchPage.spec.tsx:79
vi.mock('~/states/server-configurations', () => ({
disableUserPagesAtom: atom(false),
showPageLimitationLAtom: atom<number | undefined>(undefined), // ← 本番は atom<number>(50)
}));本番の apps/app/src/states/server-configurations/server-configurations.ts:59 は atom<number>(50)。モックが undefined を返すため、テスト中の chunkSize は DEFAULT_SEARCH_CHUNK_SIZE(20)にフォールバックする。
その結果 reachedResultWindowLimit の境界テストは chunkSize 20 前提の 499/500 チャンクを検証しており、本番の境界(chunkSize 50 → 199/200 チャンク)の回帰を検出できない。「本番では起こり得ない設定でしか通らないテスト」になっている。
修正案: モックを atom<number>(50) に合わせる(本番既定と一致させる)か、chunkSize をテストパラメータ化して 20 / 50 の両方で境界を検証する。
T-5. カバレッジの穴
- A-1(
mutateSearchingの no-op)を検出するテストがない。修正時は「mutateSearching()後に infinite フックが再検証される」ことを検証するテストを併せて追加すること - A-2 / A-3(プレビューの回帰)を検出するテストがない。
SearchPageBase.spec.tsxのプレビューテストはpagesが resetKey 変更と同時に空配列になる前提(infinite 経路)のみで、keepPreviousDataにより旧データが残ったまま resetKey が変わる legacy 経路のケースが欠けている。ここを 1 ケース足せば A-2 は自動的に captured される - A-5(再レンダー抑止)は挙動テストが難しいため、テストではなくコード側の
useCallback化で担保するのが妥当
8. 総括
| 観点 | 前回 | 今回 | コメント |
|---|---|---|---|
| 設計・方針 | ⭐️⭐️⭐️⭐️⭐️ | ⭐️⭐️⭐️⭐️⭐️ | 変わらず良好 |
| 実装品質 | ⭐️⭐️⭐️ | ⭐️⭐️⭐️⭐️ | P1-1 / P2-1 / P2-4 が解決。ただし副作用で新規回帰 2 件 |
| テスト | ⭐️⭐️⭐️⭐️ | ⭐️⭐️⭐️⭐️ | T-1〜T-3 解消・ドリフト境界テスト追加は高評価。モック乖離とプレビュー回帰の穴が残る |
| セキュリティ | ⭐️⭐️⭐️⭐️⭐️ | ⭐️⭐️⭐️⭐️⭐️ | 新規リスクなし |
| spec 整合 | ⭐️⭐️⭐️⭐️ | ⭐️⭐️⭐️⭐️ | S-1〜S-5 解消。legacy 非回帰(S-8)と design の記述漏れ(S-9)が新規 |
マージ前に対応を推奨
- A-1
mutateSearching()をunstable_serialize(getKey)方式へ(stores/page-listing.tsx:86の既存パターン踏襲)+ 回帰テスト - A-2 legacy でプレビューが前ページ先頭に固定される問題(
keepPreviousData× 適用済みフラグ)+ 回帰テスト - A-3 一括削除後に削除済みページが右ペインに残る問題(design.md への追記も)
- S-6 tasks 5.3 の実機スモーク(A-2 / A-3 はここで発見されるはず)
フォローアップで対応可
- A-4 Retry の全チャンク再取得(せめてコメントで明示)
- A-5
usePageDeleteModalForBulkDeletionのuseCallback化(P3-4 の完遂) - A-6
loadedCountの削除 or 用途明記(D-4) - A-7 削除完了処理 3 箇所の集約 +
selectAllControlRef.deselect()の挙動統一(R-2) - T-4
showPageLimitationLAtomモックを本番既定(50)に合わせる - T-5 A-1 / A-2 / A-3 の回帰テスト追加
- P3-5 JSX 内 IIFE の整理(スタイル)
task: https://redmine.weseek.co.jp/issues/187701
2026-07-30.18.44.37.731.mov