Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
WalkthroughAdds a new "Get File Text" action to extract text via Box Representations API, updates multiple Box action/source version fields and package version, and enhances the Box app with a Changes
Sequence DiagramsequenceDiagram
participant User as User/Workflow
participant Action as Get File Text Action
participant BoxApp as Box App
participant BoxAPI as Box API
User->>Action: Invoke with fileId
Action->>BoxApp: getFile(fileId)
BoxApp->>BoxAPI: GET /files/{fileId}?fields=representations
BoxAPI-->>BoxApp: File metadata (representations)
BoxApp-->>Action: representations array
Action->>Action: Find representation with entry.content.url_template
alt text representation found
Action->>BoxApp: _makeRequest(url)
BoxApp->>BoxAPI: GET extracted text URL
BoxAPI-->>BoxApp: Text content
BoxApp-->>Action: Text content
Action-->>User: Return text content
else no text representation
Action-->>User: Throw ConfigurationError (no text representation)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/box/actions/get-file-text/get-file-text.mjs`:
- Around line 60-64: Replace the fragile URL slicing logic that builds the
request URL from urlTemplate; instead locate the "{+asset_path}" placeholder in
urlTemplate and substitute it with the asset_path variable (preserving path
slashes as Box expects for {+asset_path}), then pass that resulting URL to
this.app._makeRequest; update the code around urlTemplate, asset_path and the
this.app._makeRequest call in the get-file-text flow to perform a direct
placeholder replacement rather than using urlTemplate.slice(...).
- Around line 40-57: The current getFile call catches all errors and always
throws ConfigurationError("File not found") and also assumes destructuring
always yields entries and a usable url_template; update the error handling
around this.app.getFile to rethrow or wrap the original error when it's not a
404/NotFound, and defensively check that the response has representations and
entries before using entries.find; inspect each representation entry for a
successful status (e.g., check entry.representation?.status or equivalent) and
only consider entries whose status is "success"; when deriving urlTemplate from
entries (the variable currently computed by
entries.find(...).content.url_template), validate existence and then construct
the download URL by replacing the {+asset_path} placeholder per Box docs instead
of slicing the string; use ConfigurationError only for
configuration/representation-missing conditions and preserve or include original
error info for API failures from this.app.getFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a6535be-4caf-40a7-89c9-bddb600e92fd
📒 Files selected for processing (12)
components/box/actions/create-sign-request/create-sign-request.mjscomponents/box/actions/download-file/download-file.mjscomponents/box/actions/get-comments/get-comments.mjscomponents/box/actions/get-file-text/get-file-text.mjscomponents/box/actions/search-content/search-content.mjscomponents/box/actions/upload-file-version/upload-file-version.mjscomponents/box/actions/upload-file/upload-file.mjscomponents/box/box.app.mjscomponents/box/package.jsoncomponents/box/sources/new-event/new-event.mjscomponents/box/sources/new-file/new-file.mjscomponents/box/sources/new-folder/new-folder.mjs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/box/actions/get-file-text/get-file-text.mjs (1)
39-57:⚠️ Potential issue | 🟠 MajorNarrow error mapping and validate extracted_text readiness before download.
Line 51 maps every
getFilefailure to “File not found”, which hides auth/rate-limit/transient API errors. Also, Line 55 should not select bycontent.url_templatealone—filter forrepresentation === "extracted_text"and gate onstatus.state(e.g.,success/viewable) to return a clear “not ready yet” message.🔧 Proposed fix
- let entries; + let entries = []; try { - ({ representations: { entries } } = await this.app.getFile({ + const file = await this.app.getFile({ $, fileId: this.fileId, params: { fields: "representations", }, headers: { "x-rep-hints": "[extracted_text]", }, - })); + }); + entries = file?.representations?.entries ?? []; } catch (error) { - throw new ConfigurationError(`File not found: ${this.fileId}`); + if (error?.response?.status === 404) { + throw new ConfigurationError(`File not found: ${this.fileId}`); + } + throw error; } - const urlTemplate = entries.find((entry) => entry?.content?.url_template)?.content.url_template; - if (!urlTemplate) { - throw new ConfigurationError("File does not have a text representation"); + const extractedText = entries.find((entry) => entry?.representation === "extracted_text"); + const state = extractedText?.status?.state; + const urlTemplate = extractedText?.content?.url_template; + if (!urlTemplate || !["success", "viewable"].includes(state)) { + throw new ConfigurationError( + state && !["success", "viewable"].includes(state) + ? `Text representation is ${state}. Try again later.` + : "File does not have a text representation", + ); }In Box Representations API, which `status.state` values indicate an extracted_text representation is downloadable, and should `content.url_template` be treated as opaque with only `{+asset_path}` substitution?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/box/actions/get-file-text/get-file-text.mjs` around lines 39 - 57, The catch block currently maps all errors from this.app.getFile to a generic ConfigurationError("File not found: this.fileId"); change it to rethrow non-404 errors (inspect error.status or error.response?.status) and only convert 404/NotFound into ConfigurationError with this.fileId; after extracting entries from the response, replace the urlTemplate lookup with logic that finds entries.find(e => e.representation === "extracted_text") and then check its status.state is one of the downloadable states (e.g., "success" or "viewable") before reading e.content.url_template; if state is not downloadable, throw a ConfigurationError("Extracted text not ready for file: this.fileId") and treat content.url_template as opaque except for replacing the documented {+asset_path} variable when constructing the final download URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/box/actions/get-file-text/get-file-text.mjs`:
- Around line 39-57: The catch block currently maps all errors from
this.app.getFile to a generic ConfigurationError("File not found: this.fileId");
change it to rethrow non-404 errors (inspect error.status or
error.response?.status) and only convert 404/NotFound into ConfigurationError
with this.fileId; after extracting entries from the response, replace the
urlTemplate lookup with logic that finds entries.find(e => e.representation ===
"extracted_text") and then check its status.state is one of the downloadable
states (e.g., "success" or "viewable") before reading e.content.url_template; if
state is not downloadable, throw a ConfigurationError("Extracted text not ready
for file: this.fileId") and treat content.url_template as opaque except for
replacing the documented {+asset_path} variable when constructing the final
download URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6fc0042-9078-42ca-b16f-f0c9cd0b4568
📒 Files selected for processing (1)
components/box/actions/get-file-text/get-file-text.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #20469
Summary by CodeRabbit
New Features
Improvements
Updates