Fix: Fields menu improvement #10135
Fix: Fields menu improvement #10135
Conversation
📝 WalkthroughWalkthroughThis pull request encompasses styling and component updates across multiple files in 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 (
|
|
Uffizzi Preview |
![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)
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (1)
75-120: Careful bulk creation increateLookups.Good checks for
hasSelectedFieldsand structured bulk ops. Consider displaying an error alert to the user for better feedback instead of only logging the exception.packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
683-701: Integration withSmartsheetToolbarAddLookupsDropdown.Great approach to nest the new lookup dropdown in each row. Make sure the dynamic key (
lookupDropdownsTickle) effectively triggers updates without stale references.
784-806:New Fielddropdown.Encapsulating the logic in
LazySmartsheetColumnEditOrAddProvideris a neat approach. Consider adding basic error handling if the creation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(7 hunks)
🔇 Additional comments (15)
packages/nc-gui/components/nc/Switch.vue (3)
6-6: Include 'xxsmall' in the size prop type.
Great job adding the new 'xxsmall' option to expand the Switch component's flexibility.
59-59: Double-check usage of 'xxsmall'.
Make sure the 'xxsmall' setting is recognized wherever size is passed and confirm it renders correctly in all use cases.
105-125: Implementing .size-xxsmall styles.
The specialized styles look coherent. Be sure to test thoroughly under different DPI or zoom settings.
packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue (3)
71-71: Changed vertical spacing from py-2 to pb-2.
Confirm that this new spacing meets design specs and doesn’t negatively impact adjacent elements.
77-77: Reduced bottom padding in container.
Check that removing pb-2 still aligns the search input properly and doesn't break the layout when content grows.
82-82: Updated input styling.
Applying !border-x-0 !border-t-0 !py-2 !rounded-t-lg can improve aesthetics. Looks consistent with the rest of the UI.
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (4)
6-9: Well-defined props interface.
Providing a clear Props interface significantly improves type safety and maintainability.
199-224: Reordering via Draggable.
The usage of vuedraggable for ordered lists is appropriate. Keep an eye on performance if the list grows large.
217-218: Simple and clear checkbox binding.
v-model:checked on NcCheckbox is straightforward. Ensure no conflicting click handlers overshadow this logic.
227-239: User-friendly button state handling.
Tying button disabled to !hasSelectedFields and loading to isLoading gracefully prevents duplicate submissions.
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
430-443: New references: addColumnDropdown, lookupDropdownsTickle, scrollToLatestField.
These additions enhance dropdown triggers and scroll management. Verify that frequent toggles don’t cause performance issues.
653-653: Drag handle logic.
Enabling a draggable handle is intuitive. Ensure that locked states properly prevent dragging without visual confusion.
675-683: Click handling for link columns.
Preventing toggling visibility for 'Links' columns is good. Double-check that the condition doesn’t inadvertently block other interactions.
746-755: NcSwitch with size xxsmall.
Consistent with the newly introduced .size-xxsmall. Validate it visually, especially in dense layouts.
packages/nc-gui/assets/style.scss (1)
957-957: LGTM: Enhanced visual feedback for search interaction.
The change improves the user experience by making the search icon more prominent when the search field is interacted with, switching from a subtle gray to the primary color on hover/focus states. This provides better visual feedback and aligns with the fields menu improvement objective.
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
3 times, most recently
from
bbe1ed2 to
6ce664d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
429-442: LGTM! Consider adjusting the scroll timeout.The new reactive references and helper functions improve the component's functionality and readability. The
scrollToLatestFieldfunction provides a good UX improvement by automatically scrolling to newly added fields.Consider reducing the scroll timeout from 500ms to improve responsiveness:
function scrollToLatestField() { setTimeout(() => { document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) - }, 500); + }, 300); }
675-756: Refactor duplicate click handlers for better maintainability.There's duplicate logic for toggling field visibility in two click handlers. Consider extracting this into a single method.
+ const toggleLinkFieldVisibility = (field: Field) => { + if (getColumnOfField(field)?.uidt === 'Links') { + field.show = !field.show + toggleFieldVisibility(field.show, field) + } + } @click=" () => { if (isLocked || getColumnOfField(field)?.uidt === 'Links') return field.show = !field.show toggleFieldVisibility(field.show, field) } " ... - @click=" - () => { - if (getColumnOfField(field)?.uidt === 'Links') { - field.show = !field.show - toggleFieldVisibility(field.show, field) - } - } - " + @click="() => toggleLinkFieldVisibility(field)"packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (3)
60-73: Consider improving error handling in async computed propertyThe
relatedModelcomputed property could benefit from better error handling and race condition prevention:
- Missing error handling for failed table lookups
- Potential race condition if multiple calls to
loadTables()occur simultaneouslyConsider this improvement:
const relatedModel = computedAsync(async () => { const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id if (fkRelatedModelId) { - let table = tables.value.find((t) => t.id === fkRelatedModelId) - - if (!table) { - await loadTables() - table = tables.value.find((t) => t.id === fkRelatedModelId) - } - return table + try { + let table = tables.value.find((t) => t.id === fkRelatedModelId) + if (!table) { + await loadTables() + table = tables.value.find((t) => t.id === fkRelatedModelId) + if (!table) { + throw new Error(`Table with id ${fkRelatedModelId} not found`) + } + } + return table + } catch (error) { + console.error('Failed to load related model:', error) + return null + } } return null })
145-176: Optimize watchers and focus managementA few improvements could enhance the component's reactivity handling:
- Replace setTimeout with nextTick for focus management
- Optimize search filtering with debounce
Consider these improvements:
+import { nextTick } from 'vue' +import { debounce } from 'lodash-es' -watch([relatedModel, searchField], async () => { +const updateFilteredColumns = debounce(async () => { if (relatedModel.value) { const columns = metas.value[relatedModel.value.id].columns filteredColumns.value = columns .filter((c) => !isSystemColumn(c) && !isLinksOrLTAR(c)) .filter((c) => c?.title?.toLowerCase().includes(searchField.value?.toLowerCase())) } -}) +}, 300) + +watch([relatedModel, searchField], updateFilteredColumns) watch(isInSearchMode, (val) => { if (val) { - setTimeout(() => { + nextTick(() => { refSearchField.value?.focus() - }, 500) }) } })
178-277: Enhance accessibility and clean up unused codeThe template could benefit from improved accessibility and code cleanup:
- Missing aria-labels on interactive elements
- Commented-out drag functionality code
Consider these improvements:
- class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100" + class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100" + role="button" + :aria-label="`Add lookup field ${field.title}`" @click.stop="selectedFields[field.id] = !selectedFields[field.id]" > - <!-- <component :is="iconMap.drag" class="cursor-move !h-3.75 text-gray-600 mr-1" /> -->Also, since the drag functionality appears to be unused, consider removing the Draggable component and related code if it's not planned for future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (6)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
451-451: LGTM! Smart dropdown behavior.
The auto-close condition based on openSubmenusCount prevents accidental closing when interacting with submenus, improving the user experience.
Line range hint 497-615: LGTM! Enhanced UI/UX for better usability.
The improvements include:
- Better visual hierarchy with proper spacing and borders
- Intuitive search field with integrated show/hide all columns switch
- Improved scrollbar styling for better visual consistency
767-807: LGTM! Clean interface for field management.
The improvements provide:
- Intuitive new field creation workflow
- Clear visual feedback for system fields visibility
- Proper event handling and state management
827-830: LGTM! Enhanced visual feedback.
The style improvements provide better visual feedback for user interactions while maintaining consistency with the design system.
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (2)
1-34: LGTM! Component setup follows Vue 3 best practices
The component setup is well-structured with proper TypeScript integration, clear prop definitions, and appropriate store/meta injections.
1-277: Verify alignment with PR objectives
The component successfully implements the fields menu improvement objectives:
- Provides a clean UI for lookup field creation
- Implements quick lookup creation functionality
- Handles scrolling within the fields menu appropriately
However, please verify the following:
- Integration with the parent FieldsMenu component
- Performance impact of the new lookup creation workflow
✅ Verification successful
Verified implementation aligns with PR objectives
The component has been successfully integrated and shows no performance concerns:
-
Integration with FieldsMenu is properly implemented:
- Correctly embedded within FieldsMenu.vue with proper props and event handling
- Lookup creation events are properly tracked via
lookupDropdownsTickle - Parent menu state is maintained via
openSubmenusCount
-
Performance is well-optimized:
- Watchers are properly scoped and have clear cleanup paths
- Search filtering is efficiently implemented with debounced input
- Column filtering uses simple boolean operations
The implementation provides a clean and efficient interface for lookup field creation while maintaining good performance characteristics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check integration points with FieldsMenu
rg -l "AddLookupsDropdown" --type vue
# Check for potential performance bottlenecks
ast-grep --pattern 'watch($$$)' --lang vue
Length of output: 217
Script:
#!/bin/bash
# Check integration points with FieldsMenu (using generic text search)
rg -l "AddLookupsDropdown"
# Check for potential performance bottlenecks (using typescript language)
ast-grep --pattern 'watch($$$)' --lang typescript
Length of output: 55253
Script:
#!/bin/bash
# Check integration with FieldsMenu
rg -C 5 "AddLookupsDropdown" packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue
# Check for watchers in AddLookupsDropdown
rg -C 5 "watch\(" packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
Length of output: 2054
| const createLookups = async () => { | ||
| if (!hasSelectedFields.value) { | ||
| return | ||
| } | ||
| try { | ||
| isLoading.value = true | ||
| const bulkOpsCols: { | ||
| op: 'add' | ||
| column: ColumnType | ||
| }[] = [] as any | ||
| const currIndex = meta.value?.columns?.length ?? 0 | ||
| for (const [k] of Object.entries(selectedFields.value).filter(([, v]) => v)) { | ||
| const lookupCol = metas.value[relatedModel.value?.id].columns.find((c) => c.id === k) | ||
| const index = filteredColumns.value.findIndex((c) => c.id === k) | ||
| const tempCol = { | ||
| uidt: UITypes.Lookup, | ||
| fk_lookup_column_id: k, | ||
| fk_relation_column_id: column.value.id, | ||
| lookupTableTitle: relatedModel.value?.title, | ||
| lookupColumnTitle: lookupCol?.title || lookupCol.column_name, | ||
| table_name: meta.value?.table_name, | ||
| title: `${lookupCol?.title} from ${relatedModel.value?.title}`, | ||
| view_id: activeView.value?.id, | ||
| order: currIndex + index, | ||
| column_name: `${lookupCol?.title} from (${relatedModel.value?.title})`, | ||
| column_order: { | ||
| order: currIndex + index, | ||
| view_id: activeView.value?.id, | ||
| }, | ||
| } | ||
| const newColName = generateUniqueColumnName({ | ||
| formState: tempCol, | ||
| metaColumns: relatedModel.value?.columns, | ||
| tableExplorerColumns: meta.value?.columns, | ||
| }) | ||
| bulkOpsCols.push({ | ||
| op: 'add', | ||
| column: { | ||
| ...tempCol, | ||
| title: newColName, | ||
| }, | ||
| }) | ||
| } | ||
| await $api.dbTableColumn.bulk(meta.value?.id, { | ||
| hash: columnsHash.value, | ||
| ops: bulkOpsCols, | ||
| }) | ||
| await getMeta(meta?.value?.id as string, true) | ||
| isOpened.value = false | ||
| selectedFields.value = {} | ||
| emit('created') | ||
| } catch (e) { | ||
| console.error(e) | ||
| } finally { | ||
| isLoading.value = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and reducing method complexity
The createLookups method has several areas for improvement:
- Basic error handling that only logs to console
- Long method that could be split into smaller functions
- Potential for duplicate column names
Consider these improvements:
- Extract the column creation logic:
const createLookupColumn = (lookupCol: ColumnType, index: number) => {
const newColName = generateUniqueColumnName({
formState: {
title: `${lookupCol?.title} from ${relatedModel.value?.title}`,
// ... other properties
},
metaColumns: relatedModel.value?.columns,
tableExplorerColumns: meta.value?.columns,
})
return {
op: 'add',
column: {
uidt: UITypes.Lookup,
// ... other properties
title: newColName,
},
}
}- Improve error handling:
} catch (e) {
- console.error(e)
+ const errorMessage = e instanceof Error ? e.message : 'Unknown error occurred'
+ console.error('Failed to create lookup fields:', errorMessage)
+ // Consider using a toast or notification system
+ throw new Error(`Failed to create lookup fields: ${errorMessage}`)
}
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
6ce664d to
d0a9f8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
675-678: Improve field visibility toggle logicThe field visibility toggle logic now includes a special case for 'Links' type fields. Consider extracting this logic into a computed property or method for better maintainability.
-if (isLocked || getColumnOfField(field)?.uidt === 'Links') return +const canToggleFieldVisibility = computed(() => !isLocked && getColumnOfField(field)?.uidt !== 'Links') + +if (!canToggleFieldVisibility.value) return
747-756: Simplify click handler logicThe click handler for the NcSwitch component could be simplified by combining it with the parent click handler since they perform the same operation for 'Links' type fields.
-@click=" - () => { - if (getColumnOfField(field)?.uidt === 'Links') { - field.show = !field.show - toggleFieldVisibility(field.show, field) - } - } -" +@click="toggleFieldVisibility(field.show, field)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
🔇 Additional comments (6)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (6)
429-442: LGTM: New reactive variables and helper functions look good!
The new additions improve the component's functionality:
addColumnDropdownfor managing the new field creation UIgetColumnOfFieldutility function for field-column mappingopenSubmenusCountfor managing nested dropdownsscrollToLatestFieldfor better UX after field creation
Line range hint 497-615: UI/UX Improvements: Enhanced search and visibility controls
The search field and column visibility toggle are well-integrated. The improvements include:
- Better styling with proper padding and borders
- Integrated search icon
- Column visibility toggle in the search input suffix
767-807: LGTM: Well-implemented field creation UI
The new field creation UI is well-structured:
- Clear visual hierarchy with the "New Field" button
- Proper dropdown placement
- Good separation of concerns with LazySmartsheetColumnEditOrAddProvider
- Smooth UX with auto-scroll to the new field
827-830: LGTM: Improved button styling
The styling improvements for system fields and show all fields buttons are well-implemented:
- Consistent text size and color
- Clear hover states
- Proper spacing and alignment
451-451: Verify the dropdown auto-close behavior
The dropdown's auto-close behavior is now tied to openSubmenusCount, which prevents accidental closing when submenus are open. However, we should ensure this doesn't lead to any edge cases where the dropdown remains stuck open.
✅ Verification successful
Auto-close behavior is properly implemented with safeguards
The implementation of the auto-close behavior in the FieldsMenu component is well-designed with proper safeguards:
- The
openSubmenusCountref tracks the number of open submenus, which is incremented/decremented when submenus are opened/closed via the@update:is-openedevent. - The NcDropdown component only auto-closes when
openSubmenusCount === 0, preventing accidental closing when submenus are active. - The NcDropdown base component has built-in click-outside handling that respects the auto-close prop.
- The implementation includes proper cleanup as submenus are closed, ensuring the counter stays accurate.
The code shows no potential edge cases where the dropdown could get stuck open:
- The counter is properly managed through event handling
- The base dropdown component has escape key handling
- Click-outside behavior is properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar dropdown patterns to verify consistency
ast-grep --pattern 'NcDropdown {
$$$
:auto-close="$_"
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Search for NcDropdown usage patterns with auto-close
rg -U "NcDropdown.*auto-close" -A 5 -B 5
# Search for openSubmenusCount usage
rg "openSubmenusCount"
# Search for similar dropdown components
fd "Dropdown" --type f
Length of output: 4542
Script:
#!/bin/bash
# Let's examine the NcDropdown component implementation
cat packages/nc-gui/components/nc/Dropdown.vue
# Let's also check the FieldsMenu implementation for context
cat packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue
Length of output: 30995
683-702: Verify lookup dropdown integration
The integration of SmartsheetToolbarAddLookupsDropdown looks good, but we should verify:
- The lookup dropdown tickle mechanism for updates
- The submenu count management
✅ Verification successful
Let me gather more information about the submenu count management.
Lookup dropdown integration is properly implemented
The verification shows that:
lookupDropdownsTickleis correctly initialized as a ref and consistently used for key updates when new lookups are createdopenSubmenusCountis properly managed:- Initialized as a ref
- Used to control auto-close behavior of the parent menu
- Correctly incremented/decremented based on submenu open state
The implementation follows a consistent pattern and maintains proper state management for both the lookup dropdown updates and submenu counting mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of lookupDropdownsTickle to ensure consistency
rg -A 2 "lookupDropdownsTickle"
# Search for similar submenu count patterns
ast-grep --pattern 'openSubmenusCount += $_ === true ? 1 : -1'
Length of output: 981
Script:
#!/bin/bash
# Search for openSubmenusCount usage and initialization
rg -A 2 "openSubmenusCount"
# Search for similar dropdown components to understand the pattern
ast-grep --pattern 'v-if="metas"
:key="$_"
:column="$_"
@created="$_"
@update:is-opened="$_"'
Length of output: 1196
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
d0a9f8a to
c385673
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (3)
6-14: Consider adding JSDoc comments for better documentationWhile the props and emits are well-defined, adding JSDoc comments would improve code documentation and provide better IDE support.
+/** Props interface for AddLookupsDropdown component */ interface Props { + /** The column to create lookups from */ column: ColumnType + /** Optional value prop for v-model binding */ value?: boolean } +/** Emits 'created' event when lookups are successfully created */ const emit = defineEmits([ 'created', ]);
168-174: Improve focus management implementationThe current implementation has two potential issues:
- Magic number (500ms) used for timeout
- No cleanup of setTimeout on component unmount
+const FOCUS_DELAY = 500 +let focusTimeout: number watch(isInSearchMode, (val) => { if (val) { - setTimeout(() => { + focusTimeout = setTimeout(() => { refSearchField.value?.focus() - }, 500) + }, FOCUS_DELAY) } }) + +onBeforeUnmount(() => { + if (focusTimeout) { + clearTimeout(focusTimeout) + } +})
178-256: Enhance accessibility for better user experienceConsider adding the following accessibility improvements:
- ARIA labels for interactive elements
- Keyboard navigation support
- Screen reader friendly messages
- <NcDropdown v-model:visible="isOpened" :disabled="column.uidt !== 'Links'" placement="right" :trigger="['click']"> + <NcDropdown + v-model:visible="isOpened" + :disabled="column.uidt !== 'Links'" + placement="right" + :trigger="['click']" + aria-label="Add lookup fields dropdown" + > <slot /> <template #overlay> - <div class="flex flex-col !rounded-t-lg overflow-hidden w-[256px]"> + <div + class="flex flex-col !rounded-t-lg overflow-hidden w-[256px]" + role="dialog" + aria-label="Lookup fields selection" + >tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts (1)
93-93: Consider using a more unique or test-specific selector.Relying on
.nc-switch.size-xsmallmight lead to brittle tests if other elements introduce similar classes. Adata-testidattribute is often more robust and descriptive.packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (12)
438-442: Scroll performance could improve with requestAnimationFrame.Using
setTimeoutis fine for now, but if we anticipate frequent calls, consider usingrequestAnimationFrameor a similar approach for smoother scrolling.
503-503: Maintain consistent styling.Ensure that
w-80is the intended fixed width for this container. Sometimes using responsive classes (e.g.,max-w-*) can provide better adaptability.
602-602: Avoid overly deep event nesting.
@click.stopis useful, but watch out for nested spamming of.stop. Overuse can make debugging click behavior more difficult.
609-614: Combine common scroll logic.Using
<div class="nc-scrollbar-thin">inside the suffix is a neat approach, but check if a shared reusable component might reduce duplication across the codebase.
618-621: Memory usage considerations.Large
.flexcontainers withmax-hmight cause performance issues if the element list grows significantly. Consider virtualization (e.g., Vue Virtual Scroll) for extremely large field lists.
653-653: Hover states for locked vs. unlocked.
.hover:bg-gray-100is fine, but ensure consistent styling for locked fields. Possibly add a tooltip explaining why the field is locked rather than only disabling hover.
675-675: Conditional logic clarity.When toggling visibility, skipping
Linksfields is helpful. Consider logging or visually indicating why a field is not toggled for debugging and user feedback.
706-715: Keep toggling actions consistent across styling.We see separate toggles for
bold,italic, etc. If the interactions are similar, consider abstracting them into a single utility function or composable for maintainability.
732-741: Consolidate underline toggling.As with bold/italic, reuse a shared approach to reduce duplication.
749-756: Avoid event overshadowing.When
@clicktriggers, consider merging logic with the@changeevent if feasible to prevent out-of-sync states between user clicks and component updates.
770-783: System fields toggle.
.nc-fields-show-system-fieldstoggles system fields. Document or comment how this interacts withshowSystemFieldfor future maintainers.
827-830: Maintain a consistent color palette.These Tailwind utility classes adapt well, but ensure
.hover:(!text-gray-800 bg-gray-200)aligns with your design tokens for a cohesive UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/components/nc/Switch.vue
🔇 Additional comments (14)
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (3)
1-5: LGTM: Proper setup with TypeScript and necessary imports
The component is well-structured with appropriate imports and TypeScript integration.
77-143: Consider improving error handling and reducing method complexity
The previous review comments about refactoring the createLookups method and improving error handling are still applicable.
258-277: LGTM: Well-structured styles with proper transitions
The styles are well-organized with proper scoping and transitions.
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (11)
429-430: Initialization looks correct.
Defining addColumnDropdown as a reactive reference with a default false state is a clear approach for controlling UI toggles.
431-433: Check for null or undefined column references.
getColumnOfField directly accesses field.fk_column_id. Ensure that field.fk_column_id is always defined or handle potential null/undefined cases gracefully.
435-436: Track nested submenus carefully.
openSubmenusCount and lookupDropdownsTickle should be consistently updated. Consider adding tests to ensure these counters remain in sync when submenus open/close.
451-451: Validate auto-close condition.
:auto-close="openSubmenusCount === 0" ensures the dropdown remains open if submenus are open. Confirm that all submenus increment/decrement openSubmenusCount appropriately to avoid stuck states.
497-497: Tailwind utility approach is consistent.
Using utility classes for layout and design is clear and maintains consistency with the rest of the codebase.
607-607: Responsive placeholders.
placeholder="$t('placeholder.searchFields')" is good. Consider verifying that the translation string is present in all languages to avoid fallback issues.
682-702: Reusable sub-component approach.
<SmartsheetToolbarAddLookupsDropdown> is a good pattern for modular UI. Ensure that future changes to field or lookupDropdownsTickle are always tracked properly for reactivity.
747-747: Check new size prop usage.
size="xxsmall" is newly introduced. Confirm that the Switch component styling is verified across multiple themes or screen sizes.
766-766: Keep code sections separated.
This blank line helps visually separate logic blocks. It’s readable, so no issues here.
767-767: Guard user queries.
<div v-if="!filterQuery"...> ensures no extra actions are displayed while searching. Verify this doesn’t hide essential UI if a user accidentally leaves partial text in the field.
785-807: Lazy-loading the column add form is a good approach.
Ensuring the form is loaded only when needed can improve performance. Watch for race conditions if multiple add column sessions open simultaneously.
| '!text-primary': field.italic, | ||
| }" | ||
| class="!rounded-x-none !border-x-0 !w-5 !h-5" | ||
| size="xxsmall" | ||
| type="secondary" | ||
| :shadow="false" | ||
| class="!w-5 !h-5 hover:!bg-gray-200 active:!bg-gray-300 relative" | ||
| size="xsmall" | ||
| type="text" | ||
| :disabled="isLocked" | ||
| @click.stop="toggleFieldStyles(field, 'italic', !field.italic)" | ||
| > | ||
| <component :is="iconMap.italic" class="!w-3 !h-3" /> | ||
| <component :is="iconMap.italic" class="!w-3.5 !h-3.5" /> | ||
| <div v-if="field.italic" class="bg-primary w-1 h-1 rounded-full absolute top-0 right-0" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated style toggling.
Same suggestion as with bold styling. Combining repeated toggles into a single function streamlines future maintenance.
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
c385673 to
c35cd63
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (3)
1-5: Consider optimizing type importsThe type imports can be grouped together for better organization and clarity.
-import { type ColumnType, UITypes, isLinksOrLTAR, isSystemColumn, isVirtualCol } from 'nocodb-sdk' +import type { ColumnType } from 'nocodb-sdk' +import { UITypes, isLinksOrLTAR, isSystemColumn, isVirtualCol } from 'nocodb-sdk'
39-54: Add TypeScript types to refs for better type safetyConsider adding explicit types to these refs for improved type safety and code maintainability.
-const columnsHash = ref() +const columnsHash = ref<string>() -const isLoading = ref(false) +const isLoading = ref<boolean>(false) -const searchField = ref('') +const searchField = ref<string>('')
168-174: Replace setTimeout with nextTick for DOM updatesUsing
setTimeoutfor DOM updates can be replaced with Vue'snextTickfor better reliability.watch(isInSearchMode, (val) => { if (val) { - setTimeout(() => { + nextTick(() => { refSearchField.value?.focus() - }, 500) + }) } })packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
Line range hint
497-614: UI/UX improvements look good with a minor suggestion.The search field and toolbar layout improvements enhance usability. However, consider extracting the search field into a separate component for better maintainability.
- <a-input - ref="fieldsMenuSearchRef" - v-model:value="filterQuery" - :placeholder="$t('placeholder.searchFields')" - class="nc-toolbar-dropdown-search-field-input !border-none !shadow-none !pb-1.5 !pt-2.5" - > - <template #prefix> <GeneralIcon icon="search" class="nc-search-icon h-3.5 w-3.5 mr-1 ml-2" /> </template> - <template #suffix> - <div class="nc-scrollbar-thin" style="scrollbar-gutter: stable;"> - <NcSwitch v-model:checked="showAllColumns" size="xsmall" class="!mr-1" /> - </div> - </template> - </a-input> + <FieldsMenuSearch + ref="fieldsMenuSearchRef" + v-model:value="filterQuery" + v-model:show-all="showAllColumns" + />
767-807: New field creation UI looks good with a suggestion.The new field creation dropdown is well implemented. However, consider adding a loading state while the new field is being created.
<LazySmartsheetColumnEditOrAddProvider v-if="addColumnDropdown" ref="editOrAddProviderRef" + v-model:loading="isCreatingField" @submit="addColumnDropdown = false; scrollToLatestField();" @cancel="addColumnDropdown = false" @click.stop @keydown.stop />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (7)
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (2)
77-143: Previous review comments about error handling and method complexity are still applicable
The suggestions about improving error handling and reducing method complexity from the previous review remain relevant.
258-277: LGTM! Clean and well-organized styles
The styles are properly scoped and the transitions are well-defined.
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (5)
429-442: LGTM: New reactive state management looks good.
The new reactive variables and helper functions are well-implemented:
addColumnDropdownfor managing the new field creation UIgetColumnOfFieldutility function for field metadata lookupopenSubmenusCountfor tracking open submenusscrollToLatestFieldfor improved UX after field creation
683-702: LGTM: Lookup fields dropdown integration.
The new SmartsheetToolbarAddLookupsDropdown component is well integrated with proper event handling for submenu state management.
827-830: LGTM: Style improvements.
The button hover states are well defined and consistent with the design system.
451-451: Verify the auto-close behavior.
The dropdown's auto-close behavior now depends on open submenus count. This prevents accidental closing when interacting with nested dropdowns.
✅ Verification successful
Auto-close behavior implementation is correct and safe
The implementation of the auto-close behavior is properly handled:
openSubmenusCountis initialized as a ref with value 0- It's incremented/decremented correctly when submenus are opened/closed using
openSubmenusCount += $event === true ? 1 : -1 - The dropdown's
:auto-closeprop is bound toopenSubmenusCount === 0, ensuring it only auto-closes when no submenus are open
This prevents accidental closing of the main dropdown while interacting with nested menus, which is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential issues with dropdown closing behavior
ast-grep --pattern 'openSubmenusCount += $event === true ? 1 : -1'
Length of output: 68
Script:
#!/bin/bash
# Search for openSubmenusCount usage in the component
rg "openSubmenusCount" packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue -B 2 -A 2
Length of output: 834
675-679: Verify the field visibility toggle logic.
The field visibility toggle now has special handling for 'Links' type fields. Ensure this doesn't break existing functionality.
✅ Verification successful
Based on the search results, I can verify the field visibility toggle logic:
The Links field visibility toggle implementation is correct and safe
The code shows proper handling of Links field visibility:
-
The toggle visibility check is implemented in two places:
- Main click handler checks
if (isLocked || getColumnOfField(field)?.uidt === 'Links') return - Switch click handler has special handling
if (getColumnOfField(field)?.uidt === 'Links')
- Main click handler checks
-
The toggle functionality is consistent:
- Both handlers update
field.showstate - Both call
toggleFieldVisibility(field.show, field)to persist changes - The
toggleFieldVisibilityfunction properly handles undo/redo operations
- Both handlers update
-
The Links field type is a standard UIType used throughout the codebase for relationship columns, as evidenced by its usage in test files.
The special handling for Links fields does not break existing functionality since it follows the same pattern as other field types but adds an extra validation layer specific to Links fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent handling of Links fields
ast-grep --pattern 'if ($condition) {
field.show = !field.show
toggleFieldVisibility(field.show, field)
}'
Length of output: 112
Script:
#!/bin/bash
# Search for field visibility toggle patterns and Links field handling
rg -U "field.show.*toggleFieldVisibility" -A 5 -B 5
# Search for Links field type handling
rg "uidt.*Links" -A 5 -B 5
# Search for toggleFieldVisibility function definition
ast-grep --pattern 'toggleFieldVisibility($$$) {
$$$
}'
Length of output: 74260
Script:
#!/bin/bash
# Search for field visibility toggle implementation
rg -U "toggleFieldVisibility" -A 10 -B 10
# Search for field show property usage
rg -U "field\.show" -A 5 -B 5
# Search for Links field type handling
rg "uidt.*Links" -A 5 -B 5
Length of output: 99469
| const relatedModel = computedAsync(async () => { | ||
| const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id | ||
| if (fkRelatedModelId) { | ||
| let table = tables.value.find((t) => t.id === fkRelatedModelId) | ||
| if (!table) { | ||
| await loadTables() | ||
| table = tables.value.find((t) => t.id === fkRelatedModelId) | ||
| } | ||
| return table | ||
| } | ||
| return null | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in relatedModel computed property
The async computation lacks proper error handling and could have race conditions.
const relatedModel = computedAsync(async () => {
+ try {
const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id
if (fkRelatedModelId) {
let table = tables.value.find((t) => t.id === fkRelatedModelId)
if (!table) {
await loadTables()
table = tables.value.find((t) => t.id === fkRelatedModelId)
}
return table
}
return null
+ } catch (error) {
+ console.error('Failed to load related model:', error)
+ return null
+ }
})📝 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 relatedModel = computedAsync(async () => { | |
| const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id | |
| if (fkRelatedModelId) { | |
| let table = tables.value.find((t) => t.id === fkRelatedModelId) | |
| if (!table) { | |
| await loadTables() | |
| table = tables.value.find((t) => t.id === fkRelatedModelId) | |
| } | |
| return table | |
| } | |
| return null | |
| }) | |
| const relatedModel = computedAsync(async () => { | |
| try { | |
| const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id | |
| if (fkRelatedModelId) { | |
| let table = tables.value.find((t) => t.id === fkRelatedModelId) | |
| if (!table) { | |
| await loadTables() | |
| table = tables.value.find((t) => t.id === fkRelatedModelId) | |
| } | |
| return table | |
| } | |
| return null | |
| } catch (error) { | |
| console.error('Failed to load related model:', error) | |
| return null | |
| } | |
| }) |
| interface Props { | ||
| column: ColumnType | ||
| value?: boolean | ||
| } | ||
| const emit = defineEmits([ | ||
| 'created', | ||
| ]); | ||
There was a problem hiding this comment.
💡 Codebase verification
Remove unused 'value' prop and its related code
The value prop and its corresponding useVModel hook are not being used anywhere in the component. The dropdown visibility is controlled by the isOpened model value instead. You can safely remove:
- The
value?: booleanfrom Props interface - The
value = useVModel(props, 'value')line
🔗 Analysis chain
Verify the necessity of the 'value' prop
The value prop appears to be only used with useVModel but its purpose isn't clear. Consider removing it if not essential or adding documentation to explain its usage.
Let me check how the component is used and verify the usage of useVModel with the value prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the value prop in parent components
rg "AddLookupsDropdown.*value=" -A 5
Length of output: 38
Script:
#!/bin/bash
# Search for any usage of AddLookupsDropdown component
rg -l "AddLookupsDropdown"
# Search for useVModel usage in the component
cat "packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue"
Length of output: 8333
| <div | ||
| :key="field.id" | ||
| :data-testid="`nc-lookup-add-menu-${field.title}`" | ||
| class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100" | ||
| @click.stop="selectedFields[field.id] = !selectedFields[field.id]" | ||
| > | ||
| <!-- <component :is="iconMap.drag" class="cursor-move !h-3.75 text-gray-600 mr-1" /> --> | ||
| <div class="flex flex-row items-center w-full cursor-pointer truncate ml-1 py-[2px] pr-2"> | ||
| <component :is="getIcon(field)" class="!w-3.5 !h-3.5 !text-gray-500" /> | ||
| <NcTooltip class="flex-1 pl-1 pr-2 truncate" show-on-truncate-only> | ||
| <template #title> | ||
| {{ field.title }} | ||
| </template> | ||
| <template #default>{{ field.title }}</template> | ||
| </NcTooltip> | ||
|
|
||
| <NcCheckbox v-model:checked="selectedFields[field.id]" size="default" /> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for keyboard navigation and screen readers
The click handlers and keyboard navigation need improvement for better accessibility.
<div
:key="field.id"
:data-testid="`nc-lookup-add-menu-${field.title}`"
class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100"
+ role="menuitem"
+ tabindex="0"
+ @keydown.enter="selectedFields[field.id] = !selectedFields[field.id]"
+ @keydown.space.prevent="selectedFields[field.id] = !selectedFields[field.id]"
@click.stop="selectedFields[field.id] = !selectedFields[field.id]"
>📝 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.
| <div | |
| :key="field.id" | |
| :data-testid="`nc-lookup-add-menu-${field.title}`" | |
| class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100" | |
| @click.stop="selectedFields[field.id] = !selectedFields[field.id]" | |
| > | |
| <!-- <component :is="iconMap.drag" class="cursor-move !h-3.75 text-gray-600 mr-1" /> --> | |
| <div class="flex flex-row items-center w-full cursor-pointer truncate ml-1 py-[2px] pr-2"> | |
| <component :is="getIcon(field)" class="!w-3.5 !h-3.5 !text-gray-500" /> | |
| <NcTooltip class="flex-1 pl-1 pr-2 truncate" show-on-truncate-only> | |
| <template #title> | |
| {{ field.title }} | |
| </template> | |
| <template #default>{{ field.title }}</template> | |
| </NcTooltip> | |
| <NcCheckbox v-model:checked="selectedFields[field.id]" size="default" /> | |
| </div> | |
| <div | |
| :key="field.id" | |
| :data-testid="`nc-lookup-add-menu-${field.title}`" | |
| class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100" | |
| role="menuitem" | |
| tabindex="0" | |
| @keydown.enter="selectedFields[field.id] = !selectedFields[field.id]" | |
| @keydown.space.prevent="selectedFields[field.id] = !selectedFields[field.id]" | |
| @click.stop="selectedFields[field.id] = !selectedFields[field.id]" | |
| > | |
| <!-- <component :is="iconMap.drag" class="cursor-move !h-3.75 text-gray-600 mr-1" /> --> | |
| <div class="flex flex-row items-center w-full cursor-pointer truncate ml-1 py-[2px] pr-2"> | |
| <component :is="getIcon(field)" class="!w-3.5 !h-3.5 !text-gray-500" /> | |
| <NcTooltip class="flex-1 pl-1 pr-2 truncate" show-on-truncate-only> | |
| <template #title> | |
| {{ field.title }} | |
| </template> | |
| <template #default>{{ field.title }}</template> | |
| </NcTooltip> | |
| <NcCheckbox v-model:checked="selectedFields[field.id]" size="default" /> | |
| </div> |
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
c35cd63 to
cc6ba86
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
429-442: LGTM! Consider adding error handling for the scroll function.The new reactive variables and helper functions improve state management and user experience. The
scrollToLatestFieldfunction could benefit from error handling in case the element is not found.function scrollToLatestField() { setTimeout(() => { - document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) + const lastField = document.querySelector('.nc-fields-menu-item:last-child') + if (lastField) { + lastField.scrollIntoView({ behavior: 'smooth' }) + } }, 500); }
675-679: Refactor duplicated visibility toggle logic.The visibility toggle logic is duplicated between the click handlers. Consider extracting it into a reusable method.
+const toggleLinkFieldVisibility = (field: Field) => { + if (getColumnOfField(field)?.uidt === 'Links') { + field.show = !field.show + toggleFieldVisibility(field.show, field) + } +} -@click=" - () => { - if (isLocked || (getColumnOfField(field)?.uidt === 'Links' && !isLocalMode)) return - field.show = !field.show - toggleFieldVisibility(field.show, field) - } -" +@click=" + () => { + if (isLocked || (getColumnOfField(field)?.uidt === 'Links' && !isLocalMode)) return + toggleLinkFieldVisibility(field) + } +" -@click=" - () => { - if (getColumnOfField(field)?.uidt === 'Links') { - field.show = !field.show - toggleFieldVisibility(field.show, field) - } - } -" +@click="() => toggleLinkFieldVisibility(field)"Also applies to: 750-757
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (4)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
451-451: LGTM! Smart dropdown behavior improvement.
The dynamic auto-close behavior based on open submenus count prevents accidental closure during submenu interactions.
Line range hint 497-614: LGTM! Clean and intuitive UI improvements.
The enhanced styling and layout create a better visual hierarchy. The integration of the show/hide all switch within the search field is a smart space-saving design choice.
768-807: LGTM! Well-implemented field management controls.
The system fields toggle and new field creation button are well-implemented with clear visual feedback and efficient lazy loading.
827-830: LGTM! Enhanced visual feedback.
The improved button styling with hover states provides clear visual feedback for user interactions.
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
cc6ba86 to
ce60707
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (1)
438-442: Consider using a more reliable method for scrolling to new fields.The current implementation using setTimeout with a fixed 500ms delay might be unreliable:
- The element might not be ready after 500ms
- The delay might be unnecessarily long in some cases
Consider using a MutationObserver or Vue's nextTick:
-function scrollToLatestField() { - setTimeout(() => { - document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) - }, 500); -} +function scrollToLatestField() { + nextTick(() => { + const observer = new MutationObserver((mutations, obs) => { + const element = document.querySelector('.nc-fields-menu-item:last-child') + if (element) { + element.scrollIntoView({ behavior: 'smooth' }) + obs.disconnect() + } + }) + + observer.observe(document.body, { + childList: true, + subtree: true + }) + + // Fallback cleanup after 2s + setTimeout(() => observer.disconnect(), 2000) + }) +}packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (2)
174-176: Replace setTimeout with nextTick for focus managementUsing
setTimeoutwith a fixed delay for focus management is not reliable. Use Vue'snextTickinstead.- setTimeout(() => { + nextTick(() => { refSearchField.value?.focus() - }, 500) + })
189-189: Consider consistent transition durationsThe transition duration is asymmetric (500ms enter, 0ms leave) which might create a jarring user experience.
- <transition name="slide-out" :duration="{ enter: 500, leave: 0 }"> + <transition name="slide-out" :duration="{ enter: 250, leave: 250 }">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (11)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
451-451: LGTM: Smart dropdown auto-close behavior
Good improvement to prevent accidental closing when submenus are open.
602-615: LGTM: Improved menu layout and scrolling behavior
Good improvements:
- Stable scrollbar gutter prevents layout shifts
- Consistent min/max height constraints
- Clean search field styling
Also applies to: 618-621
768-807: LGTM: Well-implemented field management UI
Good implementation with:
- Lazy loading of the column edit provider
- Clear visual feedback for system fields visibility
- Intuitive new field creation flow
827-830: LGTM: Consistent button styling
Good use of Tailwind classes for consistent button styling and hover states.
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (4)
8-9: Remove unused 'value' prop and its related code
The value prop and useVModel hook are not being used as the dropdown visibility is controlled by isOpened model value.
interface Props {
column: ColumnType
- value?: boolean
disabled?: boolean
}
-const value = useVModel(props, 'value')Also applies to: 38-38
63-77: Improve error handling in relatedModel computed property
While the initiatedTableLoad flag helps prevent multiple loads, the async computation still lacks proper error handling.
const relatedModel = computedAsync(async () => {
+ try {
const fkRelatedModelId = (column.value.colOptions as any)?.fk_related_model_id
if (fkRelatedModelId) {
let table = tables.value.find((t) => t.id === fkRelatedModelId)
if (!table && !initiatedTableLoad.value) {
await loadTables()
table = tables.value.find((t) => t.id === fkRelatedModelId)
initiatedTableLoad.value = true
}
return table
}
return null
+ } catch (error) {
+ console.error('Failed to load related model:', error)
+ return null
+ }
})81-147: Improve error handling and reduce method complexity
The createLookups method could be improved in several ways:
- Basic error handling only logs to console without user feedback
- Long method that could be split into smaller functions
- Column creation logic could be extracted
Consider these improvements:
+const createLookupColumn = (lookupCol: ColumnType, index: number) => {
+ const tempCol = {
+ uidt: UITypes.Lookup,
+ // ... other properties
+ }
+
+ const newColName = generateUniqueColumnName({
+ formState: tempCol,
+ metaColumns: relatedModel.value?.columns,
+ tableExplorerColumns: meta.value?.columns,
+ })
+
+ return {
+ op: 'add',
+ column: {
+ ...tempCol,
+ title: newColName,
+ },
+ }
+}
const createLookups = async () => {
try {
// ... existing code ...
} catch (e) {
- console.error(e)
+ const errorMessage = e instanceof Error ? e.message : 'Unknown error occurred'
+ console.error('Failed to create lookup fields:', errorMessage)
+ // Use your notification system to show error to user
+ useMessage().error(errorMessage)
}
}217-235: Improve accessibility for keyboard navigation and screen readers
The field selection items need better keyboard navigation support and ARIA attributes.
<div
:key="field.id"
:data-testid="`nc-lookup-add-menu-${field.title}`"
class="px-1 py-0.75 mx-2 flex flex-row items-center rounded-md hover:bg-gray-100"
+ role="menuitem"
+ tabindex="0"
+ @keydown.enter="selectedFields[field.id] = !selectedFields[field.id]"
+ @keydown.space.prevent="selectedFields[field.id] = !selectedFields[field.id]"
@click.stop="selectedFields[field.id] = !selectedFields[field.id]"
>packages/nc-gui/components/nc/Switch.vue (3)
6-6: LGTM: Type definition is properly extended
The addition of 'xxsmall' to the size prop type union is well-defined and maintains type safety.
59-59: LGTM: Class binding follows established pattern
The class binding for 'size-xxsmall' is correctly implemented and consistent with the existing pattern.
105-125: Consider accessibility implications of extremely small size
While the CSS implementation is technically correct and maintains proportions, the 'xxsmall' size (10px height) raises some concerns:
- The touch target size falls below recommended minimums (44x44px for mobile)
- The small size might affect visual clarity and usability
- The reduced size could make it difficult to click/tap accurately
Consider these improvements:
.size-xxsmall {
@apply h-2.5 min-w-[18px] leading-[12px];
+ // Add touch target padding for better accessibility
+ @apply md:cursor-pointer;
+
+ // Prevent xxsmall size on touch devices
+ @media (pointer: coarse) {
+ @apply h-3.5 min-w-[26px];
+
+ :deep(.ant-switch-handle) {
+ @apply h-[10px] w-[10px];
+ }
+ }
:deep(.ant-switch-handle) {
@apply h-[6px] w-[6px] top-[2px] left-[calc(100%_-_16px)];
}
// ... rest of the implementation
}Let's verify if this component is used in touch-interactive contexts:
| if (isLocked || (getColumnOfField(field)?.uidt === 'Links' && !isLocalMode)) return | ||
| field.show = !field.show | ||
| toggleFieldVisibility(field.show, field) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate click handlers for field visibility toggle.
The visibility toggle logic is duplicated between the div click handler and switch click handler, which could lead to maintenance issues and inconsistent behavior.
Consider consolidating the logic into a single method:
+const toggleFieldVisibilityIfAllowed = (field: Field) => {
+ if (isLocked || (getColumnOfField(field)?.uidt === 'Links' && !isLocalMode)) return
+ field.show = !field.show
+ toggleFieldVisibility(field.show, field)
+}
-@click="
- () => {
- if (isLocked || (getColumnOfField(field)?.uidt === 'Links' && !isLocalMode)) return
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
- }
-"
+@click="() => toggleFieldVisibilityIfAllowed(field)"
-@click="
- () => {
- if (getColumnOfField(field)?.uidt === 'Links') {
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
- }
- }
-"
+@click="() => toggleFieldVisibilityIfAllowed(field)"Also applies to: 750-757
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
ce60707 to
2555c85
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
438-442: Consider using a more reliable method for scrolling to the latest fieldThe current implementation using
setTimeoutmight be unreliable as it doesn't guarantee the DOM will be ready. Consider using Vue'snextTickor a mutation observer.function scrollToLatestField() { - setTimeout(() => { + nextTick(() => { document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) - }, 500); + }); }
444-449: Extract 'Links' as a constantThe string literal 'Links' should be extracted as a constant to improve maintainability and prevent typos.
+const COLUMN_TYPE = { + LINKS: 'Links' +} as const; + function handleFieldVisibilityClick(field: Field) { - if (getColumnOfField(field)?.uidt === 'Links') { + if (getColumnOfField(field)?.uidt === COLUMN_TYPE.LINKS) { field.show = !field.show toggleFieldVisibility(field.show, field) } }
Line range hint
665-714: Add keyboard accessibility for field itemsThe field items should be keyboard accessible for better accessibility. Consider adding appropriate ARIA attributes and keyboard navigation.
<div :data-testid="`nc-fields-menu-${field.title}`" class="nc-fields-menu-item pl-2 flex flex-row items-center rounded-md" + role="menuitem" + tabindex="0" :class="{ 'hover:bg-gray-100': !isLocked, }" + @keydown.enter="handleFieldVisibilityClick(field)" + @keydown.space.prevent="handleFieldVisibilityClick(field)" @click.stop >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
🔇 Additional comments (3)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
Line range hint 509-633: LGTM! UI improvements enhance usability
The changes improve the visual hierarchy and user experience with:
- Better organized layout with proper spacing
- Improved search functionality
- Well-handled scrollbar styling
773-812: LGTM! Well-implemented system fields toggle and new field button
The implementation:
- Uses appropriate icons for better visual feedback
- Has clean dropdown integration
- Follows Vue best practices
830-835: LGTM! Clean and maintainable styles
The style implementation:
- Uses proper CSS nesting
- Has clear hover states
- Follows BEM-like naming convention
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
2555c85 to
c6e4e2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
434-438: Consider using MutationObserver instead of setTimeout.The setTimeout approach might be unreliable if the DOM update takes longer than 500ms. Using a MutationObserver would be more robust.
-function scrollToLatestField() { - setTimeout(() => { - document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) - }, 500); -} +function scrollToLatestField() { + const observer = new MutationObserver((mutations, obs) => { + const lastField = document.querySelector('.nc-fields-menu-item:last-child') + if (lastField) { + lastField.scrollIntoView({ behavior: 'smooth' }) + obs.disconnect() + } + }) + observer.observe(document.body, { + childList: true, + subtree: true + }) +}
Line range hint
670-711: Consider lazy loading lookup dropdowns.The lookup dropdowns are rendered for all fields, which could impact performance with many fields. Consider lazy loading them only when needed.
-<SmartsheetToolbarAddLookupsDropdown - v-if="metas" +<SmartsheetToolbarAddLookupsDropdown + v-if="metas && meta?.columnsById?.[field.fk_column_id!]?.uidt === 'Links'"packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (3)
62-62: Document the purpose of initiatedTableLoadAdd a comment explaining that this flag prevents infinite loops during table loading.
+// Flag to prevent infinite loops during table loading const initiatedTableLoad = ref(false);
150-157: Add debouncing to search field updatesConsider debouncing the search field updates to prevent excessive filtering operations.
+const debouncedSearch = ref('') +watch(searchField, useDebounceFn((val) => { + debouncedSearch.value = val +}, 300)) -watch([relatedModel, searchField], async () => { +watch([relatedModel, debouncedSearch], async () => { if (relatedModel.value) { const columns = metas.value[relatedModel.value.id].columns filteredColumns.value = columns .filter((c) => !isSystemColumn(c) && !isLinksOrLTAR(c)) - .filter((c) => searchCompare([c?.title], searchField.value)) + .filter((c) => searchCompare([c?.title], debouncedSearch.value)) } })
266-269: Ensure consistent transition durationsThe transition duration in the template (500ms) differs from the CSS transition duration (250ms). Consider using a consistent duration across all transitions.
.slide-out-enter-active, .slide-out-leave-active { - transition: all 0.25s ease-out; + transition: all 0.5s ease-out; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/nc/Switch.vue
🔇 Additional comments (9)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
429-432: LGTM: Well-structured reactive state management.
The new reactive variables are appropriately named and serve clear purposes for managing dropdown states and component updates.
468-468: LGTM: Improved dropdown behavior and search functionality.
The auto-close behavior based on submenu state and the enhanced search input implementation with proper styling look good.
Also applies to: 514-638
822-827: LGTM: Consistent styling with design system.
The button styles and interactive states are well-defined and follow the application's design patterns.
440-454: 🛠️ Refactor suggestion
Consolidate field visibility toggle logic.
The visibility toggle logic is duplicated between conditionalToggleFieldVisibility and handleFieldVisibilityClick. Consider consolidating into a single method.
-function conditionalToggleFieldVisibility(field: Field) {
- if (isLocked.value || (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links' && !isLocalMode.value)) {
- return
- }
-
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
-}
-
-function handleFieldVisibilityClick(field: Field) {
- if (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links') {
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
- }
-}
+function toggleFieldVisibility(field: Field) {
+ const isLinksField = meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links'
+
+ if (isLocked.value || (isLinksField && !isLocalMode.value)) {
+ return
+ }
+
+ if (!isLinksField || (isLinksField && isLocalMode.value)) {
+ field.show = !field.show
+ toggleFieldVisibility(field.show, field)
+ }
+}Likely invalid or redundant comment.
packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue (4)
7-8: Remove unused 'value' prop and useVModel
The value prop and its corresponding useVModel hook are not being used in the component as the dropdown visibility is controlled by isOpened model value.
interface Props {
column: ColumnType
- value?: boolean
disabled?: boolean
}
-const value = useVModel(props, 'value')Also applies to: 39-39
64-78: Improve error handling in relatedModel computed property
The async computation lacks proper error handling and could have race conditions.
216-221: Improve accessibility for keyboard navigation and screen readers
The click handlers and keyboard navigation need improvement for better accessibility.
142-144: 🛠️ Refactor suggestion
Improve error handling with user feedback
Currently, errors are only logged to console. Consider adding user feedback using a toast or notification system.
} catch (e) {
console.error(e)
+ const errorMessage = e instanceof Error ? e.message : 'Unknown error occurred'
+ message.error(t('error.failedToCreateLookup', { error: errorMessage }))
}Likely invalid or redundant comment.
tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts (1)
80-80: Ensure class name consistency across the test suite.
Looks good. Please verify that other references to the old class name (.nc-dropdown-grid-add-column) have also been updated throughout the repository to avoid inconsistent selectors.
Use the following script to locate any remaining instances:
✅ Verification successful
Let me verify the class name consistency by searching for both the old and new class names to get a complete picture.
Let me search for any variations of the class name to ensure we catch all instances.
Let me try searching without file type restrictions since Vue files may not be recognized.
Let me try one more search focusing specifically on test files since this is a test-related change.
Class name change appears to be isolated to this test file
The search results show that the class name nc-dropdown-add-column is only used in the file tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts, and there are no other occurrences of the old class name nc-dropdown-grid-add-column in the codebase. This indicates that the class name change is isolated to this test file and doesn't require updates elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if `.nc-dropdown-grid-add-column` is used elsewhere.
rg "\.nc-dropdown-grid-add-column"
Length of output: 36
Script:
#!/bin/bash
# Search for both old and new class names
echo "Searching for new class name '.nc-dropdown-add-column':"
rg "\.nc-dropdown-add-column"
echo -e "\nSearching for variations of dropdown add column:"
rg -i "dropdown.*add.*column"
Length of output: 66126
Script:
#!/bin/bash
# Search for variations of dropdown add column classes
echo "Searching for dropdown add column class variations:"
rg -i "dropdown.*add.*column" --type ts --type vue
echo -e "\nSearching for specific class names:"
rg "nc-dropdown-add-column|nc-dropdown-grid-add-column" --type ts --type vue
Length of output: 387
Script:
#!/bin/bash
# Search for variations of dropdown add column classes without file type restrictions
echo "Searching for dropdown add column class variations:"
rg -i "dropdown.*add.*column"
echo -e "\nSearching for specific class names:"
rg "nc-dropdown-add-column|nc-dropdown-grid-add-column"
Length of output: 65890
Script:
#!/bin/bash
# Search for dropdown add column classes in test files
echo "Searching in test files:"
find . -type f -name "*.spec.ts" -exec grep -l "nc-dropdown.*add.*column" {} \;
Length of output: 206
| function switchToSearchMode() { | ||
| isInSearchMode.value = true | ||
| setTimeout(() => { | ||
| refSearchField.value?.focus() | ||
| }, 500) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up setTimeout on component unmount
The setTimeout in switchToSearchMode should be cleaned up to prevent memory leaks.
+const searchModeTimeout = ref<number>()
function switchToSearchMode() {
isInSearchMode.value = true
- setTimeout(() => {
+ searchModeTimeout.value = window.setTimeout(() => {
refSearchField.value?.focus()
}, 500)
}
+onBeforeUnmount(() => {
+ if (searchModeTimeout.value) {
+ window.clearTimeout(searchModeTimeout.value)
+ }
+})📝 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.
| function switchToSearchMode() { | |
| isInSearchMode.value = true | |
| setTimeout(() => { | |
| refSearchField.value?.focus() | |
| }, 500) | |
| } | |
| const searchModeTimeout = ref<number>() | |
| function switchToSearchMode() { | |
| isInSearchMode.value = true | |
| searchModeTimeout.value = window.setTimeout(() => { | |
| refSearchField.value?.focus() | |
| }, 500) | |
| } | |
| onBeforeUnmount(() => { | |
| if (searchModeTimeout.value) { | |
| window.clearTimeout(searchModeTimeout.value) | |
| } | |
| }) |
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
c6e4e2a to
9c504d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/nc-gui/components/nc/Dropdown.vue (1)
68-68: Avoid castingplacementtoany.Casting
placementtoanybypasses type-checking and can mask real issues if'right'is not properly handled by the<a-dropdown>component. Consider updating type definitions or confirming compatibility to ensure the code remains type-safe.packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
429-432: LGTM! Consider adding TypeScript types for better type safety.The new reactive refs are well-structured and serve clear purposes. However, consider adding explicit types for better maintainability:
-const addColumnDropdown = ref(false) -const openSubmenusCount = ref(0) -const lookupDropdownsTickle = ref(0) +const addColumnDropdown = ref<boolean>(false) +const openSubmenusCount = ref<number>(0) +const lookupDropdownsTickle = ref<number>(0)
Line range hint
679-720: Enhance accessibility with ARIA labels.While the UI improvements are good, consider adding ARIA labels for better accessibility:
-<div class="nc-fields-menu-item pl-2 flex flex-row items-center rounded-md" +<div + class="nc-fields-menu-item pl-2 flex flex-row items-center rounded-md" + role="menuitem" + :aria-label="field.title" + :aria-checked="field.show" -<component :is="iconMap.drag" class="!h-3.75 text-gray-600 mr-1" +<component + :is="iconMap.drag" + class="!h-3.75 text-gray-600 mr-1" + aria-label="Drag to reorder"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Dropdown.vue(2 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (4)
packages/nc-gui/components/nc/Dropdown.vue (1)
8-8: Confirm support for 'right' placement.
While adding 'right' to the placement prop expands the options, please ensure that the underlying <a-dropdown> component/library supports this new placement. Otherwise, it could lead to unexpected UI behavior.
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
477-477: LGTM! Smart handling of nested dropdowns.
The auto-close behavior tied to openSubmenusCount is a good UX improvement, preventing accidental closure when submenus are open.
Line range hint 724-837: LGTM! Clean and consistent styling.
The styling improvements provide better visual feedback and maintain consistency with the design system. The use of Tailwind classes is well-organized and the hover/focus states are properly handled.
440-454: 🛠️ Refactor suggestion
Consolidate duplicate field visibility toggle logic.
The visibility toggle logic is duplicated between conditionalToggleFieldVisibility and handleFieldVisibilityClick. Consider consolidating them:
-function conditionalToggleFieldVisibility(field: Field) {
- if (isLocked.value || (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links' && !isLocalMode.value)) {
- return
- }
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
-}
-
-function handleFieldVisibilityClick(field: Field) {
- if (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links') {
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
- }
-}
+function toggleFieldVisibilityWithChecks(field: Field, isClickEvent = false) {
+ const isLinksField = meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links'
+
+ if (!isClickEvent && (isLocked.value || (isLinksField && !isLocalMode.value))) {
+ return
+ }
+
+ if (isClickEvent && !isLinksField) {
+ return
+ }
+
+ field.show = !field.show
+ toggleFieldVisibility(field.show, field)
+}Likely invalid or redundant comment.
| function scrollToLatestField() { | ||
| setTimeout(() => { | ||
| document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' }) | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using MutationObserver instead of setTimeout.
The hardcoded 500ms timeout might not be reliable across different scenarios (slow networks, large datasets). Consider using MutationObserver to detect when the new field is actually added to the DOM:
-function scrollToLatestField() {
- setTimeout(() => {
- document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' })
- }, 500);
-}
+function scrollToLatestField() {
+ const observer = new MutationObserver((mutations, obs) => {
+ const lastField = document.querySelector('.nc-fields-menu-item:last-child')
+ if (lastField) {
+ lastField.scrollIntoView({ behavior: 'smooth' })
+ obs.disconnect()
+ }
+ })
+ observer.observe(document.querySelector('.nc-fields-list')!, { childList: true, subtree: true })
+}Committable suggestion skipped: line range outside the PR's diff.
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
9c504d3 to
8855b2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (1)
228-233: LGTM! Consider a minor improvement to the event handler.The addition of
allow-clearimproves UX by enabling users to clear their format selection. The event handler change is also good, but could be slightly simplified.Consider simplifying the event handler to a direct assignment since it's a simple operation:
-@change=" - (v) => { - savedDisplayType = v - } -" +@change="(v) => savedDisplayType = v"packages/nc-gui/components/smartsheet/column/RollupOptions.vue (2)
317-317: LGTM! Consider simplifying the zero checkThe condition correctly handles both truthy values and explicit zero case. However, we could make it more concise.
Consider using
v-if="vModel.meta?.precision !== undefined"which would be more explicit about the intent to show the select when precision has any value including zero.-v-if="vModel.meta?.precision || vModel.meta?.precision === 0" +v-if="vModel.meta?.precision !== undefined"
Line range hint
1-391: Enhance keyboard accessibility and shortcutsTo align with the PR objectives of improving the fields menu and adding shortcuts for quick field creation, consider the following enhancements:
- Add keyboard shortcuts for quick navigation between select dropdowns:
<a-select v-model:value="vModel.fk_relation_column_id" placeholder="-select-" dropdown-class-name="!w-64 nc-dropdown-relation-table !rounded-md" + :keyboard-shortcuts="{ 'alt+1': () => /* focus first dropdown */ }" @change="onRelationColChange" >
- Improve accessibility by adding aria-labels:
<a-form-item class="flex w-1/2 !max-w-[calc(50%_-_4px)] pb-2" :label="`${$t('general.link')} ${$t('objects.field')}`" + aria-label="Select relation field" v-bind="validateInfos.fk_relation_column_id" >
- Show keyboard shortcuts in dropdown UI:
<a-select-option v-for="(table, i) of refTables" :key="i" :value="table.col.fk_column_id"> <div class="flex gap-2 w-full justify-between truncate items-center"> <!-- existing content --> + <span class="text-xs text-gray-500">Alt+1</span> </div> </a-select-option>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Dropdown.vue(2 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/DecimalOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/RollupOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(8 hunks)packages/nc-gui/components/virtual-cell/Rollup.vue(1 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/nc-gui/components/virtual-cell/Rollup.vue
- packages/nc-gui/components/smartsheet/column/DecimalOptions.vue
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts
- packages/nc-gui/components/nc/Dropdown.vue
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
8855b2c to
e126706
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (1)
307-325: Consider extracting visibility conditions into separate functions for better maintainability.The visibility logic is correct but could be more readable by extracting each condition into a named function.
const visibleFields = computed( () => fields.value?.filter((field: Field) => { - if (!field.initialShow && isLocalMode.value) { - return false - } - - if (metaColumnById?.value?.[field.fk_column_id!]?.pv) { - return false - } - - // hide system columns if not enabled - if (!showSystemFields.value && isSystemColumn(metaColumnById?.value?.[field.fk_column_id!])) { - return false - } - - return true + const isInitiallyHidden = !field.initialShow && isLocalMode.value + const isPrimaryValueField = metaColumnById?.value?.[field.fk_column_id!]?.pv + const isHiddenSystemField = !showSystemFields.value && isSystemColumn(metaColumnById?.value?.[field.fk_column_id!]) + + return !isInitiallyHidden && !isPrimaryValueField && !isHiddenSystemField }) || [], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Dropdown.vue(2 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/DecimalOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/RollupOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(10 hunks)packages/nc-gui/components/virtual-cell/Rollup.vue(1 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/nc-gui/components/virtual-cell/Rollup.vue
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/column/FormulaOptions.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/column/DecimalOptions.vue
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
- packages/nc-gui/components/nc/Switch.vue
- tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/components/nc/Dropdown.vue
- packages/nc-gui/assets/style.scss
🔇 Additional comments (4)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (4)
736-759: LGTM! Well-structured lookup dropdown integration.
The SmartsheetToolbarAddLookupsDropdown integration is clean and properly handles state management.
826-863: LGTM! Clean implementation of the new column addition UI.
The implementation follows Vue best practices with proper event handling and lazy loading.
881-886: LGTM! Well-structured and consistent styling.
The styles follow the project's conventions and maintain visual consistency.
454-458: 🛠️ Refactor suggestion
Replace setTimeout with MutationObserver for more reliable scrolling.
Using a fixed timeout is not reliable as the element might not be available after 500ms, especially on slower connections.
function scrollToLatestField() {
- setTimeout(() => {
- document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' })
- }, 500)
+ const observer = new MutationObserver((mutations, obs) => {
+ const lastField = document.querySelector('.nc-fields-menu-item:last-child')
+ if (lastField) {
+ lastField.scrollIntoView({ behavior: 'smooth' })
+ obs.disconnect()
+ }
+ })
+
+ const target = document.querySelector('.nc-fields-list')
+ if (target) {
+ observer.observe(target, { childList: true, subtree: true })
+ }
}Likely invalid or redundant comment.
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
e126706 to
e314298
Compare
…ubmenu slide animation header
o1lab
force-pushed
the
nc-fix/fields-menu-improvement
branch
from
540fe2a to
1cdee11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (1)
228-233: Consider enhancing error handling and state managementWhile the current changes improve the UX, there are opportunities to enhance the component's robustness:
- Consider using Vue's
watchEffectfor more reactive formula validation- Add error handling for edge cases when clearing the selection
- Consider extracting the format-specific logic into a separate composable
Example of using
watchEffect:// Extract to composable const useFormulaValidation = (formula: Ref<string>, columns: Ref<Column[]>) => { const validationState = ref({ isValid: false, error: null, parsedTree: null }) watchEffect(async () => { try { const parsed = await validateFormulaAndExtractTreeWithType({ formula: formula.value, columns: columns.value, // ... other params }) validationState.value = { isValid: true, error: null, parsedTree: parsed } } catch (e) { validationState.value = { isValid: false, error: e, parsedTree: null } } }) return validationState }packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (1)
307-325: Consider improving readability of the visibleFields computed property.The filtering logic is correct but could be more maintainable by breaking down the conditions into named variables.
Consider this refactor:
const visibleFields = computed( () => fields.value?.filter((field: Field) => { - if (!field.initialShow && isLocalMode.value) { - return false - } - - if (metaColumnById?.value?.[field.fk_column_id!]?.pv) { - return false - } - - // hide system columns if not enabled - if (!showSystemFields.value && isSystemColumn(metaColumnById?.value?.[field.fk_column_id!])) { - return false - } - - return true + const isHiddenInLocalMode = !field.initialShow && isLocalMode.value + const isPrimaryValueColumn = metaColumnById?.value?.[field.fk_column_id!]?.pv + const isHiddenSystemColumn = !showSystemFields.value && isSystemColumn(metaColumnById?.value?.[field.fk_column_id!]) + + return !isHiddenInLocalMode && !isPrimaryValueColumn && !isHiddenSystemColumn }) || [], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/nc/Dropdown.vue(2 hunks)packages/nc-gui/components/nc/Switch.vue(3 hunks)packages/nc-gui/components/smartsheet/column/DecimalOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/RollupOptions.vue(1 hunks)packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue(1 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(10 hunks)packages/nc-gui/components/virtual-cell/Rollup.vue(1 hunks)tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts(1 hunks)tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/playwright/pages/Dashboard/common/Toolbar/Fields.ts
- packages/nc-gui/components/smartsheet/column/RollupOptions.vue
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/virtual-cell/Rollup.vue
- packages/nc-gui/components/nc/Dropdown.vue
- tests/playwright/tests/db/columns/columnLtarDragdrop.spec.ts
- packages/nc-gui/components/smartsheet/column/DecimalOptions.vue
- packages/nc-gui/components/nc/Switch.vue
- packages/nc-gui/components/smartsheet/column/UITypesOptionsWithSearch.vue
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/smartsheet/toolbar/AddLookupsDropdown.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
🔇 Additional comments (4)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (1)
228-233: Good UX improvement with clear option!
The addition of allow-clear improves user experience by allowing users to reset their format selection. The event handler change also makes the value assignment more explicit.
Please verify that clearing the selection:
- Properly resets related format-specific options
- Doesn't interfere with the formula validation logic in
debouncedValidate
Run this script to check for proper state cleanup when clearing selections:
✅ Verification successful
Clearing format selection is properly handled
Based on the code analysis, the format selection clearing behavior is well-implemented with proper state management:
- When the display type is cleared:
- A watcher on
vModel.value.meta?.display_typehandles the cleanup - It properly initializes
display_column_metawith empty meta and custom objects
- A watcher on
- The formula validation logic:
- Maintains
previousDisplayTypefor error recovery - Properly resets display type to null when data types don't match
- Restores
savedDisplayTypewhen validation succeeds
- Maintains
The implementation ensures that clearing the format selection properly resets related state and maintains formula validation integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for state cleanup patterns in format-related components
ast-grep --pattern 'watch(() => $value?.meta?.display_type, ($v, $old) => {
if (!$v) {
$$$
}
})'
Length of output: 106
Script:
#!/bin/bash
# Let's check for any watch handlers or state cleanup related to display_type
rg "display_type" -A 5 -B 5
# Also check for any state cleanup in the FormulaOptions component
rg "savedDisplayType" -A 5 -B 5
Length of output: 16299
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
848-885: LGTM! Well-structured UI implementation.
The new dropdown for adding columns and the system fields toggle are well-implemented with:
- Clear visual hierarchy
- Proper keyboard navigation support
- Consistent styling with the rest of the application
454-458: 🛠️ Refactor suggestion
Replace setTimeout with MutationObserver for reliable scrolling.
The hardcoded 500ms timeout might not be reliable across different scenarios (slow networks, large datasets).
Consider using MutationObserver to detect when the new field is actually added to the DOM:
function scrollToLatestField() {
- setTimeout(() => {
- document.querySelector('.nc-fields-menu-item:last-child')?.scrollIntoView({ behavior: 'smooth' })
- }, 500)
+ const observer = new MutationObserver((mutations, obs) => {
+ const lastField = document.querySelector('.nc-fields-menu-item:last-child')
+ if (lastField) {
+ lastField.scrollIntoView({ behavior: 'smooth' })
+ obs.disconnect()
+ }
+ })
+
+ const fieldsList = document.querySelector('.nc-fields-list')
+ if (fieldsList) {
+ observer.observe(fieldsList, { childList: true, subtree: true })
+ }
+}Likely invalid or redundant comment.
460-474: 🛠️ Refactor suggestion
Consolidate duplicate field visibility toggle logic.
The visibility toggle logic is duplicated between conditionalToggleFieldVisibility and handleFieldVisibilityClick, which could lead to maintenance issues.
Consider consolidating the logic:
-function conditionalToggleFieldVisibility(field: Field) {
- if (isLocked.value || (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links' && !isLocalMode.value)) {
- return
- }
-
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
-}
-
-function handleFieldVisibilityClick(field: Field) {
- if (meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links') {
- field.show = !field.show
- toggleFieldVisibility(field.show, field)
- }
-}
+function toggleFieldVisibilityIfAllowed(field: Field, isSwitch = false) {
+ const isLinksField = meta.value?.columnsById?.[field.fk_column_id!]?.uidt === 'Links'
+
+ if (!isSwitch && (isLocked.value || (isLinksField && !isLocalMode.value))) {
+ return
+ }
+
+ if (isSwitch && !isLinksField) {
+ return
+ }
+
+ field.show = !field.show
+ toggleFieldVisibility(field.show, field)
+}Update the template to use the new function:
-@click="conditionalToggleFieldVisibility(field)"
+@click="toggleFieldVisibilityIfAllowed(field)"
-@click="handleFieldVisibilityClick(field)"
+@click="toggleFieldVisibilityIfAllowed(field, true)"Likely invalid or redundant comment.
Change Summary
Improves the fields menu
Change type