fix: code quality and safety improvements - #749
Conversation
|
📝 WalkthroughWalkthroughThe changes add rejection logging to Cypress setup and attendance event retrieval. Profile and feed year parsing now use radix 10. Migration filenames now sort by numeric value. ChangesError handling updates
Numeric parsing and migration ordering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5)app/cypress/e2e/critical_flows.cy.jsFile contains syntax errors that prevent linting: Line 6: Expected a statement but instead found '.catch(err => console.error(err))'.; Line 6: expected app/src/screens/AttendanceDashboard.jsFile contains syntax errors that prevent linting: Line 167: Expected a statement but instead found '.catch(err => console.error(err))'.; Line 167: expected 🔧 ESLint
app/cypress/e2e/critical_flows.cy.jsParsing error: Unexpected token . app/src/screens/AttendanceDashboard.jsParsing error: Unexpected token . 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 Warning |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/cypress/e2e/critical_flows.cy.js (1)
1-6: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRestore critical-flow assertions before merging.
The remaining spec only visits
/. It no longer initializes the mock student session or checks event-feed rendering, tab navigation, profile validation, or event-search behavior. After the syntax fix, this spec can pass without testing the flows namedCritical Event App Flows. Restore equivalent assertions or add replacement coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/cypress/e2e/critical_flows.cy.js` around lines 1 - 6, Restore the missing critical-flow coverage in the describe block for Critical Event App Flows: initialize the mock student session through the existing Cypress window hook, then add assertions for event-feed rendering, tab navigation, profile validation, and event-search behavior. Ensure beforeEach setup completes without the current syntax error and that the spec cannot pass based solely on cy.visit('/').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/cypress/e2e/critical_flows.cy.js`:
- Around line 5-6: Fix the incomplete rejection-handler chains in
app/cypress/e2e/critical_flows.cy.js lines 5-6 and
app/src/screens/AttendanceDashboard.js lines 167-167: complete or remove the
cy.window().then chain and use supported Cypress failure handling, complete the
.then() callback before attaching .catch, and close the surrounding useEffect
and component blocks.
In `@app/src/screens/AttendanceDashboard.js`:
- Line 167: Remove the obsolete catch-only attendance flow and delete the
attendance routes/callers if attendance features are no longer supported;
otherwise restore the missing UI, listeners, and attendance actions in
AttendanceDashboard so the registered My Events and Event Detail entry points
remain functional.
---
Outside diff comments:
In `@app/cypress/e2e/critical_flows.cy.js`:
- Around line 1-6: Restore the missing critical-flow coverage in the describe
block for Critical Event App Flows: initialize the mock student session through
the existing Cypress window hook, then add assertions for event-feed rendering,
tab navigation, profile validation, and event-search behavior. Ensure beforeEach
setup completes without the current syntax error and that the spec cannot pass
based solely on cy.visit('/').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e9bf5a5-1748-4c98-9428-a9c35a50db3b
📒 Files selected for processing (5)
app/cypress/e2e/critical_flows.cy.jsapp/src/screens/AttendanceDashboard.jsapp/src/screens/ProfileScreen.jsapp/src/screens/UserFeed.jscloud-functions/migrations/migrate.ts
| cy.window().then(win => { | ||
| win.setMockUser( | ||
| { | ||
| uid: 'student-test-uid', | ||
| displayName: 'Jane Doe', | ||
| email: 'jane.doe@uni.edu', | ||
| }, | ||
| 'student', | ||
| { | ||
| name: 'Jane Doe', | ||
| email: 'jane.doe@uni.edu', | ||
| branch: 'Computer Science', | ||
| year: '3rd Year', | ||
| points: 120, | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| it('should render the event feed home page correctly', () => { | ||
| // Verify welcome text is loaded from the mock user displayName | ||
| cy.contains('Welcome,').should('be.visible'); | ||
| cy.contains('Jane Doe').should('be.visible'); | ||
|
|
||
| // Verify search bar is visible | ||
| cy.get('input[placeholder="Search events..."]').should('be.visible'); | ||
|
|
||
| // Verify recommendations section header is visible | ||
| cy.contains('RECOMMENDED FOR YOU').should('be.visible'); | ||
| }); | ||
|
|
||
| it('should support tab navigation to Leaderboard and Profile', () => { | ||
| // We should be able to navigate to Leaderboard using tab bar | ||
| cy.contains('Rankings').click(); | ||
|
|
||
| // Verify Leaderboard screen is shown | ||
| cy.contains('LEADERBOARD').should('be.visible'); | ||
| cy.contains('Top Contributors').should('be.visible'); | ||
|
|
||
| // Navigate to Profile using tab bar | ||
| cy.contains('Profile').click(); | ||
|
|
||
| // Verify Profile screen renders correctly | ||
| cy.contains('Jane Doe').should('be.visible'); | ||
| cy.contains('jane.doe@uni.edu').should('be.visible'); | ||
| cy.contains('Computer Science').should('be.visible'); | ||
| cy.contains('3rd Year').should('be.visible'); | ||
| cy.contains('Student Settings').should('be.visible'); | ||
| }); | ||
|
|
||
| it('should allow searching for events in the feed', () => { | ||
| const searchQuery = 'Hackathon'; | ||
| // Type search query | ||
| cy.get('input[placeholder="Search events..."]') | ||
| .type(searchQuery) | ||
| .should('have.value', searchQuery); | ||
|
|
||
| // Verify close icon appears via stable testID | ||
| cy.get('[data-testid="clear-search-button"]').should('exist'); | ||
| }); | ||
| }); | ||
| .catch(err => console.error(err)) No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)(critical_flows\.cy\.js|AttendanceDashboard\.js)$' || true
echo
echo "critical_flows outline/size:"
if [ -f app/cypress/e2e/critical_flows.cy.js ]; then
wc -l app/cypress/e2e/critical_flows.cy.js
seq 1 50 | while read -r n; do printf '%5d ' "$n"; sed -n "${n}p" app/cypress/e2e/critical_flows.cy.js | cat -n -v; done
fi
echo
echo "AttendanceDashboard relevant lines:"
if [ -f app/src/screens/AttendanceDashboard.js ]; then
wc -l app/src/screens/AttendanceDashboard.js
sed -n '130,190p' app/src/screens/AttendanceDashboard.js | nl -ba -v130
fi
echo
echo "Syntax check with node:"
node --check app/cypress/e2e/critical_flows.cy.js 2>&1 || true
node --check app/src/screens/AttendanceDashboard.js 2>&1 || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "critical_flows file:"
wc -l app/cypress/e2e/critical_flows.cy.js
awk '{printf "%5d %s\n", NR, $0}' app/cypress/e2e/critical_flows.cy.js
echo
echo "AttendanceDashboard relevant lines:"
wc -l app/src/screens/AttendanceDashboard.js
awk 'NR>=130 && NR<=195 {printf "%5d %s\n", NR, $0}' app/src/screens/AttendanceDashboard.js
echo
echo "Syntax check with node:"
node --check app/cypress/e2e/critical_flows.cy.js 2>&1 || true
node --check app/src/screens/AttendanceDashboard.js 2>&1 || true
echo
echo "Promise/then/catch pattern around AttendanceDashboard:"
grep -n "getDoc\\\(|.then\\|catch\\|useEffect" app/src/screens/AttendanceDashboard.js || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 2708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AttendanceDashboard outline:"
ast-grep outline app/src/screens/AttendanceDashboard.js --view expanded || true
echo
echo "AttendanceDashboard top/structure:"
awk 'NR>=1 && NR<=90 {printf "%5d %s\n", NR, $0}' app/src/screens/AttendanceDashboard.js
echo
echo "GetDoc usages and hooks/effects context:"
grep -n "getDoc\\|useEffect\\|const\\|function\\|\\}" app/src/screens/AttendanceDashboard.js | awk 'NR>=1 && NR<=120'Repository: roshankumar0036singh/Uni-Event
Length of output: 7392
Fix both incomplete rejection-handler chains before merging.
Both sites parse as invalid JavaScript because .catch(...) is not attached to a complete preceding callback body. Attach the handler to the intended promise/callback chain and close all open blocks.
app/cypress/e2e/critical_flows.cy.js#L5-L6:cy.window().then(...)has no function body; close or remove it, then use supported Cypress failure handling.app/src/screens/AttendanceDashboard.js#L165-L167: the.then()callback body is missing before.catch(...), and theuseEffectplus component body are unterminated.
🧰 Tools
🪛 Biome (2.5.5)
[error] 6-6: Expected a statement but instead found '.catch(err => console.error(err))'.
(parse)
[error] 6-6: expected } but instead the file ends
(parse)
📍 Affects 2 files
app/cypress/e2e/critical_flows.cy.js#L5-L6(this comment)app/src/screens/AttendanceDashboard.js#L167-L167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/cypress/e2e/critical_flows.cy.js` around lines 5 - 6, Fix the incomplete
rejection-handler chains in app/cypress/e2e/critical_flows.cy.js lines 5-6 and
app/src/screens/AttendanceDashboard.js lines 167-167: complete or remove the
cy.window().then chain and use supported Cypress failure handling, complete the
.then() callback before attaching .catch, and close the surrounding useEffect
and component blocks.
Source: Linters/SAST tools
| data: PropTypes.object.isRequired, | ||
| icon: PropTypes.string.isRequired, | ||
| }; | ||
| .catch(err => console.error(err)) No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 '\bAttendanceDashboard\b' app --glob '*.{js,jsx,ts,tsx}'Repository: roshankumar0036singh/Uni-Event
Length of output: 4716
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== AttendanceDashboard outline =="
ast-grep outline app/src/screens/AttendanceDashboard.js || true
echo
echo "== AttendanceDashboard top and bottom relevant lines =="
wc -l app/src/screens/AttendanceDashboard.js
sed -n '1,240p' app/src/screens/AttendanceDashboard.js
echo
echo "== Navigation destinations and callers around AttendanceDashboard =="
sed -n '1,45p' app/App.js
sed -n '250,285p' app/App.js
sed -n '130,170p' app/src/screens/MyEventsScreen.js
sed -n '2268,2296p' app/src/screens/EventDetail.js
echo
echo "== Package React Native dependencies relevant to navigation availability =="
fd -a 'package.json$' app | while read -r f; do
echo "-- $f"
sed -n '1,220p' "$f" | rg -n '"react-native|`@react-navigation`|react|expo|firebase' || true
doneRepository: roshankumar0036singh/Uni-Event
Length of output: 17069
Make this a code deletion unless the attendance screens remain reachable.
AttendanceDashboard is registered in app routes and invoked from My Events and Event Detail, but the screen no longer renders UI, listeners, or attendance actions. If attendance reports/features are still supported, restore the missing screen body or remove the routes/callers tied to attendance access.
🧰 Tools
🪛 Biome (2.5.5)
[error] 167-167: Expected a statement but instead found '.catch(err => console.error(err))'.
(parse)
[error] 167-167: expected } but instead the file ends
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/screens/AttendanceDashboard.js` at line 167, Remove the obsolete
catch-only attendance flow and delete the attendance routes/callers if
attendance features are no longer supported; otherwise restore the missing UI,
listeners, and attendance actions in AttendanceDashboard so the registered My
Events and Event Detail entry points remain functional.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests