refactor: Install rest-typings and migrate helper types #6876
Conversation
WalkthroughReplaced several local REST typing definitions with re-exports and types from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
deepak0x
force-pushed
the
refactor/rest-typings-setup_and_helpers_migration
branch
from
39693ae to
1c1e6dc
Compare
![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
🧹 Nitpick comments (1)
scripts/verify-migration-phase.sh (1)
1-52: Consider adding test execution to match documentation.MIGRATION_SUMMARY.md and MIGRATION_CHECKLIST.md both reference running
yarn testas part of the verification process, but this script only checks TypeScript compilation and linting. Consider adding test execution or updating the documentation to reflect the actual verification steps.🔎 Proposed addition of test execution
echo "✅ Linting passed with 0 errors" +# Run tests +echo "🧪 Running tests..." +if ! yarn test; then + echo "❌ Tests failed!" + exit 1 +fi +echo "✅ Tests passed" + echo "" echo "✅ Phase $PHASE verification complete!" echo " - TypeScript: ✅" echo " - Linting: ✅ (0 errors)" +echo " - Tests: ✅"
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.gitignoreMIGRATION_CHECKLIST.mdMIGRATION_PLAN.mdMIGRATION_SUMMARY.mdapp/definitions/rest/COMPATIBILITY_LAYER.mdapp/definitions/rest/helpers/PaginatedRequest.tsapp/definitions/rest/helpers/PaginatedResult.tsapp/definitions/rest/helpers/index.tspackage.jsonscripts/verify-migration-phase.sh
🔇 Additional comments (8)
app/definitions/rest/COMPATIBILITY_LAYER.md (1)
1-52: Documentation looks good, but note the .gitignore conflict.The compatibility layer documentation is well-structured and provides clear guidance with helpful examples. However, this file is listed in
.gitignore(line 92), which conflicts with it being committed. Please refer to the review comment on.gitignorefor resolution.MIGRATION_PLAN.md (1)
1-315: Comprehensive migration plan, but conflicts with .gitignore.The migration plan is well-structured with clear phases, time estimates, and quality gates. However, this file matches the
*MIGRATION*.mdpattern in.gitignore(line 88), which would prevent it from being tracked. Please refer to the review comment on.gitignorefor resolution.app/definitions/rest/helpers/PaginatedResult.ts (1)
1-2: No action needed—verification confirms the external type exists and all usages are type-safe.The
@rocket.chat/rest-typingspackage (v6.13.1) is properly declared in package.json, and the re-export is working correctly across all seven files that importPaginatedResult(directory.ts, settings.ts, teams.ts, videoConference.ts, omnichannel.ts, emojiCustom.ts, chat.ts). No type errors or import failures are present, confirming structure compatibility and type-safety.package.json (1)
50-53: The specified versions (6.13.1) exist, are published, and have no known security vulnerabilities. They are compatible by design, released together as matched versions in Rocket.Chat 6.13.1. The exact version pinning indicates intentional selection for stable compatibility with a specific Rocket.Chat server release, which is appropriate for a mobile client targeting that version. No action needed.app/definitions/rest/helpers/index.ts (1)
2-4: LGTM! Clear migration plan documented.The TODO comments effectively communicate the migration strategy and rationale for maintaining the current implementation during the transition phase.
MIGRATION_SUMMARY.md (1)
1-58: Well-structured migration documentation.The summary provides a clear overview of the migration process with helpful quick references, phase descriptions, quality gates, and important notes. The structure will guide developers effectively through the phased migration.
MIGRATION_CHECKLIST.md (1)
1-288: Excellent comprehensive migration checklist.This checklist is well-structured and provides clear, actionable steps for each migration phase. The inclusion of verification commands (
yarn lint,tsc --noEmit) after each category and standardized commit messages will help maintain quality and consistency throughout the migration process.app/definitions/rest/helpers/PaginatedRequest.ts (1)
1-2: No action needed. The re-export to@rocket.chat/rest-typingsis confirmed compatible.The migration refactored the local
PaginatedRequestdefinition to use the external package type, which correctly supports generic parametersT(request body) and an optionalS(sort field) as evidenced by usage patterns throughout the codebase (PaginatedRequest<{ query: string }, 'name'>,PaginatedRequest<{ text: string }>). The external type includes the expected fields:count,offset,sort, andquery. The codebase actively uses this re-export without type errors across multiple endpoint definitions.
deepak0x
force-pushed
the
refactor/rest-typings-setup_and_helpers_migration
branch
4 times, most recently
from
861a4fa to
d9b821b
Compare
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
Left a few comments.
| @@ -1,4 +1,7 @@ | |||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | |||
| // TODO: Once we migrate all endpoints to use @rocket.chat/rest-typings, | |||
There was a problem hiding this comment.
Can you do it on this PR or start the work on this PR, so it serves as a POC?
Preparation & Setup - Install @rocket.chat/[email protected] - Install @rocket.chat/[email protected] (required dependency) - Explore and document package structure Helper Types Migration - Migrate PaginatedRequest to re-export from @rocket.chat/rest-typings - Migrate PaginatedResult to re-export from @rocket.chat/rest-typings - Update helpers/index.ts with TODO for future endpoint migration - Keep custom ResultFor, Serialized, ErrorResult for SDK compatibility
- Remove PaginatedRequest.ts and PaginatedResult.ts wrapper files
- Use KeyOfEach and ReplacePlaceholders from @rocket.chat packages - Implement rest-typings patterns: PathFor, PathWithParamsFor, PathWithoutParamsFor - Add utilities: JoinPathPattern, UrlParams, MethodOf, ParamsFor - Use MethodToPathPatternToParamsMap and MethodToPathPatternToResultMap patterns - Fix TypeScript errors in useEndpointData with proper type guards - Update import order in endpoint definition files
deepak0x
force-pushed
the
refactor/rest-typings-setup_and_helpers_migration
branch
from
0aa5c81 to
b5e17a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/definitions/rest/helpers/index.ts (2)
1-1: Verify the necessity of the eslint-disable directive.The
no-unused-varsdisable is common in type-only files, but it can mask legitimate issues. Consider reviewing whether all type parameters are actually necessary.
2-4: Track the migration TODO for future completion.The POC comment indicates this is a temporary bridge implementation until full migration to
@rocket.chat/rest-typings. This technical debt should be tracked to ensure the migration is completed.Would you like me to create a tracking issue for the full migration to
@rocket.chat/rest-typingsendpoints (replacing the current Endpoints structure with/v1/prefixed paths)?
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
app/definitions/rest/helpers/PaginatedRequest.tsapp/definitions/rest/helpers/PaginatedResult.tsapp/definitions/rest/helpers/index.tsapp/definitions/rest/v1/channels.tsapp/definitions/rest/v1/chat.tsapp/definitions/rest/v1/directory.tsapp/definitions/rest/v1/emojiCustom.tsapp/definitions/rest/v1/groups.tsapp/definitions/rest/v1/im.tsapp/definitions/rest/v1/omnichannel.tsapp/definitions/rest/v1/settings.tsapp/definitions/rest/v1/teams.tsapp/definitions/rest/v1/videoConference.tsapp/lib/hooks/useEndpointData.tspackage.json
💤 Files with no reviewable changes (2)
- app/definitions/rest/helpers/PaginatedRequest.ts
- app/definitions/rest/helpers/PaginatedResult.ts
✅ Files skipped from review due to trivial changes (1)
- app/definitions/rest/v1/videoConference.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
app/definitions/rest/helpers/index.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
app/lib/hooks/useEndpointData.ts (1)
app/definitions/rest/helpers/index.ts (4)
ErrorResult(182-182)Serialized(48-56)ResultFor(177-180)MatchPathPattern(59-61)
🔇 Additional comments (13)
app/lib/hooks/useEndpointData.ts (1)
45-51: The error-handling logic is correct and intentional.The SDK resolves promises in both success and error cases—it only rejects for network/connection errors. The logic correctly identifies errors when responses have
success: falseand treats all other resolved values as results. This matches the actual SDK behavior shown in the tests and is part of the@rocket.chat/rest-typingsmigration.The guards are appropriate:
e && typeof e === 'object' && 'success' in e && !e.successensures non-objects and objects without a success property pass through tosetResult, which is the correct behavior since the type system guarantees all SDK responses are objects with a success property.Likely an incorrect or invalid review comment.
app/definitions/rest/v1/im.ts (1)
1-1: LGTM! Import migration correctly applied.The migration from local
PaginatedRequestto@rocket.chat/rest-typingsfollows the same pattern as other files in this PR. Type-only import is appropriate.app/definitions/rest/v1/channels.ts (1)
1-1: LGTM! Consistent with the rest-typings migration.The migration from local
PaginatedRequestto@rocket.chat/rest-typingsis correctly implemented.app/definitions/rest/v1/teams.ts (1)
1-1: LGTM! Migration correctly applied.The migration from local
PaginatedResultto@rocket.chat/rest-typingsis consistent with the broader refactoring effort.app/definitions/rest/v1/settings.ts (1)
1-1: LGTM! Import migration correctly applied.The migration from local
PaginatedResultto@rocket.chat/rest-typingsis correctly implemented. The type is used in an intersection type at line 70, which should work seamlessly with the external definition.app/definitions/rest/v1/chat.ts (1)
1-1: LGTM! Migration to @rocket.chat/rest-typings is correctly implemented.The type-only import from
@rocket.chat/rest-typingsforPaginatedResultis properly used and compatible. The old local definition has been removed, and the genericPaginatedResult<T>type matches the usage patterns in chat.ts and other REST endpoint definitions. Package version 6.13.1 is stable and was released as part of the official Rocket.Chat 6.13.1 release.app/definitions/rest/v1/emojiCustom.ts (1)
1-1: LGTM! Clean migration to external types.The import consolidation from local helpers to
@rocket.chat/rest-typingsaligns with the PR objectives and maintains existing type signatures.app/definitions/rest/v1/directory.ts (1)
1-1: LGTM! Consistent with migration pattern.The import source change maintains the existing type contract while aligning with the centralized type definitions from
@rocket.chat/rest-typings.app/definitions/rest/v1/omnichannel.ts (1)
1-1: LGTM! Extensive migration completed successfully.The import migration to
@rocket.chat/rest-typingsis applied consistently across all OmnichannelEndpoints, maintaining type safety while reducing local type duplication.app/definitions/rest/helpers/index.ts (3)
29-29: Type intersection ensures string compatibility.The
TPathPattern & stringintersection correctly constrains the type forReplacePlaceholders, ensuring that only string-compatible path patterns are processed.
175-180: LGTM! Error handling wrapper aligns with SDK needs.The
ResultFortype correctly wraps operation results withSuccessResult,FailureResult, andUnauthorizedResultpatterns, providing comprehensive error handling for SDK consumers.
64-158: This is a POC adaptation—verify against actual rest-typings 6.13.1 when migrating endpoints.The type utilities are correctly documented as adaptations rather than direct implementations from
@rocket.chat/rest-typings. The file comment explicitly notes this is a temporary POC pattern awaiting full endpoint migration. The only direct import from rest-typings isReplacePlaceholders, which is properly re-exported; the remaining utilities (UrlParams,MethodOf,PathFor, etc.) are locally implemented to match the current Endpoints structure.Before merging, confirm:
- The local type implementations are compatible with rest-typings 6.13.1's type signatures when endpoints are eventually migrated to use
/v1/prefix paths- No breaking changes in future rest-typings versions (6.13.1+) will affect the mapped type patterns used here
The TypeScript strict mode configuration and tsc in the lint script will catch type errors during compilation, but real compatibility should be verified once these types are actively used.
app/definitions/rest/v1/groups.ts (1)
1-1: Clean refactor with confirmed package compatibility.The migration to
@rocket.chat/rest-typingsimproves type consistency across the codebase. The package version6.13.1is correctly specified inpackage.json, andPaginatedRequestis properly exported and already in use across multiple endpoint definitions.
2 participants
Preparation & Setup
Helper Types Migration
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.