-
Notifications
You must be signed in to change notification settings - Fork 0
Code cleanup #158
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
Code cleanup #158
Changes from all commits
759c773
420e74c
5bdb449
3842876
eb30f12
bec3fba
fb85a5b
3456875
ed5a4e3
ba7b849
6c9d9c2
5bb122d
23b5d17
50dcdda
1c14ed7
f94cfa3
210a4ee
e320c92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,31 +1,192 @@ | ||||||||||||
| import './App.css'; | ||||||||||||
| import { | ||||||||||||
| ClearFilter, | ||||||||||||
| ContactForm, | ||||||||||||
| Course, | ||||||||||||
| FloatingActionable, | ||||||||||||
| Loader, | ||||||||||||
| MobileNavigation, | ||||||||||||
| Navigation, | ||||||||||||
| TagBar, | ||||||||||||
| TopBar, | ||||||||||||
| } from './components'; | ||||||||||||
| // import { ParsePDF } from './components/ParsePDF'; | ||||||||||||
| import { auth, db, provider } from './config'; | ||||||||||||
| import { TAGS } from './data'; | ||||||||||||
| import localCourseData from './data/coursedata.json'; | ||||||||||||
| import { CourseDataType, CourseType } from './types'; | ||||||||||||
| import { getCookieValue, getWidth } from './utils'; | ||||||||||||
| import firebase from 'firebase/compat/app'; | ||||||||||||
| import { FC, useCallback, useEffect } from 'react'; | ||||||||||||
| import { FC, useCallback, useEffect, useMemo, useState } from 'react'; | ||||||||||||
|
|
||||||||||||
| interface AppProps { | ||||||||||||
| user: firebase.User | null; | ||||||||||||
| classItems: JSX.Element; | ||||||||||||
| authLevel: number; | ||||||||||||
| } | ||||||||||||
| const COURSE_ID_REGEX = /[A-Z]{3}[0-9]{3}/g; | ||||||||||||
| const COURSE_ID_REGEX_SINGLE = /[A-Z]{3}[0-9]{3}/; | ||||||||||||
|
|
||||||||||||
| const App: FC<AppProps> = ({ user, classItems, authLevel }): JSX.Element => { | ||||||||||||
| // Hacky workaround because something with React probably interferes with the default browser behavior | ||||||||||||
| // Also, reactivates the highlight on the course element | ||||||||||||
| const getNumColumns = (width: number): number => { | ||||||||||||
| if (width > 1400) return 4; | ||||||||||||
| if (width > 1100) return 3; | ||||||||||||
| if (width > 800) return 2; | ||||||||||||
| return 1; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const App: FC = (): JSX.Element => { | ||||||||||||
| const [courseData, setCourseData] = useState<CourseDataType | null>(null); | ||||||||||||
| const [user, setUser] = useState<firebase.User | null>(() => { | ||||||||||||
| const cookie = getCookieValue('user'); | ||||||||||||
| if (!cookie) return null; | ||||||||||||
| try { | ||||||||||||
| return JSON.parse(cookie); | ||||||||||||
| } catch { | ||||||||||||
| return null; | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
| const [authLevel, setAuthLevel] = useState(0); | ||||||||||||
| const [searchQuery, setSearchQuery] = useState(''); | ||||||||||||
| const [activeTags, setActiveTags] = useState<string[]>([]); | ||||||||||||
| const [windowWidth, setWindowWidth] = useState(getWidth()); | ||||||||||||
|
|
||||||||||||
| // Fetch course data from Firebase, fall back to local data | ||||||||||||
| useEffect(() => { | ||||||||||||
| db.ref() | ||||||||||||
| .get() | ||||||||||||
| .then((snapshot) => { | ||||||||||||
| setCourseData( | ||||||||||||
| snapshot.exists() ? snapshot.val().courses : localCourseData | ||||||||||||
| ); | ||||||||||||
| }) | ||||||||||||
| .catch(() => setCourseData(localCourseData)); | ||||||||||||
| }, []); | ||||||||||||
|
|
||||||||||||
| // Fetch auth level for the signed-in user | ||||||||||||
| useEffect(() => { | ||||||||||||
| if (!user) return; | ||||||||||||
| db.ref() | ||||||||||||
| .get() | ||||||||||||
| .then((snapshot) => { | ||||||||||||
| if (!snapshot.exists()) return; | ||||||||||||
| const users: { [key: string]: { email: string; level: number } } = | ||||||||||||
| snapshot.val().users; | ||||||||||||
| Object.values(users).forEach(({ email, level }) => { | ||||||||||||
| if (user.email === email) setAuthLevel(level); | ||||||||||||
| }); | ||||||||||||
|
||||||||||||
| }); | |
| }); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to fetch auth level:', error); |
Copilot
AI
Feb 20, 2026
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 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
AI
Feb 20, 2026
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 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();
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.
@Ascent817 is coursedata gonna ever be null, don't we load in the pre-downloaded coursedata json regardless
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.
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
AI
Feb 20, 2026
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 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.
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.
dang we still have a lot of technical debt...
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,45 +1,38 @@ | ||
| import '../App.css'; | ||
| import { filterCourses } from '../index'; | ||
| import { getWidth } from '../utils'; | ||
| import { FC } from 'react'; | ||
| import { FC, useEffect } from 'react'; | ||
|
|
||
| export const ClearFilter: FC = (): JSX.Element => { | ||
| const courseContainer = document.getElementById( | ||
| 'course-container' | ||
| ) as HTMLDivElement; | ||
| courseContainer.style.display = 'flex'; | ||
| courseContainer.style.justifyContent = 'center'; | ||
| interface ClearFilterProps { | ||
| onClearFilters: () => void; | ||
| } | ||
|
|
||
| const adjustCourseContainerHeight = () => { | ||
| if (getWidth() >= 525) { | ||
| courseContainer.style.height = 'calc(100vh - 180px)'; | ||
| } else { | ||
| courseContainer.style.height = | ||
| 'calc(100vh - (45.5px + 65.5px + (clamp(15px, 5vw, 20px) * 2)))'; | ||
| } | ||
| }; | ||
|
|
||
| adjustCourseContainerHeight(); | ||
| window.addEventListener('resize', adjustCourseContainerHeight); | ||
| export const ClearFilter: FC<ClearFilterProps> = ({ | ||
| onClearFilters, | ||
| }): JSX.Element => { | ||
| 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 = ''; | ||
| }; | ||
|
Comment on lines
+12
to
+29
|
||
| }, []); | ||
|
|
||
| return ( | ||
| <div className="ClearFilter" id="clear-filter-class"> | ||
| <h1>No Results</h1> | ||
| <button className="clear-results" onClick={clearResults}> | ||
| <button className="clear-results" onClick={onClearFilters}> | ||
| Clear Filters | ||
| </button> | ||
| </div> | ||
|
|
||
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 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.