-
-
Notifications
You must be signed in to change notification settings - Fork 40
⭐ feat: Add Steam authentication and game querying #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 73 out of 80 changed files in this pull request and generated no comments.
Files not reviewed (7)
- nestri.sln: Language not supported
- package.json: Language not supported
- packages/core/migrations/0002_simple_outlaw_kid.sql: Language not supported
- packages/core/migrations/meta/_journal.json: Language not supported
- packages/core/package.json: Language not supported
- packages/functions/Containerfile: Language not supported
- packages/functions/package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/core/src/team/index.ts:126
- Consider adding tests to verify the behavior when the query returns an empty array to ensure that .at(0) handles edge cases as expected.
.then((rows) => rows.map(serialize).at(0))
packages/core/src/member/index.ts:101
- Consider adding tests for scenarios where the query returns an empty array to ensure the usage of .at(0) produces the intended result.
.then((rows) => rows.map(serialize).at(0))
WalkthroughThis pull request introduces extensive refactoring across the codebase. Major components of the infrastructure, API, and authentication services have been restructured from function‑based implementations to containerized, service‑oriented architectures. Various modules have been transitioned from module to namespace exports, and new database migrations have been added or modified to update table schemas and indices. Enhancements include improved error handling, updated dependencies, new container and TS configuration files, and enriched client-side assets for animations and real‑time interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server as API Server
participant Logger
participant CORS
participant RealtimeRouter as Realtime Router
participant SteamRouter as Steam Router
Client->>API_Server: Send HTTP Request
API_Server->>Logger: Log request details
API_Server->>CORS: Process CORS checks
CORS-->>API_Server: Forward request
alt Request path is /realtime
API_Server->>RealtimeRouter: Forward to realtime handler
RealtimeRouter-->>API_Server: Return realtime response
else Request path is /steam
API_Server->>SteamRouter: Forward to Steam handler
SteamRouter-->>API_Server: Return Steam response
end
API_Server->>Client: Return consolidated response
sequenceDiagram
participant User
participant SteamClass as Steam
participant UnixSocket
Note over SteamClass: Handles UNIX socket connection
User->>SteamClass: Call login()
SteamClass->>UnixSocket: Open connection
UnixSocket-->>SteamClass: Connection established
SteamClass->>SteamClass: Process login events (auth_required, challenge, success, error, etc.)
SteamClass->>User: Return login result and account info
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
packages/www/src/providers/steam.tsx (1)
221-257
: 🛠️ Refactor suggestionRefine connection cleanup approach.
The current strategy keeps the connection alive if other components might still use it, but the global state doesn’t track usage counts. Consider implementing a reference-counted disconnection or an explicit shared context to avoid leaving the stream open unintentionally.
🧹 Nitpick comments (44)
packages/www/src/pages/new.tsx (1)
170-195
: Duplicate Autofocus Attributes on Input ElementsThere are two
<Input>
elements (one for "Team Name" and one for "Team Slug") that both include theautofocus
attribute. Typically, only one element per page should have autofocus to avoid unpredictable behavior on load. Please review whether both fields truly need autofocus or if one should be removed.packages/www/src/pages/not-found.tsx (1)
36-38
: Clarify Nested Ternary ininset
PropThe
inset
prop for the<FullScreen>
component is defined using a nested ternary operator:inset={props.inset ? props.inset : props.header ? "header" : "none"}While functionally correct, the nested ternary can be hard to read. Consider refactoring by adding parentheses or extracting the logic into a helper variable for better clarity and maintainability.
packages/functions/Containerfile (1)
1-15
: Containerization looks good with some potential improvementsThe Containerfile effectively containerizes the functions package with good layer caching practices by adding package files before full source code.
Consider these potential improvements:
- Add an
EXPOSE
instruction to document which port the API listens on- Consider adding a
HEALTHCHECK
instruction for better container health monitoring- You might benefit from using a multi-stage build to reduce the final image size
packages/core/src/actor.ts (2)
63-71
: New helper functionuseUser
is concise.This mirrors the
useUserID
logic effectively. Consider adding unit tests to ensure coverage for all actor types.
88-101
: More specific error classification is good.Using
"validation"
andMISSING_REQUIRED_FIELD
clarifies that the missing flag is a validation issue. However, if a user row truly doesn't exist, an alternative error code (NotFound
or similar) might be more accurate.infra/auth.ts (2)
9-13
: Commented-out snippet forsst.Linkable.wrap
.If no longer needed, consider removing entirely to keep the codebase clean.
15-21
: Unused random string generation is commented out.Remove it if it is no longer required. Unused code can cause confusion later.
packages/www/src/providers/steam.tsx (2)
60-77
: Augment waiting logic with error handling.
Waiting onisConnecting
in a loop could hang in rare cases (e.g., if a connection attempt silently fails). Consider adding a timeout and clearing the flag on errors to robustly handle abnormal connection states.
121-133
: Safeguard all listener notifications.
If one callback innotifyListeners
throws an exception, it won’t stop subsequent listeners, but it might flood your console with errors. Ensure that critical exceptions are handled or logged in a structured manner for clearer diagnostics.packages/core/src/steam/steam.sql.ts (2)
12-12
: Consider addressing the commented-out fieldThere's a commented-out
steamEmail
field. If this is no longer needed, consider removing the comment. If it's planned for future implementation, add a TODO comment explaining the purpose and timeline.- // steamEmail: varchar("steam_email", { length: 255 }).notNull(), + // TODO: Add steamEmail field when Steam provides separate login emails + // steamEmail: varchar("steam_email", { length: 255 }).notNull(),
21-22
: Review email indexing strategyThe schema creates both a regular index on email and a unique composite index on [userID, email]. Consider if both are necessary:
- The regular index (
global_steam_email
) supports queries filtering by email only- The unique index (
steam_email
) ensures email uniqueness per user and supports queries filtering by both userID and emailIf you rarely query by email alone, you might consider removing the regular index to reduce index maintenance overhead.
packages/www/src/common/portal/index.tsx (6)
27-28
: Remove commented-out code.The commented-out backgroundColor properties should be removed since they're not being used.
position: "absolute", inset: 0, - // backgroundColor:"red", height: "100%",
position: "relative", inset: 0, - // backgroundColor:"red", zIndex: 1,
Also applies to: 38-39
48-48
: Remove unused commented-out code.This commented-out signal declaration is not being used and should be removed.
const [iconRef, setIconRef] = createSignal<HTMLCanvasElement | undefined>(); const [buttonRef, setButtonRef] = createSignal<HTMLCanvasElement | undefined>(); - // const [imagesLoaded, setImagesLoaded] = createSignal(false);
79-79
: Improve array destructuring clarity.The array destructuring has a skipped element which makes the code harder to understand. Consider using a more explicit approach or adding a comment explaining what's being skipped.
- const [introImg, idleImg, exitImg, , loopImg] = await loadImages(); + // Destructure images for each animation type - skipping introIconImg at index 3 + const [introImg, idleImg, exitImg, , loopImg] = await loadImages();Alternatively, use object destructuring or named variables:
const images = await loadImages(); const introImg = images[0]; const idleImg = images[1]; const exitImg = images[2]; const loopImg = images[4];
83-83
: Remove commented-out code.This commented-out condition check should be removed since it's not being used.
const button = new PortalButton(btnRef); const icon = new PortalIcon(icnRef) - // if (!isMounted) return;
58-70
: Add error handling for the entire image loading process.The
loadImages
function has error handling for individual images but there's no try/catch around thePromise.all
call. Consider adding comprehensive error handling.const loadImages = () => { - return Promise.all(imageUrls.map(url => { + return Promise.all(imageUrls.map((url, index) => { return new Promise((resolve, reject) => { const img = new Image(); img.onload = () => resolve(img); img.onerror = (e) => { - console.error(`Failed to load image from ${url}:`, e); + console.error(`Failed to load image ${index} from ${url}:`, e); reject(new Error(`Failed to load image from ${url}`)); }; img.src = url; }); })); }And in the
createEffect
:createEffect(() => { (async () => { const btnRef = buttonRef() const icnRef = iconRef() if (icnRef && btnRef) { + try { const [introImg, idleImg, exitImg, , loopImg] = await loadImages(); // Rest of the code... + } catch (error) { + console.error("Failed to load animation images:", error); + // Fallback rendering or error handling + } } })() });
72-102
: Add cleanup logic for animations.There's no cleanup logic when the component is unmounted, which could lead to memory leaks if animations are still running. Consider adding an
onCleanup
handler.import { createEffect, createSignal, onCleanup } from "solid-js"; // ... createEffect(() => { + let isActive = true; (async () => { const btnRef = buttonRef() const icnRef = iconRef() if (icnRef && btnRef) { const [introImg, idleImg, exitImg, , loopImg] = await loadImages(); const button = new PortalButton(btnRef); const icon = new PortalIcon(icnRef) + + if (!isActive) return; // Stop if component has been unmounted // Rest of your animation code } })() + + onCleanup(() => { + isActive = false; + // Additional cleanup if needed + }); });packages/functions/src/api/steam/Program.cs (2)
86-91
: Consider more graceful disconnection handlingThe disconnection callback immediately sets
isRunning = false
, which terminates the application. Consider adding cleanup logic to ensure resources are properly disposed of.void OnDisconnected(SteamClient.DisconnectedCallback callback) { Console.WriteLine("Disconnected from Steam"); + + // Perform any necessary cleanup + userData = new UserData { Success = false }; + Console.WriteLine(JsonSerializer.Serialize(userData)); isRunning = false; }
159-168
: Add null annotations to UserData propertiesThe UserData class should include nullable annotations for properties that can be null to help prevent null reference exceptions.
class UserData { public bool Success { get; set; } - public string Username { get; set; } - public string AccessToken { get; set; } - public string PersonaName { get; set; } - public string Country { get; set; } - public string AvatarUrl { get; set; } + public string? Username { get; set; } + public string? AccessToken { get; set; } + public string? PersonaName { get; set; } + public string? Country { get; set; } + public string? AvatarUrl { get; set; } public DateTime Timestamp { get; set; } }packages/www/src/components/qr-code.tsx (2)
243-248
: Enhance error handling in Steam connection event listenersThe event listener error handling could be improved to provide more detailed error information and recovery options.
const loginUnsuccessfulUnsubscribe = steamConnection.addEventListener('error', (data) => { - setLoginError(true); + console.error("Steam authentication error:", data); + setLoginError(true); + // Optionally retry connection after delay for transient errors + // if (data.retryable) { + // setTimeout(() => reconnect(), 5000); + // } });
307-339
: Add accessibility improvements to QR code UIThe QR code container and related elements should have improved accessibility attributes.
<QRContainer login={typeof loginUrl() === "string" && !loginError()}> <QRBg /> - <QRWrapper> + <QRWrapper aria-live="polite" role="region" aria-label="Steam QR Code Authentication"> <LogoContainer> <LogoIcon xmlns="http://www.w3.org/2000/svg" width={!loginError() && loginUrl() ? 32 : 60} height={!loginError() && loginUrl() ? 32 : 60} viewBox="0 0 16 16"> <g fill="currentColor"> <path d="M.329 10.333A8.01 8.01 0 0 0 7.99 16C12.414 16 16 12.418 16 8s-3.586-8-8.009-8A8.006 8.006 0 0 0 0 7.468l.003.006l4.304 1.769A2.2 2.2 0 0 1 5.62 8.88l1.96-2.844l-.001-.04a3.046 3.046 0 0 1 3.042-3.043a3.046 3.046 0 0 1 3.042 3.043a3.047 3.047 0 0 1-3.111 3.044l-2.804 2a2.223 2.223 0 0 1-3.075 2.11a2.22 2.22 0 0 1-1.312-1.568L.33 10.333Z" /><path d="M4.868 12.683a1.715 1.715 0 0 0 1.318-3.165a1.7 1.7 0 0 0-1.263-.02l1.023.424a1.261 1.261 0 1 1-.97 2.33l-.99-.41a1.7 1.7 0 0 0 .882.84Zm3.726-6.687a2.03 2.03 0 0 0 2.027 2.029a2.03 2.03 0 0 0 2.027-2.029a2.03 2.03 0 0 0-2.027-2.027a2.03 2.03 0 0 0-2.027 2.027m2.03-1.527a1.524 1.524 0 1 1-.002 3.048a1.524 1.524 0 0 1 .002-3.048" /> </g> </LogoIcon> </LogoContainer> <!-- Rest of the code --> </QRWrapper> - <Show when={loginError()}> - <QRReloadBtn onClick={() => reconnect()} disabled={isConnecting()}> + <Show when={loginError()}> + <QRReloadBtn + onClick={() => reconnect()} + disabled={isConnecting()} + aria-label="Retry Steam connection" + title="Retry connection"> <QRRealoadContainer> <QRReloadSvg aria-hidden="true" width="32" height="32" viewBox="0 0 32 32" fill="currentColor" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" clip-rule="evenodd" d="M32 16C32 24.8366 24.8366 32 16 32C7.16344 32 0 24.8366 0 16C0 7.16344 7.16344 0 16 0C24.8366 0 32 7.16344 32 16ZM24.5001 8.74263C25.0834 8.74263 25.5563 9.21551 25.5563 9.79883V14.5997C25.5563 15.183 25.0834 15.6559 24.5001 15.6559H19.6992C19.1159 15.6559 18.643 15.183 18.643 14.5997C18.643 14.0164 19.1159 13.5435 19.6992 13.5435H21.8378L20.071 11.8798C20.0632 11.8724 20.0555 11.865 20.048 11.8574C19.1061 10.915 17.8835 10.3042 16.5643 10.1171C15.2452 9.92999 13.9009 10.1767 12.7341 10.82C11.5674 11.4634 10.6413 12.4685 10.0955 13.684C9.54968 14.8994 9.41368 16.2593 9.70801 17.5588C10.0023 18.8583 10.711 20.0269 11.7273 20.8885C12.7436 21.7502 14.0124 22.2582 15.3425 22.336C16.6726 22.4138 17.9919 22.0572 19.1017 21.3199C19.5088 21.0495 19.8795 20.7333 20.2078 20.3793C20.6043 19.9515 21.2726 19.9262 21.7004 20.3228C22.1282 20.7194 22.1534 21.3876 21.7569 21.8154C21.3158 22.2912 20.8176 22.7161 20.2706 23.0795C18.7793 24.0702 17.0064 24.5493 15.2191 24.4448C13.4318 24.3402 11.7268 23.6576 10.3612 22.4998C8.9956 21.3419 8.0433 19.7716 7.6478 18.0254C7.2523 16.2793 7.43504 14.4519 8.16848 12.8186C8.90192 11.1854 10.1463 9.83471 11.7142 8.97021C13.282 8.10572 15.0884 7.77421 16.861 8.02565C18.6282 8.27631 20.2664 9.09278 21.5304 10.3525L23.4439 12.1544V9.79883C23.4439 9.21551 23.9168 8.74263 24.5001 8.74263Z" fill="currentColor"></path></QRReloadSvg> </QRRealoadContainer> </QRReloadBtn> </Show> </QRContainer>packages/functions/src/api/index.ts (1)
16-18
: Consider the logging order with CORS middleware.
By calling.use(cors())
before.use(logger())
, requests that fail CORS checks might be omitted from logs. If logging these attempts is beneficial, reorder them so the logger runs first.packages/functions/src/api/steam/index.ts (2)
13-13
: Prefer usingnamespace
overmodule
.
Static analysis flagsexport module SteamApi
as deprecated. Consider switching toexport namespace SteamApi
for clarity and ECMAScript compatibility.- export module SteamApi { + export namespace SteamApi {🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
70-72
: Handle the'error'
event more explicitly.
Currently, the'error'
listener is empty, so important errors could be lost. Consider logging or re-throwing them to ensure visibility and consistent error handling.steam.on('error', async (error) => { - + console.error('Authentication error:', error.message ?? error); + // Possibly re-throw or handle the error for consistent error reporting });packages/core/migrations/0002_simple_outlaw_kid.sql (1)
1-14
: Revisit use of fixed char(30) and handle deletion logic.
Storing IDs as fixed length char(30) may be overly restrictive or lead to trailing spaces. Also, consider how thetime_deleted
column is used in combination with the primary key. If rows are never physically deleted, ensure the application logic properly checkstime_deleted
or use a partial index for “active” records.packages/core/src/steam/index.ts (3)
10-10
: Replaceexport module
withexport namespace
.
As flagged by static analysis,module
is deprecated in favor ofnamespace
. This improves clarity and avoids confusion with ES modules.-export module Steam { +export namespace Steam {🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.(lint/suspicious/useNamespaceKeyword)
60-71
: Ensure secure handling ofaccessToken
.
TheaccessToken
is directly inserted into the table. Consider encrypting or hashing sensitive tokens before storing them, depending on usage.
86-98
: Improve function naming for consistency.
serialize
effectively transforms DB rows intoSteam.Info
. Renaming it totoSteamInfo
or a more descriptive name could clarify intent and usage.packages/www/src/pages/team/home.tsx (11)
6-6
: Check usage of newly imported SolidJS functions
You importedcreateEffect
,createSignal
,Match
,onCleanup
, andSwitch
but there's no usage below. If they're not needed, consider removing them to keep the codebase clean.
130-131
: z-index usage and isolation property
SettingzIndex
to 10 andisolation
to"isolate"
is valid. Ensure cross-browser compatibility if older browsers matter.
228-249
: Potential naming collision between SubTitle and SteamGameSubTitle
Although functional, these similar names may cause confusion later. Consider more descriptive or unified naming.
265-279
: Use of FriendContainer to unify styling
The design pattern is good, but consider making this container a reusable component for other social features.
296-315
: FriendsInviteButton introduced
Consider adding or passing an onClick handler to handle invite logic in the future.
355-378
: SteamGameBtn usage
Styling is consistent with your button design language. Consider hooking up actual install/update functionality next.
391-407
: Header wrapper and QrCodeComponent integration
Good to see the new QR code-based auth in place. Remove the commented-out remnants if no longer needed.
408-414
: Block for GamesContainer and GameSquareImage
Hardcoded images suit a demo, but consider loading from an API or config for production readiness.
415-448
: Team Mate Suggestions
Mapping a dynamic list of friends rather than repeating FriendContainer instances can reduce duplication.
505-505
: Unclear comment block
It looks like the comment for GamesContainer might be mismatched or outdated. Check if it's still necessary.
510-519
: Commented-out code with GameImageCapsule
If you don’t plan to reuse these references, consider removing them to keep the file tidy.packages/functions/src/api/steam/steamAuth.ts (5)
5-12
: SteamCredentials interface
All essential fields are included. However, using[key: string]: any
may hide type errors. Remove or narrow it if possible.
28-45
: Steam class constructor
Taking in the path to the C# executable and a timeout is logical. Consider validating the path or falling back safely if not found.
59-78
: ensureExecutable method
chmod +x
is fine for Unix-like systems but might fail on Windows. You could skip or conditionally handle cross-platform setups.
175-181
: cancel method
Completing the process forcibly is straightforward. Consider emitting a specific “cancelled” event for improved clarity.
193-206
: completeAuth
Emitting a “completed” event with the final result is a clean design. Consider distinguishing normal completion from error completion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
bun.lock
is excluded by!**/*.lock
packages/www/src/assets/portal/play_button_disabled_bg.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_button_focused_bg.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_button_idle.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_button_intro.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_icon_exit.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_icon_intro.png
is excluded by!**/*.png
packages/www/src/assets/portal/play_icon_loop.png
is excluded by!**/*.png
packages/www/src/assets/portal/portal_background_placeholder.png
is excluded by!**/*.png
📒 Files selected for processing (70)
infra/api.ts
(1 hunks)infra/auth.ts
(1 hunks)infra/postgres.ts
(1 hunks)infra/steam.ts
(0 hunks)infra/www.ts
(0 hunks)nestri.sln
(1 hunks)package.json
(1 hunks)packages/core/migrations/0002_simple_outlaw_kid.sql
(1 hunks)packages/core/migrations/meta/0002_snapshot.json
(1 hunks)packages/core/migrations/meta/_journal.json
(1 hunks)packages/core/package.json
(1 hunks)packages/core/src/actor.ts
(3 hunks)packages/core/src/drizzle/types.ts
(1 hunks)packages/core/src/examples.ts
(1 hunks)packages/core/src/member/index.ts
(1 hunks)packages/core/src/steam/index.ts
(1 hunks)packages/core/src/steam/steam.sql.ts
(1 hunks)packages/core/src/team/index.ts
(2 hunks)packages/core/src/utils/id.ts
(1 hunks)packages/functions/Containerfile
(1 hunks)packages/functions/package.json
(2 hunks)packages/functions/src/api/auth.ts
(3 hunks)packages/functions/src/api/index.ts
(3 hunks)packages/functions/src/api/steam/Program.cs
(1 hunks)packages/functions/src/api/steam/index.ts
(1 hunks)packages/functions/src/api/steam/steam.csproj
(1 hunks)packages/functions/src/api/steam/steamAuth.ts
(1 hunks)packages/functions/src/auth.ts
(4 hunks)packages/functions/src/log-polyfill.ts
(1 hunks)packages/functions/src/party/authorizer.ts
(3 hunks)packages/functions/sst-env.d.ts
(1 hunks)packages/steam/DBContext.cs
(0 hunks)packages/steam/DBSchema.cs
(0 hunks)packages/steam/Migrations/20250322023207_InitialCreate.Designer.cs
(0 hunks)packages/steam/Migrations/20250322023207_InitialCreate.cs
(0 hunks)packages/steam/Migrations/AppDbContextModelSnapshot.cs
(0 hunks)packages/steam/Program.cs
(0 hunks)packages/steam/Properties/launchSettings.json
(0 hunks)packages/steam/SSEEvents.cs
(0 hunks)packages/steam/SteamClient.cs
(0 hunks)packages/steam/SteamService.cs
(0 hunks)packages/steam/appsettings.Development.json
(0 hunks)packages/steam/appsettings.json
(0 hunks)packages/steam/steam.csproj
(0 hunks)packages/www/package.json
(1 hunks)packages/www/src/assets/portal/play_button_idle.json
(1 hunks)packages/www/src/assets/portal/play_button_intro.json
(1 hunks)packages/www/src/assets/portal/play_icon_exit.json
(1 hunks)packages/www/src/assets/portal/play_icon_intro.json
(1 hunks)packages/www/src/assets/portal/play_icon_loop.json
(1 hunks)packages/www/src/common/portal/assets/play_button_idle.json
(1 hunks)packages/www/src/common/portal/assets/play_button_intro.json
(1 hunks)packages/www/src/common/portal/assets/play_icon_exit.json
(1 hunks)packages/www/src/common/portal/assets/play_icon_intro.json
(1 hunks)packages/www/src/common/portal/assets/play_icon_loop.json
(1 hunks)packages/www/src/common/portal/button.ts
(1 hunks)packages/www/src/common/portal/index.tsx
(1 hunks)packages/www/src/components/index.ts
(1 hunks)packages/www/src/components/qr-code.tsx
(1 hunks)packages/www/src/pages/new.tsx
(1 hunks)packages/www/src/pages/not-found.tsx
(1 hunks)packages/www/src/pages/team/header.tsx
(6 hunks)packages/www/src/pages/team/home.tsx
(4 hunks)packages/www/src/providers/api.tsx
(1 hunks)packages/www/src/providers/steam.tsx
(6 hunks)packages/www/src/sst-env.d.ts
(0 hunks)packages/www/src/ui/custom-qr.tsx
(1 hunks)packages/www/vite.config.ts
(0 hunks)sst-env.d.ts
(1 hunks)sst.config.ts
(2 hunks)
💤 Files with no reviewable changes (17)
- infra/www.ts
- packages/www/vite.config.ts
- packages/steam/DBContext.cs
- packages/steam/appsettings.Development.json
- packages/steam/appsettings.json
- packages/steam/steam.csproj
- packages/www/src/sst-env.d.ts
- packages/steam/DBSchema.cs
- packages/steam/Migrations/20250322023207_InitialCreate.Designer.cs
- packages/steam/Migrations/AppDbContextModelSnapshot.cs
- packages/steam/Migrations/20250322023207_InitialCreate.cs
- packages/steam/SteamService.cs
- packages/steam/Program.cs
- packages/steam/SSEEvents.cs
- infra/steam.ts
- packages/steam/SteamClient.cs
- packages/steam/Properties/launchSettings.json
🧰 Additional context used
🧬 Code Definitions (12)
packages/core/src/examples.ts (1)
packages/functions/src/api/steam/steamAuth.ts (1)
Steam
(28-227)
packages/core/src/steam/steam.sql.ts (1)
packages/core/src/drizzle/types.ts (3)
userID
(20-27)userID
(24-26)timestamps
(35-39)
packages/core/src/member/index.ts (4)
packages/core/src/team/index.ts (2)
serialize
(141-150)fromID
(119-128)packages/core/src/steam/index.ts (1)
serialize
(86-98)packages/core/src/user/index.ts (2)
serialize
(173-184)fromID
(161-171)packages/core/src/member/member.sql.ts (1)
memberTable
(5-18)
packages/www/src/common/portal/index.tsx (1)
packages/www/src/common/portal/button.ts (2)
PortalButton
(33-167)PortalIcon
(169-306)
packages/core/src/team/index.ts (3)
packages/core/src/user/index.ts (1)
serialize
(173-184)packages/core/src/member/index.ts (1)
serialize
(116-125)packages/core/src/steam/index.ts (1)
serialize
(86-98)
packages/functions/src/party/authorizer.ts (2)
packages/functions/sst-env.d.ts (1)
Resource
(8-85)sst-env.d.ts (1)
Resource
(8-85)
packages/core/src/actor.ts (1)
packages/core/src/error.ts (2)
ErrorCodes
(48-92)VisibleError
(97-145)
infra/auth.ts (3)
packages/functions/src/api/auth.ts (1)
auth
(24-64)infra/bus.ts (1)
bus
(6-6)infra/postgres.ts (1)
postgres
(5-40)
packages/functions/src/api/steam/index.ts (2)
packages/functions/src/api/auth.ts (1)
notPublic
(13-22)packages/functions/src/api/steam/steamAuth.ts (1)
Steam
(28-227)
packages/www/src/components/qr-code.tsx (2)
packages/functions/src/api/steam/steamAuth.ts (1)
cleanup
(212-226)packages/www/src/ui/custom-qr.tsx (1)
QRCode
(31-159)
infra/api.ts (4)
infra/bus.ts (1)
bus
(6-6)infra/auth.ts (1)
auth
(24-82)packages/functions/src/api/auth.ts (1)
auth
(24-64)infra/postgres.ts (1)
postgres
(5-40)
packages/functions/src/api/steam/steamAuth.ts (1)
packages/core/src/examples.ts (1)
Steam
(34-42)
🪛 Biome (1.9.4)
packages/functions/src/api/steam/index.ts
[error] 13-13: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
packages/core/src/steam/index.ts
[error] 10-10: Use the namespace keyword instead of the outdated module keyword.
The module keyword is deprecated to avoid any confusion with the ECMAScript modules which are often called modules.
Safe fix: Use namespace instead.
(lint/suspicious/useNamespaceKeyword)
🔇 Additional comments (95)
packages/www/src/pages/new.tsx (2)
11-11
: Updated Import: RenamingScreen
asFullScreen
The revised import on line 11, which now reads:
import { Container, Screen as FullScreen } from "@nestri/www/ui/layout";is correctly applied. This change ensures that the component is referred to consistently as
FullScreen
throughout the file. Please verify that all usages of this component in the codebase follow the same naming convention.
123-251
: Missing Play Button Element in Homepage LogicThe PR objectives mention adding a play button to the homepage, yet this file (which appears to be a "Create Team" page) does not include any new element corresponding to a play button. Please confirm whether the play button logic is implemented in another file or if additional changes are expected in the homepage component.
packages/www/src/pages/not-found.tsx (1)
7-7
: Consistent Update of Layout ImportThe update on line 7:
import { Screen as FullScreen, Container } from "@nestri/www/ui/layout";is consistent with similar changes in other parts of the project. This helps maintain uniformity in component naming and usage across the codebase.
packages/www/package.json (1)
42-42
: New "motion" Dependency Added.
The addition of"motion": "^12.6.2"
enhances UI animation capabilities on the homepage and aligns with the new interactive feature (e.g., the play button). Please ensure that its usage is documented and that there are no version conflicts with existing animation or UI libraries.packages/core/package.json (1)
7-7
: Added "db:dev" Script for Database Operations.
The new script"db:dev": "drizzle-kit"
facilitates local database development tasks. It would be beneficial to update documentation or scripts comments to explain its usage for future maintainers.packages/functions/package.json (1)
7-10
: Introduced Dev Scripts for Auth and API.
The added scripts"dev:auth": "bun run --watch ./src/auth.ts"
and"dev:api": "bun run --watch ./src/api/index.ts"
streamline local development for the authentication and API functionalities. Ensure that the referenced file paths are accurate and that any environment-specific configurations are consistently maintained across the project.package.json (1)
39-39
: SST Dependency Version Updated.
Updating"sst"
from a fixed version to"^3.11.21"
allows minor and patch upgrades to be automatically incorporated, which is beneficial given the migration toward a containerized, service-oriented architecture. Please verify that this change does not introduce compatibility issues with the updated API and authentication service configurations.packages/www/src/providers/api.tsx (1)
8-9
: Context Identifier Renamed to "ApiContext".
Changing the argument from"Api"
to"ApiContext"
in thecreateInitializedContext
call improves clarity and ensures consistency with the new service-oriented architecture. Make sure all components and hooks consuming this context are updated accordingly.packages/core/src/drizzle/types.ts (1)
20-27
: LGTM: Clean addition of userID exportThe addition of the
userID
export follows the established pattern in this file, similar to howteamID
is implemented. This provides a consistent way to define user ID fields in database tables.packages/functions/src/api/steam/steam.csproj (1)
1-12
: Clean Steam integration project setupThe new Steam project is well-configured with .NET 8.0 and the SteamKit2 library. This appears to be part of a larger transition to a service-oriented architecture, moving Steam functionality to a dedicated service.
packages/core/src/utils/id.ts (1)
6-7
: LGTM: Added Steam ID prefixThe addition of the "steam" prefix with value "stm" follows the established naming pattern in the file (3-letter abbreviations). This enables generating Steam-related IDs with the
createID
function.sst.config.ts (2)
22-22
: Dynamic import of fs module improves modularityThis change transitions from a static import to a dynamic one, which allows for lazy loading of the module. This is a good practice for modules that might not be needed immediately on startup.
34-34
: Updated fs module referenceUpdated the readdirSync call to use the dynamically imported fs module. This change maintains consistency with the import change above.
packages/www/src/assets/portal/play_icon_exit.json (1)
1-237
: LGTM: Well-structured sprite animation configurationThis sprite sheet configuration for the play button exit animation follows industry standards for frame animations. The JSON structure properly defines frames with appropriate positioning, dimensions, and transformation properties.
This file works in conjunction with the other animation assets to create a cohesive play button interactive element as mentioned in the PR objectives.
packages/www/src/common/portal/assets/play_icon_loop.json (1)
1-237
: LGTM: Properly configured animation loop assetThis sprite sheet configuration for the play button loop animation is well-structured with appropriate frame definitions. The loop animation complements the exit animation to create a complete interactive experience.
The metadata indicates this was created with TexturePacker, which is a standard tool for sprite sheet generation. The smart update hash ensures integrity of the asset.
packages/functions/src/party/authorizer.ts (3)
13-13
: Updated Auth URL reference to match new Resource structureThe change from
Resource.Urls.auth
toResource.Auth.url
aligns with the updated Resource interface structure shown in the relevant code snippets. This update is consistent with the architectural changes mentioned in the summary.
26-26
: Authentication logic refined to be user-specificChanged the authorization condition from excluding devices to specifically requiring users. This represents a tightening of security by only allowing user-type subjects to access these resources.
35-36
:❓ Verification inconclusive
Verify security implications of wildcard topic subscription
The change to use wildcard patterns (
/*
) for subscribe and publish topics rather than specific team slugs simplifies the implementation but potentially broadens access.Consider whether this broader access pattern aligns with your security requirements. Wildcard subscriptions could potentially allow users to access topics they shouldn't have permission for if the authorization isn't properly enforced elsewhere.
🏁 Script executed:
#!/bin/bash # Search for other authorization checks that might restrict access to specific topics rg -A 5 -B 5 "publish|subscribe" --glob "*.ts" | grep -A 2 -B 2 "Resource.App.name"Length of output: 572
Security Check: Wildcard Topic Subscription Verification
The update to use wildcard patterns (
/*
) for both subscribe and publish topics inpackages/functions/src/party/authorizer.ts
(lines 35-36) broadens the topic access scope. The executed script confirms that there are no additional authorization checks elsewhere in the codebase to further restrict access.
- Location:
packages/functions/src/party/authorizer.ts
, Lines 35-36- Observation: The wildcard pattern (
${Resource.App.name}/${Resource.App.stage}/*
) appears to be the sole mechanism controlling access.- Recommendation: Please verify that this broader access scheme aligns with your security requirements. If tighter access controls (such as restricting to specific team slugs) are needed, consider implementing additional authorization logic to mitigate potential risks.
packages/www/src/assets/portal/play_button_idle.json (1)
1-2317
: Good to see detailed sprite frame data.This JSON file appears structurally sound and valid. Each frame is well-defined with consistent properties. No immediate technical or logical issues stand out.
packages/core/src/actor.ts (2)
3-3
: Import statement aligns well with the updated error handling.Adding
ErrorCodes
andVisibleError
is appropriate for more granular error reporting.
53-61
: Clear, consistent authentication error.Throwing a
VisibleError
withauthentication
type andErrorCodes.Authentication.UNAUTHORIZED
properly communicates unauthorized access.infra/auth.ts (9)
6-7
: New imports for cluster and VPC.Ensures the service can be deployed within the cluster and VPC as intended.
22-22
: EFS filesystem creation is clear.Linking EFS to the
vpc
is a valid approach for persistent storage.
24-27
: CPU and memory choices match stage-based scaling.Selecting higher resources for production is sensible.
28-44
: Command, links, and environment are well-structured.Linking bus, secrets, and other resources is consistent with the microservices pattern.
45-55
: Load balancer configuration looks appropriate.Defines HTTP/HTTPS listeners and forwarding rules in a straightforward manner.
58-63
: Permissions for SES are well-scoped.Granting only email send rights helps limit the service’s AWS privileges.
64-69
: Using EFS for volume is beneficial.Mounting at
/mnt/efs
matches the environment storage path for non-development.
70-74
: Local development settings are properly isolated.Overriding command, directory, and URL ensures a smooth local experience.
75-81
: Stage-based autoscaling configured.Min and max tasks for production help manage load effectively.
sst-env.d.ts (2)
9-13
: Resource type transition from Router to Service architectureThe
Api
resource type has been updated to use a service-oriented architecture ("type": "sst.aws.Service"
) instead of the previous router-based approach. The change fromname
toservice
property aligns with this architectural shift.
14-18
: Auth resource migration to service-oriented architectureSimilar to the API changes, the Auth resource has been transitioned from
"sst.aws.Auth"
to a service-based implementation. This change is part of the broader architectural shift to containerized services.nestri.sln (4)
7-14
: Project structure reorganizationThe solution file has been restructured to add new hierarchical project folders (
functions
,src
,api
) and relocate thesteam
project topackages\functions\src\api\steam\steam.csproj
. This change provides better organization and aligns with the new service-oriented architecture.
21-24
: Updated project configuration for steam projectThe build configurations have been updated to reference the new steam project GUID. This ensures proper build integration with the new project structure.
30-33
: Established project hierarchyThe nested project relationships have been properly defined, establishing a clear hierarchy from packages → functions → src → api → steam. This organization improves project maintainability.
36-36
: Solution GUID updatedThe SolutionGuid has been updated, reflecting the significant restructuring of the solution. This is a required change when making major organizational changes to a solution.
packages/functions/src/auth.ts (4)
6-6
: Added missing imports for enhanced functionalityNew imports have been added to support logging improvements, password authentication, and memory-based storage capabilities.
Also applies to: 13-14
26-32
: Added configurable memory storageThe implementation now includes storage configuration with environment variable-based persistence path. This provides better flexibility for data persistence across deployments.
47-47
: Updated CSS import for fontThe CSS import statement for fonts has been updated to use a more standard approach with Google Fonts, improving font consistency across different environments.
167-177
: Refactored export pattern for containerizationThe export pattern has been changed from a simple handler to a more comprehensive object that includes container configuration (port, idle timeout) and a fetch method with proper error handling. This change supports the transition to a containerized service architecture.
packages/functions/sst-env.d.ts (2)
9-13
: Resource type transition from Router to Service architectureThe
Api
resource type has been updated to use a service-oriented architecture ("type": "sst.aws.Service"
) instead of the previous router-based approach. The change fromname
toservice
property aligns with this architectural shift.
14-18
: Auth resource migration to service-oriented architectureSimilar to the API changes, the Auth resource has been transitioned from
"sst.aws.Auth"
to a service-based implementation. This change is part of the broader architectural shift to containerized services.infra/api.ts (2)
2-2
: Validate the new imports fromauth
andcluster
.
The newly introduced imports forauth
andcluster
appear consistent with the broader shift to a service-oriented architecture. Ensure that any references to the previous function-based approach have been fully removed or updated to avoid conflicts.Also applies to: 5-5
8-50
: Consider verifying the production scaling policy.
Thescaling
config setsmin=2
andmax=10
instances in production, which can handle high traffic but might lead to unused capacity and increased costs. Confirm that this scaling baseline aligns with your expected load and budget constraints.infra/postgres.ts (1)
46-46
: Confirm the updated dev command for the studio.
Switching tobun db:dev studio
may require new or modified scripts in your project. Please verify that your local environment andpackage.json
define this command, ensuring it functions as expected with the new naming convention.packages/www/src/providers/steam.tsx (1)
136-210
: Validate your reconnection edge cases.
The exponential backoff logic is a prudent measure. Confirm that it won’t unintentionally create multiple simultaneousEventSource
connections if the event loop is delayed, especially under intermittent network conditions.packages/www/src/components/index.ts (1)
1-1
: Export looks goodThis barrel export pattern makes the QR code component for Steam authentication easily accessible throughout the application.
packages/core/migrations/meta/_journal.json (1)
18-25
: Migration entry added correctlyThe new migration entry follows the established pattern and maintains consistent formatting with previous entries.
I noticed the timestamp (1743794969007) corresponds to May 2025. Please verify this is intentional, as future-dated migrations might affect the order of execution.
packages/core/src/examples.ts (1)
34-42
: Steam example object follows established patternThe new Steam example object properly uses the same structure and conventions as other example objects, with appropriate field names and realistic sample data.
packages/core/src/steam/steam.sql.ts (1)
4-24
: Well-structured table schema with appropriate indexesThe Steam table schema is well-designed with proper field types, constraints, and indexes. The composite primary key on [userID, id] provides efficient lookups, and the timestamps are appropriately incorporated.
packages/www/src/common/portal/assets/play_icon_intro.json (1)
1-237
: Sprite sheet data for play button intro animation looks well-structured.This JSON file defines the sprite frames for the play button intro animation, which appears to be part of the new play button functionality mentioned in the PR objectives. The sprite sheet contains 25 frames with properly defined dimensions, positions, and metadata.
The file is correctly formatted according to TexturePacker's JSON format with proper frame coordinates, pivot points, and trimming information.
packages/functions/src/log-polyfill.ts (1)
3-16
: Well-implemented logger utility function.The
patchLogger
function creates a consistent logging format by wrapping console methods. It properly:
- Creates a higher-order function for different log levels
- Formats log messages with appropriate prefixes
- Handles variadic arguments via rest parameters
- Replaces newlines for consistent output
Note that this function has side effects as it overwrites the global console object methods, which is important to be aware of when used.
packages/www/src/common/portal/assets/play_icon_exit.json (1)
1-237
: Sprite sheet data for play button exit animation looks well-structured.This JSON file defines the sprite frames for the play button exit animation, complementing the intro animation file. The sprite sheet contains 25 frames with properly defined attributes.
I noticed that frames 17-24 (from lines 156-227) have identical properties, suggesting the animation holds on the same frame at the end, which is a common animation technique.
packages/www/src/ui/custom-qr.tsx (1)
39-39
: Increased logo size for QR code clear area.The logo size has been increased to 38 pixels when
clearArea
is true, which likely improves the visibility or aesthetics of logos embedded within QR codes.This change works well with the existing QR code component's clear area calculation logic on line 103.
packages/www/src/assets/portal/play_icon_intro.json (1)
1-238
: Overall JSON Structure Looks Solid
This file defines the play icon intro animation with a consistent array of frame objects and a clear metadata section. The properties such as "filename", "frame", "rotated", "trimmed", "spriteSourceSize", "sourceSize", and "pivot" are uniformly applied, and the meta section is complete.packages/www/src/common/portal/assets/play_button_idle.json (1)
1-2317
: Well-Structured Sprite Sheet JSON
The JSON file for the play button idle animation is clean and consistent. All frame objects include the expected keys and the meta section details (including image name, overall size, and smart update value) are appropriately specified.packages/www/src/assets/portal/play_button_intro.json (1)
1-589
: Comprehensive Play Button Intro Animation Data
This JSON file clearly defines the frames for the play button introduction animation. The frame objects are detailed with their coordinates, dimensions, and transformation properties. One point to verify is that the first frame ("intro_00000.png") has a width and height of 3 pixels while later frames are 150×150. Please confirm that this extremely small frame is intended, perhaps as an initial transition or a placeholder.packages/www/src/assets/portal/play_icon_loop.json (1)
1-238
: Play Icon Loop Animation JSON is Clear and Consistent
The file defines the loop animation frames for the play icon with concise frame definitions and a well-detailed meta section. The frame dimensions and other properties appear to be appropriate for this animation type.packages/www/src/common/portal/assets/play_button_intro.json (1)
1-588
: Animation asset file looks good.This JSON file defines the frames for a play button introduction animation with 64 frames (intro_00000.png to intro_00063.png). The structure follows the TexturePacker format with proper frame data including positions, dimensions, and transformation properties.
packages/www/src/common/portal/index.tsx (1)
105-105
: Consider accessibility implications of autofocus.Using
autofocus
can cause accessibility issues by immediately moving focus to this element when the page loads, which may confuse screen reader users.Consider if this is the desired behavior and if alternative approaches might be more appropriate for your use case.
packages/core/src/member/index.ts (2)
101-102
: Good optimization of promise chain.This change simplifies the promise chain by combining the mapping and array access operations, making the code more concise and easier to follow.
112-113
: Good optimization of promise chain.Similar to the previous change, this simplifies the promise chain by combining the mapping and array access operations, improving code readability.
packages/core/src/team/index.ts (2)
126-126
: Good optimization of promise chain.This change simplifies the promise chain by combining the mapping and array access operations into a single function call, making the code more concise and easier to follow.
137-137
: Good optimization of promise chain.Similar to the previous change, this simplifies the promise chain by combining operations, improving code readability.
packages/core/migrations/meta/0002_snapshot.json (1)
1-420
: Well-structured database schema for Steam integrationThis migration file establishes a comprehensive PostgreSQL schema including a new
steam
table with appropriate columns for Steam authentication data. The schema includes proper indexing strategies and constraints.packages/functions/src/api/steam/Program.cs (1)
1-168
: Well-implemented Steam authentication with QR code supportThe Steam authentication implementation using SteamKit2 is well-structured, with proper event handling and a clean flow from QR code generation to user authentication.
packages/functions/src/api/auth.ts (3)
8-11
: Updated authentication client issuer configurationThe authentication client issuer has been properly updated to use
Resource.Auth.url
.
27-27
: Improved handling of unauthenticated requestsThe code now properly sets a public actor type for requests without an authorization header, enhancing security and clarity of the authentication flow.
46-63
: Simplified team context handlingThe user and team context handling has been streamlined with improved actor management, making the authentication flow clearer.
packages/www/src/components/qr-code.tsx (2)
219-289
: Well-structured Steam authentication hookThe
useSteamAuth
hook provides a clean interface for managing the Steam authentication process, with proper error handling and connection management.
291-353
: Well-implemented QR code component with appropriate UI statesThe QR code component provides a good user experience with different states for loading, displaying the QR code, and handling errors. It also includes helpful information for users about how to use the Steam Mobile App.
packages/www/src/common/portal/button.ts (1)
155-166
: Queue tasks may never run.
BothbuttonQueue
andiconQueue
simply accumulate tasks when an animation is already in progress, but there is no code to dequeue and execute them once the current task completes. This could cause queued animations to remain pending indefinitely.Consider re-executing queued tasks as soon as
isButtonRunning
orisIconRunning
becomesfalse
. Would you like a patch that implements queue processing on animation completion?Also applies to: 295-302
packages/functions/src/api/index.ts (1)
92-100
: Verify the idle timeout configuration.
The default export setsidleTimeout
to255
. Confirm that this is the intended value for production, as it may affect long-lived connections.Do you want me to run any checks or open a new issue for verifying timeouts across deployments?
packages/functions/src/api/steam/index.ts (3)
11-11
: Investigate the "FIXME" marker.
This comment indicates that the "credentials" event handler isn't triggering. You may need to confirm that the event is emitted within theSteam
class and that no concurrency or race condition blocks it.
44-44
: Verify the relative path to the Steam binary.
Passingpath.join(__dirname, "/bin/steam")
assumes the binary is in a/bin
subdirectory. Confirm that this folder and file location are correct at runtime.
88-92
: Confirm database insertion of Steam credentials.
The comment suggests that the code to persist credentials (SteamDB.create(credentials)
) is intentionally commented out. If you plan to store them, ensure you handle security concerns and properly parse credentials before saving.packages/core/migrations/0002_simple_outlaw_kid.sql (1)
16-17
: Assess indexing strategy and concurrency.
Creating unique and non-unique indexes on email can help performance, but concurrency on the “user_id,email” uniqueness might cause conflicts if email updates occur frequently. Verify that the desired uniqueness matches your real-world usage.packages/core/src/steam/index.ts (1)
50-74
: Review partial input handling duringcreate
.
UsingInfo.partial()
is flexible, but ensure required fields never end up missing at database insertion time. For optional fields likeuserID
, confirm fallback logic or validations are comprehensive.packages/www/src/pages/team/home.tsx (11)
10-12
: Imports for Avatar, Portal, and QrCodeComponent look good
They are properly utilized in the updated code. No issues found here.
200-201
: Column layout change approved
Switching from four columns to two columns is consistent with the straightforward UI design. Looks good for readability.
206-206
: Renaming SteamLibraryTitle to Title
Renaming styled components for clarity is a good practice, improving maintainability.
218-226
: New SteamGameTitle styled component
Extracting the game title text into a dedicated styled component helps maintain design consistency.
251-263
: New FriendsList grid layout
Implementation looks straightforward for displaying multiple friend entries. No concerns noted.
281-287
: Introduced FriendsSubText
The styling and naming are properly aligned with your design system. Looks good.
288-294
: FriendsText styling matches heading specs
Consistent typography hierarchy helps maintain overall visual cohesion.
317-333
: SteamGameContainer styling is consistent
Using border lines for multiple items is neat and keeps the layout well-defined.
335-343
: SteamGameImg usage
The structure conveys clarity for displaying game cover art. No issues found.
345-354
: SteamGameText for game details
Separating the text helps maintain modular and readable styling.
380-390
: PortalContainer
Providing a dedicated container for portal usage is a clean approach to layering.packages/functions/src/api/steam/steamAuth.ts (6)
1-4
: Imports for child_process spawn and events
These set the foundation for Steam authentication. No issues found in the import approach.
14-18
: SteamAuthResult interface
Clear usage to encompass challenge URLs, credentials, and errors. Flexible interface design.
20-26
: SteamAuthEvents interface
Defining typed callbacks for each event is a solid approach in an EventEmitter-based architecture.
47-57
: Typed event listener registration
Using generics for typed callbacks fosters clarity and maintainability.
183-191
: getState method
A direct, simple method to retrieve current challenge URLs and credentials.
208-227
: cleanup method
Properly clearing timeouts and terminating the child process helps avoid resource leaks. Well done.
// Global connection state to prevent multiple instances | ||
let globalEventSource: EventSource | null = null; | ||
let globalReconnectAttempts = 0; | ||
const MAX_RECONNECT_ATTEMPTS = 1; | ||
let isConnecting = false; | ||
let activeConnection: SteamConnection | null = null; | ||
|
||
// FIXME: The redo button is not working as expected... it does not reinitialise the connection | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Reassess global state usage for concurrency.
Storing globalEventSource
, isConnecting
, and activeConnection
as global variables could introduce race conditions if multiple components or tabs share them simultaneously. A more localized or reference-counted approach may help avoid unexpected overwrites.
<Title>Your Steam library</Title> | ||
<SubTitle>These titles from your Steam Library are fully functional on Nestri</SubTitle> | ||
<div> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets-prd.ignimgs.com/2023/05/27/alanwake2-1685200534966.jpg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Alan Wake II</SteamGameTitle> | ||
<SteamGameSubTitle>Action, Adventure, Horror</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Install</SteamGameBtn> | ||
</SteamGameContainer> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets-prd.ignimgs.com/2022/09/22/slime-rancher-2-button-02-1663890048548.jpg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Slime Rancher 2</SteamGameTitle> | ||
<SteamGameSubTitle>Action, Adventure, Casual, Indie</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Install</SteamGameBtn> | ||
</SteamGameContainer> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets1.ignimgs.com/2019/07/17/doom-eternal---button-fin-1563400339680.jpg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Doom Eternal</SteamGameTitle> | ||
<SteamGameSubTitle>Action</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Install</SteamGameBtn> | ||
</SteamGameContainer> | ||
</div> | ||
<div> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets-prd.ignimgs.com/2022/10/12/dead-space-2023-button-3-1665603079064.jpg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Dead Space</SteamGameTitle> | ||
<SteamGameSubTitle>Action, Adventure</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Update</SteamGameBtn> | ||
</SteamGameContainer> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets-prd.ignimgs.com/2023/01/25/hifirush-1674680068070.jpg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Hi-Fi Rush</SteamGameTitle> | ||
<SteamGameSubTitle>Action</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Install</SteamGameBtn> | ||
</SteamGameContainer> | ||
<SteamGameContainer> | ||
<SteamGameImg draggable={false} src="https://assets-prd.ignimgs.com/2023/08/24/baldursg3-1692894717196.jpeg" /> | ||
<SteamGameText> | ||
<SteamGameTitle>Baldur's Gate 3</SteamGameTitle> | ||
<SteamGameSubTitle>Adventure, RPG, Strategy</SteamGameSubTitle> | ||
</SteamGameText> | ||
<SteamGameBtn>Install</SteamGameBtn> | ||
</SteamGameContainer> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Steam library display
Displaying the list of games is well-structured. Unify repeated SteamGameContainer blocks by iterating over a data array to reduce redundancy.
/** | ||
* Start the authentication process | ||
* The method will emit events as the process progresses | ||
* - 'challengeUrl' when the challenge URL is available | ||
* - 'credentials' when login credentials are available | ||
* - 'error' if any error occurs | ||
* - 'completed' when the entire process completes | ||
*/ | ||
async startAuth(): Promise<void> { | ||
try { | ||
// Clean up any previous state | ||
this.cleanup(); | ||
|
||
// Ensure executable permissions first | ||
await this.ensureExecutable(); | ||
|
||
console.log("Starting Steam authentication process..."); | ||
this.authProcess = spawn(this.csharpAppPath, [], { | ||
stdio: ['pipe', 'pipe', 'pipe'] | ||
}); | ||
if (this.authProcess.stdout) { | ||
// Listen for data from stdout | ||
this.authProcess.stdout.on("data", (data) => { | ||
const output = data.toString(); | ||
console.log("C# output:", output); | ||
|
||
// Try to parse any JSON in the output | ||
try { | ||
// Look for JSON objects in the text | ||
const jsonMatches = output.match(/(\{.*?\})/g); | ||
if (jsonMatches) { | ||
for (const jsonStr of jsonMatches) { | ||
const jsonData = JSON.parse(jsonStr); | ||
|
||
// Check if this JSON contains a challenge URL - emit for ALL URL updates | ||
if (jsonData.challengeUrl) { | ||
const newUrl = jsonData.challengeUrl; | ||
// Always emit the event, even if the URL is the same | ||
this.challengeUrl = newUrl; // Update the stored URL | ||
this.emit('challengeUrl', newUrl); | ||
} | ||
|
||
if (jsonData.error && jsonData.message) { | ||
console.log("Found error:", jsonData.message); | ||
this.emit('error', jsonData.message); | ||
} | ||
|
||
// Check if this JSON contains credentials | ||
if (jsonData.username && jsonData.accessToken) { | ||
this.credentials = jsonData; | ||
console.log("Found login credentials"); | ||
this.emit('credentials', this.credentials); | ||
|
||
// Once we have credentials, we can complete the process | ||
this.completeAuth(); | ||
} | ||
} | ||
} | ||
} catch (e) { | ||
// Not valid JSON, continue listening | ||
} | ||
}); | ||
} | ||
|
||
|
||
if (this.authProcess.stderr) | ||
// Handle errors | ||
this.authProcess.stderr.on("data", (data) => { | ||
console.error("C# error:", data.toString()); | ||
}); | ||
|
||
// Process completion | ||
this.authProcess.on("close", (code) => { | ||
if (!this.credentials && !this.challengeUrl) { | ||
const error = new Error(`C# process exited with code ${code} before providing any useful data`); | ||
this.emit('error', error); | ||
} | ||
this.completeAuth(); | ||
}); | ||
|
||
// Set a timeout in case the process hangs | ||
this.timeout = setTimeout(() => { | ||
const timeoutError = new Error("Timeout waiting for authentication"); | ||
console.error(timeoutError.message); | ||
this.emit('error', timeoutError); | ||
this.completeAuth(); | ||
}, this.timeoutDuration); | ||
|
||
} catch (error: any) { | ||
console.error("Steam authentication error:", error.message); | ||
this.emit('error', error); | ||
this.completeAuth(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
startAuth logic
Regex-based JSON extraction can be fragile if the process outputs multiline or partial JSON. Consider a buffer-based parser for more reliability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
♻️ Duplicate comments (2)
packages/steam/Program.cs (2)
92-92
:⚠️ Potential issueSanitize logged credentials to prevent PII leaks.
Logging
"Using provided credentials for {username}"
may risk PII leaks in log files. Consider masking or omitting sensitive data in logs.- Console.WriteLine($"Using provided credentials for {username}"); + Console.WriteLine("Using provided credentials for user login.");
75-78
:⚠️ Potential issueAdd error handling for missing request type.
When the
"type"
property is missing, the request is silently ignored. It would be safer to respond with an error message.if (request.TryGetProperty("type", out var typeElement)) { string requestType = typeElement.GetString() ?? string.Empty; // ... } +else +{ + SendErrorResponse(client, "Missing request type"); +}
🧹 Nitpick comments (11)
packages/steam/package.json (1)
1-8
: Add standard package.json fields for improved package maintainability.The package.json file defines scripts for development, client execution, and building, but is missing several standard fields that would improve package discoverability and maintenance.
Consider adding these additional fields:
{ "name": "@nestri/steam", + "version": "0.1.0", + "description": "Steam authentication module for Nestri", + "private": true, + "main": "index.ts", "scripts": { "dev": "dotnet watch run", "client": "bun index", "build": "dotnet build" - } + }, + "dependencies": { + + } }packages/functions/src/api/realtime/actor-core.ts (1)
3-26
: Implement message history limit to prevent memory issues.There's no limit to the size of the
messages
array, which could lead to memory issues if the chat room is active for a long time.Add a configuration option for maximum message history and implement a mechanism to trim old messages:
// Define a chat room actor const chatRoom = actor({ + // Configuration + config: { + maxMessages: 100, // Limit message history to prevent memory issues + }, + // Initialize state when the actor is first created createState: () => ({ messages: [] as any[], }), // Define actions clients can call actions: { // Action to send a message sendMessage: (c, sender, text) => { // Update state c.state.messages.push({ sender, text }); + + // Trim message history if needed + if (c.state.messages.length > c.config.maxMessages) { + c.state.messages = c.state.messages.slice(-c.config.maxMessages); + } // Broadcast to all connected clients c.broadcast("newMessage", { sender, text }); },packages/steam/Program.cs (1)
128-141
: Add newline terminator to JSON responses for consistency.The
SendErrorResponse
method doesn't add a newline terminator to the JSON response, unlike theSendToClient
method in theSteamLoginComponent
class. This inconsistency could cause parsing issues for clients.Add a newline terminator to the JSON response:
private static void SendErrorResponse(Socket client, string errorMessage) { try { var errorResponse = new { type = "error", message = errorMessage }; string jsonResponse = JsonSerializer.Serialize(errorResponse); + jsonResponse += "\n"; byte[] responseBytes = Encoding.UTF8.GetBytes(jsonResponse); client.Send(responseBytes); } catch { // Ignore send errors on an already problematic connection } }
packages/functions/src/api/index.ts (4)
17-22
: Add types for the realtime application setup.The
setup
function is called without explicit type parameters, which could lead to type inference issues. Additionally, theactorRouter
andwebSocketHandler
don't have explicit type annotations.Add explicit type annotations to improve code maintainability:
+interface ChatRoomActors { + chatRoom: typeof chatRoom; +} -const realtimeApp = setup({ +const realtimeApp = setup<ChatRoomActors>({ actors: { chatRoom }, basePath: "/realtime" }); -const { router: actorRouter, webSocketHandler } = createRouter(realtimeApp); +const { router: actorRouter, webSocketHandler } = createRouter<ChatRoomActors>(realtimeApp);
25-31
: Middleware order inconsistency in HTTP handler setup.The logger middleware is applied before CORS, which means CORS-rejected requests will still be logged. Additionally, the async middleware for setting the Cache-Control header could be simplified.
Reorder middleware and simplify the async handler:
app - .use(logger()) .use(cors()) + .use(logger()) - .use(async (c, next) => { - c.header("Cache-Control", "no-store"); - return next(); - }) + .use((c, next) => { + c.header("Cache-Control", "no-store"); + return next(); + }) .use(auth)
36-38
: Document the reason for commenting out the Steam API route.The SteamApi route is commented out without explanation, which could lead to confusion for other developers working on the codebase.
Add a comment explaining why the Steam API route is commented out:
.route("/realtime", actorRouter) .route("/team", TeamApi.route) - // .route("/steam", SteamApi.route) + // Steam API route has been replaced with Unix socket-based implementation + // .route("/steam", SteamApi.route) .route("/account", AccountApi.route)
103-112
: Add documentation for the exported configuration object.The default export contains configuration options without any documentation explaining their purpose or usage, which could make it difficult for other developers to understand how to use this module.
Add JSDoc comments to document the exported configuration:
+/** + * API server configuration + * @property {number} port - The port to listen on + * @property {number} idleTimeout - The idle timeout in seconds + * @property {Function} webSocketHandler - Handler for WebSocket connections + * @property {Function} fetch - Handler for HTTP requests + */ export default { port: 3001, idleTimeout: 255, webSocketHandler, fetch: (req: Request) => app.fetch(req, undefined, { waitUntil: (fn) => fn, passThroughOnException: () => { }, }), };packages/steam/SteamSocketAuth.cs (2)
135-152
: Effective logged-on callback.The logging and error feedback to the client is clear. Ensure final states (like
_isRunning
) are updated or confirmed if logon fails repeatedly to avoid indefinite waiting states in the callback loop.
212-232
: Potential large message handling.
SendToClient
serializes and sends the entire object in one shot. For extremely large payloads, consider chunking or a streaming approach. Otherwise, no immediate issues.packages/steam/index.ts (2)
128-141
: Reusing saved credentials with user consent.Asking whether to reuse saved credentials is good practice. Consider adding an explicit fallback if the saved credentials are invalid for some reason.
270-280
: Loading credentials from file.The logic safely checks if the file exists. Consider validating token freshness or error states for expired credentials.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lock
is excluded by!**/*.lock
📒 Files selected for processing (10)
infra/steam.ts
(1 hunks)packages/functions/Containerfile
(1 hunks)packages/functions/package.json
(2 hunks)packages/functions/src/api/index.ts
(2 hunks)packages/functions/src/api/realtime/actor-core.ts
(1 hunks)packages/steam/.gitignore
(1 hunks)packages/steam/Program.cs
(1 hunks)packages/steam/SteamSocketAuth.cs
(1 hunks)packages/steam/index.ts
(1 hunks)packages/steam/package.json
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/steam/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/functions/Containerfile
- packages/functions/package.json
🧰 Additional context used
🧬 Code Definitions (3)
packages/steam/Program.cs (1)
packages/steam/SteamSocketAuth.cs (7)
Task
(40-59)SteamLoginComponent
(9-249)SteamLoginComponent
(19-33)SetClientSocket
(35-38)LoginRequest
(251-256)SetCredentials
(238-242)StopProcess
(244-248)
packages/steam/SteamSocketAuth.cs (1)
packages/steam/Program.cs (1)
Task
(52-126)
packages/steam/index.ts (1)
packages/steam/SteamSocketAuth.cs (1)
LoginRequest
(251-256)
🔇 Additional comments (22)
infra/steam.ts (1)
1-7
:❓ Verification inconclusive
Confirm local development compatibility and required dependencies.
This DevCommand uses
bun dev
in thepackages/steam
directory withautostart: true
, which is convenient for streamlined local development. However, please ensure that:
- The
bun
runtime is installed and compatible across your development team’s environments.- The
packages/steam
directory includes an entry point that aligns with thebun dev
command’s conventions.- Any environment variables or configurations from the old setup have been properly transitioned or removed if no longer needed.
Below is a script to verify the presence of the
bun
runtime in the project dev environment, as well as confirm that thepackages/steam
directory exists and includes an appropriatedev
script:
🏁 Script executed:
#!/bin/bash # 1. Check if bun is installed: if ! command -v bun &> /dev/null then echo "bun is not installed" exit 1 fi # 2. Verify that the packages/steam directory exists and includes files for bun dev: if [ ! -d "packages/steam" ]; then echo "Directory packages/steam not found" exit 1 fi echo "All checks passed: bun is installed and packages/steam directory is present."Length of output: 76
Ensure Bun Runtime Is Properly Installed for Local Development
TheDevCommand
ininfra/steam.ts
relies on runningbun dev
in thepackages/steam
directory withautostart: true
. However, our verification script indicates that Bun is missing in the current environment. Please manually verify that:
- The Bun runtime is installed and available on all development environments.
- The
packages/steam
directory includes a proper entry point or dev script that aligns with Bun’s conventions.- Any legacy environment variables or configuration settings have been removed or updated accordingly.
Manual verification is necessary to confirm that every team member has the required dependencies installed.
packages/steam/SteamSocketAuth.cs (9)
19-33
: Ensure robust exception handling in constructor.The constructor correctly initializes the core SteamKit2 components and registers the necessary callbacks. However, consider wrapping the handler creation with try/catch blocks or verifying that
_steamClient.GetHandler<T>()
does not produce null references. This helps prevent partial initialization states or silent failures if a handler is missing.Would you like me to generate a script to scan for any unhandled exceptions or missing handler usage across the codebase?
35-38
: Socket assignment looks good.Assigning the client socket in this method centralizes the socket reference. No issues found.
61-61
: Previously noted "async void" approachThis code reproduces a past recommendation: use
async Task
instead ofasync void
to avoid unhandled exceptions and lost stack traces.
61-124
: Proactively handle partial success states during QR auth.In the QR code flow (lines 85–117), if
BeginAuthSessionViaQRAsync
fails mid-way, the_authCompletionSource
is set to false only in the catch block. Ensure that partial success states or mid-process fail states also trigger_authCompletionSource.TrySetResult(false)
to prevent potential deadlocks.
126-133
: Clean disconnection signaling.Gracefully handling
OnDisconnected
by notifying the client and setting_authCompletionSource
ensures the consumer is aware of the current state. Good approach.
175-197
: Logging off immediately after receiving PersonaState.You log off as soon as the avatar data is received (line 192), presumably concluding authentication. Double-check if there's a scenario where more user information is needed (friends list, for instance) before disconnecting.
238-242
: Credential setter meets local usage needs.The approach is straightforward for storing direct login credentials. Keep in mind any potential security implications if these values stay in memory for extended periods or are logged inadvertently.
244-249
: Graceful shutdown logic.
StopProcess
stops the callback loop and disconnects the client. This is a clean approach. No issues detected.
251-256
: Lightweight request object.The
LoginRequest
class suits the socket login flow. No issues with the declared fields.packages/steam/index.ts (12)
1-28
: Interfaces effectively mirror server data.Defining structured message interfaces ensures type safety and clarity when handling incoming/outgoing data. This is a solid approach to managing communication protocols.
67-81
: Constructor sets up readline and local properties.The
SteamSocketClient
constructor properly initializes the readline interface. Consider adding an optional logger or debug mode to track user input if needed.
82-105
: Connection and startup logic.
start()
attempts to load credentials, connects to the socket, and handles primary events. Excellent use of try/catch for initial error handling.
106-115
: Clean connected event.The
onConnected()
event setsisConnected
and either reuses existing credentials or prompts the user. Straightforward flow.
117-126
: Prompt-based approach for login method.Offering a choice between QR login and direct credentials is user-friendly. No issues found here.
156-217
: Incremental JSON parsing strategy.
onDataReceived
andprocessBuffer
attempt to handle partial or multiple JSON messages. While workable, this approach could break if multiple complete JSON objects arrive in a single chunk. Ensure thorough testing with varying chunk boundaries.Would you like a script that simulates multiple combined JSON messages in a single buffer for stress testing?
219-241
: Comprehensive message handling.
handleMessage
delegates to specialized methods bytype
. This is maintainable and easy to extend for new commands.
243-253
: QR code generation is user-friendly.
handleChallengeUrl
stashes the challenge URL and generates a terminal QR code. This effectively streamlines QR-based logins. No issues found.
289-295
: Socket write is straightforward.
sendMessage
does a direct JSON write to the socket. Ensure error handling for partial writes or network issues is well tested.
297-302
: Use of 'disconnect' message.Explicitly sending a
disconnect
request to the server is a good approach. No issues found.
303-316
: Cleanup method ensures graceful shutdown.Ending the socket and closing the readline interface is well-handled. No further suggestions.
319-328
: Main entry logic is clear.Instantiating the client and handling SIGINT to disconnect is a neat approach. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
♻️ Duplicate comments (2)
packages/steam/index.ts (1)
1-149
: Revisit credential storage securityA previous review comment flagged concerns about credential storage security. While this file doesn't explicitly show credential storage, it's important to address these concerns if you're persisting credentials.
Consider encryption or more secure storage methods for any user credentials that might be persisted in the system. Plain text storage of Steam credentials could pose security risks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/steam/Program.cs (1)
9-9
: Security risk in Unix socket pathUsing
/tmp/steam.sock
for the Unix domain socket is a security risk as the/tmp
directory is writable by all users on most systems. Consider using a location with stricter permissions, like/var/run/nestri/steam.sock
.This was previously flagged in a review but the issue persists. A secure socket location is essential for production systems.
🧹 Nitpick comments (22)
packages/steam/index.ts (2)
130-137
: Improve disconnect method with proper typingAdd return type annotation to the
disconnect
method for better type safety.- disconnect() { + public disconnect(): void { if (this.httpClient) { this.httpClient.destroy(); this.httpClient = null; console.log('Disconnected from Steam login service'); } }
144-149
: Consider moving process signal handling outside the class fileThe process signal handling is directly in the module file, which makes it execute immediately upon import. This could lead to multiple handlers if the module is imported multiple times.
Consider moving the signal handling to a separate file or export a function to set up the handlers that can be called explicitly by the consumer.
- // Handle process termination - process.on('SIGINT', () => { - console.log('Closing connections...'); - steam.disconnect(); - setTimeout(() => process.exit(0), 1000); - }); + // Export a function to set up process handlers + export function setupProcessHandlers(steamInstance: Steam): void { + process.on('SIGINT', () => { + console.log('Closing connections...'); + steamInstance.disconnect(); + setTimeout(() => process.exit(0), 1000); + }); + } + + // Then it can be called optionally: setupProcessHandlers(steam);packages/steam/package.json (4)
2-2
: Consider using a scoped package nameThe package name is simply "steam", but the AI summary suggests it should be "@nestri/steam". Using a scoped name helps with organization and prevents name collisions.
- "name": "steam", + "name": "@nestri/steam",
20-25
: Review dependencies for usage and add missing type definitionsSome dependencies don't appear to be used in the index.ts file, and type definitions are missing for qrcode-terminal.
"dependencies": { - "eventsource": "^3.0.6", "qrcode": "^1.5.4", "qrcode-terminal": "^0.12.0", - "ws": "^8.18.1" }Also, consider adding type definitions for qrcode-terminal:
"devDependencies": { "@types/bun": "latest", + "@types/qrcode-terminal": "^0.12.0", "ts-node": "^10.9.2" },
6-12
: Clarify mixed technology stack in scriptsThe scripts section contains a mix of Node.js and .NET commands, which can be confusing. Consider adding comments or organizing them better.
"scripts": { + "// .NET commands": "", "dev": "dotnet watch run", - "client": "bun index", - "client:node": "ts-node index", "build": "dotnet build", - "db:migrate": "dotnet ef migrations add" + "db:migrate": "dotnet ef migrations add", + "// Node.js commands": "", + "client": "bun index", + "client:node": "ts-node index" },Alternatively, you could add a brief comment explaining the dual technology approach in the package.json.
1-26
: Consider pinning dependency versions for better reproducibilityUsing caret (^) ranges for dependencies can introduce subtle bugs when dependencies update. For a production package, consider pinning exact versions.
"dependencies": { - "eventsource": "^3.0.6", - "qrcode": "^1.5.4", - "qrcode-terminal": "^0.12.0", - "ws": "^8.18.1" + "eventsource": "3.0.6", + "qrcode": "1.5.4", + "qrcode-terminal": "0.12.0", + "ws": "8.18.1" }packages/steam/Configuration.cs (1)
3-14
: Consider adding more configuration options and contextThe configuration method provides limited options. You might want to expose more SteamKit2 configuration options as needed for your application.
As your Steam integration grows, you may need to add more configuration options such as:
- Web API key configuration
- Cell ID setting
- Machine info
- Server list provider
Consider creating a more comprehensive configuration approach, perhaps using the options pattern with dependency injection if you're using ASP.NET Core.
packages/steam/DBSchema.cs (1)
1-9
: Consider adding validation attributes and index annotationsThe entity lacks validation attributes and database index annotations that would define constraints at the model level.
If you're using Entity Framework, consider adding data annotations:
+using System; +using System.ComponentModel.DataAnnotations; +using System.ComponentModel.DataAnnotations.Schema; +/// <summary> +/// Represents Steam user credentials for authentication. +/// </summary> +[Table("SteamUserCredentials")] +[Index(nameof(UserId), IsUnique = true)] public class SteamUserCredentials { + [Key] + [DatabaseGenerated(DatabaseGeneratedOption.Identity)] public int Id { get; set; } // Keep as primary key + [Required] + [StringLength(50)] public required string UserId { get; set; } + [Required] + [StringLength(100)] public required string AccountName { get; set; } + [Required] public required string RefreshToken { get; set; } public DateTime CreatedAt { get; set; } = DateTime.UtcNow; public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; }packages/functions/src/api/realtime/index.ts (1)
5-15
: Well-structured real-time communication setupThe namespace approach nicely encapsulates the real-time functionality with clear exports. Consider adding error handling to make the implementation more robust.
export namespace Realtime { const app = setup({ actors: { chatRoom }, basePath: "/realtime" }); const realtimeRouter = createRouter(app); + // Consider adding error handling for WebSocket connections export const route = realtimeRouter.router; export const webSocketHandler = realtimeRouter.webSocketHandler; }
packages/steam/Program.cs (3)
86-153
: Complex login endpoint handling - consider refactoringThe
/login
endpoint implementation is quite complex with SSE setup, error handling, and subscription management all in one place. This could become difficult to maintain.Consider breaking this down into smaller methods for better maintainability:
app.MapGet("/login", async (HttpContext context, SteamService steamService) => { - var userID = context.Request.Headers["user-id"].ToString(); - if (string.IsNullOrEmpty(userID)) - { - context.Response.StatusCode = 400; - await context.Response.WriteAsync("Missing user ID"); - return; // Early return, no Results object - } - - // Set SSE headers - context.Response.Headers.Append("Connection", "keep-alive"); - context.Response.Headers.Append("Cache-Control", "no-cache"); - context.Response.Headers.Append("Content-Type", "text/event-stream"); - context.Response.Headers.Append("Access-Control-Allow-Origin", "*"); - - var responseBodyFeature = context.Features.Get<IHttpResponseBodyFeature>(); - responseBodyFeature?.DisableBuffering(); - var clientId = userID; - var cancellationToken = context.RequestAborted; + if (!ValidateUserID(context, out var userID, out var clientId)) + { + return; + } + + SetupSSEHeaders(context); + var cancellationToken = context.RequestAborted; try { - // Start Steam authentication - await steamService.StartAuthentication(userID!); - // Register for updates - var subscription = steamService.SubscribeToEvents(clientId, async (evt) => - { - try - { - // Serialize the event to SSE format - string eventMessage = evt.Serialize(); - byte[] buffer = Encoding.UTF8.GetBytes(eventMessage); - await context.Response.Body.WriteAsync(buffer, cancellationToken); - await context.Response.Body.FlushAsync(cancellationToken); - Console.WriteLine($"Sent event type '{evt.Type}' to client {clientId}"); - } - catch (Exception ex) - { - Console.WriteLine($"Error sending event to client {clientId}: {ex.Message}"); - } - }); + await steamService.StartAuthentication(userID!); + var subscription = SubscribeToSteamEvents(steamService, clientId, context, cancellationToken); try { await Task.Delay(Timeout.Infinite, cancellationToken); } catch (TaskCanceledException) { Console.WriteLine($"Client {clientId} disconnected"); } finally { steamService.Unsubscribe(clientId, subscription); } } catch (Exception ex) { HandleAuthenticationError(context, clientId, ex); } // No return statement with Results.Ok() - we're handling the response manually }); // Helper methods below private bool ValidateUserID(HttpContext context, out string userID, out string clientId) { userID = context.Request.Headers["user-id"].ToString(); clientId = userID; if (string.IsNullOrEmpty(userID)) { context.Response.StatusCode = 400; context.Response.WriteAsync("Missing user ID").GetAwaiter().GetResult(); return false; } return true; } private void SetupSSEHeaders(HttpContext context) { context.Response.Headers.Append("Connection", "keep-alive"); context.Response.Headers.Append("Cache-Control", "no-cache"); context.Response.Headers.Append("Content-Type", "text/event-stream"); context.Response.Headers.Append("Access-Control-Allow-Origin", "*"); var responseBodyFeature = context.Features.Get<IHttpResponseBodyFeature>(); responseBodyFeature?.DisableBuffering(); } private Action SubscribeToSteamEvents(SteamService steamService, string clientId, HttpContext context, CancellationToken cancellationToken) { return steamService.SubscribeToEvents(clientId, async (evt) => { try { // Serialize the event to SSE format string eventMessage = evt.Serialize(); byte[] buffer = Encoding.UTF8.GetBytes(eventMessage); await context.Response.Body.WriteAsync(buffer, cancellationToken); await context.Response.Body.FlushAsync(cancellationToken); Console.WriteLine($"Sent event type '{evt.Type}' to client {clientId}"); } catch (Exception ex) { Console.WriteLine($"Error sending event to client {clientId}: {ex.Message}"); } }); } private void HandleAuthenticationError(HttpContext context, string clientId, Exception ex) { Console.WriteLine($"Error during authentication for client {clientId}: {ex.Message}"); if (!context.Response.HasStarted) { context.Response.StatusCode = 500; context.Response.WriteAsync("An error occurred during authentication.").GetAwaiter().GetResult(); } }
131-131
: Infinite wait risks resource leaksThe login endpoint sets up SSE connections that wait indefinitely with
Task.Delay(Timeout.Infinite, cancellationToken)
. While the cancellation token should handle disconnections, consider implementing an additional timeout mechanism or heartbeat to ensure resources are released if clients disconnect improperly.- await Task.Delay(Timeout.Infinite, cancellationToken); + // Add a timeout of 4 hours to prevent indefinite resource allocation + using var timeoutCts = new CancellationTokenSource(TimeSpan.FromHours(4)); + using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token); + try { + await Task.Delay(Timeout.Infinite, linkedCts.Token); + } catch (TaskCanceledException) { + if (timeoutCts.IsCancellationRequested) { + Console.WriteLine($"Client {clientId} session timed out after 4 hours"); + } else { + Console.WriteLine($"Client {clientId} disconnected"); + } + }
184-186
: Commented-out database operations in production codeThe database operations contain commented out code for development operations like
EnsureDeleted
andEnsureCreated
.Consider completely removing these commented out lines, or moving them to a development-only conditional block:
- // dbContext.Database.EnsureDeleted(); // Only use during development! - // dbContext.Database.EnsureCreated(); dbContext.Database.Migrate();packages/functions/src/api/index.ts (2)
17-18
: Consider optimizing middleware orderThe CORS middleware is placed after the logger middleware. This means the logger will log requests that might be rejected by CORS. Consider placing CORS before logger for more efficient request handling.
app - .use(logger()) - .use(cors()) + .use(cors()) + .use(logger()) .use(async (c, next) => {
6-6
: Remove commented-out codeThe
SteamApi
route import and route mapping are both commented out. If this is a permanent change, consider removing the commented code rather than leaving it in the codebase.import { TeamApi } from "./team"; -// import { SteamApi } from "./steam"; import { logger } from "hono/logger"; ... .route("/realtime", Realtime.route) .route("/team", TeamApi.route) - // .route("/steam", SteamApi.route) .route("/account", AccountApi.route)Also applies to: 29-29
packages/steam/SteamClient.cs (2)
239-242
: Fix whitespace in status messageThe status message for logging in has an extra space at the beginning which may cause inconsistent formatting in the UI.
NotifyEvent(new ServerSentEvent("status", new { - message = $" Logging in as '{pollResponse.AccountName}'" + message = $"Logging in as '{pollResponse.AccountName}'" }));
259-262
: Fix whitespace in error status messageSimilar to the previous issue, this error message has an extra space at the beginning.
NotifyEvent(new ServerSentEvent("status", new { - message = $" Authentication polling error: {ex.Message}" + message = $"Authentication polling error: {ex.Message}" }));packages/steam/Auth.cs (6)
31-34
: Ensure SSE headers are set.
While setting_httpResponse
is straightforward, also confirm that your caller configures SSE headers (e.g."Content-Type": "text/event-stream"
) and handles keep-alive timeouts.
56-137
: Direct login vs. QR flow and error handling.
The fallback-based approach using direct token login and then QR authentication is flexible. However, consider these points:
- Validate
_username
and_refreshToken
for null or invalid values before attempting a logon.- Surface more specific error messages to help diagnose user credentials issues.
- Confirm that repeated QR session creation won’t lead to concurrency or memory overhead if multiple login requests crash or remain unfinished.
150-174
: Consider setting SSE response headers at send time.
TheSendToClient
method is a good central location to handle SSE messages. As a further improvement, validate that the initial response sets appropriate streaming headers once (e.g.,Cache-Control: no-cache
).
205-208
: LoggedOff callback clarity.
TheOnLoggedOff
callback effectively notifies the client. Consider whether you want to stop the callback loop (_isRunning
) here too or if forcibly logging off in other scenarios might leave the loop running.
209-210
: Refresh tokens stored in memory.
Storing_refreshToken
in memory can suffice for short-lived operations, but consider encrypting or securing it for longer sessions in case the process memory is dumped.
212-216
: Input validation on credentials.
TheSetCredentials
method should potentially check for empty or invalid credentials before assigning them to_username
and_refreshToken
. This helps avoid accidental overwriting with invalid data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lock
is excluded by!**/*.lock
📒 Files selected for processing (23)
nestri.sln
(1 hunks)packages/functions/src/api/index.ts
(2 hunks)packages/functions/src/api/realtime/index.ts
(1 hunks)packages/steam/.gitignore
(2 hunks)packages/steam/Auth.cs
(1 hunks)packages/steam/Configuration.cs
(1 hunks)packages/steam/DBContext.cs
(2 hunks)packages/steam/DBSchema.cs
(1 hunks)packages/steam/Migrations/20250408090438_InitialCreate.Designer.cs
(2 hunks)packages/steam/Migrations/20250408090438_InitialCreate.cs
(1 hunks)packages/steam/Migrations/20250408125920_RemoveTeam.Designer.cs
(1 hunks)packages/steam/Migrations/20250408125920_RemoveTeam.cs
(1 hunks)packages/steam/Migrations/SteamDbContextModelSnapshot.cs
(2 hunks)packages/steam/Program.cs
(1 hunks)packages/steam/Properties/launchSettings.json
(2 hunks)packages/steam/README.md
(1 hunks)packages/steam/SteamClient.cs
(14 hunks)packages/steam/SteamService.cs
(5 hunks)packages/steam/index.ts
(1 hunks)packages/steam/package.json
(1 hunks)packages/steam/steam.csproj
(1 hunks)packages/steam/steam.http
(1 hunks)packages/steam/tsconfig.json
(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/steam/steam.http
- nestri.sln
- packages/steam/README.md
- packages/steam/Migrations/20250408125920_RemoveTeam.cs
- packages/steam/Migrations/20250408125920_RemoveTeam.Designer.cs
- packages/steam/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/steam/.gitignore
- packages/steam/steam.csproj
- packages/steam/Properties/launchSettings.json
- packages/steam/DBContext.cs
🧰 Additional context used
🧬 Code Definitions (3)
packages/steam/Program.cs (4)
packages/steam/SteamService.cs (6)
SteamService
(5-154)SteamService
(11-14)Task
(25-53)Task
(55-90)Task
(92-128)Unsubscribe
(140-143)packages/steam/DBContext.cs (2)
SteamDbContext
(3-18)SteamDbContext
(7-9)packages/steam/SteamClient.cs (2)
Task
(40-151)Task
(153-187)packages/steam/SSEevent.cs (1)
Serialize
(14-18)
packages/steam/Auth.cs (3)
packages/steam/Configuration.cs (1)
SteamConfig
(3-14)packages/steam/SteamClient.cs (5)
OnLoggedOn
(305-350)OnConnected
(200-279)OnDisconnected
(281-303)Task
(40-151)Task
(153-187)packages/steam/SteamService.cs (3)
Task
(25-53)Task
(55-90)Task
(92-128)
packages/steam/SteamClient.cs (2)
packages/steam/SSEevent.cs (2)
ServerSentEvent
(3-19)ServerSentEvent
(8-12)packages/steam/SteamService.cs (1)
SteamUserInfo
(145-153)
🪛 Biome (1.9.4)
packages/steam/index.ts
[error] 8-8: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (22)
packages/steam/Migrations/SteamDbContextModelSnapshot.cs (2)
12-12
: Good renaming to make the context more specificThe class name change from
AppDbContextModelSnapshot
toSteamDbContextModelSnapshot
better reflects the specific purpose of this context, making the code more self-descriptive.
45-46
: Appropriate index simplificationThe index has been correctly simplified to only include
UserId
after the removal ofTeamId
from the model. This change maintains the uniqueness constraint while streamlining the data model.packages/steam/Migrations/20250408090438_InitialCreate.cs (1)
31-35
: Correctly updated index definitionThe index has been properly updated to focus solely on
UserId
with a unique constraint, removing the previous composite index dependency onTeamId
. This change aligns with the schema simplification and maintains the appropriate uniqueness constraints.packages/functions/src/api/realtime/index.ts (1)
1-4
: Good library selection for real-time communicationThe import choices for actor-core and related libraries provide a solid foundation for implementing real-time features.
packages/steam/Migrations/20250408090438_InitialCreate.Designer.cs (2)
13-13
: Updated migration identifierThe migration identifier has been properly updated to reflect the new version of the migration.
48-49
: Consistent index simplificationThe index definition has been correctly simplified to only include
UserId
with a unique constraint, consistent with the changes in other migration files.packages/steam/Program.cs (1)
25-30
: Extended timeout settings may risk resource exhaustionThe Kestrel server is configured with a 10-minute keep-alive timeout and 5-minute request header timeout. These lengthy timeouts could lead to resource exhaustion if many connections are made but not properly closed.
Consider monitoring resource usage and reducing these timeouts if needed, especially in high-traffic scenarios.
packages/functions/src/api/index.ts (3)
27-27
: Newly added realtime route implementationThe addition of a new route for
realtime
API is a significant architectural change that introduces real-time communication capabilities to the application.Good implementation of this feature, adding the real-time route. Make sure to test for authentication, rate limiting, and handling of connection drops.
95-104
: Well-structured default export with extended configurationThe new default export with configuration settings for port, idle timeout, WebSocket handler, and a fetch method is a good architectural decision that improves flexibility for handling incoming requests.
This change aligns well with the shift from HTTP-based routing to socket-based communication as seen in the Steam service.
87-87
: Updated sandbox URLUpdated the sandbox server URL for the API documentation, which is a good update to keep the documentation in sync with the actual environments.
packages/steam/SteamClient.cs (4)
52-55
: Enhanced user feedback with detailed status notificationsThe addition of multiple ServerSentEvent notifications greatly improves user experience by providing real-time feedback about the authentication process status.
These notifications will enhance frontend feedback during the Steam authentication flow, which aligns with the addition of the play button on the homepage mentioned in the PR objectives.
Also applies to: 76-79, 97-100, 130-133, 204-207, 219-222, 239-242, 259-262, 272-275, 285-288, 297-300, 311-314, 356-359
251-251
: Machine name set to "Nestri OS"When logging in to Steam, the machine name is now set to "Nestri OS", which is a nice touch for branding in Steam's login records.
This will make your service more recognizable in users' Steam login history.
224-224
:❓ Verification inconclusive
Changed event type for QR code URLs
The event type for QR code URLs has been updated from "url" to "challenge_url", which is more descriptive and specific.
Ensure that frontend code listening for these events has been updated accordingly:
Also applies to: 228-228, 378-378
🏁 Script executed:
#!/bin/bash # Search for frontend code that might be listening to the old "url" event type echo "Checking for code listening to the old 'url' event type:" rg "\"url\".*addEventListener" --type=ts --type=js # Search for code handling the new "challenge_url" event type echo "Checking for code handling the new 'challenge_url' event type:" rg "\"challenge_url\".*addEventListener" --type=ts --type=jsLength of output: 381
Frontend Event Listener Update Verification
The event type for QR code URLs was updated from
"url"
to"challenge_url"
in the backend (packages/steam/SteamClient.cs
, lines 224, 228, and 378). Our automated search did not return any instances of event listeners for either"url"
or"challenge_url"
, so it wasn’t possible to conclusively verify the frontend updates. Please manually confirm that the frontend code has been updated to listen for the"challenge_url"
event instead of the old"url"
and that this change is applied consistently.
105-105
:❓ Verification inconclusive
Modified SteamId type from string to ulong?
The SteamId property in SteamUserInfo has been changed from string to ulong?, and all assignments now convert SteamID values to this type using ConvertToUInt64().
Make sure that any code consuming SteamUserInfo.SteamId has been updated to handle this type change. Run the following to check for potential issues:
Also applies to: 334-334, 434-434
🏁 Script executed:
#!/bin/bash # Find all references to SteamId in typescript/javascript files echo "Checking references to SteamId in TS/JS files:" rg "steamId" --type=ts --type=js # Find all references to SteamId in C# files echo "Checking references to SteamId in C# files:" rg "SteamId" --type=csLength of output: 671
Action Required: Confirm Consuming Code Correctly Handles the SteamId Type Change
The verification shows that the SteamId property is now defined as a nullable ulong in
packages/steam/SteamClient.cs
and that its conversion is consistently performed viaConvertToUInt64()
. References in the repository—specifically inpackages/steam/Program.cs
—assignuserInfo.SteamId
directly to local variables. There are no TS/JS references, which is expected.Please ensure that:
- Any code in
packages/steam/Program.cs
(and elsewhere) consumingSteamUserInfo.SteamId
correctly handles the nullableulong
(e.g., appropriate null checking and any conversions needed if the value is to be used as a string or a non-nullable numeric type).- The changes also propagate to lines 334 and 434 as noted.
packages/steam/SteamService.cs (4)
92-128
: Simplified user info retrieval without teamIdThe GetUserInfoFromStoredCredentials method also removes the teamId parameter, consistent with the other methods. This creates a cleaner API but assumes users are unique across all teams.
The consistent removal of teamId from authentication methods creates a simpler and more intuitive API, though it assumes globally unique user IDs.
25-53
:✅ Verification successful
Simplified authentication by removing teamId parameter
The StartAuthentication method has been simplified by removing the teamId parameter, which streamlines the authentication process. All related method calls and clientId construction now only use userId.
This is a breaking change in the method signature. Ensure all callers have been updated:
🏁 Script executed:
#!/bin/bash # Search for calls to StartAuthentication to check if they've been updated echo "Checking calls to StartAuthentication:" rg "StartAuthentication\(" --type=cs --type=ts --type=js -A 2 -B 2Length of output: 868
Authentication Method Signature Updated – Verified Caller Updates
The breaking change of removing the
teamId
parameter fromStartAuthentication
has been verified. The method now only requiresuserId
, and our search confirms that all call sites—such as the one inpackages/steam/Program.cs
—use the updated single-parameter signature.
- File:
packages/steam/SteamService.cs
(Lines 25-53) now defines:public async Task StartAuthentication(string userId)- Caller verified: For example,
packages/steam/Program.cs
calls:await steamService.StartAuthentication(userID!);No further changes are required.
55-90
:❓ Verification inconclusive
Simplified credential storage by removing teamId
The SaveCredentials method no longer includes teamId parameter or field in the database records. This simplifies the data model but assumes users are unique across all teams.
Verify that the database schema has been updated accordingly and check for any TeamId references:
🏁 Script executed:
#!/bin/bash # Search for migration files that might update the schema echo "Checking migration files for schema changes:" fd "Migration" --type f # Check for TeamId references in SteamUserCredentials model echo "Checking SteamUserCredentials model for TeamId references:" rg "TeamId" --type=cs --path-glob "*SteamUserCredentials.cs"Length of output: 756
Action Required: Re-verify the complete removal of TeamId across the database schema and model.
The updated
SaveCredentials
method now omits theteamId
parameter and related updates. To ensure the database schema and theSteamUserCredentials
model reflect this change, please verify that no TeamId references remain. The initial verification script using--path-glob
failed; therefore, re-run the following commands to check for any lingering references:#!/bin/bash echo "Searching for TeamId references in SteamUserCredentials.cs files:" fd "SteamUserCredentials.cs" --type file | xargs rg "TeamId" echo "Searching for TeamId references in migration files:" fd "Migration" --type file | xargs rg "TeamId"If these commands return no output, it confirms that the TeamId column and its usage have been successfully removed. Please manually review the outputs to verify that the changes are complete.
27-27
:❓ Verification inconclusive
Simplified clientId construction
ClientId is now constructed simply as the userId string without combining with teamId. This change supports the architectural shift but assumes unique userIds across the entire system.
Also applies to: 94-94
🌐 Web query:
What are best practices for ensuring unique user IDs across a multi-tenant system?
💡 Result:
To ensure unique user IDs across a multi-tenant system, follow these best practices based on industry standards and architectural guidelines:
1. Prefer Single Identity Per User
- Use one identity per user across all tenants whenever possible. This simplifies management and enhances security by leveraging modern IdP features like threat detection and MFA[1][5].
- Avoid creating separate user identities for each tenant unless required by regulatory or operational needs (e.g., strict data isolation laws)[1][3].
2. Combine Tenant and User Identifiers
- Append a tenant-specific prefix (e.g., tenant ID or name) to the IdP-provided user ID to ensure global uniqueness. For example:
user_id = tenant_id + "|" + idp_user_id
[3][5].
This prevents collisions across tenants while retaining the IdP’s per-tenant uniqueness guarantee[3][9].3. Centralized Identity Management
- Use a centralized IdP (e.g., Auth0 Organizations, Azure AD) to manage cross-tenant identities, avoiding credential duplication[1][4].
- Store tenant-specific authorization data (roles, permissions) separately from authentication credentials[1][8].
4. Database Design and Enforcement
- Composite keys: Use
(tenant_id, user_id)
as a composite primary key in user tables to enforce uniqueness at the database layer[10].- Row-Level Security (RLS): Automatically scope queries to the user’s tenant using RLS policies, ensuring data isolation without manual filtering[10].
Example PostgreSQL RLS policy:CREATE POLICY tenant_policy ON users USING (tenant_id = current_setting('app.current_tenant')::UUID);5. Avoid Credential Redundancy
- Never store credentials multiple times for the same user across tenants. Use guest identities or IdP features like Azure AD B2B to link a single credential set to multiple tenants[1][5].
6. Leverage IdP Features for Multi-Tenancy
- Auth0 Organizations: Assign users to tenant-specific organizations with isolated roles and permissions[4][5].
- Dynamic token claims: Include
tenant_id
in access tokens to simplify authorization checks in APIs and microservices[8][10].7. Handling Multi-Tenant Access
- For users needing access to multiple tenants:
- Use role-based access control (RBAC) with tenant-scoped roles[6][7].
- Implement a tenant-switching mechanism in the UI/API, validated against the user’s authorized tenants[1][5].
8. Regulatory and Compliance Considerations
- For tenants requiring strict isolation (e.g., GDPR), use tenant-specific IdP instances or separate database schemas, but still combine identifiers for global uniqueness[1][8].
Example Workflow
- User Signs Up:
- The IdP (e.g., Auth0) issues a unique
user_id
scoped to the tenant.- The application stores
user_id
astenant123|auth0_abcd1234
[3][5].- Authorization:
- Access tokens include
tenant_id
anduser_id
claims[8][10].- Backend services validate access using RLS or tenant-specific policies[10].
By integrating these practices, you ensure scalable, secure, and compliant user identity management in multi-tenant systems.
Citations:
- 1: https://learn.microsoft.com/en-us/azure/architecture/guide/multitenant/considerations/identity
- 2: https://jumpcloud.com/blog/multi-tenant-user-management
- 3: https://community.auth0.com/t/ensuring-unique-user-ids-in-auth0-across-multiple-tenants/120970
- 4: https://auth0.com/docs/get-started/auth0-overview/create-tenants/multi-tenant-apps-best-practices
- 5: https://belitsoft.com/multitenant-saas-identity-access-control-management
- 6: https://qrvey.com/blog/multi-tenant-security/
- 7: https://www.permit.io/blog/best-practices-for-multi-tenant-authorization
- 8: https://curity.io/blog/manage-multi-tenancy-in-iam-system/
- 9: https://developer.monday.com/apps/docs/multitenancy
- 10: https://blog.devops.dev/best-practices-for-database-authorization-in-multi-tenant-systems-001a1bcf2568
ClientId Construction Simplification & Uniqueness Assumption
The updated assignment:
var clientId = $"{userId}";now derives the clientId solely from the userId. This change supports the architectural shift by assuming that userIds are globally unique across the system. However, best practices for multi-tenant architectures recommend ensuring uniqueness by either:
- Enforcing a single identity per user with a centralized IdP, or
- Combining a tenant (or team) identifier with the userId (e.g., using a composite key or a tenant-specific prefix)
to guard against collisions if the system might host identities from multiple tenants.
Action Items:
- Verify that the system guarantees globally unique userIds (e.g., via a centralized IdP or database constraints).
- Consider revisiting the clientId construction if there's any risk of userId collisions in a multi-tenant environment.
This update applies similarly to line 94.
packages/steam/Auth.cs (4)
1-4
: Confirm SteamKit2 versioning and references.
It's good practice to ensure compatibility and security by verifying that theSteamKit2
andSteamKit2.Authentication
libraries are at recommended versions, free of known vulnerabilities.Would you like to run a script to check whether the referenced NuGet packages are up to date?
5-30
: Constructor initializes the Steam components correctly.
The setup of_steamClient
,_manager
, and_steamUser
looks correct, and registering callbacks is a standard approach. Just confirm that disposing or tearing down these callbacks when the component is no longer in use is not needed to avoid potential memory leaks.
139-148
: SSE updates for QR challenge URLs.
Sending the challenge URL through a dedicated function is neat. Make sure the event name (challenge_url
) is consistently monitored client‑side and can handle repeated re‑emissions if the URL is refreshed multiple times.
196-204
: Disconnect logic gracefully stops callbacks.
Setting_isRunning = false
andTrySetResult(false)
will terminate the callback loop, preventing resource leakage. This approach looks well-structured.
method: 'GET', | ||
headers: { | ||
'Accept': 'text/event-stream', | ||
'user-id': 'usr_XXXXXXXXXXXXXXX' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove hardcoded user ID in headers
The code contains a hardcoded user ID in the request headers. This should be parameterized to make the class reusable.
headers: {
'Accept': 'text/event-stream',
- 'user-id': 'usr_XXXXXXXXXXXXXXX'
+ 'user-id': this.userId || 'anonymous'
}
Consider adding a userId
property to the class and allowing it to be set via constructor or a setter 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.
'user-id': 'usr_XXXXXXXXXXXXXXX' | |
headers: { | |
'Accept': 'text/event-stream', | |
'user-id': this.userId || 'anonymous' | |
} |
packages/steam/Configuration.cs
Outdated
public class SteamConfig | ||
{ | ||
public static SteamConfiguration GetDefaultSteamClientConfig() | ||
{ | ||
return SteamConfiguration.Create(config => | ||
{ | ||
config.WithConnectionTimeout(TimeSpan.FromSeconds(30)); | ||
config.WithProtocolTypes(ProtocolTypes.Tcp | ProtocolTypes.WebSocket); | ||
} | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add XML documentation and consider parameterizing the configuration
The class lacks documentation, and the configuration values are hardcoded. Consider adding XML documentation and parameterizing timeout and protocol types.
using SteamKit2;
+using System;
+/// <summary>
+/// Provides configuration for Steam client connections.
+/// </summary>
public class SteamConfig
{
+ /// <summary>
+ /// Creates a default Steam client configuration with standard settings.
+ /// </summary>
+ /// <param name="timeoutSeconds">Connection timeout in seconds. Default is 30.</param>
+ /// <param name="protocolTypes">Allowed protocol types. Default is TCP and WebSocket.</param>
+ /// <returns>A configured SteamConfiguration instance.</returns>
- public static SteamConfiguration GetDefaultSteamClientConfig()
+ public static SteamConfiguration GetDefaultSteamClientConfig(
+ int timeoutSeconds = 30,
+ ProtocolTypes protocolTypes = ProtocolTypes.Tcp | ProtocolTypes.WebSocket)
{
return SteamConfiguration.Create(config =>
{
- config.WithConnectionTimeout(TimeSpan.FromSeconds(30));
- config.WithProtocolTypes(ProtocolTypes.Tcp | ProtocolTypes.WebSocket);
+ config.WithConnectionTimeout(TimeSpan.FromSeconds(timeoutSeconds));
+ config.WithProtocolTypes(protocolTypes);
}
);
}
}
📝 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 class SteamConfig | |
{ | |
public static SteamConfiguration GetDefaultSteamClientConfig() | |
{ | |
return SteamConfiguration.Create(config => | |
{ | |
config.WithConnectionTimeout(TimeSpan.FromSeconds(30)); | |
config.WithProtocolTypes(ProtocolTypes.Tcp | ProtocolTypes.WebSocket); | |
} | |
); | |
} | |
} | |
using SteamKit2; | |
using System; | |
/// <summary> | |
/// Provides configuration for Steam client connections. | |
/// </summary> | |
public class SteamConfig | |
{ | |
/// <summary> | |
/// Creates a default Steam client configuration with standard settings. | |
/// </summary> | |
/// <param name="timeoutSeconds">Connection timeout in seconds. Default is 30.</param> | |
/// <param name="protocolTypes">Allowed protocol types. Default is TCP and WebSocket.</param> | |
/// <returns>A configured SteamConfiguration instance.</returns> | |
public static SteamConfiguration GetDefaultSteamClientConfig( | |
int timeoutSeconds = 30, | |
ProtocolTypes protocolTypes = ProtocolTypes.Tcp | ProtocolTypes.WebSocket) | |
{ | |
return SteamConfiguration.Create(config => | |
{ | |
config.WithConnectionTimeout(TimeSpan.FromSeconds(timeoutSeconds)); | |
config.WithProtocolTypes(protocolTypes); | |
} | |
); | |
} | |
} |
packages/steam/Auth.cs
Outdated
public async Task HandleLoginRequest(LoginRequest loginRequest) | ||
{ | ||
_authCompletionSource = new TaskCompletionSource<bool>(); | ||
|
||
// Connect to Steam | ||
await SendToClient("status", new { message = "Connecting to Steam..." }); | ||
_steamClient.Connect(); | ||
|
||
// Start callback handling loop in a separate task | ||
await Task.Run(() => | ||
{ | ||
while (_isRunning) | ||
{ | ||
_manager.RunWaitCallbacks(TimeSpan.FromSeconds(1)); | ||
} | ||
}); | ||
|
||
// Wait for authentication to complete | ||
await _authCompletionSource.Task; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential infinite loop in callback handling.
When calling _manager.RunWaitCallbacks
in a while (_isRunning)
loop, confirm that _isRunning
is guaranteed to be set to false
in all cases (e.g., on error/cleanup) to prevent a stuck task. Currently, it is toggled in OnDisconnected
, but make sure there are no other code paths that leave the loop running indefinitely.
packages/steam/Auth.cs
Outdated
{ | ||
if (callback.Result != EResult.OK) | ||
{ | ||
await SendToClient("error", new | ||
{ | ||
message = $"Unable to log on to Steam: {callback.Result} / {callback.ExtendedResult}" | ||
}); | ||
|
||
_authCompletionSource!.SetResult(false); | ||
return; | ||
} | ||
|
||
await SendToClient("status", new { message = "Successfully logged on to Steam" }); | ||
|
||
_authCompletionSource!.SetResult(true); | ||
|
||
// _steamUser.LogOff(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Authentication state finalization.
In OnLoggedOn
, calling _authCompletionSource.SetResult(true)
is a nice way to unblock waiting tasks. If you expect multiple logins in the same object lifecycle, consider re-initializing _authCompletionSource
. Otherwise, the second login might never set a fresh completion source.
packages/steam/Auth.cs
Outdated
public class LoginRequest | ||
{ | ||
public required string Type { get; set; } | ||
public string? Username { get; set; } | ||
public string? RefreshToken { get; set; } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
LoginRequest fields could use server-side validation.
The Type
, Username
, and RefreshToken
fields are user-supplied. Implement validation or sanitation on server side to prevent malformed inputs from causing errors in the authentication flow.
@dependabot rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
packages/steam/SteamClient.cs (2)
329-330
:⚠️ Potential issuePotential performance issue with duplicate API calls
The code calls
PollingWaitForResultAsync().Result
twice, which could trigger duplicate API calls. This should be refactored to store the result in a variable.- string accountName = _authSession?.PollingWaitForResultAsync().Result.AccountName ?? "Unknown"; - string refreshToken = _authSession?.PollingWaitForResultAsync().Result.RefreshToken ?? ""; + var authResult = _authSession?.PollingWaitForResultAsync().Result; + string accountName = authResult?.AccountName ?? "Unknown"; + string refreshToken = authResult?.RefreshToken ?? "";Additionally, using
.Result
on an async method can cause deadlocks. Consider making this method async or storing the result earlier in the authentication flow.
145-150
: 💡 Verification agent🧩 Analysis chain
Commented-out cleanup code may lead to resource leaks
The commented-out cleanup code would have unsubscribed event handlers. Without this cleanup, you might be creating multiple subscriptions that never get removed.
Are you handling the unsubscription of these event handlers elsewhere? If not, consider uncommenting and implementing the cleanup to prevent potential resource leaks.
🏁 Script executed:
#!/bin/bash # Check if there are any other places in the codebase where the connectedHandler and loggedOnHandler are unsubscribed rg -A 2 -B 2 "Unsubscribe\(connectedHandler\)" --type cs rg -A 2 -B 2 "Unsubscribe\(loggedOnHandler\)" --type csLength of output: 711
Attention: Unsubscribe Cleanup Missing
The search confirms that neitherconnectedHandler
norloggedOnHandler
are unsubscribed anywhere else in the codebase. Leaving the unsubscribe calls commented out (as seen inpackages/steam/SteamClient.cs
, Lines 145-150) may lead to resource leaks due to multiple lingering subscriptions.Action:
- Please uncomment and properly implement the cleanup code to unsubscribe these event handlers, or otherwise ensure they are cleaned up appropriately to prevent potential memory/resource leaks.
🧹 Nitpick comments (8)
packages/steam/SteamClient.cs (2)
212-212
: Consider adding device identification configurationThe device friendly name "Nestri OS" is hardcoded. Consider making this configurable, especially if this code will be used in different contexts or environments.
- _authSession = await _steamClient.Authentication.BeginAuthSessionViaQRAsync(new AuthSessionDetails() { DeviceFriendlyName = "Nestri OS" }); + _authSession = await _steamClient.Authentication.BeginAuthSessionViaQRAsync(new AuthSessionDetails() { DeviceFriendlyName = _deviceName ?? "Nestri OS" });Add a private field and constructor parameter:
private readonly string? _deviceName; public SteamClientHandler(string clientId, Action<string, string>? onCredentialsObtained = null, string? deviceName = null) { _clientId = clientId; _onCredentialsObtained = onCredentialsObtained; _deviceName = deviceName; // ... }
251-251
: Ensure consistent device namingThe machine name "Nestri OS" is hardcoded here, but should use the same value as the device name in
BeginAuthSessionViaQRAsync
. Consider using a constant or configuration property for consistency.- MachineName = "Nestri OS", + MachineName = _deviceName ?? "Nestri OS",packages/functions/src/auth.ts (5)
28-29
: Remove or protect debug logging in production.Logging environment variable names and values could potentially expose sensitive configuration information in production logs.
-console.log("STORAGE", process.env.STORAGE) +// Only log in development environment +if (process.env.NODE_ENV === "development") { + console.log("STORAGE", process.env.STORAGE) +}
31-34
: Validate environment variables before usage.The storage persistence relies directly on an environment variable without validation, which could lead to unexpected behavior if the variable is undefined or contains invalid values.
storage: MemoryStorage({ - persist: process.env.STORAGE //"/tmp/persist.json", + persist: process.env.STORAGE || "/tmp/persist.json", }),
49-49
: Consider self-hosting the Geist font for better performance.External font imports can impact page load times. Consider preloading or self-hosting the font.
135-140
: Enhance TODO comment with specific action items.The TODO comment lacks clarity on what specific implementation is needed. Consider adding more details about what needs to be done and who is responsible.
-// TODO: This works, so use this while registering the task +// TODO: Use these geolocation headers when registering the task to track user location +// Implementation should include storing these values in the user profile
228-236
: Document service configuration parameters.The exported service configuration includes specific values for port and idleTimeout without explanation. Adding comments would improve maintainability.
export default { + // Port for the auth service to listen on port: 3002, + // Keep-alive timeout in seconds for the auth service idleTimeout: 255, fetch: (req: Request) => app.fetch(req, undefined, { waitUntil: (fn) => fn, passThroughOnException: () => { }, }), };packages/functions/src/api/index.ts (1)
12-12
: Purpose of patchLogger needs clarificationYou've imported and called
patchLogger()
but its purpose isn't clear from the context. Consider adding a comment explaining what this function does and why it's necessary.import { ErrorCodes, VisibleError } from "@nestri/core/error"; +// Import patchLogger to ensure consistent logging across environments import { patchLogger } from "../log-polyfill"; // ... +// Apply logging polyfills for environment compatibility patchLogger();Also applies to: 95-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
infra/www.ts
(0 hunks)packages/core/package.json
(1 hunks)packages/core/src/actor.ts
(4 hunks)packages/core/src/examples.ts
(2 hunks)packages/core/src/member/index.ts
(1 hunks)packages/core/src/team/index.ts
(1 hunks)packages/core/src/user/index.ts
(1 hunks)packages/core/src/utils/id.ts
(1 hunks)packages/functions/src/api/auth.ts
(3 hunks)packages/functions/src/api/index.ts
(2 hunks)packages/functions/src/auth.ts
(5 hunks)packages/functions/sst-env.d.ts
(1 hunks)packages/steam/SteamClient.cs
(13 hunks)sst-env.d.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- infra/www.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/core/package.json
- packages/core/src/team/index.ts
- packages/core/src/member/index.ts
- packages/core/src/user/index.ts
- packages/core/src/utils/id.ts
- packages/core/src/examples.ts
- sst-env.d.ts
- packages/functions/src/api/auth.ts
- packages/functions/sst-env.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/functions/src/auth.ts (2)
packages/functions/src/ui/select.tsx (1)
Select
(15-65)packages/functions/src/log-polyfill.ts (1)
patchLogger
(14-27)
🔇 Additional comments (12)
packages/steam/SteamClient.cs (4)
52-55
: Enhanced status reporting improves observability throughout the authentication flowThe addition of status notifications via
ServerSentEvent
throughout the authentication process significantly improves the user experience and debuggability. These statuses provide a more granular view of the connection, authentication, and error states.Also applies to: 76-79, 97-100, 130-133, 171-174, 204-207, 219-222, 239-242, 259-262, 272-275, 285-288, 297-300, 311-314, 356-359
105-105
: Appropriate type conversion for SteamIdThe conversion of SteamId from string to unsigned long using
ConvertToUInt64()
is a good change that aligns with Steam's actual ID format. This enables more efficient operations with the ID as a numeric value rather than a string.Also applies to: 334-334, 341-341
224-224
: Better event type naming for QR challenge URLChanging from "url" to "challenge_url" makes the event type more specific and descriptive, which improves code readability and reduces potential confusion with other URL-related events.
Also applies to: 378-378
434-434
: Type change for SteamId property aligns with Steam's data modelChanging
SteamId
fromstring
toulong?
is appropriate as it correctly represents Steam's numeric ID format, enabling more efficient operations and proper type safety.packages/functions/src/auth.ts (2)
6-6
: Authentication logging and storage changes look good.The addition of
patchLogger
from log-polyfill and the newMemoryStorage
implementation enhance the authentication system with better logging and more flexible storage options. ThepatchLogger()
call at line 226 ensures consistent log formatting across the authentication service.Also applies to: 16-16, 226-226
14-14
: Explicit import of PasswordAdapter is a good addition.Adding the explicit import for PasswordAdapter improves code clarity and maintainability.
packages/core/src/actor.ts (4)
3-3
: Improved error handling with specific error codes in useUserIDThe change from "unauthorized" to "authentication" as the error type and addition of
ErrorCodes.Authentication.UNAUTHORIZED
makes error handling more specific and consistent. This will help with error filtering and providing appropriate responses to the client.Also applies to: 77-80
83-101
: Well-implemented useUser function with proper error handlingNice addition of the
useUser
function that provides access to all user properties while maintaining consistent error handling withuseUserID
. This promotes code reuse and reduces duplication in places where multiple user properties are needed.
145-146
: Improved error handling with specific error codes in assertUserFlagGood update from "user.flags" to "not_found" error type and addition of
ErrorCodes.Validation.MISSING_REQUIRED_FIELD
error code. This makes the error more descriptive and allows for better error categorization.
63-72
: Excellent JSDoc documentation for functionsThe comprehensive JSDoc comments added to
useUserID
,useUser
, andassertUserFlag
significantly improve code readability and maintainability. They clearly explain function purpose, parameters, return values, and potential errors.Also applies to: 83-92, 124-134
packages/functions/src/api/index.ts (2)
97-106
: Server configuration and architecture changeGood implementation of the default export with server configuration. This structure is more suitable for a containerized service-oriented model as mentioned in the PR summary. The webSocketHandler integration for Realtime functionality is particularly valuable for real-time features.
6-6
:❓ Verification inconclusive
Inconsistency between PR objective and implementation
The PR objective mentions adding Steam authentication, but the SteamApi import and route are commented out. Meanwhile, you've added Realtime functionality. Is this intentional or should the Steam functionality also be included?
Please verify if:
- The Steam functionality is implemented elsewhere
- This PR includes both Realtime and Steam features but the Steam API is intentionally disabled
- The PR description needs to be updated to match the actual implementation
Also applies to: 8-8, 28-28, 30-30
🏁 Script executed:
#!/bin/bash # Check if Steam-related functionality exists in the codebase echo "Looking for Steam-related files..." fd -t f "steam" --exec grep -l "Steam" {} \; echo "Looking for Steam authentication in imported Realtime module..." if [ -f "packages/functions/src/api/realtime/index.ts" ]; then cat "packages/functions/src/api/realtime/index.ts" | grep -i "steam" fiLength of output: 675
Clarify Steam Authentication Scope
Our investigation shows several Steam-related files exist in the repository (e.g., in
infra/steam.ts
,packages/www/src/providers/steam.tsx
, and some components underpackages/steam
), but the current implementation inpackages/functions/src/api/index.ts
has the SteamApi import (and similar references on lines 8, 28, and 30) commented out. Additionally, a search within the realtime module (packages/functions/src/api/realtime/index.ts
) produced no Steam-related code.Could you please confirm:
- Is the omission of Steam authentication in the realtime module intentional (i.e., are you planning to only ship realtime functionality in this PR)?
- Should the PR description be updated to reflect that only realtime features are being added, or is Steam functionality expected to be implemented as part of this work?
Thanks for clarifying.
waitUntil: (fn) => fn, | ||
passThroughOnException: () => { }, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Review error handling behavior of passThroughOnException
.
The current implementation of passThroughOnException
is an empty function. This could potentially hide errors instead of handling them properly. Consider adding logging or other error handling mechanisms.
🏁 Script executed:
#!/bin/bash
# Check if there are other instances of passThroughOnException in the codebase
# to understand if this is a consistent pattern
echo "Searching for other instances of passThroughOnException..."
rg "passThroughOnException" --type ts
Length of output: 325
Improve error logging in passThroughOnException
The current implementation of passThroughOnException
(an empty function) may lead to silent failures, obscuring possible errors. This pattern appears consistently in both:
packages/functions/src/auth.ts
(lines 233-235)packages/functions/src/api/index.ts
Please consider one of the following:
- Implement a logging mechanism or another appropriate error-handling strategy.
- If the empty implementation is intentional, add a clarifying comment to explain why no logging or error propagation is performed.
@@ -1,31 +1,36 @@ | |||
import "zod-openapi/extend"; | |||
import { Hono } from "hono"; | |||
import { auth } from "./auth"; | |||
import { cors } from "hono/cors"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
CORS middleware added without specific configuration
You've added CORS middleware, which is good for cross-origin requests. However, consider specifying CORS configuration options rather than using defaults, which may be too permissive for production use.
- .use(cors())
+ .use(cors({
+ origin: ['https://nestri.io', 'https://dev.nestri.io'],
+ allowHeaders: ['Authorization', 'Content-Type', 'x-nestri-team'],
+ allowMethods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'],
+ exposeHeaders: ['Content-Length'],
+ maxAge: 86400,
+ }))
Also applies to: 19-19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/steam/Program.cs (3)
9-16
: Revisit the Unix socket location for security.Storing the socket in
/tmp
can be unsafe on multi-user systems. A more restrictive directory (e.g.,/var/run/nestri/
) is recommended.
35-38
: Database file path in/tmp
is insecure.Past feedback also highlighted this issue. Consider storing
steam.db
in a more secure location (e.g.,/var/run/nestri/
).
66-82
: Consider handling disconnections or exceptions in the SSE loop.This endpoint does not verify if the client remains connected, potentially causing resource usage if the client disconnects prematurely.
🧹 Nitpick comments (25)
packages/steam/AuthSchema.cs (1)
1-9
: Consider additional protection for sensitive data.The
SteamUserCredentials
class is well-structured with required properties and proper timestamp handling using UTC. However, since it stores sensitive authentication data, consider:
- Adding encryption for the
RefreshToken
property- Including string length constraints (e.g.,
[MaxLength(255)]
) to prevent overly large inputs- Adding documentation for how long refresh tokens are valid
+using System.ComponentModel.DataAnnotations; public class SteamUserCredentials { public int Id { get; set; } // Keep as primary key + [MaxLength(255)] public required string UserId { get; set; } + [MaxLength(255)] public required string AccountName { get; set; } + [MaxLength(1024)] public required string RefreshToken { get; set; } public DateTime CreatedAt { get; set; } = DateTime.UtcNow; public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; }packages/steam/AccountInfoSchema.cs (1)
1-22
: Comprehensive model with privacy considerations.The
SteamAccountInfo
class is well-designed with appropriate nullable properties and good use of data types. Some considerations:
Since you're storing potentially sensitive information like email addresses:
- Consider documenting data retention policies
- Ensure GDPR compliance if this applies to your users
- Consider whether all these fields are necessary
Adding documentation explaining how these fields map to Steam API responses would be helpful for future developers.
+using System.ComponentModel.DataAnnotations; +/// <summary> +/// Represents information about a Steam account retrieved from the Steam API. +/// </summary> public class SteamAccountInfo { public int Id { get; set; } + [MaxLength(255)] public required string UserId { get; set; } + [MaxLength(255)] public string? Username { get; set; } public ulong? SteamId { get; set; } + [MaxLength(255)] public string? Email { get; set; } + [MaxLength(50)] public string? Country { get; set; } + [MaxLength(255)] public string? PersonaName { get; set; } public bool? IsLocked { get; set; } public bool? IsBanned { get; set; } public bool? IsLimited { get; set; } public bool? IsAllowedToInviteFriends { get; set; } + [MaxLength(1024)] public string? AvatarUrl { get; set; }packages/steam/Program.cs (3)
45-64
: Remove or clarify the large commented-out block.If this code is no longer needed, removing it improves maintainability. If needed later, consider feature flags or partial implementation.
- // app.MapGet("/status", ... - // ... - // } + // Remove this block if it's no longer necessary
278-314
: Validate user headers.Relying on
user-id
from headers could pose security or trust issues if not accurately validated.
316-324
: Use caution with automatic migrations in production.Invoking
dbContext.Database.Migrate()
at runtime can have unexpected effects on a live system. Consider migration orchestration externally.packages/steam/AccountLimited.cs (1)
6-44
: Console logging could mask more structured logging.Using
Console.WriteLine
is acceptable for debugging but consider a structured logging framework (e.g.,ILogger
) for production.packages/steam/SteamAuth.cs (4)
1-34
: Watch for potential token/credential exposure.Logging
_username
or_refreshToken
can leak sensitive info. Avoid printing them in plain text to prevent accidental PII leakage.
200-214
: Reconnect delay mismatch.The console message says 500 seconds, but the code delays for 30 seconds. Align them for consistent behavior.
- message = $"Reconnecting in 500 seconds..." + message = $"Reconnecting in 30 seconds..."
303-319
: Account details capturing is efficient; ensure no PII leaks in logs.Storing user’s email, country, etc., is helpful; double-check logs to prevent unintentional disclosure.
556-620
: Saving account info to DB is well-structured.The approach of creating/updating records is good. Consider adding try/catch for concurrency conflicts if multiple sessions update the same record.
packages/steam/DBContext.cs (1)
15-17
: Update the comment to accurately reflect removal of TeamId.
The explanatory comment on line 14 is now obsolete—it references a unique index on TeamId + UserId, but the code references only UserId.- // Create a unique index on TeamId and UserId + // Create a unique index on UserIdpackages/steam/index.ts (6)
5-8
: Parameterize or inject user ID if needed.
Currently,httpClient
andsocketPath
are declared, but there's no direct mention of a user ID property. Later in the code,'user-id': 'usr_XXXXXXXXXXXXXXX'
is hardcoded. Consider passing the user ID into this class for more flexibility.
9-9
: Remove the empty constructor.
According to static analysis hints, this constructor is unnecessary if it’s not performing any initialization.-class Steam { - httpClient: http.ClientRequest | null = null; - socketPath: string = '/tmp/steam.sock'; - constructor() { } +class Steam { + httpClient: http.ClientRequest | null = null; + socketPath: string = '/tmp/steam.sock';🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
69-130
: Refine console usage and correct typos in event handling.
Some successful events are logged viaconsole.error
, and there's a small typo ("succesful" → "successful"). Usingconsole.log
for successful events lowers confusion.case 'login-attempt': - console.error(`Attempting to login to Steam\n`); + console.log(`Attempting to login to Steam\n`); break; case 'login-successful': - console.error(`Login succesful: ${data.username}\n`); + console.log(`Login successful: ${data.username}\n`); break;
132-165
: Handle HTTP response statuses and errors more robustly.
You log the status code but don’t reject the promise ifres.statusCode
indicates an error (e.g., 400 or 500). Consider rejecting on invalid statuses so upstream consumers can handle failures properly.-res.on('end', () => { +res.on('end', () => { + if (res.statusCode && res.statusCode >= 400) { + return reject(new Error(`Request failed with status code: ${res.statusCode}`)); + } console.log(`Status Code: ${res.statusCode}`); resolve(data); });
167-171
: Add error handling when parsing user data.
JSON.parse(userResponse as any)
can throw an exception if the server response is not valid JSON. Consider a try/catch block or a fallback.async getUser() { console.log('\nMaking GET request to /user endpoint:'); const userResponse = await this.makeGetRequest('/user'); - console.log('Response:', JSON.parse(userResponse as any)); + try { + const parsed = JSON.parse(userResponse as string); + console.log('Response:', parsed); + } catch (err) { + console.error('Failed to parse JSON response:', err); + } }
183-197
: Consider removing or uncommenting thesteam.login()
call.
The example usage is incomplete—login()
is commented out, yetgetUser()
is called. IfgetUser()
requires an authenticated session, you might want to invokesteam.login()
first or remove it if not relevant.packages/steam/SteamService.cs (3)
26-45
: Successfully refactored authentication to be user-centricThe removal of
teamId
parameter simplifies the authentication flow by making it user-centric rather than team-centric. The method now correctly:
- Searches for credentials using only userId
- Creates a handler with the proper callback for saving credentials
- Sets credentials if found and initiates login
Consider adding more descriptive comments about the authentication flow in this method to help future developers understand the process better.
93-133
: Clean up commented code after migration is completeKeep the commented code during the transition, but ensure it's removed or properly refactored in a future PR once the new architecture is proven stable.
-// public async Task<SteamUserInfo?> GetUserInfoFromStoredCredentials(string userId) -// { -// try -// { -// // Check if we have an active session -// if (_clientHandlers.TryGetValue(userId, out var activeHandler) && activeHandler.UserInfo != null) -// { -// return activeHandler.UserInfo; -// } - -// // Try to get stored credentials -// using var scope = _serviceProvider.CreateScope(); -// var dbContext = scope.ServiceProvider.GetRequiredService<SteamDbContext>(); -// var storedCredential = await dbContext.SteamUserCredentials -// .FirstOrDefaultAsync(c => c.UserId == userId); - -// if (storedCredential == null) -// { -// return null; -// } - -// // Create a new handler with service provider instead of dbContext -// var handler = new SteamClientHandler(userId, _serviceProvider, userId); -// var success = await handler.LoginWithStoredCredentialsAsync( -// storedCredential.AccountName, -// storedCredential.RefreshToken -// ); - -// if (success) -// { -// _clientHandlers.TryAdd(userId, handler); -// return handler.UserInfo; -// } -// return null; -// } -// catch (Exception ex) -// { -// Console.WriteLine($"Error retrieving cached user info: {ex.Message}"); -// return null; -// } -// }
179-187
: Clean up commented code after migration is completeSimilar to the other commented method, ensure this is removed or properly refactored in a future PR.
-// public SteamUserInfo? GetUserInfo(string clientId) -// { -// if (_clientHandlers.TryGetValue(clientId, out var handler)) -// { -// return handler.UserInfo; -// } - -// return null; -// }packages/steam/SteamClientOld.cs.txt (5)
254-288
: Good implementation of timeout handling, consider refactoring error handlingThe timeout logic is well implemented, but there's some room for improvement in the error handling.
Consider extracting the error handling logic into a separate method for better reusability across the class:
- catch (Exception ex) - { - Console.WriteLine($"[{_clientId}] Error logging in with stored credentials: {ex.Message}"); - return false; - } + catch (Exception ex) + { + HandleError($"Error logging in with stored credentials", ex); + return false; + } + private void HandleError(string context, Exception ex) + { + Console.WriteLine($"[{_clientId}] {context}: {ex.Message}"); + NotifyEvent(new ServerSentEvent("error", new { message = $"{context}: {ex.Message}" })); + }
283-287
: Remove commented-out codeThere's commented-out code that should be removed if it's no longer needed.
- // finally - // { - // // nager.Unsubscribe(connectedHandler); - // // _manager.Unsubscribe(loggedOnHandler);_ma - // }
362-368
: Remove commented-out methodThere's a commented-out
OnLoggedOn
method that should be removed if it's no longer needed.- // private async void OnLoggedOn(SteamUser.LoggedOnCallback callback) - // { - - // Console.WriteLine($"[{_clientId}] Successfully logged on as {callback.ClientSteamID}"); - - // _isAuthenticated = true; - // }
497-563
: Well-implemented database operation with comprehensive error handlingThe
SaveAccountInfoToDatabaseAsync
method is well implemented with:
- Proper separation between create and update operations
- Good use of the service provider to get a fresh DbContext
- Comprehensive error handling
- Proper logging
However, there's a potential optimization opportunity.
Consider using a Dictionary<string, Func<dynamic, object>> approach to reduce the repetitive pattern of checking dictionary keys and assigning values:
private async Task SaveAccountInfoToDatabaseAsync() { try { // Create a new scope and get a fresh DB context using var scope = _serviceProvider.CreateScope(); var dbContext = scope.ServiceProvider.GetRequiredService<SteamDbContext>(); var existingInfo = await dbContext.SteamAccountInfo .FirstOrDefaultAsync(info => info.UserId == _userId); var propertySetters = new Dictionary<string, Action<SteamAccountInfo, object>> { ["username"] = (info, val) => info.Username = (string)val, ["steamId"] = (info, val) => info.SteamId = (ulong)val, // Add other properties similarly }; if (existingInfo == null) { // Create new record var newInfo = new SteamAccountInfo { UserId = _userId }; // Apply all available properties foreach (var key in _accountInfoCache.Keys) { if (propertySetters.TryGetValue(key, out var setter)) { setter(newInfo, _accountInfoCache[key]); } } dbContext.SteamAccountInfo.Add(newInfo); } else { // Update existing record foreach (var key in _accountInfoCache.Keys) { if (propertySetters.TryGetValue(key, out var setter)) { setter(existingInfo, _accountInfoCache[key]); } } existingInfo.UpdatedAt = DateTime.UtcNow; } await dbContext.SaveChangesAsync(); Console.WriteLine($"[{_clientId}] Saved account info to database for user {_userId}"); } catch (Exception ex) { Console.WriteLine($"[{_clientId}] Error saving account info to database: {ex.Message}"); } }
569-571
: Move credential fields to top of classFor better code organization, move these private fields to the top of the class with the other field declarations.
- private string? _refreshToken; - private string? _accountName; + // Move these to line ~23 with the other field declarations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/steam/AccountInfoSchema.cs
(1 hunks)packages/steam/AccountLimited.cs
(1 hunks)packages/steam/AuthSchema.cs
(1 hunks)packages/steam/DBContext.cs
(1 hunks)packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs
(1 hunks)packages/steam/Migrations/20250409045105_AccountInfo.cs
(1 hunks)packages/steam/Migrations/SteamDbContextModelSnapshot.cs
(1 hunks)packages/steam/Program.cs
(1 hunks)packages/steam/SteamAuth.cs
(1 hunks)packages/steam/SteamClient.cs
(0 hunks)packages/steam/SteamClientOld.cs.txt
(1 hunks)packages/steam/SteamService.cs
(3 hunks)packages/steam/appsettings.Development.json
(1 hunks)packages/steam/index.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- packages/steam/SteamClient.cs
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/steam/DBContext.cs (2)
packages/steam/AuthSchema.cs (1)
SteamUserCredentials
(1-9)packages/steam/AccountInfoSchema.cs (1)
SteamAccountInfo
(1-22)
packages/steam/SteamService.cs (4)
packages/steam/SteamAuth.cs (4)
Task
(83-108)Task
(556-620)SetCredentials
(523-527)Action
(449-471)packages/steam/DBContext.cs (2)
SteamDbContext
(3-23)SteamDbContext
(8-10)packages/steam/AuthSchema.cs (1)
SteamUserCredentials
(1-9)packages/steam/AccountInfoSchema.cs (1)
SteamAccountInfo
(1-22)
packages/steam/Program.cs (3)
packages/steam/SteamService.cs (6)
SteamService
(6-188)SteamService
(11-14)Task
(26-45)Task
(56-91)Task
(135-153)Task
(155-172)packages/steam/DBContext.cs (2)
SteamDbContext
(3-23)SteamDbContext
(8-10)packages/steam/SSEevent.cs (3)
ServerSentEvent
(3-19)ServerSentEvent
(8-12)Serialize
(14-18)
packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs (3)
packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
DbContext
(11-121)packages/steam/DBContext.cs (2)
SteamDbContext
(3-23)SteamDbContext
(8-10)packages/steam/Migrations/20250409045105_AccountInfo.cs (1)
AccountInfo
(9-57)
🪛 Biome (1.9.4)
packages/steam/index.ts
[error] 9-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (30)
packages/steam/appsettings.Development.json (1)
5-6
: Good addition to logging configuration.Adding a specific log level for Entity Framework database commands is a sensible approach. Setting this to "Warning" will help reduce noise in development logs while still capturing important database-related issues. This is particularly relevant given the database schema changes being introduced in this PR.
packages/steam/Migrations/20250409045105_AccountInfo.cs (1)
1-58
:❓ Verification inconclusive
Well-structured migration with appropriate indexing.
This migration correctly implements the database schema for the
SteamAccountInfo
class with a proper primary key and a unique index on theUserId
column, which is a good practice for optimizing queries.The migration includes a proper
Down
method that allows for clean rollbacks if needed.One thing to note is the date in the migration filename (20250409) appears to be from April 2025. Is this intentional or a typo? EF Core migrations typically use the current date.
🏁 Script executed:
#!/bin/bash # Check if there are other migrations in the project with future dates echo "Checking for other migrations with future dates..." find packages/steam/Migrations -name "*.cs" | grep -E '2025|2026|2027|2028|2029'Length of output: 558
FYI: Migration Filename Date Verification
The migration for creating the
SteamAccountInfo
table is well-structured with appropriate indexing and a proper rollback implementation. The filename date (20250409
) is consistent with other migrations in the repository (e.g., those from20250408
), which suggests that using a date in early April 2025 is intentional. Please confirm that the date values are correct and that they match your intended timeline.
- File affected:
packages/steam/Migrations/20250409045105_AccountInfo.cs
packages/steam/Program.cs (3)
3-7
: No major concerns on the added imports and namespace.This namespace declaration and new
using
statements appear straightforward.
18-30
: Configuration settings look good.Setting keep-alive and request timeouts is a recommended practice for long-running connections.
85-274
: Implementation of SSE-based login flow is robust; verify concurrency safety.The approach for multi-Task flow (subscription, heartbeat, timeout) appears coherent. Confirm that the
_steamService
usage is threadsafe and that abrupt cancellations are handled without leftover subscriptions.packages/steam/AccountLimited.cs (1)
1-4
: Confirm desired access modifiers.Currently, this file-scoped class is default-internal, which is fine if it’s only used within the assembly. If external references are needed, consider
public
.packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
1-123
:✅ Verification successful
Snapshot structure looks valid; verify EF Core version.
The
ProductVersion
is set to9.0.3
, which is unconventional if you're on EF Core 7 or 8. Ensure alignment with the actual EF package version.
🌐 Web query:
Is "ProductVersion": "9.0.3" valid for the current EF Core release?
💡 Result:
Yes, the ProductVersion "9.0.3" is valid for the current Entity Framework Core (EF Core) release. Multiple sources confirm EF Core 9.0.3 is actively maintained and supported as of April 2025:
- NuGet Package lists
Microsoft.EntityFrameworkCore 9.0.3
as the latest stable version, with support for .NET 8.0[3].- Microsoft's March 2025 servicing updates specifically reference EF Core 9.0.3 as part of their security and compatibility patches[7].
- EF Core's official roadmap shows version 9.0 as the current Long-Term Support (LTS) release, supported until May 12, 2026[5].
This version includes features like JSON column support, bulk updates, and performance enhancements[1][7]. The next major release, EF Core 10.0, is scheduled for November 2025 but has not yet replaced 9.x as the recommended production version[5].
Citations:
- 1: https://en.wikipedia.org/wiki/Entity_Framework
- 2: https://archive.org/stream/AaronSwartzSecretService/Swartz_FOIA_Request16_djvu.txt
- 3: https://www.nuget.org/packages/microsoft.entityframeworkcore
- 4: https://docs.redhat.com/pt/documentation/red_hat_enterprise_linux/8/pdf/building_running_and_managing_containers/red_hat_enterprise_linux-8-building_running_and_managing_containers-en-us.pdf
- 5: https://learn.microsoft.com/en-us/ef/core/what-is-new/
- 6: https://devcenter.heroku.com/articles/getting-started-with-dotnet?singlepage=true
- 7: https://devblogs.microsoft.com/dotnet/dotnet-and-dotnet-framework-march-2025-servicing-updates/
- 8: https://docs.redhat.com/de/documentation/red_hat_enterprise_linux/8/epub/building_running_and_managing_containers/
- 9: https://www.techcronus.com/blog/asp-net-core-2025-key-updates-and-innovations-you-need-to-know
- 10: https://www.joesandbox.com/analysis/675968/0/pdf
EF Core Version Confirmed
The snapshot structure is valid, and the ProductVersion "9.0.3" aligns with the current EF Core release as verified. No changes are necessary.packages/steam/SteamAuth.cs (5)
66-80
: Thread safety for_subscribers
list.Adding/removing callbacks while iterating could lead to concurrency issues, though the
.ToList()
usage and careful approach helps. Confirm no other threads mutate_subscribers
concurrently.
110-182
: Exception handling is comprehensive.Catching and surfacing errors to the SSE clients is a good practice. No immediate concerns.
226-285
: Login success flow is well-organized.The interplay with
_authSession
,_refreshToken
, and_username
is logical. Good job covering success vs. error scenarios.
449-471
: Subscription approach is flexible.Returning an unsubscribe function fosters a clean pattern for SSE or event-driven logic.
531-555
: Disconnect handling is thorough.Canceling the token source and disconnecting the client is a proper approach.
packages/steam/DBContext.cs (2)
19-20
: Confirm that a singleSteamAccountInfo
per user is desired.
MakingUserId
unique is good if each user can have only one Steam account info record. If multiple records per user might be necessary in the future, consider removing.IsUnique()
.
5-6
:❓ Verification inconclusive
Ensure consistent naming usage throughout the solution.
These newDbSet
properties (SteamUserCredentials
andSteamAccountInfo
) align well with the updated naming convention. Verify that references everywhere else in the solution are updated to match these collection names.Run the following script to ensure that all references have been updated:
🏁 Script executed:
#!/bin/bash # Searching for any references to outdated naming "SteamUserCredential" or "SteamUserInfo". rg 'SteamUserCredential|SteamUserInfo' -A 3 -B 3Length of output: 13659
Naming consistency verified in active code – verify legacy instances if intentional
The new active DbSet properties are correctly referenced as
SteamUserCredentials
andSteamAccountInfo
throughout the updated code (e.g., inDBContext.cs
andSteamService.cs
). Note that some legacy references—such as occurrences ofSteamUserCredential
(singular) in migration designer files and commented code (e.g., inMigrations/20250408125920_RemoveTeam.Designer.cs
and20250408090438_InitialCreate.Designer.cs
, as well as commented sections inSteamClientOld.cs.txt
)—still remain. These legacy usages are common in historical migration snapshots and are typically left unchanged.Please verify that these legacy references are acceptable given they do not impact runtime behavior or new migrations. If a uniform naming update in migration files is required, consider creating a dedicated migration to rename the affected artifacts.
packages/steam/index.ts (2)
11-67
: Consider using POST for login.
Yourlogin()
method currently issues a GET request for a login operation, which typically has side effects. Many RESTful APIs use POST instead. Confirm this aligns with your backend requirements.Do you want me to generate a script to search for references, or are you comfortable verifying the server logic independently?
173-181
: Graceful disconnect logic looks good.
No issues identified. This ensures thehttpClient
request is properly destroyed.packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs (1)
1-125
: Auto-generated migration file.
Generally, manual edits to auto-generated migrations are discouraged. The definitions forSteamAccountInfo
andSteamUserCredentials
appear correct and align with the newly introduced schema.packages/steam/SteamService.cs (6)
8-8
: Good refactoring to use ConcurrentDictionary for thread-safe operationsUsing ConcurrentDictionary is appropriate here for handling concurrent access to the client handlers without explicit locking.
47-54
: Good addition of user disconnection functionalityThe new
DisconnectUser
method properly handles cleanup of resources when a user disconnects by:
- Finding the handler associated with the userId
- Calling the handler's Disconnect method
- Removing the handler from the dictionary
This helps prevent memory leaks and ensures proper resource management.
56-91
: Properly simplified credential saving processThe
SaveCredentials
method has been correctly updated to remove theteamId
parameter, aligning with the user-centric approach. The database queries and operations now properly use only theuserId
.
135-153
: Well-implemented caching method with proper error handlingGood implementation of
GetCachedUserInfoAsync
with:
- Input validation
- Proper error handling
- Use of AsNoTracking for read-only operations which improves performance
- Async/await pattern usage
155-172
: Good implementation of existence check methodThe
HasCachedUserInfoAsync
method correctly implements a lightweight check for the existence of cached user info, following the same error handling and validation patterns.
40-45
:❓ Verification inconclusive
Verify login and disconnection sequence
The code now immediately disconnects after handling login request. Make sure this is intentional and doesn't cause issues with the authentication flow.
🏁 Script executed:
#!/bin/bash # Search for references to HandleLoginRequest to understand the expected flow rg "HandleLoginRequest" -A 5 -B 5Length of output: 1954
Action Required: Confirm the Login/Disconnection Flow
The asynchronous login process completes with
await handler.HandleLoginRequest()
, after which the code immediately callshandler.Disconnect()
. Please verify that disconnecting right after handling the login is intentional—that no further communication or dependency on an open connection is required for the authentication flow. In particular, ensure that:
- All necessary login tasks are fully complete and that no subsequent operations expect the connection to remain open.
- If additional post-login interactions must occur, consider delaying or restructuring the disconnect sequence.
packages/steam/SteamClientOld.cs.txt (7)
6-24
: Well-structured class with proper state managementThe
SteamClientHandler
class follows good practices with:
- Clear separation of concerns
- Proper event mechanism for notifications
- Good state tracking with fields like
_isAuthenticated
28-47
: Strong initialization with proper exception handlingThe constructor properly initializes all required components and sets up callback handlers. The use of null checks with exceptions (
throw new InvalidOperationException
) is a good practice to fail fast if critical components are missing.
49-91
: Good implementation of login process with proper event notificationsThe first part of the
LoginAsync
method properly:
- Handles already connected states
- Sets up cancellation token for task management
- Provides informative status notifications to clients
- Properly connects to Steam client
92-133
: Comprehensive credential-based login implementationThis section correctly handles the case where stored credentials are available and when QR code authentication is needed, with proper logging and error notification.
134-183
: Thorough QR code authentication handling with proper error managementGood implementation of QR code authentication flow with:
- Polling for results
- Error handling for authentication polling
- Clear event notifications to the client about the status
189-252
: Robust login result handlingThe login result handler properly:
- Differentiates between successful and failed logins
- Provides appropriate notifications to clients
- Captures and stores user information on success
- Properly sets the authentication state
290-324
: Good authentication initiation with state checkingThe
StartAuthenticationAsync
method properly:
- Checks if authentication is already in progress to avoid duplicate efforts
- Resends QR code if already available
- Sets up proper cancellation token and task management
- Provides status notifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
♻️ Duplicate comments (11)
packages/steam/index.ts (7)
1-204
: Consider storing credentials securely.The class appears to handle authentication credentials but doesn't implement any secure storage. Consider:
- Implementing encryption for stored credentials
- Using environment variables for sensitive IDs
- Adding an option to use a more secure location than
/tmp/steam.sock
- Adding a timeout for stored credentials
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
5-9
: 🛠️ Refactor suggestionRemove unnecessary constructor and refine class properties.
The empty constructor doesn't add any value and should be removed. Consider making the socketPath a constructor parameter with a default value for better flexibility.
class Steam { httpClient: http.ClientRequest | null = null; socketPath: string = '/tmp/steam.sock'; - constructor() { } + constructor(socketPath: string = '/tmp/steam.sock') { + this.socketPath = socketPath; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
18-18
: 🛠️ Refactor suggestionRemove hardcoded user ID in headers.
The code contains a hardcoded user ID in the request headers. This should be parameterized to make the class reusable.
headers: { 'Accept': 'text/event-stream', - 'user-id': 'usr_XXXXXXXXXXXXXXX' + 'user-id': this.userId || 'anonymous' }Consider adding a
userId
property to the class and allowing it to be set via constructor or a setter method.
118-124
: 🛠️ Refactor suggestionFix console.error usage for non-error events.
The methods use
console.error
for non-error events like 'login-attempt' and 'login-successful', which is misleading. Useconsole.log
for informational messages.case 'login-attempt': - console.error(`Attempting to login to Steam\n`); + console.log(`Attempting to login to Steam\n`); break; case 'login-successful': - console.error(`Login succesful: ${data.username}\n`); + console.log(`Login successful: ${data.username}\n`); break;Also note: "succesful" is misspelled and should be "successful".
148-148
: 🛠️ Refactor suggestionRemove hardcoded user ID in headers (makeGetRequest).
Similar to the login method, this also contains a hardcoded user ID that should be parameterized.
headers: { 'Content-Type': 'application/json', - 'user-id': 'usr_XXXXXXXXXXXXXXX' + 'user-id': this.userId || 'anonymous' }
69-69
: 🛠️ Refactor suggestionAdd private modifier and return type to handleEvent method.
The
handleEvent
method should be marked as private since it's an internal implementation detail, and should have an explicit return type.- handleEvent(eventType: string, eventData: string) { + private handleEvent(eventType: string, eventData: string): void {
11-11
: 🛠️ Refactor suggestionAdd return type annotation to login method.
Methods should have explicit return type annotations for better type safety and documentation.
- login() { + public login(): void {packages/steam/Program.cs (3)
9-9
:⚠️ Potential issueFix insecure socket path location.
Using
/tmp/steam.sock
for the Unix domain socket is a security risk as the/tmp
directory is world-writable on most systems. This issue was previously flagged in a review but hasn't been fully addressed.- const string UnixSocketPath = "/tmp/steam.sock"; + const string UnixSocketPath = "/var/run/nestri/steam.sock";Also, ensure you create the directory with appropriate permissions:
+ // Ensure directory exists with proper permissions + var socketDirectory = Path.GetDirectoryName(UnixSocketPath); + if (!Directory.Exists(socketDirectory)) + { + Directory.CreateDirectory(socketDirectory); + } // Delete the socket file if it exists if (File.Exists(UnixSocketPath)) { File.Delete(UnixSocketPath); }
32-32
:⚠️ Potential issueFix insecure database location.
The SQLite database is configured to be stored at
/tmp/steam.db
, which has the same security concerns as the socket path - it's in a world-writable directory. This issue was previously flagged in a review.- options.UseSqlite($"Data Source=/tmp/steam.db")); + options.UseSqlite($"Data Source=/var/run/nestri/steam.db"));
40-55
:⚠️ Potential issueAdd error handling to long-running process endpoint.
The long-running process endpoint lacks proper error handling for connection drops or exceptions, which was flagged in a previous review.
app.MapGet("/api/longprocess", async (HttpResponse response) => { response.Headers.Append("Content-Type", "text/event-stream"); response.Headers.Append("Cache-Control", "no-cache"); response.Headers.Append("Connection", "keep-alive"); + try { for (int i = 0; i <= 100; i += 10) { + if (response.HttpContext.RequestAborted.IsCancellationRequested) { + Console.WriteLine("Client disconnected during long process"); + return; + } await response.WriteAsync($"data: {{\"progress\": {i}}}\n\n"); await response.Body.FlushAsync(); await Task.Delay(1000); // Simulate work being done } await response.WriteAsync($"data: {{\"status\": \"completed\"}}\n\n"); await response.Body.FlushAsync(); + } catch (Exception ex) { + Console.WriteLine($"Error during long process: {ex.Message}"); + if (!response.HasStarted) { + response.StatusCode = 500; + } + } });packages/functions/src/api/index.ts (1)
19-19
: CORS middleware added without specific configurationThis can lead to overly permissive cross-origin rules in production. Consider specifying origin(s), methods, and allowed headers explicitly to reduce security attack surface.
🧹 Nitpick comments (26)
packages/steam/Migrations/20250410104213_new.Designer.cs (2)
14-15
: Consider using a more descriptive migration nameThe migration name
new
is too generic and doesn't convey the purpose of this database change. Additionally,new
is a C# keyword (even though it's escaped with@
), making it less ideal as an identifier.- [Migration("20250410104213_new")] - partial class @new + [Migration("20250410104213_UpdateSteamTables")] + partial class UpdateSteamTables
11-12
: Namespace inconsistencyThe namespace
steam.Migrations
uses lowercase for "steam", but the using statement importsSteam
with an uppercase 'S'. This inconsistency can lead to confusion.- namespace steam.Migrations + namespace Steam.Migrationspackages/steam/index.ts (2)
170-170
: Remove commented-out code.The commented-out
req.write(body)
line suggests incomplete implementation. If POST requests are needed, implement them properly; otherwise, remove the comment.- // req.write(body);
191-202
: Consider separating example code from class implementation.The example code at the bottom of the file should be moved to a separate file or clearly marked as example only. This helps distinguish between the library code and its usage examples.
+ // Note: This is for demonstration purposes only. + // In a real application, this code would be in a separate file. (async () => { const steam = new Steam(); steam.login(); // await steam.getUser(); process.on('SIGINT', () => { console.log('Closing connections...'); steam.disconnect(); setTimeout(() => process.exit(0), 1000); }); })()Consider wrapping this code in a conditional block that only executes when the file is run directly, not when imported:
if (require.main === module) { // Example code here }packages/steam/Migrations/20250410104213_new.cs (2)
186-194
: Assigning new primary keys.
Replacing the old primary keys with new ones based onUserId
is valid, but confirm whether composite keys or foreign keys to other tables are required for referential integrity.
200-370
: Down Migration reintroducing dropped columns.
Re-adding all dropped columns on rollback can be helpful, but data that was removed or refactored out might not reappear in the same state. Confirm that rolling back won't cause data corruption for live systems.packages/functions/sst-env.d.ts (1)
15-16
: Validate alignment with new service-oriented schema for Auth.
Again, ensure consistency with the updated"type": "sst.aws.Service"
. This shift could break references to"sst.aws.Auth"
if not updated across the codebase.If you plan to use more specialized auth features, consider carving them out into dedicated parameters or an extended interface under
"sst.aws.Service"
.packages/steam/sst-env.d.ts (1)
1-9
: Remove empty export to satisfy lint rule.
The static analysis reports that the empty export at line 9 is redundant due to the existing import at line 8. Removing it will resolve the warning without affecting any functionality.1 /* This file is auto-generated by SST. Do not edit. */ 2 /* tslint:disable */ 3 /* eslint-disable */ 4 /* deno-fmt-ignore-file */ 5 6 /// <reference path="../../sst-env.d.ts" /> 7 8 import "sst" -9 export {}
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: This empty export is useless because there's another export or import.
This import makes useless the empty export.
Safe fix: Remove this useless empty export.
(lint/complexity/noUselessEmptyExport)
packages/functions/src/api/steam.ts (1)
35-45
: Consider caching Steam account information for improved performanceThe current implementation fetches Steam information on each request. For frequently accessed data like this, consider implementing caching to reduce external API calls and improve response times.
async (c) => { const actor = assertActor("user"); + // Use a caching layer with an appropriate TTL + const cacheKey = `steam:user:${actor.properties.userID}`; + // Check cache first before making external call const steamInfo = await Steam.fromUserID(actor.properties.userID); if (!steamInfo) throw new VisibleError( "not_found", ErrorCodes.NotFound.RESOURCE_NOT_FOUND, "Steam account information not found", ); + // Update cache with fresh data return c.json({ data: steamInfo }, 200); }packages/steam/ServerSentEvent.cs (1)
16-29
: Consider adding line break handling for multiline string dataThe current implementation doesn't handle multiline strings properly for SSE protocol. Each line in a multiline data string should be prefixed with "data:".
public string Serialize() { string dataString; if (Data is string stringData) { dataString = stringData; } else { dataString = JsonSerializer.Serialize(Data); } + // Handle multiline data strings by prefixing each line with "data: " + if (dataString.Contains('\n')) + { + dataString = string.Join("\ndata: ", dataString.Split('\n')); + } + return $"event: {Type}\ndata: {dataString}\n\n"; }packages/core/src/steam/steam.sql.ts (1)
6-21
: Remove C# comment code that appears to be reference material.These lines contain C# property definitions that seem to be reference comments for the table structure. While they provide context, they should be converted to proper TypeScript comments or documentation if needed, or removed if they're just leftover code.
-// public string Username { get; set; } = string.Empty; -// public ulong SteamId { get; set; } -// public string Email { get; set; } = string.Empty; -// public string Country { get; set; } = string.Empty; -// public string PersonaName { get; set; } = string.Empty; -// public string AvatarUrl { get; set; } = string.Empty; -// public bool IsLimited { get; set; } -// public bool IsLocked { get; set; } -// public bool IsBanned { get; set; } -// public bool IsAllowedToInviteFriends { get; set; } -// public ulong GameId { get; set; } -// public string GamePlayingName { get; set; } = string.Empty; -// public DateTime LastLogOn { get; set; } -// public DateTime LastLogOff { get; set; } -// public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; +/** + * Steam table schema definition for storing Steam user information: + * - User identification + * - Steam profile details + * - Account status + * - Last played game info + */packages/steam/Migrations/20250408125920_RemoveTeam.Designer.cs (1)
17-18
: Consider adding versioning for migration.The
BuildTargetModel
method overrides the base method correctly, but consider adding a comment with your application version number in addition to the "ProductVersion" annotation to track when this migration was created relative to your application releases.packages/steam/Program.cs (1)
196-199
: Consider using IHost lifetime events to handle service startup/shutdown.The current approach of logging startup information doesn't properly handle graceful shutdown. Consider using IHost lifetime events to manage service lifecycle.
+ // Register a service to handle application lifetime events + builder.Services.AddHostedService<ApplicationLifetimeService>(); // Ensure database is created and migrated using (var scope = app.Services.CreateScope()) {Then create a new class:
public class ApplicationLifetimeService : IHostedService { private readonly IHostApplicationLifetime _appLifetime; private readonly ILogger<ApplicationLifetimeService> _logger; public ApplicationLifetimeService( IHostApplicationLifetime appLifetime, ILogger<ApplicationLifetimeService> logger) { _appLifetime = appLifetime; _logger = logger; } public Task StartAsync(CancellationToken cancellationToken) { _appLifetime.ApplicationStarted.Register(OnStarted); _appLifetime.ApplicationStopping.Register(OnStopping); _appLifetime.ApplicationStopped.Register(OnStopped); return Task.CompletedTask; } public Task StopAsync(CancellationToken cancellationToken) { return Task.CompletedTask; } private void OnStarted() { _logger.LogInformation("Steam Auth Service started successfully"); } private void OnStopping() { _logger.LogInformation("Steam Auth Service is stopping"); } private void OnStopped() { _logger.LogInformation("Steam Auth Service has stopped"); } }packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
10-13
: Fix namespace inconsistency.The namespace
steam.Migrations
is inconsistent with the namespaceSteam
used for the entities. This inconsistency could lead to difficulties importing the correct types and confusion in the codebase.-namespace steam.Migrations +namespace Steam.Migrationspackages/functions/src/api/index.ts (1)
95-95
: Ensure logger patch behavior meets expectationsCalling
patchLogger()
here should be verified to ensure it is applied before any critical logging. Otherwise, there is a risk of missing logs or failing to apply custom formatting.packages/steam/SteamAuthComponent.cs (2)
84-93
: Potential performance concern in the callback loopUsing
Thread.Sleep(10)
on every iteration may introduce unnecessary delays or elevated CPU usage over time, especially if usage scales. Consider an event-driven or await-based approach (e.g., using a more efficient mechanism to block until callbacks are available).
196-208
: Automatic reconnection logicAutomatically reconnecting every 30 seconds is helpful for maintaining presence but might create repeated login attempts if credentials are invalid or the Steam service is unavailable. Consider bounding the number of retries or adding backoff logic to prevent infinite loops and possible rate limits.
packages/core/migrations/0003_first_big_bertha.sql (1)
15-19
: Evaluate NOT NULL constraintsColumns
"time_seen", "steam_id", "last_game", "steam_email", and "limitation"
are all markedNOT NULL
. Verify that your application can provide valid values for these fields in all scenarios; otherwise consider allowingNULL
where needed.packages/core/src/steam/index.ts (1)
21-23
: Fix typo in the description.“regisered” should be spelled “registered” to maintain clarity and consistency in your documentation.
- description: "The email regisered with this Steam account", + description: "The email registered with this Steam account",packages/core/migrations/meta/0003_snapshot.json (1)
194-328
: Consider adding a foreign key constraint onuser_id
.Although you have a composite key on
(user_id, id)
, explicitly referencinguser.user_id
via a foreign key can help prevent orphaned entries in thesteam
table and improve data integrity.packages/steam/SteamService.cs (3)
4-25
: Use structured logging instead ofConsole.WriteLine
.Relying on
Console.WriteLine
can limit observability in production. Consider a structured logging library (e.g., Microsoft.Extensions.Logging) for more flexible, contextual logs.
64-82
: Consider structured logging for error handling.The
try/catch
block prints errors viaConsole.WriteLine
, which can hamper observability. Transitioning to a structured logger would give more context for debugging.
84-121
: Maintain consistent logging practices.You already catch and log exceptions in this method. Consider using a structured logging framework for uniform log formatting and filtering across your application.
packages/steam/SteamModels.cs (3)
47-55
: Consider adding default values for timestamp properties.The
LastLogOn
andLastLogOff
properties don't have default values, unlike other timestamp fields.public bool IsAllowedToInviteFriends { get; set; } public ulong GameId { get; set; } public string GamePlayingName { get; set; } = string.Empty; -public DateTime LastLogOn { get; set; } -public DateTime LastLogOff { get; set; } +public DateTime LastLogOn { get; set; } = DateTime.UtcNow; +public DateTime LastLogOff { get; set; } = DateTime.UtcNow; public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;
4-56
: Consider adding XML documentation for better code clarity.Adding XML documentation comments to the classes and properties would improve code readability and help other developers understand the purpose and usage of these models.
Example implementation:
namespace Steam { + /// <summary> + /// Database context for Steam-related entities + /// </summary> public class SteamDbContext : DbContext { // ... } + /// <summary> + /// Stores Steam user credentials for authentication purposes + /// </summary> public class SteamUserCredentials { + /// <summary> + /// Primary key, corresponds to the application's user ID + /// </summary> [Key] public string UserId { get; set; } = string.Empty; // ... } + /// <summary> + /// Stores Steam account information and status + /// </summary> public class SteamAccountInfo { // ... } }
35-55
: Consider adding validation attributes for data integrity.The model properties lack validation attributes which could help ensure data integrity.
public class SteamAccountInfo { [Key] public string UserId { get; set; } = string.Empty; + [Required] public string Username { get; set; } = string.Empty; public ulong SteamId { get; set; } + [EmailAddress] public string Email { get; set; } = string.Empty; + [StringLength(2)] public string Country { get; set; } = string.Empty; // Additional validations for other properties }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
infra/realtime.ts
(1 hunks)packages/core/migrations/0003_first_big_bertha.sql
(1 hunks)packages/core/migrations/meta/0003_snapshot.json
(1 hunks)packages/core/migrations/meta/_journal.json
(1 hunks)packages/core/src/email/index.ts
(1 hunks)packages/core/src/examples.ts
(2 hunks)packages/core/src/steam/index.ts
(1 hunks)packages/core/src/steam/steam.sql.ts
(1 hunks)packages/core/src/user/user.sql.ts
(1 hunks)packages/functions/src/api/index.ts
(2 hunks)packages/functions/src/api/machine.ts
(7 hunks)packages/functions/src/api/steam.ts
(1 hunks)packages/functions/src/realtime/authorizer.ts
(1 hunks)packages/functions/sst-env.d.ts
(1 hunks)packages/steam/DBContext.cs
(0 hunks)packages/steam/Migrations/20250408090438_InitialCreate.Designer.cs
(2 hunks)packages/steam/Migrations/20250408125920_RemoveTeam.Designer.cs
(2 hunks)packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs
(1 hunks)packages/steam/Migrations/20250410104213_new.Designer.cs
(1 hunks)packages/steam/Migrations/20250410104213_new.cs
(1 hunks)packages/steam/Migrations/SteamDbContextModelSnapshot.cs
(1 hunks)packages/steam/Program.cs
(1 hunks)packages/steam/ServerSentEvent.cs
(1 hunks)packages/steam/SteamAuthComponent.cs
(1 hunks)packages/steam/SteamConfiguration.cs
(1 hunks)packages/steam/SteamModels.cs
(1 hunks)packages/steam/SteamService.cs
(1 hunks)packages/steam/index.ts
(1 hunks)packages/steam/sst-env.d.ts
(1 hunks)sst-env.d.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- packages/steam/DBContext.cs
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/user/user.sql.ts
- packages/functions/src/api/machine.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/steam/Migrations/20250408090438_InitialCreate.Designer.cs
- packages/core/migrations/meta/_journal.json
- packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/functions/src/realtime/authorizer.ts (3)
packages/functions/sst-env.d.ts (1)
Resource
(8-90)sst-env.d.ts (1)
Resource
(8-90)packages/maitred/internal/resource/resource.go (1)
Resource
(10-28)
packages/core/src/email/index.ts (2)
packages/functions/sst-env.d.ts (1)
Resource
(8-90)sst-env.d.ts (1)
Resource
(8-90)
infra/realtime.ts (2)
packages/functions/src/api/auth.ts (1)
auth
(26-71)infra/postgres.ts (1)
postgres
(5-40)
packages/steam/Migrations/SteamDbContextModelSnapshot.cs (3)
packages/core/src/examples.ts (1)
Steam
(34-54)packages/steam/Migrations/20250410104213_new.Designer.cs (1)
DbContext
(13-109)packages/steam/SteamModels.cs (2)
SteamDbContext
(7-22)SteamDbContext
(9-9)
packages/core/src/steam/steam.sql.ts (1)
packages/core/src/drizzle/types.ts (4)
userID
(20-27)userID
(24-26)timestamps
(35-39)utc
(29-33)
packages/steam/Migrations/20250410104213_new.Designer.cs (4)
packages/core/src/examples.ts (1)
Steam
(34-54)packages/steam/Migrations/20250409045105_AccountInfo.Designer.cs (2)
DbContext
(13-125)BuildTargetModel
(18-124)packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
DbContext
(12-106)packages/steam/SteamModels.cs (2)
SteamDbContext
(7-22)SteamDbContext
(9-9)
packages/steam/SteamService.cs (1)
packages/steam/SteamAuthComponent.cs (6)
SteamAuthComponent
(6-301)SteamAuthComponent
(29-49)Action
(51-62)Task
(70-97)SetCredentials
(64-68)Disconnect
(99-111)
packages/steam/SteamModels.cs (2)
packages/core/src/examples.ts (1)
Steam
(34-54)packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
DbContext
(12-106)
🪛 Biome (1.9.4)
packages/steam/index.ts
[error] 9-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/steam/sst-env.d.ts
[error] 8-9: This empty export is useless because there's another export or import.
This import makes useless the empty export.
Safe fix: Remove this useless empty export.
(lint/complexity/noUselessEmptyExport)
🔇 Additional comments (35)
packages/core/src/examples.ts (2)
2-2
: Good refactoring to use namespace instead of module.Changing from
export module Examples
toexport namespace Examples
follows modern TypeScript best practices. Thenamespace
keyword is the preferred approach in current TypeScript development for organizing related code.
34-55
: Well-structured mock Steam user data.The new
Steam
constant provides comprehensive example data for Steam integration, including all necessary fields for authentication and user information. The structure follows the established pattern in this file and includes appropriate placeholder data.A few observations:
- The mock data covers both basic user info and Steam-specific properties
- The consistency with other examples in the file is maintained
- The data structure aligns with the Steam authentication feature mentioned in the PR
This will be valuable for testing the Steam authentication and game querying functionality being added to the homepage.
packages/steam/Migrations/20250410104213_new.Designer.cs (2)
23-82
:❓ Verification inconclusive
Review the required fields in SteamAccountInfo table
All fields in
SteamAccountInfo
are now marked as required, which is a significant change from the previous migration where many were nullable. This could cause migration issues if existing records have null values for these fields.Additionally, some fields like
GameId
andGamePlayingName
may not always be applicable (e.g., when a user is not playing any game), yet they're required in this schema.Consider making appropriate fields nullable to accommodate scenarios where data might not be available:
b.Property<string>("AvatarUrl") .IsRequired() .HasColumnType("TEXT"); // ... b.Property<ulong>("GameId") - .HasColumnType("INTEGER"); + .HasColumnType("INTEGER") + .IsRequired(false); b.Property<string>("GamePlayingName") - .IsRequired() .HasColumnType("TEXT");
Revisit Nullability Settings in SteamAccountInfo Migration
- The migration in
packages/steam/Migrations/20250410104213_new.Designer.cs
now marks most fields as required. This includes string properties with an explicit.IsRequired()
and value types, which are inherently non-nullable.- Compared to previous migrations where several fields were nullable, these changes might lead to migration issues if existing records contain null values.
- In particular, the
GameId
(a value type) andGamePlayingName
(a string) may not always have applicable data (e.g., when a user isn’t playing any game). If the business logic expects these fields to sometimes be absent, it’s advisable to allow nulls.b.Property<ulong>("GameId") - .HasColumnType("INTEGER"); + .HasColumnType("INTEGER") + .IsRequired(false); b.Property<string>("GamePlayingName") - .IsRequired() .HasColumnType("TEXT");Please verify that these adjustments align with your intended data model and that existing data will be handled appropriately during migration.
25-27
:❓ Verification inconclusive
Review primary key strategy change
This migration changes the primary key from an auto-incrementing integer
Id
to stringUserId
. This is a breaking change that will require data migration in production environments.Ensure there's a corresponding migration script that handles this transition properly, especially if there's existing data in these tables.
Run the following script to check for the corresponding migration implementation:
Also applies to: 86-88
🏁 Script executed:
#!/bin/bash # Check for the migration implementation file cat packages/steam/Migrations/20250410104213_new.csLength of output: 12631
Primary Key Update — Verify Data Migration Impact
The migration file (
packages/steam/Migrations/20250410104213_new.cs
) correctly updates the primary keys for bothSteamUserCredentials
andSteamAccountInfo
by removing the auto-increment integer column (Id
) and definingUserId
as the new primary key. The schema changes (i.e., dropping the old keys and indexes and adding new primary keys based onUserId
) are in place as expected.
- The transformation is effectively applied in the
Up()
method of the migration.- Caution: This is a breaking change that could affect production data. Please ensure a comprehensive data migration strategy is implemented to handle the transition for any existing records.
- The same considerations apply to the changes noted at lines 86-88.
Please double-check that all production data migration requirements are met before deploying this change.
packages/core/src/email/index.ts (1)
13-13
:❓ Verification inconclusive
Ensure valid email address construction.
Appending an "@" symbol to thefrom
andResource.Email.sender
is a creative approach for deriving a complete email address. However, verify thatfrom
always holds the local-part (e.g., "noreply") and thatResource.Email.sender
is a valid domain to prevent malformed email addresses (e.g., "[email protected]").Consider running a search for all usages or references of
Email.send
to confirm that thefrom
parameter does not contain unexpected values (e.g., includes a domain or is empty).
🏁 Script executed:
#!/bin/bash # Searching for all references to Email.send usage to ensure `from` is safe. rg -A 5 $'Email.send('Length of output: 96
Action: Verify the Consistency of the
from
Parameter and Email.send UsageThe current code concatenates the
from
local-part with a domain fromResource.Email.sender
using an “@” separator. This approach is acceptable provided thatfrom
always contains only the local-part (like “noreply”) and thatResource.Email.sender
always supplies a valid domain. However, to be confident there aren’t unexpected values (such as an already complete email address or an empty string), we need to double-check all calls toEmail.send
.The initial regex search returned a parse error because the parenthesis wasn’t escaped. To verify the usage, please run the following corrected shell script:
Manually review the output of this command to ensure that all instances adhere to the expected usage pattern. If discrepancies are found, consider adding additional validations or clarifications.
🏁 Script executed:
#!/bin/bash # Search for all references of Email.send with correctly escaped parentheses to inspect the `from` parameter. rg -A 5 'Email\.send\('Length of output: 985
Verify that the email construction logic is exercised properly when enabled
We ran a corrected search for references to
Email.send
and found only commented-out code in the repository (in bothpackages/functions/src/auth.ts
andpackages/functions/src/event/event.ts
). Since there is no active usage, it’s unclear if and how thefrom
parameter is validated at runtime. The current implementation:from = from + "@" + Resource.Email.sender;assumes that
from
is a local-part (e.g., "noreply") and thatResource.Email.sender
is a valid domain. Please ensure that:
- When email sending is enabled, the
from
parameter is indeed always a valid local-part (without a domain already appended).Resource.Email.sender
reliably provides the correct domain.- Consider adding tests or validations for email address construction to catch any potential misconfigurations if unexpected values (like an already complete email or an empty string) are passed.
infra/realtime.ts (2)
6-7
: Link property updated to useauth
.
Changinglink: [auth, postgres]
can affect event-handling or security checks within the Realtime service. Ensureauth
does not introduce unwanted performance overhead or conflicts.
1-1
:✅ Verification successful
Confirm the new import is valid.
Switching fromurls
toauth
indicates a new dependency. Verify that theauth
module is successfully exported from"./auth"
and provides all necessary functionality for your real-time configuration.
🏁 Script executed:
#!/bin/bash # Description: Ensure `auth` is exported in the "infra/auth.ts" file. rg -A 15 'export\s+const\s+auth' infra/auth.tsLength of output: 756
New Import Validated
Theauth
module is correctly exported frominfra/auth.ts
(as confirmed by the grep output) and provides the required functionality. The change from usingurls
toauth
ininfra/realtime.ts
is valid and no further modifications are needed.packages/steam/Migrations/20250410104213_new.cs (2)
14-20
: Dropping Primary Key from SteamUserCredentials.
Removing the primary key before reassigning a new one can temporarily break constraints if other migrations or operations rely on it. Verify the ordering of migrations or ensure no references are left broken.
46-184
: All columns set to non-null with defaults.
Forcing non-null constraints helps maintain data integrity but might risk defaulting critical fields in existing production data. Ensure that your application code and legacy records align with these columns now being non-null.sst-env.d.ts (2)
15-16
:❓ Verification inconclusive
Confirm references to updated Auth resource type.
Same reasoning as theApi
resource. Since"type"
changed to"sst.aws.Service"
, verify that references to the old"sst.aws.Auth"
type are removed or updated to avoid inconsistencies.
🏁 Script executed:
#!/bin/bash # Description: Verify references to old "sst.aws.Auth" usage rg -A 3 'sst\.aws\.Auth'Length of output: 26
Updated Auth Resource Type Verification
The search for"sst.aws.Auth"
returned no results, indicating that references to the deprecated type appear to have been removed. However, since the verification command produced no output, please manually confirm in any modules or tests that may indirectly reference the old type.
10-11
:❓ Verification inconclusive
Confirm references to updated API resource type.
You've introduced"service": string
and changed"type"
to"sst.aws.Service"
. Please ensure that any consumer code or infrastructure definitions referencing"sst.aws.Router"
have been updated accordingly throughout the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify references to old "sst.aws.Router" usage rg -A 3 'sst\.aws\.Router'Length of output: 28
Confirm Consumer References to Updated API Resource Type
The modifications in
sst-env.d.ts
(lines 10–11) now introduce"service": string
and update"type"
to"sst.aws.Service"
. An automated search for the old"sst.aws.Router"
yielded no results. However, since the output was empty, please perform a manual verification to ensure that no consumer code or infrastructure definitions are still referencing"sst.aws.Router"
.
- Verify that all references in the codebase have been updated from
"sst.aws.Router"
to"sst.aws.Service"
.- Double-check any related infrastructure definitions or consumer modules for outdated references.
packages/functions/sst-env.d.ts (1)
10-11
:❓ Verification inconclusive
Validate alignment with new service-oriented schema for Api.
The introduction of"service"
and the change from"sst.aws.Router"
to"sst.aws.Service"
should be consistently reflected in your infrastructure and usage.Please confirm that all code referencing the old type has been updated to reflect the new approach.
🏁 Script executed:
#!/bin/bash rg -A 3 'sst\.aws\.Router'Length of output: 28
Verification Complete: Confirm New Service-Oriented Schema
- The file
packages/functions/sst-env.d.ts
now declares:
"service": string
"type": "sst.aws.Service"
- A search across the codebase for occurrences of
sst.aws.Router
returned no results, suggesting that all references to the old type have been removed.Please manually verify that any infrastructure or configuration settings (which might not be picked up by static text searches) are updated accordingly to align with the new service-oriented schema.
packages/functions/src/realtime/authorizer.ts (1)
8-8
: LGTM: Resource path update aligned with new structureThe update from
Resource.Urls.auth
toResource.Auth.url
aligns with the revisedResource
interface structure, where resources like Auth now directly expose their URL property. This change maintains the same functionality while adapting to the new standardized approach for accessing resource URLs across the codebase.packages/steam/SteamConfiguration.cs (1)
3-17
: LGTM: Well-structured Steam client configuration classThe implementation of
SteamConfig
with a static factory method provides a centralized way to configure Steam clients consistently throughout the application. The 30-second connection timeout and dual protocol support (TCP and WebSocket) are sensible defaults that enhance reliability.packages/functions/src/api/steam.ts (1)
9-47
: LGTM: Well-designed Steam API endpoint with comprehensive documentationThe Steam API implementation is well-structured with clear separation of concerns, comprehensive OpenAPI documentation, and proper error handling. The endpoint correctly asserts user authentication before attempting to retrieve Steam account information.
packages/steam/ServerSentEvent.cs (1)
3-31
: LGTM: Clean implementation of Server-Sent Events serializationThe
ServerSentEvent
class correctly implements the SSE protocol format with proper serialization of different data types. The class is well-encapsulated with clear responsibilities.packages/core/src/steam/steam.sql.ts (2)
40-41
:✅ Verification successful
Confirm the primary key design is intentional.
The table uses both
id
anduserID
from the userID object in the primary key. Since both are coming from the same userID object and are likely ULIDs, consider if this composite key is necessary or if a single field would be sufficient.
🏁 Script executed:
#!/bin/bash # Check how userID and id are used in other tables for consistency rg -A 5 'primaryKey\(' --type ts packages/core/src/Length of output: 987
Confirm Composite Primary Key Design is Intentional
The
steam.sql.ts
table currently defines its primary key as a composite ofuserID
andid
, a pattern that is also observed inteam.sql.ts
. Since both values are derived from the same object and are likely ULIDs, please verify that using a composite key is an intentional design choice. If a single primary key would suffice for ensuring uniqueness, consider updating the design accordingly.
43-51
:❓ Verification inconclusive
Ensure field alignment with external schema.
The fields defined here should align with the Steam API response structure and the C# SteamAccountInfo entity seen in the model snapshot. Confirm that all field names and types match between systems to prevent data mapping issues.
🏁 Script executed:
#!/bin/bash # Look for related Steam model definitions to verify field alignment rg -A 5 'SteamAccountInfo' --type cs packages/steam/Length of output: 26923
Action: Confirm Field Mapping Consistency Across Systems
- Mapping Verification:
• The primary fields such assteamID
,avatarUrl
, andusername
appear to align with the correspondingSteamId
,AvatarUrl
, andUsername
properties in the C#SteamAccountInfo
model and the migration snapshots.- Extra Fields Check:
• Fields likelastGame
,countryCode
,steamEmail
,personaName
, andlimitation
do not appear in the basic C# model definition (which currently includes onlyUserId
,Username
, andSteamId
).
• Please verify whether these additional fields are intended solely for mapping the Steam API response or if the C# model should be updated to incorporate them.- Recommended Next Steps:
• Double-check the external Steam API schema and ensure that these extra fields (and their corresponding types) have matching entries in your internal models (or that the separation is intentional).
• Align the names and types across the SQL schema, the external API response, and the C# entity to prevent data mapping issues.packages/steam/Migrations/20250408125920_RemoveTeam.Designer.cs (2)
49-50
: Ensure index on UserId is properly defined.The migration properly sets up a unique index on the UserId field. Make sure this aligns with the database access patterns and that all queries filtering by UserId leverage this index.
14-15
:❓ Verification inconclusive
Migration name implies removal but doesn't show removed code.
The migration class is named "RemoveTeam" implying the removal of a team-related field (likely TeamId), but since we're only seeing the target state without the Up/Down methods, it's hard to confirm the exact changes. Make sure this migration correctly removes the TeamId property.
🏁 Script executed:
#!/bin/bash # Check for TeamId in previous migration files or model definitions rg 'TeamId' --type cs packages/steam/Length of output: 37
Action Required: Verify Removal of TeamId in Migration Code
The search for occurrences of "TeamId" returned no results, so it's unclear if the removal logic is implemented elsewhere. Please manually verify that the primary migration file (likely named
20250408125920_RemoveTeam.cs
) contains the appropriate Up/Down operations (e.g., a call to drop the "TeamId" column) corresponding to the migration’s intent.
- Confirm that the Up method in
20250408125920_RemoveTeam.cs
removes the "TeamId" property.- Ensure that, if applicable, the Down method correctly reverts the removal.
packages/steam/Migrations/SteamDbContextModelSnapshot.cs (1)
20-79
: SteamAccountInfo entity aligns with SQL schema.The SteamAccountInfo entity properties align well with the fields defined in the PostgreSQL schema in
packages/core/src/steam/steam.sql.ts
. This consistency is important for cross-platform data sharing.packages/functions/src/api/index.ts (3)
28-30
: Nice addition of new routesThe introduction of
/realtime
and/steam
endpoints neatly organizes real-time features and Steam API functionalities. No issues found with your routing approach.
89-89
: OpenAPI servers updateExpanding the servers array to include the sandbox environment is helpful for staging and testing. This change looks good.
97-106
:❓ Verification inconclusive
Confirm idleTimeout usage
The idle timeout value of 255 seconds may be unintentional or arbitrary. If your connections are expected to remain open for a longer duration, users might experience undesired disconnections. Please verify or adjust as needed.
🏁 Script executed:
#!/bin/bash # Searching references to idleTimeout within the repository rg -A 5 "idleTimeout"Length of output: 1274
Idle Timeout Value: Please Confirm Consistency
Our repository search shows that the
idleTimeout
is set to 255 seconds in bothpackages/functions/src/api/index.ts
andpackages/functions/src/auth.ts
, while ininfra/zero.ts
it is defined as 3600 seconds. Please verify whether the 255-second timeout is intentional for these function endpoints or if it should be adjusted for consistency with the longer timeout used elsewhere.packages/core/migrations/0003_first_big_bertha.sql (4)
1-11
: Confirm database support for the "point" typeThe “location” column uses the
"point"
type, which may require a PostGIS or a native PostgreSQL extension, depending on your environment. Ensure your target infrastructure supports or enables this type.
13-13
: Renaming columns can break referencesChanging
"country"
to"country_code"
might affect queries or code that still expects the old column name. Please verify and update references in your application code accordingly.
20-20
: Unique index on machine fingerprintCreating a unique index on
"fingerprint"
is a good way to prevent duplicates. This change looks correct for identifying machines.
21-22
: Dropping existing columnsRemoving
"access_token"
from"steam"
and"flags"
from"user"
is irreversible without a backup or revert plan. Confirm these columns are no longer needed to avoid data loss.packages/core/src/steam/index.ts (1)
94-105
: Confirm behavior when multiple records exist for a single user.This function returns the first matching record (
rows.map(serialize).at(0)
). If you expect users to possibly have more than one Steam record, review whether selecting just the first one is appropriate or if you need additional selection logic.packages/steam/SteamService.cs (3)
27-48
: Check for possible concurrency issues.If multiple authentication requests occur for the same user concurrently, race conditions may arise when creating or retrieving the handler and setting credentials. Confirm that this logic is safe under parallel calls or consider synchronizing these operations.
50-57
: LGTM.This disconnection logic correctly ensures resources are freed and the user is removed from the handler dictionary.
59-62
: LGTM.The unsubscribe approach is straightforward and effectively detaches the callback.
packages/steam/SteamModels.cs (3)
1-6
: Appropriate imports and namespace setup.The file correctly imports the necessary dependencies for Entity Framework Core and data annotations, setting up a dedicated Steam namespace for the models.
7-22
: Database context properly configured with primary keys.The
SteamDbContext
is well-structured with constructor dependency injection and appropriate DbSet properties. TheOnModelCreating
method correctly defines primary keys for both entities.
36-46
: Model properties align well with Steam API data structure.The properties in
SteamAccountInfo
match the structure shown in the example code frompackages/core/src/examples.ts
, which suggests good alignment with the Steam data you're working with.
b.Property<string>("RefreshToken") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider securing the RefreshToken
The RefreshToken
is stored directly in the database without any encryption or hashing. Since refresh tokens grant long-term access to user accounts, they should be treated as sensitive data.
Consider implementing encryption for this field or at minimum adding a comment indicating that sensitive data should be encrypted before storage. If using Entity Framework's value converters would be appropriate:
// In OnModelCreating method of SteamDbContext
modelBuilder.Entity<SteamUserCredentials>()
.Property(e => e.RefreshToken)
.HasConversion(
v => EncryptToken(v),
v => DecryptToken(v));
namespace steam.Migrations | ||
{ | ||
[DbContext(typeof(SteamDbContext))] | ||
[Migration("20250410104213_new")] | ||
partial class @new | ||
{ | ||
/// <inheritdoc /> | ||
protected override void BuildTargetModel(ModelBuilder modelBuilder) | ||
{ | ||
#pragma warning disable 612, 618 | ||
modelBuilder.HasAnnotation("ProductVersion", "9.0.3"); | ||
|
||
modelBuilder.Entity("Steam.SteamAccountInfo", b => | ||
{ | ||
b.Property<string>("UserId") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("AvatarUrl") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("Country") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("Email") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<ulong>("GameId") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<string>("GamePlayingName") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<bool>("IsAllowedToInviteFriends") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<bool>("IsBanned") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<bool>("IsLimited") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<bool>("IsLocked") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<DateTime>("LastLogOff") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<DateTime>("LastLogOn") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("PersonaName") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<ulong>("SteamId") | ||
.HasColumnType("INTEGER"); | ||
|
||
b.Property<DateTime>("UpdatedAt") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("Username") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.HasKey("UserId"); | ||
|
||
b.ToTable("SteamAccountInfo"); | ||
}); | ||
|
||
modelBuilder.Entity("Steam.SteamUserCredentials", b => | ||
{ | ||
b.Property<string>("UserId") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("AccountName") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<DateTime>("CreatedAt") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("RefreshToken") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<DateTime>("UpdatedAt") | ||
.HasColumnType("TEXT"); | ||
|
||
b.HasKey("UserId"); | ||
|
||
b.ToTable("SteamUserCredentials"); | ||
}); | ||
#pragma warning restore 612, 618 | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Verify consistency with JavaScript model
There are naming inconsistencies between this database schema and the JavaScript model in packages/core/src/examples.ts
:
- DB:
Country
vs JS:countryCode
- DB:
SteamId
vs JS:steamID
- DB schema has flattened boolean properties, while JS model has them nested in a
limitation
object - DB:
GameId
/GamePlayingName
vs JS: nestedlastGame
object withgameID
/gameName
Ensure consistent naming and structure across your codebase to avoid confusion.
async getUser() { | ||
console.log('\nMaking GET request to /user endpoint:'); | ||
const userResponse = await this.makeGetRequest('/user'); | ||
console.log('Response:', JSON.parse(userResponse as any)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve typings and return type in getUser method.
The method should have a return type annotation and avoid using as any
by properly typing the response.
- async getUser() {
+ public async getUser(): Promise<any> {
console.log('\nMaking GET request to /user endpoint:');
const userResponse = await this.makeGetRequest('/user');
- console.log('Response:', JSON.parse(userResponse as any));
+ const userData = JSON.parse(userResponse);
+ console.log('Response:', userData);
+ return userData;
}
📝 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.
async getUser() { | |
console.log('\nMaking GET request to /user endpoint:'); | |
const userResponse = await this.makeGetRequest('/user'); | |
console.log('Response:', JSON.parse(userResponse as any)); | |
} | |
public async getUser(): Promise<any> { | |
console.log('\nMaking GET request to /user endpoint:'); | |
const userResponse = await this.makeGetRequest('/user'); | |
const userData = JSON.parse(userResponse); | |
console.log('Response:', userData); | |
return userData; | |
} |
} | ||
} | ||
|
||
makeGetRequest(path: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add access modifier and return type to makeGetRequest method.
The method should have an access modifier and proper return type annotation for clarity and type safety.
- makeGetRequest(path: string) {
+ public makeGetRequest(path: string): Promise<string> {
📝 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.
makeGetRequest(path: string) { | |
public makeGetRequest(path: string): Promise<string> { |
this.httpClient = http.request(options, (res) => { | ||
let data = ''; | ||
|
||
res.on('data', (chunk) => { | ||
data += chunk; | ||
}); | ||
|
||
res.on('end', () => { | ||
console.log(`Status Code: ${res.statusCode}`); | ||
resolve(data); | ||
}); | ||
}); | ||
|
||
this.httpClient.on('error', (error) => { | ||
console.error(`Error making request: ${error.message}`); | ||
reject(error); | ||
}); | ||
|
||
// req.write(body); | ||
this.httpClient.end(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid overwriting httpClient without properly closing previous connections.
Each call to makeGetRequest
assigns a new request to httpClient
, potentially overwriting an existing one without properly closing it first, which could lead to resource leaks.
- this.httpClient = http.request(options, (res) => {
+ const req = http.request(options, (res) => {
let data = '';
res.on('data', (chunk) => {
data += chunk;
});
res.on('end', () => {
console.log(`Status Code: ${res.statusCode}`);
resolve(data);
});
});
- this.httpClient.on('error', (error) => {
+ req.on('error', (error) => {
console.error(`Error making request: ${error.message}`);
reject(error);
});
// req.write(body);
- this.httpClient.end();
+ req.end();
Consider using a separate variable for one-off requests or ensure existing connections are properly closed before reassigning.
📝 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.
this.httpClient = http.request(options, (res) => { | |
let data = ''; | |
res.on('data', (chunk) => { | |
data += chunk; | |
}); | |
res.on('end', () => { | |
console.log(`Status Code: ${res.statusCode}`); | |
resolve(data); | |
}); | |
}); | |
this.httpClient.on('error', (error) => { | |
console.error(`Error making request: ${error.message}`); | |
reject(error); | |
}); | |
// req.write(body); | |
this.httpClient.end(); | |
}); | |
const req = http.request(options, (res) => { | |
let data = ''; | |
res.on('data', (chunk) => { | |
data += chunk; | |
}); | |
res.on('end', () => { | |
console.log(`Status Code: ${res.statusCode}`); | |
resolve(data); | |
}); | |
}); | |
req.on('error', (error) => { | |
console.error(`Error making request: ${error.message}`); | |
reject(error); | |
}); | |
// req.write(body); | |
req.end(); | |
}); |
app.MapGet("/user", async (HttpContext context, SteamService steamService) => | ||
{ | ||
// Also check standard claims as fallback | ||
userId = jwtToken.Claims.FirstOrDefault(c => c.Type == "sub")?.Value; | ||
email = jwtToken.Claims.FirstOrDefault(c => c.Type == "email")?.Value; | ||
var userId = context.Request.Headers["user-id"].ToString(); | ||
if (string.IsNullOrEmpty(userId)) | ||
{ | ||
return Results.BadRequest("Missing user ID"); | ||
} | ||
|
||
if (string.IsNullOrEmpty(userId) || string.IsNullOrEmpty(email)) | ||
// Get user info from database | ||
var userInfo = await steamService.GetCachedUserInfoAsync(userId); | ||
if (userInfo == null) | ||
{ | ||
return (false, null, null); | ||
return Results.NotFound(new { error = "User not authenticated with Steam" }); | ||
} | ||
|
||
return Results.Ok(new | ||
{ | ||
userId = userInfo.UserId, | ||
username = userInfo.Username, | ||
steamId = userInfo.SteamId, | ||
email = userInfo.Email, | ||
country = userInfo.Country, | ||
personaName = userInfo.PersonaName, | ||
avatarUrl = userInfo.AvatarUrl, | ||
isLimited = userInfo.IsLimited, | ||
isLocked = userInfo.IsLocked, | ||
isBanned = userInfo.IsBanned, | ||
isAllowedToInviteFriends = userInfo.IsAllowedToInviteFriends, | ||
lastGame = new | ||
{ | ||
gameId = userInfo.GameId, | ||
name = userInfo.GamePlayingName | ||
}, | ||
lastLogOn = userInfo.LastLogOn, | ||
lastLogOff = userInfo.LastLogOff | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add authentication check to user info endpoint.
The user info endpoint only verifies that a user ID is provided, but doesn't authenticate the request. This could allow unauthorized access to user data if the user ID is known.
app.MapGet("/user", async (HttpContext context, SteamService steamService) =>
{
var userId = context.Request.Headers["user-id"].ToString();
if (string.IsNullOrEmpty(userId))
{
return Results.BadRequest("Missing user ID");
}
+ // Verify request has valid authentication token or session
+ // This is placeholder code - implement your actual authentication logic
+ var authHeader = context.Request.Headers["Authorization"].ToString();
+ if (string.IsNullOrEmpty(authHeader) || !authHeader.StartsWith("Bearer "))
+ {
+ return Results.Unauthorized();
+ }
+
+ // Verify the token is valid and matches the requested userId
+ var token = authHeader.Substring(7);
+ if (!steamService.ValidateAuthToken(token, userId))
+ {
+ return Results.Forbid();
+ }
// Get user info from database
var userInfo = await steamService.GetCachedUserInfoAsync(userId);
if (userInfo == null)
{
return Results.NotFound(new { error = "User not authenticated with Steam" });
}
return Results.Ok(new
{
// Existing user data...
});
});
📝 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.
app.MapGet("/user", async (HttpContext context, SteamService steamService) => | |
{ | |
// Also check standard claims as fallback | |
userId = jwtToken.Claims.FirstOrDefault(c => c.Type == "sub")?.Value; | |
email = jwtToken.Claims.FirstOrDefault(c => c.Type == "email")?.Value; | |
var userId = context.Request.Headers["user-id"].ToString(); | |
if (string.IsNullOrEmpty(userId)) | |
{ | |
return Results.BadRequest("Missing user ID"); | |
} | |
if (string.IsNullOrEmpty(userId) || string.IsNullOrEmpty(email)) | |
// Get user info from database | |
var userInfo = await steamService.GetCachedUserInfoAsync(userId); | |
if (userInfo == null) | |
{ | |
return (false, null, null); | |
return Results.NotFound(new { error = "User not authenticated with Steam" }); | |
} | |
return Results.Ok(new | |
{ | |
userId = userInfo.UserId, | |
username = userInfo.Username, | |
steamId = userInfo.SteamId, | |
email = userInfo.Email, | |
country = userInfo.Country, | |
personaName = userInfo.PersonaName, | |
avatarUrl = userInfo.AvatarUrl, | |
isLimited = userInfo.IsLimited, | |
isLocked = userInfo.IsLocked, | |
isBanned = userInfo.IsBanned, | |
isAllowedToInviteFriends = userInfo.IsAllowedToInviteFriends, | |
lastGame = new | |
{ | |
gameId = userInfo.GameId, | |
name = userInfo.GamePlayingName | |
}, | |
lastLogOn = userInfo.LastLogOn, | |
lastLogOff = userInfo.LastLogOff | |
}); | |
}); | |
app.MapGet("/user", async (HttpContext context, SteamService steamService) => | |
{ | |
var userId = context.Request.Headers["user-id"].ToString(); | |
if (string.IsNullOrEmpty(userId)) | |
{ | |
return Results.BadRequest("Missing user ID"); | |
} | |
// Verify request has valid authentication token or session | |
// This is placeholder code - implement your actual authentication logic | |
var authHeader = context.Request.Headers["Authorization"].ToString(); | |
if (string.IsNullOrEmpty(authHeader) || !authHeader.StartsWith("Bearer ")) | |
{ | |
return Results.Unauthorized(); | |
} | |
// Verify the token is valid and matches the requested userId | |
var token = authHeader.Substring(7); | |
if (!steamService.ValidateAuthToken(token, userId)) | |
{ | |
return Results.Forbid(); | |
} | |
// Get user info from database | |
var userInfo = await steamService.GetCachedUserInfoAsync(userId); | |
if (userInfo == null) | |
{ | |
return Results.NotFound(new { error = "User not authenticated with Steam" }); | |
} | |
return Results.Ok(new | |
{ | |
userId = userInfo.UserId, | |
username = userInfo.Username, | |
steamId = userInfo.SteamId, | |
email = userInfo.Email, | |
country = userInfo.Country, | |
personaName = userInfo.PersonaName, | |
avatarUrl = userInfo.AvatarUrl, | |
isLimited = userInfo.IsLimited, | |
isLocked = userInfo.IsLocked, | |
isBanned = userInfo.IsBanned, | |
isAllowedToInviteFriends = userInfo.IsAllowedToInviteFriends, | |
lastGame = new | |
{ | |
gameId = userInfo.GameId, | |
name = userInfo.GamePlayingName | |
}, | |
lastLogOn = userInfo.LastLogOn, | |
lastLogOff = userInfo.LastLogOff | |
}); | |
}); |
modelBuilder.Entity("Steam.SteamUserCredentials", b => | ||
{ | ||
b.Property<string>("UserId") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("AccountName") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<DateTime>("CreatedAt") | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<string>("RefreshToken") | ||
.IsRequired() | ||
.HasColumnType("TEXT"); | ||
|
||
b.Property<DateTime>("UpdatedAt") | ||
.HasColumnType("TEXT"); | ||
|
||
b.HasKey("UserId"); | ||
|
||
b.ToTable("SteamUserCredentials"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify SteamUserCredentials entity fields.
The SteamUserCredentials entity structure looks good. Ensure that the RefreshToken field is properly encrypted or hashed before storage since it contains sensitive authentication information.
🏁 Script executed:
#!/bin/bash
# Check if RefreshToken is encrypted or properly handled before storage
rg -A 10 'RefreshToken' --type cs packages/steam/
Length of output: 11113
Security Improvement: Encrypt/Hash RefreshToken before storage
The entity structure appears correct overall. However, our verification indicates that the RefreshToken is currently stored in plain text (see for example its handling in SteamService.cs
and SteamModels.cs
). Since this field contains sensitive authentication data, please implement proper encryption or hashing before storing it in the database.
- Action Required:
- Update the logic in the relevant service (e.g.,
SteamService.cs
) to encrypt/hash theRefreshToken
prior to database storage. - Ensure consistency in retrieval and verification of the encrypted/hashed value.
- Update the logic in the relevant service (e.g.,
private void OnLoggedOn(SteamUser.LoggedOnCallback callback) | ||
{ | ||
if (callback.Result != EResult.OK) | ||
{ | ||
NotifyEvent(new ServerSentEvent("error", new { message = $"Login failed: {callback.Result}" })); | ||
_authCompletionSource?.SetResult(false); | ||
return; | ||
} | ||
|
||
_isLoggedIn = true; | ||
NotifyEvent(new ServerSentEvent("status", new { message = "Successfully logged in to Steam" })); | ||
|
||
// Store basic info | ||
if (callback.ClientSteamID != null) | ||
{ | ||
_accountInfo["steamId"] = callback.ClientSteamID.ConvertToUInt64(); | ||
} | ||
|
||
NotifyEvent(new ServerSentEvent("login-success", new | ||
{ | ||
steamId = _steamUser.SteamID?.ConvertToUInt64(), | ||
username = _username | ||
})); | ||
|
||
// Save credentials if callback provided | ||
if (_onCredentialsObtained != null && !string.IsNullOrEmpty(_refreshToken) && !string.IsNullOrEmpty(_username)) | ||
{ | ||
_onCredentialsObtained(_username, _refreshToken); | ||
} | ||
|
||
// Request persona state | ||
if (_steamUser.SteamID != null) | ||
{ | ||
_steamFriends.RequestFriendInfo(_steamUser.SteamID); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Secure handling of tokens and PII
You store _refreshToken
and _username
in fields and emit them in logs/events (“Logging in as ...” for the username). While the refresh token isn’t directly printed, ensure these credentials are never inadvertently exposed. Storing or logging such sensitive data should follow a secure storage or masking strategy.
public ulong GameId { get; set; } | ||
public string GamePlayingName { get; set; } = string.Empty; | ||
public DateTime LastLogOn { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Consider creating a separate entity for game information.
The current model includes game information directly in the SteamAccountInfo
class, which limits tracking to only the most recently played game.
If your application needs to track multiple games per user, consider creating a separate SteamGame
or UserGame
entity with a many-to-one relationship to SteamAccountInfo
.
🏁 Script executed:
#!/bin/bash
# Check if there are other places in the codebase that might suggest
# a need for tracking multiple games per user
# Search for references to game-related models or multiple games
rg -i -A 2 -B 2 "games.*steam|steam.*games"
# Check if there are any models with collections of games
ast-grep --pattern 'class $_ { $$$; public $_ Games { get; set; } $$$; }'
Length of output: 7945
Action Required: Enhance Multiple Game Tracking in Steam Models
Based on our verification, the front-end components (e.g., in packages/www/src/pages/team/home.tsx
) clearly expect to handle multiple game entries per user. However, the current backend model in packages/steam/SteamModels.cs
(lines 50-52) only supports a single game's properties (i.e., GameId
, GamePlayingName
, and LastLogOn
). To align the backend with the UI expectations, please consider refactoring as follows:
- Separate Entity Creation: Create a dedicated entity (e.g.,
SteamGame
orUserGame
) to store game-specific information. - Define Relationships: Establish a one-to-many relationship between
SteamAccountInfo
and the new game entity so that users can have multiple games associated with their account. - Review Code Impact: Ensure that any changes to the data model are reflected in both the database schema and the related service layers that interact with these models.
public class SteamUserCredentials | ||
{ | ||
[Key] | ||
public string UserId { get; set; } = string.Empty; | ||
public string AccountName { get; set; } = string.Empty; | ||
public string RefreshToken { get; set; } = string.Empty; | ||
public DateTime CreatedAt { get; set; } = DateTime.UtcNow; | ||
public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider enhancing security for sensitive credential storage.
The SteamUserCredentials
class contains sensitive data (RefreshToken
) which should be handled securely.
Consider implementing encryption for the RefreshToken
or using a secure storage mechanism. Also, consider adding a relationship navigation property to the SteamAccountInfo
entity.
public class SteamUserCredentials
{
[Key]
public string UserId { get; set; } = string.Empty;
public string AccountName { get; set; } = string.Empty;
- public string RefreshToken { get; set; } = string.Empty;
+ // Store encrypted or hashed version of the token
+ public string RefreshToken { get; set; } = string.Empty;
public DateTime CreatedAt { get; set; } = DateTime.UtcNow;
public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;
+
+ // Navigation property
+ public virtual SteamAccountInfo AccountInfo { get; set; }
}
📝 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 class SteamUserCredentials | |
{ | |
[Key] | |
public string UserId { get; set; } = string.Empty; | |
public string AccountName { get; set; } = string.Empty; | |
public string RefreshToken { get; set; } = string.Empty; | |
public DateTime CreatedAt { get; set; } = DateTime.UtcNow; | |
public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; | |
} | |
public class SteamUserCredentials | |
{ | |
[Key] | |
public string UserId { get; set; } = string.Empty; | |
public string AccountName { get; set; } = string.Empty; | |
// Store encrypted or hashed version of the token | |
public string RefreshToken { get; set; } = string.Empty; | |
public DateTime CreatedAt { get; set; } = DateTime.UtcNow; | |
public DateTime UpdatedAt { get; set; } = DateTime.UtcNow; | |
// Navigation property | |
public virtual SteamAccountInfo AccountInfo { get; set; } | |
} |
Description
This adds logic to the homepage
Summary by CodeRabbit