Fix custom date range Apply state - #2444
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59e05fffbe
ℹ️ 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".
| const range = extractRangeExpressions(rangeValue); | ||
| if (range) { | ||
| startValue = range.start ?? ''; | ||
| endValue = range.end ?? ''; | ||
| } |
There was a problem hiding this comment.
Seed fields from the persisted quick range
In the production DateFacetedFilter flow, selecting a quick range invokes handleSelect, which sets open = false and unmounts the picker inside Popover.Content, so the state assigned here is discarded. When the picker is reopened, its value is a common range and the initialization effect intentionally skips common ranges; expanding Custom and changing now-90d to now-1y therefore still leaves endValue empty and Apply disabled. Initialize the fields from the current common value when the picker mounts or Custom is expanded, rather than only storing them during the quick-range click.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 026301f. The picker now derives its custom fields from the persisted string value on mount and prop changes, so the values survive the DateFacetedFilter popover unmount/remount. Quick-range selection no longer stores a second ephemeral copy. Added a remount regression test and QuickRangeCustomEditing Storybook scenario. Verified with 34 client test files / 402 tests, svelte-check, lint, and agent-browser: [now-90d TO now] opens Custom with now-90d/now, accepts now-1y, and Apply is enabled/clickable.
Summary: Seed custom range fields when a quick range is selected so the now end value is preserved when changing Last 90 days to now-1y. Add focused regression coverage. Verification: npm run test:unit -- --run passed 34 files and 402 tests; npm run check passed with 0 errors and 0 warnings; Prettier and ESLint passed; agent-browser Storybook dogfood passed the Last 90 days to Custom range to now-1y to Apply flow; thermo-nuclear code-quality review found no structural blockers. Full local AppHost verification was blocked because Elasticsearch exited with code 1 after a Docker hostname-resolution UnknownHostException. Breaking changes: none.