Fix thumbnails, reduce network activity, rate limiting, caching, script review page #222

Closed
ic3w0lf22 wants to merge 13 commits from thumbnail-cache-batch into staging
Member

I made sure the cache is cleared eventually (checked whenever the endpoints are executed), didn't use setInterval since build kept failing with (according to AI) whatever the "best practice" was. I am not a certified NextJS/React dev.

This should also help with Roblox's rate limiting by calling their endpoints with up to 50 IDs per request, while also reducing how many network requests the site makes, it used to make 3 requests per map, map thumbnail, username, user avatar, a whopping 72 requests per page‼️
Now it only makes 3 requests per page 😎😎

Closes #212

I made sure the cache is cleared eventually (checked whenever the endpoints are executed), didn't use `setInterval` since build kept failing with (according to AI) whatever the "best practice" was. I am not a certified NextJS/React dev. This should also help with Roblox's rate limiting by calling their endpoints with up to 50 IDs per request, while also reducing how many network requests the site makes, it used to make 3 requests per map, map thumbnail, username, user avatar, a whopping 72 requests per page‼️ Now it only makes 3 requests per page 😎😎 Closes #212
ic3w0lf22 added 11 commits 2025-07-01 23:33:52 +00:00
severe vibe coding going on here...
All checks were successful
continuous-integration/drone/push Build is passing
f0abb9ffbf
on another note, build succeeds :D (i love eslint-disable >:)) <- is this even safe? we'll find out if the server explodes
why were these untracked... severely vibe coded files btw
All checks were successful
continuous-integration/drone/push Build is passing
c21afaa846
more batching & shii
All checks were successful
continuous-integration/drone/push Build is passing
82284947ee
Batch limits, AI suggested anti-spam.
All checks were successful
continuous-integration/drone/push Build is passing
87c1d161fc
build succeed + use media for thumbnail if available
All checks were successful
continuous-integration/drone/push Build is passing
709bb708d3
User information caching
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
dfe9107112
Author
Member

oh no why is there a conflict

oh no why is there a conflict
ic3w0lf22 added 1 commit 2025-07-01 23:47:18 +00:00
Merge branch 'staging' into thumbnail-cache-batch
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
3e0bc9804f
Author
Member

no way i did that first try holy moly!! but build failed cuz i forgot 1 thing

no way i did that first try holy moly!! but build failed cuz i forgot 1 thing
ic3w0lf22 added 1 commit 2025-07-01 23:57:12 +00:00
Sorry to whoever uses 2 spaces instead of a tab
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
1f7ba5bb9b
ic3w0lf22 added the frontend label 2025-07-02 00:39:27 +00:00
ic3w0lf22 added 3 commits 2025-07-02 11:06:05 +00:00
Script review page, server-side session fetching & no more manually fetching session information
All checks were successful
continuous-integration/drone/push Build is passing
5995737dc3
Build Succeed.
All checks were successful
continuous-integration/drone/push Build is passing
7cf0bc3187
Build Succeed - working script review page
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
184795a513
ic3w0lf22 changed title from Fix thumbnails, reduce network activity, rate limiting, caching to Fix thumbnails, reduce network activity, rate limiting, caching, script review page 2025-07-02 11:06:28 +00:00
ic3w0lf22 added 1 commit 2025-07-02 11:18:41 +00:00
diff
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
ddadbc3e00
Owner

Please split features into manageable chunks with their own pull request! New feature new branch man

Please split features into manageable chunks with their own pull request! New feature new branch man
Author
Member

How do I make sure they wont have conflicts if they modify the same files?

Do I just copy the last branch I was working on?

How do I make sure they wont have conflicts if they modify the same files? Do I just copy the last branch I was working on?
Member

holy pr

holy pr
Author
Member

real shit

real shit
Owner

How do I make sure they wont have conflicts if they modify the same files?

Do I just copy the last branch I was working on?

It's called stacked PRs.

  1. Work feature A on branch feature/A which is branched off staging
  2. Create PR feature/A -> staging
  3. Work feature B on branch feature/B which is branched off feature/A
  4. Create PR feature/B -> feature/A, set a dependency on PR A
  5. Rinse and repeat stacking PRs onto the previous PR.

When PR A gets merged, PR B will automatically be updated to target the default branch (staging). This keeps our diffs clean and the review scope small. This feature should work on this site, but also works on Github/Gitlab.

> How do I make sure they wont have conflicts if they modify the same files? > > Do I just copy the last branch I was working on? It's called stacked PRs. 1. Work feature A on branch `feature/A` which is branched off `staging` 2. Create PR `feature/A` -> `staging` 3. Work feature B on branch `feature/B` which is branched off `feature/A` 4. Create PR `feature/B` -> `feature/A`, set a dependency on PR A 5. Rinse and repeat stacking PRs onto the previous PR. When PR A gets merged, PR B will automatically be updated to target the default branch (staging). This keeps our diffs clean and the review scope small. This feature _should_ work on this site, but also works on Github/Gitlab.
Owner

@Quaternions are you reviewing this or am I supposed to? 😬

@Quaternions are you reviewing this or am I supposed to? 😬
Owner

It's entirely website changes which is clearly your domain 😁 I was hoping @ic3w0lf22 would split out the different features into separate PRs so I don't have to figure out which lines of code correspond to which features while reviewing.

It's entirely website changes which is clearly your domain 😁 I was hoping @ic3w0lf22 would split out the different features into separate PRs so I don't have to figure out which lines of code correspond to which features while reviewing.
Author
Member

I would try to split them but I'm 98% sure I'd fuck up something somewhere somehow

I would try to split them but I'm 98% sure I'd fuck up something somewhere somehow
Owner

I'll keep a local copy of the branch. Even if you eviscerate the entire gitzaname repo, restoring it is as simple as force pushing from my branch. Every clone is a full repo backup. Learn the git tech now so you can take advantage of it in the future.

I would start by creating branches at critical points along the chain where each feature is complete. Make a new branch at the same place as thumbnail-cache-batch, and force push thumbnail-cache-batch back to the point where it was a single feature.

So like, create the branches:

CommitE -> Branch3 + thumbnail-cache-batch
   |
CommitD
   |
CommitC -> Branch2
   |
CommitB
   |
CommitA -> Branch1

Then force push thumbnail-cache-batch to CommitA

Then you end up with:

CommitE -> Branch3
   |
CommitD
   |
CommitC -> Branch2
   |
CommitB
   |
CommitA -> Branch1 + thumbnail-cache-batch

Then you can make a stacked PR for each feature branch, and you don't need to close this PR.

I'll keep a local copy of the branch. Even if you eviscerate the entire gitzaname repo, restoring it is as simple as force pushing from my branch. Every clone is a full repo backup. Learn the git tech now so you can take advantage of it in the future. I would start by creating branches at critical points along the chain where each feature is complete. Make a new branch at the same place as `thumbnail-cache-batch`, and force push `thumbnail-cache-batch` back to the point where it was a single feature. So like, create the branches: ``` CommitE -> Branch3 + thumbnail-cache-batch | CommitD | CommitC -> Branch2 | CommitB | CommitA -> Branch1 ``` Then force push `thumbnail-cache-batch` to CommitA Then you end up with: ``` CommitE -> Branch3 | CommitD | CommitC -> Branch2 | CommitB | CommitA -> Branch1 + thumbnail-cache-batch ``` Then you can make a stacked PR for each feature branch, and you don't need to close this PR.
ic3w0lf22 force-pushed thumbnail-cache-batch from ddadbc3e00 to 3e0bc9804f 2025-07-08 19:20:08 +00:00 Compare
ic3w0lf22 added 1 commit 2025-07-08 19:23:13 +00:00
Sorry to whoever uses 2 spaces instead of a tab
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
1f7ba5bb9b
Author
Member

I think I did it correctly but this whole PR is still a mess. I'll try to not have future PRs be so messy 👽👍

I think I did it correctly but this whole PR is still a mess. I'll try to not have future PRs be so messy 👽👍
ic3w0lf22 added a new dependency 2025-07-08 20:03:23 +00:00
Owner

This is still like 1000+loc and 3 features rolled into one is it not?

This is still like 1000+loc and 3 features rolled into one is it not?
Owner

3e0bc9804f is a big mistake btw, merging staging branch into feature branch makes the git impossible to work with, you needed to rebase the feature branch onto staging instead

3e0bc9804f8073c28e3c4bda8ce1d51f889ff69a is a big mistake btw, merging staging branch into feature branch makes the git impossible to work with, you needed to rebase the feature branch onto staging instead
Owner

Another thing: formatting an entire file should be its own commit since it majorly confuses git, never fold code changes into the same commit as massive formatting changes

Another thing: formatting an entire file should be its own commit since it majorly confuses git, never fold code changes into the same commit as massive formatting changes
Owner

The merge conflicts happened because this branch wasn't based on this commit:
image.png

It is based on an earlier commit and conflicts with the changes early on, meaning that a lot of commits that build on it end up conflicting also

The merge conflicts happened because this branch wasn't based on this commit: ![image.png](/attachments/6fd4ca01-d911-46f3-92ee-fee9795f81e5) It is based on an earlier commit and conflicts with the changes early on, meaning that a lot of commits that build on it end up conflicting also
Quaternions requested changes 2025-07-09 14:37:44 +00:00
Quaternions left a comment
Owner

There is so much going on here I still have no idea what most of this code does even after I read all of it

There is so much going on here I still have no idea what most of this code does even after I read all of it
@@ -0,0 +1,61 @@
import { useState, useEffect } from "react";
function chunkArray<T>(arr: T[], size: number): T[][] {
Owner

This is cursed, surely it can be done in chunks without allocating the chunks ahead of time

This is cursed, surely it can be done in chunks without allocating the chunks ahead of time
@@ -0,0 +55,4 @@
.catch(err => setError(err))
.finally(() => setLoading(false));
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [assetIds && assetIds.filter(Boolean).join(",")]);
Owner

is this joining an array of ids into a string and then parsing it out again later?

is this joining an array of ids into a string and then parsing it out again later?
@@ -6,25 +6,26 @@ import { useParams, useRouter } from "next/navigation";
import React, { useState, useEffect } from "react";
Owner

This entire file is extremely difficult to review because of the formatting changes. the green and red code essentially doubles the entire file and is interspersed randomly, making it impossible to spot actual changes

This entire file is extremely difficult to review because of the formatting changes. the green and red code essentially doubles the entire file and is interspersed randomly, making it impossible to spot actual changes
@@ -0,0 +1,99 @@
// NOTE: This API endpoint proxies Roblox user info in batch and implements in-memory rate limiting.
// For production, this logic should be moved to a dedicated backend API server (not serverless/edge)
Owner

wtf is this comment

wtf is this comment
@@ -79,0 +88,4 @@
if (avatarUrls[uid]) auditEventUserAvatarUrls[uid] = avatarUrls[uid];
}
const showSnackbar = (message: string, severity: 'success' | 'error' | 'info' | 'warning' = 'success') => {
Owner

showSnackbar and handleCloseSnackbar are moved for no reason, I got confused because I couldn't see the code relevant to where they were used in the diff. My error I guess not noticing it was moved right away

`showSnackbar` and `handleCloseSnackbar` are moved for no reason, I got confused because I couldn't see the code relevant to where they were used in the diff. My error I guess not noticing it was moved right away
@@ -0,0 +1,32 @@
// NOTE: This file is used as a shared in-memory per-IP rate limiter for all Next.js API routes that need it.
// Not for production-scale, but good for basic abuse prevention.
Owner

The entire rate limit thing seems extremely overbuilt and should probably be ripped out, or if it becomes a problem, implemented in the backend like it suggests

The entire rate limit thing seems extremely overbuilt and should probably be ripped out, or if it becomes a problem, implemented in the backend like it suggests
Owner

I did manage to remove the staging->feature merge, you can find the branch here: https://git.itzana.me/StrafesNET/maps-service/src/branch/ic3-features-clean/

However, I think you need to start over, there is way too much going on here.

  1. Create a branch based on staging
  2. Format files and push a formatting-only commit, open a formatting-only PR
  3. For each feature, start a new branch based on the previous branch and push one or more commits that implement it completely. Doing it this way means that all the branches are ready for stacked PRs
  4. Open a PR for each feature branch

Ideally you make 5 (?) feature PRs plus the format PR, and see if you can boil down the ai cruft.

In the end it should look like this:
image.png

I did manage to remove the staging->feature merge, you can find the branch here: https://git.itzana.me/StrafesNET/maps-service/src/branch/ic3-features-clean/ However, I think you need to start over, there is way too much going on here. 1. Create a branch based on staging 2. Format files and push a formatting-only commit, open a formatting-only PR 3. For each feature, start a new branch based on the previous branch and push one or more commits that implement it completely. Doing it this way means that all the branches are ready for stacked PRs 4. Open a PR for each feature branch Ideally you make 5 (?) feature PRs plus the format PR, and see if you can boil down the ai cruft. In the end it should look like this: ![image.png](/attachments/bb0c36e1-ab5f-47cd-8bc2-22a4e7d01de5)
Owner

A method I've used to clean up commit histories is to squash everything into one commit, and then edit commit contents and pull out things into separate commits bit by bit. Check out my https://git.itzana.me/StrafesNET/maps-service/src/branch/ic3-features-clean/ branch if you are having trouble dealing with the merge commit (3e0bc9804f) in the middle.

An overview of what I think of the features in the title:

  • Fix thumbnails -> want to land 👍
  • reduce network activity -> needs explanation and review
  • rate limiting -> this should be in the backend, not in javascript
  • caching -> thumbnail caching should be done by the browser itself, the thumbnails should not be proxied and cached in javascript
  • script review page -> want to land 👍

These all must be separated to enable us to land the code that is deemed ready, and make review and revisions possible for the code that is deemed not ready.

A method I've used to clean up commit histories is to squash everything into one commit, and then edit commit contents and pull out things into separate commits bit by bit. Check out my https://git.itzana.me/StrafesNET/maps-service/src/branch/ic3-features-clean/ branch if you are having trouble dealing with the merge commit (3e0bc9804f8073c28e3c4bda8ce1d51f889ff69a) in the middle. An overview of what I think of the features in the title: - Fix thumbnails -> want to land 👍 - reduce network activity -> needs explanation and review - rate limiting -> this should be in the backend, not in javascript - caching -> thumbnail caching should be done by the browser itself, the thumbnails should not be proxied and cached in javascript - script review page -> want to land 👍 These all must be separated to enable us to land the code that is deemed ready, and make review and revisions possible for the code that is deemed not ready.
Owner

Don’t think there is going to be much movement on this one. Probably either need to accept it as is or make the changes yourself.

Don’t think there is going to be much movement on this one. Probably either need to accept it as is or make the changes yourself.
Owner

@Quaternions just yolo this into staging and hope for the best if you actually want these features.

@Quaternions just yolo this into staging and hope for the best if you actually want these features.
Owner

@Quaternions we still scared of this PR?

@Quaternions we still scared of this PR?
Owner

My current plan is to manually pull out the features I want into separate pull requests, and then close this with the features I don't want. I'm just not working on the website right now. I plan to power through physics bugs on strafe client until the end of the year.

I just want:

  • Fix thumnails
  • Script review page

Everything else can probably be implemented somewhere other than nextjs in javascript.

My current plan is to manually pull out the features I want into separate pull requests, and then close this with the features I don't want. I'm just not working on the website right now. I plan to power through physics bugs on strafe client until the end of the year. I just want: - Fix thumnails - Script review page Everything else can probably be implemented somewhere other than nextjs in javascript.
Owner

How can I cut funding to strafe client?

How can I cut funding to strafe client?
Owner

So rude.

So rude.
Owner

Closing as OBE

Closing as OBE
itzaname closed this pull request 2025-12-27 08:14:19 +00:00
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing

Pull request closed

Sign in to join this conversation.