Skip to content

Conversation

findarian
Copy link

@findarian findarian commented Sep 20, 2025

PvP Leaderboard plugin.

Overall it sends POST request after you fight someone to my AWS API gateway. Backend does logic and keeps this data in a dynamoDB to process matches and be able to get this information to my website and back to the plugin.

The plugin pulls ranks of people around them in game, this information is gotten from my website https://devsecopsautomated.com/ and the API gateway endpoints depending on which information is requested. Then that information is pushed back into client to display people's ranks.

It loads ranks of people around you that are also on the leaderboard and works with different styles of combat (multi/dmm/nh/pvp/veng).

I appreciate all of your time in reviewing this, you guys keep this community great <3

@findarian findarian force-pushed the add-pvp-leaderboard branch 2 times, most recently from 788ff3c to e6a7c24 Compare September 20, 2025 09:35
@runelite-github-app
Copy link

runelite-github-app bot commented Sep 20, 2025

@findarian findarian force-pushed the add-pvp-leaderboard branch 4 times, most recently from 14d492c to b0cf9f6 Compare September 20, 2025 09:49
@findarian findarian changed the title Add/Update pvp leaderboard (pin 39435072fb5e011b8f5ac87a01086f2c075df… Add pvp leaderboard Sep 20, 2025
@runelite-github-app
Copy link

runelite-github-app bot commented Sep 20, 2025

This plugin requires a review from a Plugin Hub maintainer. The reviewer will request any additional changes if needed.


Internal use only: Reviewer details Maintainer details

@pajlada
Copy link
Contributor

pajlada commented Sep 20, 2025

We don't permit the use of reflection in plugins - take a read through https://github.com/runelite/runelite/wiki/Rejected-or-Rolled-Back-Features

If you're making network requests, you need to include a warning about it in your plugin hub manifest file (see https://github.com/runelite/plugin-hub/blob/56fca1636653cbed5de79ab5994a327d74f5b6e5/plugins/tts as an example)

@pajlada pajlada added the waiting for author waiting for the pr author to make changes or respond to questions label Sep 20, 2025
@runelite-github-app runelite-github-app bot added plugin change non-author plugin change and removed waiting for author waiting for the pr author to make changes or respond to questions labels Sep 20, 2025
@findarian
Copy link
Author

Beta tester found an issue that was causing. I've fixed this and have pushed the new commit here.

@pajlada
Copy link
Contributor

pajlada commented Sep 21, 2025

you've made changes to another plugin

@pajlada pajlada added the waiting for author waiting for the pr author to make changes or respond to questions label Sep 21, 2025
@runelite-github-app runelite-github-app bot added plugin removed and removed plugin change waiting for author waiting for the pr author to make changes or respond to questions labels Sep 21, 2025
@findarian
Copy link
Author

Sorry about that, this should be fixed now.

@pajlada
Copy link
Contributor

pajlada commented Sep 21, 2025

Some minor feedback, haven't done a full review

You should probably do your development on a single branch (i.e. the commit hashes you push here) - you're now pushing commit hashes from your add-pvp-leaderboard branch which I assume is a feature branch of some sort

The plugins directory in your repository serves no purpose, you only need to update the plugin hub manifest file in the plugin-hub fork that you PR to this repo.

You should not need to make the dependency changes you've made in your build.gradle file (line 20 & 29) - other plugins use gson and lombok file without specifying those. Did you run into any issues when you didn't make those changes?

@findarian
Copy link
Author

findarian commented Sep 21, 2025

The dependency changes were when I was messing around with going through the problems panel in cursor.

I've removed the plugins directory.

I'm going to keep everything from the current branch going forward. I'm not planning on making any changes unless something is brought up here or if one of my testers or myself find a bug that is breaking the plugin in some way.

Thank you for the feedback, I appreciate your time.

@pajlada
Copy link
Contributor

pajlada commented Sep 21, 2025

for JSON things, inject & use gson (see https://github.com/pajlads/DinkPlugin/blob/075b32f12e223eab8b67116c856dec43701b0209/src/main/java/dinkplugin/message/DiscordMessageHandler.java#L205 as an example for parsing a json body from a network request, and see https://github.com/pajlads/DinkPlugin/blob/075b32f12e223eab8b67116c856dec43701b0209/src/main/java/dinkplugin/message/DiscordMessageHandler.java#L325 for how to send a json payload in a request)
for network requests, inject & use OkHttpClient and make async requests with enqueue (see https://github.com/pajlads/DinkPlugin/blob/075b32f12e223eab8b67116c856dec43701b0209/src/main/java/dinkplugin/message/DiscordMessageHandler.java#L186-L227 as an example)
for executor stuff, inject & use our ScheduledExecutor (unless you have really long-lived/heavy-to-perform tasks)

@findarian
Copy link
Author

These changes have been applied.

@pajlada
Copy link
Contributor

pajlada commented Sep 22, 2025

You're still using some random ways of parsing json & network requests despite what I told you, and you telling me you've fixed it.

Your auth seems overly complex - could you solve this without involving a web server?

You're still doing really wonky things with the executor, despite what I told you, and you telling me you've fixed it.

You've removed the warning from the plugin hub manifest - it still needs to be there.


I get the feeling that you're vibing too hard - please look at core plugins (i.e. plugins inside the runelite/runelite repository) and see how they do things and use those as reference.

@riktenx riktenx added the waiting for author waiting for the pr author to make changes or respond to questions label Sep 22, 2025
@runelite-github-app runelite-github-app bot added plugin change non-author plugin change and removed waiting for author waiting for the pr author to make changes or respond to questions labels Sep 22, 2025
@findarian
Copy link
Author

You're still using some random ways of parsing json & network requests despite what I told you, and you telling me you've fixed it.
-I've fixed these now. Sorry about before, thank you for pointing this out.

Your auth seems overly complex - could you solve this without involving a web server?
-Short answer is yes, but I don't think that this is a good idea.

So the current implementation of Authorization Code + PKCE with loopback was chosen for security mainly. I won't have the secret in the plugin. There's no passwords handling in the plugin as well. Ideally I don't want to know the passwords at all and this is how I set up cognito.
https://docs.aws.amazon.com/cognito/latest/developerguide/what-is-amazon-cognito.html
I also wanted to minimize how hard this was for the user to login (which isn't even required, it just shows more stats for those that want it).
If you have another method that I haven't considered that would follow best practice I'm open to it, but plugins like https://github.com/cbrewitt/flipping-copilot have a different setup, but it still uses a web server regardless

You're still doing really wonky things with the executor, despite what I told you, and you telling me you've fixed it.
-I still have executor for a few things, what would you like to see changed and why?

PvPLeaderboardPlugin
Injected ScheduledExecutorService named scheduler:
scheduleAtFixedRate: 1s GC for fight state.
schedule: 1500 ms finalize guard and 2000 ms watchdog.
schedule: 15 s delayed API fallback for rank overlay when shard miss occurs.

RankOverlay
Injected ScheduledExecutorService used as executor for lookup futures. No custom thread pools. Backpressure via per-tick cap and in-flight guards. OK.

CognitoAuthService
Injected ScheduledExecutorService for token refresh scheduling (next refresh, retry). CompletableFuture for login uses default (test path), but operational flow is bound to injected scheduler for scheduled work.

DashboardPanel
One injector fetch of the shared scheduler for auth service setup only.

You've removed the warning from the plugin hub manifest - it still needs to be there.
-I've added this back in.

Thank you again for your time here, I am learning a lot going through this process with you.

@findarian
Copy link
Author

https://discord.com/channels/301497432909414422/419891709883973642/1404614275519221860

Discussion around a month ago when I was starting to make the plugin side of this project. Pajlada said this would be a good idea to put here for reference for whoever does the full review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants