feat: integrate nudge API for new application #1144
Open
MayankBansal12 wants to merge 7 commits intodevelopfrom
Open
feat: integrate nudge API for new application #1144MayankBansal12 wants to merge 7 commits intodevelopfrom
MayankBansal12 wants to merge 7 commits intodevelopfrom
Conversation
Deploying www-rds with
|
| Latest commit: |
db78447
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3a241e2e.www-rds.pages.dev |
| Branch Preview URL: | https://feat-nudge-api.www-rds.pages.dev |
WalkthroughThis PR implements nudge functionality for applications by extending the detail header component with async API calls, loading states, and toast notifications. It also refactors the application detail route to fetch applications by user ID instead of application ID, with supporting utilities and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant DetailHeader as DetailHeader Component
participant Toast as Toast Service
participant NudgeAPI as nudge-api Utility
participant RemoteAPI as Remote API
participant Controller as Detail Controller
User->>DetailHeader: Clicks nudge button
activate DetailHeader
DetailHeader->>DetailHeader: Set isLoading = true
DetailHeader->>DetailHeader: Disable nudge button
DetailHeader->>NudgeAPI: Call nudgeApplication(applicationId)
activate NudgeAPI
NudgeAPI->>RemoteAPI: POST /nudge/{applicationId}
activate RemoteAPI
RemoteAPI-->>NudgeAPI: Response with updated nudge data
deactivate RemoteAPI
deactivate NudgeAPI
DetailHeader->>DetailHeader: Update local nudge data
DetailHeader->>Toast: Show success toast
DetailHeader->>Controller: Emit onNudge with updated data
activate Controller
Controller->>Controller: Update nudgeCount & lastNudgedAt
deactivate Controller
DetailHeader->>DetailHeader: Set isLoading = false
deactivate DetailHeader
User->>User: See updated nudge state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🤖 Fix all issues with AI agents
In `@app/components/application/detail-header.js`:
- Around line 94-100: The client currently builds updatedNudgeData using
this.nudgeCount + 1 and a client-side lastNudgedAt after calling
nudgeApplication(this.application.id), which can diverge from server state;
update the post-nudge logic to read the server response from nudgeApplication
(use returned nudgeCount and lastNudgedAt if present) and only fall back to
computing nudgeCount = this.nudgeCount + 1 and lastNudgedAt = new
Date().toISOString() when the API response lacks those fields, then set the
local state accordingly.
- Line 6: The imported utility nudgeApplication conflicts with the action method
of the same name; rename the component's action to handleNudgeClick, update the
action method declaration (e.g., change the action previously named
nudgeApplication to handleNudgeClick and call the imported nudgeApplication
inside it), and update the template/button to use {{this.handleNudgeClick}} (and
update any other internal references to the old action name).
In `@app/controllers/applications/detail.js`:
- Around line 46-51: The handleNudge action mutates application.nudgeCount and
application.lastNudgedAt directly which won't notify Glimmer; to fix, make the
controller's application reference tracked (add a `@tracked` application property
on the controller class) or update handleNudge to replace the application object
instead of mutating it (e.g. set this.application = { ...this.application,
nudgeCount: nudgeData.nudgeCount, lastNudgedAt: nudgeData.lastNudgedAt });
update the controller class to declare the `@tracked` field if you choose that
approach and ensure handleNudge uses the tracked property or object replacement
so the UI re-renders.
In `@app/routes/applications/detail.js`:
- Line 51: Replace the explicit property assignment "application: application"
with ES6 object shorthand by using just "application" inside the object literal
(locate the object that currently contains the property 'application' in
routes/applications/detail.js and update it to use the shorthand form).
- Around line 47-48: The current code blindly picks applications[0] from
applicationData.applications; update the logic to handle multiple applications:
if applications.length > 1, log a warning including the number of applications
found (use processLogger or the existing logger), then sort the applications
array by creation timestamp (e.g., createdAt or created_at) descending and set
application = applications[0]; alternatively replace this with a selector UI if
UX requires — ensure you reference the variables applicationData, applications
and application when implementing the change.
- Line 16: Restore explicit ID handling in the route: change model() back to
accept params (async model(params)) and use params.id to fetch the requested
application (e.g., call the application lookup by id inside model using
params.id); if params.id is missing and you still need user-based fallback,
explicitly fetch all applications for the current user and either return the
matched application or return the list (do not silently pick applications[0]) so
callers can handle multiples; also ensure callers (e.g., the code that
transitions using this.applicationId) continue to pass the application id or are
updated to transition to a user-based route if you intentionally remove the
'/:id' param.
In `@tests/integration/components/application/detail-header-test.js`:
- Around line 150-162: The test replaces window.fetch with a stub but doesn't
guarantee restoration on failure; update the block that sets const originalFetch
= window.fetch and window.fetch = () => Promise.resolve({ ok: true }) to wrap
the render(...) and await click('[data-test-button="nudge-button"]') calls in a
try/finally so that window.fetch = originalFetch is executed in the finally
clause; locate the replacement around the Application::DetailHeader render in
tests/integration/components/application/detail-header-test.js (symbols:
originalFetch, window.fetch, render, click, '[data-test-button="nudge-button"]')
and move the restoration into finally to ensure cleanup regardless of test
assertions.
- Around line 113-129: The test overrides window.fetch (originalFetch) and
restores it only at the end, which leaves the mock in place if an assertion
fails; wrap the code that sets window.fetch, the
click('[data-test-button="nudge-button"]'), waitFor(...) and settled() calls in
a try block and move window.fetch = originalFetch into a finally block so
window.fetch is always restored even on test failures; reference the existing
symbols originalFetch, resolveNudge, window.fetch, click, waitFor, and settled
when editing the test.
In `@tests/unit/routes/applications/detail-test.js`:
- Around line 81-106: The assertion is referencing this.toast which isn't
defined; update the test to assert against the stubbed toast on the route
(this.route.toast.error) instead of this.toast. Locate the test "displays error
toast on 404 response" and replace the failing assertion that checks
this.toast.error.calledOnce with an assertion that this.route.toast.error was
called (e.g., this.route.toast.error.calledOnce) so it uses the actual stubbed
method used when calling this.route.model.
- Around line 134-151: The test references a missing toast stub and should also
assert the applications API isn't called when userId is missing: ensure the test
initializes a toast error stub on the test context (e.g., set this.toast = {
error: sinon.stub() } or equivalent) before calling this.route.model, keep using
this.fetchStub and this.route.model from the diff, and after invoking
this.route.model assert the toast error stub was called once and that
this.fetchStub was not invoked for the applications endpoint (e.g., assert no
calls/matches to a URL containing '/applications' or that fetchStub.callCount
equals the expected number for only the user fetch).
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
1 participant
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.
Date: 05-02-26
Developer Name: @MayankBansal12
Issue Ticket Number:-
Description:
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