fix: picking settingsConfig from ApiEditorContext instead of props #37758
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (2)
Line range hint
113-113: Consider adding proper TypeScript types for settingsConfigThe
anytype reduces type safety. Consider defining an interface for the settings configuration object.- settingsConfig: any; + interface SettingsConfig { + // Define the expected shape of your settings + // Example: + // showPagination?: boolean; + // enableCaching?: boolean; + } + settingsConfig: SettingsConfig;
Line range hint
365-365: Consider using consistent prop namingThe prop name changes from
settingsConfigtoactionSettingsConfigwhen passed to RequestTabs. Consider using consistent naming to improve maintainability.- actionSettingsConfig={settingsConfig} + settingsConfig={settingsConfig}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx(1 hunks)
🔇 Additional comments (2)
app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (2)
184-184: LGTM: Correctly moved settingsConfig from context to props
The change aligns with the PR objective of picking settingsConfig from props instead of context.
Line range hint 113-113: Verify all CommonEditorForm usages are updated
This change requires all components using CommonEditorForm to pass settingsConfig as a prop instead of relying on context.
Also applies to: 184-184
AmanAgarwal041
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12045074502. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Editor/ActionSettings.tsx (2)
44-51: Consider structuring the Cypress log for better debuggingThe logging implementation is correct but could be more structured for debugging purposes.
/* @ts-expect-error: Types are not available */ if (typeof window.Cypress?.log === "function") { /* @ts-expect-error: Types are not available */ window.Cypress.log( - "props.actionSettingsConfig", - props.actionSettingsConfig, + { + name: 'ActionSettings', + message: 'Rendering with config', + consoleProps: () => ({ + actionSettingsConfig: props.actionSettingsConfig + }) + } ); }
Line range hint
9-14: Technical Debt: Consider addressing the TODO comments and any typesThere are multiple TODOs and
anytype usages that should be properly typed to improve type safety.Consider creating proper interfaces for:
actionSettingsConfigstructure- The section and control configuration objects
This will improve type safety and reduce the need for eslint-disable directives.
Also applies to: 65-66
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/limited-tests.txt(1 hunks)app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx(2 hunks)app/client/src/pages/Editor/ActionSettings.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (2)
app/client/cypress/limited-tests.txt (1)
2-3: LGTM! Verifying test file paths.
The test specifications follow proper naming conventions and are correctly organized under their respective test categories.
Let's verify the existence of these test files:
✅ Verification successful
Test files exist at the specified paths
Both test files are present in their respective directories:
app/client/cypress/e2e/Regression/ClientSide/BugTests/AbortAction_Spec.tsapp/client/cypress/e2e/Regression/ServerSide/ApiTests/API_MultiPart_Spec.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the specified test files exist in the correct locations
# Check if the test files exist
fd "AbortAction_Spec.ts|API_MultiPart_Spec.ts" --type f
Length of output: 207
app/client/src/pages/Editor/ActionSettings.tsx (1)
Line range hint 9-14: Verify implementation against PR objectives
The PR description mentions moving settingsConfig from props to context, but this file still receives configuration via props. Please clarify if this is intentional or if additional changes are needed.
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12046542345. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12046542345. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12047228995. |
|
/ci-test-limit-count run_count=5 runId=12046542345 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12047325773. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12047228995. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12047325773. |
|
/ci-test-limit-count run_count=5 runId=12047325773 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12048237282. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/APIEditor/ApiEditorContext.tsx (3)
Line range hint
7-12: Address the TODO comment in the interface.The TODO comment indicates that the
anytype needs to be fixed. Consider defining a proper type forsettingsConfigto improve type safety.Would you like me to help define a proper type for the
settingsConfigproperty?
62-68: Consider improving the TypeScript integration with Cypress.Instead of suppressing TypeScript errors, consider:
- Installing proper Cypress types (
@types/cypress)- Using proper type guards for the Cypress check
Here's a more type-safe approach:
- /* @ts-expect-error: Types are not available */ - if (typeof window.Cypress?.log === "function") { - /* @ts-expect-error: Types are not available */ - window.Cypress.log({ + if (window.Cypress && typeof window.Cypress.log === 'function') { + window.Cypress.log({Also, consider extracting the debug message to a constant for better readability:
const debugMessage = `ApiEditorContextProvider, ${JSON.stringify(settingsConfig || {})} ${JSON.stringify(value.settingsConfig || {})}`; window.Cypress.log({ message: debugMessage });
66-66: Improve the debug message format.The current debug message format makes it difficult to distinguish between the two config objects. Consider using a more structured format.
- message: `ApiEditorContextProvider, ${JSON.stringify(settingsConfig || {})} ${JSON.stringify(value.settingsConfig || {})}`, + message: `ApiEditorContextProvider | Props config: ${JSON.stringify(settingsConfig || {})} | Context config: ${JSON.stringify(value.settingsConfig || {})}`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/pages/Editor/APIEditor/ApiEditorContext.tsx(1 hunks)app/client/src/pages/Editor/APIEditor/index.tsx(1 hunks)app/client/src/sagas/PluginSagas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/APIEditor/index.tsx
🔇 Additional comments (1)
app/client/src/sagas/PluginSagas.ts (1)
186-196: Verify consistent logging across related components.
Since this PR involves moving settingsConfig from props to ApiEditorContext, ensure logging is consistently implemented across all affected components.
✅ Verification successful
Cypress logging is consistently implemented across API editor components
The logging implementation for settings is already consistent across the related components:
PluginSagas.ts: Logs plugin form settingsActionSettings.tsx: Logs action settings configAPIEditor/index.tsx: Logs settings config with plugin IDAPIEditor/CommonEditorForm.tsx: Logs both context and props settingsAPIEditor/ApiEditorContext.tsx: Logs provider settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Cypress logging implementations in related files
rg -l "window\.Cypress.*log.*setting" --type ts
Length of output: 49
Script:
#!/bin/bash
# Let's broaden the search to find any Cypress logging in the codebase
rg "window\.Cypress.*log" --type ts -A 2
# Also search for any settings-related logging patterns
rg "log.*setting" --type ts -A 2
Length of output: 7013
| if ( | ||
| /* @ts-expect-error: Types are not available */ | ||
| typeof window.Cypress?.log === "function" && | ||
| plugin?.type === PluginType.API | ||
| ) { | ||
| /* @ts-expect-error: Types are not available */ | ||
| window.Cypress.log({ | ||
| message: `fetchPluginFormConfigsSaga, ${JSON.stringify(pluginFormData[index].setting || {})} ${JSON.stringify(defaultActionSettings[plugin.type] || {})}`, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and logging implementation.
The current implementation has several areas for improvement:
- Using
@ts-expect-erroris not ideal for handling Cypress types - The logging implementation could be more structured
Consider these improvements:
+ // Add to a types file
+ interface CypressWindow extends Window {
+ Cypress?: {
+ log: (args: { message: string }) => void;
+ };
+ }
- /* @ts-expect-error: Types are not available */
- typeof window.Cypress?.log === "function"
+ typeof (window as CypressWindow).Cypress?.log === "function"
- /* @ts-expect-error: Types are not available */
- window.Cypress.log({
+ (window as CypressWindow).Cypress?.log({
- message: `fetchPluginFormConfigsSaga, ${JSON.stringify(pluginFormData[index].setting || {})} ${JSON.stringify(defaultActionSettings[plugin.type] || {})}`,
+ message: `Plugin Settings:
+ Current: ${JSON.stringify(pluginFormData[index].setting || {})}
+ Default: ${JSON.stringify(defaultActionSettings[plugin.type] || {})}`
});Committable suggestion skipped: line range outside the PR's diff.
|
/ci-test-limit-count run_count=5 runId=12046542345 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12048371654. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12048371654. |
|
/ci-test-limit-count run_count=5 runId=12046542345 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12049158548. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12049158548. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12050373379. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12050373379. |
|
/ci-test-limit-count run_count=10 runId=12050373379 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12051047387. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12051047387. |
|
/ci-test-limit-count run_count=10 runId=12050373379 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12056743023. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12056743023. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Warning
Tests have not run on the HEAD 0b396fc yet
Wed, 27 Nov 2024 19:04:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
CommonEditorFormcomponent to acceptsettingsConfigdirectly from props for improved data management.CommonEditorForm,ActionSettings,ApiEditorContextProvider, andApiEditorWrappercomponents to aid debugging during tests.fetchPluginFormConfigsSagafor better visibility during Cypress testing.Refactor
settingsConfigis accessed, removing it from context in favor of prop-based sourcing.