fix(nc-gui): allow user to submit form in locked view #9705
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve updates to 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/nc-gui/components/smartsheet/details/Fields.vue (3)
Line range hint
1170-1200: Consider enhancing field validation for virtual columns.While the validation logic is comprehensive, consider these improvements for virtual columns:
- Add validation for circular references in LTAR relationships
- Validate that referenced tables and columns exist
- Add more specific error messages for each validation failure case
if ((column.uidt === UITypes.Links || column.uidt === UITypes.LinkToAnotherRecord) && isNew) { if ( (!column.childColumn || !column.childTable || !column.childId) && (!column.custom?.ref_model_id || !column.custom?.ref_column_id) ) { + // Add specific error message for each missing field + const missingFields = [] + if (!column.childColumn) missingFields.push('Child Column') + if (!column.childTable) missingFields.push('Child Table') + if (!column.childId) missingFields.push('Child ID') + return { + isValid: false, + error: `Missing required fields: ${missingFields.join(', ')}` + } } + // Add circular reference check + if (isCircularReference(column)) { + return { + isValid: false, + error: 'Circular reference detected in LTAR relationship' + } + } return false }
Line range hint
600-700: Consider adding batch operation support for better performance.The current implementation processes operations individually. Consider these improvements:
- Add batch operation support for multiple field changes
- Implement operation history for audit purposes
- Add operation debouncing for better performance during rapid changes
Line range hint
1-50: Enhance accessibility support.Consider implementing these accessibility improvements:
- Add ARIA labels for interactive elements
- Implement focus management for keyboard navigation
- Add screen reader announcements for operation status changes
packages/nc-gui/components/smartsheet/Form.vue (3)
313-313: Consider notifying the user when form submission is disallowed due to permissionsIn the
submitFormfunction, the checkif (!isUIAllowed('dataInsert')) returnprevents form submission for users without the 'dataInsert' permission. To enhance user experience, consider notifying the user that they lack the necessary permissions.Apply this diff to inform the user:
async function submitForm() { if (!isUIAllowed('dataInsert')) { + message.error(t('msg.error.permissionDenied')); return; } // ...
352-352: Consider notifying the user when form clearing is disallowed due to permissionsIn the
clearFormfunction, the checkif (!isUIAllowed('dataInsert')) returnprevents clearing the form for users without the 'dataInsert' permission. To improve user experience, consider informing the user that they lack the necessary permissions.Apply this diff to notify the user:
async function clearForm() { if (!isUIAllowed('dataInsert')) { + message.error(t('msg.error.permissionDenied')); return; } // ...
1216-1216: Simplify the conditional expressionThe condition
!isLocked || (isLocked && element?.visible)can be simplified to!isLocked || element?.visiblefor better readability without changing the logic.Apply this diff to simplify the condition:
- v-if="!isLocked || (isLocked && element?.visible)" + v-if="!isLocked || element?.visible"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/nc-gui/components/smartsheet/Form.vue (17 hunks)
- packages/nc-gui/components/smartsheet/details/Fields.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/nc-gui/components/smartsheet/details/Fields.vue (2)
1170-1170: LGTM! Improved color semantics for new field status.The change from orange to green for new field status badges improves visual clarity and follows common UI patterns where green represents successful additions.
Line range hint
1-1500: Changes align with PR objectives.The modifications to field state management and validation feedback support the PR's goal of improving form submission in locked view by providing better visual feedback and state management.
packages/nc-gui/components/smartsheet/Form.vue (2)
976-976: Verify the condition for disabling the tooltipThe
:disabledattribute in<NcTooltip :disabled="isEeUI || isLocked">disables the tooltip whenisEeUIistrueorisLockedistrue. If the intention is to show the tooltip when the feature is not available (i.e.,!isEeUI), consider revising the condition to ensure the tooltip behaves as expected.
2135-2141: LGTM!The added CSS styles enhance the hover effect for the
.nc-textarea-rich-editorclass within the.editableclass, improving user experience.
| watch(isLocked, (newValue) => { | ||
| if (newValue) { | ||
| activeRow.value = '' | ||
| } | ||
|
|
||
| clearForm() | ||
| }) | ||
|
|
There was a problem hiding this comment.
Ensure form is only cleared when the form becomes locked
In the watcher for isLocked, the clearForm() function is called unconditionally whenever isLocked changes, regardless of its value. This may result in the form being cleared even when the form becomes unlocked, which could disrupt the user's input. Consider modifying the logic to call clearForm() only when isLocked changes to true.
Apply this diff to modify the logic:
watch(isLocked, (newValue) => {
if (newValue) {
activeRow.value = ''
+ clearForm()
}
})📝 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.
| watch(isLocked, (newValue) => { | |
| if (newValue) { | |
| activeRow.value = '' | |
| } | |
| clearForm() | |
| }) | |
| watch(isLocked, (newValue) => { | |
| if (newValue) { | |
| activeRow.value = '' | |
| clearForm() | |
| } | |
| }) |
There was a problem hiding this comment.
Yeah, thats what we wanted here
There was a problem hiding this comment.
clearForm fucntion will reload the form
There was a problem hiding this comment.
@rameshmane7218, glad to hear that! 😊
(_/)
( ^‿^)
c(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
@rameshmane7218, understood. Thank you for the clarification.
(^･ｪ･^)
✏️ Learnings added
Learnt from: rameshmane7218
PR: nocodb/nocodb#9705
File: packages/nc-gui/components/smartsheet/Form.vue:800-807
Timestamp: 2024-10-24T06:00:55.861Z
Learning: The `clearForm()` function in `packages/nc-gui/components/smartsheet/Form.vue` reloads the form, and it is intended to be called unconditionally when `isLocked` changes.
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.
|
Uffizzi Preview |
101267b to
a940e73
Compare
a940e73 to
5311c2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/nc-gui/components/general/FormBranding.vue (3)
6-6: LGTM! Consider security improvements for external link.The URL change from GitHub to the product website is appropriate for branding. However, consider implementing proper security measures for external links.
Consider updating the template to use an anchor tag with security attributes:
- @click="openLink('https://www.nocodb.com')" + <a + href="https://www.nocodb.com" + target="_blank" + rel="noopener noreferrer" + class="flex items-center gap-3" + >
Line range hint
4-10: Improve accessibility for branding element.The current implementation using a div with click handler lacks proper keyboard navigation and accessibility attributes.
Consider these improvements:
- <div - class="flex items-center gap-3 cursor-pointer text-gray-700 dark:text-slate-300" - @click="openLink('https://www.nocodb.com')" - > + <a + href="https://www.nocodb.com" + target="_blank" + rel="noopener noreferrer" + class="flex items-center gap-3 text-gray-700 dark:text-slate-300" + aria-label="Visit NocoDB website" + > <img src="~assets/img/brand/nocodb-logo.svg" alt="NocoDB" class="flex-none w-6 h-6" /> <div class="text-sm">NocoDB Forms</div> - </div> + </a>
Missing import for
openLinkutility functionThe verification shows that:
openLinkis used in the component to open nocodb.com linkopenLinkis implemented inutils/urlUtils.ts- There's no import statement in FormBranding.vue
- No global injection of this function was found
The component needs to import the
openLinkutility from~/utils/urlUtilsto use it.🔗 Analysis chain
Line range hint
1-1: Verify the openLink implementation.The component uses
openLinkbut it's not defined in the setup script.Let's verify the implementation:
Let me gather more context about the usage of
openLinkin the FormBranding.vue file.
Let me check if there's a composable or plugin that might be injecting the
openLinkfunction globally.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for openLink implementation rg "openLink.*=" packages/nc-guiLength of output: 231
Script:
#!/bin/bash # Search for openLink usage in FormBranding.vue rg "openLink" packages/nc-gui/components/general/FormBranding.vue -A 2 # Check if there's an import statement for openLink rg "import.*openLink" packages/nc-gui/components/general/FormBranding.vue # Let's also see the implementation from urlUtils.ts cat packages/nc-gui/utils/urlUtils.tsLength of output: 2591
Script:
#!/bin/bash # Check for any plugin or composable that might provide openLink rg -g "*.ts" -g "*.js" -g "*.vue" "provide.*openLink|app.config.globalProperties.*openLink" packages/nc-gui # Check for any auto-imports configuration fd "auto-?imports" packages/nc-gui -x cat {}Length of output: 158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/nc-gui/components/general/FormBranding.vue (1 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (17 hunks)
- packages/nc-gui/components/smartsheet/details/Fields.vue (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nc-gui/components/smartsheet/Form.vue
- packages/nc-gui/components/smartsheet/details/Fields.vue
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of