Skip to content

[SCAL-284563] Added Margin value check #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ruchI9897 wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[SCAL-284563] Added Margin value check #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e72e2c0
added new variable
ruchI9897 Nov 20, 2025
5fa76fa
removed
ruchI9897 Nov 20, 2025
37b3d6c
doc fix
ruchI9897 Nov 26, 2025
bb43cf6
variable fix
ruchI9897 Nov 26, 2025
24be201
fix docs
ruchI9897 Dec 9, 2025
1dc20bb
resolved conflicts
ruchI9897 Dec 10, 2025
da0a85d
typedoc
ruchI9897 Dec 10, 2025
8ac4896
added check on margin value
ruchI9897 Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/embed/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

import { logger } from '../utils/logger';
import { calculateVisibleElementData, getQueryParamString, isUndefined } from '../utils';
import { calculateVisibleElementData, getQueryParamString, isUndefined, isValidCssMargin } from '../utils';
import {
Param,
DOMSelector,
Expand Down Expand Up @@ -761,7 +761,13 @@ export class AppEmbed extends V1Embed {
params[Param.fullHeight] = true;
if (this.viewConfig.lazyLoadingForFullHeight) {
params[Param.IsLazyLoadingForEmbedEnabled] = true;
params[Param.RootMarginForLazyLoad] = this.viewConfig.lazyLoadingMargin;
if(typeof this.viewConfig.lazyLoadingMargin !== 'string' || !isValidCssMargin(this.viewConfig.lazyLoadingMargin)) {
console.error('lazyLoadingMargin must be a valid CSS margin string value (e.g., "10px"). Defaulting to "0px".');
params[Param.RootMarginForLazyLoad] = '0px';
}
else {
params[Param.RootMarginForLazyLoad] = this.viewConfig.lazyLoadingMargin;
}
Comment on lines +764 to +770
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

medium

This validation logic has a few areas for improvement:

  1. Logging on undefined: It currently logs an error to the console even when lazyLoadingMargin is not provided (i.e., undefined). It's better to only log an error when an invalid value is explicitly provided.
  2. Logging utility: The project uses a logger utility, which is already imported in this file. For consistency, logger.error should be used instead of console.error.
  3. Code Duplication: This same logic is duplicated in src/embed/liveboard.ts. Consider extracting it into a shared utility function in src/utils.ts to improve maintainability.

Here's a suggested refactoring that addresses these points.

                const { lazyLoadingMargin } = this.viewConfig;
                if (typeof lazyLoadingMargin === 'string' && isValidCssMargin(lazyLoadingMargin)) {
                    params[Param.RootMarginForLazyLoad] = lazyLoadingMargin;
                } else {
                    if (lazyLoadingMargin != null) {
                        logger.error('lazyLoadingMargin must be a valid CSS margin string value (e.g., "10px"). Defaulting to "0px".');
                    }
                    params[Param.RootMarginForLazyLoad] = '0px';
                }

}
}

Expand Down
10 changes: 8 additions & 2 deletions src/embed/liveboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
ErrorDetailsTypes,
EmbedErrorCodes,
} from '../types';
import { calculateVisibleElementData, getQueryParamString, isUndefined } from '../utils';
import { calculateVisibleElementData, getQueryParamString, isUndefined, isValidCssMargin } from '../utils';
import { getAuthPromise } from './base';
import { TsEmbed, V1Embed } from './ts-embed';
import { addPreviewStylesIfNotPresent } from '../utils/global-styles';
Expand Down Expand Up @@ -528,7 +528,13 @@ export class LiveboardEmbed extends V1Embed {
params[Param.fullHeight] = true;
if (this.viewConfig.lazyLoadingForFullHeight) {
params[Param.IsLazyLoadingForEmbedEnabled] = true;
params[Param.RootMarginForLazyLoad] = this.viewConfig.lazyLoadingMargin;
if(typeof this.viewConfig.lazyLoadingMargin !== 'string' || !isValidCssMargin(this.viewConfig.lazyLoadingMargin)) {
console.error('lazyLoadingMargin must be a valid CSS margin string value (e.g., "10px"). Defaulting to "0px".');
params[Param.RootMarginForLazyLoad] = '0px';
}
else {
params[Param.RootMarginForLazyLoad] = this.viewConfig.lazyLoadingMargin;
}
}
}
this.defaultHeight = minimumHeight || defaultHeight || this.defaultHeight;
Expand Down
11 changes: 10 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2247,7 +2247,8 @@ export enum EmbedEvent {
*/
ALL = '*',
/**
* Emitted when an Answer is saved in the app
* Emitted when an Answer is saved in the app.
* Use start:true to subscribe to when save is initiated, or end:true to subscribe to when save is completed. Default is end:true.
* @Version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand Down Expand Up @@ -2282,6 +2283,7 @@ export enum EmbedEvent {
Download = 'download',
/**
* Emitted when the download action is triggered on an Answer.
* Use start:true to subscribe to when download is initiated, or end:true to subscribe to when download is completed. Default is end:true.
* @version SDK: 1.21.0 | ThoughtSpot: 9.2.0.cl, 9.4.0.sw
* @example
*```js
Expand All @@ -2296,6 +2298,7 @@ export enum EmbedEvent {
DownloadAsPng = 'downloadAsPng',
/**
* Emitted when the Download as PDF action is triggered on an Answer
* Use start:true to subscribe to when download as PDF is initiated, or end:true to subscribe to when download as PDF is completed. Default is end:true.
Comment on lines 2300 to +2301
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

medium

The comment for DownloadAsPng seems to have been copied from another event. It incorrectly refers to 'PDF' instead of 'PNG'.

Suggested change
* Emitted when the Download as PDF action is triggered on an Answer
* Use start:true to subscribe to when download as PDF is initiated, or end:true to subscribe to when download as PDF is completed. Default is end:true.
* Emitted when the Download as PNG action is triggered on an Answer
* Use start:true to subscribe to when download as PNG is initiated, or end:true to subscribe to when download as PNG is completed. Default is end:true.

* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand All @@ -2310,6 +2313,7 @@ export enum EmbedEvent {
DownloadAsPdf = 'downloadAsPdf',
/**
* Emitted when the Download as CSV action is triggered on an Answer.
* Use start:true to subscribe to when download as CSV is initiated, or end:true to subscribe to when download as CSV is completed. Default is end:true.
Comment on lines 2315 to +2316
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

medium

The comment for DownloadAsPdf seems to have been copied from another event. It incorrectly refers to 'CSV' instead of 'PDF'.

Suggested change
* Emitted when the Download as CSV action is triggered on an Answer.
* Use start:true to subscribe to when download as CSV is initiated, or end:true to subscribe to when download as CSV is completed. Default is end:true.
* Emitted when the Download as PDF action is triggered on an Answer.
* Use start:true to subscribe to when download as PDF is initiated, or end:true to subscribe to when download as PDF is completed. Default is end:true.

* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand All @@ -2324,6 +2328,7 @@ export enum EmbedEvent {
DownloadAsCsv = 'downloadAsCsv',
/**
* Emitted when the Download as XLSX action is triggered on an Answer.
* Use start:true to subscribe to when download as XLSX is initiated, or end:true to subscribe to when download as XLSX is completed. Default is end:true.
Comment on lines 2330 to +2331
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

medium

The comment for DownloadAsCsv seems to have been copied from another event. It incorrectly refers to 'XLSX' instead of 'CSV'.

Suggested change
* Emitted when the Download as XLSX action is triggered on an Answer.
* Use start:true to subscribe to when download as XLSX is initiated, or end:true to subscribe to when download as XLSX is completed. Default is end:true.
* Emitted when the Download as CSV action is triggered on an Answer.
* Use start:true to subscribe to when download as CSV is initiated, or end:true to subscribe to when download as CSV is completed. Default is end:true.

* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand All @@ -2338,6 +2343,7 @@ export enum EmbedEvent {
DownloadAsXlsx = 'downloadAsXlsx',
/**
* Emitted when an Answer is deleted in the app
* Use start:true to subscribe to when delete is initiated, or end:true to subscribe to when delete is completed. Default is end:true.
* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand All @@ -2364,6 +2370,7 @@ export enum EmbedEvent {
/**
* Emitted when a user initiates the Pin action to
* add an Answer to a Liveboard.
* Use start:true to subscribe to when pin is initiated, or end:true to subscribe to when pin is completed. Default is end:true.
* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand Down Expand Up @@ -2477,6 +2484,7 @@ export enum EmbedEvent {
/**
* Emitted when the **Export TML** action is triggered on an
* an embedded object in the app
* Use start:true to subscribe to when export is initiated, or end:true to subscribe to when export is completed. Default is end:true.
* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand All @@ -2502,6 +2510,7 @@ export enum EmbedEvent {
SaveAsView = 'saveAsView',
/**
* Emitted when the user creates a copy of an Answer.
* Use start:true to subscribe to when copy and edit is initiated, or end:true to subscribe to when copy and edit is completed. Default is end:true.
Comment on lines 2512 to +2513
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

medium

The comment for SaveAsView seems to be incorrect. It describes creating a copy, which is likely intended for the CopyAEdit event. The comment should describe the 'Save as View' action.

Suggested change
* Emitted when the user creates a copy of an Answer.
* Use start:true to subscribe to when copy and edit is initiated, or end:true to subscribe to when copy and edit is completed. Default is end:true.
* Emitted when an Answer is saved as a View.
* Use start:true to subscribe to when save as view is initiated, or end:true to subscribe to when save as view is completed. Default is end:true.

* @version SDK: 1.11.0 | ThoughtSpot: 8.3.0.cl, 8.4.1.sw
* @example
*```js
Expand Down
17 changes: 17 additions & 0 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,23 @@ export const getCssDimension = (value: number | string): string => {
return value;
};

/**
* Validates if a string is a valid CSS margin value.
* @param value - The string to validate
* @returns true if the value is a valid CSS margin value, false otherwise
*/
export const isValidCssMargin = (value: string): boolean => {
if (typeof value !== 'string' || value.trim() === '') {
return false;
}

// Check for CSS length values (e.g., "10px", "1em", "50%")
const cssLengthPattern = /^\d+(\.\d+)?(px|em|rem|%|vh|vw)$/i;
const parts = value.trim().split(/\s+/);

return parts.length <= 4 && parts.every(part => cssLengthPattern.test(part.trim()));
};
Comment on lines +140 to +150
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

high

The CSS margin validation can be improved to support negative values and unitless zero, which are valid in CSS. For example, margin: -10px and margin: 0 are both valid.

I've also removed a redundant empty line for better code style.

Suggested change
export const isValidCssMargin = (value: string): boolean => {
if (typeof value !== 'string' || value.trim() === '') {
return false;
}
// Check for CSS length values (e.g., "10px", "1em", "50%")
const cssLengthPattern = /^\d+(\.\d+)?(px|em|rem|%|vh|vw)$/i;
const parts = value.trim().split(/\s+/);
return parts.length <= 4 && parts.every(part => cssLengthPattern.test(part.trim()));
};
export const isValidCssMargin = (value: string): boolean => {
if (typeof value !== 'string' || value.trim() === '') {
return false;
}
// Check for CSS length values (e.g., "10px", "1em", "50%", "-10px").
// Also allows for unitless 0.
const cssLengthPattern = /^-?\d+(\.\d+)?(px|em|rem|%|vh|vw)$/i;
const parts = value.trim().split(/\s+/);
return parts.length <= 4 && parts.every(part => part === '0' || cssLengthPattern.test(part));
};


export const getSSOMarker = (markerId: string) => {
const encStringToAppend = encodeURIComponent(markerId);
return `tsSSOMarker=${encStringToAppend}`;
Expand Down
Loading
Loading