Skip to content
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

Error rendering when using autolink (@customgroup) in mobile app (even in v2) #6451

Open
LouisSung opened this issue Jul 1, 2022 · 7 comments

Comments

@LouisSung
Copy link

LouisSung commented Jul 1, 2022

Summary

It's glad to hear the mobile v2.0 is under beta 🎉
However, this issue for custom group rendering does not seem to be fixed even in v2.0.0 (409)
Actually, I have been filed the issue for v1 as mattermost-community/mattermost-plugin-autolink#124 two years ago.

I guess maybe here is the better place to file the issue?

Sorry about not available to trace the code and pending that issue for such a long time, but I think maybe it's the suitable time to get this fixed.

Environment Information

  • Device Name: iPhone 12 Pro Max
  • OS Version: iOS 15.5
  • Mattermost App Version: Beta 2.0.0 (409)
  • Mattermost Server Version: 7.0.1 team-edition (supposed to be 5.2x enterprise-editon (without license) at that time)

Steps to reproduce

  • Set up a tmp @customgroup autolink as state out in Example 3
/autolink add tmp
/autolink set tmp Pattern @customgroup*
/autolink set tmp Template [@customgroup]( \\* @user1 @user2 \\* )

Expected behavior

Web and Desktop app can render correctly (also works with the auto followed of the newly introduced collapse reply threads)

Observed behavior (that appears unintentional)

Mobile app cannot render correctly

Possible fixes

Due to it's a rendering issue, I guess the most possible place it that the (\\* @user1 @user2 \\*) here as the herf is not a valid URL, making the !child.props.context.includes('link') passed and literally return child;.

Possible fix (if here is the problem): add a RegExp test to check if it's a valid custom group
(e.g., /^ *\\\\\* +( *@\w+ +)+\\\\\* *$/u.test(href))

However, I'm not familiar with React and the use of props, so I didn't trace further; besides, I have no build environment to set up my own mobile dev app.
I will keep tracing (or tack the implementation of Web app) and willing to create a PR if needed

Thank you for all your amazing works : )

@LouisSung
Copy link
Author

LouisSung commented Jul 1, 2022

Alright, seems like the mattermost-webapp just ignore everything and escape them as an http URL
That is, the if (!scheme) { outHref = `http://${outHref}`; } @utils/markdown/renderer.tsx#L127-L128

An acceptable solution except if there is any security concern I haven't though of 🤔

@enahum
Copy link
Contributor

enahum commented Jul 1, 2022

Hey @LouisSung thanks for reporting.. i just don't know if we ever going to address that issue as there is now a new custom groups feature that does not require the autolink plugin and the hack for the custom groups :)

@LouisSung
Copy link
Author

wow ok, glad to hear that :D
I see the doc for custom group and will make it a try
Just haven't caught up for a really long time and report the issue once I tried the mobile v2 beta app

I'll close this issue as there's already a better approach

@LouisSung
Copy link
Author

LouisSung commented Jul 2, 2022

Sorry for not taking closer look, the custom group feature seems limited to enterprise users (that why I didn't aware of)
Is this feature planned to land on Team Edition in the future?
If not, then I think this issue is still worth to resolve...

Although hacking with autolink is not the best and native choice, at least it works as is.
image

@LouisSung LouisSung reopened this Jul 2, 2022
@enahum
Copy link
Contributor

enahum commented Jul 2, 2022

I'm not sure about the enterprise or not enterprise version of the feature, but if you really really want the autolinking resolved, you can always submit a pull request for review :)

@LouisSung
Copy link
Author

That's reasonable, but I'm not sure if to align the implementation of Webapp (i.e., escape everything as http link) is safe enough in mobile app.
Should that be discussed in the PR (if there is)?

@enahum
Copy link
Contributor

enahum commented Jul 2, 2022

That's reasonable, but I'm not sure if to align the implementation of Webapp (i.e., escape everything as http link) is safe enough in mobile app.

Should that be discussed in the PR (if there is)?

Absolutely, we could diacuss it in the PR

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

No branches or pull requests

2 participants