Add "Where's my project?" link to FW Lite and corresponding page to Lexbox#1650
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a "Where's my project?" help feature to the FW Lite home. It adds a help section with a link to explanatory content, implements a GraphQL mutation and supporting backend/frontend logic for users to request access to the FW Lite beta, and provides new email templates, localized text, and conditional UI based on feature flags. Changes
Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
rmunn
force-pushed
the
feat/wheres-my-project
branch
from
d28b5c6 to
ec415b3
Compare
|
Have now rebased on top of |
|
Going to wait until #1652 is merged before continuing to work on this one, so that I can catch any other |
|
Rebasing on top of #1652 for now so I can continue to make progress on this PR. |
rmunn
force-pushed
the
feat/wheres-my-project
branch
from
0377194 to
2c40d60
Compare
Page content is placeholder for now
Email will contain a link to approve the request. Link doesn't yet do anything; that implementation is next.
|
Never mind that rebase, it makes this PR look unmanageable. There were no merge conflicts anyway so it's not a problem to be based off develop, it won't harm my ability to merge #1652 if this one gets merged first. |
rmunn
force-pushed
the
feat/wheres-my-project
branch
from
f22ed8a to
20387b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
frontend/src/lib/layout/FeatureFlagAlternateContent.svelte (2)
17-17: Consider providing context for the eslint-disable comment.While the eslint-disable is likely needed, it would be helpful to add a short explanation of why this rule needs to be disabled to help future developers understand the reasoning.
-<!-- eslint-disable-next-line @typescript-eslint/no-unsafe-argument --> +<!-- eslint-disable-next-line @typescript-eslint/no-unsafe-argument -- Snippet functions are properly typed but ESLint cannot verify this -->
14-14: Consider adding a default value for children to avoid potential null errors.Since
childrenis optional and used in line 19 with the null coalescing operator, you might want to provide a default empty function to avoid potential errors in case bothhasFlagContentandchildrenare undefined.-let { flag, missingFlagContent, hasFlagContent, children }: Props = $props(); +let { flag, missingFlagContent, hasFlagContent, children = () => '' }: Props = $props();frontend/src/routes/(authenticated)/wheresMyProject/+page.ts (2)
6-6: Remove underscore prefix from exported function.The underscore prefix (
_) conventionally indicates a private function, but this function is exported. Either keep it private (not exported) or remove the underscore.-export async function _sendFWLiteBetaRequestEmail(userId: UUID, name: string): $OpResult<SendFwLiteBetaRequestEmailMutation> { +export async function sendFWLiteBetaRequestEmail(userId: UUID, name: string): $OpResult<SendFwLiteBetaRequestEmailMutation> {
18-19: Fix indentation for consistency.The indentation of
__typenameis inconsistent with surrounding code.errors { - __typename + __typename ... on Error {frontend/src/routes/(authenticated)/wheresMyProject/+page.svelte (2)
9-9: Consider using project-specific UUID type definition.Importing the UUID type from 'crypto' module might not be necessary if the project already has a UUID type definition. Check if there's a project-specific type definition that could be used instead.
31-32: Remove empty style block.The empty style block doesn't serve any purpose and can be removed.
-<style> -</style>backend/LexBoxApi/GraphQL/UserMutations.cs (1)
53-53: Consider adding a more specific error message.For improved debugging and user experience, consider adding a more descriptive message when throwing the
UnauthorizedAccessException.- if (loggedInContext.User.Id != input.UserId) throw new UnauthorizedAccessException(); + if (loggedInContext.User.Id != input.UserId) throw new UnauthorizedAccessException("User can only request beta access for themselves");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/LexBoxApi/GraphQL/UserMutations.cs(2 hunks)backend/LexBoxApi/Services/Email/EmailTemplates.cs(2 hunks)backend/LexBoxApi/Services/Email/IEmailService.cs(1 hunks)backend/LexBoxApi/Services/EmailService.cs(1 hunks)frontend/schema.graphql(4 hunks)frontend/src/lib/email/JoinFWLiteBetaRequest.svelte(1 hunks)frontend/src/lib/i18n/locales/en.json(2 hunks)frontend/src/lib/layout/FeatureFlagAlternateContent.svelte(1 hunks)frontend/src/routes/(authenticated)/wheresMyProject/+page.svelte(1 hunks)frontend/src/routes/(authenticated)/wheresMyProject/+page.ts(1 hunks)frontend/src/routes/email/emails.ts(5 hunks)frontend/viewer/src/home/HomeView.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/LexBoxApi/Services/EmailService.cs (2)
backend/LexBoxApi/Services/Email/IEmailService.cs (5)
Task(10-10)Task(12-12)Task(22-22)Task(31-37)Task(46-52)backend/LexCore/Entities/User.cs (1)
User(5-57)
🪛 Biome (1.9.4)
frontend/src/lib/i18n/locales/en.json
[error] 671-671: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 671-671: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 673-674: Invalid escape sequence
Valid escape sequences are: \\, \/, /", \b\, \f, \n, \r, \t or any unicode escape sequence \uXXXX where X is hexedecimal number.
(parse)
[error] 673-674: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 674-674: expected , but instead found near
Remove near
(parse)
[error] 674-674: expected : but instead found the
Remove the
(parse)
[error] 674-674: expected , but instead found top
Remove top
(parse)
[error] 674-674: expected : but instead found of
Remove of
(parse)
[error] 674-674: expected , but instead found the
Remove the
(parse)
[error] 674-674: expected : but instead found page
Remove page
(parse)
[error] 674-674: unexpected character .
(parse)
[error] 674-675: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 675-675: expected , but instead found it
Remove it
(parse)
[error] 675-676: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 676-676: expected , but instead found "user_not_in_beta"
Remove "user_not_in_beta"
(parse)
[error] 676-677: Invalid escape sequence
Valid escape sequences are: \\, \/, /", \b\, \f, \n, \r, \t or any unicode escape sequence \uXXXX where X is hexedecimal number.
(parse)
[error] 676-677: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 677-677: expected , but instead found have
Remove have
(parse)
[error] 677-677: expected : but instead found chosen
Remove chosen
(parse)
[error] 677-677: expected , but instead found to
Remove to
(parse)
[error] 677-677: expected : but instead found be
Remove be
(parse)
[error] 677-677: expected , but instead found part
Remove part
(parse)
[error] 677-677: expected : but instead found of
Remove of
(parse)
[error] 677-677: expected , but instead found the
Remove the
(parse)
[error] 677-677: expected : but instead found beta
Remove beta
(parse)
[error] 677-677: unexpected character .
(parse)
[error] 677-677: Property key must be double quoted
(parse)
[error] 677-677: expected : but instead found click
Remove click
(parse)
[error] 677-677: expected , but instead found the
Remove the
(parse)
[error] 677-677: expected : but instead found button
Remove button
(parse)
[error] 677-677: expected , but instead found below
Remove below
(parse)
[error] 677-677: unexpected character \
(parse)
[error] 677-677: expected , but instead found n
Remove n
(parse)
[error] 677-678: unexpected character \
(parse)
[error] 678-678: expected , but instead found to
Remove to
(parse)
[error] 678-678: expected : but instead found ask
Remove ask
(parse)
[error] 678-678: expected , but instead found to
Remove to
(parse)
[error] 678-678: expected : but instead found join
Remove join
(parse)
[error] 678-678: expected , but instead found the
Remove the
(parse)
[error] 678-678: expected : but instead found beta
Remove beta
(parse)
[error] 678-678: expected , but instead found test
Remove test
(parse)
[error] 678-678: unexpected character .
(parse)
[error] 678-678: expected , but instead found Depending
Remove Depending
(parse)
[error] 678-678: expected : but instead found on
Remove on
(parse)
[error] 678-678: expected , but instead found what
Remove what
(parse)
[error] 678-678: expected : but instead found time
Remove time
(parse)
[error] 678-678: expected , but instead found zone
Remove zone
(parse)
[error] 678-678: expected : but instead found you
Remove you
(parse)
[error] 678-679: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 679-679: expected , but instead found to
Remove to
(parse)
[error] 679-679: expected : but instead found see
Remove see
(parse)
[error] 679-679: expected , but instead found your
Remove your
(parse)
[error] 679-679: expected : but instead found email
Remove email
(parse)
[error] 679-679: expected , but instead found and
Remove and
(parse)
[error] 679-679: expected : but instead found respond
Remove respond
(parse)
[error] 679-679: unexpected character .
(parse)
[error] 679-680: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 680-680: expected , but instead found "request_beta_access"
Remove "request_beta_access"
(parse)
[error] 681-681: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 671-681: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build API / publish-api
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (13)
frontend/src/lib/layout/FeatureFlagAlternateContent.svelte (1)
9-11: Clean implementation of conditional content rendering based on feature flags.This component provides a flexible way to conditionally render content based on user feature flags. The API with optional snippet props gives good flexibility to consumers.
Also applies to: 18-21
frontend/viewer/src/home/HomeView.svelte (1)
209-217: Good addition of help content for users.This section provides useful guidance to users who may not see their projects, improving the overall user experience.
backend/LexBoxApi/Services/Email/EmailTemplates.cs (1)
26-26: Proper extension of email templates.The new email template type follows the existing patterns in the file and is well integrated with the current structure.
Also applies to: 45-45
frontend/src/lib/email/JoinFWLiteBetaRequest.svelte (1)
8-8:Details
❓ Verification inconclusive
Consider security implications of email in query parameters.
Including email addresses in query parameters might expose sensitive user information in server logs, browser history, and referrer headers.
Consider using a POST form or a more secure approach for passing the email. If query parameters must be used, ensure server logs are appropriately configured to mask sensitive information.
🏁 Script executed:
#!/bin/bash # Check if there are other instances of email addresses in query parameters rg -g "*.ts" -g "*.svelte" "userEmail=\$\{encodeURIComponent\(" --no-heading # Check for any email masking in logging configuration rg -g "*.json" -g "*.config" "mask" --no-headingLength of output: 126
Ensure secure handling of userEmail in URLs
We only found this instance and no masking configurations in logs:
- Location: frontend/src/lib/email/JoinFWLiteBetaRequest.svelte (line 8)
let approveUrl = new URL( `/admin/approveFWBetaRequest?userEmail=${encodeURIComponent(email)}`, baseUrl );- No other
userEmail=${encodeURIComponent(occurrences in.ts/.sveltefiles.- No masking settings detected in
.jsonor.configfiles.Recommendations:
- Prefer sending sensitive data in the request body (POST) or via a session-bound token.
- If a query parameter is unavoidable:
- Implement server-side masking of email in logs.
- Review/refine referrer policies and logging configuration to strip or obfuscate sensitive query fields.
frontend/schema.graphql (4)
278-278: LGTM: Mutation correctly declares required parameters and return type.The
sendFWLiteBetaRequestEmailmutation is properly declared with the necessary input parameter and return payload. Good use of the@costdirective for query complexity management.
497-500: LGTM: Payload type follows schema conventions.The
SendFWLiteBetaRequestEmailPayloadtype properly includes themeDtoanderrorsfields, following the conventions established in other mutation payloads in the schema.
654-654: LGTM: Error union is correctly defined.The
SendFWLiteBetaRequestEmailErrorunion appropriately includes common error types that might occur during the email sending process. This aligns with the error handling approach seen in other mutations.
1105-1108: LGTM: Input type contains necessary fields.The
SendFWLiteBetaRequestEmailInputis properly defined with the requireduserIdandnamefields, which matches the backend implementation in UserMutations.cs.frontend/src/lib/i18n/locales/en.json (1)
750-754: LGTM: Email template localization strings look good.The localization strings for the beta join request email are properly formatted and provide all necessary text elements for the email template.
frontend/src/routes/(authenticated)/wheresMyProject/+page.svelte (2)
16-29: LGTM: Page structure and feature flag handling look good.The page correctly uses the
FeatureFlagAlternateContentcomponent to show different content based on whether the user has the "FwLiteBeta" feature flag. The layout follows established patterns with breadcrumb and title components.
25-25:Details
✅ Verification successful
Verify event handler naming convention.
The PR comments mentioned changes from
on:clicktoonclickdue to Svelte 5 migration. Confirm that usingonclick(rather than the Svelte-idiomaticon:click) is consistent with the project's current Svelte event handling conventions after the migration.
🏁 Script executed:
#!/bin/bash # Check for other button components in the codebase to verify convention echo "Checking Button component usage in the codebase..." fd ".svelte$" frontend/src -x grep -l "Button" {} \; | xargs grep -l "on[cC]lick" | head -n 5 | xargs grep -A 1 -B 1 "on[cC]lick"Length of output: 4422
Event handler naming convention verified. The codebase consistently uses
onclickin Svelte components following the Svelte 5 migration, including<Button>and native<button>elements. No changes needed here.backend/LexBoxApi/GraphQL/UserMutations.cs (2)
39-39: LGTM: Input record type is well-defined.The
SendFWLiteBetaRequestEmailInputrecord type correctly defines the required parameters for the mutation.
41-64: LGTM: Mutation implementation looks solid.The implementation follows the established patterns for mutations in this class:
- Proper authorization checks
- Good error handling with appropriate exception types
- Clean integration with the email service
- Correct return of user data in the MeDto
Serialization of C# enums was a little ambiguous when the name was FWLite, so FwLite removes all ambiguity re how the name will end up.
|
The "Where's my project" link in the FW Lite homepage is a placeholder in need of suggestions for how to actually style it for UI consistency. @myieye - Got any suggestions for how I should implement it? See commit 20387b4 for how I added it. EDIT: Forgot to mention that I'd had the idea of putting a "Where's my project?" link in only if the user is logged in (to whichever server) and no projects were returned. Currently I just have one link at the bottom of the page instead. |
|
I'm taking a look now |
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I touched up the button styles/positions:
If no projects:
Also notify user if they were already in the beta, in which case no email was sent.
|
@myieye - All comments addressed EXCEPT the wording of the email as I was running out of time for the day. Would you like to take a stab at that one, just to get it done quicker? Otherwise I'll work on it tomorrow. |
3 participants
Fixes #1628.