fix: review comments

Author: Elessar1802

src/Common/Hooks/UseRegisterShortcut/UseRegisterShortcutProvider.tsx

@@ -26,10 +26,8 @@ const DEFAULT_TIMEOUT = 300
 const UseRegisterShortcutProvider = ({
     ignoreTags,
     preventDefault = false,
-    stopPropagation = false,
     shortcutTimeout,
     children,
-    eventListenerTargetRef,
 }: UseRegisterShortcutProviderType) => {
     const disableShortcutsRef = useRef<boolean>(false)
     const shortcutsRef = useRef<Record<string, ShortcutType>>({})
@@ -122,10 +120,6 @@ const UseRegisterShortcutProvider = ({
             event.preventDefault()
         }
 
-        if (stopPropagation) {
-            event.stopPropagation()
-        }
-
         if (
             ignoredTags.map((tag) => tag.toUpperCase()).indexOf((event.target as HTMLElement).tagName.toUpperCase()) >
                 -1 ||
@@ -162,16 +156,14 @@ const UseRegisterShortcutProvider = ({
     }, [])
 
     useEffect(() => {
-        const target = eventListenerTargetRef?.current ?? window
-
-        target.addEventListener('keydown', handleKeydownEvent)
-        target.addEventListener('keyup', handleKeyupEvent)
-        target.addEventListener('blur', handleBlur)
+        window.addEventListener('keydown', handleKeydownEvent)
+        window.addEventListener('keyup', handleKeyupEvent)
+        window.addEventListener('blur', handleBlur)
 
         return () => {
-            target.removeEventListener('keydown', handleKeydownEvent)
-            target.removeEventListener('keyup', handleKeyupEvent)
-            target.removeEventListener('blur', handleBlur)
+            window.removeEventListener('keydown', handleKeydownEvent)
+            window.removeEventListener('keyup', handleKeyupEvent)
+            window.removeEventListener('blur', handleBlur)
 
             if (keyDownTimeoutRef.current > -1) {
                 clearTimeout(keyDownTimeoutRef.current)

src/Common/Hooks/UseRegisterShortcut/types.ts

@@ -93,13 +93,4 @@ export interface UseRegisterShortcutProviderType {
      * If true, call preventDefault on the event
      */
     preventDefault?: boolean
-    /**
-     * If true, call stopPropagation on the event
-     */
-    stopPropagation?: boolean
-    /**
-     * The target to which the event listener is attached
-     * If not provided, the event listener will be attached to window
-     */
-    eventListenerTargetRef?: React.RefObject<HTMLElement>
 }

src/Shared/Components/Table/InternalTable.tsx

@@ -41,10 +41,10 @@ const InternalTable = ({
     handleToggleBulkSelectionOnRow,
     paginationVariant,
     RowActionsOnHoverComponent,
-    tableContainerRef,
 }: InternalTableProps) => {
     const rowsContainerRef = useRef<HTMLDivElement>(null)
     const parentRef = useRef<HTMLDivElement>(null)
+    const activeRowRef = useRef<HTMLDivElement>(null)
 
     const { BulkActionsComponent } = bulkSelectionConfig ?? {}
 
@@ -138,15 +138,8 @@ const InternalTable = ({
     )
 
     useEffectAfterMount(() => {
-        rowsContainerRef.current.scrollTo({
-            top: 0,
-            left: 0,
-            behavior: 'smooth',
-        })
-
-        // !FIXME: this will scroll to top but without smooth animation
         setActiveRowIndex(0)
-    }, [offset])
+    }, [offset, visibleRows])
 
     useEffect(() => {
         setIdentifiers?.(
@@ -163,13 +156,14 @@ const InternalTable = ({
         handleSorting(newSortBy)
     }
 
-    const scrollIntoViewActiveRowRefCallback = (node: HTMLDivElement) => {
-        if (!node || node.dataset.active !== 'true') {
-            return
-        }
-
-        node.focus()
-    }
+    useEffect(() => {
+        // Focus the active row only when activeRowIndex changes. This ensures that focus is not stolen from other elements,
+        // such as the search bar, when it is already focused. For example, when typing in the search bar, the rows may be replaced
+        // with loading shimmers, causing activeRowRef to be null. During this time, activeRowIndex might reset to 0, but focus
+        // will not shift. However, when navigating with arrow keys, activeRowIndex changes, and activeRowRef will point to the
+        // correct row once it is mounted, allowing focus to update appropriately.
+        activeRowRef.current?.focus()
+    }, [activeRowIndex])
 
     const showPagination =
         paginationVariant === PaginationEnum.PAGINATED && filteredRows?.length > DEFAULT_BASE_PAGE_SIZE
@@ -225,12 +219,12 @@ const InternalTable = ({
             return (
                 <div
                     key={row.id}
-                    ref={scrollIntoViewActiveRowRefCallback}
+                    ref={isRowActive ? activeRowRef : null}
                     onClick={handleChangeActiveRowIndex}
-                    className={`dc__grid px-20 form__checkbox-parent ${
+                    className={`dc__grid px-20 checkbox__parent-container ${
                         showSeparatorBetweenRows ? 'border__secondary--bottom' : ''
                     } fs-13 fw-4 lh-20 cn-9 generic-table__row dc__gap-16 ${
-                        isRowActive ? 'generic-table__row--active form__checkbox-parent--active' : ''
+                        isRowActive ? 'generic-table__row--active checkbox__parent-container--active' : ''
                     } ${RowActionsOnHoverComponent ? 'dc__position-rel dc__opacity-hover dc__opacity-hover--parent' : ''} ${
                         isRowBulkSelected ? 'generic-table__row--bulk-selected' : ''
                     }`}
@@ -299,7 +293,7 @@ const InternalTable = ({
         }
 
         return (
-            <>
+            <div tabIndex={0} role="grid" className="generic-table flexbox-col dc__overflow-hidden flex-grow-1">
                 <div className="flexbox-col flex-grow-1 w-100 dc__overflow-auto" ref={parentRef}>
                     <div className="bg__primary dc__min-width-fit-content px-20 border__secondary--bottom">
                         {loading && !visibleColumns.length ? (
@@ -369,7 +363,7 @@ const InternalTable = ({
                         size={filteredRows.length}
                     />
                 )}
-            </>
+            </div>
         )
     }
 
@@ -388,9 +382,7 @@ const InternalTable = ({
                     : {}),
             }}
         >
-            <div ref={tableContainerRef} className="generic-table flexbox-col dc__overflow-hidden flex-grow-1">
-                {renderContent()}
-            </div>
+            {renderContent()}
         </Wrapper>
     )
 }

src/Shared/Components/Table/Table.component.tsx

@@ -1,4 +1,4 @@
-import { useCallback, useEffect, useMemo, useRef, useState } from 'react'
+import { useCallback, useEffect, useMemo, useState } from 'react'
 import {
     noop,
     UseRegisterShortcutProvider,
@@ -25,10 +25,10 @@ import InternalTable from './InternalTable'
 import './styles.scss'
 
 const UseResizableTableConfigWrapper = (props: InternalTableProps) => {
-    const { columns } = props
+    const { visibleColumns } = props
 
     const resizableConfig = useResizableTableConfig({
-        headersConfig: columns.map(({ label, size }) => {
+        headersConfig: visibleColumns.map(({ label, size }) => {
             if (size.range) {
                 const {
                     range: { minWidth, maxWidth, startWidth },
@@ -189,25 +189,20 @@ const UseUrlFilterWrapper = (props: FilterWrapperProps) => {
 
 const TableWrapper = (tableProps: TableProps) => {
     const { filtersVariant } = tableProps
-    const tableContainerRef = useRef<HTMLDivElement>(null)
 
     const renderContent = () => {
-        if (filtersVariant === FiltersTypeEnum.NONE) {
-            return <UseStateFilterWrapper {...{ ...tableProps, tableContainerRef }} />
+        if (filtersVariant === FiltersTypeEnum.STATE) {
+            return <UseStateFilterWrapper {...tableProps} />
         }
 
         if (filtersVariant === FiltersTypeEnum.URL) {
-            return <UseUrlFilterWrapper {...{ ...tableProps, tableContainerRef }} />
+            return <UseUrlFilterWrapper {...tableProps} />
         }
 
-        return <VisibleColumnsWrapper {...{ ...tableProps, tableContainerRef, filterData: null }} />
+        return <VisibleColumnsWrapper {...{ ...tableProps, filterData: null }} />
     }
 
-    return (
-        <UseRegisterShortcutProvider eventListenerTargetRef={tableContainerRef} shortcutTimeout={50} stopPropagation>
-            {renderContent()}
-        </UseRegisterShortcutProvider>
-    )
+    return <UseRegisterShortcutProvider shortcutTimeout={50}>{renderContent()}</UseRegisterShortcutProvider>
 }
 
 export default TableWrapper

src/Shared/Components/Table/constants.ts

@@ -11,7 +11,7 @@ export const EVENT_TARGET = new EventTarget()
 export const DRAG_SELECTOR_IDENTIFIER = 'table-drag-selector'
 
 export const TABLE_ID_MAP = {
-    STORYBOOK: 'storybook',
+    STORYBOOK: 'storybook-table',
 } as const
 
 export const SHIMMER_DUMMY_ARRAY = [1, 2, 3]

src/Shared/Components/Table/types.ts

@@ -190,8 +190,6 @@ export type InternalTableProps = Required<Pick<ConfigurableColumnsType, 'visible
         showSeparatorBetweenRows: boolean
     }
 
-    tableContainerRef: React.RefObject<HTMLDivElement>
-
     /**
      * Use this component to display additional content at the end of a row when it is hovered over.
      */

src/Shared/Components/Table/utils.ts

@@ -22,7 +22,7 @@ export const searchAndSortRows = (
     filterData: UseFiltersReturnType,
     comparator?: Column['comparator'],
 ) => {
-    const { searchKey, sortBy, sortOrder } = filterData
+    const { searchKey, sortBy, sortOrder } = filterData ?? {}
 
     const filteredRows = searchKey ? rows.filter((row) => filter(row, filterData)) : rows