feat: add label to viewer #483
Conversation
|
Warning Rate limit exceeded@vishwa2710 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request enhances the Changes
Sequence DiagramsequenceDiagram
participant User
participant Viewer
participant CesiumEntities
User->>Viewer: add_target(position, label=optional)
alt Label provided
Viewer->>Viewer: add_label(position, label)
end
Viewer->>CesiumEntities: Create target/label entities
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bindings/python/test/test_viewer.py (2)
130-155: Consider adding negative tests for invalid label inputs.This test verifies the happy path but doesn't test scenarios where
labelis an empty string, excessively long string, orNone. Adding negative tests helps ensure robust handling of edge cases.
179-202: Remove leftover debug-related code.Lines 192–194 (importing IPython and embedding a shell) appear to be temporary debug statements. Consider removing these to keep the test file clean and free of interactive stubs.
- import IPython - - IPython.embed()bindings/python/tools/python/ostk/astrodynamics/viewer.py (1)
250-276: Use consistent naming for label parameters.The
add_labelfunction is straightforward, but consider naming thetextparameter “label_text” or “message” for consistency withadd_target's “label”. This reduces potential confusion.- def add_label( - self, - position: Position, - text: str, - size: int | None = None, - color: str | None = None, - ) -> None: + def add_label( + self, + position: Position, + label_text: str, + size: int | None = None, + color: str | None = None, + ) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bindings/python/test/test_viewer.py(1 hunks)bindings/python/tools/python/ostk/astrodynamics/viewer.py(4 hunks)
🔇 Additional comments (3)
bindings/python/test/test_viewer.py (1)
156-178: Good coverage for polyline addition.Adding multiple positions and verifying the rendered HTML is sufficiently tested here. The logic and asserts look solid. No immediate concerns.
bindings/python/tools/python/ostk/astrodynamics/viewer.py (2)
Line range hint
191-200: Docstring properly updated to reflect new label parameter.The parameter documentation appears consistent with the function's signature, clarifying usage of the
labelargument. Great work!
211-213: Sanitize or validate label to mitigate potential injection risks.Because
labelis directly placed within rendered HTML/JS, it’s prudent to sanitize or validate user-provided label content. This helps prevent cross-site scripting (XSS) or other injection vulnerabilities if untrusted input is passed in.
[security]Consider searching for all references to
add_targetcalls to ensure user-provided labels are from a trusted source:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
=======================================
Coverage 91.21% 91.21%
=======================================
Files 86 86
Lines 8720 8720
=======================================
Hits 7954 7954
Misses 766 766 ☔ View full report in Codecov by Sentry. |
| text (str): Label text. | ||
| size (int, optional): Label size. Defaults to None. | ||
| color (str, optional): Label color. Defaults to None. | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bindings/python/test/test_viewer.py (3)
131-154: Enhance test coverage for labeled targetsWhile the basic functionality is tested, consider adding the following test cases:
- Edge cases for the label parameter (empty string, special characters, very long text)
- Separate size controls for point and label scale
- Different colors for point and label
Example test cases to add:
def test_add_target_with_empty_label(self, viewer: Viewer): viewer.add_target( position=Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), size=10, color="red", label="" ) def test_add_target_with_special_chars_label(self, viewer: Viewer): viewer.add_target( position=Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), size=10, color="red", label="Test #1 & <special> chars" )
156-177: Expand line rendering test coverageThe current test only covers a basic line with two points. Consider adding tests for:
- Edge cases (single point, multiple points)
- Different line styles (dashed, dotted)
- Validation of coordinate transformations
Also, document the significance of the hardcoded size value (10.0) or make it more meaningful:
- size=10.0, + size=Length.pixels(2.0), # Standard line width for visibility
131-198: General test improvements neededConsider the following improvements to strengthen the test suite:
- Add docstrings to new test methods describing test scenarios
- Include negative test cases (invalid inputs, error conditions)
- Use pytest.mark.parametrize for testing multiple scenarios efficiently
Example parametrized test:
@pytest.mark.parametrize("position,label,size,color", [ (Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), "Test1", 10, "red"), (Position.meters([0.0, 0.0, 6371000.0], Frame.ITRF()), "Test2", 20, "blue"), ]) def test_add_target_with_label_parametrized(self, viewer: Viewer, position, label, size, color): viewer.add_target(position=position, size=size, color=color, label=label) rendered_html: str = viewer.render() # assertions
bindings/python/test/test_viewer.py
Outdated
| def test_add_label_success( | ||
| self, | ||
| viewer: Viewer, | ||
| ): | ||
| viewer.add_label( | ||
| position=Position.meters([6671000.0, 0.0, 0.0], Frame.ITRF()), | ||
| text="Hello, World!", | ||
| size=1.0, | ||
| color="red", | ||
| ) | ||
|
|
||
| rendered_html: str = viewer.render() | ||
|
|
||
| assert rendered_html.startswith('<meta charset="utf-8">') | ||
| assert "var widget = new Cesium.Viewer" in rendered_html | ||
| assert ( | ||
| 'widget.entities.add({position: Cesium.Cartesian3.fromDegrees(63.43494882292201, 18.22447811510915, -6376045.535225509), label: {text: "Hello, World!", fillColor: Cesium.Color.RED, scale: 10.0}});' | ||
| in rendered_html | ||
| ) | ||
| assert rendered_html.endswith("</script>") |
There was a problem hiding this comment.
Fix inconsistencies in label test
There are several issues in this test:
- The test sets
size=1.0but assertsscale: 10.0in the rendered HTML - The position coordinates and their rendered transformation appear inconsistent:
- Input:
[6671000.0, 0.0, 0.0] - Rendered:
-6376045.535225509(negative Z coordinate)
- Input:
Additionally, consider adding tests for:
- Label positioning relative to the point
- Text alignment options
- Label visibility at different zoom levels
Apply this fix for the size inconsistency:
- size=1.0,
+ size=10.0, # Match the expected scale in rendered HTMLCommittable suggestion skipped: line range outside the PR's diff.
vishwa2710
force-pushed
the
feat/add-label-to-viewer
branch
from
bb877fa to
40a7298
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bindings/python/test/test_viewer.py (1)
179-198:⚠️ Potential issueAddress label rendering inconsistencies and enhance test coverage
The test has several issues that need attention:
- Size inconsistency between input and rendered scale
- Position coordinates inconsistency between input and rendered transformation
- Missing tests for text alignment options
- No validation of multi-line labels or special characters
Apply this fix for the size and position inconsistencies:
- position=Position.meters([6671000.0, 0.0, 0.0], Frame.ITRF()), - size=1.0, + position=Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), # Use consistent position + size=10.0, # Match the expected scale in rendered HTMLAdditionally, add tests for:
def test_add_multiline_label_success(self, viewer: Viewer): viewer.add_label( position=Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), text="Hello\nWorld!", # Test multi-line text size=10.0, color="red", alignment="CENTER" # Test text alignment )
🧹 Nitpick comments (3)
bindings/python/test/test_viewer.py (3)
131-154: Enhance test coverage for labeled targetsThe test could be more comprehensive. Consider:
- Testing label positioning relative to the target
- Using a more realistic point size (e.g., 5-10 pixels instead of 123)
- Testing different label colors and text content
- Verifying label visibility at different zoom levels
- size=123, + size=8, # More realistic point size for visualizationAdditionally, consider adding test cases for:
def test_add_target_with_label_different_color(self, viewer: Viewer): viewer.add_target( position=Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), size=8, color="red", label="TEST", label_color="blue" # Different color for label )
156-177: Improve line rendering test coverageThe test could be enhanced in several ways:
- Test polylines with multiple segments (>2 points)
- Verify line visibility at different zoom levels
- Test different line styles (dashed, dotted)
- Test line width visibility thresholds
Consider adding test cases for:
def test_add_complex_line_success(self, viewer: Viewer): viewer.add_line( positions=[ Position.meters([1.0, 2.0, 3.0], Frame.ITRF()), Position.meters([4.0, 5.0, 6.0], Frame.ITRF()), Position.meters([7.0, 8.0, 9.0], Frame.ITRF()), ], size=2.0, # More realistic line width color="red", style="dashed" # Test different line styles )
131-198: Consider structural improvements for test maintainabilityTo improve test maintainability and reduce duplication:
- Add shared test constants for common values (positions, colors, sizes)
- Create helper methods for common assertions
- Consider using parameterized tests for different configurations
Example implementation:
# At the top of the test file TEST_POSITION = Position.meters([1.0, 2.0, 3.0], Frame.ITRF()) DEFAULT_POINT_SIZE = 8 DEFAULT_LINE_WIDTH = 2 def assert_rendered_entity(html: str, expected_position: str, entity_type: str): """Helper method to verify entity rendering""" assert f"widget.entities.add({{position: {expected_position}" in html if entity_type == "point": assert "point: {" in html elif entity_type == "label": assert "label: {" in html
Summary by CodeRabbit
New Features
Tests