-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: update Servers history and Servers List layout #6733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update Servers history and Servers List layout #6733
Conversation
WalkthroughThis PR replaces long-press deletion with swipe-to-delete for server items, adds swipeable components and animated delete actions, updates ServerItem and ServersHistory UI, persists iconURL in server history with schema/migration, updates sagas to populate iconURL, revises E2E tests for swipe flows, and adds i18n keys across locales. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Gesture as Pan Gesture Handler
participant SwipeUI as SwipeableDeleteTouchable
participant Anim as Reanimated
participant Action as DeleteAction
participant App as App Callback
User->>Gesture: Swipe item (left/right per locale)
Gesture->>Anim: update transX (shared value)
Anim->>SwipeUI: animate item position
Anim->>Action: reveal delete button (parallax/threshold)
alt transX crosses longSwipe threshold
Anim->>App: runOnJS -> onDeletePress (auto)
App->>SwipeUI: remove item
else user taps revealed delete button
User->>Action: tap delete
Action->>App: onDeletePress
App->>SwipeUI: remove item
end
SwipeUI->>Anim: spring transX to closed state (if not deleted)
SwipeUI->>User: UI updated (deleted or closed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas to focus:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/views/NewServerView/components/ServersHistoryItem/index.tsx (1)
44-46: Consider explicit handling for empty username.While React safely renders falsy values as empty, explicitly handling the empty case could improve clarity and allow for a placeholder if desired.
Optional refactor:
<Text numberOfLines={1} style={[styles.subtitle, { color: colors.fontSecondaryInfo }]}> - {item.username} + {item.username || ''} </Text>app/containers/ServerItem/SwipeableDeleteItem/Actions.tsx (1)
29-29: Consider extracting SERVER_ITEM_PADDING_VERTICAL to shared constants.The
SERVER_ITEM_PADDING_VERTICALconstant (value: 12) appears specific to server item layout. If this value is used elsewhere or needs to stay synchronized with padding defined instyles.ts, consider extracting it to a shared constants file to avoid duplication.#!/bin/bash # Check if this constant is duplicated or should be shared rg -n "SERVER_ITEM_PADDING|padding.*12" app/containers/ServerItem/ --type=ts --type=tsx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
app/containers/ServerItem/__snapshots__/ServerItem.test.tsx.snapis excluded by!**/*.snapapp/views/NewServerView/components/ServersHistoryItem/__snapshots__/ServersHistoryItem.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (30)
app/containers/ServerItem/SwipeableDeleteItem/Actions.tsx(1 hunks)app/containers/ServerItem/SwipeableDeleteItem/Touchable.tsx(1 hunks)app/containers/ServerItem/Touchable.tsx(1 hunks)app/containers/ServerItem/index.tsx(3 hunks)app/i18n/locales/ar.json(2 hunks)app/i18n/locales/bn-IN.json(2 hunks)app/i18n/locales/cs.json(1 hunks)app/i18n/locales/de.json(1 hunks)app/i18n/locales/en.json(2 hunks)app/i18n/locales/es.json(2 hunks)app/i18n/locales/fi.json(1 hunks)app/i18n/locales/fr.json(1 hunks)app/i18n/locales/hi-IN.json(2 hunks)app/i18n/locales/hu.json(1 hunks)app/i18n/locales/it.json(2 hunks)app/i18n/locales/ja.json(2 hunks)app/i18n/locales/nl.json(2 hunks)app/i18n/locales/nn.json(2 hunks)app/i18n/locales/no.json(2 hunks)app/i18n/locales/pt-BR.json(1 hunks)app/i18n/locales/pt-PT.json(2 hunks)app/i18n/locales/ru.json(2 hunks)app/i18n/locales/sl-SI.json(1 hunks)app/i18n/locales/sv.json(1 hunks)app/i18n/locales/ta-IN.json(2 hunks)app/i18n/locales/te-IN.json(2 hunks)app/i18n/locales/tr.json(2 hunks)app/i18n/locales/zh-CN.json(1 hunks)app/i18n/locales/zh-TW.json(1 hunks)app/views/NewServerView/components/ServersHistoryItem/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- app/i18n/locales/fr.json
- app/i18n/locales/hu.json
- app/i18n/locales/cs.json
- app/i18n/locales/de.json
- app/i18n/locales/ja.json
- app/i18n/locales/ru.json
- app/containers/ServerItem/Touchable.tsx
- app/i18n/locales/ta-IN.json
- app/i18n/locales/hi-IN.json
- app/i18n/locales/no.json
- app/i18n/locales/fi.json
- app/i18n/locales/te-IN.json
- app/i18n/locales/bn-IN.json
- app/i18n/locales/sv.json
- app/i18n/locales/tr.json
- app/i18n/locales/en.json
- app/i18n/locales/pt-PT.json
- app/i18n/locales/it.json
- app/i18n/locales/zh-CN.json
- app/i18n/locales/es.json
🧰 Additional context used
🧬 Code graph analysis (4)
app/containers/ServerItem/SwipeableDeleteItem/Actions.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
colors(280-302)app/containers/CustomIcon/index.tsx (1)
CustomIcon(38-38)
app/containers/ServerItem/index.tsx (2)
app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)
app/containers/ServerItem/SwipeableDeleteItem/Touchable.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/containers/ServerItem/SwipeableDeleteItem/Actions.tsx (1)
DeleteAction(31-126)app/lib/constants/colors.ts (1)
colors(280-302)
app/views/NewServerView/components/ServersHistoryItem/index.tsx (4)
app/definitions/IServerHistory.ts (1)
TServerHistoryModel(11-11)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)app/lib/constants/colors.ts (1)
colors(280-302)
🔇 Additional comments (16)
app/i18n/locales/ar.json (1)
15-16: ✓ Translations properly placed and accurately rendered.The three new keys are correctly positioned in alphabetical order and use natural Arabic terminology:
- "Activate_to_select_server" and its compound accessibility variant align with the new swipe-to-delete server item gestures
- "Workspaces" follows standard terminology
JSON structure is valid. Translations are appropriately scoped for the server/workspace management UI updates described in the PR objectives.
Also applies to: 609-609
app/i18n/locales/nn.json (1)
14-14: Localization additions are well-structured and alphabetically ordered.The new keys are placed correctly in alphabetical sequence, translations are grammatically sound Norwegian Nynorsk, and the additions align with the PR objective to enhance Servers History/List UI. The three new entries support the swipe-to-delete interaction and workspace selection features.
Also applies to: 394-394
app/views/NewServerView/components/ServersHistoryItem/index.tsx (4)
22-25: Good use of responsive hook.The
useResponsiveLayout()hook at line 24 provides reactive width updates, addressing the previous review feedback about making the component responsive.
26-28: Excellent accessibility implementation.The conditional accessibility label and localized hint provide good screen reader support across locales.
1-53: Well-structured component with strong fundamentals.The component demonstrates good practices:
- Performance optimization via
React.memo- Comprehensive accessibility (label, hint)
- Theme-aware styling with dynamic colors
- Responsive layout via
useResponsiveLayout- Clean TypeScript interface with public exports
- Proper image handling with fallback
38-38: Database schema properly includes iconURL support.Verification confirms that
iconURLis correctly defined throughout the stack:
- Database Schema (
app/lib/database/schema/servers.js):icon_urlcolumn defined as optional string- Migration (
app/lib/database/model/servers/migrations.js, version 17): Addsicon_urlcolumn toservers_historytable- WatermelonDB Model (
app/lib/database/model/ServersHistory.js, line 15):@field('icon_url') iconURLproperly maps the database column- TypeScript Interface (
app/definitions/IServerHistory.ts, line 8):iconURL?: stringmatches the schemaThe code at line 38 safely uses
item.iconURLwith a fallback todefaultLogo, and the optional property definition allows for backward compatibility.app/i18n/locales/pt-BR.json (1)
21-22: LGTM! New accessibility strings are clear and appropriate.The two new translation keys provide clear accessibility labels for server selection and the delete action, properly localized for pt-BR.
app/i18n/locales/nl.json (2)
15-16: LGTM! Dutch accessibility strings are clear and consistent.The two new translation keys match the structure and intent of the pt-BR translations, properly localized for Dutch.
749-749: Workspace terminology in nl.json is correctly and consistently applied.All workspace-related translations in the file use "werkruimte" (singular) where appropriate, and line 749 correctly uses "Werkruimtes" as the plural form for the "Workspaces" key. There are no inconsistencies or terminology conflicts.
app/containers/ServerItem/SwipeableDeleteItem/Touchable.tsx (2)
151-178: LGTM! Pan gesture configuration with RTL-aware directional constraints.The gesture handling properly accounts for RTL layouts:
- RTL: Allows positive translations (right swipes), blocks negative
- LTR: Allows negative translations (left swipes), blocks positive
The
activeOffsetXandfailOffsetYconfiguration appropriately distinguishes horizontal swipes from vertical scrolling.
85-149: State machine transitions are correctly implemented with proper RTL/LTR symmetry and edge case handling.The
handleReleasestate machine has been verified for all edge cases:
State 0 → 1 (open delete): The
longSwipethreshold correctly triggers immediate delete in both RTL (translationX >= longSwipe) and LTR (translationX <= -longSwipe), withhandleDeletePress()resetting state immediately.State 1 → 0 (close): RTL and LTR thresholds are symmetric—RTL closes when
valueRef.current < smallSwipewhile LTR closes whenvalueRef.current > -smallSwipe, both properly balanced around zero.Long swipe in state 1: No conflicts exist. When triggered from State 0,
handleDeletePress()callsclose()(lines 70-75) which resetsrowStateto 0 andvalueRef.currentto 0, preventing the code from remaining in State 1 with accumulated values that could re-trigger delete logic.The gesture handler's
onUpdatemethod (lines 154-175) also includes directional constraints that prevent swipes opposite to the intended direction, reinforcing the RTL/LTR correctness.app/containers/ServerItem/index.tsx (2)
32-32: LGTM! Responsive width calculation properly implemented.Using
useResponsiveLayout()ensures the width recalculates on orientation changes and tablet window resizes, addressing the past review concern about static width values.
34-38: LGTM! Accessibility labels and hints are well-implemented.The accessibility strings properly combine server name and ID, with conditional hints that clearly communicate available actions to screen reader users.
app/containers/ServerItem/SwipeableDeleteItem/Actions.tsx (3)
42-57: LGTM! Well-implemented haptic feedback on threshold crossing.The
useAnimatedReactioncorrectly detects when the swipe crosses thelongSwipethreshold in both directions and triggers haptic feedback with a smooth spring animation. The RTL/LTR branching properly handles directional logic.
59-98: LGTM! Parallax effect adds visual polish.The animated styles implement a nice parallax effect for long swipes (beyond
2 * actionWidth), with proper RTL/LTR positioning. The interpolation creates smooth visual feedback as the user continues swiping.
114-121: LGTM! Delete button is properly accessible.The
RectButtonincludes an appropriate accessibility label usingI18n.t('Delete'), making the action discoverable to screen reader users.
| }, | ||
| { | ||
| "width": 750, | ||
| "width": undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regressions on snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/RoomsListView/components/ServersList.tsx (1)
114-121: Confusing and potentially incorrect deletion logic.The expression
item.id === server || remove(item.id)misuses the||operator for conditional execution. While it may technically prevent deletion of the current server via short-circuit evaluation, the logic is non-idiomatic and unclear. Additionally, the past review comment suggests this approach "doesn't work properly."Apply this diff to use explicit conditional logic:
- const renderItem = ({ item }: { item: { id: string; iconURL: string; name: string; version: string } }) => ( - <ServerItem - item={item} - onPress={() => select(item.id, item.version)} - onDeletePress={() => item.id === server || remove(item.id)} - hasCheck={item.id === server} - /> - ); + const renderItem = ({ item }: { item: { id: string; iconURL: string; name: string; version: string } }) => ( + <ServerItem + item={item} + onPress={() => select(item.id, item.version)} + onDeletePress={() => { + if (item.id !== server) { + remove(item.id); + } + }} + hasCheck={item.id === server} + /> + );Alternatively, if
ServerItemsupportsundefinedforonDeletePressto disable deletion:- onDeletePress={() => item.id === server || remove(item.id)} + onDeletePress={item.id !== server ? () => remove(item.id) : undefined}
♻️ Duplicate comments (1)
app/i18n/locales/sl-SI.json (1)
19-20: Align Slovenian grammar across related accessibility keys (unresolved from prior review).The previous review flagged a grammatical inconsistency between lines 19 and 20. Line 19 uses the accusative form "izbiro" while line 20 uses the nominative form "izbor," creating inconsistency. The English source repeats the same phrase in both keys, and other locales (e.g., Spanish) maintain consistent phrasing. Update line 20's value to match line 19's grammatical form:
- "Activate_to_select_server_Available_actions_delete": "Aktiviraj za izbor strežnika. Na voljo dejanja: izbriši", + "Activate_to_select_server_Available_actions_delete": "Aktiviraj za izbiro strežnika. Na voljo dejanja: izbriši",
🧹 Nitpick comments (2)
app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.stories.tsx (2)
17-23: Use a fixedupdatedAtvalue to keep stories/tests deterministicUsing
new Date()on Line 21 makes the story time-dependent; ifupdatedAtis rendered, snapshots or visual comparisons may change over time. Prefer a fixed example timestamp instead.- updatedAt: new Date(), + updatedAt: new Date('2024-01-01T00:00:00.000Z'),
25-33: Consider adding variants that exercise condensed and large-font layouts
responsiveLayoutProviderValuecurrently always usesBASE_ROW_HEIGHT,BASE_ROW_HEIGHT_CONDENSED, andfontScale: 1(Lines 25–32), so all stories (Lines 61–88) render only the default layout path. You could add additional stories that overrideisLargeFontScale,fontScale, orrowHeight/rowHeightCondensedto visually validate condensed rows and accessibility/large-font behavior.Also applies to: 61-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
app/containers/ServerItem/__snapshots__/ServerItem.test.tsx.snapis excluded by!**/*.snapapp/views/NewServerView/components/ServersHistoryItem/__snapshots__/ServersHistoryItem.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
app/i18n/locales/ar.json(2 hunks)app/i18n/locales/bn-IN.json(2 hunks)app/i18n/locales/cs.json(1 hunks)app/i18n/locales/de.json(1 hunks)app/i18n/locales/en.json(2 hunks)app/i18n/locales/es.json(2 hunks)app/i18n/locales/fi.json(1 hunks)app/i18n/locales/fr.json(1 hunks)app/i18n/locales/hi-IN.json(2 hunks)app/i18n/locales/hu.json(1 hunks)app/i18n/locales/it.json(2 hunks)app/i18n/locales/ja.json(2 hunks)app/i18n/locales/nl.json(2 hunks)app/i18n/locales/nn.json(2 hunks)app/i18n/locales/no.json(2 hunks)app/i18n/locales/pt-BR.json(1 hunks)app/i18n/locales/pt-PT.json(2 hunks)app/i18n/locales/ru.json(2 hunks)app/i18n/locales/sl-SI.json(1 hunks)app/i18n/locales/sv.json(1 hunks)app/i18n/locales/ta-IN.json(2 hunks)app/i18n/locales/te-IN.json(2 hunks)app/i18n/locales/tr.json(2 hunks)app/i18n/locales/zh-CN.json(1 hunks)app/i18n/locales/zh-TW.json(1 hunks)app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.stories.tsx(1 hunks)app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.test.tsx(1 hunks)app/views/RoomsListView/components/ServersList.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- app/i18n/locales/pt-BR.json
- app/i18n/locales/hi-IN.json
- app/i18n/locales/cs.json
- app/i18n/locales/de.json
- app/i18n/locales/zh-CN.json
- app/i18n/locales/ru.json
- app/i18n/locales/bn-IN.json
- app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.test.tsx
- app/i18n/locales/nl.json
- app/i18n/locales/es.json
- app/i18n/locales/ar.json
- app/i18n/locales/ta-IN.json
- app/i18n/locales/no.json
- app/i18n/locales/fi.json
- app/i18n/locales/nn.json
- app/i18n/locales/zh-TW.json
- app/i18n/locales/en.json
- app/i18n/locales/te-IN.json
- app/i18n/locales/sv.json
- app/i18n/locales/fr.json
- app/i18n/locales/hu.json
- app/i18n/locales/it.json
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.stories.tsx (5)
app/definitions/IServerHistory.ts (1)
TServerHistoryModel(11-11)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (3)
BASE_ROW_HEIGHT(21-21)BASE_ROW_HEIGHT_CONDENSED(22-22)ResponsiveLayoutContext(18-18)app/theme.tsx (2)
TSupportedThemes(8-8)ThemeContext(18-18)app/views/NewServerView/components/ServersHistoryItem/index.tsx (1)
IServersHistoryItem(14-18)app/lib/constants/colors.ts (1)
themes(304-304)
app/views/RoomsListView/components/ServersList.tsx (2)
app/lib/constants/colors.ts (1)
colors(280-302)app/containers/message/Components/Attachments/Image/Button.tsx (1)
Button(13-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (6)
app/i18n/locales/tr.json (2)
15-16: Accessibility localization keys added correctly.The new Turkish translations for
Activate_to_select_serverandActivate_to_select_server_Available_actions_deleteare properly formatted and align with the swipe-to-delete feature context. The phrasing is natural and accessible in Turkish.
640-640: Workspaces capitalization aligned with other locales.The update from lowercase to capitalized "Çalışma Alanları" improves consistency across the i18n locales and matches the UI pattern used elsewhere in the codebase.
app/i18n/locales/pt-PT.json (1)
15-16: Localization additions in pt-PT.json are correct, but incomplete translation coverage exists across supported locales.The three new keys (
Activate_to_select_server,Activate_to_select_server_Available_actions_delete,Workspaces) are properly added to pt-PT.json with accurate Portuguese translations and correct alphabetical ordering.However, verification shows these keys are only present in 25 out of 67 supported locales (37% coverage). The application uses
i18n.fallbacks = truewith English (en.json) as the default fallback locale, so incomplete translations will gracefully fall back to English. While this is expected and acceptable, it should be noted that for a production-quality user experience, translations for at least the major locales (ar, de, es, fr, pt-BR, ru, zh-CN, etc.) should be prioritized.The review comment's verification suggestion was sound and confirmed the fallback mechanism is in place to handle missing translations in less-frequently-used locales.
app/views/NewServerView/components/ServersHistoryItem/ServersHistoryItem.stories.tsx (1)
35-59: Wrapper component composition and typing look solidThe
ServersHistoryItemstory wrapper correctly wiresThemeContextandResponsiveLayoutContextand acceptsPartial<TServerHistoryModel>so stories can override only the needed fields while keeping sensible defaults. TheonPress/onDeletePresstypings viaIServersHistoryItemkeep the story API aligned with the production component.app/views/RoomsListView/components/ServersList.tsx (2)
124-133: LGTM! UI improvements align with PR objectives.The background color change to
surfaceLightand label update to "Workspaces" are consistent with the new action sheet layout design described in the PR objectives.
143-153: All style and color token definitions are properly defined and exist.Verification confirms:
styles.addServerButtonContainer(line 50, RoomsListView/styles.ts)styles.buttonCreateWorkspace(line 43, RoomsListView/styles.ts)colors.buttonFontSecondary(line 86, lib/constants/colors.ts)colors.buttonBackgroundSecondaryDefault(line 68, lib/constants/colors.ts)The style name
buttonCreateWorkspacedoes not need to match the button text "Add_Server"—style reuse across different components is standard practice and poses no risk. No runtime errors will occur.
Proposed changes
Improve Servers History and Servers List action sheet layout.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1063
How to test or reproduce
Open the app.
Log in to two servers.
Tap the Servers List button.
Swipe on a workspace and tap Delete.
After logging in to two workspaces, remove one of them.
Tap the Servers List button.
Tap Add Workspace.
Tap the Servers History button.
Swipe to delete the workspace that was previously removed.
Screenshots
Servers List
Servers History
RTL
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.