Mobile files tab + CTO pipeline + multi-service fixes#103
Closed
arul28 wants to merge 1 commit into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactored the iOS Files module with a comprehensive new implementation spanning file tree navigation, full-text/filename search, code editing, diff viewing, and Git operations. This includes updated architecture documentation emphasizing iOS design systems and typed WebSocket communication, revised build configuration, and corresponding test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
apps/ios/ADE/Views/Files/FileViewerCodeEditorView.swift (1)
56-61: Redundant dual update for text changes.Both
parent.text = textView.text(line 58) andparent.onTextChange(textView.text)(line 59) ultimately callviewModel.updateDraftText()in the parent view. While the ViewModel guards against duplicate updates, this pattern is redundant. Consider removing theonTextChangecallback since the binding already propagates changes.♻️ Suggested simplification
The
onTextChangecallback appears redundant given the binding already updates the ViewModel. InFileViewerView.swift, the binding'ssetclosure andonTextChangeboth callviewModel.updateDraftText(). You could simplify by removing one path:func textViewDidChange(_ textView: UITextView) { guard textView === container?.textView else { return } parent.text = textView.text - parent.onTextChange(textView.text) container?.updateLineNumbers() }And remove the
onTextChangeparameter from the component if unused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerCodeEditorView.swift` around lines 56 - 61, Remove the redundant second update in textViewDidChange: stop calling parent.onTextChange(textView.text) since assigning parent.text already updates the binding which triggers viewModel.updateDraftText; update the FileViewerCodeEditorView.textViewDidChange to only set parent.text and call container?.updateLineNumbers(), and remove the onTextChange parameter/usage from the parent component (FileViewerView.swift) if it becomes unused, ensuring all references to onTextChange and its call sites are removed or refactored accordingly.apps/ios/ADE/Views/Files/FileViewerChromeViews.swift (1)
107-157: Consider extracting shared button logic to reduce duplication.The selected and unselected button branches (lines 117-134 and 135-153) are nearly identical, differing only in
buttonStyleandtint. This pattern is repeated inFilesDiffModeControlas well.♻️ Optional extraction
struct FilesModeControl: View { // ... properties ... var body: some View { HStack(spacing: 8) { ForEach(availableModes) { mode in modeButton(for: mode, isSelected: selection == mode) } } } `@ViewBuilder` private func modeButton(for mode: FilesEditorMode, isSelected: Bool) -> some View { let isLocked = mode == .edit && !canEdit Button { onSelectMode(mode) } label: { VStack(spacing: 2) { Text(mode.title) .font(.caption.weight(.semibold)) if isLocked { Text("Locked") .font(.caption2) } } .frame(maxWidth: .infinity) } .buttonStyle(isSelected ? .glassProminent : .glass) .tint(isSelected ? ADEColor.accent : ADEColor.textSecondary) .disabled(isLocked) .accessibilityLabel(mode.title + (isLocked ? ", locked" : "")) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerChromeViews.swift` around lines 107 - 157, The selected/unselected button branches in FilesModeControl are duplicated; extract the shared button rendering into a private helper (e.g., add a modeButton(for:isSelected:) `@ViewBuilder` method used inside body) that computes isLocked = (mode == .edit && !canEdit) and only varies .buttonStyle and .tint based on isSelected, while keeping the shared VStack, .disabled(isLocked) and .accessibilityLabel logic; apply the same refactor to FilesDiffModeControl so both controls call their shared modeButton helper to eliminate duplication.apps/ios/ADE/Views/Files/FileTreeView.swift (1)
558-579: Consider using stable identifiers for breadcrumb items.Using
\.offsetas the identity forForEachwithenumerated()can cause SwiftUI diffing issues if the breadcrumb list changes (e.g., animations may not work correctly when navigating between directories). Consider using thepathproperty as the identifier since it should be unique for each breadcrumb.♻️ Suggested improvement
- ForEach(Array(breadcrumbs.enumerated()), id: \.offset) { _, breadcrumb in + ForEach(breadcrumbs, id: \.path) { breadcrumb in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileTreeView.swift` around lines 558 - 579, The ForEach is using Array(breadcrumbs.enumerated()) with id: \.offset which uses unstable index identities and can break SwiftUI diffing; change the loop to iterate over breadcrumbs with a stable identifier (use breadcrumb.path) so SwiftUI can track items reliably—replace the ForEach(Array(breadcrumbs.enumerated()), id: \.offset) usage with a ForEach(breadcrumbs, id: \.path) (or equivalent) and update the closure to accept a single breadcrumb parameter used by the existing Image/Button/Text branches.apps/ios/ADE/Views/Files/FilesTabView.swift (2)
264-277: Silent failure onrefreshLaneSnapshotsmay hide connectivity issues.Using
try?on line 267 silently ignores errors fromrefreshLaneSnapshots(). WhilelistWorkspaces()errors are properly surfaced, a failed refresh could leave stale data without user awareness.Consider logging the error or showing a non-blocking notice when remote refresh fails but local data loads successfully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FilesTabView.swift` around lines 264 - 277, In reload(), avoid silently swallowing errors from refreshLaneSnapshots(); replace the try? await syncService.refreshLaneSnapshots() call with explicit error handling that captures failures from syncService.refreshLaneSnapshots() and surfaces them non-blockingly (for example by logging via your app logger and/or setting a transient UI notice variable such as a new refreshErrorMessage or banner state) while still allowing the subsequent listWorkspaces() call to proceed; update reload, and reference refreshLaneSnapshots, listWorkspaces, reload, searchViewModel, canUseLiveFileActions, and errorMessage when implementing the logging/notice so the refresh failure is visible without blocking local data display.
185-186: Potential duplicatereload()calls on initial load.Both
.task { await reload() }(line 185) and.task(id: syncService.localStateRevision)(line 186) will execute on view appearance. The first runs unconditionally, while the second runs whenlocalStateRevisionchanges. On initial load, both triggerreload().Consider removing line 185 since line 186 will handle the initial load (its
idis evaluated on appear, triggering the task).♻️ Remove redundant initial reload task
.refreshable { await refreshFromPullGesture() } .sensoryFeedback(.selection, trigger: selectedWorkspaceId) .sensoryFeedback(.success, trigger: refreshFeedbackToken) - .task { await reload() } .task(id: syncService.localStateRevision) { await reload() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FilesTabView.swift` around lines 185 - 186, The view currently schedules reload() twice on appear: an unconditional .task { await reload() } and a .task(id: syncService.localStateRevision) { await reload() }, causing duplicate initial calls; remove the unconditional .task (the one calling reload() without an id) so that only the .task tied to syncService.localStateRevision runs (it will execute on appear because its id is evaluated) — locate the two .task modifiers in FilesTabView.swift and delete the first .task { await reload() } entry.apps/ios/ADE/Views/Files/FileViewerViewModel.swift (4)
111-113: Image caching stores data without validating it's a valid image.If
imageData(for:)returns data that isn't actually a valid image (e.g., corrupted or wrong content), it gets cached anyway. Later loads will retrieve this invalid cache entry. Consider validating withUIImage(data:)before caching:♻️ Validate image before caching
if loaded.isBinary, isImagePreviewable(relativePath: relativePath), let data = imageData(for: relativePath) { - ADEImageCache.shared.store(data, for: cacheKey) + if UIImage(data: data) != nil { + ADEImageCache.shared.store(data, for: cacheKey) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerViewModel.swift` around lines 111 - 113, The code caches raw data returned by imageData(for:) without verifying it's a valid image; update the block that checks loaded.isBinary and isImagePreviewable(relativePath:) to attempt constructing a UIImage from imageData(for: relativePath) and only call ADEImageCache.shared.store(data, for: cacheKey) if UIImage(data: data) succeeds (non-nil); if the data is invalid, avoid storing it (and optionally remove any existing cache entry for cacheKey) so subsequent loads don't retrieve corrupted image data.
29-40: Consider validating decoded data is actually an image.
imageData(for:)decodes content based on encoding but doesn't verify the decoded bytes represent valid image data. Combined withisImagePreviewablechecking only file extension, a corrupted or misnamed file could pass both checks. TheUIImage(data:)call inFilesBinaryPreviewViewwill return nil in that case, which is handled, but the cache would store invalid data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerViewModel.swift` around lines 29 - 40, imageData(for:) currently decodes blob content but doesn't verify the bytes are a valid image, which can lead to caching invalid data; update imageData(for:) to decode the blob (as now), then attempt to initialize a UIImage from the decoded Data (using UIImage(data:)) and only return the Data if the UIImage is non-nil (otherwise return nil), so FilesBinaryPreviewView and the cache only receive valid image data; reference functions: imageData(for:), isImagePreviewable(_:), and the UIImage(data:) check used by FilesBinaryPreviewView.
346-356: Error message parsing is fragile and locale-dependent.
shouldClearLoadedFilerelies on substring matching inerror.localizedDescription, which:
- May not match localized error messages in non-English locales
- Could match unintended errors containing these strings
Consider checking error types/codes instead of message content when possible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerViewModel.swift` around lines 346 - 356, The current shouldClearLoadedFile(for:) implementation relies on localized error.localizedDescription substring matching which is fragile; replace it with robust NSError checks: cast error to NSError and return true when NSError.domain is NSCocoaErrorDomain with code NSFileNoSuchFileError (or other relevant NSFile... codes), or when NSError.domain is NSPOSIXErrorDomain with code ENOENT (errNo 2); keep the existing message-based contains() check only as a last-resort fallback. Update the shouldClearLoadedFile(for:) function to perform these domain/code checks (using NSError) before falling back to localized string matching.
161-175: N+1 query pattern may cause performance issues for metadata loading.The code iterates through up to 25 commits and makes a separate
listCommitFilescall for each until finding one containing the file. For files not in recent commits, this could result in 25 sequential network requests.Consider whether the server could provide a "last commit for file" endpoint, or accept this as a best-effort feature with the existing approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerViewModel.swift` around lines 161 - 175, The loop causes an N+1 pattern: FileViewerViewModel uses syncService.listRecentCommits and then calls syncService.listCommitFiles sequentially up to 25 times; change this to avoid sequential per-commit network calls by either (A) adding a server-side endpoint on syncService such as findLastCommitForFile(laneId:filePath:) and call that to set lastCommitTitle and lastCommitDateText via relativeDateDescription(from:), or (B) if server change isn’t possible, modify the client to perform the lookups in parallel using TaskGroup (concurrently call listCommitFiles for the commits returned by listRecentCommits and stop when the first contains the file) to eliminate sequential waiting; update the codepaths that reference syncService.listCommitFiles and ensure the error handling remains best-effort.apps/ios/ADE/Views/Files/FileViewerRenderingViews.swift (1)
121-179: Consider clamping offset when scale returns to 1.The
magnificationGesturecorrectly resets offset when scale reaches 1, but if the user zooms out while panned, andscalesnaps to 1 inonEnded, the offset animation happens. However, if the user is mid-drag when the zoom ends,lastOffsetmay not be reset before the next drag starts. This is minor and self-corrects on double-tap.The implementation is solid overall—good use of simultaneous gestures and spring animations for smooth UX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Files/FileViewerRenderingViews.swift` around lines 121 - 179, When scale snaps back to 1 in magnificationGesture (in the magnificationGesture's onEnded), ensure offset and lastOffset are clamped/reset so subsequent dragGesture starts from zero: inside magnificationGesture.onEnded where you already set scale and lastScale, set offset = .zero and lastOffset = .zero (use withAnimation the same spring block) when scale == 1; additionally ensure dragGesture.onChanged continues to guard scale > 1 before using lastOffset so no stale lastOffset is applied mid-drag. This ensures the variables used by DragGesture (offset and lastOffset) are cleared whenever scale returns to 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.factory/services.yaml:
- Around line 2-5: Replace the hard-coded simulator name in the xcodebuild
destination arguments used by the build, test, build_full, and test_full
targets: stop using "name=iPhone 17 Pro Max" and instead reference a simulator
by ID (e.g., "id=<SIMULATOR_UDID>") or a lookup that resolves an available
simulator UDID at runtime; update each destination string to use "platform=iOS
Simulator,id=..." (or a CI-friendly UDID variable) so xcodebuild will run
reliably on CI images that may not include that named device.
- Around line 2-3: Update the two service commands ("build: xcodebuild build
-project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS
Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO
2>&1 | tail -5" and "test: xcodebuild test -project apps/ios/ADE.xcodeproj
-scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max'
-configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40") so the xcodebuild
exit status is preserved when piping: enable bash pipefail or capture
xcodebuild's exit code (e.g. use set -o pipefail or inspect PIPESTATUS) and
return that code after tail so CI fails on xcodebuild errors; also replace the
hard-coded destination device name with a configurable destination (use an
environment variable like IOS_DESTINATION or a simulator UDID resolved
dynamically via xcrun simctl) to avoid relying on "iPhone 17 Pro Max" being
present in CI.
In @.gitignore:
- Line 21: The recursive ignore rule "build/" currently matches nested build
directories (e.g., apps/desktop/build/) and hides desktop packaging assets
referenced by apps/desktop/package.json; update the .gitignore by scoping the
pattern to the repo root (change "build/" to "/build/") or alternatively add
explicit negation rules to unignore the desktop assets (e.g., add negation
entries for the specific icon/entitlements paths under apps/desktop) so the
desktop packaging files are not masked.
In `@apps/ios/ADE/Views/Files/FileTreeViewModel.swift`:
- Around line 260-271: validatePromptValue currently validates against
root-level nodes (`nodes`) causing false conflicts when creating files in
subdirectories; change it to validate against the correct sibling list by
passing the directory's child nodes (e.g., `childNodesByPath[prompt.basePath] ??
[]`) to filesNameValidationError instead of `nodes`. Keep the existing exclusion
(`prompt.node?.path`) and assignment to `actionErrorMessage`/return behavior the
same, but replace the `existingNodes: nodes` argument with the sibling nodes
lookup so conflicts are checked within the target directory.
In `@apps/ios/ADE/Views/Files/FileViewerCodeEditorView.swift`:
- Around line 35-43: The updateUIView is overwriting the UITextView.text during
delegate-driven changes, resetting the cursor; add a Bool flag (e.g.,
isUpdatingFromDelegate) to Coordinator (class Coordinator: NSObject,
UITextViewDelegate) and set it true before assigning parent.text in
textViewDidChange and false after, and call container?.updateLineNumbers() from
the delegate; then in updateUIView (the method with uiView.textView.text !=
text) guard that replacement with !context.coordinator.isUpdatingFromDelegate so
you skip reassigning uiView.textView.text when the change originated from the
delegate, preserving selection.
In `@apps/ios/ADE/Views/Files/FileViewerView.swift`:
- Around line 174-179: The view triggers viewModel.load twice on first appear
because both .task and the .task(id: syncService.localStateRevision) run; modify
the revision-based task (the .task with id: syncService.localStateRevision) to
skip its first invocation or combine both behaviors into one task: add a guard
(e.g., an internal hasAppeared or previousRevision check) so that
viewModel.load(syncService:syncService, workspace:workspace,
relativePath:relativePath, isFilesLive:isFilesLive, refreshDiff:viewModel.mode
== .diff) only runs on subsequent revision changes (or only when refreshDiff
needs to be applied), leaving the unconditional initial .task to perform the
first load and ensuring refreshDiff is still applied on later revisions.
---
Nitpick comments:
In `@apps/ios/ADE/Views/Files/FilesTabView.swift`:
- Around line 264-277: In reload(), avoid silently swallowing errors from
refreshLaneSnapshots(); replace the try? await
syncService.refreshLaneSnapshots() call with explicit error handling that
captures failures from syncService.refreshLaneSnapshots() and surfaces them
non-blockingly (for example by logging via your app logger and/or setting a
transient UI notice variable such as a new refreshErrorMessage or banner state)
while still allowing the subsequent listWorkspaces() call to proceed; update
reload, and reference refreshLaneSnapshots, listWorkspaces, reload,
searchViewModel, canUseLiveFileActions, and errorMessage when implementing the
logging/notice so the refresh failure is visible without blocking local data
display.
- Around line 185-186: The view currently schedules reload() twice on appear: an
unconditional .task { await reload() } and a .task(id:
syncService.localStateRevision) { await reload() }, causing duplicate initial
calls; remove the unconditional .task (the one calling reload() without an id)
so that only the .task tied to syncService.localStateRevision runs (it will
execute on appear because its id is evaluated) — locate the two .task modifiers
in FilesTabView.swift and delete the first .task { await reload() } entry.
In `@apps/ios/ADE/Views/Files/FileTreeView.swift`:
- Around line 558-579: The ForEach is using Array(breadcrumbs.enumerated()) with
id: \.offset which uses unstable index identities and can break SwiftUI diffing;
change the loop to iterate over breadcrumbs with a stable identifier (use
breadcrumb.path) so SwiftUI can track items reliably—replace the
ForEach(Array(breadcrumbs.enumerated()), id: \.offset) usage with a
ForEach(breadcrumbs, id: \.path) (or equivalent) and update the closure to
accept a single breadcrumb parameter used by the existing Image/Button/Text
branches.
In `@apps/ios/ADE/Views/Files/FileViewerChromeViews.swift`:
- Around line 107-157: The selected/unselected button branches in
FilesModeControl are duplicated; extract the shared button rendering into a
private helper (e.g., add a modeButton(for:isSelected:) `@ViewBuilder` method used
inside body) that computes isLocked = (mode == .edit && !canEdit) and only
varies .buttonStyle and .tint based on isSelected, while keeping the shared
VStack, .disabled(isLocked) and .accessibilityLabel logic; apply the same
refactor to FilesDiffModeControl so both controls call their shared modeButton
helper to eliminate duplication.
In `@apps/ios/ADE/Views/Files/FileViewerCodeEditorView.swift`:
- Around line 56-61: Remove the redundant second update in textViewDidChange:
stop calling parent.onTextChange(textView.text) since assigning parent.text
already updates the binding which triggers viewModel.updateDraftText; update the
FileViewerCodeEditorView.textViewDidChange to only set parent.text and call
container?.updateLineNumbers(), and remove the onTextChange parameter/usage from
the parent component (FileViewerView.swift) if it becomes unused, ensuring all
references to onTextChange and its call sites are removed or refactored
accordingly.
In `@apps/ios/ADE/Views/Files/FileViewerRenderingViews.swift`:
- Around line 121-179: When scale snaps back to 1 in magnificationGesture (in
the magnificationGesture's onEnded), ensure offset and lastOffset are
clamped/reset so subsequent dragGesture starts from zero: inside
magnificationGesture.onEnded where you already set scale and lastScale, set
offset = .zero and lastOffset = .zero (use withAnimation the same spring block)
when scale == 1; additionally ensure dragGesture.onChanged continues to guard
scale > 1 before using lastOffset so no stale lastOffset is applied mid-drag.
This ensures the variables used by DragGesture (offset and lastOffset) are
cleared whenever scale returns to 1.
In `@apps/ios/ADE/Views/Files/FileViewerViewModel.swift`:
- Around line 111-113: The code caches raw data returned by imageData(for:)
without verifying it's a valid image; update the block that checks
loaded.isBinary and isImagePreviewable(relativePath:) to attempt constructing a
UIImage from imageData(for: relativePath) and only call
ADEImageCache.shared.store(data, for: cacheKey) if UIImage(data: data) succeeds
(non-nil); if the data is invalid, avoid storing it (and optionally remove any
existing cache entry for cacheKey) so subsequent loads don't retrieve corrupted
image data.
- Around line 29-40: imageData(for:) currently decodes blob content but doesn't
verify the bytes are a valid image, which can lead to caching invalid data;
update imageData(for:) to decode the blob (as now), then attempt to initialize a
UIImage from the decoded Data (using UIImage(data:)) and only return the Data if
the UIImage is non-nil (otherwise return nil), so FilesBinaryPreviewView and the
cache only receive valid image data; reference functions: imageData(for:),
isImagePreviewable(_:), and the UIImage(data:) check used by
FilesBinaryPreviewView.
- Around line 346-356: The current shouldClearLoadedFile(for:) implementation
relies on localized error.localizedDescription substring matching which is
fragile; replace it with robust NSError checks: cast error to NSError and return
true when NSError.domain is NSCocoaErrorDomain with code NSFileNoSuchFileError
(or other relevant NSFile... codes), or when NSError.domain is
NSPOSIXErrorDomain with code ENOENT (errNo 2); keep the existing message-based
contains() check only as a last-resort fallback. Update the
shouldClearLoadedFile(for:) function to perform these domain/code checks (using
NSError) before falling back to localized string matching.
- Around line 161-175: The loop causes an N+1 pattern: FileViewerViewModel uses
syncService.listRecentCommits and then calls syncService.listCommitFiles
sequentially up to 25 times; change this to avoid sequential per-commit network
calls by either (A) adding a server-side endpoint on syncService such as
findLastCommitForFile(laneId:filePath:) and call that to set lastCommitTitle and
lastCommitDateText via relativeDateDescription(from:), or (B) if server change
isn’t possible, modify the client to perform the lookups in parallel using
TaskGroup (concurrently call listCommitFiles for the commits returned by
listRecentCommits and stop when the first contains the file) to eliminate
sequential waiting; update the codepaths that reference
syncService.listCommitFiles and ensure the error handling remains best-effort.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a47dd0e3-9c74-4364-8287-94dd14b2f3d5
⛔ Files ignored due to path filters (1)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxproj
📒 Files selected for processing (17)
.factory/library/architecture.md.factory/services.yaml.gitignoreapps/ios/ADE/Views/Files/FileOperationsHelper.swiftapps/ios/ADE/Views/Files/FileSearchView.swiftapps/ios/ADE/Views/Files/FileSearchViewModel.swiftapps/ios/ADE/Views/Files/FileTreeView.swiftapps/ios/ADE/Views/Files/FileTreeViewModel.swiftapps/ios/ADE/Views/Files/FileViewerChromeViews.swiftapps/ios/ADE/Views/Files/FileViewerCodeEditorView.swiftapps/ios/ADE/Views/Files/FileViewerHelpers.swiftapps/ios/ADE/Views/Files/FileViewerRenderingViews.swiftapps/ios/ADE/Views/Files/FileViewerView.swiftapps/ios/ADE/Views/Files/FileViewerViewModel.swiftapps/ios/ADE/Views/Files/FilesTabView.swiftapps/ios/ADE/Views/FilesTabView.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/ios/ADE/Views/FilesTabView.swift
Comment on lines
+2
to
+5
| build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5 | ||
| test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40 | ||
| build_full: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO | ||
| test_full: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify available destinations for this scheme on the runner.
xcodebuild -project apps/ios/ADE.xcodeproj -scheme ADE -showdestinations | sed -n '1,220p'
# Verify whether the hard-coded name exists.
xcodebuild -project apps/ios/ADE.xcodeproj -scheme ADE -showdestinations | rg -n "iPhone 17 Pro Max" || trueRepository: arul28/ADE
Length of output: 104
🏁 Script executed:
# Check if the services.yaml file exists and examine its content
find . -name "services.yaml" -o -name "synthesis.json" | head -20Repository: arul28/ADE
Length of output: 188
🏁 Script executed:
# If services.yaml exists, read it
if [ -f ".factory/services.yaml" ]; then
echo "=== .factory/services.yaml ==="
cat -n ".factory/services.yaml"
else
echo ".factory/services.yaml not found"
fiRepository: arul28/ADE
Length of output: 898
🏁 Script executed:
# Check for synthesis.json file
if [ -f ".factory/validation/foundation/scrutiny/synthesis.json" ]; then
echo "=== .factory/validation/foundation/scrutiny/synthesis.json ==="
cat -n ".factory/validation/foundation/scrutiny/synthesis.json" | head -80
else
echo "synthesis.json not found"
fiRepository: arul28/ADE
Length of output: 2277
Avoid hard-coding a simulator name that may not exist on CI images.
Lines 2–5 hard-code name=iPhone 17 Pro Max. If that device/runtime is unavailable, all iOS commands fail. The .factory/validation/foundation/scrutiny/synthesis.json shows a better pattern using ID-based destinations (e.g., id=2107A402-C2A7-4323-AF26-74A0AC406C44), which is more reliable across environments.
💡 Proposed fix
- build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5
- test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40
- build_full: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO
- test_full: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO
+ build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination "${IOS_DESTINATION:-platform=iOS Simulator}" -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5
+ test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination "${IOS_DESTINATION:-platform=iOS Simulator}" -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40
+ build_full: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination "${IOS_DESTINATION:-platform=iOS Simulator}" -configuration Debug CODE_SIGNING_ALLOWED=NO
+ test_full: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination "${IOS_DESTINATION:-platform=iOS Simulator}" -configuration Debug CODE_SIGNING_ALLOWED=NO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/services.yaml around lines 2 - 5, Replace the hard-coded simulator
name in the xcodebuild destination arguments used by the build, test,
build_full, and test_full targets: stop using "name=iPhone 17 Pro Max" and
instead reference a simulator by ID (e.g., "id=<SIMULATOR_UDID>") or a lookup
that resolves an available simulator UDID at runtime; update each destination
string to use "platform=iOS Simulator,id=..." (or a CI-friendly UDID variable)
so xcodebuild will run reliably on CI images that may not include that named
device.
Comment on lines
+2
to
+3
| build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5 | ||
| test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Without pipefail (expected misleading exit=0):"
bash -lc 'false | tail -n 1; echo "exit=$?"'
echo "With pipefail (expected exit=1):"
bash -lc 'set -o pipefail; false | tail -n 1; echo "exit=$?"'Repository: arul28/ADE
Length of output: 362
🏁 Script executed:
cd /tmp && find . -name "services.yaml" -o -name ".factory" -type d 2>/dev/null | head -20Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
git ls-files | grep -E "(\.factory|services\.yaml)" | head -20Repository: arul28/ADE
Length of output: 746
🏁 Script executed:
cat -n .factory/services.yaml 2>/dev/null || echo "File not found"Repository: arul28/ADE
Length of output: 867
🏁 Script executed:
rg "services.yaml" --type yaml --type json --type md -A 2 -B 2 | head -50Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
find . -name "*.sh" -o -name "Makefile" -o -name ".github" -type d | head -10Repository: arul28/ADE
Length of output: 63
🏁 Script executed:
cat -n .factory/library/architecture.md | head -100Repository: arul28/ADE
Length of output: 3795
🏁 Script executed:
rg "services" --type yaml -B 3 -A 3 | head -60Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
ls -la .factory/Repository: arul28/ADE
Length of output: 344
🏁 Script executed:
cat .github/workflows/* 2>/dev/null | head -150Repository: arul28/ADE
Length of output: 4525
🏁 Script executed:
find . -name "README*" -o -name "*.md" | xargs rg -l "services.yaml|\.factory|commands:" 2>/dev/null | head -10Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
cat README.md | head -100Repository: arul28/ADE
Length of output: 4860
🏁 Script executed:
rg "commands:" -B 5 -A 5 --type yaml | head -80Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
rg "\.factory" --type md --type json --type ts --type js -B 2 -A 2 | head -100Repository: arul28/ADE
Length of output: 36
🏁 Script executed:
find . -name "*.json" -o -name "*.config.*" | xargs rg "services" 2>/dev/null | head -40Repository: arul28/ADE
Length of output: 482
🏁 Script executed:
rg "iPhone|iOS|xcodebuild" --type yaml --type json --type md -B 2 -A 2 | head -100Repository: arul28/ADE
Length of output: 8383
🏁 Script executed:
find .factory -name "*.md" -o -name "*.json" | xargs cat 2>/dev/null | head -200Repository: arul28/ADE
Length of output: 15783
🏁 Script executed:
rg "commands:" -B 10 -A 10 --type yamlRepository: arul28/ADE
Length of output: 36
🏁 Script executed:
git log --oneline .factory/services.yaml 2>/dev/null | head -10Repository: arul28/ADE
Length of output: 74
Preserve xcodebuild failure status when piping to tail.
Lines 2–3 pipe xcodebuild output to tail, which masks build and test failures by returning tail's exit code (0) instead of xcodebuild's. This can cause failed builds/tests to be marked as passing in CI.
Proposed fix
- build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5
- test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40
+ build: bash -lc "set -o pipefail; xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5"
+ test: bash -lc "set -o pipefail; xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40"Additionally, lines 2–5 hard-code the simulator device name iPhone 17 Pro Max. CI environments may not have this specific device available. Consider using a UUID-based destination or a variable to support flexible environments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5 | |
| test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40 | |
| build: bash -lc "set -o pipefail; xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5" | |
| test: bash -lc "set -o pipefail; xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -40" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/services.yaml around lines 2 - 3, Update the two service commands
("build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE
-destination 'platform=iOS Simulator,name=iPhone 17 Pro Max' -configuration
Debug CODE_SIGNING_ALLOWED=NO 2>&1 | tail -5" and "test: xcodebuild test
-project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS
Simulator,name=iPhone 17 Pro Max' -configuration Debug CODE_SIGNING_ALLOWED=NO
2>&1 | tail -40") so the xcodebuild exit status is preserved when piping: enable
bash pipefail or capture xcodebuild's exit code (e.g. use set -o pipefail or
inspect PIPESTATUS) and return that code after tail so CI fails on xcodebuild
errors; also replace the hard-coded destination device name with a configurable
destination (use an environment variable like IOS_DESTINATION or a simulator
UDID resolved dynamically via xcrun simctl) to avoid relying on "iPhone 17 Pro
Max" being present in CI.
| *.db | ||
|
|
||
| # Build outputs | ||
| build/ |
There was a problem hiding this comment.
Scope build/ ignore rule to repo root to avoid masking required desktop packaging assets.
On Line 21, build/ is recursive and will match nested paths like apps/desktop/build/. That conflicts with desktop packaging assets referenced in apps/desktop/package.json (icon/entitlements files). Please anchor this ignore to root (or explicitly unignore desktop assets).
Proposed fix
- build/
+ /build/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| build/ | |
| /build/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 21, The recursive ignore rule "build/" currently matches
nested build directories (e.g., apps/desktop/build/) and hides desktop packaging
assets referenced by apps/desktop/package.json; update the .gitignore by scoping
the pattern to the repo root (change "build/" to "/build/") or alternatively add
explicit negation rules to unignore the desktop assets (e.g., add negation
entries for the specific icon/entitlements paths under apps/desktop) so the
desktop packaging files are not masked.
Comment on lines
+260
to
+271
| private func validatePromptValue(_ value: String, prompt: FilesPathPrompt) -> Bool { | ||
| if let validationError = filesNameValidationError( | ||
| for: value, | ||
| existingNodes: nodes, | ||
| excluding: prompt.node?.path | ||
| ) { | ||
| actionErrorMessage = validationError | ||
| return false | ||
| } | ||
| actionErrorMessage = nil | ||
| return true | ||
| } |
There was a problem hiding this comment.
Validation checks root nodes instead of siblings in the target directory.
validatePromptValue uses self.nodes (root-level nodes), but when creating a file in a subdirectory, the conflict check should be against sibling nodes in that directory (childNodesByPath[prompt.basePath]), not root nodes.
🐛 Fix validation to check correct directory
private func validatePromptValue(_ value: String, prompt: FilesPathPrompt) -> Bool {
+ let siblingNodes = prompt.basePath.isEmpty ? nodes : (childNodesByPath[prompt.basePath] ?? [])
if let validationError = filesNameValidationError(
for: value,
- existingNodes: nodes,
+ existingNodes: siblingNodes,
excluding: prompt.node?.path
) {
actionErrorMessage = validationError
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func validatePromptValue(_ value: String, prompt: FilesPathPrompt) -> Bool { | |
| if let validationError = filesNameValidationError( | |
| for: value, | |
| existingNodes: nodes, | |
| excluding: prompt.node?.path | |
| ) { | |
| actionErrorMessage = validationError | |
| return false | |
| } | |
| actionErrorMessage = nil | |
| return true | |
| } | |
| private func validatePromptValue(_ value: String, prompt: FilesPathPrompt) -> Bool { | |
| let siblingNodes = prompt.basePath.isEmpty ? nodes : (childNodesByPath[prompt.basePath] ?? []) | |
| if let validationError = filesNameValidationError( | |
| for: value, | |
| existingNodes: siblingNodes, | |
| excluding: prompt.node?.path | |
| ) { | |
| actionErrorMessage = validationError | |
| return false | |
| } | |
| actionErrorMessage = nil | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Files/FileTreeViewModel.swift` around lines 260 - 271,
validatePromptValue currently validates against root-level nodes (`nodes`)
causing false conflicts when creating files in subdirectories; change it to
validate against the correct sibling list by passing the directory's child nodes
(e.g., `childNodesByPath[prompt.basePath] ?? []`) to filesNameValidationError
instead of `nodes`. Keep the existing exclusion (`prompt.node?.path`) and
assignment to `actionErrorMessage`/return behavior the same, but replace the
`existingNodes: nodes` argument with the sibling nodes lookup so conflicts are
checked within the target directory.
Comment on lines
+35
to
+43
| if uiView.textView.text != text { | ||
| uiView.textView.text = text | ||
| uiView.updateLineNumbers() | ||
| } | ||
|
|
||
| let safeSelection = selection.clamped(toUTF16Length: (text as NSString).length) | ||
| if uiView.textView.selectedRange != safeSelection { | ||
| uiView.textView.selectedRange = safeSelection | ||
| } |
There was a problem hiding this comment.
Text replacement may disrupt cursor during rapid typing.
When textViewDidChange fires, updating parent.text triggers SwiftUI to call updateUIView. If the text differs (line 35), textView.text gets reassigned, which can reset the cursor position mid-keystroke. The selection restoration at lines 40-43 may not fully prevent this during rapid input.
Consider tracking an "isUpdating" flag to skip updateUIView text replacement when the change originated from the delegate.
🔧 Possible fix pattern
final class Coordinator: NSObject, UITextViewDelegate {
var parent: FilesCodeEditorView
weak var container: FilesCodeEditorContainerView?
var isUpdatingFromDelegate = false // Add flag
func textViewDidChange(_ textView: UITextView) {
guard textView === container?.textView else { return }
isUpdatingFromDelegate = true
parent.text = textView.text
isUpdatingFromDelegate = false
container?.updateLineNumbers()
}
}
// In updateUIView:
func updateUIView(_ uiView: FilesCodeEditorContainerView, context: Context) {
// ...
if uiView.textView.text != text && !context.coordinator.isUpdatingFromDelegate {
uiView.textView.text = text
uiView.updateLineNumbers()
}
// ...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Files/FileViewerCodeEditorView.swift` around lines 35 -
43, The updateUIView is overwriting the UITextView.text during delegate-driven
changes, resetting the cursor; add a Bool flag (e.g., isUpdatingFromDelegate) to
Coordinator (class Coordinator: NSObject, UITextViewDelegate) and set it true
before assigning parent.text in textViewDidChange and false after, and call
container?.updateLineNumbers() from the delegate; then in updateUIView (the
method with uiView.textView.text != text) guard that replacement with
!context.coordinator.isUpdatingFromDelegate so you skip reassigning
uiView.textView.text when the change originated from the delegate, preserving
selection.
Comment on lines
+174
to
+179
| .task { | ||
| await viewModel.load(syncService: syncService, workspace: workspace, relativePath: relativePath, isFilesLive: isFilesLive) | ||
| } | ||
| .task(id: syncService.localStateRevision) { | ||
| await viewModel.load(syncService: syncService, workspace: workspace, relativePath: relativePath, isFilesLive: isFilesLive, refreshDiff: viewModel.mode == .diff) | ||
| } |
There was a problem hiding this comment.
Duplicate load calls on initial appear.
Both .task modifiers will fire when the view first appears: the first unconditionally loads, and the second triggers because syncService.localStateRevision gets its initial value. This causes two load requests on initial appear.
Consider guarding the revision-based task to skip the initial load or combining the logic.
🔧 Possible fix
.task {
await viewModel.load(syncService: syncService, workspace: workspace, relativePath: relativePath, isFilesLive: isFilesLive)
}
- .task(id: syncService.localStateRevision) {
- await viewModel.load(syncService: syncService, workspace: workspace, relativePath: relativePath, isFilesLive: isFilesLive, refreshDiff: viewModel.mode == .diff)
- }
+ .onChange(of: syncService.localStateRevision) { _, _ in
+ Task {
+ await viewModel.load(syncService: syncService, workspace: workspace, relativePath: relativePath, isFilesLive: isFilesLive, refreshDiff: viewModel.mode == .diff)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Files/FileViewerView.swift` around lines 174 - 179, The
view triggers viewModel.load twice on first appear because both .task and the
.task(id: syncService.localStateRevision) run; modify the revision-based task
(the .task with id: syncService.localStateRevision) to skip its first invocation
or combine both behaviors into one task: add a guard (e.g., an internal
hasAppeared or previousRevision check) so that
viewModel.load(syncService:syncService, workspace:workspace,
relativePath:relativePath, isFilesLive:isFilesLive, refreshDiff:viewModel.mode
== .diff) only runs on subsequent revision changes (or only when refreshDiff
needs to be applied), leaving the unconditional initial .task to perform the
first load and ensuring refreshDiff is still applied on later revisions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
vitest, Xcode tests) to validate no regressions🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests