Fix CSP eval and analytics violations#5008
Conversation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
front_end/src/utils/forecasts/__tests__/switch_forecast_type.test.ts (1)
84-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the wide
maxErrortolerances weaken regression sensitivity.For the skewed and wide-uncertainty rows,
maxErrorof0.1–0.25means the median/quartiles may drift by up to 25% of CDF space and still pass. This reflects the bounded slider range's fit limitations, but it also lets meaningful accuracy regressions slip through. If tighter fits are achievable, consider snugging these tolerances or adding an assertion on the total scored error.🤖 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/utils/forecasts/__tests__/switch_forecast_type.test.ts` around lines 84 - 104, Improve regression sensitivity in the parameterized “returns a valid slider distribution near the input quartiles” test by tightening the 0.1–0.25 maxError values for skewed and wide-uncertainty cases where feasible, and add an assertion on aggregate quartile error if individual tolerances must remain broad. Keep the assertions aligned with the bounded-range behavior of convertToSliderQuartiles.front_end/src/utils/forecasts/switch_forecast_type.ts (1)
106-125: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: bound the synchronous cost of the search.
Each
SEARCH_RADIIpass evaluates5^3 = 125candidates, so this runsscoreSliderParams~625 times, and every call rebuilds a CDF overinbound_outcome_countpoints viagetUserContinuousQuartiles. This executes synchronously on the interaction thread when the user switches to slider mode. Ifinbound_outcome_countcan be large in production, consider early-exit once the score is within tolerance, or memoizing per candidate, to avoid UI jank.🤖 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/utils/forecasts/switch_forecast_type.ts` around lines 106 - 125, Bound the synchronous optimization cost in the slider-search logic around SEARCH_RADII and scoreSliderParams: add a suitable score-tolerance early exit and/or memoize scores for repeated candidates, while preserving the existing best-candidate behavior. Ensure the search cannot cause noticeable UI blocking when inbound_outcome_count is large.
🤖 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.
Nitpick comments:
In `@front_end/src/utils/forecasts/__tests__/switch_forecast_type.test.ts`:
- Around line 84-104: Improve regression sensitivity in the parameterized
“returns a valid slider distribution near the input quartiles” test by
tightening the 0.1–0.25 maxError values for skewed and wide-uncertainty cases
where feasible, and add an assertion on aggregate quartile error if individual
tolerances must remain broad. Keep the assertions aligned with the bounded-range
behavior of convertToSliderQuartiles.
In `@front_end/src/utils/forecasts/switch_forecast_type.ts`:
- Around line 106-125: Bound the synchronous optimization cost in the
slider-search logic around SEARCH_RADII and scoreSliderParams: add a suitable
score-tolerance early exit and/or memoize scores for repeated candidates, while
preserving the existing best-candidate behavior. Ensure the search cannot cause
noticeable UI blocking when inbound_outcome_count is large.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae7c74b5-bf41-489d-a11e-653ce9f60141
⛔ Files ignored due to path filters (1)
front_end/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
front_end/package.jsonfront_end/src/utils/csp.tsfront_end/src/utils/forecasts/__tests__/switch_forecast_type.test.tsfront_end/src/utils/forecasts/switch_forecast_type.ts
💤 Files with no reviewable changes (1)
- front_end/package.json
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
| }; | ||
| let bestScore = scoreSliderParams(best, question, q1, q2, q3); | ||
|
|
||
| for (const radius of SEARCH_RADII) { |
There was a problem hiding this comment.
Could we add an open-bound case here and tighten the search before merging? On a both-open 0–100 question with quartiles 5/65/95, this converts back to 14.14/52.20/99.43. The old optimizer came back at 6.86/60.66/99.21, so switching input modes can now move a valid forecast quite a lot. This feels risky for a core forecasting interaction.
| center: clamp01(best.center + centerOffset), | ||
| right: clamp01(best.right + rightOffset), | ||
| }; | ||
| const score = scoreSliderParams(candidate, question, q1, q2, q3); |
There was a problem hiding this comment.
This search is also seems fairly expensive for something that runs synchronously in the forecast UI. It scores 626 candidates, and each score rebuilds and standardizes the full CDF. Did we test the impact on UI and see any problems especially on group questions?
2 participants
Summary
Fixes CSP Report-Only violations found after the rollout: Google Analytics was reporting blocked beacon requests, and forecast table-to-slider conversion was triggering
unsafe-evalthroughnumeric.uncmin.Implemented changes:
utils/csp.ts: expandedconnect-srcto allow confirmed Google Analytics collection endpoints, including regional*.google-analytics.comandwww.google.combeacon URLs.utils/forecasts/switch_forecast_type.ts: replacednumeric.uncminwith an in-house bounded search for converting table quantiles into slider distribution parameters, removing the eval-based dependency path.utils/forecasts/__tests__/switch_forecast_type.test.ts: added coverage for table-to-slider conversion across balanced, clustered, skewed, and wide-uncertainty quantile inputs.package.json/bun.lock: removed the unusednumericdependency after replacing the only runtime usage, so it can no longer be bundled and trigger eval-based CSP reports.Note: This PR fixes some of the most noisy alerts. Other CSP violations will be fixed on the following PRs.
Summary by CodeRabbit
Improvements
Bug Fixes