feat: Show warning when previewing a subform #14486
Conversation
Jondyr
added
text/content
📝 WalkthroughWalkthroughThis pull request introduces changes to handle unsupported subform previews across multiple frontend components. Modifications include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/ui-editor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/hooks/useGetLayoutSetByName.ts (1)
16-16: LGTM! Consider adding type assertion for better type safety.The optional chaining operator improves error handling. For additional type safety, consider adding a non-null assertion for the
setsproperty since it's guaranteed by the API type.- return layoutSetsResponse?.sets.find((set) => set.id === name); + return layoutSetsResponse?.sets!.find((set) => set.id === name);frontend/packages/ux-editor/src/components/Preview/Preview.tsx (1)
6-6: Consider organizing imports by category.Group related imports together for better maintainability:
- React and hooks
- Components
- Types
- Utilities
import React, { useEffect, useState } from 'react'; +import { useTranslation } from 'react-i18next'; +import cn from 'classnames'; + +import { useAppContext, useGetLayoutSetByName } from '../../hooks'; +import { useChecksum } from '../../hooks/useChecksum.ts'; +import { useStudioEnvironmentParams } from 'app-shared/hooks/useStudioEnvironmentParams'; +import { useSelectedTaskId } from 'app-shared/hooks/useSelectedTaskId'; +import { useCreatePreviewInstanceMutation } from 'app-shared/hooks/mutations/useCreatePreviewInstanceMutation'; +import { useUserQuery } from 'app-shared/hooks/queries'; + +import { Paragraph } from '@digdir/designsystemet-react'; +import { + StudioAlert, + StudioButton, + StudioCenter, + StudioErrorMessage, + StudioSpinner, +} from '@studio/components'; +import { ShrinkIcon } from '@studio/icons'; + +import type { SupportedView } from './ViewToggler/ViewToggler'; +import { ViewToggler } from './ViewToggler/ViewToggler'; +import { PreviewLimitationsInfo } from 'app-shared/components/PreviewLimitationsInfo/PreviewLimitationsInfo'; + +import { previewPage } from 'app-shared/api/paths'; +import classes from './Preview.module.css'; -// Remove existing importsAlso applies to: 10-16
frontend/app-preview/src/views/LandingPage.tsx (1)
50-51: Consider extracting duplicated layout set detection logic into a shared hook.The layout set detection logic is duplicated between LandingPage and Preview components. Consider creating a shared hook:
// hooks/useIsSubform.ts export const useIsSubform = (layoutSetName: string) => { const { org, app } = useStudioEnvironmentParams(); const { data: layoutSets } = useLayoutSetsQuery(org, app); const currentLayoutSet = layoutSets?.sets?.find((set) => set.id === layoutSetName); return currentLayoutSet?.type === 'subform'; };Then use it in both components:
const isSubform = useIsSubform(selectedFormLayoutSetName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app-preview/src/views/LandingPage.tsx(3 hunks)frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Preview/Preview.tsx(3 hunks)frontend/packages/ux-editor/src/hooks/useGetLayoutSetByName.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/language/src/nb.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (4)
frontend/packages/ux-editor/src/components/Preview/Preview.tsx (2)
107-108: LGTM! Layout set detection is implemented correctly.The implementation properly uses the hook to detect subform layouts, maintaining consistency with the LandingPage component.
143-147: LGTM! Warning alert implementation is clean and user-friendly.The warning alert is properly placed and uses translations for internationalization support.
frontend/app-preview/src/views/LandingPage.tsx (2)
11-16: Consider organizing imports by category.Similar to Preview.tsx, group related imports together for better maintainability.
101-105: LGTM! Warning alert implementation maintains consistency.The warning alert implementation is consistent with the Preview component and properly placed within the header.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14486 +/- ##
=======================================
Coverage 95.68% 95.69%
=======================================
Files 1888 1888
Lines 24568 24574 +6
Branches 2820 2822 +2
=======================================
+ Hits 23509 23515 +6
Misses 799 799
Partials 260 260 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Preview/Preview.test.tsx (2)
137-145: Enhance test coverage and cleanup.While the test correctly verifies the presence of the warning, consider these improvements:
- Use exact text matching instead of regex to ensure the precise warning message.
- Reset
appContextMock.selectedFormLayoutSetNameafter the test to prevent side effects.- Verify the warning's styling/type using the appropriate test-id or role.
it('should show a warning that subform is unsupported in preview', async () => { appContextMock.selectedFormLayoutSetName = subformLayoutMock.layoutSetName; render(); await waitForElementToBeRemoved(() => screen.queryByTitle(textMock('preview.loading_preview_controller')), ); - expect(screen.getByText(/ux_editor.preview.subform_unsupported_warning/i)).toBeInTheDocument(); + const warning = screen.getByText(textMock('ux_editor.preview.subform_unsupported_warning')); + expect(warning).toBeInTheDocument(); + expect(warning.closest('[role="alert"]')).toHaveClass('warning'); + }); + + afterEach(() => { + appContextMock.selectedFormLayoutSetName = undefined; });
155-159: Improve mock implementation flexibility.The current mock implementation is hardcoded to always return a subform type. Consider:
- Making the mock response configurable through options.
- Ensuring the mock response structure matches the actual API.
- Adding test cases for error scenarios.
getLayoutSets: jest .fn() - .mockImplementation(() => - Promise.resolve({ sets: [{ id: subformLayoutMock.layoutSetName, type: 'subform' }] }), + .mockImplementation(() => { + const layoutSet = options.layoutSet ?? { + id: subformLayoutMock.layoutSetName, + type: 'subform', + // Add other required fields to match API response + }; + return Promise.resolve({ sets: [layoutSet] }); + }),Then add a test case for error handling:
it('should handle layout set fetch error gracefully', async () => { const errorMessage = 'Failed to fetch layout sets'; render({ queries: { getLayoutSets: jest.fn().mockRejectedValue(new Error(errorMessage)) } }); await waitForElementToBeRemoved(() => screen.queryByTitle(textMock('preview.loading_preview_controller')) ); expect(screen.getByText(textMock('ux_editor.preview.error'))).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/Preview/Preview.test.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/Preview/Preview.test.tsx (1)
12-12: LGTM!The import is correctly placed and properly utilized in the test implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app-preview/src/views/LandingPage.test.tsx (1)
93-105: Consider expanding test coverage.The test effectively verifies the basic warning message display. Consider adding the following test scenarios:
- Verify iframe behavior when a subform is detected
- Test handling of multiple layout sets
- Verify the exact content and styling of the warning message
Example test case for iframe behavior:
it('should handle iframe correctly when previewing a subform', async () => { renderLandingPage({ getLayoutSets: jest .fn() .mockImplementation(() => Promise.resolve({ sets: [{ id: '', type: 'subform' }] })), }); await waitForElementToBeRemoved(screen.queryByTitle(textMock('preview.loading_page'))); // Verify iframe presence/absence or any specific attributes const iframe = screen.queryByTitle(textMock('preview.title')); expect(iframe).toBeInTheDocument(); // or not.toBeInTheDocument() based on requirements });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app-preview/src/views/LandingPage.test.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (1)
frontend/app-preview/src/views/LandingPage.test.tsx (1)
9-9: LGTM! Good TypeScript practice.The change to a type-only import is appropriate since
ServicesContextPropsis only used as a type annotation in the test file.
There was a problem hiding this comment.
It works fine. As we discussed, the position of the warning message should be reviewed with the UX team. I believe we should consider changing the width of the warning message and possibly its placement as well. You can check how we used StudioPageError for reference
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Localization
These updates enhance user communication regarding layout compatibility and improve the overall stability of the application.