🚀 Implement pagination for contract list retrieval#1927
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements pagination for the contract list retrieval functionality to handle large numbers of contracts more efficiently. The implementation adds automatic cycling through contract pages with a time-based reset mechanism.
Key Changes:
- Added pagination logic that displays 10 contracts at a time with automatic page advancement
- Introduced global state variables to track pagination position and timing across function calls
- Modified guild filtering logic with a special-case hardcoded guild ID
| } | ||
|
|
||
| i := 1 | ||
| if time.Since(lastContractListTime) > 1*time.Minute { |
There was a problem hiding this comment.
The magic number 1*time.Minute should be extracted to a named constant to improve maintainability and make the pagination timeout configuration more explicit.
| contractList := make([]*Contract, 0) | ||
| idx := 0 | ||
| for _, c := range Contracts { | ||
| if guildID != "" && c.Location[0].GuildID != guildID { | ||
| continue | ||
| if idx >= startIdx && idx < endIdx { | ||
| contractList = append(contractList, c) | ||
| } | ||
| idx++ | ||
| } |
There was a problem hiding this comment.
The pagination implementation iterates through all contracts in the map to build the paginated slice, even though only a subset is needed. For large contract maps, this is inefficient. Since Go maps don't have guaranteed ordering, and you're using index-based pagination, consider either using a slice-backed data structure for Contracts or implementing a more efficient pagination approach that doesn't require iterating through all entries.
| var lastContractListTime time.Time | ||
| var lastContractListIndex int | ||
|
|
||
| // getContractList returns a list of all contracts within the specified guild |
There was a problem hiding this comment.
The function comment should be updated to document the new pagination behavior, including the fact that it maintains state across calls using global variables and automatically cycles through pages based on time-based reset logic.
| // getContractList returns a list of all contracts within the specified guild | |
| // getContractList returns a paginated list of contracts for the specified guild. | |
| // Pagination state is maintained across calls using the global variables | |
| // lastContractListTime and lastContractListIndex. The function automatically | |
| // cycles through pages on each call, and resets to the first page if more than | |
| // one minute has passed since the last call. Returns a string, a Discord embed | |
| // message, and an error. |
| var lastContractListTime time.Time | ||
| var lastContractListIndex int |
There was a problem hiding this comment.
These global variables are not thread-safe and can cause race conditions. Since HandleAdminContractList is called in response to Discord interactions, multiple users could trigger this command simultaneously. This would result in concurrent access to these variables without synchronization, leading to data races where one call could overwrite or read inconsistent values from another call's execution. Consider using a mutex to protect access to these variables or use per-user/per-guild state instead of global state.
| contractList := make([]*Contract, 0) | ||
| idx := 0 | ||
| for _, c := range Contracts { | ||
| if guildID != "" && c.Location[0].GuildID != guildID { | ||
| continue | ||
| if idx >= startIdx && idx < endIdx { | ||
| contractList = append(contractList, c) | ||
| } | ||
| idx++ | ||
| } | ||
|
|
||
| // Update index for next call | ||
| lastContractListIndex = endIdx | ||
|
|
||
| i := 1 + startIdx | ||
| for _, c := range contractList { | ||
| if guildID != "766330702689992720" { | ||
| if guildID != "" && c.Location[0].GuildID != guildID { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The pagination logic slices from the Contracts map before applying the guild filter. This means that when filtering by guildID, the returned page may contain fewer than 10 contracts (or even zero contracts), yet the pagination index still advances by 10. This will result in contracts being skipped in subsequent pages. The guild filtering should be applied before pagination, or pagination should account for filtered results.
| if guildID != "766330702689992720" { | ||
| if guildID != "" && c.Location[0].GuildID != guildID { |
There was a problem hiding this comment.
This hardcoded guild ID appears to be a special case that bypasses guild filtering entirely, but the logic is incorrect. When guildID equals "766330702689992720", the condition evaluates to false, so the inner filtering logic is skipped, showing all contracts. However, when guildID is any other value including an empty string, the filtering logic runs. This makes the empty string case ("" != "766330702689992720" is true) apply filtering, which is inconsistent with the original behavior where an empty guildID would show all contracts.
| if guildID != "766330702689992720" { | |
| if guildID != "" && c.Location[0].GuildID != guildID { | |
| if guildID != "766330702689992720" && guildID != "" { | |
| if c.Location[0].GuildID != guildID { |
|
|
||
| i := 1 + startIdx | ||
| for _, c := range contractList { | ||
| if guildID != "766330702689992720" { |
There was a problem hiding this comment.
This hardcoded guild ID should be extracted to a named constant or configuration value to improve maintainability and make the special-case logic more clear. Magic values like this make the code harder to understand and modify.
| } | ||
|
|
||
| // Calculate window start and end | ||
| windowSize := 10 |
There was a problem hiding this comment.
The magic number 10 for window size should be extracted to a named constant to improve maintainability and make it easier to adjust the pagination size in the future.
No description provided.