✅ 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!

📈 Frontend coverage change detected

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

• Current PR: 86.3800% (12658 lines, 1723 missed)
• Latest develop: 86.3500% (12616 lines, 1721 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison

@cursor review

Summary

This PR adds a new feature to st.json that displays a tooltip with the JSON path when a user clicks on a value in the rendered JSON. The tooltip includes a copy button to easily copy the path to the clipboard. This addresses issue #13057.

Key changes:

• New useJsonTooltip hook to manage tooltip state and mouse position tracking
• New JsonPathTooltip component using BaseWeb's Popover for consistent positioning
• formatJsonPath utility function that converts react-json-view's namespace array into a valid JSON path string
• Integration with the existing Json component via the onSelect callback from react-json-view
• Comprehensive unit tests and E2E tests with visual snapshots

Code Quality

Strengths:

• ✅ Excellent separation of concerns: the hook manages state logic, the component handles rendering
• ✅ Proper use of memo on both Json and JsonPathTooltip components for performance
• ✅ useCallback is used appropriately for referential stability of handlers
• ✅ The useEffect in useJsonTooltip.ts (lines 83-90) correctly synchronizes with external DOM events and properly cleans up the listener
• ✅ The formatJsonPath function (lines 46-71 in useJsonTooltip.ts) handles edge cases thoroughly: array indices, special characters, empty strings, escaping quotes/backslashes
• ✅ Styled components follow the project convention with Styled prefix
• ✅ Uses theme values from useEmotionTheme for consistent styling
• ✅ Good TypeScript typing with explicit return types and interfaces

Minor observations:

1. Potentially redundant state (JsonPathTooltip.tsx, line 48): The isOpen state is initialized to true and only transitions to false on close. Since the JsonPathTooltip component is only rendered when tooltip is truthy in the parent, the isOpen state could theoretically be eliminated. However, BaseWeb's Popover likely needs this for proper animation/transition behavior, so this is acceptable.

2. CSS class selector in E2E test (st_json_test.py, line 79): The test uses .locator(".string-value") which depends on react-json-view's internal class names. This is fragile but acceptable since there's no better way to target specific JSON values in the third-party component.

Test Coverage

Unit Tests:

• ✅ useJsonTooltip.test.ts: Excellent parameterized tests for formatJsonPath covering 20+ edge cases (empty namespace, array indices, special characters, escaping, null values)
• ✅ Hook tests cover: initialization, event listener setup/cleanup, state updates, and mouse position tracking
• ✅ JsonPathTooltip.test.tsx: Tests rendering, test ID, and copy button presence

Potential improvements (non-blocking):

• Consider adding a test for the copy button click functionality in JsonPathTooltip.test.tsx
• Consider testing the onEsc callback behavior

E2E Tests:

• ✅ test_shows_json_path_tooltip_on_click properly tests the full user flow
• ✅ Uses themed_app fixture for both light and dark theme coverage
• ✅ Visual snapshots added for all three browsers (chromium, firefox, webkit) in both themes
• ✅ Uses expect(tooltip).to_be_visible() with auto-wait assertions as recommended
• ✅ Follows naming convention: st_json-path_tooltip
• ✅ Screenshots the tooltip element specifically (not the whole page) as recommended

Backwards Compatibility

No breaking changes identified:

• The onSelect prop of react-json-view was previously unused, now it's wired up to show the tooltip
• This is purely additive behavior - existing functionality is preserved
• No changes to the Python API or protobuf definitions
• No changes to existing test assertions

Behavioral consideration:

• Users who previously clicked on JSON values (perhaps accidentally) won't notice any difference since the tooltip is easily dismissible by clicking elsewhere or pressing Escape.

Security & Risk

No security concerns:

• The JSON path is derived from the parsed JSON structure, not from arbitrary user input
• The path is displayed as text (not executed) and only copied to clipboard on explicit user action
• No XSS vectors since React escapes content in JSX by default
• The document.addEventListener for mousedown is properly cleaned up on unmount

Low regression risk:

• The feature is well-isolated to the Json component
• The hook and tooltip component are independent modules with their own tests
• The integration point (onSelect callback) is a standard API from react-json-view

Recommendations

No blocking issues found. The following are minor suggestions for future consideration:

1. Documentation: Consider adding a brief mention in the st.json docstring that clicking on values shows the path (if this behavior becomes user-facing documentation).

2. Accessibility: The tooltip is properly using ACCESSIBILITY_TYPE.tooltip from BaseWeb. Consider ensuring the copy button has proper focus management for keyboard users.

3. Test enhancement: The JsonPathTooltip.test.tsx could be expanded to test the copy functionality using userEvent to simulate button clicks.

Verdict

APPROVED: This PR implements a useful feature with clean, well-tested code that follows Streamlit's coding conventions. The changes are additive with no breaking changes, comprehensive test coverage including E2E visual tests, and proper cleanup of event listeners. Ready for merge.

This is an automated AI review. Please verify the feedback and use your judgment.