Try2 #161
Try2 #161
Conversation
* babyproof arrivals shuttle * always powered lights * uncuttable cables from terminal PR. --------- Co-authored-by: slarticodefast <[email protected]>
It functions identically to how V1 of orientation worked and it's incredibly annoying.
* Draw muzzle flash below mobs * Better naming --------- Co-authored-by: Winkarst <[email protected]>
This check conflicts with an attempt to FTL from the planet before expedition ends
Co-authored-by: deltanedas <@deltanedas:kde.org>
* begin * bones + pyrotten + goliath hide inhands * Update Resources/Prototypes/Entities/Objects/Materials/materials.yml Co-authored-by: slarticodefast <[email protected]> * Update Resources/Textures/Objects/Materials/materials.rsi/meta.json Co-authored-by: slarticodefast <[email protected]> * Update Resources/Textures/Objects/Materials/materials.rsi/meta.json Co-authored-by: slarticodefast <[email protected]> * Update Resources/Textures/Objects/Materials/materials.rsi/meta.json Co-authored-by: slarticodefast <[email protected]> * pyrottOn --------- Co-authored-by: slarticodefast <[email protected]>
* add headset * Add icons * Meta change * fix * Revert + delete headset from locker
fix door auto close timer
* fix bug, in ghost command lobby * fix * Fix build
removed opsolete netmessage createor
Shark goes rawr more
The light itself should already turn off due to `LitOnPowered` component, but the broken state of a VendingMachine did not. Fixes #33382
* adds more diona names * more stuff * AHHHHHHHHHHHHHHHHHHHHHHH * further additions * removes depression + adds comment * fixes + remove some weird stuff + more stuff * remove haste
* minor cleanup * to
* dropdown shares margin width with children * removed dependency that rider added for some reason * reduced vertical margin from 8 to 2
* proof of concept * full implementation * I commited a crime * t * min players increase
* Make shuttle airlocks not snapcardinals * Update Resources/Prototypes/Entities/Structures/Doors/Airlocks/shuttle.yml Co-authored-by: slarticodefast <[email protected]> * Update Resources/Prototypes/Entities/Structures/Doors/Airlocks/shuttle.yml Co-authored-by: slarticodefast <[email protected]> --------- Co-authored-by: Winkarst <[email protected]> Co-authored-by: slarticodefast <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (54)
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatLogEntry.xaml.cs (1)
10-16: 🛠️ Refactor suggestion
Add parameter validation in the constructor.
While the implementation is straightforward, it's missing validation for the input parameters. This could lead to potential issues if null or invalid values are passed.
Consider adding parameter validation:
public NanoChatLogEntry(int number, string time, string message) { + ArgumentNullException.ThrowIfNull(time); + ArgumentNullException.ThrowIfNull(message); + RobustXamlLoader.Load(this); NumberLabel.Text = number.ToString(); TimeLabel.Text = time; MessageLabel.Text = message; }📝 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.public NanoChatLogEntry(int number, string time, string message) { ArgumentNullException.ThrowIfNull(time); ArgumentNullException.ThrowIfNull(message); RobustXamlLoader.Load(this); NumberLabel.Text = number.ToString(); TimeLabel.Text = time; MessageLabel.Text = message; }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml.cs (2)
44-60:
⚠️ Potential issueAdd null checks and error handling for UI controls.
The UI manipulation code assumes all controls exist and are properly initialized. Consider adding null checks and error handling.
+ if (MessageContainer == null || MessageBox == null || + FlexSpace == null || LeftSpacer == null || RightSpacer == null) + { + Logger.Error($"{nameof(NanoChatMessageBubble)}: Required UI controls are not initialized"); + return; + } + MessageContainer.RemoveAllChildren(); MessageBox.Parent?.RemoveChild(MessageBox);📝 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.if (MessageContainer == null || MessageBox == null || FlexSpace == null || LeftSpacer == null || RightSpacer == null) { Logger.Error($"{nameof(NanoChatMessageBubble)}: Required UI controls are not initialized"); return; } MessageContainer.RemoveAllChildren(); // fuuuuuck MessageBox.Parent?.RemoveChild(MessageBox); if (isOwnMessage) { MessageContainer.AddChild(FlexSpace); MessageContainer.AddChild(MessageBox); MessageContainer.AddChild(RightSpacer); } else { MessageContainer.AddChild(LeftSpacer); MessageContainer.AddChild(MessageBox); MessageContainer.AddChild(FlexSpace); }
25-26: 🛠️ Refactor suggestion
Add error logging for invalid panel style.
The early return should log an error to help diagnose styling issues during development.
if (MessagePanel.PanelOverride is not StyleBoxFlat) +{ + Logger.Warning($"{nameof(NanoChatMessageBubble)}: MessagePanel style is not StyleBoxFlat"); return; +}📝 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.if (MessagePanel.PanelOverride is not StyleBoxFlat) { Logger.Warning($"{nameof(NanoChatMessageBubble)}: MessagePanel style is not StyleBoxFlat"); return; }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml (2)
149-161: 🛠️ Refactor suggestion
Enhance message input feedback.
The message input could benefit from:
- Visual feedback for maximum length
- Clear indication of why the send button is disabled
<LineEdit Name="MessageInput" PlaceHolder="{Loc nano-chat-message-placeholder}" HorizontalExpand="True" + MaxLength="1000" + ToolTip="{Loc nano-chat-max-length-tooltip}" StyleClasses="OpenRight" /> <Button Name="SendButton" MinSize="32 32" Disabled="True" + ToolTip="{Loc nano-chat-send-disabled-tooltip}" StyleClasses="OpenLeft" Margin="4 0 0 0">Committable suggestion skipped: line range outside the PR's diff.
125-133: 🛠️ Refactor suggestion
Add loading and error states to the messages area.
The messages scroll container should handle loading and error states to provide better user feedback.
<ScrollContainer Name="MessagesScroll" VerticalExpand="True" HorizontalExpand="True" Visible="False"> + <BoxContainer Orientation="Vertical"> + <!-- Loading indicator --> + <Control Name="LoadingIndicator" Visible="False"> + <Label Text="{Loc nano-chat-loading}" + HorizontalAlignment="Center" + StyleClasses="LabelSubText" /> + </Control> + <!-- Error message --> + <Control Name="ErrorMessage" Visible="False"> + <Label Text="{Loc nano-chat-error}" + HorizontalAlignment="Center" + StyleClasses="LabelSubText" /> + </Control> <BoxContainer Name="MessageList" Orientation="Vertical" VerticalExpand="True" HorizontalExpand="True" /> + </BoxContainer> </ScrollContainer>📝 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.<ScrollContainer Name="MessagesScroll" VerticalExpand="True" HorizontalExpand="True" Visible="False"> <BoxContainer Orientation="Vertical"> <!-- Loading indicator --> <Control Name="LoadingIndicator" Visible="False"> <Label Text="{Loc nano-chat-loading}" HorizontalAlignment="Center" StyleClasses="LabelSubText" /> </Control> <!-- Error message --> <Control Name="ErrorMessage" Visible="False"> <Label Text="{Loc nano-chat-error}" HorizontalAlignment="Center" StyleClasses="LabelSubText" /> </Control> <BoxContainer Name="MessageList" Orientation="Vertical" VerticalExpand="True" HorizontalExpand="True" /> </BoxContainer> </ScrollContainer>Content.Client/Lathe/UI/LatheMenu.xaml.cs (3)
96-106: 🛠️ Refactor suggestion
Improve null checks and card validation
The card validation logic could be improved to be more explicit and handle edge cases better.
private void UpdateMiningPoints(uint points) { + var session = _player.LocalSession; + var player = session?.AttachedEntity; + var hasValidCard = player != null && _miningPoints.TryFindIdCard(player) != null; + MiningPointsClaimButton.Disabled = points == 0 || - _player.LocalSession?.AttachedEntity is not {} player || - _miningPoints.TryFindIdCard(player) == null; + !hasValidCard;📝 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.private void UpdateMiningPoints(uint points) { var session = _player.LocalSession; var player = session?.AttachedEntity; var hasValidCard = player != null && _miningPoints.TryFindIdCard(player) != null; MiningPointsClaimButton.Disabled = points == 0 || !hasValidCard; if (points == _lastMiningPoints) return; _lastMiningPoints = points; MiningPointsLabel.Text = Loc.GetString("lathe-menu-mining-points", ("points", points)); }
121-127: 🛠️ Refactor suggestion
Optimize frame updates for mining points
The current implementation checks mining points every frame, which could be inefficient. Consider using an event-based approach or updating at a lower frequency.
+ private float _updateAccumulator = 0f; + private const float UpdateInterval = 0.5f; // Update every 500ms + protected override void FrameUpdate(FrameEventArgs args) { base.FrameUpdate(args); + _updateAccumulator += args.DeltaSeconds; + if (_updateAccumulator < UpdateInterval) + return; + + _updateAccumulator = 0f; if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) UpdateMiningPoints(points.Points); }📝 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.private float _updateAccumulator = 0f; private const float UpdateInterval = 0.5f; // Update every 500ms protected override void FrameUpdate(FrameEventArgs args) { base.FrameUpdate(args); _updateAccumulator += args.DeltaSeconds; if (_updateAccumulator < UpdateInterval) return; _updateAccumulator = 0f; if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) UpdateMiningPoints(points.Points); }
83-89:
⚠️ Potential issueAdd cleanup for event handler registration
The event handler for
MiningPointsClaimButton.OnPressedis added but never removed, which could lead to memory leaks or multiple handler registrations.+ private void CleanupMiningPoints() + { + if (MiningPointsClaimButton != null) + MiningPointsClaimButton.OnPressed -= _ => OnClaimMiningPoints?.Invoke(); + } public void SetEntity(EntityUid uid) { + CleanupMiningPoints(); Entity = uid;Committable suggestion skipped: line range outside the PR's diff.
Content.Server/Connection/ConnectionManager.cs (1)
203-205: 🛠️ Refactor suggestion
Review the simultaneous use of
hwIdandmodernHwidIn
ShouldDeny, bothhwIdandmodernHwidare passed toGetServerBansAsync. Consider whether it's necessary to pass both, or ifmodernHwidreplaceshwId. Simplifying this could reduce redundancy and potential confusion.Content.Server/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (2)
33-37:
⚠️ Potential issueUndefined variable
SubsinInitializemethodThe variable
Subsis used but not defined within the context of theInitializemethod. This will result in a compile-time error.Please ensure that
Subsis properly defined or replaced with the appropriate method for subscribing to UI events. Here's a possible correction:- Subs.BuiEvents<DockingConsoleComponent>(DockingConsoleUiKey.Key, subs => - { - subs.Event<BoundUIOpenedEvent>(OnOpened); - subs.Event<DockingConsoleFTLMessage>(OnFTL); - }); + SubscribeLocalEvent<DockingConsoleComponent, BoundUIOpenedEvent>(OnOpened); + SubscribeLocalEvent<DockingConsoleComponent, DockingConsoleFTLMessage>(OnFTL);📝 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.SubscribeLocalEvent<DockingConsoleComponent, BoundUIOpenedEvent>(OnOpened); SubscribeLocalEvent<DockingConsoleComponent, DockingConsoleFTLMessage>(OnFTL);
109-110:
⚠️ Potential issueOff-by-one error in destination index check
The condition checking
args.Indexmay allow an out-of-range index, potentially causing anIndexOutOfRangeExceptionwhen accessingdocking.Destinations[args.Index].Adjust the condition to ensure that
args.Indexis within the valid range:- if (args.Index < 0 || args.Index > docking.Destinations.Count) + if (args.Index < 0 || args.Index >= docking.Destinations.Count)This change prevents accessing an index equal to
docking.Destinations.Count, which is outside the bounds of the list.📝 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.if (args.Index < 0 || args.Index >= docking.Destinations.Count) return;Content.Server.Database/Migrations/Postgres/PostgresServerDbContextModelSnapshot.cs (4)
1795-1824: 🛠️ Refactor suggestion
Optimize the
TypedHwidConfiguration inServerBanIn the
ServerBanentity, theTypedHwidowned entity includes explicit table mapping and verbose constraint names that might not be needed. Simplifying these configurations can reduce complexity and potential confusion in the model.
1872-1901: 🛠️ Refactor suggestion
Ensure Consistent Mapping for
HWIdOwned Entity inServerRoleBanThe
HWIdowned entity inServerRoleBanfollows a similar pattern with explicit table mappings and detailed constraint names. Reviewing and refining these configurations to rely on defaults where appropriate can improve the model's readability and maintain consistency across entities.
1627-1654: 🛠️ Refactor suggestion
Simplify the Configuration of the
TypedHwidOwned Entity inConnectionLogThe mapping of the
TypedHwidowned entity withinConnectionLogincludes explicit table mapping withToTable("connection_log")and a constraint namedFK_connection_log_connection_log_connection_log_id. These may be redundant as owned entities share the table with their owner by default. Consider removing unnecessary specifications to streamline the configuration and improve maintainability.
1670-1699: 🛠️ Refactor suggestion
Review the Mapping of
LastSeenHWIdin thePlayerEntityThe
LastSeenHWIdowned entity inPlayerspecifiesToTable("player")and uses a constraint name likeFK_player_player_player_id, which may be unnecessary and can reduce readability. Streamlining this configuration by relying on default behaviors can enhance clarity and consistency.Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml.cs (1)
44-45:
⚠️ Potential issueAvoid Early Return in Constructor
Returning early from the constructor may result in an incompletely initialized object, which could lead to unexpected behavior if any methods are called on the partially constructed instance.
Consider refactoring the constructor to ensure the object is fully initialized. You might handle the absence of
DockingConsoleComponentby throwing an exception or by using a factory pattern to only create instances when the component is available.Apply this diff to refactor:
public DockingConsoleWindow(EntityUid owner) { RobustXamlLoader.Load(this); IoCManager.InjectDependencies(this); _access = _entMan.System<AccessReaderSystem>(); _owner = owner; _ftlStyle = new StyleBoxFlat(Color.LimeGreen); FTLBar.ForegroundStyleBoxOverride = _ftlStyle; - if (!_entMan.TryGetComponent<DockingConsoleComponent>(owner, out var comp)) - return; + if (!_entMan.TryGetComponent<DockingConsoleComponent>(owner, out var comp)) + throw new InvalidOperationException("DockingConsoleComponent not found on the entity."); Title = Loc.GetString(comp.WindowTitle); // Rest of the code... }📝 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.if (!_entMan.TryGetComponent<DockingConsoleComponent>(owner, out var comp)) throw new InvalidOperationException("DockingConsoleComponent not found on the entity.");Content.Server.Database/Migrations/Postgres/20241111193608_ConnectionTrust.Designer.cs (1)
604-606: 🛠️ Refactor suggestion
Consider using 'double' or 'decimal' instead of 'float' for 'Trust' property
Using a
floatfor theTrustproperty may lead to precision issues due to the limited precision of floating-point numbers in binary representation. It's recommended to usedoubleordecimaltypes for higher precision, especially if the trust values require exactness in calculations or comparisons.Apply this diff to update the
Trustproperty:- b.Property<float>("Trust") + b.Property<double>("Trust")📝 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.b.Property<double>("Trust") .HasColumnType("real") .HasColumnName("trust");Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml.cs (3)
127-129: 🛠️ Refactor suggestion
Avoid modifying mutable structs in collections
You're modifying the
recipientretrieved from the_recipientsdictionary and then reassigning it back. IfNanoChatRecipientis a struct, this pattern can lead to potential issues due to value copying and immutability expectations.Consider changing
NanoChatRecipientto a class to avoid these issues or avoid reassigning the modified struct back into the dictionary.
45-49:
⚠️ Potential issueHandle potential null reference for
NewChatButtonEnsure that
NewChatButtonis not null before attaching theOnPressedevent handler to prevent potential null reference exceptions.Apply this diff to add a null check:
+if (NewChatButton != null) { NewChatButton.OnPressed += _ => { _newChatPopup.ClearInputs(); _newChatPopup.OpenCentered(); }; +}📝 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.if (NewChatButton != null) { NewChatButton.OnPressed += _ => { _newChatPopup.ClearInputs(); _newChatPopup.OpenCentered(); }; }
68-76: 🛠️ Refactor suggestion
Properly manage the
LabelDangerstyle class to prevent accumulationIn the
MessageInput.OnTextChangedevent handler, you're adding theLabelDangerstyle class when the message length exceedsMaxMessageLength. However, you don't remove this style class when the length is within limits. This could lead to the style class being applied even when it's not needed.Apply this diff to manage the style class correctly:
if (length > MaxMessageLength) { CharacterCount.Text = Loc.GetString("nano-chat-message-too-long", ("current", length), ("max", MaxMessageLength)); - CharacterCount.StyleClasses.Add("LabelDanger"); + CharacterCount.StyleClasses.Set("LabelDanger", true); } +else +{ + CharacterCount.Visible = false; + CharacterCount.StyleClasses.Set("LabelDanger", false); +}Committable suggestion skipped: line range outside the PR's diff.
Content.Server.Database/Migrations/Sqlite/SqliteServerDbContextModelSnapshot.cs (2)
1550-1577:
⚠️ Potential issueReview the use of
ToTablein owned entity configurationsIn the configurations of the
TypedHwidowned type withinConnectionLog(lines 1550-1577),Player(lines 1593-1622),ServerBan(lines 1718-1747), andServerRoleBan(lines 1795-1824), theToTablemethod is called to specify the table name. Since owned types share the table with their owners by default, specifyingToTablemay be unnecessary or could cause unintended behavior. Consider removing theToTablecalls to ensure correct mapping.Apply the following diffs to remove the
ToTablecalls:ConnectionLog:
b1.HasKey("ConnectionLogId"); - b1.ToTable("connection_log"); b1.WithOwner() .HasForeignKey("ConnectionLogId") .HasConstraintName("FK_connection_log_connection_log_connection_log_id");Player:
b1.HasKey("PlayerId"); - b1.ToTable("player"); b1.WithOwner() .HasForeignKey("PlayerId") .HasConstraintName("FK_player_player_player_id");ServerBan:
b1.HasKey("ServerBanId"); - b1.ToTable("server_ban"); b1.WithOwner() .HasForeignKey("ServerBanId") .HasConstraintName("FK_server_ban_server_ban_server_ban_id");ServerRoleBan:
b1.HasKey("ServerRoleBanId"); - b1.ToTable("server_role_ban"); b1.WithOwner() .HasForeignKey("ServerRoleBanId") .HasConstraintName("FK_server_role_ban_server_role_ban_server_role_ban_id");Also applies to: 1593-1622, 1718-1747, 1795-1824
568-571:
⚠️ Potential issueEnsure the
Trustproperty handles existing data correctlyThe new
Trustproperty inConnectionLogis defined as a non-nullablefloat. This may cause issues with existing records lacking this field. Consider setting a default value or making the property nullable to prevent migration errors.Apply this diff to set a default value:
b.Property<float>("Trust") .HasColumnType("REAL") .HasColumnName("trust") + .HasDefaultValue(0);📝 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.b.Property<float>("Trust") .HasColumnType("REAL") .HasColumnName("trust") .HasDefaultValue(0);Content.Server/DeltaV/Station/Systems/StationPlanetSpawnerSystem.cs (2)
1-4: 🛠️ Refactor suggestion
Add missing using statement for EntitySystem.
The code is missing the required using statement for the EntitySystem base class.
Add this line at the top:
+using Robust.Shared.GameObjects; using Content.Server.DeltaV.Planet; using Content.Server.DeltaV.Station.Components;📝 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.using Robust.Shared.GameObjects; using Content.Server.DeltaV.Planet; using Content.Server.DeltaV.Station.Components; namespace Content.Server.DeltaV.Station.Systems;
18-29: 💡 Codebase verification
⚠️ Potential issueBased on the analysis of the code and usage patterns, I can now generate the final response:
Add error handling but remove unnecessary checks.
The suggested error handling for LoadPlanet is good practice, but some of the proposed changes are unnecessary:
- The
_planetdependency is injected by the dependency injection system and will never be null in a properly configured environment.- The
Planetfield is marked as required in the component definition, so it cannot be null/empty.- The map deletion in OnShutdown is safe as the Map field is only set after successful planet loading.
Here's the recommended implementation:
private void OnMapInit(Entity<StationPlanetSpawnerComponent> ent, ref MapInitEvent args) { if (ent.Comp.GridPath is not {} path) return; + try + { ent.Comp.Map = _planet.LoadPlanet(ent.Comp.Planet, path.ToString()); + } + catch (Exception e) + { + Logger.Error($"Failed to load planet {ent.Comp.Planet}: {e}"); + } } private void OnShutdown(Entity<StationPlanetSpawnerComponent> ent, ref ComponentShutdown args) { if (!EntityManager.IsAlive(ent.Comp.Map)) return; QueueDel(ent.Comp.Map); }🔗 Analysis chain
Add error handling and validation.
The event handlers have several potential issues that should be addressed:
- Missing null check for the
_planetdependency- No error handling for the
LoadPlanetoperation- Missing validation of the
Planetproperty- Potential race condition in map deletion
Consider implementing these improvements:
private void OnMapInit(Entity<StationPlanetSpawnerComponent> ent, ref MapInitEvent args) { if (ent.Comp.GridPath is not {} path) return; + if (string.IsNullOrEmpty(ent.Comp.Planet)) + { + Logger.Warning($"StationPlanetSpawner at {ent} has no Planet specified"); + return; + } + + try + { ent.Comp.Map = _planet.LoadPlanet(ent.Comp.Planet, path.ToString()); + } + catch (Exception e) + { + Logger.Error($"Failed to load planet {ent.Comp.Planet}: {e}"); + } } private void OnShutdown(Entity<StationPlanetSpawnerComponent> ent, ref ComponentShutdown args) { + if (!EntityManager.IsAlive(ent.Comp.Map)) + return; + QueueDel(ent.Comp.Map); }Let's verify the usage of this system:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other systems that might be using StationPlanetSpawnerSystem rg -l "StationPlanetSpawnerSystem" # Check for potential race conditions in map deletion ast-grep --pattern 'QueueDel($map)'Length of output: 214
Script:
#!/bin/bash # Check the implementation of StationPlanetSpawnerComponent to understand the Map property cat Content.Server/DeltaV/Station/Components/StationPlanetSpawnerComponent.cs # Look for other usages of QueueDel with maps or entities rg "QueueDel" -A 2 -B 2 # Check for IPlanet interface or implementation to understand LoadPlanet behavior fd -e cs planet -X catLength of output: 66612
Content.Server/DeltaV/Planet/PlanetSystem.cs (2)
21-21:
⚠️ Potential issuePotential thread safety issue with the shared
_setTilesfieldThe
_setTileslist is being used as a class-level field but is only utilized within theLoadPlanetmethod. IfLoadPlanetis called concurrently, this could lead to race conditions and unexpected behavior due to shared state.To resolve this, declare
_setTilesas a local variable within theLoadPlanetmethod:- private readonly List<(Vector2i, Tile)> _setTiles = new(); ... foreach (var gridUid in grids) { - _setTiles.Clear(); + var setTiles = new List<(Vector2i, Tile)>(); var aabb = Comp<MapGridComponent>(gridUid).LocalAABB; - _biome.ReserveTiles(map, aabb.Enlarged(0.2f), _setTiles); + _biome.ReserveTiles(map, aabb.Enlarged(0.2f), setTiles); }Also applies to: 68-70
28-28: 🛠️ Refactor suggestion
Handle potential exceptions when indexing prototypes
The call to
_proto.Index(id)may throw an exception if the prototype with the giveniddoes not exist. Consider adding error handling to manage this scenario gracefully.Apply this change to handle exceptions:
try { var planet = _proto.Index(id); } catch (UnknownPrototypeException e) { Log.Error($"Planet prototype with ID {id} not found: {e.Message}"); throw; }📝 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.try { var planet = _proto.Index(id); } catch (UnknownPrototypeException e) { Log.Error($"Planet prototype with ID {id} not found: {e.Message}"); throw; }Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeSystem.cs (3)
245-247:
⚠️ Potential issueAvoid logging message content to prevent PII leakage
Logging the content of user messages (
msg.Content) may expose sensitive information and could lead to privacy concerns. Consider removing message content from the admin logs or ensure that logs are appropriately secured and comply with data protection policies.Apply this diff to exclude message content from the logs:
_adminLogger.Add(LogType.Chat, LogImpact.Low, - $"{ToPrettyString(card):user} sent NanoChat message to {recipientsText}: {msg.Content}{(deliveryFailed ? " [DELIVERY FAILED]" : "")}"); + $"{ToPrettyString(card):user} sent NanoChat message to {recipientsText}{(deliveryFailed ? " [DELIVERY FAILED]" : "")}");Alternatively, if logging message content is necessary, ensure compliance with privacy regulations and implement proper data handling procedures.
📝 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._adminLogger.Add(LogType.Chat, LogImpact.Low, $"{ToPrettyString(card):user} sent NanoChat message to {recipientsText}{(deliveryFailed ? " [DELIVERY FAILED]" : "")}");
278-281: 🛠️ Refactor suggestion
Rename 'recipient' to 'recipients' for clarity
In the
AttemptMessageDeliverymethod, the variablerecipientis a list of recipients. Renaming it torecipientsimproves readability and aligns with standard naming conventions.Apply this diff to rename the variable:
-private (bool failed, List<Entity<NanoChatCardComponent>> recipient) AttemptMessageDelivery( +private (bool failed, List<Entity<NanoChatCardComponent>> recipients) AttemptMessageDelivery(Ensure all references to
recipientwithin the method are updated accordingly.Committable suggestion skipped: line range outside the PR's diff.
404-423:
⚠️ Potential issuePrevent potential null reference exceptions when accessing recipients
In
HandleUnreadNotification, directly accessingrecipient.Comp.Recipients[message.SenderId]without checking if the key exists can lead to aKeyNotFoundException. UseTryGetValueto safely access the recipient.Apply this diff to safely access recipient data:
- if (!recipient.Comp.Recipients[message.SenderId].HasUnread && !recipient.Comp.NotificationsMuted) + if (recipient.Comp.Recipients.TryGetValue(message.SenderId, out var recipientData) && !recipientData.HasUnread && !recipient.Comp.NotificationsMuted)Similarly, update the code when setting the
HasUnreadstatus:- _nanoChat.SetRecipient((recipient, recipient.Comp), - message.SenderId, - existingRecipient with { HasUnread = true }); + if (existingRecipient != null) + { + _nanoChat.SetRecipient((recipient, recipient.Comp), + message.SenderId, + existingRecipient with { HasUnread = true }); + }Committable suggestion skipped: line range outside the PR's diff.
Content.Server/DeltaV/Station/Components/StationPlanetSpawnerComponent.cs (1)
30-31: 🛠️ Refactor suggestion
Consider making Map property private
The
Mapproperty appears to be internal state that should be managed by theStationPlanetSpawnerSystem. Consider making it private or internal to prevent unauthorized modifications.- [DataField] - public EntityUid? Map; + [DataField] + internal EntityUid? Map;📝 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.[DataField] internal EntityUid? Map;Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUi.cs (2)
10-10:
⚠️ Potential issueProtect against potential NullReferenceException.
The
_fragmentfield is nullable, butGetUIFragmentRoot()assumes it's non-null. This could throw ifSetup()hasn't been called.Consider this safer implementation:
- return _fragment!; + if (_fragment == null) + throw new InvalidOperationException("NanoChatUi not properly initialized. Ensure Setup() is called first."); + return _fragment;Also applies to: 14-14
17-25: 🛠️ Refactor suggestion
Add parameter validation and proper cleanup.
Several improvements needed for robustness:
- Add null check for userInterface parameter
- Consider using the fragmentOwner parameter or document why it's unused
- Implement proper event handler cleanup to prevent memory leaks
Consider this implementation:
public override void Setup(BoundUserInterface userInterface, EntityUid? fragmentOwner) { + ArgumentNullException.ThrowIfNull(userInterface); + + // Cleanup existing fragment if any + if (_fragment != null) + { + _fragment.OnMessageSent -= OnMessageSent; + } + _fragment = new NanoChatUiFragment(); + _fragment.OnMessageSent += OnMessageSent; - _fragment.OnMessageSent += (type, number, content, job) => + void OnMessageSent(NanoChatUiMessageType type, uint? number, string? content, string? job) { SendNanoChatUiMessage(type, number, content, job, userInterface); - }; + } }📝 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.public override void Setup(BoundUserInterface userInterface, EntityUid? fragmentOwner) { ArgumentNullException.ThrowIfNull(userInterface); // Cleanup existing fragment if any if (_fragment != null) { _fragment.OnMessageSent -= OnMessageSent; } _fragment = new NanoChatUiFragment(); _fragment.OnMessageSent += OnMessageSent; void OnMessageSent(NanoChatUiMessageType type, uint? number, string? content, string? job) { SendNanoChatUiMessage(type, number, content, job, userInterface); } }Content.Server.Database/Model.cs (1)
714-714:
⚠️ Potential issueReview impact of changing interface property type in
IBanCommon<TUnban>Changing the
HWIdproperty type in theIBanCommon<TUnban>interface frombyte[]?toTypedHwid?is a breaking change. This affects all implementations of the interface and may impact external code if this interface is part of a public API. Ensure that all implementing classes have been updated accordingly and consider the implications for any external consumers of this interface.Content.Server/DeltaV/NanoChat/NanoChatSystem.cs (1)
75-102:
⚠️ Potential issueEnsure safe modification of collections during iteration in
ScrambleMessages.When modifying the
component.Messagesdictionary inside the loop, ensure that you're not causing unintended side effects or exceptions due to collection modification during iteration. Specifically, adding or removing keys from the dictionary while iterating over it can lead to issues.Consider collecting keys to modify in a separate list before performing the reassignment and clearing operations.
Content.Server/Administration/Managers/BanManager.cs (2)
258-258:
⚠️ Potential issueChange 'async void' to 'async Task' in method signature
The method
CreateRoleBanis declared aspublic async void, which can lead to unobserved exceptions and unintended behavior. It's recommended to useasync Taskinstead ofasync voidfor asynchronous methods to enable proper exception handling and awaiting.Apply this diff to fix the method signature:
-public async void CreateRoleBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, string role, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan) +public async Task CreateRoleBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, string role, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan)📝 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.public async Task CreateRoleBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, string role, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan)
159-159:
⚠️ Potential issueChange 'async void' to 'async Task' in method signature
The method
CreateServerBanis declared aspublic async void, which can lead to unobserved exceptions and unintended behavior. It's recommended to useasync Taskinstead ofasync voidfor asynchronous methods to enable proper exception handling and awaiting.Apply this diff to fix the method signature:
-public async void CreateServerBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, uint? minutes, NoteSeverity severity, string reason) +public async Task CreateServerBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, uint? minutes, NoteSeverity severity, string reason)📝 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.public async Task CreateServerBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, uint? minutes, NoteSeverity severity, string reason)Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiState.cs (1)
15-29: 🛠️ Refactor suggestion
Add parameter validation and XML documentation
Consider adding:
- Parameter validation for maxRecipients (should be positive)
- XML documentation for public members and constructor parameters
+ /// <summary> + /// Initializes a new instance of the NanoChatUiState class. + /// </summary> + /// <param name="recipients">Dictionary of chat recipients indexed by their IDs</param> + /// <param name="messages">Dictionary of message lists indexed by recipient IDs</param> + /// <param name="currentChat">Currently selected chat ID, if any</param> + /// <param name="ownNumber">User's own chat identifier</param> + /// <param name="maxRecipients">Maximum allowed recipients (must be positive)</param> + /// <param name="notificationsMuted">Whether notifications are currently muted</param> + /// <exception cref="ArgumentException">Thrown when maxRecipients is not positive</exception> public NanoChatUiState( Dictionary<uint, NanoChatRecipient> recipients, Dictionary<uint, List<NanoChatMessage>> messages, uint? currentChat, uint ownNumber, int maxRecipients, bool notificationsMuted) { + if (maxRecipients <= 0) + throw new ArgumentException("Maximum recipients must be positive", nameof(maxRecipients)); + Recipients = recipients; Messages = messages; CurrentChat = currentChat; OwnNumber = ownNumber; MaxRecipients = maxRecipients; NotificationsMuted = notificationsMuted; }📝 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./// <summary> /// Initializes a new instance of the NanoChatUiState class. /// </summary> /// <param name="recipients">Dictionary of chat recipients indexed by their IDs</param> /// <param name="messages">Dictionary of message lists indexed by recipient IDs</param> /// <param name="currentChat">Currently selected chat ID, if any</param> /// <param name="ownNumber">User's own chat identifier</param> /// <param name="maxRecipients">Maximum allowed recipients (must be positive)</param> /// <param name="notificationsMuted">Whether notifications are currently muted</param> /// <exception cref="ArgumentException">Thrown when maxRecipients is not positive</exception> public NanoChatUiState( Dictionary<uint, NanoChatRecipient> recipients, Dictionary<uint, List<NanoChatMessage>> messages, uint? currentChat, uint ownNumber, int maxRecipients, bool notificationsMuted) { if (maxRecipients <= 0) throw new ArgumentException("Maximum recipients must be positive", nameof(maxRecipients)); Recipients = recipients; Messages = messages; CurrentChat = currentChat; OwnNumber = ownNumber; MaxRecipients = maxRecipients; NotificationsMuted = notificationsMuted; }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml.cs (1)
23-24: 🛠️ Refactor suggestion
Consider implementing IDisposable for event cleanup
The class manages event subscriptions but doesn't implement cleanup. Consider implementing IDisposable to ensure proper event handler cleanup when the entry is removed.
-public sealed partial class NanoChatEntry : BoxContainer +public sealed partial class NanoChatEntry : BoxContainer, IDisposable { public event Action<uint>? OnPressed; private uint _number; private Action<EventArgs>? _pressHandler; + + public void Dispose() + { + if (_pressHandler != null) + ChatButton.OnPressed -= _pressHandler; + }Also applies to: 29-30
Content.Shared.Database/TypedHwid.cs (1)
21-41: 🛠️ Refactor suggestion
Consider adding validation for empty/null HWID
The
TryParsemethod should validate that the resulting HWID array is not empty.public static bool TryParse(string value, [NotNullWhen(true)] out ImmutableTypedHwid? hwid) { + if (string.IsNullOrEmpty(value)) + { + hwid = null; + return false; + } + var type = HwidType.Legacy; if (value.StartsWith("V2-", StringComparison.Ordinal)) { value = value["V2-".Length..]; type = HwidType.Modern; } var array = new byte[GetBase64ByteLength(value)]; if (!Convert.TryFromBase64String(value, array, out _)) { hwid = null; return false; } + if (array.Length == 0) + { + hwid = null; + return false; + } + hwid = new ImmutableTypedHwid(ImmutableArray.Create(array), type); return true; }📝 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.public static bool TryParse(string value, [NotNullWhen(true)] out ImmutableTypedHwid? hwid) { if (string.IsNullOrEmpty(value)) { hwid = null; return false; } var type = HwidType.Legacy; if (value.StartsWith("V2-", StringComparison.Ordinal)) { value = value["V2-".Length..]; type = HwidType.Modern; } var array = new byte[GetBase64ByteLength(value)]; if (!Convert.TryFromBase64String(value, array, out _)) { hwid = null; return false; } if (array.Length == 0) { hwid = null; return false; } hwid = new ImmutableTypedHwid(ImmutableArray.Create(array), type); return true; }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml (1)
17-27: 🛠️ Refactor suggestion
Consider making the unread indicator more maintainable.
The unread indicator has hardcoded colors and fixed sizes which could cause issues:
- Fixed pixel sizes may not scale well across different display densities
- Hardcoded colors make theming difficult
Consider moving these values to a theme or style resource:
- MinSize="8 8" - MaxSize="8 8" + MinSize="{DynamicResource UnreadIndicatorSize}" + MaxSize="{DynamicResource UnreadIndicatorSize}"- BackgroundColor="#17c622" - BorderColor="#0f7a15" + BackgroundColor="{DynamicResource UnreadIndicatorBackground}" + BorderColor="{DynamicResource UnreadIndicatorBorder}"Committable suggestion skipped: line range outside the PR's diff.
Content.Server/Administration/Managers/IBanManager.cs (2)
43-43:
⚠️ Potential issueInconsistent HWID type in WebhookUpdateRoleBans
The
WebhookUpdateRoleBansmethod still usesImmutableArray<byte>?for the HWID parameter while other methods have been updated to useImmutableTypedHwid?.- public void WebhookUpdateRoleBans(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableArray<byte>? hwid, IReadOnlyCollection<string> roles, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan); + public void WebhookUpdateRoleBans(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, IReadOnlyCollection<string> roles, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan);📝 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.public void WebhookUpdateRoleBans(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableTypedHwid? hwid, IReadOnlyCollection<string> roles, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan);
28-28: 💡 Codebase verification
HWID type inconsistency found in implementation
The verification revealed two issues:
- The
WebhookUpdateRoleBansmethod in bothIBanManagerandBanManagerstill uses the oldImmutableArray<byte>?type for the HWID parameter- The database layer (
ServerDbSqlite,ServerDbPostgres, etc.) still uses the old HWID type, suggesting this refactoring is incompleteThe type change needs to be consistently applied across:
WebhookUpdateRoleBansmethod inIBanManager.csandBanManager.cs- Database-related classes in the
Content.Server/Database/directory🔗 Analysis chain
HWID type changes look good but verify implementations
The change from
ImmutableArray<byte>?toImmutableTypedHwid?improves type safety and encapsulation. However, we should verify that all implementations have been updated accordingly.Also applies to: 41-41
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all implementations of IBanManager to verify they've been updated ast-grep --pattern 'class $_ : IBanManager' # Find any remaining usages of ImmutableArray<byte> with HWID rg -i 'ImmutableArray<byte>.*[Hh]wid'Length of output: 45
Script:
#!/bin/bash # Let's try a different approach to find implementations rg -l "IBanManager" # Also search for CreateServerBan implementations rg "CreateServerBan.*ImmutableTypedHwid" -A 2 # And look for any remaining old HWID type references rg "ImmutableArray<byte>.*[Hh][Ww][Ii][Dd]"Length of output: 7082
Content.Server/Database/ServerBanDef.cs (1)
15-15:
⚠️ Potential issueType mismatch between property and constructor parameter
The property is declared as
ImmutableTypedHwid?but the constructor parameter isTypedHwid?. This could lead to potential immutability issues.- public ImmutableTypedHwid? HWId { get; } - public ServerBanDef(..., TypedHwid? hwId, ...) + public ImmutableTypedHwid? HWId { get; } + public ServerBanDef(..., ImmutableTypedHwid? hwId, ...)Also applies to: 32-32
Content.Server/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.cs (1)
38-44: 🛠️ Refactor suggestion
Add error handling for NanoChat card scanning
The NanoChat card scanning logic lacks error handling. Consider adding try-catch blocks and validation.
if (TryComp<NanoChatCardComponent>(target, out var nanoChatCard)) { + try { ScanNanoChatCard(ent, args, target, nanoChatCard); + } catch (Exception e) { + _popupSystem.PopupCursor(Loc.GetString("log-probe-scan-failed"), args.InteractEvent.User); + Logger.Error($"Failed to scan NanoChat card: {e}"); + } args.InteractEvent.Handled = true; return; }📝 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.// DeltaV begin - Add NanoChat card scanning if (TryComp<NanoChatCardComponent>(target, out var nanoChatCard)) { try { ScanNanoChatCard(ent, args, target, nanoChatCard); } catch (Exception e) { _popupSystem.PopupCursor(Loc.GetString("log-probe-scan-failed"), args.InteractEvent.User); Logger.Error($"Failed to scan NanoChat card: {e}"); } args.InteractEvent.Handled = true; return; }Content.Client/Access/UI/AgentIDCardWindow.xaml.cs (1)
49-61: 🛠️ Refactor suggestion
Add input sanitization before text processing
The text filtering logic should sanitize input before processing to prevent potential issues.
NumberLineEdit.OnTextChanged += args => { + // Sanitize input first + args.Text = args.Text.Trim(); + if (args.Text.Length > MaxNumberLength) { NumberLineEdit.Text = args.Text[..MaxNumberLength]; } // Filter to digits only var newText = string.Concat(args.Text.Where(char.IsDigit)); if (newText != args.Text) NumberLineEdit.Text = newText; };📝 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.// DeltaV - Filter to only allow digits NumberLineEdit.OnTextChanged += args => { // Sanitize input first args.Text = args.Text.Trim(); if (args.Text.Length > MaxNumberLength) { NumberLineEdit.Text = args.Text[..MaxNumberLength]; } // Filter to digits only var newText = string.Concat(args.Text.Where(char.IsDigit)); if (newText != args.Text) NumberLineEdit.Text = newText; };Content.Server/DeltaV/Station/Systems/CaptainStateSystem.cs (1)
13-13:
⚠️ Potential issueRemove unused import
The
Microsoft.CodeAnalysis.CSharp.Syntaximport appears to be unused and should be removed.-using Microsoft.CodeAnalysis.CSharp.Syntax;Content.Server/DeltaV/Shuttles/Systems/DockingShuttleSystem.cs (1)
68-69:
⚠️ Potential issuePotential
InvalidOperationExceptionwhen accessingdata.Grids.First()If
data.Gridsis empty, callingFirst()will throw anInvalidOperationException. To prevent this, ensure thatdata.Gridscontains at least one element before accessingFirst().Apply this diff to add a check before accessing
data.Grids.First():if (_station.GetOwningStation(uid) is not {} station || !TryComp<StationDataComponent>(station, out var data)) return; + if (!data.Grids.Any()) + return; // add the source station as a destination comp.Station = station; comp.Destinations.Add(new DockingDestination() { Name = Name(station), Map = Transform(data.Grids.First()).MapID });Committable suggestion skipped: line range outside the PR's diff.
Content.Client/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (1)
5-5:
⚠️ Potential issueSyntax Error: Missing class body
The class
DockingConsoleSystemis missing its body. In C#, even if the class is empty, it must include opening and closing braces{ }.Apply this diff to fix the syntax error:
-public sealed class DockingConsoleSystem : SharedDockingConsoleSystem; +public sealed class DockingConsoleSystem : SharedDockingConsoleSystem +{ + // Class implementation goes here +}📝 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.public sealed class DockingConsoleSystem : SharedDockingConsoleSystem { // Class implementation goes here }Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml.cs (3)
68-78:
⚠️ Potential issueAdd input sanitization for name and job fields.
The current implementation doesn't sanitize the name and job inputs for potentially harmful characters.
private void CreateChat() { if (!uint.TryParse(NumberInput.Text, out var number)) return; - var name = NameInput.Text.Trim(); - var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : JobInput.Text.Trim(); + var name = SanitizeInput(NameInput.Text); + var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : SanitizeInput(JobInput.Text); OnChatCreated?.Invoke(number, name, job); Close(); } + + private static string SanitizeInput(string input) + { + // Remove potentially harmful characters and excessive whitespace + return string.Join(" ", input.Split(new[] { ' ' }, + StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) + .Replace("<", "<") + .Replace(">", ">"); + }📝 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.private void CreateChat() { if (!uint.TryParse(NumberInput.Text, out var number)) return; var name = SanitizeInput(NameInput.Text); var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : SanitizeInput(JobInput.Text); OnChatCreated?.Invoke(number, name, job); Close(); } private static string SanitizeInput(string input) { // Remove potentially harmful characters and excessive whitespace return string.Join(" ", input.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) .Replace("<", "<") .Replace(">", ">"); }
59-66:
⚠️ Potential issueStrengthen input validation.
The current validation lacks maximum value check for the number and proper input sanitization for name and job fields.
private void ValidateInputs() { + const uint MaxAllowedNumber = 9999; + var isValid = !string.IsNullOrWhiteSpace(NumberInput.Text) && !string.IsNullOrWhiteSpace(NameInput.Text) && - uint.TryParse(NumberInput.Text, out _); + uint.TryParse(NumberInput.Text, out var number) && + number <= MaxAllowedNumber; CreateButton.Disabled = !isValid; }📝 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.private void ValidateInputs() { const uint MaxAllowedNumber = 9999; var isValid = !string.IsNullOrWhiteSpace(NumberInput.Text) && !string.IsNullOrWhiteSpace(NameInput.Text) && uint.TryParse(NumberInput.Text, out var number) && number <= MaxAllowedNumber; CreateButton.Disabled = !isValid; }
28-43:
⚠️ Potential issueRemove duplicate event handler and optimize validation calls.
There are two OnTextChanged handlers for NumberInput, and ValidateInputs is called multiple times unnecessarily.
- // Input validation - NumberInput.OnTextChanged += _ => ValidateInputs(); - NameInput.OnTextChanged += _ => ValidateInputs(); - // Input validation NumberInput.OnTextChanged += args => { if (args.Text.Length > MaxNumberLength) NumberInput.Text = args.Text[..MaxNumberLength]; // Filter to digits only var newText = string.Concat(NumberInput.Text.Where(char.IsDigit)); if (newText != NumberInput.Text) NumberInput.Text = newText; ValidateInputs(); };Committable suggestion skipped: line range outside the PR's diff.
Content.Server/Access/Systems/AgentIDCardSystem.cs (1)
36-45: 🛠️ Refactor suggestion
Inconsistent method parameter type usage
The method
OnNumberChangedusesEntity<AgentIDCardComponent>as a parameter, which is inconsistent with the typical usage ofEntityUidin event handlers. This may lead to confusion and potential errors in entity identification.Consider refactoring the method to use
EntityUidas follows:-private void OnNumberChanged(Entity<AgentIDCardComponent> ent, ref AgentIDCardNumberChangedMessage args) +private void OnNumberChanged(EntityUid uid, AgentIDCardComponent component, ref AgentIDCardNumberChangedMessage args) { - if (!TryComp<NanoChatCardComponent>(ent, out var comp)) + if (!TryComp<NanoChatCardComponent>(uid, out var comp)) return; - _nanoChat.SetNumber((ent, comp), args.Number); - Dirty(ent, comp); + _nanoChat.SetNumber((uid, comp), args.Number); + Dirty(uid, comp); }📝 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.// DeltaV - Add number change handler private void OnNumberChanged(EntityUid uid, AgentIDCardComponent component, ref AgentIDCardNumberChangedMessage args) { if (!TryComp<NanoChatCardComponent>(uid, out var comp)) return; _nanoChat.SetNumber((uid, comp), args.Number); Dirty(uid, comp); }Content.Server/Administration/PlayerLocator.cs (1)
148-148:
⚠️ Potential issueIncorrect initialization of
ImmutableArrayThe syntax used to initialize
ImmutableArray<ImmutableArray<byte>>is incorrect. Using[]and[hwid.Hwid]does not create anImmutableArray. This will result in a compiler error.Apply this diff to correct the initialization:
// In HandleAuthServerResponse method return new LocatedPlayerData( new NetUserId(responseData.UserId), null, null, responseData.UserName, null, - []); + ImmutableArray<ImmutableArray<byte>>.Empty); // In ReturnForPlayerRecord method return new LocatedPlayerData( record.UserId, record.LastSeenAddress, hwid, record.LastSeenUserName, - hwid is { Type: HwidType.Legacy } ? hwid.Hwid : null, - hwid is { Type: HwidType.Modern } ? [hwid.Hwid] : []); + hwid is { Type: HwidType.Legacy } ? hwid.Hwid : null, + hwid is { Type: HwidType.Modern } ? ImmutableArray.Create(hwid.Hwid) : ImmutableArray<ImmutableArray<byte>>.Empty);Also applies to: 175-175
Content.Server/Database/ServerDbPostgres.cs (1)
180-221: 🛠️ Refactor suggestion
Null query check may lead to runtime exception
In the
MakeBanLookupQualitySharedmethod, if all parameters (userId,hwId,modernHWIds) arenull, the resultingquerywill benull, which could lead to aNullReferenceExceptionwhen the method returns and the calling code attempts to use the query.Consider adding a check to throw an
ArgumentExceptionif all parameters arenull, ensuring that the method behaves consistently and prevents runtime exceptions.+if (userId == null && hwId == null && (modernHWIds == null || !modernHWIds.Any())) +{ + throw new ArgumentException("At least one of userId, hwId, or modernHWIds must be provided."); +}📝 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.private static IQueryable<TBan>? MakeBanLookupQualityShared<TBan, TUnban>( NetUserId? userId, ImmutableArray<byte>? hwId, ImmutableArray<ImmutableArray<byte>>? modernHWIds, DbSet<TBan> set) where TBan : class, IBanCommon<TUnban> where TUnban : class, IUnbanCommon { if (userId == null && hwId == null && (modernHWIds == null || !modernHWIds.Any())) { throw new ArgumentException("At least one of userId, hwId, or modernHWIds must be provided."); } IQueryable<TBan>? query = null; if (userId is { } uid) { var newQ = set .Include(p => p.Unban) .Where(b => b.PlayerUserId == uid.UserId); query = query == null ? newQ : query.Union(newQ); } if (hwId != null && hwId.Value.Length > 0) { var newQ = set .Include(p => p.Unban) .Where(b => b.HWId!.Type == HwidType.Legacy && b.HWId!.Hwid.SequenceEqual(hwId.Value.ToArray())); query = query == null ? newQ : query.Union(newQ); } if (modernHWIds != null) { foreach (var modernHwid in modernHWIds) { var newQ = set .Include(p => p.Unban) .Where(b => b.HWId!.Type == HwidType.Modern && b.HWId!.Hwid.SequenceEqual(modernHwid.ToArray())); query = query == null ? newQ : query.Union(newQ); } } return query; }
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (46)
Content.Client/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (1)
5-5:
⚠️ Potential issueCritical: Implementation is incomplete.
The class is missing essential functionality described in the AI summary:
- Event handlers for docking/undocking
- UI update methods
- FTL state management
- Console state synchronization
- Constructor and initialization logic
Please implement the following core methods:
Initialize()- Subscribe to relevant eventsOnDock()/OnUndock()- Handle docking eventsUpdateUI()- Manage console UI stateOnFTL()- Process FTL docking requestsWould you like me to help generate the implementation for these missing components?
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml.cs (2)
46-47: 🛠️ Refactor suggestion
Clean up unprofessional comment and improve parent-child handling.
The comment and parent-child manipulation could be improved.
- // fuuuuuck - MessageBox.Parent?.RemoveChild(MessageBox); + // Ensure MessageBox is detached from any previous parent + if (MessageBox.Parent != null && MessageBox.Parent != MessageContainer) + { + MessageBox.Parent.RemoveChild(MessageBox); + }📝 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.// Ensure MessageBox is detached from any previous parent if (MessageBox.Parent != null && MessageBox.Parent != MessageContainer) { MessageBox.Parent.RemoveChild(MessageBox); }
23-24: 🛠️ Refactor suggestion
Add parameter validation and XML documentation.
The public method lacks parameter validation and documentation.
+/// <summary> +/// Configures the message bubble's appearance and layout based on the message content and sender. +/// </summary> +/// <param name="message">The chat message to display</param> +/// <param name="isOwnMessage">True if the message was sent by the current user, false otherwise</param> +/// <exception cref="ArgumentNullException">Thrown when message is null</exception> public void SetMessage(NanoChatMessage message, bool isOwnMessage) { + ArgumentNullException.ThrowIfNull(message);📝 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./// <summary> /// Configures the message bubble's appearance and layout based on the message content and sender. /// </summary> /// <param name="message">The chat message to display</param> /// <param name="isOwnMessage">True if the message was sent by the current user, false otherwise</param> /// <exception cref="ArgumentNullException">Thrown when message is null</exception> public void SetMessage(NanoChatMessage message, bool isOwnMessage) { ArgumentNullException.ThrowIfNull(message);Content.Server/Administration/Systems/BwoinkSystem.cs (1)
175-177:
⚠️ Potential issueAdd error handling for the database operation
The database call to check for bans lacks try-catch error handling, which could lead to unhandled exceptions if the database is unavailable.
- var ban = await _dbManager.GetServerBanAsync(null, e.Session.UserId, null, null); + try { + var ban = await _dbManager.GetServerBanAsync(null, e.Session.UserId, null, null); + if (ban != null) + { + var banMessage = Loc.GetString("bwoink-system-player-banned", ("banReason", ban.Reason)); + NotifyAdmins(e.Session, banMessage, PlayerStatusType.Banned); + _activeConversations.Remove(e.Session.UserId); + return; + } + } catch (Exception ex) { + _sawmill.LogError($"Failed to check ban status for {e.Session.UserId}: {ex}"); + }Committable suggestion skipped: line range outside the PR's diff.
Content.Client/Lathe/UI/LatheMenu.xaml.cs (2)
83-89: 🛠️ Refactor suggestion
Consider adding null check before updating mining points.
The mining points update could be more defensive:
-if (points != null) +if (points != null && MiningPointsContainer.Visible) UpdateMiningPoints(points.Points);📝 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.// Begin DeltaV Additions: Mining points UI MiningPointsContainer.Visible = _entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points); MiningPointsClaimButton.OnPressed += _ => OnClaimMiningPoints?.Invoke(); if (points != null && MiningPointsContainer.Visible) UpdateMiningPoints(points.Points); // End DeltaV Additions
118-127: 🛠️ Refactor suggestion
Consider optimizing the mining points update mechanism.
Checking and updating mining points every frame might be inefficient. Consider:
- Using an event-based system to update only when points change
- Adding a update frequency limiter if events aren't feasible
+private float _updateAccumulator = 0f; +private const float UPDATE_FREQUENCY = 1f; // Update once per second protected override void FrameUpdate(FrameEventArgs args) { base.FrameUpdate(args); + _updateAccumulator += args.DeltaSeconds; + if (_updateAccumulator < UPDATE_FREQUENCY) + return; + + _updateAccumulator = 0f; if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) UpdateMiningPoints(points.Points); }📝 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./// <summary> /// DeltaV: Update mining points UI whenever it changes. /// </summary> private float _updateAccumulator = 0f; private const float UPDATE_FREQUENCY = 1f; // Update once per second protected override void FrameUpdate(FrameEventArgs args) { base.FrameUpdate(args); _updateAccumulator += args.DeltaSeconds; if (_updateAccumulator < UPDATE_FREQUENCY) return; _updateAccumulator = 0f; if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) UpdateMiningPoints(points.Points); }Content.Server.Database/Migrations/Postgres/PostgresServerDbContextModelSnapshot.cs (2)
560-571:
⚠️ Potential issuePotential issue with
ValueGeneratedOnAdd()onUserIdproperty of typeGuidUsing
ValueGeneratedOnAdd()on aGuidproperty may cause issues if the database is not configured to generate GUID values automatically. Ensure that the database is set up to generate GUIDs for theUserIdcolumn, or consider generating the GUIDs within the application code before saving the entities.
1627-1654:
⚠️ Potential issueReview the usage of
ValueGeneratedOnAdd()on theTypeproperty in theHWIdowned entityIn the
HWIdowned entity withinConnectionLog, theTypeproperty is configured withValueGeneratedOnAdd()and a default value of0. SinceTypeis not an identity column and has a default value, usingValueGeneratedOnAdd()may not be appropriate. Consider removingValueGeneratedOnAdd()and managing the default value through the application code or database defaults.Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml (1)
17-27: 🛠️ Refactor suggestion
Use theme-aware colors for the unread indicator
The unread indicator uses hardcoded colors which might not align with the application's theme system.
Consider using theme-aware color resources:
- BackgroundColor="#17c622" - BorderColor="#0f7a15" + BackgroundColor="{DynamicResource PositiveGreenColor}" + BorderColor="{DynamicResource PositiveDarkGreenColor}"📝 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.<PanelContainer Name="UnreadIndicator" MinSize="8 8" MaxSize="8 8" VerticalAlignment="Center" Margin="0 0 6 0"> <PanelContainer.PanelOverride> <graphics:StyleBoxFlat BackgroundColor="{DynamicResource PositiveGreenColor}" BorderColor="{DynamicResource PositiveDarkGreenColor}" /> </PanelContainer.PanelOverride> </PanelContainer>Content.Server/DeltaV/Shuttles/Systems/DockingShuttleSystem.cs (1)
68-77:
⚠️ Potential issuePotential exception when accessing 'data.Grids.First()'
In the
OnStationGridAddedmethod, there is no check to ensure thatdata.Gridscontains elements before callingFirst(). Ifdata.Gridsis empty, this will throw an exception.Apply this diff to add a check before accessing the first element:
+ if (!data.Grids.Any()) + return; // add the source station as a destination comp.Station = station; comp.Destinations.Add(new DockingDestination() { Name = Name(station), Map = Transform(data.Grids.First()).MapID });📝 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.if (_station.GetOwningStation(uid) is not {} station || !TryComp<StationDataComponent>(station, out var data)) return; if (!data.Grids.Any()) return; // add the source station as a destination comp.Station = station; comp.Destinations.Add(new DockingDestination() { Name = Name(station), Map = Transform(data.Grids.First()).MapID });Content.Server/DeltaV/Objectives/Systems/TeachLessonConditionSystem.cs (1)
40-40:
⚠️ Potential issuePotential type mismatch when comparing
targetObjective.TargetandmindIdEnsure that
targetObjective.TargetandmindIdare of the same type for accurate comparison. IftargetObjective.Targetis anEntityUidandmindIdis aMind.MindId, directly comparing them may lead to incorrect behavior.Consider modifying the comparison to ensure type compatibility. For example:
- if (targetObjective.Target != mindId) + if (!_mind.TryGetMind(targetObjective.Target, out var targetMindId, out _) || targetMindId != mindId) continue;This change retrieves the
MindIdoftargetObjective.Targetand compares it tomindId, ensuring the comparison is valid.Committable suggestion skipped: line range outside the PR's diff.
Content.Server/Administration/PlayerLocator.cs (2)
174-175:
⚠️ Potential issueFix invalid array initialization in
ReturnForPlayerRecordThe syntax
[hwid.Hwid]is not valid for initializing anImmutableArray<ImmutableArray<byte>>. Instead, useImmutableArray.Create(hwid.Hwid)to create a new immutable array withhwid.Hwidas its element. Similarly, useImmutableArray<ImmutableArray<byte>>.Emptyfor an empty array.Apply this diff to fix the issue:
- hwid is { Type: HwidType.Modern } ? [hwid.Hwid] : []); + hwid is { Type: HwidType.Modern } ? ImmutableArray.Create(hwid.Hwid) : ImmutableArray<ImmutableArray<byte>>.Empty);📝 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.hwid is { Type: HwidType.Legacy } ? hwid.Hwid : null, hwid is { Type: HwidType.Modern } ? ImmutableArray.Create(hwid.Hwid) : ImmutableArray<ImmutableArray<byte>>.Empty);
148-149:
⚠️ Potential issueCorrect array initialization in
HandleAuthServerResponseThe use of
[]to initialize an emptyImmutableArray<ImmutableArray<byte>>is invalid in C#. Instead, useImmutableArray<ImmutableArray<byte>>.Emptyto represent an empty immutable array.Apply this diff to fix the issue:
- return new LocatedPlayerData(new NetUserId(responseData.UserId), null, null, responseData.UserName, null, []); + return new LocatedPlayerData(new NetUserId(responseData.UserId), null, null, responseData.UserName, null, ImmutableArray<ImmutableArray<byte>>.Empty);📝 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.return new LocatedPlayerData(new NetUserId(responseData.UserId), null, null, responseData.UserName, null, ImmutableArray<ImmutableArray<byte>>.Empty); }Content.Server/Database/ServerDbManager.cs (1)
200-201:
⚠️ Potential issueDatabase schema update required for 'trust' parameter
The addition of the
float trustparameter toAddConnectionLogAsyncsuggests a corresponding update is needed in the database schema to store the trust value. Confirm that the database has been migrated to include this new field and that all data access layers handle it appropriately.Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml.cs (1)
82-83:
⚠️ Potential issueEnsure the Destinations list updates correctly when contents change
Comparing only the counts of the destinations may not detect changes in the list content. This could result in the UI not reflecting the actual destinations. Consider comparing the contents of the lists to ensure accurate updates.
Apply this diff to modify the condition:
- if (Destinations.Count == state.Destinations.Count) + if (DestinationsAreSame(state.Destinations)) return;Add the following method to compare the destination lists:
private bool DestinationsAreSame(List<DockingDestination> newDestinations) { if (Destinations.Count != newDestinations.Count) return false; for (int i = 0; i < Destinations.Count; i++) { if (Destinations[i].Text != newDestinations[i].Name) return false; } return true; }Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.Designer.cs (2)
282-284:
⚠️ Potential issueInconsistent use of 'IsRequired' with nullable properties
The property
LastEditedAtis defined as nullable (DateTime?) but is marked with.IsRequired(). This inconsistency occurs in bothAdminNoteandAdminWatchlistentities:
- Lines 282-284:
b.Property<DateTime?>("LastEditedAt") .IsRequired() .HasColumnType("timestamp with time zone") .HasColumnName("last_edited_at");- Lines 417-419:
b.Property<DateTime?>("LastEditedAt") .IsRequired() .HasColumnType("timestamp with time zone") .HasColumnName("last_edited_at");Recommendation:
To resolve this inconsistency, choose one of the following options:
Option 1: Make the property non-nullable if it should always have a value.
- b.Property<DateTime?>("LastEditedAt") + b.Property<DateTime>("LastEditedAt") .IsRequired() .HasColumnType("timestamp with time zone") .HasColumnName("last_edited_at");Option 2: Remove
.IsRequired()if the property can be null.b.Property<DateTime?>("LastEditedAt") - .IsRequired() .HasColumnType("timestamp with time zone") .HasColumnName("last_edited_at");Ensure that the chosen option aligns with the desired business logic and data model constraints.
Also applies to: 417-419
1717-1720:
⚠️ Potential issueConstraint names may be truncated due to identifier length limits
Some constraint names specified using
HasConstraintNameappear to be truncated, ending with a~character. For example:
- Line 1719:
.HasConstraintName("FK_profile_loadout_group_profile_role_loadout_profile_role_loa~");- Line 1802:
.HasConstraintName("FK_server_ban_server_ban_server_ban_id");PostgreSQL has a maximum identifier length of 63 characters, and longer names are automatically truncated. This truncation can lead to duplicate constraint names or loss of meaningful identifiers.
Recommendation:
Shorten the constraint names to ensure they are unique and within the allowed length. Consider using abbreviated names or removing redundant parts. For example:
- .HasConstraintName("FK_profile_loadout_group_profile_role_loadout_profile_role_loa~"); + .HasConstraintName("FK_profile_loadout_group_role_loadout_id");This change shortens the constraint name while retaining its meaning.
Also applies to: 1799-1803
Content.Server/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (1)
109-110:
⚠️ Potential issueFix off-by-one error in index validation.
The condition
args.Index > docking.Destinations.Countallowsargs.Index == docking.Destinations.Count, which is out of bounds since list indices are zero-based. The correct condition should beargs.Index >= docking.Destinations.Countto prevent an out-of-range exception when accessingdocking.Destinations[args.Index].Apply this diff to fix the issue:
- if (args.Index < 0 || args.Index > docking.Destinations.Count) + if (args.Index < 0 || args.Index >= docking.Destinations.Count)📝 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.if (args.Index < 0 || args.Index >= docking.Destinations.Count) return;Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeSystem.cs (2)
218-219: 🛠️ Refactor suggestion
Validate message content for emptiness
In
HandleSendMessage, the message content is checked for null but not for empty or whitespace-only strings.Update the condition to ensure messages are meaningful:
- if (msg.RecipientNumber == null || msg.Content == null || card.Comp.Number == null) + if (msg.RecipientNumber == null || string.IsNullOrWhiteSpace(msg.Content) || card.Comp.Number == null) return;📝 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.if (msg.RecipientNumber == null || string.IsNullOrWhiteSpace(msg.Content) || card.Comp.Number == null) return;
283-287:
⚠️ Potential issueHandle missing radio channel prototype gracefully
In
AttemptMessageDelivery, the code uses_prototype.Index(sender.Comp.RadioChannel)to retrieve the radio channel prototype. If the prototype is not found, it throws an exception.Add error handling to manage cases where the prototype might not be found:
- var channel = _prototype.Index(sender.Comp.RadioChannel); + if (!_prototype.TryIndex<RadioChannelPrototype>(sender.Comp.RadioChannel, out var channel)) + { + // Log the error and fail delivery + _adminLogger.Add(LogType.Error, + LogImpact.High, + $"Radio channel prototype '{sender.Comp.RadioChannel}' not found for NanoChat message delivery."); + return (true, new List<Entity<NanoChatCardComponent>>()); + }📝 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.if (!_prototype.TryIndex<RadioChannelPrototype>(sender.Comp.RadioChannel, out var channel)) { // Log the error and fail delivery _adminLogger.Add(LogType.Error, LogImpact.High, $"Radio channel prototype '{sender.Comp.RadioChannel}' not found for NanoChat message delivery."); return (true, new List<Entity<NanoChatCardComponent>>()); } var sendAttemptEvent = new RadioSendAttemptEvent(channel, sender); RaiseLocalEvent(ref sendAttemptEvent); if (sendAttemptEvent.Cancelled) return (true, new List<Entity<NanoChatCardComponent>>());Content.Server/DeltaV/Station/Systems/StationPlanetSpawnerSystem.cs (2)
18-24:
⚠️ Potential issueAdd error handling and validation.
The current implementation could be more robust:
- Missing error handling around
LoadPlanetcall- No validation of
Planetproperty- Potential null reference when calling
ToString()Consider this safer implementation:
private void OnMapInit(Entity<StationPlanetSpawnerComponent> ent, ref MapInitEvent args) { if (ent.Comp.GridPath is not {} path) return; + if (string.IsNullOrEmpty(ent.Comp.Planet)) + { + Log.Error($"StationPlanetSpawner at {ent} has no Planet specified"); + return; + } + + try + { + var pathString = path.ToString() ?? throw new InvalidOperationException("GridPath.ToString() returned null"); + ent.Comp.Map = _planet.LoadPlanet(ent.Comp.Planet, pathString); + } + catch (Exception e) + { + Log.Error($"Failed to load planet for station {ent}: {e}"); + } }📝 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.private void OnMapInit(Entity<StationPlanetSpawnerComponent> ent, ref MapInitEvent args) { if (ent.Comp.GridPath is not {} path) return; if (string.IsNullOrEmpty(ent.Comp.Planet)) { Log.Error($"StationPlanetSpawner at {ent} has no Planet specified"); return; } try { var pathString = path.ToString() ?? throw new InvalidOperationException("GridPath.ToString() returned null"); ent.Comp.Map = _planet.LoadPlanet(ent.Comp.Planet, pathString); } catch (Exception e) { Log.Error($"Failed to load planet for station {ent}: {e}"); } }
26-29: 🛠️ Refactor suggestion
Add null check and consider additional cleanup needs.
The shutdown handler should verify the Map exists before queuing deletion and might need to handle additional cleanup.
private void OnShutdown(Entity<StationPlanetSpawnerComponent> ent, ref ComponentShutdown args) { - QueueDel(ent.Comp.Map); + if (ent.Comp.Map is {} map) + { + // Consider if any additional cleanup is needed before map deletion + QueueDel(map); + ent.Comp.Map = default; + } }📝 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.private void OnShutdown(Entity<StationPlanetSpawnerComponent> ent, ref ComponentShutdown args) { if (ent.Comp.Map is {} map) { // Consider if any additional cleanup is needed before map deletion QueueDel(map); ent.Comp.Map = default; } }Content.Server/DeltaV/NanoChat/NanoChatSystem.cs (1)
75-101:
⚠️ Potential issuePotential issue when modifying collection during iteration
Modifying the
component.Messagesdictionary while iterating over it can lead to a runtimeInvalidOperationException. Specifically, reassigning conversations to a different recipient and clearing messages within theforeachloop may cause issues.To fix this, collect the necessary changes during iteration and apply them after the loop.
Consider refactoring the code as follows:
private void ScrambleMessages(NanoChatCardComponent component) { var messagesToAdd = new Dictionary<uint, List<NanoChatMessage>>(); var recipientsToRemove = new List<uint>(); foreach (var (recipientNumber, messages) in component.Messages) { for (var i = 0; i < messages.Count; i++) { // 50% chance to scramble each message if (!_random.Prob(0.5f)) continue; var message = messages[i]; message.Content = ScrambleText(message.Content); messages[i] = message; } // 25% chance to reassign the conversation to a random recipient if (_random.Prob(0.25f) && component.Recipients.Count > 0) { var newRecipient = _random.Pick(component.Recipients.Keys.ToList()); if (newRecipient == recipientNumber) continue; if (!messagesToAdd.ContainsKey(newRecipient)) messagesToAdd[newRecipient] = new List<NanoChatMessage>(); messagesToAdd[newRecipient].AddRange(messages); recipientsToRemove.Add(recipientNumber); } } // Apply the collected changes after iteration foreach (var recipientNumber in recipientsToRemove) { component.Messages.Remove(recipientNumber); } foreach (var (newRecipient, messages) in messagesToAdd) { if (!component.Messages.ContainsKey(newRecipient)) component.Messages[newRecipient] = new List<NanoChatMessage>(); component.Messages[newRecipient].AddRange(messages); } }Content.Server/DeltaV/Station/Components/StationPlanetSpawnerComponent.cs (1)
1-32: 💡 Codebase verification
Based on the gathered information, I can now provide the final response:
FTL whitelist integration needs to be implemented for planet maps
The component mentions FTL whitelist functionality in its documentation, but the implementation is missing. The code shows that:
FTLDestinationComponenthandles whitelist restrictions for destinations- The planet spawner creates maps but doesn't set up FTL destination components
- Other systems (like Cargo) properly set up FTL destinations with whitelists
Required changes:
- Add FTL destination setup in
StationPlanetSpawnerSystem.OnMapInit- Add whitelist configuration field in
StationPlanetSpawnerComponent🔗 Analysis chain
Verify integration with FTL whitelist system
The component's documentation mentions FTL whitelist functionality, but there's no explicit whitelist field. Let's verify how this integrates with the broader FTL system.
Let me gather more information about how the FTL destination and planet systems are connected.
Let me search for one more piece of information to understand how the planet spawner sets up FTL destinations.
Let me check one final piece to see how the planet system handles FTL destinations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FTL whitelist implementation in related systems echo "Searching for FTL whitelist implementation..." rg -A 5 "FTL.*whitelist" # Look for planet-related FTL configurations echo "Searching for planet FTL configurations..." ast-grep --pattern 'class $_ { $$$ whitelist$_($$$) { $$$ } $$$ }'Length of output: 2664
Script:
#!/bin/bash # Search for FTL destination component implementation echo "Searching for FTL destination component..." rg -A 10 "class FTLDestinationComponent" # Search for planet spawner system implementation echo "Searching for StationPlanetSpawnerSystem..." rg -A 15 "class StationPlanetSpawnerSystem" # Search for planet-related FTL setup echo "Searching for planet FTL setup..." rg -A 5 "Map\s*=\s*map" Content.Server/DeltaV/Station/Systems/Length of output: 3146
Script:
#!/bin/bash # Search for the complete implementation of OnMapInit in StationPlanetSpawnerSystem echo "Searching for planet spawner map initialization..." rg -A 20 "OnMapInit.*StationPlanetSpawnerComponent" Content.Server/DeltaV/Station/Systems/ # Search for where FTLDestinationComponent is added to planets echo "Searching for FTLDestinationComponent addition..." rg -A 5 "EnsureComp.*FTLDestinationComponent"Length of output: 2872
Script:
#!/bin/bash # Search for PlanetSystem implementation echo "Searching for PlanetSystem implementation..." rg -A 20 "LoadPlanet.*EntityUid" Content.Server/DeltaV/Planet/ # Search for mining shuttle whitelist setup echo "Searching for mining shuttle whitelist setup..." rg -A 10 "MiningShuttleComponent.*whitelist"Length of output: 315
Content.Server/Database/ServerDbPostgres.cs (2)
199-207:
⚠️ Potential issueAdd null check for
b.HWIdbefore accessing its propertiesIn the
MakeBanLookupQualitySharedmethod, you are accessingb.HWId.Typeandb.HWId.Hwidwithout checking ifb.HWIdis null. This could lead to aNullReferenceExceptionifb.HWIdis null. Consider adding a null check to ensureb.HWIdis not null before accessing its members.Apply this diff to add a null check:
if (hwId != null && hwId.Value.Length > 0) { var newQ = set .Include(p => p.Unban) - .Where(b => b.HWId!.Type == HwidType.Legacy && b.HWId!.Hwid.SequenceEqual(hwId.Value.ToArray())); + .Where(b => b.HWId != null && b.HWId.Type == HwidType.Legacy && b.HWId.Hwid.SequenceEqual(hwId.Value.ToArray())); query = query == null ? newQ : query.Union(newQ); }📝 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.if (hwId != null && hwId.Value.Length > 0) { var newQ = set .Include(p => p.Unban) .Where(b => b.HWId != null && b.HWId.Type == HwidType.Legacy && b.HWId.Hwid.SequenceEqual(hwId.Value.ToArray())); query = query == null ? newQ : query.Union(newQ); }
208-221:
⚠️ Potential issueAdd null check for
b.HWIdwhen handling modern hardware IDsSimilarly, in the loop handling modern hardware IDs,
b.HWIdis accessed without a null check. To prevent potentialNullReferenceException, ensureb.HWIdis not null before accessing its properties.Apply this diff:
if (modernHWIds != null) { foreach (var modernHwid in modernHWIds) { var newQ = set .Include(p => p.Unban) - .Where(b => b.HWId!.Type == HwidType.Modern && b.HWId!.Hwid.SequenceEqual(modernHwid.ToArray())); + .Where(b => b.HWId != null && b.HWId.Type == HwidType.Modern && b.HWId.Hwid.SequenceEqual(modernHwid.ToArray())); query = query == null ? newQ : query.Union(newQ); } }📝 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.if (modernHWIds != null) { foreach (var modernHwid in modernHWIds) { var newQ = set .Include(p => p.Unban) .Where(b => b.HWId != null && b.HWId.Type == HwidType.Modern && b.HWId.Hwid.SequenceEqual(modernHwid.ToArray())); query = query == null ? newQ : query.Union(newQ); } } return query; }Content.Server/Database/ServerDbBase.cs (3)
665-665:
⚠️ Potential issueValidate Nullability in
MakePlayerRecordMethodIn the method
MakePlayerRecord, when passingplayer.LastSeenHWIdto thePlayerRecordconstructor, ensure that the constructor handles null values appropriately. This prevents potential null reference issues ifLastSeenHWIdis null.If necessary, update the
PlayerRecordconstructor to accept nullable HWIDs.
619-619:
⚠️ Potential issueCheck Nullability in Assignment to
record.LastSeenHWIdWhen assigning
hwIdtorecord.LastSeenHWId, ensure thatLastSeenHWIdcan accept null values ifhwIdis null. IfLastSeenHWIdis not nullable, this could lead to aNullReferenceException.Consider making
LastSeenHWIdnullable if it isn't already, or add null checks before assignment.
397-398: 💡 Codebase verification
Let me analyze the findings from the script results. I can see all the callers of both methods and verify if they've been properly updated.
For
GetServerBanAsync, the callers are:
- DetectionManager.cs:
GetServerBanAsync(addr, null, null)- BwoinkSystem.cs:
GetServerBanAsync(null, e.Session.UserId, null, null)- BanManager.cs:
GetServerBanAsync(null, target, null)- PardonCommand.cs and tests: Various calls with
nullfor modernHWIdsFor
GetServerBansAsync, the callers are:
- ConnectionManager.cs:
GetServerBansAsync(addr, userId, hwId, modernHwid, includeUnbanned: false)- Properly updated!- PlayerPanelEui.cs:
GetServerBansAsync(null, _targetPlayer.UserId, null, null)- BanListCommand.cs:
GetServerBansAsync(data.LastAddress, data.UserId, data.LastLegacyHWId, data.LastModernHWIds, false)- Properly updated!- BanListEui.cs:
GetServerBansAsync(null, userId, null, null)Several Callers Need Updates for New
modernHWIdsParameterMultiple callers of these methods are not providing the new
modernHWIdsparameter correctly:
Content.Server/DeltaV/ProxyDetection/DetectionManager.csContent.Server/Administration/Systems/BwoinkSystem.csContent.Server/Administration/Managers/BanManager.csContent.Server/Administration/PlayerPanelEui.csContent.Server/Administration/BanList/BanListEui.cs🔗 Analysis chain
Verify All Callers Updated for New
modernHWIdsParameterThe methods
GetServerBanAsyncandGetServerBansAsyncnow include amodernHWIdsparameter. Please verify that all callers of these methods have been updated to pass the appropriate modern HWIDs to prevent runtime issues.Run the following script to identify all usages of these methods and check if they pass the new parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all calls to GetServerBanAsync and GetServerBansAsync rg 'GetServerBanAsync\(' --context 2 rg 'GetServerBansAsync\(' --context 2Length of output: 19258
Content.Shared.Database/TypedHwid.cs (1)
21-41:
⚠️ Potential issueAdd input validation to TryParse
The method should validate the input string for null/empty and maximum length to prevent potential DoS attacks.
public static bool TryParse(string value, [NotNullWhen(true)] out ImmutableTypedHwid? hwid) { + if (string.IsNullOrEmpty(value) || value.Length > 1024) // Adjust max length as needed + { + hwid = null; + return false; + } var type = HwidType.Legacy; // ... rest of the method📝 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.public static bool TryParse(string value, [NotNullWhen(true)] out ImmutableTypedHwid? hwid) { if (string.IsNullOrEmpty(value) || value.Length > 1024) // Adjust max length as needed { hwid = null; return false; } var type = HwidType.Legacy; if (value.StartsWith("V2-", StringComparison.Ordinal)) { value = value["V2-".Length..]; type = HwidType.Modern; } var array = new byte[GetBase64ByteLength(value)]; if (!Convert.TryFromBase64String(value, array, out _)) { hwid = null; return false; } // ReSharper disable once UseCollectionExpression // Do not use collection expression, C# compiler is weird and it fails sandbox. hwid = new ImmutableTypedHwid(ImmutableArray.Create(array), type); return true; }Content.Server/Administration/Commands/RoleBanListCommand.cs (1)
51-51: 🛠️ Refactor suggestion
Ensure consistent HWID handling with BanListCommand
The HWID handling should match the approach used in BanListCommand for consistency.
- var bans = await _dbManager.GetServerRoleBansAsync(data.LastAddress, data.UserId, data.LastLegacyHWId, data.LastModernHWIds, includeUnbanned); + var bans = await _dbManager.GetServerRoleBansAsync( + data.LastAddress, + data.UserId, + data.LastLegacyHWId ?? ImmutableArray<byte>.Empty, + data.LastModernHWIds ?? ImmutableArray<ImmutableTypedHwid>.Empty, + includeUnbanned);Additionally, consider adding logging for ban queries to help with security auditing.
+ _logger.Debug("Querying role bans for user {userId} with address {address}", + data.UserId, + data.LastAddress); var bans = await _dbManager.GetServerRoleBansAsync(...);Committable suggestion skipped: line range outside the PR's diff.
Content.Server/Administration/Managers/IBanManager.cs (2)
28-28: 💡 Codebase verification
⚠️ Potential issueBreaking change detected in CreateServerBan implementation
The codebase search reveals that
Content.Server/DeltaV/ProxyDetection/DetectionManager.csis still trying to use the removedServerBanExemptFlagsparameter, while other implementations have been updated. This confirms that the removal of the parameter was intentional but not all code has been properly updated.
Content.Server/DeltaV/ProxyDetection/DetectionManager.cs: Still attempts to passServerBanExemptFlags.Noneas a parameter, which will cause compilation errors.🔗 Analysis chain
Verify removal of exemptFlags parameter
The removal of the
exemptFlagsparameter fromCreateServerBancould affect existing ban logic. Please confirm if this was intentional and ensure all implementing classes have been updated accordingly.
Let me search for implementations of CreateServerBan to verify if the removal was properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for implementations and usages of CreateServerBan rg -l "CreateServerBan.*ServerBanExemptFlags" || echo "No references found with exemptFlags"Length of output: 130
Script:
#!/bin/bash # Search for implementations of CreateServerBan ast-grep --pattern 'public void CreateServerBan($$$)' # Also search for any calls to CreateServerBan to see current usage rg "CreateServerBan\(" -A 3Length of output: 2753
28-28: 💡 Codebase verification
Based on the search results, I can now generate the final response:
Hardware ID type migration is in progress but incomplete
The codebase shows mixed usage of hardware ID types:
ImmutableTypedHwidis properly used in the ban system interfaces and most implementations- Legacy
ImmutableArray<byte>still exists in WebhookUpdateRoleBans method of IBanManager/BanManager
Content.Server/Administration/Managers/IBanManager.cs: Update WebhookUpdateRoleBans signature to useImmutableTypedHwid?instead ofImmutableArray<byte>?Content.Server/Administration/Managers/BanManager.cs: Update the implementation accordingly🔗 Analysis chain
Ensure consistent hardware ID type usage
The change from
ImmutableArray<byte>?toImmutableTypedHwid?improves type safety but requires verification of all implementations and callers.Also applies to: 41-41
Let me gather more information about the usage of
ImmutableTypedHwidto understand the migration status.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining ImmutableArray<byte> usages in ban-related code rg "ImmutableArray<byte>" --type csLength of output: 5341
Script:
#!/bin/bash # Search for ImmutableTypedHwid usage rg "ImmutableTypedHwid" --type cs -A 2 # Also check for any CreateServerBan implementations ast-grep --pattern 'CreateServerBan($$$)'Length of output: 10371
Content.Server/Database/ServerBanDef.cs (1)
15-15:
⚠️ Potential issueFix type inconsistency between property and constructor parameter
The property is declared as
ImmutableTypedHwid?but the constructor acceptsTypedHwid?. This inconsistency should be resolved.Apply this change:
- public ImmutableTypedHwid? HWId { get; } - TypedHwid? hwId, + public ImmutableTypedHwid? HWId { get; } + ImmutableTypedHwid? hwId,Also applies to: 32-32
Content.Server/DeltaV/Planet/PlanetSystem.cs (4)
21-21: 🛠️ Refactor suggestion
Change '_setTiles' to a local variable to ensure thread safety
The
_setTileslist is used within theLoadPlanetmethod and is cleared at the beginning of the method. IfLoadPlanetcan be called concurrently from different threads, sharing this mutable state could lead to threading issues or data corruption. Consider moving_setTilesinsideLoadPlanetas a local variable to prevent potential concurrency problems.Apply this diff to remove the class-level declaration:
-private readonly List<(Vector2i, Tile)> _setTiles = new();And declare it locally in
LoadPlanet:// Inside the LoadPlanet method, before its usage var setTiles = new List<(Vector2i, Tile)>();Update the usages of
_setTilestosetTileswithin the method.
34-34:
⚠️ Potential issueEnsure that 'BiomeComponent' exists before accessing it
The call to
Comp<BiomeComponent>(map)assumes that theBiomeComponentis present on themapentity. If it's missing, this will throw an exception. Consider usingTryComp<BiomeComponent>(map, out var biome)and handle the case where the component might not be present.Apply this diff to safely access the component:
-var biome = Comp<BiomeComponent>(map); +if (!TryComp<BiomeComponent>(map, out var biome)) +{ + Log.Error($"BiomeComponent is missing on map entity {map}."); + return map; +}📝 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.if (!TryComp<BiomeComponent>(map, out var biome)) { Log.Error($"BiomeComponent is missing on map entity {map}."); return map; }
69-69:
⚠️ Potential issueEnsure that 'MapGridComponent' exists before accessing it
When accessing
Comp<MapGridComponent>(gridUid), it's important to verify that the component exists to avoid exceptions.Apply this diff:
-var aabb = Comp<MapGridComponent>(gridUid).LocalAABB; +if (!TryComp<MapGridComponent>(gridUid, out var gridComp)) +{ + Log.Error($"MapGridComponent is missing on grid entity {gridUid}."); + continue; +} +var aabb = gridComp.LocalAABB;📝 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.if (!TryComp<MapGridComponent>(gridUid, out var gridComp)) { Log.Error($"MapGridComponent is missing on grid entity {gridUid}."); continue; } var aabb = gridComp.LocalAABB;
57-57:
⚠️ Potential issueEnsure that 'MapComponent' exists before accessing it
Similar to the previous comment, accessing
Comp<MapComponent>(map)assumes the component is present. To prevent potential exceptions, consider usingTryComp<MapComponent>(map, out var mapComp)and handle the null case appropriately.Apply this diff:
-var mapId = Comp<MapComponent>(map).MapId; +if (!TryComp<MapComponent>(map, out var mapComp)) +{ + Log.Error($"MapComponent is missing on map entity {map}."); + Del(map); + return null; +} +var mapId = mapComp.MapId;📝 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.if (!TryComp<MapComponent>(map, out var mapComp)) { Log.Error($"MapComponent is missing on map entity {map}."); Del(map); return null; } var mapId = mapComp.MapId;Content.Server/DeltaV/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.NanoChat.cs (1)
60-81:
⚠️ Potential issueAdd null check for
SoundScanbefore playing soundEnsure that
ent.Comp.SoundScanis not null before calling_audioSystem.PlayEntityto prevent potential null reference exceptions.Apply this diff to add a null check:
+ if (ent.Comp.SoundScan != null) + { _audioSystem.PlayEntity(ent.Comp.SoundScan, args.InteractEvent.User, target, AudioHelpers.WithVariation(0.25f, _random)); + }📝 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.private void ScanNanoChatCard(Entity<LogProbeCartridgeComponent> ent, CartridgeAfterInteractEvent args, EntityUid target, NanoChatCardComponent card) { if (ent.Comp.SoundScan != null) { _audioSystem.PlayEntity(ent.Comp.SoundScan, args.InteractEvent.User, target, AudioHelpers.WithVariation(0.25f, _random)); } _popupSystem.PopupCursor(Loc.GetString("log-probe-scan-nanochat", ("card", target)), args.InteractEvent.User); ent.Comp.PulledAccessLogs.Clear(); ent.Comp.ScannedNanoChatData = new NanoChatData( new Dictionary<uint, NanoChatRecipient>(card.Recipients), new Dictionary<uint, List<NanoChatMessage>>(card.Messages), card.Number, GetNetEntity(target) ); UpdateUiState(ent, args.Loader); }Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiMessageEvent.cs (1)
144-154: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid mutable collections in readonly struct
NanoChatDataThe
NanoChatDatastruct is declared asreadonlybut contains mutable collections (Dictionary). This can lead to unintended side effects since the collections can be modified even though the struct is marked asreadonly. Consider using immutable collections or removing thereadonlymodifier.Apply this diff to remove the
readonlymodifier:- [Serializable, NetSerializable, DataRecord] - public readonly struct NanoChatData( + [Serializable, NetSerializable, DataRecord] + public struct NanoChatData( Dictionary<uint, NanoChatRecipient> recipients, Dictionary<uint, List<NanoChatMessage>> messages, uint? cardNumber, NetEntity card)Alternatively, use immutable collections:
+ using System.Collections.Immutable; - Dictionary<uint, NanoChatRecipient> recipients, - Dictionary<uint, List<NanoChatMessage>> messages, + ImmutableDictionary<uint, NanoChatRecipient> recipients, + ImmutableDictionary<uint, ImmutableList<NanoChatMessage>> messages,Committable suggestion skipped: line range outside the PR's diff.
Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiState.cs (1)
23-24: 💡 Codebase verification
Defensive copying is necessary for collections in constructor
The constructor directly assigns the input collections
recipientsandmessagesto the instance fields. Since these collections are mutable (Dictionary and List) and are exposed as public fields, external code could modify the internal state. Create defensive copies:Recipients = new Dictionary<uint, NanoChatRecipient>(recipients); Messages = new Dictionary<uint, List<NanoChatMessage>>( messages.ToDictionary( kvp => kvp.Key, kvp => new List<NanoChatMessage>(kvp.Value) ) );This ensures:
- Protection against external modifications to the original collections
- Deep copy of nested List to prevent modification of inner collections
🔗 Analysis chain
Verify defensive copying of collections in constructor
The constructor directly assigns the input collections. Consider creating defensive copies to prevent external modifications.
Based on the search results, I can see that both
RecipientsandMessagescollections are modified in various places in the codebase, particularly inSharedNanoChatSystem.cs. For example:
Recipients modifications:
card.Comp.Recipients.Clear()card.Comp.Recipients.Remove(recipientNumber)card.Comp.Recipients.ContainsKey(recipientNumber)Messages modifications:
card.Comp.Messages.Remove(recipientNumber)card.Comp.Messages.Clear()card.Comp.Messages[recipientNumber] = new List<NanoChatMessage>()Let me verify the type of these collections to determine if defensive copying is necessary.
Let me try a different pattern to find the type declarations.
Now I can see that both collections are declared as:
RecipientsisDictionary<uint, NanoChatRecipient>MessagesisDictionary<uint, List<NanoChatMessage>>And they are initialized as new instances in the field declarations. Let me check the constructor implementation to see the full context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if collections are modified elsewhere rg -A 5 "Recipients\." rg -A 5 "Messages\."Length of output: 55590
Script:
#!/bin/bash # Check the type declarations of Recipients and Messages ast-grep --pattern 'class NanoChatUiState { $$$ Recipients $_ Messages $_ $$$ }'Length of output: 90
Script:
#!/bin/bash # Find the file content to check type declarations rg -A 10 "class NanoChatUiState"Length of output: 1244
Script:
#!/bin/bash # Get the constructor implementation rg -A 15 "public NanoChatUiState\(" Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiState.csLength of output: 588
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUi.cs (3)
21-24: 🛠️ Refactor suggestion
Consider implementing IDisposable for proper event cleanup
The event handler subscription should be cleaned up to prevent memory leaks.
+ private Action<NanoChatUiMessageType, uint?, string?, string?>? _messageHandler; + public override void Setup(BoundUserInterface userInterface, EntityUid? fragmentOwner) { _fragment = new NanoChatUiFragment(); - _fragment.OnMessageSent += (type, number, content, job) => + _messageHandler = (type, number, content, job) => { SendNanoChatUiMessage(type, number, content, job, userInterface); }; + _fragment.OnMessageSent += _messageHandler; } + + public override void Dispose() + { + if (_fragment != null && _messageHandler != null) + _fragment.OnMessageSent -= _messageHandler; + base.Dispose(); + }📝 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.private Action<NanoChatUiMessageType, uint?, string?, string?>? _messageHandler; public override void Setup(BoundUserInterface userInterface, EntityUid? fragmentOwner) { _fragment = new NanoChatUiFragment(); _messageHandler = (type, number, content, job) => { SendNanoChatUiMessage(type, number, content, job, userInterface); }; _fragment.OnMessageSent += _messageHandler; } public override void Dispose() { if (_fragment != null && _messageHandler != null) _fragment.OnMessageSent -= _messageHandler; base.Dispose(); }
14-14:
⚠️ Potential issueAdd null check to prevent NullReferenceException
The
!operator assumes _fragment is non-null, but this might not be true if Setup hasn't been called.- return _fragment!; + if (_fragment == null) + throw new InvalidOperationException("NanoChatUi not properly initialized. Ensure Setup is called first."); + return _fragment;📝 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.if (_fragment == null) throw new InvalidOperationException("NanoChatUi not properly initialized. Ensure Setup is called first."); return _fragment;
29-31:
⚠️ Potential issueAdd null check in UpdateState
The null-conditional operator is used on _fragment but not when casting state.
- if (state is NanoChatUiState cast) - _fragment?.UpdateState(cast); + if (state is not NanoChatUiState cast) + return; + + _fragment?.UpdateState(cast);📝 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.if (state is not NanoChatUiState cast) return; _fragment?.UpdateState(cast); }Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml.cs (2)
31-43:
⚠️ Potential issueConsolidate duplicate input validation logic
The NumberInput validation is duplicated - it's subscribed twice to OnTextChanged (lines 28 and 32).
Remove the duplicate subscription and consolidate the logic:
- // Input validation - NumberInput.OnTextChanged += _ => ValidateInputs(); - NameInput.OnTextChanged += _ => ValidateInputs(); - - // Input validation NumberInput.OnTextChanged += args => { if (args.Text.Length > MaxNumberLength) NumberInput.Text = args.Text[..MaxNumberLength]; // Filter to digits only var newText = string.Concat(NumberInput.Text.Where(char.IsDigit)); if (newText != NumberInput.Text) NumberInput.Text = newText; ValidateInputs(); };Committable suggestion skipped: line range outside the PR's diff.
68-78: 🛠️ Refactor suggestion
Add input sanitization in CreateChat method
The CreateChat method should sanitize inputs before processing to prevent potential injection or formatting issues.
private void CreateChat() { if (!uint.TryParse(NumberInput.Text, out var number)) return; - var name = NameInput.Text.Trim(); - var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : JobInput.Text.Trim(); + var name = SanitizeInput(NameInput.Text.Trim()); + var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : SanitizeInput(JobInput.Text.Trim()); OnChatCreated?.Invoke(number, name, job); Close(); } + + private string SanitizeInput(string input) + { + // Remove any potentially problematic characters + return new string(input.Where(c => char.IsLetterOrDigit(c) || char.IsWhiteSpace(c)).ToArray()); + }📝 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.private void CreateChat() { if (!uint.TryParse(NumberInput.Text, out var number)) return; var name = SanitizeInput(NameInput.Text.Trim()); var job = string.IsNullOrWhiteSpace(JobInput.Text) ? null : SanitizeInput(JobInput.Text.Trim()); OnChatCreated?.Invoke(number, name, job); Close(); } private string SanitizeInput(string input) { // Remove any potentially problematic characters return new string(input.Where(c => char.IsLetterOrDigit(c) || char.IsWhiteSpace(c)).ToArray()); }
There was a problem hiding this comment.
Actionable comments posted: 43
♻️ Duplicate comments (1)
Content.Client/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (1)
5-5:⚠️ Potential issueIncomplete implementation of DockingConsoleSystem
The
DockingConsoleSystemclass is declared but lacks implementation. This could lead to missing functionality in the docking console system.Implement the
DockingConsoleSystemclass:public sealed class DockingConsoleSystem : SharedDockingConsoleSystem { // Add necessary method overrides and client-side logic here. }Alternatively, if the class is not needed, consider removing it.
🧹 Nitpick comments (78)
Content.Server.Database/Migrations/Postgres/20241111193608_ConnectionTrust.cs (1)
13-18: Consider using decimal type for trust scoresWhile
floatis a valid choice, consider ifdecimalwould be more appropriate for trust scores to avoid floating-point precision issues. Also, document the expected range and meaning of trust values.migrationBuilder.AddColumn<float>( name: "trust", table: "connection_log", type: "real", nullable: false, defaultValue: 0f);Alternative implementation using decimal:
migrationBuilder.AddColumn<decimal>( name: "trust", table: "connection_log", type: "decimal(5,2)", nullable: false, defaultValue: 0m);Content.Server.Database/Migrations/Sqlite/20241111170107_ModernHwid.cs (1)
7-8: Add documentation for hwid_type values.Consider adding XML documentation to explain:
- The purpose of the hwid_type column
- The meaning of different integer values
- Why this change was needed
Example:
/// <inheritdoc /> + /// <summary> + /// Adds hwid_type columns to support modern hardware identifier types. + /// Values: 0 = Legacy, 1 = Modern, etc. + /// </summary> public partial class ModernHwid : MigrationContent.Server/Administration/PlayerPanelEui.cs (1)
177-180: Consider improving role ban database designThe current implementation works around a database design limitation where role bans are stored individually and deduplicated by ban time. This is fragile and could lead to incorrect counting if bans happen at the exact same time.
Consider these improvements:
- Normalize the role bans table to have a parent-child relationship
- Add a unique identifier for grouped role bans
- Add a dedicated column for tracking related role bans
Would you like me to help draft a database schema improvement proposal?
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml (3)
7-13: Consider adding accessibility attributes for screen readers.While the spacer implementation provides good visual layout, consider adding ARIA attributes to improve accessibility:
<BoxContainer Name="MessageContainer" Orientation="Horizontal" + AutomationProperties.Name="{Loc 'nano-chat-message-container'}" HorizontalExpand="True">Also applies to: 48-54
15-35: Consider improving responsiveness and style definitions.Two suggestions for enhancement:
- The hard-coded MaxWidth might not be ideal for all screen sizes
- The StyleBoxFlat has BorderThickness defined but no border color specified
<BoxContainer Name="MessageBox" Orientation="Vertical" - MaxWidth="320" + MaxWidth="{Loc 'nano-chat-max-message-width'}" HorizontalExpand="True"> <PanelContainer Name="MessagePanel" - MaxWidth="320" + MaxWidth="{Loc 'nano-chat-max-message-width'}" HorizontalExpand="True"> <PanelContainer.PanelOverride> <graphics:StyleBoxFlat ContentMarginLeftOverride="10" ContentMarginRightOverride="10" ContentMarginTopOverride="6" ContentMarginBottomOverride="6" - BorderThickness="1"> + BorderThickness="1" + BorderColor="{Loc 'nano-chat-border-color'}">
38-44: Consider expanding message status indicators.The current implementation only shows delivery failure. Consider adding states for:
- Message sending
- Message delivered
- Message read
This could be implemented by:
- Adding a status container
- Using different icons/labels for each state
- Implementing smooth transitions between states
Would you like me to provide a more detailed implementation suggestion?
Content.Server/DeltaV/Station/Systems/CaptainStateSystem.cs (2)
13-13: Remove unused namespace import.The
Microsoft.CodeAnalysis.CSharp.Syntaxnamespace appears to be unused in this file. Consider removing this import unless it's needed for future changes.-using Microsoft.CodeAnalysis.CSharp.Syntax;
Line range hint
17-166: Consider splitting responsibilities for better maintainability.The
CaptainStateSystemcurrently handles multiple concerns:
- Captain state management
- Station announcements
- Access control modifications
Consider splitting these into separate systems or helper classes to improve maintainability and follow the Single Responsibility Principle. This would make the code easier to test and modify in the future.
Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeComponent.cs (2)
9-13: Consider adding station entity validationWhile the property is well-documented, consider:
- Adding validation to ensure the Station entity is of the correct type
- Expanding documentation to explain when Station might be null
/// <summary> -/// Station entity to keep track of. +/// Station entity to keep track of. Can be null when the cartridge is not associated with any station. +/// Must be an entity with StationComponent when set. /// </summary> [DataField] public EntityUid? Station;
15-19: Enhance Card property documentationThe property would benefit from more detailed documentation about:
- The expected type of card entity
- The relationship between the card and the cartridge
/// <summary> -/// The NanoChat card to keep track of. +/// The NanoChat card entity associated with this cartridge. +/// Null when no card is inserted or connected. /// </summary> [DataField] public EntityUid? Card;Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.cs (1)
1-62: Consider documenting the HWID type system designThis migration introduces a structured way to track different types of hardware IDs across multiple tables. To ensure maintainability:
- Consider adding documentation about:
- The different HWID types and their meanings
- The rationale behind making the columns nullable
- The significance of the default value (0)
- Consider creating a database diagram showing the relationships between these tables
- Consider adding data migration scripts if existing HWID data needs to be converted
Content.Server/Administration/BanList/BanListEui.cs (1)
Line range hint
98-124: Consider refactoring duplicated ban processing logicThe HWID handling and ban processing logic is duplicated between
LoadBansandLoadRoleBans. Consider extracting the common logic into a shared helper method.Example refactor:
+ private async Task<(string, int cidrMask)?, string> GetBanDetails(IHasBanCommon ban) + { + (string, int cidrMask)? ip = ("*Hidden*", 0); + var hwid = "*Hidden*"; + + if (_admins.HasAdminFlag(Player, AdminFlags.Pii)) + { + ip = ban.Address is { } address + ? (address.address.ToString(), address.cidrMask) + : null; + hwid = ban.HWId?.ToString(); + } + + return (ip, hwid); + }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml.cs (2)
8-13: Consider implementing IDisposable for proper event cleanupWhile the event handling setup is good, consider implementing IDisposable to ensure proper cleanup of event handlers when the control is disposed.
Here's a suggested implementation:
[GenerateTypedNameReferences] -public sealed partial class NanoChatEntry : BoxContainer +public sealed partial class NanoChatEntry : BoxContainer, IDisposable { public event Action<uint>? OnPressed; private uint _number; private Action<EventArgs>? _pressHandler; + + public void Dispose() + { + if (_pressHandler != null) + ChatButton.OnPressed -= _pressHandler; + } }
20-38: Consider splitting the method responsibilitiesThe method currently handles both event management and UI updates. Consider splitting these responsibilities for better maintainability.
Here's a suggested refactor:
-public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected) +public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected) +{ + ArgumentNullException.ThrowIfNull(recipient); + UpdateEventHandler(number); + UpdateUI(recipient, isSelected); +} + +private void UpdateEventHandler(uint number) { // Remove old handler if it exists if (_pressHandler != null) ChatButton.OnPressed -= _pressHandler; _number = number; // Create and store new handler _pressHandler = _ => OnPressed?.Invoke(_number); ChatButton.OnPressed += _pressHandler; +} + +private void UpdateUI(NanoChatRecipient recipient, bool isSelected) +{ NameLabel.Text = recipient.Name; JobLabel.Text = recipient.JobTitle ?? ""; JobLabel.Visible = !string.IsNullOrEmpty(recipient.JobTitle); UnreadIndicator.Visible = recipient.HasUnread; ChatButton.ModulateSelfOverride = isSelected ? NanoChatMessageBubble.OwnMessageColor : null; }Content.Client/Lathe/UI/LatheMenu.xaml (1)
136-139: Consider adding accessibility attributesWhile the UI implementation is clean, consider enhancing accessibility by adding:
ToolTipto explain the mining points featureAccessKeyfor keyboard navigation- ARIA attributes for screen readers
<BoxContainer Orientation="Horizontal" Name="MiningPointsContainer" Visible="False"> - <Label Name="MiningPointsLabel" HorizontalExpand="True"/> - <Button Name="MiningPointsClaimButton" Text="{Loc 'lathe-menu-mining-points-claim-button'}"/> + <Label Name="MiningPointsLabel" HorizontalExpand="True" + ToolTip="{Loc 'lathe-menu-mining-points-tooltip'}"/> + <Button Name="MiningPointsClaimButton" + Text="{Loc 'lathe-menu-mining-points-claim-button'}" + ToolTip="{Loc 'lathe-menu-mining-points-claim-tooltip'}" + AccessKey="M"/> </BoxContainer>Content.Client/Lathe/UI/LatheMenu.xaml.cs (1)
96-106: Consider adding error handling for edge casesWhile the implementation is good, consider handling potential edge cases:
- Race condition when player is detached
- Invalid points value
private void UpdateMiningPoints(uint points) { + try { MiningPointsClaimButton.Disabled = points == 0 || _player.LocalSession?.AttachedEntity is not {} player || _miningPoints.TryFindIdCard(player) == null; if (points == _lastMiningPoints) return; _lastMiningPoints = points; MiningPointsLabel.Text = Loc.GetString("lathe-menu-mining-points", ("points", points)); + } catch (Exception e) { + Logger.ErrorS("ui.lathe", e, "Failed to update mining points"); + MiningPointsContainer.Visible = false; + } }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml.cs (3)
9-17: Add XML documentation for color constantsConsider adding XML documentation to explain the purpose and usage of each color constant. This would help maintain the code and ensure consistent usage across the application.
Here's a suggested documentation format:
+ /// <summary> + /// Background color for messages sent by the current user. + /// </summary> public static readonly Color OwnMessageColor = Color.FromHex("#173717d9");
42-61: Refactor message layout logic for better maintainabilityThe message layout logic could be simplified by extracting it into a separate method and using a more declarative approach.
+ private void UpdateMessageLayout(bool isOwnMessage) + { + MessageContainer.RemoveAllChildren(); + + var components = isOwnMessage + ? new Control[] { FlexSpace, MessageBox, RightSpacer } + : new Control[] { LeftSpacer, MessageBox, FlexSpace }; + + foreach (var component in components) + { + MessageContainer.AddChild(component); + } + }Then update the SetMessage method:
- if (isOwnMessage) - { - MessageContainer.AddChild(FlexSpace); - MessageContainer.AddChild(MessageBox); - MessageContainer.AddChild(RightSpacer); - } - else - { - MessageContainer.AddChild(LeftSpacer); - MessageContainer.AddChild(MessageBox); - MessageContainer.AddChild(FlexSpace); - } + UpdateMessageLayout(isOwnMessage);
10-61: Consider separating styling from layout logicThe current implementation mixes styling and layout concerns. Consider splitting this into two components:
- A styling service/helper for managing message appearances
- A pure UI component for layout
This separation would improve maintainability and make the code more testable.
Content.Server.Database/Migrations/Sqlite/20241111193602_ConnectionTrust.Designer.cs (1)
545-595: Consider adding an index on the 'Trust' column for improved query performanceIf the
Trustproperty will be used frequently in queries for filtering or sorting, adding an index can significantly improve query performance:b.HasIndex("Trust") .HasDatabaseName("IX_connection_log_trust");Content.Server/DeltaV/Planet/PlanetSystem.cs (2)
37-38: Check for null or empty BiomeMarkerLayersBefore iterating over
planet.BiomeMarkerLayers, it's good practice to check if the collection is not null or empty to prevent possibleNullReferenceException.Add a null or empty check:
if (planet.BiomeMarkerLayers != null && planet.BiomeMarkerLayers.Count > 0) { foreach (var layer in planet.BiomeMarkerLayers) { _biome.AddMarkerLayer(map, biome, layer); } }
56-75: Handle potential failures in loading planet gridsIn the
LoadPlanetmethod, if_mapLoader.TryLoadfails, the map is deleted, but there is no notification or exception handling beyond logging an error. Consider providing more feedback or retry mechanisms.Consider enhancing error handling:
if (!_mapLoader.TryLoad(mapId, path, out var grids)) { Log.Error($"Failed to load planet grid {path} for planet {id}!"); // Potentially notify the user or trigger a fallback mechanism. Del(map); return null; }Content.Server/DeltaV/Shuttles/Systems/DockingShuttleSystem.cs (1)
60-62: Combine conditions to reduce nestingTo improve readability, combine the conditions in
OnStationGridAddedto reduce nesting levels.Refactor the code:
if (!TryComp<DockingShuttleComponent>(uid, out var comp) || comp.Station != null) return;Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml.cs (2)
70-78: Localize FTL state strings and colors consistentlyEnsure that all FTL states have corresponding localized strings and colors. Missing entries might lead to incorrect UI display.
Verify that all FTL states are handled:
MapFTLState.Text = Loc.GetString($"shuttle-console-ftl-state-{_state}"); _ftlStyle.BackgroundColor = _state switch { FTLState.Available => Color.FromHex("#80C71F"), FTLState.Starting => Color.FromHex("#169C9C"), FTLState.Travelling => Color.FromHex("#8932B8"), FTLState.Arriving => Color.FromHex("#F9801D"), FTLState.Cooldown => Color.FromHex("#B02E26"), _ => Color.Gray };
94-95: Simplify null check using C# 8.0 null-coalescing assignmentYou can simplify the null check in the
UpdateButtonmethod by using the null-coalescing assignment operator.Refactor the code:
FTLButton.Disabled = _selected == null || _state != FTLState.Available || !HasAccess();Note: The code is already optimal; consider this as a confirmation.
Content.Server/DeltaV/Station/Components/StationPlanetSpawnerComponent.cs (1)
24-25: Consider validating ResPath at compile timeThe
GridPathis marked as required but can still be null. Consider using a compile-time validation attribute.- [DataField(required: true)] + [DataField(required: true)] + [ValidatePrototypeId<EntityPrototype>] public ResPath? GridPath;Content.Client/DeltaV/Shuttles/UI/DockingConsoleBoundUserInterface.cs (1)
18-21: Consider handling window creation failureThe window creation could potentially fail, and the event subscriptions should be more defensive.
- _window = new DockingConsoleWindow(Owner); - _window.OnFTL += index => SendMessage(new DockingConsoleFTLMessage(index)); - _window.OnClose += Close; - _window.OpenCentered(); + try + { + _window = new DockingConsoleWindow(Owner); + if (_window == null) + { + Log.Error("Failed to create DockingConsoleWindow"); + return; + } + _window.OnFTL += index => SendMessage(new DockingConsoleFTLMessage(index)); + _window.OnClose += Close; + _window.OpenCentered(); + } + catch (Exception e) + { + Log.Error($"Error creating DockingConsoleWindow: {e}"); + }Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml (2)
1-4: Consider implementing responsive designThe fixed window size of 500x500 might not be optimal for different screen resolutions or window scaling settings.
Consider adding:
-SetSize="500 500"> +MinSize="400 400" +HorizontalExpand="True" +VerticalExpand="True">
5-7: Enhance list view with loading and empty statesThe destinations list lacks visual feedback for loading and empty states, which could impact user experience.
Consider adding:
-<ItemList Name="Destinations"/> <!-- Populated from comp.Destinations --> +<BoxContainer Orientation="Vertical"> + <ItemList Name="Destinations" VerticalExpand="True"/> + <Label Name="EmptyLabel" + Text="{Loc 'docking-console-no-destinations'}" + Visible="False" + HorizontalAlignment="Center"/> +</BoxContainer>Content.Server/DeltaV/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.NanoChat.cs (2)
21-22: Add braces to single-line conditional statementsFor better readability and to prevent potential errors in the future, it's recommended to enclose the bodies of
ifstatements and loops in braces, even if they contain only a single line.Apply this diff to add braces:
if (probe.ScannedNanoChatData == null || GetEntity(probe.ScannedNanoChatData.Value.Card) != args.CardUid) - continue; +{ + continue; +} if (!TryComp<NanoChatCardComponent>(args.CardUid, out var card)) - continue; +{ + continue; +} if (cartridge.LoaderUid != null) - UpdateUiState((uid, probe), cartridge.LoaderUid.Value); +{ + UpdateUiState((uid, probe), cartridge.LoaderUid.Value); +}Similar changes should be made in the
OnMessageReceivedmethod.Also applies to: 24-25, 33-34, 43-44, 46-47, 55-56
16-36: Refactor duplicate code inOnRecipientUpdatedandOnMessageReceivedThe methods
OnRecipientUpdatedandOnMessageReceivedshare similar logic. Consider refactoring them to reduce code duplication, which will enhance maintainability and readability.You could extract the shared logic into a private helper method.
Also applies to: 38-58
Content.Server/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (2)
133-133: Enhance the clarity and professionalism of the commentThe comment "prevent picking vgroid and stuff" is informal. Consider rephrasing for clarity and professionalism.
Suggested change:
-// prevent picking vgroid and stuff +// Exclude non-station grids from selection
157-157: Correct typo and improve comment clarityThere's a typo in the comment ("cant" should be "can't"). Additionally, rephrasing can enhance understanding.
Suggested change:
-// no error if it cant find one since it would fail every test as shuttle.grid_fill is false in dev +// No error if it can't find one since it would fail tests when shuttle.grid_fill is false during developmentContent.Server/Database/BanMatcher.cs (1)
56-75: Ensure all HWID types are handled appropriatelyThe
switchstatement handlesHwidType.LegacyandHwidType.Modern, but it lacks adefaultcase. Consider adding adefaultcase to handle unexpectedHwidTypevalues and possibly log a warning for unhandled types. This enhances the robustness of the code against future changes.Apply this diff to add a default case:
switch (ban.HWId?.Type) { case HwidType.Legacy: // existing logic... break; case HwidType.Modern: // existing logic... break; + default: + // Log or handle unexpected HwidType + _log.Warning($"Unhandled HwidType: {ban.HWId?.Type}"); + break; }Content.Server/Database/ServerDbSqlite.cs (1)
259-279: Optimize hardware ID matching inRoleBanMatchesIn the
RoleBanMatchesmethod, the logic for matching modern HWIDs iterates overmodernHWIdsand compares each toban.HWId.Hwid. This can be optimized by convertingmodernHWIdsto aHashSetfor faster lookups, especially if the list is large.Apply this diff to optimize the matching:
if (ban.HWId?.Type == HwidType.Modern && modernHWIds != null) { + var modernHwIdSet = new HashSet<ImmutableArray<byte>>(modernHWIds); + if (modernHwIdSet.Contains(ban.HWId.Hwid)) + return true; - - foreach (var modernHWId in modernHWIds) - { - if (modernHWId.AsSpan().SequenceEqual(ban.HWId.Hwid)) - return true; - } }This change improves the performance of the HWID matching logic.
Content.Server/Administration/Managers/BanManager.cs (2)
Line range hint
333-372: Refactor entity configurations to reduce duplicationConsider creating a helper method to configure HWID owned entity properties to avoid code duplication across multiple entities.
Apply this suggestion to simplify the model configurations:
// Create a helper method private void ConfigureTypedHwid<TEntity>(ModelBuilder modelBuilder, Expression<Func<TEntity, TypedHwid?>> navigationExpression) where TEntity : class { modelBuilder.Entity<TEntity>() .OwnsOne(navigationExpression, hwid => { hwid.Property(p => p.Hwid).HasColumnName("hwid"); hwid.Property(p => p.Type).HasDefaultValue(HwidType.Legacy); }); } // Then use the helper for each entity ConfigureTypedHwid<Player>(modelBuilder, p => p.LastSeenHWId); ConfigureTypedHwid<ServerBan>(modelBuilder, p => p.HWId); ConfigureTypedHwid<ServerRoleBan>(modelBuilder, p => p.HWId); ConfigureTypedHwid<ConnectionLog>(modelBuilder, p => p.HWId);
Line range hint
1256-1286: Add XML documentation forTypedHwidclassIncluding XML documentation comments will enhance code readability and maintainability.
For example:
/// <summary> /// Represents a hardware ID value together with its <see cref="HwidType"/>. /// </summary> /// <seealso cref="ImmutableTypedHwid"/> [Owned] public sealed class TypedHwid { // existing code... }Content.Server/Database/ServerDbManager.cs (3)
72-79: Update XML documentation to accurately reflect parametersEnsure that the XML documentation comments for
GetServerBanAsynccorrectly match the method's parameters, including the newmodernHWIdsparameter.Update the
<param>tags accordingly.
595-595: Update method documentation forUpdatePlayerRecordAsyncThe parameter type for
hwIdhas changed toImmutableTypedHwid?; ensure that the XML documentation is updated to reflect this change.Update the
<param>tag forhwIdin the method's XML comments.
617-623: Addtrustparameter to XML documentationThe
AddConnectionLogAsyncmethod now includes a newfloat trustparameter. Update the XML documentation to include this parameter.Add
<param name="trust">to the method's documentation.Content.Server/Database/ServerDbBase.cs (3)
391-398: Update XML documentation forGetServerBanAsyncEnsure that the XML documentation for
GetServerBanAsyncaccurately reflects the method parameters, including themodernHWIdsparameter.Update the
<param>tags accordingly.
506-512: Update XML documentation forGetServerBansAsyncUpdate the method's XML documentation to include the new
modernHWIdsparameter.Update the
<param>tags to reflect the updated parameters.
674-678: Addtrustparameter to XML documentationThe method
AddConnectionLogAsyncnow includes afloat trustparameter. Ensure that the method's XML documentation includes this parameter.Add
<param name="trust">to the method's documentation.Content.Server/Connection/UserDataExt.cs (1)
15-17: Typographical error in method remarksIn the remarks section, the sentence "This method returns a nullable method." should be corrected to "This method returns a nullable HWID." to accurately reflect the return value of the method.
Apply this diff to fix the typographical error:
- /// only want a single one. This method returns a nullable method. + /// only want a single one. This method returns a nullable HWID.Content.Shared.Database/TypedHwid.cs (2)
15-19: Consider adding format validation in ToString()The method assumes the byte array is valid. Consider adding validation to ensure the byte array is not empty or within expected size limits.
public override string ToString() { + if (Hwid.IsEmpty) + throw new InvalidOperationException("HWID cannot be empty"); + if (Hwid.Length > 1024) // Add appropriate max size + throw new InvalidOperationException("HWID exceeds maximum allowed size"); var b64 = Convert.ToBase64String(Hwid.AsSpan()); return Type == HwidType.Modern ? $"V2-{b64}" : b64; }
43-47: Document the base64 padding calculation logicThe comment suggests complexity in the .NET base64 handling. Consider adding a more detailed explanation of the calculation logic.
private static int GetBase64ByteLength(string value) { - // Why is .NET like this man wtf. + // Base64 encoding: Every 3 bytes becomes 4 characters with padding ('=') + // This calculation accounts for the padding characters at the end of the base64 string + // by subtracting them from the total length calculation return 3 * (value.Length / 4) - value.TakeLast(2).Count(c => c == '='); }Content.Server/Administration/Managers/IBanManager.cs (1)
28-28: Update parameter documentation for HWIDThe
hwidparameter documentation is incomplete (shows only "H"). Please update it to reflect the newImmutableTypedHwidtype and its purpose.Content.Server/Administration/BanPanelEui.cs (2)
60-60: Document the purpose of the 'erase' parameterThe newly added
eraseparameter's purpose is not immediately clear. Please add documentation explaining its functionality and impact on the ban process.
Line range hint
60-141: Consider refactoring the BanPlayer methodThe method is quite long (80+ lines) and handles multiple responsibilities including:
- Input validation
- IP address parsing
- Player lookup
- Ban creation
- Webhook updates
Consider breaking it down into smaller, focused methods for better maintainability.
Example structure:
- private async void BanPlayer(string? target, string? ipAddressString, bool useLastIp, ImmutableTypedHwid? hwid, bool useLastHwid, uint minutes, NoteSeverity severity, string reason, IReadOnlyCollection<string>? roles, bool erase) + private async void BanPlayer(...) + { + if (!ValidatePermissions()) + return; + + if (!ValidateInputs(target, ipAddressString, hwid)) + return; + + var addressRange = await ParseIpAddress(ipAddressString); + var (targetUid, targetHWid) = await ResolveTargetDetails(target, useLastIp, useLastHwid, hwid); + + await CreateBans(targetUid, target, addressRange, targetHWid, minutes, severity, reason, roles); + }Content.Server/DeltaV/Objectives/Systems/TeachLessonConditionSystem.cs (1)
24-25: Consider subscribing to the event by reference as per the TODO commentSubscribing to the
MobStateChangedEventby reference can improve performance by avoiding unnecessary copies of the event data.Would you like assistance in updating the event subscription and handler to use by-reference parameters?
Content.Server/DeltaV/NanoChat/NanoChatSystem.cs (2)
48-52: Offer assistance to resolve the TODO regarding conflicting popupsThe TODO comment indicates that the popups are conflicting with those from
IdCardSystem. Consider adjusting the timing or using a different notification method to prevent overlap.Would you like assistance in implementing a solution to prevent the popups from conflicting?
33-71: Ensure proper null-checking and error handlingIn the
OnMicrowavedmethod, consider adding error handling for potential null references or unexpected states, such as whenargs.Microwaveis null or missing components.Content.Client/CartridgeLoader/Cartridges/LogProbeUiFragment.xaml.cs (1)
75-118: Optimize message display loops to improve performanceConsider combining the incoming and outgoing message loops or refactoring the code to reduce redundancy, which can enhance readability and performance.
Content.Server/Access/Systems/AgentIDCardSystem.cs (1)
37-43: Add Error Handling for Setting NanoChat NumberIn the
OnNumberChangedmethod, while setting the NanoChat number, there is no error handling in case_nanoChat.SetNumberfails or throws an exception. This could lead to unhandled exceptions and inconsistent states.Consider adding error handling to manage potential failures when setting the NanoChat number. For example:
private void OnNumberChanged(Entity<AgentIDCardComponent> ent, ref AgentIDCardNumberChangedMessage args) { if (!TryComp<NanoChatCardComponent>(ent, out var comp)) return; + try + { _nanoChat.SetNumber((ent, comp), args.Number); Dirty(ent, comp); + } + catch (Exception ex) + { + Logger.ErrorS("agentid", $"Failed to set NanoChat number: {ex.Message}"); + // Optionally notify the user or take corrective action + } }Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml.cs (2)
58-76: Validate Message Length and Input More RobustlyThe current validation in the
OnTextChangedevent handler checks for message length and emptiness. However, it could be enhanced to provide better user feedback and handle edge cases.Consider updating the validation logic to trim whitespace and provide clearer feedback:
MessageInput.OnTextChanged += args => { var text = args.Text.Trim(); var length = text.Length; var isValid = length > 0 && length <= MaxMessageLength && (_currentChat != null || _pendingChat != null); SendButton.Disabled = !isValid; // Show character count when over limit CharacterCount.Visible = length > MaxMessageLength; if (length > MaxMessageLength) { CharacterCount.Text = Loc.GetString("nano-chat-message-too-long", ("current", length), ("max", MaxMessageLength)); CharacterCount.StyleClasses.Add("LabelDanger"); } + else + { + CharacterCount.Visible = false; + } };
124-132: Optimize Chat Selection State ManagementIn the
SelectChatmethod, the use of both_currentChatand_pendingChatcan be confusing and may lead to state management complexities.Consider simplifying the state management by using a single variable or clearly documenting the purpose of each.
Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeSystem.cs (2)
76-105: Missing Error Handling for Invalid UI MessagesThe
OnMessagemethod switches onmsg.Typebut does not handle unexpected or invalid message types, which could lead to unhandled exceptions.Consider adding a default case to handle unexpected message types:
switch (msg.Type) { // Existing cases... case NanoChatUiMessageType.SendMessage: HandleSendMessage(ent, card, msg); break; + default: + Logger.WarningS("nanochat", $"Received unknown message type: {msg.Type}"); + break; }
351-362: Optimize Server Availability CheckThe
HasActiveServermethod iterates over all entities withTelecomServerComponent, which may be inefficient.Consider caching active servers or optimizing the query to reduce performance overhead, especially if this check is frequent.
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatLogEntry.xaml.cs (1)
10-16: Ensure Proper Localization for Displayed TextThe
NanoChatLogEntryconstructor sets the text labels directly with the provided parameters. This may lead to issues with localization and formatting.Consider using localization functions for the labels:
NumberLabel.Text = Loc.GetString("{0}", number); TimeLabel.Text = Loc.GetString("{0}", time); MessageLabel.Text = Loc.GetString("{0}", message);Content.Client/CartridgeLoader/Cartridges/LogProbeUi.cs (1)
26-26: LGTM! Consider adding a comment explaining the state object usage.The change to pass the entire state object is appropriate as it allows handling both PulledLogs and NanoChatData. Consider adding a brief comment explaining what state properties are used by the fragment.
- _fragment?.UpdateState(logProbeUiState); // DeltaV - just take the state + // Pass full state to handle both PulledLogs and NanoChatData + _fragment?.UpdateState(logProbeUiState);Content.Client/Access/UI/AgentIDCardWindow.xaml (1)
9-12: Consider enhancing the NanoChat number input UX.While the implementation is functional, consider these UX improvements:
- Add a tooltip explaining the expected format
- Add validation feedback for invalid inputs
- Consider using a specialized number input control
<!-- DeltaV - Add NanoChat number field --> - <Label Name="CurrentNumber" Text="{Loc 'agent-id-card-current-number'}" /> - <LineEdit Name="NumberLineEdit" PlaceHolder="#0000" /> + <Label Name="CurrentNumber" Text="{Loc 'agent-id-card-current-number'}" + ToolTip="{Loc 'agent-id-card-number-tooltip'}" /> + <LineEdit Name="NumberLineEdit" + PlaceHolder="#0000" + ToolTip="{Loc 'agent-id-card-number-format-hint'}" + MaxLength="4" /> <!-- DeltaV end -->Content.Shared/CartridgeLoader/Cartridges/LogProbeUiState.cs (1)
19-22: Consider adding validation for NanoChatData.While the implementation is correct, consider adding validation logic if there are any constraints on the NanoChatData that should be enforced.
Example validation:
public LogProbeUiState(List<PulledAccessLog> pulledLogs, NanoChatData? nanoChatData = null) { + ArgumentNullException.ThrowIfNull(pulledLogs); PulledLogs = pulledLogs; NanoChatData = nanoChatData; }Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiState.cs (1)
8-13: Consider encapsulation and documentation improvements.While the properties are correctly marked as readonly, consider these improvements:
- Add XML documentation for the class and properties
- Consider returning IReadOnlyDictionary to prevent external modifications
Example:
+/// <summary> +/// Represents the state of the NanoChat user interface. +/// </summary> public sealed class NanoChatUiState : BoundUserInterfaceState { - public readonly Dictionary<uint, NanoChatRecipient> Recipients = new(); - public readonly Dictionary<uint, List<NanoChatMessage>> Messages = new(); + private readonly Dictionary<uint, NanoChatRecipient> _recipients = new(); + private readonly Dictionary<uint, List<NanoChatMessage>> _messages = new(); + + /// <summary> + /// Gets the dictionary of chat recipients. + /// </summary> + public IReadOnlyDictionary<uint, NanoChatRecipient> Recipients => _recipients; + + /// <summary> + /// Gets the dictionary of chat messages. + /// </summary> + public IReadOnlyDictionary<uint, List<NanoChatMessage>> Messages => _messages;Content.Client/CartridgeLoader/Cartridges/LogProbeUiFragment.xaml (1)
31-33: Consider responsive width adjustmentsThe fixed widths (26, 100, 390) might not accommodate different screen sizes or text lengths well. Consider using relative widths or implementing a responsive layout system.
Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml (2)
11-13: Consider adding input validation feedbackThe LineEdit controls should provide visual feedback for invalid input. Consider adding:
- Real-time validation indicators
- Error message display areas
- Input length limits
Also applies to: 21-23, 31-33
1-52: Consider adding keyboard navigation supportTo improve accessibility, consider:
- Adding tab order attributes
- Implementing keyboard shortcuts for Create/Cancel actions
- Adding tooltips for input fields
Content.Shared/Access/SharedAgentIDCardSystem.cs (1)
42-51: Consider adding validation for the card numberWhile the implementation is clean, consider adding validation or constraints for the card number to ensure it follows any business rules (e.g., range limits, format requirements).
public AgentIDCardNumberChangedMessage(uint number) { + if (number < 1000 || number > 9999) // Assuming 4-digit numbers based on NewChatPopup + throw new ArgumentOutOfRangeException(nameof(number), "Card number must be a 4-digit number"); Number = number; }Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml.cs (2)
11-12: Consider making MaxNumberLength configurableThe hardcoded value with the comment "i hardcoded it to be 4 so suffer" suggests this might need to be more flexible in the future. Consider making this configurable or at least documenting why it must be 4 digits.
- private const int MaxNumberLength = 4; // i hardcoded it to be 4 so suffer + // Card numbers must be 4 digits to match the system-wide card number format + private const int MaxNumberLength = 4;
14-14: Add XML documentation for public membersThe public event
OnChatCreatedshould have XML documentation explaining its purpose and parameters.+ /// <summary> + /// Event triggered when a new chat is created with validated inputs. + /// </summary> + /// <param name="number">The 4-digit chat number</param> + /// <param name="name">The chat name</param> + /// <param name="job">Optional job title</param> public event Action<uint, string, string?>? OnChatCreated;Content.Client/Access/UI/AgentIDCardWindow.xaml.cs (1)
49-61: Consider adding user feedback for invalid inputWhile the digit-only filtering is well implemented, consider providing visual feedback when non-digit characters are filtered out.
NumberLineEdit.OnTextChanged += args => { if (args.Text.Length > MaxNumberLength) { NumberLineEdit.Text = args.Text[..MaxNumberLength]; + // Show tooltip or status message + NumberLineEdit.SetTooltip(Loc.GetString("agent-id-max-length-exceeded")); } // Filter to digits only var newText = string.Concat(args.Text.Where(char.IsDigit)); if (newText != args.Text) + { NumberLineEdit.Text = newText; + // Show tooltip or status message + NumberLineEdit.SetTooltip(Loc.GetString("agent-id-digits-only")); + } };Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml (3)
21-26: Consider adding alt text for accessibilityThe app icon should have an alt text for screen readers.
<TextureRect Name="AppIcon" TexturePath="/Textures/Interface/Nano/ntlogo.svg.png" Stretch="KeepAspectCentered" VerticalAlignment="Center" MinSize="32 32" + ToolTip="{Loc nano-chat-app-icon-tooltip}" Margin="8 0 0 0" />
83-103: Consider adding loading state for chat listThe chat list panel should handle loading states to improve user experience.
<PanelContainer StyleClasses="AngleRect" VerticalExpand="True" MaxWidth="150"> <ScrollContainer VerticalExpand="True" MinWidth="145" HorizontalExpand="True" HScrollEnabled="False"> <BoxContainer Name="ChatList" Orientation="Vertical" VerticalExpand="True" HorizontalExpand="True" Margin="4"> + <!-- Add loading indicator --> + <Control Name="LoadingIndicator" + Visible="False"> + <Label Text="{Loc nano-chat-loading}" + HorizontalAlignment="Center" + StyleClasses="LabelSubText" /> + </Control> <!-- Chat entries will be added here dynamically --> <Label Name="NoChatsLabel" Text="{Loc nano-chat-no-chats}" HorizontalAlignment="Center" VerticalAlignment="Center" StyleClasses="LabelSubText" /> </BoxContainer> </ScrollContainer> </PanelContainer>
149-162: Add character limit indicatorConsider showing the maximum character limit in the message input placeholder.
<LineEdit Name="MessageInput" - PlaceHolder="{Loc nano-chat-message-placeholder}" + PlaceHolder="{Loc 'nano-chat-message-placeholder-with-limit' maxChars={MaxMessageLength}}" HorizontalExpand="True" StyleClasses="OpenRight" />Content.Server.Database/Model.cs (5)
334-343: Consolidate Owned Entity Configuration forLastSeenHWIdThe configuration for the owned entity
LastSeenHWIdin thePlayerentity is split into separate calls. For better readability and maintainability, consider consolidating them into a single configuration block.You can refactor the code as follows:
modelBuilder.Entity<Player>() .OwnsOne(p => p.LastSeenHWId, hwid => { hwid.Property(h => h.Hwid) .HasColumnName("last_seen_hwid"); hwid.Property(h => h.Type) .HasDefaultValue(HwidType.Legacy); });
344-353: Consolidate Owned Entity Configuration forHWIdinServerBanSimilar to the
Playerentity, the configuration for the owned entityHWIdin theServerBanentity can be consolidated into a single block to enhance clarity.Refactored code:
modelBuilder.Entity<ServerBan>() .OwnsOne(b => b.HWId, hwid => { hwid.Property(h => h.Hwid) .HasColumnName("hwid"); hwid.Property(h => h.Type) .HasDefaultValue(HwidType.Legacy); });
354-363: Consolidate Owned Entity Configuration forHWIdinServerRoleBanThe configuration for the owned entity
HWIdin theServerRoleBanentity is also split. Consolidating these configurations improves code maintainability.Refactored code:
modelBuilder.Entity<ServerRoleBan>() .OwnsOne(b => b.HWId, hwid => { hwid.Property(h => h.Hwid) .HasColumnName("hwid"); hwid.Property(h => h.Type) .HasDefaultValue(HwidType.Legacy); });
364-372: Consolidate Owned Entity Configuration forHWIdinConnectionLogConsolidating the
HWIdconfiguration in theConnectionLogentity enhances readability and aligns with best practices.Refactored code:
modelBuilder.Entity<ConnectionLog>() .OwnsOne(c => c.HWId, hwid => { hwid.Property(h => h.Hwid) .HasColumnName("hwid"); hwid.Property(h => h.Type) .HasDefaultValue(HwidType.Legacy); });
333-372: Reduce Code Duplication by Creating a Helper MethodThe configurations for the
HWIdowned entity are duplicated across several entities. Consider creating a helper method to apply this configuration, reducing code duplication and enhancing maintainability.Example implementation:
private void ConfigureHWId<TEntity>(OwnedNavigationBuilder<TEntity, TypedHwid?> navigationBuilder, string columnName) where TEntity : class { navigationBuilder.Property(h => h.Hwid) .HasColumnName(columnName); navigationBuilder.Property(h => h.Type) .HasDefaultValue(HwidType.Legacy); } // Usage: ConfigureHWId( modelBuilder.Entity<Player>() .OwnsOne(p => p.LastSeenHWId), "last_seen_hwid"); ConfigureHWId( modelBuilder.Entity<ServerBan>() .OwnsOne(b => b.HWId), "hwid"); ConfigureHWId( modelBuilder.Entity<ServerRoleBan>() .OwnsOne(b => b.HWId), "hwid"); ConfigureHWId( modelBuilder.Entity<ConnectionLog>() .OwnsOne(c => c.HWId), "hwid");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (38)
Resources/Textures/DeltaV/Interface/VerbIcons/bell.svgis excluded by!**/*.svgResources/Textures/DeltaV/Interface/VerbIcons/bell.svg.pngis excluded by!**/*.pngResources/Textures/DeltaV/Interface/VerbIcons/bell_muted.pngis excluded by!**/*.pngResources/Textures/DeltaV/Misc/program_icons.rsi/nanochat.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Devices/cartridge.rsi/cart-chat.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Devices/pda.rsi/pda-hygienetechnician.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/icon-on.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/icon.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/inhand-left.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/inhand-right.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/on-equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/black.rsi/on-equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/icon-on.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/icon.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/inhand-left.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/inhand-right.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/on-equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/blue.rsi/on-equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/icon-on.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/icon.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/inhand-left.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/inhand-right.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/on-equipped-BACKPACK.pngis excluded by!**/*.pngResources/Textures/DeltaV/Objects/Tanks/Jetpacks/security.rsi/on-equipped-SUITSTORAGE.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/dangernana.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/litdakka.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/posterearthnanotrasen.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/posterroidsyndicate.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/posterworknanotrasen.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/posterworksyndicate.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/Posters/grayposters.rsi/work.pngis excluded by!**/*.pngResources/Textures/DeltaV/Structures/Wallmounts/intercom.rsi/random_intercom.pngis excluded by!**/*.png
📒 Files selected for processing (82)
Content.Client/Access/UI/AgentIDCardBoundUserInterface.cs(2 hunks)Content.Client/Access/UI/AgentIDCardWindow.xaml(1 hunks)Content.Client/Access/UI/AgentIDCardWindow.xaml.cs(2 hunks)Content.Client/Administration/UI/BanPanel/BanPanel.xaml.cs(3 hunks)Content.Client/CartridgeLoader/Cartridges/LogProbeUi.cs(1 hunks)Content.Client/CartridgeLoader/Cartridges/LogProbeUiFragment.xaml(1 hunks)Content.Client/CartridgeLoader/Cartridges/LogProbeUiFragment.xaml.cs(2 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml.cs(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatLogEntry.xaml(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatLogEntry.xaml.cs(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml.cs(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUi.cs(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml.cs(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml(1 hunks)Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml.cs(1 hunks)Content.Client/DeltaV/NanoChat/NanoChatSystem.cs(1 hunks)Content.Client/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs(1 hunks)Content.Client/DeltaV/Shuttles/UI/DockingConsoleBoundUserInterface.cs(1 hunks)Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml(1 hunks)Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml.cs(1 hunks)Content.Client/Lathe/UI/LatheBoundUserInterface.cs(2 hunks)Content.Client/Lathe/UI/LatheMenu.xaml(1 hunks)Content.Client/Lathe/UI/LatheMenu.xaml.cs(5 hunks)Content.IntegrationTests/Tests/Commands/PardonCommand.cs(7 hunks)Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.Designer.cs(1 hunks)Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.cs(1 hunks)Content.Server.Database/Migrations/Postgres/20241111193608_ConnectionTrust.Designer.cs(1 hunks)Content.Server.Database/Migrations/Postgres/20241111193608_ConnectionTrust.cs(1 hunks)Content.Server.Database/Migrations/Postgres/PostgresServerDbContextModelSnapshot.cs(6 hunks)Content.Server.Database/Migrations/Sqlite/20241111170107_ModernHwid.Designer.cs(1 hunks)Content.Server.Database/Migrations/Sqlite/20241111170107_ModernHwid.cs(1 hunks)Content.Server.Database/Migrations/Sqlite/20241111193602_ConnectionTrust.Designer.cs(1 hunks)Content.Server.Database/Migrations/Sqlite/20241111193602_ConnectionTrust.cs(1 hunks)Content.Server.Database/Migrations/Sqlite/SqliteServerDbContextModelSnapshot.cs(6 hunks)Content.Server.Database/Model.cs(9 hunks)Content.Server/Access/Systems/AgentIDCardSystem.cs(5 hunks)Content.Server/Administration/BanList/BanListEui.cs(4 hunks)Content.Server/Administration/BanPanelEui.cs(3 hunks)Content.Server/Administration/Commands/BanListCommand.cs(1 hunks)Content.Server/Administration/Commands/RoleBanListCommand.cs(1 hunks)Content.Server/Administration/Managers/BanManager.cs(6 hunks)Content.Server/Administration/Managers/IBanManager.cs(2 hunks)Content.Server/Administration/PlayerLocator.cs(3 hunks)Content.Server/Administration/PlayerPanelEui.cs(1 hunks)Content.Server/Administration/Systems/AdminSystem.cs(1 hunks)Content.Server/Administration/Systems/BwoinkSystem.cs(1 hunks)Content.Server/CartridgeLoader/Cartridges/LogProbeCartridgeComponent.cs(2 hunks)Content.Server/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.cs(5 hunks)Content.Server/Connection/ConnectionManager.cs(3 hunks)Content.Server/Connection/UserDataExt.cs(1 hunks)Content.Server/Database/BanMatcher.cs(3 hunks)Content.Server/Database/DatabaseRecords.cs(1 hunks)Content.Server/Database/ServerBanDef.cs(2 hunks)Content.Server/Database/ServerDbBase.cs(7 hunks)Content.Server/Database/ServerDbManager.cs(8 hunks)Content.Server/Database/ServerDbPostgres.cs(16 hunks)Content.Server/Database/ServerDbSqlite.cs(13 hunks)Content.Server/Database/ServerRoleBanDef.cs(2 hunks)Content.Server/DeltaV/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.NanoChat.cs(1 hunks)Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeComponent.cs(1 hunks)Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeSystem.cs(1 hunks)Content.Server/DeltaV/NanoChat/NanoChatSystem.cs(1 hunks)Content.Server/DeltaV/Objectives/Components/TeachLessonConditionComponent.cs(1 hunks)Content.Server/DeltaV/Objectives/Systems/TeachLessonConditionSystem.cs(1 hunks)Content.Server/DeltaV/Planet/PlanetSystem.cs(1 hunks)Content.Server/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs(1 hunks)Content.Server/DeltaV/Shuttles/Systems/DockingShuttleSystem.cs(1 hunks)Content.Server/DeltaV/Station/Components/StationPlanetSpawnerComponent.cs(1 hunks)Content.Server/DeltaV/Station/Components/StationSurfaceComponent.cs(0 hunks)Content.Server/DeltaV/Station/Systems/CaptainStateSystem.cs(5 hunks)Content.Server/DeltaV/Station/Systems/StationPlanetSpawnerSystem.cs(1 hunks)Content.Server/DeltaV/Station/Systems/StationSurfaceSystem.cs(0 hunks)Content.Shared.Database/TypedHwid.cs(1 hunks)Content.Shared/Access/SharedAgentIDCardSystem.cs(1 hunks)Content.Shared/Administration/BanPanelEuiState.cs(2 hunks)Content.Shared/CartridgeLoader/Cartridges/LogProbeUiState.cs(2 hunks)Content.Shared/DeltaV/CCVars/DCCVars.cs(2 hunks)Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiMessageEvent.cs(1 hunks)Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiState.cs(1 hunks)
⛔ Files not processed due to max files limit (32)
- Content.Shared/DeltaV/NanoChat/NanoChatCardComponent.cs
- Content.Shared/DeltaV/NanoChat/SharedNanoChatSystem.cs
- Content.Shared/DeltaV/Planet/PlanetPrototype.cs
- Content.Shared/DeltaV/Salvage/Components/MiningPointsComponent.cs
- Content.Shared/DeltaV/Salvage/Components/MiningPointsLatheComponent.cs
- Content.Shared/DeltaV/Salvage/MiningPointsUI.cs
- Content.Shared/DeltaV/Salvage/Systems/MiningPointsSystem.cs
- Content.Shared/DeltaV/Shuttles/Components/DockingConsoleComponent.cs
- Content.Shared/DeltaV/Shuttles/Components/DockingShuttleComponent.cs
- Content.Shared/DeltaV/Shuttles/Components/MiningShuttleComponent.cs
- Content.Shared/DeltaV/Shuttles/DockingConsoleUI.cs
- Content.Shared/DeltaV/Shuttles/Systems/SharedDockingConsoleSystem.cs
- Content.Shared/DeltaV/Shuttles/Systems/SharedDockingShuttleSystem.cs
- Content.Shared/Nyanotrasen/ReverseEngineering/ReverseEngineeringMachineComponent.cs
- Content.Shared/Research/Prototypes/LatheRecipePrototype.cs
- Resources/Changelog/DeltaVAdmin.yml
- Resources/Changelog/DeltaVChangelog.yml
- Resources/ConfigPresets/Build/debug.toml
- Resources/ConfigPresets/DeltaV/deltav.toml
- Resources/Credits/GitHub.txt
- Resources/Locale/en-US/deltav/access/components/agent-id-card-component.ftl
- Resources/Locale/en-US/deltav/cartridge-loader/cartridges.ftl
- Resources/Locale/en-US/deltav/ghost/roles/ghost-role-component.ftl
- Resources/Locale/en-US/deltav/job/job-names.ftl
- Resources/Locale/en-US/deltav/lathe/ui/lathe-menu.ftl
- Resources/Locale/en-US/deltav/nanochat/components/nanochat-card-component.ftl
- Resources/Locale/en-US/deltav/preferences/loadout-groups.ftl
- Resources/Locale/en-US/deltav/shuttles/docking-console.ftl
- Resources/Locale/en-US/deltav/station-laws/laws.ftl
- Resources/Maps/DeltaV/centcomm.yml
- Resources/Maps/Nonstations/glacier_surface.yml
- Resources/Maps/Ruins/DeltaV/djstation.yml
💤 Files with no reviewable changes (2)
- Content.Server/DeltaV/Station/Components/StationSurfaceComponent.cs
- Content.Server/DeltaV/Station/Systems/StationSurfaceSystem.cs
✅ Files skipped from review due to trivial changes (5)
- Content.Client/DeltaV/NanoChat/NanoChatSystem.cs
- Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatLogEntry.xaml
- Content.Server/DeltaV/Objectives/Components/TeachLessonConditionComponent.cs
- Content.Server.Database/Migrations/Sqlite/20241111170107_ModernHwid.Designer.cs
- Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.Designer.cs
🔇 Additional comments (105)
Content.Server.Database/Migrations/Postgres/20241111193608_ConnectionTrust.cs (1)
1-29: Verify the integration with connection logging system
The migration looks correct, but let's verify the related changes in the connection logging system to ensure proper integration.
#!/bin/bash
# Description: Verify the integration of the trust column with the connection logging system
# Check for connection log entity definition
echo "Checking for ConnectionLog entity definition..."
rg -A 10 "class ConnectionLog"
# Check for trust property usage in connection handling
echo "Checking for trust property usage..."
rg -A 5 "connection.*trust|trust.*connection"
# Check for any existing trust calculation logic
echo "Checking for trust calculation implementation..."
rg -A 10 "calculate.*trust|trust.*calculate"Consider documenting:
- How trust values are calculated and updated
- The impact of trust scores on connection handling
- Any cleanup/maintenance procedures for trust scores
Content.Server.Database/Migrations/Sqlite/20241111170107_ModernHwid.cs (2)
13-39: LGTM! Migration structure looks solid.
The migration is well-structured with good backward compatibility:
- Nullable columns with default values ensure existing records remain valid
- Column naming follows the established convention
- Column type (INTEGER) is appropriate for type/enum values
13-39: Verify impact on existing HWID functionality.
The migration adds new type columns alongside existing HWID columns. Let's verify the related code changes.
#!/bin/bash
# Description: Check how existing HWID columns are used and verify the migration's impact
# Find existing HWID column definitions
echo "Checking existing HWID columns in migrations:"
rg -l "hwid" --glob "*.cs" | grep -i "migration"
# Find HWID usage in models and services
echo -e "\nChecking HWID usage in models:"
ast-grep --pattern 'class $_ {
$$$
hwid
$$$
}'
echo -e "\nChecking HWID usage in services:"
rg -A 5 "hwid" --glob "**/*Service.cs"Content.Server/Administration/PlayerPanelEui.cs (1)
176-176: Verify the impact of null parameters in ban retrieval
The call to GetServerBansAsync with all null parameters might miss important ban data. While the comment suggests this is intentional to exclude IP/HWID bans, please verify that this doesn't create security gaps in ban enforcement.
Let's check the usage patterns of this method across the codebase:
#!/bin/bash
# Search for other calls to GetServerBansAsync to compare parameter usage
ast-grep --pattern 'GetServerBansAsync($$$)'
# Search for related ban check implementations
rg -A 5 'GetServerBansAsync|CheckBanAsync'Content.Server.Database/Migrations/Sqlite/20241111193602_ConnectionTrust.cs (4)
1-9: LGTM: Migration structure follows best practices
The migration class is properly structured with appropriate namespace and inheritance.
22-27: LGTM: Clean rollback implementation
The Down migration correctly removes the column, providing a clean rollback path.
11-19: Consider a more descriptive column name
While the migration is technically correct, the column name 'trust' could be more descriptive (e.g., 'connection_trust_level' or 'user_trust_score') to better indicate its purpose.
Let's verify the consistency between SQLite and PostgreSQL migrations:
#!/bin/bash
# Description: Verify migration consistency across database providers
# Check if the corresponding PostgreSQL migration exists and compare column definitions
# Find the PostgreSQL migration file
pg_file=$(fd -e cs ConnectionTrust Content.Server.Database/Migrations/Postgres)
if [ -n "$pg_file" ]; then
echo "Checking PostgreSQL migration..."
# Verify column type and constraints
rg -A 3 'AddColumn.*trust' "$pg_file"
fi1-29: Verify integration with connection logging system
Since this migration affects the connection logging system, we should verify the changes in related components.
#!/bin/bash
# Description: Verify integration with connection logging system
# Check for connection log related files and their modifications
echo "Checking connection log usage..."
# Look for files handling connection logging
rg -l "connection_log" --type cs
echo "Checking trust value usage..."
# Look for places where the trust value is set or used
rg "trust.*=.*\d+f?" --type cs
echo "Checking for potential null handling..."
# Look for places where connection log data is accessed
ast-grep --pattern 'connection_log.$$$trust$$$'Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml (2)
1-5: LGTM! Root component setup is well-structured.
The namespace declarations and root element configuration are properly defined with appropriate horizontal expansion setting.
1-55: Verify theme system integration.
Ensure that this component properly integrates with the game's theme system for consistent styling across different game modes (e.g., light/dark themes).
#!/bin/bash
# Search for theme-related files and verify integration points
rg -l "StyleBoxFlat|StyleClasses" "Resources/" "Content.Client/"
# Check for other chat-related UI components that might need similar styling
fd -e xaml . | xargs rg "nano-chat|StyleBoxFlat"Content.Server/DeltaV/Station/Systems/CaptainStateSystem.cs (3)
34-35: LGTM! Good improvement in time handling.
Converting the delay values to TimeSpan during initialization makes the time units explicit and prevents potential confusion. This is a good practice for handling time-based configurations.
44-45: LGTM! Good performance optimization.
The early return check prevents unnecessary processing when the ACO delay hasn't been reached yet. The code comment effectively explains the reasoning, and caching the round duration reduces redundant calls.
78-78: Verify message consistency across announcement types.
The announcement formatting has been standardized, but we should verify that all announcement types maintain consistent messaging patterns.
#!/bin/bash
# Search for all DispatchStationAnnouncement calls to verify message consistency
rg --type cs "DispatchStationAnnouncement.*Loc\.GetString" -A 2Also applies to: 96-97
Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeComponent.cs (3)
1-4: LGTM! Clean and minimal imports
The imports are specific to the component's needs and the namespace follows the project structure.
6-8: LGTM! Good security practice with access restriction
The component is properly registered and follows best practices:
- Access restriction to NanoChatCartridgeSystem enhances security
- Sealed class prevents unwanted inheritance
- Partial class allows for code generation
21-25: Verify the Common radio channel prototype
The RadioChannel property looks good, but let's verify the "Common" channel exists in the prototypes.
#!/bin/bash
# Search for the Common radio channel prototype definition
rg -A 5 '"Common".*RadioChannelPrototype'Consider enhancing the documentation to explain:
- The significance of the "Common" default channel
- Whether this can be changed after initialization
/// <summary>
-/// The <see cref="RadioChannelPrototype" /> required to send or receive messages.
+/// The <see cref="RadioChannelPrototype" /> required to send or receive messages.
+/// Defaults to "Common" channel for general communication.
+/// This channel determines the scope and accessibility of messages sent through this cartridge.
/// </summary>Content.Server.Database/Migrations/Postgres/20241111170112_ModernHwid.cs (3)
1-9: LGTM: File structure follows EF Core migration standards
The migration class is properly structured with appropriate namespace and inheritance.
11-40: Verify the column definitions and data implications
A few concerns about the column definitions:
- The columns are marked as nullable but have a default value of 0, which seems contradictory. Consider either:
- Making the columns non-nullable since they have a default value, or
- Removing the default value if null values are meaningful
- The consistent use of integer type across tables suggests an enum type. Consider adding documentation about the valid values.
Let's check for any enum definitions or documentation about these HWID types:
#!/bin/bash
# Search for HWID type definitions or documentation
rg -i "hwid.*type|hardware.*id.*type" --type cs
# Look for existing enum definitions that might be related
ast-grep --pattern 'enum $_ {
$$$
}'43-60: LGTM: Down migration correctly reverses all changes
The Down method properly removes all columns in the reverse order. However, we should verify the impact on existing data.
Let's check for existing data that might be affected:
#!/bin/bash
# Look for repositories or services that might be using these tables
rg -A 5 "server_role_ban|server_ban|connection_log|player" --type cs
# Search for existing migrations that might have related changes
fd --extension cs . | grep -i "migration" | xargs rg -l "hwid|hardware"Content.Server/Administration/BanList/BanListEui.cs (3)
57-57: Verify the impact of passing null parameters to GetServerBansAsync
The method now passes all null parameters to GetServerBansAsync. While this aligns with the interface changes, we should verify that this doesn't inadvertently exclude any ban records.
#!/bin/bash
# Search for other usages of GetServerBansAsync to understand the parameter patterns
ast-grep --pattern 'GetServerBansAsync($$$)'77-77: Verify HWID string representation format
The HWID conversion has been simplified to use ToString(). Given the type change to ImmutableTypedHwid?, please verify that this produces the expected string format for display.
#!/bin/bash
# Search for ImmutableTypedHwid usage and string conversion patterns
ast-grep --pattern 'ImmutableTypedHwid'
rg -A 2 'ToString\(\).*HWId'98-98: Maintain consistency with GetServerBansAsync implementation
The changes to GetServerRoleBansAsync mirror those in GetServerBansAsync. Ensure that both methods handle the null parameters consistently.
#!/bin/bash
# Compare implementations of both methods
ast-grep --pattern 'GetServerRoleBansAsync($$$)'Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml.cs (2)
1-6: LGTM: Clean and appropriate imports
The imports are well-organized and contain only the necessary dependencies for a XAML-based UI component.
15-18: LGTM: Clean constructor implementation
The constructor follows the standard pattern for XAML-based controls.
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatEntry.xaml (3)
1-4: LGTM! Root container configuration is appropriate.
The namespace declarations and expansion settings are correctly configured for a SS14 UI component.
5-9: Consider dynamic sizing for better localization support.
The fixed MaxSize="137 64" might cause text truncation issues with longer names or when the text is translated into other languages.
#!/bin/bash
# Check for localization files that might affect this component
fd -e ftl -e resx . | xargs grep -l "NanoChat"Consider using dynamic sizing or at least verifying that the maximum size accommodates the longest possible text in all supported languages.
34-44: Review text clipping behavior.
The JobLabel has ClipText="False" which might cause text to overflow its container and potentially break the layout with very long job titles.
#!/bin/bash
# Search for job title definitions to assess potential length
rg -g '!*.{dll,cache}' -l "job.*title" Consider enabling text clipping or implementing text truncation with ellipsis for consistency:
-ClipText="False"
+ClipText="True"
+TextAlign="Center"Content.Client/Lathe/UI/LatheBoundUserInterface.cs (1)
35-36: LGTM: Clean event handler implementation
The event handler follows the established pattern and properly delegates the mining points claim action to the server.
Content.Client/Lathe/UI/LatheMenu.xaml.cs (2)
24-31: LGTM: Clean dependency injection setup
Dependencies are properly organized and follow the dependency injection pattern.
83-89: LGTM: Proper UI initialization
The mining points UI initialization is well-structured with appropriate component checks and event handling.
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatMessageBubble.xaml.cs (3)
1-8: LGTM: Imports and namespace are well-organized
The using directives are appropriate for the UI component's requirements, and the namespace follows the project structure.
18-21: LGTM: Constructor follows standard XAML pattern
The constructor correctly uses RobustXamlLoader to load the XAML layout.
12-16: Verify color contrast ratios for accessibility
The colors use alpha channels which could affect readability. Ensure that the text remains readable against these semi-transparent backgrounds under all conditions.
#!/bin/bash
# Search for any existing color contrast or accessibility tests
rg -l "contrast|accessibility|wcag" --type csContent.Server.Database/Migrations/Sqlite/20241111193602_ConnectionTrust.Designer.cs (1)
571-574: Verify the usage of the 'Trust' property throughout the codebase
Ensure that all places where the ConnectionLog entity is created or modified are updated to handle the new Trust property appropriately. This includes setting the Trust value where necessary to avoid NullReferenceException or default value issues.
Run the following script to identify usages of ConnectionLog creation that may need updates:
#!/bin/bash
# Description: Find code that creates or updates 'ConnectionLog' without setting 'Trust'
# Search for 'new ConnectionLog' that does not set 'Trust'
rg --type cs 'new ConnectionLog\([^\)]*\)' -A 5 | rg -L 'Trust'This script searches for instances where ConnectionLog is instantiated but Trust might not be set, helping to identify potential areas that need code adjustments.
Content.Server/DeltaV/Planet/PlanetSystem.cs (2)
40-41: Verify that added components are valid and applicable
When adding components from planet.AddedComponents, ensure that the components are valid and appropriate for the map entity to prevent runtime issues.
Run the following script to check the validity of components:
#!/bin/bash
# Description: Verify that all components in AddedComponents are valid and can be added to the map entity.
# Test: List all components to be added and check their applicability.
echo "Components to be added:"
cat planet_prototype_file.yaml | grep "AddedComponents" -A 10
# Verify that each component is applicable to the map entity.43-43: Ensure atmospheric settings are correctly applied
After setting the map atmosphere, verify that the atmospheric parameters are correctly initialized and that the map reacts as expected to atmospheric conditions.
Run the following script to confirm atmospheric settings:
#!/bin/bash
# Description: Confirm that atmospheric settings are correctly applied to the map.
# Test: Retrieve atmospheric components of the map entity.
ast-grep --pattern $'comp AtmosphericComponent { $$$ }' --jsonContent.Server/DeltaV/Shuttles/Systems/DockingShuttleSystem.cs (1)
34-46: Optimize entity query by adding necessary filters
In the OnMapInit method, the EntityQueryEnumerator may benefit from filtering entities upfront to reduce unnecessary iterations, especially in large maps.
[performance]
Consider adding filters to the query:
var query = EntityQueryEnumerator<FTLDestinationComponent, MapComponent>(filter: dest => dest.Enabled);Content.Client/DeltaV/Shuttles/UI/DockingConsoleWindow.xaml (2)
8-10: LGTM!
Good use of localization and proper centering of the FTL label.
1-17: Verify accessibility features
While the basic UI structure is sound, we should verify that the interface meets accessibility standards.
#!/bin/bash
# Check for other UI components that might provide accessibility patterns
rg -l "StyleClasses=\"LabelSecondary\"" --type xaml
rg -l "ToolTip=" --type xamlContent.Server/DeltaV/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.NanoChat.cs (1)
71-71: Verify the necessity of clearing PulledAccessLogs
In ScanNanoChatCard, the method clears ent.Comp.PulledAccessLogs. Please verify whether clearing access logs when scanning a NanoChat card is intended behavior, as it might lead to loss of previously collected access logs.
Content.Server/DeltaV/Shuttles/Systems/DockingConsoleSystem.cs (1)
121-121: Verify the logging syntax in interpolated strings
The logging statement uses format specifiers like :user, :shuttle, and :grid within an interpolated string, which may not produce the intended output. Ensure that the logging framework supports this syntax or adjust it accordingly.
Content.Server/Connection/ConnectionManager.cs (5)
121-123: Variables hwid and trust initialized correctly
The hardware ID and trust level are appropriately retrieved from e.UserData, enhancing connection handling.
128-128: Connection log updated with hwid and trust
Including hwid and trust in _db.AddConnectionLogAsync improves the detail and usefulness of connection logs for denied connections.
140-140: Successful connection logging enhanced
The addition of hwid and trust to the connection log for successful connections ensures consistent logging of user data.
145-145: Player record updated with new hardware ID
The player's hardware ID is correctly updated in the database using _db.UpdatePlayerRecordAsync, maintaining accurate player information.
203-205: Server ban checks include modern hardware IDs
Retrieving modernHwid from e.UserData.ModernHWIds and including it in _db.GetServerBansAsync ensures comprehensive ban checks against both legacy and modern hardware IDs.
Content.Server/Database/BanMatcher.cs (1)
96-104: LGTM
The addition of the ModernHWIds field in the PlayerInfo struct correctly accommodates modern hardware IDs. This enhances the system's capability to handle multiple HWID formats.
Content.Server/Administration/PlayerLocator.cs (2)
151-163: LGTM
The ReturnForSession method accurately constructs the LocatedPlayerData instance using session data. This ensures that all relevant player information is correctly captured.
165-175: LGTM
The ReturnForPlayerRecord method correctly initializes the LocatedPlayerData with player record details, properly handling both legacy and modern HWIDs.
Content.Server/Database/ServerDbPostgres.cs (1)
Line range hint 501-517: LGTM
The AddConnectionLogAsync method correctly updates the HWID parameter to ImmutableTypedHwid? and assigns it appropriately. This change ensures consistent handling of hardware IDs in connection logs.
Content.Server/Database/ServerDbSqlite.cs (1)
Line range hint 442-458: LGTM
The AddConnectionLogAsync method correctly updates the HWID parameter to ImmutableTypedHwid? and assigns it appropriately. This ensures that connection logs accurately reflect the updated HWID structures.
Content.Server/Administration/Managers/BanManager.cs (3)
91-92: Ensure modernHwids is properly handled
Verify that netChannel.UserData.ModernHWIds is correctly initialized and handled to prevent potential null reference exceptions when passed to _db.GetServerRoleBansAsync.
Run the following script to check if ModernHWIds is being initialized properly:
#!/bin/bash
# Description: Search for assignments to `ModernHWIds` to ensure it is initialized.
rg 'ModernHWIds' -A 5Line range hint 159-193: Verify updates to method signature of CreateServerBan
Ensure that all calls to CreateServerBan are updated to match the new method signature with ImmutableTypedHwid? hwid.
Run the following script to find all usages of CreateServerBan:
#!/bin/bash
# Description: Find all calls to `CreateServerBan` to check for correct parameters.
rg 'CreateServerBan\(' -A 3 --type cs237-237: Ensure ModernHWIds are correctly utilized in BanMatchesPlayer
Verify that the inclusion of ModernHWIds in PlayerInfo is properly handled throughout the ban matching logic.
Run the following script to examine usages of BanMatchesPlayer and ModernHWIds:
#!/bin/bash
# Description: Find usages of `BanMatchesPlayer` to check handling of `ModernHWIds`.
rg 'BanMatchesPlayer' -A 5 --type csContent.Server/Database/ServerDbManager.cs (2)
490-494: Verify all overloads and usages of GetServerBanAsync are updated
Ensure that all overloads and calls to GetServerBanAsync throughout the codebase have been updated to include the new modernHWIds parameter.
Run the following script to find all usages:
#!/bin/bash
# Description: Find all calls to `GetServerBanAsync` to check for updated parameters.
rg 'GetServerBanAsync\(' -A 3 --type cs501-505: Verify all overloads and usages of GetServerBansAsync are updated
Ensure that all calls to GetServerBansAsync have been updated to include the new modernHWIds parameter.
Run the following script to find all usages:
#!/bin/bash
# Description: Find all calls to `GetServerBansAsync` to check for updated parameters.
rg 'GetServerBansAsync\(' -A 3 --type csContent.Server/Database/ServerDbBase.cs (1)
Line range hint 602-619: Verify that UpdatePlayerRecord handles ImmutableTypedHwid? hwId correctly
Ensure that the change in parameter type for hwId to ImmutableTypedHwid? in UpdatePlayerRecord is handled correctly throughout the method.
Run the following script to check usages:
#!/bin/bash
# Description: Find usages of `UpdatePlayerRecord` to verify proper handling of `ImmutableTypedHwid? hwId`.
rg 'UpdatePlayerRecord\(' -A 3 --type csContent.Server.Database/Migrations/Sqlite/SqliteServerDbContextModelSnapshot.cs (6)
529-541: Reintroduction of the 'Blacklist' entity is correctly configured
The 'Blacklist' entity has been re-added with 'UserId' as the primary key and mapped to the 'blacklist' table. The configuration aligns with the existing database schema.
568-571: Consider adding default values or constraints for the 'Trust' property
The addition of the 'Trust' property of type float to the ConnectionLog entity is acceptable. However, consider specifying default values or validation constraints to ensure data integrity and prevent invalid data entries.
1550-1577: 'HWId' owned entity in 'ConnectionLog' is properly configured
The owned entity HWId of type TypedHwid within ConnectionLog is correctly implemented with appropriate property mappings and foreign key constraints.
1593-1622: 'LastSeenHWId' owned entity in 'Player' entity is correctly defined
The Player entity now includes an owned entity LastSeenHWId of type TypedHwid, which appears to be properly configured and integrated.
1718-1747: 'HWId' owned entity in 'ServerBan' is correctly added
The inclusion of the HWId owned entity in ServerBan enhances hardware identifier tracking for bans and is correctly defined with the necessary property mappings.
1795-1824: 'HWId' owned entity in 'ServerRoleBan' is appropriately implemented
The addition of the HWId owned entity to ServerRoleBan is properly configured, ensuring consistent hardware ID handling in role bans.
Content.Server.Database/Migrations/Postgres/PostgresServerDbContextModelSnapshot.cs (6)
560-572: Readdition of the 'Blacklist' entity is correctly configured
The 'Blacklist' entity has been reintroduced with 'UserId' as the primary key, mapped to the 'blacklist' table. The configuration is consistent with the database schema.
601-604: Consider specifying defaults or constraints for the 'Trust' property
Adding the Trust property of type float to the ConnectionLog entity is acceptable. To maintain data integrity, consider adding default values or validation constraints.
1627-1654: 'HWId' owned entity in 'ConnectionLog' is correctly defined
The owned entity HWId within ConnectionLog is properly configured, enhancing hardware identifier management in connection logs.
1670-1699: 'LastSeenHWId' owned entity in 'Player' entity is properly implemented
The addition of the LastSeenHWId owned entity to the Player entity is correctly set up, allowing for better tracking of the last seen hardware identifiers.
1795-1824: 'HWId' owned entity in 'ServerBan' is correctly added
The HWId owned entity in ServerBan is appropriately configured, enhancing the management of hardware IDs in server bans.
1872-1901: 'HWId' owned entity in 'ServerRoleBan' is properly configured
The addition of the HWId owned entity to ServerRoleBan ensures consistent handling of hardware IDs in role bans and is correctly implemented.
Content.Server/Connection/UserDataExt.cs (1)
18-23: 'GetModernHwid' method implementation is correct
The GetModernHwid extension method correctly returns the first modern HWID or null if none are present, aligning with the intended functionality.
Content.Shared/Administration/BanPanelEuiState.cs (2)
28-28: Update of 'Hwid' property to 'ImmutableTypedHwid?'
Changing the Hwid property type to ImmutableTypedHwid? reflects the new structure for hardware identifiers. Ensure that all usages and serialization logic are updated accordingly to handle the new type.
37-37: Constructor parameter 'hwid' updated to 'ImmutableTypedHwid?'
The constructor now accepts hwid as ImmutableTypedHwid?. Verify that all calls to this constructor are updated to match the new parameter type and that serialization processes accommodate this change.
Content.Server/Database/ServerRoleBanDef.cs (1)
12-12: LGTM: Type update aligns with the new HWID system
The change from ImmutableArray<byte>? to ImmutableTypedHwid? is consistent with the broader HWID system update. The validation logic remains intact, ensuring at least one identifier (user, address, or hardware ID) is provided.
Let's verify the usage of this updated type across the codebase:
#!/bin/bash
# Search for any remaining ImmutableArray<byte> usages related to HWID
rg "ImmutableArray<byte>" -A 3 -B 3
# Search for places where ServerRoleBanDef is constructed
ast-grep --pattern 'new ServerRoleBanDef($$$)'Also applies to: 28-28
Content.Shared.Database/TypedHwid.cs (2)
6-14: LGTM: Well-documented immutable HWID type
The class design is clean with proper immutability and documentation.
21-41: LGTM: Robust parsing implementation with proper error handling
The TryParse implementation properly handles:
- Version prefix detection
- Base64 validation
- Memory allocation
Content.Server/Administration/Commands/BanListCommand.cs (1)
41-41: Verify data population for new HWID parameters
The update to use LastLegacyHWId and LastModernHWIds is consistent with the new HWID system. However, we should verify that these values are properly populated by the PlayerLocator.
Let's verify the data population:
#!/bin/bash
# Search for PlayerLocator implementation to verify HWID data population
ast-grep --pattern 'class PlayerLocator {
$$$
}'
# Search for other calls to GetServerBansAsync to ensure consistent usage
rg "GetServerBansAsync" -A 2 -B 2Content.Server/Administration/Commands/RoleBanListCommand.cs (1)
51-51: LGTM: Updated HWID parameter handling
The change correctly implements the new HWID parameter structure, using both legacy and modern HWID fields. This aligns with the broader system update for HWID management.
Let's verify the consistency of HWID parameter usage across the codebase:
#!/bin/bash
# Search for GetServerRoleBansAsync usage to ensure consistent parameter passing
rg -l "GetServerRoleBansAsync" | xargs rg "GetServerRoleBansAsync.*LastModernHWIds"Content.Server/Administration/Managers/IBanManager.cs (1)
41-41: Verify impact of interface changes
The method signature changes (HWID type and removed exemptFlags) could affect implementing classes. Please ensure all implementations are updated accordingly.
#!/bin/bash
# Find all implementations of IBanManager
rg "class.*:.*IBanManager" --type csharp
# Check for any remaining uses of the old HWID type
rg "ImmutableArray<byte>\?.*hwid" --type csharpContent.Server/Database/DatabaseRecords.cs (1)
123-123: Verify consistent HWID type usage across the codebase
The change from ImmutableArray<byte>? to ImmutableTypedHwid? improves type safety. Let's verify that this type is consistently used across the codebase.
#!/bin/bash
# Description: Check for any remaining ImmutableArray<byte> usages for HWIDs
# that might have been missed during the migration
# Search for remaining ImmutableArray<byte> declarations in HWID-related contexts
rg -l "ImmutableArray<byte>" | while read -r file; do
echo "=== $file ==="
rg -A 2 -B 2 "ImmutableArray<byte>.*(?i:hwid|hardware)" "$file" || true
done
# Search for ImmutableTypedHwid usage to ensure consistent adoption
rg -l "ImmutableTypedHwid" | while read -r file; do
echo "=== $file ==="
rg -A 2 -B 2 "ImmutableTypedHwid" "$file" || true
doneContent.IntegrationTests/Tests/Commands/PardonCommand.cs (1)
35-37: LGTM! Test updates properly handle the new parameter structure
The changes correctly update the database method calls to include the new null parameters for modern HWIDs while maintaining the existing test coverage and assertions.
Also applies to: 46-48, 60-62, 73-73, 79-79, 98-98, 108-108, 136-136, 142-142
Content.Client/Administration/UI/BanPanel/BanPanel.xaml.cs (2)
25-25: LGTM: Type-safe HWID handling
The change from byte[]? to ImmutableTypedHwid? improves type safety and provides a more structured approach to handling hardware identifiers.
Also applies to: 29-29
Line range hint 374-392: LGTM: Improved HWID parsing logic
The updated HWID parsing logic using ImmutableTypedHwid.TryParse is more robust than the previous implementation:
- Properly handles parsing failures
- Maintains consistent error state management
- Clearer control flow for HWID assignment
Content.Server/Administration/Systems/BwoinkSystem.cs (1)
175-175: LGTM: Updated ban check signature
The addition of the null parameter aligns with the updated GetServerBanAsync method signature, maintaining consistency with the new HWID handling system.
Content.Server/DeltaV/Objectives/Systems/TeachLessonConditionSystem.cs (1)
1-44: LGTM!
The code changes correctly implement the logic to complete the objective when the target entity dies. The implementation appears sound and integrates well with the existing systems.
Content.Shared/DeltaV/CartridgeLoader/Cartridges/NanoChatUiMessageEvent.cs (1)
1-166: LGTM!
The new classes and structs are well-defined and adhere to serialization requirements. The implementation appears correct and aligns with the intended functionality.
Content.Server/Access/Systems/AgentIDCardSystem.cs (3)
22-22: Approved: Dependency on SharedNanoChatSystem Added Correctly
The addition of the SharedNanoChatSystem dependency is appropriate for integrating NanoChat functionality into the AgentIDCardSystem.
33-33: Approved: Subscribed to AgentIDCardNumberChangedMessage Event
Subscribing to the AgentIDCardNumberChangedMessage event ensures that changes to the NanoChat number are properly handled within the system.
111-121: Approved: NanoChat Number Integrated into UI State Successfully
The modifications in the AfterUIOpen method correctly retrieve the current NanoChat number and pass it to the AgentIDCardBoundUserInterfaceState. This ensures that the UI displays the current NanoChat number when opened.
Content.Client/DeltaV/CartridgeLoader/Cartridges/NanoChatUiFragment.xaml.cs (1)
217-221: Approved: Mute Button Updates Correctly Reflect Notification Status
The UpdateMuteButton method correctly updates the visibility of the mute icon based on the _notificationsMuted flag.
Content.Server/DeltaV/CartridgeLoader/Cartridges/NanoChatCartridgeSystem.cs (2)
332-335: Approved: Proper Handling of Radio Receive Attempt Event
The RadioReceiveAttemptEvent is correctly raised and checked for cancellation, ensuring that only valid recipients receive the message.
218-229:
Validate Sender Number Before Processing Messages
In HandleSendMessage, the sender's NanoChat number is assumed to be non-null. If card.Comp.Number is null, this will result in a null reference exception.
Add a null check for card.Comp.Number before proceeding:
var senderNumber = card.Comp.Number;
if (senderNumber == null)
return;
// Proceed with using senderNumber.ValueLikely invalid or redundant comment.
Content.Server/CartridgeLoader/Cartridges/LogProbeCartridgeComponent.cs (1)
2-2: LGTM! Clean implementation of NanoChat data storage.
The new property is well-documented and properly implemented with appropriate nullability and serialization support.
Also applies to: 22-27
Content.Shared/CartridgeLoader/Cartridges/LogProbeUiState.cs (1)
14-17: LGTM! Well-documented property addition.
The NanoChatData property is properly documented and correctly marked as nullable, following C# best practices.
Content.Client/CartridgeLoader/Cartridges/LogProbeUiFragment.xaml (2)
14-18: LGTM: Title label implementation follows best practices
The title label implementation follows UI best practices with proper localization, styling, and alignment.
22-26: Verify CardNumberLabel visibility toggle logic
The CardNumberLabel is initially set to invisible. Please ensure the visibility toggle logic is properly implemented in the corresponding code-behind file.
#!/bin/bash
# Search for CardNumberLabel visibility toggle implementation
ast-grep --pattern 'CardNumberLabel.Visible = $_'Content.Client/Access/UI/AgentIDCardBoundUserInterface.cs (2)
29-29: LGTM: Number change handling follows established patterns
The implementation follows the existing event handling pattern and properly integrates with the message system.
Also applies to: 32-36
66-66: Verify null handling for CurrentNumber
Ensure that cast.CurrentNumber is properly null-checked in the SetCurrentNumber implementation.
#!/bin/bash
# Search for SetCurrentNumber implementation to verify null handling
ast-grep --pattern 'SetCurrentNumber($_)'Content.Client/DeltaV/CartridgeLoader/Cartridges/NewChatPopup.xaml (1)
44-48: Verify Create button enable/disable logic
The Create button is initially disabled. Please ensure the enable/disable logic is properly implemented based on valid input states.
#!/bin/bash
# Search for Create button state management
ast-grep --pattern 'CreateButton.Disabled = $_'Content.Shared/Access/SharedAgentIDCardSystem.cs (1)
31-31: LGTM: Clean implementation of the card number feature
The addition of the CurrentNumber property and its integration into the constructor follows the existing patterns. The nullable uint type is appropriate for optional card numbers.
Also applies to: 33-40
Content.Server/CartridgeLoader/Cartridges/LogProbeCartridgeSystem.cs (2)
12-12: Verify the partial class implementation
The class has been made partial, but the other part(s) of the implementation are not visible in the current changes.
#!/bin/bash
# Search for other parts of the LogProbeCartridgeSystem implementation
rg -l "partial class LogProbeCartridgeSystem"80-81: Verify state synchronization
Ensure that the UI state is properly synchronized when switching between access logs and NanoChat data.
#!/bin/bash
# Search for UI state handling in related components
ast-grep --pattern 'class $_ {
$$$
UpdateState($_) {
$$$
}
$$$
}'Content.Shared/DeltaV/CCVars/DCCVars.cs (2)
72-73: LGTM: Time representation simplified for RequestAcoDelay
The change from TimeSpan to float (minutes) makes the configuration more straightforward and easier to manage.
91-92: Verify the time unit conversion in dependent code
While the change to float minutes is good, ensure that all code consuming this value has been updated to handle minutes instead of TimeSpan.
#!/bin/bash
# Search for usages of AutoUnlockAllAccessDelay to verify time unit handling
rg -A 5 "AutoUnlockAllAccessDelay" --type csContent.Client/Access/UI/AgentIDCardWindow.xaml.cs (1)
24-25: LGTM: Clear constant definition
Good practice to define the maximum length as a constant with a clear comment referencing the related component.
Content.Server/Administration/Systems/AdminSystem.cs (1)
343-343: Verify panic bunker behavior with the new permission check
The change affects how the panic bunker enables/disables based on admin presence. Let's verify the impact across the codebase.
#!/bin/bash
# Description: Check for panic bunker related code and admin permission checks
# Check for panic bunker related code
echo "=== Panic Bunker Usage ==="
rg -A 5 "PanicBunker"
# Check for admin permission checks
echo "=== Admin Permission Checks ==="
rg -A 5 "HasAdminFlag.*Ban"
# Check for admin help related code
echo "=== Admin Help Related Code ==="
rg -A 5 "AdminHelp"| public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected) | ||
| { | ||
| // Remove old handler if it exists | ||
| if (_pressHandler != null) | ||
| ChatButton.OnPressed -= _pressHandler; | ||
|
|
||
| _number = number; | ||
|
|
||
| // Create and store new handler | ||
| _pressHandler = _ => OnPressed?.Invoke(_number); | ||
| ChatButton.OnPressed += _pressHandler; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the recipient parameter
The method should validate the input parameter to prevent potential null reference exceptions.
Here's a suggested implementation:
public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected)
{
+ ArgumentNullException.ThrowIfNull(recipient);
+
// Remove old handler if it exists
if (_pressHandler != null)
ChatButton.OnPressed -= _pressHandler;📝 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.
| public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected) | |
| { | |
| // Remove old handler if it exists | |
| if (_pressHandler != null) | |
| ChatButton.OnPressed -= _pressHandler; | |
| _number = number; | |
| // Create and store new handler | |
| _pressHandler = _ => OnPressed?.Invoke(_number); | |
| ChatButton.OnPressed += _pressHandler; | |
| public void SetRecipient(NanoChatRecipient recipient, uint number, bool isSelected) | |
| { | |
| ArgumentNullException.ThrowIfNull(recipient); | |
| // Remove old handler if it exists | |
| if (_pressHandler != null) | |
| ChatButton.OnPressed -= _pressHandler; | |
| _number = number; | |
| // Create and store new handler | |
| _pressHandler = _ => OnPressed?.Invoke(_number); | |
| ChatButton.OnPressed += _pressHandler; | |
| <PanelContainer Name="UnreadIndicator" | ||
| MinSize="8 8" | ||
| MaxSize="8 8" | ||
| VerticalAlignment="Center" | ||
| Margin="0 0 6 0"> | ||
| <PanelContainer.PanelOverride> | ||
| <graphics:StyleBoxFlat | ||
| BackgroundColor="#17c622" | ||
| BorderColor="#0f7a15" /> | ||
| </PanelContainer.PanelOverride> | ||
| </PanelContainer> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme-aware colors for better accessibility.
The unread indicator uses hardcoded colors (#17c622, #0f7a15) which might not respect system theme or accessibility settings.
Consider using theme-aware color resources:
-BackgroundColor="#17c622"
-BorderColor="#0f7a15"
+BackgroundColor="{DynamicResource positiveGreenColor}"
+BorderColor="{DynamicResource positiveGreenDarkColor}"📝 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.
| <PanelContainer Name="UnreadIndicator" | |
| MinSize="8 8" | |
| MaxSize="8 8" | |
| VerticalAlignment="Center" | |
| Margin="0 0 6 0"> | |
| <PanelContainer.PanelOverride> | |
| <graphics:StyleBoxFlat | |
| BackgroundColor="#17c622" | |
| BorderColor="#0f7a15" /> | |
| </PanelContainer.PanelOverride> | |
| </PanelContainer> | |
| <PanelContainer Name="UnreadIndicator" | |
| MinSize="8 8" | |
| MaxSize="8 8" | |
| VerticalAlignment="Center" | |
| Margin="0 0 6 0"> | |
| <PanelContainer.PanelOverride> | |
| <graphics:StyleBoxFlat | |
| BackgroundColor="{DynamicResource positiveGreenColor}" | |
| BorderColor="{DynamicResource positiveGreenDarkColor}" /> | |
| </PanelContainer.PanelOverride> | |
| </PanelContainer> |
| <Button Name="ChatButton" | ||
| StyleClasses="ButtonSquare" | ||
| HorizontalExpand="True" | ||
| MaxSize="137 64" | ||
| Margin="0 1"> | ||
| <BoxContainer Orientation="Horizontal" | ||
| HorizontalExpand="True" | ||
| VerticalExpand="True" | ||
| MinWidth="132" | ||
| Margin="6 4" | ||
| VerticalAlignment="Center"> | ||
| <!-- Unread indicator dot --> | ||
| <PanelContainer Name="UnreadIndicator" | ||
| MinSize="8 8" | ||
| MaxSize="8 8" | ||
| VerticalAlignment="Center" | ||
| Margin="0 0 6 0"> | ||
| <PanelContainer.PanelOverride> | ||
| <graphics:StyleBoxFlat | ||
| BackgroundColor="#17c622" | ||
| BorderColor="#0f7a15" /> | ||
| </PanelContainer.PanelOverride> | ||
| </PanelContainer> | ||
|
|
||
| <!-- Text container --> | ||
| <BoxContainer Orientation="Vertical" | ||
| HorizontalExpand="True" | ||
| VerticalExpand="True" | ||
| VerticalAlignment="Center"> | ||
| <RichTextLabel Name="NameLabel" | ||
| StyleClasses="LabelHeading" | ||
| HorizontalExpand="True" | ||
| HorizontalAlignment="Center" | ||
| VerticalAlignment="Center" | ||
| Margin="0 -2 0 0" /> | ||
| <Label Name="JobLabel" | ||
| StyleClasses="LabelSubText" | ||
| HorizontalExpand="True" | ||
| ClipText="False" | ||
| HorizontalAlignment="Center" /> | ||
| </BoxContainer> | ||
| </BoxContainer> | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user feedback.
The button lacks important accessibility features and visual feedback states:
- No tooltip for additional context
- No ARIA labels or roles
- No visual feedback for hover/pressed states
Consider adding:
<Button Name="ChatButton"
StyleClasses="ButtonSquare"
+ ToolTip="{Binding TooltipText}"
+ AccessibleName="{Binding Name}"
+ AccessibleDescription="{Binding JobTitle}"
HorizontalExpand="True"Committable suggestion skipped: line range outside the PR's diff.
| protected override void FrameUpdate(FrameEventArgs args) | ||
| { | ||
| base.FrameUpdate(args); | ||
|
|
||
| if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) | ||
| UpdateMiningPoints(points.Points); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the frame update
The current implementation checks for mining points every frame, which might be unnecessary. Consider:
- Using an event-based approach
- Adding a throttle/debounce mechanism
+ private const float UpdateInterval = 0.5f; // Update every 500ms
+ private float _timeSinceLastUpdate = 0f;
+
protected override void FrameUpdate(FrameEventArgs args)
{
base.FrameUpdate(args);
+ _timeSinceLastUpdate += args.DeltaSeconds;
+ if (_timeSinceLastUpdate < UpdateInterval)
+ return;
+
+ _timeSinceLastUpdate = 0f;
if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points))
UpdateMiningPoints(points.Points);
}📝 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.
| protected override void FrameUpdate(FrameEventArgs args) | |
| { | |
| base.FrameUpdate(args); | |
| if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) | |
| UpdateMiningPoints(points.Points); | |
| } | |
| private const float UpdateInterval = 0.5f; // Update every 500ms | |
| private float _timeSinceLastUpdate = 0f; | |
| protected override void FrameUpdate(FrameEventArgs args) | |
| { | |
| base.FrameUpdate(args); | |
| _timeSinceLastUpdate += args.DeltaSeconds; | |
| if (_timeSinceLastUpdate < UpdateInterval) | |
| return; | |
| _timeSinceLastUpdate = 0f; | |
| if (_entityManager.TryGetComponent<MiningPointsComponent>(Entity, out var points)) | |
| UpdateMiningPoints(points.Points); | |
| } |
| // fuuuuuck | ||
| MessageBox.Parent?.RemoveChild(MessageBox); |
There was a problem hiding this comment.
Remove unprofessional comment and improve code clarity
The comment is unprofessional and doesn't explain the purpose of the code. Additionally, the parent-child manipulation could be clearer.
- // fuuuuuck
- MessageBox.Parent?.RemoveChild(MessageBox);
+ // Ensure MessageBox is detached from any previous parent before reattaching
+ if (MessageBox.Parent != null)
+ {
+ MessageBox.Parent.RemoveChild(MessageBox);
+ }📝 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.
| // fuuuuuck | |
| MessageBox.Parent?.RemoveChild(MessageBox); | |
| // Ensure MessageBox is detached from any previous parent before reattaching | |
| if (MessageBox.Parent != null) | |
| { | |
| MessageBox.Parent.RemoveChild(MessageBox); | |
| } |
| // DeltaV - Add number validation and event | ||
| private void OnNumberEntered(LineEdit.LineEditEventArgs args) | ||
| { | ||
| if (uint.TryParse(args.Text, out var number) && number > 0) | ||
| OnNumberChanged?.Invoke(number); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null/empty input handling
The number validation should explicitly handle null or empty input cases.
private void OnNumberEntered(LineEdit.LineEditEventArgs args)
{
+ if (string.IsNullOrEmpty(args.Text))
+ return;
+
if (uint.TryParse(args.Text, out var number) && number > 0)
OnNumberChanged?.Invoke(number);
}📝 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.
| // DeltaV - Add number validation and event | |
| private void OnNumberEntered(LineEdit.LineEditEventArgs args) | |
| { | |
| if (uint.TryParse(args.Text, out var number) && number > 0) | |
| OnNumberChanged?.Invoke(number); | |
| } | |
| // DeltaV - Add number validation and event | |
| private void OnNumberEntered(LineEdit.LineEditEventArgs args) | |
| { | |
| if (string.IsNullOrEmpty(args.Text)) | |
| return; | |
| if (uint.TryParse(args.Text, out var number) && number > 0) | |
| OnNumberChanged?.Invoke(number); | |
| } |
| @@ -340,7 +340,7 @@ private void UpdatePanicBunker() | |||
| var admins = PanicBunker.CountDeadminnedAdmins | |||
| ? _adminManager.AllAdmins | |||
| : _adminManager.ActiveAdmins; | |||
| var hasAdmins = admins.Any(); | |||
| var hasAdmins = admins.Any(p => _adminManager.HasAdminFlag(p, AdminFlags.Ban)); // DeltaV: Add predicate to require someone that can handle ahelps, not just curators | |||
There was a problem hiding this comment.
Security: Consider implications of requiring Ban permission for panic bunker control
The change makes panic bunker control more restrictive by requiring admins to have Ban permissions. While this increases security, it might be too restrictive as not all admin help handlers necessarily need Ban permissions.
Consider these alternatives:
- Use a more appropriate permission flag specific to admin help handling
- Add a fallback mechanism when no admins with Ban permissions are available
- Create a new permission flag specifically for panic bunker control
-var hasAdmins = admins.Any(p => _adminManager.HasAdminFlag(p, AdminFlags.Ban)); // DeltaV: Add predicate to require someone that can handle ahelps, not just curators
+// Option 1: Use admin help specific permission
+var hasAdmins = admins.Any(p => _adminManager.HasAdminFlag(p, AdminFlags.AdminHelp));
+// Option 2: Allow either Ban or AdminHelp permission
+var hasAdmins = admins.Any(p => _adminManager.HasAdminFlag(p, AdminFlags.Ban) || _adminManager.HasAdminFlag(p, AdminFlags.AdminHelp));Committable suggestion skipped: line range outside the PR's diff.
Content.Server.Database/Model.cs
Outdated
| @@ -668,7 +711,7 @@ public interface IBanCommon<TUnban> where TUnban : IUnbanCommon | |||
| int Id { get; set; } | |||
| Guid? PlayerUserId { get; set; } | |||
| NpgsqlInet? Address { get; set; } | |||
| byte[]? HWId { get; set; } | |||
| TypedHwid? HWId { get; set; } | |||
There was a problem hiding this comment.
Handle Potential Null Values for HWId Properties
The HWId properties in IBanCommon, ServerBan, ConnectionLog, and ServerRoleBan are defined as nullable (TypedHwid?). Ensure that all usages of these properties include null checks to prevent potential NullReferenceExceptions.
Also applies to: 799-799, 937-937, 993-993
Content.Server.Database/Model.cs
Outdated
| [return: NotNullIfNotNull(nameof(immutable))] | ||
| public static implicit operator TypedHwid?(ImmutableTypedHwid? immutable) | ||
| { | ||
| if (immutable == null) | ||
| return null; | ||
|
|
||
| return new TypedHwid | ||
| { | ||
| Hwid = immutable.Hwid.ToArray(), | ||
| Type = immutable.Type, | ||
| }; | ||
| } | ||
|
|
||
| [return: NotNullIfNotNull(nameof(hwid))] | ||
| public static implicit operator ImmutableTypedHwid?(TypedHwid? hwid) | ||
| { | ||
| if (hwid == null) | ||
| return null; | ||
|
|
||
| return new ImmutableTypedHwid(hwid.Hwid.ToImmutableArray(), hwid.Type); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure Safe Handling of Null Values in Implicit Conversions
The implicit conversion operators in TypedHwid should safely handle null values to prevent exceptions during runtime.
Modify the conversion operators to include null checks:
[return: NotNullIfNotNull(nameof(immutable))]
public static implicit operator TypedHwid?(ImmutableTypedHwid? immutable)
{
if (immutable == null)
return null;
return new TypedHwid
{
Hwid = immutable.Hwid.ToArray(),
Type = immutable.Type,
};
}
[return: NotNullIfNotNull(nameof(hwid))]
public static implicit operator ImmutableTypedHwid?(TypedHwid? hwid)
{
if (hwid == null)
return null;
return new ImmutableTypedHwid(hwid.Hwid.ToImmutableArray(), hwid.Type);
}
Content.Server.Database/Model.cs
Outdated
| { | ||
| public byte[] Hwid { get; set; } = default!; | ||
| public HwidType Type { get; set; } |
There was a problem hiding this comment.
Initialize Hwid Property to Avoid Null References
In the TypedHwid class, the Hwid property is initialized with default!, which could lead to null reference issues if not properly assigned before use. Initialize it with an empty array to ensure it's not null.
Apply this change:
-public byte[] Hwid { get; set; } = default!;
+public byte[] Hwid { get; set; } = Array.Empty<byte>();📝 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.
| { | |
| public byte[] Hwid { get; set; } = default!; | |
| public HwidType Type { get; set; } | |
| { | |
| public byte[] Hwid { get; set; } = Array.Empty<byte>(); | |
| public HwidType Type { get; set; } |
This reverts commit 26b36db.
* Add the locker + fill * In DeltaV folder * Generic! * Remove appearance * Fix meta.json
* initial commit * oops * Update flora.yml * Update flora.yml
* initial commit * fixes
О чем PR
Почему
Технические детали
Медиа
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation