From cccb42a683e54acb3d62538e62193123a857906e Mon Sep 17 00:00:00 2001 From: Patrick O'Sullivan Date: Wed, 8 Jan 2025 16:25:48 -0600 Subject: [PATCH 1/5] refine manage channels screen --- packages/app/hooks/useGroupContext.ts | 41 +- packages/shared/src/db/queries.ts | 32 +- packages/shared/src/store/groupActions.ts | 193 +++-- .../ui/src/components/ChannelNavSection.tsx | 15 +- .../components/GroupChannelsScreenView.tsx | 2 +- .../ManageChannelsScreenView.tsx | 776 ++++++++++-------- 6 files changed, 658 insertions(+), 401 deletions(-) diff --git a/packages/app/hooks/useGroupContext.ts b/packages/app/hooks/useGroupContext.ts index ee47a4ce48..220309e623 100644 --- a/packages/app/hooks/useGroupContext.ts +++ b/packages/app/hooks/useGroupContext.ts @@ -56,9 +56,20 @@ export const useGroupContext = ({ const groupNavSectionsWithChannels = useMemo(() => { return groupNavSections.map((section) => ({ ...section, - channels: groupChannels.filter((channel) => - section.channels.map((c) => c.channelId).includes(channel.id) - ), + channels: groupChannels + .filter((channel) => + section.channels.map((c) => c.channelId).includes(channel.id) + ) + .sort((a, b) => { + const aIndex = + section.channels.find((c) => c.channelId === a.id)?.channelIndex ?? + 0; + const bIndex = + section.channels.find((c) => c.channelId === b.id)?.channelIndex ?? + 0; + + return aIndex - bIndex; + }), })); }, [groupNavSections, groupChannels]); @@ -176,13 +187,25 @@ export const useGroupContext = ({ const moveChannelToNavSection = useCallback( async (channelId: string, navSectionId: string) => { - if (group) { - await store.addChannelToNavSection({ - group, - channelId, - navSectionId, - }); + if (!group) return; + + // Find current section for the channel + const currentSection = group.navSections?.find((section) => + section.channels?.some((channel) => channel.channelId === channelId) + ); + + if (!currentSection) { + console.error('Channel not found in any section'); + return; } + + // Use addChannelToNavSection which handles both adding to new section + // and removing from the previous section + await store.addChannelToNavSection({ + group, + channelId, + navSectionId, + }); }, [group] ); diff --git a/packages/shared/src/db/queries.ts b/packages/shared/src/db/queries.ts index 8e904f091e..b37d6c6388 100644 --- a/packages/shared/src/db/queries.ts +++ b/packages/shared/src/db/queries.ts @@ -673,7 +673,7 @@ export const updateGroup = createWriteQuery( async (group: Partial & { id: string }, ctx: QueryCtx) => { return ctx.db.update($groups).set(group).where(eq($groups.id, group.id)); }, - ['groups'] + ['groups', 'channels', 'groupNavSections', 'groupNavSectionChannels'] ); export const deleteGroup = createWriteQuery( @@ -1607,6 +1607,33 @@ export const addNavSectionToGroup = createWriteQuery( ['groupNavSections'] ); +export const updateNavSectionChannel = createWriteQuery( + 'updateNavSectionChannel', + async ( + { + channelId, + groupNavSectionId, + channelIndex, + }: { + channelId: string; + groupNavSectionId: string; + channelIndex: number; + }, + ctx: QueryCtx + ) => { + return ctx.db + .update($groupNavSectionChannels) + .set({ channelIndex }) + .where( + and( + eq($groupNavSectionChannels.channelId, channelId), + eq($groupNavSectionChannels.groupNavSectionId, groupNavSectionId) + ) + ); + }, + ['groupNavSectionChannels'] +); + export const updateNavSection = createWriteQuery( 'updateNavSection', async ( @@ -1656,7 +1683,7 @@ export const addChannelToNavSection = createWriteQuery( }) .onConflictDoNothing(); }, - ['groupNavSectionChannels'] + ['groupNavSectionChannels', 'groups'] ); export const deleteChannelFromNavSection = createWriteQuery( @@ -2765,6 +2792,7 @@ export const getGroup = createReadQuery( 'channels', 'groupJoinRequests', 'groupMemberBans', + 'groupNavSectionChannels' ] ); diff --git a/packages/shared/src/store/groupActions.ts b/packages/shared/src/store/groupActions.ts index 71286bf889..ffcc35e69e 100644 --- a/packages/shared/src/store/groupActions.ts +++ b/packages/shared/src/store/groupActions.ts @@ -6,7 +6,7 @@ import { getRandomId } from '../logic'; import { createSectionId } from '../urbit'; import { createChannel } from './channelActions'; -const logger = createDevLogger('groupActions', true); +const logger = createDevLogger('groupActions', false); interface CreateGroupParams { title?: string; @@ -257,10 +257,9 @@ export async function updateGroupMeta(group: db.Group) { } catch (e) { console.error('Failed to update group', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); + if (existingGroup) { + await db.updateGroup(existingGroup); + } } } @@ -317,12 +316,10 @@ export async function addNavSection( } catch (e) { console.error('Failed to add nav section', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); - - await db.deleteNavSection(groupNavSectionId); + if (existingGroup) { + await db.updateGroup(existingGroup); + await db.deleteNavSection(groupNavSectionId); + } } } @@ -350,10 +347,9 @@ export async function updateNavSectionMeta( } catch (e) { console.error('Failed to update nav section', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); + if (existingGroup) { + await db.updateGroup(existingGroup); + } } } @@ -411,7 +407,7 @@ export async function moveNavSection( // optimistic update await db.updateGroup({ - ...group, + id: group.id, navSections: newNavSections, }); @@ -431,17 +427,15 @@ export async function moveNavSection( } catch (e) { console.error('Failed to move nav section', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); - - navSections.forEach(async (section, index) => { - await db.updateNavSection({ - ...section, - sectionIndex: index, + if (existingGroup) { + await db.updateGroup(existingGroup); + navSections.forEach(async (section, index) => { + await db.updateNavSection({ + ...section, + sectionIndex: index, + }); }); - }); + } } } @@ -463,6 +457,11 @@ export async function addChannelToNavSection({ const existingGroup = await db.getGroup({ id: group.id }); + if (!existingGroup) { + logger.error('Group not found', group.id); + return; + } + const navSections = group.navSections ?? []; const navSection = navSections.find( (section) => section.sectionId === navSectionId @@ -484,7 +483,7 @@ export async function addChannelToNavSection({ ...(section.channels ?? []), { channelId, - index: (section.channels?.length ?? 0) + 1, + index: section.channels?.length ?? 0, }, ], }; @@ -498,12 +497,7 @@ export async function addChannelToNavSection({ undefined ); - await db.addChannelToNavSection({ - channelId, - groupNavSectionId: navSectionId, - index: (navSection?.channels?.length ?? 0) + 1, - }); - + // First remove from previous section if it exists if (previousNavSection) { await db.deleteChannelFromNavSection({ channelId, @@ -511,6 +505,14 @@ export async function addChannelToNavSection({ }); } + // Then add to new section + await db.addChannelToNavSection({ + channelId, + groupNavSectionId: navSectionId, + // The %groups agent only supports adding new channels to the start of a section. + index: 0, + }); + try { await api.addChannelToNavSection({ groupId: group.id, @@ -521,17 +523,23 @@ export async function addChannelToNavSection({ } catch (e) { logger.log('failed to add channel to nav section', e); console.error('Failed to add channel', e); - // rollback optimistic update + + // rollback optimistic update - first remove from new section await db.deleteChannelFromNavSection({ channelId, groupNavSectionId: navSectionId, }); + // then add back to previous section if it existed if (previousNavSection) { + const prevIndex = + previousNavSection.channels?.findIndex( + (c) => c.channelId === channelId + ) ?? 0; await db.addChannelToNavSection({ channelId, groupNavSectionId: previousNavSection.sectionId, - index: (previousNavSection.channels?.length ?? 0) + 1, + index: prevIndex, }); } } @@ -547,10 +555,14 @@ export async function moveChannel({ channelId: string; navSectionId: string; index: number; -}) { +}): Promise { logger.log('moving channel', group.id, channelId, navSectionId, index); const existingGroup = await db.getGroup({ id: group.id }); + if (!existingGroup) { + logger.error('Group not found'); + return; + } const navSections = group.navSections ?? []; const navSection = navSections.find( @@ -558,35 +570,91 @@ export async function moveChannel({ ); if (!navSection && navSectionId !== 'default') { - console.error('Nav section not found', navSectionId); + logger.error('Nav section not found'); + return; + } + + // Verify channel exists in this section + const sectionChannels = navSection?.channels ?? []; + const channelIndex = + sectionChannels.find((c) => c.channelId === channelId)?.channelIndex ?? -1; + + if (channelIndex === -1) { + logger.error('Channel not found in this section'); + return; + } + + // Validate index bounds + if (index < 0 || index > sectionChannels.length) { + logger.error('Invalid index'); return; } const newNavSections = navSections.map((section) => { if (section.sectionId !== navSectionId) { + logger.log('section', section.sectionId, 'not updated'); return section; } - const newChannels = - section.channels?.filter((channel) => channel.channelId !== channelId) ?? - []; - const [channel] = - section.channels?.filter((channel) => channel.channelId === channelId) ?? - []; - newChannels.splice(index, 0, channel); + logger.log( + 'section', + section.sectionId, + 'updated', + 'channels', + section.channels + ); + + const channels = [ + ...(section.channels?.sort((a, b) => { + const aIndex = a.channelIndex ?? 0; + const bIndex = b.channelIndex ?? 0; + return aIndex - bIndex; + }) ?? []), + ]; + const [channel] = channels.splice(channelIndex, 1); + channels.splice(index, 0, channel); + + // Update indices + const updatedChannels = channels.map((c, idx) => ({ + ...c, + channelIndex: idx, + })); return { ...section, - channels: newChannels, + channels: updatedChannels, }; }); + logger.log('newNavSections', newNavSections); + // optimistic update await db.updateGroup({ - ...group, + id: group.id, navSections: newNavSections, }); + // Update the channel indices in the groupNavSectionChannels table + const updatedSection = newNavSections.find( + (section) => section.sectionId === navSectionId + ); + + if (updatedSection && updatedSection.channels) { + for (const channel of updatedSection.channels) { + if (!channel.channelId) continue; + logger.log( + 'updating channel index', + channel.channelId, + channel.channelIndex + ); + await db.updateNavSectionChannel({ + channelId: channel.channelId, + groupNavSectionId: updatedSection.id, + channelIndex: channel.channelIndex ?? 0, + }); + } + } + try { await api.moveChannel({ groupId: group.id, @@ -595,12 +663,21 @@ export async function moveChannel({ index, }); } catch (e) { - console.error('Failed to move channel', e); + logger.error('Failed to move channel', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); + if (existingGroup) { + await db.updateGroup(existingGroup); + + // Rollback channel indices + for (const channel of sectionChannels) { + if (!channel.channelId) continue; + await db.updateNavSectionChannel({ + channelId: channel.channelId, + groupNavSectionId: navSectionId, + channelIndex: channel.channelIndex ?? 0, + }); + } + } } } @@ -647,14 +724,12 @@ export async function updateNavSection({ } catch (e) { console.error('Failed to update nav section', e); // rollback optimistic update - await db.updateGroup({ - id: group.id, - ...existingGroup, - }); - - await db.updateNavSection({ - ...existingNavSection, - }); + if (existingGroup) { + await db.updateGroup(existingGroup); + await db.updateNavSection({ + ...existingNavSection, + }); + } } } diff --git a/packages/ui/src/components/ChannelNavSection.tsx b/packages/ui/src/components/ChannelNavSection.tsx index ddd036086a..7ad90a09cf 100644 --- a/packages/ui/src/components/ChannelNavSection.tsx +++ b/packages/ui/src/components/ChannelNavSection.tsx @@ -18,9 +18,18 @@ export default function ChannelNavSection({ const sectionChannels = useMemo( () => section.channels - ? section.channels.filter( - (item) => !!channels.find((c) => c.id === item.channelId) - ) + ? section.channels + .filter((item) => !!channels.find((c) => c.id === item.channelId)) + .sort((a, b) => { + const aChannelIndex = + section.channels?.find((c) => c.channelId === a.channelId) + ?.channelIndex ?? 0; + const bChannelIndex = + section.channels?.find((c) => c.channelId === b.channelId) + ?.channelIndex ?? 0; + + return aChannelIndex - bChannelIndex; + }) : [], [section, channels] ); diff --git a/packages/ui/src/components/GroupChannelsScreenView.tsx b/packages/ui/src/components/GroupChannelsScreenView.tsx index fdccd80af5..9df763f296 100644 --- a/packages/ui/src/components/GroupChannelsScreenView.tsx +++ b/packages/ui/src/components/GroupChannelsScreenView.tsx @@ -1,5 +1,5 @@ import * as db from '@tloncorp/shared/db'; -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useState } from 'react'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { ScrollView, View, YStack, getVariableValue, useTheme } from 'tamagui'; diff --git a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx index 7c85fbc82c..edfda74a4d 100644 --- a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx +++ b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx @@ -1,14 +1,13 @@ import * as db from '@tloncorp/shared/db'; import { omit } from 'lodash'; -import { useCallback, useEffect, useMemo, useState } from 'react'; -import { LayoutRectangle } from 'react-native'; -import Animated, { useSharedValue } from 'react-native-reanimated'; +import { useCallback, useMemo, useState } from 'react'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; -import { Text, View, XStack, YStack } from 'tamagui'; +import { ScrollView, Text, View, XStack, YStack } from 'tamagui'; +import { capitalize } from '../../utils'; import { Button } from '../Button'; -import { DraggableItem } from '../DraggableItem'; import { Icon } from '../Icon'; +import { ListItem } from '../ListItem'; import Pressable from '../Pressable'; import { ScreenHeader } from '../ScreenHeader'; import { CreateChannelSheet } from './CreateChannelSheet'; @@ -22,91 +21,138 @@ export type Section = { type Channel = { id: string; + type: db.ChannelType; title?: string | null; + index?: number; }; -type DraggedItem = { - type: 'section' | 'channel'; - channelId?: string; - sectionId: string; - layout: LayoutRectangle; -}; - -function EmptySection({ - deleteSection, - isDefault, -}: { - deleteSection: () => Promise; - isDefault: boolean; -}) { +function EmptySection() { return ( - - - No channels + + + No channels in this section - {!isDefault && ( - - )} ); } -function DraggableChannel({ +function ChannelItem({ channel, onEdit, - onDrag, - onDragStart, - onDragEnd, - isDragging = false, + moveChannelWithinNavSection, + moveChannelToNavSection, + index, + sectionIndex, + isLast, + isFirst, + isLastSection, + isFirstSection, }: { channel: Channel; onEdit: () => void; - onDrag: (translateY: number) => void; - onDragStart: (layout: LayoutRectangle) => void; - onDragEnd: (translateY: number) => void; - isDragging?: boolean; + moveChannelWithinNavSection: (newIndex: number) => void; + moveChannelToNavSection: (newSectionIndex: number) => void; + index: number; + sectionIndex: number; + isLast: boolean; + isFirst: boolean; + isLastSection: boolean; + isFirstSection: boolean; }) { + const handleMoveUp = useCallback(() => { + if (isFirst) { + if (isFirstSection) { + console.error( + `Channel "${channel.title}" is already at the top of the first section` + ); + return; + } else { + moveChannelToNavSection(sectionIndex - 1); + } + return; + } + moveChannelWithinNavSection(index - 1); + }, [ + moveChannelWithinNavSection, + index, + sectionIndex, + isFirst, + isFirstSection, + moveChannelToNavSection, + channel.title, + ]); + + const handleMoveDown = useCallback(() => { + if (isLast) { + if (isLastSection) { + console.error( + `Channel "${channel.title}" is already at the bottom of the last section` + ); + return; + } else { + moveChannelToNavSection(sectionIndex + 1); + } + return; + } + moveChannelWithinNavSection(index + 1); + }, [ + moveChannelWithinNavSection, + index, + sectionIndex, + isLast, + isLastSection, + moveChannelToNavSection, + channel.title, + ]); + return ( - - - {!isDragging && ( - <> - - console.log('edit')}> - - - - {channel?.title} - - - - - - - - - )} + + + + + {channel?.title} + + + {capitalize(channel.type)} + + + + + + + + + + + + + + + + - + ); } @@ -117,6 +163,11 @@ function NavSection({ onMoveUp, onMoveDown, editSection, + deleteSection, + setShowCreateChannel, + setShowAddSection, + isEmpty, + isDefault, children, }: { section: Section; @@ -125,45 +176,87 @@ function NavSection({ onMoveUp: (sectionId: string) => void; onMoveDown: (sectionId: string) => void; editSection: (section: Section) => void; + deleteSection: (sectionId: string) => void; + setShowCreateChannel: (show: boolean) => void; + setShowAddSection: (show: boolean) => void; + isEmpty: boolean; + isDefault: boolean; children: React.ReactNode; }) { - console.log('section, index', section.title, index); + const borderTopWidth = useMemo(() => (index === 0 ? 'unset' : 1), [index]); + const borderColor = useMemo( + () => (index === 0 ? 'transparent' : '$border'), + [index] + ); + const paddingTop = useMemo(() => (index === 0 ? '$l' : '$xl'), [index]); + return ( - - - {section.title} + + + {section.title} + + + {!isDefault ? ( + <> + + {isEmpty && ( + + )} + + ) : ( + <> + + + + )} - + {/* TODO: Implement these actions after we get design feedback + {index !== 0 && ( onMoveUp(section.id)}> - + )} {index !== totalSections - 1 && ( onMoveDown(section.id)}> - - - )} - {section.id !== 'default' && ( - editSection(section)}> - + )} + */} {children} @@ -209,161 +302,21 @@ export function ManageChannelsScreenView({ enableCustomChannels = false, }: ManageChannelsScreenViewProps) { const [sections, setSections] = useState(() => { - console.log('componentDidMount', groupNavSectionsWithChannels); return groupNavSectionsWithChannels.map((s) => ({ id: s.sectionId, title: s.title ?? 'Untitled Section', channels: s.channels.map((c) => ({ id: c.id, title: c.title ?? 'Untitled Channel', + type: c.type, })), })); }); - useEffect(() => { - console.log('componentDidUpdate', groupNavSectionsWithChannels); - setSections( - groupNavSectionsWithChannels.map((s) => ({ - id: s.sectionId, - title: s.title ?? 'Untitled Section', - channels: s.channels.map((c) => ({ - id: c.id, - title: c.title ?? 'Untitled Channel', - })), - })) - ); - }, [groupNavSectionsWithChannels]); - const [editSection, setEditSection] = useState
(null); const [showAddSection, setShowAddSection] = useState(false); const [showCreateChannel, setShowCreateChannel] = useState(false); - const [draggedItem, setDraggedItem] = useState(null); - const draggedItemY = useSharedValue(0); - - // const handleSectionDragEnd = useCallback( - // async (index: number, translateY: number) => { - // const newIndex = Math.min( - // Math.max(0, Math.round(index + translateY / 40)), - // sections.length - 1 - // ); - - // setSections((prevSections) => { - // const newSections = [...prevSections]; - // const [movedSection] = newSections.splice(index, 1); - // newSections.splice(newIndex, 0, movedSection); - - // return newSections; - // }); - // setDraggedItem(null); - // draggedItemY.value = 0; - // await moveNavSection(sections[index].id, newIndex); - // }, - // [draggedItemY, moveNavSection, sections] - // ); - - const handleChannelDragEnd = useCallback( - async (sectionId: string, channelIndex: number, translateY: number) => { - let newSections: Section[] = []; - let newChannelIndex; - let targetSectionIndex; - setSections((prevSections) => { - newSections = [...prevSections]; - const currentSectionIndex = newSections.findIndex( - (s) => s.id === sectionId - ); - const currentSection = newSections[currentSectionIndex]; - const [movedChannel] = currentSection.channels.splice(channelIndex, 1); - - // Calculate the target section index - targetSectionIndex = currentSectionIndex; - const sectionHeight = 150; // Approximate height of a section - const direction = Math.sign(translateY); - const sectionsMoved = Math.floor(Math.abs(translateY) / sectionHeight); - - if (direction > 0) { - // Moving down - targetSectionIndex = Math.min( - currentSectionIndex + sectionsMoved, - newSections.length - 1 - ); - } else if (direction < 0) { - // Moving up - targetSectionIndex = Math.max(currentSectionIndex - sectionsMoved, 0); - } - - // Calculate new index within the target section - const targetSection = newSections[targetSectionIndex]; - const channelHeight = 72; // Approximate height of a channel - if (targetSectionIndex === currentSectionIndex) { - newChannelIndex = Math.min( - Math.max(0, Math.round(channelIndex + translateY / channelHeight)), - targetSection.channels.length - ); - } else { - newChannelIndex = direction > 0 ? 0 : targetSection.channels.length; - } - - // Insert the channel at its new position - targetSection.channels.splice(newChannelIndex, 0, movedChannel); - - return newSections; - }); - - setDraggedItem(null); - draggedItemY.value = 0; - - if ( - newChannelIndex !== undefined && - targetSectionIndex !== undefined && - newSections.length > 0 - ) { - if (sections[targetSectionIndex].id !== sectionId) { - await moveChannelToNavSection( - newSections[targetSectionIndex].channels[newChannelIndex].id, - newSections[targetSectionIndex].id - ); - } else { - await moveChannelWithinNavSection( - newSections[targetSectionIndex].channels[newChannelIndex].id, - newSections[targetSectionIndex].id, - newChannelIndex - ); - } - } - }, - [ - draggedItemY, - moveChannelWithinNavSection, - sections, - moveChannelToNavSection, - ] - ); - - const handleDragStart = useCallback( - ( - type: 'section' | 'channel', - layout: LayoutRectangle, - sectionId: string, - channelId?: string - ) => { - draggedItemY.value = layout.y; - setDraggedItem({ type, sectionId, channelId, layout }); - }, - [draggedItemY] - ); - - const handleDrag = useCallback( - (translateY: number) => { - if (!draggedItem) { - return; - } - - draggedItemY.value = translateY + draggedItem.layout.y; - }, - [draggedItemY, draggedItem] - ); - const handleUpdateSection = useCallback( async (sectionId: string, title: string) => { const navSection = groupNavSectionsWithChannels.find( @@ -371,19 +324,30 @@ export function ManageChannelsScreenView({ ); if (!navSection) { + console.error(`Section not found: ${sectionId} (for update operation)`); return; } - await updateNavSection({ - ...omit(navSection, 'channels'), - title, - }); + // Store original state for rollback + const originalSections = [...sections]; + // Update local state first for optimistic UI setSections((prevSections) => prevSections.map((s) => (s.id === sectionId ? { ...s, title } : s)) ); + + try { + await updateNavSection({ + ...omit(navSection, 'channels'), + title, + }); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to update section:', e); + } }, - [groupNavSectionsWithChannels, updateNavSection] + [groupNavSectionsWithChannels, updateNavSection, sections] ); const handleDeleteSection = useCallback( @@ -393,129 +357,295 @@ export function ManageChannelsScreenView({ ); if (!navSection) { + console.error(`Section not found: ${sectionId} (for deletion)`); return; } - await deleteNavSection(navSection.id); + + // Store original state for rollback + const originalSections = [...sections]; + + // Update local state first for optimistic UI + setSections((prevSections) => + prevSections.filter((s) => s.id !== sectionId) + ); + + try { + await deleteNavSection(navSection.id); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to delete section:', e); + } }, - [deleteNavSection, groupNavSectionsWithChannels] + [deleteNavSection, groupNavSectionsWithChannels, sections] ); const handleMoveSectionUp = useCallback( async (sectionId: string) => { - console.log('move up', sectionId); const index = sections.findIndex((s) => s.id === sectionId); if (index === 0) { + console.error( + `Section "${sections[index].title}" is already at the top` + ); return; } + + // Store original state for rollback + const originalSections = [...sections]; + + // Update local state first for optimistic UI setSections((prevSections) => { const newSections = [...prevSections]; const [movedSection] = newSections.splice(index, 1); newSections.splice(index - 1, 0, movedSection); - return newSections; }); - await moveNavSection(sectionId, index - 1); + + try { + await moveNavSection(sectionId, index - 1); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to move section up:', e); + } }, [sections, moveNavSection] ); const handleMoveSectionDown = useCallback( async (sectionId: string) => { - console.log('move down', sectionId); const index = sections.findIndex((s) => s.id === sectionId); if (index === sections.length - 1) { + console.error( + `Section "${sections[index].title}" is already at the bottom` + ); return; } + + // Store original state for rollback + const originalSections = [...sections]; + + // Update local state first for optimistic UI setSections((prevSections) => { const newSections = [...prevSections]; const [movedSection] = newSections.splice(index, 1); newSections.splice(index + 1, 0, movedSection); - return newSections; }); - await moveNavSection(sectionId, index + 1); + + try { + await moveNavSection(sectionId, index + 1); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to move section down:', e); + } }, [sections, moveNavSection] ); + const handleMoveChannelWithinNavSection = useCallback( + async (channelId: string, sectionId: string, newIndex: number) => { + const sectionIndex = sections.findIndex((s) => s.id === sectionId); + if (sectionIndex === -1) { + console.error('Invalid section ID:', sectionId); + return; + } + + const channelIndex = sections[sectionIndex].channels.findIndex( + (c) => c.id === channelId + ); + if (channelIndex === -1) { + console.error( + `Channel not found: ${channelId} (expected in section ${sectionId})` + ); + return; + } + + // Validate newIndex is within bounds + if (newIndex < 0 || newIndex >= sections[sectionIndex].channels.length) { + console.error( + 'Invalid new index:', + newIndex, + 'for section with', + sections[sectionIndex].channels.length, + 'channels' + ); + return; + } + + // Store the original state for rollback + const originalSections = [...sections]; + + // Update local state first for optimistic UI + setSections((prevSections) => { + const newSections = [...prevSections]; + const newChannels = [...newSections[sectionIndex].channels]; + const [movedChannel] = newChannels.splice(channelIndex, 1); + newChannels.splice(newIndex, 0, movedChannel); + newSections[sectionIndex] = { + ...newSections[sectionIndex], + channels: newChannels, + }; + return newSections; + }); + + try { + await moveChannelWithinNavSection(channelId, sectionId, newIndex); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to move channel within section:', e); + } + }, + [sections, moveChannelWithinNavSection] + ); + + const handleMoveChannelToNavSection = useCallback( + async ( + channelId: string, + newSectionId: string, + previousSectionId: string + ) => { + const previousSectionIndex = sections.findIndex( + (s) => s.id === previousSectionId + ); + + if (previousSectionIndex === -1) { + console.error('Invalid previous section ID:', previousSectionId); + return; + } + + // Prevent moving to the same section + if (newSectionId === previousSectionId) { + console.error( + `Cannot move channel to section "${sections[previousSectionIndex].title}" as it is already there` + ); + return; + } + + const channel = sections[previousSectionIndex].channels.find( + (c) => c.id === channelId + ); + + if (!channel) { + console.error( + `Channel not found: ${channelId} (expected in section ${previousSectionId})` + ); + return; + } + + const sectionIndex = sections.findIndex((s) => s.id === newSectionId); + if (sectionIndex === -1) { + console.error('Invalid new section ID:', newSectionId); + return; + } + + // Store the original state for rollback + const originalSections = [...sections]; + + // Update local state first for optimistic UI + setSections((prevSections) => { + const newSections = [...prevSections]; + const newChannels = [...newSections[sectionIndex].channels]; + + // The %groups agent only supports adding new channels to the start of a section. + newChannels.unshift({ + id: channelId, + title: channel.title, + index: 0, + type: channel.type, + }); + + newSections[sectionIndex] = { + ...newSections[sectionIndex], + channels: newChannels, + }; + + // Remove from the previous section + const newPreviousChannels = newSections[ + previousSectionIndex + ].channels.filter((c) => c.id !== channelId); + + newSections[previousSectionIndex] = { + ...newSections[previousSectionIndex], + channels: newPreviousChannels, + }; + + return newSections; + }); + + try { + await moveChannelToNavSection(channelId, newSectionId); + } catch (e) { + // Restore original state on error + setSections(originalSections); + console.error('Failed to move channel to new section:', e); + } + }, + [sections, moveChannelToNavSection] + ); + const { bottom } = useSafeAreaInsets(); const renderSectionsAndChannels = useMemo(() => { - return sections.map((section, index) => ( + return sections.map((section, sectionIndex) => ( - {section.channels.length === 0 && ( - handleDeleteSection(section.id)} - /> - )} - {section.channels.map((channel, index) => ( - } + {section.channels.map((channel, channelIndex) => ( + { + handleMoveChannelWithinNavSection( + channel.id, + section.id, + newIndex + ); + }} + moveChannelToNavSection={(newSectionIndex) => { + handleMoveChannelToNavSection( + channel.id, + sections[newSectionIndex].id, + section.id + ); + }} + index={channelIndex} + sectionIndex={sectionIndex} + isLast={channelIndex === section.channels.length - 1} + isFirst={channelIndex === 0} + isFirstSection={sectionIndex === 0} + isLastSection={sectionIndex === sections.length - 1} onEdit={() => goToEditChannel(channel.id)} - onDrag={handleDrag} - onDragStart={(layout) => - handleDragStart('channel', layout, section.id, channel.id) - } - onDragEnd={(translateY) => - handleChannelDragEnd(section.id, index, translateY) - } - isDragging={ - draggedItem?.type === 'channel' && - draggedItem.channelId === channel.id - } /> ))} )); }, [ sections, - draggedItem, - handleChannelDragEnd, - handleDragStart, - handleDrag, handleDeleteSection, goToEditChannel, handleMoveSectionDown, handleMoveSectionUp, + handleMoveChannelWithinNavSection, + handleMoveChannelToNavSection, ]); return ( - {draggedItem && ( - - s.id === draggedItem.sectionId)! - .channels.find((c) => c.id === draggedItem.channelId)! - } - onEdit={() => {}} - onDrag={() => {}} - onDragStart={() => {}} - onDragEnd={() => {}} - /> - - )} - - - {renderSectionsAndChannels} - - - - - - + {renderSectionsAndChannels} + {showCreateChannel && group && ( @@ -567,11 +682,18 @@ export function ManageChannelsScreenView({ open={showAddSection} mode="add" onOpenChange={(open) => setShowAddSection(open)} - onSave={async (title) => - createNavSection({ - title, - }) - } + onSave={async (title) => { + try { + await createNavSection({ + title, + }); + } catch (e) { + console.error('Failed to create section:', e); + // Note: We don't need state rollback here since the section + // hasn't been added to local state yet - the UI will update + // via the useEffect when groupNavSectionsWithChannels changes + } + }} /> Date: Thu, 9 Jan 2025 06:30:06 -0600 Subject: [PATCH 2/5] Add useEffect back in, necessary for handling new sections. Update the query deps for addNavSectionToGroup --- packages/shared/src/db/queries.ts | 2 +- .../ManageChannelsScreenView.tsx | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/db/queries.ts b/packages/shared/src/db/queries.ts index b37d6c6388..a54ee90ac1 100644 --- a/packages/shared/src/db/queries.ts +++ b/packages/shared/src/db/queries.ts @@ -1604,7 +1604,7 @@ export const addNavSectionToGroup = createWriteQuery( set: conflictUpdateSetAll($groupNavSections), }); }, - ['groupNavSections'] + ['groups','groupNavSections', 'groupNavSectionChannels'] ); export const updateNavSectionChannel = createWriteQuery( diff --git a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx index edfda74a4d..3698aaca8d 100644 --- a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx +++ b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx @@ -1,6 +1,6 @@ import * as db from '@tloncorp/shared/db'; import { omit } from 'lodash'; -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { ScrollView, Text, View, XStack, YStack } from 'tamagui'; @@ -22,7 +22,7 @@ export type Section = { type Channel = { id: string; type: db.ChannelType; - title?: string | null; + title: string; index?: number; }; @@ -317,6 +317,25 @@ export function ManageChannelsScreenView({ const [showAddSection, setShowAddSection] = useState(false); const [showCreateChannel, setShowCreateChannel] = useState(false); + useEffect(() => { + const newNavSections = groupNavSectionsWithChannels.map((s) => ({ + id: s.sectionId, + title: s.title ?? 'Untitled Section', + channels: s.channels.map((c) => ({ + id: c.id, + title: c.title ?? 'Untitled Channel', + type: c.type, + })), + })); + + // Only update if the total number of sections have changed + if (newNavSections.length === sections.length) { + return; + } + + setSections(newNavSections); + }, [groupNavSectionsWithChannels, sections]); + const handleUpdateSection = useCallback( async (sectionId: string, title: string) => { const navSection = groupNavSectionsWithChannels.find( From 21e088f14f165bab5a9aa55b279f45da8a2cfc5e Mon Sep 17 00:00:00 2001 From: Patrick O'Sullivan Date: Thu, 9 Jan 2025 09:25:14 -0600 Subject: [PATCH 3/5] More groupActions/groupsApi hardening --- packages/app/hooks/useGroupContext.ts | 4 +- packages/shared/src/api/groupsApi.ts | 4 ++ packages/shared/src/store/groupActions.ts | 63 ++++++++++------------- packages/shared/src/store/sync.ts | 13 +++-- 4 files changed, 41 insertions(+), 43 deletions(-) diff --git a/packages/app/hooks/useGroupContext.ts b/packages/app/hooks/useGroupContext.ts index 220309e623..934800bd50 100644 --- a/packages/app/hooks/useGroupContext.ts +++ b/packages/app/hooks/useGroupContext.ts @@ -175,7 +175,7 @@ export const useGroupContext = ({ async (channelId: string, navSectionId: string, index: number) => { if (group) { await store.moveChannel({ - group, + groupId: group.id, channelId, navSectionId, index, @@ -202,7 +202,7 @@ export const useGroupContext = ({ // Use addChannelToNavSection which handles both adding to new section // and removing from the previous section await store.addChannelToNavSection({ - group, + groupId: group.id, channelId, navSectionId, }); diff --git a/packages/shared/src/api/groupsApi.ts b/packages/shared/src/api/groupsApi.ts index 0503716cd7..ec7bc4c330 100644 --- a/packages/shared/src/api/groupsApi.ts +++ b/packages/shared/src/api/groupsApi.ts @@ -743,6 +743,7 @@ export type GroupChannelDelete = { export type GroupChannelNavSectionAdd = { type: 'addChannelToNavSection'; channelId: string; + groupId: string; navSectionId: string; sectionId: string; }; @@ -778,6 +779,7 @@ export type GroupnavSectionMoveChannel = { type: 'moveChannel'; navSectionId: string; sectionId: string; + groupId: string; channelId: string; index: number; }; @@ -1205,6 +1207,7 @@ export const toGroupUpdate = ( type: 'moveChannel', navSectionId, sectionId, + groupId, channelId: zoneDelta['mov-nest'].nest, index: zoneDelta['mov-nest'].idx, }; @@ -1262,6 +1265,7 @@ export const toGroupUpdate = ( return { type: 'addChannelToNavSection', channelId, + groupId, navSectionId: `${groupId}-${zoneId}`, sectionId: zoneId, }; diff --git a/packages/shared/src/store/groupActions.ts b/packages/shared/src/store/groupActions.ts index ffcc35e69e..bca2ca500f 100644 --- a/packages/shared/src/store/groupActions.ts +++ b/packages/shared/src/store/groupActions.ts @@ -5,6 +5,7 @@ import { createDevLogger } from '../debug'; import { getRandomId } from '../logic'; import { createSectionId } from '../urbit'; import { createChannel } from './channelActions'; +import isEqual from 'lodash/isEqual'; const logger = createDevLogger('groupActions', false); @@ -440,29 +441,29 @@ export async function moveNavSection( } export async function addChannelToNavSection({ - group, + groupId, channelId, navSectionId, }: { - group: db.Group; + groupId: string; channelId: string; navSectionId: string; }) { logger.log( 'adding channel to nav section', - group.id, + groupId, channelId, navSectionId ); - const existingGroup = await db.getGroup({ id: group.id }); + const existingGroup = await db.getGroup({ id: groupId }); if (!existingGroup) { - logger.error('Group not found', group.id); + logger.error('Group not found', groupId); return; } - const navSections = group.navSections ?? []; + const navSections = existingGroup.navSections ?? []; const navSection = navSections.find( (section) => section.sectionId === navSectionId ); @@ -472,33 +473,20 @@ export async function addChannelToNavSection({ return; } - const newNavSections = navSections.map((section) => { - if (section.sectionId !== navSectionId) { - return section; - } - - return { - ...section, - channels: [ - ...(section.channels ?? []), - { - channelId, - index: section.channels?.length ?? 0, - }, - ], - }; - }); - - logger.log('newNavSections', newNavSections); - const previousNavSection = navSections.find( (section) => section.channels?.find((channel) => channel.channelId === channelId) !== undefined ); - // First remove from previous section if it exists if (previousNavSection) { + // First make sure this channel isn't already in the section + if (previousNavSection.sectionId === navSectionId) { + logger.log('Channel already in section', channelId, navSectionId); + return; + } + + // Then remove from previous section if it exists await db.deleteChannelFromNavSection({ channelId, groupNavSectionId: previousNavSection.id, @@ -508,14 +496,14 @@ export async function addChannelToNavSection({ // Then add to new section await db.addChannelToNavSection({ channelId, - groupNavSectionId: navSectionId, + groupNavSectionId: `${groupId}-${navSectionId}`, // The %groups agent only supports adding new channels to the start of a section. index: 0, }); try { await api.addChannelToNavSection({ - groupId: group.id, + groupId: groupId, channelId, navSectionId, }); @@ -546,25 +534,25 @@ export async function addChannelToNavSection({ } export async function moveChannel({ - group, + groupId, channelId, navSectionId, index, }: { - group: db.Group; + groupId: string; channelId: string; navSectionId: string; index: number; }): Promise { - logger.log('moving channel', group.id, channelId, navSectionId, index); + logger.log('moving channel', groupId, channelId, navSectionId, index); - const existingGroup = await db.getGroup({ id: group.id }); + const existingGroup = await db.getGroup({ id: groupId }); if (!existingGroup) { logger.error('Group not found'); return; } - const navSections = group.navSections ?? []; + const navSections = existingGroup.navSections ?? []; const navSection = navSections.find( (section) => section.sectionId === navSectionId ); @@ -626,11 +614,14 @@ export async function moveChannel({ }; }); - logger.log('newNavSections', newNavSections); + if (isEqual(newNavSections, navSections)) { + logger.log('No change in channel order'); + return; + } // optimistic update await db.updateGroup({ - id: group.id, + id: groupId, navSections: newNavSections, }); @@ -657,7 +648,7 @@ export async function moveChannel({ try { await api.moveChannel({ - groupId: group.id, + groupId: groupId, channelId, navSectionId, index, diff --git a/packages/shared/src/store/sync.ts b/packages/shared/src/store/sync.ts index 5e69346a50..9491f47d62 100644 --- a/packages/shared/src/store/sync.ts +++ b/packages/shared/src/store/sync.ts @@ -13,6 +13,7 @@ import { INFINITE_ACTIVITY_QUERY_KEY, resetActivityFetchers, } from '../store/useActivityFetchers'; +import { addChannelToNavSection, moveChannel } from './groupActions'; import { verifyUserInviteLink } from './inviteActions'; import { useLureState } from './lure'; import { getSyncing, updateIsSyncing, updateSession } from './session'; @@ -620,18 +621,20 @@ async function handleGroupUpdate(update: api.GroupUpdate) { break; case 'moveChannel': logger.log('moving channel', update); - await db.addChannelToNavSection({ + await moveChannel({ channelId: update.channelId, - groupNavSectionId: update.navSectionId, + groupId: update.groupId, + navSectionId: update.sectionId, index: update.index, }); break; case 'addChannelToNavSection': logger.log('adding channel to nav section', update); - await db.addChannelToNavSection({ + + await addChannelToNavSection({ channelId: update.channelId, - groupNavSectionId: update.navSectionId, - index: 0, + groupId: update.groupId, + navSectionId: update.sectionId, }); break; case 'setUnjoinedGroups': From bfc7c7be7269f36815ac776f4b4ee22cc67cf579 Mon Sep 17 00:00:00 2001 From: Patrick O'Sullivan Date: Thu, 9 Jan 2025 09:30:41 -0600 Subject: [PATCH 4/5] Fix bottom padding issue in ManageChannelsScreenView --- .../src/components/ManageChannels/ManageChannelsScreenView.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx index 3698aaca8d..50f21d3422 100644 --- a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx +++ b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx @@ -674,16 +674,15 @@ export function ManageChannelsScreenView({ {renderSectionsAndChannels} From 7c90d1555db9fe0bbf81fbfd2e09f5eca0e9495b Mon Sep 17 00:00:00 2001 From: Patrick O'Sullivan Date: Thu, 9 Jan 2025 13:48:07 -0600 Subject: [PATCH 5/5] Replace console.error calls with logger.error --- .../ManageChannelsScreenView.tsx | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx index 50f21d3422..f351a18be7 100644 --- a/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx +++ b/packages/ui/src/components/ManageChannels/ManageChannelsScreenView.tsx @@ -1,3 +1,4 @@ +import { createDevLogger } from '@tloncorp/shared'; import * as db from '@tloncorp/shared/db'; import { omit } from 'lodash'; import { useCallback, useEffect, useMemo, useState } from 'react'; @@ -13,6 +14,8 @@ import { ScreenHeader } from '../ScreenHeader'; import { CreateChannelSheet } from './CreateChannelSheet'; import { EditSectionNameSheet } from './EditSectionNameSheet'; +const logger = createDevLogger('ManageChannelsScreenView', false); + export type Section = { id: string; title: string; @@ -62,7 +65,7 @@ function ChannelItem({ const handleMoveUp = useCallback(() => { if (isFirst) { if (isFirstSection) { - console.error( + logger.error( `Channel "${channel.title}" is already at the top of the first section` ); return; @@ -85,7 +88,7 @@ function ChannelItem({ const handleMoveDown = useCallback(() => { if (isLast) { if (isLastSection) { - console.error( + logger.error( `Channel "${channel.title}" is already at the bottom of the last section` ); return; @@ -343,7 +346,7 @@ export function ManageChannelsScreenView({ ); if (!navSection) { - console.error(`Section not found: ${sectionId} (for update operation)`); + logger.error(`Section not found: ${sectionId} (for update operation)`); return; } @@ -363,7 +366,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to update section:', e); + logger.error('Failed to update section:', e); } }, [groupNavSectionsWithChannels, updateNavSection, sections] @@ -376,7 +379,7 @@ export function ManageChannelsScreenView({ ); if (!navSection) { - console.error(`Section not found: ${sectionId} (for deletion)`); + logger.error(`Section not found: ${sectionId} (for deletion)`); return; } @@ -393,7 +396,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to delete section:', e); + logger.error('Failed to delete section:', e); } }, [deleteNavSection, groupNavSectionsWithChannels, sections] @@ -403,7 +406,7 @@ export function ManageChannelsScreenView({ async (sectionId: string) => { const index = sections.findIndex((s) => s.id === sectionId); if (index === 0) { - console.error( + logger.error( `Section "${sections[index].title}" is already at the top` ); return; @@ -425,7 +428,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to move section up:', e); + logger.error('Failed to move section up:', e); } }, [sections, moveNavSection] @@ -435,7 +438,7 @@ export function ManageChannelsScreenView({ async (sectionId: string) => { const index = sections.findIndex((s) => s.id === sectionId); if (index === sections.length - 1) { - console.error( + logger.error( `Section "${sections[index].title}" is already at the bottom` ); return; @@ -457,7 +460,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to move section down:', e); + logger.error('Failed to move section down:', e); } }, [sections, moveNavSection] @@ -467,7 +470,7 @@ export function ManageChannelsScreenView({ async (channelId: string, sectionId: string, newIndex: number) => { const sectionIndex = sections.findIndex((s) => s.id === sectionId); if (sectionIndex === -1) { - console.error('Invalid section ID:', sectionId); + logger.error('Invalid section ID:', sectionId); return; } @@ -475,7 +478,7 @@ export function ManageChannelsScreenView({ (c) => c.id === channelId ); if (channelIndex === -1) { - console.error( + logger.error( `Channel not found: ${channelId} (expected in section ${sectionId})` ); return; @@ -483,7 +486,7 @@ export function ManageChannelsScreenView({ // Validate newIndex is within bounds if (newIndex < 0 || newIndex >= sections[sectionIndex].channels.length) { - console.error( + logger.error( 'Invalid new index:', newIndex, 'for section with', @@ -514,7 +517,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to move channel within section:', e); + logger.error('Failed to move channel within section:', e); } }, [sections, moveChannelWithinNavSection] @@ -531,13 +534,13 @@ export function ManageChannelsScreenView({ ); if (previousSectionIndex === -1) { - console.error('Invalid previous section ID:', previousSectionId); + logger.error('Invalid previous section ID:', previousSectionId); return; } // Prevent moving to the same section if (newSectionId === previousSectionId) { - console.error( + logger.error( `Cannot move channel to section "${sections[previousSectionIndex].title}" as it is already there` ); return; @@ -548,7 +551,7 @@ export function ManageChannelsScreenView({ ); if (!channel) { - console.error( + logger.error( `Channel not found: ${channelId} (expected in section ${previousSectionId})` ); return; @@ -556,7 +559,7 @@ export function ManageChannelsScreenView({ const sectionIndex = sections.findIndex((s) => s.id === newSectionId); if (sectionIndex === -1) { - console.error('Invalid new section ID:', newSectionId); + logger.error('Invalid new section ID:', newSectionId); return; } @@ -599,7 +602,7 @@ export function ManageChannelsScreenView({ } catch (e) { // Restore original state on error setSections(originalSections); - console.error('Failed to move channel to new section:', e); + logger.error('Failed to move channel to new section:', e); } }, [sections, moveChannelToNavSection] @@ -706,7 +709,7 @@ export function ManageChannelsScreenView({ title, }); } catch (e) { - console.error('Failed to create section:', e); + logger.error('Failed to create section:', e); // Note: We don't need state rollback here since the section // hasn't been added to local state yet - the UI will update // via the useEffect when groupNavSectionsWithChannels changes