Gleam frontend to get up to date with typescript one. #47
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new TODO list in the Changes
Poem
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 review |
✅ Actions performedReview triggered.
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
frontend/TODO.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
frontend/TODO.md
[grammar] ~1-~1: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # FE-gleam todo list - [ ] Client-server communicati...
(TO_DO_HYPHEN)
[misspelling] ~11-~11: This expression is normally spelled as one or with a hyphen.
Context: ... ] Sub page loading - [ ] Loading sub pages (main, side) - [ ] Loading specia...
(EN_COMPOUNDS_SUB_PAGES)
[misspelling] ~12-~12: This expression is normally spelled as one or with a hyphen.
Context: ...main, side) - [ ] Loading special sub pages (editor, settings) - [ ] Sub page...
(EN_COMPOUNDS_SUB_PAGES)
🪛 Markdownlint
frontend/TODO.md
4-4: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
6-6: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
7-7: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
8-8: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
9-9: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
11-11: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
12-12: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
13-13: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
15-15: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
17-17: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
19-19: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
21-21: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
strawmelonjuice
changed the title
But I need to know stuff, and any is better than none.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 34
🧹 Outside diff range comments (3)
frontend-ts/app.test.ts (1)
Line range hint
4-6: Enhance test coverage with meaningful test casesThe current test appears to be a placeholder. Given this is
app.test.ts, it should contain meaningful tests for your application's core functionality.Would you like assistance in generating proper test cases for your frontend application? I can help create tests for:
- Component rendering
- State management
- User interactions
- Client-server communication
This would align better with the PR's objective of bringing the frontend up to date.
build.sh (1)
Line range hint
123-152: Consider extracting common build steps into functionsThe build and bundle steps are duplicated between TypeScript and Gleam frontends. Consider extracting these into functions to improve maintainability and reduce duplication.
Here's a suggested approach:
build_frontend() { local frontend_type="$1" local input_file="$2" local output_dir="$3" bun "$BUNFLAGS" build "$input_file" --minify --target=browser --outdir "$output_dir" --sourcemap=linked || { errnoti "Error while building the ${frontend_type} frontend with bun build." exit 1 } } bundle_frontend() { local frontend_type="$1" local js_file="$2" bun "$BUNFLAGS" "$LOCA/tobundle.ts" -- js-1 "$js_file" || { errnoti "Error while bundling the ${frontend_type} frontend with tobundle.ts." exit 1 } }These functions could then be used in both frontend build sections, reducing code duplication and making future maintenance easier.
🧰 Tools
🪛 Shellcheck (0.9.0-1)
[info] 121-121: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 123-123: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 127-127: Double quote to prevent globbing and word splitting.
(SC2086)
backend/src/lumina/web/routing/api_fe.gleam (1)
Line range hint
1-365: Consider standardising error handling patterns across endpointsThe error handling patterns vary across different endpoints. Some provide detailed error responses (like in
create_user), while others return basic status codes. Consider:
- Standardising error response formats
- Ensuring consistent use of error logging levels
- Adding detailed error messages where missing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (15)
.gitignore(2 hunks)backend/src/lumina/web/routing.gleam(1 hunks)backend/src/lumina/web/routing/api_fe.gleam(3 hunks)build.sh(2 hunks)frontend-ts/app.test.ts(1 hunks)frontend/TODO.md(1 hunks)frontend/src/elementactions_ffi.ts(1 hunks)frontend/src/fejson_ffi.ts(1 hunks)frontend/src/ffi.mjs(0 hunks)frontend/src/frontend.gleam(4 hunks)frontend/src/frontend/other/element_actions.gleam(1 hunks)frontend/src/frontend/other/fejson.gleam(1 hunks)frontend/src/frontend/other/formdata.gleam(1 hunks)frontend/src/frontend/page/login.gleam(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/ffi.mjs
🧰 Additional context used
🪛 Shellcheck (0.9.0-1)
build.sh
[info] 123-123: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 127-127: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 145-145: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 149-149: Double quote to prevent globbing and word splitting.
(SC2086)
🪛 LanguageTool
frontend/TODO.md
[misspelling] ~11-~11: This expression is normally spelled as one or with a hyphen.
Context: ... [ ] Sub page loading - [ ] Loading sub pages (main, side) - [ ] Loading special ...
(EN_COMPOUNDS_SUB_PAGES)
[misspelling] ~12-~12: This expression is normally spelled as one or with a hyphen.
Context: ... (main, side) - [ ] Loading special sub pages (editor, settings) - [ ] Sub page l...
(EN_COMPOUNDS_SUB_PAGES)
🪛 Markdownlint (0.35.0)
frontend/TODO.md
4-4: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
6-6: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
7-7: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
8-8: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
9-9: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
11-11: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
12-12: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
13-13: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
15-15: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
16-16: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
17-17: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
19-19: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
21-21: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (16)
.gitignore (1)
6-6: LGTM! Correctly ignoring the generated file.
The addition of frontend/prelude.mjs is appropriate as it's a generated file from the build process.
frontend/src/frontend/other/element_actions.gleam (1)
4-4: LGTM! Import statement is well-structured
The import statement correctly specifies the Element type from the plinth browser module.
frontend/TODO.md (5)
1-1: Skipping title format review
A previous review comment already addresses the title format standardisation.
17-17: Verify Markdown parsing dependencies
Check if a Markdown parsing library is specified in the project dependencies.
🧰 Tools
🪛 Markdownlint (0.35.0)
17-17: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
5-5: Verify FEJSON fetching implementation
The task for fetching FEJSON from server is marked as complete. Let's verify its implementation and error handling.
✅ Verification successful
FEJSON fetching implementation is properly implemented with error handling
The implementation is complete and well-structured with:
- Regular FEJSON updates every 30 seconds (
fejsontimeout = 30_000) - Proper error handling for both fetch failures and JSON decoding errors
- Comprehensive client-server synchronisation via
/api/fe/updateendpoint - Type-safe implementation using shared types between frontend and backend
- Proper state management through window object with TypeScript typings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FEJSON fetching implementation
ast-grep --pattern 'fetch$_($$$)' | grep -i 'fejson'
# Look for error handling patterns
rg -i 'fejson.*error|error.*fejson'
Length of output: 90
Script:
#!/bin/bash
# Search for FEJSON-related code with broader patterns
rg -i "fejson" -A 3
# Search for any fetch-related implementations
ast-grep --pattern 'const $_ = fetch($$$)'
ast-grep --pattern 'await fetch($$$)'
# Look for API endpoints or routes handling FEJSON
rg -i "api.*fejson|fejson.*api"
Length of output: 12541
🧰 Tools
🪛 Markdownlint (0.35.0)
5-5: Expected: 4; Actual: 8
Unordered list indentation
(MD007, ul-indent)
18-21: Verify security implementation for authentication
Ensure proper security measures are implemented for the authentication flow.
✅ Verification successful
Let me gather more information about the authentication implementation.
Security measures are properly implemented for authentication
Based on the codebase analysis, the authentication implementation includes essential security measures:
- Password hashing using the
arguslibrary with proper salt configuration - Secure session management via
wisp_kv_sessionswith configurable expiry - Password validation with minimum length requirement (8 characters)
- Protection against session hijacking with proper session deletion on logout
- Encrypted storage of sensitive data using environment-configured encryption keys
Key security implementations found in:
backend/src/lumina/users.gleam: Password hashing and validationbackend/src/lumina/web/routing/fence.gleam: Session protection and authentication middlewarebackend/src/lumina/web/routing/api_fe.gleam: Secure authentication flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security-related implementations
rg -i 'hash|encrypt|salt|secure|csrf|xss'
# Look for authentication error handling
ast-grep --pattern 'catch($$$) {
$$$
}'
Length of output: 3462
Script:
#!/bin/bash
# Check for authentication-related files and implementations
rg -A 5 "login|signin|signup|authenticate" --type ts --type gleam
# Look for password validation patterns
ast-grep --pattern 'password$_'
# Check for session/token handling
rg -i "session|token|jwt" --type ts --type gleam
Length of output: 212
Script:
#!/bin/bash
# Check for authentication-related files and implementations without file type filter
rg -A 5 "login|signin|signup|authenticate"
# Look for password validation and session handling
rg -i "password|session|token|jwt"
# Find authentication-related files
fd -e ts -e gleam "auth|login|sign"
Length of output: 30900
🧰 Tools
🪛 Markdownlint (0.35.0)
19-19: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
21-21: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
10-13: Verify routing implementation for sub-pages
Ensure proper routing mechanisms are in place for handling different sub-page types.
✅ Verification successful
Routing implementation for sub-pages appears to be in place
The codebase shows a well-structured routing system for handling sub-pages:
- Backend has proper page routing via
routing.gleamwith API endpoints for page serving - Frontend implements page loading through TypeScript and Gleam modules
- Type definitions exist for page serve requests/responses
- Dedicated modules handle different page types (login, signup, site)
The TODO items are tracking implementation tasks for a planned enhancement to the existing system rather than indicating missing core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for routing-related code
rg -A 3 'route|path|navigation'
# Look for page loading implementations
ast-grep --pattern 'load$_($$$)' | grep -i 'page'
Length of output: 126982
Script:
#!/bin/bash
# Search for page loading and routing implementations
rg -A 3 'load.*page|render.*page|page.*load'
# Look for page-related functions and types
ast-grep --pattern 'fn $_(page|Page)$_($$$) {
$$$
}'
# Search for page-related imports and type definitions
rg -A 2 'type.*Page|import.*page'
Length of output: 2513
🧰 Tools
🪛 LanguageTool
[misspelling] ~11-~11: This expression is normally spelled as one or with a hyphen.
Context: ... [ ] Sub page loading - [ ] Loading sub pages (main, side) - [ ] Loading special ...
(EN_COMPOUNDS_SUB_PAGES)
[misspelling] ~12-~12: This expression is normally spelled as one or with a hyphen.
Context: ... (main, side) - [ ] Loading special sub pages (editor, settings) - [ ] Sub page l...
(EN_COMPOUNDS_SUB_PAGES)
🪛 Markdownlint (0.35.0)
11-11: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
12-12: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
13-13: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
frontend/src/frontend/other/fejson.gleam (3)
51-51: LGTM! The external function declaration is consistent
The function signature and external reference are properly aligned with the TypeScript migration.
48-48: Verify the external TypeScript file path
The function declaration looks correct, but let's ensure the TypeScript file exists at the specified location.
✅ Verification successful
External TypeScript reference is correct and properly implemented
The external function reference is correctly pointing to ../../fejson_ffi.ts from frontend/src/frontend/other/fejson.gleam. The TypeScript file exists at the expected location and contains the properly implemented getJsonObj() function with the correct type signature. The relative path resolution works as intended, and the module is appropriately imported by the necessary files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the fejson_ffi.ts file
# Expected: Find the getJsonObj function implementation
# Check if the file exists relative to this file's location
fd -t f "fejson_ffi.ts" frontend/src
# Verify the function implementation
ast-grep --pattern 'export function getJsonObj()'
Length of output: 115
Script:
#!/bin/bash
# Let's check the actual content of the fejson_ffi.ts file
cat frontend/src/fejson_ffi.ts
# Also verify if there are any other files importing this module
rg -l "fejson_ffi"
Length of output: 2117
54-56: Verify timestamp implementation for timezone consistency
The function looks correct, but ensure the TypeScript implementation handles timezones appropriately for consistent synchronisation.
✅ Verification successful
Timestamp implementation uses UTC milliseconds without timezone complexity
The implementation uses Date.now() which returns UTC milliseconds since Unix epoch. This is appropriate for synchronisation as it's timezone-agnostic and provides consistent values across different systems. No timezone-related issues are present in this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the timestamp implementation in the TypeScript file
# Expected: Find the dateToTimestamp function and check its implementation
ast-grep --pattern 'export function dateToTimestamp()'
Length of output: 56
Script:
#!/bin/bash
# Let's try to find the TypeScript file and its content first
fd "fejson_ffi.ts" --type f
# Then let's look for any timestamp-related functions
rg -l "dateToTimestamp|timestamp" --type ts
# Finally, let's examine the content of any found files
fd "fejson_ffi.ts" --type f --exec cat {}
Length of output: 2188
frontend/src/frontend/other/formdata.gleam (1)
1-6: Proper licensing and minimal imports - looks good!
The file includes appropriate licensing information and imports only the necessary modules.
package.json (1)
13-14: Excellent standardisation of frontend arguments!
The change to use --frontend=typescript and --frontend=gleam follows a more conventional argument format, making the interface more consistent and self-documenting.
frontend/src/fejson_ffi.ts (1)
9-29: Verify input sanitisation for user data
The setJsonObj function handles sensitive user data. Ensure proper sanitisation is performed before storing in window.fejson.
frontend/src/frontend.gleam (1)
145-146: Verify TypeScript implementation exists
The external function declaration references "./fejson_ffi.ts", but we should verify its implementation.
✅ Verification successful
Let me verify the implementation further with a different approach.
TypeScript implementation verified and correct
The getQueuedFejsonFunctions function is properly implemented in frontend/src/fejson_ffi.ts. The implementation correctly returns a List of queued functions, which matches the Gleam type signature List(fn() -> Nil). The function also handles the initialisation of the queue when undefined, ensuring type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the TypeScript implementation exists and contains the required function
fd "fejson_ffi.ts" frontend/src -x cat {} | grep -l "getQueuedFejsonFunctions"
Length of output: 95
Script:
#!/bin/bash
# First, let's find the exact file path
fd "fejson_ffi.ts" frontend/src
# Then let's examine its contents
fd "fejson_ffi.ts" frontend/src -x cat {}
Length of output: 2099
backend/src/lumina/web/routing/api_fe.gleam (1)
83-83: LGTM: Good addition of request logging
The addition of authentication request logging improves observability.
frontend/src/frontend/page/login.gleam (1)
51-54: Verify the element ID "Aaa1".
The element with ID "Aaa1" may not exist or could be a placeholder. Please confirm that this is the correct element ID intended.
Run the following script to check if the element ID "Aaa1" is used elsewhere in the codebase:
✅ Verification successful
Based on the search results, I can see that the element ID "Aaa1" is consistently used across multiple files in the codebase:
- In the login page (
frontend/src/frontend/page/login.gleam), it's used to display login status messages - In the signup page (
backend/src/lumina/web/pages.gleam), it's used for account-related messages - In the frontend TypeScript code (
frontend-ts/signup.tsandfrontend-ts/login.ts), it's referenced for updating status messages
The element ID appears to be intentionally used as a designated element for displaying authentication-related status messages to users. Therefore:
The element ID "Aaa1" is correctly used and verified.
This element serves as a status message container across the authentication flow (login and signup pages) of the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for occurrences of the ID "Aaa1" in the codebase.
# Test: Find where "Aaa1" is used. Expect: Should find matching elements if it exists.
rg 'id="Aaa1"'
Length of output: 16
Script:
#!/bin/bash
# Search for any HTML/template files that might contain element definitions
fd -e html -e svelte -e eex -e heex -e jsx -e tsx
# Also search for any string containing "Aaa1" in case it's defined differently
rg "Aaa1"
# Search in Gleam files specifically as this is a Gleam project
fd -e gleam --exec cat {} \;
Length of output: 124628
|
Okay, I'm bringing the rust backend back, it's way more complete. Also might be more future proof |
|
@coderabbitai summarize |
|
Tip For best results, initiate chat on the files or code changes. @strawmelonjuice Here’s a concise summary of the changes introduced by this pull request:
Overall, the PR systematically enhances the Gleam frontend to bring it up to speed with the TypeScript counterpart, while adding improved error handling, logging, and build script robustness. |
The Gleam frontend is meant to compete and eventually replace the Typescript frontend, however, this isn't happening yet, mainly because the Gleam frontend lacks many many features. This PR is to bring it up to speed.
Summary by CodeRabbit
New Features
Bug Fixes
Chores