Fix frontend bugs/UX bucket from audit (#531)#538
Merged
Conversation
…sist api key (#514) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #514 fix declared two local copies of the '********' sentinel; import the existing exported REDACTED_SENTINEL from models/config instead, so the backend-coupled value lives in one place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-pair toggle errors Review found the #516 commit only covered the SSE-handler JSON.parse; the issue's other acceptance criteria were unaddressed. Complete them: - autoqueue.service: guard the patterns JSON.parse (log + empty fallback, no throw) - logs page: distinguish a history-fetch failure from an empty result via a historyLoadFailed flag + a distinct "failed to load" template state - path-pairs: enable/auto-queue toggle failures now set an error banner and refresh() to reconcile the one-way-bound checkbox with the server Tests added for each. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request refactors Angular file-action handlers to emit event objects with callbacks, adds inline delete confirmations, guards SSE-payload parsing across multiple services, implements exponential backoff for stream reconnects, and ensures API key values persist without being overwritten by redaction sentinels. ChangesFile actions with event payloads and parent error handling
Stream backoff and configuration API key handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/angular/src/app/services/files/model-file.service.ts (1)
87-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the entire SSE event handling in model-file.service.ts (not just
JSON.parse).
modelFileFromJsoniteratesjson.childrenand reads other fields without validation, so valid JSON with the wrong shape (e.g.,{}formodel-init, or missingnew_file/old_file) can throw during the mapping/update logic. UnlikeLogServiceandServerStatusService(which catch both parse +*FromJsonmapping),ModelFileService.parseEvent()only catchesJSON.parse, andStreamDispatchServicecallsonEvent(...)without a try/catch, so the exception can escape the SSE listener. Wrap the event-specific processing (init/added/removed/updated) in the same try/catch and log/skip bad events.🛡️ Proposed fix: wrap event processing in the same guard
let parsed: unknown; try { parsed = JSON.parse(data); } catch (e) { this.logger.error('Failed to parse %s event: %O', name, e); return; } + try { if (name === this.EVENT_INIT) { const init = parsed as ModelFileJson[]; // ... } else if (name === this.EVENT_ADDED) { // ... } else if (name === this.EVENT_REMOVED) { // ... } else if (name === this.EVENT_UPDATED) { // ... } else { this.logger.error('Unrecognized event:', name); } + } catch (e) { + this.logger.error('Failed to handle %s event: %O', name, e); + }🤖 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 `@src/angular/src/app/services/files/model-file.service.ts` around lines 87 - 148, The SSE handling currently only wraps JSON.parse but not the downstream mapping, so calls to modelFileFromJson (and subsequent logic using fileKey, EVENT_INIT, EVENT_ADDED, EVENT_REMOVED, EVENT_UPDATED) can throw on malformed-but-parseable payloads; update parseEvent() (or the method handling the SSE data) to wrap the entire switch/if-block that handles name === EVENT_INIT / EVENT_ADDED / EVENT_REMOVED / EVENT_UPDATED in the same try/catch used for JSON.parse, so any exception from modelFileFromJson or the map updates is caught, logged via this.logger.error with the event name and error, and the handler returns/continues without mutating filesSubject or letting the error escape. Ensure logs mirror existing style and do not change behavior for valid events.
🤖 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 `@src/angular/src/app/pages/logs/logs-page.component.html`:
- Line 29: The error message paragraph (<p class="record error">Failed to load
log history. Check the connection and try again.</p>) is currently only visual;
update that element in logs-page.component.html to be announced to assistive
tech by adding an appropriate accessibility attribute such as role="alert" or
aria-live="assertive" (or aria-live="polite" if less urgent) so screen readers
will notify users when the fetch fails.
In `@src/angular/src/app/pages/logs/logs-page.component.spec.ts`:
- Around line 44-60: Add a spec that exercises the real fetch-failure path by
spying on the service method used by the component (spyOn(LogService,
'fetchHistory') or the actual service instance used in the test) to return an
observable that errors, then run the component's fetch trigger under fakeAsync
and advance time past the component's debounce interval with tick(...), call
fixture.detectChanges(), and assert that renderedText() contains 'Failed to load
log history' and that component.historyLoadFailed is true; reference the
component.fetchHistory()/ngOnInit() flow and the LogService.fetchHistory spy so
the test fails if the catchError pipeline stops setting the failure flag.
- Around line 19-29: Tests fail because LogsPageComponent now injects
LoggerService but the TestBed configuration doesn't mock it; update the
TestBed.configureTestingModule providers to include a mocked LoggerService
(useValue with vi.fn()-backed methods used by the component) so the spec uses an
explicit vi.fn() mock instead of the real root service; reference
LogsPageComponent and LoggerService in the TestBed.configureTestingModule call
and add the minimal mocked methods the component calls (e.g., log/debug/info or
any method names used) to the mock's useValue.
In `@src/angular/src/app/services/base/stream-dispatch.service.spec.ts`:
- Around line 271-282: The test currently mocks Math.random() to 1 which is
impossible in production because nextRetryDelayMs() calculates jitter as
Math.random() * RETRY_JITTER_MS; change the test to mock Math.random() to a
value just below 1 (e.g., 0.999) so jitter is < RETRY_JITTER_MS and then adjust
the timer expectations to match base delay + jitter (use service.start(),
latestEventSource(), and MockEventSource.instances to verify reconnection
timing); ensure references to nextRetryDelayMs and RETRY_JITTER_MS are
considered when computing the new advanceTimersByTime values.
In `@src/angular/src/app/services/files/model-file.service.spec.ts`:
- Around line 233-303: Add tests that cover valid-JSON but wrong-shape payloads
(not just invalid JSON) for the model-file SSE handlers: call
service.onEvent("model-init", "{}") and service.onEvent("model-init", "[]") and
assert they don't throw, that mockLogger.error was called, and that files$ state
is preserved; similarly call service.onEvent("model-added", JSON.stringify({}))
and service.onEvent("model-updated", JSON.stringify({})) and
service.onEvent("model-removed", JSON.stringify({})) (or payloads missing
new_file/old_file fields) and assert no throw, error logged, and map unchanged
(use makeFileJson to pre-seed state where needed); also add a test showing a
subsequent valid model-init/model-added still parses correctly after a
wrong-shaped payload to ensure no poisoned state.
In `@src/angular/src/app/services/settings/config.service.spec.ts`:
- Around line 122-127: The test replaces sessionStorage using
Object.defineProperty(globalThis, "sessionStorage", ...) but only saved and
restored the value (originalSessionStorage), which loses the original property
descriptor (accessor vs data property); capture the full original descriptor
with Object.getOwnPropertyDescriptor(globalThis, "sessionStorage") and use
Object.defineProperty to restore that original descriptor in afterEach; when
installing the mock, you can still define the mock via Object.defineProperty but
ensure you record the originalDescriptor variable and restore it (do the same
fix for the second hook around lines 154-159), referencing the existing
originalSessionStorage/mockSessionStorage and the
Object.defineProperty(globalThis, "sessionStorage", ...) calls to locate where
to change.
- Around line 246-259: Replace the hardcoded redaction literal "********" in
this spec with the shared constant REDACTED_SENTINEL from
src/angular/src/app/models/config.ts; update the two expectations that reference
"********" (the negative assertion on mockStreamDispatch.setApiKey and any other
literal checks in this test) to use REDACTED_SENTINEL so the test uses the
canonical sentinel, leaving other references (StorageKeys.API_KEY,
mockStreamDispatch.setApiKey, createService) unchanged.
In `@src/angular/src/app/services/settings/config.service.ts`:
- Around line 96-102: Do not persist the raw API key in client-side storage:
stop writing the real key into sessionStorage in the block that handles section
=== 'web' && option === 'api_key' by removing or changing the call to
writePersistedApiKey(realKey); instead either (a) send the real key to the
backend to mint a short-lived session credential or HttpOnly cookie and persist
that, or (b) persist only a redacted/hashed identifier (not the secret) and
continue to call setStreamApiKey(realKey) in-memory; update or remove
writePersistedApiKey so it no longer stores the unredacted key, and add
server-side logic to return a recoverable session token if reload recovery is
required.
---
Outside diff comments:
In `@src/angular/src/app/services/files/model-file.service.ts`:
- Around line 87-148: The SSE handling currently only wraps JSON.parse but not
the downstream mapping, so calls to modelFileFromJson (and subsequent logic
using fileKey, EVENT_INIT, EVENT_ADDED, EVENT_REMOVED, EVENT_UPDATED) can throw
on malformed-but-parseable payloads; update parseEvent() (or the method handling
the SSE data) to wrap the entire switch/if-block that handles name ===
EVENT_INIT / EVENT_ADDED / EVENT_REMOVED / EVENT_UPDATED in the same try/catch
used for JSON.parse, so any exception from modelFileFromJson or the map updates
is caught, logged via this.logger.error with the event name and error, and the
handler returns/continues without mutating filesSubject or letting the error
escape. Ensure logs mirror existing style and do not change behavior for valid
events.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1387d272-a733-4806-890b-bb4339a11981
📒 Files selected for processing (26)
src/angular/src/app/common/storage-keys.tssrc/angular/src/app/pages/files/bulk-action-bar.component.spec.tssrc/angular/src/app/pages/files/bulk-action-bar.component.tssrc/angular/src/app/pages/files/file-list.component.spec.tssrc/angular/src/app/pages/files/file-list.component.tssrc/angular/src/app/pages/files/file.component.spec.tssrc/angular/src/app/pages/files/file.component.tssrc/angular/src/app/pages/logs/logs-page.component.htmlsrc/angular/src/app/pages/logs/logs-page.component.spec.tssrc/angular/src/app/pages/logs/logs-page.component.tssrc/angular/src/app/pages/settings/path-pairs.component.spec.tssrc/angular/src/app/pages/settings/path-pairs.component.tssrc/angular/src/app/services/autoqueue/autoqueue.service.spec.tssrc/angular/src/app/services/autoqueue/autoqueue.service.tssrc/angular/src/app/services/base/stream-dispatch.service.spec.tssrc/angular/src/app/services/base/stream-dispatch.service.tssrc/angular/src/app/services/files/model-file.service.spec.tssrc/angular/src/app/services/files/model-file.service.tssrc/angular/src/app/services/logs/log.service.spec.tssrc/angular/src/app/services/logs/log.service.tssrc/angular/src/app/services/server/server-status.service.spec.tssrc/angular/src/app/services/server/server-status.service.tssrc/angular/src/app/services/settings/config.service.spec.tssrc/angular/src/app/services/settings/config.service.tssrc/angular/src/app/services/utils/api-key.interceptor.spec.tssrc/angular/src/app/services/utils/api-key.interceptor.ts
CodeQL flagged the #514 sessionStorage persistence of web.api_key as high-severity clear-text storage of sensitive information. There is no safe way to auto-recover the key after a reload (the auth-exempt /server/config/get redacts it; an authenticated endpoint recreates the deadlock), so drop the client-side persistence entirely. Kept: the core deadlock fix — config is still bootstrapped independent of the SSE stream (UI renders instead of hanging), the redacted sentinel is never sent as a credential, and reconnect backoff stays. After a reload on an auth-enabled instance the user re-enters the key in Settings (which re-pushes it to the stream). Removed the sessionStorage read/write, StorageKeys.API_KEY, and the interceptor's persisted-key lookup, plus the persistence tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #516 "failed to load" banner reused class="record error", which the Playwright log-record selector (#logs .record) matched — so when the history fetch errored in CI, the banner was read as a log record (and an ERROR-level one), breaking logs E2E tests. Give it a dedicated history-load-error class so it never collides with actual log-record queries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/angular/src/app/services/settings/config.service.ts (1)
102-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the in-memory API key across config refreshes.
Line 105 replaces
configSubjectwith the redacted/server/config/getpayload. Afterset("web", "api_key", "real-key"), the next successful refresh storesREDACTED_SENTINELinconfigSnapshot, soapiKeyInterceptorstops sendingX-Api-Keyon every later/server/*request. Lines 109-113 have the same downstream effect on transient refresh failures by dropping the snapshot tonull. Keep the last known non-redacted key in memory when publishing refresh results, or store it separately from the fetched config.Proposed fix
private getConfig(): void { this.logger.debug('Getting config...'); this.restService.sendRequest(this.CONFIG_GET_URL).subscribe({ next: (reaction) => { + const previousConfig = this.configSubject.getValue(); + const previousApiKey = previousConfig?.web?.api_key; + if (reaction.success) { try { const configJson: Config = JSON.parse(reaction.data!); - this.configSubject.next(configJson); - this.syncStreamApiKey(configJson); + const nextConfig = + configJson.web.api_key === REDACTED_SENTINEL && + previousApiKey && + previousApiKey !== REDACTED_SENTINEL + ? { + ...configJson, + web: { ...configJson.web, api_key: previousApiKey }, + } + : configJson; + + this.configSubject.next(nextConfig); + this.syncStreamApiKey(nextConfig); } catch (e) { this.logger.error('Failed to parse config: %O', e); - this.configSubject.next(null); + if (!previousConfig) { + this.configSubject.next(null); + } } } else { this.logger.error('Failed to get config: %s', reaction.errorMessage); - this.configSubject.next(null); + if (!previousConfig) { + this.configSubject.next(null); + } } }, }); }🤖 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 `@src/angular/src/app/services/settings/config.service.ts` around lines 102 - 113, The service currently overwrites the in-memory config/api key on every refresh (in the block handling reaction.success and the else branch), causing the real API key to be lost when the server returns REDACTED_SENTINEL or on transient failures; modify how configSubject and syncStreamApiKey handle the api key by preserving the lastKnownApiKey separately (e.g., a private lastKnownApiKey field) or by changing syncStreamApiKey to only update the stored key when configJson.web.api_key is a real value (not REDACTED_SENTINEL/null), and on non-success responses avoid clearing the stored snapshot/key (keep the previous configSubject value) so apiKeyInterceptor continues sending the real X-Api-Key.
🤖 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 `@src/angular/src/app/services/settings/config.service.spec.ts`:
- Around line 203-214: Add a regression test that exercises the sequence
set("web","api_key","new-key") followed by a refresh response containing
REDACTED_SENTINEL: call the service's set() helper to push "new-key", then
mockRestService.sendRequest to return a successful get with data containing the
redacted sentinel and call createService()/trigger the refresh; assert that
mockStreamDispatch.setApiKey was called with "new-key" and not with
REDACTED_SENTINEL and that the service's config snapshot (used by
apiKeyInterceptor) still retains the real key; reference the set() helper,
REDACTED_SENTINEL, mockRestService.sendRequest, createService,
mockStreamDispatch.setApiKey and the configSnapshot/apiKeyInterceptor in the
test.
---
Outside diff comments:
In `@src/angular/src/app/services/settings/config.service.ts`:
- Around line 102-113: The service currently overwrites the in-memory config/api
key on every refresh (in the block handling reaction.success and the else
branch), causing the real API key to be lost when the server returns
REDACTED_SENTINEL or on transient failures; modify how configSubject and
syncStreamApiKey handle the api key by preserving the lastKnownApiKey separately
(e.g., a private lastKnownApiKey field) or by changing syncStreamApiKey to only
update the stored key when configJson.web.api_key is a real value (not
REDACTED_SENTINEL/null), and on non-success responses avoid clearing the stored
snapshot/key (keep the previous configSubject value) so apiKeyInterceptor
continues sending the real X-Api-Key.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6d67d38-7092-4061-8f01-d24de41beafe
📒 Files selected for processing (6)
src/angular/src/app/pages/logs/logs-page.component.htmlsrc/angular/src/app/pages/logs/logs-page.component.scsssrc/angular/src/app/services/settings/config.service.spec.tssrc/angular/src/app/services/settings/config.service.tssrc/angular/src/app/services/utils/api-key.interceptor.spec.tssrc/angular/src/app/services/utils/api-key.interceptor.ts
- config.service: a redacted /server/config/get refresh no longer clobbers the real api_key in the snapshot, which the apiKeyInterceptor reads for REST auth (would break authed REST calls mid-session). Preserve the real key when the refresh returns the sentinel. + regression test. - model-file.service: the SSE guard wrapped only JSON.parse; a valid-JSON but wrong-shape event (model-init not an array, model-added missing new_file) still threw in the mapping. Guard the dispatch too (log + skip). + wrong-shape tests. - logs page: role="alert" on the history-load error banner (a11y announcement). - logs spec: mock LoggerService in TestBed; add a real debounced-pipe fetch-failure test (fake timers) that fails if catchError stops setting the flag. - config spec: use the REDACTED_SENTINEL constant instead of literal "********". - stream-dispatch spec: Math.random() mocked to 0.999 (it's [0,1), never 1). Skipped (already resolved by the earlier persistence removal): the CodeQL clear-text-storage alert, "don't persist the API key", and the sessionStorage descriptor restore — that code path no longer exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Fixes the 🖥️ Frontend bugs / UX bucket of tracking issue #531 (4 issues) in one branch, the sibling to the backend bucket (#537). Each fix has its own commit and Vitest tests.
Note
Intentional grouping. This bundles 4 issues, deviating from CLAUDE.md's "one concern per branch" rule — done deliberately at the maintainer's request for a single grouped review. Commits are one-per-issue (plus review-polish commits).
How this was built
A multi-agent workflow: parallel investigation (one agent per issue, re-verifying each finding against current Angular code), sequential implementation (ordered so the two issues sharing
file-list.component.ts— #513, #515 — ran adjacently and conflict-free; each = fix + Vitest tests + its own commit, validated withng test+ng lint --max-warnings 0), then parallel adversarial review. The review surfaced gaps, which were then addressed (below).Issues fixed
NotificationServiceDANGER banner, and a failed action no longer leaves the row's buttons/spinner stuck (activeActionis reset).JSON.parseguarded across the SSE/autoqueue handlers; log-history fetch failure shows a distinct "failed to load" state (not an empty result); path-pair enable/auto-queue toggle failures set an error banner andrefresh()to reconcile the UI.Review findings → resolution
Adversarial review flagged two items, both resolved in this PR (no follow-ups):
acMet=false): scope gap. The initial commit covered only the SSE-handlerJSON.parse. Completed the issue's other acceptance criteria — autoqueue parse guard, the log-history failure state, and path-pair toggle error handling +refresh()— each with tests.'********'sentinel; now imports the existing exportedREDACTED_SENTINELfrommodels/config.#513 and #515 passed review with all acceptance criteria met.
Test plan
npx ng lint --max-warnings 0: clean.Security review (CodeQL)
CodeQL flagged an initial #514 approach that persisted
web.api_keyinsessionStorageas high-severity clear-text storage of a credential. Since there is no safe way to auto-recover the key after a reload (the auth-exempt/server/config/getredacts it, and an authenticated endpoint recreates the deadlock), the client-side persistence was dropped. #514 still fixes the core deadlock (the UI renders rather than hanging forever); a reload on an auth-enabled instance simply needs the key re-entered in Settings. No credential is stored client-side.Closes #513. Closes #514. Closes #515. Closes #516.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements