Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| replyToEmail: rest[8] as string, | ||
| replyToName: rest[9] as string, | ||
| draft: rest[10] as boolean, | ||
| html: rest[11] as boolean, | ||
| scheduledAt: rest[12] as string, |
There was a problem hiding this comment.
If a caller uses the previous positional signature for createEmail, this overload reads the existing draft boolean as replyToEmail and shifts the later options. The request can fail or create an email with the wrong settings. updateEmail has the same issue, and the Apple and Google OAuth2 update overloads shift the existing enabled argument into nativeClientIds.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/messaging.ts
Line: 249-253
Comment:
**Positional email arguments shift**
If a caller uses the previous positional signature for `createEmail`, this overload reads the existing `draft` boolean as `replyToEmail` and shifts the later options. The request can fail or create an email with the wrong settings. `updateEmail` has the same issue, and the Apple and Google OAuth2 update overloads shift the existing `enabled` argument into `nativeClientIds`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| const apiHeaders: { [header: string]: string } = { | ||
| 'X-Appwrite-Project': this.client.config.project, | ||
| accept: 'text/plain', | ||
| }; | ||
|
|
||
| return this.client.call('get', uri, apiHeaders, apiPayload); |
There was a problem hiding this comment.
When the server returns the requested DNS zone as plain text, Client.call wraps it in { message: text }. getZone therefore returns an object rather than the zone-file content callers need, while its Promise<{}> type does not describe where that content went. The test supplies JSON, so it does not cover this response path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/domains.ts
Line: 4563-4568
Comment:
**Zone content returned wrapped**
When the server returns the requested DNS zone as plain text, `Client.call` wraps it in `{ message: text }`. `getZone` therefore returns an object rather than the zone-file content callers need, while its `Promise<{}>` type does not describe where that content went. The test supplies JSON, so it does not cover this response path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| test('test method createRecordAAAA()', async () => { | ||
| const data = { | ||
| '\\$id': '5f40a6e10c65e', | ||
| '\\$createdAt': '2020-10-15T06:38:00.000+00:00', | ||
| '\\$updatedAt': '2020-10-15T06:38:00.000+00:00', | ||
| type: 'A', | ||
| name: 'mail', | ||
| value: '192.0.2.1', | ||
| ttl: 86400, | ||
| priority: 10, | ||
| lock: true, | ||
| weight: 10, | ||
| port: 443, | ||
| comment: 'Mail server record', | ||
| }; | ||
| mockedFetch.mockImplementation(() => Response.json(data)); | ||
| const response = await domains.createRecordAAAA( | ||
| '<DOMAIN_ID>', | ||
| '', | ||
| '', | ||
| 1, | ||
| ); | ||
|
|
||
| // Remove custom toString method on the objects to allow for clean data comparison. | ||
| delete response.toString; | ||
|
|
||
| expect(response).toEqual(data); |
There was a problem hiding this comment.
These new tests return a mocked response and assert that the SDK returns the same fixture, without checking the request. The createRecordAAAA test even uses an A-record response, so it would pass with the wrong route, method, or payload. The repository requires tests of observable behavior rather than mirrored source or schemas; this requirement must be satisfied before merging. The added account and messaging tests repeat the pattern.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/services/domains.test.js
Line: 381-407
Comment:
**Domain tests mirror fixtures**
These new tests return a mocked response and assert that the SDK returns the same fixture, without checking the request. The `createRecordAAAA` test even uses an A-record response, so it would pass with the wrong route, method, or payload. The repository requires tests of observable behavior rather than mirrored source or schemas; this requirement must be satisfied before merging. The added account and messaging tests repeat the pattern.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| * Domain registration price. Null when the price could not be resolved, for example for an unsupported TLD. | ||
| */ | ||
| price?: number; |
There was a problem hiding this comment.
The documentation says an unresolved price is null, but price?: number allows only a number or an absent value. The client preserves JSON null, so TypeScript callers checking only for undefined can still receive null where they expect a number. Including null in the type would prevent that mistake. renewalPrice, transferStatus, and passwordPwned have the same documented-null mismatch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/models.ts
Line: 8720-8722
Comment:
**Nullable prices typed incorrectly**
The documentation says an unresolved price is `null`, but `price?: number` allows only a number or an absent value. The client preserves JSON `null`, so TypeScript callers checking only for `undefined` can still receive `null` where they expect a number. Including `null` in the type would prevent that mistake. `renewalPrice`, `transferStatus`, and `passwordPwned` have the same documented-null mismatch.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
2 participants
This PR contains updates to the SDK for version 30.0.0.
What's Changed
account.listLogs,users.listLogs,LogandLogListremovedorganizationkey methods renamed tocreateEphemeralProjectKey,getProjectKey,listProjectKeys,updateProjectKey,deleteProjectKey, andOrganizationKeyScopesremovedsetDevKeyclient method,DevKeymodel andProject.devKeysremovedregion,reason,projectName,organizationId,organizationName,billingPlanremoved fromBlockDedicatedDatabaseSpecificationList.pricingandDedicatedDatabaseSpecificationPricingremoved; rates now live on eachDedicatedDatabaseSpecificationBillingPlanGroup.Starterreplaced byFreeandStartoauth2.introspectfor RFC 7662 token introspection with an API key, returning theOauth2Introspectionmodeldomainsservice for domains, DNS records, email presets, prices and transfersaccount.createIdTokenSessionand email verification and recovery OTP methodsmessaging.createAppwriteProviderandmessaging.updateAppwriteProviderproject.updatePasswordPwnedPolicywithPolicyPasswordPwnedmodel andUser.passwordPwnedqosandexpiryon topicsImageGravity.Auto,jasprframework, anddart-3.13andflutter-3.47runtimesX-Appwrite-Response-Format2.3.0🤖 Generated with Claude Code