Merged
Conversation
…and observer cleanup
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough프론트엔드에서 채팅 타이틀 응답 시퀀스 검증과 AI 메시지의 IntersectionObserver 기반 가시성 관찰이 도입되고, 사이드바 필터 초기화 로직이 추가되었으며, 백엔드의 책 파일명 폴백 파싱 방식과 README 문서가 변경되었습니다. Changes
Sequence Diagram(s)(생성 조건 미충족 — 생략) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
frontend/components/chat/MessageList.tsx (1)
78-79: DOM 직접 쿼리 대신 React ref 패턴 사용을 권장합니다.
document.querySelectorAll(".ai-message-card")는 React의 선언적 패턴과 맞지 않으며, effect 실행 시점에 새 메시지가 아직 렌더링되지 않았을 수 있습니다.refcallback을 사용하여 각 AI 메시지 요소를 observer에 등록하는 것이 더 안정적입니다.♻️ ref callback 패턴 예시
+ const observeElement = useCallback((el: HTMLDivElement | null) => { + if (el && observer.current) { + observer.current.observe(el); + } + }, []); - const elements = document.querySelectorAll(".ai-message-card"); - elements.forEach(el => { observer.current?.observe(el); }); // In JSX: - <div key={msg.id} data-message-id={msg.id} className="ai-message-card flex gap-4 md:gap-6 group"> + <div key={msg.id} ref={observeElement} data-message-id={msg.id} className="ai-message-card flex gap-4 md:gap-6 group">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/chat/MessageList.tsx` around lines 78 - 79, Replace the document.querySelectorAll usage with a React ref callback pattern: instead of querying ".ai-message-card" in the effect, add a callback ref on the AI message element render (e.g., in the map that renders each AI message in MessageList.tsx) that calls observer.current?.observe(node) when node is non-null and observer.current?.unobserve(prev) when it becomes null; remove the document.querySelectorAll(...) loop and ensure observer.current is used consistently (observer.current?.observe / unobserve) so newly mounted messages are registered reliably.frontend/components/sidebar/Sidebar.tsx (1)
14-43: LGTM - 필터링 로직이 잘 구현되어 있습니다.철학자 필터 토글 로직과 메타데이터 중복 제거가 올바르게 구현되어 있습니다.
allMetadata에서id기준 중복 제거와handlePhilosopherClick의 토글 동작이 적절합니다.한 가지 선택적 개선 사항:
messages가 변경되어filterScholar에 해당하는 철학자가 더 이상 존재하지 않을 경우, 빈 결과가 표시될 수 있습니다. 이런 경우 필터를 자동으로 초기화하는 것을 고려해볼 수 있습니다.♻️ 선택적 개선: 존재하지 않는 철학자 필터 자동 초기화
+ // Reset filter if the filtered scholar no longer exists + useEffect(() => { + if (filterScholar && !allMetadata.some(m => m.scholar === filterScholar)) { + setFilterScholar(null); + } + }, [allMetadata, filterScholar]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/sidebar/Sidebar.tsx` around lines 14 - 43, The filter may remain set to a scholar that no longer exists in messages, producing empty results; add a useEffect that watches messages (or allMetadata) and clears filterScholar via setFilterScholar(null) when the current filterScholar value is not present in allMetadata (compare by meta.id or meta.scholar), ensuring you reference the existing state and helper names (filterScholar, setFilterScholar, allMetadata, messages) and run the effect whenever messages or allMetadata change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/scripts/generate_book_mapping.py`:
- Around line 118-122: The code redundantly re-applies Path(...).stem to
file_name in the block that computes name_without_ext; remove the extra stem
call and use the file_name argument directly (since callers already pass
file_path.stem), i.e., replace name_without_ext = Path(file_name).stem with
name_without_ext = file_name in the function that computes
fallback_title/fallback_author and before calling kyobo_fallback so titles
containing dots aren't truncated; keep the subsequent rsplit(" by ", 1) and
kyobo_fallback(...) call unchanged.
In `@frontend/app/page.tsx`:
- Around line 75-86: The title fetch can race and overwrite a new session's
title; modify the logic around isFirstMessage / the fetch to guard by a session
identifier or AbortController so only the latest request can call setChatTitle:
when initiating the title request include the current sessionId (or create an
AbortController) and store it in component state or a ref, then before calling
setChatTitle verify the response's sessionId matches the stored currentSessionId
(or ensure the controller wasn't aborted) and ignore/stale responses; update the
fetch to `${baseUrl}/api/v1/chat/title` handling to attach/validate the session
token or support aborting previous requests so stale responses cannot overwrite
the active chat title.
In `@frontend/components/chat/MessageList.tsx`:
- Around line 47-54: There are two identical definitions of
emitLatestMetadataOrEmpty; remove the duplicate and keep a single function
defined once at the top of the callback (the one that filters messages for role
"ai" and calls onVisibleMessageChange with the last metadata or an empty array).
Update usages to call that single emitLatestMetadataOrEmpty, ensuring references
to messages and onVisibleMessageChange remain unchanged (look for the
emitLatestMetadataOrEmpty definitions around the callback and the duplicate
block at lines similar to 47-54 and 63-70).
- Around line 18-85: The IntersectionObserver callback closes over stale
messages/onVisibleMessageChange; fix by storing latest values in refs (e.g.,
latestMessagesRef, latestOnVisibleMessageChangeRef) and have the observer
callback read from those refs (useVisibleMessagesRef/observerRef), so the
callback always uses current messages and callback; also defer querying DOM for
".ai-message-card" until after paint (use requestAnimationFrame or setTimeout 0
inside the effect) before calling document.querySelectorAll and observing nodes,
and ensure you update latestRefs on every render (outside the observer creation)
and only create/disconnect the observer lifecycle in the useEffect that
currently creates observer.
---
Nitpick comments:
In `@frontend/components/chat/MessageList.tsx`:
- Around line 78-79: Replace the document.querySelectorAll usage with a React
ref callback pattern: instead of querying ".ai-message-card" in the effect, add
a callback ref on the AI message element render (e.g., in the map that renders
each AI message in MessageList.tsx) that calls observer.current?.observe(node)
when node is non-null and observer.current?.unobserve(prev) when it becomes
null; remove the document.querySelectorAll(...) loop and ensure observer.current
is used consistently (observer.current?.observe / unobserve) so newly mounted
messages are registered reliably.
In `@frontend/components/sidebar/Sidebar.tsx`:
- Around line 14-43: The filter may remain set to a scholar that no longer
exists in messages, producing empty results; add a useEffect that watches
messages (or allMetadata) and clears filterScholar via setFilterScholar(null)
when the current filterScholar value is not present in allMetadata (compare by
meta.id or meta.scholar), ensuring you reference the existing state and helper
names (filterScholar, setFilterScholar, allMetadata, messages) and run the
effect whenever messages or allMetadata change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/app/api/routes/chat.pybackend/app/services/llm.pybackend/data/books_mapping.jsonbackend/scripts/generate_book_mapping.pybackend/scripts/generate_sql_updates.pybackend/scripts/update_db_metadata.pybackend/update_metadata.sqlfrontend/app/page.tsxfrontend/components/chat/ChatMain.tsxfrontend/components/chat/FloatingInput.tsxfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/ActivePhilosophers.tsxfrontend/components/sidebar/Sidebar.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/components/chat/MessageList.tsx (1)
83-85: 주석과 실제 구현이 불일치합니다.Line 83-85는
setTimeout기반 동작을 언급하지만 현재 코드에는 해당 로직이 없습니다. 오해를 줄이기 위해 주석을 현재 구현 기준으로 정리하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/chat/MessageList.tsx` around lines 83 - 85, The comment in MessageList.tsx references a setTimeout-based approach but the code uses a callback ref; update the comment to reflect the current implementation (callback ref ensuring DOM updates) and remove the mention of setTimeout to avoid confusion—edit the comment near the callback ref or the MessageList component (e.g., references to the callback ref variable or the scroll/anchor logic) so it accurately describes that the callback ref guarantees DOM readiness and any legacy setTimeout behavior has been removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/chat/MessageList.tsx`:
- Around line 27-29: The effect that creates the IntersectionObserver currently
bails out if callbackRef.current is falsy so the observer is never created later
when onVisibleMessageChange is set; update the useEffect (the block that calls
visibleMessages.current.clear()) to not early-return based on
callbackRef.current, instead always create the IntersectionObserver and attach
it to targets, and add the callback (or
callbackRef.current/onVisibleMessageChange) to the effect's dependency array so
the effect re-runs when the callback becomes available; ensure the observer uses
callbackRef.current inside its handler and that you properly disconnect/cleanup
the observer on unmount.
- Around line 92-96: The ref callback observeElement is sometimes called before
the IntersectionObserver is created (observer.current), so initial AI message
elements aren't observed; fix by lazily initializing the observer inside
observeElement when observer.current is null (create observer.current with the
same callback/options you use elsewhere) before calling
observer.current.observe(el), or alternatively queue the incoming el when
observer.current is absent and flush/observe queued elements from the effect
that sets up the observer; reference observeElement and observer.current to
locate where to change.
---
Nitpick comments:
In `@frontend/components/chat/MessageList.tsx`:
- Around line 83-85: The comment in MessageList.tsx references a
setTimeout-based approach but the code uses a callback ref; update the comment
to reflect the current implementation (callback ref ensuring DOM updates) and
remove the mention of setTimeout to avoid confusion—edit the comment near the
callback ref or the MessageList component (e.g., references to the callback ref
variable or the scroll/anchor logic) so it accurately describes that the
callback ref guarantees DOM readiness and any legacy setTimeout behavior has
been removed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdbackend/scripts/generate_book_mapping.pyfrontend/app/page.tsxfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/Sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/page.tsx
Comment on lines
+27
to
29
| useEffect(() => { | ||
| if (!callbackRef.current) return; | ||
| visibleMessages.current.clear(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "MessageList.tsx" frontend/Repository: SanghunYun95/philo-rag
Length of output: 106
🏁 Script executed:
cat -n frontend/components/chat/MessageList.tsx | sed -n '20,100p'Repository: SanghunYun95/philo-rag
Length of output: 3558
옵저버 생성이 초기 콜백 존재 여부에 의존합니다.
Line 28의 early return으로 인해, 마운트 시점에 onVisibleMessageChange가 없으면 IntersectionObserver가 생성되지 않습니다. Line 90의 빈 의존성 배열로 인해 effect는 재실행되지 않으므로, 이후 콜백이 제공되어도 옵저버를 생성할 수 없습니다.
🔧 제안 수정안
useEffect(() => {
- if (!callbackRef.current) return;
visibleMessages.current.clear();
observer.current = new IntersectionObserver((entries) => {
const currentMessages = messagesRef.current;
const callback = callbackRef.current;
- if (!callback) return;
const emitLatestMetadataOrEmpty = () => {
+ if (!callback) return;
const aiMessages = currentMessages.filter(m => m.role === "ai" && m.metadata && m.metadata.length > 0);
if (aiMessages.length > 0) {
callback(aiMessages[aiMessages.length - 1].metadata!);
} else {
callback([]);
}
};
let changed = false;
// ...
if (changed) {
+ if (!callback) return;
// ...
}
}, {
threshold: [0, 0.25, 0.5, 0.75, 1.0]
});
return () => {
observer.current?.disconnect();
visibleMessages.current.clear();
};
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/components/chat/MessageList.tsx` around lines 27 - 29, The effect
that creates the IntersectionObserver currently bails out if callbackRef.current
is falsy so the observer is never created later when onVisibleMessageChange is
set; update the useEffect (the block that calls visibleMessages.current.clear())
to not early-return based on callbackRef.current, instead always create the
IntersectionObserver and attach it to targets, and add the callback (or
callbackRef.current/onVisibleMessageChange) to the effect's dependency array so
the effect re-runs when the callback becomes available; ensure the observer uses
callbackRef.current inside its handler and that you properly disconnect/cleanup
the observer on unmount.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/chat/MessageList.tsx`:
- Around line 95-99: The observeElement ref callback currently ignores null
values so removed DOM nodes are never unobserved and stay in pendingElements and
the IntersectionObserver; update the observeElement function to handle both
mounting and unmounting: when el is non-null add it to pendingElements.current
and call observer.current?.observe(el), and when el is null iterate any
previously-tracked element (or accept the passed element being null via closure)
to call observer.current?.unobserve(...) and remove it from
pendingElements.current; reference the observeElement callback, pendingElements,
and observer.current to locate and implement this unobserve-and-cleanup logic so
observers don't leak.
- Line 86: The arrow-function used as the forEach callback creates an implicit
return which triggers the lint rule; change the callback at the
pendingElements.current.forEach(...) site in MessageList.tsx to use a block body
so it does not return a value (e.g., replace the expression-bodied "(el) =>
observer.current?.observe(el)" with a block-bodied function that simply calls
observer.current?.observe(el) or use a for/of loop), ensuring you still
reference pendingElements.current and observer.current?.observe.
In `@README.md`:
- Line 12: Update the README version entries for React and Next.js to match
package.json: replace the React version text (currently "18") with "19.2.4" and
replace the Next.js version text (currently "14+") with "16.1.6" at both README
locations referenced (the top framework line and the second occurrence around
line 123); ensure both spots use the exact package.json versions so the
documentation and dependencies remain consistent.
…-deps, and update docs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/components/sidebar/Sidebar.tsx (1)
48-48: 광범위한 ESLint 비활성화는 지양해주세요.Line 48의
// eslint-disable-next-line은 불필요하게 넓은 범위의 규칙을 숨길 수 있습니다. 필요한 규칙만 명시하거나, 가능하면 해당 주석을 제거하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/sidebar/Sidebar.tsx` at line 48, There is a broad `// eslint-disable-next-line` in frontend/components/sidebar/Sidebar.tsx (inside the Sidebar component) that silences all rules; instead determine which specific ESLint rule is failing for that line and replace the comment with a targeted disable (e.g., `// eslint-disable-next-line <rule-name>`) or, if possible, remove the disable entirely and fix the underlying issue; locate the `// eslint-disable-next-line` token and either delete it or change it to a rule-specific disable after confirming the exact rule reported by your linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/chat/MessageList.tsx`:
- Around line 104-117: In the observeElement callback, when you unobserve or
delete an element (in the branch handling prev && prev !== el and in the else
branch when el is null), also remove the corresponding entry from
visibleMessages so stale visibility data isn't used for ratio/max calculations;
update the logic surrounding elementById.current, pendingElements.current, and
observer.current to call visibleMessages.delete(id) (or the appropriate removal
method) whenever an element is unobserved or removed to keep visibility state
consistent.
---
Nitpick comments:
In `@frontend/components/sidebar/Sidebar.tsx`:
- Line 48: There is a broad `// eslint-disable-next-line` in
frontend/components/sidebar/Sidebar.tsx (inside the Sidebar component) that
silences all rules; instead determine which specific ESLint rule is failing for
that line and replace the comment with a targeted disable (e.g., `//
eslint-disable-next-line <rule-name>`) or, if possible, remove the disable
entirely and fix the underlying issue; locate the `// eslint-disable-next-line`
token and either delete it or change it to a rule-specific disable after
confirming the exact rule reported by your linter.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/Sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
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 by CodeRabbit
버그 수정
변경 사항
문서