fix(nc-gui): instead of rendering multiple lookup attachment cell render only 1 #10518
Conversation
📝 WalkthroughWalkthroughThe changes update the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/virtual-cell/Lookup.vue(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (2)
packages/nc-gui/components/virtual-cell/Lookup.vue (2)
282-283: LGTM! Improved rendering conditions.The changes improve the code by:
- Simplifying the attachment cell rendering check
- Adding a guard clause for the dropdown overlay
Also applies to: 352-353
484-488: LGTM! Well-structured CSS for attachment wrapper.The new styles follow the project's CSS conventions and properly handle the attachment cell height in the lookup dropdown.
|
Uffizzi Preview |
ae741e9 to
5fbbc13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/virtual-cell/Lookup.vue (2)
296-298: Simplify attachment rendering condition.The condition for rendering attachments can be simplified by extracting the check into a computed property.
+const hasValidAttachment = computed(() => + isAttachment(lookupColumn.value) && + arrValue.value[0] && + ncIsObject(arrValue.value[0]) +) -<div v-if="isAttachment(lookupColumn) && arrValue[0] && ncIsObject(arrValue[0])"> +<div v-if="hasValidAttachment">
412-414: Reuse attachment rendering logic.The attachment rendering logic is duplicated from the main template. Consider extracting it into a reusable component.
+<!-- AttachmentCell.vue --> +<template> + <div class="nc-lookup-attachment-wrapper"> + <LazySmartsheetCell + :model-value="modelValue" + :column="column" + :edit-enabled="false" + :read- + /> + </div> +</template> +<script setup lang="ts"> +defineProps({ + modelValue: { type: Array, required: true }, + column: { type: Object, required: true } +}) +</script> -<div v-if="isAttachment(lookupColumn) && arrValue[0] && ncIsObject(arrValue[0])" class="nc-lookup-attachment-wrapper"> - <LazySmartsheetCell :model-value="arrValue" :column="lookupColumn" :edit-enabled="false" :read- /> -</div> +<AttachmentCell + v-if="hasValidAttachment" + :model-value="arrValue" + :column="lookupColumn" +/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/virtual-cell/Lookup.vue(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (2)
packages/nc-gui/components/virtual-cell/Lookup.vue (2)
80-105: Add missing imports and optimize array flattening.The array handling logic could be improved in two ways:
The utility functions
ncIsArray,ncIsObject, andncIsNullare used but not imported.The array flattening logic can be simplified using
flatMap.Add the missing imports at the top of the file:
+import { ncIsArray, ncIsObject, ncIsNull } from 'nocodb-sdk'
- Simplify the array flattening logic:
- return cellValue.value - .filter((v) => v !== null) - .reduce((acc, v) => { - acc.push(...v) - - return acc - }, []) + return cellValue.value + .filter((v) => v !== null) + .flatMap((v) => v)
500-504: LGTM! New attachment wrapper styles.The styles for the attachment wrapper ensure proper layout and appearance.
5fbbc13 to
02acad1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nc-gui/components/cell/SingleSelect/Readonly.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/canvas/cells/Attachment.ts(2 hunks)packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts(3 hunks)packages/nc-gui/components/virtual-cell/Lookup.vue(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nc-gui/components/virtual-cell/Lookup.vue
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (5)
packages/nc-gui/components/cell/SingleSelect/Readonly.vue (1)
34-36: LGTM! Good defensive programming practice.The introduction of optional chaining (
?.) formodelValue.trim()is a good improvement that safely handles cases wheremodelValueis undefined, preventing potential runtime errors. This change aligns well with the component's Props interface wheremodelValueis defined as optional.packages/nc-gui/components/smartsheet/grid/canvas/cells/Attachment.ts (2)
100-114: LGTM! The render method signature has been updated to support flexible alignment.The addition of the
textAlignparameter enhances the flexibility of attachment rendering, particularly for lookup cells.
188-191:❓ Verification inconclusive
Verify the impact of left-aligned attachments on the UI.
The change allows for left-aligned attachments in lookup cells, which aligns with the PR objective. However, ensure that this change:
- Maintains visual consistency with other cell types
- Doesn't cause layout issues when attachments are too wide
🏁 Script executed:
#!/bin/bash # Description: Search for other cell renderers that might be affected by this alignment change # Search for other cell renderers that handle alignment ast-grep --pattern 'class $_ implements CellRenderer { $$$ render($_, $_) { $$$ textAlign $$$ } $$$ }' # Search for lookup-related components that might need similar alignment handling rg -g '*.{ts,vue}' -l 'isUnderLookup.*textAlign'Length of output: 324
Action: Confirm UI Impact of Left-Aligned Attachments
The recent change in
packages/nc-gui/components/smartsheet/grid/canvas/cells/Attachment.ts(lines 188–191) now permits left-aligned attachments in lookup cells. However, automated searches did not unveil similar cases in other cell renderers, so please manually verify the following:
- That the new left alignment maintains visual consistency with other cell types.
- That there are no layout issues (e.g., when attachments are wider than expected).
Please double-check these aspects to ensure the UI remains robust.
packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts (2)
91-91: LGTM!The assignment captures the return value from
getArrValuecorrectly.
109-109: LGTM! Changes align with PR objectives.The conditional height and text alignment for attachment cells improve the rendering:
- Full height for attachments ensures proper display
- Centered alignment for attachments enhances visual presentation
Also applies to: 119-119
packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts
Outdated
Show resolved
Hide resolved
02acad1 to
712df8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts (1)
54-89: Enhance type safety and simplify array flattening logic.The function correctly addresses previous scoping issues, but could benefit from the following improvements:
- Add type annotations for better type safety
- Simplify the array flattening logic using
flatMap- Make the conditions more explicit with type guards
-const getArrValue = () => { +const getArrValue = (): unknown[] => { if (!value) return [] if (lookupColumn.uidt === UITypes.Attachment) { if ([RelationTypes.BELONGS_TO, RelationTypes.ONE_TO_ONE].includes(relatedColObj?.colOptions?.type)) { return [value] } - if ( - ncIsArray(value) && - value.every((v) => { - if (ncIsNull(v)) return true - - if (ncIsArray(v)) { - return !v.length || ncIsObject(v[0]) - } - - return false - }) - ) { - return value - .filter((v) => v !== null) - .reduce((acc, v) => { - acc.push(...v) - - return acc - }, []) - } + if (ncIsArray(value)) { + const isNestedArray = value.every(v => + ncIsNull(v) || (ncIsArray(v) && (!v.length || ncIsObject(v[0]))) + ) + if (isNestedArray) { + return value + .filter((v): v is unknown[] => ncIsArray(v)) + .flatMap(v => v) + } + } } if (ncIsArray(value)) { return value.filter((v) => v !== null) } return [value] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nc-gui/components/cell/SingleSelect/Readonly.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/canvas/cells/Attachment.ts(2 hunks)packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts(3 hunks)packages/nc-gui/components/virtual-cell/Lookup.vue(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nc-gui/components/cell/SingleSelect/Readonly.vue
- packages/nc-gui/components/smartsheet/grid/canvas/cells/Attachment.ts
- packages/nc-gui/components/virtual-cell/Lookup.vue
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (2)
packages/nc-gui/components/smartsheet/grid/canvas/cells/Lookup.ts (2)
91-93: LGTM!The usage of
getArrValueis clean and maintains proper validation of the result.
109-109: LGTM!The conditional rendering properties for attachments are well-implemented and align with the PR's objective of fixing lookup attachment cell rendering.
Also applies to: 119-119
d55e161 to
9abe9f0
Compare
9abe9f0 to
8b05c72
Compare
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of