refactor(webdav): simplify getNodeIconType return type#38618
refactor(webdav): simplify getNodeIconType return type#38618sezallagwal wants to merge 1 commit into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts`:
- Around line 12-29: The MIME type
application/vnd.oasis.opendocument.presentation is duplicated in
getNodeIconType: remove it from the first branch (the array that returns
'file-document') so that only the ODT text MIME remains there, and keep
application/vnd.oasis.opendocument.presentation in the later branch that returns
'file-sheets' (ensuring presentations map to 'file-sheets' and documents to
'file-document').
🧹 Nitpick comments (3)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts (2)
8-10: Minor: regex for PDF MIME is overly permissive.
/application\/pdf/will also match strings like"xapplication/pdf-extra". Consider using an exact equality check for consistency with the other MIME checks in this function.Proposed fix
- if (mime?.match(/application\/pdf/)) { + if (mime === 'application/pdf') { return 'file-pdf'; }
3-3: Remove the unused_basenameparameter and update all 4 call sites.The
_basenameparameter is never referenced in the function body. However, all existing callers (WebdavFilePickerTable.tsx, WebdavFilePickerGrid.tsx, and two test cases in getNodeIconType.spec.ts) pass this argument positionally, so removing it requires updating those call sites to omit the first argument.apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts (1)
5-13: Test coverage only covers 2 of 6 branches.The function has branches for PDF, ODP text/presentation, spreadsheets, PowerPoint, and the default fallback, but only the default (
'clip') and directory ('folder') cases are tested. Consider adding tests for the MIME-specific branches to prevent regressions — especially given the duplicate MIME noted ingetNodeIconType.ts.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerGrid/WebdavFilePickerGrid.tsxapps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerTable.tsxapps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.tsapps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerGrid/WebdavFilePickerGrid.tsxapps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerTable.tsxapps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.tsapps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
🧠 Learnings (11)
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.tsapps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts
🧬 Code graph analysis (3)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerGrid/WebdavFilePickerGrid.tsx (2)
packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx (1)
icon(361-367)apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts (1)
getNodeIconType(3-32)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerTable.tsx (1)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts (1)
getNodeIconType(3-32)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.spec.ts (1)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts (1)
getNodeIconType(3-32)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerTable.tsx (1)
75-75: LGTM!Correctly updated to use the direct return value from
getNodeIconType.apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerGrid/WebdavFilePickerGrid.tsx (1)
36-36: LGTM!Correctly updated to use the direct return value, consistent with the table component.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
Proposed changes
Simplifies
getNodeIconTypeto returnIconNamedirectly instead of{ icon, type, extension }, as noted in the existing TODO comment.The
typeandextensionfields were unused and both callers only destructured{ icon }.Changes
IconNamedirectly, remove unusedtype/extensionvariables and TODO commentconst { icon } =→const icon =Issue(s)
Resolves TODO in:
apps/meteor/client/views/room/webdav/WebdavFilePickerModal/lib/getNodeIconType.ts// TODO: This function should be simplified, it only needs to return the icon nameSteps to test or reproduce
Summary by CodeRabbit