chore: enable no load by default #5968
Conversation
WalkthroughThe changes involve the removal of the Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (2)
web/core/components/workspace/sidebar/help-section.tsx (1)
114-126: Consider improving the Local Cache toggle UX.A few suggestions to enhance the user experience:
- Add a tooltip explaining what Local Cache does and its implications
- Consider extracting this into a separate component for better maintainability
Example implementation:
interface LocalCacheToggleProps { enabled: boolean; onToggle: () => void; } const LocalCacheToggle: React.FC<LocalCacheToggleProps> = ({ enabled, onToggle }) => ( <Tooltip tooltipContent="Enable local cache to improve performance by storing data offline"> <div onClick={(e) => { e.preventDefault(); e.stopPropagation(); }} className="flex w-full items-center justify-between text-xs hover:bg-custom-background-80" > <span className="tracking-tight">Local Cache</span> <ToggleSwitch value={enabled} onChange={onToggle} /> </div> </Tooltip> );web/core/store/user/index.ts (1)
280-280: Document the change in user settings.Consider adding a comment to explain that local DB caching is now controlled solely through user settings, making it clearer for future maintainers.
get localDBEnabled() { - return this.userSettings.canUseLocalDB; + // Local DB caching is controlled solely through user settings + // after removal of the ENABLE_LOCAL_DB_CACHE feature flag + return this.userSettings.canUseLocalDB; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/ce/constants/issues.ts(0 hunks)web/core/components/workspace/sidebar/help-section.tsx(2 hunks)web/core/store/user/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/ce/constants/issues.ts
🔇 Additional comments (3)
web/core/components/workspace/sidebar/help-section.tsx (2)
174-176: LGTM: Clean formatting improvement.
The class name formatting changes improve readability while maintaining the same functionality.
123-124: Verify removal of ENABLE_LOCAL_DB_CACHE constant.
Let's ensure there are no remaining references to the removed constant and that the toggle behavior works correctly.
✅ Verification successful
The search results show that the ENABLE_LOCAL_DB_CACHE constant has been completely removed, with no remaining references. The local DB functionality is now controlled through the user settings store with canUseLocalDB and localDBEnabled properties, which are properly used throughout the codebase. All code paths handle the toggle behavior correctly:
settings.store.tsmanages the state with proper initialization from local storagestorage.sqlite.tsandload-issues.tscheck the enabled state before operationsbase-issues.store.tshandles clearing based on the enabled state- The UI component properly uses the toggle through
useUserSettingshook
The removal of ENABLE_LOCAL_DB_CACHE constant is clean and well-handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to ENABLE_LOCAL_DB_CACHE
echo "Checking for remaining references to ENABLE_LOCAL_DB_CACHE..."
rg "ENABLE_LOCAL_DB_CACHE"
# Check for any conditional logic that might have depended on this constant
echo "Checking for related conditional logic..."
rg "localDBEnabled|canUseLocalDB"
Length of output: 2691
web/core/store/user/index.ts (1)
280-280: Verify the removal of the feature flag.
The simplified logic looks good, as it now solely relies on user preferences through userSettings.canUseLocalDB. However, let's verify that removing the ENABLE_LOCAL_DB_CACHE feature flag was intentional and won't impact existing users.
✅ Verification successful
Let me check the Dockerfile.dev to understand the context of local cache usage.
Based on the comprehensive search results, I can now provide the final response:
The removal of ENABLE_LOCAL_DB_CACHE is safe and intentional
The codebase shows that local DB functionality is properly managed through the user settings system:
- Local DB state is persisted in localStorage via
LOCAL_DB_ENABLEDkey - Default value is set to
trueinUserSettingsStore - The feature is consistently gated by
localDBEnabledgetter throughout the codebase - No other global feature flags are controlling this functionality
The change aligns with the PR objective to enable "no load" by default while maintaining user control through settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to ENABLE_LOCAL_DB_CACHE
# to ensure complete removal and identify potential impacts
echo "Checking for any remaining references to ENABLE_LOCAL_DB_CACHE..."
rg "ENABLE_LOCAL_DB_CACHE"
echo "Checking for any migration scripts or documentation about this feature flag..."
fd -t f "migration|changelog|readme|doc" -X rg -l "local.*cache|db.*cache"
Length of output: 438
Script:
#!/bin/bash
# Check the content of Dockerfile.dev for local cache references
cat ./apiserver/Dockerfile.dev
# Let's also check for any usage of canUseLocalDB in the codebase
rg "canUseLocalDB" -A 2
# Check for any configuration or constants related to local DB
rg -g "*.ts" -g "*.js" "localDB|LOCAL_DB" -A 2
Length of output: 8596
| </div> | ||
| </CustomMenu.MenuItem> | ||
| )} | ||
| <div className="my-1 border-t border-custom-border-200" />( |
There was a problem hiding this comment.
Fix syntax error: Remove unexpected parentheses.
There are unexpected parentheses on lines 112 and 128 that will cause a compilation error.
Apply this diff to fix the syntax error:
- <div className="my-1 border-t border-custom-border-200" />(
+ <div className="my-1 border-t border-custom-border-200" />
<CustomMenu.MenuItem>
...
</CustomMenu.MenuItem>
- )Also applies to: 128-128
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/components/workspace/sidebar/help-section.tsx (1)
121-121: Fix typo in className.There's a typo in the className.
-<span className="racking-tight">Local Cache</span> +<span className="tracking-tight">Local Cache</span>web/core/store/user/index.ts (1)
280-280: Document this breaking change.Since this change removes the feature flag and makes local DB caching controlled solely by user settings, it should be documented in the changelog or migration guide.
Would you like me to help draft the documentation for this breaking change?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/ce/constants/issues.ts(0 hunks)web/core/components/workspace/sidebar/help-section.tsx(2 hunks)web/core/store/user/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/ce/constants/issues.ts
🔇 Additional comments (3)
web/core/components/workspace/sidebar/help-section.tsx (2)
112-128: LGTM! Local cache toggle implementation looks good.
The implementation correctly handles the local cache toggle functionality with proper event handling to prevent menu closure on toggle. This aligns with the PR objective of enabling "no load" by default by making the toggle always accessible.
174-176: LGTM! Formatting changes look good.
The className template literal formatting changes improve readability while maintaining the same functionality.
web/core/store/user/index.ts (1)
280-280: LGTM! Please verify the impact on existing users.
The simplified implementation correctly relies on user settings for local DB caching control. However, since this is a breaking change that removes the feature flag, we should verify its impact.
Let's verify the usage of this getter across the codebase:
✅ Verification successful
The previous results show that localDBEnabled is used consistently across the codebase for feature checks, but we need to verify how canUseLocalDB is initialized and its default value to ensure existing users won't be affected.
The change is safe and correctly implemented
The verification shows that:
canUseLocalDBis initialized with a default value oftruefrom local storage (seegetValueFromLocalStorage(LOCAL_DB_ENABLED, true))- The feature is properly toggleable through user settings UI in the help section
- All usages of
localDBEnabledacross the codebase handle disabled state gracefully with early returns
This confirms that removing the feature flag is safe as:
- New users will have local DB enabled by default
- Existing users' preferences are preserved through local storage
- The codebase handles the feature toggle state consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to localDBEnabled to ensure they can handle the change
rg -A 3 "localDBEnabled"
# Description: Check for any remaining references to ENABLE_LOCAL_DB_CACHE
rg "ENABLE_LOCAL_DB_CACHE"
Length of output: 4756
Script:
#!/bin/bash
# Check for any references to canUseLocalDB in user settings
ast-grep --pattern 'canUseLocalDB'
# Check for migrations or default value settings
rg -A 3 "canUseLocalDB"
Length of output: 2783
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/helpers/issue.helper.ts (1)
313-317: Enhance function documentation for better maintainability.While the current JSDoc provides basic information, it could be more comprehensive.
Consider expanding the documentation:
/** - * This is to check if the issues list api should fall back to server or use local db - * @param queries - * @returns + * Determines whether the issues list API should bypass local DB and use server-side processing. + * + * @param queries - The query parameters for the issues list + * @param queries.expand - Optional expand parameter for additional data + * @param queries.group_by - Optional grouping parameter + * @param queries.mentions - Optional mentions filter + * @returns boolean - True if should use server, false if can use local DB + * + * @example + * const queries = { expand: "assignees", group_by: null }; + * const shouldUseServer = getIssuesShouldFallbackToServer(queries); // returns true */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/core/components/workspace/sidebar/help-section.tsx(2 hunks)web/core/services/issue/issue.service.ts(2 hunks)web/helpers/issue.helper.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/workspace/sidebar/help-section.tsx
🔇 Additional comments (3)
web/helpers/issue.helper.ts (1)
319-320: Verify the impact of expand query handling.
The condition !isEmpty(queries.expand as string) && !queries.group_by suggests that any expand query without grouping requires server processing.
Let's verify the usage of expand queries in the codebase:
✅ Verification successful
The previous command had a syntax error. Let's try different approaches to find expand query usage in the codebase, including direct string matches and AST-based search. This will help us understand how expand queries are handled across the codebase and verify their impact.
Expand query handling is correctly implemented for server-side processing
The codebase analysis confirms that the expand query handling is properly implemented. The condition in question is justified because:
-
Expand queries are used to fetch related data that requires server-side joins:
issue_reactions,issue_attachments,issue_link,parentfor issue detailsissue_relation,issue_relatedfor issue filtersissue_inboxfor inbox issues
-
The server fallback is necessary as these expanded relationships cannot be efficiently computed client-side, especially when no grouping is applied.
-
The implementation in
issue.service.tsspecifically handles this by using different API endpoints based on the expand query presence:
(queries.expand as string)?.includes("issue_relation") && !queries.group_by
? `/api/workspaces/${workspaceSlug}/projects/${projectId}/issues-detail/`
: `/api/workspaces/${workspaceSlug}/projects/${projectId}/issues/`🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for expand query usage in API calls
rg -A 3 'expand["\']?\s*:' --type ts
Length of output: 86
Script:
#!/bin/bash
# Search for expand query usage with different patterns
rg -A 3 "expand(\s*:|=)" --type ts
# Search for expand parameter in function signatures and type definitions
ast-grep --pattern 'expand: $_'
# Search for expand-related code
rg -A 5 "expand" --type ts
Length of output: 7181
web/core/services/issue/issue.service.ts (2)
13-13: LGTM: Clean import addition
The new helper function import follows the existing import organization pattern.
70-73: Verify fallback behavior implementation
The refactoring to use getIssuesShouldFallbackToServer helper improves code clarity. However, let's verify the implementation of this helper function to ensure it maintains the same fallback conditions.
✅ Verification successful
Helper function maintains correct fallback conditions
The implementation of getIssuesShouldFallbackToServer in web/helpers/issue.helper.ts correctly maintains the fallback logic by checking two specific conditions:
- Falls back to server when there's an expand query without grouping
- Falls back to server when mentions are present in the query
The helper function improves code readability while preserving the necessary fallback behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getIssuesShouldFallbackToServer helper
# Find and display the helper function implementation
ast-grep --pattern 'export function getIssuesShouldFallbackToServer($_) {
$$$
}'
# Search for any tests covering this helper
rg -l "getIssuesShouldFallbackToServer.*test"
Length of output: 132
Script:
#!/bin/bash
# Let's try different approaches to locate the helper function
# Search for the function name in the codebase
rg "getIssuesShouldFallbackToServer" -A 5
# Search for files containing the function name
fd . -t f -x grep -l "getIssuesShouldFallbackToServer" {}
# Try to find any imports of this function
rg "import.*getIssuesShouldFallbackToServer"
Length of output: 1887
| export const getIssuesShouldFallbackToServer = (queries: any) => { | ||
| // If there is expand query and is not grouped then fallback to server | ||
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | ||
| // If query has mentions then fallback to server | ||
| if (!isEmpty(queries.mentions)) return true; | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and input validation for the new function.
The function implementation looks correct for its purpose, but there are several improvements that could make it more robust:
Consider applying these improvements:
-export const getIssuesShouldFallbackToServer = (queries: any) => {
+interface IssueQueries {
+ expand?: string;
+ group_by?: string;
+ mentions?: string[];
+}
+
+export const getIssuesShouldFallbackToServer = (queries: IssueQueries): boolean => {
+ if (!queries) return false;
+
// If there is expand query and is not grouped then fallback to server
if (!isEmpty(queries.expand as string) && !queries.group_by) return true;
// If query has mentions then fallback to server
if (!isEmpty(queries.mentions)) return true;
return false;
};This refactor:
- Adds type safety by replacing
anywith a proper interface - Adds null check for the queries parameter
- Makes the return type explicit
📝 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.
| export const getIssuesShouldFallbackToServer = (queries: any) => { | |
| // If there is expand query and is not grouped then fallback to server | |
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | |
| // If query has mentions then fallback to server | |
| if (!isEmpty(queries.mentions)) return true; | |
| return false; | |
| }; | |
| interface IssueQueries { | |
| expand?: string; | |
| group_by?: string; | |
| mentions?: string[]; | |
| } | |
| export const getIssuesShouldFallbackToServer = (queries: IssueQueries): boolean => { | |
| if (!queries) return false; | |
| // If there is expand query and is not grouped then fallback to server | |
| if (!isEmpty(queries.expand as string) && !queries.group_by) return true; | |
| // If query has mentions then fallback to server | |
| if (!isEmpty(queries.mentions)) return true; | |
| return false; | |
| }; |
This PR enables no load by default in the repo
Summary by CodeRabbit
New Features
Bug Fixes
ENABLE_LOCAL_DB_CACHEconstant.Refactor
UserStorelogic related to local database configuration.