feat: support V2 Backing Image in UI #831
Conversation
Signed-off-by: Yi-Ya Chen <[email protected]>
Signed-off-by: Yi-Ya Chen <[email protected]>
Signed-off-by: Yi-Ya Chen <[email protected]>
Signed-off-by: Yi-Ya Chen <[email protected]>
WalkthroughThe pull request introduces enhancements to the Backing Image management system, focusing on data engine version control and user interface improvements. Changes span multiple files in the Changes
Sequence DiagramsequenceDiagram
participant User
participant CreateBackingImage
participant BackingImageActions
User->>CreateBackingImage: Select Data Engine
CreateBackingImage->>CreateBackingImage: Validate Data Engine
CreateBackingImage-->>User: Enable/Disable Actions
User->>BackingImageActions: Attempt Action
BackingImageActions->>BackingImageActions: Check Data Engine
BackingImageActions-->>User: Allow/Restrict Action
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![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 (3)
src/routes/backingImage/BackingImageBulkActions.js (1)
93-115: LGTM: Bulk actions configuration aligns with single action behaviorThe implementation correctly:
- Maintains consistent v2 download restrictions across single and bulk actions
- Preserves existing validation logic
- Improves code readability through better structure
However, consider extracting the disabled state logic into shared utility functions to ensure future consistency.
+ // Add to src/utils/backingImage.js + export const isDownloadDisabled = (rows) => { + return rows.length === 0 + || rows.every(row => !hasReadyBackingDisk(row)) + || rows.some(row => row.dataEngine === 'v2') + } + + export const isBackupDisabled = (rows, backupTargetAvailable) => { + return rows.length === 0 + || backupTargetAvailable === false + || rows.every(row => !hasReadyBackingDisk(row)) + } // In BackingImageBulkActions.js const allActions = [ { key: 'download', name: 'Download', - disabled() { - return selectedRows.length === 0 - || selectedRows.every(row => !hasReadyBackingDisk(row)) - || selectedRows.some(row => row.dataEngine === 'v2') - } + disabled: () => isDownloadDisabled(selectedRows) }, { key: 'backup', name: 'Back Up', - disabled() { - return selectedRows.length === 0 - || backupTargetAvailable === false - || selectedRows.every(row => !hasReadyBackingDisk(row)) - } + disabled: () => isBackupDisabled(selectedRows, backupTargetAvailable) }, ]src/routes/backingImage/BackingImageList.js (1)
129-141: Add visual indication for v2 backing imagesSince v2 backing images have download restrictions, consider adding visual indication to help users quickly identify them.
render: (text) => { return ( <div> - {text} + {text === 'v2' ? ( + <Tooltip title="Download not available for v2 backing images"> + <Tag color="blue">{text}</Tag> + </Tooltip> + ) : ( + <span>{text}</span> + )} </div> ) },src/routes/backingImage/CreateBackingImage.js (1)
89-90: Consider reversing the default valuesThe default values suggest v1 is enabled and v2 is disabled, but the initialization logic in the form (line 317) prefers v2 when enabled. This could be confusing.
Consider this more consistent approach:
- v1DataEngineEnabled = true, - v2DataEngineEnabled = false + v2DataEngineEnabled = true, + v1DataEngineEnabled = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/backingImage/BackingImageActions.js(1 hunks)src/routes/backingImage/BackingImageBulkActions.js(1 hunks)src/routes/backingImage/BackingImageList.js(1 hunks)src/routes/backingImage/BackupBackingImageList.js(4 hunks)src/routes/backingImage/CreateBackingImage.js(4 hunks)src/routes/backingImage/index.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/routes/backingImage/index.js
[error] 120-120: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (7)
src/routes/backingImage/BackingImageActions.js (1)
55-76: LGTM: Actions configuration is well-structured and implements v2 restrictions correctly
The restructured actions array improves readability and correctly implements the download restriction for v2 backing images as per requirements.
src/routes/backingImage/BackupBackingImageList.js (1)
50-50: Verify column width adjustments across different screen sizes
The column width increases look reasonable but should be tested across different screen sizes to ensure:
- No horizontal scrolling issues on smaller screens
- Proper content accommodation without truncation
- Balanced column distribution
Also applies to: 61-61, 74-74, 103-103
✅ Verification successful
Let me gather more information about the table structure and responsive behavior.
Column widths are properly configured with horizontal scrolling support
The implementation is correctly handling wide tables:
- Total column width (1601px) is managed with horizontal scrolling (
scroll={{ x: 970 }}) - Table uses Ant Design's built-in responsive features
- Horizontal scroll threshold is set to 970px, ensuring smaller screens can navigate the content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the total width of columns exceeds typical screen sizes
# Description: Calculate total width of all columns
# Get all width values from the columns array
rg -n 'width:.*\d+' src/routes/backingImage/BackupBackingImageList.js | \
grep -o '[0-9]\+' | \
awk '{ sum += $1 } END { print "Total width: " sum "px" }'
Length of output: 172
Script:
#!/bin/bash
# Check for responsive table implementation and container width constraints
rg -A 5 -B 5 "Table|table" src/routes/backingImage/BackupBackingImageList.js
# Check for scroll related properties
rg "scroll|overflow" src/routes/backingImage/BackupBackingImageList.js
# Look for any parent container width definitions
rg "width:|maxWidth:|minWidth:" src/routes/backingImage/BackupBackingImageList.js
Length of output: 1559
src/routes/backingImage/BackingImageList.js (1)
Line range hint 3-9: Verify download restriction implementation
The PR objectives mention disabling downloads for v2 backing images. Let's verify the integration between BackingImageList and BackingImageActions.
✅ Verification successful
Download restriction for v2 backing images properly implemented ✅
The verification confirms that the download functionality is properly restricted for v2 backing images in BackingImageActions.js. The implementation includes:
- Download action is disabled when
selected.dataEngine === 'v2' - The condition is correctly placed in the action configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if BackingImageActions properly implements the download restriction
ast-grep --pattern 'const availableActions = $$$disabled$$$dataEngine === "v2"$$$'
# Check if the bulk actions component also implements this restriction
ast-grep --pattern 'const bulkActions = $$$disabled$$$dataEngine === "v2"$$$'
Length of output: 164
Script:
#!/bin/bash
# Let's check BackingImageActions.js for download-related code and v2 engine checks
rg -A 5 "download" src/routes/backingImage/BackingImageActions.js
# Also check for any v2 engine related conditions
rg -A 5 "dataEngine.*v2|v2.*dataEngine" src/routes/backingImage/BackingImageActions.js
# Look for the actual implementation of availableActions
ast-grep --pattern 'const availableActions = {
$$$
}'
Length of output: 1105
src/routes/backingImage/CreateBackingImage.js (3)
27-27: LGTM: Data engine field added to payload
The dataEngine field is correctly added to the payload object in genDataFromType.
371-372: LGTM: PropTypes correctly defined
The new props are properly typed as boolean values.
315-335: Verify data engine validation logic
The validation logic correctly prevents selection of disabled engines. However, there are a few considerations:
- The initial value logic prefers v2 when enabled, which might not align with the default prop values
- The validation message could be more descriptive
Consider these improvements:
initialValue: v2DataEngineEnabled ? 'v2' : 'v1',
rules: [
{
validator: (_rule, value, callback) => {
if ((value === 'v1' && !v1DataEngineEnabled) || (value === 'v2' && !v2DataEngineEnabled)) {
- callback(`${value} data engine is not enabled`)
+ callback(`Data engine ${value} is not enabled in the system settings`)
} else {
callback()
}
},
},
],✅ Verification successful
Based on the codebase analysis, I can now provide the final response:
Data engine validation logic is appropriate but needs default value alignment
The validation logic is correctly implemented for preventing selection of disabled engines. The codebase shows that:
- The v2 data engine has special handling (disabled for download actions)
- The default preference for v2 when enabled is consistent with the application's behavior across volume and backing image components
- The validation message improvement suggestion remains valid for better user experience
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any direct usages of data engine values
rg -l "dataEngine.*[\"']v[12][\"']"
Length of output: 397
Script:
#!/bin/bash
# Let's check the usage context of dataEngine in these files to understand the validation patterns
rg -A 5 -B 5 "dataEngine.*[\"']v[12][\"']" src/routes/volume/CreateVolume.js src/routes/backingImage/BackingImageBulkActions.js src/routes/backingImage/BackingImageActions.js
Length of output: 2657
Script:
#!/bin/bash
# Let's check the props and default values in the component
ast-grep --pattern 'const CreateBackingImage = ({ $$$, v1DataEngineEnabled, v2DataEngineEnabled, $$$}) => {
$$$
}' src/routes/backingImage/CreateBackingImage.js
Length of output: 164
Script:
#!/bin/bash
# Let's check the component definition and props differently
rg -A 5 "CreateBackingImage = \(" src/routes/backingImage/CreateBackingImage.js
# Also check where this component is used to understand default prop values
rg -l "<CreateBackingImage"
Length of output: 144
Script:
#!/bin/bash
# Let's check how CreateBackingImage is used in the parent component
rg -B 5 -A 10 "<CreateBackingImage" src/routes/backingImage/index.js
# Also check the file content to understand the component definition
cat src/routes/backingImage/CreateBackingImage.js | head -n 20
Length of output: 1360
src/routes/backingImage/index.js (1)
215-216: LGTM: Data engine props correctly passed
The data engine enablement flags are properly passed to the create modal component.
| }, { | ||
| title: 'Data Engine', | ||
| dataIndex: 'dataEngine', | ||
| key: 'dataEngine', | ||
| width: 130, | ||
| sorter: (a, b) => a.dataEngine.toString().localeCompare(b.dataEngine.toString()), | ||
| render: (text) => { | ||
| return ( | ||
| <div> | ||
| {text} | ||
| </div> | ||
| ) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PropTypes validation for dataEngine field
The new dataEngine field should be included in the component's PropTypes validation.
Add the following to the PropTypes definition at the bottom of the file:
list.propTypes = {
loading: PropTypes.bool,
dataSource: PropTypes.arrayOf(PropTypes.shape({
+ dataEngine: PropTypes.string.isRequired,
id: PropTypes.string.isRequired,
// ... other existing prop validations
})),
// ... rest of the propTypes
}Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: Yi-Ya Chen <[email protected]>
|
@houhoucoop , please backport this PR to v1.8.x branch |
|
@mergify backport v1.8.x |
|
|
What this PR does / why we need it
Data Engineselect in create backing image modalData Engineto backing image tableDownloadoptions for v2 backing imageDownloadif user select one of v2 backing imageIssue
[UI][FEATURE] Support V2 Backing Image in UI #9880
Test Result
User are able to select
Data Enginewhen create a backing imageShow
Data Engineinfo on the tableDisable
Downloadoptions for v2 backing imageDisable bulk action
Downloadif user select one of v2 backing imageAdditional documentation or context
Test with
LONGHORN_MANAGER_IP=http://159.89.205.43:30001/ npm run devSummary by CodeRabbit
New Features
Enhancements
Bug Fixes