Skip to content

Conversation

@mertmit
Copy link
Member

@mertmit mertmit commented Dec 3, 2024

No description provided.

Signed-off-by: mertmit <mertmit99@gmail.com>
Signed-off-by: mertmit <mertmit99@gmail.com>
@mertmit mertmit requested a review from dstala December 3, 2024 18:41
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Dec 3, 2024
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2024

📝 Walkthrough

Walkthrough

The changes in this pull request encompass various updates across multiple Vue components and related files. Key modifications include enhancements to the PromptWithFields component's placeholder functionality, the introduction of new components such as DropdownSelect and EditableText, and improvements to existing components like SingleSelect and Gallery. Additionally, new utility functions for file handling and feature toggles were added, along with updates to documentation files for clarity. The overall focus is on improving user interaction, enhancing component functionality, and maintaining code clarity.

Changes

File Path Change Summary
packages/nc-gui/components/ai/PromptWithFields.vue Added placeholder prop with detailed comments, modified editor's placeholder behavior, and updated CSS for line breaks.
packages/nc-gui/components/cell/SingleSelect.vue Removed getMeta function, refined options handling, enhanced new option creation logic, and updated keyboard event handling.
packages/nc-gui/components/cell/attachment/index.vue Added updateAttachmentTitle method, updated open function for safe event handling, and exposed several methods for parent component access.
packages/nc-gui/components/cell/attachment/utils.ts Removed getReadableFileSize, added updateAttachmentTitle, and modified renameFile to utilize the new function.
packages/nc-gui/components/dashboard/settings/base/index.vue Condensed activeMenu variable declaration for formatting without functional changes.
packages/nc-gui/components/general/WorkspaceIconSelector.vue Activated import for WorkspaceIconType, updated Props interface for iconType.
packages/nc-gui/components/nc/DropdownSelect.vue Introduced new dropdown selection component with tooltip support and defined props.
packages/nc-gui/components/nc/EditableText.vue Added editable text component with internal state management and editing functionality.
packages/nc-gui/components/nc/List/RecordItem.vue Introduced new component for displaying record items with props for row and column management.
packages/nc-gui/components/nc/List/index.vue Renamed interfaces and added showSearchAlways prop for search input visibility.
packages/nc-gui/components/nc/SelectTab.vue Added new tabbed select component with props for tab management.
packages/nc-gui/components/smartsheet/Cell.vue Enhanced rendering logic with loading state and tooltip integration.
packages/nc-gui/components/smartsheet/Gallery.vue Simplified class binding and introduced lazy loading for gallery items.
packages/nc-gui/components/smartsheet/column/AiButtonOptions.vue Updated logic for handling input and output panels, reorganized template structure.
packages/nc-gui/components/smartsheet/expanded-form/Sidebar/index.vue Added showFieldsTab prop for conditional rendering of a new tab.
packages/nc-gui/components/smartsheet/expanded-form/index.vue Removed unused imports, added view mode handling, and restructured rendering logic.
packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/*.vue Introduced several new components for attachment management with basic templates.
packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/*.vue Added components for managing and displaying fields in expanded forms.
packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue Enhanced functionality with new AI integration features and updated props.
packages/nc-gui/components/virtual-cell/Button.vue Simplified logic for isFieldAiIntegrationAvailable and updated tooltip conditions.
packages/nc-gui/utils/fileUtils.ts Updated file extension handling and added new utility functions for file type checks.
packages/nc-gui/utils/iconUtils.ts Added new file type icons to the icon mapping system.
packages/noco-docs/docs/*.md Multiple documentation files updated for clarity and consistency.
packages/nocodb-sdk/src/lib/Api.ts Added SnapshotType interface for snapshot management.
packages/nocodb-sdk/src/lib/globals.ts Introduced ExpandedFormMode constant and type for expanded form modes.
packages/nocodb/src/models/View.ts Updated methods to handle new expanded record mode parameters.
packages/nocodb/src/services/columns.service.ts Added logic to manage updates for attachment columns in views.
tests/playwright/pages/Dashboard/ExpandedForm/index.ts Enhanced ExpandedFormPage class with new methods and locators.
tests/playwright/pages/Dashboard/Settings/Miscellaneous.ts Added method for tab selection and updated existing methods for tab interactions.
tests/playwright/tests/db/columns/columnDuration.spec.ts Skipped the test case for creating a duration column.
tests/playwright/tests/db/features/expandedFormModeFiles.spec.ts Introduced a new test suite for expanded form files mode with two tests.

Possibly related PRs

Suggested reviewers

  • rameshmane7218
  • dstala
  • pranavxc

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 3, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 30

🧹 Outside diff range and nitpick comments (51)
packages/nc-gui/components/general/WorkspaceIconSelector.vue (2)

Line range hint 147-178: Consider extracting file handling logic

The file handling logic in handleChange is complex and handles multiple responsibilities. Consider extracting it into smaller, focused functions for better maintainability.

+ const handleFileUploadStatus = (file: UploadFile) => {
+   if (file.status === 'uploading') {
+     isUploadingImage.value = true
+     return
+   }
+   isUploadingImage.value = false
+ }

+ const handleFileUploadSuccess = (file: UploadFile) => {
+   if (!(file.originFileObj instanceof File)) return
+   
+   if (imageCropperData.value.imageConfig.src) {
+     URL.revokeObjectURL(imageCropperData.value.imageConfig.src)
+   }
+   
+   const blob = URL.createObjectURL(file.originFileObj)
+   imageCropperData.value.imageConfig = {
+     src: blob,
+     type: file.originFileObj.type,
+     name: file.originFileObj.name,
+   }
+   
+   showImageCropper.value = true
+ }

  const handleChange = (info: UploadChangeParam) => {
    const status = info.file.status
-   if (status === 'uploading') {
-     isUploadingImage.value = true
-     return
-   }
+   handleFileUploadStatus(info.file)

    if (status === 'done' && info.file.originFileObj instanceof File) {
-     if (imageCropperData.value.imageConfig.src) {
-       URL.revokeObjectURL(imageCropperData.value.imageConfig.src)
-     }
-     const blob = URL.createObjectURL(info.file.originFileObj)
-     imageCropperData.value.imageConfig = {
-       src: blob,
-       type: info.file.originFileObj.type,
-       name: info.file.originFileObj.name,
-     }
-     isUploadingImage.value = false
-     showImageCropper.value = true
+     handleFileUploadSuccess(info.file)
      return
    }

    if (status === 'error') {
-     isUploadingImage.value = false
      message.error(`${t('msg.error.fileUploadFailed')} ${info.file.name}`)
    }
  }

Line range hint 193-199: Extract file size limit constant and internationalize error message

Consider these improvements to the beforeUpload function:

  1. Extract the file size limit as a constant
  2. Use i18n for the error message
+ const MAX_FILE_SIZE_MB = 2
+ const MAX_FILE_SIZE_BYTES = MAX_FILE_SIZE_MB * 1024 * 1024

  const beforeUpload = (file: UploadFile) => {
-   const exceedLimit = file.size! / 1024 / 1024 > 2
+   const exceedLimit = file.size! > MAX_FILE_SIZE_BYTES
    if (exceedLimit) {
-     message.error(`File ${file.name} is too big. The accepted file size is less than 2MB.`)
+     message.error(t('msg.error.fileSizeExceeded', { name: file.name, size: MAX_FILE_SIZE_MB }))
    }
    return !exceedLimit || Upload.LIST_IGNORE
  }
packages/nc-gui/components/nc/DropdownSelect.vue (3)

4-11: Add prop validation and default values.

Consider adding validation and defaults for better component reliability:

  1. Validate that items array is not empty
  2. Provide default values for optional props
 const props = defineProps<{
   disabled?: boolean
   tooltip?: string
   items: {
     label: string
     value: string
   }[]
-}>()
+}>({
+  disabled: false,
+  tooltip: undefined,
+  items: () => {
+    return []
+  }
+})

+const validateItems = computed(() => {
+  return props.items.length > 0
+})

28-31: Add theme and dark mode support.

The current styling uses hardcoded colors. Consider:

  1. Using theme variables instead of hardcoded colors
  2. Adding dark mode support
-            class="flex items-center justify-between px-2 py-1 rounded-md transition-colors hover:bg-gray-100 cursor-pointer"
+            class="flex items-center justify-between px-2 py-1 rounded-md transition-colors hover:bg-accent cursor-pointer dark:hover:bg-accent-dark"
             :class="{
-              'bg-gray-100': modelValue === item.value,
+              'bg-accent dark:bg-accent-dark': modelValue === item.value,
             }"

1-14: Add component documentation.

Please add JSDoc comments to document:

  1. Component purpose and usage
  2. Props description and examples
  3. Events emitted
  4. Slot usage
+/**
+ * A dropdown select component with tooltip support.
+ * @component
+ * @example
+ * <DropdownSelect
+ *   :items="[{label: 'Option 1', value: '1'}, {label: 'Option 2', value: '2'}]"
+ *   v-model="selected"
+ *   tooltip="Select an option"
+ * >
+ *   <button>Click me</button>
+ * </DropdownSelect>
+ */
 <script lang="ts" setup>
packages/nc-gui/components/nc/EditableText.vue (3)

4-8: Consider adding more customization props.

While the current implementation is functional, consider adding these props to enhance flexibility:

  • placeholder: For custom placeholder text when empty
  • maxLength: To limit input length
  • validateInput: For custom validation function
 const props = defineProps<{
   disabled?: boolean
+  placeholder?: string
+  maxLength?: number
+  validateInput?: (value: string) => boolean
 }>()

14-20: Consider adding debounce for performance optimization.

For scenarios with frequent text changes, consider implementing debounce to reduce unnecessary updates.

+import { debounce } from 'lodash-es'
+
 watch(
   modelValue,
-  (value) => {
+  debounce((value) => {
     internalText.value = value || ''
-  },
+  }, 300),
   { immediate: true },
 )

1-66: Consider architectural improvements for better component integration.

  1. Emit events for better parent component integration:

    • @edit-start
    • @edit-complete
    • @edit-cancel
    • @validation-error
  2. Add unit tests to verify:

    • Double-click to edit functionality
    • Keyboard navigation
    • Disabled state behavior
    • Event emissions

Would you like me to help create unit tests for this component?

packages/nc-gui/components/nc/SelectTab.vue (2)

2-8: Enhance component documentation

While the basic documentation is present, consider enhancing it with:

  • Description of each prop
  • More comprehensive example showing all props
  • Return value/events documentation

Example enhancement:

 /**
  * @description
  * Tabbed select component
  *
  * @example
  * <NcSelectTab 
-   :items="items" 
-   v-model="modelValue" 
+   :items="[
+     { icon: 'plus', title: 'Add', value: 'add' },
+     { icon: 'minus', title: 'Remove', value: 'remove' }
+   ]"
+   :disabled="false"
+   tooltip="Select an action"
+   v-model="modelValue" 
  />
+ *
+ * @prop {boolean} [disabled] - Disables user interaction
+ * @prop {string} [tooltip] - Tooltip text to show on hover
+ * @prop {Array<{icon: string, title?: string, value: string}>} items - Array of tab items
+ * @emits {string} update:modelValue - Emitted when selection changes
  */

57-59: Add focus states for better accessibility

The tab styling should include focus states for keyboard navigation.

 .tab {
-  @apply flex flex-row items-center h-6 justify-center px-2 py-1 rounded-md gap-x-2 text-gray-600 hover:text-black cursor-pointer transition-all duration-300 select-none;
+  @apply flex flex-row items-center h-6 justify-center px-2 py-1 rounded-md gap-x-2 text-gray-600 hover:text-black focus:text-black focus:outline-none focus:ring-2 focus:ring-brand-500 cursor-pointer transition-all duration-300 select-none;
 }
packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/MiniColumnsWrapper.vue (3)

6-8: Import store type and consider adding prop validation.

The store type is referenced but not imported. Also, consider adding validation for the required store prop.

Consider these improvements:

+ import type { useProvideExpandedFormStore } from '../../../composables/useExpandedForm'

const props = defineProps<{
-  store: ReturnType<typeof useProvideExpandedFormStore>
+  store: ReturnType<typeof useProvideExpandedFormStore> & {
+    required: true;
+    validator: (value: unknown) => boolean;
+  }
}>()

10-16: Document the purpose and initial values of the reactive flags.

The reactive flags lack documentation explaining their purpose and why they default to false.

Add JSDoc comments to explain the flags:

+ /** Metadata injection with fallback to empty ref */
const meta = inject(MetaInj, ref())

/* flags */

+ /** Controls whether to maintain the default view column order */
const maintainDefaultViewOrder = ref(false)
+ /** Controls whether to use metadata fields instead of injected fields */
const useMetaFields = ref(false)

38-47: Extract complex filtering logic into separate functions.

The filtering logic in hiddenFields is complex and could benefit from being broken down into smaller, more readable functions.

Consider refactoring like this:

+ const isColumnVisible = (col: any) => {
+   if (!isLocalMode.value || !col?.id) return true
+   return fieldsMap.value[col.id]?.initialShow ?? true
+ }

+ const isValidColumn = (col: any) => {
+   return !fields.value?.includes(col) && 
+          isColumnVisible(col) &&
+          !isSystemColumn(col)
+ }

const hiddenFields = computed(() => {
  return (meta.value?.columns ?? [])
-   .filter(
-     (col) =>
-       !fields.value?.includes(col) &&
-       (isLocalMode.value && col?.id && fieldsMap.value[col.id] ? fieldsMap.value[col.id]?.initialShow : true),
-   )
-   .filter((col) => !isSystemColumn(col))
+   .filter(isValidColumn)
})
packages/nc-gui/components/smartsheet/column/AiButtonOptions.vue (1)

890-899: Remove redundant CSS width property in scoped style.

The width is set using Tailwind CSS utilities and directly in the CSS, which may cause conflicts or redundancy.

Consider removing the redundant width specification in the CSS:

.nc-ai-button-config-left-section {
  @apply mx-auto p-6 w-full max-w-[568px];
-  width: min(100%, 568px);
}

.nc-ai-button-config-right-section {
  @apply mx-auto px-6 w-full max-w-[576px] flex flex-col;
-  width: min(100%, 576px);
}

This ensures consistency and relies on the Tailwind CSS utility classes for styling.

packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue (2)

Line range hint 2616-2645: Refine CSS selectors to prevent unintended side effects

The CSS selectors using td:not(.nc-grid-add-new-cell-item) may unintentionally affect other table cells. Review and adjust the selectors to target only the intended elements to prevent unexpected styling issues.


Line range hint 2826-2855: Avoid brittle nth-child selectors for maintainability

Using nth-child selectors like td:nth-child(1) can be fragile if the table structure changes. Consider adding specific class names to the target elements and using class selectors instead, enhancing the maintainability of the CSS.

packages/nc-gui/components/nc/List/RecordItem.vue (1)

105-105: Typographical Error in Class Attribute

There is a missing space between !max-w-11 and object-cover in the class attribute, which may cause incorrect styling.

Apply this diff to fix the typo:

-class="!w-11 !h-11 !max-h-11 !max-w-11object-cover !rounded-l-xl"
+class="!w-11 !h-11 !max-h-11 !max-w-11 object-cover !rounded-l-xl"
packages/nc-gui/components/smartsheet/expanded-form/index.vue (1)

560-560: TODO Comment: Potential Dead Code

The TODO comment on line 560 suggests that the code may no longer be needed. Consider verifying and removing it if it is unnecessary to keep the codebase clean and maintainable.

Would you like assistance in verifying whether this code can be safely removed and updating the code accordingly?

packages/nocodb/src/models/View.ts (1)

2456-2464: Empty Method 'updateIfColumnUsedAsExpandedMode' Without Implementation

The static method updateIfColumnUsedAsExpandedMode is currently empty. If this is intended as a placeholder for future implementation, consider adding a TODO comment to indicate that it needs to be completed. If the method is unnecessary, consider removing it to avoid confusion.

Do you need help implementing this method or deciding whether it should be removed?

tests/playwright/pages/Dashboard/Settings/Miscellaneous.ts (1)

21-21: Refactor to Use 'selectTab' Method for Tab Selection

In the methods clickShowM2MTables and clickShowNullEmptyFilters, you're directly clicking on the 'visibility-tab'. Since you've added the selectTab method, consider using it for consistency and to reduce code duplication.

Apply this refactor:

 async clickShowM2MTables() {
-  await this.settings.get().locator(`[data-testid="visibility-tab"]`).click();
+  await this.selectTab('visibility-tab');
   const clickAction = () => this.get().locator('.nc-settings-meta-misc-m2m').first().click();
   await this.waitForResponse({
     uiAction: clickAction,
     requestUrlPathToMatch: 'tables?includeM2M',
     httpMethodsToMatch: ['GET'],
   });
 }

 async clickShowNullEmptyFilters() {
-  await this.settings.get().locator(`[data-testid="visibility-tab"]`).click();
+  await this.selectTab('visibility-tab');
   await this.waitForResponse({
     uiAction: () => this.rootPage.locator('.nc-settings-show-null-and-empty-in-filter').first().click(),
     requestUrlPathToMatch: '/api/v1/db/meta/projects',
     httpMethodsToMatch: ['PATCH'],
   });
 }

Also applies to: 31-31

packages/noco-docs/docs/.135.extensions/060.world-clock.md (1)

18-18: Consider Replacing 'Amongst' with 'Among'

The word 'amongst' is acceptable but may be considered old-fashioned or literary. Replacing it with 'among' can improve clarity and modernize the language.

Apply this change:

-- **Theme**: Choose one amongst the available themes for the clock display.
+- **Theme**: Choose one among the available themes for the clock display.
🧰 Tools
🪛 LanguageTool

[style] ~18-~18: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...cted City name. - Theme: Choose one amongst the available themes for the clock disp...

(AMONGST)

packages/noco-docs/docs/.135.extensions/020.data-exporter.md (2)

12-19: Enhance consistency in step formatting

The numbered steps could be more consistent in their formatting:

 Follow these steps to export data from your NocoDB tables:
 1. Select the table and associated view you wish to export.
 2. Configure optional settings for Separator and Encoding:
     - Default separator: Comma `,`
     - Other options: Semicolon `;`, Pipe `|` and `Tab`
-3. Click the **Export** button.
-4. Once the export is complete, the file will be listed in the **Recent Exports** section.
-5. Click the **Download** button to save the CSV file to your local device.
+3. Click the **Export** button to initiate the export.
+4. Wait for the export to complete. The file will appear in the **Recent Exports** section.
+5. Click the **Download** button to save the CSV file.

32-37: Standardize action descriptions

The managing exports section could be more consistent in its formatting and wording:

-1. In the Recent Exports section, locate the desired file.
-2. Click the Download icon next to the file to save it to your device.
+1. Locate the desired file in the Recent Exports section.
+2. Click the Download icon to save the file to your device.

-1. Locate it in the Recent Exports section.
-2. Click the `x` icon to remove it from the list.
+1. Locate the file in the Recent Exports section.
+2. Click the Delete icon (x) to remove it from the list.
packages/noco-docs/docs/.135.extensions/040.bulk-update.md (2)

12-21: Enhance step clarity and consistency

The steps could be more precise and consistent:

 Follow these steps to update multiple records in a table:
 1. Select the table and associated view you wish to update.
 2. Click the **New Action** button.
 3. Choose the **Field** you want to update.
 4. Select an **Update Type**:
    - **Set Value**: Set a specific value for the field.
    - **Clear Value**: Clear the field value.
-5. If you selected **Set Value**, provide the **Value** to be applied.
+5. For **Set Value** type: Enter the value to be applied.
 6. Click the **Update Records** button to apply the changes.
-7. A confirmation dialog will display the number of fields and records to be updated. Review the details and click **Confirm Update** to proceed.
+7. Review the confirmation dialog showing affected fields and records, then click **Confirm Update**.

34-34: Improve warning and info section clarity

The warning and info sections could be more informative:

-Bulk updates are irreversible. Ensure you review the changes before confirming the update.
+Bulk updates cannot be undone. Please carefully review all changes before confirming the update.

-Bulk updates are applied to all records in the view, including those not visible on the current page.
+Bulk updates affect all records in the current view, including those not visible in the current pagination.

Also applies to: 38-38

packages/nc-gui/components/smartsheet/expanded-form/Sidebar/index.vue (3)

2-5: Consider adding prop validation

The props could benefit from additional validation:

 const props = defineProps<{
   store: ReturnType<typeof useProvideExpandedFormStore>
-  showFieldsTab?: boolean
+  showFieldsTab: boolean
 }>()
+
+const props = withDefaults(defineProps<{
+  store: ReturnType<typeof useProvideExpandedFormStore>
+  showFieldsTab?: boolean
+}>(), {
+  showFieldsTab: false
+})

15-23: Enhance accessibility for the Fields tab

The Fields tab could benefit from improved accessibility:

-      <a-tab-pane v-if="props.showFieldsTab" key="fields" class="w-full h-full">
+      <a-tab-pane 
+        v-if="props.showFieldsTab" 
+        key="fields" 
+        class="w-full h-full"
+        aria-label="Fields tab content"
+      >
         <template #tab>
-          <div v-e="['c:row-expand:fields']" class="flex items-center gap-2">
+          <div 
+            v-e="['c:row-expand:fields']" 
+            class="flex items-center gap-2"
+            role="tab"
+            aria-selected="tab === 'fields'"
+          >
             <GeneralIcon icon="fields" class="w-4 h-4" />
             <span class="<lg:hidden"> Fields </span>
           </div>
         </template>
         <SmartsheetExpandedFormPresentorsFieldsMiniColumnsWrapper :store="props.store" />
       </a-tab-pane>

9-9: Consider using computed property for initial tab

The initial tab value could be more reactive:

-const tab = ref<'fields' | 'comments' | 'audits'>(props.showFieldsTab ? 'fields' : 'comments')
+const defaultTab = computed(() => props.showFieldsTab ? 'fields' : 'comments')
+const tab = ref<'fields' | 'comments' | 'audits'>(defaultTab.value)
packages/noco-docs/docs/.135.extensions/010.overview.md (2)

44-44: Add missing article

Add "the" before "extensions side panel" for better grammar.

- To add an extension to your NocoDB instance, open extensions side panel and follow these steps:
+ To add an extension to your NocoDB instance, open the extensions side panel and follow these steps:
🧰 Tools
🪛 LanguageTool

[uncategorized] ~44-~44: Possible missing article found.
Context: ...extension to your NocoDB instance, open extensions side panel and follow these steps: 1. C...

(AI_HYDRA_LEO_MISSING_THE)


61-61: Add hyphen in compound adjective

Add a hyphen in "full screen" when used as a compound adjective.

- Use the `expand` icon to view the extension in full screen mode.
+ Use the `expand` icon to view the extension in full-screen mode.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~61-~61: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... expand icon to view the extension in full screen mode. <img src="/img/v2/extensions/expa...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

packages/nc-gui/utils/fileUtils.ts (1)

Line range hint 1-1: Remove duplicate 'png' extension from imageExt array

The imageExt array contains a duplicate entry for 'png' which should be removed.

const imageExt = [
  'jpeg',
  'gif',
  'png',
-  'png',
  'svg',
  'bmp',
  'ico',
  'jpg',
  'webp',
  'avif',
  'heif',
  'heifs',
  'heic',
  'heic-sequence',
]
packages/nc-gui/composables/useBetaFeatureToggle.ts (1)

53-59: Enhance feature description for better clarity

While the implementation is correct, consider expanding the description to better explain the feature's impact and use cases.

  {
    id: 'expanded_form_file_preview_mode',
    title: 'Expanded form file preview mode',
-    description: 'Preview mode allow you to see attachments inline',
+    description: 'Enables inline preview of attachments within expanded forms, improving the visibility and accessibility of file attachments without leaving the form context',
    enabled: false,
    isEngineering: true,
  },
packages/nc-gui/components/ai/PromptWithFields.vue (1)

Line range hint 97-122: Consider simplifying cursor position logic

The cursor position handling in newFieldSuggestionNode is complex and could benefit from being broken down into smaller, more focused functions.

+ const shouldInsertSpace = (editor: Editor, fromPos: number) => {
+   const lastCharacter = editor.state.doc.textBetween(fromPos - 1, fromPos)
+   const textBefore = editor.state.doc.textBetween(fromPos - 1, fromPos, '\n', '\n')
+   return lastCharacter !== ' ' && fromPos !== 1 && !textBefore.includes('\n')
+ }

const newFieldSuggestionNode = () => {
  if (!editor.value) return

  const { $from } = editor.value.state.selection
-  const textBefore = editor.value.state.doc.textBetween($from.pos - 1, $from.pos, '\n', '\n')
  const lastCharacter = editor.value.state.doc.textBetween($from.pos - 1, $from.pos)

-  // Check if the text before cursor contains a newline
-  const hasNewlineBefore = textBefore.includes('\n')

  if (lastCharacter === '{') {
    editor.value
      .chain()
      .deleteRange({ from: $from.pos - 1, to: $from.pos })
      .run()
-  } else if (lastCharacter !== ' ' && $from.pos !== 1 && !hasNewlineBefore) {
+  } else if (shouldInsertSpace(editor.value, $from.pos)) {
    editor.value?.commands.insertContent(' {')
    editor.value?.chain().focus().run()
  } else {
    editor.value?.commands.insertContent('{')
    editor.value?.chain().focus().run()
  }
}
tests/playwright/tests/db/features/expandedFormModeFiles.spec.ts (4)

5-6: Consider adding a description for the skipped test suite.

Add a comment explaining why the test suite is skipped and under what conditions it should be enabled.

-// Todo: Enable this test once we enable this feature for all
+// TODO: Enable this test suite once the expanded form files mode feature is enabled for all users.
+// Related feature flag: <feature-flag-name>
+// Related issue: <issue-number>

19-33: Improve the helper function's error handling and type safety.

The addFileToRow helper function could benefit from improved error handling and TypeScript types.

-async function addFileToRow(rowIndex: number, filePathAppned: string[]) {
+interface AddFileParams {
+  rowIndex: number;
+  columnHeader: string;
+  filePaths: string[];
+}
+
+async function addFileToRow({ rowIndex, columnHeader, filePaths }: AddFileParams) {
+  if (!filePaths.length) {
+    throw new Error('At least one file path must be provided');
+  }
+
   await dashboard.grid.cell.attachment.addFile({
     index: rowIndex,
-    columnHeader: 'testAttach',
-    filePath: filePathAppned.map(filePath => `${process.cwd()}/fixtures/sampleFiles/${filePath}`),
+    columnHeader,
+    filePath: filePaths.map(filePath => `${process.cwd()}/fixtures/sampleFiles/${filePath}`),
   });
 
   await dashboard.rootPage.waitForTimeout(500);
 
+  // Verify file count matches expected
   await dashboard.grid.cell.attachment.verifyFileCount({
     index: rowIndex,
-    columnHeader: 'testAttach',
-    count: filePathAppned.length,
+    columnHeader,
+    count: filePaths.length,
   });
 }

26-26: Replace hardcoded timeout with a constant.

Using a magic number for timeout duration makes it harder to maintain. Consider extracting it to a named constant.

+const FILE_UPLOAD_STABILIZATION_TIMEOUT = 500;
+
-await dashboard.rootPage.waitForTimeout(500);
+await dashboard.rootPage.waitForTimeout(FILE_UPLOAD_STABILIZATION_TIMEOUT);

Also applies to: 27-27


35-64: Enhance test case readability and structure.

The first test case could benefit from better organization using test.step() for logical groupings.

 test('Mode switch and functionality', async () => {
   test.slow();
 
-  await dashboard.treeView.openTable({ title: 'Country' });
-  await dashboard.grid.column.create({
-    title: 'testAttach',
-    type: 'Attachment',
-  });
+  await test.step('Setup test data', async () => {
+    await dashboard.treeView.openTable({ title: 'Country' });
+    await dashboard.grid.column.create({
+      title: 'testAttach',
+      type: 'Attachment',
+    });
+  });
 
-  await addFileToRow(0, ['1.json']);
-  await addFileToRow(2, ['1.json', '2.json']);
+  await test.step('Add test files', async () => {
+    await addFileToRow({ rowIndex: 0, columnHeader: 'testAttach', filePaths: ['1.json'] });
+    await addFileToRow({ rowIndex: 2, columnHeader: 'testAttach', filePaths: ['1.json', '2.json'] });
+  });
 
+  await test.step('Verify mode switching', async () => {
   await dashboard.grid.openExpandedRow({ index: 0 });
   await dashboard.expandedForm.verifyTableNameShown({ name: 'Country' });
 
   await dashboard.expandedForm.verifyIsInFieldsMode();
   await dashboard.expandedForm.switchToFilesMode();
   await dashboard.expandedForm.verifyIsInFilesMode();
+  });
packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/index.vue (2)

6-16: Consider adding JSDoc comments for props.

Add documentation for the props to improve maintainability and developer experience.

+/**
+ * Props for the ExpandedFormPresentorsFields component
+ * @prop {ReturnType<typeof useProvideExpandedFormStore>} store - The expanded form store
+ * @prop {string} [rowId] - The ID of the current row
+ * @prop {ColumnType[]} fields - List of visible fields
+ * @prop {ColumnType[]} hiddenFields - List of hidden fields
+ * @prop {boolean} isUnsavedDuplicatedRecordExist - Flag indicating if unsaved duplicated record exists
+ * @prop {boolean} isUnsavedFormExist - Flag indicating if unsaved form exists
+ * @prop {boolean} isLoading - Flag indicating if data is being loaded
+ * @prop {boolean} isSaving - Flag indicating if data is being saved
+ * @prop {string} [newRecordSubmitBtnText] - Custom text for the submit button
+ */
 const props = defineProps<{

185-193: Optimize CSS selectors for better performance.

The nested selectors with multiple :not() pseudo-classes can impact performance.

-.nc-data-cell {
+/* Define base styles */
+.nc-data-cell:not(.nc-readonly-div-data-cell):not(.nc-system-field):not(.nc-attachment-cell):not(.nc-virtual-cell-button) {
+  box-shadow: 0px 0px 4px 0px rgba(0, 0, 0, 0.08);
+}
+
+/* Define hover styles */
+.nc-data-cell:hover:not(.nc-readonly-div-data-cell):not(.nc-system-field):not(.nc-virtual-cell-button):not(:focus-within) {
   @apply !border-1;
-  &:not(:focus-within):hover:not(.nc-readonly-div-data-cell):not(.nc-system-field):not(.nc-virtual-cell-button) {
-    @apply !border-1;
-    &:not(.nc-attachment-cell):not(.nc-virtual-cell-button) {
-      box-shadow: 0px 0px 4px 0px rgba(0, 0, 0, 0.24);
-    }
-  }
+}
+
+/* Define hover shadow */
+.nc-data-cell:hover:not(.nc-readonly-div-data-cell):not(.nc-system-field):not(.nc-virtual-cell-button):not(:focus-within):not(.nc-attachment-cell):not(.nc-virtual-cell-button) {
+  box-shadow: 0px 0px 4px 0px rgba(0, 0, 0, 0.24);
+}
packages/nc-gui/components/virtual-cell/components/ListItem.vue (1)

269-270: Consider using CSS custom properties for hover state

The transparent background on hover could be better managed using CSS custom properties for better maintainability.

- @apply !bg-transparent !hover:bg-transparent;
+ @apply !bg-var(--tooltip-bg) !hover:bg-var(--tooltip-hover-bg);
tests/playwright/pages/Dashboard/ExpandedForm/index.ts (2)

252-258: LGTM! Consider adding wait for navigation completion

The navigation methods look good but might benefit from waiting for navigation to complete.

 async moveToNextField() {
   await this.btn_nextField.click();
+  await this.rootPage.waitForLoadState('networkidle');
 }

 async moveToPreviousField() {
   await this.btn_previousField.click();
+  await this.rootPage.waitForLoadState('networkidle');
 }

280-282: Consider adding timeout for file viewer visibility

The file viewer verification might need a longer timeout for larger files.

 async verifyFilesViewerMode({ mode }: { mode: 'image' | 'video' | 'audio' | 'pdf' | 'unsupported' }) {
-  await expect(this.get().locator(`.nc-files-mode-container .nc-files-viewer-${mode}`)).toBeVisible();
+  await expect(this.get().locator(`.nc-files-mode-container .nc-files-viewer-${mode}`)).toBeVisible({ timeout: 10000 });
 }
packages/nc-gui/composables/useSharedView.ts (1)

424-435: LGTM! Consider adding error handling

The implementation looks good but could benefit from error handling for API failures.

 const setCurrentViewExpandedFormMode = async (viewId: string, mode: 'field' | 'attachment', columnId?: string) => {
+  try {
     await $api.dbView.update(viewId, {
       expanded_record_mode: mode,
       attachment_mode_column_id: columnId,
     })
+  } catch (error) {
+    console.error('Failed to update expanded form mode:', error)
+    throw error
+  }
 }

 const setCurrentViewExpandedFormAttachmentColumn = async (viewId: string, columnId: string) => {
+  try {
     await $api.dbView.update(viewId, {
       attachment_mode_column_id: columnId,
     })
+  } catch (error) {
+    console.error('Failed to update attachment column:', error)
+    throw error
+  }
 }
packages/nc-gui/components/nc/List/index.vue (1)

111-113: Consider extracting the condition into a separate computed property.

The isSearchEnabled computed property combines multiple conditions. Consider splitting it for better maintainability:

-const isSearchEnabled = computed(
-  () => props.showSearchAlways || slots.headerExtraLeft || slots.headerExtraRight || props.list.length > props.minItemsForSearch,
-)
+const hasExtraSlots = computed(() => slots.headerExtraLeft || slots.headerExtraRight)
+const hasEnoughItems = computed(() => props.list.length > props.minItemsForSearch)
+const isSearchEnabled = computed(() => props.showSearchAlways || hasExtraSlots.value || hasEnoughItems.value)
packages/nc-gui/components/cell/attachment/index.vue (1)

264-271: Consider consolidating icon rendering logic.

The file icon rendering logic is duplicated between the form view and grid view. Consider extracting it into a separate component.

+// Create a new component FileIcon.vue
+<script setup lang="ts">
+const props = defineProps<{
+  item: { icon?: string; title: string }
+  size?: number
+}>()
+</script>
+
+<template>
+  <component
+    :is="FileIcon(item.icon)"
+    v-if="item.icon"
+    :height="size || 45"
+    :width="size || 45"
+    class="text-white"
+  />
+  <GeneralIcon
+    v-else
+    icon="ncFileTypeUnknown"
+    :height="size || 45"
+    :width="size || 45"
+    class="text-white"
+  />
+</template>

Then use it in both places:

-<component
-  :is="FileIcon(item.icon)"
-  v-if="item.icon"
-  :height="45"
-  :width="45"
-  class="text-white"
-  @click="selectedFile = item"
-/>
-<GeneralIcon v-else icon="ncFileTypeUnknown" :height="45" :width="45" class="text-white" @click="selectedFile = item" />
+<FileIcon :item="item" @click="selectedFile = item" />

Also applies to: 273-273, 438-438

packages/nocodb-sdk/src/lib/Api.ts (1)

3296-3320: Consider enhancing the SnapshotType interface documentation and type safety.

The interface implementation is solid but could be improved with:

  1. JSDoc descriptions for each property to improve code documentation
  2. Define status as a union type to restrict valid values
  3. Add examples in JSDoc comments for better developer experience

Example enhancement:

 /**
  * Model for Snapshot
  */
 export interface SnapshotType {
   /** Unique identifier for the snapshot */
   id?: IdType;
   /** Title of the snapshot 
    * @example "Weekly Backup 2024-01-01"
    */
   title?: string;
   /** ID of the base this snapshot belongs to */
   base_id?: IdType;
   /** ID of the snapshot base */
   snapshot_base_id?: IdType;
   /** ID of the workspace this snapshot belongs to */
   fk_workspace_id?: IdType;
   /** 
    * Date when snapshot was created
    * @format date
    * @example "2024-01-01T00:00:00.000Z"
    */
   created_at?: string;
   /** ID of user who created the snapshot */
   created_by?: IdType;
   /** Status of the snapshot
    * @example "completed"
    */
-  status?: string;
+  status?: 'pending' | 'in_progress' | 'completed' | 'failed';
 }
packages/nocodb/src/services/columns.service.ts (2)

1576-1585: Add error handling for view update during column type change

The view update during attachment column type change should include error handling to gracefully handle potential failures.

Consider wrapping the view update in a try-catch:

 if (
   column.uidt === UITypes.Attachment &&
   colBody.uidt !== UITypes.Attachment
 ) {
+  try {
     await View.updateIfColumnUsedAsExpandedMode(
       context,
       column.id,
       column.fk_model_id,
     );
+  } catch (error) {
+    console.error('Failed to update view expanded mode:', error);
+    // Consider whether to throw or handle gracefully based on requirements
+  }
 }

2708-2713: Add error handling for view update during column deletion

Similar to the column type change, the view update during column deletion should include error handling.

Consider adding error handling:

+  try {
     await View.updateIfColumnUsedAsExpandedMode(
       context,
       column.id,
       column.fk_model_id,
       ncMeta,
     );
+  } catch (error) {
+    console.error('Failed to update view expanded mode during deletion:', error);
+    // Consider whether to throw or handle gracefully based on requirements
+  }
packages/nc-gui/utils/iconUtils.ts (1)

3737-3764: Standardize keywords for file type icons

The keywords for file type icons could be more consistent and comprehensive to improve searchability.

Consider standardizing keywords across file types:

 ncFileTypePdf: {
   icon: NcFileTypePdf,
-  keywords: ['pdf', 'document', 'file', 'adobe', 'reader'],
+  keywords: ['pdf', 'document', 'file', 'adobe', 'reader', 'portable document format'],
 },
 ncFileTypeWord: {
   icon: NcFileTypeWord,
-  keywords: ['word', 'document', 'file', 'microsoft', 'writer'],
+  keywords: ['word', 'document', 'file', 'microsoft', 'writer', 'doc', 'docx'],
 },
 ncFileTypePresentation: {
   icon: NcFileTypePresentation,
-  keywords: ['powerpoint', 'presentation', 'file', 'microsoft', 'office'],
+  keywords: ['powerpoint', 'presentation', 'file', 'microsoft', 'office', 'ppt', 'pptx', 'slides'],
 },
 ncFileTypeVideo: {
   icon: NcFileTypeVideo,
-  keywords: ['video', 'movie', 'file', 'media', 'player'],
+  keywords: ['video', 'movie', 'file', 'media', 'player', 'mp4', 'avi', 'mov'],
 },
 ncFileTypeAudio: {
   icon: NcFileTypeAudio,
-  keywords: ['audio', 'music', 'file', 'media', 'player'],
+  keywords: ['audio', 'music', 'file', 'media', 'player', 'mp3', 'wav', 'sound'],
 },
 ncFileTypeZip: {
   icon: NcFileTypeZip,
-  keywords: ['zip', 'archive', 'file', 'compression', 'zipper'],
+  keywords: ['zip', 'archive', 'file', 'compression', 'zipper', 'rar', '7z', 'tar'],
 },
 ncFileTypeUnknown: {
   icon: NcFileTypeUnknown,
-  keywords: ['unknown', 'file', 'type', 'extension', 'unknown'],
+  keywords: ['unknown', 'file', 'type', 'extension', 'other', 'generic'],
 },
packages/nc-gui/components/smartsheet/Gallery.vue (2)

Line range hint 393-512: Consider implementing lazy loading for carousel images

While the current implementation uses virtual scrolling and lazy component loading effectively, the carousel images could benefit from lazy loading to improve initial load performance and reduce memory usage.

Consider adding lazy loading attributes to the carousel images:

 <LazyCellAttachmentPreviewImage
   v-if="isImage(attachment.title, attachment.mimetype ?? attachment.type)"
   :key="`carousel-${record.rowMeta.rowIndex}-${index}`"
   class="h-52"
+  loading="lazy"
   :class="[`${coverImageObjectFitClass}`]"
   :srcs="getPossibleAttachmentSrc(attachment, 'card_cover')"
   @click="expandFormClick($event, record)"
 />

Line range hint 393-512: Enhance accessibility for gallery interactions

The gallery implementation would benefit from additional accessibility attributes to improve the experience for screen reader users.

Consider adding these accessibility improvements:

 <div
   v-for="(record, rowIndex) in visibleRows"
   :key="`record-${record.rowMeta.rowIndex}`"
   :data-card-id="`record-${record.rowMeta.rowIndex}`"
+  role="article"
+  :aria-label="`Gallery item ${rowIndex + 1} of ${visibleRows.length}`"
 >
   <LazySmartsheetRow :row="record">
     <a-card
       class="!rounded-xl h-full border-gray-200 border-1 group overflow-hidden break-all max-w-[450px] cursor-pointer"
       :body-style="{ padding: '16px !important' }"
       :data-testid="`nc-gallery-card-${record.rowMeta.rowIndex}`"
+      tabindex="0"
+      :aria-expanded="false"
       @click="expandFormClick($event, record)"
       @contextmenu="showContextMenu($event, { row: record, index: rowIndex })"
     >
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 615a9c1 and ee90f04.

⛔ Files ignored due to path filters (21)
  • packages/nc-gui/assets/nc-icons-v2/eye-off.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-audio.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-compressed.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-csv.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-doc.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-excel.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-pdf.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-presentation.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-unknown.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-unsupported.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-video.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-word.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons-v2/file-type-zip.svg is excluded by !**/*.svg, !**/*.svg
  • packages/nc-gui/assets/nc-icons/eye-off.svg is excluded by !**/*.svg, !**/*.svg
  • packages/noco-docs/docs/.135.extensions/_category_.json is excluded by !**/*.json
  • packages/noco-docs/static/img/v2/extensions/url-preview.png is excluded by !**/*.png, !**/*.png
  • packages/noco-docs/static/img/v2/extensions/world-clock-1.png is excluded by !**/*.png, !**/*.png
  • packages/noco-docs/static/img/v2/extensions/world-clock-2.png is excluded by !**/*.png, !**/*.png
  • tests/playwright/fixtures/sampleFiles/sample.mp3 is excluded by !**/*.mp3, !**/*.mp3
  • tests/playwright/fixtures/sampleFiles/sample.mp4 is excluded by !**/*.mp4, !**/*.mp4
  • tests/playwright/fixtures/sampleFiles/sample.pdf is excluded by !**/*.pdf, !**/*.pdf
📒 Files selected for processing (45)
  • packages/nc-gui/components/ai/PromptWithFields.vue (2 hunks)
  • packages/nc-gui/components/cell/SingleSelect.vue (0 hunks)
  • packages/nc-gui/components/cell/attachment/index.vue (5 hunks)
  • packages/nc-gui/components/cell/attachment/utils.ts (6 hunks)
  • packages/nc-gui/components/dashboard/settings/base/index.vue (1 hunks)
  • packages/nc-gui/components/general/WorkspaceIconSelector.vue (2 hunks)
  • packages/nc-gui/components/nc/DropdownSelect.vue (1 hunks)
  • packages/nc-gui/components/nc/EditableText.vue (1 hunks)
  • packages/nc-gui/components/nc/List/RecordItem.vue (1 hunks)
  • packages/nc-gui/components/nc/List/index.vue (12 hunks)
  • packages/nc-gui/components/nc/SelectTab.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/Cell.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/Gallery.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/column/AiButtonOptions.vue (12 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/Sidebar/index.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/index.vue (15 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/AttachmentView.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/PreviewBar.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/PreviewCell.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/index.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/Columns.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/MiniColumnsWrapper.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/index.vue (1 hunks)
  • packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue (11 hunks)
  • packages/nc-gui/components/virtual-cell/Button.vue (3 hunks)
  • packages/nc-gui/components/virtual-cell/components/ListItem.vue (1 hunks)
  • packages/nc-gui/composables/useBetaFeatureToggle.ts (1 hunks)
  • packages/nc-gui/composables/useSharedView.ts (2 hunks)
  • packages/nc-gui/utils/fileUtils.ts (3 hunks)
  • packages/nc-gui/utils/iconUtils.ts (3 hunks)
  • packages/noco-docs/docs/.135.extensions/010.overview.md (3 hunks)
  • packages/noco-docs/docs/.135.extensions/020.data-exporter.md (2 hunks)
  • packages/noco-docs/docs/.135.extensions/030.upload-data-from-csv.md (1 hunks)
  • packages/noco-docs/docs/.135.extensions/040.bulk-update.md (2 hunks)
  • packages/noco-docs/docs/.135.extensions/050.url-preview.md (1 hunks)
  • packages/noco-docs/docs/.135.extensions/060.world-clock.md (1 hunks)
  • packages/nocodb-sdk/src/lib/Api.ts (1 hunks)
  • packages/nocodb-sdk/src/lib/globals.ts (1 hunks)
  • packages/nocodb/src/helpers/NcPluginMgrv2.ts (1 hunks)
  • packages/nocodb/src/models/View.ts (7 hunks)
  • packages/nocodb/src/services/columns.service.ts (2 hunks)
  • tests/playwright/pages/Dashboard/ExpandedForm/index.ts (3 hunks)
  • tests/playwright/pages/Dashboard/Settings/Miscellaneous.ts (2 hunks)
  • tests/playwright/tests/db/columns/columnDuration.spec.ts (1 hunks)
  • tests/playwright/tests/db/features/expandedFormModeFiles.spec.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/nc-gui/components/cell/SingleSelect.vue
✅ Files skipped from review due to trivial changes (7)
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/AttachmentView.vue
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/PreviewBar.vue
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/index.vue
  • packages/nc-gui/components/smartsheet/expanded-form/presentors/Attachments/PreviewCell.vue
  • packages/nc-gui/components/dashboard/settings/base/index.vue
  • packages/noco-docs/docs/.135.extensions/050.url-preview.md
  • packages/nocodb/src/helpers/NcPluginMgrv2.ts
🧰 Additional context used
🪛 LanguageTool
packages/noco-docs/docs/.135.extensions/060.world-clock.md

[style] ~18-~18: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...cted City name. - Theme: Choose one amongst the available themes for the clock disp...

(AMONGST)

packages/noco-docs/docs/.135.extensions/030.upload-data-from-csv.md

[style] ~9-~9: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ...his tool enables you to efficiently add new records, update existing ones, and map CSV colu...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~17-~17: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ...d for matching) while optionally adding new records. The available options under this mode ...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~18-~18: ‘New Records’ might be wordy. Consider a shorter alternative.
Context: ...ptions under this mode are: - Create New Records Only - Only adds new records from ...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[duplication] ~18-~18: Possible typo: you repeated a word
Context: ... this mode are: - Create New Records Only - Only adds new records from the CSV. Existing...

(ENGLISH_WORD_REPEAT_RULE)


[style] ~19-~19: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ...reate New Records Only* - Only adds new records from the CSV. Existing records are not ...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~19-~19: ‘New records’ might be wordy. Consider a shorter alternative.
Context: ... CSV. Existing records are not updated. New records are identified based on the merge field...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...specified. - Update Existing Records Only - Only updates records that already exist in t...

(ENGLISH_WORD_REPEAT_RULE)


[style] ~21-~21: ‘New records’ might be wordy. Consider a shorter alternative.
Context: ... in the table based on the merge field. New records are not added. - *Create and Update R...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~23-~23: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... Create and Update Records - Adds new records and updates existing ones as needed, ba...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~36-~36: ‘New Records’ might be wordy. Consider a shorter alternative.
Context: ...owing Import Types: - Create New Records Only - *Update Existing Records...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~37-~37: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...Only* - Update Existing Records Only - Create and Update Records ...

(ADVERB_REPETITION_PREMIUM)


[style] ~48-~48: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... a summary detailing: - The number of new records added - The number of existing record...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)

packages/noco-docs/docs/.135.extensions/010.overview.md

[uncategorized] ~44-~44: Possible missing article found.
Context: ...extension to your NocoDB instance, open extensions side panel and follow these steps: 1. C...

(AI_HYDRA_LEO_MISSING_THE)


[uncategorized] ~61-~61: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... expand icon to view the extension in full screen mode. <img src="/img/v2/extensions/expa...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

🪛 Markdownlint (0.35.0)
packages/noco-docs/docs/.135.extensions/030.upload-data-from-csv.md

18-18: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


19-19: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)


20-20: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


21-21: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)


22-22: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


23-23: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)


48-48: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


49-49: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


50-50: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)

🔇 Additional comments (33)
packages/nc-gui/components/smartsheet/Cell.vue (2)

176-182: LGTM: Well-implemented loading state with user feedback.

The loading state implementation is clean and follows Vue best practices:

  • Proper conditional rendering
  • Good use of flex layout for alignment
  • Internationalization support for messages
  • Tooltip implementation for truncated text

186-186: Verify the ref usage for LazyCellAttachment.

The addition of the ref suggests programmatic access to the attachment cell component. Please ensure this ref is being used appropriately in the parent components.

✅ Verification successful

The attachmentCell ref is used appropriately

The ref is correctly defined and used within the component structure. The LazyCellAttachment component internally manages its own ref (attachmentCellRef) for layout and sizing purposes, and the parent's ref is used for component instance access, which is a standard Vue pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of attachmentCell ref in parent components
rg -l "attachmentCell" --type vue

Length of output: 61


Script:

#!/bin/bash
# Search for usage of attachmentCell ref in the codebase
rg "attachmentCell" -A 5 -B 5

Length of output: 2847

packages/nc-gui/components/virtual-cell/Button.vue (1)

131-132: LGTM: Enhanced security controls for AI button

The additional checks for public access and data edit permissions improve the security of the AI feature.

packages/nc-gui/components/general/WorkspaceIconSelector.vue (2)

8-8: LGTM: Type safety improvements

Good improvement in type safety by properly importing and utilizing WorkspaceIconType in the Props interface.

Also applies to: 11-14


70-71: Verify parent component behavior with unconditional submit event

The submit event is now emitted unconditionally when removing an icon, before the dropdown state changes. Ensure this doesn't affect parent components that might depend on the previous behavior.

✅ Verification successful

After analyzing the full component implementation, I can now generate the final response:

Unconditional submit event emission is intentional and correct

The submit event is consistently emitted after state updates across all icon-related actions in the component:

  • selectIcon(): Updates icon/type, emits submit, then closes dropdown
  • selectEmoji(): Updates icon/type, emits submit, then closes dropdown
  • handleOnUploadImage(): Updates icon/type, emits submit, then closes dropdown
  • handleRemoveIcon(): Updates icon/type, emits submit, then optionally closes dropdown

The consistent pattern shows this is the intended behavior, where the submit event signals icon changes to parent components. The timing is correct as it follows the state updates.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components using WorkspaceIconSelector to verify event handling
rg -l "WorkspaceIconSelector" --type vue

Length of output: 68


Script:

#!/bin/bash
# Search for components using WorkspaceIconSelector with ripgrep, without file type filter
rg -l "WorkspaceIconSelector"

Length of output: 29


Script:

#!/bin/bash
# Search for actual usage of the submit event from WorkspaceIconSelector
rg "@submit.*WorkspaceIconSelector" -A 2

Length of output: 42


Script:

#!/bin/bash
# Search for the component definition to understand the full context
fd "WorkspaceIconSelector.vue" --exec cat {}

Length of output: 14988

packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/Columns.vue (1)

183-184: Same ref assignment issue with index 0 in hidden fields

The same issue occurs here. The condition if(i) skips assignment when i is 0, preventing cellWrapperEl from being assigned for the first hidden field.

Apply this diff to fix the condition:

- :ref="(el: any) => { if (i) cellWrapperEl = el }"
+ :ref="(el: any) => { if (i === 0) cellWrapperEl = el }"
packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/MiniColumnsWrapper.vue (2)

39-39: Address the TODO comment about undefined meta.value.

The TODO comment indicates uncertainty about when meta.value is undefined. This should be investigated and properly handled.

Let's search for other usages of MetaInj to understand the possible undefined scenarios:


50-58: Review hardcoded values in template props.

The template contains two potentially problematic prop values:

  1. is-loading is hardcoded to false which might not reflect actual loading states
  2. force-vertical-mode is used without clear documentation of its purpose

Let's check how these props are used in other components:

packages/nc-gui/components/smartsheet/column/AiButtonOptions.vue (8)

56-62: Initialize previewOutputRow with a default row object for consistency.

The introduction of defaultRow and its use in initializing previewOutputRow improves code clarity and ensures consistency.


72-72: Computed property displayField is appropriately defined.

The logic for selecting the display field is clear and efficient, providing a fallback to the first column if needed.


212-216: Toggle function for expansionInputPanel is correctly implemented.

The handleUpdateExpansionInputPanel function effectively toggles the expansion state of the input panel.


220-221: expansionOutputPanel initialized to show output panel by default.

Initializing expansionOutputPanel with [ExpansionPanelKeys.output] ensures that the output panel is expanded when the component loads.


229-236: Verify the necessity of the 500ms delay when closing the AI Button Config Modal.

Using setTimeout with a 500ms delay when setting isAiButtonConfigModalOpen to false may cause unintended side effects or impact user experience. Ensure that this delay is necessary, possibly for animation purposes, and confirm it does not interfere with modal behavior.


261-266: handleResetOutput function effectively resets the preview output.

The function correctly resets isAlreadyGenerated and clears previewOutputRow to its default state.


Line range hint 356-377: Template structure and localization are properly implemented for the configuration section.

The UI components are well-structured, and the title uses $t('general.configure') for localization.


378-386: Form component is correctly configured with appropriate bindings.

The <a-form> element and its properties are set up properly, ensuring reactivity and validation.

packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue (1)

1008-1056: Ensure validity of fields.value and selectedRange.start.col in generateAIBulk

Similar to the previous comment, accessing fields.value[selectedRange.start.col] without checking may result in runtime errors if fields.value is undefined or selectedRange.start.col is out of bounds. Please add null checks to prevent potential issues.

tests/playwright/tests/db/columns/columnDuration.spec.ts (1)

59-59: ⚠️ Potential issue

Document reason for skipping the test

The test for creating a duration column is being skipped without explanation. Please add a comment explaining why this test is skipped or create an issue to track its re-enablement.

packages/nc-gui/utils/fileUtils.ts (3)

38-44: LGTM: Well-organized file extension categorization

The new extension arrays are well-organized and follow a logical grouping pattern.


88-106: LGTM: Consistent implementation of file type checking functions

The new type-checking functions follow the established pattern and maintain consistency with existing implementations.


167-170: LGTM: Efficient implementation of file size conversion

The getReadableFileSize function efficiently uses logarithmic calculation to determine the appropriate unit and format the size.

packages/nc-gui/components/ai/PromptWithFields.vue (3)

24-28: LGTM: Well-documented placeholder property

The placeholder property is well-documented with clear examples of multi-line usage.


184-184: LGTM: Proper handling of line breaks in placeholder

The addition of white-space: pre-line correctly preserves line breaks in the placeholder text.


Line range hint 97-122: Add unit tests for cursor position handling

The complex cursor position logic in newFieldSuggestionNode should be covered by unit tests to ensure reliability.

Would you like me to help create unit tests for the cursor position handling logic?

packages/nocodb-sdk/src/lib/globals.ts (1)

47-53: LGTM! Well-structured type definitions.

The implementation follows TypeScript best practices:

  • Using as const for type safety
  • Deriving the type using indexed access types
  • Clear and descriptive naming
packages/nc-gui/components/smartsheet/expanded-form/presentors/Fields/index.vue (1)

31-31: 🛠️ Refactor suggestion

Improve type safety by avoiding any type.

The context variable is typed as any, which reduces type safety.

-const { commentsDrawer, changedColumns, isNew, loadRow: _loadRow, row: _row } = props.store
+interface StoreContext {
+  commentsDrawer: Ref<boolean>
+  changedColumns: Set<string>
+  isNew: Ref<boolean>
+  loadRow: () => Promise<void>
+  row: Ref<Record<string, any>>
+}
+
+const { commentsDrawer, changedColumns, isNew, loadRow: _loadRow, row: _row } = props.store as StoreContext

Likely invalid or redundant comment.

packages/nc-gui/composables/useSharedView.ts (1)

457-458: LGTM! Methods are properly exported

The new methods are correctly added to the return object.

packages/nc-gui/components/nc/List/index.vue (1)

8-11: LGTM: Type definitions and props are well-documented.

The interface renaming to NcListItemType and the props structure with JSDoc comments improve code clarity and maintainability.

Also applies to: 17-63

packages/nc-gui/components/cell/attachment/utils.ts (3)

5-9: LGTM: Icon imports updated consistently.

The icon imports have been updated to use the new nc-icons-v2 package.


269-272: LGTM: New utility function for attachment title updates.

The updateAttachmentTitle function provides a clean way to update attachment titles and trigger model updates.


361-361: Verify the impact of changing download behavior.

The download target has been changed from '_blank' to '_self', which will affect how files are downloaded in the browser.

Also applies to: 363-363

✅ Verification successful

Using _self target for file downloads is appropriate

The change from _blank to _self for file downloads in attachment/utils.ts is actually correct. Looking at the codebase:

  • All other window.open calls with _blank are used for opening external URLs, social links, or sharing features
  • The attachment download is the only file download implementation, and using _self is a common pattern for file downloads as it provides a better user experience by not creating unnecessary new tabs
  • The change aligns with the file's purpose of handling attachments and downloads
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other download implementations to ensure consistency
rg -A 2 "window\.open.*download" --glob '!{*.md,*.json}'

Length of output: 58


Script:

#!/bin/bash
# Search for window.open usage patterns in the codebase
rg -A 2 "window\.open\(" --glob '!{*.md,*.json}'

Length of output: 3197

packages/nc-gui/components/cell/attachment/index.vue (1)

225-231: LGTM: Well-structured component API exposure.

The exposed methods provide a clear interface for parent components to interact with attachment functionality.

packages/nc-gui/components/smartsheet/Gallery.vue (1)

393-393: LGTM: Flex container implementation is appropriate

The use of flex-1 class ensures the gallery container takes up the remaining available space while maintaining proper layout structure.

class="w-full flex items-center"
>
<NcTooltip :disabled="isFieldAiIntegrationAvailable" class="flex">
<NcTooltip :disabled="isFieldAiIntegrationAvailable || isPublic || !isUIAllowed('dataEdit')" class="flex">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix tooltip visibility logic

The tooltip disabled condition appears to be inverted. Currently, it's disabled when AI integration is available, but it should be the opposite - the tooltip should appear when integration is not available to explain why.

Apply this fix:

-    <NcTooltip :disabled="isFieldAiIntegrationAvailable || isPublic || !isUIAllowed('dataEdit')" class="flex">
+    <NcTooltip :disabled="!isFieldAiIntegrationAvailable || isPublic || !isUIAllowed('dataEdit')" class="flex">
📝 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.

Suggested change
<NcTooltip :disabled="isFieldAiIntegrationAvailable || isPublic || !isUIAllowed('dataEdit')" class="flex">
<NcTooltip :disabled="!isFieldAiIntegrationAvailable || isPublic || !isUIAllowed('dataEdit')" class="flex">

Comment on lines +40 to +46
const isFieldAiIntegrationAvailable = computed(() => {
if (column.value?.colOptions?.type !== ButtonActionsType.Ai) return true
const fkIntegrationId = column.value?.colOptions?.fk_integration_id
return !!fkIntegrationId
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify the removal of aiIntegrations validation

The simplified check for AI integration availability might cause issues. By only checking for fk_integration_id presence without validating against aiIntegrations, the button might appear enabled even when the integration is not properly configured or available.

Consider reverting to the previous implementation or adding additional validation:

 const isFieldAiIntegrationAvailable = computed(() => {
   if (column.value?.colOptions?.type !== ButtonActionsType.Ai) return true

   const fkIntegrationId = column.value?.colOptions?.fk_integration_id

-  return !!fkIntegrationId
+  return !!fkIntegrationId && aiIntegrations.value?.some(i => i.id === fkIntegrationId)
 })

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +21 to +41
<NcDropdown :disabled="props.disabled" :class="{ 'pointer-events-none opacity-50': props.disabled }">
<slot />
<template #overlay>
<div class="flex flex-col gap-1 p-1">
<div
v-for="item of props.items"
:key="item.value"
class="flex items-center justify-between px-2 py-1 rounded-md transition-colors hover:bg-gray-100 cursor-pointer"
:class="{
'bg-gray-100': modelValue === item.value,
}"
@click="modelValue = item.value"
>
<span>
{{ item.label }}
</span>
<GeneralIcon v-if="modelValue === item.value" icon="check" class="text-primary" />
</div>
</div>
</template>
</NcDropdown>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance accessibility and keyboard navigation.

The dropdown implementation needs accessibility improvements:

  1. Add ARIA attributes for screen readers
  2. Implement keyboard navigation (arrow keys, Enter, Escape)
  3. Consider adding loading and error states
-    <NcDropdown :disabled="props.disabled" :class="{ 'pointer-events-none opacity-50': props.disabled }">
+    <NcDropdown
+      :disabled="props.disabled"
+      :class="{ 'pointer-events-none opacity-50': props.disabled }"
+      role="listbox"
+      :aria-expanded="isOpen"
+      :aria-label="props.tooltip || 'Select an option'"
+      @keydown.up.prevent="selectPrevious"
+      @keydown.down.prevent="selectNext"
+      @keydown.enter.prevent="selectCurrent"
+      @keydown.esc="close"
+    >
       <slot />
       <template #overlay>
-        <div class="flex flex-col gap-1 p-1">
+        <div class="flex flex-col gap-1 p-1" role="presentation">
           <div
             v-for="item of props.items"
             :key="item.value"
+            role="option"
+            :aria-selected="modelValue === item.value"
+            :tabindex="modelValue === item.value ? 0 : -1"
             class="flex items-center justify-between px-2 py-1 rounded-md transition-colors hover:bg-gray-100 cursor-pointer"

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +32 to +34
nextTick(() => {
inputRef.value?.select?.()
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for select() operation.

The select() call might fail silently if the input reference is invalid.

 nextTick(() => {
-  inputRef.value?.select?.()
+  try {
+    if (inputRef.value && typeof inputRef.value.select === 'function') {
+      inputRef.value.select()
+    }
+  } catch (error) {
+    console.warn('Failed to select input text:', error)
+  }
 })
📝 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.

Suggested change
nextTick(() => {
inputRef.value?.select?.()
})
nextTick(() => {
try {
if (inputRef.value && typeof inputRef.value.select === 'function') {
inputRef.value.select()
}
} catch (error) {
console.warn('Failed to select input text:', error)
}
})

Comment on lines +49 to +65
<div class="inline-block">
<template v-if="!isInEditMode">
<span :class="{ 'cursor-pointer': !disabled }" @dblclick="goToEditMode()">
{{ internalText }}
</span>
</template>
<template v-else>
<a-input
ref="inputRef"
v-model:value="internalText"
class="!rounded-lg !w-72"
@blur="finishEdit()"
@keyup.enter="finishEdit()"
@keyup.esc="cancelEdit()"
/>
</template>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance accessibility support.

The component should include proper ARIA attributes and keyboard navigation support.

-<div class="inline-block">
+<div 
+  class="inline-block"
+  role="textbox"
+  :aria-label="placeholder || 'Editable text'"
+  :aria-disabled="disabled"
+>
   <template v-if="!isInEditMode">
-    <span :class="{ 'cursor-pointer': !disabled }" @dblclick="goToEditMode()">
+    <span 
+      :class="{ 'cursor-pointer': !disabled }" 
+      @dblclick="goToEditMode()"
+      @keydown.enter="goToEditMode()"
+      tabindex="0"
+      role="button"
+      :aria-label="'Click to edit ' + internalText"
+    >
       {{ internalText }}
     </span>
   </template>
   <template v-else>
     <a-input
       ref="inputRef"
       v-model:value="internalText"
       class="!rounded-lg !w-72"
+      :aria-label="placeholder || 'Edit text'"
       @blur="finishEdit()"
       @keyup.enter="finishEdit()"
       @keyup.esc="cancelEdit()"
     />
   </template>
 </div>

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +52 to +53
const attachmentColumn = computed(() => {
return props.attachmentColumn || (allColumns.value || []).find((c) => isAttachment(c))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing Imports for Utility Functions

The utility functions isAttachment, isPrimary, isAiButton, and isImage are used but not imported or defined within this file. This may lead to runtime errors.

Please import these functions from their respective modules. For example:

+import { isAttachment, isPrimary, isAiButton, isImage } from 'nocodb-sdk'

Also applies to: 75-77, 104-105

Comment on lines +44 to +45
const { isFeatureEnabled } = useBetaFeatureToggle()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing Imports for 'isEeUI' and 'FEATURE_FLAG'

The variables isEeUI and FEATURE_FLAG are used but not imported or defined within this file, which may lead to runtime errors.

Please import these variables from their respective modules. For example:

+import { isEeUI } from '@/utils' // Update the path as necessary
+import { FEATURE_FLAG } from '@/composables/useBetaFeatureToggle'

Also applies to: 707-709

Comment on lines +116 to +125
watch(activeViewMode, async (v) => {
const viewId = props.view?.id
if (!viewId) return
if (v === 'field') {
await setCurrentViewExpandedFormMode(viewId, v)
} else if (v === 'attachment') {
const firstAttachmentField = fields.value?.find((f) => f.uidt === 'Attachment')
await setCurrentViewExpandedFormMode(viewId, v, props.view?.attachment_mode_column_id ?? firstAttachmentField?.id)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Undefined 'firstAttachmentField'

In the watcher on activeViewMode, firstAttachmentField may be undefined if there are no attachment fields. This could result in passing undefined as the attachment_mode_column_id, causing unexpected behavior.

Modify the code to handle the case when firstAttachmentField is undefined. For example:

 watch(activeViewMode, async (v) => {
   const viewId = props.view?.id
   if (!viewId) return
   if (v === 'field') {
     await setCurrentViewExpandedFormMode(viewId, v)
   } else if (v === 'attachment') {
     const firstAttachmentField = fields.value?.find((f) => f.uidt === 'Attachment')
+    if (!props.view?.attachment_mode_column_id && !firstAttachmentField?.id) {
+      // Handle the case where there are no attachment fields
+      return
+    }
     await setCurrentViewExpandedFormMode(
       viewId,
       v,
       props.view?.attachment_mode_column_id ?? firstAttachmentField?.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.

Suggested change
watch(activeViewMode, async (v) => {
const viewId = props.view?.id
if (!viewId) return
if (v === 'field') {
await setCurrentViewExpandedFormMode(viewId, v)
} else if (v === 'attachment') {
const firstAttachmentField = fields.value?.find((f) => f.uidt === 'Attachment')
await setCurrentViewExpandedFormMode(viewId, v, props.view?.attachment_mode_column_id ?? firstAttachmentField?.id)
}
})
watch(activeViewMode, async (v) => {
const viewId = props.view?.id
if (!viewId) return
if (v === 'field') {
await setCurrentViewExpandedFormMode(viewId, v)
} else if (v === 'attachment') {
const firstAttachmentField = fields.value?.find((f) => f.uidt === 'Attachment')
if (!props.view?.attachment_mode_column_id && !firstAttachmentField?.id) {
// Handle the case where there are no attachment fields
return
}
await setCurrentViewExpandedFormMode(
viewId,
v,
props.view?.attachment_mode_column_id ?? firstAttachmentField?.id,
)
}
})

Comment on lines 18 to +23
- *Create New Records Only*
- Only adds new records from the CSV. Existing records are not updated. New records are identified based on the merge field specified.
- *Update Existing Records Only*
- Only updates records that already exist in the table based on the merge field. New records are not added.
- *Create and Update Records*
- Adds new records and updates existing ones as needed. New and existing records are identified based on the merge field.

**Merge Field** is field that will be used to match records from the CSV file with existing records in NocoDB table for the purpose of updating.

**Field Mapping:** You can select the fields from the CSV file to import and map them to the corresponding fields in the NocoDB table.

### Steps
To import data from a CSV file into NocoDB, drag-drop (or upload) the CSV file into the extension area and follow these steps:
1. Select the table you want to import the data into. By default, current active table is selected.
2. Select the mode of import: **Add Records** or **Merge Records**. If you choose Merge Records, you will have to additionally
- Select "Import Type" :
- *Create New Records Only*,
- *Update Existing Records Only*, or
- *Create and Update Records*.
- Select "Merge Field". Merge field typically will be your Primary Key or a unique identifier. Composite keys are not supported.
3. **Use first record as header**: If the first row of the CSV file contains the column headers, enable this option to use them as the field names.
4. Map the CSV columns to the NocoDB fields.
5. Click on the **Import** button to start the import process.
- Adds new records and updates existing ones as needed, based on the merge field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix markdown list indentation

The nested list indentation is inconsistent. According to markdown standards, nested lists should be indented with 2 spaces.

- &nbsp; **2. Merge Records**: Update existing records based on a designated **merge field** (a unique field used for matching) while optionally adding new records. The available options under this mode are:
-   - *Create New Records Only*
-     - Only adds new records from the CSV. Existing records are not updated. New records are identified based on the merge field specified.
-   - *Update Existing Records Only* 
-     - Only updates records that already exist in the table based on the merge field. New records are not added.
-   - *Create and Update Records*
-     - Adds new records and updates existing ones as needed, based on the merge field.
+ &nbsp; **2. Merge Records**: Update existing records based on a designated **merge field** (a unique field used for matching) while optionally adding new records. The available options under this mode are:
+   - *Create New Records Only*
+     - Only adds new records from the CSV. Existing records are not updated. New records are identified based on the merge field specified.
+   - *Update Existing Records Only* 
+     - Only updates records that already exist in the table based on the merge field. New records are not added.
+   - *Create and Update Records*
+     - Adds new records and updates existing ones as needed, based on the merge field.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 LanguageTool

[style] ~18-~18: ‘New Records’ might be wordy. Consider a shorter alternative.
Context: ...ptions under this mode are: - Create New Records Only - Only adds new records from ...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[duplication] ~18-~18: Possible typo: you repeated a word
Context: ... this mode are: - Create New Records Only - Only adds new records from the CSV. Existing...

(ENGLISH_WORD_REPEAT_RULE)


[style] ~19-~19: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ...reate New Records Only* - Only adds new records from the CSV. Existing records are not ...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~19-~19: ‘New records’ might be wordy. Consider a shorter alternative.
Context: ... CSV. Existing records are not updated. New records are identified based on the merge field...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...specified. - Update Existing Records Only - Only updates records that already exist in t...

(ENGLISH_WORD_REPEAT_RULE)


[style] ~21-~21: ‘New records’ might be wordy. Consider a shorter alternative.
Context: ... in the table based on the merge field. New records are not added. - *Create and Update R...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)


[style] ~23-~23: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... Create and Update Records - Adds new records and updates existing ones as needed, ba...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)

🪛 Markdownlint (0.35.0)

18-18: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


19-19: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)


20-20: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


21-21: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)


22-22: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


23-23: Expected: 2; Actual: 4
Unordered list indentation

(MD007, ul-indent)

Comment on lines +48 to +50
- The number of new records added
- The number of existing records updated
- Any records left unchanged
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix list indentation in post-import summary

The list items should be aligned with the same indentation level.

- Once the import is complete, NocoDB will display a summary detailing:
-   - The number of new records added
-   - The number of existing records updated
-   - Any records left unchanged
+ Once the import is complete, NocoDB will display a summary detailing:
+ - The number of new records added
+ - The number of existing records updated
+ - Any records left unchanged

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 LanguageTool

[style] ~48-~48: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... a summary detailing: - The number of new records added - The number of existing record...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)

🪛 Markdownlint (0.35.0)

48-48: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


49-49: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)


50-50: Expected: 0; Actual: 2
Unordered list indentation

(MD007, ul-indent)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Uffizzi Preview deployment-58791 was deleted.

@mertmit mertmit merged commit ec464a3 into develop Dec 3, 2024
27 of 60 checks passed
@mertmit mertmit deleted the chore/syncoff branch December 3, 2024 20:35
@DarkPhoenix2704 DarkPhoenix2704 mentioned this pull request Dec 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants