feat(notifications): re-generates swagger and sdks#2268
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2268 +/- ##
==========================================
- Coverage 51.33% 51.06% -0.28%
==========================================
Files 1071 1061 -10
Lines 29108 28763 -345
Branches 6416 6361 -55
==========================================
- Hits 14942 14687 -255
+ Misses 13831 13689 -142
- Partials 335 387 +52
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
WalkthroughRestructures notification/alert schemas and SDK types: nests alert fields into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/notifications/swagger/swagger.json (1)
4691-5150: Consider de-duplicating the conditions schema via shared$refin a follow-up
AlertPatchInput.data.conditions(and the other alert schemas) inline essentially the same nestedoperator/field/valuestructure multiple times, with only minor differences between generic and balance-only variants. This is correct but quite verbose and harder to maintain if condition semantics evolve.If you plan more alert types or condition operators, consider extracting:
- A generic
ConditionExpressionschema for string/number/boolean fields.- A
BalanceConditionExpressionschema forfield: "balance"+ numericvalue.and then referencing them via
$refin the variousoneOfbranches. That refactor should happen at the source that emits swagger and be tracked as a separate issue rather than in this regeneration PR.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/notifications/swagger/swagger.json(21 hunks)packages/react-query-sdk/src/notifications/schema.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🔇 Additional comments (4)
packages/react-query-sdk/src/notifications/schema.ts (2)
247-290: WALLET_BALANCE AlertCreateInput variant looks structurally consistentThe new
type: "WALLET_BALANCE"branch mirrors the existingDEPLOYMENT_BALANCEbalance-conditions shape (restrictedfield: "balance"and numericvalue) and introducesparamswith{ owner, denom, suppressedBySystem? }, which aligns with the swagger schema and the intended wallet-level semantics. No structural issues stand out here from the SDK consumer perspective.
341-387: Output and list unions correctly model CHAIN_EVENT and WALLET_BALANCE variantsThe added
CHAIN_EVENTandWALLET_BALANCEcases inAlertOutputResponse.dataandAlertListOutputResponse.dataare internally consistent and match the swagger definitions:
- Each variant has a clear discriminator via
type: "CHAIN_MESSAGE" | "CHAIN_EVENT" | "DEPLOYMENT_BALANCE" | "WALLET_BALANCE".- Balance-based variants (
DEPLOYMENT_BALANCE,WALLET_BALANCE) consistently restrict conditions tofield: "balance"withvalue: number.paramspayloads are specialized: deployment balance uses{ dseq, owner, suppressedBySystem? }, wallet balance uses{ owner, denom, suppressedBySystem? }, while chain-based variants keep the existing{ dseq, type, suppressedBySystem? }.This should give React Query consumers a clean discriminated-union surface over the regenerated API.
Also applies to: 453-487, 557-603, 669-702
apps/notifications/swagger/swagger.json (2)
1121-2211: AlertCreateInput: WALLET_BALANCE and extended alert variants are coherentWithin
components.schemas.AlertCreateInput.data.oneOf, the four variants (CHAIN_MESSAGE, CHAIN_EVENT, DEPLOYMENT_BALANCE, WALLET_BALANCE) are well-formed and mutually distinguishable via theirtypeenums. The WALLET balance branch:
- Constrains conditions to numeric
balancechecks (and/or + single condition).- Uses
params{ owner, denom, suppressedBySystem? }withowneranddenomrequired.- Mirrors the deployment-balance pattern while correctly omitting
dseq.This looks consistent with how the SDK types were generated and should be safe to expose as the canonical contract for creating wallet-balance alerts.
2212-4690: AlertOutputResponse & AlertListOutputResponse unions align across all alert typesThe new
AlertOutputResponseandAlertListOutputResponseschemas correctly:
- Standardize common metadata (
notificationChannelId,name,enabled,summary,description,id,userId,status,createdAt,updatedAt).- Discriminate per-alert behavior via
typeenums:"CHAIN_MESSAGE" | "CHAIN_EVENT" | "DEPLOYMENT_BALANCE" | "WALLET_BALANCE".- Keep chain-related branches on generic conditions (
field: string,value: string|number|boolean) and optionalparams { dseq, type, suppressedBySystem? }.- Restrict balance-based branches to
field: "balance"and numericvalue, with specializedparamsfor deployment vs wallet.These shapes match the generated TypeScript in
notifications/schema.tsand give clients a predictable discriminated union across list/read/delete responses as well as create.
ygrishajev
force-pushed
the
feature/notifications-sdk
branch
from
06978f1 to
c253d18
Compare
ygrishajev
force-pushed
the
feature/notifications-sdk
branch
from
c253d18 to
b6a1557
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx(1 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsxapps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-06-05T21:07:51.985Z
Learnt from: baktun14
Repo: akash-network/console PR: 1432
File: apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentCloseAlert.tsx:38-38
Timestamp: 2025-06-05T21:07:51.985Z
Learning: The ContactPointSelect component in apps/deploy-web/src/components/alerts/ContactPointSelectForm/ContactPointSelect.tsx uses the useFormContext hook internally to connect to React Hook Form, so it doesn't need to be wrapped in a FormField component.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
📚 Learning: 2025-11-25T17:45:39.561Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T17:45:39.561Z
Learning: Applies to **/*.{ts,tsx,js} : Never use deprecated methods from libraries.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (2)
apps/notifications/src/modules/alert/model-schemas/alert.schema.ts (1)
Alert(10-42)apps/deploy-web/src/utils/urlUtils.ts (1)
UrlService(16-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx (1)
125-125: LGTM! Improved type safety for dseq extraction.The stricter existence checks (
item.params && "dseq" in item.params) ensure type-safe access to the dseq property before callinggetDeploymentName. This aligns well with the broader restructuring toward explicit param handling across alert components.apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (1)
64-64: Good refactoring with centralized dseq extraction.The
extractDseqhelper successfully consolidates dseq extraction logic, and all usages correctly handle the optional nature of dseq:
- Line 64: Passes extracted dseq to onToggle
- Lines 74-83: Conditionally renders Link only when dseq exists
- Line 87: Uses nullish coalescing for fallback display
Also applies to: 72-84, 85-88
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
Outdated
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/notifications-sdk
branch
from
b6a1557 to
015ad34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (1)
46-51: Complete the logic simplification suggested in the previous review.While the
anytype was correctly changed tounknown, the logic still needs simplification as previously suggested. The current patterndseq || undefinedunnecessarily coerces falsy values and lacks explicit type narrowing.🔎 Apply the previously suggested diff to properly narrow the type and simplify:
const extractDseq = useCallback((info: CellContext<Alert, unknown>) => { const { params } = info.row.original; - const dseq = params && "dseq" in params && params.dseq; - - return dseq || undefined; + return params && typeof params === "object" && params !== null && "dseq" in params ? params.dseq : undefined; }, []);
🧹 Nitpick comments (1)
apps/notifications/swagger/swagger.json (1)
1945-2204: Regenerated alert / notification schemas are coherent and match the SDK typesThe extended schemas for
AlertCreateInput,AlertOutputResponse, andAlertListOutputResponse(includingCHAIN_EVENTandWALLET_BALANCEvariants), plus the standardized error and pagination wrappers, are internally consistent and line up with the generated TS types:
- Balance‑based variants restrict conditions to
field: "balance"and numericvalue, with appropriateparams({ dseq, owner }vs{ owner, denom }).- CHAIN* variants retain the generic condition/value typing and optional
paramswhile being cleanly discriminated viatype.- New shared responses (
ValidationErrorResponse,UnauthorizedErrorResponse,ForbiddenErrorResponse,InternalServerErrorResponse, list outputs, deployment alert DTOs, notification‑channel DTOs) are correctly referenced from the corresponding paths.Given this file is generated from the backend OpenAPI and the TS SDK is derived from it, I would keep this PR focused on regeneration; any broader modeling/doc tweaks (e.g., refining query param descriptions or DRY‑ing repeated condition schemas) are better handled by updating the source API spec in a separate issue/PR and regenerating. Based on learnings, this keeps the scope of this change tight.
Also applies to: 2212-5639
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx(1 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx(4 hunks)apps/notifications/swagger/swagger.json(21 hunks)packages/react-query-sdk/src/notifications/schema.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsxpackages/react-query-sdk/src/notifications/schema.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-11-25T17:45:39.561Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T17:45:39.561Z
Learning: Applies to **/*.{ts,tsx,js} : Never use type `any` or cast to type `any`. Always define the proper TypeScript types.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
📚 Learning: 2025-05-24T04:26:25.511Z
Learnt from: stalniy
Repo: akash-network/console PR: 1362
File: apps/api/src/core/services/open-api-hono-handler/open-api-hono-handler.ts:21-21
Timestamp: 2025-05-24T04:26:25.511Z
Learning: In TypeScript strict mode, double casts like `c as unknown as AppContext` may be necessary even when types are structurally compatible, as strict mode enforces more restrictive type compatibility rules than regular mode.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
📚 Learning: 2025-11-25T17:45:39.561Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T17:45:39.561Z
Learning: Applies to **/*.{ts,tsx,js} : Never use deprecated methods from libraries.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
📚 Learning: 2025-06-05T21:07:51.985Z
Learnt from: baktun14
Repo: akash-network/console PR: 1432
File: apps/deploy-web/src/components/deployments/DeploymentAlerts/DeploymentCloseAlert.tsx:38-38
Timestamp: 2025-06-05T21:07:51.985Z
Learning: The ContactPointSelect component in apps/deploy-web/src/components/alerts/ContactPointSelectForm/ContactPointSelect.tsx uses the useFormContext hook internally to connect to React Hook Form, so it doesn't need to be wrapped in a FormField component.
Applied to files:
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx
🧬 Code graph analysis (1)
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (2)
apps/notifications/src/modules/alert/model-schemas/alert.schema.ts (1)
Alert(10-42)apps/deploy-web/src/utils/urlUtils.ts (1)
UrlService(16-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate (apps/provider-console) / should-validate-unsafe / should-validate
- GitHub Check: Validate local packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/react-query-sdk/src/notifications/schema.ts (1)
247-290: NewCHAIN_EVENT/WALLET_BALANCEalert variants look consistent and type‑safeThe added union arms for
CHAIN_EVENTandWALLET_BALANCEacrossAlertCreateInput,AlertOutputResponse, andAlertListOutputResponsekeep the discriminated‑union pattern (typeliteral), constrain balance conditions tofield: "balance"with numericvalue, and introduceparamswith the expected shapes ({ dseq, owner }for deployment balance and{ owner, denom }for wallet balance). Noanyusage, and everything aligns with the regenerated swagger contracts, so this looks good to ship as‑is, with any future tweaks flowing from the OpenAPI source rather than editing this file directly.Also applies to: 341-387, 453-486, 557-603, 669-702
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.