Skip to content

fix: match the dropdown field surface to the search field#3583

Merged
matthiasn merged 2 commits into
mainfrom
feat/unify-field-surfaces
Jul 25, 2026
Merged

fix: match the dropdown field surface to the search field#3583
matthiasn merged 2 commits into
mainfrom
feat/unify-field-surfaces

Conversation

@matthiasn

Copy link
Copy Markdown
Owner

Item 3 of lotti3-6fz — the last of the three deferrals from the
relationship-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:

fill border radius
DesignSystemSearch surface.enabled (translucent overlay) decorative.level01, hairline l (16)
DesignSystemDropdown background.level01 (opaque) decorative.level02, 2px xl (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.level01 field 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 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:

  • expanded state — the dropdown border still switches to
    interactive.enabled. The search field has no focus treatment to match, and
    dropping an open-state signal to win a cosmetic match would be a bad trade.
  • height — the dropdown stays taller, because it stacks a label above its
    value where a small search field holds one line. That difference is honest.

Result

dark light
dark light

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 analyze clean. 227 tests across dropdowns, search, linked-tasks
and AI settings green. Every added line in the diff is covered.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

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).
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@matthiasn, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5a94cd5a-839d-4644-8c9a-ed8fe919b91c

📥 Commits

Reviewing files that changed from the base of the PR and between 997b20c and 86089d9.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • flatpak/com.matthiasn.lotti.metainfo.xml
  • lib/features/design_system/README.md
  • lib/features/design_system/components/dropdowns/design_system_dropdown.dart
  • lib/features/tasks/ui/linked_tasks/relationship_type_selector.dart
  • test/features/design_system/components/dropdowns/design_system_dropdown_test.dart
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/unify-field-surfaces

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@matthiasn
matthiasn force-pushed the feat/unify-field-surfaces branch from 033ef2c to 571e743 Compare July 25, 2026 13:54
@matthiasn

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread lib/features/design_system/components/dropdowns/design_system_dropdown.dart Outdated
Comment thread lib/features/design_system/README.md Outdated
Comment thread test/features/design_system/components/dropdowns/design_system_dropdown_test.dart Outdated
Comment thread CHANGELOG.md Outdated
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.
@matthiasn
matthiasn merged commit 9020493 into main Jul 25, 2026
13 of 14 checks passed
@matthiasn
matthiasn deleted the feat/unify-field-surfaces branch July 25, 2026 14:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread lib/features/tasks/ui/linked_tasks/relationship_type_selector.dart
matthiasn added a commit that referenced this pull request Jul 25, 2026
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.
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.

1 participant