Enhance the application model and render new fields#1039
Enhance the application model and render new fields#1039Dhirenderchoudhary wants to merge 9 commits intoRealDevSquad:developfrom
Conversation
WalkthroughAdds a "Request Changes" button to the application details interface with accompanying feedback submission flow. Introduces new utility functions for nudging applications and adding feedback. Updates styling to support application status badges and a highlights section displaying application score and nudge count. Extends mock data with new application fields. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Request Changes<br/>Button
participant Script as script.js<br/>(updateUserApplication)
participant Utils as utils.js<br/>(addApplicationFeedback)
participant API as Backend API
User->>UI: Click "Request Changes"
UI->>Script: Trigger with isRequestChanges: true
Script->>Script: Validate feedback exists
Script->>Utils: Call addApplicationFeedback(id, feedbackData)
Utils->>API: PATCH /applications/{id}/feedback
API-->>Utils: Response
Utils-->>Script: Return feedback update result
Script->>API: updateApplication call
API-->>Script: Success response
Script->>UI: Show success toast
Script->>UI: Close application details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/script.js (1)
121-156:⚠️ Potential issue | 🟠 Major
Promise.allcan leave the application in a partially updated state.If
addApplicationFeedbacksucceeds butupdateApplicationfails (or vice versa), the application ends up with feedback submitted but status unchanged, or status changed but feedback lost.Promise.allrejects on the first failure, so the success toast is never shown, but the successful call's side effect has already persisted server-side.Consider sequencing these calls (feedback first, then status update) so you can handle partial failure more gracefully, or at minimum document/accept this as a known trade-off.
🤖 Fix all issues with AI agents
In `@applications/index.html`:
- Around line 135-140: Update the button with class
"application-details-request-changes" to use a clearer aria-label: replace
"Request Changes Application" with "Request changes for this application" so
screen readers read a natural phrase; locate the <button> element that currently
sets aria-label and update that attribute value accordingly.
In `@applications/script.js`:
- Around line 92-95: The current branch that handles isRequestChanges silently
returns when applicationTextarea is missing or empty; change this so it
validates and informs the user instead of returning silently: inside the
isRequestChanges handling (the block referencing isRequestChanges and
applicationTextarea), replace the early return with a validation flow that
displays a visible error/validation message (e.g., render or call an existing
showValidationError/toast function), set focus on applicationTextarea, and
prevent submission until non-empty input is provided; keep the existing guard
for null textarea but ensure the user sees the error rather than no-op.
- Around line 89-116: In updateUserApplication, the isRequestChanges branch
incorrectly calls showToast directly instead of the dev-aware wrapper
showToastMessage; replace the showToast(...) calls inside the isRequestChanges
branch with showToastMessage(...) using the same message and type payload so the
request-changes flow uses the same toast system as the accept/reject path, and
ensure showToastMessage is available in the module scope (same usage pattern as
in the accept/reject branch).
In `@applications/style.css`:
- Around line 779-822: The button width rules for selectors
`.application-details .application-details-actions .application-details-reject`,
`.application-details .application-details-actions .application-details-accept`,
`.application-details .application-details-actions .application-details-nudge`,
and `.application-details .application-details-actions
.application-details-request-changes` are currently placed outside the `@media
(max-width: 850px)` block and therefore apply globally; wrap these width
declarations in an appropriate media query (e.g., a desktop/mid-range breakpoint
such as `@media (min-width: 851px)` or the same breakpoint group used for other
desktop styles) so the 18%/24% widths only apply on larger viewports, or
alternatively move them into the global button styles block if they truly are
intended to be global.
- Around line 462-496: The three rules for .description.status-pending,
.description.status-accepted, and .description.status-rejected duplicate
padding, border-radius, display, color, and text-transform; extract those shared
declarations into a single base class (e.g., .description.status-badge) and
leave only background-color in the specific selectors
(.description.status-pending, .description.status-accepted,
.description.status-rejected); update the CSS selectors to use
.description.status-badge alongside the existing nested context
(.application-details .application-details-main .application-section) so
specificity is preserved and only background-color varies per status.
In `@applications/utils.js`:
- Around line 120-140: Remove the unused exported function nudgeApplication from
applications/utils.js: delete the async function nudgeApplication and its export
so the module no longer exports an unused symbol; if you intend to keep it for a
future PR instead, move the implementation to a feature branch or comment it out
and add a TODO explaining where it will be wired up, but do not leave an
exported, unused function in this PR.
In `@mock-data/applications/index.js`:
- Around line 5-7: The mock data only includes applicationScore and nudgeCount
on the first object, so update the mock arrays fetchedApplications,
acceptedApplications, and pendingApplications to include those fields on
multiple entries with varied values (e.g., present with >0, present as 0, and
omitted) so tests exercise both the numeric rendering and the 'N/A' fallback;
locate the objects inside
fetchedApplications/acceptedApplications/pendingApplications in
mock-data/applications/index.js and add applicationScore and nudgeCount
properties across several entries accordingly.
…efactor application status badge styling, and update dependencies.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@applications/script.js`:
- Around line 134-141: The accept/reject branch currently checks
applicationTextarea.value directly, allowing whitespace-only feedback; mirror
the isRequestChanges path by trimming input first: compute const feedback =
applicationTextarea.value.trim() (or use applicationTextarea.value &&
applicationTextarea.value.trim()) and only push
addApplicationFeedback(currentApplicationId, { feedback, status:
applicationStatus }) when feedback is non-empty, ensuring you reference
applicationTextarea, addApplicationFeedback, currentApplicationId, and
applicationStatus so the validation is consistent across both code paths.
- Around line 739-741: The click handler passes a redundant isAccepted flag to
updateUserApplication even though updateUserApplication short-circuits when
isRequestChanges is true; remove the misleading parameter and call
updateUserApplication({ isRequestChanges: true }) from
applicationRequestChangesButton's listener so only the meaningful property is
supplied (reference: applicationRequestChangesButton and updateUserApplication
with the isRequestChanges early-return).
In `@applications/style.css`:
- Around line 790-819: Remove the orphaned CSS rules for the non-existent
.application-details-nudge selector or add the missing element; locate the rules
under the .application-details .application-details-actions
.application-details-nudge block and either delete all related selectors
(including the :is(:hover, :active, :focus-visible), :hover and :active variants
and the bounceBackAnimation usage) or implement a corresponding button element
(e.g., class="application-details-nudge") in the application details markup
where nudgeCount is shown so the styles apply.
…nager, improve API base URL loading, and refine application feedback and toast handling.
Dhirenderchoudhary
added
enhancement
…base URL environment configuration, and upgrade to Yarn 4.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
…and refine API base URL loading logic.
| </button> | ||
| <button | ||
| class="application-details-request-changes" | ||
| aria-label="Request changes for this application" |
There was a problem hiding this comment.
add data-testId and id also in the button so we can use the id for getting this button and data-testid for tests
There was a problem hiding this comment.
In whole index.html we didn't using id so if i use for one it breaks consistency
either have to give id to all elements suck as accept reject
There was a problem hiding this comment.
it't not about the consistency issue with the using the query selector is that you are using the css class for the selection and in future we can use the same css class for any other component and it will get the first one it find out , I think id is preferred as we want to find out the particular button
applications/script.js
Outdated
| const applicationRejectButton = document.querySelector( | ||
| '.application-details-reject', | ||
| ); | ||
| const applicationRequestChangesButton = document.querySelector( |
There was a problem hiding this comment.
we can use getElementById here
There was a problem hiding this comment.
same reason as above in script.js we used queryselector for all elements
There was a problem hiding this comment.
done used getelementbyid
| createToast(type, message); | ||
| } else if (oldToastFunction.length === 1) { | ||
| oldToastFunction(message); | ||
| oldToastFunction({ message, type }); |
There was a problem hiding this comment.
same here why are we changing in the common component?
There was a problem hiding this comment.
To handle success/error styling.
There was a problem hiding this comment.
The previous call passed only a string
There was a problem hiding this comment.
don't see the new format of the application here you can check the application response new one for the correct response structure or check the backend repo
There was a problem hiding this comment.
this is also not replied
There was a problem hiding this comment.
what you mean by new format i did that according to my knowledge and changes the test cases
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
I don't see any test attached to this pr or any test pr link make sure to add them
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
I don't see the admin feedback api changes in this PR , make sure to include them and test your changes with your local backend or staging backend not with mock data please
…and enhance styling for application statuses
Dhirenderchoudhary
force-pushed
the
develop
branch
from
e3ddb4f to
4209c6d
Compare
| createToast(type, message); | ||
| } else if (oldToastFunction.length === 1) { | ||
| oldToastFunction(message); | ||
| oldToastFunction({ message, type }); |
There was a problem hiding this comment.
this is also not replied
| .application-details | ||
| .application-details-actions | ||
| .application-details-request-changes:is(:hover, :active, :focus-visible) { | ||
| background: var(--color-pending); | ||
| color: var(--white); | ||
| transition: transform 0.2s ease; | ||
| will-change: transform; | ||
| } | ||
|
|
There was a problem hiding this comment.
why us nested selection?
There was a problem hiding this comment.
I used the nested selectors to match the existing pattern in the file. The other buttons (Accept and Reject) .
There was a problem hiding this comment.
used the same flow didn't tried to break the flow
| ); | ||
| applicationRequestChangesButton.addEventListener('click', () => { | ||
| updateUserApplication({ isRequestChanges: true }); | ||
| }); |
| </button> | ||
| <button | ||
| class="application-details-request-changes" | ||
| aria-label="Request changes for this application" |
There was a problem hiding this comment.
it't not about the consistency issue with the using the query selector is that you are using the css class for the selection and in future we can use the same css class for any other component and it will get the first one it find out , I think id is preferred as we want to find out the particular button
Still don't see the test or Test PR link attached |
…uttons after updates, add success toast for requesting changes, and update Yarn configuration.
2 participants
Date: 11 Feb 2026
Developer Name: Dhirender
Issue Ticket Number
#1037
Tech Doc Link
Business Doc Link
Description
We are revamping the flow to join RDS and as part of this revamp, new fields have been introduced such as nudge count, application score, etc. These fields are currently not rendered in the application details modal. Apart from this currently the admins can only accept/reject an application. But in this revamp we are introducing the option to request changes on an application. This issue ticket aims at adding the new fields which are being stored but not rendered in the application detail modal and adding a button to request changes on an application. Apart from this we will also improve the UI of the application.
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Screen.Recording.2026-02-11.at.6.52.17.PM.mov
Test Coverage
Screenshot 1
Additional Notes