feat(knowledge): implement knowledge base management with CRUD operat…#18
feat(knowledge): implement knowledge base management with CRUD operat…#18
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds admin-protected knowledge management: API routes (list, ingest, delete), dashboard UI + server actions to add/delete documents, LLM vectorstore delete support, workspace build/test/typecheck updates, and workflow environment flags for GitHub Actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Dashboard (Client)
participant ServerAction as Next.js Server Action
participant API as API Server
participant LLM as LLM Package
participant Supabase as Supabase DB
User->>Browser: Submit add-document form
Browser->>ServerAction: addKnowledgeDocument(formData)
ServerAction->>ServerAction: Read session token & build headers
ServerAction->>API: POST /api/admin/knowledge
API->>LLM: ingestKnowledge(...)
LLM->>Supabase: insert documents & chunks
Supabase-->>LLM: success
LLM-->>API: success
API-->>ServerAction: 200 { success: true }
ServerAction->>Browser: success
Browser->>Browser: revalidate /knowledge
sequenceDiagram
participant User as User
participant Browser as Dashboard (Client)
participant ServerAction as Next.js Server Action
participant API as API Server
participant LLM as LLM Package
participant Supabase as Supabase DB
User->>Browser: Click delete & confirm
Browser->>ServerAction: deleteKnowledgeDocument(id)
ServerAction->>ServerAction: Read session token & build headers
ServerAction->>API: DELETE /api/admin/knowledge/:id
API->>LLM: deleteDocument(id)
LLM->>Supabase: clear chunks then delete document row
Supabase-->>LLM: success
LLM-->>API: success
API-->>ServerAction: 200 { success: true }
ServerAction->>Browser: success
Browser->>Browser: revalidate /knowledge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/dashboard/app/knowledge/components/delete-button.tsx (1)
6-12: Unusedtokenprop in interface and component.The
tokenprop is declared in the interface (Line 9) but explicitly noted as unused. The component destructures onlydocumentIdandtitle(Line 12). If it's not needed because the server action handles auth via session, consider removing it from the interface to avoid confusion.♻️ Remove unused prop
interface DeleteButtonProps { documentId: string; title?: string; - token?: string; // Kept for prop signature match if needed, but unused since action uses session } -export function DeleteButton({ documentId, title }: DeleteButtonProps) { +export function DeleteButton({ documentId, title }: DeleteButtonProps) {Also update the caller in
page.tsxto stop passingtoken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/knowledge/components/delete-button.tsx` around lines 6 - 12, Remove the unused token prop from the DeleteButtonProps interface and any related destructuring: update the interface DeleteButtonProps to only include documentId and optional title, and update the DeleteButton component signature to accept only those props (DeleteButton({ documentId, title })). Also update any callers (e.g., in page.tsx) to stop supplying token when rendering <DeleteButton /> so the prop list remains consistent.apps/dashboard/components/dashboard-shell.tsx (1)
39-44: Styling differs from other navigation links.The "Knowledge Base" link uses a prominent pill-style with background color (
bg-emerald-50,rounded-md,px-3 py-1,font-semibold) while other nav items use plain text styling. If this is intentional to highlight a new feature, consider adding a comment. Otherwise, align the styling with existing nav links for visual consistency.♻️ Optional: Match existing nav link styling
<Link href="/knowledge" - className="bg-emerald-50 text-emerald-700 px-3 py-1 rounded-md transition-colors hover:bg-emerald-100 dark:bg-emerald-950 dark:text-emerald-300 dark:hover:bg-emerald-900 font-semibold" + className="text-gray-600 transition-colors hover:text-gray-900 dark:text-gray-300 dark:hover:text-white" > Knowledge Base </Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/components/dashboard-shell.tsx` around lines 39 - 44, The "Knowledge Base" Link in dashboard-shell.tsx renders with pill-style classes on the Link element (className containing bg-emerald-50, rounded-md, px-3 py-1, font-semibold) which differs from other nav items; either add an inline comment above this Link explaining the intentional highlight, or remove/replace those classes so the Link's className matches the plain text styling used by other nav links—locate the Link whose children are "Knowledge Base" to update its className or add the explanatory comment.apps/dashboard/app/knowledge/components/knowledge-form.tsx (1)
40-129: Modal lacks keyboard accessibility.The modal doesn't handle Escape key to close, doesn't trap focus within the modal while open, and lacks ARIA attributes (
role="dialog",aria-modal="true",aria-labelledby). These are important for screen reader users and keyboard navigation.♻️ Add basic accessibility attributes
<div className="fixed inset-0 z-50 flex items-center justify-center bg-black/50 backdrop-blur-sm"> - <div className="w-full max-w-lg rounded-xl bg-white p-6 shadow-xl dark:bg-gray-900 border dark:border-gray-800"> - <h2 className="text-xl font-semibold mb-4 text-gray-900 dark:text-gray-100"> + <div + role="dialog" + aria-modal="true" + aria-labelledby="knowledge-form-title" + className="w-full max-w-lg rounded-xl bg-white p-6 shadow-xl dark:bg-gray-900 border dark:border-gray-800" + > + <h2 id="knowledge-form-title" className="text-xl font-semibold mb-4 text-gray-900 dark:text-gray-100"> Add Knowledge Source </h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/knowledge/components/knowledge-form.tsx` around lines 40 - 129, The modal in knowledge-form.tsx (render guarded by isOpen, form using onSubmit, cancel handled by setIsOpen, button state via isLoading) lacks keyboard accessibility; add role="dialog" aria-modal="true" and aria-labelledby on the modal container and ensure the heading element has the matching id, implement an Escape key handler that calls setIsOpen(false) when isOpen is true, and add simple focus trapping: on open move focus to the first focusable element (e.g., the title input) and keep focus cycling inside the modal (or prevent focus leaving) until closed, and on close restore focus to the element that opened the modal; wire these behaviors into the component lifecycle (useEffect or equivalent) so they activate only while isOpen is true.apps/dashboard/app/knowledge/page.tsx (1)
70-70: Unnecessarytokenprop passed to DeleteButton.As noted in the DeleteButton component, the
tokenprop is unused since the server action handles authentication via session. Consider removing this prop to reduce confusion.♻️ Remove unused token prop
- <DeleteButton documentId={doc.id} title={doc.title} token={session?.access_token || ''} /> + <DeleteButton documentId={doc.id} title={doc.title} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/knowledge/page.tsx` at line 70, Remove the unnecessary token prop passed to DeleteButton: delete the token={session?.access_token || ''} from the usage in page.tsx and update the DeleteButton component's props/signature (remove the token prop from its parameter/interface and any reference inside DeleteButton) so authentication continues to rely on the server action/session; ensure all other call sites of DeleteButton are updated to match the new prop shape.packages/llm/src/rag/vectorstore.ts (1)
68-84: Remove redundantclearDocumentChunkscall—FK constraint handles cascading delete.The
knowledge_chunkstable is defined withON DELETE CASCADEon itsdocument_idforeign key constraint. Deleting the document automatically removes all associated chunks at the database level, making the manualclearDocumentChunks(documentId)call on line 73 unnecessary.Remove the
clearDocumentChunkscall and delete the document directly for simpler, more efficient code:export async function deleteDocument(documentId: string): Promise<void> { const { error } = await supabaseClient .from('knowledge_documents') .delete() .eq('id', documentId); if (error) { throw new Error(`Failed to delete knowledge document ${documentId}: ${error.message}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/llm/src/rag/vectorstore.ts` around lines 68 - 84, The deleteDocument function currently calls clearDocumentChunks(documentId) before deleting the knowledge_documents row; remove that redundant call so deletion relies on the database FK ON DELETE CASCADE (knowledge_chunks → document_id) and simply call supabaseClient.from('knowledge_documents').delete().eq('id', documentId) inside deleteDocument, preserving the existing error check that throws on error.apps/dashboard/app/knowledge/actions.ts (1)
6-17: Consider passing actual user identity for audit purposes.Hardcoding
'dashboard-admin'asx-wa-userloses the actual user's identity. If the API logs or audits this header, all actions will appear from the same generic user. Consider passing the authenticated user's email or ID.Suggested improvement
-function getApiHeaders(token: string) { +function getApiHeaders(token: string, userEmail?: string) { return { 'Content-Type': 'application/json', Authorization: `Bearer ${token}`, - 'x-wa-user': 'dashboard-admin', + 'x-wa-user': userEmail || 'dashboard-admin', 'x-wa-role': 'admin', }; }Then pass the user's email from the session:
headers: getApiHeaders(session.access_token, session.user?.email),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/knowledge/actions.ts` around lines 6 - 17, getApiHeaders currently hardcodes 'x-wa-user' to 'dashboard-admin', which hides the actual user identity; update the getApiHeaders function signature (getApiHeaders) to accept a user identifier (email or id) as a second parameter and set the 'x-wa-user' header to that passed identity (falling back to 'dashboard-admin' or an empty string if missing), then update callers (e.g., where getApiHeaders(session.access_token) is used) to pass session.user?.email or session.user?.id so real user info is sent for auditing.apps/api/src/routes/knowledge.ts (2)
9-13: Consider validating required environment variables at startup.If
SUPABASE_URLorSUPABASE_SERVICE_ROLE_KEYare undefined, theas stringcast will pass"undefined"tocreateClient, leading to cryptic runtime errors. Explicit validation provides clearer failure messages.Suggested improvement
-const supaUrl = process.env.SUPABASE_URL as string; -const supaKey = process.env.SUPABASE_SERVICE_ROLE_KEY as string; +const supaUrl = process.env.SUPABASE_URL; +const supaKey = process.env.SUPABASE_SERVICE_ROLE_KEY; +if (!supaUrl || !supaKey) { + throw new Error('Missing required environment variables: SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY'); +} const supabase = createClient(supaUrl, supaKey, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/knowledge.ts` around lines 9 - 13, Check and fail fast if SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY are missing instead of casting them to string; update the top-level supaUrl/supaKey initialisation that currently uses "const supaUrl = process.env.SUPABASE_URL as string" and "const supaKey = process.env.SUPABASE_SERVICE_ROLE_KEY as string" to first validate process.env.SUPABASE_URL and process.env.SUPABASE_SERVICE_ROLE_KEY (throw a clear Error or call processLogger.error + process.exit(1)) with a descriptive message, then pass the validated values into createClient so createClient receives real strings and not "undefined".
69-74: Array check is unnecessary for Express route params.Express route params like
:idare always strings. TheArray.isArray(id)check adds defensive code that won't execute under normal circumstances. Consider simplifying.Simplified version
const { id } = req.params; if (!id) { res.status(400).json({ error: 'Document ID is required' }); return; } - // If id is an array, use the first element; otherwise, use as is - const docId = Array.isArray(id) ? id[0] : id; - if (typeof docId !== 'string') { - res.status(400).json({ error: 'Document ID must be a string' }); - return; - } - await deleteDocument(docId); - res.status(200).json({ success: true, deletedId: docId }); + await deleteDocument(id); + res.status(200).json({ success: true, deletedId: id });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/knowledge.ts` around lines 69 - 74, The Array.isArray and typeof checks around id are unnecessary for an Express route param; simplify the handler by removing the Array.isArray(id) branch and the string-type guard and just use the route param directly (keep the variable name docId for clarity). Locate the route handler that reads const { id } = req.params and replace the docId assignment with a direct use of id (or a simple const docId = id) and remove the subsequent early-return error path; if TypeScript complains, assert the param type via the request generic or a string cast so docId is typed as string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/knowledge.ts`:
- Around line 43-52: The handler is assigning an invalid default for sourceType
("inline") which conflicts with the IngestionSource union; change the default to
a valid value (e.g., set const type = sourceType || 'other') or add validation
before calling ingestKnowledge (validate sourceType against the IngestionSource
values and return a 400 on invalid input) so ingestKnowledge({ ..., sourceType:
type, ... }) always receives a permitted value; update references in this code
path (the const type, sourceUrl/source, and the call to ingestKnowledge)
accordingly.
In `@apps/dashboard/app/knowledge/actions.ts`:
- Around line 20-22: Replace the insecure call to supabase.auth.getSession()
with a server-validated call to supabase.auth.getUser(): after creating the
client with createClient(), call const { data: { user } } = await
supabase.auth.getUser(); then check that user exists (e.g. if (!user) throw new
Error('Unauthorized: No active user')) instead of checking
session?.access_token; update any downstream usage that relied on session to use
the validated user (e.g. user.id) so the action trusts a server-verified
identity.
In `@apps/dashboard/app/knowledge/components/knowledge-form.tsx`:
- Around line 11-29: Form inputs are not being cleared after success or when
closing the modal; update onSubmit to reset the form (e.g., call
e.currentTarget.reset() after a successful addKnowledgeDocument and before
setIsOpen(false)) and also ensure the modal-close/Cancel flow clears the form
(either call the same reset logic when hiding the modal or attach a form ref via
useRef and call formRef.current.reset() in the Cancel/close handler). Reference
onSubmit, addKnowledgeDocument, setIsOpen and the modal Cancel/close handler to
implement the reset so reopened modal shows a fresh form.
In `@apps/dashboard/app/knowledge/page.tsx`:
- Around line 17-24: The Supabase fetch sets `error` and leaves `documents` null
on failure, but the UI only checks `documents?.length` so no user-facing error
is shown; update the page component that runs the Supabase query (the block
assigning `const { data: documents, error } = await
supabase.from('knowledge_documents'...)`) and the table rendering logic to
explicitly handle the error state: when `error` is truthy render a clear error
row/placeholder in the tbody (or a visible error banner) instead of relying on
`documents` optional chaining, and ensure downstream code that reads `documents`
(e.g., checks like `documents.length` or maps over `documents`) guards for
`null`/undefined to avoid runtime issues.
- Around line 54-55: The code reads doc.metadata?.sourceType as unknown because
metadata is typed as Record<string, unknown>; change metadata's type to a
stricter interface (e.g., add sourceType?: string to the document/metadata type)
or cast when reading so TypeScript knows it's a string: update the document type
definition used by documents (or the type for metadata) to include sourceType?:
string, or replace the read with an explicit cast like (doc.metadata?.sourceType
as string | undefined) before falling back to 'unknown' in the documents?.map
callback to ensure safe typing when rendering.
---
Nitpick comments:
In `@apps/api/src/routes/knowledge.ts`:
- Around line 9-13: Check and fail fast if SUPABASE_URL or
SUPABASE_SERVICE_ROLE_KEY are missing instead of casting them to string; update
the top-level supaUrl/supaKey initialisation that currently uses "const supaUrl
= process.env.SUPABASE_URL as string" and "const supaKey =
process.env.SUPABASE_SERVICE_ROLE_KEY as string" to first validate
process.env.SUPABASE_URL and process.env.SUPABASE_SERVICE_ROLE_KEY (throw a
clear Error or call processLogger.error + process.exit(1)) with a descriptive
message, then pass the validated values into createClient so createClient
receives real strings and not "undefined".
- Around line 69-74: The Array.isArray and typeof checks around id are
unnecessary for an Express route param; simplify the handler by removing the
Array.isArray(id) branch and the string-type guard and just use the route param
directly (keep the variable name docId for clarity). Locate the route handler
that reads const { id } = req.params and replace the docId assignment with a
direct use of id (or a simple const docId = id) and remove the subsequent
early-return error path; if TypeScript complains, assert the param type via the
request generic or a string cast so docId is typed as string.
In `@apps/dashboard/app/knowledge/actions.ts`:
- Around line 6-17: getApiHeaders currently hardcodes 'x-wa-user' to
'dashboard-admin', which hides the actual user identity; update the
getApiHeaders function signature (getApiHeaders) to accept a user identifier
(email or id) as a second parameter and set the 'x-wa-user' header to that
passed identity (falling back to 'dashboard-admin' or an empty string if
missing), then update callers (e.g., where getApiHeaders(session.access_token)
is used) to pass session.user?.email or session.user?.id so real user info is
sent for auditing.
In `@apps/dashboard/app/knowledge/components/delete-button.tsx`:
- Around line 6-12: Remove the unused token prop from the DeleteButtonProps
interface and any related destructuring: update the interface DeleteButtonProps
to only include documentId and optional title, and update the DeleteButton
component signature to accept only those props (DeleteButton({ documentId, title
})). Also update any callers (e.g., in page.tsx) to stop supplying token when
rendering <DeleteButton /> so the prop list remains consistent.
In `@apps/dashboard/app/knowledge/components/knowledge-form.tsx`:
- Around line 40-129: The modal in knowledge-form.tsx (render guarded by isOpen,
form using onSubmit, cancel handled by setIsOpen, button state via isLoading)
lacks keyboard accessibility; add role="dialog" aria-modal="true" and
aria-labelledby on the modal container and ensure the heading element has the
matching id, implement an Escape key handler that calls setIsOpen(false) when
isOpen is true, and add simple focus trapping: on open move focus to the first
focusable element (e.g., the title input) and keep focus cycling inside the
modal (or prevent focus leaving) until closed, and on close restore focus to the
element that opened the modal; wire these behaviors into the component lifecycle
(useEffect or equivalent) so they activate only while isOpen is true.
In `@apps/dashboard/app/knowledge/page.tsx`:
- Line 70: Remove the unnecessary token prop passed to DeleteButton: delete the
token={session?.access_token || ''} from the usage in page.tsx and update the
DeleteButton component's props/signature (remove the token prop from its
parameter/interface and any reference inside DeleteButton) so authentication
continues to rely on the server action/session; ensure all other call sites of
DeleteButton are updated to match the new prop shape.
In `@apps/dashboard/components/dashboard-shell.tsx`:
- Around line 39-44: The "Knowledge Base" Link in dashboard-shell.tsx renders
with pill-style classes on the Link element (className containing bg-emerald-50,
rounded-md, px-3 py-1, font-semibold) which differs from other nav items; either
add an inline comment above this Link explaining the intentional highlight, or
remove/replace those classes so the Link's className matches the plain text
styling used by other nav links—locate the Link whose children are "Knowledge
Base" to update its className or add the explanatory comment.
In `@packages/llm/src/rag/vectorstore.ts`:
- Around line 68-84: The deleteDocument function currently calls
clearDocumentChunks(documentId) before deleting the knowledge_documents row;
remove that redundant call so deletion relies on the database FK ON DELETE
CASCADE (knowledge_chunks → document_id) and simply call
supabaseClient.from('knowledge_documents').delete().eq('id', documentId) inside
deleteDocument, preserving the existing error check that throws on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 729319c5-2841-4002-abf6-42aca24288ff
📒 Files selected for processing (10)
apps/api/package.jsonapps/api/src/index.tsapps/api/src/routes/knowledge.tsapps/dashboard/app/knowledge/actions.tsapps/dashboard/app/knowledge/components/delete-button.tsxapps/dashboard/app/knowledge/components/knowledge-form.tsxapps/dashboard/app/knowledge/page.tsxapps/dashboard/components/dashboard-shell.tsxpackages/llm/src/rag/ingestion.tspackages/llm/src/rag/vectorstore.ts
| const type = sourceType || 'inline'; | ||
| const source = sourceUrl || `manual-${type}-${Date.now()}`; | ||
|
|
||
| const result = await ingestKnowledge({ | ||
| content, | ||
| title: title || 'Untitled Document', | ||
| sourceType: type, | ||
| sourceUrl: source, | ||
| version: '1.0.0', | ||
| }); |
There was a problem hiding this comment.
Invalid default sourceType value.
The default 'inline' is not a valid sourceType. Per the IngestionSource type in packages/shared/src/rag.ts, the allowed values are: 'faq' | 'policy' | 'product_catalog' | 'shipping_rule' | 'other'. This will cause type mismatches and potentially corrupt metadata.
Proposed fix
- const type = sourceType || 'inline';
+ const validSourceTypes = ['faq', 'policy', 'product_catalog', 'shipping_rule', 'other'] as const;
+ const type = validSourceTypes.includes(sourceType) ? sourceType : 'other';
const source = sourceUrl || `manual-${type}-${Date.now()}`;Alternatively, validate and reject invalid values:
+ const validSourceTypes = ['faq', 'policy', 'product_catalog', 'shipping_rule', 'other'];
+ if (sourceType && !validSourceTypes.includes(sourceType)) {
+ res.status(400).json({ error: `Invalid sourceType. Must be one of: ${validSourceTypes.join(', ')}` });
+ return;
+ }
- const type = sourceType || 'inline';
+ const type = sourceType || 'other';📝 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.
| const type = sourceType || 'inline'; | |
| const source = sourceUrl || `manual-${type}-${Date.now()}`; | |
| const result = await ingestKnowledge({ | |
| content, | |
| title: title || 'Untitled Document', | |
| sourceType: type, | |
| sourceUrl: source, | |
| version: '1.0.0', | |
| }); | |
| const validSourceTypes = ['faq', 'policy', 'product_catalog', 'shipping_rule', 'other'] as const; | |
| const type = validSourceTypes.includes(sourceType) ? sourceType : 'other'; | |
| const source = sourceUrl || `manual-${type}-${Date.now()}`; | |
| const result = await ingestKnowledge({ | |
| content, | |
| title: title || 'Untitled Document', | |
| sourceType: type, | |
| sourceUrl: source, | |
| version: '1.0.0', | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/knowledge.ts` around lines 43 - 52, The handler is
assigning an invalid default for sourceType ("inline") which conflicts with the
IngestionSource union; change the default to a valid value (e.g., set const type
= sourceType || 'other') or add validation before calling ingestKnowledge
(validate sourceType against the IngestionSource values and return a 400 on
invalid input) so ingestKnowledge({ ..., sourceType: type, ... }) always
receives a permitted value; update references in this code path (the const type,
sourceUrl/source, and the call to ingestKnowledge) accordingly.
| const supabase = await createClient(); | ||
| const { data: { session } } = await supabase.auth.getSession(); | ||
| if (!session?.access_token) throw new Error('Unauthorized: No active session'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Supabase getSession vs getUser server-side security best practices
💡 Result:
Supabase getSession vs getUser for server-side security best practices: getSession: Returns the locally stored session (JWT access_token + refresh_token). Fast (no network call), but only validates JWT signature/expiry locally - does not check if session revoked or user deleted server-side. Suitable for client-side or non-critical server checks where RLS handles security. getUser(jwt): Validates JWT with Supabase Auth server (network + DB call), fetches latest user data from auth.users table. Ensures session still valid (e.g., not revoked). More secure but slower/expensive. Server-side best practices: - Extract JWT from Authorization header or cookies. - For performance + security: Use getClaims(jwt) first - verifies JWT locally (fast, supports asymmetric keys, no DB hit). Extract user ID/email/role from claims. Use for auth checks before DB queries. - If need fresh user data (e.g., update password, check ban): Use getUser(jwt). - Never trust client session storage server-side; always validate incoming JWT. - In Edge Functions/middleware: Parse header, call supabase.auth.getClaims(token), check claims.email/sub/role, return 401 if invalid. - Rely on RLS + JWT claims for DB security; minimize getUser calls. - Avoid getSession server-side as it relies on local storage (not available/reliable). Example (Edge Function): const token = authHeader.replace('Bearer ', ''); const { data, error } = await supabase.auth.getClaims(token); if (!data?.claims?.email || error) return 401; const userId = data.claims.sub; // Proceed with supabase.from('table').select - RLS uses JWT role/claims {getSession: local session return, fast but no server validation [3,9,10,11], getUser: server-validated user data [4,7,19], getClaims: local JWT verify for perf/security [6,7,8,13], server practices: validate header JWT w/ getClaims [6,7,8,20], citations: [2,3,4,6,7,8,9,10,11,13,14,20], confidence: high}
Citations:
- 1: https://supabase.com/docs/guides/auth/sessions
- 2: https://www.reddit.com/r/Supabase/comments/1d8vg80/getuser_vs_getsesssion_to_check_if_user_is_signed/
- 3: https://supabase.com/docs/reference/javascript/auth-getuser?example=get-the-logged-in-user-with-a-custom-access-token-jwt
- 4: https://supabase.io/docs/guides/functions/auth
- 5: Clarify when to use
getClaimsandgetUserand discourage usage ofgetSessionsupabase/supabase#40985 - 6: https://supabase.com/docs/guides/functions/auth
- 7: https://www.reddit.com/r/Supabase/comments/1iziebu/best_practices_for_managing_user_auth_and_data_in/
- 8: Security and performance risk with
getUserandgetSessionsupabase/auth-js#898 - 9: https://supabase.com/docs/reference/javascript/auth-getsession
- 10: https://supabase.com/docs/reference/javascript/auth-getclaims
- 11: https://supabase.com/docs/guides/auth/jwt-fields
- 12: https://supabase.com/docs/guides/auth/jwts
🏁 Script executed:
cat -n apps/dashboard/app/knowledge/actions.tsRepository: Ryanakml/ChatFuse
Length of output: 2946
🏁 Script executed:
# Search for createClient definition
rg "createClient" apps/dashboard/app/knowledge/actions.ts -B 3 -A 3Repository: Ryanakml/ChatFuse
Length of output: 784
🏁 Script executed:
# Search for createClient function definition in the codebase
fd -t ts -t js | xargs grep -l "export.*createClient" | head -10Repository: Ryanakml/ChatFuse
Length of output: 230
🏁 Script executed:
find . -path "*/lib/supabase/server*" -type fRepository: Ryanakml/ChatFuse
Length of output: 100
🏁 Script executed:
# Also search for createClient definition more broadly
rg "export.*createClient" --type ts -A 10 | head -40Repository: Ryanakml/ChatFuse
Length of output: 1953
🏁 Script executed:
cat -n apps/dashboard/lib/supabase/server.tsRepository: Ryanakml/ChatFuse
Length of output: 1137
Use getUser() instead of getSession() for secure server-side verification.
getSession() reads from cookies without verifying the JWT with Supabase servers, making it susceptible to tampering. For Server Actions, Supabase recommends getUser() which validates the token server-side before trusting it.
Proposed fix
const supabase = await createClient();
- const { data: { session } } = await supabase.auth.getSession();
- if (!session?.access_token) throw new Error('Unauthorized: No active session');
+ const { data: { user }, error: userError } = await supabase.auth.getUser();
+ if (userError || !user) throw new Error('Unauthorized: No active session');
+
+ // Get session for the access token after verifying user
+ const { data: { session } } = await supabase.auth.getSession();
+ if (!session?.access_token) throw new Error('Unauthorized: No access token');📝 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.
| const supabase = await createClient(); | |
| const { data: { session } } = await supabase.auth.getSession(); | |
| if (!session?.access_token) throw new Error('Unauthorized: No active session'); | |
| const supabase = await createClient(); | |
| const { data: { user }, error: userError } = await supabase.auth.getUser(); | |
| if (userError || !user) throw new Error('Unauthorized: No active session'); | |
| // Get session for the access token after verifying user | |
| const { data: { session } } = await supabase.auth.getSession(); | |
| if (!session?.access_token) throw new Error('Unauthorized: No access token'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/knowledge/actions.ts` around lines 20 - 22, Replace the
insecure call to supabase.auth.getSession() with a server-validated call to
supabase.auth.getUser(): after creating the client with createClient(), call
const { data: { user } } = await supabase.auth.getUser(); then check that user
exists (e.g. if (!user) throw new Error('Unauthorized: No active user')) instead
of checking session?.access_token; update any downstream usage that relied on
session to use the validated user (e.g. user.id) so the action trusts a
server-verified identity.
| async function onSubmit(e: React.FormEvent<HTMLFormElement>) { | ||
| e.preventDefault(); | ||
| setIsLoading(true); | ||
| setError(''); | ||
|
|
||
| const formData = new FormData(e.currentTarget); | ||
| try { | ||
| await addKnowledgeDocument(formData); | ||
| setIsOpen(false); | ||
| } catch (err: unknown) { | ||
| if (err && typeof err === 'object' && 'message' in err) { | ||
| setError((err as { message?: string }).message || 'Error uploading document'); | ||
| } else { | ||
| setError('Error uploading document'); | ||
| } | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Form data persists after successful submission or modal close.
When the modal closes after a successful submission (Line 19) or via Cancel, the form fields retain their values. If the user reopens the modal, they'll see stale data. Consider resetting the form on success or when closing.
🔧 Proposed fix to reset form on close
async function onSubmit(e: React.FormEvent<HTMLFormElement>) {
e.preventDefault();
setIsLoading(true);
setError('');
const formData = new FormData(e.currentTarget);
try {
await addKnowledgeDocument(formData);
+ e.currentTarget.reset();
setIsOpen(false);
} catch (err: unknown) {And for the Cancel button:
<button
type="button"
- onClick={() => setIsOpen(false)}
+ onClick={() => {
+ setError('');
+ setIsOpen(false);
+ }}
disabled={isLoading}📝 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.
| async function onSubmit(e: React.FormEvent<HTMLFormElement>) { | |
| e.preventDefault(); | |
| setIsLoading(true); | |
| setError(''); | |
| const formData = new FormData(e.currentTarget); | |
| try { | |
| await addKnowledgeDocument(formData); | |
| setIsOpen(false); | |
| } catch (err: unknown) { | |
| if (err && typeof err === 'object' && 'message' in err) { | |
| setError((err as { message?: string }).message || 'Error uploading document'); | |
| } else { | |
| setError('Error uploading document'); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| } | |
| async function onSubmit(e: React.FormEvent<HTMLFormElement>) { | |
| e.preventDefault(); | |
| setIsLoading(true); | |
| setError(''); | |
| const formData = new FormData(e.currentTarget); | |
| try { | |
| await addKnowledgeDocument(formData); | |
| e.currentTarget.reset(); | |
| setIsOpen(false); | |
| } catch (err: unknown) { | |
| if (err && typeof err === 'object' && 'message' in err) { | |
| setError((err as { message?: string }).message || 'Error uploading document'); | |
| } else { | |
| setError('Error uploading document'); | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/knowledge/components/knowledge-form.tsx` around lines 11 -
29, Form inputs are not being cleared after success or when closing the modal;
update onSubmit to reset the form (e.g., call e.currentTarget.reset() after a
successful addKnowledgeDocument and before setIsOpen(false)) and also ensure the
modal-close/Cancel flow clears the form (either call the same reset logic when
hiding the modal or attach a form ref via useRef and call
formRef.current.reset() in the Cancel/close handler). Reference onSubmit,
addKnowledgeDocument, setIsOpen and the modal Cancel/close handler to implement
the reset so reopened modal shows a fresh form.
| const { data: documents, error } = await supabase | ||
| .from('knowledge_documents') | ||
| .select('id, source, title, version, metadata, created_at, updated_at') | ||
| .order('created_at', { ascending: false }); | ||
|
|
||
| if (error) { | ||
| console.error('Error fetching knowledge documents:', error); | ||
| } |
There was a problem hiding this comment.
Error state not surfaced to users; potential null documents handling gap.
When the Supabase query fails (Line 22), the error is logged but documents will be null. The current code handles documents?.length === 0 (Line 47) but doesn't explicitly handle the null case from an error, which would skip rendering anything in the tbody due to optional chaining on Line 54.
Consider showing a user-facing error message when the fetch fails.
🔧 Proposed fix to handle error state
if (error) {
console.error('Error fetching knowledge documents:', error);
}
return (
<DashboardShell>
<main className="flex-1 p-6 lg:p-8">
<div className="mx-auto max-w-6xl space-y-8">
<div className="flex flex-col gap-4 sm:flex-row sm:items-center justify-between">
<h1 className="text-3xl font-bold tracking-tight">Knowledge Base (RAG)</h1>
<KnowledgeForm />
</div>
+ {error && (
+ <div className="rounded-md bg-red-50 p-4 text-sm text-red-700 dark:bg-red-900/30 dark:text-red-400">
+ Failed to load knowledge documents. Please try again later.
+ </div>
+ )}
+
<div className="rounded-xl border border-gray-200 bg-white shadow-sm dark:border-gray-800 dark:bg-gray-950">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/knowledge/page.tsx` around lines 17 - 24, The Supabase
fetch sets `error` and leaves `documents` null on failure, but the UI only
checks `documents?.length` so no user-facing error is shown; update the page
component that runs the Supabase query (the block assigning `const { data:
documents, error } = await supabase.from('knowledge_documents'...)`) and the
table rendering logic to explicitly handle the error state: when `error` is
truthy render a clear error row/placeholder in the tbody (or a visible error
banner) instead of relying on `documents` optional chaining, and ensure
downstream code that reads `documents` (e.g., checks like `documents.length` or
maps over `documents`) guards for `null`/undefined to avoid runtime issues.
| documents?.map((doc) => { | ||
| const metaType = doc.metadata?.sourceType || 'unknown'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for KnowledgeDocument type definition
ast-grep --pattern 'type KnowledgeDocument = {
$$$
}'
# Also check interface definition
ast-grep --pattern 'interface KnowledgeDocument {
$$$
}'
# Search for metadata typing in shared package
rg -n 'metadata.*:' --glob '**/shared/**/*.ts' -A2Repository: Ryanakml/ChatFuse
Length of output: 2830
🏁 Script executed:
# Find and examine the knowledge/page.tsx file
fd 'knowledge.*page.tsx' --type fRepository: Ryanakml/ChatFuse
Length of output: 43
🏁 Script executed:
# Get context around lines 54-55 in knowledge/page.tsx
cat -n apps/dashboard/app/knowledge/page.tsx | head -80 | tail -40Repository: Ryanakml/ChatFuse
Length of output: 2469
🏁 Script executed:
# Search for other metadata property accesses in the file
rg -n 'metadata\?' apps/dashboard/app/knowledge/page.tsx -B2 -A2Repository: Ryanakml/ChatFuse
Length of output: 369
Add stricter typing for metadata.sourceType property.
The metadata field is typed as Record<string, unknown>, making the type of doc.metadata?.sourceType effectively unknown. While the optional chaining prevents runtime errors, TypeScript cannot validate this property at compile time. Either:
- Define a more specific type for
metadata(e.g.,{ sourceType?: string; ...}instead ofRecord<string, unknown>), or - Cast the result to ensure type safety:
const metaType = (doc.metadata?.sourceType as string | undefined) || 'unknown';
This ensures proper type checking when rendering in JSX at line 63.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/knowledge/page.tsx` around lines 54 - 55, The code reads
doc.metadata?.sourceType as unknown because metadata is typed as Record<string,
unknown>; change metadata's type to a stricter interface (e.g., add sourceType?:
string to the document/metadata type) or cast when reading so TypeScript knows
it's a string: update the document type definition used by documents (or the
type for metadata) to include sourceType?: string, or replace the read with an
explicit cast like (doc.metadata?.sourceType as string | undefined) before
falling back to 'unknown' in the documents?.map callback to ensure safe typing
when rendering.
1 participant
…ions
Summary by CodeRabbit
New Features
API
Refactor
Chores / CI