uppercase react lazy components and remove their react lazy tests#273
uppercase react lazy components and remove their react lazy tests#273dimaMachina wants to merge 1 commit intomainfrom
Conversation
|
![inkeep-internal-ci[bot]](https://avatars.githubusercontent.com/u/123133689?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
🟠 1) GraphPanel.tsx:293 / SourceEditor.tsx:103 SCREAMING_CASE naming breaks established codebase conventions
Issue: This PR introduces GRAPH_PANEL_REACT_LAZY_ONLY and SOURCE_EDITOR_REACT_LAZY_ONLY — SCREAMING_CASE function components that are entirely novel in this codebase.
Why: All ~75 React components in packages/app/src/components/ and packages/app/src/editor/ use PascalCase (e.g., GraphPanel, SourceEditor, DocumentBoundary, EditorActivityPool). SCREAMING_CASE in this codebase is reserved for constants (e.g., ACTIVITY_MOUNT_LIMIT, MAX_POOL, SYNC_TIMEOUT_MS) — never for React components or functions. The _REACT_LAZY_ONLY suffix convention also has no precedent anywhere in the codebase.
Fix: Consider alternatives that don't break naming conventions:
- Keep PascalCase with JSDoc: Add
/** @internal Only for React.lazy - do not import directly */ - Separate file pattern: Move to
GraphPanel.lazy.tsxthat re-exports - Suffix pattern: Use
GraphPanelLazy/SourceEditorLazy
Refs:
Inline Comments:
- 🟠 Major:
GraphPanel.tsx:293SCREAMING_CASE naming convention
🟡 Minor (1) 🟡
🟡 1) EditorActivityPool.test.ts:10-11 Stale documentation reference after test deletion
Issue: The file EditorActivityPool.test.ts at lines 10-11 states: "The bundle-split contract itself is pinned in EditorActivityPool.lazy.test.ts." This reference becomes stale after deleting the test file.
Why: Documentation drift creates confusion for future maintainers who see a reference to a non-existent file.
Fix: Update EditorActivityPool.test.ts lines 10-11 to remove or replace the reference to the deleted test file.
Refs:
Inline Comments:
- 🟡 Minor:
EditorActivityPool.lazy.test.ts:1Stale documentation reference
💭 Consider (1) 💭
💭 1) scope Test coverage trade-off is acceptable but should be documented
Issue: The deleted tests verified that React.lazy correctly defers module loading — a code-splitting boundary contract.
Why: While these tests were primarily verifying bundler behavior (that dynamic imports create separate chunks) rather than application logic, they provided automated verification. The SCREAMING_CASE naming is a developer-facing signal but has no enforcement mechanism. However, React.lazy + dynamic import() is a standard, well-tested pattern that bundlers handle correctly by design, and E2E tests would catch actual load failures.
Fix: No action required if team accepts this trade-off. If stricter verification is desired later, consider bundle-size smoke tests or an E2E test asserting Suspense fallback renders.
🚫 REQUEST CHANGES
Summary: The core code-splitting simplification is fine, but the SCREAMING_CASE naming convention for React components breaks established codebase patterns (all ~75 other components use PascalCase). Please either revert to PascalCase with JSDoc annotations, or use a different pattern that maintains convention consistency. Also, the stale documentation reference in EditorActivityPool.test.ts should be cleaned up.
Discarded (0)
No findings were discarded.
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
2 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-consistency |
2 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-tests |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
| Total | 6 | 2 | 1 | 0 | 3 | 0 | 0 |
| } | ||
|
|
||
| export function GraphPanel({ activeDocName }: { activeDocName: string }) { | ||
| export function GRAPH_PANEL_REACT_LAZY_ONLY({ activeDocName }: { activeDocName: string }) { |
There was a problem hiding this comment.
🟠 MAJOR: SCREAMING_CASE naming breaks codebase conventions
Issue: GRAPH_PANEL_REACT_LAZY_ONLY uses SCREAMING_CASE for a React component, which is unprecedented in this codebase.
Why: All ~75 React components in packages/app/src/components/ and packages/app/src/editor/ use PascalCase (e.g., GraphPanel, DocumentBoundary, EditorActivityPool). SCREAMING_CASE is reserved for constants like ACTIVITY_MOUNT_LIMIT, MAX_POOL, SYNC_TIMEOUT_MS — never for React components.
Fix: Consider alternatives that don't break naming conventions:
- Keep PascalCase and add JSDoc:
/** @internal Only for React.lazy - do not import directly */ - Move to a separate file pattern like
GraphPanel.lazy.tsx - Use a
_Lazysuffix:GraphPanelLazy
Refs:
| @@ -1,23 +0,0 @@ | |||
| import { describe, expect, mock, test } from 'bun:test'; | |||
There was a problem hiding this comment.
🟡 Minor: Deleting this test leaves a stale documentation reference
Issue: The file EditorActivityPool.test.ts at lines 10-11 states: "The bundle-split contract itself is pinned in EditorActivityPool.lazy.test.ts." This reference will become stale after this deletion.
Why: Documentation drift creates confusion for future maintainers who see a reference to a non-existent file.
Fix: Update EditorActivityPool.test.ts lines 10-11 to remove or replace the reference to the deleted test file.
Refs:
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
1 participant
No description provided.