421 collaborator management component refresh required

[cubap]

This pull request improves the handling of collaborator roles and user feedback in the project management UI. The main changes ensure that collaborator role updates are immediately reflected in the frontend state and UI, and that user actions trigger toast notifications for clearer feedback. Error handling has also been improved for better reliability.

Collaborator Role State Synchronization:

• The Project class (api/Project.js) now updates its local collaborators state when members are removed, promoted, demoted, or have their roles changed, ensuring the frontend always reflects the most current roles. [1] [2] [3] [4] [5]

UI Updates and Feedback:

• Added a refreshCollaborators method to the project-collaborators component, and integrated it into the roles-handler component so that collaborator list UI updates automatically after role changes, without needing a full page refresh. [1] [2]
• Replaced alert dialogs with toast notifications for all collaborator role actions (add/remove/promote/demote/set to viewer), and improved error handling to display error toasts when actions fail. [1] [2]

[github-actions]

🚀 Deployed on https://69951b2c3e014c8dd94eede6--tpen-interfaces-preview.netlify.app

[cubap]

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.

So, maybe we try this again? I can remove the last "leader" legally and get back a 200.

I think this would work if we actually get an error back. I can confirm that the leader role pops back in when I refresh.

[thehabes]

Static Review Comments

Branch: 421-collaborator-management-component-refresh-required
PR: #469
Review Date: 2026-02-17
Files Changed: 5

Summary

This PR overhauls the collaborator role management UI, replacing inline button panels with a unified <dialog>-based modal, replacing alert() / confirm() feedback with toast notifications, and synchronizing local collaborator state in Project.js after role mutations. The UX improvements are substantial. The modal approach is cleaner, the toggle-based role selection is more intuitive, and the local state sync avoids stale UI.

However, there are several issues: a formatting defect in Project.js, an error-handling architecture mismatch where Project class methods swallow errors that the UI components expect to catch, a logic inconsistency in unsaved-changes detection, and a leftover alert() call that contradicts the PR's stated goal.

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

---

Critical Issues 🔴

Issue 1: Error-handling architecture mismatch — API errors are silently swallowed

Files: api/Project.js (multiple methods) + components/roles-handler/index.js:630-644, 691-703
Category: Unhandled Errors / Logic Error

Problem:
The Project class methods (removeMember, makeLeader, demoteLeader, setToViewer, cherryPickRoles) all internally catch errors and call userMessage(), then return undefined without re-throwing. The new roles-handler UI wraps calls to these methods in try/catch blocks expecting errors to propagate — but they never will.

This means:

• If cherryPickRoles() fails on the server, saveRoleChanges() receives undefined, the if (response) block is skipped, no success toast fires — but the catch block's error toast also never fires. The user sees a cryptic userMessage from the API layer instead of the intended toast.
• Same issue for handleRemoveMember → removeMember and handleTransferOwnership → transferOwnership (the latter uses alert() internally — see Major Issue 1).

Current Code (example — saveRoleChanges in roles-handler/index.js:630):

try {
    const response = await TPEN.activeProject.cherryPickRoles(memberID, selectedRoles)
    if (response) {
        // This never runs on API error because cherryPickRoles returns undefined
        TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Roles updated successfully.', status: 'success' })
    }
    // If response is undefined due to swallowed error, nothing happens here
} catch (error) {
    // This catch NEVER fires because cherryPickRoles swallows its own errors
    TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Error updating roles', status: 'error' })
}

Current Code (example — cherryPickRoles in Project.js:219):

async cherryPickRoles(userId, roles) {
    try {
        // ...
    } catch (error) {
        userMessage(error.message) // Error is swallowed here
        // No re-throw — returns undefined
    }
}

Suggested Fix — choose ONE approach:

Option A (recommended — re-throw from Project methods):

async cherryPickRoles(userId, roles) {
    try {
        // ...
    } catch (error) {
        userMessage(error.message)
        throw error  // Let caller handle it too
    }
}

Option B (check for falsy return in the UI):

try {
    const response = await TPEN.activeProject.cherryPickRoles(memberID, selectedRoles)
    if (response) {
        TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Roles updated successfully.', status: 'success' })
        this.closeRoleModal()
        this.refreshCollaborators()
    } else {
        // Handle the "swallowed error" case
        TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'Error updating roles', status: 'error' })
    }
} catch (error) {
    // ...
}

How to Verify:

1. Open the collaborators management page
2. Open browser DevTools Network tab
3. Simulate a network failure (go offline or block the API request)
4. Try saving role changes — verify the user gets a clear toast notification, not silence or a different error mechanism

---

Major Issues 🟠

Issue 1: Leftover alert() contradicts the PR's toast migration goal

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

Problem:
The PR description states it "Replaced alert dialogs with toast notifications for all collaborator role actions." However, the rolesHandler catch block still uses alert():

} catch (error) {
    console.error("Error handling button action:", error)
    alert("An error occurred. Please try again.")
}

Suggested Fix:

} catch (error) {
    console.error("Error handling button action:", error)
    TPEN.eventDispatcher.dispatch('tpen-toast', { message: 'An error occurred. Please try again.', status: 'error' })
}

How to Verify: Search the file for remaining alert( calls.

---

Issue 2: Formatting defect — missing newline in setToViewer

File: api/Project.js:213
Category: Syntax / Formatting Error

Problem:
The closing brace and return are on the same line with excess whitespace between them:

            if (this.collaborators[userId]) {
                this.collaborators[userId].roles = ["VIEWER"]
            }            return response

Suggested Fix:

            if (this.collaborators[userId]) {
                this.collaborators[userId].roles = ["VIEWER"]
            }
            return response

How to Verify: Run a linter or visually inspect the file at line 213.

---

Issue 3: Unsaved-changes detection logic is inconsistent with save logic

File: components/roles-handler/index.js:647-672
Category: Logic Error

Problem:
handleModalClose and saveRoleChanges use different logic for determining when to add VIEWER:

In saveRoleChanges (line 622):

if (!selectedRoles.includes("LEADER") && !selectedRoles.includes("CONTRIBUTOR")) {
    selectedRoles.push("VIEWER")  // Always adds VIEWER when no Leader/Contributor
}

In handleModalClose (line 659):

if (!currentRoles.includes("LEADER") && !currentRoles.includes("CONTRIBUTOR") && customRoleToggles.length === 0) {
    currentRoles.push("VIEWER")  // Only adds VIEWER when ALSO no custom roles are active
}

This means a collaborator with roles ["VIEWER", "CUSTOM_ROLE"] will trigger a false "unsaved changes" prompt even when nothing was changed, because:

• originalRoles = ["CUSTOM_ROLE", "VIEWER"]
• currentRoles = ["CUSTOM_ROLE"] (VIEWER not pushed because customRoleToggles.length > 0)
• Mismatch → false positive

Suggested Fix — align the conditions:

if (!currentRoles.includes("LEADER") && !currentRoles.includes("CONTRIBUTOR")) {
    currentRoles.push("VIEWER")
}

How to Verify:

1. Add a custom role to a collaborator who has no Leader/Contributor roles
2. Open their manage modal
3. Close without changes — should NOT prompt for unsaved changes

---

Minor Issues 🟡

Issue 1: openManageModal is needlessly async

File: components/roles-handler/index.js:427
Category: Dead Code

Problem:
openManageModal is declared async but contains no await expressions.

async openManageModal(memberID) {

Suggested Fix:

openManageModal(memberID) {

---

Issue 2: Shadow DOM encapsulation violated — reaching into another component's shadow root

File: components/roles-handler/index.js:381, 392
Category: Code Hygiene / Architecture

Problem:
Multiple methods reach into project-collaborators' shadow DOM:

document.querySelector("project-collaborators")?.shadowRoot?.querySelector(".group-members")

This creates tight coupling between the two components. If project-collaborators changes its internal class names, roles-handler would silently break. This is a pre-existing pattern that wasn't introduced by this PR, but the new refreshCollaborators() method adds another coupling point.

Suggested Fix: Consider having project-collaborators expose a part attribute on .group-members or provide a public API method for injecting action elements, rather than reaching into its internals. (This may be out of scope for this PR but worth noting for future work.)

---

Issue 3: onclick handlers bypass CleanupRegistry

File: components/roles-handler/index.js:506, 546, 552, 559, 570, 580, 588, 593, 599
Category: Code Hygiene

Problem:
The openRoleModal method sets multiple .onclick = property assignments on elements within the shadow DOM (leader/contributor toggles, save/close buttons, transfer/remove buttons, custom role toggles). These bypass the CleanupRegistry pattern used everywhere else in the component.

Since these are set on elements within the component's own shadow DOM and are reassigned each time the modal opens (replacing previous handlers), this works correctly. However, it's inconsistent with the established pattern. The CleanupRegistry exists specifically to prevent this kind of ad-hoc handler management.

Suggested Fix: For consistency, consider using this.renderCleanup.onElement(...) for these handlers, though this is low priority since the property-assignment approach is functionally correct here.

---

Issue 4: Potential runtime error — this.errorHTML may not exist

File: components/roles-handler/index.js:388
Category: Runtime Error

Problem:

if (!TPEN.activeProject) {
    return this.errorHTML.innerHTML = "No project"
}

this.errorHTML is never defined in the RolesHandler class. If TPEN.activeProject is nullish, this will throw TypeError: Cannot set properties of undefined. This is pre-existing code, but it's in a method that was modified by the PR.

Suggested Fix:

if (!TPEN.activeProject) {
    console.error("No active project for roles-handler")
    return
}

---

Suggestions 🔵

Suggestion 1: Global style injection is fragile

File: components/roles-handler/index.js:30-63
Category: Architecture

The _injectManageButtonStyles method injects a <style> element into document.head to style project-collaborators::part(manage-button). While this works (it's necessary because ::part() selectors must be in the document scope), it means:

• Multiple instances of roles-handler share one style element (handled via ID check)
• The cleanup removes it when ANY instance disconnects, potentially affecting other instances

Consider documenting this cross-component styling dependency clearly, or adding the styles to the page's CSS file (css/collaborators/index.css) instead of injecting dynamically.

---

Suggestion 2: Consider a disabled state for the Save button when no changes exist

File: components/roles-handler/index.js:587-590
Category: UX Enhancement

The Save button is always enabled. It could be disabled when the current toggle state matches the original roles, providing a clearer signal to users about whether changes have been made.

---

Suggestion 3: transferOwnership in Project.js still uses alert() and console.error

File: api/Project.js:260-261
Category: Consistency

This pre-existing method was not changed in this PR but is called by the new handleTransferOwnership. It still uses:

console.error("Error updating roles:", error)
alert("Failed to update roles. Please try again.")

This means on transfer ownership failure, the user sees an alert() from Project.js rather than the toast the PR intends. This ties directly to Critical Issue 1 (the error is swallowed, so the caller's toast never fires).

---

Files Reviewed

• api/Project.js — Local state sync additions are sound; formatting defect on line 213
• components/manage-role/index.js — Local cache pattern improves reliability; proper error handling added
• components/project-collaborators/index.js — Clean refreshCollaborators() addition
• components/roles-handler/index.js — Major rewrite; dialog-based modal is a significant UX improvement; several issues noted above
• interfaces/manage-project/collaborators.html — Layout/style changes look correct

Checklist for Developer

• Address Critical Issue: Error-handling mismatch between Project API and UI
• Address Major Issues: leftover alert(), formatting defect, unsaved-changes logic
• Review and address Minor Issues as appropriate
• Consider Suggestions for future improvements
• Manually test: role save, role toggle close without changes, error scenarios (network failure), transfer ownership, remove member

[thehabes]

Issues discovered during testing

1. Tooltip is cut off in role picker

2. I have to refresh the page to see the invited user after a successful invitation. I think it is within this scope to make sure they paginate into the list without needing to refresh.
3. I no longer see error messages on the page when I perform illegal actions, such as here after trying to remove the owner and getting a 403.

Once I close the role picker, I see the message

[cubap]

There are a few things noted here that are out of scope but should be handled somewhere else.

1. UserMessage replaces error toasts and preempts some upstream error handling right now.
2. Services returns a 200 for some illegal actions that appear to work until a refresh in this interface, like removing the last Leader.
3. Old pattern has alert() and confirm() where a dialog or modal makes more sense.