v2 #66

jaspermayone · 2025-11-13T18:08:58Z

No description provided.

jaspermayone · 2025-12-22T19:48:30Z

@Cattn this is good for review

Copilot

Pull request overview

This PR introduces a multi-environment system allowing users to switch between development, staging, and production environments within the calendar extension. The implementation enables environment-specific JWT token management and API base URL configuration.

Key Changes:

Added environment management system with support for dev, staging, and production environments
Refactored API class to use dynamic base URLs based on the selected environment
Implemented environment-aware JWT token storage and migration from legacy single-token format

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
client/src/lib/environment.ts	New file implementing EnvironmentManager class for multi-environment support with JWT token management per environment
client/src/lib/api.ts	Refactored to use async base URLs from EnvironmentManager instead of hardcoded URL
client/src/lib/components/Settings.svelte	Added environment switcher UI and logic for switching between environments
client/src/routes/calendar/+page.svelte	Added environment change listener and data clearing logic for environment switches
client/src/routes/gcalendar/+page.svelte	Added JWT validation with redirect for missing tokens
client/src/routes/loading/+page.svelte	Added JWT token migration call on mount
client/src/routes/feature-access-denied/+page.svelte	New access denied page for feature flag restrictions
client/static/service-worker.js	Extended OAuth success URL patterns to support multiple environments
client/static/manifest.json	Added wildcard host permission for witcc.dev domains
client/src/lib/types.ts	Added feature flag constants and cleaned up interface formatting
client/package-lock.json	Updated dependency peer relationships (removed peer flags)

Files not reviewed (1)

client/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-22T19:52:54Z

client/src/routes/calendar/+page.svelte

    let editDescriptionManual = $state("");
    let editLocationManual = $state("");

+    async function listenforEnvironmentChanges() {


Typo in function name: 'listenforEnvironmentChanges' should be 'listenForEnvironmentChanges' (capital 'F'). Function names should follow camelCase convention with proper capitalization.

Suggested change

async function listenforEnvironmentChanges() {

async function listenForEnvironmentChanges() {

Copilot · 2025-12-22T19:52:54Z

client/src/routes/calendar/+page.svelte

+            storedProcessedData.set([]);
+            storedUserSettings.set(undefined);
+            storedIcsUrl.set(undefined);
+            attemptedTerms = new Set();
+            refreshedTerms = new Set();


Code duplication: The data clearing logic (lines 700-704) is duplicated from lines 672-676 in the environment change listener. Consider extracting this into a separate function like 'clearEnvironmentData()' to avoid duplication and improve maintainability.

Copilot · 2025-12-22T19:52:54Z

client/src/routes/calendar/+page.svelte

+        chrome.storage.onChanged.addListener((changes: any) => {
+            Object.entries(changes).forEach(async ([key]) => {
+                if (key === 'environment_data') {
+                    checkBetaAccess();
+                    jwt_token = await API.getJwtToken();
+                    if (!jwt_token) {
+                        // No JWT token for current environment, redirect to welcome page
+                        goto('/');
+                        return;
+                    }
+
+                    // IMPORTANT: Clear data FIRST before fetching anything for environment switches
+                    if (shouldClearData) {
+                        // Clear stored data to force refetch for new environment
+                        storedProcessedData.set([]);
+                        storedUserSettings.set(undefined);
+                        storedIcsUrl.set(undefined);
+                        attemptedTerms = new Set();
+                        refreshedTerms = new Set();
+                    }
+
+                    // Now fetch fresh data for the current environment
+                    terms = await API.getTerms();
+                    storedUserSettings.set(await API.userSettings());
+                    otherCalUser = checkIsOtherCalendar();
+                }
+            });


Async forEach anti-pattern: Using forEach with an async callback function on line 659 means the async operations won't be properly awaited. The outer listener callback should be async, and Promise.all should be used if parallel execution is desired, or a for-of loop if sequential execution is needed. This could lead to race conditions or unhandled promise rejections.

Copilot · 2025-12-22T19:52:55Z

client/package-lock.json

 			"version": "4.1.16",
 			"resolved": "https://registry.npmjs.org/tailwindcss/-/tailwindcss-4.1.16.tgz",
 			"integrity": "sha512-pONL5awpaQX4LN5eiv7moSiSPd/DLDzKVRJz8Q9PgzmAdd1R4307GQS2ZpfiN7ZmekdQrfhZZiSE5jkLR4WNaA==",
 			"dev": true,


The removal of "peer: true" flags from multiple dependencies in package-lock.json suggests these packages may have been moved from peer dependencies to regular dependencies. This could impact the package installation behavior and bundle size. Ensure this change is intentional and that these packages should indeed be direct dependencies rather than peer dependencies.

Suggested change

"dev": true,

"dev": true,

"peer": true,

Copilot · 2025-12-22T19:52:55Z

client/src/lib/types.ts

    meeting_times: MeetingTime[];
 }

+const FEATUE_FLAGS = [


Typo in constant name: 'FEATUE_FLAGS' should be 'FEATURE_FLAGS'. This spelling error makes the constant name inconsistent with its purpose and the feature flag functionality referenced elsewhere in the codebase.

Suggested change

const FEATUE_FLAGS = [

const FEATURE_FLAGS = [

Copilot · 2025-12-22T19:52:56Z

client/src/routes/calendar/+page.svelte

+                    if (shouldClearData) {
+                        // Clear stored data to force refetch for new environment
+                        storedProcessedData.set([]);
+                        storedUserSettings.set(undefined);
+                        storedIcsUrl.set(undefined);
+                        attemptedTerms = new Set();
+                        refreshedTerms = new Set();
+                    }


The variable 'shouldClearData' is used on line 670 within the storage change listener, but it's defined at the module level based on sessionStorage values that were already read and cleared earlier in the code (lines 638-646). This means the condition on line 670 will always be false when the listener fires, making the data clearing logic ineffective for environment changes detected via the storage listener.

Suggested change

if (shouldClearData) {

// Clear stored data to force refetch for new environment

storedProcessedData.set([]);

storedUserSettings.set(undefined);

storedIcsUrl.set(undefined);

attemptedTerms = new Set();

refreshedTerms = new Set();

}

// Clear stored data to force refetch for new environment

storedProcessedData.set([]);

storedUserSettings.set(undefined);

storedIcsUrl.set(undefined);

attemptedTerms = new Set();

refreshedTerms = new Set();

Copilot · 2025-12-22T19:52:56Z

client/src/lib/environment.ts

+    public static async migrateOldJwtToken(): Promise<void> {
+        const result = await chrome.storage.local.get('jwt_token');
+        if (result.jwt_token) {
+            await this.setJwtToken(result.jwt_token, 'dev');


The migration function assumes old JWT tokens belong to the 'dev' environment, but this may not be accurate for users who were on production. This could cause users to lose their production authentication and be forced to re-authenticate. Consider adding a mechanism to detect which environment the old token belongs to, or default to 'prod' instead of 'dev' since that's likely the more common case.

Suggested change

await this.setJwtToken(result.jwt_token, 'dev');

const environmentData = await this.getEnvironmentData();

const targetEnvironment: Environment = environmentData.current_environment || 'prod';

await this.setJwtToken(result.jwt_token, targetEnvironment);

Copilot · 2025-12-22T19:52:57Z

client/src/lib/components/Settings.svelte

+        if (!hasJwt) {
+            storedProcessedData.set([]);
+            snackbar(`Switched to ${envDisplayName}. Please sign in.`, undefined, true);
+            await goto('/');


Missing navigation when switching environments with JWT token: When a JWT token exists for the new environment (hasJwt is true), the function returns without navigating or reloading, leaving the user on the settings page with stale data. The sessionStorage flags are set but won't be read until the user manually navigates. Consider adding navigation to the calendar page or triggering a page reload when hasJwt is true.

Suggested change

await goto('/');

await goto('/');

} else {

// When a JWT exists for the new environment, reload so the app

// can read the sessionStorage flags and refresh data accordingly.

snackbar(`Switched to ${envDisplayName}.`, undefined, true);

if (browser) {

window.location.reload();

}

Copilot · 2025-12-22T19:52:57Z

client/src/lib/components/Settings.svelte

+                            const { color, ...meetingWithoutColor } = meeting;
+                            return meetingWithoutColor;


The immutability pattern used here (creating new objects with spread operator) is good for reactivity, but the color property is being removed without proper type handling. The destructuring '{ color, ...meetingWithoutColor }' assumes color exists, but TypeScript may not allow returning an object without the color property if it's a required field in the MeetingTime type. Consider explicitly typing the return value or ensuring color is optional in the type definition.

Suggested change

const { color, ...meetingWithoutColor } = meeting;

return meetingWithoutColor;

return { ...meeting, color: undefined };

Copilot · 2025-12-22T19:52:57Z

client/src/lib/types.ts

+    type Course, type CurrentTerm, type DayItem,
+    type EventPreferences, type GetPreferencesResponse, type isProcessed, type Location,
+    type MeetingTime, type NextTerm, type NotificationMethod, type NotificationSetting, type NotificationType, type Preview, type ProcessedEvents, type Professor, type ReminderSettings, type ResolvedData, type ResponseData, type TemplateVariables, type Term, type TermResponse, type UserSettings


Poor code formatting: The export statement is poorly formatted with inconsistent type grouping on a single line, making it difficult to read and maintain. Consider formatting each export on its own line or using consistent grouping for better readability.

Suggested change

type Course, type CurrentTerm, type DayItem,

type EventPreferences, type GetPreferencesResponse, type isProcessed, type Location,

type MeetingTime, type NextTerm, type NotificationMethod, type NotificationSetting, type NotificationType, type Preview, type ProcessedEvents, type Professor, type ReminderSettings, type ResolvedData, type ResponseData, type TemplateVariables, type Term, type TermResponse, type UserSettings

type Course,

type CurrentTerm,

type DayItem,

type EventPreferences,

type GetPreferencesResponse,

type isProcessed,

type Location,

type MeetingTime,

type NextTerm,

type NotificationMethod,

type NotificationSetting,

type NotificationType,

type Preview,

type ProcessedEvents,

type Professor,

type ReminderSettings,

type ResolvedData,

type ResponseData,

type TemplateVariables,

type Term,

type TermResponse,

type UserSettings,

jaspermayone and others added 2 commits November 13, 2025 12:59

ground work for feature flag checking

e90eab9

feat: env switcher

0ea1219

jaspermayone self-assigned this Nov 13, 2025

add env listener

44653e4

Cattn marked this pull request as ready for review November 25, 2025 17:53

Cattn self-requested a review as a code owner November 25, 2025 17:53

Cattn marked this pull request as draft November 25, 2025 17:53

jaspermayone marked this pull request as ready for review December 22, 2025 19:48

Copilot AI review requested due to automatic review settings December 22, 2025 19:48

jaspermayone changed the title ~~[draft] v2~~ v2 Dec 22, 2025

Copilot started reviewing on behalf of jaspermayone December 22, 2025 19:48 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

	async function listenforEnvironmentChanges() {
	async function listenForEnvironmentChanges() {

-            await this.setJwtToken(result.jwt_token, 'dev');
+            const environmentData = await this.getEnvironmentData();
+            const targetEnvironment: Environment = environmentData.current_environment || 'prod';
+            await this.setJwtToken(result.jwt_token, targetEnvironment);

-            await goto('/');
+            await goto('/');
+        } else {
+            // When a JWT exists for the new environment, reload so the app
+            // can read the sessionStorage flags and refresh data accordingly.
+            snackbar(`Switched to ${envDisplayName}.`, undefined, true);
+            if (browser) {
+                window.location.reload();
+            }

		const { color, ...meetingWithoutColor } = meeting;
		return meetingWithoutColor;

	const { color, ...meetingWithoutColor } = meeting;
	return meetingWithoutColor;
	return { ...meeting, color: undefined };

v2 #66

Are you sure you want to change the base?

v2 #66

Uh oh!

Conversation

jaspermayone commented Nov 13, 2025

Uh oh!

jaspermayone commented Dec 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants