feature(demo-game): make report's page available for players#78
feature(demo-game): make report's page available for players#78
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files in a game application. The changes include updating navigation structures, implementing optional chaining in components, modifying GraphQL queries, and enhancing authentication mechanisms. The updates primarily focus on improving data handling, routing, and user access control in the game's frontend and backend components. Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/components/NavBar.tsx (1)
52-54: Nitpick: “hover:” class name appears incomplete
Check if"hover:"needs additional tailwind classes, such ashover:bg-slate-100. If nothing is needed, consider removing it for clarity.<Button className={{ root: 'p-0 hover:' }}> - <Logo imgPathAvatar={playerImgPathAvatar} color={playerColor} /> +<Button className={{ root: 'p-0' }}> + <Logo imgPathAvatar={playerImgPathAvatar} color={playerColor} /> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/demo-game/src/components/DecisionsDisplay.tsx(1 hunks)apps/demo-game/src/pages/play/cockpit.tsx(1 hunks)apps/demo-game/src/pages/play/welcome.tsx(4 hunks)packages/ui/src/components/NavBar.tsx(3 hunks)
🔇 Additional comments (10)
apps/demo-game/src/components/DecisionsDisplay.tsx (1)
110-112: Great use of optional chaining to avoid runtime errors
This change safeguards against potential undefined values for period or segment, preventing crashes at runtime. Ensure that the rest of the component logic gracefully handles cases when these objects are absent (e.g., display fallback labels or placeholders to maintain user clarity).
apps/demo-game/src/pages/play/cockpit.tsx (1)
127-133: Validate permissions for accessing “Reports”
Replacing all tabs with just “Reports” might cause unauthorized users to see or access /admin/reports/<id>. If only certain roles can access this page, remember to check permissions or show an appropriate error. Otherwise, this concise navigation is fine.
packages/ui/src/components/NavBar.tsx (2)
10-10: Importing Button is appropriate
Introducing the Button from @uzh-bf/design-system is consistent with using the design system for styling and interaction. Looks good.
27-27: Defaulting playerHref to “/play/welcome”
This aligns the navigation flow with the new default start page, ensuring consistency after removing or shifting the “Cockpit” tab.
apps/demo-game/src/pages/play/welcome.tsx (6)
6-7: Switching to FormikSelectField and FormikTextField is a smart move.
This aligns with the standard naming conventions and presumably harmonizes your code with the latest library components.
18-18: Import placement looks consistent.
Importing LogoSelector earlier clarifies when it’s used and promotes better readability.
120-120: Styling consistency.
Changing to double quotes for the className attribute is fine and aligns with many code style guidelines.
160-165: Form field usage is consistent with library standards.
Using FormikTextField for “Name of bank” and FormikSelectField for “Location” is straightforward and clear. Be sure to validate these fields thoroughly in your tests.
179-179: LogoSelector integration.
This component nicely encapsulates avatar selection. Consider verifying that the selected avatar updates and persists as expected.
185-185: Color selection logic is well-integrated.
Providing color options via the FormikSelectField ensures consistent design with the rest of your form.
rschlaefli
left a comment
rschlaefli
left a comment
There was a problem hiding this comment.
We should also make sure that the admin reports page does not return future data anywhere (not sure if we already ensured that in the backend or not). Just so that it is not possible to see the future dice or similar when looking at the browser console (some students might be smart enough for that :D).
jajakob
force-pushed
the
jakob/report-for-players
branch
from
86da2ad to
ab80807
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/demo-game/src/pages/play/cockpit.tsx (1)
Line range hint
262-264: Improve error handling and loading statesThe current error handling is basic and might not be user-friendly. Consider adding proper error boundaries and meaningful error messages.
- if (loading) return null; - if (error) return `Error! ${error}`; + if (loading) return <LoadingSpinner />; + if (error) return ( + <ErrorDisplay + message="Unable to load game data" + details={error.message} + retry={() => refetch()} + /> + );
🧹 Nitpick comments (2)
apps/demo-game/src/pages/play/cockpit.tsx (2)
Line range hint
1-1000: Consider splitting game states into separate componentsThe switch statement handling different game states makes the component very large and complex. Consider splitting each game state into its own component for better maintainability and testing.
Example structure:
// components/game-states/PreparationState.tsx const PreparationState = ({ currentGame }) => { return ( <GameLayout> <div className="w-full"> <GameHeader currentGame={currentGame} /> </div> </GameLayout> ); }; // cockpit.tsx const stateComponents = { PREPARATION: PreparationState, RUNNING: RunningState, // ... other states }; const StateComponent = stateComponents[currentGame?.status] || DefaultState; return <StateComponent currentGame={currentGame} />;
Line range hint
43-47: Optimize data transformations with useMemoThe data transformations for charts are recalculated on every render. Consider using useMemo to optimize performance.
+ const memoizedAssets = useMemo(() => + previousPeriodResults.map((e, ix) => ({ + ...e.facts.assets, + period: 'Period ' + (ix + 1), + })), + [previousPeriodResults] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/demo-game/src/components/DecisionsDisplay.tsx(1 hunks)apps/demo-game/src/pages/play/cockpit.tsx(1 hunks)packages/ui/src/components/NavBar.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/demo-game/src/components/DecisionsDisplay.tsx
- packages/ui/src/components/NavBar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| const tabs = [ | ||
| { | ||
| name: 'Reports', | ||
| href: `/admin/reports/${data?.result?.currentGame?.id}`, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
Add null check for game ID
The href construction doesn't handle the case where data?.result?.currentGame?.id is undefined, which could lead to an invalid URL.
Add a null check or provide a fallback:
const tabs = [
{
name: 'Reports',
- href: `/admin/reports/${data?.result?.currentGame?.id}`,
+ href: data?.result?.currentGame?.id
+ ? `/play/reports/${data?.result?.currentGame?.id}`
+ : '#',
},
]📝 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.
| const tabs = [ | |
| { | |
| name: 'Reports', | |
| href: `/admin/reports/${data?.result?.currentGame?.id}`, | |
| }, | |
| ] | |
| const tabs = [ | |
| { | |
| name: 'Reports', | |
| href: data?.result?.currentGame?.id | |
| ? `/play/reports/${data?.result?.currentGame?.id}` | |
| : '#', | |
| }, | |
| ] |
Incorrect route for player reports
The href points to an admin route (/admin/reports), but according to the PR objectives, this page should be accessible to players. This could lead to access permission issues.
Consider updating the route to a player-specific path:
const tabs = [
{
name: 'Reports',
- href: `/admin/reports/${data?.result?.currentGame?.id}`,
+ href: `/play/reports/${data?.result?.currentGame?.id}`,
},
]📝 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.
| const tabs = [ | |
| { | |
| name: 'Reports', | |
| href: `/admin/reports/${data?.result?.currentGame?.id}`, | |
| }, | |
| ] | |
| const tabs = [ | |
| { | |
| name: 'Reports', | |
| href: `/play/reports/${data?.result?.currentGame?.id}`, | |
| }, | |
| ] |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/demo-game/src/pages/admin/reports/[id].tsx (1)
80-81: Avoid leaving debugging logs in production code
Consider removing or guarding thisconsole.logstatement to keep the console clean in production environments.- console.log('gamedata', data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/demo-game/src/graphql/generated/ops.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
apps/demo-game/src/pages/admin/games/[id].tsx(1 hunks)apps/demo-game/src/pages/admin/reports/[id].tsx(1 hunks)packages/platform/public/ops/QGame.graphql(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
apps/demo-game/src/pages/admin/reports/[id].tsx (1)
75-75: Request to verify the performance implications of turning off facts
By settingincludeFactstofalse, the query fetches less data, which may improve performance. Please confirm that no downstream code relies on these facts.apps/demo-game/src/pages/admin/games/[id].tsx (1)
79-79: Good utilization ofincludeFacts
Fetching additional details withincludeFacts: truealigns with the updated schema. Double-check for any unnecessary overhead if facts are large or rarely used.packages/platform/public/ops/QGame.graphql (2)
4-4: Introducing a required$includeFactsparameter
This change cleanly separates logic for whether to fetch additional facts. Make sure all callers provide a default for$includeFactsto avoid query errors.
21-48: Expanded query fields for deeper insights
The newly included fields enhance the granularity of game and segment data. This looks correct, though please ensure all code paths handle the optionalfactsfields when$includeFactsis false.
Ah I see ^^ I double-checked and we actually give all the data back Let me know if we can proceed with the current solution. |
|
|
||
| const { data, error, loading } = useQuery(GameDocument, { | ||
| variables: { id: Number(router.query.id) }, | ||
| variables: { id: Number(router.query.id), includeFacts: true }, |
There was a problem hiding this comment.
coming from a security perspective, this is not "secure" as the variable can just be changed and the request sent with true to receive all data (even though probably no student ever would do that for a game :D). to make it secure, we would need two requests and authorization such that one request (including facts) is only allowed for the game master, and one request (excluding facts) is accessible to the student. the authorization would work based on the player role.
even if it is not really critical here, I think it would be worth it to do it the right way so that we have 1) a good example for how it should be done and 2) so that you have seen how it works for all of the other game elements where it is more critical to authorize
There was a problem hiding this comment.
ah yes, I see what you mean - good point :D!
I took a look and modified it to the best of my knowledge -> I basically check with the useSession hook who is authenticated and based on that I load the corresponding graphql doc.
Changing the variable (in the new case) isAdmin, should not be possible, right?
Please have a look and let me know if it's aligning with your description
rschlaefli
left a comment
rschlaefli
left a comment
There was a problem hiding this comment.
we should build it using authorization mechanisms in the backend, can discuss this next week
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/demo-game/src/pages/reports/[id].tsx (1)
320-323: Improve loading and error statesThe current implementation shows a generic "loading..." message. Consider adding:
- A proper loading spinner/skeleton
- Specific error message for unauthorized access
if ( loading || segmentEndResultsLoading || periodEndResultsLoading || !memoizedDataPeriod || !memoizedData || skipQuery ) { - return <div>loading...</div> + return ( + <div className="flex items-center justify-center min-h-screen"> + <LoadingSpinner /> + </div> + ) } + if (!isAuthenticated) { + return ( + <div className="flex items-center justify-center min-h-screen"> + <div className="text-red-500"> + You must be logged in to view this page + </div> + </div> + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
apps/demo-game/src/graphql/generated/nexus-typegen.tsis excluded by!**/generated/**apps/demo-game/src/graphql/generated/ops.tsis excluded by!**/generated/**apps/demo-game/src/graphql/generated/schema.graphqlis excluded by!**/generated/**apps/demo-game/src/graphql/generated/schema.jsonis excluded by!**/generated/**
📒 Files selected for processing (5)
apps/demo-game/src/pages/admin/games/[id].tsx(2 hunks)apps/demo-game/src/pages/play/cockpit.tsx(1 hunks)apps/demo-game/src/pages/reports/[id].tsx(4 hunks)packages/platform/public/ops/QGameWithoutFacts.graphql(1 hunks)packages/platform/src/types/Query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/demo-game/src/pages/admin/games/[id].tsx
🔇 Additional comments (5)
apps/demo-game/src/pages/reports/[id].tsx (2)
76-78: LGTM! Authentication implementation is secure.The implementation correctly uses next-auth session to determine user role and access level.
82-89: LGTM! Proper query selection based on user role.The code correctly uses different queries (
GameDocumentvsGameWithoutFactsDocument) based on the user's admin status.packages/platform/public/ops/QGameWithoutFacts.graphql (1)
1-3: LGTM! Proper fragment organizationThe query properly imports and reuses fragments from separate files, following good GraphQL practices.
apps/demo-game/src/pages/play/cockpit.tsx (2)
127-132:⚠️ Potential issueAdd null check for game ID and update route path
The href construction still needs improvement:
- Missing null check for
data?.result?.currentGame?.id- The route should be prefixed with
/playfor consistency with player routesApply this diff to fix both issues:
const tabs = [ { name: 'Reports', - href: `/reports/${data?.result?.currentGame?.id}`, + href: data?.result?.currentGame?.id + ? `/play/reports/${data?.result?.currentGame?.id}` + : '#', }, ]Likely invalid or redundant comment.
127-132: Verify data access controls for player reportsBased on the PR comments about future data access concerns, ensure that:
- The backend properly filters data for player access
- No sensitive or future game data is exposed through the reports page
Run this script to check for any sensitive data exposure in the reports route:
✅ Verification successful
✓ Reports page access is properly secured
The reports route implementation includes appropriate security measures:
- Authentication verification
- Role-based data access control
- Filtered data exposure for players using
GameWithoutFactsDocument🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential sensitive data exposure in reports route # Look for GraphQL queries used in reports page rg -A 10 "query.*Report.*" # Look for data filtering logic in resolvers ast-grep --pattern 'class $_ implements Resolver { $$$ async $_(player: Player, $$$) { $$$ } }'Length of output: 136
Script:
#!/bin/bash # Find report-related files echo "=== Report-related files ===" fd "report" --type f # Look for GraphQL operations more broadly echo -e "\n=== GraphQL operations ===" rg -l "useQuery|useMutation|gql`|gql\(" # Check for authorization mechanisms echo -e "\n=== Authorization checks ===" rg -l "authorize|permission|isAllowed|canAccess|restricted"Length of output: 324
Script:
#!/bin/bash # Look for GraphQL operations in play directory echo "=== GraphQL operations in play directory ===" rg -A 5 "useQuery|useMutation|gql" "apps/demo-game/src/pages/play" # Check route definitions and guards echo -e "\n=== Route definitions and guards ===" rg -A 5 "router|Route|guard|protect" "apps/demo-game/src/pages" # Look for report-related components and types echo -e "\n=== Report components and types ===" rg -A 5 "interface.*Report|type.*Report|class.*Report" "apps/demo-game/src"Length of output: 17473
| t.field('gameWithoutFacts', { | ||
| type: Game, | ||
| args: { | ||
| id: intArg(), | ||
| }, | ||
| async resolve(_, args, ctx) { | ||
| return GameService.getGame(args, ctx) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Security concern: Implement facts filtering in resolver
The gameWithoutFacts field uses the same resolver as game, which means it doesn't actually filter out facts at the data access level. This could potentially expose sensitive data if the GraphQL schema's field masking fails.
Consider implementing a separate resolver that explicitly filters out facts:
t.field('gameWithoutFacts', {
type: Game,
args: {
id: intArg(),
},
async resolve(_, args, ctx) {
- return GameService.getGame(args, ctx)
+ const game = await GameService.getGame(args, ctx)
+ return {
+ ...game,
+ periods: game.periods.map(period => ({
+ ...period,
+ segments: period.segments.map(segment => ({
+ ...segment,
+ facts: undefined
+ }))
+ }))
+ }
},
})Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@rschlaefli that's an interesting suggestion ... Maybe we only need one graphql doc?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| number | ||
| name | ||
|
|
||
| facts |
There was a problem hiding this comment.
Security concern: Remove facts field from player data
The query still includes the facts field in player data, which could potentially expose future game data. This contradicts the PR's goal of preventing access to future data.
Remove this line to ensure no facts are exposed to non-admin users:
- facts2 participants
Note: this should be merged after #77
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes