feat(explore): Add hooks for date page filter props based on data cat… #103822
Conversation
…egory This replaces the custom functions we wrote for each data category with a hook that accepts a list of data categories so we can automatically determine what the expected date page filter props should be. This sets it up so that we can replace the hook with an alternate implementation in order to support the coming retention changes.
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| export function useMaxPickableDays({ | ||
| dataCategories, | ||
| organization, | ||
| }: UseMaxPickableDaysProps): MaxPickableDaysOptions { |
There was a problem hiding this comment.
This hook will be replaced by an implementation that looks at the org's subscription.
k-fish
left a comment
k-fish
left a comment
There was a problem hiding this comment.
The picking logic is a little convoluted but the test coverage looks good enough, so 👍. Couple minor nits
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| case DataCategory.TRACE_METRICS: | ||
| case DataCategory.LOG_BYTE: | ||
| case DataCategory.LOG_ITEM: | ||
| return { | ||
| maxPickableDays: 30, | ||
| maxUpgradableDays: 30, | ||
| defaultPeriod: '24h', | ||
| }; |
There was a problem hiding this comment.
Bug: The max() function in useMaxPickableDays.tsx can incorrectly drop the defaultPeriod property when comparing MaxPickableDaysOptions objects.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The max() function in useMaxPickableDays.tsx (lines 55-68) incorrectly handles MaxPickableDaysOptions objects when one contains a defaultPeriod and another does not. If useMaxPickableDays is called with multiple data categories, and one category provides a defaultPeriod while another does not, the max() function may return an object missing the defaultPeriod. This can lead to datePageFilterProps.defaultPeriod being undefined in content.tsx, causing defaultSelection to be undefined when a default period should have been applied.
💡 Suggested Fix
Modify the max() function in useMaxPickableDays.tsx to ensure defaultPeriod is preserved if present in any of the compared MaxPickableDaysOptions objects, prioritizing the defaultPeriod from the object that "wins" the maxPickableDays and maxUpgradableDays comparison, or merging it if appropriate.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/utils/useMaxPickableDays.tsx#L90-L97
Potential issue: The `max()` function in `useMaxPickableDays.tsx` (lines 55-68)
incorrectly handles `MaxPickableDaysOptions` objects when one contains a `defaultPeriod`
and another does not. If `useMaxPickableDays` is called with multiple data categories,
and one category provides a `defaultPeriod` while another does not, the `max()` function
may return an object missing the `defaultPeriod`. This can lead to
`datePageFilterProps.defaultPeriod` being `undefined` in `content.tsx`, causing
`defaultSelection` to be `undefined` when a default period should have been applied.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3289398
Zylphrex
deleted the
txiao/feat/add-hooks-for-date-page-filter-props-based-on-data-category
branch
|
PR reverted: 8a3d936 |
#103931) …egory This replaces the custom functions we wrote for each data category with a hook that accepts a list of data categories so we can automatically determine what the expected date page filter props should be. This sets it up so that we can replace the hook with an alternate implementation in order to support the coming retention changes. Same as #103822 but fixed typing issues in 17c69c7
…egory
This replaces the custom functions we wrote for each data category with a hook that accepts a list of data categories so we can automatically determine what the expected date page filter props should be. This sets it up so that we can replace the hook with an alternate implementation in order to support the coming retention changes.