Side help panel #1488
Side help panel #1488
Conversation
|
WalkthroughThis pull request introduces several new components and modifications to existing ones within the web application. A new icon component, Changes
Possibly related PRs
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 (1.23.1)
apps/webapp/app/assets/icons/SideMenuRightClosed.tsx(node:29480) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.15.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 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: 4
🧹 Outside diff range and nitpick comments (12)
apps/webapp/app/assets/icons/SideMenuRightClosed.tsx (1)
1-15: LGTM! Consider adding aria-label for accessibility.The component implementation is clean and follows React/TypeScript best practices. The use of
currentColorfor fill/stroke ensures proper theme integration.Consider adding accessibility attributes to make the icon more screen-reader friendly:
<svg className={className} width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-label="Side menu closed" + role="img" >apps/webapp/app/services/preferences/uiPreferences.server.ts (1)
4-14: Consider adjusting cookie configuration for better security and maintenance.While the basic security settings are good, consider the following improvements:
- The 1-year maxAge is quite long for UI preferences and could lead to stale data
- Adding explicit size limits could prevent storage abuse
- Consider adding an explicit domain setting for better control in different environments
export const uiPreferencesStorage = createCookieSessionStorage({ cookie: { name: "__ui_prefs", sameSite: "lax", path: "/", httpOnly: true, secrets: [env.SESSION_SECRET], secure: env.NODE_ENV === "production", - maxAge: 60 * 60 * 24 * 365, // 1 year + maxAge: 60 * 60 * 24 * 30, // 30 days + domain: env.COOKIE_DOMAIN, // Add if available in env }, });apps/webapp/app/components/primitives/AnimatingArrow.tsx (2)
4-41: Consider improving maintainability and consistency of variants.The variants configuration has a few areas for improvement:
- Hardcoded pixel values could be problematic for responsive design
- Inconsistent translation values across variants (e.g.,
translate-x-[3px]vstranslate-x-1)- Repetitive structure that could be simplified
Consider this refactor to improve maintainability:
+const baseTranslations = { + small: '3px', + medium: '3px', + large: '0.25rem', + 'extra-large': '0.25rem' +}; + +const baseLineHeights = { + small: '1.5px', + medium: '1.5px', + large: '2.3px', + 'extra-large': '3px' +}; + const variants = { small: { size: "size-[1rem]", - arrowHeadRight: "group-hover:translate-x-[3px]", + arrowHeadRight: `group-hover:translate-x-[${baseTranslations.small}]`, // ... similar refactoring for other properties }, // ... other variants };
43-80: Add documentation for theme usage and color tokens.The themes configuration is well-structured, but could benefit from documentation explaining:
- The intended use case for each theme
- The color token system being used
- Whether these themes align with the design system
Consider adding JSDoc comments:
+/** + * Predefined themes for the AnimatingArrow component. + * @property {Object} dark - Used for dark backgrounds + * @property {Object} dimmed - Default state with reduced emphasis + * @property {Object} bright - High contrast state + * @property {Object} primary - Uses the primary brand color + * ... (document other themes) + */ export const themes = { dark: { textStyle: "text-background-bright", arrowLine: "bg-background-bright", }, // ... other themes };apps/webapp/app/components/primitives/Buttons.tsx (2)
271-278: Simplify the conditional logicThe condition can be simplified since "after-trailing-icon" is the default behavior.
- {shortcut && - (!props.shortcutPosition || props.shortcutPosition === "after-trailing-icon") && ( + {shortcut && props.shortcutPosition !== "before-trailing-icon" && (
241-247: Consider extracting ShortcutKey rendering to reduce duplicationThe ShortcutKey component rendering is duplicated. Consider extracting it to a helper function for better maintainability.
+ const renderShortcutKey = () => ( + <ShortcutKey + className={cn(shortcutClassName)} + shortcut={shortcut} + variant={variation.shortcutVariant ?? "medium"} + /> + ); // Then in JSX: - <ShortcutKey - className={cn(shortcutClassName)} - shortcut={shortcut} - variant={variation.shortcutVariant ?? "medium"} - /> + {renderShortcutKey()}Also applies to: 271-278
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx (2)
Line range hint
142-150: Consider standardizing error message formattingWhile the error handling is robust, the error message formatting could be improved for consistency and readability.
Consider extracting the error formatting logic into a utility function:
+ const formatErrorMessage = (e: unknown, context: string) => + `${context}: ${e instanceof Error ? e.message : JSON.stringify(e)}`; - `${scheduleParam} could not be deleted: ${ - e instanceof Error ? e.message : JSON.stringify(e) - }` + formatErrorMessage(e, `${scheduleParam} could not be deleted`) - `${scheduleParam} could not be ${active ? "enabled" : "disabled"}: ${ - e instanceof Error ? e.message : JSON.stringify(e) - }` + formatErrorMessage(e, `${scheduleParam} could not be ${active ? "enabled" : "disabled"}`)Also applies to: 183-191
Line range hint
324-326: Improve placeholder message for debugging contextThe placeholder message "You found a bug" is not very informative for users or developers.
Consider providing more context:
- <PlaceholderText title="You found a bug" /> + <PlaceholderText title="No upcoming runs found. This might indicate an issue with the schedule configuration." />apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx (4)
303-305: Simplify: Remove unnecessary Fragment in Suspense fallbackThe
fallbackprop ofSuspensecontains a Fragment wrapping a single child, which is unnecessary. Removing it simplifies the code.Apply this diff to remove the unnecessary Fragment:
<Suspense fallback={ - <> <Spinner color="muted" /> - </> } >🧰 Tools
🪛 Biome (1.9.4)
[error] 303-305: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
317-317: Simplify: Replace empty Fragment withnullin Suspense fallbackAn empty Fragment is used as the
fallbackinSuspense, which is unnecessary. Usenullinstead to make it clear that nothing should be rendered during the fallback.Apply this diff:
<Suspense fallback={ - <> - </> + null }>🧰 Tools
🪛 Biome (1.9.4)
[error] 317-317: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
347-347: Simplify: Replace empty Fragment withnullin Suspense fallbackAn empty Fragment is used as the
fallbackinSuspense, which is unnecessary. Usingnullimproves clarity.Apply this diff:
<Suspense fallback={ - <> - </> + null }>🧰 Tools
🪛 Biome (1.9.4)
[error] 347-347: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
118-132: Add error handling in theactionfunctionThe
actionfunction lacks error handling for potential exceptions that might occur when setting user preferences or committing the session. Adding atry-catchblock ensures that errors are properly managed and users receive appropriate feedback.Apply this diff to add error handling:
export async function action({ request }: ActionFunctionArgs) { + try { const formData = await request.formData(); const showUsefulLinks = formData.get("showUsefulLinks") === "true"; const session = await setUsefulLinksPreference(showUsefulLinks, request); return json( { success: true }, { headers: { "Set-Cookie": await uiPreferencesStorage.commitSession(session), }, } ); + } catch (error) { + console.error(error); + return json( + { success: false, error: "Failed to update preferences." }, + { status: 500 } + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/webapp/app/assets/icons/SideMenuRightClosed.tsx(1 hunks)apps/webapp/app/components/primitives/AnimatingArrow.tsx(1 hunks)apps/webapp/app/components/primitives/Buttons.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx(7 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx(1 hunks)apps/webapp/app/services/preferences/uiPreferences.server.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
[error] 303-305: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 317-317: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
[error] 347-347: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
🔇 Additional comments (8)
apps/webapp/app/assets/icons/SideMenuRightClosed.tsx (1)
11-12: Verify the icon dimensions match the design system.
The icon uses specific dimensions (20x20) and internal spacing. Let's ensure this aligns with other icons in the application.
✅ Verification successful
Icon dimensions are consistent with the design system
Based on the codebase analysis, the SideMenuRightClosed icon follows the consistent pattern used across the application's icons:
- It uses a 20x20 viewBox, matching other icons like TimedOutIcon, TwoTreesIcon
- The internal spacing with rect elements at x="12" and x="2.5" maintains proportional layout similar to other icons
- The stroke and fill attributes use "currentColor" which is the standard approach used across all icons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent icon dimensions across the codebase
# Look for other SVG icons to verify size consistency
# Search for SVG width/height declarations
echo "Checking SVG dimensions in other icon components:"
rg 'width="|height="' --type tsx --glob '**/assets/icons/*.tsx'
# Search for viewBox declarations
echo -e "\nChecking viewBox dimensions:"
rg 'viewBox="0 0 \d+ \d+"' --type tsx --glob '**/assets/icons/*.tsx'
Length of output: 374
Script:
#!/bin/bash
# Let's try a different approach to find icon files and check their dimensions
# First, find all icon files in the assets directory
echo "Finding icon files:"
fd -e tsx -p 'icons.*\.tsx$'
# Then let's examine the content of these files
echo -e "\nChecking SVG attributes in icon files:"
fd -e tsx -p 'icons.*\.tsx$' -x cat {}
Length of output: 37066
apps/webapp/app/services/preferences/uiPreferences.server.ts (2)
16-18: LGTM! Clean and type-safe implementation.
The function follows Remix patterns and properly handles cookie retrieval.
1-29: Verify integration with the side help panel component.
Let's ensure this preference service is properly integrated with the side help panel mentioned in the PR objectives.
✅ Verification successful
Integration verified - the side help panel is properly implemented
The code shows proper integration of the UI preferences with the side help panel:
- The preference is correctly stored and retrieved using the
uiPreferences.server.tsservice - The Tasks page (
_index/route.tsx) properly implements the panel toggle:- Shows/hides based on
showUsefulLinksstate - Provides a toggle button when panel is hidden
- Persists preference through form submission
- Shows/hides based on
- The panel contains helpful documentation, links and examples as intended
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these preference functions in the side help panel component
rg -t typescript -t tsx "getUsefulLinksPreference|setUsefulLinksPreference" --context 3
Length of output: 124
Script:
#!/bin/bash
# Search for usage of these preference functions without file type restrictions
rg "getUsefulLinksPreference|setUsefulLinksPreference" -l
# Search for files containing "useful links" or "help panel" related components
rg -i "usefullinks|helppanel" -l
# Find TypeScript/TSX files in the codebase
fd "\.(ts|tsx)$"
Length of output: 76467
Script:
#!/bin/bash
# Search for the content of the route file to understand the usage
cat apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
# Search for any components that might be using these preferences
rg -l "showUsefulLinks"
Length of output: 344
Script:
#!/bin/bash
# Try to read the route file with the correct path
cat "apps/webapp/app/routes/_app.orgs.\$organizationSlug.projects.v3.\$projectParam._index/route.tsx"
# Look for the content around showUsefulLinks usage
rg "showUsefulLinks" -C 5
Length of output: 41495
apps/webapp/app/components/primitives/AnimatingArrow.tsx (2)
1-2: LGTM! Clean and focused imports.
The imports are minimal and purposeful, using specific icon imports from HeroIcons and a utility function for class name management.
82-90: LGTM! Well-structured type definitions.
The type definitions are clean, properly derived from configuration objects, and provide good type safety for the component props.
apps/webapp/app/components/primitives/Buttons.tsx (2)
177-177: LGTM: Well-defined type for shortcut positioning
The new optional shortcutPosition prop is well-typed and maintains backward compatibility.
Line range hint 177-278: Verify impact on existing button usages
The new shortcut positioning feature is backward compatible, but let's verify existing button usages to ensure no unintended layout shifts.
✅ Verification successful
Let me analyze the existing button usages to verify the impact. I'll search for actual instances where ButtonContent is used with shortcuts.
Let me try one more search to find actual JSX usage of ButtonContent with shortcuts.
No impact on existing button usages
Based on the verification results:
- The shortcut feature is only used within the
Buttons.tsxcomponent itself for styling definitions - No existing
ButtonContentusages in the codebase utilize theshortcutprop - The new
shortcutPositionprop is optional with a default fallback, ensuring backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing ButtonContent usages to verify impact
rg -l "ButtonContent" | xargs rg "shortcut[^A-Za-z]"
Length of output: 2745
Script:
#!/bin/bash
# Search for ButtonContent component usage with shortcuts
ast-grep --pattern 'ButtonContent $$$shortcut=$$$'
Length of output: 3750
Script:
#!/bin/bash
# Search for ButtonContent JSX usage
rg -A 2 "<ButtonContent.*>"
Length of output: 2940
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx (1)
219-222: LGTM: Button component changes align with PR objectives
The modifications to the LinkButton component correctly implement the new shortcut positioning feature while maintaining a consistent UI with the small variant and trailing icon placement.
| export async function getUsefulLinksPreference(request: Request): Promise<boolean | undefined> { | ||
| const session = await getUiPreferencesSession(request); | ||
| return session.get("showUsefulLinks"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for the retrieved preference value.
The current implementation doesn't validate the type of the retrieved value, which could lead to runtime issues if the stored value is not a boolean.
export async function getUsefulLinksPreference(request: Request): Promise<boolean | undefined> {
const session = await getUiPreferencesSession(request);
- return session.get("showUsefulLinks");
+ const value = session.get("showUsefulLinks");
+ return typeof value === "boolean" ? value : undefined;
}📝 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 getUsefulLinksPreference(request: Request): Promise<boolean | undefined> { | |
| const session = await getUiPreferencesSession(request); | |
| return session.get("showUsefulLinks"); | |
| } | |
| export async function getUsefulLinksPreference(request: Request): Promise<boolean | undefined> { | |
| const session = await getUiPreferencesSession(request); | |
| const value = session.get("showUsefulLinks"); | |
| return typeof value === "boolean" ? value : undefined; | |
| } |
| export async function setUsefulLinksPreference(show: boolean, request: Request) { | ||
| const session = await getUiPreferencesSession(request); | ||
| session.set("showUsefulLinks", show); | ||
| return session; | ||
| } |
There was a problem hiding this comment.
Critical: Session changes are not persisted.
The function returns the modified session but doesn't commit it, which means changes won't be saved to the cookie. Also, the return type should be explicitly annotated.
-export async function setUsefulLinksPreference(show: boolean, request: Request) {
+export async function setUsefulLinksPreference(show: boolean, request: Request): Promise<Response> {
const session = await getUiPreferencesSession(request);
session.set("showUsefulLinks", show);
- return session;
+ return uiPreferencesStorage.commitSession(session);
}📝 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 setUsefulLinksPreference(show: boolean, request: Request) { | |
| const session = await getUiPreferencesSession(request); | |
| session.set("showUsefulLinks", show); | |
| return session; | |
| } | |
| export async function setUsefulLinksPreference(show: boolean, request: Request): Promise<Response> { | |
| const session = await getUiPreferencesSession(request); | |
| session.set("showUsefulLinks", show); | |
| return uiPreferencesStorage.commitSession(session); | |
| } |
| export function AnimatingArrow({ | ||
| className, | ||
| variant = "medium", | ||
| theme = "dimmed", | ||
| direction = "right", | ||
| }: AnimatingArrowProps) { | ||
| const variantStyles = variants[variant]; | ||
| const themeStyles = themes[theme]; | ||
|
|
||
| return ( | ||
| <span className={cn("relative -mr-1 ml-1 flex", variantStyles.size, className)}> | ||
| {direction === "topRight" && ( | ||
| <> | ||
| <svg | ||
| className={cn( | ||
| "absolute top-[5px] transition duration-200 ease-in-out", | ||
| themeStyles.textStyle | ||
| )} | ||
| width="9" | ||
| height="8" | ||
| viewBox="0 0 9 8" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <path d="M1.5 7L7.5 1" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" /> | ||
| </svg> | ||
|
|
||
| <svg | ||
| className={cn( | ||
| "absolute top-[5px] transition duration-300 ease-in-out", | ||
| themeStyles.textStyle, | ||
| variantStyles.arrowHeadTopRight | ||
| )} | ||
| width="9" | ||
| height="8" | ||
| viewBox="0 0 9 8" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <path d="M1 1H7.5" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" /> | ||
| <path d="M7.5 7L7.5 1" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" /> | ||
| <path d="M1 7.5L7.5 1" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" /> | ||
| </svg> | ||
| </> | ||
| )} | ||
| {direction === "right" && ( | ||
| <> | ||
| <span | ||
| className={cn( | ||
| "absolute rounded-full opacity-0 transition duration-300 ease-in-out group-hover:opacity-100", | ||
| variantStyles.arrowLineRight, | ||
| themeStyles.arrowLine | ||
| )} | ||
| /> | ||
| <ChevronRightIcon | ||
| className={cn( | ||
| "absolute -translate-x-0.5 transition duration-300 ease-in-out", | ||
| variantStyles.arrowHeadRight, | ||
| variantStyles.size, | ||
| themeStyles.textStyle | ||
| )} | ||
| /> | ||
| </> | ||
| )} | ||
| {direction === "left" && ( | ||
| <> | ||
| <span | ||
| className={cn( | ||
| "absolute rounded-full opacity-0 transition duration-300 ease-in-out group-hover:opacity-100", | ||
| variantStyles.arrowLineLeft, | ||
| themeStyles.arrowLine | ||
| )} | ||
| /> | ||
| <ChevronLeftIcon | ||
| className={cn( | ||
| "absolute translate-x-0.5 transition duration-300 ease-in-out", | ||
| variantStyles.arrowHeadLeft, | ||
| variantStyles.size, | ||
| themeStyles.textStyle | ||
| )} | ||
| /> | ||
| </> | ||
| )} | ||
| </span> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility improvements and optimize SVGs.
The component implementation could be enhanced with:
- Accessibility attributes for SVGs (aria-hidden, role)
- Optimized SVG paths
- Error boundaries for SVG rendering failures
Apply these improvements:
<svg
className={cn(
"absolute top-[5px] transition duration-200 ease-in-out",
themeStyles.textStyle
)}
+ aria-hidden="true"
+ role="presentation"
width="9"
height="8"
viewBox="0 0 9 8"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
- <path d="M1.5 7L7.5 1" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" />
+ <path d="M1.5 7l6-6" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" />
</svg>Also consider wrapping the component in an error boundary:
class SVGErrorBoundary extends React.Component {
// ... implement error boundary
}
export function AnimatingArrow({...}: AnimatingArrowProps) {
return (
<SVGErrorBoundary fallback={<span>⟶</span>}>
{/* existing implementation */}
</SVGErrorBoundary>
);
}| export async function action({ request }: ActionFunctionArgs) { | ||
| const formData = await request.formData(); | ||
| const showUsefulLinks = formData.get("showUsefulLinks") === "true"; | ||
|
|
||
| const session = await setUsefulLinksPreference(showUsefulLinks, request); | ||
|
|
||
| return json( | ||
| { success: true }, | ||
| { | ||
| headers: { | ||
| "Set-Cookie": await uiPreferencesStorage.commitSession(session), | ||
| }, | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure CSRF protection in the action function
The action function handles form submissions via POST requests to update user preferences. Verify that CSRF protection is implemented to prevent unauthorized requests from malicious sources.
@trigger.dev/build
@trigger.dev/rsc
@trigger.dev/sdk
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/core
commit: |
Additional