-
Notifications
You must be signed in to change notification settings - Fork 0
jsp working updates #83
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.
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
base: main
Are you sure you want to change the base?
Conversation
Backend API compatibility: - Add pub_id to Building, Professor, Term, CurrentTerm, NextTerm - Add start_date and end_date to CurrentTerm and NextTerm - Update MeetingTime.id to accept string (public_id) or number - Add CalendarConfig interface for resolved calendar preferences University events support: - Add sync_university_events and university_event_categories to UserSettings - Add available_university_event_categories for settings UI - Add UniversityEventCategory interface - Add UniversityEventCategoryWithCount interface - Add UniversityCalendarEvent interface for event data
New methods: - getUniversityEventCategories(): Get all categories with event counts - getUniversityEvents(): Get events with filters (category, dates, term, pagination) - getHolidays(): Get holiday events with optional term/date filters All methods use JWT authentication and return properly typed responses.
Features: - Add "Sync University Events" toggle switch - Show category selection when enabled (academic, campus_event, meeting, exhibit, announcement) - Display category descriptions to help users understand each type - Note that holidays are always synced (no opt-in needed) - Changes trigger calendar sync automatically via backend
- Add processCourses for initial course schedule processing - Add reprocessCourses for refreshing schedules (handles dropped/added courses) - Add getMeetingTimePreference, updateMeetingTimePreference, deleteMeetingTimePreference - Add getUniversityEventCategories, getUniversityEvents, getHolidays
- Add refreshSchedule function to detect dropped/added courses - Add Refresh button next to term selector - Replace all raw fetch() calls with API class methods - Fix map type to support number | string meeting time IDs
|
this closes #77 |
Always display 'Copy Calendar Link' button regardless of initial calendar choice during onboarding. This allows Google Calendar users to also access the ICS subscription URL. - Remove otherCalUser conditional around Copy Calendar Link button - Remove checkIsOtherCalendar function and related state - Update text to be more generic for all users Closes #68
Settings improvements: - Add 'Disable All Notifications' toggle to turn off all calendar reminders - Add 'Connected Google Accounts' section to manage multiple Google accounts - Users can add additional Google accounts and sync calendar to all of them - Users can disconnect accounts (must keep at least one) API additions: - getGlobalCalendarPreference/setGlobalCalendarPreference for notification settings - getConnectedAccounts to list OAuth credentials - requestOAuthForEmail to initiate OAuth for new email - disconnectAccount to remove OAuth credential Closes #62 Closes #12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This work-in-progress PR refactors API interactions by centralizing fetch calls into reusable API methods and adds several new features including schedule refresh functionality, Google account management, and university event synchronization settings. The changes modernize the codebase by removing inline fetch calls and improving code organization.
- Refactored inline fetch calls to use centralized API methods in
api.ts - Added schedule refresh functionality with LeopardWeb scraping
- Implemented Google account connection/disconnection management in Settings
- Added university calendar event sync configuration with category selection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| client/src/lib/types.ts | Added new type definitions for calendar config, university events, connected accounts, and updated existing types to support string IDs |
| client/src/lib/api.ts | Centralized API methods for course processing, event preferences, global calendar settings, and OAuth account management |
| client/src/lib/components/Settings.svelte | Added UI for managing Google account connections, notification toggles, and university event sync preferences |
| client/src/routes/calendar/+page.svelte | Refactored to use new API methods, added schedule refresh button, removed unused otherCalUser logic, improved calendar link UI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static async processCourses(courses: any[]): Promise<{ user_pub: string; ics_url: string }> { | ||
| const baseUrl = await this.getBaseUrl(); | ||
| const token = await this.getJwtToken(); | ||
| const response = await fetch(`${baseUrl}/process_courses`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Authorization': `Bearer ${token}`, | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify(courses) | ||
| }); | ||
| return response.json(); | ||
| } |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The processCourses method doesn't check if the response is successful before attempting to parse JSON. If the fetch fails with a non-2xx status code, calling .json() could throw an error or return unexpected data. Consider adding response status validation similar to the pattern used in the old inline fetch code.
| headers: { | ||
| 'Authorization': `Bearer ${token}` | ||
| } | ||
| }); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getMeetingTimePreference method doesn't check the response status. If the request fails (e.g., 404 for non-existent meeting time), calling .json() on the error response could produce unexpected results or throw an error that isn't properly handled.
| }); | |
| }); | |
| if (!response.ok) { | |
| let errorBody: string | undefined; | |
| try { | |
| errorBody = await response.text(); | |
| } catch { | |
| // Ignore body parsing errors; we'll fall back to status text. | |
| } | |
| const message = `Failed to fetch meeting time preference (status ${response.status}): ` + | |
| (errorBody && errorBody.trim().length > 0 ? errorBody : response.statusText); | |
| throw new Error(message); | |
| } |
| const map = new Map<number | string, GetPreferencesResponse>(); | ||
| for (const r of responses) { | ||
| if (r?.id && r.data) map.set(r.id, r.data); | ||
| if (r?.id !== undefined && r.data) map.set(r.id, r.data); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition check r?.id !== undefined is inconsistent with the previous pattern. In the old code (line removed), it was r?.id && r.data, which would reject falsy id values like 0. However, since id can now be number | string, checking !== undefined is more correct. But this means if an id is 0 or an empty string, it will be included in the map, which might not be intended. Consider whether these edge cases need explicit handling.
| if (r?.id !== undefined && r.data) map.set(r.id, r.data); | |
| if (r && r.data && r.id !== undefined && r.id !== 0 && r.id !== '') { | |
| map.set(r.id, r.data); | |
| } |
| let tabToUse: any; | ||
| let shouldCloseTab = false; | ||
| const targetUrl = 'https://selfservice.wit.edu/StudentRegistrationSsb/ssb/registrationHistory/registrationHistory'; | ||
| const [currentTab] = await chrome.tabs.query({ | ||
| active: true, | ||
| currentWindow: true | ||
| }); | ||
| const isOnTargetPage = currentTab.url === targetUrl; | ||
| tabToUse = currentTab; | ||
| if (!isOnTargetPage) { | ||
| tabToUse = await chrome.tabs.create({ url: targetUrl }); | ||
| shouldCloseTab = true; | ||
| await new Promise<void>((resolve) => { | ||
| const listener = (tabId: number, changeInfo: any) => { | ||
| if (tabId === tabToUse.id && changeInfo.status === 'complete') { | ||
| chrome.tabs.onUpdated.removeListener(listener); | ||
| resolve(); | ||
| } | ||
| }; | ||
| chrome.tabs.onUpdated.addListener(listener); | ||
| }); | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| } | ||
| if (!tabToUse?.id) { | ||
| snackbar('Failed to open LeopardWeb tab', undefined, true); | ||
| return; | ||
| } | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tabToUse.id }, | ||
| world: 'MAIN', | ||
| func: async (term: string) => { | ||
| try { | ||
| const r0 = await fetch(`https://selfservice.wit.edu/StudentRegistrationSsb/ssb/registrationHistory/reset?term=${term}`, { | ||
| credentials: 'include' | ||
| }); | ||
| await r0.json(); | ||
| const r1 = await fetch('https://selfservice.wit.edu/StudentRegistrationSsb/ssb/classRegistration/getRegistrationEvents?termFilter=', { | ||
| credentials: 'include' | ||
| }); | ||
| return await r1.json(); | ||
| } catch (e) { | ||
| return ({ error: (e as Error).message }); | ||
| } | ||
| }, | ||
| args: [termId] | ||
| }); | ||
| if (shouldCloseTab && tabToUse?.id) { | ||
| await chrome.tabs.remove(tabToUse.id); | ||
| } |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refreshSchedule function contains extensive tab management and scraping logic (lines 448-503) that is duplicated from the fetchFromCurrentPage function. This creates significant code duplication and maintenance burden. Consider extracting the shared tab management and scraping logic into a reusable helper function.
| const pollTimer = setInterval(async () => { | ||
| if (popup?.closed) { | ||
| clearInterval(pollTimer); | ||
| // Refresh connected accounts | ||
| try { | ||
| const accounts = await API.getConnectedAccounts(); | ||
| connectedAccounts = accounts.oauth_credentials || []; | ||
| snackbar('Account connected successfully!', undefined, true); | ||
| } catch (e) { | ||
| console.error('Failed to refresh accounts:', e); | ||
| } | ||
| addEmailInput = ""; | ||
| } | ||
| }, 500); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addGoogleAccount function opens an OAuth popup and polls every 500ms to detect when it closes. However, there's no timeout mechanism. If the popup is never closed or the user navigates away, the polling interval will continue indefinitely, causing a memory leak. Consider adding a timeout and cleanup logic.
| snackbar('Failed to save event preferences: ' + put.statusText, undefined, true); | ||
| } else { | ||
| try { | ||
| await API.updateMeetingTimePreference(activeMeeting?.id!, payload); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the non-null assertion operator (!) on activeMeeting?.id is unsafe. If activeMeeting is undefined or if id is undefined, this will pass undefined to the API method, potentially causing issues. Consider adding proper validation before calling the API method.
| await API.updateMeetingTimePreference(activeMeeting?.id!, payload); | |
| const meetingIdForUpdate = activeMeeting?.id; | |
| if (!meetingIdForUpdate) { | |
| snackbar('Unable to save event preferences: no meeting selected.', undefined, true); | |
| return; | |
| } | |
| await API.updateMeetingTimePreference(meetingIdForUpdate, payload); |
| } catch (e) { | ||
| activeCourse = undefined; | ||
| activeMeeting = undefined; | ||
| activeDay = undefined; | ||
| currentEventPrefs = undefined; | ||
| editTitle = ""; | ||
| editDescription = ""; | ||
| editLocation = ""; | ||
| editTitleManual = ""; | ||
| editDescriptionManual = ""; | ||
| editLocationManual = ""; | ||
| courseColor = "#d50000"; | ||
| notifications = []; | ||
| editMode = false; | ||
| snackbar('Failed to save event preferences: ' + e, undefined, true); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handler in the catch block resets all state variables, which duplicates the exact same logic from the success path (lines 680-688). This creates code duplication and makes the code harder to maintain. Consider extracting this reset logic into a separate function.
| courseColor = "#d50000"; | ||
| notifications = []; | ||
| editMode = false; | ||
| snackbar('Failed to save event preferences: ' + e, undefined, true); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message concatenates the error object directly: 'Failed to save event preferences: ' + e. When e is an Error object, this will result in "[object Object]" being displayed to the user instead of a meaningful error message. Use e.message or convert the error to a string properly.
| let editDescriptionManual = $state(""); | ||
| let editLocationManual = $state(""); | ||
| let refreshing = $state(false); | ||
| let lastRefreshResult = $state<{ removed: number; removedCourses: Array<{ crn: number; title: string; course_number: number }> } | null>(null); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable lastRefreshResult is defined but never appears to be used in the visible UI template. If this is intended for displaying refresh results to the user, the UI code seems to be missing. Otherwise, this variable may be unnecessary.
| let lastRefreshResult = $state<{ removed: number; removedCourses: Array<{ crn: number; title: string; course_number: number }> } | null>(null); |
| async function addGoogleAccount() { | ||
| if (!addEmailInput.trim()) { | ||
| snackbar('Please enter an email address', undefined, true); | ||
| return; | ||
| } | ||
| try { | ||
| const response = await API.requestOAuthForEmail(addEmailInput.trim()); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The email input field lacks proper email validation. While the input type is set to "email", the addGoogleAccount function only checks if the input is not empty (.trim()). The browser's built-in email validation can be bypassed. Consider adding explicit email format validation before making the API call to provide better user feedback.
| async function addGoogleAccount() { | |
| if (!addEmailInput.trim()) { | |
| snackbar('Please enter an email address', undefined, true); | |
| return; | |
| } | |
| try { | |
| const response = await API.requestOAuthForEmail(addEmailInput.trim()); | |
| function isValidEmail(email: string): boolean { | |
| // Basic email pattern to validate format before making API calls | |
| const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| return emailPattern.test(email); | |
| } | |
| async function addGoogleAccount() { | |
| const trimmedEmail = addEmailInput.trim(); | |
| if (!trimmedEmail) { | |
| snackbar('Please enter an email address', undefined, true); | |
| return; | |
| } | |
| if (!isValidEmail(trimmedEmail)) { | |
| snackbar('Please enter a valid email address', undefined, true); | |
| return; | |
| } | |
| try { | |
| const response = await API.requestOAuthForEmail(trimmedEmail); |
|
description wip_