feat: extract callbacks to library #1280
Conversation
WalkthroughThis pull request modifies the environment variable for callbacks and updates the operating system version from a beta release to a stable one. It also refactors the code by renaming Changes
Sequence Diagram(s)sequenceDiagram
participant C as Vue Component
participant CAS as CallbackActionsStore
participant UC as useCallback (from @unraid/shared-callbacks)
participant P as Pinia Store
C->>CAS: Initialize callback actions
CAS->>P: setActivePinia(createPinia())
CAS->>UC: Destructure send & watcher from useCallback()
UC-->>CAS: Return send & watcher
CAS->>C: Expose callback methods for handling actions
Suggested reviewers
Poem
🪧 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 (
|
elibosley
force-pushed
the
chore/callback-extracted-to-library
branch
from
63ab033 to
c4f21d8
Compare
elibosley
force-pushed
the
chore/callback-extracted-to-library
branch
from
c4f21d8 to
ba94cd9
Compare
|
Before |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/store/callbackActions.ts (1)
21-23: Consider moving store initialization elsewhereWhile
setActivePinia(createPinia())is needed to initialize the Pinia store, placing it directly in the store definition file is unusual. This kind of initialization typically belongs in a main application file or entry point.-setActivePinia(createPinia());This line could be moved to your main application initialization file (like main.ts or app.ts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/store/callbackActions.ts(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
web/store/callbackActions.ts (3)
web/store/account.ts (1)
useAccountStore(25-334)web/store/installKey.ts (1)
useInstallKeyStore(11-74)web/store/updateOsActions.ts (1)
useUpdateOsActionsStore(39-239)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
web/store/callbackActions.ts (9)
1-13: Good migration to shared library for callback typesThe import changes here reflect the migration from local types to a shared library (
@unraid/shared-callbacks). This promotes code reuse and type consistency across the application.
25-28: Good use of the shared callback functionalityDestructuring
sendandwatcherfrom the importeduseCallbackfunction is a clean approach. RenamingwatchertoprovidedWatcherhelps avoid naming conflicts.
40-45: Well-implemented wrapper for the provided watcherThe new
watcherfunction properly encapsulates the provided watcher functionality and ensures callback data is saved when results are available.
72-77: Improved conditional structureThe reformatted conditional checks are more readable and maintainable.
86-89: Good explicit typing for action parameterAdding explicit typing for the action parameter as
ExternalActionsimproves type safety and code readability.
108-119: Comprehensive handling of OS update actionsThe implementation properly handles both update and downgrade OS actions with appropriate logic for refreshing the server state. The early return for single actions is a good optimization.
135-144: Clean formatting of success condition checksThe reformatted condition checks for determining success status are more readable.
153-155: Simplified callback status setterThe refactored
setCallbackStatusfunction is concise and to the point.
176-177: Good exposure of callback functionalityExporting
sendandwatcherfrom the store makes these functions accessible to consumers, which is appropriate for a callback store.
|
@coderabbitai - This nitpick would break our build. The various Vue components are compiled into web components. This difference in initialization of the Pinia stores makes the web components share a singular store. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@zackspear Thank you for pointing this out! I understand now that the initialization approach with I apologize for the nitpick - maintaining the correct store initialization is critical for your architecture. Thanks for explaining the technical constraints of your web component implementation. 🏁 Script executed: Length of output: 6089 @zackspear I see the issue now - the Thank you for the correction and for explaining the technical constraint. I now understand why maintaining separate Pinia store instances for each web component is critical for your application. ✏️ Learnings added
|
@coderabbitai You're understanding it in the wrong way, m8. setActivePinia(createPinia() within each of the stores prevents isolation, which is what we don't want. A single Pinia instance is used across all web components thanks to this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@zackspear Thank you for the correction! I completely misunderstood your explanation. You're right, and I had it backward. To clarify: I apologize for the confusion in my previous response. Thanks for taking the time to set the record straight. 🏁 Script executed: Length of output: 6089 @zackspear Thanks for the clarification. Looking at your code, I now understand correctly: The I completely misunderstood and had it backward in my previous response. Thanks for setting me straight on how your web component architecture works with Pinia. ✏️ Learnings added
|
Noticed this yesterday night and was going to try to fix this morning, thanks a ton. |
* enables webcomponent redirection on new account application
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
web/pages/redirect.vue (4)
5-5: Remove console.log statementConsole logs should be removed from production code.
- console.log(target);
14-16: Improve data parameter handlingThe condition checks if there are exactly 1 or 2 search parameters, but this may be too restrictive for future use cases. Consider keeping the data parameter regardless of how many other parameters exist.
- if (search.has('data') && (search.size === 1 || search.size === 2)) { + if (search.has('data')) { return `${window.location.origin}${targetRoute}?data=${encodeURIComponent(search.get('data')!)}`; }
34-37: Replace inline styles with CSS classesUsing inline styles makes the component harder to maintain. Consider using CSS classes instead.
- <div - id="text" - style="text-align: center; margin-top: calc(100vh - 75%); display: none; font-family: sans-serif" - > + <div + id="text" + class="redirect-message" + >Then add a style section:
<style scoped> .redirect-message { text-align: center; margin-top: calc(100vh - 75%); display: none; font-family: sans-serif; } </style>
1-43: Consider adding security checks for redirect targetsCurrently, the function always redirects to '/', but if you implement actual target parsing, be sure to include security checks to prevent open redirect vulnerabilities.
When implementing proper target validation:
- Only allow relative URLs or URLs within your domain
- Validate URLs against an allowlist of permitted destinations
- Consider using a library like sanitize-url to prevent JavaScript injection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/pages/redirect.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/pages/redirect.vue (1)
11-18:❓ Verification inconclusive
Verify the
getRedirectUrlfunction logicSince
parseRedirectTargetcurrently always returns '/', this function effectively ignores any target parameter value and always redirects to the home page. Additionally, the condition checking for search.size is unclear in its purpose.
🏁 Script executed:
#!/bin/bash # Search for other redirect implementations for comparison rg -A 5 -B 5 "redirect.*url|url.*redirect" --type vue --type tsLength of output: 93
Action Required: Clarify Redirection Logic
The current implementation of
getRedirectUrlinweb/pages/redirect.vueshows two points that need manual verification:
Redirection Target:
The function callsparseRedirectTarget(search.get('target')), but available evidence indicates thatparseRedirectTargetalways returns'/'. This results in ignoring any dynamictargetparameter. Please verify if this is the intended behavior or ifparseRedirectTargetshould actually process different target values.Search Size Condition:
The condition(search.size === 1 || search.size === 2)used alongside checking for the presence of a'data'parameter is unclear. Confirm whether these bounds are necessary and, if so, document their purpose.Please review and confirm these behaviors to ensure the redirection logic meets the intended requirements.
| onMounted(() => { | ||
| setTimeout(() => { | ||
| const textElement = document.getElementById('text'); | ||
| if (textElement) { | ||
| textElement.style.display = 'block'; | ||
| } | ||
| }, 750); | ||
|
|
||
| window.location.href = getRedirectUrl(); | ||
| }); |
There was a problem hiding this comment.
Missing import for onMounted
The onMounted function is used but there's no import statement for it.
<script setup lang="ts">
+import { onMounted } from 'vue';
const parseRedirectTarget = (target: string | null) => {📝 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.
| onMounted(() => { | |
| setTimeout(() => { | |
| const textElement = document.getElementById('text'); | |
| if (textElement) { | |
| textElement.style.display = 'block'; | |
| } | |
| }, 750); | |
| window.location.href = getRedirectUrl(); | |
| }); | |
| <script setup lang="ts"> | |
| import { onMounted } from 'vue'; | |
| const parseRedirectTarget = (target: string | null) => { | |
| // ... rest of the code | |
| } | |
| onMounted(() => { | |
| setTimeout(() => { | |
| const textElement = document.getElementById('text'); | |
| if (textElement) { | |
| textElement.style.display = 'block'; | |
| } | |
| }, 750); | |
| window.location.href = getRedirectUrl(); | |
| }); | |
| </script> |
| const parseRedirectTarget = (target: string | null) => { | ||
| if (target && target !== '/') { | ||
| // parse target and ensure it is a bare path with no query parameters | ||
| console.log(target); | ||
| return '/'; | ||
| } | ||
| return '/'; | ||
| }; |
There was a problem hiding this comment.
The parseRedirectTarget function doesn't perform any actual validation
This function has a comment indicating it should parse and ensure the target is a bare path without query parameters, but it actually just logs the target and always returns '/'. This makes the conditional logic ineffective as the function will always return the home route regardless of input.
const parseRedirectTarget = (target: string | null) => {
if (target && target !== '/') {
// parse target and ensure it is a bare path with no query parameters
- console.log(target);
- return '/';
+ // Remove query parameters if present
+ const parsedUrl = new URL(target, window.location.origin);
+ return parsedUrl.pathname;
}
return '/';
};📝 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.
| const parseRedirectTarget = (target: string | null) => { | |
| if (target && target !== '/') { | |
| // parse target and ensure it is a bare path with no query parameters | |
| console.log(target); | |
| return '/'; | |
| } | |
| return '/'; | |
| }; | |
| const parseRedirectTarget = (target: string | null) => { | |
| if (target && target !== '/') { | |
| // parse target and ensure it is a bare path with no query parameters | |
| // Remove query parameters if present | |
| const parsedUrl = new URL(target, window.location.origin); | |
| return parsedUrl.pathname; | |
| } | |
| return '/'; | |
| }; |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| }; | ||
|
|
||
| onMounted(() => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This looks like it will immediately redirect without ever showing the text. What's the desired ux for this?
🤖 I have created a release *beep* *boop* --- ## [4.5.0](v4.4.1...v4.5.0) (2025-04-02) ### Features * add webgui theme switcher component ([#1304](#1304)) ([e2d00dc](e2d00dc)) * api plugin system & offline versioned dependency vendoring ([#1252](#1252)) ([9f492bf](9f492bf)) * **api:** add `unraid-api --delete` command ([#1289](#1289)) ([2f09445](2f09445)) * basic array controls ([#1291](#1291)) ([61fe696](61fe696)) * basic docker controls ([#1292](#1292)) ([12eddf8](12eddf8)) * copy to webgui repo script docs + wc build options ([#1285](#1285)) ([e54f189](e54f189)) ### Bug Fixes * additional url fixes ([4b2763c](4b2763c)) * **api:** redirect benign pnpm postinstall warning to log file ([#1290](#1290)) ([7fb7849](7fb7849)) * **deps:** update dependency chalk to v5 ([#1296](#1296)) ([6bed638](6bed638)) * **deps:** update dependency diff to v7 ([#1297](#1297)) ([3c6683c](3c6683c)) * disable all config watchers ([#1306](#1306)) ([5c1b435](5c1b435)) * extract callbacks to library ([#1280](#1280)) ([2266139](2266139)) * OEM plugin issues ([#1288](#1288)) ([d5a3d0d](d5a3d0d)) * replace files lost during pruning ([d0d2ff6](d0d2ff6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Chores
Refactor
New Features