Skip to content

Conversation

dprevost-LMI
Copy link
Contributor

@dprevost-LMI dprevost-LMI commented Oct 18, 2025

Summary:

As I point out in this issue, when integration reanimated, we end up with two debug targets having the same title. This can confuse and lead people to think something is wrong.

Adding the description alongside the title to better explain where the second debug target comes from eliminates the confusion.

Changelog:

[General] [Fixed] - Add description to target debugs to differentiate RNBridge and Reanimated debug targets

Test Plan:

Tested in this RN 82 project with the latest reanimated by overriding the code in the node_module

Before:
Screenshot 2025-10-18 at 10 52 18 AM

After:
Screenshot 2025-10-18 at 10 56 27 AM

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 18, 2025
@dprevost-LMI dprevost-LMI marked this pull request as ready for review October 18, 2025 15:24
@dprevost-LMI dprevost-LMI marked this pull request as draft October 18, 2025 15:25
@dprevost-LMI dprevost-LMI marked this pull request as ready for review October 18, 2025 15:29
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 18, 2025
Copy link

meta-codesync bot commented Oct 20, 2025

@cortinico has imported this pull request. If you are a Meta employee, you can view this in D85041647.

Comment on lines +110 to +111
({title, description}, i) =>
`${styleText(['white', 'inverse'], ` ${i + 1} `)} - "${title} (${description})"`,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could tweak this to disambiguate only when there are conflicting titles in the target list. This would prevent showing the verbose description field in the default case.

Suggested change
({title, description}, i) =>
`${styleText(['white', 'inverse'], ` ${i + 1} `)} - "${title} (${description})"`,
const hasDuplicateTitles =
new Set(targets.map(target => target.title)).size < targets.length;
this.#setTerminalMenu(
`Multiple debug targets available, please select:\n ${targets
.slice(0, 9)
.map(
({title, description}, i) =>
`${styleText(['white', 'inverse'], ` ${i + 1} `)} - "${title}"${hasDuplicateTitles ? ` (${description})` : ''}`,
)
.join('\n ')}`,
);

Copy link
Contributor Author

@dprevost-LMI dprevost-LMI Oct 20, 2025

Choose a reason for hiding this comment

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

I see that the PR was "imported", which I'm unsure what does that mean. I do not mind doing the above. @cortinico let me know if this is required and can be done

Copy link
Member

@huntie huntie Oct 20, 2025

Choose a reason for hiding this comment

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

Yeah this is feedback from me on this PR — it goes through a second review once imported. If you push changes here, we'll re-import :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yup basically what @huntie said

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants