Repository navigation
Add ExtensionKit right sidebar demo - #3101
lawrencecchen wants to merge 15 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an ExtensionKit right‑sidebar feature: new Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant Host as Host App (GhosttyTabs)
participant Store as RightSidebarExtensionDemoStore
participant Monitor as AppExtensionPoint.Monitor
participant Discovery as System Discovery
participant HostVC as EXHostViewController
participant Extension as ExtensionKit Extension
User->>Host: switch sidebar to ExtensionKit mode
Host->>Store: reload()
Store->>Monitor: start/refresh monitor (macOS 26+)
Monitor->>Discovery: query installed extensions
Discovery-->>Monitor: return identities
Monitor-->>Store: update identities & status
User->>Host: select identity from Picker
Host->>HostVC: configure(identity, sceneID)
HostVC->>Extension: activate scene / resume XPC connection
Extension-->>HostVC: ready / UI provided
HostVC-->>Host: present extension UI
User->>Extension: interact with sidebar UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5b59f907
ℹ️ 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".
| if sortedIdentities.isEmpty, !identities.isEmpty { | ||
| statusMessage = String( | ||
| localized: "rightSidebar.extensionDemo.settlingStatus", | ||
| defaultValue: "Extension discovery is settling. Keeping the last extension." | ||
| ) | ||
| return |
There was a problem hiding this comment.
When discovery transitions from non-empty to empty (for example after uninstalling or unregistering an extension), this early return preserves the old identities forever, so the UI keeps showing extensions that are no longer available. Because identities is never cleared, subsequent empty monitor updates keep hitting the same branch and the panel cannot recover to the true empty state without restarting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Empty snapshots now clear stale identities unless the only recoverable stale identity is the bundled demo extension that still exists on disk.
— Claude Code
| } | ||
|
|
||
| identities = sortedIdentities | ||
| if selectedIdentity == nil { |
There was a problem hiding this comment.
This guard does not repair a stale selectedIdentityID when the previously selected extension disappears but other identities remain. selectedIdentity falls back to identities.first, so the condition stays false and selectedIdentityID can point to a non-existent tag, leaving the picker bound to an invalid selection state instead of selecting an available extension.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Selection now resets whenever the selected identity is missing from the latest identity list.
— Claude Code
Greptile SummaryThis PR adds an ExtensionKit right sidebar demo host into the main app and ships a bundled demo extension ( Confidence Score: 5/5Safe to merge; all findings are P2 style/polish items with no correctness or data-integrity impact. No P0 or P1 issues found. The main concerns are the hardcoded debug extension point identifier (intentional for a debug-only demo), the [StaticString] type annotation quirk, the missing production extension point declaration, and the arm64-only default in the sample script — all P2. The build verification steps in the PR description confirm the feature works end-to-end. Sources/RightSidebarExtensionDemo.swift (StaticString typing), RightSidebarDemoExtension/Info.plist (hardcoded debug extension point), Resources/RightSidebarExtensionPoints.xcappextensionpoints (missing production entry) Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmux DEV App
participant Store as RightSidebarExtensionDemoStore
participant LS as lsregister
participant PK as pluginkit
participant Monitor as AppExtensionPoint.Monitor
participant Host as EXHostViewController
participant Ext as RightSidebarDemoExtension.appex
App->>Store: onAppear → reload()
Store->>LS: lsregister -f app.app (register host)
Store->>PK: pluginkit -a extension.appex (register ext)
Store->>Monitor: addAppExtensionPoint(identifier)
Monitor-->>Store: onChange (identities discovered)
Store->>Store: applyMonitorState → identities updated
Store->>Host: configure(identity, sceneID)
Host->>Ext: XPC connection established
Ext-->>Host: PrimitiveAppExtensionScene activated
Host-->>Store: hostViewControllerDidActivate
Store-->>App: statusMessage = Extension scene activated.
Reviews (1): Last reviewed commit: "feat: add ExtensionKit sidebar demo" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static let discoveryIdentifiers: [StaticString] = [ | ||
| "com.cmuxterm.app.debug.extkit.right-sidebar-panel", | ||
| "com.cmuxterm.right-sidebar-panel", | ||
| ] |
There was a problem hiding this comment.
[StaticString] type for discoveryIdentifiers is non-standard
AppExtensionPoint(identifier:) and AppExtensionIdentity.matching(appExtensionPointIDs:) take String, not StaticString. StaticString has no implicit conversion to String in Swift, so callers that iterate these arrays and pass elements directly to those APIs rely on whatever protocol conformance happens to resolve at compile time. The parallel legacyDiscoveryIdentifiers array is typed [StaticString] by inference for the same reason. Using [String] throughout would make the intent explicit and remove the type ambiguity.
| static let discoveryIdentifiers: [StaticString] = [ | |
| "com.cmuxterm.app.debug.extkit.right-sidebar-panel", | |
| "com.cmuxterm.right-sidebar-panel", | |
| ] | |
| static let discoveryIdentifiers: [String] = [ | |
| "com.cmuxterm.app.debug.extkit.right-sidebar-panel", | |
| "com.cmuxterm.right-sidebar-panel", | |
| ] |
| static let legacyDiscoveryIdentifiers = [ | ||
| identifier, | ||
| legacyIdentifier, | ||
| ] |
There was a problem hiding this comment.
legacyDiscoveryIdentifiers misleadingly contains the current debug identifier
The name legacyDiscoveryIdentifiers implies these are only the pre-26 / legacy identifiers, but the array includes identifier (the current debug extension point). loadLegacyIdentities() therefore queries both the debug and legacy extension point on every pre-macOS-26 path — which is correct behaviour, but the name suggests only the legacy identifier would be there. Consider renaming to allDiscoveryIdentifiers or just reusing discoveryIdentifiers (converting to [String]), since the two arrays are identical in content.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <key>EXAppExtensionAttributes</key> | ||
| <dict> | ||
| <key>EXExtensionPointIdentifier</key> | ||
| <string>com.cmuxterm.app.debug.extkit.right-sidebar-panel</string> |
There was a problem hiding this comment.
EXExtensionPointIdentifier is a static string rather than a build-setting variable. The Release Xcode configuration sets CMUX_HOST_BUNDLE_IDENTIFIER = com.cmuxterm.app, which would produce a production extension point of com.cmuxterm.app.extkit.right-sidebar-panel, but this plist always binds to com.cmuxterm.app.debug.extkit.right-sidebar-panel. The extension is embedded in Release builds (via the Embed ExtensionKit Extensions phase) but will silently fail to activate there because the extension point doesn't match. If the plan is for this to remain debug-only indefinitely, gating the embed phase on the Debug configuration or adding a comment would make the intent explicit.
There was a problem hiding this comment.
Fixed. The extension Info.plist now uses a build setting so Debug and Release resolve to their matching extension point identifiers.
— Claude Code
| EXTENSION_POINT_ID="com.cmuxterm.app.debug.extkit.right-sidebar-panel" | ||
| SCENE_ID="cmux-right-sidebar-demo" | ||
| SDK_PATH="$(xcrun --sdk macosx --show-sdk-path)" | ||
| TARGET_TRIPLE="${TARGET_TRIPLE:-arm64-apple-macos26.0}" |
There was a problem hiding this comment.
TARGET_TRIPLE="${TARGET_TRIPLE:-arm64-apple-macos26.0}" produces a binary that won't run on Intel Macs (x86_64). The script README should mention that Intel users need export TARGET_TRIPLE=x86_64-apple-macos26.0 before running, or add a platform check with a clear error message.
There was a problem hiding this comment.
Fixed. The BYO build script now defaults TARGET_TRIPLE from the host architecture and documents manual cross-build overrides.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
scripts/reload.sh (1)
528-550: Optional: scope loop/path variables withlocalinside the function.
UNTAGGED_APP_PATHandEXTENSION_BUNDLEare assigned withoutlocal, so they leak into the surrounding script scope after the function returns. It's harmless today (no later code reads those names), but addinglocalmakes this helper safer against future edits that add another variable of the same name outside.♻️ Proposed nit
register_extensionkit_extensions() { + local UNTAGGED_APP_PATH EXTENSION_BUNDLE if [[ ! -d "$APP_PATH/Contents/Extensions" ]]; then return fiOtherwise the sequence (guard → untagged-bundle cleanup under
TAG→lsregister -fon the current app →pluginkit -aper.appex, with a warning on failure) looks correct and matches theContents/Extensionscopy destination set up by theEmbed ExtensionKit Extensionsbuild phase inGhosttyTabs.xcodeproj/project.pbxproj.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/reload.sh` around lines 528 - 550, The function register_extensionkit_extensions leaks shell variables into the outer scope because UNTAGGED_APP_PATH and EXTENSION_BUNDLE are not declared local; update register_extensionkit_extensions to declare local UNTAGGED_APP_PATH (used when TAG is set) and local EXTENSION_BUNDLE (used in the while read loop) so their scope is limited to the function and they don't accidentally override or pollute variables elsewhere in the script.Samples/RightSidebarBYOExtension/Sources/Extension/RightSidebarSampleExtension.swift (1)
11-17:accept(connection:)unconditionally returnstrue(sample).For a BYO sample this is fine, but please add a comment here noting that a real extension should validate the incoming
NSXPCConnection(e.g. viaauditSessionIdentifier, code-signing requirement checks) before accepting. Otherwise downstream developers copying this scaffold inherit an always-accept XPC handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Samples/RightSidebarBYOExtension/Sources/Extension/RightSidebarSampleExtension.swift` around lines 11 - 17, Add a clarifying comment inside CmuxRightSidebarConfiguration's accept(connection:) implementation noting that the current stub returns true only for the sample and that real extensions must validate the incoming NSXPCConnection (e.g. check connection.auditSessionIdentifier, perform code‑signing or entitlement checks, verify remote process identity) before returning true; reference the accept(connection:) method and NSXPCConnection so maintainers replacing the stub know to implement actual validation logic in production.RightSidebarDemoExtension/RightSidebarDemoExtension.swift (1)
65-145: Consider aligning state types with the BYO sample (enums vs. raw Ints).The BYO sample at
Samples/RightSidebarBYOExtension/Sources/Extension/RightSidebarSampleExtension.swiftexpressesselectedTab/Mode/Model/Priority/ScopeasCaseIterableenums and drives pickers viaForEach(SampleTab.allCases). Mirroring that here would (a) fix thecopySnapshotbug noted separately for free, (b) remove the parallel.tag(0/1/2)lists at L118-120, L156-158, L274-276, L283-285, L347-350, and (c) keep the "canonical" vs. "BYO" demos genuinely parallel so the prompt text inSources/RightSidebarExtensionDemo.swift(starterPrompt) stays truthful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift` around lines 65 - 145, Replace the raw Int `@State` properties selectedTab, selectedMode, selectedModel, selectedPriority and selectedScope with CaseIterable enum types (e.g., SampleTab, SampleMode, SampleModel, SamplePriority, SampleScope) and update their `@State` declarations and initializers accordingly; change Picker usages (the ones using selection: $selectedTab / $selectedMode / $selectedModel / $selectedPriority / $selectedScope) to iterate with ForEach(SampleX.allCases) and tag each case with the enum value instead of hardcoded .tag(0/1/2), remove the parallel .tag(0/1/2) Text entries, and update any switch statements (e.g., switch selectedTab) to switch on the enum cases so the copySnapshot bug and parity with the BYO sample (RightSidebarSampleExtension.swift) are resolved.Sources/RightSidebarExtensionDemo.swift (1)
163-172: Add a clarifying comment toloadLegacyIdentities()explaining the one-shot snapshot pattern.
AppExtensionIdentity.matching(appExtensionPointIDs:)returns an async sequence where the first element is a snapshot of currently enabled extensions and subsequent elements are updates. The code correctly reads only the first batch viaawait iterator.next(), which matches the pattern used in the macOS 26.0+ monitor path (line 71). A short comment like// one-shot: relies on the monitor path on macOS 26+would clarify intent for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarExtensionDemo.swift` around lines 163 - 172, Add a short clarifying comment at the top of loadLegacyIdentities() explaining that AppExtensionIdentity.matching(appExtensionPointIDs:) returns an async sequence whose first element is a one-shot snapshot of currently enabled extensions and later elements are updates; note that this function intentionally consumes only the first batch via await iterator.next() to implement the one-shot snapshot pattern (matching the macOS 26+ monitor path used elsewhere, e.g., the monitor on line ~71), so readers understand why only the first element is used for identifiers from RightSidebarExtensionPoint.legacyDiscoveryIdentifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 1288-1345: The Debug and Release XCBuildConfiguration blocks
(named Debug and Release) enable ENABLE_APP_SANDBOX = YES but do not set
CODE_SIGN_ENTITLEMENTS; add a new entitlements file
RightSidebarDemoExtension/RightSidebarDemoExtension.entitlements and set
CODE_SIGN_ENTITLEMENTS =
RightSidebarDemoExtension/RightSidebarDemoExtension.entitlements in both the
Debug and Release buildSettings, ensuring the entitlements plist contains
com.apple.security.app-sandbox = true in both and
com.apple.security.get-task-allow = true only for the Debug entitlements so
signing and pluginkit registration succeed.
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 407-422: copySnapshot() is writing raw Int tag values for
selectedMode/selectedModel/selectedPriority to the clipboard; change it to emit
human-readable titles by translating those Ints to their enum representations
(or by switching the stored state to the enums). In copySnapshot(), map
selectedMode -> SampleMode(rawValue: selectedMode)?.title (or use a helper like
SampleMode.title(for: selectedMode)), similarly for selectedModel -> SampleModel
and selectedPriority -> SamplePriority, falling back to the numeric value if
mapping fails, and use those strings when building snapshot so the clipboard
shows localized titles (matching the BYO sample's .title accessors).
In `@Samples/RightSidebarBYOExtension/build-and-register.sh`:
- Line 13: The script hardcodes TARGET_TRIPLE="arm64-apple-macos26.0" which
forces cross-compilation on Intel hosts; change the default to auto-detect the
host arch (or build a universal binary) and use that value for the swiftc
invocations that produce the .appex before pluginkit registration. Specifically,
replace the fixed TARGET_TRIPLE default with logic that maps `uname -m` (e.g.,
x86_64 → x86_64-apple-macos26.0, arm64 → arm64-apple-macos26.0) or falls back to
building a universal binary, and ensure the swiftc command blocks that produce
the .appex use this computed TARGET_TRIPLE variable so pluginkit receives a
compatible architecture.
In `@Sources/RightSidebarExtensionDemo.swift`:
- Around line 174-213: The detached registration can hang because runProcess
waits indefinitely; change registerBundledDemoExtensionIfNeeded/runProcess so
the spawned subprocesses are launched without blocking the app: in runProcess
(or a new runProcessWithTimeout) set process.standardOutput and
process.standardError to a Pipe (or /dev/null) and start the process, then
schedule a termination after a short deadline (e.g. 5s) using
DispatchQueue.global().asyncAfter to call process.terminate() if still running,
and asynchronously wait for exit (or return a failure code if terminated);
update registerBundledDemoExtensionIfNeeded to call this timeout-aware runner
(or fire-and-forget the detached task and avoid awaiting its .value) so the
monitor creation and loadIdentities flow are not blocked by
lsregister/pluginkit.
---
Nitpick comments:
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 65-145: Replace the raw Int `@State` properties selectedTab,
selectedMode, selectedModel, selectedPriority and selectedScope with
CaseIterable enum types (e.g., SampleTab, SampleMode, SampleModel,
SamplePriority, SampleScope) and update their `@State` declarations and
initializers accordingly; change Picker usages (the ones using selection:
$selectedTab / $selectedMode / $selectedModel / $selectedPriority /
$selectedScope) to iterate with ForEach(SampleX.allCases) and tag each case with
the enum value instead of hardcoded .tag(0/1/2), remove the parallel .tag(0/1/2)
Text entries, and update any switch statements (e.g., switch selectedTab) to
switch on the enum cases so the copySnapshot bug and parity with the BYO sample
(RightSidebarSampleExtension.swift) are resolved.
In
`@Samples/RightSidebarBYOExtension/Sources/Extension/RightSidebarSampleExtension.swift`:
- Around line 11-17: Add a clarifying comment inside
CmuxRightSidebarConfiguration's accept(connection:) implementation noting that
the current stub returns true only for the sample and that real extensions must
validate the incoming NSXPCConnection (e.g. check
connection.auditSessionIdentifier, perform code‑signing or entitlement checks,
verify remote process identity) before returning true; reference the
accept(connection:) method and NSXPCConnection so maintainers replacing the stub
know to implement actual validation logic in production.
In `@scripts/reload.sh`:
- Around line 528-550: The function register_extensionkit_extensions leaks shell
variables into the outer scope because UNTAGGED_APP_PATH and EXTENSION_BUNDLE
are not declared local; update register_extensionkit_extensions to declare local
UNTAGGED_APP_PATH (used when TAG is set) and local EXTENSION_BUNDLE (used in the
while read loop) so their scope is limited to the function and they don't
accidentally override or pollute variables elsewhere in the script.
In `@Sources/RightSidebarExtensionDemo.swift`:
- Around line 163-172: Add a short clarifying comment at the top of
loadLegacyIdentities() explaining that
AppExtensionIdentity.matching(appExtensionPointIDs:) returns an async sequence
whose first element is a one-shot snapshot of currently enabled extensions and
later elements are updates; note that this function intentionally consumes only
the first batch via await iterator.next() to implement the one-shot snapshot
pattern (matching the macOS 26+ monitor path used elsewhere, e.g., the monitor
on line ~71), so readers understand why only the first element is used for
identifiers from RightSidebarExtensionPoint.legacyDiscoveryIdentifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e38a6e21-cff7-4a50-b132-a9c904a3cd15
📒 Files selected for processing (14)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsResources/RightSidebarExtensionPoints.xcappextensionpointsRightSidebarDemoExtension/Info.plistRightSidebarDemoExtension/InfoPlist.xcstringsRightSidebarDemoExtension/RightSidebarDemoExtension.swiftSamples/RightSidebarBYOExtension/README.mdSamples/RightSidebarBYOExtension/Resources/Localizable.xcstringsSamples/RightSidebarBYOExtension/Sources/ContainerApp/RightSidebarSampleContainerApp.swiftSamples/RightSidebarBYOExtension/Sources/Extension/RightSidebarSampleExtension.swiftSamples/RightSidebarBYOExtension/build-and-register.shSources/RightSidebarExtensionDemo.swiftSources/RightSidebarPanelView.swiftscripts/reload.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
RightSidebarDemoExtension/RightSidebarDemoExtension.swift (1)
408-423:⚠️ Potential issue | 🟡 MinorClipboard snapshot still uses raw picker tags.
Mode,Model, andPriorityare copied as0/1/2instead of localized titles like “Run”, “Fast”, or “Normal”. This matches the earlier unresolved review finding.🛠️ Minimal fix
private func copySnapshot() { + let modeTitles = [ + String(localized: "sampleExtension.mode.run", defaultValue: "Run"), + String(localized: "sampleExtension.mode.review", defaultValue: "Review"), + String(localized: "sampleExtension.mode.watch", defaultValue: "Watch"), + ] + let modelTitles = [ + String(localized: "sampleExtension.model.fast", defaultValue: "Fast"), + String(localized: "sampleExtension.model.balanced", defaultValue: "Balanced"), + String(localized: "sampleExtension.model.deep", defaultValue: "Deep"), + ] + let priorityTitles = [ + String(localized: "sampleExtension.priority.low", defaultValue: "Low"), + String(localized: "sampleExtension.priority.normal", defaultValue: "Normal"), + String(localized: "sampleExtension.priority.high", defaultValue: "High"), + ] + let modeTitle = modeTitles.indices.contains(selectedMode) ? modeTitles[selectedMode] : "\(selectedMode)" + let modelTitle = modelTitles.indices.contains(selectedModel) ? modelTitles[selectedModel] : "\(selectedModel)" + let priorityTitle = priorityTitles.indices.contains(selectedPriority) ? priorityTitles[selectedPriority] : "\(selectedPriority)" + let snapshot = [ String(localized: "sampleExtension.titleField", defaultValue: "Title") + ": \(title)", - String(localized: "sampleExtension.mode", defaultValue: "Mode") + ": \(selectedMode)", - String(localized: "sampleExtension.model", defaultValue: "Model") + ": \(selectedModel)", - String(localized: "sampleExtension.priority", defaultValue: "Priority") + ": \(selectedPriority)", + String(localized: "sampleExtension.mode", defaultValue: "Mode") + ": \(modeTitle)", + String(localized: "sampleExtension.model", defaultValue: "Model") + ": \(modelTitle)", + String(localized: "sampleExtension.priority", defaultValue: "Priority") + ": \(priorityTitle)", String(localized: "sampleExtension.batchSize", defaultValue: "Batch size") + ": \(batchSize)" ].joined(separator: "\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift` around lines 408 - 423, The snapshot currently interpolates the raw picker values (selectedMode, selectedModel, selectedPriority) producing numeric tags; update copySnapshot() to use the human/localized display strings for those pickers instead (e.g., call the existing label/title helpers or enum computed properties you use elsewhere such as modeTitle(for:), modelDisplayName(for:), or selectedMode.displayName) so the pasted snapshot shows localized names like “Run”, “Fast”, “Normal” instead of 0/1/2; ensure you reference the same mapping logic used by the UI pickers to keep text consistent.
🧹 Nitpick comments (1)
Sources/RightSidebarExtensionDemo.swift (1)
84-87: Error status strings concatenate outside the localization key.
String(localized: …) + " \(error.localizedDescription)"splits a user-facing message in two: the localized stem plus a hard-coded leading space and the unlocalized error. Translators cannot reorder the error relative to the stem, and the same pattern repeats at Lines 520–525.Prefer composing the full sentence via
defaultValueinterpolation so the whole string is a single localizable unit:♻️ Proposed refactor
- statusMessage = String( - localized: "rightSidebar.extensionDemo.errorStatus", - defaultValue: "Extension discovery failed." - ) + " \(error.localizedDescription)" + let description = error.localizedDescription + statusMessage = String( + localized: "rightSidebar.extensionDemo.errorStatus", + defaultValue: "Extension discovery failed. \(description)" + )Apply the same treatment to the
deactivatedWithErrorbranch at Lines 520–525.As per coding guidelines: "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarExtensionDemo.swift` around lines 84 - 87, The status message currently concatenates a localized stem with error.localizedDescription (assignment to statusMessage), which breaks localization; change the String(localized: ...) call to include the error via defaultValue interpolation (e.g., "Extension discovery failed: {0}" style) and pass the error.localizedDescription into the localized initializer so the entire sentence is one localizable unit; also apply the same change in the deactivatedWithError branch so both places produce a single String(localized: "…", defaultValue: "…\(error.localizedDescription)") call rather than concatenating after localization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 1-6: The top-level conditional currently only checks the compiler
version; update the conditional around the import block to also use
canImport(...) for the frameworks so the block is compiled only when the SDK
actually provides ExtensionFoundation and ExtensionKit (i.e., change the
directive that guards the import statements for ExtensionFoundation and
ExtensionKit to include canImport(ExtensionFoundation) &&
canImport(ExtensionKit) alongside compiler(>=6.3)), leaving the existing imports
(ExtensionFoundation, ExtensionKit, Foundation, AppKit, SwiftUI) intact.
---
Duplicate comments:
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 408-423: The snapshot currently interpolates the raw picker values
(selectedMode, selectedModel, selectedPriority) producing numeric tags; update
copySnapshot() to use the human/localized display strings for those pickers
instead (e.g., call the existing label/title helpers or enum computed properties
you use elsewhere such as modeTitle(for:), modelDisplayName(for:), or
selectedMode.displayName) so the pasted snapshot shows localized names like
“Run”, “Fast”, “Normal” instead of 0/1/2; ensure you reference the same mapping
logic used by the UI pickers to keep text consistent.
---
Nitpick comments:
In `@Sources/RightSidebarExtensionDemo.swift`:
- Around line 84-87: The status message currently concatenates a localized stem
with error.localizedDescription (assignment to statusMessage), which breaks
localization; change the String(localized: ...) call to include the error via
defaultValue interpolation (e.g., "Extension discovery failed: {0}" style) and
pass the error.localizedDescription into the localized initializer so the entire
sentence is one localizable unit; also apply the same change in the
deactivatedWithError branch so both places produce a single String(localized:
"…", defaultValue: "…\(error.localizedDescription)") call rather than
concatenating after localization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5608ce1a-3289-4d7f-a1fa-42082c643b15
📒 Files selected for processing (2)
RightSidebarDemoExtension/RightSidebarDemoExtension.swiftSources/RightSidebarExtensionDemo.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 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="Samples/RightSidebarBYOExtension/build-and-register.sh">
<violation number="1" location="Samples/RightSidebarBYOExtension/build-and-register.sh:140">
P0: Remove `--deep` from the container app signing step. Using `--deep` here will recurse into the already-signed `.appex` and re-sign it without entitlements, destroying the extension's sandbox and permissions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/RightSidebarExtensionDemo.swift (1)
205-241:⚠️ Potential issue | 🟠 Major
runProcessstill blocks the entire reload path without a timeout.
registerBundledDemoExtensionIfNeeded()is awaited fromloadIdentitiesbefore theAppExtensionPoint.Monitoris created, andrunProcess(L229-241) invokesprocess.waitUntilExit()with no deadline and no stdout/stderr redirection. Iflsregister -forpluginkit -ahang (Launch Services DB contention has been observed in the wild),isLoadingstaystrueindefinitely, the refresh spinner never clears, and the monitor never attaches.Prior suggestion still applies: either (a) add a termination deadline (e.g.,
DispatchQueue.global().asyncAfter+process.terminate()after ~5s) and pipe stdout/stderr to discardedPipes, or (b) fire-and-forget the registration (don't await.value) and let the monitor pick up the extension once registration finishes.🛠️ Minimal timeout + output-drain guard
nonisolated private static func runProcess(executablePath: String, arguments: [String]) -> Int32 { let process = Process() process.executableURL = URL(fileURLWithPath: executablePath) process.arguments = arguments + process.standardOutput = Pipe() + process.standardError = Pipe() do { try process.run() + DispatchQueue.global().asyncAfter(deadline: .now() + .seconds(5)) { [weak process] in + if process?.isRunning == true { process?.terminate() } + } process.waitUntilExit() return process.terminationStatus } catch { return -1 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarExtensionDemo.swift` around lines 205 - 241, The current runProcess(executablePath:arguments:) is synchronous and blocks because it calls process.waitUntilExit() with no timeout or stdout/stderr handling, causing registerBundledDemoExtensionIfNeeded() to hang the reload path; update runProcess (or its caller) to either (A) enforce a termination deadline: set process.standardOutput/standardError to Pipe() to drain outputs, start the process, schedule a DispatchQueue.global().asyncAfter (e.g., ~5s) to call process.terminate() if still running, then waitUntilExit and return terminationStatus (handle errors), or (B) change registerBundledDemoExtensionIfNeeded() to fire-and-forget the detached Task without awaiting .value so the app does not block while lsregister/pluginkit complete; modify the codepaths referencing runProcess and registerBundledDemoExtensionIfNeeded accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/RightSidebarExtensionDemo.swift`:
- Around line 205-241: The current runProcess(executablePath:arguments:) is
synchronous and blocks because it calls process.waitUntilExit() with no timeout
or stdout/stderr handling, causing registerBundledDemoExtensionIfNeeded() to
hang the reload path; update runProcess (or its caller) to either (A) enforce a
termination deadline: set process.standardOutput/standardError to Pipe() to
drain outputs, start the process, schedule a DispatchQueue.global().asyncAfter
(e.g., ~5s) to call process.terminate() if still running, then waitUntilExit and
return terminationStatus (handle errors), or (B) change
registerBundledDemoExtensionIfNeeded() to fire-and-forget the detached Task
without awaiting .value so the app does not block while lsregister/pluginkit
complete; modify the codepaths referencing runProcess and
registerBundledDemoExtensionIfNeeded accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e037bd-6641-4b34-a2eb-e89b4114b16c
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojResources/RightSidebarExtensionPoints.xcappextensionpointsRightSidebarDemoExtension/Info.plistRightSidebarDemoExtension/RightSidebarDemoExtension-Debug.entitlementsRightSidebarDemoExtension/RightSidebarDemoExtension.entitlementsRightSidebarDemoExtension/RightSidebarDemoExtension.swiftSamples/RightSidebarBYOExtension/README.mdSamples/RightSidebarBYOExtension/build-and-register.shSources/RightSidebarExtensionDemo.swift
✅ Files skipped from review due to trivial changes (6)
- RightSidebarDemoExtension/RightSidebarDemoExtension.entitlements
- RightSidebarDemoExtension/RightSidebarDemoExtension-Debug.entitlements
- Resources/RightSidebarExtensionPoints.xcappextensionpoints
- RightSidebarDemoExtension/Info.plist
- Samples/RightSidebarBYOExtension/README.md
- Samples/RightSidebarBYOExtension/build-and-register.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Resources/Localizable.xcstrings (1)
91452-91468: Duplicate "waiting for registration" strings.
rightSidebar.extensionDemo.registrationSettlingStatus("Bundled demo extension is installed. Waiting for macOS registration.") andrightSidebar.extensionDemo.waitingForBundledDemo("Bundled demo extension is installed. Waiting for macOS to finish registration.") are semantically identical. Consider collapsing to a single key to reduce translation drift and keep UI copy consistent.Also applies to: 91554-91570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 91452 - 91468, There are two duplicate localization keys for the same copy: rightSidebar.extensionDemo.registrationSettlingStatus and rightSidebar.extensionDemo.waitingForBundledDemo; pick one canonical key (e.g., rightSidebar.extensionDemo.waitingForBundledDemo), remove the other key's block, copy/merge its "en" and "ja" stringUnit values into the canonical key (ensuring wording is consistent), and update all code/IB strings to reference the chosen key; repeat the same collapse for the other duplicate range noted (91554-91570) so only a single localization entry exists to avoid translation drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 91231-91247: The deactivatedWithError localization duplicates
rightSidebar.extensionDemo.deactivated and therefore doesn't convey an error;
update rightSidebar.extensionDemo.deactivatedWithError to include a placeholder
(e.g., "Extension scene disconnected: %@") in both the "en" and "ja"
stringUnit.value (use a suitable Japanese template like "拡張シーンが切断されました：%@"), or
remove the redundant deactivatedWithError key and consolidate call sites to use
rightSidebar.extensionDemo.deactivated; also ensure the code paths that call the
deactivatedWithError key (where an error object exists) are updated to pass the
error.localizedDescription into the placeholder.
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 193-200: The icon-only reset Button (the Button invoking
resetControls() with Image(systemName: "arrow.counterclockwise")), the
TextEditor, the ProgressView, and the Slider need explicit VoiceOver semantics:
add .accessibilityLabel(String(localized: "sampleExtension.reset", defaultValue:
"Reset")) to the reset Button (instead of relying on .help), and for the
TextEditor, ProgressView and Slider add appropriate .accessibilityLabel(...) and
where applicable .accessibilityValue(...) using localized strings (e.g.,
"sampleExtension.textEditor", "sampleExtension.progress",
"sampleExtension.slider") and bind the slider/value strings to the current
numeric/percentage state so screen readers announce meaningful names and values.
Ensure you use the existing localized keys and state properties and attach these
modifiers directly to the controls (Button, TextEditor, ProgressView, Slider)
referenced in the diff.
---
Nitpick comments:
In `@Resources/Localizable.xcstrings`:
- Around line 91452-91468: There are two duplicate localization keys for the
same copy: rightSidebar.extensionDemo.registrationSettlingStatus and
rightSidebar.extensionDemo.waitingForBundledDemo; pick one canonical key (e.g.,
rightSidebar.extensionDemo.waitingForBundledDemo), remove the other key's block,
copy/merge its "en" and "ja" stringUnit values into the canonical key (ensuring
wording is consistent), and update all code/IB strings to reference the chosen
key; repeat the same collapse for the other duplicate range noted (91554-91570)
so only a single localization entry exists to avoid translation drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57d55974-80d4-44f5-9af9-2e5c33a2c493
📒 Files selected for processing (4)
Resources/Localizable.xcstringsRightSidebarDemoExtension/RightSidebarDemoExtension.swiftSamples/RightSidebarBYOExtension/build-and-register.shSources/RightSidebarExtensionDemo.swift
✅ Files skipped from review due to trivial changes (1)
- Samples/RightSidebarBYOExtension/build-and-register.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/RightSidebarExtensionDemo.swift
There was a problem hiding this comment.
1 issue found across 10 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="Samples/RightSidebarBYOExtension/build-and-register.sh">
<violation number="1" location="Samples/RightSidebarBYOExtension/build-and-register.sh:13">
P2: The unsupported-architecture branch exits before applying a user-provided `TARGET_TRIPLE`, so the script cannot be overridden as the error message suggests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
RightSidebarDemoExtension/RightSidebarDemoExtension.swift (1)
252-270: Minor: hide the placeholderTextfrom VoiceOver to avoid double announcement.The
TextEditoralready sets.accessibilityLabel(...)to thesampleExtension.notesPlaceholderstring (Line 257-260). Whilenotesis empty, the overlayed placeholderTexton Line 263 is also exposed to accessibility (it only has.allowsHitTesting(false)), so VoiceOver will read “Write sidebar notes…” twice for the same control. Mark the decorative placeholder hidden from accessibility.♿ Proposed fix
if notes.isEmpty { Text(String(localized: "sampleExtension.notesPlaceholder", defaultValue: "Write sidebar notes...")) .font(.system(size: 11)) .foregroundStyle(.tertiary) .padding(.horizontal, 5) .padding(.vertical, 7) .allowsHitTesting(false) + .accessibilityHidden(true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift` around lines 252 - 270, The overlay placeholder Text in the ZStack is still exposed to accessibility causing duplicate VoiceOver announcements; update the placeholder Text (the one shown when notes.isEmpty) to be hidden from accessibility by adding the appropriate SwiftUI accessibility modifier (e.g., .accessibilityHidden(true) / .accessibility(hidden: true)) so only the TextEditor (which already has .accessibilityLabel(...)) is exposed; locate the placeholder Text in the ZStack near the TextEditor/notes binding and add the modifier to it.Sources/RightSidebarExtensionDemo.swift (2)
131-148: Preservation branch is hard to follow and re-evaluatesbundledDemoExtensionExists()per identity.The filter at L133-136 evaluates the file-system check inside the predicate for every element, and
bundledIdentifier.map { $0 == identity.bundleIdentifier } == trueis a roundabout spelling ofbundledIdentifier == identity.bundleIdentifier. Hoist the existence check and simplify the comparison — cheaper and easier to read.♻️ Minor cleanup
- if sortedIdentities.isEmpty, !identities.isEmpty { - let bundledIdentifier = RightSidebarExtensionDemoStore.bundledDemoExtensionBundleIdentifier() - let bundledIdentities = identities.filter { identity in - bundledIdentifier.map { $0 == identity.bundleIdentifier } == true && - RightSidebarExtensionDemoStore.bundledDemoExtensionExists() - } + if sortedIdentities.isEmpty, + !identities.isEmpty, + Self.bundledDemoExtensionExists(), + let bundledIdentifier = Self.bundledDemoExtensionBundleIdentifier() { + let bundledIdentities = identities.filter { $0.bundleIdentifier == bundledIdentifier } if !bundledIdentities.isEmpty {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarExtensionDemo.swift` around lines 131 - 148, The preservation branch repeatedly calls RightSidebarExtensionDemoStore.bundledDemoExtensionExists() inside the identities.filter predicate and uses a convoluted bundledIdentifier.map comparison; fix it by hoisting let bundledIdentifier = RightSidebarExtensionDemoStore.bundledDemoExtensionBundleIdentifier() and let bundledExists = RightSidebarExtensionDemoStore.bundledDemoExtensionExists() before the filter, then filter with a simple optional-equality check (bundledIdentifier == identity.bundleIdentifier) && bundledExists, leaving the rest of the logic that sets identities, selectedIdentityID and statusMessage unchanged.
79-79: Apply the_legacyLoadMatchingIdentities()wrapper pattern to confine the deprecation warning to a single site.
loadMatchingIdentities()carries@available(macOS, introduced: 13.0, deprecated: 26.0, message: "Use AppExtensionPoint.Monitor")(L217). The call at L79 is inside the macOS 26 path, so the compiler will emit a deprecation warning. Follow the centralized helper pattern used elsewhere in the repo (AppDelegate.swift, UpdateTestSupport.swift): create a private_legacyLoadMatchingIdentities()helper annotated with the deprecation, and call it from L79 and other sites within the#available(macOS 26.0, *) block. This confines the warning to the helper rather than propagating it to every call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarExtensionDemo.swift` at line 79, The call site uses loadMatchingIdentities() inside the macOS 26 branch which triggers a deprecation warning; define a private helper named _legacyLoadMatchingIdentities() annotated with the same `@available`(macOS, introduced: 13.0, deprecated: 26.0, message: "Use AppExtensionPoint.Monitor") and move the call to loadMatchingIdentities() into that helper (returning the same result type and rethrowing/awaiting as needed), then replace the call at the original location with (try? await _legacyLoadMatchingIdentities()) ?? [] so the deprecation is confined to the single helper instead of every call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@RightSidebarDemoExtension/RightSidebarDemoExtension.swift`:
- Around line 252-270: The overlay placeholder Text in the ZStack is still
exposed to accessibility causing duplicate VoiceOver announcements; update the
placeholder Text (the one shown when notes.isEmpty) to be hidden from
accessibility by adding the appropriate SwiftUI accessibility modifier (e.g.,
.accessibilityHidden(true) / .accessibility(hidden: true)) so only the
TextEditor (which already has .accessibilityLabel(...)) is exposed; locate the
placeholder Text in the ZStack near the TextEditor/notes binding and add the
modifier to it.
In `@Sources/RightSidebarExtensionDemo.swift`:
- Around line 131-148: The preservation branch repeatedly calls
RightSidebarExtensionDemoStore.bundledDemoExtensionExists() inside the
identities.filter predicate and uses a convoluted bundledIdentifier.map
comparison; fix it by hoisting let bundledIdentifier =
RightSidebarExtensionDemoStore.bundledDemoExtensionBundleIdentifier() and let
bundledExists = RightSidebarExtensionDemoStore.bundledDemoExtensionExists()
before the filter, then filter with a simple optional-equality check
(bundledIdentifier == identity.bundleIdentifier) && bundledExists, leaving the
rest of the logic that sets identities, selectedIdentityID and statusMessage
unchanged.
- Line 79: The call site uses loadMatchingIdentities() inside the macOS 26
branch which triggers a deprecation warning; define a private helper named
_legacyLoadMatchingIdentities() annotated with the same `@available`(macOS,
introduced: 13.0, deprecated: 26.0, message: "Use AppExtensionPoint.Monitor")
and move the call to loadMatchingIdentities() into that helper (returning the
same result type and rethrowing/awaiting as needed), then replace the call at
the original location with (try? await _legacyLoadMatchingIdentities()) ?? [] so
the deprecation is confined to the single helper instead of every call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae54a4c6-7a7b-486e-a565-d87c110a82ee
📒 Files selected for processing (3)
Resources/Localizable.xcstringsRightSidebarDemoExtension/RightSidebarDemoExtension.swiftSources/RightSidebarExtensionDemo.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c5b30a0a0
ℹ️ 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".
| if sortedIdentities.isEmpty, !identities.isEmpty { | ||
| let bundledIdentifier = RightSidebarExtensionDemoStore.bundledDemoExtensionBundleIdentifier() | ||
| let bundledIdentities = identities.filter { identity in | ||
| bundledIdentifier.map { $0 == identity.bundleIdentifier } == true && | ||
| RightSidebarExtensionDemoStore.bundledDemoExtensionExists() | ||
| } | ||
| if !bundledIdentities.isEmpty { |
There was a problem hiding this comment.
The bundled-stale fallback returns before disabledCount/unapprovedCount is evaluated, so a monitor snapshot with identities == [] but nonzero unavailable counts will still keep the old bundled identity selectable and show a settling message instead of the unavailable state. That leaves the picker in a misleading state (appears usable, but activation fails because macOS has the extension disabled/unapproved). Fresh evidence versus the earlier stale-clear fix: this specific early return still bypasses the later unavailable-status branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ce1b33e43
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eacad2c09
ℹ️ 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".
| if monitorState.identities.isEmpty { | ||
| let matchingIdentities = (try? await loadMatchingIdentities()) ?? [] | ||
| guard generation == reloadGeneration else { return } | ||
| if matchingIdentities.isEmpty { | ||
| applyMonitorState(monitorState) |
There was a problem hiding this comment.
loadIdentities only calls loadMatchingIdentities() when monitor.state.identities is empty, so any identities registered under the production/legacy extension-point IDs are dropped whenever the current point has at least one match (for example, the bundled debug extension). In that mixed-install scenario, the picker can never show those other extensions even though matchingDiscoveryIdentifiers explicitly includes them; merge monitor identities with cross-point discovery results unconditionally (then dedupe) instead of treating cross-point discovery as empty-state fallback only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 5 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/RightSidebarExtensionDemo.swift">
<violation number="1" location="Sources/RightSidebarExtensionDemo.swift:73">
P2: The monitor now tracks only one extension point, which can hide extensions registered under the other supported identifiers once any identity is found. Add all supported point identifiers to the monitor to keep discovery complete.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ebar-demo # Conflicts: # Sources/AppDelegate.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
3 issues found across 6 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="scripts/reload.sh">
<violation number="1" location="scripts/reload.sh:546">
P2: This `grep -q` pipeline runs under `pipefail`, so a successful match can still produce a non-zero pipeline status (SIGPIPE on `pluginkit`) and incorrectly trigger re-election.</violation>
</file>
<file name="Sources/RightSidebarExtensionDemo.swift">
<violation number="1" location="Sources/RightSidebarExtensionDemo.swift:130">
P1: `registryGeneration` is incorrectly incremented when falling back to the bundled extension, causing the ExtensionKit view to unnecessarily restart and flash.</violation>
<violation number="2" location="Sources/RightSidebarExtensionDemo.swift:557">
P2: The `reconnectAttempts` counter is not reset when a new extension is explicitly selected, preventing the new extension from retrying if it fails on its first attempt.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| $0.localizedName.localizedCaseInsensitiveCompare($1.localizedName) == .orderedAscending | ||
| } | ||
| let currentIdentityIDs = sortedIdentities.map(\.id) | ||
| if currentIdentityIDs != previousIdentityIDs { |
There was a problem hiding this comment.
P1: registryGeneration is incorrectly incremented when falling back to the bundled extension, causing the ExtensionKit view to unnecessarily restart and flash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/RightSidebarExtensionDemo.swift, line 130:
<comment>`registryGeneration` is incorrectly incremented when falling back to the bundled extension, causing the ExtensionKit view to unnecessarily restart and flash.</comment>
<file context>
@@ -125,11 +122,17 @@ final class RightSidebarExtensionDemoStore: ObservableObject {
$0.localizedName.localizedCaseInsensitiveCompare($1.localizedName) == .orderedAscending
}
+ let currentIdentityIDs = sortedIdentities.map(\.id)
+ if currentIdentityIDs != previousIdentityIDs {
+ registryGeneration += 1
+ }
</file context>
| return | ||
| fi | ||
|
|
||
| if ! pluginkit -m -A -D -v -p "$extension_point_id" 2>/dev/null | grep -q '^[[:space:]]*+'; then |
There was a problem hiding this comment.
P2: This grep -q pipeline runs under pipefail, so a successful match can still produce a non-zero pipeline status (SIGPIPE on pluginkit) and incorrectly trigger re-election.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/reload.sh, line 546:
<comment>This `grep -q` pipeline runs under `pipefail`, so a successful match can still produce a non-zero pipeline status (SIGPIPE on `pluginkit`) and incorrectly trigger re-election.</comment>
<file context>
@@ -530,6 +530,24 @@ register_extensionkit_extensions() {
+ return
+ fi
+
+ if ! pluginkit -m -A -D -v -p "$extension_point_id" 2>/dev/null | grep -q '^[[:space:]]*+'; then
+ pluginkit -e use -i "$extension_bundle_id" -p "$extension_point_id" >/dev/null 2>&1 || true
+ fi
</file context>
| if ! pluginkit -m -A -D -v -p "$extension_point_id" 2>/dev/null | grep -q '^[[:space:]]*+'; then | |
| if ! (set +o pipefail; pluginkit -m -A -D -v -p "$extension_point_id" 2>/dev/null | grep -q '^[[:space:]]*+'); then |
| } | ||
|
|
||
| func hostViewControllerDidActivate(_ viewController: EXHostViewController) { | ||
| reconnectAttempts = 0 |
There was a problem hiding this comment.
P2: The reconnectAttempts counter is not reset when a new extension is explicitly selected, preventing the new extension from retrying if it fails on its first attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/RightSidebarExtensionDemo.swift, line 557:
<comment>The `reconnectAttempts` counter is not reset when a new extension is explicitly selected, preventing the new extension from retrying if it fails on its first attempt.</comment>
<file context>
@@ -543,6 +554,7 @@ struct ExtensionKitSidebarHostView: NSViewControllerRepresentable {
}
func hostViewControllerDidActivate(_ viewController: EXHostViewController) {
+ reconnectAttempts = 0
statusMessage.wrappedValue = String(
localized: "rightSidebar.extensionDemo.xpcConnected",
</file context>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Demo Video
No video attached. The tagged dogfood build is available for local testing.