New onboarding question #1404
New onboarding question #1404
Conversation
|
WalkthroughThe changes in this pull request include modifications to several components and routes within the web application. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant API
User->>App: Fill out organization creation form
App->>API: Submit form with user details and feedback
API-->>App: Confirm user details and feedback saved
App-->>User: Display confirmation message
Possibly related PRs
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 (7)
apps/webapp/app/routes/storybook.textarea/route.tsx (1)
8-13: LGTM: Form elements are well-configured with a minor suggestion.The variety of
TextAreaconfigurations effectively demonstrates different use cases, and the inclusion of anInputcomponent provides contrast. The use of thegap-4class ensures proper spacing.Consider adding
aria-labelattributes to improve accessibility, especially for the disabledTextArea.You could enhance accessibility by adding
aria-labelattributes:- <TextArea placeholder="6 rows (default)" autoFocus /> - <Input placeholder="Input" /> - <TextArea placeholder="3 rows" rows={3} /> - <TextArea disabled placeholder="Disabled" /> + <TextArea placeholder="6 rows (default)" autoFocus aria-label="Default TextArea" /> + <Input placeholder="Input" aria-label="Example Input" /> + <TextArea placeholder="3 rows" rows={3} aria-label="Three-row TextArea" /> + <TextArea disabled placeholder="Disabled" aria-label="Disabled TextArea" />apps/webapp/app/routes/storybook/route.tsx (1)
129-130: LGTM! Consider adding a section title for consistency.The changes to the
storiesarray look good. The reordering of "Input fields" and "Simple form" entries, along with the addition of the new "Textarea" entry, are consistent with the PR objectives and maintain the existing structure.For consistency with other form-related entries, consider adding a
sectionTitle: "Forms"to the "Textarea" entry:{ + sectionTitle: "Forms", name: "Textarea", slug: "textarea", },Also applies to: 137-142
apps/webapp/app/routes/confirm-basic-details.tsx (1)
225-225: LGTM! Consider expanding referral options.The update to include "X (Twitter)" in the placeholder text is a good reflection of current branding. This change aligns well with the PR objective of gathering insights into how users discover the platform.
To further improve user experience and data collection, consider expanding the placeholder text to include more diverse referral sources. For example:
- placeholder="Google, X (Twitter)…?" + placeholder="Google, X (Twitter), LinkedIn, Friend…?"This could help prompt users to provide more varied and specific responses, enhancing the quality of collected data.
apps/webapp/app/routes/_app.orgs.new/route.tsx (4)
47-47: Use Consistent User Retrieval MethodYou're using
requireUser(request)to retrieve the user, which throws an error if the user is not authenticated. Since you already haveuserIdfromrequireUserId(request), you might streamline the code by retrieving the user data directly usinguserId, reducing redundant calls.Consider refactoring:
- const user = await requireUser(request); + const user = await getUserById(userId);Ensure that
getUserByIdis a function that fetches user details based onuserId.
91-92: Improve Error Handling for Customer UpsertWhen the
upsertCustomeroperation fails, you're logging the error to the console. Consider implementing a more robust error handling mechanism, such as sending an alert to monitoring services or retrying the operation if appropriate.For example:
if (upsertCustomerRes.error) { - console.error("Failed to upsert customer in Plain", upsertCustomerRes.error); + // Log error to monitoring service + sendErrorToMonitoringService(upsertCustomerRes.error); + // Optionally, inform the user or take corrective action }Ensure that
sendErrorToMonitoringServiceis implemented accordingly.
117-118: Handle Thread Creation Errors AppropriatelySimilar to the previous comment, when
createThreadfails, consider enhancing the error handling strategy to include logging to a monitoring service or implementing retry logic if necessary.For example:
if (createThreadRes.error) { - console.error("Failed to create thread in Plain", createThreadRes.error); + // Log error to monitoring service + sendErrorToMonitoringService(createThreadRes.error); + // Optionally, inform the user or take corrective action }
165-206: Optimize Form Rendering LogicThe render logic within
{isManagedCloud && (...)}uses a fragment<>unnecessarily, as there's only one root element. You can remove the fragment to simplify the code.Apply this diff:
{isManagedCloud && ( - <> <InputGroup> {/* companySize input */} </InputGroup> <InputGroup> {/* whyUseUs input */} </InputGroup> - </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/webapp/app/components/primitives/TextArea.tsx (1 hunks)
- apps/webapp/app/routes/_app.orgs.new/route.tsx (4 hunks)
- apps/webapp/app/routes/confirm-basic-details.tsx (1 hunks)
- apps/webapp/app/routes/storybook.textarea/route.tsx (1 hunks)
- apps/webapp/app/routes/storybook/route.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
apps/webapp/app/routes/storybook.textarea/route.tsx (3)
1-2: LGTM: Imports are correctly specified.The imports for
InputandTextAreacomponents from the~/components/primitivesdirectory are properly defined and utilized in the component.
4-7: LGTM: Component structure is well-organized.The
Storycomponent is properly defined and exported. The use of flexbox for layout and the nesteddivstructure provides a good organization for the form elements.Also applies to: 14-17
1-17: Overall assessment: Well-structured and purposeful component.This storybook entry effectively showcases various configurations of
TextAreaandInputcomponents. The code is clean, organized, and follows React best practices. The minor suggestion for addingaria-labelattributes would further enhance accessibility.Great job on creating a comprehensive demonstration of form elements!
apps/webapp/app/components/primitives/TextArea.tsx (1)
11-11: LGTM! Color scheme update enhances consistency.The changes to the
TextAreacomponent's styling look good. The update from tertiary colors to specific charcoal shades likely improves consistency with the overall design system. The addition of hover states for both border and background enhances the user interaction feedback.A few observations:
- The border color is now 'border-charcoal-800'.
- The background color is now 'bg-charcoal-750'.
- A hover background color 'hover:bg-charcoal-650' has been added.
- A hover border color 'hover:border-charcoal-600' has been introduced.
These changes should provide a more refined and consistent user experience.
apps/webapp/app/routes/confirm-basic-details.tsx (1)
Line range hint
219-229: New onboarding question successfully implementedThe implementation of the new onboarding question about the referral source is well-integrated into the existing form structure. Key points:
- The question is conditionally rendered based on the
isManagedCloudfeature flag, allowing for controlled rollout.- The input is properly included in the form submission and user update process.
- The placeholder text has been updated to reflect current branding (as noted in the previous comment).
To ensure the referral source data is being correctly submitted to Plain as mentioned in the PR objectives, please verify the following:
This will help confirm that the referral source data is properly integrated with the Plain API as intended.
apps/webapp/app/routes/_app.orgs.new/route.tsx (1)
62-121: Ensure Compliance with Privacy Policies When Sending User DataThe code sends user's email and input from
whyUseUsto an external service (PlainClient). Ensure that this complies with your application's privacy policies and relevant data protection regulations (e.g., GDPR).Please confirm that:
- You have user consent for sharing their data with third parties.
- The external service complies with necessary data handling standards.
commit: |
| @@ -40,7 +44,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { | |||
|
|
|||
| export const action: ActionFunction = async ({ request }) => { | |||
| const userId = await requireUserId(request); | |||
There was a problem hiding this comment.
You don't need this if you have the requireUser
| @@ -55,6 +59,67 @@ export const action: ActionFunction = async ({ request }) => { | |||
| companySize: submission.value.companySize ?? null, | |||
| }); | |||
|
|
|||
| if (env.PLAIN_API_KEY) { | |||
| const client = new PlainClient({ | |||
There was a problem hiding this comment.
Create a new plain.server.ts file, maybe in the webapp utilities folder.
It would be:
export async function sendToPlain({ userId, email, orgName, title, components}: Input) {
const client = new ......
}Then in this particular case you would wrap this in try/catch so it doesn't stop people creating orgs if Plain is down.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
apps/webapp/app/routes/resources.feedback.ts (1)
Line range hint
95-128: Approved: Improved implementation for sending feedback to PlainThe new implementation using
sendToPlainfunction is more streamlined and aligns well with the PR objectives. It correctly passes user information and formats the feedback message using UI components, which should improve readability on the Plain platform.However, I have one suggestion for improvement:
Consider adding error handling for the
sendToPlainfunction call. This will ensure that any issues with sending data to Plain are properly logged and handled. Here's a suggested implementation:- await sendToPlain({ + try { + await sendToPlain({ userId: user.id, email: user.email, name: user.name ?? user.displayName ?? user.email, title, components: [ // ... (components remain unchanged) ], }); + } catch (error) { + console.error('Error sending feedback to Plain:', error); + submission.error.message = "Failed to send feedback. Please try again later."; + return json(submission); + }This change will make the error handling more robust and consistent with the existing error handling for
upsertCustomerRes.apps/webapp/app/components/Feedback.tsx (3)
3-4: LGTM! Minor suggestion for import organization.The changes in the import statements look good. The
InformationCircleIconandEnvelopeIconimports from different versions of heroicons are correct, and the reordering of React hooks is fine.For better organization, consider grouping related imports together. You could move the heroicons imports next to each other:
import { InformationCircleIcon } from "@heroicons/react/20/solid"; import { EnvelopeIcon } from "@heroicons/react/24/solid"; import { Form, useActionData, useLocation, useNavigation } from "@remix-run/react"; import { type ReactNode, useEffect, useState } from "react";Also applies to: 6-6
Line range hint
47-55: Good improvement in form submission handling. Consider enhancing readability.The additional conditions in the
useEffecthook improve the dialog closing logic, ensuring it only closes when the form submission is successful and there are no errors. This enhancement provides a better user experience.To improve readability, consider extracting the condition into a separate variable with a descriptive name:
useEffect(() => { const isSuccessfulSubmission = navigation.formAction === "/resources/feedback" && navigation.state === "loading" && form.error === undefined && form.errors.length === 0; if (isSuccessfulSubmission) { setOpen(false); } }, [navigation, form]);This change makes the logic more self-documenting and easier to understand at a glance.
Line range hint
70-97: Great addition of contextual information panels. Consider consistency in icon usage.The introduction of
InfoPanelcomponents with specific messages for "feature" and "help" feedback types is an excellent improvement. It provides valuable guidance to users based on their selected feedback type, enhancing the overall user experience.For consistency, consider using the same icon size and import source across the component. Currently, you're using:
import { InformationCircleIcon } from "@heroicons/react/20/solid"; import { EnvelopeIcon } from "@heroicons/react/24/solid";It might be better to use the same version for both icons, preferably the 24/solid version:
import { InformationCircleIcon, EnvelopeIcon } from "@heroicons/react/24/solid";This change would ensure visual consistency across the component and simplify imports.
apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (4)
95-95: Typo in 'cancelation' – should be 'cancellation'The word "cancelation" in the
titlefield is misspelled. The correct spelling is "cancellation".Apply this diff to fix the typo:
-title: "Plan cancelation feedback", +title: "Plan cancellation feedback",
Line range hint
104-104: Improve error log message for clarityConsider rephrasing the error message in the logger for better clarity.
Apply this diff to enhance the log message:
-logger.error("Failed to submit to Plain the unsubscribe reason", { error: e }); +logger.error("Failed to submit the unsubscribe reason to Plain", { error: e });
Line range hint
81-89: Ensure consistent handling of 'reasons' in form data parsingThere's a potential type mismatch with the
reasonsfield. You retrievereasonsas an array usingformData.getAll("reasons"), but the schema allows it to be either a string or an array. This could cause issues during validation.To ensure consistent handling, adjust the schema to expect
reasonsas an array of strings:const schema = z.object({ type: z.enum(["free", "paid"]), planCode: z.string().optional(), callerPath: z.string(), - reasons: z.union([z.string(), z.array(z.string())]).optional(), + reasons: z.array(z.string()).optional(), message: z.string().optional(), });This way,
reasonswill always be an array, matching the result fromformData.getAll("reasons").
Line range hint
228-230: Avoid magic numbers by defining a constant for one dayUsing the raw number
86400000can reduce code readability. Defining a constant improves clarity.Apply this diff to define a constant for one day in milliseconds:
+const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000; ... <DateTime includeTime={false} - date={new Date(periodEnd.getTime() + 86400000)} + date={new Date(periodEnd.getTime() + ONE_DAY_IN_MS)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/webapp/app/components/Feedback.tsx (1 hunks)
- apps/webapp/app/routes/_app.orgs.new/route.tsx (4 hunks)
- apps/webapp/app/routes/resources.feedback.ts (2 hunks)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.select-plan.tsx (4 hunks)
- apps/webapp/app/utils/plain.server.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
apps/webapp/app/routes/resources.feedback.ts (2)
9-9: LGTM: New import for sendToPlain functionThe import statement for the
sendToPlainfunction from the~/utils/plain.servermodule is correctly added. This aligns with the PR objective of submitting the new onboarding question responses to Plain.
Line range hint
1-128: Summary: Changes align well with PR objectivesThe modifications to this file successfully implement the new onboarding question submission to Plain. The changes include:
- Adding a new import for the
sendToPlainfunction.- Replacing the previous
client.createThreadimplementation with a more streamlinedsendToPlainfunction call.- Maintaining existing error handling for customer upsert operations.
These changes effectively fulfill the PR objective of submitting new onboarding question responses to Plain, enhancing the data collection process during user onboarding.
To further improve the implementation, consider adding error handling for the
sendToPlainfunction call as suggested in the previous comment.apps/webapp/app/components/Feedback.tsx (1)
Line range hint
1-156: Overall, excellent improvements to the Feedback component!The changes made to this component align well with the PR objectives and significantly enhance the feedback collection process. The addition of context-specific information panels, improved form submission handling, and better user guidance contribute to a more robust and user-friendly experience.
Key improvements:
- More informative feedback process with type-specific guidance.
- Enhanced form submission logic to prevent premature dialog closing.
- Better visual cues with the use of icons.
The code is well-structured and the changes are thoughtfully implemented. With the minor suggestions for import organization and consistency, this update is a valuable addition to the onboarding process.
apps/webapp/app/utils/plain.server.ts (1)
34-40: Verify the structure ofonUpdatefields inupsertCustomerIn the
upsertCustomercall, theonUpdatefieldsexternalIdandfullNameare wrapped in{ value: ... }, whereas inonCreate, they are provided directly. Ensure that this structure aligns with the Plain API requirements.If the API specifies that
onUpdatefields should be wrapped in{ value: ... }, then this is appropriate. Otherwise, consider making the structure consistent withonCreate.
| if (upsertCustomerRes.error) { | ||
| console.error("Failed to upsert customer in Plain", upsertCustomerRes.error); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling across API calls
When upsertCustomerRes.error occurs, the function logs the error and returns. However, when createThreadRes.error occurs, the function only logs the error without returning or throwing an exception. For consistency and to prevent unexpected behaviors, consider returning after logging the error or throwing an exception in both cases.
Apply this diff for consistent error handling:
if (upsertCustomerRes.error) {
console.error("Failed to upsert customer in Plain", upsertCustomerRes.error);
return;
}
...
if (createThreadRes.error) {
console.error("Failed to create thread in Plain", createThreadRes.error);
+ return;
}Alternatively, if you prefer to throw exceptions:
if (upsertCustomerRes.error) {
console.error("Failed to upsert customer in Plain", upsertCustomerRes.error);
+ throw new Error("Failed to upsert customer in Plain");
}
...
if (createThreadRes.error) {
console.error("Failed to create thread in Plain", createThreadRes.error);
+ throw new Error("Failed to create thread in Plain");
}Also applies to: 56-58
| if (!env.PLAIN_API_KEY) { | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider logging a warning when PLAIN_API_KEY is not set
Currently, if PLAIN_API_KEY is not set, the function silently returns. To aid in debugging and monitoring, consider logging a warning message to notify that the API key is missing.
Apply this diff to log a warning:
if (!env.PLAIN_API_KEY) {
+ console.warn("PLAIN_API_KEY is not set. Skipping sendToPlain.");
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!env.PLAIN_API_KEY) { | |
| return; | |
| if (!env.PLAIN_API_KEY) { | |
| console.warn("PLAIN_API_KEY is not set. Skipping sendToPlain."); | |
| return; |
| export async function sendToPlain({ userId, email, name, title, components }: Input) { | ||
| if (!env.PLAIN_API_KEY) { | ||
| return; | ||
| } | ||
|
|
||
| const client = new PlainClient({ | ||
| apiKey: env.PLAIN_API_KEY, | ||
| }); | ||
|
|
||
| const upsertCustomerRes = await client.upsertCustomer({ | ||
| identifier: { | ||
| emailAddress: email, | ||
| }, | ||
| onCreate: { | ||
| externalId: userId, | ||
| fullName: name, | ||
| email: { | ||
| email: email, | ||
| isVerified: true, | ||
| }, | ||
| }, | ||
| onUpdate: { | ||
| externalId: { value: userId }, | ||
| fullName: { value: name }, | ||
| email: { | ||
| email: email, | ||
| isVerified: true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (upsertCustomerRes.error) { | ||
| console.error("Failed to upsert customer in Plain", upsertCustomerRes.error); | ||
| return; | ||
| } | ||
|
|
||
| const createThreadRes = await client.createThread({ | ||
| customerIdentifier: { | ||
| customerId: upsertCustomerRes.data.customer.id, | ||
| }, | ||
| title: title, | ||
| components: components, | ||
| }); | ||
|
|
||
| if (createThreadRes.error) { | ||
| console.error("Failed to create thread in Plain", createThreadRes.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a try-catch block to handle unexpected exceptions
While the code checks for errors returned in the response objects, it doesn't handle unexpected exceptions that might be thrown by the SDK or due to network issues. Wrapping the API calls in a try-catch block will ensure that any unforeseen errors are caught and handled appropriately.
Apply this diff to add exception handling:
+ try {
const upsertCustomerRes = await client.upsertCustomer({
// existing code...
});
if (upsertCustomerRes.error) {
console.error("Failed to upsert customer in Plain", upsertCustomerRes.error);
+ return;
}
const createThreadRes = await client.createThread({
// existing code...
});
if (createThreadRes.error) {
console.error("Failed to create thread in Plain", createThreadRes.error);
+ return;
}
+ } catch (error) {
+ console.error("An unexpected error occurred in sendToPlain", error);
+ throw error;
}📝 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 sendToPlain({ userId, email, name, title, components }: Input) { | |
| if (!env.PLAIN_API_KEY) { | |
| return; | |
| } | |
| const client = new PlainClient({ | |
| apiKey: env.PLAIN_API_KEY, | |
| }); | |
| const upsertCustomerRes = await client.upsertCustomer({ | |
| identifier: { | |
| emailAddress: email, | |
| }, | |
| onCreate: { | |
| externalId: userId, | |
| fullName: name, | |
| email: { | |
| email: email, | |
| isVerified: true, | |
| }, | |
| }, | |
| onUpdate: { | |
| externalId: { value: userId }, | |
| fullName: { value: name }, | |
| email: { | |
| email: email, | |
| isVerified: true, | |
| }, | |
| }, | |
| }); | |
| if (upsertCustomerRes.error) { | |
| console.error("Failed to upsert customer in Plain", upsertCustomerRes.error); | |
| return; | |
| } | |
| const createThreadRes = await client.createThread({ | |
| customerIdentifier: { | |
| customerId: upsertCustomerRes.data.customer.id, | |
| }, | |
| title: title, | |
| components: components, | |
| }); | |
| if (createThreadRes.error) { | |
| console.error("Failed to create thread in Plain", createThreadRes.error); | |
| } | |
| } | |
| export async function sendToPlain({ userId, email, name, title, components }: Input) { | |
| if (!env.PLAIN_API_KEY) { | |
| return; | |
| } | |
| const client = new PlainClient({ | |
| apiKey: env.PLAIN_API_KEY, | |
| }); | |
| try { | |
| const upsertCustomerRes = await client.upsertCustomer({ | |
| identifier: { | |
| emailAddress: email, | |
| }, | |
| onCreate: { | |
| externalId: userId, | |
| fullName: name, | |
| email: { | |
| email: email, | |
| isVerified: true, | |
| }, | |
| }, | |
| onUpdate: { | |
| externalId: { value: userId }, | |
| fullName: { value: name }, | |
| email: { | |
| email: email, | |
| isVerified: true, | |
| }, | |
| }, | |
| }); | |
| if (upsertCustomerRes.error) { | |
| console.error("Failed to upsert customer in Plain", upsertCustomerRes.error); | |
| return; | |
| } | |
| const createThreadRes = await client.createThread({ | |
| customerIdentifier: { | |
| customerId: upsertCustomerRes.data.customer.id, | |
| }, | |
| title: title, | |
| components: components, | |
| }); | |
| if (createThreadRes.error) { | |
| console.error("Failed to create thread in Plain", createThreadRes.error); | |
| return; | |
| } | |
| } catch (error) { | |
| console.error("An unexpected error occurred in sendToPlain", error); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (1)
Line range hint
92-94: Unnecessary reassignment oftabvariableIn the
SpanBodycomponent, thetabvariable is reassigned from"context"to"overview", but there is no"context"tab in this component. This could lead to confusion or unintended behavior. Consider removing this conditional check since"context"is not a valid tab here.Apply this change to remove the unnecessary reassignment:
- if (tab === "context") { - tab = "overview"; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.v3.$projectParam.runs.$runParam.spans.$spanParam/route.tsx (1 hunks)
🧰 Additional context used
| @@ -314,7 +314,7 @@ function SpanBody({ | |||
| <Property.Table> | |||
| <Property.Item> | |||
| <Property.Label>Message</Property.Label> | |||
| <Property.Value>{span.message}</Property.Value> | |||
| <Property.Value className="whitespace-pre-wrap">{span.message}</Property.Value> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle long messages to prevent layout issues
Adding className="whitespace-pre-wrap" preserves whitespace and wraps text for better readability of span.message. However, if span.message contains excessively long text, it may impact the layout or overflow the container. Consider limiting the maximum height and adding overflow handling to maintain a consistent UI.
You might apply the following change:
- <Property.Value className="whitespace-pre-wrap">{span.message}</Property.Value>
+ <Property.Value className="whitespace-pre-wrap max-h-64 overflow-auto">{span.message}</Property.Value>This will limit the message display to a maximum height and allow scrolling if the content exceeds this height.
📝 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.
| <Property.Value className="whitespace-pre-wrap">{span.message}</Property.Value> | |
| <Property.Value className="whitespace-pre-wrap max-h-64 overflow-auto">{span.message}</Property.Value> |
| const userId = await requireUserId(request); | ||
| const user = await requireUser(request); | ||
| const presenter = new NewOrganizationPresenter(); | ||
| const { hasOrganizations } = await presenter.call({ userId }); | ||
| const { hasOrganizations } = await presenter.call({ userId: user.id }); |
There was a problem hiding this comment.
This one could have stayed as requireUserId. The requireUser call does a database call. It's not a big deal because this is such an infrequently viewed page though.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/webapp/app/routes/resources.feedback.ts (1)
87-88: Consider adding error logging for better debugging.The error handling has been simplified, which is good. However, consider adding error logging before setting the error message on the submission object. This would help with debugging and monitoring of issues in production.
Here's a suggested improvement:
} catch (e) { + console.error("Error in feedback submission:", e); submission.error.message = e instanceof Error ? e.message : "Unknown error"; return json(submission); }apps/webapp/app/routes/_app.orgs.new/route.tsx (1)
Line range hint
47-91: LGTM with suggestion: Action function updatesThe changes to the action function look good overall:
- Using
requireUserinstead ofrequireUserIdprovides more user information for the Plain integration.- The new code block correctly handles the optional
whyUseUsfield and sends the data to Plain.- Error handling is properly implemented for the Plain integration.
However, there's one improvement that can be made:
Instead of using
formData.get("whyUseUs"), use the validated data from the schema parsing. This ensures that the data has been properly validated before use. Apply this change:- const whyUseUs = formData.get("whyUseUs"); + const { whyUseUs } = submission.value; if (whyUseUs) { try { await sendToPlain({ // ... (rest of the code remains the same) }); } catch (error) { logger.error("Error sending data to Plain when creating an org:", { error }); } }This change ensures that you're using the validated data from the schema parsing, maintaining data integrity and consistency with the defined schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/webapp/app/routes/_app.orgs.new/route.tsx (4 hunks)
- apps/webapp/app/routes/resources.feedback.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (8)
apps/webapp/app/routes/resources.feedback.ts (3)
9-9: LGTM: Good abstraction of API interaction.The import of
sendToPlainfrom a separate utility file is a good practice. It centralizes the API interaction logic and makes the code more modular and maintainable.
36-36: Improved feedback quality with increased minimum length.Changing the minimum message length from 1 to 10 characters is a good improvement. It encourages users to provide more detailed and meaningful feedback, which can be more actionable for the team.
49-54: Improved code structure with abstracted API interaction.The use of
sendToPlainfunction instead of direct API interactions is a good refactoring. It simplifies the action function, improves readability, and centralizes the API interaction logic. The function is called with all necessary user and feedback details, maintaining the required functionality.apps/webapp/app/routes/_app.orgs.new/route.tsx (5)
7-7: LGTM: New import for Plain SDKThe new import for
uiComponentfrom the Plain SDK is correctly added and will be used for structuring the feedback data sent to Plain.
21-21: LGTM: New import for TextArea componentThe import for the
TextAreacomponent is correctly added and will be used for the new feedback input field in the form.
25-28: LGTM: Updated imports for new functionalityThe new imports for
logger,requireUser, andsendToPlainare correctly added and will be used for the new functionality of sending feedback to Plain and error logging. The replacement ofrequireUserIdwithrequireUseris consistent with the changes in the action function.
33-33: LGTM: New schema field for user feedbackThe
whyUseUsfield is correctly added to the schema as an optional string. This change aligns with the new feedback input field in the form.
Line range hint
1-194: Overall assessment: Well-implemented feature with minor suggestionsThe changes in this file successfully implement a new feature for collecting user feedback during organization creation. The implementation includes proper error handling, integration with the Plain service, and appropriate UI updates. The code is generally well-structured and follows good practices.
Two minor suggestions were made to improve data validation and error handling:
- Use the validated data from the schema parsing in the action function.
- Add error handling for the new
whyUseUsfield in the form.These suggestions will further enhance the robustness and consistency of the implementation. Overall, great job on this feature implementation!
| <> | ||
| <InputGroup> | ||
| <Label htmlFor={"companySize"}>Number of employees</Label> | ||
| <RadioGroup name="companySize" className="flex items-center justify-between gap-2"> | ||
| <RadioGroupItem | ||
| id="employees-1-5" | ||
| label="1-5" | ||
| value={"1-5"} | ||
| variant="button/small" | ||
| className="grow" | ||
| /> | ||
| <RadioGroupItem | ||
| id="employees-6-49" | ||
| label="6-49" | ||
| value={"6-49"} | ||
| variant="button/small" | ||
| className="grow" | ||
| /> | ||
| <RadioGroupItem | ||
| id="employees-50-99" | ||
| label="50-99" | ||
| value={"50-99"} | ||
| variant="button/small" | ||
| className="grow" | ||
| /> | ||
| <RadioGroupItem | ||
| id="employees-100+" | ||
| label="100+" | ||
| value={"100+"} | ||
| variant="button/small" | ||
| className="grow" | ||
| /> | ||
| </RadioGroup> | ||
| </InputGroup> | ||
| <InputGroup> | ||
| <Label htmlFor={"whyUseUs"}>What problem are you trying to solve?</Label> | ||
| <TextArea name="whyUseUs" rows={4} spellCheck={false} /> | ||
| <Hint> | ||
| Your answer will help us understand your use case and provide better support. | ||
| </Hint> | ||
| </InputGroup> | ||
| </> |
There was a problem hiding this comment.
LGTM with suggestion: Form updates for new feedback field
The changes to the form look good:
- The new TextArea component for the
whyUseUsfield is correctly implemented. - The layout structure using a fragment to group the
companySizeandwhyUseUsinputs is appropriate. - The hint text provides clear guidance to the user.
However, there's one improvement that can be made:
Add error handling for the whyUseUs field to be consistent with other form fields. Apply this change:
<InputGroup>
<Label htmlFor={"whyUseUs"}>What problem are you trying to solve?</Label>
- <TextArea name="whyUseUs" rows={4} spellCheck={false} />
+ <TextArea {...conform.textarea(whyUseUs)} rows={4} spellCheck={false} />
<Hint>
Your answer will help us understand your use case and provide better support.
</Hint>
+ <FormError id={whyUseUs.errorId}>{whyUseUs.error}</FormError>
</InputGroup>Also, update the form hook to include whyUseUs:
- const [form, { orgName }] = useForm({
+ const [form, { orgName, whyUseUs }] = useForm({
// ... (rest of the code remains the same)
});These changes will ensure that any validation errors for the whyUseUs field are properly displayed to the user, maintaining consistency with other form fields.
📝 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.
| <> | |
| <InputGroup> | |
| <Label htmlFor={"companySize"}>Number of employees</Label> | |
| <RadioGroup name="companySize" className="flex items-center justify-between gap-2"> | |
| <RadioGroupItem | |
| id="employees-1-5" | |
| label="1-5" | |
| value={"1-5"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-6-49" | |
| label="6-49" | |
| value={"6-49"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-50-99" | |
| label="50-99" | |
| value={"50-99"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-100+" | |
| label="100+" | |
| value={"100+"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| </RadioGroup> | |
| </InputGroup> | |
| <InputGroup> | |
| <Label htmlFor={"whyUseUs"}>What problem are you trying to solve?</Label> | |
| <TextArea name="whyUseUs" rows={4} spellCheck={false} /> | |
| <Hint> | |
| Your answer will help us understand your use case and provide better support. | |
| </Hint> | |
| </InputGroup> | |
| </> | |
| <> | |
| <InputGroup> | |
| <Label htmlFor={"companySize"}>Number of employees</Label> | |
| <RadioGroup name="companySize" className="flex items-center justify-between gap-2"> | |
| <RadioGroupItem | |
| id="employees-1-5" | |
| label="1-5" | |
| value={"1-5"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-6-49" | |
| label="6-49" | |
| value={"6-49"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-50-99" | |
| label="50-99" | |
| value={"50-99"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| <RadioGroupItem | |
| id="employees-100+" | |
| label="100+" | |
| value={"100+"} | |
| variant="button/small" | |
| className="grow" | |
| /> | |
| </RadioGroup> | |
| </InputGroup> | |
| <InputGroup> | |
| <Label htmlFor={"whyUseUs"}>What problem are you trying to solve?</Label> | |
| <TextArea {...conform.textarea(whyUseUs)} rows={4} spellCheck={false} /> | |
| <Hint> | |
| Your answer will help us understand your use case and provide better support. | |
| </Hint> | |
| <FormError id={whyUseUs.errorId}>{whyUseUs.error}</FormError> | |
| </InputGroup> | |
| </> |
* Updated “Twitter” to be “X (Twitter)” * added Textarea to storybook * Updated textarea styling to match input field * WIP adding new text field to org creation page * Added description to field * Submit feedback to Plain when an org signs up * Formatting improvement * type improvement * removed userId * Moved submitting to Plain into its own file * Change orgName with name * use sendToPlain function for the help & feedback email form * use name not orgName * import cleanup * Downgrading plan form uses sendToPlain * Get the userId from requireUser only * Added whitespace-pre-wrap to the message property on the run page * use requireUserId * Removed old Plain submit code
* refactor finalize run service * refactor complete attempt service * remove separate graceful exit handling * refactor task status helpers * clearly separate statuses in prisma schema * all non-final statuses should be failable * new import payload error code * store default retry config if none set on task * failed run service now respects retries * fix merged task retry config indexing * some errors should never be retried * finalize run service takes care of acks now * execution payload helper now with single object arg * internal error code enum export * unify failed and crashed run retries * Prevent uncaught socket ack exceptions (#1415) * catch all the remaining socket acks that could possibly throw * wrap the remaining handlers in try catch * New onboarding question (#1404) * Updated “Twitter” to be “X (Twitter)” * added Textarea to storybook * Updated textarea styling to match input field * WIP adding new text field to org creation page * Added description to field * Submit feedback to Plain when an org signs up * Formatting improvement * type improvement * removed userId * Moved submitting to Plain into its own file * Change orgName with name * use sendToPlain function for the help & feedback email form * use name not orgName * import cleanup * Downgrading plan form uses sendToPlain * Get the userId from requireUser only * Added whitespace-pre-wrap to the message property on the run page * use requireUserId * Removed old Plain submit code * Added a new Context page for the docs (#1416) * Added a new context page with task context properties * Removed code comments * Added more crosslinks * Fix updating many environment variables at once (#1413) * Move code example to the side menu * New docs example for creating a HN email summary * doc: add instructions to create new reference project and run it locally (#1417) * doc: add instructions to create new reference project and run it locally * doc: Add instruction for running tunnel * minor language improvement * Fix several restore and resume bugs (#1418) * try to correct resume messages with missing checkpoint * prevent creating checkpoints for outdated task waits * prevent creating checkpoints for outdated batch waits * use heartbeats to check for and clean up any leftover containers * lint * improve exec logging * improve resume attempt logs * fix for resuming parents of canceled child runs * separate SIGTERM from maybe OOM errors * pretty errors can have magic dashboard links * prevent uncancellable checkpoints * simplify task run error code enum export * grab the last, not the first child run * Revert "prevent creating checkpoints for outdated batch waits" This reverts commit f2b5c2a. * Revert "grab the last, not the first child run" This reverts commit 89ec5c8. * Revert "prevent creating checkpoints for outdated task waits" This reverts commit 11066b4. * more logs for resume message handling * add magic error link comment * add changeset * chore: Update version for release (#1410) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Release 3.0.13 * capture ffmpeg oom errors * respect maxAttempts=1 when failing before first attempt creation * request worker exit on fatal errors * fix error code merge * add new error code to should retry * pretty segfault errors * pretty internal errors for attempt spans * decrease oom false positives * fix timeline event color for failed runs * auto-retry packet import and export * add sdk version check and complete event while completing attempt * all internal errors become crashes by default * use pretty error helpers exclusively * error to debug log * zodfetch fixes * rename import payload to task input error * fix true non-zero exit error display * fix retry config parsing * correctly mark crashes as crashed * add changeset * remove non-zero exit comment * pretend we don't support default default retry configs yet --------- Co-authored-by: James Ritchie <[email protected]> Co-authored-by: shubham yadav <[email protected]> Co-authored-by: Tarun Pratap Singh <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Added a new onboarding question to learn more about why users are choosing us and what they're building.
The form now also submits this specific question to Plain.
Summary by CodeRabbit
New Features
TextAreacomponent for user feedback during organization creation.Storycomponent for theTextAreain the storybook.TextAreacomponent with new styling and hover effects.Bug Fixes
Chores