Harden project access, reduce client-side data exposure, and move workspace persistence to Supabase#30
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { Plus, Edit, Trash2, Search, Filter } from "lucide-react" | ||
| import { useLanguage } from "@/lib/config" | ||
| import Link from "next/link" | ||
| import { createDiagram, deleteDiagram, getDiagrams, updateDiagram, type Diagram as DBDiagram } from "@/lib/database" |
| } | ||
| } | ||
|
|
||
| function toCachedUser(user: User): MinimalCachedUser { |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/hooks/useAuth.ts (2)
26-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCache not updated when user exists but profile doesn't.
When
authUserexists butprofileis null (lines 26-40), the hook skips callingsetCachedUser. This differs fromgetCurrentUserAsync()inauth-service.tswhich still caches a basic user object when profile doesn't exist (lines 94-104). This inconsistency means the in-memory cache won't be populated for users without profiles when using this hook.Proposed fix to match auth-service behavior
if (profile) { const userData: User = { id: profile.id, email: authUser.email || '', name: profile.name, createdAt: profile.created_at, language: profile.language, bio: profile.bio, location: profile.location, website: profile.website, avatar: profile.avatar } setUser(userData) setCachedUser(userData) + } else { + // Handle user without profile (matches auth-service behavior) + const userData: User = { + id: authUser.id, + email: authUser.email || '', + name: authUser.user_metadata?.name || authUser.email?.split('@')[0] || 'User', + createdAt: authUser.created_at, + language: 'en' + } + setUser(userData) + setCachedUser(userData) }🤖 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 `@lib/hooks/useAuth.ts` around lines 26 - 40, The hook currently only sets and caches a User when profile exists; mirror auth-service.ts/getCurrentUserAsync behavior by handling the case where authUser exists but profile is null: construct a minimal User object from authUser (id, email, name if available, and default/empty values for profile fields like createdAt, language, bio, location, website, avatar), then call setUser(user) and setCachedUser(user); update the branch around the existing profile check in useAuth (referencing variables authUser, profile and functions setUser, setCachedUser) so both full-profile and no-profile cases populate the in-memory cache.
10-10:⚠️ Potential issue | 🔴 Critical | 💤 Low valueMemoize
createClient()to prevent infinite effect re-runs.
createClient()is called at render time (line 10) and creates a new instance on every render. Sincesupabaseis in the useEffect dependency array (line 84), this causes the effect to re-run on every render. Wrap the call inuseMemoto stabilize the reference:const supabase = useMemo(() => createClient(), [])Alternatively, move
const supabase = createClient()inside the useEffect to avoid the dependency entirely.🤖 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 `@lib/hooks/useAuth.ts` at line 10, The supabase client is recreated on every render because createClient() is called directly in the component scope (const supabase = createClient()), which causes the useEffect that depends on supabase to re-run continuously; fix this by stabilizing the reference either by memoizing the client with React's useMemo (e.g., const supabase = useMemo(() => createClient(), [])) or by moving the createClient() call inside the effect that currently depends on supabase so the dependency can be removed; update the useAuth hook file to use createClient, supabase, and useEffect accordingly to prevent infinite rerenders.
🧹 Nitpick comments (2)
lib/server/project-access.ts (1)
94-107: 💤 Low valueFunction name
requireAuthenticatedUseris misleading.The name implies the function will throw or halt execution if no user is found (similar to common "require" patterns), but it returns
nullinstead. Consider renaming togetAuthenticatedUserortryGetAuthenticatedUserto better reflect the nullable return semantics.Suggested rename
-export async function requireAuthenticatedUser( +export async function getAuthenticatedUser( supabase: SupabaseClient, ): Promise<User | null> {🤖 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 `@lib/server/project-access.ts` around lines 94 - 107, The function requireAuthenticatedUser is misleading because it returns null instead of throwing; rename it to tryGetAuthenticatedUser (or getAuthenticatedUser) to reflect nullable semantics, update the exported function name and all internal references/usages (calls, imports, tests, and any JSDoc/types) to the new identifier, and run/adjust TypeScript types and exports so no orphaned references remain; keep the implementation unchanged and ensure any callers handle the nullable return accordingly.app/dashboard/diagrams/text/[id]/page.tsx (1)
25-50: 💤 Low valueSVG sanitization is solid; consider expanding the element blocklist.
The sanitization correctly handles major XSS vectors (
script,foreignObject,on*handlers,javascript:URLs). For additional hardening, consider addingiframe,embed, andobjectto the disallowed tags, though these are unlikely in Mermaid output.♻️ Optional: expand blocklist
- const disallowedTags = new Set(["script", "foreignObject"]) + const disallowedTags = new Set(["script", "foreignObject", "iframe", "embed", "object"])🤖 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 `@app/dashboard/diagrams/text/`[id]/page.tsx around lines 25 - 50, The sanitizeMermaidSvg function currently blocks "script" and "foreignObject" but should also disallow potentially dangerous embedding tags; update the disallowedTags Set in sanitizeMermaidSvg to include "iframe", "embed", and "object" (in addition to the existing tags) so the walker will remove those elements as well; keep the rest of the attribute stripping logic (on* and javascript: checks) unchanged.
🤖 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 `@app/api/auth/signup/route.ts`:
- Around line 41-48: The email validation currently uses lastIndexOf('@') which
allows multiple '@' characters (e.g., "a@b@c.com"); update the logic in the
signup route where variables email, atIndex, and hasValidEmailShape are computed
to reject emails with more than one '@' by checking email.indexOf('@') ===
email.lastIndexOf('@') (or counting '@' occurrences) in addition to the existing
checks so that hasValidEmailShape becomes false when multiple '@' characters are
present.
In `@app/dashboard/community/page.tsx`:
- Around line 38-62: The loadDiscussions function currently performs async calls
to getDiscussions() and the Supabase profile query without error handling; wrap
the body of loadDiscussions in a try/catch (and optional finally for loading
state) so any errors from getDiscussions() or
createClient().from(...).select(...).in(...) are caught, log the error, and
surface a UI error state/toast instead of failing silently; only call
setDiscussions(...) on success and add or reuse an error state setter (e.g.,
setError or a toast helper) to show the user a message and optionally clear or
preserve the previous list as appropriate.
- Around line 69-88: The handler handleCreateDiscussion lacks error handling and
a submission guard which can cause silent failures and double-submits; add a
component state like isSubmitting via useState(false), return early if
isSubmitting at the top of handleCreateDiscussion, wrap the async sequence
(getCurrentUserAsync, createDiscussion, loadDiscussions, setNewDiscussion,
setShowNewDiscussionModal) in try/catch/finally, setIsSubmitting(true) before
the try and setIsSubmitting(false) in finally, surface errors in the catch
(e.g., show a toast or call processLogger/error handler), and bind isSubmitting
to your submit button's disabled/loading prop so users cannot resubmit while the
request is in flight.
In `@app/dashboard/diagrams/`[id]/page.tsx:
- Around line 498-502: The success alert in handleSave is shown unconditionally
even if updateDiagram fails; wrap the await updateDiagram(diagram.id, { data,
updated_at: new Date().toISOString() }) call in a try/catch inside handleSave,
only show alert((t.diagrams as any).saved || "Diagram saved successfully!") on
success, and in the catch block log the error and show a failure alert (or other
UI error indication) mentioning the save failed so users aren't misled; ensure
you still return early if !diagram as before.
In `@app/dashboard/diagrams/page.tsx`:
- Around line 194-199: The deletion handler handleDeleteDiagram performs an
unguarded async delete and optimistically mutates UI via setDiagrams even if
deleteDiagram fails; wrap the await deleteDiagram(id) in a try/catch and only
call setDiagrams(diagrams.filter(...)) on success (or, alternatively, call a
refetch function after a successful delete), and on error log/show the failure
so the UI remains consistent; update references to handleDeleteDiagram,
deleteDiagram, and setDiagrams accordingly.
- Around line 72-93: The handler handleCreateDiagram should not reset the form
or close the modal when createDiagram fails; wrap the createDiagram +
loadDiagrams calls in a try/catch, call createDiagram(...) and await
loadDiagrams(user.id) inside the try, and only then call setNewDiagram(...) and
setShowCreateModal(false); in the catch block log the error and surface it to
the user (e.g., set an error state or call your toast/error notification) so
failures don’t give a false success; keep getDefaultTemplate usage unchanged and
ensure user existence check remains before the try.
In `@app/dashboard/diagrams/text/`[id]/page.tsx:
- Around line 157-170: handleSave currently calls setSavedMessage(true)
regardless of whether updateDiagram(diagram.id, ...) succeeds, causing a false
success indicator; wrap the updateDiagram call in a try/catch inside handleSave,
only call setSavedMessage(true) and start the timeout if updateDiagram resolves,
and handle failures in the catch (e.g., surface an error state/message or
console.error) so users are not shown a success when save fails; reference the
handleSave function, updateDiagram call, and setSavedMessage/setTimeout logic to
locate where to add the try/catch and success-only UI update.
In `@app/dashboard/layout.tsx`:
- Around line 62-73: The snapshot currently hardcodes language:"en" which masks
the user's real preference; update the profile SELECT (the profile query
referenced near the dashboard layout) to include the language field and then set
dashboardUserSnapshot's language from the fetched profile (e.g. use
user?.language or sidebarUser.language if you add it there) instead of the
literal "en" so readDashboardUserSnapshot in auth-service.ts will cache the
correct language; if you intentionally want a default, document that behavior
and fall back to "en" only when no language is present.
In `@lib/features/projects/index.ts`:
- Around line 94-113: getProjectCollaborators uses a relative fetch URL which
breaks when called server-side; change the fetch call to use an absolute URL by
constructing the endpoint with your app's base/origin instead of "/api/...".
Replace the string `/api/projects/${projectId}/collaborators` with something
like `${getBaseUrl()}/api/projects/${projectId}/collaborators` (or use your
existing helper like getAbsoluteUrl/getOrigin) and import/consume that helper in
this module, keeping the same fetch options, response parsing and error handling
in getProjectCollaborators.
---
Outside diff comments:
In `@lib/hooks/useAuth.ts`:
- Around line 26-40: The hook currently only sets and caches a User when profile
exists; mirror auth-service.ts/getCurrentUserAsync behavior by handling the case
where authUser exists but profile is null: construct a minimal User object from
authUser (id, email, name if available, and default/empty values for profile
fields like createdAt, language, bio, location, website, avatar), then call
setUser(user) and setCachedUser(user); update the branch around the existing
profile check in useAuth (referencing variables authUser, profile and functions
setUser, setCachedUser) so both full-profile and no-profile cases populate the
in-memory cache.
- Line 10: The supabase client is recreated on every render because
createClient() is called directly in the component scope (const supabase =
createClient()), which causes the useEffect that depends on supabase to re-run
continuously; fix this by stabilizing the reference either by memoizing the
client with React's useMemo (e.g., const supabase = useMemo(() =>
createClient(), [])) or by moving the createClient() call inside the effect that
currently depends on supabase so the dependency can be removed; update the
useAuth hook file to use createClient, supabase, and useEffect accordingly to
prevent infinite rerenders.
---
Nitpick comments:
In `@app/dashboard/diagrams/text/`[id]/page.tsx:
- Around line 25-50: The sanitizeMermaidSvg function currently blocks "script"
and "foreignObject" but should also disallow potentially dangerous embedding
tags; update the disallowedTags Set in sanitizeMermaidSvg to include "iframe",
"embed", and "object" (in addition to the existing tags) so the walker will
remove those elements as well; keep the rest of the attribute stripping logic
(on* and javascript: checks) unchanged.
In `@lib/server/project-access.ts`:
- Around line 94-107: The function requireAuthenticatedUser is misleading
because it returns null instead of throwing; rename it to
tryGetAuthenticatedUser (or getAuthenticatedUser) to reflect nullable semantics,
update the exported function name and all internal references/usages (calls,
imports, tests, and any JSDoc/types) to the new identifier, and run/adjust
TypeScript types and exports so no orphaned references remain; keep the
implementation unchanged and ensure any callers handle the nullable return
accordingly.
🪄 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: a93a0ed0-0069-4ce2-99f7-349809145654
📒 Files selected for processing (15)
.github/workflows/ci.ymlapp/api/auth/signup/route.tsapp/api/projects/[id]/collaborators/route.tsapp/api/projects/route.tsapp/dashboard/community/page.tsxapp/dashboard/diagrams/[id]/page.tsxapp/dashboard/diagrams/page.tsxapp/dashboard/diagrams/text/[id]/page.tsxapp/dashboard/layout.tsxlib/config/i18n.tslib/features/auth/auth-service.tslib/features/projects/index.tslib/hooks/useAuth.tslib/server/project-access.tssupabase/migrations/20260520_fix_project_collaborators_rls.sql
Comment on lines
+41
to
+48
| const atIndex = email.lastIndexOf('@') | ||
| const hasValidEmailShape = | ||
| atIndex > 0 && | ||
| atIndex < email.length - 3 && | ||
| !email.includes(' ') && | ||
| email.slice(atIndex + 1).includes('.') | ||
|
|
||
| if (!hasValidEmailShape) { |
There was a problem hiding this comment.
Reject multiple @ characters in email validation.
On Line 41, lastIndexOf('@') plus current checks allows malformed values like a@b@c.com. This is overly permissive for signup input validation.
Suggested fix
- const atIndex = email.lastIndexOf('@')
+ const atIndex = email.indexOf('@')
+ const hasSingleAt = atIndex === email.lastIndexOf('@')
+ const domain = atIndex >= 0 ? email.slice(atIndex + 1) : ''
const hasValidEmailShape =
+ hasSingleAt &&
atIndex > 0 &&
atIndex < email.length - 3 &&
!email.includes(' ') &&
- email.slice(atIndex + 1).includes('.')
+ domain.includes('.') &&
+ !domain.startsWith('.') &&
+ !domain.endsWith('.')📝 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 atIndex = email.lastIndexOf('@') | |
| const hasValidEmailShape = | |
| atIndex > 0 && | |
| atIndex < email.length - 3 && | |
| !email.includes(' ') && | |
| email.slice(atIndex + 1).includes('.') | |
| if (!hasValidEmailShape) { | |
| const atIndex = email.indexOf('@') | |
| const hasSingleAt = atIndex === email.lastIndexOf('@') | |
| const domain = atIndex >= 0 ? email.slice(atIndex + 1) : '' | |
| const hasValidEmailShape = | |
| hasSingleAt && | |
| atIndex > 0 && | |
| atIndex < email.length - 3 && | |
| !email.includes(' ') && | |
| domain.includes('.') && | |
| !domain.startsWith('.') && | |
| !domain.endsWith('.') | |
| if (!hasValidEmailShape) { |
🤖 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 `@app/api/auth/signup/route.ts` around lines 41 - 48, The email validation
currently uses lastIndexOf('@') which allows multiple '@' characters (e.g.,
"a@b@c.com"); update the logic in the signup route where variables email,
atIndex, and hasValidEmailShape are computed to reject emails with more than one
'@' by checking email.indexOf('@') === email.lastIndexOf('@') (or counting '@'
occurrences) in addition to the existing checks so that hasValidEmailShape
becomes false when multiple '@' characters are present.
Comment on lines
+38
to
62
| const loadDiscussions = async () => { | ||
| const supabase = createClient() | ||
| const rows = await getDiscussions() | ||
| const userIds = Array.from(new Set(rows.map((row) => row.user_id).filter(Boolean))) | ||
| const { data: profiles } = userIds.length | ||
| ? await supabase.from("profiles").select("id, name, email").in("id", userIds) | ||
| : { data: [] } | ||
| const profileMap = new Map((profiles || []).map((profile) => [profile.id, profile])) | ||
|
|
||
| setDiscussions( | ||
| rows.map((row) => { | ||
| const profile = profileMap.get(row.user_id) | ||
| return { | ||
| id: row.id, | ||
| title: row.title, | ||
| content: row.content, | ||
| category: row.category || "general", | ||
| author: profile?.name || profile?.email || "Member", | ||
| authorEmail: profile?.email || "", | ||
| replies: row.replies || 0, | ||
| createdAt: row.created_at, | ||
| } | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent silent failures.
If getDiscussions() or the profile query fails, the error propagates uncaught, leaving users with an empty or stale list and no indication of what went wrong. Wrap the async operations in try/catch and consider showing an error state or toast notification.
🛡️ Proposed fix to add error handling
const loadDiscussions = async () => {
+ try {
const supabase = createClient()
const rows = await getDiscussions()
const userIds = Array.from(new Set(rows.map((row) => row.user_id).filter(Boolean)))
const { data: profiles } = userIds.length
? await supabase.from("profiles").select("id, name, email").in("id", userIds)
: { data: [] }
const profileMap = new Map((profiles || []).map((profile) => [profile.id, profile]))
setDiscussions(
rows.map((row) => {
const profile = profileMap.get(row.user_id)
return {
id: row.id,
title: row.title,
content: row.content,
category: row.category || "general",
author: profile?.name || profile?.email || "Member",
authorEmail: profile?.email || "",
replies: row.replies || 0,
createdAt: row.created_at,
}
}),
)
+ } catch (error) {
+ console.error("Failed to load discussions:", error)
+ // Consider setting an error state to display to users
+ }
}📝 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 loadDiscussions = async () => { | |
| const supabase = createClient() | |
| const rows = await getDiscussions() | |
| const userIds = Array.from(new Set(rows.map((row) => row.user_id).filter(Boolean))) | |
| const { data: profiles } = userIds.length | |
| ? await supabase.from("profiles").select("id, name, email").in("id", userIds) | |
| : { data: [] } | |
| const profileMap = new Map((profiles || []).map((profile) => [profile.id, profile])) | |
| setDiscussions( | |
| rows.map((row) => { | |
| const profile = profileMap.get(row.user_id) | |
| return { | |
| id: row.id, | |
| title: row.title, | |
| content: row.content, | |
| category: row.category || "general", | |
| author: profile?.name || profile?.email || "Member", | |
| authorEmail: profile?.email || "", | |
| replies: row.replies || 0, | |
| createdAt: row.created_at, | |
| } | |
| }), | |
| ) | |
| } | |
| const loadDiscussions = async () => { | |
| try { | |
| const supabase = createClient() | |
| const rows = await getDiscussions() | |
| const userIds = Array.from(new Set(rows.map((row) => row.user_id).filter(Boolean))) | |
| const { data: profiles } = userIds.length | |
| ? await supabase.from("profiles").select("id, name, email").in("id", userIds) | |
| : { data: [] } | |
| const profileMap = new Map((profiles || []).map((profile) => [profile.id, profile])) | |
| setDiscussions( | |
| rows.map((row) => { | |
| const profile = profileMap.get(row.user_id) | |
| return { | |
| id: row.id, | |
| title: row.title, | |
| content: row.content, | |
| category: row.category || "general", | |
| author: profile?.name || profile?.email || "Member", | |
| authorEmail: profile?.email || "", | |
| replies: row.replies || 0, | |
| createdAt: row.created_at, | |
| } | |
| }), | |
| ) | |
| } catch (error) { | |
| console.error("Failed to load discussions:", error) | |
| // Consider setting an error state to display to users | |
| } | |
| } |
🤖 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 `@app/dashboard/community/page.tsx` around lines 38 - 62, The loadDiscussions
function currently performs async calls to getDiscussions() and the Supabase
profile query without error handling; wrap the body of loadDiscussions in a
try/catch (and optional finally for loading state) so any errors from
getDiscussions() or createClient().from(...).select(...).in(...) are caught, log
the error, and surface a UI error state/toast instead of failing silently; only
call setDiscussions(...) on success and add or reuse an error state setter
(e.g., setError or a toast helper) to show the user a message and optionally
clear or preserve the previous list as appropriate.
Comment on lines
+69
to
88
| const handleCreateDiscussion = async () => { | ||
| const user = await getCurrentUserAsync() | ||
| if (!user) return | ||
|
|
||
| const finalCategory = newDiscussion.category === "custom" ? newDiscussion.customCategory : newDiscussion.category | ||
|
|
||
| if (!newDiscussion.title || !newDiscussion.content || !finalCategory) return | ||
|
|
||
| const discussion: Discussion = { | ||
| id: Date.now().toString(), | ||
| title: newDiscussion.title, | ||
| content: newDiscussion.content, | ||
| category: finalCategory, | ||
| author: user.name, | ||
| authorEmail: user.email, | ||
| replies: 0, | ||
| createdAt: new Date().toISOString(), | ||
| } | ||
| await createDiscussion({ | ||
| user_id: user.id, | ||
| title: newDiscussion.title.trim(), | ||
| content: newDiscussion.content.trim(), | ||
| category: finalCategory.trim(), | ||
| tags: [], | ||
| }) | ||
|
|
||
| saveDiscussions([discussion, ...discussions]) | ||
| await loadDiscussions() | ||
| setNewDiscussion({ title: "", content: "", category: "", customCategory: "" }) | ||
| setShowNewDiscussionModal(false) | ||
| } |
There was a problem hiding this comment.
Add error handling and loading state to prevent silent failures and double-submission.
If createDiscussion fails, users receive no feedback about the failure. Additionally, without a loading/disabled state on the submit button, users could accidentally trigger multiple submissions while the async operation is in progress.
🛡️ Proposed fix with error handling and submission guard
Add a loading state to the component:
const [isSubmitting, setIsSubmitting] = useState(false)Then wrap the handler:
const handleCreateDiscussion = async () => {
+ if (isSubmitting) return
const user = await getCurrentUserAsync()
if (!user) return
const finalCategory = newDiscussion.category === "custom" ? newDiscussion.customCategory : newDiscussion.category
if (!newDiscussion.title || !newDiscussion.content || !finalCategory) return
+ setIsSubmitting(true)
+ try {
await createDiscussion({
user_id: user.id,
title: newDiscussion.title.trim(),
content: newDiscussion.content.trim(),
category: finalCategory.trim(),
tags: [],
})
await loadDiscussions()
setNewDiscussion({ title: "", content: "", category: "", customCategory: "" })
setShowNewDiscussionModal(false)
+ } catch (error) {
+ console.error("Failed to create discussion:", error)
+ // Consider showing error toast/message to user
+ } finally {
+ setIsSubmitting(false)
+ }
}And disable the button while submitting:
-<Button onClick={handleCreateDiscussion} className="flex-1">
+<Button onClick={handleCreateDiscussion} disabled={isSubmitting} className="flex-1">
{t.community.post}
</Button>🤖 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 `@app/dashboard/community/page.tsx` around lines 69 - 88, The handler
handleCreateDiscussion lacks error handling and a submission guard which can
cause silent failures and double-submits; add a component state like
isSubmitting via useState(false), return early if isSubmitting at the top of
handleCreateDiscussion, wrap the async sequence (getCurrentUserAsync,
createDiscussion, loadDiscussions, setNewDiscussion, setShowNewDiscussionModal)
in try/catch/finally, setIsSubmitting(true) before the try and
setIsSubmitting(false) in finally, surface errors in the catch (e.g., show a
toast or call processLogger/error handler), and bind isSubmitting to your submit
button's disabled/loading prop so users cannot resubmit while the request is in
flight.
Comment on lines
+498
to
502
| const handleSave = async () => { | ||
| if (!diagram) return | ||
|
|
||
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | ||
| const index = allDiagrams.findIndex((d: any) => d.id === diagram.id) | ||
| if (index !== -1) { | ||
| allDiagrams[index] = { | ||
| ...diagram, | ||
| data, | ||
| updatedAt: new Date().toISOString(), | ||
| } | ||
| localStorage.setItem("lab68_diagrams", JSON.stringify(allDiagrams)) | ||
| alert((t.diagrams as any).saved || "Diagram saved successfully!") | ||
| } | ||
| await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString() }) | ||
| alert((t.diagrams as any).saved || "Diagram saved successfully!") | ||
| } |
There was a problem hiding this comment.
Success alert shown even when save fails.
If updateDiagram throws, the success alert is still displayed, misleading users into thinking their work was saved. Wrap in try/catch.
🛡️ Proposed fix
const handleSave = async () => {
if (!diagram) return
- await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString() })
- alert((t.diagrams as any).saved || "Diagram saved successfully!")
+ try {
+ await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString() })
+ alert((t.diagrams as any).saved || "Diagram saved successfully!")
+ } catch (err) {
+ console.error("Failed to save diagram:", err)
+ alert("Failed to save diagram. Please try again.")
+ }
}📝 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 handleSave = async () => { | |
| if (!diagram) return | |
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | |
| const index = allDiagrams.findIndex((d: any) => d.id === diagram.id) | |
| if (index !== -1) { | |
| allDiagrams[index] = { | |
| ...diagram, | |
| data, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| localStorage.setItem("lab68_diagrams", JSON.stringify(allDiagrams)) | |
| alert((t.diagrams as any).saved || "Diagram saved successfully!") | |
| } | |
| await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString() }) | |
| alert((t.diagrams as any).saved || "Diagram saved successfully!") | |
| } | |
| const handleSave = async () => { | |
| if (!diagram) return | |
| try { | |
| await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString() }) | |
| alert((t.diagrams as any).saved || "Diagram saved successfully!") | |
| } catch (err) { | |
| console.error("Failed to save diagram:", err) | |
| alert("Failed to save diagram. Please try again.") | |
| } | |
| } |
🤖 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 `@app/dashboard/diagrams/`[id]/page.tsx around lines 498 - 502, The success
alert in handleSave is shown unconditionally even if updateDiagram fails; wrap
the await updateDiagram(diagram.id, { data, updated_at: new Date().toISOString()
}) call in a try/catch inside handleSave, only show alert((t.diagrams as
any).saved || "Diagram saved successfully!") on success, and in the catch block
log the error and show a failure alert (or other UI error indication) mentioning
the save failed so users aren't misled; ensure you still return early if
!diagram as before.
Comment on lines
+72
to
93
| const handleCreateDiagram = async () => { | ||
| if (!newDiagram.name.trim() || !user) return | ||
|
|
||
| const diagram: Diagram = { | ||
| id: crypto.randomUUID(), | ||
| name: newDiagram.name, | ||
| description: newDiagram.description, | ||
| userId: user.id, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| data: newDiagram.diagramType === "visual" ? { nodes: [], connections: [] } : {}, | ||
| diagramType: newDiagram.diagramType, | ||
| category: newDiagram.diagramType === "text" ? newDiagram.category : undefined, | ||
| textContent: newDiagram.diagramType === "text" ? getDefaultTemplate(newDiagram.category) : "", | ||
| } | ||
|
|
||
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | ||
| allDiagrams.push(diagram) | ||
| localStorage.setItem("lab68_diagrams", JSON.stringify(allDiagrams)) | ||
| await createDiagram({ | ||
| user_id: user.id, | ||
| title: newDiagram.name.trim(), | ||
| description: newDiagram.description.trim(), | ||
| type: newDiagram.diagramType, | ||
| data: | ||
| newDiagram.diagramType === "visual" | ||
| ? { nodes: [], connections: [], diagramType: "visual" } | ||
| : { | ||
| diagramType: "text", | ||
| category: newDiagram.category, | ||
| textContent: getDefaultTemplate(newDiagram.category), | ||
| }, | ||
| }) | ||
|
|
||
| setDiagrams([...diagrams, diagram]) | ||
| await loadDiagrams(user.id) | ||
| setNewDiagram({ name: "", description: "", diagramType: "text", category: "c4" }) | ||
| setShowCreateModal(false) | ||
| } |
There was a problem hiding this comment.
Missing error handling can show false success state.
If createDiagram throws, the modal still closes and form resets, giving the user a false impression that the diagram was created. Wrap in try/catch and show an error message on failure.
🛡️ Proposed fix
const handleCreateDiagram = async () => {
if (!newDiagram.name.trim() || !user) return
- await createDiagram({
- user_id: user.id,
- title: newDiagram.name.trim(),
- description: newDiagram.description.trim(),
- type: newDiagram.diagramType,
- data:
- newDiagram.diagramType === "visual"
- ? { nodes: [], connections: [], diagramType: "visual" }
- : {
- diagramType: "text",
- category: newDiagram.category,
- textContent: getDefaultTemplate(newDiagram.category),
- },
- })
-
- await loadDiagrams(user.id)
- setNewDiagram({ name: "", description: "", diagramType: "text", category: "c4" })
- setShowCreateModal(false)
+ try {
+ await createDiagram({
+ user_id: user.id,
+ title: newDiagram.name.trim(),
+ description: newDiagram.description.trim(),
+ type: newDiagram.diagramType,
+ data:
+ newDiagram.diagramType === "visual"
+ ? { nodes: [], connections: [], diagramType: "visual" }
+ : {
+ diagramType: "text",
+ category: newDiagram.category,
+ textContent: getDefaultTemplate(newDiagram.category),
+ },
+ })
+
+ await loadDiagrams(user.id)
+ setNewDiagram({ name: "", description: "", diagramType: "text", category: "c4" })
+ setShowCreateModal(false)
+ } catch (err) {
+ console.error("Failed to create diagram:", err)
+ alert("Failed to create diagram. Please try again.")
+ }
}📝 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 handleCreateDiagram = async () => { | |
| if (!newDiagram.name.trim() || !user) return | |
| const diagram: Diagram = { | |
| id: crypto.randomUUID(), | |
| name: newDiagram.name, | |
| description: newDiagram.description, | |
| userId: user.id, | |
| createdAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| data: newDiagram.diagramType === "visual" ? { nodes: [], connections: [] } : {}, | |
| diagramType: newDiagram.diagramType, | |
| category: newDiagram.diagramType === "text" ? newDiagram.category : undefined, | |
| textContent: newDiagram.diagramType === "text" ? getDefaultTemplate(newDiagram.category) : "", | |
| } | |
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | |
| allDiagrams.push(diagram) | |
| localStorage.setItem("lab68_diagrams", JSON.stringify(allDiagrams)) | |
| await createDiagram({ | |
| user_id: user.id, | |
| title: newDiagram.name.trim(), | |
| description: newDiagram.description.trim(), | |
| type: newDiagram.diagramType, | |
| data: | |
| newDiagram.diagramType === "visual" | |
| ? { nodes: [], connections: [], diagramType: "visual" } | |
| : { | |
| diagramType: "text", | |
| category: newDiagram.category, | |
| textContent: getDefaultTemplate(newDiagram.category), | |
| }, | |
| }) | |
| setDiagrams([...diagrams, diagram]) | |
| await loadDiagrams(user.id) | |
| setNewDiagram({ name: "", description: "", diagramType: "text", category: "c4" }) | |
| setShowCreateModal(false) | |
| } | |
| const handleCreateDiagram = async () => { | |
| if (!newDiagram.name.trim() || !user) return | |
| try { | |
| await createDiagram({ | |
| user_id: user.id, | |
| title: newDiagram.name.trim(), | |
| description: newDiagram.description.trim(), | |
| type: newDiagram.diagramType, | |
| data: | |
| newDiagram.diagramType === "visual" | |
| ? { nodes: [], connections: [], diagramType: "visual" } | |
| : { | |
| diagramType: "text", | |
| category: newDiagram.category, | |
| textContent: getDefaultTemplate(newDiagram.category), | |
| }, | |
| }) | |
| await loadDiagrams(user.id) | |
| setNewDiagram({ name: "", description: "", diagramType: "text", category: "c4" }) | |
| setShowCreateModal(false) | |
| } catch (err) { | |
| console.error("Failed to create diagram:", err) | |
| alert("Failed to create diagram. Please try again.") | |
| } | |
| } |
🤖 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 `@app/dashboard/diagrams/page.tsx` around lines 72 - 93, The handler
handleCreateDiagram should not reset the form or close the modal when
createDiagram fails; wrap the createDiagram + loadDiagrams calls in a try/catch,
call createDiagram(...) and await loadDiagrams(user.id) inside the try, and only
then call setNewDiagram(...) and setShowCreateModal(false); in the catch block
log the error and surface it to the user (e.g., set an error state or call your
toast/error notification) so failures don’t give a false success; keep
getDefaultTemplate usage unchanged and ensure user existence check remains
before the try.
Comment on lines
+194
to
199
| const handleDeleteDiagram = async (id: string) => { | ||
| if (!confirm(t.diagrams.confirmDelete)) return | ||
|
|
||
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | ||
| const filtered = allDiagrams.filter((d: Diagram) => d.id !== id) | ||
| localStorage.setItem("lab68_diagrams", JSON.stringify(filtered)) | ||
| await deleteDiagram(id) | ||
| setDiagrams(diagrams.filter((d) => d.id !== id)) | ||
| } |
There was a problem hiding this comment.
Optimistic state update after unguarded async delete.
If deleteDiagram fails, the diagram is removed from UI state but still exists in the database. Either wrap in try/catch and only update state on success, or refetch after delete.
🛡️ Proposed fix
const handleDeleteDiagram = async (id: string) => {
if (!confirm(t.diagrams.confirmDelete)) return
- await deleteDiagram(id)
- setDiagrams(diagrams.filter((d) => d.id !== id))
+ try {
+ await deleteDiagram(id)
+ setDiagrams(diagrams.filter((d) => d.id !== id))
+ } catch (err) {
+ console.error("Failed to delete diagram:", err)
+ alert("Failed to delete diagram. Please try again.")
+ }
}📝 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 handleDeleteDiagram = async (id: string) => { | |
| if (!confirm(t.diagrams.confirmDelete)) return | |
| const allDiagrams = JSON.parse(localStorage.getItem("lab68_diagrams") || "[]") | |
| const filtered = allDiagrams.filter((d: Diagram) => d.id !== id) | |
| localStorage.setItem("lab68_diagrams", JSON.stringify(filtered)) | |
| await deleteDiagram(id) | |
| setDiagrams(diagrams.filter((d) => d.id !== id)) | |
| } | |
| const handleDeleteDiagram = async (id: string) => { | |
| if (!confirm(t.diagrams.confirmDelete)) return | |
| try { | |
| await deleteDiagram(id) | |
| setDiagrams(diagrams.filter((d) => d.id !== id)) | |
| } catch (err) { | |
| console.error("Failed to delete diagram:", err) | |
| alert("Failed to delete diagram. Please try again.") | |
| } | |
| } |
🤖 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 `@app/dashboard/diagrams/page.tsx` around lines 194 - 199, The deletion handler
handleDeleteDiagram performs an unguarded async delete and optimistically
mutates UI via setDiagrams even if deleteDiagram fails; wrap the await
deleteDiagram(id) in a try/catch and only call setDiagrams(diagrams.filter(...))
on success (or, alternatively, call a refetch function after a successful
delete), and on error log/show the failure so the UI remains consistent; update
references to handleDeleteDiagram, deleteDiagram, and setDiagrams accordingly.
Comment on lines
+157
to
170
| const handleSave = async () => { | ||
| if (!diagram) return | ||
| await updateDiagram(diagram.id, { | ||
| data: { | ||
| ...(diagram.data || {}), | ||
| diagramType: "text", | ||
| category: diagram.category || "c4", | ||
| textContent, | ||
| }, | ||
| updated_at: new Date().toISOString(), | ||
| }) | ||
|
|
||
| localStorage.setItem("lab68_diagrams", JSON.stringify(updatedDiagrams)) | ||
| setSavedMessage(true) | ||
| setTimeout(() => setSavedMessage(false), 2000) | ||
| } |
There was a problem hiding this comment.
Success message shown even when save fails.
If updateDiagram throws, setSavedMessage(true) is still called, showing a false success indicator. Users may navigate away thinking their work is saved.
🛡️ Proposed fix
const handleSave = async () => {
if (!diagram) return
- await updateDiagram(diagram.id, {
- data: {
- ...(diagram.data || {}),
- diagramType: "text",
- category: diagram.category || "c4",
- textContent,
- },
- updated_at: new Date().toISOString(),
- })
- setSavedMessage(true)
- setTimeout(() => setSavedMessage(false), 2000)
+ try {
+ await updateDiagram(diagram.id, {
+ data: {
+ ...(diagram.data || {}),
+ diagramType: "text",
+ category: diagram.category || "c4",
+ textContent,
+ },
+ updated_at: new Date().toISOString(),
+ })
+ setSavedMessage(true)
+ setTimeout(() => setSavedMessage(false), 2000)
+ } catch (err) {
+ console.error("Failed to save diagram:", err)
+ setError("Failed to save diagram. Please try again.")
+ }
}🤖 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 `@app/dashboard/diagrams/text/`[id]/page.tsx around lines 157 - 170, handleSave
currently calls setSavedMessage(true) regardless of whether
updateDiagram(diagram.id, ...) succeeds, causing a false success indicator; wrap
the updateDiagram call in a try/catch inside handleSave, only call
setSavedMessage(true) and start the timeout if updateDiagram resolves, and
handle failures in the catch (e.g., surface an error state/message or
console.error) so users are not shown a success when save fails; reference the
handleSave function, updateDiagram call, and setSavedMessage/setTimeout logic to
locate where to add the try/catch and success-only UI update.
Comment on lines
+62
to
+73
| const dashboardUserSnapshot = sidebarUser | ||
| ? Buffer.from( | ||
| JSON.stringify({ | ||
| id: sidebarUser.id, | ||
| email: sidebarUser.email, | ||
| name: sidebarUser.name, | ||
| avatar: sidebarUser.avatar, | ||
| createdAt: user?.created_at || "", | ||
| language: "en", | ||
| }), | ||
| ).toString("base64") | ||
| : "" |
There was a problem hiding this comment.
Hardcoded language: "en" ignores user's actual language preference.
The snapshot hardcodes language: "en" but the profile query (line 44) doesn't fetch the user's language preference. When readDashboardUserSnapshot() in auth-service.ts reads this, it will always cache "en" regardless of the user's actual setting.
Either add language to the profile select query and use it here, or document that this snapshot intentionally uses the default.
Proposed fix
const { data } = await supabase
.from("profiles")
- .select("onboarding_completed, name, avatar")
+ .select("onboarding_completed, name, avatar, language")
.eq("id", user.id)
.single()Then use it in the snapshot:
createdAt: user?.created_at || "",
- language: "en",
+ language: profile?.language || "en",
}),🤖 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 `@app/dashboard/layout.tsx` around lines 62 - 73, The snapshot currently
hardcodes language:"en" which masks the user's real preference; update the
profile SELECT (the profile query referenced near the dashboard layout) to
include the language field and then set dashboardUserSnapshot's language from
the fetched profile (e.g. use user?.language or sidebarUser.language if you add
it there) instead of the literal "en" so readDashboardUserSnapshot in
auth-service.ts will cache the correct language; if you intentionally want a
default, document that behavior and fall back to "en" only when no language is
present.
Comment on lines
94
to
113
| export async function getProjectCollaborators(projectId: string) { | ||
| guard() | ||
| const supabase = createClient() | ||
| const { data, error } = await supabase | ||
| .from('project_collaborators') | ||
| .select(`*, profiles:user_id (id, email, name, avatar)`) | ||
| .eq('project_id', projectId) | ||
| if (error) throw error | ||
| return data || [] | ||
| const res = await fetch(`/api/projects/${projectId}/collaborators`, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| cache: 'no-store', | ||
| }) | ||
|
|
||
| const payload = await res.json().catch(() => ({} as CollaboratorsApiResponse)) as CollaboratorsApiResponse | ||
|
|
||
| if (!res.ok) { | ||
| const error = new Error(payload.error || 'Failed to fetch collaborators') as Error & { status?: number } | ||
| error.status = res.status | ||
| throw error | ||
| } | ||
|
|
||
| return Array.isArray(payload.collaborators) ? payload.collaborators : [] | ||
| } |
There was a problem hiding this comment.
Same relative URL issue applies to getProjectCollaborators.
This function has the same problem — the relative URL /api/projects/${projectId}/collaborators will fail when called from server-side code.
Proposed fix
export async function getProjectCollaborators(projectId: string) {
guard()
- const res = await fetch(`/api/projects/${projectId}/collaborators`, {
+ const res = await fetch(`${getApiBaseUrl()}/api/projects/${projectId}/collaborators`, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
},
cache: 'no-store',
})📝 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 async function getProjectCollaborators(projectId: string) { | |
| guard() | |
| const supabase = createClient() | |
| const { data, error } = await supabase | |
| .from('project_collaborators') | |
| .select(`*, profiles:user_id (id, email, name, avatar)`) | |
| .eq('project_id', projectId) | |
| if (error) throw error | |
| return data || [] | |
| const res = await fetch(`/api/projects/${projectId}/collaborators`, { | |
| method: 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| cache: 'no-store', | |
| }) | |
| const payload = await res.json().catch(() => ({} as CollaboratorsApiResponse)) as CollaboratorsApiResponse | |
| if (!res.ok) { | |
| const error = new Error(payload.error || 'Failed to fetch collaborators') as Error & { status?: number } | |
| error.status = res.status | |
| throw error | |
| } | |
| return Array.isArray(payload.collaborators) ? payload.collaborators : [] | |
| } | |
| export async function getProjectCollaborators(projectId: string) { | |
| guard() | |
| const res = await fetch(`${getApiBaseUrl()}/api/projects/${projectId}/collaborators`, { | |
| method: 'GET', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| cache: 'no-store', | |
| }) | |
| const payload = await res.json().catch(() => ({} as CollaboratorsApiResponse)) as CollaboratorsApiResponse | |
| if (!res.ok) { | |
| const error = new Error(payload.error || 'Failed to fetch collaborators') as Error & { status?: number } | |
| error.status = res.status | |
| throw error | |
| } | |
| return Array.isArray(payload.collaborators) ? payload.collaborators : [] | |
| } |
🤖 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 `@lib/features/projects/index.ts` around lines 94 - 113,
getProjectCollaborators uses a relative fetch URL which breaks when called
server-side; change the fetch call to use an absolute URL by constructing the
endpoint with your app's base/origin instead of "/api/...". Replace the string
`/api/projects/${projectId}/collaborators` with something like
`${getBaseUrl()}/api/projects/${projectId}/collaborators` (or use your existing
helper like getAbsoluteUrl/getOrigin) and import/consume that helper in this
module, keeping the same fetch options, response parsing and error handling in
getProjectCollaborators.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens project access and addresses multiple CodeQL findings by moving sensitive client flows behind server APIs and reducing browser-side persistence.
Changes
project_collaboratorsRLS policieslab68_sessionstorageVerification
npm run buildpassedCommits
fix(projects): move collaborator access behind server APIsfix(security): reduce client-side exposure and harden CIrefactor(workspace): persist community and diagrams in supabaseNotes
SUPABASE_SERVICE_ROLE_KEYconfigured for the server-side project access pathSummary by CodeRabbit
New Features
Bug Fixes
Security