Update Serving API - from backend PR #6358#368
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the TypeScript Serving API definitions to reflect recent changes in the backend repository. It introduces new types and constants related to account billing status, product lines, and visitor sampling rates, while also extending existing definitions for custom domains and traffic sources. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
There was a problem hiding this comment.
Code Review
This pull request introduces several new types and constants to packages/types/src/config/types.gen.ts. Key additions include PlanStatus and Products definitions, a sampling_rate type and constant for Convert Signals, an optional version field for custom domains within ConfigProject, and an extension of the source type to include ai_tool. Review feedback indicates opportunities to improve the generated code by removing redundant JSDoc comments for PlanStatus, Products, and sampling_rate constants, and by correcting the naming convention of sampling_rate to SamplingRate for consistency. These issues are attributed to the code generator or OpenAPI specification.
| /** | ||
| * Account billing status | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment here is a duplicate of the one for the PlanStatus type on lines 42-44. To avoid redundancy and improve maintainability, this comment block can be removed. Since this is a generated file, this issue likely originates from the code generator or the underlying OpenAPI specification.
| /** | ||
| * The Convert product line this billing plan pertains to. | ||
| * - `experiences`: Relates to A/B testing, MVT, Split URL, and personalization features. | ||
| * - `deploy`: Relates to the "Deploy" feature for rolling out changes to specific audiences without A/B testing reports. Knowledge Base: "Deployments have the potential to contain small segments...and this could be interpreted by Privacy Authorities in Europe as identification of data subjects." | ||
| * - `addons`: Relates to add-on products that extend the core platform capabilities. | ||
| * | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment here is a duplicate of the one for the Products type on lines 58-64. To avoid redundancy and improve maintainability, this comment block can be removed. Since this is a generated file, this issue likely originates from the code generator or the underlying OpenAPI specification.
| /** | ||
| * The percentage of visitors included in sampling for Convert Signals session recordings and Heatmaps for this project. | ||
| * Higher values collect data from a larger share of traffic and consume allocation faster. | ||
| * | ||
| */ | ||
| export type sampling_rate = 5 | 10 | 20 | 30 | 40 | 50; | ||
|
|
||
| /** | ||
| * The percentage of visitors included in sampling for Convert Signals session recordings and Heatmaps for this project. | ||
| * Higher values collect data from a larger share of traffic and consume allocation faster. | ||
| * | ||
| */ | ||
| export const sampling_rate = { | ||
| '_5': 5, | ||
| '_10': 10, | ||
| '_20': 20, | ||
| '_30': 30, | ||
| '_40': 40, | ||
| '_50': 50 | ||
| } as const; |
There was a problem hiding this comment.
There are a couple of areas for improvement with the new sampling_rate type and constant:
- Duplicated JSDoc: The documentation comment is repeated for both the type and the constant. This is redundant.
- Inconsistent Naming: The name
sampling_rateuses snake_case, which is inconsistent with the PascalCase convention used for other new types likePlanStatusandProductsin this file.
To improve maintainability and consistency, I suggest renaming it to SamplingRate and removing the duplicated comment. Since this is a generated file, these changes should ideally be addressed in the OpenAPI specification or the generator configuration.
/**
* The percentage of visitors included in sampling for Convert Signals session recordings and Heatmaps for this project.
* Higher values collect data from a larger share of traffic and consume allocation faster.
*
*/
export type SamplingRate = 5 | 10 | 20 | 30 | 40 | 50;
export const SamplingRate = {
'_5': 5,
'_10': 10,
'_20': 20,
'_30': 30,
'_40': 40,
'_50': 50
} as const;
Updating TS Serving API after the latest changes from backend repo,
PR #6358