Skip to content

Fix label creation and corpus label-set selection - #2328

Merged
JSv4 merged 1 commit into
mainfrom
fix/2295-labelset-state
Sep 10, 2026
Merged

JSv4 merged 1 commit into
mainfrom
fix/2295-labelset-state

Conversation

@JSv4

@JSv4 JSv4 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Label creation could send an invalid bare hex color, report success for a rejected mutation, and close before refreshed labels arrived. Choosing a different label set while creating a corpus also left the dropdown displaying the old selection.

Fixes #2295. Created from current main, using #2296 as a reference.

Changes

  • Normalize create/edit colors to #RRGGBB, including stored short hex values and values with or without #.
  • Check the nested mutation payload for ok: true, preserve form contents on rejection, and wait for refreshed lists/counts before success feedback. Keep submission disabled during the refresh and distinguish refresh failures from failed creation.
  • Put Add Label beside search for all four populated label types. Clear the search when starting a label so an unmatched filter cannot hide the form.
  • Return a typed label-set selection containing both the ID and object; update and clear both in CorpusModal.
  • Add browser regressions for mutation failures, delayed/failed refreshes, first-label creation, stored colors, toolbar placement, and corpus selection/submission. Update existing mutation mock colors to match the corrected API payload.
  • Ratchet the TypeScript any baseline from 433 to 431 and add a changelog fragment.

Test plan

  • Focused Playwright component tests: 53 passed across LabelSetDetailPage.ct.tsx, LabelSetDetailPage.coverage.ct.tsx, LabelSetSelector.ct.tsx, and CorpusModalLabelSet.ct.tsx (including focused reruns after fixing new test fixtures).
  • yarn test:unit run src/utils/__tests__/colorUtils.test.ts: 18 passed.
  • NODE_OPTIONS=--max-old-space-size=4096 yarn build: passed.
  • yarn tsc --noEmit: passed.
  • Prettier checks for changed frontend/test files: passed.
  • yarn any:check:strict: passed.
  • python3 scripts/collate_changelog.py --check: passed.
  • pre-commit run --all-files: passed.

Checklist

  • Tests pass locally for any code this PR touches
  • pre-commit run --all-files passes
  • TypeScript compiles cleanly
  • A changelog fragment was added under changelog.d/
  • No new dependency was introduced

Contributor License Agreement

By submitting this pull request, I agree to license this contribution under the project's Contributor License Agreement.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Code Review

Reviewed the diff, the changed frontend files in full, the backend validate_color resolver, and the new/updated Playwright tests.

Summary

This is a solid, well-scoped bugfix PR. The core issue — createAnnotationLabelForLabelset's success check was reading result.data?.createAnnotationLabelForLabelset?.ok against an Outputs interface that (incorrectly) modeled the payload as flat { ok, message } instead of nested { createAnnotationLabelForLabelset: { ok, message } } — is a real bug, and the fix is correct. I confirmed against config/graphql/label_mutations.py / validation_utils.py that the backend's HEX_COLOR_PATTERN requires a leading #, so the old sanitizeColor (which stripped # and never added it back) really was sending invalid bare-hex values that the backend would reject — exactly the silent-failure bug described in #2295.

Things that look right

  • sanitizeColor in LabelSetDetailPage.tsx now delegates to the shared frontend/src/utils/colorUtils.ts helpers (isValidHexColor / normalizeHexColor) instead of a local, subtly different implementation — good DRY cleanup consistent with CLAUDE.md's utility-file guidance.
  • createLoading is now tracked as local state (setCreateLoading) rather than relying on Apollo's mutation loading, and is folded into the existing isMutating flag together with the await refetch() in the .then() handler — this correctly keeps the Create/Cancel buttons disabled until the post-create refetch settles, closing the race where success toast/UI could show before fresh data arrived. The three new failure-path tests (rejected ok:false, missing payload, network error) and the "refresh fails but creation succeeded" test look like they genuinely exercise the new branches rather than just asserting happy path.
  • Moving the "Add Label" button into a SearchToolbar sibling of the search box, keyed off the unfiltered label count (text_labels.length > 0 etc.) rather than the filtered/search-matched list, correctly fixes the "unmatched filter hides the Add button" bug. Clearing searchTerm in handleStartCreate is a reasonable extra safeguard.
  • LabelSetSelector returning { labelSet, labelSetObj } avoids CorpusModal (or any other consumer) having to re-derive the selected object from a separately-fetched list, and fixes the stale-dropdown-label bug described in the PR summary. The any-typed handleLabelSetChange in CorpusModal.tsx is now properly typed via the exported LabelSetSelection interface.
  • Good cleanup: removed unused lodash import, unused error/fetchMore destructures from useQuery in LabelSetSelector.tsx.

Minor observations (non-blocking)

  1. frontend/src/components/labelsets/LabelSetDetailPage.tsx:786-792 — The edit-existing-label "Save" button doesn't get disabled={isMutating} the way the two "Create" buttons and all three "Cancel" buttons do (only Cancel at line 781 is disabled). handleSaveEdit already guards against re-entrancy internally (if (isMutating) return;), so this isn't exploitable, but it's a slight UX inconsistency in the same area this PR is fixing — a double-click on "Save" briefly no-ops instead of visually disabling. Since this button/handler weren't touched by this diff, this is likely out of scope for Label-set creation and corpus selection provide incorrect frontend state #2295, but worth a quick follow-up if you're back in this file.
  2. There's a small pre-existing (not introduced by this PR) edge case worth being aware of: if a user opens the "Add Label" form (which clears searchTerm) and then types into the now-visible search box while the form is still open, and the typed term matches zero existing labels, renderLabelsList will return the "No labels match" empty state before reaching the isCreating form-render branch, hiding the open create form (state is preserved, just not rendered). Not something this PR needs to fix, but it's adjacent to the exact "search hides the form" class of bug being addressed here, so calling it out in case it's useful context.
  3. sanitizeColor's fallback parameter (DEFAULT_LABEL_COLOR / PRIMARY_LABEL_COLOR) is trusted as always-valid and isn't itself validated before being passed through normalizeHexColor — fine in practice since both are hardcoded 6-hex-digit constants in constants.ts, just flagging the implicit assumption.

Tests / process

  • New tests directly target the fixed behaviors (mutation-failure toasts, disabled state during refresh, toolbar placement/geometry via boundingBox(), stored-color normalization for both #abc/abc/#123abc/123abc shapes, and the CorpusModal label-set selection/clear round trip). This is good coverage for a UI bugfix PR.
  • Changelog fragment, .any-baseline.json ratchet, and mutation-mock color updates ("0F766E" → "#0F766E") are all consistent with the code change and CLAUDE.md's changelog-fragment process.

No blocking issues found. Nice, tightly-scoped fix with good regression coverage.

@JSv4
JSv4 merged commit d55d8ce into main Sep 10, 2026
18 checks passed
@JSv4
JSv4 deleted the fix/2295-labelset-state branch September 10, 2026 07:12
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 10, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Label-set creation and corpus selection provide incorrect frontend state

1 participant