[ENGG-4251] feat: index request body via template strings #32
Conversation
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
wrongsahil
changed the title
Member
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (2)
src/types/internal.ts (1)
1-1: Use a type-only import for har-format.Prevents an unnecessary runtime dependency from being bundled.
Apply this diff:
-import { Request } from "har-format"; +import type { Request } from "har-format";src/core/utils/templating/helpers/requestHelpers.ts (1)
27-40: Handle content-types, HAR params, and non-string keys in body() helper.Improves correctness for JSON and x-www-form-urlencoded bodies and avoids odd behavior when no key is passed.
Apply this diff:
- body: (key: string) => { // passes key - const rawData = params.data - const defaultResponse = '' - if(rawData && rawData.text) { - try { - // fix-me: handle url encoded params and other operations on body later - const parsedData = JSON.parse(rawData.text) - return parsedData[key] ?? defaultResponse - } catch (error) { - /* NOOP */ - } - } - return defaultResponse - } + body: (key: string) => { + const defaultResponse = ''; + if (typeof key !== 'string' || !key) return defaultResponse; + + const rawData = params.data; + const contentType = params.headers?.['content-type'] || ''; + + // Prefer structured HAR params if present + const fromParams = rawData?.params?.find?.((p: any) => p?.name === key)?.value; + if (fromParams != null) return String(fromParams); + + const text = rawData?.text || ''; + if (!text) return defaultResponse; + + if (contentType.includes('application/json')) { + try { + const parsed = JSON.parse(text); + return parsed?.[key] != null ? String(parsed[key]) : defaultResponse; + } catch { /* NOOP */ } + } + + if (contentType.includes('application/x-www-form-urlencoded')) { + try { + const qp = new URLSearchParams(text); + return qp.get(key) ?? defaultResponse; + } catch { /* NOOP */ } + } + + return defaultResponse; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/common/mockProcessor.ts(2 hunks)src/core/utils/templating/helpers/requestHelpers.ts(1 hunks)src/test/dummy/mock1.ts(1 hunks)src/types/internal.ts(1 hunks)
🔇 Additional comments (1)
src/core/common/mockProcessor.ts (1)
10-10: LGTM: bring post body into render context.Import aligns with the new template helper.
Comment on lines
+39
to
40
| data: getPostData(request) | ||
| }; |
There was a problem hiding this comment.
Redact or remove logging of contextParams; adding data now risks leaking PII/secrets.
With data included, the existing console.log (Line 42) will emit request bodies and sensitive headers.
Suggested mitigation (replace the current log):
// Redacted debug logging
const { data: _redacted, headers, ...rest } = contextParams;
const redactedHeaders = Object.fromEntries(
Object.entries(headers || {}).map(([k, v]) => [
k,
/authorization|cookie|token|key/i.test(k) ? '[REDACTED]' : v,
])
);
console.debug({ ...rest, headers: redactedHeaders });🤖 Prompt for AI Agents
In src/core/common/mockProcessor.ts around lines 39-40, the patch adds request
data into contextParams which causes the existing console.log to emit request
bodies and sensitive headers; remove or redact sensitive fields before logging:
do not log the full data field, strip or rename it (e.g. omit data or set to
"[REDACTED]"), sanitize headers by replacing values for keys matching
authorization|cookie|token|key (case-insensitive) with "[REDACTED]", and log
only the remaining non-sensitive properties; update the
console.debug/console.log call to use the sanitized object instead of the raw
contextParams.
| "content-type": "application/json", | ||
| }, | ||
| body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}" }}', | ||
| body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}", body: "{{body \'test\'}}" }', |
There was a problem hiding this comment.
🧹 Nitpick
Fix invalid JSON key: quote "body".
Unquoted property names make the response body invalid JSON.
Apply this diff:
- body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}", body: "{{body \'test\'}}" }',
+ body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}", "body": "{{body \'test\'}}" }',📝 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
| body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}", body: "{{body \'test\'}}" }', | |
| body: '{"Hello":"There","mockId":"1", "statusCode": {{ statusCode }}, "method": "{{ method }}", "urlParams": "{{ urlParam \'userId\' }}", "header": "{{ header \'userid\' \'test\' }}", "body": "{{body \'test\'}}" }', |
🤖 Prompt for AI Agents
In src/test/dummy/mock1.ts around line 21, the JSON string assigned to the body
contains an unquoted property name body which makes it invalid JSON; update the
string so the property name "body" is quoted (and ensure any inner quotes are
properly escaped or the surrounding string delimiter adjusted) so the entire
value is valid JSON.
Comment on lines
+1
to
10
| import { Request } from "har-format"; | ||
| import { RequestMethod } from "."; | ||
|
|
||
| export interface MockContextParams { | ||
| urlParams: Record<string, string>; | ||
| method: RequestMethod; | ||
| statusCode: number; | ||
| headers: Record<string, string>; | ||
| data: Request["postData"] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify har-format is declared in package.json (dev dep is fine).
Ensures the type import resolves in CI.
Run:
🏁 Script executed:
#!/bin/bash
fd -a package.json | xargs -I{} rg -nC2 -H '"har-format"' {}Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
found=false
while IFS= read -r -d '' file; do
found=true
echo "=== $file ==="
if command -v rg >/dev/null 2>&1; then
rg -nC2 -H '"har-format"' "$file" || echo "no match in $file"
else
grep -n '"har-format"' "$file" || echo "no match in $file"
fi
done < <(find . -type f -name package.json -print0)
if [ "$found" = false ]; then
echo "no package.json files found"
fiLength of output: 57
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# find package.json via git or find
files="$(git ls-files -- package.json 2>/dev/null || true)"
if [ -z "$files" ]; then
files="$(find . -type f -name 'package.json' -print 2>/dev/null || true)"
fi
if [ -z "$files" ]; then
echo "no package.json files found"
exit 0
fi
for f in $files; do
echo "=== $f ==="
if command -v jq >/dev/null 2>&1; then
jq -r '["dependencies","devDependencies","peerDependencies","optionalDependencies"] | .[] as $k | (.[$k] // {}) | keys[]' "$f" 2>/dev/null | sort -u | rg -n --hidden -F 'har-format' || echo "har-format not listed in deps of $f"
else
if grep -q '"har-format"' "$f"; then
echo "har-format entry:"
grep -n '"har-format"' "$f" || true
else
echo "har-format not found in $f"
fi
fi
doneLength of output: 66
Add har-format to package.json (devDependency OK)
Type import in src/types/internal.ts (import { Request } from "har-format") is not declared in package.json; add "har-format" as a dependency/devDependency so the type import resolves in CI.
🤖 Prompt for AI Agents
In src/types/internal.ts around lines 1 to 10, the code imports Request from
"har-format" but the package is not declared in package.json, causing CI type
resolution failures; add "har-format" to package.json (as a devDependency is
fine) and run your package manager install (e.g., npm install --save-dev
har-format or pnpm add -D har-format) so the type import resolves in CI; commit
the updated package.json and lockfile.
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit