Fix thumbnails, reduce network activity, rate limiting, caching, script review page #222
4 Participants
Notifications
Due Date
No due date set.
Blocks
#231 Script Review page + SessionProvider
StrafesNET/maps-service
Reference: StrafesNET/maps-service#222
Reference in New Issue
Block a user
Delete Branch "thumbnail-cache-batch"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I made sure the cache is cleared eventually (checked whenever the endpoints are executed), didn't use
setIntervalsince 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
oh no why is there a conflict
no way i did that first try holy moly!! but build failed cuz i forgot 1 thing
Fix thumbnails, reduce network activity, rate limiting, cachingto Fix thumbnails, reduce network activity, rate limiting, caching, script review pagePlease split features into manageable chunks with their own pull request! New feature new branch man
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?
holy pr
real shit
It's called stacked PRs.
feature/Awhich is branched offstagingfeature/A->stagingfeature/Bwhich is branched offfeature/Afeature/B->feature/A, set a dependency on PR AWhen 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.
@Quaternions are you reviewing this or am I supposed to? 😬
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.
I would try to split them but I'm 98% sure I'd fuck up something somewhere somehow
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 pushthumbnail-cache-batchback to the point where it was a single feature.So like, create the branches:
Then force push
thumbnail-cache-batchto CommitAThen you end up with:
Then you can make a stacked PR for each feature branch, and you don't need to close this PR.
ddadbc3e00to3e0bc9804fI think I did it correctly but this whole PR is still a mess. I'll try to not have future PRs be so messy 👽👍
This is still like 1000+loc and 3 features rolled into one is it not?
3e0bc9804fis 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 insteadAnother 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
The merge conflicts happened because this branch wasn't based on this commit:

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
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[][] {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(",")]);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";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)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') => {showSnackbarandhandleCloseSnackbarare 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.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
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.
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:

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:
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.
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.
@Quaternions just yolo this into staging and hope for the best if you actually want these features.
@Quaternions we still scared of this PR?
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:
Everything else can probably be implemented somewhere other than nextjs in javascript.
How can I cut funding to strafe client?
So rude.
Closing as OBE
Pull request closed