Dev - #24
Merged
Merged
Conversation
Replace hardcoded colors and styles in dashboard components and layout with CSS custom properties for dynamic theming support. Introduce ThemeToggle component for user-controlled theme switching.
…faults Replace remaining hardcoded color values in components and styles with CSS custom properties for full theming support. Switch default theme to dark mode, update light mode color palette for improved contrast and consistency, and refine theme resolution logic in ThemeProvider. This enhances maintainability and ensures uniform application of themes across layouts and UI elements.
…ming Add Portal component for improved modal rendering, refactor CreateExpenseModal and CreateGroupModal to use portals with updated z-indexes and backgrounds. Introduce cardSolid color variable for solid backgrounds in themes. Update dashboard to dynamically use group currency and label multi-group balances. BREAKING CHANGE: Modal z-indexes adjusted for layering consistency.
Introduce soft delete for groups in the database schema and backend service, allowing group owners to delete groups via a new API endpoint. Update frontend to include a delete group modal in group actions, with proper confirmation and action handling. Modify queries to exclude deleted groups from user summaries and dashboards. BREAKING CHANGE: Group deletion now permanently removes group data from active views and balances.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds soft-delete support for groups ( Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Web Client
participant Modal as DeleteGroupModal
participant Action as deleteGroupAction
participant API as Backend API
participant Service as GroupsService
participant DB as Database
participant Redis as RedisService
User->>Client: Click "Delete group"
Client->>Modal: Open DeleteGroupModal
User->>Modal: Confirm deletion
Modal->>Modal: setIsDeleting(true)
Modal->>Action: deleteGroupAction(groupId)
Action->>API: DELETE /groups/:id (Auth)
API->>Service: groupsService.delete(groupId, userId)
Service->>Service: Verify user is OWNER
Service->>DB: Update group (set deletedAt, shareEnabled=false, clear token)
DB-->>Service: Success
Service->>Redis: invalidateGroupCache(groupId)
Service->>Redis: invalidateUserDashboardCache(userId)
Service-->>API: { success: true }
API-->>Action: 200 OK
Action-->>Modal: { success: true }
Modal->>Client: Show toast, close modal, navigate /dashboard, refresh
Client->>User: Display updated dashboard (soft-deleted group excluded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/backend/src/groups/groups.service.ts (1)
253-270:⚠️ Potential issue | 🔴 CriticalRestore the missing
findManycall ingetDashboard().This block no longer parses:
where,include, andorderByare floating inside the function body, andgroupsis never declared. The current PR breaks backend compilation.Minimal fix
async getDashboard(userId: string): Promise<GroupDashboardDto> { + const groups = await this.prisma.group.findMany({ where: { members: { some: { userId, }, }, deletedAt: null, }, include: { _count: { select: { members: true, }, }, }, orderBy: { createdAt: 'desc' }, }); const groupIds = groups.map((group) => group.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/groups/groups.service.ts` around lines 253 - 270, The function getDashboard has stray query option objects and never calls the ORM; wrap the existing where/include/orderBy into an awaited Prisma findMany call and assign its result to a groups variable. Specifically, replace the floating blocks with: const groups = await this.prisma.group.findMany({ where: { members: { some: { userId } }, deletedAt: null }, include: { _count: { select: { members: true } } }, orderBy: { createdAt: 'desc' } }); so subsequent logic in getDashboard() can use the populated groups array.apps/web/src/components/groups/GroupActions.tsx (1)
194-223:⚠️ Potential issue | 🟠 MajorHide the delete flow for non-owners.
The backend only allows
OWNERusers to delete a group, but this button and modal are rendered for everyone. Every regular member will hit a guaranteed forbidden response here.Suggested direction
type GroupActionsProps = { groupId: string; groupName: string; + canDelete: boolean; currency: CurrencyCode; members: GroupMemberSummaryDto[]; shareEnabled: boolean; shareToken?: string | null; defaultSplitPreference?: GroupDefaultSplitDto | null; }; export function GroupActions({ groupId, groupName, + canDelete, currency, members, shareEnabled, shareToken, defaultSplitPreference, }: GroupActionsProps) { @@ - <button - type="button" - onClick={() => setDeleteOpen(true)} - className="flex items-center justify-center gap-2 rounded-xl border border-rose-500/20 bg-rose-500/5 px-4 py-3 text-sm font-bold text-rose-600 hover:bg-rose-500/10 transition-colors" - > - <Trash2 className="h-4 w-4" /> - Delete group - </button> + {canDelete && ( + <button + type="button" + onClick={() => setDeleteOpen(true)} + className="flex items-center justify-center gap-2 rounded-xl border border-rose-500/20 bg-rose-500/5 px-4 py-3 text-sm font-bold text-rose-600 hover:bg-rose-500/10 transition-colors" + > + <Trash2 className="h-4 w-4" /> + Delete group + </button> + )} @@ - <DeleteGroupModal - groupId={groupId} - groupName={groupName} - open={deleteOpen} - onClose={() => setDeleteOpen(false)} - /> + {canDelete && ( + <DeleteGroupModal + groupId={groupId} + groupName={groupName} + open={deleteOpen} + onClose={() => setDeleteOpen(false)} + /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/groups/GroupActions.tsx` around lines 194 - 223, In GroupActions, hide the delete flow for non-owners by conditionally rendering the Delete button and the DeleteGroupModal only when the current user is the owner; use an ownership check (e.g., a boolean prop isOwner or compare currentUserId to the group's ownerId) and wrap the JSX that references setDeleteOpen, deleteOpen, and <DeleteGroupModal groupId={groupId} groupName={groupName} ... /> in that guard so regular members never see or trigger the forbidden delete flow.apps/web/src/components/theme/ThemeProvider.tsx (1)
23-60:⚠️ Potential issue | 🟠 MajorApply the resolved theme back to the document root.
resolvedis now only stored in React state. Since the stylesheet switches on[data-theme="light"], light mode will not reach the app shell unless some nested layout manually adds that attribute.Navbar/Footerinapps/web/app/layout.tsxwill stay on the dark:rootvars.Minimal fix
export function ThemeProvider({ children }: { children: React.ReactNode }) { const [mode, setModeState] = useState<ThemeMode>('system'); const [resolved, setResolved] = useState<ResolvedTheme>('dark'); @@ useEffect(() => { const media = window.matchMedia('(prefers-color-scheme: dark)'); @@ }, [mode]); + + useEffect(() => { + document.documentElement.dataset.theme = resolved; + document.body.dataset.theme = resolved; + }, [resolved]); const setMode = (next: ThemeMode) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/theme/ThemeProvider.tsx` around lines 23 - 60, The component updates only React state but never applies the theme to the DOM; add a sync that writes the resolved theme to the document root so CSS selectors like [data-theme="light"] take effect. Implement a useEffect in ThemeProvider that depends on resolved and sets document.documentElement.setAttribute('data-theme', resolved) (or removes the attribute if you prefer treating 'dark' as default) and ensure this runs after your initial readStored/setResolved and after setResolved(resolveTheme(next)) in setMode so the document root always reflects the current resolved theme; reference resolveTheme, ThemeProvider, resolved, setResolved, setMode and STORAGE_KEY to locate the places to update.
🧹 Nitpick comments (2)
apps/web/src/lib/actions.ts (1)
377-383: EncodegroupIdbefore interpolating it into the request path.This makes the action resilient to unexpected/special characters in identifiers.
🔧 Suggested fix
- const response = await fetch(`${getBackendBaseUrl()}/groups/${groupId}`, { + const response = await fetch(`${getBackendBaseUrl()}/groups/${encodeURIComponent(groupId)}`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/actions.ts` around lines 377 - 383, The DELETE request URL currently interpolates groupId directly into the path; update the call that constructs the fetch URL (the expression using getBackendBaseUrl() and groupId in apps/web/src/lib/actions.ts) to encode the identifier with encodeURIComponent(groupId) before interpolation so special characters are safely escaped; keep the rest of the fetch options (method, headers, cache) unchanged.apps/web/src/components/groups/CreateExpenseModal.tsx (1)
273-277: Consider unifying modal z-index tokens across dialogs.These hardcoded layers (
z-[80]/z-[110]) differ from other modals (apps/web/src/components/groups/CreateGroupModal.tsxandapps/web/src/components/groups/DeleteGroupModal.tsx), which can create inconsistent overlay ordering if dialogs stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/groups/CreateExpenseModal.tsx` around lines 273 - 277, The hardcoded z-index classes in CreateExpenseModal (the backdrop div with z-[80] and the wrapper div with z-[110]) should be unified with the shared modal z-index tokens used by CreateGroupModal and DeleteGroupModal; replace the literal z-[80]/z-[110] with the common token classes/variables (or import the shared constants from your UI/token module) for backdrop and content layers so all dialogs use the same stacking order and avoid inconsistent overlay ordering when dialogs stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/prisma/schema.prisma`:
- Line 70: Add a soft-delete guard to every group read so deleted groups are
never returned or modified: update the group lookup calls in getGroupById,
toggleShare, getGroupByShareToken and the balance export logic
(balances.service.ts) to include a Prisma where clause filtering on deletedAt:
null (or the equivalent query guard used across the codebase) and ensure any
single-group fetches use this same condition before returning or performing
mutations; if a lookup already combines other where filters, merge deletedAt:
null into that object so all reads and any share/token lookups reject
soft-deleted groups.
In `@apps/backend/src/groups/groups.service.ts`:
- Around line 677-698: The soft-delete only sets deletedAt but other methods
(e.g. assertMembership(), any findUnique/findFirst/getById/group lookup helpers)
still return groups with deletedAt set; update those lookup functions and any
direct group reads/updates in this file to filter out soft-deleted groups by
adding a condition like where: { id: groupId, deletedAt: null } (or include
deletedAt: null in composite where clauses used by assertMembership()), and make
them throw NotFound/Forbidden when the group is not found; ensure all usages of
assertMembership(), findUnique/findFirst on Group, and any group update/read
helpers reference this deletedAt check so deleted groups are inaccessible via
member endpoints.
In `@apps/web/app/dashboard/page.tsx`:
- Around line 62-71: The totalBalanceCents is being formatted with
primaryCurrency (groups[0]?.currency) which incorrectly presents a
mixed-currency aggregate as a single currency; compute the set of distinct
currencies from groups (e.g., map groups -> currency and dedupe) and if more
than one currency exists, do not call formatMoney(totalBalanceCents,
primaryCurrency); instead set a clear fallback value (e.g., "Multiple
currencies" or render a per-currency breakdown) and pass that to
SummaryCard.value; when all groups share the same currency continue using
primaryCurrency with formatMoney. Ensure you update the block around
primaryCurrency, totalBalanceCents, SummaryCard and any display logic to use an
isMixedCurrencies guard (distinct currencies check) before formatting.
In `@apps/web/src/components/groups/CreateExpenseModal.tsx`:
- Around line 286-287: The modal container in CreateExpenseModal.tsx currently
uses conflicting overflow utilities ("overflow-y-auto" and "overflow-hidden")
that can prevent vertical scrolling; edit the element that defines className
(the modal wrapper) to remove "overflow-hidden" and keep "overflow-y-auto"
(ensure the existing "max-h-[90vh]" remains) so the form can scroll on small
screens without being clipped.
In `@apps/web/src/components/groups/CreateGroupModal.tsx`:
- Around line 76-80: The close button in CreateGroupModal (the button using
onClick={onClose}) is icon-only and lacks accessible semantics; add
type="button" and an appropriate aria-label (e.g., aria-label="Close" or
aria-label="Close dialog") to that button element so screen readers can announce
its purpose while preserving the existing className and onClick handler.
In `@apps/web/src/components/groups/DeleteGroupModal.tsx`:
- Around line 26-43: The handleDelete function currently only calls
setIsDeleting(false) in the catch path, leaving isDeleting true on success; move
the teardown into a finally block so setIsDeleting(false) always runs.
Specifically, keep setIsDeleting(true) at the start of handleDelete, keep the
deleteGroupAction result checks and success flow (toast, onClose, router.push,
router.refresh), and add a finally that calls setIsDeleting(false) (and
optionally ensure setError(null) stays where it belongs) so state is reset
regardless of success or failure.
- Around line 51-76: The modal root (motion.div ref={modalRef}) lacks an
accessible name and the icon-only close button (onClick={onClose}) lacks an
aria-label; add aria-labelledby on the dialog pointing to the heading's id (give
the h3 a unique id like delete-group-title) so role="dialog" is programmatically
named, and add an appropriate aria-label (e.g., "Close" or "Close delete
dialog") to the close button (the element rendering <X />) to make it
screen-reader accessible; update DeleteGroupModal's JSX accordingly.
In `@apps/web/src/components/theme/ThemeToggle.tsx`:
- Around line 12-44: The three icon-only buttons that call
setMode('light'|'dark'|'system') should be made accessible: add type="button",
an explicit aria-label (e.g., "Light mode", "Dark mode", "System preference")
and an aria-pressed attribute that reflects the current mode (aria-pressed={mode
=== 'light' | 'dark' | 'system'} respectively). Update the buttons that render
Sun, Moon and Laptop so their aria-labels and aria-pressed state match the value
passed to setMode to ensure screen readers see the name and active state.
In `@apps/web/src/components/ui/Portal.tsx`:
- Line 16: The Portal currently uses createPortal(children, document.body) which
breaks theme scoping because it mounts outside the element that holds
data-theme; update the Portal component to locate the nearest theme-scoped
container (e.g., the closest ancestor or document.querySelector('[data-theme]'))
and mount the portal into that element (or create and append a dedicated wrapper
div inside that container) instead of document.body so modal/content inherits
the correct light-theme CSS variables; update references in the Portal code
where createPortal is called to use the found themeContainer.
---
Outside diff comments:
In `@apps/backend/src/groups/groups.service.ts`:
- Around line 253-270: The function getDashboard has stray query option objects
and never calls the ORM; wrap the existing where/include/orderBy into an awaited
Prisma findMany call and assign its result to a groups variable. Specifically,
replace the floating blocks with: const groups = await
this.prisma.group.findMany({ where: { members: { some: { userId } }, deletedAt:
null }, include: { _count: { select: { members: true } } }, orderBy: {
createdAt: 'desc' } }); so subsequent logic in getDashboard() can use the
populated groups array.
In `@apps/web/src/components/groups/GroupActions.tsx`:
- Around line 194-223: In GroupActions, hide the delete flow for non-owners by
conditionally rendering the Delete button and the DeleteGroupModal only when the
current user is the owner; use an ownership check (e.g., a boolean prop isOwner
or compare currentUserId to the group's ownerId) and wrap the JSX that
references setDeleteOpen, deleteOpen, and <DeleteGroupModal groupId={groupId}
groupName={groupName} ... /> in that guard so regular members never see or
trigger the forbidden delete flow.
In `@apps/web/src/components/theme/ThemeProvider.tsx`:
- Around line 23-60: The component updates only React state but never applies
the theme to the DOM; add a sync that writes the resolved theme to the document
root so CSS selectors like [data-theme="light"] take effect. Implement a
useEffect in ThemeProvider that depends on resolved and sets
document.documentElement.setAttribute('data-theme', resolved) (or removes the
attribute if you prefer treating 'dark' as default) and ensure this runs after
your initial readStored/setResolved and after setResolved(resolveTheme(next)) in
setMode so the document root always reflects the current resolved theme;
reference resolveTheme, ThemeProvider, resolved, setResolved, setMode and
STORAGE_KEY to locate the places to update.
---
Nitpick comments:
In `@apps/web/src/components/groups/CreateExpenseModal.tsx`:
- Around line 273-277: The hardcoded z-index classes in CreateExpenseModal (the
backdrop div with z-[80] and the wrapper div with z-[110]) should be unified
with the shared modal z-index tokens used by CreateGroupModal and
DeleteGroupModal; replace the literal z-[80]/z-[110] with the common token
classes/variables (or import the shared constants from your UI/token module) for
backdrop and content layers so all dialogs use the same stacking order and avoid
inconsistent overlay ordering when dialogs stack.
In `@apps/web/src/lib/actions.ts`:
- Around line 377-383: The DELETE request URL currently interpolates groupId
directly into the path; update the call that constructs the fetch URL (the
expression using getBackendBaseUrl() and groupId in apps/web/src/lib/actions.ts)
to encode the identifier with encodeURIComponent(groupId) before interpolation
so special characters are safely escaped; keep the rest of the fetch options
(method, headers, cache) unchanged.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc221e6c-c53f-4be8-9500-dbe50bc31b25
📒 Files selected for processing (28)
apps/backend/prisma/schema.prismaapps/backend/src/groups/groups.controller.tsapps/backend/src/groups/groups.service.tsapps/web/app/dashboard/groups/[groupId]/page.tsxapps/web/app/dashboard/page.tsxapps/web/app/globals.cssapps/web/app/layout.tsxapps/web/app/theme.cssapps/web/components/home/GridBackground.tsxapps/web/components/ui/GlassCard.tsxapps/web/src/components/dashboard/ActivityList.tsxapps/web/src/components/dashboard/SpendingChart.tsxapps/web/src/components/dashboard/SummaryCard.tsxapps/web/src/components/groups/CreateExpenseModal.tsxapps/web/src/components/groups/CreateGroupModal.tsxapps/web/src/components/groups/DeleteGroupModal.tsxapps/web/src/components/groups/GroupActions.tsxapps/web/src/components/layout/DashboardLayout.tsxapps/web/src/components/layout/GuestLayout.tsxapps/web/src/components/layout/Sidebar.tsxapps/web/src/components/layout/Topbar.tsxapps/web/src/components/profile/ProfilePanel.tsxapps/web/src/components/theme/ThemeProvider.tsxapps/web/src/components/theme/ThemeToggle.tsxapps/web/src/components/ui/Portal.tsxapps/web/src/design/colors.tsapps/web/src/design/theme.tsapps/web/src/lib/actions.ts
| shareToken String? @unique | ||
| shareEnabled Boolean @default(false) | ||
| createdAt DateTime @default(now()) | ||
| deletedAt DateTime? |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Group queries:"
rg -n --type=ts -C2 'prisma\.group\.(findUnique|findFirst|findMany)\(' apps/backend/src
echo
echo "2) Soft-delete filters present:"
rg -n --type=ts -C2 'deletedAt\s*:\s*null' apps/backend/src/groups apps/backend/src/balancesRepository: Arun-kushwaha007/FairShare
Length of output: 3109
🏁 Script executed:
#!/bin/bash
echo "=== Checking groups.service.ts around line 114 ==="
sed -n '110,130p' apps/backend/src/groups/groups.service.ts
echo
echo "=== Checking groups.service.ts around line 380 ==="
sed -n '375,390p' apps/backend/src/groups/groups.service.ts
echo
echo "=== Checking groups.service.ts around line 418 ==="
sed -n '415,435p' apps/backend/src/groups/groups.service.ts
echo
echo "=== Checking balances.service.ts around line 50 ==="
sed -n '45,65p' apps/backend/src/balances/balances.service.tsRepository: Arun-kushwaha007/FairShare
Length of output: 2835
Enforce soft-delete filters on all user-facing group read operations.
Current group queries bypass soft-delete checks in several critical paths:
getGroupById()(line 114): NodeletedAtfilter; returns deleted groupstoggleShare()(line 380): NodeletedAtfilter on lookup; allows modifications to deleted groupsgetGroupByShareToken()(line 418): NodeletedAtfilter; public share links to deleted groups remain accessible- Balance export in
balances.service.ts(line 50): NodeletedAtfilter; deleted group data can be exported
All group reads must include where: { ..., deletedAt: null } or equivalent guard to prevent access to deleted groups.
🧰 Tools
🪛 GitHub Actions: CI
[error] Command failed with exit code 1: tsc --noEmit (backend@1.0.0 lint).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/prisma/schema.prisma` at line 70, Add a soft-delete guard to
every group read so deleted groups are never returned or modified: update the
group lookup calls in getGroupById, toggleShare, getGroupByShareToken and the
balance export logic (balances.service.ts) to include a Prisma where clause
filtering on deletedAt: null (or the equivalent query guard used across the
codebase) and ensure any single-group fetches use this same condition before
returning or performing mutations; if a lookup already combines other where
filters, merge deletedAt: null into that object so all reads and any share/token
lookups reject soft-deleted groups.
Comment on lines
+677
to
+698
| async delete(groupId: string, actorUserId: string): Promise<{ success: true }> { | ||
| const membership = await this.prisma.groupMember.findUnique({ | ||
| where: { | ||
| groupId_userId: { | ||
| groupId, | ||
| userId: actorUserId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (!membership || membership.role !== 'OWNER') { | ||
| throw new ForbiddenException('Only the group owner can delete the group'); | ||
| } | ||
|
|
||
| await this.prisma.group.update({ | ||
| where: { id: groupId }, | ||
| data: { | ||
| deletedAt: new Date(), | ||
| shareEnabled: false, | ||
| shareToken: null | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Soft-deleted groups are still accessible through existing member endpoints.
This only hides deleted groups from list/dashboard-style queries. Because assertMembership() and the other groupId lookups do not check deletedAt, a member with an old URL can still fetch or mutate a deleted group after this method runs.
Suggested direction
- private async assertMembership(groupId: string, userId: string): Promise<void> {
- const membership = await this.prisma.groupMember.findUnique({
- where: {
- groupId_userId: {
- groupId,
- userId,
- },
- },
- });
+ private async assertMembership(groupId: string, userId: string): Promise<void> {
+ const membership = await this.prisma.groupMember.findFirst({
+ where: {
+ groupId,
+ userId,
+ group: {
+ deletedAt: null,
+ },
+ },
+ });
if (!membership) {
throw new ForbiddenException('Actor is not a group member');
}
}The same rule needs to be applied to direct group reads/updates in this file, otherwise deleted groups remain operational through stale clients.
🧰 Tools
🪛 Biome (2.4.10)
[error] 677-677: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 677-677: expected ) but instead found :
(parse)
[error] 677-677: the target for a delete operator cannot be a single identifier
(parse)
[error] 677-677: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 677-677: Expected a statement but instead found '>'.
(parse)
🪛 GitHub Actions: CI
[error] Command failed with exit code 1: tsc --noEmit (backend@1.0.0 lint).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/groups/groups.service.ts` around lines 677 - 698, The
soft-delete only sets deletedAt but other methods (e.g. assertMembership(), any
findUnique/findFirst/getById/group lookup helpers) still return groups with
deletedAt set; update those lookup functions and any direct group reads/updates
in this file to filter out soft-deleted groups by adding a condition like where:
{ id: groupId, deletedAt: null } (or include deletedAt: null in composite where
clauses used by assertMembership()), and make them throw NotFound/Forbidden when
the group is not found; ensure all usages of assertMembership(),
findUnique/findFirst on Group, and any group update/read helpers reference this
deletedAt check so deleted groups are inaccessible via member endpoints.
Comment on lines
+62
to
+71
| const primaryCurrency = groups[0]?.currency ?? 'USD'; | ||
| const totalBalanceLabel = groups.length > 1 ? 'Multi-Crew Balance' : 'Total Balance'; | ||
|
|
||
| return ( | ||
| <DashboardLayout> | ||
| <div className="space-y-8"> | ||
| <div className="grid grid-cols-2 gap-3 sm:gap-4 md:grid-cols-4"> | ||
| <SummaryCard | ||
| title="Total Balance" | ||
| value={formatMoney(totalBalanceCents, 'USD')} | ||
| title={totalBalanceLabel} | ||
| value={formatMoney(totalBalanceCents, primaryCurrency)} |
Contributor
There was a problem hiding this comment.
Don't render a mixed-currency sum as one currency.
dashboard.totalBalanceCents is aggregated across all groups, but this now formats it using groups[0]?.currency. If a user has both USD and EUR groups, the card will show a mixed total as if it were entirely USD/EUR.
Suggested guard
const primaryCurrency = groups[0]?.currency ?? 'USD';
+ const hasSingleCurrency = new Set(groups.map((group) => group.currency)).size <= 1;
const totalBalanceLabel = groups.length > 1 ? 'Multi-Crew Balance' : 'Total Balance';
@@
<SummaryCard
title={totalBalanceLabel}
- value={formatMoney(totalBalanceCents, primaryCurrency)}
+ value={hasSingleCurrency ? formatMoney(totalBalanceCents, primaryCurrency) : '—'}
icon="dollar"
change="Live"
trend={isPositive ? 'up' : 'down'}
- hint={isPositive ? 'Surplus Protocol' : 'Deficit Detected'}
+ hint={
+ hasSingleCurrency
+ ? isPositive
+ ? 'Surplus Protocol'
+ : 'Deficit Detected'
+ : 'Multiple currencies'
+ }
/>Longer term, this probably wants a per-currency breakdown from the backend instead of a single aggregate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/dashboard/page.tsx` around lines 62 - 71, The totalBalanceCents
is being formatted with primaryCurrency (groups[0]?.currency) which incorrectly
presents a mixed-currency aggregate as a single currency; compute the set of
distinct currencies from groups (e.g., map groups -> currency and dedupe) and if
more than one currency exists, do not call formatMoney(totalBalanceCents,
primaryCurrency); instead set a clear fallback value (e.g., "Multiple
currencies" or render a per-currency breakdown) and pass that to
SummaryCard.value; when all groups share the same currency continue using
primaryCurrency with formatMoney. Ensure you update the block around
primaryCurrency, totalBalanceCents, SummaryCard and any display logic to use an
isMixedCurrencies guard (distinct currencies check) before formatting.
Comment on lines
+286
to
+287
| className="w-full max-h-[90vh] overflow-y-auto rounded-3xl border border-[var(--fs-border)] bg-[var(--fs-card-solid)] shadow-[var(--fs-shadow-elevated)] overflow-hidden" | ||
| > |
Contributor
There was a problem hiding this comment.
Conflicting overflow utilities can block modal scrolling.
Line 286 includes both overflow-y-auto and overflow-hidden; the latter can suppress vertical scroll and hide form content on smaller screens.
Suggested fix
- className="w-full max-h-[90vh] overflow-y-auto rounded-3xl border border-[var(--fs-border)] bg-[var(--fs-card-solid)] shadow-[var(--fs-shadow-elevated)] overflow-hidden"
+ className="w-full max-h-[90vh] overflow-y-auto overflow-x-hidden rounded-3xl border border-[var(--fs-border)] bg-[var(--fs-card-solid)] shadow-[var(--fs-shadow-elevated)]"📝 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
| className="w-full max-h-[90vh] overflow-y-auto rounded-3xl border border-[var(--fs-border)] bg-[var(--fs-card-solid)] shadow-[var(--fs-shadow-elevated)] overflow-hidden" | |
| > | |
| className="w-full max-h-[90vh] overflow-y-auto overflow-x-hidden rounded-3xl border border-[var(--fs-border)] bg-[var(--fs-card-solid)] shadow-[var(--fs-shadow-elevated)]" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/groups/CreateExpenseModal.tsx` around lines 286 -
287, The modal container in CreateExpenseModal.tsx currently uses conflicting
overflow utilities ("overflow-y-auto" and "overflow-hidden") that can prevent
vertical scrolling; edit the element that defines className (the modal wrapper)
to remove "overflow-hidden" and keep "overflow-y-auto" (ensure the existing
"max-h-[90vh]" remains) so the form can scroll on small screens without being
clipped.
Comment on lines
+76
to
80
| <button | ||
| onClick={onClose} | ||
| className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]" | ||
| title="Close" | ||
| > |
Contributor
There was a problem hiding this comment.
Add an accessible name to the icon-only close button.
Line 76 should include aria-label (and type="button" for explicit semantics).
Suggested fix
- <button
+ <button
+ type="button"
onClick={onClose}
+ aria-label="Close create group dialog"
className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]"
title="Close"
>📝 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 | |
| onClick={onClose} | |
| className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]" | |
| title="Close" | |
| > | |
| <button | |
| type="button" | |
| onClick={onClose} | |
| aria-label="Close create group dialog" | |
| className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]" | |
| title="Close" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/groups/CreateGroupModal.tsx` around lines 76 - 80,
The close button in CreateGroupModal (the button using onClick={onClose}) is
icon-only and lacks accessible semantics; add type="button" and an appropriate
aria-label (e.g., aria-label="Close" or aria-label="Close dialog") to that
button element so screen readers can announce its purpose while preserving the
existing className and onClick handler.
Comment on lines
+26
to
+43
| const handleDelete = async () => { | ||
| try { | ||
| setIsDeleting(true); | ||
| setError(null); | ||
| const result = await deleteGroupAction(groupId); | ||
|
|
||
| if (!result.success) { | ||
| throw new Error(result.message); | ||
| } | ||
|
|
||
| toast(`Group "${groupName}" deleted successfully`); | ||
| onClose(); | ||
| router.push('/dashboard'); | ||
| router.refresh(); | ||
| } catch (err) { | ||
| setError((err as Error).message || 'Failed to delete group'); | ||
| setIsDeleting(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Reset isDeleting in a finally block.
Line 42 resets only in catch; success path leaves stale state until unmount. Use finally to guarantee cleanup.
Suggested fix
const handleDelete = async () => {
try {
setIsDeleting(true);
setError(null);
const result = await deleteGroupAction(groupId);
if (!result.success) {
throw new Error(result.message);
}
toast(`Group "${groupName}" deleted successfully`);
onClose();
router.push('/dashboard');
router.refresh();
} catch (err) {
setError((err as Error).message || 'Failed to delete group');
- setIsDeleting(false);
+ } finally {
+ setIsDeleting(false);
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/groups/DeleteGroupModal.tsx` around lines 26 - 43,
The handleDelete function currently only calls setIsDeleting(false) in the catch
path, leaving isDeleting true on success; move the teardown into a finally block
so setIsDeleting(false) always runs. Specifically, keep setIsDeleting(true) at
the start of handleDelete, keep the deleteGroupAction result checks and success
flow (toast, onClose, router.push, router.refresh), and add a finally that calls
setIsDeleting(false) (and optionally ensure setError(null) stays where it
belongs) so state is reset regardless of success or failure.
Comment on lines
+51
to
+76
| <motion.div | ||
| ref={modalRef} | ||
| tabIndex={-1} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| initial={{ opacity: 0, scale: 0.95, y: 20 }} | ||
| animate={{ opacity: 1, scale: 1, y: 0 }} | ||
| exit={{ opacity: 0, scale: 0.95, y: 20 }} | ||
| className="relative w-full max-w-md rounded-3xl border-2 border-rose-500/20 bg-[var(--fs-card-solid)] p-6 sm:p-8 shadow-[0_20px_50px_rgba(244,63,94,0.15)] overflow-hidden" | ||
| > | ||
| <div className="flex items-center justify-between mb-6"> | ||
| <div className="flex h-12 w-12 items-center justify-center rounded-2xl bg-rose-500/10 text-rose-500"> | ||
| <AlertTriangle className="h-6 w-6" /> | ||
| </div> | ||
| <button | ||
| onClick={onClose} | ||
| className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]" | ||
| > | ||
| <X className="h-5 w-5" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="space-y-4 text-center sm:text-left"> | ||
| <h3 className="text-2xl font-extrabold tracking-tight text-[var(--fs-text-primary)]"> | ||
| Delete Group? | ||
| </h3> |
Contributor
There was a problem hiding this comment.
Dialog is missing an accessible name.
The dialog has role="dialog" but no aria-labelledby/aria-label. Also, the icon-only close button should have aria-label.
Suggested fix
<motion.div
ref={modalRef}
tabIndex={-1}
role="dialog"
aria-modal="true"
+ aria-labelledby="delete-group-title"
initial={{ opacity: 0, scale: 0.95, y: 20 }}
animate={{ opacity: 1, scale: 1, y: 0 }}
exit={{ opacity: 0, scale: 0.95, y: 20 }}
className="relative w-full max-w-md rounded-3xl border-2 border-rose-500/20 bg-[var(--fs-card-solid)] p-6 sm:p-8 shadow-[0_20px_50px_rgba(244,63,94,0.15)] overflow-hidden"
>
...
- <button
+ <button
+ type="button"
onClick={onClose}
+ aria-label="Close delete group dialog"
className="p-2 rounded-xl bg-[var(--fs-background)] hover:bg-[var(--fs-background)]/70 transition-colors text-[var(--fs-text-muted)]"
>
...
- <h3 className="text-2xl font-extrabold tracking-tight text-[var(--fs-text-primary)]">
+ <h3 id="delete-group-title" className="text-2xl font-extrabold tracking-tight text-[var(--fs-text-primary)]">
Delete Group?
</h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/groups/DeleteGroupModal.tsx` around lines 51 - 76,
The modal root (motion.div ref={modalRef}) lacks an accessible name and the
icon-only close button (onClick={onClose}) lacks an aria-label; add
aria-labelledby on the dialog pointing to the heading's id (give the h3 a unique
id like delete-group-title) so role="dialog" is programmatically named, and add
an appropriate aria-label (e.g., "Close" or "Close delete dialog") to the close
button (the element rendering <X />) to make it screen-reader accessible; update
DeleteGroupModal's JSX accordingly.
Comment on lines
+12
to
+44
| <button | ||
| onClick={() => setMode('light')} | ||
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | ||
| mode === 'light' | ||
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | ||
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | ||
| }`} | ||
| title="Light Mode" | ||
| > | ||
| <Sun size={16} /> | ||
| </button> | ||
| <button | ||
| onClick={() => setMode('dark')} | ||
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | ||
| mode === 'dark' | ||
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | ||
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | ||
| }`} | ||
| title="Dark Mode" | ||
| > | ||
| <Moon size={16} /> | ||
| </button> | ||
| <button | ||
| onClick={() => setMode('system')} | ||
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | ||
| mode === 'system' | ||
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | ||
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | ||
| }`} | ||
| title="System Preference" | ||
| > | ||
| <Laptop size={16} /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add accessible labels/state to icon-only toggle buttons.
These controls rely on title, but they should expose explicit button name/state for assistive tech (aria-label + aria-pressed). Also set type="button" for safety in form contexts.
♿ Suggested fix
<button
+ type="button"
onClick={() => setMode('light')}
+ aria-label="Light mode"
+ aria-pressed={mode === 'light'}
className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${
mode === 'light'
? 'bg-[var(--fs-primary)] text-white shadow-md'
: 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]'
}`}
title="Light Mode"
>
<Sun size={16} />
</button>
<button
+ type="button"
onClick={() => setMode('dark')}
+ aria-label="Dark mode"
+ aria-pressed={mode === 'dark'}
className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${
mode === 'dark'
? 'bg-[var(--fs-primary)] text-white shadow-md'
: 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]'
}`}
title="Dark Mode"
>
<Moon size={16} />
</button>
<button
+ type="button"
onClick={() => setMode('system')}
+ aria-label="System theme"
+ aria-pressed={mode === 'system'}
className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${
mode === 'system'
? 'bg-[var(--fs-primary)] text-white shadow-md'
: 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]'
}`}
title="System Preference"
>
<Laptop size={16} />
</button>📝 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 | |
| onClick={() => setMode('light')} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'light' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="Light Mode" | |
| > | |
| <Sun size={16} /> | |
| </button> | |
| <button | |
| onClick={() => setMode('dark')} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'dark' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="Dark Mode" | |
| > | |
| <Moon size={16} /> | |
| </button> | |
| <button | |
| onClick={() => setMode('system')} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'system' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="System Preference" | |
| > | |
| <Laptop size={16} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setMode('light')} | |
| aria-label="Light mode" | |
| aria-pressed={mode === 'light'} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'light' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="Light Mode" | |
| > | |
| <Sun size={16} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setMode('dark')} | |
| aria-label="Dark mode" | |
| aria-pressed={mode === 'dark'} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'dark' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="Dark Mode" | |
| > | |
| <Moon size={16} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setMode('system')} | |
| aria-label="System theme" | |
| aria-pressed={mode === 'system'} | |
| className={`flex h-8 w-8 items-center justify-center rounded-xl transition-all ${ | |
| mode === 'system' | |
| ? 'bg-[var(--fs-primary)] text-white shadow-md' | |
| : 'text-[var(--fs-text-muted)] hover:bg-[var(--fs-primary)]/10 hover:text-[var(--fs-primary)]' | |
| }`} | |
| title="System Preference" | |
| > | |
| <Laptop size={16} /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/theme/ThemeToggle.tsx` around lines 12 - 44, The
three icon-only buttons that call setMode('light'|'dark'|'system') should be
made accessible: add type="button", an explicit aria-label (e.g., "Light mode",
"Dark mode", "System preference") and an aria-pressed attribute that reflects
the current mode (aria-pressed={mode === 'light' | 'dark' | 'system'}
respectively). Update the buttons that render Sun, Moon and Laptop so their
aria-labels and aria-pressed state match the value passed to setMode to ensure
screen readers see the name and active state.
|
|
||
| if (!mounted) return null; | ||
|
|
||
| return createPortal(children, document.body); |
Contributor
There was a problem hiding this comment.
Portal target breaks theme scoping for modals in light mode.
createPortal(children, document.body) mounts outside the data-theme container, so portaled UI can ignore light-theme overrides and render with root (dark) variables.
🎯 Suggested fix
export function Portal({ children }: { children: ReactNode }) {
- const [mounted, setMounted] = useState(false);
+ const [container, setContainer] = useState<HTMLElement | null>(null);
useEffect(() => {
- setMounted(true);
- return () => setMounted(false);
+ const themedRoot = document.querySelector<HTMLElement>('[data-theme]');
+ setContainer(themedRoot ?? document.body);
+ return () => setContainer(null);
}, []);
- if (!mounted) return null;
+ if (!container) return null;
- return createPortal(children, document.body);
+ return createPortal(children, container);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ui/Portal.tsx` at line 16, The Portal currently uses
createPortal(children, document.body) which breaks theme scoping because it
mounts outside the element that holds data-theme; update the Portal component to
locate the nearest theme-scoped container (e.g., the closest ancestor or
document.querySelector('[data-theme]')) and mount the portal into that element
(or create and append a dedicated wrapper div inside that container) instead of
document.body so modal/content inherits the correct light-theme CSS variables;
update references in the Portal code where createPortal is called to use the
found themeContainer.
Add database services startup, migration, and Playwright installation to CI pipeline. Configure Playwright for api and web test projects with web server setup. Add invalidateUserDashboardCache method in RedisService. Enhance getDashboard in GroupsService to fetch user groups. Add dashboard.spec.ts e2e test file and test-results directory.
Owner
Author
|
@copilot review this pr |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/e2e/dashboard.spec.ts (2)
27-36: Consider more specific assertions for dashboard verification.The assertion
getByText('group', { exact: false })is quite broad and could match unintended elements (e.g., navigation items, any text containing "group"). A more targeted selector would make the test less fragile.♻️ Suggested improvement
// The user doesn't have any groups yet, so check for empty state - await expect(page.getByText('group', { exact: false }).first()).toBeVisible(); + // Use a more specific selector for the empty state component + await expect(page.getByRole('heading', { name: /no groups/i })).toBeVisible(); + // Or target a data-testid if available: + // await expect(page.getByTestId('empty-groups-state')).toBeVisible();If the exact empty state text varies, consider adding a
data-testidattribute to the empty state component in the dashboard page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/dashboard.spec.ts` around lines 27 - 36, The test step "Verify Dashboard Elements" uses a broad locator page.getByText('group', { exact: false }) which can match unintended elements; update the assertion to target the empty-state component specifically (e.g., use a data-testid on the empty-state element and assert with page.getByTestId('dashboard-empty-state') or a more specific locator like page.locator('selector-for-empty-state').getByText('expected empty text')), replacing the existing page.getByText call in the Verify Dashboard Elements step and adding a data-testid to the dashboard empty-state component if the exact text can vary.
10-15: Test data is not cleaned up after the test.Each test run creates a new user in the database that persists after the test completes. Over time, this could accumulate significant test data. Consider adding cleanup in an
afterEachhook or using a test-specific database that gets reset between CI runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/dashboard.spec.ts` around lines 10 - 15, The test seeds a persistent user via request.post to `${apiBaseUrl}/auth/register` (see apiBaseUrl and registerResp) but never cleans it up; add an afterEach hook that removes the created user (e.g., call the API delete user endpoint or a test-only cleanup endpoint using request or invoke a DB reset helper) using the same email returned/used by registerResp, or switch the suite to use a disposable/test database that is reset between runs; ensure the cleanup runs even on failed assertions so tests do not accumulate users..github/workflows/ci.yml (1)
75-82: Consider adding Docker Compose cleanup.If E2E tests fail or the workflow is cancelled, Docker Compose services remain running. While GitHub-hosted runners are ephemeral, adding cleanup improves consistency and is good practice for self-hosted runners.
♻️ Suggested cleanup step
- name: Run E2E Tests env: CI: true DATABASE_URL: postgresql://postgres:postgres@localhost:5432/fairshare?schema=public JWT_SECRET: supersecret STRIPE_SECRET_KEY: test_key NEXT_PUBLIC_APP_URL: http://localhost:3000 run: pnpm run e2e + - name: Stop Database Services + if: always() + run: docker compose down + - name: Upload coverage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 75 - 82, The "Run E2E Tests" job step currently starts services but lacks cleanup; add a separate always-run cleanup step (e.g., named "Teardown Docker Compose" or similar) that runs after the "Run E2E Tests" step using if: always() and executes a docker compose down --volumes --remove-orphans (or docker-compose down --volumes --remove-orphans) to stop containers and remove volumes/orphans; reference the existing step name "Run E2E Tests" and the command "pnpm run e2e" so the teardown executes regardless of test success/failure or cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 62-70: The CI runs migrations immediately after the "Start
Database Services" step so the Postgres container may not be ready; add a
health/wait step between the "Start Database Services" and "Migrate Database"
steps to ensure readiness. Either change the start command to use docker compose
up -d --wait (if healthchecks exist) or add a separate step that polls the
DATABASE_URL (e.g., using pg_isready, wait-for-it, or a small loop against
localhost:5432) and only proceeds when the DB is accepting connections before
running the pnpm --filter backend prisma:migrate command.
In `@apps/backend/src/groups/groups.service.ts`:
- Around line 701-702: The code currently invalidates only the actor's dashboard
cache after group changes; update the logic in GroupsService (around the calls
to invalidateGroupCache and invalidateUserDashboardCache) to also invalidate
dashboard caches for all other group members: fetch the member IDs for the
affected group (use the existing groupId/group membership retrieval method in
this service), then call this.redis.invalidateUserDashboardCache(memberId) for
each member (exclude or include actorUserId as desired), batching or using Redis
pipeline if available to avoid N+1 latency; keep the existing await
this.redis.invalidateGroupCache(groupId) call and ensure errors are
handled/logged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 75-82: The "Run E2E Tests" job step currently starts services but
lacks cleanup; add a separate always-run cleanup step (e.g., named "Teardown
Docker Compose" or similar) that runs after the "Run E2E Tests" step using if:
always() and executes a docker compose down --volumes --remove-orphans (or
docker-compose down --volumes --remove-orphans) to stop containers and remove
volumes/orphans; reference the existing step name "Run E2E Tests" and the
command "pnpm run e2e" so the teardown executes regardless of test
success/failure or cancellation.
In `@tests/e2e/dashboard.spec.ts`:
- Around line 27-36: The test step "Verify Dashboard Elements" uses a broad
locator page.getByText('group', { exact: false }) which can match unintended
elements; update the assertion to target the empty-state component specifically
(e.g., use a data-testid on the empty-state element and assert with
page.getByTestId('dashboard-empty-state') or a more specific locator like
page.locator('selector-for-empty-state').getByText('expected empty text')),
replacing the existing page.getByText call in the Verify Dashboard Elements step
and adding a data-testid to the dashboard empty-state component if the exact
text can vary.
- Around line 10-15: The test seeds a persistent user via request.post to
`${apiBaseUrl}/auth/register` (see apiBaseUrl and registerResp) but never cleans
it up; add an afterEach hook that removes the created user (e.g., call the API
delete user endpoint or a test-only cleanup endpoint using request or invoke a
DB reset helper) using the same email returned/used by registerResp, or switch
the suite to use a disposable/test database that is reset between runs; ensure
the cleanup runs even on failed assertions so tests do not accumulate users.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: febf51a0-b37f-44df-a540-812fd56cff7a
📒 Files selected for processing (6)
.github/workflows/ci.ymlapps/backend/src/groups/groups.service.tsapps/backend/src/redis/redis.service.tsplaywright.config.tstest-results/.last-run.jsontests/e2e/dashboard.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test-results/.last-run.json
Comment on lines
+62
to
+70
| - name: Start Database Services | ||
| run: docker compose up -d | ||
|
|
||
| - name: Migrate Database | ||
| env: | ||
| DATABASE_URL: postgresql://postgres:postgres@localhost:5432/fairshare?schema=public | ||
| JWT_SECRET: supersecret | ||
| STRIPE_SECRET_KEY: test_key | ||
| run: pnpm --filter backend prisma:migrate |
Contributor
There was a problem hiding this comment.
Add health check before running migrations.
There's no wait between starting Docker Compose services and running migrations. The database may not be ready to accept connections immediately after docker compose up -d returns, which could cause intermittent CI failures.
🛠️ Proposed fix: Add a wait-for-healthy step
- name: Start Database Services
run: docker compose up -d
+ - name: Wait for Database
+ run: |
+ timeout 60 bash -c 'until docker compose exec -T postgres pg_isready; do sleep 1; done'
+
- name: Migrate DatabaseAlternatively, if your docker-compose.yml has a healthcheck defined, you can use:
run: docker compose up -d --wait🧰 Tools
🪛 Checkov (3.2.519)
[medium] 67-68: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 62 - 70, The CI runs migrations
immediately after the "Start Database Services" step so the Postgres container
may not be ready; add a health/wait step between the "Start Database Services"
and "Migrate Database" steps to ensure readiness. Either change the start
command to use docker compose up -d --wait (if healthchecks exist) or add a
separate step that polls the DATABASE_URL (e.g., using pg_isready, wait-for-it,
or a small loop against localhost:5432) and only proceeds when the DB is
accepting connections before running the pnpm --filter backend prisma:migrate
command.
Comment on lines
+701
to
+702
| await this.redis.invalidateGroupCache(groupId); | ||
| await this.redis.invalidateUserDashboardCache(actorUserId); |
Contributor
There was a problem hiding this comment.
Other group members' dashboard caches are not invalidated.
Only the actor's dashboard cache is invalidated. Other group members will continue seeing the deleted group in their cached dashboards until the cache TTL (120s) expires. Consider invalidating dashboard caches for all group members.
🛠️ Suggested approach
+ // Fetch all member IDs before invalidation
+ const members = await this.prisma.groupMember.findMany({
+ where: { groupId },
+ select: { userId: true },
+ });
await this.redis.invalidateGroupCache(groupId);
- await this.redis.invalidateUserDashboardCache(actorUserId);
+ // Invalidate dashboard cache for all members
+ await Promise.all(
+ members.map((m) => this.redis.invalidateUserDashboardCache(m.userId))
+ );
return { success: true };📝 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
| await this.redis.invalidateGroupCache(groupId); | |
| await this.redis.invalidateUserDashboardCache(actorUserId); | |
| // Fetch all member IDs before invalidation | |
| const members = await this.prisma.groupMember.findMany({ | |
| where: { groupId }, | |
| select: { userId: true }, | |
| }); | |
| await this.redis.invalidateGroupCache(groupId); | |
| // Invalidate dashboard cache for all members | |
| await Promise.all( | |
| members.map((m) => this.redis.invalidateUserDashboardCache(m.userId)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/groups/groups.service.ts` around lines 701 - 702, The code
currently invalidates only the actor's dashboard cache after group changes;
update the logic in GroupsService (around the calls to invalidateGroupCache and
invalidateUserDashboardCache) to also invalidate dashboard caches for all other
group members: fetch the member IDs for the affected group (use the existing
groupId/group membership retrieval method in this service), then call
this.redis.invalidateUserDashboardCache(memberId) for each member (exclude or
include actorUserId as desired), batching or using Redis pipeline if available
to avoid N+1 latency; keep the existing await
this.redis.invalidateGroupCache(groupId) call and ensure errors are
handled/logged.
- Moved build step before lint and test in CI pipeline - Changed Prisma migration to db push for faster database setup - Added build dependency to lint task in turbo.json for proper execution order
…l animations, cleanup - ThemeProvider: restore data-theme application to document elements - layout.tsx: restore theme init script to prevent FOUC - groups.service.ts: add existence and already-deleted checks in delete method - CreateExpenseModal: restore AnimatePresence for exit animations - ThemeToggle: remove unused imports and variables - .gitignore: add test-results/ and remove committed artifact Agent-Logs-Url: https://github.com/Arun-kushwaha007/FairShare/sessions/3067ae7f-0172-4370-8ac6-04de20b46b7c Co-authored-by: Arun-kushwaha007 <121769639+Arun-kushwaha007@users.noreply.github.com>
…ared-types/src Agent-Logs-Url: https://github.com/Arun-kushwaha007/FairShare/sessions/3067ae7f-0172-4370-8ac6-04de20b46b7c Co-authored-by: Arun-kushwaha007 <121769639+Arun-kushwaha007@users.noreply.github.com>
Contributor
Here's my review of the PR. I found and fixed several issues in commit Critical fixes:
Cleanup:
Additional observation (not fixed, pre-existing): The CI workflow uses Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…tions - Reorder constructor parameters in ReceiptsService tests to match implementation changes - Add groupId select to expense query expectation in receipts service test - Include findUnique mock for settlement in integration test setup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/backend/src/groups/groups.service.ts (1)
705-717: Consider adding activity log and realtime notification for deletion.Other mutating operations in this service (e.g.,
create,invite,remindSettlement) create activity records and/or emit realtime events. For audit trail consistency and immediate member notification, consider:
- Creating an activity record for the deletion
- Emitting a realtime event (e.g.,
group_deleted) so connected members are informed immediately💡 Optional enhancement
await this.prisma.group.update({ where: { id: groupId }, data: { deletedAt: new Date(), shareEnabled: false, shareToken: null }, }); + await this.prisma.activity.create({ + data: { + groupId, + actorUserId, + type: 'group_deleted', + entityId: groupId, + metadata: {}, + }, + }); + await this.redis.invalidateGroupCache(groupId); await this.redis.invalidateUserDashboardCache(actorUserId); + this.realtime.emitToGroup(groupId, 'group_deleted', { groupId }); + return { success: true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/groups/groups.service.ts` around lines 705 - 717, The deletion flow currently updates the group and invalidates caches but lacks an activity audit and realtime notification; after the prisma update and before returning, create an activity record (e.g., call this.activity.createActivity or this.activityService.create with actorUserId, groupId, action: 'group_deleted', and metadata) and emit a realtime event to members (e.g., this.realtime.emit or this.realtimeGateway.emit('group_deleted', { groupId, actorUserId })) so connected clients are notified, then continue to call this.redis.invalidateGroupCache(groupId) and this.redis.invalidateUserDashboardCache(actorUserId) and return the existing { success: true } response..github/workflows/ci.yml (1)
65-82: Consider consolidating duplicated environment variables.The
DATABASE_URL,JWT_SECRET, andSTRIPE_SECRET_KEYvariables are duplicated across the "Migrate Database" and "Run E2E Tests" steps. Defining them once at the job level would reduce duplication and make maintenance easier.♻️ Suggested refactor using job-level env
validate: runs-on: ubuntu-latest timeout-minutes: 30 + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/fairshare?schema=public + JWT_SECRET: supersecret + STRIPE_SECRET_KEY: test_key steps: # ... earlier steps ... - name: Migrate Database - env: - DATABASE_URL: postgresql://postgres:postgres@localhost:5432/fairshare?schema=public - JWT_SECRET: supersecret - STRIPE_SECRET_KEY: test_key run: pnpm --filter backend exec prisma db push # ... Playwright install ... - name: Run E2E Tests env: CI: true - DATABASE_URL: postgresql://postgres:postgres@localhost:5432/fairshare?schema=public - JWT_SECRET: supersecret - STRIPE_SECRET_KEY: test_key NEXT_PUBLIC_APP_URL: http://localhost:3000 run: pnpm run e2e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 65 - 82, Move the duplicated environment variables (DATABASE_URL, JWT_SECRET, STRIPE_SECRET_KEY and any others like NEXT_PUBLIC_APP_URL/CI if applicable) from the individual steps "Migrate Database" and "Run E2E Tests" into the job-level env block so they are defined once for the job; update or remove the per-step env maps in those steps ("Migrate Database" and "Run E2E Tests") so they inherit the job-level env, ensuring pnpm --filter backend exec prisma db push and pnpm run e2e continue to run unchanged with the shared variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/layout.tsx`:
- Around line 12-19: The try block that reads storageKey/stored/mode and sets
document.documentElement.dataset.theme can throw and currently the empty catch
swallows errors leaving no data-theme; update the catch to set a safe fallback
theme (e.g., 'light' or 'system') on document.documentElement.dataset.theme and
optionally log the error (capture the exception object) so failures are visible;
ensure the logic still resolves based on prefersDark/mode when available and
that the catch covers both localStorage and matchMedia failures.
In `@apps/web/src/components/theme/ThemeProvider.tsx`:
- Line 34: The component initializes the local state const [resolved,
setResolved] = useState<ResolvedTheme>('dark') which can mismatch the theme
already applied on document.documentElement and cause a flash; update
ThemeProvider to derive the initial resolved value from the DOM (e.g., read
document.documentElement.dataset.theme or computed style) when creating the
state (or in an effect that runs once) so resolved reflects the pre-hydration
theme; modify the useState initialization or add a useEffect that reads
document.documentElement.dataset.theme, validates it against ResolvedTheme
values, and calls setResolved(...) to avoid the first-paint mismatch.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 65-82: Move the duplicated environment variables (DATABASE_URL,
JWT_SECRET, STRIPE_SECRET_KEY and any others like NEXT_PUBLIC_APP_URL/CI if
applicable) from the individual steps "Migrate Database" and "Run E2E Tests"
into the job-level env block so they are defined once for the job; update or
remove the per-step env maps in those steps ("Migrate Database" and "Run E2E
Tests") so they inherit the job-level env, ensuring pnpm --filter backend exec
prisma db push and pnpm run e2e continue to run unchanged with the shared
variables.
In `@apps/backend/src/groups/groups.service.ts`:
- Around line 705-717: The deletion flow currently updates the group and
invalidates caches but lacks an activity audit and realtime notification; after
the prisma update and before returning, create an activity record (e.g., call
this.activity.createActivity or this.activityService.create with actorUserId,
groupId, action: 'group_deleted', and metadata) and emit a realtime event to
members (e.g., this.realtime.emit or this.realtimeGateway.emit('group_deleted',
{ groupId, actorUserId })) so connected clients are notified, then continue to
call this.redis.invalidateGroupCache(groupId) and
this.redis.invalidateUserDashboardCache(actorUserId) and return the existing {
success: true } response.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40551356-4c8f-4080-8d90-f5088041d640
📒 Files selected for processing (10)
.github/workflows/ci.yml.gitignoreapps/backend/src/groups/groups.service.tsapps/backend/src/receipts/receipts.service.spec.tsapps/backend/src/settlements/settlements.integration.spec.tsapps/web/app/layout.tsxapps/web/src/components/groups/CreateExpenseModal.tsxapps/web/src/components/theme/ThemeProvider.tsxapps/web/src/components/theme/ThemeToggle.tsxturbo.json
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/theme/ThemeToggle.tsx
- apps/web/src/components/groups/CreateExpenseModal.tsx
Comment on lines
+12
to
+19
| try { | ||
| const storageKey = 'fs-theme'; | ||
| const stored = localStorage.getItem(storageKey); | ||
| const mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system'; | ||
| const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; | ||
| const resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode; | ||
| var storageKey = 'fs-theme'; | ||
| var stored = localStorage.getItem(storageKey); | ||
| var mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system'; | ||
| var prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; | ||
| var resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode; | ||
| document.documentElement.dataset.theme = resolved; | ||
| document.body.dataset.theme = resolved; | ||
| } catch (_) { | ||
| document.documentElement.dataset.theme = 'light'; | ||
| document.body.dataset.theme = 'light'; | ||
| } | ||
| } catch (_) {} |
Contributor
There was a problem hiding this comment.
Don’t swallow init failures without applying a fallback theme.
With Line 44 removing the default data-theme, an empty catch on Line 19 can leave the page without a theme attribute if localStorage/matchMedia access throws.
🛠️ Proposed fix
(function() {
try {
var storageKey = 'fs-theme';
var stored = localStorage.getItem(storageKey);
var mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system';
var prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches;
var resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode;
document.documentElement.dataset.theme = resolved;
- } catch (_) {}
+ } catch (_) {
+ document.documentElement.dataset.theme = 'light';
+ }
})();📝 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
| try { | |
| const storageKey = 'fs-theme'; | |
| const stored = localStorage.getItem(storageKey); | |
| const mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system'; | |
| const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; | |
| const resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode; | |
| var storageKey = 'fs-theme'; | |
| var stored = localStorage.getItem(storageKey); | |
| var mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system'; | |
| var prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; | |
| var resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode; | |
| document.documentElement.dataset.theme = resolved; | |
| document.body.dataset.theme = resolved; | |
| } catch (_) { | |
| document.documentElement.dataset.theme = 'light'; | |
| document.body.dataset.theme = 'light'; | |
| } | |
| } catch (_) {} | |
| (function() { | |
| try { | |
| var storageKey = 'fs-theme'; | |
| var stored = localStorage.getItem(storageKey); | |
| var mode = stored === 'light' || stored === 'dark' || stored === 'system' ? stored : 'system'; | |
| var prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches; | |
| var resolved = mode === 'system' ? (prefersDark ? 'dark' : 'light') : mode; | |
| document.documentElement.dataset.theme = resolved; | |
| } catch (_) { | |
| document.documentElement.dataset.theme = 'light'; | |
| } | |
| })(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/layout.tsx` around lines 12 - 19, The try block that reads
storageKey/stored/mode and sets document.documentElement.dataset.theme can throw
and currently the empty catch swallows errors leaving no data-theme; update the
catch to set a safe fallback theme (e.g., 'light' or 'system') on
document.documentElement.dataset.theme and optionally log the error (capture the
exception object) so failures are visible; ensure the logic still resolves based
on prefersDark/mode when available and that the catch covers both localStorage
and matchMedia failures.
| export function ThemeProvider({ children }: { children: React.ReactNode }) { | ||
| const [mode, setModeState] = useState<ThemeMode>('system'); | ||
| const [resolved, setResolved] = useState<ResolvedTheme>('light'); | ||
| const [resolved, setResolved] = useState<ResolvedTheme>('dark'); |
Contributor
There was a problem hiding this comment.
Initialize resolved from existing DOM theme to avoid first-paint mismatch.
Line 34 defaults resolved to 'dark', which can briefly conflict with the pre-hydration theme set in apps/web/app/layout.tsx (script sets document.documentElement.dataset.theme), causing a short flicker for light users.
🎯 Proposed fix
- const [resolved, setResolved] = useState<ResolvedTheme>('dark');
+ const [resolved, setResolved] = useState<ResolvedTheme>(() => {
+ if (typeof document !== 'undefined') {
+ const initial = document.documentElement.dataset.theme;
+ if (initial === 'light' || initial === 'dark') return initial;
+ }
+ return 'light';
+ });📝 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 [resolved, setResolved] = useState<ResolvedTheme>('dark'); | |
| const [resolved, setResolved] = useState<ResolvedTheme>(() => { | |
| if (typeof document !== 'undefined') { | |
| const initial = document.documentElement.dataset.theme; | |
| if (initial === 'light' || initial === 'dark') return initial; | |
| } | |
| return 'light'; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/theme/ThemeProvider.tsx` at line 34, The component
initializes the local state const [resolved, setResolved] =
useState<ResolvedTheme>('dark') which can mismatch the theme already applied on
document.documentElement and cause a flash; update ThemeProvider to derive the
initial resolved value from the DOM (e.g., read
document.documentElement.dataset.theme or computed style) when creating the
state (or in an effect that runs once) so resolved reflects the pre-hydration
theme; modify the useState initialization or add a useEffect that reads
document.documentElement.dataset.theme, validates it against ResolvedTheme
values, and calls setResolved(...) to avoid the first-paint mismatch.
…tion script - Remove theme initialization script from layout.tsx - Refactor ThemeProvider to only resolve themes without applying DOM changes - Hardcode dark background and text colors in globals.css - Enhance ThemeToggle with framer-motion animations and resolved theme usage
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/theme/ThemeProvider.tsx (1)
55-60:⚠️ Potential issue | 🟠 MajorTheme resolution is disconnected from guaranteed DOM application.
Line 60 updates React state only. But the stylesheet contract depends on a
data-themeattribute selector (apps/web/src/design/theme.tsLine 91-101). In provided context, onlyapps/web/src/components/layout/DashboardLayout.tsxLine 12-18 appliesdata-theme={resolved}, so routes/layouts not doing this will stay on default styling.🔧 Proposed fix (provider-level guarantee)
+ useEffect(() => { + document.documentElement.dataset.theme = resolved; + }, [resolved]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/theme/ThemeProvider.tsx` around lines 55 - 60, The ThemeProvider's setMode currently only updates React state (setModeState and setResolved) and localStorage (STORAGE_KEY), but never guarantees the document's data-theme attribute used by CSS selectors (resolveTheme) is applied to the DOM; update setMode to also set document.documentElement.setAttribute('data-theme', <resolved value>) after calling setResolved(resolveTheme(next)), and ensure the provider also applies the current resolved theme to document.documentElement on mount (e.g., in an effect that reads resolveTheme(modeState or stored value)) so routes/layouts that don't set data-theme still get correct styles.
♻️ Duplicate comments (1)
apps/web/src/components/theme/ThemeProvider.tsx (1)
30-42:⚠️ Potential issue | 🟡 MinorInitialize
resolvedfrom the initial persisted/system mode to avoid first-render mismatch.Line 30 starts
resolvedas'dark', then Line 41 corrects it after mount. That can briefly render the wrong theme for users with stored'light'or'system'+light preference.🎯 Proposed fix
- const [resolved, setResolved] = useState<ResolvedTheme>('dark'); + const [resolved, setResolved] = useState<ResolvedTheme>(() => { + if (typeof window === 'undefined') return 'dark'; + const raw = localStorage.getItem(STORAGE_KEY); + const initialMode: ThemeMode = + raw === 'light' || raw === 'dark' || raw === 'system' ? raw : 'system'; + return resolveTheme(initialMode); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/theme/ThemeProvider.tsx` around lines 30 - 42, resolved is initialized to the literal 'dark' causing a flash before useEffect runs; change initialization so resolved uses the same initial persisted/system calculation as readStored + resolveTheme (i.e., compute initial mode via readStored() or equivalent and pass resolveTheme(initialMode) to useState) and keep setResolved/useEffect as-is; update references to readStored, resolveTheme, resolved, and setResolved to ensure consistent initial value on first render.
🧹 Nitpick comments (1)
apps/web/app/globals.css (1)
45-46: Avoid conflicting hover shadow declarations in the same rule.Line 45 sets
box-shadow: var(--fs-shadow-elevated)but Line 46 addsshadow-xlvia@apply, which can override it. Keep one source of truth to avoid unpredictable styling drift.♻️ Proposed cleanup
.marketing-card:hover { box-shadow: var(--fs-shadow-elevated); - `@apply` -translate-y-1 border-[var(--fs-border)] shadow-xl; + `@apply` -translate-y-1 border-[var(--fs-border)]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/globals.css` around lines 45 - 46, This rule currently declares box-shadow: var(--fs-shadow-elevated) and also applies shadow-xl via `@apply` (in the same selector), causing conflicting shadow declarations; pick a single source of truth and remove one of them — either delete shadow-xl from the `@apply` list (-translate-y-1 border-[var(--fs-border)] shadow-xl) and keep box-shadow: var(--fs-shadow-elevated), or remove the explicit box-shadow line and rely on `@apply` shadow-xl; update the selector in apps/web/app/globals.css accordingly so only one shadow is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/globals.css`:
- Around line 147-148: The .grid-bg rule uses element-level opacity which makes
all children translucent; replace that approach by removing the opacity property
from the .grid-bg selector and instead add alpha channels to the gradient color
stops (e.g., use rgba()/hex with alpha or CSS color-mix for --fs-border) so only
the grid lines are semi-transparent while text and child elements retain full
opacity; update the CSS gradients in the .grid-bg rule (the two linear-gradient
declarations) to use transparentized versions of var(--fs-border) rather than
relying on opacity.
- Around line 20-21: The body has hardcoded colors (background-color: `#030303`;
color: white) that override theme variables and block theme switching; replace
these hardcoded values with the theme CSS variables (use var(--fs-background)
for background and var(--fs-text-primary) for text) and remove any conflicting
static colors so light/dark themes driven by --fs-background / --fs-text-primary
can take effect (add sensible fallbacks inside the var(...) if needed).
---
Outside diff comments:
In `@apps/web/src/components/theme/ThemeProvider.tsx`:
- Around line 55-60: The ThemeProvider's setMode currently only updates React
state (setModeState and setResolved) and localStorage (STORAGE_KEY), but never
guarantees the document's data-theme attribute used by CSS selectors
(resolveTheme) is applied to the DOM; update setMode to also set
document.documentElement.setAttribute('data-theme', <resolved value>) after
calling setResolved(resolveTheme(next)), and ensure the provider also applies
the current resolved theme to document.documentElement on mount (e.g., in an
effect that reads resolveTheme(modeState or stored value)) so routes/layouts
that don't set data-theme still get correct styles.
---
Duplicate comments:
In `@apps/web/src/components/theme/ThemeProvider.tsx`:
- Around line 30-42: resolved is initialized to the literal 'dark' causing a
flash before useEffect runs; change initialization so resolved uses the same
initial persisted/system calculation as readStored + resolveTheme (i.e., compute
initial mode via readStored() or equivalent and pass resolveTheme(initialMode)
to useState) and keep setResolved/useEffect as-is; update references to
readStored, resolveTheme, resolved, and setResolved to ensure consistent initial
value on first render.
---
Nitpick comments:
In `@apps/web/app/globals.css`:
- Around line 45-46: This rule currently declares box-shadow:
var(--fs-shadow-elevated) and also applies shadow-xl via `@apply` (in the same
selector), causing conflicting shadow declarations; pick a single source of
truth and remove one of them — either delete shadow-xl from the `@apply` list
(-translate-y-1 border-[var(--fs-border)] shadow-xl) and keep box-shadow:
var(--fs-shadow-elevated), or remove the explicit box-shadow line and rely on
`@apply` shadow-xl; update the selector in apps/web/app/globals.css accordingly so
only one shadow is defined.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a17e1cf-2466-4641-a3cd-02b2e5059d0e
📒 Files selected for processing (4)
apps/web/app/globals.cssapps/web/app/layout.tsxapps/web/src/components/theme/ThemeProvider.tsxapps/web/src/components/theme/ThemeToggle.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/theme/ThemeToggle.tsx
Comment on lines
+20
to
+21
| background-color: #030303; | ||
| color: white; |
Contributor
There was a problem hiding this comment.
Hardcoded body colors break theme switching behavior.
Lines 20-21 force dark colors (#030303 / white) and bypass --fs-background and --fs-text-primary, so light mode cannot render correctly even when theme state resolves to light.
🔧 Proposed fix
body {
- background-color: `#030303`;
- color: white;
+ background-color: var(--fs-background);
+ color: var(--fs-text-primary);
font-family: var(--fs-font-sans);
`@apply` transition-colors duration-300;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/globals.css` around lines 20 - 21, The body has hardcoded colors
(background-color: `#030303`; color: white) that override theme variables and
block theme switching; replace these hardcoded values with the theme CSS
variables (use var(--fs-background) for background and var(--fs-text-primary)
for text) and remove any conflicting static colors so light/dark themes driven
by --fs-background / --fs-text-primary can take effect (add sensible fallbacks
inside the var(...) if needed).
Comment on lines
+147
to
+148
| linear-gradient(to right, var(--fs-border) 1px, transparent 1px), | ||
| linear-gradient(to bottom, var(--fs-border) 1px, transparent 1px); |
Contributor
There was a problem hiding this comment.
opacity on .grid-bg will fade all child content, not just the grid lines.
Line 150 applies element-level opacity, so any text/content inside a .grid-bg container becomes translucent and may hurt readability/contrast. Prefer encoding transparency in the gradient colors instead of the whole element.
🔧 Proposed fix
.grid-bg {
background-image:
- linear-gradient(to right, var(--fs-border) 1px, transparent 1px),
- linear-gradient(to bottom, var(--fs-border) 1px, transparent 1px);
+ linear-gradient(to right, color-mix(in srgb, var(--fs-border) 15%, transparent) 1px, transparent 1px),
+ linear-gradient(to bottom, color-mix(in srgb, var(--fs-border) 15%, transparent) 1px, transparent 1px);
background-size: 40px 40px;
- opacity: 0.15;
}Also applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/globals.css` around lines 147 - 148, The .grid-bg rule uses
element-level opacity which makes all children translucent; replace that
approach by removing the opacity property from the .grid-bg selector and instead
add alpha channels to the gradient color stops (e.g., use rgba()/hex with alpha
or CSS color-mix for --fs-border) so only the grid lines are semi-transparent
while text and child elements retain full opacity; update the CSS gradients in
the .grid-bg rule (the two linear-gradient declarations) to use transparentized
versions of var(--fs-border) rather than relying on opacity.
…lean import - Remove BOM from React import - Change test assertion to check for 'Sign In' instead of 'Login'
…ables - Updated .env.example and added apps/backend/.env.example with new variables - Modified CI workflow to include SUPABASE_DATABASE_URL and DIRECT_URL for database migration and testing jobs
- Added JWT_REFRESH_SECRET, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, REDIS_URL, AWS_REGION, and S3_BUCKET - Updated STRIPE_SECRET_KEY to use test key format for consistency in CI pipeline
- Added NEXT_PUBLIC_SUPABASE_URL, NEXT_PUBLIC_SUPABASE_ANON_KEY, and NEXT_PUBLIC_API_URL to CI e2e job - Updated .env.example with NEXT_PUBLIC_SUPABASE_URL and NEXT_PUBLIC_SUPABASE_ANON_KEY placeholders for local development consistency
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 9 file(s) based on 9 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
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
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 by CodeRabbit
New Features
Improvements
UI