Draft 1 of slack app page polish, add search, refactor, fix dates#2009
Draft 1 of slack app page polish, add search, refactor, fix dates#2009sarah-inkeep merged 4 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
workspace-default-section.tsx:53-58Non-focusable TooltipTrigger - keyboard users cannot access locked state explanation
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
bulk-select-agent-bar.tsx:52Empty space fragment renders when not loading
💭 Consider (5) 💭
💭 1) alert-dialog.tsx AlertDialogAction variant prop creates split-world
Issue: The new variant prop to AlertDialogAction is a good improvement aligning with buttonVariants, but it creates a split-world where some dialogs use the new variant prop and others use inline className styling.
Why: Existing usages across the codebase (delete-dataset-item-confirmation.tsx, delete-evaluator-confirmation.tsx, etc.) still use className="bg-destructive text-destructive-foreground hover:bg-destructive/90".
Fix: This PR introduces the new pattern correctly. Consider either migrating existing usages in a follow-up PR, or adding a tracking comment noting this as the new convention.
Refs:
- delete-dataset-item-confirmation.tsx:68 — existing inline styling pattern
💭 2) index.tsx:55-58 Channel search/filter clears selections silently
Issue: When a user types in the search field or changes the channel filter, all selected channels are automatically cleared. This could frustrate admins who are trying to bulk-select channels while narrowing down their search.
Why: For example: Admin selects 5 channels, then realizes they want to filter to only private channels — all selections are lost.
Fix: The biome-ignore comment indicates this is intentional, but consider: (1) only clearing selections that are filtered out, preserving valid selections, or (2) maintaining selections across filters and only applying bulk actions to visible+selected channels.
💭 3) channel-agent-cell.tsx:51 "Default" label styling ambiguity
Issue: Channels without a custom agent show "Default" with text-muted-foreground/80 font-light styling, making it appear disabled rather than indicating "using workspace default."
Why: Combined with removing the Bot icon that was previously shown for custom agents, the visual hierarchy between "Default" and "AgentName" is now primarily font-weight/color based.
Fix: Consider making "Default" slightly more prominent, using "Workspace Default" for clarity, or showing the actual default agent name with an inherited indicator.
💭 4) types.ts:5-17 Channel interface allows theoretically illegal states
Issue: The Channel interface allows hasAgentConfig: true with agentConfig: undefined (or vice versa). These fields represent the same concept but are not structurally linked.
Why: In practice, the code maintains these in sync, but the type system doesn't enforce this invariant.
Fix: Consider a discriminated union or deriving hasAgentConfig from agentConfig !== undefined. This is defensive since the current implementation keeps them synchronized.
Refs:
💭 5) channel-defaults-section.tsx Decorative icons should have aria-hidden
Issue: Icons in channel rows (Globe, Lock, Hash) and the section header Hash icon are decorative since the channel name/type badge provides the context, but they lack aria-hidden="true".
Why: Screen reader users may hear redundant icon announcements. Lucide icons include default titles that may be announced.
Fix: Add aria-hidden="true" to decorative icons: <Globe className="..." aria-hidden="true" />.
Refs:
- accessibility-checklist — decorative icons should be hidden from assistive technology
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-executed refactor that breaks down a large monolithic component into maintainable pieces, adds useful search functionality, and applies consistent UI polish using existing badge/button variants. The code quality is solid — no bugs found by the standards reviewer. The main actionable item is the accessibility fix for the TooltipTrigger. The other items are valid considerations but don't block the PR. Nice work on the component decomposition and pattern consolidation (especially using formatDateAgo and semantic badge variants)! 🎉
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
globals.css:11 |
New --text-1sm CSS variable only used once |
INFO level - minor addition following existing naming convention |
index.tsx:55-58 |
Biome lint suppression for exhaustive deps | Documented intentional behavior with clear comment |
app-sidebar.tsx, work-apps-nav.tsx |
Icon change Plug → Blocks | INFO level - positive consistency observation |
linked-users-section.tsx:36 |
Date formatting utility consolidation | INFO level - positive pattern (consolidates duplicate code) |
my-link-status.tsx:55 |
Badge variant usage change | INFO level - positive pattern (using component API correctly) |
workspace-hero.tsx:313 |
DropdownMenuItem variant usage | INFO level - positive pattern (using component API correctly) |
types.ts:1-2 |
Type re-export pattern | INFO level - positive pattern (avoids duplication) |
slack-dashboard.tsx:149 |
Admin Tips section removed | Intentional UI simplification; export functionality remains discoverable in LinkedUsersSection dropdown |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
5 | 0 | 1 | 0 | 2 | 0 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
8 | 0 | 1 | 0 | 0 | 0 | 6 |
pr-review-product |
5 | 0 | 2 | 0 | 0 | 0 | 3 |
pr-review-types |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
| Total | 19 | 0 | 5 | 0 | 2 | 0 | 11 |
Note: Standards reviewer found no issues - the refactor is clean and follows established patterns.
| <Tooltip> | ||
| <TooltipTrigger> | ||
| <Lock className="h-3.5 w-3.5 text-muted-foreground" /> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Only admins can modify workspace defaults</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
🟠 MAJOR: Non-focusable TooltipTrigger for accessibility
Issue: The Lock icon wrapped in TooltipTrigger is not keyboard-focusable. Tooltip triggers must be focusable for keyboard/screen reader users to access the tooltip content.
Why: Keyboard users cannot access the tooltip explaining why workspace defaults are locked. This blocks important information for non-admin users who may be confused about why the control is disabled.
Fix:
| <Tooltip> | |
| <TooltipTrigger> | |
| <Lock className="h-3.5 w-3.5 text-muted-foreground" /> | |
| </TooltipTrigger> | |
| <TooltipContent>Only admins can modify workspace defaults</TooltipContent> | |
| </Tooltip> | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <span tabIndex={0} role="img" aria-label="Locked - admin only"> | |
| <Lock className="h-3.5 w-3.5 text-muted-foreground" /> | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent>Only admins can modify workspace defaults</TooltipContent> | |
| </Tooltip> |
Refs:
- accessibility-checklist — tooltip triggers must be keyboard-focusable
| className="h-7 text-muted-foreground" | ||
| disabled={bulkSaving} | ||
| > | ||
| {bulkSaving ? <Loader2 className="h-3 w-3 animate-spin" /> : <> </>} |
There was a problem hiding this comment.
🟡 Minor: Empty space fragment renders when not loading
Issue: An empty fragment with a space is rendered when bulkSaving is false. This is unnecessary and may cause inconsistent spacing.
Why: Clean code - the empty fragment serves no purpose and adds visual noise to the code.
Fix:
| {bulkSaving ? <Loader2 className="h-3 w-3 animate-spin" /> : <> </>} | |
| {bulkSaving && <Loader2 className="h-3 w-3 animate-spin" />} |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to the 3 commits since the last automated review:
44e2d904Address claude changes, layout fixesc0cb2b85Fix table header hovera4ff65c2Tweak for dark mode
✅ Prior Feedback Status
All feedback from the previous review has been addressed:
| Prior Issue | Resolution |
|---|---|
| 🟠 Major: Non-focusable TooltipTrigger (Lock icon) | ✅ Resolved by removing the Lock icon entirely — disabled state now communicated via bg-muted/50 input styling |
| 🟡 Minor: Empty space fragment | ✅ Fixed with conditional rendering: {bulkSaving && <Loader2 />} |
| 💭 Consider: Decorative icons aria-hidden | ✅ Added aria-hidden="true" to Globe/Lock/Hash icons |
| 💭 Consider: AlertDialogAction variant | ✅ Migrated 5 delete confirmations to variant="destructive" |
Delta Changes Summary
The delta commits clean up prior feedback and add polish:
-
Layout restructuring — Cards now rendered as siblings via Fragment instead of sections within a single Card with Collapsible. This provides better visual hierarchy.
-
Sticky table header refinement — Moved
bg-cardand shadow styling fromTableHeadertoTableRowfor proper dark mode support. The patternshadow-[0_1px_0_0_var(--border)]correctly uses the CSS variable which is defined in both light and dark themes. -
Consistency improvements — Button sizing standardized to
size="icon-sm", Card styling usesshadow-noneconsistently with other work-apps features.
✅ APPROVE
Summary: Excellent follow-up commits that address all prior review feedback cleanly. The accessibility concern with the Lock icon was resolved by removing it (the disabled input styling now communicates the constraint). The component refactoring is well-executed, the AlertDialogAction variant migration is complete and consistent, and the dark mode fixes are properly implemented. Ship it! 🚀
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
channel-defaults-section.tsx:200 |
Sticky header CSS shadow may not work in all themes | Not valid — --border is defined in both light/dark themes in globals.css |
workspace-default-section.tsx:111 |
Non-editable state lacks explanation text | Developer preference — accessibility (removing non-focusable tooltip) was prioritized over explanatory UX |
index.tsx:366 |
Fragment return pattern changes component structure | Informational only — intentional design change, no action needed |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 3 | 0 | 0 | 0 | 0 | 0 | 3 |
Note: All reviewers confirmed prior feedback was addressed. Consistency reviewer verified AlertDialogAction migration is complete across all 5 delete confirmation dialogs.
No description provided.