Fix/4745 fix sam targeting nuke pathing - #4753
Conversation
7ed85c9 to
470db04
Compare
WalkthroughSAM targeting now ranks and launches multiple interception candidates, while the statistics UI gains typed player/team columns, clan-tag propagation, dynamic layouts, and an in-table sticky ranking row. Curve spacing retains accumulated distance between cached points. ChangesSAM targeting and interception
Statistics and leaderboard UI
Bezier curve spacing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerImpl
participant GameUpdateUtils
participant PlayerView
participant StatsTable
participant LeaderboardPlayerList
PlayerImpl->>GameUpdateUtils: emit clanTag in player update
GameUpdateUtils-->>PlayerView: apply clanTag delta
PlayerView-->>StatsTable: provide clanTag row data
StatsTable->>LeaderboardPlayerList: render columns and rows
LeaderboardPlayerList-->>StatsTable: render sticky ranking footer
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
|
Ofcourse I forgot prettier, one second. |
## Description: split out clan tag, and made it filterable also added proper cell splits with rank/clan/player <img width="426" height="264" alt="image" src="https://github.com/user-attachments/assets/4dc0f99e-8755-41ac-887e-9c1811cd53f1" /> <img width="124" height="107" alt="image" src="https://github.com/user-attachments/assets/b5d6661b-d2f1-412f-9c86-67defbbedaa5" /> <img width="384" height="281" alt="image" src="https://github.com/user-attachments/assets/51b2f9c9-00a0-4952-a1a0-5c6cec321d0c" /> also updated the team icon to be the same as the button, was the player icon before: <img width="404" height="427" alt="image" src="https://github.com/user-attachments/assets/f53c8f45-f5fe-4048-b9f3-19a4be8d6e4e" /> ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory ## Please put your Discord username so you can be contacted if a bug or regression is found: w.o.n
openfrontio#4713) ## Description: Two changes to the 1v1 Ranked leaderboard. ### Clan tags removed 1v1 ranked is an individual ladder, and the tag the API returns is whichever one the player last happened to use in _any_ game mode — it says nothing about the ranked results being listed. Dropped from the rows, from the pinned "your ranking" bar, and from the view model (`PlayerLeaderboardEntry`, which is used only by this component). ### The pinned bar now lines up with the table It was an overlay laid out by hand — flex boxes with `px-6` / `ml-4` / `w-20` — instead of following the table it sits under. Measured in a headless browser at 1600px wide: | | Before | After | | ---------------------------- | ------------------------------------ | ------------ | | Player name vs Player column | 25px to the left | aligned | | Bar height vs row height | 73px vs 65px | 65px vs 65px | | Elo value | far right, under the Win/Loss header | under Elo | Hand-picked padding cannot fix this: the table is `table-fixed w-full`, so the resolved column widths grow with the modal and no fixed offset matches at more than one width. It is now the table's own sticky `<tfoot>`, mirroring the `<thead>` that is already sticky. Sharing the table means the columns cannot drift, and the row follows horizontal scrolling — which matters below the colgroup's 34rem total, where the table scrolls sideways and an overlay outside `.scroll-container` did not move with it. At 420px wide, scrolling fully right desynced the pinned row by 126px and pushed its Win/Loss cell to x=545 against a container edge of 419, where it could not be reached at all. Thanks to the Codex bot for catching that one. Being in flow also removes the `pb-16` spacer that existed only to keep the overlay off the last row. Verified in a headless browser: columns align to the pixel at 420 / 700 / 1600px wide, both unscrolled and scrolled fully right; the row stays pinned to the container bottom mid-scroll and at the end; and it is exactly one row tall. The container's bottom border is dropped so the pinned row sits flush against that edge. That border is unchanged from `main`, but the row is now solid `bg-blue-600` instead of `bg-blue-600/90` over a backdrop blur — `backdrop-filter` on a table row is unreliable and a sticky row has to be opaque — and against the brighter blue the dark hairline read as a stray outline. Its bottom inset against the container measures 0px now, against 1px before. One judgment call worth a look: now that the columns line up, the bar also fills the Games and Win/Loss cells. Aligning the columns and then leaving two of them blank looked worse than filling them, and every value is already in `currentUserEntry`. Easy to blank them again if you'd rather. before: had a double hbar? <img width="1328" height="941" alt="image" src="https://github.com/user-attachments/assets/cc72a0ca-2383-4095-85e1-f350bd473c79" /> columns didnt align: <img width="957" height="653" alt="image" src="https://github.com/user-attachments/assets/603001ae-b5cc-4ff1-ba2b-8047a422f510" /> after: ## Please complete the following: - [ ] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory No new user-facing strings — the bar reuses the existing `leaderboard_modal.your_ranking`, `leaderboard_modal.elo` and `leaderboard_modal.ratio` keys, so `en.json` is unchanged. ## Please put your Discord username so you can be contacted if a bug or regression is found: w.o.n --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/execution/SAMLauncherExecution.ts (1)
80-97: 🎯 Functional Correctness | 🔵 TrivialDuplicate "ticks remaining until explosion" computation.
computeInterceptionTile'sexplosionTickandcomputeTargetScore'stimeToExplodeboth derivetrajectory.length - currentIndexfromunit.trajectory()/unit.trajectoryIndex(), just with a different floor (Math.max(1, ...)vs none). Extracting a small shared helper would remove the duplication and keep the two calculations in sync if the formula ever changes.♻️ Proposed helper
+ private remainingTicksToExplode(unit: Unit): number { + return unit.trajectory().length - unit.trajectoryIndex(); + } + private computeInterceptionTile( unit: Unit, samTile: TileRef, rangeSquared: number, ): InterceptionTile | undefined { const trajectory = unit.trajectory(); const currentIndex = unit.trajectoryIndex(); - const explosionTick: number = trajectory.length - currentIndex; + const explosionTick: number = this.remainingTicksToExplode(unit);Also applies to: 99-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/execution/SAMLauncherExecution.ts` around lines 80 - 97, Extract the shared “ticks remaining until explosion” calculation into a helper near computeInterceptionTile and computeTargetScore, using the existing trajectory and trajectoryIndex values and preserving the required floor behavior for each caller. Replace explosionTick and timeToExplode derivations with this helper so both methods stay synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/utilities/Line.ts`:
- Around line 147-150: Add regression coverage for the spacing logic in Line.ts
under tests/**/*.test.ts, using setup(). Verify that excess cumulative distance
is retained after a point is emitted and that long-distance curves continue
producing correctly spaced points across multiple intervals.
---
Nitpick comments:
In `@src/core/execution/SAMLauncherExecution.ts`:
- Around line 80-97: Extract the shared “ticks remaining until explosion”
calculation into a helper near computeInterceptionTile and computeTargetScore,
using the existing trajectory and trajectoryIndex values and preserving the
required floor behavior for each caller. Replace explosionTick and timeToExplode
derivations with this helper so both methods stay synchronized.
🪄 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 Plus
Run ID: e7f92dd1-8a95-446f-902e-4383e216b1d0
📒 Files selected for processing (3)
src/core/execution/SAMLauncherExecution.tssrc/core/utilities/Line.tstests/core/executions/SAMLauncherExecution.test.ts
…hub.com/JB940/openfrontIO into fix/4745-fix-SAM-targeting-nuke-pathing
…hub.com/JB940/openfrontIO into fix/4745-fix-SAM-targeting-nuke-pathing
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/view/PlayerView.ts (1)
441-445: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOwn clan tag also disappears under anonymous names.
name()/displayName()keep working for the local player because theiranonymousNameis their real name, so the returned string is unchanged.clanTag()has no such escape: with anonymous names on, your own row loses the tag while your name stays intact. If that asymmetry is not wanted, gate on "not me" instead.♻️ Optional: keep the local player's own tag
clanTag(): string | null { - return this.anonymousName !== null && userSettings.anonymousNames() + return !this.isMe() && userSettings.anonymousNames() ? null : this.static.clanTag; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/view/PlayerView.ts` around lines 441 - 445, Update PlayerView.clanTag() so anonymous-name masking applies only to other players, not the local player; gate the null return on the existing local-player identity check while preserving the current userSettings.anonymousNames() behavior for non-local players.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/client/view/PlayerView.ts`:
- Around line 441-445: Update PlayerView.clanTag() so anonymous-name masking
applies only to other players, not the local player; gate the null return on the
existing local-player identity check while preserving the current
userSettings.anonymousNames() behavior for non-local players.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba5fc5fe-fba3-4332-80a9-4a5cd1f45e61
⛔ Files ignored due to path filters (1)
resources/images/GuildIconWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
CREDITS.mdresources/lang/en.jsonsrc/client/StatsConstants.tssrc/client/components/StatsTable.tssrc/client/components/leaderboard/LeaderboardPlayerList.tssrc/client/hud/HotbarIcons.tssrc/client/hud/layers/PlayerStats.tssrc/client/hud/layers/TeamStats.tssrc/client/hud/layers/lib/StatsColumns.tssrc/client/render/types/Renderer.tssrc/client/view/GameView.tssrc/client/view/PlayerView.tssrc/core/ApiSchemas.tssrc/core/game/Game.tssrc/core/game/GameUpdateUtils.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/UserSettings.tstests/ColumnPicker.test.tstests/GameLeftSidebar.test.tstests/GameUpdateUtils.test.tstests/PlayerStats.test.tstests/PlayerUpdateDiff.test.tstests/StatsColumns.test.tstests/StatsTableVirtualization.test.tstests/TeamStats.test.tstests/UserSettings.test.tstests/client/LeaderboardModal.test.tstests/client/view/PlayerView.test.tstests/util/viewStubs.ts
Add approved & assigned issue number here:
Resolves #4745 bullet points 1, 2, 4, 6 and 8. A new issue will be made for the other bullet points, as they are lower priority.
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
JB940