Skip to content

Bug 2069243 - Add the containers component - #7577

Merged
bakulf merged 1 commit into
mozilla:mainfrom
bakulf:containers
Sep 22, 2026
Merged

bakulf merged 1 commit into
mozilla:mainfrom
bakulf:containers

Conversation

@bakulf

@bakulf bakulf commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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 diligence applied in selecting them.

@bakulf
bakulf force-pushed the containers branch 2 times, most recently from 0efa8a8 to 0713de6 Compare September 2, 2026 22:36
@bakulf bakulf changed the title Add the container component Add the containers component Sep 2, 2026
@bakulf
bakulf force-pushed the containers branch 2 times, most recently from 15cb50c to 38b3ec6 Compare September 4, 2026 07:35
@bakulf bakulf changed the title Add the containers component Bug 2069243 - Add the containers component Sep 4, 2026
@bakulf
bakulf requested a review from moztcampbell September 4, 2026 13:53

@bendk bendk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know enough about the containers code to really review this, so I mostly focused on general Rust component issues.

Comment thread components/containers/src/error.rs
Comment thread components/containers/src/store.rs
@bakulf
bakulf force-pushed the containers branch 3 times, most recently from 623f8c6 to 16ba90f Compare September 18, 2026 14:35

@jonalmeida jonalmeida left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I walked through the code myself locally and it looks fine to me. I don't have a strong sense of what is good rust style these days, but it's good start either way.

We synced on this patch and these are my take-away notes:

  • The l10n IDs will be removed in a future iteration because they are desktop only and cannot link to Android.
  • A syncable future is separate, but we don't want to combine this component with an existing syncable storage like remote_tabs because it will use a different sync machine for this store.
  • Icons and colours need to be generated on the frontend side of the platforms to be used there, and we link them with a shared ID.
  • This is an in-memory copy of the containers, and the platforms will push and pull the list from here.

@bakulf
bakulf added this pull request to the merge queue Sep 22, 2026
Merged via the queue into mozilla:main with commit 1b81fb7 Sep 22, 2026
15 checks passed
@bakulf
bakulf deleted the containers branch September 22, 2026 03:37
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.

3 participants