Fix uploaded lesson prompts through LMS preview#628
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 46 minutes and 38 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR centralizes uploaded-document lesson authoring and preview intent detection into a new ChangesLesson Preview Intent Routing & Frontend Integration
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
maritime-ai-service/app/engine/multi_agent/tool_collection.py (1)
623-629: 🧹 Nitpick | 🔵 TrivialAdd explicit risk telemetry + rollback toggle for forced preview routing.
Risk note: this branch forces a host-action tool in uploaded-doc turns; misclassification can change write-adjacent LMS behavior.
Rollback note: keep a dedicated runtime flag (or env switch) to disable lesson-preview forcing independently of other host-action features, and log forced-route reason (coursevslesson_preview) for quick rollback decisions.As per coding guidelines, changes under
maritime-ai-service/app/{auth,core,engine,repositories}/**should include explicit risk and rollback notes for provider/runtime high-risk surfaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@maritime-ai-service/app/engine/multi_agent/tool_collection.py` around lines 623 - 629, Add explicit risk telemetry, a dedicated runtime rollback toggle, and a reason tag when forcing document-preview routing: guard the branch that calls _looks_like_document_preview_request(...) and uses _preferred_document_preview_host_action_tools(...) with a separate runtime flag/env (e.g., ENABLE_FORCE_LESSON_PREVIEW) so preview forcing can be disabled independently; emit structured telemetry/metrics and logger entries that include a clear forced-route reason field ("course" vs "lesson_preview") and a risk note string before returning preview_tools[:1], True; also add an inline comment summarizing the rollback instruction and high-risk surface for future reviewers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@maritime-ai-service/app/engine/multi_agent/document_preview_contract.py`:
- Around line 90-106: The tuple _LESSON_PREVIEW_REQUEST_MARKERS contains overly
broad standalone tokens ("draft", "preview", "citation", "diff") that produce
false-positive routing to the LMS preview host-action; tighten matching so these
weak markers only trigger when anchored to a lesson context (e.g., require
presence of a lesson anchor token like "lesson", "bai hoc", "lesson patch", "ban
xem truoc" or localized equivalents, or convert them to explicit anchored
phrases such as "lesson preview", "preview_lesson", "lesson draft", "lesson
citation"); update _LESSON_PREVIEW_REQUEST_MARKERS accordingly and apply the
same tightening to the other preview-marker tuple defined elsewhere in this
module to preserve correct fallback behavior and routing.
In `@wiii-desktop/src/EmbedApp.tsx`:
- Around line 320-322: The Suspense wrapper around PreviewPanel currently uses
fallback={null}, which shows nothing while PreviewPanel lazy-loads; change the
Suspense fallback to a minimal loading indicator to avoid a flash (e.g., a small
spinner or placeholder). Locate the Suspense component wrapping <PreviewPanel />
in EmbedApp (symbols: Suspense and PreviewPanel) and replace fallback={null}
with a lightweight UX element (spinner, skeleton, or "Loading preview…") or
conditionally keep null only if PreviewPanel is guaranteed hidden on mount.
---
Outside diff comments:
In `@maritime-ai-service/app/engine/multi_agent/tool_collection.py`:
- Around line 623-629: Add explicit risk telemetry, a dedicated runtime rollback
toggle, and a reason tag when forcing document-preview routing: guard the branch
that calls _looks_like_document_preview_request(...) and uses
_preferred_document_preview_host_action_tools(...) with a separate runtime
flag/env (e.g., ENABLE_FORCE_LESSON_PREVIEW) so preview forcing can be disabled
independently; emit structured telemetry/metrics and logger entries that include
a clear forced-route reason field ("course" vs "lesson_preview") and a risk note
string before returning preview_tools[:1], True; also add an inline comment
summarizing the rollback instruction and high-risk surface for future reviewers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ba4a380-1e63-4f0e-86dd-c30e223979fc
📒 Files selected for processing (7)
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/tests/unit/test_document_context_api.pymaritime-ai-service/tests/unit/test_document_preview_contract.pywiii-desktop/src/EmbedApp.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Backend Unit Gate
- GitHub Check: Desktop Gate
- GitHub Check: Unit Tests
- GitHub Check: Build Images
- GitHub Check: Security Audit (pip-audit)
- GitHub Check: CodeQL Analyze (python)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: CodeQL Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
wiii-desktop/src/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve Vietnamese-first user-facing copy in UI, prompts, and error messages unless the surrounding product surface is intentionally English
Files:
wiii-desktop/src/EmbedApp.tsx
wiii-desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
wiii-desktop/src/**/*.{ts,tsx}: For frontend-visible changes, include screenshots or a clear reason why visual evidence is not applicable
Run desktop verification:cd wiii-desktop && npx vitest run && npx tsc --noEmit && npm run build:embed
Forwiii-desktop/src/**, verify SSE V3 event handling, persisted Zustand state, auth refresh, Tauri HTTP/fetch fallback parity, accessibility, responsive behavior, and no accidental loss of conversation state
wiii-desktop/src/**/*.{ts,tsx}: Never show raw internal tool JSON as the final answer in chat output
Keep renderer fixes in renderer or repair helpers, not inside random chat components
Avoid adding hidden global state for chat lifecycle; use explicit Zustand stores
Source references must remain visible when an answer uses uploaded documents
Vietnamese UI copy should be natural and accented unless the surrounding surface is intentionally English
Files:
wiii-desktop/src/EmbedApp.tsx
wiii-desktop/src/**
⚙️ CodeRabbit configuration file
wiii-desktop/src/**: Review React/Tauri changes for SSE correctness, persisted Zustand state, auth refresh behavior, accessibility, responsive UI, and CORS/Tauri HTTP fallback compatibility.
Files:
wiii-desktop/src/EmbedApp.tsx
maritime-ai-service/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run backend verification:
cd maritime-ai-service && set PYTHONIOENCODING=utf-8 && pytest tests/unit/ -p no:capture --tb=short -q && ruff check app/ --select=E9,F63,F7
Files:
maritime-ai-service/tests/unit/test_document_context_api.pymaritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/tests/unit/test_document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/tool_collection.py
maritime-ai-service/app/{auth,core,engine,repositories}/**
📄 CodeRabbit inference engine (AGENTS.md)
maritime-ai-service/app/{auth,core,engine,repositories}/**: For backend, auth, memory, tenant isolation, migration, provider/runtime, MCP, or deployment changes, include explicit risk and rollback notes
Treat auth, JWT, OAuth, LMS token exchange, organization context, tenant isolation, semantic memory, long-term memory, MCP/tool execution, provider routing, migrations, and GitHub automation as high-risk surfaces requiring P0/P1 flagging when changes expose private data, cross tenant boundaries, bypass authorization, corrupt persistent memory, break streaming contracts, weaken deployment safety, or make rollback unclear
Files:
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/tool_collection.py
maritime-ai-service/app/engine/**
📄 CodeRabbit inference engine (AGENTS.md)
For
maritime-ai-service/app/engine/**, verify routing correctness, source propagation, memory/tool boundaries, streaming parity, structured output robustness, and fallback behavior
Files:
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/tool_collection.py
maritime-ai-service/app/engine/multi_agent/**
📄 CodeRabbit inference engine (maritime-ai-service/AGENTS.md)
Agent runtime changes should preserve routing, tool loop, provider behavior, and source propagation
Files:
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/tool_collection.py
⚙️ CodeRabbit configuration file
maritime-ai-service/app/engine/multi_agent/**: Focus on routing correctness, streaming parity, memory/tool boundaries, structured output robustness, and fallback behavior. Flag hidden prompt or state-contract changes.
Files:
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.pymaritime-ai-service/app/engine/multi_agent/document_preview_contract.pymaritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.pymaritime-ai-service/app/engine/multi_agent/tool_collection.py
🔇 Additional comments (7)
wiii-desktop/src/EmbedApp.tsx (1)
20-20: PreviewPanel lazy import/export mapping is consistent
wiii-desktop/src/components/layout/PreviewPanel.tsxexportsPreviewPanelas a named export, andEmbedApp.tsxcorrectly wraps it forReact.lazyvia{ default: mod.PreviewPanel }. Using<PreviewPanel />is also valid sinceinlineis optional; the component returnsnullwhen the preview panel is closed, soSuspense fallback={null}won’t introduce unwanted placeholder UI.maritime-ai-service/app/engine/multi_agent/document_preview_contract.py (1)
169-174: LGTM!maritime-ai-service/app/engine/multi_agent/tool_collection.py (1)
27-39: LGTM!Also applies to: 291-305
maritime-ai-service/app/engine/multi_agent/direct_document_preview_payloads.py (1)
12-13: LGTM!Also applies to: 138-149
maritime-ai-service/app/engine/multi_agent/direct_node_uploaded_context.py (1)
11-13: LGTM!Also applies to: 121-130
maritime-ai-service/tests/unit/test_document_preview_contract.py (1)
11-12: LGTM!Also applies to: 48-53, 70-73
maritime-ai-service/tests/unit/test_document_context_api.py (1)
510-535: LGTM!
| <Suspense fallback={null}> | ||
| <PreviewPanel /> | ||
| </Suspense> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider a loading fallback for better UX.
The fallback={null} means no loading indicator is shown during PreviewPanel's initial lazy load. If PreviewPanel is conditionally visible (only during LMS preview actions), this is acceptable. However, if it's immediately visible on mount, users may experience a brief content flash.
Optional: Add a minimal loading fallback
- <Suspense fallback={null}>
+ <Suspense fallback={<div className="sr-only">Loading preview...</div>}>
<PreviewPanel />
</Suspense>Note: Only needed if PreviewPanel is immediately visible. If it's hidden by default until preview actions trigger, fallback={null} is fine.
📝 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.
| <Suspense fallback={null}> | |
| <PreviewPanel /> | |
| </Suspense> | |
| <Suspense fallback={<div className="sr-only">Loading preview...</div>}> | |
| <PreviewPanel /> | |
| </Suspense> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wiii-desktop/src/EmbedApp.tsx` around lines 320 - 322, The Suspense wrapper
around PreviewPanel currently uses fallback={null}, which shows nothing while
PreviewPanel lazy-loads; change the Suspense fallback to a minimal loading
indicator to avoid a flash (e.g., a small spinner or placeholder). Locate the
Suspense component wrapping <PreviewPanel /> in EmbedApp (symbols: Suspense and
PreviewPanel) and replace fallback={null} with a lightweight UX element
(spinner, skeleton, or "Loading preview…") or conditionally keep null only if
PreviewPanel is guaranteed hidden on mount.
meiiie
force-pushed
the
codex/627-fix-uploaded-lesson-preview-intent
branch
from
0a614da to
1ebe7b1
Compare
|
Governance note: repo-owned checks are green on commit 1ebe7b1. CodeRabbit is failing only with \Insufficient review credits, so this is treated as non-blocking per Wiii automation governance. |
1 participant
Summary
document_preview_contractand reuses it from preflight, tool payload shortcut, and tool collection.PreviewPanelin the embed surface so host-action previews returned inside LMS iframe can be reviewed and applied.Scope
Non-Scope
dist-embed/artifacts.Verification
cd maritime-ai-service; uv run --python 3.12 --extra dev pytest tests/unit/test_document_preview_contract.py tests/unit/test_document_context_api.py::test_uploaded_document_preview_request_bypasses_fact_fast_path tests/unit/test_document_context_api.py::test_uploaded_document_lesson_creation_request_bypasses_fact_fast_path tests/unit/test_direct_node_document_preview_runtime.py tests/unit/test_direct_node_provider_errors.py::test_direct_response_node_rebinds_document_preview_tool_when_collection_misses tests/unit/test_direct_node_provider_errors.py::test_direct_response_node_preflights_document_preview_before_tool_collection -q --tb=short-> 12 passed.cd maritime-ai-service; uv run --python 3.12 --extra dev ruff check app/engine/multi_agent/document_preview_contract.py app/engine/multi_agent/direct_node_uploaded_context.py app/engine/multi_agent/direct_document_preview_payloads.py app/engine/multi_agent/tool_collection.py tests/unit/test_document_preview_contract.py tests/unit/test_document_context_api.py-> passed.cd wiii-desktop; npx vitest run src/__tests__/preview-panel-ui.test.tsx src/__tests__/context-bridge.test.ts --pool forks --maxWorkers=1-> 24 passed.cd wiii-desktop; npx tsc --noEmit-> passed.cd wiii-desktop; npm run build:embed-> passed; Vite emitted existing chunk-size / ineffective dynamic import warnings.embed.htmliframe + real DOCX uploadke-hoach-bai-hoc-wiii-e2e.docx-> document parse via markitdown -> prompt "t?o cho m?nh b?i h?c" ->authoring.preview_lesson_patchwith source refs -> PreviewPanel visible withpreview-e2e-token-> Apply forwardspreview_tokenandapproval_token-> fake course editor draft updated -> 0 Pointy actions.git diff --check-> passed.Risk
approval_tokenwhen the host capability schema requires it.Rollback
Summary by CodeRabbit
New Features
Improvements
Tests