-
Notifications
You must be signed in to change notification settings - Fork 61
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(regulations-admin): No diff no addition in appendix #16293
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this 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 (2)
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (2)
243-253
: Improved handling of appendix changes. Consider minor refactoring for clarity.The changes to
getAppendixDiffHtml
function effectively address the PR objective by eliminating unnecessary additions when there are no modifications in the appendix. The function now correctly handles new appendices, unchanged appendices, and those with modifications.To further improve code clarity, consider refactoring the last condition:
- if (diff) { - // If the appendix has changes - return diff - } else { - // If the appendix has no diff - return undefined - } + // If the appendix has changes, return the diff; otherwise, return undefined + return diff || undefinedThis change maintains the same logic while making the code more concise and easier to read.
Line range hint
359-361
: Enhance type safety forlocalActions.revokeAppendix
The
revokeAppendix
function in thelocalActions
object is currently defined with a return type ofundefined
. This might not accurately represent the intended behavior of the function.Consider updating the function signature to better reflect its purpose:
revokeAppendix: (idx: number, revoked: boolean) => voidIf the function is intended to perform an action without returning a value, using
void
as the return type would be more appropriate thanundefined
. This change enhances type safety and makes the code's intent clearer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1)
Pattern
libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1)
Line range hint
1-577
: Overall assessment: Changes align with PR objectives and maintain code qualityThe modifications to the
EditChange
component, particularly in thegetAppendixDiffHtml
function, successfully address the PR objective of eliminating unnecessary additions to the payload when there are no modifications in the appendix. The changes maintain good coding practices, including:
- Effective use of TypeScript for type safety.
- Adherence to React best practices in component structure and state management.
- Maintaining reusability of the component within the
libs
directory.The suggested minor improvements in the previous comments will further enhance code clarity and type safety. Overall, the changes are well-implemented and ready for merging, pending the addressed suggestions.
Datadog ReportBranch report: ✅ 0 Failed, 24 Passed, 0 Skipped, 15.85s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16293 +/- ##
==========================================
- Coverage 36.86% 36.83% -0.03%
==========================================
Files 6798 6796 -2
Lines 140483 140404 -79
Branches 39935 39922 -13
==========================================
- Hits 51792 51724 -68
+ Misses 88691 88680 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…-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 <[email protected]> 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 <[email protected]> Co-authored-by: veronikasif <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Ásdís Erna Guðmundsdóttir <[email protected]> Co-authored-by: Þorkell Máni Þorkelsson <[email protected]> Co-authored-by: Svanhildur Einarsdóttir <[email protected]> Co-authored-by: Kristofer <[email protected]> Co-authored-by: helgifr <[email protected]> Co-authored-by: hfhelgason <[email protected]> Co-authored-by: veronikasif <[email protected]> Co-authored-by: Rafn Árnason <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Rúnar Vestmann <[email protected]> Co-authored-by: mannipje <[email protected]> Co-authored-by: unakb <[email protected]> Co-authored-by: juni-haukur <[email protected]> Co-authored-by: birkirkristmunds <[email protected]> Co-authored-by: Kristján Albert <[email protected]>
What
When no addition or change is in appendix, no need to add diff to payload.
Why
Fix unnecessary additions.
Checklist:
Summary by CodeRabbit