Skip to content

fix(ONYX-2196): show checkbox instead of toggle for newly created artwork lists - #13848

Open
evaschilken wants to merge 2 commits into
mainfrom
evaschilken/ONYX-2196-eigen-new-list-shows-toggle-instead-of-checkbox
Open

fix(ONYX-2196): show checkbox instead of toggle for newly created artwork lists#13848
evaschilken wants to merge 2 commits into
mainfrom
evaschilken/ONYX-2196-eigen-new-list-shows-toggle-instead-of-checkbox

Conversation

@evaschilken

@evaschilken evaschilken commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This PR resolves ONYX-2196

Description

When saving an artwork and creating a new list on the fly from the "Save to lists" sheet, the new list's row rendered a Switch (toggle) instead of the checkbox every other list shows.

Root cause: ArtworkListItem decides checkbox vs. toggle based on field presence — shareableWithPartners !== undefined && isSavedArtwork === undefined — to distinguish the artwork-picker context from the Offer Settings screen context (which never fetches isSavedArtwork). The createCollection mutation's Relay updater spliced the newly created list straight into the picker's connection without ever setting isSavedArtwork on that record, so it accidentally matched the Offer Settings branch and rendered a toggle.

Fix: the updater now seeds isSavedArtwork: false on the new record (keyed to the current artwork's ID) before inserting it into the connection — always correct, since a brand-new list can't already contain the artwork.

Assisted by: Claude Sonnet 5

Before
Screenshot 2026-07-29 at 15 33 24

After
new-list-checkbox

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one. (Existing AREnableArtworkListOfferability flag already gates this code path.)
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • Fix newly created artwork lists showing a toggle instead of a checkbox in the "Save to lists" flow

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

…work lists

The Relay updater never seeded `isSavedArtwork` on a freshly-created list's
record, so ArtworkListItem mistook it for an Offer Settings row (which also
never fetches `isSavedArtwork`) and rendered a Switch instead of a checkbox.
Seed it to `false` since a brand-new list can't already contain the artwork.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@evaschilken evaschilken self-assigned this Jul 29, 2026
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@github-actions github-actions Bot deleted a comment from claude Bot Jul 29, 2026
@artsyit

artsyit commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Cross-platform user-facing changes (Fix newly created artwork lists showing a toggle instead of a checkbox in the "Save to lists" flow - evaschilken)

Generated by 🚫 dangerJS against 8b64065

@evaschilken
evaschilken requested review from a team and MounirDhahri July 29, 2026 13:52
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Seeds isSavedArtwork: false on the newly-created collection record in the createCollection Relay updater, keyed to the current artwork's internalID. That stops ArtworkListItem from misreading a fresh list as an Offer Settings row and rendering a Switch instead of a checkbox. One regression test added.

The fix is correct. CreateNewArtworkListForm passes artwork?.internalID, the same value SelectArtworkListsForArtwork feeds the query as $artworkID (SelectArtworkListsForArtwork.tsx:17), so setValue(false, "isSavedArtwork", { artworkID }) writes the exact storage key both ArtworkLists_me and ArtworkListItem_item read. The Offer Settings query passes includeArtwork: false, so it never reads the field and the toggle path is unaffected. When a list is created from the Saves tab headers instead, artwork is null and nothing is seeded — also fine, since that screen renders ArtworkListItem_collection, a different fragment with no isSavedArtwork.

Issues Found

🟢 Suggestion

The heuristic in ArtworkListItem is the actual fragilitysrc/app/Components/ArtworkLists/components/ArtworkListItem.tsx:32-35

const offerSettingsList =
  artworkList.shareableWithPartners !== undefined &&
  artworkList.isSavedArtwork === undefined &&
  !!isArtworkListOfferabilityEnabled

Inferring "which screen am I on" from field absence means any future write path that skips isSavedArtwork reintroduces this bug, and the failure mode is a silently wrong control rather than a type error. ArtworkLists already knows its context — it renders under either SelectArtworkListsForArtwork or ArtworkListOfferSettings. Passing that down as an explicit prop would make the seeding unnecessary. Not blocking; worth a follow-up ticket.

Test assertion leans on an unstated invariantsrc/app/Components/ArtworkLists/__tests__/ArtworkListsContext.tests.tsx:373

expect(screen.getByTestId("artworkListItemSelectedIcon")).toBeOnTheScreen()

This passes because "Saved Artworks" is mocked with isSavedArtwork: false and the new list is the only preselected row, so getByTestId resolves to exactly one node. That is load-bearing but invisible to the next person editing the fixture. A row-count assertion (expect(screen.getAllByTestId("artworkListItem")).toHaveLength(2)) would make the intent explicit.

Areas Reviewed

  • Architecture & Design — The updater now needs context (artworkID) it did not before. Acceptable, but see the suggestion above.
  • Bugs & Edge Cases — No regression for the Offer Settings path or the Saves-tab create flow, per the reasoning above. reset on picker dismissal (SelectArtworkListsForArtworkView.tsx:22) clears artwork, so no stale ID leaks into a later create. Seeding false is unconditionally safe since a brand-new list is empty.
  • Testing — The regression test does exercise the fix: without the seed, queryAllByRole("switch") returns 1 and the assertion fails.
  • Performance / Security — Nothing notable.

Questions for Author

The checklist leaves Android untested with no Android screenshots. This is a UI change, the surrounding component tree branches on Platform.OS around the switch/checkbox area (ArtworkListOfferSettings.tsx:49-57), and the new test only drives the Android submit path. Please add an Android recording or screenshot before merge.

LLM disclosure is present ("Assisted by: Claude Sonnet 5").

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