Conversation
|
@copilot Please make a review and make sure that everything will work as expected. |
|
@kelliherm I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to create groups with custom or auto-generated codes and introduces group naming functionality. It also refactors map initialization to prevent race conditions and adds cleanup mechanisms for empty groups.
- Added group creation feature with custom or auto-generated 4-8 character alphanumeric codes
- Introduced group naming with optional display names that default to the group code
- Refactored map initialization and added presence cleanup handlers for disconnections
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setCurrentGroup(code); | ||
| // Fetch meta name if exists | ||
| try { | ||
| const metaSnap = await database.ref(`groups/${code}/meta/name`).once('value'); | ||
| if (metaSnap.exists()) { | ||
| setCurrentGroupName(metaSnap.val()); | ||
| } else { | ||
| setCurrentGroupName(null); | ||
| } | ||
| } catch (e) { | ||
| console.warn('Could not load group name:', e); | ||
| } | ||
|
|
||
| // Add user to group members | ||
| await database.ref(`groups/${code}/members/${user.uid}`).set({ |
There was a problem hiding this comment.
Missing validation: The function doesn't verify if the group exists before joining. A user can join a non-existent group code, creating an empty group entry. Consider checking if the group exists or has at least one member before allowing a join.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
| try { | ||
| let codeInput = groupCode.trim().toUpperCase(); | ||
| const codePattern = /^[A-Z0-9]{4,8}$/; // allow 4-8 chars alphanumeric |
There was a problem hiding this comment.
Magic numbers in regex pattern. The min length (4) and max length (8) are hardcoded here and also at line 418 (maxLength="8"). Consider extracting these as named constants (e.g., MIN_GROUP_CODE_LENGTH, MAX_GROUP_CODE_LENGTH) to keep them in sync and improve maintainability.
| @@ -182,6 +257,17 @@ function App() { | |||
|
|
|||
| const code = groupCode.toUpperCase(); | |||
| setCurrentGroup(code); | |||
There was a problem hiding this comment.
State is updated before validation completes. If the subsequent database operations fail (lines 262-284), currentGroup remains set to the failed group code, potentially causing the app to render the map view for a group the user never successfully joined. Move setCurrentGroup to after successful member addition.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
| console.log('[CreateGroup] Final code selected:', finalCode); | ||
| setGroupCode(finalCode); | ||
| setCurrentGroup(finalCode); // triggers map view render |
There was a problem hiding this comment.
Similar to handleJoinGroup, state is set before database operations complete. If any of the subsequent database writes fail (lines 341-355), the user will see the map view but won't actually be a member of the group. Consider setting setCurrentGroup only after all critical database operations succeed.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
[WIP] Add ability to create a group
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@kelliherm I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@kelliherm I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@kelliherm I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.