fix: code quality and safety improvements - #751
Conversation
|
📝 WalkthroughWalkthroughThe PR adds HTTP status checks to calendar requests, replaces global ChangesRuntime handling
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 🔧 ESLint
app/src/lib/usePushNotifications.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: 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/src/lib/CalendarService.js`:
- Around line 87-88: Move the response.json() statements inside their respective
try blocks in both calendar functions, including addToCalendar, so each function
uses its local response and data for subsequent checks. Update the “Request
failed” error text to single quotes to match formatting conventions.
- Around line 87-88: Update the successful fetch response mocks in the
CalendarService tests to include ok: true, covering both fixture blocks
referenced by the success cases. Keep the existing json() mock behavior
unchanged so CalendarService proceeds past its response.ok guard.
In `@app/src/lib/usePushNotifications.js`:
- Line 55: Update the notification channel setup around
Notifications.setNotificationChannelAsync to await the Promise or attach a local
rejection handler, ensuring Android channel-registration failures are handled by
the existing error-reporting flow.
- Line 55: Fix the Promise chain in the useEffect registration flow by attaching
catch directly to registerForPushNotificationsAsync().then(...), keeping the
error logging behavior and ending the statement only after the catch handler.
🪄 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: 694c27b8-e92a-4d4a-9994-10cbb2223ef2
📒 Files selected for processing (3)
app/src/lib/CalendarService.jsapp/src/lib/notificationService.jsapp/src/lib/usePushNotifications.js
| if (!response.ok) throw new Error("Request failed"); | ||
| const data = await response.json(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Keep response.json() inside each function.
The new const data statements are not indented under their try blocks. They execute at module scope, where response is undefined, and data is unavailable to the following checks. Indent both statements. Use single quotes for the error text to satisfy Prettier.
Proposed fix
- if (!response.ok) throw new Error("Request failed");
-const data = await response.json();
+ if (!response.ok) throw new Error('Request failed');
+ const data = await response.json();Apply the same change in addToCalendar.
Also applies to: 129-130
🧰 Tools
🪛 ESLint
[error] 87-87: Replace "Request·failed" with 'Request·failed'
(prettier/prettier)
[error] 88-88: Insert ········
(prettier/prettier)
🤖 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, Move the
response.json() statements inside their respective try blocks in both calendar
functions, including addToCalendar, so each function uses its local response and
data for subsequent checks. Update the “Request failed” error text to single
quotes to match formatting conventions.
Source: Linters/SAST tools
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the successful fetch mocks with ok: true.
The success fixtures in app/src/lib/__tests__/CalendarService.test.js omit response.ok. The new guard therefore evaluates !response.ok as true and both success tests throw before calling json().
Add ok: true to the mocked responses at app/src/lib/__tests__/CalendarService.test.js:23-42 and app/src/lib/__tests__/CalendarService.test.js:63-83.
Also applies to: 129-130
🧰 Tools
🪛 ESLint
[error] 87-87: Replace "Request·failed" with 'Request·failed'
(prettier/prettier)
[error] 88-88: Insert ········
(prettier/prettier)
🤖 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 successful
fetch response mocks in the CalendarService tests to include ok: true, covering
both fixture blocks referenced by the success cases. Keep the existing json()
mock behavior unchanged so CalendarService proceeds past its response.ok guard.
|
|
||
| return { expoPushToken, notification }; | ||
| }; | ||
| .catch(err => console.error(err)) No newline at end of file |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching usePushNotifications:\n'
fd -a 'usePushNotifications\.js$|usePushNotifications\.jsx$' . || true
printf '\nRelevant source (if present):\n'
if [ -f app/src/lib/usePushNotifications.js ]; then
nl -ba app/src/lib/usePushNotifications.js | sed -n '1,120p'
fi
printf '\nPackage manifests containing expo-notifications:\n'
rg -n --hidden --glob '!node_modules/**' '"expo-notifications"|expo-notifications' package.json pnpm-lock.yaml yarn.lock package-lock.json app/package.json app/package-lock.json 2>/dev/null || true
printf '\nLocal expo-notifications references if available:\n'
for pkg in node_modules expo-notifications node_modules/expo-notifications; do
if [ -e "$pkg" ]; then
echo "FOUND $pkg"
[ -f "$pkg/package.json" ] && sed -n '1,120p' "$pkg/package.json"
rg -n "setNotificationChannelAsync|async" "$pkg/lib" "$pkg" 2>/dev/null | head -200 || true
fi
doneRepository: roshankumar0036singh/Uni-Event
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant source:\n'
cat -n app/src/lib/usePushNotifications.js | sed -n '1,120p'
printf '\nPackage manifests containing expo-notifications:\n'
rg -n --hidden --glob '!node_modules/**' '"expo-notifications"|expo-notifications' package.json pnpm-lock.yaml yarn.lock package-lock.json app/package.json 2>/dev/null || true
printf '\nLocal expo-notifications package availability:\n'
for dir in node_modules/expo-notifications; do
if [ -d "$dir" ]; then
echo "FOUND $dir"
sed -n '1,140p' "$dir/package.json"
rg -n --hidden "setNotificationChannelAsync|NotificationChannel|exports|ReactNativeAsyncStorage|__esModule" "$dir" 2>/dev/null | head -300 || true
fi
done
printf '\nTop-level dependency files:\n'
for f in package.json app/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
python3 - "$f" <<'PY'
import json, sys
p=json.load(open(sys.argv[1]))
for k,v in p.items():
if k in {"dependencies","devDependencies","peerDependencies"} and isinstance(v, dict):
hits={kk:vv for kk,vv in v.items() if "expo" in kk.lower() or "notification" in kk.lower()}
if hits:
print(k, hits)
PY
fi
doneRepository: roshankumar0036singh/Uni-Event
Length of output: 3414
🌐 Web query:
expo-notifications 0.29.14 setNotificationChannelAsync returns Promise
💡 Result:
Yes, the function setNotificationChannelAsync in expo-notifications version 0.29.14 returns a Promise [1][2]. According to the official Expo documentation, this method assigns channel configuration to a channel of a specified name (creating it if necessary) [3][4]. The returned Promise resolves to an object of type NotificationChannel that describes the modified channel, or it resolves to null if the platform does not support notification channels [3][4]. As this function is asynchronous, it is standard practice to use the await keyword when calling it [5][6].
Citations:
- 1: https://github.com/expo/expo/blob/97172ac0/packages/expo-notifications/src/NotificationChannelManager.types.ts
- 2: https://app.unpkg.com/expo-notifications@0.29.14/files/CHANGELOG.md
- 3: https://docs.expo.dev/versions/latest/sdk/notifications/
- 4: https://docs.expo.dev/versions/v57.0.0/sdk/notifications/
- 5: https://docs.expo.dev/versions/unversioned/sdk/notifications
- 6: https://docs.expo.dev/versions/v56.0.0/sdk/notifications/
Handle Android channel-registration failures.
Notifications.setNotificationChannelAsync returns a Promise; since this call is not awaited and has no local .catch, channel errors bypass the existing error handler on line 55. Await the channel setup or attach a local rejection handler for errors such as invalid vibration patterns.
🧰 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)
🤖 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, Update the notification
channel setup around Notifications.setNotificationChannelAsync to await the
Promise or attach a local rejection handler, ensuring Android
channel-registration failures are handled by the existing error-reporting flow.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
npx biome check app/src/lib/usePushNotifications.jsRepository: roshankumar0036singh/Uni-Event
Length of output: 2397
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- files ---\n'
git ls-files | sed -n '/usePushNotifications/p'
printf '\n--- hook file ---\n'
if [ -f app/src/lib/usePushNotifications.js ]; then
nl -ba app/src/lib/usePushNotifications.js | sed -n '1,120p'
fi
printf '\n--- parser probe ---\n'
node <<'JS'
const fs = require('fs');
const source = fs.readFileSync('app/src/lib/usePushNotifications.js', 'utf8');
try {
new Function(source);
console.log('parse: ok');
} catch (e) {
console.log('parse: failed');
console.log(`${e.name}: ${e.message}`);
for (const p of [e.lineNumber, e.column]) {
console.log(`position: ${p}`);
}
}
JSRepository: roshankumar0036singh/Uni-Event
Length of output: 259
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- files ---'
git ls-files | sed -n '/usePushNotifications/p'
printf '%s\n' ''
printf '%s\n' '--- hook file ---'
if [ -f app/src/lib/usePushNotifications.js ]; then
nl -ba app/src/lib/usePushNotifications.js | sed -n '1,120p'
else
printf '%s\n' 'missing app/src/lib/usePushNotifications.js'
fi
printf '%s\n' ''
printf '%s\n' '--- parser probe ---'
node <<'JS'
const fs = require('fs');
const source = fs.readFileSync('app/src/lib/usePushNotifications.js', 'utf8');
try {
new Function(source);
console.log('parse: ok');
} catch (e) {
console.log('parse: failed');
console.log(`${e.name}: ${e.message}`);
for (const p of [e.lineNumber, e.column]) {
console.log(`position: ${p}`);
}
}
JSRepository: roshankumar0036singh/Uni-Event
Length of output: 458
🏁 Script executed:
#!/bin/sh
set -eu
cat -n app/src/lib/usePushNotifications.js | sed -n '1,140p'
printf '%s\n' ''
node --input-type=module <<'JS'
import fs from 'node:fs';
const source = fs.readFileSync('app/src/lib/usePushNotifications.js', 'utf8');
try {
new Function(source);
console.log('parse: ok');
} catch (e) {
console.log('parse: failed');
console.log(`${e.name}: ${e.message}`);
}
JSRepository: roshankumar0036singh/Uni-Event
Length of output: 2395
Fix the broken Promise chain.
Line 55 uses .catch(...) as a standalone statement after registerForPushNotificationsAsync().then(...), so app/src/lib/usePushNotifications.js cannot parse. Attach the catch to the registration Promise before ending the effect.
Proposed fix
useEffect(() => {
registerForPushNotificationsAsync()
.then(token => setExpoPushToken(token))
.catch(err => console.error(err));
}, []);🧰 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)
🤖 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, Fix the Promise chain in the
useEffect registration flow by attaching catch directly to
registerForPushNotificationsAsync().then(...), keeping the error logging
behavior and ending the statement only after the catch handler.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
Bug Fixes
Changes