EDM-1433: Add YAML viewer for fleet, device and repository detail pages#278
Conversation
## Walkthrough
This change introduces YAML editor functionality for Fleet, Device, and Repository detail pages by integrating the Monaco code editor and PatternFly React code editor components. New dependencies and type definitions are added to relevant `package.json` files. Several UI components are updated to provide tabbed navigation between details and YAML views, and new components for YAML editing, shortcut display, and custom Monaco themes are implemented. Translation strings are expanded to cover new UI actions and shortcuts. The device status debug modal and associated styles are removed, and routing is adjusted to support nested routes for the new YAML tabs.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `apps/ocp-plugin/package.json`, `apps/standalone/package.json`, `libs/ui-components/package.json`| Added dependencies for Monaco editor and PatternFly React code editor; added `file-saver` and its types; updated peer dependencies. |
| `libs/ui-components/src/components/common/CodeEditor/CodeEditorTheme.ts` | Added function to define custom Monaco editor themes for light and dark modes. |
| `libs/ui-components/src/components/common/CodeEditor/Shortcut.tsx`, `Shortcut.css` | Added new React component and CSS for displaying keyboard/mouse shortcuts in the UI. |
| `libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx`, `YamlEditorBase.tsx`, `YamlEditor.css` | Added new YAML editor component with version change detection, reload, and download; base editor component for rendering Monaco editor; associated CSS for layout and appearance. |
| `libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx` | Added hook to provide a popover listing editor shortcuts. |
| `libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx` | Renamed and enhanced Fleet details component to support tabbed navigation and YAML editing; updated wrapper component. |
| `libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx` | Enhanced repository details component to support tabs and YAML editing via nested routing. |
| `libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx` | Removed device status debug modal; added "YAML" tab and route with YAML editor for device details. |
| `libs/ui-components/src/components/Device/DeviceDetails/DeviceStatusDebugModal.tsx`, `.css` | Removed device status debug modal component and its CSS. |
| `libs/ansible/src/hooks/useFctlNavigation.tsx`, `apps/ocp-plugin/src/components/Fleets/FleetDetailsPage.tsx`, `apps/standalone/src/app/routes.tsx` | Updated imports and route paths to use the renamed and enhanced Fleet details page and support nested routing. |
| `libs/i18n/locales/en/translation.json` | Added and reorganized translation keys for new editor actions, shortcuts, and UI elements; removed obsolete debug info keys. |
| `libs/ui-components/src/components/DetailsPage/DetailsPageActions.tsx` | Simplified type annotation of `children` prop in `DetailsPageActions` component. |
| `libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx` | Simplified type alias for `CreateRepositoryFormContentProps` by removing unnecessary intersection. |
| `libs/ui-components/src/components/form/FlightCtlActionGroup.tsx` | Simplified type annotation of `children` prop in `FlightCtlActionGroup` component. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UIComponent
participant YamlEditor
participant Backend
User->>UIComponent: Navigate to Details Page
UIComponent->>UIComponent: Display tabs (Details, YAML)
User->>UIComponent: Click "YAML" tab
UIComponent->>YamlEditor: Render YAML editor with resource
YamlEditor->>Backend: (optional) Refetch resource on reload
YamlEditor->>User: Display YAML, allow download/copy/reloadSuggested reviewers
|
celdrake
force-pushed
the
EDM-1433-read-only-yaml-viewer
branch
2 times, most recently
from
d5745ac to
3f1d74c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx (1)
7-36: Good implementation of reusable hook for editor shortcutsThe hook is well-structured and provides proper typing with
PopoverProps. It effectively encapsulates the popover configuration for keyboard shortcuts that can be reused across the application.One optimization to consider is memoizing the return value since it creates a new object on every render:
export const useShortcutPopover = (onHideShortcuts?: VoidFunction): PopoverProps => { const { t } = useTranslation(); - return { + return React.useMemo(() => ({ 'aria-label': t('View shortcuts'), bodyContent: ( <table> <tbody> <Shortcut keyName="F1">{t('View all editor shortcuts')}</Shortcut> <Shortcut ctrl keyName="space"> {t('Activate auto complete')} </Shortcut> <Shortcut ctrl shift keyName="m"> {t('Toggle Tab action between insert Tab character and move focus out of editor')} </Shortcut> <Shortcut ctrlCmd shift keyName="o"> {t('View document outline')} </Shortcut> <Shortcut hover>{t('View property descriptions')}</Shortcut> <Shortcut ctrlCmd keyName="s"> {t('Save')} </Shortcut> </tbody> </table> ), maxWidth: '25rem', distance: 18, onHide: onHideShortcuts, - }; + }), [t, onHideShortcuts]); };libs/ui-components/src/components/common/CodeEditor/CodeEditorTheme.ts (1)
5-19: Consider using a more structured approach for color constantsThe current color constants use a flat naming convention. Consider organizing them into a color object or using PatternFly's existing token system once available (as noted in the TODO).
// TODO import from PF6 react-tokens. These are not available in PF5 -const t_color_green_70 = '#204d00'; -const t_color_white = '#ffffff'; -const t_color_black = '#000000'; -const t_color_gray_60 = '#4d4d4d'; -const t_color_yellow_70 = '#73480b'; -const t_color_yellow_30 = '#ffcc17'; -const t_color_green_30 = '#afdc8f'; -const t_color_blue_30 = '#92c5f9'; -const t_color_blue_70 = '#003366'; -const t_color_purple_70 = '#21134d'; -const t_color_purple_30 = '#b6a6e9'; -const t_color_gray_90 = '#1f1f1f'; -const t_color_gray_20 = '#e0e0e0'; +const colors = { + green: { + 30: '#afdc8f', + 70: '#204d00' + }, + white: '#ffffff', + black: '#000000', + gray: { + 20: '#e0e0e0', + 60: '#4d4d4d', + 90: '#1f1f1f' + }, + yellow: { + 30: '#ffcc17', + 70: '#73480b' + }, + blue: { + 30: '#92c5f9', + 70: '#003366' + }, + purple: { + 30: '#b6a6e9', + 70: '#21134d' + } +};This would make the theme definition more readable and maintainable going forward.
libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx (2)
2-2: Imports need to be reorganized for better structureThe imports on line 2 mix both UI components and routing components. Consider reorganizing the imports for better readability.
-import { DropdownItem, DropdownList, Grid, GridItem, Nav, NavList } from '@patternfly/react-core'; +import { + DropdownItem, + DropdownList, + Grid, + GridItem, + Nav, + NavList +} from '@patternfly/react-core';
78-99: Well-structured routing setup for the tabsThe routing configuration is well-implemented with a default redirect to the details tab for backward compatibility. The route structure follows React Router best practices.
One consideration: you might want to add error handling for the YAML editor in case the API object is malformed or doesn't meet expected format:
<Route path="yaml" - element={<YamlEditor filename={repositoryId} apiObj={repoDetails} refetch={refetch} />} + element={ + <YamlEditor + filename={repositoryId} + apiObj={repoDetails} + refetch={refetch} + onError={(error) => console.error('YAML editor error:', error)} + /> + } />libs/ui-components/src/components/common/CodeEditor/Shortcut.tsx (2)
23-27: Add TypeScript typing for ShortcutCommand componentThe
ShortcutCommandcomponent is missing proper TypeScript typing for its props.-export const ShortcutCommand = ({ children }) => ( +export const ShortcutCommand: React.FC<{ children: React.ReactNode }> = ({ children }) => ( <span className="fctl-shortcut__command"> <kbd className="fctl-shortcut__command-key">{children}</kbd> </span> );
46-85: Table structure doesn't include accessibility elementsThe table structure is missing proper accessibility elements for screen readers.
Consider adding proper table structure elements:
return ( - <tr> - <td className="fctl-shortcut__cell"> + <tr role="row"> + <td className="fctl-shortcut__cell" role="cell"> {/* shortcut content */} </td> - <td className="fctl-shortcut__cell">{children}</td> + <td className="fctl-shortcut__cell" role="cell">{children}</td> </tr> );libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (2)
36-42: Add error handling for download failuresThe download YAML function doesn't include error handling for potential failures.
const downloadYaml = () => { const resource = editorRef.current?.getValue(); if (resource) { + try { const blob = new Blob([resource], { type: 'text/yaml;charset=utf-8' }); saveAs(blob, `${filename}.yaml`); + } catch (error) { + console.error('Failed to download YAML:', error); + // Consider adding user notification for download failure + } } };
86-86: Avoid inline styles with CSS variablesUsing inline styles with CSS variables can be fragile. Consider moving this to a CSS class.
-<ActionGroup style={{ '--pf-v5-c-form__group--m-action--MarginTop': 0 } as React.CSSProperties}> +<ActionGroup className="fctl-yaml-editor-action-group">And add to your CSS file:
.fctl-yaml-editor-action-group { --pf-v5-c-form__group--m-action--MarginTop: 0; }libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (2)
51-56: Add type assertion for resource version comparisonThe comparison of
resourceVersionmight lead to unexpected behavior if values are undefined.- hasChanged = resourceVersion !== apiObj.metadata.resourceVersion; + hasChanged = resourceVersion !== (apiObj.metadata.resourceVersion || '0');
87-89: Avoid hardcoded navigation pathThe navigation path
../is hardcoded, which may be brittle if routing structure changes.Consider using a more explicit navigation approach:
onCancel={() => { - navigate('../.'); + // Navigate to parent route more explicitly, e.g.: + const parentPath = window.location.pathname.split('/').slice(0, -1).join('/'); + navigate(parentPath || '.'); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/ocp-plugin/package.json(2 hunks)apps/ocp-plugin/src/components/Fleets/FleetDetailsPage.tsx(1 hunks)apps/standalone/package.json(2 hunks)apps/standalone/src/app/routes.tsx(2 hunks)libs/ansible/src/hooks/useFctlNavigation.tsx(1 hunks)libs/i18n/locales/en/translation.json(3 hunks)libs/ui-components/package.json(2 hunks)libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx(3 hunks)libs/ui-components/src/components/Device/DeviceDetails/DeviceStatusDebugModal.css(0 hunks)libs/ui-components/src/components/Device/DeviceDetails/DeviceStatusDebugModal.tsx(0 hunks)libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx(4 hunks)libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx(3 hunks)libs/ui-components/src/components/common/CodeEditor/CodeEditorTheme.ts(1 hunks)libs/ui-components/src/components/common/CodeEditor/Shortcut.css(1 hunks)libs/ui-components/src/components/common/CodeEditor/Shortcut.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditor.css(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- libs/ui-components/src/components/Device/DeviceDetails/DeviceStatusDebugModal.css
- libs/ui-components/src/components/Device/DeviceDetails/DeviceStatusDebugModal.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
libs/ui-components/src/components/common/CodeEditor/Shortcut.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/common/CodeEditor/CodeEditorTheme.ts (1)
defineConsoleThemes(21-53)
libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx (4)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(119-119)libs/ui-components/src/hooks/useFetchPeriodically.ts (1)
useFetchPeriodically(9-86)libs/types/index.ts (1)
RepoSpecType(104-104)
libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
🔇 Additional comments (30)
apps/ocp-plugin/package.json (1)
51-51: Monaco and PatternFly code editor dependencies added correctly.The new dependencies for Monaco and PatternFly code editor are appropriate and necessary for YAML editing features. No issues found.
Also applies to: 77-78
libs/ui-components/src/components/common/CodeEditor/Shortcut.css (1)
1-22: Shortcut key styling is clear and consistent.The CSS for shortcut key display is well-structured and uses PatternFly variables for consistency. No issues found.
apps/ocp-plugin/src/components/Fleets/FleetDetailsPage.tsx (1)
2-2: Import path updated for refactored FleetDetailsPage.The import now correctly points to the new FleetDetailsPage component with tabbed navigation. No issues found.
libs/ui-components/src/components/common/CodeEditor/YamlEditor.css (1)
1-8: YAML editor styling is appropriate and theme-consistent.The CSS for the YAML editor is minimal, targeted, and uses theme variables. No issues found.
libs/ansible/src/hooks/useFctlNavigation.tsx (1)
40-40: Import path updated for refactored FleetDetailsPage.The import now correctly points to the new FleetDetailsPage component with tabbed navigation. No issues found.
apps/standalone/src/app/routes.tsx (2)
61-61: Import path updated correctly for FleetDetailsPage component.The import path now correctly points to the FleetDetailsPage component, aligning with the component renaming in the ui-components library.
204-204: Route path correctly updated to support nested routes.The path change from
:fleetIdto:fleetId/*properly enables nested routing, which is essential for the new tabbed interface with YAML viewer functionality. This change maintains consistency with similar device and repository route patterns.apps/standalone/package.json (2)
29-29: Monaco editor webpack plugin correctly added as a dev dependency.The
monaco-editor-webpack-pluginis necessary for properly bundling Monaco editor in a webpack-based build process. Version 7.1.0 is appropriate for the Monaco editor version being used.
54-55: Required dependencies added for code editing functionality.The added dependencies (
monaco-editorand@patternfly/react-code-editor) are essential for implementing the YAML viewer feature mentioned in the PR objectives. These versions are compatible with the other PatternFly components used in the project.libs/ui-components/package.json (2)
28-28: File-saving dependencies added for YAML download functionality.The
file-saverlibrary and its TypeScript type definitions will allow users to download YAML content, enhancing the editor functionality.Also applies to: 35-35
53-55: Required peer dependencies correctly specified.The updated peer dependencies ensure that consuming applications provide compatible versions of React Router DOM, Monaco Editor, and PatternFly React Code Editor components, which are needed for the new YAML editing functionality.
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (4)
20-20: YamlEditor component imported correctly.The import statement correctly references the YamlEditor component from the common CodeEditor directory.
95-95: YAML tab added to navigation.The YAML tab has been appropriately added to the tertiary navigation alongside existing tabs, providing users with access to the new YAML editing functionality.
135-138: YAML route and editor implemented correctly.The YAML route is properly configured to render the YamlEditor component with the necessary props:
filename: Uses device alias or ID for identificationapiObj: Passes the device object for rendering as YAMLrefetch: Provides callback to refresh data after editsThis implementation ensures the YAML editor has all required context for proper operation.
1-158:Details
✅ Verification successful
Verify the absence of debug functionality is intended.
The removal of the DeviceStatusDebugModal and related functionality appears to be replacing debug information viewing with the more comprehensive YAML editor. Please confirm this is the intended behavior and that no debug capabilities are lost with this change.
🏁 Script executed:
#!/bin/bash # Search for references to DeviceStatusDebugModal in the entire codebase rg "DeviceStatusDebugModal"Length of output: 27
Debug functionality removal verified – YAML editor now serves as the debug interface.
- A repository-wide search for
DeviceStatusDebugModalreturned no results, confirming its removal.- The current implementation leverages the YAML editor to display device configuration details, which can serve as a consolidated debugging view.
Please confirm that replacing the debug modal with the YAML editor is the desired design change and that no additional debug features are required.
libs/ui-components/src/components/common/CodeEditor/CodeEditorTheme.ts (1)
21-53: LGTM! Well-structured theme definitions for Monaco editorThe console light and dark themes are well-defined and follow Monaco's theming API correctly. The color assignments are appropriately different between light and dark themes to maintain proper contrast and readability.
libs/i18n/locales/en/translation.json (3)
40-63: Good addition of translation strings for the new YAML editor featuresThe new translation strings are comprehensive and follow the existing pattern, covering keyboard shortcuts, editor actions, and YAML-specific terminology.
126-126: Good addition of YAML translation string for the new tabThis translation string correctly supports the new YAML tab added to the UI.
407-408: LGTM! Added translation strings for resource sync behaviorThese strings provide clear messaging about fleets that are managed by resource syncs.
libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryDetails.tsx (4)
18-19: LGTM! Good addition of required components for the tabbed interfaceThe NavItem and YamlEditor imports are correctly added to support the new tabbed interface with YAML editing capability.
24-24: LGTM! Updated to use React Router hooksGood use of modern React Router hooks from the application context.
27-27: Correct extraction of the refetch function for the YAML editorYou've properly extracted the refetch function from useFetchPeriodically to pass to the YamlEditor component, allowing the YAML view to refresh data after changes.
67-74: Good implementation of tabbed navigationThe tertiary navigation implementation follows PatternFly's navigation patterns correctly, providing a clean way to switch between details and YAML views.
libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx (3)
47-54: LGTM - Consistent tabbed navigation implementationThe tabbed navigation pattern with tertiary nav matches the PatternFly design guidelines and is consistent with other detail pages in the application.
131-131: Component reference updated correctlyThe wrapper component has been updated to reference the renamed
FleetDetailPagecomponent.
101-108:Details
❓ Verification inconclusive
Verify resource reload propagation
The YAML editor is properly configured with the fleet object and refetch callback. When changes are made in the YAML editor, ensure that the main view is also updated to reflect these changes.
🏁 Script executed:
#!/bin/bash # Check if FleetDetailsContent is also updated when refetch is called echo "Checking how FleetDetailsContent handles updates:" rg -A 10 -B 10 "FleetDetailsContent" --type tsxLength of output: 184
Action: Manual Verification of FleetDetailsPage Refetch Flow
The routing in
FleetDetailsPage.tsxcorrectly passes therefetchcallback to the YAML editor, and the fleet object is provided as expected. However, our automated search did not yield clear evidence that theFleetDetailsContentcomponent updates whenrefetchis called. Please verify manually that:
- FleetDetailsContent Updates: When changes are made in the YAML editor (triggering
refetch), the main view (i.e.,FleetDetailsContent) properly reloads its data.- Data Propagation: The updated fleet object is correctly reflected in the main view after YAML modifications.
If necessary, consider adding temporary logging or state checks within
FleetDetailsContentto confirm that it reacts appropriately to updates.libs/ui-components/src/components/common/CodeEditor/Shortcut.tsx (2)
7-7:Details
✅ Verification successful
Verify CSS file existence
Ensure that the referenced CSS file exists and contains the necessary styles for the Shortcut component.
🏁 Script executed:
#!/bin/bash # Check if the CSS file exists if [ -f "libs/ui-components/src/components/common/CodeEditor/Shortcut.css" ]; then echo "CSS file exists" cat "libs/ui-components/src/components/common/CodeEditor/Shortcut.css" else echo "CSS file doesn't exist!" fiLength of output: 747
Shortcut CSS file verified
The CSS file at
libs/ui-components/src/components/common/CodeEditor/Shortcut.cssexists and contains the necessary styling for the Shortcut component. No further changes are needed.
29-29:Details
❓ Verification inconclusive
Replace deprecated platform detection method
The
window.navigator.platformproperty is deprecated. Consider using a more modern approach for platform detection.-export const isMac = window.navigator.platform.includes('Mac'); +export const isMac = navigator.userAgent.includes('Mac') || navigator.userAgent.includes('Macintosh');Alternatively, consider using a more robust platform detection library:
-export const isMac = window.navigator.platform.includes('Mac'); +// You could add 'platform' npm package as a dependency +import platform from 'platform'; +export const isMac = platform.os?.family?.includes('Mac') || platform.os?.family?.includes('OS X');
🌐 Web query:
Is window.navigator.platform deprecated and what should be used instead?💡 Result:
Yes,
window.navigator.platform(also accessible asnavigator.platform) is deprecated and should generally be avoided for modern web development[1][4][5][7]. This API once provided a string identifying the user's operating system (e.g., "Win32", "MacIntel", "Linux x86_64"), but has been removed from the latest web standards and may cease to work at any time in the future[1][4][5][7].Why Is It Deprecated?
- Browsers are deprecating
navigator.platformdue to privacy concerns and its unreliability—modern browsers may provide misleading or spoofed values, and some may always return an empty string[1][7].Alternatives and Best Practices
1. Feature Detection
- The widely recommended alternative is to use feature detection instead of trying to infer the user's environment. This means testing directly for the capabilities your code needs (e.g., testing for touch support, specific APIs, etc.), rather than guessing based on the platform string[1][9].
2. User-Agent Client Hints
- The emerging standard in browsers like Chromium is User-Agent Client Hints (
navigator.userAgentData), which provide structured and permission-controlled access to user agent information[8][9].- However, these are not universally supported yet—Firefox and Safari are still missing implementation—so you cannot rely on them for cross-browser compatibility as of 2025[8][9].
3. User-Agent String
- The traditional user agent string (
navigator.userAgent) is also being deprecated and reduced in detail by modern browsers for privacy reasons, so it is not a futureproof solution[3][8][9].Summary Table
API Status Recommended Use navigator.platform Deprecated Avoid, unreliable navigator.userAgent Deprecated Avoid, unreliable navigator.appVersion Deprecated Avoid, unreliable navigator.userAgentData Emerging Use (Chromium only, limited cross-browser support) Feature Detection Supported Preferred Guidance
Avoid using
navigator.platformand similar OS-detection properties. Where practical, use feature detection to adapt your web application to different environments. For cases where platform identification is absolutely necessary, consider usingnavigator.userAgentDataif you are targeting browsers that support it, but be aware of its limited compatibility at this time[1][8][9].In short:
window.navigator.platformis deprecated. Use feature detection instead, and explorenavigator.userAgentDatafor platforms that support it, but be aware of current browser compatibility limitations[1][8][9].Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform
- 2: mdn/content#14429
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/appVersion
- 4: https://www.w3schools.com/js/js_window_navigator.asp
- 5: https://www.w3schools.com/jsref/prop_nav_platform.asp
- 6: https://groups.google.com/a/chromium.org/g/chromium-discuss/c/8cCllrVX4kI
- 7: https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator/platform
- 8: nextcloud/text#2770
- 9: highcharts/highcharts#18301
- 10: https://community.cloudflare.com/t/deprecated-feature-used/349018
Action: Update OS detection logic due to deprecation of
navigator.platformThe current usage of
window.navigator.platformis deprecated because it’s unreliable and may be removed in the future. Instead of relying solely on this property or even falling back tonavigator.userAgent—which is also subject to deprecation and privacy limitations—we recommend a twofold approach:
- Primary Option (if browser support permits): Use the emerging
navigator.userAgentDataAPI for detecting the platform in a more structured and privacy-conscious manner.- Fallback Option: Fall back to checking
navigator.userAgentfor Mac-related substrings only ifuserAgentDataisn’t available.Below are revised diff snippets that illustrate these updated approaches:
-export const isMac = window.navigator.platform.includes('Mac'); +// Primary: Use navigator.userAgentData if available; fallback to userAgent +export const isMac = (navigator.userAgentData?.platform === 'macOS') + || navigator.userAgent.includes('Mac') + || navigator.userAgent.includes('Macintosh');Alternatively, if you prefer a more robust, library-driven solution, consider using the
platformpackage:-export const isMac = window.navigator.platform.includes('Mac'); +import platform from 'platform'; +export const isMac = platform.os?.family?.includes('Mac') + || platform.os?.family?.includes('OS X');Notes:
- Browser Compatibility: The
navigator.userAgentDataAPI is currently supported in Chromium-based browsers. For Firefox, Safari, and others, the fallback tonavigator.userAgentis essential.- Feature Detection Consideration: If the only goal is to adapt UI behavior (e.g., for shortcut keys), consider whether directly detecting the OS is required or if feature detection might serve as a more future-proof alternative.
Please verify these changes in your targeted browsers to ensure consistent behavior across environments.
libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx (1)
57-85: LGTM - Well-structured editor implementationThe CodeEditor component is well-configured with appropriate translations, code manipulation, and theming. The editor mounting logic properly initializes Monaco with custom themes.
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (1)
68-80: Well-implemented window resize handlingThe resize handling for Monaco editor is properly implemented with React's useEffect and useCallback hooks to ensure the editor maintains proper dimensions when the window size changes.
| import YamlEditor from '../../common/CodeEditor/YamlEditor'; | ||
|
|
||
| const FleetDetails = () => { | ||
| const FleetDetailPage = () => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Component rename detected
The component name changed from FleetDetails to FleetDetailPage. This improves consistency with the naming pattern seen in other detail pages, but make sure to update all imports and references throughout the codebase.
🏁 Script executed:
#!/bin/bash
# Check if there are any references to the old component name that might need updating
echo "Searching for potential missed references to the old component name:"
rg "FleetDetails(?!Page|Content|WithPermissions)" --type ts --type tsxLength of output: 479
Below is a revised verification script that uses PCRE2 to properly handle the look-ahead syntax. Please run this updated script to re-check for any lingering references to "FleetDetails" (excluding those already updated to Page, Content, or WithPermissions):
Once you run this script, verify that no results are returned. If the output remains empty, it confirms that all references have been updated accordingly.
Next steps:
- If any results appear, please update the imports/references accordingly.
- If no results are found, you can safely consider the component rename change complete.
Note: The original review comment regarding the need to update all references still applies until this verification is confirmed.
🏁 Script executed:
#!/bin/bash
echo "Searching for potential references to the old component name using PCRE2 regex:"
rg --pcre2 "FleetDetails(?!Page|Content|WithPermissions)" --type ts --type tsx .Length of output: 401
Below is an updated verification script that searches TS and TSX files by explicitly using glob patterns to avoid the “unrecognized file type” error. Please run the following script to verify that there are no lingering references to "FleetDetails" (except those meant to match the exceptions):
If the script returns no output, it indicates that all references to the old name have been updated. Otherwise, please update any output results accordingly.
Component Rename Verification:
- The component has been correctly renamed from
FleetDetailstoFleetDetailPage. - All imports and references should be updated consistently across the codebase.
- Run the above script to ensure that no obsolete references remain.
🏁 Script executed:
#!/bin/bash
echo "Searching for potential references to the old component name in TS and TSX files using PCRE2 regex:"
rg --pcre2 "FleetDetails(?!Page|Content|WithPermissions)" -g "*.ts" -g "*.tsx" .Length of output: 1667
Action: Update All Imports of the Renamed Component
The file libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx now defines the component as FleetDetailPage, which aligns with our naming conventions. However, the following files still reference the old name (FleetDetails):
-
libs/ansible/src/hooks/useFctlNavigation.tsx:
- Lazy-loaded import and JSX usage still reference
<FleetDetails />.
- Lazy-loaded import and JSX usage still reference
-
apps/standalone/src/app/routes.tsx:
- The component is imported and used as
<FleetDetails />.
- The component is imported and used as
-
apps/ocp-plugin/src/components/Fleets/FleetDetailsPage.tsx:
- Similarly, the import and usage are still using the old name.
Please update these references to use FleetDetailPage to maintain consistency across the codebase.
There was a problem hiding this comment.
The current name aligns to how we're importing and using the Devices equivalent components.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the Device details component files to see their naming structure
echo "===== Device Component Structure ====="
find libs/ui-components/src/components/Device -name "*Detail*" -type f | sort
# Check the content of DeviceDetailsPage.tsx to see the component name
echo -e "\n===== DeviceDetailsPage.tsx Component Name ====="
grep -n "^const DeviceDetails" libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx
# Check how DeviceDetailsPage is imported in other files
echo -e "\n===== Import Patterns for Device Components ====="
rg "import.*from.*Device\/DeviceDetails" --type ts --type tsx
# Look for import paths that include DeviceDetailsPage
echo -e "\n===== Import for DeviceDetailsPage ====="
rg "import.*DeviceDetailsPage" --type ts --type tsx
Length of output: 1267
🏁 Script executed:
#!/bin/bash
# Let's check the actual component names more precisely
echo "===== DeviceDetailsPage Component Name ====="
grep -A 3 "^const DeviceDetails" libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx
echo -e "\n===== FleetDetailPage Component Name ====="
grep -A 3 "^const FleetDetail" libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx
# Let's check how these components are imported
echo -e "\n===== DeviceDetailsPage Imports ====="
rg "import.*DeviceDetails.*from" -g "*.ts" -g "*.tsx"
echo -e "\n===== FleetDetails/FleetDetailPage Imports ====="
rg "import.*(?:FleetDetails|FleetDetailPage).*from" -g "*.ts" -g "*.tsx" --pcre2
# Let's check file exports
echo -e "\n===== DeviceDetailsPage Export ====="
grep -A 2 "export default" libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx
echo -e "\n===== FleetDetailsPage Export ====="
grep -A 2 "export default" libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx
Length of output: 2613
@celdrake I see what you mean now. You're absolutely right. Looking at the codebase patterns:
DeviceDetailsPage.tsxcontainsDeviceDetailsPagecomponent but is imported asDeviceDetailsin other filesFleetDetailsPage.tsxcontainsFleetDetailPagecomponent but is imported asFleetDetailsin other files
This follows a consistent pattern where components are imported without the "Page" suffix regardless of their internal name. Thanks for clarifying this pattern - the internal component rename doesn't require changing all imports to maintain consistency with your existing conventions.
🧬 Code Graph Analysis Results
Relevant snippets for libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsx
From libs/ui-components/src/hooks/useAppContext.tsx (line 119)
export const useAppContext = () => React.useContext(AppContext);- Provides access to the global app context, including router, fetch, i18n, and other shared resources used in
FleetDetailsPage.tsx.
From libs/ui-components/src/hooks/useTranslation.ts (lines 5-8)
export const useTranslation = () => {
const { i18n } = useAppContext();
return useI18nTranslation(i18n.transNamespace);
};- Hook used for internationalization (i18n) in the component, returning a translation function
tused for UI strings.
From libs/ui-components/src/hooks/useNavigate.tsx (lines 43-66)
export const useNavigate = () => {
const {
router: { useNavigate: useRouterNavigate, appRoutes },
} = useAppContext();
const navigate = useRouterNavigate();
return React.useCallback<NavigateFunction>(
(to: Route | number | ToObj) => {
if (typeof to === 'number') {
navigate(to);
} else {
if (toParamIsToObj(to)) {
const route = appRoutes[to.route];
navigate(`${route}/${to.postfix}`);
} else {
const route = appRoutes[to];
navigate(route);
}
}
},
[navigate, appRoutes],
);
};- Custom navigation hook wrapping router navigation, used in the component for programmatic route changes, e.g., navigating to edit or fleet list pages.
From libs/ui-components/src/hooks/useAccessReview.ts (lines 8-33)
export const useAccessReview = (kind: RESOURCE, verb: VERB): AccessReviewResult => {
const [isLoading, setIsLoading] = React.useState(true);
const [error, setError] = React.useState<string>();
const [isAllowed, setIsAllowed] = React.useState(false);
const {
fetch: { checkPermissions },
} = useAppContext();
React.useEffect(() => {
const doItAsync = async () => {
setIsLoading(true);
try {
const allowed = await checkPermissions(kind, verb);
setIsAllowed(allowed);
} catch (err) {
setError(getErrorMessage(err));
setIsAllowed(false);
} finally {
setIsLoading(false);
}
};
doItAsync();
}, [kind, verb, checkPermissions]);
return [isAllowed, isLoading, error];
};- Hook to check user permissions for a given resource and verb, used in the component to conditionally render edit/delete actions.
From libs/ui-components/src/hooks/useFetchPeriodically.ts (lines 9-86)
export const useFetchPeriodically = <R>(
query: FlightControlQuery,
onFetchComplete?: (data: R) => void,
): [R | undefined, boolean, unknown, VoidFunction, boolean] => {
const [isLoading, setIsLoading] = React.useState(true);
const [isUpdating, setIsUpdating] = React.useState(false);
const [data, setData] = React.useState<R>();
const [error, setError] = React.useState<unknown>();
const [forceUpdate, setForceUpdate] = React.useState(0);
const ref = React.useRef(0);
const prevResolvedQueryHash = React.useRef<string>();
const { get } = useFetch();
// When the query parameters change, the hash will too. We must perform a refetch outside the timeout loop
const queryStringHash = getQueryStringHash(query);
// Callback that generates the updated query for refreshes. It may provide updated values for parameters such as from/to for metric queries
const getRequestQuery = React.useCallback(() => getRequestQueryString(query), [query]);
React.useEffect(() => {
let abortController: AbortController;
const fetchPeriodically = async (id: number) => {
while (ref.current === id) {
const requestQuery = getRequestQuery();
if (requestQuery) {
try {
abortController = new AbortController();
if (id > 0 && prevResolvedQueryHash.current !== queryStringHash) {
setIsUpdating(true);
}
const data = (await get<R>(requestQuery, abortController.signal)) as R;
if (isLoading) {
setIsLoading(false);
}
setIsUpdating(false);
if (onFetchComplete) {
onFetchComplete(data); // Data might be mutated at this point
setData(data);
} else {
setData(data);
}
setError(undefined);
} catch (err) {
// aborting fetch trows 'AbortError', we can ignore it
if (abortController.signal.aborted) {
return;
}
setError(err);
setIsLoading(false);
setIsUpdating(false);
}
} else {
setIsLoading(false);
setError(undefined);
setData(undefined);
}
prevResolvedQueryHash.current = queryStringHash;
await new Promise((resolve) => setTimeout(resolve, TIMEOUT));
}
};
fetchPeriodically(ref.current);
return () => {
// eslint-disable-next-line
ref.current++;
abortController?.abort();
};
// eslint-disable-next-line
}, [get, forceUpdate, queryStringHash]);
const refetch = React.useCallback(() => setForceUpdate((val) => val + 1), []);
return [data, isLoading, error, refetch, isUpdating];
};- Hook used to fetch fleet data periodically from the API endpoint
fleets/${fleetId}?addDevicesSummary=true. - Returns data, loading state, error, a refetch function, and updating state.
- Used in the component to keep fleet data fresh and to trigger refetch after deletion.
These snippets provide context on hooks and utilities used in FleetDetailsPage.tsx for routing, permissions, data fetching, and translation, which are relevant to understanding the component's structure and behavior.
| // TODO add useShortcutPopover when adding saving capabilities to the YAML editor | ||
| // import { useShortcutPopover } from './useShortcutPopover'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TODO comment before merging
There's a TODO comment about adding useShortcutPopover when adding saving capabilities to the YAML editor. Consider addressing this now or creating a follow-up task.
🏁 Script executed:
#!/bin/bash
# Check if useShortcutPopover hook is implemented
if [ -f "libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx" ]; then
echo "useShortcutPopover hook exists:"
head -n 20 "libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx"
echo "..."
else
echo "useShortcutPopover hook doesn't exist yet!"
fiLength of output: 988
Action Required: Update the TODO in YamlEditorBase.tsx
The useShortcutPopover hook now exists in the codebase (see libs/ui-components/src/components/common/CodeEditor/useShortcutPopover.tsx). Please update the TODO in YamlEditorBase.tsx accordingly. If saving capabilities aren’t ready for integration, either remove the commented-out import or create a follow-up task to integrate the hook once the feature is enabled.
There was a problem hiding this comment.
I'll remove the commented import.
The idea is that we'll add it back when the editor allows to make modifications, as the shortcuts refer to the editing actions.
There was a problem hiding this comment.
@celdrake That approach makes perfect sense. Since the YAML editor is currently in read-only mode, the shortcuts from useShortcutPopover wouldn't be applicable yet. Removing the commented import and TODO now, then adding it back when implementing editing capabilities is a clean solution.
✏️ Learnings added
Learnt from: celdrake
PR: flightctl/flightctl-ui#278
File: libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx:13-14
Timestamp: 2025-04-16T12:05:30.695Z
Learning: The YAML editor component is intentionally configured as read-only in the current implementation. The useShortcutPopover hook should only be added when editing capabilities are implemented, as the shortcuts are specifically for editing actions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const convertObjToYAMLString = (obj: FlightCtlYamlResource) => { | ||
| let yaml = ''; | ||
| if (obj) { | ||
| try { | ||
| yaml = dump(obj, { lineWidth: -1 }); | ||
| } catch (e) { | ||
| yaml = 'Error'; | ||
| } | ||
| } | ||
| return yaml; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for YAML conversion
The current implementation simply returns "Error" when YAML conversion fails. Consider improving error handling to provide more context.
const convertObjToYAMLString = (obj: FlightCtlYamlResource) => {
let yaml = '';
if (obj) {
try {
yaml = dump(obj, { lineWidth: -1 });
} catch (e) {
- yaml = 'Error';
+ console.error('Failed to convert object to YAML:', e);
+ yaml = `# Error converting object to YAML\n# ${e instanceof Error ? e.message : String(e)}`;
}
}
return yaml;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const convertObjToYAMLString = (obj: FlightCtlYamlResource) => { | |
| let yaml = ''; | |
| if (obj) { | |
| try { | |
| yaml = dump(obj, { lineWidth: -1 }); | |
| } catch (e) { | |
| yaml = 'Error'; | |
| } | |
| } | |
| return yaml; | |
| }; | |
| const convertObjToYAMLString = (obj: FlightCtlYamlResource) => { | |
| let yaml = ''; | |
| if (obj) { | |
| try { | |
| yaml = dump(obj, { lineWidth: -1 }); | |
| } catch (e) { | |
| console.error('Failed to convert object to YAML:', e); | |
| yaml = `# Error converting object to YAML\n# ${e instanceof Error ? e.message : String(e)}`; | |
| } | |
| } | |
| return yaml; | |
| }; |
celdrake
force-pushed
the
EDM-1433-read-only-yaml-viewer
branch
from
3f1d74c to
35b0e69
Compare
|
We should also check permissions if user can edit the resource. Otherwise show read-only editor |
|
@rawagner The YAML is read-only ATM, because for Devices the updates (of internal "resourceVersion") happen so fast that it's literally impossible to modify a device. It would be frustrating for the user to attempt to edit/reload etc and we need to address this first. |
celdrake
force-pushed
the
EDM-1433-read-only-yaml-viewer
branch
from
56b67d4 to
b55eae2
Compare
celdrake
force-pushed
the
EDM-1433-read-only-yaml-viewer
branch
from
b55eae2 to
5d4e37b
Compare
2 participants
In Standalone mode:
https://github.com/user-attachments/assets/956a616f-b249-4170-b786-ff9960b63778
In OCP console:
https://github.com/user-attachments/assets/b02ab13f-c219-4bb3-8d65-44026ec4628a
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Removals
Style
Chores