Skip to content

feat: make the core functions asynchronous #40

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

osm-Jatin
Copy link

@osm-Jatin osm-Jatin commented Jun 18, 2025

Task Link

REST-1564

Description

  • Make PdfDocumentGenerator class methods asynchronous
  • Make GenerateDocumentByTemplate class methods asynchronous
  • Add await operator for asyncshronous function calls in controllers

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced OsmoDoc library for generating PDF and Word documents with asynchronous processing and enhanced placeholder and image handling.
    • Added new data models and configuration for PDF generation and Word document templating.
  • Refactor

    • Renamed all project references, namespaces, and documentation from "DocumentService" to "OsmoDoc" across codebase, configuration, and docs.
    • Centralized PDF tool path configuration and improved cross-platform support for PDF generation.
  • Bug Fixes

    • Initialized data model properties in API requests to prevent null values.
  • Documentation

    • Updated all documentation, API references, and README to reflect OsmoDoc naming and updated usage.
  • Chores

    • Updated Docker, docker-compose, project, and solution files for OsmoDoc naming consistency.
    • Removed legacy DocumentService files and models.

@osm-Jatin osm-Jatin requested a review from sameer-s-b June 18, 2025 03:59
@osm-Jatin osm-Jatin self-assigned this Jun 18, 2025
Copy link

coderabbitai bot commented Jun 18, 2025

Walkthrough

This update renames the project from "DocumentService" to "OsmoDoc" across all code, configuration, documentation, and Docker files. It deletes the old DocumentService source files and introduces equivalent files under the new OsmoDoc namespace, including models, document generators, and project files. All references, documentation, and configuration paths are updated to match the new naming.

Changes

Files/Groups Change Summary
DocumentService/* Deleted all source files: models, enums, document generators, and project file.
OsmoDoc/*, OsmoDoc/Pdf/*, OsmoDoc/Word/* Added new source files under OsmoDoc namespace: models, enums, document generators, and project file.
OsmoDoc.API/*, OsmoDoc.API/Controllers/*, OsmoDoc.API/Helpers/* Updated namespaces/imports from DocumentService to OsmoDoc. Changed references to new models and generators.
OsmoDoc.API/Models/* Updated namespaces and imports; added default initializers to some properties.
Dockerfile, docker-compose.yaml Renamed project references, image, and container names from "DocumentService" to "OsmoDoc".
README.md, CONTRIBUTING.md, .github/PULL_REQUEST_TEMPLATE/* Updated all project references, URLs, and names from "DocumentService" to "OsmoDoc".
OsmoDoc.sln, OsmoDoc.API/OsmoDoc.API.sln, OsmoDoc.API/OsmoDoc.API.csproj Renamed solution/project files and references to use "OsmoDoc" naming.
docs/site/*, docs/site/xrefmap.yml, docs/site/manifest.json Updated documentation to reflect new namespaces, assembly names, and file paths for "OsmoDoc".

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant OsmoDoc.API (Controller)
    participant OsmoDoc.Pdf/Word (Generator)
    participant FileSystem/Tools

    Client->>OsmoDoc.API (Controller): POST /pdf or /word (with template & data)
    OsmoDoc.API (Controller)->>OsmoDoc.Pdf/Word (Generator): await GeneratePdf/GenerateDocumentByTemplate(...)
    OsmoDoc.Pdf/Word (Generator)->>FileSystem/Tools: (If EJS) Convert EJS to HTML (external process)
    OsmoDoc.Pdf/Word (Generator)->>FileSystem/Tools: Replace placeholders in template
    OsmoDoc.Pdf/Word (Generator)->>FileSystem/Tools: Generate PDF/Word document (external process for PDF)
    OsmoDoc.Pdf/Word (Generator)-->>OsmoDoc.API (Controller): Return output file path
    OsmoDoc.API (Controller)-->>Client: Return file or response
Loading

Poem

🐇
A hop, a leap, a namespace swap—
OsmoDoc now takes the lap!
From docs to code, the name anew,
The Docker’s changed its label too.
Goodbye, old service, thanks for the ride—
OsmoDoc’s here, with ears open wide!
🌱


📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 36db8f2 and 5f0e3bf.

📒 Files selected for processing (1)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 309-309:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 309-309:
Dereference of a possibly null reference.


[warning] 303-303:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
OsmoDoc/Word/WordDocumentGenerator.cs (6)

30-127: Excellent async conversion implementation!

The method has been properly converted to async with correct usage of await for GetXWPFDocument and ReplaceImagePlaceholders. The async design aligns well with the PR objectives for making core functions asynchronous.

The use of TryAdd for placeholder dictionaries (lines 57, 62, 67) is a good practice that prevents exceptions from duplicate keys.


134-139: Well-implemented async file reading.

Good use of File.ReadAllBytesAsync and MemoryStream approach, which properly handles the stream lifecycle and avoids the disposal issues that were flagged in previous reviews.


146-158: Correct synchronous implementation with good practices.

The method properly creates directories and uses the IOPath alias to resolve namespace ambiguity. Being synchronous is appropriate since no async operations are needed here.


244-253: Excellent table cell handling implementation.

The code correctly ensures data rows have sufficient cells by using the header row's cell count and adding missing cells before populating them. This addresses the cell count mismatch issue effectively.


274-335: Solid async implementation for image replacement.

The method demonstrates good async patterns with proper use of await for file operations, HTTP requests, and stream operations. The approach of loading into memory, processing, and writing back is well-suited for document manipulation scenarios.


30-335: Successful async conversion achieving PR objectives.

The conversion to asynchronous operations has been implemented effectively throughout the class. The main GenerateDocumentByTemplate method and supporting async methods properly use await for I/O operations, which aligns perfectly with the PR goal of making core functions asynchronous. This will improve the application's responsiveness and scalability.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this 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.

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.

Copy link

@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: 20

🧹 Nitpick comments (11)
CONTRIBUTING.md (1)

68-71: Clean up markdown formatting
The embedded HTML (<strong style="color:black">�</strong>) and unicode bullets may not render correctly. Replace with standard markdown list syntax or emojis for clarity.

docs/site/10.0.2/api/OsmoDocWord.html (2)

8-9: Remove newline in <title> content.

An embedded newline may break the attribute value. Combine on one line:

<title>Namespace OsmoDoc.Word | Some Documentation</title>

11-12: Consolidate meta name="title" content.

Remove the line break so the content attribute is a single string:

<meta name="title" content="Namespace OsmoDoc.Word | Some Documentation">
OsmoDoc.API/Helpers/Base64StringHelper.cs (2)

5-17: Use GetValue<T> for configuration binding
Instead of parsing the section value manually, leverage IConfiguration.GetValue<long>("CONFIG:UPLOAD_FILE_SIZE_LIMIT_BYTES") for stronger typing and null-safety:

- long uploadFileSizeLimitBytes = Convert.ToInt64(configuration.GetSection("CONFIG:UPLOAD_FILE_SIZE_LIMIT_BYTES").Value);
+ long uploadFileSizeLimitBytes = configuration.GetValue<long>("CONFIG:UPLOAD_FILE_SIZE_LIMIT_BYTES");

5-17: Consider accepting a CancellationToken
To enable cancellation of file I/O operations, add an overload or parameter:

public static async Task SaveBase64StringToFilePath(
    string base64String,
    string filePath,
    IConfiguration configuration,
+   CancellationToken cancellationToken = default)
{
    // ...
-   await File.WriteAllBytesAsync(filePath, data);
+   await File.WriteAllBytesAsync(filePath, data, cancellationToken);
}

Repeat for ConvertFileToBase64String.

Dockerfile (1)

17-18: Simplify restore paths
The ./ segments are redundant. You can streamline to:

- RUN dotnet restore "OsmoDoc.API/OsmoDoc.API.csproj"
- RUN dotnet restore "OsmoDoc/OsmoDoc.csproj"
README.md (1)

126-127: Inconsistent example file paths
The two paths use different folder names—OsmoDoc Component vs. OsmoDoc Service Component. Align these for clarity in the example.

OsmoDoc/Pdf/Models/DocumentData.cs (1)

7-10: Make the collection non-nullable and immutable to prevent null injection

Because the setter is public, callers can still overwrite Placeholders with null, defeating the “non-null list” intent.

-public List<ContentMetaData> Placeholders { get; set; } = new List<ContentMetaData>();
+public List<ContentMetaData> Placeholders { get; init; } = new();

init keeps the property write-able for deserializers but prevents runtime reassignment, and the shorthand new() is cleaner.

OsmoDoc.API/Program.cs (1)

70-76: Prefer configuration binding over direct Environment.GetEnvironmentVariable

Using Environment.GetEnvironmentVariable ties you to process-level vars only. Reading the key from IConfiguration keeps parity with other settings and allows JSON/User-Secrets overrides.

-string JWT_KEY = Environment.GetEnvironmentVariable("JWT_KEY") ?? throw new InvalidOperationException("No JWT key specified");
+string jwtKey = builder.Configuration["JWT_KEY"] 
+               ?? throw new InvalidOperationException("No JWT key specified");
OsmoDoc.API/Models/WordGenerationRequestDTO.cs (1)

22-24: Minor nit: use collection shorthand & init for immutability

-public List<WordContentDataRequestDTO> Placeholders { get; set; } = new List<WordContentDataRequestDTO>();
-public List<TableData> TablesData { get; set; } = new List<TableData>();
+public List<WordContentDataRequestDTO> Placeholders { get; init; } = new();
+public List<TableData> TablesData { get; init; } = new();
OsmoDoc/Word/WordDocumentGenerator.cs (1)

125-135: Avoid Task.Run for pure I/O – use async streams

Wrapping synchronous NPOI I/O in Task.Run offloads to the thread-pool but still blocks a thread per request. Consider using asynchronous file APIs (FileStream with ReadAsync/WriteAsync) or accept synchronous work inside controllers instead of fire-and-forget background tasks.

Also applies to: 142-150

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d055cd3 and e14780a.

⛔ Files ignored due to path filters (1)
  • OsmoDoc.API/wwwroot/Tools/wkhtmltopdf.exe is excluded by !**/*.exe
📒 Files selected for processing (49)
  • .github/PULL_REQUEST_TEMPLATE/pull_request_template_api.md (1 hunks)
  • CONTRIBUTING.md (7 hunks)
  • Dockerfile (2 hunks)
  • DocumentService/DocumentService.csproj (0 hunks)
  • DocumentService/Pdf/Models/ContentMetaData.cs (0 hunks)
  • DocumentService/Pdf/Models/DocumentData.cs (0 hunks)
  • DocumentService/Pdf/PdfDocumentGenerator.cs (0 hunks)
  • DocumentService/Word/Models/ContentData.cs (0 hunks)
  • DocumentService/Word/Models/DocumentData.cs (0 hunks)
  • DocumentService/Word/Models/Enums.cs (0 hunks)
  • DocumentService/Word/Models/TableData.cs (0 hunks)
  • DocumentService/Word/WordDocumentGenerator.cs (0 hunks)
  • OsmoDoc.API/Controllers/PdfController.cs (1 hunks)
  • OsmoDoc.API/Controllers/WordController.cs (1 hunks)
  • OsmoDoc.API/DotEnv.cs (1 hunks)
  • OsmoDoc.API/Helpers/AuthenticationHelper.cs (1 hunks)
  • OsmoDoc.API/Helpers/AutoMappingProfile.cs (1 hunks)
  • OsmoDoc.API/Helpers/Base64StringHelper.cs (1 hunks)
  • OsmoDoc.API/Helpers/CommonMethodsHelper.cs (1 hunks)
  • OsmoDoc.API/Models/BaseResponse.cs (1 hunks)
  • OsmoDoc.API/Models/PdfGenerationRequestDTO.cs (1 hunks)
  • OsmoDoc.API/Models/WordGenerationRequestDTO.cs (1 hunks)
  • OsmoDoc.API/OsmoDoc.API.csproj (1 hunks)
  • OsmoDoc.API/OsmoDoc.API.sln (1 hunks)
  • OsmoDoc.API/Program.cs (1 hunks)
  • OsmoDoc.sln (1 hunks)
  • OsmoDoc/OsmoDoc.csproj (1 hunks)
  • OsmoDoc/Pdf/Models/ContentMetaData.cs (1 hunks)
  • OsmoDoc/Pdf/Models/DocumentData.cs (1 hunks)
  • OsmoDoc/Pdf/Models/OsmoDocPdfConfig.cs (1 hunks)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs (1 hunks)
  • OsmoDoc/Word/Models/ContentData.cs (1 hunks)
  • OsmoDoc/Word/Models/DocumentData.cs (1 hunks)
  • OsmoDoc/Word/Models/Enums.cs (1 hunks)
  • OsmoDoc/Word/Models/TableData.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
  • README.md (6 hunks)
  • docker-compose.yaml (1 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.ContentData.html (5 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.ContentType.html (2 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.DocumentData.html (4 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.ParentBody.html (2 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.TableData.html (3 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.Models.html (2 hunks)
  • docs/site/10.0.2/api/OsmoDoc.Word.WordDocumentGenerator.html (3 hunks)
  • docs/site/10.0.2/api/OsmoDocWord.html (2 hunks)
  • docs/site/10.0.2/api/toc.html (1 hunks)
  • docs/site/manifest.json (1 hunks)
  • docs/site/xrefmap.yml (1 hunks)
💤 Files with no reviewable changes (9)
  • DocumentService/Pdf/Models/ContentMetaData.cs
  • DocumentService/Word/Models/TableData.cs
  • DocumentService/DocumentService.csproj
  • DocumentService/Word/Models/Enums.cs
  • DocumentService/Pdf/Models/DocumentData.cs
  • DocumentService/Word/Models/DocumentData.cs
  • DocumentService/Word/Models/ContentData.cs
  • DocumentService/Word/WordDocumentGenerator.cs
  • DocumentService/Pdf/PdfDocumentGenerator.cs
🧰 Additional context used
🪛 LanguageTool
README.md

[duplication] ~1-~1: Possible typo: you repeated a word.
Context: # OsmoDoc OsmoDoc is a library with the following functio...

(ENGLISH_WORD_REPEAT_RULE)

🪛 GitHub Check: build
OsmoDoc/Pdf/Models/ContentMetaData.cs

[warning] 6-6:
Non-nullable property 'Content' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.


[warning] 5-5:
Non-nullable property 'Placeholder' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.

OsmoDoc/Word/Models/DocumentData.cs

[warning] 22-22:
Non-nullable property 'TablesData' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.


[warning] 16-16:
Non-nullable property 'Placeholders' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.

OsmoDoc/Word/Models/ContentData.cs

[warning] 17-17:
Non-nullable property 'Content' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.


[warning] 12-12:
Non-nullable property 'Placeholder' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.

OsmoDoc/Word/Models/TableData.cs

[warning] 21-21:
Non-nullable property 'Data' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.

OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 300-300:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 300-300:
Dereference of a possibly null reference.


[warning] 294-294:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (43)
.github/PULL_REQUEST_TEMPLATE/pull_request_template_api.md (1)

5-5: Update contributing URL to osmodoc repository
The link now correctly points to the OsmoDoc contributing guidelines.

OsmoDoc.API/Helpers/CommonMethodsHelper.cs (2)

5-19: CreateDirectoryIfNotExists implementation looks solid
The logic correctly handles file-only paths (empty directoryName) and ensures all necessary directories are created.


21-25: GenerateRandomFileName utility is correct
Random filename generation with a GUID suffix is implemented as expected and avoids naming collisions.

OsmoDoc/Pdf/Models/OsmoDocPdfConfig.cs (1)

3-7: Introduce static PDF configuration holder
Centralizing WkhtmltopdfPath in a dedicated config class is appropriate for global settings and simplifies PDF generator initialization.

CONTRIBUTING.md (1)

16-49: Consistently rename project references to osmodoc
All links and mentions of document-service have been updated to osmodoc, ensuring the contributing guide reflects the new project name.

Also applies to: 94-113

OsmoDoc.API/OsmoDoc.API.csproj (1)

15-15: Update project reference to new core library
The API project now correctly references OsmoDoc.csproj instead of the deprecated DocumentService.csproj.

OsmoDoc.API/Models/BaseResponse.cs (1)

1-3: Namespace update is consistent.

The using Microsoft.AspNetCore.Mvc import and namespace rename to OsmoDoc.API.Models align with the refactoring.

OsmoDoc.API/Helpers/AutoMappingProfile.cs (1)

1-5: Namespace and imports updated correctly.

The profile now references OsmoDoc.Word.Models and OsmoDoc.API.Models appropriately.

OsmoDoc.API/DotEnv.cs (1)

1-3: Namespace refactor approved.

Renaming to OsmoDoc.API is consistent; the dotenv loader logic is unchanged.

docs/site/10.0.2/api/OsmoDocWord.html (2)

70-72: Namespace identifiers updated accurately.

The article data-uid and <h1> data-uid now correctly reference OsmoDoc.Word. Using an underscore in the id is acceptable to avoid invalid characters.


80-80: Class link updated.

The <a> tag now points to OsmoDoc.Word.WordDocumentGenerator.html, matching the refactored namespace.

OsmoDoc.API/OsmoDoc.API.sln (2)

6-6: Project reference renamed correctly.

The solution now references OsmoDoc.API.csproj with the same GUID, ensuring continuity.


2-5: Also applies to: 7-26

OsmoDoc.API/Helpers/Base64StringHelper.cs (1)

1-31: Inconsistent summary: Missing async changes
The AI-generated summary only mentions namespace renames but omits that both methods were converted to async/await.

Likely an incorrect or invalid review comment.

docs/site/10.0.2/api/OsmoDoc.Word.Models.ContentType.html (2)

70-80: Documentation update aligns with namespace rename
All namespace and assembly references have been correctly updated from DocumentService to OsmoDoc.


96-100: Enum member IDs updated correctly
Field IDs now use the OsmoDoc.Word.Models.ContentType namespace and match the code.

OsmoDoc.API/Models/PdfGenerationRequestDTO.cs (2)

1-1: Import updated for new PDF models namespace
The using directive now points to OsmoDoc.Pdf.Models, which matches the relocated ContentMetaData and DocumentData classes.


4-4: Namespace corrected to OsmoDoc.API.Models
The DTO’s namespace has been renamed appropriately.

docs/site/10.0.2/api/OsmoDoc.Word.Models.ParentBody.html (2)

70-75: Consistent UID and ID namespace rename
The data-uid and id attributes have been correctly updated from DocumentService.Word.Models.ParentBody to OsmoDoc.Word.Models.ParentBody. No residual references found.


79-80: Assembly reference updated correctly
The <h6> tag now points to OsmoDoc.dll as expected. Documentation aligns with the new project name.

docs/site/10.0.2/api/OsmoDoc.Word.Models.html (4)

8-12: Head metadata updated for new namespace
The <title> and <meta name="title"> entries reflect OsmoDoc.Word.Models. Ensure these match all other docs pages.


70-73: Article data-uid and heading IDs renamed
The id and data-uid on the <article> and <h1> have been updated to OsmoDoc.Word.Models. Good consistency.


80-85: Class links updated to OsmoDoc namespace
All <a href> entries for ContentData, DocumentData, and TableData now point to OsmoDoc.Word.Models.*. No old references detected.


89-92: Enum links updated correctly
The links for ContentType and ParentBody have been updated to the new namespace. Documentation is in sync.

docs/site/10.0.2/api/OsmoDoc.Word.Models.TableData.html (3)

70-75: Article identifiers renamed
The <article> data-uid and <h1> id/data-uid values now use OsmoDoc.Word.Models.TableData. Looks consistent.


107-110: Namespace, assembly, and syntax block updated
The namespace declaration, assembly name, and syntax section ID correctly reference OsmoDoc.Word.Models.TableData.


115-116: Property anchors updated
The anchors and headings for Data and TablePos have been updated to include the new namespace in their data-uid values.

OsmoDoc.API/Helpers/AuthenticationHelper.cs (1)

6-6: Namespace updated for project rebrand
The namespace has been changed to OsmoDoc.API.Helpers. Please verify that all dependent files and imports reference this updated namespace to prevent build errors.

README.md (3)

27-27: Clone command updated
The repository clone step now correctly instructs users to clone osmodoc instead of the old document-service.


52-52: Docker-compose instruction updated
The example command now targets osmodoc in docker-compose.yaml, matching the new project name.


190-195: License and contributor links updated
The license reference and contributors badge now point to osmodoc on GitHub. All links appear correct.

docs/site/manifest.json (1)

3-3: Avoid absolute, user-specific paths in DocFX manifest

source_base_path points to a local Windows user directory (C:/Users/user/Desktop/osmodoc).
This breaks builds on CI agents and other developers’ machines.

-"source_base_path": "C:/Users/user/Desktop/osmodoc",
+"source_base_path": "../../..",  // or any repo-relative path

Consider making the path repo-relative or leave it empty to let DocFX infer the path.

docs/site/10.0.2/api/OsmoDoc.Word.Models.ContentData.html (1)

70-76: Namespace rename reflected correctly – no action required

The HTML shows the expected switch from DocumentService to OsmoDoc; no broken anchors or mismatched data-uid attributes detected in the touched lines.

Also applies to: 107-110

docker-compose.yaml (1)

2-7: Renaming looks good

Service, image, and container names now align with the new branding; compose syntax unchanged.

OsmoDoc.sln (1)

6-8: Solution file project references updated.
Project entries have been updated to reference OsmoDoc and OsmoDoc.API. Ensure that no stale references to the old DocumentService remain in the solution file.

docs/site/10.0.2/api/toc.html (1)

17-43: TOC links updated correctly.
All entries have been renamed from DocumentService to OsmoDoc. No stale or broken links detected in the table of contents.

OsmoDoc/Word/Models/Enums.cs (1)

1-36: Enums defined with correct namespace and documentation.
The ContentType and ParentBody enums are properly scoped under OsmoDoc.Word.Models and include clear XML documentation.

docs/site/xrefmap.yml (1)

1-196: Documentation only update – nothing to flag.

docs/site/10.0.2/api/OsmoDoc.Word.Models.DocumentData.html (5)

70-70: Confirm updated data-uid on <article>
The data-uid attribute has been correctly changed to OsmoDoc.Word.Models.DocumentData, matching the new namespace.


74-74: Verify <h1> ID and data-uid update
The <h1> tag’s id and data-uid now reflect OsmoDoc.Word.Models.DocumentData—this aligns with the refactoring.


107-108: Namespace and assembly references updated
Both the displayed namespace and assembly name have been changed to OsmoDoc/OsmoDoc.dll, which is consistent with the project-wide rename.


133-133: ContentData link updated
The ContentData type link now points to OsmoDoc.Word.Models.ContentData.html, correctly reflecting its new location.


156-156: TableData link updated
The TableData type link has been updated to OsmoDoc.Word.Models.TableData.html, matching the renamed namespace.

Copy link

@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: 2

♻️ Duplicate comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

43-60: Duplicate-placeholder crash resolved – good improvement

Switching from Dictionary.Add to TryAdd prevents runtime failures when a placeholder repeats. 👍


290-306: Null-safety gaps when replacing images

docProperty.Name, blip.Embed, and MainDocumentPart may all be null, matching the static-analysis warnings.
Dereferencing them without checks can crash the generator.

-if (docProperty != null && imagePlaceholders.ContainsKey(docProperty.Name))
+if (docProperty?.Name is { } name
+    && imagePlaceholders.TryGetValue(name, out var imagePath))
 {
-    ...
-    OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(blip.Embed);
+    var partId = blip?.Embed?.Value;
+    if (partId is null || !wordDocument.MainDocumentPart!.PartExists(partId))
+        continue;
+
+    OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(partId);
🧹 Nitpick comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

33-35: Redundant try/catch that simply re-throws

The catch (Exception) { throw; } block adds no value and obscures stack traces when future logging is added. Remove the wrapper or log/contextualize the exception before rethrowing.

Also applies to: 114-117


142-150: Task.Run around synchronous file I/O is sub-optimal

FileStream/WriteAsync already provide async APIs. Wrapping them in Task.Run offloads work to the thread-pool without yielding true async I/O.

Consider rewriting WriteDocument and GetXWPFDocument to use the native async methods (File.OpenRead, File.Create, CopyToAsync, etc.) without Task.Run.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between e14780a and e0b3638.

📒 Files selected for processing (2)
  • OsmoDoc.API/Controllers/WordController.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • OsmoDoc.API/Controllers/WordController.cs
🧰 Additional context used
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 300-300:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 300-300:
Dereference of a possibly null reference.


[warning] 294-294:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)

Copy link

@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: 2

♻️ Duplicate comments (1)
OsmoDoc/Word/WordDocumentGenerator.cs (1)

290-299: Potential null-reference and invalid key access during image replacement

docProperty.Name, blip.Embed, and their downstream uses can be null, triggering runtime exceptions and static-analysis warnings.

-if (docProperty != null && imagePlaceholders.ContainsKey(docProperty.Name))
+if (docProperty?.Name is { } placeholder
+    && imagePlaceholders.TryGetValue(placeholder, out var imagePath))
 {
-    List<Blip> drawingBlips = drawing.Descendants<Blip>().ToList();
+    foreach (Blip blip in drawing.Descendants<Blip>())
     {
-        OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(blip.Embed);
+        string? partId = blip.Embed?.Value;
+        if (partId is null) continue;
+        if (!wordDocument.MainDocumentPart!.PartExists(partId)) continue;
+
+        OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(partId);
 
-        string imagePath = imagePlaceholders[docProperty.Name];
+        // imagePath already retrieved via TryGetValue

This guards all nullable access points and removes the illegal lookup with a possibly null key.

🧹 Nitpick comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

125-132: Avoid wrapping pure I/O in Task.Run

File.OpenRead is non-blocking on modern OS file systems; the expensive work happens when the stream is consumed. Off-loading the open call to the thread-pool adds overhead without real benefit. Either keep it synchronous or return a ValueTask<XWPFDocument> that opens the stream directly.

-private async static Task<XWPFDocument> GetXWPFDocument(string docFilePath)
-{
-    return await Task.Run(() =>
-    {
-        FileStream readStream = File.OpenRead(docFilePath);
-        return new XWPFDocument(readStream);
-    });
-}
+private static Task<XWPFDocument> GetXWPFDocument(string docFilePath)
+{
+    FileStream readStream = File.OpenRead(docFilePath);
+    return Task.FromResult(new XWPFDocument(readStream));
+}

74-81: Placeholder regex too restrictive

@"{[a-zA-Z]+}" fails for placeholders containing digits, underscores or hyphens (e.g., {First_Name}, {address1}). Consider widening the character class:

-if (!new Regex(@"{[a-zA-Z]+}").IsMatch(paragraph.ParagraphText))
+if (!Regex.IsMatch(paragraph.ParagraphText, @"{\w+}")

\w matches letters, digits and underscore, covering common cases.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 66031ed and 5a6e3c6.

📒 Files selected for processing (2)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs
🧰 Additional context used
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 297-297:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 297-297:
Dereference of a possibly null reference.


[warning] 291-291:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
OsmoDoc/Word/WordDocumentGenerator.cs (1)

39-60: 👍 Duplicate-placeholder handling looks good

TryAdd eliminates the crash path previously flagged in older reviews—nice fix.

@osm-Jatin osm-Jatin force-pushed the feat/asynchronous branch from 5a6e3c6 to 76fcb3f Compare June 18, 2025 14:17
Copy link

@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: 3

♻️ Duplicate comments (3)
OsmoDoc/Word/WordDocumentGenerator.cs (3)

139-148: Ensure output directory exists before File.Create
Without guaranteeing the directory, File.Create throws DirectoryNotFoundException for nested paths. This was flagged earlier but hasn’t been applied in the renamed file.

 private async static Task WriteDocument(XWPFDocument document, string filePath)
 {
     await Task.Run(() =>
     {
-        using (FileStream writeStream = File.Create(filePath))
+        // Ensure parent directory is present
+        string? dir = Path.GetDirectoryName(filePath);
+        if (!string.IsNullOrWhiteSpace(dir))
+        {
+            Directory.CreateDirectory(dir);
+        }
+
+        using (FileStream writeStream = File.Create(filePath))
         {
             document.Write(writeStream);
         }
     });
 }

286-306: Add null-safety for DocProperties.Name, blip.Embed and part lookup
docProperty.Name, blip.Embed and wordDocument.MainDocumentPart.GetPartById can all be null, triggering runtime exceptions (the static-analysis warnings still fire).

-                    // If drawing / image name is present in imagePlaceholders dictionary, then replace image
-                    if (docProperty != null && imagePlaceholders.ContainsKey(docProperty.Name))
+                    // Safely resolve placeholder & part
+                    if (docProperty?.Name is { } name
+                        && imagePlaceholders.TryGetValue(name, out var imagePath))
                     {
                         List<Blip> drawingBlips = drawing.Descendants<Blip>().ToList();
 
                         foreach (Blip blip in drawingBlips)
                         {
-                            OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(blip.Embed);
+                            string? partId = blip?.Embed?.Value;
+                            if (partId is null || !wordDocument.MainDocumentPart!.PartExists(partId))
+                                continue;
+
+                            OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(partId);
 
-                            string imagePath = imagePlaceholders[docProperty.Name];
-
                             // Asynchronously download image data using HttpClient
                             byte[] imageData = await _httpClient.GetByteArrayAsync(imagePath);

236-250: Row population still assumes cells already exist
Iterating over row.GetTableCells().Count on a freshly created row (often 0) leaves new rows blank. Iterate over the header’s column count and add cells on demand.

-            // For each cell in row
-            for (int cellNumber = 0; cellNumber < row.GetTableCells().Count; cellNumber++)
+            int columnCount = headerRow.GetTableCells().Count;
+            for (int cellNumber = 0; cellNumber < columnCount; cellNumber++)
             {
-                XWPFTableCell cell = row.GetCell(cellNumber);
+                // Ensure the row has enough cells
+                while (row.GetTableCells().Count <= cellNumber)
+                    row.AddNewTableCell();
+
+                XWPFTableCell cell = row.GetCell(cellNumber);
🧹 Nitpick comments (3)
OsmoDoc/Word/WordDocumentGenerator.cs (1)

74-78: Use string.IsNullOrWhiteSpace for clarity
A minor readability/nitpick: checking paragraph.ParagraphText == string.Empty ignores whitespace-only paragraphs. string.IsNullOrWhiteSpace expresses the intent more clearly.

OsmoDoc/Pdf/PdfDocumentGenerator.cs (2)

68-71: Redundant catch / re-throw adds noise

Catching Exception only to throw; again provides no additional value and hides the real stack frame in some profilers. You can safely delete the whole try/catch block and let the original exception propagate.


100-148: output / errors variables are read but never used

Lines 146-147 read the process streams into output and errors, but only errors is later referenced inside the exit-code block. output is dead code. Either log it or remove the read to avoid unnecessary buffering.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5a6e3c6 and 76fcb3f.

📒 Files selected for processing (5)
  • OsmoDoc.API/Controllers/PdfController.cs (1 hunks)
  • OsmoDoc.API/Controllers/WordController.cs (1 hunks)
  • OsmoDoc/OsmoDoc.csproj (1 hunks)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • OsmoDoc/OsmoDoc.csproj
🚧 Files skipped from review as they are similar to previous changes (2)
  • OsmoDoc.API/Controllers/WordController.cs
  • OsmoDoc.API/Controllers/PdfController.cs
🧰 Additional context used
🧠 Learnings (1)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/osmodoc#40
File: OsmoDoc/Pdf/PdfDocumentGenerator.cs:24-37
Timestamp: 2025-06-18T14:03:39.722Z
Learning: In OsmoDoc PdfDocumentGenerator.GeneratePdf method, an empty metaDataList is a valid scenario when users want to convert HTML/EJS templates to PDF without any placeholder replacements. Only null metaDataList should be validated, not empty ones.
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 297-297:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 297-297:
Dereference of a possibly null reference.


[warning] 291-291:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
OsmoDoc/Word/WordDocumentGenerator.cs (1)

93-96: ✅ Guard for TablePos looks solid

Nice catch adding the insertData.TablePos >= 1 lower-bound check—the off-by-one issue is gone.

Copy link

@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

♻️ Duplicate comments (4)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

307-313: Implement null safety checks for image replacement.

The code still has potential null reference issues that were flagged in previous reviews but not fully addressed:

Apply the suggested null safety pattern:

-                    if (docProperty != null && imagePlaceholders.ContainsKey(docProperty.Name))
+                    if (docProperty?.Name is { } name && imagePlaceholders.TryGetValue(name, out var imagePath))
                     {
                         List<Blip> drawingBlips = drawing.Descendants<Blip>().ToList();

                         foreach (Blip blip in drawingBlips)
                         {
-                            OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(blip.Embed);
+                            var partId = blip?.Embed?.Value;
+                            if (partId is not null && wordDocument.MainDocumentPart!.PartExists(partId))
+                            {
+                                OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(partId);

-                            string imagePath = imagePlaceholders[docProperty.Name];
                             // ... rest of the logic
+                            }
                         }
                     }

254-266: Fix row cell-count mismatch when populating tables.

This issue was identified in previous reviews but remains unaddressed. row.GetTableCells().Count on a newly created row may be 0, leaving the row blank.

Apply the suggested fix:

-            for (int cellNumber = 0; cellNumber < row.GetTableCells().Count; cellNumber++)
+            int columnCount = headerRow.GetTableCells().Count;
+            for (int cellNumber = 0; cellNumber < columnCount; cellNumber++)
+            {
+                // Ensure the row has enough cells
+                while (row.GetTableCells().Count <= cellNumber)
+                    row.AddNewTableCell();
+
                 XWPFTableCell cell = row.GetCell(cellNumber);
OsmoDoc/Pdf/PdfDocumentGenerator.cs (2)

98-99: Address concurrent access issues with fixed directory names.

Using fixed directory names like "Modified" creates race conditions when multiple PDF generation requests execute concurrently. This was identified in previous reviews but remains unaddressed.

Apply the suggested fix:

-        string tempHtmlFilePath = Path.Combine(directoryPath, "Modified");
-        string tempHtmlFile = Path.Combine(tempHtmlFilePath, "modifiedHtml.html");
+        string uniqueId = Guid.NewGuid().ToString("N");
+        string tempHtmlFilePath = Path.Combine(directoryPath, $"Modified_{uniqueId}");
+        string tempHtmlFile = Path.Combine(tempHtmlFilePath, "modifiedHtml.html");

180-180: Apply unique naming to "Temp" directory for concurrent safety.

The fixed "Temp" directory name creates the same concurrency issues as mentioned for the "Modified" directory.

Apply similar fix:

-        string tempDirectoryFilePath = Path.Combine(directoryPath, "Temp");
+        string uniqueId = Guid.NewGuid().ToString("N");
+        string tempDirectoryFilePath = Path.Combine(directoryPath, $"Temp_{uniqueId}");
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 76fcb3f and 8944612.

📒 Files selected for processing (2)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (2)
Learnt from: osm-Jatin
PR: OsmosysSoftware/osmodoc#40
File: OsmoDoc/Pdf/PdfDocumentGenerator.cs:24-37
Timestamp: 2025-06-18T14:03:39.722Z
Learning: In OsmoDoc PdfDocumentGenerator.GeneratePdf method, an empty metaDataList is a valid scenario when users want to convert HTML/EJS templates to PDF without any placeholder replacements. Only null metaDataList should be validated, not empty ones.
Learnt from: osm-Jatin
PR: OsmosysSoftware/osmodoc#40
File: OsmoDoc/Pdf/PdfDocumentGenerator.cs:84-92
Timestamp: 2025-06-18T14:44:46.549Z
Learning: osm-Jatin prefers to document input requirements clearly in the README file rather than adding runtime validation code for scenarios like null/duplicate placeholders in the OsmoDoc PDF generation functionality.
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[failure] 151-151:
'Path' is an ambiguous reference between 'DocumentFormat.OpenXml.Drawing.Path' and 'System.IO.Path'


[warning] 313-313:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 313-313:
Dereference of a possibly null reference.


[warning] 307-307:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.


[failure] 151-151:
'Path' is an ambiguous reference between 'DocumentFormat.OpenXml.Drawing.Path' and 'System.IO.Path'

🪛 GitHub Actions: Build and Test
OsmoDoc/Word/WordDocumentGenerator.cs

[error] 151-151: CS0104: 'Path' is an ambiguous reference between 'DocumentFormat.OpenXml.Drawing.Path' and 'System.IO.Path'.

🔇 Additional comments (2)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (2)

262-262: Path quoting properly implemented for Linux.

Great! The file paths are now properly quoted in the Linux branch, addressing the previous concern about paths containing spaces.


23-23: Excellent async implementation throughout the class.

The asynchronous pattern is properly implemented with:

  • Correct use of async/await keywords
  • Proper use of Task.Run for CPU-bound operations
  • Process.WaitForExitAsync() for process waiting
  • Async file operations where available

This aligns well with the PR objective of making core functions asynchronous.

@osm-Jatin osm-Jatin force-pushed the feat/asynchronous branch from 8944612 to e0d94d6 Compare June 19, 2025 03:40
Copy link

@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

♻️ Duplicate comments (3)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (1)

181-181: Fix potential concurrency issue with fixed temp directory name.

The method uses a fixed "Temp" directory name which could cause collisions during concurrent requests, similar to the issue that was resolved in ReplaceFileElementsWithMetaData.

Apply the same unique directory approach:

-        string tempDirectoryFilePath = Path.Combine(directoryPath, "Temp");
+        string uniqueId = Guid.NewGuid().ToString("N");
+        string tempDirectoryFilePath = Path.Combine(directoryPath, $"Temp_{uniqueId}");
OsmoDoc/Word/WordDocumentGenerator.cs (2)

255-268: Fix row cell count mismatch when populating tables

The loop uses row.GetTableCells().Count on a newly created row, which can be zero because NPOI doesn't auto-create cells to match the header row. This results in no cells being populated.

Apply this fix to ensure proper cell population:

-for (int cellNumber = 0; cellNumber < row.GetTableCells().Count; cellNumber++)
+int columnCount = headerRow.GetTableCells().Count;
+for (int cellNumber = 0; cellNumber < columnCount; cellNumber++)
 {
+    // Ensure the row has enough cells
+    while (row.GetTableCells().Count <= cellNumber)
+        row.AddNewTableCell();
+        
     XWPFTableCell cell = row.GetCell(cellNumber);

308-314: Add null safety checks for image replacement

The static analysis warnings are valid - docProperty.Name and blip.Embed can be null, leading to runtime exceptions.

Apply this fix to add proper null checks:

-if (docProperty != null && imagePlaceholders.ContainsKey(docProperty.Name))
+if (docProperty?.Name != null && imagePlaceholders.ContainsKey(docProperty.Name))
 {
     List<Blip> drawingBlips = drawing.Descendants<Blip>().ToList();

     foreach (Blip blip in drawingBlips)
     {
+        if (blip.Embed?.Value == null)
+            continue;
+            
         OpenXmlPart imagePart = wordDocument.MainDocumentPart.GetPartById(blip.Embed);
🧹 Nitpick comments (1)
OsmoDoc/Word/WordDocumentGenerator.cs (1)

301-301: Consider performance optimization for image replacement loop

The FIXME comment suggests improving the nested loop operation. Consider caching the image placeholder lookups or using a more efficient approach.

You could optimize this by:

  1. Pre-filtering drawings that have DocProperties with names in the imagePlaceholders dictionary
  2. Using parallel processing for independent image downloads
  3. Implementing a lookup cache for repeated placeholder names

Would you like me to generate an optimized version of this image replacement logic?

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8944612 and 83181c8.

📒 Files selected for processing (2)
  • OsmoDoc/Pdf/PdfDocumentGenerator.cs (1 hunks)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (2)
Learnt from: osm-Jatin
PR: OsmosysSoftware/osmodoc#40
File: OsmoDoc/Pdf/PdfDocumentGenerator.cs:24-37
Timestamp: 2025-06-18T14:03:39.722Z
Learning: In OsmoDoc PdfDocumentGenerator.GeneratePdf method, an empty metaDataList is a valid scenario when users want to convert HTML/EJS templates to PDF without any placeholder replacements. Only null metaDataList should be validated, not empty ones.
Learnt from: osm-Jatin
PR: OsmosysSoftware/osmodoc#40
File: OsmoDoc/Pdf/PdfDocumentGenerator.cs:84-92
Timestamp: 2025-06-18T14:44:46.549Z
Learning: osm-Jatin prefers to document input requirements clearly in the README file rather than adding runtime validation code for scenarios like null/duplicate placeholders in the OsmoDoc PDF generation functionality.
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 314-314:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 314-314:
Dereference of a possibly null reference.


[warning] 308-308:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
OsmoDoc/Pdf/PdfDocumentGenerator.cs (5)

1-14: Class structure and imports look good.

The using statements include all necessary namespaces for async PDF generation, JSON handling, and cross-platform operations. The class declaration is appropriate.


23-82: Excellent async implementation of the main PDF generation method.

The method properly implements asynchronous patterns with comprehensive input validation, appropriate exception handling, and resource cleanup. The conversion from synchronous to async is well-executed and aligns with the PR objectives.


84-109: Good implementation with concurrency fix.

The method correctly addresses the potential concurrency issue by using unique GUID-based directory names. The placeholder replacement logic is clean and functional.


111-171: Well-implemented async PDF conversion with proper cross-platform support.

The method demonstrates excellent async practices with proper process management, cross-platform compatibility, and resource cleanup. The quoted file paths address previous concerns about handling paths with spaces.


242-269: Helper methods are well-implemented.

The IsValidJSON method uses appropriate exception handling, and the EjsToHtmlArgumentsBasedOnOS method properly handles cross-platform differences with quoted file paths for both Windows and Linux.

OsmoDoc/Word/WordDocumentGenerator.cs (5)

1-128: Inconsistency between PR objectives and actual changes

The PR objectives mention making core functions asynchronous, but the AI summary and code changes indicate this is actually a project rename from "DocumentService" to "OsmoDoc" with equivalent async functionality already implemented.

Likely an incorrect or invalid review comment.


24-24: Good use of static HttpClient

The static HttpClient instance follows best practices for avoiding socket exhaustion and improves performance for multiple HTTP requests.


59-59: Proper use of TryAdd prevents duplicate key exceptions

The use of TryAdd correctly handles potential duplicate placeholders without throwing exceptions, addressing the previous review concern.

Also applies to: 64-64, 69-69


104-104: Correct bounds checking prevents off-by-one errors

The condition insertData.TablePos >= 1 properly guards against negative indexing, addressing the previous review concern.


152-156: Proper directory creation and Path disambiguation

The directory creation logic and use of IOPath alias correctly address previous review comments about missing directories and ambiguous Path references.

@osm-Jatin osm-Jatin force-pushed the feat/asynchronous branch from e84b4b3 to 325426a Compare June 19, 2025 04:23
Copy link

@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: 4

🧹 Nitpick comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

24-24: Consider HttpClient lifecycle management.

The static HttpClient instance will persist for the application lifetime. While this is generally acceptable, consider implementing IDisposable or using a factory pattern for better resource management in larger applications.


189-189: Questionable paragraph spacing modification.

Setting SpacingAfter = 0 for every paragraph that contains placeholders may not be desirable, as it removes spacing that might be intentionally part of the document formatting.

Consider removing this line or making it conditional based on the document requirements:

-            paragraph.SpacingAfter = 0;
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4950f3e and 3f78650.

📒 Files selected for processing (1)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 316-316:
Possible null reference argument for parameter 'id' in 'OpenXmlPart OpenXmlPartContainer.GetPartById(string id)'.


[warning] 316-316:
Dereference of a possibly null reference.


[warning] 310-310:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)

Copy link

@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: 2

♻️ Duplicate comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

83-83: Extract regex pattern to a constant.

The regex pattern @"{[a-zA-Z]+}" is duplicated in multiple locations (lines 83, 169, 204), violating the DRY principle.

Extract it to a private constant:

public static class WordDocumentGenerator
{
+   private const string PlaceholderPattern = @"{[a-zA-Z]+}";

-   if (paragraph.ParagraphText == string.Empty || !new Regex(@"{[a-zA-Z]+}").IsMatch(paragraph.ParagraphText))
+   if (paragraph.ParagraphText == string.Empty || !new Regex(PlaceholderPattern).IsMatch(paragraph.ParagraphText))

303-309: Address null safety issues flagged by static analysis.

The static analysis correctly identifies potential null reference issues that were previously flagged in past reviews.

The following null safety issues need to be addressed:

  1. docProperty.Name could be null when used as dictionary key (line 303)
  2. blip.Embed could be null when passed to GetPartById (line 309)

These issues were mentioned as fixed in PR #43, but appear to still be present in this code version.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3f78650 and 36db8f2.

📒 Files selected for processing (1)
  • OsmoDoc/Word/WordDocumentGenerator.cs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
OsmoDoc/Word/WordDocumentGenerator.cs

[warning] 309-309:
Dereference of a possibly null reference.


[warning] 303-303:
Possible null reference argument for parameter 'key' in 'bool Dictionary<string, string>.ContainsKey(string key)'.


[warning] 146-146:
This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
OsmoDoc/Word/WordDocumentGenerator.cs (2)

30-127: Async conversion successfully implemented!

The method signature and implementation correctly support asynchronous operations as intended by the PR objectives. The use of TryAdd properly handles duplicate placeholders, and the overall structure maintains good separation of concerns.

The redundant try-catch block (lines 123-126) remains as previously discussed - understood this will be addressed later.


134-139: Excellent fix for stream disposal issue!

Using File.ReadAllBytesAsync with a MemoryStream elegantly solves the premature stream disposal problem while maintaining async operation. The XWPFDocument now has reliable access to its data throughout its lifecycle.

@osm-Jatin osm-Jatin force-pushed the feat/asynchronous branch from 36db8f2 to 5f0e3bf Compare June 19, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants