feat: workspace instruction modal with glassmorphism UI#186
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 3d35b32 in 9 seconds. Click for details.
- Reviewed
282lines of code in7files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_x5wB0Fr5vTHzbbOF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces localStorage/userId-based first-open detection with an Changes
Sequence Diagram(s)sequenceDiagram
participant Dashboard as Dashboard
participant WC as WorkspaceContext
participant Hook as useWorkspaceInstructionModal
participant Modal as WorkspaceInstructionModal
Dashboard->>WC: request allWorkspaces, loadingAllWorkspaces
WC-->>Dashboard: return allWorkspaces, loadingAllWorkspaces
Dashboard->>Hook: init(workspaceId, isFirstWorkspace)
Hook->>Hook: check hasShownFirstOpenRef (in-memory)
alt isFirstWorkspace && not shown
Hook->>Modal: open(mode="first-open", canClose=false, showFallback=...)
else
Hook->>Modal: normal/autogen initialization
end
Modal-->>Dashboard: user interacts / closes modal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile OverviewGreptile SummaryThis PR refactors the workspace instruction modal with glassmorphism UI and simplifies the trigger mechanism from localStorage-based tracking to workspace count-based logic. The changes include: Major changes:
Code quality improvements:
Potential issues identified:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Dashboard
participant Hook as useWorkspaceInstructionModal
participant Modal as WorkspaceInstructionModal
participant Carousel as useCarousel
User->>Dashboard: Navigate to workspace
Dashboard->>Hook: workspaceId, isFirstWorkspace, assistantIsRunning
alt isFirstWorkspace && !hasShownFirstOpenRef
Hook->>Hook: hasShownFirstOpenRef = true
Hook->>Hook: setOpen(true), setMode("first-open")
Hook->>Modal: open=true, mode="first-open", canClose=false
Modal->>Carousel: Initialize carousel (activeIndex=0)
Carousel->>Carousel: Start video playback
Hook->>Hook: Start 7s timer for canClose
Hook-->>Hook: After 7s: setCanClose(true)
Hook->>Modal: canClose=true
User->>Modal: Click close button or ESC
Modal->>Hook: onRequestClose()
Hook->>Hook: closeInternal("manual")
else isAutogenRoute
Hook->>Hook: setOpen(true), setMode("autogen")
Hook->>Modal: open=true, mode="autogen", isGenerating=true
Hook->>Hook: Start 45s fallback timer
alt assistantIsRunning becomes false
Hook->>Hook: seenRunningInCurrentAutogenRef
alt userInteractedRef is true
Hook->>Hook: setGenerationComplete(true)
Hook->>Modal: isGenerating=false
User->>Modal: Click "Continue to workspace"
Modal->>Hook: onFallbackContinue()
else userInteractedRef is false
Hook->>Hook: closeInternal("autogen_complete")
end
else 45s timeout
Hook->>Hook: setShowFallback(true)
Hook->>Modal: showFallback=true
User->>Modal: Click "Continue to workspace"
Modal->>Hook: onFallbackContinue()
end
end
User->>Modal: Click chevron/dot navigation
Modal->>Carousel: goTo(index) / goNext() / goPrev()
Carousel->>Carousel: transitionTo(nextIndex)
Carousel->>Carousel: setFading(true), wait 250ms
Carousel->>Carousel: setActiveIndex(nextIndex), setFading(false)
Modal->>Hook: onUserInteracted()
Hook->>Hook: userInteractedRef = true, pausedRef = true
Carousel->>Carousel: Video ends
Carousel->>Carousel: handleVideoEnded() -> auto-advance
|
| // First workspace ever — show intro modal (once per session) | ||
| if (isFirstWorkspace && !hasShownFirstOpenRef.current) { | ||
| hasShownFirstOpenRef.current = true; | ||
| setOpen(true); | ||
| setMode("first-open"); | ||
| setCanClose(false); | ||
| setShowFallback(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
hasShownFirstOpenRef persists across workspace changes - if user navigates from workspace A (with 2 total) to workspace B (user's first ever), modal won't show for B because ref is already true from session
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/onboarding/WorkspaceInstructionModal.tsx (2)
371-373:⚠️ Potential issue | 🟡 Minor
whitespace-nowrapmay clip longer descriptions on narrower viewports.Several step descriptions (e.g., "Upload a PDF and have the AI automatically create summaries and study guides.") are fairly long. With
whitespace-nowrapon a container maxed at 1100 px, text will overflow or be clipped on smaller screens or when the modal is padded.Consider removing
whitespace-nowrapor replacing it withtext-wrap/text-balanceto allow graceful wrapping.Proposed fix
- <p className="text-center whitespace-nowrap text-sm text-sidebar-foreground/70"> + <p className="text-center text-sm text-sidebar-foreground/70">
250-259:⚠️ Potential issue | 🟡 MinorModal lacks a focus trap — keyboard users can tab behind the overlay.
The dialog sets
aria-modal="true"but doesn't trap focus. When the modal is open, pressing Tab lets focus escape to elements behind the backdrop. This is an accessibility gap for keyboard and screen-reader users.Consider adding a focus-trap (e.g.,
focus-trap-react, RadixDialog, or a manual sentinel approach) to keep focus within the modal while it's visible.
🧹 Nitpick comments (2)
src/hooks/workspace/use-workspace-instruction-modal.ts (1)
147-156: Verify the suppressed exhaustive-deps warning is safe.The effect on Line 156 excludes
openandmodefrom the dependency array but references them on Line 143. This is intentional to avoid re-triggering when the effect itself setsopen/mode, but it means theopen && mode === "autogen"early-return guard (Line 143) won't re-evaluate if those values change from outside this effect.Since
openandmodeare only mutated inside this same effect and the autogen completion effect, this appears safe today. Just ensure future changes don't introduce other paths that setopen/modeindependently, or the guard could go stale.src/components/onboarding/WorkspaceInstructionModal.tsx (1)
260-267: Clicking the modal body pauses auto-advance but doesn't stop video playback.The outer
onClickcallspause()(which setspausedRef = trueand clears the fallback timer) andonUserInteracted?.(). However, the currently playing video continues — and when it ends,handleVideoEnded(Line 152) still callstransitionTo, advancing the slide regardless of the paused state. This is documented as intentional (Line 153), but it means "pause" only affects icon-only slides, not video slides. If the intent is for user clicks to fully pause progression,handleVideoEndedshould also checkpausedRef.
|



Summary
backdrop-blur,backdrop-saturate, inset highlights, glass-styled chevrons/buttons, gradient orbsinstruction-modal.ts, unused props,markNewWorkspaceInstructioncallsTest plan
Important
Introduces a glassmorphism UI workspace instruction modal with simplified triggers and bug fixes, while cleaning up dead code.
WorkspaceInstructionModalwith a carousel walkthrough for new users inWorkspaceInstructionModal.tsx.use-workspace-instruction-modal.ts.backdrop-blur,backdrop-saturate, and gradient orbs inWorkspaceInstructionModal.tsx.instruction-modal.tsand related dead code.globals.css.This description was created by
for 3d35b32. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Style
Refactor
Behavior