Client #93: Coach role pages#102
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR rebuilds the web-client with a role-based type model, mock/live data-switching infrastructure, shared UI primitives, and rewritten feature pages (dashboard, members, feedback, organization, letters, sport-events, helper) driven by view-model hooks. Separately, spring-event gains trainer/director entities and authorization for event creation. ChangesWeb client mock-driven feature rebuild
Event creation authorization
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SportEventEditorDialog
participant useCreateSportEvent
participant mockOr
participant QueryCache
SportEventEditorDialog->>useCreateSportEvent: mutateAsync(payload)
useCreateSportEvent->>mockOr: select mock or live
mockOr-->>QueryCache: setQueryData(eventKeys.detail)
useCreateSportEvent-->>SportEventEditorDialog: close editor, notice
sequenceDiagram
participant Client
participant EventService
participant TrainerRepository
participant DirectorRepository
Client->>EventService: createEvent(requesterId, body)
EventService->>EventService: check isAdmin
EventService->>TrainerRepository: existsById(teamId, requesterId)
TrainerRepository-->>EventService: false
EventService->>DirectorRepository: existsById(sportId, requesterId)
DirectorRepository-->>EventService: true/false
EventService-->>Client: created or ForbiddenException
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)services/spring-event/src/main/java/tum/devoops/eventservice/repository/DirectorRepository.javaNo java executable found in PATH Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
web-client/src/app/pages/model/useDashboardViewModel.ts (1)
203-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch waterfall: org/events queries gated on
data?.roleinstead of the already-knownuser.role.
sportsQuery/teamsQuery/eventsQueryare only enabled oncedashboardQueryresolves and exposesdata.role, even thoughrole = user.role(line 219) is available synchronously fromuseAuth(). This forces a sequential round trip (dashboard fetch → then org/events fetch) instead of firing them in parallel, adding latency to admin and trainee/trainer/director dashboards alike.If the token role reliably predicts the envelope role (which
shouldShowBalancebelow already assumes), gating onroleinstead ofdata?.rolewould let these fire in parallel with the dashboard call.♻️ Proposed fix
- const isAdmin = data?.role === 'admin' + const isAdmin = role === 'admin' const sportsQuery = useSportsList(isAdmin) const teamsQuery = useTeamsList(isAdmin) // ... - const isNonAdmin = !!data && data.role !== 'admin' + const isNonAdmin = role !== 'admin' const eventsQuery = useEventsList(isNonAdmin)🤖 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 203 - 217, The org/events queries in useDashboardViewModel are being gated on dashboardQuery.data.role, which creates a fetch waterfall even though useAuth already provides user.role synchronously. Update the enablement logic for sportsQuery, teamsQuery, and eventsQuery to key off the already-known role (or an equivalent derived value) instead of data?.role, while keeping the existing admin/non-admin branching behavior intact. This will let the dashboard and dependent queries start in parallel without changing the role-based routing in useDashboardViewModel.web-client/src/features/feedback/model/useFeedbackViewModel.ts (1)
290-296: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate
useTeamsListhere;useMembersneeds anenabledoption first
buildFeedbackCoverage()returnsnullfor non-trainers, souseTeamsListcan be skipped on those roles.useMembersis still always on, so add anenabledparam there if you want to avoid that fetch too.🤖 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/model/useFeedbackViewModel.ts` around lines 290 - 296, Gate the team/member fetches in useFeedbackViewModel so non-trainers do not load unnecessary data: use the user.role check to conditionally run useTeamsList since buildFeedbackCoverage() returns null for non-trainers, and update useMembers to accept an enabled option so its fetch can also be skipped when the role should not load members. Keep the change localized to useFeedbackViewModel and the useMembers hook contract so the enabled flag controls the query behavior.web-client/src/mocks/fixtures/organization.ts (1)
756-758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
TEAM_U16id with a name that doesn't match the referenced team.The comment says "first team the signed-in member belongs to," but the constant is named
TEAM_U16while it points to"Football Juniors"(bbbbbbbb-0001…), which isn't described as a U16-specific squad anywhere in this file. Consider deriving this frommyTeamFixtures[0]?.id(or renaming to something accurate) so it can't drift fromCURRENT_MEMBER_ID/myTeamFixturesif either changes.🤖 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 756 - 758, The TEAM_U16 constant is hardcoded and may drift from the actual first team for the signed-in member. Update the fixture in organization.ts so the exported team id is derived from myTeamFixtures[0]?.id (or rename the constant to match the actual team it represents) and keep it aligned with CURRENT_MEMBER_ID and the first entry in myTeamFixtures.web-client/src/mocks/scope.test.ts (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
scopeTeamReporthas no test coverage.
scope.tsexportsscopeTeamReport(used to gate team-report visibility for trainer/director/member roles), but this file only imports/testsscopeBalances,scopeEvents,scopeFeedback,scopeMembers,scopeReport,scopeTransactions. Given this function guards role-based access to team reports, add adescribe('scopeTeamReport', ...)block mirroring thescopeReporttests (trainer sees own-team reports, director sees own-sport's teams, member is always denied).🤖 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 1 - 22, Add test coverage for scopeTeamReport in the scope.test suite. The current test file imports scopeBalances, scopeEvents, scopeFeedback, scopeMembers, scopeReport, and scopeTransactions but never exercises scopeTeamReport, so create a describe('scopeTeamReport', ...) block alongside the existing role-based scope tests. Mirror the scopeReport patterns using MOCK_PERSONAS and the fixtures helpers to verify trainer access to own-team reports, director access to own-sport teams, and that member access is always denied.web-client/vite.config.ts (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the rewrite regex into a testable pure function.
The path-rewrite logic (query splitting, service-segment duplication, trailing-slash trim) is non-trivial and currently only lives inline in the Vite config with no unit coverage. A regex edit (new service name, edge case) could silently break every dev API call. Since dev-proxy misbehavior is easy to miss until manual testing, a small extracted/tested helper would catch regressions early.
🤖 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 - 34, The inline path rewrite logic in the Vite config is too complex to leave untested, so extract it from the rewrite callback into a pure helper that handles query splitting, service duplication, and trailing-slash normalization. Move the regex-based transformation from the Vite proxy’s rewrite function into a named function that can be imported and unit tested, then keep the config callback as a thin wrapper around that helper. Add tests for the extracted helper covering existing service names, query strings, and trailing slash cases so future regex changes don’t silently break API calls.web-client/src/features/auth/currentUser.ts (1)
19-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the mock-persona identity path.
getCurrentUser()branches onUSE_MOCKS/VITE_MOCK_PERSONA, but only thetokenUser()fallback path is exercised inuseAuth.test.ts. Given this function is the single source of identity for role-gated navigation and data scoping across the app, consider adding coverage for:USE_MOCKS=truewith a valid persona key, an invalid/unsetVITE_MOCK_PERSONA(should default to'member'), andUSE_MOCKS=false(falls through to token).🤖 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/currentUser.ts` around lines 19 - 29, Add test coverage for the identity branching in getCurrentUser/mockPersona so the mock-persona path is exercised, not just tokenUser(). In useAuth.test.ts, cover USE_MOCKS=true with a valid VITE_MOCK_PERSONA key, an invalid or unset VITE_MOCK_PERSONA defaulting to the 'member' entry in MOCK_PERSONAS, and USE_MOCKS=false falling through to tokenUser(); use the getCurrentUser and mockPersona symbols to locate the logic.web-client/src/__tests__/useAuth.test.ts (1)
1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests implicitly depend on
USE_MOCKSbeing false; not isolated from env.Both tests set
keycloakMock.tokenParsedand expectuseAuth()to use the token path, butgetCurrentUser()now checksmockPersona()first:if (!USE_MOCKS) return null(currentUser.ts, Line 20). SinceUSE_MOCKSis derived fromimport.meta.env.VITE_USE_MOCKSat module load and neithermockSwitchnorpersonasis mocked here, these assertions silently rely on that env var being unset/false during the vitest run. If a root.envsetsVITE_USE_MOCKS=true(loaded by Vite for any mode unless overridden by.env.test),getCurrentUser()would return a mock persona instead of the token-derived user, and both tests would fail for a reason unrelated to the code under test.Consider explicitly mocking
@/mocks/mockSwitchto force the token path, making the test deterministic regardless of environment configuration.🧪 Suggested test isolation fix
import { describe, expect, it, vi } from 'vitest' +vi.mock('`@/mocks/mockSwitch`', () => ({ USE_MOCKS: false, mockOr: (_m: unknown, live: () => unknown) => live() })) + const keycloakMock = {🤖 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/__tests__/useAuth.test.ts` around lines 1 - 34, The useAuth tests are implicitly relying on the runtime value of USE_MOCKS, which makes them environment-dependent and brittle. Update the test setup in useAuth.test.ts to explicitly mock the mock-switch path used by currentUser/getCurrentUser so the tokenParsed branch is always exercised, regardless of VITE_USE_MOCKS. Keep the assertions focused on useAuth, keycloakMock, and the getCurrentUser/mockPersona flow so the tests remain deterministic.web-client/src/features/organization/api/queries.ts (1)
47-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
retry: falsefor single-item lookups.When the mock/live lookup misses,
Sport not found/Team not foundis a deterministic error, not a transient one. Withoutretry: false, react-query's default retry behavior will retry the failed query multiple times before surfacing the error, delaying any "not found" UI state.Also applies to: 113-127
🤖 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/api/queries.ts` around lines 47 - 61, The single-item lookup queries in useSport and the matching useTeam hook are throwing deterministic “not found” errors, so react-query should not retry them. Add retry: false to the useQuery options for these id-based lookups so a missing Sport/Team fails immediately and the UI can show the not-found state without delay.web-client/src/features/organization/model/useTeamsViewModel.ts (1)
16-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
SportTeamsViewdropsid, forcing downstream code to key offname.
OrganizationPage.tsxusessport.namefor the Reactkey, theactiveOpenSportexpand/collapse state, andstats.mySports(aSetofsportName). If two sports ever share the samename, this causes duplicate React keys, colliding expand/collapse state between unrelated sports, and undercountedmySports. Prefer carrying the sportidthroughSportTeamsViewand using it as the stable identifier.♻️ Proposed fix
export interface SportTeamsView { + id: string name: string description: string directors: MemberRef[] teams: TeamView[] }const joinedSports = sports.map((sport) => ({ + id: sport.id, name: sport.name, description: sport.description, directors: sport.directors, teams: teamsBySportId.get(sport.id) ?? [], }))Also applies to: 59-64
🤖 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/model/useTeamsViewModel.ts` around lines 16 - 21, SportTeamsView is missing the sport identifier, which forces downstream consumers to rely on name as the stable key. Update the SportTeamsView interface and the related mapping in useTeamsViewModel so the sport id is preserved alongside name, then switch OrganizationPage.tsx and any consumers of activeOpenSport and stats.mySports to use that id as the unique identifier instead of sport.name.web-client/src/features/helper/model/useReportViewModel.ts (1)
46-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTrainer with no matched team surfaces a generic "Report not allowed" error.
If
teamisnull(trainer not yet linked to any team, orteamsQueryhasn't resolved),generateTeamis created withteamId=''; clicking generate will fail insidescopeTeamReport('', ...)and surfacegenerateError.messageas the unhelpful "Report not allowed" string in the UI, rather than indicating the real cause (no team assigned).Consider disabling the generate action (or short-circuiting
generate) whenscope === 'team' && !team.🤖 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/helper/model/useReportViewModel.ts` around lines 46 - 52, The team-report generation path in useReportViewModel is creating a generator with an empty team id when team is null, which leads to the generic failure message instead of a clear no-team state. Update the generateTeam/useGenerateTeamReport flow so that when scope is 'team' and team is missing, the generate action is disabled or short-circuited before calling scopeTeamReport, and only use generateTeam when team?.id is available.web-client/src/features/helper/api/queries.ts (1)
96-128: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPer-report read/delete skip the scope checks used elsewhere in this file.
useReport(lines 98-111) anduseDeleteReport(lines 116-128) look up/delete byreportIdalone with noscopeReport/scopeTeamReportcheck in the mock branch, unlikeuseGenerateMemberReport/useGenerateTeamReportand the list hooks, which all gate access via scope. Today this is masked becausereportIdonly ever comes from an already-scoped list row, but it's an inconsistent invariant: any code path (or future URL-driven detail view) that supplies an arbitraryreportIdcould read or delete a report the current mock user shouldn't have access to.🔒 Suggested fix: reuse scope helpers for single-report access
export function useReport(reportId: string | null) { return useQuery<Report>({ queryKey: helperKeys.report(reportId ?? ''), enabled: !!reportId, queryFn: () => mockOr( () => { const found = reportId ? reportById[reportId] : undefined - if (!found) throw new Error('Report not found') + if (!found) throw new Error('Report not found') + const allowed = + found.kind === 'team' + ? scopeTeamReport(found.team?.id ?? '', getCurrentUser()) + : scopeReport(found.member?.id ?? '', getCurrentUser()) + if (!allowed) throw new Error('Report not found') return Promise.resolve(found) }, () => helperClient.get<Report>(`/reports/${reportId}`).then(r => r.data), ), }) }🤖 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/helper/api/queries.ts` around lines 96 - 128, The single-report hooks bypass the same scope guard used elsewhere, so align `useReport` and `useDeleteReport` with the existing scoped access pattern. In the mock branch of `queryFn` and `mutationFn`, resolve the report through `scopeReport` or `scopeTeamReport` before reading or deleting, instead of using `reportId` directly. Reuse the same helper logic already present in `useGenerateMemberReport`, `useGenerateTeamReport`, and the list hooks so `helperKeys.report`, `reportById`, and `helperClient` only operate on scoped reports.web-client/src/features/sport-events/model/useEventsViewModel.ts (1)
25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
missedCountis declared but never populated.
EventsView.missedCountis an optional field thatbuildEventsViewnever sets, andSportEventsPage.tsx's stat cards only readstats.upcoming/thisWeek/total. This looks like leftover scaffolding for an unfinished "missed" stat card — either wire it up or drop the field to avoid misleading future consumers who might rely on it being present.Also applies to: 156-164
🤖 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 25 - 34, The EventsView.missedCount field is leftover scaffolding because buildEventsView does not populate it and SportEventsPage only consumes stats.upcoming/thisWeek/total. Either remove missedCount from EventsView and any related typing in useEventsViewModel/buildEventsView, or wire it through consistently by computing and assigning it where the view model is built and updating the consuming UI if it is meant to be displayed.web-client/src/features/sport-events/api/queries.ts (1)
86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
canManageEventfor reuse instead of duplicating the check in the UI.
SportEventsPage.tsx(line 274) reimplements this exact condition (user.role === 'admin' || detail.creator?.id === user.id) inline. Exporting this helper and importing it in the page would prevent the two authorization checks from silently drifting apart.♻️ Proposed refactor
-function canManageEvent(user: AuthUser, event: SportEvent): boolean { +export function canManageEvent(user: AuthUser, event: SportEvent): boolean { return user.role === 'admin' || event.creator?.id === user.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/sport-events/api/queries.ts` around lines 86 - 88, Export the canManageEvent helper from queries.ts and reuse it in SportEventsPage instead of keeping the same authorization condition inline. Update the existing canManageEvent function to be the single source of truth for the admin-or-creator check, then import and call it from the page where the inline user.role === 'admin' || detail.creator?.id === user.id logic currently lives. This keeps the permission rule centralized and prevents the two checks from diverging.
🤖 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/model/useDashboardViewModel.ts`:
- Around line 92-101: The trainer/coach dashboard is missing a loading state for
the team card because DashboardViewModel.states has no myTeam entry and
fillSections does not assign a skeleton for the trainer path. Update
useDashboardViewModel’s DashboardViewModel and fillSections logic to include a
myTeam DashboardSectionState during the loading branch, then thread that state
through DashboardPage.tsx so TeamCard can render a placeholder while data is
loading instead of appearing only after view.myTeam is populated.
In `@web-client/src/components/ui/alert-dialog.tsx`:
- Around line 27-41: The AlertDialogOverlay className uses an invalid Tailwind
v4 supports variant, so the backdrop blur utility is not generated. Update the
variant in AlertDialogOverlay to the bracketed supports-[...] form while keeping
the existing backdrop blur behavior, and make sure the cn() class string still
includes the blur utility for the overlay.
In `@web-client/src/components/ui/table-toolbar.tsx`:
- Around line 25-32: The local search state in table-toolbar.tsx is only
initialized from searchValue once, so draftSearch can get stale when the parent
updates the filter externally. Update the table toolbar component to resync
draftSearch whenever searchValue changes, using the existing useState/useEffect
flow around draftSearch, setDraftSearch, and the search input handlers. Make
sure the sync keeps user typing behavior intact while allowing external resets
like clear filters or tab changes to immediately reflect in the input.
In `@web-client/src/features/helper/api/queries.ts`:
- Around line 28-60: The mock generate mutations in useGenerateMemberReport and
useGenerateTeamReport only resolve and invalidate queries, so they never change
the static report fixtures. Update the mock branches to mutate the in-memory
records in web-client/src/mocks/fixtures/report.ts (including reportById,
memberReportSummariesById, and teamReportSummariesById) before invalidating
queries, so generated reports persist in mock mode and refetches return the
updated data.
In `@web-client/src/features/organization/pages/OrganizationPage.tsx`:
- Around line 319-323: The Coaches list in OrganizationPage is missing the
current user context, so MemberBadges cannot mark the signed-in coach as “you.”
Update the MemberBadges usage for team.trainers to pass the same currentUserId
value that RosterMembers already receives, ensuring MemberBadges can correctly
compute isCurrentUser and display the badge for the coach’s own name.
In `@web-client/src/features/sport-events/api/queries.ts`:
- Around line 106-127: The mock event creation path in mockCreateEvent is
missing the same role-based authorization used by mockUpdateEvent and
mockDeleteEvent. Add an authorization check before building/upserting the event,
reusing the existing canManageEvent or equivalent create-role gate so only
coach/director/admin can create events, and reject unauthorized callers of
useCreateSportEvent() in the mock API layer rather than relying only on
SportEventsPage.tsx.
In `@web-client/src/features/sport-events/components/SportEventEditorDialog.tsx`:
- Around line 186-203: The handleTeamToggle logic in SportEventEditorDialog is
preserving sports that were only added via previously selected teams because it
unions nextTeamIds’ sports into current.sportIds without removing deselected
team-driven sports. Update the team toggle flow so sportIds is derived from the
current teamIds selection (or split explicit sport picks from team-derived ones)
instead of only appending to current.sportIds, and keep syncAttendeesForTeams
aligned with the updated teamIds/sportIds state.
In `@web-client/src/lib/format.ts`:
- Around line 39-57: The date helpers in format.ts currently call new Date(iso)
and pass the result straight into Intl.DateTimeFormat.format, which can throw on
invalid timestamps. Update formatDate, formatDateShort, formatDateTime, and
formatTime to validate the parsed Date first and return a safe fallback when the
input is missing or malformed; use the existing helper names and DATE,
DATE_SHORT, WEEKDAY, and TIME formatters to keep the fix localized.
In `@web-client/src/mocks/fixtures/dashboard.ts`:
- Around line 69-99: The trainer/director dashboard mock is inconsistent because
`dashboard.ts` returns a single displayed `team` or `sport` from `trainerTeams`,
`directorSportIds`, and `sportRef`, but the aggregate fields still sum across
all matched teams/sports. Update the `trainer` and `director` cases so the
totals in `total_members`, `upcoming_events`, `recent_feedback`,
`sport_balance_cents`, and `teams` are scoped to the same selected team/sport,
or otherwise change the payload shape to represent multiple teams/sports
consistently. Use the existing symbols `trainerTeams`, `directorSportIds`,
`upcomingEvents`, `scopeFeedback`, and `teamBalance` to keep the data model
aligned.
---
Nitpick comments:
In `@web-client/src/__tests__/useAuth.test.ts`:
- Around line 1-34: The useAuth tests are implicitly relying on the runtime
value of USE_MOCKS, which makes them environment-dependent and brittle. Update
the test setup in useAuth.test.ts to explicitly mock the mock-switch path used
by currentUser/getCurrentUser so the tokenParsed branch is always exercised,
regardless of VITE_USE_MOCKS. Keep the assertions focused on useAuth,
keycloakMock, and the getCurrentUser/mockPersona flow so the tests remain
deterministic.
In `@web-client/src/app/pages/model/useDashboardViewModel.ts`:
- Around line 203-217: The org/events queries in useDashboardViewModel are being
gated on dashboardQuery.data.role, which creates a fetch waterfall even though
useAuth already provides user.role synchronously. Update the enablement logic
for sportsQuery, teamsQuery, and eventsQuery to key off the already-known role
(or an equivalent derived value) instead of data?.role, while keeping the
existing admin/non-admin branching behavior intact. This will let the dashboard
and dependent queries start in parallel without changing the role-based routing
in useDashboardViewModel.
In `@web-client/src/features/auth/currentUser.ts`:
- Around line 19-29: Add test coverage for the identity branching in
getCurrentUser/mockPersona so the mock-persona path is exercised, not just
tokenUser(). In useAuth.test.ts, cover USE_MOCKS=true with a valid
VITE_MOCK_PERSONA key, an invalid or unset VITE_MOCK_PERSONA defaulting to the
'member' entry in MOCK_PERSONAS, and USE_MOCKS=false falling through to
tokenUser(); use the getCurrentUser and mockPersona symbols to locate the logic.
In `@web-client/src/features/feedback/model/useFeedbackViewModel.ts`:
- Around line 290-296: Gate the team/member fetches in useFeedbackViewModel so
non-trainers do not load unnecessary data: use the user.role check to
conditionally run useTeamsList since buildFeedbackCoverage() returns null for
non-trainers, and update useMembers to accept an enabled option so its fetch can
also be skipped when the role should not load members. Keep the change localized
to useFeedbackViewModel and the useMembers hook contract so the enabled flag
controls the query behavior.
In `@web-client/src/features/helper/api/queries.ts`:
- Around line 96-128: The single-report hooks bypass the same scope guard used
elsewhere, so align `useReport` and `useDeleteReport` with the existing scoped
access pattern. In the mock branch of `queryFn` and `mutationFn`, resolve the
report through `scopeReport` or `scopeTeamReport` before reading or deleting,
instead of using `reportId` directly. Reuse the same helper logic already
present in `useGenerateMemberReport`, `useGenerateTeamReport`, and the list
hooks so `helperKeys.report`, `reportById`, and `helperClient` only operate on
scoped reports.
In `@web-client/src/features/helper/model/useReportViewModel.ts`:
- Around line 46-52: The team-report generation path in useReportViewModel is
creating a generator with an empty team id when team is null, which leads to the
generic failure message instead of a clear no-team state. Update the
generateTeam/useGenerateTeamReport flow so that when scope is 'team' and team is
missing, the generate action is disabled or short-circuited before calling
scopeTeamReport, and only use generateTeam when team?.id is available.
In `@web-client/src/features/organization/api/queries.ts`:
- Around line 47-61: The single-item lookup queries in useSport and the matching
useTeam hook are throwing deterministic “not found” errors, so react-query
should not retry them. Add retry: false to the useQuery options for these
id-based lookups so a missing Sport/Team fails immediately and the UI can show
the not-found state without delay.
In `@web-client/src/features/organization/model/useTeamsViewModel.ts`:
- Around line 16-21: SportTeamsView is missing the sport identifier, which
forces downstream consumers to rely on name as the stable key. Update the
SportTeamsView interface and the related mapping in useTeamsViewModel so the
sport id is preserved alongside name, then switch OrganizationPage.tsx and any
consumers of activeOpenSport and stats.mySports to use that id as the unique
identifier instead of sport.name.
In `@web-client/src/features/sport-events/api/queries.ts`:
- Around line 86-88: Export the canManageEvent helper from queries.ts and reuse
it in SportEventsPage instead of keeping the same authorization condition
inline. Update the existing canManageEvent function to be the single source of
truth for the admin-or-creator check, then import and call it from the page
where the inline user.role === 'admin' || detail.creator?.id === user.id logic
currently lives. This keeps the permission rule centralized and prevents the two
checks from diverging.
In `@web-client/src/features/sport-events/model/useEventsViewModel.ts`:
- Around line 25-34: The EventsView.missedCount field is leftover scaffolding
because buildEventsView does not populate it and SportEventsPage only consumes
stats.upcoming/thisWeek/total. Either remove missedCount from EventsView and any
related typing in useEventsViewModel/buildEventsView, or wire it through
consistently by computing and assigning it where the view model is built and
updating the consuming UI if it is meant to be displayed.
In `@web-client/src/mocks/fixtures/organization.ts`:
- Around line 756-758: The TEAM_U16 constant is hardcoded and may drift from the
actual first team for the signed-in member. Update the fixture in
organization.ts so the exported team id is derived from myTeamFixtures[0]?.id
(or rename the constant to match the actual team it represents) and keep it
aligned with CURRENT_MEMBER_ID and the first entry in myTeamFixtures.
In `@web-client/src/mocks/scope.test.ts`:
- Around line 1-22: Add test coverage for scopeTeamReport in the scope.test
suite. The current test file imports scopeBalances, scopeEvents, scopeFeedback,
scopeMembers, scopeReport, and scopeTransactions but never exercises
scopeTeamReport, so create a describe('scopeTeamReport', ...) block alongside
the existing role-based scope tests. Mirror the scopeReport patterns using
MOCK_PERSONAS and the fixtures helpers to verify trainer access to own-team
reports, director access to own-sport teams, and that member access is always
denied.
In `@web-client/vite.config.ts`:
- Around line 21-34: The inline path rewrite logic in the Vite config is too
complex to leave untested, so extract it from the rewrite callback into a pure
helper that handles query splitting, service duplication, and trailing-slash
normalization. Move the regex-based transformation from the Vite proxy’s rewrite
function into a named function that can be imported and unit tested, then keep
the config callback as a thin wrapper around that helper. Add tests for the
extracted helper covering existing service names, query strings, and trailing
slash cases so future regex changes don’t silently break API calls.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 6f495221-4725-4f89-ad58-15f703925f79
⛔ 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 (81)
.gitignoreweb-client/.env.development.exampleweb-client/index.htmlweb-client/package.jsonweb-client/src/__tests__/DashboardPage.test.tsxweb-client/src/__tests__/useAuth.test.tsweb-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/alert-dialog.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/dialog.tsxweb-client/src/components/ui/input.tsxweb-client/src/components/ui/label.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/components/ui/textarea.tsxweb-client/src/features/auth/currentUser.tsweb-client/src/features/auth/useAuth.tsweb-client/src/features/feedback/api/queries.tsweb-client/src/features/feedback/components/FeedbackComposeDialog.tsxweb-client/src/features/feedback/index.tsweb-client/src/features/feedback/model/feedbackUiStore.tsweb-client/src/features/feedback/model/useFeedbackViewModel.test.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/helperUiStore.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/letters/types/index.tsweb-client/src/features/members/api/queries.tsweb-client/src/features/members/model/membersUiStore.tsweb-client/src/features/members/model/useMembersViewModel.test.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.test.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/components/SportEventEditorDialog.tsxweb-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/lib/server-error.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.test.tsweb-client/src/types.tsweb-client/src/vite-env.d.tsweb-client/vite.config.ts
Comment on lines
+25
to
+32
| const searchId = useId() | ||
| const [draftSearch, setDraftSearch] = useState(searchValue) | ||
|
|
||
| useEffect(() => { | ||
| const timeout = window.setTimeout(() => onSearchChange(draftSearch), 250) | ||
|
|
||
| return () => window.clearTimeout(timeout) | ||
| }, [draftSearch, onSearchChange]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
draftSearch never resyncs when searchValue changes externally.
draftSearch is only seeded from searchValue on mount. If a parent clears/resets the search filter externally (e.g., a "clear filters" button, tab switch, or programmatic reset), the input keeps showing the stale local value since there's no effect syncing draftSearch to prop changes.
🐛 Proposed fix to sync draftSearch with external searchValue changes
const searchId = useId()
const [draftSearch, setDraftSearch] = useState(searchValue)
+ useEffect(() => {
+ setDraftSearch(searchValue)
+ }, [searchValue])
+
useEffect(() => {
const timeout = window.setTimeout(() => onSearchChange(draftSearch), 250)
return () => window.clearTimeout(timeout)
}, [draftSearch, onSearchChange])📝 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 searchId = useId() | |
| const [draftSearch, setDraftSearch] = useState(searchValue) | |
| useEffect(() => { | |
| const timeout = window.setTimeout(() => onSearchChange(draftSearch), 250) | |
| return () => window.clearTimeout(timeout) | |
| }, [draftSearch, onSearchChange]) | |
| const searchId = useId() | |
| const [draftSearch, setDraftSearch] = useState(searchValue) | |
| useEffect(() => { | |
| setDraftSearch(searchValue) | |
| }, [searchValue]) | |
| useEffect(() => { | |
| const timeout = window.setTimeout(() => onSearchChange(draftSearch), 250) | |
| return () => window.clearTimeout(timeout) | |
| }, [draftSearch, onSearchChange]) |
🤖 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/table-toolbar.tsx` around lines 25 - 32, The
local search state in table-toolbar.tsx is only initialized from searchValue
once, so draftSearch can get stale when the parent updates the filter
externally. Update the table toolbar component to resync draftSearch whenever
searchValue changes, using the existing useState/useEffect flow around
draftSearch, setDraftSearch, and the search input handlers. Make sure the sync
keeps user typing behavior intact while allowing external resets like clear
filters or tab changes to immediately reflect in the input.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
POST /events was gated only by the blanket hasAnyRole('admin','member')
check, so any authenticated member (including plain trainees) could
create events, and a trainer/director could create one for a team/sport
they don't actually coach/direct -- contradicting the endpoint's own
OpenAPI description ("Directors: for their sport. Trainers: for their
team. Admins: any event.").
Adds read-only shadows of organization.trainers/organization.directors
(same pattern already used for TeamEntity/SportEntity here, and for all
four in spring-letter) plus a sport_id column on the existing TeamEntity
shadow, so createEvent can check the requester is a trainer of one of
the linked teams, or a director of one of the linked/team-derived
sports, before saving -- otherwise 403.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
|
lgtm! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@services/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.java`:
- Around line 192-215: The canCreateEvent method in EventService currently
grants access when requesterId matches any single team or sport in the request,
which allows mixed authorized and unauthorized links to slip through. Update the
validation logic so every linked team in teamsLinked and every linked sport in
sportsLinked is checked individually against trainerRepository and
directorRepository, rather than using anyMatch over the whole set. Keep the
existing parsing and repository lookups, but change the authorization flow to
reject the request unless all linked entities are in scope for the requester.
🪄 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: 06938f0d-a088-4475-8788-15779f16bf62
📒 Files selected for processing (7)
services/spring-event/src/main/java/tum/devoops/eventservice/entity/DirectorEntity.javaservices/spring-event/src/main/java/tum/devoops/eventservice/entity/TeamEntity.javaservices/spring-event/src/main/java/tum/devoops/eventservice/entity/TrainerEntity.javaservices/spring-event/src/main/java/tum/devoops/eventservice/repository/DirectorRepository.javaservices/spring-event/src/main/java/tum/devoops/eventservice/repository/TrainerRepository.javaservices/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.javaservices/spring-event/src/test/java/tum/devoops/eventservice/service/EventServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- services/spring-event/src/main/java/tum/devoops/eventservice/repository/TrainerRepository.java
- services/spring-event/src/main/java/tum/devoops/eventservice/repository/DirectorRepository.java
Comment on lines
+192
to
+215
| // Per the API contract: trainers may only create events for a team they coach; directors | ||
| // only for a sport they direct (either linked directly via sports_linked, or via one of the | ||
| // linked teams' sport). Admins bypass this entirely (checked by the caller). | ||
| private boolean canCreateEvent(UUID requesterId, EventCreate body) { | ||
| List<UUID> teamIds = body.getTeamsLinked() == null | ||
| ? List.of() | ||
| : body.getTeamsLinked().stream().map(t -> parseUuid(t, "teams_linked")).collect(Collectors.toList()); | ||
|
|
||
| boolean isTrainerOfTeam = teamIds.stream() | ||
| .anyMatch(teamId -> trainerRepository.existsById(new TrainerEntity.Id(teamId, requesterId))); | ||
| if (isTrainerOfTeam) { | ||
| return true; | ||
| } | ||
|
|
||
| Set<UUID> sportIds = new HashSet<>(); | ||
| if (body.getSportsLinked() != null) { | ||
| sportIds.addAll(body.getSportsLinked()); | ||
| } | ||
| teamRepository.findAllById(teamIds).forEach(team -> sportIds.add(team.getSportId())); | ||
|
|
||
| return sportIds.stream() | ||
| .anyMatch(sportId -> directorRepository.existsById(new DirectorEntity.Id(sportId, requesterId))); | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the service and nearby tests
git ls-files 'services/spring-event/**' | sed -n '1,200p'
echo '--- EventService outline ---'
ast-grep outline services/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.java --view expanded
echo '--- Search for canCreateEvent and create-event tests ---'
rg -n "canCreateEvent|teamsLinked|sportsLinked|create event|EventCreate" services/spring-event/src/test services/spring-event/src/main/java -SRepository: AET-DevOps26/team-devoops
Length of output: 10170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- EventService.java (100-220) ---'
sed -n '100,220p' services/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.java | cat -n
echo '--- EventServiceTest.java (190-290) ---'
sed -n '190,290p' services/spring-event/src/test/java/tum/devoops/eventservice/service/EventServiceTest.java | cat -n
echo '--- EventCreate model fields ---'
sed -n '1,220p' services/spring-event/src/generated/java/tum/devoops/eventservice/model/EventCreate.java | cat -nRepository: AET-DevOps26/team-devoops
Length of output: 18727
Check every linked team/sport before allowing create
services/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.java:195-215 accepts the request if the requester matches any one linked team or any sport in the combined set. That lets a trainer/director mix one authorized link with unauthorized teams/sports and still create the event with the full payload. Add coverage for mixed authorized/unauthorized links and require each linked team/sport to be individually in scope.
🤖 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
`@services/spring-event/src/main/java/tum/devoops/eventservice/service/EventService.java`
around lines 192 - 215, The canCreateEvent method in EventService currently
grants access when requesterId matches any single team or sport in the request,
which allows mixed authorized and unauthorized links to slip through. Update the
validation logic so every linked team in teamsLinked and every linked sport in
sportsLinked is checked individually against trainerRepository and
directorRepository, rather than using anyMatch over the whole set. Keep the
existing parsing and repository lookups, but change the authorization flow to
reject the request unless all linked entities are in scope for the requester.
This was referenced Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Why
Build the Coach (trainer) role experience so a coach can see their team on the dashboard, run their team's events end to end, and give/track feedback for their trainees, without any of it leaking to other roles.
What changed
Notes
Testing
pnpm -C web-client lint,typecheck,build, andtestall pass (74 tests)Relates to #93
Summary by CodeRabbit
New Features
Bug Fixes