docs(calling): Spark 787601 templates#4784
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 976c12c5b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/calling/AGENTS.md
Outdated
|
|
||
| | Module | Scope / Keywords | Location | ai-docs | | ||
| |---|---|---|---| | ||
| | **CallingClient** | line, register, call, dial, answer, hold, transfer, mute, media | `src/CallingClient/` | [`src/CallingClient/ai-docs/`](src/CallingClient/ai-docs/AGENTS.md) | |
There was a problem hiding this comment.
The routing table points CallingClient tasks to src/CallingClient/ai-docs/AGENTS.md, but this commit creates the module docs under src/CallingClient/calling/ai-docs/. Following the documented workflow sends agents to a non-existent path, so CallingClient requests will miss their module-specific AGENTS/architecture context.
Useful? React with 👍 / 👎.
packages/calling/AGENTS.md
Outdated
| 1. **Classify the task** - Determine if it's a new module, new method, bug fix, feature enhancement, or architecture question. | ||
| 2. **Load context** - Use the [Module Routing Table](#module-routing-table) to find and read the target module's `ai-docs/AGENTS.md` and `ARCHITECTURE.md`. | ||
| 3. **Load rules** - Read [`ai-docs/RULES.md`](ai-docs/RULES.md) for coding standards. | ||
| 4. **Load patterns** - Read relevant patterns from [`ai-docs/patterns/`](ai-docs/patterns/). |
There was a problem hiding this comment.
The quick-start checklist requires loading ai-docs/patterns/, but this commit does not add a packages/calling/ai-docs/patterns/ directory or the referenced pattern docs. As written, agents that follow the workflow hit a missing prerequisite and cannot complete the documented process end-to-end.
Useful? React with 👍 / 👎.
akulakum
left a comment
akulakum
left a comment
There was a problem hiding this comment.
Review — AI-docs Templates for Calling Package
Overall this is a solid set of templates with excellent domain knowledge of the calling SDK internals (Logger, MetricManager, Eventing, error hierarchy, state machines, multi-backend patterns). The "STOP — Ask These Questions First" gates and the common bug patterns table are particularly well done.
Please address the inline comments below before merge. Most are quick fixes around path consistency, script names, and template content accuracy.
| @@ -0,0 +1,113 @@ | |||
| # New Module Workflow - Master Orchestrator | |||
There was a problem hiding this comment.
[Medium] There's no top-level entry point (ai-docs/README.md or ai-docs/templates/README.md) that explains:
- What these templates are for
- How to choose between
new-module,new-method, andexisting-module - Prerequisites and dependencies on other docs (AGENTS.md, RULES.md, patterns/)
Without a navigation doc, someone landing in ai-docs/templates/ for the first time won't know where to start. Consider adding a lightweight README as the entry point.
There was a problem hiding this comment.
We have another PR where Root AGENTS.md and README.md are being reviewed. And there we will have information on how these templates are chosen
|
|
||
| Use the [Module Routing Table](../../../AGENTS.md#module-routing-table) to identify the affected module, then: | ||
|
|
||
| 1. Read the module's `ai-docs/AGENTS.md` (if exists) — understand the public API and expected behavior |
There was a problem hiding this comment.
[Medium] This references ../../../AGENTS.md#module-routing-table, which resolves to packages/calling/AGENTS.md. Since that file is being introduced in a separate PR, this link will be broken until that PR merges.
Consider either:
- Adding a note: "Requires the Module Routing Table from AGENTS.md (see PR #XXXX)"
- Or gating this PR to merge after the AGENTS.md PR
Also, the path convention is inconsistent across templates — 05-validation.md references src/Agents.md instead. Please standardize.
| **Every enhancement MUST update tests:** | ||
|
|
||
| 1. **Update existing tests** if method signature or behavior changed: | ||
| ```typescript |
There was a problem hiding this comment.
[Medium] These pattern file references don't exist yet:
../../patterns/typescript-patterns.md../../patterns/event-driven-patterns.md
Same issue as the AGENTS.md dependency — if these are coming in a separate PR, add a note indicating that. Otherwise agents following this template will fail at Step 1 of the implementation workflow.
There was a problem hiding this comment.
Adding a note for something that will be merged right away doesn't make sense so it's ok to keep them this way and based on whichever PR first merges, paths can be updated
| npx jest src/ModuleName/ModuleName.test.ts --verbose | ||
| ``` | ||
|
|
||
| ### Verify All Pass |
There was a problem hiding this comment.
[Medium] Script name mismatch. The @webex/calling package's package.json defines:
test:unit(nottest)test:style(notlint)
These commands should be:
# Run ESLint:
yarn test:style
# Or from repo root:
yarn workspace @webex/calling test:styleAnd the test section below should use yarn test:unit. This mismatch appears in multiple templates — please do a pass across all files to align with actual package scripts.
|
|
||
| --- | ||
|
|
||
| ## Add Metric Events |
There was a problem hiding this comment.
[Medium] Using submitVoicemailMetric as the example for a generic module template is misleading. A new module named e.g. "Recordings" or "Presence" should not be calling a voicemail-specific metric method.
Suggestion: Replace with neutral guidance like:
"Use the appropriate domain-specific metric submit method (e.g.,
submitCallMetric,submitRegistrationMetric). If no existing method matches, add a new one toIMetricManagerinsrc/Metrics/types.ts."
Or rename the example to clearly show it as a placeholder pattern.
| const successHandler = jest.fn(); | ||
| call.on(CALL_EVENT_KEYS.PARKED, successHandler); | ||
|
|
||
| // Act — trigger the method |
There was a problem hiding this comment.
[Low] midCallEvent as any contradicts the validation checklist in 04-validation.md which states: "No any types (use proper typing or unknown + type assertion)".
For consistency with the project's own standards, this should be:
call['handleMidCallEvent'](midCallEvent as unknown as CallEvent);Or define a proper mock type.
| expect(client).toBeTruthy(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Low] The angle-bracket type assertion <unknown> on this line appears to be getting swallowed by markdown rendering, making the code snippet look broken when viewed on GitHub.
Use as unknown syntax instead to avoid markdown/HTML conflicts:
const mockPayload = MOCK_SUCCESS_RESPONSE_BODY as unknown as WebexRequestPayload;This also aligns with the TypeScript as assertion style used elsewhere in the templates.
| ``` | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
[Low] The validation checklist and build/verify commands (yarn test:unit, yarn test:style, yarn build) are repeated nearly verbatim across bug-fix.md, feature-enhancement.md, new-method/04-validation.md, and new-module/05-validation.md.
Consider extracting these into a shared ai-docs/templates/common/validation-checklist.md and referencing it from each template. This reduces maintenance burden — when script names change (as noted in another comment), you'd only need to update one file.
| @@ -0,0 +1,565 @@ | |||
| # Code Generation | |||
There was a problem hiding this comment.
[Low] At 565 lines, this is the longest file in the PR. While thoroughness is appreciated, the full class templates (base template, no-events customization, multi-backend customization, singleton, backend connector) make this quite dense.
Consider:
- Keeping the main flow concise with the base template
- Moving customization variants (multi-backend, singleton, no-events) to an appendix section or separate file
- Referencing actual source files (e.g., "See
src/Voicemail/Voicemail.tsfor the multi-backend pattern") rather than reproducing full class bodies
This would make the template easier to follow while still providing the detail when needed.
|
|
||
| For significant modules, create an `ai-docs/` folder within the module directory: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
[Medium] This references src/Agents.md but other templates reference packages/calling/AGENTS.md (via relative paths like ../../../AGENTS.md). These are different paths.
Please standardize to one consistent reference. Since the AGENTS.md is expected at the package root (packages/calling/AGENTS.md), this should probably be a relative path from this file's location.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec3797d8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - [ ] You have read `ai-docs/RULES.md` for coding standards | ||
| - [ ] You have read `ai-docs/patterns/typescript-patterns.md` for type and interface conventions | ||
| - [ ] You have read `ai-docs/patterns/event-driven-patterns.md` if the module emits events | ||
| - [ ] You have read `ai-docs/patterns/error-handling-patterns.md` for error class usage, error mapping, and error event emission conventions | ||
| - [ ] You have read `ai-docs/patterns/testing-patterns.md` for Jest conventions |
There was a problem hiding this comment.
This template makes ai-docs/RULES.md and several ai-docs/patterns/*.md files hard prerequisites, but those files do not exist anywhere under packages/calling/ai-docs in this commit, so the workflow blocks before implementation starts. The same missing pattern docs are also referenced by other newly added templates, so contributors and agents cannot complete the documented process end-to-end.
Useful? React with 👍 / 👎.
|
|
||
| ### Step 1: Load Module Context | ||
|
|
||
| Use the [Module Routing Table](../../../AGENTS.md#module-routing-table) to identify the affected module, then: |
There was a problem hiding this comment.
The investigation flow sends readers to ../../../AGENTS.md#module-routing-table, which resolves to packages/calling/AGENTS.md, but that file is not present in this commit. Because this is the first step for module identification, bug triage cannot follow the documented path and agents lose the routing context the template depends on.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636c972578
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 2. Read the target module's `ai-docs/ARCHITECTURE.md` (if exists) — understand current flows | ||
| 3. Read the specific file(s) being modified — understand current implementation | ||
| 4. Read the test file(s) — understand existing test coverage | ||
| 5. Load patterns: [`../../patterns/typescript-patterns.md`](../../patterns/typescript-patterns.md), [`../../patterns/event-driven-patterns.md`](../../patterns/event-driven-patterns.md), [`../../patterns/error-handling-patterns.md`](../../patterns/error-handling-patterns.md) |
There was a problem hiding this comment.
This workflow step requires loading ../../patterns/event-driven-patterns.md, but that file is not present in packages/calling/ai-docs/patterns (the available doc is event-patterns.md). Because this is in the mandatory context-loading step, agents following the template will hit a dead link and skip or miss event-pattern guidance before implementation.
Useful? React with 👍 / 👎.
| | Coding Standards | [`../../RULES.md`](../../RULES.md) | | ||
| | TypeScript Patterns | [`../../patterns/typescript-patterns.md`](../../patterns/typescript-patterns.md) | | ||
| | Testing Patterns | [`../../patterns/testing-patterns.md`](../../patterns/testing-patterns.md) | | ||
| | Event Patterns | [`../../patterns/event-driven-patterns.md`](../../patterns/event-driven-patterns.md) | |
There was a problem hiding this comment.
The reference table links Event Patterns to ../../patterns/event-driven-patterns.md, but that file does not exist in this package, so the documented prerequisite cannot be completed as written. This leaves method-generation tasks without the intended event-pattern source and makes the workflow inconsistent with the actual docs tree.
Useful? React with 👍 / 👎.
mkesavan13
left a comment
mkesavan13
left a comment
There was a problem hiding this comment.
- This review is for
existing-moduleandnew-method - While this is being fixed, will review
new-module
Apart from this, here are few Non-blocking, nitpick, do-later type, "junk code" to remove from the Codebase:
SUPPLEMENTARY_SERVICES.DIVERTandSUPPLEMENTARY_SERVICES.PARKDivertContext&ParkContextWEBSOCKET_SCOPEandWEBSOCKET_KEYSCallProgressMessage
|
|
||
| ### 6. Metrics | ||
|
|
||
| 10. **"Do existing metrics need to change, or do new metrics need to be added?"** |
There was a problem hiding this comment.
Do we need to ask for more info around this if it is a yes?
| - **Creating an entirely new module** (new class/folder) -- Use [`../new-module/00-master.md`](../new-module/00-master.md) | ||
| - **Enhancing or modifying an existing method** -- Use [`../existing-module/feature-enhancement.md`](../existing-module/feature-enhancement.md) | ||
| - **Fixing a bug** -- Use [`../existing-module/bug-fix.md`](../existing-module/bug-fix.md) |
There was a problem hiding this comment.
| - **Creating an entirely new module** (new class/folder) -- Use [`../new-module/00-master.md`](../new-module/00-master.md) | |
| - **Enhancing or modifying an existing method** -- Use [`../existing-module/feature-enhancement.md`](../existing-module/feature-enhancement.md) | |
| - **Fixing a bug** -- Use [`../existing-module/bug-fix.md`](../existing-module/bug-fix.md) | |
| - **Creating an entirely new module** (new class/folder) -- Use [`../new-module/00-master.md`](../new-module/00-master.md) instead. | |
| - **Enhancing or modifying an existing method** -- Use [`../existing-module/feature-enhancement.md`](../existing-module/feature-enhancement.md) instead. | |
| - **Fixing a bug** -- Use [`../existing-module/bug-fix.md`](../existing-module/bug-fix.md) instead. |
There was a problem hiding this comment.
Why is this change required ? What difference does it make ?
|
|
||
| Write the method following the calling SDK's established patterns: | ||
| - Logger with `{ file, method }` context | ||
| - MetricManager via `getMetricManager()` for success/failure metrics |
There was a problem hiding this comment.
Is it only success/failure we measure or do we also have progress metrics? Like when the calls state changes from Alerting to Connected
| - Logger with `{ file, method }` context | ||
| - MetricManager via `getMetricManager()` for success/failure metrics | ||
| - Error hierarchy (`ExtendedError` -> `CallError` / `LineError` / `CallingClientError`) | ||
| - `Eventing<T>` base class for event emission |
There was a problem hiding this comment.
What is this particular Eventing about in a method implementation? May be some more expansion on the description would help?
| * @param param1 - <Description of param1.> | ||
| * @param param2 - <Description of param2.> | ||
| * @returns <Description of return value.> | ||
| */ | ||
| public async methodName(param1: ParamType, param2?: OptionalType): Promise<ReturnType> { |
There was a problem hiding this comment.
Should we mention ... param{n} here?
| * @param param1 - <Description of param1.> | |
| * @param param2 - <Description of param2.> | |
| * @returns <Description of return value.> | |
| */ | |
| public async methodName(param1: ParamType, param2?: OptionalType): Promise<ReturnType> { | |
| * @param param1 - <Description of param1.> | |
| * @param param2 - <Description of param2.> | |
| * . | |
| * . | |
| * . | |
| * @param param{n} - <Description of param{n}.> | |
| * @returns <Description of return value.> | |
| */ | |
| public async methodName(param1: ParamType1, param2?: ParamType2, ..., param{n}?: OptionalType): Promise<ReturnType> { |
| ### Run all tests for the calling package | ||
|
|
||
| ```bash | ||
| cd packages/calling | ||
| yarn test:unit | ||
| ``` | ||
|
|
||
| ### Run a specific test file | ||
|
|
||
| ```bash | ||
| cd packages/calling | ||
| yarn jest src/CallingClient/calling/call.test.ts | ||
| ``` | ||
|
|
||
| ### Run tests matching a pattern | ||
|
|
||
| ```bash | ||
| cd packages/calling | ||
| yarn jest --testPathPattern="call.test" --verbose | ||
| ``` | ||
|
|
||
| ### Run a specific describe block | ||
|
|
||
| ```bash | ||
| cd packages/calling | ||
| yarn jest src/CallingClient/calling/call.test.ts -t "parkCall tests" | ||
| ``` | ||
|
|
||
| ### Run lint check | ||
|
|
||
| ```bash | ||
| cd packages/calling | ||
| yarn test:style | ||
| ``` |
There was a problem hiding this comment.
Have we tried to run these commands locally to know if they're correct?
There was a problem hiding this comment.
Yes I have checked and they work
| ### Error Logging Level Guide | ||
|
|
||
| Use the correct log level for each situation: | ||
|
|
||
| | Level | Usage | Example | | ||
| |---|---|---| | ||
| | `log.error` | Caught exceptions, API failures, unrecoverable errors | `log.error('Failed to park call: ...', logContext)` | | ||
| | `log.warn` | Recoverable issues, timeouts, degraded behavior | `log.warn('Park response timed out', logContext)` | | ||
| | `log.info` | Method entry/exit, significant state changes | `log.info('${METHOD_START_MESSAGE} with: ...', logContext)` | | ||
| | `log.log` | Diagnostic details (response codes, intermediate states) | `log.log('Response code: ${response.statusCode}', logContext)` | | ||
| | `log.trace` | Verbose debugging (payloads, full objects) | `log.trace('Full response: ${JSON.stringify(body)}', logContext)` | |
There was a problem hiding this comment.
I believe this was captured in patterns itself. Do we need to repeat it?
There was a problem hiding this comment.
Good to add here because these are like final steps that during actual code generation would be followed
| // Success metric | ||
| this.metricManager.submitCallMetric( | ||
| METRIC_EVENT.CALL, | ||
| 'parkCall', // action string |
There was a problem hiding this comment.
This can be a string constant or a variable but never a hardcoded string. If that's true, we need to correct the doc.
Curious if this was also caught in the patterns already but we are repeating here?
| **Incorrect** -- Incomplete error handling: | ||
|
|
||
| ```typescript | ||
| // WRONG: Swallowing the error | ||
| catch (e) { | ||
| console.error(e); | ||
| } | ||
|
|
||
| // WRONG: Throwing raw error instead of typed CallError | ||
| catch (e) { | ||
| throw e; | ||
| } | ||
|
|
||
| // WRONG: Missing metric submission | ||
| catch (e) { | ||
| const callError = createCallError(...); | ||
| this.emit(CALL_EVENT_KEYS.PARK_ERROR, callError); | ||
| // Forgot: this.submitCallErrorMetric(callError); | ||
| } | ||
|
|
||
| // WRONG: Missing event emission | ||
| catch (e) { | ||
| const callError = createCallError(...); | ||
| this.submitCallErrorMetric(callError); | ||
| // Forgot: this.emit(CALL_EVENT_KEYS.PARK_ERROR, callError); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Do we need this here as we have mentioned the steps anyway in the correct section?
There was a problem hiding this comment.
It is good to give an idea what is considered incorrect even though we mention the correct section, it is too vast for LLM to understand what could be wrong
| this.sendCallStateMachineEvt({type: 'E_CALL_ESTABLISHED', data: errData}); | ||
| }, | ||
| ERROR_LAYER.CALL_CONTROL, | ||
| /* istanbul ignore next */ (interval: number) => undefined, |
There was a problem hiding this comment.
Why do we see an istanbul ignore next here?
There was a problem hiding this comment.
Because in actual code example, its like that
| Service: janus | ||
| Request: query params only (date: string, limit: number) | ||
| Response: { statusCode: number; data: { recordings: Recording[] }; message: string } | ||
| Errors: 400, 401, 404, 500 |
There was a problem hiding this comment.
Example doesn't talk about meanings.
| > | ||
| > | Event Key | Direction | Payload Type | Description | | ||
| > |-----------|-----------|-------------|-------------| | ||
| > | `moduleName:event_name` | Outbound (to consumer) | `EventPayloadType` | When this fires | |
There was a problem hiding this comment.
| > | `moduleName:event_name` | Outbound (to consumer) | `EventPayloadType` | When this fires | | |
| > | `moduleName:event_name` | Outbound (to consumer) | `EventPayloadType` | When is this fired? | |
| Service: {mobius|janus|hydra|...} | ||
| Request: {type shape} | ||
| Response: {type shape} | ||
| Errors: {status codes} |
There was a problem hiding this comment.
| Errors: {status codes} | |
| Errors: {status code1} - {reason1}, {status code2} - {reason2}, ..., {status codeN} - {reasonN}, |
| ## File Structure by Placement Type | ||
|
|
||
| ### Top-Level Module (`src/ModuleName/`) | ||
|
|
||
| ``` | ||
| src/ModuleName/ | ||
| ModuleName.ts # Main service class + factory function | ||
| types.ts # IModuleName interface, response types, LoggerInterface | ||
| constants.ts # MODULE_NAME_FILE, METHODS, module-specific constants | ||
| ModuleName.test.ts # Co-located unit tests | ||
| moduleNameFixtures.ts # Test fixture data | ||
| ``` | ||
|
|
||
| ### Sub-Module of CallingClient (`src/CallingClient/moduleName/`) | ||
|
|
||
| ``` | ||
| src/CallingClient/moduleName/ | ||
| index.ts # Main class | ||
| types.ts # Interface and types | ||
| constants.ts # Constants | ||
| moduleName.test.ts # Co-located tests | ||
| moduleNameFixtures.ts # Test fixtures | ||
| ``` | ||
|
|
||
| ### Multi-Backend Module (`src/ModuleName/`) | ||
|
|
||
| ``` | ||
| src/ModuleName/ | ||
| ModuleName.ts # Facade class (delegates to connectors) | ||
| types.ts # IModuleName, IWxCallBackendConnector, etc. | ||
| constants.ts # Shared constants | ||
| WxCallBackendConnector.ts # WXC backend implementation | ||
| BroadworksBackendConnector.ts # Broadworks backend implementation | ||
| UcmBackendConnector.ts # UCM backend implementation | ||
| ModuleName.test.ts # Facade tests | ||
| WxCallBackendConnector.test.ts # WXC connector tests | ||
| BroadworksBackendConnector.test.ts # Broadworks connector tests | ||
| UcmBackendConnector.test.ts # UCM connector tests | ||
| moduleNameFixture.ts # Shared test fixtures | ||
| ``` |
There was a problem hiding this comment.
Seems to be repeating content from the pre-questions template itself
|
|
||
| Only create new types that do not already exist in these shared locations. | ||
|
|
||
| ### Types Template |
There was a problem hiding this comment.
Shouldn't patterns take care of this?
There was a problem hiding this comment.
Some of the instructions are repeated here (in addition to pattern docs) because this is the file-generation step where duplication mistakes usually occur
| 2. **Check `src/Events/types.ts`** -- Do not re-declare event enums that already exist | ||
| 3. **Check `src/Metrics/types.ts`** -- If adding metric actions, extend the existing enums there | ||
|
|
||
| ### Constants Template |
There was a problem hiding this comment.
Once again, shouldn't patterns cover this?
There was a problem hiding this comment.
Some of the instructions are repeated here (in addition to pattern docs) because this is the file-generation step where duplication mistakes usually occur
|
|
||
| ### A1. Export from `src/api.ts` | ||
|
|
||
| Add the module's exports to `src/api.ts` following the existing grouped pattern: |
There was a problem hiding this comment.
We have a pre-question regarding this. Should we talk about that?
|
|
||
| export enum MOBIUS_EVENT_KEYS { | ||
| // ... existing keys ... | ||
| MODULE_NAME_EVENT = 'event:janus.module_name_event', |
There was a problem hiding this comment.
Should we have a Mobius example as well here?
|
|
||
| ```bash | ||
| # Run all tests: | ||
| yarn test:unit | ||
|
|
||
| # Run only the new module's tests: | ||
| npx jest src/ModuleName/ | ||
|
|
||
| # Run with coverage: | ||
| npx jest src/ModuleName/ --coverage | ||
|
|
||
| # Run a single test file: | ||
| npx jest src/ModuleName/ModuleName.test.ts --verbose | ||
| ``` |
There was a problem hiding this comment.
Have we tried to run these to know if these are correct?
There was a problem hiding this comment.
Yes I have run it
…js-sdk into SPARK-787601-Templates
These are not junk codes, they were added for features which were deprioritized I believe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab903ca4a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Add the new module to the Module Index table in root `AGENTS.md` (`packages/calling/AGENTS.md`): | ||
|
|
||
| ```markdown | ||
| | **ModuleName** | `ModuleName` | `createModuleNameClient()` | `IModuleName` | {one-sentence description} | |
There was a problem hiding this comment.
The row template here uses a 5-column “Module Index” format, but packages/calling/AGENTS.md’s module routing table is a 3-column table (Module, Scope Keywords, Source of Truth). If contributors follow this snippet, they will insert malformed rows that break the routing table structure and omit the scope/source metadata agents rely on for module selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ada1a48aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ### A1. Export from `src/api.ts` | ||
|
|
||
| Add the module's exports to `src/api.ts` following the existing grouped pattern: |
There was a problem hiding this comment.
Path A marks integration complete after exporting only from src/api.ts, but @webex/calling is published from dist/module/index.js (generated from src/index.ts). If contributors follow this workflow, a new top-level module can pass the checklist yet remain unavailable to consumers importing from @webex/calling, which breaks the stated goal of making the module accessible to consumers.
Useful? React with 👍 / 👎.
3 participants
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-787601
This pull request addresses
Adding templates to identify if new module has to be added or, feature enhancement is in existing module, adding new method or bug fixing in existing code.
by making the following changes
Added templates at root of calling package to to triaging to identify the scope of the implementation and get the required details from the user.
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.