refactor(tabgroup): manage people from group settings - #337
Merged
Conversation
Members moves out of the tab row and Settings stops being a tablet-only tab, so the set is Transactions/History/Balances on every width and the gear icon is the single way into settings. Drops perPersonBalances and rotateInvite with the Members tab: nothing else read them.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughChangesThe PR adds a dedicated group-people screen for members, placeholders, and invite links. It moves people access into group settings, removes members and settings tabs from group detail, updates tablet navigation, and adds state, ViewModel, UI, localization, and tests. Group People Management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GroupDetailPane
participant GroupSettings
participant MainGraph
participant GroupPeopleRoot
participant GroupPeopleViewModel
participant GroupRepository
GroupDetailPane->>GroupSettings: open settings
GroupSettings->>MainGraph: navigate to GroupPeople(groupId)
MainGraph->>GroupPeopleRoot: render group people destination
GroupPeopleRoot->>GroupPeopleViewModel: collect state and events
GroupPeopleViewModel->>GroupRepository: observe group and submit actions
GroupRepository-->>GroupPeopleViewModel: group updates or operation results
GroupPeopleViewModel-->>GroupPeopleRoot: people state or error event
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (5)
features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleState.kt (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider word-based initials.
name.take(2)returns the first two characters of the first word. For "Anna Beispiel" it returns "AN" instead of "AB". It can also split a surrogate pair and render a broken glyph.♻️ Word-based initials
- val initials: String get() = name.take(2).uppercase() + val initials: String + get() = + name + .trim() + .split(' ') + .filter { it.isNotBlank() } + .take(2) + .joinToString("") { it.take(1) } + .uppercase() + .ifEmpty { "?" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleState.kt` at line 25, Update the initials getter in GroupPeopleState so it derives initials from the first character of each of the first two words in name, rather than taking the first two code units. Preserve uppercase output and safely handle names with fewer than two words or Unicode surrogate pairs.features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/testing/Fixtures.kt (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
participants.first()throws for an empty participant set.If a test calls
Fixtures.group(participants = emptySet())and omitscreator, the default expression throwsNoSuchElementException. The failure message does not point at the fixture.♻️ Fall back to a default participant
- creator: GroupParticipant = participants.first(), + creator: GroupParticipant = participants.firstOrNull() ?: participant(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/testing/Fixtures.kt` at line 30, Update the default creator expression in Fixtures.group so empty participants collections do not call participants.first() and throw. Reuse or provide a default participant fallback while preserving the existing first-participant behavior when participants is non-empty.features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModelTest.kt (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the participant as a placeholder.
The test name and the comment at line 61 describe a placeholder added on the People screen. The id
"ph-1"implies a placeholder, butFixtures.participantdefaults toParticipantType.REGISTERED. The test therefore does not prove thatpeopleCountcounts placeholders.The fixture now accepts a
typeargument. Use it so the test locks in the count contract thatPeopleCardandGroupPeopleScreenshare.💚 Use the placeholder type
- val added = Fixtures.participant(id = "ph-1", name = "Tom") + val added = + Fixtures.participant( + id = "ph-1", + name = "Tom", + type = ParticipantType.PLACEHOLDER, + ) repo.emitGroups(listOf(group.copy(participants = setOf(alice, added))))Add the import for
de.tabmates.features.tabgroup.domain.models.ParticipantType.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModelTest.kt` around lines 72 - 73, Update the participant fixture in the test around GroupSettingsViewModelTest to pass ParticipantType.PLACEHOLDER as its type, and add the ParticipantType import. Keep the existing placeholder id and count assertion unchanged so the test verifies placeholder participants are included in peopleCount.features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.kt (1)
134-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe duplicate check depends on an active state subscription.
stateusesSharingStarted.WhileSubscribed(5.seconds). When no collector is active,state.valueholds the last cached value, or theGroupPeopleState()initial value with empty lists.isKnownNamethen reportsfalsefor a name that already exists.The screen is normally subscribed while the user types, so the window is narrow. Reading the group from the repository removes the hidden coupling between a validation rule and the sharing policy.
♻️ Read the participants from the repository
- private fun isKnownName(name: String): Boolean { - val existing = state.value.let { it.members + it.placeholders } - return existing.any { it.name.equals(name, ignoreCase = true) } - } + private suspend fun isKnownName(name: String): Boolean { + val group = groupRepository.getGroups().first().firstOrNull { it.id == groupId } ?: return false + return group.participants.any { it.username.equals(name, ignoreCase = true) } + }Move the call inside the existing
viewModelScope.launchinsubmitName.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.kt` around lines 134 - 137, Update submitName to read the current group participants from the repository inside its existing viewModelScope.launch, and perform the duplicate-name check against that repository data rather than the cached state. Replace or bypass isKnownName’s state.value dependency while preserving case-insensitive matching across members and placeholders.features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModel.kt (1)
53-61: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winObserve only the requested group’s participants.
getGroups()maps every group and performs an active-participant query for each group on every emission.getActiveParticipantsByGroupId(groupId)returns all active participant types, includingPLACEHOLDER, so its size matches the People screen’s members and placeholders.♻️ Observe this group’s participants
private fun observePeopleCount() { viewModelScope.launch { groupRepository - .getGroups() - .map { groups -> groups.firstOrNull { it.id == groupId }?.participants?.size ?: 0 } + .getActiveParticipantsByGroupId(groupId) + .map { participants -> participants.size } .distinctUntilChanged() .collect { count -> _state.update { it.copy(peopleCount = count) } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModel.kt` around lines 53 - 61, Update observePeopleCount in GroupSettingsViewModel to observe only the requested group by using getActiveParticipantsByGroupId(groupId) rather than collecting all groups and counting participants locally. Preserve distinctUntilChanged and update peopleCount with the returned active participant count, including PLACEHOLDER entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.kt`:
- Around line 114-117: Update submitName’s isKnownName branch to emit
GroupPeopleEvent.Error using the new group_people_error_duplicate_name string
resource instead of clearing the input; preserve the typed text so the user can
correct and resubmit it. Add the matching group_people_error_duplicate_name
translations to string.xml and values-de/string.xml.
- Around line 65-73: Update GroupPeopleViewModel’s group-loading state
construction to distinguish an initial, un-emitted flow from a completed lookup
with no matching group. Add the GroupLookup helper with hasLoaded and group
fields, track the first getGroups() emission, and set isLoading only while
hasLoaded is false; preserve the existing member, placeholder, token, and
form-state mappings.
- Around line 118-131: Update submitName and rotateInvite in
features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.kt:118-131
and :139-148 to set their re-entrancy guards before launching viewModelScope
coroutines, then reset each guard in a finally block. Keep isAdding cleared
after addNewParticipantsToGroup and isRotatingInvite cleared after rotation,
including cancellation and exception paths.
In
`@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/MainGraph.kt`:
- Around line 265-268: Update the back-stack cleanup predicate in MainGraph so
GroupSettings and GroupPeople are removed only when their groupId matches
route.groupId, while preserving the existing GroupDetail match. Apply the same
group-scoped filtering to the detail-route cleanup used when selecting another
group around the group-selection navigation logic.
---
Nitpick comments:
In
`@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleState.kt`:
- Line 25: Update the initials getter in GroupPeopleState so it derives initials
from the first character of each of the first two words in name, rather than
taking the first two code units. Preserve uppercase output and safely handle
names with fewer than two words or Unicode surrogate pairs.
In
`@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.kt`:
- Around line 134-137: Update submitName to read the current group participants
from the repository inside its existing viewModelScope.launch, and perform the
duplicate-name check against that repository data rather than the cached state.
Replace or bypass isKnownName’s state.value dependency while preserving
case-insensitive matching across members and placeholders.
In
`@features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModel.kt`:
- Around line 53-61: Update observePeopleCount in GroupSettingsViewModel to
observe only the requested group by using
getActiveParticipantsByGroupId(groupId) rather than collecting all groups and
counting participants locally. Preserve distinctUntilChanged and update
peopleCount with the returned active participant count, including PLACEHOLDER
entries.
In
`@features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModelTest.kt`:
- Around line 72-73: Update the participant fixture in the test around
GroupSettingsViewModelTest to pass ParticipantType.PLACEHOLDER as its type, and
add the ParticipantType import. Keep the existing placeholder id and count
assertion unchanged so the test verifies placeholder participants are included
in peopleCount.
In
`@features/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/testing/Fixtures.kt`:
- Line 30: Update the default creator expression in Fixtures.group so empty
participants collections do not call participants.first() and throw. Reuse or
provide a default participant fallback while preserving the existing
first-participant behavior when participants is non-empty.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 985616c9-b35a-4c92-8e27-92fc97b7f25f
📒 Files selected for processing (20)
composeApp/src/commonMain/kotlin/de/tabmates/composeapp/navigation/GroupTwoPaneSceneStrategy.ktcore/designsystem/src/commonMain/kotlin/de/tabmates/core/designsystem/textfields/TabMatesTextField.ktfeatures/tabgroup/presentation/src/commonMain/composeResources/values-de/string.xmlfeatures/tabgroup/presentation/src/commonMain/composeResources/values/string.xmlfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/components/PersonRow.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/MainGraph.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/MainNavKeys.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupdetail/GroupDetailRoot.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupdetail/GroupDetailViewModel.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupoverview/GroupDetailPane.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleScreen.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleState.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModel.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsScreen.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsState.ktfeatures/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModel.ktfeatures/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/creategroup/FakeGroupRepository.ktfeatures/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/grouppeople/GroupPeopleViewModelTest.ktfeatures/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupsettings/GroupSettingsViewModelTest.ktfeatures/tabgroup/presentation/src/commonTest/kotlin/de/tabmates/features/tabgroup/presentation/testing/Fixtures.kt
💤 Files with no reviewable changes (2)
- features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupdetail/GroupDetailRoot.kt
- features/tabgroup/presentation/src/commonMain/kotlin/de/tabmates/features/tabgroup/presentation/navigation/groupdetail/GroupDetailViewModel.kt
One row style and one screen for everyone in a group, split into Members and Placeholders sections, with the invite link card alongside. Adding a placeholder is an inline field that stays open after each name. PersonRow takes an optional trailing slot: the server has no delete for participants yet, and that slot is where it lands when it does. The route is registered but not yet reachable; group settings links it in the next commit.
Replaces the placeholder chip row with a People entry that opens the new screen, so members and placeholders are handled in one place. peopleCount is observed rather than read once: settings stays composed on the back stack while People is open, and a placeholder added there has to reach the row on the way back. Settings and People become detail panes, so opening them on a tablet keeps the group list instead of covering the window.
DennisBauer
force-pushed
the
feat/group-screens-rework
branch
from
August 5, 2026 19:48
e859b66 to
c3e60b5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This was referenced Aug 6, 2026
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 by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
fixes: #143