Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto load more pages when running apply mode #291

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Auto load more pages when running apply mode #291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions apps/web/app/(app)/automation/ProcessRules.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export function ProcessRulesContent({ testMode }: { testMode: boolean }) {
parseAsBoolean.withDefault(false),
);

const { data, isLoading, isValidating, error, setSize } =
const { data, isLoading, isValidating, error, setSize, mutate } =
useSWRInfinite<MessagesResponse>((_index, previousPageData) => {
const pageToken = previousPageData?.nextPageToken;
if (previousPageData && !pageToken) return null;
Expand All @@ -60,9 +60,7 @@ export function ProcessRulesContent({ testMode }: { testMode: boolean }) {
return `/api/google/messages${paramsString ? `?${paramsString}` : ""}`;
});

const onLoadMore = () => {
setSize((size) => size + 1);
};
const onLoadMore = () => setSize((size) => size + 1);

const messages = useMemo(
() => data?.flatMap((page) => page.messages) || [],
Expand Down Expand Up @@ -113,10 +111,27 @@ export function ProcessRulesContent({ testMode }: { testMode: boolean }) {
const handleRunAll = async () => {
handleStart();

for (const message of messages) {
if (!isRunningAllRef.current) break;
if (results[message.id]) continue;
await onRun(message);
const PAGE_LIMIT = testMode ? 1 : 10;

for (let page = 0; page < PAGE_LIMIT; page++) {
// Get current data, only fetch if we don't have this page yet
let currentData = data;
if (!currentData?.[page]) {
await setSize((size) => size + 1);
currentData = await mutate();
}

const currentBatch = currentData?.[page]?.messages || [];

for (const message of currentBatch) {
if (!isRunningAllRef.current) break;
if (results[message.id]) continue;
await onRun(message);
}

// Check if we got new data in the last request
const lastPage = currentData?.[page];
if (!lastPage?.nextPageToken || !isRunningAllRef.current) break;
Comment on lines +116 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Add error handling for pagination and data fetching operations

The code review concerns are valid. While the code handles basic error cases through SWR's error states, the pagination and mutation operations in the handleRunAll function lack proper error handling:

  1. The sequential setSize and mutate calls are not wrapped in a try-catch block
  2. The code continues execution even if currentData is undefined after mutation
  3. There's no validation of the mutation response

Here are the specific locations that need attention:

  • apps/web/app/(app)/automation/ProcessRules.tsx lines 116-120:
let currentData = data;
if (!currentData?.[page]) {
  await setSize((size) => size + 1);
  currentData = await mutate();
}

Should include error handling:

try {
  let currentData = data;
  if (!currentData?.[page]) {
    await setSize((size) => size + 1);
    currentData = await mutate();
    if (!currentData?.[page]) {
      throw new Error("Failed to fetch page data");
    }
  }
} catch (error) {
  handleStop();
  toastError({
    title: "Error processing emails",
    description: error.message
  });
  break;
}
🔗 Analysis chain

Handle potential errors and concurrency issues when paginating

The loop for fetching additional pages based on PAGE_LIMIT works, but here are some considerations:

  1. Error handling: If mutate() fails or returns an error, the loop continues. You may want to break early or handle the error to prevent reading from undefined data.
  2. Concurrency: Double-check that isRunningAllRef and the updates from mutate() don’t cause unexpected race conditions if a user manually triggers other pagination or runs the rules from a different flow.
  3. Performance: Repeatedly calling await onRun(message) for each message could be time-consuming for large pages. Consider parallelizing if your backend can handle it.

Based on the search results, I can see that setSize and mutate are part of the SWR (React Hooks for Data Fetching) library's API. The code follows the recommended patterns for pagination and data mutation:

  1. setSize is used to load the next page of data
  2. mutate is used to revalidate/refresh the data after changes
  3. The code properly awaits both operations sequentially

Let's verify the error handling:

Let me fix the regex and try again:

Let me try a different approach to check the error handling:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for potential concurrency calls to `setSize` or `mutate` in the codebase outside this file.
# Rationale: We want to confirm that these calls aren't triggered simultaneously, which may cause race conditions.
rg -A 5 'setSize|mutate'

Length of output: 66159

Script:

#!/bin/bash
# Search for try-catch blocks around mutate calls in the file
rg -A 5 "try.*{" apps/web/app/\(app\)/automation/ProcessRules.tsx

Length of output: 143

Script:

#!/bin/bash
# Search for try-catch blocks around mutate calls in the file
rg -A 5 "try \{" "apps/web/app/(app)/automation/ProcessRules.tsx"

# Also search for error handling patterns
rg "if.*error|catch|throw" "apps/web/app/(app)/automation/ProcessRules.tsx"

Length of output: 145

Script:

#!/bin/bash
# Get the full content of the file to analyze error handling
cat "apps/web/app/(app)/automation/ProcessRules.tsx"

# Also check if there are any error handling patterns in similar pagination implementations
rg -A 5 "setSize.*size.*\+ 1.*mutate"

Length of output: 9566

}

handleStop();
Expand Down
Loading