fix(filters): ensure saved filter views never have position=0 (#1996)

Fixes #724 - Tasks in saved filter views get `position: 0` when they first appear in the filter, causing drag-and-drop sorting to not persist correctly.

**Changes:**
- Remove harmful `Position: 0` inserts from cron job and
`SavedFilter.Update` - `RecalculateTaskPositions` already creates
positions with proper values, so the intermediate inserts created a race
window
- Add on-demand position creation when fetching tasks for saved filter
views - safety net for newly matching tasks before the cron runs
- Add 5 new tests covering the fix and regression scenarios

🐰 Positions once zero, now bloom with care,
Sorted with grace, no more despair,
When filters call and tasks appear,
Numbers spring up, crystal clear!
Issue 724 hops away—
Sorting's fixed to stay, hooray! 🎉

pkg/models/saved_filter_positions_test.go

@@ -91,3 +91,216 @@ func TestCronInsertsNonZeroPosition(t *testing.T) {
 	require.True(t, exists)
 	assert.NotZero(t, tp.Position)
 }
+
+func TestCronCreatesNonZeroPositions(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	// Create a saved filter
+	sf := &SavedFilter{
+		Title:   "cron-test-filter",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	u := &user.User{ID: 1}
+	err := sf.Create(s, u)
+	require.NoError(t, err)
+
+	// Get the kanban view for this filter
+	view := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Simulate what the cron does: call RecalculateTaskPositions
+	err = RecalculateTaskPositions(s, view, u)
+	require.NoError(t, err)
+
+	// Verify no positions are 0
+	zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.Zero(t, zeroCount, "No positions should be 0")
+
+	// Verify all tasks have positions
+	positionCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.NotZero(t, positionCount, "Should have positions")
+}
+
+func TestFilterUpdateCreatesNonZeroPositions(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	// Create a saved filter
+	sf := &SavedFilter{
+		Title:   "update-test-filter",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	u := &user.User{ID: 1}
+	err := sf.Create(s, u)
+	require.NoError(t, err)
+
+	// Update the filter (this triggers position creation)
+	err = sf.Update(s, u)
+	require.NoError(t, err)
+
+	// Get the kanban view
+	view := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Verify no positions are 0
+	zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.Zero(t, zeroCount, "No positions should be 0 after filter update")
+}
+
+func TestMultipleNewTasksGetDistinctPositions(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	// Create a saved filter that matches multiple tasks
+	sf := &SavedFilter{
+		Title:   "multi-task-filter",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	u := &user.User{ID: 1}
+	err := sf.Create(s, u)
+	require.NoError(t, err)
+
+	err = sf.Update(s, u)
+	require.NoError(t, err)
+
+	// Get the kanban view
+	view := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindKanban).Get(view)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Get all positions
+	var positions []*TaskPosition
+	err = s.Where("project_view_id = ?", view.ID).Find(&positions)
+	require.NoError(t, err)
+
+	// Verify all positions are unique
+	seen := make(map[float64]int64)
+	for _, p := range positions {
+		if existingTaskID, exists := seen[p.Position]; exists {
+			t.Errorf("Position %f is duplicated between tasks %d and %d",
+				p.Position, existingTaskID, p.TaskID)
+		}
+		seen[p.Position] = p.TaskID
+	}
+}
+
+func TestTaskFetchCreatesPositionsOnDemand(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	u := &user.User{ID: 1}
+
+	// Create a saved filter
+	sf := &SavedFilter{
+		Title:   "on-demand-position-filter",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	err := sf.Create(s, u)
+	require.NoError(t, err)
+
+	// Get the list view for this filter
+	view := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindList).Get(view)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Delete any existing positions to simulate a fresh state (before cron runs)
+	_, err = s.Where("project_view_id = ?", view.ID).Delete(&TaskPosition{})
+	require.NoError(t, err)
+
+	// Verify NO positions exist now
+	existingCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.Zero(t, existingCount, "No positions should exist after deletion")
+
+	// Fetch tasks for the view - this should trigger on-demand position creation
+	tc := &TaskCollection{
+		ProjectID:     view.ProjectID,
+		ProjectViewID: view.ID,
+	}
+	result, _, _, err := tc.ReadAll(s, u, "", 1, 50)
+	require.NoError(t, err)
+
+	tasks := result.([]*Task)
+	require.NotEmpty(t, tasks, "Should have tasks matching the filter")
+
+	// Verify all returned tasks have non-zero positions
+	for _, task := range tasks {
+		assert.NotZero(t, task.Position,
+			"Task %d (%s) should have non-zero position", task.ID, task.Title)
+	}
+
+	// Verify positions were created in database
+	createdCount, err := s.Where("project_view_id = ?", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.NotZero(t, createdCount, "Positions should have been created")
+
+	// Verify no zero positions
+	zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.Zero(t, zeroCount, "No positions should be zero")
+}
+
+func TestIssue724_SortingOnFilteredViews(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	u := &user.User{ID: 1}
+
+	// Create a saved filter
+	sf := &SavedFilter{
+		Title:   "issue-724-filter",
+		Filters: &TaskCollection{Filter: "done = false"},
+	}
+	err := sf.Create(s, u)
+	require.NoError(t, err)
+
+	// Get the list view for this filter
+	view := &ProjectView{}
+	exists, err := s.Where("project_id = ? AND view_kind = ?",
+		getProjectIDFromSavedFilterID(sf.ID), ProjectViewKindList).Get(view)
+	require.NoError(t, err)
+	require.True(t, exists)
+
+	// Fetch tasks for the view (simulating what the API does)
+	tc := &TaskCollection{
+		ProjectID:     view.ProjectID,
+		ProjectViewID: view.ID,
+	}
+
+	// This should trigger position creation
+	result, _, _, err := tc.ReadAll(s, u, "", 1, 50)
+	require.NoError(t, err)
+
+	tasks := result.([]*Task)
+
+	// Verify all returned tasks have non-zero positions
+	for _, task := range tasks {
+		assert.NotZero(t, task.Position,
+			"Task %d (%s) should have non-zero position", task.ID, task.Title)
+	}
+
+	// Verify positions in database are all non-zero
+	zeroCount, err := s.Where("project_view_id = ? AND position = 0", view.ID).Count(&TaskPosition{})
+	require.NoError(t, err)
+	assert.Zero(t, zeroCount,
+		"No position=0 records should exist in database for view %d", view.ID)
+}

pkg/models/saved_filters.go

@@ -263,19 +263,12 @@ func (sf *SavedFilter) Update(s *xorm.Session, _ web.Auth) error {
 		}
 
 		taskBuckets := make([]*TaskBucket, 0, len(tasksToAdd))
-		taskPositions := make([]*TaskPosition, 0, len(tasksToAdd))
 		for _, task := range tasksToAdd {
 			taskBuckets = append(taskBuckets, &TaskBucket{
 				TaskID:        task.ID,
 				BucketID:      bucketID,
 				ProjectViewID: view.ID,
 			})
-
-			taskPositions = append(taskPositions, &TaskPosition{
-				TaskID:        task.ID,
-				ProjectViewID: view.ID,
-				Position:      0,
-			})
 		}
 
 		if len(taskBuckets) > 0 {
@@ -284,11 +277,9 @@ func (sf *SavedFilter) Update(s *xorm.Session, _ web.Auth) error {
 			}
 		}
 
-		if len(taskPositions) > 0 {
-			if _, err = s.Insert(taskPositions); err != nil {
-				return err
-			}
-
+		// Recalculate positions for all tasks - this will create positions for
+		// new tasks that don't have them yet
+		if len(tasksToAdd) > 0 {
 			if err = RecalculateTaskPositions(s, view, &user.User{ID: sf.OwnerID}); err != nil {
 				return err
 			}
@@ -504,13 +495,8 @@ func RegisterAddTaskToFilterViewCron() {
 					newTaskBuckets = append(newTaskBuckets, tb)
 				}
 				if _, exists := savedTaskPositionMap[task.ID]; !exists {
-					tp := &TaskPosition{
-						TaskID:        task.ID,
-						ProjectViewID: view.ID,
-						Position:      0,
-					}
-					newTaskPositions = append(newTaskPositions, tp)
-
+					// Mark view for recalculation - RecalculateTaskPositions will create
+					// positions for all tasks including new ones
 					if _, ok := viewsToRecalc[view.ID]; !ok {
 						viewsToRecalc[view.ID] = struct {
 							view    *ProjectView

pkg/models/task_position.go

@@ -272,3 +272,43 @@ func DeleteOrphanedTaskPositions(s *xorm.Session) (count int64, err error) {
 		Where("task_id not in (select id from tasks) OR project_view_id not in (select id from project_views)").
 		Delete(&TaskPosition{})
 }
+
+// createPositionsForTasksInView creates position records for tasks that don't have them.
+// Used as a safety net during task fetching for saved filter views.
+func createPositionsForTasksInView(s *xorm.Session, tasks []*Task, view *ProjectView, a web.Auth) error {
+	if len(tasks) == 0 {
+		return nil
+	}
+
+	// Get the current lowest position to place new tasks at the top
+	lowestPosition := &TaskPosition{}
+	has, err := s.
+		Where("project_view_id = ?", view.ID).
+		OrderBy("position asc").
+		Get(lowestPosition)
+	if err != nil {
+		return err
+	}
+
+	var basePosition float64
+	if !has || lowestPosition.Position == 0 {
+		// No existing positions or all are zero - trigger full recalculation
+		return RecalculateTaskPositions(s, view, a)
+	}
+
+	// Place new tasks before the lowest position, evenly spaced
+	basePosition = lowestPosition.Position
+	spacing := basePosition / float64(len(tasks)+1)
+
+	newPositions := make([]*TaskPosition, 0, len(tasks))
+	for i, task := range tasks {
+		newPositions = append(newPositions, &TaskPosition{
+			TaskID:        task.ID,
+			ProjectViewID: view.ID,
+			Position:      spacing * float64(i+1),
+		})
+	}
+
+	_, err = s.Insert(&newPositions)
+	return err
+}

pkg/models/tasks.go

@@ -681,6 +681,35 @@ func addMoreInfoToTasks(s *xorm.Session, taskMap map[int64]*Task, a web.Auth, vi
 		for _, position := range positions {
 			positionsMap[position.TaskID] = position
 		}
+
+		// For saved filter views, ensure all tasks have positions
+		// This is a safety net - the cron job handles bulk position creation,
+		// but we need immediate positions for newly matching tasks
+		if GetSavedFilterIDFromProjectID(view.ProjectID) > 0 {
+			tasksNeedingPositions := make([]*Task, 0)
+			for _, task := range taskMap {
+				if _, hasPosition := positionsMap[task.ID]; !hasPosition {
+					tasksNeedingPositions = append(tasksNeedingPositions, task)
+				}
+			}
+
+			if len(tasksNeedingPositions) > 0 {
+				// Create positions for tasks that don't have them
+				if err = createPositionsForTasksInView(s, tasksNeedingPositions, view, a); err != nil {
+					return err
+				}
+
+				// Reload positions after creation
+				positions, err = getPositionsForView(s, view)
+				if err != nil {
+					return err
+				}
+				positionsMap = make(map[int64]*TaskPosition, len(positions))
+				for _, p := range positions {
+					positionsMap[p.TaskID] = p
+				}
+			}
+		}
 	}
 
 	var reactions map[int64]ReactionMap