Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces an advanced mode feature across the client, enabling combat simulation with Web Workers, gear preset application, exploration risk analytics, and enhanced UI elements. It adds comprehensive combat outcome calculation (deterministic and Monte Carlo), exploration lethal-chance computations, gear suggestion workflows, and active player tracking. The changes refactor payment flow to use numeric identifiers, rename mobile component pairs (GameSettings* → CustomizeGame*), and conditionally render advanced analytics and controls throughout desktop and mobile overlays. Changes
Sequence DiagramssequenceDiagram
participant Client as Client Code
participant Hook as useCombatSimulation Hook
participant Worker as Combat Worker
participant Fallback as Inline Fallback
participant Core as Combat Core
Client->>Hook: useCombatSimulation(adventurer, beast, bag)
Hook->>Hook: Compute state hash
alt Hash unchanged
Hook->>Client: Return cached result
else Hash changed
Hook->>Hook: Debounce (100ms)
Hook->>Worker: postMessage({adventurer, beast, options})
alt Worker available & responsive
Worker->>Core: calculateCombatResult()
Core-->>Worker: result
Worker-->>Hook: postMessage(result)
Hook->>Client: Return simulationResult
else Worker error or timeout
Hook->>Fallback: runSimulationInline()
alt No stack overflow
Fallback->>Core: calculateCombatResult()
Core-->>Fallback: result
Fallback-->>Hook: result
Hook->>Client: Return simulationResult
else Stack overflow
Hook->>Client: Return defaultSimulationResult
end
end
end
sequenceDiagram
participant UI as Desktop Combat UI
participant Sim as useCombatSimulation
participant State as Game State
participant Display as Combat Insights Panel
UI->>Sim: Initialize with adventurer, beast, bag
Sim->>Sim: Run simulation (worker or inline)
Sim-->>UI: simulationResult { winRate, OTK, damageDistribution, rounds, xp, gold }
UI->>UI: Calculate Net HP (potions - damage)
UI->>Display: Render Fight/Flee/Gamble probabilities
Display->>Display: Highlight winning outcome
alt User clicks Fight
UI->>State: dispatch attack
State-->>UI: New state + events
else User clicks Auto Last-Hit
UI->>UI: Enter auto-loop until final round
UI->>UI: Continue attack sequence
end
sequenceDiagram
participant Client as Client Code
participant Hook as useExplorationWorker Hook
participant Worker as Exploration Worker
participant RNG as Deterministic RNG
Client->>Hook: useExplorationWorker(adventurer, gameSettings)
Hook->>Hook: Compute input hash
alt Input unchanged
Hook->>Client: Return cached (ambushLethalChance, trapLethalChance)
else Input changed
Hook->>Hook: Debounce (50ms)
Hook->>Worker: postMessage({adventurer, gameSettings})
Worker->>RNG: Initialize seed from adventurer state
Worker->>Worker: Monte Carlo sampling (slots: armor slots + neck)
loop For each slot
Worker->>Worker: Simulate beast/obstacle damage with specials
Worker->>Worker: Aggregate lethal outcomes
end
Worker-->>Hook: postMessage({ambushLethalChance, trapLethalChance})
Hook->>Client: Update result state
Client->>Client: Render lethal-chance panel in advanced mode
end
sequenceDiagram
participant UI as Desktop Inventory UI
participant Hook as useGearPresets Hook
participant Preset as Preset Engine
participant Store as Game Store
UI->>Hook: useGearPresets(adventurer, bag, enabled=true)
Hook->>Hook: Compute inventory hash
alt Inventory unchanged
Hook->>UI: Return cached presets
else Inventory changed
Hook->>Preset: applyGearPreset(adventurer, bag, 'cloth')
Preset->>Preset: Aggregate items by slot
Preset->>Preset: Select best by level/power per slot
Preset-->>Hook: { adventurer, bag, hasChanges: true }
Hook->>Preset: applyGearPreset(adventurer, bag, 'hide')
Preset-->>Hook: result
Hook->>Preset: applyGearPreset(adventurer, bag, 'metal')
Preset-->>Hook: result
Hook->>UI: Return { presets: { cloth, hide, metal }, ... }
end
UI->>UI: Render 3 preset buttons in advanced mode
alt User clicks CLOTH preset
UI->>Store: equipGearPreset('cloth')
Store->>Store: Apply preset + update bag
Store-->>UI: New adventurer state
UI->>UI: Re-render with updated gear
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @Await-0x's task —— View job Pull Request ReviewI've completed my review of this large PR (48 files, 8594 additions). Here's my comprehensive analysis: Review Checklist
🎯 Overall AssessmentThis is a substantial feature addition that introduces combat simulation, gear optimization, and exploration enhancements. The code quality is generally good, but there are several important concerns that should be addressed. 🔴 Critical Issues1. Zero Test Coverage
2. Worker Security ConcernsFile:
🟠 High Priority Issues3. Performance ConcernsFile: export const MAX_DETERMINISTIC_STATE_VISITS = 80_000;
export const DEFAULT_MONTE_CARLO_SAMPLES = 10_000;
File:
4. Memory Management IssuesFile: interface StateOutcome {
damageDealtDistribution: Map<number, number>;
damageTakenDistribution: Map<number, number>;
roundsDistribution: Map<number, number>;
}
5. State Management ComplexityFile:
🟡 Medium Priority Issues6. Error Handling GapsFile: } catch (error) {
console.error('[CombatSimulation] Error:', error);
}
7. Magic Numbers and ConstantsFile:
8. Type Safety ImprovementsFile:
🟢 Positive Aspects1. Excellent Performance Optimization
2. Clean Architecture
3. TypeScript Usage
📋 Specific Recommendations
🔍 Code Quality
📈 Risk Assessment
Recommendation: Address critical test coverage and performance issues before merging to main. |
|
Claude finished @Await-0x's task —— View job PR Review AnalysisReviewing this large PR with 8,596 additions across 48 files. I'll analyze the code quality, potential issues, and provide comprehensive feedback. Todo List:
🔍 Comprehensive Review Findings✅ StrengthsArchitecture & Performance
Code Quality
|
Summary of ChangesHello @Await-0x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the game's analytical and strategic capabilities by introducing an 'Advanced Mode'. This mode provides players with in-depth combat and exploration data, intelligent gear recommendations, and live game monitoring. The changes involve extensive refactoring and the integration of Web Workers to maintain high performance while delivering complex calculations and detailed feedback. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new "Advanced Mode" to the application, adding a rich set of features for power users. Key additions include detailed combat simulations, exploration risk analysis, automatic gear suggestions, and live spectating of active games. To handle the increased computational load, several features have been implemented using Web Workers, which is a great approach for maintaining UI responsiveness.
The review identifies several areas for improvement, primarily related to code duplication between desktop and mobile components, and some opportunities to enhance code quality by removing magic numbers and improving type safety. There are also a couple of high-severity issues related to an inefficient data query and a hardcoded table name that should be addressed to ensure performance and maintainability.
Overall, this is a substantial and impressive feature addition. The suggestions provided aim to refine the implementation, improve code reuse, and ensure the long-term health of the codebase.
| useEffect(() => { | ||
| if (!advancedMode || !autoLastHitActive || !untilLastHit) { | ||
| return; | ||
| } | ||
|
|
||
| if (!adventurer || !beast) { | ||
| setAutoLastHitActive(false); | ||
| setLastHitActionCount(null); | ||
| setAttackInProgress(false); | ||
| return; | ||
| } | ||
|
|
||
| if (!simulationResult.hasOutcome) { | ||
| return; | ||
| } | ||
|
|
||
| if (isFinalRound) { | ||
| setAutoLastHitActive(false); | ||
| setLastHitActionCount(null); | ||
| setAttackInProgress(false); | ||
| return; | ||
| } | ||
|
|
||
| if (lastHitActionCount !== null && adventurer.action_count <= lastHitActionCount) { | ||
| return; | ||
| } | ||
|
|
||
| if (simulationActionCount === null || simulationActionCount !== adventurer.action_count) { | ||
| return; | ||
| } | ||
|
|
||
| setLastHitActionCount(adventurer.action_count); | ||
|
|
||
| const continueAttacking = async () => { | ||
| try { | ||
| setAttackInProgress(true); | ||
| setCombatLog(attackMessage); | ||
| await executeGameAction({ type: 'attack', untilDeath: false }); | ||
| } catch (error) { | ||
| console.error('Failed to continue last-hit attack', error); | ||
| setAutoLastHitActive(false); | ||
| setLastHitActionCount(null); | ||
| setAttackInProgress(false); | ||
| } | ||
| }; | ||
|
|
||
| void continueAttacking(); | ||
| }, [ | ||
| advancedMode, | ||
| autoLastHitActive, | ||
| untilLastHit, | ||
| simulationResult.hasOutcome, | ||
| simulationResult.maxRounds, | ||
| isFinalRound, | ||
| executeGameAction, | ||
| adventurer?.action_count, | ||
| beast?.id, | ||
| ]); |
There was a problem hiding this comment.
The 'auto last hit' logic in this useEffect is duplicated from the desktop CombatOverlay. This complex logic should be extracted into a shared custom hook to avoid duplication and make both components simpler and more maintainable.
| const gameEventsQuery = (gameId: number) => { | ||
| return new GameQueryBuilder() | ||
| .withClause( | ||
| new ClauseBuilder().keys( | ||
| [ | ||
| `${currentNetworkConfig.namespace}-GameEvent`, | ||
| ], | ||
| [addAddressPadding(`0x${gameId.toString(16)}`)] | ||
| ).build() | ||
| ) | ||
| .withEntityModels([ | ||
| `${currentNetworkConfig.namespace}-GameEvent`, | ||
| ]) | ||
| return new HistoricalToriiQueryBuilder() | ||
| .addEntityModel(`${currentNetworkConfig.namespace}-GameEvent`) | ||
| .withLimit(1) | ||
| .includeHashedKeys(); | ||
| }; |
There was a problem hiding this comment.
The query for game events has been changed to use HistoricalToriiQueryBuilder, but it no longer filters by gameId on the server side. The new query will fetch all GameEvent entities, and the filtering is done on the client. This is inefficient and can cause performance issues, especially with many active games, as it subscribes to a much larger set of events than necessary. The previous implementation using ClauseBuilder with keys was more efficient as it filtered events at the source (Torii).
|
|
||
| let url = `${SQL_ENDPOINT}/sql?query= | ||
| SELECT id, player_name | ||
| FROM "relayer_0_0_1-TokenPlayerNameUpdate" |
There was a problem hiding this comment.
The table name "relayer_0_0_1-TokenPlayerNameUpdate" is hardcoded. This is brittle and might break if the relayer version or namespace changes. It would be better to construct this table name dynamically, possibly using the namespace from the network configuration, to make it more robust.
| useEffect(() => { | ||
| if (!advancedMode) { | ||
| setSimulationResult(defaultSimulationResult); | ||
| setSimulationActionCount(null); | ||
| return; | ||
| } | ||
|
|
||
| let cancelled = false; | ||
|
|
||
| if (!adventurer || !beast) { | ||
| setSimulationResult(defaultSimulationResult); | ||
| setSimulationActionCount(null); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| } | ||
|
|
||
| const runSimulation = async () => { | ||
| const result = await simulateCombatOutcomes(adventurer, beast, { | ||
| initialBeastStrike: hasNewItemsEquipped, | ||
| }); | ||
|
|
||
| if (!cancelled) { | ||
| setSimulationResult(result); | ||
| setSimulationActionCount(adventurer.action_count); | ||
| } | ||
| }; | ||
|
|
||
| void runSimulation(); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [ | ||
| advancedMode, | ||
| adventurer?.health, | ||
| adventurer?.xp, | ||
| adventurer?.item_specials_seed, | ||
| adventurer?.stats.strength, | ||
| adventurer?.stats.luck, | ||
| adventurer?.equipment.weapon.id, | ||
| adventurer?.equipment.weapon.xp, | ||
| adventurer?.equipment.chest.id, | ||
| adventurer?.equipment.chest.xp, | ||
| adventurer?.equipment.head.id, | ||
| adventurer?.equipment.head.xp, | ||
| adventurer?.equipment.waist.id, | ||
| adventurer?.equipment.waist.xp, | ||
| adventurer?.equipment.hand.id, | ||
| adventurer?.equipment.hand.xp, | ||
| adventurer?.equipment.foot.id, | ||
| adventurer?.equipment.foot.xp, | ||
| adventurer?.equipment.neck.id, | ||
| adventurer?.equipment.neck.xp, | ||
| adventurer?.equipment.ring.id, | ||
| adventurer?.equipment.ring.xp, | ||
| adventurer?.beast_health, | ||
| beast?.health, | ||
| beast?.level, | ||
| beast?.tier, | ||
| beast?.specialPrefix, | ||
| beast?.specialSuffix, | ||
| hasNewItemsEquipped, | ||
| ]); |
There was a problem hiding this comment.
This useEffect block re-implements the combat simulation logic that is already encapsulated in the useCombatSimulation hook used in the desktop CombatOverlay. To reduce code duplication and improve maintainability, this component should also use the useCombatSimulation hook.
| @@ -42,15 +43,14 @@ export default function AdventurersList({ onBack }: AdventurersListProps) { | |||
| }); | |||
| const [gameTokens, setGameTokens] = useState<any[]>([]); | |||
There was a problem hiding this comment.
The state gameTokens is typed as any[], and any is used in several places for the game object. Using any undermines TypeScript's type safety. It would be beneficial to define a specific type for the game token object to improve type safety and code clarity.
For example:
interface GameToken {
adventurer_id: number;
dead: boolean;
expired: boolean;
available_at: number;
// ... other properties
}
const [gameTokens, setGameTokens] = useState<GameToken[]>([]);| const dungeon = useDungeon(); | ||
| const { getActivePlayersCount, getPlayerNames } = useGameTokens(); | ||
| const [left, setLeft] = useState(getMenuLeftOffset()); | ||
| const [playersOnline, setPlayersOnline] = useState<any[] | null>(null); |
There was a problem hiding this comment.
The state playersOnline is typed as any[] | null, and any is used for player objects throughout the component. This reduces type safety. Defining a specific interface for the player data would make the code more robust and easier to understand.
For example:
interface PlayerData {
adventurer_id: string;
details?: {
adventurer?: {
health: number;
xp: number;
};
};
// ... other properties
}
const [playersOnline, setPlayersOnline] = useState<PlayerData[] | null>(null);|
|
||
| openProfile: () => (connector as any)?.controller?.openProfile(), | ||
| openBuyTicket: () => (connector as any)?.controller?.openStarterPack("ls2-dungeon-ticket-mainnet"), | ||
| openBuyTicket: () => (connector as any)?.controller?.openStarterPack(3), |
There was a problem hiding this comment.
Using the magic number 3 for the starter pack ID makes the code harder to understand and maintain. It's better to define this as a named constant (e.g., DUNGEON_TICKET_STARTER_PACK_ID) to improve readability and make future updates easier.
| </Typography> | ||
| </Box> | ||
| {adventurer?.item_specials_seed !== 0 && (() => { | ||
| const specials = ItemUtils.getSpecials(item.id, 15, adventurer!.item_specials_seed); |
There was a problem hiding this comment.
The magic number 15 is used here for the greatness level to get item specials. This value is defined as SUFFIX_UNLOCK_GREATNESS in client/src/constants/loot.ts. Using the constant (after importing it) would improve readability and maintainability.
| const specials = ItemUtils.getSpecials(item.id, 15, adventurer!.item_specials_seed); | |
| const specials = ItemUtils.getSpecials(item.id, SUFFIX_UNLOCK_GREATNESS, adventurer!.item_specials_seed); |
| const renderAdvancedStatsView = () => ( | ||
| <> | ||
| <Box sx={styles.statGrid}> | ||
| {['strength', 'dexterity', 'vitality', 'intelligence', 'wisdom', 'charisma'].map((stat) => { | ||
| const totalStatValue = adventurer?.stats?.[stat as keyof typeof STAT_DESCRIPTIONS]! + selectedStats[stat as keyof typeof STAT_DESCRIPTIONS]!; | ||
| const effectText = STAT_HELPER_TEXT(stat, totalStatValue); | ||
|
|
||
| return ( | ||
| <Box sx={styles.statRow} key={stat}> | ||
| <Box sx={styles.statInfo}> | ||
| <Typography sx={styles.statLabel}>{STAT_TITLE(stat)}</Typography> | ||
| {effectText && ( | ||
| <Typography sx={styles.statEffect}>{effectText}</Typography> | ||
| )} | ||
| </Box> | ||
| <Box sx={styles.statControls}> | ||
| {adventurer?.stat_upgrades_available! > 0 && stat !== 'luck' && <Button | ||
| variant="contained" | ||
| size="small" | ||
| onClick={() => handleStatDecrement(stat as keyof typeof STAT_DESCRIPTIONS)} | ||
| sx={styles.controlButton} | ||
| > | ||
| - | ||
| </Button>} | ||
|
|
||
| <Typography sx={styles.statValue}> | ||
| {totalStatValue} | ||
| </Typography> | ||
|
|
||
| {adventurer?.stat_upgrades_available! > 0 && stat !== 'luck' && <Button | ||
| variant="contained" | ||
| size="small" | ||
| onClick={() => handleStatIncrement(stat as keyof typeof STAT_DESCRIPTIONS)} | ||
| disabled={(adventurer!.stats[stat as keyof typeof STAT_DESCRIPTIONS] + selectedStats[stat as keyof typeof STAT_DESCRIPTIONS]) >= (MAX_STAT_VALUE + equippedItemStats[stat as keyof typeof STAT_DESCRIPTIONS])} | ||
| sx={styles.controlButton} | ||
| > | ||
| + | ||
| </Button>} | ||
| </Box> | ||
| </Box> | ||
| ); | ||
| })} | ||
| </Box> | ||
|
|
||
| <Box sx={{ display: 'flex', justifyContent: 'center', alignItems: 'center', mt: 0.5 }}> | ||
| {adventurer?.stat_upgrades_available! > 0 && | ||
| <Typography color="secondary" >{pointsRemaining} remaining</Typography> | ||
| } | ||
| </Box> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
This function renderAdvancedStatsView and other parts of the component use non-null assertions (!) on adventurer. While the parent component might ensure adventurer is not null, it's safer and cleaner to add a guard clause at the beginning of the AdventurerStats component to handle the case where adventurer might be null. This makes the component more robust and self-contained.
For example:
export default function AdventurerStats() {
const { adventurer, ... } = useGameStore();
// ... other hooks
if (!adventurer) {
return null; // or a loading/placeholder component
}
// ... rest of the component logic without non-null assertions
}| const handleSuggestGear = async () => { | ||
| if (!adventurer || !bag || !beast) { | ||
| return; | ||
| } | ||
|
|
||
| setSuggestInProgress(true); | ||
| setSuggestMessage(null); | ||
|
|
||
| try { | ||
| const suggestion = await suggestBestCombatGear(adventurer, bag, beast); | ||
|
|
||
| if (!suggestion) { | ||
| setSuggestMessage('Best gear already equipped'); | ||
| setTimeout(() => setSuggestMessage(null), 4000); | ||
| return; | ||
| } | ||
|
|
||
| applyGearSuggestion({ adventurer: suggestion.adventurer, bag: suggestion.bag }); | ||
| setSuggestMessage('Gear equipped'); | ||
| setTimeout(() => setSuggestMessage(null), 4000); | ||
| } catch (error) { | ||
| console.error('Failed to suggest combat gear', error); | ||
| setSuggestMessage('Unable to suggest gear'); | ||
| setTimeout(() => setSuggestMessage(null), 4000); | ||
| } finally { | ||
| setSuggestInProgress(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The logic for setting and clearing the suggestion message with a timeout is repeated multiple times. You could extract this into a helper function to reduce duplication and improve readability.
For example:
const showTemporaryMessage = (message: string, duration: number = 4000) => {
setSuggestMessage(message);
setTimeout(() => setSuggestMessage(null), duration);
};Then you can call showTemporaryMessage('Best gear already equipped'); inside your try...catch block.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.