feat: Implement Infinite Scroll while Maintaining Selection Features #39820
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableComponent
participant DataService
User->>TableComponent: Scroll near bottom
TableComponent->>TableComponent: handleScroll (debounced)
TableComponent->>DataService: fetchData(currentPage, pageSize)
DataService-->>TableComponent: Return new data
TableComponent->>TableComponent: renderRows() and update state
sequenceDiagram
participant User
participant TableComponent
User->>TableComponent: Click on a row (unique ID)
TableComponent->>TableComponent: handleRowSelection(uniqueID)
TableComponent->>TableComponent: Update selectedRows map
TableComponent->>TableComponent: Trigger selection change event
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
naaa760
changed the title
naaa760
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
index.html (2)
9-10: Optional improvement: add instructions or default text for test results.
Currently, the<div id="test-results">is empty. Consider adding a brief message or instructions for clarity.
11-15: Consider adding a fallback or error handling mechanism in the test bootstrap.
IfTableComponentTest.runTests()fails, the user might not see any clear indication. Wrapping this call in a try/catch could enhance visibility of errors.TableComponent.js (2)
44-51: Infinite scroll threshold is hard-coded.
The current 80% threshold is reasonable. However, consider making it configurable for different use cases.
103-114: Optional: enhance fetch error handling.
Currently, the method throws an error on a bad response. Consider retry logic or more user-friendly error messages to improve resilience.TableComponentTest.js (1)
70-88: Infinite scroll test is valid, but consider verifying appended data.
Current test checks for an increase in data length. Optionally verify the new rows for correctness.app/server/appsmith-server/src/main/resources/application-local.properties (1)
1-5: Google Sheets OAuth configuration looks good but needs documentationThe OAuth2 configuration for Google Sheets is properly structured with environment variables for sensitive information.
Consider adding:
- A comment explaining how to obtain and set the client ID and secret from Google Developer Console
- Documentation on how to set up the OAuth consent screen
- Verification that the redirect URI matches your OAuth configuration in Google Cloud Console
# Google Sheets specific configuration for localhost oauth2.google.sheets.client-id=${GOOGLE_SHEETS_CLIENT_ID:} oauth2.google.sheets.client-secret=${GOOGLE_SHEETS_CLIENT_SECRET:} oauth2.google.sheets.allowed-domains=localhost oauth2.redirect-uri=http://localhost/oauth2/callback +# Instructions to set up OAuth2: +# 1. Go to https://console.cloud.google.com/ +# 2. Create a project and enable Google Sheets API +# 3. Configure OAuth consent screen +# 4. Create OAuth credentials and set GOOGLE_SHEETS_CLIENT_ID and GOOGLE_SHEETS_CLIENT_SECRET environment variables
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
TableComponent.js(1 hunks)TableComponentTest.js(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java(2 hunks)app/server/appsmith-server/src/main/resources/application-local.properties(1 hunks)app/server/plugins/googlesheetsplugin/src/main/java/com/appsmith/external/plugins/GoogleSheetsPlugin.java(1 hunks)index.html(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
TableComponentTest.js
[error] 1-103: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 4-4: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 5-5: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 6-6: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 7-7: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 8-8: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (12)
index.html (2)
1-7: HTML structure and script inclusion look fine.
No immediate issues found in the document setup and script references.
16-17: No issues identified; markup closure looks good.TableComponent.js (5)
27-31: Returning the last selected row.
Implementation is concise and correctly uses Map values.
33-35: Fetching selected rows.
Returning all selected rows is straightforward and correct.
37-43: Check for null or missing row element.
Optional chaining helps, but consider early return if norowElementorrowIdis found to avoid searchingvisibleDataunnecessarily.
95-101: Index translation implementation is straightforward.
Calculation logic for absolute and visible indices is correct.
116-122: Loading state rendering is fine.
Implementation is succinct and aligns with typical UI placeholders.TableComponentTest.js (4)
21-36: Consider mocking or stubbing data fetch calls.
loadMoreDatamay attempt to fetch real endpoints. For a robust test, a mock or stub approach ensures consistent test behavior.
38-50: Multiple selection works as intended.
The logic checks out, and the test covers the basics effectively.
51-68: Triggered row identification method is tested thoroughly.
Verifies that the correct row is linked to the mock event data.
90-101: Index consistency test is correct.
Properly verifies getAbsoluteIndex and getVisibleIndex logic.app/server/appsmith-git/src/main/java/com/appsmith/git/configurations/GitServiceConfig.java (1)
18-20: Configuration property reference looks goodThe
allowedDomainsproperty is correctly defined with the appropriate Value annotation.
| async loadMoreData() { | ||
| if (this.isLoading || this.visibleData.length >= this.totalRows) { | ||
| return; | ||
| } | ||
|
|
||
| this.isLoading = true; | ||
|
|
||
| try { | ||
| const newData = await this.fetchData(this.currentPage + 1); | ||
| this.currentPage += 1; | ||
|
|
||
| // Append new data while maintaining selection states | ||
| this.visibleData = [...this.visibleData, ...newData]; | ||
|
|
||
| // Update row rendering while preserving selections | ||
| this.renderRows(); | ||
| } finally { | ||
| this.isLoading = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Concurrent or repeated scroll events.
Your isLoading flag helps prevent simultaneous fetches, but quickly repeated scroll attempts might still queue up. Consider a more robust guard or cancellation approach if the data fetch is slow.
| handleRowSelection(rowData, isSelected) { | ||
| // Use a unique identifier from your data instead of index | ||
| const rowId = rowData.id || generateUniqueId(rowData); | ||
|
|
||
| if (isSelected) { | ||
| this.selectedRows.set(rowId, rowData); | ||
| } else { | ||
| this.selectedRows.delete(rowId); | ||
| } | ||
|
|
||
| // Trigger selection change event | ||
| this.onSelectionChange(Array.from(this.selectedRows.values())); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify usage of generateUniqueId(rowData) and onSelectionChange.
Ensure these functions or handlers are defined or imported. Otherwise, referencing them will cause errors at runtime.
🏁 Script executed:
#!/bin/bash
# Description: Search the codebase for definitions of generateUniqueId and onSelectionChange
rg -A 5 $'generateUniqueId|onSelectionChange'Length of output: 51100
Clarify and Fix Function Definitions in TableComponent.js
- It appears that the client-side code in TableComponent.js is referencing
generateUniqueId(rowData)without a clear local definition or import. The search only revealed definitions in server-side files, so please verify if a client-side implementation (or import from a utility module) should be used instead. - Ensure that
onSelectionChangeis correctly defined or passed as a prop. Given its extensive use across the codebase, double-check that its usage in TableComponent.js aligns with the intended event handling.
| class TableComponent { | ||
| constructor() { | ||
| this.selectedRows = new Map(); // Store selections by unique row ID instead of index | ||
| this.visibleData = []; // Currently visible/loaded rows | ||
| this.totalRows = 0; // Total number of rows available | ||
| this.pageSize = 50; // Number of rows to load per page | ||
| this.currentPage = 1; | ||
| this.isLoading = false; | ||
| this.handleScroll = debounce(this.handleScroll.bind(this), 150); | ||
| this.container.addEventListener("scroll", this.handleScroll); | ||
| } |
There was a problem hiding this comment.
Flag undefined property this.container.
this.container is referenced in the constructor but never declared or assigned. This might cause a runtime error.
constructor() {
this.selectedRows = new Map();
this.visibleData = [];
...
+ this.container = document.querySelector('#tableContainer'); // or accept container as a constructor parameter
this.handleScroll = debounce(this.handleScroll.bind(this), 150);
this.container.addEventListener("scroll", this.handleScroll);
}📝 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.
| class TableComponent { | |
| constructor() { | |
| this.selectedRows = new Map(); // Store selections by unique row ID instead of index | |
| this.visibleData = []; // Currently visible/loaded rows | |
| this.totalRows = 0; // Total number of rows available | |
| this.pageSize = 50; // Number of rows to load per page | |
| this.currentPage = 1; | |
| this.isLoading = false; | |
| this.handleScroll = debounce(this.handleScroll.bind(this), 150); | |
| this.container.addEventListener("scroll", this.handleScroll); | |
| } | |
| class TableComponent { | |
| constructor() { | |
| this.selectedRows = new Map(); // Store selections by unique row ID instead of index | |
| this.visibleData = []; // Currently visible/loaded rows | |
| this.totalRows = 0; // Total number of rows available | |
| this.pageSize = 50; // Number of rows to load per page | |
| this.currentPage = 1; | |
| this.isLoading = false; | |
| this.container = document.querySelector('#tableContainer'); // or accept container as a constructor parameter | |
| this.handleScroll = debounce(this.handleScroll.bind(this), 150); | |
| this.container.addEventListener("scroll", this.handleScroll); | |
| } | |
| } |
| renderRows() { | ||
| return this.visibleData | ||
| .map((rowData) => { | ||
| const rowId = rowData.id || generateUniqueId(rowData); | ||
| const isSelected = this.selectedRows.has(rowId); | ||
|
|
||
| return ` | ||
| <tr | ||
| data-row-id="${rowId}" | ||
| class="${isSelected ? "selected" : ""}" | ||
| onclick="handleRowSelection(${JSON.stringify( | ||
| rowData | ||
| )}, !${isSelected})" | ||
| > | ||
| ${this.renderCells(rowData)} | ||
| </tr> | ||
| `; | ||
| }) | ||
| .join(""); | ||
| } |
There was a problem hiding this comment.
Potential XSS risk with inline JSON.stringify usage.
If rowData includes user-supplied content, inserting it directly into HTML could be risky. Sanitize or escape special characters before stringifying.
<tr
data-row-id="${rowId}"
- onclick="handleRowSelection(${JSON.stringify(rowData)}, ...)
+ onclick="handleRowSelection(${escapeHTML(JSON.stringify(rowData))}, ...)
...
>📝 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.
| renderRows() { | |
| return this.visibleData | |
| .map((rowData) => { | |
| const rowId = rowData.id || generateUniqueId(rowData); | |
| const isSelected = this.selectedRows.has(rowId); | |
| return ` | |
| <tr | |
| data-row-id="${rowId}" | |
| class="${isSelected ? "selected" : ""}" | |
| onclick="handleRowSelection(${JSON.stringify( | |
| rowData | |
| )}, !${isSelected})" | |
| > | |
| ${this.renderCells(rowData)} | |
| </tr> | |
| `; | |
| }) | |
| .join(""); | |
| } | |
| renderRows() { | |
| return this.visibleData | |
| .map((rowData) => { | |
| const rowId = rowData.id || generateUniqueId(rowData); | |
| const isSelected = this.selectedRows.has(rowId); | |
| return ` | |
| <tr | |
| data-row-id="${rowId}" | |
| class="${isSelected ? "selected" : ""}" | |
| onclick="handleRowSelection(${escapeHTML(JSON.stringify(rowData))}, !${isSelected})" | |
| > | |
| ${this.renderCells(rowData)} | |
| </tr> | |
| `; | |
| }) | |
| .join(""); | |
| } |
| class TableComponentTest { | ||
| static async runTests() { | ||
| const tests = [ | ||
| this.testSelectionPersistence, | ||
| this.testMultipleSelections, | ||
| this.testTriggeredRow, | ||
| this.testInfiniteScroll, | ||
| this.testIndexConsistency, | ||
| ]; | ||
|
|
||
| for (const test of tests) { | ||
| try { | ||
| await test(); | ||
| console.log(`✅ ${test.name} passed`); | ||
| } catch (error) { | ||
| console.error(`❌ ${test.name} failed:`, error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace static-only class with plain functions or refer to the class explicitly.
Having only static methods can increase complexity. Additionally, referencing this in a static context can be confusing. Use the class name directly or switch to standalone functions.
static async runTests() {
- const tests = [
- this.testSelectionPersistence,
- this.testMultipleSelections,
- ...
- ];
+ const tests = [
+ TableComponentTest.testSelectionPersistence,
+ TableComponentTest.testMultipleSelections,
+ ...
+ ];
for (const test of tests) {
...
}
}📝 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.
| class TableComponentTest { | |
| static async runTests() { | |
| const tests = [ | |
| this.testSelectionPersistence, | |
| this.testMultipleSelections, | |
| this.testTriggeredRow, | |
| this.testInfiniteScroll, | |
| this.testIndexConsistency, | |
| ]; | |
| for (const test of tests) { | |
| try { | |
| await test(); | |
| console.log(`✅ ${test.name} passed`); | |
| } catch (error) { | |
| console.error(`❌ ${test.name} failed:`, error); | |
| } | |
| } | |
| } | |
| class TableComponentTest { | |
| static async runTests() { | |
| const tests = [ | |
| TableComponentTest.testSelectionPersistence, | |
| TableComponentTest.testMultipleSelections, | |
| TableComponentTest.testTriggeredRow, | |
| TableComponentTest.testInfiniteScroll, | |
| TableComponentTest.testIndexConsistency, | |
| ]; | |
| for (const test of tests) { | |
| try { | |
| await test(); | |
| console.log(`✅ ${test.name} passed`); | |
| } catch (error) { | |
| console.error(`❌ ${test.name} failed:`, error); | |
| } | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-103: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 4-4: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 5-5: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 6-6: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 7-7: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 8-8: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Empty file - implementation missing
This file is empty and does not contain any implementation. For a plugin to function properly, it should contain a Java class that implements the necessary interfaces for the Google Sheets integration.
At minimum, the file should include:
package com.appsmith.external.plugins;
import org.springframework.stereotype.Component;
import com.appsmith.external.models.*;
import reactor.core.publisher.Mono;
@Component
public class GoogleSheetsPlugin extends BasePlugin {
// Implement the required methods for the plugin
}| @PostConstruct | ||
| public void init() { | ||
| // Add localhost to allowed domains if not present | ||
| if (!allowedDomains.contains("localhost")) { | ||
| allowedDomains = allowedDomains + ",localhost"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for allowedDomains property
The current implementation doesn't handle the case where allowedDomains could be null or empty. This could lead to a NullPointerException.
@PostConstruct
public void init() {
// Add localhost to allowed domains if not present
- if (!allowedDomains.contains("localhost")) {
+ if (allowedDomains == null || allowedDomains.isEmpty()) {
+ allowedDomains = "localhost";
+ } else if (!allowedDomains.contains("localhost")) {
allowedDomains = allowedDomains + ",localhost";
}
}📝 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.
| @PostConstruct | |
| public void init() { | |
| // Add localhost to allowed domains if not present | |
| if (!allowedDomains.contains("localhost")) { | |
| allowedDomains = allowedDomains + ",localhost"; | |
| } | |
| } | |
| @PostConstruct | |
| public void init() { | |
| // Add localhost to allowed domains if not present | |
| if (allowedDomains == null || allowedDomains.isEmpty()) { | |
| allowedDomains = "localhost"; | |
| } else if (!allowedDomains.contains("localhost")) { | |
| allowedDomains = allowedDomains + ",localhost"; | |
| } | |
| } |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description:
Integration of infinite scroll with existing table selection features to maintain proper row selection and indices during scrolling.
Tip
TL;DR: Added infinite scroll support to table component while preserving row selection state and index accuracy across page loads.
Fixes #39814
This PR implements:
Key features:
Dependencies:
Communication
Should the DevRel and Marketing teams inform users about this change?
[x] Yes
[ ] No
This feature significantly improves table usability with large datasets while maintaining existing selection functionality, which would be valuable information for users.
Summary by CodeRabbit