[User functionality] Small PR focused on tests for user functionality#53
Merged
Conversation
Added more compatibility questions
Added compatibility question tests and verifications Updated tests to cover Keywords and Headline changes recently made Updated tests to cover all of the big5 personality traits
Updated signUp.spec.ts to use new fixture Updated Account information variable names Deleted "deleteUserFixture.ts" as it was incorporated into the "base.ts" file
Updated seedDatabase.ts to throw an error if the user already exists, to also add display names and usernames so they seedUser func acts like a normal basic user Some organising of the google auth code
Added account deletion checks
Updated filters.tsx: added data testid
Added data test attributes to search.tsx and profile-grid.tsx
Added testid attributes to people page so the info from the results can be accessed
…th each other Added an option to verify the users email when creating an account
Added tests for staring/favoriting profiles Added testIds where necessary Added messagesPage.ts Updated peoplePage.ts
|
@O-Bots is attempting to deploy a commit to the Compass Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 45 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Firebase email-verification helpers and config, a ContextManager and MessagesPage for multi-context e2e flows, enhances PeoplePage test object and fixtures for verified user seeding, expands e2e specs for filtering/messaging, and adds data-testid attributes across web components for deterministic selectors. ChangesE2E Testing Infrastructure & Expanded Test Coverage
Web Component Test IDs Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/utils/seedDatabase.ts (1)
161-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly verify email when Firebase account creation/login path is valid.
verifyEmail(...)runs wheneververifyUserEmailis true, even in fallback scenarios where Firebase auth is unavailable. That can break otherwise valid seeded-user flows.Suggested patch
- if (verifyUserEmail) await verifyEmail(userInfo.email, userInfo.password) + if (verifyUserEmail && firebaseId) { + await verifyEmail(userInfo.email, userInfo.password) + }🤖 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 `@tests/e2e/utils/seedDatabase.ts` around lines 161 - 167, verifyEmail(...) is currently called whenever verifyUserEmail is true even if Firebase auth fell back; change the condition so verifyEmail is only invoked when the Firebase path succeeded by checking the firebaseSignUp result (firebaseId). Update the verification call in the seed flow (around firebaseSignUp, seedDbUser, verifyEmail) to run only when verifyUserEmail && firebaseId is truthy, using the firebaseId variable and existing userInfo.email/userInfo.password arguments.tests/e2e/web/pages/peoplePage.ts (1)
458-461:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle empty profile results before random selection.
If
totalResultsis0,nth(chosenProfileNumber)is invalid and the method fails with a non-obvious error. Add an explicit guard with a clear message.Suggested patch
const totalResults = await this.profileResults.count() + if (totalResults === 0) { + throw new Error('No profiles available in results') + } const chosenProfileNumber = Math.floor(Math.random() * totalResults)🤖 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 `@tests/e2e/web/pages/peoplePage.ts` around lines 458 - 461, The code selects a random profile without handling empty results: check this.profileResults.count() and if totalResults === 0 throw or return an explicit error/message (e.g., "No profiles available to select") before computing chosenProfileNumber; only compute chosenProfileNumber and call this.profileResults.nth(chosenProfileNumber) when totalResults > 0 so nth() is never called with an invalid index (references: profileResults, totalResults, chosenProfileNumber, chosenProfile, getByTestId).
🤖 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.
Inline comments:
In `@tests/e2e/utils/contextManager.ts`:
- Around line 9-14: createContext in ContextManager currently overwrites
this.contexts[name] without closing the previous Playwright BrowserContext,
leaking resources; update createContext(name: string) to check if
this.contexts.has(name) and if so call await existingApp.page.context().close()
(or an exported close method on App) before creating/storing the new App, and
ensure ContextManager.closeAll() iterates stored apps/contexts and closes each
BrowserContext properly; also add a call to await contextManager.closeAll() in
the test fixture teardown used by tests/e2e/web/specs/signIn.spec.ts so all
contexts (e.g., 'devOne'/'devTwo') are always cleaned up.
In `@tests/e2e/utils/firebaseUtils.ts`:
- Around line 56-57: Remove the sensitive console.log outputs that print oobCode
and raw errors in tests/e2e/utils/firebaseUtils.ts; locate the places where
oobCode is logged (the console.log(oobCode) call) and where raw error objects
are printed (around lines 65–66) and replace them with non-sensitive logging
using debug() or log(), ensuring you never emit the token/code value — instead
log a non-sensitive message like "verification oobCode received" or include only
error.message/summary via debug() without dumping full error objects or tokens.
- Around line 54-63: The code currently calls axios.post to confirm email even
when getOobCode returns undefined; update the flow in
tests/e2e/utils/firebaseUtils.ts around the oobResponse/oobCode logic (the const
oobResponse = await axios.get(...), const oobCode = await getOobCode(...), and
the subsequent axios.post to CONFIRM_EMAIL_VERIFICATION) to guard against a
missing oobCode: if getOobCode returns falsy, throw an explicit Error (or return
a rejected Promise) with a clear message including the email and that no OOB
code was found, and only call axios.post when oobCode is present so the confirm
request never runs with undefined.
In `@tests/e2e/web/pages/messagesPage.ts`:
- Around line 51-55: The loop is incorrectly reusing the outer index and
unconditionally breaking, causing wrong comparisons and premature exit; change
the inner iteration to use a separate index (e.g., j) when looping over results,
compare results[j].textContent() to username[i] (preserving the outer i), and
only break out of the inner loop when a match is found and after calling
results[j].click(); ensure the outer loop continues if no match was found so
subsequent usernames can be checked.
In `@tests/e2e/web/pages/peoplePage.ts`:
- Around line 502-510: The loop in peoplePage.ts that iterates over profiles
(the for loop using profiles[i], profileName, displayName, messageInput, and
message) continues after a successful match and send, causing duplicate sends;
modify the code so that once you find a matching profile, click the message
button, fill and submit the message, then immediately stop the loop (e.g., break
or return) to prevent further iterations and duplicate actions.
- Line 273: In useSearch: remove the fixed await this.page.waitForTimeout(1000)
and instead wait for a concrete UI condition (e.g., waitForSelector of the
results container, waitForResponse/locator.waitFor or use Playwright auto-wait
on an action that updates results) so the test relies on actual DOM changes; in
getProfileInfo: check for totalResults === 0 and return/handle early before
calling Math.floor(Math.random() * totalResults) or using locator.nth(...) to
avoid out-of-range selection; in messageProfile: once you find and message the
first matching profile (by display name) exit the loop or return immediately so
you don’t send messages to multiple profiles with duplicate names.
In `@tests/e2e/web/specs/firebaseAccountCreationTest.ts`:
- Line 15: Replace the console.log debug prints with the repository logger
calls: import and use the debug() (or log()) helper instead of console.log for
the oobCode print and the other occurrences noted (lines 23-24); update the call
sites (the console.log(oobCode) and any other console.log usages in this spec)
to debug(oobCode) or log(...) and ensure the logger import/initialization used
throughout tests (e.g., debug or log symbol) is present at the top of the file
so the tests compile.
- Around line 11-14: The spec currently hardcodes credentials when calling
firebaseLoginEmailPassword and should instead read test credentials from
SPEC_CONFIG.ts or fixture accounts; replace the literal email/password passed to
firebaseLoginEmailPassword('AnotherTest@email.com','Password') with values
imported from SPEC_CONFIG (or obtain them via a fixture loader) and update any
dependent calls (sendVerificationEmail(loginInfo.data.idToken) and
getOobCode(..., email)) to use the same injected email variable so no hardcoded
credentials remain in the test file.
In `@tests/e2e/web/specs/signIn.spec.ts`:
- Line 54: Replace the console.log call in the skipped test: locate the call to
console.log(profileResults, age) in signIn.spec.ts and either remove it or
replace it with the project logging utility (e.g., debug(...) or log(...))
consistent with test conventions; ensure you import or use the existing
debug/log helper used across tests and pass the same values (profileResults,
age) to it, or simply delete the statement if it was only for temporary
debugging.
- Around line 33-37: Replace the silent early returns with explicit precondition
assertions so tests fail loudly when data is missing: instead of "if
(!totalProfiles || !filterdProfiles) return", assert the presence/visibility of
totalProfiles and filterdProfiles (e.g.,
expect(totalProfiles).not.toBeNull()/toBeDefined()/toMatch(...) or a visibility
check) before running the parseInt comparison; likewise replace "if (!results)
return" after calling app.people.getProfileInfo() with an assertion like
expect(results).toBeDefined()/not.toBeNull() so the test fails if
getProfileInfo() returns empty. Apply this change to the other similar guard
clauses listed (the pairs around lines 60-61, 71-72, 85-86, 96-97, 110-111,
124-125, 140-141, 154-155, 168-169, 178-179, 193-194, 216-217) referencing the
same variables or calls.
---
Outside diff comments:
In `@tests/e2e/utils/seedDatabase.ts`:
- Around line 161-167: verifyEmail(...) is currently called whenever
verifyUserEmail is true even if Firebase auth fell back; change the condition so
verifyEmail is only invoked when the Firebase path succeeded by checking the
firebaseSignUp result (firebaseId). Update the verification call in the seed
flow (around firebaseSignUp, seedDbUser, verifyEmail) to run only when
verifyUserEmail && firebaseId is truthy, using the firebaseId variable and
existing userInfo.email/userInfo.password arguments.
In `@tests/e2e/web/pages/peoplePage.ts`:
- Around line 458-461: The code selects a random profile without handling empty
results: check this.profileResults.count() and if totalResults === 0 throw or
return an explicit error/message (e.g., "No profiles available to select")
before computing chosenProfileNumber; only compute chosenProfileNumber and call
this.profileResults.nth(chosenProfileNumber) when totalResults > 0 so nth() is
never called with an invalid index (references: profileResults, totalResults,
chosenProfileNumber, chosenProfile, getByTestId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f0d2adf-2678-4a25-b702-daba30905037
📒 Files selected for processing (23)
common/src/choices.tstests/e2e/utils/contextManager.tstests/e2e/utils/firebaseUtils.tstests/e2e/utils/seedDatabase.tstests/e2e/web/SPEC_CONFIG.tstests/e2e/web/fixtures/signInFixture.tstests/e2e/web/pages/app.tstests/e2e/web/pages/messagesPage.tstests/e2e/web/pages/peoplePage.tstests/e2e/web/specs/firebaseAccountCreationTest.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/specs/signUp.spec.tsweb/components/chat/chat-message.tsxweb/components/comments/comment-input.tsxweb/components/filters/filters.tsxweb/components/icons.tsxweb/components/profile-grid.tsxweb/components/searches/button.tsxweb/components/select-users.tsxweb/components/widgets/hide-profile-button.tsxweb/components/widgets/star-button.tsxweb/pages/messages/[channelId].tsxweb/pages/messages/index.tsx
| await this.searchBox.click() | ||
| await this.searchBox.fill(item) | ||
| await this.page.keyboard.press('Enter') | ||
| await this.page.waitForTimeout(1000) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file
FILE="tests/e2e/web/pages/peoplePage.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE" >&2
exit 1
fi
# Print line range around 273 (and a bit more context)
python3 - <<'PY'
import itertools
path="tests/e2e/web/pages/peoplePage.ts"
start=240
end=320
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end:
break
PY
# Show where waitForTimeout is used in this file
rg -n "waitForTimeout" "$FILE" || true
# Extract functions/sections likely referenced
python3 - <<'PY'
import re
path="tests/e2e/web/pages/peoplePage.ts"
patterns=["def getProfileInfo","getProfileInfo","messageProfile","messageProfile("]
with open(path,'r',encoding='utf-8') as f:
text=f.read()
for pat in patterns:
m=re.search(re.escape(pat), text)
print(pat, "->", "FOUND" if m else "NOT FOUND")
PY
# Print around getProfileInfo and messageProfile definitions (best-effort by searching for "getProfileInfo" / "messageProfile")
python3 - <<'PY'
import re
path="tests/e2e/web/pages/peoplePage.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def show_around(needle, span=80):
for idx,l in enumerate(lines):
if needle in l:
start=max(0, idx-span)
end=min(len(lines), idx+span)
print("\n--- around", needle, "at", idx+1, "---")
for i in range(start,end):
print(f"{i+1:4d}: {lines[i].rstrip()}")
return True
return False
show_around("getProfileInfo", span=90)
show_around("messageProfile", span=110)
PYRepository: CompassConnections/Compass
Length of output: 18420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/e2e/web/pages/peoplePage.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE" >&2
exit 1
fi
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
start=240
end=320
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end:
break
PY
rg -n "waitForTimeout" "$FILE" || true
python3 - <<'PY'
import re
path="tests/e2e/web/pages/peoplePage.ts"
with open(path,'r',encoding='utf-8') as f:
text=f.read()
for pat in ["getProfileInfo", "messageProfile"]:
m=re.search(re.escape(pat), text)
print(pat, "->", "FOUND" if m else "NOT FOUND")
PY
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def show_around(needle, span=80):
for idx,l in enumerate(lines):
if needle in l:
start=max(0, idx-span)
end=min(len(lines), idx+span)
print("\n--- around", needle, "at", idx+1, "---")
for i in range(start,end):
print(f"{i+1:4d}: {lines[i].rstrip()}")
return True
return False
show_around("getProfileInfo", span=90)
show_around("messageProfile", span=110)
PYRepository: CompassConnections/Compass
Length of output: 18363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/e2e/web/pages/peoplePage.ts"
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
start=240
end=320
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end:
break
PY
rg -n "waitForTimeout" "$FILE" || true
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
needles=["getProfileInfo","messageProfile"]
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def show(needle,span):
for idx,l in enumerate(lines):
if needle in l:
print("\n--- around",needle,"at",idx+1,"---")
for i in range(max(0,idx-span), min(len(lines),idx+span)):
print(f"{i+1:4d}: {lines[i].rstrip()}")
return
for n in needles:
show(n,100)
PYRepository: CompassConnections/Compass
Length of output: 18315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/e2e/web/pages/peoplePage.ts"
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
start=240
end=320
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:4d}: {line.rstrip()}")
if i>end:
break
PY
rg -n "waitForTimeout" "$FILE" || true
python3 - <<'PY'
path="tests/e2e/web/pages/peoplePage.ts"
needles=["getProfileInfo","messageProfile"]
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def show(needle,span):
for idx,l in enumerate(lines):
if needle in l:
print("\n--- around",needle,"at",idx+1,"---")
for i in range(max(0,idx-span), min(len(lines),idx+span)):
print(f"{i+1:4d}: {lines[i].rstrip()}")
return
for n in needles:
show(n,100)
PYRepository: CompassConnections/Compass
Length of output: 18315
Replace fixed waitForTimeout and tighten profile selection/messaging logic in peoplePage e2e
- In
useSearch, removeawait this.page.waitForTimeout(1000)and wait for a concrete UI condition (results/update) using Playwright auto-waiting. - In
getProfileInfo, guard thetotalResults === 0path before computingMath.floor(Math.random() * totalResults)/ callingnth(...). - In
messageProfile, stop after the first matching profile (addreturn/break) so it doesn’t potentially send multiple messages when duplicate display names exist.
🤖 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 `@tests/e2e/web/pages/peoplePage.ts` at line 273, In useSearch: remove the
fixed await this.page.waitForTimeout(1000) and instead wait for a concrete UI
condition (e.g., waitForSelector of the results container,
waitForResponse/locator.waitFor or use Playwright auto-wait on an action that
updates results) so the test relies on actual DOM changes; in getProfileInfo:
check for totalResults === 0 and return/handle early before calling
Math.floor(Math.random() * totalResults) or using locator.nth(...) to avoid
out-of-range selection; in messageProfile: once you find and message the first
matching profile (by display name) exit the loop or return immediately so you
don’t send messages to multiple profiles with duplicate names.
Comment on lines
+11
to
+14
| const loginInfo = await firebaseLoginEmailPassword('AnotherTest@email.com', 'Password') | ||
| await sendVerificationEmail(loginInfo.data.idToken) | ||
| const oobResponse = await axios.get(`${config.FIREBASE_URL.FIREBASE_EMULATOR_API}`) | ||
| const oobCode = await getOobCode(oobResponse.data.oobCodes, 'AnotherTest@email.com') |
There was a problem hiding this comment.
Remove hardcoded credentials from this spec flow.
This setup hardcodes email/password values, which makes test creds leak-prone and harder to rotate. Load them from SPEC_CONFIG.ts or fixture-backed accounts instead.
As per coding guidelines, "No hardcoded credentials in spec files; use SPEC_CONFIG.ts or account fixtures."
🤖 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 `@tests/e2e/web/specs/firebaseAccountCreationTest.ts` around lines 11 - 14, The
spec currently hardcodes credentials when calling firebaseLoginEmailPassword and
should instead read test credentials from SPEC_CONFIG.ts or fixture accounts;
replace the literal email/password passed to
firebaseLoginEmailPassword('AnotherTest@email.com','Password') with values
imported from SPEC_CONFIG (or obtain them via a fixture loader) and update any
dependent calls (sendVerificationEmail(loginInfo.data.idToken) and
getOobCode(..., email)) to use the same injected email variable so no hardcoded
credentials remain in the test file.
| await sendVerificationEmail(loginInfo.data.idToken) | ||
| const oobResponse = await axios.get(`${config.FIREBASE_URL.FIREBASE_EMULATOR_API}`) | ||
| const oobCode = await getOobCode(oobResponse.data.oobCodes, 'AnotherTest@email.com') | ||
| console.log(oobCode) |
There was a problem hiding this comment.
Replace console.log with repo logger utilities.
Use debug()/log() instead of console.log for these debug statements.
As per coding guidelines, "Do not use console.log; use debug() or log() instead."
Also applies to: 23-24
🤖 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 `@tests/e2e/web/specs/firebaseAccountCreationTest.ts` at line 15, Replace the
console.log debug prints with the repository logger calls: import and use the
debug() (or log()) helper instead of console.log for the oobCode print and the
other occurrences noted (lines 23-24); update the call sites (the
console.log(oobCode) and any other console.log usages in this spec) to
debug(oobCode) or log(...) and ensure the logger import/initialization used
throughout tests (e.g., debug or log symbol) is present at the top of the file
so the tests compile.
Comment on lines
+33
to
+37
| if (!totalProfiles || !filterdProfiles) return | ||
| await expect(parseInt(totalProfiles)).not.toEqual(parseInt(filterdProfiles)) | ||
|
|
||
| const results = await app.people.getProfileInfo() | ||
| if (!results) return |
There was a problem hiding this comment.
Avoid silent-pass early returns in assertions.
These guard clauses let tests exit without validating behavior when data is missing, so failures can be masked. Prefer explicit precondition assertions (expect(...).not.toBeNull() / visibility checks) before continuing.
Suggested pattern
- if (!totalProfiles || !filterdProfiles) return
- await expect(parseInt(totalProfiles)).not.toEqual(parseInt(filterdProfiles))
+ expect(totalProfiles).not.toBeNull()
+ expect(filterdProfiles).not.toBeNull()
+ await expect(Number(totalProfiles)).not.toEqual(Number(filterdProfiles))Also applies to: 60-61, 71-72, 85-86, 96-97, 110-111, 124-125, 140-141, 154-155, 168-169, 178-179, 193-194, 216-217
🤖 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 `@tests/e2e/web/specs/signIn.spec.ts` around lines 33 - 37, Replace the silent
early returns with explicit precondition assertions so tests fail loudly when
data is missing: instead of "if (!totalProfiles || !filterdProfiles) return",
assert the presence/visibility of totalProfiles and filterdProfiles (e.g.,
expect(totalProfiles).not.toBeNull()/toBeDefined()/toMatch(...) or a visibility
check) before running the parseInt comparison; likewise replace "if (!results)
return" after calling app.people.getProfileInfo() with an assertion like
expect(results).toBeDefined()/not.toBeNull() so the test fails if
getProfileInfo() returns empty. Apply this change to the other similar guard
clauses listed (the pairs around lines 60-61, 71-72, 85-86, 96-97, 110-111,
124-125, 140-141, 154-155, 168-169, 178-179, 193-194, 216-217) referencing the
same variables or calls.
| const profileResults = await app.people.getProfileInfo() | ||
| const profileAge = parseInt(profileResults.ageGender.match(/\d+/)?.[0] ?? '0') | ||
| const age = profileAge <= 60 ? profileAge : 60 | ||
| console.log(profileResults, age) |
There was a problem hiding this comment.
Replace console.log in the skipped test.
Use debug()/log() (or remove the statement) instead of console.log.
As per coding guidelines, "Do not use console.log; use debug() or log() instead."
🤖 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 `@tests/e2e/web/specs/signIn.spec.ts` at line 54, Replace the console.log call
in the skipped test: locate the call to console.log(profileResults, age) in
signIn.spec.ts and either remove it or replace it with the project logging
utility (e.g., debug(...) or log(...)) consistent with test conventions; ensure
you import or use the existing debug/log helper used across tests and pass the
same values (profileResults, age) to it, or simply delete the statement if it
was only for temporary debugging.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@tests/e2e/web/pages/messagesPage.ts`:
- Line 74: Remove the hard-coded sleep call this.page.waitForTimeout(1000) in
messagesPage and replace it with Playwright auto-waiting: wait for the UI
element represented by messagesRow (e.g., this.messagesRow.first()) to be
visible or present using Playwright's expect/toBeVisible or a
waitForSelector/waitForURL call with a timeout (e.g., 5s) so the test
synchronizes reliably without fixed sleeps.
- Line 69: The call to verifyMessage in the test currently ignores its boolean
return, allowing silent failures; update the test to check the return value of
verifyMessage(message) and throw or assert (e.g., expect/throw) if it returns
false so the test fails when the message is not found — locate the invocation of
verifyMessage and add a conditional/assert that fails the test when
verifyMessage(...) === false, ensuring the message verification step enforces
expected outcome.
- Line 91: Replace the explicit sleep await this.page.waitForTimeout(1000) with
Playwright auto-waiting: assert the UI element you expect to appear (e.g.,
this.conversationMessage.first()) using an expectation or waitForSelector so the
test waits deterministically (for example use
expect(this.conversationMessage.first()).toBeVisible({ timeout: 5000 }) or
this.page.waitForSelector on the conversation message locator); update the
messagesPage method containing await this.page.waitForTimeout to remove the
timeout and use the locator-based wait/assert (reference: conversationMessage,
this.page, messagesPage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a187db6a-7f52-49a0-adc7-3533bf5951dd
📒 Files selected for processing (5)
tests/e2e/utils/contextManager.tstests/e2e/utils/firebaseUtils.tstests/e2e/web/pages/messagesPage.tstests/e2e/web/pages/peoplePage.tstests/e2e/web/specs/signIn.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/utils/contextManager.ts
- tests/e2e/utils/firebaseUtils.ts
- tests/e2e/web/pages/peoplePage.ts
- tests/e2e/web/specs/signIn.spec.ts
|
|
||
| async findMessageConversation(displayName: string) { | ||
| await expect(this.messagesTable).toBeVisible() | ||
| await this.page.waitForTimeout(1000) |
There was a problem hiding this comment.
Remove waitForTimeout – use Playwright's built-in auto-waiting instead.
This violates the explicit guideline: "Never use page.waitForTimeout(). Use Playwright's built-in auto-waiting or waitForURL / waitForSelector."
Fixed timeouts introduce flakiness and are discouraged as "sleep hacks for eventual consistency." Playwright's count() and visibility checks already include auto-waiting, making this unnecessary.
🔧 Proposed fix to remove the timeout
async findMessageConversation(displayName: string) {
await expect(this.messagesTable).toBeVisible()
- await this.page.waitForTimeout(1000)
const doMessagesExist = (await this.messagesRow.count()) > 0If additional synchronization is needed, use:
await expect(this.messagesRow.first()).toBeVisible({timeout: 5000})As per coding guidelines: Never use page.waitForTimeout(). Use Playwright's built-in auto-waiting or waitForURL / waitForSelector.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await this.page.waitForTimeout(1000) | |
| async findMessageConversation(displayName: string) { | |
| await expect(this.messagesTable).toBeVisible() | |
| const doMessagesExist = (await this.messagesRow.count()) > 0 |
🤖 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 `@tests/e2e/web/pages/messagesPage.ts` at line 74, Remove the hard-coded sleep
call this.page.waitForTimeout(1000) in messagesPage and replace it with
Playwright auto-waiting: wait for the UI element represented by messagesRow
(e.g., this.messagesRow.first()) to be visible or present using Playwright's
expect/toBeVisible or a waitForSelector/waitForURL call with a timeout (e.g.,
5s) so the test synchronizes reliably without fixed sleeps.
|
|
||
| async verifyMessage(messageContent: string) { | ||
| await expect(this.conversation).toBeVisible() | ||
| await this.page.waitForTimeout(1000) |
There was a problem hiding this comment.
Remove waitForTimeout – use Playwright's built-in auto-waiting instead.
This violates the explicit guideline: "Never use page.waitForTimeout(). Use Playwright's built-in auto-waiting or waitForURL / waitForSelector."
Fixed timeouts introduce flakiness and non-determinism. Playwright's count() and element queries already include auto-waiting.
🔧 Proposed fix to remove the timeout
async verifyMessage(messageContent: string) {
await expect(this.conversation).toBeVisible()
- await this.page.waitForTimeout(1000)
const messageCount = (await this.conversationMessage.count()) > 0If you need to wait for the first message to appear, use:
await expect(this.conversationMessage.first()).toBeVisible({timeout: 5000})As per coding guidelines: Never use page.waitForTimeout(). Use Playwright's built-in auto-waiting or waitForURL / waitForSelector.
🤖 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 `@tests/e2e/web/pages/messagesPage.ts` at line 91, Replace the explicit sleep
await this.page.waitForTimeout(1000) with Playwright auto-waiting: assert the UI
element you expect to appear (e.g., this.conversationMessage.first()) using an
expectation or waitForSelector so the test waits deterministically (for example
use expect(this.conversationMessage.first()).toBeVisible({ timeout: 5000 }) or
this.page.waitForSelector on the conversation message locator); update the
messagesPage method containing await this.page.waitForTimeout to remove the
timeout and use the locator-based wait/assert (reference: conversationMessage,
this.page, messagesPage).
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary by CodeRabbit
New Features
Tests
Chores