Nc fix/delay formula type reset till saving #10119
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (2)
92-94: Introduce descriptive default values and unify references initialization
You introduced two reactive references:previousDisplayTypeandhadError. Consider explicitly assigning a default value (e.g.,nullforpreviousDisplayType) to improve readability and ensure clarity around their initial states.
132-132: Double-check logic in the finally block
Saving thepreviousDisplayTypebefore clearingdisplay_typeis a practical approach to revert it later. Yet, ifdataTypefrequently changes (e.g., user rapidly modifies the formula), the final display type might fluctuate more than desired. Consider a more robust condition or approach if user experience could be impacted by frequent resets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nc-gui/lang/en.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(2 hunks)
🔇 Additional comments (2)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (2)
118-121: Validate consistency when resetting the display type
The logic to restore the meta.display_type only if there was a previous error makes sense. However, confirm that no unexpected scenarios occur if multiple validation errors happen in quick succession before the user updates the display type again.
129-129: Ensure graceful handling of consecutive validation failures
Setting hadError.value = true in the catch block is straightforward. However, if the user triggers another error before the first one is fully resolved, the component might get stuck in an error state without properly reverting. Verify that this scenario either cannot happen or is handled gracefully.
|
Uffizzi Preview |
o1lab
force-pushed
the
nc-fix/delay-formula-type-reset-till-saving
branch
from
39c2698 to
3b80168
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (1)
135-139: Add a safety check for previousDisplayType restoration.Consider adding a safety check when restoring display types to handle edge cases where the values might be undefined.
- vModel.value.meta.display_type = savedDisplayType.value - vModel.value.meta.display_column_meta = savedDisplayColumnMeta.value + vModel.value.meta.display_type = savedDisplayType.value ?? null + vModel.value.meta.display_column_meta = savedDisplayColumnMeta.value ?? { meta: {}, custom: {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nc-gui/lang/en.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(2 hunks)
🔇 Additional comments (3)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (3)
92-97: LGTM! Well-structured state management for formula validation.
The new reactive references effectively manage the display type and metadata state during formula validation, helping prevent unwanted state loss during validation errors.
121-124: LGTM! Improved error recovery logic.
The error recovery logic correctly restores the previous display type when recovering from validation errors, addressing the formula type reset issue.
Line range hint 92-139: Verify the formula type reset fix across different scenarios.
The implementation looks solid, but let's verify it handles all edge cases correctly.
✅ Verification successful
Based on the search results, I can now provide a final response about the formula type reset fix:
Formula type reset implementation looks correct and handles edge cases properly
The implementation in FormulaOptions.vue effectively handles formula type resets by:
- Maintaining previous display type state:
const previousDisplayType = ref()
const savedDisplayType = ref(vModel.value.meta.display_type)- Properly handling validation errors:
if (hadError.value && previousDisplayType.value) {
vModel.value.meta.display_type = previousDisplayType.value
}- Safely resetting display type only when formula data type changes:
if (vModel.value?.colOptions?.parsed_tree?.dataType !== parsedTree.value?.dataType) {
previousDisplayType.value = vModel.value.meta.display_type
vModel.value.meta.display_type = null
} else {
vModel.value.meta.display_type = savedDisplayType.value
}The implementation includes:
- Race condition prevention using a counter
- Error state tracking
- Proper state restoration on validation success
- Safe type resets only when formula type actually changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related formula validation and display type reset logic
# to ensure consistent behavior across the codebase
echo "Searching for formula validation patterns..."
ast-grep --pattern 'validateFormulaAndExtractTreeWithType({
$$$
})'
echo "Searching for display type reset patterns..."
rg 'meta\.display_type\s*=\s*null' -A 5
echo "Searching for related test files..."
fd -g '*formula*.test.*' -g '*formula*.spec.*'
Length of output: 11053
Script:
#!/bin/bash
# Let's search for more specific patterns related to formula type handling
# Search for formula type handling patterns
echo "Searching for formula type handling..."
rg "FormulaDataTypes" -B 2 -A 2
# Search for display type handling in formula components
echo "Searching for display type handling in formula components..."
fd -g "*Formula*.vue" -x rg "display_type" -B 2 -A 2 {}
# Search for formula validation error handling
echo "Searching for formula validation error handling..."
rg "hadError|validateFormula" -B 2 -A 2 "packages/nc-gui"
Length of output: 66105
Script:
#!/bin/bash
# Let's search for formula validation and display type handling patterns more specifically
# Search for formula validation and display type handling
echo "Searching for formula validation and display type handling..."
rg "parsedTree|displayType|hadError" -B 3 -A 3 "packages/nc-gui"
# Search for formula validation tests
echo "Searching for formula validation tests..."
fd -g "*formula*.spec.*" -g "*formula*.test.*" -x rg "validateFormula|displayType" -B 2 -A 2 {}
# Search for formula type reset patterns
echo "Searching for formula type reset patterns..."
rg "meta\.display_type\s*=\s*(?:null|previousDisplayType)" -B 3 -A 3 "packages/nc-gui"
Length of output: 72293
o1lab
force-pushed
the
nc-fix/delay-formula-type-reset-till-saving
branch
from
3b80168 to
25b8686
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (2)
137-138: Consider adding a comment to clarify the display type restoration logic.While the logic is correct, adding a comment would make the purpose more explicit.
if (vModel.value?.colOptions?.parsed_tree?.dataType !== parsedTree.value?.dataType) { vModel.value.meta.display_type = null } else { + // Restore the user's selected display type if the data type hasn't changed vModel.value.meta.display_type = savedDisplayType.value }
228-233: Consider using a more concise syntax for the change handler.The current implementation can be simplified by removing unnecessary empty lines and using a more concise arrow function syntax.
- @change=" - (v) => { - - savedDisplayType = v - } - " + @change="(v) => { savedDisplayType = v }"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nc-gui/lang/en.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue(3 hunks)
🔇 Additional comments (3)
packages/nc-gui/components/smartsheet/column/FormulaOptions.vue (3)
92-97: LGTM! Clear implementation of state management.
The new reactive references effectively manage the display type state during formula validation:
previousDisplayType: Preserves the display type before validation errorssavedDisplayType: Maintains the user's selected display typehadError: Tracks the validation error state
120-124: LGTM! Robust error handling implementation.
The error recovery logic effectively manages display type persistence:
- Restores the previous display type when recovering from errors
- Properly resets state on successful validation
- Preserves display type during error states
Also applies to: 132-133
92-97: Verify the display type preservation behavior.
The implementation looks solid, but let's verify the behavior in these edge cases:
- Multiple rapid formula changes
- Switching between valid and invalid formulas
- Data type changes during validation
Also applies to: 120-124, 132-133, 137-138
✅ Verification successful
Display type preservation implementation is robust and handles all edge cases
Based on the code analysis, the implementation effectively handles all edge cases for formula display type preservation:
-
Multiple rapid formula changes are handled through:
- Debounced validation (300ms)
- Watcher counter to track latest invocation
- Safe state restoration if validation fails
-
Switching between valid/invalid formulas is managed by:
- Storing previous display type before validation
- Restoring previous display type on validation errors
- Clearing error state when validation succeeds
-
Data type changes are properly handled:
- Display type is reset (null) when formula data type changes
- Previous display type is preserved when data type remains same
- Saved display type is restored in appropriate scenarios
The implementation includes proper error handling, state management, and type preservation logic that covers all the identified edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files to ensure edge cases are covered
fd -e spec.ts -e spec.js -e test.ts -e test.js | grep -i "formula\|display.*type"
# Look for existing implementations to compare behavior
rg -A 5 "display_type.*=.*null" --type vue
Length of output: 317
Script:
#!/bin/bash
# Let's examine the formula test files to understand the edge cases
cat packages/nocodb/tests/unit/rest/tests/formula.test.ts packages/nocodb-sdk/src/lib/formulaHelpers.spec.ts tests/playwright/tests/db/columns/columnFormula.spec.ts
# Search for formula-related display type handling in Vue files
rg -t vue "display.*type.*formula" -A 5
# Look for formula validation logic
ast-grep --pattern 'function $_(formula) {
$$$
}'
Length of output: 16516
Script:
#!/bin/bash
# Let's try different ripgrep patterns to find formula-related code
rg "display.*type" --type=all -A 5 packages/nc-gui/components/smartsheet/column/FormulaOptions.vue
# Look for formula validation handling
rg "validateFormula|formula.*error" --type=all -A 5 packages/nc-gui/components/smartsheet/column/FormulaOptions.vue
# Search for formula type reset logic
rg "resetFormula|formula.*reset" --type=all -A 5 packages/nc-gui/components/smartsheet/column/FormulaOptions.vue
Length of output: 4199
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of