fix(ui): resolve admin user table infinite scroll tracking, counter indicator, and spacing gaps#29597
fix(ui): resolve admin user table infinite scroll tracking, counter indicator, and spacing gaps#29597vaasu2906 wants to merge 2 commits into
Conversation
|
Welcome to Cal.diy, @vaasu2906! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
The branch has been completely synced with the latest upstream repository. Please review the infinite scroll logic, UI spacing fixes, and trigger the pull_request_target workflow checks. |
📝 WalkthroughWalkthroughThe DataTable component undergoes a scroll handling refactoring: the scroll event triggering mechanism shifts from effect-based reconciliation to direct onScroll prop handling via a handleScroll wrapper. The onScroll prop contract is broadened to accept the full React.UIEvent. Infinite pagination support is added by deriving totalCount from table metadata and computing hasMore, with corresponding sticky UI elements that display loading states, no-more-results messages, and a load-more button. The column header resize drag handle markup is removed, and row rendering attributes are updated. The Input component gains hydration safety by tracking mount state with useState and useEffect, conditionally rendering a server fallback input until the component is mounted on the client, then switching to the full interactive input with suppressHydrationWarning applied. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/prisma/.env`:
- Around line 1-4: Remove the `.env` file from this commit as it contains
hardcoded database passwords, NEXTAUTH_SECRET, and CALENDSO_ENCRYPTION_KEY that
must never be committed to version control. Verify that `.env` is listed in
`.gitignore` at the repository root to prevent future commits. Create a
`.env.example` file in the packages/prisma directory with the same variable
names but placeholder values (e.g.,
DATABASE_URL="postgresql://user:password@localhost:5432/dbname") so developers
know what variables are needed. Regenerate and rotate all exposed credentials
(database password, NEXTAUTH_SECRET, CALENDSO_ENCRYPTION_KEY) in your actual
environment, and ensure your deployment process loads these secrets from secure
sources like environment variables or a secret manager instead of committed
files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f21b2556-0291-49f7-91c7-b14417d9f15a
📒 Files selected for processing (1)
packages/prisma/.env
vaasu2906
force-pushed
the
feature/fix-datatable-virtual-scroll
branch
2 times, most recently
from
9b3353a to
80838c0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/prisma/.env (1)
1-4:⚠️ Potential issue | 🔴 Critical[DUPLICATE] CRITICAL: Hardcoded credentials in version control — same issue already flagged.
A previous review already identified this exact critical vulnerability on this file. The
.envfile containing plaintext database password (Vasu@5661),NEXTAUTH_SECRET, andCALENDSO_ENCRYPTION_KEYmust not be committed to Git. These credentials are now exposed in repository history to all users with access, including forks and pull requests.This is still unresolved — the file should have been removed from this PR or regenerated with redacted values before resubmission.
Refer to the existing critical review comment above (with detailed remediation:
.gitignoreverification,.env.examplecreation, credential rotation, and secret manager integration) for the required fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/prisma/.env` around lines 1 - 4, The packages/prisma/.env file contains hardcoded sensitive credentials including database passwords (DATABASE_URL, DATABASE_DIRECT_URL), authentication secrets (NEXTAUTH_SECRET), and encryption keys (CALENDSO_ENCRYPTION_KEY) that must never be committed to version control. To fix this: remove the .env file from this PR entirely, verify that .env is listed in the .gitignore file to prevent future commits, create a .env.example file with the same variable names but placeholder or redacted values (no actual secrets), and rotate all exposed credentials (database password, NEXTAUTH_SECRET, and CALENDSO_ENCRYPTION_KEY) since they are now in the repository history. Consider implementing a secrets manager or environment variable service for managing these credentials securely going forward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/prisma/.env`:
- Around line 1-4: The packages/prisma/.env file contains hardcoded sensitive
credentials including database passwords (DATABASE_URL, DATABASE_DIRECT_URL),
authentication secrets (NEXTAUTH_SECRET), and encryption keys
(CALENDSO_ENCRYPTION_KEY) that must never be committed to version control. To
fix this: remove the .env file from this PR entirely, verify that .env is listed
in the .gitignore file to prevent future commits, create a .env.example file
with the same variable names but placeholder or redacted values (no actual
secrets), and rotate all exposed credentials (database password,
NEXTAUTH_SECRET, and CALENDSO_ENCRYPTION_KEY) since they are now in the
repository history. Consider implementing a secrets manager or environment
variable service for managing these credentials securely going forward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a92949f8-040b-48b5-bbf5-d6ca19130885
📒 Files selected for processing (2)
packages/prisma/.envpackages/prisma/.env
vaasu2906
force-pushed
the
feature/fix-datatable-virtual-scroll
branch
from
80838c0 to
c1f4e13
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/prisma/.env (1)
1-4:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift[DUPLICATE] Unresolved critical security vulnerability: hardcoded credentials in version control.
The same hardcoded database password and application secrets flagged in the previous review remain present and unaddressed. This file should not have been resubmitted with these credentials still exposed.
Per the prior critical review comment:
- Database password
Vasu@5661is now exposed in Git historyNEXTAUTH_SECRETandCALENDSO_ENCRYPTION_KEYare compromised- All systems using these credentials are at risk
This must be remediated before merge:
- Remove this file from the commit (do not commit
.env)- Verify
.envis listed in.gitignore- Immediately rotate all exposed credentials in your actual environment
- Use the detailed remediation steps from the prior review comment
Note: This PR's stated objectives concern admin users table UI fixes (infinite scroll, spacing); environment configuration changes belong in a separate, carefully audited PR with proper secret rotation ceremonies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/prisma/.env` around lines 1 - 4, Remove the packages/prisma/.env file from the current commit by using git rm --cached on this file, then verify that .env files are properly listed in .gitignore at the project root and packages/prisma directory level to prevent future commits. Create a .env.example template file with placeholder values (replacing the actual credentials like the database password, NEXTAUTH_SECRET, and CALENDSO_ENCRYPTION_KEY) to serve as documentation for developers on what environment variables are needed. Immediately rotate all exposed credentials in your actual environment systems since the database password and application secrets are now compromised in the repository history. Amend the commit to remove this file before proceeding with the merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/prisma/.env`:
- Around line 1-4: Remove the packages/prisma/.env file from the current commit
by using git rm --cached on this file, then verify that .env files are properly
listed in .gitignore at the project root and packages/prisma directory level to
prevent future commits. Create a .env.example template file with placeholder
values (replacing the actual credentials like the database password,
NEXTAUTH_SECRET, and CALENDSO_ENCRYPTION_KEY) to serve as documentation for
developers on what environment variables are needed. Immediately rotate all
exposed credentials in your actual environment systems since the database
password and application secrets are now compromised in the repository history.
Amend the commit to remove this file before proceeding with the merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 330d12ce-4e17-4f7f-9a78-eb2b28bc5b4f
📒 Files selected for processing (2)
packages/prisma/.envpackages/prisma/.env
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/prisma/.env
vaasu2906
force-pushed
the
feature/fix-datatable-virtual-scroll
branch
4 times, most recently
from
bd8a2f1 to
7f44e36
Compare
…ndicator, and spacing gaps
vaasu2906
force-pushed
the
feature/fix-datatable-virtual-scroll
branch
from
7f44e36 to
bdd942a
Compare
…ndicator, and spacing gaps
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/coss-ui/src/components/input.tsx (1)
25-25: ⚡ Quick winUse “why” comments (or remove these explanatory comments)
Line 25 and Line 57 comments restate what the code is doing instead of why the decision exists. Prefer removing them or rewriting with rationale only.
As per coding guidelines, “Only add code comments that explain why, not what” and “Never add comments that simply restate what the code does.”
Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/coss-ui/src/components/input.tsx` at line 25, The comments at line 25 and line 57 in the input component explain what the code does rather than why the design decision exists. Either remove these comments entirely since the code is self-explanatory, or if they must remain, rewrite them to explain the rationale and reasoning behind implementing client-side hydration safety toggling instead of simply restating that this feature exists.Source: Coding guidelines
apps/web/modules/data-table/components/DataTable.tsx (1)
106-110: ⚡ Quick winType safety concern with
totalCountextraction.The current implementation calls
table.getMeta()multiple times and theas numbercast could produceNaNor unexpected values iftotalCountis not actually a number. Consider a safer extraction:- const totalCount = table.getMeta() && typeof table.getMeta() === "object" && "totalCount" in (table.getMeta() as Record<string, unknown>) - ? ((table.getMeta() as Record<string, unknown>).totalCount as number) - : rows.length; + const meta = table.getMeta(); + const rawTotalCount = meta && typeof meta === "object" && "totalCount" in meta + ? (meta as { totalCount?: unknown }).totalCount + : undefined; + const totalCount = typeof rawTotalCount === "number" && rawTotalCount >= 0 + ? rawTotalCount + : rows.length;This avoids multiple
getMeta()calls and validates thattotalCountis actually a non-negative number before using it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/modules/data-table/components/DataTable.tsx` around lines 106 - 110, The totalCount extraction is calling table.getMeta() multiple times and uses an unsafe type cast with as number that doesn't validate the actual value. Store the result of table.getMeta() in a single variable once, then safely extract the totalCount property with proper validation to ensure it is actually a number and a valid non-negative value before assigning it to the totalCount variable. This eliminates redundant calls to table.getMeta() and prevents NaN or unexpected values from being used in the hasMore calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/modules/data-table/components/DataTable.tsx`:
- Around line 217-228: The DataTable component contains hardcoded UI strings
"Loading next page..." and "No more results" that should be internationalized.
Replace these hardcoded strings with translation calls using the useLocale hook
that is already imported but currently unused in the component. Create
appropriate translation keys for both strings and add them to
packages/i18n/locales/en/common.json with their English values, then reference
these keys in the DataTable component where the isPending and !isPending
conditional blocks display the loading and completion messages.
- Around line 231-248: The simulated event object passed to the onScroll handler
is incomplete and may cause issues if handlers expect properties beyond
currentTarget and target. Add a dedicated onLoadMore prop to the DataTable
component that gets called when the "Load more" button is clicked, bypassing the
need for a simulated event. Additionally, replace the hardcoded "Load more"
string in the button with an appropriate i18n translation function call to
support internationalization.
In `@packages/coss-ui/src/components/input.tsx`:
- Around line 58-67: The fallback input element in the input component is
missing critical semantic and form-related props that affect pre-hydration
behavior. Add forwarding of props like id, name, autoComplete, and ARIA
attributes (such as aria-label, aria-describedby, etc.) to the fallback input
element so that label associations, autofill hints, and form semantics are
preserved during the pre-hydration phase.
---
Nitpick comments:
In `@apps/web/modules/data-table/components/DataTable.tsx`:
- Around line 106-110: The totalCount extraction is calling table.getMeta()
multiple times and uses an unsafe type cast with as number that doesn't validate
the actual value. Store the result of table.getMeta() in a single variable once,
then safely extract the totalCount property with proper validation to ensure it
is actually a number and a valid non-negative value before assigning it to the
totalCount variable. This eliminates redundant calls to table.getMeta() and
prevents NaN or unexpected values from being used in the hasMore calculation.
In `@packages/coss-ui/src/components/input.tsx`:
- Line 25: The comments at line 25 and line 57 in the input component explain
what the code does rather than why the design decision exists. Either remove
these comments entirely since the code is self-explanatory, or if they must
remain, rewrite them to explain the rationale and reasoning behind implementing
client-side hydration safety toggling instead of simply restating that this
feature exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 582a5b01-9712-45ac-ac61-73e4846c6252
📒 Files selected for processing (3)
apps/web/modules/data-table/components/DataTable.tsxpackages/coss-ui/src/components/input.tsxpackages/prisma/.env
💤 Files with no reviewable changes (1)
- packages/prisma/.env
|
|
||
| {isPending && ( | ||
| <div className="p-4 text-center text-sm text-muted-foreground bg-default border-t border-subtle sticky bottom-0 z-10 w-full animate-pulse"> | ||
| Loading next page... | ||
| </div> | ||
| )} | ||
|
|
||
| {!isPending && !hasMore && rows.length > 0 && ( | ||
| <div className="p-4 text-center text-sm text-muted-foreground bg-default border-t border-subtle sticky bottom-0 z-10 w-full"> | ||
| No more results | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
UI strings should use i18n translations.
The strings "Loading next page..." and "No more results" are hardcoded. Per coding guidelines, all UI strings should be translated. The useLocale hook is already imported but not used in this component.
+ const { t } = useLocale();
+
// ... inside the component
{isPending && (
<div className="p-4 text-center text-sm text-muted-foreground bg-default border-t border-subtle sticky bottom-0 z-10 w-full animate-pulse">
- Loading next page...
+ {t("loading_next_page")}
</div>
)}
{!isPending && !hasMore && rows.length > 0 && (
<div className="p-4 text-center text-sm text-muted-foreground bg-default border-t border-subtle sticky bottom-0 z-10 w-full">
- No more results
+ {t("no_more_results")}
</div>
)}Add the corresponding keys to packages/i18n/locales/en/common.json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/modules/data-table/components/DataTable.tsx` around lines 217 - 228,
The DataTable component contains hardcoded UI strings "Loading next page..." and
"No more results" that should be internationalized. Replace these hardcoded
strings with translation calls using the useLocale hook that is already imported
but currently unused in the component. Create appropriate translation keys for
both strings and add them to packages/i18n/locales/en/common.json with their
English values, then reference these keys in the DataTable component where the
isPending and !isPending conditional blocks display the loading and completion
messages.
Source: Coding guidelines
| {paginationMode === "infinite" && hasMore && !isPending && ( | ||
| <div style={{ gridArea: "fallback" }} className="flex justify-center w-full pt-4"> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| if (tableContainerRef.current && onScroll) { | ||
| const simulatedEvent = { | ||
| currentTarget: tableContainerRef.current, | ||
| target: tableContainerRef.current, | ||
| } as unknown as React.UIEvent<HTMLDivElement>; | ||
| onScroll(simulatedEvent); | ||
| } | ||
| }} | ||
| className="inline-flex items-center justify-center rounded-md border border-button-neutral bg-default px-4 py-2 text-sm font-medium text-default shadow-sm hover:bg-muted focus:outline-none focus:ring-2 focus:ring-brand focus:ring-offset-2 w-full max-w-xs transition-colors"> | ||
| Load more | ||
| </button> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Simulated event object may cause issues with scroll handlers expecting full event properties.
The "Load more" button creates a minimal simulated event object. If onScroll handlers rely on properties like scrollTop, scrollHeight, or event methods, this will fail silently or throw. Consider either:
- Exposing a dedicated
onLoadMoreprop for explicit manual loading - Documenting that
onScrollhandlers should only usecurrentTarget/target
Also, "Load more" should use i18n:
- Load more
+ {t("load_more")}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/modules/data-table/components/DataTable.tsx` around lines 231 - 248,
The simulated event object passed to the onScroll handler is incomplete and may
cause issues if handlers expect properties beyond currentTarget and target. Add
a dedicated onLoadMore prop to the DataTable component that gets called when the
"Load more" button is clicked, bypassing the need for a simulated event.
Additionally, replace the hardcoded "Load more" string in the button with an
appropriate i18n translation function call to support internationalization.
Source: Coding guidelines
| <input | ||
| className={inputClassName} | ||
| data-slot="input" | ||
| size={typeof size === "number" ? size : undefined} | ||
| type={props.type} | ||
| disabled={props.disabled} | ||
| placeholder={props.placeholder} | ||
| readOnly | ||
| suppressHydrationWarning | ||
| /> |
There was a problem hiding this comment.
Fallback input drops critical semantic/form props before hydration
On Line 58–67, the fallback <input> omits forwarded props like id, name, autoComplete, and ARIA attributes. That creates a transient but real pre-hydration behavior gap (label association, autofill hints, form semantics).
Suggested patch
<input
className={inputClassName}
data-slot="input"
size={typeof size === "number" ? size : undefined}
type={props.type}
disabled={props.disabled}
placeholder={props.placeholder}
+ id={props.id}
+ name={props.name}
+ autoComplete={props.autoComplete}
+ form={props.form}
+ required={props.required}
+ aria-label={props["aria-label"]}
+ aria-labelledby={props["aria-labelledby"]}
+ aria-describedby={props["aria-describedby"]}
readOnly
suppressHydrationWarning
/>📝 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.
| <input | |
| className={inputClassName} | |
| data-slot="input" | |
| size={typeof size === "number" ? size : undefined} | |
| type={props.type} | |
| disabled={props.disabled} | |
| placeholder={props.placeholder} | |
| readOnly | |
| suppressHydrationWarning | |
| /> | |
| <input | |
| className={inputClassName} | |
| data-slot="input" | |
| size={typeof size === "number" ? size : undefined} | |
| type={props.type} | |
| disabled={props.disabled} | |
| placeholder={props.placeholder} | |
| id={props.id} | |
| name={props.name} | |
| autoComplete={props.autoComplete} | |
| form={props.form} | |
| required={props.required} | |
| aria-label={props["aria-label"]} | |
| aria-labelledby={props["aria-labelledby"]} | |
| aria-describedby={props["aria-describedby"]} | |
| readOnly | |
| suppressHydrationWarning | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/coss-ui/src/components/input.tsx` around lines 58 - 67, The fallback
input element in the input component is missing critical semantic and
form-related props that affect pre-hydration behavior. Add forwarding of props
like id, name, autoComplete, and ARIA attributes (such as aria-label,
aria-describedby, etc.) to the fallback input element so that label
associations, autofill hints, and form semantics are preserved during the
pre-hydration phase.
|
|
|
going with #29593 |
4 participants
What does this PR do?
Summary of Changes:
DataTable.tsxby explicitly passing the correct element target event reference (onScroll({ target })) instead of evaluating a staletableContainerRef.currentat call time.Showing X of Y results) right above the table canvas using structural metadata tags.gap-4) so the search primitives and core grid bounds do not sit flush against each other.isPendingevaluates to true, alongside an explicit fallback "Load more" trigger button.isMountedclient-side conditional lifecycle wrapper andsuppressHydrationWarningflags insideinput.tsxto handle browser autofill/extension interference without throwing Next.js hydration boundaries.Visual Demo (For contributors especially)
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
/settings/admin/usersinside your browser environment.gap-4layout boundary between the top search input element and the data table.Checklist