test: add unit tests for utility hooks and URLUtils#41660
Open
xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
Open
test: add unit tests for utility hooks and URLUtils#41660xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
xingzihai wants to merge 2 commits intoappsmithorg:releasefrom
Conversation
- Add table of contents for easy navigation - Add project architecture overview with tech stack - Add quick start checklist for new contributors - Add finding issues section with categorized links - Add development setup quick guide - Add pull request process guidelines - Add community support channels - Improve overall structure and organization
- Add tests for usePrevious hook - Add tests for useWindowDimensions hook - Add tests for useOnClickOutside hook - Add tests for URLUtils utility functions These tests cover: - Various data types and edge cases for usePrevious - Window resize events and cleanup for useWindowDimensions - Click/touch outside detection for useOnClickOutside - Query parameter parsing, URL validation, and string sanitization for URLUtils Total: 75 new test cases
Contributor
WalkthroughThis PR updates the contribution documentation with expanded guidance including architecture overview, development setup, and pull request process details, while adding comprehensive test suites for existing client-side utility functions and custom hooks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/client/src/utils/hooks/useOnClickOutside.test.tsx (1)
188-216: Add a rerender case for changed refs with unchanged length.Nice handler-update coverage. Please also add a regression test for swapping
refsto a different element set with the same array length, so stale-ref behavior is caught explicitly.➕ Suggested test addition
+ it("should use updated refs when refs change with same length", () => { + const oldRef = createRef<HTMLDivElement>(); + const newRef = createRef<HTMLDivElement>(); + const oldEl = document.createElement("div"); + const newEl = document.createElement("div"); + document.body.appendChild(oldEl); + document.body.appendChild(newEl); + // `@ts-expect-error` test assignment + oldRef.current = oldEl; + // `@ts-expect-error` test assignment + newRef.current = newEl; + + const { rerender } = renderHook( + ({ refs }) => useOnClickOutside(refs, handler), + { initialProps: { refs: [oldRef] } }, + ); + + rerender({ refs: [newRef] }); // same length, different ref + + oldEl.dispatchEvent(new MouseEvent("mousedown", { bubbles: true })); + expect(handler).toHaveBeenCalledTimes(1); // now outside newRef + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/utils/hooks/useOnClickOutside.test.tsx` around lines 188 - 216, Add a rerender case that swaps the refs array contents to a different element (same array length) to ensure the hook uses the updated ref values: create a second DOM element and a second ref (e.g., newRef/newElement), set newRef.current = newElement, call rerender with the updated props where the refs array now contains the newRef (still same length), dispatch a click outside and assert the handler is invoked for the new target (and that any previous ref handler is not invoked again); target the existing useOnClickOutside usage in the test (renderHook + rerender) so the regression for stale refs is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/utils/hooks/useWindowDimensions.test.tsx`:
- Around line 25-27: Remove the ineffective afterEach cleanup that calls
window.removeEventListener with an anonymous function; specifically delete the
line that calls window.removeEventListener("resize", () => {} ) in the
useWindowDimensions.test.tsx afterEach block because it cannot remove the real
handler and is a no-op—rely on `@testing-library/react-hooks`' auto-cleanup
instead.
In `@app/client/src/utils/URLUtils.test.ts`:
- Around line 135-139: The test "should handle array values by converting to
string" is passing a string instead of an array; update the test for
convertObjectToQueryParams to pass an actual array (e.g., { items: ['a','b','c']
}) and adjust the expected value to match the function's serialization (e.g.,
"?items=a%2Cb%2Cc"), so the test validates array coercion rather than accepting
a pre-joined string.
In `@CONTRIBUTING.md`:
- Around line 29-42: The fenced tree-view code block (the block showing the
appsmith/ directory structure) is missing a language hint; update the opening
fence from ``` to ```text so the block is tagged as text (use the same fenced
block that contains the tree view under the appsmith/ heading).
- Around line 136-148: Update the prerequisites table in CONTRIBUTING.md to
include the frontend tools that are referenced later: add a row for Yarn (e.g.,
"Yarn | Latest | Required for frontend commands") and a row for envsubst (e.g.,
"envsubst | Latest | Required by frontend setup scripts"), ensuring the table
rows match the existing columns and formatting so the `yarn` commands referenced
around lines 169-171 and the frontend setup documented in
contributions/ClientSetup.md (envsubst at line 26) are covered.
- Around line 188-196: Update the step that says "Edit .env to point to your
local MongoDB and Redis" to explicitly list the .env keys that must be
configured: APPSMITH_DB_URL, APPSMITH_REDIS_URL, and APPSMITH_GIT_ROOT; instruct
contributors to replace the example defaults with their local MongoDB and Redis
connection strings for APPSMITH_DB_URL and APPSMITH_REDIS_URL, and to ensure
APPSMITH_GIT_ROOT points to an actual directory on their machine (used for Git
operations).
---
Nitpick comments:
In `@app/client/src/utils/hooks/useOnClickOutside.test.tsx`:
- Around line 188-216: Add a rerender case that swaps the refs array contents to
a different element (same array length) to ensure the hook uses the updated ref
values: create a second DOM element and a second ref (e.g., newRef/newElement),
set newRef.current = newElement, call rerender with the updated props where the
refs array now contains the newRef (still same length), dispatch a click outside
and assert the handler is invoked for the new target (and that any previous ref
handler is not invoked again); target the existing useOnClickOutside usage in
the test (renderHook + rerender) so the regression for stale refs is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57e377c2-09e2-49c3-aeab-f0bbe5c71ba4
📒 Files selected for processing (5)
CONTRIBUTING.mdapp/client/src/utils/URLUtils.test.tsapp/client/src/utils/hooks/useOnClickOutside.test.tsxapp/client/src/utils/hooks/usePrevious.test.tsxapp/client/src/utils/hooks/useWindowDimensions.test.tsx
Comment on lines
+25
to
+27
| // Clean up event listeners | ||
| window.removeEventListener("resize", () => {}); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether auto-cleanup is enabled/disabled and confirm the no-op cleanup line.
# 1) Confirm the no-op listener removal in this test file.
rg -n 'removeEventListener\("resize", \(\) => \{\}\)' app/client/src/utils/hooks/useWindowDimensions.test.tsx
# 2) Check repo-level test setup for react-hooks auto cleanup toggles/usages.
rg -n 'RHTL_SKIP_AUTO_CLEANUP|dont-cleanup-after-each|@testing-library/react-hooks/(dont-cleanup-after-each|pure)|\bcleanup\s*\(' app/clientRepository: appsmithorg/appsmith
Length of output: 417
Remove ineffective afterEach cleanup.
Line 26 calls removeEventListener with a fresh function reference, so it does not detach the actual listener—a no-op. The line is unnecessary and can be removed since @testing-library/react-hooks handles auto-cleanup by default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/utils/hooks/useWindowDimensions.test.tsx` around lines 25 -
27, Remove the ineffective afterEach cleanup that calls
window.removeEventListener with an anonymous function; specifically delete the
line that calls window.removeEventListener("resize", () => {} ) in the
useWindowDimensions.test.tsx afterEach block because it cannot remove the real
handler and is a no-op—rely on `@testing-library/react-hooks`' auto-cleanup
instead.
Comment on lines
+135
to
+139
| it("should handle array values by converting to string", () => { | ||
| const result = convertObjectToQueryParams({ items: "a,b,c" }); | ||
|
|
||
| expect(result).toBe("?items=a%2Cb%2Cc"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix array test input to match the test intent.
The test name says “array values” but passes a string. This won’t catch regressions in array serialization/coercion.
✅ Minimal fix
it("should handle array values by converting to string", () => {
- const result = convertObjectToQueryParams({ items: "a,b,c" });
+ const result = convertObjectToQueryParams({ items: ["a", "b", "c"] });
expect(result).toBe("?items=a%2Cb%2Cc");
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/utils/URLUtils.test.ts` around lines 135 - 139, The test
"should handle array values by converting to string" is passing a string instead
of an array; update the test for convertObjectToQueryParams to pass an actual
array (e.g., { items: ['a','b','c'] }) and adjust the expected value to match
the function's serialization (e.g., "?items=a%2Cb%2Cc"), so the test validates
array coercion rather than accepting a pre-joined string.
Comment on lines
+29
to
+42
| ``` | ||
| appsmith/ | ||
| ├── app/ | ||
| │ ├── client/ # Frontend (React + TypeScript) | ||
| │ │ ├── src/ # React components, widgets, utilities | ||
| │ │ ├── cypress/ # Cypress integration tests | ||
| │ │ └── packages/ # Shared packages including RTS (Real-Time Server) | ||
| │ └── server/ # Backend (Java + Spring + WebFlux) | ||
| │ ├── appsmith-server/ # Main server application | ||
| │ └── appsmith-plugins/ # Database/API connectors | ||
| ├── deploy/ # Docker & Kubernetes deployment configs | ||
| ├── contributions/ # Contribution guides and documentation | ||
| └── static/ # Static assets for documentation | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language hint to the fenced code block
Line 29 uses a fenced block without a language tag (MD040). Use text for the tree view block.
Suggested diff
-```
+```text
appsmith/
├── app/
│ ├── client/ # Frontend (React + TypeScript)
│ │ ├── src/ # React components, widgets, utilities
│ │ ├── cypress/ # Cypress integration tests
│ │ └── packages/ # Shared packages including RTS (Real-Time Server)
│ └── server/ # Backend (Java + Spring + WebFlux)
│ ├── appsmith-server/ # Main server application
│ └── appsmith-plugins/ # Database/API connectors
├── deploy/ # Docker & Kubernetes deployment configs
├── contributions/ # Contribution guides and documentation
└── static/ # Static assets for documentation</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @CONTRIBUTING.md around lines 29 - 42, The fenced tree-view code block (the
block showing the appsmith/ directory structure) is missing a language hint;
update the opening fence from totext so the block is tagged as text (use
the same fenced block that contains the tree view under the appsmith/ heading).
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+136
to
+148
| ### Prerequisites | ||
|
|
||
| Before setting up the development environment, ensure you have: | ||
|
|
||
| | Tool | Version | Notes | | ||
| |------|---------|-------| | ||
| | Docker | Latest | Required for containerized services | | ||
| | Node.js | 20.11.1 | Use nvm or fnm for version management | | ||
| | Java | OpenJDK 25 | Eclipse Temurin recommended | | ||
| | Maven | 3.9+ | Preferably 3.9.12 | | ||
| | Git | Latest | For version control | | ||
| | mkcert | Latest | For local HTTPS certificates | | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine CONTRIBUTING.md around line 168
wc -l CONTRIBUTING.mdRepository: appsmithorg/appsmith
Length of output: 83
🏁 Script executed:
# Read the CONTRIBUTING.md file to see the actual content, especially around line 136-172
head -n 175 CONTRIBUTING.md | tail -n 45Repository: appsmithorg/appsmith
Length of output: 1143
🏁 Script executed:
# Check if contributions/ClientSetup.md exists and search for envsubst
fd ClientSetup.mdRepository: appsmithorg/appsmith
Length of output: 92
🏁 Script executed:
# Search for yarn usage in documentation
rg "yarn" --type md -nRepository: appsmithorg/appsmith
Length of output: 1484
🏁 Script executed:
# Search for envsubst in contributions/ClientSetup.md
rg "envsubst" contributions/ClientSetup.md -nRepository: appsmithorg/appsmith
Length of output: 181
🏁 Script executed:
# Also search for any mention of envsubst in the entire codebase
rg "envsubst" --type md -nRepository: appsmithorg/appsmith
Length of output: 210
Add missing frontend prerequisites to the prerequisites table
Line 169-171 uses yarn commands but yarn is not listed in the prerequisites. Additionally, envsubst is required by the frontend setup (documented in contributions/ClientSetup.md line 26) and is missing. Both tools must be added to prevent setup failures.
Suggested diff
| Tool | Version | Notes |
|------|---------|-------|
| Docker | Latest | Required for containerized services |
| Node.js | 20.11.1 | Use nvm or fnm for version management |
+| Yarn | Latest | Required for frontend dependency management and scripts |
| Java | OpenJDK 25 | Eclipse Temurin recommended |
| Maven | 3.9+ | Preferably 3.9.12 |
| Git | Latest | For version control |
| mkcert | Latest | For local HTTPS certificates |
+| envsubst | Latest | Required by frontend setup scripts/templates |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Prerequisites | |
| Before setting up the development environment, ensure you have: | |
| | Tool | Version | Notes | | |
| |------|---------|-------| | |
| | Docker | Latest | Required for containerized services | | |
| | Node.js | 20.11.1 | Use nvm or fnm for version management | | |
| | Java | OpenJDK 25 | Eclipse Temurin recommended | | |
| | Maven | 3.9+ | Preferably 3.9.12 | | |
| | Git | Latest | For version control | | |
| | mkcert | Latest | For local HTTPS certificates | | |
| ### Prerequisites | |
| Before setting up the development environment, ensure you have: | |
| | Tool | Version | Notes | | |
| |------|---------|-------| | |
| | Docker | Latest | Required for containerized services | | |
| | Node.js | 20.11.1 | Use nvm or fnm for version management | | |
| | Yarn | Latest | Required for frontend dependency management and scripts | | |
| | Java | OpenJDK 25 | Eclipse Temurin recommended | | |
| | Maven | 3.9+ | Preferably 3.9.12 | | |
| | Git | Latest | For version control | | |
| | mkcert | Latest | For local HTTPS certificates | | |
| | envsubst | Latest | Required by frontend setup scripts/templates | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 136 - 148, Update the prerequisites table in
CONTRIBUTING.md to include the frontend tools that are referenced later: add a
row for Yarn (e.g., "Yarn | Latest | Required for frontend commands") and a row
for envsubst (e.g., "envsubst | Latest | Required by frontend setup scripts"),
ensuring the table rows match the existing columns and formatting so the `yarn`
commands referenced around lines 169-171 and the frontend setup documented in
contributions/ClientSetup.md (envsubst at line 26) are covered.
Comment on lines
+188
to
+196
| 2. **Build and run the server**: | ||
| ```bash | ||
| cd app/server | ||
| mvn clean compile | ||
| cp envs/dev.env.example .env | ||
| # Edit .env to point to your local MongoDB and Redis | ||
| ./build.sh -Dmaven.test.skip | ||
| ./scripts/start-dev-server.sh | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the canonical env variable names from repo sources
set -euo pipefail
echo "== Server env template keys =="
fd 'dev.env.example$' -t f | xargs -I{} rg -n 'APPSMITH_(DB_URL|REDIS_(URL|URI)|GIT_ROOT)' {}
echo
echo "== Server setup guide references =="
fd 'ServerSetup.md$' -t f | xargs -I{} rg -n 'APPSMITH_(DB_URL|REDIS_(URL|URI)|GIT_ROOT)' {}
echo
echo "== CONTRIBUTING references =="
rg -n 'APPSMITH_(DB_URL|REDIS_(URL|URI)|GIT_ROOT)|\.env' CONTRIBUTING.mdRepository: appsmithorg/appsmith
Length of output: 1729
🏁 Script executed:
#!/bin/bash
# Find the actual dev.env.example file(s)
echo "== Finding dev.env.example files =="
find . -name "*dev.env.example*" -o -name "dev.env*" 2>/dev/null | head -20
echo
echo "== Checking app/server directory structure =="
ls -la app/server/envs/ 2>/dev/null || echo "envs/ directory not found"
echo
echo "== Server code: Redis env variable usage =="
rg 'APPSMITH_REDIS_(URL|URI)' app/server --type java -B2 -A2 | head -40Repository: appsmithorg/appsmith
Length of output: 5269
🏁 Script executed:
#!/bin/bash
# Verify if these env keys are referenced anywhere in server/pom.xml or application configs
echo "== Checking pom.xml or properties files =="
fd 'pom.xml|application.*properties|application.*yml' app/server -t f | xargs grep -l 'APPSMITH_' 2>/dev/null | head -5
echo
echo "== Looking for environment variable documentation in app/server =="
fd 'README|SETUP|ENV' app/server -t f 2>/dev/null | head -10
echo
echo "== Checking if GIT_ROOT is truly required =="
rg 'APPSMITH_GIT_ROOT' app/server --type java -cRepository: appsmithorg/appsmith
Length of output: 447
🏁 Script executed:
#!/bin/bash
# Read the dev.env.example file to see what's documented
echo "== Content of dev.env.example =="
cat -n app/server/envs/dev.env.example
echo
echo "== Content of app/server/README.md (first 50 lines) =="
head -50 app/server/README.md
echo
echo "== CONTRIBUTING.md context around the issue (lines 185-200) =="
sed -n '185,200p' CONTRIBUTING.mdRepository: appsmithorg/appsmith
Length of output: 1891
Explicitly document which .env keys require configuration
The instruction at line 193 to "Edit .env to point to your local MongoDB and Redis" is incomplete. After copying envs/dev.env.example .env, contributors should be told which keys require custom values: APPSMITH_DB_URL, APPSMITH_REDIS_URL, and APPSMITH_GIT_ROOT. The example template provides defaults, but contributors may miss that APPSMITH_GIT_ROOT must point to an actual directory on their machine for Git operations to function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 188 - 196, Update the step that says "Edit .env
to point to your local MongoDB and Redis" to explicitly list the .env keys that
must be configured: APPSMITH_DB_URL, APPSMITH_REDIS_URL, and APPSMITH_GIT_ROOT;
instruct contributors to replace the example defaults with their local MongoDB
and Redis connection strings for APPSMITH_DB_URL and APPSMITH_REDIS_URL, and to
ensure APPSMITH_GIT_ROOT points to an actual directory on their machine (used
for Git operations).
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.
Summary
This PR adds comprehensive unit tests for utility hooks and URL utility functions.
Changes
New Test Files
Test Results
All 75 tests pass successfully.
Testing Approach
Summary by CodeRabbit
Documentation
Tests