Refacto : rename fileName to filename and introduce a new json load fun - #522
Merged
Conversation
…d function in types
Contributor
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR standardizes ChangesFilename Property Consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Babali42
changed the title
![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: 4
🧹 Nitpick comments (1)
frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts (1)
12-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the lookup mock honor its filename argument.
This stub returns the same beat for every
fileName. Tests forBeatAdapter.getBeatByFileNamecan therefore pass when the adapter forwards the wrong name. Return the fixture only for the expected asset path and returnOption.none()for unknown files.🤖 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/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts` around lines 12 - 13, Update the loadJsonByFileName mock to inspect its fileName argument, returning the existing CompactBeat fixture only for the expected asset path and Option.none() for all other filenames, so BeatAdapter.getBeatByFileName tests validate the forwarded name.
🤖 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
`@frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts`:
- Line 30: Update both fixture path references in the beat adapter service spec
to use metal/crash.mp3 via toMp3FilePath, replacing the misspelled
metal/carsh.mp3 value and preserving the matching asset path.
In
`@frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.ts`:
- Around line 16-20: Update getBeatByFileName to handle the Option returned by
loadJsonByFileName without throwing: check Option.isNone(beat), return
Effect.fail with an Error for None, and call
CompactBeatMapper.toBeatEffect(beat.value) only for Some values.
In
`@frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts`:
- Around line 13-15: Implement JsonFilesReaderService.loadJsonByFileName by
normalizing the filename to remove an optional .json suffix, delegating to
loadAllBeats (which appends the suffix), and returning the matching beat as an
Effect<Option<CompactBeat>, never> instead of throwing. Preserve Option.none
when no beat matches.
In `@frontend/src/app/ui/components/sequencer/sequencer.component.ts`:
- Line 238: Update the export tests for both handlers in the sequencer component
spec to provide valid filename values in their options, then assert that
downloadBlob is called with the corresponding blob and filename. Cover both
handler paths so the options.filename rename is verified and no test invokes
them with an empty options object.
---
Nitpick comments:
In
`@frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts`:
- Around line 12-13: Update the loadJsonByFileName mock to inspect its fileName
argument, returning the existing CompactBeat fixture only for the expected asset
path and Option.none() for all other filenames, so BeatAdapter.getBeatByFileName
tests validate the forwarded name.
🪄 Autofix
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 Plus
Run ID: 44cafea8-7820-4188-b481-eaaa581b0d9e
📒 Files selected for processing (64)
.devcontainer/scala-container/devcontainer-lock.jsonengine/src/main/scala/com/drumbeatrepo/sequencer/Track.scalaengine/src/test/scala/com/drumbeatrepo/sequencer/SequencerStateTest.scalafrontend/src/app/domain/export-options/audio-export-options.tsfrontend/src/app/domain/export-options/midi-export-options.tsfrontend/src/app/domain/ports/i-manage-beats.tsfrontend/src/app/domain/track.tsfrontend/src/app/infrastructure/adapters/audio-engine/audio-engine.adapter.spec.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/beat-source/beat-adapter.service.spec.tsfrontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.tsfrontend/src/app/infrastructure/adapters/beat-source/compact-beat.mapper.spec.tsfrontend/src/app/infrastructure/adapters/beat-source/compact-beat.mapper.tsfrontend/src/app/infrastructure/adapters/beat-source/compact-track.tsfrontend/src/app/infrastructure/adapters/beat-source/json-files-reader.interface.tsfrontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.spec.tsfrontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.tsfrontend/src/app/ui/components/modals/export-audio-modal/export-audio-modal.component.htmlfrontend/src/app/ui/components/modals/export-audio-modal/export-audio-modal.component.spec.tsfrontend/src/app/ui/components/modals/export-audio-modal/export-audio-modal.component.tsfrontend/src/app/ui/components/modals/export-midi-modal/export-midi-modal.component.htmlfrontend/src/app/ui/components/modals/export-midi-modal/export-midi-modal.component.spec.tsfrontend/src/app/ui/components/modals/export-midi-modal/export-midi-modal.component.tsfrontend/src/app/ui/components/sequencer/sequencer.component.spec.tsfrontend/src/app/ui/components/sequencer/sequencer.component.tsfrontend/src/app/ui/services/sequencer/sequencer.service.spec.tsfrontend/src/app/ui/services/sequencer/sequencer.service.tsfrontend/src/assets/beats/dancehall/modern.jsonfrontend/src/assets/beats/dancehall/reggaeton.jsonfrontend/src/assets/beats/dancehall/standard.jsonfrontend/src/assets/beats/dnb/dnb.jsonfrontend/src/assets/beats/dub/dub.jsonfrontend/src/assets/beats/ebm/ebm.jsonfrontend/src/assets/beats/hardcore-techno/gabber.jsonfrontend/src/assets/beats/hip-hop/jul.jsonfrontend/src/assets/beats/hip-hop/trap.jsonfrontend/src/assets/beats/hypnotic-techno/son-clave.jsonfrontend/src/assets/beats/hypnotic-techno/tresillo.jsonfrontend/src/assets/beats/jersey-club/jersey-club.jsonfrontend/src/assets/beats/metal/half-time-groove.jsonfrontend/src/assets/beats/metal/metal-blastbeat.jsonfrontend/src/assets/beats/metal/metal.jsonfrontend/src/assets/beats/psytrance/psytrance-ternary.jsonfrontend/src/assets/beats/psytrance/psytrance-variation.jsonfrontend/src/assets/beats/psytrance/psytrance.jsonfrontend/src/assets/beats/punk/punk-beat-eight-note-fill.jsonfrontend/src/assets/beats/punk/punk-beat-quarter-note-groove-variation.jsonfrontend/src/assets/beats/punk/punk-beat-quarter-note-groove.jsonfrontend/src/assets/beats/rock/rock.jsonfrontend/src/assets/beats/rock/variation.jsonfrontend/src/assets/beats/techno/off-beat-clap.jsonfrontend/src/assets/beats/techno/techno.jsonfrontend/src/assets/i18n/ar.jsonfrontend/src/assets/i18n/de.jsonfrontend/src/assets/i18n/en.jsonfrontend/src/assets/i18n/es.jsonfrontend/src/assets/i18n/fr.jsonfrontend/src/assets/i18n/it.jsonfrontend/src/assets/i18n/ja.jsonfrontend/src/assets/i18n/pt.jsonfrontend/src/assets/i18n/ru.jsonfrontend/src/assets/i18n/zh.jsonfrontend/src/types/engine.d.ts
| { | ||
| "name": "Hats", | ||
| "midiNote": 38, | ||
| "filename": toMp3FilePath("metal/carsh.mp3"), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 2 'carsh\.mp3|crash\.mp3' frontendRepository: Babali42/DrumBeatRepo
Length of output: 3336
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- matching asset files ---'
find frontend/src/assets -type f \( -iname '*crash*.mp3' -o -iname '*carsh*.mp3' \) -print 2>/dev/null || true
printf '%s\n' '--- beat adapter spec ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts
printf '%s\n' '--- path helper and reader references ---'
rg -n -C 3 'toMp3FilePath|loadJsonByFileName|metal/(carsh|crash)\.mp3' frontend/src/app frontend/src/assetsRepository: Babali42/DrumBeatRepo
Length of output: 26069
Correct both fixture paths to metal/crash.mp3. The only matching asset is frontend/src/assets/sounds/metal/crash.mp3; carsh.mp3 does not exist.
🤖 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/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts`
at line 30, Update both fixture path references in the beat adapter service spec
to use metal/crash.mp3 via toMp3FilePath, replacing the misspelled
metal/carsh.mp3 value and preserving the matching asset path.
Comment on lines
+16
to
+20
| getBeatByFileName(name: string): Effect.Effect<Beat, Error> { | ||
| return Effect.flatMap( | ||
| this.jsonFileReader.loadJsonByFileName(name), | ||
| beat => CompactBeatMapper.toBeatEffect(Option.getOrThrow(beat)) | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C 3 'Option\.getOrThrow|Option\.isNone|Effect\.fail|loadJsonByFileName' frontend --glob '*.ts' || trueRepository: Babali42/DrumBeatRepo
Length of output: 12794
🏁 Script executed:
#!/usr/bin/env bash
set -e
printf '%s\n' '--- beat adapter ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.ts
printf '%s\n' '--- reader interface and implementation ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.interface.ts
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts
printf '%s\n' '--- reader tests ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.spec.ts
printf '%s\n' '--- adapter tests ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.spec.ts
printf '%s\n' '--- package metadata ---'
fd -i 'package.json' -a -t f -x sh -c 'echo --- $1; rg -n "\"effect\"|\"typescript\"" "$1"' sh {}Repository: Babali42/DrumBeatRepo
Length of output: 13846
🏁 Script executed:
#!/usr/bin/env bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
adapter = Path("frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.ts").read_text()
interface = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.interface.ts").read_text()
reader = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts").read_text()
tests = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.spec.ts").read_text()
checks = {
"reader contract returns Option": bool(re.search(
r"loadJsonByFileName\(fileName: string\):\s*Effect\.Effect<Option\.Option<CompactBeat>, never>",
interface)),
"adapter uses getOrThrow in single-beat path": "Option.getOrThrow(beat)" in adapter,
"reader converts failed loads to Option.none": "Effect.option(" in reader,
"reader test covers failed load": "Promise.reject(new Error('404'))" in tests,
"reader implementation is currently unimplemented": "throw new Error('Method not implemented.')" in reader,
}
for name, result in checks.items():
print(f"{name}: {'yes' if result else 'no'}")
assert checks["reader contract returns Option"]
assert checks["adapter uses getOrThrow in single-beat path"]
assert checks["reader converts failed loads to Option.none"]
assert checks["reader test covers failed load"]
PYRepository: Babali42/DrumBeatRepo
Length of output: 383
🏁 Script executed:
#!/usr/bin/env bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
adapter = Path("frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.ts").read_text()
interface = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.interface.ts").read_text()
reader = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts").read_text()
tests = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.spec.ts").read_text()
checks = {
"reader contract returns Option": bool(re.search(
r"loadJsonByFileName\(fileName: string\):\s*Effect\.Effect<Option\.Option<CompactBeat>, never>",
interface)),
"adapter uses getOrThrow in single-beat path": "Option.getOrThrow(beat)" in adapter,
"reader converts failed loads to Option.none": "Effect.option(" in reader,
"reader test covers failed load": "Promise.reject(new Error('404'))" in tests,
"reader implementation is currently unimplemented": "throw new Error('Method not implemented.')" in reader,
}
for name, result in checks.items():
print(f"{name}: {'yes' if result else 'no'}")
assert checks["reader contract returns Option"]
assert checks["adapter uses getOrThrow in single-beat path"]
assert checks["reader converts failed loads to Option.none"]
assert checks["reader test covers failed load"]Repository: Babali42/DrumBeatRepo
Length of output: 470
Return an Error effect for Option.none.
loadJsonByFileName returns Effect<Option<CompactBeat>, never>, and failed loads can produce Option.none. When beat is None, Option.getOrThrow(beat) throws outside the declared failure channel. Check Option.isNone(beat) and return Effect.fail(...); call CompactBeatMapper.toBeatEffect(beat.value) only for Some.
🤖 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/infrastructure/adapters/beat-source/beat-adapter.service.ts`
around lines 16 - 20, Update getBeatByFileName to handle the Option returned by
loadJsonByFileName without throwing: check Option.isNone(beat), return
Effect.fail with an Error for None, and call
CompactBeatMapper.toBeatEffect(beat.value) only for Some values.
Comment on lines
+13
to
+15
| loadJsonByFileName(filename: string): Effect.Effect<Option.Option<CompactBeat>, never> { | ||
| throw new Error('Method not implemented.'); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 '\b(loadJsonByFileName|getBeatByFileName)\s*\(' frontend --glob '*.ts'
rg -n -C 3 'loadAllBeats|/assets/beats/' frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.tsRepository: Babali42/DrumBeatRepo
Length of output: 4153
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- reader service ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts
printf '%s\n' '--- adapter and interface ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/beat-adapter.service.ts
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.interface.ts
printf '%s\n' '--- all filename call sites and beat asset references ---'
rg -n -C 4 '\bgetBeatByFileName\s*\(|\bloadJsonByFileName\s*\(|/assets/beats/|loadAllBeats\s*\(' frontend --glob '*.ts' --glob '*.html' --glob '*.json'
printf '%s\n' '--- beat asset filenames, if tracked ---'
git ls-files | rg '(^|/)assets/beats/|beats/.*\.json$' || trueRepository: Babali42/DrumBeatRepo
Length of output: 14839
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- reader tests ---'
cat -n frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.spec.ts
printf '%s\n' '--- repository-wide lookup callers ---'
rg -n -C 5 '\bgetBeatByFileName\s*\(|\bloadJsonByFileName\s*\(' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'
printf '%s\n' '--- filename literals and route-like beat references ---'
rg -n -C 2 'techno\.json|/assets/beats/|filename\s*[:=]|fileName\s*[:=]' frontend --glob '!*.spec.ts'
printf '%s\n' '--- compact verifier for URL construction and asset matches ---'
python3 - <<'PY'
from pathlib import Path
import re
service = Path("frontend/src/app/infrastructure/adapters/beat-source/json-files-reader.service.ts").read_text()
assets = {
str(p.relative_to("frontend/src/assets/beats/"))[:-5]
for p in Path("frontend/src/assets/beats").rglob("*.json")
}
files = re.findall(r"\.concat\((.*?)\)", service, re.S)
entries = []
for group in files:
entries.extend(re.findall(r"'([^']+)'", group))
first = re.search(r"const files = \['([^']+)'\]", service)
if first:
entries.insert(0, first.group(1))
print("loadAllJson entries:", len(entries))
print("asset matches without suffix:", sum(x in assets for x in entries), "/", len(entries))
print("asset matches with suffix:", sum((x + ".json") in assets for x in entries), "/", len(entries))
for supplied in ("techno", "techno.json"):
url_key = supplied + ".json"
print(f"{supplied!r} -> /assets/beats/{url_key}")
print(" tracked asset:", url_key in {x + ".json" for x in assets})
PYRepository: Babali42/DrumBeatRepo
Length of output: 20685
Implement loadJsonByFileName before merging.
The method throws before creating an Effect, so BeatAdapter.getBeatByFileName cannot load any beat. Delegate to loadAllBeats and normalize an optional .json suffix because loadAllBeats appends that suffix.
🤖 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/infrastructure/adapters/beat-source/json-files-reader.service.ts`
around lines 13 - 15, Implement JsonFilesReaderService.loadJsonByFileName by
normalizing the filename to remove an optional .json suffix, delegating to
loadAllBeats (which appends the suffix), and returning the matching beat as an
Effect<Option<CompactBeat>, never> instead of throwing. Preserve Option.none
when no beat matches.
| ); | ||
|
|
||
| downloadBlob(blob, options.fileName); | ||
| downloadBlob(blob, options.filename); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass a filename in the export tests.
The handlers now read options.filename, but frontend/src/app/ui/components/sequencer/sequencer.component.spec.ts invokes both handlers with empty option objects at Line 215 and Line 226. Pass valid filename values and assert that downloadBlob receives them. Otherwise, the tests do not cover the rename and can exercise an undefined download name.
Also applies to: 250-250
🤖 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.ts` at line 238,
Update the export tests for both handlers in the sequencer component spec to
provide valid filename values in their options, then assert that downloadBlob is
called with the corresponding blob and filename. Cover both handler paths so the
options.filename rename is verified and no test invokes them with an empty
options object.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
…d function in types
Summary by CodeRabbit