-
Notifications
You must be signed in to change notification settings - Fork 0
Version/1.21.11 dev #6
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
Conversation
… handling, and update dependencies
… update project version to 1.21.11-1.0.2-SNAPSHOT
…f` for `entries`, clean up unused utilities, and optimize tablist management logic
…pdateForOthers`, adjust usages across services, and clean up tablist handling logic
…nsure base server is included in the result
…List`, ensure base server is included in the result" This reverts commit 1a92bf8.
…` and `updateForOthers`, adjust usages across services, and clean up tablist handling logic" This reverts commit f808c82.
…add `toMutableObjectList` for distinct server handling
…nces with UUID handling, and enhance Redis event processing
…service and listener logic, and improve Redis event handling
…e profile conversions
…lan integration with Redis event publishing, and enhance tablist management logic with better serialization
…nt debug prints, and streamline event processing logic
…yApi` for Redis integration, streamline `RedisApi` creation, and improve API dependency definitions
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.
Pull request overview
This PR represents a significant refactoring of the tab list plugin for version 1.21.11, transitioning from a centralized server-side tab entry management system to a more distributed, event-driven architecture. The changes simplify the codebase by removing complex server grouping logic and unused utilities.
Changes:
- Refactored tab list service to use direct Velocity tab list manipulation instead of maintaining a separate entry collection
- Introduced new Redis events for tab visibility control (hide/show) and entry updates
- Added optional integration with surf-clan-velocity plugin for clan tag display
- Updated build configuration to use surf-redis and improved publishing workflow
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle.properties | Version bump from 1.0.0 to 1.0.2-SNAPSHOT |
| build.gradle.kts | Updated gradle plugin version with dynamic selector and added publishing configuration |
| surf-tab-api/build.gradle.kts | Changed from velocity to core API and added surf-redis dependency |
| surf-tab-velocity/build.gradle.kts | Integrated surf-redis via API configuration and added optional clan plugin dependency |
| player-util.kt | Removed entire file containing Velocity-specific conversion utilities |
| formatting-util.kt | Simplified formatWithAdventure function, removed dynamic resolver and relational audience support |
| clan-member-util.kt | Added new utility to check if clan members are online |
| TablistService.kt | Complete rewrite replacing centralized entry management with direct tab list formatting |
| TabEntryRemoveRedisEvent.kt | Simplified to only contain UUID, removed server parameter |
| TabEntryAddRedisEvent.kt | Simplified to only contain UUID, removed tab entry and server parameters |
| TabRedisEventListener.kt | Added hide/show/update event handlers and refactored add/remove logic |
| ConnectionListener.kt | Simplified connection handling, removed server change tracking |
| LuckPermsHook.kt | Added async prefix/suffix retrieval and changed to publish update events via Redis |
| TablistConfig.kt | Removed nameFormat and groups configuration |
| SurfTabCommand.kt | Removed manual tab list update on reload |
| VelocityMain.kt | Added clan modification listener integration |
| TabShowRedisEvent.kt | New event for showing hidden tab entries |
| TabHideRedisEvent.kt | New event for hiding tab entries |
| TabEntryUpdateRedisEvent.kt | New event for updating existing tab entries |
| publish.yml | New GitHub Actions workflow for automated publishing |
| gradlew | Added Gradle wrapper script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private fun getClanTag(playerUuid: UUID) = | ||
| if (plugin.proxy.pluginManager.isLoaded("surf-clan-velocity")) MiniMessage.miniMessage() | ||
| .serialize( | ||
| surfClanApi.renderClanTag( | ||
| playerUuid | ||
| ) | ||
| ) else "" |
Copilot
AI
Jan 12, 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 getClanTag function checks if the surf-clan-velocity plugin is loaded, but if an error occurs while calling surfClanApi.renderClanTag (e.g., the API is not initialized), it could throw an exception. Consider adding error handling to return an empty string if the API call fails, ensuring robustness when the optional dependency is loaded but not properly initialized.
| private fun getClanTag(playerUuid: UUID) = | |
| if (plugin.proxy.pluginManager.isLoaded("surf-clan-velocity")) MiniMessage.miniMessage() | |
| .serialize( | |
| surfClanApi.renderClanTag( | |
| playerUuid | |
| ) | |
| ) else "" | |
| private fun getClanTag(playerUuid: UUID): String { | |
| if (!plugin.proxy.pluginManager.isLoaded("surf-clan-velocity")) { | |
| return "" | |
| } | |
| return try { | |
| MiniMessage.miniMessage() | |
| .serialize( | |
| surfClanApi.renderClanTag( | |
| playerUuid | |
| ) | |
| ) | |
| } catch (e: Exception) { | |
| "" | |
| } | |
| } |
| @Subscribe | ||
| fun onKick(event: KickedFromServerEvent) { | ||
| val player = event.player | ||
| val server = event.server | ||
|
|
||
| redisApi.publishEvent( | ||
| TabEntryRemoveRedisEvent( | ||
| player.uniqueId, server | ||
| ) | ||
| ) | ||
| } |
Copilot
AI
Jan 12, 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 onKick function now has an empty body. Similar to onDisconnect, if this handler is no longer needed, it should be removed entirely. Empty event handlers that remain registered can cause confusion and add unnecessary overhead.
| hiddenEntries[event.player] = (hiddenEntries[event.player] ?: mutableListOf()) + entry | ||
|
|
||
| tablistService.entries.add(event.tabEntry) | ||
| player.tabList.removeEntry(event.toHide) | ||
| } | ||
|
|
||
| visiblePlayers.forEach { | ||
| tablistService.addPlayer(it, event.tabEntry) | ||
| } | ||
| @OnRedisEvent | ||
| fun onTabShow(event: TabShowRedisEvent) { | ||
| val player = plugin.proxy.getPlayer(event.player).getOrNull() ?: return | ||
| val entry = hiddenEntries[event.player]?.find { it.profile.id == event.toShow } ?: return | ||
|
|
||
| hiddenEntries[event.player] = | ||
| hiddenEntries[event.player]?.filterNot { it.profile.id == event.toShow } | ||
| ?: mutableListOf() |
Copilot
AI
Jan 12, 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 hiddenEntries map uses a mutable list pattern but creates a new list on each update. Consider using a mutableListOf() or mutableSetOf() to avoid unnecessary list recreation. The current pattern at lines 24 and 34-36 creates new lists on every modification which is inefficient.
| SLNE_SNAPSHOTS_REPO_PASSWORD: ${{ secrets.SLNE_SNAPSHOTS_REPO_PASSWORD }} | ||
| SLNE_RELEASES_REPO_USERNAME: ${{ secrets.SLNE_RELEASES_REPO_USERNAME }} | ||
| SLNE_RELEASES_REPO_PASSWORD: ${{ secrets.SLNE_RELEASES_REPO_PASSWORD }} | ||
| MODULE_REGEX: "surf-tab-api.*-all\\.jar$|surf-tab-velocity.*-all\\.jar$" |
Copilot
AI
Jan 12, 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 variable name MODULE_REGEX suggests it should be a regex pattern, but it's actually used with grep -E which expects an extended regex pattern. The current value uses shell filename patterns (ending with $) which is correct, but the name could be more descriptive such as MODULE_JAR_PATTERN to clarify its purpose.
| fun onTabShow(event: TabShowRedisEvent) { | ||
| val player = plugin.proxy.getPlayer(event.player).getOrNull() ?: return | ||
| val entry = hiddenEntries[event.player]?.find { it.profile.id == event.toShow } ?: return | ||
|
|
||
| hiddenEntries[event.player] = | ||
| hiddenEntries[event.player]?.filterNot { it.profile.id == event.toShow } | ||
| ?: mutableListOf() | ||
|
|
||
| player.tabList.addEntry(entry) | ||
| } |
Copilot
AI
Jan 12, 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.
In the onTabShow function, if the entry is not found in hiddenEntries, the function returns early without providing any feedback. However, the subsequent code at lines 34-36 will still execute even when entry is null due to the null-safe find operation. The logic should ensure that hiddenEntries cleanup only happens when an entry is actually found and restored.
| fun onDisconnect(event: DisconnectEvent) { | ||
| val player = event.player | ||
| val server = player.currentServer.getOrNull()?.server ?: return | ||
|
|
||
| redisApi.publishEvent( | ||
| TabEntryRemoveRedisEvent( | ||
| player.uniqueId, server | ||
| ) | ||
| ) | ||
|
|
||
| } |
Copilot
AI
Jan 12, 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 onDisconnect function now has an empty body. If this is intentional and the disconnect logic has been removed, this handler should either be removed entirely or a comment should be added explaining why it's intentionally empty. Empty event handlers can be confusing for maintainability.
| return luckPerms.userManager.getUser(player) ?: luckPerms.userManager.loadUser(player) | ||
| .await() |
Copilot
AI
Jan 12, 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 getOrLoadUser function can return null from getUser and fall back to loadUser, but there's no null check after the await() call. If loadUser().await() also returns null, this will cause a NullPointerException. Consider adding a null safety check or documenting that loadUser is guaranteed to return a non-null User.
| return luckPerms.userManager.getUser(player) ?: luckPerms.userManager.loadUser(player) | |
| .await() | |
| val existingUser = luckPerms.userManager.getUser(player) | |
| if (existingUser != null) { | |
| return existingUser | |
| } | |
| val loadedUser = luckPerms.userManager.loadUser(player).await() | |
| return requireNotNull(loadedUser) { | |
| "LuckPerms returned null when loading user data for '$player'" | |
| } |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
No description provided.