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

Bug 1876211 - Add MDN for Suggest [firefox-android: main] #6086

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

ncloudioj
Copy link
Member

@ncloudioj ncloudioj commented Jan 30, 2024

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

First stab, will need to rebase against #6085. Also noticed a few duplicate code paths that require more refactors perhaps in this patch.

@ncloudioj ncloudioj requested review from a team and 0c0w3 January 30, 2024 04:03
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

Attention: 110 lines in your changes are missing coverage. Please review.

Comparison is base (dba564f) 27.53% compared to head (7367410) 27.47%.

Files Patch % Lines
components/suggest/src/store.rs 0.00% 53 Missing ⚠️
components/suggest/src/db.rs 0.00% 50 Missing ⚠️
components/suggest/src/suggestion.rs 0.00% 4 Missing ⚠️
components/suggest/src/provider.rs 0.00% 1 Missing ⚠️
components/suggest/src/rs.rs 0.00% 1 Missing ⚠️
components/suggest/src/schema.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6086      +/-   ##
==========================================
- Coverage   27.53%   27.47%   -0.06%     
==========================================
  Files         398      398              
  Lines       50450    50559     +109     
==========================================
  Hits        13889    13889              
- Misses      36561    36670     +109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ncloudioj
Copy link
Member Author

@0c0w3 wanted to confirm with you that the current implementation only returns the raw data from RS, do we want to process the data here? (e.g. add utm params; add the hardcoded icon URI, etc.)

Copy link
Contributor

@0c0w3 0c0w3 left a comment

Choose a reason for hiding this comment

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

Thanks @ncloudioj, this looks great! Sorry for the delay, I would have r+'ed this earlier today but I wanted to do an end-to-end test in Firefox and that took some time. It works great.

Yes, the current impl only reflects the data in RS. The desktop urlbar JS handles UTM params and the icon. (Maybe they should be handled by Rust too but that's a future discussion.)

@ncloudioj
Copy link
Member Author

Thanks for confirming that, Drew!

Copy link
Member

@linabutler linabutler left a comment

Choose a reason for hiding this comment

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

Thanks @ncloudioj!

Please be sure to trigger a branch build for iOS and Android. Adding new suggestion types should be a backward-compatible change (I'm realizing it would be super helpful to document what's backward-compatible, and what's a breaking change 😅), but let's make sure there's no unexpected breakage.

components/suggest/src/db.rs Outdated Show resolved Hide resolved
components/suggest/src/db.rs Outdated Show resolved Hide resolved
components/suggest/src/db.rs Outdated Show resolved Hide resolved
components/suggest/src/store.rs Outdated Show resolved Hide resolved
@ncloudioj
Copy link
Member Author

Hey @linabutler, are you suggesting to follow this particular step (i.e. for breaking changes) for this patch? If so, would adding [firefox-android: 1876211-suggest-mdn] to the PR title be sufficient?

@mhammond
Copy link
Member

The branch-name in [firefox-android: branch-name] is a branch on the firefox-android repo. eg, if you knew this was a breaking change, you'd make a branch on that repo with the fixes and specify that name in this PR (I don't think it allows branches in forks, which is a shame). In this scenario where you assume there are no breaking changes, specifying [firefox-android: main] is probably what you want

@ncloudioj
Copy link
Member Author

Gotcha! Thanks for the pointers, Mark!

@ncloudioj ncloudioj changed the base branch from main to fetch-suggestions-refactor January 31, 2024 17:02
@ncloudioj ncloudioj changed the title Bug 1876211 - Add MDN for Suggest Bug 1876211 - Add MDN for Suggest [firefox-android: main] Jan 31, 2024
Base automatically changed from fetch-suggestions-refactor to main February 1, 2024 01:21
@ncloudioj ncloudioj added this pull request to the merge queue Feb 1, 2024
Merged via the queue into main with commit f85fcae Feb 1, 2024
17 checks passed
@ncloudioj ncloudioj deleted the 1876211-suggest-mdn branch February 1, 2024 03:34
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.

None yet

5 participants