Gracefully handle missing appUrl in the app#6552
Open
lkostrowski wants to merge 5 commits intomainfrom
Open
Conversation
🦋 Changeset detectedLatest commit: 2159d3f 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6552 +/- ##
==========================================
+ Coverage 48.63% 48.73% +0.10%
==========================================
Files 2591 2592 +1
Lines 46143 46175 +32
Branches 10873 10872 -1
==========================================
+ Hits 22441 22504 +63
- Misses 22466 23291 +825
+ Partials 1236 380 -856
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens app/extension handling around missing appUrl so the dashboard no longer crashes when installed apps omit it, aligning dashboard behavior with the backend’s more permissive app model.
Changes:
- Add
appUrlto app fragments/types and use it to decide whether third-party apps should open their iframe view or fall back to the manage screen. - Reject manifests that define relative extension URLs without an
appUrl, and filter invalid runtime extensions from extension lists/sidebar handling. - Surface the “no configuration screen” state in the app details UI and add targeted tests for the new routing/filtering behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/graphql/types.generated.ts |
Regenerates GraphQL types to include appUrl in installed app data. |
src/graphql/hooks.generated.ts |
Updates generated fragment document to request appUrl. |
src/fragments/apps.ts |
Adds appUrl to the installed app details fragment. |
src/extensions/views/ViewManifestExtension/ViewManifestExtensionIframe.tsx |
Redirects apps without appUrl to the manage screen instead of rendering the iframe. |
src/extensions/views/InstalledExtensions/hooks/useInstalledExtensions.tsx |
Uses appUrl when deciding whether installed apps should link to view vs manage routes. |
src/extensions/views/InstalledExtensions/hooks/useInstalledExtensions.test.tsx |
Adds coverage for href resolution with and without appUrl. |
src/extensions/views/EditManifestExtension/components/AppDetailsPage/AppDetailsPage.tsx |
Shows an informational banner when a third-party app has no configuration screen URL. |
src/extensions/urls.ts |
Makes URL conversion helpers require non-null appUrl and appId. |
src/extensions/hooks/useExtensions.ts |
Filters out relative extensions whose parent app lacks appUrl. |
src/extensions/hooks/useExtensions.test.ts |
Adds tests for filtering invalid extensions while keeping absolute URLs. |
src/extensions/domain/app-manifest.ts |
Tightens manifest validation so relative extension URLs require appUrl. |
src/extensions/domain/app-manifest.test.ts |
Adds validation coverage for missing-appUrl manifest cases across targets. |
src/components/Sidebar/menu/utils.ts |
Avoids building sidebar dashboard URLs when an extension URL is absolute or its app lacks appUrl. |
src/components/Sidebar/menu/utils.test.ts |
Covers sidebar menu mapping for missing/absolute URL cases. |
locale/defaultMessages.json |
Adds message catalog entries for the new app-details notice. |
.changeset/safe-app-url-handling.md |
Records the patch-level user-facing behavior change. |
Comments suppressed due to low confidence (1)
locale/defaultMessages.json:6735
- The new description message is wired to id
1wcKdbinAppDetailsPage, but this catalog entry adds an unusedZcoqVFkey instead. As a result the actual string shown in the UI is missing fromdefaultMessages.json, so translators/export tooling won't pick up the text that was introduced in this PR.
"context": "metric unit system",
"string": "Metric"
},
Comment on lines
+9
to
+18
| const resolveExtensionMenuUrl = (extension: Extension): string | undefined => { | ||
| if (isUrlAbsolute(extension.url) || !extension.app.appUrl) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return ExtensionsUrls.resolveDashboardUrlFromAppCompleteUrl( | ||
| extension.url, | ||
| extension.app.appUrl, | ||
| extension.app.id, | ||
| ); |
Comment on lines
+60
to
+84
| {data.type === AppTypeEnum.THIRDPARTY && !data.appUrl && ( | ||
| <Box | ||
| data-test-id="no-configuration-screen-info" | ||
| display="flex" | ||
| alignItems="flex-start" | ||
| backgroundColor="info1" | ||
| padding={4} | ||
| gap={2} | ||
| borderRadius={3} | ||
| marginX={6} | ||
| marginY={4} | ||
| > | ||
| <Box flexShrink="0" marginTop={1}> | ||
| <Info size={20} /> | ||
| </Box> | ||
| <Box display="flex" flexDirection="column" gap={1}> | ||
| <Text size={3} fontWeight="medium"> | ||
| <FormattedMessage {...noConfigurationScreenMessages.title} /> | ||
| </Text> | ||
| <Text size={3}> | ||
| <FormattedMessage {...noConfigurationScreenMessages.description} /> | ||
| </Text> | ||
| </Box> | ||
| </Box> | ||
| )} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prior to this change:
appUrlbeing set.appUrlis used to compute "configuration iframe url" in dashboard and relative extension urls (eg extension is/fooappended to base appUrl)When app installed and user goes to configuration or opens such extension, Dashboard crashesThis is happening because Dashboard codebase was silently accepting missing appUrl, applying invalid fallbacks (like
""for url).This PR is