refactor: migrates another part of console-api to strict types#1448
refactor: migrates another part of console-api to strict types#1448
Conversation
WalkthroughThis update introduces stricter type safety, improved null handling, and more precise type annotations across authentication, block, bid, transaction, validator, and scheduler modules. Several method signatures are corrected to accurately reflect nullable return types. Schema definitions are refined for clarity and consistency, and middleware logic is hardened against missing environment variables. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Controller
participant Service
participant Repository
Client->>Middleware: Sends request
Middleware->>Middleware: Checks c.env
alt c.env missing
Middleware->>Controller: Calls next()
else c.env present
Middleware->>Middleware: Gathers client info
Middleware->>Controller: Calls next()
end
Controller->>Service: Handles request
Service->>Repository: Fetches data (may return null)
Repository-->>Service: Returns data or null
Service-->>Controller: Returns data or null
Controller-->>Client: Responds with data or null
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (20)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
|
||
| export const ListBlocksQuerySchema = z.object({ | ||
| limit: z.coerce.number().min(1).max(100).default(defaultLimit).optional().openapi({ | ||
| limit: z.coerce.number().min(1).max(100).optional().default(defaultLimit).openapi({ |
There was a problem hiding this comment.
very likely .optional() is redundant in this sequence, because the input is optional and the final value is not optional (but .default() must go after .optional())
stalniy
force-pushed
the
refactor/api-strict-types
branch
from
57bf31a to
8c1888e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/api/src/transaction/http-schemas/transaction.schema.ts (1)
8-8: Verify that the method chaining order change is intentional.The order of
.optional()and.default()has been swapped. In Zod, this can affect validation behavior:
- Previous:
.default(defaultLimit).optional()- applies default first, then makes optional- Current:
.optional().default(defaultLimit)- makes optional first, then applies defaultThe new order is more conventional and typically correct, but please verify this doesn't change the expected validation behavior for existing API consumers.
apps/api/src/bid/controllers/bid/bid.controller.ts (1)
26-26:Details
❓ Verification inconclusive
Type assertion may bypass type safety.
While the explicit casting to
ListBidsResponse["data"]enforces type conformity, usingascan bypass TypeScript's type checking. Consider if thebidHttpService.list()method should be updated to return the correct type instead.Verify that the bidHttpService.list method returns data compatible with ListBidsResponse["data"]:
🏁 Script executed:
#!/bin/bash # Description: Check the return type of bidHttpService.list method # Expected: Should return data compatible with ListBidsResponse["data"] ast-grep --pattern 'class BidHttpService { $$$ list($$$) { $$$ } $$$ }'Length of output: 85
To pinpoint the
bidHttpService.listreturn type, let’s locate wherebidHttpServiceis defined and inspect itslistmethod:
🏁 Script executed:
#!/bin/bash # 1. Show how the controller imports/uses bidHttpService echo ">> Controller imports:" grep -n "bidHttpService" apps/api/src/bid/controllers/bid/bid.controller.ts # 2. Find all files that mention bidHttpService echo -e "\n>> Files containing 'bidHttpService':" rg -l "bidHttpService" . # 3. For each candidate, look for the 'list' method signature echo -e "\n>> Inspecting 'list' methods in those files:" for f in $(rg -l "bidHttpService" . | grep '\.ts$'); do echo -e "\n--- $f ---" rg -n "list\s*\(" "$f" || echo " (no 'list' method here)" doneLength of output: 1511
To identify the exact return type of
bidHttpService.list, let’s locate its class definition and inspect thelistmethod signature:
🏁 Script executed:
#!/bin/bash # 1. Show how the controller imports the service echo ">> Controller import:" grep -n "import.*BidHttpService" apps/api/src/bid/controllers/bid/bid.controller.ts # 2. Find where BidHttpService is defined echo -e "\n>> Class definitions for BidHttpService:" rg -n "class BidHttpService" . # 3. Inspect the 'list' method in each definition for svc in $(rg -l "class BidHttpService" .); do echo -e "\n--- $svc: ---" rg -n "list\s*(\|list<" -B2 -A2 "$svc" doneLength of output: 794
To inspect the exact signature and return type of
BidHttpService.list, let’s pull a few lines around its definition:
🏁 Script executed:
#!/bin/bash echo ">> Locating `list(` in BidHttpService" rg -n -C5 "list\(" packages/http-sdk/src/bid/bid-http.service.tsLength of output: 805
To confirm whether
Bid[]truly matchesListBidsResponse["data"], let’s locate the definition ofListBidsResponseand inspect itsdatafield:
🏁 Script executed:
#!/bin/bash # 1. Find where ListBidsResponse is declared echo ">> Searching for ListBidsResponse definition:" rg -n "ListBidsResponse" -C3 . # 2. If it’s in a types file, show nearby lines for context for file in $(rg -l "ListBidsResponse" .); do echo -e "\n--- $file (±5 lines) ---" rg -n -C5 "ListBidsResponse" "$file" doneLength of output: 6841
Ensure
bidHttpService.list()output matches your response schemaThe controller currently does:
const bids = await this.bidHttpService.list(userWallet.address, dseq); return { data: bids as ListBidsResponse["data"] };—but
bidHttpService.list()(from @akashnetwork/http-sdk) returns aPromise<Bid[]>, whereas your schema expects aBidResponse[](inferred fromListBidsResponseSchema). Casting withascan mask mismatches between those shapes at compile time and lead to runtime errors.To preserve type safety, consider one of the following:
- Update
BidHttpService.listto returnPromise<ListBidsResponse["data"]>so its signature aligns with your Zod schema.- Explicitly validate or map the
Bid[]to yourBidResponseSchema(e.g. viaListBidsResponseSchema.parse) before returning.apps/api/src/scheduler.ts (1)
46-46: Consider the implications ofRequired<SchedulerConfig>.The
Required<SchedulerConfig>type forces all optional properties to be present, which might not align with the constructor's behavior where properties can still be undefined before the spread operation.Consider using a more specific type that reflects the actual runtime behavior:
-private readonly config: Required<SchedulerConfig>; +private readonly config: SchedulerConfig & { healthchecksEnabled: boolean; errorHandler: (task: TaskDef, error: Error) => void };
🛑 Comments failed to post (3)
apps/api/src/auth/controllers/auth/auth.controller.ts (1)
19-21: 🛠️ Refactor suggestion
Use optional chaining to prevent potential null reference error.
The conditional check
currentUser.userIddoesn't account for the possibility thatcurrentUseritself might beundefined. Based on the setter signature inAuthServicethat acceptsUserOutput | undefined, the safer approach would be to use optional chaining.- if (currentUser.userId) { + if (currentUser?.userId) { await this.auth0.sendVerificationEmail(currentUser.userId); }🤖 Prompt for AI Agents
In apps/api/src/auth/controllers/auth/auth.controller.ts around lines 19 to 21, the code checks currentUser.userId without verifying if currentUser is defined, which can cause a null reference error. Update the condition to use optional chaining by checking currentUser?.userId instead. This ensures the code safely accesses userId only if currentUser is not undefined.apps/api/src/auth/services/auth.service.ts (1)
16-19: 💡 Verification agent
🧩 Analysis chain
Critical type inconsistency needs attention.
The getter returns
UserOutput(non-nullable) while the setter acceptsUserOutput | undefined. This creates a type safety issue where calling the getter after settingundefinedwill violate the return type contract.Consider updating the getter return type to match the setter's nullability:
- get currentUser(): UserOutput { + get currentUser(): UserOutput | undefined { // BUGALERT: https://github.com/akash-network/console/issues/1447 return this.executionContextService.get("CURRENT_USER"); }Verify all usages of
currentUsercan handle the potentially undefined value:
🏁 Script executed:
#!/bin/bash # Description: Find all usages of currentUser getter to verify they handle undefined # Expected: All usages should have null checks or optional chaining rg -A 3 -B 1 "\.currentUser" --type tsLength of output: 4820
🏁 Script executed:
#!/bin/bash # Show setter for currentUser to confirm its signature rg -n -B1 -A5 "set currentUser" apps/api/src/auth/services/auth.service.tsLength of output: 317
🏁 Script executed:
#!/bin/bash # Locate ExecutionContextService definition and inspect get/set methods file=$(rg -l "class ExecutionContextService" -t ts | head -n1) echo "File: $file" echo "=== get signature ===" rg -n "get(" "$file" echo "=== set signature ===" rg -n "set(" "$file"Length of output: 710
🏁 Script executed:
#!/bin/bash # Inspect ExecutionContextService getter and setter signatures file="apps/api/src/core/services/execution-context/execution-context.service.ts" echo "File: $file" echo "--- Getter definitions ---" grep -R -n "^ *get " "$file" echo "--- Setter definitions ---" grep -R -n "^ *set " "$file"Length of output: 569
🏁 Script executed:
#!/bin/bash # Display ExecutionContextService to inspect get/set method signatures file="apps/api/src/core/services/execution-context/execution-context.service.ts" echo "=== Showing $file (first 200 lines) ===" sed -n '1,200p' "$file"Length of output: 1179
Align
currentUsergetter nullability with its setterThe setter accepts
UserOutput | undefinedbut the getter is declared to returnUserOutput, creating a type mismatch and risking runtime errors when the stored value isundefined. Update the getter signature and then safeguard all direct accesses:
- Change in
apps/api/src/auth/services/auth.service.ts:- get currentUser(): UserOutput { + get currentUser(): UserOutput | undefined { // BUGALERT: https://github.com/akash-network/console/issues/1447 return this.executionContextService.get("CURRENT_USER"); }- Key call sites to update (add null‐checks, optional chaining or throw when undefined):
- apps/api/src/notifications/routes/proxy/proxy.route.ts:
headers["x-user-id"] = authService.currentUser.id;- apps/api/src/auth/services/api-key/api-key.service.ts:
multiplethis.authService.currentUser.idusages- apps/api/src/certificate/services/certificate.service.ts:
this.authService.currentUser.id- apps/api/src/billing/services/managed-signer/managed-signer.service.ts:
this.authService.currentUser.userId- (and any other occurrences from
rg '\.currentUser')Make sure each site either guards against
undefinedor throws a clear “unauthorized”/“no user in context” error.📝 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.get currentUser(): UserOutput | undefined { // BUGALERT: https://github.com/akash-network/console/issues/1447 return this.executionContextService.get("CURRENT_USER"); }🤖 Prompt for AI Agents
In apps/api/src/auth/services/auth.service.ts lines 16 to 19, the getter currentUser is declared to return UserOutput but the setter accepts UserOutput | undefined, causing a type mismatch. Update the getter return type to UserOutput | undefined to match the setter. Then, review all usages of currentUser across the codebase (such as in proxy.route.ts, api-key.service.ts, certificate.service.ts, and managed-signer.service.ts) and add null checks, optional chaining, or throw explicit errors when currentUser is undefined to ensure type safety and prevent runtime errors.apps/api/src/auth/services/ability/ability.service.ts (1)
56-58: 💡 Verification agent
🧩 Analysis chain
Verify template handling of null userId values.
The parameter type change to allow
userId: string | nullis good for type safety, but the lodash templates may not handle null values gracefully. Whenuser.userIdis null, template interpolation could produce malformed JSON or runtime errors.Also note the inconsistency: the parameter uses
userIdbut some template conditions referenceuser.id(lines 21, 26, 28) while others useuser.userId(lines 19, 25, 29-31, 34-35, 37-41).Consider adding null checks or defaulting to empty string in the template:
- getAbilityFor(role: Role, user: { userId: string | null }) { - return this.toAbility(this.templates[role]({ user })); + getAbilityFor(role: Role, user: { userId: string | null }) { + const templateUser = { id: user.userId || '', userId: user.userId || '' }; + return this.toAbility(this.templates[role]({ user: templateUser }));Run this script to verify template variable usage:
🏁 Script executed:
#!/bin/bash # Check for inconsistent template variable usage in ability rules echo "Checking template variable usage patterns..." grep -n '\${user\.' apps/api/src/auth/services/ability/ability.service.tsLength of output: 1616
Align template property naming and guard against null userId
The
getAbilityFormethod currently does:getAbilityFor(role: Role, user: { userId: string | null }) { return this.toAbility(this.templates[role]({ user })); }But all of your lodash templates reference
user.id(neveruser.userId), so at runtimeuser.idisundefined, and ifuserIdisnullit will interpolate"null"into your rules. This mismatch will break ability checks or produce invalid conditions.• In apps/api/src/auth/services/ability/ability.service.ts (around lines 56–58):
- Service wraps the incoming user as
{ userId: … }- Templates expect
{ id: … }Please map and default the value before rendering:
getAbilityFor(role: Role, user: { userId: string | null }) { - return this.toAbility(this.templates[role]({ user })); + // Map userId → id and default null to empty string to avoid "null" literal + const templateUser = { + id: user.userId ?? '', + userId: user.userId ?? '', + }; + return this.toAbility(this.templates[role]({ user: templateUser }));This ensures:
user.idis defined for all templates- You avoid injecting
"null"orundefinedinto your JSON condition objects🤖 Prompt for AI Agents
In apps/api/src/auth/services/ability/ability.service.ts around lines 56 to 58, the getAbilityFor method passes the user object with a userId property, but the lodash templates expect a property named id, causing runtime errors or malformed JSON when userId is null. To fix this, map the user object to have an id property instead of userId before passing it to the template, and default id to an empty string if userId is null to prevent injecting "null" or undefined values into the templates.
stalniy
force-pushed
the
refactor/api-strict-types
branch
2 times, most recently
from
5c02abb to
39986a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/middlewares/clientInfoMiddleware.ts (1)
19-21: Good defensive programming practice, but consider adding logging.The early exit when
c.envis falsy is excellent for type safety and prevents potential runtime errors. However, consider adding logging when this condition occurs to aid in debugging and monitoring.if (!c.env) { + logger ??= LoggerService.forContext("clientInfoMiddleware"); + logger.warn("Environment context missing, skipping client info collection"); return next(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/api/src/auth/controllers/auth/auth.controller.ts(1 hunks)apps/api/src/auth/repositories/api-key/api-key.repository.ts(1 hunks)apps/api/src/auth/services/ability/ability.service.ts(1 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/auth/services/user-auth-token/user-auth-token.service.ts(1 hunks)apps/api/src/bid/controllers/bid/bid.controller.ts(1 hunks)apps/api/src/block/controllers/block/block.controller.ts(1 hunks)apps/api/src/block/http-schemas/block.schema.ts(3 hunks)apps/api/src/block/repositories/akash-block/akash-block.repository.ts(2 hunks)apps/api/src/block/services/akash-block/akash-block.service.ts(1 hunks)apps/api/src/middlewares/clientInfoMiddleware.ts(1 hunks)apps/api/src/pricing/http-schemas/pricing.schema.ts(1 hunks)apps/api/src/scheduler.ts(8 hunks)apps/api/src/transaction/controllers/transaction/transaction.controller.ts(1 hunks)apps/api/src/transaction/http-schemas/transaction.schema.ts(1 hunks)apps/api/src/transaction/repositories/transaction/transaction.repository.ts(3 hunks)apps/api/src/transaction/services/transaction/transaction.service.ts(1 hunks)apps/api/src/validator/controllers/validator/validator.controller.ts(1 hunks)apps/api/src/validator/services/validator/validator.service.ts(2 hunks)apps/api/tsconfig.build.json(1 hunks)apps/api/tsconfig.strict.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/api/src/pricing/http-schemas/pricing.schema.ts
- apps/api/src/auth/services/ability/ability.service.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/api/tsconfig.build.json
- apps/api/src/auth/controllers/auth/auth.controller.ts
- apps/api/src/transaction/http-schemas/transaction.schema.ts
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/bid/controllers/bid/bid.controller.ts
- apps/api/src/auth/services/user-auth-token/user-auth-token.service.ts
- apps/api/src/auth/repositories/api-key/api-key.repository.ts
- apps/api/tsconfig.strict.json
- apps/api/src/block/http-schemas/block.schema.ts
- apps/api/src/validator/controllers/validator/validator.controller.ts
- apps/api/src/block/services/akash-block/akash-block.service.ts
- apps/api/src/block/controllers/block/block.controller.ts
- apps/api/src/transaction/controllers/transaction/transaction.controller.ts
- apps/api/src/transaction/services/transaction/transaction.service.ts
- apps/api/src/validator/services/validator/validator.service.ts
- apps/api/src/transaction/repositories/transaction/transaction.repository.ts
- apps/api/src/block/repositories/akash-block/akash-block.repository.ts
- apps/api/src/scheduler.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: validate-api
- GitHub Check: test-api-build
🔇 Additional comments (2)
apps/api/src/middlewares/clientInfoMiddleware.ts (2)
23-44: Well-structured middleware implementation.The error handling, fingerprint generation, and context setting logic is well-implemented. The try-catch-finally pattern ensures the middleware chain continues even if client info collection fails.
19-21:Details
✅ Verification successful
Verify downstream dependencies on clientInfo.
Since
clientInfowon't be set whenc.envis falsy, ensure that downstream middleware and handlers can gracefully handle missing client information.
🏁 Script executed:
#!/bin/bash # Description: Check for potential dependencies on clientInfo in downstream code # Expected: Find usages and ensure they handle undefined clientInfo gracefully # Search for clientInfo usage in the codebase rg -A 3 "clientInfo" --type ts --type js # Look for potential destructuring or direct access that might not handle undefined rg -A 3 "\.clientInfo\." --type ts --type jsLength of output: 5213
No action needed – downstream code safely handles missing
clientInfoAll usages of
clientInfoguard against it being undefined:
- packages/logging/src/hono/http-logger/http-logger.service.ts uses
if (clientInfo) { … }before access- apps/api/src/user/controllers/user/user.controller.ts uses optional chaining (
this.httpContext.var.clientInfo?.ip, etc.)- apps/api/src/routers/userRouter.ts (and related routes) use
c.var.clientInfo?.…Since every downstream consumer either checks for existence or uses
?., they will gracefully handle the absence ofclientInfo.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (56.41%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1448 +/- ##
==========================================
- Coverage 38.95% 38.93% -0.02%
==========================================
Files 851 851
Lines 20230 20239 +9
Branches 3673 3680 +7
==========================================
Hits 7880 7880
- Misses 11642 11646 +4
- Partials 708 713 +5
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/api-strict-types
branch
from
39986a7 to
e11ead5
Compare
stalniy
force-pushed
the
refactor/api-strict-types
branch
from
e11ead5 to
908e1f9
Compare
2 participants
Why
To improve project stability and get early feedback on types incompatibilities
What
Migrates another part of console api files to strict types
Summary by CodeRabbit
Bug Fixes
Refactor
Chores