Skip to content

add section_id to display for ml sections - #1280

Open
mlplyler wants to merge 1 commit into
mainfrom
display_section_id
Open

add section_id to display for ml sections#1280
mlplyler wants to merge 1 commit into
mainfrom
display_section_id

Conversation

@mlplyler

@mlplyler mlplyler commented Aug 20, 2026

Copy link
Copy Markdown

Goal

Add section_id to display for ml sections. Currently, it appears there are duplicate sections in the tool. This is because we can have two sections with the same title in experiments.

Implementation Decisions

Got thumbs up here: https://mozilla.slack.com/archives/C09PBCCN0M6/p1787172248034109

@mlplyler
mlplyler requested a review from mmiermans August 20, 2026 18:34
@mlplyler
mlplyler requested a review from a team as a code owner August 20, 2026 18:34
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @mlplyler's task in 2m 29s —— View job


Review: add section_id to display for ml sections

  • Gather context (diff, changed files)
  • Read SectionDetails.tsx + test in full
  • Trace the duplicate-title problem to SectionsPage.tsx
  • Verify externalId is already in the GraphQL fragment (no query change needed)
  • Run the affected test suite — blocked, see note at bottom

Verdict: the change is small, correct, and low-risk. externalId is already selected by BaseSectionData (src/api/fragments/SectionData.ts:6), so no query change is required, and SectionDetails is only rendered from the ML SectionsPage (custom sections use the separate CustomSectionDetails), so the chip is correctly scoped. The two new assertions line up with the mock externalIds '1'/'2'. Comments below are one substantive gap plus polish.


1. The dropdown still can't distinguish duplicate-titled sections

The chip makes duplicates identifiable once rendered, but the filter path is still keyed on title:

  • src/curated-corpus/pages/SectionsPage/SectionsPage.tsx:133-141 builds dropdown options as { code: title, name: title }
  • src/curated-corpus/components/SectionDetails/SectionDetails.tsx:107 filters with section.title === currentSection

So for two experiment sections both titled "Technology", the filter dropdown shows two identical "Technology" entries, and picking either one renders both sections. A curator using the filter to isolate one experiment arm still can't. Keying options on externalId ({ code: section.externalId, name: section.title }) and filtering on section.externalId === currentSection would close the loop — the dropdown label could carry a short id suffix too.

Fine to defer to a follow-up if the goal here was just visual disambiguation on the "All Sections" view, but worth calling out since the PR body frames the duplicates as the problem being solved.

Fix this →

2. externalId is a UUID in production — check the chip width

The schema documents it as "An alternative primary key in UUID format" (src/api/generatedTypes.ts:2202-2203). The tests use '1'/'2', but in the real UI this renders a ~42-character chip (id: 3f2b8c1a-...) inside a display="flex" row that already holds the title block, the enable/disable switch, and the IAB chip. Two things worth confirming against dev data:

  • Layout — does the row still read cleanly, or does it squeeze the switch/IAB chip on narrower viewports?
  • Copyability — curators comparing against ML logs or Slack will want to copy the id. A truncated label (first 8 chars) with a Tooltip showing the full UUID, or an on-click copy, is more usable than a long unselectable-looking chip.

3. No test for the scenario this PR exists to fix

The new assertions prove the chip renders, but nothing covers two sections sharing a title. A case with mockSections where both have title: 'Section 1' and different externalIds, asserting getAllByText('Section 1') has length 2 while id: 1 and id: 2 are both present, would lock in the actual behaviour the PR is about.

Fix this →

4. Nits

  • SectionDetails.tsx:159-165 — the id chip uses the same variant="outlined" color="primary" as the IAB chip immediately before it, so two visually identical chips sit side by side carrying different kinds of information. color="default" (or a monospace label) would make ids scannable at a glance and keep the primary color meaningful for the IAB taxonomy.
  • SectionDetails.tsx:128 — pre-existing, not from this PR: <p>{section.active}</p> renders nothing (React drops booleans) and just adds an empty paragraph to the flex row. Easy drive-by cleanup while you're in this block.
  • The PR description still has the template's unchecked - [ ] Outstanding todo placeholder.

Note on verification: I couldn't run npm ci in this environment (command not permitted), so the test suite wasn't executed locally — CI results on this PR are the source of truth. My reading of the assertions is that getByText('id: 1') matches the MuiChip-label span exactly, so they should pass.
display_section_id

@mmiermans mmiermans 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.

🥳 Congrats on your first admin tools PR!

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