docs(contact-center): enhance core AGENTS.md with Utils.ts coverage#4805
docs(contact-center): enhance core AGENTS.md with Utils.ts coverage#4805Shreyas281299 wants to merge 4 commits intowebex:task-refactorfrom
Conversation
….ts coverage Add detailed documentation for all Utils.ts exported functions that were previously undocumented: validation utilities (isValidDialNumber, getStationLoginErrorData), consult/transfer destination resolution (getConsultedAgentId, getDestAgentIdForCBT, calculateDestAgentId, calculateDestType, buildConsultConferenceParamData, deriveConsultTransferDestinationType), and a complete consumer map showing which modules import each utility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c15f9a5df
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | `PageCache` | [`PageCache.ts`](./PageCache.ts) | Generic in-memory cache utility for paginated API responses with TTL expiry and helper methods for key generation and cache eligibility checks. | | ||
| | `Pagination Types` | [`PageCache.ts`](./PageCache.ts) | `PaginationMeta`, `PaginatedResponse<T>`, `BaseSearchParams`, and `PageCacheEntry<T>` shared across data services. | | ||
| | `Pagination Defaults` | [`PageCache.ts`](./PageCache.ts) | `PAGINATION_DEFAULTS` (`PAGE`, `PAGE_SIZE`) used by services for consistent request defaults. | | ||
| | `Specs Workflow` | [`agents.mmd`](./agents.mmd) | Mermaid flow for specs-driven utility changes, acceptance criteria, and drift checks. | |
There was a problem hiding this comment.
Remove missing workflow file reference
The new guide tells contributors to use ./agents.mmd as the canonical workflow, but that file is not present under src/utils (the directory currently contains only AGENTS.md and PageCache.ts). This makes the documented process non-actionable for anyone following this AGENTS scope and will cause confusion for future changes until the link is corrected or the file is added.
Useful? React with 👍 / 👎.
Missing file:
|
| - `attributes` | ||
| - `sortBy` | ||
|
|
||
| ### `buildCacheKey(orgId: string, page: number, pageSize: number): string` |
There was a problem hiding this comment.
buildCacheKey first param is not always an orgId
The signature and the cache lifecycle diagram (line 72) both frame the first parameter as orgId, but AddressBook.ts:126 passes bookId (the address book identifier):
// AddressBook.ts:126
const cacheKey = this.pageCache.buildCacheKey(bookId, page, pageSize);Only Queue and EntryPoint pass an actual orgId. The parameter is really a generic scoping identifier.
Impact: An LLM told to "add PageCache to a new service" would always pass orgId. If the new service needs a different scoping key (like bookId, teamId, etc.), the LLM would get it wrong.
Suggested fix: Rename the parameter in the docs to something like scopeId or identifier, and note that consumers choose the appropriate scoping key for their domain.
| | ------------- | ---------------------------------------------------------- | ----------------------------------- | | ||
| | `AddressBook` | [`../services/AddressBook.ts`](../services/AddressBook.ts) | Caches paged address-book responses | | ||
| | `EntryPoint` | [`../services/EntryPoint.ts`](../services/EntryPoint.ts) | Caches paged entry-point responses | | ||
| | `Queue` | [`../services/Queue.ts`](../services/Queue.ts) | Caches paged queue responses | |
There was a problem hiding this comment.
Consumer Map is incomplete — missing types.ts
src/types.ts (line 10) imports PaginatedResponse and BaseSearchParams from ./utils/PageCache and uses them to define 6 downstream types:
export type AddressBookEntriesResponse = PaginatedResponse<AddressBookEntry>;
export type EntryPointListResponse = PaginatedResponse<EntryPointRecord>;
export type ContactServiceQueuesResponse = PaginatedResponse<ContactServiceQueue>;
export interface AddressBookEntrySearchParams extends BaseSearchParams { ... }
export type EntryPointSearchParams = BaseSearchParams;
export interface ContactServiceQueueSearchParams extends BaseSearchParams { ... }Impact: An LLM asked to "rename a field in BaseSearchParams" would check this Consumer Map, update the 3 listed services, and miss types.ts entirely — silently breaking every downstream type that extends these contracts.
Suggested fix: Add types.ts to the Consumer Map as a contract consumer.
| Canonical paginated response type: | ||
|
|
||
| ```typescript | ||
| type PaginatedResponse<T> = { |
There was a problem hiding this comment.
PaginatedResponse is an interface, not a type
The actual code (PageCache.ts:32) declares:
export interface PaginatedResponse<T> { ... }This matters because the codebase relies on interface-specific features — types.ts uses extends BaseSearchParams (which is also an interface, not a type). An LLM following this doc would use type + intersection (&) instead of interface + extends, producing code that diverges from the established codebase pattern.
Suggested fix: Change type PaginatedResponse<T> = { to interface PaginatedResponse<T> {.
| Clears all entries and logs cleared entry count. | ||
|
|
||
| ### `getCacheSize(): number` | ||
|
|
There was a problem hiding this comment.
clearCache() and getCacheSize() are unused — should be noted
Grepping the entire packages/@webex/contact-center/ tree (src + test) confirms zero calls to either method. All three consumers (AddressBook, EntryPoint, Queue) only use canUseCache, buildCacheKey, getCachedPage, and cachePage.
Documenting them as first-class API without noting they're currently unused will cause an LLM to wire them into new services (e.g., calling clearCache() in a destroy() lifecycle method, using getCacheSize() for health checks) — creating code that diverges from the established consumer pattern.
Suggested fix: Add a note like "Available for future use; not currently called by any consumer" so LLMs follow the actual pattern.
| }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reference usage only shows cache reads — missing the write side
All three real consumers implement a read + write pattern:
- Check cache before API call (shown here ✅)
- Call API on miss
- Cache the response after a successful API call via
cachePage()(❌ not shown)
For example, from Queue.ts:235-237:
if (this.pageCache.canUseCache({search, filter, attributes, sortBy}) && response.body?.data) {
const cacheKey = this.pageCache.buildCacheKey(orgId, page, pageSize);
this.pageCache.cachePage(cacheKey, response.body.data, response.body.meta);
}Also: AddressBook.ts omits sortBy from canUseCache() (it doesn't support sorting), while Queue and EntryPoint include it. The example shows sortBy unconditionally.
Impact: An LLM copying this example would produce a consumer that reads from cache but never populates it.
Suggested fix: Show the full read+write lifecycle, and note that sortBy inclusion depends on whether the service supports sorting.
akulakum
left a comment
There was a problem hiding this comment.
Reviewed the AGENTS.md additions for accuracy against the actual source files. Found 5 additional issues beyond those already flagged by other reviewers.
|
|
||
| Creates a typed cache instance and stores `apiName` for `LoggerProxy` context. | ||
|
|
||
| ### `canUseCache(params: CacheValidationParams): boolean` |
There was a problem hiding this comment.
CacheValidationParams interface is a public export but is not documented in the Public Contracts section.
PageCache.ts:91 exports CacheValidationParams with @public JSDoc:
export interface CacheValidationParams {
search?: string;
filter?: string;
attributes?: string;
sortBy?: string;
}This is the explicit parameter type accepted by canUseCache(). Developers or LLMs building a new consumer who construct their own params object have no guidance on the contract type from this doc. Please add a CacheValidationParams entry to the Public Contracts section.
| - `search` | ||
| - `filter` | ||
| - `attributes` | ||
| - `sortBy` |
There was a problem hiding this comment.
sortOrder bypass gap is undocumented.
BaseSearchParams (and therefore every consumer's method params) includes sortOrder: 'asc' | 'desc', but CacheValidationParams does not include sortOrder. This means a request with sortOrder=desc set but sortBy absent would still be served from cache, which may or may not be intentional.
A future contributor adding sortOrder support to a service could silently introduce incorrect cached results.
Suggested fix: Add a note clarifying the intentional behavior, e.g.: "Cache bypass is not triggered by sortOrder alone — it requires sortBy to be set. If sortOrder without sortBy is a meaningful filter for a new service, CacheValidationParams must be extended."
|
|
||
| ```mermaid | ||
| graph TD | ||
| A[Request arrives with orgId/page/pageSize] --> B{canUseCache?} |
There was a problem hiding this comment.
Mermaid diagram hardcodes orgId, but AddressBook passes bookId as the first argument.
This node and line 72 (buildCacheKey orgId:page:pageSize) reinforce the inaccurate parameter name that is also flagged on the buildCacheKey signature docs. AddressBook.ts:126 passes bookId — not orgId — as the scoping key:
// AddressBook.ts:126
const cacheKey = this.pageCache.buildCacheKey(bookId, page, pageSize);Please update both diagram nodes to use a generic identifier such as scopeId so the diagram is accurate for all three consumers.
| const pageSize = PAGINATION_DEFAULTS.PAGE_SIZE; | ||
| const cacheKey = cache.buildCacheKey(orgId, page, pageSize); | ||
|
|
||
| if (cache.canUseCache({search, filter, attributes, sortBy})) { |
There was a problem hiding this comment.
Reference usage unconditionally passes sortBy to canUseCache, but AddressBook omits it.
AddressBook.ts:125 calls:
if (this.pageCache.canUseCache({search, filter, attributes})) {It intentionally omits sortBy because AddressBook does not support sorting. Queue and EntryPoint do pass sortBy. An LLM copying this example would always include sortBy even for services that don't support sorting, creating a divergence from the AddressBook pattern.
Suggested fix: Add a comment to the example noting that sortBy should only be included when the service supports sorting, or show two variant examples.
| Standard defaults exported for callers: | ||
|
|
||
| - `PAGE: 0` | ||
| - `PAGE_SIZE: 100` |
There was a problem hiding this comment.
PageCacheEntry<T> is listed in the component table (line 19) but is absent from the Public Contracts section.
The component table advertises PageCacheEntry<T> as a shared type, but the Public Contracts section only documents PaginationMeta, PaginatedResponse<T>, BaseSearchParams, and PAGINATION_DEFAULTS. Consumers need to understand the shape of PageCacheEntry<T> — especially totalMeta.totalPages and totalMeta.totalRecords — to correctly reconstruct response objects after a cache hit (e.g., the spread in ...cachedEntry.totalMeta).
Suggested fix: Add a PageCacheEntry<T> subsection to Public Contracts documenting its data, timestamp, and totalMeta fields.
COMPLETES SPARK-790606
This pull request addresses
The Contact Center SDK Documentation (SDD) initiative requires comprehensive AI documentation for all service modules. This PR adds a new AGENTS.md for utils
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.