Migration: schedule-editor-migration to Vue 3 #362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR migrates the schedule-editor to Vue 3 by rewriting all components with the Composition API and TypeScript, ensures stable list rendering with explicit keys, refactors app initialization to asynchronously mount with an i18n plugin, and modernizes the build and lint setup—including adding a type-safe API layer with Zod. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/pretalx/frontend/schedule-editor/src/main.js:13` </location>
<code_context>
-app.use(await i18n(lang))
-app.mount('#app')
+const lang = document.querySelector("#app").dataset.gettext;
+(async () => {
+ const i18nPlugin = await i18n(lang)
+ app.use(i18nPlugin)
+ app.mount('#app')
+})()
\ No newline at end of file
</code_context>
<issue_to_address>
Wrapping app initialization in an async IIFE may affect error handling.
Add error handling to the async IIFE to ensure initialization errors are logged or shown to the user.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
aeb8642 to
95dee2f
Compare
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- In the availability v-for you’re using the array index as a key; consider switching to a stable unique identifier (e.g. a timestamp or ID) to avoid rendering glitches when the list changes.
- You’ve enabled the
vue/component-api-stylerule targeting<script setup>, but all components still use the Options API—consider migrating to<script setup>for consistency with your linting config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the availability v-for you’re using the array index as a key; consider switching to a stable unique identifier (e.g. a timestamp or ID) to avoid rendering glitches when the list changes.
- You’ve enabled the `vue/component-api-style` rule targeting `<script setup>`, but all components still use the Options API—consider migrating to `<script setup>` for consistency with your linting config.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
2 times, most recently
from
103a13c to
21dce88
Compare
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
2 times, most recently
from
74824b2 to
624b14b
Compare
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- GridSchedule.vue has become very large—consider extracting the timeslice generation and drag/scroll logic into separate composables or helper modules to improve readability and maintainability.
- You never clean up the IntersectionObserver created in onMounted; use onUnmounted to disconnect it and avoid potential memory leaks.
- In a few v-for loops (e.g. availabilitySlices) you’re still using the array index as key—swap to a stable unique identifier if possible to prevent rendering glitches when the list changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GridSchedule.vue has become very large—consider extracting the timeslice generation and drag/scroll logic into separate composables or helper modules to improve readability and maintainability.
- You never clean up the IntersectionObserver created in onMounted; use onUnmounted to disconnect it and avoid potential memory leaks.
- In a few v-for loops (e.g. availabilitySlices) you’re still using the array index as key—swap to a stable unique identifier if possible to prevent rendering glitches when the list changes.
## Individual Comments
### Comment 1
<location> `src/pretalx/frontend/schedule-editor/src/App.vue:359` </location>
<code_context>
+}
+
+function startDragging({event, session}) {
+ if (availabilities.value && availabilities.value.talks[session.id] && availabilities.value.talks[session.id].length !== 0) {
+ session.availabilities = availabilities.value.talks[session.id]
+ }
</code_context>
<issue_to_address>
Directly mutating session.availabilities may break reactivity.
If session is a prop or part of a reactive source, use a reactive update method instead of direct assignment to ensure changes are tracked.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
2 times, most recently
from
176b616 to
d84dd1b
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/pretalx/frontend/schedule-editor/src/components/Session.vue:59` </location>
<code_context>
+ }
+})
+
+const link = computed(() => {
+ return generateSessionLinkUrl({eventUrl: eventUrl.value, session: props.session})
+})
+
+const isBreak = computed(() => !props.session.code)
</code_context>
<issue_to_address>
Potential undefined access for eventUrl in generateSessionLinkUrl.
'eventUrl' is injected as a plain value, not a ref, so accessing 'eventUrl.value' may be undefined. Use 'eventUrl' directly if it's not a ref, or ensure it is always a ref for consistency.
</issue_to_address>
### Comment 2
<location> `src/pretalx/frontend/schedule-editor/src/App.vue:111` </location>
<code_context>
+const unassignedSortDirection = ref(1) // asc
+const showUnassignedSortMenu = ref(false)
+const newBreakTooltip = ref('')
+const translations = ref({
+ filterSessions: inject('$t')('Filter sessions'),
+ newBreak: inject('$t')('New break'),
+})
+
+const $t = inject('$t')
</code_context>
<issue_to_address>
translations ref is initialized before $t is guaranteed to be available.
Initializing 'translations' with '$t' before it is injected may cause issues if i18n is not yet available. Initialize 'translations' after '$t' is set, or use a computed property to ensure proper timing.
</issue_to_address>
### Comment 3
<location> `src/pretalx/frontend/schedule-editor/src/App.vue:234` </location>
<code_context>
+})
+
+const dateFormat = computed(() => {
+ if ((schedule.value && schedule.value.rooms.length > 2) || !days.value || !days.value.length) return 'dddd DD. MMMM'
+ if (days.value && days.value.length <= 5) return 'dddd DD. MMMM'
+ if (days.value && days.value.length <= 7) return 'dddd DD. MMM'
</code_context>
<issue_to_address>
dateFormat computed property may return undefined if none of the conditions match.
If days.value has more than 7 elements, dateFormat will return undefined. Please add a default return value to handle this case.
</issue_to_address>
### Comment 4
<location> `src/pretalx/frontend/schedule-editor/src/App.vue:254` </location>
<code_context>
+})
+
+async function initApp() {
+ try {
+ schedule.value = await fetchSchedule()
+ eventTimezone = schedule.value.timezone
+ moment.tz.setDefault(eventTimezone)
+ locales.value = schedule.value.locales
+ eventSlug.value = window.location.pathname.split("/")[3]
+
+ await nextTick()
+ if (days.value && days.value.length) {
+ currentDay.value = days.value[0]
+ }
+
+ setTimeout(pollUpdates, 10 * 1000)
</code_context>
<issue_to_address>
Error handling in pollUpdates may cause silent failures.
Consider implementing a maximum retry limit or exponential backoff in 'pollUpdates' to prevent endless error-retry cycles if persistent errors occur.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
setTimeout(pollUpdates, 10 * 1000)
await fetchAdditionalScheduleData()
} catch (error) {
console.error('Failed to initialize app:', error)
}
}
=======
setTimeout(() => pollUpdates(), 10 * 1000)
await fetchAdditionalScheduleData()
} catch (error) {
console.error('Failed to initialize app:', error)
}
}
// Polling state for retries and backoff
let pollRetryCount = 0
const POLL_MAX_RETRIES = 5
const POLL_INITIAL_DELAY = 10 * 1000 // 10 seconds
const POLL_MAX_DELAY = 5 * 60 * 1000 // 5 minutes
function getPollDelay() {
// Exponential backoff: min(initial * 2^retries, max)
return Math.min(POLL_INITIAL_DELAY * Math.pow(2, pollRetryCount), POLL_MAX_DELAY)
}
async function pollUpdates() {
try {
// ... your polling logic here ...
// On success, reset retry state:
pollRetryCount = 0
setTimeout(() => pollUpdates(), POLL_INITIAL_DELAY)
} catch (error) {
pollRetryCount += 1
if (pollRetryCount > POLL_MAX_RETRIES) {
console.error('Polling failed after maximum retries:', error)
// Optionally, notify the user or stop polling
return
}
const delay = getPollDelay()
console.warn(`Polling failed (attempt ${pollRetryCount}), retrying in ${delay / 1000}s`, error)
setTimeout(() => pollUpdates(), delay)
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/pretalx/frontend/schedule-editor/src/components/Session.vue
Outdated
Show resolved
Hide resolved
mariobehling
changed the title
|
Please resolve conflicts and add screenshots of functioning system. |
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
3 times, most recently
from
863c0e9 to
3803140
Compare
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- Consider fully adopting <script setup> in your Vue 3 components to streamline prop/emits definitions and remove the boilerplate defineComponent calls.
- Add cleanup logic in the unmounted hook to disconnect your IntersectionObserver and avoid potential memory leaks after the component is destroyed.
- Use the v-for index parameter for computing grid‐area keys (e.g.
v-for="(room, i) in visibleRooms" :key="room.id" :style="{'grid-area':1 / ${i+2} / auto / auto}") instead of calling visibleRooms.indexOf at render time for more stable keys and better performance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider fully adopting <script setup> in your Vue 3 components to streamline prop/emits definitions and remove the boilerplate defineComponent calls.
- Add cleanup logic in the unmounted hook to disconnect your IntersectionObserver and avoid potential memory leaks after the component is destroyed.
- Use the v-for index parameter for computing grid‐area keys (e.g. `v-for="(room, i) in visibleRooms" :key="room.id" :style="{'grid-area': `1 / ${i+2} / auto / auto`}"`) instead of calling visibleRooms.indexOf at render time for more stable keys and better performance.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
d3bf966 to
69e1ff0
Compare
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
18d3d71 to
b193120
Compare
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
2 times, most recently
from
bbdaebf to
bfce941
Compare
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
bfce941 to
4005657
Compare
Working updated system with composition api using script setup
|
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
6cb87f3 to
18f7452
Compare
hongquan
requested changes
Jul 25, 2025
There was a problem hiding this comment.
Btw, you must validate API response, or any incoming data from external.
Use Zod for it.
I don't have a public code with Zod to show you as example, but you take a look at the 2nd part of this article: https://quan.hoabinh.vn/post/2025/4/do-choi-de-thay-graphql-do-phien (it is Vietnamese, though).
The article show example with Valibot and GraphQL, but you can apply the same with Zod and RESTful API.
Zod API is simpler than Valibot.
| }) | ||
| }, | ||
|
|
||
| getList(url: string): Promise<any[]> { |
There was a problem hiding this comment.
Using any type beats the purpose of using TypeScript (it is like using pure JavaScript).
Why, assume that you have some code:
const talks = await getList()
talks.fly()TypeScript can not help us check if .fly() is valid or not, because you denote talks as any, it can accept any property and method.
|
Try to make the return type narrow. For example, in this code: const durationPretty = computed<string | undefined>(() => {
const minutes = durationMinutes.value
if (!minutes) return undefinedMaking type |
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- The huge computed blocks (especially the timeslices logic) could be extracted into standalone composables or utility modules to improve readability and make unit testing easier.
- I see repeated date/grid‐area formatting logic (e.g. getSliceName and grid area strings)—consider centralizing these in a shared helper to DRY up the code across components.
- Your api.ts file is very large and mixes Zod schemas with HTTP logic—splitting schemas and request functions into separate modules or folders would help keep each file focused and maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The huge computed blocks (especially the timeslices logic) could be extracted into standalone composables or utility modules to improve readability and make unit testing easier.
- I see repeated date/grid‐area formatting logic (e.g. getSliceName and grid area strings)—consider centralizing these in a shared helper to DRY up the code across components.
- Your api.ts file is very large and mixes Zod schemas with HTTP logic—splitting schemas and request functions into separate modules or folders would help keep each file focused and maintainable.
## Individual Comments
### Comment 1
<location> `src/pretalx/frontend/schedule-editor/src/components/GridSchedule.vue:402` </location>
<code_context>
+ return result
+})
+
+const setTimesliceRef = (el: HTMLElement | null) => {
+ if (el && !timesliceRefs.value.includes(el)) timesliceRefs.value.push(el)
+}
</code_context>
<issue_to_address>
Potential memory leak: timesliceRefs array is never cleared.
Since timesliceRefs is only appended to and never cleared, this may cause memory leaks or stale references if the component re-renders or timeslices change. Reset or update the array as needed to prevent this.
</issue_to_address>
### Comment 2
<location> `src/pretalx/frontend/schedule-editor/src/App.vue:522` </location>
<code_context>
+ schedule.value.talks = updatedTalks
+ }
+ since.value = sched.now || schedule.value.now
+ window.setTimeout(pollUpdates, 10 * 30)
+}
+
</code_context>
<issue_to_address>
Polling interval is set to 300ms, which may be too frequent.
This change increases polling frequency from every 10s to every 300ms, which may lead to unnecessary network load and performance degradation. Please consider reverting to the previous interval.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
This code is very bad: return undefined as unknown as T;How come |
async createTalk(talk: Omit<TalkPayload, 'id'>): Promise<z.infer<typeof TalkSchema>> {
const response = await this.saveTalk(talk, { action: 'POST' }) as z.infer<typeof TalkSchema>;
return TalkSchema.parse(response);
}Why repeating export type Talk = z.infer<typeof TalkSchema>and reuse it. Put the Zod schemas and type definitions (from Using too many type cast ( await this.saveTalk(talk, { action: 'POST' }) as z.infer<typeof TalkSchema>You should define const response = await this.saveTalk(talk)the |
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
2 times, most recently
from
401ec92 to
b58cc83
Compare
Saksham-Sirohi
force-pushed
the
vue-3-migration
branch
from
b58cc83 to
0144bbb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migration Fixes part of #359
Summary by Sourcery
Migrate schedule-editor to Vue 3 and Composition API, update list rendering keys, and modernize build and lint configuration
Enhancements:
Build:
Summary by Sourcery
Migrate the schedule-editor codebase from Vue 2 to Vue 3 with full TypeScript and Composition API support, add explicit keys for all list renderings, introduce type-safe API schemas via zod, and modernize the build, lint and config setup.
Enhancements:
Build: