Skip to content

fix(ui): create flow - name trimming issue#93

Merged
siddhant-galileo merged 7 commits intomainfrom
siddhant/name-trimming-issue
Mar 12, 2026
Merged

fix(ui): create flow - name trimming issue#93
siddhant-galileo merged 7 commits intomainfrom
siddhant/name-trimming-issue

Conversation

@siddhant-galileo
Copy link
Collaborator

@siddhant-galileo siddhant-galileo commented Mar 11, 2026

Summary

Scope

  • User-facing/API changes:
  • Internal changes:
  • Out of scope:

Risk and Rollout

  • Risk level: low / medium / high
  • Rollback plan:

Testing

  • Added or updated automated tests
  • Ran make check (or explained why not)
  • Manually verified behavior

Checklist

  • Linked issue/spec (if applicable)
  • Updated docs/examples for user-facing changes
  • Included any required follow-up tasks

@siddhant-galileo siddhant-galileo requested a review from lan17 March 11, 2026 05:14
Copy link
Contributor

@lan17 lan17 left a comment

Choose a reason for hiding this comment

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

Added two inline review comments.

@lan17
Copy link
Contributor

lan17 commented Mar 11, 2026

I think the main unresolved issue here is the contract boundary.

This repo already has a clear pattern for agent names: normalization is defined once and applied consistently across shared models, server endpoints, and SDK entry points. Control names still do not have that. In this PR, the only canonicalization being added is inside one UI flow, while the server create/patch endpoints and both SDKs still treat name as caller-provided input.

That matters because non-UI clients are real clients here. A browser user now gets trimming/canonicalization in the modal, but Python and TypeScript SDK callers still need to know to pre-trim or they will just hit API validation. I think we should make that choice explicit: either control-name canonicalization belongs at the server/shared boundary, or this remains validation-only and every client is responsible for sending canonical names. I would avoid implicitly defining canonical control names in one modal.

Separately, the legacy-name policy should be explicit and covered by a regression test. The code reads like “existing invalid names remain editable unless the user actually renames them”, which seems reasonable, but it should be stated and tested directly.

The login logo swap also looks unrelated to the trimming fix. If it’s intentional, I’d explain it in the PR; otherwise I’d split it out.

@siddhant-galileo
Copy link
Collaborator Author

siddhant-galileo commented Mar 11, 2026

I think the main unresolved issue here is the contract boundary.

This repo already has a clear pattern for agent names: normalization is defined once and applied consistently across shared models, server endpoints, and SDK entry points. Control names still do not have that. In this PR, the only canonicalization being added is inside one UI flow, while the server create/patch endpoints and both SDKs still treat name as caller-provided input.

That matters because non-UI clients are real clients here. A browser user now gets trimming/canonicalization in the modal, but Python and TypeScript SDK callers still need to know to pre-trim or they will just hit API validation. I think we should make that choice explicit: either control-name canonicalization belongs at the server/shared boundary, or this remains validation-only and every client is responsible for sending canonical names. I would avoid implicitly defining canonical control names in one modal.

Separately, the legacy-name policy should be explicit and covered by a regression test. The code reads like “existing invalid names remain editable unless the user actually renames them”, which seems reasonable, but it should be stated and tested directly.

Fixed this - moved logic to server now. UI only uses trim to check if name has updated or not

The login logo swap also looks unrelated to the trimming fix. If it’s intentional, I’d explain it in the PR; otherwise I’d split it out.

Added a line in PR description

@siddhant-galileo siddhant-galileo requested a review from lan17 March 11, 2026 06:42
Copy link
Contributor

@lan17 lan17 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@siddhant-galileo siddhant-galileo merged commit 1a9759d into main Mar 12, 2026
6 checks passed
@siddhant-galileo siddhant-galileo deleted the siddhant/name-trimming-issue branch March 12, 2026 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants