refactor(knowledge): add toaster notifications, centralized error handling, and file upload hooks#9024
Open
preetriti1 wants to merge 1 commit intomainfrom
Open
refactor(knowledge): add toaster notifications, centralized error handling, and file upload hooks#9024preetriti1 wants to merge 1 commit intomainfrom
preetriti1 wants to merge 1 commit intomainfrom
Conversation
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
✅ Risk Level
✅ What & Why
✅ Impact of Change
✅ Test Plan
✅ Contributors
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | No change needed. |
| Commit Type | ✅ | Correctly set to refactor. |
| Risk Level | ✅ | risk:medium is appropriate. |
| What & Why | ✅ | Expand to name key breaking/behavior changes (createOrUpdateConnection signature, new optionsSlice, useFileHooks) briefly. |
| Impact of Change | ✅ | Call out update to root reducer/consumers, LoggerService usage, and localization string changes. |
| Test Plan | ✅ | Unit tests present. Consider adding or explaining lack of E2E tests. |
| Contributors | ✅ | Contributor listed. Consider adding any additional reviewers/designers. |
| Screenshots/Videos | Optional but recommended for UI toasters and upload flows. |
Final Message
Overall this PR is well-structured and follows the repo PR template. It passes the PR title/body checks.
Please consider the small improvements suggested above (expand What & Why, call out the signature and state changes clearly, and either add an E2E test or a short justification for not including one). Also run the full CI and ensure LoggerService wiring and optionsSlice consumption behave correctly in staging. Thank you for the thorough tests and the clear commit.
Last updated: Tue, 07 Apr 2026 17:37:49 GMT
preetriti1
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Knowledge Hub UX in the designer by adding explicit success/failure notifications (toasts and inline error bars) for connection create/update and file upload flows, and refactors duplicated “add file” logic into a shared hook used by both the drawer panel and modal.
Changes:
- Added notification plumbing via Redux (
optionsSlice.notification) and a reusableToasterNotificationcomponent. - Added error handling + user-visible error UI for connection create/update, plus success notifications for create/update/upload/group creation.
- Refactored add-file flows to reuse shared logic (
useFileHooks) and updated/added unit tests accordingly.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Localize/lang/strings.json | Adds/removes localized strings for new success/failure messages and updated empty-state copy. |
| libs/designer/src/lib/ui/knowledge/wizard/knowledgelist.tsx | Memoizes intl text/columns and dispatches notifications on delete completion. |
| libs/designer/src/lib/ui/knowledge/wizard/knowledgehub.tsx | Shows toaster notifications, wires notification state, and adds toast on group creation/deletion. |
| libs/designer/src/lib/ui/knowledge/wizard/test/knowledgelist.spec.tsx | Updates tests to include Redux provider for new dispatch usage. |
| libs/designer/src/lib/ui/knowledge/panel/files/useFileHooks.tsx | New shared hook for upload footer state, validation, upload handling, and notifications. |
| libs/designer/src/lib/ui/knowledge/panel/files/uploadfile.tsx | Adds success notification on group creation; memoizes intl text. |
| libs/designer/src/lib/ui/knowledge/panel/files/filelist.tsx | Memoizes intl text and column definitions to reduce rerenders. |
| libs/designer/src/lib/ui/knowledge/panel/files/addfile.tsx | Refactors drawer panel to use useFileHooks and dispatch toast notifications. |
| libs/designer/src/lib/ui/knowledge/panel/connection/usepaneltabs.tsx | Adds onCreate/onError callbacks and reports create failures to UI. |
| libs/designer/src/lib/ui/knowledge/panel/connection/tabs/model.tsx | Memoizes intl text. |
| libs/designer/src/lib/ui/knowledge/panel/connection/tabs/basics.tsx | Memoizes intl text. |
| libs/designer/src/lib/ui/knowledge/panel/connection/edit.tsx | Adds inline error MessageBar + success toast; logs via updated core util signature. |
| libs/designer/src/lib/ui/knowledge/panel/connection/create.tsx | Adds inline error MessageBar + success toast via callbacks. |
| libs/designer/src/lib/ui/knowledge/panel/connection/test/usepaneltabs.spec.tsx | Updates hook tests to assert onError behavior instead of console.error. |
| libs/designer/src/lib/ui/knowledge/panel/connection/test/edit.spec.tsx | Updates tests to validate inline error MessageBar rendering. |
| libs/designer/src/lib/ui/knowledge/panel/connection/test/create.spec.tsx | Adds tests for error MessageBar rendering via captured onError. |
| libs/designer/src/lib/ui/knowledge/notification.tsx | New reusable Fluent UI toaster wrapper component with auto-clear behavior. |
| libs/designer/src/lib/ui/knowledge/modals/delete.tsx | Memoizes derived values and intl text; keeps existing delete behavior + notification payload. |
| libs/designer/src/lib/ui/knowledge/modals/creategroup.tsx | Memoizes intl text. |
| libs/designer/src/lib/ui/knowledge/editor/index.tsx | Updates empty-state copy to new localized strings; memoizes intl text. |
| libs/designer/src/lib/ui/knowledge/editor/files.tsx | Refactors modal upload flow to reuse useFileHooks and designer-ui footer. |
| libs/designer/src/lib/ui/knowledge/editor/connection.tsx | Adds inline error MessageBar and wires onError into create-connection tabs. |
| libs/designer/src/lib/ui/knowledge/editor/test/index.spec.tsx | Updates assertions for changed empty-state strings. |
| libs/designer/src/lib/ui/knowledge/editor/test/files.spec.tsx | Updates cancel-button behavior expectation during upload. |
| libs/designer/src/lib/ui/knowledge/editor/test/connection.spec.tsx | Adds tests for error MessageBar rendering via captured onError. |
| libs/designer/src/lib/ui/knowledge/test/notification.spec.tsx | New unit tests for ToasterNotification dispatch behavior and onClear. |
| libs/designer/src/lib/core/state/knowledge/optionsSlice.ts | Adds notification field and actions to set/clear notifications. |
| libs/designer/src/lib/core/knowledge/utils/connection.ts | Adds logging + isCreate flag to distinguish create vs update failures. |
| libs/designer/src/lib/core/knowledge/utils/test/connection.spec.ts | Updates mocks for newly used LoggerService/LogEntryLevel and typing in intl mock. |
Comment on lines
1
to
9
| import { createSlice } from '@reduxjs/toolkit'; | ||
| import { initializeData } from '../../actions/bjsworkflow/knowledge'; | ||
| import type { ServerNotificationData } from '../../../ui/mcp/servers/servers'; | ||
|
|
||
| export interface OptionsState { | ||
| servicesInitialized: boolean; | ||
| isDarkMode?: boolean; | ||
| notification?: ServerNotificationData; | ||
| } |
There was a problem hiding this comment.
OptionsState now depends on ServerNotificationData imported from a UI component module (ui/mcp/servers/servers.tsx). This introduces an undesirable core->UI dependency (and can drag React/UI code into core state builds). Consider moving ServerNotificationData to a shared types module (e.g., core/state/knowledge/types) or defining a local { title: string; content: string } type in the slice and importing it from there in the UI.
| intl.formatMessage( | ||
| { | ||
| id: '+nKZHB', | ||
| defaultMessage: `Can't upload file. Please try again. Error: {errorMessage}`, |
There was a problem hiding this comment.
defaultMessage is using a template literal even though it has no JS interpolation. This tends to violate the repo's Biome rule (prefer normal string literals when no interpolation) and can create noisy diffs in localization extraction. Use a normal quoted string for this message.
Suggested change
| defaultMessage: `Can't upload file. Please try again. Error: {errorMessage}`, | |
| defaultMessage: "Can't upload file. Please try again. Error: {errorMessage}", |
| }), | ||
| addFilesSectionDescription: intl.formatMessage({ | ||
| id: 'ifDZq4', | ||
| defaultMessage: `Files are added to the specified group. Each file can't exceed 16 MB. Total upload can't exceed 100 MB.`, |
There was a problem hiding this comment.
defaultMessage is a template literal without interpolation. To align with repo linting/formatting conventions (Biome) and avoid unnecessary template strings in localization messages, change this to a normal quoted string.
Suggested change
| defaultMessage: `Files are added to the specified group. Each file can't exceed 16 MB. Total upload can't exceed 100 MB.`, | |
| defaultMessage: "Files are added to the specified group. Each file can't exceed 16 MB. Total upload can't exceed 100 MB.", |
Comment on lines
+64
to
+74
| defaultMessage: `Confirm that you want to delete these hub artifacts? Deleting a hub deletes all its artifacts. You can't undo this action.`, | ||
| id: 'uYxnwQ', | ||
| description: 'Content for the delete hub artifacts modal', | ||
| }), | ||
| hubContent: intl.formatMessage({ | ||
| defaultMessage: `Confirm that you want to delete this hub? This action also deletes all the hub's artifacts. You can't undo this action.`, | ||
| id: 'Xjhrkz', | ||
| description: 'Content for the delete hub', | ||
| }), | ||
| artifactContent: intl.formatMessage({ | ||
| defaultMessage: `Confirm that you want to delete this artifact? You can't undo this action.`, |
There was a problem hiding this comment.
These defaultMessage values are template literals without interpolation. This is inconsistent with the repo's Biome convention (use normal string literals when there are no substitutions) and can trigger lint/format churn. Convert them to normal quoted strings.
Suggested change
| defaultMessage: `Confirm that you want to delete these hub artifacts? Deleting a hub deletes all its artifacts. You can't undo this action.`, | |
| id: 'uYxnwQ', | |
| description: 'Content for the delete hub artifacts modal', | |
| }), | |
| hubContent: intl.formatMessage({ | |
| defaultMessage: `Confirm that you want to delete this hub? This action also deletes all the hub's artifacts. You can't undo this action.`, | |
| id: 'Xjhrkz', | |
| description: 'Content for the delete hub', | |
| }), | |
| artifactContent: intl.formatMessage({ | |
| defaultMessage: `Confirm that you want to delete this artifact? You can't undo this action.`, | |
| defaultMessage: "Confirm that you want to delete these hub artifacts? Deleting a hub deletes all its artifacts. You can't undo this action.", | |
| id: 'uYxnwQ', | |
| description: 'Content for the delete hub artifacts modal', | |
| }), | |
| hubContent: intl.formatMessage({ | |
| defaultMessage: "Confirm that you want to delete this hub? This action also deletes all the hub's artifacts. You can't undo this action.", | |
| id: 'Xjhrkz', | |
| description: 'Content for the delete hub', | |
| }), | |
| artifactContent: intl.formatMessage({ | |
| defaultMessage: "Confirm that you want to delete this artifact? You can't undo this action.", |
| description: 'Text for learn more link', | ||
| }), | ||
| emptyArtifacts: intl.formatMessage({ | ||
| defaultMessage: `Can't find knowledge base artifacts. Create a knowledge base and upload files to get started.`, |
There was a problem hiding this comment.
defaultMessage is a template literal without interpolation. To match repo linting/formatting conventions and keep localization strings consistent, switch this to a normal quoted string.
Suggested change
| defaultMessage: `Can't find knowledge base artifacts. Create a knowledge base and upload files to get started.`, | |
| defaultMessage: "Can't find knowledge base artifacts. Create a knowledge base and upload files to get started.", |
rllyy97
approved these changes
Apr 7, 2026
📊 Coverage CheckThe following changed files need attention:
Please add tests for the uncovered files before merging. |
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
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.
…rd and designer
Commit Type
Risk Level
What & Why
Added error handling and success / failure notification toasters during create calls.
Refactoring to reuse the code between add file from dialog and drawer
Impact of Change
Test Plan
Contributors
@divyaswarnkar
Screenshots/Videos