Refactor: Improve readability and maintainability of Crane Visualizer panel #27
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… panel.
This commit applies a series of refactorings to the crane visualizer panel
to enhance code clarity, structure, and ease of future maintenance.
Key changes include:
1. **SVG Interaction Hook:**
* Pan and zoom logic extracted from `CraneVisualizer` into a reusable
`usePanZoom` custom hook (`src/hooks/usePanZoom.ts`).
2. **Centralized Configuration Management:**
* A `usePanelConfig` custom hook (`src/hooks/usePanelConfig.ts`) now
manages the panel's configuration state (background color, viewBox
width, namespace visibility).
* Improved type definitions for `PanelConfig` and `NamespaceConfig`.
3. **Settings Panel Utilities:**
* Logic for creating settings fields (`createNamespaceFields`) and handling
settings actions (`handleSettingsAction`) moved to `src/settings_utils.ts`.
* Shared types (`PanelConfig`, `NamespaceConfig`) also centralized here.
4. **Readability Enhancements in `CraneVisualizer`:**
* Introduced constants for default values (e.g., `DEFAULT_TOPIC`,
`DEFAULT_VIEWBOX_ASPECT_RATIO`) in `src/constants.ts`.
* Translated Japanese comments to English and added new comments to
clarify complex logic and hook dependencies.
* Minor JSX refactoring (e.g., `InfoDisplay` component).
* Standardized variable names (e.g., `recv_num` to `receivedMessageCount`).
5. **Code Cleanup:**
* Removed unused variables and imports.
* Replaced magic numbers with named constants (e.g., in `usePanZoom`).
* Refined React imports for specificity.
* Removed the unused `handleCheckboxChange` function from `CraneVisualizer`
as namespace visibility is now solely managed by the settings panel.
These changes aim to make the codebase more modular, easier to understand,
and more robust for future development.
This commit addresses several issues that caused the CI build to fail:
1. **Corrected Module Import Paths:**
* I changed import paths in `src/crane_visualizer_panel.tsx` for
`settings_utils.ts` and `constants.ts` from `../` to `./` to
correctly resolve modules within the `src` directory.
2. **Addressed React UMD Global Errors:**
* I added `import React from 'react';` to `src/crane_visualizer_panel.tsx`
(merging with existing named React imports). This ensures `React` is
in scope for JSX compilation, as required by the `"jsx": "react"`
setting in `tsconfig.json`.
3. **Fixed `unsubscribe` TypeError and Logic:**
* I corrected the argument passed to `context.unsubscribe` in the topic
subscription `useEffect` hook in `src/crane_visualizer_panel.tsx`.
It now correctly passes an array of topic strings (`[topic]`)
instead of an array of objects (`[{ topic }]`), aligning with the
expected signature for unsubscribing by subscription ID.
This also fixes a potential memory leak where previous topics were
not being unsubscribed upon topic change.
These changes should resolve the CI build errors and ensure the panel
builds successfully.
This commit changes the topic unsubscription logic in the `CraneVisualizer` panel to use `context.unsubscribeAll()` instead of `context.unsubscribe([topicId])`. This change is implemented as a workaround for a persistent TypeScript type error in the CI environment where `PanelExtensionContext.unsubscribe` was reported as not existing, despite the `@foxglove/studio` version being up-to-date. Using `unsubscribeAll()` provides a more general cleanup mechanism. Given that this panel primarily manages a single dynamic topic subscription at a time, this approach should be functionally equivalent for ensuring resources are released and should avoid the problematic type error in CI. The previous attempts to fix this involved: - Correcting the arguments to `context.unsubscribe()`. - Adding explicit type assertions for the arguments. When these did not resolve the CI error message "Property 'unsubscribe' does not exist on type 'PanelExtensionContext'", this more robust fallback was chosen.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit applies a series of refactorings to the crane visualizer panel to enhance code clarity, structure, and ease of future maintenance.
Key changes include:
SVG Interaction Hook:
CraneVisualizerinto a reusableusePanZoomcustom hook (src/hooks/usePanZoom.ts).Centralized Configuration Management:
usePanelConfigcustom hook (src/hooks/usePanelConfig.ts) nowmanages the panel's configuration state (background color, viewBox
width, namespace visibility).
PanelConfigandNamespaceConfig.Settings Panel Utilities:
createNamespaceFields) and handlingsettings actions (
handleSettingsAction) moved tosrc/settings_utils.ts.PanelConfig,NamespaceConfig) also centralized here.Readability Enhancements in
CraneVisualizer:DEFAULT_TOPIC,DEFAULT_VIEWBOX_ASPECT_RATIO) insrc/constants.ts.clarify complex logic and hook dependencies.
InfoDisplaycomponent).recv_numtoreceivedMessageCount).Code Cleanup:
usePanZoom).handleCheckboxChangefunction fromCraneVisualizeras namespace visibility is now solely managed by the settings panel.
These changes aim to make the codebase more modular, easier to understand, and more robust for future development.