Cherry-picked axios changes from #3302 #3329
Conversation
WalkthroughA new environment-specific configuration file was added for the backend, while the ReactJS application context and hooks were refactored to support a dynamic function for building HTTP request headers. Several interfaces were updated to include this function, and related code was reformatted for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant SheshaProvider
participant SheshaApplicationInstance
Component->>SheshaProvider: Provide buildHttpRequestHeaders (optional)
SheshaProvider->>SheshaApplicationInstance: Initialize with buildHttpRequestHeaders
Component->>SheshaApplicationInstance: Request Axios config
SheshaApplicationInstance->>SheshaApplicationInstance: Call buildHttpRequestHeaders (if exists)
SheshaApplicationInstance-->>Component: Return config with merged headers
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shesha-core/src/Shesha.Web.Host/appsettings.Kudzai.json (1)
17-17: Fix typo in log folder pathThere's a typo in the default log folder path - "jobss" should likely be "jobs".
- "DefaultLogFolder": "~/App_Data/Logs/jobss" + "DefaultLogFolder": "~/App_Data/Logs/jobs"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shesha-core/src/Shesha.Web.Host/appsettings.Kudzai.json(1 hunks)shesha-reactjs/src/hooks/useAxiosRequestConfig.ts(1 hunks)shesha-reactjs/src/providers/sheshaApplication/application.ts(1 hunks)shesha-reactjs/src/providers/sheshaApplication/index.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-attempt
🔇 Additional comments (7)
shesha-reactjs/src/hooks/useAxiosRequestConfig.ts (1)
11-21: Well-implemented dynamic header integrationThe implementation correctly integrates the
buildHttpRequestHeadersfunction with proper:
- Optional chaining to handle when the function is undefined
- Memoization with appropriate dependencies
- Header precedence where dynamic headers can override static ones
The logic is sound and follows React hooks best practices.
shesha-reactjs/src/providers/sheshaApplication/index.tsx (2)
68-68: Good interface extension for dynamic headersThe addition of the optional
buildHttpRequestHeadersproperty to the interface is well-designed and maintains backward compatibility.
25-25: Improved code organization and readabilityThe formatting changes enhance code readability:
- Consolidated imports for better organization
- Multi-line JSX blocks for clarity
- Restructured export statements
These changes improve maintainability without affecting functionality.
Also applies to: 38-42, 72-72, 80-82, 134-153, 196-202
shesha-reactjs/src/providers/sheshaApplication/application.ts (4)
164-171: Well-designed header merging strategyThe header merging logic in
setRequestHeadersfollows a good precedence order:
- Existing headers
- Dynamic headers from
buildHttpRequestHeaders- New headers parameter
- Front-end app header (always preserved)
This ensures proper header precedence while maintaining the required app identifier.
142-162: Robust initialization implementationThe async initialization logic is well-implemented with:
- Proper state management and UI updates via
#rerender()- Comprehensive error handling and logging
- Sequential execution of initialization actions
181-186: Safe permission checking implementationThe permission checking logic correctly handles edge cases:
- Empty permissions array returns
true(good default)- Safe access to authorizer with optional chaining
- Proper null/undefined checks
188-197:Details
❓ Verification inconclusive
Verify form designer component aggregation logic
The component registration and aggregation logic looks correct, but please verify that the
hasOwnPropertycheck is necessary given the object spreading approach used elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Check if hasOwnProperty is consistently used in similar object iteration patterns # Expected: Find similar patterns to verify consistency # Search for similar object iteration patterns in the codebase rg -A 3 -B 1 "for.*in.*{" --type ts --type tsxLength of output: 132
We’ll re-run with more precise patterns to find all use of
hasOwnPropertyand anyfor…inloops:
🏁 Script executed:
#!/bin/bash # 1. List all occurrences of hasOwnProperty in TS/TSX rg "hasOwnProperty" -n --type ts --type tsx # 2. Find every for/in loop via AST to see how often this pattern is used ast-grep --pattern $'for ( $_ in $_ ) { $$$ }'Length of output: 119
Let’s retry with explicit glob patterns instead of
--type:
🏁 Script executed:
#!/bin/bash # 1. Find all hasOwnProperty uses in TS/TSX files rg -n "hasOwnProperty" -g "*.ts" -g "*.tsx" # 2. Locate all for…in loops in TS/TSX files rg -n "for .* in .*{" -g "*.ts" -g "*.tsx"Length of output: 327
Verify necessity of
hasOwnPropertyguard inregisterFormDesignerComponentsThe aggregation logic looks correct, but given that
registrationsis built via object spread from known keys, thehasOwnPropertycheck may be redundant. Please confirm whether any inherited properties could leak intoregistrations; if not, consider removing the check for clarity.• File: shesha-reactjs/src/providers/sheshaApplication/application.ts
Lines 188–197
| @@ -0,0 +1,18 @@ | |||
| { | |||
| "ConnectionStrings": { | |||
| "Default": "Data Source=localhost\\MSSQLSERVER01; Initial Catalog=MembershipApp;MultipleActiveResultSets=True;Trusted_Connection=True;TrustServerCertificate=true;" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security Concern: Hardcoded database connection details
The connection string contains hardcoded server and database names. Consider using environment-specific variables or secure configuration providers for production deployments.
- "Default": "Data Source=localhost\\MSSQLSERVER01; Initial Catalog=MembershipApp;MultipleActiveResultSets=True;Trusted_Connection=True;TrustServerCertificate=true;"
+ "Default": "${CONNECTION_STRING}"📝 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.
| "Default": "Data Source=localhost\\MSSQLSERVER01; Initial Catalog=MembershipApp;MultipleActiveResultSets=True;Trusted_Connection=True;TrustServerCertificate=true;" | |
| "Default": "${CONNECTION_STRING}" |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Web.Host/appsettings.Kudzai.json at line 3, the
database connection string is hardcoded with server and database names, which
poses a security risk. Replace these hardcoded values with environment variables
or use a secure configuration provider to inject these details at runtime,
ensuring sensitive information is not stored directly in the configuration file.
| "Authentication": { | ||
| "JwtBearer": { | ||
| "IsEnabled": "true", | ||
| "SecurityKey": "SUUDE4LZ77PT729NV5BEPXDMAFRHFX7F", |
There was a problem hiding this comment.
Critical Security Issue: Hardcoded JWT security key
The JWT security key is hardcoded in the configuration file, which poses a significant security risk. This key should be stored in a secure location such as environment variables, Azure Key Vault, or other secure configuration providers.
- "SecurityKey": "SUUDE4LZ77PT729NV5BEPXDMAFRHFX7F",
+ "SecurityKey": "${JWT_SECURITY_KEY}",Consider using environment variables or secure configuration providers to manage sensitive keys.
📝 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.
| "SecurityKey": "SUUDE4LZ77PT729NV5BEPXDMAFRHFX7F", | |
| "SecurityKey": "${JWT_SECURITY_KEY}", |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Web.Host/appsettings.Kudzai.json at line 12, the JWT
security key is hardcoded, which is a critical security risk. Remove the
hardcoded key from this file and instead configure the application to read the
JWT security key from a secure source such as environment variables or a secure
configuration provider like Azure Key Vault. Update the application
configuration code to retrieve the key securely at runtime.
2 participants
Summary by CodeRabbit
New Features
Refactor