feat: allow reading a password protected excel file

[npv12]

Important

Add support for processing password-protected Excel files in statement import and preview.

• Frontend:
  • Add PasswordPrompt component in ImportStatementModal.tsx to handle password input for protected Excel files.
  • Update usePreviewStatement and useUploadStatement in useStatements.ts to include password handling.
  • Modify previewStatement and uploadStatement in statement.ts to accept password parameter.
• Backend:
  • Update ParseStatementInput and PreviewStatementForm in models/statement.go to include password field.
  • Modify ParseStatement and PreviewStatement in statement_service.go to handle password validation and error.
  • Implement password handling in CustomParser, SBIParser, and other parsers to open password-protected Excel files.
• Error Handling:
  • Add NewStatementPasswordRequiredError in errors/statement.go for password-related errors.
  • Implement isStatementPasswordRequiredError in errors.ts to identify password errors.
• Testing:
  • Add tests for password-protected Excel files in statement_controller_test.go and parser tests.

^{This description was created by for 23a5c25. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 0595fab in 2 minutes and 7 seconds. Click for details.

• Reviewed 1723 lines of code in 21 files
• Skipped 0 files when reviewing.
• Skipped posting 9 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. server/internal/parser/custom_parser.go:136

• Draft comment:
  Update the unsupported file type error message to accurately list all supported formats (e.g. .csv, .xls, .xlsx, .txt) instead of only .csv and .xls.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. server/internal/service/statement_service.go:53

• Draft comment:
  Consider trimming the input password (e.g. using strings.TrimSpace) before checking if it is empty to avoid false negatives due to accidental whitespace.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code quality suggestion about input validation. The comment is about newly added code (the password checking logic). However, I need to consider: 1) Is this an obvious/unimportant suggestion? 2) Is this speculative? 3) Does this require a clear code change? The suggestion is somewhat speculative - it assumes users might accidentally add whitespace to passwords. In many systems, passwords are treated literally and whitespace is considered part of the password. Trimming passwords could actually break valid passwords that intentionally include leading/trailing spaces. This is a design decision, not a clear bug. The comment also uses "Consider" which makes it sound like a suggestion rather than identifying a definite issue. The comment assumes that whitespace in passwords is always accidental, but passwords with whitespace could be intentional. Trimming passwords is a design choice that could break valid use cases. This is more of a speculative suggestion than identifying a clear problem. While the critique is valid that passwords could intentionally have whitespace, the comment is still speculative about user behavior ("accidental whitespace"). It's not identifying a definite bug, just suggesting a potential improvement. This falls under the rule about not making speculative comments. The system should probably accept passwords as-is. This comment should be deleted. It's a speculative suggestion about handling accidental whitespace rather than identifying a clear bug. Password handling is a design decision, and trimming could break valid passwords. The comment doesn't identify strong evidence of an actual problem.

3. server/internal/service/statement_service.go:221

• Draft comment:
  Ensure that the preview validation includes checks for empty file bytes and blank filenames so that errors are clear; also verify that the error message (e.g. 'filename cannot be empty') propagates properly.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 1% The comment is asking the author to ensure that certain validations are in place and that error messages propagate properly. This falls under asking the author to ensure behavior is intended or tested, which is against the rules.

4. server/internal/service/statement_service_test.go:995

• Draft comment:
  Avoid relying on errors.Unwrap(err) to check error messages in tests; consider comparing err.Error() directly or using errors.As for more robust error matching.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. Overall:1

• Draft comment:
  Overall, the implementation for handling password-protected Excel files is well integrated across UI, API, and backend layers. Ensure that sensitive data (like passwords) isn’t inadvertently logged anywhere and that error messages remain consistent across parsers.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. frontend/components/custom/Modal/Statement/ImportStatementModal.tsx:217

• Draft comment:
  Typographical note: The comment reads "For custom parser (Preview step), only take first file." Consider revising it to "only take the first file" for improved clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a purely stylistic comment about grammar in a code comment. The automated tool is suggesting to change "only take first file" to "only take the first file" - adding the article "the". While this is technically more grammatically correct, this falls under the category of "obvious or unimportant" comments. The meaning is completely clear either way, and this is an extremely minor stylistic issue. According to the rules, I should "Do NOT make comments that are obvious or unimportant." This is definitely unimportant - it's just about adding the word "the" to a comment. This doesn't affect code quality, readability in any meaningful way, or functionality. Could this be part of a larger effort to maintain consistent documentation standards? Perhaps the codebase has strict style guidelines for comments that require proper grammar. Even if there are strict documentation standards, this is such a minor grammatical issue that it's not worth a PR comment. The meaning is completely clear without "the", and this type of nitpicky grammar correction is exactly the kind of unimportant comment that should be filtered out. If grammar standards are important, they should be enforced by a linter, not manual PR comments. This comment should be deleted. It's suggesting a trivial grammatical change to a code comment (adding the word "the") that doesn't improve clarity in any meaningful way. This falls squarely under "obvious or unimportant" comments that should not be made.

7. server/internal/parser/custom_parser.go:123

• Draft comment:
  Typographical consistency: The error message on this line says "failed to open xlsx file" (all lowercase), whereas other log messages use "XLSX" in uppercase. Consider using a consistent casing (e.g., "XLSX") throughout the messages.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a very minor stylistic comment about casing consistency. The rules state "Do NOT make comments that are obvious or unimportant." This is arguably unimportant - it's purely cosmetic and doesn't affect functionality. However, the rules also say "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This is actionable and clear, but is it really a "code quality refactor"? It's more of a style nitpick. The comment is about a change that was made (the new XLSX handling code), so it does relate to the diff. But the question is whether this rises to the level of importance that warrants keeping it. While consistency is generally good, this is an extremely minor issue that doesn't affect code functionality, readability in any meaningful way, or maintainability. The rules explicitly say not to make comments that are "obvious or unimportant," and this falls into the unimportant category. It's the kind of nitpick that could be caught by a linter or style guide, not something that needs human review. The critique is valid. This is indeed a very minor stylistic issue. While consistency is nice, this doesn't meet the bar for "clearly a code change required" - the code works fine as-is. The inconsistency is so minor that it's not worth the PR author's time to address. This comment should be deleted. It's too minor and unimportant to warrant keeping. The casing inconsistency doesn't affect functionality or meaningfully impact code quality. This is the type of nitpick that the rules explicitly say to avoid.

8. server/internal/parser/hdfc_parser_test.go:152

• Draft comment:
  Typo detected: The header string contains 'Value Dat'. It likely should be 'Value Date'.
• Reason this comment was not posted:
  Comment was on unchanged code.

9. server/internal/service/statement_service.go:230

• Draft comment:
  Typographical error: The if-statement has an extra set of parentheses around the condition. It should be written as: if protected := parser.IsExcelPasswordProtectedBytes(fileBytes); protected && password == "" { Please remove the unnecessary parentheses.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a minor stylistic issue. The extra parentheses don't affect functionality - the code works correctly either way. While the comment is technically correct, it falls into the category of "obvious or unimportant" changes. The rules state "Do NOT make comments that are obvious or unimportant." This is a very minor style nitpick that doesn't affect code behavior, readability in any meaningful way, or introduce bugs. Most linters wouldn't flag this, and it's not a significant code quality issue. The comment is about something that was changed (the line was added in the diff), so it does relate to the changes. While this is a minor style issue, it could be argued that consistency and clean code style are important. The parentheses are genuinely unnecessary and removing them would make the code slightly cleaner. However, this is such a trivial issue that it likely doesn't warrant a review comment. Even though code style matters, this is an extremely minor issue that doesn't affect functionality, performance, or readability in any meaningful way. The rules explicitly state not to make obvious or unimportant comments, and this falls squarely into that category. It's the kind of thing that could be caught by an automated linter if the team cared about it. This comment should be deleted. While technically correct, it's pointing out an extremely minor stylistic issue with unnecessary parentheses that doesn't affect code behavior or readability in any meaningful way. This falls under "obvious or unimportant" comments that should not be made.

Workflow ID: wflow_iQFbG71g0tn7f3uV

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed e5f365b in 1 minute and 5 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. server/internal/api/controller/statement_controller_test.go:45

• Draft comment:
  Iteration count increased from 8 to 20, extending the wait time (up to 100s with 5s sleep). This helps avoid premature test failures for longer processing times, but consider parameterizing these values to avoid unnecessary slowdowns in test runs.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_f7BD65PYMaBDaZo7

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4c801dc in 1 minute and 38 seconds. Click for details.

• Reviewed 22 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. server/pkg/database/manager/postgres/postgres_test.go:29

• Draft comment:
  Unnecessary parentheses in the if condition. Use: if os.Getenv("DB_PORT") == "" { ... }
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code style comment about removing unnecessary parentheses. While technically correct, I need to consider: 1) Is this an important enough issue to warrant a comment? 2) Does it violate the rules about obvious or unimportant comments? 3) Is this something that would be caught by linters/formatters? In Go, unnecessary parentheses in if conditions are generally considered non-idiomatic, and tools like go fmt or linters might flag this. However, go fmt doesn't actually remove these parentheses - it's more of a style guide issue. The comment is actionable and provides a clear suggestion. But it's also quite minor and could be considered "obvious" - any Go developer would know parentheses aren't needed here. This is a very minor style issue that doesn't affect functionality. The rules state "Do NOT make comments that are obvious or unimportant" - this could fall into that category. While it's technically correct, it's debatable whether removing these parentheses is important enough to warrant a review comment, especially since the code works fine as-is. While minor, this is a legitimate style issue in Go. The comment is actionable, provides a clear fix, and helps maintain idiomatic Go code. It's not about functionality but about code quality, which the rules explicitly say is good if "actionable and clear." However, given how minor this is and that it doesn't impact behavior, it leans toward being "unimportant." This is a borderline case - it's a valid style suggestion but very minor. Given the rule about not making obvious or unimportant comments, and considering this doesn't affect functionality or represent a significant code quality issue, I should lean toward deleting it. The change is too trivial to warrant a review comment.

2. server/pkg/database/manager/postgres/postgres_test.go:30

• Draft comment:
  Modifying a global environment variable in tests can lead to side effects. Consider configuring the DB port via the test config to isolate state.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 35% vs. threshold = 50% The comment raises a valid concern about test isolation - setting environment variables globally can affect other tests running in parallel or sequentially. However, I need to consider: 1) Is this actually a problem in this specific context? The code only sets it if it's empty, so it's providing a default. 2) Is the alternative clear and actionable? The comment suggests "configuring the DB port via the test config" but I don't see evidence in the diff that the config object has a DB_PORT field - it only shows DBSchema. 3) This is in a BeforeAll block which runs once, and there's no AfterAll cleanup to unset it. 4) The comment is about code that was added in the diff (lines 29-31 are new), so it is about changes made. I might be missing context about whether the config.Config struct actually supports a DB port field. The comment assumes there's a better way via config, but I can't verify that from the diff alone. Also, setting a default environment variable in tests is a common pattern, and the code is defensive (only sets if empty). The concern about side effects might be theoretical rather than practical. While the config alternative isn't clearly visible in the diff, the comment does raise a legitimate code quality concern about test isolation. Setting global environment variables is generally considered a code smell in tests. However, without seeing how the database connection actually uses this environment variable or whether config supports it, the comment may not be actionable. The rules state I should only keep comments that are "actionable and clear" - this one suggests an alternative but doesn't provide enough detail on how to implement it. This comment raises a valid concern about test isolation and global state, but it's not sufficiently actionable because it doesn't clearly show how to configure DB_PORT via the config object. The suggestion is somewhat speculative without more context about the codebase. Given the rule to only keep comments with strong evidence of correctness and clear actionability, this should be deleted.

Workflow ID: wflow_aT74KAwj9tGhOQQp

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 87a8e47 in 1 minute and 26 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. server/internal/api/controller/statement_controller_test.go:50

• Draft comment:
  Updated condition now checks for both 'processing' and 'pending' states. Consider adding a brief comment explaining that these statuses indicate that the statement is still in progress.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is suggesting adding an explanatory comment to clarify the logic. However, the function name "waitForStatementDone" and the condition itself are already quite self-explanatory - it breaks when status is no longer "processing" or "pending", meaning the statement is done. The logic is straightforward and doesn't require additional explanation. This falls into the category of an "obvious" comment that the rules say we should not make. The code is clear enough without the suggested inline comment. The function name and logic are already clear, but perhaps in a complex codebase, explicit comments about state transitions could be helpful for maintainability. The comment is actionable and specific, which are positive qualities. While the comment is actionable, it's suggesting documentation for code that is already self-documenting. The function name "waitForStatementDone" combined with the condition checking for "processing" and "pending" makes the intent obvious. Adding the suggested comment would be redundant and falls under "obvious or unimportant" comments that should be avoided. This comment should be deleted. It suggests adding an explanatory comment to code that is already self-explanatory through its function name and clear logic. This is an obvious, unimportant suggestion that doesn't add meaningful value.

Workflow ID: wflow_HSP2NAfucQS2Ang2

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 23a5c25 in 34 seconds. Click for details.

• Reviewed 21 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. server/pkg/database/manager/postgres/factory_test.go:47

• Draft comment:
  Good practice preserving/restoring DB_PORT to avoid side effects. Consider using t.Cleanup (if using testing.T) to simplify cleanup, and note that tests modifying env vars may interfere in parallel runs.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 1% None

Workflow ID: wflow_5nvUR8c6kZMOKVzj

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[codecov]

Codecov Report

❌ Patch coverage is 56.45161% with 27 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
server/internal/parser/custom_parser.go	54.05%	13 Missing and 4 partials ⚠️
server/internal/service/statement_service.go	52.94%	4 Missing and 4 partials ⚠️
server/internal/errors/statement.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!