Supermatter Engine / Multiauth #1015
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughИнтегрирование контекста сервера аутентификации в слои клиента и сервера. Изменения включают обновление URL субмодуля RobustToolbox, добавление управления идентификатором сервера в игровых интерфейсах и чатах, и обновление локализации сообщений OOC для отображения информации о сервере аутентификации. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs:
- Around line 112-115: The displayName construction can throw a
NullReferenceException because
_playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer may be
null; fix by retrieving the session into a local (e.g., var session =
_playerManager.GetSessionById(info.SessionId)), then null-check session,
session.Channel, session.Channel.UserData and
session.Channel.UserData.AuthServer before accessing Id (use conditional access
or explicit checks) and fall back to a safe string like "unknown" when any piece
is null; update the displayName expression in PlayerListControl.xaml.cs to use
that guarded value (mirror the null-safe pattern used in
PlayerListEntry.xaml.cs).
In @Content.Server/Administration/PlayerLocator.cs:
- Line 105:
AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First()
can throw InvalidOperationException for an empty/invalid list; change the call
to safely handle empties (e.g., use FirstOrDefault/null-check or check the
returned collection count) and then handle the missing server case before
accessing AuthUrl — update the usage in the method that currently reads
AuthServer.FromStringList(...).First().AuthUrl to use a safe retrieval
(FirstOrDefault), verify the result is non-null (or collection non-empty) and
provide a fallback or clear error/log message if no auth server is available.
- Line 124: The code calls
AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl
which can throw on an empty sequence and duplicates this logic; extract this
into a private helper (e.g. GetAuthServerUrl or ResolveAuthServerUrl) that calls
AuthServer.FromStringList, checks for empty/null (use
FirstOrDefault/SingleOrDefault), returns AuthUrl or handles the missing case
(log and throw a clear exception or return null/Optional) and replace all direct
.First().AuthUrl usages with this helper to remove duplication and avoid the
exception.
In @Content.Server/Chat/Managers/ChatManager.cs:
- Around line 253-264: The inline block comments around the authServer logic
contain a typo: change the opening marker "WD EDDIT START" to "WD EDIT START"
(matching the closing "WD EDIT END") so comment markers are consistent; update
only the comment text surrounding the AuthServer resolution/Loc.GetString call
(the lines containing "WD EDDIT START" and "WD EDIT END").
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitmodulesContent.Client/Administration/UI/CustomControls/PlayerListControl.xaml.csContent.Client/Administration/UI/CustomControls/PlayerListEntry.xaml.csContent.Server/Administration/PlayerLocator.csContent.Server/Chat/Managers/ChatManager.csResources/Locale/en-US/chat/managers/chat-manager.ftlResources/Locale/ru-RU/chat/managers/chat-manager.ftlRobustToolbox
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Lachklen
Repo: WWhiteDreamProject/wwdpublic PR: 863
File: Resources/Locale/ru-RU/_white/cards/danger.ftl:1-1
Timestamp: 2025-10-22T20:31:06.235Z
Learning: В PR по EvacPod для WWhiteDreamProject/wwdpublic: в ru-RU локализациях допустим осознанный сленг сообщества SS13 (напр., «дистры»), если он не ломает отображение и используется последовательно; не блокировать PR, предлагать необязательный follow-up (глоссарий/проверка консистентности).
Learnt from: Lachklen
Repo: WWhiteDreamProject/wwdpublic PR: 863
File: Resources/Locale/en-US/_white/cards/hobby.ftl:5-5
Timestamp: 2025-10-22T20:13:31.389Z
Learning: В PR по EvacPod для репозитория WWhiteDreamProject/wwdpublic: если опечатка в ключе локализации (например, card-ep_hobby_mashrooms) не влияет на отображение и используется последовательно, автор предпочитает не блокировать PR; вместо этого предлагать необязательный follow-up issue.
📚 Learning: 2026-01-04T20:41:27.251Z
Learnt from: Cinkafox
Repo: WWhiteDreamProject/wwdpublic PR: 1001
File: Content.Server/Database/ServerDbBase.cs:296-296
Timestamp: 2026-01-04T20:41:27.251Z
Learning: In Content.Server/Database/ServerDbBase.cs, ensure uniqueness of LoadoutName before persisting or converting the loadouts. If you convert the list to a dictionary with ToDictionary(p => p.LoadoutName), the key must be unique; otherwise handle duplicates (e.g., use ToLookup or GroupBy) to avoid runtime errors. This pattern can improve performance by eliminating extra lookups, but only apply when LoadoutName uniqueness is guaranteed by prior validation.
Applied to files:
Content.Server/Administration/PlayerLocator.csContent.Server/Chat/Managers/ChatManager.cs
📚 Learning: 2025-09-24T18:18:06.673Z
Learnt from: RedFoxIV
Repo: WWhiteDreamProject/wwdpublic PR: 849
File: Content.Client/_White/UserInterface/WindowTracker.cs:1-5
Timestamp: 2025-09-24T18:18:06.673Z
Learning: The WWhiteDreamProject codebase uses GlobalUsings.cs files to provide global using directives. IoCManager is available throughout Content.Client without explicit using Robust.Shared.IoC; directive due to global usings configuration. Code analysis should account for global usings when checking for missing using statements.
Applied to files:
Content.Server/Chat/Managers/ChatManager.csContent.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs
📚 Learning: 2025-09-03T16:17:01.745Z
Learnt from: Cinkafox
Repo: WWhiteDreamProject/wwdpublic PR: 827
File: Content.Client/_White/UI/Controls/Knob.cs:28-28
Timestamp: 2025-09-03T16:17:01.745Z
Learning: In this codebase with Robust UI, base Control does not expose a Disabled property; Disabled exists on specific widgets like BaseButton. Do not flag shadowing for user-defined Disabled on custom controls such as Content.Client/_White/UI/Controls/Knob.cs.
Applied to files:
Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packaging
- GitHub Check: YAML map schema validator
- GitHub Check: YAML Linter
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (5)
RobustToolbox (1)
1-1: Убедитесь что обновление SubModule протестировано, особенно функциональность ban-системы.Обновление указателя субмодуля (до текущей HEAD/master ветки Simple-Station/SupermatterEngine) является критичным для новой функциональности мультиаутентификации. Учитывая что автор выразил неуверенность в работе банов, необходимо провести тщательное тестирование интеграции:
- Корректная работа multiauth функциональности со всеми identifiers
- Проверка ban-системы при использовании нескольких auth-серверов
- Валидация ISharedPlayerManager и CVars.AuthServers в новой версии
- OOC сообщения с authServer информацией
Resources/Locale/en-US/chat/managers/chat-manager.ftl (1)
43-50: LGTM!Шаблоны локализации корректно обновлены для поддержки условного отображения сервера аутентификации. Синтаксис Fluent корректен, и логика соответствует серверным изменениям в
ChatManager.cs.Resources/Locale/ru-RU/chat/managers/chat-manager.ftl (1)
40-47: LGTM!Русская локализация корректно обновлена и соответствует английской версии. Синтаксис Fluent и логика условного отображения сервера аутентификации идентичны.
Content.Server/Chat/Managers/ChatManager.cs (1)
273-280: LGTM!Добавление
authServerк patron OOC wrap сообщению выполнено корректно. Переиспользование ранее определённой переменнойauthServerобеспечивает консистентность..gitmodules (1)
1-4: Файл.gitmodulesне содержит изменений в этом PR. Текущее состояние субмодуля (указание на Simple-Station/SupermatterEngine) уже установлено в репозитории и не изменяется данным коммитом.Likely an incorrect or invalid review comment.
Comment on lines
+112
to
+115
| // WD EDIT START | ||
| var displayName = $"{info.CharacterName} ({info.Username}@{AuthServer.GetServerFromCVarListByUrl(_config, | ||
| _playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer)?.Id})"; | ||
| // WD EDIT END |
Contributor
There was a problem hiding this comment.
Потенциальное NullReferenceException при получении сессии (аналогично PlayerListEntry).
Та же проблема, что и в PlayerListEntry.xaml.cs — цепочка вызовов _playerManager.GetSessionById(...).Channel.UserData.AuthServer может выбросить исключение.
🛠️ Предлагаемое исправление
- var displayName = $"{info.CharacterName} ({info.Username}@{AuthServer.GetServerFromCVarListByUrl(_config,
- _playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer)?.Id})";
+ var serverId = "Unknown";
+ if (_playerManager.TryGetSessionById(info.SessionId, out var session))
+ {
+ serverId = AuthServer.GetServerFromCVarListByUrl(_config, session.Channel.UserData.AuthServer)?.Id ?? "Unknown";
+ }
+ var displayName = $"{info.CharacterName} ({info.Username}@{serverId})";🤖 Prompt for AI Agents
In @Content.Client/Administration/UI/CustomControls/PlayerListControl.xaml.cs
around lines 112 - 115, The displayName construction can throw a
NullReferenceException because
_playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer may be
null; fix by retrieving the session into a local (e.g., var session =
_playerManager.GetSessionById(info.SessionId)), then null-check session,
session.Channel, session.Channel.UserData and
session.Channel.UserData.AuthServer before accessing Id (use conditional access
or explicit checks) and fall back to a safe string like "unknown" when any piece
is null; update the displayName expression in PlayerListControl.xaml.cs to use
that guarded value (mirror the null-safe pattern used in
PlayerListEntry.xaml.cs).
Comment on lines
+51
to
+56
| // WD EDIT START | ||
| PlayerEntryLabel.Text = overrideText?.Invoke( | ||
| info, | ||
| $"{info.CharacterName} ({info.Username}@{AuthServer.GetServerFromCVarListByUrl(_config, | ||
| _playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer)?.Id})") ?? $"{info.CharacterName} ({info.Username})"; | ||
| // WD EDIT END |
Contributor
There was a problem hiding this comment.
Потенциальное NullReferenceException при получении сессии.
Цепочка вызовов _playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer может выбросить исключение, если сессия не найдена или отсутствует.
Рассмотрите добавление null-проверок или использование TryGetSessionById:
🛠️ Предлагаемое исправление
- PlayerEntryLabel.Text = overrideText?.Invoke(
- info,
- $"{info.CharacterName} ({info.Username}@{AuthServer.GetServerFromCVarListByUrl(_config,
- _playerManager.GetSessionById(info.SessionId).Channel.UserData.AuthServer)?.Id})") ?? $"{info.CharacterName} ({info.Username})";
+ var serverId = "Unknown";
+ if (_playerManager.TryGetSessionById(info.SessionId, out var session))
+ {
+ serverId = AuthServer.GetServerFromCVarListByUrl(_config, session.Channel.UserData.AuthServer)?.Id ?? "Unknown";
+ }
+ var displayName = $"{info.CharacterName} ({info.Username}@{serverId})";
+ PlayerEntryLabel.Text = overrideText?.Invoke(info, displayName) ?? $"{info.CharacterName} ({info.Username})";|
|
||
| // If all else fails, ask the auth server. | ||
| var authServer = _configurationManager.GetCVar(CVars.AuthServer); | ||
| var authServer = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl; // WD EDIT |
Contributor
There was a problem hiding this comment.
Потенциальное исключение при пустом списке серверов аутентификации.
Вызов .First() на пустой последовательности выбросит InvalidOperationException. Если CVars.AuthServers пуст или не содержит валидных записей, это приведёт к сбою.
🛠️ Предлагаемое исправление
- var authServer = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl; // WD EDIT
+ var authServers = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers));
+ var authServer = authServers.FirstOrDefault()?.AuthUrl
+ ?? throw new InvalidOperationException("No auth servers configured in CVars.AuthServers"); // WD EDIT🤖 Prompt for AI Agents
In @Content.Server/Administration/PlayerLocator.cs at line 105,
AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First()
can throw InvalidOperationException for an empty/invalid list; change the call
to safely handle empties (e.g., use FirstOrDefault/null-check or check the
returned collection count) and then handle the missing server case before
accessing AuthUrl — update the usage in the method that currently reads
AuthServer.FromStringList(...).First().AuthUrl to use a safe retrieval
(FirstOrDefault), verify the result is non-null (or collection non-empty) and
provide a fallback or clear error/log message if no auth server is available.
|
|
||
| // If all else fails, ask the auth server. | ||
| var authServer = _configurationManager.GetCVar(CVars.AuthServer); | ||
| var authServer = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl; // WD EDIT |
Contributor
There was a problem hiding this comment.
Дублирование проблемы с .First() — то же самое исключение возможно здесь.
Аналогичная проблема с вызовом .First() без проверки на пустую последовательность. Рассмотрите вынос логики получения auth server URL в отдельный приватный метод для устранения дублирования.
♻️ Предлагаемый рефакторинг — вынести в отдельный метод
+ private string GetPrimaryAuthServerUrl()
+ {
+ var authServers = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers));
+ return authServers.FirstOrDefault()?.AuthUrl
+ ?? throw new InvalidOperationException("No auth servers configured");
+ }Затем использовать:
- var authServer = AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl; // WD EDIT
+ var authServer = GetPrimaryAuthServerUrl(); // WD EDIT🤖 Prompt for AI Agents
In @Content.Server/Administration/PlayerLocator.cs at line 124, The code calls
AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)).First().AuthUrl
which can throw on an empty sequence and duplicates this logic; extract this
into a private helper (e.g. GetAuthServerUrl or ResolveAuthServerUrl) that calls
AuthServer.FromStringList, checks for empty/null (use
FirstOrDefault/SingleOrDefault), returns AuthUrl or handles the missing case
(log and throw a clear exception or return null/Optional) and replace all direct
.First().AuthUrl usages with this helper to remove duplication and avoid the
exception.
Comment on lines
+253
to
+264
| // WD EDDIT START | ||
| var authServer = player.AuthType == LoginType.LoggedIn | ||
| ? AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)) | ||
| .FirstOrDefault(x => x.AuthUrl.ToString() == player.Channel.UserData.AuthServer) | ||
| ?.Id ?? "Unknown" | ||
| : "Unknown"; | ||
| var wrappedMessage = Loc.GetString( | ||
| "chat-manager-send-ooc-wrap-message", | ||
| ("playerName", $"{player.Name}"), | ||
| ("authServer", authServer), | ||
| ("message", FormattedMessage.EscapeText(message))); | ||
| // WD EDIT END |
Contributor
There was a problem hiding this comment.
Логика определения authServer корректна, но есть опечатка в комментарии.
Реализация с использованием FirstOrDefault и ?? "Unknown" обеспечивает правильную обработку случаев, когда сервер не найден. Однако, на строке 253 опечатка: WD EDDIT вместо WD EDIT.
🔧 Исправление опечатки
- // WD EDDIT START
+ // WD EDIT START📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // WD EDDIT START | |
| var authServer = player.AuthType == LoginType.LoggedIn | |
| ? AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)) | |
| .FirstOrDefault(x => x.AuthUrl.ToString() == player.Channel.UserData.AuthServer) | |
| ?.Id ?? "Unknown" | |
| : "Unknown"; | |
| var wrappedMessage = Loc.GetString( | |
| "chat-manager-send-ooc-wrap-message", | |
| ("playerName", $"{player.Name}"), | |
| ("authServer", authServer), | |
| ("message", FormattedMessage.EscapeText(message))); | |
| // WD EDIT END | |
| // WD EDIT START | |
| var authServer = player.AuthType == LoginType.LoggedIn | |
| ? AuthServer.FromStringList(_configurationManager.GetCVar(CVars.AuthServers)) | |
| .FirstOrDefault(x => x.AuthUrl.ToString() == player.Channel.UserData.AuthServer) | |
| ?.Id ?? "Unknown" | |
| : "Unknown"; | |
| var wrappedMessage = Loc.GetString( | |
| "chat-manager-send-ooc-wrap-message", | |
| ("playerName", $"{player.Name}"), | |
| ("authServer", authServer), | |
| ("message", FormattedMessage.EscapeText(message))); | |
| // WD EDIT END |
🤖 Prompt for AI Agents
In @Content.Server/Chat/Managers/ChatManager.cs around lines 253 - 264, The
inline block comments around the authServer logic contain a typo: change the
opening marker "WD EDDIT START" to "WD EDIT START" (matching the closing "WD
EDIT END") so comment markers are consistent; update only the comment text
surrounding the AuthServer resolution/Loc.GetString call (the lines containing
"WD EDDIT START" and "WD EDIT END").
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Описание PR
Я не знаю как тут будут работать баны. Проверим на ходу.
Изменения
🆑