Run page links to tasks & replay run modal improvements #1364
Conversation
…or batchTrigger from a run
# Conflicts: # apps/webapp/app/presenters/v3/SpanPresenter.server.ts
🦋 Changeset detectedLatest commit: 50d5680 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes encompass various updates across multiple components and files, primarily focusing on logging behavior, user interface enhancements, and the introduction of new hooks and properties. Key modifications include the removal of accessory attributes from logging in the Changes
Possibly related PRs
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (9)
packages/database/prisma/migrations/20240926110455_add_parent_span_id_index_to_the_task_run_table/migration.sql (1)
1-2: LGTM! Consider the following points for optimal performance.The addition of this index on the
parentSpanIdcolumn of theTaskRuntable is a good optimization step. The use ofCONCURRENTLYandIF NOT EXISTSdemonstrates thoughtful consideration for production environments and idempotent migrations.While this change is approved, please consider the following points to ensure optimal performance:
- Evaluate the impact on write performance, especially if there are frequent inserts or updates to the TaskRun table.
- Monitor the size of the index and its impact on database storage.
- Analyze query patterns to confirm that this index will indeed be beneficial for your most common queries.
- Consider implementing a monitoring strategy to track the usage and effectiveness of this index over time.
These considerations will help ensure that the index provides the intended performance benefits without introducing unexpected issues.
.changeset/three-olives-love.md (1)
1-5: LGTM! Consider adding more context about the impact.The changeset file is correctly formatted and provides a clear description of the change. This update aligns well with the PR objectives of improving the run inspector and enhancing the overall user experience.
Consider adding a brief note about how this change impacts the user experience. For example:
--- "@trigger.dev/sdk": patch --- -Removed the inline-code accessory from the logs when calling trigger or batchTrigger from a run +Removed the inline-code accessory from the logs when calling trigger or batchTrigger from a run. +This change simplifies the log output, making it easier for users to read and understand the execution flow.apps/webapp/app/hooks/useReplaceSearchParams.ts (1)
4-22: Consider adding TypeScript types for improved type safety.To enhance type safety and improve developer experience, consider adding explicit return type for the hook and type for the
replaceSearchParamfunction:import { URLSearchParams } from 'url'; type ReplaceSearchParamFunction = (key: string, value?: string) => void; export function useReplaceSearchParams(): { searchParams: URLSearchParams; setSearchParams: React.Dispatch<React.SetStateAction<URLSearchParams>>; replaceSearchParam: ReplaceSearchParamFunction; } { // ... existing implementation ... }This addition will provide better type inference and catch potential type-related errors early in the development process.
apps/webapp/app/presenters/v3/RunPresenter.server.ts (1)
152-152: LGTM: Consistent inclusion of rootTaskRun, with a refactoring suggestionThe addition of
rootTaskRunto the returned run object here is consistent with the earlier change and ensures the data is included in all code paths. This supports the PR's objective of enhancing task hierarchy navigation.Consider refactoring the method to have a single return statement or a common object construction for the run data. This could improve maintainability by reducing duplication and ensuring consistency across different code paths.
apps/webapp/app/components/runs/v3/ReplayRunDialog.tsx (1)
153-166: LGTM: Improved button layout and styling.The changes to the button layout and styling enhance the form's usability and visual appeal:
- Addition of a Cancel button improves user control.
- Smaller button variants create a more compact look.
- Justified layout improves visual balance.
Consider adding an
aria-labelto the Cancel button to improve accessibility:- <Button variant="tertiary/small">Cancel</Button> + <Button variant="tertiary/small" aria-label="Cancel replay">Cancel</Button>apps/webapp/app/presenters/v3/RunListPresenter.server.ts (1)
Line range hint
171-341: Overall implementation ofrootTaskRunIdlooks good. Consider adding documentation.The addition of
rootTaskRunIdto theRunListPresenteris well-implemented and consistent across the return type, SQL query, and mapping function. This change aligns with the PR objectives and will enable better navigation within the task hierarchy.Consider adding a brief comment explaining the purpose of
rootTaskRunIdin the context of task hierarchy navigation. This would improve code maintainability and help future developers understand the significance of this field.apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
282-287: LGTM: Enhanced task identifier display with root task indicationThe changes to the task identifier cell are well-implemented:
- The flex layout ensures proper alignment of the identifier and badge.
- The conditional rendering of the "Root" badge provides valuable information at a glance.
- Using the "extra-small" variant for the badge is a good choice for space efficiency.
Consider adding an
aria-labelto the badge for improved accessibility. For example:- {run.rootTaskRunId === null ? <Badge variant="extra-small">Root</Badge> : null} + {run.rootTaskRunId === null ? <Badge variant="extra-small" aria-label="Root task">Root</Badge> : null}This change will help screen readers provide more context about the badge's purpose.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx (1)
Line range hint
1-1148: Consider refactoring for improved maintainabilityWhile the changes made are improvements, it's worth noting that this file is quite large and complex. Consider breaking it down into smaller, more manageable components or modules. This could improve readability, maintainability, and make future changes easier to implement and review.
Some suggestions:
- Extract the
TasksTreeViewandTimelineViewinto separate files.- Consider creating a separate file for utility functions and hooks used in this component.
- If possible, split the main
Pagecomponent into smaller, more focused components.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (1)
258-258: Simplify nullish coalescing operatorThe
searchParams.get("span")method returnsstring | null, so using?? undefinedis redundant. Consider simplifying the assignment.Apply this diff to simplify the code:
- const selectedSpanId = searchParams.get("span") ?? undefined; + const selectedSpanId = searchParams.get("span");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- .changeset/three-olives-love.md (1 hunks)
- apps/webapp/app/components/primitives/Badge.tsx (1 hunks)
- apps/webapp/app/components/runs/v3/ReplayRunDialog.tsx (6 hunks)
- apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (3 hunks)
- apps/webapp/app/hooks/useReplaceLocation.ts (0 hunks)
- apps/webapp/app/hooks/useReplaceSearchParams.ts (1 hunks)
- apps/webapp/app/presenters/v3/RunListPresenter.server.ts (3 hunks)
- apps/webapp/app/presenters/v3/RunPresenter.server.ts (3 hunks)
- apps/webapp/app/presenters/v3/SpanPresenter.server.ts (3 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (12 hunks)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx (3 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (6 hunks)
- packages/database/prisma/migrations/20240926110455_add_parent_span_id_index_to_the_task_run_table/migration.sql (1 hunks)
- packages/database/prisma/schema.prisma (1 hunks)
- packages/trigger-sdk/src/v3/shared.ts (0 hunks)
- references/v3-catalog/src/trigger/email.tsx (0 hunks)
- references/v3-catalog/src/trigger/taskHierarchy.ts (1 hunks)
💤 Files not reviewed due to no reviewable changes (3)
- apps/webapp/app/hooks/useReplaceLocation.ts
- packages/trigger-sdk/src/v3/shared.ts
- references/v3-catalog/src/trigger/email.tsx
🔇 Additional comments (23)
apps/webapp/app/hooks/useReplaceSearchParams.ts (1)
1-2: LGTM: Imports are correctly defined and used.The necessary hooks and functions are imported from the appropriate libraries.
references/v3-catalog/src/trigger/taskHierarchy.ts (1)
20-23: Verify the intent of doubling the payload inbatchTriggercall.The change in the
batchTriggercall now sends two identical payload objects instead of one. This modification raises several concerns:
- Inconsistency: This change is not reflected in other similar
batchTriggercalls within the file (e.g., inchildTaskandgrandChildTask).- Functionality: This alteration could potentially double the number of child tasks triggered.
- Performance: If unintended, this change might lead to unnecessary resource usage.
Could you please clarify if this change is intentional? If not, consider reverting to the previous implementation:
- await childTask.batchTrigger([ - { payload: { useWaits, useBatch } }, - { payload: { useWaits, useBatch } }, - ]); + await childTask.batchTrigger([{ payload: { useWaits, useBatch } }]);If the change is intentional, please provide context on why it's only applied to this specific
batchTriggercall and not to others in the file. Additionally, consider updating the otherbatchTriggercalls for consistency if this new behavior is desired throughout the task hierarchy.apps/webapp/app/presenters/v3/RunPresenter.server.ts (3)
40-46: LGTM: Addition of rootTaskRun aligns with PR objectivesThe inclusion of the
rootTaskRunproperty with selected fields (friendlyId,taskIdentifier,spanId) enhances the data structure to support improved navigation within the task hierarchy. This change directly contributes to the PR's goal of adding links to parent and root tasks in the run inspector.
90-90: LGTM: Consistent inclusion of rootTaskRun in the outputThe addition of
rootTaskRunto the returned run object ensures that the newly queried data is properly included in the presenter's output. This change is consistent with the earlier modification and supports the PR's objective of enhancing task hierarchy navigation.
Line range hint
1-172: Summary: Changes successfully implement rootTaskRun supportThe modifications to
RunPresenter.server.tsconsistently implement support forrootTaskRundata, aligning well with the PR objectives to enhance navigation within the task hierarchy. The changes are well-integrated and don't introduce any apparent issues.A minor suggestion for improvement would be to consider refactoring the method to reduce duplication in the run object construction, which could enhance maintainability.
Overall, the implementation effectively supports the new feature of linking to parent and root tasks in the run inspector.
apps/webapp/app/components/runs/v3/ReplayRunDialog.tsx (6)
1-1: LGTM: New imports added for UI enhancements.The new imports for
DialogCloseandParagraphare correctly added and align with the UI changes in the component.Also applies to: 12-12
24-24: LGTM: DialogContent max-width adjusted for better UI.The max-width change from
md:max-w-3xltomd:max-w-xlmakes the dialog more compact, which aligns with the PR objectives of improving the Replay modal panel.
100-103: LGTM: Informative description added to Replay modal.The new Paragraph component adds valuable context about the replay functionality, enhancing user understanding. This aligns well with the PR objective of improving the Replay modal panel with a description.
105-105: LGTM: Improved JSON editor container layout.Adding a minimum height (
min-h-40) to the JSON editor container ensures a reasonable viewing area, enhancing the user experience when editing payloads.
132-133: LGTM: Select component styling refined.The Select component's variant change to
tertiary/smalland padding adjustment topl-1create a more compact and refined look, consistent with the UI improvements outlined in the PR objectives.
Line range hint
1-166: Summary: Excellent improvements to the ReplayRunDialog component.The changes in this file significantly enhance the ReplayRunDialog component, aligning perfectly with the PR objectives:
- The dialog's layout and styling have been refined for a more compact and modern look.
- An informative description has been added to provide context about the replay functionality.
- The JSON editor's container has been improved for better usability.
- Button layout and styling have been updated for improved user interaction.
These changes collectively contribute to a more intuitive and user-friendly interface for replaying runs. Great work on improving the overall user experience!
apps/webapp/app/presenters/v3/RunListPresenter.server.ts (3)
171-171: LGTM: Addition ofrootTaskRunIdto the return type.The addition of
rootTaskRunId: string | nullto the return type is consistent with the PR objectives and correctly allows for null values. This change will enable better navigation within the task hierarchy.
196-196: LGTM: Addition ofrootTaskRunIdto the SQL query.The SQL query has been correctly updated to select the
rootTaskRunIdfrom theTaskRuntable. The alias used matches the property name in the return type, ensuring consistency in the data structure.
341-341: LGTM: Addition ofrootTaskRunIdto the returned object.The
rootTaskRunIdis correctly added to the returned object in the mapping function. The property name is consistent with the return type and SQL query, and the value is appropriately passed through without transformation.apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (2)
44-44: LGTM: New import for Badge componentThe addition of the
Badgeimport is appropriate for the new functionality being introduced. It follows good practices by using a relative import path for internal components.
127-127: LGTM: Improved table header labelChanging the header from "Task ID" to "Task" is a good improvement. It's more concise and better represents the column's content, which now includes both the task identifier and potentially a "Root" badge.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.electric.$runParam/route.tsx (3)
67-67: LGTM: Improved URL parameter handlingThe switch from
useReplaceLocationtouseReplaceSearchParamsis a good change. It provides more specific and efficient handling of search parameters in the URL.
1148-1148: LGTM: Improved spacing for Callout componentThe addition of the "mt-2" class to the Callout component is a good styling improvement. It adds a top margin, which will help visually separate the Callout from the content above it, enhancing the overall layout.
263-264: LGTM: Simplified search parameter handlingThe use of
useReplaceSearchParamsand direct access tosearchParamssimplifies the code and makes it more maintainable. This is a good improvement.Please verify that this change doesn't affect any other parts of the application that might have depended on the previous
getSpanIdfunction. Run the following script to check for any remaining usage ofgetSpanId:✅ Verification successful
Verified: No residual usage of
getSpanIddetectedAll usages of
getSpanIdhave been successfully removed from the codebase, ensuring that the refactoring does not impact other parts of the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of getSpanId function rg "getSpanId\(" --type typescript --type tsxLength of output: 82
Script:
#!/bin/bash # Search for any remaining usage of getSpanId function in .ts and .tsx files rg "getSpanId\(" -g "*.ts" -g "*.tsx"Length of output: 39
packages/database/prisma/schema.prisma (1)
1785-1785: Approved: Index addition will improve query performanceThe addition of an index on the
parentSpanIdfield in theTaskRunmodel is a good optimization. This will likely improve the performance of queries that filter, sort, or join on theparentSpanId, which can be particularly beneficial when dealing with hierarchical task structures or analyzing task relationships.Consider the following implications:
- Positive: Faster read operations involving the
parentSpanId.- Negative: Slightly increased storage requirements and potentially slower write operations due to index maintenance.
Given that this is the only change in a large schema file, it appears to be a targeted performance optimization. Ensure that this change aligns with your application's query patterns and performance requirements.
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (2)
135-149: LGTM: RelationshipsrootTaskRunandparentTaskRunadded to the query selection.The inclusion of
rootTaskRunandparentTaskRunin theselectclause is appropriate for retrieving necessary related data.
299-307: LGTM: Relationships object correctly constructed withrootandparent.The
relationshipsobject is properly assembled, handling optionalrootTaskRunandparentTaskRun, and accurately determining if the root is also the parent via theisParentflag.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam/route.tsx (1)
965-973: Ensure consistent type definitions inShowParentLinkThe
ShowParentLinkfunction signature correctly includesrunFriendlyId,spanId, andisRoot. Ensure thatspanIdbeing optional is handled appropriately throughout the component to prevent potential undefined errors.
| export function useReplaceSearchParams() { | ||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
|
|
||
| const replaceSearchParam = useCallback( | ||
| (key: string, value?: string) => { | ||
| setSearchParams((s) => { | ||
| if (value) { | ||
| s.set(key, value); | ||
| } else { | ||
| s.delete(key); | ||
| } | ||
| return s; | ||
| }); | ||
| }, | ||
| [searchParams] | ||
| ); | ||
|
|
||
| return { searchParams, setSearchParams, replaceSearchParam }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Well-implemented custom hook with a minor optimization suggestion.
The useReplaceSearchParams hook is well-implemented, providing a clean API for managing search parameters. The use of useCallback for memoization is a good practice. However, there's a minor optimization that can be made:
Consider removing searchParams from the useCallback dependency array:
const replaceSearchParam = useCallback(
(key: string, value?: string) => {
setSearchParams((s) => {
if (value) {
s.set(key, value);
} else {
s.delete(key);
}
return s;
});
},
- [searchParams]
+ [setSearchParams]
);This change is safe because setSearchParams always provides the most up-to-date searchParams object, and removing searchParams from the dependency array can prevent unnecessary re-renders.
📝 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.
| export function useReplaceSearchParams() { | |
| const [searchParams, setSearchParams] = useSearchParams(); | |
| const replaceSearchParam = useCallback( | |
| (key: string, value?: string) => { | |
| setSearchParams((s) => { | |
| if (value) { | |
| s.set(key, value); | |
| } else { | |
| s.delete(key); | |
| } | |
| return s; | |
| }); | |
| }, | |
| [searchParams] | |
| ); | |
| return { searchParams, setSearchParams, replaceSearchParam }; | |
| } | |
| export function useReplaceSearchParams() { | |
| const [searchParams, setSearchParams] = useSearchParams(); | |
| const replaceSearchParam = useCallback( | |
| (key: string, value?: string) => { | |
| setSearchParams((s) => { | |
| if (value) { | |
| s.set(key, value); | |
| } else { | |
| s.delete(key); | |
| } | |
| return s; | |
| }); | |
| }, | |
| [setSearchParams] | |
| ); | |
| return { searchParams, setSearchParams, replaceSearchParam }; | |
| } |
| const triggeredRuns = await this._replica.taskRun.findMany({ | ||
| select: { | ||
| friendlyId: true, | ||
| taskIdentifier: true, | ||
| spanId: true, | ||
| createdAt: true, | ||
| number: true, | ||
| lockedToVersion: { | ||
| select: { | ||
| version: true, | ||
| }, | ||
| }, | ||
| }, | ||
| where: { | ||
| parentSpanId: spanId, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Potential performance issue: Consider implementing pagination or limiting the results for triggeredRuns.
Fetching all triggeredRuns without a limit might lead to performance issues if there are many triggered runs associated with a span. To prevent potential delays or excessive memory usage, it's advisable to implement pagination or set a reasonable limit on the number of results returned.
Apply this diff to add a limit to the query:
const triggeredRuns = await this._replica.taskRun.findMany({
select: {
friendlyId: true,
taskIdentifier: true,
spanId: true,
createdAt: true,
number: true,
lockedToVersion: {
select: {
version: true,
},
},
},
where: {
parentSpanId: spanId,
},
+ take: 100, // Limit the results to 100
});Alternatively, consider adding pagination parameters to the query to allow clients to request specific ranges of results.
📝 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.
| const triggeredRuns = await this._replica.taskRun.findMany({ | |
| select: { | |
| friendlyId: true, | |
| taskIdentifier: true, | |
| spanId: true, | |
| createdAt: true, | |
| number: true, | |
| lockedToVersion: { | |
| select: { | |
| version: true, | |
| }, | |
| }, | |
| }, | |
| where: { | |
| parentSpanId: spanId, | |
| }, | |
| }); | |
| const triggeredRuns = await this._replica.taskRun.findMany({ | |
| select: { | |
| friendlyId: true, | |
| taskIdentifier: true, | |
| spanId: true, | |
| createdAt: true, | |
| number: true, | |
| lockedToVersion: { | |
| select: { | |
| version: true, | |
| }, | |
| }, | |
| }, | |
| where: { | |
| parentSpanId: spanId, | |
| }, | |
| take: 100, // Limit the results to 100 | |
| }); |
| <Table className="w-full"> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHeaderCell>Run #</TableHeaderCell> | ||
| <TableHeaderCell>Task</TableHeaderCell> | ||
| <TableHeaderCell>Version</TableHeaderCell> | ||
| <TableHeaderCell>Created at</TableHeaderCell> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {span.triggeredRuns.map((run) => { | ||
| const path = v3RunSpanPath( | ||
| organization, | ||
| project, | ||
| { friendlyId: run.friendlyId }, | ||
| { spanId: run.spanId } | ||
| ); | ||
| return ( | ||
| <TableRow key={run.friendlyId}> | ||
| <TableCell to={path} actionClassName="py-1.5"> | ||
| {run.number} | ||
| </TableCell> | ||
| <TableCell to={path} actionClassName="py-1.5"> | ||
| {run.taskIdentifier} | ||
| </TableCell> | ||
| <TableCell to={path} actionClassName="py-1.5"> | ||
| {run.lockedToVersion?.version ?? "–"} | ||
| </TableCell> | ||
| <TableCell to={path} actionClassName="py-1.5"> | ||
| <DateTime date={run.createdAt} /> | ||
| </TableCell> | ||
| </TableRow> | ||
| ); | ||
| })} | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using TaskRunsTable component for consistency and code reuse
You've manually created a table to display the triggered runs. Since there is an existing TaskRunsTable component, consider using it to maintain consistency across the application and reduce code duplication.
| <Property.Item> | ||
| <Property.Label>Root & Parent</Property.Label> | ||
| <Property.Value> | ||
| <TextLink | ||
| to={v3RunSpanPath( | ||
| organization, | ||
| project, | ||
| { | ||
| friendlyId: run.relationships.root.friendlyId, | ||
| }, | ||
| { spanId: run.relationships.root.spanId } | ||
| )} | ||
| className="group flex flex-wrap items-center gap-x-1 gap-y-0" | ||
| > | ||
| {run.relationships.root.taskIdentifier} | ||
| <span className="break-all text-text-dimmed transition-colors group-hover:text-text-bright/80"> | ||
| ({run.relationships.root.friendlyId}) | ||
| </span> | ||
| </TextLink> | ||
| </Property.Value> | ||
| </Property.Item> | ||
| ) : ( | ||
| <> | ||
| <Property.Item> | ||
| <Property.Label>Root</Property.Label> | ||
| <Property.Value> | ||
| <TextLink | ||
| to={v3RunSpanPath( | ||
| organization, | ||
| project, | ||
| { | ||
| friendlyId: run.relationships.root.friendlyId, | ||
| }, | ||
| { spanId: run.relationships.root.spanId } | ||
| )} | ||
| className="group flex flex-wrap items-center gap-x-1 gap-y-0" | ||
| > | ||
| {run.relationships.root.taskIdentifier} | ||
| <span className="break-all text-text-dimmed transition-colors group-hover:text-text-bright/80"> | ||
| ({run.relationships.root.friendlyId}) | ||
| </span> | ||
| </TextLink> | ||
| </Property.Value> | ||
| </Property.Item> | ||
| {run.relationships.parent ? ( | ||
| <Property.Item> | ||
| <Property.Label>Parent</Property.Label> | ||
| <Property.Value> | ||
| <TextLink | ||
| to={v3RunSpanPath( | ||
| organization, | ||
| project, | ||
| { | ||
| friendlyId: run.relationships.parent.friendlyId, | ||
| }, | ||
| { spanId: run.relationships.parent.spanId } | ||
| )} | ||
| className="group flex flex-wrap items-center gap-x-1 gap-y-0" | ||
| > | ||
| {run.relationships.parent.taskIdentifier} | ||
| <span className="break-all text-text-dimmed transition-colors group-hover:text-text-bright/80"> | ||
| ({run.relationships.parent.friendlyId}) | ||
| </span> | ||
| </TextLink> | ||
| </Property.Value> | ||
| </Property.Item> | ||
| ) : null} | ||
| </> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code for rendering root and parent links
The code for rendering the root and parent links is duplicated. To enhance maintainability and reduce redundancy, consider extracting the repeated code into a reusable component or function.
Here's an example of how you might refactor the code:
function RelationshipLink({ label, relationship }) {
return (
<Property.Item>
<Property.Label>{label}</Property.Label>
<Property.Value>
<TextLink
to={v3RunSpanPath(
organization,
project,
{ friendlyId: relationship.friendlyId },
{ spanId: relationship.spanId }
)}
className="group flex flex-wrap items-center gap-x-1 gap-y-0"
>
{relationship.taskIdentifier}
<span className="break-all text-text-dimmed transition-colors group-hover:text-text-bright/80">
({relationship.friendlyId})
</span>
</TextLink>
</Property.Value>
</Property.Item>
);
}Then you can use it like this:
{run.relationships.root && (
run.relationships.root.isParent ? (
<RelationshipLink label="Root & Parent" relationship={run.relationships.root} />
) : (
<>
<RelationshipLink label="Root" relationship={run.relationships.root} />
{run.relationships.parent && (
<RelationshipLink label="Parent" relationship={run.relationships.parent} />
)}
</>
)
)}| {rootRun ? ( | ||
| <ShowParentLink | ||
| runFriendlyId={rootRun.friendlyId} | ||
| isRoot={true} | ||
| spanId={rootRun.spanId} | ||
| /> | ||
| ) : parentRunFriendlyId ? ( | ||
| <ShowParentLink runFriendlyId={parentRunFriendlyId} isRoot={false} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested ternary operator for better readability
The nested ternary operator in this JSX block reduces readability and can be difficult to maintain. Consider refactoring it into a more readable format using an if-else statement or a self-invoking function.
Apply this diff to refactor the conditional rendering:
- {rootRun ? (
- <ShowParentLink
- runFriendlyId={rootRun.friendlyId}
- isRoot={true}
- spanId={rootRun.spanId}
- />
- ) : parentRunFriendlyId ? (
- <ShowParentLink runFriendlyId={parentRunFriendlyId} isRoot={false} />
- ) : (
- <Paragraph variant="small" className="flex-1 text-charcoal-500">
- This is the root task
- </Paragraph>
- )}
+ {(() => {
+ if (rootRun) {
+ return (
+ <ShowParentLink
+ runFriendlyId={rootRun.friendlyId}
+ isRoot={true}
+ spanId={rootRun.spanId}
+ />
+ );
+ } else if (parentRunFriendlyId) {
+ return (
+ <ShowParentLink runFriendlyId={parentRunFriendlyId} isRoot={false} />
+ );
+ } else {
+ return (
+ <Paragraph variant="small" className="flex-1 text-charcoal-500">
+ This is the root task
+ </Paragraph>
+ );
+ }
+ })()}📝 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.
| {rootRun ? ( | |
| <ShowParentLink | |
| runFriendlyId={rootRun.friendlyId} | |
| isRoot={true} | |
| spanId={rootRun.spanId} | |
| /> | |
| ) : parentRunFriendlyId ? ( | |
| <ShowParentLink runFriendlyId={parentRunFriendlyId} isRoot={false} /> | |
| {(() => { | |
| if (rootRun) { | |
| return ( | |
| <ShowParentLink | |
| runFriendlyId={rootRun.friendlyId} | |
| isRoot={true} | |
| spanId={rootRun.spanId} | |
| /> | |
| ); | |
| } else if (parentRunFriendlyId) { | |
| return ( | |
| <ShowParentLink runFriendlyId={parentRunFriendlyId} isRoot={false} /> | |
| ); | |
| } else { | |
| return ( | |
| <Paragraph variant="small" className="flex-1 text-charcoal-500"> | |
| This is the root task | |
| </Paragraph> | |
| ); | |
| } | |
| })()} |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/webapp/app/routes/storybook.table/route.tsx (3)
20-37: LGTM: Static table implementation is correct and efficient.The static table is well-structured and uses the Table components correctly. The use of
Array.from()to generate rows is an efficient approach.Consider adding an
aria-labelto the<Table>component for improved accessibility. For example:- <Table> + <Table aria-label="Static table example">
44-61: LGTM: Sticky header table implementation is correct and consistent.The sticky header table is well-implemented, using the
containerClassNameprop to enable scrolling when the content exceeds the max-height. The structure is consistent with the static table, which is good for maintainability.For consistency with the previous suggestion, consider adding an
aria-labelto this<Table>component as well:- <Table containerClassName="max-h-[11.5rem]"> + <Table containerClassName="max-h-[11.5rem]" aria-label="Sticky header table example">
13-65: LGTM: Overall component structure is well-organized.The
Storycomponent has a clean layout structure using flexbox, with appropriate spacing and organization. The use of semantic headers and explanatory paragraphs provides good context for each table example.The component name "Story" might be a bit ambiguous without context. Consider renaming it to something more descriptive of its purpose, such as
TableExamplesorTableStorybook. Alternatively, add a comment explaining the purpose of this component in the context of Storybook:+// This component showcases different table variants for Storybook documentation export default function Story() { // ... }apps/webapp/app/routes/storybook/route.tsx (1)
91-94: LGTM! Consider using a plural form for the slug.The addition of the "Tables" story entry is consistent with the existing structure and maintains the alphabetical order within the
storiesarray.For consistency with other entries, consider using the plural form "tables" for the slug:
{ name: "Tables", - slug: "table", + slug: "tables", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/webapp/app/components/primitives/Table.tsx (5 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (6 hunks)
- apps/webapp/app/routes/storybook.table/route.tsx (1 hunks)
- apps/webapp/app/routes/storybook/route.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx
🔇 Additional comments (6)
apps/webapp/app/routes/storybook.table/route.tsx (1)
1-11: LGTM: Imports are appropriate and follow best practices.The imports are well-organized and include all necessary components for the
Storycomponent. The naming conventions follow React best practices, and there are no unused imports.apps/webapp/app/components/primitives/Table.tsx (5)
25-25: LGTM. Verify visual impact of removing row dividers.The removal of the "divide-y" class from the table element looks good. This change will eliminate the vertical dividers between table rows, potentially creating a cleaner look.
Please verify that this change aligns with the intended design and doesn't negatively impact the table's visual hierarchy.
43-46: Great improvements to the TableHeader component!The changes to the TableHeader component significantly enhance both its functionality and visual appeal:
- Sticky positioning improves usability for long tables.
- Added dividers and background color improve visual separation.
- Rounded top corners add a polished look.
- The pseudo-element border provides a subtle separation between header and body.
These updates will contribute to a better user experience and a more polished UI.
62-65: Good addition for handling large datasets. Verify scrolling behavior.Adding the "overflow-y-auto" class to the tbody element is a good approach for handling tables with many rows. This allows vertical scrolling within the table body when the content exceeds the available space.
Please ensure that:
- A parent container with a fixed height is set to enable the scrolling behavior.
- The scrolling works as expected in different browsers and screen sizes.
- The user experience is smooth when interacting with large datasets.
147-147: LGTM. Verify visual consistency without gradient background.The removal of the gradient background from the stickyStyles constant simplifies the styling of sticky elements. This change aligns with a cleaner, more modern design approach.
Please ensure that:
- The removal of the gradient background doesn't negatively impact the visual hierarchy or readability of the table.
- This change is consistent with other UI elements in the application.
- The sticky elements are still visually distinct from non-sticky elements.
189-189: Improved hover effect for better visual feedback.Changing the hover effect class from "bg-charcoal-900" to "bg-charcoal-800" is a good improvement. This update:
- Slightly lightens the background color of table cells on hover.
- Likely provides better contrast and visibility.
- Enhances the visual feedback for interactive table cells.
These small details contribute to a more polished and responsive user interface.
|
Does the batchTrigger table link to the triggered runs? |
Yep CleanShot.2024-09-27.at.08.59.25.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/webapp/app/components/primitives/Table.tsx (1)
144-150: Improved customization for table row hover effects.The introduction of the
rowHoverStyleprop androwHoverStylesconstant enhances the flexibility of defining hover effects for table rows. This allows for more nuanced control over the table's interactive appearance, with options for default, dimmed, and bright hover styles.The implementation correctly integrates these new features into the TableCell component.
Consider adding documentation or comments explaining the usage and options for the new
rowHoverStyleprop to improve developer experience when using this component.Additionally, note that the
stickyStylesconstant has been simplified. Ensure this doesn't negatively impact the appearance of sticky cells in various scenarios.Also applies to: 154-154, 168-168, 199-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/webapp/app/components/primitives/Table.tsx (6 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx
🔇 Additional comments (3)
apps/webapp/app/components/primitives/Table.tsx (3)
25-25: Verify the removal of row dividers from the table.The "divide-y" class has been removed from the table element. This change will remove the visual separation between table rows.
Please confirm if this is intentional and aligns with the updated design. If not, consider re-adding the class or using an alternative method for row separation.
43-46: Improved table header styling and functionality.The changes to the TableHeader component enhance both its appearance and functionality:
- Sticky positioning keeps the header visible during scrolling.
- Background color distinguishes the header from the body.
- A subtle separator line is added at the bottom of the header.
These improvements contribute to better visual hierarchy and user experience.
62-65: Enhanced table body scrolling capability.The addition of the "overflow-y-auto" class to the TableBody component allows for vertical scrolling within the table body when content exceeds the container's height. This improvement helps in handling large datasets more efficiently.
Please verify that this new scrolling behavior doesn't conflict with any existing scrolling at the table or page level, ensuring a smooth user experience across different screen sizes and content volumes.
The run inspector now has links to the parent and root tasks
Batchtrigger() spans now display the triggered runs in a table in the inspector
A more subtle "Root" badge
Added a description to the Replay modal panel
Summary by CodeRabbit
Release Notes
New Features
useReplaceSearchParams, for managing URL search parameters.Storycomponent for rendering static and sticky header tables.Improvements
ReplayRunDialogandTaskRunsTable.SpanPresenterandRunPresenter, enhancing the information available.Bug Fixes
Chores