Time series visualisation updates#4738
Open
ncarazon wants to merge 8 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
…over sync, and color-aligned time series visualisation for FanGraph
…ming issue that kept the tooltip invisible
…ses outside tap, no page shift on drag
…ger drag, and shows endpoint dots on all lines
ncarazon
force-pushed
the
feat/time-series-visualisation-updates-consumer-left-pane
branch
from
7dfc456 to
e1c20ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/consumer_post_card/time_series_chart/index.tsx (1)
396-449:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the real source index before filtering.
originalIndexis assigned after removing unsuccessfully resolved questions, so colorful mode shifts palette assignments whenever one of those questions is filtered out. That breaks cross-chart color alignment for every item after the gap.Suggested fix
- return [...questions] - .filter((question) => !isUnsuccessfullyResolved(question.resolution)) - .map((question, index) => { + return questions + .map((question, originalIndex) => ({ question, originalIndex })) + .filter(({ question }) => !isUnsuccessfullyResolved(question.resolution)) + .map(({ question, originalIndex }) => { const latest_centers = question.aggregations[question.default_aggregation_method].latest ?.centers?.[0]; @@ isClosed: question.status === QuestionStatus.CLOSED, resolution: question.resolution, isEmpty: !hasData, - originalIndex: index, + originalIndex, }; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/components/consumer_post_card/time_series_chart/index.tsx` around lines 396 - 449, The bug is that originalIndex is assigned after filtering out unsuccessfully resolved questions, shifting palette assignments; to fix, capture each question's original index before you call .filter by mapping the incoming questions to include an immutable originalIndex (e.g., attach a tmp property like __originalIndex using the index from the input array) and then run the existing .filter/.map chain against that list, and finally use that stored __originalIndex when setting originalIndex in the returned object (replace references to index in the current map with the preserved value); update references to originalIndex in this file (the mapping block that builds x/y/label/isClosed/etc.) to read the preserved property instead of the post-filter index.
🧹 Nitpick comments (1)
front_end/src/app/(main)/questions/[id]/components/question_page_shell/index.tsx (1)
305-305: ⚡ Quick winExtract magic number to named constant.
The hardcoded
slice(-4)appears twice without explanation. Extract it to a named constant to clarify intent and ensure consistency if this value needs to change.♻️ Proposed refactor
At the top of the file or in a constants file:
const MAX_VISIBLE_FAN_GRAPH_QUESTIONS = 4;Then use it at both call sites:
- <ConsumerTimeSeriesPane - questions={fanGraphQuestions.slice(-4)} - /> + <ConsumerTimeSeriesPane + questions={fanGraphQuestions.slice(-MAX_VISIBLE_FAN_GRAPH_QUESTIONS)} + />- visibleQuestions={fanGraphQuestions?.slice(-4)} + visibleQuestions={fanGraphQuestions?.slice(-MAX_VISIBLE_FAN_GRAPH_QUESTIONS)}Also applies to: 324-324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/app/`(main)/questions/[id]/components/question_page_shell/index.tsx at line 305, The two occurrences of fanGraphQuestions.slice(-4) are using a magic number; extract this into a named constant (e.g. MAX_VISIBLE_FAN_GRAPH_QUESTIONS = 4) declared at the top of the file or in a shared constants module, then replace both fanGraphQuestions.slice(-4) call sites in question_page_shell (the props passed as questions) with fanGraphQuestions.slice(-MAX_VISIBLE_FAN_GRAPH_QUESTIONS) to make intent explicit and keep the value consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/multiple_choices_chart_view/index.tsx:
- Around line 296-305: Add a handler for the touchcancel event to clear the
stored touch coordinates so mobile tooltips can't get stuck: in the same
component where onTouchStartCapture, onTouchMoveCapture, onTouchEndCapture,
onMouseMoveCapture and setTouchCoords are used, add an onTouchCancelCapture (or
onTouchCancel) that calls setTouchCoords(undefined) so touchCoords is always
cleared when the touch sequence is canceled.
In
`@front_end/src/app/`(main)/questions/[id]/components/question_view/consumer_question_view/consumer_time_series_pane.tsx:
- Around line 30-32: The onBarHover handler currently ignores null labels so the
hover state never clears; change the onBarHover prop implementation in
consumer_time_series_pane.tsx to call setHoveredChoiceName(label) for all values
(i.e., allow label to be null) so when TimeSeriesChart calls onBarHover(null)
the hovered state is cleared, and if the hoveredChoiceName state type doesn't
accept null update its declaration (or coerce to empty string) accordingly;
reference: onBarHover, setHoveredChoiceName, TimeSeriesChart.
In `@front_end/src/components/charts/group_chart.tsx`:
- Around line 405-475: Add an onTouchCancel handler next to
onTouchStartCapture/onTouchMoveCapture that mirrors the cleanup in onTouchEnd:
set inPlotRef.current = false, call setIsCursorActive(false), and
setLocalCursorTimestamp(null); also optionally invoke onCursorChange with the
last timestamp (timestamps.at(-1)) like onTouchEnd if you want identical
behavior — implement this within the same event props block (referencing
onTouchCancel, inPlotRef, setIsCursorActive, setLocalCursorTimestamp,
onCursorChange, timestamps).
- Around line 827-829: The pixel-to-timestamp conversion in pixelXToTimestamp
can produce NaN when plotWidth (computed as chartWidth - rightPadding -
leftPadding) is <= 0; add a guard that if plotWidth <= 0 (or if
Number.isNaN(plotWidth)), return a sensible fallback timestamp (for example
Number(xDomain[0]) or the midpoint of xDomain) instead of performing the ratio
math. Specifically, check plotWidth before computing ratio and only compute
ratio = Math.max(0, Math.min(1, (x - leftPadding) / plotWidth)) when plotWidth >
0; otherwise return the chosen fallback based on xDomain to avoid propagating
NaN.
In `@front_end/src/components/consumer_post_card/time_series_chart/index.tsx`:
- Around line 130-135: The highlight layer is only mounted when onBarHover is
provided, so externally setting hoveredBarLabel still resolves
effectiveHoveredIndex but never shows the highlight; update the mounting
condition to use effectiveHoveredIndex (or check onBarHover || hoveredBarLabel)
instead of only onBarHover so the highlight layer renders when
externalHoveredIndex/hoveredBarLabel is present; apply the same change to the
other occurrence noted (lines 255-265) and reference variables
adjustedChartData, externalHoveredIndex, hoveredIndex, effectiveHoveredIndex,
hoveredBarLabel and the onBarHover prop in the TimeSeriesChart component.
In `@front_end/src/hooks/use_chart_tooltip.ts`:
- Around line 30-31: The hoverActive state can become stale while forceOpen is
true because hover listeners are disabled; add a useEffect in
use_chart_tooltip.ts that watches forceOpen and calls setHoverActive(false)
whenever forceOpen becomes true (resetting hoverActive before entering forced
mode). Locate the hook where hoverActive and setHoverActive are defined
(useState) and add the useEffect referencing forceOpen to clear hoverActive,
ensuring isActive (computed as !!forceOpen || hoverActive) doesn’t stay true
after forced mode toggles off.
---
Outside diff comments:
In `@front_end/src/components/consumer_post_card/time_series_chart/index.tsx`:
- Around line 396-449: The bug is that originalIndex is assigned after filtering
out unsuccessfully resolved questions, shifting palette assignments; to fix,
capture each question's original index before you call .filter by mapping the
incoming questions to include an immutable originalIndex (e.g., attach a tmp
property like __originalIndex using the index from the input array) and then run
the existing .filter/.map chain against that list, and finally use that stored
__originalIndex when setting originalIndex in the returned object (replace
references to index in the current map with the preserved value); update
references to originalIndex in this file (the mapping block that builds
x/y/label/isClosed/etc.) to read the preserved property instead of the
post-filter index.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/question_page_shell/index.tsx:
- Line 305: The two occurrences of fanGraphQuestions.slice(-4) are using a magic
number; extract this into a named constant (e.g. MAX_VISIBLE_FAN_GRAPH_QUESTIONS
= 4) declared at the top of the file or in a shared constants module, then
replace both fanGraphQuestions.slice(-4) call sites in question_page_shell (the
props passed as questions) with
fanGraphQuestions.slice(-MAX_VISIBLE_FAN_GRAPH_QUESTIONS) to make intent
explicit and keep the value consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cba0aff2-0fd3-4eb6-899f-d6db310372f7
📒 Files selected for processing (19)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_page_shell/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/consumer_group_chart.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/consumer_list_chart_shell.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/consumer_time_series_pane.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/fan_graph_chart_panel.tsxfront_end/src/components/charts/fan_chart.tsxfront_end/src/components/charts/group_chart.tsxfront_end/src/components/charts/multiple_choice_chart.tsxfront_end/src/components/charts/primitives/chart_fan_tooltip.tsxfront_end/src/components/consumer_post_card/time_series_chart/index.tsxfront_end/src/components/consumer_post_card/time_series_chart/time_series_label.tsxfront_end/src/hooks/use_chart_tooltip.ts
…ht edge cases across charts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #4643
Summary
FanGraphChartPanel(new): Fan/Timeline toggle for FanGraph right pane, used in both consumer and forecaster shells.ConsumerTimeSeriesPane(new): wrapsTimeSeriesChartwith context hover sync (hoveredBarLabel/onBarHoverfromListChartExpandedContext).TimeSeriesChart: refactored to per-questionVictoryBarlayers for per-bar opacity control; newhoveredBarLabel/onBarHoverprops +HighlightBarcomponent for hover highlight.FanChart: newexternalHighlightedLabelprop dims non-hovered fan areas/lines when a bar is hovered in the left pane.QuestionPageShell: consumer FanGraph mountsConsumerTimeSeriesPane+FanGraphChartPanel; forecaster detects FanGraph and rendersFanGraphChartPanelinstead ofDetailedGroupCard.MultiChoicesChartView: fixnormalizedChartHeightinitialising to0; handle missinglegendContainerRef.fanChartkey added to all 6 locale files.Demo video
forecaster-consumer-time-series-updates.mp4
Summary by CodeRabbit
New Features
Documentation