Repository navigation
Feature/teacher student loginn - #4
Open
lenhatminh2102 wants to merge 8 commits into
Open
lenhatminh2102 wants to merge 8 commits into
lenhatminh2102 wants to merge 8 commits into
Conversation
…into ui/refactor-ui-student-curriculum
📝 WalkthroughWalkthroughThis PR replaces mock authentication and local data flows with backend sessions and API clients across administration, syllabus, document, and chatbot features. It adds Super Admin management flows, lecturer registration and review, live syllabus rendering, streaming chat, seed data, Supabase factories, and RAG fallback handling. ChangesAuthentication and Administration
Syllabus, Documents, and Chatbot
Backend Setup
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage
participant AuthContext
participant Backend
participant ProtectedRoute
User->>LoginPage: submit credentials
LoginPage->>AuthContext: login(email, password)
AuthContext->>Backend: authenticate and load session
Backend-->>AuthContext: session and role
AuthContext-->>LoginPage: authentication result
LoginPage->>ProtectedRoute: navigate to role portal
ProtectedRoute->>AuthContext: read loading and role state
AuthContext-->>ProtectedRoute: session state
ProtectedRoute-->>User: render portal or redirect
sequenceDiagram
participant Admin
participant LecturerRequestsPage
participant BackendAPI
Admin->>LecturerRequestsPage: approve lecturer request
LecturerRequestsPage->>BackendAPI: POST approval endpoint
BackendAPI-->>LecturerRequestsPage: temporary credentials
LecturerRequestsPage-->>Admin: display credentials modal
sequenceDiagram
participant Student
participant ChatbotWidget
participant ChatAPI
participant ChatMessageList
Student->>ChatbotWidget: send question
ChatbotWidget->>ChatAPI: create scoped session
ChatbotWidget->>ChatAPI: stream question
ChatAPI-->>ChatbotWidget: response chunks and citations
ChatbotWidget->>ChatMessageList: update message
ChatMessageList-->>Student: render streamed answer
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
PR Summary by QodoIntegrate Better Auth sessions with real admin/whitelist/lecturer-request APIs
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
90 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+133
to
+136
| await apiFetch("/api/whitelist", { | ||
| method: "POST", | ||
| body: JSON.stringify({ email }), | ||
| }); |
There was a problem hiding this comment.
1. Whitelist payload types missing 📘 Rule violation ⚙ Maintainability
Whitelist REST requests are constructed with inline object literals instead of a named request payload type, reducing type safety and consistency across API contracts. This violates the requirement to define and use TypeScript types for API payloads/parameters/results in changed code.
Agent Prompt
## Issue description
`apiFetch` calls for whitelist mutations build request bodies with inline object literals (e.g., `JSON.stringify({ email })`). The compliance rule requires using named TypeScript `type`/`interface` definitions for API payloads/parameters/results.
## Issue Context
This PR introduced new REST calls to `/api/whitelist` and `/api/whitelist/import`. Response types are defined, but request payloads are not.
## Fix Focus Areas
- web/app/superadmin/whitelist/page.tsx[133-136]
- web/app/superadmin/whitelist/page.tsx[200-203]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+94
to
+102
| const searchField = searchTerm.includes("@") ? "email" : "name"; | ||
| const baseQuery: Record<string, string> = { | ||
| limit: roleFilter === "ALL" ? String(PAGE_SIZE) : "500", | ||
| offset: roleFilter === "ALL" ? String((page - 1) * PAGE_SIZE) : "0", | ||
| }; | ||
| if (searchTerm) { | ||
| baseQuery.searchValue = searchTerm; | ||
| baseQuery.searchField = searchField; | ||
| } |
There was a problem hiding this comment.
2. basequery uses record 📘 Rule violation ⚙ Maintainability
The listUsers query parameters are assembled into a generic Record<string, string> instead of a named query-parameter type, weakening compile-time guarantees for API parameters. This violates the requirement to define and use TypeScript types for API parameters in changed code.
Agent Prompt
## Issue description
The admin users listing constructs API query parameters as `Record<string, string>`, which is not a named type describing the expected parameter shape.
## Issue Context
This PR added `authClient.admin.listUsers({ query: baseQuery })` usage with `baseQuery` being dynamically extended (`searchValue`, `searchField`). The compliance rule requires named TypeScript types for API parameters.
## Fix Focus Areas
- web/app/superadmin/admins/page.tsx[94-104]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+39
to
+46
| res = await fetch(`${apiBaseUrl}${path}`, { | ||
| credentials: "include", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| ...init?.headers, | ||
| }, | ||
| ...init, | ||
| }); |
There was a problem hiding this comment.
3. Apifetch merge overwritten 🐞 Bug ☼ Reliability
In web/lib/auth-client.ts, apiFetch merges headers but then spreads ...init afterwards, allowing caller-provided init (especially headers/credentials) to overwrite the merged defaults and break requests unexpectedly. This makes the helper unreliable as soon as any call passes custom headers/options.
Agent Prompt
## Issue description
`apiFetch` constructs a request object with default `credentials` and merged `headers`, but then spreads `...init` *after* those fields. In JS object spread, later properties win, so `init.headers` can overwrite the merged header object and `init.credentials` can overwrite `"include"`.
## Issue Context
This helper is intended to standardize REST calls to non–Better Auth endpoints (whitelist, lecturer-requests). It should be safe even when callers pass `headers`.
## Fix Focus Areas
- web/lib/auth-client.ts[32-62]
### Suggested implementation direction
- Build `const headers = new Headers(init?.headers)`.
- Only set `Content-Type: application/json` when sending a JSON body (or when caller didn’t provide it).
- Call `fetch(url, { ...init, credentials: "include", headers })` (note `...init` comes *before* the enforced defaults, or explicitly override after it).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+45
to
53
| const redirectAfterAuth = async () => { | ||
| const session = await authClient.getSession(); | ||
| const role = toUserRole(session.data?.user?.role); | ||
| router.push(portalPathForRole(role)); | ||
| }; | ||
|
|
||
| const handleCredentialsLogin = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| if (!email || !password) { |
There was a problem hiding this comment.
4. Login redirect misroutes users 🐞 Bug ≡ Correctness
LoginPage redirects by calling authClient.getSession() and defaulting a missing/unknown role to STUDENT, which can route ADMIN/LECTURER to the wrong portal if the session payload lacks role. Additionally, Google OAuth sets callbackURL to /login but the page has no “already authenticated → redirect” logic, so users can land back on the login screen after successful OAuth.
Agent Prompt
## Issue description
`redirectAfterAuth()` derives role from `getSession()` and falls back to `STUDENT`, which can misroute users when session responses don’t include `role`. Google OAuth returns to `/login` but the page does not auto-redirect authenticated users, leaving them stuck.
## Issue Context
- API docs show `get-session` example response omits `user.role`, while `sign-in/email` response includes it.
- Current code uses `/login` as OAuth callbackURL.
## Fix Focus Areas
- web/app/login/page.tsx[45-86]
- web/app/contexts/AuthContext.tsx[62-81]
- docs/api/00_auth.md[92-107]
- docs/api/00_auth.md[142-160]
### Suggested implementation direction
1) Credentials login:
- Change `AuthContext.login()` to return the logged-in user (or at least `role`) from `authClient.signIn.email` response.
- In `LoginPage`, redirect using that returned role (no extra `getSession()` round-trip).
2) Google OAuth:
- Keep callbackURL as `/login` **or** set it to a dedicated callback route, but add a `useEffect` in `LoginPage` that checks session/auth state (e.g., `useAuth()`/`authClient.useSession()`) and immediately redirects to `portalPathForRole(user.role)` when authenticated.
- Avoid defaulting missing role to STUDENT; if role is missing, show an error and require re-auth or fetch a role-bearing endpoint.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+295
to
+300
| const res = await authClient.admin.impersonateUser({ userId }); | ||
| if (res.error) throw new Error(res.error.message); | ||
| refetchSession(); | ||
| const role = (detail?.role as UserRole) ?? "STUDENT"; | ||
| router.push(portalPathForRole(role)); | ||
| } catch (err) { |
There was a problem hiding this comment.
5. Impersonation session race 🐞 Bug ☼ Reliability
Admin user detail navigates to the impersonated portal immediately after calling refetchSession() without awaiting session refresh, so ProtectedRoute may still see the old ADMIN session and redirect back. This makes impersonation flaky/non-functional depending on timing.
Agent Prompt
## Issue description
After `authClient.admin.impersonateUser`, the code calls `refetchSession()` (fire-and-forget) and immediately `router.push(...)`. Because route guards depend on session role, the new page can load with the stale ADMIN session and bounce back.
## Issue Context
- `refetchSession` is currently typed/implemented as void and cannot be awaited.
- `ProtectedRoute` redirects based on the current `user.role`.
## Fix Focus Areas
- web/app/superadmin/admins/[id]/page.tsx[282-310]
- web/app/contexts/AuthContext.tsx[62-95]
- web/components/ProtectedRoute.tsx[17-35]
### Suggested implementation direction
- Change `refetchSession` to return a `Promise` (`refetchSession: () => Promise<void>`), implemented as `return refetch()`.
- In `handleImpersonate`, `await refetchSession()` and then read the *fresh* session/user role (from `useAuth().user` after refresh, or by calling `authClient.getSession()` and validating the role).
- Redirect after the role is confirmed; consider `window.location.href = portalPathForRole(role)` to force a full reload if client-side navigation still races cookie/session state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+94
to
+117
| const searchField = searchTerm.includes("@") ? "email" : "name"; | ||
| const baseQuery: Record<string, string> = { | ||
| limit: roleFilter === "ALL" ? String(PAGE_SIZE) : "500", | ||
| offset: roleFilter === "ALL" ? String((page - 1) * PAGE_SIZE) : "0", | ||
| }; | ||
| if (searchTerm) { | ||
| baseQuery.searchValue = searchTerm; | ||
| baseQuery.searchField = searchField; | ||
| } | ||
|
|
||
| const res = await authClient.admin.listUsers({ query: baseQuery }); | ||
| if (res.error) { | ||
| throw new Error(res.error.message ?? "Không tải được danh sách người dùng"); | ||
| } | ||
|
|
||
| let list = (res.data?.users ?? []) as AdminListUser[]; | ||
| let count = res.data?.total ?? list.length; | ||
|
|
||
| if (roleFilter !== "ALL") { | ||
| list = list.filter((u) => u.role === roleFilter); | ||
| count = list.length; | ||
| const start = (page - 1) * PAGE_SIZE; | ||
| list = list.slice(start, start + PAGE_SIZE); | ||
| } |
There was a problem hiding this comment.
6. Role filter capped 500 🐞 Bug ≡ Correctness
Superadmin user list implements role filtering by fetching only the first 500 users and filtering client-side, so results and total counts become wrong once the system has more than 500 users. Pagination under a role filter is therefore incorrect for larger datasets.
Agent Prompt
## Issue description
When `roleFilter !== "ALL"`, the page requests `limit: "500"` and then filters/slices locally. This silently truncates datasets >500 users and yields incorrect `total`/pagination.
## Issue Context
`list-users` is a paginated endpoint (`limit`/`offset`) and does not document a role filter parameter, so the UI must either:
- iterate pages until it has accurate counts, or
- remove/disable role filtering until backend supports role filtering.
## Fix Focus Areas
- web/app/superadmin/admins/page.tsx[91-129]
- docs/api/00_auth.md[376-399]
### Suggested implementation direction
Option A (accurate, potentially more calls):
- When roleFilter is set, loop over `listUsers` pages (using `limit/offset`) accumulating matches and counting total matches; stop early once you collected enough matches to render the requested page plus know total count.
Option B (simpler UX, avoids wrong data):
- Disable the role filter UI (or show a warning) until backend adds a server-side role filter endpoint/query param.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+83
to
+96
| const [usersPageRes, whitelistRes, usersListRes] = await Promise.all([ | ||
| authClient.admin.listUsers({ query: { limit: "1", offset: "0" } }), | ||
| apiFetch<WhitelistListResponse>("/api/whitelist?page=1&limit=1"), | ||
| authClient.admin.listUsers({ query: { limit: "500", offset: "0" } }), | ||
| ]); | ||
|
|
||
| if (usersPageRes.error) { | ||
| throw new Error(usersPageRes.error.message ?? "Không tải được danh sách người dùng"); | ||
| } | ||
|
|
||
| const totalUsers = usersPageRes.data?.total ?? 0; | ||
| const users = usersListRes.data?.users ?? []; | ||
| const totalAdmins = users.filter((u) => u.role === "ADMIN").length; | ||
| const totalWhitelist = whitelistRes.pagination.total; |
There was a problem hiding this comment.
7. Admin stats capped 500 🐞 Bug ≡ Correctness
Dashboard computes totalAdmins by fetching only the first 500 users and counting role === ADMIN, so it undercounts admins when there are more than 500 total users. This makes dashboard statistics incorrect at scale.
Agent Prompt
## Issue description
Dashboard derives `totalAdmins` from a `listUsers(limit=500)` call, so the admin count is truncated when user count exceeds 500.
## Issue Context
`list-users` is paginated; there is no documented role filter to request `ADMIN` users directly.
## Fix Focus Areas
- web/app/superadmin/page.tsx[80-99]
- docs/api/00_auth.md[376-399]
### Suggested implementation direction
- Use `usersPageRes.data.total` to know total user count.
- Iterate `listUsers` in pages (e.g., limit=100/200) until `offset >= total`, accumulating `ADMIN` count.
- Alternatively, add a backend endpoint for counts by role (best long-term), then call that from the dashboard instead of scanning users client-side.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
web/app/superadmin/lecturer-requests/page.tsx (2)
84-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a subtler refresh state after approve/reject actions.
loadRequestsunconditionally setsloading=true, which replaces the entire table (including the Tabs and filter counts) with a full-page<Loader>every time the list is refreshed after an approve or reject action. This causes a visually jarring full-content swap for what is essentially a background refresh.Consider adding a separate
refreshingstate that shows a subtle indicator (e.g., a dimmed overlay or a small spinner in the toolbar) while keeping the table visible:♻️ Suggested refactor
export default function LecturerRequestsPage() { const [allRequests, setAllRequests] = useState<LecturerRequest[]>([]); const [loading, setLoading] = useState(true); + const [refreshing, setRefreshing] = useState(false); // ... const loadRequests = useCallback(async () => { - setLoading(true); + if (allRequests.length > 0) { + setRefreshing(true); + } else { + setLoading(true); + } try { const data = await apiFetch<LecturerRequestListResponse>( "/api/auth-admin/admin/lecturer-requests", ); setAllRequests(data.requests); } catch (err) { // ... } finally { - setLoading(false); + setLoading(false); + setRefreshing(false); } }, []);🤖 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 `@web/app/superadmin/lecturer-requests/page.tsx` around lines 84 - 105, The current loadRequests flow in LecturerRequestsPage uses the same loading state for both initial page load and post-approve/reject refreshes, which forces the whole table view to disappear. Split this into a dedicated refreshing state in the LecturerRequestsPage component and keep loading for the first fetch only, then have loadRequests set the subtle refresh state when reloading after actions so the Tabs, counts, and table remain visible with a lighter indicator instead of the full-page Loader.
303-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty table state is rendered outside the
<Table.Tbody>.When
filteredRequestsis empty, the<Table.Tbody>renders zero rows (only headers visible), and the "Không có yêu cầu nào phù hợp" message at lines 379-385 appears below the table inside theCard. This is functional but produces an empty table body that some screen readers may announce as an empty table. Consider conditionally rendering the empty state instead of the table whenfilteredRequests.length === 0:♻️ Suggested approach
<Card p={0} radius={0} style={{ border: "1px solid `#E2E8F0`", backgroundColor: "white" }}> - <Table layout="fixed" highlightOnHover striped> - {/* ... thead ... */} - <Table.Tbody> - {filteredRequests.map((request) => ( - /* ... rows ... */ - ))} - </Table.Tbody> - </Table> - - {filteredRequests.length === 0 && ( + {filteredRequests.length === 0 ? ( <Box p="xl" style={{ textAlign: "center", color: "`#9CA3AF`" }}> <Text size="sm" fw={700}> Không có yêu cầu nào phù hợp </Text> </Box> + ) : ( + <Table layout="fixed" highlightOnHover striped> + {/* ... existing table content ... */} + </Table> )} </Card>🤖 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 `@web/app/superadmin/lecturer-requests/page.tsx` around lines 303 - 375, The empty state for filteredRequests is currently shown outside Table.Tbody, leaving an empty table body when there are no results. Update the lecturer requests table in page.tsx so the Table, Table.Tbody, and the “Không có yêu cầu nào phù hợp” empty message are conditionally rendered based on filteredRequests.length, using the existing filteredRequests map and the surrounding Table/Card structure. Keep the normal rows rendering in Table.Tbody when data exists, and render the empty state instead of the table when there are no matching requests.web/app/superadmin/admins/[id]/page.tsx (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSingle
actionLoadingflag makes every action button spin at once.All admin action buttons (ban/unban, impersonate, remove, save role) bind
loading={actionLoading}, so triggering any one action shows a loading spinner on all of them simultaneously. Consider a per-action key (e.g.actionLoading: string | null) if the simultaneous spinners are undesirable.Also applies to: 491-536
🤖 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 `@web/app/superadmin/admins/`[id]/page.tsx at line 82, The single actionLoading boolean is shared by all admin actions in the Admin page, so every button reflects loading at once. Update the state in the page component that renders the ban/unban, impersonate, remove, and save role actions to track the active action by key (for example, a string or null) instead of one boolean, and wire each button’s loading prop to compare against its own action name. Keep the state transitions localized around the relevant handlers so only the triggered action shows a spinner.web/app/superadmin/whitelist/page.tsx (2)
95-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against out-of-order responses in
loadWhitelist.There's no cancellation or request-sequencing guard. Rapid
page/searchTermchanges (e.g., quick pagination clicks) can resolve fetches out of order and render stale results. The 350 ms debounce only mitigates typing, not pagination. Consider aborting the prior request or ignoring stale responses.♻️ Ignore stale responses via a request-id guard
+ const requestIdRef = useRef(0); + const loadWhitelist = useCallback(async () => { + const requestId = ++requestIdRef.current; setLoading(true); try { const params = new URLSearchParams({ page: String(page), limit: String(PAGE_SIZE), }); if (searchTerm) params.set("q", searchTerm); const data = await apiFetch<WhitelistListResponse>(`/api/whitelist?${params.toString()}`); + if (requestId !== requestIdRef.current) return; setEmails(data.emails); setTotal(data.pagination.total); setTotalPages(Math.max(1, data.pagination.totalPages)); } catch (err) { + if (requestId !== requestIdRef.current) return; const message = err instanceof ApiError ? err.message : "Không tải được whitelist"; notifications.show({ title: "Lỗi", message, color: "red" }); setEmails([]); setTotal(0); setTotalPages(1); } finally { + if (requestId === requestIdRef.current) setLoading(false); - setLoading(false); } }, [page, searchTerm]);Add
useRefto the React import.🤖 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 `@web/app/superadmin/whitelist/page.tsx` around lines 95 - 117, Guard `loadWhitelist` against stale out-of-order responses by adding a request-sequencing check in `page.tsx`. Introduce a `useRef`-based request id (or abort controller) near `loadWhitelist`, increment it each time the callback runs, and only call `setEmails`, `setTotal`, `setTotalPages`, and `setLoading(false)` for the latest request. Update the React import to include `useRef`, and keep the existing `useCallback` dependencies aligned with the new guard.
152-180: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDeleting the last row on the last page leaves an empty page.
onConfirmreloads with the currentpage. If you delete the only remaining item on the last page,pagenow exceedstotalPagesand the table shows the empty state until the user manually navigates back. Consider steppingpageback when the current page becomes empty after deletion.🤖 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 `@web/app/superadmin/whitelist/page.tsx` around lines 152 - 180, Deleting the last item on the current last page can leave `page` pointing past the new `totalPages`, so the whitelist table stays empty after `handleDeleteEmail` completes. Update the delete flow in `handleDeleteEmail`’s `onConfirm` to reload data and, if the current page becomes empty and is greater than 1, step the page back before calling `loadWhitelist()`. Use the existing `page`, `setPage`, and `loadWhitelist` state/helpers in `web/app/superadmin/whitelist/page.tsx` so the table always lands on a valid page after deletion.web/app/register-teacher/page.tsx (1)
80-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPublic unauthenticated endpoint — confirm backend abuse protection.
The TODO correctly notes this
/api/auth-admin/lecturer-requestPOST is publicly reachable with no auth. Ensure rate-limiting/CAPTCHA is enforced server-side before release to prevent spam/enumeration; client-side validation alone is insufficient.Want me to open an issue to track adding rate-limiting/CAPTCHA on this endpoint?
🤖 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 `@web/app/register-teacher/page.tsx` around lines 80 - 87, The public POST flow in register-teacher page uses apiFetch to call /api/auth-admin/lecturer-request with no auth, so the backend endpoint must be hardened. Add server-side abuse protection in the lecturer-request handler by enforcing rate limiting and CAPTCHA verification (and any related anti-spam checks) before accepting SubmitLecturerRequestResponse submissions; do not rely on client-side validation alone. Use the /api/auth-admin/lecturer-request route and its backend request handler as the place to implement and validate this protection.
🤖 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 `@plans/20260708-superadmin-real-api-integration/plan.md`:
- Around line 83-86: The dashboard metrics list currently duplicates GET
/api/whitelist?limit=1, leaving only two distinct data sources. Update the
metric mapping in the plan so the repeated whitelist entry is replaced with the
intended third stat source, keeping authClient.admin.listUsers({
filterField:"role", filterValue:"ADMIN" }) and pagination.total entries unique
and adding the missing dashboard metric alongside them.
- Line 80: The fenced blocks in the plan document are unlabeled, leaving the
markdown lint rule open. Update the two fenced sections in the planning content
to use explicit language tags: label the diagram fence in the diagram section
with mermaid, and label the phase-list fence with text (or remove that second
fence entirely). Use the existing fenced-block content in the plan to locate and
adjust both blocks so all fenced code sections are properly annotated.
- Around line 109-110: The Phase 5 lecturer-requests decision is duplicated and
contradictory across the plan, so consolidate it into one clear source of truth.
Update the Phase 5 section and the checklist in the plan so the decision about
whether to add the lecturer-requests nav/page is stated only once, and make sure
the references around the Phase 5 heading and the lecturer-requests bullet all
match that single decision.
In `@web/app/login/page.tsx`:
- Around line 74-86: The Google sign-in flow in handleGoogleLogin currently
sends users back to the login page via callbackURL, but LoginPage never
redirects authenticated users onward. Update the login page component to detect
an existing session on mount and route users to the correct portal, or change
authClient.signIn.social’s callbackURL to a dedicated post-auth route that
performs the role-based redirect. Use the LoginPage component and
handleGoogleLogin as the main points to fix.
In `@web/app/superadmin/admins/`[id]/page.tsx:
- Around line 295-299: The impersonation flow in the admin page navigates before
the auth session refresh completes because refetchSession is fire-and-forget via
AuthContext. Update the impersonation handler in the page component to await the
session refresh promise before calling router.push, either by returning the
underlying refetch() from refetchSession or invoking refetch() directly here, so
ProtectedRoute sees the updated session before redirecting.
In `@web/app/superadmin/admins/page.tsx`:
- Around line 91-129: The loadUsers callback in admins/page.tsx is still doing
client-side role filtering after fetching a fixed 500-user batch, so pages
beyond that window never surface all matching admins. Update the
authClient.admin.listUsers query path to apply roleFilter server-side (or via an
equivalent API-supported filter) and keep pagination consistent with
page/PAGE_SIZE instead of slicing the in-memory list for non-ALL roles. Make
sure the searchTerm flow and the existing loadUsers callback continue to use the
filtered query while preserving the current error handling and setUsers/setTotal
updates.
In `@web/app/superadmin/page.tsx`:
- Around line 83-96: The dashboard admin count is being computed from a capped
`authClient.admin.listUsers({ limit: "500" })` result, so `totalAdmins` can
undercount as the user base grows. Update the `Promise.all` flow in
`SuperadminPage` to either fetch all users or use a dedicated count-based
approach for admins, and do not rely on the 500-item page for `usersListRes`.
Also handle `usersListRes.error` explicitly, similar to `usersPageRes.error`,
instead of silently defaulting `users` to an empty array.
In `@web/app/superadmin/whitelist/page.tsx`:
- Around line 311-318: The search field in the superadmin whitelist page lacks
an accessible name because the TextInput only uses placeholder text. Update the
TextInput used in the whitelist search UI to include an explicit accessible
name, either by adding an aria-label or by providing a visually hidden label, so
screen readers can identify it reliably.
In `@web/lib/auth-client.ts`:
- Around line 39-46: The fetch options in auth-client’s request helper are
spreading init after the default settings, which allows caller-provided
credentials and headers to overwrite the helper’s defaults. Update the request
construction in the fetch wrapper so the init spread happens before the default
credentials and merged headers, or assign the defaults after init, ensuring
credentials: include and the JSON Content-Type/header merge always remain in
effect.
---
Nitpick comments:
In `@web/app/register-teacher/page.tsx`:
- Around line 80-87: The public POST flow in register-teacher page uses apiFetch
to call /api/auth-admin/lecturer-request with no auth, so the backend endpoint
must be hardened. Add server-side abuse protection in the lecturer-request
handler by enforcing rate limiting and CAPTCHA verification (and any related
anti-spam checks) before accepting SubmitLecturerRequestResponse submissions; do
not rely on client-side validation alone. Use the
/api/auth-admin/lecturer-request route and its backend request handler as the
place to implement and validate this protection.
In `@web/app/superadmin/admins/`[id]/page.tsx:
- Line 82: The single actionLoading boolean is shared by all admin actions in
the Admin page, so every button reflects loading at once. Update the state in
the page component that renders the ban/unban, impersonate, remove, and save
role actions to track the active action by key (for example, a string or null)
instead of one boolean, and wire each button’s loading prop to compare against
its own action name. Keep the state transitions localized around the relevant
handlers so only the triggered action shows a spinner.
In `@web/app/superadmin/lecturer-requests/page.tsx`:
- Around line 84-105: The current loadRequests flow in LecturerRequestsPage uses
the same loading state for both initial page load and post-approve/reject
refreshes, which forces the whole table view to disappear. Split this into a
dedicated refreshing state in the LecturerRequestsPage component and keep
loading for the first fetch only, then have loadRequests set the subtle refresh
state when reloading after actions so the Tabs, counts, and table remain visible
with a lighter indicator instead of the full-page Loader.
- Around line 303-375: The empty state for filteredRequests is currently shown
outside Table.Tbody, leaving an empty table body when there are no results.
Update the lecturer requests table in page.tsx so the Table, Table.Tbody, and
the “Không có yêu cầu nào phù hợp” empty message are conditionally rendered
based on filteredRequests.length, using the existing filteredRequests map and
the surrounding Table/Card structure. Keep the normal rows rendering in
Table.Tbody when data exists, and render the empty state instead of the table
when there are no matching requests.
In `@web/app/superadmin/whitelist/page.tsx`:
- Around line 95-117: Guard `loadWhitelist` against stale out-of-order responses
by adding a request-sequencing check in `page.tsx`. Introduce a `useRef`-based
request id (or abort controller) near `loadWhitelist`, increment it each time
the callback runs, and only call `setEmails`, `setTotal`, `setTotalPages`, and
`setLoading(false)` for the latest request. Update the React import to include
`useRef`, and keep the existing `useCallback` dependencies aligned with the new
guard.
- Around line 152-180: Deleting the last item on the current last page can leave
`page` pointing past the new `totalPages`, so the whitelist table stays empty
after `handleDeleteEmail` completes. Update the delete flow in
`handleDeleteEmail`’s `onConfirm` to reload data and, if the current page
becomes empty and is greater than 1, step the page back before calling
`loadWhitelist()`. Use the existing `page`, `setPage`, and `loadWhitelist`
state/helpers in `web/app/superadmin/whitelist/page.tsx` so the table always
lands on a valid page after deletion.
🪄 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: c1488593-32d9-41e7-b61f-0229e7e400a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
plans/20260708-superadmin-real-api-integration/plan.mdweb/app/contexts/AuthContext.tsxweb/app/login/page.tsxweb/app/register-teacher/page.tsxweb/app/student/layout.tsxweb/app/superadmin/admins/[id]/page.tsxweb/app/superadmin/admins/page.tsxweb/app/superadmin/layout.tsxweb/app/superadmin/lecturer-requests/page.tsxweb/app/superadmin/page.tsxweb/app/superadmin/whitelist/page.tsxweb/app/teacher/layout.tsxweb/components/ProtectedRoute.tsxweb/lib/auth-client.ts
|
|
||
| ## Sitemap & Endpoint Map | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the fenced blocks.
Both fenced blocks are unlabeled, which keeps MD040 open. Use mermaid for the diagram and text (or remove the second fence) for the phase list.
♻️ Suggested fix
-```
+```mermaid
...
-```
+```textAlso applies to: 312-314
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@plans/20260708-superadmin-real-api-integration/plan.md` at line 80, The
fenced blocks in the plan document are unlabeled, leaving the markdown lint rule
open. Update the two fenced sections in the planning content to use explicit
language tags: label the diagram fence in the diagram section with mermaid, and
label the phase-list fence with text (or remove that second fence entirely). Use
the existing fenced-block content in the plan to locate and adjust both blocks
so all fenced code sections are properly annotated.
Source: Linters/SAST tools
Comment on lines
+83
to
+86
| ├─ authClient.admin.listUsers({ filterField:"role", filterValue:"ADMIN" }) → total admin | ||
| ├─ GET /api/whitelist?limit=1 → pagination.total | ||
| └─ GET /api/whitelist?limit=1 → pagination.total (§4.1) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the missing third dashboard metric.
The sitemap repeats /api/whitelist?limit=1 twice, so this only defines two distinct data sources. Replace the duplicate with the third stat source intended for the dashboard.
🤖 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 `@plans/20260708-superadmin-real-api-integration/plan.md` around lines 83 - 86,
The dashboard metrics list currently duplicates GET /api/whitelist?limit=1,
leaving only two distinct data sources. Update the metric mapping in the plan so
the repeated whitelist entry is replaced with the intended third stat source,
keeping authClient.admin.listUsers({ filterField:"role", filterValue:"ADMIN" })
and pagination.total entries unique and adding the missing dashboard metric
alongside them.
Comment on lines
+109
to
+110
| > **Lecturer requests:** Không có trong `00_auth.md` → **không** thêm nav/trang Phase 5. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='plans/20260708-superadmin-real-api-integration/plan.md'
wc -l "$file"
printf '\n--- 100-120 ---\n'
sed -n '100,120p' "$file"
printf '\n--- 190-205 ---\n'
sed -n '190,205p' "$file"
printf '\n--- 235-245 ---\n'
sed -n '235,245p' "$file"Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 1999
Unify the Phase 5 lecturer-requests decision. Around lines 109-110, 196-202, and 240-241, the plan says not to add the nav/page, then adds Phase 5 — Lecturer Requests, then cancels it. Collapse this into one explicit decision so the checklist has a single source of truth.
🤖 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 `@plans/20260708-superadmin-real-api-integration/plan.md` around lines 109 -
110, The Phase 5 lecturer-requests decision is duplicated and contradictory
across the plan, so consolidate it into one clear source of truth. Update the
Phase 5 section and the checklist in the plan so the decision about whether to
add the lecturer-requests nav/page is stated only once, and make sure the
references around the Phase 5 heading and the lecturer-requests bullet all match
that single decision.
Comment on lines
+74
to
86
| const handleGoogleLogin = async () => { | ||
| setIsLoading(true); | ||
| setErrorMsg(""); | ||
| try { | ||
| const success = await loginAsRole(role); | ||
| if (success) { | ||
| if (role === "superadmin") router.push("/superadmin"); | ||
| else if (role === "teacher") router.push("/teacher"); | ||
| else router.push("/student"); | ||
| } | ||
| } catch (err) { | ||
| setErrorMsg("Đã xảy ra lỗi. Vui lòng thử lại."); | ||
| } finally { | ||
| await authClient.signIn.social({ | ||
| provider: "google", | ||
| callbackURL: `${window.location.origin}/login`, | ||
| }); | ||
| } catch { | ||
| setErrorMsg("Đã xảy ra lỗi khi đăng nhập Google."); | ||
| setIsLoading(false); | ||
| setShowRoleModal(false); | ||
| } | ||
| }; | ||
|
|
||
| const handleGoogleLogin = async (role: UserRole) => { | ||
| setIsLoading(true); | ||
| setErrorMsg(""); | ||
| try { | ||
| // Demo login using a default email | ||
| const demoEmail = role === "teacher" ? "lecturer1@fpt.edu.vn" : "student1@fpt.edu.vn"; | ||
| const success = await loginWithGoogle(demoEmail, role); | ||
| if (success) { | ||
| if (role === "teacher") router.push("/teacher"); | ||
| else router.push("/student"); | ||
| } | ||
| } catch (err) { | ||
| setErrorMsg("Đã xảy ra lỗi. Vui lòng thử lại."); | ||
| } finally { | ||
| setIsLoading(false); | ||
| setShowRoleModal(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for auth/session redirect logic.
git ls-files web/app/login/page.tsx web/app -z | tr '\0' '\n'
echo '--- outline page.tsx ---'
ast-grep outline web/app/login/page.tsx --view expanded || true
echo '--- search redirect/session refs ---'
rg -n "redirectAfterAuth|getSession|session|signIn\.social|callbackURL|router\.push|useEffect" web/app/login/page.tsx web/app -g '!**/node_modules/**' || trueRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 16917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the login page and any helper used for post-auth routing.
for f in web/app/login/page.tsx web/app/**/redirectAfterAuth* web/app/**/auth*; do
[ -f "$f" ] && echo "### $f" && wc -l "$f"
done
echo '--- page.tsx lines 1-220 ---'
sed -n '1,220p' web/app/login/page.tsx
echo '--- candidate auth redirect helpers ---'
rg -n "function redirectAfterAuth|const redirectAfterAuth|export .*redirectAfterAuth|redirectAfterAuth" web/app -g '!**/node_modules/**' || trueRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 6810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the auth redirect implementation and related login/portal routes.
rg -n "redirectAfterAuth|portal|login" web/app -g '!**/node_modules/**' || trueRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 2660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- web/app/login/page.tsx lines 220-265 ---'
sed -n '220,265p' web/app/login/page.tsx
echo '--- web/app/contexts/AuthContext.tsx outline ---'
ast-grep outline web/app/contexts/AuthContext.tsx --view expanded || true
echo '--- web/app/contexts/AuthContext.tsx lines 1-180 ---'
sed -n '1,180p' web/app/contexts/AuthContext.tsxRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 5401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the login page tail for any mount-time redirect and the auth context for session side effects.
sed -n '220,265p' web/app/login/page.tsx
echo '---'
sed -n '1,180p' web/app/contexts/AuthContext.tsxRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 4371
Google OAuth still lands on /login without a redirect. callbackURL returns users to the login page, but this component never routes an authenticated session to the correct portal, so successful Google sign-ins can leave users stranded here. Add a mount-time redirect or point the callback at a route that redirects by role.
🤖 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 `@web/app/login/page.tsx` around lines 74 - 86, The Google sign-in flow in
handleGoogleLogin currently sends users back to the login page via callbackURL,
but LoginPage never redirects authenticated users onward. Update the login page
component to detect an existing session on mount and route users to the correct
portal, or change authClient.signIn.social’s callbackURL to a dedicated
post-auth route that performs the role-based redirect. Use the LoginPage
component and handleGoogleLogin as the main points to fix.
Comment on lines
+295
to
+299
| const res = await authClient.admin.impersonateUser({ userId }); | ||
| if (res.error) throw new Error(res.error.message); | ||
| refetchSession(); | ||
| const role = (detail?.role as UserRole) ?? "STUDENT"; | ||
| router.push(portalPathForRole(role)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target page component and nearby symbols
ast-grep outline 'web/app/superadmin/admins/[id]/page.tsx' --view expanded || true
# Read the relevant slice around the reported lines
sed -n '250,340p' 'web/app/superadmin/admins/[id]/page.tsx'
# Find refetchSession definitions/usages
rg -n "refetchSession|AuthContext|ProtectedRoute|portalPathForRole" web -S
# Inspect likely auth/session implementation files if found by ripgrepRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 6528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the auth context/provider
sed -n '1,220p' 'web/app/contexts/AuthContext.tsx'
# Read the protected route implementation
sed -n '1,220p' 'web/components/ProtectedRoute.tsx'
# Read the relevant portion of the admin detail page with the impersonation handler context
sed -n '260,310p' 'web/app/superadmin/admins/[id]/page.tsx'Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 5735
Await the session refresh before pushing the portal. refetchSession() is fire-and-forget here (AuthContext wraps refetch() in void), so the impersonation flow can navigate while ProtectedRoute still sees the admin session and redirects back. Return the refetch promise from refetchSession (or call refetch() directly here) and await it before router.push(...).
🤖 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 `@web/app/superadmin/admins/`[id]/page.tsx around lines 295 - 299, The
impersonation flow in the admin page navigates before the auth session refresh
completes because refetchSession is fire-and-forget via AuthContext. Update the
impersonation handler in the page component to await the session refresh promise
before calling router.push, either by returning the underlying refetch() from
refetchSession or invoking refetch() directly here, so ProtectedRoute sees the
updated session before redirecting.
Comment on lines
+91
to
+129
| const loadUsers = useCallback(async () => { | ||
| setLoading(true); | ||
| try { | ||
| const searchField = searchTerm.includes("@") ? "email" : "name"; | ||
| const baseQuery: Record<string, string> = { | ||
| limit: roleFilter === "ALL" ? String(PAGE_SIZE) : "500", | ||
| offset: roleFilter === "ALL" ? String((page - 1) * PAGE_SIZE) : "0", | ||
| }; | ||
| if (searchTerm) { | ||
| baseQuery.searchValue = searchTerm; | ||
| baseQuery.searchField = searchField; | ||
| } | ||
|
|
||
| const res = await authClient.admin.listUsers({ query: baseQuery }); | ||
| if (res.error) { | ||
| throw new Error(res.error.message ?? "Không tải được danh sách người dùng"); | ||
| } | ||
|
|
||
| let list = (res.data?.users ?? []) as AdminListUser[]; | ||
| let count = res.data?.total ?? list.length; | ||
|
|
||
| if (roleFilter !== "ALL") { | ||
| list = list.filter((u) => u.role === roleFilter); | ||
| count = list.length; | ||
| const start = (page - 1) * PAGE_SIZE; | ||
| list = list.slice(start, start + PAGE_SIZE); | ||
| } | ||
|
|
||
| setUsers(list); | ||
| setTotal(count); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "Lỗi tải danh sách"; | ||
| notifications.show({ title: "Lỗi", message, color: "red" }); | ||
| setUsers([]); | ||
| setTotal(0); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [page, searchTerm, roleFilter]); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Role-filtered paging still truncates at 500 users
When roleFilter !== "ALL", this path loads only the first 500 users, filters them client-side, and paginates that slice. Any matching users beyond that window will never appear, and every page change re-fetches the same batch. Use server-side role filtering/pagination here instead of fetching a fixed 500.
🤖 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 `@web/app/superadmin/admins/page.tsx` around lines 91 - 129, The loadUsers
callback in admins/page.tsx is still doing client-side role filtering after
fetching a fixed 500-user batch, so pages beyond that window never surface all
matching admins. Update the authClient.admin.listUsers query path to apply
roleFilter server-side (or via an equivalent API-supported filter) and keep
pagination consistent with page/PAGE_SIZE instead of slicing the in-memory list
for non-ALL roles. Make sure the searchTerm flow and the existing loadUsers
callback continue to use the filtered query while preserving the current error
handling and setUsers/setTotal updates.
Comment on lines
+83
to
+96
| const [usersPageRes, whitelistRes, usersListRes] = await Promise.all([ | ||
| authClient.admin.listUsers({ query: { limit: "1", offset: "0" } }), | ||
| apiFetch<WhitelistListResponse>("/api/whitelist?page=1&limit=1"), | ||
| authClient.admin.listUsers({ query: { limit: "500", offset: "0" } }), | ||
| ]); | ||
|
|
||
| if (usersPageRes.error) { | ||
| throw new Error(usersPageRes.error.message ?? "Không tải được danh sách người dùng"); | ||
| } | ||
|
|
||
| const totalUsers = usersPageRes.data?.total ?? 0; | ||
| const users = usersListRes.data?.users ?? []; | ||
| const totalAdmins = users.filter((u) => u.role === "ADMIN").length; | ||
| const totalWhitelist = whitelistRes.pagination.total; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the referenced lines.
sed -n '1,180p' web/app/superadmin/page.tsx
printf '\n--- SEARCH listUsers USAGE ---\n'
rg -n "listUsers\\(|filterField|filterValue|filterOperator|role === \"ADMIN\"|totalAdmins" web -S
printf '\n--- FILE LIST FOR AUTH CLIENT REFERENCES ---\n'
rg -n "authClient" web -SRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 8678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to find local docs or type definitions for the auth client.
rg -n "filterField|filterValue|filterOperator|listUsers" . -S --glob '!**/node_modules/**' --glob '!**/.next/**' --glob '!**/dist/**'
printf '\n--- candidate package metadata ---\n'
git ls-files | rg 'package\.json$|README|docs|auth|better' -nRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 4725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local auth docs and any Better Auth references that describe listUsers behavior.
sed -n '1,220p' docs/technical/backend/better_auth_guide.md
printf '\n---\n'
rg -n "filterField|filterValue|filterOperator|listUsers|total" docs api web -S --glob '!**/node_modules/**' --glob '!**/.next/**' --glob '!**/dist/**'Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 13136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the auth client wiring and any API-side auth module for listUsers-related behavior.
sed -n '1,220p' web/lib/auth-client.ts
printf '\n--- API auth module ---\n'
sed -n '1,240p' api/src/modules/auth/auth.ts
printf '\n--- Better auth test script if present ---\n'
sed -n '1,220p' api/src/scripts/test-better-auth.tsRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 7905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the auth API docs around listUsers/admin filtering.
sed -n '340,560p' docs/api/00_auth.md
printf '\n--- search for admin listUsers examples ---\n'
rg -n "filterField|filterValue|filterOperator|listUsers|total admin|role" docs/api/00_auth.md docs/technical/backend/better_auth_guide.md plans/20260708-superadmin-real-api-integration/plan.md -SRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 11740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the admin user management page for any existing count pattern and error handling.
sed -n '1,220p' web/app/superadmin/admins/page.tsxRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 6255
Admin count should not be capped at 500 users
totalAdminsis derived fromlistUsers({ limit: "500" }), so the dashboard will undercount once the user base grows past 500.- Handle the second
listUserserror explicitly; right now it falls back to0without surfacing the failure.
🤖 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 `@web/app/superadmin/page.tsx` around lines 83 - 96, The dashboard admin count
is being computed from a capped `authClient.admin.listUsers({ limit: "500" })`
result, so `totalAdmins` can undercount as the user base grows. Update the
`Promise.all` flow in `SuperadminPage` to either fetch all users or use a
dedicated count-based approach for admins, and do not rely on the 500-item page
for `usersListRes`. Also handle `usersListRes.error` explicitly, similar to
`usersPageRes.error`, instead of silently defaulting `users` to an empty array.
Comment on lines
311
to
318
| <TextInput | ||
| placeholder="Tìm kiếm email sinh viên..." | ||
| value={searchTerm} | ||
| onChange={(e) => setSearchTerm(e.target.value)} | ||
| value={searchInput} | ||
| onChange={(e) => setSearchInput(e.target.value)} | ||
| leftSection={<IconSearch size={16} color="#9CA3AF" />} | ||
| radius={0} | ||
| style={{ maxWidth: "400px" }} | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an accessible name to the search input.
The TextInput relies solely on placeholder, which is not a reliable accessible name for screen readers. Add aria-label (or a visually-hidden label).
🛡️ Suggested fix
<TextInput
placeholder="Tìm kiếm email sinh viên..."
+ aria-label="Tìm kiếm email sinh viên"
value={searchInput}
onChange={(e) => setSearchInput(e.target.value)}📝 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
| <TextInput | |
| placeholder="Tìm kiếm email sinh viên..." | |
| value={searchTerm} | |
| onChange={(e) => setSearchTerm(e.target.value)} | |
| value={searchInput} | |
| onChange={(e) => setSearchInput(e.target.value)} | |
| leftSection={<IconSearch size={16} color="#9CA3AF" />} | |
| radius={0} | |
| style={{ maxWidth: "400px" }} | |
| /> | |
| <TextInput | |
| placeholder="Tìm kiếm email sinh viên..." | |
| aria-label="Tìm kiếm email sinh viên" | |
| value={searchInput} | |
| onChange={(e) => setSearchInput(e.target.value)} | |
| leftSection={<IconSearch size={16} color="`#9CA3AF`" />} | |
| radius={0} | |
| style={{ maxWidth: "400px" }} | |
| /> |
🤖 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 `@web/app/superadmin/whitelist/page.tsx` around lines 311 - 318, The search
field in the superadmin whitelist page lacks an accessible name because the
TextInput only uses placeholder text. Update the TextInput used in the whitelist
search UI to include an explicit accessible name, either by adding an aria-label
or by providing a visually hidden label, so screen readers can identify it
reliably.
Comment on lines
+39
to
+46
| res = await fetch(`${apiBaseUrl}${path}`, { | ||
| credentials: "include", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| ...init?.headers, | ||
| }, | ||
| ...init, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C4 '\bapiFetch\s*<' webRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 5614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web/lib/auth-client.ts =="
cat -n web/lib/auth-client.ts | sed -n '1,120p'
echo
echo "== apiFetch call sites with headers/credentials =="
rg -n -C3 'apiFetch\s*<|apiFetch\s*\(' web/app web/lib | sed -n '/apiFetch/,$p' | head -n 200
echo
echo "== explicit headers/credentials near apiFetch =="
rg -n -C2 'apiFetch\s*\(.*\{|headers\s*:|credentials\s*:' web/app web/libRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 10719
Spread init before the defaults. ...init currently overwrites credentials: "include" and the merged headers object if a caller passes either field. Move the spread earlier, or set headers after ...init so this helper keeps its defaults.
🤖 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 `@web/lib/auth-client.ts` around lines 39 - 46, The fetch options in
auth-client’s request helper are spreading init after the default settings,
which allows caller-provided credentials and headers to overwrite the helper’s
defaults. Update the request construction in the fetch wrapper so the init
spread happens before the default credentials and merged headers, or assign the
defaults after init, ensuring credentials: include and the JSON
Content-Type/header merge always remain in effect.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/config/supabase.ts (1)
8-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
persistSession: falseon the server-side anon client.
createSupabaseClient()is a server-side factory but does not disable session persistence. The service client correctly setspersistSession: false. Without it, the Supabase client may attempt to persist sessions in memory, leading to leaks and unexpected behavior in server contexts.♻️ Proposed fix
export function createSupabaseClient(): SupabaseClient { if (!supabaseUrl || !supabaseAnonKey) { throw new Error("Missing Supabase URL or anon key"); } - return createClient(supabaseUrl, supabaseAnonKey); + return createClient(supabaseUrl, supabaseAnonKey, { + auth: { persistSession: false, autoRefreshToken: false }, + }); }🤖 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 `@api/src/config/supabase.ts` around lines 8 - 13, The server-side Supabase factory in createSupabaseClient() is missing the anon client option that disables session persistence. Update the createClient call to pass an auth/config setting with persistSession disabled, matching the service client behavior, so the anon client does not try to retain sessions in server contexts.
🤖 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.
Nitpick comments:
In `@api/src/config/supabase.ts`:
- Around line 8-13: The server-side Supabase factory in createSupabaseClient()
is missing the anon client option that disables session persistence. Update the
createClient call to pass an auth/config setting with persistSession disabled,
matching the service client behavior, so the anon client does not try to retain
sessions in server contexts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfb7af09-c9fc-492e-9a1d-7036dcc889b4
📒 Files selected for processing (2)
api/src/config/supabase.tsweb/lib/supabase-client.ts
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
web/app/teacher/documents/page.tsx (1)
339-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDropzone doesn't enforce PDF-only — validation happens too late.
The Dropzone accepts any file type. Users can drop a non-PDF file, see it listed, click "Bắt đầu upload", and only then receive an error. Add an
acceptprop to reject non-PDF files at drop time, providing immediate feedback.♻️ Proposed fix: add accept prop to Dropzone
<Dropzone onDrop={(acceptedFiles) => setFiles(acceptedFiles)} maxSize={50 * 1024 ** 2} + accept={{ "application/pdf": [".pdf"] }} radius={0}🤖 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 `@web/app/teacher/documents/page.tsx` around lines 339 - 354, Configure the Dropzone component in the documents page to accept only PDF files by adding its accept prop with the appropriate PDF MIME type. Keep the existing onDrop handler and size limit unchanged so non-PDF files are rejected immediately during selection or drop.web/lib/api.ts (1)
259-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
uploadSyllabusDocumentduplicates error-handling logic fromfetchApi.The manual
fetch+ error-parsing block is a near-verbatim copy offetchApilines 37–46. Consider extendingfetchApito skipContent-Type: application/jsonwhen the body isFormData, then routing through it to eliminate the duplication.♻️ Proposed refactor: extend fetchApi to support FormData
async function fetchApi<T>( path: string, options: RequestInit = {} ): Promise<T> { const url = `${API_BASE_URL}${path}`; + const isFormData = options.body instanceof FormData; const res = await fetch(url, { ...options, credentials: "include", headers: { - "Content-Type": "application/json", + ...(isFormData ? {} : { "Content-Type": "application/json" }), ...options.headers, }, });Then
uploadSyllabusDocumentcan simply callfetchApi:export async function uploadSyllabusDocument( syllabusId: number, file: File ): Promise<{ document: ApiDocument }> { - const url = `${API_BASE_URL}/api/syllabus/${syllabusId}/documents`; const formData = new FormData(); formData.append("file", file); - - const res = await fetch(url, { - method: "POST", - credentials: "include", - body: formData, - }); - - if (!res.ok) { - let errorMessage = `API Error: ${res.status}`; - try { - const body = await res.json(); - errorMessage = body.error || errorMessage; - } catch { - // ignore parse errors - } - throw new ApiError(errorMessage, res.status); - } - - return res.json(); + return fetchApi(`/api/syllabus/${syllabusId}/documents`, { + method: "POST", + body: formData, + }); }🤖 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 `@web/lib/api.ts` around lines 259 - 285, Refactor uploadSyllabusDocument to use the shared fetchApi helper instead of duplicating fetch and error parsing. Update fetchApi to omit the application/json Content-Type header when its request body is FormData, while preserving JSON handling for other bodies, then pass the FormData request through fetchApi and return its parsed response.
🤖 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 `@api/src/modules/rag/services/rag.service.ts`:
- Around line 188-194: In the Gemini fallback catch block of RagService, keep
logging geminiError server-side but remove its message from the client-facing
errorReply. Return only the generic apology through onChunk and fullAnswer,
without exposing provider or internal error details.
In `@credentials.json`:
- Line 6: Remove plaintext passwords and user IDs from tracked credentials.json;
move seeded credentials to an ignored local fixture or generate them through
environment-backed test fixtures, ensuring student1@fpt.edu.vn does not use the
trivial password and adding the relevant file to gitignore.
In `@web/app/student/syllabus/`[subjectCode]/page.tsx:
- Around line 117-119: Remove the "FER202" fallback in SyllabusViewerPage and
preserve an undefined or empty subjectCode when params.subjectCode is missing;
update loadSyllabus to handle the missing code explicitly or render an
appropriate error state before loading syllabus data.
- Around line 192-225: Update the not-found rendering condition in the syllabus
page so API failures display a primary backend/server error message instead of
“Không tìm thấy Syllabus.” Within the error block, use error === "API_ERROR" to
select the appropriate title and supporting text while preserving the existing
not-found messaging for genuine missing syllabi.
In `@web/app/teacher/documents/page.tsx`:
- Around line 141-152: handleDelete currently calls api.deleteSyllabusDocument
without user confirmation, allowing accidental permanent deletion. Add a
confirmation step at the start of handleDelete, using the existing UI
conventions or window.confirm, and return early when the user cancels; only call
api.deleteSyllabusDocument and reload via loadDocuments after confirmation.
In `@web/app/teacher/syllabus/page.tsx`:
- Line 65: Replace the incorrect top-level syllabus.code references in
filteredSyllabi and the Subject Code table cell with syllabus.course.code,
matching the ApiSyllabusSummary shape and existing course.name/course.code
usage.
In `@web/components/chatbot/ChatbotWidget.tsx`:
- Around line 141-151: The onError handler in ChatbotWidget must always expose
failures, including when partial bot content already exists. Update the message
mapping for botMsgId so it preserves existing m.content and appends a clear
error indicator containing error, rather than using the fallback-only expression
m.content || `⚠️ Lỗi: ${error}`.
- Around line 85-177: Add a useEffect cleanup for the ChatbotWidget component
that aborts any active request in abortRef.current and clears the ref on
unmount. Ensure this cleanup covers streams started by the sendChatMessageStream
flow and prevents callbacks from continuing after the component is removed.
- Around line 100-108: Prevent the empty bot bubble and loading indicator from
rendering together by deferring insertion of the empty bot message in the
streaming handler until the first onChunk callback, then append or update it
with the received content; update the related state logic so subsequent chunks
continue targeting the same bot message ID.
In `@web/components/chatbot/subcomponents/ChatHistorySidebar.tsx`:
- Around line 45-62: Update ChatHistorySidebar’s session-selection flow to
delegate to an abort-aware onSessionSelect callback supplied by ChatbotWidget,
mirroring handleNewChat so any active stream is aborted and loading state is
reset before switching sessions. Ensure the callback replaces the local
handleSessionClick logic, and when the selected session has no messages,
explicitly clear the messages state before setting the new session ID.
In `@web/components/student/syllabus/SyllabusHelpers.tsx`:
- Around line 16-24: Remove the description-based getbootstrap.com/react.dev
conditional from isMaterialChecked, including its forced true handling for
is_main_material and is_online. Keep only the actual field-value checks and
return false when those values are not truthy.
In `@web/components/student/syllabus/SyllabusScheduleTab.tsx`:
- Around line 95-109: Replace the hardcoded FER202/PRN232 check in the syllabus
schedule rendering with a generalized course-code validation, such as matching
the API’s supported three-letter, three-digit format, or use a provided download
URL when available. Keep valid codes rendered as download Anchors with the
existing URL structure and render other values with formatMultiline.
In `@web/lib/api.ts`:
- Around line 369-405: Fix the SSE parsing in the reader loop by normalizing or
trimming trailing carriage returns from lines before checking prefixes and
parsing JSON, so CRLF-delimited events are handled correctly. After the read
loop exits, process any remaining non-empty buffer as a final SSE line, applying
the same data parsing and callbacks logic used inside the loop before invoking
callbacks.onDone().
In `@web/next.config.ts`:
- Around line 6-24: Update the top-level environment-loading try/catch in
next.config.ts to surface failures from process.loadEnvFile, including
unsupported Node versions and malformed .env files. Replace the empty catch with
a clear console error or rethrow that includes the captured error, while
preserving the existing upward .env search behavior.
---
Nitpick comments:
In `@web/app/teacher/documents/page.tsx`:
- Around line 339-354: Configure the Dropzone component in the documents page to
accept only PDF files by adding its accept prop with the appropriate PDF MIME
type. Keep the existing onDrop handler and size limit unchanged so non-PDF files
are rejected immediately during selection or drop.
In `@web/lib/api.ts`:
- Around line 259-285: Refactor uploadSyllabusDocument to use the shared
fetchApi helper instead of duplicating fetch and error parsing. Update fetchApi
to omit the application/json Content-Type header when its request body is
FormData, while preserving JSON handling for other bodies, then pass the
FormData request through fetchApi and return its parsed response.
🪄 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: c53766de-aabe-4e04-9db6-f287d7764c64
📒 Files selected for processing (27)
api/prisma/seed.tsapi/src/modules/rag/services/rag.service.tscredentials.jsonweb/app/contexts/AuthContext.tsxweb/app/login/page.tsxweb/app/student/layout.tsxweb/app/student/page.tsxweb/app/student/syllabus/[subjectCode]/page.tsxweb/app/teacher/documents/page.tsxweb/app/teacher/syllabus/page.tsxweb/components/ProtectedRoute.tsxweb/components/chatbot/ChatbotWidget.tsxweb/components/chatbot/subcomponents/ChatHistorySidebar.tsxweb/components/chatbot/subcomponents/ChatInputArea.tsxweb/components/chatbot/subcomponents/ChatMessageList.tsxweb/components/chatbot/subcomponents/types.tsweb/components/student/SearchResultRow.tsxweb/components/student/SubjectCard.tsxweb/components/student/subjectsData.tsweb/components/student/syllabus/SyllabusAssessmentTab.tsxweb/components/student/syllabus/SyllabusCLOsTab.tsxweb/components/student/syllabus/SyllabusHelpers.tsxweb/components/student/syllabus/SyllabusMaterialsTab.tsxweb/components/student/syllabus/SyllabusOverviewTab.tsxweb/components/student/syllabus/SyllabusScheduleTab.tsxweb/lib/api.tsweb/next.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/components/ProtectedRoute.tsx
- web/app/login/page.tsx
Comment on lines
+188
to
+194
| } catch (geminiError) { | ||
| console.error("[RagService] Gemini fallback also failed:", geminiError); | ||
| const errMsg = geminiError instanceof Error ? geminiError.message : String(geminiError); | ||
| const errorReply = `⚠️ Xin lỗi, hiện tại AI trợ lý đang gặp sự cố kỹ thuật. Vui lòng thử lại sau.\n\nChi tiết lỗi: ${errMsg}`; | ||
| onChunk(errorReply); | ||
| return { citations: [], fullAnswer: errorReply }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file lines =="
nl -ba api/src/modules/rag/services/rag.service.ts | sed -n '150,210p'
echo
echo "== onChunk references in rag service =="
rg -n "onChunk|errorReply|Gemini fallback|AnythingLLM" api/src/modules/rag/services/rag.service.ts
echo
echo "== call sites / definition of onChunk if present in module tree =="
rg -n "onChunk\s*[:(]|type .*onChunk|interface .*onChunk|function .*onChunk" api/src/modules/rag -g '*.ts'Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('api/src/modules/rag/services/rag.service.ts')
lines = p.read_text().splitlines()
for i in range(150, 211):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== onChunk / error handling matches =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('api/src/modules/rag/services/rag.service.ts')
text = p.read_text()
for pat in [r'onChunk', r'errorReply', r'Gemini fallback', r'AnythingLLM']:
print(f'-- {pat} --')
for i, line in enumerate(text.splitlines(), 1):
if re.search(pat, line):
print(f"{i}: {line}")
PY
echo
echo "== module-wide onChunk references =="
rg -n "onChunk|errorReply|Gemini fallback|AnythingLLM" api/src/modules/rag -g '*.ts'Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 7606
Don't stream raw Gemini error details to clients. geminiError.message is concatenated into the user-facing reply and sent through onChunk, which can expose internal provider details. Keep the server-side log, but return only the generic apology here. api/src/modules/rag/services/rag.service.ts:188-193
🤖 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 `@api/src/modules/rag/services/rag.service.ts` around lines 188 - 194, In the
Gemini fallback catch block of RagService, keep logging geminiError server-side
but remove its message from the client-facing errorReply. Return only the
generic apology through onChunk and fullAnswer, without exposing provider or
internal error details.
| "name": "Sinh vien Mot", | ||
| "email": "student1@fpt.edu.vn", | ||
| "password": "StudentPassword123!", | ||
| "password": "123456", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether credentials.json is git-ignored and where it is consumed
fd -H -t f 'credentials.json'
rg -n 'credentials\.json' -g '!credentials.json'
git check-ignore credentials.json || echo "NOT ignored by git"Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file locations =="
fd -H -t f 'credentials.json' .
echo
echo "== top-level gitignore entries =="
sed -n '1,200p' .gitignore 2>/dev/null || true
echo
echo "== credentials.json contents with line numbers =="
cat -n credentials.json
echo
echo "== references to credentials.json =="
rg -n --hidden --glob '!credentials.json' 'credentials\.json|StudentPassword123!|123456|fpt\.edu\.vn' .
echo
echo "== git ignore status =="
git check-ignore -v credentials.json || echo "NOT ignored by git"
echo
echo "== tracked status =="
git ls-files --stage -- credentials.json || echo "NOT tracked"Repository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 14114
Committed plaintext credentials. credentials.json:6 is tracked in git and not ignored, so the seeded passwords and user IDs live in the repo. student1@fpt.edu.vn also now uses the trivial 123456 password. Move this to ignored local data or generate it from environment-backed fixtures instead.
🤖 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 `@credentials.json` at line 6, Remove plaintext passwords and user IDs from
tracked credentials.json; move seeded credentials to an ignored local fixture or
generate them through environment-backed test fixtures, ensuring
student1@fpt.edu.vn does not use the trivial password and adding the relevant
file to gitignore.
Comment on lines
117
to
119
| export default function SyllabusViewerPage() { | ||
| const params = useParams(); | ||
| const subjectCode = (params.subjectCode as string)?.toUpperCase() || "FER202"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove hardcoded "FER202" fallback for missing subject code.
If params.subjectCode is undefined (e.g., navigated to /student/syllabus/ without a code), the page silently loads FER202 data instead of showing an error. This is confusing in production.
🛡️ Proposed fix
- const subjectCode = (params.subjectCode as string)?.toUpperCase() || "FER202";
+ const rawCode = params.subjectCode as string | undefined;
+ const subjectCode = rawCode?.toUpperCase() ?? "";Then handle the empty-string case in the loadSyllabus callback or render an error early.
📝 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
| export default function SyllabusViewerPage() { | |
| const params = useParams(); | |
| const subjectCode = (params.subjectCode as string)?.toUpperCase() || "FER202"; | |
| export default function SyllabusViewerPage() { | |
| const params = useParams(); | |
| const rawCode = params.subjectCode as string | undefined; | |
| const subjectCode = rawCode?.toUpperCase() ?? ""; |
🤖 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 `@web/app/student/syllabus/`[subjectCode]/page.tsx around lines 117 - 119,
Remove the "FER202" fallback in SyllabusViewerPage and preserve an undefined or
empty subjectCode when params.subjectCode is missing; update loadSyllabus to
handle the missing code explicitly or render an appropriate error state before
loading syllabus data.
Comment on lines
+192
to
225
| // Not found | ||
| if (error === "NOT_FOUND" || !syllabusData) { | ||
| return ( | ||
| <Container size="sm" py="xl" style={{ textAlign: "center" }}> | ||
| <Card p="xl" radius="lg" style={{ border: "1px dashed #fca5a5", background: "#fff5f5" }}> | ||
| <Stack align="center" gap="md"> | ||
| <ThemeIcon size={64} radius="xl" style={{ background: "#fee2e2", color: "#dc2626" }}> | ||
| <IconX size={32} /> | ||
| </ThemeIcon> | ||
| <Text fw={800} style={{ color: "#dc2626", fontSize: "18px" }}> | ||
| Không tìm thấy Syllabus | ||
| </Text> | ||
| <Text c="dimmed" size="sm"> | ||
| Môn học <strong>{subjectCode}</strong> chưa có trong hệ thống hoặc chưa được duyệt. | ||
| </Text> | ||
| {error === "API_ERROR" && ( | ||
| <Text c="dimmed" size="xs"> | ||
| <IconAlertCircle size={12} style={{ verticalAlign: "middle" }} /> Không thể kết nối đến server backend. | ||
| </Text> | ||
| )} | ||
| <Button | ||
| component={Link} | ||
| href="/student" | ||
| radius="md" | ||
| style={{ background: "linear-gradient(135deg, #1A3A5C, #0f2848)" }} | ||
| leftSection={<IconArrowLeft size={14} />} | ||
| > | ||
| Quay lại tìm kiếm | ||
| </Button> | ||
| </Stack> | ||
| </Card> | ||
| </Container> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
API errors show a misleading "Not found" primary message.
When error === "API_ERROR", syllabusData is null, so this block renders. The primary text says "Không tìm thấy Syllabus" (Not found) even though the real issue is a backend connection failure. The API error subtext at line 207-211 is shown conditionally, but the primary message is still wrong.
🛡️ Proposed fix
- <Text fw={800} style={{ color: "`#dc2626`", fontSize: "18px" }}>
- Không tìm thấy Syllabus
- </Text>
- <Text c="dimmed" size="sm">
- Môn học <strong>{subjectCode}</strong> chưa có trong hệ thống hoặc chưa được duyệt.
- </Text>
- {error === "API_ERROR" && (
- <Text c="dimmed" size="xs">
- <IconAlertCircle size={12} style={{ verticalAlign: "middle" }} /> Không thể kết nối đến server backend.
- </Text>
- )}
+ <Text fw={800} style={{ color: "`#dc2626`", fontSize: "18px" }}>
+ {error === "API_ERROR" ? "Lỗi kết nối" : "Không tìm thấy Syllabus"}
+ </Text>
+ <Text c="dimmed" size="sm">
+ {error === "API_ERROR"
+ ? "Không thể kết nối đến server backend. Vui lòng thử lại sau."
+ : <>Môn học <strong>{subjectCode}</strong> chưa có trong hệ thống hoặc chưa được duyệt.</>
+ }
+ </Text>📝 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
| // Not found | |
| if (error === "NOT_FOUND" || !syllabusData) { | |
| return ( | |
| <Container size="sm" py="xl" style={{ textAlign: "center" }}> | |
| <Card p="xl" radius="lg" style={{ border: "1px dashed #fca5a5", background: "#fff5f5" }}> | |
| <Stack align="center" gap="md"> | |
| <ThemeIcon size={64} radius="xl" style={{ background: "#fee2e2", color: "#dc2626" }}> | |
| <IconX size={32} /> | |
| </ThemeIcon> | |
| <Text fw={800} style={{ color: "#dc2626", fontSize: "18px" }}> | |
| Không tìm thấy Syllabus | |
| </Text> | |
| <Text c="dimmed" size="sm"> | |
| Môn học <strong>{subjectCode}</strong> chưa có trong hệ thống hoặc chưa được duyệt. | |
| </Text> | |
| {error === "API_ERROR" && ( | |
| <Text c="dimmed" size="xs"> | |
| <IconAlertCircle size={12} style={{ verticalAlign: "middle" }} /> Không thể kết nối đến server backend. | |
| </Text> | |
| )} | |
| <Button | |
| component={Link} | |
| href="/student" | |
| radius="md" | |
| style={{ background: "linear-gradient(135deg, #1A3A5C, #0f2848)" }} | |
| leftSection={<IconArrowLeft size={14} />} | |
| > | |
| Quay lại tìm kiếm | |
| </Button> | |
| </Stack> | |
| </Card> | |
| </Container> | |
| ); | |
| } | |
| // Not found | |
| if (error === "NOT_FOUND" || !syllabusData) { | |
| return ( | |
| <Container size="sm" py="xl" style={{ textAlign: "center" }}> | |
| <Card p="xl" radius="lg" style={{ border: "1px dashed `#fca5a5`", background: "`#fff5f5`" }}> | |
| <Stack align="center" gap="md"> | |
| <ThemeIcon size={64} radius="xl" style={{ background: "`#fee2e2`", color: "`#dc2626`" }}> | |
| <IconX size={32} /> | |
| </ThemeIcon> | |
| <Text fw={800} style={{ color: "`#dc2626`", fontSize: "18px" }}> | |
| {error === "API_ERROR" ? "Lỗi kết nối" : "Không tìm thấy Syllabus"} | |
| </Text> | |
| <Text c="dimmed" size="sm"> | |
| {error === "API_ERROR" | |
| ? "Không thể kết nối đến server backend. Vui lòng thử lại sau." | |
| : <>Môn học <strong>{subjectCode}</strong> chưa có trong hệ thống hoặc chưa được duyệt.</> | |
| } | |
| </Text> | |
| <Button | |
| component={Link} | |
| href="/student" | |
| radius="md" | |
| style={{ background: "linear-gradient(135deg, `#1A3A5C`, `#0f2848`)" }} | |
| leftSection={<IconArrowLeft size={14} />} | |
| > | |
| Quay lại tìm kiếm | |
| </Button> | |
| </Stack> | |
| </Card> | |
| </Container> | |
| ); | |
| } |
🤖 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 `@web/app/student/syllabus/`[subjectCode]/page.tsx around lines 192 - 225,
Update the not-found rendering condition in the syllabus page so API failures
display a primary backend/server error message instead of “Không tìm thấy
Syllabus.” Within the error block, use error === "API_ERROR" to select the
appropriate title and supporting text while preserving the existing not-found
messaging for genuine missing syllabi.
Comment on lines
+141
to
+152
| const handleDelete = async (documentId: string) => { | ||
| if (!selectedSyllabusId) return; | ||
| setApiError(null); | ||
|
|
||
| try { | ||
| await api.deleteSyllabusDocument(selectedSyllabusId, documentId); | ||
| void loadDocuments(selectedSyllabusId); | ||
| } catch (err) { | ||
| console.error("Delete failed:", err); | ||
| setApiError("Không thể xóa tài liệu."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Destructive delete has no confirmation dialog.
handleDelete permanently removes a document with a single click and no confirmation. An accidental click causes irreversible data loss. Add a confirmation dialog (e.g., Mantine Modal or window.confirm) before calling the API.
🛡️ Proposed fix: add confirmation before delete
const handleDelete = async (documentId: string) => {
if (!selectedSyllabusId) return;
+ if (!window.confirm("Bạn có chắc muốn xóa tài liệu này?")) return;
setApiError(null);Also applies to: 300-300
🤖 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 `@web/app/teacher/documents/page.tsx` around lines 141 - 152, handleDelete
currently calls api.deleteSyllabusDocument without user confirmation, allowing
accidental permanent deletion. Add a confirmation step at the start of
handleDelete, using the existing UI conventions or window.confirm, and return
early when the user cancels; only call api.deleteSyllabusDocument and reload via
loadDocuments after confirmation.
Comment on lines
+45
to
+62
| const handleSessionClick = async (session: api.ApiChatSession) => { | ||
| try { | ||
| const { session: detail } = await api.getChatSessionDetail(session.id); | ||
| if (detail.messages && detail.messages.length > 0) { | ||
| const mapped: Message[] = detail.messages.map((m) => ({ | ||
| id: m.id, | ||
| role: m.sender === "USER" ? "user" : "bot", | ||
| content: m.content, | ||
| timestamp: new Date(m.createdAt), | ||
| })); | ||
| setMessages(mapped); | ||
| } | ||
| setSessionId?.(session.id); | ||
| setIsHistoryOpen(false); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
handleSessionClick doesn't abort in-flight streaming and doesn't handle empty sessions.
Two issues in this handler:
-
No abort: When the user clicks a history session while a stream is in progress, the old stream's
onDonecallback will eventually setisLoadingto false, but until thenisLoadingremainstrue, which blockshandleSend(line 72:if (!input.trim() || isLoading || ...) return). The user is stuck waiting for the old stream to finish. The parent'shandleNewChatcorrectly aborts viaabortRef, buthandleSessionClickhas no access to it. -
Empty messages: If
detail.messagesis empty or undefined,setMessagesis skipped butsetSessionIdis still set to the new session. The user sees stale messages from the previous session under a new session ID.
Consider passing an onSessionSelect callback from ChatbotWidget that handles abort + state updates, similar to handleNewChat:
🔒 Suggested fix: pass abort-aware callback from parent
In ChatbotWidget.tsx:
+ const handleSessionSelect = useCallback(async (session: api.ApiChatSession) => {
+ if (abortRef.current) {
+ abortRef.current.abort();
+ abortRef.current = null;
+ }
+ setIsLoading(false);
+ try {
+ const { session: detail } = await api.getChatSessionDetail(session.id);
+ if (detail.messages && detail.messages.length > 0) {
+ setMessages(detail.messages.map((m) => ({
+ id: m.id,
+ role: m.sender === "USER" ? "user" : "bot",
+ content: m.content,
+ timestamp: new Date(m.createdAt),
+ })));
+ } else {
+ setMessages([{
+ id: Date.now().toString(),
+ role: "bot",
+ content: `Cuộc trò chuyện về ${subjectCode}.`,
+ timestamp: new Date(),
+ }]);
+ }
+ setSessionId(session.id);
+ setIsHistoryOpen(false);
+ } catch {
+ // handle error
+ }
+ }, [subjectCode]);Then pass it to ChatHistorySidebar and use it instead of the local handleSessionClick.
🤖 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 `@web/components/chatbot/subcomponents/ChatHistorySidebar.tsx` around lines 45
- 62, Update ChatHistorySidebar’s session-selection flow to delegate to an
abort-aware onSessionSelect callback supplied by ChatbotWidget, mirroring
handleNewChat so any active stream is aborted and loading state is reset before
switching sessions. Ensure the callback replaces the local handleSessionClick
logic, and when the selected session has no messages, explicitly clear the
messages state before setting the new session ID.
Comment on lines
+16
to
+24
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export const isMaterialChecked = (m: any, field: string) => { | ||
| if (m[field] === "true" || m[field] === "checked" || m[field] === true || m[field] === "1") | ||
| return true; | ||
| if (m.description?.includes("getbootstrap.com") || m.description?.includes("react.dev")) { | ||
| if (field === "is_main_material" || field === "is_online") return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove hardcoded mock-data checks from isMaterialChecked.
Lines 20-22 force is_main_material and is_online to true when a material's description contains "getbootstrap.com" or "react.dev". This is leftover mock/test data logic. In production, any real material with those URLs in its description would incorrectly display as "main material" and "online" regardless of actual API values.
🛡️ Proposed fix
export const isMaterialChecked = (m: any, field: string) => {
if (m[field] === "true" || m[field] === "checked" || m[field] === true || m[field] === "1")
return true;
- if (m.description?.includes("getbootstrap.com") || m.description?.includes("react.dev")) {
- if (field === "is_main_material" || field === "is_online") return true;
- }
return false;
};📝 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
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| export const isMaterialChecked = (m: any, field: string) => { | |
| if (m[field] === "true" || m[field] === "checked" || m[field] === true || m[field] === "1") | |
| return true; | |
| if (m.description?.includes("getbootstrap.com") || m.description?.includes("react.dev")) { | |
| if (field === "is_main_material" || field === "is_online") return true; | |
| } | |
| return false; | |
| }; | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` | |
| export const isMaterialChecked = (m: any, field: string) => { | |
| if (m[field] === "true" || m[field] === "checked" || m[field] === true || m[field] === "1") | |
| return true; | |
| return false; | |
| }; |
🤖 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 `@web/components/student/syllabus/SyllabusHelpers.tsx` around lines 16 - 24,
Remove the description-based getbootstrap.com/react.dev conditional from
isMaterialChecked, including its forced true handling for is_main_material and
is_online. Keep only the actual field-value checks and return false when those
values are not truthy.
Comment on lines
+95
to
+109
| <Table.Td> | ||
| {s.s_download === "FER202" || s.s_download === "PRN232" ? ( | ||
| <Anchor | ||
| href={`https://flm.fpt.edu.vn/download/${metadata.syllabus_id}/S/1_${s.s_download}.zip`} | ||
| size="xs" | ||
| style={{ color: "#1A3A5C" }} | ||
| > | ||
| <Group gap={4}> | ||
| <IconDownload size={12} /> | ||
| {s.s_download} | ||
| </Group> | ||
| </Anchor> | ||
| ) : ( | ||
| <Text size="xs">{formatMultiline(s.s_download) || "—"}</Text> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hardcoded course-specific download logic only works for FER202 and PRN232.
The condition s.s_download === "FER202" || s.s_download === "PRN232" generates download links only for two specific courses. All other courses render s_download as plain text via formatMultiline, even if they have valid download codes. This should use a more general pattern, such as checking if s_download matches a course-code format or if the API provides a download URL directly.
🛡️ Proposed fix
- {s.s_download === "FER202" || s.s_download === "PRN232" ? (
+ {/^[A-Z]{3}\d{3}$/.test(s.s_download) ? (
<Anchor
href={`https://flm.fpt.edu.vn/download/${metadata.syllabus_id}/S/1_${s.s_download}.zip`}This regex matches any FPT-style course code (3 letters + 3 digits). Adjust the pattern based on the actual course code format used by the API.
📝 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
| <Table.Td> | |
| {s.s_download === "FER202" || s.s_download === "PRN232" ? ( | |
| <Anchor | |
| href={`https://flm.fpt.edu.vn/download/${metadata.syllabus_id}/S/1_${s.s_download}.zip`} | |
| size="xs" | |
| style={{ color: "#1A3A5C" }} | |
| > | |
| <Group gap={4}> | |
| <IconDownload size={12} /> | |
| {s.s_download} | |
| </Group> | |
| </Anchor> | |
| ) : ( | |
| <Text size="xs">{formatMultiline(s.s_download) || "—"}</Text> | |
| )} | |
| <Table.Td> | |
| {/^[A-Z]{3}\d{3}$/.test(s.s_download) ? ( | |
| <Anchor | |
| href={`https://flm.fpt.edu.vn/download/${metadata.syllabus_id}/S/1_${s.s_download}.zip`} | |
| size="xs" | |
| style={{ color: "`#1A3A5C`" }} | |
| > | |
| <Group gap={4}> | |
| <IconDownload size={12} /> | |
| {s.s_download} | |
| </Group> | |
| </Anchor> | |
| ) : ( | |
| <Text size="xs">{formatMultiline(s.s_download) || "—"}</Text> | |
| )} |
🤖 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 `@web/components/student/syllabus/SyllabusScheduleTab.tsx` around lines 95 -
109, Replace the hardcoded FER202/PRN232 check in the syllabus schedule
rendering with a generalized course-code validation, such as matching the API’s
supported three-letter, three-digit format, or use a provided download URL when
available. Keep valid codes rendered as download Anchors with the existing URL
structure and render other values with formatMultiline.
Comment on lines
+369
to
+405
| const decoder = new TextDecoder(); | ||
| let buffer = ""; | ||
|
|
||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
|
|
||
| buffer += decoder.decode(value, { stream: true }); | ||
| const lines = buffer.split("\n"); | ||
| buffer = lines.pop() || ""; | ||
|
|
||
| for (const line of lines) { | ||
| if (line.startsWith("event: ")) { | ||
| // event type handled via next data line | ||
| continue; | ||
| } | ||
| if (line.startsWith("data: ")) { | ||
| const data = line.slice(6); | ||
| try { | ||
| const parsed = JSON.parse(data); | ||
| if (parsed.chunk) { | ||
| callbacks.onChunk(parsed.chunk); | ||
| } | ||
| if (parsed.citations) { | ||
| callbacks.onCitations(parsed.citations); | ||
| } | ||
| if (parsed.error) { | ||
| callbacks.onError(parsed.error); | ||
| } | ||
| } catch { | ||
| // ignore malformed lines | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| callbacks.onDone(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SSE stream drops last data line and silently fails on \r\n line endings.
Two parsing defects in the SSE reader:
-
After the
whileloop exits (done === true),buffermay still contain the finaldata:line that never ended with\n. This line is never processed, so the last chunk can be lost — producing truncated chat responses. -
buffer.split("\n")doesn't handle\r\nendings. Each line retains a trailing\r, causingJSON.parse(data)to throw, and thecatchblock silently drops the entire data line. If the server uses\r\n, all chunks are lost.
🔧 Proposed fix: process remaining buffer and handle `\r\n`
const decoder = new TextDecoder();
let buffer = "";
while (true) {
const { done, value } = await reader.read();
if (done) break;
buffer += decoder.decode(value, { stream: true });
- const lines = buffer.split("\n");
+ const lines = buffer.split(/\r?\n/);
buffer = lines.pop() || "";
for (const line of lines) {
if (line.startsWith("event: ")) {
continue;
}
if (line.startsWith("data: ")) {
- const data = line.slice(6);
+ const data = line.slice(6).trim();
try {
const parsed = JSON.parse(data);
if (parsed.chunk) {
callbacks.onChunk(parsed.chunk);
}
if (parsed.citations) {
callbacks.onCitations(parsed.citations);
}
if (parsed.error) {
callbacks.onError(parsed.error);
}
} catch {
// ignore malformed lines
}
}
}
}
+ // Process any remaining buffered line after stream ends
+ if (buffer.startsWith("data: ")) {
+ const data = buffer.slice(6).trim();
+ try {
+ const parsed = JSON.parse(data);
+ if (parsed.chunk) callbacks.onChunk(parsed.chunk);
+ if (parsed.citations) callbacks.onCitations(parsed.citations);
+ if (parsed.error) callbacks.onError(parsed.error);
+ } catch {
+ // ignore malformed final line
+ }
+ }
+
callbacks.onDone();📝 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
| const decoder = new TextDecoder(); | |
| let buffer = ""; | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| buffer += decoder.decode(value, { stream: true }); | |
| const lines = buffer.split("\n"); | |
| buffer = lines.pop() || ""; | |
| for (const line of lines) { | |
| if (line.startsWith("event: ")) { | |
| // event type handled via next data line | |
| continue; | |
| } | |
| if (line.startsWith("data: ")) { | |
| const data = line.slice(6); | |
| try { | |
| const parsed = JSON.parse(data); | |
| if (parsed.chunk) { | |
| callbacks.onChunk(parsed.chunk); | |
| } | |
| if (parsed.citations) { | |
| callbacks.onCitations(parsed.citations); | |
| } | |
| if (parsed.error) { | |
| callbacks.onError(parsed.error); | |
| } | |
| } catch { | |
| // ignore malformed lines | |
| } | |
| } | |
| } | |
| } | |
| callbacks.onDone(); | |
| const decoder = new TextDecoder(); | |
| let buffer = ""; | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| buffer += decoder.decode(value, { stream: true }); | |
| const lines = buffer.split(/\r?\n/); | |
| buffer = lines.pop() || ""; | |
| for (const line of lines) { | |
| if (line.startsWith("event: ")) { | |
| // event type handled via next data line | |
| continue; | |
| } | |
| if (line.startsWith("data: ")) { | |
| const data = line.slice(6).trim(); | |
| try { | |
| const parsed = JSON.parse(data); | |
| if (parsed.chunk) { | |
| callbacks.onChunk(parsed.chunk); | |
| } | |
| if (parsed.citations) { | |
| callbacks.onCitations(parsed.citations); | |
| } | |
| if (parsed.error) { | |
| callbacks.onError(parsed.error); | |
| } | |
| } catch { | |
| // ignore malformed lines | |
| } | |
| } | |
| } | |
| } | |
| // Process any remaining buffered line after stream ends | |
| if (buffer.startsWith("data: ")) { | |
| const data = buffer.slice(6).trim(); | |
| try { | |
| const parsed = JSON.parse(data); | |
| if (parsed.chunk) callbacks.onChunk(parsed.chunk); | |
| if (parsed.citations) callbacks.onCitations(parsed.citations); | |
| if (parsed.error) callbacks.onError(parsed.error); | |
| } catch { | |
| // ignore malformed final line | |
| } | |
| } | |
| callbacks.onDone(); |
🤖 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 `@web/lib/api.ts` around lines 369 - 405, Fix the SSE parsing in the reader
loop by normalizing or trimming trailing carriage returns from lines before
checking prefixes and parsing JSON, so CRLF-delimited events are handled
correctly. After the read loop exits, process any remaining non-empty buffer as
a final SSE line, applying the same data parsing and callbacks logic used inside
the loop before invoking callbacks.onDone().
Comment on lines
+6
to
+24
| try { | ||
| let dir = __dirname; | ||
| let envPath = ""; | ||
| for (let i = 0; i < 5; i++) { | ||
| const checkPath = path.resolve(dir, ".env"); | ||
| if (fs.existsSync(checkPath)) { | ||
| envPath = checkPath; | ||
| break; | ||
| } | ||
| const parent = path.dirname(dir); | ||
| if (parent === dir) break; | ||
| dir = parent; | ||
| } | ||
| if (envPath) { | ||
| process.loadEnvFile(envPath); | ||
| } | ||
| } catch (e) { | ||
| // Ignore env loading errors | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Which Node.js version added process.loadEnvFile
💡 Result:
The process.loadEnvFile() method was added to Node.js in version 21.7.0 [1]. It was introduced alongside util.parseEnv() to allow for the programmatic loading and parsing of .env files into process.env [2][3].
Citations:
- 1: https://nodejs.org/en/blog/release/v21.7.0
- 2: src: add
process.loadEnvFileandutil.parseEnvnodejs/node#51476 - 3: nodejs/node@dc8fe9ebf4
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'web/package.json' 'package.json' 'web/next.config.ts' 'web/lib/supabase-client.ts' 'web/.nvmrc' '.nvmrc' 'web/.node-version' '.node-version' 'web/.tool-versions' '.tool-versions' | sed 's#^`#-` #'
printf '\n## web/package.json\n'
if [ -f web/package.json ]; then
cat -n web/package.json | sed -n '1,220p'
fi
printf '\n## root package.json\n'
if [ -f package.json ]; then
cat -n package.json | sed -n '1,220p'
fi
printf '\n## web/next.config.ts\n'
if [ -f web/next.config.ts ]; then
cat -n web/next.config.ts | sed -n '1,220p'
fi
printf '\n## web/lib/supabase-client.ts\n'
if [ -f web/lib/supabase-client.ts ]; then
cat -n web/lib/supabase-client.ts | sed -n '1,220p'
fi
printf '\n## Node version hints\n'
for f in web/.nvmrc .nvmrc web/.node-version .node-version web/.tool-versions .tool-versions; do
if [ -f "$f" ]; then
echo "--- $f"
cat "$f"
fi
doneRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 3856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for supported Node version declarations and next config environment loading usage.
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'process\.loadEnvFile|engines|node\s*[<>=]|NEXT_PUBLIC_SUPABASE_URL|NEXT_PUBLIC_SUPABASE_ANON_KEY|__dirname|loadEnvFile' \
package.json web/package.json web/next.config.ts web/lib/supabase-client.ts .nvmrc .node-version .tool-versions 2>/dev/null || trueRepository: lgdlong/chatbot-rag-fptu-sdn
Length of output: 580
🌐 Web query:
Node.js release notes process.loadEnvFile 20.12.0 21.7.0
💡 Result:
The process.loadEnvFile function was introduced in Node.js version 20.12.0 and version 21.7.0 as a built-in programmatic API for loading.env files [1][2][3][4]. Key details regarding this feature include: Functionality: It loads environment variables from a specified.env file directly into process.env [5][6]. If no path is provided, it defaults to loading the.env file in the current working directory [2][4]. Usage: It is accessed via the node:process module [5][7]. Example usage includes: const { loadEnvFile } = require('node:process'); loadEnvFile; // Loads './.env' by default loadEnvFile('./path/to/.env'); // Loads a specific file Associated API: Along with process.loadEnvFile, Node.js introduced util.parseEnv(content), which allows for parsing a string of environment variable assignments into an object without automatically setting them on process.env [2][3][4]. Release Context: Node.js 20.12.0: This release (the LTS 'Iron' release) officially included the addition of both process.loadEnvFile and util.parseEnv [2][8]. Node.js 21.7.0: This release (the 'Current' release) also included these features and further improved.env file handling by introducing support for multi-line values [3][9]. Note that usage of NODE_OPTIONS within an.env file loaded via this method will not have any effect on the Node.js process [5][7].
Citations:
- 1: https://nodejs.org/en/blog/release/v20.12.0
- 2: https://github.com/nodejs/node/releases/tag/v20.12.0
- 3: https://github.com/nodejs/node/releases/tag/v21.7.0
- 4: src: add
process.loadEnvFileandutil.parseEnvnodejs/node#51476 - 5: https://nodejs.org/download/release/v20.12.2/docs/api/process.html
- 6: https://nodejs.org/api/environment_variables.html
- 7: https://nodejs.org/docs/latest-v21.x/api/process.html
- 8: nodejs/node@v20.11.1...v20.12.0
- 9: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V21.md
Surface .env load failures in web/next.config.ts
process.loadEnvFile needs Node 20.12.0+/21.7.0+, and this repo doesn’t declare a minimum Node version. If it’s unavailable or the .env parse fails, the empty catch leaves the Supabase env vars unset and the client throws later. Log or rethrow the load error instead of swallowing it.
🤖 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 `@web/next.config.ts` around lines 6 - 24, Update the top-level
environment-loading try/catch in next.config.ts to surface failures from
process.loadEnvFile, including unsupported Node versions and malformed .env
files. Replace the empty catch with a clear console error or rethrow that
includes the captured error, while preserving the existing upward .env search
behavior.
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
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