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.

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

Release/v0.3.3 #47

Merged
merged 2 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions backend/app/api/v1/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,24 @@ def chat(project_id: int, chat_request: ChatRequest, db: Session = Depends(get_d

ordered_file_names = [doc_id_to_filename[doc_id] for doc_id in doc_ids]

extract_vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}",
similarity_threshold=settings.chat_extraction_doc_threshold)
Comment on lines +68 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify persist_directory when initializing ChromaDB

When creating the extract_vectorstore, you should specify the persist_directory to ensure that the vector store points to the correct data location. Without it, ChromaDB might not load the expected data, leading to inconsistencies.

Apply this diff to include the persist_directory:

 extract_vectorstore = ChromaDB(
     f"panda-etl-extraction-{project_id}",
+    persist_directory=settings.persist_directory,
     similarity_threshold=settings.chat_extraction_doc_threshold
 )
📝 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
extract_vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}",
similarity_threshold=settings.chat_extraction_doc_threshold)
extract_vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}",
persist_directory=settings.persist_directory,
similarity_threshold=settings.chat_extraction_doc_threshold)


# Extract reference documents from the extraction results from db
extraction_docs = extract_vectorstore.get_relevant_docs(
chat_request.query,
k=settings.chat_extraction_max_docs
)

# Append text from single documents together
for extraction_doc in extraction_docs["metadatas"][0]:
index = next((i for i, item in enumerate(ordered_file_names) if item == extraction_doc["filename"]), None)
if index is None:
ordered_file_names.append(extraction_doc["filename"])
docs.append(extraction_doc["reference"])
else:
docs[index] = f'{extraction_doc["reference"]}\n\n{docs[index]}'
Comment on lines +79 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize filename lookup by using a dictionary

The current method of finding the index using a list comprehension has a time complexity of O(n). For better performance, consider using a dictionary to map filenames to their indices.

Refactor the code as follows:

+# Create a mapping from filename to index for quick lookup
+filename_to_index = {filename: idx for idx, filename in enumerate(ordered_file_names)}

 for extraction_doc in extraction_docs["metadatas"]:
-    index = next((i for i, item in enumerate(ordered_file_names) if item == extraction_doc["filename"]), None)
+    index = filename_to_index.get(extraction_doc["filename"])

     if index is None:
         ordered_file_names.append(extraction_doc["filename"])
         docs.append(extraction_doc["reference"])
+        # Update the mapping with the new filename and index
+        filename_to_index[extraction_doc["filename"]] = len(ordered_file_names) - 1
     else:
         docs[index] = f'{extraction_doc["reference"]}\n\n{docs[index]}'

Committable suggestion was skipped due to low confidence.


Comment on lines +68 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for potential exceptions

The new code block does not account for possible exceptions such as KeyError or AttributeError that may arise from accessing dictionary keys or list indices. To ensure robustness, include error handling around these operations.

Wrap the code in a try-except block or validate the data before accessing it. For example:

for extraction_doc in extraction_docs.get("metadatas", []):
    if "filename" in extraction_doc and "reference" in extraction_doc:
        # Proceed with processing
    else:
        # Handle missing keys appropriately

docs_formatted = [
{"filename": filename, "quote": quote}
for filename, quote in zip(ordered_file_names, docs)
Expand Down
4 changes: 4 additions & 0 deletions backend/app/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class Settings(BaseSettings):
openai_api_key: str = ""
openai_embedding_model: str = "text-embedding-ada-002"

# Extraction References for chat
chat_extraction_doc_threshold: float = 0.5
chat_extraction_max_docs: int = 50
Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding value validation for the new parameters.

While the default values are reasonable, it would be beneficial to add validation to ensure:

  • chat_extraction_doc_threshold stays between 0 and 1
  • chat_extraction_max_docs remains positive

You can use pydantic's validators to enforce these constraints:

from pydantic import validator

class Settings(BaseSettings):
    # ... existing code ...

    @validator("chat_extraction_doc_threshold")
    def validate_threshold(cls, v):
        if not 0 <= v <= 1:
            raise ValueError("chat_extraction_doc_threshold must be between 0 and 1")
        return v

    @validator("chat_extraction_max_docs")
    def validate_max_docs(cls, v):
        if v <= 0:
            raise ValueError("chat_extraction_max_docs must be positive")
        return v


class Config:
env_file = ".env"

Expand Down
48 changes: 47 additions & 1 deletion backend/app/processing/process_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ def process_step_task(
# Initial DB operations (open and fetch relevant data)
with SessionLocal() as db:
process = process_repository.get_process(db, process_id)
project_id = process.project_id
process_step = process_repository.get_process_step(db, process_step_id)

filename = process_step.asset.filename
if process.status == ProcessStatus.STOPPED:
return False # Stop processing if the process is stopped

Expand Down Expand Up @@ -84,6 +85,15 @@ def process_step_task(
output_references=data["context"],
)

# vectorize extraction result
try:
vectorize_extraction_process_step(project_id=project_id,
process_step_id=process_step_id,
filename=filename,
references=data["context"])
except Exception :
logger.error(f"Failed to vectorize extraction results for chat {traceback.format_exc()}")

Comment on lines +88 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid catching broad exceptions

Catching all exceptions with except Exception can make debugging difficult and may mask unexpected errors. It's better to catch specific exceptions or re-raise the original exception after logging.

Consider modifying the exception handling as follows:

 try:
     vectorize_extraction_process_step(project_id=project_id,
                                     process_step_id=process_step_id,
                                     filename=filename,
                                     references=data["context"])
-except Exception:
+except SomeSpecificException as e:
     logger.error(f"Failed to vectorize extraction results: {traceback.format_exc()}")
+    raise e

Replace SomeSpecificException with the specific exception(s) that vectorize_extraction_process_step might raise. If you need to handle multiple exceptions, you can list them in a tuple.

Committable suggestion was skipped due to low confidence.

success = True

except CreditLimitExceededException:
Expand Down Expand Up @@ -361,3 +371,39 @@ def update_process_step_status(
process_repository.update_process_step_status(
db, process_step, status, output=output, output_references=output_references
)

def vectorize_extraction_process_step(project_id: int, process_step_id: int, filename: str, references: dict) -> None:
# Vectorize extraction result and dump in database
field_references = {}

# Loop to concatenate sources for each reference
for extraction_references in references:
for extraction_reference in extraction_references:
sources = extraction_reference.get("sources", [])
if sources:
sources_catenated = "\n".join(sources)
field_references.setdefault(extraction_reference["name"], "")
field_references[extraction_reference["name"]] += (
"\n" + sources_catenated if field_references[extraction_reference["name"]] else sources_catenated
)

# Only proceed if there are references to add
if not field_references:
return

# Initialize Vectorstore
vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}")

docs = [f"{filename} {key}" for key in field_references]
metadatas = [
{
"project_id": project_id,
"process_step_id": process_step_id,
"filename": filename,
"reference": reference
}
for reference in field_references.values()
]

# Add documents to vectorstore
vectorstore.add_docs(docs=docs, metadatas=metadatas)
Comment on lines +375 to +409
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect assignment of documents and metadata in vectorization

Currently, the docs variable contains filename and key, which are not the actual content to be vectorized. The metadatas assign the concatenated sources to the reference field, which may not be intended. This could lead to incorrect data being stored in the vector store.

Apply this diff to correct the assignments:

 # Build docs and metadatas
-def vectorize_extraction_process_step(project_id: int, process_step_id: int, filename: str, references: dict) -> None:
-    # ... [existing code] ...
-    # Initialize Vectorstore
-    vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}")
-
-    docs = [f"{filename} {key}" for key in field_references]
-    metadatas = [
-        {
-            "project_id": project_id,
-            "process_step_id": process_step_id,
-            "filename": filename,
-            "reference": reference
-        }
-        for reference in field_references.values()
-    ]
+    # Initialize Vectorstore
+    vectorstore = ChromaDB(f"panda-etl-extraction-{project_id}")
+
+    docs = list(field_references.values())
+    metadatas = [
+        {
+            "project_id": project_id,
+            "process_step_id": process_step_id,
+            "filename": filename,
+            "reference_name": key
+        }
+        for key in field_references.keys()
+    ]

Explanation:

  • Docs Correction: docs should contain the concatenated sources (field_references.values()), which are the actual content to be vectorized.
  • Metadata Correction: In metadatas, use key (the reference name) for the reference_name field to accurately describe the metadata associated with each document.

Ensure that docs and metadatas are correctly aligned so that each document corresponds to the correct metadata entry.

Committable suggestion was skipped due to low confidence.

107 changes: 106 additions & 1 deletion backend/tests/processing/test_process_queue.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
from app.requests.schemas import ExtractFieldsResponse
import pytest
from unittest.mock import Mock, patch
from unittest.mock import MagicMock, Mock, patch
from app.processing.process_queue import (
handle_exceptions,
extract_process,
update_process_step_status,
find_best_match_for_short_reference,
vectorize_extraction_process_step,
)
from app.exceptions import CreditLimitExceededException
from app.models import ProcessStepStatus
Expand Down Expand Up @@ -180,3 +181,107 @@ def test_chroma_db_initialization(mock_extract_data, mock_chroma):

mock_chroma.assert_called_with(f"panda-etl-{process.project_id}", similarity_threshold=3)
assert mock_chroma.call_count >= 1

@patch('app.processing.process_queue.ChromaDB')
def test_vectorize_extraction_process_step_single_reference(mock_chroma_db):
# Mock ChromaDB instance
mock_vectorstore = MagicMock()
mock_chroma_db.return_value = mock_vectorstore

# Inputs
project_id = 123
process_step_id = 1
filename = "sample_file"
references = [
[
{"name": "field1", "sources": ["source1", "source2"]}
]
]

# Call function
vectorize_extraction_process_step(project_id, process_step_id, filename, references)

# Expected docs and metadata to add to ChromaDB
expected_docs = ["sample_file field1"]
expected_metadatas = [
{
"project_id": project_id,
"process_step_id": process_step_id,
"filename": filename,
"reference": "source1\nsource2"
}
]

# Assertions
mock_vectorstore.add_docs.assert_called_once_with(
docs=expected_docs,
metadatas=expected_metadatas
)

@patch('app.processing.process_queue.ChromaDB')
def test_vectorize_extraction_process_step_multiple_references_concatenation(mock_chroma_db):
# Mock ChromaDB instance
mock_vectorstore = MagicMock()
mock_chroma_db.return_value = mock_vectorstore

# Inputs
project_id = 456
process_step_id = 2
filename = "test_file"
references = [
[
{"name": "field1", "sources": ["source1", "source2"]},
{"name": "field1", "sources": ["source3"]}
],
[
{"name": "field2", "sources": ["source4"]}
]
]

# Call function
vectorize_extraction_process_step(project_id, process_step_id, filename, references)

# Expected docs and metadata to add to ChromaDB
expected_docs = ["test_file field1", "test_file field2"]
expected_metadatas = [
{
"project_id": project_id,
"process_step_id": process_step_id,
"filename": filename,
"reference": "source1\nsource2\nsource3"
},
{
"project_id": project_id,
"process_step_id": process_step_id,
"filename": filename,
"reference": "source4"
}
]

# Assertions
mock_vectorstore.add_docs.assert_called_once_with(
docs=expected_docs,
metadatas=expected_metadatas
)

Comment on lines +221 to +266
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring test setup code.

The test effectively validates multiple references, but consider extracting common setup code into fixtures to reduce duplication:

@pytest.fixture
def mock_vectorstore():
    with patch('app.processing.process_queue.ChromaDB') as mock_chroma_db:
        mock_store = MagicMock()
        mock_chroma_db.return_value = mock_store
        yield mock_store

def test_vectorize_extraction_process_step_multiple_references_concatenation(mock_vectorstore):
    # Test-specific setup and assertions
    ...

@patch('app.processing.process_queue.ChromaDB') # Replace with the correct module path
def test_vectorize_extraction_process_step_empty_sources(mock_chroma_db):
# Mock ChromaDB instance
mock_vectorstore = MagicMock()
mock_chroma_db.return_value = mock_vectorstore

# Inputs
project_id = 789
process_step_id = 3
filename = "empty_sources_file"
references = [
[
{"name": "field1", "sources": []}
]
]

# Call function
vectorize_extraction_process_step(project_id, process_step_id, filename, references)

# Expected no calls to add_docs due to empty sources
mock_vectorstore.add_docs.assert_not_called()
Comment on lines +267 to +287
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update comment and enhance empty sources test.

  1. The comment # Replace with the correct module path should be removed as it's no longer needed.
  2. Consider adding assertions to verify that the function completes successfully even with empty sources, rather than just checking that add_docs isn't called.

Example enhancement:

def test_vectorize_extraction_process_step_empty_sources(mock_vectorstore):
    # Existing test code...
    
    # Additional assertions
    assert mock_vectorstore.method_calls == []  # Verify no other methods were called
    # Verify function completes without raising exceptions

3 changes: 2 additions & 1 deletion frontend/.env.example
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
NEXT_PUBLIC_API_URL=http://localhost:3000/api/v1
NEXT_PUBLIC_STORAGE_URL=http://localhost:3000/api/assets
NEXT_PUBLIC_MIXPANEL_TOKEN=f2e8a71ab2bde33ebf346c5abf6ba9fa
NEXT_PUBLIC_MIXPANEL_TOKEN=f2e8a71ab2bde33ebf346c5abf6ba9fa
NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN=0df0bee895044430880278e2b2a5b2d2
2 changes: 2 additions & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"@radix-ui/react-context-menu": "^2.2.1",
"@radix-ui/react-radio-group": "^1.2.0",
"@react-pdf/renderer": "^4.0.0",
"@rollbar/react": "^0.12.0-beta",
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

Consider downgrading to the stable version 0.11.2 of @rollbar/react

The latest stable version (0.11.2) would be more reliable for production use than the current beta version (0.12.0-beta). Beta versions may introduce breaking changes or contain unstable features.

🔗 Analysis chain

Consider using a stable version of @rollbar/react.

The beta version might have stability issues. Consider using the latest stable version if available, or document the reason for using the beta version.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there's a stable version available
npm view @rollbar/react versions --json | jq 'map(select(test("beta|alpha|rc") | not))[-1]'

Length of output: 102

"@tanstack/react-query": "^5.51.1",
"@tippyjs/react": "^4.2.6",
"@types/mixpanel-browser": "^2.50.1",
Expand All @@ -39,6 +40,7 @@
"react-quill": "^2.0.0",
"rehype-sanitize": "^6.0.0",
"remark-gfm": "^4.0.0",
"rollbar": "^2.26.4",
"tailwind-merge": "^2.4.0"
},
"devDependencies": {
Expand Down
10 changes: 7 additions & 3 deletions frontend/src/app/(app)/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { ScrollProvider } from "@/context/ScrollContext";
import Sidebar from "@/components/ui/Sidebar";
import Navbar from "@/components/ui/Navbar";
import "@/app/style/globals.css";
import { ErrorBoundary } from "@rollbar/react";
import { ViewErrorFallback } from "@/components/ErrorFallback";

export default function RootLayout({
children,
Expand All @@ -17,9 +19,11 @@ export default function RootLayout({
<Sidebar />
<div className="flex-1 flex flex-col overflow-hidden md:ml-64">
<Navbar />
<main className="flex-1 overflow-x-hidden overflow-y-auto bg-gray-50 px-10 py-6">
{children}
</main>
<ErrorBoundary fallbackUI={ViewErrorFallback}>
<main className="flex-1 overflow-x-hidden overflow-y-auto bg-gray-50 px-10 py-6">
{children}
</main>
</ErrorBoundary>
</div>
</div>
</ScrollProvider>
Expand Down
29 changes: 20 additions & 9 deletions frontend/src/app/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@ import React from "react";
import { ReactQueryClientProvider } from "@/components/ReactQueryClientProvider";
import { Toaster } from "react-hot-toast";
import "@/app/style/globals.css";
import { Provider, ErrorBoundary } from "@rollbar/react";
import { GlobalErrorFallback } from "@/components/ErrorFallback";

const inter = Inter({ subsets: ["latin"] });

const rollbarConfig = {
accessToken: process.env.NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN,
environment: process.env.NODE_ENV ?? "production",
};
Comment on lines +12 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety and review environment fallback

Two suggestions for improvement:

  1. Consider adding TypeScript interface for the Rollbar configuration.
  2. The default fallback to "production" environment could be risky. Consider using a more restrictive fallback or throwing an error if NODE_ENV is not set.
+interface RollbarConfig {
+  accessToken: string | undefined;
+  environment: string;
+}
+
-const rollbarConfig = {
+const rollbarConfig: RollbarConfig = {
   accessToken: process.env.NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN,
-  environment: process.env.NODE_ENV ?? "production",
+  environment: process.env.NODE_ENV ?? "development",
};
📝 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
const rollbarConfig = {
accessToken: process.env.NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN,
environment: process.env.NODE_ENV ?? "production",
};
interface RollbarConfig {
accessToken: string | undefined;
environment: string;
}
const rollbarConfig: RollbarConfig = {
accessToken: process.env.NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN,
environment: process.env.NODE_ENV ?? "development",
};


export const metadata: Metadata = {
title: "PandaETL",
description:
Expand All @@ -22,14 +29,18 @@ export default function RootLayout({
children: React.ReactNode;
}>) {
return (
<ReactQueryClientProvider>
<html lang="en">
<head />
<body className={inter.className}>
<Toaster position="top-right" />
{children}
</body>
</html>
</ReactQueryClientProvider>
<html lang="en">
<head />
<body className={inter.className}>
<Provider config={rollbarConfig}>
<ErrorBoundary fallbackUI={GlobalErrorFallback}>
<ReactQueryClientProvider>
<Toaster position="top-right" />
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider specific error handling for React Query errors

While the ErrorBoundary will catch React Query errors, consider adding a specific error handler for query errors using React Query's onError global configuration.

// In ReactQueryClientProvider.tsx
const queryClient = new QueryClient({
  defaultOptions: {
    queries: {
      onError: (error) => {
        // Log to Rollbar with specific context
        rollbar.error('Query Error', error, {
          context: 'React Query'
        });
      }
    }
  }
});

{children}
</ReactQueryClientProvider>
</ErrorBoundary>
</Provider>
Comment on lines +35 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider additional Rollbar configuration options

The basic Rollbar setup is good, but consider adding these recommended configuration options for better error tracking:

  • captureUncaught: to capture unhandled exceptions
  • captureUnhandledRejections: for promise rejections
  • payload: to include version and environment data
const rollbarConfig: RollbarConfig = {
  accessToken: process.env.NEXT_PUBLIC_ROLLBAR_ACCESS_TOKEN,
  environment: process.env.NODE_ENV ?? "development",
+  captureUncaught: true,
+  captureUnhandledRejections: true,
+  payload: {
+    client: {
+      javascript: {
+        code_version: process.env.NEXT_PUBLIC_APP_VERSION,
+        source_map_enabled: true,
+      }
+    }
+  }
};

Committable suggestion was skipped due to low confidence.

</body>
</html>
);
}
49 changes: 49 additions & 0 deletions frontend/src/components/ErrorFallback.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
"use client";
import React, { useEffect, useState } from "react";
import { Button } from "./ui/Button";
import { usePathname } from "next/navigation";

interface ErrorFallbackProps {
error: Error | null;
resetError: () => void;
}

interface ErrorFallbackBaseProps extends ErrorFallbackProps {
textColor: string;
}

// Fallback UI component displayed on error
const ErrorFallbackBase = ({
error,
resetError,
textColor,
}: ErrorFallbackBaseProps) => {
const pathname = usePathname();
const [initialPathname, setInitialPathname] = useState<string | null>(null);

useEffect(() => {
if (initialPathname === null) {
setInitialPathname(pathname);
} else if (pathname !== initialPathname && error) {
resetError();
}
}, [pathname, initialPathname, error, resetError]);
Comment on lines +21 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify pathname tracking logic to prevent potential race conditions.

The current implementation using initialPathname state could be simplified to avoid potential race conditions during rapid navigation changes.

Consider this alternative implementation:

-  const [initialPathname, setInitialPathname] = useState<string | null>(null);
+  const [previousPathname, setPreviousPathname] = useState(pathname);

   useEffect(() => {
-    if (initialPathname === null) {
-      setInitialPathname(pathname);
-    } else if (pathname !== initialPathname && error) {
+    if (pathname !== previousPathname && error) {
       resetError();
+      setPreviousPathname(pathname);
     }
-  }, [pathname, initialPathname, error, resetError]);
+  }, [pathname, previousPathname, error, resetError]);
📝 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
const pathname = usePathname();
const [initialPathname, setInitialPathname] = useState<string | null>(null);
useEffect(() => {
if (initialPathname === null) {
setInitialPathname(pathname);
} else if (pathname !== initialPathname && error) {
resetError();
}
}, [pathname, initialPathname, error, resetError]);
const pathname = usePathname();
const [previousPathname, setPreviousPathname] = useState(pathname);
useEffect(() => {
if (pathname !== previousPathname && error) {
resetError();
setPreviousPathname(pathname);
}
}, [pathname, previousPathname, error, resetError]);


return (
<div className={`p-8 text-center ${textColor}`}>
<h1>Oops! Something went wrong.</h1>
<p>{error?.message || "We're working on fixing this issue."}</p>
<Button onClick={resetError} className="mt-4">
Try Again
</Button>
</div>
);
};
Comment on lines +15 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance accessibility and error handling.

Consider the following improvements:

  1. Add ARIA attributes for better screen reader support
  2. Include more detailed error information for debugging

Apply these changes to improve accessibility and error details:

 return (
-    <div className={`p-8 text-center ${textColor}`}>
+    <div 
+      role="alert"
+      aria-live="assertive"
+      className={`p-8 text-center ${textColor}`}
+    >
       <h1>Oops! Something went wrong.</h1>
-      <p>{error?.message || "We're working on fixing this issue."}</p>
+      <p>{error?.message || "We're working on fixing this issue."}</p>
+      {process.env.NODE_ENV === 'development' && error?.stack && (
+        <pre className="mt-2 text-sm text-left overflow-auto">
+          {error.stack}
+        </pre>
+      )}
       <Button onClick={resetError} className="mt-4">
         Try Again
       </Button>
📝 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
// Fallback UI component displayed on error
const ErrorFallbackBase = ({
error,
resetError,
textColor,
}: ErrorFallbackBaseProps) => {
const pathname = usePathname();
const [initialPathname, setInitialPathname] = useState<string | null>(null);
useEffect(() => {
if (initialPathname === null) {
setInitialPathname(pathname);
} else if (pathname !== initialPathname && error) {
resetError();
}
}, [pathname, initialPathname, error, resetError]);
return (
<div className={`p-8 text-center ${textColor}`}>
<h1>Oops! Something went wrong.</h1>
<p>{error?.message || "We're working on fixing this issue."}</p>
<Button onClick={resetError} className="mt-4">
Try Again
</Button>
</div>
);
};
// Fallback UI component displayed on error
const ErrorFallbackBase = ({
error,
resetError,
textColor,
}: ErrorFallbackBaseProps) => {
const pathname = usePathname();
const [initialPathname, setInitialPathname] = useState<string | null>(null);
useEffect(() => {
if (initialPathname === null) {
setInitialPathname(pathname);
} else if (pathname !== initialPathname && error) {
resetError();
}
}, [pathname, initialPathname, error, resetError]);
return (
<div
role="alert"
aria-live="assertive"
className={`p-8 text-center ${textColor}`}
>
<h1>Oops! Something went wrong.</h1>
<p>{error?.message || "We're working on fixing this issue."}</p>
{process.env.NODE_ENV === 'development' && error?.stack && (
<pre className="mt-2 text-sm text-left overflow-auto">
{error.stack}
</pre>
)}
<Button onClick={resetError} className="mt-4">
Try Again
</Button>
</div>
);
};


export const GlobalErrorFallback = (props: ErrorFallbackProps) => (
<ErrorFallbackBase {...props} textColor="text-white" />
);

export const ViewErrorFallback = (props: ErrorFallbackProps) => (
<ErrorFallbackBase {...props} textColor="text-black" />
);
Loading
Loading