Groups UI take 2 #6974
Merged
Groups UI take 2 #6974
+4,117
−278
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
MitjaBezensek
temporarily deployed
to
deploy-staging
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
MitjaBezensek
had a problem deploying
to
e2e-dotcom
GitHub Actions
Failure
- Fixed file operations including pinning and dragging files between
groups.
- Changed how we handle the home group. We used `my-files` as the group
id, but we now use the real id.
- Fixed UI issues with the drag-and-drop indicator being clipped at the
top of the sidebar.
- Cleaned up the context menu to prevent the home group from appearing
twice (both as "My files" and the user's name).
### Change type
- [ ] `bugfix`
- [x] `improvement`
- [ ] `feature`
- [ ] `api`
- [ ] `other`
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> Adds shared flag helpers and consistently resolves "my-files" to the
user’s home group for pin/move/delete/drag operations, with minor admin,
backend, and UI tweaks.
>
> - **Core/shared**:
> - Add `parseFlags` and `userHasFlag` in
`packages/dotcom-shared/src/mutators.ts`; adopt across app.
> - **App logic**:
> - In `TldrawApp`, use `parseFlags` for `getUserFlags` and add
`resolveGroupId("my-files"|id)`.
> - **File actions & drag/drop**:
> - Use `resolveGroupId` for pin/unpin, delete, and pinned checks in
`TlaFileMenu`, `TlaSidebarFileLink`, `TlaSidebarRenameInline`.
> - Translate group IDs (including targets) during drag operations in
`useDragTracking` and send normalized IDs to `handleFileDragOperation`.
> - **UI/UX**:
> - Exclude home group from "Move to" group list to avoid duplicate "My
files" entry.
> - Tweak sidebar content padding to prevent drag indicator clipping.
> - **Admin/E2E/Backend**:
> - Use `userHasFlag` for groups migration checks in `admin.tsx`, e2e
`Database.ts`, and worker `acceptInvite.ts`.
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
80fe11d. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
only had patchfork in there for a tiny bit of dogfooding during the groups ui spike. i wouldn't want to inflict it on future devs. - [x] `other` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Removes the patchfork dependency and replaces all usages with direct atom.update calls, migrating sidebar state from Map/Set to plain objects/arrays across app and sidebar components. > > - **Dependencies** > - Remove `patchfork` from `apps/dotcom/client/package.json` and `yarn.lock`. > - **App/Core** > - Delete patchfork-related prototype/typing setup in `src/main.tsx`. > - Refactor `TldrawApp.sidebarState` to use plain structures: > - `expandedGroups`: `Record<string, 'closed' | 'expanded_show_less' | 'expanded_show_more'>` (was `Map`). > - `noAnimationGroups`: `string[]` (was `Set`). > - Replace all `patch(...)` calls with `sidebarState.update(...)` patterns. > - **Sidebar/UI** > - Update state mutations to direct updates in: > - `TlaFileMenu.tsx` (rename on duplicate). > - `TlaSidebarCreateFileButton.tsx` (post-create rename state). > - `TlaSidebarFileLink.tsx` (rename start/finish handling). > - `TlaSidebarGroupItem.tsx` (expand/collapse, no-animation cleanup, create file flow). > - `TlaSidebarRecentFilesNew.tsx` (show more/less toggles). > - **Misc** > - Minor formatting/cleanup in `GroupSettingsDialog.tsx`, `dialogs.module.css`, and `useHasFlag.tsx`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 6cbd337. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
* Fixes file duplication - the duplicate was always created in the home group. * Implements the import file dialog. * Implements add file link dialog. * Fixes a display issue when moving files between groups. The moved files would show as pinned for a split second. https://github.com/user-attachments/assets/fc281987-9500-4a75-a855-03b448df04ac - [x] `improvement` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds group-scoped .tldr import and an Add File Link dialog, routes duplicates/uploads to the correct group, fixes select menu open state, and introduces a backend mutator to link files to groups. > > - **Frontend (Groups & Files)**: > - Import `.tldr` into a specific group from sidebar menu; drag/drop uploads target the current file’s group. > - New "Add file link…" dialog to link an existing file to a group via URL (with validation, analytics, i18n). > - File duplication now creates the copy in the current group and updates rename state correctly. > - Fix select component to respect `isOpen` state. > - **Backend**: > - Add `addFileLinkToGroup` mutator with membership and access checks; ensure `group_file.index` defaults to `null` on inserts/moves. > - **i18n**: > - Add strings for add-link flow (labels, placeholder, error). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1f16d4f. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
had to add a drag threshold thingy to make this not annoying af ### Change type - [x] `other` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Restores sidebar group DnD with reorder support, adds a drag-start threshold, disables dragging on mobile, and updates visuals/state accordingly. > > - **Sidebar drag-and-drop** > - **Group reordering**: Implement detect/execute logic in `useDragTracking` (new `DragGroupOperation`, fractional indexing via `updateOwnGroupUser`). > - **Drag threshold**: Add `hasDragStarted` to `DragState`; begin UI feedback only after small mouse movement. > - **Mobile**: Disable dragging on coarse pointers for files and groups. > - **UI/State** > - `ReorderCursor` shows only after drag starts; recent files/home drop state gated by `hasDragStarted`. > - New `useIsDragging` hook to flag active drags; apply `data-is-dragging` to files/groups with updated styles (opacity/drop-zone). > - Update `TlaSidebarFileLink` and `TlaSidebarGroupItem` to use thresholded drag tracking and native `dragstart` payloads. > - **Shared schema** > - Add `DragGroupOperation` to `@tldraw/dotcom-shared` and wire into drag handlers. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 3459b1b. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
- Files couldn't be correctly [reordered](https://www.notion.so/tldraw/fix-pinned-file-drop-ordering-2a03e4c324c080d28e6dd80c455a08ca?source=copy_link) within pinned section - Dragging to top of the pinned section would place item in second position (wrong initial `prevBottom` calculation) - Improved the [left alignment of items](https://www.notion.so/tldraw/sidebar-section-label-alignment-2a03e4c324c080148864c7276f30ca94?source=copy_link) ### Change type - [x] `bugfix` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Fixes pinned file drag reordering (indicator/index calc) and slightly adjusts sidebar label/pin alignment. > > - **Drag & Drop / Reordering**: > - Corrects pinned-file reorder indicator positioning by adjusting `prevBottom` calculation in `useDragTracking`. > - Fixes server-side index computation in `mutators.handleFileDragOperation` by querying all group files, ignoring `null` indexes, and properly computing `nextIndex` for end/before insert. > - **UI**: > - Slightly increases "My files" label left padding in `TlaSidebarRecentFilesNew.tsx`. > - Aligns `pinIcon` by removing the negative left offset. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e52b626. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
I removed awaits since the editor recommended it, but it was fake news 😂 Looks like the editor incorrectly sees the methods as sync (`'await' has no effect on the type of this expression.`, which is why I [removed them in this PR](#7069). But it looks like we still need them, or we don't get the most up to date state which causes a [few issues when deleting files](https://www.notion.so/tldraw/Deleting-files-is-glitchy-2a13e4c324c080d480b1c8f590b8133d?source=copy_link). @ds300 maybe we should somehow fix these types? ### Change type - [x] `bugfix` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > <sup>[Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) is generating a summary for commit 6d2ee0f. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
we incorrectly made it so that client-side mutators return a promise.
they should return a `{client, server}` promise pair like the zero
client.
### Change type
- [ ] `bugfix`
- [ ] `improvement`
- [ ] `feature`
- [ ] `api`
- [x] `other`
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> Make zero polyfill mutators return a {client, server} promise pair and
update app/components to await .client where needed.
>
> - **Zero polyfill**:
> - Change mutator wrapper to return `{ client, server }` instead of a
single promise, queuing client work and resolving `server` when
optimistic updates flush.
> - Add `promiseWithResolve` and `react` to resolve the server promise
when no optimistic updates remain.
> - **App/Components**:
> - Update calls to await `.client` on mutations (e.g.,
`removeFileFromGroup`, `addFileLinkToGroup`, `createGroup`,
`moveFileToGroup`, `leaveGroup`, `deleteGroup`, `setGroupMemberRole`,
`updateOwnGroupUser`).
> - Keep `.server` where appropriate (e.g.,
`regenerateGroupInviteSecret`).
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
c8070a5. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
* When dragging a file to a group it now expands the group if it was collapsed. * Fix inputs (groups settings, create group,...) and some other css issues. We prefixed our editor class names with `tl-` and this PR was opened before that. Also found some unused styles. ### Change type - [x] `other` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Expands a collapsed group when a file is dragged into it, updates dialog CSS to tl-* tokens, and removes unused sidebar/invite styles. > > - **Behavior (drag & drop)** > - Expands target sidebar group after a file move (`useDragTracking.ts`: `ensureSidebarGroupExpanded(...)`). > - **Styles** > - Align dialog inputs and invite controls to `--tl-radius-*` and `--tl-space-*` tokens (`components/dialogs/dialogs.module.css`). > - Remove unused sidebar section styles (`TlaSidebar/sidebar.module.css`: `sidebarFileSectionRecent`, `sidebarFileSectionGroups`, `sidebarFileSectionDivider`, etc.). > - **Cleanup** > - Delete obsolete `pages/invite-dialog.module.css`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 821718c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
MitjaBezensek
force-pushed
the
david/groups-ui-2
branch
from
2476f40 to
7654dd9
Compare
MitjaBezensek
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
e2e-dotcom
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
deploy-staging
GitHub Actions
Inactive
MitjaBezensek
force-pushed
the
david/groups-ui-2
branch
from
7654dd9 to
22f3064
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // If file is pinned, nothing to do | ||
| if (this.getFileState(fileId)?.isPinned) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Bug: Migrated users: Pinning status misidentified.
The ensureFileVisibleInSidebar function checks if a file is pinned using getFileState(fileId)?.isPinned, which only works for non-migrated users. For migrated users with the groups_backend flag, pinning is stored in group_file.index (not null means pinned), so this check always returns false for pinned files in migrated accounts, causing the function to incorrectly attempt to expand groups for already-visible pinned files. The function should use this.isPinned(fileId, file.owningGroupId) instead to handle both migrated and non-migrated users correctly.
| const MAX_FILES_TO_SHOW = 4 | ||
| const fileIndex = groupFiles.findIndex((f) => f?.fileId === fileId) | ||
|
|
||
| if (fileIndex >= MAX_FILES_TO_SHOW) { |
There was a problem hiding this comment.
Bug: Inconsistent file limits disrupt UI.
The MAX_FILES_TO_SHOW constant is hardcoded as 4, but the actual UI component in TlaSidebarGroupItem.tsx calculates it as numPinnedFiles + 4. This mismatch causes incorrect detection of whether a file is in the "show more" section, potentially failing to expand groups when needed or expanding them unnecessarily. The calculation should match the UI component's logic to account for pinned files.
| () => { | ||
| const groupFiles = app.getGroupFilesSorted(groupId) | ||
| const pinned = groupFiles.filter((f) => !!app.getFileState(f.fileId)?.isPinned) | ||
| const unpinned = groupFiles.filter((f) => !app.getFileState(f.fileId)?.isPinned) |
There was a problem hiding this comment.
Bug: Migrated Users' Pinned Files Appear Unpinned
The code filters files using app.getFileState(f.fileId)?.isPinned, which doesn't work for migrated users with the groups_backend flag. For these users, pinning is stored in group_file.index (not null means pinned), and getGroupFilesSorted already returns files with the correct isPinned property. The code should use f.isPinned from the returned objects instead of calling getFileState, which will always return false/undefined for migrated users' pinned files, causing all files to be incorrectly classified as unpinned.
|
|
||
| if (!group) return null | ||
|
|
||
| const numPinnedFiles = files.filter((f) => !!app.getFileState(f.fileId)?.isPinned).length |
There was a problem hiding this comment.
Bug: Migrated Users: Pinned File Count Error
The numPinnedFiles calculation uses app.getFileState(f.fileId)?.isPinned, which doesn't work for migrated users with the groups_backend flag. For these users, pinning is stored in group_file.index, and the files array already has the correct isPinned property from getGroupFilesSorted. The code should use f.isPinned instead, otherwise it will always count zero pinned files for migrated users, causing incorrect MAX_FILES_TO_SHOW calculations.
| } | ||
|
|
||
| async admin_migrateToGroups(userId: string, inviteSecret: string | null = null) { | ||
| console.error('admin_migrateToGroups', userId, inviteSecret) |
There was a problem hiding this comment.
Bug: Debug Code Leaks Via Error Logs
A console.error statement appears to be debugging code that was accidentally left in. This logs migration parameters at the error level when it should either use console.log for informational logging or be removed entirely.
| cancelAnimationFrame(animationFrame) | ||
| window.removeEventListener('drag', handleMouseMove) | ||
| window.removeEventListener('dragend', onDone) | ||
| window.removeEventListener('pointerup', onDone) |
There was a problem hiding this comment.
Bug: Keypress prematurely cancels drag.
The drag operation cancels on any keypress because the keydown event listener calls onCancel without checking which key was pressed. This causes unexpected behavior when users press any key during a drag operation (spacebar, arrow keys, etc.). The listener should only cancel on Escape key by checking e.key === 'Escape' in the handler.
| return mutators | ||
| } | ||
|
|
||
| export interface DragReorderOperation { |
There was a problem hiding this comment.
Bug: File Permission Bypass Vulnerability
The handleFileDragOperation mutator lacks authorization checks (noted by the TODO comment at line 795). Users can potentially move or reorder files in groups they don't have access to, or manipulate file links between groups without proper permissions. The mutator should verify the user is a member of both source and target groups before allowing file operations, similar to the moveFileToGroup mutator's permission checks.
MitjaBezensek
temporarily deployed
to
e2e-dotcom
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
deploy-staging
GitHub Actions
Inactive
MitjaBezensek
force-pushed
the
david/groups-ui-2
branch
from
83ea2b0 to
67b22c8
Compare
MitjaBezensek
temporarily deployed
to
e2e-dotcom
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
|
|
||
| const numPinnedFiles = files.filter((f) => !!app.getFileState(f.fileId)?.isPinned).length | ||
|
|
||
| const MAX_FILES_TO_SHOW = numPinnedFiles + 4 |
There was a problem hiding this comment.
Bug: Broken Sidebar File Visibility Logic
The MAX_FILES_TO_SHOW calculation as numPinnedFiles + 4 doesn't match the hardcoded value of 4 used in TldrawApp.ensureFileVisibleInSidebar. This inconsistency causes ensureFileVisibleInSidebar to incorrectly determine which files are in the "show more" section when pinned files exist, potentially expanding groups unnecessarily or failing to expand when needed.
| pinned_files_migrated: number | ||
| flag_added: boolean | ||
| }>`SELECT * FROM migrate_user_to_groups(${userId}, ${inviteSecret})`.execute(this.db) | ||
| console.error('admin_migrateToGroups result', result.rows) |
There was a problem hiding this comment.
Bug: Accidental console.error Debugging in Production
The console.error statement appears to be debugging code that was accidentally committed. It logs migration results which should either use console.log for intentional logging or be removed entirely.
ds300
added
the
other
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe what your pull request does. If you can, add GIFs or images showing the before and after of your change.
Change type
bugfiximprovementfeatureapiotherTest plan
Release notes
Note
Introduce groups-based sidebar UI with file/group DnD, group management and invites, plus server/client mutations and routes to support group workflows.
CreateGroupDialog,GroupSettingsDialog(invite link, member roles, leave/delete),AddFileLinkDialog.GET /invite/:token(client + worker) and accept flowPOST /invite/:token/accept.TlaInviteDialogand handling on/invite/:tokenand in file/local pages; copy invite links from sidebar/menu.TldrawAppenhancements: group memberships, stable ordering, ensure-visible helpers, upload/import to specific group, new drag state, toasts, navigation, andhasFlagutilities.useHasFileAdminRights).MAX_NUMBER_OF_GROUPS,ZErrorCode.max_groups_reached, flag helpers (parseFlags,userHasFlag).icon-folder,-open,-new) and extensive locale strings for groups/invites.Written by Cursor Bugbot for commit 67b22c8. This will update automatically on new commits. Configure here.