Draft
Conversation
Contributor
WalkthroughThe pull request updates trade ship mechanics by renaming a configuration parameter from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
startup.sh (1)
88-92: Redundant conditional: both branches do the same thing.After removing the timeout wrapper, the
ifandelsebranches now execute identical commands. This conditional can be simplified.♻️ Proposed simplification
-if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -else - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -fi +exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@startup.sh` around lines 88 - 92, The if/else conditional checking DOMAIN and SUBDOMAIN is redundant because both branches call exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf; remove the entire if/else block and replace it with a single exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf line (references: variables DOMAIN and SUBDOMAIN, and the supervisord command path).
🤖 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/configuration/DefaultConfig.ts`:
- Around line 387-390: In DefaultConfig (method returning the object with cost:
this.costWrapper(() => 35_000_000)), remove the unreachable `break` that follows
the `return` — locate the block in DefaultConfig where the function returns `{
cost: this.costWrapper(...) }` and delete the redundant `break` statement to
eliminate dead code.
- Around line 319-328: The current fallback for numPlayerPorts > 12 uses
Math.floor((100 * rejectionModifier) / 50) which produces an unexpected large
jump; replace this with a scaling formula that continues with numPlayerPorts
(e.g., return Math.max(2, Math.min(95, Math.floor((100 * rejectionModifier) /
Math.max(1, numPlayerPorts))))), referencing numPlayerPorts and
tradeShipSpawnRejections/rejectionModifier so spawn chance scales smoothly and
is clamped to reasonable bounds.
---
Nitpick comments:
In `@startup.sh`:
- Around line 88-92: The if/else conditional checking DOMAIN and SUBDOMAIN is
redundant because both branches call exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf; remove the entire if/else block and
replace it with a single exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf line (references: variables DOMAIN and
SUBDOMAIN, and the supervisord command path).
🪄 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: 0291d1b4-17a9-4904-af22-d80a04e33656
📒 Files selected for processing (4)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/PortExecution.tsstartup.sh
Comment on lines
+319
to
+328
| if (numPlayerPorts <= 3) return 18; | ||
| if (numPlayerPorts <= 5) return 25; | ||
| if (numPlayerPorts <= 8) return 35; | ||
| if (numPlayerPorts <= 10) return 40; | ||
| if (numPlayerPorts <= 12) return 45; | ||
|
|
||
| // Pity timer: increases spawn chance after consecutive rejections | ||
| const rejectionModifier = 1 / (tradeShipSpawnRejections + 1); | ||
|
|
||
| return Math.floor((100 * rejectionModifier) / baseSpawnRate); | ||
| return Math.floor((100 * rejectionModifier) / 50); |
Contributor
There was a problem hiding this comment.
Possible logic issue: spawn rate drops sharply for >12 ports.
For numPlayerPorts <= 12, the function returns 45 (about 2.2% spawn chance). But for numPlayerPorts > 12, it returns Math.floor(100 / 50) = 2 when there are no rejections, meaning a 50% spawn chance.
This seems like a big jump. Is this intentional? If you meant to continue the scaling pattern, consider something like:
🐛 If this is unintentional, here is a possible fix
if (numPlayerPorts <= 12) return 45;
// Pity timer: increases spawn chance after consecutive rejections
const rejectionModifier = 1 / (tradeShipSpawnRejections + 1);
- return Math.floor((100 * rejectionModifier) / 50);
+ // Continue scaling for larger port counts
+ const baseRate = 45 + Math.floor((numPlayerPorts - 12) * 5);
+ return Math.floor(baseRate * rejectionModifier);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/DefaultConfig.ts` around lines 319 - 328, The current
fallback for numPlayerPorts > 12 uses Math.floor((100 * rejectionModifier) / 50)
which produces an unexpected large jump; replace this with a scaling formula
that continues with numPlayerPorts (e.g., return Math.max(2, Math.min(95,
Math.floor((100 * rejectionModifier) / Math.max(1, numPlayerPorts))))),
referencing numPlayerPorts and tradeShipSpawnRejections/rejectionModifier so
spawn chance scales smoothly and is clamped to reasonable bounds.
Comment on lines
+387
to
390
| return { | ||
| cost: this.costWrapper(() => 35_000_000), | ||
| }; | ||
| break; |
Contributor
There was a problem hiding this comment.
Dead code: break after return is unreachable.
The return statement exits the function, so the break on Line 390 never executes.
🧹 Proposed fix
case UnitType.MIRV:
return {
cost: this.costWrapper(() => 35_000_000),
};
- break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| cost: this.costWrapper(() => 35_000_000), | |
| }; | |
| break; | |
| return { | |
| cost: this.costWrapper(() => 35_000_000), | |
| }; |
🧰 Tools
🪛 Biome (2.4.6)
[error] 390-390: This code is unreachable
(lint/correctness/noUnreachable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/DefaultConfig.ts` around lines 387 - 390, In
DefaultConfig (method returning the object with cost: this.costWrapper(() =>
35_000_000)), remove the unreachable `break` that follows the `return` — locate
the block in DefaultConfig where the function returns `{ cost:
this.costWrapper(...) }` and delete the redundant `break` statement to eliminate
dead code.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
392-396: Keep MIRV on the shared cache path.This early
returnskipsthis.unitInfoCache.set(type, info)at Line 468, so MIRV rebuilds a freshUnitInfoobject and cost closure on every lookup while every other unit type is memoized.♻️ Reuse the normal cached path
case UnitType.MIRV: - return { + info = { cost: this.costWrapper(() => 35_000_000), }; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 392 - 396, The MIRV case in DefaultConfig.ts returns early and bypasses the shared cache, causing MIRV to be rebuilt every lookup; change the UnitType.MIRV branch to assign the info object (including cost: this.costWrapper(() => 35_000_000)) to the same local variable used by the switch and do not return there so execution reaches this.unitInfoCache.set(type, info) (preserving the cost closure via this.costWrapper) instead of returning immediately.Dockerfile (1)
79-83: Drop the dead branch, or restore the non-main behavior.Both sides now
execthe samesupervisordcommand, so thisDOMAIN/SUBDOMAINcheck is dead. If this environment split is still needed, the branch needs different commands; otherwise simplify it away.♻️ Simplify the script if the split is no longer needed
-if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -else - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -fi +exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 79 - 83, The conditional block checking DOMAIN and SUBDOMAIN is dead because both branches run the same supervisord command; either remove the if/else and run exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf unconditionally, or restore the intended non-main behavior by changing the alternative branch to the correct command (or different supervisord config) for the case when DOMAIN=openfront.dev and SUBDOMAIN!=main; edit the shell snippet that references DOMAIN and SUBDOMAIN and update the branch logic accordingly so that only meaningful differences remain.
🤖 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/configuration/DefaultConfig.ts`:
- Around line 321-334: The tail branch of
tradeShipSpawnRate(tradeShipSpawnRejections, numPlayerPorts) can return 0 for
tradeShipSpawnRejections >= 2; clamp the computed pity-timer result to a minimum
of 1 so callers never receive zero. Locate the final return (currently
Math.floor((100 * rejectionModifier) / 50)) and change it to return Math.max(1,
Math.floor((100 * rejectionModifier) / 50)) (or otherwise ensure the computed
value is at least 1) so the 1/tradeShipSpawnRate contract holds.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 79-83: The conditional block checking DOMAIN and SUBDOMAIN is dead
because both branches run the same supervisord command; either remove the
if/else and run exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf unconditionally, or restore the intended
non-main behavior by changing the alternative branch to the correct command (or
different supervisord config) for the case when DOMAIN=openfront.dev and
SUBDOMAIN!=main; edit the shell snippet that references DOMAIN and SUBDOMAIN and
update the branch logic accordingly so that only meaningful differences remain.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 392-396: The MIRV case in DefaultConfig.ts returns early and
bypasses the shared cache, causing MIRV to be rebuilt every lookup; change the
UnitType.MIRV branch to assign the info object (including cost:
this.costWrapper(() => 35_000_000)) to the same local variable used by the
switch and do not return there so execution reaches this.unitInfoCache.set(type,
info) (preserving the cost closure via this.costWrapper) instead of returning
immediately.
🪄 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: d15f4214-f9d4-44be-98a3-93c6f430093b
📒 Files selected for processing (4)
Dockerfilesrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/PortExecution.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/PortExecution.ts
Comment on lines
321
to
+334
| tradeShipSpawnRate( | ||
| tradeShipSpawnRejections: number, | ||
| numTradeShips: number, | ||
| numPlayerPorts: number, | ||
| ): number { | ||
| const decayRate = Math.LN2 / 50; | ||
|
|
||
| // Approaches 0 as numTradeShips increase | ||
| const baseSpawnRate = 1 - sigmoid(numTradeShips, decayRate, 200); | ||
| if (numPlayerPorts <= 3) return 18; | ||
| if (numPlayerPorts <= 5) return 25; | ||
| if (numPlayerPorts <= 8) return 35; | ||
| if (numPlayerPorts <= 10) return 40; | ||
| if (numPlayerPorts <= 12) return 45; | ||
|
|
||
| // Pity timer: increases spawn chance after consecutive rejections | ||
| const rejectionModifier = 1 / (tradeShipSpawnRejections + 1); | ||
|
|
||
| return Math.floor((100 * rejectionModifier) / baseSpawnRate); | ||
| return Math.floor((100 * rejectionModifier) / 50); |
Contributor
There was a problem hiding this comment.
Keep tradeShipSpawnRate() above zero.
Line 334 returns 0 for any tradeShipSpawnRejections >= 2, which breaks the 1 / tradeShipSpawnRate contract in the comment above and hands the caller an invalid probability. At minimum, clamp this branch to 1.
🐛 Minimal fix to preserve the contract
// Pity timer: increases spawn chance after consecutive rejections
const rejectionModifier = 1 / (tradeShipSpawnRejections + 1);
- return Math.floor((100 * rejectionModifier) / 50);
+ return Math.max(1, Math.floor((100 * rejectionModifier) / 50));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/DefaultConfig.ts` around lines 321 - 334, The tail
branch of tradeShipSpawnRate(tradeShipSpawnRejections, numPlayerPorts) can
return 0 for tradeShipSpawnRejections >= 2; clamp the computed pity-timer result
to a minimum of 1 so callers never receive zero. Locate the final return
(currently Math.floor((100 * rejectionModifier) / 50)) and change it to return
Math.max(1, Math.floor((100 * rejectionModifier) / 50)) (or otherwise ensure the
computed value is at least 1) so the 1/tradeShipSpawnRate contract holds.
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
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:
DISCORD_USERNAME