Add ScalaJS engine#448
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a complete Scala.js sequencer engine ( ChangesScala.js Engine + Angular Integration
Sequence DiagramsequenceDiagram
participant User
participant SequencerComponent
participant SequencerService
participant SequencerEngine as SequencerEngine (Scala.js)
participant SequencerState as SequencerState (Scala)
User->>SequencerComponent: click undo/redo or select genre/beat
SequencerComponent->>SequencerService: dispatch({ type: 'UNDO' | 'SELECT_BEAT' | ... })
SequencerService->>SequencerEngine: SequencerEngine.dispatch(cmd)
SequencerEngine->>SequencerState: Command.fromJS(cmd) → state.dispatch(command)
SequencerState-->>SequencerEngine: new SequencerState
SequencerService->>SequencerEngine: SequencerEngine.getState()
SequencerEngine-->>SequencerService: {genre, beat, historyLength, futureLength}
SequencerService->>SequencerService: state$.next(newState)
SequencerComponent->>SequencerComponent: state$ subscription → cdr.markForCheck()
SequencerComponent-->>User: updated UI (beat, genre, undo/redo state)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
engine/src/test/scala/com/drumbeatrepo/sequencer/SequencerStateTest.scala (1)
7-25: ⚡ Quick winAdd regression tests for redo invalidation and clear/reset field mapping.
Current tests miss two key invariants: (1) new command after
Undomust clear redo future, and (2)ClearAllshould resetgenreandbeatto matching initial fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/test/scala/com/drumbeatrepo/sequencer/SequencerStateTest.scala` around lines 7 - 25, The test class SequencerStateTest is missing two regression tests for critical invariants in the SequencerState dispatch method. Add a new test that verifies when a new command (like SelectBeat with a different value) is dispatched after Undo, the redo future is cleared and cannot be recovered. Additionally, add a test that verifies the ClearAll command properly resets both the genre and beat fields back to their initial empty string values, confirming that ClearAll clears these state fields as expected. Both tests should follow the existing test pattern in the class using dispatch chains and shouldBe assertions.frontend/src/app/ui/components/sequencer/sequencer.service.ts (1)
13-18: Encapsulate the writable subject to prevent engine/UI state drift.
state$is publicly writable, allowing callers to bypassSequencerEngineinvariants (as seen in tests at line 38 in sequencer.service.spec.ts). The component only subscribes to it as an observable, so make the subject private and expose a read-only interface viaasObservable().♻️ Proposed refactor
export class SequencerService { - - state$ = new BehaviorSubject<SequencerState | null>(null); + private readonly stateSubject = new BehaviorSubject<SequencerState | null>(null); + readonly state$ = this.stateSubject.asObservable(); dispatch(cmd: Command) { SequencerEngine.dispatch(cmd); - this.state$.next(SequencerEngine.getState()); + this.stateSubject.next(SequencerEngine.getState()); } + + syncState(): void { + this.stateSubject.next(SequencerEngine.getState()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/ui/components/sequencer/sequencer.service.ts` around lines 13 - 18, The state$ BehaviorSubject property in the SequencerService is publicly writable, allowing callers to bypass the SequencerEngine invariants and directly modify state, as demonstrated by the tests directly writing to it. Make the state$ field private and expose a read-only observable interface to callers by creating a public method or getter property that returns state$.asObservable(). This ensures all state changes flow through the dispatch method, maintaining consistency with the SequencerEngine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/scala-test/action.yml:
- Around line 19-21: Add SHA-256 checksum verification for the sbt archive
download to prevent supply-chain security risks. After the curl command that
downloads the sbt release to /tmp/sbt.tgz, add a verification step that
downloads the corresponding SHA-256 checksum file and validates the archive
integrity using a command like sha256sum before proceeding to the tar extraction
command. This ensures the archive has not been tampered with or corrupted before
it is extracted into the system path.
In @.github/workflows/angular-deploy-github-pages.yml:
- Around line 33-37: The "Download Scala.js artifact" step references the
scalajs-artifact which is never produced during the workflow run because the
scala-test job that creates this artifact is not invoked. Add the scala-test job
to your workflow before this download step so that the scalajs-artifact is
actually generated and available for download-artifact to retrieve during the
current workflow run.
- Line 31: The cache-dependency-path parameter on line 31 of
.github/workflows/angular-deploy-github-pages.yml contains invalid YAML syntax
with comma-separated quoted strings. Remove the comma and the 'frontend/engine/'
path reference, keeping only 'frontend/package-lock.json' as the value, since
the frontend/engine/ directory does not contain package dependencies that need
to be cached.
In `@engine/src/main/scala/com/drumbeatrepo/sequencer/Command.scala`:
- Around line 13-28: The fromJS method unsafely accesses nested JavaScript
dynamic fields without validating their existence, which can crash with
uncontrolled errors if payload or genre/beat fields are missing. Add validation
before accessing cmd.selectDynamic("payload") and the subsequent genre/beat
fields in both the SELECT_GENRE and SELECT_BEAT case patterns. Use Option types
or explicit checks to safely extract these values, and handle missing fields by
either returning an error variant (if the Command type supports it) or throwing
a controlled exception that clearly describes the malformed input rather than
letting dynamic field access fail silently.
In `@engine/src/main/scala/com/drumbeatrepo/sequencer/SequencerState.scala`:
- Around line 22-23: The arguments to the constructor are being passed in the
wrong order. The constructor expects parameters in the order (genre, beat, ...),
but the code is passing SequencerState.initial.beat where genre is expected and
SequencerState.initial.genre where beat is expected. Swap the order of these two
arguments so that SequencerState.initial.genre is passed first and
SequencerState.initial.beat is passed second, matching the constructor parameter
order. This same swap needs to be applied at all locations where this reversed
order pattern occurs.
- Around line 11-26: The SelectGenre, SelectBeat, and ClearAll commands are
preserving the future (redo history) when creating new SequencerState instances.
When a user executes a new command after an Undo, the redo history should be
invalidated to prevent stale actions from being replayed. In each of these three
case statements (Command.SelectGenre, Command.SelectBeat, and Command.ClearAll),
replace the `future` parameter being passed to the new SequencerState
constructor with an empty collection (appropriate to the type of future, such as
an empty list or sequence) to clear the redo history.
In `@frontend/src/app/ui/components/sequencer/sequencer.component.html`:
- Around line 27-34: The click event handlers for the UNDO and REDO actions are
currently bound to the nested <img> elements instead of their parent <button>
elements. Move the (click)="sequencerService.dispatch({ type: 'UNDO'})" handler
from the undo icon <img> to the <button class="button play-sample-button undo">
element, and move the (click)="this.sequencerService.dispatch({ type: 'REDO'})"
handler from the redo icon <img> to the <button class="button play-sample-button
redo"> element. This will ensure keyboard activation (Enter/Space) works
properly and expands the clickable target area to the entire button rather than
just the icon bounds.
In `@frontend/src/app/ui/components/sequencer/sequencer.component.ts`:
- Around line 123-127: The initialization code unconditionally dispatches
SELECT_GENRE and SELECT_BEAT actions whenever beats are loaded, which overwrites
any existing SequencerService state and pollutes the undo history on remount.
Check the current state of SequencerService (checking for existing genre and
beat selections) before dispatching these actions. Only dispatch the actions if
the SequencerService does not already have valid state; if it does, skip the
dispatches to preserve the user's prior selection and maintain undo history
integrity.
In `@README.md`:
- Around line 19-32: The setup instructions assume the user returns to the
repository root between command blocks, but the path to the frontend directory
becomes invalid after the `cd engine/` command. Update the frontend setup
section to use `cd ../frontend/` instead of `cd frontend/` so that the command
works correctly when executed immediately after the engine setup commands. This
allows the full command sequence to be executed consecutively without requiring
the user to manually navigate back to the repo root.
---
Nitpick comments:
In `@engine/src/test/scala/com/drumbeatrepo/sequencer/SequencerStateTest.scala`:
- Around line 7-25: The test class SequencerStateTest is missing two regression
tests for critical invariants in the SequencerState dispatch method. Add a new
test that verifies when a new command (like SelectBeat with a different value)
is dispatched after Undo, the redo future is cleared and cannot be recovered.
Additionally, add a test that verifies the ClearAll command properly resets both
the genre and beat fields back to their initial empty string values, confirming
that ClearAll clears these state fields as expected. Both tests should follow
the existing test pattern in the class using dispatch chains and shouldBe
assertions.
In `@frontend/src/app/ui/components/sequencer/sequencer.service.ts`:
- Around line 13-18: The state$ BehaviorSubject property in the SequencerService
is publicly writable, allowing callers to bypass the SequencerEngine invariants
and directly modify state, as demonstrated by the tests directly writing to it.
Make the state$ field private and expose a read-only observable interface to
callers by creating a public method or getter property that returns
state$.asObservable(). This ensures all state changes flow through the dispatch
method, maintaining consistency with the SequencerEngine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12364031-c8ae-4cc7-a104-628772b49983
📒 Files selected for processing (30)
.github/actions/scala-test/action.yml.github/workflows/angular-deploy-github-pages.yml.github/workflows/tests.ymlREADME.mdengine/.gitignoreengine/.gitkeepengine/build.sbtengine/project/build.propertiesengine/project/plugins.sbtengine/src/main/scala/com/drumbeatrepo/sequencer/Command.scalaengine/src/main/scala/com/drumbeatrepo/sequencer/SequencerEngine.scalaengine/src/main/scala/com/drumbeatrepo/sequencer/SequencerState.scalaengine/src/test/scala/com/drumbeatrepo/sequencer/SequencerStateTest.scalafrontend/.gitignorefrontend/angular.jsonfrontend/eslint.config.mjsfrontend/src/app/domain/export-options/audio-export-options.tsfrontend/src/app/domain/export-options/midi-export-options.tsfrontend/src/app/infrastructure/adapters/audio-engine/audio-engine.adapter.tsfrontend/src/app/infrastructure/adapters/audio-export/audio-export.adapter.tsfrontend/src/app/infrastructure/adapters/utils/blob.utils.tsfrontend/src/app/ui/app.module.tsfrontend/src/app/ui/components/modals/export-audio-modal/export-audio-modal.component.spec.tsfrontend/src/app/ui/components/sequencer/sequencer.component.htmlfrontend/src/app/ui/components/sequencer/sequencer.component.spec.tsfrontend/src/app/ui/components/sequencer/sequencer.component.tsfrontend/src/app/ui/components/sequencer/sequencer.service.spec.tsfrontend/src/app/ui/components/sequencer/sequencer.service.tsfrontend/src/app/ui/router.spec.tsfrontend/src/types/engine.d.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/angular-deploy-github-pages.yml (1)
24-24: ⚡ Quick winConsider hardening checkout security.
Two security improvements recommended:
- Persist credentials: The GitHub token remains accessible in
.git/configand can be used by subsequent steps. Settingpersist-credentials: falselimits token exposure.- Pin to commit SHA: Using a mutable tag (
@v6) instead of an immutable commit hash introduces supply chain risk—the tag could be moved to malicious code.🔒 Proposed security hardening
steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@1e31de5234b9f8995739874a8ce0492dc87873e2 # v6.0.0 + with: + persist-credentials: false - uses: ./.github/actions/scala-test/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/angular-deploy-github-pages.yml at line 24, The checkout action at `actions/checkout@v6` needs two security hardening improvements: First, add `persist-credentials: false` as a parameter to the action to prevent the GitHub token from persisting in the `.git/config` file, which reduces token exposure to subsequent steps. Second, replace the mutable tag `@v6` with the immutable commit SHA of that specific version to eliminate supply chain risk and ensure you are always using the exact version you intend rather than allowing the tag to potentially point to malicious code in the future.Source: Linters/SAST tools
frontend/src/app/ui/components/sequencer/sequencer.component.html (1)
27-27: ⚡ Quick winUse consistent
this.prefix in template expressions.Line 27 omits
this.while line 32 includes it. For consistency and clarity, either usethis.sequencerService.dispatchin both places or omitthis.in both (Angular supports either style).✨ Suggested fix for consistency
- <button class="button play-sample-button undo" (click)="sequencerService.dispatch({ type: 'UNDO'})"> + <button class="button play-sample-button undo" (click)="this.sequencerService.dispatch({ type: 'UNDO'})">Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/ui/components/sequencer/sequencer.component.html` at line 27, The template uses inconsistent prefix style for sequencerService.dispatch calls. Line 27 omits the this. prefix while line 32 includes it. Update line 27 to add this. prefix before sequencerService.dispatch to match the style used on line 32, ensuring all template expressions use the same consistent approach throughout the sequencer component template.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/angular-deploy-github-pages.yml:
- Line 24: The checkout action at `actions/checkout@v6` needs two security
hardening improvements: First, add `persist-credentials: false` as a parameter
to the action to prevent the GitHub token from persisting in the `.git/config`
file, which reduces token exposure to subsequent steps. Second, replace the
mutable tag `@v6` with the immutable commit SHA of that specific version to
eliminate supply chain risk and ensure you are always using the exact version
you intend rather than allowing the tag to potentially point to malicious code
in the future.
In `@frontend/src/app/ui/components/sequencer/sequencer.component.html`:
- Line 27: The template uses inconsistent prefix style for
sequencerService.dispatch calls. Line 27 omits the this. prefix while line 32
includes it. Update line 27 to add this. prefix before sequencerService.dispatch
to match the style used on line 32, ensuring all template expressions use the
same consistent approach throughout the sequencer component template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f1bd235-af3d-47d5-a6bb-7901e8fe863d
📒 Files selected for processing (4)
.github/workflows/angular-deploy-github-pages.ymlREADME.mdfrontend/src/app/ui/components/sequencer/sequencer.component.htmlfrontend/src/app/ui/components/sequencer/sequencer.component.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/ui/components/sequencer/sequencer.component.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work comes from a simple observation: modeling a stateful editor with immutable updates becomes increasingly cumbersome as the domain grows.
The command pattern, undo/redo history, MIDI transformations and state transitions feel much more natural to model with Scala's immutable data structures and algebraic data types.
So I decided to experiment with Scala.js for the domain layer and keep Angular focused on the UI.
The fun part is implementing the command engine. The less fun part is the plumbing between Scala.js and Angular 😄.
I don't know yet if this is the right long-term solution, but I like the direction. It gives me a strongly-typed domain model today, and if I decide to add a Scala backend in the coming months, I'll already have a shared language and a shared model for the application.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
CI / Build