Code cleanup by Ascent817 · Pull Request #158 · magnifydev/magnify

Ascent817 · 2026-02-20T00:29:44Z

Description

Cleaned up code that modifies the DOM imperatively rather than using React best practices. Also reduced code duplication and used map to generate tag filters rather than the 12 hardcoded ones we had before.

github-actions · 2026-02-20T00:31:22Z

Visit the preview URL for this PR (updated for commit e320c92):

https://magnify--pr158-code-cleanup-tz5nhp0a.web.app

_{(expires Mon, 02 Mar 2026 00:53:11 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 9d7af0ca9f95ffb1f7aaef8a25560cf9e42173b6}

Copilot

Pull request overview

This PR refactors the codebase to eliminate imperative DOM manipulation and embrace React best practices. The changes centralize application logic in App.tsx with proper state management, extract Firebase configuration to a dedicated module, and use the modern createRoot API for rendering.

Changes:

Migrated from imperative DOM manipulation to React state management for search, tag filtering, and user authentication
Consolidated Firebase initialization into a single config module and removed redundant initialization code
Refactored TagBar component to use map() instead of 16 hardcoded tag buttons

Reviewed changes

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

Show a summary per file

File	Description
src/index.tsx	Simplified to use React 18's createRoot API and removed all business logic
src/App.tsx	Now manages all application state (courseData, user, authLevel, search, tags) with React hooks instead of global variables and DOM manipulation
src/components/TopBar.tsx	Converted to controlled component with searchQuery state and event handlers passed as props
src/components/TagBar.tsx	Refactored to generate tag buttons via map() and receive state/handlers as props
src/components/ClearFilter.tsx	Updated to use props for callbacks and useEffect for side effects
src/components/Course.tsx	Simplified to use centralized Firebase db export instead of initializing Firebase locally
src/config/firebase.ts	Centralized Firebase initialization and exports auth, db, and provider instances
src/config/index.ts	Added exports for auth, db, and provider
src/utils/getCookieValue.ts	New utility extracted for cookie parsing
src/utils/index.ts	Added export for getCookieValue utility

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

Copilot · 2026-02-20T18:15:17Z

src/App.tsx

+  const courseIDtoNameMap = useMemo(() => {
+    const map = new Map<string, string>();
+    for (const courseName in courseData) {
+      courseData![courseName].courseid.match(COURSE_ID_REGEX)?.forEach((id) => {
+        map.set(id, courseName);
+      });
+    }
+    return map;
+  }, [courseData]);


The courseIDtoNameMap computation will crash when courseData is null. The useMemo checks if courseData exists in the loop condition but uses the non-null assertion operator, which can cause a runtime error if courseData is null during the initial render before the useEffect sets it.

Add a guard check at the beginning of the useMemo: if (!courseData) return new Map();

@Ascent817 is coursedata gonna ever be null, don't we load in the pre-downloaded coursedata json regardless

Yeah, we should always have local to fall back on. We should make sure to update the local version when we update the db.

Copilot · 2026-02-20T18:15:17Z

src/App.tsx

+          snapshot.val().users;
+        Object.values(users).forEach(({ email, level }) => {
+          if (user.email === email) setAuthLevel(level);
+        });


The auth level fetch effect lacks error handling. If the database read fails, the error will be unhandled. Add a .catch() handler to the promise chain to handle potential database errors gracefully.

Suggested change

});

});

})

.catch((error) => {

console.error('Failed to fetch auth level:', error);

Copilot · 2026-02-20T18:15:17Z

src/components/ClearFilter.tsx

+  useEffect(() => {
+    const courseContainer = document.getElementById(
+      'course-container'
+    ) as HTMLDivElement;

-  const clearResults = () => {
-    const search = document.getElementById('searchbar') as HTMLInputElement;
-    search.value = '';
-
-    const tags = document.getElementsByClassName('tag');
-    for (let i = 0; i < tags.length; i++) {
-      if (tags[i].classList.contains('tag-true')) {
-        tags[i].classList.remove('tag-true');
-      }
-    }
+    const adjustHeight = () => {
+      courseContainer.style.height =
+        getWidth() >= 525
+          ? 'calc(100vh - 180px)'
+          : 'calc(100vh - (45.5px + 65.5px + (clamp(15px, 5vw, 20px) * 2)))';
+    };

-    filterCourses();
-  };
+    adjustHeight();
+    window.addEventListener('resize', adjustHeight);
+    return () => {
+      window.removeEventListener('resize', adjustHeight);
+      courseContainer.style.height = '';
+    };


The ClearFilter component directly manipulates the DOM to set the height of the course-container element. This goes against the PR's stated goal of eliminating imperative DOM manipulation in favor of React best practices. Consider managing the container height through React state and inline styles on the container div in App.tsx, similar to how display and justifyContent are already handled on lines 262-266.

@Ascent817 well well well

well this could be fixed if we used CSS subgrid

but thats a LOT of work

Copilot · 2026-02-20T18:15:18Z

src/App.tsx

+  useEffect(() => {
+    let prevScrollPos = window.scrollY;
+    const handleScroll = () => {
+      const topButton = document.getElementById('to-top');
+      if (topButton) {
+        const visible = window.scrollY >= 80 && windowWidth >= 525;
+        topButton.style.visibility = visible ? 'visible' : 'hidden';
+        topButton.style.opacity = visible ? '1' : '0';
+      }
+      if (windowWidth < 525) {
+        const nav = document.getElementById('mobile-nav');
+        if (nav) {
+          nav.style.bottom =
+            window.scrollY < prevScrollPos ? '0' : `-${nav.offsetHeight}px`;
+        }
+      }
+      prevScrollPos = window.scrollY;
+    };
+    window.addEventListener('scroll', handleScroll);
+    return () => window.removeEventListener('scroll', handleScroll);
+  }, [windowWidth]);


The scroll handler in App.tsx continues to use imperative DOM manipulation to control the visibility of the back-to-top button and mobile navigation. This is inconsistent with the PR's stated goal of using React best practices instead of imperative DOM manipulation. Consider refactoring to use React state to track scroll position and control visibility through conditional rendering or className props.

Copilot · 2026-02-20T18:15:18Z

src/App.tsx

-    );
+    ));
+    return <div className="parent">{flexParents}</div>;
+  }, [filteredCourseItems, numColumns, handleClearFilters]);


The classItems useMemo includes handleClearFilters in its dependency array, but handleClearFilters is stable (has no dependencies). This can cause unnecessary re-computation of the classItems layout when it doesn't need to change. However, since handleClearFilters is wrapped in useCallback with an empty dependency array, this is actually fine. Consider documenting this relationship or removing it from dependencies if handleClearFilters is guaranteed to be stable.

Suggested change

}, [filteredCourseItems, numColumns, handleClearFilters]);

}, [filteredCourseItems, numColumns]);

Copilot · 2026-02-20T18:15:18Z

src/App.tsx

+        const signedInUser = result.user;
+        if (!signedInUser) return;
+        setUser(signedInUser);
+        document.cookie = `user=${JSON.stringify(signedInUser)}`;


Storing the entire Firebase user object in a cookie is a security concern. The user object may contain sensitive information and tokens that should not be stored in cookies, which are vulnerable to XSS attacks and can be transmitted with every HTTP request. Consider using Firebase's built-in session persistence or storing only minimal, non-sensitive user identifiers in cookies (like UID), and relying on Firebase Auth's persistence mechanisms instead.

Suggested change

document.cookie = `user=${JSON.stringify(signedInUser)}`;

document.cookie = `user=${encodeURIComponent(signedInUser.uid)}`;

Copilot · 2026-02-20T18:15:19Z

src/App.tsx

+  const [user, setUser] = useState<firebase.User | null>(() => {
+    const cookie = getCookieValue('user');
+    if (!cookie) return null;
+    try {
+      return JSON.parse(cookie);
+    } catch {
+      return null;
+    }
+  });


The user state initialization from cookies lacks validation. If the cookie contains a malformed or malicious JSON payload that successfully parses but doesn't match the expected Firebase User structure, it could cause runtime errors when the app tries to access user properties like user.email or user.photoURL. Consider adding type validation or using a schema validator to ensure the parsed object conforms to the expected Firebase User interface.

Copilot · 2026-02-20T18:15:19Z

src/App.tsx

+      auth.signOut().then(() => {
+        setUser(null);
+        setAuthLevel(0);
+        document.cookie =
+          'user=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/;';
+      });
+    } else {
+      auth.signInWithPopup(provider).then((result) => {
+        const signedInUser = result.user;
+        if (!signedInUser) return;
+        setUser(signedInUser);
+        document.cookie = `user=${JSON.stringify(signedInUser)}`;
+      });


The authentication sign-in and sign-out promises lack error handling. If the Firebase authentication fails (network issues, popup blocked, etc.), the promise rejection will be unhandled, potentially causing user-facing errors or leaving the UI in an inconsistent state. Add .catch() handlers to both auth.signOut() and auth.signInWithPopup(provider) calls to handle errors gracefully.

Suggested change

auth.signOut().then(() => {

setUser(null);

setAuthLevel(0);

document.cookie =

'user=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/;';

});

} else {

auth.signInWithPopup(provider).then((result) => {

const signedInUser = result.user;

if (!signedInUser) return;

setUser(signedInUser);

document.cookie = `user=${JSON.stringify(signedInUser)}`;

});

auth

.signOut()

.then(() => {

setUser(null);

setAuthLevel(0);

document.cookie =

'user=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/;';

})

.catch((error) => {

console.error('Error signing out:', error);

});

} else {

auth

.signInWithPopup(provider)

.then((result) => {

const signedInUser = result.user;

if (!signedInUser) return;

setUser(signedInUser);

document.cookie = `user=${JSON.stringify(signedInUser)}`;

})

.catch((error) => {

console.error('Error signing in:', error);

});

Copilot · 2026-02-20T18:15:19Z

src/App.tsx

@@ -34,26 +211,62 @@
    contactModal?.showModal();
  }, []);


The handleContactModalOpen function uses imperative DOM manipulation to open the contact modal. This is inconsistent with the PR's goal of eliminating imperative DOM manipulation. Consider managing modal state through React state and passing it as a prop to the ContactForm component.

dang we still have a lot of technical debt...

src/config/firebase.ts

GoogolGenius · 2026-02-22T21:39:54Z

@Ascent817 I used the preview URL, and I cannot login. The popup gets blocked even though I allowed popups. Possible bug? Is it working on localhost?

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

GoogolGenius · 2026-02-22T21:22:54Z

src/components/ClearFilter.tsx

+  useEffect(() => {
+    const courseContainer = document.getElementById(
+      'course-container'
+    ) as HTMLDivElement;

-  const clearResults = () => {
-    const search = document.getElementById('searchbar') as HTMLInputElement;
-    search.value = '';
-
-    const tags = document.getElementsByClassName('tag');
-    for (let i = 0; i < tags.length; i++) {
-      if (tags[i].classList.contains('tag-true')) {
-        tags[i].classList.remove('tag-true');
-      }
-    }
+    const adjustHeight = () => {
+      courseContainer.style.height =
+        getWidth() >= 525
+          ? 'calc(100vh - 180px)'
+          : 'calc(100vh - (45.5px + 65.5px + (clamp(15px, 5vw, 20px) * 2)))';
+    };

-    filterCourses();
-  };
+    adjustHeight();
+    window.addEventListener('resize', adjustHeight);
+    return () => {
+      window.removeEventListener('resize', adjustHeight);
+      courseContainer.style.height = '';
+    };


@Ascent817 well well well

GoogolGenius · 2026-02-22T21:31:32Z

src/App.tsx

@@ -34,26 +211,62 @@ const App: FC<AppProps> = ({ user, classItems, authLevel }): JSX.Element => {
    contactModal?.showModal();
  }, []);


dang we still have a lot of technical debt...

src/App.tsx

GoogolGenius · 2026-02-22T21:37:30Z

src/App.tsx

+  const courseIDtoNameMap = useMemo(() => {
+    const map = new Map<string, string>();
+    for (const courseName in courseData) {
+      courseData![courseName].courseid.match(COURSE_ID_REGEX)?.forEach((id) => {
+        map.set(id, courseName);
+      });
+    }
+    return map;
+  }, [courseData]);


@Ascent817 is coursedata gonna ever be null, don't we load in the pre-downloaded coursedata json regardless

This reverts commit fb85a5b.

Ascent817 · 2026-02-22T23:52:27Z

@Ascent817 I used the preview URL, and I cannot login. The popup gets blocked even though I allowed popups. Possible bug? Is it working on localhost?

It was working on localhost for me, maybe the preview URL isn't an authorized domain to send OAuth requests from?

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This reverts commit fb85a5b.

GoogolGenius · 2026-02-23T00:46:29Z

@Ascent817 I used the preview URL, and I cannot login. The popup gets blocked even though I allowed popups. Possible bug? Is it working on localhost?

It was working on localhost for me, maybe the preview URL isn't an authorized domain to send OAuth requests from?

That's probably what's happening

…into code-cleanup

This reverts commit f94cfa3.

Ascent817 added 5 commits January 22, 2025 15:55

Add Computer Science Tag

759c773

Merge branch 'main' of https://github.com/magnifydev/magnify

420e74c

Merge branch 'main' of https://github.com/magnifydev/magnify

5bdb449

Remove repetition in TagBar.tsx

3842876

Clean up codebase to better follow React best practices

eb30f12

Ascent817 requested review from GoogolGenius and dniemitalo February 20, 2026 00:30

Run prettier

bec3fba

Ascent817 requested a review from Copilot February 20, 2026 18:10

Copilot started reviewing on behalf of Ascent817 February 20, 2026 18:10 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

Apply suggestions from code review

fb85a5b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

GoogolGenius requested changes Feb 22, 2026

View reviewed changes

GoogolGenius added 2 commits February 22, 2026 15:49

Revert "Apply suggestions from code review"

3456875

This reverts commit fb85a5b.

Extract tags to single structure

ed5a4e3

Ascent817 and others added 6 commits February 22, 2026 18:44

Remove repetition in TagBar.tsx

ba7b849

Clean up codebase to better follow React best practices

6c9d9c2

Run prettier

5bb122d

Apply suggestions from code review

23b5d17

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Revert "Apply suggestions from code review"

50dcdda

This reverts commit fb85a5b.

Extract tags to single structure

1c14ed7

GoogolGenius force-pushed the code-cleanup branch from ed5a4e3 to 1c14ed7 Compare February 23, 2026 00:44

GoogolGenius added 3 commits February 22, 2026 18:47

remove handleClearFilters dependency

f94cfa3

Merge branch 'code-cleanup' of https://github.com/magnifydev/magnify …

210a4ee

…into code-cleanup

Revert "remove handleClearFilters dependency"

e320c92

This reverts commit f94cfa3.

GoogolGenius approved these changes Feb 23, 2026

View reviewed changes

Ascent817 merged commit ed781b4 into main Feb 23, 2026
5 checks passed

Ascent817 deleted the code-cleanup branch February 23, 2026 01:25

-        });
+        });
+      })
+      .catch((error) => {
+        console.error('Failed to fetch auth level:', error);

	}, [filteredCourseItems, numColumns, handleClearFilters]);
	}, [filteredCourseItems, numColumns]);

	document.cookie = `user=${JSON.stringify(signedInUser)}`;
	document.cookie = `user=${encodeURIComponent(signedInUser.uid)}`;

		@@ -34,26 +211,62 @@ const App: FC<AppProps> = ({ user, classItems, authLevel }): JSX.Element => {
		contactModal?.showModal();
		}, []);

Conversation

Ascent817 commented Feb 20, 2026

Description

Uh oh!

github-actions bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GoogolGenius commented Feb 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ascent817 commented Feb 22, 2026

Uh oh!

GoogolGenius commented Feb 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 20, 2026 •

edited

Loading