Fix : more messaging flow issues #2497
Conversation
… the idea in console is to use CRUD so let's just go with
|
This PR was not deployed automatically as @HarshMN2345 does not have access to the Railway project. In order to get automatic PR deploys, please add @HarshMN2345 to your workspace on Railway. |
Console (appwrite/console)Project ID: Tip Custom domains work with both CNAME for subdomains and NS records for apex domains |
WalkthroughThis PR makes UI and behavior adjustments across multiple Svelte components: adds stop-propagation on image preview link events; reduces a helper paragraph top margin; introduces a new boolean prop Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to pay extra attention to:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte (1)
118-124: Replace two-way binding with one-way prop onEmptySearch.Line 118 uses
bind:search={data.search}, but the EmptySearch component should only read the search prop, not modify it. Since Svelte props are reactive downward by default and only flow upward when explicitly bound, thebind:directive is unnecessary here.Change
bind:search={data.search}tosearch={data.search}for clarity and to follow Svelte's idiomatic one-way data flow pattern.The button text capitalization ("Clear Search") on line 122 is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/lib/components/imagePreview.svelte(1 hunks)src/lib/elements/forms/helper.svelte(1 hunks)src/lib/layout/responsiveContainerHeader.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/messaging/createMessageDropdown.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/providers/createProviderDropdown.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(5 hunks)src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/userTargetsModal.svelte(1 hunks)
🔇 Additional comments (15)
src/lib/components/imagePreview.svelte (1)
15-16: LGTM! Event propagation fix looks correct.The
stopPropagationmodifiers prevent the link's click and mousedown events from bubbling to parent handlers, which is appropriate when the preview is embedded in other interactive components.src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte (1)
188-188: LGTM! Text normalization improves readability.Changing to lowercase makes the provider type display more natural in the sentence context.
src/routes/(console)/project-[region]-[project]/messaging/providers/createProviderDropdown.svelte (1)
12-12: LGTM! Popover alignment update is consistent.This change aligns the dropdown to the right edge, matching the similar update in
createMessageDropdown.sveltefor UI consistency.src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte (1)
189-189: LGTM! Exit confirmation improves UX.Adding
confirmExit={true}prevents accidental loss of provider configuration in the multi-step wizard.src/routes/(console)/project-[region]-[project]/messaging/createMessageDropdown.svelte (1)
10-10: LGTM! Consistent popover alignment.This matches the alignment change in
createProviderDropdown.svelte, ensuring consistent dropdown positioning across the messaging UI.src/lib/elements/forms/helper.svelte (1)
8-8: LGTM! Margin adjustment tightens form spacing.Reducing the top margin from 8 to 6 units improves form visual density. This affects all helper text instances globally.
src/routes/(console)/project-[region]-[project]/messaging/userTargetsModal.svelte (1)
142-142: LGTM! Explicit slot assignment is clearer.Adding
slot="description"makes the slot projection explicit, which is more robust and follows Svelte best practices.src/routes/(console)/project-[region]-[project]/messaging/topics/+page.svelte (1)
73-73: No issues found. ThehasCustomFiltersOnlyprop is properly implemented.The ResponsiveContainerHeader component correctly defines the prop (default
false, typed as optional boolean), and the conditional logic (lines 119-122) properly switches betweenFiltersandQuickFiltersbased on the prop value. Usage is consistent across both messaging pages.src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte (1)
26-26: LGTM! Clean addition of custom filters flag.The
hasCustomFiltersOnlyprop enables the new custom filters UI mode for the providers page, working in conjunction with the existinghasFiltersflag.src/lib/layout/responsiveContainerHeader.svelte (2)
3-8: LGTM! Filters component properly imported.The import statement is cleanly organized to include the new
Filterscomponent needed for the custom filters UI mode.
31-31: LGTM! Prop properly defined with backward-compatible default.The
hasCustomFiltersOnlyprop is correctly defined with a default value offalse, ensuring existing component usages continue to work without modification.Also applies to: 42-42
src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte (4)
75-75: LGTM! Terminology updated for consistency.The notification message now uses "created" instead of "have been added", which aligns with the broader terminology shift to "create" throughout the subscriber management flow.
91-91: LGTM! Placeholder text normalized.The placeholder text now uses singular "ID" instead of "IDs", which is more standard and consistent with search placeholders elsewhere in the codebase.
102-102: LGTM! Button label updated for clarity.The button label now uses "Create subscriber", which more accurately reflects the action of creating new subscriber entities rather than just adding existing ones.
141-141: LGTM! Modal description updated for consistency.The description now uses "Create subscribers" instead of "Add subscribers", maintaining consistent terminology throughout the subscriber creation flow.
| {#if hasCustomFiltersOnly} | ||
| <Filters query="[]" {columns} {analyticsSource} /> | ||
| {:else} | ||
| <QuickFilters {columns} {analyticsSource} {filterCols} /> | ||
| {/if} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the hardcoded query="[]" value.
The Filters component is being passed a hardcoded query="[]", which means it won't reflect any existing query state from URL parameters. In contrast, in file src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/+page.svelte (line 93), the Filters component receives query={data.query} from server data.
This could lead to a UX issue where active filters from URL params aren't displayed in the UI when using hasCustomFiltersOnly mode.
Consider whether the query should be:
- Passed as a prop to
ResponsiveContainerHeader(similar to other props likecolumns,analyticsSource) - Derived from URL search params within the component
- Intentionally hardcoded to
"[]"(please confirm if this is the expected behavior)
Run the following script to check how Filters component is used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for other usages of the Filters component to understand the expected query prop pattern
rg -n -A2 -B2 '<Filters\s+query=' --type=svelteLength of output: 30
🏁 Script executed:
#!/bin/bash
# Search for Filters component usages across all files
# Try without the type restriction
rg -n -A2 -B2 '<Filters\s+query=' Length of output: 3402
🏁 Script executed:
#!/bin/bash
# Also search for all Filters component usages to understand the pattern
rg -n '<Filters' --glob='*.svelte' | head -50Length of output: 1116
🏁 Script executed:
#!/bin/bash
# Check the specific file for context around the hardcoded query
cat src/lib/layout/responsiveContainerHeader.svelte | sed -n '110,130p'Length of output: 1045
🏁 Script executed:
#!/bin/bash
# Check the referenced comparison file
cat src/routes/*/project-*/messaging/topics/topic-*/+page.svelte 2>/dev/null || echo "File pattern not found, searching..."
fd -e svelte 'topic' --exec grep -l 'Filters.*query=' {} \;Length of output: 5835
🏁 Script executed:
#!/bin/bash
# Check the component definition and props
head -50 src/lib/layout/responsiveContainerHeader.svelteLength of output: 1915
🏁 Script executed:
#!/bin/bash
# Search for how responsiveContainerHeader is used throughout the codebase
rg -n 'responsiveContainerHeader' --glob='*.svelte' -B2 -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for how the component is imported/exported
rg -n 'responsiveContainerHeader' -C3Length of output: 419
🏁 Script executed:
#!/bin/bash
# Check the lib layout index to see what's exported
cat src/lib/layout/index.ts 2>/dev/null || cat src/lib/layout/index.js 2>/dev/null || echo "No index found"Length of output: 1653
🏁 Script executed:
#!/bin/bash
# Look at what's in the layout directory
ls -la src/lib/layout/Length of output: 2251
🏁 Script executed:
#!/bin/bash
# Search for where ResponsiveContainerHeader might be used
rg -in 'ResponsiveContainerHeader|responsive.*container.*header' --glob='*.svelte'Length of output: 3933
🏁 Script executed:
#!/bin/bash
# Check if hasCustomFiltersOnly is used anywhere
rg -n 'hasCustomFiltersOnly' --glob='*.svelte' -B2 -A2Length of output: 2505
🏁 Script executed:
#!/bin/bash
# Check a few actual ResponsiveContainerHeader usages to see their context
sed -n '15,40p' src/routes/\(console\)/project-\[region\]-\[project\]/messaging/topics/\+page.svelteLength of output: 931
🏁 Script executed:
#!/bin/bash
# Check the messaging/providers page for data.query availability
sed -n '1,50p' 'src/routes/(console)/project-[region]-[project]/messaging/providers/+page.svelte'Length of output: 1841
🏁 Script executed:
#!/bin/bash
# Check the messaging/topics page for data.query availability
sed -n '1,100p' 'src/routes/(console)/project-[region]-[project]/messaging/topics/+page.svelte'Length of output: 3357
🏁 Script executed:
#!/bin/bash
# Check if data.query is available in messaging topics page - look for +page.server.ts
cat 'src/routes/(console)/project-[region]-[project]/messaging/topics/+page.server.ts' 2>/dev/null | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the page.ts or +page.ts for these routes
cat 'src/routes/(console)/project-[region]-[project]/messaging/topics/+page.ts' 2>/dev/null | head -80Length of output: 1184
🏁 Script executed:
#!/bin/bash
# Check the PageData type definition
fd -e 'ts' '\$types' 'src/routes/(console)/project-[region]-[project]/messaging/topics/' --exec cat {} \;Length of output: 0
🏁 Script executed:
#!/bin/bash
# Confirm data.query is returned in the page load for messaging/topics
grep -n 'query' 'src/routes/(console)/project-[region]-[project]/messaging/topics/+page.ts'Length of output: 184
🏁 Script executed:
#!/bin/bash
# Check if messaging/providers also has data.query
cat 'src/routes/(console)/project-[region]-[project]/messaging/providers/+page.ts' 2>/dev/null | grep -A5 -B5 'query'Length of output: 962
Add query prop to ResponsiveContainerHeader and pass it to the Filters component.
The hardcoded query="[]" on line 120 prevents custom filters from displaying saved filter state. Both pages using hasCustomFiltersOnly=true (messaging/topics and messaging/providers) have data.query available from their page loaders, but cannot pass it to the component since the query prop doesn't exist.
This breaks the established pattern: all other direct Filters usages in the codebase receive query={data.query}.
Required changes:
- Add
query?: stringto the component's prop definition - Update line 120 to pass
{query}instead ofquery="[]" - Update both pages to pass
query={data.query}when settinghasCustomFiltersOnly
🤖 Prompt for AI Agents
In src/lib/layout/responsiveContainerHeader.svelte around lines 119-123, the
Filters invocation uses a hardcoded query="[]" which prevents restoring saved
filter state; add a new optional prop to the component (query?: string) in the
component's prop definition, change the Filters call on line 120 to pass {query}
instead of query="[]", and update the two pages that set hasCustomFiltersOnly
(messaging/topics and messaging/providers) to pass their loader value as
query={data.query} when rendering this component.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte(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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (6)
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte (6)
19-22: LGTM: Clean public API.The exported props are well-typed and the default value for
titleprovides good ergonomics.
26-31: LGTM: State variables properly initialized.The migration from
$stateto plain local variables is clean, with sensible default values.
88-98: LGTM: Correct reactivity pattern.The function properly creates a new object reference to trigger Svelte reactivity when updating selections.
111-116: LGTM: Clean reactive derivations.The computed values
selectedSize,hasSelection, andtopicSelectionStatesare properly derived from their dependencies.
118-120: Verify the initialization order is intentional.This reactive block resets
selectedtotopicsByIdwhenevershowbecomes true. Combined with lines 100-102 (which also react toshowand callrequest()), the execution order might be:
- Lines 100-102 run and call
request()- Lines 118-120 run and reset
selectedThis means
selectedis restored from props after the API request is triggered. If this is intentional (e.g., to restore previously selected topics when reopening the modal), consider adding a comment. If not, you might want to consolidate the initialization logic.
172-172: LGTM: Improved empty state messaging.The conditional inclusion of
providerType.toLowerCase()provides clearer context when there are topics but none with the required target type.
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTargets.svelte (1)
219-229: Inconsistent empty state styling between draft and non-draft modes.The non-draft empty state now uses
PinkEmptywrapped inCard.Base, but Line 217 still uses the oldEmptycomponent for draft mode. This creates visual inconsistency in the user experience.Consider updating Line 217 to also use the new Pink design system components for consistency. (Documentation link verified as accessible.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTargets.svelte(3 hunks)
🔇 Additional comments (1)
src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTargets.svelte (1)
15-25: Good use of aliasing to avoid naming conflicts.The import structure correctly uses
Empty as PinkEmptyto differentiate between the two Empty components from different libraries. All imports are properly organized and used in the component.
src/routes/(console)/project-[region]-[project]/messaging/providers/create.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
why was this moved from svelte5 to 4?
…iders/create.svelte Co-authored-by: Darshan <itznotabug@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/messaging/actions.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/routes/(console)/project-[region]-[project]/messaging/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
src/routes/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use SvelteKit file conventions: +page.svelte for components, +page.ts for data loaders, +layout.svelte for wrappers, +error.svelte for error handling, and dynamic route params in square brackets like [param]
Files:
src/routes/(console)/project-[region]-[project]/messaging/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/routes/(console)/project-[region]-[project]/messaging/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/routes/(console)/project-[region]-[project]/messaging/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Configure dynamic routes using SvelteKit convention with [param] syntax in route directory names
Files:
src/routes/(console)/project-[region]-[project]/messaging/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
🧠 Learnings (7)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/input.svelte:20-24
Timestamp: 2025-09-25T04:23:14.025Z
Learning: The Selector.Switch component from appwrite.io/pink-svelte doesn't accept `aria-describedby` or rest props (`...restProps`), limiting accessibility customization options.
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Applies to src/routes/**/*.svelte : Use SvelteKit file conventions: +page.svelte for components, +page.ts for data loaders, +layout.svelte for wrappers, +error.svelte for error handling, and dynamic route params in square brackets like [param]
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/actions.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 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/actions.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Applies to **/*.svelte : Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
📚 Learning: 2025-08-31T05:32:42.285Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2284
File: src/lib/components/searchQuery.svelte:26-27
Timestamp: 2025-08-31T05:32:42.285Z
Learning: SvelteKit automatically enables Svelte 5 runes without requiring explicit "use runes" directive or runes: true configuration when using Svelte 5.
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 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/topicsModal.svelte
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/topicsModal.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 (3)
src/routes/(console)/project-[region]-[project]/messaging/message-[message]/updateTopics.svelte (1)
167-170: LGTM!The migration from event-based (
on:update) to callback-based (onUpdate) API is correct. The handler properly receives theselectedpayload and updates the local state.src/routes/(console)/project-[region]-[project]/messaging/actions.svelte (1)
34-37: LGTM!The callback-based API usage is correct and maintains the existing event dispatch pattern with the
selectedpayload.src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte (1)
18-30: LGTM!The callback-based API with the
onUpdateprop is a cleaner pattern than event dispatching. The TypeScript signature is correct and the props are properly defined using Svelte 5 syntax.
| let selected: Record<string, Models.Topic> = $derived( | ||
| Object.fromEntries( | ||
| Object.entries(topicResultsById).filter(([id]) => !isTopicChecked(id)) | ||
| ) as Record<string, Models.Topic> | ||
| ); | ||
| let selectedSize = $derived(Object.keys(selected).length); | ||
| let hasSelection = $derived(selectedSize > 0); |
There was a problem hiding this comment.
Fix inverted selection logic.
The filter condition at line 103 is inverted. Currently, selected includes topics where !isTopicChecked(id) is true, meaning it contains topics that are NOT checked. This is incorrect—selected should contain topics that ARE checked.
When a user selects topics and clicks "Create", the parent component receives this inverted selection, causing the wrong topics to be sent.
Apply this diff to fix the logic:
let selected: Record<string, Models.Topic> = $derived(
Object.fromEntries(
- Object.entries(topicResultsById).filter(([id]) => !isTopicChecked(id))
+ Object.entries(topicResultsById).filter(([id]) => isTopicChecked(id))
) as Record<string, Models.Topic>
);📝 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.
| let selected: Record<string, Models.Topic> = $derived( | |
| Object.fromEntries( | |
| Object.entries(topicResultsById).filter(([id]) => !isTopicChecked(id)) | |
| ) as Record<string, Models.Topic> | |
| ); | |
| let selectedSize = $derived(Object.keys(selected).length); | |
| let hasSelection = $derived(selectedSize > 0); | |
| let selected: Record<string, Models.Topic> = $derived( | |
| Object.fromEntries( | |
| Object.entries(topicResultsById).filter(([id]) => isTopicChecked(id)) | |
| ) as Record<string, Models.Topic> | |
| ); | |
| let selectedSize = $derived(Object.keys(selected).length); | |
| let hasSelection = $derived(selectedSize > 0); |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
around lines 101 to 108, the filter used to build `selected` is inverted (it
currently keeps topics where `!isTopicChecked(id)`), so `selected` contains
un-checked topics; change the filter to keep topics where `isTopicChecked(id)`
is true (remove the negation) so `selected` contains only checked topics; update
any dependent derived stores (`selectedSize`, `hasSelection`) will then
correctly reflect the checked selection.
| $effect(() => { | ||
| if (search !== previousSearch) { | ||
| previousSearch = search; | ||
| const currentShow = show; | ||
| const currentOffset = offset; | ||
| const currentSearch = search; | ||
| if (currentSearch !== prevSearch && prevShow) { | ||
| offset = 0; | ||
| } | ||
| }); | ||
| $effect(() => { | ||
| if (!show) return; | ||
| offset ?? null; | ||
| search ?? null; | ||
| request(); | ||
| }); | ||
| if (currentShow) { | ||
| const shouldRequest = | ||
| !prevShow || currentOffset !== prevOffset || currentSearch !== prevSearch; | ||
| $effect(() => { | ||
| if (show && !wasOpen) { | ||
| selected = topicsById; | ||
| if (shouldRequest) { | ||
| request(); | ||
| } | ||
| } | ||
| wasOpen = show; | ||
| prevShow = currentShow; | ||
| prevOffset = currentOffset; | ||
| prevSearch = currentSearch; | ||
| }); |
There was a problem hiding this comment.
Fix duplicate API calls caused by offset state mutation.
The effect has a critical bug: when search changes, line 119 sets offset = 0, but line 131 updates prevOffset = currentOffset, which was captured before the offset modification. This causes:
- Search changes → offset set to 0 →
request()called (first call) - Offset state change triggers effect re-run
currentOffset(0) ≠prevOffset(old value) →request()called again (duplicate)
This wastes resources and may cause race conditions.
Apply this diff to fix the issue by updating prevOffset when modifying offset inside the effect:
$effect(() => {
const currentShow = show;
const currentOffset = offset;
const currentSearch = search;
+ let effectiveOffset = currentOffset;
+
if (currentSearch !== prevSearch && prevShow) {
offset = 0;
+ effectiveOffset = 0;
}
if (currentShow) {
const shouldRequest =
- !prevShow || currentOffset !== prevOffset || currentSearch !== prevSearch;
+ !prevShow || effectiveOffset !== prevOffset || currentSearch !== prevSearch;
if (shouldRequest) {
request();
}
}
prevShow = currentShow;
- prevOffset = currentOffset;
+ prevOffset = effectiveOffset;
prevSearch = currentSearch;
});🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/messaging/topicsModal.svelte
around lines 114 to 133, the effect sets offset = 0 when search changes but then
assigns prevOffset = currentOffset (captured before the mutation), causing the
effect to detect a spurious offset change and trigger a duplicate request; fix
this by updating prevOffset to 0 immediately after setting offset = 0 (i.e.,
when you mutate offset inside the effect also set prevOffset = 0) so the
subsequent effect run sees prevOffset and currentOffset as equal and avoids the
duplicate API call.
HarshMN2345
force-pushed
the
fix-SER-SER-416-More-Messaging-flow-issues
branch
from
d0d4fd9 to
10aafb7
Compare
What does this PR do?
(Provide a description of what this PR does.)
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?
(Write your answer here.)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Content
✏️ Tip: You can customize this high-level summary in your review settings.