feat(editor): add date grouping configurations #12679
Conversation
…lization capabilities ♻️ (define.ts): refactor existing group configurations for improved readability and maintainability ✨ (matcher.ts): add findGroupByConfigByName function to retrieve GroupByConfig by name for better configurability ✨ (date-group.ts): introduce DateGroupView component to enhance date grouping visualization in the UI ✨ (setting.ts): introduce dateModeLabel function to improve date mode labeling ♻️ (setting.ts): refactor group sorting logic for better readability and performance 📝 (setting.ts): update comments and formatting for improved code clarity 💡 (setting.ts): enhance code structure by simplifying conditional checks and removing unnecessary lines ✨ (setting.ts): add date grouping options and sorting functionality to enhance user experience 💡 (setting.ts): improve code readability by simplifying template literals and structure ✨ (trait.ts): introduce new signal and computed properties for better state management in GroupTrait class ♻️ (trait.ts): refactor group data handling and sorting logic for improved readability and maintainability 🐛 (trait.ts): fix potential issues with undefined values in groupInfo and property access 💡 (trait.ts): add comments to clarify the purpose of new functions and properties
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
""" WalkthroughThis update introduces comprehensive date-based grouping options to the group-by framework, including new configurations for relative, daily, weekly, monthly, and yearly date groupings. It adds a custom Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GroupSetting
participant GroupTrait
participant DateGroupView
User->>GroupSetting: Opens group setting popup
GroupSetting->>GroupTrait: Reads current group config
GroupSetting->>User: Shows date grouping options (mode, week start, sort, hide empty)
User->>GroupSetting: Selects new date grouping mode or sort order
GroupSetting->>GroupTrait: Updates group mode/sort/hideEmpty via methods
GroupTrait->>DateGroupView: Updates group data and sorting
DateGroupView->>User: Renders updated date group headers and counts
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
This PR is being worked and testing functionality on all options. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
blocksuite/affine/data-view/src/core/group-by/setting.ts (1)
31-47: Consider using a const object for better type safety.The switch statement works, but a const object would provide better type safety and maintainability.
-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': - return 'Relative'; - case 'date-day': - return 'Day'; - case 'date-week-mon': - case 'date-week-sun': - return 'Week'; - case 'date-month': - return 'Month'; - case 'date-year': - return 'Year'; - default: - return ''; - } -}; +const DATE_MODE_LABELS = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +} as const; + +const dateModeLabel = (key?: string) => { + return key && key in DATE_MODE_LABELS ? DATE_MODE_LABELS[key as keyof typeof DATE_MODE_LABELS] : ''; +};blocksuite/affine/data-view/src/core/group-by/define.ts (2)
38-44: Consider making the date format configurable.The date format is hardcoded as 'MMM d yyyy'. Consider extracting this as a constant or making it configurable for internationalization support.
+const DATE_FORMAT = 'MMM d yyyy'; + const rangeLabel = (a: Date, b: Date) => - `${fmt(a, 'MMM d yyyy')} – ${fmt(b, 'MMM d yyyy')}`; + `${fmt(a, DATE_FORMAT)} – ${fmt(b, DATE_FORMAT)}`;
61-83: Consider adding internationalization support for date labels.The relative date labels are hardcoded in English. For better internationalization support, consider making these configurable or using a translation system.
blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
183-199: Consider extracting the date group check logic.The implementation is correct, but the date group check using string prefix could be more robust.
- if (!gi || !gi.config.name?.startsWith('date-')) return; + const isDateGroup = gi?.config.matchType.type === 'date'; + if (!isDateGroup) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blocksuite/affine/data-view/src/core/group-by/define.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/matcher.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(6 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
blocksuite/affine/data-view/src/core/group-by/trait.ts (2)
property(69-71)Group(55-85)
🪛 ESLint
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (13)
blocksuite/affine/data-view/src/core/group-by/matcher.ts (2)
12-22: LGTM! Well-implemented config lookup function.The
findGroupByConfigByNamefunction provides a clean way to retrieve group configurations by name, properly combining both external and internal configurations.
25-25: Minor formatting change looks good.The constructor formatting adjustment maintains the same functionality.
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
7-49: Well-designed date group component!The component implementation is clean and follows best practices:
- Proper use of reactive properties with the
groupaccessor- Thoughtful CSS with hover states and flexible layout
- Conditional rendering of the count badge
- Good fallback text for unnamed groups
blocksuite/affine/data-view/src/core/group-by/setting.ts (1)
215-394: Excellent enhancement of date grouping UI!The implementation provides comprehensive date grouping configuration options including:
- Multiple date grouping modes (relative, day, week, month, year)
- Configurable week start day
- Sort order controls
- Empty group visibility toggle
The UI structure is well-organized with logical menu groupings.
blocksuite/affine/data-view/src/core/group-by/define.ts (4)
13-23: LGTM! Appropriate date-fns imports.All imported functions are utilized in the date grouping implementations.
45-59: Well-structured factory function.The
buildDateCfghelper provides a clean abstraction for creating date group configurations with consistent behavior.
141-234: Clean refactoring of existing matchers.The ternary expressions and arrow functions improve code readability while maintaining the same functionality.
95-119:⚠️ Potential issueFix incorrect week range calculation.
The week range calculation adds only 6 days instead of 6 full days (which would be 7 days total). This results in incomplete week ranges.
v => v - ? rangeLabel(new Date(v), new Date(v + 6 * DAY_MS)) + ? rangeLabel(new Date(v), new Date(v + 7 * DAY_MS - 1)) : '',Apply the same fix to both
dateWeekSunCfganddateWeekMonCfg.Likely an incorrect or invalid review comment.
blocksuite/affine/data-view/src/core/group-by/trait.ts (5)
29-43: Well-implemented date sorting logic.The
compareDateKeysfunction correctly handles different date key formats with appropriate fallbacks.
88-103: Clean constructor implementation.The explicit constructor with properly typed parameters improves code clarity and type safety.
104-106: Good use of reactive signals for UI state.The signals provide a clean way to manage group display options reactively.
107-124: Proper support for named group configurations.The updated logic correctly handles both named configurations and type-based matching with appropriate fallbacks.
270-309: Correct usage of jsonValueSet.The method properly handles moving cards between groups with appropriate value transformations.
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts
Outdated
Show resolved
Hide resolved
…improved readability and consistency across files
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
1-5: Import sorting issue persists from previous review.The import statements are still not sorted according to the project's ESLint configuration, as previously flagged.
🧰 Tools
🪛 ESLint
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
40-47: Consider adding error boundary and accessibility improvements.The render method correctly accesses the reactive signal and implements reasonable fallback logic. However, consider these improvements:
- Add error handling in case
this.groupis undefined- Consider accessibility attributes for screen readers
protected override render() { + if (!this.group) { + return html`<div class="dv-date-group">Loading...</div>`; + } const name = this.group.name$.value; const count = this.group.rows.length; - return html`<div class="dv-date-group"> + return html`<div class="dv-date-group" role="group" aria-label="Date group: ${name || 'Ungroups'}"> <span>${name || 'Ungroups'}</span> - ${count ? html`<span class="counter">${count}</span>` : ''} + ${count ? html`<span class="counter" aria-label="${count} items">${count}</span>` : ''} </div>`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(6 hunks)packages/backend/server/src/env.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/backend/server/src/env.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/core/group-by/setting.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (1)
blocksuite/affine/data-view/src/core/group-by/trait.ts (2)
property(69-71)Group(55-85)
🪛 ESLint
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (3)
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.ts (3)
7-35: Well-structured component with proper styling.The DateGroupView component follows good practices:
- Proper inheritance chain with SignalWatcher for reactivity and WithDisposable for cleanup
- CSS custom properties for theming consistency
- Flexbox layout with appropriate spacing and hover states
- Counter badge styling that's visually distinct
37-38: Property definition follows Lit best practices.The group property is correctly defined with
attribute: falsesince it's a complex object that shouldn't be serialized to an HTML attribute.
49-49: Custom element registration follows conventions.The component is properly registered with a descriptive tag name that follows the project's naming conventions.
Revert of accidental removal. Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
improved date formatting for better user experience ♻️ (group-by): refactor group settings menu to conditionally display options based on group configuration 🐛 (group-by): fix sorting order for date keys to ensure correct relative date comparisons
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
336-336: 💡 Verification agent🧩 Analysis chain
Verify API change from valueSet to jsonValueSet.
The change from
valueSettojsonValueSetcould be a breaking change. Ensure this is intentional and that the new method properly handles the data transformation.
🏁 Script executed:
#!/bin/bash # Description: Check for other uses of valueSet that might need updating # Expected: Find inconsistent usage of valueSet vs jsonValueSet rg "\.valueSet\(" --type ts -A 1 -B 1 rg "\.jsonValueSet\(" --type ts -A 1 -B 1Length of output: 5852
Fix inconsistent method usage in group-by trait
Two calls in
blocksuite/affine/data-view/src/core/group-by/trait.tsstill use.valueSet(newValue)even though the grouping logic now requires JSON values. Update both to.jsonValueSet(newValue)for correct behavior:• In the initial aggregation block (around line 336):
- this.view.cellGetOrCreate(rowId, propertyId).valueSet(newValue); + this.view.cellGetOrCreate(rowId, propertyId).jsonValueSet(newValue);• In the
rows.forEachloop:- this.view.cellGetOrCreate(rowId, propertyId).valueSet(value); + this.view.cellGetOrCreate(rowId, propertyId).jsonValueSet(value);Ensure that
jsonValueSeton the cell correctly accepts and persists the JSON payload.
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/core/group-by/define.ts (1)
38-59: Consider improving type safety and documentation.The helper constants and
buildDateCfgfunction are well-structured, but could benefit from better type annotations and documentation for the complex grouper function parameter.+/** + * Creates a date grouping configuration with standardized structure + * @param name - Unique identifier for the grouping mode + * @param grouper - Function that converts ISO date string to group keys/values + * @param groupName - Function that formats group values for display + */ function buildDateCfg( name: string, - grouper: (iso: string | null) => { key: string; value: number | null }[], + grouper: (iso: string | null) => Array<{ key: string; value: number | null }>, groupName: (v: number | null) => string, ): GroupByConfig {blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
191-197: Consider performance implications of date sorting.The date group sorting logic runs on every group data change. For large datasets, this could impact performance. Consider memoization or incremental updates.
+ // Consider memoizing the sort result based on group keys and sort direction + const sortedKeys$ = computed(() => { + const map = this.groupDataMap$.value; + const gi = this.groupInfo$.value; + if (!map || !gi) return []; + + return [...Object.keys(map)].sort( + compareDateKeys(gi.config.name, this.sortAsc$.value) + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocksuite/affine/data-view/src/core/group-by/define.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(6 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/core/group-by/setting.ts
🔇 Additional comments (6)
blocksuite/affine/data-view/src/core/group-by/define.ts (4)
10-10: LGTM: Clean imports for date functionality.The imports are well-organized and include all necessary date-fns functions and the new DateGroupView component.
Also applies to: 13-22
147-147: LGTM: Clean refactoring to ternary operators.The refactoring from verbose if/else blocks to ternary operators improves code conciseness while maintaining the same logic. The select, multi-select, and other existing configurations are correctly updated.
Also applies to: 157-160, 169-184
237-242:✅ Verification successful
Verify integration with date grouping system.
The new date configurations are properly added to the groupByMatchers array. Ensure these work correctly with the DateGroupView component and trait system.
🏁 Script executed:
#!/bin/bash # Description: Verify DateGroupView component exists and integrates correctly # Expected: Find the DateGroupView implementation and its usage fd "date-group" --type f --extension ts rg "DateGroupView" --type ts -A 3Length of output: 1321
Integration with DateGroupView Verified
- DateGroupView is implemented in
blocksuite/affine/data-view/src/core/group-by/renderer/date-group.tsand registered asdata-view-date-group-view.- It’s correctly imported in
define.tsand used viacreateUniComponentFromWebComponent(DateGroupView)alongside the new date configs.All integrations look correct—no further action needed.
61-84:✅ Verification successful
Verify edge cases in relative date grouping.
The relative date logic handles most cases well, but consider edge cases around daylight saving time transitions and timezone boundaries.
🏁 Script executed:
#!/bin/bash # Description: Check for potential timezone-related issues in date grouping # Expected: Find any hardcoded timezone assumptions or DST handling rg -A 3 -B 3 "startOfDay|differenceInCalendarDays" --type tsLength of output: 2966
No DST or timezone issues in relative date grouping
The
startOfDayanddifferenceInCalendarDaysfunctions from date-fns already account for local timezone offsets and daylight-saving shifts (the latter via an explicit timezone-offset correction and rounding). There are no hardcoded timezone assumptions here, so this grouping logic will remain correct across DST transitions and at timezone boundaries.blocksuite/affine/data-view/src/core/group-by/trait.ts (2)
256-265: LGTM: Clean method for changing group modes.The
changeGroupModemethod properly integrates with the new date grouping configurations and maintains sort state consistency.
110-133:✅ Verification successful
Verify reactive state synchronization.
The effect that syncs
sortAsc$with the groupBy sort descriptor looks correct, but ensure it doesn't create circular dependencies or infinite loops.
🏁 Script executed:
#!/bin/bash # Description: Check for potential circular dependencies in reactive signals # Expected: Find other places where sortAsc$ or groupBy sort is modified rg "sortAsc\$\.value\s*=" --type ts -A 2 -B 2 rg "sort.*desc" --type ts -A 2 -B 2Length of output: 8413
🏁 Script executed:
#!/bin/bash # Inspect the GroupBy trait implementation to understand two-way binding sed -n '1,200p' blocksuite/affine/data-view/src/core/group-by/trait.tsLength of output: 5838
No circular reactive dependencies detected
Verified that the
effectonly updatessortAsc$whengroupBy$.value.sort.descchanges, and all user-driven toggles go throughgroupBySet({ …, sort: { desc: !this.sortAsc$.value } }). Because the effect immediately reconcilessortAsc$to!desc(anddescwas set to!sortAsc$), it’s a no-op on the next run and doesn’t loop.
No further fixes required.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12679 +/- ##
==========================================
- Coverage 55.79% 55.77% -0.03%
==========================================
Files 2658 2660 +2
Lines 125984 126249 +265
Branches 19960 20051 +91
==========================================
+ Hits 70298 70413 +115
- Misses 54063 54209 +146
- Partials 1623 1627 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…te calculations in rangeLabel function
… sort date keys ♻️ (define.ts): refactor addToGroup functions to include type annotations for better clarity and maintainability ♻️ (trait.ts): refactor date comparison logic to improve readability and maintainability by removing unnecessary complexity ✨ (trait.ts): update group sorting logic to use new comparison method for better performance 📝 (vitest.config.ts): add Vitest configuration file to set up testing environment and coverage reporting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts (1)
38-59: Consider documenting the sorting priority for date-relative mode.The function implements a sophisticated fallback strategy: relative keys → numeric keys → lexicographic comparison. This behavior should be documented for maintainability.
Add JSDoc documentation to clarify the sorting priority:
+/** + * Creates a comparator function for date keys + * + * For 'date-relative' mode, sorting priority is: + * 1. Relative date keys (today, yesterday, etc.) + * 2. Numeric keys (timestamps) + * 3. Lexicographic comparison (fallback) + * + * For other modes, uses numeric comparison followed by lexicographic. + * + * @param mode - The date grouping mode + * @param asc - Whether to sort in ascending order + * @returns Comparator function for Array.sort() + */ export function compareDateKeys(mode: string | undefined, asc: boolean) {blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts (1)
28-34: Add more specific assertions for mixed key sorting behavior.While the test validates the general sorting priority (numeric keys before relative keys), it could be more explicit about the expected ordering.
Consider adding more specific assertions to document the expected behavior:
it('handles mixed relative and numeric keys', () => { const cmp = compareDateKeys('date-relative', true); const keys = ['today', '1', 'yesterday', '2']; const sorted = [...keys].sort(cmp); - expect(sorted[0]).toBe('1'); - expect(sorted[sorted.length - 1]).toBe('today'); + // Numeric keys should come first, then relative keys in chronological order + expect(sorted).toEqual(['1', '2', 'yesterday', 'today']); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/define.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(6 hunks)blocksuite/affine/data-view/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/data-view/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- blocksuite/affine/data-view/src/core/group-by/define.ts
- blocksuite/affine/data-view/src/core/group-by/trait.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts (1)
blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts (1)
compareDateKeys(38-59)
🔇 Additional comments (5)
blocksuite/affine/data-view/src/core/group-by/compare-date-keys.ts (3)
1-7: LGTM! Clean constant definitions with good naming.The constants are well-defined and the use of
as constensures proper type inference. The reverse operation for descending order is clean and efficient.
27-36: Good use of Number.isFinite for robust numeric validation.The function correctly uses
Number.isFinite()to avoid issues withNaNandInfinityvalues, and appropriately returns 0 when keys aren't both numeric.
12-22:✅ Verification successful
Verify the sorting logic for mixed relative/non-relative keys.
The function correctly handles the case where both keys are relative date keys, but the logic for mixed cases (one relative, one non-relative) needs verification.
🏁 Script executed:
#!/bin/bash # Verify that the sorting behavior matches the expected test cases # by examining how relative keys are prioritized over non-relative keys # Look for test cases that validate mixed key sorting rg -A 10 -B 2 "mixed.*relative" rg -A 10 -B 2 "handles mixed"Length of output: 2189
Mixed relative/non-relative key sorting is already covered by existing tests
The unit test in
blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts(“handles mixed relative and numeric keys”) confirms that numeric keys are sorted before relative keys whenascis true. ThesortRelativeKeysimplementation correctly returns positive for relative vs. non-relative (and negative for the inverse), matching the test expectations. No changes required.blocksuite/affine/data-view/src/__tests__/compare-date-keys.unit.spec.ts (2)
6-18: Excellent test coverage for relative date key sorting.These tests properly validate both ascending and descending order for relative date keys, with clear expectations that match the defined constants.
20-26: Good numeric key sorting validation.The tests correctly verify numeric string comparison in both ascending and descending order for non-relative date modes.
zzj3720
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts (1)
226-228: 🛠️ Refactor suggestionSub-menu misses the middleware you just introduced
popCreateSortis now middleware–aware, but the inner invocation doesn’t forward the array you already accepted inpopSortRoot.
Forward it so submenu placement is consistent:- popCreateSort(popupTargetFromElement(ele), { - sortUtils: props.sortUtils, - }); + popCreateSort( + popupTargetFromElement(ele), + { sortUtils: props.sortUtils }, + { middleware: (middleware ?? []).filter(Boolean) as Middleware[] } + );Without this, the “Add sort” panel falls back to its default placement while the parent obeys the new middleware, resulting in a jarring UI jump.
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/core/group-by/setting.ts (2)
276-284: Redundanttype === 'date'guard is still present
This block is already inside a'date'branch; the extra ternary wrapper only adds noise.
295-300: Nested ternary for week mode not addressedThe earlier review suggested extracting this into a
weekModeconstant for clarity. The nested ternary is still here and remains hard to scan.
🧹 Nitpick comments (8)
blocksuite/affine/data-view/src/core/sort/add-sort.ts (1)
18-24: Guard againstundefinedmiddleware & keep the payload lean
popMenureceivesmiddleware: ops?.middleware, which will explicitly set the property toundefinedwhenopsis not supplied.
While many implementations tolerate anundefined, it is cleaner – and safer for future refactors – to omit the key entirely when no middleware is provided:- popMenu(target, { - middleware: ops?.middleware, + popMenu(target, { + ...(ops?.middleware && { middleware: ops.middleware }),This prevents unnecessary property injections and spares a needless runtime check inside
popMenu.blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts (1)
384-390: Omitmiddlewarekey when absentSame nit as in
add-sort.ts: pass the key conditionally to avoid{ middleware: undefined }.- popMenu(target, { - middleware, + popMenu(target, { + ...(middleware && { middleware }),Keeps the payload minimal and future-proof.
blocksuite/affine/data-view/src/core/common/properties.ts (1)
240-244: Consistent conditional spread for middlewareReplicate the conditional-spread pattern here as well:
- popMenu(target, { - middleware, + popMenu(target, { + ...(middleware && { middleware }),Prevents sending an explicit
undefinedvalue downstream.blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (1)
121-125: DRY up the duplicated Floating-UI middleware arraysThe exact three-element array (
autoPlacement,offset,shift) is repeated many times in this file (and others).
Extracting it into a constant will:
- Eliminate copy-paste noise.
- Make future tuning (e.g., offset tweaks) one-shot.
- Reduce the risk of accidental drift.
Example:
// utils/floating-ui.ts (or local to this file) import { autoPlacement, offset, shift, type Middleware } from '@floating-ui/dom'; export const defaultMenuMiddleware: Middleware[] = [ autoPlacement({ allowedPlacements: ['bottom-start', 'top-start'] }), offset({ mainAxis: 12, crossAxis: -75 }), shift({ crossAxis: true }), ]; // usage popPropertiesSetting(target, props, defaultMenuMiddleware); popCreateFilter(target, opts, { middleware: defaultMenuMiddleware }); ...Makes the intent crystal clear and trims ~60 lines of boilerplate.
Also applies to: 167-174
blocksuite/affine/data-view/src/core/group-by/setting.ts (4)
32-48: Replaceswitchwith a map to reduce boilerplateA simple object literal is more concise and scales better when more modes are introduced.
-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': - return 'Relative'; - case 'date-day': - return 'Day'; - case 'date-week-mon': - case 'date-week-sun': - return 'Week'; - case 'date-month': - return 'Month'; - case 'date-year': - return 'Year'; - default: - return ''; - } -}; +const DATE_MODE_LABEL: Record<string, string> = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +}; +const dateModeLabel = (key?: string) => DATE_MODE_LABEL[key ?? ''] ?? '';
213-217: UI copy is inconsistent (‘Remove Grouping’ vs ‘Remove grouping’)Two actions that perform the same operation use different capitalisation. This will look sloppy in the menu.
Unify the wording (either title-case or sentence-case) in both places.
Also applies to: 465-471
291-323: Repeated inline colour logic ⇒ extract helper
isSelected ? emphasisedColor : secondaryColoris duplicated multiple times across the menu construction. Extracting a small helper (e.g.,selectedColor(is)or a styled<span>wrapper) will shrink this block dramatically and make future tweaks painless.
443-449: Toggle action lacks keyboard shortcut hintMany AFFiNE menu actions show shortcut hints; adding one here (e.g.
⇧E?) keeps the UX coherent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
blocksuite/affine/data-view/src/core/common/properties.ts(2 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(7 hunks)blocksuite/affine/data-view/src/core/sort/add-sort.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(7 hunks)
…and UI elements feat(group-by): implement group hide state management in Group and GroupTrait classes feat(kanban/table): add changeGroupHide method to update group visibility in view managers
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/core/group-by/trait.ts (2)
152-156: Side-effect insidecomputedis still present
groupInfo$callsview.propertyGetOrCreate, which mutates state during a reactive computation – the exact concern raised in a previous review. This risks subtle re-compute loops and violates computed purity.
Consider using a side-effect-free getter (or ensuring the property exists before the signal is accessed).
191-202: Rows keep accumulating on every recomputation
groupDataMap$clonesstaticMapbut re-uses the sameGroupinstances whoserowsarrays aren’t cleared. Each re-evaluation appends the same rows again, leading to duplicates & memory growth – the issue flagged earlier but still unfixed.-const map: Record<string, Group> = { ...staticMap }; +const map: Record<string, Group> = { ...staticMap }; +Object.values(map).forEach(g => (g.rows.length = 0)); // reset
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts (1)
139-150: Avoid needless writes whenhideflag is unchanged
dataUpdatewill still enqueue a patch even when the existinghidevalue already matches the requested one, triggering an extra re-render.
Guard early to save a signal emission:-const idx = list.findIndex(g => g.key === key); -if (idx >= 0) { - list[idx] = { ...list[idx], hide }; +const idx = list.findIndex(g => g.key === key); +if (idx >= 0) { + if (list[idx].hide === hide) { + return {}; // no-op – bail early + } + list[idx] = { ...list[idx], hide };blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts (1)
167-176: Same early-exit optimisation applies hereReplicate the short-circuit to avoid emitting an identical
groupPropertiesarray when nothing actually changed.-if (idx >= 0) { - list[idx] = { ...list[idx], hide }; +if (idx >= 0) { + if (list[idx].hide === hide) { + return {}; // nothing to update + } + list[idx] = { ...list[idx], hide };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(5 hunks)blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/core/group-by/setting.ts
feat(group-by): introduce groupsDataListAll$ to include hidden groups for better UI management style(group-by): add group-hidden class to visually indicate hidden groups with reduced opacity
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
blocksuite/affine/data-view/src/core/group-by/setting.ts (2)
421-426: Nested ternary resurrected – please extract to a constant for readabilityThe
Weekentry still relies on the double ternary that was already flagged in a previous review.['Week', group.groupInfo$.value?.config.name === 'date-week-mon' ? 'date-week-mon' : 'date-week-sun']A one-liner constant (e.g.
const weekMode = …) keeps the JSX block clean and prevents the visual “arrow of doom”.
402-404: Redundanttype === 'date'guard still present
popGroupSettinghas already short-circuited whentypeis not'date', yet we wrap the whole date submenu in...(type === 'date' ? [ /* … */ ] : [])The extra check bloats diff noise and future maintenance.
Delete the guard and keep the array literal directly.blocksuite/affine/data-view/src/core/group-by/trait.ts (1)
152-156: Side-effect insidecomputed– still usespropertyGetOrCreate
groupInfo$callsview.propertyGetOrCreate, which creates a property during reactive evaluation.
Computed signals are expected to be pure; creating state here can trigger subtle recomputation loops.
Switch topropertyGetand ensure the caller creates the column beforehand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(5 hunks)
…remove unused postfix function in popGroupSetting and maintain order when inserting new entries in lists to improve code clarity and functionality
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/core/group-by/setting.ts (2)
384-386: Redundanttype === 'date'guard still present
Same issue pointed out in previous review – this block already sits inside a'date'type branch.
404-408: Nested ternary for week mode unchanged
Consider extracting intoconst weekMode = …for readability as suggested earlier.
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/core/group-by/setting.ts (2)
34-50: Replaceswitchwith a declarative map for scalabilityA mapping object + fallback is easier to extend and avoids repetitive
caseblocks.-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': - return 'Relative'; - case 'date-day': - return 'Day'; - case 'date-week-mon': - case 'date-week-sun': - return 'Week'; - case 'date-month': - return 'Month'; - case 'date-year': - return 'Year'; - default: - return ''; - } -}; +const DATE_MODE_LABEL: Record<string, string> = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +}; + +const dateModeLabel = (key?: string) => DATE_MODE_LABEL[key ?? ''] ?? '';Cleaner, easier to read, and O(1) look-ups.
63-92: Scrollbar styling: add prefers-reduced-motion & non-WebKit supportNice UX touch, but consider:
- Firefox: colours set, but width cannot be styled – fine.
- Windows high-contrast / accessibility: expose via
prefers-reduced-motionor allow users to override.Optional, but improves accessibility cross-platform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts
- blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/core/group-by/setting.ts (7)
blocksuite/affine/data-view/src/core/group-by/trait.ts (3)
property(54-56)GroupTrait(98-499)view(75-77)blocksuite/affine/data-view/src/view-presets/kanban/kanban-view-manager.ts (3)
type(191-193)view(195-197)KanbanSingleView(22-284)blocksuite/affine/data-view/src/view-presets/table/table-view-manager.ts (2)
type(213-215)TableSingleView(29-287)blocksuite/affine/data-view/src/core/group-by/types.ts (1)
GroupRenderProps(5-11)blocksuite/affine/data-view/src/core/view-manager/property.ts (1)
icon(142-145)blocksuite/affine/components/src/icons/text.ts (2)
ViewIcon(270-273)DeleteIcon(154-157)blocksuite/affine/components/src/context-menu/menu-renderer.ts (2)
PopupTarget(402-406)popMenu(520-565)
…ns to MenuSubMenu for enhanced customization fix(context-menu): adjust event handling for mouse enter based on openOnHover setting feat(group-by): integrate dropdownSubMenuMiddleware and new options in popGroupSetting for improved submenu behavior refactor(group-by): separate visible and hidden groups in GroupTrait for better management of group visibility
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
blocksuite/affine/data-view/src/core/group-by/trait.ts (4)
152-156:⚠️ Potential issueSide-effect inside a
computedis still present
view.propertyGetOrCreatemutates application state every time the reactive
computation runs, breaking computed-purity and risking infinite
re-computations.
The exact same problem was pointed out in a previous review but has not been
addressed.-const prop = this.view.propertyGetOrCreate(gb.columnId); +// Do not create the property here – just read it +const prop = this.view.propertyGet(gb.columnId); +if (!prop) return;Move the “create if missing” logic outside the reactive graph (e.g. right
beforegroupInfo$is first accessed).
191-202:⚠️ Potential issueRows accumulate &
cellGetOrCreatemutates during recompute
Group.rowsis never cleared, so every recomputation appends another copy of
each row, causing duplicates and an ever-growing memory footprint.view.cellGetOrCreateinside the loop mutates the data model, again violating
computed-purity.Both issues were flagged earlier.
// before assigning rows -const map: Record<string, Group> = { ...staticMap }; +const map: Record<string, Group> = { ...staticMap }; +Object.values(map).forEach(g => (g.rows.length = 0)); -const cell = this.view.cellGetOrCreate(row.rowId, groupInfo.property.id); +const cell = this.view.cellGet(row.rowId, groupInfo.property.id); +if (!cell) return; // or skip the row
331-338:⚠️ Potential issueInconsistent setter – should use
jsonValueSet
addToGroupwrites viavalueSet, while the rest of the codebase (including
moveCardTo) usesjsonValueSet. Mixing the two risks stale caches or
conversion bugs.- this.view.cellGetOrCreate(rowId, info.property.id).valueSet(newVal); + this.view.cellGetOrCreate(rowId, info.property.id).jsonValueSet(newVal);
490-496:⚠️ Potential issueSame setter inconsistency in
removeFromGroupThe value is written with
valueSet; for parity with other paths use
jsonValueSet.- this.view.cellGetOrCreate(rowId, propId).valueSet(newValue); + this.view.cellGetOrCreate(rowId, propId).jsonValueSet(newValue);
🧹 Nitpick comments (2)
blocksuite/affine/components/src/context-menu/sub-menu.ts (2)
25-28: Consider adding JSDoc / comments for the three new flags.
openOnHover,middleware, andautoHeightsubtly change submenu behaviour. A short doc-comment in the type alias (or at least inline/** … */) will spare future readers a trip through the implementation each time they wonder what the defaults are and how they interact.
90-93:autoHeightstyle duplication—extract helper or CSS class.The identical four-line block that forces
minHeight/maxHeighttofit-contentnow appears three times.
Factor it into a tiny helper or dedicate a CSS utility class to keep the logic DRY and easier to adjust later.Also applies to: 112-115, 174-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocksuite/affine/components/src/context-menu/sub-menu.ts(8 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/core/group-by/setting.ts
🔇 Additional comments (2)
blocksuite/affine/components/src/context-menu/sub-menu.ts (2)
38-42:dropdownSubMenuMiddlewareis declared but not used in this file.If it’s intended for external consumption, export it; if it’s only a convenience for callers, place it next to the code that actually references it to avoid “dead-code” alarms.
90-93: Verify cross-browser support forfit-contenton min/max-height.
fit-contentis well-supported in modern evergreen browsers but still elicits issues in some older Safari/Chromium versions. If you need broader coverage consider a fallback (auto) or feature test.
…bMenu for better control over menu behavior feat(data-view): add onClose callback to popGroupSetting and popFilterRoot for enhanced closure handling feat(view-manager): add data$ property to SingleView interface for improved data management feat(sort): add onClose option to popSortRoot for consistent closure handling across components feat(view-options.ts): add closeMenu callback to createSettingMenus for better menu management feat(view-options.ts): set closeOnSelect to false for multiple menu actions to prevent premature closure refactor(view-options.ts): update popViewOptions to handle menu close events more effectively
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
blocksuite/affine/data-view/src/core/group-by/setting.ts (4)
176-179: Constant fallback key'k'still risks collisions
Same concern raised previously: differentundefinedgroups share the same DnD id, breaking pointer re-ordering.See earlier review; please address or acknowledge.
400-404: Redundanttype === 'date'wrapper
Already inside a date-specific branch; condition adds noise without value.(Flagged in prior review.)
423-427: Nested ternary for week mode is still presentReadability issue noted earlier remains. Extract a helper or constant instead of the inline ternary.
584-588:columnIdprop passed but not declared
<data-view-group-setting … .columnId=${gProp.id}>—GroupSettinglacks such a property, causing TS / lit warnings.(Same remark in previous review.)
blocksuite/affine/components/src/context-menu/sub-menu.ts (1)
93-107: Missing middleware fallback in click pathWhen
openOnHover === false,popMenuis called withmiddleware: this.data.middlewaredirectly.
If callers omit the field, the default placement stack is lost and sub-menus may spawn at (0,0).Identical issue reported earlier; please apply the fallback used further below:
- middleware: this.data.middleware, + middleware: this.data.middleware ?? subMenuMiddleware,(and mirror in
MobileSubMenu’sopenSubMenu).
🧹 Nitpick comments (3)
blocksuite/affine/data-view/src/core/view-manager/single-view.ts (1)
88-90: Markdata$aspublic readonly- data$ = computed(() => { + public readonly data$ = computed(() => {Making intent explicit eliminates doubts around mutation and keeps the declaration aligned with the interface above.
No functional impact, purely clarity.blocksuite/affine/components/src/context-menu/button.ts (1)
84-90: Duplication between desktop & mobile click-handlers
MenuButton.onClickandMobileMenuButton.onClicknow share identical logic (lines 84-90 and 146-152). Consider extracting a small helper to avoid future drift:- onClick() { - if (this.data.select(this) !== false) { - this.menu.options.onComplete?.(); - if (this.data.closeOnSelect !== false) { - this.menu.close(); - } - } - } + private handleSelect() { + if (this.data.select(this) !== false) { + this.menu.options.onComplete?.(); + if (this.data.closeOnSelect !== false) this.menu.close(); + } + }Then invoke
this.handleSelect()from both subclasses.
Keeps behaviour in one place, easier to tweak later.blocksuite/affine/data-view/src/core/group-by/setting.ts (1)
35-51: Hard-coded English labels & fall-through default
dateModeLabelhard-codes English strings and returns an empty string for unknown keys.
- Surface labels through your i18n utility or constant map.
- Consider returning the key itself (or throwing) when unmapped to avoid silent blanks.
-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': return 'Relative'; - ... - default: return ''; - } -}; +const DATE_MODE_LABEL: Record<string, string> = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +}; +const dateModeLabel = (k?: string) => DATE_MODE_LABEL[k ?? ''] ?? k ?? '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
blocksuite/affine/components/src/context-menu/button.ts(5 hunks)blocksuite/affine/components/src/context-menu/sub-menu.ts(8 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/core/view-manager/single-view.ts(1 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts
- blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts
… performance and flexibility in rendering options
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
blocksuite/affine/data-view/src/core/group-by/setting.ts (4)
175-179: Potential duplicate/empty drag-and-drop IDs remain
Fallback key'k'can still collide and break dnd-kit. Previous feedback is still applicable.
220-223:repeatkey fallback may collide
Using constant'k'risks duplicate keys; please generate a stable unique fallback or filter undefined items.
426-430: Nested ternary hampers readability
The earlier suggestion to extractweekModeinto a variable is still unaddressed.
598-602:columnIdattribute is still undeclared inGroupSetting
Passing an undeclared reactive property triggersno-unknown-propertylint errors and wastes memory. Either declare it or remove the attribute.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/core/group-by/setting.ts (1)
35-51: Replace verbose switch with a constant map for easier maintenanceA lookup table shortens the code and avoids future switch-statement churn when new modes are added.
-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': - return 'Relative'; - case 'date-day': - return 'Day'; - case 'date-week-mon': - case 'date-week-sun': - return 'Week'; - case 'date-month': - return 'Month'; - case 'date-year': - return 'Year'; - default: - return ''; - } -}; +const DATE_MODE_LABEL: Record<string, string> = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +}; +const dateModeLabel = (key?: string) => DATE_MODE_LABEL[key ?? ''] ?? '';
…o allow dynamic height adjustment fix(view-options.ts): set min-height for menu dynamically to ensure consistent UI across components
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (2)
124-128: DRY up the repeated floating-UI middleware arrayThe identical middleware tuple (
autoPlacement,offset,shift) is inlined nine times. Centralising it:+const POSITION_MIDDLEWARE = [ + autoPlacement({ allowedPlacements: ['bottom-start', 'top-start'] }), + offset({ mainAxis: 15, crossAxis: -162 }), + shift({ crossAxis: true }), +] as const;and replacing all occurrences with
POSITION_MIDDLEWARE:- middleware: [ - autoPlacement({ allowedPlacements: ['bottom-start', 'top-start'] }), - offset({ mainAxis: 15, crossAxis: -162 }), - shift({ crossAxis: true }), - ], + middleware: POSITION_MIDDLEWARE,• removes >70 duplicated LOC
• guarantees future tweaks stay consistent.Also applies to: 173-179, 185-190, 203-208, 247-253, 300-304, 309-314, 318-323, 499-503
504-505: Hard-coded min-height – favour CSS over runtime mutationDirect style mutation couples logic & presentation and is hard to theme. Prefer a CSS rule:
.affine-view-settings-menu { min-height: 550px; }and assign the class to
menu.menuElementonce, instead of mutating style at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/components/src/context-menu/menu-renderer.ts(2 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/components/src/context-menu/menu-renderer.ts
🔇 Additional comments (1)
blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts (1)
469-480:closeOnSelect: falselooks wrong for destructive actionsKeeping the menu open after Duplicate or Delete feels counter-intuitive and risks repeat activation. Unless UX research explicitly asked for this, flip the flag or call
closeMenuinside theselecthandler.- closeOnSelect: false, + closeOnSelect: true,(or invoke
closeMenu()manually).
blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts
Show resolved
Hide resolved
|
This PR is not ready. Still working on it. |
…, sort/root-panel.ts, view-options.ts): set minimum height for menu elements to improve UI consistency and usability
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
blocksuite/affine/data-view/src/core/group-by/setting.ts (5)
165-172: Still no guard when indices are-1Previous review flagged this; calling
moveGroupTowith-1will throw or corrupt state if drag context becomes stale.-const aIndex = groups.findIndex(g => g?.key === activeId); -const oIndex = groups.findIndex(g => g?.key === over.id); -if (aIndex > oIndex ? ... ) +const aIndex = groups.findIndex(g => g?.key === activeId); +const oIndex = groups.findIndex(g => g?.key === over.id); +if (aIndex === -1 || oIndex === -1) return; // stale drag context +this.groupTrait.moveGroupTo( + activeId, + aIndex > oIndex ? { before: true, id: over.id } : { before: false, id: over.id } +);
176-179: Duplicate key danger with constant'k'fallbackRe-using
'k'for undefined groups can collapse virtual-DOM sub-trees and break DnD. Either filter outundefinedfirst or generate a UUID.
404-406: Redundanttype === 'date'checkYou already bailed out when
!type, but not whentype !== 'date'; however all subsequent branches are inside this extra check, repeating earlier reviewer note.
430-434: Nested ternary still hurts readabilityExtract to a
weekModevariable as previously suggested.
601-606:columnIdprop still undeclared inGroupSettingYou pass
.columnId=${gProp.id}but@property accessor columnIdis missing. TS/ESLint will warn, and the value is discarded.
🧹 Nitpick comments (5)
blocksuite/affine/data-view/src/core/group-by/setting.ts (3)
35-51: Prefer mapping object over longswitchfordateModeLabelInline switches grow unwieldy as new modes are added and make i18n harder. A constant lookup table is clearer and O(1).
-const dateModeLabel = (key?: string) => { - switch (key) { - case 'date-relative': - return 'Relative'; - ... - default: - return ''; - } -}; +const DATE_MODE_LABEL: Record<string, string> = { + 'date-relative': 'Relative', + 'date-day': 'Day', + 'date-week-mon': 'Week', + 'date-week-sun': 'Week', + 'date-month': 'Month', + 'date-year': 'Year', +}; +const dateModeLabel = (key?: string) => DATE_MODE_LABEL[key ?? ''] ?? '';
64-93: Hard-coded scrollbar colours bypass theme vars
#b0b0b0ignores dark/light themes. Expose these viacssVarV2ordataViewCssVariable()so they react to theme changes.
325-330: Magic number550pxduplicated
minHeightis hard-coded in several popups; centralise as a CSS var or constant to keep look-&-feel consistent.blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts (2)
110-123: Repeated inline styling – extract menu sizing constantSetting
handler.menu.menuElement.style.minHeight = '550px'is duplicated across codebase. Consider a helper (e.g.applyDefaultMenuSizing(handler)) or a CSS class.
146-147: Same magic550pxvalue duplicated hereSee previous comment – centralising avoids divergence when the design token changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
blocksuite/affine/data-view/src/core/common/properties.ts(3 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(8 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts(3 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(4 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts(3 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- blocksuite/affine/data-view/src/core/common/properties.ts
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/sort/root-panel.ts
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts
- blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popupTargetFromElement(406-436)
…ableSimpleMenu for better positioning control fix(add-filter.ts, add-sort.ts, setting.ts): update popMenu calls to use subHandler for consistent menu behavior style(cell-renderer-css.ts): adjust dateValueContainer height for improved UI consistency chore(root-panel-view.ts): refactor filter menu to use new popMenu structure and improve readability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
blocksuite/affine/data-view/src/core/group-by/setting.ts (5)
165-172: Still no guard for-1indices when reordering
aIndex/oIndexcan be-1when groups mutate during DnD, propagating an invalid id tomoveGroupTo.
This was raised previously and remains unfixed.-const aIndex = groups.findIndex(g => g?.key === activeId); -const oIndex = groups.findIndex(g => g?.key === over.id); -this.groupTrait.moveGroupTo( - activeId, - aIndex > oIndex ? { before: true, id: over.id } : { before: false, id: over.id } -); +const aIndex = groups.findIndex(g => g?.key === activeId); +const oIndex = groups.findIndex(g => g?.key === over.id); +if (aIndex === -1 || oIndex === -1) return; // stale drag context +this.groupTrait.moveGroupTo( + activeId, + aIndex > oIndex ? { before: true, id: over.id } : { before: false, id: over.id } +);
175-179: Constant fallback key'k'can still collideUsing the same fallback key for every
undefinedgroup repeats the React-style key-collision bug.Consider the earlier suggestion (UUID or filter-out).
405-409: Redundanttype === 'date'guard persistsThe whole block is already inside the
datetype branch. Removing the ternary wrapper simplifies the array spread.
431-435: Nested ternary for week mode hurts readabilityPreviously flagged; still present.
Extract into a variable:
const weekMode = group.groupInfo$.value?.config.name === 'date-week-mon' ? 'date-week-mon' : 'date-week-sun'; ['Week', weekMode]
605-610:columnIdprop still not declared inGroupSettingThe element receives
.columnId=${gProp.id}but the class defines no@propertyfor it, continuing to raiseno-unknown-propertywarnings.export class GroupSetting extends SignalWatcher( WithDisposable(ShadowlessElement) ) { + @property({attribute:false}) accessor columnId?: string;
🧹 Nitpick comments (3)
blocksuite/affine/data-view/src/core/group-by/setting.ts (2)
35-51: Hard-coded English labels limit i18n flexibility
dateModeLabelreturns literal English strings. If the rest of the UI is localised, these literals will bypass the translation pipeline.-const dateModeLabel = (key?: string) => { +// TODO(i18n): Replace hard-coded literals with translation look-ups. +const dateModeLabel = (key?: string): string => {
195-202: Batch hide-all operation to avoid N reactive flushes
clickChangeAllloops over every group and callssetGroupHideindividually, triggering a signal update per group.
Wrap the updates in a single batch (e.g.untracked/batchutility) or add a bulk helper inGroupTraitto reduce unnecessary re-renders.blocksuite/affine/data-view/src/core/filter/add-filter.ts (1)
27-68: Avoid hard-coding the same inline style in every popup helperDirectly mutating
subHandler.menu.menuElement.style.minHeightto550pxworks, but the exact same statement now appears across multiplepop*helpers in this PR.Consider centralising this concern either:
- Extend
popMenuto accept aminHeight(and/ormaxHeight) option, or- Export a shared constant / utility that applies the sizing once.
This keeps the styling decision in a single place and avoids scattering magic numbers through the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
blocksuite/affine/components/src/context-menu/menu-renderer.ts(7 hunks)blocksuite/affine/data-view/src/core/filter/add-filter.ts(2 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(9 hunks)blocksuite/affine/data-view/src/core/sort/add-sort.ts(3 hunks)blocksuite/affine/data-view/src/property-presets/date/cell-renderer-css.ts(1 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/data-view/src/property-presets/date/cell-renderer-css.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- blocksuite/affine/data-view/src/core/sort/add-sort.ts
- blocksuite/affine/components/src/context-menu/menu-renderer.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/core/filter/add-filter.ts (1)
blocksuite/affine/components/src/context-menu/menu-renderer.ts (1)
popMenu(522-569)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts (1)
279-281: CSS valuefit-contentfor min/max-height is not universally supported
handler.menu.menuElement.style.minHeight = 'fit-content'(and the same formaxHeight) may be ignored in some browsers; the spec requires the functional notationfit-content(<length-percentage>)formin/max-height.If true cross-browser behaviour is required, consider:
elem.style.height = 'fit-content'; elem.style.minHeight = 'auto'; elem.style.maxHeight = 'auto';or providing a concrete pixel / percentage fallback.
blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts
Outdated
Show resolved
Hide resolved
… layout options feat(date-picker): add size property to date-picker for better customization feat(condition-view): enhance menu behavior by allowing dynamic sizing based on content
…iew render method to clean up code refactor(condition-view.ts): replace subMenuMiddleware with an array of middleware functions for better customization refactor(root-panel-view.ts): change popMenu placement to 'bottom-end' and adjust middleware for improved positioning and responsiveness
…ility feat(data-view): add onClose handler to properties setting for improved user experience feat(group-by): introduce hideEmpty property to manage visibility of empty groups fix(group-by): synchronize hideEmpty state with GroupBy data for consistency style(filter): set minimum width for filter menus to enhance layout stability fix(view-options.ts): restrict allowed placements in autoPlacement to 'bottom-start' for better UI consistency
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
blocksuite/affine/data-view/src/core/group-by/setting.ts (4)
165-173: Guard against stale drag indices
aIndex/oIndexcan be-1when the groups array mutates mid-drag, triggering an out-of-bounds insert. Add the early-exit proposed in the earlier review to prevent state corruption.
220-224:'k'fallback key duplicates
Using the constant key'k'still risks React/DOM subtree reuse bugs when multipleundefinedgroups exist. Generate a stable unique key (e.g.crypto.randomUUID()) or filter falsy items first.
405-435: Nested ternary + redundanttype === 'date'check remain
The readability / redundancy issues flagged in the previous round are still present here.
609-612:columnIdattribute still undeclared
GroupSettingdoesn’t declare acolumnIdaccessor, so Lit emitsno-unknown-propertywarnings. Either add@property({attribute:false}) accessor columnId?: string;or drop the attribute from the template.
blocksuite/affine/data-view/src/core/group-by/trait.ts (4)
163-168: Avoid mutations insidecomputed
view.propertyGetOrCreatecan create a property, violating computed-purity guarantees and triggering extra recomputations.
Switch toview.propertyGetinside the signal and ensure callers create the property beforehand.
205-208: State mutation during reactive computeCalling
cellGetOrCreatehere will allocate cells every time the signal re-evaluates. UsecellGetto keep the computation side-effect-free and move creation outside.
355-358: Persist viajsonValueSet, notvalueSetThe project convention (and other methods in this file) uses
jsonValueSetfor JSON payloads. UsingvalueSetrisks inconsistent caching.
512-518: Consistent setter requiredSame mismatch here – replace
valueSetwithjsonValueSetto stay consistent.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/core/common/types.ts (1)
5-5: Add brief JSDoc for the new flagA one-liner like
/** hide groups whose row count is zero */helps downstream maintainers understand the intent without scanning other files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
blocksuite/affine/components/src/context-menu/sub-menu.ts(7 hunks)blocksuite/affine/data-view/src/core/common/properties.ts(3 hunks)blocksuite/affine/data-view/src/core/common/types.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/default.ts(1 hunks)blocksuite/affine/data-view/src/core/group-by/setting.ts(9 hunks)blocksuite/affine/data-view/src/core/group-by/trait.ts(5 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts(5 hunks)blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts(5 hunks)blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- blocksuite/affine/data-view/src/core/common/properties.ts
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/condition-view.ts
- blocksuite/affine/data-view/src/widget-presets/quick-setting-bar/filter/root-panel-view.ts
- blocksuite/affine/components/src/context-menu/sub-menu.ts
- blocksuite/affine/data-view/src/widget-presets/tools/presets/view-options/view-options.ts
🔇 Additional comments (1)
blocksuite/affine/data-view/src/core/group-by/default.ts (1)
21-22: Re-evaluate the hard-codedhideEmpty: truedefaultFor existing workspaces this flips behaviour from “show” to “hide” empty groups without an explicit user choice.
Consider either:
- Leaving the flag
undefinedso the UI keeps previous default, or- Reading a user/tenant preference before opting-in.
Otherwise you risk reports of “my empty groups disappeared after upgrade”.
…idden to enhance group visibility management feat(table-view): implement message display when all groups are hidden to improve user experience style(table-view): add styles for the groups hidden message to ensure proper layout and visibility
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
blocksuite/affine/data-view/src/core/group-by/trait.ts (5)
139-146: Side-effect loop still possible – add an equality guardSame concern raised previously: writing to
sortAsc$inside theeffect
without checking current value can ping-pong with external updates.
159-168: Computed should stay pure – avoidpropertyGetOrCreateinsidecomputedCreating properties from inside a reactive computation mutates state and may
cause subtle recompute storms. Fetch withpropertyGet, and move the creation
logic to the caller.
205-207: Mutation inside reactive graph – replacecellGetOrCreatewithcellGet
groupDataMap$recomputes often; the current implementation can create a new
cell on every run, introducing hidden writes and cascading updates.
523-525: Repeat of value-vs-json setter mismatch
removeFromGroupshould align withjsonValueSetas well.- this.view.cellGetOrCreate(rowId, propId).valueSet(newValue); + this.view.cellGetOrCreate(rowId, propId).jsonValueSet(newValue);
360-365: Setter inconsistency (valueSetvsjsonValueSet) – can corrupt cachesEverywhere else JSON payloads are written with
jsonValueSet, but
addToGroupusesvalueSet, risking stale conversions.- this.view.cellGetOrCreate(rowId, info.property.id).valueSet(newVal); + this.view.cellGetOrCreate(rowId, info.property.id).jsonValueSet(newVal);Do the same for
removeFromGroup.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts (1)
158-162: Redundant filter & missed optional-chain – simplify the groups lookup
groupsDataList$already omitsundefined, so the extra filter is unnecessary.
Removing it also lets you adopt the optional-chain Biome hints pointed out:-const groups = this.logic.view.groupTrait.groupsDataList$.value?.filter( - (g): g is Group => g !== undefined -); +const groups = this.logic.view.groupTrait.groupsDataList$.value; -if (groups && groups.length) { +if (groups?.length) {This trims a few iterations and quiets the linter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocksuite/affine/data-view/src/core/group-by/trait.ts(5 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/table-view-style.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/data-view/src/view-presets/table/pc/table-view-style.ts
🧰 Additional context used
🪛 Biome (1.9.4)
blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| ${this.logic.view.groupTrait.allHidden$.value | ||
| ? html`<div class="${groupsHiddenMessageStyle}"> | ||
| All groups are hidden | ||
| </div>` | ||
| : this.renderTable()} |
There was a problem hiding this comment.
“All groups are hidden” message never appears for the hide-empty case
allHidden$ only inspects explicit hide flags, ignoring groups hidden
implicitly by hideEmpty$.
When every group is empty and hideEmpty$ === true, the UI falls back to an
empty group placeholder instead of the informative banner, confusing users.
See the proposed fix for allHidden$ in group-by/trait.ts.
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/view-presets/table/pc/table-view-ui-logic.ts
around lines 210 to 214, the condition for showing the "All groups are hidden"
message only checks the explicit hide flags via allHidden$, ignoring groups
hidden due to hideEmpty$. Update the logic to also consider the hideEmpty$ state
when determining if all groups are hidden, ensuring the message appears when all
groups are empty and hideEmpty$ is true, matching the fix suggested in
group-by/trait.ts.
| isGroupHidden(key: string): boolean { | ||
| return this.groupPropertiesMap$.value[key]?.hide ?? false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
isGroupHidden should honour hideEmpty$ for consistency
Right now it mirrors groupPropertiesMap only. Consider:
isGroupHidden(key: string): boolean {
- return this.groupPropertiesMap$.value[key]?.hide ?? false;
+ const g = this.groupDataMap$.value?.[key];
+ return g ? g.hide$.value : false;
}Ensures every consumer sees the same visibility semantics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isGroupHidden(key: string): boolean { | |
| return this.groupPropertiesMap$.value[key]?.hide ?? false; | |
| } | |
| isGroupHidden(key: string): boolean { | |
| const g = this.groupDataMap$.value?.[key]; | |
| return g ? g.hide$.value : false; | |
| } |
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/core/group-by/trait.ts around lines 307 to
309, the isGroupHidden method currently only checks groupPropertiesMap$ for the
hide property, but it should also consider the hideEmpty$ observable to maintain
consistent visibility semantics. Update the method to incorporate the hideEmpty$
state along with groupPropertiesMap$ so that the returned boolean reflects both
conditions, ensuring all consumers see the same group visibility behavior.
| allHidden$ = computed(() => { | ||
| const map = this.groupDataMap$.value; | ||
| if (!map) return false; | ||
| return Object.keys(map).every(key => this.isGroupHidden(key)); | ||
| }); |
There was a problem hiding this comment.
allHidden$ ignores hideEmpty$ – banner can mis-report state
The signal only checks explicit hides, so empty groups hidden by the
hideEmpty toggle aren’t counted.
-allHidden$ = computed(() => {
- const map = this.groupDataMap$.value;
- if (!map) return false;
- return Object.keys(map).every(key => this.isGroupHidden(key));
-});
+allHidden$ = computed(() => {
+ const groups = Object.values(this.groupDataMap$.value ?? {});
+ if (!groups.length) return false;
+ return groups.every(g => g?.hide$.value);
+});This leverages the existing hide$ logic, covering both explicit hides and
empty-group hides.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allHidden$ = computed(() => { | |
| const map = this.groupDataMap$.value; | |
| if (!map) return false; | |
| return Object.keys(map).every(key => this.isGroupHidden(key)); | |
| }); | |
| allHidden$ = computed(() => { | |
| const groups = Object.values(this.groupDataMap$.value ?? {}); | |
| if (!groups.length) return false; | |
| return groups.every(g => g?.hide$.value); | |
| }); |
🤖 Prompt for AI Agents
In blocksuite/affine/data-view/src/core/group-by/trait.ts around lines 289 to
293, the allHidden$ computed signal currently only checks explicit group hides
and ignores groups hidden due to the hideEmpty$ toggle, causing incorrect state
reporting. Update the logic to leverage the existing hide$ method or property
that accounts for both explicit hides and empty-group hides, ensuring all hidden
groups are correctly considered in the allHidden$ computation.
This PR adds the ability to group-by date with configuration which an example is shown in the image below:
Summary by CodeRabbit
New Features
Improvements