Merged
Conversation
… retry/error display fix
…or-handling # Conflicts: # frontend/app/globals.css # frontend/app/layout.tsx # frontend/app/page.tsx # frontend/components/chat/ChatMain.tsx # frontend/components/chat/FloatingInput.tsx # frontend/components/chat/MessageList.tsx # frontend/components/sidebar/ActivePhilosophers.tsx # frontend/components/sidebar/ContextSources.tsx # frontend/components/sidebar/Sidebar.tsx # frontend/package.json # frontend/tsconfig.json
…, keys, subset and mock structure
…gurations, SSE, and frontend interactivity
📝 WalkthroughWalkthrough채팅 엔드포인트에 사용자 대화 이력(history) 전달을 추가하고, 비동기 LLM 스트리밍 경로 및 지연 초기화를 도입했습니다. 클라이언트 요청은 최근 메시지(history)를 포함해 전송되고, 서버는 분당 5회(rate limit)를 적용한 뒤 검색→LLM 스트리밍으로 응답합니다. 변경 사항
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (/api/v1/chat)
participant Limiter as RateLimiter
participant Search as VectorDB\n(_search_documents)
participant LLM as LLM Service\n(get_llm / streaming)
participant ClientStream as SSE Stream
Client->>API: POST {query, history}
API->>Limiter: check(client IP)
Limiter-->>API: allowed / blocked
alt allowed
API->>Search: embed(query) -> _search_documents
Search-->>API: context documents
API->>LLM: get_response_stream_async(context, query, history)
LLM-->>API: streaming chunks (async)
API->>ClientStream: SSE events (metadata / data / end)
else blocked
API-->>Client: 429 Too Many Requests
end
코드 리뷰 예상 소요 시간🎯 4 (Complex) | ⏱️ ~45분 관련 가능성이 있는 PR
개요채팅 엔드포인트에 대화 이력(history) 전달을 추가하고, 분당 5회(rate limit) 제한을 적용합니다. LLM 초기화를 지연(get_llm)하도록 리팩토링하고 비동기 스트리밍(get_response_stream_async)을 도입했습니다. 프론트엔드는 최근 10개 메시지를 이력으로 전송하고 429 응답을 처리하도록 변경되었으며, 테스트와 테스트 인프라가 이에 맞춰 업데이트되었습니다. 시 🐰
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (11)
frontend/components/sidebar/ContextSources.tsx (1)
24-27:title을 key로 사용하는 것은 적절하나, 빈 문자열 가능성 확인 필요
uniqueBooks가title기준으로 중복 제거되므로 key로 사용하는 것은 논리적으로 맞습니다. 다만,book_info.title이 빈 문자열("")이거나 동일한 빈 문자열을 가진 항목이 여러 개 있을 경우 key 충돌이 발생할 수 있습니다.
DocumentMetadata.id가 고유하다면 더 안전한 대안이 될 수 있습니다:<li key={meta.id} className="-m-2">단, 현재 데이터에서 title이 항상 유효한 값이라면 기존 방식도 문제없습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/sidebar/ContextSources.tsx` around lines 24 - 27, Currently the list item key uses book_info.title which can be an empty string or duplicate; change the key to a stable unique identifier such as meta.id (DocumentMetadata.id) to avoid React key collisions—update the <li key=...> for the map over uniqueBooks to use meta.id instead of meta.book_info.title and ensure meta.id exists (fallback to a concatenation like `${meta.id}-${meta.book_info.title}` if necessary).frontend/components/sidebar/Sidebar.tsx (2)
47-55:disabled속성 사용 시aria-disabled와tabIndex={-1}은 중복입니다.HTML
disabled속성이 설정되면 버튼은 자동으로 탭 순서에서 제외되고 접근성 트리에 비활성 상태로 전달됩니다.aria-disabled="true"와tabIndex={-1}은 불필요합니다.♻️ 간소화된 버전
- <button type="button" aria-label="설정" aria-disabled="true" disabled tabIndex={-1} className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="설정 (준비 중)"> + <button type="button" aria-label="설정" disabled className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="설정 (준비 중)"> <Settings className="w-5 h-5" /> </button> - <button type="button" aria-label="대화 기록" aria-disabled="true" disabled tabIndex={-1} className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="대화 기록 (준비 중)"> + <button type="button" aria-label="대화 기록" disabled className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="대화 기록 (준비 중)"> <History className="w-5 h-5" /> </button> - <button type="button" aria-label="프로필" aria-disabled="true" disabled tabIndex={-1} className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="프로필 (준비 중)"> + <button type="button" aria-label="프로필" disabled className="p-2 rounded-lg text-white/20 cursor-not-allowed transition-colors focus-visible:outline-none" title="프로필 (준비 중)"> <User className="w-5 h-5" /> </button>🤖 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 47 - 55, In the Sidebar component, remove the redundant aria-disabled="true" and tabIndex={-1} attributes from the disabled buttons (the ones rendering Settings, History and User icons) because the native disabled attribute already removes them from the tab order and exposes the disabled state to assistive tech; keep the disabled prop, aria-label and title as-is and ensure other classes (e.g., cursor-not-allowed, focus-visible:outline-none) remain if styling/visual cues are desired.
29-29:console.log는 디버그/플레이스홀더 코드로 보입니다.프로덕션 코드에
console.log가 남아있습니다. 실제 기능 구현이 완료되면 제거하거나, 향후 구현 계획이 있다면 TODO 주석을 추가하는 것이 좋습니다.♻️ 제안된 수정
- <ActivePhilosophers metadata={currentMetadata} onPhilosopherClick={(scholar) => console.log('Philosopher clicked:', scholar)} /> + <ActivePhilosophers + metadata={currentMetadata} + onPhilosopherClick={(scholar) => { + // TODO: 철학자 클릭 시 상세 정보 표시 또는 필터링 기능 구현 + }} + />🤖 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 29, The inline console.log in the ActivePhilosophers usage is leftover debug code; replace it with a real handler or a clear TODO and remove the console.log. Update the onPhilosopherClick prop on ActivePhilosophers (where currentMetadata is passed) to either call the real click handler (e.g., a named function like handlePhilosopherClick) or, if the feature is not implemented yet, replace the console.log with a TODO comment such as “// TODO: implement handlePhilosopherClick” and pass a noop or forward the event to the parent prop instead of logging.frontend/components/chat/MessageList.tsx (1)
78-104: 클릭 가능 UI 신호와 실제 동작을 맞춰주세요.Line 78에서 카드 전체에
cursor-pointer를 주지만, 실제 클릭 동작은 Line 95~104의 아이콘 버튼에만 있습니다.onOpenCitation이 없을 때 특히 오해를 유발합니다. 카드도 클릭 가능하게 하거나, 반대로 포인터/호버 강조를 버튼에만 한정하는 편이 좋습니다.수정 예시 (카드 클릭 허용)
- <div key={idx} className="mt-8 flex gap-4 p-4 rounded-xl bg-white/5 border border-white/10 max-w-xl hover:border-primary/30 transition-colors cursor-pointer group/card"> + <div + key={meta.id} + className={`mt-8 flex gap-4 p-4 rounded-xl bg-white/5 border border-white/10 max-w-xl transition-colors ${ + onOpenCitation ? "hover:border-primary/30 cursor-pointer group/card" : "" + }`} + onClick={onOpenCitation ? () => onOpenCitation(meta) : undefined} + > ... - onClick={() => onOpenCitation(meta)} + onClick={(e) => { + e.stopPropagation(); + onOpenCitation(meta); + }}🤖 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 - 104, The card UI uses cursor-pointer and group/card hover styles but only the icon button triggers the action, causing a mismatch; make the entire card clickable by moving the onClick handler from the button to the outer div (the element with class "group/card" that renders the card) and ensure onOpenCitation(meta) is called when that div is clicked (retain the button for accessibility but remove its onClick or keep it as duplicate activation), and preserve aria-label on the button and focus-visible styles so keyboard and screen-reader users still have an accessible control; update any conditional rendering around onOpenCitation so the card click is only attached when onOpenCitation is defined.backend/pytest_log_utf8.txt (1)
16-21: DeprecationWarning에 대한 정보 제공Google protobuf 라이브러리에서 Python 3.14에서 deprecated될 기능을 사용하고 있다는 경고가 발생하고 있습니다. 이것은 외부 라이브러리의 문제이므로 직접 수정할 수는 없지만, 다음 사항들을 알아두시면 좋습니다:
현재 상황:
- Python 3.12에서는 경고만 표시되며 동작에는 문제 없음
- Python 3.14부터는 오류가 발생할 수 있음
권장 조치:
- 단기적으로는 무시해도 됩니다
- Google protobuf 라이브러리를 최신 버전으로 유지하세요
- Python 3.14로 업그레이드하기 전에 호환성을 재검토하세요
다음 명령어로 현재 protobuf 버전과 최신 버전을 확인할 수 있습니다:
#!/bin/bash # protobuf 라이브러리 버전 확인 # 현재 설치된 버전 확인 python -c "import google.protobuf; print(google.protobuf.__version__)" 2>/dev/null || echo "protobuf not found" # requirements.txt에서 버전 확인 rg "protobuf|grpcio-tools" --type txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/pytest_log_utf8.txt` around lines 16 - 21, 테스트 로그에 나타난 "google._upb._message.MessageMapContainer" 및 "google._upb._message.ScalarMapContainer" 관련 DeprecationWarning은 외부 protobuf 라이브러리의 Python 3.14 호환성 문제이므로, requirements/lock 파일에서 protobuf 패키지 버전을 최신으로 고정하고(또는 업그레이드), pytest 실행 시 불필요한 noise를 줄이려면 pytest.ini 또는 테스트 런너 설정에 DeprecationWarning 필터를 추가하여 해당 경고를 무시하도록 구성하며(필터 조건에 위 두 타입/모듈명 포함), CI 파이프라인에 "python -c 'import google.protobuf; print(google.protobuf.__version__)'" 검사 또는 의존성 스캔을 추가해 프로토콜 버전이 업그레이드되었는지 모니터링하도록 변경하세요.frontend/components/chat/FloatingInput.tsx (1)
50-52:onKeyDown에e.nativeEvent.isComposing를 추가로 확인하되, 50ms 그레이스 피리어드 적용 범위를 명확히 하세요.현재 가드는 ref 기반 조합 추적으로 충분히 안전하지만,
e.nativeEvent.isComposing를 추가로 확인하면 브라우저/IME 조합별 이벤트 순서 편차에 더 견고해집니다. 다만 제안된 diff는 50ms 그레이스 피리어드를 Enter 키에만 적용하도록 변경하는데, 이는 다른 키의 동작 방식을 변경할 수 있습니다.다음과 같이 구조를 유지하면서 방어 로직을 강화하는 것을 권장합니다:
개선된 제안 diff
onKeyDown={(e) => { - if (isComposing.current || Date.now() - lastCompositionEndAt.current < 50) return; + const nativeIsComposing = (e.nativeEvent as InputEvent).isComposing ?? false; + if (isComposing.current || nativeIsComposing || Date.now() - lastCompositionEndAt.current < 50) return; if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); handleSend(); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/chat/FloatingInput.tsx` around lines 50 - 52, Update the onKeyDown handler in FloatingInput.tsx to also check e.nativeEvent.isComposing in addition to the existing isComposing.current ref, and keep the 50ms grace check (lastCompositionEndAt.current) applied only for Enter key presses so other keys' behavior is unchanged; specifically, inside the onKeyDown callback, return early if e.nativeEvent.isComposing || isComposing.current, and for the Enter-key branch also ensure Date.now() - lastCompositionEndAt.current < 50 prevents handling within the grace window.frontend/app/page.tsx (1)
63-93: SSE 파서 구현이 잘 되어 있습니다.이벤트 타입별 처리(metadata, content, error)가 명확하게 분리되어 있습니다.
가독성을 위해
processLine함수를 컴포넌트 외부로 분리하거나useCallback으로 감싸는 것을 고려해 볼 수 있습니다. 현재 구현도 기능적으로는 문제없습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/page.tsx` around lines 63 - 93, The review suggests improving readability by extracting the inline SSE parser or memoizing it; move the processLine function out of the component or wrap it with useCallback so it's not re-created on every render, keeping its behavior identical (use the same function name processLine and ensure it still accepts (line: string, eventObj: { current: string }) and references aiMsgId and setMessages correctly), and if you move it outside the component pass in necessary closures (aiMsgId, setMessages) as parameters or memoize them to avoid stale values.backend/app/core/rate_limit.py (1)
1-4: LGTM! 기본적인 rate limiter 설정이 올바르게 구현되었습니다.slowapi를 사용한 IP 기반 rate limiting이 적절하게 구성되어 있습니다.
참고: 리버스 프록시(nginx, load balancer 등) 뒤에서 운영될 경우,
get_remote_address가 실제 클라이언트 IP 대신 프록시 IP를 반환할 수 있습니다. 프로덕션 환경에서는X-Forwarded-For헤더를 처리하는 커스텀 key function을 고려해 보세요.🔧 프록시 환경을 위한 개선 예시
from slowapi import Limiter from slowapi.util import get_remote_address +from starlette.requests import Request + +def get_real_client_ip(request: Request) -> str: + """Get real client IP considering X-Forwarded-For header.""" + forwarded = request.headers.get("X-Forwarded-For") + if forwarded: + return forwarded.split(",")[0].strip() + return get_remote_address(request) -limiter = Limiter(key_func=get_remote_address) +limiter = Limiter(key_func=get_real_client_ip)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/rate_limit.py` around lines 1 - 4, The current limiter uses slowapi.util.get_remote_address which can return the proxy IP behind nginx/load balancers; implement a custom key function (e.g., get_client_ip) that parses X-Forwarded-For (respecting a trusted proxies list or first/last IP policy) and falls back to get_remote_address, then instantiate limiter = Limiter(key_func=get_client_ip) so rate limits use the real client IP.backend/requirements.txt (1)
14-14: 테스트 의존성을 분리하는 것을 권장합니다.
pytest-asyncio는 테스트 전용 의존성입니다. 프로덕션 환경에서 불필요한 패키지 설치를 피하기 위해 별도의requirements-dev.txt또는requirements-test.txt파일로 분리하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/requirements.txt` at line 14, Move the test-only dependency pytest-asyncio out of backend/requirements.txt into a new dev/test requirements file (e.g., requirements-dev.txt or requirements-test.txt): remove the line "pytest-asyncio>=0.23.0" from backend/requirements.txt, add it to the new file (keeping the same version constraint), and update any CI/test scripts or developer setup docs to install the dev requirements (pip install -r requirements-dev.txt) when running tests; ensure the project README or contributing docs mention the new dev requirements file.backend/app/api/routes/chat.py (2)
4-19:ChatRequest.history기본값 선언을default_factory로 명확히 두는 것이 좋습니다.Line 19의
= []대신Field(default_factory=list)를 쓰면 의도가 분명해지고 모델 기본값 관리가 더 안전해집니다.♻️ 제안 수정안
-from typing import List, Dict, Optional +from typing import List, Dict @@ -from pydantic import BaseModel +from pydantic import BaseModel, Field @@ class ChatRequest(BaseModel): query: str - history: Optional[List[Dict[str, str]]] = [] + history: List[Dict[str, str]] = Field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 4 - 19, Change ChatRequest.history to use a pydantic default_factory instead of a shared mutable default; import Field from pydantic and update the ChatRequest model so the history field is declared like history: Optional[List[Dict[str, str]]] = Field(default_factory=list) (reference symbols: ChatRequest, history, Field) to avoid using a mutable list as a default.
85-91: 히스토리 문자열 구성에 상한이 없어 긴 대화에서 비용/지연이 급증할 수 있습니다.Line 85-91에서 전체 히스토리를 무제한 누적하고 있어 토큰 사용량과 응답 지연이 빠르게 커질 수 있습니다. 최근 N개 + 메시지 길이 제한을 두는 것이 안정적입니다.
⚙️ 제안 수정안
- formatted_history = "" - if history: - for msg in history: - role_name = "User" if msg.get("role") == "user" else "Agent (PhiloRAG)" - formatted_history += f"{role_name}: {msg.get('content')}\n\n" + MAX_HISTORY_MESSAGES = 20 + MAX_HISTORY_CHARS = 1000 + history_tail = (history or [])[-MAX_HISTORY_MESSAGES:] + formatted_parts: List[str] = [] + + for msg in history_tail: + role_name = "User" if msg.get("role") == "user" else "Agent (PhiloRAG)" + content = (msg.get("content") or "").strip() + if not content: + continue + formatted_parts.append(f"{role_name}: {content[:MAX_HISTORY_CHARS]}") + + formatted_history = "\n\n".join(formatted_parts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 85 - 91, The current loop builds formatted_history with no bounds, causing unbounded token growth; update the logic that constructs formatted_history (the loop over history and role_name assignment) to only include the most recent N messages (e.g., max_history_messages) and truncate each msg.get("content") to a safe length (e.g., max_chars_per_message) before concatenation; implement parameters like max_history_messages and max_chars_per_message (or a helper truncate_message(content, max_chars)) and use them when building formatted_history to ensure history and per-message length are limited.
🤖 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/app/core/config.py`:
- Line 14: 현재 env_file 경로가 Path(__file__).resolve().parents[3]로 프로젝트 루트를 가리키고 있어
README와 불일치하므로 backend 디렉토리를 가리키도록 parents[2]로 변경하세요; 해당 수정은
backend/app/core/config.py에서 env_file=str(Path(__file__).resolve().parents[2] /
".env")로 업데이트하여 Path(__file__) 기반 경로 계산을 올바르게 조정하면 됩니다.
In `@backend/pytest_log_utf8.txt`:
- Around line 1-26: Remove the checked-in pytest_log_utf8.txt from the
repository and stop committing test logs by deleting that file from git history
(unstage/remove it) and adding appropriate ignore rules to .gitignore; add
patterns like pytest_log*.txt, *.log, .pytest_cache/, and common build/test
artifacts (e.g. dist/, build/, .venv/) so test result files and logs are ignored
going forward and ensure the change is committed.
- Around line 10-14: Remove the module-level GEMINI_API_KEY check in
app/services/llm.py and instead validate/load the key at the start of the public
functions that need it (e.g., get_english_translation, get_response_stream,
get_response_stream_async), raising an informative error only when those
functions are actually invoked; add a test-wide fixture file
backend/tests/conftest.py that sets a safe GEMINI_API_KEY (or mocks access) for
test collection; and adjust app/core/config.py to allow optional/delayed loading
of GEMINI_API_KEY (provide a default or a getter that returns None during import
and validates only on use) so importing the module during test discovery no
longer raises.
In `@frontend/components/chat/ChatMain.tsx`:
- Line 47: The paragraph showing "세션 시작" is rendered even when mounted is false
causing a flash; change the rendering in the ChatMain component to conditionally
render the entire <p> element only when mounted is true (using mounted to gate
rendering of the paragraph that uses startTime) so nothing (not an empty label)
appears before mount completes.
In `@frontend/components/chat/MessageList.tsx`:
- Around line 74-76: The current dedupe in MessageList using book_info.title
collapses different documents that share a title; instead dedupe and iterate by
DocumentMetadata.id: build a unique list keyed by m.id from msg.metadata
(preserving book_info.title for display) and map over those unique ids, and when
selecting meta use the item with matching id (e.g., replace the Set over titles
and the find by title with a uniqueness step keyed on m.id and lookups by m.id)
so citations reference the correct DocumentMetadata.id rather than collapsing
distinct documents with identical titles.
---
Nitpick comments:
In `@backend/app/api/routes/chat.py`:
- Around line 4-19: Change ChatRequest.history to use a pydantic default_factory
instead of a shared mutable default; import Field from pydantic and update the
ChatRequest model so the history field is declared like history:
Optional[List[Dict[str, str]]] = Field(default_factory=list) (reference symbols:
ChatRequest, history, Field) to avoid using a mutable list as a default.
- Around line 85-91: The current loop builds formatted_history with no bounds,
causing unbounded token growth; update the logic that constructs
formatted_history (the loop over history and role_name assignment) to only
include the most recent N messages (e.g., max_history_messages) and truncate
each msg.get("content") to a safe length (e.g., max_chars_per_message) before
concatenation; implement parameters like max_history_messages and
max_chars_per_message (or a helper truncate_message(content, max_chars)) and use
them when building formatted_history to ensure history and per-message length
are limited.
In `@backend/app/core/rate_limit.py`:
- Around line 1-4: The current limiter uses slowapi.util.get_remote_address
which can return the proxy IP behind nginx/load balancers; implement a custom
key function (e.g., get_client_ip) that parses X-Forwarded-For (respecting a
trusted proxies list or first/last IP policy) and falls back to
get_remote_address, then instantiate limiter = Limiter(key_func=get_client_ip)
so rate limits use the real client IP.
In `@backend/pytest_log_utf8.txt`:
- Around line 16-21: 테스트 로그에 나타난 "google._upb._message.MessageMapContainer" 및
"google._upb._message.ScalarMapContainer" 관련 DeprecationWarning은 외부 protobuf
라이브러리의 Python 3.14 호환성 문제이므로, requirements/lock 파일에서 protobuf 패키지 버전을 최신으로
고정하고(또는 업그레이드), pytest 실행 시 불필요한 noise를 줄이려면 pytest.ini 또는 테스트 런너 설정에
DeprecationWarning 필터를 추가하여 해당 경고를 무시하도록 구성하며(필터 조건에 위 두 타입/모듈명 포함), CI 파이프라인에
"python -c 'import google.protobuf; print(google.protobuf.__version__)'" 검사 또는
의존성 스캔을 추가해 프로토콜 버전이 업그레이드되었는지 모니터링하도록 변경하세요.
In `@backend/requirements.txt`:
- Line 14: Move the test-only dependency pytest-asyncio out of
backend/requirements.txt into a new dev/test requirements file (e.g.,
requirements-dev.txt or requirements-test.txt): remove the line
"pytest-asyncio>=0.23.0" from backend/requirements.txt, add it to the new file
(keeping the same version constraint), and update any CI/test scripts or
developer setup docs to install the dev requirements (pip install -r
requirements-dev.txt) when running tests; ensure the project README or
contributing docs mention the new dev requirements file.
In `@frontend/app/page.tsx`:
- Around line 63-93: The review suggests improving readability by extracting the
inline SSE parser or memoizing it; move the processLine function out of the
component or wrap it with useCallback so it's not re-created on every render,
keeping its behavior identical (use the same function name processLine and
ensure it still accepts (line: string, eventObj: { current: string }) and
references aiMsgId and setMessages correctly), and if you move it outside the
component pass in necessary closures (aiMsgId, setMessages) as parameters or
memoize them to avoid stale values.
In `@frontend/components/chat/FloatingInput.tsx`:
- Around line 50-52: Update the onKeyDown handler in FloatingInput.tsx to also
check e.nativeEvent.isComposing in addition to the existing isComposing.current
ref, and keep the 50ms grace check (lastCompositionEndAt.current) applied only
for Enter key presses so other keys' behavior is unchanged; specifically, inside
the onKeyDown callback, return early if e.nativeEvent.isComposing ||
isComposing.current, and for the Enter-key branch also ensure Date.now() -
lastCompositionEndAt.current < 50 prevents handling within the grace window.
In `@frontend/components/chat/MessageList.tsx`:
- Around line 78-104: The card UI uses cursor-pointer and group/card hover
styles but only the icon button triggers the action, causing a mismatch; make
the entire card clickable by moving the onClick handler from the button to the
outer div (the element with class "group/card" that renders the card) and ensure
onOpenCitation(meta) is called when that div is clicked (retain the button for
accessibility but remove its onClick or keep it as duplicate activation), and
preserve aria-label on the button and focus-visible styles so keyboard and
screen-reader users still have an accessible control; update any conditional
rendering around onOpenCitation so the card click is only attached when
onOpenCitation is defined.
In `@frontend/components/sidebar/ContextSources.tsx`:
- Around line 24-27: Currently the list item key uses book_info.title which can
be an empty string or duplicate; change the key to a stable unique identifier
such as meta.id (DocumentMetadata.id) to avoid React key collisions—update the
<li key=...> for the map over uniqueBooks to use meta.id instead of
meta.book_info.title and ensure meta.id exists (fallback to a concatenation like
`${meta.id}-${meta.book_info.title}` if necessary).
In `@frontend/components/sidebar/Sidebar.tsx`:
- Around line 47-55: In the Sidebar component, remove the redundant
aria-disabled="true" and tabIndex={-1} attributes from the disabled buttons (the
ones rendering Settings, History and User icons) because the native disabled
attribute already removes them from the tab order and exposes the disabled state
to assistive tech; keep the disabled prop, aria-label and title as-is and ensure
other classes (e.g., cursor-not-allowed, focus-visible:outline-none) remain if
styling/visual cues are desired.
- Line 29: The inline console.log in the ActivePhilosophers usage is leftover
debug code; replace it with a real handler or a clear TODO and remove the
console.log. Update the onPhilosopherClick prop on ActivePhilosophers (where
currentMetadata is passed) to either call the real click handler (e.g., a named
function like handlePhilosopherClick) or, if the feature is not implemented yet,
replace the console.log with a TODO comment such as “// TODO: implement
handlePhilosopherClick” and pass a noop or forward the event to the parent prop
instead of logging.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
README.mdbackend/app/api/routes/chat.pybackend/app/core/config.pybackend/app/core/rate_limit.pybackend/app/main.pybackend/app/services/database.pybackend/app/services/embedding.pybackend/app/services/llm.pybackend/pytest_log.txtbackend/pytest_log_utf8.txtbackend/requirements.txtbackend/tests/integration/test_chat.pybackend/tests/unit/test_chat_models.pybackend/tests/unit/test_llm.pyfrontend/.stylelintrc.jsonfrontend/app/layout.tsxfrontend/app/page.tsxfrontend/components/chat/ChatMain.tsxfrontend/components/chat/FloatingInput.tsxfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/ActivePhilosophers.tsxfrontend/components/sidebar/ContextSources.tsxfrontend/components/sidebar/Sidebar.tsxfrontend/next.config.ts
Comment on lines
+1
to
+26
| ============================= test session starts ============================= | ||
| platform win32 -- Python 3.12.12, pytest-9.0.2, pluggy-1.6.0 | ||
| rootdir: C:\Users\ysn65\Desktop\antigravity\philo-rag\backend | ||
| plugins: anyio-4.12.1, asyncio-1.3.0, cov-7.0.0 | ||
| asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function | ||
| collected 0 items / 1 error | ||
|
|
||
| =================================== ERRORS ==================================== | ||
| ___________________ ERROR collecting tests/unit/test_llm.py ___________________ | ||
| tests\unit\test_llm.py:12: in <module> | ||
| from app.services.llm import get_english_translation, get_response_stream, get_response_stream_async | ||
| app\services\llm.py:8: in <module> | ||
| raise RuntimeError("GEMINI_API_KEY must be configured") | ||
| E RuntimeError: GEMINI_API_KEY must be configured | ||
| ============================== warnings summary =============================== | ||
| <frozen importlib._bootstrap>:488 | ||
| <frozen importlib._bootstrap>:488: DeprecationWarning: Type google._upb._message.MessageMapContainer uses PyType_Spec with a metaclass that has custom tp_new. This is deprecated and will no longer be allowed in Python 3.14. | ||
|
|
||
| <frozen importlib._bootstrap>:488 | ||
| <frozen importlib._bootstrap>:488: DeprecationWarning: Type google._upb._message.ScalarMapContainer uses PyType_Spec with a metaclass that has custom tp_new. This is deprecated and will no longer be allowed in Python 3.14. | ||
|
|
||
| -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html | ||
| =========================== short test summary info =========================== | ||
| ERROR tests/unit/test_llm.py - RuntimeError: GEMINI_API_KEY must be configured | ||
| !!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!! | ||
| ======================== 2 warnings, 1 error in 4.52s ========================= |
There was a problem hiding this comment.
로그 파일은 버전 관리에서 제외해야 합니다.
이 pytest 로그 파일은 버전 관리 시스템에 포함되어서는 안 됩니다. 로그 파일, 테스트 결과, 빌드 아티팩트는 다음과 같은 이유로 .gitignore에 추가되어야 합니다:
- 저장소 크기를 불필요하게 증가시킵니다
- 매 실행마다 변경되어 merge conflict의 원인이 됩니다
- 로컬 환경에 특화된 정보를 포함할 수 있습니다
🗑️ 권장 조치
- 이 파일을 저장소에서 제거하세요:
git rm backend/pytest_log_utf8.txt.gitignore에 다음 패턴을 추가하세요:
+# Test logs
+**/pytest_log*.txt
+**/*_log*.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/pytest_log_utf8.txt` around lines 1 - 26, Remove the checked-in
pytest_log_utf8.txt from the repository and stop committing test logs by
deleting that file from git history (unstage/remove it) and adding appropriate
ignore rules to .gitignore; add patterns like pytest_log*.txt, *.log,
.pytest_cache/, and common build/test artifacts (e.g. dist/, build/, .venv/) so
test result files and logs are ignored going forward and ensure the change is
committed.
| <div> | ||
| <h2 className="font-display text-3xl text-white/90">미덕에 관한 대화</h2> | ||
| <p className="text-sm text-white/40 mt-1">세션 시작: {new Date().toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' })}</p> | ||
| <p className="text-sm text-white/40 mt-1">세션 시작: {mounted ? startTime : ""}</p> |
There was a problem hiding this comment.
초기 렌더에서 불완전한 라벨이 잠깐 노출됩니다.
Line 47에서 mounted가 false일 때 빈 문자열만 대입되어 "세션 시작:" 텍스트가 먼저 보입니다. 문단 자체를 조건부 렌더링하면 초기 깜빡임을 줄일 수 있습니다.
수정 제안 diff
- <p className="text-sm text-white/40 mt-1">세션 시작: {mounted ? startTime : ""}</p>
+ {mounted && startTime && (
+ <p className="text-sm text-white/40 mt-1">세션 시작: {startTime}</p>
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="text-sm text-white/40 mt-1">세션 시작: {mounted ? startTime : ""}</p> | |
| {mounted && startTime && ( | |
| <p className="text-sm text-white/40 mt-1">세션 시작: {startTime}</p> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/components/chat/ChatMain.tsx` at line 47, The paragraph showing "세션
시작" is rendered even when mounted is false causing a flash; change the rendering
in the ChatMain component to conditionally render the entire <p> element only
when mounted is true (using mounted to gate rendering of the paragraph that uses
startTime) so nothing (not an empty label) appears before mount completes.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/unit/test_llm.py (1)
89-93:⚠️ Potential issue | 🟡 Minor수동 실행 시
TypeError발생 가능.
test_translation,test_streaming,test_streaming_async함수들은setup_test_env파라미터를 필요로 하지만,__main__블록에서 인자 없이 호출하고 있습니다. pytest로 실행할 때는 fixture 주입이 자동으로 되지만, 직접 실행 시에는TypeError: missing 1 required positional argument에러가 발생합니다.🔧 수정 제안
# For manual execution if __name__ == "__main__": import asyncio - test_translation() - test_streaming() - asyncio.run(test_streaming_async()) + test_translation(None) + test_streaming(None) + asyncio.run(test_streaming_async(None))또는 수동 실행 블록을 제거하고 pytest로만 테스트를 실행하는 것을 권장합니다:
-# For manual execution -if __name__ == "__main__": - import asyncio - test_translation() - test_streaming() - asyncio.run(test_streaming_async())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_llm.py` around lines 89 - 93, The manual __main__ block calls test_translation, test_streaming, and test_streaming_async without the required pytest fixture argument setup_test_env, causing a TypeError when run directly; either remove the entire if __name__ == "__main__": block (prefer running tests via pytest so fixtures are injected), or change the block to invoke the tests through pytest.main([...]) or provide a proper setup_test_env object and call the functions with that argument (reference the test functions test_translation, test_streaming, test_streaming_async and the setup_test_env fixture when editing).
🧹 Nitpick comments (9)
frontend/components/sidebar/ContextSources.tsx (1)
10-12: 중복 제거 로직을 O(n)으로 단순화하는 것을 권장합니다.Line 10~Line 11은
id별로 다시find를 호출해서 데이터가 커질 때 비용이 커집니다. 현재 동작(첫 항목 유지)은 유지하면서 1회 순회로 줄일 수 있습니다.제안 코드
- const uniqueBooks = Array.from(new Set(metadata.map(m => m.id))) - .map(id => metadata.find(m => m.id === id)) - .filter((m): m is DocumentMetadata => m !== undefined); + const uniqueBooks = Array.from( + metadata.reduce((acc, m) => { + if (!acc.has(m.id)) acc.set(m.id, m); + return acc; + }, new Map<DocumentMetadata["id"], DocumentMetadata>()).values() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/sidebar/ContextSources.tsx` around lines 10 - 12, The duplicate-removal currently building uniqueBooks by collecting ids then calling metadata.find for each causes O(n^2) work; replace that with a single-pass dedupe over metadata (keeping the first occurrence) by iterating metadata once and using a Map or plain object keyed by m.id to store the first DocumentMetadata seen, then take its values to produce uniqueBooks; update the uniqueBooks variable (and keep the type DocumentMetadata) and ensure any downstream consumers still receive the same ordering/first-item semantics.frontend/components/sidebar/Sidebar.tsx (1)
29-29: TODO 구현 시scholar파라미터 수신 필요현재 no-op 핸들러는 파라미터를 받지 않지만,
ActivePhilosophers.tsx의 관련 코드를 보면onPhilosopherClick?.(meta.scholar)로scholar인자를 전달하고 있습니다. 향후 철학자 필터 기능 구현 시 해당 파라미터를 활용해야 합니다.-<ActivePhilosophers metadata={currentMetadata} onPhilosopherClick={() => { /* TODO: Implement philosopher filter */ }} /> +<ActivePhilosophers metadata={currentMetadata} onPhilosopherClick={(scholar) => { /* TODO: Implement philosopher filter using scholar */ }} />🤖 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 29, The on-click handler passed to ActivePhilosophers in Sidebar.tsx must accept the scholar parameter because ActivePhilosophers calls onPhilosopherClick?.(meta.scholar); update the inline handler in Sidebar.tsx from () => { /* TODO */ } to a function that takes a scholar argument (e.g., scholar: string | ScholarType) and forwards it to your philosopher filter logic or state updater (or at minimum stubs call like setPhilosopherFilter(scholar)). Refer to the ActivePhilosophers prop name onPhilosopherClick and the Sidebar component where currentMetadata is used to wire the parameter through.frontend/components/chat/MessageList.tsx (2)
79-79: 클릭 가능한 div에 키보드 접근성이 누락되었습니다.
cursor-pointer와onClick이 있지만role,tabIndex,onKeyDown속성이 없어 키보드 전용 사용자가 이 카드에 접근할 수 없습니다.♿ 접근성 개선 제안
- <div key={meta.id} onClick={() => onOpenCitation?.(meta)} className="mt-8 flex gap-4 p-4 rounded-xl bg-white/5 border border-white/10 max-w-xl hover:border-primary/30 transition-colors cursor-pointer group/card"> + <div + key={meta.id} + role="button" + tabIndex={onOpenCitation ? 0 : undefined} + onClick={() => onOpenCitation?.(meta)} + onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onOpenCitation?.(meta); } }} + className="mt-8 flex gap-4 p-4 rounded-xl bg-white/5 border border-white/10 max-w-xl hover:border-primary/30 transition-colors cursor-pointer group/card" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/chat/MessageList.tsx` at line 79, The clickable citation card rendered in MessageList.tsx (the div with key={meta.id} and onClick={() => onOpenCitation?.(meta)}) lacks keyboard accessibility; update that element to be operable via keyboard by adding role="button", tabIndex={0}, and an onKeyDown handler that listens for Enter and Space and calls onOpenCitation(meta) (same handler used by onClick); ensure focus styles remain visible and keep the existing onClick intact so both mouse and keyboard invoke the same action.
74-77:Map을 사용하면 O(n) 복잡도로 개선할 수 있습니다.현재
Set+find()조합은 O(n²) 복잡도를 가집니다. 메타데이터 배열이 작으므로 실제 성능 영향은 미미하지만,Map을 사용하면 한 번의 순회로 중복 제거와 객체 참조를 동시에 처리할 수 있습니다.♻️ Map 기반 개선 제안
- {msg.metadata && msg.metadata.length > 0 && Array.from(new Set(msg.metadata.map(m => m.id))).map((id) => { - const meta = msg.metadata?.find(m => m.id === id); - if (!meta) return null; - const title = meta.book_info.title; + {msg.metadata && msg.metadata.length > 0 && + Array.from(new Map(msg.metadata.map((m) => [m.id, m])).values()).map((meta) => { + const title = meta.book_info.title;🤖 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 74 - 77, The current rendering logic in MessageList.tsx builds a Set of metadata ids then uses find() per id, causing O(n²) behavior; change it to a single-pass Map: iterate msg.metadata once to build a Map keyed by m.id (or use Array.prototype.reduce) to deduplicate and store the metadata objects, then iterate over map.values() to render entries (use the same symbols msg.metadata and title = meta.book_info.title to locate where to replace the Set+find logic). This reduces complexity to O(n) and preserves the existing render output.backend/app/api/routes/chat.py (2)
17-19: history 필드에 대한 유효성 검사 강화를 고려하세요.현재
List[Dict[str, str]]타입은 어떤 키-값 쌍이든 허용합니다.role과content필드만 허용하는 명시적인 모델을 사용하면 잘못된 데이터를 조기에 거부할 수 있습니다.♻️ 수정 제안
+from typing import Literal + +class HistoryMessage(BaseModel): + role: Literal["user", "ai"] + content: str + class ChatRequest(BaseModel): query: str - history: List[Dict[str, str]] = Field(default_factory=list) + history: List[HistoryMessage] = Field(default_factory=list)그리고
generate_chat_events에서 사용 시:- role_name = "User" if msg.get("role") == "user" else "Agent (PhiloRAG)" - content = (msg.get("content") or "").strip() + role_name = "User" if msg.role == "user" else "Agent (PhiloRAG)" + content = msg.content.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 17 - 19, 현재 ChatRequest의 history가 List[Dict[str, str]]로 되어 있어 임의 키가 들어올 수 있으니, role과 content만 허용하는 명시적 Pydantic 모델(예: ChatMessage)으로 대체하여 ChatRequest.history: List[ChatMessage]로 변경하고, ChatMessage에 role(str)과 content(str) 필수 필드를 선언해 유효성 검사를 강제하세요; 또한 generate_chat_events 함수에서 history를 순회하거나 접근할 때는 새 모델 속성명(role, content)을 사용하도록 코드(입력 처리 및 타입 주석)를 업데이트하세요.
90-95: 예상치 못한 role 값에 대한 방어 코드를 추가하세요.현재 코드는
role이 "user"가 아니면 모두 "Agent (PhiloRAG)"로 처리합니다. 유효하지 않은 role 값(예: 악의적인 입력)이 들어올 경우 예상치 못한 동작이 발생할 수 있습니다.🛡️ 방어적 코드 제안
for msg in history_tail: - role_name = "User" if msg.get("role") == "user" else "Agent (PhiloRAG)" + role = msg.get("role") + if role == "user": + role_name = "User" + elif role == "ai": + role_name = "Agent (PhiloRAG)" + else: + continue # 유효하지 않은 role은 건너뜀 content = (msg.get("content") or "").strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 90 - 95, The loop over history_tail assumes any non-"user" role is "Agent (PhiloRAG)"; add defensive role validation in that loop (referencing history_tail, msg, role_name, formatted_parts, MAX_HISTORY_CHARS) by first normalizing role = str(msg.get("role") or "").lower(), then handle only known roles (e.g., "user" -> "User", "agent" or "philo" -> "Agent (PhiloRAG)", optionally "system" -> "System"); for any other/invalid role either skip the message (continue) or set role_name to "Unknown" before appending, ensuring you still check content and slice by MAX_HISTORY_CHARS.frontend/app/page.tsx (1)
12-42:processLine의setMessages파라미터가 불필요합니다.
setMessages는 이미 컴포넌트 스코프에서 접근 가능하며,useCallback의 의존성 배열이 비어있어 안정적인 참조입니다. 파라미터로 전달할 필요가 없습니다.♻️ 수정 제안
- const processLine = useCallback((line: string, eventObj: { current: string }, aiMsgId: string, setMessages: React.Dispatch<React.SetStateAction<Message[]>>): boolean => { + const processLine = useCallback((line: string, eventObj: { current: string }, aiMsgId: string): boolean => { if (line.startsWith("event: ")) { eventObj.current = line.substring(7).trim(); } else if (line.startsWith("data: ")) { // ... 나머지 로직 동일 } return false; }, []);그리고 호출부도 함께 수정:
- if (processLine(line, eventObj, aiMsgId, setMessages)) { + if (processLine(line, eventObj, aiMsgId)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/page.tsx` around lines 12 - 42, The processLine function currently accepts a redundant setMessages parameter; remove setMessages from the processLine signature and body so it uses the component-scoped setMessages directly, then update every call site to stop passing setMessages (only pass line, eventObj, aiMsgId now). Ensure the useCallback wrapper for processLine still has the correct dependency list (include any external values it uses, or keep it empty if only using stable refs/state setters) and update references to processLine accordingly.backend/tests/conftest.py (1)
13-20: docstring과 실제 scope이 불일치합니다.docstring에는 "for each test case"라고 되어 있지만, 실제 fixture는
scope="session"으로 세션 전체에 걸쳐 하나의 event loop를 사용합니다. docstring을 수정하여 명확하게 해주세요.📝 수정 제안
`@pytest.fixture`(scope="session") def event_loop(): - """Create an instance of the default event loop for each test case. + """Create a single event loop instance shared across the entire test session. Scoped to session to prevent global client implementations (e.g. Supabase Python client) from binding `asyncio.locks.Event` instances to closed function-scoped event loops."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 13 - 20, Docstring for the pytest fixture event_loop is misleading (it says "for each test case") while the fixture uses scope="session"; update the docstring in the event_loop fixture to state that a single event loop is created for the entire test session (and note why session scope is used), keeping the existing behavior (loop = asyncio.get_event_loop_policy().new_event_loop(); yield loop; loop.close()) and ensuring the docstring references the session-wide scope and reason for using session scope to avoid binding asyncio.locks.Event to closed function-scoped loops.backend/tests/e2e/test_chat_endpoint.py (1)
30-33: 사용하지 않는args,kwargs파라미터.static analysis에서 지적한 대로
mock_async_generator의args,kwargs가 사용되지 않습니다. mock 호환성을 위한 것으로 보이며,backend/tests/integration/test_supabase_match.py의 동일한 패턴과 일관성이 있습니다. 선택적으로_prefix를 사용하여 의도를 명확히 할 수 있습니다.♻️ 선택적 개선
# Mock LLM stream generator - async def mock_async_generator(*args, **kwargs): + async def mock_async_generator(*_args, **_kwargs): for chunk in ["인생은", " ", "고통입니다."]: yield chunk🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/test_chat_endpoint.py` around lines 30 - 33, 불사용된 매개변수 때문에 정적분석 경고가 발생하므로 mock_async_generator의 시그니처에서 사용 의도가 분명하도록 args/kwargs를 _args, _kwargs 또는 단순히 _와 같은 언더스코어 접두사로 변경하세요; 이를 변경하면 mock_stream.return_value에 할당된 mock_async_generator() 호출은 동일하게 유지되며 backend/tests/integration/test_supabase_match.py의 동일 패턴과 일관성을 유지합니다.
🤖 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/app/services/llm.py`:
- Around line 10-26: get_llm() is not thread-safe: concurrent calls can race and
reinitialize _llm and re-call genai.configure(); fix by adding a module-level
lock (e.g., threading.Lock) and using it around the initialization path in
get_llm (use double-checked locking: check if _llm is None, acquire the lock,
check again, then call genai.configure(...) and construct
ChatGoogleGenerativeAI(...)), and ensure settings.GEMINI_API_KEY validation
remains before acquiring the lock so errors are raised early.
- Around line 20-21: The code currently constructs the LLM with
ChatGoogleGenerativeAI using model="gemini-2.5-flash" which is scheduled for
deprecation; update the model argument on the _llm instantiation to a currently
supported successor (e.g., "gemini-3-flash") and, in the same place where _llm
is created, add a short comment or TODO referencing the deprecation date so
future maintainers can plan migration; locate the instantiation of
ChatGoogleGenerativeAI (the _llm variable) and replace the model string
accordingly.
In `@backend/tests/conftest.py`:
- Around line 2-4: Remove the duplicate pytest import in
backend/tests/conftest.py: the top imports currently list "import pytest",
"import asyncio", "import pytest" — remove the second "import pytest" so only a
single import of pytest remains (keep "import pytest" and "import asyncio") to
avoid redundant imports and lint warnings.
In `@backend/tests/integration/test_chat.py`:
- Line 4: Remove the unused import ChatRequest from the top of test_chat.py:
locate the import statement "from app.api.routes.chat import ChatRequest" and
delete it (or alternatively use ChatRequest in the test if it was intended),
ensuring no other references to ChatRequest remain in the file so the test
module has no unused imports.
---
Outside diff comments:
In `@backend/tests/unit/test_llm.py`:
- Around line 89-93: The manual __main__ block calls test_translation,
test_streaming, and test_streaming_async without the required pytest fixture
argument setup_test_env, causing a TypeError when run directly; either remove
the entire if __name__ == "__main__": block (prefer running tests via pytest so
fixtures are injected), or change the block to invoke the tests through
pytest.main([...]) or provide a proper setup_test_env object and call the
functions with that argument (reference the test functions test_translation,
test_streaming, test_streaming_async and the setup_test_env fixture when
editing).
---
Nitpick comments:
In `@backend/app/api/routes/chat.py`:
- Around line 17-19: 현재 ChatRequest의 history가 List[Dict[str, str]]로 되어 있어 임의 키가
들어올 수 있으니, role과 content만 허용하는 명시적 Pydantic 모델(예: ChatMessage)으로 대체하여
ChatRequest.history: List[ChatMessage]로 변경하고, ChatMessage에 role(str)과
content(str) 필수 필드를 선언해 유효성 검사를 강제하세요; 또한 generate_chat_events 함수에서 history를
순회하거나 접근할 때는 새 모델 속성명(role, content)을 사용하도록 코드(입력 처리 및 타입 주석)를 업데이트하세요.
- Around line 90-95: The loop over history_tail assumes any non-"user" role is
"Agent (PhiloRAG)"; add defensive role validation in that loop (referencing
history_tail, msg, role_name, formatted_parts, MAX_HISTORY_CHARS) by first
normalizing role = str(msg.get("role") or "").lower(), then handle only known
roles (e.g., "user" -> "User", "agent" or "philo" -> "Agent (PhiloRAG)",
optionally "system" -> "System"); for any other/invalid role either skip the
message (continue) or set role_name to "Unknown" before appending, ensuring you
still check content and slice by MAX_HISTORY_CHARS.
In `@backend/tests/conftest.py`:
- Around line 13-20: Docstring for the pytest fixture event_loop is misleading
(it says "for each test case") while the fixture uses scope="session"; update
the docstring in the event_loop fixture to state that a single event loop is
created for the entire test session (and note why session scope is used),
keeping the existing behavior (loop =
asyncio.get_event_loop_policy().new_event_loop(); yield loop; loop.close()) and
ensuring the docstring references the session-wide scope and reason for using
session scope to avoid binding asyncio.locks.Event to closed function-scoped
loops.
In `@backend/tests/e2e/test_chat_endpoint.py`:
- Around line 30-33: 불사용된 매개변수 때문에 정적분석 경고가 발생하므로 mock_async_generator의 시그니처에서
사용 의도가 분명하도록 args/kwargs를 _args, _kwargs 또는 단순히 _와 같은 언더스코어 접두사로 변경하세요; 이를 변경하면
mock_stream.return_value에 할당된 mock_async_generator() 호출은 동일하게 유지되며
backend/tests/integration/test_supabase_match.py의 동일 패턴과 일관성을 유지합니다.
In `@frontend/app/page.tsx`:
- Around line 12-42: The processLine function currently accepts a redundant
setMessages parameter; remove setMessages from the processLine signature and
body so it uses the component-scoped setMessages directly, then update every
call site to stop passing setMessages (only pass line, eventObj, aiMsgId now).
Ensure the useCallback wrapper for processLine still has the correct dependency
list (include any external values it uses, or keep it empty if only using stable
refs/state setters) and update references to processLine accordingly.
In `@frontend/components/chat/MessageList.tsx`:
- Line 79: The clickable citation card rendered in MessageList.tsx (the div with
key={meta.id} and onClick={() => onOpenCitation?.(meta)}) lacks keyboard
accessibility; update that element to be operable via keyboard by adding
role="button", tabIndex={0}, and an onKeyDown handler that listens for Enter and
Space and calls onOpenCitation(meta) (same handler used by onClick); ensure
focus styles remain visible and keep the existing onClick intact so both mouse
and keyboard invoke the same action.
- Around line 74-77: The current rendering logic in MessageList.tsx builds a Set
of metadata ids then uses find() per id, causing O(n²) behavior; change it to a
single-pass Map: iterate msg.metadata once to build a Map keyed by m.id (or use
Array.prototype.reduce) to deduplicate and store the metadata objects, then
iterate over map.values() to render entries (use the same symbols msg.metadata
and title = meta.book_info.title to locate where to replace the Set+find logic).
This reduces complexity to O(n) and preserves the existing render output.
In `@frontend/components/sidebar/ContextSources.tsx`:
- Around line 10-12: The duplicate-removal currently building uniqueBooks by
collecting ids then calling metadata.find for each causes O(n^2) work; replace
that with a single-pass dedupe over metadata (keeping the first occurrence) by
iterating metadata once and using a Map or plain object keyed by m.id to store
the first DocumentMetadata seen, then take its values to produce uniqueBooks;
update the uniqueBooks variable (and keep the type DocumentMetadata) and ensure
any downstream consumers still receive the same ordering/first-item semantics.
In `@frontend/components/sidebar/Sidebar.tsx`:
- Line 29: The on-click handler passed to ActivePhilosophers in Sidebar.tsx must
accept the scholar parameter because ActivePhilosophers calls
onPhilosopherClick?.(meta.scholar); update the inline handler in Sidebar.tsx
from () => { /* TODO */ } to a function that takes a scholar argument (e.g.,
scholar: string | ScholarType) and forwards it to your philosopher filter logic
or state updater (or at minimum stubs call like setPhilosopherFilter(scholar)).
Refer to the ActivePhilosophers prop name onPhilosopherClick and the Sidebar
component where currentMetadata is used to wire the parameter through.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitignorebackend/app/api/routes/chat.pybackend/app/core/rate_limit.pybackend/app/services/llm.pybackend/pytest_log_utf8.txtbackend/requirements-dev.txtbackend/requirements.txtbackend/tests/conftest.pybackend/tests/e2e/test_chat_endpoint.pybackend/tests/integration/test_chat.pybackend/tests/integration/test_supabase_match.pybackend/tests/unit/test_llm.pyfrontend/app/page.tsxfrontend/components/chat/FloatingInput.tsxfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/ContextSources.tsxfrontend/components/sidebar/Sidebar.tsx
💤 Files with no reviewable changes (1)
- backend/pytest_log_utf8.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/components/chat/FloatingInput.tsx
- backend/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/tests/unit/test_llm.py (1)
89-93: pytest fixture가 직접 호출로 인해 우회되고 있습니다.
test_translation(),test_streaming(),test_streaming_async()함수는 모두setup_test_envfixture 매개변수를 기대하지만,__main__블록에서는None을 전달하고 있습니다.@pytest.fixture(autouse=True)로 정의된 fixture가 제대로 초기화되지 않습니다.수동 실행이 필요한 경우
pytest.main()을 사용하는 것이 안전합니다.🔧 제안 수정
if __name__ == "__main__": - import asyncio - test_translation(None) - test_streaming(None) - asyncio.run(test_streaming_async(None)) + import pytest + raise SystemExit(pytest.main([__file__]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_llm.py` around lines 89 - 93, The test __main__ block is bypassing the pytest fixture setup by calling test_translation, test_streaming, and test_streaming_async with None (fixture setup_test_env is not run); replace the manual calls in the if __name__ == "__main__": block with a call to pytest.main() (or remove the block entirely) so pytest executes the tests and injects the setup_test_env fixture properly, ensuring test_translation, test_streaming, and test_streaming_async receive the fixture rather than None.
🤖 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/tests/integration/test_chat.py`:
- Around line 11-13: mock_generator currently declares unused parameters causing
Ruff(ARG001/ARG005) warnings; change its signature to accept and ignore them
(e.g., async def mock_generator(*_args, **_kwargs): yield "data: ok\n\n") and
update the side effect lambda to match (mock_events.side_effect = lambda *_args,
**_kwargs: mock_generator()) so tests behave the same while removing
unused-argument warnings for mock_generator and the lambda.
In `@frontend/app/page.tsx`:
- Around line 15-33: SSE data lines may contain literal carriage returns (\r)
from CRLF boundaries; normalize currentData by removing or replacing \r before
further processing so metadata JSON parsing and content accumulation are not
polluted. Update the handling around currentData (the value assigned from
line.substring(6)) and prior to JSON.parse in the "metadata" branch and prior to
the newline un-escape in the "content" branch (the code referencing currentData,
eventObj.current, setMessages, aiMsgId) to strip \r (e.g., replace all '\r'
chars) so metadata parsing and message.content concatenation receive clean text.
In `@frontend/components/chat/MessageList.tsx`:
- Around line 77-84: The div rendering each citation card currently always has
role="button", tabIndex, onClick/onKeyDown handlers and the cursor-pointer class
even when onOpenCitation is undefined; change the JSX to only add interactive
attributes and styles when onOpenCitation is present: conditionally add
role="button" and tabIndex={0}, attach onClick and onKeyDown handlers (calling
onOpenCitation(meta)), and include the "cursor-pointer" class only if
onOpenCitation exists; ensure the key (meta.id) and other visual classes remain
unchanged and that key symbols to update are the props/attributes around the div
that reference onOpenCitation, role, tabIndex, onClick, onKeyDown, and the class
list.
---
Nitpick comments:
In `@backend/tests/unit/test_llm.py`:
- Around line 89-93: The test __main__ block is bypassing the pytest fixture
setup by calling test_translation, test_streaming, and test_streaming_async with
None (fixture setup_test_env is not run); replace the manual calls in the if
__name__ == "__main__": block with a call to pytest.main() (or remove the block
entirely) so pytest executes the tests and injects the setup_test_env fixture
properly, ensuring test_translation, test_streaming, and test_streaming_async
receive the fixture rather than None.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/app/api/routes/chat.pybackend/app/services/llm.pybackend/tests/conftest.pybackend/tests/e2e/test_chat_endpoint.pybackend/tests/integration/test_chat.pybackend/tests/unit/test_llm.pyfrontend/app/page.tsxfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/ContextSources.tsxfrontend/components/sidebar/Sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/conftest.py
- frontend/components/sidebar/ContextSources.tsx
- frontend/components/sidebar/Sidebar.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/tests/integration/test_chat.py (1)
1-4: 사용되지 않는pytestimport를 제거하세요.
pytest가 import 되었지만 이 테스트에서 사용되지 않습니다. fixture나 marker가 없으므로 제거할 수 있습니다.♻️ 제안된 수정
from fastapi.testclient import TestClient -import pytest from app.main import app from unittest.mock import patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/integration/test_chat.py` around lines 1 - 4, Remove the unused pytest import from the test module by deleting the line that imports pytest (the import statement at the top that reads "import pytest") since no fixtures or markers are used; keep the existing imports like TestClient, app, and patch intact to avoid affecting tests that rely on those symbols.backend/tests/unit/test_llm.py (2)
37-38:_mock_llm이 생성되지만 실제로 사용되지 않습니다.
_mock_llm이mock_get_llm.return_value로 설정되지만, 테스트에서 실제로 이 mock 객체의 메서드나 속성을 검증하거나 구성하지 않습니다. 현재로서는 동작하지만, 실제 LLM 호출이 올바르게 전달되는지 검증하려면_mock_llm에 대한 assertion을 추가하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_llm.py` around lines 37 - 38, The test sets _mock_llm as mock_get_llm.return_value but never asserts its usage; update the test to either configure _mock_llm (e.g., set a return_value for the LLM method used in the code under test) and assert it was called, or add assertions like _mock_llm.generate.assert_called_once_with(...) or _mock_llm.__call__.assert_called_with(...) (depending on which LLM method your code invokes) to verify the mock was invoked with the expected prompt/parameters; reference mock_get_llm.return_value and _mock_llm when making these changes.
30-46:setup_test_envfixture가autouse=True로 설정되어 있어 인자로 전달할 필요가 없습니다.
setup_test_envfixture는autouse=True로 설정되어 있으므로 자동으로 실행됩니다. 함수 인자로 명시적으로 전달하는 것은 불필요하며, 이 패턴이test_streaming과test_streaming_async에도 반복됩니다.♻️ 제안된 수정
-def test_translation(setup_test_env): +def test_translation(): print("Testing translation...")마찬가지로
test_streaming과test_streaming_async에서도setup_test_env인자를 제거하세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_llm.py` around lines 30 - 46, Remove the unnecessary explicit fixture parameter setup_test_env from the test functions (e.g., test_translation, test_streaming, test_streaming_async) since the fixture is declared with autouse=True; update each function signature to take no setup_test_env argument and leave the rest of the test body intact so the autouse fixture still runs automatically (locate these tests by their function names in the file to make the changes).frontend/app/page.tsx (1)
26-26: 빈catch블록에서 에러를 무시하지 마세요.metadata JSON 파싱 실패 시
console.error만 호출하고 있습니다. 이는 괜찮지만, 디버깅을 위해 실제 에러 객체도 함께 로깅하는 것이 좋습니다.♻️ 제안된 수정
- } catch { console.error("Could not parse metadata event:", currentData) } + } catch (e) { console.error("Could not parse metadata event:", currentData, e) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/page.tsx` at line 26, The catch block that currently swallows the parsing error should be changed to capture the exception and include it in the log: update the empty catch to catch (err) and call console.error with both the problematic variable (currentData) and the caught error so you get the error object alongside the "Could not parse metadata event:" message; locate the try/catch around the metadata JSON parse in page.tsx to modify that catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/integration/test_chat.py`:
- Around line 1-4: Remove the unused pytest import from the test module by
deleting the line that imports pytest (the import statement at the top that
reads "import pytest") since no fixtures or markers are used; keep the existing
imports like TestClient, app, and patch intact to avoid affecting tests that
rely on those symbols.
In `@backend/tests/unit/test_llm.py`:
- Around line 37-38: The test sets _mock_llm as mock_get_llm.return_value but
never asserts its usage; update the test to either configure _mock_llm (e.g.,
set a return_value for the LLM method used in the code under test) and assert it
was called, or add assertions like
_mock_llm.generate.assert_called_once_with(...) or
_mock_llm.__call__.assert_called_with(...) (depending on which LLM method your
code invokes) to verify the mock was invoked with the expected
prompt/parameters; reference mock_get_llm.return_value and _mock_llm when making
these changes.
- Around line 30-46: Remove the unnecessary explicit fixture parameter
setup_test_env from the test functions (e.g., test_translation, test_streaming,
test_streaming_async) since the fixture is declared with autouse=True; update
each function signature to take no setup_test_env argument and leave the rest of
the test body intact so the autouse fixture still runs automatically (locate
these tests by their function names in the file to make the changes).
In `@frontend/app/page.tsx`:
- Line 26: The catch block that currently swallows the parsing error should be
changed to capture the exception and include it in the log: update the empty
catch to catch (err) and call console.error with both the problematic variable
(currentData) and the caught error so you get the error object alongside the
"Could not parse metadata event:" message; locate the try/catch around the
metadata JSON parse in page.tsx to modify that catch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/integration/test_chat.pybackend/tests/unit/test_llm.pyfrontend/app/page.tsxfrontend/components/chat/MessageList.tsx
This was referenced Mar 2, 2026
Merged
Merged
Merged
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
릴리스 노트
새 기능
버그 수정
개선 사항
테스트