-
Notifications
You must be signed in to change notification settings - Fork 153
chore: remove feature flag from mobile page #1019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: remove feature flag from mobile page #1019
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 pull request removes the pre-condition check for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MobileRoute
Client->>MobileRoute: Request /mobile?dev=value
Note over MobileRoute: Removed pre-check for 'dev' parameter
MobileRoute->>MobileRoute: Execute model hook (fetch user profile)
MobileRoute->>Client: Return profile data
Assessment against linked issues
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/routes/mobile.js(0 hunks)tests/unit/routes/mobile-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/routes/mobile.js
🔇 Additional comments (2)
tests/unit/routes/mobile-test.js (2)
10-17: Hooks properly updated after feature flag removal.The beforeEach and afterEach hooks have been correctly simplified to only handle the fetch stub, removing the previously needed transitionTo stub and restore calls that were required for testing the feature flag behavior.
1-52:Details
❓ Verification inconclusive
Confirm test coverage is sufficient after feature flag removal.
The removed tests for feature flag behavior are no longer needed, which aligns with the PR objectives. However, consider whether it might be valuable to add a test that explicitly verifies the route no longer checks for the "dev" query parameter, ensuring complete removal of the feature flag.
🏁 Script executed:
#!/bin/bash # Check the actual mobile route implementation to confirm feature flag has been removed echo "Checking mobile route implementation:" cat app/routes/mobile.js 2>/dev/null || echo "Mobile route file not found at expected location" # Look for any remaining references to the dev parameter echo -e "\nChecking for any remaining references to the dev parameter:" rg -i "dev.*param|param.*dev" --type js appLength of output: 2848
Mobile Route Feature Flag Removal Verification
The mobile route (
app/routes/mobile.js) no longer contains any logic that checks or uses the "dev" query parameter. The tests intests/unit/routes/mobile-test.jsremain valid for the current behavior (fetching user profile data and handling error responses) after the removal of the feature flag logic.
- Verified that the mobile route implementation no longer references the "dev" parameter.
- The existing tests cover API responses and general route behavior. However, to guard against potential regressions, consider adding an explicit test that asserts the absence of "dev" query parameter handling in the mobile route.
|
Please don't remove that until it got tested completely on the main site with dev flag. |
020f3a3 to
785dcf5
Compare
Date: 22-03-2025
Developer Name: @MayankBansal12
Issue Ticket Number:-
/mobilepage #1018Description:
/mobilepage. The feature is concerned with mobile login using qr-code and has been tested and working fine in prod under feature flag.Is Under Feature Flag
Database changes
Breaking changes (If your feature is breaking/missing something please mention pending tickets)
Is Development Tested?
Tested in staging?
Add relevant Screenshot below ( e.g test coverage etc. )
Screencast
screencast.mp4
Tests