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

fix: title would not get rendered and rendering times be wrong #254

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

Phoenix616
Copy link

@Phoenix616 Phoenix616 commented Jan 28, 2025

User description

This fixes that titles would not be displayed at all due to parsing errors of the received data.

Additionally to that the default timing was not what is actually used so that was adjusted too (the legacy action bar also does not fade in) and the the game_info message type which should only display in action bar was sent to chat too, this is filtered out now.


PR Type

Bug fix, Enhancement


Description

  • Fixed titles not rendering due to parsing errors.

  • Adjusted default transition times for better accuracy.

  • Ignored game_info messages in chat to prevent incorrect display.

  • Improved handling of title and action bar components with new parsing logic.


Changes walkthrough 📝

Relevant files
Bug fix
ChatProvider.tsx
Ignore `game_info` messages in chat                                           

src/react/ChatProvider.tsx

  • Added logic to ignore game_info messages in chat.
  • Ensured action bar messages are handled by TitleProvider.
  • +1/-0     
    Enhancement
    Title.stories.tsx
    Update default transition times in stories                             

    src/react/Title.stories.tsx

  • Updated default transition times for fade-in, stay, and fade-out.
  • Adjusted storybook example to reflect new timing values.
  • +3/-3     
    Title.tsx
    Refactor transition timing logic                                                 

    src/react/Title.tsx

  • Replaced default duration with separate fade-in and fade-out defaults.
  • Updated transition logic to use new default timing values.
  • +10/-9   
    TitleProvider.tsx
    Enhance title component parsing and timing                             

    src/react/TitleProvider.tsx

  • Introduced getComponent function for parsing title components.
  • Updated default timing values for title animations.
  • Improved handling of title, subtitle, and action bar packets.
  • +21/-4   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Also the game_info message type which should only display in action bar was sent to chat.
    Copy link

    codesandbox bot commented Jan 28, 2025

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The getComponent function lacks error handling for malformed input or parsing failures. This could cause runtime errors if invalid data is received.

    const getComponent = (input: string | any) => {
      if (typeof input === 'string') {
        // raw json is sent
        return mojangson.simplify(mojangson.parse(input))
      } else if (input.type === 'string') {
        // this is used for simple chat components without any special properties
        return { 'text': input.value }
      } else if (input.type === 'compound') {
        // this is used for complex chat components with special properties
        return nbt.simplify(input)
      }
      return input
    }
    Timing Edge Case

    The transition timing logic assumes transitionTimes will always be positive numbers. Should validate or handle edge cases where timing values are invalid.

    timeout={{
      enter: transitionTimes?.fadeIn ?? defaultFadeIn,
      exit: transitionTimes?.fadeOut ?? defaultFadeOut,
    }}

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use correct React hook

    The useMemo hook is used incorrectly for side effects. Replace with useEffect since
    the code sets up event listeners.

    src/react/TitleProvider.tsx [41-46]

    -useMemo(() => {
    +useEffect(() => {
       // todo move to mineflayer
       bot._client.on('set_title_text', (packet) => {
         setTitle(getComponent(packet.text))
         setOpenTitle(true)
       })
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using useMemo for side effects is an anti-pattern that could cause issues. useEffect is the correct hook for setting up event listeners, making this a critical improvement for proper React usage.

    9
    Add error handling for malformed input

    Add error handling around getComponent to prevent crashes when receiving malformed
    input. The current implementation assumes valid input format.

    src/react/TitleProvider.tsx [19-31]

     const getComponent = (input: string | any) => {
    -  if (typeof input === 'string') {
    -    // raw json is sent
    -    return mojangson.simplify(mojangson.parse(input))
    -  } else if (input.type === 'string') {
    -    return { 'text': input.value }
    -  } else if (input.type === 'compound') {
    -    return nbt.simplify(input)
    +  try {
    +    if (typeof input === 'string') {
    +      return mojangson.simplify(mojangson.parse(input))
    +    } else if (input?.type === 'string') {
    +      return { 'text': input.value }
    +    } else if (input?.type === 'compound') {
    +      return nbt.simplify(input)
    +    }
    +    return input || defaultText
    +  } catch (error) {
    +    console.error('Failed to parse component:', error)
    +    return defaultText
       }
    -  return input
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling is crucial for preventing application crashes when receiving malformed input from the server. The suggestion properly handles edge cases and provides fallback to defaultText.

    8

    @@ -14,6 +16,20 @@ const ticksToMs = (ticks: AnimationTimes) => {
    return ticks
    }

    const getComponent = (input: string | any) => {
    Copy link
    Owner

    Choose a reason for hiding this comment

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

    Suggested change
    const getComponent = (input: string | any) => {
    const getComponent = (input: string | Record<string, any>) => {

    Copy link
    Author

    Choose a reason for hiding this comment

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

    👍 makes sense. I'm not that well versed in TypeScript so I don't really know the appropriate types at times 😅

    @zardoy
    Copy link
    Owner

    zardoy commented Jan 28, 2025

    Very good and quality fix (at least as I can see from code).
    Thank you so much for providing it!

    @zardoy zardoy merged commit 41684bc into zardoy:next Jan 28, 2025
    3 checks passed
    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