Skip to content

refactor(importer): handle nullable result from Imports.findOne#39775

Open
CodeMatrix1 wants to merge 1 commit into
RocketChat:developfrom
CodeMatrix1:refactor/importer-handle-findone-null
Open

refactor(importer): handle nullable result from Imports.findOne#39775
CodeMatrix1 wants to merge 1 commit into
RocketChat:developfrom
CodeMatrix1:refactor/importer-handle-findone-null

Conversation

@CodeMatrix1
Copy link
Copy Markdown

@CodeMatrix1 CodeMatrix1 commented Mar 20, 2026

Proposed changes

Fixes a Todo unsafe type assertion from Imports.findOne result.

Handles nullable return value explicitly and assigns only after validation.

  • remove as IImport cast
  • add null check for findOne result

Improves type safety and avoids masking potential null values.

No functional changes.

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling in the import process to ensure proper validation of import records and provide clear error messages when records cannot be found, increasing import reliability and preventing potential failures.

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Mar 20, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 20, 2026

⚠️ No Changeset found

Latest commit: 605035d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1764f7d0-50cc-4fbc-a8fc-d5f8c61c0962

📥 Commits

Reviewing files that changed from the base of the PR and between 82adf3a and 605035d.

📒 Files selected for processing (1)
  • apps/meteor/app/importer/server/classes/Importer.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • apps/meteor/app/importer/server/classes/Importer.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Applied to files:

  • apps/meteor/app/importer/server/classes/Importer.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

Applied to files:

  • apps/meteor/app/importer/server/classes/Importer.ts
🔇 Additional comments (1)
apps/meteor/app/importer/server/classes/Importer.ts (1)

375-380: Type safety improvement is correct.

The explicit null check and error throw is the appropriate approach. This prevents silent assignment of null when the record is unexpectedly absent, which is a data integrity guard.

The error will propagate through the Meteor method handler (lines 86-98 in downloadPublicImportFile.ts), which provides implicit error handling at the method level—the standard pattern for Meteor server methods. This fail-fast behavior is intentional and appropriate when a critical resource (the import record) unexpectedly disappears after an update operation.


Walkthrough

The updateRecord method in the Importer class was modified to safely fetch an updated Imports document, verify it exists, and throw an error if retrieval fails instead of assigning an unchecked value.

Changes

Cohort / File(s) Summary
Error Handling Improvement
apps/meteor/app/importer/server/classes/Importer.ts
Added null check when fetching updated Imports document; throws Error('error-import-record-not-found') if document not found instead of assigning invalid value to this.importRecord.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

type: chore

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically describes the main change: removing an unsafe type assertion and adding proper null handling for Imports.findOne's nullable result.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant