Question Page Redesign – Binary + continuous body#4695
Open
ncarazon wants to merge 7 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:
📝 WalkthroughWalkthroughThis PR implements the "Question Page Redesign – Binary + continuous body" feature by reorganizing consumer and forecaster question layouts to support side-by-side rendering for binary questions with a histogram view toggle, introducing continuous-chart cursor state management for cursor-driven CP display overlays, adding full-aggregation fetching for enriched cursor forecasts, and refining CP reveal/hide logic throughout the component tree with consistent cursor-suppression patterns across chart containers. ChangesMain feature: Question page shell and layout redesign
Cursor-driven forecast display and aggregation enrichment
NumericChart cursor infrastructure and touch/continuous consumer enhancements
Histogram view toggle and detailed card cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
ncarazon
changed the title
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
…rt card & Timeline / Histogram toggle on binary bodies (#4692) * feat: add QuestionHeaderCPStatus next to timeline for consumer binary questions * fix: remove extra mt-8 top margin from consumer continuous timeline * feat: add Timeline/Histogram toggle to binary detailed chart cards and fix consumer binary radial padding * fix: resolve layout jumping between chart views, fix histogram full-width responsive scaling, and center binary radial charts on mobile * fix: prevent chart layout snapping with synchronous measurements, enable side-by-side continuous charts on sm breakpoints, and standardize spacing utility classes * fix: show fallback message when histogram tab has no data
* feat: live cursor updates left-panel value, range, and distribution for continuous consumer view, with dot indicator on timeline and no tooltip/dashed line in consumer mode * feat: animate consumer mobile mini chart on timeline cursor and fix mobile tooltip position tracking * fix: always serialize full forecast_values in aggregation history for cursor-time distribution animation * fix: memoize cursorChartData to skip cdfToPmf recalculation when forecast_values reference is unchanged * fix: resolve cursor dot color override through resolveToCssColor * refactor: lazily fetch full aggregation history from aggregation explorer on pointer enter for continuous consumer cursor animation, keeping posts endpoint lightweight * fix: pass include_bots_in_aggregates to aggregation explorer to match stored aggregation bot semantics, format aggregate_forecasts.py * fix: use stable setActiveForecast reference in effect deps to prevent infinite re-render loop on hover
* fix: handle hideCP, cpRevealsOn, and isEmpty in the side-by-side left card for binary and continuous questions * fix: handle isEmpty, cpRevealsOn, and hideCP states in the side-by-side layout left panel and fix title overflow onto the CP block * fix: refine mobile hidden-CP states for binary and continuous in the forecaster title row and left panel * fix: prevent CP status from being squeezed off-screen in the mobile title row * fix: restore "No forecasts yet" overlay in feed cards and suppress it on the question detail page * fix: wire hideCP, cpRevealsOn, and isEmpty states into all question-type left panels for the side-by-side layout * fix: replace hardcoded isEmpty:false with suppressEmptyOverlay prop, fix missing useMemo deps, and align binary cpRevealsOn state with continuous * fix: correct import order in continuous_question_prediction.tsx
ncarazon
force-pushed
the
feat/question-page-redesign-2nd-iteration
branch
from
df30316 to
3c52441
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 2
🧹 Nitpick comments (4)
front_end/src/app/(main)/questions/[id]/components/question_page_shell/index.tsx (1)
276-276: 💤 Low valueRemove redundant boolean literal in prop.
The
isConsumerViewprop is hardcoded totrue, which is redundant. If this prop is always true in this context, consider removing it or making it the default in the component definition.♻️ Simplify by removing the explicit boolean
- isConsumerView={true} + isConsumerViewOr verify if this prop can be removed entirely from
QuestionTimeline.🤖 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 276, The prop isConsumerView is being passed explicitly as the boolean literal true to QuestionTimeline (in QuestionPageShell / index.tsx); remove the redundant prop from the JSX or make isConsumerView default to true in the QuestionTimeline component (update its props/defaults or component signature) so callers don’t need to pass true—ensure any consumed logic inside QuestionTimeline continues to work when the prop is omitted by relying on the new default.front_end/src/contexts/continuous_chart_cursor_context.tsx (2)
44-45: ⚡ Quick winEnforce provider usage with a runtime guard.
The hook returns
CursorContextValue | null, which can silently fail if called outsideContinuousChartCursorProvider. Add a guard to catch misuse early in development.🛡️ Add provider requirement check
-export const useContinuousChartCursor = () => - useContext(ContinuousChartCursorContext); +export const useContinuousChartCursor = () => { + const context = useContext(ContinuousChartCursorContext); + if (!context) { + throw new Error( + "useContinuousChartCursor must be used within ContinuousChartCursorProvider" + ); + } + return context; +};🤖 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/contexts/continuous_chart_cursor_context.tsx` around lines 44 - 45, The hook useContinuousChartCursor currently returns the raw context which may be null when called outside ContinuousChartCursorProvider; add a runtime guard inside useContinuousChartCursor to read the context via useContext(ContinuousChartCursorContext) and throw a clear error (e.g., "useContinuousChartCursor must be used within a ContinuousChartCursorProvider") when the value is null/undefined so callers fail fast; update the hook to return the non-null CursorContextValue after the check to satisfy callers.
29-32: ⚡ Quick winRemove redundant
useCallbackwrapper.The
setActiveForecastsetter returned byuseStateis already stable and doesn't change across renders. Wrapping it inuseCallbackadds unnecessary overhead without benefit.♻️ Simplify by removing the wrapper
- const stableSet = useCallback( - (f: NumericAggregateForecast | null) => setActiveForecast(f), - [] - ); const value = useMemo( - () => ({ activeForecast, setActiveForecast: stableSet }), - [activeForecast, stableSet] + () => ({ activeForecast, setActiveForecast }), + [activeForecast] );🤖 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/contexts/continuous_chart_cursor_context.tsx` around lines 29 - 32, The stableSet wrapper around setActiveForecast is redundant because the setter from useState (setActiveForecast) is stable; remove the useCallback wrapper and assign stableSet directly to the state setter (e.g., const stableSet = setActiveForecast) or export/use setActiveForecast directly wherever stableSet is used, and then remove the now-unused useCallback import from continuous_chart_cursor_context.tsx; update any references to stableSet accordingly.front_end/src/app/(main)/questions/[id]/components/question_page_shell/title_row.tsx (1)
30-32: 💤 Low valueSimplify redundant nullish coalescing.
The expression
cursorCtx?.activeForecast ?? nullis redundant—optional chaining already returnsundefinedwhencursorCtxis null/undefined, andactiveForecastitself can benull. The?? nulladds no value.♻️ Simplify the expression
- const cursorForecast = cursorCtx?.activeForecast ?? null; + const cursorForecast = cursorCtx?.activeForecast ?? undefined;Or accept
null | undefineddownstream and avoid the coalescing entirely:- const cursorForecast = cursorCtx?.activeForecast ?? null; + const cursorForecast = cursorCtx?.activeForecast;🤖 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/title_row.tsx around lines 30 - 32, The expression cursorCtx?.activeForecast ?? null is redundant; change the assignment in title_row.tsx to use cursorCtx?.activeForecast directly (e.g., const cursorForecast = cursorCtx?.activeForecast) and update any downstream code that assumes strictly null (in functions/components referencing cursorForecast or props) to accept null | undefined instead so types remain correct; references: useContinuousChartCursor, cursorCtx, cursorForecast, and any consumers of cursorForecast.
🤖 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/messages/zh-TW.json`:
- Line 2169: The translation value for the key "noHistogramData" uses the
Mainland Chinese term "數據" rather than the zh-TW-preferred "資料"; update the
string value for "noHistogramData" from "沒有可用的直方圖數據" to "沒有可用的直方圖資料" so it
matches the rest of the locale's wording conventions.
In
`@front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx`:
- Around line 98-104: The rendered chart views are still reading from
question.aggregations and ignore the enrichedAggregation returned by
useFullAggregation, and the extra fetch is disabled for binary mode; update the
rendering logic to prefer enrichedAggregation when present: use
enrichedAggregation (or enrichedAggregation.aggregations/histogram fields)
instead of question.aggregations when isContinuous || isBinary and
shouldFetchFull is true (or enrichedAggregation !== null), ensure activeForecast
continues to use enrichedAggregation if available, and enable the
useFullAggregation call for binary mode by passing the appropriate condition
(replace isContinuous && shouldFetchFull with (isContinuous || isBinary) &&
shouldFetchFull or check enrichedAggregation nullity); adjust components that
read question.aggregations[...] (including histogram and timeline render paths
around activeForecast and noHistogramData) to fall back to question.aggregations
only if enrichedAggregation is null.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/question_page_shell/index.tsx:
- Line 276: The prop isConsumerView is being passed explicitly as the boolean
literal true to QuestionTimeline (in QuestionPageShell / index.tsx); remove the
redundant prop from the JSX or make isConsumerView default to true in the
QuestionTimeline component (update its props/defaults or component signature) so
callers don’t need to pass true—ensure any consumed logic inside
QuestionTimeline continues to work when the prop is omitted by relying on the
new default.
In
`@front_end/src/app/`(main)/questions/[id]/components/question_page_shell/title_row.tsx:
- Around line 30-32: The expression cursorCtx?.activeForecast ?? null is
redundant; change the assignment in title_row.tsx to use
cursorCtx?.activeForecast directly (e.g., const cursorForecast =
cursorCtx?.activeForecast) and update any downstream code that assumes strictly
null (in functions/components referencing cursorForecast or props) to accept
null | undefined instead so types remain correct; references:
useContinuousChartCursor, cursorCtx, cursorForecast, and any consumers of
cursorForecast.
In `@front_end/src/contexts/continuous_chart_cursor_context.tsx`:
- Around line 44-45: The hook useContinuousChartCursor currently returns the raw
context which may be null when called outside ContinuousChartCursorProvider; add
a runtime guard inside useContinuousChartCursor to read the context via
useContext(ContinuousChartCursorContext) and throw a clear error (e.g.,
"useContinuousChartCursor must be used within a ContinuousChartCursorProvider")
when the value is null/undefined so callers fail fast; update the hook to return
the non-null CursorContextValue after the check to satisfy callers.
- Around line 29-32: The stableSet wrapper around setActiveForecast is redundant
because the setter from useState (setActiveForecast) is stable; remove the
useCallback wrapper and assign stableSet directly to the state setter (e.g.,
const stableSet = setActiveForecast) or export/use setActiveForecast directly
wherever stableSet is used, and then remove the now-unused useCallback import
from continuous_chart_cursor_context.tsx; update any references to stableSet
accordingly.
🪄 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: 2373a095-5037-4217-9198-6b8132f59382
📒 Files selected for processing (30)
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/key_factors/key_factors_question_consumer_section.tsxfront_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_page_shell/title_row.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/fan_chart.tsxfront_end/src/components/charts/group_chart.tsxfront_end/src/components/charts/histogram.tsxfront_end/src/components/charts/multiple_choice_chart.tsxfront_end/src/components/charts/numeric_chart.tsxfront_end/src/components/charts/numeric_timeline.tsxfront_end/src/components/charts/primitives/chart_container.tsxfront_end/src/components/charts/primitives/line_cursor_points.tsxfront_end/src/components/consumer_post_card/binary_cp_bar.tsxfront_end/src/components/consumer_post_card/consumer_question_tile/consumer_continuous_tile.tsxfront_end/src/components/detailed_question_card/detailed_group_card/index.tsxfront_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsxfront_end/src/components/detailed_question_card/detailed_question_card/hooks/use_full_aggregation.tsfront_end/src/components/detailed_question_card/detailed_question_card/index.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.tsfront_end/src/hooks/use_container_size.ts
💤 Files with no reviewable changes (1)
- front_end/src/components/detailed_question_card/detailed_question_card/index.tsx
…rs; fix zh-TW locale wording for histogram empty state
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.
Closes #4642
Summary
This PR delivers the second iteration of the Question Page redesign by completing side-by-side prediction layouts for Consumer question views, refining hidden-CP and empty-state handling, and adding live cursor interactions for continuous charts.
Implemented in Iteration 2
UI / Interaction updates
Hidden CP / empty state handling
Layout fixes & polish
Functional improvements
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization