-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: resolve recent projects refresh issues when accessed #2778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- 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
@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-side mutation to record project access and a client-side hook call to invoke it on project start, then invalidates the project list to refresh recent projects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Client Hook (useStartProject)
participant API as trpc project.trackAccess
participant DB as Database
U->>C: Open project
C->>API: trackAccess({ projectId })
API->>DB: Verify membership
alt Authorized
DB-->>API: OK
API->>DB: Update project.updatedAt
DB-->>API: Updated
API-->>C: { success: true }
C->>API: Invalidate project.list query
else Forbidden
API-->>C: TRPCError(FORBIDDEN)
C->>C: Log error (fire-and-forget)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.list
passing alimit
, so this issue isn’t triggered today. If you add alimit
later, either remove it from the DB query and slice after sorting in code (Option A) or, preferably, add anorderBy
onupdatedAt
in 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.
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
trackAccess
mutation 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