chore: oos changes #9919
chore: oos changes #9919
Conversation
dosubot
bot
added
the
size:XL
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve multiple components and files, primarily focusing on UI enhancements, modifications to caching mechanisms, and updates to method functionalities. Key updates include replacing the Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (12)
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: 3
🧹 Outside diff range and nitpick comments (14)
scripts/self-hosted-gh-runner/Dockerfile (3)
1-1: Consider pinning to a specific version instead of using 'latest'Using the
latesttag can lead to reproducibility issues and unexpected breaks. Consider pinning to a specific version for better stability and predictability.-FROM ghcr.io/actions/actions-runner:latest +FROM ghcr.io/actions/actions-runner:2.311.0
6-9: Consider pinning Python version and adding package versionsWhile the cleanup commands are good, consider:
- Pinning the Python version explicitly
- Creating a requirements.txt if any specific Python packages are needed
- Combining all apt-get commands to reduce layers
RUN apt-get update && apt-get install -y \ libnss3 libatk-bridge2.0-0 libdrm-dev libxkbcommon-dev libgbm-dev libasound-dev \ - libatspi2.0-0 libxshmfence-dev python3 python3-pip curl zip sudo rsync jq \ + libatspi2.0-0 libxshmfence-dev python3.9 python3.9-pip curl zip sudo rsync jq \ && apt-get clean && rm -rf /var/lib/apt/lists/*
17-17: Specify Playwright version and installation parametersThe Playwright installation could be more specific and reproducible:
- Pin the Playwright version
- Consider using
npminstead ofnpxfor better version control- Add
--browser chromiumflag explicitly since only Chromium is needed-RUN npx playwright install --with-deps chromium +RUN npm install -g @playwright/test@1.40.1 && \ + playwright install --browser chromium --with-depspackages/nc-gui/components/dlg/ColumnUpdateConfirm.vue (3)
18-24: Consider type-safe slot checking.While the slot-based conditional styling is good, consider using the
slotsobject for type-safe slot checking.-'mb-3': $slots['entity-preview'], +'mb-3': !!slots.entityPreview,
25-26: Enhance accessibility for warning message.Consider adding appropriate ARIA attributes to improve accessibility for this important warning message.
-<div class="flex item-center gap-2"> +<div class="flex item-center gap-2" role="alert" aria-live="polite">
Line range hint
34-50: Consider i18n for "Update" text.The button implementation looks good, but for consistency, consider using i18n for the "Update" text since
$tis already used for "Cancel".-Update +{{ $t('general.update') }}packages/nocodb/src/cache/NocoCache.ts (1)
Line range hint
6-24: Consider making the singleton pattern more explicit.While the current implementation works, using
thisin static methods can be confusing. Consider either:
- Making the singleton pattern explicit using a private constructor and getInstance() method, or
- Using the class name instead of
thisin static methods for better clarityExample implementation of option 1:
export default class NocoCache { private static instance: NocoCache; private client: CacheMgr; private cacheDisabled: boolean; private prefix: string; private constructor() { // initialization logic } public static getInstance(): NocoCache { if (!NocoCache.instance) { NocoCache.instance = new NocoCache(); } return NocoCache.instance; } public init() { this.cacheDisabled = (process.env.NC_DISABLE_CACHE || false) === 'true'; // ... rest of the init logic } // ... other methods become instance methods }Example implementation of option 2:
export default class NocoCache { private static client: CacheMgr; private static cacheDisabled: boolean; private static prefix: string; public static init() { NocoCache.cacheDisabled = (process.env.NC_DISABLE_CACHE || false) === 'true'; // ... use NocoCache instead of this NocoCache.prefix = `${CACHE_PREFIX}:${orgs}`; } // ... other methods use NocoCache instead of this }🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/nocodb-sdk/src/lib/import-export-data.ts (1)
139-149: LGTM! Consider consistent formatting for tab separator.The label updates improve clarity by explicitly showing the separator characters. For consistency with the other labels, consider updating the tab separator label to follow the same pattern.
{ - label: '<Tab>', + label: 'Tab (↹)', value: CsvColumnSeparator['tab'], },packages/nocodb/src/utils/globals.ts (2)
249-252: LGTM! Consider adding documentation.The implementation correctly handles environment variable configuration with a sensible default. This change improves system configurability and supports multi-tenant scenarios.
Consider adding a comment block above the constant to document:
- The purpose of cache prefixing
- Valid character restrictions (if any)
- Impact on existing cache entries when changed
- Example usage in different environments
+/** + * Prefix used for all cache keys in the system. + * Can be configured via NC_CACHE_PREFIX environment variable. + * + * @example + * // Development environment + * NC_CACHE_PREFIX=nc-dev + * + * // Production environment + * NC_CACHE_PREFIX=nc-prod + * + * // Multi-tenant setup + * NC_CACHE_PREFIX=tenant1 + */ export const CACHE_PREFIX = process.env.NC_CACHE_PREFIX && process.env.NC_CACHE_PREFIX.trim().length > 0 ? process.env.NC_CACHE_PREFIX : 'nc';
249-252: Consider adding validation for the cache prefix.The current implementation accepts any non-empty string as a prefix. Consider adding validation to:
- Restrict characters to a safe subset (e.g., alphanumeric and hyphens)
- Enforce a maximum length to prevent excessive cache key lengths
- Prevent common Redis key delimiters like ':' that might interfere with key parsing
Here's a suggested implementation:
+const validateCachePrefix = (prefix: string): boolean => { + // Allow alphanumeric characters and hyphens, max 32 chars + return /^[a-zA-Z0-9-]{1,32}$/.test(prefix); +}; + export const CACHE_PREFIX = process.env.NC_CACHE_PREFIX && process.env.NC_CACHE_PREFIX.trim().length > 0 - ? process.env.NC_CACHE_PREFIX + ? validateCachePrefix(process.env.NC_CACHE_PREFIX.trim()) + ? process.env.NC_CACHE_PREFIX.trim() + : 'nc' : 'nc';packages/nocodb/src/models/Integration.spec.ts (1)
Line range hint
109-132: Consider using dynamic test IDs instead of hardcoding.The test uses a hardcoded 'test-id' which matches the integration ID set in the test setup. While this works, consider using the integration's ID from the test context (
integration.id) to make the test more maintainable and less prone to errors if the setup changes.The conditions for checking deleted records have been restructured to be more consistent between EE and non-EE versions, which is good. However, the nested condition structure could be simplified further.
Consider this refactoring:
- 'test-id', + integration.id, null, isEE ? { _and: [ { _or: [ { deleted: { neq: true, }, }, { deleted: { eq: null, }, }, ], }, ], } - : { _or: [{ deleted: { neq: true } }, { deleted: { eq: null } }] }, + : { + _or: [ + { deleted: { neq: true } }, + { deleted: { eq: null } } + ] + },packages/nc-gui/components/smartsheet/header/Menu.vue (1)
419-422: LGTM! Consider adding JSDoc for better documentation.The implementation is correct and follows Vue's event handling patterns. Consider adding JSDoc to document the method's purpose and usage.
+/** + * Callback handler for column deletion that triggers a field reload event + */ const onDeleteColumn = () => { eventBus.emit(SmartsheetStoreEvents.FIELD_RELOAD) }packages/nc-gui/extensions/data-exporter/index.vue (2)
528-630: Enhance accessibility for the exports list.The current implementation could benefit from improved accessibility features:
Consider these improvements:
- Add aria-labels to status icons:
<GeneralIcon :icon="exp.status === JobStatus.COMPLETED ? 'circleCheckSolid' : 'alertTriangleSolid'" class="flex-none h-5 w-5" + :aria-label="jobStatusTooltip[exp.status]" :class="{ '!text-green-700': exp.status === JobStatus.COMPLETED, '!text-red-700': exp.status === JobStatus.FAILED, }" />
- Add keyboard navigation support:
<div v-if="exp.status === JobStatus.COMPLETED" class="flex" + role="button" + tabindex="0" + @keydown.enter="handleDownload(urlHelper(exp.result.url))" @click="handleDownload(urlHelper(exp.result.url))" >
- Add aria-live regions for status updates:
<div class="data-exporter-header sticky top-0 z-100" + aria-live="polite" > Recent Exports </div>
Line range hint
1-630: Consider architectural improvements for better maintainability and performance.While the implementation is solid, consider these architectural improvements:
Split the component into smaller, focused components:
ExportSettings.vueExportsList.vueExportItem.vueImplement virtual scrolling for the exports list to handle large datasets efficiently.
Use
v-memofor optimizing re-renders of static content in the export items:<template v-for="exp of exportedFiles" v-memo="[exp.id, exp.status, exp.result]"> <!-- export item template --> </template>Would you like me to provide a detailed implementation plan for these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/nc-gui/extensions/data-exporter/manifest.jsonis excluded by!**/*.jsonpackages/nc-gui/extensions/json-exporter/manifest.jsonis excluded by!**/*.jsonscripts/self-hosted-gh-runner/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (12)
packages/nc-gui/components/dlg/ColumnUpdateConfirm.vue(2 hunks)packages/nc-gui/components/smartsheet/header/Menu.vue(2 hunks)packages/nc-gui/composables/useExtensions.ts(2 hunks)packages/nc-gui/extensions/data-exporter/index.vue(3 hunks)packages/nocodb-sdk/src/lib/import-export-data.ts(1 hunks)packages/nocodb/src/cache/NocoCache.ts(2 hunks)packages/nocodb/src/cache/RedisCacheMgr.ts(2 hunks)packages/nocodb/src/cache/RedisMockCacheMgr.ts(2 hunks)packages/nocodb/src/models/Integration.spec.ts(1 hunks)packages/nocodb/src/modules/jobs/jobs.module.ts(2 hunks)packages/nocodb/src/utils/globals.ts(1 hunks)scripts/self-hosted-gh-runner/Dockerfile(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/nocodb/src/cache/NocoCache.ts
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (14)
packages/nocodb/src/cache/RedisMockCacheMgr.ts (2)
4-4: LGTM! Cache prefix standardization looks good.
The change to use CACHE_PREFIX from globals improves configuration flexibility and maintains consistency across cache implementations.
Also applies to: 17-17
17-17: Consider addressing the hardcoded organization value.
The hardcoded 'noco' value and the TODO comment about fetching organizations suggest this might need revisiting in the future. This could potentially cause issues when implementing multi-organization support.
Let's check if this pattern exists in other cache managers:
scripts/self-hosted-gh-runner/Dockerfile (2)
19-19: LGTM! Good security practice
Properly switches back to the non-root user after installations are complete.
1-19: Verify image size and test dependencies
While the Dockerfile looks functionally complete, consider:
- Verifying if all test dependencies are included
- Checking the final image size and potential optimizations
✅ Verification successful
Dependencies look complete, but consider optimizing image size
The Dockerfile includes all essential dependencies for running tests:
- Node.js and npm for JavaScript/TypeScript dependencies
- Python3 for Python-based tools
- Playwright with Chromium for browser testing
- System libraries for browser automation
Optimization suggestions:
- Combine
apt-getcommands into a single layer to reduce image size - Consider using multi-stage builds if build tools aren't needed at runtime
- Remove unnecessary package lists and caches in the same layer they're created
FROM ghcr.io/actions/actions-runner:latest
USER root
RUN apt-get update && apt-get install -y \
libnss3 libatk-bridge2.0-0 libdrm-dev libxkbcommon-dev libgbm-dev libasound-dev \
libatspi2.0-0 libxshmfence-dev python3 python3-pip curl zip sudo rsync jq \
&& curl -fsSL https://deb.nodesource.com/setup_18.x | bash - \
&& apt-get install -y nodejs \
&& apt-get clean && rm -rf /var/lib/apt/lists/* \
&& npx playwright install --with-deps chromium
USER runner🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential missing dependencies and image size optimization opportunities
# Look for any package.json files in the repo to identify potential missing dependencies
fd package.json --type f --exec jq -r '.dependencies + .devDependencies | keys[]' {} \; | sort -u
# Look for any requirements.txt files to identify potential Python dependencies
fd requirements.txt --type f --exec cat {} \; | sort -u
Length of output: 5408
packages/nocodb/src/cache/RedisCacheMgr.ts (2)
4-4: LGTM: Clean import addition
The import of CACHE_PREFIX aligns with the standardization of cache prefixing across the codebase.
26-26: Verify cache prefix consistency and consider migration impact
The change from hardcoded prefix to ${CACHE_PREFIX} could impact existing cached data. Ensure this change is coordinated with other cache managers and consider data migration needs.
Let's verify the consistent usage of CACHE_PREFIX across cache managers:
✅ Verification successful
Let me gather more information about the cache prefix usage and its implementation across these cache managers.
Let me check the actual definition of CACHE_PREFIX to complete the verification.
Cache prefix change is safe and consistent across implementations
The change from hardcoded prefix to ${CACHE_PREFIX} is safe because:
CACHE_PREFIXis properly defined inglobals.tswith a default value of 'nc', maintaining backward compatibility- Both
RedisCacheMgrandRedisMockCacheMgrconsistently use the sameCACHE_PREFIXimport - The prefix construction pattern
${CACHE_PREFIX}:${orgs}is identical across both implementations - The cache manager base class handles all cache operations using this prefix consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent cache prefix usage across cache managers
# and potential hardcoded values that might have been missed
# Check for cache prefix usage in cache managers
echo "Checking cache prefix usage in cache managers:"
rg -l "cache.*mgr" | xargs rg -l "CACHE_PREFIX"
# Look for potentially missed hardcoded 'nc:' prefixes in cache files
echo "Checking for hardcoded 'nc:' prefixes in cache-related files:"
rg "nc:['\"]" --type ts
# Check for other cache manager implementations
echo "List all cache manager implementations:"
fd -e ts -e js "cache.*mgr"
Length of output: 564
Script:
#!/bin/bash
# Check the implementation details of cache managers
echo "=== CacheMgr.ts ==="
cat packages/nocodb/src/cache/CacheMgr.ts
echo -e "\n=== RedisCacheMgr.ts ==="
cat packages/nocodb/src/cache/RedisCacheMgr.ts
echo -e "\n=== RedisMockCacheMgr.ts ==="
cat packages/nocodb/src/cache/RedisMockCacheMgr.ts
# Check for CACHE_PREFIX constant definition
echo -e "\n=== Searching for CACHE_PREFIX definition ==="
rg "CACHE_PREFIX.*=" --type ts
Length of output: 22497
Script:
#!/bin/bash
# Get the full content of globals.ts to see CACHE_PREFIX definition
cat packages/nocodb/src/utils/globals.ts
Length of output: 6795
packages/nc-gui/components/dlg/ColumnUpdateConfirm.vue (1)
13-16: LGTM! Clean modal implementation.
The migration to NcModal with proper header templating and configuration is well-structured.
packages/nocodb/src/cache/NocoCache.ts (1)
4-4: LGTM on the dynamic prefix, but verify the hardcoded 'noco' value.
The change to use CACHE_PREFIX improves flexibility. However, there's a TODO comment about implementing org fetching, while 'noco' remains hardcoded.
Let's verify if there are plans or existing implementations for org fetching:
Also applies to: 24-24
packages/nocodb/src/modules/jobs/jobs.module.ts (1)
56-56: 🛠️ Refactor suggestion
Consider queue prefix implications and optimize the assignment
- The dynamic prefix configuration could affect existing queued jobs when the prefix changes. Consider adding validation and migration steps.
- The template literal is unnecessary when CACHE_PREFIX isn't 'nc'.
- prefix: CACHE_PREFIX === 'nc' ? undefined : `${CACHE_PREFIX}`,
+ prefix: CACHE_PREFIX === 'nc' ? undefined : CACHE_PREFIX,Let's verify existing queue usage and potential impacts:
packages/nocodb/src/models/Integration.spec.ts (1)
Line range hint 1-289: Well-structured test suite with comprehensive coverage!
The test suite demonstrates good practices:
- Proper setup and teardown of test environment
- Comprehensive coverage of CRUD operations
- Testing of both encrypted and non-encrypted scenarios
- Clear separation of EE and non-EE test cases
packages/nc-gui/composables/useExtensions.ts (2)
451-451: LGTM! Sorting implementation handles undefined values correctly
The sorting logic appropriately uses the nullish coalescing operator with Infinity as a fallback, ensuring that extensions without an order property are placed at the end of the list.
35-36: Verify the impact on existing extensions
The addition of a required order property and the new sorting logic could affect existing extensions. Let's verify:
- The presence of
orderin existing extension manifests - Any documentation about this change for extension developers
Also applies to: 451-451
✅ Verification successful
Changes to extension manifest are safe to proceed
The verification shows that:
- Existing extensions already have the required
orderproperty:- data-exporter: order: 2
- json-exporter: order: 3
- The
disabledproperty is already in use (json-exporter hasdisabled: true)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing extension manifests for the order property
# Find and analyze all extension manifest files
echo "Checking extension manifests for 'order' property..."
fd -e json . packages/nc-gui/extensions -x sh -c '
if jq -e "has(\"order\")" {} > /dev/null; then
echo "✓ {} has order property"
else
echo "✗ {} missing order property"
fi
'
# Check if there's any migration documentation
echo -e "\nChecking for migration documentation..."
rg -l "order.*property" -g "*.md" docs/
Length of output: 645
Script:
#!/bin/bash
# Let's check the implementation details and any potential breaking changes
# Check for any extension-related interfaces or types
echo "Checking extension interfaces and types..."
rg -A 5 "interface.*Extension" .
# Look for extension loading or initialization code
echo -e "\nChecking extension loading mechanism..."
ast-grep --pattern 'function load$$$Extension($$$) {
$$$
}'
# Check for any migration or breaking changes documentation in the repository
echo -e "\nChecking for migration documentation in repository..."
rg -l "breaking.*changes.*extension" .
Length of output: 131631
Script:
#!/bin/bash
# Let's check the extension manifest files and any extension-related code
# Check for extension manifest files
echo "Checking extension manifest files..."
fd -e json . packages/nc-gui/extensions -x cat {}
# Look for extension-related code
echo -e "\nChecking extension-related code..."
rg -A 5 "interface.*Extension|type.*Extension" packages/nc-gui/composables/useExtensions.ts
# Check for any extension documentation
echo -e "\nChecking for extension documentation..."
fd -e md . -x grep -l "extension" {}
Length of output: 2057
packages/nc-gui/components/smartsheet/header/Menu.vue (1)
704-704: LGTM! Modal integration is correct.
The SmartsheetHeaderDeleteColumnModal component is properly integrated with both visibility control (v-model) and delete callback (on-delete-column).
packages/nc-gui/extensions/data-exporter/index.vue (1)
267-278: LGTM! Well-implemented header controls.
The implementation provides good user feedback through tooltips and proper handling of loading states.
|
Uffizzi Preview |
Signed-off-by: mertmit <mertmit99@gmail.com>
No description provided.