From 2c6029eac40ebc0e50b6ecfe4a20a16b7bc97481 Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 15 Apr 2026 11:57:03 +0200 Subject: [PATCH] refactor(frontend): replace for...in usages and forbid via lint rule Replaces 33 for...in loops across 18 files with for...of + Object.keys/entries or indexed for loops. for...in iterates enumerable string keys including inherited ones, which is especially risky on reactive arrays (tasks, labels, assignees, etc.) where polyfilled properties may appear. Loops that mutate via splice during iteration now iterate backwards to avoid index-shift bugs. Adds a no-restricted-syntax ESLint rule forbidding ForInStatement to prevent regressions. Closes #513 --- frontend/eslint.config.js | 5 +++++ frontend/src/components/input/Multiselect.vue | 2 +- .../src/components/project/views/ProjectList.vue | 2 +- frontend/src/components/sharing/UserTeam.vue | 10 +++++----- .../src/components/tasks/partials/Comments.vue | 2 +- .../components/tasks/partials/EditAssignees.vue | 2 +- .../src/components/tasks/partials/EditLabels.vue | 4 ++-- frontend/src/helpers/case.ts | 4 ++-- frontend/src/helpers/filters.test.ts | 8 ++++---- frontend/src/helpers/saveCollapsedBucketState.ts | 2 +- .../src/helpers/time/calculateDayInterval.test.ts | 14 +++++++------- .../modules/quickAddMagic/quickAddMagic.test.ts | 14 +++++++------- frontend/src/services/abstractService.ts | 11 ++++++----- frontend/src/services/task.ts | 2 +- frontend/src/stores/kanban.ts | 8 ++++---- frontend/src/views/tasks/ShowTasks.vue | 2 +- frontend/src/views/tasks/TaskDetailView.vue | 9 ++++++--- frontend/src/views/teams/EditTeam.vue | 6 +++--- 18 files changed, 58 insertions(+), 49 deletions(-) diff --git a/frontend/eslint.config.js b/frontend/eslint.config.js index 906b32d1e..bc7fb5e89 100644 --- a/frontend/eslint.config.js +++ b/frontend/eslint.config.js @@ -112,6 +112,11 @@ export default [ 'depend/ban-dependencies': 'warn', + 'no-restricted-syntax': ['error', { + selector: 'ForInStatement', + message: 'Use for...of with Object.keys/entries, or .forEach, instead of for...in. See https://github.com/go-vikunja/vikunja/issues/513', + }], + '@typescript-eslint/no-unused-vars': [ 'error', { diff --git a/frontend/src/components/input/Multiselect.vue b/frontend/src/components/input/Multiselect.vue index d1a7961b4..0ba654bcb 100644 --- a/frontend/src/components/input/Multiselect.vue +++ b/frontend/src/components/input/Multiselect.vue @@ -448,7 +448,7 @@ function createOrSelectOnEnter() { } function remove(item: T) { - for (const ind in internalValue.value) { + for (let ind = 0; ind < internalValue.value.length; ind++) { if (internalValue.value[ind] === item) { internalValue.value.splice(ind, 1) break diff --git a/frontend/src/components/project/views/ProjectList.vue b/frontend/src/components/project/views/ProjectList.vue index 9d014f8fe..ae8dd56ed 100644 --- a/frontend/src/components/project/views/ProjectList.vue +++ b/frontend/src/components/project/views/ProjectList.vue @@ -235,7 +235,7 @@ function updateTasks(updatedTask: ITask) { return } - for (const t in tasks.value) { + for (let t = 0; t < tasks.value.length; t++) { if (tasks.value[t].id === updatedTask.id) { tasks.value[t] = updatedTask break diff --git a/frontend/src/components/sharing/UserTeam.vue b/frontend/src/components/sharing/UserTeam.vue index ce5eb634b..b8727a711 100644 --- a/frontend/src/components/sharing/UserTeam.vue +++ b/frontend/src/components/sharing/UserTeam.vue @@ -291,7 +291,7 @@ async function deleteSharable() { await stuffService.delete(stuffModel) showDeleteModal.value = false - for (const i in sharables.value) { + for (let i = sharables.value.length - 1; i >= 0; i--) { if ( (sharables.value[i].username === stuffModel.username && props.shareType === 'user') || (sharables.value[i].id === stuffModel.teamId && props.shareType === 'team') @@ -344,15 +344,15 @@ async function toggleType(sharable) { } const r = await stuffService.update(stuffModel) - for (const i in sharables.value) { + for (const sharableEntry of sharables.value) { if ( - (sharables.value[i].username === + (sharableEntry.username === stuffModel.username && props.shareType === 'user') || - (sharables.value[i].id === stuffModel.teamId && + (sharableEntry.id === stuffModel.teamId && props.shareType === 'team') ) { - sharables.value[i].permission = r.permission + sharableEntry.permission = r.permission } } success({message: t('project.share.userTeam.updatedSuccess', {type: shareTypeName.value})}) diff --git a/frontend/src/components/tasks/partials/Comments.vue b/frontend/src/components/tasks/partials/Comments.vue index 4d6aebcfb..3c0477ee7 100644 --- a/frontend/src/components/tasks/partials/Comments.vue +++ b/frontend/src/components/tasks/partials/Comments.vue @@ -478,7 +478,7 @@ async function editComment() { commentEdit.taskId = props.taskId try { const comment = await taskCommentService.update(commentEdit) - for (const c in comments.value) { + for (let c = 0; c < comments.value.length; c++) { if (comments.value[c].id === commentEdit.id) { comments.value[c] = comment } diff --git a/frontend/src/components/tasks/partials/EditAssignees.vue b/frontend/src/components/tasks/partials/EditAssignees.vue index 79e657920..1343ec7b7 100644 --- a/frontend/src/components/tasks/partials/EditAssignees.vue +++ b/frontend/src/components/tasks/partials/EditAssignees.vue @@ -99,7 +99,7 @@ async function removeAssignee(user: IUser) { await taskStore.removeAssignee({user: user, taskId: props.taskId}) // Remove the assignee from the project - for (const a in assignees.value) { + for (let a = assignees.value.length - 1; a >= 0; a--) { if (assignees.value[a].id === user.id) { assignees.value.splice(a, 1) } diff --git a/frontend/src/components/tasks/partials/EditLabels.vue b/frontend/src/components/tasks/partials/EditLabels.vue index 29a0d5452..18677adea 100644 --- a/frontend/src/components/tasks/partials/EditLabels.vue +++ b/frontend/src/components/tasks/partials/EditLabels.vue @@ -124,9 +124,9 @@ async function removeLabel(label: ILabel) { await taskStore.removeLabel({label, taskId: props.taskId}) } - for (const l in labels.value) { + for (let l = labels.value.length - 1; l >= 0; l--) { if (labels.value[l].id === label.id) { - labels.value.splice(l, 1) // FIXME: l should be index + labels.value.splice(l, 1) } } emit('update:modelValue', labels.value) diff --git a/frontend/src/helpers/case.ts b/frontend/src/helpers/case.ts index cd29bc4d3..0b98fdb0e 100644 --- a/frontend/src/helpers/case.ts +++ b/frontend/src/helpers/case.ts @@ -13,7 +13,7 @@ export function objectToCamelCase(object: Record) { // eslint-disable-next-line @typescript-eslint/no-explicit-any const parsedObject: Record = {} - for (const m in object) { + for (const m of Object.keys(object)) { parsedObject[camelCase(m)] = object[m] // Recursive processing @@ -51,7 +51,7 @@ export function objectToSnakeCase(object: Record) { // eslint-disable-next-line @typescript-eslint/no-explicit-any const parsedObject: Record = {} - for (const m in object) { + for (const m of Object.keys(object)) { parsedObject[snakeCase(m)] = object[m] // Recursive processing diff --git a/frontend/src/helpers/filters.test.ts b/frontend/src/helpers/filters.test.ts index 63b73dc21..ec64bf62a 100644 --- a/frontend/src/helpers/filters.test.ts +++ b/frontend/src/helpers/filters.test.ts @@ -19,11 +19,11 @@ describe('Filter Transformation', () => { } describe('For API', () => { - for (const c in fieldCases) { + for (const [c, snakeCase] of Object.entries(fieldCases)) { it('should transform all filter params for ' + c + ' to snake_case', () => { const transformed = transformFilterStringForApi(c + ' = ipsum', nullTitleToIdResolver, nullTitleToIdResolver) - expect(transformed).toBe(fieldCases[c] + ' = ipsum') + expect(transformed).toBe(snakeCase + ' = ipsum') }) } @@ -416,9 +416,9 @@ describe('Filter Transformation', () => { }) describe('To API', () => { - for (const c in fieldCases) { + for (const [c, snakeCase] of Object.entries(fieldCases)) { it('should transform all filter params for ' + c + ' to snake_case', () => { - const transformed = transformFilterStringFromApi(fieldCases[c] + ' = ipsum', nullTitleToIdResolver, nullTitleToIdResolver) + const transformed = transformFilterStringFromApi(snakeCase + ' = ipsum', nullTitleToIdResolver, nullTitleToIdResolver) expect(transformed).toBe(c + ' = ipsum') }) diff --git a/frontend/src/helpers/saveCollapsedBucketState.ts b/frontend/src/helpers/saveCollapsedBucketState.ts index 1eea00876..f4e0487de 100644 --- a/frontend/src/helpers/saveCollapsedBucketState.ts +++ b/frontend/src/helpers/saveCollapsedBucketState.ts @@ -18,7 +18,7 @@ export const saveCollapsedBucketState = ( ) => { const state = getAllState() state[projectId] = collapsedBuckets - for (const bucketId in state[projectId]) { + for (const bucketId of Object.keys(state[projectId])) { if (!state[projectId][bucketId]) { delete state[projectId][bucketId] } diff --git a/frontend/src/helpers/time/calculateDayInterval.test.ts b/frontend/src/helpers/time/calculateDayInterval.test.ts index 37edef7d8..2cbb750c8 100644 --- a/frontend/src/helpers/time/calculateDayInterval.test.ts +++ b/frontend/src/helpers/time/calculateDayInterval.test.ts @@ -12,13 +12,13 @@ const days = { sunday: 0, } as Record -for (const n in days) { +for (const n of Object.keys(days)) { test(`today on a ${n}`, () => { expect(calculateDayInterval('today', days[n])).toBe(0) }) } -for (const n in days) { +for (const n of Object.keys(days)) { test(`tomorrow on a ${n}`, () => { expect(calculateDayInterval('tomorrow', days[n])).toBe(1) }) @@ -34,7 +34,7 @@ const nextMonday = { sunday: 1, } as Record -for (const n in nextMonday) { +for (const n of Object.keys(nextMonday)) { test(`next monday on a ${n}`, () => { expect(calculateDayInterval('nextMonday', days[n])).toBe(nextMonday[n]) }) @@ -50,7 +50,7 @@ const thisWeekend = { sunday: 0, } as Record -for (const n in thisWeekend) { +for (const n of Object.keys(thisWeekend)) { test(`this weekend on a ${n}`, () => { expect(calculateDayInterval('thisWeekend', days[n])).toBe(thisWeekend[n]) }) @@ -66,7 +66,7 @@ const laterThisWeek = { sunday: 0, } as Record -for (const n in laterThisWeek) { +for (const n of Object.keys(laterThisWeek)) { test(`later this week on a ${n}`, () => { expect(calculateDayInterval('laterThisWeek', days[n])).toBe(laterThisWeek[n]) }) @@ -82,13 +82,13 @@ const laterNextWeek = { sunday: 7 + 0, } as Record -for (const n in laterNextWeek) { +for (const n of Object.keys(laterNextWeek)) { test(`later next week on a ${n} (this week)`, () => { expect(calculateDayInterval('laterNextWeek', days[n])).toBe(laterNextWeek[n]) }) } -for (const n in days) { +for (const n of Object.keys(days)) { test(`next week on a ${n}`, () => { expect(calculateDayInterval('nextWeek', days[n])).toBe(7) }) diff --git a/frontend/src/modules/quickAddMagic/quickAddMagic.test.ts b/frontend/src/modules/quickAddMagic/quickAddMagic.test.ts index f1a6cc20c..b6fb5bde0 100644 --- a/frontend/src/modules/quickAddMagic/quickAddMagic.test.ts +++ b/frontend/src/modules/quickAddMagic/quickAddMagic.test.ts @@ -165,7 +165,7 @@ describe('Parse Task Text', () => { 'at 12:00 am': '0:0', } as const - for (const c in cases) { + for (const c of Object.keys(cases)) { it(`should recognize today with a time ${c}`, () => { const result = parseTaskText(`Lorem Ipsum today ${c}`) @@ -526,7 +526,7 @@ describe('Parse Task Text', () => { ] prefix.forEach(p => { - for (const d in days) { + for (const d of Object.keys(days)) { it(`should recognize ${p}${d}`, () => { const result = parseTaskText(`Lorem Ipsum ${p}${d}`) @@ -556,7 +556,7 @@ describe('Parse Task Text', () => { // This tests only standalone days are recognized and not things like "github", "monitor" or "renewed". // We're not using real words here to generate tests for all days on the fly. - for (const d in days) { + for (const d of Object.keys(days)) { it(`should not recognize ${d} with a space before it but none after it`, () => { const text = `Lorem Ipsum ${d}ipsum` const result = parseTaskText(text) @@ -660,7 +660,7 @@ describe('Parse Task Text', () => { '01.02.25': '2025-2-1', } as Record - for (const c in cases) { + for (const c of Object.keys(cases)) { const assertResult = ({date, text}: ParsedTaskText) => { if (date === null && cases[c] === null) { expect(date).toBeNull() @@ -722,7 +722,7 @@ describe('Parse Task Text', () => { '5th Mar at 3pm': '2021-3-5 15:0', } as Record - for (const c in cases) { + for (const c of Object.keys(cases)) { it(`should parse '${c}' as '${cases[c]}'`, () => { const {date} = parseDate(c, now) if (date === null && cases[c] === null) { @@ -846,7 +846,7 @@ describe('Parse Task Text', () => { }) describe('Priority', () => { - for (const p in PRIORITIES) { + for (const p of Object.keys(PRIORITIES) as Array) { it(`should parse priority ${p}`, () => { const result = parseTaskText(`Lorem Ipsum !${PRIORITIES[p]}`) @@ -969,7 +969,7 @@ describe('Parse Task Text', () => { 'yearly': {type: 'years', amount: 1}, } as Record - for (const c in cases) { + for (const c of Object.keys(cases)) { it(`should parse ${c} as recurring date every ${cases[c].amount} ${cases[c].type}`, () => { const result = parseTaskText(`Lorem Ipsum ${c}`) diff --git a/frontend/src/services/abstractService.ts b/frontend/src/services/abstractService.ts index 347719eb6..73ac253d0 100644 --- a/frontend/src/services/abstractService.ts +++ b/frontend/src/services/abstractService.ts @@ -15,7 +15,7 @@ interface Paths { reset?: string } -function convertObject(o: Record) { +function convertObject(o: unknown) { if (o instanceof Date) { return o.toISOString() } @@ -28,13 +28,14 @@ function prepareParams(params: Record) { return params } - for (const p in params) { - if (Array.isArray(params[p])) { - params[p] = params[p].map(convertObject) + for (const p of Object.keys(params)) { + const val = params[p] + if (Array.isArray(val)) { + params[p] = val.map(convertObject) continue } - params[p] = convertObject(params[p]) + params[p] = convertObject(val) } return objectToSnakeCase(params) diff --git a/frontend/src/services/task.ts b/frontend/src/services/task.ts index 7a4f7d5a7..bdb4bab7d 100644 --- a/frontend/src/services/task.ts +++ b/frontend/src/services/task.ts @@ -62,7 +62,7 @@ export default class TaskService extends AbstractService { model.reminderDates = null // remove all nulls, these would create empty reminders - for (const index in model.reminders) { + for (let index = model.reminders.length - 1; index >= 0; index--) { if (model.reminders[index] === null) { model.reminders.splice(index, 1) } diff --git a/frontend/src/stores/kanban.ts b/frontend/src/stores/kanban.ts index d68ffbc62..f81301b2e 100644 --- a/frontend/src/stores/kanban.ts +++ b/frontend/src/stores/kanban.ts @@ -124,9 +124,9 @@ export const useKanbanStore = defineStore('kanban', () => { let found = false - const findAndUpdate = b => { - for (const t in buckets.value[b].tasks) { - if (buckets.value[b].tasks[t].id === task.id) { + const findAndUpdate = (b: number) => { + for (const [t, taskInBucket] of buckets.value[b].tasks.entries()) { + if (taskInBucket.id === task.id) { const bucket = buckets.value[b] bucket.tasks[t] = task @@ -138,7 +138,7 @@ export const useKanbanStore = defineStore('kanban', () => { } } - for (const b in buckets.value) { + for (let b = 0; b < buckets.value.length; b++) { findAndUpdate(b) if (found) { return diff --git a/frontend/src/views/tasks/ShowTasks.vue b/frontend/src/views/tasks/ShowTasks.vue index 155e79012..192393e77 100644 --- a/frontend/src/views/tasks/ShowTasks.vue +++ b/frontend/src/views/tasks/ShowTasks.vue @@ -287,7 +287,7 @@ async function loadPendingTasks(from: Date|string, to: Date|string, filterId: nu // FIXME: this modification should happen in the store function updateTasks(updatedTask: ITask) { - for (const t in tasks.value) { + for (let t = 0; t < tasks.value.length; t++) { if (tasks.value[t].id === updatedTask.id) { tasks.value[t] = updatedTask // Move the task to the end of the done tasks if it is now done diff --git a/frontend/src/views/tasks/TaskDetailView.vue b/frontend/src/views/tasks/TaskDetailView.vue index 0aa98b27b..c7a70c7df 100644 --- a/frontend/src/views/tasks/TaskDetailView.vue +++ b/frontend/src/views/tasks/TaskDetailView.vue @@ -1174,9 +1174,12 @@ function setRelatedTasksActive() { // If the related tasks are already available, show the form again const el = activeFieldElements['relatedTasks'] - for (const child in el?.children) { - if (el?.children[child]?.id === 'showRelatedTasksFormButton') { - el?.children[child]?.click() + if (!el) { + return + } + for (const child of Array.from(el.children)) { + if ((child as HTMLElement).id === 'showRelatedTasksFormButton') { + (child as HTMLElement).click() break } } diff --git a/frontend/src/views/teams/EditTeam.vue b/frontend/src/views/teams/EditTeam.vue index 6a07b9e69..f1079f9df 100644 --- a/frontend/src/views/teams/EditTeam.vue +++ b/frontend/src/views/teams/EditTeam.vue @@ -349,9 +349,9 @@ async function toggleUserType(member: ITeamMember) { member.admin = !member.admin member.teamId = teamId.value const r = await teamMemberService.value.update(member) - for (const tm in team.value.members) { - if (team.value.members[tm].id === member.id) { - team.value.members[tm].admin = r.admin + for (const tm of team.value.members) { + if (tm.id === member.id) { + tm.admin = r.admin break } }