feat(metadata): add confidence score support to metadata read flow#4481
Merged
mergify[bot] merged 2 commits intobox:masterfrom Mar 26, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThreads a new isConfidenceScoreEnabled flag through metadata fetch and instance-creation flows, adds utilities to detect/unwrap "detailed" field values and extract confidence/target-location data, extends metadata Flow types, forwards a review-filter toggle through redesigned sidebar components, and updates tests and a package dependency. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant SidebarHook as useSidebarMetadataFetcher
participant API as Metadata API
participant Utils as Field Utilities
participant Instance as Template Instance Builder
UI->>SidebarHook: request metadata (fileId, isMetadataRedesign, isConfidenceScoreEnabled=true)
SidebarHook->>API: getMetadata(fileId, isMetadataRedesign, isConfidenceScoreEnabled)
API->>API: choose view param ("detailed" when redesign + enabled)
API->>Instance: createTemplateInstance(rawInstance, template, canEdit, isConfidenceScoreEnabled)
Instance->>Utils: isDetailedFieldValue(rawField)
Utils-->>Instance: boolean
alt detailed field
Instance->>Utils: extractDetailedFieldValue(rawField)
Utils-->>Instance: unwrappedValue
Instance->>Utils: mapDetailedFieldToConfidenceScore(rawField)
Utils-->>Instance: confidenceScore or undefined
Instance->>Utils: parseTargetLocation(rawField)
Utils-->>Instance: targetLocation or undefined
end
Instance-->>API: enriched template instance
API-->>SidebarHook: template instances (with optional confidence/locations)
SidebarHook-->>UI: render instances (UI may toggle review filter)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
|
Lindar90
commented
Mar 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/Metadata.js (1)
669-730:⚠️ Potential issue | 🟠 MajorKey the metadata cache by the requested view.
The new flag changes the payload shape, but
getMetadata()still reuses the samemetadata_${id}entry. On the sameMetadatainstance, a first call withisConfidenceScoreEnabled=falsewill cache hydrated data; a later call with it enabled returns that stale entry and never reachesview=detailed, so the confidence metadata never shows up until callers force-refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/Metadata.js` around lines 669 - 730, getMetadata is caching results under a single key from getMetadataCacheKey(id) but the payload shape varies with the requested view flags (isConfidenceScoreEnabled and/or isMetadataRedesign), causing stale responses; fix by making the cache key incorporate the requested view flags (e.g., extend getMetadataCacheKey to accept isConfidenceScoreEnabled and isMetadataRedesign or build a composite key in getMetadata like `metadata_${id}_detailed_${isConfidenceScoreEnabled}`) and use that key for cache.has/get/unset/successHandler so calls with different views store and retrieve separate entries (update all references in getMetadata to use the new key API).
🧹 Nitpick comments (3)
src/elements/content-sidebar/MetadataSidebarRedesign.tsx (1)
328-330: Consider memoizing the toggle callback.The inline arrow function
() => setShouldShowOnlyReviewFields(!shouldShowOnlyReviewFields)creates a new function reference on each render. While the impact is limited sinceMetadataInstanceEditoronly renders wheneditingTemplateis truthy, consider usinguseCallbackfor consistency with other handlers in this component.♻️ Optional: Memoize the toggle callback
+ const handleToggleReviewFilter = useCallback(() => { + setShouldShowOnlyReviewFields(prev => !prev); + }, []); + // ... in the JSX: - onToggleReviewFilter={() => setShouldShowOnlyReviewFields(!shouldShowOnlyReviewFields)} + onToggleReviewFilter={handleToggleReviewFilter}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx` around lines 328 - 330, The inline toggle passed to MetadataInstanceEditor creates a new function each render; replace it with a memoized callback using React.useCallback that captures setShouldShowOnlyReviewFields and shouldShowOnlyReviewFields (or use functional update to avoid reading stale state). Specifically, define a const onToggleReviewFilter = useCallback(() => setShouldShowOnlyReviewFields(v => !v), [setShouldShowOnlyReviewFields]) and pass that to the onToggleReviewFilter prop where MetadataInstanceEditor is rendered to match the style of other handlers in this component.src/common/types/metadata.js (1)
166-179: Make the newdetailsshape sparse as well.
src/api/utils.jsalready handles partialdetails, and the new metadata tests exercise payloads that only carry confidence / location data. KeepingupdatedAt,updatedBy, andupdatedAppIdrequired makes this type stricter than the runtime contract and will push future consumers toward unsafe casts.♻️ Suggested tweak
type MetadataDetailedFieldDetails = { - updatedAt: number, - updatedBy: string, - updatedAppId: string, + updatedAt?: number, + updatedBy?: string, + updatedAppId?: string, confidenceScore?: number, confidenceLevel?: string, process?: string, targetLocation?: string, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/types/metadata.js` around lines 166 - 179, The type MetadataDetailedFieldDetails is too strict—make its core fields optional so the shape is sparse like runtime usage; change updatedAt, updatedBy, and updatedAppId to optional properties on MetadataDetailedFieldDetails so partial detail objects (e.g., only confidenceScore or targetLocation) are valid; keep MetadataDetailedFieldValue.details as the optional container it already is and update the type definition for MetadataDetailedFieldDetails accordingly.src/api/Metadata.js (1)
545-550: The new flag pushes these APIs past the “too many booleans” point.
createTemplateInstance(),getTemplateInstances(), andgetMetadata()now all rely on multiple positional booleans. The tests already have to assert raw argument indexes, which is a good sign these should move to an options bag before another flag lands.Also applies to: 620-627, 669-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/Metadata.js` around lines 545 - 550, The APIs createTemplateInstance, getTemplateInstances, and getMetadata use multiple positional boolean flags—replace those boolean parameters with a single options object (options bag) to improve readability and future extensibility: update the signatures (e.g., createTemplateInstance(instance, template, options = {})) to accept named properties like { canEdit, isExternallyOwned, isConfidenceScoreEnabled } (and analogous properties for getTemplateInstances and getMetadata), update all callsites/tests to pass an object instead of positional booleans, and adjust any internal usage to read from options.<END_OF_INSTRUCTION>
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/api/Metadata.js`:
- Around line 669-730: getMetadata is caching results under a single key from
getMetadataCacheKey(id) but the payload shape varies with the requested view
flags (isConfidenceScoreEnabled and/or isMetadataRedesign), causing stale
responses; fix by making the cache key incorporate the requested view flags
(e.g., extend getMetadataCacheKey to accept isConfidenceScoreEnabled and
isMetadataRedesign or build a composite key in getMetadata like
`metadata_${id}_detailed_${isConfidenceScoreEnabled}`) and use that key for
cache.has/get/unset/successHandler so calls with different views store and
retrieve separate entries (update all references in getMetadata to use the new
key API).
---
Nitpick comments:
In `@src/api/Metadata.js`:
- Around line 545-550: The APIs createTemplateInstance, getTemplateInstances,
and getMetadata use multiple positional boolean flags—replace those boolean
parameters with a single options object (options bag) to improve readability and
future extensibility: update the signatures (e.g.,
createTemplateInstance(instance, template, options = {})) to accept named
properties like { canEdit, isExternallyOwned, isConfidenceScoreEnabled } (and
analogous properties for getTemplateInstances and getMetadata), update all
callsites/tests to pass an object instead of positional booleans, and adjust any
internal usage to read from options.<END_OF_INSTRUCTION>
In `@src/common/types/metadata.js`:
- Around line 166-179: The type MetadataDetailedFieldDetails is too strict—make
its core fields optional so the shape is sparse like runtime usage; change
updatedAt, updatedBy, and updatedAppId to optional properties on
MetadataDetailedFieldDetails so partial detail objects (e.g., only
confidenceScore or targetLocation) are valid; keep
MetadataDetailedFieldValue.details as the optional container it already is and
update the type definition for MetadataDetailedFieldDetails accordingly.
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Around line 328-330: The inline toggle passed to MetadataInstanceEditor
creates a new function each render; replace it with a memoized callback using
React.useCallback that captures setShouldShowOnlyReviewFields and
shouldShowOnlyReviewFields (or use functional update to avoid reading stale
state). Specifically, define a const onToggleReviewFilter = useCallback(() =>
setShouldShowOnlyReviewFields(v => !v), [setShouldShowOnlyReviewFields]) and
pass that to the onToggleReviewFilter prop where MetadataInstanceEditor is
rendered to match the style of other handlers in this component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c355dc3-ddc6-4abe-b5ac-0741b73a0345
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/api/Metadata.jssrc/api/__tests__/Metadata.test.jssrc/api/__tests__/utils.test.jssrc/api/utils.jssrc/common/types/metadata.jssrc/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/MetadataInstanceEditor.test.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsxsrc/elements/content-sidebar/__tests__/useSidebarMetadataFetcher.test.tsxsrc/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/__tests__/utils.test.js (1)
176-219: Add a non-array JSON test forparseTargetLocation.To lock in parser contract safety, add a case where
targetLocationis valid JSON but not an array (expectundefined).✅ Test addition
test('should return undefined when targetLocation is invalid JSON', () => { const fieldValue = { values: 'California', details: { targetLocation: 'not valid json {{{' }, }; expect(parseTargetLocation(fieldValue)).toBeUndefined(); }); + test('should return undefined when targetLocation is valid JSON but not an array', () => { + const fieldValue = { + values: 'California', + details: { targetLocation: '{"itemId":"123"}' }, + }; + expect(parseTargetLocation(fieldValue)).toBeUndefined(); + }); + test('should return undefined for null fieldValue', () => { expect(parseTargetLocation(null)).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/__tests__/utils.test.js` around lines 176 - 219, Add a new unit test in the parseTargetLocation() describe block that passes a detailed fieldValue whose details.targetLocation is valid JSON but not an array (e.g. a JSON object string like '{"itemId":"123"}') and assert parseTargetLocation(fieldValue) returns undefined; update the test suite in the same file near the other parseTargetLocation tests and name it something like "should return undefined when targetLocation JSON is not an array" so it clearly covers the non-array JSON case for the parseTargetLocation function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/utils.js`:
- Around line 76-88: The parseTargetLocation function currently returns whatever
JSON.parse(details.targetLocation) yields, which may be a non-array and violate
the declared return type; update parseTargetLocation to validate the parsed
value: after JSON.parse(details.targetLocation) ensure the result is an array
and (optionally) each entry matches the MetadataTargetLocationEntry shape (e.g.,
check required fields or types), and if validation fails return undefined; keep
the existing try/catch but add the array/shape guard before returning the parsed
value so callers only receive a proper Array<MetadataTargetLocationEntry> or
undefined.
---
Nitpick comments:
In `@src/api/__tests__/utils.test.js`:
- Around line 176-219: Add a new unit test in the parseTargetLocation() describe
block that passes a detailed fieldValue whose details.targetLocation is valid
JSON but not an array (e.g. a JSON object string like '{"itemId":"123"}') and
assert parseTargetLocation(fieldValue) returns undefined; update the test suite
in the same file near the other parseTargetLocation tests and name it something
like "should return undefined when targetLocation JSON is not an array" so it
clearly covers the non-array JSON case for the parseTargetLocation function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24c398a4-4551-4998-b48e-888b631f822d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
src/api/__tests__/Metadata.test.jssrc/api/__tests__/utils.test.jssrc/api/utils.jssrc/common/types/metadata.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/tests/Metadata.test.js
dlasecki-box
previously approved these changes
Mar 24, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/utils.js (1)
76-95: Consider validating array entry shapes for defensive parsing.The Array.isArray guard is good (addressing the previous review). However, individual entries are not validated against
MetadataTargetLocationEntry(required fields:itemId,page,text). Malformed API responses could cause runtime errors downstream.This is a minor concern since the API is presumably trusted, but for robustness you could add a shape check:
🛡️ Optional: Add entry shape validation
try { const parsed = JSON.parse(details.targetLocation); if (!Array.isArray(parsed)) { return undefined; } - return parsed; + // Validate each entry has required fields + const isValidEntry = (entry: any): boolean => + entry != null && + typeof entry === 'object' && + typeof entry.itemId === 'string' && + typeof entry.page === 'number' && + typeof entry.text === 'string'; + return parsed.every(isValidEntry) ? parsed : undefined; } catch { return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/utils.js` around lines 76 - 95, The parseTargetLocation function currently parses details.targetLocation into an array but doesn't validate each entry shape; update parseTargetLocation to iterate the parsed array and verify each element is an object containing the required MetadataTargetLocationEntry fields (itemId, page, text) with appropriate types (e.g., string/number as expected), and return undefined if any entry fails validation before returning the array; reference parseTargetLocation and the parsed variable to add this defensive shape check so downstream code cannot assume well-formed entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/utils.js`:
- Around line 76-95: The parseTargetLocation function currently parses
details.targetLocation into an array but doesn't validate each entry shape;
update parseTargetLocation to iterate the parsed array and verify each element
is an object containing the required MetadataTargetLocationEntry fields (itemId,
page, text) with appropriate types (e.g., string/number as expected), and return
undefined if any entry fails validation before returning the array; reference
parseTargetLocation and the parsed variable to add this defensive shape check so
downstream code cannot assume well-formed entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe61c523-1d03-4641-9bd3-7e59d58dd1ea
📒 Files selected for processing (3)
src/api/__tests__/utils.test.jssrc/api/utils.jssrc/common/types/metadata.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/tests/utils.test.js
tjuanitas
previously approved these changes
Mar 24, 2026
Contributor
reneshen0328
left a comment
reneshen0328
left a comment
There was a problem hiding this comment.
Hello! I have a few observations for future consideration (not blocking for this PR). Feel free to comment in the thread if my understanding is incorrect:
Observations on Architecture
- Feature flag proliferation: MetadataSidebarRedesign now has 7 feature flags, and Metadata.js is accumulating UI-specific parameters (isMetadataRedesign, isConfidenceScoreEnabled). The separation between "what the API layer does" vs "what the UI layer decides" is becoming blurred.
- Conditional coupling: isConfidenceScoreEnabled only has an effect when isMetadataRedesign is true (line 410 in Metadata.js), but it's passed through all code paths regardless. This creates implicit dependencies that aren't obvious from the function signatures.
- Prop drilling: The flag travels through 5 layers to reach where it's used. If more conditions are added, this pattern will compound.
Future Suggestion
Consider separating concerns more clearly:
// API layer: just fetch the right view (no UI flags)
getInstances(id, viewType: 'basic' | 'hydrated' | 'detailed')
// Transform layer: always extract all available data
// (extractDetailedFieldValue, mapDetailedFieldToConfidenceScore, parseTargetLocation
// are already safe for non-detailed data)
createTemplateInstance(instance, template, canEdit, isExternallyOwned)
// UI layer: decides what to display based on feature flags
<MetadataInstanceForm
showConfidenceScore={isConfidenceScoreEnabled}
/>
This would:
- Keep the API layer generic (view types, not feature flags)
- Always enrich data when available (safe operations)
- Let the UI layer control visibility
This is out of scope for this PR, but worth considering as the metadata feature set grows IMO.
dlasecki-box
previously approved these changes
Mar 25, 2026
- handle non array edge case in parseTargetLocation - make "boundingBox?: MetadataBoundingBox" optional
Lindar90
force-pushed
the
metadata-confidence-score-read-flow
branch
from
adda336 to
7795e29
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/elements/content-sidebar/MetadataSidebarRedesign.tsx`:
- Line 190: The boolean state shouldShowOnlyReviewFields is only reset in
handleCancel, causing it to persist into subsequent edit sessions; add
setShouldShowOnlyReviewFields(false) to every edit-exit and edit-entry path so
it never carries over. Specifically, add the reset call inside the save/submit
success and failure handlers (e.g., the functions around the diff at lines
328-330 and 343-347), and also reset it when entering edit mode (where the
editor is opened/toggled) so handleCancel, the save/submit handlers, and the
edit-entry handler all call setShouldShowOnlyReviewFields(false).
🪄 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: CHILL
Plan: Pro
Run ID: fbbe3c58-ed47-465f-b5ff-fd8d90169c87
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/api/Metadata.jssrc/api/__tests__/Metadata.test.jssrc/api/__tests__/utils.test.jssrc/api/utils.jssrc/common/types/metadata.jssrc/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/MetadataInstanceEditor.test.tsxsrc/elements/content-sidebar/__tests__/MetadataSidebarRedesign.test.tsxsrc/elements/content-sidebar/__tests__/useSidebarMetadataFetcher.test.tsxsrc/elements/content-sidebar/hooks/useSidebarMetadataFetcher.ts
✅ Files skipped from review due to trivial changes (3)
- package.json
- src/api/tests/utils.test.js
- src/elements/content-sidebar/tests/MetadataSidebarRedesign.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sidebar/tests/useSidebarMetadataFetcher.test.tsx
- src/elements/content-sidebar/MetadataInstanceEditor.tsx
dlasecki-box
approved these changes
Mar 26, 2026
tjuanitas
approved these changes
Mar 26, 2026
Contributor
Merge Queue Status
This pull request spent 11 minutes 33 seconds in the queue, including 11 minutes 16 seconds running CI. Required conditions to merge
|
This was referenced Mar 30, 2026
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
5 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.
When metadata.confidenceScore.enabled FF is enabled, the metadata sidebar now fetches instances using ?view=detailed, which returns per-field confidence score data including score value, confidence level, AI acceptance
status, and target location. Fields that were not AI-extracted gracefully return no confidence data.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores