Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive Kanban view feature for workflow topic lists in Discourse. The implementation adds a toggleable Kanban board interface that allows users to visualize and manipulate workflow topics through drag-and-drop interactions, while maintaining full compatibility with existing workflow behavior and permissions.
Changes:
- Added Kanban view toggle and board rendering in the workflow discovery interface with drag-and-drop topic transitions
- Extended backend models and serializers to support Kanban compatibility checks and transition metadata
- Added comprehensive test coverage including system specs for drag-and-drop interactions and request specs for API responses
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/system/workflow_quick_filters_spec.rb | Added system tests for Kanban toggle, view switching, and drag-drop transitions with legal/illegal column highlighting |
| spec/system/page_objects/pages/workflow_discovery.rb | Added page object methods for testing Kanban UI interactions including drag-drop simulation |
| spec/requests/workflow_list_filters_spec.rb | Added request specs for Kanban metadata serialization and multi-workflow compatibility checks |
| spec/requests/admin/workflows_controller_spec.rb | Added admin controller specs for Kanban compatibility flag serialization |
| spec/lib/workflow_spec.rb | Added model specs for kanban_compatible? method including cycle and disconnected graph scenarios |
| spec/lib/workflow_serializer_spec.rb | Added serializer specs for kanban_compatible attribute |
| plugin.rb | Extended TopicList with Kanban workflow detection, step/transition metadata, and workflow_can_act permission checks |
| config/locales/client.en.yml | Added localization strings for Kanban UI labels and messages |
| assets/stylesheets/common/workflow_common.scss | Added comprehensive Kanban board styling with card states, drag indicators, and responsive layout |
| assets/javascripts/discourse/initializers/init-workflow.gjs | Registered workflow_view query parameter for SPA navigation without page refresh |
| assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs | Implemented Kanban board component with drag-drop handlers, column rendering, and AJAX-based transitions |
| assets/javascripts/discourse/admin/components/workflow-editor.gjs | Added Kanban compatibility indicator to workflow editor admin UI |
| app/serializers/discourse_workflow/workflow_serializer.rb | Added kanban_compatible attribute to workflow serialization |
| app/models/discourse_workflow/workflow.rb | Implemented kanban_compatible? method using DFS to verify workflow graph structure |
Comments suppressed due to low confidence (7)
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs:41
- The clearDragState method uses a setTimeout with a hardcoded 150ms delay to clear recentlyDraggedTopicId. This timing-based approach could be problematic if the user performs rapid interactions. Consider using a more deterministic approach, such as clearing the state after the click event handler has completed or using a proper event sequence tracker.
clearDragState(topicId = null) {
const normalizedTopicId = Number(topicId || this.draggedTopicId);
if (normalizedTopicId) {
this.recentlyDraggedTopicId = normalizedTopicId;
setTimeout(() => {
if (Number(this.recentlyDraggedTopicId) === normalizedTopicId) {
this.recentlyDraggedTopicId = null;
}
}, 150);
}
this.draggedTopicId = null;
this.draggedFromPosition = null;
}
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs:600
- When an AJAX error occurs during a drag-and-drop transition (lines 595-596), popupAjaxError is called but the topic remains in its original position in the UI because updateTopicTransitionState is only called after a successful transition (line 594). This is correct behavior, but there's no visual feedback to the user that the transition failed beyond the error popup. Consider adding a temporary visual indicator (like a shake animation or brief error state) on the card to make it clearer which card failed to transition.
try {
await ajax(`/discourse-workflow/act/${topicId}`, {
type: "POST",
data: { option: optionSlug },
});
this.updateTopicTransitionState(topicId, toPosition);
} catch (error) {
popupAjaxError(error);
} finally {
this.transitionInFlightTopicId = null;
this.clearDragState(topicId);
}
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs:559
- The updateTopicTransitionState method searches through multiple possible topic collection locations (lines 536-541) and updates all occurrences of the topic. While thorough, this approach could lead to unexpected behavior if the same topic appears in multiple collections but should have different states in each. Consider whether this is intentional, or if updating should be limited to a single canonical source of truth for topic state.
updateTopicTransitionState(topicId, targetPosition) {
const topicCollections = [
this.discovery.currentTopicList?.topics,
this.topicList?.topics,
this.topicListMetadata?.topics,
this.topicListMetadata?.topic_list?.topics,
];
topicCollections.forEach((topicCollection) => {
if (!Array.isArray(topicCollection)) {
return;
}
const topic = topicCollection.find(
(candidate) => Number(candidate.id) === topicId
);
if (!topic) {
return;
}
set(topic, "workflow_step_position", targetPosition);
set(topic, "workflow_step_name", this.kanbanStepNames[targetPosition]);
set(topic, "workflow_overdue", false);
});
}
assets/stylesheets/common/workflow_common.scss:218
- The CSS uses color-mix() function with 'in srgb' color space (lines 207, 217, 218, 261, 268, 271, 280). While this is a modern CSS feature, it may not be supported in older browsers. Consider verifying that the minimum supported browser versions for Discourse support the color-mix() function, or provide fallback colors for browsers that don't support it.
background: color-mix(in srgb, var(--success) 14%, transparent);
}
}
.workflow-kanban__column--illegal {
.workflow-kanban__cards {
background-image: repeating-linear-gradient(
135deg,
transparent,
transparent 8px,
color-mix(in srgb, var(--danger) 14%, transparent) 8px,
color-mix(in srgb, var(--danger) 14%, transparent) 16px
app/models/discourse_workflow/workflow.rb:31
- The kanban_compatible? method doesn't check for duplicate positions within the same workflow (line 28 checks uniqueness, which is good), but it doesn't validate that positions are sequential or start from 1. A workflow with positions [1, 3, 5] would pass the uniqueness check. While this may be intentional to allow gaps in position numbering, consider documenting this behavior or adding validation if sequential positions are required for proper kanban display.
def kanban_compatible?
steps = workflow_steps.includes(:workflow_step_options).to_a
return false if steps.blank?
positions = steps.map { |step| step.position.to_i }
return false if positions.uniq.size != positions.size
start_steps = steps.select { |step| step.position.to_i == 1 }
return false unless start_steps.one?
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs:621
- The querySelector call on line 619 could potentially return null if the DOM structure changes, but the optional chaining (?.) safely handles this. However, consider whether this direct DOM manipulation is the best approach for a Glimmer component. A more idiomatic approach might be to use a component class or data-attribute that can be styled with CSS, rather than directly querying and modifying unrelated DOM elements outside the component's scope.
document
.querySelector("#list-area .contents")
?.classList.toggle("workflow-kanban-hide-topics", this.isKanbanView);
}
app/models/discourse_workflow/workflow.rb:57
- The DFS implementation in kanban_compatible? uses a simple visited hash to handle cycles (line 53: next if visited[current_step_id]), which is correct. However, this means that cycles are silently allowed and the method will return true for workflows with backward transitions or loops, as long as all steps are reachable from the start. This behavior should be clearly documented, as it may not be obvious to users that cyclic workflows are considered "kanban compatible".
visited = {}
stack = [start_step_id]
until stack.empty?
current_step_id = stack.pop
next if visited[current_step_id]
visited[current_step_id] = true
edges[current_step_id].each { |target_step_id| stack << target_step_id }
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs
Outdated
Show resolved
Hide resolved
assets/javascripts/discourse/connectors/discovery-list-container-top/workflow-quick-filters.gjs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ction step to step
Summary
This PR adds a Kanban view to /workflow and extends workflow discovery with actionable board interactions, compatibility checks, admin visibility, stale-state recovery, and expanded test coverage.
Compared to main, this is primarily a discovery/list UX expansion plus transition/runtime hardening, while keeping permissioning aligned with core Discourse category behavior.
New Features
1. List/Kanban toggle in workflow discovery
2. Kanban board rendering and actions
3. Kanban compatibility rules and admin visibility
4. Workflow-level Kanban tag control
5. Stale-state transition handling
6. Overdue refinements in this PR
7. Visual refinements
Technical Changes
Tests
Notes