551 cookie banner#592
Conversation
|
Warning Rate limit exceeded@DilwoarH has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a cookie consent banner feature to the application. A new JavaScript class Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (3)
src/routes/cookies.js (1)
15-16: Consider using consistent cookie optionsBoth cookies should use the same security attributes, only differing in their maxAge values.
Apply this improvement:
+const baseOptions = { + httpOnly: true, + secure: process.env.NODE_ENV === 'production', + sameSite: 'strict' +} -res.cookie('cookies_preferences_set', req.body.accept_cookies, { maxAge: 1000 * 60 * 60 * 24 * 365 }) -res.cookie('cookies_preferences_set_updated', true, { maxAge: 1000 }) +res.cookie('cookies_preferences_set', req.body.accept_cookies, { ...baseOptions, maxAge: 1000 * 60 * 60 * 24 * 365 }) +res.cookie('cookies_preferences_set_updated', true, { ...baseOptions, maxAge: 1000 })src/assets/js/components/cookie-banner.js (1)
1-96: Add JSDoc documentation for better maintainabilityThe code would benefit from comprehensive documentation explaining the purpose, usage, and integration points.
+/** + * @class CookieBanner + * @description Manages cookie consent preferences and banner display + * @example + * import CookieBanner from './cookie-banner' + * const banner = new CookieBanner() + * + * // Listen for consent changes + * banner.onConsentChange((accepted) => { + * if (accepted) { + * // Initialize analytics + * } + * }) + */ class CookieBanner {src/views/cookies.html (1)
120-132: Enhance notification banner accessibility.Consider adding
role="alert"to ensure screen readers announce the preference update immediately.{{ govukNotificationBanner({ type: "success", + role: "alert", html: html }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/assets/js/components/cookie-banner.js (1 hunks)
- src/assets/scss/components/_cookie-banner.scss (1 hunks)
- src/assets/scss/index.scss (1 hunks)
- src/routes/cookies.js (1 hunks)
- src/views/components/cookie-banner.html (1 hunks)
- src/views/cookies.html (3 hunks)
- src/views/layouts/main.html (3 hunks)
- webpack.config.mjs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/assets/scss/components/_cookie-banner.scss
🔇 Additional comments (11)
webpack.config.mjs (2)
28-29: LGTM! Well-structured webpack configuration update.The addition of the cookie-banner entry point follows proper code organisation practices by placing it in the components directory.
29-29: Verify the cookie-banner.js file location.Let's ensure the cookie-banner.js file exists at the specified path.
✅ Verification successful
Cookie banner file location verified successfully
The file exists at the expected path:
src/assets/js/components/cookie-banner.js🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of cookie-banner.js in the components directory # Expected: File should exist at the specified path fd --type f "cookie-banner.js$" src/assets/js/componentsLength of output: 100
src/views/components/cookie-banner.html (2)
18-45: LGTM! Follows GOV.UK Design System patternsThe implementation correctly uses the
govukCookieBannermacro with appropriate button types and JavaScript hooks for functionality. The structure aligns with GOV.UK Design System guidelines.
9-17: Verify screen reader compatibilityThe dynamic content in the acceptance message uses a
<span>with a JavaScript class. Ensure that screen readers correctly announce changes to this content when the user's choice is updated.✅ Verification successful
Screen reader compatibility is properly implemented
The cookie banner implementation is accessible and follows GOV.UK Design System best practices:
- Uses the official
govukCookieBannercomponent which has built-in accessibility features- Correctly manages
aria-hiddenstates when showing/hiding content- Properly handles dynamic content updates through JavaScript
- Uses semantic HTML with appropriate ARIA attributes
The dynamic confirmation message is implemented correctly and will be announced to screen readers due to:
- The confirmation message container toggles
aria-hiddenappropriately- Text content updates are made directly to the DOM using
textContent- The implementation uses the GOV.UK Design System's tested and accessible patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ARIA attributes or accessibility tests related to the cookie banner rg -g '*.{js,html,test.*}' -i '(aria-live|role.*alert|screen.?reader).*cookie'Length of output: 80
Script:
#!/bin/bash # Search for any JavaScript files that handle the cookie banner rg -g '*.js' -l 'cookie-banner' # Search for any test files related to cookie banner rg -g '*test*' -l 'cookie-banner' # Search for the cookie banner component to check for existing ARIA attributes rg -g '*.html' -A 5 -B 5 'class="[^"]*cookie-banner' # Search specifically for the confirmation message handling rg -g '*.{js,html}' 'js-app-c-cookie-banner__confirmation-decision'Length of output: 2672
Script:
#!/bin/bash # Let's examine the JavaScript file to see how the confirmation message is handled cat src/assets/js/components/cookie-banner.js # Let's also check if the cookie banner uses the GOV.UK Design System component rg -g '*.html' 'govukCookieBanner' -A 10Length of output: 5648
src/views/layouts/main.html (3)
41-42: Verify cookie banner placement and accessibility.The cookie banner placement before the header follows GOV.UK Design System guidelines. However, ensure the banner is keyboard-accessible and doesn't trap focus.
Consider implementing these accessibility enhancements:
- Ensure the banner can be dismissed using the Escape key
- Maintain proper focus management when the banner appears/disappears
- Test with screen readers to verify the banner is announced appropriately
Line range hint
1-115: Verify cookie banner integration with existing GOV.UK components.The cookie banner integration maintains the template's structure and follows GOV.UK Frontend patterns. However, ensure the banner doesn't interfere with other interactive components, especially the phase banner and header navigation.
#!/bin/bash # Description: Check for potential conflicts with existing components # Look for other components that might interact with the cookie banner rg -l "govukPhaseBanner|govukHeader" --type htmlConsider implementing these improvements:
- Add documentation comments explaining the cookie banner's lifecycle
- Consider adding data attributes for automated testing
- Document any required configuration in the README
Line range hint
103-112: Review script loading order and bundle dependencies.The cookie banner script is correctly placed after the main application bundle. However, there are a few considerations:
Consider these improvements:
- Add
deferattribute to script tags to optimise page loading- Document any dependencies between application.bundle.js and cookie-banner.bundle.js
- Consider implementing a more robust script loading strategy using modern ES modules
✅ Verification successful
Let me gather more information about the script dependencies and initialization order.
Let me check the specific content of the cookie-banner and application JavaScript files to understand their dependencies better.
Script loading order is appropriate, but optimisations recommended
The cookie banner and application scripts are independent modules with no direct dependencies between them. The cookie banner is self-initialising and manages its own state, whilst the application.js handles general page functionality. However, the loading strategy can be optimised:
- Add
deferattribute to both script tags:<script defer src="/public/js/application.bundle.js"></script> <script defer src="/public/js/cookie-banner.bundle.js"></script>This will improve page loading performance whilst maintaining the correct execution order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential script dependencies and initialisation order # Look for any cookie-related initialization in application bundle rg -l "cookie" --type jsLength of output: 78
Script:
#!/bin/bash # Check for cookie-related code and initialization patterns rg -l "cookie" --type-add 'frontend:*.{js,ts,jsx,tsx}' --type frontend # Check for module imports and dependencies rg -l "import.*cookie" --type-add 'frontend:*.{js,ts,jsx,tsx}' --type frontend # Look for script initialization patterns rg "init|initialize" --type-add 'frontend:*.{js,ts,jsx,tsx}' --type frontend -A 3Length of output: 4284
Script:
#!/bin/bash # Check cookie-banner.js implementation cat src/assets/js/components/cookie-banner.js # Check application.js implementation cat src/assets/js/application.js # Check if there are any direct dependencies between these files rg -l "cookie-banner" src/assets/js/application.jsLength of output: 3519
src/assets/scss/index.scss (1)
8-8: LGTM! Verify the imported SCSS file.The import statement follows the established pattern and maintains consistency with the existing component-based structure.
Let's verify the existence of the cookie banner SCSS file:
✅ Verification successful
Import statement is correctly referencing an existing SCSS file
The cookie banner SCSS file exists at the expected location
src/assets/scss/components/_cookie-banner.scss, confirming that the import statement is valid and properly structured.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the cookie banner SCSS file exists # Expected: File should exist in the components directory fd -e scss -g '*cookie-banner*' src/assets/scss/componentsLength of output: 107
src/views/cookies.html (3)
1-7: LGTM: Component imports and page setup are appropriate.The necessary GOV.UK Design System components are imported, and the page name accurately reflects its purpose as a cookie notice page.
73-73: Verify cookie data collection statement.The statement "The cookies do not store any information you add to the service" needs verification, particularly for Smartlook session recording cookies.
153-153: Improve cookie preference value validation.The string comparison (
== "true"/"false") could be problematic. Consider using proper boolean values and adding server-side validation.Also applies to: 158-158
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/unit/views/components/cookie-banner.test.js (1)
18-22: Group related assertions for better readabilityConsider grouping related assertions using
describeblocks. For example, group text content checks and link checks separately.- expect(header.textContent.trim()).toEqual('Cookies on Submit and update your planning data') - expect(acceptButton.textContent.trim()).toEqual('Accept analytics cookies') - expect(rejectButton.textContent.trim()).toEqual('Reject analytics cookies') - expect(viewCookieLink.textContent.trim()).toEqual('View cookies') - expect(viewCookieLink.href).toEqual('/cookies') + describe('text content', () => { + expect(header.textContent.trim()).toEqual('Cookies on Submit and update your planning data') + expect(acceptButton.textContent.trim()).toEqual('Accept analytics cookies') + expect(rejectButton.textContent.trim()).toEqual('Reject analytics cookies') + expect(viewCookieLink.textContent.trim()).toEqual('View cookies') + }) + + describe('links', () => { + expect(viewCookieLink.href).toEqual('/cookies') + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- test/unit/get-startedPage.test.js (1 hunks)
- test/unit/http-errorPage.test.js (1 hunks)
- test/unit/views/components/cookie-banner.test.js (1 hunks)
- test/unit/views/organisations/dataset-overview.test.js (1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/unit/views/organisations/dataset-overview.test.js (1)
Learnt from: GeorgeGoodall-GovUk PR: digital-land/submit#510 File: test/unit/views/organisations/dataset-overview.test.js:3-3 Timestamp: 2024-10-21T09:23:33.470Z Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🔇 Additional comments (3)
test/unit/get-startedPage.test.js (1)
28-28: Improved selector precision looks good!The refined selector
#main-content h2makes the test more precise by specifically targeting headings within the main content area, reducing the risk of false positives.Let's verify consistency of heading selectors across other test files:
✅ Verification successful
Heading selector patterns are consistent across test files
The verification shows a clear pattern where:
h1selectors are used for page titles, consistently without#main-contentscopeh2selectors within content are properly scoped with#main-content, matching the change in question🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of main-content scoped heading selectors # Expected: Similar heading selector patterns in other test files # Search for heading selectors in test files rg -g '*.test.js' "querySelector.*h[1-6]" ./testLength of output: 1059
test/unit/http-errorPage.test.js (1)
26-26: The selector refinement looks good but seems unrelated to cookie banner implementation.The change to use
#main-content h2instead of justh2is a good practice as it makes the test more resilient by being more specific about where to find the heading.Let's verify if this is part of a broader heading selector update:
test/unit/views/organisations/dataset-overview.test.js (1)
73-73: Improved selector specificity enhances test reliability.The change to include
#main-contentin the selector makes the test more precise by ensuring the heading is found within the main content area, reducing the likelihood of false positives if similar headings exist elsewhere in the document.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
src/routes/cookies.js (1)
15-18: Use constants for magic numbers and improve cookie configurationThe code uses magic numbers for cookie expiry times and lacks clear naming for timing constants.
Apply this diff to improve clarity and configuration:
+ const MILLISECONDS_PER_YEAR = 1000 * 60 * 60 * 24 * 365 + const PREFERENCE_UPDATED_MESSAGE_DURATION = 1000 // 1 second + - const defaultCookieExpiry = 1000 * 60 * 60 * 24 * 365 // 1 year - res.cookie('cookies_preferences_set', req.body.accept_cookies, { maxAge: defaultCookieExpiry }) - res.cookie('cookies_preferences_set_updated', true, { maxAge: 1000 }) + res.cookie('cookies_preferences_set_updated', true, { maxAge: PREFERENCE_UPDATED_MESSAGE_DURATION })test/acceptance/cookies.test.js (2)
6-11: Consider adding URL verification after navigationWhilst the title verification is good, adding a URL check would ensure the navigation was successful before testing the title.
test('has title', async ({ page }) => { const cookiesPage = new CookiesPage(page) await cookiesPage.navigateHere() + await expect(page).toHaveURL(cookiesPage.url) await expect(page).toHaveTitle('Cookie notice for Submit and update planning and housing data for England - Submit and update your planning data') })
5-53: Consider additional test scenarios for complete coverageTo fully validate the cookie banner implementation against compliance requirements, consider adding tests for:
- Cookie banner visibility on initial page load
- Banner dismissal functionality
- Persistence of cookie preferences across page reloads
Would you like assistance in implementing these additional test scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/routes/cookies.js (1 hunks)
- test/PageObjectModels/cookiesPage.js (1 hunks)
- test/acceptance/cookies.test.js (1 hunks)
🔇 Additional comments (2)
test/PageObjectModels/cookiesPage.js (1)
1-7: LGTM! The basic page object structure is correct.The class properly extends BasePage and initialises with the correct path.
test/acceptance/cookies.test.js (1)
1-4: Well-structured test setup with Page Object Model pattern!Good practice using Page Object Model for better test maintainability and reusability.
GeorgeGoodall-GovUk
left a comment
GeorgeGoodall-GovUk
left a comment
There was a problem hiding this comment.
tested via preview link, looked over code, all looks good!
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/assets/js/components/cookie-banner.js (1 hunks)
🔇 Additional comments (1)
src/assets/js/components/cookie-banner.js (1)
29-60:⚠️ Potential issueAdd destroy method for proper cleanup
The class lacks a cleanup method to remove event listeners and reset state when the banner is no longer needed.
Consider adding:
+ destroy () { + if (this.isInitialised) { + this.acceptButton.removeEventListener('click', this.boundAccept) + this.rejectButton.removeEventListener('click', this.boundReject) + this.hideButton.removeEventListener('click', this.boundHide) + this.isInitialised = false + } + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
test/unit/javascript/components/cookie-banner.test.js (1)
1-77: Add missing test cases for comprehensive coverage.
Consider adding the following test cases:
- Error handling when DOM elements are missing
- Edge cases for cookie values
- Interaction with existing cookies
- Form submission handling
- Validation of cookie expiry dates
Would you like me to provide example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/assets/js/application.js (1 hunks)
- src/assets/js/components/cookie-banner.js (1 hunks)
- src/views/layouts/main.html (2 hunks)
- test/unit/javascript/components/cookie-banner.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/layouts/main.html
🔇 Additional comments (4)
src/assets/js/application.js (2)
6-6: LGTM! Import statement follows best practices.
The import statement is well-organised and follows ES6 module syntax conventions.
15-15: Verify interaction with js-hidden functionality.
The cookie banner initialisation occurs after initiateJsHiddenChecks. Please ensure that any elements managed by the cookie banner don't interfere with the js-hidden functionality.
✅ Verification successful
Let me gather more information about the cookie banner implementation and its DOM interactions.
Let me check how the cookie banner interacts with DOM elements, particularly focusing on any class manipulations.
No conflicts found between cookie banner and js-hidden functionality
The cookie banner implementation uses direct style manipulation (display: none) for hiding itself and doesn't interact with the js-hidden class. The js-hidden functionality operates independently through a mutation observer that watches for class changes. Both mechanisms work in parallel without interference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential conflicts between cookie banner and js-hidden elements
# Search for js-hidden classes in cookie banner related files
echo "Checking for js-hidden usage in cookie banner components:"
rg -l "js-hidden" --glob "**/*cookie*.{js,html,scss}"
# Search for potential DOM manipulations in CookieBanner class
echo "\nChecking for DOM manipulations in CookieBanner:"
ast-grep --pattern 'classList.{add,remove,toggle}($$$)'
Length of output: 345
Script:
#!/bin/bash
# Search for cookie banner implementation
echo "Searching for cookie banner implementation:"
rg -l "initCookieBanner|CookieBanner" --type js
echo "\nChecking cookie banner implementation details:"
rg -A 5 "initCookieBanner|class.*CookieBanner" --type js
echo "\nChecking for DOM element selections in cookie banner:"
rg "querySelector|getElementById.*cookie" --type js
Length of output: 20728
Script:
#!/bin/bash
# Check cookie banner implementation for DOM manipulations
echo "Checking cookie banner implementation:"
rg -A 10 "class CookieBanner" src/assets/js/components/cookie-banner.js
echo "\nChecking for any show/hide logic in cookie banner:"
rg "show|hide|display|visibility" src/assets/js/components/cookie-banner.js
echo "\nChecking for js-hidden usage in js-hidden.js:"
cat src/assets/js/js-hidden.js
Length of output: 2783
src/assets/js/components/cookie-banner.js (2)
1-9:
Ensure GDPR compliance in default cookie policy
The current implementation sets all cookie types to true by default, which violates GDPR requirements for explicit consent. Only essential cookies should be enabled by default.
Apply this modification:
defaultPolicyValue: {
essential: true,
- settings: true,
- usage: true,
- campaigns: true
+ settings: false,
+ usage: false,
+ campaigns: false
}Likely invalid or redundant comment.
30-38:
Improve event listener management
The current event binding approach could cause memory leaks and makes cleanup difficult.
Implement proper event listener management:
export default class CookieBanner {
+ constructor (document) {
+ // Store bound methods
+ this.boundAccept = this.accept.bind(this)
+ this.boundReject = this.reject.bind(this)
+ this.boundHide = this.hideCookieBanner.bind(this)
+ // ... rest of constructor
}
init () {
+ if (!this.#isInitialised) {
+ console.warn('Cookie banner not properly initialised')
+ return
+ }
+
if (this.getCookie(cookieDefaults.cookieNames.preferenceCookie) !== null) {
this.hideCookieBanner()
}
- this.acceptButton.addEventListener('click', this.accept.bind(this))
- this.rejectButton.addEventListener('click', this.reject.bind(this))
- this.hideButton.addEventListener('click', this.hideCookieBanner.bind(this))
+ this.acceptButton.addEventListener('click', this.boundAccept)
+ this.rejectButton.addEventListener('click', this.boundReject)
+ this.hideButton.addEventListener('click', this.boundHide)
}Likely invalid or redundant comment.
DilwoarH
force-pushed
the
551-cookie-banner
branch
from
3e32bcf to
501d3be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/views/cookies.html (1)
153-153: Use strict comparison for cookie preferences.
Replace loose equality operator (==) with strict equality (===) to prevent type coercion issues:
-checked: cookiePreferences == "true"
+checked: cookiePreferences === "true"
-checked: cookiePreferences == "false"
+checked: cookiePreferences === "false"Also applies to: 158-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- src/assets/js/application.js (1 hunks)
- src/assets/js/components/cookie-banner.js (1 hunks)
- src/assets/scss/components/_cookie-banner.scss (1 hunks)
- src/assets/scss/index.scss (1 hunks)
- src/routes/cookies.js (1 hunks)
- src/views/components/cookie-banner.html (1 hunks)
- src/views/cookies.html (3 hunks)
- src/views/layouts/main.html (2 hunks)
- test/PageObjectModels/cookiesPage.js (1 hunks)
- test/acceptance/cookies.test.js (1 hunks)
- test/unit/get-startedPage.test.js (1 hunks)
- test/unit/http-errorPage.test.js (1 hunks)
- test/unit/javascript/components/cookie-banner.test.js (1 hunks)
- test/unit/views/components/cookie-banner.test.js (1 hunks)
- test/unit/views/organisations/dataset-overview.test.js (1 hunks)
- webpack.config.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/assets/js/application.js
- src/assets/js/components/cookie-banner.js
- src/assets/scss/components/_cookie-banner.scss
- src/assets/scss/index.scss
- src/routes/cookies.js
- src/views/components/cookie-banner.html
- src/views/layouts/main.html
- test/PageObjectModels/cookiesPage.js
- test/acceptance/cookies.test.js
- test/unit/get-startedPage.test.js
- test/unit/http-errorPage.test.js
- test/unit/javascript/components/cookie-banner.test.js
- test/unit/views/components/cookie-banner.test.js
- test/unit/views/organisations/dataset-overview.test.js
- webpack.config.mjs
🔇 Additional comments (3)
src/views/cookies.html (3)
1-7: LGTM! Imports and page setup are well-structured.
The necessary GOV.UK Frontend macros are properly imported, and the page title accurately reflects the content.
120-132: LGTM! Notification banner implementation follows design patterns.
The success notification is well-implemented using the GOV.UK notification banner component, with clear and helpful content.
139-166: Previous review comment about CSRF protection and accessibility is still applicable.
The form still requires the improvements mentioned in the previous review regarding CSRF protection, noscript fallback, and accessibility enhancements.
| * what you click on the service | ||
|
|
||
| The cookies do not store any information you add to the service. | ||
| The cookies do not store any information you add to the service. |
There was a problem hiding this comment.
Fix service name inconsistency in Smartlook section.
The Smartlook section refers to "Check planning and housing data for England" instead of "Submit and update planning and housing data for England". This inconsistency should be corrected to maintain clarity for users.
|
Is the cookie banner tripping up the playwright tests? |
rosado
left a comment
rosado
left a comment
There was a problem hiding this comment.
When I run this locally, I can't dismiss the banner.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/views/cookies.html (1)
120-132: Enhance notification banner accessibility.Consider adding these accessibility improvements:
{{ govukNotificationBanner({ type: "success", + role: "alert", + titleText: "Success", html: html }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
src/assets/js/application.js(1 hunks)src/assets/js/components/cookie-banner.js(1 hunks)src/assets/scss/components/_cookie-banner.scss(1 hunks)src/assets/scss/index.scss(1 hunks)src/routes/cookies.js(1 hunks)src/views/components/cookie-banner.html(1 hunks)src/views/cookies.html(3 hunks)src/views/layouts/main.html(2 hunks)test/PageObjectModels/cookiesPage.js(1 hunks)test/acceptance/cookies.test.js(1 hunks)test/unit/get-startedPage.test.js(1 hunks)test/unit/http-errorPage.test.js(1 hunks)test/unit/javascript/components/cookie-banner.test.js(1 hunks)test/unit/views/components/cookie-banner.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)webpack.config.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/assets/js/application.js
- src/assets/scss/components/_cookie-banner.scss
- src/assets/scss/index.scss
- src/routes/cookies.js
- src/views/components/cookie-banner.html
- src/views/layouts/main.html
- test/PageObjectModels/cookiesPage.js
- test/acceptance/cookies.test.js
- test/unit/get-startedPage.test.js
- test/unit/http-errorPage.test.js
- test/unit/javascript/components/cookie-banner.test.js
- test/unit/views/components/cookie-banner.test.js
- test/unit/views/organisations/dataset-overview.test.js
- webpack.config.mjs
🔇 Additional comments (1)
src/views/cookies.html (1)
1-7: LGTM! Proper component imports and page setup.
The template correctly imports the necessary GOV.UK Design System components and sets an appropriate page title.
| getCookie (name) { | ||
| const cookie = this.document.cookie | ||
| .split('; ') | ||
| .find(row => row.startsWith(name)) | ||
|
|
||
| return cookie ? JSON.parse(cookie.split('=')[1]) : null | ||
| } |
There was a problem hiding this comment.
Enhance cookie parsing security
The getCookie method's JSON parsing of cookie values could throw errors with malformed data and potentially expose sensitive information in error messages.
Implement safer cookie parsing:
getCookie (name) {
const cookie = this.document.cookie
.split('; ')
.find(row => row.startsWith(name))
- return cookie ? JSON.parse(cookie.split('=')[1]) : null
+ if (!cookie) return null
+ try {
+ return JSON.parse(cookie.split('=')[1])
+ } catch (error) {
+ console.warn(`Invalid cookie value for ${name}`)
+ return null
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getCookie (name) { | |
| const cookie = this.document.cookie | |
| .split('; ') | |
| .find(row => row.startsWith(name)) | |
| return cookie ? JSON.parse(cookie.split('=')[1]) : null | |
| } | |
| getCookie (name) { | |
| const cookie = this.document.cookie | |
| .split('; ') | |
| .find(row => row.startsWith(name)) | |
| if (!cookie) return null | |
| try { | |
| return JSON.parse(cookie.split('=')[1]) | |
| } catch (error) { | |
| console.warn(`Invalid cookie value for ${name}`) | |
| return null | |
| } | |
| } |
| init () { | ||
| if (this.getCookie(cookieDefaults.cookieNames.preferenceCookie) !== null) { | ||
| this.hideCookieBanner() | ||
| } | ||
|
|
||
| this.acceptButton.addEventListener('click', this.accept.bind(this)) | ||
| this.rejectButton.addEventListener('click', this.reject.bind(this)) | ||
| this.hideButton.addEventListener('click', this.hideCookieBanner.bind(this)) | ||
| } |
There was a problem hiding this comment.
Improve initialisation logic
The init method continues to bind event listeners even when the banner is hidden. This could lead to unnecessary memory usage and potential event handling conflicts.
Consider this enhancement:
init () {
if (this.getCookie(cookieDefaults.cookieNames.preferenceCookie) !== null) {
this.hideCookieBanner()
+ return
}
this.acceptButton.addEventListener('click', this.accept.bind(this))
this.rejectButton.addEventListener('click', this.reject.bind(this))
this.hideButton.addEventListener('click', this.hideCookieBanner.bind(this))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| init () { | |
| if (this.getCookie(cookieDefaults.cookieNames.preferenceCookie) !== null) { | |
| this.hideCookieBanner() | |
| } | |
| this.acceptButton.addEventListener('click', this.accept.bind(this)) | |
| this.rejectButton.addEventListener('click', this.reject.bind(this)) | |
| this.hideButton.addEventListener('click', this.hideCookieBanner.bind(this)) | |
| } | |
| init () { | |
| if (this.getCookie(cookieDefaults.cookieNames.preferenceCookie) !== null) { | |
| this.hideCookieBanner() | |
| return | |
| } | |
| this.acceptButton.addEventListener('click', this.accept.bind(this)) | |
| this.rejectButton.addEventListener('click', this.reject.bind(this)) | |
| this.hideButton.addEventListener('click', this.hideCookieBanner.bind(this)) | |
| } |
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
DilwoarH
force-pushed
the
551-cookie-banner
branch
from
a0cb659 to
2731c81
Compare
DilwoarH
dismissed
rosado’s stale review
Dismissing as the build was not run before testing
5 participants
What type of PR is this? (check all applicable)
What
Why
We want to use Google analytics and other tools which requires users to consent to cookies.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation