feat: 14732 set up a new navigation page #14804
Conversation
|
Warning Rate limit exceeded@JamalAlabdullah has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request enhances the 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: 1
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx (2)
10-26: Hardcoded URL path should be configurable.The contact link has a hardcoded URL '/contact' which may not be ideal if URL structures change across environments.
- <Link href='/contact'>{t('general.contact')}</Link> + <Link href={`${process.env.CONTACT_PATH || '/contact'}`}>{t('general.contact')}</Link>
16-18: Context needed for appConfig display.The
appConfigvalue is displayed directly without any label or context, which might be confusing for users.- <Paragraph>{appConfig}</Paragraph> + <Paragraph>{t('general.service_name')}: {appConfig}</Paragraph>frontend/packages/shared/src/utils/featureToggleUtils.test.ts (1)
31-116: Consider adding a test for removing TaskNavigation from localStorage.While you've added tests for enabling TaskNavigation in various contexts, there's no test for removing this flag from localStorage. For completeness, consider adding a test similar to the existing removal tests but for the TaskNavigation flag.
// Add this test to the 'removeFeatureFromLocalStorage' describe block + it('should remove TaskNavigation from local storage', () => { + typedLocalStorage.setItem<string[]>('featureFlags', ['taskNavigation']); + removeFeatureFlagFromLocalStorage(FeatureFlag.TaskNavigation); + expect(typedLocalStorage.getItem<string[]>('featureFlags')).toEqual([]); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app-development/router/routes.tsx(3 hunks)frontend/packages/shared/src/utils/featureToggleUtils.test.ts(3 hunks)frontend/packages/shared/src/utils/featureToggleUtils.ts(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.module.css(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.test.tsx(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.module.css
- frontend/packages/ux-editor/src/containers/FormDesignNavigation/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (6)
frontend/packages/shared/src/utils/featureToggleUtils.ts (1)
14-14: LGTM: Feature flag added correctly.The new
TaskNavigationfeature flag follows the established naming pattern and is properly added to the enum.frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.test.tsx (1)
11-20: LGTM: Basic test coverage provided.The tests verify the component renders correctly and includes the contact link. Good basic coverage for this initial implementation.
frontend/app-development/router/routes.tsx (1)
62-64: LGTM: Conditional rendering based on feature flag.Correctly renders the new navigation component when the feature flag is enabled and no layout is selected.
frontend/packages/shared/src/utils/featureToggleUtils.test.ts (3)
31-39: Tests properly cover the TaskNavigation feature flag for localStorage.The added tests properly verify both the positive and negative cases for the TaskNavigation feature flag in localStorage, maintaining the same pattern as the existing tests.
83-91: Tests properly cover the TaskNavigation feature flag for URL parameters.These tests correctly verify the TaskNavigation feature flag behavior when enabled or disabled via URL parameters, following the established testing pattern of the codebase.
113-116: Test for adding TaskNavigation to localStorage is properly implemented.The test verifies that the TaskNavigation feature flag can be successfully added to localStorage, which is important for the feature toggle functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14804 +/- ##
=======================================
Coverage 95.77% 95.78%
=======================================
Files 1923 1925 +2
Lines 25081 25098 +17
Branches 2868 2869 +1
=======================================
+ Hits 24022 24040 +18
+ Misses 799 797 -2
- Partials 260 261 +1 ☔ View full report in Codecov by Sentry. |
…om/Altinn/altinn-studio into 14732-set-up-a-new-navigation-page
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app-development/router/routes.tsx (1)
80-82:⚠️ Potential issueGuard against undefined appConfigData.
When the feature flag is enabled and there's no selected layout set name, you pass
appConfigData?.serviceNameto the FormDesignerNavigation component. While you check for loading state, there's no guard against the case whereappConfigDatais null or undefined after loading completes. This could lead to runtime errors.Add an additional check before using appConfigData:
- isTaskNavigationEnabled && !selectedFormLayoutSetName ? ( - <FormDesignerNavigation appConfig={appConfigData?.serviceName} /> + isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData ? ( + <FormDesignerNavigation appConfig={appConfigData.serviceName} />
🧹 Nitpick comments (2)
frontend/app-development/router/routes.tsx (2)
79-89: Extract complex conditional rendering logic.The conditional rendering logic for deciding between FormDesignerNavigation and UiEditorLatest is becoming complex. Consider extracting this into a separate function to improve readability and maintainability.
+ const renderUiEditorContent = () => { + if (isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData) { + return <FormDesignerNavigation appConfig={appConfigData.serviceName} />; + } + + return ( + <UiEditorLatest + shouldReloadPreview={shouldReloadPreview} + previewHasLoaded={previewHasLoaded} + onLayoutSetNameChange={(layoutSetName) => setSelectedLayoutSetName(layoutSetName)} + /> + ); + }; + return isLatestFrontendVersion(version) ? ( - isTaskNavigationEnabled && !selectedFormLayoutSetName ? ( - <FormDesignerNavigation appConfig={appConfigData?.serviceName} /> - ) : ( - <UiEditorLatest - shouldReloadPreview={shouldReloadPreview} - previewHasLoaded={previewHasLoaded} - onLayoutSetNameChange={(layoutSetName) => setSelectedLayoutSetName(layoutSetName)} - /> - ) + renderUiEditorContent() ) : ( <UiEditorV3 /> );
63-67: Enhance error toast with more specific information.The current error message is generic. Consider providing more specific information about what went wrong to help with debugging.
useEffect(() => { if (isError) { - toast.error(t('overview.fetch_title_error_message')); + toast.error(t('overview.fetch_title_error_message') + ` (${org}/${app})`); } }, [isError, t]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app-development/router/routes.test.tsx(2 hunks)frontend/app-development/router/routes.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typechecking and linting
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/app-development/router/routes.test.tsx (1)
39-39: Good use of async testing patterns.The changes to make tests async and use
findByTestId,waitFor, and additional test cases for loading states make the tests more reliable when testing the async behavior introduced in the implementation. This aligns with React testing best practices.Also applies to: 47-47, 51-56, 58-61
frontend/app-development/router/routes.tsx (1)
52-53: Good implementation of feature toggle.Using the combination of localStorage for layout set persistence and feature flags for controlling the new navigation UI is a good approach for gradually rolling out this feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app-development/router/routes.tsx (1)
52-52: Unused setter in useLocalStorageYou're destructuring only the first value from useLocalStorage without using the setter function. If this is intentional because you only need to read the value, consider using an underscore for the second value to make it explicit, or use the setter if needed elsewhere in the component.
- const [selectedFormLayoutSetName] = useLocalStorage<string>('layoutSet/' + app); + const [selectedFormLayoutSetName, _] = useLocalStorage<string>('layoutSet/' + app);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app-development/router/routes.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
🔇 Additional comments (5)
frontend/app-development/router/routes.tsx (5)
53-53: Feature flag initialization for TaskNavigationGood job implementing the feature flag check for TaskNavigation. This approach allows for controlled rollout of the new navigation feature.
55-61: Solid implementation of error handling for app config dataI like how you've implemented the app config query with proper error handling:
- Using
hideDefaultError: trueto prevent default error UI- Custom error handling via toast notification
- Clear dependency array in the useEffect
This pattern aligns with best practices for handling API errors in React applications.
Also applies to: 63-67
69-71: Loading state handled appropriatelyGood implementation of the loading state with StudioSpinner while fetching app configuration data.
79-91: Consider adding a safety check for appConfigData.serviceNameWhile you check for
appConfigDataexistence in the conditional, you directly accessappConfigData.serviceNamewhen rendering FormDesignerNavigation. For improved robustness, consider adding a null check for serviceName as well.This comment is related to the previous review comment about guarding against potential undefined appConfigData properties.
- if (isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData) { - return <FormDesignerNavigation appConfig={appConfigData.serviceName} />; + if (isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData?.serviceName) { + return <FormDesignerNavigation appConfig={appConfigData.serviceName} />;
93-93: Good refactoring of conditional rendering logicMoving the rendering logic into a separate function improves readability while maintaining the version check. Well done!
…om/Altinn/altinn-studio into 14732-set-up-a-new-navigation-page
...end/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.module.css
Outdated
Show resolved
Hide resolved
…om/Altinn/altinn-studio into 14732-set-up-a-new-navigation-page
Nice catch 👍 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx (4)
6-8: Consider renaming the prop for better clarity.The
appConfigprop name suggests it would contain configuration data, but according to the PR summary, it's being used to display the app/service name. Consider renaming it to something more descriptive likeappNameorserviceNameto better reflect its purpose.export type FormDesignerNavigationProps = { - appConfig: string; + serviceName: string; };
16-18: Add placeholder or documentation for future navigation items.The component is currently minimal and lacks actual navigation elements. Consider adding a comment or placeholder indicating where additional navigation sections will be incorporated in future PRs to make the intention clear.
<div className={classes.content}> <div className={classes.header}>{appConfig}</div> + {/* Future navigation sections will be added here in subsequent PRs */} </div>
19-21: Consider making the contact link configurable.The contact link URL is hardcoded. Consider making it configurable through props or environment variables for better flexibility across different environments.
export type FormDesignerNavigationProps = { appConfig: string; + contactLink?: string; }; export const FormDesignerNavigation = ({ appConfig, + contactLink = '/contact' }: FormDesignerNavigationProps) => { // ... <footer className={classes.footer}> - <Link href='/contact'>{t('general.contact')}</Link> + <Link href={contactLink}>{t('general.contact')}</Link> </footer>
1-26: Add component documentation.The component would benefit from a JSDoc comment explaining its purpose as a parent component for navigation that will be expanded in future PRs.
+/** + * FormDesignerNavigation serves as the parent container for the form designer's + * navigation interface. This component will be expanded with additional navigation + * sections in future pull requests. + */ export const FormDesignerNavigation = ({ appConfig }: FormDesignerNavigationProps) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.module.css(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.module.css
🔇 Additional comments (2)
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx (2)
1-4: Good use of design system and modules.The imports follow good practices by utilizing the design system components and CSS modules for scoped styling.
10-12: Component declaration follows React best practices.Good use of TypeScript for props typing and destructuring for clean component implementation.
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx
Show resolved
Hide resolved
…om/Altinn/altinn-studio into 14732-set-up-a-new-navigation-page
|
I tested and still noticed a few mismatches with the sketches from Figma:
|
|
Here is the css code for the header: |
| const { data: appConfigData } = useAppConfigQuery(org, app, { | ||
| hideDefaultError: true, | ||
| }); |
There was a problem hiding this comment.
I’d suggest moving this code into FormDesignerNavigation, as it’s only used by FormDesignerNavigation and should not block the whole page from loading since it’s only used to show the appName.
| const { data: appConfigData } = useAppConfigQuery(org, app, { | |
| hideDefaultError: true, | |
| }); |
| if (isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData) { | ||
| return <FormDesignerNavigation appConfig={appConfigData.serviceName} />; |
There was a problem hiding this comment.
| if (isTaskNavigationEnabled && !selectedFormLayoutSetName && appConfigData) { | |
| return <FormDesignerNavigation appConfig={appConfigData.serviceName} />; | |
| if (isTaskNavigationEnabled && !selectedFormLayoutSetName) { | |
| return <FormDesignerNavigation />; |
| export type FormDesignerNavigationProps = { | ||
| appConfig: string; | ||
| }; |
There was a problem hiding this comment.
| export type FormDesignerNavigationProps = { | |
| appConfig: string; | |
| }; |
| appConfig: string; | ||
| }; | ||
|
|
||
| export const FormDesignerNavigation = ({ appConfig }: FormDesignerNavigationProps) => { |
There was a problem hiding this comment.
| export const FormDesignerNavigation = ({ appConfig }: FormDesignerNavigationProps) => { | |
| export const FormDesignerNavigation = () => { | |
| const { org, app } = useStudioEnvironmentParams(); | |
| const { data: appConfigData } = useAppConfigQuery(org, app); |
| <main className={classes.container}> | ||
| <div className={classes.panel}> | ||
| <div className={classes.content}> | ||
| <div className={classes.header}>{appConfig}</div> |
There was a problem hiding this comment.
| <div className={classes.header}>{appConfig}</div> | |
| <div className={classes.header}>{appConfigData?.serviceName}</div> |
Description
Changes for this PR:
featureFlagstaskNavigation.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
FormDesignerNavigationcomponent that enhances navigation within the form design context.UiEditorcomponent, enhancing user experience during loading and configuration fetching.TaskNavigation, has been added to toggle related functionalities within the application.FormDesignerNavigationcomponent, providing a modern and user-friendly interface.FormDesignerNavigationcomponent to verify rendering and functionality.UiEditorcomponent, improving reliability.UiEditorcomponent during layout changes and loading states.