Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review Summary by QodoReplace sdk-core string-based factory functions with explicit fixed reference constants
WalkthroughsDescription• Replace generic string-based factory functions with explicit fixed reference constants across 12 API wrapper modules • Improve type safety and auditability by eliminating dynamic function reference selection • Add comprehensive test coverage and guardrails to prevent regression to string-based factories • Mark migration checklist as complete with all verification steps passed Diagramflowchart LR
A["Generic Factories<br/>getQueryRef/getMutationRef"] -->|"Replace with"| B["Fixed Reference Constants<br/>GET_*_REF, SUBMIT_*_REF"]
B -->|"Applied to"| C["12 API Modules<br/>aiAgent, articles, carousels, etc."]
C -->|"Protected by"| D["Hardening Guards<br/>refHardeningGuard.test.ts"]
D -->|"Verified by"| E["Contract Tests<br/>contracts.test.ts"]
File Changes1. packages/sdk-core/src/api/aiAgent.ts
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors @opencom/sdk-core API wrapper modules to eliminate generic string-based Convex ref factories and replace them with explicit, module-scoped fixed FunctionReference constants, with tests/guardrails added to prevent regressions.
Changes:
- Replaced
getQueryRef(name: string)/getMutationRef(name: string)usage in multiple wrapper modules with explicit*_REFconstants built viamakeFunctionReference(...). - Expanded contract tests to assert stable function paths + argument shapes for the affected wrappers.
- Added a guard test to prevent reintroducing dynamic/string-based ref factories; marked the migration checklist tasks complete.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-core/tests/refHardeningGuard.test.ts | Adds guardrails to detect reintroduction of generic/dynamic ref factories in selected wrappers. |
| packages/sdk-core/tests/contracts.test.ts | Extends contract conformance coverage across sessions, visitors, tickets, AI, articles, carousels, events, office hours, etc. |
| packages/sdk-core/src/api/aiAgent.ts | Introduces fixed refs for AI Agent queries/mutations and updates calls to use them. |
| packages/sdk-core/src/api/articles.ts | Introduces fixed refs for article queries and updates calls to use them. |
| packages/sdk-core/src/api/carousels.ts | Introduces fixed refs for carousel queries/mutations and updates calls to use them. |
| packages/sdk-core/src/api/checklists.ts | Introduces fixed refs for checklist queries/mutations and updates calls to use them (including dismiss flow). |
| packages/sdk-core/src/api/commonIssues.ts | Introduces fixed ref for common issue buttons query and updates call to use it. |
| packages/sdk-core/src/api/conversations.ts | Introduces fixed refs for conversation/message operations and updates calls to use them. |
| packages/sdk-core/src/api/events.ts | Introduces fixed refs for event mutations and updates calls to use them. |
| packages/sdk-core/src/api/officeHours.ts | Introduces fixed refs for office-hours queries and updates calls to use them. |
| packages/sdk-core/src/api/outbound.ts | Introduces fixed refs for outbound message query/mutation and updates calls to use them. |
| packages/sdk-core/src/api/sessions.ts | Introduces fixed refs for session lifecycle mutations and updates calls to use them. |
| packages/sdk-core/src/api/tickets.ts | Introduces fixed refs for ticket queries/mutations and updates calls to use them. |
| packages/sdk-core/src/api/visitors.ts | Introduces fixed refs for visitor mutations and updates calls to use them. |
| openspec/changes/replace-sdk-core-string-ref-factories/tasks.md | Marks migration tasks as complete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+26
| const GENERIC_QUERY_FACTORY_PATTERN = /\bfunction getQueryRef\(name: string\)/; | ||
| const GENERIC_MUTATION_FACTORY_PATTERN = /\bfunction getMutationRef\(name: string\)/; | ||
| const DYNAMIC_REF_FACTORY_PATTERN = /\bmakeFunctionReference\(\s*name\s*\)/; |
There was a problem hiding this comment.
The guard regexes are narrowly scoped and can be bypassed by small refactors. For example, GENERIC_QUERY_FACTORY_PATTERN / GENERIC_MUTATION_FACTORY_PATTERN only match function get*Ref(name: string) declarations (not const get*Ref = (name: string) => ...), and DYNAMIC_REF_FACTORY_PATTERN only flags makeFunctionReference(name) (not other identifiers like refName). Consider broadening the patterns to also catch arrow-function variants and any non-string-literal makeFunctionReference(...) usage so the hardening test reliably prevents reintroducing dynamic ref factories.
Suggested change
| const GENERIC_QUERY_FACTORY_PATTERN = /\bfunction getQueryRef\(name: string\)/; | |
| const GENERIC_MUTATION_FACTORY_PATTERN = /\bfunction getMutationRef\(name: string\)/; | |
| const DYNAMIC_REF_FACTORY_PATTERN = /\bmakeFunctionReference\(\s*name\s*\)/; | |
| const GENERIC_QUERY_FACTORY_PATTERN = | |
| /\b(?:function\s+getQueryRef\s*\(\s*name:\s*string\s*\)|const\s+getQueryRef\s*=\s*\(\s*name:\s*string\s*\)\s*=>)/; | |
| const GENERIC_MUTATION_FACTORY_PATTERN = | |
| /\b(?:function\s+getMutationRef\s*\(\s*name:\s*string\s*\)|const\s+getMutationRef\s*=\s*\(\s*name:\s*string\s*\)\s*=>)/; | |
| const DYNAMIC_REF_FACTORY_PATTERN = /\bmakeFunctionReference\(\s*(?!["'`])[^\s,)]+/; |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request updates the
sdk-coreAPI wrapper modules to remove the use of generic string-based factory selectors (e.g.,getQueryRef(name: string)) and replaces them with explicit, fixed function reference constants. This improves type safety, makes the code easier to audit, and prevents accidental usage of unapproved or deprecated API references. The changes also update related tests and fixtures, and mark the relevant migration checklist as complete.Key changes by theme:
API Factory Refactoring:
Replaced all instances of
getQueryRef(name: string)andgetMutationRef(name: string)with explicit, module-scoped constants for each API operation inaiAgent.ts,articles.ts,carousels.ts,checklists.ts,commonIssues.ts, andconversations.ts. This ensures only approved function references are used throughout the codebase. [1] [2] [3] [4] [5] [6]Updated all API calls in these modules to use the new explicit reference constants instead of the previous generic helper functions. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20]
Migration Task Tracking:
openspec/changes/replace-sdk-core-string-ref-factories/tasks.md, reflecting that the refactoring and verification steps have been finished.These updates make the codebase more robust, maintainable, and less error-prone by enforcing explicit API boundaries and references.