Fix/bob various #2573
Fix/bob various #2573
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces several modifications across multiple components in the client-side trading and UI utilities. The changes primarily focus on improving resource selection, transfer, and order creation processes. Key updates include adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You are out of MentatBot reviews. Your usage will refresh December 23 at 08:00 AM. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
client/src/ui/elements/ListSelect.tsx (3)
25-27: Search input and ref
Storing searchInput in state and using a ref for the input element is logical. Consider if you need further key-based interactions or side effects.
44-53: Keyboard handling
“StopPropagation” on all keys except “Enter” is sometimes beneficial, but consider arrow-key navigation or tabbing that might be suppressed. If your design requires blocking those events, this is fine. If not, you might remove the call for better accessibility.
93-103: Search filter UI
Displaying the TextInput only if “enableFilter” is true is a great approach. The conditional rendering is straightforward and ensures no confusion for consumers not using filtering.client/src/ui/components/trading/SelectResources.tsx (1)
52-52: Layout note
Shifting to “p-3” for the container’s padding is purely stylistic but does appear consistent with spacing used elsewhere. Looks good.client/src/ui/components/worldmap/players/PlayersPanel.tsx (1)
84-84: Address matching
No issues, but confirm that toHexString handles all valid addresses, including empties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/src/ui/components/trading/MarketOrderPanel.tsx(2 hunks)client/src/ui/components/trading/SelectResources.tsx(5 hunks)client/src/ui/components/trading/TransferBetweenEntities.tsx(3 hunks)client/src/ui/components/worldmap/players/PlayersPanel.tsx(2 hunks)client/src/ui/elements/ListSelect.tsx(3 hunks)client/src/ui/utils/utils.tsx(1 hunks)
🔇 Additional comments (22)
client/src/ui/elements/ListSelect.tsx (5)
5-6: UseRef vs. forwardRef for TextInput usage
You introduce both TextInput (line 6) and a ref (line 26), but TextInput is not receiving a forwarded ref if it's a functional component. Double-check that TextInput properly consumes the ref or that you actually need it.
11-11: New optional property: searchText
The new field “searchText” is optional. Ensure that any usage of this field (for example, string transformations) gracefully handles the undefined case, as you do in line 39.
21-21: Conditional filter enabling
The new prop “enableFilter” is a neat feature. Confirm that default behavior remains intact for consumers who are not yet aware of this new prop.
35-42: Filtering logic
Your “filteredOptions” useMemo is clear and concise. However, watch out for a potential performance bottleneck if the options array grows large. Since you already useMemo with the same dependencies, you’re likely fine.
105-105: Use filteredOptions carefully
Confirm that “filteredOptions” is always non-empty before doing your .map calls in other usage contexts. The existing code is safe here, but just keep it in mind.
client/src/ui/components/trading/SelectResources.tsx (6)
8-8: Centralized resource reference
Importing RESOURCE_TIERS and ResourcesIds from “@bibliothecadao/eternum” is beneficial for maintaining a single source of truth. Good approach to ensure data consistency.
27-35: orderedResources creation
Mapping over RESOURCE_TIERS and skipping Lords is logical. However, confirm that no undefined or extraneous resource IDs slip in.
36-38: unselectedResources
You filter out selected resource IDs from orderedResources — clear approach. Ensure that any concurrency issues (like a user selecting multiple resources at the same time) are handled or that the UI prevents them.
55-56: Options array logic
Constructing the “options” array from both the found resource and unselectedResources is correct. Validate that “find” returns a valid object when the resource is already unselected.
79-82: Attach searchText for filter
Attaching “searchText” by looking up “resources.find((res) => res.id === option.id)?.trait” ensures you have the right textual label for filtering. Confirm that “resources” array has all needed traits.
94-94: enableFilter usage
Turning on filtering in this context is helpful for large resource lists. Good job integrating with the updated ListSelect.
client/src/ui/components/worldmap/players/PlayersPanel.tsx (4)
6-6: normalizeDiacriticalMarks import
Great move to unify string comparisons for diacritical marks. This approach reduces user confusion when searching for names with accents.
76-77: Normalized searchTerm
Applying “normalizeDiacriticalMarks” once at the beginning of your search is more efficient than repeating it for each comparison. Well done.
81-81: Player name comparison
Ensure that “player.name” is always a string. If it is occasionally null or undefined, consider adding a fallback within the normalization logic.
87-89: Structures array
You check each structure’s normalized name. This is consistent. Just watch for an empty structure name that could return a false positive or throw an error.
client/src/ui/components/trading/TransferBetweenEntities.tsx (4)
10-10: normalizeDiacriticalMarks import
Same usage pattern as in PlayersPanel. Good consistent approach to searching by normalized terms.
79-79: FilterEntities
You store the normalized search term in “normalizedSearchTerm” only once, which is best practice performance-wise.
83-85: Account name check
Your “entity.accountName” check is a subtle but important addition. This supports searching by account name as well. Good job.
324-334: Button layout changes
Switching to a 4-column grid with a single-col “Back” button and a 3-col “Confirm Transfer” aligns the UI better. This is more user-friendly.
client/src/ui/utils/utils.tsx (1)
534-536: LGTM! Clean implementation of diacritical mark normalization.
The function correctly uses Unicode normalization (NFD) and proper regex pattern to handle diacritical marks.
client/src/ui/components/trading/MarketOrderPanel.tsx (2)
454-454: Improved order confirmation logic.
The disabled condition now correctly handles buy orders by only checking donkey balance for sell orders. This fixes a potential UX issue where buy orders might have been incorrectly disabled due to donkey balance checks.
603-603: LGTM! Fixed edge case in donkey balance check.
The comparison now correctly allows orders when the donkey balance exactly matches the required amount.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/ui/modules/navigation/LeftNavigationModule.tsx (2)
Line range hint
259-259: Clean up commented code.Consider removing the commented-out
<EventStream />component if it's no longer needed, or add a TODO comment explaining why it's temporarily disabled.
Line range hint
64-75: Document keyboard shortcuts.Consider adding JSDoc comments or user documentation for the keyboard shortcuts. Currently, only the 'E' key for toggling entity view is implemented, but the event listener setup suggests more shortcuts might be added in the future.
Example documentation:
/** * Keyboard shortcuts: * - E: Toggle entity view */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/ui/modules/navigation/LeftNavigationModule.tsx(1 hunks)
🔇 Additional comments (1)
client/src/ui/modules/navigation/LeftNavigationModule.tsx (1)
129-129: LGTM! Verify construction menu accessibility.
The disabled condition !structureIsMine || !isRealm is logically correct, ensuring that the construction menu is only accessible for realms owned by the player.
Let's verify the construction menu accessibility conditions:
✅ Verification successful
Construction menu access control is consistently implemented
The verification confirms that the disabled condition !structureIsMine || !isRealm for the construction menu is correctly implemented and aligns with other access controls in the module:
- The construction menu button is specifically restricted to owned realms
- Other navigation buttons (military, trade, world structures) only check for
structureIsMine - The realm check is consistently implemented across related components using
structureCategory === "Realm"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the construction menu accessibility conditions
# Look for other instances where structureIsMine or isRealm are used to control access
# Search for structureIsMine usage in conditional statements
rg -A 3 "structureIsMine.*\?"
# Search for isRealm usage in conditional statements
rg -A 3 "isRealm.*\?"
# Search for other construction-related access controls
ast-grep --pattern 'disabled={$_}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to verify the construction menu accessibility
# Search for structureIsMine without the question mark
rg "structureIsMine" -A 3
# Search for disabled prop in the LeftNavigationModule
rg "disabled=" client/src/ui/modules/navigation/LeftNavigationModule.tsx -A 2
# Look for construction menu related components and their access controls
rg "ConstructionView" -A 3
# Search for realm-related conditions
rg "structureCategory.*Realm" -A 2
Length of output: 6284
Closes #2493
Closes #2505
Closes #2519
Closes #2559
Summary by CodeRabbit
New Features
Bug Fixes
Style