Redesign the widget component#28958
Conversation
Rohit0301
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| {addTagButton ?? ( | ||
| <WidgetEditButton | ||
| data-testid="edit-button" | ||
| title={t('label.edit-entit', { |
There was a problem hiding this comment.
⚠️ Bug: Typo in i18n key 'label.edit-entit' breaks edit tooltip
In TagsContainerV2.tsx the new headerExtra uses t('label.edit-entit', { entity: ... }) for the edit button title. The key label.edit-entit does not exist in the locale files (the correct key is label.edit-entity, present in en-us.json as "Edit {{entity}}"). As a result the edit button tooltip will render the raw missing key / fallback instead of the translated "Edit Tags"/"Edit Glossary Term" text. The pre-refactor code used the correct label.edit-entity key. Fix the typo.
Use the correct existing translation key 'label.edit-entity'.:
title={t('label.edit-entity', {
entity:
tagType === TagSource.Classification
? t('label.tag-plural')
: t('label.glossary-term'),
})}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| const handleExpandClick = useCallback(() => { | ||
| setIsExpanded((prev) => { | ||
| onExpandStateChange?.(prev); | ||
|
|
||
| return !prev; | ||
| }); | ||
| }, [onExpandStateChange]); |
There was a problem hiding this comment.
💡 Bug: Side effect inside setState updater in WidgetCard
WidgetCard.handleExpandClick calls onExpandStateChange?.(prev) inside the setIsExpanded updater function. State updater functions must be pure; in React StrictMode (dev) updaters are intentionally invoked twice, which would fire onExpandStateChange twice. Additionally, it passes prev (the value before the toggle) rather than the resulting expanded state, so consumers receive the stale/opposite value of what they likely expect from a callback named onExpandStateChange. Move the callback out of the updater and pass the new state.
Fix 1: Compute the next value, report it (not the previous value), to the callback. Note this still runs inside the updater; for full purity, derive next from current state outside setState.
const handleExpandClick = useCallback(() => {
setIsExpanded((prev) => {
const next = !prev;
onExpandStateChange?.(next);
return next;
});
}, [onExpandStateChange]);
- Apply fix
Fix 2: Keep the updater pure by toggling based on current state and invoking the callback as a separate effect of the click handler.
const handleExpandClick = useCallback(() => {
const next = !isExpanded;
setIsExpanded(next);
onExpandStateChange?.(next);
}, [isExpanded, onExpandStateChange]);
- Apply fix
Check a box to apply a fix or reply for a change | Was this helpful? React with 👍 / 👎
| const AttachmentWidget: FC<AttachmentWidgetProps> = ({ hasPermission }) => { | ||
| const { t } = useTranslation(); | ||
|
|
||
| const attachments: AttachmentItem[] = []; | ||
|
|
||
| const handleCopy = (item: AttachmentItem) => { | ||
| navigator.clipboard.writeText(item.downloadUrl ?? item.name); | ||
| }; | ||
|
|
||
| const handleDownload = (item: AttachmentItem) => { | ||
| if (item.downloadUrl) { | ||
| window.open(item.downloadUrl, '_blank'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
⚠️ Quality: AttachmentWidget renders hardcoded empty list; props/handlers dead
In AttachmentWidget.tsx, attachments is hardcoded to an empty array (const attachments: AttachmentItem[] = [];, line 34). As a result the widget always renders the "no attachments" message, the .map() branch, handleCopy, and handleDownload are unreachable dead code, and the hasPermission prop is never read. If this is an intentional WIP scaffold, please gate it behind a ticket reference; otherwise wire attachments to a real data source. As written, the entire interactive part of the component (copy/download) can never execute, and isExpandDisabled={attachments.length === 0} is always true.
Was this helpful? React with 👍 / 👎
| const AttachmentWidget: FC<AttachmentWidgetProps> = ({ hasPermission }) => { | ||
| const { t } = useTranslation(); | ||
|
|
||
| const attachments: AttachmentItem[] = []; |
There was a problem hiding this comment.
💡 Quality: useMemo deps recreate every render; hasPermission unused
In AttachmentWidget.tsx the content useMemo lists [attachments, hasPermission] as dependencies (line 113), but attachments is a fresh array literal created on every render, so the memo recomputes every time and provides no benefit. Additionally hasPermission is in the dependency list but never referenced inside the memo body. Once the component is wired to real data, derive attachments from a stable source (props/state/query) and either use hasPermission (e.g. to gate the copy/download actions) or remove it from the deps.
Was this helpful? React with 👍 / 👎
| const handleCopy = (item: AttachmentItem) => { | ||
| navigator.clipboard.writeText(item.downloadUrl ?? item.name); | ||
| }; |
There was a problem hiding this comment.
💡 Edge Case: navigator.clipboard.writeText promise unhandled
In handleCopy (AttachmentWidget.tsx line 36-38), navigator.clipboard.writeText(...) returns a Promise that can reject (e.g. permission denied, insecure context, or navigator.clipboard undefined on non-HTTPS origins). The rejection is unhandled, producing an unhandled promise rejection and no user feedback. Consider awaiting it with a try/catch and showing a success/error toast, consistent with the project's showErrorToast utility.
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
🔴 Playwright Results — 15 failure(s), 15 flaky✅ 4254 passed · ❌ 15 failed · 🟡 15 flaky · ⏭️ 88 skipped
Genuine Failures (failed on all attempts)❌
|
1 participant
Describe your changes:
Fixes 28957
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
WidgetCardto standardize UI components and replacedExpandableCardacross multiple features, includingRelatedDataAssets,TagsContainerV2, andOwnerLabelV2.WidgetActionButtonsuite (Edit, Plus, Comment, Request) to provide consistent header interactions withinWidgetCard.AttachmentWidgetforKnowledgePageto manage file attachments, including support for download and copy actions.This will update automatically on new commits.