-
Notifications
You must be signed in to change notification settings - Fork 95
fix: redirect /tasks to status site
#637
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
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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
WalkthroughA new constant for the status site prefix is added and used to define a tasks redirect URL. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmberRouter
participant TasksRoute
User->>EmberRouter: Navigates to /tasks
EmberRouter->>TasksRoute: beforeModel()
TasksRoute->>EmberRouter: transitionTo('goto', { queryParams: { from: 'tasks' } })
EmberRouter->>User: Navigates to 'goto' route
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for staging-my ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Redundant API Call After Redirect ▹ view | ||
| Missing Deprecation Strategy ▹ view | ||
| Missing Environment Variable Validation ▹ view | ||
| Potentially Unnecessary Query Parameter ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| app/routes/tasks.js | ✅ |
| app/constants/url.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
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: 2
🔭 Outside diff range comments (1)
app/routes/tasks.js (1)
22-39: Consider removing unused model method.Since this route now redirects immediately in
beforeModel, themodelmethod will never be executed. Consider removing it to avoid confusion and reduce dead code.- model = async () => { - try { - const response = await fetch(`${API_BASE_URL}/tasks/self`, { - credentials: 'include', - }); - if (!response.ok) { - if (response.status == 401) { - throw new Error('Please log in to continue'); - } - throw new Error('Oops, We ran into a problem!'); - } - return await response.json(); - } catch (error) { - this.toast.error(error, '', toastNotificationTimeoutOptions); - // added setTimeout here because before new page opens user should be notified of error by toast - setTimeout(redirectAuth, 2000); - } - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
app/constants/url.js(2 hunks)app/routes/tasks.js(1 hunks)tests/integration/components/tasks-test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in ember.js route hooks like `beforemodel`, when using multiple `transitionto` calls, each call shou...
Learnt from: yesyash
PR: Real-Dev-Squad/website-my#632
File: app/routes/notifications.js:17-19
Timestamp: 2025-05-22T18:21:46.220Z
Learning: In Ember.js route hooks like `beforeModel`, when using multiple `transitionTo` calls, each call should be preceded by a `return` statement to prevent double transitions and TransitionAborted warnings. This is especially important when conditional redirects are used, as subsequent transitions would otherwise override earlier ones.
Applied to files:
app/routes/tasks.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - staging-my
- GitHub Check: Header rules - staging-my
- GitHub Check: Pages changed - staging-my
- GitHub Check: build (16.x)
🔇 Additional comments (3)
app/constants/url.js (2)
18-18: LGTM! New constant follows established pattern.The
STATUS_SITE_PREFIXconstant is properly defined using the environment variable, following the same pattern asMAIN_SITE_PREFIX.
29-29: LGTM! Tasks redirect URL correctly configured.The tasks redirect URL properly uses the new
STATUS_SITE_PREFIXand includes thedev=trueparameter consistent with other redirect URLs in the object.app/routes/tasks.js (1)
16-16: Verify the correct issue reference.The comment references issue #1031, but this PR addresses issue #1060. Please verify which issue number is correct.
Date: 04-08-2025
Developer Name: @MayankBansal12
Issue Ticket Number
Description
/tasksto status site (previously left as a ToDo comment during my-site migration)Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Screencast.from.2025-08-05.00-16-52.mp4
Description by Korbit AI
What change is being made?
Redirect
/tasksto the status site by updating the URL constants, adding a route redirection, and marking a specific test as skipped.Why are these changes being made?
The
/taskspath is deprecated and should redirect to the status site for a more consistent user experience, according to the project's redirection strategy. Updating the constants and route code ensures the system correctly handles requests to the deprecated path, and skipping the affected test prevents test inaccuracies while the functionality changes.