Skip to content

Conversation

@rongquan1
Copy link
Contributor

@rongquan1 rongquan1 commented Oct 31, 2025

Summary by CodeRabbit

  • Refactor
    • Simplified storage API authentication: request headers now rely on a configured API key rather than per-document credentials.
    • Enforced required API key configuration and stronger CSRF token validation to surface missing credentials earlier.
    • Preserved existing error handling and cross-site request behavior to avoid regressions.

@coderabbitai
Copy link

coderabbitai bot commented Oct 31, 2025

Walkthrough

Three functions in src/common/API/storageAPI.tsx were updated: getHeaders now reads the API key from environment variable REACT_APP_API_KEY_DOCUMENT_STORAGE and accepts only an optional csrfToken; callers (fetchCsrfToken, getQueueNumber, uploadToStorage) were updated to use the new getHeaders signature and header behavior.

Changes

Cohort / File(s) Summary
Header construction & callers
src/common/API/storageAPI.tsx
Changed getHeaders signature to (csrfToken?); it now derives the API key from REACT_APP_API_KEY_DOCUMENT_STORAGE and throws if missing. Updated fetchCsrfToken to include getHeaders() headers in its axios request. Updated getQueueNumber and uploadToStorage to call getHeaders() with only csrfToken (or none). Removed use of documentStorage.apiKey.

Sequence Diagram(s)

sequenceDiagram
  participant UI as Caller (UI / Service)
  participant API as storageAPI.getHeaders
  participant Axios as axios

  Note over UI,API: New header flow — API key from env, optional csrfToken param

  UI->>API: getHeaders(optional csrfToken)
  API-->>UI: headers { Authorization: env-key, X-CSRF-Token? }
  UI->>Axios: axios request (headers returned, withCredentials as needed)
  Axios-->>UI: response (may include csrfToken for fetchCsrfToken flow)

  rect rgb(230,245,255)
    Note right of API: getHeaders no longer accepts documentStorage\nthrows if env var missing
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Inspect src/common/API/storageAPI.tsx for correct environment-variable usage and thrown error path.
  • Verify fetchCsrfToken still correctly handles missing response.data.csrfToken and that adding headers doesn't introduce unintended side effects.
  • Confirm no remaining callers pass documentStorage to getHeaders.

Poem

🐰 I nibbled headers, neat and small,
Trimmed the args to fit the wall,
Env-key shining, tokens kept true,
Requests hop light across the queue,
A rabbit's fix — quick, tidy, and new.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is entirely missing, with no content provided by the author. The required template specifies three sections—Summary (background), Changes (what was modified), and Issues (related issues)—none of which are present. Without a description, reviewers lack essential context about why this refactoring was necessary, what problems it solves, and which issues or stories it addresses. This represents a complete departure from the repository's documentation standards. Add a comprehensive pull request description following the repository template. Include a Summary explaining why the header construction logic needed refactoring (e.g., removing documentStorage dependency), a Changes section detailing the modifications to getHeaders(), fetchCsrfToken(), getQueueNumber(), and uploadToStorage(), and an Issues section linking any related GitHub issues or stories. This will provide reviewers with the context necessary to evaluate the refactoring thoroughly.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The pull request title "fix: update headers" is vague and generic, using non-descriptive language that fails to convey meaningful information about the changeset. While technically related to the modifications in storageAPI.tsx (which do involve header updates), the title doesn't clearly explain the substantial refactoring involved—specifically, the removal of documentStorage dependency from getHeaders(), the shift to deriving API keys from environment variables, and the signature changes affecting multiple functions. A reader scanning the commit history would not understand the actual scope or purpose of this change from the title alone. Consider revising the title to be more specific and descriptive of the main change. For example: "refactor: decouple header construction from documentStorage" or "fix: use environment variable for API key in headers" would better communicate the intent and primary modification to reviewers. The current title should be updated to reflect the key architectural change rather than using vague terminology like "update headers."
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/csrf

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

Comment @coderabbitai help to get the list of available commands and usage tips.

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a22a2e2 and df36599.

📒 Files selected for processing (1)
  • src/common/API/storageAPI.tsx (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Redirect rules - tradetrust-creator
  • GitHub Check: Header rules - tradetrust-creator
  • GitHub Check: Pages changed - tradetrust-creator
  • GitHub Check: Lint & Test

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

🧹 Nitpick comments (1)
src/common/API/storageAPI.tsx (1)

36-38: Consider validating the API key at application startup.

While throwing an error when the API key is missing is good defensive programming, it only catches the issue at runtime when getHeaders is called. Consider validating REACT_APP_API_KEY_DOCUMENT_STORAGE at application initialization for earlier error detection and better developer experience.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 858e04e and 6623138.

📒 Files selected for processing (1)
  • src/common/API/storageAPI.tsx (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Redirect rules - tradetrust-creator
  • GitHub Check: Header rules - tradetrust-creator
  • GitHub Check: Pages changed - tradetrust-creator
  • GitHub Check: Lint & Test
🔇 Additional comments (4)
src/common/API/storageAPI.tsx (4)

15-15: LGTM! Correctly calls getHeaders() without CSRF token.

The call to getHeaders() without parameters is correct here since this function is fetching the CSRF token itself. The previous issue about missing documentStorage parameter has been resolved by the refactor.


55-55: LGTM! Correctly omits CSRF token for GET request.

The call to getHeaders() without a CSRF token is appropriate for this GET request. CSRF protection is typically only required for state-changing operations. The previous issue about missing documentStorage parameter has been resolved by the refactor.


72-72: LGTM! Correctly includes CSRF token for POST request.

The call to getHeaders(csrfToken) is correct for this POST request. The CSRF token is properly included to protect against CSRF attacks on state-changing operations. The previous issue about incorrect parameter type has been resolved.


29-47: Verification complete—no issues found.

All callers of getHeaders within the codebase have been properly updated to match the new signature. The function is only used internally in storageAPI.tsx and is not exported, so the architectural change from per-storage API keys to a centralized environment variable has no impact on other files. Error handling for a missing API key is in place.

@rongquan1 rongquan1 requested a review from RishabhS7 October 31, 2025 07:39
@RishabhS7 RishabhS7 merged commit fee615f into master Oct 31, 2025
9 checks passed
@RishabhS7 RishabhS7 deleted the fix/csrf branch October 31, 2025 07:48
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.

3 participants