✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

✅ PR preview is ready!

• Bind widgets to query params - Part 4 #13722
• Bind widgets to query params - Part 3 #13683
• Bind widgets to query params - Part 2 #13682 👈 (View in Graphite)
• Bind widgets to query params - Part 1 #13681
• develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

📉 Python coverage change detected

The Python unit test coverage has decreased by 0.0000%

• Current PR: 93.1073% (23329 statements, 1608 missed)
• Latest develop: 93.1073% (23329 statements, 1608 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.0100%

• Current PR: 86.4500% (13841 lines, 1875 missed)
• Latest develop: 86.4400% (13715 lines, 1859 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

@cursor review

Summary

Adds query param binding support in the frontend by syncing bound widget updates to the URL, preserving bound params across page navigation, and initializing App state from the current URL. Updates useBasicWidgetState to honor setValue for initial state and adjusts unit tests accordingly.

Code Quality

Encapsulation is clean: query-param logic lives in WidgetStateManager with small, testable helpers, and App only wires state + host messaging. The changes are readable and consistent with existing patterns.

Test Coverage

Strong unit coverage was added for query param binding and useBasicWidgetState, and App navigation tests were updated. No e2e tests were added; if this feature is user-facing, an e2e scenario would provide additional confidence.

Backwards Compatibility

Behavior should remain compatible provided the backend correctly sets setValue when seeding widget values. The new useBasicWidgetState logic relies on setValue rather than value != default.

Security & Risk

No new security concerns identified. URL updates are local and use URLSearchParams, reducing injection risk. Overall regression risk appears low.

Recommendations

1. Consider adding a focused e2e test that covers a bound widget and verifies query param preservation across page navigation (including embed params).

Verdict

APPROVED: Solid implementation with thorough unit tests; only optional e2e coverage is missing.

This is an automated AI review using gpt-5.2-codex-high. Please verify the feedback and use your judgment.

📈 Significant bundle size change detected

Please verify that this change is expected.

📊 View detailed bundle comparison

Summary

This PR adds frontend infrastructure for binding widgets to URL query parameters, enabling two-way synchronization between widget interactions and the browser's URL. Key changes include:

1. WidgetStateManager enhancements (WidgetStateManager.ts):

   • New binding registry (boundWidgets, paramKeyToWidgetId) for tracking widget-to-param relationships
   • URL sync methods (registerQueryParamBinding, filterParamsForPageChange, syncToUrlIfBound, etc.)
   • Default value detection to keep URLs clean when widgets return to default state
   • Index-to-string mapping for selection widgets

2. App integration (App.tsx):

   • Initialize queryParams from URL to preserve bound widget params from shared links
   • New handleQueryParamsFromWidget callback for URL change notifications
   • Modified page navigation to use filterParamsForPageChange for preserving bound widget params

3. Widget state hook improvements (useBasicWidgetState.ts):

   • Uses setValue flag to correctly initialize widget state from backend (URL-seeded or session_state values)
   • Relies on WidgetStateManager to persist values across React Strict Mode remounts

Code Quality

Strengths:

• Clean, well-structured implementation that follows existing codebase patterns
• Good separation of concerns between pure conversion logic and side-effect URL updates in WidgetStateManager
• Clear comments explaining the purpose of new methods
• Proper TypeScript types for WidgetValueType, QueryParamBinding

Code structure observations:

  const getDefaultState = useCallback<(wm: WidgetStateManager, el: P) => T>(
    (_wm, el) => {
      // Backend explicitly set a value (e.g., from URL params or session_state).
      // This handles both initial URL seeding and session_state updates.
      // On React Strict Mode remount, WidgetStateManager will have the value
      // (stored by the first mount's effect), so this path won't be reached.
      if (el.setValue) {
        return getCurrStateFromProto(el)
      }

      return getDefaultStateFromProto(el)
    },
    [getDefaultStateFromProto, getCurrStateFromProto]
  )

Good pattern using setValue as an explicit signal from the backend rather than inferring seeding from value !== default.

  private maybeSyncValueToUrl(
    widgetId: string,
    source: Source,
    value: unknown
  ): void {
    if (!source.fromUi) return

    const binding = this.boundWidgets.get(widgetId)
    if (!binding) return

    // Pure: Convert value to URL format
    const urlValue = this.convertToUrlValue(value, binding)

    // Pure: Check if we should clear the param
    const shouldClear = this.shouldClearUrlParam(urlValue, binding)

    // Side effect: Update URL
    this.updateUrlParam(
      binding.paramKey,
      shouldClear ? null : urlValue,
      binding.urlFormat
    )
  }

Clean separation of pure logic and side effects.

Minor notes:

• Multiple TODO comments about removing options after wire format changes (e.g., lines 73-75, 1205, 1219) - ensure these are tracked for cleanup in future PRs

Test Coverage

Excellent test coverage with 524+ new lines of tests for query param binding. The tests follow best practices:

• WidgetStateManager.test.ts: Comprehensive tests covering:

  • Registration/unregistration of bindings
  • URL sync for all scalar value types (bool, int, double, string)
  • URL sync for array values with different formats (repeated, comma)
  • Index-to-option string conversion for selection widgets
  • Default value detection and URL param clearing
  • Edge cases (null values, invalid values, no handler set)
  • filterParamsForPageChange preservation logic

• useBasicWidgetState.test.ts (new file): Tests the setValue behavior:

  • Uses currValue when setValue is true
  • Uses defaultValue when setValue is false
  • WidgetStateManager value takes precedence when available
  • Tests for arrays and numeric values

• App.test.tsx: Updated tests correctly verify:

  • Query params preservation during page navigation
  • Proper filtering and clearing of non-bound params
  • Uses Mock type from Vitest appropriately

Test pattern quality:

• Proper beforeEach/afterEach for setup/teardown
• Correct mocking of window.location and window.history.replaceState
• Good use of it.each for parameterized tests
• Includes both positive assertions and negative/anti-regression checks

Backwards Compatibility

The changes are backwards compatible:

• All new functionality is additive (new methods, new state fields)
• Existing widgets without query param bindings continue to work unchanged
• The setValue flag behavior in useBasicWidgetState is consistent with existing session_state update patterns
• No breaking changes to existing APIs

The URL initialization change in App.tsx:

queryParams: window.location?.search?.replace(/^\?/, "") ?? "",

Is safe as it only affects the initial state and preserves expected behavior for shared links.

Security & Risk

No security concerns identified:

• URL manipulation uses the standard window.history.replaceState API appropriately
• The query-string library is used correctly for URL encoding/decoding
• No user input is directly executed or stored unsafely
• Bound widget values come from the trusted backend

Low regression risk:

• Changes are well-isolated to specific code paths
• Unbound widgets are not affected by the new sync logic
• The fromUi check ensures only user-initiated changes sync to URL

Potential edge case to monitor:

• Rapid page navigation could theoretically read stale URL params in filterParamsForPageChange, but this is unlikely in practice and would self-correct on the next navigation.

Recommendations

1. Track TODO cleanup: The TODO comments about removing options normalization after wire format changes should be tracked in an issue to ensure they're addressed in a future PR.

2. E2E test consideration: The PR description mentions testing "via widgets in downstream PRs & with prototype." Consider adding an E2E test in the final PR of this feature to verify the complete URL-to-widget synchronization flow works end-to-end.

3. Documentation: Once the feature is complete across all PRs, consider adding developer documentation explaining the query param binding architecture for future maintainers.

Verdict

APPROVED: This PR provides well-structured, thoroughly tested frontend infrastructure for widget-to-URL query param binding. The code follows existing patterns, has comprehensive test coverage (500+ new lines), is backwards compatible, and introduces no security concerns. The implementation is clean and ready for merge.

This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.

Summary

This PR adds frontend infrastructure to bind widget values to URL query parameters (two-way-ish sync for UI → URL, plus preserving bound params during page navigation). The core work is in frontend/lib/src/WidgetStateManager.ts (binding registry + URL update helpers) and frontend/app/src/App.tsx (keeping App state and host communication in sync).

Code Quality

• Overall structure: The new API on WidgetStateManager is cohesive and reasonably separated into “pure” conversion logic vs URL side effects (convertToUrlValue / shouldClearUrlParam vs updateUrlParam / maybeSyncValueToUrl). The App integration (setQueryParamsChangeHandler) is minimal and keeps navigation logic centralized.

• Potential regression in setDoubleArrayValue early-return: In frontend/lib/src/WidgetStateManager.ts the new “filter invalid values” behavior returns early when all values are invalid:

  • setDoubleArrayValue filters and then returns without calling createWidgetState or onWidgetValueChanged when validValue.length === 0 (L607-L615).
  • This is risky because it changes semantics for any call path that can produce an “all invalid” array (including backend-originated updates where source.fromUi is false). Even though maybeSyncValueToUrl will no-op for fromUi: false, the function still returns early and skips updating WidgetStateManager state and skips triggering a rerun/update flush.
  • File reference: frontend/lib/src/WidgetStateManager.ts L607-L620.

• Duplicate paramKey handling: registerQueryParamBinding overwrites paramKeyToWidgetId for the paramKey but does not reconcile a previously-bound widget that used the same paramKey (the old widget remains in boundWidgets). If the backend ever allows duplicate keys (or transient duplication during remounts), this can lead to inconsistent state.

  • File reference: frontend/lib/src/WidgetStateManager.ts L1102-L1110.

• filterParamsForPageChange reads from window.location: This is a pragmatic choice, but it means navigation preservation is tied to the current URL rather than App state. That’s likely intended, but it’s worth noting as it can be surprising if URL state and App state diverge.

  • File reference: frontend/lib/src/WidgetStateManager.ts L1138-L1168 and frontend/app/src/App.tsx L1902-L1908.

Test Coverage

• Frontend unit tests: Good coverage was added for query-param binding mechanics in frontend/lib/src/WidgetStateManager.test.ts (scalar types, arrays with repeated vs comma format, default clearing, and fromUi gating). A new unit test file validates the setValue semantics in useBasicWidgetState (frontend/lib/src/hooks/useBasicWidgetState.test.ts).

• App-level tests: frontend/app/src/App.test.tsx was updated to assert SET_QUERY_PARAM calls more robustly (filtering by message type) during navigation (L2043-L2112, L5341-L5365).

• Gaps / missing edge-case coverage:

  • There’s no test that exercises the “all invalid values” path for setDoubleArrayValue with fromUi: false (backend-originated), which is the path most likely to cause a subtle state mismatch.
  • There’s no test that covers duplicate paramKey registration (two widgetIds bound to the same key) and the expected behavior (reject/overwrite/cleanup).

• E2E tests: None added. Given this PR is “infrastructure” and not full user-facing behavior yet, unit tests may be sufficient for now, but I’d expect an E2E test once a concrete widget binding ships (to validate URL updates + navigation across pages in a real browser).

Backwards Compatibility

• Mostly additive: New APIs on WidgetStateManager are additive and shouldn’t affect existing widgets unless bindings are registered.

• Behavioral change risk: The setDoubleArrayValue change (filter + early return) can change runtime behavior in cases where invalid values occur. Even if “invalid values” are not expected from the backend, this can cause hard-to-debug issues if any caller ever passes [NaN] / [null] / [undefined] (or mixed) and expects the widget state manager to update consistently.

  • File reference: frontend/lib/src/WidgetStateManager.ts L607-L615.

• setValue semantics: useBasicWidgetState now uses el.setValue as the explicit signal to use current proto value as the initial default (frontend/lib/src/hooks/useBasicWidgetState.ts L188-L200). This looks like the correct direction (avoids inferring seeding from value !== default), and is paired with tests.

Security & Risk

• No obvious security issues (no new network calls, no sensitive data handling).
• Regression risk is mainly around URL/query-string manipulation and state synchronization:
  • URL encoding/decoding correctness across repeated vs comma formats.
  • Ensuring backend-driven widget updates do not accidentally trigger URL updates (fromUi gating is good and tested).
  • The early-return in setDoubleArrayValue is the highest-risk change in this PR.

Recommendations

1. Fix setDoubleArrayValue “all invalid values” behavior so it does not skip state updates for backend-originated changes and does not silently drop updates. Concretely:
   • frontend/lib/src/WidgetStateManager.ts L607-L615: avoid unconditional early-return; consider:
     • Only early-return when source.fromUi is true and the intention is only to clear URL params, or
     • Store an empty DoubleArray (or delete the widget state) and still call onWidgetValueChanged so the rest of the pipeline remains consistent.
2. Add/adjust unit tests for the above:
   • Exercise the “all invalid values” path with fromUi: false and assert WidgetStateManager updates/flush behavior is correct (and that it does not update the URL).
3. Define behavior for duplicate paramKey bindings:
   • frontend/lib/src/WidgetStateManager.ts L1102-L1110: either prevent duplicates (throw/log + ignore) or explicitly overwrite and clean up the prior binding so boundWidgets/paramKeyToWidgetId stay consistent.

Verdict

CHANGES REQUESTED: The query-param binding infrastructure looks solid and well-tested overall, but setDoubleArrayValue’s “all invalid values” early return is a regression risk that should be addressed before merge.

This is an automated AI review using gpt-5.2-high. Please verify the feedback and use your judgment.

CHANGES REQUESTED: The query-param binding infrastructure looks solid and well-tested overall, but setDoubleArrayValue’s “all invalid values” early return is a regression risk that should be addressed before merge.

I added logging when all values are filtered out, but I'm not sure this poses a "regression risk" - invalid values can't come from the UI, and if the backend sends them it would indicate an unhandled edge case in user code.
I chose to preserve previous valid state rather than store an empty array because setDoubleArrayValue is only used by the Slider, which requires at least one value - storing empty would cause rendering issues.