Open
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a background indexing feature to proactively parse ObjC and Swift metadata for loaded images and their dependencies, utilizing a new actor-based manager and a dedicated coordinator. The implementation includes AppKit UI components for real-time progress visibility and configuration. Feedback identifies several important improvements, including the need for batch deduplication in the manager, resolving path normalization inconsistencies for simulator environments, and ensuring the coordinator correctly handles engine reassignments during source switches. Additionally, recommendations were made to optimize the popover's outline view performance, ensure consistent semaphore signaling, and enhance documentation for specific internal behaviors.
Comment on lines
+57
to
+72
| let id = RuntimeIndexingBatchID() | ||
| let items = await expandDependencyGraph(rootPath: rootImagePath, depth: depth) | ||
| let batch = RuntimeIndexingBatch( | ||
| id: id, rootImagePath: rootImagePath, depth: depth, | ||
| reason: reason, items: items, | ||
| isCancelled: false, isFinished: false) | ||
| let state = BatchState(batch: batch, maxConcurrency: max(1, maxConcurrency)) | ||
| activeBatches[id] = state | ||
| continuation.yield(.batchStarted(batch)) | ||
|
|
||
| let drivingTask = Task { [weak self] in | ||
| guard let self else { return } | ||
| await self.runBatch(id: id) | ||
| } | ||
| activeBatches[id]?.drivingTask = drivingTask | ||
| return id |
There was a problem hiding this comment.
The implementation of startBatch currently lacks the deduplication logic mentioned in the design documentation (Evolution 0002) and the coordinator's comments. Multiple calls for the same root image and reason will spawn redundant batches, leading to wasted CPU cycles. Implementing a check for an existing active batch with the same parameters is recommended.
if let existing = activeBatches.values.first(where: {
$0.batch.rootImagePath == rootImagePath && $0.batch.reason == reason && !$0.batch.isFinished
}) {
return existing.batch.id
}
let id = RuntimeIndexingBatchID()
let items = await expandDependencyGraph(rootPath: rootImagePath, depth: depth)
let batch = RuntimeIndexingBatch(
id: id, rootImagePath: rootImagePath, depth: depth,
reason: reason, items: items,
isCancelled: false, isFinished: false)
let state = BatchState(batch: batch, maxConcurrency: max(1, maxConcurrency))
activeBatches[id] = state
continuation.yield(.batchStarted(batch))
let drivingTask = Task { [weak self] in
guard let self else { return }
await self.runBatch(id: id)
}
activeBatches[id]?.drivingTask = drivingTask
return id
Comment on lines
+30
to
+39
| try await request { | ||
| // Mirror loadImage(at:) byte-for-byte sans reloadData(isReloadImageNodes:). | ||
| try DyldUtilities.loadImage(at: path) | ||
| _ = try await objcSectionFactory.section(for: path) | ||
| _ = try await swiftSectionFactory.section(for: path) | ||
| loadedImagePaths.insert(path) | ||
| } remote: { senderConnection in | ||
| try await senderConnection.sendMessage( | ||
| name: .loadImageForBackgroundIndexing, request: path) | ||
| } |
There was a problem hiding this comment.
There is a path normalization asymmetry between isImageIndexed (which patches the path) and loadImageForBackgroundIndexing (which does not). When DYLD_ROOT_PATH is set (e.g., in an iOS Simulator environment), this will cause isImageIndexed to fail to find the cached sections populated by loadImageForBackgroundIndexing, leading to redundant indexing cycles. Both methods should use the patched path for consistency.
try await request {
let normalized = DyldUtilities.patchImagePathForDyld(path)
// Mirror loadImage(at:) byte-for-byte sans reloadData(isReloadImageNodes:).
try DyldUtilities.loadImage(at: normalized)
_ = try await objcSectionFactory.section(for: normalized)
_ = try await swiftSectionFactory.section(for: normalized)
loadedImagePaths.insert(normalized)
} remote: { senderConnection in
try await senderConnection.sendMessage(
name: .loadImageForBackgroundIndexing, request: path)
}
Comment on lines
+37
to
+38
| public private(set) lazy var backgroundIndexingCoordinator = | ||
| RuntimeBackgroundIndexingCoordinator(documentState: self) |
There was a problem hiding this comment.
The backgroundIndexingCoordinator captures the runtimeEngine at initialization. Since MainCoordinator reassigns runtimeEngine during a source switch, this coordinator will remain bound to the stale engine. Consider adding a mechanism to recreate the coordinator when the engine changes to ensure the UI and background tasks remain synchronized with the active session.
| return false | ||
| } | ||
|
|
||
| func outlineView(_ outlineView: NSOutlineView, |
There was a problem hiding this comment.
The outlineView(_:viewFor:item:) implementation allocates a new NSTableCellView and its subviews for every row on every refresh. For batches with many dependencies, this can lead to significant performance degradation and memory churn. It is highly recommended to use outlineView.makeView(withIdentifier:owner:) to leverage AppKit's view recycling mechanism.
| break | ||
| } | ||
| if Task.isCancelled { wasCancelled = true; break } | ||
| group.addTask { [weak self] in |
There was a problem hiding this comment.
If Task.isCancelled becomes true immediately after semaphore.waitUnlessCancelled() succeeds, the loop breaks without calling semaphore.signal(). While the semaphore is local to this function and will be deallocated, it's better practice to ensure signals match waits to avoid potential issues if the semaphore is ever refactored to be shared.
if Task.isCancelled {
semaphore.signal()
wasCancelled = true
break
}|
|
||
| /// Like `loadImage(at:)` but does **not** call `reloadData()`. | ||
| /// Used by the background indexing manager to avoid UI refresh storms. | ||
| public func loadImageForBackgroundIndexing(at path: String) async throws { |
There was a problem hiding this comment.
loadImageForBackgroundIndexing(at:) intentionally avoids emitting imageDidLoadSubject to prevent recursive batch spawning. This is a critical behavioral difference from loadImage(at:) and should be explicitly documented in the method's doc comment.
| for id in ids { cancelBatch(id) } | ||
| } | ||
|
|
||
| public func prioritize(imagePath: String) { |
There was a problem hiding this comment.
The prioritize(imagePath:) method only affects items currently in the pending state that have not yet been dispatched. It cannot preempt running tasks or affect items already removed from the internal queue. This limitation should be documented to clarify the "best-effort" nature of the priority boost.
Comment on lines
+259
to
+261
| if item == nil { | ||
| let batches = renderedNodes.compactMap { node -> RuntimeIndexingBatch? in | ||
| if case .batch(let batch) = node { return batch } else { return nil } |
There was a problem hiding this comment.
Filtering renderedNodes using compactMap inside outlineView(_:child:ofItem:) results in O(N^2) complexity during outline view reloads as this method is called for every child of every item. Caching the list of batch nodes whenever renderedNodes is updated would improve performance.
Comment on lines
+67
to
+70
| case .disabled: | ||
| iconView.contentTintColor = .tertiaryLabelColor | ||
| spinner.stopAnimation(nil) | ||
| failureDot.isHidden = true |
There was a problem hiding this comment.
The .disabled state in BackgroundIndexingToolbarState appears to be unused as it is never emitted by the coordinator or main window controller. Consider either removing this case or wiring it to the background indexing enabled setting.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “background indexing” pipeline that pre-indexes ObjC/Swift metadata for the dependency closure of loaded images, with Settings configuration and an AppKit toolbar popover for progress + cancellation. This spans the Core engine (new APIs + actor-based scheduler), an Application-layer coordinator bridging events to RxSwift, and UI plumbing (toolbar item, popover, settings page).
Changes:
- Introduces
RuntimeBackgroundIndexingManager(actor) + supporting value types and engine APIs (isImageIndexed,mainExecutablePath,loadImageForBackgroundIndexing,imageDidLoadPublisher). - Adds per-Document
RuntimeBackgroundIndexingCoordinator(@mainactor) and wires it into document lifecycle + sidebar prioritize + toolbar popover routing. - Adds Settings model + SwiftUI settings page and extensive unit tests for Core background indexing behavior.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/Main/MainWindowController.swift | Wires background-indexing toolbar click + binds toolbar state from coordinator aggregate observable. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/Main/MainViewModel.swift | Adds backgroundIndexingClick input and routes to MainRoute.backgroundIndexing. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/Main/MainToolbarController.swift | Registers new toolbar item identifier + inserts it into default/allowed sets. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/Main/MainRoute.swift | Adds new route case for presenting background indexing popover. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/Main/MainCoordinator.swift | Presents background indexing popover and injects per-document coordinator. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/BackgroundIndexing/BackgroundIndexingToolbarItemView.swift | Implements toolbar icon/spinner/failure-dot view and state rendering. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/BackgroundIndexing/BackgroundIndexingToolbarItem.swift | Implements toolbar item with relay-based tap output and view binding. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/BackgroundIndexing/BackgroundIndexingPopoverViewModel.swift | Bridges coordinator batches/aggregate state into observed properties and UI outputs. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/BackgroundIndexing/BackgroundIndexingPopoverViewController.swift | Implements popover UI, outline rendering, and action relays (cancel/clear/open settings). |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/BackgroundIndexing/BackgroundIndexingNode.swift | Defines outline model nodes for batch + item display. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit/App/Document.swift | Hooks background indexing coordinator into document open/close lifecycle. |
| RuntimeViewerUsingAppKit/RuntimeViewerUsingAppKit.xcodeproj/project.pbxproj | Adds new BackgroundIndexing UI files to the Xcode project build. |
| RuntimeViewerPackages/Sources/RuntimeViewerSettingsUI/SettingsRootView.swift | Adds “Background Indexing” settings page entry. |
| RuntimeViewerPackages/Sources/RuntimeViewerSettingsUI/Components/BackgroundIndexingSettingsView.swift | Adds SwiftUI settings form for enable/depth/concurrency. |
| RuntimeViewerPackages/Sources/RuntimeViewerSettings/Settings.swift | Persists backgroundIndexing settings with autosave. |
| RuntimeViewerPackages/Sources/RuntimeViewerSettings/Settings+Types.swift | Defines Settings.BackgroundIndexing codable type + defaults. |
| RuntimeViewerPackages/Sources/RuntimeViewerApplication/Sidebar/SidebarRootViewModel.swift | Adds best-effort prioritize hook on leaf selection to boost pending tasks. |
| RuntimeViewerPackages/Sources/RuntimeViewerApplication/DocumentState.swift | Documents runtimeEngine semantics and adds lazy per-document coordinator. |
| RuntimeViewerPackages/Sources/RuntimeViewerApplication/BackgroundIndexing/RuntimeBackgroundIndexingCoordinator.swift | MainActor coordinator: pumps manager events, observes settings, starts/cancels batches, exposes relays/aggregate. |
| RuntimeViewerCore/Tests/RuntimeViewerCoreTests/BackgroundIndexing/RuntimeIndexingValueTypesTests.swift | Unit tests for new value types and progress semantics. |
| RuntimeViewerCore/Tests/RuntimeViewerCoreTests/BackgroundIndexing/RuntimeEngineIndexStateTests.swift | Engine-level tests for indexed state, mainExecutablePath, publisher emission, and background indexing load path. |
| RuntimeViewerCore/Tests/RuntimeViewerCoreTests/BackgroundIndexing/RuntimeBackgroundIndexingManagerTests.swift | Manager tests covering BFS expansion, concurrency cap, cancel, prioritize, failure handling, event sequencing. |
| RuntimeViewerCore/Tests/RuntimeViewerCoreTests/BackgroundIndexing/MockBackgroundIndexingEngine.swift | Mock engine implementing the background-indexing engine seam for unit tests. |
| RuntimeViewerCore/Tests/RuntimeViewerCoreTests/BackgroundIndexing/DylibPathResolverTests.swift | Tests dylib install-name resolution (@rpath/@loader_path/@executable_path). |
| RuntimeViewerCore/Sources/RuntimeViewerCore/Utils/DylibPathResolver.swift | Implements install-name → filesystem path resolution logic. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/RuntimeEngine.swift | Adds new commands, publishers, manager creation, remote forwarding, and relaxes visibility for extension use. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/RuntimeEngine+BackgroundIndexing.swift | Adds isImageIndexed, mainExecutablePath, loadImageForBackgroundIndexing and engine protocol conformance. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/Core/RuntimeSwiftSection.swift | Adds hasCachedSection(for:) cache query for indexed-state checks. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/Core/RuntimeObjCSection.swift | Adds hasCachedSection(for:) cache query for indexed-state checks. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingTaskState.swift | Defines task state enum + terminal-state helper. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingTaskItem.swift | Defines task item model used in batches/events. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingEvent.swift | Defines event stream emitted by the manager. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingBatchReason.swift | Defines batch reasons (app launch, image loaded, etc.). |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingBatchID.swift | Defines batch ID wrapper around UUID. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeIndexingBatch.swift | Defines batch model and progress counters. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/RuntimeBackgroundIndexingManager.swift | Core actor scheduling batches, BFS expansion, concurrency cap, cancel/prioritize, and events stream. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/ResolvedDependency.swift | Adds resolved dependency value type used in expansion logic. |
| RuntimeViewerCore/Sources/RuntimeViewerCore/BackgroundIndexing/BackgroundIndexingEngineRepresenting.swift | Defines Sendable protocol seam used by the manager for testability/remote dispatch. |
| RuntimeViewerCore/Package.swift | Adds Semaphore dependency to RuntimeViewerCore target. |
| Documentations/Reviews/2026-04-26-background-indexing-review.md | Review record (plan/evolution verification). |
| Documentations/Reviews/2026-04-26-background-indexing-implementation-review.md | Implementation review record and known issues list. |
| Documentations/Reviews/2026-04-25-background-indexing-review.md | Prior review record (plan/evolution verification). |
| Documentations/Reviews/2026-04-24-background-indexing-review.md | Prior review record (plan/evolution verification). |
| Documentations/Plans/2026-04-24-background-indexing-design.md | Removes superseded design doc (moved to Evolution). |
| Documentations/Evolution/0002-background-indexing.md | Accepted evolution doc describing architecture, decisions, and semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+288
to
+294
| let cell = NSTableCellView() | ||
| let label = Label("") | ||
| cell.hierarchy { label } | ||
| label.snp.makeConstraints { make in | ||
| make.leading.trailing.centerY.equalToSuperview() | ||
| } | ||
|
|
There was a problem hiding this comment.
outlineView(_:viewFor:item:) allocates a new NSTableCellView + label + constraints on every call. Since nodes can update frequently while indexing is running, this can create a lot of allocations/Auto Layout work. Consider using outlineView.makeView(withIdentifier:owner:) with a reusable view identifier (and configure subviews once) to keep popover updates lightweight.
Comment on lines
+51
to
+66
| public func startBatch( | ||
| rootImagePath: String, | ||
| depth: Int, | ||
| maxConcurrency: Int, | ||
| reason: RuntimeIndexingBatchReason | ||
| ) async -> RuntimeIndexingBatchID { | ||
| let id = RuntimeIndexingBatchID() | ||
| let items = await expandDependencyGraph(rootPath: rootImagePath, depth: depth) | ||
| let batch = RuntimeIndexingBatch( | ||
| id: id, rootImagePath: rootImagePath, depth: depth, | ||
| reason: reason, items: items, | ||
| isCancelled: false, isFinished: false) | ||
| let state = BatchState(batch: batch, maxConcurrency: max(1, maxConcurrency)) | ||
| activeBatches[id] = state | ||
| continuation.yield(.batchStarted(batch)) | ||
|
|
There was a problem hiding this comment.
startBatch always creates a new batch ID and inserts a new entry into activeBatches. This means callers can accidentally start duplicate batches for the same rootImagePath (e.g. app-launch + immediate image-load event) even though other parts of the codebase/documentation assume a dedup behavior. Consider adding a small dedup check (scan activeBatches for an unfinished batch with the same rootImagePath and the same reason discriminant) and return the existing ID instead of creating a new batch, or update downstream comments/spec to match the actual behavior.
Comment on lines
+238
to
+241
| // Avoid double-starting if the path is the main executable being opened | ||
| // at app launch — documentDidOpen already dispatched that batch. Manager | ||
| // dedups batches that share rootImagePath + reason discriminant, so a | ||
| // second call here is a no-op rather than a wasted batch. |
There was a problem hiding this comment.
This comment says the manager deduplicates batches with the same rootImagePath + reason discriminant, but RuntimeBackgroundIndexingManager.startBatch currently always creates a new batch. Either implement the dedup logic in the manager, or update this comment to avoid promising behavior the code doesn't provide (otherwise app-launch + image-loaded can double-index the same root).
Suggested change
| // Avoid double-starting if the path is the main executable being opened | |
| // at app launch — documentDidOpen already dispatched that batch. Manager | |
| // dedups batches that share rootImagePath + reason discriminant, so a | |
| // second call here is a no-op rather than a wasted batch. | |
| // This may start a separate batch even if the same path was already | |
| // indexed from `documentDidOpen()`. Any deduplication of batches with | |
| // overlapping roots/reasons must be implemented in the manager; this | |
| // call site does not currently rely on `startBatch` being a no-op. |
Comment on lines
+6
to
+11
| public func isImageIndexed(path: String) async throws -> Bool { | ||
| try await request { | ||
| let normalized = DyldUtilities.patchImagePathForDyld(path) | ||
| let hasObjC = await objcSectionFactory.hasCachedSection(for: normalized) | ||
| let hasSwift = await swiftSectionFactory.hasCachedSection(for: normalized) | ||
| return hasObjC && hasSwift |
There was a problem hiding this comment.
isImageIndexed normalizes the input via DyldUtilities.patchImagePathForDyld, but loadImageForBackgroundIndexing (and loadImage(at:)) caches sections and updates loadedImagePaths using the raw path. If patchImagePathForDyld ever changes the path (e.g. when DYLD_ROOT_PATH is set), this can cause isImageIndexed to return false even after indexing, leading to repeated work. Normalize consistently on both read and write paths (pick either patched or raw as the canonical cache key).
Comment on lines
+27
to
+28
| /// Like `loadImage(at:)` but does **not** call `reloadData()`. | ||
| /// Used by the background indexing manager to avoid UI refresh storms. |
There was a problem hiding this comment.
The doc comment for loadImageForBackgroundIndexing(at:) only mentions that it skips reloadData(), but it also intentionally does not emit imageDidLoadPublisher. That behavior is important for correctness (otherwise background indexing would recursively spawn new batches). Please document this explicitly so callers/tests don’t assume it behaves like loadImage(at:) aside from reloadData().
Suggested change
| /// Like `loadImage(at:)` but does **not** call `reloadData()`. | |
| /// Used by the background indexing manager to avoid UI refresh storms. | |
| /// Like `loadImage(at:)` but intentionally does **not** call `reloadData()` | |
| /// or emit `imageDidLoadPublisher`. | |
| /// Used by the background indexing manager to avoid UI refresh storms and | |
| /// to prevent background indexing from recursively spawning new batches. |
Comment on lines
+66
to
+67
| let engine = RuntimeEngine(source: .local) | ||
| try await engine.loadImageForBackgroundIndexing(at: path) |
There was a problem hiding this comment.
This test is named ...doesNotTriggerReloadData, but it currently only asserts that the image becomes indexed. Since loadImageForBackgroundIndexing is specifically intended to avoid UI/event storms, consider adding assertions that (a) reloadDataPublisher does not emit and (b) imageDidLoadPublisher does not emit for this call (use inverted expectations with a short timeout).
Suggested change
| let engine = RuntimeEngine(source: .local) | |
| try await engine.loadImageForBackgroundIndexing(at: path) | |
| let engine = RuntimeEngine(source: .local) | |
| let reloadDataExpectation = expectation(description: "reloadData should not fire") | |
| reloadDataExpectation.isInverted = true | |
| let imageDidLoadExpectation = expectation(description: "imageDidLoad should not fire") | |
| imageDidLoadExpectation.isInverted = true | |
| let reloadDataCancellable = engine.reloadDataPublisher.sink { | |
| reloadDataExpectation.fulfill() | |
| } | |
| // imageDidLoadPublisher is `nonisolated` — no await needed. | |
| let imageDidLoadCancellable = engine.imageDidLoadPublisher.sink { _ in | |
| imageDidLoadExpectation.fulfill() | |
| } | |
| try await engine.loadImageForBackgroundIndexing(at: path) | |
| await fulfillment(of: [reloadDataExpectation, imageDidLoadExpectation], timeout: 0.25) | |
| reloadDataCancellable.cancel() | |
| imageDidLoadCancellable.cancel() |
Comment on lines
+10
to
+20
| /// The runtime engine backing this Document. | ||
| /// | ||
| /// Per Evolution 0002 (Background Indexing) Assumption #1, this property | ||
| /// is treated as **immutable for the lifetime of the Document**. The | ||
| /// declaration uses `@Observed public var` for historical reasons (early | ||
| /// callers needed to swap in a remote engine after init), but current | ||
| /// callers MUST NOT reassign it after the Document is opened. | ||
| /// | ||
| /// `RuntimeBackgroundIndexingCoordinator` (and any future per-engine | ||
| /// actor) captures this reference at init time; reassignment would | ||
| /// silently route work to a stale engine. |
There was a problem hiding this comment.
This doc comment states runtimeEngine is treated as immutable after the Document opens, but MainCoordinator.prepareTransition(for: .main) currently reassigns documentState.runtimeEngine when switching sources. Since backgroundIndexingCoordinator captures runtimeEngine at init time, this mismatch can silently route indexing work/events to a stale engine after a source switch. Either enforce immutability (stop reassigning / make it non-mutable), or add a supported engine-swap path (e.g., recreate the background-indexing coordinator/manager when runtimeEngine changes).
Comment on lines
19
to
+26
| override func makeWindowControllers() { | ||
| addWindowController(mainCoordinator.windowController) | ||
| documentState.backgroundIndexingCoordinator.documentDidOpen() | ||
| } | ||
|
|
||
| override func close() { | ||
| documentState.backgroundIndexingCoordinator.documentWillClose() | ||
| super.close() |
There was a problem hiding this comment.
backgroundIndexingCoordinator is documented as lazy to avoid paying cost when the feature is disabled, but Document.makeWindowControllers() and Document.close() now unconditionally access it, which will instantiate the coordinator for every document regardless of settings. Either update the doc comment to reflect eager creation, or gate these calls (e.g., only touch the coordinator when background indexing is enabled / when the coordinator has already been created).
Mx-Iris
force-pushed
the
feature/runtime-background-indexing
branch
2 times, most recently
from
42f79cf to
7558f21
Compare
… plan/review Move the design draft into Evolution proposal 0002 (Accepted), update the implementation plan to reflect the review decisions, and sync the review document with the finalized scope.
…hinese Translate the Evolution 0002 proposal and the implementation plan into Chinese while preserving all code blocks, file paths, commands, and identifiers in their original form.
…itch ultrareview N2 / implementation-review I3: MainCoordinator reassigns documentState.runtimeEngine when the user switches source (Local ↔ XPC ↔ Bonjour), but the coordinator captured the initial engine in init and kept routing pumps / cancels / prioritize calls to the dead manager. Toolbar silently drifted out of sync with reality. Coordinator now subscribes to documentState.\$runtimeEngine.skip(1) (BehaviorRelay exposed by @observed) and runs handleEngineSwap(to:): cancel old pumps, fire-and-forget cancel of doc batches on the old manager, clear UI relays, swap the engine reference, restart pumps, re-trigger documentDidOpen() if the feature is enabled. DocumentState.runtimeEngine doc comment updated — the prior "treat as immutable" assumption is retired (Evolution 0002 assumption #1).
Sync spec, plan and review docs with the three post-review fixes shipped in this batch. Captures rationale future readers will need when the implementation surface drifts: - Evolution 0002: protocol/manager wording switches to AnyObject + unowned, scenario G (source switch) added, assumption #1 retracted in favour of "reassignable", assumption #4 documents the deliberate no-normalization choice for dyld shared cache lookups, three new decision-log entries dated 2026-04-28. - Plan: appended a "Post-review fixes (2026-04-28)" section listing each fix's changed files and why the surrounding plan tasks were not reopened. - Reviews: I3 / N1 / N2 / N4 marked FIXED inline with the original finding so the issue trail stays attached. The N4 entry also explains why the reviewer's suggested versioned/unversioned normalization was rejected in favour of literal matching.
Align Bonjour reliability proposal with the numeric evolution scheme adopted by 0002-background-indexing.md. Pure rename; content unchanged.
XCFramework script now builds against RuntimeViewer-Distribution.xcworkspace and explicitly resolves package dependencies before each archive. Without the explicit resolve, SPM happily reuses the default DerivedData SourcePackages/checkouts directory and pins stale transitive versions (e.g. swift-dyld-private 1.1.0 instead of the available 1.2.0). Adds --no-update-packages to opt out of the resolve step when reproducing a specific Package.resolved is desired.
Convert the four BackgroundIndexing test files (36 tests total) from XCTest to Swift Testing. Suites become @suite struct (or final class where mutable state is needed), XCTAssert* turns into #expect, and XCTSkipUnless turns into .enabled(if:) traits — preserving the "skipped" status when host conditions aren't met (e.g. system frameworks absent on Apple Silicon hosts). RuntimeBackgroundIndexingManagerTests stays a class because it carries mutable aliveObjects + keep(_:) helper that pin mock engines past async suspension points (the manager's unowned engine reference would otherwise dangle when ARC eagerly releases parallel test locals). Drops underscore separators from method names per project convention.
Repeated patching used to produce a doubled prefix like /sim_root/sim_root/usr/lib/libobjc.A.dylib because the function only checked starts(with: "/"). Add a guard that returns input unchanged when it already equals rootPath or has rootPath + "/" as a prefix. Splits the function into a default env-reading wrapper and a pure overload taking rootPath explicitly so tests can drive the logic without touching ProcessInfo. Covers the iOS Simulator scenario where dyld already reports patched paths to callers — re-patching is now a no-op rather than a corruption, removing a trap that would surface once cache writers also adopt patching to align with readers.
Mark Pre-1 as 🟡 partial fix with commit a033d3d. Document why Step 1 (idempotency guard) precedes Step 2 (writer-side patching): without the guard, Step 2 would double-patch in iOS Simulator where dyld already returns patched image names.
Pre-1 Step 2. `loadImage(at:)` and `loadImageForBackgroundIndexing(at:)` now patch the path on entry via `DyldUtilities.patchImagePathForDyld` before touching internal storage (loadedImagePaths, section factory caches) and notification subjects (imageDidLoadSubject, sendRemoteImageDidLoadIfNeeded). This brings them in line with `_objects(in:)` and `_localObjectsWithProgress`, which already patched at entry, and with all reader-side lookups (`isImageLoaded`, `isImageIndexed`), which all patch first. Reader/writer is now symmetric on the canonical form. The wire still carries the raw path (`request: path` on the remote branch) so receivers patch with their own DYLD_ROOT_PATH, not the sender's. Step 1 (commit a033d3d) made `patchImagePathForDyld` idempotent, so re-patching an already-patched path produced by dyld in iOS Simulator runners is now a safe no-op. Adds two contract tests pinning the writer-side normalization. They pass trivially on macOS (where patch is identity) but catch any regression that drops the writer-side patch.
Replace the hand-rolled NSOutlineViewDataSource/Delegate, @objc target- action plumbing, and imperative isHidden updates with RxAppKit reactive bindings: - outlineView.rx.nodes drives the outline; BackgroundIndexingNode now conforms to OutlineNodeType + Differentiable, and the .batch case carries its child items inline so the tree structure is self-contained - button.rx.click.asSignal() feeds the ViewModel's Input directly, removing the cancelAll/clearFailed/openSettings PublishRelays and @objc action methods - output.isEnabled / hasAnyFailure drive .rx.isHidden via Driver combinators instead of subscribeOnNext side effects - Cell rendering is extracted into private BatchCellView and ItemCellView NSTableCellView subclasses for clearer ownership
Three fixes from the ultrareview's N3 / Nit-2 / Nit-3: N3 — Manager batch dedup. `RuntimeBackgroundIndexingManager.startBatch` now returns the existing batch's id when another non-finished batch shares the same `rootImagePath`. Reason discriminant is intentionally ignored so the realistic `.appLaunch` ↔ `.imageLoaded(path:)` pair collapses. Two checks: an early one before `expandDependencyGraph` to short-circuit work, and a re-check after the suspension to handle actor reentrancy races. Coordinator's `handleImageLoaded` comment updated to match the broader rule. Nit-2 — `.settingsEnabled` reason is no longer dead. Off→on transition in `handleSettingsChange` previously routed through `documentDidOpen()` which hardcoded `.appLaunch`, so the popover always showed "App launch indexing". Extracted `startMainExecutableBatch(reason:)` helper; both entry points now pass their own reason. No behavior change beyond the title string. Nit-3 — `documentBatchIDs` no longer leaks failed-finalized batch ids. The `.batchFinished` failure-retain branch and `clearFailedBatches()` both updated to drop their cleared ids. Without this, the set grew monotonically over a Document's lifetime and `documentWillClose` fired no-op cancel Tasks for ghost ids that had already finalized on the manager. Adds two manager tests: dedup hits across reasons, and dedup releases after finalize so a fresh batch on the same root works.
Nit-1 from the ultrareview. The popover ViewModel's `cancelBatch` Input slot was already plumbed end-to-end (relay → coordinator → manager) but no UI element ever fired it — the ViewController passed `.never()` for the slot and the cell only rendered a label. `BatchCellView` now hosts an inline `xmark.circle` button next to the title. The button is hidden when the batch has already been finalized on the manager (failed-retain rows pending user dismiss) so users don't try to cancel something that's no longer running. Click routes through a closure on the cell into `cancelBatchRelay`, which feeds the existing Input.cancelBatch signal.
All four follow-up review items now have a dated 2026-04-28 fix block explaining the chosen approach and any new tests / contracts pinned. The summary at the top of the doc is updated accordingly: every item the ultrareview surfaced is now ✅ except Pre-existing Pre-1 (closed in earlier commits with Step 1 + Step 2).
Two related bugs the user surfaced as "toolbar item is always disabled and won't click": 1. Click was a no-op. `BackgroundIndexingToolbarItem` set `target/action` on the NSToolbarItem itself, but its `view` was a plain NSView wrapping NSImageView + NSProgressIndicator + a small dot — none of them are NSControls, and NSToolbarItem only routes clicks to its action via NSControl subviews (or via the overflow menu). The button never fired. 2. Looked greyed-out. Idle state tinted the icon with `.secondaryLabelColor`, which renders as a muted gray next to peer toolbar buttons that use the standard tint — easy to read as "disabled". `BackgroundIndexingToolbarItemView` now hosts a `ToolbarButton` (the same `bezelStyle = .toolbar` NSButton subclass that `IconButtonToolbarItem` uses for MCP/Save/etc.) as its bottom layer. The spinner and failure dot are layered on top as click-through overlays — small private NSProgressIndicator / NSView subclasses override `hitTest(_:)` to return nil so clicks fall through to the button beneath. Idle tint is `nil` (default toolbar color) so the icon looks like its peers. `BackgroundIndexingToolbarItem` wires `itemView.button.target/action` to its existing `clicked()` selector. The item-level target/action are kept so overflow-menu clicks still work. Removed the dead `bindState` API — nothing called it.
…tton User feedback: the indexing toolbar widget was unresponsive and looked disabled. Even after wiring the click handler in the previous commit (dbeea06), the icon-with-overlay design felt out of place next to the peer toolbar items. Strip the state visualization for now and revisit later — the popover itself already shows progress, failures, and batches in detail. `BackgroundIndexingToolbarItem` now subclasses `MainToolbarController.IconButtonToolbarItem` (the same base used by MCP / Save / Attach / etc.) with the `square.stack.3d.down.right` SF Symbol. No spinner, no failure dot, no tinting. Click flows via the inherited `button` and `MainWindowController` switches its Input wire from the now-removed `tapRelay` to the standard `.button.rx.clickWithSelf` pattern that mirrors `mcpStatusClick`. Drops `BackgroundIndexingToolbarItemView.swift` — its NSView wrapper, overlay layout, and the `BackgroundIndexingToolbarState` enum are no longer needed. Removes the `aggregateStateObservable → toolbar item view` binding in `MainWindowController`.
In Debug builds Xcode emits the product as a thin stub at Contents/MacOS/<Name> plus a sibling <Name>.debug.dylib that holds the real code. MachOImage(name:) strips both extensions and matches by basename, so it picked the stub (loaded first at dyld index 0) and the caller never saw the actual dependency graph or sections — background indexing expanded only 3 trivial items (root + libSystem + an unresolved @rpath/.debug.dylib) and finished in ~25 ms with nothing useful indexed. Centralize main-binary MachOImage retrieval in DyldUtilities and use MachOImage.current() (resolves via #dsohandle of the calling code) for the main executable's path. In Debug this returns the .debug.dylib that contains all 60+ real deps; in statically linked Release it returns the main exe, identical to before. Apply the helper in RuntimeEngine canOpen/rpaths/dependencies and in RuntimeObjCSection / RuntimeSwiftSection inits so cached sections also reflect the actual code-bearing image.
Move the source-describing #log call to after backgroundIndexingManager is assigned so the initialization-complete log line follows the last stored property assignment.
…d scenarios Three independent root causes for spurious "path unresolved" rows in the indexing popover: - `imageNames().first` returns the wrong path under `DYLD_INSERT_LIBRARIES` (Xcode injects `libLogRedirect.dylib` at dyld index 0 during debug runs). Add `DyldUtilities.mainExecutablePath()` via `_NSGetExecutablePath` and use it for both the host main exe path and the `MachOImage.current()` short-circuit. - BFS only passed each image's own LC_RPATH to `dependencies(for:)`, so a framework with no LC_RPATH but loaded via the host's rpath always failed. Walk the loader chain in the BFS and pass accumulated ancestor rpaths to the engine; merge them with the image's own rpaths in loader-first order, matching dyld's lookup. - LC_LOAD_WEAK_DYLIB entries that miss on disk are tolerated by dyld at runtime (e.g. Xcode-embedded `libswiftCompatibilitySpan.dylib`). Silently drop them from the BFS instead of surfacing red ✗ rows. Also adds resolution-failure logs in `DylibPathResolver` so the cause of any remaining miss is visible in the unified log.
…kgroundMode Rename `Settings.BackgroundIndexing` to `Settings.Indexing.BackgroundMode` to make room for non-background indexing modes (e.g. on-demand) without having to rename callers again later. The nested struct keeps the existing `isEnabled` / `depth` / `maxConcurrency` keys intact, so persisted defaults migrate transparently. Also clamps `maxConcurrency`'s upper bound to the host's `processorCount` (was a hardcoded 8) and renames the Settings sidebar entry from "Background Indexing" to "Indexing".
… Symbol icons The popover used to call `cell.configure(...)` once per `viewFor:item:`, so cell content went stale after the first render — RxAppKit's `elementUpdated` path goes through `NSOutlineView.reloadItem(_:)` which only marks the row for redisplay, it does not re-invoke `viewFor:item:`. Switch each cell to `bind(...)` against a per-cell driver (`viewModel.batch(for:)` / `viewModel.item(for:itemID:)`) so the row updates whenever the batch's progress or the item's state changes without scroll/click forcing a relayout. Visual rework that comes with the live binding: - Batch row gains a horizontal progress bar driven by completed/total. - Item row swaps the `↻ ✓ ✗` text glyphs for SF Symbols with the state-appropriate tint, and `arrow.triangle.2.circlepath` rotates while running. Uses raw `NSImageView` instead of the project's `ImageView` wrapper because the wrapper sets `wantsUpdateLayer=true` which flattens the image into `layer.contents` and disables symbol effects. - Header/footer separators and a slightly taller content size to give the new progress bar room to breathe. `RuntimeEngineManager` and its `Reactive` extension are marked `@MainActor` so the popover VM can read them synchronously without hopping actors.
Rename BackgroundIndexingEngineRepresenting and ResolvedDependency to RuntimeBackgroundIndexingEngineRepresenting / RuntimeResolvedDependency so the internal types match the module-wide Runtime* naming convention, making them unambiguous when surfaced through public APIs and search.
Replace Alternative E's failure-retention-in-batchesRelay with a parallel historyRelay holding all finalized batches (success / failure / cancelled). Active and historical concerns split into two outline sections, Clear Failed becomes Clear History, and AggregateState.hasAnyFailure goes unused now that the toolbar is a static IconButton. Plan doc captures the task-by-task migration ahead of implementation.
Finalized batches (success / failure / cancelled) now also flow into historyRelay alongside the existing active-batch tracking. No UI consumer yet — failure-retention in batchesRelay stays unchanged in this commit; the history relay is wired so the popover can render it in the next commit. handleEngineSwap clears history along with active batches since the old engine's metadata no longer applies.
BackgroundIndexingNode gains a .section(SectionKind, batches:) case so the popover outline can render top-level Active / History groups. Identifier for the section is kind-only so RxAppKit's staged-changeset preserves the user's expand-collapse state across updates. ViewModel still produces flat batch nodes for now — sectioning is wired in the next commit.
Popover now groups batches under top-level ACTIVE (always present, default-expanded) and HISTORY (rendered only when non-empty, default-collapsed). Failed batches no longer linger in batchesRelay; they land in history alongside successes and cancels. Clear Failed button replaced by Clear History which empties historyRelay. Empty state hides whenever active or history has content.
…icate batch nodes `apply(event:)` previously appended a finished/cancelled batch to historyRelay before settling batchesRelay, so combineLatest in the ViewModel briefly emitted nodes where the same batch appeared in both ACTIVE and HISTORY sections — producing a duplicate `differenceIdentifier` in the staged-changeset dataset. Defer history additions to after the batches accept, and derive hasAnyBatch / hasAnyHistory from the closure params instead of reaching back into the coordinator's sync accessors. The unused sync accessors are removed.
…ory relay too Per-cell drivers `batch(for:)` and `item(for:itemID:)` only searched `coordinator.batchesObservable`. Once a batch moved to the history relay the compactMap returned nil and BatchCellView/ItemCellView never received a value — the HISTORY row rendered its initial empty state (no title, visible progress bar, visible cancel button). Combine both relays and prefer active over history when an id appears in both during the short-lived hand-off between the two accept calls.
VStackView's AppKit default alignment is .centerX, so children sit at their horizontal intrinsicContentSize. NSProgressIndicator (.bar style) returns noIntrinsicMetric horizontally and HStackView's intrinsic width isn't enough to anchor the cell either, so View Debugger flagged an ambiguous width / horizontal position on every batch row. Pin both the top row and the progress bar to the stack's leading/trailing.
…w selection Cell views switch from NSTableCellView with manual init/coder to the project's TableCellView base class via setup(), and use rx.disposeBag instead of locally-held DisposeBags — keeps cell lifetimes consistent with the rest of the AppKit codebase. Also wire NSOutlineViewDelegate so selection is allowed only on .item rows; users can no longer place focus on a section header or batch row by clicking.
The proxy server only registered handlers for the pre-existing engine commands. Requests for the new `mainExecutablePath`, `isImageIndexed`, and `loadImageForBackgroundIndexing` commands therefore arrived at the proxy with no matching handler and were silently dropped. `RuntimeMessageChannel.sendRequest` holds `sendSemaphore` for the entire duration of the await, so a single dropped request blocked every later request on the same channel — including `isImageLoaded` issued from `SidebarRuntimeObjectViewModel.reloadData()`. The view model's `loadState` stayed at `.unknown` until the TCP connection eventually died and `finishReceiving` drained the pending continuations. Register the three missing handlers so the proxy delegates background indexing requests to the wrapped engine, mirroring the engine's own `setupMessageHandlerForServer` registration.
Mx-Iris
force-pushed
the
feature/runtime-background-indexing
branch
from
27dadad to
5c49d90
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RuntimeBackgroundIndexingManager) insideRuntimeEngine, with aRuntimeBackgroundIndexingCoordinator(@mainactor) in the Application layer bridging events to RxSwift for UI.Settings.backgroundIndexing.isEnabled = false); zero risk surface for users who don't enable it.Implementation breakdown
DylibPathResolver, 3 new engine methods (isImageIndexed/mainExecutablePath/loadImageForBackgroundIndexing) viarequest { local } remote: { … }dispatch,imageDidLoadPublisher,BackgroundIndexingEngineRepresentingprotocol + mock,RuntimeBackgroundIndexingManageractor (BFS expansion, TaskGroup + AsyncSemaphore concurrent execution, cancel, prioritize), manager hung offRuntimeEngine.Settings.BackgroundIndexingstruct + SwiftUI page (BackgroundIndexingSettingsView).documentDidOpen/documentWillClose), subscription toimageDidLoadPublisher,withObservationTrackingon Settings.BackgroundIndexingNode, popover ViewModel/ViewController, toolbar item + view,MainRoute.backgroundIndexingcase, MainCoordinator route handler, Document lifecycle wiring, sidebar-selection → prioritize.batchesRelayuntilclearFailedBatches()is called; oneengine.reloadData(isReloadImageNodes: false)per batch terminal event.Test plan
swift testinRuntimeViewerCore— 445/445 pass (background indexing unit tests cover value types,DylibPathResolver, manager BFS, concurrency cap, cancellation, prioritize event emission).swift buildinRuntimeViewerPackages— clean (0 errors, 0 new warnings).xcodebuildforRuntimeViewer macOSworkspace — clean (0 errors, 0 warnings, ~50s).Final review
See
Documentations/Reviews/2026-04-26-background-indexing-implementation-review.mdfor the complete final review.Verdict: SHIP, with conditions. No critical blockers. 6 Important issues to address before merge or as immediate follow-ups:
loadImageForBackgroundIndexingdeliberately doesn't emitimageDidLoadSubjectbut neither doc nor test notes this — add doc + test assertion.MainCoordinatorreassignsdocumentState.runtimeEngine, but the lazybackgroundIndexingCoordinatorcaptured the old engine; toolbar silently stops reflecting reality after a source switch. Recommend recreating the coordinator on source switch.prioritize(imagePath:)is best-effort (no preemption of running tasks, no effect on already-dispatched paths) — document the limitation.isImageIndexed(patches) andloadImageForBackgroundIndexing(does not). Dormant on macOS host (patchImagePathForDyldis a no-op whenDYLD_ROOT_PATHis unset); will activate under iOS Simulator support.NSTableCellViewper call — switch tomakeView(withIdentifier:owner:)recycling.10 Minor polish items (M1–M10) listed in the review document.
Design
See
Documentations/Evolution/0002-background-indexing.mdand the implementation plan atDocumentations/Plans/2026-04-24-background-indexing-plan.md.Commits
29 commits with conventional-commit style:
chore(core): dependency wiringfeat(core): 17 commits across value types, engine methods, manager actorfix(core): path normalization fixrefactor(core): unabbreviation + assertion tighteningfeat(settings)/feat(settings-ui): settings struct + pagefeat(application): 4 coordinator commitsfeat(ui): 4 AppKit UI commitsfeat(app): Document lifecycle + sidebar prioritizedocs(core)/docs(review): doc improvements + this review file