Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] - Leo QA Iteration 3 #22627

Merged
merged 2 commits into from
Mar 16, 2024
Merged

[iOS] - Leo QA Iteration 3 #22627

merged 2 commits into from
Mar 16, 2024

Conversation

Brandon-T
Copy link
Contributor

@Brandon-T Brandon-T commented Mar 15, 2024

  • Fixing AI-Chat bugs in the SDK where it does not save the user's model selection no matter what
  • Potentially fixing bugs in the SDK where the user is somehow premium but still rate-limited

Resolves brave/brave-browser#36824
Resolves brave/brave-browser#36816
Resolves brave/brave-browser#36856
Resolves brave/brave-browser#36857

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@Brandon-T Brandon-T added this to the 1.66.x - Nightly milestone Mar 15, 2024
@Brandon-T Brandon-T self-assigned this Mar 15, 2024
@Brandon-T Brandon-T requested a review from a team as a code owner March 15, 2024 18:31
Comment on lines 108 to 109
currentModel = api.models.first(where: { $0.key == defaultModelKey }) ?? api.currentModel
changeModel(modelKey: defaultModelKey)
Copy link
Member

Choose a reason for hiding this comment

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

ConversationDriver has its own mechanism for setting the default model from pref or varying via Premium status, which could be async and race with this. At the moment it's not likely to be a bad race condition but that could change in the future. Could you at least set a TODO here to move to the shared pref for default model? We should sync it anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I exported the preference here: 64dd821

…nces and premium subscription credentials

Export Default-Model preference from Brave-Core to iOS

Make SkusSDK a singleton
@Brandon-T Brandon-T changed the title Fixing AI-Chat bugs in the SDK and Brave-Core with regards to prefere… [iOS] - Leo QA Iteration 3 Mar 16, 2024
@Brandon-T Brandon-T merged commit cbedd44 into master Mar 16, 2024
19 checks passed
@Brandon-T Brandon-T deleted the bugfix/QA-Iteration-3 branch March 16, 2024 08:40
@kjozwiak
Copy link
Member

Verification PASSED on iPhone 11 running iOS 17.4 using the following build(s):

1.66.10 Chromium: 123.0.6312.46 (Official Build) unknown (64-bit)
--- | ---
Revision | 741e5a4ef519...
OS | iOS

Test Case #1 - brave/brave-browser#36824

The following issue can't really be verified on Nightly and requires an RC. As per the conversation via https://bravesoftware.slack.com/archives/C04KPE0BUDU/p1710518963710699, we'll need to create new accounts to run through the above and ensure that Premium users aren't getting rate limited. @Uni-verse @hffvld and myself will discuss and run through three different scenarios that involve the Rate Limit modal.

Test Case #2 - brave/brave-browser#36816

Using the STR/Cases outlined via brave/brave-browser#36816 (comment), ensured the following:

  • ensured that switching between Mixtral, Claude and Lama via Advanced Settings retained the users selection
  • ensured that switching the default LLM via Leo panel didn't change the global setting
    • Example: Claude set as default and then selecting Lama as a model for the tab instance (shouldn't change global setting)
RPReplay_Final1710717329.MP4

However, running through the above, it looks like the global setting is not retained/remembered when restarting Brave and Mixtral is always selected as the default. The above fixed the issue re: same session but didn't fix the issue when restarting Brave. Created brave/brave-browser#36886 as a follow up that should be fixed for 1.63.x.

Test Case #3 - brave/brave-browser#36856

Using the STR/Cases outlined via brave/brave-browser#36856 (comment), ensured that the string/text isn't being cut off on certain locales such as German & Russian. As per the conversation via https://bravesoftware.slack.com/archives/C05NDQP03AQ/p1710531689040729, we're going to live with the string taking up two lines rather than a single line for the initial release but will address the issues via brave/brave-browser#36870 & brave/brave-browser#36871.

German

Example Example
IMG_0137 IMG_0138

Russian

Example Example
IMG_0139 IMG_0140

Test Case #4 - brave/brave-browser#36857

Using the STR/Cases outlined via brave/brave-browser#36857 (comment), ensured that Add Feedback was visible for Dislike via both Light & Dark themes as per the following:

Disliked

Example Example
IMG_0141 IMG_0143

Liked

Example Example
IMG_0142 IMG_0144

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