Settings: tag/token list editor for repo/label fields#515
Merged
Conversation
dgershman
approved these changes
Jun 15, 2026
dgershman
left a comment
Collaborator
There was a problem hiding this comment.
Code & Security Review
Small, well-scoped UI refactor: 6 CSV TextFields replaced by a reusable TokenListEditor chip editor in AutomationSettingsView and WorkspaceFormView. The model fields were already [String], so there's no persistence change or migration to worry about.
Security Review
Strengths:
- No new persistence path, no new IPC, no new code-eval surface. Token strings are stored as-is and bound to existing
[String]fields. - Input is trimmed and exact-deduped on commit — no surprise quoting or escaping behavior.
- Wildcard semantics (
owner/*) are interpreted by downstream consumers, not by this editor, so the editor itself adds no new injection surface.
Concerns:
- None.
Code Quality
Strengths:
- Pure
TokenListEditor.adding(_:to:)cleanly separates split/trim/dedup logic from SwiftUI plumbing, and the new@Suite("TokenListEditor.adding")covers trim, empty-ignore, comma split, case-sensitive dedupe against existing + intra-commit, order preservation, and trailing comma. - Backspace-on-empty-removes-last-chip is handled via
.onKeyPress(.delete)becauseTextFielddoesn't emit a text change in that state — the inline comment (Packages/CrowUI/Sources/CrowUI/TokenListEditor.swift:91-93) calls out exactly that subtlety. AutomationSettingsViewwires.onChange { onSave?() }on each token array so chip add/remove triggers a single config write (commit batches multi-token paste into one array mutation).WorkspaceFormViewkeeps the existing "save on form submit" pattern. Behavior matches the previous CSV fields.- All six converted fields are listed in the PR description and match the diff; no leftover
parseCSV/csvJoinedreferences remain (verified via ripgrep). FlowLayoutis a small, self-contained macOS 14Layoutimpl with sensible single-row fallback whenproposal.widthis nil.
Greens (consider, not blocking):
- 🟢
ForEach(Array(tokens.enumerated()), id: \.offset)(TokenListEditor.swift:32) — offset-based identity means removing chip at index N visually morphs each subsequent chip's text rather than removing the deleted one. Sinceadding()guarantees uniqueness,id: \.element(or unzipping toForEach(tokens, id: \.self)and deriving the index) would give stable identity and cleaner removal animations. Not a correctness bug —remove(at:)guards withtokens.indices.contains(index). - 🟢
trimmingCharacters(in: .whitespaces)(TokenListEditor.swift:123) handles spaces/tabs but not newlines.TextFieldnormally rejects newlines, so this is fine in practice, but.whitespacesAndNewlineswould be slightly more paste-robust. - 🟢 Typing comma is consumed by
.onKeyPress(",") { … return .handled }(TokenListEditor.swift:87-90), so the comma never appears in the input field — intentional and documented, but mildly surprising for first-time users. A short caption hint (e.g., "Press Return or comma to add") would aid discoverability. - 🟢 Could not run
swift test --package-path Packages/CrowUIin this checkout —GhosttyKit.xcframeworkis missing a binary artifact (pre-existing environment issue, unrelated to this PR). Trusting the PR description's "tests pass" claim, which is consistent with the test code I read.
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Approve — driven by [0 Red, 0 Yellow, 4 Green] findings.
Replace the six comma-delimited TextFields in Settings with a reusable TokenListEditor bound to Binding<[String]>. Entries render as removable gold chips; add via type/paste (commit on Return or comma, split pasted "a, b, c" into multiple chips); Backspace on empty input removes the last chip. Trims, de-dupes (case-sensitive), and ignores empty input. Chips wrap via a small FlowLayout (macOS 14 Layout protocol). Styling borrows LinkChip's capsule look. The model fields are already [String], so this is bound directly with no persistence or schema change — existing comma-separated configs decode into the arrays and render as chips. - AutomationSettingsView: excludeReviewRepos, ignoreReviewLabels, excludeTicketRepos (drop the @State CSV mirrors; persist via onChange). - WorkspaceFormView: alwaysInclude, autoReviewRepos, excludeReviewRepos (switch @State to [String]; drop the parseCSV glue). - Add TokenListEditor.adding unit suite. Closes #513 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 4CBC6396-0AE1-4F5C-B64B-E201DBDE2BF1
TokenListEditor conforms to View, so the type is implicitly @mainactor; the static adding(_:to:) helper inherited that isolation. Under the package's Swift 6 language mode, the nonisolated test suite could not call it synchronously ("call to main actor-isolated static method in a synchronous nonisolated context"). Mark the pure helper nonisolated. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 4CBC6396-0AE1-4F5C-B64B-E201DBDE2BF1
The previous design embedded the text field inside the same wrapping flow as the chips, so accumulated chips crowded the typing area and made entry awkward. Switch to the conventional "add to a list" layout: a standard rounded-border TextField plus an Add button on top, with committed values listed as removable chips beneath. Commit on Return or Add; paste of "a, b, c" still splits into three chips; focus returns to the field after each add for rapid entry. Dropped the comma-key and backspace-to-remove interceptions that fought normal text editing (incl. paste). 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 4CBC6396-0AE1-4F5C-B64B-E201DBDE2BF1
e79ad81 to
aa1f655
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #513
What
Replaces the six comma-delimited
TextFields in Settings with a reusableTokenListEditor— entries render as removable chips, you add by typing/pasting, and you delete items individually.Fields converted (all already
[String]in the model)AutomationSettingsViewdefaults.excludeReviewReposAutomationSettingsViewdefaults.ignoreReviewLabelsAutomationSettingsViewdefaults.excludeTicketReposWorkspaceFormViewworkspace.alwaysIncludeWorkspaceFormViewworkspace.autoReviewReposWorkspaceFormViewworkspace.excludeReviewReposComponent (
Packages/CrowUI/Sources/CrowUI/TokenListEditor.swift)public struct TokenListEditor: Viewbound toBinding<[String]>.LinkChip's capsule styling) with anxbutton.a, b, csplits into three chips on commit..onKeyPress(.delete)—TextFielddoesn't report backspace when empty).FlowLayout(macOS 14Layoutprotocol). Includes a#Preview.TokenListEditor.adding(_:to:)helper.No persistence/schema change
The model fields are already
[String]; the CSV was only a text representation. The editor binds directly to the existing arrays, so existing comma-separated configs decode and render as chips with no migration. The per-fieldparseCSV/joinedglue is removed.Tests
swift test --package-path Packages/CrowUIpasses, including a new@Suite("TokenListEditor.adding")covering trim, empty-ignore, comma-split, case-sensitive de-dupe, intra-commit de-dupe, order, and trailing-comma.make appbuilds clean.🤖 Generated with Claude Code