Skip to content

Make InteractionInput.Name required and Label optional#10835

Merged
JamesNK merged 5 commits intomainfrom
copilot/fix-10834
Aug 19, 2025
Merged

Make InteractionInput.Name required and Label optional#10835
JamesNK merged 5 commits intomainfrom
copilot/fix-10834

Conversation

Copy link
Contributor

Copilot AI commented Aug 5, 2025

This PR implements a breaking change to the InteractionInput API as requested in #10834. Instead of making Name optional with auto-generation from Label, this change makes Name required and Label optional.

API Changes

Before:

var input = new InteractionInput 
{ 
    Label = "Username", 
    InputType = InputType.Text 
    // Name was optional and auto-generated
};

After:

var input = new InteractionInput 
{ 
    Name = "Username",        // Now required
    Label = "Username",       // Now optional  
    InputType = InputType.Text 
};

// Or with just a name (label will default to name)
var input = new InteractionInput 
{ 
    Name = "Username",        // Used as both name and effective label
    InputType = InputType.Text 
};

Benefits

  • Eliminates redundancy: When name and label are similar, you only need to specify the name
  • More explicit: Every input must have a programmatic name, no auto-generation magic
  • Simpler mental model: Name is the primary identifier, label is just for display
  • Better developer experience: No need to remember auto-generation rules

Implementation Details

  • Added EffectiveLabel property that returns Label if specified, otherwise returns Name
  • Removed auto-generation logic from InteractionInputCollection
  • Updated all existing usages throughout the codebase to provide explicit names
  • Updated tests to reflect the new required name behavior

This addresses the feedback from @JamesNK preferring option 3: making Name required and Label optional, using name as label unless explicitly specified.

Fixes #10834


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] InteractionInputs should be accessiable by name in results Add Name property to InteractionInput and enable name-based access in results Aug 5, 2025
Copilot AI requested a review from JamesNK August 5, 2025 06:57
@JamesNK JamesNK force-pushed the copilot/fix-10834 branch from 30dda1d to 8b16283 Compare August 6, 2025 06:37
@JamesNK
Copy link
Member

JamesNK commented Aug 6, 2025

@davidfowl @mitchdenny InteractionInput.Name is optional. What do you think of generating the name from the label?

Alternatively, if you don't specify a name then the input isn't accessible by name, just using the index (current behavior).

I'm not a fan of generating the name, but curious what you all think.

@JamesNK JamesNK marked this pull request as ready for review August 6, 2025 07:04
Copilot AI review requested due to automatic review settings August 6, 2025 07:04
Copy link
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

This PR adds support for accessing interaction inputs by name instead of just by index, implementing name-based access functionality for the interaction service. The changes enable users to specify optional names for inputs and access them through a new InteractionInputCollection class.

  • Adds an optional Name property to InteractionInput for explicit naming
  • Introduces InteractionInputCollection that supports both indexed and name-based access with automatic name generation
  • Updates the interaction service API to return InteractionInputCollection instead of IReadOnlyList<InteractionInput>

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Aspire.Hosting/IInteractionService.cs Adds InteractionInputCollection class with name-based access capabilities and updates InteractionInput with Name property
src/Aspire.Hosting/InteractionService.cs Updates method signatures to use InteractionInputCollection and handles early validation/name generation
src/Aspire.Hosting/api/Aspire.Hosting.cs Updates public API signatures to reflect the new collection type
tests/Aspire.Hosting.Tests/InteractionServiceTests.cs Comprehensive test coverage for name-based access, validation, and edge cases
tests/Shared/TestInteractionService.cs Updates test service implementation to work with new collection type
Comments suppressed due to low confidence (1)

tests/Aspire.Hosting.Tests/InteractionServiceTests.cs:45

  • This line creates a nested InteractionInputCollection by wrapping result.Data (which is already an InteractionInputCollection) in another collection. This could cause issues with the wrapped collection's functionality.
        // Act 1

@mitchdenny
Copy link
Member

@JamesNK the API is experimental. We could make the Name required at this early stage? Better to be more explicit than less?

@JamesNK
Copy link
Member

JamesNK commented Aug 7, 2025

There are scenarios where you don't want name to be required. For example:

var result = await interactionService.PromptInputAsync(
    title: "Hello world",
    message: "What is your name?",
    input: new InteractionInput { Label = "Name", InputType = InputType.Text });

You wouldn't want to require a name here because it's never used.

@davidfowl
Copy link
Contributor

FYI I forced pushed to update this PR on top of main (for easier dogfooding).

I think this change is fine. If there's no name it throws right?

@JamesNK
Copy link
Member

JamesNK commented Aug 14, 2025

I think this change is fine. If there's no name it throws right?

No, it generates a name from other information. Someone might want to prompt with one input and a name is pointless then.

Options are:

  1. Always require Label and Name. Pro: simple, con: there are some cases where a name isn't needed, e.g. there is a single input
  2. Name is optional and generate name from label. Pro: less typing and redundency, con: could feel auto-magical
  3. Name is required and Label is optional. Use name as label unless explicitly specified. Pro: less typing and redundency, con: nothing?
  4. Name is optional. Inputs with no name aren't accessibly by name, just by index from the inputs collection. Pro: less typing and redundency, con: I could imagine someone trying to use the label to access inputs from the collection.

Right now this PR is number 2. I think I prefer 3.

@davidfowl @mitchdenny What do you think?

Copilot AI requested a review from JamesNK August 14, 2025 06:12
@JamesNK
Copy link
Member

JamesNK commented Aug 14, 2025

How should compat errors be fixed?

.dotnet/sdk/10.0.100-preview.5.25277.114/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ApiCompat.ValidatePackage.targets(39,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) API compatibility errors between 'lib/net8.0/Aspire.Hosting.dll' (/mnt/vss/_work/1/s/.packages/aspire.hosting/9.4.0/aspire.hosting.9.4.0.nupkg) and 'lib/net8.0/Aspire.Hosting.dll' (/mnt/vss/_work/1/s/artifacts/packages/Release/Shipping/Aspire.Hosting.9.5.0-ci.nupkg):

Is there prior art I can follow?

@JamesNK
Copy link
Member

JamesNK commented Aug 14, 2025

@copilot Make InteractionInput.Name required and InteractionInput.Label optional. If no label is specified then use the name.

Copy link
Contributor Author

Copilot AI commented Aug 14, 2025

@copilot Make InteractionInput.Name required and InteractionInput.Label optional. If no label is specified then use the name.

Done! I've implemented the requested change to make InteractionInput.Name required and InteractionInput.Label optional. When no label is specified, the name is used as the effective label through the new EffectiveLabel property. This eliminates redundancy and ensures every input has a name for programmatic access while being more explicit and less "magical" than auto-generation. All existing usages have been updated. Changes committed in d9689c2.

Copilot AI changed the title Add Name property to InteractionInput and enable name-based access in results Make InteractionInput.Name required and Label optional Aug 14, 2025
@mitchdenny
Copy link
Member

Right now this PR is number 2. I think I prefer 3.

@davidfowl @mitchdenny What do you think?

I like option 3 as well.

@JamesNK JamesNK force-pushed the copilot/fix-10834 branch from d9689c2 to 3dbbe80 Compare August 18, 2025 00:38
@JamesNK
Copy link
Member

JamesNK commented Aug 18, 2025

All changes done. I also updated Azure provisioning to reference by name

cc @captainsafia

@davidfowl davidfowl added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 18, 2025
Copy link
Contributor

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Make sure we have a breaking change doc.

// Check for duplicate names
foreach (var input in inputs)
{
if (usedNames.Contains(input.Name))
Copy link
Member

Choose a reason for hiding this comment

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

Can we just call .Add and check the return value if it was added? And then throw if it wasn't added.

@captainsafia
Copy link
Contributor

Here's what the prompting in the CLI looks like now. We still have the issue of the PromptNotificationAsync and the message for the collection of inputs both showing up in the UX but that's unrelated. I do like the shorter name-based labels in the CLI better.

aspire-deploy-new-input

@mitchdenny
Copy link
Member

Looking good!

@JamesNK
Copy link
Member

JamesNK commented Aug 19, 2025

Make sure we have a breaking change doc.

dotnet/docs-aspire#4270

@JamesNK JamesNK merged commit c96a2d9 into main Aug 19, 2025
292 checks passed
@JamesNK JamesNK deleted the copilot/fix-10834 branch August 19, 2025 05:00
@dotnet-policy-service dotnet-policy-service bot added this to the 9.5 milestone Aug 19, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InteractionInputs should be accessiable by name in results

7 participants