Refactor/improve UI - #35
Conversation
Extract quiz preview modal from practice-view into dedicated full-page component (quiz-preview-view.tsx). Replace modal-based state management with URL-based navigation (?preview=true&id=quizId). Add Explanation field to QuizQuestion interface. Simplify header styling in student and lecturer practice views for consistency. This improves component reusability and enables direct sharing of quiz preview links.
Remove the 'Kết quả sinh viên' (student results) tab and associated table displaying quiz completion data. This includes removing the results panel UI, the Tabs.List navigation, and the mockStudentResults data.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughChangesQuiz previewing moves from an embedded lecturer modal to a routed preview page. The page selects preview or student mode via a query parameter, fetches and renders quiz details, supports draft publishing, adds optional question explanations, and updates lecturer and student practice layouts. Quiz Preview Flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Lecturer
participant TeacherPracticeView
participant usePractice
participant QuizPreviewPage
participant QuizPreviewView
participant quizApi
Lecturer->>TeacherPracticeView: select quiz preview
TeacherPracticeView->>usePractice: preview quiz
usePractice->>QuizPreviewPage: navigate with preview=true
QuizPreviewPage->>QuizPreviewView: render lecturer preview
QuizPreviewView->>quizApi: fetch quiz detail
quizApi-->>QuizPreviewView: return questions and metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (4)
frontend/src/components/lecturer/quiz/quiz-preview-view.tsx (2)
23-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew file already exceeds the 200-line guideline (244 lines).
Consider extracting the per-question rendering (options list + AI explanation block, roughly lines 148-239) into a separate
QuestionCard/QuestionExplanationcomponent to keepQuizPreviewViewfocused on data loading and page layout.As per coding guidelines, "Avoid God files; keep files under 200 lines and focused on a single purpose."
🤖 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 `@frontend/src/components/lecturer/quiz/quiz-preview-view.tsx` around lines 23 - 244, The new QuizPreviewView exceeds the 200-line guideline and mixes page state management with detailed question rendering. Extract the questions.map rendering, including option display and AI explanation markup, into a focused QuestionCard component (and optionally a small QuestionExplanation component), then render QuestionCard from QuizPreviewView while preserving the existing question, option, and explanation behavior.Source: Coding guidelines
149-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback
(q as any).xcasts undermine the typedQuizQuestioncontract.
QuizQuestion(inapi/quiz.ts) already definesID,Content,QuestionType,Options, and nowExplanation. Falling back to(q as any).id/.explanation/etc. bypasses that contract and hides real API mismatches instead of surfacing them.Please confirm the backend response for quiz questions actually matches the PascalCase
QuizQuestionshape; if it doesn't, the mapping should be normalized once inquizApi.getQuizDetailrather than defensively at every call site.🤖 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 `@frontend/src/components/lecturer/quiz/quiz-preview-view.tsx` around lines 149 - 153, Remove the `(q as any)` lowercase-property fallbacks from the question mapping in the quiz preview component and rely on the typed `QuizQuestion` fields. Verify the backend response and, if it uses a different shape, normalize it once in `quizApi.getQuizDetail` so callers receive the PascalCase `QuizQuestion` contract.frontend/src/hooks/lecturer/use-practice.ts (1)
178-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover modal-era state (
previewQuiz,isViewQuizOpen) now that preview navigation is route-based. Both files still declare/destructure this state even though the only consumer (the in-page preview modal) has been deleted.
frontend/src/hooks/lecturer/use-practice.ts#L178-L182: droppreviewQuiz/setPreviewQuizandisViewQuizOpen/setIsViewQuizOpen(and the no-opsetIsViewQuizOpen(false)inhandlePublishQuiz) from the hook's state and return value.frontend/src/components/lecturer/practice/practice-view.tsx#L626-L627: stop destructuringisViewQuizOpen,setIsViewQuizOpen,previewQuizfromusePractice()since nothing in the remaining JSX uses them.🤖 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 `@frontend/src/hooks/lecturer/use-practice.ts` around lines 178 - 182, Remove the unused previewQuiz/setPreviewQuiz and isViewQuizOpen/setIsViewQuizOpen state, including the no-op setIsViewQuizOpen(false) call in handlePublishQuiz, from usePractice and its return value. In frontend/src/hooks/lecturer/use-practice.ts:178-182, update the hook state and returned API; in frontend/src/components/lecturer/practice/practice-view.tsx:626-627, stop destructuring these values from usePractice.frontend/src/components/lecturer/practice/practice-view.tsx (1)
120-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSingle-panel
Tabswith noTabs.Listis now vestigial.After removing the "results" tab, only
Tabs.Panel value="dashboard"remains and there's noTabs.Listto switch tabs. TheTabs/activeTabmachinery no longer serves a purpose here; consider replacing with a plain wrapperdiv.♻️ Suggested simplification
- <div className="max-w-6xl mx-auto px-6 pb-12 space-y-8"> - <Tabs value={activeTab} onChange={setActiveTab} variant="outline" radius="lg"> - - <Tabs.Panel value="dashboard" className="space-y-8"> - <div className="grid gap-6 lg:grid-cols-3"> + <div className="max-w-6xl mx-auto px-6 pb-12 space-y-8"> + <div className="space-y-8"> + <div className="grid gap-6 lg:grid-cols-3">(and close the corresponding wrapper instead of
</Tabs.Panel>/</Tabs>)Also applies to: 430-433
🤖 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 `@frontend/src/components/lecturer/practice/practice-view.tsx` around lines 120 - 121, Remove the vestigial Tabs and activeTab machinery from the practice view, since only the dashboard panel remains and no tab list can switch it. Replace the outer Tabs wrapper with a plain div and update the matching closing wrapper around the dashboard content, removing the corresponding Tabs.Panel closing tag while preserving the panel’s contents.
🤖 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 `@frontend/src/app/`[role]/practice/quiz/page.tsx:
- Around line 9-18: Update QuizPageContent so the preview=true branch renders
QuizPreviewView only for authorized lecturers/quiz owners, using the existing
role or authorization context; all other users must fall back to TakeQuizView.
Preserve the current non-preview behavior.
In `@frontend/src/components/lecturer/quiz/quiz-preview-view.tsx`:
- Around line 29-50: Ensure the useEffect handling quizId clears loading when
the query parameter is missing, so the component reaches the “Không thể tải
thông tin bài tập.” branch instead of remaining stuck. Update the early-return
path before fetchDetail in the quiz detail loading flow, while preserving the
existing fetch behavior for valid quizId values.
---
Nitpick comments:
In `@frontend/src/components/lecturer/practice/practice-view.tsx`:
- Around line 120-121: Remove the vestigial Tabs and activeTab machinery from
the practice view, since only the dashboard panel remains and no tab list can
switch it. Replace the outer Tabs wrapper with a plain div and update the
matching closing wrapper around the dashboard content, removing the
corresponding Tabs.Panel closing tag while preserving the panel’s contents.
In `@frontend/src/components/lecturer/quiz/quiz-preview-view.tsx`:
- Around line 23-244: The new QuizPreviewView exceeds the 200-line guideline and
mixes page state management with detailed question rendering. Extract the
questions.map rendering, including option display and AI explanation markup,
into a focused QuestionCard component (and optionally a small
QuestionExplanation component), then render QuestionCard from QuizPreviewView
while preserving the existing question, option, and explanation behavior.
- Around line 149-153: Remove the `(q as any)` lowercase-property fallbacks from
the question mapping in the quiz preview component and rely on the typed
`QuizQuestion` fields. Verify the backend response and, if it uses a different
shape, normalize it once in `quizApi.getQuizDetail` so callers receive the
PascalCase `QuizQuestion` contract.
In `@frontend/src/hooks/lecturer/use-practice.ts`:
- Around line 178-182: Remove the unused previewQuiz/setPreviewQuiz and
isViewQuizOpen/setIsViewQuizOpen state, including the no-op
setIsViewQuizOpen(false) call in handlePublishQuiz, from usePractice and its
return value. In frontend/src/hooks/lecturer/use-practice.ts:178-182, update the
hook state and returned API; in
frontend/src/components/lecturer/practice/practice-view.tsx:626-627, stop
destructuring these values from usePractice.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b54fd6ad-6bec-4379-a020-5df66a3bd7d3
📒 Files selected for processing (6)
frontend/src/api/quiz.tsfrontend/src/app/[role]/practice/quiz/page.tsxfrontend/src/components/lecturer/practice/practice-view.tsxfrontend/src/components/lecturer/quiz/quiz-preview-view.tsxfrontend/src/components/student/practice/student-practice-view.tsxfrontend/src/hooks/lecturer/use-practice.ts
| function QuizPageContent() { | ||
| const searchParams = useSearchParams(); | ||
| const isPreview = searchParams.get("preview") === "true"; | ||
|
|
||
| if (isPreview) { | ||
| return <QuizPreviewView />; | ||
| } | ||
|
|
||
| return <TakeQuizView />; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "useParams|role" frontend/src/app -g '*layout*'Repository: UGing265/SWD392_Chatbot_RAG
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== quiz page ==\n'
sed -n '1,220p' frontend/src/app/[role]/practice/quiz/page.tsx
printf '\n== role layout ==\n'
sed -n '1,220p' frontend/src/app/[role]/layout.tsx
printf '\n== role shell ==\n'
sed -n '1,260p' frontend/src/components/layout/role-shell.tsxRepository: UGing265/SWD392_Chatbot_RAG
Length of output: 2510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth client refs ==\n'
rg -n "auth-client|Better Auth|session|role" frontend/src -g '!**/node_modules/**' | sed -n '1,220p'Repository: UGing265/SWD392_Chatbot_RAG
Length of output: 21172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backend quiz api refs ==\n'
rg -n "publishQuiz|getQuizDetail|quizApi" . -g '!**/node_modules/**' | sed -n '1,220p'Repository: UGing265/SWD392_Chatbot_RAG
Length of output: 2801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== frontend/src/app/[role]/practice/quiz/page.tsx =='
sed -n '1,220p' frontend/src/app/[role]/practice/quiz/page.tsx
echo
echo '== frontend/src/app/[role]/layout.tsx =='
sed -n '1,220p' frontend/src/app/[role]/layout.tsx
echo
echo '== frontend/src/components/layout/role-shell.tsx =='
sed -n '1,260p' frontend/src/components/layout/role-shell.tsx
echo
echo '== role/auth references =='
rg -n "RoleShell|useSession|session|role" frontend/src/components frontend/src/app frontend/lib -g '!**/node_modules/**' | sed -n '1,260p'Repository: UGing265/SWD392_Chatbot_RAG
Length of output: 21169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
frontend/src/app/[role]/layout.tsx \
frontend/src/app/[role]/practice/quiz/page.tsx \
frontend/src/components/layout/role-shell.tsx
do
echo "### $f"
wc -l "$f"
sed -n '1,220p' "$f"
echo
doneRepository: UGing265/SWD392_Chatbot_RAG
Length of output: 2735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' frontend/src/components/lecturer/quiz/quiz-preview-view.tsxRepository: UGing265/SWD392_Chatbot_RAG
Length of output: 9422
Gate quiz preview to lecturers
RoleShell already blocks cross-role routing, but this page still opens QuizPreviewView for any user on their own /[role]/practice/quiz route when preview=true is present. That exposes lecturer-only quiz content/correct answers and the publish control; restrict the branch to lecturers/quiz owners or split it out of the shared page.
🤖 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 `@frontend/src/app/`[role]/practice/quiz/page.tsx around lines 9 - 18, Update
QuizPageContent so the preview=true branch renders QuizPreviewView only for
authorized lecturers/quiz owners, using the existing role or authorization
context; all other users must fall back to TakeQuizView. Preserve the current
non-preview behavior.
Source: Coding guidelines
| const quizId = searchParams.get("id"); | ||
| const [quizDetail, setQuizDetail] = useState<QuizDetailResponse | null>(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const [publishing, setPublishing] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (!quizId) return; | ||
|
|
||
| const fetchDetail = async () => { | ||
| try { | ||
| setLoading(true); | ||
| const data = await quizApi.getQuizDetail(quizId); | ||
| setQuizDetail(data); | ||
| } catch (err) { | ||
| console.error("Failed to fetch quiz detail:", err); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchDetail(); | ||
| }, [quizId]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stuck spinner when id query param is missing.
loading starts true and is only set to false inside fetchDetail, which never runs if quizId is falsy — the component is stuck on the loading state forever instead of falling through to the "Không thể tải thông tin bài tập." error branch.
🐛 Proposed fix
useEffect(() => {
- if (!quizId) return;
+ if (!quizId) {
+ setLoading(false);
+ return;
+ }
const fetchDetail = async () => {Also applies to: 70-92
🤖 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 `@frontend/src/components/lecturer/quiz/quiz-preview-view.tsx` around lines 29
- 50, Ensure the useEffect handling quizId clears loading when the query
parameter is missing, so the component reaches the “Không thể tải thông tin bài
tập.” branch instead of remaining stuck. Update the early-return path before
fetchDetail in the quiz detail loading flow, while preserving the existing fetch
behavior for valid quizId values.
1 participant
Summary by CodeRabbit