feat: port ItemCondition from LustStation#4066
feat: port ItemCondition from LustStation#4066ThereDrD0 merged 1 commit intospace-sunrise:masterfrom
Conversation
📝 WalkthroughПошаговое описаниеВведён новый класс Изменения
Прогнозируемые затраты на код-ревью🎯 2 (Простой) | ⏱️ ~10 минут Предлагаемые метки
Стихотворение
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs (2)
57-70: Проверка всех контейнеров может дать неожиданные результаты.Когда
EquipmentSlotsпуст, код проверяет все контейнеры сущности, включая хранилища (карманы, сумки и т.д.), а не только экипировку. Это может привести к ложным срабатываниям, если требуется проверить именно надетые предметы.Рассмотрите возможность:
- Явно указывать слоты экипировки по умолчанию
- Добавить в документацию, что пустой список означает "все контейнеры"
- Или использовать
InventorySystemдля точной проверки экипировки🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs` around lines 57 - 70, The current CheckEquipped block iterates all ContainerManagerComponent.Containers when EquipmentSlots is empty, which checks non-equip containers (pockets/bags); change the behavior so an empty EquipmentSlots triggers checking only actual equipment containers: when CheckEquipped is true and EquipmentSlots.Count == 0, query the InventorySystem (or call a helper like InventorySystem.GetEquipmentContainers/GetEquippedEntities) for the entity and iterate those containers/entities instead of ContainerManagerComponent.Containers; alternatively, require callers to supply explicit EquipmentSlots by throwing or documenting that empty means "all containers"—update the logic in ItemCondition (the CheckEquipped branch that uses entityManager.TryGetComponent<ContainerManagerComponent> and ContainerManagerComponent.Containers) to filter by equipment-only containers via InventorySystem methods or a container.IsEquipment flag before calling IsMatchingItem.
12-16: Граничный случай: оба флага проверки false.Если
CheckInitiator = falseиCheckTarget = false, методIsMetвсегда вернётtrueбез выполнения каких-либо проверок. Убедитесь, что это ожидаемое поведение. Если нет — рассмотрите добавление валидации или логирования предупреждения.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs` around lines 12 - 16, ItemCondition currently has two flags CheckInitiator and CheckTarget; if both are false IsMet will always return true which is likely unintended; update the ItemCondition.IsMet method to handle the both-false case (e.g., detect if CheckInitiator == false && CheckTarget == false and either return false or log a warning/error and return false) and additionally add validation after deserialization (or in the class initialization path) to assert at least one of CheckInitiator or CheckTarget is true so invalid configs are caught early; reference CheckInitiator, CheckTarget, IsMet and ItemCondition when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs`:
- Around line 18-19: IsMatchingItem currently treats an empty ItemWhiteList as
non-matching which makes the condition impossible when CheckInitiator is true;
update IsMatchingItem (and any callers) so that if ItemWhiteList is null or
Count == 0 it returns true (treating an empty whitelist as "any item"), or
alternatively add validation in the ItemCondition constructor/OnDeserialize to
log/throw when ItemWhiteList is empty—preferred: implement the early-return
match inside IsMatchingItem to short-circuit and return true when ItemWhiteList
is empty to preserve expected default behavior.
---
Nitpick comments:
In `@Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs`:
- Around line 57-70: The current CheckEquipped block iterates all
ContainerManagerComponent.Containers when EquipmentSlots is empty, which checks
non-equip containers (pockets/bags); change the behavior so an empty
EquipmentSlots triggers checking only actual equipment containers: when
CheckEquipped is true and EquipmentSlots.Count == 0, query the InventorySystem
(or call a helper like
InventorySystem.GetEquipmentContainers/GetEquippedEntities) for the entity and
iterate those containers/entities instead of
ContainerManagerComponent.Containers; alternatively, require callers to supply
explicit EquipmentSlots by throwing or documenting that empty means "all
containers"—update the logic in ItemCondition (the CheckEquipped branch that
uses entityManager.TryGetComponent<ContainerManagerComponent> and
ContainerManagerComponent.Containers) to filter by equipment-only containers via
InventorySystem methods or a container.IsEquipment flag before calling
IsMatchingItem.
- Around line 12-16: ItemCondition currently has two flags CheckInitiator and
CheckTarget; if both are false IsMet will always return true which is likely
unintended; update the ItemCondition.IsMet method to handle the both-false case
(e.g., detect if CheckInitiator == false && CheckTarget == false and either
return false or log a warning/error and return false) and additionally add
validation after deserialization (or in the class initialization path) to assert
at least one of CheckInitiator or CheckTarget is true so invalid configs are
caught early; reference CheckInitiator, CheckTarget, IsMet and ItemCondition
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3d96a6c-a1be-4283-b013-a0d0ad8d895f
📒 Files selected for processing (1)
Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs
| [DataField] | ||
| public List<EntProtoId> ItemWhiteList { get; private set; } = new(); |
There was a problem hiding this comment.
Пустой whitelist делает условие невыполнимым.
Если ItemWhiteList пуст (значение по умолчанию), то IsMatchingItem всегда возвращает false, и условие никогда не будет выполнено при CheckInitiator = true (также по умолчанию).
Рассмотрите добавление валидации или раннего возврата true, если whitelist пуст (если это интерпретируется как "любой предмет подходит").
💡 Возможное исправление
private bool IsMatchingItem(EntityUid entity, EntityManager entityManager)
{
+ // Пустой whitelist = любой предмет подходит
+ if (ItemWhiteList.Count == 0)
+ return true;
+
if (!entityManager.TryGetComponent<MetaDataComponent>(entity, out var meta))
return false;
return meta.EntityPrototype != null && ItemWhiteList.Contains(meta.EntityPrototype.ID);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs`
around lines 18 - 19, IsMatchingItem currently treats an empty ItemWhiteList as
non-matching which makes the condition impossible when CheckInitiator is true;
update IsMatchingItem (and any callers) so that if ItemWhiteList is null or
Count == 0 it returns true (treating an empty whitelist as "any item"), or
alternatively add validation in the ItemCondition constructor/OnDeserialize to
log/throw when ItemWhiteList is empty—preferred: implement the early-return
match inside IsMatchingItem to short-circuit and return true when ItemWhiteList
is empty to preserve expected default behavior.
There was a problem hiding this comment.
Поведение намеренное. ItemWhiteList — это вайтлист конкретных предметов, а не "наличие хоть какого-то предмета". Пустой список означает, что предметы для проверки не заданы, поэтому false — корректный результат.
Если CheckInitiator = true и ItemWhiteList пуст — это ошибка конфигурации в YAML, а не баг кода.
There was a problem hiding this comment.
@Soragy-bot, понял, спасибо за пояснение. Согласен — если ItemWhiteList пуст, это ошибка конфигурации в YAML, а не баг кода. Поведение корректно.
(>‿‿>)
✏️ Learnings added
Learnt from: Soragy-bot
Repo: space-sunrise/sunrise-station PR: 4066
File: Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs:18-19
Timestamp: 2026-03-18T17:27:20.984Z
Learning: In Content.Shared/_Sunrise/InteractionsPanel/Data/Conditions/ItemCondition.cs, an empty ItemWhiteList intentionally causes IsMatchingItem to return false. It is a whitelist of specific items, not a "any item present" check. An empty ItemWhiteList combined with CheckInitiator=true is considered a YAML configuration error, not a code bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kanopus952
Repo: space-sunrise/sunrise-station PR: 3973
File: Content.Server/_Sunrise/StationEvents/Components/ResearchPointVirusRuleComponent.cs:1-10
Timestamp: 2026-03-09T11:33:51.097Z
Learning: In C# projects using global using directives, attributes and types from Robust.Shared.Analyzers are accessible without per-file using statements when the corresponding global usings are configured (as in Content.Server/GlobalUsings.cs, Content.Shared/GlobalUsings.cs, Content.Client/GlobalUsings.cs). Do not add an explicit 'using Robust.Shared.Analyzers;' to individual files like Content.Server/_Sunrise/StationEvents/Components/ResearchPointVirusRuleComponent.cs unless a file outside the existing global usings context requires a separate import. Apply this guidance to all C# files; verify global usings cover the needed symbol in any new file.
github-project-automation
bot
moved this from НЕ РАЗОБРАНО
to СДЕЛАНО
in Разбор PR и Issues
2 participants
Кратное описание
Делаю порт, чтобы не было конфликтов по просьбе space-sunrise/lust-station#499
По какой причине
Медиа(Видео/Скриншоты)
Changelog
🆑 Soragy
Summary by CodeRabbit
New Features