fix: match the dropdown field surface to the search field#3583
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
The link modal stacks a search field directly above a dropdown. They were
built independently and had drifted into two different treatments:
DesignSystemSearch surface.enabled (translucent overlay)
decorative.level01 hairline, radius l
DesignSystemDropdown background.level01 (opaque)
decorative.level02 at 2px, radius xl
Stacked four pixels apart, the pair read as controls from two different
products. The opaque fill is the load-bearing half: on an elevated surface —
every modal this control appears in — background.level01 is a dark sunken
hole in dark theme, while the search field beside it sits a touch lighter
than its host. Two fields, opposite reactions to the same sheet.
The dropdown moves to the search field's shell. It has two real call sites
(the relationship selector and the AI settings header bar), against the
search field's sixteen, and the search field's treatment is the better
reasoned of the two.
Two differences are kept deliberately. Expanded, the dropdown border still
switches to interactive.enabled — the search field has no focus treatment to
match, and losing the open-state signal to win a cosmetic match would be a
bad trade. And the field stays taller, because it stacks a label above its
value where a small search field holds one line.
Nothing enforced the match, which is why it drifted. A new test renders both
in one tree and asserts fill, border and radius agree — against each other
and against the tokens, so a change moving both in the same wrong direction
still fails. Verified it bites by reverting the fill.
Part of lotti3-6fz (item 3).
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
033ef2c to
571e743
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 571e743b83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review on #3583 found the change fixed one of the two production pairings and described the other backwards. The AI settings header stacks its dropdown under an AiSettingsSearchBar whose default maps to DesignSystemSearchSize.medium — radius m — while the change pinned every dropdown at radius l. That pairing stayed mismatched, and the new test could not see it because it only covered the small variant. DesignSystemDropdownSize now mirrors the search sizes and selects the radius: small -> l, medium -> m, defaulting to medium since the field is already the height of a medium search field. It controls radius only; height stays content-driven because the field stacks a label above its value. The relationship selector asks for small, pairing it with the task picker's small search field. The pairing test runs both real pairings across both themes, and now pins the radius and border width against the tokens as well as against the search shell — comparing only the two components left them free to drift off the documented contract together, which is the drift the guard exists to prevent. Two documentation corrections: * the link modal builds the relationship dropdown *above* the search field, not below it. The README and the CHANGELOG entry both had it reversed. * surface.enabled is theme-relative — white at 6% in dark, black at 6% in light — so the field lifts off a dark host and insets slightly into a light one. "Sits lighter than whatever panel it's on" was only true in dark theme, and had been copied unqualified into the CHANGELOG and the Flatpak metainfo.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86089d9025
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fifth review round. Both findings land on the previous round's privacy fix. Privacy never reached the database. `createDbEntity` rebuilds the entity with `private: linked?.meta.private` — with no `linkedId` that is an explicit null into copyWith, which *erases* the privacy the caller set rather than leaving it alone. So the previous round's fix threaded `private` all the way down to createMetadata and then had it wiped one call later. Now falls back to the entity's own value; a linked parent still wins when it has an opinion. That test passed anyway, which is the more useful lesson: it asserted on the in-memory Task returned to the caller, not on the stored row. Re-reading from JournalDb makes it fail against the old code, which is now verified. The erasure was never specific to this feature. Any create without a `linkedId` lost its privacy, so `createMeasurementEntryImpl` (which takes `required bool private`) and `createHabitCompletionEntryImpl` (which passes `habitDefinition?.private`) were both affected: a private measurement, or a completion of a private habit, created unlinked, was stored public. Hence its own CHANGELOG entry rather than being folded into the task-links work. The glados property test failed on the fix because it encoded the bug as an invariant — `expectedPrivate => hasLinkedEntity ? linkedPrivate : null`. The expectation is corrected to "a linked parent's privacy wins; without one the entity keeps what it was built with", with the reasoning inline so it does not get re-asserted. Required-link failures were masked. Hoisting the applied verdict past `createLink` meant a link write that threw still reported success, so a caller would navigate to a task whose link does not exist. `applied` is now set only after the link lands: ancillary badge and geolocation failures still preserve success, the requested link does not. CHANGELOG entries moved to 0.9.1069, the version current since main's bump; the entries from #3579 and #3583 stay under 0.9.1068 where they shipped.
Item 3 of
lotti3-6fz— the last of the three deferrals from therelationship-links redesign that is a straightforward fix.
The problem
The link modal stacks a search field directly above a dropdown. The two were
built independently and had drifted:
DesignSystemSearchsurface.enabled(translucent overlay)decorative.level01, hairlinel(16)DesignSystemDropdownbackground.level01(opaque)decorative.level02, 2pxxl(24)Four pixels apart, the pair read as controls from two different products.
The fill is the load-bearing half. On an elevated surface — every modal this
control appears in — an opaque
background.level01field is a dark sunkenhole in dark theme, while the search field beside it sits a touch lighter than
its host. Two fields, opposite reactions to the same sheet.
The fix
The dropdown moves to the search field's shell. It has two real call sites (the
relationship selector and the AI settings header bar) against the search field's
sixteen, and the search field's treatment is the better reasoned of the two —
its "elevation-aware overlay rather than an absolute background level" comment
is exactly the argument the dropdown loses.
Two differences are kept deliberately:
interactive.enabled. The search field has no focus treatment to match, anddropping an open-state signal to win a cosmetic match would be a bad trade.
value where a small search field holds one line. That difference is honest.
Result
The guard
Nothing enforced the match, which is why it drifted. The new test renders both
components in one tree and asserts fill, border and radius agree — against each
other and against the tokens, so a change moving both in the same wrong
direction still fails rather than agreeing vacuously. Verified it bites by
reverting the fill: both theme cases fail, and pass again on restore.
The contract is written down in the design-system README under "One Field
Surface", including why the two kept differences are kept.
fvm flutter analyzeclean. 227 tests across dropdowns, search, linked-tasksand AI settings green. Every added line in the diff is covered.