Skip to content

Refactor/foundry service use httpclient#8880

Closed
ccastrotrejo wants to merge 9 commits intomainfrom
refactor/foundry-service-use-httpclient
Closed

Refactor/foundry service use httpclient#8880
ccastrotrejo wants to merge 9 commits intomainfrom
refactor/foundry-service-use-httpclient

Conversation

@ccastrotrejo
Copy link
Copy Markdown
Contributor

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

Impact of Change

  • Users:
  • Developers:
  • System:

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

Screenshots/Videos

ccastrotrejo and others added 3 commits March 4, 2026 14:05
…entService

Replace the custom foundryRequest wrapper (raw fetch + AbortController) with
the shared IHttpClient abstraction used by all other services. This gives
Foundry API calls consistent retry (axios-retry), error handling, and
request patterns.

- Add getHttpClient() to ICognitiveServiceService and BaseCognitiveServiceService
- Refactor all foundryAgentService functions to accept IHttpClient as first param
- Use httpClient.get/post with noAuth:true and custom Bearer headers
- Update callers in designer and designer-v2 (useCognitiveService, foundryUpdates)
- Update all tests to mock IHttpClient instead of globalThis.fetch

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the Tools label, summary text, toolsList style, and related tests
from the Foundry agent inline details component.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… dropdown

- Remove portal link rendering from FoundryAgentDetails component
- Add FoundryPortalLink component in parametersTab (designer + designer-v2)
- Change label to lowercase "Edit in foundry portal"
- Fix duplicate NavigateIcon export; re-export useFoundryAgentDetailsStyles
- Update tests to cover buildFoundryPortalUrl instead of removed UI

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 4, 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: Refactor/foundry service use httpclient
  • Issue: The title is terse, uses a slash instead of conventional punctuation/casing, and doesn't convey the scope clearly. It should be an imperative, concise description and indicate the main intent (e.g., what was changed and why).
  • Recommendation: Use a clear imperative-style title that describes the primary change and scope. Example: Refactor: Use shared IHttpClient for Foundry service and add Foundry agent versioning support

Commit Type

  • Commit type not selected in the PR body template.
  • Note: The changes are cross-cutting (API surface, services, UI, tests). The correct commit type appears to be refactor (or feature if adding significant new functionality like version selection UI). Please select exactly one in the PR's Commit Type section.
  • Recommendation: mark - [x] refactor - Code restructuring without behavior change (if behavior is preserved) or - [x] feature - New functionality if the agent-version selection and auto-selection behavior constitute new functional behavior.

Risk Level

  • Assessment: No risk level selected in the PR body and no risk:* label present on the PR. From the diff, this change touches many core modules (libs/designer, libs/designer-v2, designer-ui, logic-apps-shared services), modifies the Foundry update flow, introduces a shared IHttpClient use, adds new state handling for version-refresh, and updates tests and UI behavior. This is a broad, cross-cutting change with potential runtime impact.
  • Advised risk level: High — please add the matching label (e.g., risk:high) to the PR and select High in the PR body.
  • Note: If you believe this should be Medium, include a short justification in the PR body explaining why (for example: thorough unit test coverage + limited runtime surface). Given the number of files and behavioral changes, I recommend High.

What & Why

  • Current: (Missing)
  • Issue: The PR body does not include a summary explaining what changed and why. Reviewers need a short context paragraph to understand intent, especially for cross-cutting refactors.
  • Recommendation: Add a brief What & Why section. Example to paste:
    • What: "Refactor Foundry service to use the project's shared IHttpClient. Add support for fetching and displaying Foundry agent versions in the UI, persist selected version number to workflow parameters, auto-select new version after a Foundry save, and update the Foundry update flush logic to mark flushed nodes for version refresh. Update unit tests to cover new behaviors."
    • Why: "Unify HTTP usage for consistent retry/auth behavior, enable agent version selection and auto-sync with Foundry after updates, and reduce brittle fetch() usage across codebase."

⚠️ Impact of Change

  • Issue: Impact section is blank. This change affects UI, core services, and data flow; you must communicate who/what is impacted and any migration notes.
  • Recommendation: Fill the Impact of Change section with concise bullet points referencing the code areas changed. Suggested content:
    • Users: Adds version dropdown for Foundry agents; auto-selects newly-created versions after saving; adds portal link that can include a version query param.
    • Developers: Replaces direct fetch calls with IHttpClient across Foundry client code; API for Foundry client functions now expects an IHttpClient parameter in many places (update imports/usages accordingly); new exported helpers (consumeVersionRefresh / needsVersionRefresh) added to core action module.
    • System: Introduces calls to new httpClient-based endpoints and changes behavior of flushPendingFoundryUpdates (it now takes an optional onFlushed callback and marks flushed nodes). Ensure runtime environment provides the httpClient and getFoundryAccessToken as before; VS Code / non-browser envs may need to stub/provide httpClient.

Test Plan

  • Assessment: The diff includes many test updates and new tests (see updated/added spec files in libs/designer-ui, libs/designer-v2, libs/designer, and libs/logic-apps-shared). Unit tests are present and updated to mock the new IHttpClient shape.

  • Recommendation: In the PR body Test Plan, check:

    • - [x] Unit tests added/updated (there are many changed tests)
    • - [ ] E2E tests added/updated (if not applicable, leave unchecked and explain)
    • - [x] Manual testing completed — add a short summary of manual steps you ran (e.g., saved a workflow, verified version auto-selection, verified portal link building, verified no regressions in Foundry update flow).

    Also include a brief list of the test files changed (for reviewer convenience), e.g.: libs/designer-ui/.../foundryAgentDetails.spec.tsx, libs/designer-v2/.../foundryUpdates.spec.ts, libs/logic-apps-shared/.../foundryAgentService.spec.ts


⚠️ Contributors

  • Assessment: Contributors section is blank.
  • Recommendation: If others (PMs, designers, or reviewers) contributed, mention them. If not, you can leave blank; include a short reminder to credit collaborators before merge.

⚠️ Screenshots/Videos

  • Assessment: N/A — No visual asset included. This appears to add UI (version dropdown and portal link). If the visual change is non-trivial, include a screenshot.
  • Recommendation: If the version dropdown or portal link changes the UI significantly, attach a screenshot. If minimal, optional.

Summary Table

Section Status Recommendation
Title Use an imperative, descriptive title. Example: Refactor: Use shared IHttpClient for Foundry service and add Foundry agent versioning support
Commit Type Select a single commit type in the PR body; likely refactor or feature
Risk Level Add risk:high label and select High in PR body (advised: High)
What & Why Add a short What & Why section describing intent and reason (see examples above)
Impact of Change ⚠️ Describe Users/Developers/System impacts, migration steps, and runtime expectations
Test Plan Tick Unit tests added/updated and add short manual test summary; add E2E if applicable
Contributors ⚠️ Add contributors if applicable (optional but recommended)
Screenshots/Videos ⚠️ Add if UI changes are significant (optional)

Final message:
Please update the PR title and body as recommended above, add a risk label (recommended risk:high), and mark the appropriate commit type and test plan checkboxes. Highlights to update in the PR body:

  • Commit Type: check refactor (or feature if you consider new user-facing behavior).
  • Risk Level: check High and add label risk:high to the PR.
  • What & Why: add the suggested short description (one paragraph each).
  • Impact of Change: fill in Users/Developers/System bullets (use the suggested text or refine it to be specific to your changes).
  • Test Plan: check Unit tests added/updated and add a brief manual testing summary. Mention the main test files you updated.

Once you update the PR body and add the risk label, please re-request review. If you intended this to be lower risk, include reasoning (test coverage, limited runtime surface) and we will reconsider the advised risk level.


Last updated: Thu, 05 Mar 2026 17:32:14 GMT

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 4, 2026

📊 Coverage Check

The following changed files need attention:

⚠️ libs/designer-v2/src/lib/ui/panel/connectionsPanel/createConnection/custom/useCognitiveService.ts - 19% covered (needs improvement)
⚠️ libs/designer/src/lib/ui/panel/connectionsPanel/createConnection/custom/useCognitiveService.ts - 19% covered (needs improvement)

Please add tests for the uncovered files before merging.

ccastrotrejo and others added 4 commits March 4, 2026 17:33
…, and post-save refresh

- Add version picker dropdown to FoundryAgentDetails with auto-select
  of latest version on first load and stored version restoration
- Add listFoundryAgentVersions API with data-plane + portal BFF
  fallback and resilient response parsing (extractVersionsData)
- Add foundryAgentVersionNumber internal parameter to agent loop manifest
- Add useFoundryAgentVersions React Query hook in both designer packages
- Wire version change to update model, instructions, and portal link
- Add post-save version refresh: flushPendingFoundryUpdates tracks
  flushed nodes, onFlushed callback invalidates versions cache
- Refactor CognitiveServiceService: replace getHttpClient() getter with
  readonly httpClient property to match standard service pattern
- Add 35 foundry service tests, 9 UI tests, and foundryUpdates tests

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s on initial load

On first agent load, the version number and system instructions were
not written to the workflow parameters, causing validation errors
(empty system message content). The initial-load effect now writes
both foundryAgentVersionNumber and syncs instructions into the
messages parameter alongside user instructions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… warning

- Use useRef flag for one-shot initial version sync instead of state
  guard that could race with batched Redux dispatches
- Add value and selectedOptions props to disabled version dropdown
  to prevent uncontrolled-to-controlled transition

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Foundry code

- Add needsVersionRefresh() to check flag without consuming it
- Only consume flag after confirming new version differs from stored
- Extract shared helpers: useFoundryConnectionResourceId, getFoundryServiceContext
- Simplify foundryAgentService query params and extractVersionsData
- Update tests for needsVersionRefresh behavior in both designer packages

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
/** Normalize the project endpoint to the Foundry data-plane host. */
function normalizeEndpoint(projectEndpoint: string): string {
const base = projectEndpoint.endsWith('/') ? projectEndpoint.replace(/\/+$/, '') : projectEndpoint;
const base = projectEndpoint.replace(/\/+$/, '');

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data High

This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
This
regular expression
that depends on
library input
may run slow on strings with many repetitions of '/'.
… fix ReDoS and portal URL fallback

- Version selection no longer queues a Foundry agent update API call
- Portal URL omits ?version= when no version is selected (defaults to latest)
- Fix polynomial regex in buildProjectEndpointFromResourceId (greedy [^/]+)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ccastrotrejo ccastrotrejo marked this pull request as ready for review March 5, 2026 16:47
Copilot AI review requested due to automatic review settings March 5, 2026 16:47
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants