Conversation
|
Warning Rate limit exceeded@codycooperross has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughUpdated Next.js config (public source maps, React strict mode enabled, swcMinify commented out). Disabled or removed several UI pieces: RelatedAggregateGraph on DOI page, resource-types & licenses charts in WorksDashboard, Sankey graph in WorksListing, and resourceType badge rendering in WorkMetadata while switching language badge to a forwarded/render-prop Badge. Changes
Sequence Diagram(s)No sequence diagram — changes are configuration updates and removal/replacement of UI components without new control flow to illustrate. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
akita
|
||||||||||||||||||||||||||||
| Project |
akita
|
| Branch Review |
debug-client-issue
|
| Run status |
|
| Run duration | 01m 41s |
| Commit |
|
| Committer | codycooperross |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
3
|
Skipped
|
0
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 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). (5)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (3)
- GitHub Check: test / cypress (5)
| // Use the hidden-source-map option when you don't want the source maps to be | ||
| // publicly available on the servers, only to the error reporting | ||
| const withSourceMaps = require('@zeit/next-source-maps')({ | ||
| devtool: 'hidden-source-map' | ||
| devtool: 'source-map' | ||
| }) |
There was a problem hiding this comment.
Exposing source maps publicly is a security risk in production.
Switching from hidden-source-map to source-map makes source maps publicly accessible, exposing your source code to end users. This contradicts the comment above (lines 1-2) and creates a security vulnerability in production environments.
Additionally, the comment is now inconsistent with the implementation.
Consider using environment-based configuration to enable full source maps only in development:
-// Use the hidden-source-map option when you don't want the source maps to be
-// publicly available on the servers, only to the error reporting
+// Use source-map in development for debugging, hidden-source-map in production
+// to keep source maps private and only available to error reporting
const withSourceMaps = require('@zeit/next-source-maps')({
- devtool: 'source-map'
+ devtool: process.env.NODE_ENV === 'development' ? 'source-map' : 'hidden-source-map'
})If this change is temporary for debugging a specific issue, consider reverting it before merging to the target branch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use the hidden-source-map option when you don't want the source maps to be | |
| // publicly available on the servers, only to the error reporting | |
| const withSourceMaps = require('@zeit/next-source-maps')({ | |
| devtool: 'hidden-source-map' | |
| devtool: 'source-map' | |
| }) | |
| // Use source-map in development for debugging, hidden-source-map in production | |
| // to keep source maps private and only available to error reporting | |
| const withSourceMaps = require('@zeit/next-source-maps')({ | |
| devtool: process.env.NODE_ENV === 'development' ? 'source-map' : 'hidden-source-map' | |
| }) |
🤖 Prompt for AI Agents
In next.config.js around lines 1 to 5 the devtool was set to 'source-map', which
exposes source maps publicly and contradicts the file comment; change the
configuration to use 'hidden-source-map' (or conditionally use 'source-map' only
when NODE_ENV !== 'production') and update the comment to match; implement an
environment check so production uses hidden-source-map while development uses
full source-map, and ensure the final comment accurately reflects the chosen
behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/WorksDashboard/WorksDashboard.tsx (1)
40-42: Remove unused data processing for commented-out charts.Lines 40-42 compute
resourceTypesandlicensesdata that is never used because the charts consuming this data are commented out (lines 57-74).If the Resource Types and Licenses charts are permanently removed:
- Remove the data processing at lines 40-42
- Remove the commented chart render blocks at lines 57-74
- Update the
ShowChartstype to remove these propertiesIf this is a temporary change for debugging, consolidate the explanation and ensure the data processing is also commented to avoid unnecessary computation on every render.
Also applies to: 57-74
🧹 Nitpick comments (4)
src/components/WorksListing/WorksListing.tsx (2)
16-16: Clarify intent and clean up if this is a permanent removal.Similar to the DOI page, this change removes the SankeyGraph visualization, which appears unrelated to the PR objective of displaying source maps for debugging.
If this is a permanent removal, the component interface should be cleaned up to remove unused props:
showSankey,sankeyTitle, andconnectionTypesCounts(lines 21-23) are now unused.If this is temporary, add a clear TODO comment.
40-40: Remove dead code and empty conditional blocks.The commented-out
sankeyTitledefault parameter (line 40) andsankeyDatacomputation (line 53) serve no purpose in the commented state.Additionally, lines 77-81 create an empty conditional block that renders a
<Col>with no content whenshowSankeyis true. If the Sankey graph is permanently removed:
- Remove the entire conditional block (lines 77-81)
- Remove the commented parameter and data computation
- Update the Props interface to remove
showSankey,sankeyTitle, andconnectionTypesCountsIf temporary, consolidate explanations into a single clear comment block at the top of the file.
Also applies to: 53-53, 77-81
src/components/WorksDashboard/WorksDashboard.tsx (1)
9-10: Address commented imports and update type definitions.The HorizontalStackedBarChart and color palette imports are commented out, consistent with removing Resource Types and Licenses charts below. However, this appears unrelated to the PR's stated purpose of displaying source maps for debugging.
If this removal is permanent:
- Remove the commented imports entirely
- Update the
ShowChartstype (lines 15-23) to removeresourceTypesandlicensesproperties- Clean up unused data processing (see next comment)
If temporary, add a clear explanation for why these charts are disabled.
src/app/(main)/doi.org/[...doi]/page.tsx (1)
13-13: Address commented code and clarify PR scope.The RelatedAggregateGraph component is still actively used in the codebase (specifically in
src/app/(embed)/embed/related-aggregate/[...doi]/page.tsx), so the component file and its dependencies must be retained. However, the commented import and component usage at lines 13 and 97-99 should be cleaned up.Either:
- Remove the commented lines entirely if this change is permanent
- Add a TODO comment explaining the temporary removal (e.g.,
// TODO: Disabled while implementing source maps debugging - restore after #XYZ)Additionally, clarify how this removal relates to the PR title "Display source maps for debugging"—the changes appear to affect component rendering rather than source map configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/(main)/doi.org/[...doi]/page.tsx(2 hunks)src/components/WorksDashboard/WorksDashboard.tsx(3 hunks)src/components/WorksListing/WorksListing.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). (1)
- GitHub Check: test / cypress (2)
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/WorksDashboard/WorksDashboard.tsx (2)
57-74: Remove block-commented JSX or restore functionality.Large block comments containing JSX are a code smell and hinder maintainability. If the resourceTypes and licenses charts are being permanently removed, delete this entire block. If they're temporarily disabled, use conditional rendering with feature flags.
For permanent removal:
- {/* {(show.resourceTypes || show.all) && <Col xs={12} sm={4}> - <HorizontalStackedBarChart - chartTitle={'Work Types'} - topCategory={{ title: resourceTypes.topCategory, percent: resourceTypes.topPercent }} - data={resourceTypes.data} - domain={resourceTypeDomain} - range={resourceTypeRange} - tooltipText={tooltipText('resourceType')} /> - </Col>} - {(show.licenses || show.all) && <Col xs={12} sm={4}> - <HorizontalStackedBarChart - chartTitle='Licenses' - topCategory={{ title: licenses.topCategory, percent: licenses.topPercent }} - data={licenses.data} - domain={[...otherDomain, ...licenses.data.map(l => l.title)]} - range={[...otherRange, ...licenseRange]} - tooltipText={'The field "rights" from DOI metadata was used to generate this chart, showing the % of licenses used across works.'} /> - </Col>} */}For temporary disabling via feature flag:
- {/* {(show.resourceTypes || show.all) && <Col xs={12} sm={4}> + {ENABLE_RESOURCE_CHARTS && (show.resourceTypes || show.all) && <Col xs={12} sm={4}> <HorizontalStackedBarChart chartTitle={'Work Types'} topCategory={{ title: resourceTypes.topCategory, percent: resourceTypes.topPercent }} data={resourceTypes.data} domain={resourceTypeDomain} range={resourceTypeRange} tooltipText={tooltipText('resourceType')} /> - </Col>} + </Col>} + {ENABLE_RESOURCE_CHARTS && (show.licenses || show.all) && <Col xs={12} sm={4}> - {(show.licenses || show.all) && <Col xs={12} sm={4}> <HorizontalStackedBarChart chartTitle='Licenses' topCategory={{ title: licenses.topCategory, percent: licenses.topPercent }} data={licenses.data} domain={[...otherDomain, ...licenses.data.map(l => l.title)]} range={[...otherRange, ...licenseRange]} tooltipText={'The field "rights" from DOI metadata was used to generate this chart, showing the % of licenses used across works.'} /> - </Col>} */} + </Col>}
15-23: Update ShowCharts type to reflect disabled features.The
ShowChartstype still includesresourceTypes?: booleanandlicenses?: boolean, but these properties are no longer functional since the corresponding charts are commented out. This creates a misleading API surface where consumers can pass these props but they have no effect.If the charts are permanently removed, update the type:
export type ShowCharts = { publicationYear?: boolean - resourceTypes?: boolean - licenses?: boolean creatorsAndContributors?: boolean fieldsOfScience?: boolean languages?: boolean all?: boolean }If they're temporarily disabled, add JSDoc comments to warn consumers:
export type ShowCharts = { publicationYear?: boolean /** @deprecated Temporarily disabled for debugging */ resourceTypes?: boolean /** @deprecated Temporarily disabled for debugging */ licenses?: boolean creatorsAndContributors?: boolean fieldsOfScience?: boolean languages?: boolean all?: boolean }src/components/WorkMetadata/WorkMetadata.tsx (1)
23-30: Remove non-functional showClaimStatus prop and update all call sites.The
showClaimStatusprop is defined in Props (line 26) but commented out in destructuring (line 35) and never used. Two components actively pass this prop expecting functionality:
- src/components/WorksListing/WorksListing.tsx:85
- src/components/Work/Work.tsx:20
To resolve this breaking change, remove the prop from the Props interface and update both call sites:
type Props = { metadata: Work linkToExternal?: boolean - showClaimStatus?: boolean hideTitle?: boolean hideMetadataInTable?: boolean includeMetricsDisplay?: boolean }Update src/components/WorksListing/WorksListing.tsx:85:
- <WorkMetadata metadata={doi} linkToExternal={false} showClaimStatus={showClaimStatus} /> + <WorkMetadata metadata={doi} linkToExternal={false} />Update src/components/Work/Work.tsx:20:
- <WorkMetadata metadata={doi} linkToExternal={true} showClaimStatus={false} hideMetadataInTable hideTitle /> + <WorkMetadata metadata={doi} linkToExternal={true} hideMetadataInTable hideTitle />Also uncomment the destructuring parameter at line 35 if the feature is being restored:
export default function WorkMetadata({ metadata, linkToExternal, - // showClaimStatus, + showClaimStatus, hideTitle = false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/WorkMetadata/WorkMetadata.tsx(6 hunks)src/components/WorksDashboard/WorksDashboard.tsx(5 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). (5)
- GitHub Check: test / cypress (2)
- GitHub Check: test / cypress (4)
- GitHub Check: test / cypress (5)
- GitHub Check: test / cypress (1)
- GitHub Check: test / cypress (3)
1 participant
Purpose
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
Chores
UI Changes