Trade route capture blacklist rebase#3695
Open
scamiv wants to merge 3 commits intoopenfrontio:mainfrom
Open
Conversation
4 tasks
Contributor
WalkthroughThis PR adds trade-route blocking: when a trade ship is captured, the game can block the src→dst port route until a future tick; PortExecution excludes blocked routes from trade candidate lists and GameImpl tracks/prunes blocked routes. Changes
Sequence DiagramsequenceDiagram
participant TradeShipExec as TradeShipExecution
participant GameImpl as GameImpl
participant PortExec as PortExecution
Note over TradeShipExec,PortExec: Normal flow (route allowed)
PortExec->>GameImpl: isTradeRouteBlocked(srcId, dstId, tick)
GameImpl-->>PortExec: false
PortExec->>PortExec: include dst in trading candidates
Note over TradeShipExec,GameImpl: Ship captured by other owner
TradeShipExec->>GameImpl: blockTradeRouteUntil(srcId, dstId, tick + 100)
GameImpl->>GameImpl: store "srcId:dstId" -> expiryTick
Note over PortExec,GameImpl: Route now blocked
PortExec->>GameImpl: isTradeRouteBlocked(srcId, dstId, tick)
GameImpl-->>PortExec: true
PortExec->>PortExec: skip dst from candidates
Note over GameImpl: After expiry
PortExec->>GameImpl: isTradeRouteBlocked(srcId, dstId, tick + 101)
GameImpl->>GameImpl: prune expired entry
GameImpl-->>PortExec: false
PortExec->>PortExec: include dst again
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/PortExecution.ts (1)
125-143:⚠️ Potential issue | 🟡 MinorBlocked ports still influence proximity weighting.
After Line 126-Line 134 skips blocked routes, Line 142 still uses
iandports.lengthfrom the unfiltered list. This can accidentally remove proximity bonus from valid routes when earlier entries are blocked.💡 Suggested fix
- const weightedPorts: Unit[] = []; - - for (const [i, otherPort] of ports.entries()) { - if ( - this.mg.isTradeRouteBlocked( - this.port.id(), - otherPort.id(), - this.mg.ticks(), - ) - ) { - continue; - } + const eligiblePorts = ports.filter( + (otherPort) => + !this.mg.isTradeRouteBlocked( + this.port.id(), + otherPort.id(), + this.mg.ticks(), + ), + ); + + const weightedPorts: Unit[] = []; + for (const [i, otherPort] of eligiblePorts.entries()) { const expanded = new Array(otherPort.level()).fill(otherPort); weightedPorts.push(...expanded); const tooClose = this.mg.manhattanDist(this.port!.tile(), otherPort.tile()) < this.mg.config().tradeShipShortRangeDebuff(); const closeBonus = - i < this.mg.config().proximityBonusPortsNb(ports.length); + i < this.mg.config().proximityBonusPortsNb(eligiblePorts.length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/PortExecution.ts` around lines 125 - 143, The loop in PortExecution.ts uses the original index `i` and `ports.length` even after skipping blocked routes, which causes proximity bonuses to be calculated against the unfiltered list; fix by computing proximity using only non-blocked ports: either pre-filter `ports` with `this.mg.isTradeRouteBlocked(...)` into a new array and iterate that, or maintain a separate counter (e.g., `validIndex`) that increments only when a port is not blocked and use that plus the filtered length for the `closeBonus`/proximity calculation; update references to `i`, `ports.length`, and `proximityBonusPortsNb` accordingly so `tooClose` and `closeBonus` are evaluated against the filtered ordering.
🧹 Nitpick comments (3)
tests/core/executions/TradeShipExecution.test.ts (1)
161-173: Prefer public behavior checks over private map access.This test depends on
(game as any).tradeRouteBlockedUntil, which makes it fragile to internal renames. A spy ongame.blockTradeRouteUntilkeeps the test stable while validating the same “only once” behavior.💡 Suggested test style
it("does not add a second blacklist event when the ship is recaptured", () => { - const routeKey = `${srcPort.id()}:${dstPort.id()}`; + const blockSpy = vi.spyOn(game, "blockTradeRouteUntil"); tradeShip.owner = vi.fn(() => pirate); tradeShipExecution.tick(1); - const blockedUntil = (game as any).tradeRouteBlockedUntil.get(routeKey); tradeShip.owner = vi.fn(() => origOwner); tradeShipExecution.tick(2); - expect((game as any).tradeRouteBlockedUntil.get(routeKey)).toBe( - blockedUntil, - ); + expect(blockSpy).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/executions/TradeShipExecution.test.ts` around lines 161 - 173, The test is asserting internal state via (game as any).tradeRouteBlockedUntil; instead, spy on the public method game.blockTradeRouteUntil and verify it is only invoked once for the same route when the ship is recaptured: set a spy/mock on game.blockTradeRouteUntil before calling tradeShipExecution.tick(1), capture the routeKey argument or call count, then change tradeShip.owner and call tradeShipExecution.tick(2) and assert the spy was not called a second time (or was called exactly once with the expected routeKey). Locate uses of tradeShipExecution.tick, tradeShip.owner, and the routeKey computation to implement the spy-based assertions.tests/PortExecution.test.ts (1)
156-159: Avoid mutating private_ticksdirectly in tests.Directly changing
_ticksmakes these tests less realistic and more brittle. Prefer advancing time throughexecuteNextTick()so normal game-side effects remain in play.💡 Suggested helper
+const advanceTicks = (game: Game, count: number) => { + for (let i = 0; i < count; i++) { + game.executeNextTick(); + } +}; - (game as any)._ticks += 1; + advanceTicks(game, 1); - (game as any)._ticks += 100; + advanceTicks(game, 100);Also applies to: 181-183, 199-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/PortExecution.test.ts` around lines 156 - 159, Tests are directly mutating the private field _ticks (e.g., (game as any)._ticks += 1) which is brittle; instead advance the game clock via the public tick mechanism by calling executeNextTick() (or add a small test helper that calls executeNextTick() N times) before asserting game.isTradeRouteBlocked(port.id(), blockedPort.id(), game.ticks()); update all occurrences (including the ones noted around lines 181–183 and 199–200) to use executeNextTick() so side effects and invariants are preserved.src/core/execution/TradeShipExecution.ts (1)
10-10: Reduce coupling: avoid importing constants fromGameImplhere.
TradeShipExecutionmostly targets theGameinterface; pulling this constant fromGameImplties execution code to a concrete implementation. Consider moving the constant tosrc/core/game/Game.ts(or a small shared constants module).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TradeShipExecution.ts` at line 10, TradeShipExecution is importing TRADE_ROUTE_BLOCK_DURATION_TICKS from GameImpl which couples execution logic to a concrete implementation; move the constant into the shared Game interface or a small constants module and update references. Specifically, define TRADE_ROUTE_BLOCK_DURATION_TICKS in src/core/game/Game.ts (or a new src/core/game/constants.ts) and then change the import in TradeShipExecution to import from the Game interface module (or constants module) instead of GameImpl; ensure any other modules that used the constant are updated to the new location and exported from the Game interface module if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/execution/PortExecution.ts`:
- Around line 125-143: The loop in PortExecution.ts uses the original index `i`
and `ports.length` even after skipping blocked routes, which causes proximity
bonuses to be calculated against the unfiltered list; fix by computing proximity
using only non-blocked ports: either pre-filter `ports` with
`this.mg.isTradeRouteBlocked(...)` into a new array and iterate that, or
maintain a separate counter (e.g., `validIndex`) that increments only when a
port is not blocked and use that plus the filtered length for the
`closeBonus`/proximity calculation; update references to `i`, `ports.length`,
and `proximityBonusPortsNb` accordingly so `tooClose` and `closeBonus` are
evaluated against the filtered ordering.
---
Nitpick comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Line 10: TradeShipExecution is importing TRADE_ROUTE_BLOCK_DURATION_TICKS from
GameImpl which couples execution logic to a concrete implementation; move the
constant into the shared Game interface or a small constants module and update
references. Specifically, define TRADE_ROUTE_BLOCK_DURATION_TICKS in
src/core/game/Game.ts (or a new src/core/game/constants.ts) and then change the
import in TradeShipExecution to import from the Game interface module (or
constants module) instead of GameImpl; ensure any other modules that used the
constant are updated to the new location and exported from the Game interface
module if needed.
In `@tests/core/executions/TradeShipExecution.test.ts`:
- Around line 161-173: The test is asserting internal state via (game as
any).tradeRouteBlockedUntil; instead, spy on the public method
game.blockTradeRouteUntil and verify it is only invoked once for the same route
when the ship is recaptured: set a spy/mock on game.blockTradeRouteUntil before
calling tradeShipExecution.tick(1), capture the routeKey argument or call count,
then change tradeShip.owner and call tradeShipExecution.tick(2) and assert the
spy was not called a second time (or was called exactly once with the expected
routeKey). Locate uses of tradeShipExecution.tick, tradeShip.owner, and the
routeKey computation to implement the spy-based assertions.
In `@tests/PortExecution.test.ts`:
- Around line 156-159: Tests are directly mutating the private field _ticks
(e.g., (game as any)._ticks += 1) which is brittle; instead advance the game
clock via the public tick mechanism by calling executeNextTick() (or add a small
test helper that calls executeNextTick() N times) before asserting
game.isTradeRouteBlocked(port.id(), blockedPort.id(), game.ticks()); update all
occurrences (including the ones noted around lines 181–183 and 199–200) to use
executeNextTick() so side effects and invariants are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40b51943-5536-4072-917e-5ec2a3de3553
📒 Files selected for processing (6)
src/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/PortExecution.test.tstests/core/executions/TradeShipExecution.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/game/GameImpl.ts (2)
77-77: Small nit: export the constant where it is declared.Declaring at line 77 and re-exporting at the bottom (line 1285) splits the public surface of the module. Easier to read for everyone if you just mark it
exporton declaration and drop the trailing re-export.Proposed tweak
-const TRADE_ROUTE_BLOCK_DURATION_TICKS = 100; +export const TRADE_ROUTE_BLOCK_DURATION_TICKS = 100;-export { TRADE_ROUTE_BLOCK_DURATION_TICKS }; -Also applies to: 1285-1285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` at line 77, Export the constant at its declaration: mark TRADE_ROUTE_BLOCK_DURATION_TICKS as exported where it is declared (const TRADE_ROUTE_BLOCK_DURATION_TICKS = 100 -> export const TRADE_ROUTE_BLOCK_DURATION_TICKS = 100) and remove the redundant re-export at the bottom of the module so the public surface isn’t split between declaration and re-export; update any imports if needed to reference the single exported symbol.
543-558: Queries that mutate state are surprising.
isTradeRouteBlockeddeletes expired entries as a side effect of a read. SincepruneExpiredTradeRouteBlocksalready runs insideblockTradeRouteUntilandhash(), the delete here buys very little and makes the method non-const for callers (e.g.PortExecutioniterating candidate ports). A pure read is easier to reason about.Proposed tweak
isTradeRouteBlocked( srcPortId: number, dstPortId: number, nowTick: Tick, ): boolean { const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId); const blockedUntil = this.tradeRouteBlockedUntil.get(routeKey); - if (blockedUntil === undefined) { - return false; - } - if (blockedUntil <= nowTick) { - this.tradeRouteBlockedUntil.delete(routeKey); - return false; - } - return true; + return blockedUntil !== undefined && blockedUntil > nowTick; }If you also call
pruneExpiredTradeRouteBlocks(this._ticks)once per tick inexecuteNextTick, the map stays bounded without the read-side delete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 543 - 558, isTradeRouteBlocked currently mutates state by deleting expired entries from tradeRouteBlockedUntil; remove that side effect so the method is a pure read: drop the this.tradeRouteBlockedUntil.delete(routeKey) and simply return blockedUntil > nowTick. Ensure expired entries are pruned elsewhere by calling pruneExpiredTradeRouteBlocks(this._ticks) from the existing mutation paths (blockTradeRouteUntil and hash()) and add a single call to pruneExpiredTradeRouteBlocks(this._ticks) once per tick inside executeNextTick so the map stays bounded; refer to isTradeRouteBlocked, pruneExpiredTradeRouteBlocks, blockTradeRouteUntil, hash, executeNextTick, and tradeRouteBlockedUntil when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/GameImpl.ts`:
- Line 77: Export the constant at its declaration: mark
TRADE_ROUTE_BLOCK_DURATION_TICKS as exported where it is declared (const
TRADE_ROUTE_BLOCK_DURATION_TICKS = 100 -> export const
TRADE_ROUTE_BLOCK_DURATION_TICKS = 100) and remove the redundant re-export at
the bottom of the module so the public surface isn’t split between declaration
and re-export; update any imports if needed to reference the single exported
symbol.
- Around line 543-558: isTradeRouteBlocked currently mutates state by deleting
expired entries from tradeRouteBlockedUntil; remove that side effect so the
method is a pure read: drop the this.tradeRouteBlockedUntil.delete(routeKey) and
simply return blockedUntil > nowTick. Ensure expired entries are pruned
elsewhere by calling pruneExpiredTradeRouteBlocks(this._ticks) from the existing
mutation paths (blockTradeRouteUntil and hash()) and add a single call to
pruneExpiredTradeRouteBlocks(this._ticks) once per tick inside executeNextTick
so the map stays bounded; refer to isTradeRouteBlocked,
pruneExpiredTradeRouteBlocks, blockTradeRouteUntil, hash, executeNextTick, and
tradeRouteBlockedUntil when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5ce0667-acad-4849-9489-c0a629c01317
📒 Files selected for processing (2)
src/core/game/Game.tssrc/core/game/GameImpl.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/Game.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rebase of #3600
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME