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 introduces a new SDK capability that empowers users to manage and test Liveboard email schedules more effectively within embedded applications. By adding the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
midhun-kr
force-pushed
the
SCAL-299968-sdk
branch
from
228eb5e to
e271306
Compare
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces the isSendNowLiveboardSchedulingEnabled flag, enabling a new 'Send Now' feature for Liveboard scheduling. This includes adding the configuration option to AppViewConfig and LiveboardViewConfig, implementing its handling in AppEmbed and LiveboardEmbed classes, and adding corresponding tests. Additionally, new EmbedEvent, HostEvent, Param, and Action enum members have been added in src/types.ts to support this functionality. The changes are well-tested and documented.
src/embed/app.ts
Outdated
| * @version SDK: 1.47.0 | ThoughtSpot Cloud: 26.5.0.cl | ||
| * @default false |
There was a problem hiding this comment.
The @version tag specifies SDK: 1.47.0 | ThoughtSpot Cloud: 26.5.0.cl. Please ensure this version mapping is accurate and has been reviewed by a TSE (ThoughtSpot Embedded) developer, as per the style guide's requirement to flag any mismatches.
References
- The
@versiontag must follow the canonical mapping between SDK and ThoughtSpot Cloud versions. If the version pairing in a PR does not match this sequence, it should be flagged for review by a TSE (ThoughtSpot Embedded) developer. (link)
src/embed/app.ts
Outdated
| isPNGInScheduledEmailsEnabled = false, | ||
| isLiveboardXLSXCSVDownloadEnabled = false, | ||
| isGranularXLSXCSVSchedulesEnabled = false, | ||
| isSendNowLiveboardSchedulingEnabled = false, |
There was a problem hiding this comment.
The style guide recommends preferring undefined over false for boolean parameters to avoid unnecessarily increasing the URL/query string size. Here, isSendNowLiveboardSchedulingEnabled is destructured with a default value of false, which will always send ?isSendNowLiveboardSchedulingEnabled=false even when not explicitly set by the caller. It should be undefined by default and only added to the URL if explicitly provided.
isSendNowLiveboardSchedulingEnabled,| isSendNowLiveboardSchedulingEnabled = false, | |
| isSendNowLiveboardSchedulingEnabled, |
References
- On the ThoughtSpot side,
undefined(omitted param) is treated as falsy. There is no need to explicitly passfalsewhen a flag is not set. Explicitly passingfalsefor every unset flag increases the URL/query string size unnecessarily. Omit the param entirely when the value is not explicitly provided. Do not use= falseas a default in destructuring. Leave the value asundefinedwhen not set, and only add the param to the request when the caller explicitly provides a value (!== undefined). (link)
src/embed/liveboard.ts
Outdated
| * @version SDK: 1.47.0 | ThoughtSpot Cloud: 26.5.0.cl | ||
| * @default false |
There was a problem hiding this comment.
The @version tag specifies SDK: 1.47.0 | ThoughtSpot Cloud: 26.5.0.cl. Please ensure this version mapping is accurate and has been reviewed by a TSE (ThoughtSpot Embedded) developer, as per the style guide's requirement to flag any mismatches.
References
- The
@versiontag must follow the canonical mapping between SDK and ThoughtSpot Cloud versions. If the version pairing in a PR does not match this sequence, it should be flagged for review by a TSE (ThoughtSpot Embedded) developer. (link)
There was a problem hiding this comment.
Changed to 1.48
src/embed/liveboard.ts
Outdated
| isPNGInScheduledEmailsEnabled = false, | ||
| isLiveboardXLSXCSVDownloadEnabled = false, | ||
| isGranularXLSXCSVSchedulesEnabled = false, | ||
| isSendNowLiveboardSchedulingEnabled = false, |
There was a problem hiding this comment.
The style guide recommends preferring undefined over false for boolean parameters to avoid unnecessarily increasing the URL/query string size. Here, isSendNowLiveboardSchedulingEnabled is destructured with a default value of false, which will always send ?isSendNowLiveboardSchedulingEnabled=false even when not explicitly set by the caller. It should be undefined by default and only added to the URL if explicitly provided.
isSendNowLiveboardSchedulingEnabled,| isSendNowLiveboardSchedulingEnabled = false, | |
| isSendNowLiveboardSchedulingEnabled, |
References
- On the ThoughtSpot side,
undefined(omitted param) is treated as falsy. There is no need to explicitly passfalsewhen a flag is not set. Explicitly passingfalsefor every unset flag increases the URL/query string size unnecessarily. Omit the param entirely when the value is not explicitly provided. Do not use= falseas a default in destructuring. Leave the value asundefinedwhen not set, and only add the param to the request when the caller explicitly provides a value (!== undefined). (link)
src/types.ts
Outdated
| * ``` | ||
| * @version SDK: 1.48.0 | ThoughtSpot Cloud: 26.6.0.cl |
There was a problem hiding this comment.
The @version tag specifies SDK: 1.47.0 | ThoughtSpot Cloud: 26.5.0.cl. Please ensure this version mapping is accurate and has been reviewed by a TSE (ThoughtSpot Embedded) developer, as per the style guide's requirement to flag any mismatches.
References
- The
@versiontag must follow the canonical mapping between SDK and ThoughtSpot Cloud versions. If the version pairing in a PR does not match this sequence, it should be flagged for review by a TSE (ThoughtSpot Embedded) developer. (link)
midhun-kr
force-pushed
the
SCAL-299968-sdk
branch
from
e271306 to
19ae4e1
Compare
midhun-kr
force-pushed
the
SCAL-299968-sdk
branch
from
19ae4e1 to
d1e1f13
Compare
|
SonarQube Quality Gate
|
2 participants
No description provided.