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 targeted UI and layout changes across the messaging console: restructures the non-small responsive header DOM to move QuickFilters into a row-aligned inner Layout.Stack; replaces an inline two-column header with ResponsiveContainerHeader on the providers page; adjusts empty-state and popover presentations (documentation link placement, image wrapper, popover placements); adds form binding to enable programmatic submit; changes schedule formatting and labels; conditionally renders wizard step headers; and applies several small copy and button-label tweaks. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 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)
🔇 Additional comments (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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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.
| icon={IconBookOpen} | ||
| target="_blank" | ||
| rel="noreferrer" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add noopener to the rel attribute for security.
When using target="_blank", best practice is to include both noopener and noreferrer to prevent security vulnerabilities (tabnapping) and ensure privacy.
Apply this diff:
icon={IconBookOpen}
target="_blank"
- rel="noreferrer" />
+ rel="noopener noreferrer" />📝 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.
| icon={IconBookOpen} | |
| target="_blank" | |
| rel="noreferrer" /> | |
| icon={IconBookOpen} | |
| target="_blank" | |
| rel="noopener noreferrer" /> |
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte
around lines 229 to 231, the anchor with target="_blank" currently uses
rel="noreferrer"; update the rel attribute to include noopener as well (e.g.
rel="noopener noreferrer") to prevent tabnapping while preserving privacy.
| <span>Topics selected</span> | ||
| </Layout.Stack> | ||
| <Button submit disabled={!hasSelection}>Add</Button> | ||
| <Button submit disabled={!hasSelection}>Create</Button> |
There was a problem hiding this comment.
Reconsider the button label change from "Add" to "Create".
Similar to the change in userTargetsModal.svelte, this modal's purpose is to select existing topics to send a message to. The button label "Add" more accurately describes this action (adding selected topics to the message), whereas "Create" suggests creating new topics.
Consider reverting to "Add" or using "Add topics" for semantic clarity.
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
around line 193, the button label was changed from "Add" to "Create" but this
modal selects existing topics rather than creating new ones; change the button
text back to "Add" (or to a clearer phrase like "Add topics") to reflect that
the action attaches selected topics to the message rather than creating topics,
updating the Button component label accordingly.
| <span>Users selected</span> | ||
| </Layout.Stack> | ||
| <Button submit disabled={!hasSelection}>Add</Button> | ||
| <Button submit disabled={!hasSelection}>Create</Button> |
There was a problem hiding this comment.
Reconsider the button label change from "Add" to "Create".
The modal's purpose is to select existing user targets to add as subscribers to a topic. The button label "Add" more accurately describes this action, whereas "Create" suggests creating new entities. The underlying logic (addTargets handler in the parent component) does create subscriber records, but from the user's perspective in this modal, they're selecting/adding targets, not creating them.
Consider reverting to "Add" or using "Add subscribers" for semantic clarity.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/messaging/userTargetsModal.svelte
around line 219, the button label was changed to "Create" but the modal's action
is selecting/adding existing user targets; change the label back to "Add" or a
clearer label like "Add subscribers" to accurately reflect the UX. Update the
Button text prop/value accordingly so it reads "Add" or "Add subscribers" and
ensure any aria-labels or tests referencing the old label are updated to match.
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