fix: api client testing UI enhancement and refresh fix#3723
Conversation
…into api-testing-ui-enhancement-and-refresh-fix
|
|
WalkthroughThis PR updates API Client UI and small interaction behavior: tweaks tab/count styles (padding, border, border-radius, badge backgrounds), adds distinct tab-dot classes for success/error, refactors RequestTabLabel to accept a dotIndicator and delegates indicator rendering to RequestTabLabelIndicator, threads an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 4
🧹 Nitpick comments (2)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (2)
103-120: Consider reducing code duplication in conditional rendering.Both branches render the same
.test-results-listwrapper with similar mapping logic. Consider refactoring to reduce duplication:- { isResultRefreshing ? ( - <div className="test-results-list"> - {filteredTestResults.map((testResult, index) => ( - <div key={index} className="test-result-item"> - <Skeleton.Button size="small"/> - <Skeleton.Button shape="round" size="small" block={true} /> - </div> - ))} - </div> - ) : ( - - <div className="test-results-list"> - {filteredTestResults.map((testResult, index) => ( - <TestResultItem key={index} testResult={testResult} /> - ))} - </div> - ) - } + <div className="test-results-list"> + {filteredTestResults.map((testResult, index) => + isResultRefreshing ? ( + <div key={index} className="test-result-item"> + <Skeleton.Button size="small"/> + <Skeleton.Button shape="round" size="small" block={true} /> + </div> + ) : ( + <TestResultItem key={index} testResult={testResult} /> + ) + )} + </div>
96-100: Consider adding accessibility attributes for the loading state.To improve screen reader support, consider adding ARIA attributes to indicate the loading state to assistive technologies.
<RQButton className="tests-refresh-btn" size="small" type="transparent" icon={isResultRefreshing ? <LoadingOutlined/> : <MdRefresh />} onClick={handleRefresh} + aria-busy={isResultRefreshing} + aria-live="polite" > {isResultRefreshing ? "Refreshing..." : "Refresh"} </RQButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/package.json(1 hunks)app/src/features/apiClient/screens/apiClient/apiClient.scss(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/request/components/ApiClientRequestTabs/apiClientRequestTabs.scss(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/request/components/ApiClientRequestTabs/components/RequestTabLabel/RequestTabLabel.tsx(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/ApiClientBottomSheet.tsx(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/apiclientBottomSheet.scss(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx(3 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/testsView.scss(2 hunks)app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T10:01:54.545Z
Learnt from: Aarushsr12
PR: requestly/requestly#3515
File: app/vite.config.ts:0-0
Timestamp: 2025-09-10T10:01:54.545Z
Learning: In the Requestly codebase, certain non-VITE environment variables (NODE_ENV, BUILD_MODE, REACT_APP_ENV, BROWSER, ENV, GITHUB_SHA) are intentionally exposed in the vite.config.ts define mapping alongside VITE_* variables for easier centralized management, rather than handling them separately in individual utility files like EnvUtils.js.
Applied to files:
app/package.json
🧬 Code graph analysis (1)
app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx (2)
shared/src/types/entities/apiClient/api.ts (1)
ExecutionError(403-409)app/src/features/apiClient/types.ts (1)
ExecutionError(227-233)
🪛 Biome (2.1.2)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx
[error] 106-106: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (12)
app/package.json (1)
32-34: Clarify intent: package.json formatting changes appear unrelated to PR objectives.Lines 32–34 show cosmetic formatting changes to npm scripts (quote style updates); the underlying commands and functionality remain unchanged. However, these changes appear unrelated to the PR's stated objectives (UI enhancements for API testing: error indicators, refresh state, test results styling).
This could be:
- An accidental commit from an unrelated linting/formatting pass
- A separate cosmetic update that should be split into its own PR for clarity
- Intentional housekeeping
As a best practice, please confirm whether these formatting changes were intentional and, if so, whether they should be grouped with feature work or separated for cleaner git history and PR focus.
app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/ApiClientBottomSheet.tsx (1)
66-66: LGTM!The removal of parentheses from the test results display improves readability and aligns with the UI enhancement objectives.
app/src/features/apiClient/screens/apiClient/apiClient.scss (1)
152-157: LGTM!The updated padding and border-radius provide better visual balance for count badges, consistent with the UI enhancement goals.
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
801-801: LGTM!Passing the error prop to HttpRequestTabs correctly enables error-aware UI behavior in the Scripts tab, fulfilling a key PR objective.
app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/apiclientBottomSheet.scss (1)
19-27: LGTM!The background color additions for passed/failed states provide clear visual feedback, enhancing the test results display.
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/testsView.scss (2)
39-40: LGTM!Adding flexbox alignment ensures the filter buttons display correctly with their badge indicators.
65-70: LGTM!The padding and border-radius adjustments create visual consistency with other badge elements across the API client UI.
app/src/features/apiClient/screens/apiClient/components/views/components/request/components/ApiClientRequestTabs/apiClientRequestTabs.scss (1)
42-54: LGTM!The dot styling split into success and error variants cleanly supports the new error indicator feature for the Scripts tab.
app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx (2)
54-55: Clean error detection logic.The hasScriptError computation correctly identifies script-related errors for the dot indicator.
164-164: LGTM!Correctly added hasScriptError to the useMemo dependencies to ensure the tab items recompute when error state changes.
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (2)
4-4: LGTM! New imports support the refresh loading state.The added
Skeleton,Logger, andLoadingOutlinedimports are correctly used for the refresh enhancement functionality.Also applies to: 10-11
96-99: LGTM! Clear visual feedback during refresh.The dynamic icon and text provide good user feedback during the refresh operation.
...nents/request/components/ApiClientRequestTabs/components/RequestTabLabel/RequestTabLabel.tsx
Show resolved
Hide resolved
...res/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx
Show resolved
Hide resolved
...res/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx
Outdated
Show resolved
Hide resolved
...lient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx
Show resolved
Hide resolved
|
@mustafa-sayyed Can you please address the above Coderabbit comments? |
|
@mustafa-sayyed Design implementation looks good to me. But tabs count color should not be red when all tests are passed. Please refer the screenshot below: 
|
|
@vijaygupta-bstack I will fix this logic Thanks @vijaygupta-bstack for pointing out |
rohanmathur91
left a comment
rohanmathur91
left a comment
There was a problem hiding this comment.
@mustafa-sayyed Do match the design, skeleton row style and height differs from design
Current:
Expected:
|
Ok, I will make changes in the height of the skeletons But should I add 3 column? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (1)
91-99: Disable the refresh button during refresh operation.The button should be disabled while refreshing to prevent users from triggering multiple simultaneous refresh operations, which could lead to race conditions or unexpected behavior.
Apply this diff to disable the button during refresh:
<RQButton className="tests-refresh-btn" size="small" type="transparent" + disabled={isResultRefreshing} icon={isResultRefreshing ? <LoadingOutlined /> : <MdRefresh />} onClick={handleRefresh} >
🧹 Nitpick comments (1)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (1)
102-117: Key prop issue fixed; consider using stable identifiers.The key prop issue from the previous review has been correctly resolved—the key is now properly placed on the outer
divelement.However, both the skeleton and actual result rendering use
indexas the key (lines 105, 114). IfTestResulthas a stable identifier field (e.g.,id,name, ortestName), prefer using that instead of the array index to ensure correct React reconciliation when the filtered list changes.If
TestResulthas a stable identifier, apply this pattern:- <div key={index} className="test-result-item"> + <div key={testResult.id ?? testResult.name ?? index} className="test-result-item">And similarly for line 114:
- <TestResultItem key={index} testResult={testResult} /> + <TestResultItem key={testResult.id ?? testResult.name ?? index} testResult={testResult} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/ApiClientBottomSheet.tsx(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx(3 hunks)app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx
- app/src/features/apiClient/screens/apiClient/components/views/components/response/ApiClientBottomSheet/ApiClientBottomSheet.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mustafa-sayyed
PR: requestly/requestly#3723
File: app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx:146-151
Timestamp: 2025-10-22T22:57:45.936Z
Learning: In the API Client Scripts tab (app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx), the dot indicator should only appear when scripts are present (count > 0). When no scripts exist, the dot should not be rendered. This is the intended behavior per issue #3731.
📚 Learning: 2025-10-22T22:52:06.496Z
Learnt from: mustafa-sayyed
PR: requestly/requestly#3723
File: app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx:26-35
Timestamp: 2025-10-22T22:52:06.496Z
Learning: In the API Client feature's TestsView component, when refreshing test results via handleTestResultRefresh(), error notifications are not needed in the catch block because the Script Runner already displays error messages to the user when script execution fails.
Applied to files:
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx
🔇 Additional comments (1)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (1)
23-34: Loading state implementation looks good.The refresh flow correctly manages the loading state with proper error handling and cleanup in the finally block. The approach of only logging errors (without user notifications) is appropriate, as the Script Runner already displays error messages to users.
Based on learnings.
…github.com/mustafa-sayyed/requestly into api-testing-ui-enhancement-and-refresh-fix
|
Hey @RuntimeTerror10 I have resolved all the issues |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (1)
106-106: Consider using unique identifiers for React keys.Both the skeleton placeholders and actual test result items use
indexas the key. IfTestResultobjects have a unique identifier (e.g.,testResult.idortestResult.name), using that would improve React's reconciliation and prevent potential issues when the list is filtered or reordered.Apply this diff if a unique identifier is available:
- <div key={index} className="test-result-item"> + <div key={testResult.id ?? index} className="test-result-item">And similarly for line 115:
- <TestResultItem key={index} testResult={testResult} /> + <TestResultItem key={testResult.id ?? index} testResult={testResult} />Also applies to: 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mustafa-sayyed
PR: requestly/requestly#3723
File: app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx:146-151
Timestamp: 2025-10-22T22:57:45.936Z
Learning: In the API Client Scripts tab (app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx), the dot indicator should only appear when scripts are present (count > 0). When no scripts exist, the dot should not be rendered. This is the intended behavior per issue #3731.
📚 Learning: 2025-10-22T22:52:06.496Z
Learnt from: mustafa-sayyed
PR: requestly/requestly#3723
File: app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx:26-35
Timestamp: 2025-10-22T22:52:06.496Z
Learning: In the API Client feature's TestsView component, when refreshing test results via handleTestResultRefresh(), error notifications are not needed in the catch block because the Script Runner already displays error messages to the user when script execution fails.
Applied to files:
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx
🔇 Additional comments (4)
app/src/features/apiClient/screens/apiClient/components/views/components/response/TestsView/TestsView.tsx (4)
4-4: LGTM! Imports are appropriate for the new functionality.The added imports (Skeleton, Logger, LoadingOutlined) correctly support the skeleton-based loading UX and error logging during refresh operations.
Also applies to: 10-11
23-34: LGTM! Refresh logic and error handling are correct.The loading state management and error handling in
handleRefreshare well-structured. The error is logged appropriately, and the finally block ensures the loading state is always reset. Based on learnings, user-facing error notifications are not needed here since the Script Runner already displays error messages when script execution fails.
95-99: LGTM! Button UX enhancements are well-implemented.The refresh button correctly disables during refresh operations and provides clear visual feedback with the loading icon and "Refreshing..." label. This aligns well with the PR objectives for improved UI/UX.
107-108: Verify skeleton height matches actual content.The skeleton buttons have a height of 16px, which appears quite small. Please verify this matches the actual
TestResultItemheight for a more realistic loading state. Consider testing the visual alignment with the actual rendered content.
rohanmathur91
changed the title
rohanmathur91
left a comment
rohanmathur91
left a comment
There was a problem hiding this comment.
Looks good to me.
@mustafa-sayyed Thanks for contributing!
5 participants
Closes issue: #3627
Task Completed
📜 Summary of changes:
🎥 Demo Video:
Screen.Recording.2025-10-18.153006.mp4
✅ Checklist:
🧪 Test instructions:
Scripts Tab Indicators:
Test Results and Refresh behavior:
Summary by CodeRabbit
New Features
Bug Fixes & Improvements