Nc canvas related issues #10711
Nc canvas related issues #10711
Conversation
|
Warning Rate limit exceeded@o1lab has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces changes to several components handling rollup columns and canvas rendering. In the rollup options component, a new computed property conditionally displays formatting options based on column type and metadata. The rollup cell renderer now builds its render properties conditionally when a formula-type column is detected, while other components update property access to use optional chaining. The image loader removes the cross-origin setting and calls its completion callback unconditionally, resulting in adjusted control flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Template as Template
participant Computed as enableFormattingOptions
participant Component as RollupOptions Component
Template->>Component: Trigger render/update
Component->>Computed: Evaluate computed property
Computed->>Computed: Check if related column exists and inspect display type
Computed-->>Component: Return true/false for formatting options
Component->>Template: Render precision selection and locale switch conditionally
sequenceDiagram
participant RCR as RollupCellRenderer
participant Child as ChildColumn
participant RP as RenderProps
participant RC as renderCell
RCR->>Child: Check if column type is Formula
alt Is Formula
RCR->>RP: Parse meta and update UIDT and additional metadata
else Not Formula
RCR->>RP: Initialize renderProps with default properties
end
RCR->>RC: Invoke renderCell with updated renderProps
Possibly related PRs
Suggested labels
Suggested reviewers
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:
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/grid/canvas/loaders/ImageLoader.ts (1)
138-138: Callback is now always triggered regardless of pending sprite count.The
onSettled()callback is now called unconditionally, removing the check forthis.pendingSprites === 0. This ensures the callback is always triggered when an image load completes, regardless of other pending sprites.Consider adding a comment explaining why the callback is now unconditionally invoked, as this represents a behavioral change from the previous implementation:
} finally { this.pendingSprites-- - this.onSettled?.() + // Always notify listeners when an image load attempt completes, regardless of pending sprites + this.onSettled?.() }packages/nc-gui/utils/columnUtils.ts (1)
384-403: New utility function for determining number formatting availability.This utility function improves formatting decisions for different column types, ensuring that number formatting is only applied to appropriate column/aggregation combinations.
The implementation correctly identifies restricted types and provides appropriate rules, but would benefit from documentation:
+/** + * Determines if number formatting should be available based on column type and aggregation function + * + * @param type - The UI type of the column + * @param aggregationFunction - The aggregation function being applied + * @returns Whether number formatting is available for this combination + */ const isNumberFormattingAvailable = (type: UITypes, aggregationFunction: string) => { const restrictedTypes = [ UITypes.Year, UITypes.Time, UITypes.Date, UITypes.DateTime, UITypes.CreatedTime, UITypes.LastModifiedTime, UITypes.Currency, UITypes.Duration, ] // For restricted types, only allow formatting for 'count' and 'countDistinct' if (restrictedTypes.includes(type)) { return ['count', 'countDistinct'].includes(aggregationFunction) } // For all other types, always allow formatting return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nc-gui/components/smartsheet/column/RollupOptions.vue(4 hunks)packages/nc-gui/components/smartsheet/grid/canvas/cells/Rollup.ts(2 hunks)packages/nc-gui/components/smartsheet/grid/canvas/composables/useCanvasRender.ts(2 hunks)packages/nc-gui/components/smartsheet/grid/canvas/index.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/canvas/loaders/ImageLoader.ts(1 hunks)packages/nc-gui/utils/columnUtils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (15)
packages/nc-gui/components/smartsheet/grid/canvas/index.vue (1)
1132-1132: Added optional chaining improves code robustness.The use of optional chaining (
meta.value?.synced) prevents potential null or undefined reference errors when accessing the synced property.packages/nc-gui/components/smartsheet/grid/canvas/loaders/ImageLoader.ts (1)
116-140:❓ Verification inconclusive
Image loading no longer sets cross-origin attribute.
The removal of the
img.crossOrigin = 'anonymous'setting means images are now loaded without the cross-origin attribute. This change might affect how cross-origin images are handled, potentially impacting security and rendering in certain scenarios.Check if this change could lead to CORS issues for certain image sources:
🏁 Script executed:
#!/bin/bash # Search for any references to handling cross-origin images or CORS issues in image loading rg -i -A 3 -B 3 "crossOrigin|cors|cross.?origin" --type tsLength of output: 15863
Attention: Potential CORS Impact on Image Loading
After verifying the codebase, the search confirms that while other parts of the project (e.g. in the Nuxt configuration) explicitly set the cross-origin attribute, the ImageLoader in question no longer applies
img.crossOrigin = 'anonymous'. This removal means that images loaded via this function will not carry a cross-origin attribute, which could lead to CORS issues if images are fetched from domains that require it (especially if they later need to be drawn to a canvas or processed further).
- Verify that images loaded by this loader are either from the same origin or are served with proper CORS headers.
- If cross-origin images are a possibility, consider reintroducing the crossOrigin assignment or exposing a configuration option to handle such cases.
packages/nc-gui/components/smartsheet/grid/canvas/composables/useCanvasRender.ts (2)
226-235: Added optional chaining improves code robustness.The use of optional chaining (
meta.value?.synced) prevents potential null or undefined reference errors when accessing the synced property.
457-466: Added optional chaining improves code robustness.The use of optional chaining (
meta.value?.synced) prevents potential null or undefined reference errors when accessing the synced property.packages/nc-gui/utils/columnUtils.ts (1)
420-420: Function correctly added to exports.The new utility function has been properly added to the module exports.
packages/nc-gui/components/smartsheet/column/RollupOptions.vue (5)
13-13: Added rfdc import for deep cloningGood addition of the
rfdc(Really Fast Deep Clone) library which will be useful for creating deep copies of objects without modifying the originals.
19-19: Initialized clone functionThe
clonefunction is properly instantiated from the imported rfdc library, allowing for efficient deep cloning throughout the component.
211-227: Added computed property to conditionally display formatting optionsThe new
enableFormattingOptionscomputed property correctly determines when number formatting options should be available based on the column type and rollup function. It handles formula columns properly by checking their display_type in metadata.This enhances the UX by only showing relevant options to users based on their column selection.
341-341: Conditional rendering of precision selectionGood improvement to conditionally show the precision form item only when number formatting is applicable to the selected column type and rollup function.
365-365: Conditional rendering of locale string optionThe locale string (thousands separator) option is now correctly shown only when applicable to the selected column type and rollup function, improving the user interface clarity.
packages/nc-gui/components/smartsheet/grid/canvas/cells/Rollup.ts (5)
27-28: Improved variable declaration for renderPropsChanged the variable declaration to allow for conditional assignment based on column type, which enables more sophisticated rendering logic to follow.
29-48: Enhanced formula column handlingAdded specialized handling for formula columns that properly preserves the display type and metadata from the formula configuration. This ensures that formula-based rollup columns render correctly according to their intended display format rather than just their raw type.
50-58: Added fallback renderProps initializationGood implementation of a fallback for non-formula columns that ensures renderProps is always properly initialized before use, preventing potential undefined errors.
60-60: Updated renderAsTextFun to use renderPropsModified to reference the uidt from renderProps.column instead of directly from childColumn, ensuring that any type transformations from formula handling are respected when determining the render function.
70-70: Updated renderCell call to use renderPropsThe renderCell call now correctly uses renderProps.column instead of childColumn, ensuring that all the column transformations (especially for formula columns) are respected during the actual rendering process.
|
Uffizzi Preview |
o1lab
force-pushed
the
nc-canvas-related-issues
branch
from
5723f97 to
a51beaf
Compare
o1lab
force-pushed
the
nc-canvas-related-issues
branch
from
a51beaf to
cb87bc1
Compare
Change Summary
fix: Canvas throwing error in console
fix: Rollup not inheriting formula formatting options
Closes https://github.com/nocodb/nocohub/issues/4730
Closes #10712
Change type