Skip to content

Clean up react bad pratices [SYNTH-224]#1399

Open
0xda157 wants to merge 5 commits into
devfrom
dionkud/224/react-bad-pratice
Open

Clean up react bad pratices [SYNTH-224]#1399
0xda157 wants to merge 5 commits into
devfrom
dionkud/224/react-bad-pratice

Conversation

@0xda157

@0xda157 0xda157 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Task

SYNTH-224

Symptom

many bad practices which make code less readable and in some cases less performant

Solution

clean up the bad code

Verification

bun run fmt, bun run lint and bun run build all pass


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch from 743f001 to 0270205 Compare July 9, 2026 18:57
@0xda157
0xda157 marked this pull request as ready for review July 9, 2026 19:39
@0xda157
0xda157 requested review from a team as code owners July 9, 2026 19:39
@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch 4 times, most recently from dd70e0d to eefd0c1 Compare July 9, 2026 20:18
Comment thread fission/src/ui/components/Label.tsx Outdated
@0xda157 0xda157 changed the title Clean up react bad pratice [SYNTH-224] Clean up react bad pratices [SYNTH-224] Jul 9, 2026
@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch 3 times, most recently from b338494 to 6d8d27d Compare July 9, 2026 22:36
Comment thread fission/src/ui/components/ContextMenu.tsx Outdated

const AnalyticsConsent: React.FC<AnalyticsConsentProps> = ({ onConsent, onClose }) => {
return (
const AnalyticsConsent = ({ onConsent, onClose }: AnalyticsConsentProps): JSX.Element => (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are quite a few changes to switch the typing of functional components, what's the theory behind that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that React.FC<AnalyticsConsentProps> is too broad and AnalyticsConsentProps -> React.JSX.Element is a much more straight forward and narrower type. If we don't use React.FC the return type becomes much more useful, for example (): React.JSX.Element | null tells someone using their lsp hover that the function may not render, without having to view implementation of the component. As far as I can tell, aside from React.FC previously being bad pratice, there's no strong recommendation either way in the react community.

Comment thread fission/src/util/TypeConversions.ts Outdated
@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch 6 times, most recently from f4c10a5 to 5f4a41b Compare July 10, 2026 16:34
@0xda157
0xda157 requested a review from rutmanz July 10, 2026 18:36

const AnalyticsConsent: React.FC<AnalyticsConsentProps> = ({ onConsent, onClose }) => {
return (
const AnalyticsConsent = ({ onConsent, onClose }: AnalyticsConsentProps): React.JSX.Element => (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My IDE is also warning that React should be imported instead of used as a global namespace to make resolution more reliable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And this matters less but stylistically I prefer React.ReactElement over React.JSX.Element (which is basically an alias)

@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch from 5f4a41b to cb8df2a Compare July 10, 2026 21:09
@0xda157

0xda157 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

lowkey decided that React.FC doesn't matter that much the remove ternary blocks doesn't really matter and too much of a pain

@0xda157
0xda157 force-pushed the dionkud/224/react-bad-pratice branch from cb8df2a to 376c493 Compare July 13, 2026 17:42

@rutmanz rutmanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Otherwise LGTM

For the future though, it's a lot more tedious to re-review a PR if you force push because you can't see what's changed since the last review anymore.

Comment thread fission/src/.npmrc Outdated
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