Open
Conversation
Contributor
WalkthroughAdds utilities Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
🧹 Nitpick comments (1)
src/core/Util.ts (1)
95-101:findClosestByis a thin alias overfindMinimumBy— consider whether both exports are needed long-term.Right now
findClosestByjust forwards tofindMinimumBywith zero added logic. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites — "closest by distance" reads more naturally than "minimum by score" when dealing with spatial lookups. Just keep in mind that maintaining two nearly identical public exports adds surface area. IffindMinimumBynever gets used directly outside offindClosestBy, you could collapse them later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 95 - 101, findClosestBy is a trivial wrapper around findMinimumBy with no added behavior (only a parameter rename), which risks duplicated public surface area; either remove the redundant export or make the intent explicit: search the repo for external usages of findClosestBy and findMinimumBy and if both are used keep both, otherwise collapse into a single export (prefer the clearer name for call sites, e.g., keep findClosestBy and remove or inline findMinimumBy), or add a short comment above findClosestBy documenting it's an intentional alias for readability; update any imports accordingly to use the chosen single function (references: findClosestBy, findMinimumBy).
🤖 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/Util.ts`:
- Around line 95-101: findClosestBy is a trivial wrapper around findMinimumBy
with no added behavior (only a parameter rename), which risks duplicated public
surface area; either remove the redundant export or make the intent explicit:
search the repo for external usages of findClosestBy and findMinimumBy and if
both are used keep both, otherwise collapse into a single export (prefer the
clearer name for call sites, e.g., keep findClosestBy and remove or inline
findMinimumBy), or add a short comment above findClosestBy documenting it's an
intentional alias for readability; update any imports accordingly to use the
chosen single function (references: findClosestBy, findMinimumBy).
Collaborator
|
looks like a test is failing |
evanpelle
reviewed
Feb 20, 2026
…redicate is provided
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/Util.ts (1)
110-116:findClosestByis a pure alias — consider whether it pulls its own weight.Right now
findClosestByadds no behaviour on top offindMinimumBy; they share the same signature and the body is a single delegation line. Two options worth considering:
- Keep as-is — the distinct name signals "this is a spatial distance query" at call sites, which is fine.
- Remove
findClosestByand have callers importfindMinimumBydirectly, reducing surface area.Either is valid; this is just a nudge to settle the "possible consolidation" point the PR already raises before more callers are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 110 - 116, findClosestBy is a no-op alias of findMinimumBy; decide whether to remove it or clarify intent: either delete the findClosestBy function and replace all its call sites/imports to use findMinimumBy (update tests and exports accordingly), or keep findClosestBy but add a brief doc comment above the function explaining it's a semantic alias for spatial/distance queries so callers understand the intent and keep the single-line delegation as-is; reference the symbols findClosestBy and findMinimumBy when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/Util.ts`:
- Around line 102-109: The JSDoc example for findClosestBy uses the incorrect
GameMap variable name "mg" — change it to "gm" to match the file's convention
and other helpers like distSort and distSortUnit; update the example line
containing mg.manhattanDist(...) to use gm.manhattanDist(...) so examples are
consistent and copy-pasteable for readers referencing findClosestBy.
---
Nitpick comments:
In `@src/core/Util.ts`:
- Around line 110-116: findClosestBy is a no-op alias of findMinimumBy; decide
whether to remove it or clarify intent: either delete the findClosestBy function
and replace all its call sites/imports to use findMinimumBy (update tests and
exports accordingly), or keep findClosestBy but add a brief doc comment above
the function explaining it's a semantic alias for spatial/distance queries so
callers understand the intent and keep the single-line delegation as-is;
reference the symbols findClosestBy and findMinimumBy when making the change.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/game/PlayerImpl.ts (1)
1183-1193:warshipSpawnstill usesthis.mginstead of a local cache.Not a bug, but
nukeSpawncachesthis.mginto a localmgfor speed, whilewarshipSpawndoes not. If this method is also on a hot path, the same local-cache pattern would be consistent. Low priority sincewarshipSpawndoes fewer lookups.Optional: cache mg locally for consistency
warshipSpawn(tile: TileRef): TileRef | false { - if (!this.mg.isOcean(tile)) { + const mg = this.mg; + if (!mg.isOcean(tile)) { return false; } - const bestPort = findClosestBy(this.units(UnitType.Port), (port) => - this.mg.manhattanDist(port.tile(), tile), + const bestPort = findClosestBy(this.units(UnitType.Port), (port) => + mg.manhattanDist(port.tile(), tile), ); return bestPort?.tile() ?? false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/PlayerImpl.ts` around lines 1183 - 1193, warshipSpawn currently accesses this.mg repeatedly; mirror the nukeSpawn optimization by caching this.mg into a local const (e.g., const mg = this.mg) at the top of the warshipSpawn method and then use mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are consistent and slightly faster on hot paths.src/core/Util.ts (1)
102-116:findClosestByis a pure alias forfindMinimumBy— consider if both are needed.Right now
findClosestByjust callsfindMinimumBywith identical signature/behavior. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites, but be aware the two could drift apart if someone later adds logic to only one of them.If the intent is purely a naming alias, a brief inline comment like
// Semantic alias – keep in sync with findMinimumBywould help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 102 - 116, findClosestBy currently just forwards to findMinimumBy and should be documented as an intentional semantic alias to avoid future drift; update the findClosestBy declaration to include a short inline comment like "// Semantic alias of findMinimumBy — keep behavior/signature in sync" (or alternatively remove the alias and replace call sites with findMinimumBy if duplication isn't desired), referencing the functions findClosestBy and findMinimumBy so readers know this is deliberate.
🤖 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/PlayerImpl.ts`:
- Around line 1183-1193: warshipSpawn currently accesses this.mg repeatedly;
mirror the nukeSpawn optimization by caching this.mg into a local const (e.g.,
const mg = this.mg) at the top of the warshipSpawn method and then use
mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave
findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are
consistent and slightly faster on hot paths.
In `@src/core/Util.ts`:
- Around line 102-116: findClosestBy currently just forwards to findMinimumBy
and should be documented as an intentional semantic alias to avoid future drift;
update the findClosestBy declaration to include a short inline comment like "//
Semantic alias of findMinimumBy — keep behavior/signature in sync" (or
alternatively remove the alias and replace call sites with findMinimumBy if
duplication isn't desired), referencing the functions findClosestBy and
findMinimumBy so readers know this is deliberate.
…or map "ocean_and_land" which lead to NaN as outcome of manhattanDist. Until now we used distSortUnit which just returned the first and only port it was given in the test, even though manhattanDist both returned NaN. Now with findClosestBy we need it to have actual existing tiles because we need actual comparable number to come out of manhattanDist.) Also for TradeShipExecution use some already cached values as input for findClosestBy.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 87-97: The nearest-port lookup in TradeShipExecution (using
findClosestBy over tradeShipOwner.units(UnitType.Port)) does not exclude
inactive/captured ports, so a captured ship can target an inactive port
indefinitely; update the call to findClosestBy to pass a candidate filter that
only returns ports which are active (e.g., port.isActive() or !port.captured)
when iterating units, and if that filtered search returns null behave as before
by calling this.tradeShip.delete(false), setting this.active = false and
returning; ensure you update references around nearestPort, this._dstPort and
this.tradeShip.setTargetUnit to only run when a valid active port is found.
…eletion, still under construction (even if it might be finished when we arrive, we don't know that) or is inactive.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 87-103: The port mocks used in tests (dstPort, piratePort,
piratePort2, srcPort) are missing stubs for Unit methods isMarkedForDeletion()
and isUnderConstruction(), causing CI failures when TradeShipExecution calls
these; update the test beforeEach where those port mocks are created to add
vi.fn() stubs for both methods on each mock (e.g., isMarkedForDeletion: vi.fn(()
=> false), isUnderConstruction: vi.fn(() => false)) so findClosestBy/filtering
in TradeShipExecution works as expected.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 87-103: Tests are failing because the port mocks used in
TradeShipExecution.test.ts lack the new filter-method stubs; in the beforeEach
add isMarkedForDeletion() and isUnderConstruction() stubs on each port mock
(dstPort, srcPort, piratePort, piratePort2) returning appropriate values (e.g.,
false) so findClosestBy's filter in TradeShipExecution (which calls
isMarkedForDeletion and isUnderConstruction) can run without error.
… make test happy...
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.
Description:
Performance improvements.
Some of this goes along with PR #3220 since playerImpl buildableUnits makes use of the underlying spawn functions via canBuild. Just like ConstructionExecution does. But i didn't want to add more to PR 3220 since there's already a lot in there.
The new function findClosestBy could also be applied to some other parts of code to benefit of it being faster, so i did that.
findClosestBy uses findMinimumBy, which is a little more generic in name. I think findMinimumBy could be used by other parts of code, while findClosestBy is more clear naming for what it does now. But we could ditch findMinimumBy and just leave findClosestBy?
Examples of synthetic benchmarks (not included in this PR):
BEFORE CHANGES (before Scamiv's PR #3241)
AFTER CHANGES (before Scamiv's PR #3241)
BEFORE CHANGES (after Scamiv's PR #3241)
AFTER CHANGES (after Scamiv's PR #3241)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33