feat: support multi level grouping(Upper limit up to 4th level) #149
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChannelGrouper
participant ChannelManipulator
participant DomChannelManipulator
User->>ChannelGrouper: applyGroupingToContexts(channelItemContexts)
ChannelGrouper->>ChannelGrouper: Process contexts and establish connections
ChannelGrouper-->>User: ConnectedGroupedChannelItemContext[]
User->>DomChannelManipulator: updateChannelItems(ConnectedGroupedChannelItemContext[])
DomChannelManipulator->>DomChannelManipulator: Determine display logic
DomChannelManipulator->>DomChannelManipulator: getSpans(item)
DomChannelManipulator-->>User: Render channel items
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning @types/[email protected]: This is a stub types definition. prettier provides its own type definitions, so you do not need this installed. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
app/styles/content.css (1)
29-58: Consider using CSS custom properties for better maintainabilityTo further improve maintainability and support for multi-level grouping, consider using CSS custom properties (variables) for shared values like spacing, scaling, and font families. This would make it easier to maintain consistent styling across different grouping levels.
Example approach:
:root { --scg-separator-margin: 0 .2em 0 .1em; --scg-separator-scale: 1.0, 1.5; --scg-separator-font: "SlackChannelsGrounping-NotoSansJP-Medium", monospace; } .scg-ch-separator-base { margin: var(--scg-separator-margin); transform: scale(var(--scg-separator-scale)); font-family: var(--scg-separator-font); /* ... other properties ... */ }app/scripts/content/channel-manipulators/channel-manipulator.ts (1)
38-39: Remove commented-out code to improve code cleanlinessThe commented-out code in
removeRightConnectionseems unnecessary. Removing it can enhance readability and maintainability.Apply this diff to remove the commented code:
-// } else if (conn === '└' || conn === "─") { -// return " ";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
app/scripts/content/channel-grouper.ts(3 hunks)app/scripts/content/channel-manipulators/channel-manipulator.ts(1 hunks)app/scripts/content/channel-manipulators/dom-channel-manipulator.ts(3 hunks)app/styles/content.css(1 hunks)tests/channel-grouper.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/channel-grouper.test.ts
🔇 Additional comments (2)
app/styles/content.css (1)
50-58: Verify the impact of selector changes
The simplified selectors look good and align with the multi-level grouping support. However, we should verify that removing .scg-ch-separator from the selectors doesn't affect existing styles.
✅ Verification successful
Selector changes are safe to proceed
The simplified selectors are safe because:
- The TypeScript code correctly applies the base class
scg-ch-separatoralong with the pseudo classes where vertical lines are needed - The
scg-ch-separator-no-vertical-lineclass is used for cases where no vertical line is desired - The pseudo classes (top/bottom/both) are always used in combination with their appropriate base classes
- The CSS structure maintains the necessary styling hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other CSS rules that might rely on the original selector specificity
# Search for any rules using the original selectors
rg -l "scg-ch-separator.*pseudo"
# Search for any JavaScript code that might be adding these classes
rg -l "scg-ch-separator-pseudo"
Length of output: 248
Script:
#!/bin/bash
# Let's check how these classes are used in the TypeScript file
rg -A 5 -B 5 "scg-ch-separator-pseudo" "app/scripts/content/channel-manipulators/dom-channel-manipulator.ts"
# Also check the full CSS context to understand the selector relationships
cat "app/styles/content.css"
Length of output: 1859
app/scripts/content/channel-grouper.ts (1)
98-115: Ensure consistent handling of special characters in channel names
When splitting channel names and assigning prefixes, consider channel names that contain special characters or multiple separators. This can prevent unexpected grouping behavior.
Run the following script to check for channel names with special characters:
✅ Verification successful
Let me try a different approach to find channel name patterns in the actual code rather than dependency files.
Let me check the actual channel name splitting regex pattern used in the code.
Based on the evidence gathered, I can now provide a final response:
Channel name splitting implementation handles special characters appropriately
The code already implements proper handling of special characters in channel names:
- The regex pattern
/[-_]/specifically splits on hyphens and underscores only - Test cases cover various scenarios including special characters (
a_b-1,a.c-1) - Direct messages (
im) and multi-person direct messages (mpim) are explicitly excluded from grouping - The implementation preserves other special characters in channel names without affecting the grouping logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find channel names with special characters that might affect grouping logic.
# Search for channel names with non-alphanumeric characters
rg -oP 'name: "\K[^"]*' | grep '[^\w-]'
Length of output: 228
Script:
#!/bin/bash
# Search for channel name patterns in test files and source code
rg -g '!yarn.lock' -g '!package*.json' "channelItemType|channelName|name.*[:'\"].*[^a-zA-Z0-9-].*[:'\"]" -A 2
Length of output: 18671
Script:
#!/bin/bash
# Find the regex pattern used for splitting channel names
rg "REGEX_CHANNEL_MATCH_FOR_SPLIT" -A 2
Length of output: 461
|
I have found a bug in below case.
I will fix it. |
…rev, the prefixes at the higher level needed to be the same.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/scripts/content/channel-grouper.ts (2)
98-115: Consider performance optimization for large channel listsThe current implementation creates new objects for each channel context and performs string splits for every item. For large channel lists, consider:
- Memoizing split results
- Using array destructuring for better readability
- const split = context.name.split(REGEX_CHANNEL_MATCH_FOR_SPLIT); + const [prefix, prefix2, prefix3, ...rest] = context.name.split(REGEX_CHANNEL_MATCH_FOR_SPLIT); return { ...context, - prefix: split[0] || null, - prefix2: split[1] || null, - prefix3: split[2] || null, + prefix: prefix || null, + prefix2: prefix2 || null, + prefix3: prefix3 || null, };
Line range hint
78-222: Consider architectural improvements for maintainabilityWhile the implementation successfully supports multi-level grouping, consider these architectural improvements:
- Create a dedicated ConnectionManager class to handle connection logic
- Implement a Builder pattern for constructing grouped contexts
- Add unit tests for edge cases and connection logic
- Consider using a configuration object for grouping rules instead of hardcoding them
This would improve:
- Code maintainability
- Testing capabilities
- Future extensibility
Would you like help implementing any of these architectural improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/scripts/content/channel-grouper.ts(3 hunks)
🔇 Additional comments (1)
app/scripts/content/channel-grouper.ts (1)
21-22: Verify regex patterns for edge cases
The regex patterns for channel matching look correct but should be tested with various edge cases:
- Channels with multiple hyphens/underscores
- Special characters in channel names
- Very long channel names
✅ Verification successful
Let me gather more specific information about channel name patterns and their tests.
Let me check the actual test file found by fd and examine the test cases.
The regex patterns are well-tested and handle all edge cases appropriately
Based on the comprehensive test file, the regex patterns /(^.+?)[-_]/ and /[-_]/ are thoroughly validated against various scenarios including:
- Multiple hyphens/underscores in channel names
- Special characters in channel names (e.g., 'a_b-1', 'a.c-1')
- Very long channel names (tested with repeated patterns)
- Numeric prefixes ('1-project-a')
- Complex nested structures ('a-b-c-a-c')
- Empty channel lists
- Mixed channel types (im, mpim, channel)
The test cases cover all potential edge cases and the regex implementation is proven to work correctly in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for channel name patterns in the codebase to verify regex compatibility
rg -l "test.*[-_].*" || echo "No test files found with channel name patterns"
Length of output: 45
Script:
#!/bin/bash
# Look for test files related to channel grouping
fd "channel.*test" --type f
# Search for test cases or example channel names in the test files
rg -A 5 "channel.*[-_]" test/
rg -A 5 "channel.*[-_]" app/scripts/content/
# Check if there are any existing tests for the channel grouper
ast-grep --pattern 'describe($_, () => { $$$ })' app/scripts/content/channel-grouper.test.ts || echo "No test file found"
Length of output: 9717
Script:
#!/bin/bash
# Check the content of the test file
cat tests/channel-grouper.test.ts
# Look for actual channel name examples in the implementation
rg -B 2 -A 2 "REGEX_CHANNEL_MATCH" app/scripts/content/channel-grouper.ts
# Check how the regex is used in the code
ast-grep --pattern 'REGEX_CHANNEL_MATCH.exec($_)' app/scripts/content/channel-grouper.ts
Length of output: 13938
|
done. |
|
Thank you for submitting the PR! I apologize, but I'm little busy with other commitments, which may make it difficult for me to review this PR and implement the config page before the end of the year. Thanks again for your contribution! |
Support Multi level grouping (Upper limit up to 4th level)
Purpose
Fixed structure to display tree view with multiple levels
Changes
groupTypeand addconnection[1-3]in its place.Issue