maintenance: fixing minor code smells #29706

Pedrao1234 · 2024-11-25T22:27:40Z

What I did

This PR addresses and resolves 8 code smells, categorized and sourced as follows:

JSX outside the render method

Fixed two instances where JSX was incorrectly placed outside of the render method.
Files affected:
- storybook/code/core/src/manager/FakeProvider.tsx
- storybook/code/core/src/manager/settings/shortcuts.tsx
Identified using the ReactSniffer library.

Direct DOM manipulations

Fixed two occurrences where DOM was being manipulated directly, replacing them with React-compatible methods.
Files affected:
- storybook/code/lib/blocks/src/blocks/DocsContainer.tsx
- storybook/code/lib/blocks/src/blocks/mdx.tsx
Identified using the ReactSniffer library.

Inheritance instead of Composition

Refactored two places where inheritance was used instead of composition, improving code flexibility and maintainability.
Files affected:
- storybook/code/core/src/manager/FakeProvider.tsx
- storybook/code/renderers/react/src/renderToCanvas.tsx
Identified using the ReactSniffer library.

Force Update

Corrected two cases where forceUpdate was used, replacing them with more appropriate state management solutions to ensure proper re-rendering.
Files affected:
- storybook/code/core/src/manager/components/sidebar/RefBlocks.tsx
- storybook/code/core/src/manager/components/sidebar/RefIndicator.tsx
Identified using the ReactSniffer library.

These changes improve the overall code quality, readability, and maintainability.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

This pull request focuses on code quality improvements across multiple Storybook components, addressing React anti-patterns and modernizing component implementations.

Converted class components to functional components in code/core/src/manager/FakeProvider.tsx and code/renderers/react/src/renderToCanvas.tsx, favoring composition over inheritance
Replaced direct DOM manipulations with React state management in code/lib/blocks/src/blocks/DocsContainer.tsx and code/lib/blocks/src/blocks/mdx.tsx
Improved error handling and state management in code/core/src/manager/components/sidebar/RefBlocks.tsx and RefIndicator.tsx
Refactored code/core/src/manager/settings/shortcuts.tsx for better type safety and event handling consistency
Added proper dependency arrays to useEffect/useCallback hooks across multiple components

greptile-apps

_{7 file(s) reviewed, 18 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-25T22:28:10Z

code/core/src/manager/FakeProvider.tsx

+  // @ts-expect-error (Converted from ts-ignore)
+  const addonsInstance = addons;
+  // @ts-expect-error (Converted from ts-ignore)
+  const channel = {
+    on: () => {},
+    once: () => {},
+    off: () => {},
+    emit: () => {},
+    addListener: () => {},
+    removeListener: () => {},
+  };


style: Consider using a proper mock object or testing utility instead of ts-expect-error and empty function stubs

greptile-apps · 2024-11-25T22:28:10Z

code/core/src/manager/FakeProvider.tsx

-  handleAPI(api) {
-    addons.loadAddons(api);
-  }
+  const handleAPI = (api: any) => addonsInstance.loadAddons(api);


style: api parameter should be properly typed instead of any

greptile-apps · 2024-11-25T22:28:11Z

code/core/src/manager/FakeProvider.tsx

-    );
-  }
-}
+export const PrettyFakeProvider = (props: any) => {


style: props should be properly typed instead of any

greptile-apps · 2024-11-25T22:28:37Z

code/core/src/manager/components/sidebar/RefBlocks.tsx

  const refresh = useCallback(() => {
-    globalWindow.document.location.reload();
+    setAuthAttempted(false);
  }, []);


logic: Setting isAuthAttempted to false won't trigger a page refresh. Need to actually reload the page to fetch new data.

greptile-apps · 2024-11-25T22:29:04Z

code/core/src/manager/components/sidebar/RefIndicator.tsx

@@ -278,6 +278,8 @@ const SourceCodeMessage: FC<{

 const LoginRequiredMessage: FC<RefType> = ({ loginUrl, id }) => {
  const theme = useTheme();
+  const [isLoggedIn, setIsLoggedIn] = useState(false);


syntax: useState import is missing from React imports at the top of the file

greptile-apps · 2024-11-25T22:31:16Z

code/lib/blocks/src/blocks/mdx.tsx

  const OcticonHeader = OcticonHeaders[as];
  const hash = `#${id}`;
+  const elementRef = useRef<HTMLAnchorElement | null>(null);


logic: elementRef is declared but never used

greptile-apps · 2024-11-25T22:31:17Z

code/lib/blocks/src/blocks/mdx.tsx

@@ -177,10 +165,16 @@ const HeaderWithOcticonAnchor: FC<PropsWithChildren<HeaderWithOcticonAnchorProps
  ...rest
 }) => {
  const context = useContext(DocsContext);
-
-  // @ts-expect-error (Converted from ts-ignore)
  const OcticonHeader = OcticonHeaders[as];


logic: potential runtime error if 'as' is not a valid header type

greptile-apps · 2024-11-25T22:31:46Z

code/renderers/react/src/renderToCanvas.tsx

+  // We replace the lifecycle method with a try-catch block
+  const errorHandler = (error: Error) => {
+    componentDidCatch(error);
+  };


logic: errorHandler function is defined but never used, and doesn't properly replicate error boundary functionality

greptile-apps · 2024-11-25T22:31:46Z

code/renderers/react/src/renderToCanvas.tsx

+  const componentDidCatch = (err: Error) => {
    showException(err);
-  }
+    setHasError(true);
+  };


logic: componentDidCatch is defined but not hooked into React's error boundary system - functional components cannot be error boundaries

greptile-apps · 2024-11-25T22:31:47Z

code/renderers/react/src/renderToCanvas.tsx

  if (forceRemount) {
    unmountElement(canvasElement);
  }


style: Consider adding back the important comment explaining why forceRemount is necessary for React instance recreation

nx-cloud · 2024-11-25T23:43:56Z

View your CI Pipeline Execution ↗ for commit 43a045c.

Command	Status	Duration	Result
`nx run-many -t build --parallel=3`	✅ Succeeded	1m 35s	View ↗

☁️ Nx Cloud last updated this comment at 2024-12-22 21:52:24 UTC

fix: fixing minor code smells

43a045c

greptile-apps bot reviewed Nov 25, 2024

View reviewed changes

yannbf assigned kasperpeulen Dec 2, 2024

yannbf added maintenance User-facing maintenance tasks ci:merged Run the CI jobs that normally run when merged. labels Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maintenance: fixing minor code smells #29706

maintenance: fixing minor code smells #29706

Pedrao1234 commented Nov 25, 2024 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

greptile-apps bot Nov 25, 2024

nx-cloud bot commented Nov 25, 2024 •

edited

Loading

maintenance: fixing minor code smells #29706

Are you sure you want to change the base?

maintenance: fixing minor code smells #29706

Conversation

Pedrao1234 commented Nov 25, 2024 • edited by greptile-apps bot Loading

What I did

JSX outside the render method

Direct DOM manipulations

Inheritance instead of Composition

Force Update

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 25, 2024

Choose a reason for hiding this comment

nx-cloud bot commented Nov 25, 2024 • edited Loading

Pedrao1234 commented Nov 25, 2024 •

edited by greptile-apps bot

Loading

nx-cloud bot commented Nov 25, 2024 •

edited

Loading