Skip to content

feat(knowledge/designer): Adding knowledge hub configuration support in agent loop actions#8977

Open
preetriti1 wants to merge 5 commits intomainfrom
priti/knowledgedesigner
Open

feat(knowledge/designer): Adding knowledge hub configuration support in agent loop actions#8977
preetriti1 wants to merge 5 commits intomainfrom
priti/knowledgedesigner

Conversation

@preetriti1
Copy link
Copy Markdown
Contributor

@preetriti1 preetriti1 commented Mar 27, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Refactored existing create connection and upload file panels for knowledge to be reused in modal dialogs.
Added knowledge hub editor in designer parameters ui for agent loop for users to configure knowledge hub in their action.
This also marks as an entry point for users to configure their knowledge base and create knowledge hub connection.
No edit support will be present in the designer.

Impact of Change

  • Users: UI adds Knowledge Hub editor in action parameters; users can create connection + upload artifacts from designer; note that "no edit support" is currently listed.
  • Developers: New designer service method uploadFileArtifact; new modal/dialog components under libs/designer; state slice changes (modal state) and new initializer for resourceService — mention any required consumers to initialize resource service.
  • System: Calls to ARM management endpoints for artifact upload; ensure token handling and CORS/security are reviewed

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:
    Tests will be added in next iteration

Contributors

@bonicaayala @divyaswarnkar

Screenshots/Videos

designer

Copilot AI review requested due to automatic review settings March 27, 2026 22:52
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: feat(knowledge/designer): Adding knowledge hub configuration support in agent loop actions
  • Issue: None — the title follows conventional commit style, is scoped and descriptive.
  • Recommendation: (Optional) If you want to be more explicit, you can include a short note that this adds the Knowledge Hub editor + upload support for artifacts (example: feat(knowledge/designer): Add Knowledge Hub editor and artifact upload for agent-loop actions).

Commit Type

  • Properly selected (feature).
  • Only one box selected in the PR body which is correct. Good.

Risk Level

  • The PR body marks this as High and the repository labels include risk:high.
  • Assessment: matches and is appropriate given the breadth of the change (new editor UI, upload flow to management API, new services, many test and UI changes). No change to the label required.

What & Why

  • Current: "Refactored existing create connection and upload file panels for knowledge to be reused in modal dialogs... Added knowledge hub editor in designer parameters ui..."
  • Issue: None — this is concise and explains intent.
  • Recommendation: Keep as-is. If you want, add one line calling out that upload artifacts call the ARM management endpoint (so reviewers pay special attention to security/CORS/token handling). Example sentence: "Note: artifact uploads call the ARM management endpoint and will use the designer's resource/ARM token.".

Impact of Change

  • The PR body includes Users/Developers/System sections describing the impact.
  • Assessment: Good and appropriately scoped. The body correctly calls out new service methods and UI changes.

⚠️ Test Plan

  • Current claim: Unit tests added/updated and Manual testing completed; E2E tests not added.
  • Assessment: Unit tests are present in the diff (many new test files were added). Manual testing note is fine. E2E tests are not present — acceptable for UI features but consider adding at least an integration/E2E that covers the upload flow in a subsequent PR.
  • Recommendation: Update the Test Plan text in the PR body to remove the line "Tests will be added in next iteration" (it contradicts the added unit tests). Also add a short note about future E2E coverage for the upload flow and security tests around token handling and CORS.

⚠️ Contributors

  • Current: @bonicaayala @divyaswarnkar listed in the PR body.
  • Assessment: Present. Good practice to credit contributors. No blocking issues.

⚠️ Screenshots/Videos

  • Current: Screenshot included in PR body.
  • Assessment: OK — visual change has a screenshot.

Notes from code diff & action items (important)

I reviewed the code diff and tests. The overall changes are large (2885 additions, 829 deletions across 42 files) and touch UI, services, and the upload path to ARM. The submitted risk level (High) is appropriate. A few actionable items and potential issues I recommend addressing before merging:

  1. WorkflowAndArtifacts.uploadFileToKnowledgeHub references environment.armToken but I didn't see an environment import in that file in the diff. Please verify that environment.armToken is available and not accidentally left unresolved. If authentication should come from the resourceService or host service, use the appropriate token provider instead of a global environment value.

    • Recommendation: Use the centralized resource/ARM token provider (e.g., BaseResourceService or the host service) and avoid relying on a global environment variable for tokens in runtime code.
  2. Authorization header usage in client-side code:

    • The new upload implementation sets an Authorization header directly on an XHR request to management.azure.com. Ensure this is the intended flow and that token acquisition and scope are correct. Confirm the token is short-lived and retrieved via a secure path (not baked into a static config) and that CORS and the management endpoint will accept the request flow from the client.
    • Recommendation: If possible, route uploads via a backend/host runtime or use an SDK/token acquisition flow (resourceService.getAccessToken) rather than exposing/storing tokens in client code.
  3. Wiring uploadFileArtifact between services and client usage:

    • You added uploadFileArtifact to the IWorkflowService interface and also provided uploadFileToKnowledgeHub in WorkflowAndArtifacts and wired uploadFileArtifact in getDesignerServices. Ensure that WorkflowService() (the consumer in KnowledgeHubEditor) resolves to the same service container or has the newly-added uploadFileArtifact available at runtime. The tests mock WorkflowService().uploadFileArtifact, but runtime wiring should be validated end-to-end.
    • Recommendation: Add a brief comment in code that documents how uploadFileArtifact is provided to WorkflowService and add an integration/smoke test verifying the KnowledgeHubEditor upload path uses the provided implementation.
  4. FileReader + base64 path and memory usage:

    • The upload converts files to base64 in memory before sending; the upload limit is enforced at UI (16MB). Converting to base64 increases payload size (~33%) and memory usage. Validate the limit and UX for larger files.
    • Recommendation: Document the size limit and the reason for base64 path. Consider streaming multi-part upload if larger files are ever needed.
  5. Tests & test plan text consistency:

    • The PR body said "Unit tests added/updated" (true) but also included "Tests will be added in next iteration". Update the PR body to remove confusing note and mention where tests live (e.g., libs/designer/src/lib/ui/knowledge/editor/test, libs/designer...files test, etc.).
    • Recommendation: Add a short line in the Test Plan listing the location of the new unit tests and that E2E coverage will be added in a follow-up for upload/ARM flows.
  6. Security review / CORS / ARM management endpoints:

    • The PR body already mentions to ensure token handling and CORS/security are reviewed. Please ensure security review is completed and noted in the PR (who reviewed / what was checked).
    • Recommendation: Add a short checklist or a reviewer tag for security (e.g., @security-team) or mention the responsible reviewer in the PR body.
  7. Minor: PR body Test Plan checks the Unit test box — good. Consider adding at least one integration/E2E test for the upload flow in a follow-up PR (since it touches external management APIs).


Summary Table

Section Status Recommendation
Title Title is fine; optionally make it slightly more explicit about upload support.
Commit Type Correct (feature).
Risk Level High — label matches body and is appropriate.
What & Why Good; consider adding one-line note about ARM uploads.
Impact of Change Good detail provided.
Test Plan Unit tests exist; update PR body to remove contradictory text and list test locations; plan E2E for upload.
Contributors Good.
Screenshots/Videos Screenshot included.

Final message: This PR passes the PR title/body checklist. The advised risk level is risk:high which matches your selection and is appropriate for the scope of changes. Before merge, please address the runtime/token wiring concerns outlined above (environment token usage, Authorization header in client, and ensuring WorkflowService.uploadFileArtifact is available at runtime) and update the PR Test Plan text to reflect the unit tests you added and the plan for E2E/integration coverage. Thank you for the detailed PR and added tests — great work!


Last updated: Sat, 28 Mar 2026 06:47:57 GMT

@preetriti1 preetriti1 added the risk:high High risk change requiring careful review label Mar 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Knowledge Hub configuration to the Agent Loop action parameter UI by introducing a custom “knowledgebase” editor and refactoring existing Knowledge Hub connection/file upload UX to be reusable in both panels and modal dialogs.

Changes:

  • Introduces knowledgeBaseName parameter in the Agent Loop manifest using a new knowledgebase editor.
  • Refactors Knowledge Hub “add files” experience into reusable components and adds modal-based entry points (connection + file upload).
  • Extends designer service initialization and workflow services to support ResourceService usage and knowledge artifact uploads.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
libs/logic-apps-shared/src/designer-client-services/lib/workflow.ts Extends IWorkflowService with getLogicAppId and uploadFileArtifact for Knowledge Hub integration.
libs/logic-apps-shared/src/designer-client-services/lib/standard/manifest/agentloop.ts Adds knowledgeBaseName parameter using x-ms-editor: knowledgebase.
libs/designer/src/lib/ui/knowledge/wizard/knowledgehub.tsx Adjusts wizard container height.
libs/designer/src/lib/ui/knowledge/panel/styles.ts Updates panel body overflow/height constraints to allow scrolling.
libs/designer/src/lib/ui/knowledge/panel/panelroot.tsx Updates Add Files panel import path after refactor.
libs/designer/src/lib/ui/knowledge/panel/files/uploadfile.tsx New reusable file upload section component (group selection + dropzone + file list).
libs/designer/src/lib/ui/knowledge/panel/files/filelist.tsx New reusable file list table with validation for artifact names.
libs/designer/src/lib/ui/knowledge/panel/files/addfile.tsx New Add Files drawer panel rebuilt to use FileUpload.
libs/designer/src/lib/ui/knowledge/panel/files/test/addfile.spec.tsx Fixes import path after Add Files refactor.
libs/designer/src/lib/ui/knowledge/panel/connection/usepaneltabs.tsx Refactors tab hook to accept selectTab/close callbacks for reuse outside Redux panel.
libs/designer/src/lib/ui/knowledge/panel/connection/tabs/model.tsx Updates model tab to use injected selectTab callback.
libs/designer/src/lib/ui/knowledge/panel/connection/tabs/basics.tsx Updates basics tab to use injected close callback.
libs/designer/src/lib/ui/knowledge/panel/connection/create.tsx Adapts create connection drawer panel to new hook signature (passes callbacks).
libs/designer/src/lib/ui/knowledge/panel/addfile.tsx Removes legacy monolithic Add Files panel implementation (moved into /files).
libs/designer/src/lib/ui/knowledge/modals/creategroup.tsx Updates create-group dialog configuration (modalType).
libs/designer/src/lib/ui/knowledge/editor/styles.ts Adds styling for the new Knowledge Hub parameter editor and connection modal content.
libs/designer/src/lib/ui/knowledge/editor/index.tsx Adds KnowledgeHubEditor custom parameter editor (connection + hub selection + artifacts + upload modal).
libs/designer/src/lib/ui/knowledge/editor/files.tsx Adds AddFilesModal dialog wrapper around the reusable FileUpload component.
libs/designer/src/lib/ui/knowledge/editor/connection.tsx Adds CreateConnectionModal dialog wrapper around existing connection panel tabs.
libs/designer/src/lib/ui/DesignerDialog.tsx Adds a designer-level dialog host for the knowledge connection modal.
libs/designer/src/lib/ui/Designer.tsx Mounts DesignerDialog so knowledge modals can portal correctly.
libs/designer/src/lib/core/utils/parameters/helper.ts Wires new knowledgebase editor to the parameter rendering system.
libs/designer/src/lib/core/state/modal/modalSlice.ts Adds Redux state/actions to open/close the knowledge connection modal.
libs/designer/src/lib/core/state/designerOptions/designerOptionsSlice.ts Initializes resourceService to support Knowledge Hub queries.
libs/designer/src/lib/core/state/designerOptions/designerOptionsInterfaces.ts Adds resourceService to service initialization options.
libs/designer/src/lib/common/constants.ts Adds EDITOR.KNOWLEDGE_BASE.
libs/designer-ui/src/lib/constants.ts Adds PARAMETER.EDITOR.KNOWLEDGE_BASE to shared designer-ui constants.
apps/Standalone/src/knowledge/app/KnowledgeHub.tsx Switches Knowledge Hub upload implementation to shared uploadFileToKnowledgeHub.
apps/Standalone/src/designer/app/AzureLogicAppsDesigner/laDesigner.tsx Implements getLogicAppId/uploadFileArtifact on workflow service and initializes BaseResourceService.
apps/Standalone/src/designer/app/AzureLogicAppsDesigner/Services/customConnectionParameterEditorService.tsx Adjusts editor override logic for agent connection parameters based on parameterKey.
apps/Standalone/src/designer/app/AzureLogicAppsDesigner/Services/WorkflowAndArtifacts.tsx Adds exported uploadFileToKnowledgeHub implementation used by workflow service + standalone knowledge app.
Localize/lang/strings.json Adds new localized strings for the knowledge base editor and modals.

@ecfan ecfan self-assigned this Mar 27, 2026
@ecfan ecfan self-requested a review March 27, 2026 23:09
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

📊 Coverage Check

The following changed files need attention:

⚠️ libs/designer/src/lib/ui/knowledge/panel/files/addfile.tsx - 73% covered (needs improvement)
⚠️ libs/designer/src/lib/core/state/designerOptions/designerOptionsSlice.ts - 31% covered (needs improvement)
⚠️ libs/designer/src/lib/core/utils/parameters/helper.ts - 41% covered (needs improvement)
⚠️ libs/designer/src/lib/ui/Designer.tsx - 75% covered (needs improvement)

Please add tests for the uncovered files before merging.

@preetriti1 preetriti1 enabled auto-merge (squash) March 28, 2026 04:05

expect(screen.getByText('Knowledge base')).toBeInTheDocument();
expect(
screen.getByText('Create a connection and add knowledge hub sources your agent will use to generate responses.')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`'Set up your database and model connections. Add knowledge base sources that your agent uses to generate responses.'


renderComponent();

expect(screen.getByText('Please create a connection to add knowledge hubs.')).toBeInTheDocument();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where exactly does this text appear? I didn't see it in the video. I need better context for more accurate text. Right now, I can only suggest the following, which seems ambiguous to me.

'Create a connection to add knowledge bases.'


renderComponent();

expect(screen.getByText('Select a knowledge hub')).toBeInTheDocument();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Select a knowledge base.'

fireEvent.click(dropdown);

await waitFor(() => {
expect(screen.getByText('No hub artifacts found. You can create hub and upload files to get started.')).toBeInTheDocument();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Can't find knowledge base artifacts. Create a knowledge base and upload files to get started.'

description: 'Text for learn more link',
}),
emptyArtifacts: intl.formatMessage({
defaultMessage: 'No hub artifacts found. You can create hub and upload files to get started.',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Can't find knowledge base artifacts. Create a knowledge base and upload files to get started.'`

description: 'Text to indicate that there are no artifacts in the knowledge hub',
}),
noConnectionMessage: intl.formatMessage({
defaultMessage: 'Please create a connection to add knowledge hubs.',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question. Create what kind of connection? I need more context.

'Create a connection to add knowledge hubs.'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a knowledge hub connection not the action connection. Also @ecfan, I can address the content changes in next PR.. need to get this checked in for the release fork which is waiting on this pr for vendor test pass. I am not sending any more iterations in this PR for a quick checkin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:high High risk change requiring careful review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants