Skip to content

Dont render credential modals inside td #3389

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 15, 2025

Conversation

midigofrank
Copy link
Collaborator

@midigofrank midigofrank commented Jul 11, 2025

Description

Problem: The credential modals were being rendered inside <td> elements, which violates HTML semantics and causes layout issues. Additionally, the credential tables and events were duplicated in different liveviews - the /credentials page and the project settings page

Solution Overview:
The solution involves moving all the credential listing logic, modal state management, and event handling into a centralized component that is used in /credentials and project settings.

Key Changes

1. Introduced CredentialIndexComponent (lib/lightning_web/live/credential_live/credential_index_component.ex)

  • This is the heart of the solution - a reusable LiveComponent that encapsulates everything that was previously duplicated across the /credentials page and project settings page
  • Handles all credential and OAuth client listing
  • Manages the active_modal state to control which modal is currently shown (new_credential, edit_credential, delete_credential, transfer_credential, new_oauth_client, edit_oauth_client, delete_oauth_client)

2. Standardized Modal Closing System

The main goal was that when you click Cancel or the X button on any credential modal, it should hide the modal and remove it from the DOM. This is achieved by having all modal close events target the CredentialIndexComponent.

To make it easy to target the CredentialIndexComponent, I added 3 helper components:

  • Components.Credentials.credentials_index_live_component/1: Ensures that the livecomponent always uses the same ID.
    • Components.Credentials.credential_modal/1: A <.modal /> wrapper. By default targets the CredentialIndexComponent by id for close events.
    • Components.Credentials.credential_modal_cancel_button: It automatically targets the CredentialIndexComponent by id when clicked.

Both thecredential_modal and credential_modal_cancel_button have a default on_modal_close assign that sends close_active_modal events to the CredentialIndexComponent, but this can be overridden for special cases such as in the Workflow edit page.

Ensure you test all the actions, create, edit, transfer, revoke transfer and delete

Closes #1588

Validation steps

This needs a lot of click testing on all the credential pages. Visit any of the /credentials, project settings or even the workflow edit page for adding a new credential.

Additional notes for the reviewer

  • I have renamed ModalPortal to LiveComponentPortal for better clarity about its purpose. This component isn't used in the current PR but provides a foundation for future modal rendering improvements. The rename makes it clear that this is for rendering LiveComponents outside their normal DOM location
  • Codecov has highlighted missing coverage for oauth client edit and deletion. Most of those tests were tagged with :skip. I have created a separate issue to track this: Fix OAuth Client tests #3391

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@midigofrank midigofrank self-assigned this Jul 11, 2025
@github-project-automation github-project-automation bot moved this to New Issues in v2 Jul 11, 2025
Copy link

codecov bot commented Jul 14, 2025

Codecov Report

Attention: Patch coverage is 83.60656% with 30 lines in your changes missing coverage. Please review.

Project coverage is 89.98%. Comparing base (df6def1) to head (3791c1e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...live/credential_live/credential_index_component.ex 73.49% 22 Missing ⚠️
lib/lightning_web/live/livecomponent_portal.ex 38.46% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3389      +/-   ##
==========================================
- Coverage   89.99%   89.98%   -0.01%     
==========================================
  Files         368      369       +1     
  Lines       14458    14414      -44     
==========================================
- Hits        13011    12971      -40     
+ Misses       1447     1443       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@midigofrank midigofrank marked this pull request as ready for review July 14, 2025 09:59
@midigofrank midigofrank requested review from elias-ba and stuartc July 14, 2025 09:59
@midigofrank
Copy link
Collaborator Author

@taylordowns2000 kindly take this for a spin when you find some time, just to double check that all actions are working as they were

Copy link
Member

@taylordowns2000 taylordowns2000 left a comment

Choose a reason for hiding this comment

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

  1. when attempting to transfer to a user and the transfer fails (if they're not a member of all projects shared with that credential, for example) no feedback is shown to the user - please fix this
  2. does this prevent us from actually (and secretly) rendering and firing off Oauth requests for all credential modals when visiting the credentials list page?

@github-project-automation github-project-automation bot moved this from New Issues to In review in v2 Jul 14, 2025
@midigofrank
Copy link
Collaborator Author

midigofrank commented Jul 14, 2025

  1. when attempting to transfer to a user and the transfer fails (if they're not a member of all projects shared with that credential, for example) no feedback is shown to the user - please fix this
    This is working perfectly on my end. @elias-ba When you find time, could you please try this action on your end?
Screenshot 2025-07-14 at 14 56 52
  1. does this prevent us from actually (and secretly) rendering and firing off Oauth requests for all credential modals when visiting the credentials list page?

Yes it does. Now it's only one oauth credential at a time and only when you click to edit. Wait, was it intended to fire for all at once?

@taylordowns2000
Copy link
Member

Yes it does. Now it's only one oauth credential at a time and only when you click to edit. Wait, was it intended to fire for all at once?

This is what we wanted. Great.

For the transfer bug, I've followed up on Slack

@midigofrank
Copy link
Collaborator Author

midigofrank commented Jul 14, 2025

@taylordowns2000 I've created a separated issue to track the error message not showing in credential transfer form. This bug can be reproduced in main as well: #3402

The 'use LightningWeb, :live_component' already aliases LightningWeb.Components
to Components, making the full module path redundant.
Copy link
Member

@stuartc stuartc left a comment

Choose a reason for hiding this comment

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

Great work @midigofrank!

@stuartc stuartc merged commit e0f3428 into main Jul 15, 2025
7 of 8 checks passed
@stuartc stuartc deleted the 1588-dont-render-credential-modals-inside-td branch July 15, 2025 06:59
@github-project-automation github-project-automation bot moved this from In review to Done in v2 Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Don't render credential modals instead of hiding them
3 participants