Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Open in Editor: Add filter for short URL patterns + test#44475

Merged
vdavid merged 2 commits into
mainfrom
dv/open-in-editor-respect-repositorypathpattern
Nov 16, 2022
Merged

Open in Editor: Add filter for short URL patterns + test#44475
vdavid merged 2 commits into
mainfrom
dv/open-in-editor-respect-repositorypathpattern

Conversation

@vdavid

@vdavid vdavid commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

I've done some digging, and I don't see a truly elegant solution that I could deliver before tomorrow's branch cut.
The solution in this PR is not elegant, but it solves the current customer issue, and I find it unlikely that it'd break other cases.

My findings:

  • The repositoryPathPattern is not available on the front end (for a reason).
    • Only the modified repo URLs are available, that is, the results of applying the repositoryPathPattern on the original URLs.
    • repositoryPathPattern is a part of the code host connection settings (The config visible at /site-admin/external-services/{id}), and the whole thing is not accessible to the front end. This is expected because this raw information should probably be available for admins.
    • Without repositoryPathPattern, we can't reliably determine how many pieces we should cut off from the URL.
  • We could move the logic to the back end, and have the back end return an alternative path made for Open in Editor. This would probably involve introducing a new setting to the code host connection config (for each code host type), to set the number of pieces to cut off explicitly. Then the front-end logic would have no logic; it'd just use the URL provided.
    • I'm not confident that this would be a user-friendly solution; it's a bit too many settings, and I'm still wondering whether the cutoff point could be reliably calculated on the back end.

Test plan

I've added a new test that covers this case; the old test and the new one are all passing, so I think this should be fine.

App preview:

Check out the client app preview documentation to learn more.

@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Nov 16, 2022

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.02 kb) 0.00% (+0.02 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 9882089 and 0e68f41 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@philipp-spiess philipp-spiess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!! The test suite is really paying off here to have confidence that we don't break other tested branches. 💯

I think the logic makes sense 👍

@philipp-spiess

Copy link
Copy Markdown
Contributor

Maybe we want to add a change log entry for this though?

@vdavid

vdavid commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

@philipp-spiess Nice catch! The thing is, I've never added a changelog item before :embassarred_laugh: so it was totally not in my workflow. Added it now.

@philipp-spiess

Copy link
Copy Markdown
Contributor

@philipp-spiess Nice catch! The thing is, I've never added a changelog item before :embassarred_laugh: so it was totally not in my workflow. Added it now.

Haha yes it's not something that's naturally to me yet either 😬

@DaedalusG

Copy link
Copy Markdown
Contributor

Looks good to me @vdavid thanks for the quick fix!

@vdavid vdavid merged commit 62c8e8d into main Nov 16, 2022
@vdavid vdavid deleted the dv/open-in-editor-respect-repositorypathpattern branch November 16, 2022 20:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Open in Editor url doesn't consider repo name when repositorypathpattern is set for IntelliJ

4 participants