-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: remove useInsightsParameters() #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: eval-pr-25020-target-1763486137736
Are you sure you want to change the base?
refactor: remove useInsightsParameters() #11
Conversation
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
@sanxroz Sorry, I encountered an error while trying to start the review. Please try again later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 3 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/features/insights/hooks/useInsightsRoutingParameters.ts">
<violation number="1" location="packages/features/insights/hooks/useInsightsRoutingParameters.ts:14">
This refactoring breaks encapsulation by removing timezone logic from `useInsightsRoutingParameters` and shifting responsibility to the UI component. Any component using this hook is now implicitly dependent on being rendered with a `DateRangeFilter` that has the `convertToTimeZone: true` option. Since only one of the many consuming components was updated, this is an incomplete refactor that will likely cause incorrect data to be displayed in other insight components.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }, [createdAtRange?.endDate]) | ||
| ); | ||
|
|
||
| const startDate = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactoring breaks encapsulation by removing timezone logic from useInsightsRoutingParameters and shifting responsibility to the UI component. Any component using this hook is now implicitly dependent on being rendered with a DateRangeFilter that has the convertToTimeZone: true option. Since only one of the many consuming components was updated, this is an incomplete refactor that will likely cause incorrect data to be displayed in other insight components.
Prompt for AI agents
Address the following comment on packages/features/insights/hooks/useInsightsRoutingParameters.ts at line 14:
<comment>This refactoring breaks encapsulation by removing timezone logic from `useInsightsRoutingParameters` and shifting responsibility to the UI component. Any component using this hook is now implicitly dependent on being rendered with a `DateRangeFilter` that has the `convertToTimeZone: true` option. Since only one of the many consuming components was updated, this is an incomplete refactor that will likely cause incorrect data to be displayed in other insight components.</comment>
<file context>
@@ -11,23 +10,18 @@ export function useInsightsRoutingParameters() {
- }, [createdAtRange?.endDate])
- );
+
+ const startDate = useMemo(() => {
+ return dayjs(createdAtRange?.startDate ?? getDefaultStartDate().toISOString())
+ .startOf("day")
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: System Design Agent
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
This PR removes the usage of useInsightsParameters() from /insights/routing and uses useInsightsRoutingParameters() instead.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
nothing changes on /insights/routing
Summary by cubic
Refactored Insights Routing to remove useInsightsParameters and rely on useInsightsRoutingParameters/useInsightsOrgTeams. Behavior stays the same and date handling is centralized.
Written for commit 7a49f7a. Summary will update automatically on new commits.