feat: validate embed set state params upfront with setValidState#9670
feat: validate embed set state params upfront with setValidState#9670AdityaHegde wants to merge 5 commits into
Conversation
|
@codex: review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df211696db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Expected params to be an object with a string `state` property", | ||
| ); | ||
| } | ||
| const { state, failOnError = false } = params; |
There was a problem hiding this comment.
Default to failing when validation errors are present
When callers omit failOnError, this defaults to applying the cleaned URL even if validation reported errors, but both the inline API contract above and the new public docs say the default is to fail and not apply invalid state. In that common omitted-flag case, an embedder can receive success: true and have invalid parameters silently dropped instead of the state remaining unchanged as documented.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const currentUrl = new URL(pageState.url); | ||
| currentUrl.search = url.toString(); |
There was a problem hiding this comment.
Avoid applying default-stripped URLs through session restore
Applying the canonical URL here can render a different state than the appliedState returned when the validated URL is empty or view-only and the viewer has prior embed session state. The explore embed uses DashboardStateManager with session storage enabled, and DashboardStateDataLoader.getMergedExploreState calls getPartialExploreStateFromSessionStorage for empty URLs / view-only URLs, so after a parent calls setValidState({ state: "" }) to reset the dashboard, the subsequent navigation can restore the previous session filter instead of the rill defaults that buildValidatedExploreUrl computed.
Useful? React with 👍 / 👎.
Adding
setValidState({ state, failOnError })to our embed state API.appliedStateCloses APP-918
Checklist: