-
Notifications
You must be signed in to change notification settings - Fork 5
Implement calendar changes #293
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: dev
Are you sure you want to change the base?
Conversation
Removed guide and analytics options from settings.
Removed console log from isAuthenticated method.
Removed unnecessary console logs during loading of persisted stores.
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 PR implements a significant refactoring of the calendar functionality along with various configuration and authentication updates. The changes are marked as "Not prod-ready" and are opened for discussion, which is appropriate given the scope of modifications.
Key Changes:
- Complete calendar UI/UX overhaul with new grid-based month view and modular component architecture
- Authentication domain migration from
aristomate.grtoaristomate.auth.grwith OAuth scope changes - Metro scraper switched from API-based to direct web scraping approach
- Code formatting standardization changes (tabs to spaces)
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
src/routes/pages/calendar/+page.svelte |
Major refactoring: replaced swipe-based day navigation with calendar grid view, removed exam fetching, restructured event management |
src/lib/components/calendar/CalendarGrid.svelte |
New component implementing month-view calendar grid with day selection |
src/lib/components/calendar/calendarUtils.ts |
New utility functions for calendar week building and date navigation |
src/lib/components/calendar/calendarState.ts |
New (unused) calendar state management module |
src/lib/components/calendar/event/EventModal.svelte |
New modal component extracted from main calendar page for event creation/editing |
src/lib/components/calendar/event/EventCard.svelte |
UI improvements with better styling, spacing fixes, and conditional description rendering |
src/lib/authentication/OIDCClient.ts |
Modified state parameter handling and simplified refresh token validation logic |
src/app.config.ts |
Domain changes, added offline_access scope, removed metro API config, added platform detection flags |
src/lib/storage/capacitorPersistedStore.ts |
Added explicit Preferences writes in set/update methods |
src/routes/persistedStoreDeclarations.ts |
Minor formatting change and unused import added |
src/lib/metroScraper/scraper.ts |
Switched from API consumption to direct web scraping |
src/lib/components/map/map.svelte |
Fixed import typo, formatting improvements |
src/routes/pages/homepage/homepage.svelte |
Added platform-conditional webmail login visibility |
src/lib/components/webmailLogin/* |
Added platform-conditional rendering for Android/development only |
src/lib/components/personalInfo/settings.svelte |
Removed guide and analytics menu items |
src/lib/translations/translations.js |
Added metro error messages and new calendar no-events message |
android/app/src/main/AndroidManifest.xml |
Added intent filter for new aristomate.auth.gr domain |
src/app.html |
Added Cloudflare Web Analytics scripts (two different tokens) |
.prettierrc |
Changed from tabs to spaces with 4-space indentation |
Comments suppressed due to low confidence (1)
src/routes/persistedStoreDeclarations.ts:10
- Unused import get.
import { get } from "svelte/store";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {#if eventItem.slot.end && new Date(eventItem.slot.end).getTime() != new Date(eventItem.slot.start).getTime()} | ||
| {#if new Date(eventItem.slot.end).getDay() - new Date(eventItem.slot.start).getDay() != 0} | ||
|  -* | ||
| -* |
Copilot
AI
Dec 4, 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.
[nitpick] Inconsistent non-breaking space usage. Line 70 uses -* while line 72 uses - with spaces around the dash. For consistency, consider using on both sides of the dash in both cases.
| -* | |
| - * |
| return Array.from({ length: 7 }, (_, i) => { | ||
| const date = new Date(2024, 0, i); |
Copilot
AI
Dec 4, 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.
[nitpick] Hardcoded year in weekday name generation. The function uses new Date(2024, 0, i) to generate weekday names. This will work correctly, but consider using a dynamic approach or adding a comment explaining why 2024 is used (likely just as a reference year for getting weekday names).
| return Array.from({ length: 7 }, (_, i) => { | |
| const date = new Date(2024, 0, i); | |
| // Use current year to avoid hardcoding and ensure future-proofing | |
| const currentYear = new Date().getFullYear(); | |
| return Array.from({ length: 7 }, (_, i) => { | |
| const date = new Date(currentYear, 0, i); |
| <link rel="icon" href="%sveltekit.assets%/favicon.png" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <!-- Cloudflare Web Analytics --><script defer src='https://static.cloudflareinsights.com/beacon.min.js' data-cf-beacon='{"token": "9a78acf6d3c44e1ba7b4805affdc9c57"}'></script><!-- End Cloudflare Web Analytics --> | ||
| <!-- Cloudflare Web Analytics --><script defer src='https://static.cloudflareinsights.com/beacon.min.js' data-cf-beacon='{"token": "aa5ba1d501724ca1af72ab16c6a2af1a"}'></script><!-- End Cloudflare Web Analytics --> |
Copilot
AI
Dec 4, 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.
Duplicate Cloudflare Web Analytics scripts. Two different analytics scripts with different tokens are loaded on the same page. This could lead to duplicate tracking or conflicts. Consider consolidating to a single token or explaining why multiple tokens are needed.
| <!-- Cloudflare Web Analytics --><script defer src='https://static.cloudflareinsights.com/beacon.min.js' data-cf-beacon='{"token": "aa5ba1d501724ca1af72ab16c6a2af1a"}'></script><!-- End Cloudflare Web Analytics --> |
| // const state = this.generateRandomString(); | ||
| const state = encodeURIComponent(btoa(`{"production":${this.config.isProduction}}`)); |
Copilot
AI
Dec 4, 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.
Hardcoded production state in authorization URL. The state parameter now encodes the production flag as {"production":${this.config.isProduction}}. This is unusual - the state parameter is typically used for CSRF protection and should be a random value. Encoding application state here could be a security concern. If you need to pass the production flag, consider using a separate custom parameter or including it in the redirect URI.
| setTimeout(() => { | ||
| selectedDay = { day: dayObj.day, month, year }; | ||
| activeDate = new Date(year, month, dayObj.day); | ||
| }, 0); |
Copilot
AI
Dec 4, 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.
[nitpick] Potential navigation timing issue. When selecting a day from a different month (lines 60-65), the month is changed and then a setTimeout with 0ms delay is used to set the selectedDay. This setTimeout is likely used to ensure the calendar rebuilds before selecting the day, but it's fragile. Consider using a reactive statement or ensuring the selection happens after the calendar rebuild in a more deterministic way.
| import { scheduledNotifications } from "$lib/calendarNotifications/notificationsStore"; | ||
| import { loginStore } from "$src/lib/authentication/loginStore"; | ||
| import { userCredsFlag } from "$components/webmailLogin/userCredsFlagStore"; | ||
| import { get } from "svelte/store"; |
Copilot
AI
Dec 4, 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.
Unused import. The get function from 'svelte/store' is imported but not used anywhere in this file.
| import { get } from "svelte/store"; |
| console.log('Loading persisted stores'); | ||
| persistedStores.forEach(async (store) => await store.loadFromStorage()); | ||
| } | ||
| persistedStores.forEach(async (store) => { await store.loadFromStorage();}); |
Copilot
AI
Dec 4, 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.
Potential issue with async forEach. Using forEach with async callbacks doesn't wait for promises to complete. The function returns before all stores finish loading. Consider using Promise.all() with map() instead:
await Promise.all(persistedStores.map(store => store.loadFromStorage()));| persistedStores.forEach(async (store) => { await store.loadFromStorage();}); | |
| await Promise.all(persistedStores.map(store => store.loadFromStorage())); |
| redirectUri: 'https://applink.aristomate.auth.gr/authsso/callback', | ||
| scope: 'students:read offline_access', | ||
| isMobile: isMobile, | ||
| // Use proxy for web | ||
| tokenUrl: isMobile ? undefined : 'https://applink.aristomate.gr/api/auth/token' | ||
| tokenUrl: isMobile ? undefined : 'https://applink.aristomate.auth.gr/api/auth/token', |
Copilot
AI
Dec 4, 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.
Domain name discrepancy. The redirect URI and token URL domains have changed from applink.aristomate.gr to applink.aristomate.auth.gr. Ensure that:
- The new domain is properly configured and accessible
- OAuth provider is configured to accept this new redirect URI
- SSL certificates are valid for the new domain
| "useTabs": false, | ||
| "tabWidth": 4, |
Copilot
AI
Dec 4, 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.
[nitpick] Significant formatting change. Switching from tabs to spaces (with 4-space indentation) is a major change that will affect the entire codebase formatting. This should be applied consistently across all files, ideally in a separate commit/PR dedicated to formatting changes only. Mixing formatting changes with functional changes makes code review more difficult.
| @@ -0,0 +1,44 @@ | |||
| import { writable, derived } from 'svelte/store'; | |||
Copilot
AI
Dec 4, 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.
Unused import derived.
| import { writable, derived } from 'svelte/store'; | |
| import { writable } from 'svelte/store'; |
Not prod-ready. Opened PR for further discussion