Merged
Conversation
…and observer cleanup
…-deps, and update docs
…ent thread pool hang
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough요약Walkthrough이 PR은 번역 및 임베딩 호출을 동기 방식에서 비동기 방식으로 변환하고, 타임아웃 처리를 추가하며, 앱 시작 시 모델을 미리 로드하는 라이프사이클 관리자를 도입합니다. 테스트도 새로운 비동기 패턴에 맞게 업데이트됩니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 2
🧹 Nitpick comments (2)
frontend/components/sidebar/ActivePhilosophers.tsx (1)
46-46: 그라디언트 오버레이가 항상 보이지 않습니다.Line 46에서
opacity-0이 고정되어 있어 오버레이가 항상 투명합니다. 지금 상태에선isActive에 따른 그라디언트 차이가 실제 UI에 반영되지 않습니다. 활성 상태에서 보이게 하거나, 의도적으로 제거할 거면 관련 스타일을 정리하는 편이 좋습니다.수정 예시
- <div className={`absolute inset-0 bg-gradient-to-r ${isActive ? "from-primary/10" : "from-primary/5"} to-transparent opacity-0 transition-opacity`}></div> + <div className={`absolute inset-0 bg-gradient-to-r ${isActive ? "from-primary/10 opacity-100" : "from-primary/5 opacity-0"} to-transparent transition-opacity`}></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/sidebar/ActivePhilosophers.tsx` at line 46, The gradient overlay div in ActivePhilosophers uses a hardcoded "opacity-0" so the gradient never appears; update the class logic on that div (referencing the isActive prop/variable and the overlay div in ActivePhilosophers) to conditionally set opacity (e.g., use isActive to choose "opacity-100" vs "opacity-0") or remove the opacity class entirely and adjust transitions accordingly so the gradient is actually visible when isActive is true.backend/app/services/embedding.py (1)
42-49: 동기/비동기 경로의 차원 검증 로직을 공통화하는 것을 권장합니다.현재
generate_embedding와agenerate_embedding에 동일한 길이 검증이 중복되어 있어, 이후 수정 시 불일치가 생기기 쉽습니다. 검증을 private helper로 묶어두면 유지보수가 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/embedding.py` around lines 42 - 49, The two methods generate_embedding and agenerate_embedding duplicate the embedding-length check; extract that logic into a private helper (e.g., _validate_embedding_dimension or _check_embedding_length) that accepts the embedding list and expected dimension and raises the same ValueError, then call this helper from both generate_embedding and agenerate_embedding to centralize validation and avoid future drift.
🤖 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/api/routes/chat.py`:
- Around line 42-47: Wrap the external translation call
get_english_translation(query) inside asyncio.wait_for with a sensible timeout
(e.g., 5-10s) to prevent SSE from hanging; update the try/except to catch
asyncio.TimeoutError separately (and logger.exception or logger.warning with
context) and yield the same error SSE payload, and keep the existing generic
Exception handler for other failures—modify the block around
get_english_translation in chat route to use asyncio.wait_for(...) and explicit
timeout handling.
In `@backend/app/main.py`:
- Around line 23-25: The except block that catches exceptions during the
pre-load models step currently only logs the error (logger.error(f"Failed to
pre-load models: {e}")) and continues, which delays startup failures to runtime;
change the except handling in the pre-load models section so it fails fast by
either re-raising the caught exception (raise) or terminating startup (e.g.,
sys.exit(1)) after logging, ensuring the error is surfaced immediately;
references to look for: the try/except around the pre-load models logic, the
logger.error call, and the surrounding startup generator that yields.
---
Nitpick comments:
In `@backend/app/services/embedding.py`:
- Around line 42-49: The two methods generate_embedding and agenerate_embedding
duplicate the embedding-length check; extract that logic into a private helper
(e.g., _validate_embedding_dimension or _check_embedding_length) that accepts
the embedding list and expected dimension and raises the same ValueError, then
call this helper from both generate_embedding and agenerate_embedding to
centralize validation and avoid future drift.
In `@frontend/components/sidebar/ActivePhilosophers.tsx`:
- Line 46: The gradient overlay div in ActivePhilosophers uses a hardcoded
"opacity-0" so the gradient never appears; update the class logic on that div
(referencing the isActive prop/variable and the overlay div in
ActivePhilosophers) to conditionally set opacity (e.g., use isActive to choose
"opacity-100" vs "opacity-0") or remove the opacity class entirely and adjust
transitions accordingly so the gradient is actually visible when isActive is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea234020-cf8a-4396-ba4d-ffa958c5191c
⛔ Files ignored due to path filters (1)
frontend/app/icon.pngis excluded by!**/*.png
📒 Files selected for processing (9)
README.mdbackend/app/api/routes/chat.pybackend/app/core/env_utils.pybackend/app/main.pybackend/app/services/embedding.pybackend/app/services/llm.pybackend/scripts/generate_book_mapping.pyfrontend/components/chat/MessageList.tsxfrontend/components/sidebar/ActivePhilosophers.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/services/embedding.py (1)
29-33: 차원 값384는 상수로 분리해 관리하는 편이 안전합니다.Line 29-Line 33의 매직 넘버를 상수화하면 모델 교체 시 검증 로직 누락 위험을 줄일 수 있습니다.
변경 예시
MODEL_NAME = "sentence-transformers/all-MiniLM-L6-v2" +EXPECTED_EMBEDDING_DIM = 384 @@ def _validate_embedding_dimension(self, embedding: list[float]) -> None: - if len(embedding) != 384: + if len(embedding) != EXPECTED_EMBEDDING_DIM: raise ValueError( - f"Unexpected embedding dimension: {len(embedding)} (expected 384)" + f"Unexpected embedding dimension: {len(embedding)} (expected {EXPECTED_EMBEDDING_DIM})" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/embedding.py` around lines 29 - 33, The _validate_embedding_dimension method uses the magic number 384; define a named constant (e.g., EMBEDDING_DIMENSION = 384) at the module or class level and replace the literal in both the len(embedding) comparison and the error message so the check reads against EMBEDDING_DIMENSION and the raised ValueError reports EMBEDDING_DIMENSION instead of the hardcoded 384.backend/app/main.py (1)
14-20: 의도 명시로 린트 경고(ARG001/B018)를 줄여주세요.Line 14의 미사용 인자와 Line 20의 사이드이펙트-only 표현식은 린트 노이즈를 만들 수 있습니다. 의도를 코드에 명시하면 CI 안정성이 좋아집니다.
변경 예시
-async def lifespan(app: FastAPI): +async def lifespan(_app: FastAPI): @@ - embedding_service.embeddings + _ = embedding_service.embeddings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 14 - 20, Rename the unused FastAPI parameter in the lifespan function to a named throwaway (e.g., _app) to signal the unused argument and avoid ARG001/B018 lint warnings, and replace the side-effect-only expression embedding_service.embeddings with an explicit intentful action: either call get_llm() to instantiate the LLM (e.g., _ = get_llm()) and explicitly consume the embeddings attribute (e.g., _ = embedding_service.embeddings) or call a dedicated preload method on embedding_service if available; target the lifespan function and the symbols embedding_service.embeddings and get_llm when making these changes.
🤖 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/app/main.py`:
- Around line 14-20: Rename the unused FastAPI parameter in the lifespan
function to a named throwaway (e.g., _app) to signal the unused argument and
avoid ARG001/B018 lint warnings, and replace the side-effect-only expression
embedding_service.embeddings with an explicit intentful action: either call
get_llm() to instantiate the LLM (e.g., _ = get_llm()) and explicitly consume
the embeddings attribute (e.g., _ = embedding_service.embeddings) or call a
dedicated preload method on embedding_service if available; target the lifespan
function and the symbols embedding_service.embeddings and get_llm when making
these changes.
In `@backend/app/services/embedding.py`:
- Around line 29-33: The _validate_embedding_dimension method uses the magic
number 384; define a named constant (e.g., EMBEDDING_DIMENSION = 384) at the
module or class level and replace the literal in both the len(embedding)
comparison and the error message so the check reads against EMBEDDING_DIMENSION
and the raised ValueError reports EMBEDDING_DIMENSION instead of the hardcoded
384.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cf150e1-01e0-4215-9813-98f7873bde40
📒 Files selected for processing (9)
backend/app/api/routes/chat.pybackend/app/main.pybackend/app/services/embedding.pybackend/pytest_log.txtbackend/pytest_log_utf8.txtbackend/tests/e2e/test_chat_endpoint.pybackend/tests/integration/test_supabase_match.pybackend/tests/unit/test_llm.pyfrontend/components/sidebar/ActivePhilosophers.tsx
✅ Files skipped from review due to trivial changes (1)
- backend/pytest_log_utf8.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/api/routes/chat.py
- frontend/components/sidebar/ActivePhilosophers.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/components/sidebar/Sidebar.tsx (1)
47-48:sizesprop 추가를 권장합니다.Next.js
Image컴포넌트에서fillprop을 사용할 때sizes속성을 지정하지 않으면, 브라우저가 적절한 이미지 크기를 선택하는 데 필요한 정보가 부족하여 최적화가 제대로 이루어지지 않을 수 있습니다. 고정 크기(40x40px)이므로 명시적으로 지정하는 것이 좋습니다.♻️ 제안하는 수정
<div className="h-10 w-10 shrink-0 relative flex items-center justify-center"> - <Image src="/icon.png" alt="PhiloRAG Logo" fill className="rounded-full object-cover shadow-[0_0_15px_rgba(217,183,74,0.3)] border border-[`#d9b74a`]/30" /> + <Image src="/icon.png" alt="PhiloRAG Logo" fill sizes="40px" className="rounded-full object-cover shadow-[0_0_15px_rgba(217,183,74,0.3)] border border-[`#d9b74a`]/30" /> </div>🤖 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 - 48, The Image component in Sidebar.tsx uses the fill prop inside a fixed 40x40 container without specifying sizes, which prevents Next.js from selecting the correct optimized image; update the <Image ... fill ... /> usage (in the Sidebar component) to include an appropriate sizes attribute—e.g. sizes="40px" or a responsive rule like sizes="(max-width: 640px) 40px, 40px"—so the browser and Next.js can pick the correct image source for that 40x40 UI element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/components/sidebar/Sidebar.tsx`:
- Around line 47-48: The Image component in Sidebar.tsx uses the fill prop
inside a fixed 40x40 container without specifying sizes, which prevents Next.js
from selecting the correct optimized image; update the <Image ... fill ... />
usage (in the Sidebar component) to include an appropriate sizes attribute—e.g.
sizes="40px" or a responsive rule like sizes="(max-width: 640px) 40px, 40px"—so
the browser and Next.js can pick the correct image source for that 40x40 UI
element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cb0ec55-1174-4835-88c5-fd49368cf8c2
📒 Files selected for processing (3)
backend/app/main.pybackend/app/services/embedding.pyfrontend/components/sidebar/Sidebar.tsx
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
업데이트
새 기능
버그 수정
스타일