fix: export jobs getting stuck when userNameTable is null #37387
Conversation
🦋 Changeset detectedLatest commit: 21b5b00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is ready to merge! 🎉 |
… export jobs Signed-off-by: Abhinav Kumar <[email protected]>
Signed-off-by: Abhinav Kumar <[email protected]>
abhinavkrin
force-pushed
the
fix/export-user-data-stuck-null-username-table
branch
from
1e0c216 to
a5ebda4
Compare
WalkthroughFixes data export requests that could remain stuck by refactoring the userNameTable structure from a nested object to a flat map, ensuring proper initialization throughout the export pipeline. Adds comprehensive test coverage for data download processing and includes a release note documenting the patch. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant requestDataDownload
participant processDataDownloads
participant exportRoomMessagesToFile
participant FileSystem
User->>requestDataDownload: Request data export
requestDataDownload->>requestDataDownload: Create exportOperation<br/>with userNameTable: {}
requestDataDownload-->>User: Pending export created
Note over processDataDownloads: Async processing begins
processDataDownloads->>processDataDownloads: Guard: Initialize userNameTable<br/>if undefined
processDataDownloads->>exportRoomMessagesToFile: Process room messages<br/>with usersMap (flat map)
exportRoomMessagesToFile->>exportRoomMessagesToFile: Obfuscate usernames<br/>directly in usersMap[username]
exportRoomMessagesToFile->>FileSystem: Write exported data
FileSystem-->>exportRoomMessagesToFile: Files created
exportRoomMessagesToFile-->>processDataDownloads: Export complete
processDataDownloads->>processDataDownloads: Update status to completed
processDataDownloads-->>User: Export ready for download
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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)
apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts (1)
28-35: Force persisteduserNameTabletonullso the regression stays coveredRight now the
userNameTable: nullinitializer gets overwritten by the spread because the real code now passes{}. That means the spec never drives the legacy-null scenario and would still pass if the guard disappeared. Flip the assignment order (or explicitly set to null after the spread) so the stored operation actually hasnullbeforeprocessDataDownloadsruns.- create: async (data: any) => { - exportOperation = { - userNameTable: null, // need to keep this null for testing purposes - ...data, - _id: 'exportOp1', - createdAt: new Date(), - }; + create: async (data: any) => { + exportOperation = { + ...data, + _id: 'exportOp1', + createdAt: new Date(), + }; + exportOperation.userNameTable = null; // simulate legacy persisted nullapps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
187-196: Default empty object prevents null pointer errors.The default value
usersMap: IExportOperation['userNameTable'] = {}is a critical part of the fix, ensuring that even if callers don't provide this parameter, the function won't encounter null/undefined errors.Consider typing the userData parameter.
The
anytype on line 192 reduces type safety. If this is pre-existing and out of scope for this PR, it could be addressed in a future refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/funny-rocks-admire.md(1 hunks)apps/meteor/.mocharc.js(1 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(4 hunks)apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts(1 hunks)apps/meteor/server/lib/dataExport/processDataDownloads.ts(1 hunks)apps/meteor/server/methods/requestDataDownload.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: When generating tests, provide complete runnable TypeScript test files with proper imports, clear describe/test blocks, and adherence to these guidelines
Applied to files:
apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Implement proper wait strategies for dynamic content
Applied to files:
apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use test.step() to organize complex test scenarios
Applied to files:
apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts
🧬 Code graph analysis (2)
apps/meteor/server/lib/dataExport/processDataDownloads.spec.ts (2)
apps/meteor/server/methods/requestDataDownload.ts (2)
requestDataDownload(25-109)requestDataDownload(112-120)apps/meteor/server/lib/dataExport/processDataDownloads.ts (1)
processDataDownloads(248-285)
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
apps/meteor/.mocharc.js (1)
16-16: Include data export specs in the mocha runGood call adding this glob—without it the new server/lib/dataExport specs would stay invisible to the main mocha run.
.changeset/funny-rocks-admire.md (1)
1-5: Release note is spot onThanks for documenting this regression fix so downstream teams know the exporting stall is addressed.
apps/meteor/server/lib/dataExport/processDataDownloads.ts (1)
181-183: GuardinguserNameTableprevents the observed TypeErrorInitializing the map here keeps legacy operations (persisted with
null) from crashing whenexportRoomMessagesToFilereads from it.apps/meteor/server/methods/requestDataDownload.ts (1)
87-87: Seed the username map on creationSeeding the export operation with
{}keeps new jobs from persistingnullgoing forward—nice preventative hardening.apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (4)
3-3: LGTM: Type import supports improved type safety.Adding
IExportOperationenables proper typing of theuserNameTableparameter throughout the file, replacing less specific types.
11-21: LGTM: Flat map structure eliminates null pointer risk.The refactoring from nested
usersMap.userNameTableto flatusersMapdirectly addresses the root cause of the bug. The function correctly maintains username mappings through mutation, andObject.keys(usersMap).length + 1works properly with the flat structure.
57-62: LGTM: Function signature aligns with the refactored structure.The updated type
IExportOperation['userNameTable']provides better type safety than the previous nested structure and matches the changes inhideUserName.
231-251: LGTM: Consistent null-safe defaults throughout the export pipeline.The default value
usersMap: IExportOperation['userNameTable'] = {}on line 249 maintains consistency withexportRoomMessagesand ensures the entire export flow is protected from null pointer errors.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37387 +/- ##
===========================================
+ Coverage 67.01% 67.08% +0.07%
===========================================
Files 3418 3419 +1
Lines 117751 117938 +187
Branches 21532 21581 +49
===========================================
+ Hits 78907 79124 +217
+ Misses 36164 36128 -36
- Partials 2680 2686 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
abhinavkrin
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
kodiakhq
bot
deleted the
fix/export-user-data-stuck-null-username-table
branch
Proposed changes (including videos or screenshots)
This PR fixes a bug that causes "My Data → Export Data" jobs to remain stuck in the
exportingstate and never transition tocompleted.Root cause:
TypeError: Cannot read properties of null (reading 'userNameTable')is thrown fromserver/lib/dataExport/exportRoomMessagesToFile.tswhenhideUserName/anonymization logic tries to accessuserNameTablewhile it isnull.Issue(s)
Steps to test or reproduce
Further comments
SUP-909
Summary by CodeRabbit
Bug Fixes
Tests