Skip to content

Conversation

@severo
Copy link
Contributor

@severo severo commented Nov 12, 2025

Screenshot From 2025-11-12 17-11-02

@severo severo requested review from Copilot and platypii and removed request for platypii November 12, 2025 16:10
Copilot finished reviewing on behalf of severo November 12, 2025 16:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a URL input field to the hyparquet demo, allowing users to manually enter a parquet file URL to load. Similar functionality appears to already exist in the icebird demo.

  • Added a form with URL input and Load button to the hyparquet Welcome component
  • Added CSS styles for form inputs, labels, and input groups
  • Connected the Welcome component to the existing URL loading mechanism via the onUrlDrop callback

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
hyparquet/src/Welcome.tsx Added Props interface, URL input form with submit handler that calls setUrl
hyparquet/src/App.tsx Passed onUrlDrop as setUrl prop to Welcome component
hyparquet/src/index.css Added styling for label, input, textarea, and inputGroup elements
icebird/src/Welcome.tsx Removed urlRef from useCallback dependency array

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const value = urlRef.current?.value ?? ''
const url = value === '' ? exampleUrl : value
setUrl(url)
}, [setUrl])
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The useCallback dependency array is missing exampleUrl and urlRef. While urlRef is a ref and stable, exampleUrl is a local constant that's captured in the closure. Since exampleUrl is defined inside the component function, it should be included in the dependency array, or moved outside the component to avoid this issue. To fix, either add exampleUrl to the dependencies: }, [setUrl, exampleUrl]) or move the exampleUrl constant declaration outside the component function.

Suggested change
}, [setUrl])
}, [setUrl, exampleUrl])

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@platypii platypii Nov 12, 2025

Choose a reason for hiding this comment

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

Not sure why a const needs to be in the dependency array. If you move the exampleUrl to the top level then I think it will stop complaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the comment does not make sense to me. Anyway, moving outside does not harm either

@severo severo merged commit 9a4b68f into master Nov 12, 2025
8 checks passed
@severo severo deleted the add-url-input branch November 12, 2025 17:22
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.

3 participants