chore: feedback rework#99
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds parameterized haptic feedback via useFeedback and updates many components to invoke vibrate() or vibrate('success'|'error'). Introduces task store isInitialized and guards UI rendering in index.vue with loaders until initialization. Minor presentational tweaks across Loader, Navigation, and header layout. No exported component APIs changed; store now exposes isInitialized. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component
participant useFeedback
participant HapticAPI
User->>Component: click / submit / change
Component->>useFeedback: vibrate(type)
alt type == 'light'
useFeedback->>HapticAPI: impactOccurred('light')?
HapticAPI-->>useFeedback: supported / no-op
else type == 'success'
useFeedback->>HapticAPI: notificationOccurred('success')?
HapticAPI-->>useFeedback: supported / no-op
else type == 'error'
useFeedback->>HapticAPI: notificationOccurred('error')?
HapticAPI-->>useFeedback: supported / no-op
end
note over Component,HapticAPI: If unsupported, vibrate is a no-op
sequenceDiagram
autonumber
participant index.vue
participant taskStore
participant Backend
participant UI
index.vue->>taskStore: update()
taskStore->>Backend: fetch lists/tasks
Backend-->>taskStore: data
taskStore->>taskStore: set isInitialized = true
index.vue->>UI: conditional render based on isInitialized
alt isInitialized == false
UI-->>index.vue: show Loader
else isInitialized == true
UI-->>index.vue: show TasksTodaySwitch + TaskList/CreateCard
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/atrium-telegram/app/composables/useFeedback.ts (1)
23-23: Add missing import ofcreateSharedComposablein apps/atrium-telegramBecause the
@vueuse/nuxtmodule isn’t enabled inapps/atrium-telegram/nuxt.config.ts(and no@vueusepackages are declared in its package.json), calls tocreateSharedComposable(...)won’t be auto-imported and will trigger a ReferenceError at runtime.Please add an explicit import at the top of each composable that uses it. For example, in
apps/atrium-telegram/app/composables/useFeedback.ts:+ import { createSharedComposable } from '@vueuse/core' import { _useFeedback } from './useFeedback.base' export const useFeedback = createSharedComposable(_useFeedback)You should apply the same change wherever
createSharedComposableis used without an import, such as:
apps/atrium-telegram/app/composables/useBackButton.ts- (and any other composables across the repo lacking the import)
This will ensure
createSharedComposableis defined at runtime.
♻️ Duplicate comments (2)
apps/atrium-telegram/app/components/form/UploadUserAvatar.vue (1)
39-39: Duplicate of vibrate API verification.Same concern/checks as noted in CreateTaskList.vue about SSR safety and accepted types.
apps/atrium-telegram/app/components/form/UpdateTaskList.vue (1)
76-76: Duplicate of vibrate API verification.Same SSR/device guard check as other files.
🧹 Nitpick comments (11)
apps/atrium-telegram/app/components/form/UpdateTaskList.vue (1)
154-161: Optional: run store updates in parallel.Minor latency win; semantics remain the same after setAsFocused/Unfocused resolves.
async function onFocus() { try { - await taskStore.setAsFocused(task.id) - await taskStore.update() - await userStore.update() + await taskStore.setAsFocused(task.id) + await Promise.all([taskStore.update(), userStore.update()]) vibrate('success') } catch (error) { console.error(error) vibrate('error') } } async function onUnfocus() { try { - await taskStore.setAsUnfocused(task.id) - await taskStore.update() - await userStore.update() + await taskStore.setAsUnfocused(task.id) + await Promise.all([taskStore.update(), userStore.update()]) vibrate('success') } catch (error) { console.error(error) vibrate('error') } }Also applies to: 167-174
apps/atrium-telegram/app/components/Button.vue (1)
2-5: Add type="button" to avoid unintended form submits.Prevents accidental submit when used inside forms.
- <button + <button + type="button" class="p-3 tg-bg-button w-full rounded-2xl font-medium cursor-pointer active:scale-95 duration-200" @click="vibrate()" >apps/atrium-telegram/app/components/TaskCard.vue (1)
154-161: Optional: parallelize post-action refreshes.Slightly improves responsiveness without changing behavior.
try { - await taskStore.setAsFocused(task.id) - await taskStore.update() - await userStore.update() + await taskStore.setAsFocused(task.id) + await Promise.all([taskStore.update(), userStore.update()]) vibrate('success')try { - await taskStore.setAsUnfocused(task.id) - await taskStore.update() - await userStore.update() + await taskStore.setAsUnfocused(task.id) + await Promise.all([taskStore.update(), userStore.update()]) vibrate('success')Also applies to: 167-174
apps/atrium-telegram/app/components/CreateCard.vue (1)
14-15: Forward the click event to the parent to guarantee handler executionUButton may not bubble click reliably (some UI libs stopPropagation). Emit a component-level click after vibrate to make parent
<CreateCard @click="...">robust.Apply:
<script setup lang="ts"> defineProps<{ label: string, icon: string }>() const { vibrate } = useFeedback() +const emit = defineEmits<{ (e: 'click'): void }>() </script> <template> <div class="flex flex-col gap-4 justify-center items-center h-full min-h-40 border-2 border-default border-dashed rounded-lg"> <UIcon :name="icon" class="size-10 text-muted/50" /> <UButton @@ - @click="vibrate()" + @click="() => { vibrate(); emit('click') }" /> </div> </template>Run a quick manual check that clicking the button opens the modal on the index page.
apps/atrium-telegram/app/stores/task.ts (2)
73-78: Minor: avoid creating a new route instance inside watchBind
useRoute()once for clarity and to align with common patterns.- watch( - () => useRoute().fullPath, + const route = useRoute() + watch( + () => route.fullPath, () => { isTodayOnly.value = false }, )
80-88: Consider explicit loading and error statesRight now
isInitializedconflates “attempt completed” with “data ready.” ConsiderisLoading,error, andhasLoadedfor clearer UI control and retry flows.I can sketch a minimal pattern if helpful.
apps/atrium-telegram/app/components/TasksTodaySwitch.vue (1)
6-6: Use i18n keys and prefer update:modelValue for switch changes
- Replace hardcoded RU strings with
$t(...)for consistency.- Hook into
@update:modelValueto guarantee firing on value changes from the control.- :label="taskStore.isTodayOnly ? 'Задачи на сегодня' : 'Показаны все задачи'" + :label="taskStore.isTodayOnly ? $t('tasks.today_only') : $t('tasks.all_shown')" @@ - @change="vibrate()" + @update:modelValue="() => vibrate()"Add the keys
tasks.today_onlyandtasks.all_shownto your locale files.Also applies to: 8-10, 11-11
apps/atrium-telegram/app/pages/index.vue (4)
3-8: Make the clickable avatar accessibleIf
UAvatarisn’t a native button, add button semantics and keyboard support, or wrap it in a<button>.- <UAvatar + <UAvatar :src="userStore?.avatarUrl ?? undefined" - class="size-14 cursor-pointer hover:scale-95 active:scale-90 duration-200" - @click="handleUploadUserAvatar" + class="size-14 cursor-pointer hover:scale-95 active:scale-90 duration-200" + role="button" + tabindex="0" + aria-label="Upload avatar" + @click="handleUploadUserAvatar" + @keydown.enter.space="handleUploadUserAvatar" />
35-57: Guarding with isInitialized relies on store fixWith the current store, errors or empty responses can keep
isInitializedfalse and the loader permanent. After applying the storefinallyfix, this block will behave as intended. Consider also showing an error state on fetch failure.
76-81: Use.some(...)and return a boolean explicitly; minor perf/readability winCurrent code returns a number (
length) in the filter predicate. Use.somefor short-circuiting and clarity.-const myLists = computed(() => - taskStore.lists.filter( - (taskList) => taskList.chat?.members.some((member) => member.userId === userStore.id), - ).filter((taskList) => taskStore.isTodayOnly ? taskList.tasks.filter((task) => !task.completedAt && task.date && isToday(parseDate(task.date), getLocalTimeZone())).length : true), -) +const myLists = computed(() => + taskStore.lists + .filter((taskList) => + taskList.chat?.members.some((m) => m.userId === userStore.id), + ) + .filter((taskList) => + taskStore.isTodayOnly + ? taskList.tasks.some( + (task) => + !task.completedAt && + task.date && + isToday(parseDate(task.date), getLocalTimeZone()), + ) + : true, + ), +)
81-82: DRY/timezone micro-optimizationCache the timezone once to avoid repeated
getLocalTimeZone()calls in tight loops.-import { getLocalTimeZone, isToday, parseDate } from '@internationalized/date' +import { getLocalTimeZone, isToday, parseDate } from '@internationalized/date' @@ -const myTodayTasks = computed(() => myLists.value.flatMap((taskList) => taskList.tasks.filter((task) => !task.completedAt && task.date && isToday(parseDate(task.date), getLocalTimeZone())))) +const tz = getLocalTimeZone() +const myTodayTasks = computed(() => + myLists.value.flatMap((taskList) => + taskList.tasks.filter( + (task) => !task.completedAt && task.date && isToday(parseDate(task.date), tz), + ), + ), +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
apps/atrium-telegram/app/components/Button.vue(1 hunks)apps/atrium-telegram/app/components/CreateCard.vue(1 hunks)apps/atrium-telegram/app/components/Loader.vue(1 hunks)apps/atrium-telegram/app/components/TaskCard.vue(2 hunks)apps/atrium-telegram/app/components/TasksTodaySwitch.vue(1 hunks)apps/atrium-telegram/app/components/form/CompleteTask.vue(2 hunks)apps/atrium-telegram/app/components/form/CreateTask.vue(2 hunks)apps/atrium-telegram/app/components/form/CreateTaskList.vue(2 hunks)apps/atrium-telegram/app/components/form/UpdateTask.vue(3 hunks)apps/atrium-telegram/app/components/form/UpdateTaskList.vue(3 hunks)apps/atrium-telegram/app/components/form/UploadUserAvatar.vue(2 hunks)apps/atrium-telegram/app/composables/useFeedback.ts(1 hunks)apps/atrium-telegram/app/pages/index.vue(2 hunks)apps/atrium-telegram/app/stores/task.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
apps/atrium-telegram/app/components/form/CreateTask.vue (1)
99-99: LGTM on integrating haptic feedback.Success/error vibrations are placed in the right branches and won’t interfere with the toast flow.
Also applies to: 174-175, 179-180
apps/atrium-telegram/app/components/form/UpdateTask.vue (1)
114-114: LGTM: feedback hooks on update/delete paths.Placement and types look correct; aligns with the composable API.
Also applies to: 201-202, 206-207, 225-226, 230-231
apps/atrium-telegram/app/components/form/CompleteTask.vue (1)
53-53: LGTM: success/error haptics added after toast outcomes.Clean integration; no behavioral regressions apparent.
Also applies to: 84-85, 89-90
apps/atrium-telegram/app/components/form/CreateTaskList.vue (2)
107-107: LGTM: success/error haptics are triggered at sensible points.Vibration after toast and before emit('success') is a good UX sequence; error path mirrors it consistently.
Also applies to: 112-112
62-62: Please verify SSR no-op for hapticFeedback
- The
vibratefunction signature isvibrate(type: 'light' | 'success' | 'error' = 'light'), which matches the expected union of'light','success', and'error'(script output lines 4–14)- Each call is guarded by
hapticFeedback.*.isAvailable(), ensuring no-op on devices without haptic support (script output lines 5–6, 9–10, 13–14)- There is no explicit SSR check in
useFeedback.ts; please confirm that the imported@telegram-apps/sdk-vuehapticFeedback API safely no-ops during server-side renderingapps/atrium-telegram/app/components/form/UploadUserAvatar.vue (1)
78-78: LGTM: haptics on success/error match the pattern elsewhere.Placement after toast is consistent and unobtrusive.
Also applies to: 83-83
apps/atrium-telegram/app/components/form/UpdateTaskList.vue (1)
123-123: LGTM: consistent haptics for update and delete flows.Success/error feedback aligns with the rest of the app.
Also applies to: 128-128, 147-147, 152-152
apps/atrium-telegram/app/components/Button.vue (1)
4-4: LGTM on inline vibrate call.Inline invocation avoids passing the MouseEvent to vibrate; matches new API.
apps/atrium-telegram/app/components/TaskCard.vue (1)
46-46: LGTM: haptics added for open, focus/unfocus, and completion flows.Good differentiation: light tap for UI actions, success/error for outcomes.
Also applies to: 160-160, 163-163, 173-173, 176-176
apps/atrium-telegram/app/pages/index.vue (1)
47-51: Confirm parent click handler triggers from CreateCardGiven CreateCard handles its own button click, ensure the parent
@clickstill fires. After applying the emit-forwarding change in CreateCard.vue, this is guaranteed.
| src="/sushi-heart.svg" | ||
| alt="" | ||
| class="w-12 grayscale-100 opacity-10 motion-preset-pulse motion-duration-1000" | ||
| class="w-12 opacity-50 invert-50 motion-preset-pulse motion-duration-1000" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check Tailwind class support for invert-50.
Tailwind’s core provides invert (100%) and invert-0; invert-50 isn’t standard unless you’ve extended utilities. Prefer invert or an arbitrary value (invert-[.5]) if partial inversion is desired.
Apply if you’re not extending Tailwind:
- class="w-12 opacity-50 invert-50 motion-preset-pulse motion-duration-1000"
+ class="w-12 opacity-50 invert-[.5] motion-preset-pulse motion-duration-1000"Run to verify Tailwind config and usage:
🏁 Script executed:
#!/bin/bash
fd -a 'tailwind.config.*' | xargs -I{} rg -n 'theme|plugins|extend|invert' {}
rg -n 'invert-50|invert-\[\.\d+\]' -SLength of output: 251
Replace non-standard invert-50 with a supported Tailwind utility
Our codebase doesn’t include a tailwind.config.* file extending invert to 50%, so invert-50 won’t generate any CSS by default. You have two options:
- Use Tailwind’s built-in full inversion:
invert(100%) orinvert-0 - Use an arbitrary value for partial inversion:
invert-[.5]
Since there’s no existing extension, please update the class on line 5 of apps/atrium-telegram/app/components/Loader.vue:
- class="w-12 opacity-50 invert-50 motion-preset-pulse motion-duration-1000"
+ class="w-12 opacity-50 invert-[.5] motion-preset-pulse motion-duration-1000"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class="w-12 opacity-50 invert-50 motion-preset-pulse motion-duration-1000" | |
| class="w-12 opacity-50 invert-[.5] motion-preset-pulse motion-duration-1000" |
🤖 Prompt for AI Agents
In apps/atrium-telegram/app/components/Loader.vue around line 5, the class list
uses a non-standard Tailwind utility `invert-50` which won't generate CSS;
replace it with a supported utility such as `invert` (100%) or `invert-0`, or
use an arbitrary value for partial inversion like `invert-[.5]` depending on the
intended visual effect, and update the class string accordingly.
| function vibrate(type: 'light' | 'success' | 'error' = 'light') { | ||
| if (type === 'light' && hapticFeedback.impactOccurred.isAvailable()) { | ||
| hapticFeedback.impactOccurred('light') | ||
| } | ||
|
|
||
| if (type === 'success' && hapticFeedback.notificationOccurred.isAvailable()) { | ||
| hapticFeedback.notificationOccurred('success') | ||
| } | ||
|
|
||
| if (type === 'error' && hapticFeedback.notificationOccurred.isAvailable()) { | ||
| hapticFeedback.notificationOccurred('error') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden availability checks and add sensible fallbacks.
Calling .isAvailable() without guarding can throw if the API isn’t wired (SSR, older clients). Also add a fallback impact for success/error when notification isn’t available, and consider supporting 'warning'.
Apply:
- function vibrate(type: 'light' | 'success' | 'error' = 'light') {
- if (type === 'light' && hapticFeedback.impactOccurred.isAvailable()) {
- hapticFeedback.impactOccurred('light')
- }
-
- if (type === 'success' && hapticFeedback.notificationOccurred.isAvailable()) {
- hapticFeedback.notificationOccurred('success')
- }
-
- if (type === 'error' && hapticFeedback.notificationOccurred.isAvailable()) {
- hapticFeedback.notificationOccurred('error')
- }
- }
+ function vibrate(type: 'light' | 'success' | 'warning' | 'error' = 'light') {
+ const impactAvailable = !!hapticFeedback?.impactOccurred?.isAvailable?.()
+ const notifAvailable = !!hapticFeedback?.notificationOccurred?.isAvailable?.()
+
+ switch (type) {
+ case 'light':
+ if (impactAvailable) hapticFeedback.impactOccurred('light')
+ break
+ case 'success':
+ case 'warning':
+ case 'error':
+ if (notifAvailable) {
+ hapticFeedback.notificationOccurred(type)
+ } else if (impactAvailable) {
+ hapticFeedback.impactOccurred(type === 'error' ? 'heavy' : 'medium')
+ }
+ break
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function vibrate(type: 'light' | 'success' | 'error' = 'light') { | |
| if (type === 'light' && hapticFeedback.impactOccurred.isAvailable()) { | |
| hapticFeedback.impactOccurred('light') | |
| } | |
| if (type === 'success' && hapticFeedback.notificationOccurred.isAvailable()) { | |
| hapticFeedback.notificationOccurred('success') | |
| } | |
| if (type === 'error' && hapticFeedback.notificationOccurred.isAvailable()) { | |
| hapticFeedback.notificationOccurred('error') | |
| } | |
| } | |
| function vibrate(type: 'light' | 'success' | 'warning' | 'error' = 'light') { | |
| const impactAvailable = !!hapticFeedback?.impactOccurred?.isAvailable?.() | |
| const notifAvailable = !!hapticFeedback?.notificationOccurred?.isAvailable?.() | |
| switch (type) { | |
| case 'light': | |
| if (impactAvailable) hapticFeedback.impactOccurred('light') | |
| break | |
| case 'success': | |
| case 'warning': | |
| case 'error': | |
| if (notifAvailable) { | |
| hapticFeedback.notificationOccurred(type) | |
| } else if (impactAvailable) { | |
| hapticFeedback.impactOccurred(type === 'error' ? 'heavy' : 'medium') | |
| } | |
| break | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/atrium-telegram/app/composables/useFeedback.ts around lines 4 to 16, the
vibrate function calls .isAvailable() and API methods directly which can throw
in SSR or older clients and lacks sensible fallbacks; update the function to
first guard that hapticFeedback and the specific API
(impactOccurred/notificationOccurred) exist and that isAvailable is a function
before calling it (wrap availability checks in a try/catch to avoid runtime
throws), add support for a new 'warning' type, and when notificationOccurred
isn’t available fall back to calling impactOccurred with a neutral/medium impact
(and when impactOccurred isn’t available just no-op safely).
| const isInitialized = ref(false) | ||
|
|
There was a problem hiding this comment.
Loader can get stuck: set isInitialized in finally and avoid early return
If the request fails or returns falsy, isInitialized remains false and the UI shows a perpetual loader. Also, the early return prevents initialization state from flipping.
Apply:
async function update() {
try {
const data = await $fetch('/api/task/list', {
headers: {
Authorization: `tma ${initDataRaw.value}`,
},
})
- if (!data) {
- return
- }
-
- lists.value = data
-
- isInitialized.value = true
+ lists.value = data ?? []
} catch (error) {
if (error instanceof Error) {
if (error.message.includes('401')) {
// No session
}
if (error.message.includes('404')) {
// Not found
}
}
- }
+ } finally {
+ isInitialized.value = true
+ }
}Also applies to: 31-34
🤖 Prompt for AI Agents
In apps/atrium-telegram/app/stores/task.ts around lines 16-17 (and similarly
lines 31-34), the loader can get stuck because isInitialized is only set true on
successful paths and early returns prevent flipping the flag; move the
assignment into a finally-equivalent path so isInitialized is set to true
regardless of success or failure, remove early returns that skip setting it, and
ensure any error handling still runs before marking initialized (e.g., wrap
async logic in try/catch and set isInitialized = true in the finally block).
|
1 participant
Summary by CodeRabbit
New Features
Style