feat(desktop): add pro plan paywall dialog#808
Merged
saddlepaddle merged 8 commits intomainfrom Jan 19, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a global Paywall React modal, PRO feature constants/mappings, a usePaywall gating hook that triggers the modal when access is denied, TypeScript declarations for stripe-gradient, and mounts in the authenticated layout. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Feature as Feature\n(caller)
participant Hook as usePaywall\n(hook)
participant PaywallComp as Paywall\n(component)
participant AuthLayout as Authenticated\nLayout
User->>Feature: interact with gated feature
Feature->>Hook: gateFeature(feature, callback, context?)
Hook->>Hook: hasAccess(feature)?
alt access denied
Hook->>PaywallComp: paywall(feature, trackingContext)
PaywallComp->>AuthLayout: render modal (global)
PaywallComp->>User: show PRO list & preview
User->>PaywallComp: select feature / upgrade / cancel
PaywallComp->>Feature: invoke onUpgrade or onCancel callbacks
PaywallComp->>User: close modal
else access granted
Hook->>Feature: execute callback (sync/async)
Feature->>User: perform action
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/components/Paywall/index.ts`:
- Around line 1-3: The export statements in index.ts are unsorted by Biome
(exports: Paywall, paywall, GATED_FEATURES, usePaywall, GatedFeature); run the
repository formatter (Biome) or apply Biome's export-sorting rules to this file
so the exports are organized into the canonical order expected by CI (adjust the
export order in apps/desktop/src/renderer/components/Paywall/index.ts to match
Biome's sorted output for the symbols Paywall, paywall, GATED_FEATURES,
usePaywall, and GatedFeature).
In `@apps/desktop/src/renderer/components/Paywall/Paywall.tsx`:
- Around line 1-8: The import list in Paywall.tsx is unsorted per Biome causing
the lint failure; reorder and/or run the Biome formatter to organize imports
(ensure grouped and alphabetized) for the imported symbols Badge, Button,
Dialog, DialogContent, MeshGradient, cn, useEffect, useState, GatedFeature,
FEATURE_ID_MAP, and PRO_FEATURES so the file follows the project's import
ordering rules and CI passes.
- Around line 10-20: The module-level handler showPaywallFn can hold stale
setters after Paywall unmount; inside the Paywall component add a useEffect that
returns a cleanup function which resets showPaywallFn = null on unmount so
paywall() cannot call setTriggeredFeature or setIsOpen after the component is
gone; reference the existing showPaywallFn assignment and the
setTriggeredFeature/setIsOpen state setters when adding the cleanup.
In `@apps/desktop/src/renderer/components/Paywall/usePaywall.ts`:
- Around line 70-73: Change the gateFeature function to accept a single object
parameter instead of two positional args: use a signature like gateFeature({
feature, callback }: { feature: GatedFeature; callback: () => void |
Promise<void> }) and update the JSDoc examples to show named properties; update
the function body to destructure feature and callback, and modify all call sites
to pass an object ({ feature: ..., callback: ... }) and any tests/uses that rely
on the old positional signature.
Comment on lines
+70
to
+73
| function gateFeature( | ||
| feature: GatedFeature, | ||
| callback: () => void | Promise<void>, | ||
| ): void { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use an object parameter for gateFeature to match codebase guidelines.
This function has two parameters and should take a single object (named properties) for clarity and extensibility. Update the JSDoc examples accordingly.
♻️ Suggested refactor
- function gateFeature(
- feature: GatedFeature,
- callback: () => void | Promise<void>,
- ): void {
+ function gateFeature(params: {
+ feature: GatedFeature;
+ callback: () => void | Promise<void>;
+ }): void {
+ const { feature, callback } = params;
if (hasAccess(feature)) {
// Execute callback - handle both sync and async
const result = callback();As per coding guidelines, use object parameters for functions with 2+ parameters.
📝 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
| function gateFeature( | |
| feature: GatedFeature, | |
| callback: () => void | Promise<void>, | |
| ): void { | |
| function gateFeature(params: { | |
| feature: GatedFeature; | |
| callback: () => void | Promise<void>; | |
| }): void { | |
| const { feature, callback } = params; | |
| if (hasAccess(feature)) { | |
| // Execute callback - handle both sync and async | |
| const result = callback(); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/components/Paywall/usePaywall.ts` around lines 70 -
73, Change the gateFeature function to accept a single object parameter instead
of two positional args: use a signature like gateFeature({ feature, callback }:
{ feature: GatedFeature; callback: () => void | Promise<void> }) and update the
JSDoc examples to show named properties; update the function body to destructure
feature and callback, and modify all call sites to pass an object ({ feature:
..., callback: ... }) and any tests/uses that rely on the old positional
signature.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/components/Paywall/Paywall.tsx`:
- Around line 162-165: The Cancel button currently calls setIsOpen(false)
directly, bypassing the component's cancel callback; update the Button onClick
to invoke the same close path that triggers callbacks.onCancel (for example call
callbacks.onCancel() and then setIsOpen(false), or call the existing close
handler method used elsewhere such as handleClose/handleCancel if present) so
the explicit Cancel action runs callbacks.onCancel before closing the Paywall
component.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/components/Paywall/Paywall.tsx (1)
16-47: Reset the module-level paywall handler on unmount.Without cleanup,
paywall()can call state setters after the component unmounts and trigger React warnings.
Please verify by unmounting<Paywall />and invokingpaywall()in a dev build.🧹 Suggested cleanup
showPaywallFn = (feature: GatedFeature) => { setTriggeredFeature(feature); setIsOpen(true); callbacks.onOpen?.(feature); }; + + useEffect(() => { + return () => { + showPaywallFn = null; + }; + }, []);
Comment on lines
+162
to
+165
| <div className="box-border flex items-center justify-between border-t bg-background px-5 py-4"> | ||
| <Button variant="outline" onClick={() => setIsOpen(false)}> | ||
| Cancel | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Cancel button skips onCancel callback.
setIsOpen(false) bypasses the cancel handler, so callbacks.onCancel won’t run for the explicit Cancel action.
✅ Align Cancel with the same close path
- <Button variant="outline" onClick={() => setIsOpen(false)}>
+ <Button variant="outline" onClick={() => handleOpenChange(false)}>
Cancel
</Button>🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/components/Paywall/Paywall.tsx` around lines 162 -
165, The Cancel button currently calls setIsOpen(false) directly, bypassing the
component's cancel callback; update the Button onClick to invoke the same close
path that triggers callbacks.onCancel (for example call callbacks.onCancel() and
then setIsOpen(false), or call the existing close handler method used elsewhere
such as handleClose/handleCancel if present) so the explicit Cancel action runs
callbacks.onCancel before closing the Paywall component.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts`:
- Around line 160-166: gitDiscardAllStaged currently calls git.reset(["HEAD"])
and git.checkout(["--", "."]) which affects all files; change it to target only
the staged files: use simple-git (via the git variable created in
gitDiscardAllStaged) to list staged paths (e.g., diff --name-only --cached or
equivalent), return immediately if empty, then call git.reset with HEAD limited
to those paths and git.checkout limited to those same paths so only staged
changes are discarded; keep the initial assertRegisteredWorktree(worktreePath)
and the simpleGit(worktreePath) usage.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx`:
- Around line 42-48: The close button in WorkspaceDiffStats.tsx is icon-only and
lacks an accessible name; update the button element (the one with
onClick={onClose} and containing <HiMiniXMark />) to include an appropriate
aria-label (for example "Close" or a contextual label like `Close diff`), so
screen readers can announce its purpose; ensure the label is concise and matches
the button's action.
- Around line 19-46: The hover show/hide and background transition are broken
because the wrapper uses the named group "group/diff" but the hover variants use
the unnamed prefix "group-hover:"; update all occurrences of "group-hover:"
targeting this component (e.g., the background classes in the outer div, the
conditional inner diff-stats div and the close-button classes used when
rendering onClose and Tooltip/TooltipTrigger in WorkspaceDiffStats) to use the
matching named variant "group-hover/diff:". Also extract the hardcoded numeric
tooltip settings into module-level constants (e.g., TOOLTIP_DELAY and
TOOLTIP_SIDE_OFFSET) and replace delayDuration={300} and sideOffset={4} with
those constants.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 468-480: The close button in WorkspaceListItem remains visually
hidden on keyboard focus because it uses "opacity-0" and only becomes visible on
hover; update the button's visibility classes so keyboard focus also shows it
(e.g., add focus-visible:opacity-100 or focus:opacity-100 and optionally
focus-visible:outline or ring classes) on the button inside TooltipTrigger in
WorkspaceListItem.tsx (the button that calls handleDeleteClick and renders
HiMiniXMark) so it becomes visible and accessible to keyboard users.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsx`:
- Around line 40-256: The file defines multiple React components violating the
one-component-per-file rule; extract BaseBranchSelector, StashDropdown,
RefreshButton, and PRStatusLink into their own files (e.g.,
BaseBranchSelector.tsx, StashDropdown.tsx, RefreshButton.tsx, PRStatusLink.tsx),
move their implementations and any hooks/refs they use, export them (default or
named consistently), and update the original ChangesHeader file to import these
components and retain only the ChangesHeader component; ensure to carry over
related props types (e.g., ChangesHeaderProps) and any used imports
(electronTrpc, useChangesStore, usePRStatus, Tooltip, Select, Button, icons,
etc.) so behavior and typings remain identical.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx (1)
40-51: Hoist tooltip timing/offset magic numbers to constants.This aligns with the “no magic numbers” guideline and makes tuning easier later.
♻️ Suggested refactor
import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; import { cn } from "@superset/ui/utils"; import { HiMiniXMark } from "react-icons/hi2"; +const TOOLTIP_DELAY_MS = 300; +const TOOLTIP_SIDE_OFFSET_PX = 4; + interface WorkspaceDiffStatsProps {- <Tooltip delayDuration={300}> + <Tooltip delayDuration={TOOLTIP_DELAY_MS}> ... - <TooltipContent side="top" sideOffset={4}> + <TooltipContent side="top" sideOffset={TOOLTIP_SIDE_OFFSET_PX}>Please verify the chosen values still match the intended UX spec. As per coding guidelines, ...
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
120-127: Add error logging for local changes query failures.
This can fail silently; please log with context (and optionally toast) to meet the error-handling guideline.♻️ Proposed fix
const { data: localChanges } = electronTrpc.changes.getStatus.useQuery( { worktreePath }, { enabled: hasHovered && type === "worktree" && !!worktreePath, staleTime: GITHUB_STATUS_STALE_TIME, + onError: (error) => { + console.error( + "[workspace/changes] Failed to load local changes", + error, + ); + }, }, );As per coding guidelines, please avoid swallowing errors silently. Also verify
useQuerysupportsonErrorin your current tRPC/react-query version.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx (1)
140-197: Prefix new error logs with context tags.🧾 Suggested log prefixes
- console.error("Failed to discard all unstaged:", error); + console.error( + "[changes/discard-all-unstaged] Failed to discard all unstaged:", + error, + ); ... - console.error("Failed to discard all staged:", error); + console.error( + "[changes/discard-all-staged] Failed to discard all staged:", + error, + ); ... - console.error("Failed to stash:", error); + console.error("[changes/stash] Failed to stash:", error); ... - console.error("Failed to stash:", error); + console.error( + "[changes/stash-include-untracked] Failed to stash:", + error, + ); ... - console.error("Failed to pop stash:", error); + console.error("[changes/stash-pop] Failed to pop stash:", error);As per coding guidelines, please use the
[domain/operation] messagelog prefix.
Comment on lines
+160
to
+166
| export async function gitDiscardAllStaged(worktreePath: string): Promise<void> { | ||
| assertRegisteredWorktree(worktreePath); | ||
|
|
||
| const git = simpleGit(worktreePath); | ||
| await git.reset(["HEAD"]); | ||
| await git.checkout(["--", "."]); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git reset HEAD behavior unstaged changes
💡 Result:
Short answer: git reset HEAD (no flags) resets the index/staging area to match HEAD (default mode = --mixed), which unstages any files you previously git-added but leaves your working-tree files unchanged. It does not discard unstaged (working-directory) changes. To discard working-tree edits you must use checkout/restore or --hard. [1][2][3]
Examples:
- Unstage a file you added:
git reset HEAD path/to/file— file becomes "modified" (unstaged) but your edits remain on disk. [2] - Unstage everything:
git reset HEAD(default =git reset --mixed HEAD). [1] - To throw away working-tree changes as well:
git checkout -- path/to/fileorgit restore --source=HEAD --staged path/to/file/git reset --hard HEAD(careful: destroys uncommitted work). [2][1]
References: Git docs (git-reset), Atlassian tutorial, StackOverflow discussion. [1][2][3]
🌐 Web query:
git checkout -- . after git reset HEAD behavior
💡 Result:
Short answer: git reset HEAD unstages changes (sets the index to match HEAD) but leaves your working tree alone; git checkout -- . copies files from the index into the working tree. So if you run git reset HEAD (or git reset HEAD .) to unstage, then git checkout -- . will overwrite your working copies with whatever is now in the index (which after the reset will match HEAD), effectively discarding the unstaged working-tree edits. (git-scm.com)
Citations:
🌐 Web query:
git diff --name-only --cached staged files
💡 Result:
Use:
git diff --name-only --cached
This lists filenames staged in the index (vs HEAD). --staged is a synonym for --cached; you can add --diff-filter=AM to exclude deletions or -z for NUL-separated output for safe piping. [1][2][4]
gitDiscardAllStaged discards unrelated unstaged changes.
git reset HEAD unstages all files and resets the index to match HEAD. Then git checkout -- . restores all tracked files from the now-synchronized index, wiping any unstaged changes in other files. To only discard staged changes, limit both reset and checkout to staged files.
🐛 Proposed fix (scope to staged files)
export async function gitDiscardAllStaged(worktreePath: string): Promise<void> {
assertRegisteredWorktree(worktreePath);
const git = simpleGit(worktreePath);
- await git.reset(["HEAD"]);
- await git.checkout(["--", "."]);
+ const staged = await git.raw(["diff", "--name-only", "--cached", "-z"]);
+ const stagedFiles = staged.split("\0").filter(Boolean);
+ if (stagedFiles.length === 0) return;
+ await git.reset(["HEAD", "--", ...stagedFiles]);
+ await git.checkout(["--", ...stagedFiles]);
}🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts` around
lines 160 - 166, gitDiscardAllStaged currently calls git.reset(["HEAD"]) and
git.checkout(["--", "."]) which affects all files; change it to target only the
staged files: use simple-git (via the git variable created in
gitDiscardAllStaged) to list staged paths (e.g., diff --name-only --cached or
equivalent), return immediately if empty, then call git.reset with HEAD limited
to those paths and git.checkout limited to those same paths so only staged
changes are discarded; keep the initial assertRegisteredWorktree(worktreePath)
and the simpleGit(worktreePath) usage.
Comment on lines
+19
to
+46
| <div | ||
| className={cn( | ||
| "group/diff flex items-center text-[10px] font-mono tabular-nums px-1.5 py-0.5 rounded relative cursor-pointer", | ||
| isActive | ||
| ? "bg-foreground/10 group-hover:bg-transparent" | ||
| : "bg-muted/50 group-hover:bg-transparent", | ||
| )} | ||
| > | ||
| {/* Diff stats - hidden on card hover when onClose provided */} | ||
| <div | ||
| className={ | ||
| onClose | ||
| ? "flex items-center gap-1.5 group-hover:hidden" | ||
| : "flex items-center gap-1.5" | ||
| } | ||
| > | ||
| <span className="text-emerald-500/90">+{additions}</span> | ||
| <span className="text-red-400/90">−{deletions}</span> | ||
| </div> | ||
| {/* X icon - shown on card hover */} | ||
| {onClose && ( | ||
| <Tooltip delayDuration={300}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={onClose} | ||
| className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground" | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "WorkspaceDiffStats.tsx" -type f 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cd /tmp && git ls-files | grep -i "workspacediffstats"Repository: superset-sh/superset
Length of output: 132
🏁 Script executed:
cat -n /tmp/apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx 2>/dev/null || echo "File not found at expected path"Repository: superset-sh/superset
Length of output: 95
🏁 Script executed:
# Search for tailwind config files
find /tmp -name "tailwind.config.*" -type f 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Also check for the file in a broader search
fd "WorkspaceDiffStats" --extension tsx 2>/dev/nullRepository: superset-sh/superset
Length of output: 171
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsxRepository: superset-sh/superset
Length of output: 2014
🏁 Script executed:
# Find and inspect Tailwind config
fd "tailwind.config" --extension ts --extension jsRepository: superset-sh/superset
Length of output: 97
🏁 Script executed:
# Check package.json or tsconfig for project config hints
head -50 tailwind.config.ts 2>/dev/null || head -50 tailwind.config.js 2>/dev/null || echo "Check for tailwind config in root"Repository: superset-sh/superset
Length of output: 97
🏁 Script executed:
# Look for similar group-hover patterns in the codebase to understand the pattern
rg "group/\w+" --no-heading -A2 -B2 | head -100Repository: superset-sh/superset
Length of output: 8048
Fix named group hover classes so hover UI actually toggles.
The code defines group/diff on line 21 but uses group-hover: (unnamed) on lines 23, 24, 31, and 45 instead of group-hover/diff: (named). This prevents the hide/show behavior and background transition from triggering.
✅ Suggested fix
- className={cn(
- "group/diff flex items-center text-[10px] font-mono tabular-nums px-1.5 py-0.5 rounded relative cursor-pointer",
- isActive
- ? "bg-foreground/10 group-hover:bg-transparent"
- : "bg-muted/50 group-hover:bg-transparent",
- )}
+ className={cn(
+ "group/diff flex items-center text-[10px] font-mono tabular-nums px-1.5 py-0.5 rounded relative cursor-pointer",
+ isActive
+ ? "bg-foreground/10 hover:bg-transparent"
+ : "bg-muted/50 hover:bg-transparent",
+ )}- onClose
- ? "flex items-center gap-1.5 group-hover:hidden"
- : "flex items-center gap-1.5"
+ onClose
+ ? "flex items-center gap-1.5 group-hover/diff:hidden"
+ : "flex items-center gap-1.5"- className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground"
+ className="hidden group-hover/diff:flex items-center justify-center text-muted-foreground hover:text-foreground"Extract hardcoded values delayDuration={300} and sideOffset={4} to named constants at the module top.
📝 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
| <div | |
| className={cn( | |
| "group/diff flex items-center text-[10px] font-mono tabular-nums px-1.5 py-0.5 rounded relative cursor-pointer", | |
| isActive | |
| ? "bg-foreground/10 group-hover:bg-transparent" | |
| : "bg-muted/50 group-hover:bg-transparent", | |
| )} | |
| > | |
| {/* Diff stats - hidden on card hover when onClose provided */} | |
| <div | |
| className={ | |
| onClose | |
| ? "flex items-center gap-1.5 group-hover:hidden" | |
| : "flex items-center gap-1.5" | |
| } | |
| > | |
| <span className="text-emerald-500/90">+{additions}</span> | |
| <span className="text-red-400/90">−{deletions}</span> | |
| </div> | |
| {/* X icon - shown on card hover */} | |
| {onClose && ( | |
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={onClose} | |
| className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| > | |
| <div | |
| className={cn( | |
| "group/diff flex items-center text-[10px] font-mono tabular-nums px-1.5 py-0.5 rounded relative cursor-pointer", | |
| isActive | |
| ? "bg-foreground/10 hover:bg-transparent" | |
| : "bg-muted/50 hover:bg-transparent", | |
| )} | |
| > | |
| {/* Diff stats - hidden on card hover when onClose provided */} | |
| <div | |
| className={ | |
| onClose | |
| ? "flex items-center gap-1.5 group-hover/diff:hidden" | |
| : "flex items-center gap-1.5" | |
| } | |
| > | |
| <span className="text-emerald-500/90">+{additions}</span> | |
| <span className="text-red-400/90">−{deletions}</span> | |
| </div> | |
| {/* X icon - shown on card hover */} | |
| {onClose && ( | |
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={onClose} | |
| className="hidden group-hover/diff:flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| > |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx`
around lines 19 - 46, The hover show/hide and background transition are broken
because the wrapper uses the named group "group/diff" but the hover variants use
the unnamed prefix "group-hover:"; update all occurrences of "group-hover:"
targeting this component (e.g., the background classes in the outer div, the
conditional inner diff-stats div and the close-button classes used when
rendering onClose and Tooltip/TooltipTrigger in WorkspaceDiffStats) to use the
matching named variant "group-hover/diff:". Also extract the hardcoded numeric
tooltip settings into module-level constants (e.g., TOOLTIP_DELAY and
TOOLTIP_SIDE_OFFSET) and replace delayDuration={300} and sideOffset={4} with
those constants.
Comment on lines
+42
to
+48
| <button | ||
| type="button" | ||
| onClick={onClose} | ||
| className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground" | ||
| > | ||
| <HiMiniXMark className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible label to the icon-only close button.
Tooltips don’t provide an accessible name for screen readers; the button needs an aria-label.
✅ Suggested fix
<button
type="button"
+ aria-label="Close workspace"
onClick={onClose}
className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground"
>📝 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
| <button | |
| type="button" | |
| onClick={onClose} | |
| className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| > | |
| <HiMiniXMark className="size-3.5" /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="Close workspace" | |
| onClick={onClose} | |
| className="hidden group-hover:flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| > | |
| <HiMiniXMark className="size-3.5" /> | |
| </button> |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceDiffStats.tsx`
around lines 42 - 48, The close button in WorkspaceDiffStats.tsx is icon-only
and lacks an accessible name; update the button element (the one with
onClick={onClose} and containing <HiMiniXMark />) to include an appropriate
aria-label (for example "Close" or a contextual label like `Close diff`), so
screen readers can announce its purpose; ensure the label is concise and matches
the button's action.
Comment on lines
+468
to
+480
| <Tooltip delayDuration={300}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| handleDeleteClick(); | ||
| }} | ||
| className="opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center text-muted-foreground hover:text-foreground" | ||
| aria-label="Close workspace" | ||
| > | ||
| <HiMiniXMark className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Make the close button visible on keyboard focus.
The button remains visually hidden on focus due to opacity-0, which hurts keyboard accessibility.
♿️ Suggested tweak
- className="opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center text-muted-foreground hover:text-foreground"
+ className="opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity flex items-center justify-center text-muted-foreground hover:text-foreground"📝 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
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleDeleteClick(); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| aria-label="Close workspace" | |
| > | |
| <HiMiniXMark className="size-3.5" /> | |
| </button> | |
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleDeleteClick(); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 focus-visible:opacity-100 transition-opacity flex items-center justify-center text-muted-foreground hover:text-foreground" | |
| aria-label="Close workspace" | |
| > | |
| <HiMiniXMark className="size-3.5" /> | |
| </button> |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`
around lines 468 - 480, The close button in WorkspaceListItem remains visually
hidden on keyboard focus because it uses "opacity-0" and only becomes visible on
hover; update the button's visibility classes so keyboard focus also shows it
(e.g., add focus-visible:opacity-100 or focus:opacity-100 and optionally
focus-visible:outline or ring classes) on the button inside TooltipTrigger in
WorkspaceListItem.tsx (the button that calls handleDeleteClick and renders
HiMiniXMark) so it becomes visible and accessible to keyboard users.
Comment on lines
+40
to
256
| function BaseBranchSelector({ worktreePath }: { worktreePath: string }) { | ||
| const { baseBranch, setBaseBranch } = useChangesStore(); | ||
| const { data: branchData, isLoading } = | ||
| electronTrpc.changes.getBranches.useQuery( | ||
| { worktreePath }, | ||
| { enabled: !!worktreePath }, | ||
| ); | ||
|
|
||
| const effectiveBaseBranch = baseBranch ?? branchData?.defaultBranch ?? "main"; | ||
| const sortedBranches = [...(branchData?.remote ?? [])].sort((a, b) => { | ||
| if (a === branchData?.defaultBranch) return -1; | ||
| if (b === branchData?.defaultBranch) return 1; | ||
| return a.localeCompare(b); | ||
| }); | ||
|
|
||
| const handleChange = (value: string) => { | ||
| if (value === branchData?.defaultBranch && baseBranch === null) return; | ||
| setBaseBranch(value); | ||
| }; | ||
|
|
||
| if (isLoading || !branchData) { | ||
| return ( | ||
| <span className="px-1.5 py-0.5 rounded bg-muted/50 text-foreground text-[10px] font-medium truncate"> | ||
| {effectiveBaseBranch} | ||
| </span> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Tooltip> | ||
| <Select value={effectiveBaseBranch} onValueChange={handleChange}> | ||
| <TooltipTrigger asChild> | ||
| <SelectTrigger | ||
| size="sm" | ||
| className="h-5 px-1.5 py-0 text-[10px] font-medium border-none bg-muted/50 hover:bg-muted text-foreground min-w-0 w-auto gap-0.5 rounded" | ||
| > | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| </TooltipTrigger> | ||
| <SelectContent align="start"> | ||
| {sortedBranches | ||
| .filter((branch) => branch) | ||
| .map((branch) => ( | ||
| <SelectItem key={branch} value={branch} className="text-xs"> | ||
| {branch} | ||
| {branch === branchData.defaultBranch && ( | ||
| <span className="ml-1 text-muted-foreground">(default)</span> | ||
| )} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Change base branch | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ); | ||
| } | ||
|
|
||
| function StashDropdown({ | ||
| onStash, | ||
| onStashIncludeUntracked, | ||
| onStashPop, | ||
| isPending, | ||
| }: { | ||
| onStash: () => void; | ||
| onStashIncludeUntracked: () => void; | ||
| onStashPop: () => void; | ||
| isPending: boolean; | ||
| }) { | ||
| return ( | ||
| <DropdownMenu> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-6 p-0" | ||
| disabled={isPending} | ||
| > | ||
| <VscGitStash className="size-4" /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Stash operations | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| <DropdownMenuContent align="start" className="w-52"> | ||
| <DropdownMenuItem onClick={onStash} className="text-xs"> | ||
| <VscGitStash className="size-4" /> | ||
| Stash Changes | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem onClick={onStashIncludeUntracked} className="text-xs"> | ||
| <VscGitStash className="size-4" /> | ||
| Stash (Include Untracked) | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSeparator /> | ||
| <DropdownMenuItem onClick={onStashPop} className="text-xs"> | ||
| <VscGitStashApply className="size-4" /> | ||
| Pop Stash | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| ); | ||
| } | ||
|
|
||
| function RefreshButton({ onRefresh }: { onRefresh: () => void }) { | ||
| const [isSpinning, setIsSpinning] = useState(false); | ||
| const timeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
|
|
||
| const handleRefresh = () => { | ||
| setIsManualRefresh(true); | ||
| const handleClick = () => { | ||
| setIsSpinning(true); | ||
| onRefresh(); | ||
| // Clear any existing timeout | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
| // Stop spinning after a short delay | ||
| timeoutRef.current = setTimeout(() => { | ||
| setIsManualRefresh(false); | ||
| }, 600); | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| timeoutRef.current = setTimeout(() => setIsSpinning(false), 600); | ||
| }; | ||
|
|
||
| // Cleanup timeout on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| timeoutRef.current = null; | ||
| } | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| }; | ||
| }, []); | ||
|
|
||
| const { baseBranch, setBaseBranch } = useChangesStore(); | ||
|
|
||
| const { data: branchData, isLoading } = | ||
| electronTrpc.changes.getBranches.useQuery( | ||
| { worktreePath }, | ||
| { enabled: !!worktreePath }, | ||
| ); | ||
| return ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={handleClick} | ||
| disabled={isSpinning} | ||
| className="size-6 p-0" | ||
| > | ||
| <HiArrowPath | ||
| className={`size-3.5 ${isSpinning ? "animate-spin" : ""}`} | ||
| /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Refresh changes | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ); | ||
| } | ||
|
|
||
| const { pr, isLoading: isPRLoading } = usePRStatus({ | ||
| function PRStatusLink({ workspaceId }: { workspaceId?: string }) { | ||
| const { pr, isLoading } = usePRStatus({ | ||
| workspaceId, | ||
| refetchInterval: 10000, | ||
| }); | ||
|
|
||
| const effectiveBaseBranch = baseBranch ?? branchData?.defaultBranch ?? "main"; | ||
| const availableBranches = branchData?.remote ?? []; | ||
| if (isLoading) { | ||
| return ( | ||
| <LuLoaderCircle className="w-4 h-4 animate-spin text-muted-foreground" /> | ||
| ); | ||
| } | ||
|
|
||
| const sortedBranches = [...availableBranches].sort((a, b) => { | ||
| if (a === branchData?.defaultBranch) return -1; | ||
| if (b === branchData?.defaultBranch) return 1; | ||
| return a.localeCompare(b); | ||
| }); | ||
| if (!pr) return null; | ||
|
|
||
| const handleChange = (value: string) => { | ||
| if (value === branchData?.defaultBranch && baseBranch === null) { | ||
| return; | ||
| } | ||
| setBaseBranch(value); | ||
| }; | ||
| return ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <a | ||
| href={pr.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="flex items-center gap-1 hover:opacity-80 transition-opacity" | ||
| > | ||
| <PRIcon state={pr.state} className="w-4 h-4" /> | ||
| <span className="text-xs text-muted-foreground font-mono"> | ||
| #{pr.number} | ||
| </span> | ||
| </a> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| View PR on GitHub | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ); | ||
| } | ||
|
|
||
| export function ChangesHeader({ | ||
| onRefresh, | ||
| viewMode, | ||
| onViewModeChange, | ||
| worktreePath, | ||
| workspaceId, | ||
| onStash, | ||
| onStashIncludeUntracked, | ||
| onStashPop, | ||
| isStashPending, | ||
| }: ChangesHeaderProps) { | ||
| return ( | ||
| <div className="flex items-center justify-between gap-1.5 px-2 py-1.5"> | ||
| <div className="flex items-center gap-1 min-w-0 flex-1"> | ||
| <div className="flex flex-col"> | ||
| <div className="flex items-center gap-1.5 px-2 py-1.5"> | ||
| <span className="text-[10px] text-muted-foreground shrink-0"> | ||
| Base: | ||
| </span> | ||
| {isLoading || !branchData ? ( | ||
| <span className="px-1.5 py-0.5 rounded bg-muted/50 text-foreground text-[10px] font-medium truncate"> | ||
| {effectiveBaseBranch} | ||
| </span> | ||
| ) : ( | ||
| <Tooltip> | ||
| <Select value={effectiveBaseBranch} onValueChange={handleChange}> | ||
| <TooltipTrigger asChild> | ||
| <SelectTrigger | ||
| size="sm" | ||
| className="h-5 px-1.5 py-0 text-[10px] font-medium border-none bg-muted/50 hover:bg-muted text-foreground min-w-0 w-auto gap-0.5 rounded" | ||
| > | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| </TooltipTrigger> | ||
| <SelectContent align="start"> | ||
| {sortedBranches | ||
| .filter((branch) => branch) | ||
| .map((branch) => ( | ||
| <SelectItem key={branch} value={branch} className="text-xs"> | ||
| {branch} | ||
| {branch === branchData.defaultBranch && ( | ||
| <span className="ml-1 text-muted-foreground"> | ||
| (default) | ||
| </span> | ||
| )} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Change base branch | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| )} | ||
| <BaseBranchSelector worktreePath={worktreePath} /> | ||
| </div> | ||
| <div className="flex items-center shrink-0"> | ||
|
|
||
| <div className="flex items-center gap-0.5 px-2 pb-1.5"> | ||
| <StashDropdown | ||
| onStash={onStash} | ||
| onStashIncludeUntracked={onStashIncludeUntracked} | ||
| onStashPop={onStashPop} | ||
| isPending={isStashPending} | ||
| /> | ||
| <ViewModeToggle | ||
| viewMode={viewMode} | ||
| onViewModeChange={onViewModeChange} | ||
| /> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={handleRefresh} | ||
| disabled={isManualRefresh} | ||
| className="size-6 p-0" | ||
| > | ||
| <HiArrowPath | ||
| className={`size-3.5 ${isManualRefresh ? "animate-spin" : ""}`} | ||
| /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| Refresh changes | ||
| </TooltipContent> | ||
| </Tooltip> | ||
|
|
||
| {/* PR Status Icon */} | ||
| {isPRLoading ? ( | ||
| <LuLoaderCircle className="w-4 h-4 animate-spin text-muted-foreground shrink-0" /> | ||
| ) : pr ? ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <a | ||
| href={pr.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="flex items-center gap-1 shrink-0 hover:opacity-80 transition-opacity" | ||
| > | ||
| <PRIcon state={pr.state} className="w-4 h-4" /> | ||
| <span className="text-xs text-muted-foreground font-mono"> | ||
| #{pr.number} | ||
| </span> | ||
| </a> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" showArrow={false}> | ||
| View PR on GitHub | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ) : null} | ||
| <RefreshButton onRefresh={onRefresh} /> | ||
| <PRStatusLink workspaceId={workspaceId} /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split helper components into their own files.
This file now defines BaseBranchSelector, StashDropdown, RefreshButton, PRStatusLink, and ChangesHeader, which conflicts with the one-component-per-file rule. Please extract each helper component to its own file and keep ChangesHeader as the sole component here. As per coding guidelines, one component per file.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/ChangesHeader/ChangesHeader.tsx`
around lines 40 - 256, The file defines multiple React components violating the
one-component-per-file rule; extract BaseBranchSelector, StashDropdown,
RefreshButton, and PRStatusLink into their own files (e.g.,
BaseBranchSelector.tsx, StashDropdown.tsx, RefreshButton.tsx, PRStatusLink.tsx),
move their implementations and any hooks/refs they use, export them (default or
named consistently), and update the original ChangesHeader file to import these
components and retain only the ChangesHeader component; ensure to carry over
related props types (e.g., ChangesHeaderProps) and any used imports
(electronTrpc, useChangesStore, usePRStatus, Tooltip, Select, Button, icons,
etc.) so behavior and typings remain identical.
- Implement paywall UI with Alerter-style pattern (no Zustand) - Add usePaywall hook for feature gating with type-safe constants - Co-locate component + hook + constants in Paywall/ folder - Support async callbacks in gateFeature() - Add mesh gradient backgrounds matching marketing site - Update copy to focus on team collaboration & multi-agent workflows
- Implement paywall UI with Alerter-style pattern (no Zustand) - Add usePaywall hook for feature gating with type-safe constants - Co-locate component + hook + constants in Paywall/ folder - Support async callbacks in gateFeature() - Add optional analytics callbacks (onOpen, onUpgrade, onCancel) - Remove unnecessary comments for cleaner code - Fix import paths to use co-located modules
- Add useEffect cleanup to reset showPaywallFn = null when Paywall unmounts - Prevents stale state setters from being called after component unmounts
- Add apps/desktop/src/types/stripe-gradient.d.ts with proper type definitions - Matches approach used in marketing and web apps - Avoids hacky tsconfig includes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/types/stripe-gradient.d.ts`:
- Around line 1-16: The Gradient class declaration is missing properties used
elsewhere; update the Gradient class (not MiniGl) to include the instance
properties matching the real shape: add conf (use a loose type like
Record<string, any> or an interface), el (HTMLElement | null), and uniforms
(Record<string, any> or an appropriate typed map), and ensure the existing
methods such as play(), pause(), disconnect() remain; this aligns the
declaration with the GradientInstance shape used in mesh-gradient.tsx and
restores type safety.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/components/Paywall/Paywall.tsx (1)
169-172: Cancel should use the same close path to trigger onCancel.Directly setting
isOpenbypasses the cancel callback. Align it with the close handler.✅ Suggested fix
- <Button variant="outline" onClick={() => setIsOpen(false)}> + <Button variant="outline" onClick={() => handleOpenChange(false)}> Cancel </Button>
- Remove PaywallCallbacks type and props - Remove module-level callbacks variable - Simplify to just show/hide dialog - Consumers can add their own tracking if needed
- Add optional context parameter to gateFeature() - Logs context when paywall is triggered - Allows consumers to track source/origin of paywall triggers
- Auto-include userId, organizationId, and userPlan from session - Consumer only needs to pass additional context like source/location - Merge custom context with internal context
- Pass options through showPaywallFn and store in state (like Alerter) - Remove userId from tracking (already identified in authenticated route) - Auto-include organizationId and userPlan in tracking context - Consumers only pass additional context like source/location
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
4 tasks
This was referenced Feb 10, 2026
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
usePaywallhook for feature gating with type-safeGATED_FEATURESconstantsPaywall/foldergateFeature()Architecture
Follows the
Alerterpattern frompackages/ui:<Paywall />component at app rootpaywall(feature)function from anywhere to show dialogUsage
```tsx
import { usePaywall, GATED_FEATURES } from 'renderer/components/Paywall';
const { gateFeature } = usePaywall();
// Guard a feature
gateFeature(GATED_FEATURES.INVITE_MEMBERS, async () => {
await inviteTeamMember();
});
```
Test Plan
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.