Get tweets from archive#4
Open
erosika wants to merge 8 commits intofractal-nyc:mainfrom
Open
Conversation
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to f09f510 in 3 minutes and 5 seconds
More details
- Looked at
255lines of code in4files - Skipped
2files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/twitter/getTweets.ts:27
- Draft comment:
Ensure 'fetch' is available in Node (<v18) or include a polyfill. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment raises a valid technical point about Node.js compatibility. However, this feels like something that would be immediately caught during testing or first run. Additionally, if Node <18 compatibility was required, it would likely be specified in package.json or other project requirements. Without seeing those, we can't be certain this is actually an issue.
I might be underestimating the importance of backwards compatibility. Also, even if it would be caught in testing, preventing the error before testing could save time.
While valid points, we should trust that the author has considered their Node.js version requirements. If fetch isn't available, it would be immediately obvious during first run or basic testing.
Delete the comment. While technically correct, this is something that would be immediately apparent during basic testing and depends on project-level requirements we don't have context for.
2. src/twitter/getTweets.ts:26
- Draft comment:
Ensure the Node environment supports the global fetch API or include a polyfill (e.g., node-fetch) for compatibility in older Node versions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the comment identifies a real potential issue with Node compatibility, it falls into the category of dependency/environment concerns that we're supposed to ignore. The PR author likely knows their Node environment and build setup. If fetch isn't available, it would fail immediately in testing/building.
The fetch API compatibility could be a real runtime issue that breaks the code. Not having this polyfill could cause production failures.
Per our rules, we should not comment on dependency or environment setup issues. This would be caught immediately during testing/building, making it an obvious issue that doesn't need a comment.
Delete this comment as it relates to dependency/environment setup which we've been instructed to ignore.
3. src/twitter/getTweets.ts:33
- Draft comment:
Consider verifying the structure of 'data.tweets' exists and is in the expected format before using Object.values on it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a valid concern since the code assumes the response has a specific structure. A malformed response could cause runtime errors. However, this is a new file being added, and the code is presumably working with a known API endpoint. The error would be caught by the try-catch in saveTweetsToJson anyway.
The comment is speculative - we don't know if the API response structure is well-defined and guaranteed. The existing error handling might be sufficient for the use case.
While the concern is valid from a defensive programming standpoint, the existing error handling would catch any issues, and we don't have enough context about the API contract to know if additional validation is truly necessary.
The comment should be deleted as it's speculative and the existing error handling is sufficient for a new file working with a presumably known API endpoint.
4. src/twitter/getTweets.ts:57
- Draft comment:
Consider using asynchronous file system methods (e.g., fs.promises.mkdir, fs.promises.writeFile) to avoid blocking the event loop in non-CLI contexts. - Reason this comment was not posted:
Marked as duplicate.
5. src/twitter/getTweets.ts:27
- Draft comment:
Consider normalizing the username consistently (use lower-case for both fetch URL and output file naming) to avoid potential mismatches. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. src/ui/mycustomchat.tsx:1
- Draft comment:
Removal confirmed; ensure that no dependencies or references to MyCustomChat remain in the project. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_JS1aJ8HovnEK4wFq
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| async function fetchUserTweets(username: username): Promise<user> { | ||
| const baseUrl = 'https://fabxmporizzqflnftavs.supabase.co/storage/v1/object/public/archives'; | ||
| const response = await fetch(`${baseUrl}/${username.toLowerCase()}/archive.json`); |
There was a problem hiding this comment.
Consider sanitizing user input to prevent injection into URL.
Suggested change
| const response = await fetch(`${baseUrl}/${username.toLowerCase()}/archive.json`); | |
| const response = await fetch(`${baseUrl}/${encodeURIComponent(username.toLowerCase())}/archive.json`); |
| // Ensure the output directory exists | ||
| const outputDir = './training_data'; | ||
| if (!fs.existsSync(outputDir)) { | ||
| fs.mkdirSync(outputDir); |
There was a problem hiding this comment.
Consider using asynchronous FS APIs (mkdir and writeFile) for non-blocking I/O.
Suggested change
| fs.mkdirSync(outputDir); | |
| await fs.promises.mkdir(outputDir, { recursive: true }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
digital clone for community-archive hackathon
Important
Adds script to fetch and save tweets from archive and removes unused UI component.
getTweets.tsto fetch tweets from a specified archive URL and save them as JSON files.getUserInput(),fetchUserTweets(), andsaveTweetsToJson()functions to handle user input, fetch tweets, and save them..gitattributesto manageVividVoid__tweets.jsonwith Git LFS.mycustomchat.tsxfromsrc/uias it is no longer needed.This description was created by
for f09f510. It will automatically update as commits are pushed.