Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Credentials UI #1828

Merged
merged 1 commit into from
Feb 24, 2025
Merged

Credentials UI #1828

merged 1 commit into from
Feb 24, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 24, 2025

Important

Add credential handling to the workflow editor with new UI components and types for credential parameters.

  • Credential Handling:
    • Add CredentialApiResponse, PasswordCredentialApiResponse, and CreditCardCredentialApiResponse types in types.ts.
    • New CredentialSelector component in CredentialSelector.tsx for selecting credentials using a dropdown.
    • Update FlowRenderer.tsx and WorkflowEditor.tsx to handle CredentialParameterYAML and CredentialParameter.
  • UI Components:
    • Add DropdownWithOptions component in DropdownWithOptions.tsx for generic dropdown functionality.
    • Add KeyIcon component in KeyIcon.tsx for UI representation.
  • Workflow Editor:
    • Update WorkflowParameterAddPanel.tsx and WorkflowParameterEditPanel.tsx to support adding and editing credential parameters.
    • Modify WorkflowParametersPanel.tsx to include credential parameters in the dropdown menu.
    • Update workflowEditorUtils.ts to convert credential parameters to YAML format.
  • Context:
    • Introduce CloudContext in CloudContext.ts to manage cloud-specific features.

This description was created by Ellipsis for 1e9ae54. It will automatically update as commits are pushed.

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add a new Credentials UI feature with components, routes, and workflow integration for managing passwords and credit cards.
>
>   - **UI Components**:
>     - Add `CredentialsPage`, `CredentialsList`, `CredentialsModal`, `CredentialItem`, `DeleteCredentialButton`, `PasswordCredentialContent`, `CreditCardCredentialContent` for managing credentials.
>     - Add `DropdownWithOptions` and `KeyIcon` components.
>   - **Routing**:
>     - Add new route `/credentials` with `CredentialsPageLayout` and `CredentialsPage` in `router.tsx`.
>     - Update `SideNav.tsx` to include a link to the Credentials page.
>   - **Context and State**:
>     - Introduce `CloudContext` in `CloudContext.ts` to manage cloud-specific features.
>     - Implement `useCredentialModalState` for managing credential modal state.
>   - **API and Types**:
>     - Define types for credentials in `types.ts` and `api/types.ts`.
>     - Add functions `isPasswordCredential` and `isCreditCardCredential` in `api/types.ts`.
>   - **Workflow Integration**:
>     - Update `FlowRenderer.tsx`, `WorkflowEditor.tsx`, and related panels to support credential parameters.
>     - Add `CredentialSelector` component for selecting credentials in workflows.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 9276028e5855356f178604ec1ee1dde8ab59a1fd. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 1e9ae54 in 53 seconds

More details
  • Looked at 732 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern-frontend/src/store/CloudContext.ts:3
  • Draft comment:
    CloudContext is set up correctly with a default value of false. Consider adding a brief comment to explain its intended usage (e.g., to toggle cloud-specific features), especially if this flag may evolve in the future.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. skyvern-frontend/src/store/CloudContext.ts:1
  • Draft comment:
    Good use of createContext. For enhanced debugging, consider setting CloudContext.displayName (e.g., CloudContext.displayName = 'CloudContext';).
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:1860
  • Draft comment:
    There are a couple of instances in the getWorkflowErrors function (e.g., around line 1860 and 1886) where the error message says 'Error messages is not valid JSON.' Consider rephrasing it to be grammatically correct, for example: 'Error message is not valid JSON' or 'Error messages are not valid JSON.'
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_4YR2yzv7D6WscRiv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@msalihaltun msalihaltun merged commit c5a2438 into main Feb 24, 2025
7 checks passed
@msalihaltun msalihaltun deleted the salih/credentials-ui branch February 24, 2025 19:54
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 1e9ae54 in 3 minutes and 53 seconds

More details
  • Looked at 732 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. skyvern-frontend/src/store/CloudContext.ts:1
  • Draft comment:
    CloudContext is implemented succinctly and follows best practices. No issues noted.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the implementation without offering any constructive criticism or specific guidance.
2. skyvern-frontend/src/api/types.ts:300
  • Draft comment:
    Consider adding JSDoc comments to credential response types for better clarity and documentation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The types are quite self-explanatory. Field names like username, last_four, brand are clear. The pattern in the rest of the file is to not use JSDoc. Adding JSDoc here would be inconsistent. The comment doesn't point out any actual problems or confusion.
    JSDoc could potentially add value by explaining the expected format of fields or any special constraints. The types might be more complex than they appear at first glance.
    While JSDoc can be valuable, this seems like a purely stylistic suggestion that doesn't align with the codebase's conventions. The types are simple enough that the field names communicate their purpose effectively.
    Delete the comment. It's a minor stylistic suggestion that doesn't highlight any real issues and would make these types inconsistent with the rest of the file.
3. skyvern-frontend/src/components/DropdownWithOptions.tsx:31
  • Draft comment:
    The callback parameter in onValueChange shadows the outer 'value' variable; consider renaming it for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While technically correct about the shadowing, this is a very common pattern in React components. The shadowed variable is only used in a single-line arrow function where the scope is extremely clear. The code is still readable and maintainable. The suggestion to rename would likely make the code less idiomatic without providing meaningful benefits.
    The shadowing could potentially cause confusion for junior developers. There's a small chance it could lead to bugs if the function body becomes more complex in the future.
    The code follows common React patterns, the function is simple and contained, and the shadowing actually makes semantic sense since it's the new value replacing the old value.
    This comment should be deleted as it suggests a change that would make the code less idiomatic without providing meaningful benefits.
4. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParameterAddPanel.tsx:280
  • Draft comment:
    The onSave handler contains duplicated logic for different parameter types. Consider refactoring to reduce repetition.
  • Reason this comment was not posted:
    Comment was on unchanged code.
5. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParameterEditPanel.tsx:330
  • Draft comment:
    Similar to the add panel, the onSave handler here could benefit from refactoring to avoid duplicated parameter-saving logic.
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx:230
  • Draft comment:
    Inline style for positioning the operation panel is used; consider using Tailwind utilities or extracting into a constant for maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:152
  • Draft comment:
    Avoid magic numbers in layout functions (e.g., loop node width of 600px). Consider defining these as named constants for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:1253
  • Draft comment:
    Throwing an error in convertEchoParameters for unknown parameter types may be too strict. Consider handling unexpected cases more gracefully.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. skyvern-frontend/src/store/CloudContext.ts:1
  • Draft comment:
    CloudContext is simply created with a default false value. Ensure that its usage across the app correctly handles cloud vs on-prem scenarios.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the usage of CloudContext is correct across the app, which is a form of asking for confirmation or verification. This violates the rule against asking the author to ensure behavior is intended or tested.
10. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParameterEditPanel.tsx:216
  • Draft comment:
    There is a potential typographical inconsistency in the conditional that checks 'if (parameterType === "file_url" ...'. Based on the workflowParameterTypeOptions, the file type is represented as 'file' (mapped from FileURL), so this comparison may never be true. Consider verifying and updating this constant to match the intended value (e.g. 'file') for consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_BoO3uFpf1C4t1u7P


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

function CredentialSelector({ value, onChange }: Props) {
const credentialGetter = useCredentialGetter();

const { data: credentials, isFetching } = useQuery<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider handling error states or adding an error UI for the react-query query in case fetching credentials fails.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants