fix: Init runbooks ui #241
Conversation
WalkthroughThis pull request introduces several enhancements and new components related to job conditions and runbooks in the web service application. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts (1)🔇 Additional comments (2)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts (2)
The implementation follows the recommended approach of using
The function should handle edge cases and add proper type constraints. -export const nFormatter = (num: number, digits: number) =>
+export const nFormatter = (num: number, digits: number): string => {
+ if (!Number.isFinite(num)) return 'N/A';
+ if (!Number.isInteger(digits) || digits < 0) {
+ throw new Error('digits must be a non-negative integer');
+ }
+ try {
+ return new Intl.NumberFormat("en", {
+ notation: "compact",
+ maximumFractionDigits: digits,
+ }).format(num);
+ } catch (error) {
+ console.error('Error formatting number:', error);
+ return 'Error';
+ }
+};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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (22)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts (2)
11-15: Optimize performance and improve readability.The current implementation has several performance and readability issues:
reverse()creates a new array on every call- The regular expression could be pre-compiled
- The chained operations could be more readable
- const regexp = /\.0+$|(?<=\.[0-9]*[1-9])0+$/; - const item = lookup.reverse().find((item) => num >= item.value); - return item - ? (num / item.value).toFixed(digits).replace(regexp, "").concat(item.symbol) - : "0"; +const TRAILING_ZEROS_REGEX = /\.0+$|(?<=\.[0-9]*[1-9])0+$/; + +export const nFormatter = (num: number, digits: number): string => { + // ... validation code from previous comment ... + + // Find the largest value that num is greater than or equal to + const item = NUMBER_FORMAT_LOOKUP + .findLast((item) => num >= item.value); + + if (!item) return "0"; + + const scaledNum = num / item.value; + const formattedNum = scaledNum.toFixed(digits) + .replace(TRAILING_ZEROS_REGEX, ''); + + return `${formattedNum}${item.symbol}`;
1-16: Add JSDoc documentation and examples.The function would benefit from comprehensive documentation explaining its purpose, parameters, and usage examples.
+/** + * Formats a number using metric prefixes (k, M, G, etc.) + * @param num - The number to format + * @param digits - Number of decimal places to show + * @returns Formatted string with metric prefix + * @example + * nFormatter(1234, 1) // "1.2k" + * nFormatter(1234567, 2) // "1.23M" + * nFormatter(-1234, 1) // "-1.2k" + * @throws {Error} If digits is not a non-negative integer + */ export const nFormatter = ...apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/layout.tsx (2)
5-7: Consider using path aliases for better maintainabilityThe relative imports using multiple parent directory navigation (
../../../) can be fragile and hard to maintain. Consider using path aliases to make imports more maintainable.Example:
-import { SystemBreadcrumbNavbar } from "../../../SystemsBreadcrumb"; -import { TopNav } from "../../../TopNav"; +import { SystemBreadcrumbNavbar } from "@/components/SystemsBreadcrumb"; +import { TopNav } from "@/components/TopNav";
30-37: Consider optimizing multiple API callsThe component makes three separate API calls that could potentially be parallelized for better performance.
- const workspace = await api.workspace.bySlug(params.workspaceSlug); - if (workspace == null) return notFound(); - - const runbook = await api.runbook.byId(params.runbookId); - if (runbook == null) return notFound(); - - const { total } = await api.runbook.jobs({ runbookId, limit: 0 }); + const [workspace, runbook, jobStats] = await Promise.all([ + api.workspace.bySlug(params.workspaceSlug), + api.runbook.byId(params.runbookId), + api.runbook.jobs({ runbookId: params.runbookId }) + ]); + + if (workspace == null || runbook == null) return notFound(); + const { total } = jobStats;apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionRender.tsx (2)
17-20: Enhance component documentation.While the width requirement is noted, the documentation could be more comprehensive:
- Add a description of the component's purpose
- Document the expected props and their types
- Explain why the 1000px minimum width is required
- Consider adding usage examples
/** + * A component that renders different types of job conditions based on their type. + * Supports comparison, created at, metadata, and status conditions. + * + * @param {JobConditionRenderProps<JobCondition>} props + * @param {JobCondition} props.condition - The condition to render + * @param {Function} props.onChange - Callback when condition changes + * @param {number} props.depth - Nesting depth for layout (default: 0) + * @param {string} props.className - Additional CSS classes + * * The parent container should have min width of 1000px * to render this component properly. + * This is required to ensure proper alignment of nested condition components + * and prevent layout shifts. */
63-63: Consider adding error handling and fallback UI.The component silently returns null for invalid conditions. Consider:
- Adding error boundaries
- Logging invalid conditions for debugging
- Rendering a fallback UI to indicate invalid state
- return null; + console.warn('Invalid condition type:', condition); + return ( + <div className="text-red-500"> + Invalid condition type. Please check console for details. + </div> + );apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionDialog.tsx (5)
32-36: Consider cleanup on dialog close.The local state persists even after the dialog is closed. Consider resetting the local state when the dialog closes to ensure a fresh start on next open.
const [open, setOpen] = useState(false); const [error, setError] = useState<string | null>(null); const [localCondition, setLocalCondition] = useState( condition ?? defaultCondition, ); + + useEffect(() => { + if (!open) { + setLocalCondition(condition ?? defaultCondition); + setError(null); + } + }, [open, condition]);
41-44: Optimize event handler with useCallback.The onClick handler should be memoized to prevent unnecessary re-renders.
+ const handleClick = useCallback((e: React.MouseEvent) => { + e.stopPropagation(); + }, []); + <DialogContent className="min-w-[1000px]" - onClick={(e) => e.stopPropagation()} + onClick={handleClick} >
68-83: Extract save logic into a separate handler.The save handler is quite complex and would benefit from being extracted into a separate function for better readability and maintainability.
+ const handleSave = useCallback(() => { + if (!isValidJobCondition(localCondition)) { + setError( + "Invalid job condition, ensure all fields are filled out correctly.", + ); + return; + } + setOpen(false); + setError(null); + if (isEmptyCondition(localCondition)) { + onChange(null); + return; + } + onChange(localCondition); + }, [localCondition, onChange]); + <Button - onClick={() => { - if (!isValidJobCondition(localCondition)) { - setError( - "Invalid job condition, ensure all fields are filled out correctly.", - ); - return; - } - setOpen(false); - setError(null); - if (isEmptyCondition(localCondition)) { - onChange(null); - return; - } - onChange(localCondition); - }} + onClick={handleSave} >
38-89: Enhance accessibility attributes.The dialog implementation could benefit from additional accessibility attributes.
<Dialog open={open} onOpenChange={setOpen} + aria-describedby="job-condition-dialog-description" > <DialogTrigger asChild>{children}</DialogTrigger> <DialogContent className="min-w-[1000px]" onClick={(e) => e.stopPropagation()} + role="dialog" + aria-modal="true" + aria-labelledby="job-condition-dialog-title" > <DialogHeader> - <DialogTitle>Edit Job Condition</DialogTitle> + <DialogTitle id="job-condition-dialog-title">Edit Job Condition</DialogTitle> <DialogDescription + id="job-condition-dialog-description" >
69-74: Enhance error validation feedback.The current error message is generic. Consider providing more specific feedback about what exactly is invalid in the condition.
- if (!isValidJobCondition(localCondition)) { - setError( - "Invalid job condition, ensure all fields are filled out correctly.", - ); - return; - } + const validationError = validateJobCondition(localCondition); + if (validationError) { + setError(validationError); + return; + }Note: This assumes the existence of a
validateJobConditionfunction that returns specific error messages. If it doesn't exist, consider implementing it.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/RunbookNavBar.tsx (1)
30-36: Consider using URL utilities for more robust URL handling.While the current URL construction works, consider using
URLPatternor a URL builder utility to handle special characters and ensure URL validity.Example implementation:
const buildUrl = (base: string, params: Record<string, string>, path?: string) => { const url = new URL(base, window.location.origin); Object.entries(params).forEach(([key, value]) => { url.pathname += `/${encodeURIComponent(value)}`; }); if (path) url.pathname += `/${path}`; return url.pathname; }; const baseUrl = buildUrl("/", { workspaceSlug, systemSlug, runbookId, }); const settingsUrl = buildUrl("/", { workspaceSlug, systemSlug, runbookId, }, "settings");apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsBreadcrumb.tsx (1)
42-42: Consider adding error handling for the runbook fetchWhile the conditional fetching is good, consider adding error handling to gracefully handle cases where the runbook fetch fails.
- const runbook = runbookId ? await api.runbook.byId(runbookId) : null; + let runbook = null; + if (runbookId) { + try { + runbook = await api.runbook.byId(runbookId); + } catch (error) { + console.error('Failed to fetch runbook:', error); + // Optionally handle the error in the UI + } + }apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
Line range hint
108-119: Fix incorrect URL for Jobs navigationThe Jobs navigation menu item is currently linking to the variables URL (
variablesUrl), which is incorrect and could confuse users.Apply this fix:
- <Link href={variablesUrl} legacyBehavior passHref> + <Link href={`/${workspaceSlug}/systems/${systemSlug}/deployments/${deploymentSlug}/jobs`} legacyBehavior passHref>
Line range hint
47-53: Consider more explicit Settings active state detectionThe current implementation determines Settings as active by the absence of other active states. This could lead to unexpected behavior if new sections are added.
Consider using an explicit path check:
- const isSettingsActive = - !isReleasesActive && - !isVariablesActive && - !isJobsActive && - !isReleaseChannelsActive && - !isHooksActive; + const isSettingsActive = pathname === overviewUrl;apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/page.tsx (1)
121-121: Simplify opacity calculationThe multiplication by
1in1 * (1 - i / 10)is unnecessary. You can simplify it to just(1 - i / 10).Apply this diff to simplify the calculation:
- style={{ opacity: 1 * (1 - i / 10) }} + style={{ opacity: 1 - i / 10 }}packages/api/src/router/runbook.ts (2)
14-25: Handle cases where the runbook is not foundCurrently, if no runbook is found with the given ID, the procedure may return
nullorundefined. Consider returning a more informative error message or throwing an exception to handle this case appropriately.
70-74: Ensure consistent optionality ofvariablesincreateandupdateproceduresIn the
createprocedure,variablesis required, whereas in theupdateprocedure,variablesis optional. For consistency and better usability, consider makingvariablesoptional in both procedures unless it's essential forcreate.Also applies to: 107-107
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx (4)
189-189: Remove unnecessarykeyprop from child componentThe
keyprop is used by React for list rendering and is not passed to the child component via props. Passingkey={index}toJobConditionRenderis unnecessary unless the component explicitly requires akeyprop.Remove the
keyprop to clean up the code:- <JobConditionRender - key={index} condition={subCond} onChange={(c) => updateCondition(index, c)} depth={depth + 1} className={cn(depth === 0 ? "col-span-11" : "col-span-10")} />
310-337: Define a constant for maximum depth to avoid magic numbersUsing the hardcoded value
2for maximum depth in multiple places can make the code less maintainable. Defining a constant improves readability and makes future adjustments easier.At the top of the file, define the constant:
const MAX_DEPTH = 2;Update the conditions to use
MAX_DEPTH:- {depth < 2 && ( + {depth < MAX_DEPTH && (Apply this change wherever
depth < 2appears.
82-83: Optimize repeated function calls for performanceThe function
doesConvertingToComparisonRespectMaxDepthis called multiple times with the same arguments, which can be inefficient.Cache the result within the scope to avoid redundant computations:
// Inside the map function const canConvertToComparison = doesConvertingToComparisonRespectMaxDepth( depth + 1, subCond, ); // Use `canConvertToComparison` instead of calling the function againThis approach improves performance, especially if the component re-renders frequently.
Also applies to: 225-228, 237-241
225-249: Provide user feedback when actions are unavailableIf an action cannot be performed due to depth constraints, the user may not understand why the option is missing.
Consider disabling the menu item with a tooltip explaining why it's unavailable:
<DropdownMenuItem onClick={() => canConvertToComparison && convertToComparison(index)} + disabled={!canConvertToComparison} className="flex items-center gap-2" > <IconRefresh className="h-4 w-4" /> Turn into group </DropdownMenuItem>Add a tooltip or explanatory text to inform the user:
{!canConvertToComparison && ( <span className="text-xs text-muted-foreground"> Maximum nesting depth reached. </span> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobComparisonConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsBreadcrumb.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/RunbookNavBar.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/layout.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/page.tsx(1 hunks)packages/api/src/router/runbook.ts(4 hunks)
🔇 Additional comments (17)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts (1)
1-16: Verify usage in badge components.
The function is used in multiple badge components. Let's verify the implementation matches the requirements.
✅ Verification successful
The nFormatter implementation is correctly used in badge components.
The verification shows that:
- The nFormatter is used consistently in navigation bars for formatting numbers in badges (RunbookNavBar.tsx and DeploymentNavBar.tsx)
- The implementation matches the requirements with proper digit formatting and symbol appending
- No inconsistent number formatting patterns are used in badge components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of nFormatter to ensure consistent formatting
# Search for imports and usages
echo "Searching for nFormatter imports and usages..."
rg -A 2 "import.*nFormatter|nFormatter\(" --type ts --type tsx
# Search for other number formatting approaches that should be consolidated
echo "Searching for other number formatting patterns..."
rg "toLocaleString|Intl\.NumberFormat|\.toFixed|\.toString" --type ts --type tsx
Length of output: 415
Script:
#!/bin/bash
# Search for nFormatter imports and usages without file type restrictions
echo "Searching for nFormatter imports and usages..."
rg -A 2 "import.*nFormatter|nFormatter\("
# Search for other number formatting patterns
echo "Searching for other number formatting patterns..."
rg "toLocaleString|Intl\.NumberFormat|\.toFixed|\.toString"
# Find all badge components
echo "Searching for badge components..."
fd -e ts -e tsx ".*badge.*"
Length of output: 18522
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/layout.tsx (2)
48-48: Review the fixed height calculation for responsiveness
The scroll container uses a fixed height calculation (h-[calc(100vh-53px-49px)]) which might break in different viewport sizes or when the nav heights change.
Consider using CSS custom properties or a more flexible layout approach.
✅ Verification successful
Fixed height calculations are consistently used across the application
The codebase shows a consistent pattern of using fixed height calculations with viewport units (vh) for scroll containers. This appears to be an intentional design choice across multiple layouts and components:
- Multiple layouts use the same
h-[calc(100vh-53px-49px)]calculation - Similar patterns are used throughout the app with variations like
h-[calc(100vh-40px)],h-[calc(100vh-110px)], etc. - The calculations account for fixed-height elements like headers and navigation bars
Given this is a deliberate architectural decision implemented consistently across the application, the current implementation aligns with the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of fixed height calculations
rg -g '*.{tsx,css}' 'h-\[calc\(.*\)\]'
Length of output: 4931
36-37: 🛠️ Refactor suggestion
Optimize runbookId usage and verify job counting logic
Two issues to address:
- The
runbookIdextraction is redundant as it's already available inparams - Using
limit: 0for counting jobs seems unusual and might not work as intended
- const { runbookId } = params;
- const { total } = await api.runbook.jobs({ runbookId, limit: 0 });
+ const { total } = await api.runbook.jobs({ runbookId: params.runbookId });apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionRender.tsx (3)
1-15: LGTM! Well-organized imports with clear separation.
The imports are logically grouped between external dependencies and internal components/types.
21-23: LGTM! Well-typed component definition.
The component is properly typed with React.FC and includes appropriate props with sensible defaults.
54-61: Ensure consistent prop usage across condition renderers.
The depth prop is passed to all condition renderers except StatusConditionRender. This inconsistency should either be documented or fixed.
<StatusConditionRender
condition={condition}
onChange={onChange}
+ depth={depth}
className={className}
/>apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/RunbookJobConditionDialog.tsx (2)
1-21: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly structured, utilizing specific components and utilities from the @ctrlplane ecosystem.
23-27: LGTM! Well-defined prop types with proper typing.
The type definitions are clear and follow React TypeScript best practices.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/RunbookNavBar.tsx (3)
1-21: LGTM! Clean imports and type definitions.
The imports are well-organized and the props type is properly defined.
24-28: LGTM! Type-safe parameter extraction.
The URL parameter extraction is well-implemented with proper TypeScript types.
23-78: Verify component integration with RunbookLayout.
Let's verify how this component integrates with the parent layout and ensure proper data flow.
✅ Verification successful
Component integration is properly implemented
The RunbookNavBar is correctly integrated within the RunbookLayout component with:
- Proper data fetching for total jobs count via TRPC
- Correct prop passing of
totalJobs - Appropriate positioning in the layout hierarchy
- Proper usage of the shared
nFormatterutility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RunbookNavBar usage and RunbookLayout implementation
rg -t tsx "RunbookNavBar|RunbookLayout" --glob "apps/webservice/src/**"
# Verify the implementation of nFormatter
ast-grep --pattern 'export function nFormatter($_,$_) { $$$ }'
Length of output: 166
Script:
#!/bin/bash
# Search for RunbookNavBar usage and RunbookLayout implementation without file type restriction
rg "RunbookNavBar|RunbookLayout" --type-add 'ts:*.{ts,tsx}' --type ts apps/webservice/src/
# Find files containing nFormatter function
rg "nFormatter" --type-add 'ts:*.{ts,tsx}' --type ts
Length of output: 1758
Script:
#!/bin/bash
# Let's check the layout.tsx implementation to verify proper integration
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId]/layout.tsx"
# Also check the nFormatter implementation
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts"
Length of output: 2201
apps/webservice/src/app/[workspaceSlug]/(app)/systems/SystemsBreadcrumb.tsx (3)
32-32: LGTM: Parameter changes are well-structured
The addition of the optional runbookId parameter and its destructuring are implemented correctly following TypeScript best practices.
Also applies to: 35-36
92-100: LGTM: Breadcrumb entry is well-integrated
The new runbook breadcrumb entry follows the existing patterns and is properly integrated into the navigation hierarchy. The path construction and conditional rendering are implemented correctly.
99-99: Verify router configuration for runbook paths
Let's ensure the path structure /${workspaceSlug}/systems/${systemSlug}/runbooks/${runbookId} is properly configured in the router.
✅ Verification successful
The previous command failed due to typescript type not being recognized. Let's try with explicit type definition and also look for the directory structure to verify the routing configuration, as Next.js uses file-system based routing.
Path structure is correctly configured across the application
The path structure /${workspaceSlug}/systems/${systemSlug}/runbooks/${runbookId} is properly configured in the Next.js file-system based routing and consistently used across the application:
- File structure confirms the route exists:
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/[runbookId] - Path is consistently used in:
- Navigation components (AppSidebarContent.tsx, AppSidebarPopoverSystem.tsx)
- Runbook-specific components (RunbookNavBar.tsx, CreateRunbookForm.tsx)
- Page components (RunbookGettingStarted.tsx, page.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for router configuration files and route definitions
rg -t typescript "systems.*runbooks.*" --glob "**/*router*"
Length of output: 96
Script:
#!/bin/bash
# Search for route configurations in app directory structure
fd --type directory "runbooks?" --exec ls -R {} \;
# Search for route patterns in TypeScript/JavaScript files
rg "systems.*runbooks.*" --type-add 'ts:*.{ts,tsx}' --type ts
Length of output: 2080
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/DeploymentNavBar.tsx (2)
14-14: LGTM! Good refactoring of the formatter.
Moving the nFormatter to a shared location improves code reusability and maintainability.
Line range hint 1-143: Verify runbook integration requirements
Given that this PR is about initializing runbooks UI, should there be a runbook-related navigation item in this deployment navigation bar?
Let's check for any runbook-related patterns in the codebase:
packages/api/src/router/runbook.ts (1)
149-225: Enforce a maximum limit on the limit parameter
The limit parameter defaults to 500 but can be set to any number by the client. To prevent potential performance issues due to large data retrievals, consider enforcing a maximum allowable limit.
[performance_issue]
You might implement it like this:
const MAX_LIMIT = 1000; // Set an appropriate maximum limit
const limit = Math.min(input.limit, MAX_LIMIT);| @@ -0,0 +1,16 @@ | |||
| export const nFormatter = (num: number, digits: number) => { | |||
There was a problem hiding this comment.
Add input validation and type constraints.
The function should handle edge cases like negative numbers, NaN, and Infinity. Also, digits should be constrained to non-negative integers.
-export const nFormatter = (num: number, digits: number) => {
+export const nFormatter = (num: number, digits: number): string => {
+ if (!Number.isFinite(num)) return 'N/A';
+ if (num < 0) return '-' + nFormatter(-num, digits);
+ if (!Number.isInteger(digits) || digits < 0) {
+ throw new Error('digits must be a non-negative integer');
+ }📝 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 const nFormatter = (num: number, digits: number) => { | |
| export const nFormatter = (num: number, digits: number): string => { | |
| if (!Number.isFinite(num)) return 'N/A'; | |
| if (num < 0) return '-' + nFormatter(-num, digits); | |
| if (!Number.isInteger(digits) || digits < 0) { | |
| throw new Error('digits must be a non-negative integer'); | |
| } |
| const lookup = [ | ||
| { value: 1, symbol: "" }, | ||
| { value: 1e3, symbol: "k" }, | ||
| { value: 1e6, symbol: "M" }, | ||
| { value: 1e9, symbol: "G" }, | ||
| { value: 1e12, symbol: "T" }, | ||
| { value: 1e15, symbol: "P" }, | ||
| { value: 1e18, symbol: "E" }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize lookup table definition.
The lookup table is recreated on every function call. Consider moving it outside the function scope.
+const NUMBER_FORMAT_LOOKUP = [
+ { value: 1, symbol: "" },
+ { value: 1e3, symbol: "k" },
+ { value: 1e6, symbol: "M" },
+ { value: 1e9, symbol: "G" },
+ { value: 1e12, symbol: "T" },
+ { value: 1e15, symbol: "P" },
+ { value: 1e18, symbol: "E" },
+] as const;
+
export const nFormatter = (num: number, digits: number) => {
- const lookup = [
- { value: 1, symbol: "" },
- { value: 1e3, symbol: "k" },
- { value: 1e6, symbol: "M" },
- { value: 1e9, symbol: "G" },
- { value: 1e12, symbol: "T" },
- { value: 1e15, symbol: "P" },
- { value: 1e18, symbol: "E" },
- ];📝 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 lookup = [ | |
| { value: 1, symbol: "" }, | |
| { value: 1e3, symbol: "k" }, | |
| { value: 1e6, symbol: "M" }, | |
| { value: 1e9, symbol: "G" }, | |
| { value: 1e12, symbol: "T" }, | |
| { value: 1e15, symbol: "P" }, | |
| { value: 1e18, symbol: "E" }, | |
| ]; | |
| const NUMBER_FORMAT_LOOKUP = [ | |
| { value: 1, symbol: "" }, | |
| { value: 1e3, symbol: "k" }, | |
| { value: 1e6, symbol: "M" }, | |
| { value: 1e9, symbol: "G" }, | |
| { value: 1e12, symbol: "T" }, | |
| { value: 1e15, symbol: "P" }, | |
| { value: 1e18, symbol: "E" }, | |
| ] as const; | |
| export const nFormatter = (num: number, digits: number) => { |
There was a problem hiding this comment.
Do we have a library for this? Do we have this logic somewhere else in the code?
There was a problem hiding this comment.
@jsbroks, I checked the codebase and didn't find any existing library or similar logic for this number formatting functionality. If we need more advanced or consistent number formatting across the application, we might consider adding a library like numeral or numbro. Otherwise, we can proceed with this implementation as it stands.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing number formatting functions similar to 'nFormatter' in the codebase.
rg 'function\s+(nFormatter|numberFormatter|formatNumberWithSuffix)\s*\(' --type=typescript --type=javascript
# Search for any imports of number formatting libraries.
rg 'import\s+.*\s+from\s+[\'"](numeral|numbro|number-format|human-number)[\'"]' --type=typescript --type=javascript
# Search `package.json` and `package-lock.json` for dependencies related to number formatting.
rg '"(numeral|numbro|number-format|human-number)"' package.json package-lock.json
# Search for code that includes similar lookup tables for number formatting.
rg 'value:\s*1e3' --type=typescript --type=javascript
Length of output: 263
There was a problem hiding this comment.
@jsbroks, great point! We can indeed use Intl.NumberFormat with the compact notation to format numbers with suffixes like 'K', 'M', etc. This provides a more standardized and localized approach. Here's how we could implement it:
export const nFormatter = (num: number) => {
return new Intl.NumberFormat('en', { notation: 'compact' }).format(num);
};This will format numbers as:
1000→'1K'1500000→'1.5M'
Would you like to proceed with this implementation?
(*^_^)/
✏️ Learnings added
Learnt from: jsbroks
PR: ctrlplanedev/ctrlplane#241
File: apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/_components/nFormatter.ts:2-10
Timestamp: 2024-11-29T04:21:47.818Z
Learning: Use `Intl.NumberFormat` with `compact` notation for number formatting with suffixes like 'K', 'M' instead of custom functions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| export async function generateMetadata({ | ||
| params, | ||
| }: PageProps): Promise<Metadata> { | ||
| const runbook = await api.runbook.byId(params.runbookId); | ||
| if (runbook == null) return notFound(); | ||
|
|
||
| return { | ||
| title: `${runbook.name} | Runbooks`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit error handling for API calls
The metadata generator should handle potential API errors explicitly to prevent unhandled exceptions from propagating to the UI.
export async function generateMetadata({
params,
}: PageProps): Promise<Metadata> {
- const runbook = await api.runbook.byId(params.runbookId);
- if (runbook == null) return notFound();
+ try {
+ const runbook = await api.runbook.byId(params.runbookId);
+ if (runbook == null) return notFound();
- return {
- title: `${runbook.name} | Runbooks`,
- };
+ return {
+ title: `${runbook.name} | Runbooks`,
+ };
+ } catch (error) {
+ console.error('Failed to fetch runbook metadata:', error);
+ return {
+ title: 'Error | Runbooks',
+ };
+ }
}📝 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 async function generateMetadata({ | |
| params, | |
| }: PageProps): Promise<Metadata> { | |
| const runbook = await api.runbook.byId(params.runbookId); | |
| if (runbook == null) return notFound(); | |
| return { | |
| title: `${runbook.name} | Runbooks`, | |
| }; | |
| } | |
| export async function generateMetadata({ | |
| params, | |
| }: PageProps): Promise<Metadata> { | |
| try { | |
| const runbook = await api.runbook.byId(params.runbookId); | |
| if (runbook == null) return notFound(); | |
| return { | |
| title: `${runbook.name} | Runbooks`, | |
| }; | |
| } catch (error) { | |
| console.error('Failed to fetch runbook metadata:', error); | |
| return { | |
| title: 'Error | Runbooks', | |
| }; | |
| } | |
| } |
| <Button variant="secondary" className="flex items-center gap-2"> | ||
| <IconBolt className="h-4 w-4" /> | ||
| Trigger runbook | ||
| </Button> |
There was a problem hiding this comment.
Add onClick handler for the trigger button.
The "Trigger runbook" button appears to be missing its click handler implementation.
Example implementation:
-<Button variant="secondary" className="flex items-center gap-2">
+<Button
+ variant="secondary"
+ className="flex items-center gap-2"
+ onClick={() => {
+ // TODO: Implement runbook trigger logic
+ }}
+>Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <div className="flex items-center justify-between border-b p-2"> | ||
| <div> | ||
| <NavigationMenu> | ||
| <NavigationMenuList> | ||
| <NavigationMenuItem> | ||
| <Link href={baseUrl} legacyBehavior passHref> | ||
| <NavigationMenuLink | ||
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | ||
| active={isJobsActive} | ||
| > | ||
| Jobs | ||
| <Badge | ||
| variant="outline" | ||
| className="ml-1.5 rounded-full text-muted-foreground" | ||
| > | ||
| {nFormatter(totalJobs, 1)} | ||
| </Badge> | ||
| </NavigationMenuLink> | ||
| </Link> | ||
| </NavigationMenuItem> | ||
| <NavigationMenuItem> | ||
| <Link href={settingsUrl} legacyBehavior passHref> | ||
| <NavigationMenuLink | ||
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | ||
| active={isSettingsActive} | ||
| > | ||
| Settings | ||
| </NavigationMenuLink> | ||
| </Link> | ||
| </NavigationMenuItem> | ||
| </NavigationMenuList> | ||
| </NavigationMenu> | ||
| </div> | ||
| <Button variant="secondary" className="flex items-center gap-2"> | ||
| <IconBolt className="h-4 w-4" /> | ||
| Trigger runbook | ||
| </Button> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with ARIA attributes.
Consider adding the following accessibility improvements:
-<div className="flex items-center justify-between border-b p-2">
+<nav
+ className="flex items-center justify-between border-b p-2"
+ aria-label="Runbook navigation"
+>
<div>
- <NavigationMenu>
+ <NavigationMenu aria-label="Runbook sections">Also, consider adding keyboard navigation support for the navigation menu items.
📝 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.
| return ( | |
| <div className="flex items-center justify-between border-b p-2"> | |
| <div> | |
| <NavigationMenu> | |
| <NavigationMenuList> | |
| <NavigationMenuItem> | |
| <Link href={baseUrl} legacyBehavior passHref> | |
| <NavigationMenuLink | |
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | |
| active={isJobsActive} | |
| > | |
| Jobs | |
| <Badge | |
| variant="outline" | |
| className="ml-1.5 rounded-full text-muted-foreground" | |
| > | |
| {nFormatter(totalJobs, 1)} | |
| </Badge> | |
| </NavigationMenuLink> | |
| </Link> | |
| </NavigationMenuItem> | |
| <NavigationMenuItem> | |
| <Link href={settingsUrl} legacyBehavior passHref> | |
| <NavigationMenuLink | |
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | |
| active={isSettingsActive} | |
| > | |
| Settings | |
| </NavigationMenuLink> | |
| </Link> | |
| </NavigationMenuItem> | |
| </NavigationMenuList> | |
| </NavigationMenu> | |
| </div> | |
| <Button variant="secondary" className="flex items-center gap-2"> | |
| <IconBolt className="h-4 w-4" /> | |
| Trigger runbook | |
| </Button> | |
| </div> | |
| ); | |
| return ( | |
| <nav | |
| className="flex items-center justify-between border-b p-2" | |
| aria-label="Runbook navigation" | |
| > | |
| <div> | |
| <NavigationMenu aria-label="Runbook sections"> | |
| <NavigationMenuList> | |
| <NavigationMenuItem> | |
| <Link href={baseUrl} legacyBehavior passHref> | |
| <NavigationMenuLink | |
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | |
| active={isJobsActive} | |
| > | |
| Jobs | |
| <Badge | |
| variant="outline" | |
| className="ml-1.5 rounded-full text-muted-foreground" | |
| > | |
| {nFormatter(totalJobs, 1)} | |
| </Badge> | |
| </NavigationMenuLink> | |
| </Link> | |
| </NavigationMenuItem> | |
| <NavigationMenuItem> | |
| <Link href={settingsUrl} legacyBehavior passHref> | |
| <NavigationMenuLink | |
| className="group inline-flex h-9 w-max items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-colors hover:bg-accent/50 hover:text-accent-foreground focus:outline-none disabled:pointer-events-none disabled:opacity-50 data-[active]:bg-accent/50 data-[state=open]:bg-accent/50" | |
| active={isSettingsActive} | |
| > | |
| Settings | |
| </NavigationMenuLink> | |
| </Link> | |
| </NavigationMenuItem> | |
| </NavigationMenuList> | |
| </NavigationMenu> | |
| </div> | |
| <Button variant="secondary" className="flex items-center gap-2"> | |
| <IconBolt className="h-4 w-4" /> | |
| Trigger runbook | |
| </Button> | |
| </nav> | |
| ); |
| const { setJobId } = useJobDrawer(); | ||
| const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery( | ||
| { workspaceId }, | ||
| { workspaceId, limit: 0 }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results and analysis of the codebase, I can now generate the final response:
Consider using pagination instead of limit: 0 for better performance
Setting limit: 0 to fetch all records is not the optimal approach. The codebase shows that pagination is already implemented in other routers (as seen in resources.ts, system.ts, and release.ts) with a pattern that returns both items and total. This pattern should be used here instead.
apps/webservice/src/app/[workspaceSlug]/(app)/(job)/jobs/JobTable.tsx: Replace the unlimited query with a paginated one that only fetches the total count- Other components like
TargetPageContent.tsxandJobHistoryChart.tsxshow similar patterns of usinglimit: 0which should also be reviewed
Example implementation:
-const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
- { workspaceId, limit: 0 },
- { refetchInterval: 60_000, placeholderData: (prev) => prev },
-);
+const jobsTotal = api.job.config.byWorkspaceId.list.useQuery(
+ { workspaceId, limit: 1, page: 0 },
+ { refetchInterval: 60_000, select: (data) => data.total },
+);🔗 Analysis chain
Consider optimizing the data fetching strategy
The current implementation fetches all records with limit: 0 when only the total count is needed for the "NoFilterMatch" component. This could be inefficient for large datasets.
Consider these improvements:
- Add a separate endpoint to fetch only the count
- Or modify the existing endpoint to accept a
countOnlyparameter - Or use the same query instance with different parameters to avoid potential UI inconsistencies
Example implementation:
-const allReleaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
- { workspaceId, limit: 0 },
- { refetchInterval: 60_000, placeholderData: (prev) => prev },
-);
+const jobCount = api.job.config.byWorkspaceId.count.useQuery(
+ { workspaceId },
+ { refetchInterval: 60_000, placeholderData: (prev) => prev },
+);Let's verify if there are any existing count-only endpoints or if other components are using similar patterns:
Let me analyze the results and generate additional verification scripts to check for count-specific endpoints in the codebase.
The results will help us determine if there are existing count endpoints or if we need to suggest creating new ones.
Let me try one more search pattern to look for any count-related implementations.
Let me try one final search to specifically look for the job router implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar query patterns in the codebase
rg -A 2 "byWorkspaceId.list.useQuery"
# Search for any count-specific endpoints
ast-grep --pattern 'export const count = $$$'
Length of output: 7204
Script:
#!/bin/bash
# Search for count-related router endpoints
rg -A 5 "router.*count"
# Search for count-related procedures in TRPC
rg -A 5 "procedure.*count"
# Check if there are any existing count implementations
ast-grep --pattern 'count = async ($$$) => {
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Search for count-related implementations with broader patterns
rg -i "count.*=" -A 5
# Look for any pagination or total count related code
rg -i "total.*items|pagination" -A 5
# Check the specific router file for the jobs endpoint
fd -e ts -e tsx "job.*router" --exec cat {}
Length of output: 36347
Script:
#!/bin/bash
# Search for job router implementation
rg -A 10 "job.*router.*="
# Search for job-related TRPC procedures
rg -A 10 "job.*procedure"
# Look for any count-related endpoints in job routes
rg -A 10 "job.*count"
Length of output: 995
| {runbookJobs.items.map((job) => ( | ||
| <TableRow key={job.job.id} className="cursor-pointer"> | ||
| <TableCell> | ||
| <div className="flex items-center gap-1"> | ||
| <JobTableStatusIcon status={job.job.status} /> | ||
| {JobStatusReadable[job.job.status]} | ||
| </div> | ||
| </TableCell> | ||
| <TableCell> | ||
| {formatDistanceToNowStrict(job.job.createdAt, { | ||
| addSuffix: true, | ||
| })} | ||
| </TableCell> | ||
| <VariableCell variables={job.job.variables} /> | ||
| </TableRow> | ||
| ))} |
There was a problem hiding this comment.
Fix incorrect property access in job mapping
The properties of job are being accessed incorrectly. Instead of job.job.id, job.job.status, and job.job.variables, you should access job.id, job.status, and job.variables.
Apply this diff to correct the property access:
- {runbookJobs.items.map((job) => (
- <TableRow key={job.job.id} className="cursor-pointer">
- <TableCell>
- <div className="flex items-center gap-1">
- <JobTableStatusIcon status={job.job.status} />
- {JobStatusReadable[job.job.status]}
- </div>
- </TableCell>
- <TableCell>
- {formatDistanceToNowStrict(job.job.createdAt, {
- addSuffix: true,
- })}
- </TableCell>
- <VariableCell variables={job.job.variables} />
- </TableRow>
- ))}
+ {runbookJobs.items.map((job) => (
+ <TableRow key={job.id} className="cursor-pointer">
+ <TableCell>
+ <div className="flex items-center gap-1">
+ <JobTableStatusIcon status={job.status} />
+ {JobStatusReadable[job.status]}
+ </div>
+ </TableCell>
+ <TableCell>
+ {formatDistanceToNowStrict(job.createdAt, {
+ addSuffix: true,
+ })}
+ </TableCell>
+ <VariableCell variables={job.variables} />
+ </TableRow>
+ ))}Committable suggestion skipped: line range outside the PR's diff.
| )} | ||
| <div className="space-y-2"> | ||
| {condition.conditions.map((subCond, index) => ( | ||
| <div key={index} className="flex items-start gap-2"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index as key in list rendering
Using the array index index as a key in list rendering can lead to issues with component state and may cause unnecessary re-renders if the list changes order. It's better to use a unique identifier for each item if available.
If subCond has a unique identifier, consider using it as the key:
- <div key={index} className="flex items-start gap-2">
+ <div key={getUniqueKeyForCondition(subCond)} className="flex items-start gap-2">Ensure that getUniqueKeyForCondition returns a unique and stable identifier for each condition.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
RunbookJobConditionDialog,RunbookJobComparisonConditionRender, andRunbookJobConditionRender.RunbookNavBarfor navigation related to runbooks.RunbookPageto display job lists associated with specific runbooks.Improvements
JobTableto include a limit parameter for better data management.nFormatterutility for consistent number formatting across components.Bug Fixes