Add button for downloading course completions #1307
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds short-lived JWT token endpoints for exporting course completions (CSV/Excel), a frontend modal to request and download exports, new admin-or-course-owner authorization helpers and middleware applied across GraphQL resolvers and frontend HOCs, plus supporting dependency, config, and i18n updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant BackendAPI
participant Database
User->>Frontend: Click "Download Completions"
Frontend->>Frontend: Open modal, select date & format
User->>Frontend: Click Download
Frontend->>BackendAPI: GET /completions/:courseId/csv/token (Bearer, optional fromDate, format)
BackendAPI->>BackendAPI: Validate auth (admin or course owner) and course exists
BackendAPI->>BackendAPI: Sign short-lived JWT (courseId, fromDate, format)
BackendAPI-->>Frontend: Return token
Frontend->>BackendAPI: GET /completions/:courseId/csv?token=...
BackendAPI->>BackendAPI: Verify & decode token
BackendAPI->>Database: Query completions (apply fromDate filter)
BackendAPI->>BackendAPI: Generate CSV or Excel
BackendAPI-->>Frontend: Stream file response (attachment)
Frontend->>User: Trigger download, show success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/api/routes/completions.ts`:
- Around line 26-36: Add JWT_SECRET to .env.example and ensure it's set in
deployment configs; keep the existing startup check using JWT_SECRET. When
verifying tokens in the code that uses jwt.verify(), pass the allowed algorithms
(e.g., { algorithms: ['HS256'] }) and include/validate standard claims (iss and
aud) when signing and verifying the token payload (refer to DownloadTokenPayload
and the code paths that call jwt.sign/jwt.verify). Replace the current naive
fromDate handling by parsing and validating the date with date.getTime() /
isNaN(date.getTime()) (refer to the variable fromDate and its parsing),
rejecting invalid dates instead of relying on try/catch. Finally, avoid loading
all completions into memory via stringify(); switch the CSV export to a
streaming pipeline (the same transform/stream approach used elsewhere in this
file) to handle large datasets.
In `@frontend/components/CompletionsDownloadButton.tsx`:
- Around line 97-105: The success alert fires immediately after link.click() so
it may report success before the download completes; replace the current
behavior in CompletionsDownloadButton.tsx by using fetch to request
`/api/completions/${courseId}/csv?token=${token}`, check response.ok, read
response.blob(), create an object URL, set it on a created <a> with download
filename, call click(), revokeObjectURL after, and only then call addAlert({
title: t("downloadCompletionsSuccess"), ... }); on non-OK responses or fetch
errors call addAlert with a failure message; if you prefer a smaller change,
instead change the success message key used in addAlert (where addAlert is
invoked) to indicate the download was "initiated" rather than "completed".
🧹 Nitpick comments (4)
frontend/hooks/useQueryParameter.tsx (1)
29-29: Consider removing redundant optional chaining.Since the guard on line 25 already ensures
routeris defined and ready, the optional chaining onrouter?.query?.[parameter]is now redundant.♻️ Suggested simplification
- const paramValue = router?.query?.[parameter] + const paramValue = router.query[parameter]backend/package.json (1)
75-93: Move@types/jsonwebtokento devDependencies.
@types/* packages are compile-time only and should not be in runtime dependencies for server applications. The jsonwebtoken ^9.0.3 and csv-stringify ^6.6.0 versions are current and free of known security advisories.♻️ Suggested dependency placement
"dependencies": { - "@types/jsonwebtoken": "^9.0.10", "axios": "^1.4.0", "body-parser": "^1.20.2", @@ "devDependencies": { + "@types/jsonwebtoken": "^9.0.10", "@babel/core": "^7.22.9",frontend/components/CompletionsDownloadButton.tsx (2)
33-33: Empty styled component.
StyledDialogContentdoesn't add any styles. Consider usingDialogContentdirectly or add the intended styles.
94-94: Consider validating the token exists in the response.If the API returns an unexpected response structure,
tokencould beundefined, resulting in a URL withtoken=undefined.♻️ Suggested improvement
const { token } = await tokenResponse.json() + if (!token) { + throw new Error("Invalid token response") + }
| // JWT secret for signing download tokens | ||
| const JWT_SECRET = process.env.JWT_SECRET as Secret | ||
|
|
||
| if (!JWT_SECRET) { | ||
| throw new Error("JWT_SECRET environment variable is required") | ||
| } | ||
|
|
||
| interface DownloadTokenPayload { | ||
| courseId: string | ||
| fromDate?: string | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the completions.ts file
find . -name "completions.ts" -type fRepository: rage/mooc.fi
Length of output: 91
🏁 Script executed:
# Get the file size and content around the specified lines
wc -l backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 93
🏁 Script executed:
# Read lines 26-36 to see the JWT secret handling
sed -n '26,36p' backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 311
🏁 Script executed:
# Read lines 113-144 to see what also applies
sed -n '113,144p' backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 859
🏁 Script executed:
# Check for JWT_SECRET usage throughout the codebase
rg "JWT_SECRET" --type ts --type tsx -B2 -A2Repository: rage/mooc.fi
Length of output: 83
🏁 Script executed:
# Look for environment variable configuration files and test setup
fd "\.env" -o "jest.config" -o "setup" -o "test"Repository: rage/mooc.fi
Length of output: 197
🌐 Web query:
jsonwebtoken v9 best practices for expiresIn and required claims iss aud sub short-lived tokens
💡 Result:
Short checklist / best practices for jsonwebtoken (v9) regarding expiresIn and required claims (iss, aud, sub) and short-lived tokens:
- Use short-lived access tokens (typical: 15–30 minutes) and issue refresh tokens for session continuation. Short lifetimes reduce exposure if stolen. (jwtauditor.com)
- Always set expiresIn when signing (jwt.sign(payload, key, { expiresIn: '15m' })) and enforce exp on verify (default). For very sensitive ops use minutes (e.g., 5m). (deepwiki.com)
- Require and validate issuer (iss), audience (aud) and subject (sub) on verification by passing options to verify: { issuer, audience, subject, algorithms }. Reject tokens missing or with unexpected values. (deepwiki.com)
- Use mutually‑exclusive JWT profiles (different aud/iss/typ or claim sets) for different token kinds (access, refresh, id) so tokens can’t be substituted. RFC‑8725 recommends explicit typing or different aud/iss. (ietf.org)
- Prefer asymmetric signatures (RS256/ES256) for distributed systems; explicitly whitelist algorithms when verifying to avoid confusion attacks. Enforce adequate key sizes (RSA ≥2048). (deepwiki.com)
- Add jti for revocation and use a revocation mechanism (blacklist, store jti in DB/Redis or use short lifespan + refresh rotation). (jwtauditor.com)
- Set iat/nbf when appropriate and allow small clock skew (e.g., 30s) in verification logic if needed. (deepwiki.com)
- Minimize sensitive data in payload; keep tokens small and use encrypted tokens (JWE) if payload secrecy is required. (deepwiki.com)
- Key management: rotate keys, store private keys securely, and use KMS when possible. Document and deploy key rotation strategy that supports validating old tokens until they expire or are revoked. (deepwiki.com)
Practical verify call pattern (conceptual):
- jwt.verify(token, pubKeyOrSecret, { algorithms: ['RS256'], issuer: 'https://auth.example', audience: 'api://default', subject: userId, clockTolerance: 30 })
References: jsonwebtoken v9 security recommendations and README; RFC 8725 (JWT best practices); modern JWT implementation guides. (deepwiki.com)
If you want, I can produce a short code snippet showing sign/verify options for your exact use case (access vs refresh).
Citations:
- 1: https://jwtauditor.com/docs/secure-jwt-implementation.html?utm_source=openai
- 2: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 3: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 4: https://www.ietf.org/rfc/rfc8725.html?utm_source=openai
- 5: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 6: https://jwtauditor.com/docs/secure-jwt-implementation.html?utm_source=openai
- 7: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 8: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 9: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
- 10: https://deepwiki.com/auth0/node-jsonwebtoken/5-security-best-practices?utm_source=openai
🏁 Script executed:
# Search for jwt.verify calls to see how tokens are verified
rg "jwt.verify" --max-count=20Repository: rage/mooc.fi
Length of output: 161
🏁 Script executed:
# Search for where fromDate is used (date validation)
rg "fromDate" --max-count=20Repository: rage/mooc.fi
Length of output: 708
🏁 Script executed:
# Read the complete completionsCSVToken endpoint to see CSV streaming
sed -n '113,180p' backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 1837
🏁 Script executed:
# Check for environment variable setup in test and deployment configs
find . -type f \( -name "*.env*" -o -name "*.yml" -o -name "docker*" \) | head -20Repository: rage/mooc.fi
Length of output: 984
🏁 Script executed:
# Look for any token verification endpoint
rg "completionsCSV" -A5 -B5Repository: rage/mooc.fi
Length of output: 2000
🏁 Script executed:
# Check the full date validation and CSV generation logic
sed -n '145,200p' backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 1640
🏁 Script executed:
# Check backend/.env.example for JWT_SECRET documentation
cat backend/.env.example | grep -A2 -B2 "JWT"Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check if there's any validation of the date
rg "new Date" -B3 -A3 backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 315
🏁 Script executed:
# Check the rest of the CSV generation to see if it's streaming
sed -n '190,250p' backend/api/routes/completions.tsRepository: rage/mooc.fi
Length of output: 1660
🏁 Script executed:
# Check package.json to confirm jsonwebtoken version
rg "jsonwebtoken" package.jsonRepository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Look for JWT_SECRET in environment files
rg "JWT_SECRET" -B2 -A2 backend/Repository: rage/mooc.fi
Length of output: 1140
🏁 Script executed:
# Check package.json for jsonwebtoken version
cat package.json | grep -A2 -B2 "jsonwebtoken"Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Look for test setup to see if JWT_SECRET is provisioned
fd "test\|spec\|jest" -type f | head -10Repository: rage/mooc.fi
Length of output: 226
🏁 Script executed:
# Check if there are any other JWT implementations or token types
rg "jwt\." --max-count=30Repository: rage/mooc.fi
Length of output: 331
🏁 Script executed:
# Find package.json files
find . -name "package.json" -type fRepository: rage/mooc.fi
Length of output: 230
🏁 Script executed:
# Check backend package.json for jsonwebtoken version
cat backend/package.json | grep "jsonwebtoken"Repository: rage/mooc.fi
Length of output: 123
🏁 Script executed:
# Check root package.json
cat package.json | grep "jsonwebtoken"Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Look for environment setup in backend tests or docker
find backend -name "*.test.ts" -o -name "*.spec.ts" | head -5Repository: rage/mooc.fi
Length of output: 304
🏁 Script executed:
# Check docker-compose for JWT_SECRET
cat backend/docker-compose.yml | grep -A5 -B5 "JWT_SECRET"Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check completions test file
cat backend/api/routes/__test__/completions.test.ts | head -100Repository: rage/mooc.fi
Length of output: 3283
🏁 Script executed:
# Look for JWT_SECRET setup in test files
rg "JWT_SECRET" backend/api/routes/__test__/Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check for .env.test or similar
find backend -name ".env*" -o -name "setup*"Repository: rage/mooc.fi
Length of output: 76
🏁 Script executed:
# Check what's in backend/.env.example
cat backend/.env.exampleRepository: rage/mooc.fi
Length of output: 708
🏁 Script executed:
# Search for where completionsCSVToken tests are
rg "completionsCSVToken\|completionsCSV" backend/api/routes/__test__/completions.test.tsRepository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Search the entire test file for CSV-related tests
rg "CSV\|csv" backend/api/routes/__test__/completions.test.ts -A10Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check if there's a global test setup file that sets JWT_SECRET
find backend -name "setup*" -o -name "config*" | grep -i testRepository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Look for where getTestContext sets up environment
rg "getTestContext" backend/tests/ -A20 | head -50Repository: rage/mooc.fi
Length of output: 1012
🏁 Script executed:
# Check if JWT_SECRET is actually set in test environment
rg "process.env.JWT_SECRET\|JWT_SECRET =" backend/tests/ -B5 -A5Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Look for where process.env variables are mocked in tests
rg "process.env\." backend/tests/index.ts | head -20Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check if there's a before hook that sets environment variables
cat backend/tests/index.ts | head -80Repository: rage/mooc.fi
Length of output: 1772
🏁 Script executed:
# Check if JWT_SECRET is set anywhere in the actual code startup
rg "JWT_SECRET" backend/ --type ts | grep -v test | grep -v node_modulesRepository: rage/mooc.fi
Length of output: 483
🏁 Script executed:
# Look at the server startup to see if JWT_SECRET is required
rg "process.env" backend/server.ts -A2 -B2 | head -40Repository: rage/mooc.fi
Length of output: 38
🏁 Script executed:
# Check if there's a date parsing issue - new Date() doesn't throw on invalid dates
node -e "console.log(new Date('invalid')); console.log(new Date('invalid') instanceof Date)"Repository: rage/mooc.fi
Length of output: 73
🏁 Script executed:
# Look for stringify import to see CSV approach
rg "stringify" backend/api/routes/completions.ts -B5 -A1Repository: rage/mooc.fi
Length of output: 519
Ensure JWT_SECRET is documented in .env.example and set in all environments.
The startup check is good, but JWT_SECRET is missing from .env.example, making it easy to miss during deployment setup.
Additionally, the JWT token implementation needs security improvements:
- Pass
{ algorithms: ['HS256'] }tojwt.verify()to prevent algorithm confusion attacks - Include standard claims (
iss,aud) in the signed payload and validate them on verify - Date validation is broken:
new Date(fromDate)silently returns anInvalid Dateobject instead of throwing, bypassing the try-catch. Validate withisNaN(date.getTime())before querying. - CSV generation loads all completions into memory via
stringify(). Consider streaming with a transform pipeline (as used elsewhere in this file) for large datasets.
🤖 Prompt for AI Agents
In `@backend/api/routes/completions.ts` around lines 26 - 36, Add JWT_SECRET to
.env.example and ensure it's set in deployment configs; keep the existing
startup check using JWT_SECRET. When verifying tokens in the code that uses
jwt.verify(), pass the allowed algorithms (e.g., { algorithms: ['HS256'] }) and
include/validate standard claims (iss and aud) when signing and verifying the
token payload (refer to DownloadTokenPayload and the code paths that call
jwt.sign/jwt.verify). Replace the current naive fromDate handling by parsing and
validating the date with date.getTime() / isNaN(date.getTime()) (refer to the
variable fromDate and its parsing), rejecting invalid dates instead of relying
on try/catch. Finally, avoid loading all completions into memory via
stringify(); switch the CSV export to a streaming pipeline (the same
transform/stream approach used elsewhere in this file) to handle large datasets.
| const link = document.createElement("a") | ||
| link.href = `/api/completions/${courseId}/csv?token=${token}` | ||
| link.click() | ||
|
|
||
| addAlert({ | ||
| title: t("downloadCompletionsSuccess"), | ||
| message: t("downloadCompletionsSuccessMessage"), | ||
| severity: "success", | ||
| }) |
There was a problem hiding this comment.
Success alert may appear before download completes.
link.click() triggers the download but doesn't wait for it to finish. The success alert appears immediately, even if the download fails or is cancelled by the user.
Consider either:
- Changing the message to indicate the download was "initiated" rather than "completed"
- Using
fetchwith blob handling to have full control over the download lifecycle
🤖 Prompt for AI Agents
In `@frontend/components/CompletionsDownloadButton.tsx` around lines 97 - 105, The
success alert fires immediately after link.click() so it may report success
before the download completes; replace the current behavior in
CompletionsDownloadButton.tsx by using fetch to request
`/api/completions/${courseId}/csv?token=${token}`, check response.ok, read
response.blob(), create an object URL, set it on a created <a> with download
filename, call click(), revokeObjectURL after, and only then call addAlert({
title: t("downloadCompletionsSuccess"), ... }); on non-OK responses or fetch
errors call addAlert with a failure message; if you prefer a smaller change,
instead change the success message key used in addAlert (where addAlert is
invoked) to indicate the download was "initiated" rather than "completed".
also fix context for navigation menu buttons
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/lib/authentication.ts`:
- Around line 98-107: The function getAccessToken currently declares a return
type of string but can return undefined (document.cookie may not contain
access_token and nookies.get(ctx)["access_token"] can be undefined); update the
function signature return type to string | undefined OR add a deterministic
fallback (e.g., return "" or throw) to ensure the declared return type matches
actual behavior; locate the getAccessToken function and modify its signature
and/or return path accordingly (referencing getAccessToken and the
nookies.get(ctx) usage).
In `@frontend/lib/with-admin-or-course-owner.tsx`:
- Line 37: router.query.slug is typed as string | string[] | undefined so the
current cast in the slug constant is unsafe; update the normalization in
with-admin-or-course-owner.tsx by replacing the blind cast of router.query.slug
with logic that handles string[], undefined, and string (e.g., check
Array.isArray(router.query.slug) to pick the first element or join, and default
to empty string) so the const slug is always a plain string before use in
functions like any permission checks or API calls.
♻️ Duplicate comments (3)
backend/api/routes/completions.ts (3)
223-243: Consider streaming for large datasets.The current implementation loads all completions into memory before generating CSV/Excel output. For courses with many completions, this could cause memory pressure. The file already uses streaming elsewhere (line 110-111 with JSONStream). Consider a streaming approach for CSV, though Excel generation inherently requires buffering.
214-221: Date validation is broken -new Date()does not throw on invalid input.
new Date("invalid")returns anInvalid Dateobject rather than throwing an exception, so the try-catch block won't catch malformed dates. The invalid date will be passed to the query, potentially causing unexpected behavior.🐛 Proposed fix
if (fromDate && typeof fromDate === "string") { - try { - const date = new Date(fromDate) - query = query.where("com.completion_date", ">=", date) - } catch (e) { - return res.status(400).json({ message: "Invalid date format" }) - } + const date = new Date(fromDate) + if (isNaN(date.getTime())) { + return res.status(400).json({ message: "Invalid date format" }) + } + query = query.where("com.completion_date", ">=", date) }
167-175: Specify allowed algorithms in jwt.verify to prevent algorithm confusion attacks.Without explicitly specifying the allowed algorithms, the verification is vulnerable to algorithm confusion attacks where an attacker could potentially use a different algorithm than intended.
🔒 Proposed fix
let tokenData: DownloadTokenPayload try { - tokenData = jwt.verify(token, JWT_SECRET) as DownloadTokenPayload + tokenData = jwt.verify(token, JWT_SECRET, { + algorithms: ["HS256"], + issuer: "mooc.fi", + audience: "completions-download", + }) as DownloadTokenPayload } catch (error) {
🧹 Nitpick comments (3)
frontend/lib/with-admin-or-course-owner.tsx (2)
70-72: Consider distinguishing query errors from authorization errors.Showing
AdminErrorfor GraphQL query errors (e.g., network failures) may confuse users since it implies an authorization issue. Consider rendering a more generic error message or retry option for non-auth errors.
49-54: Missing course may not be handled gracefully.If
data?.courseisnull(course doesn't exist),isOwnerwill befalse, and non-admin users will seeAdminError. Consider checking for course existence explicitly and showing a "course not found" message instead.Suggested approach
+ if (!ownershipLoading && data && !data.course) { + return <div>Course not found</div> + } + if (!admin && !isOwner) { return <AdminError /> }Also applies to: 74-76
backend/api/routes/completions.ts (1)
147-149: Add standard JWT claims and specify algorithm.The token should include
iss(issuer) andaud(audience) claims for better security posture. This helps prevent token substitution attacks and aligns with RFC 8725 JWT best practices.♻️ Suggested improvement
const payload: DownloadTokenPayload = { courseId, fromDate: typeof fromDate === "string" ? fromDate : undefined, format: format === "excel" ? "excel" : "csv", } const token = jwt.sign(payload, JWT_SECRET, { expiresIn: "30s", + issuer: "mooc.fi", + audience: "completions-download", })
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/schema/Completion/mutations.ts`:
- Around line 20-29: The authorizeByCourseIdentifier function has its parameters
in the wrong order for Nexus (currently declared as (args, ctx, root, info));
change the signature to (root, args, ctx, info) and update any references inside
the function so that code that expects ctx.prisma uses the new ctx parameter and
code that reads course_id/course_slug/slug uses the new args parameter; ensure
the function continues to return the same authorization result after the
parameter reorder.
In `@frontend/lib/with-admin-or-course-owner.tsx`:
- Around line 57-69: Add a router.isReady guard to avoid the spinner hang:
update the useEffect and the render gating to wait for router.isReady before
using router.query.slug and before deciding loading/ownershipLoading/slug state.
Specifically, in the existing useEffect (the one that redirects to
`${baseUrl}/sign-in`), include router.isReady in the dependency list and only
run the redirect when router.isReady is true; likewise, include router.isReady
in the condition that renders <Spinner /> (i.e., check loading ||
ownershipLoading || !router.isReady || !slug) so the component doesn’t block on
an uninitialized router. Ensure references are to the same symbols used in the
diff: useEffect, router, router.isReady, slug, loading, ownershipLoading,
Spinner, and router.push.
🧹 Nitpick comments (1)
frontend/lib/with-admin-or-course-owner.tsx (1)
35-85: Unify admin source of truth to avoid drift.You already fetch
currentUser.administratorbut rely on the clientadminflag. If those ever drift (e.g., stale client state), admins could be incorrectly blocked or granted. Consider preferring the server value when available.♻️ Suggested adjustment
- const { signedIn, loading, admin } = useAuth() + const { signedIn, loading, admin } = useAuth() const baseUrl = router.pathname.includes("_old") ? "/_old" : "" const rawSlug = router.query.slug const slug = Array.isArray(rawSlug) ? rawSlug[0] ?? "" : rawSlug ?? "" @@ - const isOwner = Boolean( + const effectiveAdmin = data?.currentUser?.administrator ?? admin + const isOwner = Boolean( data?.course?.id && data?.currentUser?.course_ownerships?.some( (o: { course_id: string | null }) => o.course_id === data.course?.id, ), ) @@ - if (!admin && !isOwner) { + if (!effectiveAdmin && !isOwner) { return <AdminError /> } @@ - admin={admin} + admin={effectiveAdmin}
| useEffect(() => { | ||
| if (!loading && !signedIn) { | ||
| router.push(`${baseUrl}/sign-in`) | ||
| } | ||
| }, [loading, signedIn, router, baseUrl]) | ||
|
|
||
| if (loading || ownershipLoading || !slug) { | ||
| return <Spinner /> | ||
| } | ||
|
|
||
| if (!signedIn) { | ||
| return <div>Redirecting...</div> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js router.isReady router.query dynamic routes best practice pattern 2024 2025
💡 Result:
Best-practice patterns (2024–2025) for dynamic routes + router.query / router.isReady
1) Pages Router (pages/): treat router.query as empty on first render unless you SSR
In the Pages Router, router.query “will be an empty object during prerendering” for non-SSR pages, and router.isReady indicates when router fields are ready client-side. Use router.isReady inside useEffect before reading router.query / router.asPath to avoid hydration mismatches and “missing param” bugs. [1]
Recommended pattern (Pages Router):
import { useEffect, useMemo } from "react";
import { useRouter } from "next/router";
export default function PostPage() {
const router = useRouter();
const slug = useMemo(() => {
if (!router.isReady) return null;
const v = router.query.slug;
return Array.isArray(v) ? v[0] : v ?? null;
}, [router.isReady, router.query.slug]);
useEffect(() => {
if (!router.isReady) return;
// safe to use router.query / router.asPath here
}, [router.isReady, router.query]);
if (!router.isReady) return null; // or a skeleton
return <div>slug: {slug}</div>;
}Also avoid using router.asPath until router.isReady === true, because it can cause client/server mismatch. [1]
2) Prefer data-fetching context params when possible (Pages Router)
If you’re using getServerSideProps / getStaticProps, prefer the context.params / context.query values for initial render (they exist on the server), instead of relying on client router.query readiness. (This avoids the need for isReady gating for initial data.)
3) App Router (app/): prefer useParams / searchParams over router.query
In the App Router, dynamic segments should be read via useParams() in Client Components. [2] Query string values should be read via useSearchParams() (Client Component) or via the Page’s searchParams prop (Server Component), depending on your rendering needs. [3]
Recommended pattern (App Router):
"use client";
import { useParams } from "next/navigation";
export default function Page() {
const { slug } = useParams<{ slug: string }>();
return <div>slug: {slug}</div>;
}4) Sharing components between app/ and pages/
If you need a component that works in both routers, Next.js explicitly documents using useSearchParams() with a null fallback during Pages pre-rendering. [4]
Sources: Next.js useRouter (router.query, router.isReady, asPath mismatch warning) [1]; Next.js useParams (dynamic params; Pages Router returns null initially until ready) [2]; Next.js useSearchParams (static rendering behavior + recommended Suspense) [3]; Next.js Pages useSearchParams (cross-router sharing + null fallback) [4].
🏁 Script executed:
# First, locate and examine the file to understand the slug source and route context
fd "with-admin-or-course-owner.tsx" -type fRepository: rage/mooc.fi
Length of output: 226
🏁 Script executed:
# Find the file with correct syntax
fd "with-admin-or-course-owner" --type fRepository: rage/mooc.fi
Length of output: 99
🏁 Script executed:
# Also search for any files containing this component
rg "with-admin-or-course-owner" -l --type tsxRepository: rage/mooc.fi
Length of output: 83
🏁 Script executed:
# Read the file to understand the slug source and context
cat -n frontend/lib/with-admin-or-course-owner.tsxRepository: rage/mooc.fi
Length of output: 2933
Add router.isReady guard to prevent endless spinner on uninitialized router.
The code directly accesses router.query.slug (line 37) without checking router.isReady first. In Next.js Pages Router, router.query is empty on initial render, so slug will be an empty string. The spinner condition at line 63 will then block rendering indefinitely if the router never populates the query param.
Suggested adjustment
- if (loading || ownershipLoading || !slug) {
+ if (loading || ownershipLoading || !router.isReady) {
return <Spinner />
}
+ if (!slug) {
+ return <AdminError />
+ }
+
if (!signedIn) {
return <div>Redirecting...</div>
}🤖 Prompt for AI Agents
In `@frontend/lib/with-admin-or-course-owner.tsx` around lines 57 - 69, Add a
router.isReady guard to avoid the spinner hang: update the useEffect and the
render gating to wait for router.isReady before using router.query.slug and
before deciding loading/ownershipLoading/slug state. Specifically, in the
existing useEffect (the one that redirects to `${baseUrl}/sign-in`), include
router.isReady in the dependency list and only run the redirect when
router.isReady is true; likewise, include router.isReady in the condition that
renders <Spinner /> (i.e., check loading || ownershipLoading || !router.isReady
|| !slug) so the component doesn’t block on an uninitialized router. Ensure
references are to the same symbols used in the diff: useEffect, router,
router.isReady, slug, loading, ownershipLoading, Spinner, and router.push.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1307 +/- ##
==========================================
- Coverage 66.31% 65.72% -0.60%
==========================================
Files 154 154
Lines 6460 6587 +127
Branches 1591 1626 +35
==========================================
+ Hits 4284 4329 +45
- Misses 2037 2108 +71
- Partials 139 150 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/schema/Completion/mutations.ts`:
- Around line 33-59: The GraphQLUserInputError currently reports "slug" even
when the resolver argument is courseSlug; update the error argument names to
match the actual input names (courseId and courseSlug) by choosing the field
name based on which identifier is present—e.g., when checking for
missing/duplicate identifiers or when throwing "course not found", use
"courseSlug" (or "course_slug" if that’s the external arg name), and when using
courseId use "courseId"; adjust the GraphQLUserInputError calls in the
validation block around courseId/courseSlug and the not-found check that follows
the ctx.prisma.course.findUniqueOrAlias call so the second parameter lists the
exact input variable name used by callers.
♻️ Duplicate comments (1)
backend/schema/Completion/mutations.ts (1)
20-29: Fix Nexus authorize signature order (root, args, ctx, info).Nexus calls authorize as
(root, args, ctx, info). The current order will shiftctxintoroot, makingctx.prismaundefined and breaking the lookup.🐛 Proposed fix
async function authorizeByCourseIdentifier( - args: { + root: any, + args: { course_id?: string | null course_slug?: string | null slug?: string | null }, ctx: any, - root: any, info: any, ) {Nexus authorize function signature order (root, args, ctx, info) documentation
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/schema/Completion/mutations.ts (1)
103-111: Validate that exactly one course identifier is provided.
With the new helper, sending both identifiers can force a strict match and reject legitimate requests if one is stale. Align withrecheckCompletionsby enforcing mutual exclusivity.🛠️ Proposed fix
validate: (_, { course_id, course_slug }) => { if (!course_id && !course_slug) { throw new GraphQLUserInputError( "must provide course_id or course_slug", ["course_id", "course_slug"], ) } + if (course_id && course_slug) { + throw new GraphQLUserInputError( + "must provide exactly one of course_id or course_slug", + ["course_id", "course_slug"], + ) + } },
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.