fix: resolve 2 bugs - #745
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds promise rejection logging to Cypress setup and attendance event retrieval, specifies radix 10 for year parsing in profile and feed screens, and changes migration filename sorting to numeric order. ChangesRuntime consistency updates
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 6: Restore the removed implementations at
app/cypress/e2e/critical_flows.cy.js:5-6 and
app/src/screens/AttendanceDashboard.js:167. In critical_flows.cy.js, reinstate
setMockUser and the critical-flow assertions, close cy.window().then(...), and
rely on Cypress failure handling rather than appending .catch. In
AttendanceDashboard.js, restore event-data fetching, listeners, UI, routing,
component/request closure, and preserve getAttendanceStats returning null on
errors.
In `@cloud-functions/migrations/migrate.ts`:
- Line 51: Update the migration sorting logic in the migration discovery flow to
extract and numerically compare each filename’s leading migration number rather
than subtracting complete filenames. Validate every migration name matches the
expected numeric-prefix format and reject malformed names before sorting or
execution.
🪄 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: d71a9da6-4014-45bd-8048-d3de4fbbd7b8
📒 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.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 | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
app/cypress/e2e/critical_flows.cy.js \
app/src/screens/AttendanceDashboard.js
do
ast-grep outline "$file" --items all
done
rg -n -C 10 'setMockUser|cy\.window\(\)|getDoc|getAttendanceStats|\.catch\(' \
app/cypress/e2e/critical_flows.cy.js \
app/src/screens/AttendanceDashboard.js \
app/src/lib/AuthContext.js \
app/src/lib/checkInService.js \
app/src/screens/EventDetail.jsRepository: roshankumar0036singh/Uni-Event
Length of output: 28298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
wc -l app/cypress/e2e/critical_flows.cy.js app/src/screens/AttendanceDashboard.js app/src/lib/checkInService.js app/src/screens/EventDetail.js
echo "== critical_flows.cy.js =="
cat -n app/cypress/e2e/critical_flows.cy.js
echo "== AttendanceDashboard.js relevant end scope =="
sed -n '120,220p' app/src/screens/AttendanceDashboard.js | cat -n -v
echo "== AttendanceDashboard.js imports/functions outline snippets =="
sed -n '1,130p' app/src/screens/AttendanceDashboard.js | cat -n -vRepository: roshankumar0036singh/Uni-Event
Length of output: 8093
Restore the deleted code before merging the rejection logging.
Both files leave behind a dangling .catch after removing the callback body, so the files fail parsing and lose existing behavior.
app/cypress/e2e/critical_flows.cy.js:5: RestoresetMockUserand the critical-flow assertions, closecy.window().then(...), and use Cypress failure behavior instead of appending.catchto the Cypress chain.app/src/screens/AttendanceDashboard.js:165: Restore the event-data fetch, listeners, UI, and route behavior, close the component/request scope, and preservegetAttendanceStatsnull-on-error behavior.
🧰 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#L6-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` at line 6, Restore the removed
implementations at app/cypress/e2e/critical_flows.cy.js:5-6 and
app/src/screens/AttendanceDashboard.js:167. In critical_flows.cy.js, reinstate
setMockUser and the critical-flow assertions, close cy.window().then(...), and
rely on Cypress failure handling rather than appending .catch. In
AttendanceDashboard.js, restore event-data fetching, listeners, UI, routing,
component/request closure, and preserve getAttendanceStats returning null on
errors.
Source: Linters/SAST tools
| .readdirSync(__dirname) | ||
| .filter(f => f.endsWith('.ts') && f !== 'migrate.ts') | ||
| .sort(); | ||
| .sort((a, b) => a - b); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Parse the numeric migration prefix before sorting.
The comparator subtracts complete filenames. For 001_add_feedbackRequestSent_to_events.ts, this produces NaN, so Array.sort treats the comparison as equal and does not apply numeric ordering. Migrations can therefore run in filesystem order instead of migration order.
Parse the numeric prefix and reject malformed migration names.
Proposed fix
- .sort((a, b) => a - b);
+ .sort((a, b) => {
+ const aNumber = Number.parseInt(a.split('_', 1)[0], 10);
+ const bNumber = Number.parseInt(b.split('_', 1)[0], 10);
+ if (Number.isNaN(aNumber) || Number.isNaN(bNumber)) {
+ throw new Error(`Invalid migration filename: ${a} or ${b}`);
+ }
+ return aNumber - bNumber;
+ });🤖 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 `@cloud-functions/migrations/migrate.ts` at line 51, Update the migration
sorting logic in the migration discovery flow to extract and numerically compare
each filename’s leading migration number rather than subtracting complete
filenames. Validate every migration name matches the expected numeric-prefix
format and reject malformed names before sorting or execution.
saurabhhhcodes
changed the title
|
f7157f8
into
roshankumar0036singh:main
2 participants
Description
This PR fixes real bugs found in the codebase:
.map()on an undefined collection threwTypeError; now falls back to[]..map()on an undefined collection threwTypeError; now falls back to[].Type of Change
How Has This Been Tested?
Checklist
Related Issue
Ref: #752