Feature/automatic builds project options #1301
Conversation
WalkthroughIntroduces a reusable Toggle component and a ToggleForm wrapper, replaces PublicPrivateToggle usages, adds project toggles for Auto Publish and Rebuild on Software Update (UI + server actions), includes these fields in project details, updates project Settings form to use ToggleForm, simplifies project page props, and adds related locale strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant T as ToggleForm (Svelte)
participant S as +page.server.ts (Action)
participant DB as Project Store
U->>T: Click toggle
T->>T: Update bound formVar
T->>T: enhance() intercepts submit
T->>S: POST /toggle... { name: bool }
S->>DB: Update field
DB-->>S: Success/Failure
S-->>T: { ok: true/false, form }
alt ok == true
T->>U: Show on/off toast
else ok == false
T->>T: Revert formVar
T->>U: Show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Overall, this looks good; there are just a few things that were lost when refactoring to Toggle.svelte.
- Any time that we used
PublicPrivateToggleshould still have the lock icons, unless @chrisvire or @7dev7urandom say otherwise. Similarly, there are some other places (I haven't actually looked) where you could probably use the newToggleorToggleForm. - We recently made some changes (see #1275 ) that disable the ability to modify the settings on a project if a user is allowed to view, but not edit, the project. Those changes have disappeared entirely. (VS Code shows that the
canEditvariable in/projects/[id]/forms/Settings.svelteas being completely unused). - You will want to run
npm run checkandnpm run lint. Many of the issues that I have pointed out in the comments are in fact found by running those commands.
Generalize the PublicPrivateToggle component into Toggle. Use Toggle in the project settings page. This will enable its use in settings.
Paper over the repetitive `form`s in settings using a new ToggleForm component.
Several PublicPrivateToggle uses sstill existed, replace them with just Toggle.
Add the internationalizations for auto publish/rebuild toggle notifications.
Add the routes for AutoPublishOnRebuild and RebuildOnSoftwareUpdate.
Remove the parameterization of endpoints, as Settings is not a good abstraction already. Add the new toggles and their endpoints.
Correct the field used by the server for the toggleRebuild path.
Restore the lock icon in locations where PublicPrivateToggle was replaced with Toggle.
Replace ActionData with { ok: boolean } type.
judah-sotomayor
force-pushed
the
feature/automatic-builds-project-options
branch
from
e782bf5 to
a206027
Compare
|
Rebased onto main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/lib/components/settings/ToggleForm.svelte (1)
50-61: Past review concern appears resolved.The cast to
{ ok: boolean }on line 51 addresses the previous feedback aboutActionData. The type is now inline as suggested.However, verify the revert logic on line 60: when
!ok,formVaris toggled back. SinceformVarhas already been updated by the Toggle'sonchange(line 72-74), the revert correctly undoes the optimistic update. The toast messages on lines 54 and 56 use the current (new) state offormVarto decide which message to show, which aligns with the expected behavior.src/routes/(authenticated)/projects/[id=idNumber]/forms/Settings.svelte (1)
18-18: Verify thatcanEditdisables the toggle inputs.The past review comment indicates that
canEditshould disable form inputs. In the current implementation,canEditis passed to eachToggleForm, which forwards it to the underlyingTogglecomponent.Ensure that the
Togglecomponent properly disables the input whencanEditisfalse. This behavior is critical to prevent unauthorized modifications.Run the following script to verify the Toggle component's handling of
canEdit:#!/bin/bash # Check if Toggle component properly handles canEdit/disabled state ast-grep --pattern $'export let canEdit$$$' ast-grep --pattern $'disabled={$_}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/components/settings/Toggle.svelte(2 hunks)src/lib/components/settings/ToggleForm.svelte(1 hunks)src/lib/locales/en-US.json(8 hunks)src/lib/locales/es-419.json(8 hunks)src/lib/locales/fr-FR.json(2 hunks)src/lib/projects/sse.ts(1 hunks)src/routes/(authenticated)/admin/settings/organizations/edit/+page.svelte(2 hunks)src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.svelte(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts(2 hunks)src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte(1 hunks)src/routes/(authenticated)/projects/[id=idNumber]/forms/Settings.svelte(1 hunks)src/routes/(authenticated)/projects/new/[id=idNumber]/+page.svelte(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (1)
src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
🪛 GitHub Actions: Scriptoria v2 Test and Deploy
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
[error] 321-321: eslint: Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*() sveltekit-sec/require-security-check
🪛 GitHub Check: typecheck-lint
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
[failure] 337-337:
Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()
[failure] 321-321:
Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()
🔇 Additional comments (20)
src/lib/components/settings/ToggleForm.svelte (1)
66-80: LGTM!The Toggle integration is clean:
checkedis bound toformVar, and theinputAttr.onchangehandler triggers form submission. All necessary props are forwarded correctly.src/lib/projects/sse.ts (1)
30-31: LGTM!The addition of
AutoPublishOnRebuildandRebuildOnSoftwareUpdateto the project select correctly exposes the new fields needed for the automatic builds feature.src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (1)
243-243: LGTM!Simplifying the Settings component props by removing
publicEndpointanddownloadEndpointis a good encapsulation refactor. The endpoints are now managed internally within Settings.src/lib/locales/fr-FR.json (2)
368-372: LGTM!The French locale additions for auto-publish and auto-rebuild features are properly structured and align with the new toggle functionality.
397-399: LGTM!The action message translations for auto-publish on/off/error states are consistent with the existing pattern for other toggle features.
src/routes/(authenticated)/admin/settings/organizations/edit/+page.svelte (1)
103-111: LGTM!The migration from
PublicPrivateToggletoTogglewith the updated props (name,onIcon,offIcon) is consistent with the broader refactoring effort across the codebase.src/routes/(authenticated)/projects/[id=idNumber]/forms/Settings.svelte (1)
33-82: LGTM!The four
ToggleFormusages follow a consistent pattern for handling project settings toggles. Each correctly binds to its state variable, specifies appropriate endpoints, and provides clear success/error messages.The addition of lock icons (
mdi:lock-open-variant,mdi:lock) for the visibility toggle enhances the UI clarity.src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.svelte (1)
36-47: LGTM!The migration to the
Togglecomponent with updated props (name,onIcon,offIcon) is consistent with the broader refactoring pattern across the application.src/routes/(authenticated)/projects/new/[id=idNumber]/+page.svelte (1)
8-8: LGTM!The import has been correctly updated to use the new
Togglecomponent instead ofPublicPrivateToggle.src/lib/locales/es-419.json (3)
23-61: LGTM!The reformatting of
localePickerentries from multi-line to single-line arrays preserves semantic content while improving readability.
245-254: LGTM!The reformatting of
products_numArtifactsdeclarations and selectors is consistent with thelocalePickerchanges.
368-373: LGTM!The new translation keys for auto-publish and auto-rebuild features are well-defined and align with the PR objectives. The Spanish translations are appropriate.
Also applies to: 398-400
src/lib/locales/en-US.json (3)
23-61: LGTM!The reformatting of
localePickerentries is consistent with the changes ines-419.jsonand preserves semantic content.
245-254: LGTM!The reformatting of
products_numArtifactsis consistent with the corresponding changes ines-419.json.
368-373: LGTM!The new translation keys for auto-publish and auto-rebuild features are well-defined and align with the PR objectives. The English translations are clear and appropriate.
Also applies to: 401-403
src/lib/components/settings/Toggle.svelte (5)
7-17: LGTM!The Props interface has been correctly updated to reflect the new API:
formNamerenamed toname- New props
canEdit,onIcon,offIcon, andclassNameaddedThese changes support the component's enhanced flexibility for different use cases.
19-29: LGTM!The destructuring correctly handles the new props with appropriate defaults:
canEdit = true(enables editing by default)onIcon = ''andoffIcon = ''(no icons by default)
32-32: LGTM!The conditional
cursor-not-allowedstyling correctly addresses the past review comment, applying the disabled cursor only whencanEditis false while preserving any customclassNamepassed in.
35-42: LGTM!The input element has been correctly updated:
namebinding replacesformNamedisabledattribute correctly bound to!canEditThis ensures the toggle is properly disabled when
canEditis false.
49-56: LGTM!The icon rendering has been correctly updated to use dynamic
IconContainercomponents with configurableonIconandoffIconprops, replacing the hardcoded icons. This provides the flexibility needed for different toggle use cases.
| async toggleAutoPublishOnRebuild(event) { | ||
| // permissions checked in auth | ||
| const form = await superValidate( | ||
| event.request, | ||
| valibot( | ||
| v.object({ | ||
| autoPublishOnRebuild: v.boolean() | ||
| }) | ||
| ) | ||
| ); | ||
| if (!form.valid) return fail(400, { form, ok: false }); | ||
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | ||
| AutoPublishOnRebuild: form.data.autoPublishOnRebuild | ||
| }); | ||
| return { form, ok: true }; | ||
| }, |
There was a problem hiding this comment.
CRITICAL: Missing authorization check.
The toggleAutoPublishOnRebuild action lacks an explicit authorization check. While the comment on line 322 states "permissions checked in auth," there is no call to event.locals.security.requireProjectWriteAccess() or similar. This is inconsistent with other actions in the file (e.g., toggleVisibility at line 277-283) and could allow unauthorized users to modify project settings.
Apply this diff to add the required security check:
async toggleAutoPublishOnRebuild(event) {
- // permissions checked in auth
+ event.locals.security.requireProjectWriteAccess(
+ await DatabaseReads.projects.findUnique({
+ where: { Id: parseInt(event.params.id) },
+ select: { OwnerId: true, OrganizationId: true }
+ })
+ );
const form = await superValidate(📝 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.
| async toggleAutoPublishOnRebuild(event) { | |
| // permissions checked in auth | |
| const form = await superValidate( | |
| event.request, | |
| valibot( | |
| v.object({ | |
| autoPublishOnRebuild: v.boolean() | |
| }) | |
| ) | |
| ); | |
| if (!form.valid) return fail(400, { form, ok: false }); | |
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | |
| AutoPublishOnRebuild: form.data.autoPublishOnRebuild | |
| }); | |
| return { form, ok: true }; | |
| }, | |
| async toggleAutoPublishOnRebuild(event) { | |
| event.locals.security.requireProjectWriteAccess( | |
| await DatabaseReads.projects.findUnique({ | |
| where: { Id: parseInt(event.params.id) }, | |
| select: { OwnerId: true, OrganizationId: true } | |
| }) | |
| ); | |
| const form = await superValidate( | |
| event.request, | |
| valibot( | |
| v.object({ | |
| autoPublishOnRebuild: v.boolean() | |
| }) | |
| ) | |
| ); | |
| if (!form.valid) return fail(400, { form, ok: false }); | |
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | |
| AutoPublishOnRebuild: form.data.autoPublishOnRebuild | |
| }); | |
| return { form, ok: true }; | |
| }, |
🧰 Tools
🪛 GitHub Actions: Scriptoria v2 Test and Deploy
[error] 321-321: eslint: Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*() sveltekit-sec/require-security-check
🪛 GitHub Check: typecheck-lint
[failure] 321-321:
Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()
🤖 Prompt for AI Agents
In src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts around
lines 321 to 336, the toggleAutoPublishOnRebuild action is missing an explicit
authorization check; add a call to
event.locals.security.requireProjectWriteAccess(parseInt(event.params.id)) (or
the existing project write access helper used elsewhere in this file)
immediately after validating the form and before performing
DatabaseWrites.projects.update so the action enforces the same project write
permissions as other actions in the file.
| async toggleRebuildOnSoftwareUpdate(event) { | ||
| // permissions checked in auth | ||
| const form = await superValidate( | ||
| event.request, | ||
| valibot( | ||
| v.object({ | ||
| autoRebuildOnSoftwareUpdate: v.boolean() | ||
| }) | ||
| ) | ||
| ); | ||
| if (!form.valid) return fail(400, { form, ok: false }); | ||
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | ||
| RebuildOnSoftwareUpdate: form.data.autoRebuildOnSoftwareUpdate | ||
| }); | ||
| return { form, ok: true }; | ||
| }, |
There was a problem hiding this comment.
CRITICAL: Missing authorization check.
The toggleRebuildOnSoftwareUpdate action also lacks an explicit authorization check. This mirrors the issue in toggleAutoPublishOnRebuild and must be fixed to prevent unauthorized access.
Apply this diff to add the required security check:
async toggleRebuildOnSoftwareUpdate(event) {
- // permissions checked in auth
+ event.locals.security.requireProjectWriteAccess(
+ await DatabaseReads.projects.findUnique({
+ where: { Id: parseInt(event.params.id) },
+ select: { OwnerId: true, OrganizationId: true }
+ })
+ );
const form = await superValidate(📝 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.
| async toggleRebuildOnSoftwareUpdate(event) { | |
| // permissions checked in auth | |
| const form = await superValidate( | |
| event.request, | |
| valibot( | |
| v.object({ | |
| autoRebuildOnSoftwareUpdate: v.boolean() | |
| }) | |
| ) | |
| ); | |
| if (!form.valid) return fail(400, { form, ok: false }); | |
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | |
| RebuildOnSoftwareUpdate: form.data.autoRebuildOnSoftwareUpdate | |
| }); | |
| return { form, ok: true }; | |
| }, | |
| async toggleRebuildOnSoftwareUpdate(event) { | |
| event.locals.security.requireProjectWriteAccess( | |
| await DatabaseReads.projects.findUnique({ | |
| where: { Id: parseInt(event.params.id) }, | |
| select: { OwnerId: true, OrganizationId: true } | |
| }) | |
| ); | |
| const form = await superValidate( | |
| event.request, | |
| valibot( | |
| v.object({ | |
| autoRebuildOnSoftwareUpdate: v.boolean() | |
| }) | |
| ) | |
| ); | |
| if (!form.valid) return fail(400, { form, ok: false }); | |
| await DatabaseWrites.projects.update(parseInt(event.params.id), { | |
| RebuildOnSoftwareUpdate: form.data.autoRebuildOnSoftwareUpdate | |
| }); | |
| return { form, ok: true }; | |
| }, |
🧰 Tools
🪛 GitHub Check: typecheck-lint
[failure] 337-337:
Missing call to requireAuthenticated() or similar security check. This must be of the form locals.security.require*()
🤖 Prompt for AI Agents
In src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts around
lines 337 to 352, the toggleRebuildOnSoftwareUpdate action is missing an
explicit authorization check; copy the same authorization check used in
toggleAutoPublishOnRebuild into the start of this action (before any DB update
or form validation result handling), verifying the current user has permission
to modify the project identified by parseInt(event.params.id) and returning an
appropriate 403/fail response if unauthorized, then proceed with existing
validation and the DatabaseWrites.projects.update call.
| <Toggle | ||
| title={{ key: 'project_public' }} | ||
| message={{ key: 'project_visibilityDescription' }} | ||
| className="py-2 md:max-w-xs" | ||
| formName="IsPublic" | ||
| name="IsPublic" | ||
| inputAttr={{ onchange: () => {} }} | ||
| bind:checked={$form.IsPublic} | ||
| onIcon="mdi:lock-open-variant" | ||
| offIcon="mdi:lock" | ||
| /> |
There was a problem hiding this comment.
Remove the empty onchange handler.
The inputAttr prop contains an empty onchange handler () => {} on line 92, which serves no purpose and should be removed.
Apply this diff to remove the unnecessary handler:
<Toggle
title={{ key: 'project_public' }}
message={{ key: 'project_visibilityDescription' }}
className="py-2 md:max-w-xs"
name="IsPublic"
- inputAttr={{ onchange: () => {} }}
bind:checked={$form.IsPublic}
onIcon="mdi:lock-open-variant"
offIcon="mdi:lock"
/>📝 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.
| <Toggle | |
| title={{ key: 'project_public' }} | |
| message={{ key: 'project_visibilityDescription' }} | |
| className="py-2 md:max-w-xs" | |
| formName="IsPublic" | |
| name="IsPublic" | |
| inputAttr={{ onchange: () => {} }} | |
| bind:checked={$form.IsPublic} | |
| onIcon="mdi:lock-open-variant" | |
| offIcon="mdi:lock" | |
| /> | |
| <Toggle | |
| title={{ key: 'project_public' }} | |
| message={{ key: 'project_visibilityDescription' }} | |
| className="py-2 md:max-w-xs" | |
| name="IsPublic" | |
| bind:checked={$form.IsPublic} | |
| onIcon="mdi:lock-open-variant" | |
| offIcon="mdi:lock" | |
| /> |
🤖 Prompt for AI Agents
In src/routes/(authenticated)/projects/new/[id=idNumber]/+page.svelte around
lines 87 to 96, the Toggle component's inputAttr includes an empty onchange
handler (() => {}); remove this unused onchange property from inputAttr so the
prop no longer passes a no-op handler—leave inputAttr out entirely or only
include meaningful attributes.
Add the toggles necessary for automatic builds.
Addresses #1235
Summary by CodeRabbit
New Features
Refactor