refactor: extract row cell alignment into GenericTableRow#41442
refactor: extract row cell alignment into GenericTableRow#41442Pr241singh wants to merge 4 commits 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 |
|
Walkthrough
ChangesGeneric table alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx (1)
16-20: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove unused
customAlignTopdefinition and fixcssreference error.The
cssimport from@rocket.chat/css-in-jswas removed, but thecustomAlignTopvariable definition usingcsswas left behind. This will cause a compilation/reference error forcssand leavescustomAlignTopas an unused variable. Please remove this block completely.🐛 Proposed fix to remove the unused code
-const customAlignTop = css` - td { - vertical-align: top; - } -`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx` around lines 16 - 20, Remove the unused customAlignTop styled definition from ContactTableRow, including its css template block, so no unresolved css reference or unused variable remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx`:
- Around line 16-20: Remove the unused customAlignTop styled definition from
ContactTableRow, including its css template block, so no unresolved css
reference or unused variable remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 993a9b53-1055-4abc-b50f-eb71c670a3ac
📒 Files selected for processing (3)
apps/meteor/client/views/omnichannel/directory/chats/ChatsTable/ChatsTableRow.tsxapps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsxpackages/ui-client/src/components/GenericTable/GenericTableRow.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 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:
packages/ui-client/src/components/GenericTable/GenericTableRow.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatsTable/ChatsTableRow.tsxapps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx
🧠 Learnings (2)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
packages/ui-client/src/components/GenericTable/GenericTableRow.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatsTable/ChatsTableRow.tsxapps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/ui-client/src/components/GenericTable/GenericTableRow.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatsTable/ChatsTableRow.tsxapps/meteor/client/views/omnichannel/directory/contacts/ContactTableRow.tsx
🔇 Additional comments (2)
apps/meteor/client/views/omnichannel/directory/chats/ChatsTable/ChatsTableRow.tsx (1)
56-56: LGTM!packages/ui-client/src/components/GenericTable/GenericTableRow.tsx (1)
15-28: 🎯 Functional CorrectnessVerify
TableRowaccepts arrayclassNamevalues. ReactclassNameexpects a string; if@rocket.chat/fuselageforwards[className, alignTop]directly, the DOM will get a comma-separated class string and the top-alignment styling will break.
dougfabris
left a comment
There was a problem hiding this comment.
actually it has to be made on fuselage side
|
@dougfabris Thanks for the review! I understand that this should be implemented in Fuselage instead of Should I open a PR in the Fuselage repository to add the alignment support there, and then update this PR to use the new API? |
|
The fix on fuselage is enough for me |
|
@dougfabris Thanks for the feedback! I've moved the implementation into Fuselage as suggested and opened a corresponding PR: RocketChat/fuselage##2101 Once that change is reviewed and merged, I'll update this PR accordingly. Could you please take another look when you have a chance? Thanks! |
|
I will take a look as soon as possible, for now we can close this one |
Proposed changes (including videos or screenshots)
This refactoring removes duplicated row cell alignment styles from the Omnichannel directory table rows by moving the behavior into
GenericTableRow.Changes made
cellVerticalAlignprop toGenericTableRowcustomAlignTopstyles in:ChatsTableRowContactTableRowThis keeps the alignment logic centralized and avoids repeating the same custom styling across multiple table row implementations.
Issue(s)
Ref #41439
Steps to test or reproduce
Further comments
This change is a small refactoring that centralizes the row cell alignment behavior in
GenericTableRow, removing duplicated custom CSS while preserving the existing appearance of the Omnichannel directory tables.Summary by CodeRabbit