Live cursor updates the side-by-side left card#4694
Live cursor updates the side-by-side left card#4694ncarazon wants to merge 5 commits intofeat/question-page-redesign-2nd-iterationfrom
Conversation
|
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:
✨ 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 |
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
ncarazon
changed the base branch from
feat/question-page-redesign-1st-iteration
to
feat/question-page-redesign-2nd-iteration
…or continuous consumer view, with dot indicator on timeline and no tooltip/dashed line in consumer mode
…obile tooltip position tracking
… cursor-time distribution animation
…cast_values reference is unchanged
ncarazon
force-pushed
the
feat/live-cursor-updates-the-side-by-side-left-card
branch
from
14a3222 to
fbf720b
Compare
| serialize_aggregate_forecast( | ||
| forecast, question.type, full=full_forecast_values | ||
| ) | ||
| serialize_aggregate_forecast(forecast, question.type, full=True) |
There was a problem hiding this comment.
For BE eng:
We also need forecast_values (CDF array) on every history entry to animate the distribution shape when hovering over the timeline.
Currently history entries are serialized with full=False, stripping it out.
Fix: changed full=full_forecast_values → full=True in the history loop.
Concern: this adds the CDF array to every history entry for all question types. Fine for continuous, but unnecessary overhead for binary/MC which don't use it. To prevent unnecessary payload increase, I think it should be gated to continuous question types only (Numeric, Discrete, Date). Would appreciate a help from BE eng on this one.
There was a problem hiding this comment.
I'd suggest: Dont change this endpoint and keep /posts/:id/ history lightweight.
On continuous question pages only, lazily fetch /aggregation_explorer/?question_id=...&aggregation_methods=(default) the first time the chart is interacted with, or prefetch it on pointer enter / idle.
Make sure the request matches the page’s default aggregation method and bot inclusion semantics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/charts/numeric_chart.tsx (1)
231-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
isContinuousConsumerViewtocontainerComponentmemo dependencies.
isContinuousConsumerViewis used in the memo body (line 263) but missing from the dependency array (lines 298-307). WhenisConsumerVieworquestionTypechanges, the cursor rendering mode won't update until another dependency changes.Suggested fix
}, [ defaultCursor, xScale, height, hasExternalTheme, getThemeColor, handleCursorChange, nonInteractive, isCursorActive, + isContinuousConsumerView, ]);🤖 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/charts/numeric_chart.tsx` around lines 231 - 307, The useMemo producing containerComponent references isContinuousConsumerView but it is not included in the dependency array, causing stale cursor rendering; update the dependency list for the useMemo that defines containerComponent to include isContinuousConsumerView (alongside defaultCursor, xScale, height, hasExternalTheme, getThemeColor, handleCursorChange, nonInteractive, isCursorActive) so the memo re-evaluates when isContinuousConsumerView changes.
🧹 Nitpick comments (2)
questions/serializers/aggregate_forecasts.py (1)
63-64: ⚡ Quick win
full_forecast_valuesis now a no-op; remove or deprecate it.With
historynow hardcoded tofull=True(Line 131) andlatestalready hardcodedfull=True(Line 135),full_forecast_valuesno longer changes behavior. Keeping it in the signature/docstring is misleading for callers.Suggested cleanup
def serialize_question_aggregations( question: Question, aggregate_forecasts: list[AggregateForecast], - full_forecast_values: bool = False, minimize: bool = True, ) -> dict:If backward compatibility is needed, keep the argument temporarily but mark it deprecated and ignored.
Also applies to: 130-133
🤖 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 `@questions/serializers/aggregate_forecasts.py` around lines 63 - 64, The parameter full_forecast_values in the function signature is now a no-op because history and latest are being constructed with full=True unconditionally; remove full_forecast_values from the signature and docstring and delete any references to it, or if you need backward compatibility keep the parameter but mark it deprecated (add a DeprecationWarning via warnings.warn when full_forecast_values is passed and ignore its value) and update the docstring to state it is deprecated/ignored; ensure any callers/tests are updated to stop relying on it and that any internal mentions (e.g., where history and latest are created with full=True) are left consistent.front_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/prediction/single_question_prediction/continuous_question_prediction.tsx (1)
43-54: ⚡ Quick winCursor area chart data derivation is duplicated across two components.
The
cursorForecastValuesextraction +useMemo(cdfToPmf(...))pattern is copy-pasted almost verbatim inquestion_header_cp_status.tsx. The only difference is theContinuousAreaTypeselection (that file includes"community_resolved"; this one does not, relying on the early return for resolved questions). A shared hook—e.g.,useCursorAreaChartData(cursorForecast, questionStatus)—would centralise the CDF→PMF conversion and make future changes easier.♻️ Suggested shared hook
// e.g. front_end/src/hooks/use_cursor_area_chart_data.ts import { useMemo } from "react"; import { ContinuousAreaGraphInput } from "@/components/charts/continuous_area_chart"; import { ContinuousAreaType } from "@/types/charts"; import { QuestionStatus } from "@/types/post"; import { NumericAggregateForecast } from "@/types/question"; import { cdfToPmf } from "@/utils/math"; export function useCursorAreaChartData( cursorForecast: NumericAggregateForecast | null | undefined, questionStatus: QuestionStatus ): ContinuousAreaGraphInput | null { const cursorForecastValues = cursorForecast?.forecast_values ?? null; return useMemo<ContinuousAreaGraphInput | null>(() => { if (!cursorForecastValues) return null; const type = ( questionStatus === QuestionStatus.RESOLVED ? "community_resolved" : questionStatus === QuestionStatus.CLOSED ? "community_closed" : "community" ) as ContinuousAreaType; return [{ pmf: cdfToPmf(cursorForecastValues), cdf: cursorForecastValues, type }]; }, [cursorForecastValues, questionStatus]); }🤖 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_view/consumer_question_view/prediction/single_question_prediction/continuous_question_prediction.tsx around lines 43 - 54, Extract the duplicated cursorForecastValues + useMemo(cdfToPmf(...)) logic into a shared hook named useCursorAreaChartData that accepts (cursorForecast, questionStatus) and returns ContinuousAreaGraphInput | null; inside the hook derive cursorForecastValues from cursorForecast?.forecast_values, memoize the conversion using cdfToPmf, and compute the ContinuousAreaType mapping (RESOLVED -> "community_resolved", CLOSED -> "community_closed", else "community"), then replace the duplicated logic in both continuous_question_prediction (where cursorForecastValues and useMemo are currently used) and question_header_cp_status by calling useCursorAreaChartData(cursorForecast, question.status). Ensure you reference ContinuousAreaGraphInput, ContinuousAreaType, cdfToPmf, and QuestionStatus types in the new hook.
🤖 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/components/charts/numeric_chart.tsx`:
- Around line 818-833: The VictoryScatter marker uses colorOverride directly and
currently passes String(colorOverride) which can serialize ThemeColor objects to
invalid CSS; instead resolve colorOverride with resolveToCssColor before passing
to VictoryScatter's fill: when computing the style.data.fill for VictoryScatter
(the component rendering [highlightedPoint]), call
resolveToCssColor(colorOverride) when colorOverride is present, otherwise fall
back to getThemeColor(colorPalette.chip); update the style block inside the
VictoryScatter rendering to use resolveToCssColor so it matches other uses in
this file.
In
`@front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx`:
- Around line 217-221: The current effect causes churn because including
activeForecast in dependencies runs cleanup that sets
cursorCtx.setActiveForecast(null) before every update; split into two effects:
(1) an effect that watches [activeForecast, question, cursorCtx] and only calls
cursorCtx.setActiveForecast(activeForecast) when isContinuousQuestion(question)
is true and calls cursorCtx.setActiveForecast(null) immediately if the question
becomes non-continuous, and (2) a mount/unmount effect that depends only on
cursorCtx and returns a cleanup that sets cursorCtx.setActiveForecast(null) on
unmount; keep using the existing isContinuousQuestion and
cursorCtx.setActiveForecast symbols.
---
Outside diff comments:
In `@front_end/src/components/charts/numeric_chart.tsx`:
- Around line 231-307: The useMemo producing containerComponent references
isContinuousConsumerView but it is not included in the dependency array, causing
stale cursor rendering; update the dependency list for the useMemo that defines
containerComponent to include isContinuousConsumerView (alongside defaultCursor,
xScale, height, hasExternalTheme, getThemeColor, handleCursorChange,
nonInteractive, isCursorActive) so the memo re-evaluates when
isContinuousConsumerView changes.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/question_view/consumer_question_view/prediction/single_question_prediction/continuous_question_prediction.tsx:
- Around line 43-54: Extract the duplicated cursorForecastValues +
useMemo(cdfToPmf(...)) logic into a shared hook named useCursorAreaChartData
that accepts (cursorForecast, questionStatus) and returns
ContinuousAreaGraphInput | null; inside the hook derive cursorForecastValues
from cursorForecast?.forecast_values, memoize the conversion using cdfToPmf, and
compute the ContinuousAreaType mapping (RESOLVED -> "community_resolved", CLOSED
-> "community_closed", else "community"), then replace the duplicated logic in
both continuous_question_prediction (where cursorForecastValues and useMemo are
currently used) and question_header_cp_status by calling
useCursorAreaChartData(cursorForecast, question.status). Ensure you reference
ContinuousAreaGraphInput, ContinuousAreaType, cdfToPmf, and QuestionStatus types
in the new hook.
In `@questions/serializers/aggregate_forecasts.py`:
- Around line 63-64: The parameter full_forecast_values in the function
signature is now a no-op because history and latest are being constructed with
full=True unconditionally; remove full_forecast_values from the signature and
docstring and delete any references to it, or if you need backward compatibility
keep the parameter but mark it deprecated (add a DeprecationWarning via
warnings.warn when full_forecast_values is passed and ignore its value) and
update the docstring to state it is deprecated/ignored; ensure any callers/tests
are updated to stop relying on it and that any internal mentions (e.g., where
history and latest are created with full=True) are left 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: 54f10cd1-4b21-4497-9afb-ebff6c012bf5
📒 Files selected for processing (12)
front_end/src/app/(main)/questions/[id]/components/question_page_shell/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/prediction/single_question_prediction/continuous_question_prediction.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsxfront_end/src/components/charts/numeric_chart.tsxfront_end/src/components/charts/numeric_timeline.tsxfront_end/src/components/charts/primitives/line_cursor_points.tsxfront_end/src/components/consumer_post_card/consumer_question_tile/consumer_continuous_tile.tsxfront_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsxfront_end/src/components/post_card/question_tile/continuous_cp_bar.tsxfront_end/src/contexts/continuous_chart_cursor_context.tsxfront_end/src/hooks/use_chart_tooltip.tsquestions/serializers/aggregate_forecasts.py
cemreinanc
left a comment
cemreinanc
left a comment
There was a problem hiding this comment.
LGTM on frontend, left a comment about BE.
hlbmtc
left a comment
hlbmtc
left a comment
There was a problem hiding this comment.
Blocking not to miss this for now
| serialize_aggregate_forecast( | ||
| forecast, question.type, full=full_forecast_values | ||
| ) | ||
| serialize_aggregate_forecast(forecast, question.type, full=True) |
There was a problem hiding this comment.
It's quite heavy change that negatively affects performance in many places. Need to revisit
3 participants
Related to #4642
This PR implements live cursor updates for the consumer view: hovering the timeline updates the left-panel value, range, and mini distribution chart in real-time. Applies to all consumer continuous charts. Binary timeline/radial is explicitly excluded. Live updates are scoped to consumer view only - forecaster left panel and mobile header remain static.
Implemented features & fixes
Consumer continuous left panel-QuestionHeaderCPStatusnow accepts acursorForecastprop; on hover, the center value, confidence interval, and minified area chart all animate to the cursor position; reverts to latest aggregate when cursor leavesConsumer continuous timeline- removed dashed cursor line and floating tooltip (replaced by left-panel live update); added a filled 8px dot on the CP line at cursor positionConsumer mobile mini chart-ContinuousQuestionPredictionnow reads cursor forecast fromContinuousChartCursorContextand updates both the center value display and the minified distribution chart in real-time on mobileContinuousChartCursorContext- new context scoped toConsumerShellthat bridges the timeline cursor forecast to the mobile mini chart without prop drilling;DetailedContinuousChartCardwrites,ContinuousQuestionPredictionreadsMobile tooltip position- floating tooltip now follows touch position in real-time viauseClientPointx/y override inuseChartTooltip; touch offset increased to 50px to clear the finger;onTouchEndadded to Victory chart events so cursor resets on touch liftForecaster continuous timeline- unchanged; dashed line, floating tooltip, and value label chip are preservedhideCursorValueLabel- added toNumericChart/NumericTimelineto suppress the on-chart value chip when the left panel is already showing the value (consumer continuous only)ContinuousCPBar- addedoverrideCenter/overrideBoundsprops so the value/range display can be driven by cursor data without mutating the underlying question objectdemo.mp4
Summary by CodeRabbit
New Features
Bug Fixes