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
3 times, most recently
from
27dadad to
5c49d90
Compare
Mx-Iris
added a commit
that referenced
this pull request
May 5, 2026
Four independent improvements driven by the PR review: 1. Cache `mainExecutablePath` once at BFS entry and thread it through `dependencies(for:ancestorRpaths:mainExecutablePath:)` so a deep graph no longer pays one main-path lookup per node — the wins compound on remote (XPC / TCP) sources where each call is a network round-trip. 2. Bound `RuntimeBackgroundIndexingCoordinator.historyRelay` at 100 entries so a long-running session that triggers many `imageLoaded` batches no longer grows history without limit; users can still clear manually via `clearHistory()`. 3. Switch the BFS frontier from `Array.removeFirst()` (O(n)) to `Deque.popFirst()` (O(1)) via `swift-collections` — explicit dependency added so we no longer rely on a transitive product. 4. Make `RuntimeEngine.backgroundIndexingManager` a `lazy var` instead of an IUO; the actor's isolation guarantees thread-safe lazy init and removes the force-unwrap.
Member
Author
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Mx-Iris
added a commit
that referenced
this pull request
May 6, 2026
- loadImageForBackgroundIndexing: explicitly state it suppresses both reloadData() and imageDidLoadPublisher; emitting either would feed the coordinator's pump and recursively spawn batches during a BFS. - prioritize(imagePath:): document best-effort semantics — only .pending items get boosted, no preemption of running tasks. - startBatch second dedup check: note the deliberate trade-off of running full BFS in both racers rather than holding the actor across BFS, keeping cancel/prioritize responsive. - DocumentState.backgroundIndexingCoordinator: lazy is init-deferral, not gating-by-enablement — coordinator stays alive for off→on toggles.
… 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.
…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.
Tracks the published SwiftSyntax 603 binary release: refreshed all xcframework URLs / checksums, added the new SwiftWarningControl product, and seeded SwiftSyntax602 / SwiftSyntax603 aggregation targets so downstream macro plugins can opt into the newer toolchain.
Adds an `AppCoordinator` (with `AppRoute.settings`) and an `appRouter` dependency on `ViewModel`. Call sites that previously reached into `SettingsWindowController.shared` directly (AppDelegate menu action, MCPStatusPopoverViewModel disabled-tap, BackgroundIndexingPopoverViewModel "Open Settings" button) now trigger `.settings` through the router so the Settings UI module can stay `package`-scoped instead of leaking a public window controller. Bumps swift-memberwise-init-macro to gohanlon/0.6.0 to satisfy the upstream macro toolchain.
Four independent improvements driven by the PR review: 1. Cache `mainExecutablePath` once at BFS entry and thread it through `dependencies(for:ancestorRpaths:mainExecutablePath:)` so a deep graph no longer pays one main-path lookup per node — the wins compound on remote (XPC / TCP) sources where each call is a network round-trip. 2. Bound `RuntimeBackgroundIndexingCoordinator.historyRelay` at 100 entries so a long-running session that triggers many `imageLoaded` batches no longer grows history without limit; users can still clear manually via `clearHistory()`. 3. Switch the BFS frontier from `Array.removeFirst()` (O(n)) to `Deque.popFirst()` (O(1)) via `swift-collections` — explicit dependency added so we no longer rely on a transitive product. 4. Make `RuntimeEngine.backgroundIndexingManager` a `lazy var` instead of an IUO; the actor's isolation guarantees thread-safe lazy init and removes the force-unwrap.
`startMainExecutableBatch` and `handleImageLoaded` re-read `self.engine` across `await` suspensions. Because `handleEngineSwap` runs on the same @mainactor and reassigns `self.engine`, a swap that lands between awaits would submit the old engine's root path to the new engine's manager and leak the resulting batch id into `documentBatchIDs`. Capture `engine` in each Task's capture list so every await targets the engine that started the work, mirroring the existing `cancelBatch` / `documentWillClose` pattern. Add a `self.engine === engine` guard before mutating `documentBatchIDs` so a stray id from the now-old manager is dropped instead of polluting the new engine's tracking set.
The relay was declared and accepted but never exposed in `Output` and never subscribed; the open-settings action is already routed earlier via `input.openSettings.emit(to: appRouter.rx.trigger(.settings))`. The accompanying comment claimed the relay was forwarded to Output — which it wasn't — making the dead code actively misleading.
- loadImageForBackgroundIndexing: explicitly state it suppresses both reloadData() and imageDidLoadPublisher; emitting either would feed the coordinator's pump and recursively spawn batches during a BFS. - prioritize(imagePath:): document best-effort semantics — only .pending items get boosted, no preemption of running tasks. - startBatch second dedup check: note the deliberate trade-off of running full BFS in both racers rather than holding the actor across BFS, keeping cancel/prioritize responsive. - DocumentState.backgroundIndexingCoordinator: lazy is init-deferral, not gating-by-enablement — coordinator stays alive for off→on toggles.
handleEngineSwap iterated only documentBatchIDs, which misses any startBatch Task that suspended after the swap began but before its id was inserted. The self.engine === engine guard in startMainExecutableBatch / handleImageLoaded correctly drops the stray id, but the batch itself remained active on the orphaned old manager and ran to completion uninterrupted, occupying CPU and section-cache slots until the old engine deinit'd. Switch to oldEngine.backgroundIndexingManager.cancelAllBatches() so both already-tracked batches and any swap-window arrivals are cancelled.
The previous subscribeToIsEnabled both seeded `isEnabled` from settings and registered the Observation tracker, with the seeding line running on every recursive call from onChange. Idempotent (both paths converge on the same value) but the trailing comment claiming "initial subscribe" was misleading. Mirror RuntimeBackgroundIndexingCoordinator's bootstrap/register pair: bootstrapIsEnabledObservation seeds once, registerIsEnabledObservation only registers and is the recursion target.
The prior test was named ...DoesNotTriggerReloadData but only asserted the image became indexed, leaving the publisher-silence contract unverified. Rename to MarksIndexedAndDoesNotEmitPublishers and add NSLock-backed counters that sink reloadDataPublisher and imageDidLoadPublisher, asserting both stay at zero across the call. Regression coverage if someone wires loadImageForBackgroundIndexing into the canonical loadImage path (which would feed the coordinator's image-loaded pump and recursively spawn batches).
Switches Distribution workspace from MxIris-Library-Forks fork (0.5.3-fork.1) to gohanlon/swift-memberwise-init-macro 0.6.0 — upstream now has the changes the fork was carrying.
Member
Author
Code reviewFound 3 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
… finishedCount split) Two PR #44 review items in the same popover surface, easier to review together than apart: * Route the popover's Close button through the ViewModel via `Input.close → router.trigger(.dismiss)` instead of calling `dismiss(nil)` from inside `setupBindings(for:)`. The direct call was the only point in the new BackgroundIndexing UI where navigation bypassed MVVM-C. * `RuntimeIndexingBatch.completedCount` previously summed every terminal item (`.completed | .failed | .cancelled`) and the popover cell rendered it as "X/Y" next to a green checkmark, so a fully failed batch read as fully successful. Split that into `finishedCount` (drives `progress` + the bar — every stopped item counts) and `succeededCount` / `failedCount` / `cancelledCount`. The cell now shows "succeeded/total" with a tinted "N failed" / "N cancelled" tail when present. Aggregate-state progress (`RuntimeBackgroundIndexingCoordinator`) keeps using `finishedCount` since it should reach 100% once work has stopped. Tests: rename `batchProgressReportsCompletedFraction` → `batchProgressReportsFinishedFraction` and assert the new counters including a `.cancelled` item, so all four properties are exercised.
`Output.openSettings` and the matching ViewController binding were removed earlier; the relay declaration was the only remaining residue. Same cleanup that 8656055 applied to the BackgroundIndexing twin.
Mx-Iris
force-pushed
the
feature/runtime-background-indexing
branch
from
7e97922 to
a792144
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