Feature #92: Member role pages and role-aware foundation#98
Feature #92: Member role pages and role-aware foundation#98FadyGergesRezk wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a mock-data infrastructure (persona-based fixtures, role-based scoping, a mock/live switch) and rebuilds the web client's dashboard, members, feedback, payments, sport-events, organization, and helper pages from placeholder "hello" screens into fully functional, filterable, role-aware views backed by shared UI primitives and view-model hooks. ChangesWeb Client Feature Buildout
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Page as Feature Page
participant ViewModel as useXViewModel
participant Query as useXQuery
participant MockOr as mockOr
participant Fixtures as Fixtures/Scope
participant Client as API Client
Page->>ViewModel: request view
ViewModel->>Query: fetch data
Query->>MockOr: choose mock or live
MockOr->>Fixtures: scopeX(getCurrentUser())
MockOr->>Client: GET endpoint (fallback)
Fixtures-->>Query: scoped fixture rows
Client-->>Query: live API rows
Query-->>ViewModel: raw data
ViewModel-->>Page: filtered/sorted view + stats
Estimated code review effortEstimated code review effort: 5 (Critical) | ~120 minutes Related Issues: Not specified in provided context. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!found) throw new Error('Sport not found') | ||
| return Promise.resolve(found) | ||
| }, | ||
| () => organizationClient.get<Sport>(`/sports/${name}`).then(r => r.data), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-client/src/index.css (1)
65-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify dark-mode contrast for
--primary-foreground.
--primarykeeps the same light/bright hue (oklch(0.768 0.233 130.85)) in dark mode, but--primary-foregroundis now set to white (oklch(1 0 0)) — likely low contrast on that light background. The parallel--sidebar-primary-foregroundfor the identical--sidebar-primarycolor uses a dark value (oklch(0.274 0.072 132.109), line 93) instead, suggesting the correct foreground for this hue is dark, not white.💡 Possible fix
--primary: oklch(0.768 0.233 130.85); - --primary-foreground: oklch(1 0 0); + --primary-foreground: oklch(0.274 0.072 132.109);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/index.css` around lines 65 - 73, The dark-mode `--primary-foreground` value in the `.dark` theme block appears to have poor contrast against the unchanged bright `--primary` color. Update the `--primary-foreground` token in `web-client/src/index.css` to match the darker contrast approach already used by `--sidebar-primary-foreground`, and keep the change scoped to the `.dark` theme variables so the primary button/text styling remains readable.web-client/src/features/auth/useAuth.ts (1)
1-20: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the stale snapshot block
useAuth.tsstill referencesKeycloakTokenParsedwithout importing it, and the localuser: AuthUserliteral is dead code now; it also no longer satisfiesAuthUserbecauseidis missing. Keep only thegetCurrentUser()path here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/auth/useAuth.ts` around lines 1 - 20, useAuth currently keeps a stale token snapshot path that is no longer needed and introduces type issues: it references KeycloakTokenParsed without importing it, defines an unused local user literal, and that literal does not satisfy AuthUser because id is missing. Remove the AuthTokenSnapshot type and the parsed/user construction from useAuth, and keep the return value based only on getCurrentUser() plus logout so the hook stays consistent with the current auth source.
🧹 Nitpick comments (9)
web-client/src/features/sport-events/model/useEventsViewModel.ts (1)
79-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated default filters — reuse the single source of truth.
This inline default duplicates
defaultFiltersalready defined ineventsUiStore.ts. If the two literals ever drift,buildEventsView's standalone default (used in tests/without a store) would silently diverge from the store's default.♻️ Suggested fix
-import { useEventsUiStore } from './eventsUiStore' +import { defaultEventsFilters, useEventsUiStore } from './eventsUiStore' ... - filters: EventsFilters = { - search: '', - status: 'all', - fromDate: '', - toDate: '', - sort: 'date-asc', - }, + filters: EventsFilters = defaultEventsFilters,(export
defaultFiltersasdefaultEventsFiltersfromeventsUiStore.ts.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/sport-events/model/useEventsViewModel.ts` around lines 79 - 90, The default filters in buildEventsView are duplicated and can drift from the store’s single source of truth. Update buildEventsView in useEventsViewModel.ts to use the shared default exported from eventsUiStore.ts (prefer the suggested defaultEventsFilters export) instead of an inline object literal, so tests and standalone usage stay aligned with the store’s defaultFilters.web-client/src/features/sport-events/api/queries.ts (1)
10-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused legacy aliases
sportEventsKeys,useSportEvents, anduseSportEvent; nothing in the codebase references them anymore, so they can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/sport-events/api/queries.ts` around lines 10 - 18, The module still exports unused legacy aliases that should be removed. Delete the `sportEventsKeys` re-export from `eventKeys` in this file, and also remove the unused `useSportEvents` and `useSportEvent` symbols wherever they are defined so only the current `eventKeys` API remains. Keep the existing `eventKeys` object intact and update any nearby exports or references to match the trimmed public surface.web-client/src/features/organization/pages/OrganizationPage.tsx (1)
26-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
aria-expanded/aria-hiddento the sport toggle for screen readers.The sport toggle button (
SportSection, Line 168-172) conveys expand/collapse state only visually via the rotatingChevronRighticon. Addingaria-expanded={expanded}on the button andaria-hidden="true"on the decorative chevron would make the disclosure state perceivable to assistive tech.♿ Proposed tweak
<button type="button" onClick={onToggle} + aria-expanded={expanded} className="flex w-full items-center gap-3 px-4 py-3 text-left transition-colors hover:bg-surface-sunken" > <ChevronRight + aria-hidden="true" className={`size-4 shrink-0 text-text-tertiary transition-transform ${ expanded ? 'rotate-90' : '' }`} />Also applies to: 151-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/organization/pages/OrganizationPage.tsx` around lines 26 - 104, The sport disclosure toggle in SportSection only communicates expanded state visually, so update the button to expose its state to assistive tech by adding aria-expanded based on the expanded prop and marking the ChevronRight icon as aria-hidden because it is decorative. Make the change in the SportSection toggle handler/UI used by OrganizationPage so screen readers can perceive the expand/collapse state correctly.web-client/src/app/pages/DashboardPage.tsx (1)
59-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the exported view-model interfaces over
NonNullable<ReturnType<...>>.
AdminCountsSection,BalanceCard,EventsCards, andFeedbackStatall derive their prop types viaNonNullable<ReturnType<typeof useDashboardViewModel>['view']['X']>, even thoughuseDashboardViewModel.tsalready exports clean named interfaces (DashboardAdminCountsSection,DashboardBalanceSection,DashboardEventsSection,DashboardFeedbackSection) for exactly this purpose.♻️ Proposed refactor
import { + type DashboardAdminCountsSection, + type DashboardBalanceSection, + type DashboardEventsSection, type DashboardFeedbackItem, + type DashboardFeedbackSection, type DashboardSectionState, type DashboardSportSection, useDashboardViewModel, } from './model/useDashboardViewModel' @@ - counts: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['adminCounts']> + counts: DashboardAdminCountsSection @@ - balance: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['myBalance']> + balance?: DashboardBalanceSection @@ - events: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['myEvents']> + events: DashboardEventsSection @@ - feedback: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['myFeedback']> + feedback: DashboardFeedbackSection🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/app/pages/DashboardPage.tsx` around lines 59 - 157, The section prop types are inferred indirectly with NonNullable<ReturnType<typeof useDashboardViewModel>['view'][...]> instead of using the exported view-model interfaces. Update AdminCountsSection, BalanceCard, EventsCards, and FeedbackStat to import and use the named types from useDashboardViewModel.ts (DashboardAdminCountsSection, DashboardBalanceSection, DashboardEventsSection, DashboardFeedbackSection) so the component props stay explicit and aligned with the view-model contract.web-client/src/features/feedback/pages/FeedbackPage.tsx (1)
293-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
Field/FieldLabelhelpers across pages.Per the provided graph context, near-identical
Field/FieldLabelcomponents are independently defined inSportEventsPage.tsxandOrganizationPage.tsxas well. Consider hoisting these into a shared UI primitive (e.g.components/ui/field.tsx) alongside the other new shared primitives added in this PR (Badge, StatCard, etc.) so styling stays in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/feedback/pages/FeedbackPage.tsx` around lines 293 - 306, The `Field` and `FieldLabel` helpers are duplicated across `FeedbackPage`, `SportEventsPage`, and `OrganizationPage`, so consolidate them into a shared UI primitive. Move the shared rendering/styling into a common component such as `components/ui/field.tsx` alongside the other reusable primitives in this PR, then update the page-level `Field`/`FieldLabel` usages to import the shared version instead of redefining them locally.web-client/src/features/payments/api/queries.ts (1)
37-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated "find + scope + throw" mock-lookup pattern.
useMemberBalance,useTransactionhere, anduseFeedbackinfeedback/api/queries.tsall repeat the same shape: find a fixture by id, scope it for the current user, and throw a "not found" error if the scoped result is empty. Extracting a small shared helper (e.g.mockFindScopedOrThrow(fixtures, predicate, scopeFn, message)) would remove this duplication and centralize the not-found-error convention across features.♻️ Example helper
// web-client/src/mocks/mockSwitch.ts export function mockFindScopedOrThrow<T>( items: T[], predicate: (item: T) => boolean, scope: (items: T[]) => T[], notFoundMessage: string, ): Promise<T> { const found = items.find(predicate) const scoped = found ? scope([found]) : [] if (!scoped[0]) return Promise.reject(new Error(notFoundMessage)) return Promise.resolve(scoped[0]) }Also applies to: 68-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/features/payments/api/queries.ts` around lines 37 - 53, The `useMemberBalance` query repeats the same fixture lookup/scoping/not-found flow used elsewhere, so extract it into a shared helper such as `mockFindScopedOrThrow` in the mock utilities. Update `useMemberBalance` (and the matching `useTransaction`/`useFeedback` call sites) to use that helper instead of inlining `find`, `scopeBalances`, and the manual `throw new Error('Balance not found')`, keeping the existing not-found message behavior consistent.web-client/src/mocks/scope.test.ts (1)
37-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMost scope tests re-derive "expected" values using copies of the implementation under test.
memberTeamIds,trainerTeamIds,directorSports,sportTeamIds,teamMemberIds(lines 37-79) duplicatescope.ts's own private helpers verbatim, and thescopeFeedback/scopeTransactions/scopeBalances/scopeEvents/scopeMembersdescribe blocks build their expectations by calling these copies rather than asserting concrete member/team IDs. Since this is role-based data-visibility logic (who sees whose feedback/finances), a shared bug in the team/sport-membership derivation would reproduce identically in both the implementation and the test, and go undetected. ThescopeMembersmember/adminassertions (line 180,toEqual([users.member.id])) show the better pattern already in use — extending a couple of hardcoded, concrete-ID assertions per role to the other scope functions would meaningfully raise confidence in this RBAC-adjacent logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/mocks/scope.test.ts` around lines 37 - 188, The scope tests are reusing copied helper logic instead of asserting concrete expected IDs, so they can mirror the same bug as the implementation. Update the expectations in scopeFeedback, scopeTransactions, scopeBalances, scopeEvents, and scopeMembers to use explicit fixture IDs and hardcoded role-visible results, and remove or minimize reliance on memberTeamIds, trainerTeamIds, directorSports, sportTeamIds, and teamMemberIds copies of scope.ts helpers. Keep the existing concrete pattern used in scopeMembers for users.member/users.admin and extend that style to the other role-based assertions.web-client/src/mocks/fixtures/organization.ts (1)
2122-2142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
TEAM_U16can drift from fixture data.
TEAM_U16is manually pinned to a team id rather than derived frommyTeamFixtures, which is computed right below it and already encodes "teams the signed-in member belongs to." IfteamFixtures/CURRENT_MEMBER_IDever change, this constant can silently point to a team the member no longer belongs to. The nameTEAM_U16is also unrelated to the team's actual name ("Football Juniors"), which is confusing.♻️ Suggested fix: derive from myTeamFixtures instead of hardcoding
-/** First team the signed-in member belongs to, for convenience. */ -export const TEAM_U16 = 'bbbbbbbb-0001-0000-9e37-000000009e37' - /** name -> Sport. */ export const sportsByName: Record<string, Sport> = Object.fromEntries( sportFixtures.map((s) => [s.name, s]), ) @@ /** Teams the signed-in member belongs to (trainees are resolved MemberRefs). */ export const myTeamFixtures: Team[] = teamFixtures.filter((t) => t.trainees.some((m) => m.id === CURRENT_MEMBER_ID), ) + +/** First team the signed-in member belongs to, for convenience. */ +export const TEAM_U16 = myTeamFixtures[0]?.id ?? ''🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/mocks/fixtures/organization.ts` around lines 2122 - 2142, The hardcoded TEAM_U16 constant can drift from the fixture data and should not be manually pinned; derive it from the already-computed myTeamFixtures (or the relevant team lookup) so it always matches the signed-in member’s actual team. Update the organization fixture in the area around TEAM_U16 and myTeamFixtures so the identifier is sourced from the fixture list rather than a literal UUID, and keep the reference aligned with the Football Juniors team data.web-client/src/mocks/fixtures/report.ts (1)
2-7: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent markdown formatting across report fixtures.
The Lena Roth entry (Line 3) uses proper markdown syntax (
# Development report,## Attendance, bold/italic), while the other three entries (Lines 4-6) use the same section titles as plain text with no#/##markers. SincereportTextByIdis rendered through a markdown renderer, these three reports will display as unstyled paragraphs instead of headed sections, unlike the first.💚 Suggested fix: normalize headings for the remaining entries
- "99999999-0013-0001-be1e-0000000bbe15": "Development report — Marie Wolf\n\nAttendance\nMarie has attended 10 of 14 sessions this term (71%).\n\nTechnical progress\nCoach feedback over the last month highlights steady improvement in core skills, with consistency under pressure noted as the main growth area.\n\nMatch involvement\nFeatured in recent fixtures with feedback trending positive.\n\nSuggested focus\nMaintain attendance momentum and dedicate warm-up drills to the highlighted growth area. Revisit in 4 weeks.", + "99999999-0013-0001-be1e-0000000bbe15": "# Development report — Marie Wolf\n\n## Attendance\nMarie has attended **10 of 14** sessions this term (71%).\n\n## Technical progress\nCoach feedback over the last month highlights steady improvement in core skills, with consistency under pressure noted as the main growth area.\n\n## Match involvement\nFeatured in recent fixtures with feedback trending positive.\n\n## Suggested focus\nMaintain attendance momentum and dedicate warm-up drills to the highlighted growth area. Revisit in **4 weeks**.",(apply the same pattern to the remaining two entries)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-client/src/mocks/fixtures/report.ts` around lines 2 - 7, The report fixtures in reportTextById are inconsistent because the Lena Roth entry uses markdown headings while the other entries are plain text, so they won’t render as headings in the markdown viewer. Update the remaining report strings in reportTextById to match the markdown structure used by the existing styled entry, including the main title and section headings in the same format. Keep the content aligned across the report fixture entries so the renderer displays them consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-client/src/app/pages/DashboardPage.tsx`:
- Around line 40-48: The DashboardPage rendering guard is using view.myBalance
instead of the loading/error state, which prevents BalanceCard from rendering
while data is still loading and can leave it unsafe when balance is undefined.
Update the conditional around BalanceCard in DashboardPage to gate on
states.myBalance so the card mounts for loading and error states, while still
passing the existing balance and state props through to BalanceCard.
In `@web-client/src/app/pages/model/useDashboardViewModel.ts`:
- Around line 86-87: The fixed DASHBOARD_NOW reference in useDashboardViewModel
is being applied unconditionally in buildEventsSection, which breaks
upcoming-event calculations in live mode. Update the logic around DASHBOARD_NOW
and buildEventsSection so the frozen date is only used when the mock data path
is enabled, and live mode uses the actual current time instead. Keep the
mock-vs-live decision tied to the existing env-switching flow in
useDashboardViewModel so the upcoming filter remains correct in both modes.
- Around line 221-226: `useDashboardViewModel` populates `myBalance` only when
`data?.balance` exists, unlike `myEvents` and `myFeedback`, so the balance card
disappears during loading/error states. Update the `myBalance` branch in
`buildDashboardViewModel` to always assign a balance view model placeholder when
`shouldShowBalance(user.role)` is true, and keep `states.myBalance` in sync with
loading state so `DashboardPage` can render the slot consistently. Use the
existing `buildBalanceSection` and `states.myBalance` pattern to mirror the
events/feedback behavior.
In `@web-client/src/components/ui/badge.tsx`:
- Line 1: BadgeProps uses React.ComponentProps but React is not imported in this
module. Update the imports in badge.tsx by adding a type-only React import, or
switch BadgeProps to use a direct ComponentProps import, so the type reference
is available where BadgeProps is defined.
In `@web-client/src/features/feedback/api/queries.ts`:
- Around line 36-51: The `useFeedback` detail query is missing the same cache
freshness window used by other mock-aware queries, so it refetches too often and
re-shows loading UI. Update `useFeedback` in `queries.ts` to include `staleTime:
30_000`, matching `useFeedbackList`, `useMemberBalance`, and `useTransaction`,
while keeping the existing `queryKey`, `queryFn`, and `enabled` behavior
unchanged.
In `@web-client/src/features/members/model/useMembersViewModel.ts`:
- Around line 29-46: The `memberTeamRows` helper is still letting `undefined`
sports flow into `MemberRow.sports`, which can later render an empty facet badge
instead of the intended empty state. Update the `sports` निर्माण in
`useMembersViewModel.ts` to filter out falsy `team.sport` values before
deduping/sorting, keeping it consistent with the `sportOptions` handling and
preventing `FacetBadges` from receiving blank labels.
In `@web-client/src/features/organization/api/queries.ts`:
- Around line 47-59: The useSport hook is using the wrong identifier throughout:
it accepts id but builds organizationKeys.sport, the mock lookup, the API URL,
and the enabled flag from name. Update useSport to consistently use the same
parameter name or swap all references to id in the hook, including the
sportsByName lookup and organizationClient.get call. If the identifier is used
in the request path, make sure it is safely encoded before interpolating it into
the /sports/... URL.
In `@web-client/src/features/payments/model/usePaymentsViewModel.ts`:
- Around line 14-23: `PaymentRow` is missing the stable transaction identifier,
which forces `PaymentsPage.tsx` to rely on an unstable composite key. Update the
`PaymentRow` interface and the `usePaymentsViewModel` mapping so each row
carries the underlying `Transaction.id`, similar to how `FeedbackRow` preserves
`id`. Then adjust `PaymentsPage.tsx` to key rows with that `id` instead of the
current date/description/index composite.
In `@web-client/src/features/sport-events/pages/SportEventsPage.tsx`:
- Around line 267-292: The FieldLabel and Field components in SportEventsPage
are duplicated verbatim in other detail pages, so consolidate them into a shared
UI primitive and reuse it here. Move the shared FieldLabel/Field implementation
into a common component such as a field UI module, then update SportEventsPage,
FeedbackPage, and OrganizationPage to import the shared version instead of
keeping local copies.
In `@web-client/src/types.ts`:
- Around line 95-102: The AuthUser contract is inconsistent: it declares roles[]
but all consumers and builders use role, so update AuthUser to include role:
Role (and remove or reconcile roles if it is unused) so tokenUser() and
mockPersona() in currentUser.ts type-check,
scopeFeedback/scopeTransactions/scopeBalances/scopeEvents/scopeMembers/scopeReport
in scope.ts can continue switching on user.role, AppShell.tsx can filter
NAV_ITEMS by user.role, and the AuthUser fixtures in scope.test.ts satisfy the
corrected shape.
- Around line 22-32: The EventSummary type currently intersects
S['EventSummary'] with a narrower attendees shape, which leaves the original
Reference[] requirement in place and causes schema mismatches. Update
EventSummary to omit attendees from S['EventSummary'] before reintroducing the
MemberRef[] version, and leave teams_linked as an added optional field only if
needed since it is not part of the generated summary schema. Use the existing
type aliases SportEvent and EventSummary as the locator when making this change.
In `@web-client/vite.config.ts`:
- Around line 21-32: The rewrite callback in the Vite proxy config does not
handle query-string-only service URLs like /api/v1/organization?active=true, so
those requests bypass the duplication logic and can 404 after Traefik strips the
prefix. Update the rewrite logic in the rewrite function to treat a ? after the
service name the same as a trailing slash or end-of-path, so the service segment
is duplicated before the query string. Keep the existing behavior for bare
service paths and preserve the current service list symbols used in the regex.
---
Outside diff comments:
In `@web-client/src/features/auth/useAuth.ts`:
- Around line 1-20: useAuth currently keeps a stale token snapshot path that is
no longer needed and introduces type issues: it references KeycloakTokenParsed
without importing it, defines an unused local user literal, and that literal
does not satisfy AuthUser because id is missing. Remove the AuthTokenSnapshot
type and the parsed/user construction from useAuth, and keep the return value
based only on getCurrentUser() plus logout so the hook stays consistent with the
current auth source.
In `@web-client/src/index.css`:
- Around line 65-73: The dark-mode `--primary-foreground` value in the `.dark`
theme block appears to have poor contrast against the unchanged bright
`--primary` color. Update the `--primary-foreground` token in
`web-client/src/index.css` to match the darker contrast approach already used by
`--sidebar-primary-foreground`, and keep the change scoped to the `.dark` theme
variables so the primary button/text styling remains readable.
---
Nitpick comments:
In `@web-client/src/app/pages/DashboardPage.tsx`:
- Around line 59-157: The section prop types are inferred indirectly with
NonNullable<ReturnType<typeof useDashboardViewModel>['view'][...]> instead of
using the exported view-model interfaces. Update AdminCountsSection,
BalanceCard, EventsCards, and FeedbackStat to import and use the named types
from useDashboardViewModel.ts (DashboardAdminCountsSection,
DashboardBalanceSection, DashboardEventsSection, DashboardFeedbackSection) so
the component props stay explicit and aligned with the view-model contract.
In `@web-client/src/features/feedback/pages/FeedbackPage.tsx`:
- Around line 293-306: The `Field` and `FieldLabel` helpers are duplicated
across `FeedbackPage`, `SportEventsPage`, and `OrganizationPage`, so consolidate
them into a shared UI primitive. Move the shared rendering/styling into a common
component such as `components/ui/field.tsx` alongside the other reusable
primitives in this PR, then update the page-level `Field`/`FieldLabel` usages to
import the shared version instead of redefining them locally.
In `@web-client/src/features/organization/pages/OrganizationPage.tsx`:
- Around line 26-104: The sport disclosure toggle in SportSection only
communicates expanded state visually, so update the button to expose its state
to assistive tech by adding aria-expanded based on the expanded prop and marking
the ChevronRight icon as aria-hidden because it is decorative. Make the change
in the SportSection toggle handler/UI used by OrganizationPage so screen readers
can perceive the expand/collapse state correctly.
In `@web-client/src/features/payments/api/queries.ts`:
- Around line 37-53: The `useMemberBalance` query repeats the same fixture
lookup/scoping/not-found flow used elsewhere, so extract it into a shared helper
such as `mockFindScopedOrThrow` in the mock utilities. Update `useMemberBalance`
(and the matching `useTransaction`/`useFeedback` call sites) to use that helper
instead of inlining `find`, `scopeBalances`, and the manual `throw new
Error('Balance not found')`, keeping the existing not-found message behavior
consistent.
In `@web-client/src/features/sport-events/api/queries.ts`:
- Around line 10-18: The module still exports unused legacy aliases that should
be removed. Delete the `sportEventsKeys` re-export from `eventKeys` in this
file, and also remove the unused `useSportEvents` and `useSportEvent` symbols
wherever they are defined so only the current `eventKeys` API remains. Keep the
existing `eventKeys` object intact and update any nearby exports or references
to match the trimmed public surface.
In `@web-client/src/features/sport-events/model/useEventsViewModel.ts`:
- Around line 79-90: The default filters in buildEventsView are duplicated and
can drift from the store’s single source of truth. Update buildEventsView in
useEventsViewModel.ts to use the shared default exported from eventsUiStore.ts
(prefer the suggested defaultEventsFilters export) instead of an inline object
literal, so tests and standalone usage stay aligned with the store’s
defaultFilters.
In `@web-client/src/mocks/fixtures/organization.ts`:
- Around line 2122-2142: The hardcoded TEAM_U16 constant can drift from the
fixture data and should not be manually pinned; derive it from the
already-computed myTeamFixtures (or the relevant team lookup) so it always
matches the signed-in member’s actual team. Update the organization fixture in
the area around TEAM_U16 and myTeamFixtures so the identifier is sourced from
the fixture list rather than a literal UUID, and keep the reference aligned with
the Football Juniors team data.
In `@web-client/src/mocks/fixtures/report.ts`:
- Around line 2-7: The report fixtures in reportTextById are inconsistent
because the Lena Roth entry uses markdown headings while the other entries are
plain text, so they won’t render as headings in the markdown viewer. Update the
remaining report strings in reportTextById to match the markdown structure used
by the existing styled entry, including the main title and section headings in
the same format. Keep the content aligned across the report fixture entries so
the renderer displays them consistently.
In `@web-client/src/mocks/scope.test.ts`:
- Around line 37-188: The scope tests are reusing copied helper logic instead of
asserting concrete expected IDs, so they can mirror the same bug as the
implementation. Update the expectations in scopeFeedback, scopeTransactions,
scopeBalances, scopeEvents, and scopeMembers to use explicit fixture IDs and
hardcoded role-visible results, and remove or minimize reliance on
memberTeamIds, trainerTeamIds, directorSports, sportTeamIds, and teamMemberIds
copies of scope.ts helpers. Keep the existing concrete pattern used in
scopeMembers for users.member/users.admin and extend that style to the other
role-based assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba49071e-4c5b-4064-b082-a4f0b9572451
⛔ Files ignored due to path filters (4)
web-client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlweb-client/public/RoostFavIcon.svgis excluded by!**/*.svgweb-client/public/RoostIcon.svgis excluded by!**/*.svgweb-client/public/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (64)
.gitignoreweb-client/.env.development.exampleweb-client/index.htmlweb-client/package.jsonweb-client/src/app/layout/AppShell.tsxweb-client/src/app/pages/DashboardPage.tsxweb-client/src/app/pages/api/dashboardQueries.tsweb-client/src/app/pages/model/useDashboardViewModel.tsweb-client/src/app/router/routes.tsxweb-client/src/components/ui/badge.tsxweb-client/src/components/ui/calendar.tsxweb-client/src/components/ui/data-table.tsxweb-client/src/components/ui/date-range-filter.tsxweb-client/src/components/ui/page-header.tsxweb-client/src/components/ui/popover.tsxweb-client/src/components/ui/select.tsxweb-client/src/components/ui/sidebar.tsxweb-client/src/components/ui/stat-card.tsxweb-client/src/components/ui/table-toolbar.tsxweb-client/src/features/auth/currentUser.tsweb-client/src/features/auth/useAuth.tsweb-client/src/features/feedback/api/queries.tsweb-client/src/features/feedback/model/feedbackUiStore.tsweb-client/src/features/feedback/model/useFeedbackViewModel.tsweb-client/src/features/feedback/pages/FeedbackPage.tsxweb-client/src/features/helper/api/queries.tsweb-client/src/features/helper/model/useReportViewModel.tsweb-client/src/features/helper/pages/HelperPage.tsxweb-client/src/features/helper/pages/ReportMarkdown.test.tsxweb-client/src/features/helper/pages/ReportMarkdown.tsxweb-client/src/features/letters/api/queries.tsweb-client/src/features/members/api/queries.tsweb-client/src/features/members/model/membersUiStore.tsweb-client/src/features/members/model/useMembersViewModel.tsweb-client/src/features/members/pages/MembersPage.tsxweb-client/src/features/organization/api/queries.tsweb-client/src/features/organization/model/useTeamsViewModel.tsweb-client/src/features/organization/pages/OrganizationPage.tsxweb-client/src/features/payments/api/queries.tsweb-client/src/features/payments/model/paymentsUiStore.tsweb-client/src/features/payments/model/usePaymentsViewModel.tsweb-client/src/features/payments/pages/PaymentsPage.tsxweb-client/src/features/sport-events/api/queries.tsweb-client/src/features/sport-events/model/eventsUiStore.tsweb-client/src/features/sport-events/model/useEventsViewModel.test.tsweb-client/src/features/sport-events/model/useEventsViewModel.tsweb-client/src/features/sport-events/pages/SportEventsPage.tsxweb-client/src/index.cssweb-client/src/lib/format.tsweb-client/src/mocks/fixtures/dashboard.tsweb-client/src/mocks/fixtures/events.tsweb-client/src/mocks/fixtures/feedback.tsweb-client/src/mocks/fixtures/finance.tsweb-client/src/mocks/fixtures/index.tsweb-client/src/mocks/fixtures/members.tsweb-client/src/mocks/fixtures/organization.tsweb-client/src/mocks/fixtures/report.tsweb-client/src/mocks/mockSwitch.tsweb-client/src/mocks/personas.tsweb-client/src/mocks/scope.test.tsweb-client/src/mocks/scope.tsweb-client/src/types.tsweb-client/src/vite-env.d.tsweb-client/vite.config.ts
| {(view.myBalance || view.myEvents || view.myFeedback) && ( | ||
| <div className="grid grid-cols-1 gap-3 sm:grid-cols-2 xl:grid-cols-4"> | ||
| {view.myBalance && <BalanceCard balance={view.myBalance} state={states.myBalance} />} | ||
| {view.myEvents && <EventsCards events={view.myEvents} state={states.myEvents} />} | ||
| {view.myFeedback && ( | ||
| <FeedbackStat feedback={view.myFeedback} state={states.myFeedback} /> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gate BalanceCard rendering on states.myBalance, not view.myBalance.
Because view.myBalance is only set once data has arrived (see companion comment in useDashboardViewModel.ts), the balance card doesn't render at all during loading (no skeleton) and could also crash if it ever needs to render with a state.error but balance undefined, since BalanceCard unconditionally accesses balance.balanceFormatted once past the isLoading check.
🐛 Proposed fix
- {view.myBalance && <BalanceCard balance={view.myBalance} state={states.myBalance} />}
+ {states.myBalance && <BalanceCard balance={view.myBalance} state={states.myBalance} />} function BalanceCard({
balance,
state,
}: {
- balance: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['myBalance']>
+ balance?: NonNullable<ReturnType<typeof useDashboardViewModel>['view']['myBalance']>
state?: DashboardSectionState
}) {
- if (state?.isLoading) return <Skeleton className="h-32 border" />
+ if (state?.isLoading || !balance) return <Skeleton className="h-32 border" />Also applies to: 85-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/app/pages/DashboardPage.tsx` around lines 40 - 48, The
DashboardPage rendering guard is using view.myBalance instead of the
loading/error state, which prevents BalanceCard from rendering while data is
still loading and can leave it unsafe when balance is undefined. Update the
conditional around BalanceCard in DashboardPage to gate on states.myBalance so
the card mounts for loading and error states, while still passing the existing
balance and state props through to BalanceCard.
| const DASHBOARD_NOW = new Date('2026-06-19T00:00:00Z') | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Hardcoded reference date breaks "upcoming events" once live data is used.
DASHBOARD_NOW is frozen at 2026-06-19T00:00:00Z and used unconditionally in buildEventsSection regardless of mock vs. live mode. Once the app is switched to the real API (the PR's stated goal: swap mocked→live by one env var), "upcoming" will be computed against a permanently stale reference date instead of the actual current time — real events before that frozen date will incorrectly show as upcoming, and events shortly after it will incorrectly be excluded once real "now" passes it.
🐛 Proposed fix
-const DASHBOARD_NOW = new Date('2026-06-19T00:00:00Z')
-
// "Recent" = latest N by created_at, no time window (decision 2026-06-26).
const RECENT_FEEDBACK_COUNT = 3
@@
function buildEventsSection(events: EventSummary[]): DashboardEventsSection {
+ const now = new Date()
const upcoming = events
- .filter((event) => new Date(event.start_time) >= DASHBOARD_NOW)
+ .filter((event) => new Date(event.start_time) >= now)
.toSorted((a, b) => new Date(a.start_time).getTime() - new Date(b.start_time).getTime())If a frozen "now" is genuinely needed for deterministic mock-mode demos, gate it behind the mock switch instead of applying it universally.
Also applies to: 116-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/app/pages/model/useDashboardViewModel.ts` around lines 86 -
87, The fixed DASHBOARD_NOW reference in useDashboardViewModel is being applied
unconditionally in buildEventsSection, which breaks upcoming-event calculations
in live mode. Update the logic around DASHBOARD_NOW and buildEventsSection so
the frozen date is only used when the mock data path is enabled, and live mode
uses the actual current time instead. Keep the mock-vs-live decision tied to the
existing env-switching flow in useDashboardViewModel so the upcoming filter
remains correct in both modes.
| if (shouldShowBalance(user.role) && data?.balance) { | ||
| view.myBalance = buildBalanceSection(data.balance) | ||
| states.myBalance = state | ||
| } else if (shouldShowBalance(user.role)) { | ||
| states.myBalance = state | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
myBalance populated inconsistently with myEvents/myFeedback, causing a missing loading skeleton.
Unlike myEvents/myFeedback, which are always populated via data?.x ?? [] regardless of load state, view.myBalance is only set when data?.balance is truthy. During loading (and on error), view.myBalance stays undefined, so DashboardPage's {view.myBalance && <BalanceCard .../>} never renders — the balance stat card slot is entirely absent (no skeleton) until data arrives, causing a layout shift, while the events/feedback cards correctly render their skeletons.
♻️ Suggested fix (pairs with a DashboardPage.tsx change to gate rendering on `states.myBalance` instead of `view.myBalance`)
- if (shouldShowBalance(user.role) && data?.balance) {
- view.myBalance = buildBalanceSection(data.balance)
- states.myBalance = state
- } else if (shouldShowBalance(user.role)) {
- states.myBalance = state
- }
+ if (shouldShowBalance(user.role)) {
+ view.myBalance = data?.balance ? buildBalanceSection(data.balance) : undefined
+ states.myBalance = state
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/app/pages/model/useDashboardViewModel.ts` around lines 221 -
226, `useDashboardViewModel` populates `myBalance` only when `data?.balance`
exists, unlike `myEvents` and `myFeedback`, so the balance card disappears
during loading/error states. Update the `myBalance` branch in
`buildDashboardViewModel` to always assign a balance view model placeholder when
`shouldShowBalance(user.role)` is true, and keep `states.myBalance` in sync with
loading state so `DashboardPage` can render the slot consistently. Use the
existing `buildBalanceSection` and `states.myBalance` pattern to mirror the
events/feedback behavior.
| @@ -0,0 +1,36 @@ | |||
| import { cn } from '@/lib/utils' | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check tsconfig for allowUmdGlobalAccess and confirm React import convention across UI components.
fd -e json tsconfig -x cat {}
rg -n "allowUmdGlobalAccess" .
rg -n "^import \* as React|^import type React|^import React" web-client/src/components/ui/Repository: AET-DevOps26/team-devoops
Length of output: 2518
Import React before using React.ComponentProps
BadgeProps references React.ComponentProps<'span'>, but this file only imports cn. Add import type * as React from 'react' (or switch to a direct ComponentProps import) so the type reference is valid in this module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/components/ui/badge.tsx` at line 1, BadgeProps uses
React.ComponentProps but React is not imported in this module. Update the
imports in badge.tsx by adding a type-only React import, or switch BadgeProps to
use a direct ComponentProps import, so the type reference is available where
BadgeProps is defined.
| export function useFeedback(id: string) { | ||
| return useQuery<Feedback>({ | ||
| queryKey: feedbackKeys.detail(id), | ||
| queryFn: () => feedbackClient.get<Feedback>(`/${id}`).then(r => r.data), | ||
| queryFn: () => | ||
| mockOr( | ||
| () => { | ||
| const found = feedbackDetailsById[id] | ||
| const scoped = found ? scopeFeedback([found], getCurrentUser()) : [] | ||
| if (!scoped[0]) throw new Error('Feedback not found') | ||
| return Promise.resolve(scoped[0]) | ||
| }, | ||
| () => feedbackClient.get<Feedback>(`/${id}`).then(r => r.data), | ||
| ), | ||
| enabled: !!id, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Missing staleTime on useFeedback detail query.
Every other single-record mock-aware query added in this PR (useMemberBalance, useTransaction in payments/api/queries.ts) sets staleTime: 30_000, and useFeedbackList in this same file does too. useFeedback is the outlier, so opening the same feedback detail sheet repeatedly will refetch (and briefly show the skeleton) every time instead of using cached data.
🩹 Proposed fix
export function useFeedback(id: string) {
return useQuery<Feedback>({
queryKey: feedbackKeys.detail(id),
+ staleTime: 30_000,
queryFn: () =>
mockOr(📝 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.
| export function useFeedback(id: string) { | |
| return useQuery<Feedback>({ | |
| queryKey: feedbackKeys.detail(id), | |
| queryFn: () => feedbackClient.get<Feedback>(`/${id}`).then(r => r.data), | |
| queryFn: () => | |
| mockOr( | |
| () => { | |
| const found = feedbackDetailsById[id] | |
| const scoped = found ? scopeFeedback([found], getCurrentUser()) : [] | |
| if (!scoped[0]) throw new Error('Feedback not found') | |
| return Promise.resolve(scoped[0]) | |
| }, | |
| () => feedbackClient.get<Feedback>(`/${id}`).then(r => r.data), | |
| ), | |
| enabled: !!id, | |
| }) | |
| } | |
| export function useFeedback(id: string) { | |
| return useQuery<Feedback>({ | |
| queryKey: feedbackKeys.detail(id), | |
| staleTime: 30_000, | |
| queryFn: () => | |
| mockOr( | |
| () => { | |
| const found = feedbackDetailsById[id] | |
| const scoped = found ? scopeFeedback([found], getCurrentUser()) : [] | |
| if (!scoped[0]) throw new Error('Feedback not found') | |
| return Promise.resolve(scoped[0]) | |
| }, | |
| () => feedbackClient.get<Feedback>(`/${id}`).then(r => r.data), | |
| ), | |
| enabled: !!id, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/features/feedback/api/queries.ts` around lines 36 - 51, The
`useFeedback` detail query is missing the same cache freshness window used by
other mock-aware queries, so it refetches too often and re-shows loading UI.
Update `useFeedback` in `queries.ts` to include `staleTime: 30_000`, matching
`useFeedbackList`, `useMemberBalance`, and `useTransaction`, while keeping the
existing `queryKey`, `queryFn`, and `enabled` behavior unchanged.
| export interface PaymentRow { | ||
| createdAt: string | ||
| date: string | ||
| title: string | ||
| rawDescription: string | ||
| description: string | ||
| amountFormatted: string | ||
| kind: PaymentKind | ||
| creatorName: string | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PaymentRow drops the transaction id.
Unlike FeedbackRow (which keeps id: feedback.id), PaymentRow never carries the underlying Transaction.id through. As a result, PaymentsPage.tsx has to key its table rows with a fragile composite of date/description/array-index instead of a stable identifier — reordering rows via sort/filter can cause React to misattribute row identity across renders.
🩹 Proposed fix
export interface PaymentRow {
+ id: string
createdAt: string
date: string
title: string rows: sortPaymentRows(filterPaymentRows(memberTransactions.map((transaction) => ({
+ id: transaction.id,
createdAt: transaction.created_at,
date: formatDateShort(transaction.created_at),PaymentsPage.tsx can then use key={transaction.id} instead of the composite key.
Also applies to: 102-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/features/payments/model/usePaymentsViewModel.ts` around lines
14 - 23, `PaymentRow` is missing the stable transaction identifier, which forces
`PaymentsPage.tsx` to rely on an unstable composite key. Update the `PaymentRow`
interface and the `usePaymentsViewModel` mapping so each row carries the
underlying `Transaction.id`, similar to how `FeedbackRow` preserves `id`. Then
adjust `PaymentsPage.tsx` to key rows with that `id` instead of the current
date/description/index composite.
| function EventsTableSkeleton() { | ||
| return ( | ||
| <div className="border bg-card p-5"> | ||
| <div className="space-y-3"> | ||
| {Array.from({ length: 5 }).map((_, index) => ( | ||
| <Skeleton key={index} className="h-9 w-full" /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| function FieldLabel({ children }: { children: React.ReactNode }) { | ||
| return ( | ||
| <p className="text-caption uppercase tracking-[0.1em] text-text-tertiary">{children}</p> | ||
| ) | ||
| } | ||
|
|
||
| function Field({ label, value }: { label: string; value: string }) { | ||
| return ( | ||
| <div> | ||
| <h1 className="font-display text-display-lg uppercase tracking-wide">Events</h1> | ||
| {hello && <p className="mt-2 text-sm text-muted-foreground">{hello}</p>} | ||
| <FieldLabel>{label}</FieldLabel> | ||
| <p className="mt-0.5 text-body-sm">{value}</p> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
FieldLabel/Field are duplicated verbatim across pages.
The same FieldLabel/Field implementations also appear in FeedbackPage.tsx and OrganizationPage.tsx (per cross-file evidence). Extracting these into a shared UI primitive (e.g. components/ui/field.tsx) would avoid three copies drifting apart as detail views evolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/features/sport-events/pages/SportEventsPage.tsx` around lines
267 - 292, The FieldLabel and Field components in SportEventsPage are duplicated
verbatim in other detail pages, so consolidate them into a shared UI primitive
and reuse it here. Move the shared FieldLabel/Field implementation into a common
component such as a field UI module, then update SportEventsPage, FeedbackPage,
and OrganizationPage to import the shared version instead of keeping local
copies.
| export type SportEvent = Omit<S['Event'], 'attendees' | 'creator' | 'teams_linked'> & { | ||
| attendees?: MemberRef[] | ||
| creator: MemberRef | ||
| teams_linked?: TeamRef[] | ||
| } | ||
| export type EventSummary = S['EventSummary'] & { | ||
| attendees?: MemberRef[] | ||
| teams_linked?: TeamRef[] | ||
| } | ||
| export type EventCreate = S['EventCreate'] | ||
| export type EventPartialUpdate = S['EventPartialUpdate'] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the generated OpenAPI schema type for EventSummary's attendees/teams_linked shape.
rg -n "EventSummary" --type=ts -g '!node_modules' -B2 -A10Repository: AET-DevOps26/team-devoops
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== top-level files =="
git ls-files | sed -n '1,200p'
echo
echo "== likely schema/type files =="
fd -H -I -t f 'types.ts|schema|openapi|swagger|generated|client' . 2>/dev/null | sed -n '1,200p'
echo
echo "== search EventSummary and related fields =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"EventSummary|teams_linked|attendees|type S =|export type S|interface S" .
echo
echo "== inspect web-client/src/types.ts =="
sed -n '1,120p' web-client/src/types.tsRepository: AET-DevOps26/team-devoops
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api/openapi.yaml: EventSummary schema block =="
sed -n '1941,1995p' api/openapi.yaml
echo
echo "== web-client/src/types.ts =="
sed -n '1,80p' web-client/src/types.ts
echo
echo "== generated EventSummary models for comparison =="
sed -n '1,260p' services/spring-event/src/generated/java/tum/devoops/eventservice/model/EventSummary.javaRepository: AET-DevOps26/team-devoops
Length of output: 9275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api/openapi.yaml: Reference schema block =="
sed -n '1880,1940p' api/openapi.yaml
echo
echo "== web-client generated schema typings for Reference/EventSummary =="
rg -n --hidden --glob 'web-client/src/**' --glob '!**/node_modules/**' \
"Reference:|EventSummary:|MemberRef|TeamRef" web-client/src
echo
echo "== web-client api schema type excerpts =="
sed -n '1,260p' web-client/src/api/client.tsRepository: AET-DevOps26/team-devoops
Length of output: 3781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web-client/src/api.ts: Reference and EventSummary =="
sed -n '620,900p' web-client/src/api.ts
echo
echo "== api/openapi.yaml: Reference schema block =="
rg -n -A20 -B5 "^ Reference:" api/openapi.yamlRepository: AET-DevOps26/team-devoops
Length of output: 12257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web-client fixture EventSummary shape =="
rg -n -A12 -B4 '"teams_linked"|"attendees"|"EventSummary"' web-client/src/mocks/fixtures/events.ts | sed -n '1,220p'
echo
echo "== web-client scope test references =="
sed -n '1,160p' web-client/src/mocks/scope.test.ts
echo
echo "== any EventSummary consumer expecting teams_linked =="
rg -n --hidden --glob 'web-client/src/**' "teams_linked.*EventSummary|EventSummary.*teams_linked|type EventSummary|EventSummary\[" web-client/srcRepository: AET-DevOps26/team-devoops
Length of output: 13161
Omit attendees before overriding EventSummary
S['EventSummary'] only defines attendees?: Reference[]; intersecting it with MemberRef[] forces both shapes at once, so the current EventSummary fixtures/consumers don’t match the generated schema. teams_linked isn’t part of S['EventSummary'], so the fix only needs to omit attendees before re-adding the resolved ref shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/types.ts` around lines 22 - 32, The EventSummary type
currently intersects S['EventSummary'] with a narrower attendees shape, which
leaves the original Reference[] requirement in place and causes schema
mismatches. Update EventSummary to omit attendees from S['EventSummary'] before
reintroducing the MemberRef[] version, and leave teams_linked as an added
optional field only if needed since it is not part of the generated summary
schema. Use the existing type aliases SportEvent and EventSummary as the locator
when making this change.
| export interface AuthUser { | ||
| id: string | ||
| name: string | ||
| email: string | ||
| // Membership roles from the Keycloak `member_roles` claim — a subset of | ||
| // 'Trainee' | 'Coach' | 'Director' | 'Admin'. | ||
| roles: string[] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
AuthUser is missing role: Role, but every consumer reads .role.
This interface declares roles: string[], yet:
currentUser.tsbuildstokenUser()/mockPersona()results and returns{ id, name, email, role: tokenRole(...) }(norolesfield at all).scope.tsswitches onuser.rolein every exported scope function (scopeFeedback,scopeTransactions,scopeBalances,scopeEvents,scopeMembers,scopeReport).scope.test.tsbuildsusersviaMOCK_PERSONAS... satisfies Record<string, AuthUser>and then calls scope functions expecting.roleto exist.AppShell.tsxfilters nav items withNAV_ITEMS.filter((item) => item.roles.includes(user.role)).
A function/object literal checked against a declared type undergoes excess-property and missing-required-property checking, so tokenUser()'s return statement (missing roles, adding unknown role) and any direct AuthUser object literals would fail to type-check as written. This is a pervasive cross-file contract break, not a local defect.
🛠️ Proposed fix
export interface AuthUser {
id: string
name: string
email: string
- // Membership roles from the Keycloak `member_roles` claim — a subset of
- // 'Trainee' | 'Coach' | 'Director' | 'Admin'.
- roles: string[]
+ role: Role
}📝 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.
| export interface AuthUser { | |
| id: string | |
| name: string | |
| email: string | |
| // Membership roles from the Keycloak `member_roles` claim — a subset of | |
| // 'Trainee' | 'Coach' | 'Director' | 'Admin'. | |
| roles: string[] | |
| } | |
| export interface AuthUser { | |
| id: string | |
| name: string | |
| email: string | |
| role: Role | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/src/types.ts` around lines 95 - 102, The AuthUser contract is
inconsistent: it declares roles[] but all consumers and builders use role, so
update AuthUser to include role: Role (and remove or reconcile roles if it is
unused) so tokenUser() and mockPersona() in currentUser.ts type-check,
scopeFeedback/scopeTransactions/scopeBalances/scopeEvents/scopeMembers/scopeReport
in scope.ts can continue switching on user.role, AppShell.tsx can filter
NAV_ITEMS by user.role, and the AuthUser fixtures in scope.test.ts satisfy the
corrected shape.
| // Traefik strips the full `/api/v1/<name>` prefix, so duplicate the service | ||
| // segment to survive it; also trim a bare trailing slash the services 404 on. | ||
| rewrite: (p) => | ||
| p | ||
| .replace( | ||
| /^(\/api\/v1\/(?:organization|members|events|feedback|finance|letters|helper))\/(\?|$)/, | ||
| '$1$2', | ||
| ) | ||
| .replace( | ||
| /^\/api\/v1\/(organization|members|events|feedback|finance|letters|helper)(\/|$)/, | ||
| '/api/v1/$1/$1$2', | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Query-string-only requests bypass the duplication rewrite.
For a bare service call like /api/v1/organization?active=true (no trailing slash before ?), neither regex matches: the first requires a literal / before ?/end, and the second requires / or end-of-string immediately after the service name — ? satisfies neither. The path is passed through unrewritten, so it won't survive Traefik's prefix-stripping as described in the comment, and any list/filter endpoint called this way (a very common REST pattern) will likely 404 against the real backend.
🐛 Proposed fix
- // Traefik strips the full `/api/v1/<name>` prefix, so duplicate the service
- // segment to survive it; also trim a bare trailing slash the services 404 on.
- rewrite: (p) =>
- p
- .replace(
- /^(\/api\/v1\/(?:organization|members|events|feedback|finance|letters|helper))\/(\?|$)/,
- '$1$2',
- )
- .replace(
- /^\/api\/v1\/(organization|members|events|feedback|finance|letters|helper)(\/|$)/,
- '/api/v1/$1/$1$2',
- ),
+ // Traefik strips the full `/api/v1/<name>` prefix, so duplicate the service
+ // segment to survive it; also trim a bare trailing slash the services 404 on.
+ // Query strings are split off first so this works regardless of a trailing slash.
+ rewrite: (p) => {
+ const services = 'organization|members|events|feedback|finance|letters|helper'
+ const [rawPath, query = ''] = p.split(/(?=\?)/)
+ const path = rawPath.replace(new RegExp(`^(/api/v1/(?:${services}))/$`), '$1')
+ return (
+ path.replace(new RegExp(`^/api/v1/(${services})(/|$)`), '/api/v1/$1/$1$2') + query
+ )
+ },📝 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.
| // Traefik strips the full `/api/v1/<name>` prefix, so duplicate the service | |
| // segment to survive it; also trim a bare trailing slash the services 404 on. | |
| rewrite: (p) => | |
| p | |
| .replace( | |
| /^(\/api\/v1\/(?:organization|members|events|feedback|finance|letters|helper))\/(\?|$)/, | |
| '$1$2', | |
| ) | |
| .replace( | |
| /^\/api\/v1\/(organization|members|events|feedback|finance|letters|helper)(\/|$)/, | |
| '/api/v1/$1/$1$2', | |
| ), | |
| // Traefik strips the full `/api/v1/<name>` prefix, so duplicate the service | |
| // segment to survive it; also trim a bare trailing slash the services 404 on. | |
| // Query strings are split off first so this works regardless of a trailing slash. | |
| rewrite: (p) => { | |
| const services = 'organization|members|events|feedback|finance|letters|helper' | |
| const [rawPath, query = ''] = p.split(/(?=\?)/) | |
| const path = rawPath.replace(new RegExp(`^(/api/v1/(?:${services}))/$`), '$1') | |
| return ( | |
| path.replace(new RegExp(`^/api/v1/(${services})(/|$)`), '/api/v1/$1/$1$2') + query | |
| ) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-client/vite.config.ts` around lines 21 - 32, The rewrite callback in the
Vite proxy config does not handle query-string-only service URLs like
/api/v1/organization?active=true, so those requests bypass the duplication logic
and can 404 after Traefik strips the prefix. Update the rewrite logic in the
rewrite function to treat a ? after the service name the same as a trailing
slash or end-of-path, so the service segment is duplicated before the query
string. Keep the existing behavior for bare service paths and preserve the
current service list symbols used in the regex.
2 participants
Why
Builds the Member-role UI and the role-aware foundation the other role PRs (#93/#94/#95) will extend. Replaces pages that read raw fixtures inline with a proper three-layer flow (TanStack query → selector/view-model → zustand UI store), adds a runtime mock switch so the app runs fully on fixtures today and flips to live with one env var, and ships the dashboard, role-filtered sidebar, and Roost branding.
The fixtures in
web-client/src/mocks/fixtures/are not throwaway test data — they are how we believe the server responses should be shaped. The client is typed against them, so they double as a contract proposal. Where a fixture's shape differs from what a service returns today, that difference is a request to the server team, not an accident. Please read the highlights below and comment in this PR if you disagree.Going live is designed to be a one-line swap: every read query is
mockOr(mockFn, liveFn)and the liveaxioscall is already written, just gated behindVITE_USE_MOCKS.git grep mockOris the full inventory.New things that need your agreement (object to any of these)
MemberRef { id, first_name, last_name }andTeamRef { id, name }instead of uuid strings — e.g.Team.trainers/trainees,Sport.directors,Feedback.member/creator,Transaction.member/creator,Event.attendees/creator. Write DTOs (*Create/*PartialUpdate) stay bare ids. This removes the need for client-side id→name resolution.GET /members/dashboardreturning a role-scoped superset object ({ member, events, feedback, balance, report, sports, teams }) — sections by entitlement, not a{role,data}discriminator. The server identifies the caller from the token (no/me). The dashboard page reads this verbatim and derives nothing client-side.GET /feedback: members see feedback about themselves, trainers see what they authored, directors see none, admins see all). The mock layer (src/mocks/scope.ts) emulates this exactly so the demo shows real role differences — but the real enforcement must live on the server.Feedback.rating— a nullable0–10integer onFeedback+FeedbackSummary(not in the live schema yet). UI hides the rating column when absent.Sportkeyed byname(its PK / path param);Team.sportis a sport name.Event.sports_linkedstaysstring[]of names.amount_cents, no currency field — client displays EUR; negative = charge, positive = payment. Overdue/Clear is derived client-side from balance sign.What changed
subfrom the token via a singlegetCurrentUser()resolver;roleLabel()(member → "Member");VITE_USE_MOCKSswitch + devVITE_MOCK_PERSONAto view the app as any role.scope.ts) + per-persona dashboard fixture.api/queries.ts(cache) →model/use*ViewModel.ts(joins/derivations) →model/*UiStore.ts(UI-only state). Pages consume only the view-model + store; no page imports fixtures.dangerouslySetInnerHTML), Members, and a composed Dashboard on the index route.react-day-pickerfor the calendar; dev Vite proxy rewrite;.env.development.example.Notes
src/mocks/fixtures/— review effort should focus onsrc/types.ts,src/mocks/scope.ts, themodel/view-models, and the contract highlights above.VITE_USE_MOCKS=true). Copy.env.development.example→.env.development. To run against live services setVITE_USE_MOCKS=false.mockOrswap.Testing
pnpm -C web-client lint,typecheck,build, andtest(50 passing) all green. Scope helpers have unit coverage across all four roles.Closes #92
Summary by CodeRabbit