-
Notifications
You must be signed in to change notification settings - Fork 699
Various speed improvements #13533
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: master
Are you sure you want to change the base?
Various speed improvements #13533
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
No files reviewed, 2 comments
| team: { teamMembers }, | ||
| allTeams, | ||
| } = useStaticQuery(teamQuery) | ||
| export default function People({ searchTerm, filteredMembers, teamMembers, allTeams }: PeopleProps = {}) { |
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.
logic: default parameter value could cause runtime error if props are undefined
The default value = {} doesn't provide values for required props teamMembers and allTeams. When People component is called without props, teamMembers will be undefined causing errors at lines 305 and 310.
| export default function People({ searchTerm, filteredMembers, teamMembers, allTeams }: PeopleProps = {}) { | |
| export default function People({ searchTerm, filteredMembers, teamMembers = [], allTeams = { nodes: [] } }: PeopleProps = {}) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/People/index.tsx
Line: 304:304
Comment:
**logic:** default parameter value could cause runtime error if props are undefined
The default value `= {}` doesn't provide values for required props `teamMembers` and `allTeams`. When People component is called without props, `teamMembers` will be undefined causing errors at lines 305 and 310.
```suggestion
export default function People({ searchTerm, filteredMembers, teamMembers = [], allTeams = { nodes: [] } }: PeopleProps = {}) {
```
How can I resolve this? If you propose a fix, please make it concise.| <CloudinaryImage | ||
| src="https://res.cloudinary.com/dmukukwp6/image/upload/keyboard_garden_light_opt_compressed_5094746caf.png" | ||
| alt="" | ||
| width={1401} | ||
| height={1400} | ||
| className="size-[300px] md:size-[700px] dark:hidden" | ||
| /> |
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.
style: inconsistent loading attribute for wallpaper images
Light theme image lacks loading="lazy" while dark version has it (line 114). For consistent lazy loading behavior across themes, add the attribute.
| <CloudinaryImage | |
| src="https://res.cloudinary.com/dmukukwp6/image/upload/keyboard_garden_light_opt_compressed_5094746caf.png" | |
| alt="" | |
| width={1401} | |
| height={1400} | |
| className="size-[300px] md:size-[700px] dark:hidden" | |
| /> | |
| <CloudinaryImage | |
| loading="lazy" | |
| src="https://res.cloudinary.com/dmukukwp6/image/upload/keyboard_garden_light_opt_compressed_5094746caf.png" | |
| alt="" | |
| width={1401} | |
| height={1400} | |
| className="size-[300px] md:size-[700px] dark:hidden" | |
| /> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/Desktop/index.tsx
Line: 106:112
Comment:
**style:** inconsistent `loading` attribute for wallpaper images
Light theme image lacks `loading="lazy"` while dark version has it (line 114). For consistent lazy loading behavior across themes, add the attribute.
```suggestion
<CloudinaryImage
loading="lazy"
src="https://res.cloudinary.com/dmukukwp6/image/upload/keyboard_garden_light_opt_compressed_5094746caf.png"
alt=""
width={1401}
height={1400}
className="size-[300px] md:size-[700px] dark:hidden"
/>
```
How can I resolve this? If you propose a fix, please make it concise.|
This basically introduces CLS, which arguably isn't any better than the loading state. Is there any way we can calculate where the window is most likely to appear (eg: a little bit of offset) instead of just rendering at the top, then calcing after the page finishes loading? |
|
Windows should always initially load in the correct place. Are you looking at the right deployment? Or maybe you have boring mode on? Screen.Recording.2025-11-06.at.6.06.05.PM.mov |
|
@smallbrownbike We good to merge this or did you want to make some more tweaks? |
|
@corywatilo just need to make a couple tweaks! will ping you when it's ready |
|
that's actually a good showcase of what this does - window loads immediately while everything loads behind it. if this were prod, your slow internet would just show the loading bar until the page eventually loads. |
Changes