-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: models hub should show latest data only #5925
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: models hub should show latest data only #5925
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to cfcdedb in 2 minutes and 14 seconds. Click for details.
- Reviewed
64lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useModelSources.ts:33
- Draft comment:
Replace merging logic with assignment to latest data. Note that usingnewSources.length ? newSources : get().sourcesmeans that if the API returns an empty array, the old sources are retained. Ensure that this fallback behavior is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/routes/hub/index.tsx:164
- Draft comment:
Adding 'sources' to the dependency array ensures that the deduplication check uses the latest state. However, this might trigger extra API calls or re-renders if sources changes frequently. Verify if this behavior is intended or consider using a ref for stable access. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests a potential issue with the current implementation, specifically that adding 'sources' to the dependency array might lead to unnecessary API calls or re-renders. It also provides a suggestion to use a ref for stable access. However, it asks the author to verify if the behavior is intended, which violates the rules. The suggestion to consider using a ref is valid, but the request to verify the behavior is not allowed.
3. web-app/src/routes/hub/index.tsx:142
- Draft comment:
Both the useEffect and the search change handler set a timeout to fetch HF repo info. Consider adding a cleanup function in the useEffect hook (and ensuring any pending timeouts are cleared) to prevent overlapping timer callbacks on rapid dependency changes. - 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_yH7ai8YlNmHABnPD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 5096a2a in 1 minute and 17 seconds. Click for details.
- Reviewed
100lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/__tests__/useModelSources.test.ts:98
- Draft comment:
Updated test name and expectation to verify that new sources replace (not merge with) existing ones. This change aligns with the requirement to display only the latest catalog data. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what was done and why. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
2. web-app/src/hooks/__tests__/useModelSources.test.ts:175
- Draft comment:
Deduplication test updated to expect only new sources when duplicate model names occur. This ensures that existing HF models are not duplicated, meeting the issue requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what a test update does. It doesn't provide a suggestion, ask for confirmation on a specific change, or point out a potential issue. It violates the rule against making purely informative comments.
3. web-app/src/hooks/__tests__/useModelSources.test.ts:470
- Draft comment:
Multiple fetch operations test now verifies that subsequent fetch calls replace rather than merge source data. This confirms that only the latest fetched data is maintained. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what a test does. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
Workflow ID: wflow_pxVo1pfxSnHmEstF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 37.38%Your code coverage diff: -0.03% ▾ Uncovered files and lines
|
Describe Your Changes
This PR is to enhance the hub model list to just display latest data from model catalog instead of merging previous models list with new update. It should also not to show HF model which is already exist in the list.
Fixes Issues
Self Checklist
Important
Ensure model hub displays only the latest data by updating
useModelSourcesto avoid merging with existing data and preventing duplicate models from HuggingFace.useModelSourcesnow fetches and displays only the latest model data, avoiding merging with existing data inuseModelSources.ts.index.tsxby checking existingsourcesbefore adding.useModelSources.test.tsto reflect changes in merging logic, ensuring only new data is used.This description was created by
for 5096a2a. You can customize this summary. It will automatically update as commits are pushed.