feat(mobile): add Jest tests and CI pipeline#44
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
andrewx-bu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
mobile/app/(onboarding)/onboarding.tsx (1)
71-115: Add user-facing feedback when staff role request fails (still outstanding from prior review)The overall
handleGetStartedflow is well-structured: you gate onagreedToTerms, persist the user doc, short-circuit admins to/(admin), route staff to a pending screen, and send students to/(student). TheisSubmittingflag also prevents double-submits.However, in the staff branch, if
requestStaffRole(getToken)throws, the user only sees the button re-enabled and a console log:} catch (err) { console.error('Failed to request staff role:', err); setIsSubmitting(false); }There is still no in-app error state or alert, so the failure is invisible to non-technical users and can feel like a dead end on flaky networks. This is the same concern raised in the previous review and remains unresolved here.
You could minimally surface this via local error state or a native alert. For example:
-import React, { useState, useRef } from 'react'; +import React, { useState, useRef } from 'react'; +import { Alert, /* ... */ } from 'react-native'; ... if (selectedRole === 'staff') { try { await requestStaffRole(getToken); router.replace('/(onboarding)/pending'); } catch (err) { console.error('Failed to request staff role:', err); setIsSubmitting(false); + Alert.alert( + 'Request failed', + 'We could not submit your staff access request. Please check your connection and try again.' + ); } return; }Or, if you prefer not to use alerts, store a short error string in state and render it inline under the button.
mobile/src/components/SettingsScreen.tsx (1)
480-508: Confirm whether staff should also see the Student FAQs sectionThe FAQ block is still gated on
role === 'student', so'staff'users won’t see it. A previous review on this file raised the same question; if staff should be treated like students for FAQs, this conditional could be broadened:{(role === 'student' || role === 'staff') && ( ... )}If the intent is that staff use different docs/channels, this is fine as-is—just confirming the product decision.
🧹 Nitpick comments (7)
mobile/src/components/PushTokenRegistrar.tsx (2)
10-10: Good refactor - extractingprimaryEmailimproves clarity.The extraction of
primaryEmailmakes the code more readable and the dependency on the email address explicit.Minor: The
?? undefinedis redundant since optional chaining already returnsundefinedfor nullish values:- const primaryEmail = user?.primaryEmailAddress?.emailAddress ?? undefined; + const primaryEmail = user?.primaryEmailAddress?.emailAddress;
14-15: Consider logging errors for debugging.The dependency array correctly includes
primaryEmail, ensuring the effect re-runs when the email changes. However, the empty catch block silently swallows errors, which can make debugging difficult.Consider logging errors to aid debugging:
- registerForPushNotificationsAsync(user.id, primaryEmail).catch(() => {}); + registerForPushNotificationsAsync(user.id, primaryEmail).catch((error) => { + console.warn('Push notification registration failed:', error); + });mobile/app/(onboarding)/onboarding.tsx (2)
45-60: Avoid scattering the hard-coded last page indexThe last page index (
3) is duplicated across navigation (handleNext,handleSkip), validation (canProceedFromPage), conditional Skip visibility, dots, and the footer button. This is easy to forget if you add/remove pages later.Consider introducing a
const LAST_PAGE = 3;(or deriving from an array of pages) and reusing it in these locations to keep the flow consistent if the step count changes.Also applies to: 118-121, 125-137, 314-320, 325-349
355-582: Style factory cleanly leverages theme tokens
createStylespulls colors, spacing, typography, and borderRadius from your theme, and conditionally usescolors.surfaceElevated ?? colors.surfacefor selected cards. This keeps onboarding visually consistent with the rest of the app and centralizes design tokens nicely. Typingcolorsasanyis pragmatic here; if you later introduce a theme type, this function will be a good candidate to tighten up.mobile/app/(shared)/settings/edit-name.tsx (1)
25-48: Async name loading pattern looks good; loading state could be simplifiedThe Firestore load with
useCallback/useEffectand error handling is solid and safe against missinguser. Right nowisLoadingis write-only (onlysetIsLoading(false)is used and no UI branches on it), so you could either wire it to a spinner/skeleton or remove it to shave a state/update:- const [, setIsLoading] = useState(true); + // If no loading indicator is needed, drop the unused loading state + // const [, setIsLoading] = useState(true); ... - } finally { - setIsLoading(false); - } + } finally { + // no-op if loading state is removed + }Same pattern exists in
edit-location.tsxandSettingsScreen.tsxif you want to tidy them together.mobile/src/components/SettingsScreen.tsx (2)
38-67: User preference loading is solid; consider consolidating or dropping unused loading stateThe Firestore-backed
loadUserPreferenceshook is clean and correctly guarded onuserpresence. As with the edit screens,isLoadinghere is write-only (onlysetIsLoading(false)is used, nothing reads it), so it currently only introduces an extra state + re-render.If you don’t plan to show a loading indicator on this screen, you can safely remove that state and the
setIsLoadingcall; alternatively, you could reuse it to gate the initial render if you want parity with other loading UX.
109-132: Guard approve/revoke actions against rapid repeat tapsRight now,
handleApprove/handleRevokedon’t track per-row in-flight state and thePressables stay enabled, so users can double-tap and send multiple requests for the sameuserId. If the backend operations aren’t strictly idempotent, this could create noisy logs or edge-case errors.A lightweight improvement would be to track a
mutatingUserIdor booleanadminActionLoadingand disable the action buttons (and/or show a small spinner) while an approve/revoke is in progress:- const handleApprove = async (uid: string) => { + const [mutatingUserId, setMutatingUserId] = useState<string | null>(null); + + const handleApprove = async (uid: string) => { try { + setMutatingUserId(uid); await approveStaff(uid, getToken); Alert.alert('Approved', 'Staff access has been approved.'); await loadAdminLists(); } catch (err: any) { console.error('Failed to approve staff:', err); Alert.alert('Error', err?.message ?? 'Failed to approve staff'); + } finally { + setMutatingUserId(null); } };and then:
- <Pressable + <Pressable + disabled={mutatingUserId === u.userId}Same idea for
handleRevoke. Not required to ship but can harden the UX and backend interaction.Also applies to: 269-323, 385-423
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
mobile/app/(admin)/create/page.tsx(3 hunks)mobile/app/(admin)/index.tsx(8 hunks)mobile/app/(auth)/sign-in.tsx(5 hunks)mobile/app/(auth)/sign-up.tsx(8 hunks)mobile/app/(onboarding)/onboarding.tsx(8 hunks)mobile/app/(shared)/settings/edit-location.tsx(8 hunks)mobile/app/(shared)/settings/edit-name.tsx(8 hunks)mobile/src/components/EventCard.tsx(10 hunks)mobile/src/components/EventEditorModal.tsx(30 hunks)mobile/src/components/PushTokenRegistrar.tsx(1 hunks)mobile/src/components/SettingsScreen.tsx(6 hunks)mobile/src/hooks/useEvents.ts(3 hunks)mobile/src/lib/constants.ts(1 hunks)mobile/src/lib/time.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- mobile/app/(auth)/sign-up.tsx
- mobile/app/(admin)/index.tsx
- mobile/src/lib/time.ts
- mobile/src/components/EventCard.tsx
- mobile/app/(auth)/sign-in.tsx
- mobile/app/(admin)/create/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
🧬 Code graph analysis (7)
mobile/src/components/PushTokenRegistrar.tsx (1)
mobile/src/lib/notifications.ts (1)
registerForPushNotificationsAsync(43-75)
mobile/src/components/SettingsScreen.tsx (3)
mobile/src/lib/rbacClient.tsx (5)
RbacUser(18-23)fetchPendingStaff(105-109)fetchActiveStaff(115-117)approveStaff(123-133)revokeStaff(140-150)mobile/src/lib/firebase/config.ts (1)
firestore(41-43)mobile/src/components/NotificationsToggle.tsx (1)
NotificationsToggle(14-68)
mobile/app/(shared)/settings/edit-location.tsx (2)
mobile/src/lib/firebase/config.ts (1)
firestore(41-43)mobile/src/lib/theme.ts (1)
colors(58-58)
mobile/app/(onboarding)/onboarding.tsx (5)
mobile/src/lib/ThemeProvider.tsx (1)
useTheme(73-79)mobile/src/lib/firebase/config.ts (1)
firestore(41-43)mobile/src/lib/rbacClient.tsx (1)
requestStaffRole(93-99)mobile/src/content/terms.ts (1)
TERMS_AND_CONDITIONS(2-16)mobile/src/lib/theme.ts (3)
spacing(81-88)typography(68-78)borderRadius(91-97)
mobile/src/hooks/useEvents.ts (1)
mobile/src/lib/firebase/events.ts (2)
fetchOpenEventsPage(144-169)fetchEventsPage(110-135)
mobile/app/(shared)/settings/edit-name.tsx (2)
mobile/src/lib/firebase/config.ts (1)
firestore(41-43)mobile/src/lib/theme.ts (1)
colors(58-58)
mobile/src/components/EventEditorModal.tsx (3)
mobile/src/lib/constants.ts (1)
PRESET_LOCATIONS(4-54)mobile/src/lib/theme.ts (4)
spacing(81-88)typography(68-78)borderRadius(91-97)status(60-65)mobile/src/types/index.ts (1)
EventStatus(5-5)
🔇 Additional comments (11)
mobile/src/lib/constants.ts (1)
4-9: LGTM – cleaner array type syntax.The change from
Array<...>to{...}[]is a common TypeScript convention that improves readability for simple array types. No functional impact.mobile/src/hooks/useEvents.ts (1)
19-21: LGTM! Formatting improvements enhance readability.The multi-line object literals and added whitespace improve code readability. Note that the empty
catchblocks (lines 32, 65, 98) are a slight code change rather than pure formatting—this pattern is acceptable in modern TypeScript when the error object isn't needed, as all three handlers set generic error messages regardless of the specific error.Also applies to: 32-32, 58-61, 65-65, 86-89, 98-98
mobile/src/components/EventEditorModal.tsx (1)
46-52: LGTM! Well-executed formatting refactor with performance optimization.The multiline destructured props improve readability, and moving styles into a memoized
StyleSheet.createblock is a solid performance optimization that prevents unnecessary style object recreation on each render.Also applies to: 395-772
mobile/app/(onboarding)/onboarding.tsx (3)
34-37: Role selection state wiring looks correct and UX is clear
selectedRolestate is initialized sensibly to'student', and the role cards correctly derive selected styling and behavior from that state. Copy on the cards and the note about staff review makes the RBAC expectations explicit to users, which is good for clarity.Also applies to: 160-230
63-69: Campus preference toggling and persistence look solidThe
toggleLocationhelper implements idempotent add/remove semantics, and the campus chips reflectselectedLocationscorrectly with selected styling. PersistinglocPref: selectedLocationsinto the Firestore user doc inhandleGetStartedties the UI nicely to backend state.Also applies to: 232-267
270-281: Terms & Conditions step and gating logic are consistentThe terms page renders the content in a scrollable area, and the checkbox correctly controls
agreedToTerms. The final “Get Started” button is disabled unlessagreedToTermsis true, and you persistagreedToTerms: trueto Firestore, which keeps UI and backend in sync. TheisSubmitting-based loading label is also a nice touch.Also applies to: 287-297, 340-347
mobile/app/(shared)/settings/edit-name.tsx (1)
50-83: Role-aware routing on save/back is consistent with admin/staff flowsUsing
publicMetadata.roleto send both'admin'and'staff'back to the admin settings page and everyone else to the student page matches the broader RBAC patterns inSettingsScreenandedit-location. Error handling on save looks appropriate for this screen.mobile/src/components/SettingsScreen.tsx (2)
73-132: Admin staff list loading/refresh flow is well-structuredThe
loadAdminListsimplementation withPromise.all, per-list error handling, and ahasLoadedAdminListsref to ensure a single fire-and-forget load per mount is a nice pattern. ReusingloadAdminListsafter approve/revoke to refresh both pending and active lists keeps the state consistent without duplicating fetch logic.
210-427: Admin-only staff UI and supporting styles look cohesiveThe “Staff access requests” / “Existing staff” sections, combined with the new
centerRow,pendingRow,pendingActions, andpillButton*styles, result in a clear, consistent admin experience. Error and empty states are handled explicitly, and the layout aligns with the rest of the settings UI.Also applies to: 581-606
mobile/app/(shared)/settings/edit-location.tsx (2)
28-48: Location loading and toggle logic look correct
loadCurrentLocationscorrectly pullslocPreffrom the Firestore user doc and defends against missinguser, andtoggleLocation’s use of functional state +includes/filteris a clean, idempotent way to manage the selection chips.Also applies to: 50-56
58-88: Role-based routing after save/back aligns with other settings screensUsing
publicMetadata.roleto send admins and staff back through the admin settings page and everyone else through the student page keeps navigation consistent withEditNameScreenandSettingsScreen. Error handling andisSavingusage are reasonable for this flow.Also applies to: 92-103
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/mobile-lint.yml (1)
3-6: Consider adding path filters to reduce unnecessary workflow runs.The workflow triggers on all push and pull_request events without filtering for mobile-specific changes. Node.js 24 is the "Current" release and suitable for CI pipelines.
For a monorepo structure, you may want to add path filters to ensure the lint job only runs when mobile files change.
Apply this diff to add path filtering:
on: push: + paths: + - 'mobile/**' + - '.github/workflows/mobile-lint.yml' pull_request: + paths: + - 'mobile/**' + - '.github/workflows/mobile-lint.yml'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/mobile-lint.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
🔇 Additional comments (3)
.github/workflows/mobile-lint.yml (3)
7-14: Job configuration is well-structured.Using
defaults.run.working-directoryis the correct approach and avoids repetition across steps. This follows GitHub Actions best practices.
19-24: Node.js 24 is now LTS-eligible but check downstream compatibility.Node.js 24 will become LTS (Long-Term Support) in October 2025, and the version selection is appropriate. However, verify that:
- The mobile project's dependencies are compatible with Node.js 24 and npm 11.
- There are no deprecation warnings or breaking changes when running the lint command with this Node version.
Consider pinning to a specific minor version (e.g.,
node-version: '24.11') once the Node.js 24.x LTS line stabilizes, rather than using a major version that tracks pre-LTS updates.
26-30: Workflow appears incomplete: Jest tests mentioned in PR title are missing.The PR title references "add Jest tests and GitHub Actions CI pipeline," but this workflow file only runs linting. Jest test execution is not present. Clarify whether:
- Jest tests are run in a separate workflow file (not provided for review).
- Jest test steps should be added to this workflow.
- Jest tests are planned but not yet implemented.
If tests should be included, add a step to run Jest before or after linting.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/jest.config.js (1)
1-22: LGTM! Solid Jest configuration for backend testing.The configuration is well-structured with appropriate settings for TypeScript testing, coverage collection, and a 30-second timeout suitable for integration tests that interact with Clerk APIs.
Consider these optional enhancements for better test isolation:
module.exports = { preset: 'ts-jest', testEnvironment: 'node', + clearMocks: true, + resetMocks: true, testMatch: ['**/__tests__/**/*.test.ts'], collectCoverageFrom: [ 'api/**/*.ts', 'src/**/*.ts', '!**/*.d.ts', ], coverageDirectory: 'coverage', coverageReporters: ['text', 'lcov', 'html'], testTimeout: 30000, verbose: true, transform: { '^.+\\.tsx?$': ['ts-jest', { tsconfig: { esModuleInterop: true, allowSyntheticDefaultImports: true, } }] } };backend/__tests__/rbac.test.ts (1)
58-67: Consider replacing hardcoded delays with polling for Clerk propagation.The fixed 1000ms delay (line 66) assumes Clerk's eventual consistency window, but this is brittle. If Clerk is slow, tests fail; if it's fast, you're waiting unnecessarily.
Consider adding a polling helper:
async function waitForClerkPropagation( checkFn: () => Promise<boolean>, maxWaitMs: number = 5000, intervalMs: number = 200 ): Promise<void> { const startTime = Date.now(); while (Date.now() - startTime < maxWaitMs) { if (await checkFn()) return; await new Promise(resolve => setTimeout(resolve, intervalMs)); } throw new Error('Clerk propagation timeout'); }Then use it in tests where you check for specific state changes instead of blind delays.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/__tests__/rbac.test.ts(1 hunks)backend/jest.config.js(1 hunks)backend/package.json(1 hunks)backend/test_rbac.sh(0 hunks)
💤 Files with no reviewable changes (1)
- backend/test_rbac.sh
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
🔇 Additional comments (6)
backend/package.json (2)
7-11: LGTM! Standard Jest test scripts.The test scripts follow best practices and provide good developer experience with watch mode and coverage reporting.
17-26: Update test dependency versions; Jest 30 upgrade requires careful testing.No security vulnerabilities were detected in the current versions. However, the dependencies are outdated (from Nov 2023–early 2024):
- Jest 30 drops support for Node 14, 16, 19, and 21 and changes the --testPathPattern CLI flag to --testPathPatterns
- jest-environment-jsdom was upgraded from jsdom 21 to 26, which has become more spec compliant and might break some use cases, most notably mocking window.location in tests
- jest.mock() is case-sensitive now
Consider updating to the latest versions (jest 30.2.0, @jest/globals 30.2.0, @types/jest 30.0.0, ts-jest 29.4.5), but run the full test suite and review the migration guide before deploying, as these are significant changes requiring manual verification.
backend/__tests__/rbac.test.ts (4)
78-123: LGTM! Health check and authentication tests are well-structured.The tests properly validate both unauthenticated access (401) and authenticated RBAC information for different user roles.
126-182: LGTM! Role request and pending list tests provide good coverage.The tests properly validate the complete flow: requesting a role, handling conflicts, and admin-only access to pending users.
185-227: Approval flow tests are comprehensive.Good coverage of access control, successful approval, and prevention of duplicate approvals. Note that line 208 has the same hardcoded delay issue mentioned earlier.
229-298: LGTM! Staff listing and revocation tests are thorough.Excellent coverage including the important self-revocation prevention check (lines 268-275). The tests properly validate access control and state transitions. Note that line 288 has the same hardcoded delay issue mentioned earlier.
andrewx-bu
force-pushed
the
feat/ci-and-tests
branch
from
cad442d to
6886908
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/__tests__/rbac.test.ts (1)
77-77: Consider extracting the Clerk propagation delay to a constant.The 1000ms delay for Clerk propagation appears three times throughout the test suite. Extracting it to a named constant would improve maintainability.
Add at the top of the file after the environment variables:
const CLERK_PROPAGATION_DELAY_MS = 1000;Then replace the hardcoded values:
- await new Promise(resolve => setTimeout(resolve, 1000)); + await new Promise(resolve => setTimeout(resolve, CLERK_PROPAGATION_DELAY_MS));Also applies to: 219-219, 299-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/backend-rbac-e2e.yml(1 hunks)backend/__tests__/rbac.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/backend-rbac-e2e.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
📚 Learning: 2025-11-08T23:00:55.189Z
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
Applied to files:
backend/__tests__/rbac.test.ts
🔇 Additional comments (2)
backend/__tests__/rbac.test.ts (2)
1-12: LGTM! Environment validation implemented correctly.The environment variable validation has been properly implemented with a clear error message, addressing the previous review feedback effectively.
88-310: Excellent test coverage and structure.This comprehensive RBAC test suite effectively validates the entire role lifecycle (request → pending → approval → staff → revocation) with appropriate positive and negative test cases. The sequential workflow is well-organized and covers authorization, authentication, and state transition edge cases.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
mobile/src/lib/rbacClient.tsx (2)
283-286: Duplicate: Force reload on every mount still bypasses deduplication.This is the same issue flagged in previous reviews: passing
force=trueon line 285 bypasses in-flight request deduplication, so multiple simultaneous component mounts will each trigger separate/api/mecalls.The previous suggestion was to check if data exists before forcing:
- // Always (re)load once per signed-in session - if (isSignedIn && !rbacLoading) { - log('useRbac', 'Initiating fetchRbacInternal...'); - fetchRbacInternal(getToken, true); + // Load once per signed-in session if data is missing + if (isSignedIn && !rbacLoading && !rbacData) { + log('useRbac', 'Initiating fetchRbacInternal...'); + fetchRbacInternal(getToken, false); }Based on learnings from past reviews.
302-309: Duplicate: Unstable getToken reference in dependencies.These dependency array issues were flagged in previous reviews:
- Line 303: Missing
getTokenfrom the effect dependencies (despite using it on line 285), combined with an eslint-disable comment that suppresses the warning- Line 309: Including
getTokenin the refresh callback dependencies causes unnecessary re-renders because Clerk'suseAuthhook is not memoizedThe previous suggestion was to remove
getTokenfrom dependency arrays since its reference is unstable, or stabilize it withuseCallback. The effect should depend onisSignedInonly, and the refresh callback should be restructured to avoid the getToken dependency.Based on learnings from past reviews.
🧹 Nitpick comments (1)
mobile/app/welcome.tsx (1)
35-46: Consider making the effect idempotent instead of using a guard ref.The
hasCheckedref prevents the effect from running more than once, which works around React 18 Strict Mode double-mounting. However, a more robust approach is to make the effect idempotent or use proper cleanup.Since the onboarding check involves async operations and redirects, the current approach is acceptable. Just ensure that if the component unmounts during async operations, you don't update state on an unmounted component. Consider adding an abort controller:
useEffect(() => { if (!authLoaded || !userLoaded) return; const abortController = new AbortController(); const checkOnboardingStatus = async () => { // ... existing logic ... if (abortController.signal.aborted) return; // ... before each setState call, check abort signal ... }; checkOnboardingStatus(); return () => abortController.abort(); }, [authLoaded, userLoaded, /* other deps */]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mobile/app/welcome.tsx(7 hunks)mobile/src/lib/rbacClient.tsx(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
🧬 Code graph analysis (1)
mobile/app/welcome.tsx (2)
mobile/src/lib/firebase/config.ts (1)
firestore(41-43)mobile/src/lib/rbacClient.tsx (1)
fetchMe(121-124)
🔇 Additional comments (4)
mobile/app/welcome.tsx (2)
182-184: LGTM!The text wrapping changes improve code readability without affecting functionality.
Also applies to: 198-200
131-136: LGTM!The error handling for
fetchMefailures is appropriate, with proper logging and a graceful fallback to the student route. This ensures users aren't blocked if RBAC loading fails.mobile/src/lib/rbacClient.tsx (2)
54-115: LGTM!The refactored
backendFetchincludes good error handling improvements:
- Explicit token validation before making requests
- Try/catch wrapper for better exception handling
- Defensive JSON parsing with fallback
- Detailed error messages
The extensive logging is acceptable as long as the DEBUG flag is properly configured (flagged separately).
131-138: LGTM!The additions of log statements to RBAC functions (
requestStaffRole,fetchPendingStaff,fetchActiveStaff,approveStaff,revokeStaff) improve observability and are consistent with the logging pattern throughout the file. The formatting changes (trailing commas) align with project conventions.Also applies to: 144-149, 155-158, 164-175, 182-193
| const log = (msg: string, data?: any) => { | ||
| const timestamp = new Date().toISOString(); | ||
| if (data) { | ||
| console.log(`[${timestamp}] [welcome.tsx] ${msg}`, data); | ||
| } else { | ||
| console.log(`[${timestamp}] [welcome.tsx] ${msg}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the log helper to avoid duplication.
This log helper duplicates the same pattern in mobile/src/lib/rbacClient.tsx (lines 38-47). Additionally, there's no DEBUG flag to disable logging in production, which can impact performance and expose implementation details.
Extract the log helper to a shared utility module and add a DEBUG flag:
// mobile/src/lib/utils/logger.ts
const DEBUG = __DEV__ || process.env.EXPO_PUBLIC_DEBUG === 'true';
export const createLogger = (moduleName: string) => {
return (msg: string, data?: any) => {
if (!DEBUG) return;
const timestamp = new Date().toISOString();
if (data) {
console.log(`[${timestamp}] [${moduleName}] ${msg}`, data);
} else {
console.log(`[${timestamp}] [${moduleName}] ${msg}`);
}
};
};Then import and use it:
-const log = (msg: string, data?: any) => {
- const timestamp = new Date().toISOString();
- if (data) {
- console.log(`[${timestamp}] [welcome.tsx] ${msg}`, data);
- } else {
- console.log(`[${timestamp}] [welcome.tsx] ${msg}`);
- }
-};
+import { createLogger } from '../src/lib/utils/logger';
+
+const log = createLogger('welcome.tsx');🤖 Prompt for AI Agents
In mobile/app/welcome.tsx around lines 18-25, the inline log helper duplicates
code in mobile/src/lib/rbacClient.tsx and lacks a DEBUG toggle; extract the
helper into a shared file mobile/src/lib/utils/logger.ts that exports a
createLogger(moduleName: string) factory which uses a DEBUG flag (e.g. __DEV__
|| process.env.EXPO_PUBLIC_DEBUG === 'true') to no-op when false and otherwise
logs timestamped messages, then replace the local log function with an import of
createLogger('welcome.tsx') and update mobile/src/lib/rbacClient.tsx to use the
same shared logger, removing the duplicated implementation and adding the
appropriate import/exports.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [authLoaded, userLoaded]); |
There was a problem hiding this comment.
Address the incomplete dependency array.
The effect uses getToken, isSignedIn, and user but only includes authLoaded and userLoaded in the dependency array. While the hasChecked ref prevents re-runs, this creates a stale closure risk and suppressing the exhaustive-deps lint rule hides the issue.
Either include all dependencies or explicitly document why they're safe to omit:
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [authLoaded, userLoaded]);
+ }, [authLoaded, userLoaded, isSignedIn, user?.id, getToken]);Note: If getToken reference instability causes issues (Clerk's useAuth is not memoized), consider stabilizing it with useCallback or using a ref to store the latest value.
📝 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.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [authLoaded, userLoaded]); | |
| }, [authLoaded, userLoaded, isSignedIn, user?.id, getToken]); |
🤖 Prompt for AI Agents
In mobile/app/welcome.tsx around lines 145 to 146, remove the
eslint-disable-next-line comment and properly address the incomplete dependency
array in the useEffect hook. Add `getToken`, `isSignedIn`, and `user` to the
dependency array if they should trigger the effect, or if these dependencies are
intentionally excluded for performance reasons, add an explanatory comment
documenting why it's safe to omit them. If reference instability from Clerk's
useAuth is causing issues, consider wrapping `getToken` with useCallback to
stabilize its reference before using it in the effect.
| let API_BASE_URL = process.env.EXPO_PUBLIC_BACKEND_URL; | ||
|
|
||
| // Fallback for iOS Expo Go env var loading issues | ||
| if (!API_BASE_URL) { | ||
| API_BASE_URL = | ||
| 'https://freebites-backend-2a41n9xzn-andrewx-bus-projects.vercel.app'; | ||
| console.warn( | ||
| '[rbacClient] EXPO_PUBLIC_BACKEND_URL is not set. RBAC calls will fail.' | ||
| '[rbacClient] ⚠️ EXPO_PUBLIC_BACKEND_URL not found, using hardcoded fallback', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove the hardcoded fallback URL and use immutable configuration.
Critical issues with this implementation:
- Mutable module-level state (
letinstead ofconst) is an anti-pattern that can lead to unexpected behavior - Hardcoded URL appears to be a personal/temporary Vercel deployment and should never be in production code
- Security risk: Hardcoded endpoints can expose temporary or development infrastructure
- Fail-fast principle violated: The app should error clearly if the environment variable is missing, not silently use a fallback
Apply this diff to make the configuration immutable and fail explicitly:
-let API_BASE_URL = process.env.EXPO_PUBLIC_BACKEND_URL;
+const API_BASE_URL = process.env.EXPO_PUBLIC_BACKEND_URL;
-// Fallback for iOS Expo Go env var loading issues
if (!API_BASE_URL) {
- API_BASE_URL =
- 'https://freebites-backend-2a41n9xzn-andrewx-bus-projects.vercel.app';
- console.warn(
- '[rbacClient] ⚠️ EXPO_PUBLIC_BACKEND_URL not found, using hardcoded fallback',
- );
+ throw new Error(
+ '[rbacClient] EXPO_PUBLIC_BACKEND_URL must be set. Check your .env configuration.',
+ );
}If you genuinely need a fallback for development, use a separate dev-only environment variable or feature flag, not a hardcoded URL.
📝 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.
| let API_BASE_URL = process.env.EXPO_PUBLIC_BACKEND_URL; | |
| // Fallback for iOS Expo Go env var loading issues | |
| if (!API_BASE_URL) { | |
| API_BASE_URL = | |
| 'https://freebites-backend-2a41n9xzn-andrewx-bus-projects.vercel.app'; | |
| console.warn( | |
| '[rbacClient] EXPO_PUBLIC_BACKEND_URL is not set. RBAC calls will fail.' | |
| '[rbacClient] ⚠️ EXPO_PUBLIC_BACKEND_URL not found, using hardcoded fallback', | |
| ); | |
| } | |
| const API_BASE_URL = process.env.EXPO_PUBLIC_BACKEND_URL; | |
| if (!API_BASE_URL) { | |
| throw new Error( | |
| '[rbacClient] EXPO_PUBLIC_BACKEND_URL must be set. Check your .env configuration.', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In mobile/src/lib/rbacClient.tsx around lines 25 to 34, the module currently
uses mutable state and a hardcoded fallback URL; change the implementation to
use an immutable constant for API_BASE_URL (const), remove the hardcoded Vercel
URL entirely, and throw an explicit, descriptive error if
process.env.EXPO_PUBLIC_BACKEND_URL is missing so the app fails fast; if you
need a dev-only fallback, read a separate env var (e.g.
EXPO_PUBLIC_BACKEND_URL_DEV) and only use it behind a NODE_ENV/dev feature-flag,
otherwise do not provide any in-code fallback.
| const DEBUG = true; // Toggle for debugging | ||
|
|
||
| const log = (tag: string, msg: string, data?: any) => { | ||
| if (DEBUG) { | ||
| const timestamp = new Date().toISOString(); | ||
| if (data) { | ||
| console.log(`[${timestamp}] [${tag}] ${msg}`, data); | ||
| } else { | ||
| console.log(`[${timestamp}] [${tag}] ${msg}`); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| console.log('[rbacClient] API_BASE_URL:', API_BASE_URL); |
There was a problem hiding this comment.
Use environment-based DEBUG flag instead of hardcoded true.
The DEBUG flag is hardcoded to true, meaning all debug logs will run in production, impacting performance and exposing implementation details. Line 49's console.log runs on every module import.
Apply this diff to make DEBUG configurable:
-const DEBUG = true; // Toggle for debugging
+const DEBUG = __DEV__ || process.env.EXPO_PUBLIC_DEBUG === 'true';And make the startup log conditional:
-console.log('[rbacClient] API_BASE_URL:', API_BASE_URL);
+if (DEBUG) {
+ console.log('[rbacClient] API_BASE_URL:', API_BASE_URL);
+}Note: The log helper duplication with welcome.tsx was already flagged in that file's review.
📝 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 DEBUG = true; // Toggle for debugging | |
| const log = (tag: string, msg: string, data?: any) => { | |
| if (DEBUG) { | |
| const timestamp = new Date().toISOString(); | |
| if (data) { | |
| console.log(`[${timestamp}] [${tag}] ${msg}`, data); | |
| } else { | |
| console.log(`[${timestamp}] [${tag}] ${msg}`); | |
| } | |
| } | |
| }; | |
| console.log('[rbacClient] API_BASE_URL:', API_BASE_URL); | |
| const DEBUG = __DEV__ || process.env.EXPO_PUBLIC_DEBUG === 'true'; | |
| const log = (tag: string, msg: string, data?: any) => { | |
| if (DEBUG) { | |
| const timestamp = new Date().toISOString(); | |
| if (data) { | |
| console.log(`[${timestamp}] [${tag}] ${msg}`, data); | |
| } else { | |
| console.log(`[${timestamp}] [${tag}] ${msg}`); | |
| } | |
| } | |
| }; | |
| if (DEBUG) { | |
| console.log('[rbacClient] API_BASE_URL:', API_BASE_URL); | |
| } |
🤖 Prompt for AI Agents
In mobile/src/lib/rbacClient.tsx around lines 36 to 49, replace the hardcoded
DEBUG=true with an environment-driven flag (e.g., derive DEBUG from
process.env.NODE_ENV !== 'production' or a specific REACT_APP_DEBUG/ENV var) so
debug logging only runs when enabled, and update the log helper to check that
flag; also make the startup console.log('[rbacClient] API_BASE_URL:',
API_BASE_URL) conditional on the same debug flag so it does not run on every
module import in production; ensure you do not reintroduce duplicate logging
helpers already flagged in welcome.tsx — reuse or centralize the log helper if
one exists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mobile/app/index.tsx (1)
4-4: Remove debug console.log.This module-level console.log statement should be removed before production deployment, as it provides minimal value and executes every time the module is loaded.
Apply this diff to remove the debug statement:
-console.log('index.tsx loaded - redirecting to welcome'); - export default function IndexScreen() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mobile/app/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewx-bu
Repo: BU-Spark/se-bu-catering-leftovers PR: 38
File: backend/api/admin/revoke/[uid].ts:27-0
Timestamp: 2025-11-08T23:00:55.189Z
Learning: User andrewx-bu (in repository BU-Spark/se-bu-catering-leftovers) has declined social media promotion requests and should not be asked about sharing CodeRabbit on X/Twitter or other social platforms in future interactions.
🔇 Additional comments (1)
mobile/app/index.tsx (1)
6-8: LGTM!The component correctly redirects to the welcome route, providing a clean entry point for the onboarding flow.
2 participants
Note: This PR is stacked on top of the RBAC wireup PR (#43). Please review and merge #43 first. I will rebase after.
Summary:
backend/mobile/mobile/Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.