Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded defensive null checks in property layout spawn/access logic, tightened RediSearch field-scoping for OR lists, added PropertyLocation.Clone(), centralized and hardened starship relocation during property deletion, and wrapped property spawn operations in try/catch to log and continue on errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Deleter as "Property.DeleteProperty"
participant DB as "DB Service"
participant Ship as "Starship Entity"
participant Space as "Space Service"
participant Logger as "Log"
Deleter->>DB: Load target property, snapshot ChildPropertyIds
loop for each childId
Deleter->>DB: Get(childId)
alt child found
Deleter->>DB: Load child interior & docked ships
loop per ship
Deleter->>Ship: Clear RegisteredStarport
Deleter->>Ship: Set DockPosition / LastNPCDockPosition (use Clone or CZ-220 fallback)
Deleter->>DB: Persist ship changes
end
Deleter->>Space: Unregister dock points
else missing child
Deleter->>Logger: Write error and continue
end
end
Deleter->>Logger: Log completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 976-1007: In RelocateDockedShips, after clearing
RegisteredStarport for a ship but before skipping when LastNPCDockPosition is
missing, add a warning log that the ship was left without a dock fallback
(include ship.Id, ship.CustomName and the parent
property.Id/property.CustomName) so stranded ships are visible; reference the
RelocateDockedShips method, the RegisteredStarport key mutation, and the
Positions keys (PropertyLocationType.LastNPCDockPosition and
PropertyLocationType.DockPosition) and ensure you still avoid mutating
DockPosition when LastNPCDockPosition is absent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: af108ad8-079f-4549-8e35-d5dc0e412b03
📒 Files selected for processing (5)
SWLOR.Game.Server/Feature/PropertyLayoutDefinition/BankLayoutDefinition.csSWLOR.Game.Server/Feature/PropertyLayoutDefinition/StarportLayoutDefinition.csSWLOR.Game.Server/Service/DBService/DBQuery.csSWLOR.Game.Server/Service/Property.csSWLOR.Game.Server/Service/PropertyService/StructureChangedAction.cs
|
@coderabbitai - review now that the rate limit is over |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
@coderabbitai - Try to review the code again. The rate limit should be over |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
SWLOR.Game.Server/Service/Property.cs (1)
1026-1068: Optional:cz220Cacheis used only as a "resolved successfully" flag.The
PropertyLocationreturned at line 1056 is never stored on a ship — it's only read for the null check at line 1060, and lines 1067-1068 rebuild it from scratch (deliberately, to avoid cross-ship aliasing). A boolean would convey the intent more clearly and avoid the wasted instance:♻️ Optional simplification
- // Lazily resolved + cached across the loop so we only hit GetWaypointByTag once, - // and only when at least one stranded ship actually needs it. - PropertyLocation cz220Cache = null; - var cz220Resolved = false; + // Lazily probed across the loop so we only hit GetWaypointByTag once, + // and only when at least one stranded ship actually needs it. + bool? cz220Available = null; @@ - // No personal fallback - try the guaranteed CZ-220 NPC starport. - if (!cz220Resolved) - { - cz220Cache = BuildGuaranteedFallbackDockPosition(); - cz220Resolved = true; - } - - if (cz220Cache != null) + // No personal fallback - try the guaranteed CZ-220 NPC starport. + cz220Available ??= BuildGuaranteedFallbackDockPosition() != null; + + if (cz220Available == true)Functionally equivalent; purely a readability nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SWLOR.Game.Server/Service/Property.cs` around lines 1026 - 1068, cz220Cache is only used as a null-check marker while cz220Resolved indicates resolution; replace cz220Cache with a simple bool to avoid creating unused PropertyLocation instances: remove cz220Cache, keep cz220Resolved (or rename to cz220Resolved/hasGuaranteedFallback), call BuildGuaranteedFallbackDockPosition() once when cz220Resolved is false to set the flag, and later always call BuildGuaranteedFallbackDockPosition() when assigning ship.Positions[PropertyLocationType.DockPosition] and ship.Positions[PropertyLocationType.LastNPCDockPosition]; update references in the loop accordingly (symbols: cz220Cache, cz220Resolved, BuildGuaranteedFallbackDockPosition, PropertyLocationType.DockPosition, PropertyLocationType.LastNPCDockPosition).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 1043-1051: The code assigns
ship.Positions[PropertyLocationType.DockPosition] to the same reference as
ship.Positions[PropertyLocationType.LastNPCDockPosition], causing aliasing;
update the LastNPCDockPosition path in the relocation logic (the block using
hasOwnFallback) to produce a distinct copy instead of assigning the same
object—either by calling BuildGuaranteedFallbackDockPosition() twice (as the
CZ-220 path does) or by cloning the PropertyLocation instance before assigning
to DockPosition; also apply the same clone/fix to the identical aliasing at
CleanUpData (line ~433) and first confirm whether PropertyLocation in
SWLOR.Game.Server.Entity is a class (reference) or struct (value) to choose
cloning vs. direct assignment.
---
Nitpick comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 1026-1068: cz220Cache is only used as a null-check marker while
cz220Resolved indicates resolution; replace cz220Cache with a simple bool to
avoid creating unused PropertyLocation instances: remove cz220Cache, keep
cz220Resolved (or rename to cz220Resolved/hasGuaranteedFallback), call
BuildGuaranteedFallbackDockPosition() once when cz220Resolved is false to set
the flag, and later always call BuildGuaranteedFallbackDockPosition() when
assigning ship.Positions[PropertyLocationType.DockPosition] and
ship.Positions[PropertyLocationType.LastNPCDockPosition]; update references in
the loop accordingly (symbols: cz220Cache, cz220Resolved,
BuildGuaranteedFallbackDockPosition, PropertyLocationType.DockPosition,
PropertyLocationType.LastNPCDockPosition).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 253eb7a3-c38d-4618-8724-8f927fadb50a
📒 Files selected for processing (1)
SWLOR.Game.Server/Service/Property.cs
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)
SWLOR.Game.Server/Service/Property.cs (1)
422-441:⚠️ Potential issue | 🟠 MajorUnguarded
Positions[LastNPCDockPosition]access can crash CleanUpData.
RelocateDockedShipsguards its LastNPCDockPosition path withContainsKey(line 1047), but this block doesn't. If any legacy starship record was persisted without aLastNPCDockPositionentry and itsDockPosition.InstancePropertyIdpoints at a deleted starport, line 437 will throwKeyNotFoundExceptionand abort the rest of the starship cleanup loop — potentially leaving other ships stranded on the same boot.Given this was the exact class of orphaning that motivated the PR, please align with
RelocateDockedShipsand handle the missing-fallback case (CZ-220 backfill, or at minimum a logged warning + skip).🛡️ Suggested guard
var dbStarport = DB.Get<WorldProperty>(dockPosition.InstancePropertyId); if (dbStarport == null) { // The PC starport no longer exists (probably destroyed by the previous cleanup) // Luckily we track the last NPC starport they visited so we can simply replace // their docked position with it. // // Clone rather than assigning the same reference to both keys. PropertyLocation // is a reference type, and later in-place mutations of DockPosition (e.g. in // migrations) would otherwise silently corrupt LastNPCDockPosition too. - property.Positions[PropertyLocationType.DockPosition] = property.Positions[PropertyLocationType.LastNPCDockPosition].Clone(); - - Log.Write(LogGroup.Property, $"Starship '{property.CustomName}' ({property.Id}) was docked at a non-existent player starport. It has been relocated to the last NPC dock position at '{property.Positions[PropertyLocationType.LastNPCDockPosition].AreaResref}'."); + if (property.Positions.TryGetValue(PropertyLocationType.LastNPCDockPosition, out var lastNpc)) + { + property.Positions[PropertyLocationType.DockPosition] = lastNpc.Clone(); + Log.Write(LogGroup.Property, $"Starship '{property.CustomName}' ({property.Id}) was docked at a non-existent player starport. It has been relocated to the last NPC dock position at '{lastNpc.AreaResref}'."); + } + else + { + Log.Write(LogGroup.Error, $"Starship '{property.CustomName}' ({property.Id}) was docked at a non-existent player starport and has no LastNPCDockPosition. DockPosition left untouched; manual intervention may be required.", true); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SWLOR.Game.Server/Service/Property.cs` around lines 422 - 441, The code in Property.cs unguards access to property.Positions[PropertyLocationType.LastNPCDockPosition] when replacing a missing player dock, which can throw KeyNotFoundException and abort CleanUpData; update the block in the starship cleanup logic (same area as the DockPosition handling) to first check property.Positions.ContainsKey(PropertyLocationType.LastNPCDockPosition) (matching RelocateDockedShips) and if missing either log a warning via Log.Write and skip relocation or use a safe fallback before calling Clone(), ensuring you reference Property.Positions, PropertyLocationType.DockPosition, PropertyLocationType.LastNPCDockPosition, and preserve the existing log message when relocation occurs.
🧹 Nitpick comments (1)
SWLOR.Game.Server/Service/Property.cs (1)
1060-1080: Reusecz220Cacheinstead of re-resolving the waypoint per assignment.Lines 1028-1031 introduce
cz220Cachespecifically to hash the waypoint lookup once per call, but lines 1074-1075 ignore the cache and re-invokeBuildGuaranteedFallbackDockPosition()twice for every stranded ship (and once more each for any additional ships). Beyond the redundantGetWaypointByTag/GetArea/GetPositioncalls, it also means the two assignments could theoretically diverge (e.g. DockPosition set, second call returns null on a transient failure, LastNPCDockPosition clobbered to null).Now that
PropertyLocation.Clone()exists, clone the cached instance instead — same no-aliasing guarantee, one waypoint resolution per deletion.♻️ Suggested cleanup
if (cz220Cache != null) { // Use independent PropertyLocation instances per Positions key. Storing the // same reference under both keys would alias them, so a later in-place // mutation of one (see _11_RefundMobilityAndUpdateVelesStarport for an // example of in-place mutation of these structs) would silently corrupt // the other. - ship.Positions[PropertyLocationType.DockPosition] = BuildGuaranteedFallbackDockPosition(); - ship.Positions[PropertyLocationType.LastNPCDockPosition] = BuildGuaranteedFallbackDockPosition(); + ship.Positions[PropertyLocationType.DockPosition] = cz220Cache.Clone(); + ship.Positions[PropertyLocationType.LastNPCDockPosition] = cz220Cache.Clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SWLOR.Game.Server/Service/Property.cs` around lines 1060 - 1080, The code re-computes the CZ-220 fallback waypoint twice per ship by calling BuildGuaranteedFallbackDockPosition() for both ship.Positions[PropertyLocationType.DockPosition] and PropertyLocationType.LastNPCDockPosition instead of reusing cz220Cache; replace those two calls with cloned copies of the already-resolved cz220Cache (via PropertyLocation.Clone()) so each position gets an independent instance while avoiding redundant GetWaypointByTag/GetArea/GetPosition calls and possible transient divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 1082-1086: The warning about an unrecoverable dock position loss
is being logged to LogGroup.Property; change the Log.Write call so it uses
LogGroup.Error instead of LogGroup.Property to ensure operators see it as an
error; do the same for the companion DB.Get<WorldProperty> miss noted earlier
(replace its Log.Write LogGroup.Property usage with LogGroup.Error). Ensure the
Log.Write signatures and message text remain unchanged except for the LogGroup
enum value.
---
Outside diff comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 422-441: The code in Property.cs unguards access to
property.Positions[PropertyLocationType.LastNPCDockPosition] when replacing a
missing player dock, which can throw KeyNotFoundException and abort CleanUpData;
update the block in the starship cleanup logic (same area as the DockPosition
handling) to first check
property.Positions.ContainsKey(PropertyLocationType.LastNPCDockPosition)
(matching RelocateDockedShips) and if missing either log a warning via Log.Write
and skip relocation or use a safe fallback before calling Clone(), ensuring you
reference Property.Positions, PropertyLocationType.DockPosition,
PropertyLocationType.LastNPCDockPosition, and preserve the existing log message
when relocation occurs.
---
Nitpick comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 1060-1080: The code re-computes the CZ-220 fallback waypoint twice
per ship by calling BuildGuaranteedFallbackDockPosition() for both
ship.Positions[PropertyLocationType.DockPosition] and
PropertyLocationType.LastNPCDockPosition instead of reusing cz220Cache; replace
those two calls with cloned copies of the already-resolved cz220Cache (via
PropertyLocation.Clone()) so each position gets an independent instance while
avoiding redundant GetWaypointByTag/GetArea/GetPosition calls and possible
transient divergence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8020b6b3-1187-4ca1-a8c8-c7417a7d06b8
📒 Files selected for processing (2)
SWLOR.Game.Server/Service/Property.csSWLOR.Game.Server/Service/PropertyService/PropertyLocation.cs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
SWLOR.Game.Server/Service/Property.cs (2)
446-456: Mirror theRelocateDockedShipscache-and-Clone()pattern here.This branch calls
BuildGuaranteedFallbackDockPosition()twice per stranded ship — once intocz220Dockand again on Line 453 — to avoid aliasing. The newRelocateDockedShipshelper (Lines 1083-1099) handles the same problem more cleanly: resolve the waypoint once, thenClone()for eachPositionskey. That pattern is also faster across the loop becauseGetWaypointByTag/GetArea/GetPositiononly run once total instead of twice per stranded ship, and it keeps the two cleanup paths consistent so future edits to one don't drift from the other.♻️ Suggested alignment with RelocateDockedShips
- var cz220Dock = BuildGuaranteedFallbackDockPosition(); - if (cz220Dock != null) - { - property.Positions[PropertyLocationType.DockPosition] = cz220Dock; - property.Positions[PropertyLocationType.LastNPCDockPosition] = BuildGuaranteedFallbackDockPosition(); + var cz220Dock = BuildGuaranteedFallbackDockPosition(); + if (cz220Dock != null) + { + property.Positions[PropertyLocationType.DockPosition] = cz220Dock.Clone(); + property.Positions[PropertyLocationType.LastNPCDockPosition] = cz220Dock.Clone();Optionally hoist
BuildGuaranteedFallbackDockPosition()out of theforeach (var property in starshipProperties)loop and lazy-resolve it (the same pattern ascz220Cache/cz220Resolvedon Lines 1052-1053) so the waypoint is looked up at most once across the wholeCleanUpDatapass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SWLOR.Game.Server/Service/Property.cs` around lines 446 - 456, This code calls BuildGuaranteedFallbackDockPosition() twice which re-resolves the waypoint and causes aliasing; mirror the RelocateDockedShips pattern by resolving the fallback once into a local cache (like cz220Cache/cz220Resolved), then assign property.Positions[PropertyLocationType.DockPosition] = cz220Cache.Clone() and property.Positions[PropertyLocationType.LastNPCDockPosition] = cz220Cache.Clone(); alternatively hoist the cached resolve outside the starship loop so BuildGuaranteedFallbackDockPosition() runs at most once per CleanUpData pass, and ensure you use Clone() to avoid shared references between the two Positions entries.
1057-1067: Surface the missing-ship case in logs.When
DB.Get<WorldProperty>(shipId)returnsnullwe silentlycontinue, which is inconsistent with the equivalent missing-child path inDeleteProperty(Line 900) that logs atLogGroup.Error. A starport tracking a ship id that no longer resolves is the same class of orphaned reference this PR is trying to make visible — it should be logged the same way so operators don't have to guess why a stranded ship never showed up at CZ-220.🪵 Suggested log on the missing-ship branch
var ship = DB.Get<WorldProperty>(shipId); if (ship == null) + { + Log.Write(LogGroup.Error, $"Starport '{property.CustomName}' ({property.Id}) referenced docked ship '{shipId}' which no longer exists. Skipping relocation.", true); continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SWLOR.Game.Server/Service/Property.cs` around lines 1057 - 1067, The loop over dockedShipIds silently skips when DB.Get<WorldProperty>(shipId) returns null; add an error log (using the same LogGroup.Error pattern used in DeleteProperty) that reports the missing shipId and the enclosing property context (e.g., the starport/property.Id or property.Name) so orphaned ship references are visible to operators; make this change in the same foreach that uses dockedShipIds and DB.Get<WorldProperty>(shipId) (referencing the ship variable and the current property variable) and ensure the new log message mirrors the style and severity used at the DeleteProperty missing-child branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 446-456: This code calls BuildGuaranteedFallbackDockPosition()
twice which re-resolves the waypoint and causes aliasing; mirror the
RelocateDockedShips pattern by resolving the fallback once into a local cache
(like cz220Cache/cz220Resolved), then assign
property.Positions[PropertyLocationType.DockPosition] = cz220Cache.Clone() and
property.Positions[PropertyLocationType.LastNPCDockPosition] =
cz220Cache.Clone(); alternatively hoist the cached resolve outside the starship
loop so BuildGuaranteedFallbackDockPosition() runs at most once per CleanUpData
pass, and ensure you use Clone() to avoid shared references between the two
Positions entries.
- Around line 1057-1067: The loop over dockedShipIds silently skips when
DB.Get<WorldProperty>(shipId) returns null; add an error log (using the same
LogGroup.Error pattern used in DeleteProperty) that reports the missing shipId
and the enclosing property context (e.g., the starport/property.Id or
property.Name) so orphaned ship references are visible to operators; make this
change in the same foreach that uses dockedShipIds and
DB.Get<WorldProperty>(shipId) (referencing the ship variable and the current
property variable) and ensure the new log message mirrors the style and severity
used at the DeleteProperty missing-child branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79f1f54c-fbcc-4e50-b48a-afa93427b751
📒 Files selected for processing (1)
SWLOR.Game.Server/Service/Property.cs
|
@coderabbitai - Any other issues you can find that haven't been resolved already? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me do a deeper look at the changed files to find any remaining issues. 🏁 Script executed: Length of output: 500 🏁 Script executed: Length of output: 120487 🏁 Script executed: Length of output: 7941 🏁 Script executed: Length of output: 29373
1. 🐛
|
|
@coderabbitai - Open tickets for the extra issues you found and provide AI agent prompts for each of the 5 issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainOpening all 5 issues now. 🏁 Script executed: Length of output: 325
Issue 1 —
|
|
@coderabbitai - Review now that the rate limit is over |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SWLOR.Game.Server/Service/DBService/DBQuery.cs`:
- Around line 59-60: The current early return "if (list.Count == 0) return this"
in DBQuery<T> turns an empty filter into "no filter" which widens results;
change both overloads (the blocks at the shown diff and at lines ~94-95) to
instead mark the query as match-none. Implement this by setting the DBQuery<T>
internal state to a match-none sentinel (e.g., a private _matchNone flag or
equivalent in DBQuery<T>) or by appending a false condition so subsequent
processing returns zero rows; update methods that build/execute the query to
check that match-none flag and short-circuit to an empty result. Ensure the
change is applied to both overloads that currently return this on empty lists.
In `@SWLOR.Game.Server/Service/Property.cs`:
- Around line 436-467: When handling the dbStarport == null repair path inside
the CleanUpData branch that relocates DockPosition (the code that checks
property.Positions and/or builds cz220Dock), also clear the stale
RegisteredStarport relation so the ship is no longer marked as docked to a
deleted starport: remove or clear
property.ChildPropertyIds[PropertyChildType.RegisteredStarport] (or call the
existing helper that unregisters a child) in both the LastNPCDockPosition and
cz220 fallback branches after setting DockPosition/LastNPCDockPosition, and log
the change similarly; do not leave the RegisteredStarport entry populated when
the starport record is missing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 660e169a-bf30-476f-bf81-044f4f38bae5
📒 Files selected for processing (3)
SWLOR.Game.Server/Feature/PropertyLayoutDefinition/StarportLayoutDefinition.csSWLOR.Game.Server/Service/DBService/DBQuery.csSWLOR.Game.Server/Service/Property.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- SWLOR.Game.Server/Feature/PropertyLayoutDefinition/StarportLayoutDefinition.cs
| if (list.Count == 0) | ||
| return this; |
There was a problem hiding this comment.
Fail closed on empty enumerable filters.
Returning this here turns “match none” into “drop the filter entirely”. That broadens callers unexpectedly — for example, Property.GetCategoryPermissions would return all of a player's permissions when categoryIds is empty instead of none.
💡 Safer direction
- if (list.Count == 0)
- return this;
+ if (list.Count == 0)
+ throw new ArgumentException("Search list cannot be empty.", nameof(search));Apply the same change to both overloads, or introduce an explicit internal match-none state in DBQuery<T> if empty lists should mean zero results.
Also applies to: 94-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SWLOR.Game.Server/Service/DBService/DBQuery.cs` around lines 59 - 60, The
current early return "if (list.Count == 0) return this" in DBQuery<T> turns an
empty filter into "no filter" which widens results; change both overloads (the
blocks at the shown diff and at lines ~94-95) to instead mark the query as
match-none. Implement this by setting the DBQuery<T> internal state to a
match-none sentinel (e.g., a private _matchNone flag or equivalent in
DBQuery<T>) or by appending a false condition so subsequent processing returns
zero rows; update methods that build/execute the query to check that match-none
flag and short-circuit to an empty result. Ensure the change is applied to both
overloads that currently return this on empty lists.
|
@coderabbitai - Please update with the latest changes made |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 159887
✅ Issue
|
|
@coderabbitai - Anything left to fix? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 159908
However, I spotted one remaining gap in // StructureChangedAction.cs — RetrieveStarport()
var dbCity = DB.Get<WorldProperty>(property.ParentPropertyId);
var cityArea = Area.GetAreaByResref(dbCity.ParentPropertyId); // ← NRE if dbCity is null
var instance = Property.GetRegisteredInstance(interiorId); // ← KeyNotFoundException if not in cacheTwo concerns here:
Suggested fix: var dbCity = DB.Get<WorldProperty>(property.ParentPropertyId);
if (dbCity == null)
{
Log.Write(LogGroup.Error, $"RetrieveStarport: parent city '{property.ParentPropertyId}' not found for property '{property.CustomName}' ({property.Id}). Skipping dock point removal.", true);
return;
}
if (!_propertyInstances.ContainsKey(interiorId))
{
Log.Write(LogGroup.Error, $"RetrieveStarport: interior '{interiorId}' was not registered as an instance. Skipping dock point removal.", true);
return;
}
var cityArea = Area.GetAreaByResref(dbCity.ParentPropertyId);
var instance = Property.GetRegisteredInstance(interiorId);
var dockPoint = GetLandingWaypoint(instance.Area);
Space.RemoveLandingPoint(dockPoint, cityArea);Note: 🧠 Learnings used |
|
@codex review this PR |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor