weseek__growi/.kiro/specs/auto-scroll/research.md

Research & Design Decisions

Summary

• Feature: auto-scroll
• Discovery Scope: Extension (refactoring existing hook for reusability)
• Key Findings:
  • src/client/hooks/ does not exist; hooks are collocated with features — a new shared hooks directory is needed
  • SearchResultContent has independent scroll-to-highlighted-keyword logic using MutationObserver; coordination needed
  • MermaidViewer does not implement the rendering attribute protocol; DrawioViewer is the only adopter

Research Log

Hook Location and Existing Patterns

• Context: Requirement 5.5 specifies placing the hook in src/client/hooks/
• Findings:
  • apps/app/src/client/hooks/ does not exist
  • Existing hooks are collocated: features/page-tree/hooks/, features/openai/client/components/.../hooks/
  • No precedent for a top-level shared hooks directory in src/client/
• Implications: Creating src/client/hooks/ establishes a new pattern for cross-feature hooks

SearchResultContent Scroll Behavior

• Context: Requirement 5 mandates reusability for search result pages
• Sources: apps/app/src/features/search/client/components/SearchPage/SearchResultContent.tsx
• Findings:
  • Container ID: search-result-content-body-container
  • Container has overflow-y-scroll — is the scroll unit, not the viewport
  • Uses MutationObserver to find .highlighted-keyword elements and scroll to the first one using scrollWithinContainer
  • Debounced at 500ms; SCROLL_OFFSET_TOP = 30
  • Does NOT use URL hash — scrolls to highlighted search terms
  • useEffect has no dependency array (fires on every render); no cleanup (intentional per inline comment)
• Implications (updated):
  • scrollIntoView() default is inappropriate; custom scrollTo using scrollWithinContainer is required
  • When window.location.hash is non-empty, the keyword scroll overrides hash scroll after 500ms debounce — must be suppressed via early return guard
  • The resolveTarget default (document.getElementById) works correctly; heading id attributes are set by the remark pipeline

DrawioViewer Rendering Attribute Pattern

• Context: Requirement 4.4 mandates declarative true/false toggling
• Sources: packages/remark-drawio/src/components/DrawioViewer.tsx
• Findings:
  • Initial render: {[GROWI_RENDERING_ATTR]: 'true'} in JSX spread (line 188)
  • On error: removeAttribute(GROWI_RENDERING_ATTR) (line 131)
  • On complete: removeAttribute(GROWI_RENDERING_ATTR) (line 148)
  • This is imperative add/remove, not declarative value toggle
• Implications: Needs refactoring to setAttribute(attr, 'false') on completion/error instead of removeAttribute

MermaidViewer Status

• Context: Could benefit from rendering attribute protocol
• Sources: apps/app/src/features/mermaid/components/MermaidViewer.tsx
• Findings:
  • Does NOT use GROWI_RENDERING_ATTR
  • Uses mermaid.render() async with direct innerHTML assignment
  • Mermaid sanitize options only allow value attribute
• Implications: Adding Mermaid support is a separate task, not in scope for this spec, but the design should be compatible

Rendering Attribute Naming

• Context: Reviewer feedback requests a more descriptive name
• Findings:
  • Current: data-growi-rendering — ambiguous (rendering what?)
  • Candidates considered:
  • data-growi-is-rendering-in-progress — explicit but verbose
  • data-growi-rendering-status — implies multiple states
  • data-growi-content-rendering — slightly more specific
  • With declarative true/false, a boolean-style name like data-growi-is-content-rendering works well
• Implications: Selected data-growi-is-content-rendering — clearly a boolean predicate, reads naturally as is-content-rendering="true"/"false"

Architecture Pattern Evaluation

Option	Description	Strengths	Risks / Limitations	Notes
Custom hook with options object	Single hook with configurable resolveTarget and scrollTo callbacks	Clean API, single import, testable	Options object may grow over time	Selected approach
Separate hooks per page type	usePageHashScroll, useSearchScroll	Type-specific optimization	Duplicated watch/cleanup logic	Rejected — violates DRY
HOC wrapper	Higher-order component wrapping scroll behavior	Framework-agnostic	Harder to compose, less idiomatic React	Rejected — hooks are idiomatic

Design Decisions

Decision: Hook API Shape

• Context: Hook must support PageView (hash-based) and SearchResultContent (keyword-based) with different scroll strategies
• Alternatives Considered:
  1. Positional parameters — useAutoScroll(key, containerId, resolveTarget?, scrollFn?)
  2. Options object — useAutoScroll(options)
• Selected Approach: Options object with required key and contentContainerId, optional resolveTarget and scrollTo
• Rationale: Options object is extensible without breaking existing call sites and self-documents parameter intent
• Trade-offs: Slightly more verbose at call site; mitigated by clear defaults

Decision: Attribute Name

• Context: Reviewer feedback: name should clearly convey "rendering in progress"
• Selected Approach: data-growi-is-content-rendering with values "true" / "false"
• Rationale: Boolean predicate naming (is-*) is natural for a two-state attribute; content-rendering disambiguates from other rendering concepts
• Follow-up: Update @growi/core constant and all consumers

Decision: CSS Selector for In-Progress State

• Context: Requirement 4.6 — selector must match only in-progress state
• Selected Approach: [data-growi-is-content-rendering="true"] instead of bare attribute selector
• Rationale: With declarative true/false toggling, bare [attr] matches both states; value selector is required

Risks & Mitigations

• Risk: SearchResultContent's existing keyword-highlight scroll may conflict with hash-based scroll — Mitigation: Guard the keyword-scroll useEffect with if (window.location.hash.length > 0) return; so hash scroll takes priority when a hash is present; keyword scroll proceeds unchanged otherwise
• Risk: scrollIntoView() default scrolls the viewport when SearchResultContent's container has overflow-y-scroll — Mitigation: Provide a custom scrollTo closure using scrollWithinContainer with offset from the container's bounding rect
• Risk: Renaming the attribute requires coordinated changes across @growi/core, remark-drawio, and consuming apps — Mitigation: Constants are centralized; single constant rename propagates via imports
• Risk: MutationObserver on subtree: true may be expensive on large pages — Mitigation: Retained 10s maximum watch timeout from current implementation

Post-Implementation Finding: SearchResultContent Integration Misalignment

Discovered after task 6 implementation during code review conversation.

Problem

The task 6 implementation added useContentAutoScroll to SearchResultContent, but this was architecturally incorrect. useContentAutoScroll is URL-hash–driven (if (hash.length === 0) return) and will never activate in the search results context — the search page URL (/search?q=foo) carries no fragment identifier.

Actual Requirement

The real requirement for SearchResultContent is:

1. Keyword scroll (already working): scroll to the first .highlighted-keyword element when content loads, via MutationObserver + 500ms debounce
2. Re-scroll after rendering (missing): when drawio / mermaid diagrams render asynchronously after the initial keyword scroll, the layout shifts and the keyword moves out of view — watchRenderingAndReScroll should re-scroll to the keyword once rendering settles

Current Code State (as of this writing)

apps/app/src/features/search/client/components/SearchPage/SearchResultContent.tsx contains:

• useContentAutoScroll(...) call — should be removed
• keyword scroll useEffect with hash guard (if (window.location.hash.length > 0) return) — the guard may also be removable depending on how the hook is refactored
• scrollToTargetWithinContainer local helper (shared distance calculation) — keep

Proposed Refactoring Direction

Two-phase refactor, designed for the next session:

Phase 1 — Immediate fix (SearchResultContent)

Wire watchRenderingAndReScroll directly into the keyword scroll useEffect:

useEffect(() => {
  const scrollElement = scrollElementRef.current;
  if (scrollElement == null) return;

  const scrollToKeyword = (): boolean => {
    const toElem = scrollElement.querySelector('.highlighted-keyword') as HTMLElement | null;
    if (toElem == null) return false;
    scrollToTargetWithinContainer(toElem, scrollElement);
    return true;
  };

  // MutationObserver for incremental content loading (debounced)
  const observer = new MutationObserver(() => {
    scrollToFirstHighlightedKeywordDebounced(scrollElement);
  });
  observer.observe(scrollElement, MUTATION_OBSERVER_CONFIG);

  // Rendering watch: re-scroll after drawio/mermaid layout shifts
  const cleanupWatch = watchRenderingAndReScroll(scrollElement, scrollToKeyword);
  return cleanupWatch;
}, [page._id]);

Remove the useContentAutoScroll import and call entirely.

Phase 2 — Architecture improvement (shared hook)

Reorganize the relationship between useContentAutoScroll and watchRenderingAndReScroll:

• watchRenderingAndReScroll (pure function) is the core shared primitive — promote it to a named export so callers other than useContentAutoScroll can use it directly
• Consider introducing a thin React wrapper hook useRenderingRescroll(scrollToTarget, deps) that manages the useEffect lifecycle for watchRenderingAndReScroll, making it composable
• useContentAutoScroll becomes the hash-navigation–specific hook: hash guard → target resolution → initial scroll → delegates to useRenderingRescroll
• SearchResultContent keyword scroll becomes: MO-debounce → initial scroll → delegates to useRenderingRescroll
• PageView-specific logic (default scrollIntoView, getElementById resolver) stays in PageView or in useContentAutoScroll

Resulting dependency graph:

useContentAutoScroll  ─┐
                        ├── useRenderingRescroll ── watchRenderingAndReScroll
SearchResultContent   ─┘

Key Questions for Next Session Design

1. Should useRenderingRescroll be a hook (managing useEffect internally) or should callers be responsible for calling it inside their own effect? A hook is more ergonomic; a plain function is more flexible.
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 — core browser API used for DOM observation
• Element.scrollIntoView() — default scroll behavior
• PR #10853 reviewer feedback from yuki-takei — driving force for this refactoring