DocumentLibraryView #408

reneefromhold · 2025-02-10T21:20:19Z

Overview

This PR has 3 components.
Components

DocumentLibraryPreview
DocumentLibraryView
DocumentDetailView

Full spec here

Security

REMINDER: All file contents are public.

[ x] I have ensured no secure credentials or sensitive information remain in code, metadata, comments, etc.
- [ x] Please verify that you double checked that .storybook/preview.js does not contain your participant access key details.
- [x ] There are no temporary testing changes committed such as API base URLs, access tokens, print/log statements, etc.
[ x] These changes do not introduce any security risks, or any such risks have been properly mitigated.

Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.

Checklist

Testing

[x ] MyDataHelps iOS app
MyDataHelps Android app
Web browser
MyDataHelps website

Documentation

[ x] I have added relevant Storybook updates to this PR as well.
If this feature requires a developer doc update, tag @CareEvolution/api-docs.

Consider "Squash and merge" as needed to keep the commit history reasonable on main.

Reviewers

Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:

Subject-matter experts
Style/editing reviewers
Others requested by the content owner

…meter from detail view

coderabbitai · 2025-02-20T21:44:13Z

Note

Reviews paused

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Walkthrough

The changes encompass several areas across the codebase. Unit tests have been added for managing survey files, specifically to verify file querying and deletion behaviors using mocked external modules. New helper functions have been introduced for querying all survey files and deleting survey result files, along with corresponding TypeScript interfaces. Enhancements have been made to the DocumentDetailView component, including new props, asynchronous initialization logic, file download handling, and deletion confirmation, as well as a dedicated CSS file for styling. A preview function for DocumentDetailView and another for DocumentLibraryView have been implemented to provide sample data. Storybook configurations for both DocumentDetailView and DocumentLibraryView have been expanded with multiple stories to cover different document types and states. Additionally, localization strings for various file-related actions have been added or updated across several language files to ensure consistent internationalization of document management features.

🪧 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 or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

coderabbitai

Actionable comments posted: 8

🧹 Nitpick comments (21)

src/helpers/query-all-survey-files.ts (4)
33-33: Consider removing async from the forEach callback.

This callback does not await any asynchronous operation, so marking it as async can be misleading and might cause confusion if future modifications introduce asynchronous steps that won’t be properly awaited.

38-38: Use optional chaining for cleaner and safer expressions.

Repetitive checks like x && x.answers && x.answers[0] can be replaced with optional chaining to reduce complexity. For instance, change:
- const fileName = fileNameResults && fileNameResults.answers ? fileNameResults.answers[0] : "";
+ const fileName = fileNameResults?.answers?.[0] ?? "";
Apply similar changes for the date, title, fileNameResultID, type, and notes to improve readability.

Also applies to: 44-44, 46-47, 49-49, 51-51

🧰 Tools

🪛 Biome (1.9.4)

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

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

91-94: Avoid mixing async/await with .then(...).

Currently, the function is declared as async, yet a promise chain handles the result. For better consistency and clarity, either remove async and return the promise, or use await fully. For example:
-export async function queryAllSurveyFiles(props: SurveyUploadedFileQueryParameters): Promise<SurveyUploadedFile[]> {
-    return await queryAllSurveyAnswers(queryParameters).then((surveyAnswers: SurveyAnswer[]) => {
-        return groupSurveyAnswersByResults(surveyAnswers);
-    });
+export async function queryAllSurveyFiles(props: SurveyUploadedFileQueryParameters): Promise<SurveyUploadedFile[]> {
+    const surveyAnswers = await queryAllSurveyAnswers(queryParameters);
+    return groupSurveyAnswersByResults(surveyAnswers);
}
101-106: Consider re-throwing errors to allow callers to handle them.

Currently, errors are only logged without providing feedback to the caller about failure. Re-throwing or returning an error value ensures upstream code can respond appropriately:
}).catch((error) => {
    console.error('Error deleting survey results', error);
    console.error('The survey must be configured to support result deletion');
+   throw error;
});
src/components/view/DocumentLibraryView/DocumentLibraryView.previewData.ts (1)

23-23: Use deterministic dates if you rely on snapshot testing.

Generating random dates can produce inconsistent test snapshots. If you want consistent outputs, consider using a fixed date or a predictable seed for the random calculation.
src/components/view/DocumentLibraryView/DocumentLibraryView.stories.tsx (1)
33-46: Consider adding error state story.

While the current stories cover Default, Loading, and Live states, consider adding an error state story to demonstrate how the component handles failures.
export const Error: Story = {
    args: { ...defaultProps, preview: 'PreviewError' },
    render: render
};
src/components/view/DocumentDetailView/DocumentDetailView.previewData.ts (1)
24-36: Refactor preview data for better maintainability.

Consider extracting common properties into a base object to reduce duplication and improve maintainability.
const basePreviewData: Partial<DocumentDetail> = {
    surveyResultId: "12345",
    fileCategory: "test category",
    showDownload: false,
    fileKey: "file key"
};

const previewPdfData: DocumentDetail = {
    ...basePreviewData,
    title: "Sample Portrait PDF",
    fileName: "PDF32000_2008.pdf",
    // ... other specific properties
} as DocumentDetail;
Also applies to: 38-50, 52-64, 66-78, 80-92, 94-106, 108-120
src/components/view/DocumentLibraryView/DocumentLibraryView.tsx (2)
104-112: Improve sorting logic readability and type safety.

The sorting logic in buildFileList could be more readable and type-safe.

Consider using a more structured approach:
function buildFileList(files: SurveyUploadedFile[], selectedSegment: "date" | "name" | "type") {
+   const sortingStrategies = {
+       date: (a: SurveyUploadedFile, b: SurveyUploadedFile) => b.date.getTime() - a.date.getTime(),
+       name: (a: SurveyUploadedFile, b: SurveyUploadedFile) => a.title.localeCompare(b.title),
+       type: (a: SurveyUploadedFile, b: SurveyUploadedFile) => a.type.localeCompare(b.type)
+   };
+   const sortingStrategy = sortingStrategies[selectedSegment];
-   const sortedFiles = files.sort((a, b) => {
-       if (selectedSegment === "date") {
-           return a.date > b.date ? -1 : 1;
-       } else if (selectedSegment === "name") {
-           return a.title < b.title ? -1 : 1;
-       } else {
-           return a.type < b.type ? -1 : 1;
-       }
-   });
+   const sortedFiles = [...files].sort(sortingStrategy);
122-124: Consider adding cleanup in useInitializeView.

The useInitializeView hook might need cleanup if the component unmounts during initialization.

Add cleanup:
useInitializeView(() => {
+   let mounted = true;
    initialize();
+   return () => {
+       mounted = false;
+   };
}, [], []);
src/components/view/DocumentDetailView/DocumentDetailView.tsx (1)
22-27: Consider making DocumentDetail properties optional.

The DocumentDetail interface extends SurveyUploadedFile but some properties might not be available when file is not found.

Make properties optional:
export interface DocumentDetail extends SurveyUploadedFile {
-   presignedDocUrl: string,
-   presignedImageUrl: string,
+   presignedDocUrl?: string,
+   presignedImageUrl?: string,
    showDownload: boolean,
    fileKey: string
}
__tests__/helpers/query-all-survey-files.test.ts (2)
83-87: Add test for concurrent file operations.

The test doesn't verify behavior during concurrent delete operations.

Add concurrent operation test:
test("should handle concurrent delete operations correctly", async () => {
    const deletePromises = [
        deleteSurveyResultFiles("test-id-1", "test-file-key-1"),
        deleteSurveyResultFiles("test-id-2", "test-file-key-2")
    ];
    await Promise.all(deletePromises);
    
    expect(MyDataHelps.deleteSurveyResult).toHaveBeenCalledTimes(2);
    expect(MyDataHelps.deleteFile).toHaveBeenCalledTimes(2);
});
89-94: Add retry mechanism test.

The test doesn't verify retry behavior for transient failures.

Add retry test:
test("should retry failed operations", async () => {
    (MyDataHelps.deleteSurveyResult as jest.Mock)
        .mockRejectedValueOnce(new Error("Transient error"))
        .mockResolvedValueOnce(undefined);
    
    await deleteSurveyResultFiles("test-id", "test-file-key");
    expect(MyDataHelps.deleteSurveyResult).toHaveBeenCalledTimes(2);
});
src/helpers/strings-en.ts (1)
526-533: Consider adding more descriptive error messages.

The error message for file loading failure could be more specific.

Enhance error messages:
-   "file-not-loaded": "The file could not be loaded. Please contact support.",
+   "file-not-loaded": "Unable to load the file. This could be due to network issues or the file may have been deleted. Please try again or contact support if the problem persists.",
+   "file-load-network-error": "Network error while loading the file. Please check your connection and try again.",
+   "file-load-permission-error": "You don't have permission to access this file.",
src/helpers/strings-pt.ts (1)

525-533: Well-structured localization implementation!

The localization implementation follows best practices:

Consistent string keys across all language files

Type-safe implementation using TypeScript

Clear and maintainable organization of UI strings

Consider adding JSDoc comments for the string keys to help other developers understand their context and usage.

Also applies to: 526-533, 526-533
src/helpers/strings-fr.ts (1)
526-533: LGTM! Consider adding a period to the error message.

The French translations for document-related functionality are accurate and well-structured. Consider adding a period at the end of the error message for consistency with other error messages in the file.

Apply this diff to add a period to the error message:
-    "file-not-loaded": "Le fichier n'a pas pu être chargé. Veuillez contacter le support.",
+    "file-not-loaded": "Le fichier n'a pas pu être chargé. Veuillez contacter le support."
src/components/view/DocumentDetailView/DocumentDetailView.css (6)
1-8: Consider using CSS variables for better maintainability.

The container styles look good, but consider using CSS variables for values that might need to be adjusted across different themes or breakpoints.

Apply this diff to use CSS variables:
 .mdhui-survey-answer-file-container {
     max-width: 600px;
     margin: auto;
-    padding: 16px;
-    border-radius: 8px;
+    padding: var(--mdhui-spacing-4, 16px);
+    border-radius: var(--mdhui-border-radius-2, 8px);
     background: #f9f9f9;
-    box-shadow: 0px 4px 10px rgba(0, 0, 0, 0.1);
+    box-shadow: var(--mdhui-box-shadow-1, 0px 4px 10px rgba(0, 0, 0, 0.1));
 }
10-19: Consider a more responsive height approach.

Using a fixed viewport height percentage might cause issues on mobile devices. Consider using a min-height or a more responsive approach.

Apply this diff to make the height more responsive:
 .mdhui-survey-answer-file-preview-container {
     width: 100%;
-    height: 55vh;
+    min-height: 300px;
+    height: min(55vh, 600px);
     display: flex;
     align-items: center;
     justify-content: center;
     background: #fff;
-    border-radius: 8px;
+    border-radius: var(--mdhui-border-radius-2, 8px);
     overflow: hidden;
 }
32-37: Consider using CSS variables for spacing.

The file name styles look good, but consider using CSS variables for consistency with other spacing values.

Apply this diff to use CSS variables:
 .mdhui-survey-answer-file-document-file-name {
     display: flex;
     justify-content: center;
-    margin-top: 10px;
+    margin-top: var(--mdhui-spacing-2-5, 10px);
 }
38-41: Consider using CSS variables for padding.

Consider using CSS variables for consistent spacing across the application.

Apply this diff to use CSS variables:
 .mdhui-survey-answer-file-document-details-parent {
-    padding: 10px;
+    padding: var(--mdhui-spacing-2-5, 10px);
 }
42-45: Consider using CSS variables for spacing and font size.

The use of em units for font size is good for accessibility. Consider using CSS variables for consistent spacing and typography.

Apply this diff to use CSS variables:
 .mdhui-survey-answer-file-document-details {
-    font-size: .88em;
-    margin: 5px;
+    font-size: var(--mdhui-font-size-sm, .88em);
+    margin: var(--mdhui-spacing-1-25, 5px);
 }
47-53: Consider using CSS variables for spacing.

The button container styles are well-structured. Consider using CSS variables for consistent spacing.

Apply this diff to use CSS variables:
 .mdhui-survey-answer-file-download-button {
     display: flex;
     flex-direction: column;
     align-items: flex-end;
     justify-content: flex-end;
-    margin-bottom: 10px;
+    margin-bottom: var(--mdhui-spacing-2-5, 10px);
 }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a49b32 and 8a74f3f.

📒 Files selected for processing (21)

__tests__/helpers/query-all-survey-files.test.ts (1 hunks)
src/components/view/DocumentDetailView/DocumentDetailView.css (1 hunks)
src/components/view/DocumentDetailView/DocumentDetailView.previewData.ts (1 hunks)
src/components/view/DocumentDetailView/DocumentDetailView.stories.tsx (1 hunks)
src/components/view/DocumentDetailView/DocumentDetailView.tsx (1 hunks)
src/components/view/DocumentDetailView/index.ts (1 hunks)
src/components/view/DocumentLibraryView/DocumentLibraryView.previewData.ts (1 hunks)
src/components/view/DocumentLibraryView/DocumentLibraryView.stories.tsx (1 hunks)
src/components/view/DocumentLibraryView/DocumentLibraryView.tsx (1 hunks)
src/components/view/DocumentLibraryView/index.ts (1 hunks)
src/components/view/index.ts (1 hunks)
src/helpers/index.ts (1 hunks)
src/helpers/query-all-survey-files.ts (1 hunks)
src/helpers/strings-de.ts (1 hunks)
src/helpers/strings-en.ts (1 hunks)
src/helpers/strings-es.ts (1 hunks)
src/helpers/strings-fr.ts (1 hunks)
src/helpers/strings-it.ts (1 hunks)
src/helpers/strings-nl.ts (1 hunks)
src/helpers/strings-pl.ts (1 hunks)
src/helpers/strings-pt.ts (1 hunks)

✅ Files skipped from review due to trivial changes (2)

src/components/view/DocumentDetailView/index.ts
src/components/view/DocumentLibraryView/index.ts

🧰 Additional context used

🪛 Biome (1.9.4)

src/components/view/DocumentDetailView/DocumentDetailView.tsx

[error] 128-128: Avoid passing children using a prop

The canonical way to pass children in React is to use JSX elements

(lint/correctness/noChildrenProp)

[error] 151-151: Avoid passing children using a prop

The canonical way to pass children in React is to use JSX elements

(lint/correctness/noChildrenProp)

src/helpers/query-all-survey-files.ts

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

Unsafe fix: Change to an optional chain.