Feature/automatic builds project options #1301
Conversation
WalkthroughAdds a reusable Toggle and ToggleForm, replaces PublicPrivateToggle with Toggle across routes, adds server actions and project fields for AutoPublishOnRebuild and RebuildOnSoftwareUpdate, updates Settings to use ToggleForm, and adds localization keys for auto-publish/rebuild. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ToggleForm (client)
participant S as +page.server.ts (action)
participant DB as Project Store
User->>UI: click toggle
UI->>UI: update bound formVar
UI->>UI: call form.submit() (enhance)
UI->>S: POST { <field>: boolean }
S->>DB: update Project.<field>
DB-->>S: success / failure
S-->>UI: { ok: true/false, form }
alt ok == true
UI->>User: show on/off toast
else
UI->>UI: revert formVar
UI->>User: show error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (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 |
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.
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.
judah-sotomayor
force-pushed
the
feature/automatic-builds-project-options
branch
from
a206027 to
772c3d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/routes/(authenticated)/projects/new/[id=idNumber]/+page.svelte (1)
87-96: Remove the emptyonchangehandler.The
inputAttrprop contains an emptyonchangehandler on line 92 that 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" />
📜 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)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/(authenticated)/admin/settings/organizations/edit/+page.svelte
- src/lib/locales/fr-FR.json
- src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.svelte
- src/lib/components/settings/Toggle.svelte
- src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts
- src/lib/locales/en-US.json
⏰ 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)
- GitHub Check: build-and-test
🔇 Additional comments (4)
src/lib/projects/sse.ts (1)
30-32: LGTM!The additions of
AutoPublishOnRebuildandRebuildOnSoftwareUpdateto the project data projection are consistent with the existing boolean fields pattern and align with the PR's objective to support automatic builds.src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (1)
242-242: LGTM!The simplified Settings component usage improves encapsulation by moving endpoint management into the component itself, consistent with the new ToggleForm-based architecture.
src/routes/(authenticated)/projects/[id=idNumber]/forms/Settings.svelte (1)
33-81: LGTM!All four ToggleForm instances consistently pass the
canEditprop, which addresses the previous review concern about preserving the ability to disable form inputs based on edit permissions.src/lib/components/settings/ToggleForm.svelte (1)
1-80: LGTM!This component provides a clean abstraction for toggle-based form submissions with proper error handling, state rollback on failure, and auto-submit behavior. The inline type assertion on line 51 addresses the previous review feedback about the
ActionDatatype.
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.
The toggle component is now greatly simplified. I replaced the custom toggle with daisyui's toggle-and-icon support. I also enabled an opacity reduction when the toggle is not enabled.
judah-sotomayor
force-pushed
the
feature/automatic-builds-project-options
branch
from
772c3d6 to
4c53d98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/locales/es-419.json (2)
370-370: Fix Spanish typo.Line 370 contains "propietaro" which should be "propietario" (missing the second "i").
- "project_autoPublish_description": "Cuando se ejecuta la compilación, omite la verificación y publica la aplicación. Notificar al propietaro y a los revisores que la aplicación se publicó sin revisión.", + "project_autoPublish_description": "Cuando se ejecuta la compilación, omite la verificación y publica la aplicación. Notificar al propietario y a los revisores que la aplicación se publicó sin revisión.",
399-399: Fix missing accent in Spanish verb.Line 399 contains "esta" which should be "está" (present tense third-person singular of "estar" requires an accent mark).
- "project_acts_autoPublish_on": "Publicaciones automáticas esta activada", + "project_acts_autoPublish_on": "Publicaciones automáticas está activada",
🧹 Nitpick comments (2)
src/lib/components/settings/Toggle.svelte (2)
2-5: Clean up imports and satisfy import-order lint.Remove the unused IconContainer and place value imports before type-only imports to appease the linter.
-import type { HTMLInputAttributes } from 'svelte/elements'; -import IconContainer from '../IconContainer.svelte'; -import Icon from '@iconify/svelte'; -import InputWithMessage from './InputWithMessage.svelte'; -import type { ValueKey } from '$lib/locales.svelte'; +import Icon from '@iconify/svelte'; +import InputWithMessage from './InputWithMessage.svelte'; +import type { HTMLInputAttributes } from 'svelte/elements'; +import type { ValueKey } from '$lib/locales.svelte';
38-45: Avoid attribute collisions; let consumers override or merge classes.With class set after the spread, inputAttr.class is currently ignored. Either move the spread after class (to allow overrides) or explicitly merge classes by extracting class from inputAttr.
Minimal change to allow overrides:
- {...inputAttr} - class="checked:bg-accent checked:border-accent rounded-full" + class="toggle checked:bg-accent checked:border-accent rounded-full" + {...inputAttr}Alternative (merge classes):
<script lang="ts"> // ... let otherInputAttr: HTMLInputAttributes | undefined; let inputClass = ''; $effect.pre(() => { const { class: cls, ...rest } = inputAttr ?? {}; inputClass = cls ?? ''; otherInputAttr = rest as HTMLInputAttributes; }); </script> <input {name} type="checkbox" disabled={!canEdit} bind:checked class="toggle checked:bg-accent checked:border-accent rounded-full {inputClass}" {...otherInputAttr} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/lib/components/settings/PublicPrivateToggle.svelte(0 hunks)src/lib/components/settings/Toggle.svelte(1 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)
💤 Files with no reviewable changes (1)
- src/lib/components/settings/PublicPrivateToggle.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/(authenticated)/projects/new/[id=idNumber]/+page.svelte
- src/lib/projects/sse.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
🪛 GitHub Check: typecheck-lint
src/lib/components/settings/Toggle.svelte
[warning] 4-4:
@iconify/svelte import should occur before type import of svelte/elements
[warning] 3-3:
'IconContainer' is defined but never used
⏰ 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)
- GitHub Check: build-and-test
🔇 Additional comments (15)
src/routes/(authenticated)/projects/[id=idNumber]/+page.svelte (1)
242-242: LGTM: Settings API simplified.The removal of
publicEndpointanddownloadEndpointprops aligns with the migration to ToggleForm-based toggles with per-block endpoints. This simplification reduces coupling and improves component encapsulation.src/routes/(authenticated)/projects/[id=idNumber]/+page.server.ts (2)
321-342: LGTM: Toggle action correctly implemented.The
toggleAutoPublishOnRebuildaction follows the established pattern:
- Security check properly enforces project write access
- Form validation uses valibot schema for type safety
- Database update targets the correct field
The field naming convention (camelCase for form, PascalCase for database) is consistent with other actions in this file.
343-364: LGTM: Toggle action correctly implemented.The
toggleRebuildOnSoftwareUpdateaction mirrors thetoggleAutoPublishOnRebuildimplementation with proper security checks, form validation, and database updates.src/lib/locales/en-US.json (2)
25-26: LGTM: Formatting improved for consistency.The inline array format for
declarationsandselectorsimproves readability without changing semantics.Also applies to: 35-36, 45-46, 55-56, 249-250
369-375: LGTM: Translation keys support new auto-publish/rebuild features.The new keys are well-structured and follow the existing naming conventions. The descriptions clearly explain the feature behavior to users.
Note: Line 371 changes "Automatic Rebuilds" to "Automatic Rebuild" (singular), which may be intentional for consistency with the new "Rebuild on Software Update" feature.
Also applies to: 402-404
src/lib/locales/es-419.json (1)
25-26: LGTM: Formatting consistent with en-US.json.The inline array format maintains consistency across locale files.
Also applies to: 35-36, 45-46, 55-56, 249-250
src/lib/locales/fr-FR.json (1)
369-370: LGTM: Translation keys added for consistency.The new keys maintain consistency across locale files. Note that the values remain in English, which is consistent with other untranslated strings in this file.
Also applies to: 372-373, 398-400
src/routes/(authenticated)/organizations/[id=idNumber]/settings/products/+page.svelte (1)
6-6: LGTM: Migration to Toggle component.The replacement of
PublicPrivateTogglewith the more flexibleTogglecomponent is well-executed. The icon choices (mdi:lock-open-variantfor public,mdi:lockfor private) clearly communicate the toggle state.Also applies to: 36-47
src/routes/(authenticated)/admin/settings/organizations/edit/+page.svelte (1)
7-7: LGTM: Consistent Toggle migration.The migration from
PublicPrivateToggletoTogglefollows the same pattern as other pages in this PR, with appropriate icon choices and proper form binding.Also applies to: 103-111
src/lib/components/settings/ToggleForm.svelte (3)
11-39: LGTM: Well-defined component interface.The Props interface clearly defines all required and optional properties. The use of
ValueKeyfor title and message ensures type-safe localization.
48-64: Verify state management during error handling.The error handling at lines 59-60 reverts the toggle state with
formVar = !formVar. However, this might create unexpected behavior if the Toggle component has already updatedformVarvia thebind:checkeddirective before the form submission completes.Consider testing this flow: Toggle on → form submits → server returns error → state reverted. Ensure the UI correctly reflects the reverted state.
66-79: LGTM: Clean Toggle integration.The Toggle component is properly integrated with two-way binding and automatic form submission on change.
src/routes/(authenticated)/projects/[id=idNumber]/forms/Settings.svelte (3)
31-46: Public visibility block looks good.Bindings, messages, icons, and endpoint wiring read clean and consistent.
33-81: No issues found—canEdit properly disables inputs end-to-end.Verification confirms ToggleForm forwards
canEditto Toggle, which correctly setsdisabled={!canEdit}on the input element. Visual feedback (opacity-50, pointer-events-none) reinforces the disabled state. The implementation is correct.
20-24: All server actions are properly defined and exported.Verified that all four endpoints exist in
+page.server.ts:
toggleVisibility(line 277)toggleDownload(line 299)toggleAutoPublishOnRebuild(line 321)toggleRebuildOnSoftwareUpdate(line 343)All action names match exactly and are exported within the actions object, so no 404 risk.
|
Latest changes ensure the toggles grey out when not enabled, and the locked cursor appears correctly now. I also replaced the custom toggle setup with DaisyUI's toggle label. |
2 participants
Add the toggles necessary for automatic builds.
Addresses #1235
Summary by CodeRabbit
New Features
Refactor