Apply better UX to collaboration management

[cubap]

This pull request makes a minor adjustment to the interfaces/manage-project/collaborators.html file. The change removes unnecessary CSS properties from the #projectManagementBtn style, which likely improves the button's visibility and layout.

• Removed position: absolute and display: none from the #projectManagementBtn CSS rules in collaborators.html.
• Applied TPEN Default styles where available.
• Shortened interaction chains to update user roles and avoid conflicts.
• Upgraded dialog to modern HTMLElements.

[thehabes]

Static Review Comments

Branch: 470-manage-roles-interactions-in-collaboration-management-is-icky
PR: #471 - Apply better UX to collaboration management
Review Date: 2026-02-13
Files Changed: 3

Summary

This PR is a significant UX improvement to the collaboration management interface. The core changes are:

1. roles-handler: Replaces the old inline button expansion with a native <dialog> modal using toggle-style role buttons (Leader/Contributor). Much cleaner UX.
2. manage-role: Adds a local group cache to avoid full re-fetches after CRUD operations on custom roles. Upgrades .then() chains to try/catch with proper error handling.
3. collaborators.html: Fixes layout/visibility bugs with the "Go to Project Management" button.

Overall the architecture is heading in the right direction. The dialog-based approach is a nice upgrade from the old hidden-div modal. The local cache pattern avoids unnecessary round-trips. There are a few issues to address before merging.

Category	Issues Found
🔴 Critical	1
🟠 Major	4
🟡 Minor	4
🔵 Suggestions	3

Critical Issues 🔴

Issue 1: Duplicate "Manage" buttons appended on every refresh

File: components/roles-handler/index.js:335-350
Category: Logic Error

Problem:
renderProjectCollaborators() always calls groupMembersActionsElement.appendChild(memberHTML) without clearing existing content first. When refreshCollaborators() is called after saving roles or removing a member, the flow is:

1. collaboratorsComponent.refreshCollaborators() rebuilds the collaborator DOM (clears and re-renders)
2. requestAnimationFrame calls this.renderProjectCollaborators() which appends new "Manage" buttons

This works once. But if authgate fires again (via onProjectReady, which can fire multiple times per the project conventions), render() is called which invokes both addEventListeners() and renderProjectCollaborators(). Since project-collaborators may have already been rendered with manage buttons from a prior call, duplicate buttons accumulate.

Current Code:

Array.from(groupMembersElement.children).forEach(child => {
    const groupMembersActionsElement = child.querySelector(".actions")
    for (const collaboratorId in collaborators) {
        if (groupMembersActionsElement?.getAttribute("data-member-id") == collaboratorId) {
            // ... always appends, never clears
            groupMembersActionsElement.appendChild(memberHTML)
        }
    }
})

Suggested Fix:
Clear existing manage buttons before appending new ones:

Array.from(groupMembersElement.children).forEach(child => {
    const groupMembersActionsElement = child.querySelector(".actions")
    for (const collaboratorId in collaborators) {
        if (groupMembersActionsElement?.getAttribute("data-member-id") == collaboratorId) {
            // Clear any previously injected manage buttons
            groupMembersActionsElement.querySelectorAll(".manage-button")
                .forEach(btn => btn.closest("div")?.remove())

            let memberHTML
            if (userHasEditAccess) {
                memberHTML = this.createMemberHTML(collaboratorId)
            } else {
                memberHTML = document.createElement("div")
                memberHTML.innerHTML = "You do not have permission to edit member roles"
            }
            groupMembersActionsElement.appendChild(memberHTML)
        }
    }
})

How to Verify:

1. Open the collaborators page for a project
2. Click "Manage" on a user, toggle a role, and save
3. Verify only one "Manage" button appears per collaborator (not two)
4. Navigate away and back, or trigger another tpen-project-loaded event
5. Verify buttons are still not duplicated

---

Major Issues 🟠

Issue 2: Injected <style> element cleanup is a no-op

File: components/roles-handler/index.js:59
Category: Dead Code / Misleading Pattern

Problem:
this.cleanup.onElement(styleEl, 'remove', () => {}) registers an event listener for a 'remove' event on a DOM element. There is no standard DOM event called 'remove'. This line does nothing - the style element is never removed from document.head when the component disconnects, and the empty callback () => {} wouldn't do anything even if the event fired.

The style element persists forever in document.head. For a singleton style injection this is arguably acceptable behavior, but the code is misleading because it looks like it handles cleanup.

Current Code:

document.head.appendChild(styleEl)
this.cleanup.onElement(styleEl, 'remove', () => {})

Suggested Fix:
Either remove the no-op line and add a comment explaining the style is intentionally persistent, or actually clean it up in disconnectedCallback:

document.head.appendChild(styleEl)
// Track for cleanup on disconnect
this.cleanup.add(() => {
    document.getElementById('roles-handler-manage-button-styles')?.remove()
})

---

Issue 3: updateRole() sends group parameter instead of this.group to API

File: components/manage-role/index.js:537 (line 102 in PR diff)
Category: Logic Error (latent)

Problem:
The method signature is async updateRole(group). The preceding loop correctly updates this.group[key], but the API call sends group (the parameter) rather than this.group. Currently these are the same object reference, so it works. But this is fragile and inconsistent with the explicit shift toward using this.group everywhere else in this PR.

Current Code:

Object.keys(this.group || {}).forEach(key => {
    if (key.toUpperCase() === role.value.toUpperCase()) {
        this.group[key] = [...this.permissions]  // updates this.group
    }
})

// ...
body: JSON.stringify({ roles: group })  // sends parameter, not this.group

Suggested Fix:

body: JSON.stringify({ roles: this.group })

---

Issue 4: alert() still used instead of tpen-toast in two places

File: components/roles-handler/index.js:396 and components/roles-handler/index.js:557
Category: UX Inconsistency

Problem:
The PR upgrades most user notifications to tpen-toast, but two alert() calls remain:

1. In rolesHandler() catch block: alert("An error occurred. Please try again.")
2. In handleTransferOwnership(): alert("Ownership transferred successfully.")

These are blocking modal dialogs that break the improved UX pattern established by this PR.

Current Code:

// rolesHandler catch:
alert("An error occurred. Please try again.")

// handleTransferOwnership:
alert("Ownership transferred successfully.")

Suggested Fix:

// rolesHandler catch:
TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'An error occurred. Please try again.', status: 'error' })

// handleTransferOwnership (before reload):
TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Ownership transferred successfully.', status: 'success' })

Note: The transfer ownership case also calls location.reload() immediately after, so the toast may not be visible. Consider if the reload is still desired or if a softer refresh would be better.

---

Issue 5: handleTransferOwnership has no try/catch

File: components/roles-handler/index.js:552-561
Category: Unhandled Error

Problem:
handleTransferOwnership calls await TPEN.activeProject.transferOwnership(memberID) without a try/catch. If the API call throws (network error, etc.), the error propagates unhandled. Every other async action handler in this PR (saveRoleChanges, handleRemoveMember) has proper try/catch with toast notifications.

Current Code:

async handleTransferOwnership(memberID, memberName) {
    const confirmMessage = `...`
    if (window.confirm(confirmMessage)) {
        const response = await TPEN.activeProject.transferOwnership(memberID)
        if (response) {
            alert("Ownership transferred successfully.")
            location.reload()
        }
    }
}

Suggested Fix:

async handleTransferOwnership(memberID, memberName) {
    const confirmMessage = `You are about to transfer ownership of this project to ${memberName}. This action is irreversible. Please confirm if you want to proceed.`
    if (!window.confirm(confirmMessage)) return
    try {
        const response = await TPEN.activeProject.transferOwnership(memberID)
        if (response) {
            TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Ownership transferred successfully.', status: 'success' })
            location.reload()
        }
    } catch (error) {
        console.error("Error transferring ownership:", error)
        TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Error transferring ownership', status: 'error', dismissible: true })
    }
}

---

Minor Issues 🟡

Issue 6: readonlyBtn does not update aria-pressed on toggles

File: components/roles-handler/index.js:464-468
Category: Accessibility

Problem:
When the "Set to Read-Only" button is clicked, it removes the active class from both toggles and calls updateToggleStates(). However, it does not update the aria-pressed attributes on the toggle buttons. Screen readers will still report the old state.

Current Code:

readonlyBtn.onclick = () => {
    leaderToggle.classList.remove("active")
    contributorToggle.classList.remove("active")
    updateToggleStates()
}

Suggested Fix:

readonlyBtn.onclick = () => {
    leaderToggle.classList.remove("active")
    contributorToggle.classList.remove("active")
    leaderToggle.setAttribute("aria-pressed", "false")
    contributorToggle.setAttribute("aria-pressed", "false")
    updateToggleStates()
}

---

Issue 7: Dialog lacks aria-labelledby for screen readers

File: components/roles-handler/index.js:279
Category: Accessibility

Problem:
The <dialog> element does not have an aria-labelledby attribute pointing to the #modalTitle heading. While screen readers can often infer the label from content, explicitly associating it is best practice for accessible dialogs.

Suggested Fix:

<dialog id="roleModal" aria-labelledby="modalTitle">

---

Issue 8: Hardcoded color values in _injectManageButtonStyles

File: components/roles-handler/index.js:36-56
Category: Maintainability

Problem:
The injected styles use hardcoded color values (#1976d2, #1565c0, rgba(25, 118, 210, ...)) instead of CSS custom properties like var(--primary-color). The rest of the shadow DOM styles in render() correctly use CSS custom properties with fallbacks. This creates an inconsistency where theme changes won't affect these injected button styles.

Current Code:

project-collaborators::part(manage-button) {
    background: linear-gradient(135deg, #1976d2 0%, #1565c0 100%);
    /* ... */
    box-shadow: 0 2px 6px rgba(25, 118, 210, 0.25);
}

Suggested Fix:

project-collaborators::part(manage-button) {
    background: linear-gradient(135deg, var(--primary-color, #1976d2) 0%, var(--primary-dark, #1565c0) 100%);
    /* ... */
    box-shadow: 0 2px 6px rgba(25, 118, 210, 0.25);
}

---

Issue 9: switch(true) with single case in rolesHandler

File: components/roles-handler/index.js:387-393
Category: Code Hygiene

Problem:
The switch(true) pattern now only has one case (manage-button) plus a no-op default. This was originally justified when there were 7+ button types, but now it's just unnecessary complexity.

Current Code:

switch (true) {
    case button.classList.contains("manage-button"):
        this.openManageModal(memberId)
        break
    default:
        break
}

Suggested Fix:

if (button.classList.contains("manage-button")) {
    this.openManageModal(memberId)
}

---

Suggestions 🔵

Suggestion 1: Add loading state to Save button during API call

The "Save Changes" button remains clickable during the cherryPickRoles API call. Users could double-click or get confused if the API is slow. Consider disabling the button and showing a loading state:

async saveRoleChanges(memberID, leaderToggle, contributorToggle) {
    const saveButton = this.shadowRoot.querySelector("#modalSaveButton")
    saveButton.disabled = true
    saveButton.textContent = "Saving..."
    try {
        // ... existing code
    } catch (error) {
        // ... existing code
    } finally {
        saveButton.disabled = false
        saveButton.textContent = "Save Changes"
    }
}

---

Suggestion 2: Consider focus management after modal close

When the dialog closes, the browser's default behavior for <dialog> is to return focus to the element that opened it. This should work automatically, but verify that after closeRoleModal(), focus returns to the "Manage" button that triggered the modal. If not, you may need to store and restore the trigger element.

---

Suggestion 3: Custom roles not reflected in the toggle UI

The modal currently only shows Leader and Contributor toggles. If a user has custom roles (from the manage-role component), those roles are silently preserved or removed depending on how cherryPickRoles works on the backend. Consider either:

• Documenting this behavior (that custom roles are preserved by the backend)
• Adding a note in the modal: "Custom roles are not affected"
• Or showing custom roles as read-only pills in the dialog

---

Positive Observations

• Excellent upgrade from the old hidden-div modal to native <dialog> - proper stacking context, backdrop, and escape key handling for free
• The toggle button UX with gradient animation is a nice touch
• Proper aria-pressed on toggle buttons (except the read-only case noted above)
• Good unsaved-changes detection on modal close
• Local cache updates in manage-role avoid unnecessary network round-trips
• Consistent use of tpen-toast for user feedback (with the two exceptions noted)

---

Files Reviewed

• components/manage-role/index.js - Local cache pattern added, error handling improved
• components/roles-handler/index.js - Major refactor to dialog-based modal with toggle UX
• interfaces/manage-project/collaborators.html - Layout fixes for management button

Checklist for Developer

• Address Critical issue (duplicate manage buttons on re-render)
• Address Major issues (style cleanup no-op, group vs this.group, remaining alert() calls, missing try/catch)
• Review Minor issues (aria-pressed on read-only, aria-labelledby, hardcoded colors, switch simplification)
• Consider Suggestions for future improvements
• Manual test the full flow: open modal, toggle roles, save, verify UI updates correctly
• Test with keyboard-only navigation (Tab through modal, Escape to close)

---

🤖 Generated with Claude Code

[thehabes]

Some issues found during manual testing.

• I still have to refresh the component to see a user in the group after a successful invitation.

• The page automatically refreshes after ownership is transferred, can we stop that one too?

• When I do something wrong (such as try to remove the only OWNER or remove myself) I do not see the error message because the role picker covers the message. This is a regression from main.

main

470-manage-roles-interactions-in-collaboration-management-is-icky

• I do not see custom roles in the roles picker when I am managing roles for a user. This is a regression from main where I do see those roles in the role picker.

main

470-manage-roles-interactions-in-collaboration-management-is-icky

• The UI allows me demote myself from being the LEADER, even when I am the last leader. The changes paginate. However, that is an illegal role change that is actually denied. I see my leader role still after I refresh the page. main behaves the same way.