feat: login required functionality v1#9
Conversation
- Updated components to use TypeScript for better type safety. - Refined form field type definitions and imports in RenderField.vue and FieldRenderer.vue. - Added new properties to the Form interface for improved form management. - Cleaned up unused functions and ensured consistent formatting across files.
- Added `is_login_required` function to check if login is required for a specific form. - Updated router to enforce login checks before entering the form submission page. - Enhanced `FormBuilderHeader` to construct submission page URLs correctly. - Introduced utility function `isLoginRequired` for API calls related to login requirements.
📝 WalkthroughWalkthroughAdds backend endpoints for login checks and brand logo; introduces a session-aware PageHeader; tightens TypeScript typings for forms and form fields; implements per-route login guards and router changes; updates form settings UI to sync login_required with allow_incomplete; several frontend components migrated/typed. Changes
Sequence DiagramsequenceDiagram
participant User
participant Router as Router & Guards
participant Frontend as Frontend Page
participant API as Backend API
participant Session as Session Manager
User->>Router: Navigate to /p/form-route
Router->>API: request is_login_required(route)
API-->>Router: { login_required: true/false }
alt login_required && !Session.logged_in
Router->>User: Redirect to /login?redirect-to=/p/form-route
else
Router->>Frontend: allow navigation / render page
Frontend->>Session: check session state
alt Session.logged_in
Frontend-->>User: show avatar + logout option
User->>Frontend: click logout
Frontend->>Session: session.logout.submit()
else
Frontend-->>User: show Login button
User->>Frontend: click Login
Frontend->>User: redirect to /login?redirect-to=/p/form-route
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/router.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/utils/form_fields.ts (1)
147-195: Function can returnundefinedfor unknown field types.The return type is
string, but indexingFIELD_TYPE_MAPwith an unknown key returnsundefined. This could cause issues downstream when the mapped field type is used.🔎 Proposed fix with fallback
export const mapDoctypeFieldForForm = (fieldtype: string): string => { const FIELD_TYPE_MAP = { // ... existing mappings }; - return FIELD_TYPE_MAP[fieldtype as keyof typeof FIELD_TYPE_MAP]; + return FIELD_TYPE_MAP[fieldtype as keyof typeof FIELD_TYPE_MAP] ?? "Data"; };Alternatively, update the return type to
string | undefinedand handle it at call sites.
🧹 Nitpick comments (2)
frontend/src/components/builder/FieldRenderer.vue (1)
112-112: Replaceanytype with proper type inference.The explicit
anytype annotation bypasses TypeScript's type checking. Consider either removing the type annotation to allow TypeScript to infer the type, or using a more specific type based on the field data structure.🔎 Suggested improvement
- @change="(value: any) => (modelValue = value)" + @change="(value) => (modelValue = value)"Or define a proper type if the inferred type is too broad.
frontend/src/components/builder/sidebar/SettingsSection.vue (1)
71-98: Consider UX implications of automatic restoration.When
allow_incompleteis toggled from false → true → false,login_requiredis restored tooriginalFormData.login_required. This means any manual changes tologin_requiredmade whileallow_incompletewas false will be lost. This might be unexpected for users who adjustlogin_requiredbefore enabling incomplete forms.Suggested behavior: Consider preserving the user's manual changes to
login_requiredwhen they were made beforeallow_incompletewas enabled.🔎 Alternative implementation
+// Track the login_required state before allow_incomplete was enabled +const loginRequiredBeforeIncomplete = ref(null); watch( () => editFormStore.formData.allow_incomplete, (newVal) => { if (newVal) { + // Save current state before forcing true + loginRequiredBeforeIncomplete.value = editFormStore.formData.login_required; editFormStore.formData.login_required = true; } if (!newVal) { - editFormStore.formData.login_required = editFormStore.originalFormData.login_required; + // Restore to the state before incomplete was enabled, if available + if (loginRequiredBeforeIncomplete.value !== null) { + editFormStore.formData.login_required = loginRequiredBeforeIncomplete.value; + loginRequiredBeforeIncomplete.value = null; + } } } );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
forms_pro/api/form.pyforms_pro/api/settings.pyfrontend/components.d.tsfrontend/src/components/FormBuilderHeader.vuefrontend/src/components/RenderField.vuefrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/builder/sidebar/SettingsSection.vuefrontend/src/components/submission/FormHeader.vuefrontend/src/components/submission/PageHeader.vuefrontend/src/pages/SubmissionPage.vuefrontend/src/router.tsfrontend/src/stores/editForm.tsfrontend/src/types/form.tsfrontend/src/utils/form.tsfrontend/src/utils/form_fields.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/types/form.ts (1)
frontend/src/types/formfield.ts (1)
FormField(1-10)
frontend/src/router.ts (2)
frontend/src/utils/form.ts (1)
isLoginRequired(10-20)frontend/src/data/session.ts (1)
session(32-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Frappe Linter
- GitHub Check: Server
🔇 Additional comments (12)
frontend/src/components/builder/FieldRenderer.vue (1)
1-1: LGTM! TypeScript migration improves type safety.The migration to TypeScript aligns with the broader typing improvements across the project.
frontend/components.d.ts (1)
25-25: LGTM! Component declaration added correctly.The
PageHeadercomponent is now properly registered in the global component types, enabling TypeScript autocomplete and type checking.frontend/src/components/builder/sidebar/SettingsSection.vue (1)
21-31: LGTM! Watcher correctly synchronizes login and incomplete form settings.The watcher ensures that when incomplete forms are allowed, login is required, which is a sensible security constraint. The restoration to
originalFormData.login_requiredwhen incomplete forms are disabled maintains the original setting.forms_pro/api/settings.py (1)
5-13: LGTM! Guest access is appropriate for public form branding.The function correctly allows guest access for retrieving the brand logo, which is necessary for public form submission pages. The
str()wrapper ensures type safety.frontend/src/pages/SubmissionPage.vue (2)
6-7: LGTM!The PageHeader component integration is clean and follows Vue 3 composition API patterns.
Also applies to: 15-15
28-28: No action needed—the custom color class is defined in the Frappe UI preset.The
text-ink-gray-8class is not a custom class that needs to be added to your local Tailwind configuration. It is already provided by thefrappe-uipreset imported intailwind.config.ts. Frappe UI includes an ink-gray color palette with multiple shades (ink-gray-4, ink-gray-5, ink-gray-6, ink-gray-8, etc.) as part of its preset, and these classes are available automatically.frontend/src/components/FormBuilderHeader.vue (1)
12-29: Route name is correctly matched to router configuration.The hardcoded route name
"Form Submission Page"is confirmed to exist inrouter.ts(line 32) and matches the route definition.frontend/src/stores/editForm.ts (1)
101-110: LGTM! Clean refactor of the save submission.The change to submit the entire
formResource.value.docsimplifies the code. The success and error handling callbacks are appropriately structured.Note that
togglePublish(lines 125-141) still uses partial object submission ({ is_published: ... }). This is fine since it only needs to update a single field, but worth being aware of the intentional difference in patterns.frontend/src/types/form.ts (1)
3-14: Good type improvements.The removal of the index signature (
[key: string]: any) and addition of explicit optional fields improves type safety. The new fields (route,is_published,allow_incomplete,linked_doctype,linked_team_id,login_required) properly model the form configuration options needed for this feature.frontend/src/router.ts (1)
31-34: Route pattern and guest meta look good.The
(.*)pattern allows multi-segment form routes (e.g.,/p/org/form-name), andallowGuest: truecorrectly marks this route for the global guard logic.frontend/src/utils/form_fields.ts (2)
18-25: Well-structured type definitions.The
FormFieldTypeandFormFieldstypes provide good type safety. The separation allows reuse of field definitions (likeDataField) whileFormFieldsadds thenameproperty for the lookup array.
130-145: Clean array construction.Using the spread operator with named field constants keeps the code DRY and maintainable.
| @frappe.whitelist(allow_guest=True) | ||
| def is_login_required(route: str) -> bool: | ||
| """ | ||
| Check if login is enabled for a form. | ||
|
|
||
| args: | ||
| form_id: str - The ID of the form to check. | ||
|
|
||
| returns: | ||
| bool - True if login is required, False otherwise. | ||
| """ | ||
| login_enabled = frappe.db.get_value( | ||
| doctype="Form", | ||
| filters={"route": route}, | ||
| fieldname="login_required", | ||
| ) | ||
| return bool(login_enabled) |
There was a problem hiding this comment.
Fix docstring parameter name and verify None handling.
Two concerns:
- Documentation error: The docstring references
form_idin the args section, but the parameter is namedroute. - Silent failure for non-existent routes: When
frappe.db.get_valuereturnsNone(route doesn't exist),bool(None)returnsFalse, making non-existent forms appear as "login not required". Consider if this is the desired behavior or if you should explicitly handle the non-existent route case.
🔎 Proposed fix for docstring
"""
Check if login is enabled for a form.
args:
- form_id: str - The ID of the form to check.
+ route: str - The route of the form to check.
returns:
bool - True if login is required, False otherwise.
"""📝 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.
| @frappe.whitelist(allow_guest=True) | |
| def is_login_required(route: str) -> bool: | |
| """ | |
| Check if login is enabled for a form. | |
| args: | |
| form_id: str - The ID of the form to check. | |
| returns: | |
| bool - True if login is required, False otherwise. | |
| """ | |
| login_enabled = frappe.db.get_value( | |
| doctype="Form", | |
| filters={"route": route}, | |
| fieldname="login_required", | |
| ) | |
| return bool(login_enabled) | |
| @frappe.whitelist(allow_guest=True) | |
| def is_login_required(route: str) -> bool: | |
| """ | |
| Check if login is enabled for a form. | |
| args: | |
| route: str - The route of the form to check. | |
| returns: | |
| bool - True if login is required, False otherwise. | |
| """ | |
| login_enabled = frappe.db.get_value( | |
| doctype="Form", | |
| filters={"route": route}, | |
| fieldname="login_required", | |
| ) | |
| return bool(login_enabled) |
🤖 Prompt for AI Agents
In forms_pro/api/form.py around lines 20 to 36, fix the docstring to reference
the actual parameter name "route" and correct the returns description, and add
explicit handling for when frappe.db.get_value returns None (non-existent route)
instead of silently treating it as False; if login_enabled is None raise a clear
exception (e.g. frappe.DoesNotExistError or frappe.throw with a descriptive
message) so callers know the route was not found, otherwise return
bool(login_enabled).
| import frappe | ||
| from frappe.core.doctype.navbar_settings.navbar_settings import get_app_logo | ||
|
|
||
|
|
||
| @frappe.whitelist(allow_guest=True) | ||
| def get_brand_logo() -> str: | ||
| """ | ||
| Get the brand logo for the form. | ||
|
|
||
| Returns: | ||
| str: The brand logo of the website or the default app logo if brand logo is not set. | ||
| """ | ||
| return str(get_app_logo()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the get_app_logo function definition in the Frappe codebase
find . -type f -name "*.py" | head -20Repository: BuildWithHussain/forms_pro
Length of output: 917
🏁 Script executed:
# Search for get_app_logo function definition
rg -n "def get_app_logo" --type=py -A 15Repository: BuildWithHussain/forms_pro
Length of output: 52
🏁 Script executed:
# Search for imports of get_app_logo and usage patterns
rg -n "get_app_logo" --type=py | head -30Repository: BuildWithHussain/forms_pro
Length of output: 234
🌐 Web query:
Frappe get_app_logo function guest context allow_guest
💡 Result:
Short answer — use website context or a guest-allowed endpoint.
Steps:
-
For static/logo values, set them via hooks (website_context) or implement update_website_context to add logo paths (this is available to website pages, including Guest). Example in hooks.py:
- website_context = {"favicon": "/assets/your_app/img/favicon.png", "splash_image": "/files/your_logo.png"}
- or implement update_website_context = "your_app.overrides.website_context" and set context["brand_html"] / context["banner_image"]. [1]
-
If you need to call a Python method from client-side code to return the app logo, expose that method with @frappe.whitelist(allow_guest=True) so Guest can call it. Note: allow_guest=True only lets the Guest call the method — it does not bypass permission checks inside the method, so ensure you only access data that guests may read. [2][3]
References:
- website_context / update_website_context (hooks) documentation. [1]
- @frappe.whitelist(allow_guest=True) and guest-method behavior / forum guidance. [2][3]
If you want, tell me whether you need an example update_website_context implementation or a whitelisted method snippet.
🌐 Web query:
frappe.core.doctype.navbar_settings get_app_logo source code
💡 Result:
The function is implemented in the Navbar Settings DocType class at:
frappe/core/doctype/navbar_settings/navbar_settings.py — method get_app_logo (i.e. frappe.core.doctype.navbar_settings.navbar_settings.get_app_logo). See the Frappe repo and endpoint listing for reference. [1][2]
Sources:
- Frappe GitHub repository (code tree root). [1]
- Whitelisted endpoints listing showing the exact import path. [2]
🌐 Web query:
site:github.com frappe get_app_logo implementation navbar_settings
💡 Result:
I searched GitHub for Frappe references to get_app_logo and navbar/navbar_settings. Relevant repo issues and threads discussing navbar/app logo behavior and Website Settings include:
- "Website Settings > Login Page > 'App Logo' doesn't change the App Logo" (frappe/frappe). [1]
- "App logo Not changing after changing navbar logo" (frappe/erpnext). [2]
- Example traceback referencing navbar/header image usage in ERPNext templates (frappe/erpnext). [3]
If you want, I can (a) search for the exact function/implementation file that computes/returns the app logo within the Frappe codebase, or (b) open the specific files in the frappe/erpnext repos to locate where navbar_settings or get_app_logo are implemented. Which would you like next?
🏁 Script executed:
# Try to find if get_app_logo is documented in Frappe or if there's any error handling pattern
rg -i "get_app_logo|navbar_settings" --type=pyRepository: BuildWithHussain/forms_pro
Length of output: 229
🌐 Web query:
frappe navbar_settings.py get_app_logo def source code implementation
💡 Result:
I couldn't find a definitive copy of navbar_settings.py/get_app_logo across public Frappe sources. Which Frappe version or repository (GitHub URL or your site) should I look in? I can fetch and paste the exact function once you point me to the correct branch/repo/version.
🌐 Web query:
"def get_app_logo" frappe source
💡 Result:
I couldn't find a function named def get_app_logo in the Frappe codebase. Frappe exposes app/logo via hooks and website settings instead — e.g. the login page uses frappe.get_website_settings("app_logo") or the hooks value app_logo_url (frappe.get_hooks("app_logo_url")[-1]). See the hooks docs and the forum discussion for the code references. [1][2]
Sources:
- Frappe hooks documentation (lists app_logo_url hook). [1]
- Forum snippet showing login/signup using frappe.get_website_settings("app_logo") or frappe.get_hooks("app_logo_url")[-1]. [2]
🏁 Script executed:
# Look for test files that might exercise this code
find . -name "*test*.py" | xargs grep -l "get_brand_logo\|settings" 2>/dev/null | head -10Repository: BuildWithHussain/forms_pro
Length of output: 52
🏁 Script executed:
# Check if there are any requirements or setup files specifying Frappe version
find . -name "requirements.txt" -o -name "setup.py" -o -name "pyproject.toml" | xargs cat 2>/dev/nullRepository: BuildWithHussain/forms_pro
Length of output: 1103
🏁 Script executed:
# Search for any error handling or try-catch around the get_app_logo import
rg -B5 -A5 "get_app_logo" --type=pyRepository: BuildWithHussain/forms_pro
Length of output: 756
Verify get_app_logo() exists and add error handling for guest context.
The web search could not find get_app_logo() in the Frappe codebase—Frappe typically uses hooks (app_logo_url) or website_settings("app_logo") instead. Even if the function exists, @frappe.whitelist(allow_guest=True) does not bypass permission checks inside the called function, so verify it works safely with guest users. Additionally, the function call lacks any try-catch block to handle potential exceptions when accessed in a guest context.
Confirm the import path and add error handling:
@frappe.whitelist(allow_guest=True)
def get_brand_logo() -> str:
"""
Get the brand logo for the form.
Returns:
str: The brand logo of the website or the default app logo if brand logo is not set.
"""
try:
return str(get_app_logo())
except Exception as e:
# Return a fallback or log the error
frappe.logger().warning(f"Failed to get app logo: {e}")
return ""🤖 Prompt for AI Agents
In forms_pro/api/settings.py around lines 1-13, verify that get_app_logo is a
valid import from frappe (if not, use the standard website settings/app_logo
hook such as website_settings("app_logo") or app_logo_url) and make the function
safe for guest access by wrapping the call in a try/except: call the verified
logo getter inside try, log any exception with frappe.logger().warning including
the exception details, and return a safe fallback (empty string or configured
default) on error so guest requests never raise; ensure no privileged APIs are
invoked that require authentication.
There was a problem hiding this comment.
It exists in frappe's codebase
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const getComponent = computed(() => { | ||
| return formFields.find((field) => field.name === props.field.fieldtype); | ||
| }); | ||
|
|
||
| const getBinds = computed(() => { | ||
| return { | ||
| ...props.field, | ||
| ...props.field.options, | ||
| ...props.field.default, | ||
| }; | ||
| return formFields.find( | ||
| (field: FormFields) => field.name === props.field.fieldtype | ||
| ) as FormFieldType; | ||
| }); |
There was a problem hiding this comment.
Potential runtime error when field type is not found.
formFields.find() returns undefined if no matching field type exists. The cast to FormFieldType silences the TypeScript error but doesn't prevent the runtime crash when accessing getComponent.component in the template.
🔎 Proposed fix with fallback handling
const getComponent = computed(() => {
- return formFields.find(
+ const found = formFields.find(
(field: FormFields) => field.name === props.field.fieldtype
- ) as FormFieldType;
+ );
+ if (!found) {
+ console.warn(`Unknown field type: ${props.field.fieldtype}`);
+ return DataField; // fallback to DataField
+ }
+ return found;
});You'll need to import DataField from @/utils/form_fields as the fallback.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/RenderField.vue around lines 13 to 17, getComponent
currently casts the result of formFields.find(...) to FormFieldType which can be
undefined at runtime; update the computed to import DataField from
"@/utils/form_fields" and use it as a safe fallback when find returns undefined
(i.e., return the found FormFieldType or DataField), removing the unsafe cast so
the template can always access getComponent.component without crashing.
| <div v-if="submissionFormStore.formResource.data" class="flex flex-col gap-4 text-gray-800"> | ||
| <h1 class="text-3xl font-bold"> | ||
| {{ submissionFormStore.formResource.data?.title ?? "" }} | ||
| {{ submissionFormStore.formResource.data.title }} | ||
| </h1> | ||
| <div | ||
| v-html="submissionFormStore.formResource.data?.description" | ||
| class="form-description" | ||
| v-html="submissionFormStore.formResource.data.description" | ||
| class="form-description !px-0" | ||
| ></div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the submission form store and formResource definition
fd -e ts -e vue -e js | head -20
# Search for formResource usage
rg -n 'formResource' --type vue --type ts -B2 -A2
# Look for the API endpoints
fd -name '*.py' | xargs grep -l 'form.*resource\|frappe.whitelist'Repository: BuildWithHussain/forms_pro
Length of output: 1241
🏁 Script executed:
# Find all Vue and TypeScript files with formResource
rg 'formResource' --type-list | head
# Search for formResource in the codebase
rg 'formResource' -B2 -A2
# Find the submission form store
fd 'store|submission' --type f | grep -E '\.(ts|js|vue)$'
# Look for API endpoints
fd '\.py$' | xargs grep -l 'form' | head -20Repository: BuildWithHussain/forms_pro
Length of output: 17886
🏁 Script executed:
# Check the API implementation for get_form_by_route
cat frontend/src/stores/submissionForm.ts | head -50
# Look at the form.py API
cat forms_pro/api/form.py
# Check the Form doctype definition
fd 'form.json' | head -5Repository: BuildWithHussain/forms_pro
Length of output: 5302
🏁 Script executed:
# Read the Form doctype JSON
cat forms_pro/forms_pro/doctype/form/form.json | grep -A10 -B2 '"fieldname": "title"' | head -20
# Also check for description field
cat forms_pro/forms_pro/doctype/form/form.json | grep -A10 -B2 '"fieldname": "description"' | head -20
# Get file size first to decide how to read it
wc -l forms_pro/forms_pro/doctype/form/form.jsonRepository: BuildWithHussain/forms_pro
Length of output: 549
Add optional chaining for the description property.
The title field is required in the Form doctype, but description is optional. Accessing formResource.data.description directly without optional chaining will cause issues if the API returns an undefined value. Use formResource.data.description?. or add a fallback value.
🤖 Prompt for AI Agents
In frontend/src/components/submission/FormHeader.vue around lines 11 to 18, the
template binds submissionFormStore.formResource.data.description directly but
description is optional; update the v-html binding to use optional chaining or a
safe fallback (e.g.,
submissionFormStore.formResource.data.description?.toString() or
submissionFormStore.formResource.data.description ?? '') so that undefined
descriptions do not throw errors and the template renders safely.
There was a problem hiding this comment.
@harshtandiya, understood. I'll note your decision on this.
| const brandLogo = createResource({ | ||
| url: "forms_pro.api.settings.get_brand_logo", | ||
| auto: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling and loading state for brand logo.
The brand logo is rendered directly without handling loading or error states. If brandLogo.data is undefined (during loading or on error), the img tag will have a broken src attribute.
🔎 Proposed improvements
<template>
<div class="max-w-screen-md mx-auto flex justify-between items-center">
- <img :src="brandLogo.data" alt="Brand Logo" class="w-10 h-10" />
+ <img
+ v-if="brandLogo.data"
+ :src="brandLogo.data"
+ alt="Brand Logo"
+ class="w-10 h-10"
+ />
+ <div v-else class="w-10 h-10 bg-gray-200 rounded animate-pulse"></div>
<div>Also applies to: 19-19
🤖 Prompt for AI Agents
In frontend/src/components/submission/PageHeader.vue around lines 8-11 and line
19, the brandLogo is used directly which can produce a broken img src when
brandLogo.data is undefined; update the template to conditionally render the
<img> only when brandLogo.data is truthy, show a loading skeleton or placeholder
while brandLogo.loading is true, and render a fallback/logo text when
brandLogo.error is set; additionally add an img @error handler to swap to the
fallback source or hide the image to ensure no broken src is displayed.
| function redirectToLogin() { | ||
| window.location.href = `/login?redirect-to=${route.fullPath}`; | ||
| } |
There was a problem hiding this comment.
URL-encode the redirect path.
The route.fullPath should be URL-encoded when used as a query parameter to handle special characters correctly.
🔎 Proposed fix
function redirectToLogin() {
- window.location.href = `/login?redirect-to=${route.fullPath}`;
+ window.location.href = `/login?redirect-to=${encodeURIComponent(route.fullPath)}`;
}🤖 Prompt for AI Agents
In frontend/src/components/submission/PageHeader.vue around lines 13 to 15, the
redirect URL uses route.fullPath directly as a query value which can break with
special characters; fix by URL-encoding the path when constructing the query
(use encodeURIComponent(route.fullPath)) so the redirect-to parameter is safely
encoded before assigning window.location.href.
| /** | ||
| * Check if login is required for a form submission page. | ||
| * | ||
| * @param formId - The ID of the form to check. | ||
| * | ||
| * @returns bool - True if login is required, False otherwise. | ||
| */ |
There was a problem hiding this comment.
Fix JSDoc parameter name.
The JSDoc references formId but the parameter is named route.
🔎 Proposed fix
/**
* Check if login is required for a form submission page.
*
- * @param formId - The ID of the form to check.
+ * @param route - The route of the form to check.
*
* @returns bool - True if login is required, False otherwise.
*/📝 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.
| /** | |
| * Check if login is required for a form submission page. | |
| * | |
| * @param formId - The ID of the form to check. | |
| * | |
| * @returns bool - True if login is required, False otherwise. | |
| */ | |
| /** | |
| * Check if login is required for a form submission page. | |
| * | |
| * @param route - The route of the form to check. | |
| * | |
| * @returns bool - True if login is required, False otherwise. | |
| */ |
🤖 Prompt for AI Agents
In frontend/src/utils/form.ts around lines 3 to 9, the JSDoc incorrectly names
the parameter as "formId" while the actual function parameter is "route"; update
the JSDoc to reference "@param route" (and adjust the description to explain
that route is the form submission route or identifier expected by the function)
and ensure the "@returns" line uses correct casing/description (e.g., "boolean"
or "True if login is required, false otherwise") so the doc matches the function
signature and return value.
| export async function isLoginRequired(route: string) { | ||
| const loginRequired = useCall({ | ||
| baseUrl: "/api/v2/method/", | ||
| url: "forms_pro.api.form.is_login_required", | ||
| params: { route }, | ||
| immediate: false, | ||
| }); | ||
|
|
||
| await loginRequired.execute(); | ||
| return loginRequired.data; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling and return type annotation.
The function lacks error handling for API failures and a TypeScript return type. If the API call fails, loginRequired.data may be undefined, leading to unexpected behavior.
🔎 Proposed improvements
-export async function isLoginRequired(route: string) {
+export async function isLoginRequired(route: string): Promise<boolean> {
const loginRequired = useCall({
baseUrl: "/api/v2/method/",
url: "forms_pro.api.form.is_login_required",
params: { route },
immediate: false,
});
await loginRequired.execute();
+
+ if (loginRequired.error) {
+ console.error("Failed to check login requirement:", loginRequired.error);
+ return false; // Or throw an error, depending on desired behavior
+ }
+
return loginRequired.data;
}🤖 Prompt for AI Agents
In frontend/src/utils/form.ts around lines 10 to 20, the isLoginRequired
function is missing a TypeScript return type and lacks error handling for failed
API calls; update the signature to return Promise<boolean>, wrap the API call in
try/catch, await loginRequired.execute() inside the try, read loginRequired.data
but guard against undefined (return a sensible default like false or rethrow a
wrapped error), and log or surface the caught error so callers receive a
deterministic boolean result instead of potential undefined.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.