fix(compass-query-bar): add a maxTimeMS over 5min warning to CompassWeb COMPASS-9023 #7380
Conversation
| ); | ||
|
|
||
| // Data Explorer limits for maxTimeMS (5 minutes = 300,000ms) | ||
| const WEB_MAX_TIME_MS_LIMIT = 300_000; // 5 minutes |
There was a problem hiding this comment.
I'd suggest storing this in a single place + explaining why 5 minutes is the value we chose here
nbbeeken
force-pushed
the
COMPASS-9023
branch
from
467c47f to
07cfa90
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a maxTimeMS warning system for CompassWeb to prevent users from setting query timeouts longer than 5 minutes (300,000ms), which is the Data Explorer backend limit. The implementation uses a new preference showMaxTimeMSWarning that is enabled by default in the web environment.
- Adds
showMaxTimeMSWarningpreference to control the warning behavior - Implements hard limit enforcement and validation for maxTimeMS fields
- Adds tooltip warnings when users attempt to exceed the 5-minute limit
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-web/src/entrypoint.tsx | Enables the showMaxTimeMSWarning preference by default for CompassWeb |
| packages/compass-preferences-model/src/preferences-schema.tsx | Defines the new showMaxTimeMSWarning preference with proper schema validation |
| packages/compass-query-bar/src/constants/query-option-definition.ts | Adds WEB_MAX_TIME_MS_LIMIT constant and updates maxTimeMS input validation |
| packages/compass-query-bar/src/utils/query.ts | Updates validation functions to enforce the 5-minute limit when warnings are enabled |
| packages/compass-query-bar/src/stores/query-bar-reducer.ts | Passes showMaxTimeMSWarning preference to validation functions |
| packages/compass-query-bar/src/components/query-option.tsx | Adds tooltip warning UI for maxTimeMS fields exceeding the limit |
| packages/compass-aggregations/src/components/pipeline-toolbar/pipeline-options/pipeline-collation.tsx | Applies the same maxTimeMS validation and warning to aggregation pipeline options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...compass-aggregations/src/components/pipeline-toolbar/pipeline-options/pipeline-collation.tsx
Outdated
Show resolved
Hide resolved
...compass-aggregations/src/components/pipeline-toolbar/pipeline-options/pipeline-collation.tsx
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
| } | ||
| const fields = mapQueryToFormFields( | ||
| { maxTimeMS: preferences.getPreferences().maxTimeMS }, | ||
| { |
There was a problem hiding this comment.
there are two more instances of mapQueryToFormFields in this file and another in the option-editor.tsx, is it expected that those don't get the new param?
| if (field === 'maxTimeMS') { | ||
| const maxTimeMS = Number(value); | ||
|
|
||
| // When warning is enabled, enforce hard limit |
There was a problem hiding this comment.
This comment is needed because the name of the preference is not clear ;) Sounds like the switch is really to enforce or not enforce an upper bound to maxTimeMs - WEB_MAX_TIME_MS_LIMIT.
What do you think about having the upper bound as numeric optional preference, so that the constant IS the preference and it is only defined once?
There was a problem hiding this comment.
Ooo I like that, thanks for the idea 🫡
Description
Using a preference I control whether or not to show the warning and prevent setting a higher maxTimeMS.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes