fix: ensure file component text is visible in selected table rows #4365
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds an intersection-observer-based lazy-loading hook and integrates it into form cells to render Skeleton placeholders until visible; also introduces two new column config options and a presentational CSS override for selected file-upload thumbnails. Changes
Sequence Diagram(s)sequenceDiagram
participant FormCell as FormCell component
participant Hook as useIntersectionObserver
participant Browser as Browser (IntersectionObserver)
participant Renderer as Actual Cell Renderer
FormCell->>Hook: call useIntersectionObserver({ rootMargin, threshold })
Hook->>FormCell: return { elementRef, isVisible, hasBeenVisible }
alt not hasBeenVisible
FormCell->>FormCell: render Skeleton.Input with elementRef
end
Browser->>Hook: intersection events (observe elementRef)
Hook->>FormCell: update isVisible / hasBeenVisible
alt hasBeenVisible becomes true
FormCell->>Renderer: render real form cell content with elementRef attached
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @shesha-reactjs/src/components/dataTable/cell/formCell/formCell.tsx:
- Around line 137-148: Replace the unsafe "as any" ref casts by using the
generic typed ref hook consistently across FormCell, CreateFormCell and
EditFormCell: create/use a ref typed for HTMLDivElement (e.g., elementRef =
useRef<HTMLDivElement | null>(null) or the existing generic hook used elsewhere)
and pass it directly as ref={elementRef} instead of ref={elementRef as any};
update all occurrences referring to elementRef in the components CreateFormCell,
EditFormCell and the main form cell render paths so the ref is strongly typed
and no "as any" casts remain.
In @shesha-reactjs/src/hooks/useIntersectionObserver.ts:
- Line 58: The hook useIntersectionObserver currently defaults
options.rootMargin to '100px', which conflicts with the documented default
'150px' in ITableFormColumn.lazyLoadMargin; update the default in
useIntersectionObserver (the const destructuring of options in
useIntersectionObserver) to '150px' so it matches the interface documentation
and avoids inconsistencies when callers omit rootMargin (note formCell.tsx
already overrides to '150px', but keep the hook default aligned).
- Around line 74-92: The observer is being recreated because hasBeenVisible is
in the effect deps; remove hasBeenVisible from the dependency array and track it
with a ref inside the useIntersectionObserver hook (create hasBeenVisibleRef,
update it whenever you call setHasBeenVisible) so the IntersectionObserver
callback can read/set the ref without forcing the effect to rerun; keep
threshold, rootMargin and enabled as deps, update the observer callback to check
hasBeenVisibleRef.current and call setHasBeenVisible(true) once while also
setting hasBeenVisibleRef.current = true.
- Around line 1-61: The hook should be made generic to avoid casts: add a
generic parameter T extends HTMLElement = HTMLElement to useIntersectionObserver
and to the UseIntersectionObserverReturn so elementRef is React.RefObject<T> and
useRef<T | null>(null) is used; update any related types
(UseIntersectionObserverReturn, the function signature, and the elementRef
declaration) so consumers can call useIntersectionObserver<HTMLDivElement>() and
get properly typed refs without as any.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
shesha-reactjs/src/components/dataTable/cell/formCell/formCell.tsxshesha-reactjs/src/hooks/index.tsshesha-reactjs/src/hooks/useIntersectionObserver.tsshesha-reactjs/src/providers/dataTable/interfaces.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CodeRabbitReview-Project-Specific-Guidelines.md)
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types, forcing explicit type checking
Prefer type guards over type casting for type checking
Avoid monolithic types; use discriminated unions with a discriminator property instead
Leverage TypeScript to its full potential as a type system, not merely as a linter
Files:
shesha-reactjs/src/providers/dataTable/interfaces.tsshesha-reactjs/src/hooks/index.tsshesha-reactjs/src/hooks/useIntersectionObserver.tsshesha-reactjs/src/components/dataTable/cell/formCell/formCell.tsx
🔇 Additional comments (3)
shesha-reactjs/src/hooks/index.ts (1)
7-7: LGTM!The new export follows the established pattern in this file, properly re-exporting the hook and its associated types for public consumption.
shesha-reactjs/src/providers/dataTable/interfaces.ts (1)
100-116: LGTM!The new optional properties are well-documented with clear JSDoc comments explaining their purpose and default values. The types are appropriate (
booleanandstring).shesha-reactjs/src/components/dataTable/cell/formCell/formCell.tsx (1)
50-56: LGTM on the lazy loading integration.The hook usage is correct with appropriate configuration. The threshold of 0.1 and margin from config (defaulting to '150px') aligns well with the documented interface defaults.
Comment on lines
+69
to
+80
| // Render placeholder until row is visible | ||
| if (lazyLoadEnabled && !hasBeenVisible) { | ||
| return ( | ||
| <div | ||
| ref={elementRef as any} | ||
| className={styles.shaFormCell} | ||
| style={styleMinHeight} | ||
| > | ||
| <Skeleton.Input active size="small" style={{ width: '100%', height: '24px' }} /> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid as any type cast; prefer a properly typed ref.
The ref={elementRef as any} cast violates the guideline to eliminate the any type. The hook returns RefObject<HTMLElement>, but the div expects Ref<HTMLDivElement>. Consider making the hook generic to accept the element type, or use a callback ref pattern.
♻️ Suggested approach: Make the hook generic
In useIntersectionObserver.ts:
-export interface UseIntersectionObserverReturn {
+export interface UseIntersectionObserverReturn<T extends HTMLElement = HTMLElement> {
/**
* Ref to attach to the element to observe
*/
- elementRef: React.RefObject<HTMLElement>;
+ elementRef: React.RefObject<T>;
// ... rest unchanged
}
-export const useIntersectionObserver = (
+export const useIntersectionObserver = <T extends HTMLElement = HTMLElement>(
options: UseIntersectionObserverOptions = {}
-): UseIntersectionObserverReturn => {
+): UseIntersectionObserverReturn<T> => {
// ...
- const elementRef = useRef<HTMLElement>(null);
+ const elementRef = useRef<T>(null);Then in formCell.tsx:
- const { elementRef, hasBeenVisible } = useIntersectionObserver({
+ const { elementRef, hasBeenVisible } = useIntersectionObserver<HTMLDivElement>({
enabled: lazyLoadEnabled,
threshold: 0.1,
rootMargin: props.columnConfig.lazyLoadMargin ?? '150px'
});
// ...
- <div ref={elementRef as any} ...>
+ <div ref={elementRef} ...>As per coding guidelines, eliminate the any type and use proper type parameters.
Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+137
to
+148
| // Render placeholder until row is visible | ||
| if (lazyLoadEnabled && !hasBeenVisible) { | ||
| return ( | ||
| <div | ||
| ref={elementRef as any} | ||
| className={styles.shaFormCell} | ||
| style={styleMinHeight} | ||
| > | ||
| <Skeleton.Input active size="small" style={{ width: '100%', height: '24px' }} /> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same as any cast issue in CreateFormCell and EditFormCell.
The same type casting issue exists on lines 141, 156, 200, and 215. Apply the generic hook approach consistently across all three cell components.
Also applies to: 196-207
🤖 Prompt for AI Agents
In @shesha-reactjs/src/components/dataTable/cell/formCell/formCell.tsx around
lines 137 - 148, Replace the unsafe "as any" ref casts by using the generic
typed ref hook consistently across FormCell, CreateFormCell and EditFormCell:
create/use a ref typed for HTMLDivElement (e.g., elementRef =
useRef<HTMLDivElement | null>(null) or the existing generic hook used elsewhere)
and pass it directly as ref={elementRef} instead of ref={elementRef as any};
update all occurrences referring to elementRef in the components CreateFormCell,
EditFormCell and the main form cell render paths so the ref is strongly typed
and no "as any" casts remain.
Comment on lines
+1
to
+61
| import { useEffect, useRef, useState } from 'react'; | ||
|
|
||
| export interface UseIntersectionObserverOptions { | ||
| /** | ||
| * Threshold at which to trigger the callback (0-1, where 1 means 100% visible) | ||
| * @default 0.1 | ||
| */ | ||
| threshold?: number; | ||
| /** | ||
| * Margin around the root to expand or shrink the intersection area | ||
| * Positive values load content before it's visible (e.g., '100px') | ||
| * @default '100px' | ||
| */ | ||
| rootMargin?: string; | ||
| /** | ||
| * Whether intersection observer is enabled | ||
| * @default true | ||
| */ | ||
| enabled?: boolean; | ||
| } | ||
|
|
||
| export interface UseIntersectionObserverReturn { | ||
| /** | ||
| * Ref to attach to the element to observe | ||
| */ | ||
| elementRef: React.RefObject<HTMLElement>; | ||
| /** | ||
| * Whether the element is currently visible in the viewport | ||
| */ | ||
| isVisible: boolean; | ||
| /** | ||
| * Whether the element has ever been visible (doesn't reset to false) | ||
| */ | ||
| hasBeenVisible: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Hook to detect when an element enters the viewport using Intersection Observer API | ||
| * Useful for lazy loading content in tables or infinite scroll scenarios | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * const { elementRef, hasBeenVisible } = useIntersectionObserver({ | ||
| * threshold: 0.1, | ||
| * rootMargin: '150px' | ||
| * }); | ||
| * | ||
| * return ( | ||
| * <div ref={elementRef}> | ||
| * {hasBeenVisible ? <ExpensiveComponent /> : <Skeleton />} | ||
| * </div> | ||
| * ); | ||
| * ``` | ||
| */ | ||
| export const useIntersectionObserver = ( | ||
| options: UseIntersectionObserverOptions = {} | ||
| ): UseIntersectionObserverReturn => { | ||
| const { threshold = 0.1, rootMargin = '100px', enabled = true } = options; | ||
| const [isVisible, setIsVisible] = useState(false); | ||
| const [hasBeenVisible, setHasBeenVisible] = useState(false); | ||
| const elementRef = useRef<HTMLElement>(null); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the hook generic for better type safety.
The elementRef is typed as RefObject<HTMLElement>, which requires as any casts when used with specific element types (e.g., HTMLDivElement). Making the hook generic would improve type safety and eliminate the need for type casts in consumers.
♻️ Proposed generic hook signature
-export interface UseIntersectionObserverReturn {
+export interface UseIntersectionObserverReturn<T extends HTMLElement = HTMLElement> {
/**
* Ref to attach to the element to observe
*/
- elementRef: React.RefObject<HTMLElement>;
+ elementRef: React.RefObject<T>;
/**
* Whether the element is currently visible in the viewport
*/
isVisible: boolean;
/**
* Whether the element has ever been visible (doesn't reset to false)
*/
hasBeenVisible: boolean;
}
-export const useIntersectionObserver = (
+export const useIntersectionObserver = <T extends HTMLElement = HTMLElement>(
options: UseIntersectionObserverOptions = {}
-): UseIntersectionObserverReturn => {
+): UseIntersectionObserverReturn<T> => {
const { threshold = 0.1, rootMargin = '100px', enabled = true } = options;
const [isVisible, setIsVisible] = useState(false);
const [hasBeenVisible, setHasBeenVisible] = useState(false);
- const elementRef = useRef<HTMLElement>(null);
+ const elementRef = useRef<T>(null);As per coding guidelines, leverage TypeScript to its full potential and avoid type casts.
📝 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
| import { useEffect, useRef, useState } from 'react'; | |
| export interface UseIntersectionObserverOptions { | |
| /** | |
| * Threshold at which to trigger the callback (0-1, where 1 means 100% visible) | |
| * @default 0.1 | |
| */ | |
| threshold?: number; | |
| /** | |
| * Margin around the root to expand or shrink the intersection area | |
| * Positive values load content before it's visible (e.g., '100px') | |
| * @default '100px' | |
| */ | |
| rootMargin?: string; | |
| /** | |
| * Whether intersection observer is enabled | |
| * @default true | |
| */ | |
| enabled?: boolean; | |
| } | |
| export interface UseIntersectionObserverReturn { | |
| /** | |
| * Ref to attach to the element to observe | |
| */ | |
| elementRef: React.RefObject<HTMLElement>; | |
| /** | |
| * Whether the element is currently visible in the viewport | |
| */ | |
| isVisible: boolean; | |
| /** | |
| * Whether the element has ever been visible (doesn't reset to false) | |
| */ | |
| hasBeenVisible: boolean; | |
| } | |
| /** | |
| * Hook to detect when an element enters the viewport using Intersection Observer API | |
| * Useful for lazy loading content in tables or infinite scroll scenarios | |
| * | |
| * @example | |
| * ```tsx | |
| * const { elementRef, hasBeenVisible } = useIntersectionObserver({ | |
| * threshold: 0.1, | |
| * rootMargin: '150px' | |
| * }); | |
| * | |
| * return ( | |
| * <div ref={elementRef}> | |
| * {hasBeenVisible ? <ExpensiveComponent /> : <Skeleton />} | |
| * </div> | |
| * ); | |
| * ``` | |
| */ | |
| export const useIntersectionObserver = ( | |
| options: UseIntersectionObserverOptions = {} | |
| ): UseIntersectionObserverReturn => { | |
| const { threshold = 0.1, rootMargin = '100px', enabled = true } = options; | |
| const [isVisible, setIsVisible] = useState(false); | |
| const [hasBeenVisible, setHasBeenVisible] = useState(false); | |
| const elementRef = useRef<HTMLElement>(null); | |
| import { useEffect, useRef, useState } from 'react'; | |
| export interface UseIntersectionObserverOptions { | |
| /** | |
| * Threshold at which to trigger the callback (0-1, where 1 means 100% visible) | |
| * @default 0.1 | |
| */ | |
| threshold?: number; | |
| /** | |
| * Margin around the root to expand or shrink the intersection area | |
| * Positive values load content before it's visible (e.g., '100px') | |
| * @default '100px' | |
| */ | |
| rootMargin?: string; | |
| /** | |
| * Whether intersection observer is enabled | |
| * @default true | |
| */ | |
| enabled?: boolean; | |
| } | |
| export interface UseIntersectionObserverReturn<T extends HTMLElement = HTMLElement> { | |
| /** | |
| * Ref to attach to the element to observe | |
| */ | |
| elementRef: React.RefObject<T>; | |
| /** | |
| * Whether the element is currently visible in the viewport | |
| */ | |
| isVisible: boolean; | |
| /** | |
| * Whether the element has ever been visible (doesn't reset to false) | |
| */ | |
| hasBeenVisible: boolean; | |
| } | |
| /** | |
| * Hook to detect when an element enters the viewport using Intersection Observer API | |
| * Useful for lazy loading content in tables or infinite scroll scenarios | |
| * | |
| * @example | |
| * |
🤖 Prompt for AI Agents
In @shesha-reactjs/src/hooks/useIntersectionObserver.ts around lines 1 - 61, The
hook should be made generic to avoid casts: add a generic parameter T extends
HTMLElement = HTMLElement to useIntersectionObserver and to the
UseIntersectionObserverReturn so elementRef is React.RefObject<T> and useRef<T |
null>(null) is used; update any related types (UseIntersectionObserverReturn,
the function signature, and the elementRef declaration) so consumers can call
useIntersectionObserver<HTMLDivElement>() and get properly typed refs without as
any.
| export const useIntersectionObserver = ( | ||
| options: UseIntersectionObserverOptions = {} | ||
| ): UseIntersectionObserverReturn => { | ||
| const { threshold = 0.1, rootMargin = '100px', enabled = true } = options; |
Contributor
There was a problem hiding this comment.
Default rootMargin inconsistency with interface documentation.
The hook defaults rootMargin to '100px', but ITableFormColumn.lazyLoadMargin documents '150px' as the default. While the formCell.tsx overrides this with '150px', consider aligning the hook's default for consistency if it's used elsewhere without an explicit margin.
🤖 Prompt for AI Agents
In @shesha-reactjs/src/hooks/useIntersectionObserver.ts at line 58, The hook
useIntersectionObserver currently defaults options.rootMargin to '100px', which
conflicts with the documented default '150px' in
ITableFormColumn.lazyLoadMargin; update the default in useIntersectionObserver
(the const destructuring of options in useIntersectionObserver) to '150px' so it
matches the interface documentation and avoids inconsistencies when callers omit
rootMargin (note formCell.tsx already overrides to '150px', but keep the hook
default aligned).
Comment on lines
+74
to
+92
| const observer = new IntersectionObserver( | ||
| ([entry]) => { | ||
| const visible = entry.isIntersecting; | ||
| setIsVisible(visible); | ||
|
|
||
| // Once visible, always mark as "has been visible" | ||
| if (visible && !hasBeenVisible) { | ||
| setHasBeenVisible(true); | ||
| } | ||
| }, | ||
| { threshold, rootMargin } | ||
| ); | ||
|
|
||
| observer.observe(element); | ||
|
|
||
| return () => { | ||
| observer.disconnect(); | ||
| }; | ||
| }, [threshold, rootMargin, enabled, hasBeenVisible]); |
Contributor
There was a problem hiding this comment.
Stale closure and unnecessary observer recreation when hasBeenVisible changes.
Including hasBeenVisible in the dependency array causes the observer to disconnect and reconnect when the element first becomes visible. This is inefficient. Use a ref to track hasBeenVisible internally, or use a functional state update to avoid the closure issue.
🐛 Proposed fix using ref for hasBeenVisible tracking
export const useIntersectionObserver = (
options: UseIntersectionObserverOptions = {}
): UseIntersectionObserverReturn => {
const { threshold = 0.1, rootMargin = '100px', enabled = true } = options;
const [isVisible, setIsVisible] = useState(false);
const [hasBeenVisible, setHasBeenVisible] = useState(false);
+ const hasBeenVisibleRef = useRef(false);
const elementRef = useRef<HTMLElement>(null);
useEffect(() => {
// If disabled, immediately mark as visible
if (!enabled) {
setIsVisible(true);
setHasBeenVisible(true);
+ hasBeenVisibleRef.current = true;
return;
}
const element = elementRef.current;
if (!element) return;
const observer = new IntersectionObserver(
([entry]) => {
const visible = entry.isIntersecting;
setIsVisible(visible);
// Once visible, always mark as "has been visible"
- if (visible && !hasBeenVisible) {
+ if (visible && !hasBeenVisibleRef.current) {
+ hasBeenVisibleRef.current = true;
setHasBeenVisible(true);
}
},
{ threshold, rootMargin }
);
observer.observe(element);
return () => {
observer.disconnect();
};
- }, [threshold, rootMargin, enabled, hasBeenVisible]);
+ }, [threshold, rootMargin, enabled]);
return { elementRef, isVisible, hasBeenVisible };
};🤖 Prompt for AI Agents
In @shesha-reactjs/src/hooks/useIntersectionObserver.ts around lines 74 - 92,
The observer is being recreated because hasBeenVisible is in the effect deps;
remove hasBeenVisible from the dependency array and track it with a ref inside
the useIntersectionObserver hook (create hasBeenVisibleRef, update it whenever
you call setHasBeenVisible) so the IntersectionObserver callback can read/set
the ref without forcing the effect to rerun; keep threshold, rootMargin and
enabled as deps, update the observer callback to check hasBeenVisibleRef.current
and call setHasBeenVisible(true) once while also setting
hasBeenVisibleRef.current = true.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.