Fix incorrect path in error overlay on Win #6679
Conversation
🦋 Changeset detectedLatest commit: 5f41cc5 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
|
Ah, it seems that the title of the file path element that is shown in the error overlay is supposed to be absolute while the text is supposed to be relative to the project root? That's how it is on Linux. Well, I fixed the issued with the double path on Windows so that now it at least shows a valid Windows path and the Should I work some more on this and make it show the relative path in the text content like it does on Linux? |
If you have time, that'd be awesome! We generally aim for parity. But since this is ultimately a minor detail, it's fine either way imo |
@Princesseuh That's done! I updated the test so that it will check that the relative path is displayed and the full path is used for opening the editor. |
|
I just discovered that while fixing the path on Windows when the error is in |
|
I fixed it and added another test to check the path when the error is in a component. |
| @@ -636,7 +636,8 @@ class ErrorOverlay extends HTMLElement { | |||
| const codeContent = code.querySelector<HTMLDivElement>('#code-content'); | |||
|
|
|||
| if (codeHeader) { | |||
| const cleanFile = err.loc.file.split('/').slice(-2).join('/'); | |||
| const separator = err.loc.file.includes('/') ? '/' : '\\'; | |||
There was a problem hiding this comment.
Account for Windows separators
|
|
||
| // Normalize the paths so that we can correctly detect if it's absolute on any platform | ||
| const normalizedFile = error.loc?.file?.replace(/\\/g, '/'); | ||
| const normalizedRootFolder = removeLeadingForwardSlashWindows(rootFolder?.pathname || ''); |
There was a problem hiding this comment.
Remove a leading forward slash which rootFolder.pathname comes with which is invalid on Windows (e.g. /C:/) and will always fail the check for the path being absolute below which results in the bug that is described in the issue.
| @@ -52,6 +52,28 @@ test.describe('Error display', () => { | |||
| expect(await page.locator('vite-error-overlay').count()).toEqual(0); | |||
| }); | |||
|
|
|||
| test('shows correct file path when a page has an error', async ({ page, astro }) => { | |||
There was a problem hiding this comment.
Two different tests because on Windows the path separators are different depending on the location of the error (i.e. component vs page).
|
@Princesseuh Thank you for the hint about the path utilities! I think I found the underlying cause and made the fix more robust. I added a comment for the less clear part and I added more explanations in the comments for the PR as I'm not sure which of those should be added to the code. |
Changes
Fix #6678.
Testing
An e2e test was added.
Docs
This just fixes a bug so no docs changes are needed.