fix: resolve recent projects refresh issues when accessed #2778
Conversation
|
@binu-baiju is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a server mutation to record project access (validates membership and updates Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Hook as useStartProject
participant Client as TRPC Client
participant Server as project.trackAccess
participant DB as Database
participant Cache as Query Cache
User->>Hook: Open project
Hook->>Client: trackProjectAccess(projectId)
Client->>Server: RPC trackAccess(input: { projectId })
Server->>DB: Check userProjects for membership
alt membership exists
Server->>DB: Update project.updatedAt
DB-->>Server: ok
Server-->>Client: { success: true }
Client->>Cache: invalidate api.project.list
Cache->>Cache: refetch latest projects
else no membership
Server-->>Client: TRPCError FORBIDDEN
Client->>Client: .catch(console.error) (fire-and-forget)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/client/src/server/api/routers/project/project.ts (3)
426-431: Avoid conflating “last accessed” with project.updatedAt (per-user recency).Updating projects.updatedAt on access will:
- Reorder the project for all members, not just the accessing user.
- Overload “updatedAt” (mutations vs. access), which can ripple into any feature relying on it.
Recommended direction (follow-up, requires migration):
- Add lastAccessedAt timestamp to user_projects (per user, per project).
- Update that field here; sort recent lists by user_projects.lastAccessedAt.
If the column existed, this block would look like:
- await ctx.db.update(projects).set({ - updatedAt: new Date(), - }).where(eq(projects.id, input.projectId)); + await ctx.db.update(userProjects).set({ + // use DB time if available to avoid clock skew (see next comment) + lastAccessedAt: new Date(), + }).where(and( + eq(userProjects.userId, ctx.user.id), + eq(userProjects.projectId, input.projectId), + ));Happy to provide a minimal Drizzle migration and list() query update if you want to pursue this in this PR or a follow-up.
426-429: Prefer database time to avoid clock skew (minor).Using application time can drift across servers. If you have drizzle’s sql available, consider delegating to the DB.
-import { and, eq, ne } from 'drizzle-orm'; +import { and, eq, ne, sql } from 'drizzle-orm'; … - await ctx.db.update(projects).set({ - updatedAt: new Date(), - }).where(eq(projects.id, input.projectId)); + await ctx.db.update(projects).set({ + updatedAt: sql`now()`, + }).where(eq(projects.id, input.projectId));
165-182: Guard against limit-before-sort if future callers uselimit
Our search didn’t find any calls toproject.listpassing alimit, so this issue isn’t triggered today. If you add alimitlater, either remove it from the DB query and slice after sorting in code (Option A) or, preferably, add anorderByonupdatedAtin SQL and keep yourlimit(Option B).apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
50-54: Surface failures via onError instead of per-call .catch (minor UX).Centralizing error handling on the mutation keeps effects clean and makes failures visible to users.
-const { mutateAsync: trackProjectAccess } = api.project.trackAccess.useMutation({ - onSuccess: () => { - // Invalidate project list to refresh recent projects - apiUtils.project.list.invalidate(); - }, -}); +const { mutateAsync: trackProjectAccess } = api.project.trackAccess.useMutation({ + onSuccess: () => { + // Invalidate project list to refresh recent projects + apiUtils.project.list.invalidate(); + }, + onError: (err) => { + console.error('Failed to track project access', err); + toast.error('Failed to record recent activity', { + description: err instanceof Error ? err.message : 'Unknown error', + }); + }, +}); … - trackProjectAccess({ projectId: project.id }).catch(console.error); + void trackProjectAccess({ projectId: project.id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)
apps/web/client/src/server/api/routers/project/project.ts (3)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)packages/db/src/schema/user/user-project.ts (1)
userProjects(10-23)packages/db/src/schema/project/project.ts (1)
projects(12-31)
🔇 Additional comments (2)
apps/web/client/src/server/api/routers/project/project.ts (1)
408-424: Access control: solid membership check and correct error semantics.Verifying membership in userProjects before mutating state and throwing TRPCError(FORBIDDEN) is the right call. This prevents probing and keeps error semantics tight under protectedProcedure.
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
39-44: Good integration: fire-and-forget mutation with list invalidation.Hook wiring looks correct; onSuccess invalidation will refresh the recent projects list as intended.
binu-baiju
changed the title
saddlepaddle
force-pushed
the
clean-bug-fixes
branch
from
3abf2df to
587db4c
Compare
- Fix GitHub profile picture display by using correct avatar_url property - Add trackAccess mutation with proper authorization checks - Integrate project access tracking to refresh recent projects list Closes onlook-dev#2760
saddlepaddle
force-pushed
the
clean-bug-fixes
branch
from
587db4c to
1028bfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
41-45: Consider extracting the mutation options to avoid potential re-creation.The inline options object is recreated on every render. While tRPC typically handles this correctly, extracting the options or using
useCallbackfor theonSuccesshandler ensures the mutation hook remains stable and avoids potential edge cases.Apply this diff to extract the callback:
+ const handleTrackAccessSuccess = useCallback(() => { + apiUtils.project.list.invalidate(); + }, [apiUtils]); + - const { mutateAsync: trackProjectAccess } = api.project.trackAccess.useMutation({ - onSuccess: () => { - apiUtils.project.list.invalidate(); - }, - }); + const { mutateAsync: trackProjectAccess } = api.project.trackAccess.useMutation({ + onSuccess: handleTrackAccessSuccess, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)
🔇 Additional comments (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
62-64: RemovetrackProjectAccessfrom the dependency array or memoize the mutation to prevent excessive re-renders.The
mutateAsyncfunction fromuseMutationis not referentially stable, meaning the mutation object recreates on each render. IncludingtrackProjectAccessin the dependency array causes the effect to run on every render rather than only whenprojectIdchanges, triggering redundant tracking mutations.Recommended fixes:
- Remove
trackProjectAccessfrom deps and rely only oneditorEngine.projectId(if mutation stability is not critical for this effect)- Wrap the effect logic in
useCallbackto memoize the tracking call- Move the mutation to a stable reference using
useRefand async cleanup patternThe current fire-and-forget error handling also silently swallows errors; consider logging them for debugging tracking issues.
[mandatory_refactors_recommended]
| useEffect(() => { | ||
| trackProjectAccess({ projectId: editorEngine.projectId }).catch(console.error); | ||
| }, [editorEngine.projectId, trackProjectAccess]); |
There was a problem hiding this comment.
Fire-and-forget error handling silences all failures.
The .catch(console.error) pattern silently swallows errors from the tracking mutation. While this may be acceptable for non-critical tracking, consider whether authorization failures, network errors, or invalid project IDs should be handled more explicitly (e.g., with a toast notification for unexpected errors).
2 participants
Closes #2760
Description
Fixed critical bug in the Onlook web application affecting user experience:
Recent Projects Refresh Bug: The recent projects list was not updating when users accessed projects, meaning recently opened projects did not move to the top of the list as expected. Added
trackAccessmutation with proper authorization checks to update project timestamps when accessed.Related Issues
Closes #2760
Type of Change
Testing
Manual Testing Performed:
Recent Projects Refresh:
Environment: Local development setup with Supabase backend, Windows 11 with Docker Desktop, tested in Chrome browser
Screenshots (if applicable)
None required - functional bug fix
Additional Notes
Summary by CodeRabbit