fix: confusing server -> status query #1635
Conversation
represent the target server's status instead of whether it's connected to Mothership. Resolves #1627
WalkthroughChanges update GraphQL schema metadata for Server.status and refactor the server resolver to build a single ServerModel instead of an array. The resolver now returns a single object for server() and an array containing that object for servers(), and sets status to ONLINE. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant G as GraphQL API
participant R as ServerResolver
C->>G: Query server { status, wanip, lanip }
G->>R: server()
R->>R: getLocalServer()
Note over R: Build single ServerModel<br/>status := ONLINE
R-->>G: ServerModel
G-->>C: Response { server: { status, wanip, lanip } }
%% Alternative list query
C->>G: Query servers { id, name }
G->>R: servers()
R->>R: getLocalServer()
R-->>G: [ServerModel]
G-->>C: Response { servers: [ ... ] }
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @pujitm's task —— View job Code Review AnalysisTodo List:
Analysis ResultsAfter reviewing the diff, I found one potential issue in the changed lines: Line 73 in server.resolver.ts: The server status is now hardcoded to status: ServerStatus.ONLINE,The previous logic checked the mothership connection status: status: connectConfig?.mothership?.status === MinigraphStatus.CONNECTED
? ServerStatus.ONLINE
: ServerStatus.OFFLINE,Potential Issue: If this change is intentional to represent the target server's status instead of mothership connectivity (as mentioned in the PR description), this could be correct. However, hardcoding it to always
The other changes (return type fixes, array wrapping, description addition) look correct and resolve the confusing query structure. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1)
48-79: Harden getLocalServer: guard emhttp and avoidhttp://:undefinedURLsDirect access to
emhttp.varcan throw if unavailable, andlocalurlmay becomehttp://:undefinedwhenlaniporportis missing. Make the method nullable, guard inputs, and buildlocalurlconditionally.- private getLocalServer(): ServerModel { + private getLocalServer(): ServerModel | null { const emhttp = getters.emhttp(); const connectConfig = this.configService.get('connect'); - - const guid = emhttp.var.regGuid; - const name = emhttp.var.name; - const wanip = ''; - const lanip: string = emhttp.networks[0]?.ipaddr[0] || ''; - const port = emhttp.var?.port; - const localurl = `http://${lanip}:${port}`; - const remoteurl = ''; + if (!emhttp?.var) return null; + + const guid = emhttp.var?.regGuid ?? ''; + const name = emhttp.var?.name ?? 'Local Server'; + const wanip = ''; + const lanip: string = emhttp.networks?.[0]?.ipaddr?.[0] ?? ''; + const port = emhttp.var?.port; + const localurl = lanip ? `http://${lanip}${port ? `:${port}` : ''}` : ''; + const remoteurl = ''; @@ - return { + return { id: 'local', owner, guid: guid || '', apikey: connectConfig?.config?.apikey ?? '', name: name ?? 'Local Server', status: ServerStatus.ONLINE, wanip, lanip, localurl, remoteurl, }; }Follow-ups (if you accept nullable):
- In
server(), returningthis.getLocalServer()is fine because the return type already allowsnull.- In
servers(), guard null as below.
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/servers/server.model.ts (1)
41-43: Clarify status doc to match enum and new semanticsEnum includes NEVER_CONNECTED and the PR intent is to decouple from Mothership. Update the field description accordingly.
- @Field(() => ServerStatus, { - description: 'Whether this server is online or offline', - }) + @Field(() => ServerStatus, { + description: 'The target server’s own status: ONLINE, OFFLINE, or NEVER_CONNECTED (independent of Mothership connectivity).', + })Optionally, also document the enum:
// near registerEnumType(...) registerEnumType(ServerStatus, { name: 'ServerStatus', description: 'Server availability relative to itself; not its Mothership connection.', });api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (2)
27-28: Remove redundant|| null
getLocalServer()never returns a falsy value in current code. Simplify.- return this.getLocalServer() || null; + return this.getLocalServer();
35-37: Align servers() with nullable getLocalServer()If
getLocalServer()can return null, avoid[null].- return [this.getLocalServer()]; + const s = this.getLocalServer(); + return s ? [s] : [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
api/src/unraid-api/graph/resolvers/servers/server.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/servers/server.resolver.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/servers/server.model.tsapi/src/unraid-api/graph/resolvers/servers/server.resolver.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/servers/server.model.tsapi/src/unraid-api/graph/resolvers/servers/server.resolver.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/servers/server.model.tsapi/src/unraid-api/graph/resolvers/servers/server.resolver.ts
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1)
67-74: Hard-coded ServerStatus.ONLINE is correct
Ripgrep search confirms no consumers expect OFFLINE or NEVER_CONNECTED here, so always returning ONLINE matches intended semantics.
🤖 I have created a release *beep* *boop* --- ## [4.18.0](v4.17.0...v4.18.0) (2025-09-02) ### Features * **api:** enhance OIDC redirect URI handling in service and tests ([#1618](#1618)) ([4e945f5](4e945f5)) ### Bug Fixes * api key creation cli ([#1637](#1637)) ([c147a6b](c147a6b)) * **cli:** support `--log-level` for `start` and `restart` cmds ([#1623](#1623)) ([a1ee915](a1ee915)) * confusing server -> status query ([#1635](#1635)) ([9d42b36](9d42b36)) * use unraid css variables in sonner ([#1634](#1634)) ([26a95af](26a95af)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
represent the target server's status instead of whether it's connected
to Mothership.
Resolves #1627
Summary by CodeRabbit
Bug Fixes
Documentation