-
Notifications
You must be signed in to change notification settings - Fork 44
feat: Command Line SDK update for version 13.0.0-rc.5 #232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR releases version 13.0.0-rc.5 of the Appwrite CLI with a fix for the "Push all" functionality not working correctly. The changes include updating version references across documentation, installation scripts, and package manifests from rc.4 to rc.5. The primary functional changes are: introducing code deployment prompts and validation for functions and sites in the push command, expanding type definitions to accept bigint in addition to number for numeric configuration fields, switching JSONBigInt handling from string-based to native BigInt representation, adding error handling for batch attribute updates, and explicit type coercion in calculation operations. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
| code: allowFunctionsCodePush === true, | ||
| withVariables: false, | ||
| }, | ||
| siteOptions: { code: allowSitesCodePush === true, withVariables: false }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default behaviour to not ask for with variables as before, maybe in future we can ask this too
There was a problem hiding this 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
🤖 Fix all issues with AI agents
In @lib/commands/utils/attributes.ts:
- Around line 594-608: The current try/catch around Promise.all for
changedAttributes swallows errors from this.updateAttribute (called on
collection["databaseId"], collection["$id"]) by only logging them; change it to
propagate the failure so callers know updates failed—either re-throw the caught
error after calling error(...) or convert the block to return a failure result
(e.g., throw new Error or return a rejected Promise) so subsequent deletion
logic does not run on partial failure; ensure the handler for the Promise.all
over changedAttributes preserves the original error details when propagating.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mdinstall.ps1install.shlib/client.tslib/commands/config.tslib/commands/push.tslib/commands/schema.tslib/commands/utils/attributes.tslib/commands/utils/pools.tslib/config.tslib/constants.tslib/questions.tspackage.jsonscoop/appwrite.config.json
🧰 Additional context used
🧬 Code graph analysis (2)
lib/commands/push.ts (1)
lib/questions.ts (2)
questionsPushFunctionsCode(371-377)questionsPushSitesCode(414-420)
lib/commands/utils/attributes.ts (1)
lib/commands/push.ts (1)
error(170-174)
🔇 Additional comments (24)
install.ps1 (1)
16-17: LGTM!The download URLs are correctly updated to version 13.0.0-rc.5 for both x64 and arm64 architectures, consistent with the version bump across the repository.
package.json (1)
6-6: LGTM!Version correctly updated to 13.0.0-rc.5, consistent with the release across all package manifests.
CHANGELOG.md (1)
3-5: LGTM!Changelog entry appropriately documents the release with a concise description of the fix.
README.md (2)
32-32: LGTM!Version reference correctly updated to 13.0.0-rc.5 in the installation verification example.
63-63: LGTM!Version reference correctly updated to 13.0.0-rc.5 in the binary installation verification example.
lib/constants.ts (1)
4-4: LGTM!SDK_VERSION constant correctly updated to 13.0.0-rc.5, ensuring consistent version reporting at runtime.
install.sh (1)
99-99: LGTM!Version bump to 13.0.0-rc.5 aligns with the release changes across other files in this PR.
scoop/appwrite.config.json (1)
3-3: LGTM!Scoop manifest version and download URLs are consistently updated to 13.0.0-rc.5 for both x64 and arm64 architectures.
Also applies to: 9-9, 18-18
lib/commands/utils/pools.ts (1)
47-47: LGTM!The
Number(total)coercion correctly handles potential BigInt values returned from the API when using native BigInt JSON parsing. This preventsTypeErrorfrom mixing BigInt with regular number division. Given that attribute/index counts are unlikely to exceed safe integer limits, this is an appropriate fix.Also applies to: 86-86
lib/commands/utils/attributes.ts (1)
4-4: LGTM!Import addition for
errorlogging utility is appropriate.lib/config.ts (1)
27-27: LGTM!Switching to
useNativeBigInt: trueenables native JavaScript BigInt support for proper arithmetic operations. The configuration is consistent acrosslib/config.ts,lib/client.ts, andlib/commands/schema.ts, and aligns with theNumber()coercion added inlib/commands/utils/pools.tsfor handling values that may now be BigInt type.lib/client.ts (1)
22-22: LGTM! Native BigInt support enabled.The switch from
storeAsString: falsetouseNativeBigInt: trueenables native JavaScript BigInt representation during JSON parsing/stringifying. This aligns with the schema changes inlib/commands/config.tsthat now acceptbiginttypes for numeric fields.lib/commands/schema.ts (1)
12-12: LGTM! Consistent BigInt handling in Schema operations.This change aligns with the
lib/client.tsupdate, ensuring config files are read and written with native BigInt support. This maintains consistency across the codebase for handling large integers.lib/questions.ts (2)
371-377: LGTM! New deployment prompt for functions.The new confirmation prompt follows the existing pattern used by
questionsPullFunctionsCode. This enables users to decide whether to create deployments when pushing functions.
414-420: LGTM! New deployment prompt for sites.Consistent with the functions prompt above and the existing pull patterns.
lib/commands/push.ts (6)
813-832: Good validation: Early path checks with clear error messages.The validation properly handles:
- Missing path configuration
- Non-existent paths
- Empty directories
The error messages include relative paths for better user experience.
1172-1191: Consistent path validation for sites.Mirrors the function path validation pattern, ensuring consistent behavior and error messaging across both resource types.
1641-1682: Well-structured code push flow with user prompts.The implementation:
- Respects
--forceflag to bypass prompts- Only prompts when there are resources to push
- Properly distinguishes between "not prompted" (
null) and "user declined" (false)
1845-1858: Correct code push decision logic.The logic properly combines the
--no-codeCLI flag with the user prompt response:
code !== falseensures the flag wasn't explicitly disabledallowCodePush === trueensures user confirmed (or force mode)
1973-1986: Consistent with site push logic.The function push follows the same pattern as site push, maintaining consistency in the CLI experience.
420-436: Number() coercion handles BigInt from config parsing.The config system uses JSONBigInt with native BigInt support, which can result in
settings.auth.securityvalues being BigInt after parsing. TheNumber()conversions ensure these values are converted to standard numbers for API compatibility. All documented constraints (duration up to 31536000 seconds, sessionsLimit 1-100) are well within JavaScript's safe integer range, so no precision loss occurs.lib/commands/config.ts (3)
130-133: Type expansion for BigInt compatibility.The schema now accepts both
numberandbigintfor these security settings, accommodating native BigInt values from the JSON parser. This aligns with the JSONBigInt configuration changes inlib/client.tsandlib/commands/schema.ts.
158-158: Site timeout supports BigInt.Consistent with the security settings changes above.
185-185: Function timeout supports BigInt.Consistent with site timeout and security settings changes.
| try { | ||
| await Promise.all( | ||
| changedAttributes.map((changed) => | ||
| this.updateAttribute( | ||
| collection["databaseId"], | ||
| collection["$id"], | ||
| changed, | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| ); | ||
| } catch (err) { | ||
| error( | ||
| `Error updating attribute for ${collection["$id"]}: ${String(err)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling swallows failures, causing silent data inconsistency.
The try/catch logs the error but does not propagate it. This means:
- If any
updateAttributecall fails, execution continues silently - The caller receives no indication that updates failed
- Subsequent deletion operations (lines 612-616) proceed without error handling, creating inconsistent behavior
Consider re-throwing after logging, or returning a result that indicates partial failure:
🐛 Proposed fix to propagate errors after logging
if (changes.length > 0) {
changedAttributes = changes.map((change) => change.attribute);
try {
await Promise.all(
changedAttributes.map((changed) =>
this.updateAttribute(
collection["databaseId"],
collection["$id"],
changed,
),
),
);
} catch (err) {
error(
`Error updating attribute for ${collection["$id"]}: ${String(err)}`,
);
+ throw err;
}
}🤖 Prompt for AI Agents
In @lib/commands/utils/attributes.ts around lines 594 - 608, The current
try/catch around Promise.all for changedAttributes swallows errors from
this.updateAttribute (called on collection["databaseId"], collection["$id"]) by
only logging them; change it to propagate the failure so callers know updates
failed—either re-throw the caught error after calling error(...) or convert the
block to return a failure result (e.g., throw new Error or return a rejected
Promise) so subsequent deletion logic does not run on partial failure; ensure
the handler for the Promise.all over changedAttributes preserves the original
error details when propagating.
This PR contains updates to the Command Line SDK for version 13.0.0-rc.5.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.