Feat: Transport boats now show arrival ETA#3381
Feat: Transport boats now show arrival ETA#3381camclark wants to merge 11 commits intoopenfrontio:mainfrom
Conversation
|
|
WalkthroughAdds a client-side ETA estimator and per-boat countdowns/progress bars in AttacksDisplay, introduces a seconds-abbrev localization key, and adds tests for the estimator. Changes are client-side UI and tests only. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/AttacksDisplay.ts (1)
27-33: ExtractestimateBoatEtaSecondsinto a plain helper module.
tests/client/graphics/layers/BoatEta.test.tsnow has to import the whole Lit element module just to test a pure function, which also pulls in custom-element registration and asset imports. A small utility file would keep this test simpler and side-effect free.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 27 - 33, Move the pure function estimateBoatEtaSeconds out of the Lit element file into a small plain helper module that exports the same named function; in AttacksDisplay.ts replace the local implementation with an import of estimateBoatEtaSeconds from the new helper (or re-export it) so external callers keep working, and update tests to import the helper module directly instead of importing the whole Lit element module.
🤖 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/client/graphics/layers/AttacksDisplay.ts`:
- Around line 363-370: In getBoatEtaSeconds, replace the Manhattan-distance
based ETA with one computed from the boat's actual remaining route length:
instead of calling this.game.manhattanDist(boat.tile(), targetTile), obtain the
boat's remaining path/route length (e.g., via a method on UnitView like
boat.remainingRouteLength() or boat.route()/boat.path() that returns remaining
steps) or ask the game's pathfinder for the current path length from boat.tile()
to boat.targetTile(), then pass that step count into estimateBoatEtaSeconds so
ETA reflects route steps around land/chokepoints (update references in
getBoatEtaSeconds and remove the manhattanDist usage).
---
Nitpick comments:
In `@src/client/graphics/layers/AttacksDisplay.ts`:
- Around line 27-33: Move the pure function estimateBoatEtaSeconds out of the
Lit element file into a small plain helper module that exports the same named
function; in AttacksDisplay.ts replace the local implementation with an import
of estimateBoatEtaSeconds from the new helper (or re-export it) so external
callers keep working, and update tests to import the helper module directly
instead of importing the whole Lit element module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80c72ee4-2eb1-4bf8-83a9-ec856cdd2177
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/graphics/layers/AttacksDisplay.tstests/client/graphics/layers/BoatEta.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/AttacksDisplay.ts (1)
387-425:⚠️ Potential issue | 🟠 MajorDrive the ETA refresh from a 1s wall-clock tick.
This renders the new countdown, but the layer still does not opt into a 1-second wall-clock schedule. As a result, both outgoing and incoming boat ETAs will follow the default layer tick cadence instead of the PR requirement of updating once per second.
Suggested fix
export class AttacksDisplay extends LitElement implements Layer { public eventBus: EventBus; public game: GameView; public uiState: UIState; @@ tick() { this.active = true; @@ this.requestUpdate(); } + + getTickIntervalMs(): number { + return 1000; + } shouldTransform(): boolean { return false; }Based on learnings, UI/layer updates should be wall-clock throttled independently of simulation speed, and layers should expose
getTickIntervalMs()for that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 387 - 425, The layer currently renders ETA countdowns but doesn't opt into a 1s wall-clock schedule; add/override getTickIntervalMs() on the AttacksDisplay class to return 1000 (ms) so the renderer uses a 1-second wall-clock tick and the values from getBoatEtaSeconds(boat) refresh once per second for both outgoingBoats and incoming boats; ensure this method exists on the class (same class that defines renderButton, getBoatEtaSeconds, outgoingBoats mapping) so the layer infrastructure will throttle updates independently of simulation speed.
🤖 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/client/graphics/layers/AttacksDisplay.ts`:
- Around line 27-35: The estimateBoatEtaSeconds function validates distance but
not turnIntervalMs, so invalid or non-positive intervals can produce bad ETAs;
update estimateBoatEtaSeconds to also return null when turnIntervalMs is not a
finite number or is <= 0 (e.g., use Number.isFinite(turnIntervalMs) &&
turnIntervalMs > 0), then compute secondsPerTick and the ceil as before;
reference the function estimateBoatEtaSeconds and the parameter turnIntervalMs
when making this change.
---
Outside diff comments:
In `@src/client/graphics/layers/AttacksDisplay.ts`:
- Around line 387-425: The layer currently renders ETA countdowns but doesn't
opt into a 1s wall-clock schedule; add/override getTickIntervalMs() on the
AttacksDisplay class to return 1000 (ms) so the renderer uses a 1-second
wall-clock tick and the values from getBoatEtaSeconds(boat) refresh once per
second for both outgoingBoats and incoming boats; ensure this method exists on
the class (same class that defines renderButton, getBoatEtaSeconds,
outgoingBoats mapping) so the layer infrastructure will throttle updates
independently of simulation speed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aed723fc-04da-435d-8114-962bc361a05a
📒 Files selected for processing (1)
src/client/graphics/layers/AttacksDisplay.ts
|
Regarding the
Adding a 1s interval would actually make the ETA update less frequently than the betrayal timer, which would be inconsistent. Leaving it as-is. |
| if (!Number.isFinite(distance) || distance < 0) return null; | ||
| if (!Number.isFinite(turnIntervalMs) || turnIntervalMs <= 0) return null; |
There was a problem hiding this comment.
do we need these checks? I don't think this should ever happen.
Maybe just throw an error instead
There was a problem hiding this comment.
Good call — changed to throw errors with descriptive messages since these values come from game internals and should never be invalid. Updated tests to expect throws. 66455bf
| } from "./Leaderboard"; | ||
| import swordIcon from "/images/SwordIcon.svg?url"; | ||
|
|
||
| /** Estimates boat arrival time in seconds from Manhattan distance and server tick interval. */ |
There was a problem hiding this comment.
manhattan distance is a bad heuristic. could be way off if boat is going around a peninsula. Instead we should use actual distance
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/client/graphics/layers/BoatEta.test.ts (1)
1-56: Good test coverage for the ETA calculator.The test suite covers standard cases, ceiling behavior, zero/large distances, and all the error cases for invalid inputs. The test names are clear and easy to read.
One optional addition: you might add tests for
-Infinitydistance and+InfinityturnIntervalMs to fully cover all non-finite edge cases. These should also throw, per theNumber.isFinitechecks.,
Optional: Additional edge case tests
test("throws for Infinity distance", () => { expect(() => estimateBoatEtaSeconds(Infinity, 100)).toThrow( "Invalid distance", ); }); + test("throws for -Infinity distance", () => { + expect(() => estimateBoatEtaSeconds(-Infinity, 100)).toThrow( + "Invalid distance", + ); + }); + test("throws for zero turnIntervalMs", () => { expect(() => estimateBoatEtaSeconds(100, 0)).toThrow( "Invalid turnIntervalMs", ); }); + test("throws for Infinity turnIntervalMs", () => { + expect(() => estimateBoatEtaSeconds(100, Infinity)).toThrow( + "Invalid turnIntervalMs", + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/graphics/layers/BoatEta.test.ts` around lines 1 - 56, Add tests that assert estimateBoatEtaSeconds throws for non-finite inputs: specifically add one test calling estimateBoatEtaSeconds(-Infinity, 100) expecting an "Invalid distance" throw, and another calling estimateBoatEtaSeconds(100, Infinity) expecting an "Invalid turnIntervalMs" throw; place these alongside the existing NaN/Infinity tests so the suite fully covers Number.isFinite edge cases for the estimateBoatEtaSeconds function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/client/graphics/layers/BoatEta.test.ts`:
- Around line 1-56: Add tests that assert estimateBoatEtaSeconds throws for
non-finite inputs: specifically add one test calling
estimateBoatEtaSeconds(-Infinity, 100) expecting an "Invalid distance" throw,
and another calling estimateBoatEtaSeconds(100, Infinity) expecting an "Invalid
turnIntervalMs" throw; place these alongside the existing NaN/Infinity tests so
the suite fully covers Number.isFinite edge cases for the estimateBoatEtaSeconds
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c758b1cc-eb90-4e8f-ac41-115a2373d37f
📒 Files selected for processing (2)
src/client/graphics/layers/AttacksDisplay.tstests/client/graphics/layers/BoatEta.test.ts
camclark
force-pushed
the
feature/boat-eta-countdown
branch
from
a280a40 to
533a6e4
Compare
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/client/graphics/layers/AttacksDisplay.ts`:
- Around line 372-379: The ETA calculation undercounts time because remaining
path steps (computed as remaining) are passed to estimateBoatEtaSeconds as if
they were ticks; multiply remaining by plan.ticksPerStep before calling
estimateBoatEtaSeconds so the function receives remaining ticks, e.g., replace
the call using remaining with (remaining * plan.ticksPerStep); refer to elapsed,
remaining, plan.ticksPerStep, estimateBoatEtaSeconds and
TransportShipExecution.ticksPerMove to locate and fix the logic in
AttacksDisplay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 102e3958-091f-452f-a346-f3e073f58496
📒 Files selected for processing (1)
src/client/graphics/layers/AttacksDisplay.ts
camclark
force-pushed
the
feature/boat-eta-countdown
branch
from
6caf9fa to
7c79c9b
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/client/graphics/layers/BoatEta.test.ts (1)
4-72: Add one regression test forticksPerStep > 1.This suite only proves
estimateBoatEtaSeconds(). It does not protect the motion-plan conversion inAttacksDisplay.getBoatEtaSeconds(), so Line 378 could regress back to passing steps instead of ticks and these tests would still pass. Please add one case that exercises a plan withticksPerStep > 1, or extract that conversion into a small pure helper and test it directly.Based on learnings,
AttacksDisplay.getBoatEtaSeconds()must convert motion-plan remaining steps into remaining ticks withplan.ticksPerStepbefore converting to seconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/graphics/layers/BoatEta.test.ts` around lines 4 - 72, Add a regression test that ensures motion-plan steps are converted to ticks using plan.ticksPerStep before converting to seconds: in BoatEta.test.ts add one case that simulates a plan with ticksPerStep > 1 (or call AttacksDisplay.getBoatEtaSeconds with a mock plan) and asserts the result equals estimateBoatEtaSeconds(remainingSteps * plan.ticksPerStep, turnIntervalMs). Reference estimateBoatEtaSeconds and AttacksDisplay.getBoatEtaSeconds (or the extracted helper) and ensure the test uses ticksPerStep > 1 so it would fail if steps were passed directly instead of ticks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/client/graphics/layers/BoatEta.test.ts`:
- Around line 4-72: Add a regression test that ensures motion-plan steps are
converted to ticks using plan.ticksPerStep before converting to seconds: in
BoatEta.test.ts add one case that simulates a plan with ticksPerStep > 1 (or
call AttacksDisplay.getBoatEtaSeconds with a mock plan) and asserts the result
equals estimateBoatEtaSeconds(remainingSteps * plan.ticksPerStep,
turnIntervalMs). Reference estimateBoatEtaSeconds and
AttacksDisplay.getBoatEtaSeconds (or the extracted helper) and ensure the test
uses ticksPerStep > 1 so it would fail if steps were passed directly instead of
ticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b5761aa-3cbf-467e-9cb7-8ae896094f7d
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/graphics/layers/AttacksDisplay.tstests/client/graphics/layers/BoatEta.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
| ${etaSeconds !== null | ||
| ? html`<span class="text-xs" | ||
| >${translateText("events_display.seconds_abbrev", { | ||
| seconds: etaSeconds, | ||
| })}</span | ||
| >` | ||
| : ""} |
There was a problem hiding this comment.
Seconds makes it a bit too busy, I think instead of showing seconds, just have a bar that decreases until the boat arrives.
There was a problem hiding this comment.
I do really think the last X seconds are important to brace for impact, so I've implemented what you've asked and then added a countdown on the last 5 seconds.
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/client/graphics/layers/AttacksDisplay.ts`:
- Around line 381-389: getBoatProgress currently computes progress as
elapsedSteps/totalSteps which ignores plan.ticksPerStep; change it to compute
progress based on ticks: calculate totalTicks = (plan.path.length - 1) *
plan.ticksPerStep, elapsedTicks = this.game.ticks() - plan.startTick (clamped
>=0), then return Math.min(1, Math.max(0, elapsedTicks / totalTicks)); keep the
early-null/zero guards (when no plan or totalTicks <= 0) and use the same
symbols (getBoatProgress, game.motionPlans(), plan.path, plan.ticksPerStep,
plan.startTick, this.game.ticks()) to locate and update the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4169071-f37f-44f3-b713-25a4065e7e2a
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/graphics/layers/AttacksDisplay.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
| private getBoatProgress(boat: UnitView): number | null { | ||
| const plan = this.game.motionPlans().get(boat.id()); | ||
| if (!plan) return null; | ||
| const totalSteps = plan.path.length - 1; | ||
| if (totalSteps <= 0) return 1; | ||
| const elapsed = Math.floor( | ||
| (this.game.ticks() - plan.startTick) / plan.ticksPerStep, | ||
| ); | ||
| return Math.min(1, Math.max(0, elapsed / totalSteps)); |
There was a problem hiding this comment.
Compute progress from ticks, not just completed path steps.
getBoatEtaSeconds() was fixed to account for plan.ticksPerStep, but getBoatProgress() still uses elapsedSteps / totalSteps. For transport boats with ticksPerStep > 1, the bar will stay flat for most of each move and then jump at tile boundaries, so the new “journey completion” UI lags behind real travel time. Use elapsed/total ticks here as well.
♻️ Proposed fix
private getBoatProgress(boat: UnitView): number | null {
const plan = this.game.motionPlans().get(boat.id());
if (!plan) return null;
const totalSteps = plan.path.length - 1;
if (totalSteps <= 0) return 1;
- const elapsed = Math.floor(
- (this.game.ticks() - plan.startTick) / plan.ticksPerStep,
- );
- return Math.min(1, Math.max(0, elapsed / totalSteps));
+ const totalTicks = totalSteps * plan.ticksPerStep;
+ const elapsedTicks = this.game.ticks() - plan.startTick;
+ return Math.min(1, Math.max(0, elapsedTicks / totalTicks));
}Based on learnings: transport-boat motion plans in this PR can use ticksPerStep > 1, so boat timing UI should be derived from ticks, not only path steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 381 - 389,
getBoatProgress currently computes progress as elapsedSteps/totalSteps which
ignores plan.ticksPerStep; change it to compute progress based on ticks:
calculate totalTicks = (plan.path.length - 1) * plan.ticksPerStep, elapsedTicks
= this.game.ticks() - plan.startTick (clamped >=0), then return Math.min(1,
Math.max(0, elapsedTicks / totalTicks)); keep the early-null/zero guards (when
no plan or totalTicks <= 0) and use the same symbols (getBoatProgress,
game.motionPlans(), plan.path, plan.ticksPerStep, plan.startTick,
this.game.ticks()) to locate and update the logic.
3 participants
Description:
Adds a progress bar to transport boats in the attacks display panel, showing journey completion for both outgoing and incoming boats. A countdown overlay appears for the final 5 seconds before arrival.
Resolves #1793
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
cool_clarky