Refactor settings into a single component across Chat/Ask #2111

bnodir · 2024-11-03T11:11:36Z

Purpose

We had a refactoring conversation below. In this PR, I tried that.

#2021 (comment)
I wish that I had refactored these settings to be in a single component, as now we have duplicated code each time we add a new setting. This is fine for this PR, but eventually I want to try a single Settings.tsx across Chat/Ask.

Does this introduce a breaking change?

When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.

[ ] Yes
[x] No

Does this require changes to learn.microsoft.com docs?

This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.

[ ] Yes
[x] No

Type of change

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

Code quality checklist

See CONTRIBUTING.md for more details.

The current tests all pass (python -m pytest).
I added tests that prove my fix is effective or that my feature works
I ran python -m pytest --cov to verify 100% coverage of added lines
I ran python -m mypy to check for type errors
I either used the pre-commit hooks or ran ruff and black manually on my code.

zedhaque · 2024-11-03T22:07:49Z

@bnodir - Nice work! Would it be worth exploring a single menu in the top-right main navigation (black header) for options like "Clear Chat," "Manage Uploads," and "Dev Settings"? These menu items could appear only when their respective settings are activated.

app/frontend/src/pages/chat/Chat.tsx

pamelafox · 2024-11-04T18:23:53Z

Is this a WIP as you're still testing it? You could add playwright tests to click each of the settings and check that the subsequent /chat or /ask network request had the expected options. Am not sure how much that'd slow the tests down to test each of them - you could do just one test that touched all the options, so it'd be one for chat and one for ask.

pamelafox · 2024-11-04T18:25:32Z

The code change looks good! I don't think this PR needs any additional UI changes, per @zedhaque's comments, as this is currently one that folks could merge fairly cleanly.

@zedhaque Are you finding the top-right links getting a bit cluttersome?

zedhaque · 2024-11-04T21:06:39Z

@pamelafox - The issue I was referring to is specifically with the “menu items” — options like “clear chat” or “upload file” quickly disappear as your chat progresses on both mobile and desktop. Having these items on a sticky navigation would allow users to start a new chat more easily. I understand the menu could get crowded on mobile, but we could use icons only on mobile and icons with text on desktop.

Yes, I totally agree it should be a different PR as it will be fairly straightforward to rollout. Cheers

bnodir · 2024-11-05T14:23:47Z

I have manually tested the changes, not with Playwright, and they were working as expected. Therefore, I removed WIP from the title.

Regarding document updates, at least data_ingestion.md needs to be edited with the Settings.tsx file path. Should I wait for #2118 to be merged?

github-actions · 2024-11-05T17:43:06Z

Check Broken Paths

We have automatically detected the following broken relative paths in your files.
Review and fix the paths to resolve this issue.

Check the file paths and associated broken paths inside them. For more details, check our Contributing Guide.

File Full Path Issues

docs/data_ingestion.md

#	Link	Line Number
1	`../app/frontend/src/components/settings/Settings.tsx`	`49`

github-actions · 2024-11-05T17:44:02Z

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues

README.md

#	Link	Line Number
1	`https://aka.ms/entgptsearchblog`	`270`
2	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/access-control-in-generative-ai-applications-with-azure/ba-p/3956408`	`274`

docs/deploy_lowcost.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`43`

docs/customization.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`120`

docs/productionizing.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/azure-architecture-blog/azure-openai-landing-zone-reference-architecture/ba-p/3882102`	`86`

docs/deploy_features.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`209`

docs/data_ingestion.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`75`

samples/data-ingestion/README.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`88`

samples/chat/README.md

#	Link	Line Number
1	`https://aka.ms/entgptsearchblog`	`272`
2	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/access-control-in-generative-ai-applications-with-azure/ba-p/3956408`	`276`

github-actions · 2024-11-05T17:45:11Z

Check Broken Paths

We have automatically detected the following broken relative paths in your files.
Review and fix the paths to resolve this issue.

Check the file paths and associated broken paths inside them. For more details, check our Contributing Guide.

File Full Path Issues

docs/data_ingestion.md

#	Link	Line Number
1	`../app/frontend/src/components/settings/Settings.tsx`	`60`

github-actions · 2024-11-05T17:45:50Z

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues

README.md

#	Link	Line Number
1	`https://aka.ms/entgptsearchblog`	`291`
2	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/access-control-in-generative-ai-applications-with-azure/ba-p/3956408`	`295`

docs/deploy_lowcost.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`43`

docs/customization.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`120`

docs/productionizing.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/azure-architecture-blog/azure-openai-landing-zone-reference-architecture/ba-p/3882102`	`86`

docs/deploy_features.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`209`

docs/data_ingestion.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`78`

pamelafox · 2024-11-05T17:54:23Z

The techcommunity link breakage is expected due to an ongoing upgrade of that site.
I am just adding a bit more to the playwright test on settings and will push that soon.
No changes needed from you @bnodir, as I also made the docs change.

github-actions · 2024-11-05T18:05:59Z

Check Broken Paths

We have automatically detected the following broken relative paths in your files.
Review and fix the paths to resolve this issue.

Check the file paths and associated broken paths inside them. For more details, check our Contributing Guide.

File Full Path Issues

docs/data_ingestion.md

#	Link	Line Number
1	`/app/frontend/src/components/settings/Settings.tsx`	`60`

github-actions · 2024-11-05T18:06:44Z

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues

README.md

#	Link	Line Number
1	`https://aka.ms/entgptsearchblog`	`291`
2	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/access-control-in-generative-ai-applications-with-azure/ba-p/3956408`	`295`

docs/deploy_lowcost.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`43`

docs/customization.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`120`

docs/productionizing.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/azure-architecture-blog/azure-openai-landing-zone-reference-architecture/ba-p/3882102`	`86`

docs/deploy_features.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`209`

docs/data_ingestion.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`78`

github-actions · 2024-11-05T18:26:05Z

Check Broken Paths

We have automatically detected the following broken relative paths in your files.
Review and fix the paths to resolve this issue.

Check the file paths and associated broken paths inside them. For more details, check our Contributing Guide.

File Full Path Issues

docs/data_ingestion.md

#	Link	Line Number
1	`/app/frontend/src/components/settings/Settings.tsx`	`60`

github-actions · 2024-11-05T18:26:53Z

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues

README.md

#	Link	Line Number
1	`https://aka.ms/entgptsearchblog`	`291`
2	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/access-control-in-generative-ai-applications-with-azure/ba-p/3956408`	`295`

docs/deploy_lowcost.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`43`

docs/customization.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/azure-ai-search-outperforming-vector-search-with-hybrid/ba-p/3929167`	`120`

docs/productionizing.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/azure-architecture-blog/azure-openai-landing-zone-reference-architecture/ba-p/3882102`	`86`

docs/deploy_features.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`209`

docs/data_ingestion.md

#	Link	Line Number
1	`https://techcommunity.microsoft.com/t5/ai-azure-ai-services-blog/announcing-the-public-preview-of-integrated-vectorization-in/ba-p/3960809#:~:text=Integrated%20vectorization%20is%20a%20new%20feature%20of%20Azure,pull-indexers%2C%20and%20vectorization%20of%20text%20queries%20through%20vectorizers`	`78`

pamelafox · 2024-11-05T18:30:25Z

I believe all the broken links are either due to techcommunity blog outage or due to Settings.tsx not being in main branch yet, so I'll merge this.

Thanks @bnodir!

bnodir added 2 commits November 3, 2024 20:01

Refactor settings code across Chat/Ask

0e8087c

Refactor settings code across Chat/Ask

328f12a

cforce mentioned this pull request Nov 4, 2024

Bug: adlsgen2setup.py does not use $AZURE_ADLS_GEN2_FILESYSTEM env var #2110

Open

pamelafox reviewed Nov 4, 2024

View reviewed changes

app/frontend/src/pages/chat/Chat.tsx Show resolved Hide resolved

bnodir changed the title ~~WIP: Refactor settings into a single component across Chat/Ask~~ Refactor settings into a single component across Chat/Ask Nov 5, 2024

Update docs for settings change

94a5fed

Merge branch 'main' into refactor/shared-settings-component

d0a211f

Update playwright test

7968c4c

pamelafox approved these changes Nov 5, 2024

View reviewed changes

pamelafox merged commit 86b6dad into Azure-Samples:main Nov 5, 2024
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor settings into a single component across Chat/Ask #2111

Refactor settings into a single component across Chat/Ask #2111

bnodir commented Nov 3, 2024 •

edited

Loading

zedhaque commented Nov 3, 2024

pamelafox commented Nov 4, 2024

pamelafox commented Nov 4, 2024

zedhaque commented Nov 4, 2024

bnodir commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

pamelafox commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

pamelafox commented Nov 5, 2024

Refactor settings into a single component across Chat/Ask #2111

Refactor settings into a single component across Chat/Ask #2111

Conversation

bnodir commented Nov 3, 2024 • edited Loading

Purpose

Does this introduce a breaking change?

Does this require changes to learn.microsoft.com docs?

Type of change

Code quality checklist

zedhaque commented Nov 3, 2024

pamelafox commented Nov 4, 2024

pamelafox commented Nov 4, 2024

zedhaque commented Nov 4, 2024

bnodir commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

Check Broken Paths

github-actions bot commented Nov 5, 2024

Check Broken URLs

github-actions bot commented Nov 5, 2024

Check Broken Paths

github-actions bot commented Nov 5, 2024

Check Broken URLs

pamelafox commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

Check Broken Paths

github-actions bot commented Nov 5, 2024

Check Broken URLs

github-actions bot commented Nov 5, 2024

Check Broken Paths

github-actions bot commented Nov 5, 2024

Check Broken URLs

pamelafox commented Nov 5, 2024

bnodir commented Nov 3, 2024 •

edited

Loading