Skip to content

Conversation

@ChiragAgg5k
Copy link
Member

@ChiragAgg5k ChiragAgg5k commented Dec 22, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Build now fails on TypeScript errors to prevent producing incomplete binaries.
    • Fixed multipart file handling to use explicit upload objects for more reliable uploads.
  • Improvements

    • Stronger, clearer type definitions across CLI config and APIs for more predictable behavior.
    • Upload progress callback uses richer progress details for better progress reporting.
    • Pagination API supports both plain arrays and wrapped results with totals for flexible responses.

✏️ Tip: You can customize this high-level summary in your review settings.

@ChiragAgg5k ChiragAgg5k requested a review from Copilot December 22, 2025 13:01
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Warning

Rate limit exceeded

@ChiragAgg5k has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 6e621e7 and b1cbac9.

📒 Files selected for processing (3)
  • .github/workflows/tests.yml
  • templates/cli/lib/client.ts.twig
  • templates/cli/lib/types.ts.twig

Walkthrough

The PR modernizes and tightens the CLI SDK TypeScript templates and CI build. Changes include: removing build fallbacks so TypeScript/pack builds now fail on error; adding enum generation and new enum template; introducing FileInput/UploadProgress types and switching upload handling to Upload objects (stream/filename/size); changing TYPE_OBJECT to emit string types; migrating several require-style imports to ESM; adding paginate overloads; broad, stricter retyping of config and project types; wrapping many inquirer prompts in arrays; and updating test node version to 20. Multiple template files and type signatures were added or adjusted.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • Verify upload flow updates across:
    • templates/cli/base/params.twig
    • templates/cli/base/requests/file.twig
    • templates/cli/lib/client.ts.twig
    • templates/cli/lib/types.ts.twig
  • Validate type surface and exports in:
    • templates/cli/lib/types.ts.twig (many new/renamed interfaces)
    • templates/cli/lib/config.ts.twig (readonly path, typed returns, API changes)
    • src/SDK/Language/CLI.php (enum generation and TYPE_OBJECT → string mapping)
  • Review paginate overloads and implementation:
    • templates/cli/lib/paginate.ts.twig — ensure runtime behavior matches overloads and casts
  • Inspect changes to command templates for:
    • import style migrations (ignore, commander, undici/Blob)
    • inquirer prompt array wrapping in init/pull/push/run and correct answer extraction
  • CI/build and package changes:
    • .github/workflows/sdk-build-validation.yml and templates/cli/package.json.twig — build now fails on tsc/npm build errors
  • Tests:
    • tests/CLINode20Test.php — Node version bump and class rename

Potential tricky spots: cross-file type references (ensure no broken imports/unused imports), new public CHUNK_SIZE visibility, and any generated code consumers expecting previous FileUpload shapes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: cli typescript errors' is directly related to the main changes in the PR, which involve fixing TypeScript type safety issues, import styles, and compilation strictness across the CLI codebase.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (4)
templates/cli/lib/enums/enum.ts.twig (1)

1-7: Clean TypeScript enum template with optional escaping consideration.

The template correctly:

  • Exports the enum with proper casing
  • Handles both keyed and non-keyed enum definitions
  • Sanitizes keys by removing dashes (invalid in TS identifiers)
  • Uses string literal values

Consider adding escaping for enum values: since enum values come directly from the API spec without sanitization, if a value contains special characters like single quotes, it could break the generated TypeScript. Add | escape('js') to the value: '{{ value | escape('js') }}'. Twig's escape('js') filter is standard and designed for this use case, encoding unsafe characters using backslash escape sequences.

templates/cli/lib/types.ts.twig (1)

136-179: Consider consolidating similar interfaces.

AttributeConfig (lines 142-160) and ColumnConfig (lines 181-199) have nearly identical structures, differing only in relatedCollection vs relatedTable. Similarly, IndexConfig and TableIndexConfig differ only in attributes vs columns.

If these represent the same underlying concept with different naming conventions (collections vs tables), consider using a shared base interface with mapped types or type parameters to reduce duplication.

🔎 Example refactor for consideration
// Shared base interface
interface BaseAttributeConfig {
    key: string;
    type: string;
    required?: boolean;
    array?: boolean;
    // ... other shared fields
}

// Specific interfaces extend with their unique fields
export interface AttributeConfig extends BaseAttributeConfig {
    relatedCollection?: string;
}

export interface ColumnConfig extends BaseAttributeConfig {
    relatedTable?: string;
}
templates/cli/base/params.twig (1)

80-83: Clarify the purpose of the boolean-to-empty-array coercion.

The (param as unknown) === true check suggests handling a CLI edge case where an array parameter might receive true as a value (likely from Commander.js when a flag is provided without arguments). Consider adding a comment in the template explaining this behavior.

🔎 Suggested clarification
 {% elseif parameter.type == 'array' %}
+    // Handle edge case where Commander.js passes `true` for flags without arguments
     {{ parameter.name | caseCamel | escapeKeyword}} = ({{ parameter.name | caseCamel | escapeKeyword}} as unknown) === true ? [] : {{ parameter.name | caseCamel | escapeKeyword}};
templates/cli/lib/commands/run.ts.twig (1)

250-256: Consider documenting tar extraction option removal.

The gzip option was removed from the tar.extract() call. If the tar file is no longer gzipped at this point (Line 255: file: buildPath), this is correct. However, a brief comment explaining why gzip decompression is not needed would improve clarity.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14b9ebd and e2b2431.

📒 Files selected for processing (19)
  • .github/workflows/sdk-build-validation.yml
  • src/SDK/Language/CLI.php
  • templates/cli/base/params.twig
  • templates/cli/base/requests/file.twig
  • templates/cli/index.ts.twig
  • templates/cli/lib/client.ts.twig
  • templates/cli/lib/commands/command.ts.twig
  • templates/cli/lib/commands/generic.ts.twig
  • templates/cli/lib/commands/init.ts.twig
  • templates/cli/lib/commands/pull.ts.twig
  • templates/cli/lib/commands/push.ts.twig
  • templates/cli/lib/commands/run.ts.twig
  • templates/cli/lib/config.ts.twig
  • templates/cli/lib/emulation/docker.ts.twig
  • templates/cli/lib/enums/enum.ts.twig
  • templates/cli/lib/paginate.ts.twig
  • templates/cli/lib/questions.ts.twig
  • templates/cli/lib/types.ts.twig
  • templates/cli/package.json.twig
⏰ 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). (18)
  • GitHub Check: Agent
  • GitHub Check: build (8.3, KotlinJava8)
  • GitHub Check: build (8.3, PHP83)
  • GitHub Check: build (8.3, Android5Java17)
  • GitHub Check: build (8.3, AppleSwift56)
  • GitHub Check: build (8.3, CLINode16)
  • GitHub Check: build (8.3, Swift56)
  • GitHub Check: build (8.3, DotNet90)
  • GitHub Check: build (8.3, WebChromium)
  • GitHub Check: build (8.3, WebNode)
  • GitHub Check: build (8.3, Node18)
  • GitHub Check: build (8.3, Android14Java17)
  • GitHub Check: build (8.3, DotNet80)
  • GitHub Check: build (8.3, DotNet60)
  • GitHub Check: build (8.3, DartStable)
  • GitHub Check: swift (server)
  • GitHub Check: android (client)
  • GitHub Check: apple (client)
🔇 Additional comments (39)
.github/workflows/sdk-build-validation.yml (1)

151-154: LGTM! Stricter build validation now enforced.

Removing the fallback || echo ... ensures the CI pipeline will fail on TypeScript compilation errors, which aligns with the PR objective to fix CLI TypeScript errors. This is the correct approach—builds should not silently pass with type errors.

src/SDK/Language/CLI.php (2)

299-303: LGTM! Enum generation support added.

The new enum generation entry follows the existing pattern and will generate TypeScript enum files for each enum definition.


392-392: TYPE_OBJECT → 'string' mapping in CLI is correct.

This change correctly reflects CLI parameter handling—objects are passed as JSON strings via command-line arguments. The getParamExample method confirms objects are serialized to strings (e.g., '{ "key": "value" }'), and this type mapping aligns the CLI type annotation with actual runtime behavior. No compatibility issues arise since this is CLI-specific type naming and doesn't affect other language generators or API parameter deserialization.

templates/cli/lib/questions.ts.twig (3)

40-40: LGTM! Comprehensive type for choices property.

The expanded union type correctly covers all usage patterns in the file:

  • Async functions with answers parameter
  • Async functions without parameters
  • Static arrays of choices or strings

253-254: LGTM! Added explicit response typing.

The generic type parameter <{ regions: any[] }> provides type safety when accessing response.regions.


714-714: LGTM! Appropriate response typing.

Using { version?: string } correctly marks version as optional, which aligns with the null check on line 715.

templates/cli/lib/emulation/docker.ts.twig (3)

1-1: LGTM! Migrated to ES module import syntax.

Switching from CommonJS require to ES module import is the correct modernization approach for TypeScript.


11-11: LGTM! Centralized type import.

Using import type for FunctionConfig from the shared types module improves maintainability by consolidating type definitions.


54-54: LGTM! Correct ES module usage.

Changed from ignore.default() to ignore() which is the proper way to invoke the default export when using ES module imports.

templates/cli/lib/commands/generic.ts.twig (3)

56-59: LGTM - Session now includes endpoint data.

The addSession call now properly includes the endpoint in the session data object, aligning with the SessionData interface defined in types.ts.twig.


284-290: LGTM - Improved type safety for API calls.

Good use of the generic type parameter on client.call<{ version?: string }>() to properly type the response. The session handling is consistent with the login flow.


325-326: LGTM - Explicit type assertions for legacy config migration.

The as string casts are appropriate here since globalConfig.get() returns unknown (based on ConfigData interface), and the preceding has() checks ensure these values exist.

templates/cli/lib/types.ts.twig (4)

1-1: LGTM - Clean response type definition.

Narrow union type for response formats is a good TypeScript practice.


11-24: LGTM - Well-structured upload types.

The FileUpload interface with stream, filename, and size properties provides proper typing for file upload handling. The UploadProgress interface captures all necessary progress tracking fields.


30-51: LGTM - Utility interfaces for CLI operations.

Good addition of Entity, ParsedData, CommandDescription, and CliConfig interfaces to provide proper typing across the CLI codebase.


282-294: LGTM - Comprehensive project configuration type.

ProjectConfigData properly extends ConfigData and aggregates all resource configuration arrays, providing strong typing for the project configuration structure.

templates/cli/base/requests/file.twig (3)

3-3: LGTM - Consistent use of Upload object.

The template now correctly references the Upload object's size property, aligning with the FileUpload interface.


85-95: LGTM - Correct stream iteration pattern.

The for await...of loop correctly iterates over the ReadableStream from the Upload object for chunked uploads.


62-62: Correct File availability statement for Node.js versions.

The new File() constructor is available globally in Node.js 18.13.0 and later, not just Node.js 20+. It works in Node.js v18.16.0 and v20.0.0 but not in v16.20.0 and earlier. If this SDK generator targets Node.js 18.13.0 or later, no polyfill is required. A polyfill is only necessary if the project needs to support Node.js 16 or earlier versions.

templates/cli/base/params.twig (1)

56-62: LGTM - Proper FileUpload object construction.

The generated code correctly constructs the FileUpload object with all required properties (type, stream, filename, size) matching the interface definition.

templates/cli/lib/paginate.ts.twig (3)

5-21: LGTM - Well-designed function overloads.

The overload signatures provide excellent TypeScript inference based on the wrapper parameter:

  • Empty string '' returns T[]
  • Specific key K returns Record<K, T[]> & { total: number }

This enables callers to get precise return types without manual type assertions.


69-77: LGTM - Correct conditional return with type assertion.

The return logic properly handles both cases, and the type assertion as Record<string, T[]> & { total: number } is necessary to satisfy the overloaded return type when using a computed property key.


48-52: Verify that the API response structure meets the assumed contract when wrapper is empty.

When wrapper === '', the code at line 60 accesses response.total after treating response as an array. This means API endpoints called with an empty wrapper must return an array-like response with a .total property, not just a plain array. Confirm this requirement in the API endpoint contract.

templates/cli/lib/config.ts.twig (3)

120-120: LGTM: Visibility change improves encapsulation.

The path property changed from protected to readonly, which is appropriate since it's set in the constructor and exposed via the getter (Line 267). This prevents accidental modification while maintaining access.


183-227: LGTM: Entity typing strengthens type safety.

The DB entity methods now use the Entity type instead of any, providing better type checking throughout the configuration system. The explicit type casts on Lines 214, 221, and 226 are necessary to satisfy TypeScript's type checker when calling the generic set method.


676-695: LGTM: Const assertions and readonly improve immutability.

The as const assertions on static preference constants (Lines 676-684) ensure literal types, and the readonly modifier on IGNORE_ATTRIBUTES (Line 686) prevents runtime modification. This aligns with best practices for constant values.

templates/cli/package.json.twig (1)

17-17: LGTM: Build now fails on TypeScript errors.

Removing the || true fallback ensures TypeScript compilation errors are caught during the build process rather than silently ignored. This aligns with the PR objective to fix CLI TypeScript errors and improves code quality.

templates/cli/index.ts.twig (1)

8-8: LGTM: ES module import modernizes codebase.

The migration from CommonJS require to ES module import for the commander library is appropriate and aligns with the broader TypeScript modernization effort in this PR.

templates/cli/lib/client.ts.twig (2)

125-125: LGTM: Property rename aligns with FileUpload type.

The change from fileUpload.file to fileUpload.stream is consistent with the FileUpload type restructuring mentioned in the AI summary, where the file property was renamed to stream with an added size field.


12-12: Keep CHUNK_SIZE private to maintain encapsulation.

The CHUNK_SIZE property was changed from private readonly to readonly, making it publicly accessible. No external references to this property exist in the codebase, so making it public is unnecessary and violates encapsulation principles. Restore the private modifier.

templates/cli/lib/commands/init.ts.twig (1)

44-44: Inquirer prompt array wrapping pattern.

The prompt calls now wrap individual questions in arrays (Lines 44, 88-90). This pattern is consistent with changes in pull.ts.twig and run.ts.twig. Verify that the inquirer v8.2.4 API handles single-element arrays correctly and that answers properties remain accessible as expected (e.g., answers.resource, answers.organization).

Also applies to: 88-90

templates/cli/lib/commands/command.ts.twig (2)

15-25: LGTM: Dynamic enum imports reduce bundle size.

The conditional enum imports (Lines 15-25) only include enums that are actually used by method parameters. This prevents importing unused enums and keeps the generated code lean. The deduplication logic using addedEnums ensures each enum is imported only once.


70-70: LGTM: UploadProgress type improves type safety.

The onProgress callback now uses the UploadProgress type instead of number, providing structured progress information. The default implementation on Line 88 uses any for the parameter, which is acceptable for a no-op callback, though UploadProgress would be more precise.

Also applies to: 88-88

templates/cli/lib/commands/pull.ts.twig (1)

57-59: No issue with inquirer.prompt wrapping.

The inquirer.prompt() API requires questions as an Array containing Question Objects. Wrapping the single question in an array [questionsPullResources[0]] is the correct and required pattern. The answer is returned as an object where the key is the 'name' property of the question, so accessing answers.resource is correct regardless of whether one or multiple questions are provided. The code is functioning as intended.

templates/cli/lib/commands/run.ts.twig (1)

4-4: ES module import for ignore package is correct.

The change from ignore.default() to ignore() is the correct API for the ignore package when using ES module imports. The default export of the package is the function itself, not an object with a .default property.

Also applies to: 193-193, 258-258

templates/cli/lib/commands/push.ts.twig (4)

95-96: LGTM!

The new enum imports support type-safe casting in pushSettings and align with the PR's TypeScript improvements.


1148-1148: Same prompt wrapping pattern applied consistently.

The same wrapping pattern [questionsPushXxx[0]] is applied to all resource push prompts (sites, functions, buckets, teams, topics). This consistency is good, but the underlying concern about the pattern remains—verify that this is the correct approach rather than a workaround.

Also applies to: 1472-1472, 2296-2296, 2385-2385, 2458-2458


1079-1089: The type casting of service to ApiService and method to AuthMethod is safe and does not require additional validation.

The settings object is constructed via the createSettingsObject method in config.ts.twig, which explicitly maps only predefined, hardcoded keys:

  • services contains only: account, avatars, databases, locale, health, storage, teams, users, sites, functions, graphql, messaging
  • auth.methods contains only: jwt, phone, invites, anonymous, email-otp, magic-url, email-password

Since these keys are constructed programmatically with known, valid enum values rather than from arbitrary user input, the type casting is guaranteed to be valid. No runtime errors from invalid enum values can occur.

Likely an incorrect or invalid review comment.


1104-1116: No config validation concern needed—auth methods are controlled via whitelist.

The settings.auth.methods object is built through createSettingsObject() in config.ts.twig, which explicitly includes only the seven valid auth method keys (jwt, phone, invites, anonymous, email-otp, magic-url, email-password). When iterating via Object.entries() at line 1107, only these predefined keys are present, so the cast to AuthMethod is safe by design.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes TypeScript compilation errors in the CLI by adding proper type definitions, improving type safety, and removing workarounds that suppressed errors. The changes enable strict TypeScript compilation by removing || true from the build script.

  • Reorganized and expanded type definitions with new interfaces for proper type safety
  • Added TypeScript generics and function overloads for better type inference
  • Fixed import statements to use correct module syntax (default imports vs named imports)
  • Added enum support for API parameters with auto-generated enum files

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
templates/cli/package.json.twig Removed error suppression from build script to enforce TypeScript compilation
templates/cli/lib/types.ts.twig Reorganized and expanded type definitions, added new interfaces for entities, configs, and upload progress
templates/cli/lib/questions.ts.twig Enhanced Choice type definition and added generic type parameters to API calls
templates/cli/lib/paginate.ts.twig Added function overloads for better type inference based on wrapper parameter
templates/cli/lib/enums/enum.ts.twig New template for generating TypeScript enums from API specifications
templates/cli/lib/emulation/docker.ts.twig Fixed import statement to use default import and imported FunctionConfig type
templates/cli/lib/config.ts.twig Added explicit type annotations, improved type safety with proper casting, and marked properties as readonly
templates/cli/lib/commands/run.ts.twig Fixed inquirer.prompt call to pass array and removed redundant gzip option from tar.extract
templates/cli/lib/commands/push.ts.twig Added enum imports, wrapped inquirer.prompt calls in arrays, removed invalid API parameters, added type assertions for enum values
templates/cli/lib/commands/pull.ts.twig Wrapped inquirer.prompt call in array for consistency
templates/cli/lib/commands/init.ts.twig Wrapped inquirer.prompt calls in arrays for type consistency
templates/cli/lib/commands/generic.ts.twig Added generic type parameters and provided proper session initialization
templates/cli/lib/commands/command.ts.twig Added type imports for UploadProgress and FileUpload, added enum imports, typed progress callback parameter
templates/cli/lib/client.ts.twig Changed property from private to public readonly and updated FileUpload property access
templates/cli/index.ts.twig Fixed commander import to use named export
templates/cli/base/requests/file.twig Fixed variable naming to use correct Upload suffix
templates/cli/base/params.twig Restructured file upload creation and added type casting for boolean comparison
src/SDK/Language/CLI.php Changed TYPE_OBJECT mapping from 'object' to 'string' and added enum file generation support
.github/workflows/sdk-build-validation.yml Removed error suppression from CLI build validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (3)
templates/cli/lib/commands/push.ts.twig (1)

1079-1089: Consider validating enum values before type assertions.

The type assertions to ApiService (line 1084) and AuthMethod (line 1110) assume configuration values are valid enum members. While acceptable for validated configuration data, runtime validation could make this more robust.

Example: Add validation helpers
function isValidApiService(value: string): value is ApiService {
  return Object.values(ApiService).includes(value as ApiService);
}

function isValidAuthMethod(value: string): value is AuthMethod {
  return Object.values(AuthMethod).includes(value as AuthMethod);
}

// Then use:
for (let [service, status] of Object.entries(settings.services)) {
  if (!isValidApiService(service)) {
    warn(`Skipping invalid service: ${service}`);
    continue;
  }
  await projectsUpdateServiceStatus({
    projectId,
    service,
    status,
    parseOutput: false
  });
}

Also applies to: 1104-1115

templates/cli/lib/types.ts.twig (1)

148-174: Intentional duplication for collection vs. table terminology.

AttributeConfig/IndexConfig and ColumnConfig/TableIndexConfig are parallel structures with different naming (relatedCollection vs relatedTable, attributes vs columns). This appears intentional to support both legacy collection-based and newer table-based APIs. Consider adding a brief comment documenting this relationship if it's meant to be long-term.

Also applies to: 187-213

templates/cli/lib/commands/command.ts.twig (1)

70-70: Consider using UploadProgress type consistently in the default function.

The interface correctly types onProgress as (progress: UploadProgress) => void, but the default implementation uses (progress: any) => {}. This undermines the type safety gained from the interface change. Consider using the proper type:

-        {%- if 'multipart/form-data' in method.consumes -%},onProgress = (progress: any) => {}{%- endif -%}
+        {%- if 'multipart/form-data' in method.consumes -%},onProgress = (_progress: UploadProgress) => {}{%- endif -%}

Using underscore prefix indicates the parameter is intentionally unused in the default implementation.

Also applies to: 88-88

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2b2431 and 6e621e7.

📒 Files selected for processing (6)
  • templates/cli/base/params.twig
  • templates/cli/lib/client.ts.twig
  • templates/cli/lib/commands/command.ts.twig
  • templates/cli/lib/commands/push.ts.twig
  • templates/cli/lib/types.ts.twig
  • tests/CLINode20Test.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • templates/cli/base/params.twig
🧰 Additional context used
🧬 Code graph analysis (1)
tests/CLINode20Test.php (1)
tests/Base.php (1)
  • Base (17-335)
⏰ 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). (17)
  • GitHub Check: build (8.3, Python313)
  • GitHub Check: build (8.3, Python310)
  • GitHub Check: build (8.3, Swift56)
  • GitHub Check: build (8.3, KotlinJava17)
  • GitHub Check: build (8.3, WebChromium)
  • GitHub Check: build (8.3, FlutterBeta)
  • GitHub Check: build (8.3, KotlinJava11)
  • GitHub Check: build (8.3, Node20)
  • GitHub Check: build (8.3, AppleSwift56)
  • GitHub Check: build (8.3, WebNode)
  • GitHub Check: build (8.3, DotNet90)
  • GitHub Check: build (8.3, Android5Java17)
  • GitHub Check: build (8.3, Go112)
  • GitHub Check: build (8.3, Android14Java17)
  • GitHub Check: swift (server)
  • GitHub Check: apple (client)
  • GitHub Check: android (client)
🔇 Additional comments (11)
templates/cli/lib/commands/push.ts.twig (2)

95-96: LGTM! Enum imports improve type safety.

The addition of ApiService and AuthMethod enum imports enhances type safety for the settings push flow.


1025-1025: LGTM! Inquirer prompt calls are consistent.

All inquirer.prompt() calls correctly pass the question arrays directly without unnecessary wrapping. The pattern is consistent across all resource types (resources, sites, functions, buckets, teams, topics).

Also applies to: 1148-1148, 1472-1472, 2296-2296, 2385-2385, 2458-2458

templates/cli/lib/client.ts.twig (3)

2-2: LGTM on the Blob import.

Adding the Blob import from undici is the correct approach for Node.js environments where the global Blob may not be available.


12-12: Visibility change from private to public is intentional.

Changing CHUNK_SIZE to public readonly allows external code (e.g., chunked upload logic in command templates) to access this constant. This aligns with the broader typing improvements in this PR.


125-125: Remove redundant File to Blob cast.

The cast is unnecessary because File extends Blob per the WHATWG spec and undici's FormData.append() already accepts File objects directly. Simplify line 125 to: formData.append(key, fileUpload.file, fileUpload.filename);

Likely an incorrect or invalid review comment.

tests/CLINode20Test.php (2)

8-8: Good upgrade to Node.js 20 LTS.

Node.js 16 reached end-of-life in September 2023. Upgrading to Node.js 20 (current LTS) ensures the CLI SDK tests run on a supported runtime with the latest features and security patches.

Also applies to: 17-23


40-40: ASCII art escape adjustments look correct.

The backslash escaping in the logo strings appears to be adjusted for consistency. Ensure tests pass to confirm these escape sequences render correctly.

Also applies to: 49-49

templates/cli/lib/types.ts.twig (1)

1-57: Comprehensive type additions for CLI configuration.

The new types (UploadProgress, FileInput, CliConfig, ProjectSettings, RawProjectSettings, FunctionConfig, SiteConfig, TableConfig, ProjectConfigData) provide strong typing for CLI configuration and operations. This improves type safety and developer experience.

Also applies to: 66-72, 74-110, 112-146, 176-185, 215-300

templates/cli/lib/commands/command.ts.twig (3)

4-4: Good migration to ESM import.

Switching from require-style to ESM default import for ignore aligns with modern TypeScript/ESM practices and is consistent with the PR's import modernization.


15-25: Smart dynamic enum imports.

Dynamically importing only the enums that are actually used by the service methods prevents unused import warnings and keeps the generated code clean.


14-14: FileInput import is used in the included template cli/base/params.twig.

The import is necessary because {{ include('cli/base/params.twig') }} renders that template within the current scope with access to the imported types. The included template uses FileInput as a type annotation on line 56, so no unused import warning will occur.

Likely an incorrect or invalid review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants