-
Notifications
You must be signed in to change notification settings - Fork 537
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
refactor: convert virtual table into a real table #2855
Conversation
… into feat/logs-v2-data-fetching
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
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx (1)
119-119
: Remove unnecessary string literal space.The string literal space
{" "}
after the closing div is unnecessary and can be removed.- </div>{" "} + </div>apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx (1)
91-93
: Consider using a more reliable key for the permission badges.Using array indices as React keys can lead to rendering issues if the array items are reordered or modified. Consider using a more stable unique identifier.
- // biome-ignore lint/suspicious/noArrayIndexKey: its okay to use it as a key - key={index} + key={`${permission}-${index}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/logs/components/control-cloud/index.tsx
(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx
(2 hunks)apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx
(1 hunks)apps/dashboard/components/timestamp-info.tsx
(2 hunks)
🔇 Additional comments (5)
apps/dashboard/app/(app)/logs/components/controls/components/logs-search/index.tsx (2)
7-7
: LGTM!The XMark icon import is properly added alongside other icon imports from @unkey/icons.
122-126
: LGTM! Clear search functionality is well implemented.The clear search button is properly implemented with:
- Correct conditional rendering
- Accessible aria-label
- Consistent icon sizing and styling
apps/dashboard/app/(app)/logs/components/table/log-details/components/log-footer.tsx (1)
87-88
: LGTM! Improved layout for permissions badges.The flex layout with wrap and gap provides better handling of multiple permissions, and the justify-end alignment maintains consistency with the design pattern.
Also applies to: 98-98
apps/dashboard/components/timestamp-info.tsx (1)
84-84
: LGTM! Clean class name management.Good use of the
cn
utility function for managing conditional class names. This approach is more maintainable and consistent with best practices.apps/dashboard/app/(app)/logs/components/control-cloud/index.tsx (1)
102-107
: LGTM! Better separation of concerns.Good refactoring to let
TimestampInfo
handle the formatting internally instead of pre-formatting the value. This improves component encapsulation and maintainability.
… into feat/logs-v2-data-fetching
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm build
pnpm fmt
console.logs
git pull origin main
Appreciated
Summary by CodeRabbit
New Features
Refactor
Chores