Skip to content

🧹 [code health improvement] Use centralized error handling in autoNotifications #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TargetMisser wants to merge 2 commits into
base: main
Choose a base branch
from
+31 −33
Open

🧹 [code health improvement] Use centralized error handling in autoNotifications #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d852bf
🧹 refactor: Use centralized error handling in autoNotifications
google-labs-jules[bot] Apr 7, 2026
f806148
🧹 fix: update package-lock.json to resolve npm ci failure
google-labs-jules[bot] Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 24 additions & 27 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions src/utils/autoNotifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
dismissShiftOngoingNotification,
syncShiftOngoingExpiry,
} from './shiftOngoingNotification';
import { handleError } from './errorHandler';

const NOTIF_IDS_KEY = 'aerostaff_notif_ids_v1';
const LAST_SCHEDULE_KEY = 'aerostaff_notif_last_schedule';
Expand Down Expand Up @@ -133,8 +134,8 @@ export async function autoScheduleNotifications(): Promise<number> {
trigger: { type: Notifications.SchedulableTriggerInputTypes.TIME_INTERVAL, seconds: Math.round(secondsUntilNotify), repeats: false },
});
newIds.push(id);
} catch (err) {
if (__DEV__) console.error('Failed to schedule arrival notification:', err);
} catch (err: unknown) {
handleError(err, 'notification', true);
}
Comment on lines +137 to 139
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching from a message-specific console.error('Failed to schedule arrival notification:', err) to handleError(err, 'notification', true) drops the location-specific log message and (with the current handleError implementation) only logs error.message/String(error), which can lose stack traces and useful metadata. Consider passing additional context (e.g., wrap with a new Error including the original error as cause, or extend handleError to accept an optional message/details and log the original error object).

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -186,8 +187,8 @@ export async function autoScheduleNotifications(): Promise<number> {
});
newIds.push(id);
}
} catch (err) {
if (__DEV__) console.error('Failed to schedule departure notification:', err);
} catch (err: unknown) {
handleError(err, 'notification', true);
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: replacing the explicit "Failed to schedule departure notification" logging with handleError(err, 'notification', true) removes the per-operation context and may drop stack traces/metadata. Consider adding an operation-specific message (or wrapping the error with a cause) so production logs remain actionable.

Suggested change
handleError(err, 'notification', true);
const contextualError =
err instanceof Error
? new Error(`Failed to schedule departure notification for ${flightNumber}`, { cause: err })
: new Error(`Failed to schedule departure notification for ${flightNumber}: ${String(err)}`);
handleError(contextualError, 'notification', true);

Copilot uses AI. Check for mistakes.
}
}

Expand All @@ -210,8 +211,8 @@ export async function autoScheduleNotifications(): Promise<number> {
await AsyncStorage.setItem(NOTIF_IDS_KEY, JSON.stringify(newIds));
await AsyncStorage.setItem(LAST_SCHEDULE_KEY, todayKey);
return newIds.length;
} catch (e) {
if (__DEV__) console.error('autoScheduleNotifications error:', e);
} catch (e: unknown) {
handleError(e, 'notification', true);
return 0;
Comment on lines +214 to 216
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description states "no behavior changes", but this catch block used to log only under __DEV__ and now always calls handleError, which currently always console.errors. If production logging is intended, consider updating the PR description; if not intended, gate the call (or add an option to handleError) to preserve the previous dev-only logging behavior.

Copilot uses AI. Check for mistakes.
}
}
Loading