feat(analytics): add DecisionsScoreDistribution - #1207
Conversation
a86c90b to
e1a2835
Compare
050132c to
69ffa33
Compare
ChibiBlasphem
left a comment
There was a problem hiding this comment.
I didn't highlight them but can you also move out all the functions from Decisions which do not use local variables from it?
| Outcome, | ||
| RangeId, | ||
| } from '@app-builder/models/analytics'; | ||
| import { OUTCOME_COLORS } from '@app-builder/routes/_builder+/_analytics+/analytics.$scenarioId'; |
There was a problem hiding this comment.
This constant should IMO be part of the constants folder, taking it from the route file while being in a component will surely have a bad effect later.
| <TooltipV2.Provider> | ||
| <TooltipV2.Tooltip> | ||
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon icon="tip" className="size-5 text-grey-60 hover:text-purple-65 cursor-pointer" /> | ||
| </TooltipV2.TooltipTrigger> | ||
| <TooltipV2.TooltipContent> | ||
| <span className="font-normal">{t('analytics:rule_vs_decision_outcomes.tooltip')}</span> | ||
| </TooltipV2.TooltipContent> | ||
| </TooltipV2.Tooltip> | ||
| </TooltipV2.Provider> | ||
| </span> |
There was a problem hiding this comment.
This tooltip is repeated a lot of time while the only thing that changes is the content. Can it be extracted in a GraphTooltip component with only the content as a prop so it can be more maintainable?
| <TooltipV2.Provider> | ||
| <TooltipV2.Tooltip> | ||
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> | ||
| </TooltipV2.TooltipTrigger> | ||
| <TooltipV2.TooltipContent> | ||
| <span className="font-normal">{t('analytics:screening_hits.columns.execs.tooltip')}</span> | ||
| </TooltipV2.TooltipContent> | ||
| </TooltipV2.Tooltip> | ||
| </TooltipV2.Provider> |
| <TooltipV2.Provider> | ||
| <TooltipV2.Tooltip> | ||
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> | ||
| </TooltipV2.TooltipTrigger> | ||
| <TooltipV2.TooltipContent> | ||
| <span className="font-normal">{t('analytics:screening_hits.columns.hits.tooltip')}</span> | ||
| </TooltipV2.TooltipContent> | ||
| </TooltipV2.Tooltip> | ||
| </TooltipV2.Provider> |
| <TooltipV2.Provider> | ||
| <TooltipV2.Tooltip> | ||
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> | ||
| </TooltipV2.TooltipTrigger> | ||
| <TooltipV2.TooltipContent> | ||
| <span className="font-normal">{t('analytics:screening_hits.columns.hit_ratio.tooltip')}</span> | ||
| </TooltipV2.TooltipContent> | ||
| </TooltipV2.Tooltip> | ||
| </TooltipV2.Provider> |
| <TooltipV2.Provider> | ||
| <TooltipV2.Tooltip> | ||
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> | ||
| </TooltipV2.TooltipTrigger> | ||
| <TooltipV2.TooltipContent> | ||
| <span className="font-normal"> | ||
| {t('analytics:screening_hits.columns.avg_hits_per_screening.tooltip')} | ||
| </span> | ||
| </TooltipV2.TooltipContent> | ||
| </TooltipV2.Tooltip> | ||
| </TooltipV2.Provider> |
| export const OUTCOME_COLORS: Record<Outcome, string> = { | ||
| approve: '#46BB7F', | ||
| review: '#FDBD35', | ||
| blockAndReview: '#FF8533', | ||
| decline: '#DB5F4A', | ||
| }; |
There was a problem hiding this comment.
As previously said, it would be better to either move this to the constants folder or create a constant file located with the Analytics components and import it here. It's generally a bad idea to have file A importing from file B and file B importing from file A.
| // wait for a random time between 1 and 5 seconds | ||
| // const randomTime = Math.floor(Math.random() * 5000) + 1000; | ||
| // console.log('--------> randomTime', randomTime); | ||
| // await new Promise((resolve) => setTimeout(resolve, randomTime)); |
…en scenario IDs match
…ters based on priority index
…component across multiple analytics components
4c3ea17 to
13a68f7
Compare
…ant function and directly using formatNumber
4d52763 to
675ed06
Compare
| <AnalyticsTooltip content={t('analytics:rule_hits.columns.hit_count.tooltip')}> | ||
| <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> | ||
| </AnalyticsTooltip> |
There was a problem hiding this comment.
All the tooltips on the page use <Icon icon="tip" className="size-4 text-grey-60 hover:text-purple-65 cursor-pointer ml-v2-sm" /> can it not be directly set inside the component?
There's only one place where the class is size-5 instead of size-4, make the className of the icon customizable instead. Right now there's still too much code duplication in my opinion
…to streamline tooltip rendering and remove redundant Icon components
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| onMouseEnter={() => { | ||
| setIsHovered(true); | ||
| }} | ||
| > |
There was a problem hiding this comment.
Bug: Missing onMouseLeave handler causes stuck hover state
The isHovered state is set to true on onMouseEnter but never reset to false on onMouseLeave. This causes the hover state to persist indefinitely after the user moves their mouse over the component once, preventing the highlight animation from working correctly on subsequent hovers and potentially affecting UI behavior that depends on the hover state.
Additional Locations (1)
There was a problem hiding this comment.
This is intentional. We want the ping effect only once, the first time the user hover the component.
…r improved state management
| <TooltipV2.TooltipTrigger asChild> | ||
| <Icon | ||
| icon="tip" | ||
| className={cn('text-grey-60 text-purple-65 hover:text-grey-50 cursor-pointer ml-v2-sm', className)} |
There was a problem hiding this comment.
Bug: Conflicting Tailwind CSS text color classes
The className contains two conflicting text color classes text-grey-60 and text-purple-65. In Tailwind CSS, when multiple classes of the same property type are present, only the last one applies, so text-purple-65 overrides text-grey-60, making the first class ineffective.
✨ Improvements
🧹 UX & Behavior
Introduced a minimum 3px height for decisions in the Decisions graph to ensure visibility when outcomes are extremely small (e.g., < 1px).Logic is available in commit 67b0276.
Note
Adds Decisions Score Distribution, centralized outcome colors, contextual tooltips, restored comparison date range, and refactors queries/filters UI with multiple UX improvements.
constants/analytics.tsand use inDecisions,OutcomeFilter,RuleVsDecisionOutcomes.AnalyticsTooltipcomponent; add tooltip icons/descriptions to charts and table headers.OutcomeFilter.DecisionsScoreDistributionpanel to layout; resize main/side columns.compareRangefilter in FiltersBar; keep instant updates forscenarioId/date ranges.queries/analytics/get-data.ts; add endpoints fordecisions-score-distributionand wire server route handler.triggerwhen scenario unchanged; remove stray logging.RulesHitandScreeningHits: header tooltips; percentage formatting adjustments.Tableheader markup refinement for header content container.animate-ping-oncefor highlight effect.Written by Cursor Bugbot for commit 8e49872. This will update automatically on new commits. Configure here.