implemented E2E-CHAT-008 Chat Search and resolved mobile viewport dra…#407
Conversation
|
@sandy4242 is attempting to deploy a commit to the Louis Girifalco's projects Team on Vercel. A member of the Team first needs to authorize it. |
motirebuma
left a comment
There was a problem hiding this comment.
Hey @sandy4242 there's good stuff in here. The ChatList search improvement (matching against postDetails.title and postDetails.text in addition to room title) is a real fix, the duplicate MobileDrawer removal from ChatMenu is a nice cleanup, and the drawer interact-outside prevention makes sense for the chat panel UX. The E2E spec covers the search flow well.
A few things to address before this can land:
- Remove debug console.logs from
ChatList.tsx. You've gotconsole.log('ChatList state:', ...)andconsole.log('ChatList filteredRooms details:', ...)that shouldn't ship to production. Easy fix. - Conflicts with other open PRs. Your
ChatSearchInputprevAddBuddyModeRef fix duplicates PR #402 (which is smaller and ready to merge). TheSelectionPopover_topOffsetrename overlaps with PR #401's full portal rewrite. The Playwright config and jest exclusion conflict with PRs #399 and #405. I'd suggest dropping the SelectionPopover and ChatSearchInput changes from this PR entirely and letting those other PRs handle them -- that way yours can focus on the chat search feature and drawer fix without stepping on anyone's toes. - The
_topOffsetrename -- if you're not using the prop, remove it from the interface rather than prefixing with underscore. But per the above, probably just drop this change since #401 rewrites the whole component anyway. - Scope for future PRs: this bundles an E2E test, a search logic fix, a drawer behavior change, a ChatMenu cleanup, a login SSR fix, and auth token handling into one PR. Smaller, focused PRs are easier to review and less likely to conflict. Not a blocker for this one, just something to keep in mind going forward.
Verdict: needs a quick follow-up remove the console.logs and drop the changes that overlap with #401 and #402, then this is good to go.
Thankyou!!
all the above conflicts and console logs issues are addressed |
motirebuma
left a comment
There was a problem hiding this comment.
thanks @sandy4242 all issues are addressed as requested, LGTM
all tests were done using ai and no issues were foundissue reference: #396
@flyblackbox