feat: add sidebar-based workspace navigation#1
Merged
ratulsarna merged 27 commits intomainfrom Mar 16, 2026
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Correct file paths, API method names, test source set, add session-workspace association and cross-reset persistence details. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 tasks across 4 chunks: SidebarViewModel, sidebar UI, navigation/toolbar rewiring, and cleanup/deletion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
On re-expand, previously-loaded sessions display instantly while a background fetch updates them. Loading indicator only shows on first fetch when there are no cached sessions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…kspace path to toolbar Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ring Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The GlassEffect type cannot be referenced directly as a variable type. Use inline .glassEffect() calls matching the existing codebase pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove glass card wrapping, use plain rows with subtle backgrounds - Remove chevron from workspace headers - Add relative timestamps to session rows (now, 1h, 2d) - Active session gets subtle highlight background + accent dot - Workspaces stay in natural order (no sorting active to top) - Remove isActive param — cards are visually uniform - Cleaner spacing and lighter visual weight throughout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On iPhone, NavigationSplitView collapses into a stack — setting columnVisibility programmatically doesn't pop the sidebar. Use @Environment(\.dismiss) to properly navigate back. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NavigationSplitView on iPhone doesn't support programmatic dismissal — columnVisibility changes are ignored on compact widths. Switch to pushing the sidebar as a .sidebar route on the existing NavigationStack. Tapping a session pops the route to return to chat. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Updated `getSessions` method across multiple interfaces and implementations to include an optional `directory` parameter. - Adjusted the logic in `MockState` and `MockOpenCodeApi` to filter sessions based on the provided directory. - Modified `SidebarViewModel` to utilize the new directory parameter when loading sessions for a workspace. - Updated related tests to ensure proper functionality with the new directory filtering feature.
…specs - Deleted the implementation plan and design specification documents for the sidebar navigation rework. - This change is part of the transition to a unified sidebar that integrates workspaces and sessions, streamlining the user experience.
- Updated the `addWorkspace` method to expose workspace creation errors in the UI state. - Introduced `workspaceCreationError` property in `SidebarUiState` to store error messages. - Modified the `WorkspacesSidebarView` to display an alert for workspace creation errors. - Added tests to verify error handling during workspace addition failures.
- Replaced hardcoded DEVELOPMENT_TEAM identifier with a dynamic variable "$(TEAM_ID)" in multiple project settings. - This change enhances flexibility for team configurations across different environments.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConsolidates Sessions and Workspaces view models into a new SidebarViewModel, adds optional directory filtering to session queries across API/repository/mock layers, removes legacy Sessions/Workspaces UI and tests, and introduces an iOS sidebar UI with new components and toolbar changes. Also updates .gitignore for Git worktrees. Changes
Sequence DiagramsequenceDiagram
participant User
participant SidebarUI as "WorkspacesSidebarView / iOS UI"
participant SVM as "SidebarViewModel"
participant WSRepo as "WorkspaceRepository"
participant SRepo as "SessionRepository"
participant Settings as "AppSettings"
User->>SidebarUI: open sidebar
SidebarUI->>SVM: init / request workspaces
SVM->>WSRepo: getWorkspaces()
WSRepo-->>SVM: workspaces list
SVM->>Settings: get activeWorkspaceId
Settings-->>SVM: activeWorkspaceId
SVM->>SRepo: getSessions(directory=workspaceDirectory)
SRepo-->>SVM: sessions (filtered by directory)
SVM-->>SidebarUI: emit SidebarUiState (workspaces + sessions)
User->>SidebarUI: select workspace / session
SidebarUI->>SVM: switchWorkspace(projectId, sessionId?)
SVM->>Settings: persist current session id
SVM->>WSRepo: activateWorkspace(projectId)
WSRepo-->>SVM: activation result
SVM->>SRepo: getSessions(directory=newWorkspaceDirectory)
SRepo-->>SVM: sessions for new workspace
SVM-->>SidebarUI: update UI state (active workspace, sessions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Owner
Author
|
@coderabbitai review |
Owner
Author
|
@codex review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt (2)
196-225: Consider providing user feedback on session creation failure.Similar to
switchWorkspace, thecreateSessionfunction logs failures but doesn't update UI state with an error message for the user.♻️ Add error feedback to UI state
fun createSession(workspaceProjectId: String) { if (_uiState.value.isCreatingSession) return - _uiState.update { it.copy(isCreatingSession = true) } + _uiState.update { it.copy(isCreatingSession = true, sessionCreationError = null) } viewModelScope.launch { // ... existing logic ... sessionRepository.createSession(parentId = null) .onSuccess { session -> _uiState.update { it.copy( isCreatingSession = false, createdSessionId = session.id, switchedWorkspaceId = if (!isActiveWorkspace) workspaceProjectId else null ) } } .onFailure { error -> OcMobileLog.w(TAG, "Failed to create session: ${error.message}") - _uiState.update { it.copy(isCreatingSession = false) } + _uiState.update { it.copy( + isCreatingSession = false, + sessionCreationError = error.message ?: "Failed to create session" + ) } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt` around lines 196 - 225, The createSession flow logs failures but never surfaces an error to the UI; update the _uiState in both failure branches so the UI receives an error message and resets isCreatingSession. Specifically, modify createSession (and the failure lambda after workspaceRepository.activateWorkspace and the onFailure after sessionRepository.createSession) to set a new/appropriate error field on _uiState (e.g., sessionCreationError or errorMessage) and clear/reset createdSessionId/switchedWorkspaceId as needed, while still setting isCreatingSession = false so the UI can display the error to the user.
169-190: Consider providing user feedback on workspace switch failure.The
switchWorkspacefunction logs failures but doesn't surface errors to the UI state. Users won't know if the workspace switch failed.♻️ Add error feedback to UI state
fun switchWorkspace(projectId: String, sessionId: String?) { if (_uiState.value.isSwitchingWorkspace) return - _uiState.update { it.copy(isSwitchingWorkspace = true) } + _uiState.update { it.copy(isSwitchingWorkspace = true, workspaceSwitchError = null) } viewModelScope.launch { workspaceRepository.activateWorkspace(projectId) .onSuccess { if (sessionId != null) { sessionRepository.updateCurrentSessionId(sessionId) .onFailure { error -> OcMobileLog.w(TAG, "Failed to persist session $sessionId for workspace $projectId: ${error.message}") } } _uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) } } .onFailure { error -> OcMobileLog.w(TAG, "Failed to switch workspace: ${error.message}") - _uiState.update { it.copy(isSwitchingWorkspace = false) } + _uiState.update { it.copy( + isSwitchingWorkspace = false, + workspaceSwitchError = error.message ?: "Failed to switch workspace" + ) } } } }And add to
SidebarUiState:data class SidebarUiState( // ... existing fields ... val workspaceSwitchError: String? = null )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt` around lines 169 - 190, The switchWorkspace function logs failures but never surfaces them to the UI; add a nullable workspaceSwitchError:String? property to SidebarUiState and update _uiState inside switchWorkspace: clear workspaceSwitchError (set null) when activation succeeds (alongside setting isSwitchingWorkspace = false and switchedWorkspaceId = projectId), and set workspaceSwitchError to a user-friendly message (e.g., error.message) when workspaceRepository.activateWorkspace or sessionRepository.updateCurrentSessionId fail; update the onSuccess and onFailure handlers in switchWorkspace (referencing switchWorkspace, _uiState, workspaceRepository.activateWorkspace, sessionRepository.updateCurrentSessionId, isSwitchingWorkspace, switchedWorkspaceId) so the UI can display the error.iosApp/iosApp/SwiftUIInterop/WorkspaceCardView.swift (2)
162-172: Consider using RelativeDateTimeFormatter for localization support.The manual time formatting works but won't adapt to user locale settings. For production, consider using
RelativeDateTimeFormatterfor proper localization.♻️ Optional: Use RelativeDateTimeFormatter
private func relativeTime(_ instant: KotlinInstant) -> String { let epochMs = instant.toEpochMilliseconds() let date = Date(timeIntervalSince1970: TimeInterval(epochMs) / 1000.0) let formatter = RelativeDateTimeFormatter() formatter.unitsStyle = .abbreviated return formatter.localizedString(for: date, relativeTo: Date()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iosApp/iosApp/SwiftUIInterop/WorkspaceCardView.swift` around lines 162 - 172, The relativeTime(_ instant: KotlinInstant) function builds durations manually and doesn't localize; replace the manual formatting with RelativeDateTimeFormatter to respect user locales: convert the KotlinInstant to a Date as you do now, create a RelativeDateTimeFormatter (set unitsStyle = .abbreviated if you want compact output), and return formatter.localizedString(for: date, relativeTo: Date()); update the relativeTime function to use this formatter so outputs are localized and consistent across locales.
31-33: Remove unnecessaryInt()cast.The
sessionsproperty is a KotlinList<Session>, which bridges to a SwiftArray<Session>in KMM. The.countproperty on Swift arrays is already anInt, so the explicitInt()conversion on line 32 is redundant. Simplify to:Suggested change
private var hiddenCount: Int { max(0, workspaceWithSessions.sessions.count - 3) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iosApp/iosApp/SwiftUIInterop/WorkspaceCardView.swift` around lines 31 - 33, In WorkspaceCardView, simplify the hiddenCount computed property by removing the unnecessary Int(...) cast around workspaceWithSessions.sessions.count — sessions.count is already an Int in Swift (bridged from KMM), so update hiddenCount to compute max(0, workspaceWithSessions.sessions.count - 3) using the existing hiddenCount property and workspaceWithSessions.sessions references.composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt (1)
105-117: Decouple this assertion from init-side prefetching.The exact
requestedDirectorieslist now depends on the active workspace being auto-loaded once during startup. Clearing the list after the initialadvanceUntilIdle()(or asserting only the last call) would make this test validate theproj-2directory propagation directly.Proposed tightening
val vm = SidebarViewModel( workspaceRepository = repo, sessionRepository = sessionRepo, appSettings = appSettings ) advanceUntilIdle() + requestedDirectories.clear() vm.loadSessionsForWorkspace("proj-2") advanceUntilIdle() val loaded = vm.uiState.value.workspaces.first { it.workspace.projectId == "proj-2" } - assertEquals(listOf<String?>("/path/to/project-a", "/path/to/project-b"), requestedDirectories) + assertEquals(listOf<String?>("/path/to/project-b"), requestedDirectories) assertEquals(listOf("ses-b"), loaded.sessions.map { it.id })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt` around lines 105 - 117, The test's assertion on requestedDirectories is polluted by the ViewModel's init prefetch; after creating SidebarViewModel and calling advanceUntilIdle(), clear or reset the requestedDirectories collection (or capture/replace it) before calling vm.loadSessionsForWorkspace("proj-2") and the second advanceUntilIdle(), then assert only the subsequent entries (or assert the last call) to verify proj-2 propagation; update the SidebarViewModelTest to reset requestedDirectories (or assert requestedDirectories.last() / the post-load snapshot) and keep the existing vm.loadSessionsForWorkspace("proj-2") and loaded.sessions assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt`:
- Around line 319-321: The fake's getActiveWorkspaceSnapshot() and
ensureInitialized() are returning the initial constructor value instead of the
mutable state updated by activateWorkspace(), causing stale reads; modify the
fake so both getActiveWorkspaceSnapshot() and ensureInitialized() read from the
mutable activeWorkspace backing field (the one that activateWorkspace() updates)
rather than a captured constructor parameter, and ensure any other similar
methods around the other block (lines referencing the same fake between 324-330)
also reference that mutable property so workspace switches are reflected
immediately.
- Around line 173-214: The test currently only checks final state; modify
SidebarViewModel_switchWorkspacePersistsSessionIdBeforeActivating to record the
call order and assert the session update occurs before workspace activation: add
a shared mutable list (e.g., callOrder) captured by FakeSessionRepository's
updateCurrentSessionIdHandler (push "updateSession" or include sessionId) and by
FakeWorkspaceRepository's activateHandler (push "activate:<id>"), then after
vm.switchWorkspace(...) assert that callOrder shows the update entry appears
before the activate entry (e.g., ["updateSession","activate:proj-2"]); keep
existing final-state assertions but add this ordering assertion to prove
updateCurrentSessionId() runs before activateWorkspace().
In `@iosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift`:
- Around line 80-92: The .task(id:) handlers in WorkspacesSidebarView are
performing one-shot side effects (consuming latestUiState?.switchedWorkspaceId
and latestUiState?.createdSessionId, calling viewModel.clearWorkspaceSwitch(),
viewModel.clearCreatedSession(), onRequestAppReset(), and onSelectSession())
inside a transient sidebar view; move that logic out of WorkspacesSidebarView
and into the always-on collector in SwiftUIAppRootView (which already observes
the shared SidebarViewModel/uiState), so the app-scoped SidebarViewModel flags
are consumed and cleared there to avoid stale latched behavior when the sidebar
is dismissed; update WorkspacesSidebarView to remove these .task handlers and
ensure SwiftUIAppRootView performs the checks for switchedWorkspaceId and
createdSessionId, calls viewModel.clearWorkspaceSwitch()/clearCreatedSession(),
and then invokes onRequestAppReset()/onSelectSession() as appropriate.
- Around line 13-14: The code uses expanded.isEmpty to detect "first load" which
breaks once the user collapses everything; introduce a dedicated one-time flag
(e.g. `@State` private var didAutoExpandInitially: Bool = false) in
WorkspacesSidebarView and replace checks that rely on expanded.isEmpty (and the
analogous fullyExpanded usage) with !didAutoExpandInitially && /* other
conditions */; after performing the initial auto-expand and calling
loadSessionsForWorkspace, set didAutoExpandInitially = true so subsequent
uiState emissions don't re-expand workspaces when the user has manually
collapsed them.
- Around line 48-50: The sheet bound to isShowingAddWorkspace should be
prevented from interactive swipe-to-dismiss while workspace creation is in
progress: add an interactiveDismissDisabled(...) modifier to the sheet (on
addWorkspaceSheet or the sheet content) and bind it to the creation-running flag
(create a `@State/`@Published isCreatingWorkspace if none exists), update that
flag to true when creation starts and false when creation finishes, and only set
isShowingAddWorkspace = false after success/failure handling completes so the
sheet cannot be dismissed mid-operation.
---
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt`:
- Around line 196-225: The createSession flow logs failures but never surfaces
an error to the UI; update the _uiState in both failure branches so the UI
receives an error message and resets isCreatingSession. Specifically, modify
createSession (and the failure lambda after
workspaceRepository.activateWorkspace and the onFailure after
sessionRepository.createSession) to set a new/appropriate error field on
_uiState (e.g., sessionCreationError or errorMessage) and clear/reset
createdSessionId/switchedWorkspaceId as needed, while still setting
isCreatingSession = false so the UI can display the error to the user.
- Around line 169-190: The switchWorkspace function logs failures but never
surfaces them to the UI; add a nullable workspaceSwitchError:String? property to
SidebarUiState and update _uiState inside switchWorkspace: clear
workspaceSwitchError (set null) when activation succeeds (alongside setting
isSwitchingWorkspace = false and switchedWorkspaceId = projectId), and set
workspaceSwitchError to a user-friendly message (e.g., error.message) when
workspaceRepository.activateWorkspace or
sessionRepository.updateCurrentSessionId fail; update the onSuccess and
onFailure handlers in switchWorkspace (referencing switchWorkspace, _uiState,
workspaceRepository.activateWorkspace, sessionRepository.updateCurrentSessionId,
isSwitchingWorkspace, switchedWorkspaceId) so the UI can display the error.
In
`@composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt`:
- Around line 105-117: The test's assertion on requestedDirectories is polluted
by the ViewModel's init prefetch; after creating SidebarViewModel and calling
advanceUntilIdle(), clear or reset the requestedDirectories collection (or
capture/replace it) before calling vm.loadSessionsForWorkspace("proj-2") and the
second advanceUntilIdle(), then assert only the subsequent entries (or assert
the last call) to verify proj-2 propagation; update the SidebarViewModelTest to
reset requestedDirectories (or assert requestedDirectories.last() / the
post-load snapshot) and keep the existing vm.loadSessionsForWorkspace("proj-2")
and loaded.sessions assertion.
In `@iosApp/iosApp/SwiftUIInterop/WorkspaceCardView.swift`:
- Around line 162-172: The relativeTime(_ instant: KotlinInstant) function
builds durations manually and doesn't localize; replace the manual formatting
with RelativeDateTimeFormatter to respect user locales: convert the
KotlinInstant to a Date as you do now, create a RelativeDateTimeFormatter (set
unitsStyle = .abbreviated if you want compact output), and return
formatter.localizedString(for: date, relativeTo: Date()); update the
relativeTime function to use this formatter so outputs are localized and
consistent across locales.
- Around line 31-33: In WorkspaceCardView, simplify the hiddenCount computed
property by removing the unnecessary Int(...) cast around
workspaceWithSessions.sessions.count — sessions.count is already an Int in Swift
(bridged from KMM), so update hiddenCount to compute max(0,
workspaceWithSessions.sessions.count - 3) using the existing hiddenCount
property and workspaceWithSessions.sessions references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8b68600-93c9-4b4e-a184-b04628724916
📒 Files selected for processing (27)
.gitignorecomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/IosViewModelOwners.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/data/api/OpenCodeApi.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/data/api/OpenCodeApiImpl.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/data/mock/MockOpenCodeApi.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/data/mock/MockState.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/data/repository/SessionRepositoryImpl.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/di/AppModule.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/domain/repository/SessionRepository.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sessions/SessionsViewModel.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.ktcomposeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/workspaces/WorkspacesViewModel.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/data/repository/AgentRepositoryDefaultSelectionTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/data/repository/ModelRepositoryDefaultSelectionTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/domain/repository/WorkspaceRepositoryTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/chat/ChatViewModelBootstrapTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/chat/ChatViewModelSendDefaultsTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sessions/SessionsViewModelTest.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.ktiosApp/iosApp/ChatUIKit/ChatScreenChromeView.swiftiosApp/iosApp/ChatUIKit/SwiftUIChatUIKitView.swiftiosApp/iosApp/SwiftUIInterop/SwiftUIAppRootView.swiftiosApp/iosApp/SwiftUIInterop/SwiftUISessionsViews.swiftiosApp/iosApp/SwiftUIInterop/SwiftUISettingsViews.swiftiosApp/iosApp/SwiftUIInterop/SwiftUIWorkspacesViews.swiftiosApp/iosApp/SwiftUIInterop/WorkspaceCardView.swiftiosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift
💤 Files with no reviewable changes (6)
- composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sessions/SessionsViewModelTest.kt
- composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sessions/SessionsViewModel.kt
- composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/workspaces/WorkspacesViewModel.kt
- iosApp/iosApp/SwiftUIInterop/SwiftUIWorkspacesViews.swift
- iosApp/iosApp/SwiftUIInterop/SwiftUISessionsViews.swift
- iosApp/iosApp/SwiftUIInterop/SwiftUISettingsViews.swift
Comment on lines
+173
to
+214
| fun SidebarViewModel_switchWorkspacePersistsSessionIdBeforeActivating() = runTest(dispatcher) { | ||
| val activatedIds = mutableListOf<String>() | ||
| val appSettings = MockAppSettings() | ||
| appSettings.setActiveServerId("server-1") | ||
| appSettings.setInstallationIdForServer("server-1", "inst-1") | ||
|
|
||
| val workspace1 = workspace("proj-1", "/p1") | ||
| val workspace2 = workspace("proj-2", "/p2") | ||
| appSettings.setWorkspacesForInstallation("inst-1", listOf(workspace1, workspace2)) | ||
| appSettings.setActiveWorkspace("inst-1", workspace1) | ||
|
|
||
| val repo = FakeWorkspaceRepository( | ||
| workspaces = listOf(workspace1, workspace2), | ||
| activeWorkspace = workspace1, | ||
| appSettings = appSettings, | ||
| activateHandler = { id -> | ||
| activatedIds.add(id) | ||
| Result.success(Unit) | ||
| } | ||
| ) | ||
| val sessionRepo = FakeSessionRepository( | ||
| updateCurrentSessionIdHandler = { sessionId -> | ||
| appSettings.setCurrentSessionId(sessionId) | ||
| Result.success(Unit) | ||
| } | ||
| ) | ||
|
|
||
| val vm = SidebarViewModel( | ||
| workspaceRepository = repo, | ||
| sessionRepository = sessionRepo, | ||
| appSettings = appSettings | ||
| ) | ||
| advanceUntilIdle() | ||
|
|
||
| vm.switchWorkspace("proj-2", "ses-target") | ||
| advanceUntilIdle() | ||
|
|
||
| assertEquals("proj-2", appSettings.getActiveWorkspaceSnapshot()?.projectId) | ||
| assertEquals("ses-target", appSettings.getCurrentSessionIdSnapshot()) | ||
| assertEquals(listOf("proj-2"), activatedIds) | ||
| assertEquals("proj-2", vm.uiState.value.switchedWorkspaceId) | ||
| } |
There was a problem hiding this comment.
This test doesn't prove “before activating” yet.
The current assertions only validate final state, so activateWorkspace() could happen before updateCurrentSessionId() and the test would still pass. Capture the call sequence and assert it explicitly.
Proposed change
- val activatedIds = mutableListOf<String>()
+ val operations = mutableListOf<String>()
val appSettings = MockAppSettings()
appSettings.setActiveServerId("server-1")
appSettings.setInstallationIdForServer("server-1", "inst-1")
@@
workspaces = listOf(workspace1, workspace2),
activeWorkspace = workspace1,
appSettings = appSettings,
activateHandler = { id ->
- activatedIds.add(id)
+ operations.add("activate:$id")
Result.success(Unit)
}
)
val sessionRepo = FakeSessionRepository(
updateCurrentSessionIdHandler = { sessionId ->
+ operations.add("session:$sessionId")
appSettings.setCurrentSessionId(sessionId)
Result.success(Unit)
}
)
@@
assertEquals("proj-2", appSettings.getActiveWorkspaceSnapshot()?.projectId)
assertEquals("ses-target", appSettings.getCurrentSessionIdSnapshot())
- assertEquals(listOf("proj-2"), activatedIds)
+ assertEquals(listOf("session:ses-target", "activate:proj-2"), operations)
assertEquals("proj-2", vm.uiState.value.switchedWorkspaceId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt`
around lines 173 - 214, The test currently only checks final state; modify
SidebarViewModel_switchWorkspacePersistsSessionIdBeforeActivating to record the
call order and assert the session update occurs before workspace activation: add
a shared mutable list (e.g., callOrder) captured by FakeSessionRepository's
updateCurrentSessionIdHandler (push "updateSession" or include sessionId) and by
FakeWorkspaceRepository's activateHandler (push "activate:<id>"), then after
vm.switchWorkspace(...) assert that callOrder shows the update entry appears
before the activate entry (e.g., ["updateSession","activate:proj-2"]); keep
existing final-state assertions but add this ordering assertion to prove
updateCurrentSessionId() runs before activateWorkspace().
composeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+48
to
+50
| .sheet(isPresented: $isShowingAddWorkspace) { | ||
| addWorkspaceSheet | ||
| } |
There was a problem hiding this comment.
Disable swipe-to-dismiss while workspace creation is running.
The buttons are disabled during creation, but the sheet can still be dismissed interactively. That lets the success/error path complete off-screen and undermines the new add-workspace failure handling.
Suggested fix
.sheet(isPresented: $isShowingAddWorkspace) {
addWorkspaceSheet
+ .interactiveDismissDisabled(latestUiState?.isCreatingWorkspace == true)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift` around lines 48 -
50, The sheet bound to isShowingAddWorkspace should be prevented from
interactive swipe-to-dismiss while workspace creation is in progress: add an
interactiveDismissDisabled(...) modifier to the sheet (on addWorkspaceSheet or
the sheet content) and bind it to the creation-running flag (create a
`@State/`@Published isCreatingWorkspace if none exists), update that flag to true
when creation starts and false when creation finishes, and only set
isShowingAddWorkspace = false after success/failure handling completes so the
sheet cannot be dismissed mid-operation.
Comment on lines
+80
to
+92
| .task(id: latestUiState?.switchedWorkspaceId ?? "") { | ||
| guard let switchedId = latestUiState?.switchedWorkspaceId, !switchedId.isEmpty else { return } | ||
| viewModel.clearWorkspaceSwitch() | ||
| onRequestAppReset() | ||
| } | ||
| .task(id: latestUiState?.createdSessionId ?? "") { | ||
| guard let sessionId = latestUiState?.createdSessionId, !sessionId.isEmpty else { return } | ||
| viewModel.clearCreatedSession() | ||
| if latestUiState?.switchedWorkspaceId != nil { | ||
| return | ||
| } | ||
| onSelectSession() | ||
| } |
There was a problem hiding this comment.
Move these one-shot side effects out of the transient sidebar view.
SidebarViewModel is app-scoped, but these .task(id:) handlers only exist while the sidebar is presented. If the user closes the sidebar before createSession or switchWorkspace completes, createdSessionId / switchedWorkspaceId stays latched in the shared view model, and the next sidebar open can immediately close itself or fire onRequestAppReset(). SwiftUIAppRootView already has an always-on collector for this uiState, so that is the safer place to consume and clear these flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift` around lines 80 -
92, The .task(id:) handlers in WorkspacesSidebarView are performing one-shot
side effects (consuming latestUiState?.switchedWorkspaceId and
latestUiState?.createdSessionId, calling viewModel.clearWorkspaceSwitch(),
viewModel.clearCreatedSession(), onRequestAppReset(), and onSelectSession())
inside a transient sidebar view; move that logic out of WorkspacesSidebarView
and into the always-on collector in SwiftUIAppRootView (which already observes
the shared SidebarViewModel/uiState), so the app-scoped SidebarViewModel flags
are consumed and cleared there to avoid stale latched behavior when the sidebar
is dismissed; update WorkspacesSidebarView to remove these .task handlers and
ensure SwiftUIAppRootView performs the checks for switchedWorkspaceId and
createdSessionId, calls viewModel.clearWorkspaceSwitch()/clearCreatedSession(),
and then invokes onRequestAppReset()/onSelectSession() as appropriate.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37b865da34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+31
to
+34
| init { | ||
| ensureInitialized() | ||
| observeWorkspaces() | ||
| observeActiveSessionId() |
There was a problem hiding this comment.
The sidebar startup path only calls ensureInitialized() and observers, but never triggers workspaceRepository.refresh(). Since ensureInitialized() seeds just the current project when local state is empty, users on a fresh install/pairing won’t see other server-known workspaces in the new sidebar unless they manually add each path, which regresses workspace switching compared with the previous workspaces flow that refreshed on first appearance.
Useful? React with 👍 / 👎.
Comment on lines
+220
to
+223
| .onFailure { error -> | ||
| OcMobileLog.w(TAG, "Failed to create session: ${error.message}") | ||
| _uiState.update { it.copy(isCreatingSession = false) } | ||
| } |
There was a problem hiding this comment.
When creating a session in a non-active workspace, the code activates that workspace first, but on createSession failure it only clears isCreatingSession and emits no switch/reset signal. Because the UI reset is keyed off switchedWorkspaceId, this leaves global active workspace changed while the chat screen remains bound to the previous session, so subsequent chat calls can run under the wrong directory context after a transient create failure.
Useful? React with 👍 / 👎.
Owner
Author
|
@codex review |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c54e0dd36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+228
to
+232
| _uiState.update { | ||
| it.copy( | ||
| isCreatingSession = false, | ||
| switchedWorkspaceId = if (!isActiveWorkspace) workspaceProjectId else null | ||
| ) |
There was a problem hiding this comment.
When createSession fails, the state update only clears isCreatingSession (and may set switchedWorkspaceId) but does not store any error that the SwiftUI sidebar can render, so users see a silent no-op whenever the API rejects session creation or networking fails. The previous sessions flow exposed these failures, and this regression makes troubleshooting and recovery much harder for normal runtime errors.
Useful? React with 👍 / 👎.
Comment on lines
+185
to
+189
| .onFailure { error -> | ||
| OcMobileLog.w(TAG, "Failed to persist session $sessionId for workspace $projectId: ${error.message}") | ||
| } | ||
| } | ||
| _uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) } |
There was a problem hiding this comment.
This path marks the workspace switch as complete even if updateCurrentSessionId fails. In this commit, SwiftUIAppRootView treats switchedWorkspaceId as a successful switch and resets the app immediately, so any persistence failure leaves the app reset into the new workspace with a stale session selection. The impact appears when settings writes fail or are interrupted, but in that case users can be routed to the wrong/invalid session context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt`:
- Around line 138-149: In the onSuccess block where you compute filtered and
call _uiState.update to set sessions and isLoading, also clear any previous
error for that workspace by setting error = null (or empty) on the matching
workspace copy; i.e., in the lambda that returns it.copy(sessions = filtered,
isLoading = false) change it to it.copy(sessions = filtered, isLoading = false,
error = null) so successful loads remove stale errors.
- Around line 181-189: The code currently sets _uiState.switchedWorkspaceId
immediately after workspaceRepository.activateWorkspace succeeds, even if the
subsequent sessionRepository.updateCurrentSessionId(sessionId) call fails;
change the flow so switchedWorkspaceId is only set when both
workspaceRepository.activateWorkspace(projectId) succeeds and, if sessionId !=
null, sessionRepository.updateCurrentSessionId(sessionId) also succeeds — i.e.,
move the _uiState.update { it.copy(isSwitchingWorkspace = false,
switchedWorkspaceId = projectId) } into the success branch of
updateCurrentSessionId (or set switchedWorkspaceId only in the onSuccess of
updateCurrentSessionId, and in the onFailure set isSwitchingWorkspace = false
without toggling switchedWorkspaceId) while keeping OcMobileLog.w(...) on update
failure.
In `@iosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift`:
- Around line 84-133: The workspace cards remain tappable during a running
workspace switch; update the UI to prevent interactions while
state.isSwitchingWorkspace is true by disabling the card actions: add a
.disabled(state.isSwitchingWorkspace) modifier to the WorkspaceCardView (or its
parent view) so taps are ignored, and also guard the action closures (the
onSelectSession and onCreateSession blocks) to early-return when
state.isSwitchingWorkspace is true before calling viewModel.switchWorkspace,
viewModel.switchSession, viewModel.createSession or invoking onSelectSession().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bac496d-6f8e-49b8-938f-25127f740234
📒 Files selected for processing (4)
composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.ktcomposeApp/src/jvmTest/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModelTest.ktiosApp/iosApp/SwiftUIInterop/SwiftUIAppRootView.swiftiosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift
Comment on lines
+138
to
+149
| .onSuccess { allSessions -> | ||
| val filtered = allSessions | ||
| .filter { it.parentId == null && it.directory == workspace.workspace.worktree } | ||
| .sortedByDescending { it.updatedAt } | ||
| _uiState.update { state -> | ||
| state.copy(workspaces = state.workspaces.map { | ||
| if (it.workspace.projectId == projectId) { | ||
| it.copy(sessions = filtered, isLoading = false) | ||
| } else it | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear stale load errors on success.
The success path updates sessions and isLoading, but it leaves error untouched. After one failed refresh, later successful loads can still render the old error.
Suggested fix
_uiState.update { state ->
state.copy(workspaces = state.workspaces.map {
if (it.workspace.projectId == projectId) {
- it.copy(sessions = filtered, isLoading = false)
+ it.copy(sessions = filtered, isLoading = false, error = null)
} else it
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt`
around lines 138 - 149, In the onSuccess block where you compute filtered and
call _uiState.update to set sessions and isLoading, also clear any previous
error for that workspace by setting error = null (or empty) on the matching
workspace copy; i.e., in the lambda that returns it.copy(sessions = filtered,
isLoading = false) change it to it.copy(sessions = filtered, isLoading = false,
error = null) so successful loads remove stale errors.
Comment on lines
+181
to
+189
| workspaceRepository.activateWorkspace(projectId) | ||
| .onSuccess { | ||
| if (sessionId != null) { | ||
| sessionRepository.updateCurrentSessionId(sessionId) | ||
| .onFailure { error -> | ||
| OcMobileLog.w(TAG, "Failed to persist session $sessionId for workspace $projectId: ${error.message}") | ||
| } | ||
| } | ||
| _uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) } |
There was a problem hiding this comment.
Don’t signal a completed workspace switch on partial failure.
updateCurrentSessionId(sessionId) is best-effort here, but switchedWorkspaceId is still published even when that write fails. SwiftUIAppRootView resets the app as soon as this flag is set, so this can reopen the new workspace with the previous session ID still stored.
Suggested fix
viewModelScope.launch {
workspaceRepository.activateWorkspace(projectId)
.onSuccess {
if (sessionId != null) {
- sessionRepository.updateCurrentSessionId(sessionId)
- .onFailure { error ->
- OcMobileLog.w(TAG, "Failed to persist session $sessionId for workspace $projectId: ${error.message}")
- }
+ val persistResult = sessionRepository.updateCurrentSessionId(sessionId)
+ if (persistResult.isFailure) {
+ OcMobileLog.w(
+ TAG,
+ "Failed to persist session $sessionId for workspace $projectId: ${persistResult.exceptionOrNull()?.message}"
+ )
+ _uiState.update { it.copy(isSwitchingWorkspace = false) }
+ return@onSuccess
+ }
}
_uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) }
}
.onFailure { error ->
OcMobileLog.w(TAG, "Failed to switch workspace: ${error.message}")📝 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
| workspaceRepository.activateWorkspace(projectId) | |
| .onSuccess { | |
| if (sessionId != null) { | |
| sessionRepository.updateCurrentSessionId(sessionId) | |
| .onFailure { error -> | |
| OcMobileLog.w(TAG, "Failed to persist session $sessionId for workspace $projectId: ${error.message}") | |
| } | |
| } | |
| _uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) } | |
| viewModelScope.launch { | |
| workspaceRepository.activateWorkspace(projectId) | |
| .onSuccess { | |
| if (sessionId != null) { | |
| val persistResult = sessionRepository.updateCurrentSessionId(sessionId) | |
| if (persistResult.isFailure) { | |
| OcMobileLog.w( | |
| TAG, | |
| "Failed to persist session $sessionId for workspace $projectId: ${persistResult.exceptionOrNull()?.message}" | |
| ) | |
| _uiState.update { it.copy(isSwitchingWorkspace = false) } | |
| return@onSuccess | |
| } | |
| } | |
| _uiState.update { it.copy(isSwitchingWorkspace = false, switchedWorkspaceId = projectId) } | |
| } | |
| .onFailure { error -> | |
| OcMobileLog.w(TAG, "Failed to switch workspace: ${error.message}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@composeApp/src/commonMain/kotlin/com/ratulsarna/ocmobile/ui/screen/sidebar/SidebarViewModel.kt`
around lines 181 - 189, The code currently sets _uiState.switchedWorkspaceId
immediately after workspaceRepository.activateWorkspace succeeds, even if the
subsequent sessionRepository.updateCurrentSessionId(sessionId) call fails;
change the flow so switchedWorkspaceId is only set when both
workspaceRepository.activateWorkspace(projectId) succeeds and, if sessionId !=
null, sessionRepository.updateCurrentSessionId(sessionId) also succeeds — i.e.,
move the _uiState.update { it.copy(isSwitchingWorkspace = false,
switchedWorkspaceId = projectId) } into the success branch of
updateCurrentSessionId (or set switchedWorkspaceId only in the onSuccess of
updateCurrentSessionId, and in the onFailure set isSwitchingWorkspace = false
without toggling switchedWorkspaceId) while keeping OcMobileLog.w(...) on update
failure.
Comment on lines
+84
to
+133
| ScrollView { | ||
| LazyVStack(spacing: 4) { | ||
| ForEach(state.workspaces, id: \.workspace.projectId) { workspaceWithSessions in | ||
| let projectId = workspaceWithSessions.workspace.projectId | ||
| let isExp = expanded.contains(projectId) | ||
| let isFull = fullyExpanded.contains(projectId) | ||
|
|
||
| WorkspaceCardView( | ||
| workspaceWithSessions: workspaceWithSessions, | ||
| activeSessionId: state.activeSessionId, | ||
| isExpanded: isExp, | ||
| isFullyExpanded: isFull, | ||
| isCreatingSession: state.isCreatingSession, | ||
| onToggleExpand: { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| if expanded.contains(projectId) { | ||
| expanded.remove(projectId) | ||
| } else { | ||
| expanded.insert(projectId) | ||
| viewModel.loadSessionsForWorkspace(projectId: projectId) | ||
| } | ||
| } | ||
| }, | ||
| onToggleFullExpand: { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| if fullyExpanded.contains(projectId) { | ||
| fullyExpanded.remove(projectId) | ||
| } else { | ||
| fullyExpanded.insert(projectId) | ||
| } | ||
| } | ||
| }, | ||
| onSelectSession: { sessionId in | ||
| let isActiveWorkspace = projectId == state.activeWorkspaceId | ||
| if isActiveWorkspace { | ||
| viewModel.switchSession(sessionId: sessionId) | ||
| onSelectSession() | ||
| } else { | ||
| viewModel.switchWorkspace(projectId: projectId, sessionId: sessionId) | ||
| } | ||
| }, | ||
| onCreateSession: { | ||
| viewModel.createSession(workspaceProjectId: projectId) | ||
| } | ||
| ) | ||
| } | ||
| } | ||
| .padding(.horizontal, 12) | ||
| .padding(.top, 8) | ||
| } |
There was a problem hiding this comment.
Disable the workspace cards while a switch is running.
state.isSwitchingWorkspace is never applied here, so the list stays tappable during switchWorkspace(). A second tap can still fire createSession() or another selection against a half-switched state.
Suggested fix
private func sidebarContent(state: SidebarUiState) -> some View {
ScrollView {
LazyVStack(spacing: 4) {
ForEach(state.workspaces, id: \.workspace.projectId) { workspaceWithSessions in
let projectId = workspaceWithSessions.workspace.projectId
let isExp = expanded.contains(projectId)
let isFull = fullyExpanded.contains(projectId)
WorkspaceCardView(
workspaceWithSessions: workspaceWithSessions,
activeSessionId: state.activeSessionId,
isExpanded: isExp,
isFullyExpanded: isFull,
isCreatingSession: state.isCreatingSession,
onToggleExpand: {
withAnimation(.easeInOut(duration: 0.25)) {
if expanded.contains(projectId) {
expanded.remove(projectId)
} else {
expanded.insert(projectId)
viewModel.loadSessionsForWorkspace(projectId: projectId)
}
}
},
onToggleFullExpand: {
withAnimation(.easeInOut(duration: 0.25)) {
if fullyExpanded.contains(projectId) {
fullyExpanded.remove(projectId)
} else {
fullyExpanded.insert(projectId)
}
}
},
onSelectSession: { sessionId in
let isActiveWorkspace = projectId == state.activeWorkspaceId
if isActiveWorkspace {
viewModel.switchSession(sessionId: sessionId)
onSelectSession()
} else {
viewModel.switchWorkspace(projectId: projectId, sessionId: sessionId)
}
},
onCreateSession: {
viewModel.createSession(workspaceProjectId: projectId)
}
)
}
}
.padding(.horizontal, 12)
.padding(.top, 8)
}
+ .disabled(state.isSwitchingWorkspace)
}📝 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
| ScrollView { | |
| LazyVStack(spacing: 4) { | |
| ForEach(state.workspaces, id: \.workspace.projectId) { workspaceWithSessions in | |
| let projectId = workspaceWithSessions.workspace.projectId | |
| let isExp = expanded.contains(projectId) | |
| let isFull = fullyExpanded.contains(projectId) | |
| WorkspaceCardView( | |
| workspaceWithSessions: workspaceWithSessions, | |
| activeSessionId: state.activeSessionId, | |
| isExpanded: isExp, | |
| isFullyExpanded: isFull, | |
| isCreatingSession: state.isCreatingSession, | |
| onToggleExpand: { | |
| withAnimation(.easeInOut(duration: 0.25)) { | |
| if expanded.contains(projectId) { | |
| expanded.remove(projectId) | |
| } else { | |
| expanded.insert(projectId) | |
| viewModel.loadSessionsForWorkspace(projectId: projectId) | |
| } | |
| } | |
| }, | |
| onToggleFullExpand: { | |
| withAnimation(.easeInOut(duration: 0.25)) { | |
| if fullyExpanded.contains(projectId) { | |
| fullyExpanded.remove(projectId) | |
| } else { | |
| fullyExpanded.insert(projectId) | |
| } | |
| } | |
| }, | |
| onSelectSession: { sessionId in | |
| let isActiveWorkspace = projectId == state.activeWorkspaceId | |
| if isActiveWorkspace { | |
| viewModel.switchSession(sessionId: sessionId) | |
| onSelectSession() | |
| } else { | |
| viewModel.switchWorkspace(projectId: projectId, sessionId: sessionId) | |
| } | |
| }, | |
| onCreateSession: { | |
| viewModel.createSession(workspaceProjectId: projectId) | |
| } | |
| ) | |
| } | |
| } | |
| .padding(.horizontal, 12) | |
| .padding(.top, 8) | |
| } | |
| ScrollView { | |
| LazyVStack(spacing: 4) { | |
| ForEach(state.workspaces, id: \.workspace.projectId) { workspaceWithSessions in | |
| let projectId = workspaceWithSessions.workspace.projectId | |
| let isExp = expanded.contains(projectId) | |
| let isFull = fullyExpanded.contains(projectId) | |
| WorkspaceCardView( | |
| workspaceWithSessions: workspaceWithSessions, | |
| activeSessionId: state.activeSessionId, | |
| isExpanded: isExp, | |
| isFullyExpanded: isFull, | |
| isCreatingSession: state.isCreatingSession, | |
| onToggleExpand: { | |
| withAnimation(.easeInOut(duration: 0.25)) { | |
| if expanded.contains(projectId) { | |
| expanded.remove(projectId) | |
| } else { | |
| expanded.insert(projectId) | |
| viewModel.loadSessionsForWorkspace(projectId: projectId) | |
| } | |
| } | |
| }, | |
| onToggleFullExpand: { | |
| withAnimation(.easeInOut(duration: 0.25)) { | |
| if fullyExpanded.contains(projectId) { | |
| fullyExpanded.remove(projectId) | |
| } else { | |
| fullyExpanded.insert(projectId) | |
| } | |
| } | |
| }, | |
| onSelectSession: { sessionId in | |
| let isActiveWorkspace = projectId == state.activeWorkspaceId | |
| if isActiveWorkspace { | |
| viewModel.switchSession(sessionId: sessionId) | |
| onSelectSession() | |
| } else { | |
| viewModel.switchWorkspace(projectId: projectId, sessionId: sessionId) | |
| } | |
| }, | |
| onCreateSession: { | |
| viewModel.createSession(workspaceProjectId: projectId) | |
| } | |
| ) | |
| } | |
| } | |
| .padding(.horizontal, 12) | |
| .padding(.top, 8) | |
| } | |
| .disabled(state.isSwitchingWorkspace) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iosApp/iosApp/SwiftUIInterop/WorkspacesSidebarView.swift` around lines 84 -
133, The workspace cards remain tappable during a running workspace switch;
update the UI to prevent interactions while state.isSwitchingWorkspace is true
by disabling the card actions: add a .disabled(state.isSwitchingWorkspace)
modifier to the WorkspaceCardView (or its parent view) so taps are ignored, and
also guard the action closures (the onSelectSession and onCreateSession blocks)
to early-return when state.isSwitchingWorkspace is true before calling
viewModel.switchWorkspace, viewModel.switchSession, viewModel.createSession or
invoking onSelectSession().
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24fa7c9f58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+127
to
+129
| if isActiveWorkspace { | ||
| viewModel.switchSession(sessionId: sessionId) | ||
| onSelectSession() |
There was a problem hiding this comment.
When a user taps a session in the active workspace, the sidebar is dismissed immediately, but the underlying switch call is fire-and-forget and only logs failures. If persisting the new session ID fails (for example due to a local settings write error), the UI closes as if the switch succeeded while chat remains on the previous session, with no error or retry path.
Useful? React with 👍 / 👎.
Comment on lines
+113
to
+117
| if (workspace.isLoading) return | ||
|
|
||
| // If sessions are already cached, show them immediately and refresh in background. | ||
| // Only show loading indicator on first fetch (no cached sessions). | ||
| val hasCachedSessions = workspace.sessions.isNotEmpty() |
There was a problem hiding this comment.
This method only marks a workspace as loading when there is no cached data, so once sessions are cached the isLoading guard no longer prevents concurrent fetches. Because refreshes can be triggered repeatedly (e.g., workspace flow re-emits or users re-expand quickly) and there is no request cancellation/version check, a slower older response can overwrite a newer one and regress the sidebar list to stale sessions.
Useful? React with 👍 / 👎.
…iewModel - Added logic to prevent overlapping session loading requests for the same workspace. - Introduced a new method `clearSwitchedSession` to reset the switched session state. - Enhanced error handling during session switching, updating the UI state accordingly. - Updated tests to verify the new behavior for session loading and switching operations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d185edec9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| } | ||
|
|
||
| loadActiveSessionTitle(sessionId = id) |
There was a problem hiding this comment.
activeSessionTitle is only fetched when currentSessionId changes, so title updates for the same session are never propagated to the toolbar. This shows up when a session starts untitled and the backend later assigns a title: the header can remain stale ("OpenCode") until the user switches sessions or resets the app, even though session metadata has changed.
Useful? React with 👍 / 👎.
| viewModel.createSession(workspaceProjectId: projectId) | ||
| } | ||
| ) | ||
| .disabled(state.isSwitchingWorkspace || state.isSwitchingSession) |
There was a problem hiding this comment.
The workspace cards stay interactive during isCreatingSession, because only switching flags are used in .disabled(...). If a user taps another session while creation is still running (slow network), the later createSession completion can persist its new session ID and override the user’s second selection, causing an unexpected context jump.
Useful? React with 👍 / 👎.
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
SidebarViewModelTEAM_IDusage in the Xcode projectTesting
./gradlew :composeApp:jvmTest./gradlew :composeApp:jvmTest --tests 'com.ratulsarna.ocmobile.ui.screen.sidebar.SidebarViewModelTest'xcodebuild -project iosApp/iosApp.xcodeproj -scheme iosApp -destination 'generic/platform=iOS Simulator' buildSummary by CodeRabbit
New Features
Removed