feat: allow filtering the category ids in anaytics#129
Merged
Conversation
Signed-off-by: Pranav <pranav10121@gmail.com>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 6566795 in 1 minute and 39 seconds. Click for details.
- Reviewed
594lines of code in9files - Skipped
0files when reviewing. - Skipped posting
9draft 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. frontend/app/page.tsx:24
- Draft comment:
Good use of a separate state for selectedCategoryIds to manage filtering. Consider extracting filtering logic into a custom hook if it grows. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
2. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:153
- Draft comment:
The dropdown filtering UI is well-implemented. Ensure that using '-1' to represent 'Uncategorized' is consistently handled across the UI and backend. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
3. frontend/hooks/useCategoryAnalytics.ts:9
- Draft comment:
Including categoryIds in the query key ensures proper cache invalidation and refetching when filters change. This is a good practice. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
4. frontend/lib/api/analytics.ts:58
- Draft comment:
Appending 'category_ids' to URL parameters when provided correctly enables backend filtering. Ensure backend expects a comma-separated list. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
5. server/internal/api/controller/analytics_controller.go:70
- Draft comment:
The parseCategoryIds function properly validates comma‐separated integer input. Logging or further tracing on error messages could aid in debugging. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
6. server/internal/api/controller/analytics_controller_test.go:600
- Draft comment:
New test cases for filtering category analytics (valid and invalid inputs) are comprehensive and improve reliability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
7. server/internal/mock/repository/analytics_repository.go:138
- Draft comment:
In the mock repository, the categoryIds parameter is currently unused. Consider simulating filtering behavior if specific tests require it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a mock repository used for testing. The author added thecategoryIdsparameter to match an interface signature (likely the real repository interface was updated). In mock repositories, it's completely normal and acceptable to have unused parameters - the mock doesn't need to implement full functionality unless tests specifically require it. The comment is speculative ("if specific tests require it") and doesn't identify any actual problem. It's essentially saying "you might want to do more work here in the future" which violates the rule about not making speculative comments. The code is working as intended for a mock. Could there be a specific test that already requires this filtering behavior that I'm not seeing? Maybe the comment is pointing out that tests are currently broken because the mock doesn't filter by categoryIds? If tests were broken, that would be caught by the build/test suite, which the rules say I should assume will catch such issues. The comment explicitly says "if specific tests require it" which is conditional/speculative language, not stating that tests are currently failing. This is clearly a speculative suggestion about future enhancement. This comment should be deleted. It's making a speculative suggestion about potential future enhancements to the mock rather than identifying a concrete issue with the current change. Mock repositories commonly have unused parameters to match interface signatures, and this is acceptable practice.
8. server/internal/repository/analytics_repository.go:140
- Draft comment:
The dynamic construction of the filterClause for category analytics uses parameterized queries effectively to mitigate SQL injection risks. Verify that the placeholder order remains correct as the length of categoryIds varies. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
9. server/internal/service/analytics_service.go:153
- Draft comment:
Validation in GetMonthlyAnalytics (ensuring end date is not before start date) is a good safeguard. Overall, delegating GetCategoryAnalytics and GetMonthlyAnalytics to the repository keeps the service layer clean. - Reason this comment was not posted:
Confidence changes required:0%<= threshold1%None
Workflow ID: wflow_GqaFSzFL2FOb8pSk
You can customize 
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Pranav <pranav10121@gmail.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 827d03c in 2 minutes and 15 seconds. Click for details.
- Reviewed
179lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6draft 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. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:56
- Draft comment:
Initializing draftSelectedIds from selectedCategoryIds is good. Note that the useEffect resets the draft state on prop changes; ensure this behavior suits cases with unsaved modifications. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold1%The comment is asking the author to ensure that the behavior of resetting the draft state on prop changes is suitable for cases with unsaved modifications. This is a request for confirmation of intended behavior, which violates the rules.
2. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:84
- Draft comment:
The isDirty flag detects differences between applied and draft selections. Document that order is irrelevant as it only checks length and membership. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold1%The comment is purely informative, explaining how the isDirty flag works and suggesting documentation. It doesn't provide a specific code suggestion or ask for a test to be written. It doesn't align with the rules for good comments.
3. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:90
- Draft comment:
toggleCategorySelection updates draftSelectedIds following the draft/apply pattern, which is clear. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold1%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the code is clear, which does not align with the rules for useful comments.
4. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:102
- Draft comment:
toggleSelectAll correctly uses hasAllSelectedDraft to update draft selections. Ensure the -1 identifier for 'Uncategorized' is consistently handled. - Reason this comment was not posted:
Comment was on unchanged code.
5. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:214
- Draft comment:
The 'Apply' button commits draft selections, with its disabled state tied to isDirty, enhancing UX. Consider adding a cancel/discard option for flexibility. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is suggesting an additional feature (a cancel button) rather than identifying a bug or problem. The current implementation already has implicit cancel behavior - when the dropdown closes and reopens, the useEffect resets the draft state to match the applied state. Adding an explicit cancel button would be a UX enhancement, but it's not a required change. The comment uses "Consider" which is soft language suggesting this is optional. This falls under the category of speculative/optional suggestions rather than identifying a clear issue that needs to be fixed. The rules state not to make comments unless there is clearly a code change required, and to avoid speculative comments. The comment could be valid from a UX perspective - having an explicit cancel button might be clearer to users than relying on implicit behavior. Some users might want to discard changes without closing the dropdown. This could be considered a reasonable UX improvement suggestion. While it could be a UX improvement, the comment doesn't identify a bug or required change. It's a "nice to have" suggestion using "Consider" language. The current implementation works correctly with implicit cancel behavior via the useEffect. According to the rules, comments should only be made when there is clearly a code change required, not for optional enhancements or speculative improvements. This comment should be deleted. It's an optional UX suggestion rather than identifying a required code change. The current implementation already handles the cancel scenario implicitly through the useEffect that resets draft state.
6. frontend/components/custom/CategoryAnalytics/CategoryAnalytics.tsx:190
- Draft comment:
The use of onSelect handlers with event.preventDefault() on DropdownMenuCheckboxItem is repeated. Consider refactoring to reduce duplication. - 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% The comment is about theonSelect={(event) => event.preventDefault()}pattern that was added in this PR. Looking at the diff, this is indeed a new addition (lines withonSelectare marked with+). The comment suggests reducing duplication, which is a code quality refactor suggestion. However, the duplication is minimal - it's just a single line repeated 4 times across two dropdown menus. The refactoring would require creating a wrapper component or a helper function, which might be overkill for such a simple one-liner. The rules state that "Comments that suggest code quality refactors are good! But only if they are actionable and clear." While this is actionable, it's debatable whether this level of duplication warrants a refactor. The pattern is idiomatic React and very clear as-is. This feels like a minor nitpick rather than a significant code quality issue. This is a borderline case. While the comment is technically correct about duplication, the duplication is minimal and the current code is clear and idiomatic. Refactoring this might actually make the code less readable by introducing unnecessary abstraction. The comment might be too pedantic for such a simple pattern. The comment is about a legitimate code change and suggests a refactor, which is allowed. However, given that it's only 4 instances of a very simple one-liner, and that refactoring would require additional abstraction that might not improve readability, this comment is likely not valuable enough to keep. It falls into the category of "obvious or unimportant" comments. This comment should be deleted. While it's technically about a change and suggests a refactor, the duplication is minimal (4 instances of a simple one-liner), and refactoring would likely add unnecessary complexity without meaningful benefit. This is too minor to be actionable.
Workflow ID: wflow_7024slZjxQhfJZe2
You can customize
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
1 participant
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.
Important
Add category ID filtering to category analytics, updating frontend components and backend services to support this feature.
page.tsx: Add stateselectedCategoryIdsand pass it touseCategoryAnalyticsandCategoryAnalytics.CategoryAnalytics.tsx: Add dropdown for category selection, handle category filter changes, and update UI based on selected categories.analytics_controller.go: Addcategory_idsquery parameter handling inGetCategoryAnalytics().analytics_repository.go: ModifyGetCategoryAnalytics()to filter bycategoryIds.analytics_service.go: UpdateGetCategoryAnalytics()to acceptcategoryIds.analytics_controller_test.go: Add tests for filtering bycategory_idsand handling invalidcategory_ids.This description was created by
for 827d03c. You can customize this summary. It will automatically update as commits are pushed.