Use sveltekit-search-params to simplify query string manipulation
#168
Conversation
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (3)
frontend/src/lib/constants/date-ranges.ts (1)
64-64: Handlenullmore explicitly.
Return early ifvalueis null for clarity and consistency.export function getDateRangeByValue(value: string | null): DateRange | undefined { + if (!value) { + return undefined; + } return DATE_RANGES.find((range) => range.value === value); }frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2)
61-69: Elegant alternative to manual URL updates.
Unsure ifpushHistory: falsealigns with user expectations for back navigation; consider toggling it if needed.
79-93: Remove commented code if unneeded.
It clutters the file with no clear purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte(4 hunks)frontend/src/lib/constants/date-ranges.ts(1 hunks)
🔇 Additional comments (3)
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (3)
2-3: Nice imports.
Clean approach to param management with sveltekit-search-params.
25-32: Params definition is clear.
Using typed query parameters simplifies handling.
56-56: Good usage of typed timestamps.
This avoids confusion over string conversions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2)
Line range hint
68-74: Validate date range orderAdd check to ensure end date is after start date.
if (endDate <= startDate) { return; // Or show error message }
77-86: Add type safety for range parameterUse union type for better type safety:
function updateURLParams( range: typeof DATE_RANGES[number]['value'], start?: number, end?: number )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte(2 hunks)
🔇 Additional comments (2)
frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2)
2-3: Clean query parameter setup!Good choice using
sveltekit-search-paramswith proper configuration.Also applies to: 22-29
42-58: Add timestamp validationAdd checks for invalid timestamp values before conversion to prevent runtime errors.
if ( (isCustomPreset && typeof params[DATE_RANGE_START_PARAM] !== 'number') || (isCustomPreset && typeof params[DATE_RANGE_END_PARAM] !== 'number') ) { // Handle invalid timestamps }
… range preset default to fix condition (and align with existing utils)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
frontend/src/lib/util/date-ranges.ts (1)
37-43: Add doc comment
A brief doc forgetDateRangeParamsConfigwould help.frontend/src/lib/types/MetricType/MetricType.ts (2)
20-29: Simplify decode
Use concise checks for METRIC_TYPES.
32-35: Reuse config
Avoid duplicating logic betweengetMetricTypeParamsConfigandgetMetricTypeFromParams.frontend/src/lib/util/sort.ts (1)
4-5: Clear naming
SORT_DIRECTIONSis good, maybe rename for clarity?frontend/src/lib/components/ActionButtons/ActionButtons.svelte (4)
12-12: Maintain consistency
Organize imports similarly across files.
28-30: Effective param usage
Consider defaultingpushHistoryto true if user expects it.
58-58: UI feedback
Disabled button state might confuse users.
60-60: Inline text
"Sort A-Z" might be "Ascending" for clarity.frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (1)
40-55: Add error handling for invalid datesThe effect block should handle cases where
fromAbsolutemight fail with invalid timestamps.$effect(() => { const selectedRange = selectDateRange?.getRange(); const isCustomPreset = selectDateRange?.value === CUSTOM; untrack(() => { + try { calendarDateRange = { start: fromAbsolute( isCustomPreset ? params[DATE_RANGE_START_PARAM] : selectedRange?.[0], getLocalTimeZone() ), end: fromAbsolute( isCustomPreset ? params[DATE_RANGE_END_PARAM] : selectedRange?.[1], getLocalTimeZone() ) }; + } catch (error) { + console.error('Invalid date range:', error); + calendarDateRange = { start: undefined, end: undefined }; + } }); });frontend/src/routes/joins/[slug]/+page.svelte (3)
455-461: Add type safety for sort parametersConsider using a type-safe approach for sort parameters.
-const sortKey = getSortParamKey(sortContext); +const sortKey = getSortParamKey(sortContext) as keyof ReturnType<typeof getSortParamsConfig>;
Line range hint
392-421: Add debouncing to prevent concurrent loadsConsider adding debouncing to prevent multiple rapid calls to
loadDistributions.+let loadDebounceTimeout: NodeJS.Timeout; async function loadDistributions() { if (distributions.length > 0 || isLoadingDistributions) return; + clearTimeout(loadDebounceTimeout); + loadDebounceTimeout = setTimeout(async () => { isLoadingDistributions = true; try { // ... existing code ... } finally { isLoadingDistributions = false; } + }, 300); }
Line range hint
464-497: Add cleanup for stale eventsConsider clearing selectedEvents when the matching point is not found.
if (updatedPoint) { selectedEvents = [ { ...selectedEvents[0], data: updatedPoint } ]; +} else { + selectedEvents = []; + isSheetOpen = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
frontend/src/lib/components/ActionButtons/ActionButtons.svelte(4 hunks)frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte(2 hunks)frontend/src/lib/components/MetricTypeToggle/MetricTypeToggle.svelte(1 hunks)frontend/src/lib/types/MetricType/MetricType.ts(2 hunks)frontend/src/lib/util/date-ranges.ts(2 hunks)frontend/src/lib/util/sort.ts(1 hunks)frontend/src/routes/joins/[slug]/+page.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/MetricTypeToggle/MetricTypeToggle.svelte
🔇 Additional comments (8)
frontend/src/lib/util/date-ranges.ts (1)
8-8: Usesspimport
Good addition for param parsing.frontend/src/lib/types/MetricType/MetricType.ts (1)
1-1: Check import path
EnsureEncodeAndDecodeOptionsimport is correct.frontend/src/lib/util/sort.ts (2)
2-2: Check import version
Make sure we’re pulling from the right subpath.
12-22: Type safety
Theencode/decodeapproach is neat.frontend/src/lib/components/ActionButtons/ActionButtons.svelte (2)
2-3: Unused imports
Check if removed imports affect other components.
33-34: Naming
toggleSortis clear and minimal.frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2)
23-26: LGTM: Clean parameter configurationParameter configuration with
pushHistory: falseandshowDefaults: falseis appropriate for this use case.
74-83: LGTM: Clean parameter updatesGood practice using null to clear unused parameters.
…params config to get values from URLSearchParams (particularly useful server side)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/lib/util/search-params.ts (1)
10-14: Add error handling for invalid parameter configurationsConsider adding try-catch to handle potential decode failures.
const paramEntries = Object.entries(paramsConfig).map(([paramName, paramConfig]) => { const value = searchParams.get(paramName); - let decodedValue = paramConfig.decode(value) ?? paramConfig.defaultValue; + let decodedValue; + try { + decodedValue = paramConfig.decode(value) ?? paramConfig.defaultValue; + } catch (error) { + console.warn(`Failed to decode ${paramName}:`, error); + decodedValue = paramConfig.defaultValue; + } return [paramName, decodedValue]; });frontend/src/lib/types/MetricType/MetricType.ts (1)
24-24: Validate input in encode functionAdd type validation to encode function.
-encode: (value) => value, +encode: (value: MetricType) => METRIC_TYPES.includes(value) ? value : DEFAULT_METRIC_TYPE,frontend/src/lib/util/date-ranges.ts (1)
29-29: Use strict equality checkReplace
==with===for null check.-if (paramValues[DATE_RANGE_START_PARAM] == null || paramValues[DATE_RANGE_END_PARAM] == null) { +if (paramValues[DATE_RANGE_START_PARAM] === null || paramValues[DATE_RANGE_END_PARAM] === null) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
frontend/src/lib/types/MetricType/MetricType.ts(2 hunks)frontend/src/lib/util/date-ranges.ts(1 hunks)frontend/src/lib/util/search-params.ts(1 hunks)
| if (paramValues[DATE_RANGE_START_PARAM] == null || paramValues[DATE_RANGE_END_PARAM] == null) { | ||
| const [start, end] = getDateRange(paramValues[DATE_RANGE_PARAM] || PAST_1_WEEK); | ||
| paramValues[DATE_RANGE_START_PARAM] = start; | ||
| paramValues[DATE_RANGE_END_PARAM] = end; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timestamp validation
Ensure start timestamp is before end timestamp.
if (paramValues[DATE_RANGE_START_PARAM] == null || paramValues[DATE_RANGE_END_PARAM] == null) {
const [start, end] = getDateRange(paramValues[DATE_RANGE_PARAM] || PAST_1_WEEK);
paramValues[DATE_RANGE_START_PARAM] = start;
paramValues[DATE_RANGE_END_PARAM] = end;
+} else if (paramValues[DATE_RANGE_START_PARAM] > paramValues[DATE_RANGE_END_PARAM]) {
+ const [start, end] = getDateRange(PAST_1_WEEK);
+ paramValues[DATE_RANGE_START_PARAM] = start;
+ paramValues[DATE_RANGE_END_PARAM] = end;
}📝 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.
| if (paramValues[DATE_RANGE_START_PARAM] == null || paramValues[DATE_RANGE_END_PARAM] == null) { | |
| const [start, end] = getDateRange(paramValues[DATE_RANGE_PARAM] || PAST_1_WEEK); | |
| paramValues[DATE_RANGE_START_PARAM] = start; | |
| paramValues[DATE_RANGE_END_PARAM] = end; | |
| if (paramValues[DATE_RANGE_START_PARAM] == null || paramValues[DATE_RANGE_END_PARAM] == null) { | |
| const [start, end] = getDateRange(paramValues[DATE_RANGE_PARAM] || PAST_1_WEEK); | |
| paramValues[DATE_RANGE_START_PARAM] = start; | |
| paramValues[DATE_RANGE_END_PARAM] = end; | |
| } else if (paramValues[DATE_RANGE_START_PARAM] > paramValues[DATE_RANGE_END_PARAM]) { | |
| const [start, end] = getDateRange(PAST_1_WEEK); | |
| paramValues[DATE_RANGE_START_PARAM] = start; | |
| paramValues[DATE_RANGE_END_PARAM] = end; | |
| } |
Summary
Simplify client side query string / URL management by using sveltekit-search-params. This is especially helpful to manage types when reading and also setting/updating values without having to manually use
import { page } from '$app/stores'andimport { goto } from '$app/navigation'in reactive statements ($derived,$effect, etc)Also added
getSearchParamValues(searchParams, paramConfigs)to simplify getting value server side, handling type parsing and defaults with the same configs.Checklist
Summary by CodeRabbit
Release Notes
New Dependencies
sveltekit-search-paramspackage to improve URL parameter management.Improvements
Technical Updates