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

feat(ojoi-application): Adding addtions as html #16797

Merged
merged 13 commits into from
Nov 12, 2024

Conversation

jonbjarnio
Copy link
Member

@jonbjarnio jonbjarnio commented Nov 11, 2024

What

New feature

  • Added additions as HTML so that users can both upload "fylgiskjöl" and/or write them directly as HTML.

Why

So that users can both upload additions as html or pdf.

Screenshots / Gifs

Attach Screenshots / Gifs to help reviewers understand the scope of the pull request

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new component for managing additions, allowing users to toggle between numeric and Roman numeral displays.
    • Enhanced the Attachments component to switch between "as Attachment" and "as Document" modes, improving file upload clarity.
    • Added new constants and schema to support additional data structures related to advertisements.
    • Implemented new utility functions for generating additions and validating their structure.
    • Added localized messages for new buttons and improved clarity of existing labels.
  • Bug Fixes

    • Improved error handling for managing additions to ensure data integrity.
  • Documentation

    • Updated button labels and user interface messages for better clarity and user experience.
  • Chores

    • Refactored utility functions to include new methods for handling additions and converting numbers to Roman numerals.

@jonbjarnio jonbjarnio requested a review from a team as a code owner November 11, 2024 15:12
Copy link
Contributor

coderabbitai bot commented Nov 11, 2024

Walkthrough

This pull request introduces a new Additions component for managing a list of additions within the application. It modifies existing components, such as HTMLEditor and Attachments, to enhance their functionality and user interface. New constants and schemas are defined to support these updates, while various utility functions are added or modified to assist in managing and validating additions. The overall structure of the application is improved to better handle additions and enhance the user experience.

Changes

File Change Summary
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx Added Additions component for managing additions, including state management and validation.
libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.tsx Added optional controller prop to HTMLEditor for conditional rendering of the Controller.
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx Modified Attachments to include a toggle for adding attachments or documents, integrating Additions.
libs/application/templates/official-journal-of-iceland/src/lib/constants.ts Added constants MAXIMUM_ADDITIONS_COUNT set to 10 and DEFAULT_ADDITIONS_COUNT set to 1.
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts Introduced additionSchema and modified advertSchema to include an optional additions field.
libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts Expanded attachments messages with new button labels and refined existing labels.
libs/application/templates/official-journal-of-iceland/src/lib/types.ts Added additions property to InputFields under Routes.ADVERT.
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts Added utility functions: getAddition, isAddition, and convertNumberToRoman.
libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.tsx Updated to include getAddition for initializing application state with additions.

Suggested reviewers

  • albinagu
  • Rikkiabb

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
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: 8

🧹 Outside diff range and nitpick comments (16)
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (3)

17-17: Document the default state choice.

Consider adding a comment explaining why asAddition defaults to true. This helps other developers understand the component's initial behavior.

+  // Default to additions mode as it's the primary use case
   const [asAddition, setAsAddition] = useState(true)

22-35: Extract button configuration for better maintainability.

Consider extracting the button configuration to a constant to improve code maintainability and reusability.

+  const modeToggleButton = {
+    icon: asAddition ? 'upload' : 'document',
+    iconType: 'outline' as const,
+    variant: 'ghost' as const,
+    size: 'small' as const,
+  }

   <Button
-    icon={asAddition ? 'upload' : 'document'}
-    iconType="outline"
-    variant="ghost"
-    size="small"
+    {...modeToggleButton}
     onClick={() => setAsAddition((toggle) => !toggle)}
   >

Line range hint 11-57: Consider making the component more reusable.

The component is tightly coupled to the OJOI application context. Consider extracting the application-specific logic to make it more reusable across different NextJS apps, as per the library guidelines.

Suggestions:

  1. Extract the file upload configuration to props
  2. Make the Additions component optional
  3. Allow customization of allowed file types

Example refactor:

interface AttachmentsProps {
  onFileChange: (files: File[]) => void;
  onFileRemove: (fileId: string) => void;
  files: Array<{ name: string; id: string }>;
  allowedFileTypes?: string[];
  additionsComponent?: React.ComponentType<any>;
  defaultMode?: 'additions' | 'attachments';
}

export const Attachments = ({
  onFileChange,
  onFileRemove,
  files,
  allowedFileTypes = ALLOWED_FILE_TYPES,
  additionsComponent: AdditionsComponent,
  defaultMode = 'additions',
}: AttachmentsProps) => {
  // ... rest of the implementation
}
libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (2)

82-82: Remove unnecessary blank line

This blank line serves no purpose and should be removed.


83-83: Add documentation and consider grouping with similar constants

The constant is properly exported and follows the naming convention. However, consider:

  1. Adding a JSDoc comment explaining its purpose and usage
  2. Grouping it with other maximum count constants (like MAXIMUM_REGULAR_SIGNATURE_COUNT) for better organization
- export const MAXIMUM_ADDITIONS_COUNT = 10
+ /** Maximum number of additions (fylgiskjöl) that can be attached to an application */
+ export const MAXIMUM_ADDITIONS_COUNT = 10

Also consider moving this constant near other maximum count constants for better code organization.

libs/application/templates/official-journal-of-iceland/src/lib/types.ts (1)

24-24: Consider enhancing type safety and documentation for additions field.

The new field follows the established pattern and aligns with the PR objectives. However, consider the following improvements:

  1. Add TypeScript types for the additions structure
  2. Document the purpose and format of additions
  3. Consider if this field should be required and added to RequiredInputFieldsNames

Example implementation:

// Define the structure of an addition
export interface Addition {
  content: string;
  type: 'html' | 'pdf';
  title?: string;
}

// Update InputFields type (if not inferred)
export interface InputFieldTypes {
  'advert.additions': Addition[];
}
libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.tsx (2)

Line range hint 47-116: Consider refactoring to reduce code duplication.

The controlled and uncontrolled versions share significant code. Consider extracting the common JSX into a separate component or render function.

Here's a suggested refactor:

const EditorContent = ({ updateFormValue, ...props }) => (
  <>
    {props.title && (
      <Text marginBottom={2} variant="h5">
        {props.title}
      </Text>
    )}
    <Box
      className={editorWrapper({
        error: !!props.error,
      })}
    >
      <Editor
        readOnly={props.readOnly}
        hideWarnings={props.hideWarnings}
        elmRef={props.editorRef}
        config={props.config}
        fileUploader={props.fileUploader}
        valueRef={props.valueRef}
        classes={classes}
        onChange={() => props.onChange?.(props.valueRef.current())}
        onBlur={() => {
          updateFormValue?.(props.valueRef.current())
          props.onChange?.(props.valueRef.current())
        }}
      />
    </Box>
    {props.error && <div className={errorStyle}>{props.error}</div>}
  </>
)

return controller ? (
  <Controller
    name={name}
    defaultValue={initialValue}
    render={({ field: { onChange: updateFormValue, value } }) => (
      <EditorContent {...{ updateFormValue, ...props }} />
    )}
  />
) : (
  <EditorContent {...props} />
)
🧰 Tools
🪛 Biome

[error] 108-108: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 111-111: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


107-112: Use optional chaining for onChange callbacks.

The static analysis correctly identifies missing optional chaining operators.

Apply this fix:

          onChange={() => {
-            onChange && onChange(valueRef.current())
+            onChange?.(valueRef.current())
          }}
          onBlur={() => {
-            onChange && onChange(valueRef.current())
+            onChange?.(valueRef.current())
          }}
🧰 Tools
🪛 Biome

[error] 108-108: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 111-111: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (2)

29-50: Add TypeScript types for message definitions

While the button messages are well-structured, consider adding TypeScript types to improve type safety and maintainability across the application.

+type MessageDefinition = {
+  id: string;
+  defaultMessage: string;
+  description: string;
+};

+type ButtonMessages = {
+  asDocument: MessageDefinition;
+  asAttachment: MessageDefinition;
+  removeAddition: MessageDefinition;
+  addAddition: MessageDefinition;
+};

-  buttons: defineMessages({
+  buttons: defineMessages<ButtonMessages>({

78-97: Fix inconsistent indentation in radio section

The indentation in the radio section is inconsistent, particularly around the numeric and roman message definitions.

    radio: {
      title: defineMessages({
        label: {
          id: 'ojoi.application:attachments.radio.title.label',
          defaultMessage: 'Viðaukar eða fylgiskjöl',
          description: 'Label of the radio buttons',
        },
      }),
-      numeric: defineMessages({
-        label: {
+      numeric: defineMessages({
+        label: {
           id: 'ojoi.application:attachments.radio.additions.label',
           defaultMessage: 'Viðauki (1, 2, 3..)',
           description: 'Label of the additions radio button',
-        },
-      }),
+        },
+      }),
      roman: defineMessages({
        label: {
          id: 'ojoi.application:attachments.radio.documents.label',
          defaultMessage: 'Viðauki (I, II, III..)',
          description: 'Label of the documents radio button',
        },
      }),
    },
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (2)

372-375: Improve robustness and maintainability

The current implementation has several limitations:

  1. No input validation
  2. Hard-coded array limits extensibility
  3. Missing documentation

Consider this more maintainable approach:

+/**
+ * Converts a number to its Roman numeral representation
+ * @param {number} num - Number between 1-10 to convert
+ * @returns {string} Roman numeral representation
+ * @throws {Error} If number is not between 1-10
+ */
 export const convertNumberToRoman = (num: number) => {
+  if (num < 1 || num > 10) {
+    throw new Error('Number must be between 1 and 10');
+  }
+  const romanMap = new Map([
+    [10, 'X'],
+    [9, 'IX'],
+    [5, 'V'],
+    [4, 'IV'],
+    [1, 'I']
+  ]);
+  let result = '';
+  let remaining = num;
+  for (const [value, numeral] of romanMap) {
+    while (remaining >= value) {
+      result += numeral;
+      remaining -= value;
+    }
+  }
+  return result;
-  const roman = ['I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX', 'X']
-  return roman[num - 1]
 }

This implementation:

  • Validates input
  • Uses an algorithmic approach that's easier to extend
  • Adds documentation
  • Follows functional programming principles

Line range hint 74-375: Add test coverage for new utility functions

The PR objectives mention that tests haven't been added yet. Please add unit tests for the new functions:

  • getAddition: Test ID generation, title formatting, and parameter validation
  • isAddition: Test validation of valid and invalid inputs
  • convertNumberToRoman: Test conversion and validation logic

Would you like me to help generate the test suite for these functions?

libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (1)

73-73: LGTM! Consider exporting the addition type

The schema integration looks good. However, for better TypeScript support across the codebase, consider exporting the addition type.

Add this export at the bottom of the file:

export type Addition = z.infer<typeof additionSchema>[number]
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (3)

35-35: Redundant type annotation in useState

The type parameter <boolean> in useState<boolean>(false) is unnecessary because TypeScript can infer the type from the initial value.

Apply this diff to simplify the code:

-const [asRoman, setAsRoman] = useState<boolean>(false)
+const [asRoman, setAsRoman] = useState(false)

81-93: Optimize function definition by moving handleTitleChange outside

Defining handleTitleChange inside onRomanChange leads to a new function being created every time onRomanChange is called. To improve performance, move handleTitleChange outside of onRomanChange so it's defined only once.

Apply this diff to restructure the code:

+const handleTitleChange = (addition: Addition, i: number, asRoman: boolean) => {
+  if (addition.type !== 'html') return addition
+
+  const title = f(attachments.additions.title, {
+    index: asRoman ? convertNumberToRoman(i + 1) : i + 1,
+  })
+  return {
+    ...addition,
+    title: title,
+  }
+}

 const onRomanChange = (val: boolean) => {
-  const handleTitleChange = (addition: Addition, i: number) => {
-    if (addition.type !== 'html') return addition
-
-    const title = f(attachments.additions.title, {
-      index: asRoman ? convertNumberToRoman(i + 1) : i + 1,
-    })
-    return {
-      ...addition,
-      title: title,
-    }
-  }

   const currentAnswers = cloneDeep(currentApplication.answers)
-  const updatedAdditions = additions.map(handleTitleChange)
+  const updatedAdditions = additions.map((addition, i) =>
+    handleTitleChange(addition, i, val),
+  )

34-228: Enhance component reusability across applications

To improve the reusability of the Additions component in different NextJS apps, consider decoupling it from application-specific logic and dependencies. Abstract localization messages and application hooks to make the component more generic.

Suggestions:

  • Parameterize Localization Strings: Pass localized messages as props to the component instead of importing them directly.
  • Abstract Application Hooks: Use context or higher-order components to inject application-specific logic, allowing the component to remain unaware of the underlying application details.
  • Export Types and Interfaces: Ensure all props and types are well-defined and exported for use in other applications.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d7bffc4 and 146b0b0.

📒 Files selected for processing (9)
  • libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.tsx (4 hunks)
  • libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (2 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (2 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (2 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/types.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (5 hunks)
  • libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.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."
libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.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."
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.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."
libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (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."
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (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."
libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (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."
libs/application/templates/official-journal-of-iceland/src/lib/types.ts (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."
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (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."
libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.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."
🪛 Biome
libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.tsx

[error] 108-108: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 111-111: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (7)
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (1)

1-9: LGTM! Well-structured imports and proper TypeScript usage.

The imports are well-organized, following tree-shaking practices by importing specific components. TypeScript types are properly used for props definition.

libs/application/templates/official-journal-of-iceland/src/components/htmlEditor/HTMLEditor.tsx (2)

16-16: LGTM! Props type definition follows best practices.

The new optional controller prop is well-typed and maintains component reusability across different NextJS apps.


28-28: LGTM! Default prop value ensures backward compatibility.

Setting controller = true as default maintains compatibility with existing component usage while enabling new functionality.

libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (2)

51-57: LGTM! Well-structured message with dynamic parameter

The additions title message is well-designed with the {index} parameter, making it reusable for both numeric and roman numeral formats.


Line range hint 1-97: Verify uniqueness of message IDs

Let's ensure there are no duplicate message IDs across the application.

✅ Verification successful

The output shows no duplicate message IDs as the only line returned is just a single occurrence of a message with its defaultMessage. Let me verify this with a different approach to be thorough.


All message IDs are unique and properly namespaced

The verification confirms that all message IDs in the attachments messages file are unique, with each ID appearing exactly once in the codebase. The IDs follow a consistent namespacing pattern ojoi.application:attachments.* and are properly organized by their functional groups (general, buttons, inputs, etc.).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for duplicate message IDs in all translation files
fd -e ts -e tsx | xargs rg "id: 'ojoi\.application:attachments\." -A 1 | sort | uniq -d

Length of output: 230


Script:

#!/bin/bash
# Search for all message IDs with 'ojoi.application:attachments' prefix across the codebase
rg "id: 'ojoi\.application:attachments\.[^']*'" --no-filename | sort | uniq -c | sort -nr

Length of output: 1182

libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.tsx (1)

21-25: LGTM! Import changes follow best practices.

The addition of getAddition maintains consistent import grouping and supports tree-shaking by importing specific functions.

libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (1)

133-137: LGTM! Well-implemented type guard

The implementation follows best practices for type guards and maintains consistency with other validation functions in the file.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.44%. Comparing base (f7726b2) to head (d239421).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16797      +/-   ##
==========================================
- Coverage   36.44%   36.44%   -0.01%     
==========================================
  Files        6853     6856       +3     
  Lines      143479   143566      +87     
  Branches    40942    40954      +12     
==========================================
+ Hits        52288    52316      +28     
- Misses      91191    91250      +59     
Flag Coverage Δ
api 3.34% <ø> (ø)
api-domains-auth-admin 48.48% <ø> (ø)
api-domains-education 30.55% <ø> (ø)
application-system-api 40.97% <ø> (ø)
application-template-api-modules 27.64% <ø> (+<0.01%) ⬆️
application-ui-shell 20.82% <ø> (ø)
download-service 44.24% <ø> (ø)
services-auth-ids-api 52.08% <ø> (ø)
services-endorsements-api 53.34% <ø> (ø)
services-user-notification 46.83% <ø> (-0.02%) ⬇️
services-user-profile 61.77% <ø> (ø)

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

see 11 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 f7726b2...d239421. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Nov 11, 2024

Datadog Report

All test runs 6948222 🔗

4 Total Test Services: 0 Failed, 4 Passed
➡️ Test Sessions change in coverage: 8 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
api 0 0 0 4 0 2.6s 1 no change Link
application-system-api 0 0 0 120 2 3m 32.98s 1 no change Link
application-template-api-modules 0 0 0 123 0 2m 3.45s 1 no change Link
application-ui-shell 0 0 0 74 0 31.61s 1 no change Link

jonbjarnio and others added 2 commits November 11, 2024 16:05
…ponents/additions/Additions.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@jonbjarnio jonbjarnio requested a review from disaerna November 11, 2024 16:05
@jonbjarnio jonbjarnio added the automerge Merge this PR as soon as all checks pass label Nov 11, 2024
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: 1

🧹 Outside diff range and nitpick comments (3)
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (3)

34-53: Consider adding type safety for the initial state

While the code is well-structured, we can improve type safety for the initial state.

-  const [asRoman, setAsRoman] = useState<boolean>(false)
+  const [asRoman, setAsRoman] = useState(false)
+  const { formatMessage: f } = useLocale()
+  const { setValue } = useFormContext<{
+    [InputFields.advert.additions]: Addition[]
+  }>()

166-171: Add aria-label to radio buttons for better accessibility

The radio buttons should have aria-labels for better screen reader support.

 <RadioButton
   label={f(attachments.inputs.radio.numeric.label)}
   name="asNumbers"
   checked={!asRoman}
+  aria-label={f(attachments.inputs.radio.numeric.label)}
   onChange={() => onRomanChange(false)}
 />
 <RadioButton
   label={f(attachments.inputs.radio.roman.label)}
   name="asRoman"
   checked={asRoman}
+  aria-label={f(attachments.inputs.radio.roman.label)}
   onChange={() => onRomanChange(true)}
 />

Also applies to: 172-177


202-210: Add aria-label to remove button for better accessibility

The remove button should have an aria-label describing its action.

 <Button
   variant="utility"
   colorScheme="destructive"
   icon="remove"
   size="small"
+  aria-label={f(attachments.buttons.removeAddition)}
   onClick={() => onRemoveAddition(additionIndex)}
 >
   {f(attachments.buttons.removeAddition)}
 </Button>
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 146b0b0 and 22baab1.

📒 Files selected for processing (1)
  • libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.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."
🪛 Biome
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx

[error] 118-118: Can't assign currentAdditions because it's a constant

This is where the variable is defined as constant

Unsafe fix: Replace const with let if you assign it to a new value.

(lint/correctness/noConstAssign)

🔇 Additional comments (2)
libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (2)

1-32: LGTM: Well-structured imports and type definitions

The code follows TypeScript best practices with proper type definitions and organized imports.


150-158: LGTM: Proper debounce implementation

The debouncing of addition changes is well implemented with proper cleanup.

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: 1

🧹 Outside diff range and nitpick comments (2)
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (2)

17-17: Consider using a more descriptive state variable name.

The boolean state asAddition could be renamed to better reflect its purpose, such as isHtmlMode or isDocumentMode.

-  const [asAddition, setAsAddition] = useState(true)
+  const [isHtmlMode, setIsHtmlMode] = useState(true)

36-53: Consider adding error boundaries and loading states.

The component could benefit from proper error handling and loading states, especially during file uploads or when loading additions.

+  const [isLoading, setIsLoading] = useState(false)
+
+  const handleFileChange = async (files: File[]) => {
+    try {
+      setIsLoading(true)
+      await onChange(files)
+    } catch (error) {
+      // Handle error appropriately
+    } finally {
+      setIsLoading(false)
+    }
+  }

   return (
     <Stack space={4}>
       {/* ... */}
       {!asAddition ? (
-        <Additions application={application} />
+        <ErrorBoundary fallback={<ErrorMessage />}>
+          <Additions application={application} />
+        </ErrorBoundary>
       ) : (
         <InputFileUpload
           {...props}
-          onChange={onChange}
+          onChange={handleFileChange}
+          isLoading={isLoading}
         />
       )}
     </Stack>
   )
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 22baab1 and 2fc991c.

📒 Files selected for processing (4)
  • libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (2 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx
  • libs/application/templates/official-journal-of-iceland/src/lib/constants.ts
  • libs/application/templates/official-journal-of-iceland/src/screens/RequirementsScreen.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.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 (2)
libs/application/templates/official-journal-of-iceland/src/fields/Attachments.tsx (2)

2-2: LGTM: Import statements are well-organized and tree-shaking friendly.

The specific imports from '@island.is/island-ui/core' and React core facilitate effective tree-shaking.

Also applies to: 7-8


Line range hint 1-54: Verify integration with other NextJS applications.

While the component follows the coding guidelines for reusability and TypeScript usage, we should verify its integration with other NextJS applications that might use it.

✅ Verification successful

Component is properly integrated and follows NextJS application structure

The verification shows that the Attachments component is:

  • Properly exported and used within the official-journal-of-iceland template
  • Part of a larger NextJS application structure at apps/web
  • Follows the monorepo's modular architecture with proper imports and exports
  • Used consistently with other components in the official-journal-of-iceland module

The component's integration with other NextJS applications is verified through its:

  • Location in the libs directory for shared usage
  • Export pattern matching other shared components
  • Proper TypeScript usage and import/export structure
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other NextJS apps using this component and verify proper imports

# Find all NextJS apps in the repository
echo "Searching for NextJS apps using this component..."
rg -l "next\.config\.js" | while read -r app_path; do
  dir=$(dirname "$app_path")
  echo "Checking $dir for Attachments component usage..."
  rg -l "Attachments" "$dir"
done

# Verify proper imports in other applications
echo "Verifying import patterns..."
rg -A 2 "import.*Attachments.*from.*official-journal-of-iceland"

Length of output: 1415


Script:

#!/bin/bash
# Let's try a different approach to verify the component's usage and integration

# Check for any imports or usage of the Attachments component across the codebase
echo "Checking for Attachments component usage..."
rg -l "Attachments" --type tsx --type ts

# Look for files that might be importing from official-journal-of-iceland
echo -e "\nChecking for imports from official-journal-of-iceland..."
rg -l "from.*official-journal-of-iceland"

# Check if this is part of a published package by looking for package.json
echo -e "\nChecking package configuration..."
fd "package.json" libs/application/templates/official-journal-of-iceland | xargs cat

# Look for any test files related to this component
echo -e "\nChecking for related test files..."
fd "Attachments.spec.tsx" -o "Attachments.test.tsx" libs/application/templates/official-journal-of-iceland

Length of output: 1859


Script:

#!/bin/bash
# Let's try one final verification with corrected commands

# Check for Attachments component usage with ripgrep
echo "Checking for Attachments component usage..."
rg -l "Attachments" --type typescript --type typescriptreact

# Look for the module's usage in application configurations
echo -e "\nChecking module usage in applications..."
rg -A 5 "OfficialJournalOfIceland" apps/

# Check if the component is exported from the module
echo -e "\nChecking component exports..."
rg -l "export.*Attachments" libs/application/templates/official-journal-of-iceland/

# Check for integration tests
echo -e "\nChecking for integration tests..."
fd "integration" libs/application/templates/official-journal-of-iceland

Length of output: 46206

@kodiakhq kodiakhq bot merged commit 1e266af into main Nov 12, 2024
52 checks passed
@kodiakhq kodiakhq bot deleted the feat/ojoi-add-html-additions branch November 12, 2024 12:32
jonnigs pushed a commit that referenced this pull request Nov 26, 2024
* Addition logic now ready

* Removed unused imports.

* Moved strings to messages

* Update libs/application/templates/official-journal-of-iceland/src/components/additions/Additions.tsx

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

* Fixed PR comments

* Fixed onAddAddition handler

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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.

3 participants