Initial version of pages #21
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces extensive modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 53.25% 54.31% +1.06%
==========================================
Files 15 15
Lines 1707 1751 +44
==========================================
+ Hits 909 951 +42
- Misses 709 711 +2
Partials 89 89
|
![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 (18)
testData/kindeSrc/environment/pages/(auth)/loginPage/page.tsx (2)
1-1: Consider adding type safety for server actionsWhile the "use server" directive is correctly placed, consider enhancing type safety by defining proper types for server actions.
+"use server" + +import { z } from 'zod'; + +export type ServerActionResponse = { + success: boolean; + data?: any; + error?: string; +};
1-9: Review the architectural approach to authentication pagesA few architectural concerns to consider:
- This file is in a
testDatadirectory but contains actual implementation code- The authentication implementation seems too simplistic for production use
- Consider integrating with Kinde's authentication SDK for proper security
Consider:
- Moving the implementation to a proper source directory if this isn't meant to be test data
- Implementing proper authentication flows using Kinde's SDK
- Adding proper session management and security headers
- Following authentication best practices (OWASP guidelines)
projectBundler/projectBundler_test.go (2)
12-12: Improve comment clarity for path selectionThe current comment could be more descriptive about the purpose of this path. Consider clarifying that this path points to the environment directory containing both workflows and pages.
- somePathInsideProject, _ := filepath.Abs("../testData/kindeSrc/environment") //starting in a middle of nowhere, so we need to go up to the root of the project + somePathInsideProject, _ := filepath.Abs("../testData/kindeSrc/environment") // Path to environment directory containing both workflows and pages
38-40: Enhance page testing coverageThe current assertions only verify the number of pages and empty error bundles. Consider adding more comprehensive tests:
- Extract the magic number 2 into a constant with meaningful name
- Add assertions for actual page properties and content
+ const expectedPageCount = 2 // Number of test pages in ../testData/kindeSrc/environment - assert.Equal(2, len(kindeProject.Environment.Pages)) + assert.Equal(expectedPageCount, len(kindeProject.Environment.Pages)) assert.Empty(kindeProject.Environment.Pages[0].Bundle.Errors) assert.Empty(kindeProject.Environment.Pages[1].Bundle.Errors) + // Add assertions for page properties + for _, page := range kindeProject.Environment.Pages { + assert.NotEmpty(page.ID, "Page should have an ID") + assert.NotEmpty(page.Path, "Page should have a path") + // Add more specific assertions based on expected page structure + }workflowBundler/workflowBundler_test.go (3)
23-28: Consider improving the validation logic in the testThe current implementation unconditionally adds an error message in the
OnDiscoveredcallback, which doesn't accurately test the validation logic. Consider:
- Making the error conditional based on the actual ID value
- Using a constant for the error message to maintain consistency
- Moving the validation logic to the actual implementation if it's a business requirement
+const errIDRequired = "ID is required" workflowBuilder := NewWorkflowBundler[workflowSettings](BundlerOptions[workflowSettings]{ WorkingFolder: workflowPath, EntryPoints: []string{"tokensWorkflow.ts"}, OnDiscovered: func(bundle *BundlerResult[workflowSettings]) { - bundle.Errors = append(bundle.Errors, "ID is required") + if bundle.Content.Settings.Additional.ID == "" { + bundle.Errors = append(bundle.Errors, errIDRequired) + } }, })
42-45: Improve test maintainability with constants and documentationThe test assertions use magic strings that should be defined as constants to improve maintainability and make the expected values more explicit.
+const ( + expectedID = "tokenGen" + expectedTrigger = "onTokenGeneration" +) -assert.Equal("tokenGen", bundlerResult.Content.Settings.Additional.ID) -assert.Equal("onTokenGeneration", bundlerResult.Content.Settings.Additional.Trigger) +assert.Equal(expectedID, bundlerResult.Content.Settings.Additional.ID, "workflow ID should match") +assert.Equal(expectedTrigger, bundlerResult.Content.Settings.Additional.Trigger, "workflow trigger should match")
Line range hint
13-46: Consider adding more test cases for better coverageThe current test focuses on the happy path. Consider adding test cases for:
- Invalid settings (empty trigger, malformed ID)
- Bundling failures (invalid entry points, plugin errors)
- Edge cases with the generic type parameter
Example additional test:
func Test_WorkflowBundlerErrors(t *testing.T) { assert := assert.New(t) // Test with invalid entry point workflowBuilder := NewWorkflowBundler[workflowSettings](BundlerOptions[workflowSettings]{ WorkingFolder: workflowPath, EntryPoints: []string{"nonexistent.ts"}, }) result := workflowBuilder.Bundle(context.Background()) assert.NotEmpty(result.Errors, "should contain bundling errors") }runtime_test.go (2)
24-30: Add documentation for new types and clarify empty struct usage.The new types lack documentation explaining their purpose and usage. Additionally,
pageSettingsis an empty struct which might indicate incomplete implementation.Consider adding documentation and either:
- Add a comment explaining why
pageSettingsis empty, or- Remove it if it's not needed yet and add it when required.
+// workflowSettings contains configuration for workflow execution type workflowSettings struct { ID string `json:"id"` } +// pageSettings contains configuration for page handling +// TODO: Implement required settings for pages type pageSettings struct { }
164-164: LGTM: Function signature correctly updated for generic types.The function signature properly incorporates the generic type parameter for workflows. Consider adding error handling for type assertions in the test to make it more robust.
func testExecution(workflow projectBundler.KindeWorkflow[workflowSettings], assert *assert.Assertions) func(t *testing.T) { return func(t *testing.T) { runner := getGojaRunner() logger := testLogger{} workflowRuncontext := context.WithValue(context.Background(), testContextValue, "test") result, err := runner.Execute(workflowRuncontext, registry.WorkflowDescriptor{ + // ... existing code ... }) if !assert.Nil(err) { t.FailNow() } - assert.Equal("testing return", fmt.Sprintf("%v", result.GetExitResult())) + exitResult := result.GetExitResult() + assert.NotNil(exitResult, "Exit result should not be nil") + assert.Equal("testing return", fmt.Sprintf("%v", exitResult)) idTokenMap, err := result.GetContext().GetValueAsMap("idToken") - assert.Nil(err) + if !assert.Nil(err, "Failed to get idToken map") { + t.FailNow() + } + assert.NotNil(idTokenMap, "idToken map should not be nil") assert.Equal("test", idTokenMap["random"])workflowBundler/workflowBundler.go (5)
19-21: Assess the necessity of introducing generics inWorkflowSettings.The addition of generics with
WorkflowSettings[TSettings any]increases complexity. While it provides flexibility forAdditionalsettings, evaluate if this complexity is justified or if simpler alternatives could suffice for maintainability and readability.
Line range hint
77-125: Ensure build errors are handled before processing output files.In the
Bundlemethod, the code proceeds to processtr.OutputFileseven iftr.Errorscontains errors. This could lead to unexpected behavior if the build fails but outputs are still processed. It's critical to check for build errors before handling the output files.Apply this diff to handle build errors appropriately:
func (b *builder[TSettings]) Bundle(ctx context.Context) BundlerResult[TSettings] { opts := api.BuildOptions{ // existing options... } tr := api.Build(opts) + result := BundlerResult[TSettings]{} + if len(tr.Errors) > 0 { + for _, buildError := range tr.Errors { + result.addCompilationError(buildError) + } + if b.bundleOptions.OnDiscovered != nil { + b.bundleOptions.OnDiscovered(&result) + } + return result + } - result := BundlerResult[TSettings]{} if len(tr.OutputFiles) > 0 { // existing code to process output files... } - for _, buildError := range tr.Errors { - result.addCompilationError(buildError) - } if b.bundleOptions.OnDiscovered != nil { b.bundleOptions.OnDiscovered(&result) } return result }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-134: workflowBundler/workflowBundler.go#L134
Added line #L134 was not covered by tests
[warning] 138-138: workflowBundler/workflowBundler.go#L138
Added line #L138 was not covered by tests
[warning] 142-142: workflowBundler/workflowBundler.go#L142
Added line #L142 was not covered by tests
134-134: Increase test coverage for theHasOutputmethod.The
HasOutputmethod is currently not covered by unit tests. Adding tests for this method will help ensure its correctness and prevent future regressions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-134: workflowBundler/workflowBundler.go#L134
Added line #L134 was not covered by tests
175-194: Improve error handling inUnmarshalJSON.In the
UnmarshalJSONmethod, errors fromjson.Unmarshalare returned directly without additional context. Enhancing the error messages will aid in debugging.Apply this diff to add context to the errors:
func (settings *WorkflowSettings[TSettings]) UnmarshalJSON(data []byte) error { type bindings struct { Bindings map[string]runtimesRegistry.BindingSettings `json:"bindings"` } mapData := bindings{} err := json.Unmarshal(data, &mapData) if err != nil { - return err + return fmt.Errorf("failed to unmarshal bindings: %w", err) } settings.Bindings = mapData.Bindings set := new(TSettings) err = json.Unmarshal(data, set) if err != nil { - return err + return fmt.Errorf("failed to unmarshal additional settings: %w", err) } settings.Additional = *set return nil } +// Make sure to import "fmt" import ( "context" "encoding/json" + "fmt" "errors" "time" )🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 184-184: workflowBundler/workflowBundler.go#L184
Added line #L184 was not covered by tests
184-184: Add unit tests to cover error scenarios inUnmarshalJSON.The error path in
UnmarshalJSON, particularly whenjson.Unmarshalfails, is not covered by tests. Introduce unit tests with invalid JSON data to ensure that errors are handled gracefully and as expected.Would you like assistance in creating these unit tests?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 184-184: workflowBundler/workflowBundler.go#L184
Added line #L184 was not covered by testsprojectBundler/projectBundler.go (4)
97-98: Return early if the pages folder does not exist.In
discoverPages, after logging the warning about the missing pages folder, the function continues execution. To prevent potential errors or unnecessary processing, consider returning early, similar todiscoverWorkflows.Apply this diff to return early when the pages folder is missing:
log.Warn().Msgf("could not find pages folder: %s", pagesPath) + return🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-98: projectBundler/projectBundler.go#L97-L98
Added lines #L97 - L98 were not covered by tests
112-124: Rename variables for consistency and clarity inmaybeAddPagefunction.The parameter
kwrepresents aKindeEnvironment, so renaming it tokeimproves readability and aligns with its type. Also, update references within the function accordingly.Apply this diff to rename the parameter and variable references:
-func maybeAddPage[TWorkflowSettings, TPageSettings any](ctx context.Context, file string, rootDirectory string, kw *KindeEnvironment[TWorkflowSettings, TPageSettings]) { +func maybeAddPage[TWorkflowSettings, TPageSettings any](ctx context.Context, file string, rootDirectory string, ke *KindeEnvironment[TWorkflowSettings, TPageSettings]) { fileName := strings.ToLower(file) if strings.HasSuffix(fileName, "page.ts") || strings.HasSuffix(fileName, "page.js") || strings.HasSuffix(fileName, "page.tsx") || strings.HasSuffix(fileName, "page.jsx") { discoveredPage := KindePage[TPageSettings]{ RootDirectory: rootDirectory, EntryPoints: []string{file}, } discoveredPage.bundleAndIntrospect(ctx) - kw.Pages = append(kw.Pages, discoveredPage) + ke.Pages = append(ke.Pages, discoveredPage) } }
93-110: Rename receiver indiscoverPagesmethod for consistency.The receiver
kwin thediscoverPagesmethod refers toKindeEnvironment. Renaming it tokeenhances clarity and maintains consistency with variable naming conventions.Apply this diff to rename the receiver:
-func (kw *KindeEnvironment[TWorkflowSettings, TPageSettings]) discoverPages(ctx context.Context, absLocation string) { +func (ke *KindeEnvironment[TWorkflowSettings, TPageSettings]) discoverPages(ctx context.Context, absLocation string) { pagesPath := filepath.Join(absLocation, "environment", "pages") _, err := os.Stat(pagesPath) if err != nil { log.Warn().Msgf("could not find pages folder: %s", pagesPath) + return } filepath.Walk(pagesPath, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if !info.IsDir() { - maybeAddPage(ctx, info.Name(), filepath.Dir(path), kw) + maybeAddPage(ctx, info.Name(), filepath.Dir(path), ke) } return nil }) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-98: projectBundler/projectBundler.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 102-103: projectBundler/projectBundler.go#L102-L103
Added lines #L102 - L103 were not covered by tests
71-71: Increase test coverage for error handling paths.The error handling code at lines 71, 97-98, and 102-103 is not covered by tests. Adding tests for these scenarios will ensure that the code behaves correctly under error conditions.
Would you like assistance in creating unit tests for these error cases to improve coverage?
Also applies to: 97-98, 102-103
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: projectBundler/projectBundler.go#L71
Added line #L71 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
projectBundler/projectBundler.go(5 hunks)projectBundler/projectBundler_test.go(2 hunks)runtime_test.go(3 hunks)testData/kindeSrc/environment/pages/(auth)/loginPage/page.tsx(1 hunks)testData/kindeSrc/environment/pages/page.tsx(1 hunks)workflowBundler/workflowBundler.go(7 hunks)workflowBundler/workflowBundler_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- testData/kindeSrc/environment/pages/page.tsx
🧰 Additional context used
🪛 GitHub Check: codecov/patch
projectBundler/projectBundler.go
[warning] 71-71: projectBundler/projectBundler.go#L71
Added line #L71 was not covered by tests
[warning] 97-98: projectBundler/projectBundler.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 102-103: projectBundler/projectBundler.go#L102-L103
Added lines #L102 - L103 were not covered by tests
workflowBundler/workflowBundler.go
[warning] 134-134: workflowBundler/workflowBundler.go#L134
Added line #L134 was not covered by tests
[warning] 138-138: workflowBundler/workflowBundler.go#L138
Added line #L138 was not covered by tests
[warning] 142-142: workflowBundler/workflowBundler.go#L142
Added line #L142 was not covered by tests
[warning] 184-184: workflowBundler/workflowBundler.go#L184
Added line #L184 was not covered by tests
🔇 Additional comments (4)
testData/kindeSrc/environment/pages/(auth)/loginPage/page.tsx (1)
7-9:
Multiple improvements needed in request handler
The current implementation has several issues that need to be addressed:
- Using
anytype reduces type safety - Hardcoded HTML response isn't maintainable
- Missing error handling and input validation
- HTML response doesn't follow HTML5 standards
Consider implementing these improvements:
-export default function handleRequest(event: any) {
- return "<html><body><h1>Page</h1></body></html>";
+interface RequestEvent {
+ method: string;
+ headers: Record<string, string>;
+ // Add other relevant properties
+}
+
+export default async function handleRequest(event: RequestEvent) {
+ try {
+ // Input validation
+ if (!event?.method) {
+ throw new Error('Invalid request event');
+ }
+
+ // Return proper HTML5 document
+ return `
+ <!DOCTYPE html>
+ <html lang="en">
+ <head>
+ <meta charset="UTF-8">
+ <meta name="viewport" content="width=device-width, initial-scale=1.0">
+ <title>Login Page</title>
+ </head>
+ <body>
+ <h1>Page</h1>
+ </body>
+ </html>
+ `;
+ } catch (error) {
+ console.error('Request handling failed:', error);
+ throw error;
+ }
+}Let's verify if similar patterns exist in other pages:
projectBundler/projectBundler_test.go (1)
14-20: Verify if pageSettings is intentionally empty
The pageSettings struct is currently empty. If this is intentional for future extensibility, consider adding a comment explaining this. If not, it might need additional fields based on the pages discovery requirements.
✅ Verification successful
Empty pageSettings struct is intentionally used as a type parameter
The empty pageSettings struct is correctly implemented as it's being used as a type parameter in DiscoveryOptions[workflowSettings, pageSettings] for project bundling. The codebase shows that actual page settings are exported as objects in the TypeScript/Next.js pages (.tsx files), suggesting this is a type-only construct for discovery purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if pageSettings is used with fields in other files
ast-grep --pattern 'type pageSettings struct {
$$$
}'
# Check for any TODO comments related to pageSettings
rg -i "todo.*pagesettings"
Length of output: 266
Script:
#!/bin/bash
# Check for usages of pageSettings in test files and implementation
rg "pageSettings" -B 2 -A 2
# Check if there are any related page configuration or settings types
ast-grep --pattern 'type $_ struct {
$$$
Page $$$
$$$
}'
# Look for any test cases using pageSettings
rg "func.*Test.*Page" -A 5
Length of output: 1757
workflowBundler/workflowBundler_test.go (1)
14-17: LGTM: Well-structured settings type definition
The workflowSettings struct is properly defined with appropriate JSON tags and exported fields.
runtime_test.go (1)
140-142: LGTM: Generic type parameters correctly implemented.
The project bundler initialization properly uses the new generic types, maintaining type safety while allowing for future extensibility.
Initial version of pages discovery and bundling
🛟 If you need help, consider asking for advice over in the Kinde community.