Update/state change #996
Update/state change #996
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThe changes introduce conditional behavior throughout the navbar and request management scripts based on a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant NavbarScript
participant UserLoginScript
User->>Browser: Loads page with or without ?dev=true
Browser->>NavbarScript: Initialize navbar
NavbarScript->>NavbarScript: Check URL for dev flag
alt dev flag is true
NavbarScript->>Browser: Render user image + chevron icon
else dev flag is false
NavbarScript->>Browser: Render user image only
end
User->>UserLoginScript: Clicks user info
UserLoginScript->>UserLoginScript: Toggle dropdown
User->>Browser: Clicks outside dropdown
UserLoginScript->>UserLoginScript: If dev flag, close dropdown
sequenceDiagram
participant Browser
participant RequestsScript
participant API
Browser->>RequestsScript: Load requests page
RequestsScript->>RequestsScript: Check URL for dev flag
RequestsScript->>API: Fetch requests
API-->>RequestsScript: Return requests with 'state' or 'status'
RequestsScript->>RequestsScript: Use appropriate key for status
RequestsScript->>Browser: Render request cards with correct status
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
requests/script.js (1)
592-592: 🧹 Nitpick (assertive)Consider using the dynamic key in the initial request.
The initial call to
renderRequestCardsuses a hardcodedstateparameter rather than the dynamic key based onisDevthat's used elsewhere.-renderRequestCards({ state: statusValue, sort: sortByValue }); +renderRequestCards({ [isDev ? 'status' : 'state']: statusValue, sort: sortByValue });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
images/chevron-down.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
__tests__/navbar/navbar.test.js(2 hunks)navbar.global.js(2 hunks)requests/script.js(8 hunks)userLogin.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
__tests__/navbar/navbar.test.js (3)
navbar.global.js (2)
devFlag(2-2)chevronIcon(3-5)mock-data/constants.js (1)
LOCAL_TEST_PAGE_URL(2-2)userLogin.js (2)
userInfo(33-33)dropdown(32-32)
requests/script.js (2)
task-requests/script.js (1)
isDev(21-21)extension-requests/script.js (1)
state(21-23)
🪛 Biome (1.9.4)
__tests__/navbar/navbar.test.js
[error] 143-143: This let declares a variable that is only assigned once.
'dropdownIsActive' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (10)
navbar.global.js (2)
1-5: Cleanly implemented feature flag for UI changes.The implementation correctly retrieves the
devflag from URL parameters and conditionally renders the chevron icon based on that flag.
39-39: LGTM for the chevron icon integration.The insertion of the conditional chevron icon is done correctly using template literals.
__tests__/navbar/navbar.test.js (2)
19-28: Well-structured test for conditional UI elements.The test correctly checks for the presence/absence of the chevron icon based on the
devflag value.
113-133: Good test coverage for new conditional behavior.This test effectively verifies the dropdown closing behavior when clicking outside with the
devflag enabled.userLogin.js (2)
1-2: Clean implementation of feature flag check.The code correctly retrieves the
devflag from URL parameters.
94-103: Effective conditional dropdown behavior.The implementation correctly adds a document-level click listener that closes the dropdown when clicking outside, but only when the
devflag is enabled. This matches the behavior tested in the navbar test file.requests/script.js (4)
6-6: Cleanly implemented feature flag for API requests.The implementation correctly sets the
isDevflag based on the URL parameter.
30-30: Good implementation of conditional field names.The code correctly uses computed property names to dynamically choose between
statusandstatebased on theisDevflag.Also applies to: 52-52, 63-63
168-169: Good approach for handling both field names.The code extracts both
statusandstatefrom the request object and then sets up a variable that conditionally references the appropriate one based on the feature flag.Also applies to: 178-178
182-182: Consistent usage of conditional field names.All references to the status field have been updated to use the conditional approach.
Also applies to: 188-188, 425-428, 452-452, 527-528, 532-533
| it('should keep the dropdown open when clicking outside when feature flag is off', async () => { | ||
| await page.goto(`${LOCAL_TEST_PAGE_URL}?dev=false`); | ||
| await page.waitForSelector('#dropdown'); | ||
| await page.evaluate(() => { | ||
| const dropdown = document.getElementById('dropdown'); | ||
| if (dropdown && !dropdown.classList.contains('active')) { | ||
| dropdown.classList.add('active'); | ||
| } | ||
| }); | ||
| let dropdownIsActive = await page.evaluate(() => { | ||
| const dropdown = document.getElementById('dropdown'); | ||
| return dropdown?.classList.contains('active') ?? false; | ||
| }); | ||
| expect(dropdownIsActive).toBe(true); | ||
| await page.evaluate(() => { | ||
| document.body.click(); | ||
| }); | ||
| await page.waitForTimeout(200); | ||
| const newDropdownHandle = await page.$('#dropdown'); | ||
| const newDropdownIsActive = await newDropdownHandle.evaluate((el) => | ||
| el.classList.contains('active'), | ||
| ); | ||
| expect(newDropdownIsActive).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use const instead of let for immutable variables.
The variable dropdownIsActive is never reassigned and should use const instead of let.
- let dropdownIsActive = await page.evaluate(() => {
+ const dropdownIsActive = await page.evaluate(() => {📝 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.
| it('should keep the dropdown open when clicking outside when feature flag is off', async () => { | |
| await page.goto(`${LOCAL_TEST_PAGE_URL}?dev=false`); | |
| await page.waitForSelector('#dropdown'); | |
| await page.evaluate(() => { | |
| const dropdown = document.getElementById('dropdown'); | |
| if (dropdown && !dropdown.classList.contains('active')) { | |
| dropdown.classList.add('active'); | |
| } | |
| }); | |
| let dropdownIsActive = await page.evaluate(() => { | |
| const dropdown = document.getElementById('dropdown'); | |
| return dropdown?.classList.contains('active') ?? false; | |
| }); | |
| expect(dropdownIsActive).toBe(true); | |
| await page.evaluate(() => { | |
| document.body.click(); | |
| }); | |
| await page.waitForTimeout(200); | |
| const newDropdownHandle = await page.$('#dropdown'); | |
| const newDropdownIsActive = await newDropdownHandle.evaluate((el) => | |
| el.classList.contains('active'), | |
| ); | |
| expect(newDropdownIsActive).toBe(true); | |
| }); | |
| it('should keep the dropdown open when clicking outside when feature flag is off', async () => { | |
| await page.goto(`${LOCAL_TEST_PAGE_URL}?dev=false`); | |
| await page.waitForSelector('#dropdown'); | |
| await page.evaluate(() => { | |
| const dropdown = document.getElementById('dropdown'); | |
| if (dropdown && !dropdown.classList.contains('active')) { | |
| dropdown.classList.add('active'); | |
| } | |
| }); | |
| const dropdownIsActive = await page.evaluate(() => { | |
| const dropdown = document.getElementById('dropdown'); | |
| return dropdown?.classList.contains('active') ?? false; | |
| }); | |
| expect(dropdownIsActive).toBe(true); | |
| await page.evaluate(() => { | |
| document.body.click(); | |
| }); | |
| await page.waitForTimeout(200); | |
| const newDropdownHandle = await page.$('#dropdown'); | |
| const newDropdownIsActive = await newDropdownHandle.evaluate((el) => | |
| el.classList.contains('active'), | |
| ); | |
| expect(newDropdownIsActive).toBe(true); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 143-143: This let declares a variable that is only assigned once.
'dropdownIsActive' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
ZendeAditya
force-pushed
the
update/state-change
branch
from
ab5ca6a to
94588f8
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| let requesterUserDetails = await getUserDetails(request.requestedBy); | ||
| if ((isDev ? request.status : request.state) !== 'PENDING') { | ||
| superUserDetails = await getUserDetails(request.lastModifiedBy); | ||
| } |
There was a problem hiding this comment.
Duplicate declaration of requesterUserDetails variable
There are two declarations of the requesterUserDetails variable in this code section:
// First declaration
let requesterUserDetails = await getUserDetails(request.requestedBy);
// Second declaration (a few lines later)
let requesterUserDetails = await getUserDetails(
request.type === 'OOO' ? request.requestedBy : request.userId,
);This will cause a syntax error as variables cannot be declared twice in the same scope.
Consider either:
- Removing the first declaration and keeping the more specific second one, or
- Merging the logic into a single declaration that handles both cases appropriately
This needs to be resolved before merging to prevent runtime errors.
| let requesterUserDetails = await getUserDetails(request.requestedBy); | |
| if ((isDev ? request.status : request.state) !== 'PENDING') { | |
| superUserDetails = await getUserDetails(request.lastModifiedBy); | |
| } | |
| let requesterUserDetails; | |
| if ((isDev ? request.status : request.state) !== 'PENDING') { | |
| superUserDetails = await getUserDetails(request.lastModifiedBy); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Date:
Developer Name: aditya-zende
Issue Ticket Number
#2399
Description
Change a
statefield in the request tostatus.change all required occourances of
statetostatusin frontendDocumentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes