UoE/Remove debug console output and avoidable failed requests#12
Conversation
Make the running app look professional by removing development console logging and eliminating avoidable failed network requests. Each issue was reproduced against the live dev and production instances and traced to its source. Console noise removed (fired during normal browser use): - "Bitstream: <object>" logged once per file on every item page - "Environment extended with app config" on every page load - debug logs across the submission, upload, access-control and datashare services, the submission footer and the section container Avoidable failed requests eliminated (the request is now avoided, since a browser always logs a network 404/401 that JS cannot suppress): - 404 google.analytics.key on every page -> gated behind a new info.enableGoogleAnalytics flag (default false; DataShare does not use GA) - 404 bulkedit.export.max.items on /search -> only requested once the admin export button is shown - 401 qualityassurancesources on item pages for anonymous users -> gated on the CanSeeQA authorization inside the QA component - 404 /favicon.ico -> the DataShare favicon is served from the web root Specs updated for the new GA flag and CanSeeQA gate (with added coverage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
Pull request overview
This PR reduces browser console noise and eliminates several avoidable failed HTTP requests (404/401) by removing debug logging and gating backend probes behind explicit config/authorization checks, while aiming to preserve user-visible behavior.
Changes:
- Removed unguarded
console.logdebug output across submission/upload/access-control and item file listing. - Introduced
info.enableGoogleAnalytics(defaultfalse) and used it to prevent probinggoogle.analytics.key(and GA/Klaro setup) when GA isn’t intended. - Avoided unnecessary backend requests by gating: CSV export limit lookup behind “export button is actually shown”, and QA sources lookup behind
FeatureID.CanSeeQA; also served the theme favicon at/favicon.ico.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/environments/environment.test.ts | Adds info.enableGoogleAnalytics to the test environment config. |
| src/config/info-config.interface.ts | Extends InfoConfig with enableGoogleAnalytics and documents its intent. |
| src/config/default-app-config.ts | Defaults enableGoogleAnalytics to false with rationale. |
| src/config/config.util.ts | Removes an unconditional console log during config merge. |
| src/app/submission/sections/upload/section-upload.component.ts | Removes debug logging during duplicate/size validation. |
| src/app/submission/sections/container/section-container.component.ts | Removes debug logging related to open panel state. |
| src/app/submission/form/footer/submission-form-footer.component.ts | Removes debug logs tied to signal lifecycle/changes. |
| src/app/statistics/google-analytics.service.ts | Skips GA setup entirely unless enableGoogleAnalytics is true. |
| src/app/statistics/google-analytics.service.spec.ts | Updates tests to cover GA-disabled behavior via the new flag. |
| src/app/shared/search/search-export-csv/search-export-csv.component.ts | Defers bulkedit.export.max.items probe until the export button is actually showable. |
| src/app/shared/cookies/browser-klaro.service.ts | Avoids probing GA key when GA is disabled; hides GA consent option without the request. |
| src/app/shared/cookies/browser-klaro.service.spec.ts | Updates/extends tests for GA key probing and GA-disabled behavior. |
| src/app/shared/access-control-form-container/bulk-access-control.service.ts | Removes debug logging when executing the bulk access-control script. |
| src/app/shared/access-control-form-container/access-control-form-container.component.ts | Removes debug logging and unused subscribe callback. |
| src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts | Gates QA sources request on CanSeeQA authorization to avoid anonymous 401s. |
| src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts | Adds test coverage for the new CanSeeQA gate and fixes provider shadowing. |
| src/app/item-page/simple/field-components/file-section/file-section.component.ts | Removes per-bitstream debug logging when loading file/license pages. |
| src/app/datashare/datashare-submission.service.ts | Removes debug logs from service creation and signal updates. |
| src/app/datashare/datashare-submission-form-section-container.service.ts | Removes debug log from open-panel ID setter. |
| config/config.example.yml | Documents and defaults info.enableGoogleAnalytics. |
| angular.json | Copies theme favicon to the web root to satisfy /favicon.ico. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…bed when the export button is shown Locks in the fix from this PR so a regression that reintroduces the per-page 404 (probing the config property for non-admin/anonymous users) is caught. Addresses Copilot review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a4a18e9
into
datashare-UoEMainLibrary-dspace-8_x
What & why
While running DataShare, the browser console showed a lot of development logging and the Network tab showed several failed requests, which makes the app look unpolished. This PR removes that noise and eliminates the avoidable failed requests without changing user-facing behaviour.
Every issue below was reproduced against the live dev (
dev-6.pc:8853) and production (datashare.ed.ac.uk) instances with a headless browser, then traced back to its source in the code.Console output removed
Debug
console.logs that fired during normal use:Bitstream: <object>— logged once per file on every item page (10× on a typical production item) —file-section.component.tsEnvironment extended with app config— every page load —config.util.tsLeft untouched on purpose: genuine error/diagnostic logging in core services, debug logging already guarded by
appConfig.debug/environment.debug, and server-side (Node/SSR) logging that never reaches the browser console.Failed requests eliminated
A browser always logs a network 404/401 for a failed XHR (JS cannot suppress it), so each fix avoids making the request when it isn't needed rather than just swallowing the error:
404 …/config/properties/google.analytics.keyinfo.enableGoogleAnalyticsflag (default false) — DataShare does not use GA404 …/config/properties/bulkedit.export.max.items/search, for every user401 …/integration/qualityassurancesources/search/byTargetCanSeeQAauthorization inside the QA component404 /favicon.icoBehaviour change to note (Google Analytics)
info.enableGoogleAnalyticsdefaults to false. DataShare configures no backendgoogle.analytics.key, so for us this is a no-op that simply removes the per-page 404. Any installation that does use Google Analytics must setinfo.enableGoogleAnalytics: trueto keep tracking — documented inconfig.example.ymlanddefault-app-config.ts. When enabled, behaviour is identical to before (tracking snippet + the Klaro GA consent option).Deliberately NOT changed (to avoid regressions)
console.warnin core data parsing — touching it risks the object/request cachecorrectiontypesrequest inds-item-alerts— that component also renders the withdrawn / private-item alerts that anonymous users must keep seeing401and the base-DSpaceDspaceRestServiceerror log — base behaviour, not part of the observed production noise500thumbnail responses — a backend/data issue, absent on production (not a frontend bug)Verification
tsc --noEmitpasses for both the app and spec projects; ESLint is clean on every changed file (no new warnings)google-analyticsandbrowser-klarospecs for the new flag and addedCanSeeQAgate coverage (plus the previously-missing test provider) to the QA component spec🤖 Generated with Claude Code