Add searchable session tags to cmd-p workspace switcher#1089
Open
apollow wants to merge 2 commits intomanaflow-ai:mainfrom
Open
Add searchable session tags to cmd-p workspace switcher#1089apollow wants to merge 2 commits intomanaflow-ai:mainfrom
apollow wants to merge 2 commits intomanaflow-ai:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@apollow is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds per-source workspace tagging, extracts tags from Claude hook summaries, persists tags in session snapshots, threads tags into command-palette search and fingerprinting, and exposes tag management via v2 socket commands and CLI commands with a feature flag to enable session tag extraction. Changes
sequenceDiagram
participant ClaudeHook as Claude Hook
participant Extractor as ClaudeHookTagExtractor
participant Workspace as Workspace (tagsBySource)
participant Terminal as TerminalController / v2 socket
ClaudeHook->>Extractor: produce summary (on notify / prompt-submit)
Extractor->>Extractor: validate sessionId, filter tokens, dedupe tags
alt tags found and sessionTagsEnabled
Extractor->>Workspace: setTags(tags, source: "claude:<sessionId>")
Extractor->>Terminal: (optional) emit v2 event / CLI update
else no tags or disabled
Extractor->>Workspace: clearTags(source: "claude:<sessionId>")
end
Note right of Workspace: tags persisted to SessionWorkspaceSnapshot and used by search/indexing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:449">
P1: PII sanitization is ineffective: `delimiters` splits tokens on `-`, `.`, `/`, `_`, and space *before* `isSensitive()` runs, so the UUID, email, API-token, and path regexes can never match. For example, a UUID is split into five hex fragments that all pass through as tags. Either move the sensitivity check to run on the raw `combined` string before splitting (e.g., redact matches in-place), or remove conflicting characters from the delimiter set and split in two passes.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:3141">
P2: `compactMap` silently drops non-string elements, so `{"tags": [1, 2]}` passes the guard but sets empty tags on the workspace. Consider validating that all elements are strings (e.g., `as? [String]`) or at minimum that the resulting array is non-empty.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:1596">
P2: Duplicate tags are not filtered, so repeated values consume the per-source quota without adding search value. Since tags are auto-extracted from notification content, duplicates are likely. Consider deduplicating after sanitization.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR introduces searchable session tags for the cmd-p workspace switcher by adding per-source tag storage on Key changes:
Notable concern: The PII sensitive-pattern filter ( Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CC as Claude Code Hook (CLI)
participant TC as TerminalController (V2 Socket)
participant WS as Workspace
participant CP as cmd-p UI
Note over CC,CP: Tag Extraction Flow (notification event)
CC->>CC: extractTags(subtitle, body)<br/>tokenize + PII filter
CC->>TC: workspace.set_tags {workspace_id, source, tags}
TC->>WS: setTags(tags, source)
WS->>WS: sanitize + store in tagsBySource
WS-->>CP: @Published tagsBySource triggers re-render
CP->>CP: searchTags flatMap → tag footnote + search index
Note over CC,CP: Session Stop Flow
CC->>TC: workspace.clear_tags {workspace_id, source}
TC->>WS: clearTags(source)
WS->>WS: tagsBySource.removeValue(forKey: source)
Note over CC,CP: Manual CLI Flow
CC->>TC: workspace.set_tags / workspace.clear_tags
TC->>WS: setTags / clearTags / clearAllTags
Last reviewed commit: 3e99320 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+437
to
+449
| private static let sensitivePatterns: [NSRegularExpression] = { | ||
| let patterns = [ | ||
| "^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$", | ||
| "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$", | ||
| "^(sk-|pk-|ghp_|gho_|Bearer |token )", | ||
| "^(/|~/).+/", | ||
| "^[A-Z_]{2,}=", | ||
| "^[0-9]{10,}$" | ||
| ] | ||
| return patterns.compactMap { try? NSRegularExpression(pattern: $0, options: .caseInsensitive) } | ||
| }() | ||
|
|
||
| private static let delimiters = CharacterSet(charactersIn: "/\\.:_- ,;()[]{}\"'`") |
There was a problem hiding this comment.
PII filter patterns are defeated by prior tokenization
sensitivePatterns includes patterns for email addresses (^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$) and filesystem paths (^(/|~/).+/), but the delimiters CharacterSet on line 449 contains ., /, and \. This means that before isSensitive is ever called, emails and paths are already split into sub-tokens that individually do not match these patterns.
Concrete example:
- Input notification body:
"Edit /home/john@company.com/secrets" - After
components(separatedBy: delimiters):["Edit", "home", "john@company", "com", "secrets"] "john@company"is tested against the email regex — it has no TLD, so it does not match and passes through as a tag"home","secrets"also pass through
The email pattern would only match a complete user@domain.tld token, which can never exist after splitting on .. Similarly, the path pattern ^(/|~/).+/ requires a leading / and a trailing /, but / is a delimiter so no token can start with /. Both these sensitive patterns are effectively dead code.
Consider applying isSensitive to the raw combined string before tokenizing (or constructing patterns around the token-split fragments, e.g., checking for ^[a-zA-Z0-9._%+-]+@ to catch partial emails).
Author
There was a problem hiding this comment.
Addressed — pre-redact sensitive spans on the full combined string before tokenization using unanchored patterns, so multi-part tokens like UUIDs/emails/paths are caught before splitting.
Comment on lines
+1591
to
+1593
| var searchTags: [String] { | ||
| tagsBySource.values.flatMap { $0 } | ||
| } |
There was a problem hiding this comment.
Non-deterministic tag display order
Dictionary.values has no guaranteed iteration order in Swift, so tagsBySource.values.flatMap { $0 } produces tags in a different order each time the dictionary is mutated (e.g., between app launches or after any setTags call). This means the tag footnote rendered in cmd-p rows can shuffle unpredictably.
Consider sorting by source key to get a stable, deterministic order:
Suggested change
| var searchTags: [String] { | |
| tagsBySource.values.flatMap { $0 } | |
| } | |
| var searchTags: [String] { | |
| tagsBySource.keys.sorted().flatMap { tagsBySource[$0] ?? [] } | |
| } |
Author
There was a problem hiding this comment.
Addressed — searchTags now iterates keys in sorted order and deduplicates via seen-set for deterministic output.
Comment on lines
+1595
to
+1611
| func setTags(_ tags: [String], source: String) { | ||
| let sanitized = tags | ||
| .map { String($0.trimmingCharacters(in: .whitespacesAndNewlines).prefix(Self.maxTagLength)) } | ||
| .filter { !$0.isEmpty } | ||
| .prefix(Self.maxTagsPerSource) | ||
| var updated = tagsBySource | ||
| updated[source] = Array(sanitized) | ||
| if updated.count > Self.maxTagSources { | ||
| let sortedKeys = updated.keys.sorted() | ||
| for key in sortedKeys { | ||
| if key != source, updated.count > Self.maxTagSources { | ||
| updated.removeValue(forKey: key) | ||
| } | ||
| } | ||
| } | ||
| tagsBySource = updated | ||
| } |
There was a problem hiding this comment.
source key has no length bound
Individual tag strings are capped at maxTagLength (100 chars), but the source key stored in tagsBySource has no length validation. A caller (e.g. via the CLI --source flag or an automated hook) could pass an arbitrarily long source key, which would be stored in the dictionary indefinitely and serialized to the session snapshot.
Consider adding a length check:
Suggested change
| func setTags(_ tags: [String], source: String) { | |
| let sanitized = tags | |
| .map { String($0.trimmingCharacters(in: .whitespacesAndNewlines).prefix(Self.maxTagLength)) } | |
| .filter { !$0.isEmpty } | |
| .prefix(Self.maxTagsPerSource) | |
| var updated = tagsBySource | |
| updated[source] = Array(sanitized) | |
| if updated.count > Self.maxTagSources { | |
| let sortedKeys = updated.keys.sorted() | |
| for key in sortedKeys { | |
| if key != source, updated.count > Self.maxTagSources { | |
| updated.removeValue(forKey: key) | |
| } | |
| } | |
| } | |
| tagsBySource = updated | |
| } | |
| func setTags(_ tags: [String], source: String) { | |
| let trimmedSource = String(source.trimmingCharacters(in: .whitespacesAndNewlines).prefix(Self.maxTagLength)) | |
| guard !trimmedSource.isEmpty else { return } | |
| let sanitized = tags | |
| .map { String($0.trimmingCharacters(in: .whitespacesAndNewlines).prefix(Self.maxTagLength)) } | |
| .filter { !$0.isEmpty } | |
| .prefix(Self.maxTagsPerSource) | |
| var updated = tagsBySource | |
| updated[trimmedSource] = Array(sanitized) | |
| if updated.count > Self.maxTagSources { | |
| let sortedKeys = updated.keys.sorted() | |
| for key in sortedKeys { | |
| if key != trimmedSource, updated.count > Self.maxTagSources { | |
| updated.removeValue(forKey: key) | |
| } | |
| } | |
| } | |
| tagsBySource = updated | |
| } |
Author
There was a problem hiding this comment.
Addressed — added maxSourceLength = 200 constant, source strings are truncated in setTags.
Comment on lines
+6384
to
+6390
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | ||
| ClaudeHookTagExtractor.isValidSessionId(sessionId) { | ||
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | ||
| "workspace_id": workspaceId, | ||
| "source": "claude:\(sessionId)" | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Tags cleared unconditionally on session stop regardless of feature flag
The workspace.clear_tags call fires for every stop/idle event even when cmdPSessionTags is disabled. When the feature is turned off, the notification handler never sets tags (it checks isSessionTagsEnabled()), so this clear is a no-op network round-trip for every session stop.
This is not a correctness bug (it's safe to clear tags that don't exist), but it's slightly wasteful. Consider guarding this with the same isSessionTagsEnabled() check used in the notification handler:
Suggested change
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | |
| ClaudeHookTagExtractor.isValidSessionId(sessionId) { | |
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | |
| "workspace_id": workspaceId, | |
| "source": "claude:\(sessionId)" | |
| ]) | |
| } | |
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | |
| ClaudeHookTagExtractor.isValidSessionId(sessionId), | |
| ClaudeHookTagExtractor.isSessionTagsEnabled() { | |
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | |
| "workspace_id": workspaceId, | |
| "source": "claude:\(sessionId)" | |
| ]) | |
| } |
Author
There was a problem hiding this comment.
Addressed — gated the clear_tags call in claude-hook stop behind isSessionTagsEnabled() check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
1449-1477: Keeptagsin the command model’s searchable surface.
CommandPaletteCommand.tagscurrently affects rendering only. Search still depends on each caller remembering to duplicate those values intokeywords, which is easy to miss the next time a tagged command type is added.Suggested fix
var searchableTexts: [String] { - [title, subtitle] + keywords + [title, subtitle] + keywords + tags }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1449 - 1477, The searchable surface is missing tags, so update the CommandPaletteCommand model by including its tags in the searchableTexts computed property (in addition to title, subtitle and keywords) so tagged commands are found by search; locate the searchableTexts property and append the tags array to the returned array (ensure tags is included in the concatenation with [title, subtitle] + keywords + tags).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 449-479: extractTags currently tokenizes the combined text then
filters tokens with isSensitive, which lets sensitive substrings (paths, env
vars) leak after splitting; instead, pre-redact sensitive spans on the full
combined string before tokenization: in extractTags (and using
sensitivePatterns/isSensitive) scan the combined = "\(body) \(subtitle)" and
replace any matches of sensitivePatterns (e.g., path, env var regexes) with a
placeholder or remove them, then tokenize the redacted combined string using
delimiters and continue with the existing trimming/stopword/length de-dup logic;
keep the sensitivePatterns and isSensitive helpers but use them to perform
redaction up-front.
- Around line 1450-1467: The command currently allows --clear alongside
positional tags (isClear, rem2, tagArgs); change the logic so if isClear is true
and there are any non-empty positional tags (compute tagArgs from rem2 as
currently done), throw a CLIError indicating that --clear is mutually exclusive
with tags instead of proceeding to clear; update the error message (e.g., in the
set-workspace-tags handling) and perform this check before calling
client.sendV2/ workspace.clear_tags.
- Around line 6463-6473: The code currently only sends workspace.set_tags when
ClaudeHookTagExtractor.extractTags(...) yields non-empty tags, so previously-set
Claude tags persist; modify the block around
ClaudeHookTagExtractor.extractTags(subtitle:body:) so that after extracting tags
you always call client.sendV2(method: "workspace.set_tags", params:
["workspace_id": workspaceId, "source": "claude:\(sessionId)", "tags": tags]) —
i.e., if tags.isEmpty send the same API call with an empty tags array to clear
the Claude source for that session; keep using
ClaudeHookTagExtractor.isValidSessionId(sessionId) and isSessionTagsEnabled()
guards and reuse workspaceId, sessionId, client.sendV2 identifiers.
- Around line 1477-1486: The clear-workspace-tags branch currently ignores
leftover argv (variables rem0) so stray arguments like "extra" are accepted;
after parsing options (the parseOption calls that produce wsArg, rem0 and
srcArg), validate that rem0 contains no additional non-empty tokens and if it
does, return a usage/error (fail fast) before calling resolveWorkspaceId and
client.sendV2; update the "clear-workspace-tags" case to check rem0.isEmpty (or
equivalent) and emit a clear error/exit when extra args are present to prevent
accidental destructive actions.
In `@Sources/ContentView.swift`:
- Around line 6055-6057: The code is unconditionally adding "claude" when any
tagTokens exist, causing false matches; update the block that checks tagTokens
so it no longer appends "claude" unconditionally (keep "tag" and "topic"), and
instead only append "claude" when the tag provenance indicates it was created by
Claude (e.g., check the tag/source metadata used in your tag model or token
generation and append "claude" only when that metadata equals the Claude
origin); update the logic around tagTokens and contextKeywords to reflect this
conditional append.
In `@Sources/TerminalController.swift`:
- Around line 3138-3141: The handler currently uses compactMap on params["tags"]
(tagsRaw) which silently accepts mixed-type arrays (e.g., ["foo", 123]); change
validation to reject the entire request if any element is not a String (return
.err with an "invalid_params" message like "tags must be an array of strings"),
then convert to [String] only after validation and call Workspace.setTags as
before but return the canonical tags from the workspace (the
persisted/normalized/trimmed result) instead of echoing the input; apply the
same strict validation-and-return-the-canonical-tags change to the other tags
handling block at the indicated region (the code around Workspace.setTags).
- Around line 3175-3184: The code currently treats a malformed or blank "source"
as absent because v2String(params, "source") returns nil, which causes
workspace.clearAllTags() to run; change the logic to first check whether the
"source" key is present (e.g. using the existing v2HasKey or equivalent), and if
present but v2String returns nil, return an "invalid_params" error instead of
calling clearAllTags(); only call workspace.clearTags(source:) when v2String
returns a non-nil String, and call workspace.clearAllTags() only when the
"source" key is truly absent — update the block inside v2MainSync accordingly
(referencing v2String, v2HasKey, v2MainSync, workspace.clearTags and
workspace.clearAllTags).
In `@Sources/Workspace.swift`:
- Around line 1595-1611: In setTags(_:source:) sanitize and limit the incoming
tags as currently done, but if the resulting sanitized collection is empty treat
this as a clear operation: do not store an empty array for the given source in
tagsBySource (remove the key from updated or skip setting it) so empty writes
don't count toward Self.maxTagSources or get serialized; otherwise continue to
set updated[source] = Array(sanitized) and apply the existing eviction logic
that uses Self.maxTagSources, maxTagsPerSource and maxTagLength.
- Around line 197-199: The snapshot restoration currently assigns
snapshot.tagsBySource directly to tagsBySource, bypassing normalization and
failing to clear tags when the snapshot value is nil; instead iterate
snapshot.tagsBySource (for each source,keyedTags) and call setTags(_:source:) to
apply trimming/casing rules and ensure normalization, and when
snapshot.tagsBySource is nil or a source is missing, call setTags([], source:)
or otherwise clear that source so stale in-memory tags are removed; update the
restore path around tagsBySource and reference the tagsBySource property and
setTags(_:source:) method to implement this behavior.
- Around line 1591-1593: The computed property searchTags currently flattens
tagsBySource which yields non-deterministic order and duplicates; update
searchTags to collect all tags from tagsBySource.values, deduplicate them (e.g.,
via a Set) and then return a deterministically ordered array (e.g., sorted()) so
the command-palette fingerprint is stable; reference the searchTags property and
tagsBySource dictionary when applying this change.
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 1449-1477: The searchable surface is missing tags, so update the
CommandPaletteCommand model by including its tags in the searchableTexts
computed property (in addition to title, subtitle and keywords) so tagged
commands are found by search; locate the searchableTexts property and append the
tags array to the returned array (ensure tags is included in the concatenation
with [title, subtitle] + keywords + tags).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4fb3a62-3392-4c0b-9d8d-84c9d7de2d82
📒 Files selected for processing (6)
CLI/cmux.swiftSources/ContentView.swiftSources/SessionPersistence.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swift
…tion, deterministic ordering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:1609">
P2: Truncating the `source` key in `setTags` breaks readback/clear behavior for long source values because other paths still use the untruncated key.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| func setTags(_ tags: [String], source: String) { | ||
| let truncatedSource = String(source.prefix(Self.maxSourceLength)) |
There was a problem hiding this comment.
P2: Truncating the source key in setTags breaks readback/clear behavior for long source values because other paths still use the untruncated key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 1609:
<comment>Truncating the `source` key in `setTags` breaks readback/clear behavior for long source values because other paths still use the untruncated key.</comment>
<file context>
@@ -1587,22 +1590,44 @@ final class Workspace: Identifiable, ObservableObject {
- .map { String($0.trimmingCharacters(in: .whitespacesAndNewlines).prefix(Self.maxTagLength)) }
- .filter { !$0.isEmpty }
- .prefix(Self.maxTagsPerSource)
+ let truncatedSource = String(source.prefix(Self.maxSourceLength))
+ var seen = Set<String>()
+ var deduped = [String]()
</file context>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
197-202:⚠️ Potential issue | 🟡 MinorStale in-memory tags persist when snapshot lacks tag data.
When
snapshot.tagsBySourceisnil(e.g., restoring an older snapshot format), the entire block is skipped, leaving any existing in-memory tags untouched. This can cause stale tags from a previous session to linger.Consider unconditionally clearing tags, then restoring if present:
💡 Suggested fix
- if let restoredTags = snapshot.tagsBySource { - tagsBySource = [:] - for (source, tags) in restoredTags { - setTags(tags, source: source) - } - } + tagsBySource = [:] + if let restoredTags = snapshot.tagsBySource { + for source in restoredTags.keys.sorted() { + setTags(restoredTags[source] ?? [], source: source) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 197 - 202, Clear the in-memory tags unconditionally before attempting to restore from the snapshot so stale tags don't persist when snapshot.tagsBySource is nil; specifically, move the tagsBySource = [:] (or call whatever clear/reset method for tags) outside the if-let block and then, if snapshot.tagsBySource is present, iterate and call setTags(_:source:) to restore them (referencing snapshot.tagsBySource, tagsBySource, and setTags(_:source:)).
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
1475-1476: Avoid double-indexing raw tags in the search corpus.For switcher entries,
keywordsalready include the raw tag plus tokenized components viaCommandPaletteSwitcherSearchIndexer.tagTokensForSearch(_). Appendingtagsagain here doesn’t improve matching, but it does increasenormalizedSearchableTextsand fuzzy-match work on every query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1475 - 1476, The searchableTexts computed property currently returns [title, subtitle] + keywords + tags which double-includes raw tag strings because keywords already contains raw tags and their tokenized components (built by CommandPaletteSwitcherSearchIndexer.tagTokensForSearch(_:)); remove the appended tags from searchableTexts so it becomes [title, subtitle] + keywords to avoid duplicate indexing and excess work during normalizedSearchableTexts/fuzzy matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 6415-6422: The cleanup code should always call
workspace.clear_tags when a valid sessionId exists; remove the gate on
ClaudeHookTagExtractor.isSessionTagsEnabled() so that if parsedInput.sessionId
?? consumedSession?.sessionId yields a sessionId and
ClaudeHookTagExtractor.isValidSessionId(sessionId) is true, you unconditionally
call client.sendV2(method: "workspace.clear_tags", params: ["workspace_id":
workspaceId, "source": "claude:\(sessionId)"]) (keep the existing try?
behavior), ensuring stale claude:<sessionId> tags are always cleared on
stop/idle.
- Around line 1470-1489: The code currently computes isClear and scans rem2
without honoring a "--" terminator; update the logic around isClear/rem2/tagArgs
so tokens after the first "--" are treated as positional-only: split rem1 at the
first "--" into beforeTerm and afterTerm (preserve afterTerm as literal
positional tags), determine isClear and check unknown "--*" flags only within
beforeTerm, build positionalTags/tagArgs by combining afterTerm with non-flag,
non-empty tokens from beforeTerm (or simply using afterTerm when "--" is
present), and then proceed to call client.sendV2 or construct tags using the
tagArgs variable; adjust the checks that throw CLIError to reference beforeTerm
instead of the full rem1/rem2.
In `@Sources/ContentView.swift`:
- Around line 3077-3078: The tag separator is currently a hardcoded literal in
the Text created from result.command.tags.joined(separator: " · "); update the
join to use a localized separator by creating a localized string (via
String(localized: "tag.separator", defaultValue: " · ")) and pass that as the
separator to result.command.tags.joined(...), ensuring the Text(...) uses the
localized-joined string instead of a bare literal; add the "tag.separator" key
to localization resources as needed.
In `@Sources/Workspace.swift`:
- Around line 1638-1640: clearTags currently uses the raw source string key
while setTags truncates/normalizes sources to maxSourceLength before storing,
causing mismatched keys; update clearTags to apply the same
truncation/normalization logic (using maxSourceLength and the same normalization
routine used by setTags) before calling tagsBySource.removeValue(forKey:),
referencing the clearTags and setTags functions and the maxSourceLength constant
so the key lookup/removal matches stored entries.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 197-202: Clear the in-memory tags unconditionally before
attempting to restore from the snapshot so stale tags don't persist when
snapshot.tagsBySource is nil; specifically, move the tagsBySource = [:] (or call
whatever clear/reset method for tags) outside the if-let block and then, if
snapshot.tagsBySource is present, iterate and call setTags(_:source:) to restore
them (referencing snapshot.tagsBySource, tagsBySource, and setTags(_:source:)).
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 1475-1476: The searchableTexts computed property currently returns
[title, subtitle] + keywords + tags which double-includes raw tag strings
because keywords already contains raw tags and their tokenized components (built
by CommandPaletteSwitcherSearchIndexer.tagTokensForSearch(_:)); remove the
appended tags from searchableTexts so it becomes [title, subtitle] + keywords to
avoid duplicate indexing and excess work during normalizedSearchableTexts/fuzzy
matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aac0f732-c444-4444-baf2-efcfa5250895
📒 Files selected for processing (4)
CLI/cmux.swiftSources/ContentView.swiftSources/TerminalController.swiftSources/Workspace.swift
Comment on lines
+1470
to
+1489
| let isClear = rem1.contains("--clear") | ||
| let rem2 = rem1.filter { $0 != "--clear" } | ||
| if let unknownFlag = rem2.first(where: { $0.hasPrefix("--") && $0 != "--" }) { | ||
| throw CLIError(message: "set-workspace-tags: unknown flag '\(unknownFlag)'") | ||
| } | ||
| let wsId = try resolveWorkspaceId(workspaceArg, client: client) | ||
| let source = srcArg ?? "manual" | ||
| let positionalTags = rem2.filter { !$0.hasPrefix("--") && !$0.isEmpty } | ||
| if isClear && !positionalTags.isEmpty { | ||
| throw CLIError(message: "set-workspace-tags: --clear cannot be combined with positional tag arguments") | ||
| } | ||
| if isClear { | ||
| let payload = try client.sendV2(method: "workspace.clear_tags", params: [ | ||
| "workspace_id": wsId, | ||
| "source": source | ||
| ]) | ||
| printV2Payload(payload, jsonOutput: jsonOutput, idFormat: idFormat, fallbackText: v2OKSummary(payload, idFormat: idFormat, kinds: ["workspace"])) | ||
| } else { | ||
| let tagArgs = rem2.dropFirst(rem2.first == "--" ? 1 : 0) | ||
| let tags = Array(tagArgs).filter { !$0.isEmpty } |
There was a problem hiding this comment.
Honor -- before treating --clear as a flag.
This branch still scans the entire remainder for --clear and unknown --* tokens, even after a -- terminator. That means cmux set-workspace-tags -- --clear clears tags instead of setting a literal tag, and ---prefixed tags can never be passed at all.
Suggested fix
- let isClear = rem1.contains("--clear")
- let rem2 = rem1.filter { $0 != "--clear" }
- if let unknownFlag = rem2.first(where: { $0.hasPrefix("--") && $0 != "--" }) {
+ let terminatorIndex = rem1.firstIndex(of: "--")
+ let preTerminator = terminatorIndex.map { Array(rem1[..<$0]) } ?? rem1
+ let postTerminator = terminatorIndex.map { Array(rem1[rem1.index(after: $0)...]) } ?? []
+
+ let isClear = preTerminator.contains("--clear")
+ let rem2 = preTerminator.filter { $0 != "--clear" }
+ if let unknownFlag = rem2.first(where: { $0.hasPrefix("--") }) {
throw CLIError(message: "set-workspace-tags: unknown flag '\(unknownFlag)'")
}
let wsId = try resolveWorkspaceId(workspaceArg, client: client)
let source = srcArg ?? "manual"
- let positionalTags = rem2.filter { !$0.hasPrefix("--") && !$0.isEmpty }
+ let positionalTags =
+ rem2.filter { !$0.isEmpty } +
+ postTerminator.filter { !$0.isEmpty }
if isClear && !positionalTags.isEmpty {
throw CLIError(message: "set-workspace-tags: --clear cannot be combined with positional tag arguments")
}
if isClear {
let payload = try client.sendV2(method: "workspace.clear_tags", params: [
@@
} else {
- let tagArgs = rem2.dropFirst(rem2.first == "--" ? 1 : 0)
- let tags = Array(tagArgs).filter { !$0.isEmpty }
+ let tags = positionalTags
guard !tags.isEmpty else {
throw CLIError(message: "set-workspace-tags requires at least one tag or --clear")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1470 - 1489, The code currently computes isClear
and scans rem2 without honoring a "--" terminator; update the logic around
isClear/rem2/tagArgs so tokens after the first "--" are treated as
positional-only: split rem1 at the first "--" into beforeTerm and afterTerm
(preserve afterTerm as literal positional tags), determine isClear and check
unknown "--*" flags only within beforeTerm, build positionalTags/tagArgs by
combining afterTerm with non-flag, non-empty tokens from beforeTerm (or simply
using afterTerm when "--" is present), and then proceed to call client.sendV2 or
construct tags using the tagArgs variable; adjust the checks that throw CLIError
to reference beforeTerm instead of the full rem1/rem2.
Comment on lines
+6415
to
+6422
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | ||
| ClaudeHookTagExtractor.isValidSessionId(sessionId), | ||
| ClaudeHookTagExtractor.isSessionTagsEnabled() { | ||
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | ||
| "workspace_id": workspaceId, | ||
| "source": "claude:\(sessionId)" | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Always clear Claude session tags on stop.
Cleanup is incorrectly gated on isSessionTagsEnabled(). If a session generated tags while the setting was on and the user disables it before stop/idle, this branch skips workspace.clear_tags, so stale claude:<sessionId> tags remain searchable after the session ends.
Suggested fix
- if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId,
- ClaudeHookTagExtractor.isValidSessionId(sessionId),
- ClaudeHookTagExtractor.isSessionTagsEnabled() {
+ if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId,
+ ClaudeHookTagExtractor.isValidSessionId(sessionId) {
_ = try? client.sendV2(method: "workspace.clear_tags", params: [
"workspace_id": workspaceId,
"source": "claude:\(sessionId)"
])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | |
| ClaudeHookTagExtractor.isValidSessionId(sessionId), | |
| ClaudeHookTagExtractor.isSessionTagsEnabled() { | |
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | |
| "workspace_id": workspaceId, | |
| "source": "claude:\(sessionId)" | |
| ]) | |
| } | |
| if let sessionId = parsedInput.sessionId ?? consumedSession?.sessionId, | |
| ClaudeHookTagExtractor.isValidSessionId(sessionId) { | |
| _ = try? client.sendV2(method: "workspace.clear_tags", params: [ | |
| "workspace_id": workspaceId, | |
| "source": "claude:\(sessionId)" | |
| ]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 6415 - 6422, The cleanup code should always call
workspace.clear_tags when a valid sessionId exists; remove the gate on
ClaudeHookTagExtractor.isSessionTagsEnabled() so that if parsedInput.sessionId
?? consumedSession?.sessionId yields a sessionId and
ClaudeHookTagExtractor.isValidSessionId(sessionId) is true, you unconditionally
call client.sendV2(method: "workspace.clear_tags", params: ["workspace_id":
workspaceId, "source": "claude:\(sessionId)"]) (keep the existing try?
behavior), ensuring stale claude:<sessionId> tags are always cleared on
stop/idle.
Comment on lines
+3077
to
+3078
| if !result.command.tags.isEmpty { | ||
| Text(result.command.tags.joined(separator: " · ")) |
There was a problem hiding this comment.
Localize the new tag separator.
This adds a new user-visible literal inside a SwiftUI Text. Please route the separator through String(localized:..., defaultValue:...) too.
🌐 Suggested fix
- Text(result.command.tags.joined(separator: " · "))
+ Text(
+ result.command.tags.joined(
+ separator: String(
+ localized: "commandPalette.switcher.tagsSeparator",
+ defaultValue: " · "
+ )
+ )
+ )As per coding guidelines "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text"); never use bare string literals in SwiftUI Text(), Button(), alert titles, or UI elements"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 3077 - 3078, The tag separator is
currently a hardcoded literal in the Text created from
result.command.tags.joined(separator: " · "); update the join to use a localized
separator by creating a localized string (via String(localized: "tag.separator",
defaultValue: " · ")) and pass that as the separator to
result.command.tags.joined(...), ensuring the Text(...) uses the
localized-joined string instead of a bare literal; add the "tag.separator" key
to localization resources as needed.
Comment on lines
+1638
to
+1640
| func clearTags(source: String) { | ||
| tagsBySource.removeValue(forKey: source) | ||
| } |
There was a problem hiding this comment.
clearTags should truncate source to match setTags key normalization.
setTags truncates the source to maxSourceLength before storing, but clearTags uses the raw source string. If a caller sets tags with a source exceeding 200 characters and later calls clearTags with the same original source string, the removal will fail because the stored key is truncated.
💡 Suggested fix
func clearTags(source: String) {
- tagsBySource.removeValue(forKey: source)
+ let truncatedSource = String(source.prefix(Self.maxSourceLength))
+ tagsBySource.removeValue(forKey: truncatedSource)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 1638 - 1640, clearTags currently uses
the raw source string key while setTags truncates/normalizes sources to
maxSourceLength before storing, causing mismatched keys; update clearTags to
apply the same truncation/normalization logic (using maxSourceLength and the
same normalization routine used by setTags) before calling
tagsBySource.removeValue(forKey:), referencing the clearTags and setTags
functions and the maxSourceLength constant so the key lookup/removal matches
stored entries.
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
1 participant
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.
Summary
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds searchable session tags to the cmd‑p workspace switcher so you can find workspaces by topic. Tags persist per workspace, can be set via CLI, and can be auto‑extracted from Claude Code (opt‑in).
New Features
tagsBySource) with session restore and limits (10 sources, 20 tags/source, 100 chars/tag).workspace.set_tagsandworkspace.clear_tags.cmux set-workspace-tagsandcmux clear-workspace-tagswith--source/--clear.cmdPSessionTags, off by default): auto-extracts tags from notifications, clears on session end, and filters PII.Bug Fixes
--clearwith tags, empty/invalid source).Written for commit e81bcce. Summary will update on new commits.
Summary by CodeRabbit