Conversation
…age picker Adds a Learn more link inside the Explore query language picker popover, directly under the query language chips, opening in a new tab. The target follows the selected language, so PPL and SQL each reach their own documentation landing page. Any other language falls back to the docLink it already registers, which is how PromQL reaches the Prometheus docs. A language id matching nothing registered falls back to the SQL and PPL index, so the control always resolves to a real anchor rather than a disabled button. The url is built from docLinks.DOC_LINK_VERSION, so a release build links to documentation for its own version rather than whatever the current release describes. The lookup table uses a null prototype because queryLanguage is unvalidated URL state. Against a plain object literal an inherited key such as constructor resolves to a function, which the nullish fallback does not catch. Nothing renders in prompt mode, where the AI chip is selected and no language chip is, so naming a language would point at a visibly unselected chip. Signed-off-by: Chayanin Noramuttha <cnoramut@gmail.com>
cnoramut
changed the title
PR Reviewer Guide 🔍(Review updated until commit 83a8994)Here are some key observations to aid the review process:
|
Also could you elaborate more on this? cc @cnoramut |
| } | ||
|
|
||
| const docsRoot = `${DOCS_HOST}/${docLinks.DOC_LINK_VERSION}`; | ||
| const languagePath = LANGUAGE_DOC_PATHS[queryLanguage]; |
There was a problem hiding this comment.
This lookup order makes LanguageConfig.docLink an inconsistent extension point: a plugin updating PromQL's registered docLink.url changes this link, but updating PPL's or SQL's silently does nothing, since the local table wins for those two. If the local overrides stay, consider resolving langConfig?.docLink?.url first, or documenting why PPL/SQL intentionally bypass the registry, so a language owner's registration isn't silently ignored.
| // will not catch. Passed raw to `href` that threw `url.match is not a function`. Once | ||
| // interpolated as it is below it stops throwing and silently builds a nonsense url, so the | ||
| // null prototype is what actually keeps the fallback reachable. | ||
| const LANGUAGE_DOC_PATHS: Record<string, string> = Object.assign(Object.create(null), { |
There was a problem hiding this comment.
The null-prototype guard is correct, but it's defensive machinery that only exists because this is a string-keyed lookup on unvalidated URL state. An explicit Object.prototype.hasOwnProperty.call(LANGUAGE_DOC_PATHS, queryLanguage) check (or a plain two-branch allowlist) would make the intent self-evident and let the long comment shrink to one line. If URL resolution moves to docLinks.links per the other thread, this disappears entirely.
Replaces the local host constant, path table and default path with core's docLinks, so the versioned root is derived in one place. PPL and SQL read noDocumentation.ppl.base and sql.base, anything else keeps the docLink it registers, and PPL is the final fallback. Drops the null-prototype guard and its four tests, since there is no longer a string-keyed lookup on unvalidated url state. Adds two cases pinning that core wins over a registered docLink for PPL and SQL, which nothing covered before. Signed-off-by: Chayanin Noramuttha <cnoramut@gmail.com>
Both held pre-reorg search-plugins/sql paths that now only serve a redirect stub, since the documentation moved under sql-and-ppl. Also corrects the PPL and SQL language reference popovers in query_enhancements, which read the same two keys, and regenerates the nine snapshots that embed these urls. Signed-off-by: Chayanin Noramuttha <cnoramut@gmail.com>
|
Persistent review updated to latest commit c0464a7 |
PR Code Suggestions ✨Latest suggestions up to 83a8994
Previous suggestionsSuggestions up to commit c0464a7
|
|
Thank you for the feedback. All three constants and the null-prototype guard are gone. PPL and SQL now resolve through One correction, those keys live under I also repointed @mengweieric you are right, and it is now commented in the code. PPL and SQL bypass their registered Reading them from core would fix it at the source, but @RyanL1997 on the TODO, |
| // https://opensearch.org/docs/latest/search-plugins/sql/ppl/syntax/ | ||
| base: `${OPENSEARCH_WEBSITE_DOCS}/search-plugins/sql/ppl/syntax/`, | ||
| // https://opensearch.org/docs/latest/sql-and-ppl/ppl/index/ | ||
| base: `${OPENSEARCH_WEBSITE_DOCS}/sql-and-ppl/ppl/index/`, |
There was a problem hiding this comment.
one last thing to ask - does this change having zero impact to the other doc links which were referencing to the old url?
There was a problem hiding this comment.
Only ppl_language_reference.tsx and sql_language_reference.tsx in query_enhancements read those keys, both language reference popovers. They were pointing at the same stale stub, so this fixes them too. Nothing else in the repo references the old urls.
There was a problem hiding this comment.
actually it does have impact - see error logs here: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/34907834454/job/104193315620?pr=12746#step:27:358
There was a problem hiding this comment.
Only
ppl_language_reference.tsxandsql_language_reference.tsxinquery_enhancementsread those keys, both language reference popovers. They were pointing at the same stale stub, so this fixes them too. Nothing else in the repo references the old urls.
The above failure is from the discover ui test - lets see if the use case is still apply and if it is not lets just adjust the test. If the use case is still apply then we may need to separate these cases.
There was a problem hiding this comment.
here is the error: https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/34907834454/job/104193315717?pr=12746#step:27:291
1) discover Query UI tests
SQL-INDEX_PATTERN
should show correct documentation link pattern in language reference popover:
AssertionError: expected 'https://opensearch.org/docs/latest/sql-and-ppl/sql/index/' to match /\/search-plugins\/sql\/sql\/basic\/$/
at Context.eval (webpack://opensearch-dashboards/./cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/14/queries_ui.spec.js:164:40)
2) discover Query UI tests
PPL-INDEX_PATTERN
should show correct documentation link pattern in language reference popover:
AssertionError: expected 'https://opensearch.org/docs/latest/sql-and-ppl/ppl/index/' to match /\/search-plugins\/sql\/ppl\/syntax\/$/
at Context.eval (webpack://opensearch-dashboards/./cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/14/queries_ui.spec.js:167:40)
3) discover Query UI tests
SQL-INDEXES
should show correct documentation link pattern in language reference popover:
AssertionError: expected 'https://opensearch.org/docs/latest/sql-and-ppl/sql/index/' to match /\/search-plugins\/sql\/sql\/basic\/$/
at Context.eval (webpack://opensearch-dashboards/./cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/14/queries_ui.spec.js:164:40)
4) discover Query UI tests
PPL-INDEXES
should show correct documentation link pattern in language reference popover:
AssertionError: expected 'https://opensearch.org/docs/latest/sql-and-ppl/ppl/index/' to match /\/search-plugins\/sql\/ppl\/syntax\/$/
at Context.eval (webpack://opensearch-dashboards/./cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/14/queries_ui.spec.js:167:40)There was a problem hiding this comment.
You are right, sorry. I searched test/, cypress/ and packages/ for the old urls and got nothing, but the assertions are regexes rather than literal strings so the grep could not match them. The spec is cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/14/queries_ui.spec.js.
On whether the use case still applies, I think it does but the specific paths do not. The test is named "should show correct documentation link pattern" and its main assertion is /^https:\/\/opensearch\.org\/docs\/(latest|\d+\.\d+)\//, which checks the host and that the url is version aware. The per-language deep paths below it are just the values that happened to be current. So I have adjusted the two regexes rather than separating the cases, since the popovers linking a landing page rather than a syntax page is the intended change.
If you would rather not touch the cypress spec, I can revert the core url change instead and leave the popovers as they were, though the Learn more link would then land on the syntax page via a redirect rather than the language index. The old url is just redirecting from
https://opensearch.org/docs/latest/search-plugins/sql/ppl/syntax to https://docs.opensearch.org/latest/sql-and-ppl/ppl/commands/syntax/.
One last thing worth raising separately. The cy.request check just below, which expects a 200 to prove the link opens, does not work. docs.opensearch.org returns 200 for pages that do not exist, so that assertion passes on a dead link, and it was passing on the stale stubs. Checking the page title is the only reliable signal. Happy to open a follow-up for that.
The language reference popover assertions pinned the pre-reorg search-plugins/sql paths, which the repointed doc links no longer use. The primary assertion, that the href is a versioned opensearch.org/docs url, is unchanged. Signed-off-by: Chayanin Noramuttha <cnoramut@gmail.com>
|
Persistent review updated to latest commit 83a8994 |
3 participants
Description
Adds a "Learn more" documentation link to the Explore query language picker. The link renders inside the picker popover, directly under the query language chips, and opens in a new tab.
The target follows the selected language, so PPL and SQL each reach their own documentation landing page. Any other language falls back to the
docLinkthat language already registers, which is how PromQL reaches the Prometheus docs. A language id matching nothing registered falls back to the SQL and PPL index, so the control always resolves to a real anchor rather than a disabled button.The URL is built from
docLinks.DOC_LINK_VERSION, so a release build links to documentation for its own version instead of whatever the current release describes. Onlylatestserves thesql-and-pplpath directly. Every versioned build, 3.x and 2.x alike, resolves it through a redirect stub onto the oldersearch-plugins/sqlpath, which is a real page in each version checked.The language is named in the visible label rather than only in an accessible name, since the link sits under the chips and has to say which one it documents. The label reuses the same
titlethe chips render, so the two cannot disagree.Nothing renders in prompt mode. The AI chip is selected there and no language chip is, so naming a language would point at a visibly unselected chip. A TODO records that adding an AI documentation link needs both a URL and a second label, since
learnMoreLabelinterpolates a language.Issues Resolved
Follow-up on #12659
Screenshot
Screen.Recording.2026-09-12.at.12.45.12.PM.mov
Testing the changes
yarn start:explore --no-base-path --host 127.0.0.1 --explore.sqlSupport.enabled=trueCheck List
yarn test:jestyarn test:jest_integration