Fix channel rule commands handling of null and correct property names
#98
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRenamed Namespace payload field from channelNamespace to id and updated create command to map name to id. Adjusted logging conditions for batchingInterval and conflationInterval across create, update, and delete commands to use loose null checks (!= null). No changes to command signatures or overall control flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as Channel Rules CLI
participant API as Control API
U->>CLI: apps channel-rules create/update/delete
Note over CLI: Build payload using Namespace.id<br/>(renamed from channelNamespace)
CLI->>API: createNamespace/updateNamespace/deleteNamespace
API-->>CLI: Response (Namespace)
alt --json flag
CLI-->>U: JSON output
else human-readable
Note over CLI: Log batchingInterval / conflationInterval<br/>when value != null
CLI-->>U: Text summary
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/commands/apps/channel-rules/create.ts (1)
122-136: Update tests to expectidinstead ofchannelNamespace
Tests intest/unit/commands/channel-rule/create.test.ts.skipandtest/integration/control-api.test.tsstill assert onchannelNamespacein the POST body—change those toidto match the updated payload key.src/services/control-api.ts (1)
221-238: Update tests to useidinstead ofchannelNamespace
All assertions intest/unit/commands/channel-rule/create.test.ts.skipandtest/integration/control-api.test.tsstill post{ channelNamespace: … }. Replace everychannelNamespacekey withidto match the updated payload.
🧹 Nitpick comments (4)
src/services/control-api.ts (2)
47-64: Align Namespace types with API nulls for intervals.The API may return null for batchingInterval and conflationInterval. Reflect this in types to avoid unsoundness.
Apply:
export interface Namespace { appId: string; authenticated?: boolean; batchingEnabled?: boolean; - batchingInterval?: number; + batchingInterval?: number | null; conflationEnabled?: boolean; - conflationInterval?: number; + conflationInterval?: number | null; conflationKey?: string; created: number; exposeTimeSerial?: boolean; id: string; modified: number; persistLast?: boolean; persisted: boolean; populateChannelRegistry?: boolean; pushEnabled: boolean; tlsOnly?: boolean; }
555-571: Preserve API error codes to enable friendly auth errors in commands.To support handling error.code === 40100 in commands (per guidelines), include the response code when available.
As a minimal change, attach code if present:
- let errorMessage = `API request failed (${response.status} ${response.statusText})`; + let errorMessage = `API request failed (${response.status} ${response.statusText})`; + let errorCode: unknown = undefined; if ( typeof responseData === "object" && responseData !== null && "message" in responseData && typeof responseData.message === "string" ) { errorMessage += `: ${responseData.message}`; + if ("code" in responseData) { + // preserve code for downstream handling + // eslint-disable-next-line @typescript-eslint/no-explicit-any + errorCode = (responseData as any).code; + } } else if ( typeof responseData === "string" && responseData.length < 100 ) { // Include short string responses directly errorMessage += `: ${responseData}`; } - throw new Error(errorMessage); + const err = new Error(errorMessage) as Error & { code?: unknown; status?: number }; + if (errorCode !== undefined) err.code = errorCode; + err.status = response.status; + throw err;Commands can then check for (error as any).code === 40100 to print a login hint. As per coding guidelines.
src/commands/apps/channel-rules/delete.ts (1)
45-51: Avoid appId shadowing inside try.Inner let appId shadows the outer variable, so catch blocks may log undefined appId. Assign to the outer variable instead.
Apply:
- let appId = flags.app; + appId = flags.app; if (!appId) { appId = await this.resolveAppId(flags); }src/commands/apps/channel-rules/create.ts (1)
94-100: Remove appId shadowing inside try.Inner let appId hides the outer variable; errors in the try block will yield undefined appId in catch output. Assign to the outer variable.
Apply:
- let appId = flags.app; + appId = flags.app; if (!appId) { appId = await this.resolveAppId(flags); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/commands/apps/channel-rules/create.ts(3 hunks)src/commands/apps/channel-rules/delete.ts(2 hunks)src/commands/apps/channel-rules/update.ts(2 hunks)src/services/control-api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
src/commands/**/*.ts: Commands should follow the oclif structure: export default class MyCommand extends Command { ... }
Command should support auth via the standard auth helper (import { getAuth } from '../../helpers/auth') and use it in the run method.
Use try/catch for error handling in commands, and provide user-friendly error messages (e.g., handle error.code === 40100 for authentication failures).
Do not use direct HTTP requests (e.g., fetch) for data plane APIs; use the Ably SDK instead.
Do not use hardcoded endpoint URLs; use configuration values (e.g., flags.controlHost, config.controlHost) when constructing URLs.
Do not use non-standard command structures (e.g., export default class { async execute(args) { ... } }); always use the oclif Command class.
Files:
src/commands/apps/channel-rules/update.tssrc/commands/apps/channel-rules/create.tssrc/commands/apps/channel-rules/delete.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
src/commands/apps/channel-rules/update.tssrc/commands/apps/channel-rules/create.tssrc/services/control-api.tssrc/commands/apps/channel-rules/delete.ts
src/commands/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Follow oclif framework best practices as described in the oclif documentation.
Files:
src/commands/apps/channel-rules/update.tssrc/commands/apps/channel-rules/create.tssrc/commands/apps/channel-rules/delete.ts
⏰ 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). (3)
- GitHub Check: setup
- GitHub Check: e2e-cli
- GitHub Check: test
🔇 Additional comments (3)
src/commands/apps/channel-rules/update.ts (1)
308-312: Good null-handling; avoids toString on null.Switching to != null correctly skips logging when values are null or undefined, preventing runtime errors and matching API behavior.
Also applies to: 320-324
src/commands/apps/channel-rules/delete.ts (1)
138-142: Correct null-handling for interval logging.Using != null avoids logging and formatting null values. Good fix.
Also applies to: 150-154
src/commands/apps/channel-rules/create.ts (1)
212-216: Correctly handle null intervals in output.The != null checks prevent logging nulls and avoid calling toString on null. Good.
Also applies to: 224-228
| ); | ||
| } | ||
|
|
||
| if (createdNamespace.batchingInterval !== undefined) { |
There was a problem hiding this comment.
should these be falsy checks?
if (!createdNamespace.batchingInterval) {
Description
This PR fixes issues with the
channel-rulescommands;channel-rules createchannel-rules updatechannel-rules deletechannel-rules createchannelNamepsaceinstead ofidin the request.nullbeing returned forbatchingIntervalandconflationInterval.channel-rules updateandchannel-rules deletenullbeing returned forbatchingIntervalandconflationInterval.Summary by CodeRabbit