fix: Resolve issues in Messaging screens #2422
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughThis PR makes multiple UI and layout updates across the messaging console and related components: it restructures the responsive header to move QuickFilters into a row-aligned inner Layout.Stack; replaces explicit header stacks with ResponsiveContainerHeader on the providers page; standardizes several empty-state renderings to use EmptySearch or move documentation links into description slots; wraps image previews in a Layout.Stack with a max width; changes Popover placements for two dropdowns; binds forms for footer-triggered submission; updates schedule formatting and several legend/button labels; and conditionally renders wizard step headers only when title or subtitle exist. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (2)
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. Comment |
HarshMN2345
changed the title
and resolve double-click issue in topics selection using derived selection state
![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
🧹 Nitpick comments (6)
src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte (2)
24-25: Use proper typing instead ofanyfor formRef.The
anytype removes type safety and IntelliSense support. Consider using the appropriate Form component type.Apply this diff to improve type safety:
-let formRef: any; +let formRef: { triggerSubmit: () => void };Alternatively, if the Form component exports a specific type, use that instead.
245-249: Guard against undefinedformRefon Create button click.
formRef is assigned asynchronously—use optional chaining to avoid runtime errors:- <Button on:click={() => formRef.triggerSubmit()}>Create</Button> + <Button on:click={() => formRef?.triggerSubmit()}>Create</Button>src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte (2)
121-123: Consider removing this intermediate state.The
topicSelectionStatesreactive statement creates an intermediate mapping that just derives!!selected[id]. This can be computed directly in the template without the extra reactive state.The checkbox at line 147 could directly use:
-checked={topicSelectionStates[topicId] || false} +checked={!!selected[topicId]}And remove lines 121-123 entirely. This simplifies the code without changing behavior.
35-46: ApprovegetTopicTotaland recommend consolidating mapping logic
The new helper cleanly handles Email, Sms, and Push totals with a safe default. Similar provider-type→total mappings appear in sendModal.svelte, scheduleModal.svelte, wizard/store.ts, wizard/step3.svelte, and targets.svelte; consider extracting this switch into a shared utility module to remove duplication.src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/(components)/schedule.svelte (2)
32-38: Consider whether displaying seconds is necessary.The
timeOptionsincludessecond: 'numeric', which will display seconds in the helper text (line 76-79). Since users can only select date, hour, and minute in the input fields (InputTime at line 67), the seconds will always be:00. Consider removing the seconds from the display format for a cleaner UX.Apply this diff if you'd like to remove seconds from the display:
const timeOptions: Intl.DateTimeFormatOptions = { hour: 'numeric', minute: 'numeric', - second: 'numeric', hourCycle: 'h23', timeZoneName: 'longGeneric' };
76-79: Use a dynamic locale instead of hardcoding'en'.
In schedule.svelte, bothtoLocaleDateString('en', …)andtoLocaleTimeString('en', …)should consume a locale value (for example, from user preferences or$page.data.locale) rather than the fixed'en'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/(components)/schedule.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/email.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/push.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/sms.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/createMessageDropdown.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/createProviderDropdown.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/popoverContent.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/messaging/userTargetsModal.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/push.svelte
🔇 Additional comments (12)
src/routes/(console)/project-[region]-[project]/messaging/providers/createProviderDropdown.svelte (1)
12-12: LGTM! Popover alignment improved.The placement change from
bottom-endtobottom-startimproves the UI alignment by anchoring the dropdown to the left edge of the trigger button, which is consistent with the identical change increateMessageDropdown.svelte.src/routes/(console)/project-[region]-[project]/messaging/createMessageDropdown.svelte (1)
10-10: LGTM! Consistent popover alignment.The placement change from
bottom-endtobottom-startmatches the identical update increateProviderDropdown.svelte, ensuring consistent dropdown alignment across the messaging interface.src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/sms.svelte (1)
118-120: LGTM! Label updated for consistency.The fieldset legend change from "Schedule" to "Settings" aligns with similar updates across other messaging create views (email, push). This provides a more generic grouping that can accommodate additional settings beyond scheduling if needed in the future.
src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/email.svelte (1)
136-138: LGTM! Consistent label update.The fieldset legend change from "Schedule" to "Settings" matches the update in sms.svelte, maintaining consistency across all messaging create flows.
src/routes/(console)/project-[region]-[project]/messaging/providers/popoverContent.svelte (3)
3-3: LGTM!The Layout import is correctly added to support the new Layout.Stack wrapper.
15-17: Clarify the purpose of the Layout.Stack wrapper and "image-wrapper" class.The ImagePreview is now wrapped in Layout.Stack with an "image-wrapper" class, but no corresponding CSS rules are defined in this file. Please verify:
- Does the Layout.Stack component provide the necessary styling, or is the "image-wrapper" class styled globally?
- What specific layout issue does this wrapper address?
9-9: LGTM!The width constraint using
max-inline-sizeis well-implemented. Using logical properties respects writing direction and improves internationalization support.Also applies to: 22-24
src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte (1)
190-190: LGTM!The Form binding is correctly implemented and preserves the existing
onSubmithandler.src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte (2)
74-74: LGTM! Improved notification message with count and pluralization.The updated notification message provides better user feedback by showing the exact number of subscribers added with correct pluralization.
147-147: LGTM! Cleaner inline link text.The shortened "Learn more" text is more concise while remaining clear in context.
src/routes/(console)/project-[region]-[project]/messaging/create-[type]/(type)/(components)/schedule.svelte (2)
24-24: LGTM! Better label clarity.Changing the option label from "Schedule" to "Custom" improves clarity, especially now that the select field itself has the label "Schedule" (line 63). The options "Now" vs "Custom" are more intuitive than "Now" vs "Schedule".
63-69: LGTM! Good UX improvement.The changes improve the user experience:
- Adding a "Schedule" label to the select makes the form clearer
- Conditionally rendering date/time inputs only when "Custom" is selected reduces visual clutter
- The horizontal layout for date and time inputs is intuitive
The required validation and data binding remain intact.
src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/userTargetsModal.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte
Show resolved
Hide resolved
| </script> | ||
|
|
||
| <div class="u-flex-vertical u-gap-16"> | ||
| <div class="u-flex-vertical u-gap-16 popover-content"> |
There was a problem hiding this comment.
Discussed the approach for this on DMs. cc @HarshMN2345
src/routes/(console)/project-[region]-[project]/messaging/providers/popoverContent.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte (1)
48-48: Remove redundantdata.search !== 'emptycheck
The loader’sgetSearchhelper only returns a URL parameter orundefined—it never produces the literal"empty". Update the Svelte branch to:{:else if data.search}[src/routes/.../messaging/providers/+page.svelte:48]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/routes/(console)/project-[region]-[project]/messaging/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/popoverContent.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/topics/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
- src/routes/(console)/project-[region]-[project]/messaging/providers/popoverContent.svelte
- src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.svelte
- src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: Svelte 5 uses `onclick` instead of `on:click` for event handlers. Event handlers are now properties like any other rather than directives that require the `on:` prefix.
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: In Svelte 5, the `on:` directive syntax (like `on:click`) has been replaced with direct property syntax (like `onclick`). This is part of the migration from Svelte 4 to Svelte 5 where event handlers are treated as regular properties.
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (5)
src/routes/(console)/project-[region]-[project]/messaging/+page.svelte (1)
246-255: LGTM! EmptySearch usage is consistent.The addition of
target="messages"andsearch={data.search}props aligns with the pattern used in the topics and providers pages.src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte (3)
20-36: ResponsiveContainerHeader refactor looks clean.The replacement of the inline header composition with
ResponsiveContainerHeaderimproves consistency and maintainability.
49-55: LGTM! EmptySearch usage is consistent.The addition of
target="providers"andsearch={data.search}props aligns with the pattern used in other messaging pages.
61-75: Correct slot syntax for actions.The use of
<svelte:fragment slot="actions">is the correct Svelte syntax for named slots and works in both Svelte 4 and 5.src/routes/(console)/project-[region]-[project]/messaging/topics/+page.svelte (1)
100-106: Ensure consistent Svelte syntax in messaging routes
Messaging files undersrc/routes/(console)/project-[region]-[project]/messagingpredominantly use Svelte 4 (export let data), butmessaging/providers/+page.svelteuses Svelte 5 ($props()). Confirm this mix is intentional or align syntax across all messaging routes.
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte
Show resolved
Hide resolved
| offset; | ||
| search; |
There was a problem hiding this comment.
just used the vars for no reason? 👀
| </script> | ||
|
|
||
| <div class="u-flex-vertical u-gap-16"> | ||
| <div class="u-flex-vertical u-gap-16 popover-content"> |
There was a problem hiding this comment.
Discussed the approach for this on DMs. cc @HarshMN2345
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit
Refactor
UX
Style