feat: improved csv export #11429
feat: improved csv export #11429
Conversation
📝 WalkthroughWalkthroughThe changes refactor the data export functionality from direct, synchronous, controller-based endpoints to an asynchronous, job-based export flow. Legacy controllers and their tests for direct Excel/CSV export are removed, and a new controller is introduced to initiate export jobs via a public POST endpoint. The frontend is updated to use this job-based export with polling, and related OpenAPI path definitions for the old endpoints are deleted. Type annotations and module registrations are updated to reflect these changes. Additionally, a new recurring cleanup job for exported data files is added with its processor and scheduling in both fallback and redis job services. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API Server
participant Jobs Service
User->>Frontend: Click "Export CSV"
Frontend->>API Server: POST /api/v2/public/export/:publicDataUuid/csv (with options)
API Server->>Jobs Service: Create export job
Jobs Service-->>API Server: Return job ID
API Server-->>Frontend: Respond with job ID
loop Polling
Frontend->>API Server: GET /api/v2/jobs/:jobId/status
API Server-->>Frontend: Return job status (pending, running, finished)
end
Frontend->>API Server: GET /api/v2/jobs/:jobId/download (when finished)
API Server-->>Frontend: Return download URL
Frontend->>User: Trigger file download
Legacy (Removed) Flow Comparison: sequenceDiagram
participant User
participant Frontend
participant API Server
User->>Frontend: Click "Export CSV"
Frontend->>API Server: GET /api/v1/db/data/{...}/export/csv
API Server-->>Frontend: Stream/export file directly (possibly paginated)
Frontend->>User: Trigger file download
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🧪 Generate Unit Tests
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
packages/nocodb/src/modules/jobs/jobs.module.ts (2)
26-27: Missing tree-shaking / circular-dependency safeguard for new import
PublicDataExportControlleris imported directly from a deep path inside the same module tree.
If the controller (or any of its dependencies) importsJobsModuleback, you’ll create an implicit circular reference that NestJS silently ignores at runtime but breaks Jest tests and causes providers to beundefined.Although I don’t see such a cycle now, it’s a common regression point when new decorators/guards are added later.
Consider:
-import { PublicDataExportController } from '~/modules/jobs/jobs/data-export/public-data-export.controller'; +import { PublicDataExportController } from '~/modules/jobs/jobs/data-export/public-data-export.controller'; // keep +// TODO: if this controller ever needs JobsModule injectables, wrap the import with forwardRef or move to a +// dedicated sub-module to avoid accidental circular refs.
78-90: Controller registration is skipped on worker containers – verify feature availability
PublicDataExportControlleris only mounted whenprocess.env.NC_WORKER_CONTAINER !== 'true'.
That matches the convention used for other “API-only” controllers, but it means:
- CLI / worker deployments will not expose the new
/api/v2/public/export/...endpoint.- The frontend (which now always calls this endpoint) will fail if it happens to talk to a worker pod behind a load balancer.
Please double-check the deployment topology:
- If worker containers are never routed HTTP traffic, this is fine.
- If they can receive traffic, move the controller to the always-on list or route traffic away from workers.
- DataExportController, - PublicDataExportController, + DataExportController, + // Public endpoint must be reachable from any web pod + PublicDataExportController,packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts (1)
51-54: Password header case-sensitivity can lock out legitimate callersNode HTTP headers are always lower-cased, but proxies/CDNs may rewrite them.
req.headers?.['xc-password']is fine, yet some clients still sendXC-Password.
Add a fallback and trim whitespace:- if (view.password && view.password !== req.headers?.['xc-password']) { +const headerPwd = (req.headers?.['xc-password'] || req.headers?.['xc-password'.toUpperCase()] || '').toString().trim() +if (view.password && view.password !== headerPwd) { NcError.invalidSharedViewPassword(); }packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue (2)
41-49: SingleisExportingflag blocks concurrent exports and leaks UI stateIf a user clicks twice quickly or switches views, the flag remains true and disables new exports, even though each job is independent.
Prefer tracking export state per
jobIdor resetting on route change:-const isExporting = ref(false) +const exportingJobs = ref<Set<string>>(new Set()) // when starting - isExporting.value = true + exportingJobs.value.add(jobData.id) // when completed / failed + exportingJobs.value.delete(jobData.id)This avoids UX dead-ends.
Also remember toonUnmounted(() => $poller.unsubscribeAll())to prevent memory leaks when the toolbar is destroyed.
20-39: Download helper lacks fallback for CORS / Same-Site cookiesSome deployments serve the file from a different domain (e.g., S3 presigned URL).
If cookies are required (same-site), the hidden anchor trick fails silently in Safari/Firefox.Consider using
window.open(url, '_blank')as a last resort whenfetch(url, { method: 'HEAD' })returns 403/0.Not blocking, but improves cross-browser UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
packages/nocodb/src/schema/swagger-v2.jsonis excluded by!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue(2 hunks)packages/nc-gui/plugins/api.ts(2 hunks)packages/nocodb/src/controllers/data-alias-export.controller.spec.ts(0 hunks)packages/nocodb/src/controllers/data-alias-export.controller.ts(0 hunks)packages/nocodb/src/controllers/public-datas-export.controller.spec.ts(0 hunks)packages/nocodb/src/controllers/public-datas-export.controller.ts(0 hunks)packages/nocodb/src/modules/jobs/jobs.module.ts(2 hunks)packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts(1 hunks)packages/nocodb/src/modules/noco.module.ts(0 hunks)packages/nocodb/src/services/api-docs/swagger/templates/paths.ts(0 hunks)
💤 Files with no reviewable changes (6)
- packages/nocodb/src/controllers/data-alias-export.controller.spec.ts
- packages/nocodb/src/controllers/public-datas-export.controller.spec.ts
- packages/nocodb/src/modules/noco.module.ts
- packages/nocodb/src/services/api-docs/swagger/templates/paths.ts
- packages/nocodb/src/controllers/data-alias-export.controller.ts
- packages/nocodb/src/controllers/public-datas-export.controller.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/nc-gui/plugins/api.ts (1)
packages/nocodb-sdk/src/lib/Api.ts (1)
Api(4791-14704)
| import type { Api } from 'nocodb-sdk' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loss of precise typing – we now fall back to Api<any>
ReturnType<typeof createApiInstance> preserved the real generic argument (SecurityDataType) so $api methods returned strongly-typed payloads.
With
$api: Api<any>we lose compile-time validation of:
- request/response bodies
- union discriminations (e.g.
ExportTypes)
Unless there was a cyclic-import problem, prefer keeping the specific type:
-import type { Api } from 'nocodb-sdk'
+import type { Api } from 'nocodb-sdk'
+import type { createApiInstance } from '@/composables/useApi' // adjust path
- $api: Api<any>
+ $api: ReturnType<typeof createApiInstance>(or expose a typed Api<SecurityData> alias from useApi).
Restoring strong typing prevents silent breakages when backend signatures change.
Also applies to: 12-13
🤖 Prompt for AI Agents
In packages/nc-gui/plugins/api.ts around lines 1 to 2 and also lines 12 to 13,
the type of $api is currently set to Api<any>, which loses the precise generic
typing and disables compile-time validation of request/response bodies and
discriminated unions. To fix this, replace Api<any> with the specific generic
type that was originally inferred by ReturnType<typeof createApiInstance>, such
as Api<SecurityDataType>, or alternatively expose and use a typed Api alias from
useApi. This will restore strong typing and prevent silent breakages from
backend changes.
| if (!view) NcError.viewNotFound(publicDataUuid); | ||
| if ( | ||
| view.type !== ViewTypes.GRID && | ||
| view.type !== ViewTypes.KANBAN && | ||
| view.type !== ViewTypes.GALLERY && | ||
| view.type !== ViewTypes.CALENDAR && | ||
| view.type !== ViewTypes.MAP | ||
| ) | ||
| NcError.notFound('Not found'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated null-check & verbose view-type guard
-
if (!view) NcError.viewNotFound(...)appears twice (lines 41 and 60).
Remove the second occurrence to avoid dead code and misleading coverage metrics. -
The allowed-view filter enumerates every permitted
ViewTypesvalue:
if (
view.type !== ViewTypes.GRID &&
view.type !== ViewTypes.KANBAN && …
)That is error-prone when new view types are added. Use includes:
-if ( … long chain … ) NcError.notFound('Not found')
+const ALLOWED_TYPES = [ViewTypes.GRID, ViewTypes.KANBAN, ViewTypes.GALLERY, ViewTypes.CALENDAR, ViewTypes.MAP]
+if (!ALLOWED_TYPES.includes(view.type)) {
+ NcError.notFound('Unsupported view type')
+}🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts
between lines 41 and 49, remove the duplicated null-check for `view` found again
at line 60 to eliminate redundant code. Replace the verbose multiple `view.type
!==` checks with a single condition that uses an array of allowed `ViewTypes`
and the `includes` method to check if `view.type` is valid, improving
maintainability and reducing errors when new view types are added.
| // check if download is allowed | ||
| if (!view.meta?.allowCSVDownload) { | ||
| NcError.forbidden('Download is not allowed for this view'); | ||
| } |
There was a problem hiding this comment.
CSV-only permission check but endpoint allows XLSX/JSON
allowCSVDownload is enforced for every export, but exportAs accepts 'json' | 'xlsx'.
Either:
- Rename the meta field to a generic
allowDownload, or - Restrict
exportAs === 'csv'whenallowCSVDownloadistrue.
Current behavior blocks XLSX/JSON even when admins intended them to be allowed.
🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts
around lines 55 to 58, the permission check uses allowCSVDownload for all export
types, but the exportAs parameter supports 'json' and 'xlsx' as well. To fix
this, either rename allowCSVDownload to a generic allowDownload to cover all
export types, or add a condition to enforce allowCSVDownload only when exportAs
is 'csv', allowing other formats without this restriction.
| @Post(['/api/v2/public/export/:publicDataUuid/:exportAs']) | ||
| @HttpCode(200) | ||
| async exportModelData( | ||
| @TenantContext() context: NcContext, | ||
| @Req() req: NcRequest, | ||
| @Param('publicDataUuid') publicDataUuid: string, | ||
| @Param('exportAs') exportAs: 'csv' | 'json' | 'xlsx', | ||
| @Body() options: DataExportJobData['options'], | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing DTO & validation for options
@Body() options: DataExportJobData['options'] arrives unchecked.
For a public endpoint this is risky (DoS via huge limits, malformed filters).
Create a class with class-validator decorators and apply @Validate():
export class ExportOptionsDto {
@IsOptional()
@IsInt() @Min(1) @Max(100000)
rowLimit?: number
… // other fields
}
@Post('/api/v2/public/export/:publicDataUuid/:exportAs')
async exportModelData(
…
@Body() options: ExportOptionsDto,
)🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts
around lines 30 to 38, the @Body() parameter 'options' is currently typed but
not validated, which poses a security risk for this public endpoint. To fix
this, create a DTO class using class-validator decorators that define and
constrain the expected fields (e.g., rowLimit with @IsInt, @Min, @Max). Then
update the controller method to accept this DTO type and apply validation by
using the appropriate validation pipe or decorator to ensure incoming data is
checked before processing.
| const job = await this.jobsService.add(JobTypes.DataExport, { | ||
| context, | ||
| options, | ||
| modelId: view.fk_model_id, | ||
| viewId: view.id, | ||
| user: req.user, | ||
| exportAs, | ||
| ncSiteUrl: req.ncSiteUrl, | ||
| }); |
There was a problem hiding this comment.
Potential NPE: req.user is undefined for public shares
Public export calls are anonymous; req.user is undefined here but jobsService.add (and downstream processors) often assume a user object with at least id / name.
Protect against it or pass a synthetic “public-share” user:
- user: req.user,
+ user: req.user ?? { id: 0, name: 'public-share' },Alternatively make the field optional in job payload typings.
📝 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.
| const job = await this.jobsService.add(JobTypes.DataExport, { | |
| context, | |
| options, | |
| modelId: view.fk_model_id, | |
| viewId: view.id, | |
| user: req.user, | |
| exportAs, | |
| ncSiteUrl: req.ncSiteUrl, | |
| }); | |
| const job = await this.jobsService.add(JobTypes.DataExport, { | |
| context, | |
| options, | |
| modelId: view.fk_model_id, | |
| viewId: view.id, | |
| user: req.user ?? { id: 0, name: 'public-share' }, | |
| exportAs, | |
| ncSiteUrl: req.ncSiteUrl, | |
| }); |
🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts
around lines 62 to 70, the code passes req.user directly to jobsService.add, but
req.user can be undefined for public shares causing potential null pointer
errors. To fix this, check if req.user is undefined and if so, pass a synthetic
user object with default id and name values representing a "public-share" user.
This ensures downstream code always receives a valid user object. Alternatively,
update the job payload typings to make the user field optional if that fits the
design better.
| message.info('Preparing CSV for download...') | ||
|
|
||
| $poller.subscribe( | ||
| { id: jobData.id }, | ||
| async (data: { | ||
| id: string | ||
| status?: string | ||
| data?: { | ||
| error?: { | ||
| message: string | ||
| } | ||
| message?: string | ||
| result?: any | ||
| } | ||
| }) => { | ||
| if (data.status !== 'close') { | ||
| if (data.status === JobStatus.COMPLETED) { | ||
| // Export completed successfully | ||
| message.info('Successfully exported data!') | ||
|
|
||
| handleDownload(data.data?.result?.url) | ||
|
|
||
| isExporting.value = false | ||
| } else if (data.status === JobStatus.FAILED) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsubscribe from poller to avoid endless replicas
$poller.subscribe returns an unsubscribe handle, but it’s ignored.
Leaving subscriptions active after navigation/spa reloads keeps sockets open and triggers duplicate callbacks.
const unsub = $poller.subscribe(…)
onUnmounted(unsub)or if the component persists, call unsub() inside the completion/failed branch once the job ends.
🤖 Prompt for AI Agents
In packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue between
lines 59 and 82, the subscription to $poller is not unsubscribed, causing
potential memory leaks and duplicate callbacks. Fix this by capturing the
unsubscribe function returned by $poller.subscribe and either register it with
onUnmounted to clean up on component unmount or call the unsubscribe function
explicitly inside the completion and failure branches to stop polling once the
job finishes.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts (1)
228-233: 🛠️ Refactor suggestionOff-by-one: first element (index 0) is never removed
if (fIndex)skips removal whenfIndex === 0.-if (fIndex) { +if (fIndex > -1) {Although not part of the new diff, it is triggered far more often with recurring jobs; fixing it prevents unbounded growth.
♻️ Duplicate comments (1)
packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue (1)
59-89:⚠️ Potential issueUnsubscribe from poller to avoid memory leaks
The
$poller.subscribemethod returns an unsubscribe function, but it's never called. This can lead to memory leaks and duplicate callbacks if the component is unmounted without cleaning up the subscription.Apply this fix:
- $poller.subscribe( + const unsubscribe = $poller.subscribe( { id: jobData.id }, async (data: { id: string status?: string data?: { error?: { message: string } message?: string result?: any } }) => { if (data.status !== 'close') { if (data.status === JobStatus.COMPLETED) { // Export completed successfully message.info('Successfully exported data!') handleDownload(data.data?.result?.url) isExporting.value = false + unsubscribe() } else if (data.status === JobStatus.FAILED) { message.error('Failed to export data!') isExporting.value = false + unsubscribe() } } }, )Also add this to ensure cleanup on component unmount:
onUnmounted(() => { // If the component is unmounted while a job is still running if (unsubscribe) { unsubscribe() } })
🧹 Nitpick comments (2)
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts (2)
15-16: Single-threaded queue may throttle large clean-ups
new PQueue({ concurrency: 1 })guarantees safety but can be extremely slow when tens of thousands of files accumulate.
Unless the storage adapter is known to have concurrency limits, consider a higher concurrency or make it configurable (e.g. env varNC_CLEANUP_CONCURRENCY, default = 4).
67-71: String comparison relies on lexicographic ordering – be explicitThe
<operator on strings works here only because the format isYYYY-MM-DD.
For readability and to avoid future surprises if the format changes, usemoment(folderDate).isBefore(cutoffDate, 'day').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
packages/nocodb/src/schema/swagger-v2.jsonis excluded by!**/*.jsonpackages/nocodb/src/schema/swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (16)
packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue(2 hunks)packages/nc-gui/plugins/api.ts(2 hunks)packages/nocodb/src/controllers/data-alias-export.controller.spec.ts(0 hunks)packages/nocodb/src/controllers/data-alias-export.controller.ts(0 hunks)packages/nocodb/src/controllers/public-datas-export.controller.spec.ts(0 hunks)packages/nocodb/src/controllers/public-datas-export.controller.ts(0 hunks)packages/nocodb/src/interface/Jobs.ts(1 hunks)packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts(5 hunks)packages/nocodb/src/modules/jobs/fallback/jobs.service.ts(1 hunks)packages/nocodb/src/modules/jobs/jobs-map.service.ts(3 hunks)packages/nocodb/src/modules/jobs/jobs.module.ts(3 hunks)packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts(1 hunks)packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts(1 hunks)packages/nocodb/src/modules/jobs/redis/jobs.service.ts(1 hunks)packages/nocodb/src/modules/noco.module.ts(0 hunks)packages/nocodb/src/services/api-docs/swagger/templates/paths.ts(0 hunks)
💤 Files with no reviewable changes (6)
- packages/nocodb/src/controllers/public-datas-export.controller.spec.ts
- packages/nocodb/src/controllers/data-alias-export.controller.spec.ts
- packages/nocodb/src/modules/noco.module.ts
- packages/nocodb/src/services/api-docs/swagger/templates/paths.ts
- packages/nocodb/src/controllers/data-alias-export.controller.ts
- packages/nocodb/src/controllers/public-datas-export.controller.ts
✅ Files skipped from review due to trivial changes (1)
- packages/nocodb/src/interface/Jobs.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nocodb/src/modules/jobs/jobs.module.ts
- packages/nc-gui/plugins/api.ts
- packages/nocodb/src/modules/jobs/jobs/data-export/public-data-export.controller.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts (5)
packages/nocodb/src/modules/jobs/jobs-map.service.ts (1)
Injectable(16-91)packages/nocodb/src/modules/jobs/fallback/jobs.service.ts (1)
Injectable(14-165)packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts (3)
Injectable(18-235)Job(9-16)queue(92-94)packages/nocodb/src/modules/jobs/redis/jobs.service.ts (1)
Injectable(19-211)packages/nocodb/src/helpers/NcPluginMgrv2.ts (1)
storageAdapter(185-212)
packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts (1)
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts (1)
job(12-109)
⏰ 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 (6)
packages/nc-gui/components/smartsheet/toolbar/ExportSubActions.vue (2)
20-39: Link creation and expired link handling looks goodThe
handleDownloadfunction properly manages both expired and valid download links, using appropriate techniques to trigger downloads and fallback to direct navigation when needed.
43-58: Export job implementation looks goodThe refactoring from direct data export to an asynchronous job-based approach is well implemented. The code now properly initiates export jobs through the API and waits for completion.
packages/nocodb/src/modules/jobs/redis/jobs.service.ts (1)
31-42: Recurring cleanup job implementation looks goodThe data export cleanup job is scheduled to run every 5 hours using the correct cron expression. Using the job type as the job ID ensures there's only one instance of this cleanup job running.
packages/nocodb/src/modules/jobs/jobs-map.service.ts (3)
13-13: Good job adding the data export cleanup processor importThe import for the new cleanup processor is correctly added.
30-30: Processor dependency properly injectedThe cleanup processor is correctly injected as a protected readonly dependency in the constructor.
70-72: Job mapping correctly implementedThe DataExportCleanUp job type is properly mapped to its processor, following the existing pattern.
| await this.fallbackQueueService.add( | ||
| JobTypes.DataExportCleanUp, | ||
| {}, | ||
| { | ||
| jobId: JobTypes.DataExportCleanUp, | ||
| // run every 5 hours | ||
| repeat: { cron: '* * * * *' }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Fix incorrect cron expression for cleanup job
The comment states "run every 5 hours" but the cron expression * * * * * will run the job every minute, potentially causing excessive load and unnecessary file system operations.
Apply this fix to match the implementation in the Redis service:
await this.fallbackQueueService.add(
JobTypes.DataExportCleanUp,
{},
{
jobId: JobTypes.DataExportCleanUp,
// run every 5 hours
- repeat: { cron: '* * * * *' },
+ repeat: { cron: '0 */5 * * *' },
},
);📝 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.
| await this.fallbackQueueService.add( | |
| JobTypes.DataExportCleanUp, | |
| {}, | |
| { | |
| jobId: JobTypes.DataExportCleanUp, | |
| // run every 5 hours | |
| repeat: { cron: '* * * * *' }, | |
| }, | |
| ); | |
| await this.fallbackQueueService.add( | |
| JobTypes.DataExportCleanUp, | |
| {}, | |
| { | |
| jobId: JobTypes.DataExportCleanUp, | |
| // run every 5 hours | |
| repeat: { cron: '0 */5 * * *' }, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In packages/nocodb/src/modules/jobs/fallback/jobs.service.ts between lines 19
and 27, the cron expression for the DataExportCleanUp job is set to '* * * * *',
which runs every minute instead of every 5 hours as intended. Update the cron
expression to run every 5 hours by changing it to '0 */5 * * *' to align with
the comment and prevent excessive job executions.
| import type { Job } from 'bull'; | ||
| import NcPluginMgrv2 from '~/helpers/NcPluginMgrv2'; |
There was a problem hiding this comment.
Type imported from bull will not match fallback/jobs Job interface
Job is imported from Bull but this processor is executed by both the Redis queue (Bull) and the in-memory fallback queue, which passes its own Job shape (see fallback-queue.service.ts).
The structural mismatch will surface as a compile-time error in strict mode and makes future refactors harder.
-import type { Job } from 'bull';
+// Use a union to support both Bull & fallback implementations
+import type { Job as BullJob } from 'bull';
+import type { Job as MemoryJob } from '~/modules/jobs/fallback/fallback-queue.service';
+
+type AnyJob = BullJob | MemoryJob;and update the signature:
- async job(job: Job) {
+ async job(job: AnyJob) {🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts
around lines 5 to 6, the imported Job type from Bull does not match the Job
interface used by the fallback in-memory queue, causing type conflicts. To fix
this, remove the direct import of Job from Bull and instead define or import a
unified Job interface that covers both Bull and fallback queue job shapes. Then
update the function signatures in this processor to use this unified Job type to
ensure compatibility and avoid compile-time errors.
| const storageAdapter = await NcPluginMgrv2.storageAdapter(); | ||
| const cutoffDate = moment().subtract(4, 'hours'); | ||
|
|
||
| // Convert cutoff date to YYYY-MM-DD format to match folder structure | ||
| const cutoffDateFormatted = cutoffDate.format('YYYY-MM-DD'); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Cut-off resolution is day-only → files < 4 h old may be deleted or kept incorrectly
cutoffDate is “now – 4 h” but you compare only the YYYY-MM-DD folder segment.
Examples:
- 02:30 today – cutoff is “y-m-d”; a file created 23:59 yesterday (< 4 h ago) will still be deleted.
- 23:00 today – cutoff is “y-m-d”; a file created at 05:00 today (> 4 h old) will NOT be deleted.
Consider parsing the hour folder too or comparing epoch timestamps:
-const folderDate = pathParts[dateIndex]; // YYYY-MM-DD
-…
-if (folderDate && folderDate < cutoffDateFormatted)
+const hourSegment = pathParts[dateIndex + 1] ?? '00'; // HH
+const fileMoment = moment(`${folderDate} ${hourSegment}`, 'YYYY-MM-DD HH');
+
+if (fileMoment.isBefore(cutoffDate))
…Ensure path structure really contains an hour segment before applying the fix.
#!/bin/bash
set -e
FILE="packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts"
if [[ ! -f "$FILE" ]]; then
echo "❌ File not found: $FILE"
exit 1
fi
echo "🔍 Inspecting around ‘pathParts’ in $FILE"
rg -n -C3 "pathParts" "$FILE" || echo "No matches for pathParts"
echo
echo "🔍 Inspecting around ‘dateIndex’ in $FILE"
rg -n -C3 "dateIndex" "$FILE" || echo "No matches for dateIndex"🏁 Script executed:
#!/bin/bash
set -e
FILE="packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts"
if [[ ! -f "$FILE" ]]; then
echo "❌ File not found: $FILE"
exit 1
fi
echo "🔍 Inspecting 'pathParts' usage:"
rg -n -C3 "pathParts" "$FILE" || echo "No matches for pathParts"
echo
echo "🔍 Inspecting 'dateIndex' definition:"
rg -n -C3 "dateIndex" "$FILE" || echo "No matches for dateIndex"
echo
echo "🔍 Inspecting how path segments are split:"
rg -n -C5 "split(" "$FILE" || echo "No split calls found"Length of output: 2134
Include hour segment in cutoff comparison
The current cleanup logic only compares the YYYY-MM-DD folder against cutoffDateFormatted, which can delete files less than 4 h old (from late yesterday) or keep files older than 4 h (earlier today). Since the path is …/data-export/YYYY-MM-DD/HH/..., you can grab the HH segment and compare full timestamps:
Locations to update:
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.tsaround lines 64–67
Suggested diff:
- const folderDate = pathParts[dateIndex];
- // Check if folder date is before cutoff date
- if (folderDate && folderDate < cutoffDateFormatted) {
+ const folderDate = pathParts[dateIndex];
+ // Include hour folder (HH) for full timestamp accuracy
+ const hourSegment = pathParts[dateIndex + 1] ?? '00';
+ const fileMoment = moment(`${folderDate} ${hourSegment}`, 'YYYY-MM-DD HH');
+ if (fileMoment.isBefore(cutoffDate)) {📝 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.
| const storageAdapter = await NcPluginMgrv2.storageAdapter(); | |
| const cutoffDate = moment().subtract(4, 'hours'); | |
| // Convert cutoff date to YYYY-MM-DD format to match folder structure | |
| const cutoffDateFormatted = cutoffDate.format('YYYY-MM-DD'); | |
| const folderDate = pathParts[dateIndex]; | |
| // Include hour folder (HH) for full timestamp accuracy | |
| const hourSegment = pathParts[dateIndex + 1] ?? '00'; | |
| const fileMoment = moment(`${folderDate} ${hourSegment}`, 'YYYY-MM-DD HH'); | |
| if (fileMoment.isBefore(cutoffDate)) { | |
| … | |
| } |
🤖 Prompt for AI Agents
In
packages/nocodb/src/modules/jobs/jobs/data-export-clean-up/data-export-clean-up.processor.ts
around lines 64 to 67, the cleanup logic only compares the date folder segment
(YYYY-MM-DD) against the cutoff date, ignoring the hour folder segment. To fix
this, extract the hour segment (HH) from the file path, combine it with the date
segment to form a full timestamp, and compare this full timestamp against the
cutoffDate (now minus 4 hours).
F438
This ensures files less than 4 hours old are not
deleted and files older than 4 hours are properly cleaned up.
| @@ -1,6 +1,7 @@ | |||
| import { forwardRef, Inject, Injectable } from '@nestjs/common'; | |||
| import PQueue from 'p-queue'; | |||
| import Emittery from 'emittery'; | |||
| import { CronExpressionParser } from 'cron-parser'; | |||
There was a problem hiding this comment.
Invalid import – cron-parser does not export CronExpressionParser
cron-parser exports parseExpression (default) or named parseExpression.
The current import will crash at runtime.
-import { CronExpressionParser } from 'cron-parser';
+import { parseExpression as parseCronExpression } from 'cron-parser';Later usages must be updated:
-const cron = CronExpressionParser.parse(opts.repeat.cron);
+const cron = parseCronExpression(opts.repeat.cron);🤖 Prompt for AI Agents
In packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts at line
4, the import statement incorrectly imports CronExpressionParser from
'cron-parser', which does not export this symbol. Replace the import with the
correct named import or default import of parseExpression from 'cron-parser'.
Then update all subsequent usages of CronExpressionParser in the file to use
parseExpression instead.
| if (opts?.repeat) { | ||
| // Initialize recurring job execution based on cron pattern | ||
| const scheduleNextExecution = () => { | ||
| try { | ||
| const cron = CronExpressionParser.parse(opts.repeat.cron); | ||
| const nextExecutionTime = cron.next().toDate(); | ||
| const delayMs = Math.max(0, nextExecutionTime.getTime() - Date.now()); | ||
|
|
||
| const recurringJobId = `${id}-${Date.now()}`; | ||
| const childJob = { ...job, id: recurringJobId }; | ||
|
|
||
| setTimeout(() => { | ||
| this.queueMemory.push(childJob); | ||
| // Execute the current job | ||
| this.queue | ||
| .add(() => this.jobWrapper(childJob)) | ||
| .then(() => { | ||
| // Schedule the next execution only after current job completes | ||
| this.queue.add(() => scheduleNextExecution()); | ||
| }) | ||
| .catch((error) => { | ||
| console.error( | ||
| `Failed to schedule recurring job ${name}:`, | ||
| error, | ||
| ); | ||
| // Try to reschedule despite the error | ||
| this.queue.add(() => scheduleNextExecution()); | ||
| }); | ||
| }, delayMs); | ||
| } catch (error) { | ||
| console.error(`Invalid cron expression for job ${name}:`, error); | ||
| } | ||
| }; | ||
|
|
||
| this.queueMemory.push(job); | ||
|
|
||
| // Start the recurring job process | ||
| scheduleNextExecution(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cron scheduling logic uses wrong API & can explode memory
CronExpressionParser.parse(non-existent) → runtime failure (see previous comment).- Every run pushes a new
childJobintoqueueMemorybut never removes it, so long-running instances will leak memory.
Proposed fix (simplified):
-const cron = CronExpressionParser.parse(opts.repeat.cron);
-const nextExecutionTime = cron.next().toDate();
+const interval = parseCronExpression(opts.repeat.cron);
+const nextExecutionTime = interval.next().toDate();
…
this.queue
.add(() => this.jobWrapper(childJob))
.finally(() => {
this.removeJob(childJob); // ensure memory cleanup
- this.queue.add(() => scheduleNextExecution());
+ scheduleNextExecution();
});Make sure removeJob accepts missing index -1 (see next comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/nocodb/src/modules/jobs/fallback/fallback-queue.service.ts between
lines 162 and 199, replace the incorrect call to CronExpressionParser.parse with
the correct API to parse cron expressions to avoid runtime errors. Additionally,
modify the scheduling logic to prevent memory leaks by ensuring that each
recurring child job added to queueMemory is properly removed after execution.
Implement removal of the child job from queueMemory once it completes, and
verify that the removeJob method can handle cases where the job index is -1
without errors.
Change Summary
Change type