-
Notifications
You must be signed in to change notification settings - Fork 35
Update sentry sample rate #650
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
Conversation
…rendering behavior
- Added renderedTabs prop to SimpleTab and TabContentDetector components. - Updated useEffect in TabContentDetector to check if the tab has already been rendered, ensuring proper rendering behavior and preventing duplicate calls to onRendered.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @shhdgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on refining application stability, performance, and robustness. It includes an adjustment to the Sentry tracing sample rate to manage monitoring overhead, introduces a safety check for DOM node removal to prevent errors, and implements optional chaining for more resilient property access in navigation and filtering logic. Additionally, a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the Sentry tracesSampleRate to reduce the volume of traces being sent. It also includes several small fixes and improvements across different components.
My review focuses on improving code quality and efficiency:
- In
Contributors/index.tsx, I've suggested a simpler way to remove the DOM node. - In
SimpleTab.tsx, I've identified an inefficientuseEffectimplementation and provided a detailed suggestion for a more performant solution usinguseCallbackand a proper dependency array. This is the most important feedback in this review. - For
RightNav.tsxandfilterRightToc.ts, I've noted some unnecessary optional chaining that could be simplified.
Overall, the changes are good, but the suggested refactoring for the useEffect in SimpleTab is recommended to avoid potential performance issues.
| const ref = useRef<HTMLDivElement>(null); | ||
| useEffect(() => { | ||
| if (ref.current && ref.current.children.length > 0) { | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the dependency array from useEffect causes it to run after every render, which is inefficient. This was likely done to fix a linting warning about missing dependencies.
A better approach is to specify the dependencies. For this useEffect, the dependencies are id and onRendered.
However, simply adding [id, onRendered] will cause the effect to run on every render if onRendered is a new function on each render of the parent SimpleTab component.
To fix this correctly, you should wrap the onRendered prop's creation in the SimpleTab component with useCallback. This will memoize the function and ensure the useEffect in TabContentDetector only runs when id or the memoized onRendered function actually changes.
Here's how you can apply this in SimpleTab.tsx:
// In SimpleTab component
const onRenderedCallback = React.useCallback((id: string) => {
setRenderedTabs((prev) => {
if (prev.includes(id)) {
return prev;
}
// Add the new tab and then sort based on actualTabs order
const newTabs = [...prev, id];
return newTabs.sort((a, b) => {
const aIndex = actualTabs.findIndex(
(tab) => (tab.props?.value || tab.props.label) === a
);
const bIndex = actualTabs.findIndex(
(tab) => (tab.props?.value || tab.props.label) === b
);
return aIndex - bIndex;
});
});
}, [actualTabs]);
// ... then in the return statement of SimpleTab, pass onRenderedCallback
<TabContentDetector
// ...
onRendered={onRenderedCallback}
>
{child}
</TabContentDetector>Then, you can update this useEffect as suggested.
| if ( | |
| }, [id, onRendered]); |
| // Check if the node is still a child of its parent before removing | ||
| if ( | ||
| appendedNode.parentNode && | ||
| appendedNode.parentNode.contains(appendedNode) | ||
| ) { | ||
| appendedNode.parentNode.removeChild(appendedNode); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this check is more robust and correctly prevents an error if the node is not a child of its parent, you can simplify this logic by using the .remove() method on the DOM element. It achieves the same goal of removing the node from its parent without throwing an error if the node has no parent, and it's more concise.
| // Check if the node is still a child of its parent before removing | |
| if ( | |
| appendedNode.parentNode && | |
| appendedNode.parentNode.contains(appendedNode) | |
| ) { | |
| appendedNode.parentNode.removeChild(appendedNode); | |
| } | |
| appendedNode.remove(); |
| } | ||
| } | ||
| if (item.items) { | ||
| if (item?.items) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optional chaining on item here is not necessary. item is an element from the items array being iterated over with forEach, and it's not expected to be null or undefined in this context. The original code if (item.items) was sufficient and slightly cleaner.
| if (item?.items) { | |
| if (item.items) { |
src/shared/filterRightToc.ts
Outdated
|
|
||
| // Recursively filter nested items | ||
| if (item.items && item.items.length > 0) { | ||
| if (item?.items && item.items.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to another change in this PR, the optional chaining on item is not necessary here. item is an element from the items array being iterated over with .map(), and it's not expected to be null or undefined at this point in the function. The original code was sufficient.
| if (item?.items && item.items.length > 0) { | |
| if (item.items && item.items.length > 0) { |
- Integrated useCloudPlan hook to retrieve the current cloud plan. - Modified navigation links in HeaderNavStack and HeaderNavStackMobile to dynamically adjust the URL based on the cloud plan, enhancing user experience.
- Removed optional chaining for item.items to ensure proper filtering of nested items in the filterRightToc function, enhancing functionality.
* fix: update TOC item access in DocTemplate component - Modified the condition for accessing TOC items to ensure it handles undefined cases correctly. - Updated the dependency array in the useMemo hook to include the entire toc object for better stability in TOC data generation. * feat: add support for premium slug in generateConfig function (#647) * feat: add support for premium slug in generateConfig function - Introduced a new condition to handle slugs that include "premium/", allowing for better categorization in the configuration generation process. * refactor: update CloudPlan import paths across components - Replaced imports of CloudPlan from "shared/useCloudPlan" to "shared/interface" for consistency and clarity. - Adjusted related components to ensure proper usage of the updated CloudPlan type definition. * feat: extend EXTENDS_FOLDERS to include premium plan - Updated the EXTENDS_FOLDERS constant to include "premium" as a valid CloudPlan option, enhancing the flexibility of the table of contents generation. - Adjusted the type definition to ensure consistency with the new CloudPlan import. * feat: add CloudPlan type to path.ts for enhanced configuration - Imported CloudPlan into path.ts to support additional configuration options. - Updated the prefix variable type to CloudPlan | undefined for improved type safety in the generateConfig function. * feat: restructure page creation logic by modularizing create-pages functionality - Removed the existing create-pages.ts file and replaced it with modular files for creating specific pages: create-docs, create-doc-home, create-cloud-api, create-search, and create-404. - Introduced an interface file to define common types and constants, enhancing code organization and maintainability. - Each new file encapsulates the logic for creating its respective page, improving clarity and separation of concerns in the page generation process. * refactor: simplify prefix determination in generateConfig function - Replaced multiple conditional checks for slug prefixes with a more concise approach using an array and the find method. - Maintained the existing logic for handling the dedicated prefix based on the presence of a name, improving code readability and maintainability. * fix: update template import paths in page creation files - Adjusted the import paths for template files in create-404, create-cloud-api, create-doc-home, create-docs, and create-search to ensure correct resolution. - Enhanced consistency across page creation logic by standardizing the path structure. * chore: add disk usage checks throughout production workflow - Introduced disk usage checks at various stages of the production workflow to monitor system resource usage. - Added checks after Docker image cleanup, node setup, dependency installation, cache restoration, and before/after deployment steps for better resource management. * chore: enhance production workflow by maximizing build space - Added a step to maximize build space using the easimon/maximize-build-space action, allowing for better resource management during builds. - Included additional disk usage checks to monitor space before and after the build process, improving overall workflow efficiency. * refactor: streamline production workflow commands for clarity - Consolidated multiple run commands into single block executions for installing dependencies and purging CDN cache, enhancing readability and maintainability of the workflow script. * chore: rename markdown source in gatsby-config for consistency - Updated the name of the markdown source from `markdown-pages1` to `markdown-pages` for improved clarity and consistency in the configuration. * refactor: optimize disk space management in production workflow - Removed redundant disk usage checks and consolidated cleanup commands into a single run step to streamline the production workflow. - Enhanced disk space management by directly removing unnecessary directories and pruning Docker images, improving build efficiency. * Update sentry sample rate (#650) * chore: adjust tracesSampleRate in Sentry configuration for improved performance * fix: improve node removal safety in useTotalContributors hook * fix: correct dependency array in TabContentDetector to ensure proper rendering behavior * fix: enhance TabContentDetector to prevent duplicate rendering of tabs - Added renderedTabs prop to SimpleTab and TabContentDetector components. - Updated useEffect in TabContentDetector to check if the tab has already been rendered, ensuring proper rendering behavior and preventing duplicate calls to onRendered. * feat: update HeaderNav to conditionally render cloud plan links - Integrated useCloudPlan hook to retrieve the current cloud plan. - Modified navigation links in HeaderNavStack and HeaderNavStackMobile to dynamically adjust the URL based on the cloud plan, enhancing user experience. * fix: correct optional chaining in filterRightToc function - Removed optional chaining for item.items to ensure proper filtering of nested items in the filterRightToc function, enhancing functionality. * fix: update HeaderNav links to handle undefined cloud plans - Modified the navigation links in HeaderNavStack and HeaderNavStackMobile to include a condition that allows for rendering the default URL when the cloud plan is undefined, improving user experience and navigation reliability. * src, static: update cn.pingcap.com links (#652) * tweak: remove pingcap account track script (#654) * Adjust TOC to support the same article linking from multiple locations (#653) * feat: enhance navigation state management with session storage - Updated the LeftNavTree component to support saving and retrieving navigation item IDs from session storage, improving user experience by maintaining state across sessions. - Modified the calcExpandedIds function to accept an optional targetId for more precise matching. - Implemented helper functions to handle session storage interactions for navigation items. * refactor: simplify navigation state management in LeftNavTree component - Removed session storage interactions for expanded and selected navigation items, streamlining state management. - Updated the calcExpandedIds function calls to eliminate unnecessary parameters, enhancing clarity and performance. * refactor: simplify scroll behavior in scrollToElementIfInView function - Updated the scrollToElementIfInView function to directly call scrollIntoView with a "center" block option, removing unnecessary viewport checks and improving code clarity. * refactor: reorganize imports and streamline createDocs function - Updated import paths for clarity and consistency. - Removed unused navigation URL generation functions to simplify the createDocs logic. - Introduced a default value for inDefaultPlan to enhance code readability. * refactor: simplify HeaderNav links by removing cloud plan conditions - Removed cloud plan checks from navigation links in HeaderNavStack and HeaderNavStackMobile, streamlining the URL generation for the TiDB Cloud link. - Cleaned up unused imports related to cloud plan management for improved code clarity. * refactor: update createDocHome to target new documentation path - Changed the regex filter in createDocHome to point to the updated documentation file path for TiDB Cloud. - Removed unused navigation URL generation functions to streamline the code and improve clarity. --------- Co-authored-by: Lilian Lee <lilin@pingcap.com> Co-authored-by: WD <me@wangdi.ink>
No description provided.