Restrict Explore to support only Time based Datasets #11207
Conversation
LDrago27
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
Maosaic,
SuZhou-Joe,
TackAdam,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ps48,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
LDrago27
requested review from
paulstn,
ruchidh and
sumukhswamy
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR introduces filtering logic to exclude non-time-field datasets in specific contexts by adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LDrago27
force-pushed
the
restrictDatset
branch
2 times, most recently
from
197bbc8 to
fb561a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugins/data/public/ui/dataset_select/dataset_select.tsx`:
- Around line 434-437: The dropdown currently filters out datasets without a
time field but selection resolution still allows an already-selected dataset to
remain when showNonTimeFieldDatasets is false; update the selection logic in
updateSelectedDataset (and any resolver that reads currentQuery.dataset) to
perform the same check using detailedDataset.timeFieldName and
showNonTimeFieldDatasets, and if the selected dataset lacks a timeFieldName when
showNonTimeFieldDatasets is false, clear currentQuery.dataset (or pick the
nearest valid dataset) and trigger the existing selection flow so the UI and
deep links cannot bypass the restriction.
🧹 Nitpick comments (1)
src/plugins/data/public/ui/dataset_select/dataset_select.tsx (1)
235-239: ThreadshowNonTimeFieldDatasetsthrough toViewDatasetsModaland conditionally render the disclaimer.The
TimeBasedDatasetDisclaimeris rendered unconditionally in the modal, but theshowNonTimeFieldDatasetsflag exists inDatasetSelectto control whether non-time-based datasets are allowed. The flag is not currently passed toViewDatasetsModal, causing the disclaimer to appear even in contexts where non-time datasets are valid. In Explore (which setsshowNonTimeFieldDatasets={false}), this is particularly misleading.♻️ Suggested wiring
interface ViewDatasetsModalProps { datasets: DetailedDataset[]; isLoading: boolean; onClose: () => void; services: IDataPluginServices; + showNonTimeFieldDatasets?: boolean; } const ViewDatasetsModal: React.FC<ViewDatasetsModalProps> = ({ datasets, isLoading, onClose, services, + showNonTimeFieldDatasets = true, }) => { ... return ( <EuiModal onClose={onClose} maxWidth="800px"> <EuiModalHeader> ... </EuiModalHeader> <EuiModalBody> - <TimeBasedDatasetDisclaimer - onClick={() => { - onClose(); - services.application?.navigateToApp('datasets'); - }} - /> + {!showNonTimeFieldDatasets && ( + <TimeBasedDatasetDisclaimer + onClick={() => { + onClose(); + services.application?.navigateToApp('datasets'); + }} + /> + )} <EuiFieldSearch ...<ViewDatasetsModal datasets={datasets} isLoading={isLoading} onClose={() => overlay?.close()} services={services} + showNonTimeFieldDatasets={showNonTimeFieldDatasets} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.jssrc/plugins/data/public/ui/dataset_select/_dataset_select.scsssrc/plugins/data/public/ui/dataset_select/dataset_select.tsxsrc/plugins/data/public/ui/dataset_selector/advanced_selector.tsxsrc/plugins/data/public/ui/dataset_selector/configurator/configurator_v2.tsxsrc/plugins/explore/public/components/query_panel/query_panel_widgets/dataset_select/dataset_select.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: LDrago27
Repo: opensearch-project/OpenSearch-Dashboards PR: 11138
File: cypress/utils/commands.osd.js:802-806
Timestamp: 2026-01-13T21:54:42.499Z
Learning: The `setExplorePage` command in `cypress/utils/commands.osd.js` (added in PR `#11138`) is currently unused and will be cleaned up in a subsequent PR. It has a syntax error (missing closing quote in the dataset URL construction) that should be fixed when the utility is either used or removed.
📚 Learning: 2026-01-13T21:54:42.499Z
Learnt from: LDrago27
Repo: opensearch-project/OpenSearch-Dashboards PR: 11138
File: cypress/utils/commands.osd.js:802-806
Timestamp: 2026-01-13T21:54:42.499Z
Learning: The `setExplorePage` command in `cypress/utils/commands.osd.js` (added in PR `#11138`) is currently unused and will be cleaned up in a subsequent PR. It has a syntax error (missing closing quote in the dataset URL construction) that should be fixed when the utility is either used or removed.
Applied to files:
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
🧬 Code graph analysis (2)
src/plugins/data/public/ui/dataset_select/dataset_select.tsx (3)
src/plugins/data/public/ui/dataset_select/index.tsx (2)
DatasetSelect(12-16)DatasetSelectProps(19-19)src/plugins/data/public/index.ts (1)
DatasetSelectProps(514-514)src/plugins/data/public/ui/index.ts (1)
DatasetSelectProps(32-32)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (2)
cypress/utils/apps/explore/saved_queries.js (1)
config(141-141)cypress/utils/apps/query_enhancements/saved_queries.js (1)
config(155-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on macOS x64
🔇 Additional comments (15)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js (1)
70-72: Cleanup update looks consistent with the simplified setup.src/plugins/data/public/ui/dataset_select/_dataset_select.scss (1)
131-134: Spacing for the time-based disclaimer looks good.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (2)
76-76: Test title update is clear and consistent.
106-122: Good coverage for filtering non‑time datasets in the selector.src/plugins/explore/public/components/query_panel/query_panel_widgets/dataset_select/dataset_select.tsx (1)
248-253: Prop wiring cleanly enforces time‑based datasets in Explore.src/plugins/data/public/ui/dataset_selector/advanced_selector.tsx (2)
40-58: Prop surface update is straightforward and consistent.
96-106: Forwarding the flag to the configurator is correctly wired.src/plugins/data/public/ui/dataset_selector/configurator/configurator_v2.tsx (2)
32-50: Defaulting the new flag keeps behavior backward‑compatible.
205-216: Conditional non‑time option rendering matches the new restriction.src/plugins/data/public/ui/dataset_select/dataset_select.tsx (6)
32-33: No concerns with the new EUI imports.
47-72: Clean, reusable disclaimer component.
Looks good as a focused UI helper.
84-84: No concerns with the new prop wiring here.
272-277: No concerns with the default prop value here.
497-506: No concerns with the updated dependency list.
796-796: No concerns with prop propagation here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js`:
- Around line 70-72: The cleanup call uses
cleanupWorkspaceAndDataSourceAndIndices but that helper only calls
deleteWorkspaceByNameUsingEndpoint and never deletes indices like
INDEX_WITH_TIME_1; update the helper (cleanupWorkspaceAndDataSourceAndIndices in
cypress/utils/commands.osd.js) to either accept an indices parameter or
explicitly delete INDEX_WITH_TIME_1 by calling the OpenSearch index delete
endpoint (or an existing cy.osd.deleteIndex helper) before invoking
deleteWorkspaceByNameUsingEndpoint, and ensure tests that call it pass the index
name so indices are removed when workspace deletion does not cascade.
🧹 Nitpick comments (2)
src/plugins/data/public/ui/dataset_select/_dataset_select.scss (1)
131-134: Minor: Redundant margin declaration.Setting
margin: 0and thenmargin-bottom: 2pxcan be simplified to a single declaration.♻️ Suggested simplification
&__timeBasedDisclaimer { - margin: 0; - margin-bottom: 2px; + margin: 0 0 2px 0; }src/plugins/data/public/ui/dataset_select/dataset_select.tsx (1)
234-240: Consider making the disclaimer conditional.The
TimeBasedDatasetDisclaimeris always rendered in theViewDatasetsModal, but its purpose is to explain why some datasets were filtered out. Consider showing it only whenshowNonTimeFieldDatasetsisfalse, since the disclaimer message ("Only time-based Datasets are supported") may confuse users when all datasets are actually shown.Please verify if this disclaimer should always be shown or only when non-time-field datasets are being filtered. If conditional, the modal would need to receive the
showNonTimeFieldDatasetsprop.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.jscypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.jssrc/plugins/data/public/ui/dataset_select/_dataset_select.scsssrc/plugins/data/public/ui/dataset_select/dataset_select.tsxsrc/plugins/data/public/ui/dataset_selector/advanced_selector.tsxsrc/plugins/data/public/ui/dataset_selector/configurator/configurator_v2.tsxsrc/plugins/explore/public/components/query_panel/query_panel_widgets/dataset_select/dataset_select.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: LDrago27
Repo: opensearch-project/OpenSearch-Dashboards PR: 11138
File: cypress/utils/commands.osd.js:802-806
Timestamp: 2026-01-13T21:54:42.499Z
Learning: The `setExplorePage` command in `cypress/utils/commands.osd.js` (added in PR `#11138`) is currently unused and will be cleaned up in a subsequent PR. It has a syntax error (missing closing quote in the dataset URL construction) that should be fixed when the utility is either used or removed.
📚 Learning: 2026-01-13T21:54:42.499Z
Learnt from: LDrago27
Repo: opensearch-project/OpenSearch-Dashboards PR: 11138
File: cypress/utils/commands.osd.js:802-806
Timestamp: 2026-01-13T21:54:42.499Z
Learning: The `setExplorePage` command in `cypress/utils/commands.osd.js` (added in PR `#11138`) is currently unused and will be cleaned up in a subsequent PR. It has a syntax error (missing closing quote in the dataset URL construction) that should be fixed when the utility is either used or removed.
Applied to files:
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
🧬 Code graph analysis (1)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (2)
cypress/utils/apps/explore/saved_queries.js (1)
config(141-141)cypress/utils/apps/query_enhancements/saved_queries.js (1)
config(155-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
🔇 Additional comments (14)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/simple_dataset_select.spec.js (2)
70-104: LGTM!The test configuration is correctly set up to only include the time-based index pattern, and the test properly validates the presence of the timestamp column when relevant.
106-122: LGTM! Good test coverage for the new filtering behavior.The test correctly validates that non-time-based datasets are filtered out when enhancement mode is active. The assertions check both the count (exactly 1 option) and content (no options contain the non-time index pattern text).
src/plugins/explore/public/components/query_panel/query_panel_widgets/dataset_select/dataset_select.tsx (1)
248-254: LGTM!This correctly sets
showNonTimeFieldDatasets={false}for the Explore context, enforcing the time-based dataset restriction as intended by this PR.src/plugins/data/public/ui/dataset_selector/advanced_selector.tsx (2)
40-58: LGTM!The
showNonTimeFieldDatasetsprop is correctly added to the component's props interface and destructured for use.
96-116: LGTM!The prop is correctly forwarded to
ConfiguratorComponent(which is eitherConfiguratorV2orConfigurator).DatasetExplorerdoesn't need this prop as it handles browsing, not time field configuration.src/plugins/data/public/ui/dataset_selector/configurator/configurator_v2.tsx (2)
32-50: LGTM!The
showNonTimeFieldDatasetsprop is correctly added with a default value oftrue, maintaining backward compatibility for existing usages.
205-227: LGTM!The conditional rendering correctly omits the "no time field" option when
showNonTimeFieldDatasetsisfalse. The existingonChangehandler at line 220 already handles thenoTimeFiltervalue correctly by settingtimeFieldNametoundefined.src/plugins/data/public/ui/dataset_select/dataset_select.tsx (6)
47-72: LGTM!The
TimeBasedDatasetDisclaimercomponent is well-structured with clear props and appropriate i18n usage for the message and link text.
80-85: LGTM!The
showNonTimeFieldDatasetsprop is correctly added to the public interface.
272-277: LGTM!The default value
truemaintains backward compatibility for existing usages of this component.
434-438: LGTM!The filtering logic correctly excludes datasets without a
timeFieldNamewhenshowNonTimeFieldDatasetsisfalse.
497-506: LGTM!The
showNonTimeFieldDatasetsdependency is correctly added to theuseCallbackdependencies array, ensuring the filtering logic re-runs when this prop changes.
791-797: LGTM!The
showNonTimeFieldDatasetsprop is correctly forwarded to theAdvancedSelectorwhen creating a new dataset, ensuring consistent behavior between the dataset list filtering and the create flow.cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js (1)
38-53: Test setup correctly aligns with PR objectives.The test now focuses exclusively on time-based datasets (
INDEX_WITH_TIME_1withtimestampfield), which correctly reflects the new restriction that Explore only supports time-based datasets.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...s/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/03/table.spec.js
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11207 +/- ##
=======================================
Coverage 60.14% 60.15%
=======================================
Files 4643 4646 +3
Lines 129179 129107 -72
Branches 21921 21916 -5
=======================================
- Hits 77697 77665 -32
+ Misses 45947 45921 -26
+ Partials 5535 5521 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LDrago27
force-pushed
the
restrictDatset
branch
6 times, most recently
from
4880026 to
29fef87
Compare
Signed-off-by: Suchit Sahoo <[email protected]>
Signed-off-by: Suchit Sahoo <[email protected]>
LDrago27
force-pushed
the
restrictDatset
branch
2 times, most recently
from
12bb016 to
0c155ab
Compare
Signed-off-by: Suchit Sahoo <[email protected]>
LDrago27
force-pushed
the
restrictDatset
branch
from
0c155ab to
2aed3ed
Compare
4 participants
Description
This PR restricts Explore to support only Time based Datasets.
Changes:
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration