fix: Application freezing but when moving dialog to bin #1352
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx (3)
Line range hint
10-33: Good use of memo, but further optimizations possible.The memo wrapper is a good start to prevent unnecessary re-renders. However, to maximize its effectiveness, consider these additional optimizations:
- Memoize the
getContentfunction to prevent recreation on each render- Add error boundaries for safer rendering of external content
Here's how to implement these improvements:
import { Html, Markdown } from 'embeddable-markdown-html'; import styles from '../InboxItem/inboxItemDetail.module.css'; -import { memo } from 'react'; +import { memo, useCallback } from 'react'; +import { ErrorBoundary } from 'react-error-boundary'; interface AdditionalInfoContentProps { mediaType: string | undefined; value: string | undefined; } +const ContentErrorFallback = () => ( + <div>Failed to render content. Please try again later.</div> +); + export const AdditionalInfoContent = memo(({ mediaType, value }: AdditionalInfoContentProps) => { if (!value) { return null; } - const getContent = (mediaType: string) => { + const getContent = useCallback((mediaType: string) => { switch (mediaType) { case 'text/html': - return <Html>{value}</Html>; + return ( + <ErrorBoundary FallbackComponent={ContentErrorFallback}> + <Html>{value}</Html> + </ErrorBoundary> + ); case 'text/markdown': - return <Markdown>{value}</Markdown>; + return ( + <ErrorBoundary FallbackComponent={ContentErrorFallback}> + <Markdown>{value}</Markdown> + </ErrorBoundary> + ); case 'text/plain': return value; default: return value; } - }; + }, [value]); return ( <section className={styles.additionalInfo} data-id="dialog-additional-info"> {getContent(mediaType ?? 'text/plain')} </section> ); });
10-10: Consider adding a display name for debugging.To improve debugging experience in React DevTools, add a display name to the memoized component.
export const AdditionalInfoContent = memo(({ mediaType, value }: AdditionalInfoContentProps) => { +AdditionalInfoContent.displayName = 'AdditionalInfoContent';
Based on the analysis of the code, I can now provide a final response regarding the freezing issue and the memo optimization.
The memo optimization alone may not fully resolve the freezing issue during dialog movement.
Here's why:
The dialog movement operation involves multiple async operations and state updates:
- API call to update system label
- Multiple query invalidations
- State updates for loading indicators
- Snackbar notifications
The
InboxItemDetailcomponent, which is the main consumer ofAdditionalInfoContent, handles complex rendering with multiple sections (attachments, activities, etc.) but is not memoized.Recommendations:
- Memoize the
InboxItemDetailcomponent as it contains complex rendering logic- Consider batching the query invalidations during dialog movement
- Add error boundaries to prevent cascading render failures
🔗 Analysis chain
Line range hint
1-33: Verify if this change fully resolves the freezing issue.While the memo optimization is a good improvement, we should verify if this alone resolves the freezing when moving dialogs to bin. Consider:
- Are there other components in the dialog movement flow that need similar optimization?
- Could there be other causes of freezing (e.g., heavy computations during move operation)?
Let me analyze the results and generate additional verification scripts to check for other dialog-related components that might need optimization.
The first script will help identify other dialog components that could benefit from memoization. The second will help understand the dialog movement logic, and the third will identify components sharing the same styles which might affect performance during dialog operations.
Let me analyze the dialog movement operations and related components to verify if there are other potential performance bottlenecks.
This will help us understand if there are other components in the dialog movement flow that might need similar optimization and if there are any heavy computations during the move operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential performance bottlenecks in dialog movement logic # Look for bin-related operations in the codebase echo "Checking bin-related operations..." rg -A 5 "moveToBin|deleteDial|removeDialog" packages/frontend/src # Look for potential heavy computations (e.g., large loops, complex operations) echo "\nChecking for potential performance bottlenecks..." rg "while|for.*in|forEach|map|filter|reduce" $(fd --type f --extension tsx --extension jsx . packages/frontend/src | rg -i "dialog")Length of output: 8922
Script:
#!/bin/bash # Check for other dialog-related components that might need memo optimization echo "Checking for non-memoized dialog components..." ast-grep --pattern 'export const $_ = ({ $$$}) => { $$$ }' packages/frontend/src/components/ # Check for expensive operations in dialog movement flow echo "\nChecking dialog movement operations..." rg -A 5 "moveDialog|updateDialog|handleMove" packages/frontend/src # Check for other dialog components using the same styles echo "\nChecking components using dialog styles..." rg -l "inboxItemDetail.module.css" packages/frontend/srcLength of output: 86758
Script:
#!/bin/bash # Check for other components that might need memo optimization echo "Checking InboxItemDetail component..." cat packages/frontend/src/components/InboxItem/InboxItemDetail.tsx # Check for any heavy operations in dialog movement echo "\nChecking dialog movement implementation..." cat packages/frontend/src/pages/InboxItemPage/InboxItemPage.tsx # Check if there are other components using the same styles that might need optimization echo "\nChecking AdditionalInfoContent usage..." rg -A 5 "AdditionalInfoContent" packages/frontend/srcLength of output: 13691
packages/frontend/src/components/MainContentReference/MainContentReference.tsx (2)
11-11: Consider enhancing error handling strategy.While adding error handlers is a good improvement, consider these enhancements:
- Use a proper error tracking service instead of just console.error
- Implement error boundaries or fallback UI for better user experience
- Add error reporting to your monitoring system for production environments
- return <Markdown onError={(e) => console.error('Markdown error: ', e)}>{data}</Markdown>; + return ( + <ErrorBoundary fallback={<div>Failed to load content</div>}> + <Markdown + onError={(e) => { + reportError('markdown_render_error', e); + console.error('Markdown error: ', e); + }} + > + {data} + </Markdown> + </ErrorBoundary> + );Also applies to: 13-13
19-31: Optimize query configuration and type safety.The query implementation could be improved in several ways:
- Consider extracting the fetch logic into a separate function
- Add retry configuration for failed requests
- Add proper error typing
+ const fetchContent = async (url: string, token: string) => { + const response = await fetch(url, { + headers: { + 'Content-Type': 'text/plain', + Authorization: `Bearer ${token}`, + }, + }); + if (!response.ok) { + throw new Error(`Failed to fetch content: ${response.statusText}`); + } + return response.text(); + }; const { data, isSuccess } = useQuery({ queryKey: [QUERY_KEYS.MAIN_CONTENT_REFERENCE, content?.url, content?.mediaType], - queryFn: () => - fetch(content!.url, { - headers: { - 'Content-Type': 'text/plain', - Authorization: `Bearer ${dialogToken}`, - }, - }).then((res) => res.text()), + queryFn: () => fetchContent(content!.url, dialogToken), + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), enabled: content?.url !== undefined && Object.values(EmbeddableMediaType).includes(content.mediaType), });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx(2 hunks)packages/frontend/src/components/MainContentReference/MainContentReference.tsx(1 hunks)
🔇 Additional comments (1)
packages/frontend/src/components/MainContentReference/MainContentReference.tsx (1)
19-20: Verify memoization impact on performance.
While memoization is a good optimization, let's verify if it fully addresses the freezing issue when moving dialogs to bin.
| @@ -1,12 +1,13 @@ | |||
| import { Html, Markdown } from 'embeddable-markdown-html'; | |||
| import styles from '../InboxItem/inboxItemDetail.module.css'; | |||
| import { memo } from 'react'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider memoizing DialogToolbar and dialog components for performance optimization
Based on the code analysis:
DialogToolbarshould be memoized as it receives multiple callback props (archiveAction, deleteAction, undoAction) that could cause unnecessary re-rendersConfirmDeleteDialogandEditSavedSearchDialogare already optimized withforwardRefbut could benefit frommemosince they receive props and contain complex UI logicSelectedDialogsis a context provider component and doesn't need memoization
Suggested locations to add memo:
packages/frontend/src/components/DialogToolbar/DialogToolbar.tsxpackages/frontend/src/pages/SavedSearches/ConfirmDeleteDialog/ConfirmDeleteDialog.tsxpackages/frontend/src/pages/SavedSearches/EditSavedSearchDialog/EditSavedSearchDialog.tsx
🔗 Analysis chain
Verify memo import usage across the codebase.
Let's check if other components that interact with dialogs could benefit from similar memo optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find other dialog-related components that might need memo optimization
# Look for dialog-related component files
echo "Finding dialog-related components..."
fd --type f --extension tsx --extension jsx . packages/frontend/src | rg -i "dialog"
# Check current usage of memo in these files
echo "\nChecking memo usage in dialog components..."
rg "memo" $(fd --type f --extension tsx --extension jsx . packages/frontend/src | rg -i "dialog")
Length of output: 1145
Script:
#!/bin/bash
# Let's check the content of these dialog-related components to analyze their complexity and re-render patterns
echo "Checking DialogToolbar component..."
cat packages/frontend/src/components/DialogToolbar/DialogToolbar.tsx
echo -e "\nChecking SelectedDialogs component..."
cat packages/frontend/src/components/PageLayout/SelectedDialogs.tsx
echo -e "\nChecking ConfirmDeleteDialog component..."
cat packages/frontend/src/pages/SavedSearches/ConfirmDeleteDialog/ConfirmDeleteDialog.tsx
echo -e "\nChecking EditSavedSearchDialog component..."
cat packages/frontend/src/pages/SavedSearches/EditSavedSearchDialog/EditSavedSearchDialog.tsx
Length of output: 10628
| if (!content || !isSuccess) { | ||
| return null; | ||
| } | ||
| return <section className={styles.mainContentReference}>{getContent(content.mediaType, data)}</section>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and accessibility.
The component should handle loading and error states explicitly, and include proper ARIA attributes for better accessibility.
- if (!content || !isSuccess) {
+ if (!content) {
return null;
+ }
+
+ if (isLoading) {
+ return <section className={styles.mainContentReference} aria-busy="true">Loading content...</section>;
+ }
+
+ if (isError) {
+ return (
+ <section className={styles.mainContentReference} role="alert">
+ Failed to load content. Please try again later.
+ </section>
+ );
}
- return <section className={styles.mainContentReference}>{getContent(content.mediaType, data)}</section>;
+ return (
+ <section
+ className={styles.mainContentReference}
+ aria-label="Main content"
+ >
+ {getContent(content.mediaType, data)}
+ </section>
+ );Committable suggestion skipped: line range outside the PR's diff.
|
Foreslår at onError har en default-funksjon i komponenten Markdown, f.eks. "unable to parse markdown" eller noe, så kan du fjerne disse litt generiske / unødvendige |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/embeddable-markdown-html/src/html.tsx (1)
16-17: Consider enhancing error details.While making
onErrorrequired is good, the error handling could be more specific about what went wrong during HTML processing.Consider this enhancement:
export const Html: React.FC<{ children: string; - onError: (err: Error) => void; + onError: (err: Error & { phase?: 'parsing' | 'sanitization' | 'react-conversion' }) => void; }> = ({ children, onError }) => { const [reactContent, setReactContent] = useState<ReactElement | null>(null); useEffect(() => { unified() .use(rehypeParse, {} as RehypeParseOptions) .use(rehypeSanitize) .use(addClasses, defaultClassMap) .use(rehypeReact, production) .process(children) .then((vfile) => setReactContent(vfile.result as ReactElement)) - .catch((e) => onError(e)); + .catch((e: Error) => onError(Object.assign(e, { phase: 'parsing' }))); }, [children, onError]);packages/embeddable-markdown-html/src/markdown.tsx (1)
27-27: Consider enhancing error handling architecture.While making
onErrorrequired is good, consider these architectural improvements:
- Provide a default error handler in a higher-level component to reduce boilerplate
- Implement structured error types instead of
unknownfor better error handling- Consider adding error boundaries as a fallback mechanism
Example implementation of a wrapper with default error handling:
const withDefaultErrorHandler = (WrappedComponent: typeof Markdown) => { return ({ onError, ...props }: Parameters<typeof Markdown>[0]) => { const defaultErrorHandler = (error: unknown) => { console.error('Markdown parsing error:', error); // Add your default error UI/logging here }; return <WrappedComponent onError={onError || defaultErrorHandler} {...props} />; }; }; export const MarkdownWithDefaultError = withDefaultErrorHandler(Markdown);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/embeddable-markdown-html/src/html.tsx(1 hunks)packages/embeddable-markdown-html/src/markdown.tsx(1 hunks)
🔇 Additional comments (2)
packages/embeddable-markdown-html/src/markdown.tsx (2)
Line range hint 27-45: Verify error handling effectiveness for freezing issue.
The component processes markdown asynchronously in useEffect. While the error handling looks correct, we should verify:
- All potential error cases are caught
- The error handler is actually preventing the reported freezing
Let's analyze the error handling paths:
#!/bin/bash
# Description: Analyze error handling coverage
# Expected: Find potential unhandled error cases
# Search for other catch blocks or error handling in related files
rg -A 3 'catch|throw|Error|error' packages/embeddable-markdown-html/
# Look for reported freezing issues in commit history
git log -p --grep="freez" 27-27:
Breaking change: onError prop is now required.
This change enforces proper error handling by making the onError prop mandatory. While this is a good practice, it's a breaking change that requires updates to all existing usage of the Markdown component.
Let's verify the impact on existing usage:
✅ Verification successful
Let me gather more information about the Markdown component usage to ensure we have a complete picture.
Let me try a different approach to find the Markdown component usage.
Breaking change has already been addressed in existing usage
From the search results, I can see that all current usages of the Markdown component have been properly updated:
-
In
MainContentReference.tsx, it's usingonError:<Markdown onError={(e) => console.error('Markdown error: ', e)}>{data}</Markdown>
-
The only other usage in
AdditonalInfoContent.tsxis part of the same PR that fixed the freezing issue, where the component was memoized and error handling was added.
The breaking change was introduced as part of the fix for the application freezing issue, and all existing usages have been properly updated in the same PR to handle errors appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the Markdown component to assess migration impact
# Expected: List of files that need updates to provide onError prop
# Search for Markdown component usage
rg -l '<Markdown'
# Search for specific cases where onError might be missing
rg -l '<Markdown(?![^>]*onError)[^>]*>'
Length of output: 6072
Script:
#!/bin/bash
# Search for all Markdown component imports and usages
rg -A 3 'import.*Markdown|<Markdown' --type tsx --type jsx
# Search for any test files that might use the Markdown component
fd -e test.tsx -e test.jsx -x rg -l "Markdown"
Length of output: 135
Script:
#!/bin/bash
# Search for Markdown component imports and usages without file type restrictions
rg -A 3 'import.*Markdown|<Markdown'
# Search for files containing Markdown
fd . -t f -x rg -l "Markdown" {}
Length of output: 3754
| onError: (err: Error) => void; | ||
| }> = ({ children, onError }) => { |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change: onError prop is now required in Html component
The verification reveals that some existing usages of the Html component don't provide the required onError prop:
packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx: MissingonErrorproppackages/embeddable-markdown-html/Readme.md: Example in documentation missingonErrorprop- Only
MainContentReference.tsxproperly handles errors withonError={(e) => console.error('Html error: ', e)}
These components need to be updated to handle errors explicitly to prevent UI freezes and comply with the new type signature.
🔗 Analysis chain
Making onError required improves error handling.
This change enforces explicit error handling, which is crucial for preventing UI freezes when HTML processing fails. This aligns well with the PR's objective of fixing application freezing issues.
Let's verify the impact on existing usages:
Let me check the actual usage of the Html component in the files to verify if they provide the onError prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the Html component to ensure they provide onError handler
# Search for Html component usage
rg -A 5 "import.*Html.*from.*embeddable-markdown-html"
# Search for direct Html component usage
ast-grep --pattern '<Html>$$$</Html>'
Length of output: 2685
Script:
#!/bin/bash
# Check implementation of Html component in the files found
rg -A 10 "<Html" packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx packages/frontend/src/components/MainContentReference/MainContentReference.tsx packages/storybook/src/stories/embeddable-markdown-html/html.stories.tsx packages/embeddable-markdown-html/Readme.md
Length of output: 3209
alexdigdir
force-pushed
the
fix/embeddable-bug
branch
from
4d1dbc4 to
7ef2f26
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/embeddable-markdown-html/src/html.tsx(1 hunks)packages/embeddable-markdown-html/src/markdown.tsx(1 hunks)packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/embeddable-markdown-html/src/html.tsx
- packages/embeddable-markdown-html/src/markdown.tsx
🔇 Additional comments (2)
packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx (2)
3-3: LGTM: Clean memo import addition
The addition of the named import for memo is clean and follows React best practices.
10-10: Good performance optimization with memo
The memoization of AdditionalInfoContent is a good optimization that should help prevent unnecessary re-renders, especially if this component is rendered multiple times in a list of dialogs. This change aligns well with addressing the reported freezing issues.
| if (!value) { | ||
| return null; | ||
| } | ||
|
|
||
| const getContent = (mediaType: string) => { | ||
| switch (mediaType) { | ||
| case 'text/html': | ||
| return <Html>{value}</Html>; | ||
| return <Html onError={(e) => console.error('Html error: ', e)}>{value}</Html>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling feedback for better UX
While adding error handlers is good, simply logging to console doesn't provide feedback to users. Consider implementing a more user-friendly approach as suggested in the PR comments.
Here's a suggested implementation:
- return <Html onError={(e) => console.error('Html error: ', e)}>{value}</Html>;
+ return (
+ <Html
+ onError={(e) => {
+ console.error('Html error: ', e);
+ return <div className={styles.error}>Unable to display HTML content</div>;
+ }}
+ >
+ {value}
+ </Html>
+ );
- return <Markdown onError={(e) => console.error('Markdown error: ', e)}>{value}</Markdown>;
+ return (
+ <Markdown
+ onError={(e) => {
+ console.error('Markdown error: ', e);
+ return <div className={styles.error}>Unable to parse markdown</div>;
+ }}
+ >
+ {value}
+ </Markdown>
+ );This change will:
- Maintain error logging for debugging
- Show user-friendly error messages
- Align with the suggestion from the PR comments
Also applies to: 20-20
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/components/PartyDropdown/PartyDropdown.tsx (1)
36-36: LGTM! Consider adding error boundaries.The simplified conditional expression improves readability while maintaining the same functionality. The nullish coalescing and optional chaining operators provide robust handling of undefined values.
Consider wrapping the component with an error boundary to gracefully handle any potential render failures:
import { Component, ErrorInfo, ReactNode } from 'react'; class PartyDropdownErrorBoundary extends Component< { children: ReactNode }, { hasError: boolean } > { constructor(props: { children: ReactNode }) { super(props); this.state = { hasError: false }; } static getDerivedStateFromError(_: Error) { return { hasError: true }; } componentDidCatch(error: Error, errorInfo: ErrorInfo) { console.error('PartyDropdown error:', error, errorInfo); } render() { if (this.state.hasError) { return <div>Error loading party selector</div>; } return this.props.children; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/frontend/src/components/Activity/Activity.tsx(1 hunks)packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx(2 hunks)packages/frontend/src/components/MainContentReference/MainContentReference.tsx(1 hunks)packages/frontend/src/components/PartyDropdown/PartyDropdown.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/frontend/src/components/AdditonalInfoContent/AdditonalInfoContent.tsx
- packages/frontend/src/components/MainContentReference/MainContentReference.tsx
🔇 Additional comments (1)
packages/frontend/src/components/Activity/Activity.tsx (1)
38-38: LGTM! Clean syntax improvement.
The simplified nullish coalescing operation maintains the same logic while improving code readability.
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
AdditionalInfoContentandMainContentReferencecomponents through memoization, reducing unnecessary re-renders.HtmlandMarkdowncomponents, now requiring an error handling function for better error management.Bug Fixes
MainContentReference, enhancing the reliability of content display.