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

FEAT: development helper in env hint #537

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HugoPerard
Copy link
Member

@HugoPerard HugoPerard commented Oct 3, 2024

Describe your changes

I create this PR as an idea, a proposal, it's a first draft to have some feedbacks.

The idea is to have a quick way to switch between light/dark modes during development, to avoid the pain to navigate to account page and then come back to our current page to try things on different theme mode.

This is the initial need, but maybe others use cases cause the same pain.

So I want to have a quick access to DarkModeSwitcher wherever we are in the app, and one stuff that is always display on development environnement is the EnvHint, that why I decide to improve it and transform it on a "development helper" opening button.

Screenshots

image

Documentation

Checklist

  • I performed a self review of my code
  • I ensured that everything is written in English
  • I tested the feature or fix on my local environment
  • I ran the pnpm storybook command and everything is working
  • If applicable, I updated the translations for english and french files
    (If you cannot update the french language, just let us know in the PR description)
  • If applicable, I updated the README.md
  • If applicable, I created a PR or an issue on the documentation repository
  • If applicable, I’m sure that my feature or my component is mobile first and available correctly on desktop

Summary by CodeRabbit

  • New Features

    • Introduced the DevToolsDrawer for managing theme and language preferences in a responsive drawer.
    • Implemented AppHint to display environmental hints at the top of the application.
    • Added conditional rendering for developer tools based on the new environment variable.
  • Bug Fixes

    • Removed the deprecated EnvHint component, streamlining the hint display logic.
  • Chores

    • Added a new environment variable, NEXT_PUBLIC_DEV_TOOLS_ENABLED, for controlling the availability of development tools.

Copy link

vercel bot commented Oct 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
start-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 4, 2024 3:41pm

Copy link
Contributor

coderabbitai bot commented Oct 3, 2024

Walkthrough

The changes involve modifications to the Document component in src/app/Document.tsx, where EnvHint is replaced by AppHint, and conditional rendering is added for the ReactQueryDevtools component based on the NEXT_PUBLIC_DEV_TOOLS_ENABLED environment variable. The layout.tsx file updates the import for getEnvHintTitlePrefix to source from AppHint. The new environment variable NEXT_PUBLIC_DEV_TOOLS_ENABLED is introduced in src/env.mjs. Additionally, the AppHint and DevToolsDrawer components are added to manage environmental hints and development settings, while the EnvHint file is removed.

Changes

File Change Summary
src/app/Document.tsx Replaced EnvHint with AppHint and added conditional rendering for ReactQueryDevtools.
src/app/layout.tsx Updated import for getEnvHintTitlePrefix to source from AppHint.
src/env.mjs Introduced new environment variable NEXT_PUBLIC_DEV_TOOLS_ENABLED for managing development tools visibility.
src/features/devtools/AppHint.tsx Added new components for displaying environmental hints and developer tools functionality.
src/features/devtools/DevToolsDrawer.tsx Introduced DevToolsDrawer component for managing development-related settings in a drawer.
src/features/devtools/EnvHint.tsx Removed the entire file, including the EnvHint component and its associated function.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Document
    participant AppHint
    participant DevToolsDrawer

    User->>Document: Load Document component
    Document->>AppHint: Render AppHint based on environment variables
    User->>AppHint: Check for development tools
    AppHint->>Document: Conditionally render ReactQueryDevtools
    User->>DevToolsDrawer: Open DevToolsDrawer for settings
    DevToolsDrawer->>User: Display development settings options
Loading

Possibly related PRs

Suggested reviewers

  • ivan-dalmet

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
src/features/devtools/EnvHintDevPopover.tsx (2)

27-39: Popover implementation looks great. Consider enhancing flexibility.

The Popover structure is well-implemented using Chakra UI components. It provides a clear and user-friendly interface for the development helper.

To enhance flexibility, consider allowing customization of the popover content:

 export const EnvHintDevPopover: React.FC<{
   children: React.ReactElement;
+  popoverContent?: React.ReactNode;
 }> = ({
   children,
+  popoverContent = <DarkModeSwitch />,
 }) => {
   // ...
   <PopoverBody>
-    <DarkModeSwitch />
+    {popoverContent}
   </PopoverBody>
   // ...
 };

This change would allow users of the component to pass custom content to the popover while still providing the DarkModeSwitch as a default.


1-40: Great implementation! Consider future extensibility.

The EnvHintDevPopover component successfully addresses the PR objective by providing easy access to theme switching. The implementation is clean, well-structured, and follows React and Chakra UI best practices.

For future extensibility, consider:

  1. Creating a configuration object for development helpers, allowing easy addition of new tools.
  2. Implementing a custom hook (e.g., useDevHelpers) to manage the state and logic of development helpers.
  3. Adding telemetry or logging to track usage of development helpers, which could inform future improvements.

Example structure for future enhancements:

const devHelpers = [
  { name: 'Dark Mode', component: DarkModeSwitch },
  // Add more helpers here
];

const useDevHelpers = () => {
  // Implement logic for managing dev helpers
};

// In the component:
const { activeHelpers } = useDevHelpers();

// Render activeHelpers in the PopoverBody

These suggestions aim to make the component more scalable as new development helpers are added in the future.

src/features/devtools/EnvHint.tsx (1)

45-49: LGTM: Development helper implementation.

The conditional rendering based on env.NEXT_PUBLIC_NODE_ENV successfully implements the development helper feature as described in the PR objectives. It ensures that the EnvHintDevPopover is only available in the development environment, maintaining existing functionality for other environments.

Consider extracting the environment check into a constant for improved readability:

+ const isDevelopment = env.NEXT_PUBLIC_NODE_ENV === 'development';

- {env.NEXT_PUBLIC_NODE_ENV === 'development' ? (
+ {isDevelopment ? (
    <EnvHintDevPopover>{hintContent}</EnvHintDevPopover>
  ) : (
    hintContent
  )}

This minor refactoring would make the code slightly more readable and easier to maintain.

src/components/DarkModeSwitch/index.tsx (1)

17-45: LGTM: Component structure and UI implementation are well-designed.

The component structure provides a clear and accessible UI for toggling between light and dark modes. The use of conditional rendering for FormLabel styling and the controlled Switch component are particularly well-implemented.

Consider extracting the repeated FormLabel props into a separate object or component to reduce code duplication. For example:

const SharedFormLabelProps = {
  htmlFor: id,
  mb: "0",
};

// Then in your JSX:
<FormLabel
  {...SharedFormLabelProps}
  as={colorMode === 'light' ? 'span' : undefined}
  opacity={colorMode !== 'light' ? 0.5 : undefined}
  mr={0}
>
  {t('account:preferences.theme.light')}
</FormLabel>

This would make the component more maintainable and reduce the risk of inconsistencies between the two labels.

src/features/account/PageAccount.tsx (1)

Issue Detected: Missing DarkModeSwitch Component

The DarkModeSwitch component appears to have been removed from the src/components directory, but it is still being referenced in src/features/account/PageAccount.tsx at line 89. This likely results in a broken reference and could cause runtime errors.

  • File Affected: src/features/account/PageAccount.tsx
  • Line: 89

Please ensure that all references to DarkModeSwitch are updated to use the new "development helper" functionality or remove them if the component is no longer needed.

🔗 Analysis chain

Line range hint 89-89: Verify: Implementation of "development helper" functionality

The PR objectives mention transforming the EnvHint into a "development helper" button for easy access to the DarkModeSwitcher. However, the DarkModeSwitch component is still being used in the same way as before in this file. Please verify if the "development helper" functionality has been implemented in the DarkModeSwitch component itself.

To check the implementation of the "development helper" functionality, please run the following script:

This script will help us locate any new code related to the "development helper" functionality and check the current implementation of the DarkModeSwitch component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for the implementation of "development helper" functionality

# Search for EnvHint or development helper related code
rg -i "envhint|development.?helper" src

# Check the DarkModeSwitch component for any new functionality
fd -t f "DarkModeSwitch" src/components -x cat {}

Length of output: 1026

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ff723c0 and 78946f8.

📒 Files selected for processing (4)
  • src/components/DarkModeSwitch/index.tsx (1 hunks)
  • src/features/account/PageAccount.tsx (1 hunks)
  • src/features/devtools/EnvHint.tsx (3 hunks)
  • src/features/devtools/EnvHintDevPopover.tsx (1 hunks)
🔇 Additional comments (10)
src/features/devtools/EnvHintDevPopover.tsx (2)

1-20: LGTM: Imports and component declaration are well-structured.

The 'use client' directive, imports, and component declaration are correctly implemented. The use of TypeScript for prop typing is a good practice.


21-23: Conditional rendering looks good. Verify env import.

The conditional check to prevent rendering when necessary conditions are not met is a good practice. However, let's ensure that the env import is correctly set up.

Please run the following script to verify the env import:

✅ Verification successful

Please run the following script to verify the NEXT_PUBLIC_ENV_NAME usage in TypeScript files:


Env import and NEXT_PUBLIC_ENV_NAME usage verified successfully.

The env import is correctly set up, and NEXT_PUBLIC_ENV_NAME is properly utilized across the relevant TypeScript files, including EnvHintDevPopover.tsx.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the env import and NEXT_PUBLIC_ENV_NAME usage

# Test 1: Check if env.mjs exists
fd --type f "env.mjs$"

# Test 2: Check for NEXT_PUBLIC_ENV_NAME in env.mjs
rg "NEXT_PUBLIC_ENV_NAME" --type js

Length of output: 72


Script:

# 
#!/bin/bash
# Description: Verify the NEXT_PUBLIC_ENV_NAME usage in TypeScript files

# Test 1: Check if env.ts or env.tsx exists
fd --type f "env\.tsx?$"

# Test 2: Check for NEXT_PUBLIC_ENV_NAME in TypeScript files
rg "NEXT_PUBLIC_ENV_NAME" --type ts

Length of output: 449

src/features/devtools/EnvHint.tsx (3)

4-4: LGTM: New import for development helper.

The new import of EnvHintDevPopover aligns with the PR objectives to add a development helper feature. The import style is consistent with the existing code.


17-33: Good refactoring: Extracted hint content.

Extracting the JSX into a separate hintContent constant improves code readability and organization. This refactoring makes the code more maintainable without changing its functionality.


Line range hint 1-53: Overall: Successfully implemented development helper feature.

The changes in this file successfully implement the development helper feature as described in the PR objectives. The EnvHint component now conditionally renders the EnvHintDevPopover in the development environment, which should allow for easier theme switching during development.

Key improvements:

  1. Added new import for EnvHintDevPopover.
  2. Refactored hint content into a separate constant for better readability.
  3. Implemented conditional rendering based on the environment.

These changes enhance the developer experience without affecting the functionality in non-development environments. The code is well-organized and maintains consistent styling with the existing codebase.

src/components/DarkModeSwitch/index.tsx (4)

1-10: LGTM: Imports are appropriate and follow best practices.

The imports are well-organized and include all necessary components and hooks for the DarkModeSwitch functionality. The use of useId for generating unique IDs and useTranslation for internationalization are particularly good practices.


12-15: LGTM: Component declaration and hooks usage are correct.

The component is properly declared as a named export, and the hooks are used correctly at the top level of the component. The use of useId for generating a unique ID is particularly good for accessibility.


1-46: Summary: DarkModeSwitch component successfully implements PR objectives

This new component effectively addresses the PR's main objective of providing a quick method to switch between light and dark modes. It's well-structured, uses appropriate React hooks and Chakra UI components, and follows good accessibility practices.

The implementation aligns with the goal of enhancing the development experience by allowing easy access to theme switching from any location in the app. This should streamline the workflow during development as intended.

To fully meet the PR objectives:

  1. Ensure this component is integrated into the "development helper" button mentioned in the PR description.
  2. Verify that it works correctly in both mobile and desktop formats as per the PR checklist.
  3. Update any relevant documentation to reflect this new feature.

To verify the component's responsiveness, you can run:

#!/bin/bash
# Check for responsive design implementation
rg -i '(sm:|md:|lg:|xl:)' src/components/DarkModeSwitch/index.tsx

This will help identify if any responsive design considerations have been implemented directly in the component.


15-15: Good accessibility practices, with room for improvement.

The use of useId to generate a unique ID and the proper association of labels with the Switch using htmlFor are excellent accessibility practices.

To further improve accessibility:

  1. Consider adding an aria-label to the Switch component to provide a clear description for screen readers. For example:
<Switch
  aria-label={t('account:preferences.theme.switchLabel')}
  // ... other props
/>
  1. Ensure that the opacity changes for the inactive label meet WCAG color contrast requirements. You may want to test this with accessibility tools or consider using a more distinct visual difference.

To verify the color contrast, you can run the following command:

This will help identify any potential color contrast issues in the application.

Also applies to: 23-23, 32-32, 38-38

src/features/account/PageAccount.tsx (1)

20-20: LGTM: Import change for DarkModeSwitch

The import statement for DarkModeSwitch has been updated to import from a separate component file. This change aligns with the PR objectives of transforming the EnvHint into a "development helper" button and improves code modularity.

return null;
}

const childrenAsButton = cloneElement(children, { as: 'button' });
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider preserving existing properties when cloning children.

While converting the child to a button is a good approach for consistency, it might override existing properties. Consider merging the new properties with the existing ones:

- const childrenAsButton = cloneElement(children, { as: 'button' });
+ const childrenAsButton = cloneElement(children, { ...children.props, as: 'button' });

This change ensures that any existing properties or event handlers on the child element are preserved while still applying the as: 'button' property.

📝 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 childrenAsButton = cloneElement(children, { as: 'button' });
const childrenAsButton = cloneElement(children, { ...children.props, as: 'button' });

Copy link
Contributor

Choose a reason for hiding this comment

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

No, cloneElement already takes care of merging children props with the ones passed as the second parameter. This code is ok

Copy link
Contributor

Choose a reason for hiding this comment

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

@DecampsRenan, thank you for the clarification. You're correct; cloneElement automatically merges the existing child's props with the new props. I'll keep that in mind for future reviews.

(_/)

(•_•)

Curious rabbit is learning!


✏️ Learnings added
Learnt from: DecampsRenan
PR: BearStudio/start-ui-web#537
File: src/features/devtools/EnvHintDevPopover.tsx:0-0
Timestamp: 2024-10-11T14:57:52.868Z
Learning: When using `React.cloneElement`, remember that it automatically merges the child's existing `props` with the new props provided, so manually spreading `children.props` is unnecessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (9)
src/features/devtools/DevToolsDrawer.tsx (3)

23-27: Consider using a named interface for props.

While the current prop type definition is correct, using a named interface could improve readability and documentation.

Consider refactoring the props definition as follows:

interface DevToolsDrawerProps {
  disclosure: ReturnType<typeof useDisclosure>;
}

export const DevToolsDrawer: React.FC<DevToolsDrawerProps> = ({ disclosure }) => {
  // ... component body
};

This change would make the component's API more explicit and easier to understand for other developers.


44-51: LGTM: Dark mode toggle is well-implemented.

The dark mode toggle functionality is correctly implemented using the useColorMode hook and a Checkbox component.

Consider extracting the onChange handler to a separate function for improved readability:

const handleColorModeChange = (e: React.ChangeEvent<HTMLInputElement>) => {
  setColorMode(e.target.checked ? 'dark' : 'light');
};

// Then in the JSX:
<Checkbox
  isChecked={colorMode === 'dark'}
  onChange={handleColorModeChange}
>
  Dark mode
</Checkbox>

This change would make the component slightly more readable and easier to maintain.


52-66: LGTM: Language selection is well-implemented with room for optimization.

The language selection functionality is correctly implemented using the i18n instance and a RadioGroup component.

Consider memoizing the language options to optimize performance:

const languageOptions = useMemo(() => 
  AVAILABLE_LANGUAGES.map((language) => (
    <Radio key={language.key} value={language.key}>
      {t(`common:languages.${language.key}`)}
    </Radio>
  )),
  [t]
);

// Then in the JSX:
<Wrap spacing={4}>
  {languageOptions}
</Wrap>

This change would prevent unnecessary re-renders of the language options when the component re-renders for reasons unrelated to language changes.

src/features/devtools/AppHint.tsx (4)

7-11: LGTM! Consider using nullish coalescing for conciseness.

The getEnvHintTitlePrefix function logic is correct and follows a clear priority order. However, you could make it more concise using nullish coalescing.

Consider refactoring the function as follows:

export const getEnvHintTitlePrefix = () => 
  env.NEXT_PUBLIC_ENV_EMOJI ?? 
  (env.NEXT_PUBLIC_ENV_NAME ? `[${env.NEXT_PUBLIC_ENV_NAME}] ` : '');

This achieves the same result with a more compact syntax.


13-41: LGTM! Consider adding ARIA attributes for better accessibility.

The AppHint component is well-structured and correctly implements the conditional rendering based on environment variables. The styling is appropriate for a hint component, and the use of insetStart and insetEnd ensures RTL compatibility.

To improve accessibility, consider adding ARIA attributes to the Box component:

 <Box
   zIndex="9999"
   position="fixed"
   top="0"
   insetStart="0"
   insetEnd="0"
   h="2px"
   bg={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.400`}
+  role="banner"
+  aria-label="Environment and Developer Tools Hint"
 >

This will help screen readers understand the purpose of this component.


43-56: LGTM! Consider extracting color scheme to a constant for flexibility.

The EnvHint component is well-structured and consistently styled with the AppHint component. It effectively displays the environment name with appropriate styling.

To improve flexibility and maintainability, consider extracting the color scheme to a constant:

const ENV_COLOR_SCHEME = env.NEXT_PUBLIC_ENV_COLOR_SCHEME;

const EnvHint = () => {
  return (
    <Text
      bg={`${ENV_COLOR_SCHEME}.400`}
      color={`${ENV_COLOR_SCHEME}.900`}
      // ... other props
    >
      {env.NEXT_PUBLIC_ENV_NAME}
    </Text>
  );
};

This makes it easier to change the color scheme in the future if needed.


57-78: LGTM! Consider improving accessibility and type safety.

The DevToolsHint component is well-structured and consistently styled with the EnvHint component. It effectively toggles the DevToolsDrawer and uses appropriate icons.

Consider the following improvements:

  1. Use a proper button element for better accessibility:
-    <Text
-      as="button"
+    <Button
+      variant="unstyled"
       onClick={devToolsDrawer.onToggle}
       bg={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.400`}
       color={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.900`}
       px="1"
       py="0.5"
       borderBottomStartRadius="sm"
       borderBottomEndRadius="sm"
       textTransform="uppercase"
+      aria-label="Toggle Developer Tools"
     >
       <LuPencilRuler />
-    </Text>
+    </Button>
  1. Add type safety to the disclosure prop of DevToolsDrawer:
import { UseDisclosureReturn } from '@chakra-ui/react';

type DevToolsDrawerProps = {
  disclosure: UseDisclosureReturn;
};

const DevToolsDrawer: React.FC<DevToolsDrawerProps> = ({ disclosure }) => {
  // ... component implementation
};

These changes will improve accessibility and type safety in your component.

src/app/Document.tsx (1)

70-72: LGTM: Conditional rendering of ReactQueryDevtools adds flexibility.

The addition of conditional rendering for ReactQueryDevtools based on the NEXT_PUBLIC_DEV_TOOLS_ENABLED environment variable is a good improvement. It allows for more control over when the dev tools are available.

Consider updating the project documentation to mention this new environment variable and its effect on the ReactQueryDevtools. This will help other developers understand how to enable/disable this feature.

src/env.mjs (1)

65-74: LGTM! Consider simplifying the default value logic.

The new environment variable NEXT_PUBLIC_DEV_TOOLS_ENABLED is well-defined and aligns with the PR objectives. It correctly handles the conversion to boolean and provides a sensible default based on the NODE_ENV.

Consider simplifying the ternary operator on line 73:

-        return process.env.NODE_ENV === 'development' ? true : false;
+        return process.env.NODE_ENV === 'development';

This change maintains the same logic while making the code more concise.

🧰 Tools
🪛 Biome

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 78946f8 and 93f85b3.

📒 Files selected for processing (6)
  • src/app/Document.tsx (2 hunks)
  • src/app/layout.tsx (1 hunks)
  • src/env.mjs (2 hunks)
  • src/features/devtools/AppHint.tsx (1 hunks)
  • src/features/devtools/DevToolsDrawer.tsx (1 hunks)
  • src/features/devtools/EnvHint.tsx (0 hunks)
💤 Files with no reviewable changes (1)
  • src/features/devtools/EnvHint.tsx
🧰 Additional context used
🪛 Biome
src/env.mjs

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

🔇 Additional comments (10)
src/app/layout.tsx (1)

7-7: LGTM! Import statement updated correctly.

The change from EnvHint to AppHint in the import path aligns with the PR objectives of transforming the EnvHint into a more general "development helper". This update suggests a broader scope for the component, which is a good direction.

Consider renaming the imported function for consistency.

While the import path has been updated, the function name getEnvHintTitlePrefix still references "EnvHint". For better consistency and clarity, consider renaming this function to match the new "AppHint" naming convention.

For example:

-import { getEnvHintTitlePrefix } from '@/features/devtools/AppHint';
+import { getAppHintTitlePrefix } from '@/features/devtools/AppHint';

Remember to update all usages of this function throughout the codebase if you decide to rename it.

Verify implementation of new development helper functionality.

The PR objectives mention adding functionality for switching between light and dark modes, but this change is not reflected in this file.

Could you please confirm that the new development helper functionality, including the dark mode switcher, has been implemented in the AppHint component? You can use the following script to check the implementation:

src/features/devtools/DevToolsDrawer.tsx (3)

1-21: LGTM: Imports and 'use client' directive are well-organized.

The 'use client' directive is correctly placed, and imports are logically grouped. All imported items appear to be used in the component.


28-42: LGTM: Component structure and hook usage are correct.

The component correctly uses the necessary hooks and properly structures the Chakra UI Drawer component. The use of the disclosure prop for controlling the drawer's state is appropriate.


1-72: Great implementation of the DevToolsDrawer component!

The DevToolsDrawer component successfully implements the requested functionality for toggling dark mode and changing language, aligning well with the PR objectives. It provides an easy-to-use interface for developers to switch between light and dark modes, as well as change the application language, from any location within the app.

Key points:

  1. The component is well-structured and uses Chakra UI components effectively.
  2. Client-side rendering is correctly implemented with the 'use client' directive.
  3. React hooks are used appropriately throughout the component.
  4. The implementation is reusable and can be easily integrated into the main application.

The component achieves the goal of enhancing the development experience by providing quick access to theme and language changes, as outlined in the PR summary. This implementation should significantly streamline the workflow during development.

To ensure that this component is properly integrated and accessible throughout the application, please run the following verification script:

This script will help verify that the DevToolsDrawer is properly integrated into the application and that there are no remaining TODO items related to its implementation.

src/features/devtools/AppHint.tsx (1)

1-78: Overall, the implementation aligns well with the PR objectives.

The AppHint component and its subcomponents effectively implement the requested feature of a development helper for switching between light and dark modes. The code is well-structured, uses appropriate React and Chakra UI patterns, and maintains consistency in styling.

A few minor suggestions have been made to improve accessibility, type safety, and code flexibility. These changes will further enhance the quality and maintainability of the code.

src/app/Document.tsx (3)

10-11: LGTM: Import changes align with PR objectives.

The replacement of EnvHint with AppHint and the addition of the env import are consistent with the PR's goal of transforming the EnvHint into a development helper. These changes set the foundation for the new functionality.


69-69: LGTM: AppHint replaces EnvHint as per PR objectives.

The replacement of <EnvHint /> with <AppHint /> directly implements the proposed development helper functionality. This change aligns perfectly with the PR's goal.

Please ensure that the new AppHint component includes the DarkModeSwitcher functionality as mentioned in the PR objectives. You may want to verify this in your local environment.


Line range hint 1-78: Overall, great implementation that achieves PR objectives.

The changes in this file successfully implement the development helper functionality as described in the PR objectives. The replacement of EnvHint with AppHint, along with the addition of conditional rendering for ReactQueryDevtools, should significantly improve the development experience.

A few follow-up actions to consider:

  1. Verify that the AppHint component includes the DarkModeSwitcher functionality as intended.
  2. Update project documentation to reflect the new NEXT_PUBLIC_DEV_TOOLS_ENABLED environment variable.
  3. Ensure that the new AppHint component is responsive and works well on both mobile and desktop formats, as mentioned in your PR checklist.

Great work on improving the developer experience!

src/env.mjs (2)

96-96: LGTM! Runtime environment declaration is correct.

The new environment variable NEXT_PUBLIC_DEV_TOOLS_ENABLED is correctly added to the runtimeEnv object, ensuring it's accessible at runtime.


Line range hint 65-96: Summary: Changes align well with PR objectives

The implementation of the NEXT_PUBLIC_DEV_TOOLS_ENABLED environment variable effectively supports the new development helper feature described in the PR objectives. It provides a flexible way to control the visibility of development tools, defaulting to enabled in development environments and disabled in others.

The changes are well-integrated into the existing environment configuration structure, maintaining consistency with the file's overall architecture. This implementation will allow for easy toggling of the development helper button, facilitating theme testing as intended.

🧰 Tools
🪛 Biome

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

@HugoPerard HugoPerard force-pushed the feat/development-helper branch from 93f85b3 to 8c7904b Compare October 4, 2024 15:38
Copy link

sonarqubecloud bot commented Oct 4, 2024

@HugoPerard
Copy link
Member Author

HugoPerard commented Oct 4, 2024

After some feedbacks, I come with a new version of the development helper :

  1. A specific label in AppHint
  2. A new env variable to manage display of devtools
  3. A drawer with content instead of a popover
  4. Custom components for the development drawer instead of factorisation with app components
  5. Integration of language selection on the drawer

image

image

Enregistrement.de.l.ecran.2024-10-04.a.17.43.05.mov

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/features/devtools/AppHint.tsx (3)

7-11: LGTM: Well-structured utility function.

The getEnvHintTitlePrefix function is well-implemented, handling different cases of environment variables. It's a pure function, which is good for testability and predictability.

Consider using early returns for improved readability:

 export const getEnvHintTitlePrefix = () => {
-  if (env.NEXT_PUBLIC_ENV_EMOJI) return `${env.NEXT_PUBLIC_ENV_EMOJI} `;
-  if (env.NEXT_PUBLIC_ENV_NAME) return `[${env.NEXT_PUBLIC_ENV_NAME}] `;
-  return '';
+  if (env.NEXT_PUBLIC_ENV_EMOJI) {
+    return `${env.NEXT_PUBLIC_ENV_EMOJI} `;
+  }
+  if (env.NEXT_PUBLIC_ENV_NAME) {
+    return `[${env.NEXT_PUBLIC_ENV_NAME}] `;
+  }
+  return '';
 };

13-42: LGTM: Well-structured main component with conditional rendering.

The AppHint component is well-implemented, with conditional rendering based on environment variables. It uses Chakra UI components effectively for styling and layout.

Consider adding an aria-label to the Box component for better accessibility:

 <Box
   zIndex="9999"
   position="fixed"
   top="0"
   insetStart="0"
   insetEnd="0"
   h="2px"
   bg={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.400`}
+  aria-label="Environment and developer tools hint"
 >

57-82: LGTM: Well-integrated developer tools hint component.

The DevToolsHint component effectively integrates the DevToolsDrawer functionality and maintains consistent styling with the EnvHint component.

Consider adding an aria-label to the button for better accessibility:

 <Text
   as="button"
   fontWeight="bold"
   onClick={devToolsDrawer.onToggle}
   bg={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.400`}
   color={`${env.NEXT_PUBLIC_ENV_COLOR_SCHEME}.900`}
   px="1"
   borderBottomStartRadius="sm"
   borderBottomEndRadius="sm"
   textTransform="uppercase"
   display="flex"
   alignItems="center"
   gap="1"
+  aria-label="Toggle developer tools"
 >
src/env.mjs (1)

Line range hint 1-108: Summary: Well-implemented feature for development tools toggle

The addition of NEXT_PUBLIC_DEV_TOOLS_ENABLED to the environment configuration is well-executed and aligns with the PR objectives. It provides a flexible mechanism to toggle development tools, which can include the mentioned dark mode switcher and potentially other developer-focused features in the future.

Key points:

  1. The variable is correctly placed in the client-side schema, making it accessible in the browser.
  2. The implementation allows for easy configuration through environment variables or defaults based on the current environment.
  3. The changes are non-breaking and localized, maintaining the integrity of the existing environment setup.

This implementation lays a good foundation for the "development helper" button mentioned in the PR summary, enabling easy access to development tools from any location within the app.

As you continue to develop this feature, consider the following:

  1. Ensure that the development tools are only rendered when this flag is true.
  2. Document the usage of this new environment variable for other developers.
  3. If you plan to add more development tools in the future, consider creating a configuration object that can be toggled by this flag, allowing for more granular control over individual tools.
🧰 Tools
🪛 Biome

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 93f85b3 and 8c7904b.

📒 Files selected for processing (6)
  • src/app/Document.tsx (2 hunks)
  • src/app/layout.tsx (1 hunks)
  • src/env.mjs (2 hunks)
  • src/features/devtools/AppHint.tsx (1 hunks)
  • src/features/devtools/DevToolsDrawer.tsx (1 hunks)
  • src/features/devtools/EnvHint.tsx (0 hunks)
💤 Files with no reviewable changes (1)
  • src/features/devtools/EnvHint.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/app/Document.tsx
  • src/app/layout.tsx
  • src/features/devtools/DevToolsDrawer.tsx
🧰 Additional context used
🪛 Biome
src/env.mjs

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

🔇 Additional comments (4)
src/features/devtools/AppHint.tsx (3)

1-5: LGTM: Imports are well-organized and relevant.

The import statements are logically organized, with external libraries first, followed by local imports. All imports seem relevant to the components being created in this file.


44-56: LGTM: Concise and focused environment hint component.

The EnvHint component is well-implemented, rendering the environment name in a styled format consistent with the environment color scheme.


1-82: Great implementation aligning with PR objectives.

The AppHint.tsx file successfully implements the proposed "development helper" functionality, addressing the PR objectives. The components are well-structured, use Chakra UI effectively, and follow React best practices. The implementation provides an easy way to access development tools from any location within the app, as intended.

Some minor suggestions for improvement have been made in previous comments, mainly focusing on accessibility enhancements.

To ensure that this new feature is properly integrated and doesn't conflict with existing code, let's run a quick verification:

src/env.mjs (1)

65-74: LGTM! Consider minor improvements for clarity and simplicity.

The implementation of NEXT_PUBLIC_DEV_TOOLS_ENABLED is well-structured and aligns with the PR objectives. It provides a flexible way to toggle development tools across different environments. Here are a couple of suggestions for improvement:

  1. Simplify the ternary operator on line 73 as suggested by the static analysis tool.
  2. Consider refactoring the default value logic for improved readability.

Here's a suggested refactor:

 NEXT_PUBLIC_DEV_TOOLS_ENABLED: z
   .enum(['true', 'false'])
   .optional()
   .transform((value) => {
     if (value) {
       return value === 'true';
     }

-    return process.env.NODE_ENV === 'development' ? true : false;
+    return process.env.NODE_ENV === 'development';
   }),

This change simplifies the ternary operator and makes the default behavior more explicit.

To ensure this change doesn't affect other parts of the codebase, let's verify the usage:

Also applies to: 96-96

✅ Verification successful

Change Verified and Safe

The refactor simplifies the ternary operator and does not negatively impact the usage of NEXT_PUBLIC_DEV_TOOLS_ENABLED in the following files:

  • src/features/devtools/AppHint.tsx
  • src/app/Document.tsx

No further issues were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of NEXT_PUBLIC_DEV_TOOLS_ENABLED
rg --type typescript --type javascript 'NEXT_PUBLIC_DEV_TOOLS_ENABLED'

Length of output: 105


Script:

#!/bin/bash
# Search for usage of NEXT_PUBLIC_DEV_TOOLS_ENABLED in TypeScript and JavaScript files
rg --type ts --type tsx --type js --type jsx 'NEXT_PUBLIC_DEV_TOOLS_ENABLED'

Length of output: 104


Script:

#!/bin/bash
# Search for usage of NEXT_PUBLIC_DEV_TOOLS_ENABLED in TypeScript and JavaScript files
rg 'NEXT_PUBLIC_DEV_TOOLS_ENABLED' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx'

Length of output: 370

🧰 Tools
🪛 Biome

[error] 73-73: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

Comment on lines +44 to +66
<Checkbox
isChecked={colorMode === 'dark'}
onChange={(e) =>
setColorMode(e.target.checked ? 'dark' : 'light')
}
>
Dark mode
</Checkbox>
<FormControl>
<FormLabel>Language</FormLabel>
<RadioGroup
value={i18n.language}
onChange={(newValue) => i18n.changeLanguage(newValue)}
>
<Wrap spacing={4}>
{AVAILABLE_LANGUAGES.map((language) => (
<Radio key={language.key} value={language.key}>
{t(`common:languages.${language.key}`)}
</Radio>
))}
</Wrap>
</RadioGroup>
</FormControl>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we can extract these components into external files; we can see them as a lib of standalone dev components we can add / remove based on project needs :) And I think it will make the drawer more readable in long term, even if it's only development tools

I don't know if it is too much or not, but I prefer to see something like this:

<DrawerBody>
  <Stack spacing={6}>
    <DarkModeDevTool />
    <LanguageSwitcherDevTool />
  </Stack>
</DrawerBody>

instead of adding everything inside the drawer; So if I add a new tool for managing tanstack query (toggle dev tools, delete cache, etc...) I can just add and It Just Works™ :p

I also know that I can update only DarkModeDevTool instead of finding related code inside the drawer for future updates :)

I think you get the idea :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants