[general] Lightning navigation

[toly11]

Overview
This pull request enhances navigation within Salesforce pages from the popup menu by integrating Lightning Navigation. Previously, clicking a link triggered a full page reload, which could take several seconds. Now, Lightning Navigation allows users to redirect to new locations without reloading the entire page. Should Lightning Navigation fail, the system gracefully falls back to standard navigation.

Additionally, this update maintains functionality for opening links in new tabs based on user settings, including detecting when users hold down the ctrl/command key to open a new tab.

During this update, I also addressed a bug where navigating to a custom settings object from the objects tab would inconsistently open the Object Manager instead of the intended custom settings page. Additionally, I fixed an issue where Object Manager setup URLs were not functioning properly for Knowledge objects

Testing
Extensive testing was conducted in various environments to ensure compatibility and seamless integration with the existing functionalities of Inspector. I encourage further testing to validate the efficiency and usability enhancements of this feature. All unit tests have been executed as per the guidelines outlined in our contribution guide.

Documentation
Changes have been documented in the CHANGES.md file.

Request for Review
I kindly request a review of this pull request. I am open to making any necessary adjustments based on your feedback.
Thank you.

Video before:

before.mp4

Video after:

after.mp4

[tprouvot]

Hi @toly11,
That sounds promising !
I have to check about the new permissions which may be too permissive for some users but I really like this new functionality 👏

[toly11]

Hi @tprouvot,
Thank you for reviewing this PR.
I can explore an option to communicate directly with the content script by posting a message to the popup's parent. This will not require the new permissions.

What do you think?

[tprouvot]

Hi @tprouvot, Thank you for reviewing this PR. I can explore an option to communicate directly with the content script by posting a message to the popup's parent. This will not require the new permissions.

What do you think?

Hi @toly11

I like this approach !

[toly11]

Ok @tprouvot,
I'll work on that and post when complete.

[toly11]

Hey @tprouvot,
I managed to achieve this new functionality by injecting a script into the DOM, not requiring the new permissions anymore
I also resolved conflicts with releaseCandidate

Kindly, please review

Thanks

[tprouvot]

The result looks great @toly11 👏
I checked if we could also use the navigation for internal shortcut links onDataSelect for AllDataBoxShortcut class and it seems to be OK, would you like to integrate it ?

[toly11]

The result looks great @toly11 👏 I checked if we could also use the navigation for internal shortcut links onDataSelect for AllDataBoxShortcut class and it seems to be OK, would you like to integrate it ?

Great to hear about the test results!

Regarding the suggestion, are you referring to the show all data link which navigates to an extension page?

[tprouvot]

No I'm talking about those links which can redirect to setups links such as Deployment Status

[tprouvot]

Few remarks concerning the inject.js script, today it is inserted in the page even if the user doesn't interact with the extension.
I would prefer to inject only when the user opens the popup, do you think it's doable ?

Would you be able to remove the lightningNavigate listener using the componentWillUnmount hook ?
Last thing, could you remove the debug logs and add some comment explaining why you created a inject.js file instead of scripting permission (basically to make the extension work with less permissions which can lead to company blacklisting it )?

[tprouvot]

Hi @toly11
Did you had a chance to see my previous comments ?

[toly11]

No I'm talking about those links which can redirect to setups links such as Deployment Status

Just got a chance to review this.
I’d be happy to add support for the shortcuts tab. However, currently the setup shortcuts always open in a new tab. Integrating Lightning Navigation should also consider the user’s settings for opening links in a new tab and whether they clicked Command/Control. If these conditions are not met, it should try to open with Lightning Navigation.

[toly11]

Few remarks concerning the inject.js script, today it is inserted in the page even if the user doesn't interact with the extension. I would prefer to inject only when the user opens the popup, do you think it's doable ?

Would you be able to remove the lightningNavigate listener using the componentWillUnmount hook ?

Good point. It will probably be better to add and remove the event listener directly in button.js in the openPopup, and closePopup functions, right? what do you tnink

Edit:
I looked into this option.

I tried injecting the inject.js script when the popup opens and removing it when it closes, alongside removing the event listener. While this is feasible, it introduces a delay of up to a second or two from the time appendChild is called until the script is fully loaded in the DOM. If a user clicks a link before the script is loaded and the event listener is initialized, navigation will not work.

Given this potential issue, I recommend keeping the current setup where the event listener is created even if the user hasn't opened the popup yet.

What do you think?

[toly11]

Last thing, could you remove the debug logs

Do you think we should also remove the debug when Lightning navigation fails? (addon/inject.js:19)

[toly11]

and add some comment explaining why you created a inject.js file instead of scripting permission (basically to make the extension work with less permissions which can lead to company blacklisting it )?

Sure. Where will it be bast to document this?

[tprouvot]

and add some comment explaining why you created a inject.js file instead of scripting permission (basically to make the extension work with less permissions which can lead to company blacklisting it )?

Sure. Where will it be bast to document this?

I think you can add a comment directly in the header file just to explain why it was done like this.

[tprouvot]

Last thing, could you remove the debug logs

Do you think we should also remove the debug when Lightning navigation fails? (addon/inject.js:19)

We can keep the log but use the .error method instead (suggestion done in the review)

[tprouvot]

I'm thinking about one think also, since we have an option to open the links in a new tab, we should mention that enabling this will prevent the lightning navigation to be used.

So I think we should add this info in the how-to page and add a tooltip in the option.js
Let me know if you want me to do it

[tprouvot]

Hi @toly11,

I've addressed the suggested action on how-to file and option page.

[toly11]

Hi @toly11,

I've addressed the suggested action on how-to file and option page.

Did you create a suggestion, or should I create a new commit?

[tprouvot]

Hi @toly11,
I've addressed the suggested action on how-to file and option page.

Did you create a suggestion, or should I create a new commit?

I screwed up my local branch, I'll push it as suggestions

[tprouvot]

Hi @toly11,
I could push my file, thank you for authorising the updates.

Last thing, I would like to have a setting which allows users to enable / disable this functionality.
I have experienced some latency from time to time where the lightning navigation is slower than classic (many tabs opened).

Now the question is : should we enabling it by default or not ?
By experience, things that are not enabled by default are not largely used. On the other hand I fear some users complaining about this feature that may make the navigation slower.

What we can do is to enabling it by default on the beta version and see if we have some complains.
What do you think ?

[tprouvot]

Hi @toly11,
Did you had a chance to see my previous comment ?