fix: code quality and safety improvements - #743
Conversation
📝 WalkthroughWalkthroughThe changes add HTTP failure checks to calendar requests, add logging for rejected push registration and attendance fetch operations, remove push notification listeners and returned state, and replace Cypress mock-user setup with error logging. ChangesError handling updates
Estimated code review effort: 3 (Moderate) | ~20 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/src/lib/usePushNotifications.jsFile contains syntax errors that prevent linting: Line 55: Expected a statement but instead found '.catch(err => console.error(err))'.; Line 55: expected 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.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. app/src/lib/CalendarService.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. app/src/lib/usePushNotifications.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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: 4
🤖 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 authenticated mock-user setup in the protected flow tests by
calling globalThis.setMockUser(...) after cy.visit('/') in the critical flow
setup. Ensure the mock user is configured before the flow assertions execute,
preserving the existing test sequence and error handling.
- Line 6: Fix the cy.window() setup in the beforeEach hook by completing or
removing the .then(win => { callback; do not attach .catch(...) as a Cypress
command-chain API. Ensure the beforeEach and describe blocks are properly closed
so the spec parses successfully.
In `@app/src/lib/CalendarService.js`:
- Around line 87-88: Update the calendar response mocks used by the affected
tests to include ok: true for body-error scenarios, preserving their expected
“API Failure” and “Calendar Failure” results. Add a separate mock with ok: false
to cover the HTTP-failure branch introduced by the response.ok guard in the
calendar request functions.
In `@app/src/lib/usePushNotifications.js`:
- Line 55: Complete the promise chains and closing braces at
app/src/lib/usePushNotifications.js:55 by attaching catch to the registration
promise, then closing useEffect and usePushNotifications; at
app/src/screens/AttendanceDashboard.js:167, close the event-document then
callback, attach catch to the promise chain, and close useEffect and the
component.
🪄 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: d3a7e698-1974-47bc-a95a-2dbf65718f72
📒 Files selected for processing (4)
app/cypress/e2e/critical_flows.cy.jsapp/src/lib/CalendarService.jsapp/src/lib/usePushNotifications.jsapp/src/screens/AttendanceDashboard.js
| 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 | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)critical_flows\.cy\.js$|(^|/)AuthContext\.js$|(^|/).*[Ll]og[Ll]og|setMockUser|cy\.visit|cy\.getLogin' || true
echo "== target file outlint/section =="
if [ -f app/cypress/e2e/critical_flows.cy.js ]; then
wc -l app/cypress/e2e/critical_flows.cy.js
cat -n app/cypress/e2e/critical_flows.cy.js
fi
echo "== AuthContext section =="
if [ -f app/src/lib/AuthContext.js ]; then
wc -l app/src/lib/AuthContext.js
cat -n app/src/lib/AuthContext.js | sed -n '1,140p'
fi
echo "== usages in cypress =="
rg -n "setMockUser|mock(user|User)|cy\.visit|beforeEach|AuthContext|cy\.login|login" app/cypress app/src -S || true
echo "== diff stat =="
git diff --stat || true
git diff -- app/cypress/e2e/critical_flows.cy.js app/src/lib/AuthContext.js || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 13105
Restore the mock-user setup for protected flow tests.
app/cypress/e2e/critical_flows.cy.js now visits / without calling setMockUser, so these assertions can run with no authenticated user state. Call globalThis.setMockUser(...) after cy.visit('/') or add another authenticated setup before the flow tests.
🧰 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)
🤖 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 authenticated
mock-user setup in the protected flow tests by calling
globalThis.setMockUser(...) after cy.visit('/') in the critical flow setup.
Ensure the mock user is configured before the flow assertions execute,
preserving the existing test sequence and error handling.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and size =="
git ls-files | grep -F 'app/cypress/e2e/critical_flows.cy.js' || true
wc -l app/cypress/e2e/critical_flows.cy.js
echo "== relevant file contents =="
cat -n app/cypress/e2e/critical_flows.cy.js
echo "== search for mock user/window context =="
rg -n "setMockUser|beforeEach|describe|cy\.window|window" app/cypress/e2e app -S || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 7621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JS parse/check for critical_flows =="
node - <<'JS'
const fs = require('fs');
const path = 'app/cypress/e2e/critical_flows.cy.js';
const src = fs.readFileSync(path, 'utf8');
try {
new Function(src);
console.log('node parses: ok');
} catch (err) {
console.log(`node parse error: ${err.name}: ${err.message}`);
console.log(err.stack.split('\n').slice(0, 3).join('\n'));
}
JS
echo "== package/cypress tooling availability =="
node - <<'JS'
for (const mod of ['cypress']) {
try {
const p = require.resolve(mod);
console.log(`${mod}: ${p}`);
} catch (err) {
console.log(`${mod}: not resolvable`);
}
}
JS
echo "== Cypress package files mentioning catch handling / .then signatures =="
fd -a 'package.json|CHANGELOG.md|dist|src' app 2>/dev/null | sed 's#^\./##' | head -50 || trueRepository: roshankumar0036singh/Uni-Event
Length of output: 551
Fix the malformed Cypress chain before merging.
Line 5 opens a .then(win => { callback, but line 6 starts with .catch(...) instead of using catch inside that callback. The beforeEach and describe blocks are also not closed, so this spec cannot be parsed.
Complete or remove the cy.window() setup, then close beforeEach and describe; .catch is not a command-chain API and should not attach here if command failures are being handled.
🧰 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)
🤖 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, Fix the cy.window() setup in
the beforeEach hook by completing or removing the .then(win => { callback; do
not attach .catch(...) as a Cypress command-chain API. Ensure the beforeEach and
describe blocks are properly closed so the spec parses successfully.
Source: Linters/SAST tools
| if (!response.ok) throw new Error("Request failed"); | ||
| const data = await response.json(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'app/src/lib/(CalendarService\.js|__tests__/CalendarService\.test\.js)$' || true
echo "== CalendarService outline =="
ast-grep outline app/src/lib/CalendarService.js || true
echo "== CalendarService relevant lines =="
sed -n '1,140p' app/src/lib/CalendarService.js
echo "== CalendarService tests relevant lines =="
sed -n '1,220p' app/src/lib/__tests__/CalendarService.test.js
echo "== fetch resolvers in tests =="
python3 - <<'PY'
from pathlib import Path
p=Path('app/src/lib/__tests__/CalendarService.test.js')
s=p.read_text()
for i,line in enumerate(s.splitlines(),1):
if 'mockResolvedValueOnce' in line or 'mockRejectedValueOnce' in line:
print(f"{i}: {line}")
PYRepository: roshankumar0036singh/Uni-Event
Length of output: 8698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic probe for response.ok guard behavior =="
python3 - <<'PY'
def read_error_mock_without_ok(body_error='API Failure'):
response = {
"ok": None,
"status": None,
"json": lambda: {"error": body_error}
}
try:
if not response["ok"]:
raise Exception("Request failed")
data = response["json"]()
return ("parsed", data)
except Exception as e:
return ("raised", str(e))
for body in ["API Failure", "Calendar Failure"]:
print({body: read_error_mock_without_ok(body)})
PYRepository: roshankumar0036singh/Uni-Event
Length of output: 322
Update the calendar response mocks for the new response.ok guard.
Both failing calendar tests return mock responses without ok, and both functions now check if (!response.ok) before parsing response bodies. As written, these tests throw "Request failed" instead of "API Failure" or "Calendar Failure". Add ok: true to the body-error mocks and add a separate ok: false HTTP-failure path.
🤖 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/lib/CalendarService.js` around lines 87 - 88, Update the calendar
response mocks used by the affected tests to include ok: true for body-error
scenarios, preserving their expected “API Failure” and “Calendar Failure”
results. Add a separate mock with ok: false to cover the HTTP-failure branch
introduced by the response.ok guard in the calendar request functions.
|
|
||
| return { expoPushToken, notification }; | ||
| }; | ||
| .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 "== Locate files =="
git ls-files | rg '(^|/)usePushNotifications\.js$|(^|/)AttendanceDashboard\.js$' || true
echo "== Show relevant slices with line numbers =="
for f in app/src/lib/usePushNotifications.js app/src/screens/AttendanceDashboard.js; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,120p' "$f" | cat -n
else
echo "missing $f"
fi
done
echo "== Parse check with Node parser (read-only) =="
python3 - <<'PY'
from pathlib import Path
files = Path('app/src/lib/usePushNotifications.js').resolve(), Path('app/src/screens/AttendanceDashboard.js').resolve()
for p in files:
if p.exists():
print(p)
print(open(p, encoding='utf-8').read()[-1200:])
PYRepository: roshankumar0036singh/Uni-Event
Length of output: 10110
🌐 Web query:
AttendanceDashboard.js 167 getDoc doc(db COLLECTIONS.EVENTS eventId) then catch
💡 Result:
In the Firebase Modular SDK (v9+), the getDoc function returns a Promise that resolves to a DocumentSnapshot [1]. You can handle this operation using either the.then.catch pattern or async/await syntax [2][3]. Based on your query regarding AttendanceDashboard.js, the following implementation shows how to use getDoc with a promise chain and error handling: import { doc, getDoc } from "firebase/firestore"; // Assuming db and COLLECTIONS.EVENTS are defined correctly const docRef = doc(db, COLLECTIONS.EVENTS, eventId); getDoc(docRef).then((docSnap) => { if (docSnap.exists) { console.log("Document data:", docSnap.data); } else { console.log("No such document!"); } }).catch((error) => { console.error("Error getting document:", error); }); Key points for this implementation: 1. Document Reference: Use the doc function to create a reference to the specific document, passing the database instance, collection name, and document ID [2][4]. 2. Fetching: getDoc is an asynchronous operation that returns a Promise [1]. 3. Existence Check: Always check if the document exists using the.exists method on the returned snapshot before attempting to access its data with.data [2][5][6]. 4. Error Handling: The.catch block will capture errors related to the fetch operation, such as permission issues or network failures [2][3]. Alternatively, many developers prefer async/await for cleaner syntax: try { const docSnap = await getDoc(docRef); if (docSnap.exists) { console.log("Document data:", docSnap.data); } else { console.log("No such document!"); } } catch (error) { console.error("Error getting document:", error); }
Citations:
- 1: https://modularfirebase.web.app/reference/firestore_.getdoc
- 2: https://dev.to/rajatamil/firebase-9-firestore-get-a-document-by-id-using-getdoc-3j4f
- 3: https://stackoverflow.com/questions/74515242/chnges-with-regard-to-firebase-v9
- 4: https://stackoverflow.com/questions/69012256/how-do-you-find-a-document-by-its-id-using-version-9-of-the-firebase-js-sdk
- 5: https://modularfirebase.web.app/common-use-cases/firestore
- 6: https://firebase.google.com/docs/firestore/query-data/get-data
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AttendanceDashboard remaining lines =="
sed -n '140,180p' app/src/screens/AttendanceDashboard.js | cat -n
echo "== usePushNotifications remaining lines =="
sed -n '50,70p' app/src/lib/usePushNotifications.js | cat -n
echo "== Deterministic JS parse probe for dangling catch syntax =="
node - <<'JS'
const snippet = `
registerForPushNotificationsAsync().then(token => setExpoPushToken(token));
.catch(err => console.error(err))
`;
try {
Function(snippet);
console.log("Parses as", true);
} catch (err) {
console.log("Parses as", false, err.message);
}
JSRepository: roshankumar0036singh/Uni-Event
Length of output: 1732
Complete the broken promise chains and closures.
Both changed files stop before the useEffect, hook, or component closing braces, and each leaves .catch as an invalid standalone statement.
app/src/lib/usePushNotifications.js#L55: attach.catchto the registration promise, closeuseEffect, and closeusePushNotifications.app/src/screens/AttendanceDashboard.js#L167: close the event-documentthen, then attach.catch, closeuseEffect, and close the component.
🧰 Tools
🪛 Biome (2.5.5)
[error] 55-55: Expected a statement but instead found '.catch(err => console.error(err))'.
(parse)
[error] 55-55: expected } but instead the file ends
(parse)
📍 Affects 2 files
app/src/lib/usePushNotifications.js#L55-L55(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/src/lib/usePushNotifications.js` at line 55, Complete the promise chains
and closing braces at app/src/lib/usePushNotifications.js:55 by attaching catch
to the registration promise, then closing useEffect and usePushNotifications; at
app/src/screens/AttendanceDashboard.js:167, close the event-document then
callback, attach catch to the promise chain, and close useEffect and the
component.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit