-
-
Notifications
You must be signed in to change notification settings - Fork 10
Update column order of user statistics table #4078
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
Conversation
📝 WalkthroughWalkthroughThe UserStatistics component's table columns are reorganized, reordering recentDomain and domainCount relative to their previous layout. Duplicate domainCount and wordCount cells at the end of rows are removed, with wordCount inserted after username and domainCount positioned after wordCount. Column headers and data cells are synchronized to the new order. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/components/Statistics/UserStatistics.tsx:
- Around line 43-46: The second column header TitleId is wrong: HeadCell
currently uses titleId="statistics.column.senseCount" while the data cell
renders t.wordCount; update the header to match the data by changing the
HeadCell titleId to "statistics.column.wordCount" (or alternatively change the
data cell to render t.senseCount if the intent was to show senses), ensuring
HeadCell and the corresponding data cell use the same key so the header
correctly describes the t.wordCount value.
🧹 Nitpick comments (1)
src/components/Statistics/UserStatistics.tsx (1)
52-64: Column reordering looks good; consider adding max-width to recentDomain.The approach of moving the recentDomain column to the end effectively prevents overflow of other columns when domain names are long. The data cell rendering is correct and properly handles null values for recentDomain.
Regarding your question about adding a max-width: Yes, adding a max-width constraint to the recentDomain column would be a good safeguard. This would ensure that even extremely long domain names don't break the layout, especially if additional columns are added in the future.
💡 Suggested enhancement for max-width
Consider adding a
sxprop to the recentDomain Cell to constrain its width:<Cell + sx={{ maxWidth: 250, overflow: "hidden", textOverflow: "ellipsis" }} body={ t.recentDomain ? ( <DomainChip domain={{ ...t.recentDomain, userId: undefined }} provenance /> ) : null } />Note: You may need to update the Cell component's props interface in
TableCells.tsxto accept an optionalsxprop and pass it to the underlying TableCell.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Statistics/UserStatistics.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: imnasnainaec
Repo: sillsdev/TheCombine PR: 4046
File: Backend/Controllers/StatisticsController.cs:131-131
Timestamp: 2025-12-10T21:52:47.388Z
Learning: In TheCombine repository, the GetDomainWordCount endpoint in StatisticsController uses Permission.WordEntry (not Permission.Statistics) because it serves the TreeView component used during word entry workflow, so the permission should align with who needs access to this data.
🧬 Code graph analysis (1)
src/components/Statistics/UserStatistics.tsx (1)
src/components/Statistics/TableCells.tsx (2)
HeadCell(5-13)Cell(20-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: test_coverage (22)
- GitHub Check: tox (3.12)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4078 +/- ##
==========================================
- Coverage 74.57% 66.02% -8.55%
==========================================
Files 295 241 -54
Lines 10938 6161 -4777
Branches 1372 783 -589
==========================================
- Hits 8157 4068 -4089
+ Misses 2385 1841 -544
+ Partials 396 252 -144
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jasonleenaylor
left a comment
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.
@jasonleenaylor reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec).
Sometimes the domain name can be very long, causing the last columns to overflow the available table width. Moving most-recent-domain to be the last column resolves that.
Should we also consider adding a max width to that column in case more columns are added after it in the future?
This change is
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.