Преглед изворни кода

refactor: reorganize auto-scroll modules for co-location and clarity

Yuki Takei пре 1 дан
родитељ
комит
040c0a82d5

+ 113 - 0
.kiro/specs/auto-scroll/design.md

@@ -441,6 +441,119 @@ useContentAutoScroll({
 
 ---
 
+## Task 8 Design: Module Reorganization
+
+### Background
+
+After tasks 1–7, the auto-scroll modules are functional but organizationally misaligned:
+
+- `useContentAutoScroll` lives in a shared hooks directory (`src/client/hooks/use-content-auto-scroll/`) but is effectively PageView-specific — it depends on `window.location.hash` and only PageView uses it
+- `watchRenderingAndReScroll` lives alongside that hook as if it were hook-internal, but it is a plain function and the actual shared primitive used by both PageView and SearchResultContent
+- SearchResultContent's keyword-scroll + rendering-watch logic is inlined in the component, not extracted into a testable hook
+
+### Goals
+
+- Place each module where its consumer lives (co-location)
+- Move the only truly shared primitive (`watchRenderingAndReScroll`) to the shared utility directory
+- Remove the `src/client/hooks/use-content-auto-scroll/` directory — it no longer represents a shared hook
+
+### Non-Goals
+
+- Changing any scroll behavior or logic — this is a pure reorganization
+- Simplifying `useContentAutoScroll`'s generic options (`resolveTarget`, `scrollTo`) — retained for future extensibility
+
+### Architecture After Reorganization
+
+```mermaid
+graph TB
+    subgraph shared_util[src/client/util]
+        WRRS[watch-rendering-and-rescroll.ts]
+    end
+
+    subgraph page_view[src/components/PageView]
+        UHAS[use-hash-auto-scroll.ts]
+        PV[PageView.tsx]
+    end
+
+    subgraph search[features/search/.../SearchPage]
+        UKR[use-keyword-rescroll.ts]
+        SRC[SearchResultContent.tsx]
+    end
+
+    PV -->|calls| UHAS
+    UHAS -->|imports| WRRS
+    SRC -->|calls| UKR
+    UKR -->|imports| WRRS
+```
+
+### File Moves
+
+| Before | After | Change |
+|--------|-------|--------|
+| `src/client/hooks/use-content-auto-scroll/watch-rendering-and-rescroll.ts` | `src/client/util/watch-rendering-and-rescroll.ts` | Move to shared util (not a hook) |
+| `src/client/hooks/use-content-auto-scroll/watch-rendering-and-rescroll.spec.tsx` | `src/client/util/watch-rendering-and-rescroll.spec.tsx` | Co-locate test with source |
+| `src/client/hooks/use-content-auto-scroll/use-content-auto-scroll.ts` | `src/components/PageView/use-hash-auto-scroll.ts` | Rename + move next to consumer |
+| `src/client/hooks/use-content-auto-scroll/use-content-auto-scroll.spec.tsx` | `src/components/PageView/use-hash-auto-scroll.spec.tsx` | Co-locate test with source |
+| SearchResultContent.tsx (inline keyword-scroll effect) | `features/search/.../SearchPage/use-keyword-rescroll.ts` | Extract into co-located hook |
+| (new) | `features/search/.../SearchPage/use-keyword-rescroll.spec.tsx` | New test file for extracted hook |
+| `src/client/hooks/use-content-auto-scroll/` | (deleted) | Directory removed after all files moved |
+
+### Component Details
+
+#### watch-rendering-and-rescroll (move only)
+
+No code changes. Moved from `src/client/hooks/use-content-auto-scroll/` to `src/client/util/` because it is a plain function, not a React hook. Co-located with `smooth-scroll.ts` — both are DOM scroll utilities.
+
+#### use-hash-auto-scroll (rename + move)
+
+Renamed from `useContentAutoScroll` to `useHashAutoScroll`. The name reflects that this hook is hash-navigation–driven (`window.location.hash`). Logic is unchanged — the two-phase flow (target resolution → rendering watch) and generic options (`resolveTarget`, `scrollTo`) are preserved.
+
+- **Export**: `useHashAutoScroll` (named export), `UseHashAutoScrollOptions` (type export)
+- **Import update**: `watchRenderingAndReScroll` imported from `~/client/util/watch-rendering-and-rescroll`
+- **Consumer**: `PageView.tsx` updates its import path and function name
+
+#### use-keyword-rescroll (new — extracted from SearchResultContent)
+
+Extracts the keyword-scroll `useEffect` from `SearchResultContent.tsx` (lines 133–161) into a standalone hook.
+
+```typescript
+interface UseKeywordRescrollOptions {
+  /** Ref to the scrollable container element */
+  scrollElementRef: RefObject<HTMLElement | null>;
+  /** Unique key that triggers re-execution (typically page._id) */
+  key: string;
+}
+
+function useKeywordRescroll(options: UseKeywordRescrollOptions): void;
+```
+
+**Responsibilities:**
+- MutationObserver on container for keyword highlight detection (debounced 500ms)
+- `watchRenderingAndReScroll` integration for async renderer compensation
+- Cleanup of both MO and rendering watch on key change or unmount
+
+**Import**: `watchRenderingAndReScroll` from `~/client/util/watch-rendering-and-rescroll`
+
+`scrollToTargetWithinContainer` and `scrollToFirstHighlightedKeyword` helper functions move into the hook file (or stay in SearchResultContent if other code in the component also uses them).
+
+### Import Update Summary
+
+| File | Old Import | New Import |
+|------|-----------|------------|
+| `PageView.tsx` | `~/client/hooks/use-content-auto-scroll/use-content-auto-scroll` | `./use-hash-auto-scroll` |
+| `use-hash-auto-scroll.ts` | `./watch-rendering-and-rescroll` | `~/client/util/watch-rendering-and-rescroll` |
+| `use-keyword-rescroll.ts` | (inline code) | `~/client/util/watch-rendering-and-rescroll` |
+| `SearchResultContent.tsx` | `~/client/hooks/use-content-auto-scroll/watch-rendering-and-rescroll` | `./use-keyword-rescroll` |
+
+### Testing Strategy
+
+- **`watch-rendering-and-rescroll.spec.tsx`**: Move as-is; no test changes (same logic, new path)
+- **`use-hash-auto-scroll.spec.tsx`**: Move + rename imports; no test logic changes
+- **`use-keyword-rescroll.spec.tsx`**: Migrate relevant tests from `SearchResultContent.spec.tsx` (rendering watch assertions) and add hook-level tests for keyword scroll behavior
+- **`SearchResultContent.spec.tsx`**: Simplify — component tests verify that `useKeywordRescroll` is called with correct props; detailed scroll behavior tested in the hook's own spec
+
+---
+
 ## Error Handling
 
 ### Error Strategy

+ 62 - 0
.kiro/specs/auto-scroll/research.md

@@ -178,6 +178,68 @@ SearchResultContent   ─┘
 2. The current keyword-scroll `useEffect` has no dependency array (fires every render) and no cleanup — intentional per inline comment. Adding `[page._id]` deps and a cleanup changes this behavior. Is that safe?
 3. Should the hash guard on the keyword-scroll `useEffect` be removed once `useContentAutoScroll` is also removed from `SearchResultContent`?
 
+## Task 8 Analysis: useRenderingRescroll Hook Extraction
+
+### Investigation (2026-04-06)
+
+**Objective**: Determine whether extracting a shared `useRenderingRescroll` hook is architecturally beneficial after tasks 1–7 completion.
+
+**Method**: Code review of current implementations — `useContentAutoScroll` (108 lines), `watchRenderingAndReScroll` (85 lines), `SearchResultContent` keyword-scroll effect (lines 133–161).
+
+### Findings
+
+**1. Hook extraction is architecturally infeasible for `useContentAutoScroll`**
+
+`useContentAutoScroll` calls `watchRenderingAndReScroll` conditionally inside its `useEffect`:
+- On the immediate path: only after `scrollToTarget()` returns true (line 77)
+- On the deferred path: only after the MutationObserver detects the target element (line 91)
+
+React hooks cannot be called conditionally or inside callbacks. A `useRenderingRescroll` hook would need an "enabled" flag pattern, adding complexity without simplification.
+
+**2. Co-located cleanup in SearchResultContent prevents separation**
+
+The keyword-scroll `useEffect` in `SearchResultContent` (lines 135–160) combines:
+- MutationObserver for keyword highlight detection
+- `watchRenderingAndReScroll` for async renderer compensation
+- Single cleanup return that handles both
+
+Extracting the watch into a separate hook would split cleanup across two effects, making the lifecycle harder to reason about.
+
+**3. All three design questions from the original research are resolved**
+
+| Question | Resolution | How |
+|----------|------------|-----|
+| Hook vs. function | Plain function | Conditional call inside effect prevents hook usage |
+| `[page._id]` deps + cleanup safe? | Yes, safe | Implemented in task 7.1, working correctly |
+| Hash guard removal | Already done | Removed in task 7.1 alongside `useContentAutoScroll` removal |
+
+**4. Current architecture is already optimal**
+
+`watchRenderingAndReScroll` as a plain function returning a cleanup closure is the correct abstraction level:
+- Composable into any `useEffect` (conditional or unconditional)
+- No React runtime coupling (testable without `renderHook`)
+- Clean dependency graph with two independent consumers
+
+### Initial Recommendation (superseded)
+
+Initially recommended closing Task 8 without code changes. However, after discussion the scope was revised from "hook extraction" to "module reorganization" — see below.
+
+### Revised Direction: Module Reorganization (2026-04-06)
+
+**Context**: The user observed that while a shared `useRenderingRescroll` hook adds no value (confirmed by analysis above), the current file layout is inconsistent:
+
+1. `useContentAutoScroll` lives in `src/client/hooks/` (shared) but is PageView-specific (hash-dependent)
+2. `watchRenderingAndReScroll` lives next to that hook as if internal, but is the actual shared primitive
+3. SearchResultContent's scroll logic is inlined rather than extracted
+
+**Revised approach**:
+- Move `watchRenderingAndReScroll` to `src/client/util/` — co-located with `smooth-scroll.ts` (both are DOM scroll utilities)
+- Rename `useContentAutoScroll` → `useHashAutoScroll` and move next to `PageView.tsx`
+- Extract keyword-scroll effect from `SearchResultContent` into co-located `useKeywordRescroll` hook
+- Delete `src/client/hooks/use-content-auto-scroll/` directory
+
+**Rationale**: Module co-location over shared directory. Each hook lives next to its only consumer. Only the truly shared primitive (`watchRenderingAndReScroll`) stays in a shared directory — and it moves from `hooks/` to `util/` since it's a plain function, not a hook.
+
 ## References
 - [MutationObserver API](https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver) — core browser API used for DOM observation
 - [Element.scrollIntoView()](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView) — default scroll behavior

+ 1 - 1
.kiro/specs/auto-scroll/spec.json

@@ -16,7 +16,7 @@
     "tasks": {
       "generated": true,
       "approved": false,
-      "notes": "Tasks 1–7 implemented. Task 8 is Phase 2 architecture refactoring — requires design review before implementation. See research.md § Post-Implementation Finding."
+      "notes": "Task 8 redesigned as module reorganization (5 subtasks). Tasks 1–7 complete."
     }
   },
   "ready_for_implementation": false

+ 32 - 17
.kiro/specs/auto-scroll/tasks.md

@@ -104,11 +104,9 @@
 
 ---
 
-## Phase 2: Architecture Refactoring (Next Session)
+## Phase 2: Module Reorganization
 
-> **Context**: Tasks 6.1–6.3 were implemented but are architecturally incorrect. `useContentAutoScroll` is URL-hash–driven and never activates on the search page (`/search?q=foo`). The actual need is to re-scroll to `.highlighted-keyword` after async renderers (drawio, mermaid) cause layout shifts. See `research.md` § "Post-Implementation Finding" for full analysis.
->
-> **Recommended model**: Opus — this phase requires design decisions on hook decomposition.
+> **Context**: Tasks 1–7 delivered all functional requirements. Task 8 reorganizes modules for co-location: each hook moves next to its consumer, and the shared rendering watch utility moves to `src/client/util/`. No behavior changes — pure structural improvement.
 
 - [x] 7. Fix SearchResultContent: replace `useContentAutoScroll` with `watchRenderingAndReScroll`
 - [x] 7.1 Wire `watchRenderingAndReScroll` into keyword-scroll effect
@@ -124,16 +122,33 @@
   - Add tests that `watchRenderingAndReScroll` re-scrolls to `.highlighted-keyword` after a rendering element settles
   - Update MutationObserver suppression test: remove the hash-guard test (guard will be gone)
 
-- [ ] 8. (Design required) Extract shared `useRenderingRescroll` hook
-  - **Design this task in the next session before implementing**
-  - Goal: provide a reusable React hook that wraps `watchRenderingAndReScroll` with `useEffect` lifecycle management, composable by both `useContentAutoScroll` (hash path) and `SearchResultContent` (keyword path)
-  - Key design questions to resolve (see research.md):
-    1. Hook vs. plain function — who owns the `useEffect`?
-    2. Is adding `[page._id]` deps + cleanup to the keyword-scroll effect safe given the original intentional no-cleanup design?
-    3. Should the hash guard on keyword-scroll be removed entirely?
-  - Proposed dependency graph after refactor:
-    ```
-    useContentAutoScroll  ─┐
-                            ├── useRenderingRescroll ── watchRenderingAndReScroll
-    SearchResultContent   ─┘
-    ```
+- [ ] 8. Reorganize auto-scroll modules by co-locating hooks with their consumers
+- [ ] 8.1 Move the rendering watch utility to the shared utility directory
+  - Move the rendering watch function and its test file from the shared hooks directory to the client utility directory, alongside the existing smooth-scroll utility
+  - Update the import path in the hash-based auto-scroll hook to reference the new location
+  - Update the import path in SearchResultContent to reference the new location
+  - Run existing tests to verify no regressions from the path change
+  - _Requirements: 5.4, 5.5_
+- [ ] 8.2 Rename and move the hash-based auto-scroll hook next to PageView
+  - Rename the hook and its options type to reflect its hash-navigation–specific purpose (not a generic "content auto-scroll")
+  - Move the hook file and its test file to the PageView component directory
+  - Update PageView's import to use the co-located hook with the new name
+  - Update the hook's internal import of the rendering watch utility to use the path established in 8.1
+  - Run existing tests to verify the rename and move introduce no regressions
+  - _Requirements: 5.4, 5.5_
+- [ ] 8.3 Extract the keyword-scroll effect from SearchResultContent into a co-located hook
+  - Create a new hook that encapsulates the MutationObserver-based keyword detection, debounced scroll, and rendering watch integration currently inlined in the component
+  - Accept a ref to the scrollable container and a trigger key as inputs
+  - Move the scroll helper functions (container-relative scroll calculation, first-highlighted-keyword lookup) into the hook file if they are used only by this logic
+  - Replace the inline useEffect in SearchResultContent with a single call to the new hook
+  - _Requirements: 5.4, 5.5, 6.1_
+- [ ] 8.4 (P) Write tests for the extracted keyword-rescroll hook
+  - Migrate rendering watch assertions from SearchResultContent tests into the new hook's test file
+  - Add tests for keyword scroll behavior: MutationObserver setup, debounced scroll to the first highlighted keyword, cleanup on key change and unmount
+  - Simplify SearchResultContent tests to verify the hook is called with the correct container ref and key, without re-testing internal scroll behavior
+  - _Requirements: 6.1, 6.2_
+- [ ] 8.5 (P) Remove the old shared hooks directory and verify no stale imports
+  - Delete the now-empty auto-scroll hooks directory
+  - Search the codebase for any remaining references to the old directory path and fix them
+  - Run the full test suite and type check to confirm the reorganization is complete
+  - _Requirements: 5.5_