Skip to content

docs(contact-center): fix unresolved SDD review comments#4789

Open
ciscoRankush wants to merge 2 commits intowebex:task-refactorfrom
ciscoRankush:fix/unresolved-review-comments
Open

docs(contact-center): fix unresolved SDD review comments#4789
ciscoRankush wants to merge 2 commits intowebex:task-refactorfrom
ciscoRankush:fix/unresolved-review-comments

Conversation

@ciscoRankush
Copy link
Copy Markdown

COMPLETES #SPARK-775697

This pull request addresses

Unresolved review comments from 5 merged SDD PRs (#4721, #4722, #4723, #4734, #4735) that were not addressed before merging into task-refactor. These include inaccurate code examples, missing explanations, incomplete template sections, and documentation that doesn't match SDK access patterns.

by making the following changes

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios were tested

  1. Verified all 10 fixes match the original reviewer comments from PRs docs(contact-center): add root AGENTS.md and ai-docs framework for SDD #4721, doc(contact-center): add ai-docs SDD templates for code generation #4722, docs(contact-center): add ai-docs SDD pattern documentation #4723, docs(contact-center): add SDD templates for new service creation #4734, docs(contact-center): add SDD templates for new method creation #4735
  2. Verified cc.queue / cc.addressBook access pattern matches actual source code in cc.ts
  3. Confirmed markdown renders correctly and all relative links are valid
  4. Documentation-only changes — no code impact, no tests affected

The GAI Coding Policy And Copyright Annotation Best Practices

  • GAI was not used (or, no additional notation is required)
  • Code was generated entirely by GAI
  • GAI was used to create a draft that was subsequently customized or modified
  • Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
  • Tool used for AI assistance (GitHub Copilot / Other - specify)
    • Github Copilot
    • Other - Claude Code (Claude Opus 4.6)
  • This PR is related to
    • Feature
    • Defect fix
    • Tech Debt
    • Automation

I certified that

  • I have read and followed contributing guidelines
  • I discussed changes with code owners prior to submitting this pull request
  • I have not skipped any automated checks
  • All existing and new tests passed
  • I have updated the documentation accordingly

Fix 10 unresolved review comments from merged PRs webex#4721, webex#4722, webex#4723, webex#4734, webex#4735:

1. AGENTS.md: Distinguish UI/UX fix vs code-level defect in task classification
2. bug-fix.md: Explain what "regression test" means
3. feature-enhancement.md: Add descriptive context for Step 1
4. feature-enhancement.md: Mark Plugin Layer (cc.ts) step as conditional
5. create-architecture-md.md: Note file structure belongs in AGENTS.md
6. new-service/01-pre-questions.md: Club Dependencies and Exposure sections
7. new-method/01-requirements.md: Add callout to create type definitions
8. new-method/02-implementation.md: Fix cc.services.queue → cc.queue
9. new-method/03-tests.md: Add exact match assertion guidelines
10. new-service/02-code-generation.md: Complete sub-module example

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ciscoRankush ciscoRankush requested a review from a team as a code owner March 18, 2026 08:27
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de8bebf8d8

ℹ️ 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".

const queues = await cc.services.queue.getQueues();
// Consumer code calling service directly via public accessor
const queues = await cc.queue.getQueues();
const addressBooks = await cc.addressBook.getAddressBooks();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use the real AddressBook API in the direct-access example

AddressBook does not expose getAddressBooks() anywhere in this package; its public fetch method is getEntries() in src/services/AddressBook.ts. Anyone following this updated example for a direct-service method will copy a call that does not exist and end up with broken sample code/tests, which is exactly the kind of SDK-surface mismatch this PR is trying to remove.

Useful? React with 👍 / 👎.

**Method**: `getBuddyAgents(data: BuddyAgents): Promise<BuddyAgentsResponse>`
**Target file**: cc.ts
**Purpose**: Get list of available agents for consult/transfer
**Types to create**: Define `BuddyAgents` (request params) and `BuddyAgentsResponse` (response) in the appropriate types file (`src/types.ts` for public types, or `src/services/[service]/types.ts` for internal types). Export public types from `src/types.ts`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include the package entrypoint in the new public type guidance

This new instruction makes it sound like adding a public type to src/types.ts is sufficient, but @webex/contact-center exposes consumer-facing types through explicit re-exports in src/index.ts (for example, BuddyAgents and BuddyAgentsResponse). For any new public method, following this template as written will leave the new types unavailable from the package root even though they were defined locally.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#4722 (comment)
This comment isn't addressed yet

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6414ba6 — Removed duplicate questions 5 and 6. They were already covered as conditional fallbacks under question 3 ("If the developer cannot reproduce it, ask: error messages, trackingId, recent changes"). No information is lost.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please check this comment: #4722 (comment)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6414ba6 — Added "(All Mandatory)" to the heading and a note: "All 6 questions must be answered before proceeding. If the developer is unsure about a question, help them reason through it using the decision signals below."

```

### Plugin Layer (cc.ts)
### Plugin Layer (cc.ts) — if the feature is exposed as a public API
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this comment: #4722 (comment), what I meant was that everywhere we are talking about public method being exposed from cc object but we also expose public method from task object and in future we might have other objects introduced as well so documentation should clearly state that

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6414ba6 — Renamed section from "Plugin Layer (cc.ts)" to "Public API Layer — if the feature is exposed to consumers". The note now uses generic language and lists the current public objects as examples:

  • cc (cc.ts) — SDK-level operations (e.g., stationLogin, setAgentState)
  • task (Task.ts) — per-interaction operations (e.g., hold, transfer, end)

Also notes that new public objects may be introduced in the future. Same approach applied across 02-implementation.md and 00-master.md.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment is still open: #4735 (comment)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6414ba6 — Rewrote Method Invocation Patterns section with generic language ("the owning object") and added a Task.ts example (task.hold()) alongside the existing cc.ts example. Also fixed getAddressBooks() to getEntries() to match the actual AddressBook.ts API.


If the method needs to emit events, understand the two-step flow used in the SDK:

### How it works: WebSocket trigger → EventEmitter emit
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#4735 (comment) : This comment is also open

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6414ba6 — Changed "agent service methods" to "all AQM-integrated services — agent, task, etc., except config service GET methods". Verified that task/contact.ts uses aqm.req() extensively (accept, hold, consult, transfer, etc.).

Copy link
Copy Markdown
Contributor

@Shreyas281299 Shreyas281299 left a comment

Choose a reason for hiding this comment

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

Looks good to me, Can we addressed the 4 open comments from Priya

- bug-fix.md: remove duplicate questions 5-6 (already covered as
  fallbacks under question 3)
- feature-enhancement.md: mark all triage questions as mandatory,
  rename Plugin Layer to generic "Public API Layer" with cc and task
  as examples of owning objects
- new-method/00-master.md: use generic phrasing for public objects
- new-method/02-implementation.md: add Task.ts example alongside
  cc.ts, fix getAddressBooks() to getEntries(), broaden AQM req
  scope to include task service

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6414ba6b04

ℹ️ 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".

Comment on lines +273 to +276
> **Determine the owning object.** Public methods are added to the object that owns the feature's scope. Currently the SDK exposes:
> - **`cc`** (in `cc.ts`) — SDK-level operations (e.g., `cc.stationLogin()`, `cc.setAgentState()`)
> - **`task`** (in `Task.ts`) — per-interaction operations (e.g., `task.hold()`, `task.transfer()`, `task.end()`)
>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Describe all currently exposed public API objects

This new guidance says the SDK currently exposes only cc and task, but consumers also use public service objects like cc.addressBook and cc.queue (see src/cc.ts public properties and examples). If template users follow this as written, feature work for those existing public surfaces can be implemented in the wrong layer or miss required surface updates, creating API inconsistencies in generated changes.

Useful? React with 👍 / 👎.

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