Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(web): Global alert banner - Handle null case #16298

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

RunarVestmann
Copy link
Member

@RunarVestmann RunarVestmann commented Oct 7, 2024

Global alert banner - Handle null case

What

  • Alert banner can be null (if it was for example deleted from the cms)

Summary by CodeRabbit

  • New Features

    • Enhanced alert banner logic for improved safety and display.
    • Refined handling of layout properties for better functionality.
    • Improved conditional rendering for layout components.
  • Bug Fixes

    • Addressed TypeScript error suppression to improve type safety.
  • Chores

    • Ensured efficient font preloading for better performance.

@RunarVestmann RunarVestmann added the automerge Merge this PR as soon as all checks pass label Oct 7, 2024
@RunarVestmann RunarVestmann requested review from a team as code owners October 7, 2024 14:37
Copy link
Contributor

coderabbitai bot commented Oct 7, 2024

Walkthrough

The changes in the main.tsx file focus on the Layout component, enhancing its functionality and type safety. Key modifications include updated logic for displaying alert banners, refined handling of props, and the use of TypeScript error suppression comments. The getProps method has been adjusted to construct alert banner content correctly, while the GlobalContextProvider wraps the Page component for context availability. Additionally, the file maintains SEO-related meta tags and introduces conditional rendering for layout components.

Changes

File Change Summary
apps/web/layouts/main.tsx - Updated alert banner logic for safer property access.
- Added TypeScript error suppression comments.
- Refined handling of props in LayoutProps interface.
- Modified getProps method for alert banner content.
- Wrapped Page component with GlobalContextProvider.
- Maintained meta tags in <Head> component.
- Enhanced conditional rendering for layout components.
- Defined font preloading explicitly.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Layout
    participant GlobalContextProvider
    participant Page
    participant AlertBanner

    User->>Layout: Request Layout
    Layout->>GlobalContextProvider: Wrap Page
    GlobalContextProvider->>Page: Provide Context
    Page->>Layout: Render Page
    Layout->>AlertBanner: Check showAlertBanner
    AlertBanner->>Layout: Display Alert if needed
    Layout->>User: Render Complete Layout
Loading

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 36.84%. Comparing base (481a8c6) to head (223667d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
apps/web/layouts/main.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16298      +/-   ##
==========================================
- Coverage   36.86%   36.84%   -0.03%     
==========================================
  Files        6798     6796       -2     
  Lines      140486   140404      -82     
  Branches    39936    39922      -14     
==========================================
- Hits        51792    51725      -67     
+ Misses      88694    88679      -15     
Flag Coverage Δ
portals-admin-regulations-admin 1.88% <ø> (ø)
testing-e2e ?
web 1.83% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
apps/web/layouts/main.tsx 0.00% <0.00%> (ø)

... and 10 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481a8c6...223667d. Read the comment docs.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
apps/web/layouts/main.tsx (5)

Line range hint 30-36: Avoid suppressing TypeScript errors; properly type absoluteUrl function

Using // @ts-expect-error suppresses TypeScript errors, which can hide potential issues and reduce type safety. It's recommended to provide proper type annotations for the absoluteUrl function to adhere to TypeScript best practices.

Apply this diff to fix the TypeScript errors:

+import { IncomingMessage } from 'http'
+
-// @ts-expect-error make web strict
const absoluteUrl = (req, setLocalhost) => {
+const absoluteUrl = (req: IncomingMessage | undefined, setLocalhost?: string) => {
  let protocol = 'https:'
  let host = req
-    ? req.headers['x-forwarded-host'] || req.headers['host']
+    ? req.headers['x-forwarded-host'] ?? req.headers['host']
    : window.location.host
  if (host.indexOf('localhost') > -1) {
    if (setLocalhost) host = setLocalhost
    protocol = 'http:'
  }
  return {
    protocol: protocol,
    host: host,
    origin: protocol + '//' + host,
  }
}

Line range hint 105-107: Avoid suppressing TypeScript errors; properly type respOrigin in LayoutProps

The use of // @ts-expect-error indicates that respOrigin may not have proper type annotations in the LayoutProps interface. To maintain type safety, it's important to define the type of respOrigin.

Define respOrigin in the LayoutProps interface and remove the @ts-expect-error comment:

export interface LayoutProps {
  // Existing properties...
+  respOrigin: string
  // Other properties...
}

-// @ts-expect-error make web strict
respOrigin,

Line range hint 663-666: Potential null reference error when accessing req.headers.cookie

In the expression req.headers.cookie?.indexOf(alertBannerId) === -1, if req or req.headers is undefined, this can cause a runtime error. To prevent this, use optional chaining on headers as well.

Apply this diff to safely access req.headers.cookie:

alertBannerContent: {
  ...alertBanner,
  showAlertBanner:
    alertBanner?.showAlertBanner &&
-    (!req?.headers.cookie ||
-      req.headers.cookie?.indexOf(alertBannerId) === -1),
+    (!req?.headers?.cookie ||
+      req.headers?.cookie?.indexOf(alertBannerId) === -1),
},

Line range hint 689-690: Avoid suppressing TypeScript errors; ensure proper type casting for menu

Using // @ts-expect-error to bypass TypeScript errors should be avoided. Instead, ensure that the type casting for menu is correct to maintain type safety.

Adjust the type assertion to match the expected type:

-// @ts-expect-error make web strict
} as Menu)
+} as Menu

If menu does not exactly match the Menu type, consider transforming it appropriately or refining the type definition.


Line range hint 770-772: Avoid suppressing TypeScript errors; align componentProps with Component props

The @ts-expect-error suppression suggests a type mismatch between componentProps and the expected props of Component. It's important to ensure that componentProps matches the expected prop types of Component.

Review and adjust the types to resolve the mismatch:

/**
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment
- // @ts-expect-error make web strict */
 <Component {...componentProps} />

Consider updating the LayoutWrapper type or refining the componentProps interface to ensure compatibility.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c7ad42c and 5356a18.

📒 Files selected for processing (1)
  • apps/web/layouts/main.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/layouts/main.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 7, 2024

Datadog Report

All test runs 19720e5 🔗

2 Total Test Services: 0 Failed, 2 Passed
➡️ Test Sessions change in coverage: 3 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
portals-admin-regulations-admin 0 0 0 24 0 16.42s 1 no change Link
web 0 0 0 84 0 33.11s 1 no change Link

@kodiakhq kodiakhq bot merged commit 8fbd08d into main Oct 7, 2024
33 checks passed
@kodiakhq kodiakhq bot deleted the fix/web-global-alert-banner-can-be-null branch October 7, 2024 17:49
thordurhhh pushed a commit that referenced this pull request Oct 8, 2024
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
kodiakhq bot added a commit that referenced this pull request Oct 11, 2024
…-pages (#16234)

* Service portal removal. Add portals my pages

* minor fixes

* Fix

* path fix

* fix(portals-admin): locklist (#16279)

* fix(portals-admin): locklist

* tweak

* msg id fix

* tweak

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(service-portal): feature flag resolver for documents (#16285)

* fix: def info and alert

* feat: add feature flag to resolver

* fix: move ff call to seperate function

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(vehicles-bulk-mileage): Fixes after testing review (#16295)

* fix: testing fixes v1

* fix: testing comments v2

* fix: better message

* fix: function name

* fix: duplicate loading

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(tests): New @island/testing/e2e library (#16287)

* Add @swc-node/register and @swc/core

* Add testing/e2e library

* update project.json for testing/e2e

* fix import for libTestingE2e

---------

Co-authored-by: Kristofer <kristofer@andes.is>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(parental-leave): ApplicationRights (#15901)

* feat(parental-leave): ApplicationRights

Added applicationRights to parental-leave when sending application. Since we are using a new way of calculating periods

* Fix days used by period calculation

* Tests for new periods

* rename function with proper camelCase

* Refactor: Made duplicate code into a function

* Make ApplicationRights nullable

* refactor: function instead of duplicate code

* remove console.log

* error handling for period data

* clientConfig nullable fix

* Fixes for calculation of months. And using clamp to get correct value of daysLeft

* Multiply amount of months by 30 for period calculation with month durations

* Fix old calculation of endDate with months

---------

Co-authored-by: hfhelgason <hfhelgason@deloitte.is>
Co-authored-by: veronikasif <54938148+veronikasif@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(passport-application): Updated readme (#16296)

* updated readme

* updated readme

* chore: nx format:write update dirty files

---------

Co-authored-by: andes-it <builders@andes.is>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(regulations-admin): date format signature, remove self affect, disclaimer text (#16288)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(regulations-admin): No diff no addition in appendix (#16293)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(web): Global alert banner - Handle null case (#16298)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(web): Change custom syslumenn pages config for header (#16299)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(j-s): Digital mailbox API (#16301)

* feat(j-s): Block create subpoena on staging and dev

* Update subpoena.service.ts

* fix(j-s): Fix mailbox API

* remove changes not meant for this branch

* Update subpoena.service.ts

* fix(j-s): reverting changes from other branch

* Update subpoena.response.ts

* Update subpoena.response.ts

* Update subpoena.response.ts

* Update subpoena.response.ts

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* fix(signature-collection): Fix list reviewed toggle (#16300)

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* chore(scripts): Stricter shell script checking (#16242)

* Set style level for shellcheck

* Linting & formatting scripts

* Remove _podman.sh script

* Format all scripts

* Add reviewdog/action-shfmt step

* Configure shfmt

* Merge from main

* Linting

* Move shfmt to before lint

* Remove reviewdog

* Allow external sources in shellcheck

* Use Reviewdog for shellcheck

* Set version for Reviewdog

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* chore(new-primary-school): Update messages namespace (#16302)

Co-authored-by: veronikasif <54938148+veronikasif@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(driving-license): check if 65+ renewal is possible (#16292)

* check if 65 renewal is possible

* remove console log

* cleanup

* coderabbit tweaks

* coderabbit changes

* quick fix

* add type?

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

* feat(service-portal): default defender and has chosen fields for subpoena (#16306)

* fix: def info and alert

* feat: add feature flag to resolver

* fix: move ff call to seperate function

* feat: add default choices ans has chosen + loading states

* fix: use type

* fix: undefined type issue

* fix: simplify check

* Update service setup for my pages infra

* chore: charts update dirty files

* Remove from infra

* undo rename

---------

Co-authored-by: albinagu <47886428+albinagu@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is>
Co-authored-by: Þorkell Máni Þorkelsson <wowthorkell@gmail.com>
Co-authored-by: Svanhildur Einarsdóttir <54863023+svanaeinars@users.noreply.github.com>
Co-authored-by: Kristofer <kristofer@andes.is>
Co-authored-by: helgifr <helgifreyr95@gmail.com>
Co-authored-by: hfhelgason <hfhelgason@deloitte.is>
Co-authored-by: veronikasif <54938148+veronikasif@users.noreply.github.com>
Co-authored-by: Rafn Árnason <rafnarnason@gmail.com>
Co-authored-by: andes-it <builders@andes.is>
Co-authored-by: Rúnar Vestmann <43557895+RunarVestmann@users.noreply.github.com>
Co-authored-by: mannipje <135017126+mannipje@users.noreply.github.com>
Co-authored-by: unakb <una@kolibri.is>
Co-authored-by: juni-haukur <158475136+juni-haukur@users.noreply.github.com>
Co-authored-by: birkirkristmunds <142495885+birkirkristmunds@users.noreply.github.com>
Co-authored-by: Kristján Albert <kal@juni.is>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants