Skip to content

ROS: Refactor node#523

Merged
sgrizan-nv merged 1 commit into
mainfrom
sgrizan/branch2
May 21, 2026
Merged

ROS: Refactor node#523
sgrizan-nv merged 1 commit into
mainfrom
sgrizan/branch2

Conversation

@sgrizan-nv
Copy link
Copy Markdown
Collaborator

@sgrizan-nv sgrizan-nv commented May 13, 2026

Summary by CodeRabbit

  • Refactor
    • Consolidated shared utilities into a new teleop_ros2_node_lib module with dedicated sub-modules for asset resolution, geometry operations, message building, plugin discovery, and session configuration.
    • Restructured the main teleop node's message publishing into dedicated handler methods for improved modularity.
    • Removed duplicate local implementations and replaced them with library imports.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 96366698-ba5b-43e0-9648-3f9cd8f31fbf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request extracts extensive helper logic from teleop_ros2_node.py into a new teleop_ros2_node_lib package containing constants, asset resolution, geometry/message builders, plugin discovery, and session configuration assembly. The main node is refactored to import these utilities, parse teleop mode and hand-retargeter parameters into enums, resolve effective retargeter type via library helpers, and build the TeleopSessionConfig using a central dispatcher. Runtime message publishing is reorganized into dedicated _publish_* helper methods routed by teleop mode.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'ROS: Refactor node' is vague and generic, using a non-descriptive term ('Refactor') without clarifying the specific purpose or scope of the changes. Consider a more specific title that captures the main refactoring goal, such as 'ROS: Extract teleop_ros2_node utilities into shared library' or 'ROS: Refactor teleop_ros2_node to use session config builder'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sgrizan/branch2

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@examples/teleop_ros2/python/teleop_ros2_node_lib/messages.py`:
- Around line 35-38: The _as_quat function uses the wrong identity quaternion
ordering; update the fallback in _as_quat to match the codebase x,y,z,w ordering
by returning [0.0, 0.0, 0.0, 1.0] when ctrl.is_none, leaving the existing float
conversion for active controllers (function name: _as_quat, symbol: ctrl,
index).
🪄 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: Enterprise

Run ID: 3b0c5fa6-b6f1-4a29-9f94-4989502ad46e

📥 Commits

Reviewing files that changed from the base of the PR and between e4c1aa0 and 5f27195.

📒 Files selected for processing (8)
  • examples/teleop_ros2/python/teleop_ros2_node.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/__init__.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/assets.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/constants.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/geometry.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/messages.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/plugins.py
  • examples/teleop_ros2/python/teleop_ros2_node_lib/session_config.py

Comment thread examples/teleop_ros2/python/messages.py
@sgrizan-nv sgrizan-nv force-pushed the sgrizan/branch2 branch 2 times, most recently from c0a7436 to 99bd600 Compare May 21, 2026 19:54
@sgrizan-nv sgrizan-nv enabled auto-merge (squash) May 21, 2026 19:59
@sgrizan-nv sgrizan-nv merged commit b86d6c3 into main May 21, 2026
43 checks passed
@sgrizan-nv sgrizan-nv deleted the sgrizan/branch2 branch May 21, 2026 20:38
@github-project-automation github-project-automation Bot moved this from TODO to DONE in Isaac Teleop Planning May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: DONE

Development

Successfully merging this pull request may close these issues.

2 participants