Skip to content

Disable provider order reset when already at default - #4592

Merged
steipete merged 5 commits into
steipete:mainfrom
elijahfriedman:fix/provider-sidebar-controls
Oct 19, 2026
Merged

steipete merged 5 commits into
steipete:mainfrom
elijahfriedman:fix/provider-sidebar-controls

Conversation

@elijahfriedman

Copy link
Copy Markdown
Contributor

Summary

  • Disable “Restore default order” when providers are already in the default order and alphabetical sorting is off.
  • Rename “Only enabled providers” to “Only show enabled providers” and update translations.
  • Add regression coverage for reset availability across sorting, filtering, reordering, and undo.

Validation

  • make test-fast FILTER=ProviderBrowsingTests: all tests passed.
  • make check passed.
  • ran and launched debug build, verified changes

Screenshot

Screenshot 2026-10-16 at 1 44 08 PM

@clawsweeper

clawsweeper Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T07:27:02.315600Z 6a79219 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 16, 2026
@clawsweeper

clawsweeper Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codex review: blocked before merge.

What this changes

This PR disables redundant provider-order restoration and clarifies the enabled-provider filter label across translations.

Example: Providers are in catalog order and alphabetical sorting is off.

  • Before: Restore default order remains clickable but does nothing.
  • After: Restore default order is disabled.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Direct visual evidence and a narrow, apparently correct implementation support the patch; the remaining question is whether the owner wants this presentation change.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (screenshot): The supplied screenshot from the reported launched debug build shows the Settings sorting menu with the new filter wording and disabled Restore command, directly covering the changed visible behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Preference · Worth it: Needs a maintainer decision · Fix scope: Complete
User problem: Settings offers a clickable reset command even when the current order already matches its result.
Reason: The change makes the command's availability clearer, but the existing no-op is safe and disabling it is an owner-level presentation preference.

Merge readiness

⛔ Blocked before merge - 1 item remains

Keep open for an owner decision on this small Settings preference change; the implementation preserves existing restore behavior, and no introduced correctness defect was found.

Priority: P3
Reviewed head: 870a3973a2c8e1719f9fe1f4114924a665f1d482
Owner decision: Required. See Decision needed.

Decision needed

  • Question: Should Restore default order be disabled when it would make no change, alongside the clearer enabled-provider filter label?
  • Recommendation: Accept the Settings polish: Use the shared restore predicate to disable the redundant command and adopt the clearer filter wording.
  • Why: Current behavior safely ignores redundant restoration; this changes presentation rather than repairing a broken ordering contract, and no owner approval of this PR's direction is recorded.

Before merge

  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

None.

Agent review details

How this fits together

The Settings sidebar reads provider order and sorting preferences from SettingsStore, displays reset availability, and sends explicit restore actions through the existing configuration and undo owners.

flowchart TD
Catalog[Provider catalog] --> Availability[Restore availability]
Config[Current provider order] --> Availability
Sorting[Alphabetical preference] --> Availability
Availability --> Sidebar[Settings sidebar menu]
Sidebar --> Restore[Restore and undo]
Restore --> Config
Loading

Technical review

Best possible solution:

Expose the existing restore predicate in the menu while retaining provider-order, filtering, and undo semantics if the owner accepts this polish.

Do we have a high-confidence way to reproduce the issue?

Current-main source establishes the clickable no-op: open the sorting menu with default provider order and alphabetical sorting off; no failing runtime or ordering contract is demonstrated.

Is this the best way to solve the issue?

The implementation is appropriately narrow because availability and restoration share the same order calculation without adding preference state or replacing the mutation owner.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against a91669f2ff97.

Provenance checked

  • Default-order calculation and undo keeps the original intent (3a45dc7: Provide reversible catalog-order restoration while preserving hidden plugin slots, provider values, and the existing alphabetical preference.)
  • Reset command availability and filter wording changes intended behavior with a stated reason (Improve provider browsing and reversible ordering #4440: Expose enabled-only filtering and default restoration beside the alphabetical toggle; the PR explains its new disabled state and wording.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

  • Verified introduced change: The pinned introduction diff adds a shared default-order calculation and menu availability without changing the restoration predicate or persistence operation; translations consistently use the new filter key. (Sources/CodexBar/SettingsStore+ProviderBrowsing.swift:22, 870a3973a2c8)
  • Current main behavior: Fetched main still presents an enabled Restore command while its handler returns false when order is already default and alphabetical sorting is off. (Sources/CodexBar/SettingsStore+ProviderBrowsing.swift:41, a91669f2ff97)
  • Supplied visual proof: The downloaded PR screenshot directly shows the renamed enabled-provider filter and greyed-out Restore default order command; the body reports launching the debug build. (870a3973a2c8)
  • Original feature contract: The merged feature at Improve provider browsing and reversible ordering #4440 established stable-ID restoration, undo, hidden-plugin preservation, and the existing alphabetical Boolean as authoritative; steipete's landing comment confirms that narrowed direction. (Sources/CodexBar/SettingsStore+ProviderBrowsing.swift:40, 3a45dc7cf7c2)
  • Focused coverage and unchanged storage: The added test exercises reset availability through filtering, alphabetical sorting, restoration, reordering, and undo; existing tests cover provider-value and opaque-plugin preservation, and the PR changes no stored format, key, or default. (Tests/CodexBarTests/ProviderBrowsingTests.swift:17, 870a3973a2c8)
  • Policy and inspection limits: The complete root AGENTS.md was applied; no nested AGENTS.md or maintainer-notes directory was found. An older sidebar commit object was unavailable with lazy fetching disabled, but current-main blame and supplied provenance establish the relevant ownership. No builds or tests were run, and the checkout remains clean. (AGENTS.md:1, 870a3973a2c8)

Likely related people:

  • Yuxin Qiao: Raw commit 3a45dc7 adds Sources/CodexBar/SettingsStore+ProviderBrowsing.swift:29 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3a45dc7cf7c2; files: Sources/CodexBar/SettingsStore+ProviderBrowsing.swift)

Review metrics

Metric Value Why it matters
Change distribution Production Swift +17/-4; tests +25; localization +23/-23 The small implementation shares the restoration calculation, with focused state-transition coverage and matching locale keys.

Labels

Label changes:

  • add P3: This is low-risk Settings polish; existing restoration already safely handles the redundant action.
  • add proof: sufficient: Contributor real behavior proof is sufficient.
  • add proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.

Label justifications:

  • P3: This is low-risk Settings polish; existing restoration already safely handles the redundant action.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • proof: sufficient: Contributor real behavior proof is sufficient.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 16, 2026, 2:00 PM ET / 18:00 UTC.

steipete added a commit that referenced this pull request Oct 19, 2026
@steipete
steipete merged commit 348ad23 into steipete:main Oct 19, 2026
3 checks passed
@steipete

Copy link
Copy Markdown
Owner

Merged in 348ad23, thanks @elijahfriedman! The provider-order Restore button now reflects whether there's anything to restore. We kept your shared default-order calculation and all translations, and simplified the selection/undo forwarding. It will ship in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants