Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
stampercasey
left a comment
There was a problem hiding this comment.
Review from Claude Code — see inline comments for individual findings. One blocker (regen job); the verb implementation is otherwise sound.
Regen job has not run — models/refer-complete-callback.ts is absent and bandwidth.yml was not updated. The PR description lists this as a follow-up pending api-specs#2142, which has now merged (June 12). The regen needs to run and ReferCompleteCallback + the four scenario tests need to land before this is ready to merge.
… in Refer ReferCompleteCallback will land later via a separate api-specs-driven PR once VAPI-3440 merges. Refer now takes the existing Transfer-flavored SipUri instead of a duplicate ReferSipUri type, matching the reviewed csharp-sdk reference implementation (no runtime validation, no builder bloat).
models/refer-call-status.ts had no consumer once ReferSipUri/ReferCompleteCallback were removed from this PR's scope. bandwidth.yml still carried the full referCompleteCallback/referCallStatus/referSipResponseCode/notifySipResponseCode schemas and a referComplete addition to the shared eventType enum description. Also cleaned up the corresponding .openapi-generator/FILES entries.
…return type tsc (with declaration:true) failed on common.ts:110 - 'The inferred type of createRequestFunction references an inaccessible unique symbol type' - because axios 1.19's request<T,R>() return type involves an internal branded type not portable to a .d.ts. This broke npm run build (invoked via yarn's prepare hook) on every OS/Node matrix job in CI, on main as well as this PR. Annotating the returned function's return type as Promise<R> sidesteps emitting the unexported internal type.
| return <T = unknown, R = AxiosResponse<T>>(axios: AxiosInstance = globalAxios, basePath: string = BASE_PATH) => { | ||
| return function <T = unknown, R = AxiosResponse<T>>(axios: AxiosInstance = globalAxios, basePath: string = BASE_PATH): Promise<R> { | ||
| const axiosRequestArgs = {...axiosArgs.options, url: (axios.defaults.baseURL ? '' : configuration?.basePath ?? basePath) + axiosArgs.url}; | ||
| return axios.request<T, R>(axiosRequestArgs); | ||
| return axios.request<T, R>(axiosRequestArgs) as Promise<R>; |
There was a problem hiding this comment.
Fix for CI error unrelated to PR change. Actions aren't required so can remove this if wanted
There was a problem hiding this comment.
We need to remove this, I have a ticket for fixing the CI issue, but this is an autogenerated file, so we can't update it directly like this
| * @export | ||
| * @class Refer | ||
| * @extends {NestableVerb} | ||
| * Represents a Refer BXML verb. |
There was a problem hiding this comment.
| * Represents a Refer BXML verb. | |
| * Represents a Refer verb. |
| * @param {SipUri} sipUri The SipUri child element (required - spec mandates exactly one) | ||
| * @param {ReferAttributes} attributes The attributes to add to the element |
There was a problem hiding this comment.
| * @param {SipUri} sipUri The SipUri child element (required - spec mandates exactly one) | |
| * @param {ReferAttributes} attributes The attributes to add to the element | |
| * @param {ReferAttributes} attributes The attributes to add to the element | |
| * @param {SipUri} sipUri The SipUri to refer to |
| * @param {SipUri} sipUri The SipUri child element (required - spec mandates exactly one) | ||
| * @param {ReferAttributes} attributes The attributes to add to the element | ||
| */ | ||
| constructor(sipUri: SipUri, attributes?: ReferAttributes) { |
There was a problem hiding this comment.
| constructor(sipUri: SipUri, attributes?: ReferAttributes) { | |
| constructor(attributes?: ReferAttributes, sipUri?: SipUri) { |
the order of these arguments should match the NestableVerb constructor and the other nestable verbs for consistency
| * @param {ReferAttributes} attributes The attributes to add to the element | ||
| */ | ||
| constructor(sipUri: SipUri, attributes?: ReferAttributes) { | ||
| super('Refer', undefined, attributes, [sipUri]); |
There was a problem hiding this comment.
| super('Refer', undefined, attributes, [sipUri]); | |
| super('Refer', undefined, attributes, sipUri); |
we have logic in the parent constructor to convert to an array so that isn't needed here
| * @param {SipUri} sipUri The SipUri to refer to | ||
| */ | ||
| setSipUri(sipUri: SipUri): void { | ||
| // Replaces the single required SipUri child - <Refer> allows exactly one. |
There was a problem hiding this comment.
| // Replaces the single required SipUri child - <Refer> allows exactly one. |
| import { SipUri } from '../../../../../models/bxml/verbs/SipUri'; | ||
|
|
||
| describe('Refer', () => { | ||
| test('should generate Refer XML with SipUri and all attributes', () => { |
There was a problem hiding this comment.
these tests dont match any of the other nestable verb tests, we should make our tests as consistent as possible, look at Gather, Transfer, Connect, or StartStream for how this should look.
| return <T = unknown, R = AxiosResponse<T>>(axios: AxiosInstance = globalAxios, basePath: string = BASE_PATH) => { | ||
| return function <T = unknown, R = AxiosResponse<T>>(axios: AxiosInstance = globalAxios, basePath: string = BASE_PATH): Promise<R> { | ||
| const axiosRequestArgs = {...axiosArgs.options, url: (axios.defaults.baseURL ? '' : configuration?.basePath ?? basePath) + axiosArgs.url}; | ||
| return axios.request<T, R>(axiosRequestArgs); | ||
| return axios.request<T, R>(axiosRequestArgs) as Promise<R>; |
There was a problem hiding this comment.
We need to remove this, I have a ticket for fixing the CI issue, but this is an autogenerated file, so we can't update it directly like this
…mon.ts Flip Refer's constructor to (attributes, sipUri) matching NestableVerb and every other verb's convention; drop the unnecessary array-wrap since the parent constructor already normalizes a single Verb to an array. Simplify the class doc comment to match Transfer's. Rewrite Refer.test.ts to follow the Gather/Transfer/Connect/StartStream test structure. Revert the common.ts patch from the CI-fix commit: it's an autogenerated file and shouldn't be hand-edited - ckoegel has a separate ticket tracking the real fix for the TS2527 build error.
Adds the
<Refer>BXML verb.models/bxml/verbs/Refer.ts: newReferverb (referCompleteUrl,referCompleteMethod,tagattributes, one requiredSipUrichild).SipUritype for the child element instead of a separateReferSipUri- no runtime validation restricting whichSipUriattributes are legal per verb (matches csharp-sdk#201, which dropped this as over-engineering).ReferCompleteCallbackmodel is out of scope here - lands separately once VAPI-3440 merges. Deleted the orphaned generatedmodels/refer-call-status.ts/docs/ReferCallStatus.mdand the correspondingbandwidth.ymlschemas, which had no remaining consumer once that callback work was descoped.common.ts: fixed a pre-existing TS2527 build error (unrelated to Refer, also present onmain) that was failingnpm run buildon every CI matrix job - annotatedcreateRequestFunction's return type to avoid emitting an axios-internal type that isn't portable to a.d.ts.tests/unit/models/bxml/verbs/Refer.test.ts.Tests:
npm run buildandnpx jest- full suite passes (257/257 test files).VAPI-3439