SCAL-286984 Set default values true for excludeRuntimeFiltersfromURL and excludeRuntimeParametersfromURL #397
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @shivam-kumar-ts, 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 significant change to the default handling of runtime filters and parameters in embedded views. Previously, these were included in the URL by default, but now the default behavior is to exclude them, promoting cleaner URLs and potentially enhancing privacy. The change is reflected in the core embedding logic, updated documentation, and adjusted test cases to ensure compatibility and correctness. 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. 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
|
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
This pull request updates the default value for excludeRuntimeFiltersfromURL and excludeRuntimeParametersfromURL to true. The changes in the type definitions and test files are consistent with this new default.
However, I found an issue in how the default values are applied in the TsEmbed constructor. If viewConfig provides undefined for these properties, it overrides the default, leading to unexpected behavior. I've suggested a fix to make the default value assignment more robust.
Additionally, one of the updated tests in ts-embed.spec.ts seems to assert the current buggy behavior. I've left a comment explaining why it should be adjusted to align with the corrected logic.
| this.viewConfig = { | ||
| excludeRuntimeFiltersfromURL: false, | ||
| excludeRuntimeParametersfromURL: false, | ||
| excludeRuntimeFiltersfromURL: true, | ||
| excludeRuntimeParametersfromURL: true, | ||
| ...viewConfig, | ||
| }; |
There was a problem hiding this comment.
The current implementation for setting default values for excludeRuntimeFiltersfromURL and excludeRuntimeParametersfromURL has a potential issue. When viewConfig contains these properties with an undefined value, the default values (true) are overridden with undefined. This can lead to unexpected behavior, as undefined is treated as falsy, effectively behaving like false instead of the intended default of true.
To ensure that undefined values correctly fall back to the default, I suggest applying the defaults after spreading viewConfig and using the nullish coalescing operator (??). This makes the default assignment more robust.
| this.viewConfig = { | |
| excludeRuntimeFiltersfromURL: false, | |
| excludeRuntimeParametersfromURL: false, | |
| excludeRuntimeFiltersfromURL: true, | |
| excludeRuntimeParametersfromURL: true, | |
| ...viewConfig, | |
| }; | |
| this.viewConfig = { | |
| ...viewConfig, | |
| excludeRuntimeFiltersfromURL: viewConfig?.excludeRuntimeFiltersfromURL ?? true, | |
| excludeRuntimeParametersfromURL: viewConfig?.excludeRuntimeParametersfromURL ?? true, | |
| }; |
There was a problem hiding this comment.
Since default value is we have to consider as true, so undefine is also consider as true
| const searchEmbed = new SearchEmbed(getRootEl(), { | ||
| ...defaultViewConfig, | ||
| runtimeFilters: mockRuntimeFilters, | ||
| excludeRuntimeFiltersfromURL: undefined, |
There was a problem hiding this comment.
This test case seems to assert that excludeRuntimeFiltersfromURL: undefined should behave like false, meaning runtime filters are not included in the APP_INIT payload. However, undefined should ideally fall back to the default value, which is now true. With the default value of true, runtime filters should be included in the APP_INIT payload. I've suggested a fix in src/embed/ts-embed.ts to handle undefined correctly. With that fix, this test will fail. This test should be updated to reflect the expected behavior where undefined acts like the default true.
|
SonarQube Quality Gate
|
shivam-kumar-ts
changed the title
STILL WORKING NEED MORE CHANGES IN SDK