Skip to content

Fix NodeInfoDialog initiation #626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 27, 2025

Conversation

philon-
Copy link
Contributor

@philon- philon- commented May 22, 2025

Description

This PR fixes an issue that was introduced in #618 where NodeInfoDialog was not opened using DialogManager

Related Issues

Changes Made

  • Write node.num to appStore instead of passing it to the component
  • Change onClick callback
  • Update tests

Testing Done

Screenshots (if applicable)

Checklist

  • Code follows project style guidelines
  • Tests have been added or updated
  • All CI checks pass

Additional Notes

image

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 20:52
Copy link

vercel bot commented May 22, 2025

@philon- is attempting to deploy a commit to the Meshtastic Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how the NodeInfoDialog is opened by moving the node number into the app store and using DialogManager instead of passing props, and updates the related dialog component and its tests.

  • Triggering the node details dialog now writes node.num to appStore and calls setDialogOpen instead of using component state
  • NodeDetailsDialog is updated to retrieve its node from useDevice().getNode(nodeNumDetails) and no longer takes a node prop
  • Tests for NodeDetailsDialog are revised to mock the new hook-based data flow

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/pages/Nodes.tsx Refactored node detail dialog trigger to use setNodeNumDetails and setDialogOpen
src/components/Dialog/NodeDetailsDialog/NodeDetailsDialog.tsx Converted dialog component to fetch its node via store hooks instead of props
src/components/Dialog/NodeDetailsDialog/NodeDetailsDialog.test.tsx Updated mocks and test cases to reflect the new hook-based data retrieval
Comments suppressed due to low confidence (2)

src/pages/Nodes.tsx:33

  • The hasNodeError variable is no longer used in this component after the refactor. Consider removing it to avoid unused destructuring.
const { getNodes, hardware, connection, hasNodeError, setDialogOpen } = useDevice();

src/components/Dialog/NodeDetailsDialog/NodeDetailsDialog.test.tsx:55

  • Add interaction tests for the dialog action buttons (favorite toggle, ignore toggle, request position, traceroute, remove node) to verify that each handler is wired up correctly.
it("renders node details correctly", () => {

Copy link

vercel bot commented May 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-test ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 22, 2025 11:12pm

@danditomaso danditomaso merged commit 37a53b7 into meshtastic:main May 27, 2025
4 checks passed
@philon- philon- deleted the fix/NodeDetailsDialog branch May 27, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants