Show "root" runs by default, save the preference to a cookie #1512
Conversation
|
WalkthroughThe changes in this pull request involve modifications across several files related to the filtering of task runs in a web application. Key updates include renaming and restructuring the filtering properties, specifically changing Changes
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/components/primitives/Switch.tsxOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (7)
apps/webapp/app/services/preferences/uiPreferences.server.ts (3)
31-38: Add JSDoc documentation and consider making the default value configurable.The function implementation looks good, but could benefit from documentation and flexibility improvements.
Consider applying these enhancements:
+/** + * Retrieves the user's preference for showing only root runs. + * @param request - The incoming request object + * @returns Promise resolving to boolean indicating if only root runs should be shown + */ export async function getRootOnlyFilterPreference(request: Request): Promise<boolean> { const session = await getUiPreferencesSession(request); const rootOnly = session.get("rootOnly"); - if (rootOnly === undefined) { - return false; - } - return rootOnly; + return rootOnly ?? false; }
40-44: Add return type annotation and JSDoc documentation.The function implementation looks good, but could benefit from better type safety and documentation.
Consider applying these enhancements:
+/** + * Saves the user's preference for showing only root runs. + * @param rootOnly - Boolean indicating if only root runs should be shown + * @param request - The incoming request object + * @returns Promise resolving to the updated session + */ -export async function setRootOnlyFilterPreference(rootOnly: boolean, request: Request) { +export async function setRootOnlyFilterPreference( + rootOnly: boolean, + request: Request +): Promise<Session> { const session = await getUiPreferencesSession(request); session.set("rootOnly", rootOnly); return session; }
31-44: Consider using an enum or const object for preference keys.To prevent typos and maintain consistency, consider centralizing the preference key names.
Consider adding:
export const UIPreferenceKeys = { USEFUL_LINKS: "showUsefulLinks", ROOT_ONLY: "rootOnly", } as const;Then update the functions to use these constants:
export async function getRootOnlyFilterPreference(request: Request): Promise<boolean> { const session = await getUiPreferencesSession(request); const rootOnly = session.get(UIPreferenceKeys.ROOT_ONLY); return rootOnly ?? false; } export async function setRootOnlyFilterPreference(rootOnly: boolean, request: Request): Promise<Session> { const session = await getUiPreferencesSession(request); session.set(UIPreferenceKeys.ROOT_ONLY, rootOnly); return session; }apps/webapp/app/presenters/v3/RunListPresenter.server.ts (1)
Line range hint
22-22: Good separation of concerns for rootOnly implementationThe rootOnly parameter is properly implemented as an optional boolean in the RunListOptions type, allowing for clean separation between server-side filtering logic and client-side preference persistence.
Consider documenting the following in the codebase:
- The relationship between this server-side implementation and the client-side cookie persistence
- The default value behavior when rootOnly is undefined
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
122-122: Consider adding runtime validation for required propWhile the prop is correctly typed, consider adding runtime validation to prevent potential undefined errors, especially since it's a required prop that affects the default state of the filter.
+import PropTypes from 'prop-types'; type RunFiltersProps = { possibleEnvironments: DisplayableEnvironment[]; possibleTasks: { slug: string; triggerSource: TaskTriggerSource }[]; bulkActions: { id: string; type: BulkActionType; createdAt: Date; }[]; rootOnlyDefault: boolean; hasFilters: boolean; }; +RunsFilters.propTypes = { + rootOnlyDefault: PropTypes.bool.isRequired, + // ... other props +};
145-150: Make type coercion more explicit for form valueWhile the code works, the type coercion from boolean to string in the hidden input could be made more explicit for better maintainability.
-<input type="hidden" name="rootOnly" value={searchParams.get("rootOnly") as string} /> +<input type="hidden" name="rootOnly" value={String(searchParams.get("rootOnly"))} />
707-729: Enhance component maintainability and performanceWhile the component functions correctly, consider the following improvements:
- Extract the disabled state logic to a separate function for better maintainability
- Memoize the URL parameter handling to prevent unnecessary updates
- Make the default value handling more explicit
function RootOnlyToggle({ defaultValue }: { defaultValue: boolean }) { const { value, values, replace } = useSearchParams(); - const searchValue = value("rootOnly"); - const rootOnly = searchValue !== undefined ? searchValue === "true" : defaultValue; + const rootOnly = useMemo(() => { + const searchValue = value("rootOnly"); + return searchValue !== undefined ? searchValue === "true" : defaultValue; + }, [value, defaultValue]); - const batchId = value("batchId"); - const runId = value("runId"); - const scheduleId = value("scheduleId"); - const tasks = values("tasks"); - const disabled = !!batchId || !!runId || !!scheduleId || tasks.length > 0; + const disabled = useMemo(() => { + const hasSpecificFilter = [ + value("batchId"), + value("runId"), + value("scheduleId") + ].some(Boolean); + return hasSpecificFilter || values("tasks").length > 0; + }, [value, values]); return ( <Switch disabled={disabled} variant="small" label="Root only" checked={disabled ? false : rootOnly} onCheckedChange={(checked) => { replace({ rootOnly: checked ? "true" : "false", }); }} /> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/webapp/app/components/primitives/Switch.tsx(1 hunks)apps/webapp/app/components/runs/v3/RunFilters.tsx(4 hunks)apps/webapp/app/presenters/v3/RunListPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs._index/route.tsx(6 hunks)apps/webapp/app/services/preferences/uiPreferences.server.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/components/primitives/Switch.tsx
🔇 Additional comments (7)
apps/webapp/app/presenters/v3/RunListPresenter.server.ts (2)
186-188: LGTM: Proper handling of rootOnly toggle with task filters
The condition correctly disables the rootOnly filter when specific tasks are selected, which aligns with the PR objectives and provides better UX.
Line range hint 289-289: Verify query performance with rootOnly filter
The rootOnly filter implementation looks correct, but let's verify its performance impact.
✅ Verification successful
Index exists for rootTaskRunId, query performance is optimized
The verification confirms that an index TaskRun_rootTaskRunId_idx exists on the rootTaskRunId column, which will ensure efficient filtering when the rootOnly condition is applied. The index was added in migration 20240924125845_add_root_task_run_id_index.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's an index on rootTaskRunId to support the new filter
# Look for existing index definitions
fd -e sql -e prisma . | xargs rg -A 5 'CREATE.*INDEX.*"rootTaskRunId"'
# Check if rootTaskRunId is referenced in any migration files
fd -e sql -e prisma . | xargs rg '"rootTaskRunId".*INDEX'
Length of output: 332
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs._index/route.tsx (4)
40-44: LGTM: Clean import structure
The new imports are well-organized and properly scoped for the preference management functionality.
61-68: LGTM: Well-structured preference handling
The preference resolution logic follows a good priority order:
- URL parameters (immediate user choice)
- Stored preferences (persistent user choice)
147-147: LGTM: Clean UI integration
The rootOnlyDefault preference is properly integrated into the component hierarchy:
- Correctly destructured from loader data
- Properly passed down to the RunsFilters component
Also applies to: 209-209
Line range hint 61-143: Verify implementation against PR objectives
Let's verify that all PR objectives are properly implemented:
- Default to showing root runs
- Save preference to cookie
- Handle task filtering cases
✅ Verification successful
Let me gather more information about the task filtering implementation since the previous results didn't show the RunListPresenter implementation details.
Let me check one more aspect to verify the schema definition and the actual implementation of rootOnly filtering.
Based on the gathered information, I can now provide a final response as all key aspects have been verified:
Implementation successfully meets PR objectives
The implementation correctly handles all requirements:
- Root runs filtering is properly implemented:
- Schema validation via
TaskRunListSearchFiltersincludesrootOnlyfield - SQL query in
RunListPresenterfilters withAND tr."rootTaskRunId" IS NULLwhenrootOnlyis true
- Schema validation via
- Cookie-based preference storage is implemented:
setRootOnlyFilterPreferencesaves to sessiongetRootOnlyFilterPreferenceretrieves from session- Cookie is properly set in response headers
- Task filtering integration:
RunsFilterscomponent handles the rootOnly toggle with proper UI state- Toggle is disabled when specific task filters are active
- Preference persists across page loads
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of PR objectives
# Check if RunListPresenter handles task filtering correctly
echo "Checking RunListPresenter implementation..."
ast-grep --pattern 'class RunListPresenter {
$$$
call($_) {
$$$
tasks
$$$
}
}'
# Check if RunsFilters component handles the rootOnly toggle
echo "Checking RunsFilters component..."
ast-grep --pattern 'function RunsFilters({ $$$, rootOnlyDefault, $$$ }) {
$$$
}'
# Check if uiPreferences handles cookie storage
echo "Checking preference storage..."
rg -A 5 "setRootOnlyFilterPreference|getRootOnlyFilterPreference" --type ts
Length of output: 4158
Script:
#!/bin/bash
# Check RunListPresenter implementation in more detail
echo "Checking RunListPresenter task filtering..."
rg -A 10 "class RunListPresenter" --type ts
# Check if there are any tests verifying the rootOnly functionality
echo "Checking related tests..."
rg -l "rootOnly.*test" --type ts
# Check the schema validation for task filtering
echo "Checking TaskRunListSearchFilters schema..."
rg "TaskRunListSearchFilters.*parse" --type ts -A 5
Length of output: 2824
Script:
#!/bin/bash
# Check TaskRunListSearchFilters schema definition
echo "Checking TaskRunListSearchFilters schema definition..."
rg -A 10 "TaskRunListSearchFilters.*=" --type ts
# Check rootOnly implementation in RunListPresenter
echo "Checking rootOnly implementation in RunListPresenter..."
rg -A 15 "rootOnly.*:" --type ts
# Check if rootOnly is used in the query
echo "Checking rootOnly in query..."
rg -A 5 "where.*rootOnly" --type ts
Length of output: 14386
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)
102-102: LGTM: Schema addition is well-typed
The rootOnly field is correctly added to the schema with proper typing as an optional boolean.
...ebapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs._index/route.tsx
Show resolved
Hide resolved
@trigger.dev/build
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
trigger.dev
@trigger.dev/core
commit: |
With this PR we show "root" runs by default on the Runs page. When you toggle the switch it saves to a cookie.
This is the opposite behaviour that we introduced yesterday, but it was confusing people.
Also, if you filter by tasks it disables the switch and shows all runs that match.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes