Cleanup and unify CodeMirror Demo pages #2400
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the management of example documents within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files📢 Thoughts on this report? Let us know! |
VasylMarchuk
force-pushed
the
code-mirror/demo-pages-cleanup
branch
from
a5112d7 to
2903d42
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/routes/demo/file-contents-card.ts (1)
3-3: Consider adding type annotations for query parametersWhile the parameter names suggest boolean values, it would be beneficial to explicitly define their types using a TypeScript type or interface. This ensures type safety when these parameters are used throughout the application.
-const QUERY_PARAMS = ['foldGutter', 'headerTooltipText', 'isCollapsed', 'isCollapsible', 'scrollIntoViewOnCollapse', 'selectedDocumentIndex']; +type FileContentsCardParam = 'foldGutter' | 'headerTooltipText' | 'isCollapsed' | 'isCollapsible' | 'scrollIntoViewOnCollapse' | 'selectedDocumentIndex'; +const QUERY_PARAMS: FileContentsCardParam[] = ['foldGutter', 'headerTooltipText', 'isCollapsed', 'isCollapsible', 'scrollIntoViewOnCollapse', 'selectedDocumentIndex'];app/helpers/diff-to-document.ts (1)
Line range hint
3-24: Enhance robustness and efficiency of the parseDiff functionWhile the function works for basic cases, consider these improvements:
- Add input validation
- Handle empty lines correctly
- Use explicit TypeScript types
- Optimize using array methods
Consider this enhanced implementation:
-export function parseDiff(diff: string = '') { +interface DiffResult { + current: string; + original: string; +} + +export function parseDiff(diff: string = ''): DiffResult { + if (typeof diff !== 'string') { + throw new TypeError('Input must be a string'); + } + const diffLines = diff.split('\n'); - const current = []; const original = []; for (const line of diffLines) { + // Handle empty lines without substring + if (!line) { + current.push(''); + original.push(''); + continue; + } + if (line.startsWith('-')) { original.push(line.substring(1)); } else if (line.startsWith('+')) { current.push(line.substring(1)); } else { - original.push(line.substring(1)); - current.push(line.substring(1)); + // Only substring if not an empty line + const content = line.startsWith(' ') ? line.substring(1) : line; + original.push(content); + current.push(content); } } return { current: current.join('\n'), original: original.join('\n'), }; }app/router.ts (1)
106-109: Consider adding documentation for the demo routes section.Since the demo routes section is being used for component demonstrations and is actively being reorganized, it would be helpful to add a brief comment explaining its purpose and organization principles.
this.route('demo', function () { + // Demo routes for showcasing and testing UI components + // Components are organized with basic components first, + // followed by utility components like dark-mode-toggle this.route('code-mirror'); this.route('file-contents-card'); this.route('dark-mode-toggle'); });app/controllers/demo/code-mirror.ts (1)
8-8: Good architectural improvement!Moving the example documents to a shared utility improves code organization and reusability across demo pages. This change aligns well with the PR objectives of unifying the Demo Route pages.
🛑 Comments failed to post (4)
app/controllers/demo/code-mirror.ts (1)
194-194: 💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered evidence, I can now provide the final response:
The non-null assertion is indeed problematic and should be fixed
The code shows that:
ExampleDocumenthas adocumentproperty that's tracked- The getter falls back to an empty document via
createEmpty()- The non-null assertion is used right after an optional chaining check, which is inconsistent
The suggested refactoring in the review is correct as it:
- Properly handles the empty document case
- Removes the unsafe non-null assertion
- Maintains the same functionality with better type safety
🔗 Analysis chain
Potential runtime safety issue with document updates
While using
createEmpty()as a fallback in the getter is good, thedocumentDidChangeaction uses a non-null assertion (!) which could be unsafe. IfselectedDocumentis the empty fallback instance, mutations might not be handled as expected.Let's verify if the empty document case is handled properly:
Consider a safer implementation:
@action documentDidChange(_target: Controller, newValue: string) { if (!this.document) { return; // Don't overwrite the document if it's not being passed! } - if (this.selectedDocument?.document !== newValue) { - this.selectedDocument!.document = newValue; - } + const doc = this.selectedDocument; + if (doc && doc.document !== newValue) { + doc.document = newValue; + } }Also applies to: 246-248
Let me gather more context about the document handling to verify this thoroughly.
Let me check one more aspect to understand the full document handling flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests or handling of empty/fallback document cases rg -A 5 "createEmpty|EmptyDocument" # Look for other non-null assertions on selectedDocument ast-grep --pattern 'selectedDocument!'Length of output: 1284
Script:
#!/bin/bash # Check the full implementation of documentDidChange and surrounding context rg -B 5 -A 10 "documentDidChange" app/controllers/demo/code-mirror.ts # Look for document property access patterns ast-grep --pattern 'selectedDocument?.document' # Check for any null checks on selectedDocument rg "if.*selectedDocument" app/controllers/demo/code-mirror.tsLength of output: 672
Script:
#!/bin/bash # Check the document property and its type ast-grep --pattern 'document: $_' # Look for the interface or type definition of ExampleDocument rg -A 10 "class ExampleDocument" app/utils/code-mirror-documents.ts # Check where documents array is defined and populated rg -B 5 "documents\s*=" app/controllers/demo/code-mirror.tsLength of output: 413
app/controllers/demo/file-contents-card.ts (1)
16-25:
⚠️ Potential issueEnsure Proper Type Casting for Query Parameters
The query parameters
foldGutter,headerTooltipText,isCollapsed,isCollapsible,scrollIntoViewOnCollapse, andselectedDocumentIndexare defined as booleans and a number. However, Ember treats query parameters as strings by default, which can lead to unexpected behavior when these values are assigned to typed properties.To ensure that the query parameters are correctly interpreted as booleans and numbers, specify the
typefor each query parameter.Apply this diff to define the query parameters with types:
export default class DemoFileContentsCardController extends Controller { - queryParams = ['foldGutter', 'headerTooltipText', 'isCollapsed', 'isCollapsible', 'scrollIntoViewOnCollapse', 'selectedDocumentIndex']; + queryParams = [ + { foldGutter: { type: 'boolean' } }, + { headerTooltipText: { type: 'boolean' } }, + { isCollapsed: { type: 'boolean' } }, + { isCollapsible: { type: 'boolean' } }, + { scrollIntoViewOnCollapse: { type: 'boolean' } }, + { selectedDocumentIndex: { type: 'number' } } + ];📝 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.queryParams = [ { foldGutter: { type: 'boolean' } }, { headerTooltipText: { type: 'boolean' } }, { isCollapsed: { type: 'boolean' } }, { isCollapsible: { type: 'boolean' } }, { scrollIntoViewOnCollapse: { type: 'boolean' } }, { selectedDocumentIndex: { type: 'number' } } ]; @tracked documents: ExampleDocument[] = EXAMPLE_DOCUMENTS; @tracked foldGutter: boolean = OPTION_DEFAULTS.foldGutter; @tracked headerTooltipText: boolean = OPTION_DEFAULTS.headerTooltipText; @tracked isCollapsed: boolean = OPTION_DEFAULTS.isCollapsed; @tracked isCollapsible: boolean = OPTION_DEFAULTS.isCollapsible; @tracked scrollIntoViewOnCollapse: boolean = OPTION_DEFAULTS.scrollIntoViewOnCollapse; @tracked selectedDocumentIndex: number = OPTION_DEFAULTS.selectedDocumentIndex;app/utils/code-mirror-documents.ts (2)
35-37:
⚠️ Potential issueEnsure 'diff' is required in
DiffBasedExampleDocumentconstructorThe
diffparameter is currently optional (diff?: string), but it's used immediately without a check forundefined. This could lead to runtime errors ifdiffis not provided. To prevent potential issues, makediffa required parameter.Apply this diff to make
diffrequired:export class DiffBasedExampleDocument extends ExampleDocument { - @tracked diff?: string; + @tracked diff: string; constructor({ diff, filename, language }: { diff?: string; filename: string; language: string }) { + // Make 'diff' a required parameter + constructor({ diff, filename, language }: { diff: string; filename: string; language: string }) { const { current: document, original: originalDocument } = parseDiff(diff);Committable suggestion skipped: line range outside the PR's diff.
28-28: 🛠️ Refactor suggestion
Avoid using 'this' in static method
createEmptyUsing
thisin a static context can be confusing becausethisrefers to the class itself. It's clearer and more maintainable to use the class name directly when instantiating a new object within a static method.Apply this diff to improve clarity:
static createEmpty() { - return new this({ filename: 'empty.txt', language: 'text' }); + return new ExampleDocument({ filename: 'empty.txt', language: 'text' }); }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
VasylMarchuk
force-pushed
the
code-mirror/demo-pages-cleanup
branch
from
2903d42 to
29d67c6
Compare
VasylMarchuk
changed the title
Bundle ReportChanges will decrease total bundle size by 4.87kB (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
…il `parse-diff-as-document`
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
app/utils/parse-diff-as-document.ts (2)
1-1: Add return type definition for better type safety.Consider adding an explicit return type interface to improve type safety and documentation.
+interface ParsedDocument { + current: string; + original: string; +} + -export default function parseDiffAsDocument(diff: string = '') { +export default function parseDiffAsDocument(diff: string = ''): ParsedDocument {
4-21: Consider using reduce for better performance and immutability.The current implementation using arrays and push operations could be refactored to use reduce for better performance and immutability.
- const current = []; - const original = []; - - for (const line of diffLines) { - if (line.startsWith('-')) { - original.push(line.substring(1)); - } else if (line.startsWith('+')) { - current.push(line.substring(1)); - } else { - original.push(line.substring(1)); - current.push(line.substring(1)); - } - } - - return { - current: current.join('\n'), - original: original.join('\n'), - }; + return diffLines.reduce((acc, line) => { + if (!line.trim()) return acc; + + const content = line.length > 1 ? line.substring(1) : ''; + + if (line.startsWith('-')) { + return { + ...acc, + original: [...acc.original, content] + }; + } else if (line.startsWith('+')) { + return { + ...acc, + current: [...acc.current, content] + }; + } else { + return { + current: [...acc.current, content], + original: [...acc.original, content] + }; + } + }, { current: [], original: [] } as { current: string[], original: string[] }) + .map(arrays => ({ + current: arrays.current.join('\n'), + original: arrays.original.join('\n') + }));tests/unit/utils/parse-diff-as-document-test.ts (2)
5-9: Consider expanding test coverage with additional scenarios.While the current test case is valid, consider adding more test cases to cover:
- Multiple consecutive additions/deletions
- Empty lines
- Special characters
- Larger diffs
Example additional test:
test('it handles multiple consecutive changes and special characters', async function (assert) { const result = parseDiffAsDocument( ' line1\n+new line 2\n+new line 3 with $pecial chars\n-old line 2\n-old line 3\n line4\n' ); assert.strictEqual( result.current, 'line1\nnew line 2\nnew line 3 with $pecial chars\nline4\n', 'current document handles consecutive additions correctly' ); assert.strictEqual( result.original, 'line1\nold line 2\nold line 3\nline4\n', 'original document handles consecutive deletions correctly' ); });
11-15: Enhance error handling test coverage.Consider adding more error handling test cases to verify behavior with:
nullinput- Empty string input
- Invalid diff format (missing space prefix, invalid markers)
Example additional tests:
test('it handles various invalid inputs gracefully', async function (assert) { assert.deepEqual( parseDiffAsDocument(null), { current: '', original: '' }, 'handles null input' ); assert.deepEqual( parseDiffAsDocument(''), { current: '', original: '' }, 'handles empty string input' ); assert.deepEqual( parseDiffAsDocument('invalid\ndiff\nformat'), { current: '', original: '' }, 'handles invalid diff format' ); });app/utils/code-mirror-documents.ts (3)
4-30: Consider type improvements and addressing test coverage.A few suggestions to improve the implementation:
- Consider extracting the constructor parameters into a named interface
- Add test coverage for the tracked properties and
createEmptymethod- Make the static method more explicit
+interface ExampleDocumentParams { + document?: string; + originalDocument?: string; + filename: string; + language: string; +} export class ExampleDocument { @tracked document: string = ''; @tracked originalDocument: string; @tracked filename: string; @tracked language: string; - constructor({ - document = '', - originalDocument, - filename, - language, - }: { - document?: string; - originalDocument?: string; - filename: string; - language: string; - }) + constructor(params: ExampleDocumentParams) static createEmpty() { - return new this({ filename: 'empty.txt', language: 'text' }); + return new ExampleDocument({ filename: 'empty.txt', language: 'text' }); } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 5-5: app/utils/code-mirror-documents.ts#L5
Added line #L5 was not covered by tests
[warning] 28-28: app/utils/code-mirror-documents.ts#L28
Added line #L28 was not covered by tests🪛 Biome
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
195-205: Consider removing redundant table definition in SQL example.The
customer_backuptable being removed is identical tocustomer_backup3. Consider using a more diverse example that shows different schema changes.
283-285: Address TODO comment in Dockerfile example.The TODO comment suggests an unresolved caching issue. Consider either:
- Investigating and resolving the caching issue
- Documenting the reason for keeping these lines commented out
- Removing these lines if they're no longer needed
Would you like me to help investigate the caching issue or create a GitHub issue to track this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/helpers/diff-to-document.ts(1 hunks)app/utils/code-mirror-documents.ts(1 hunks)app/utils/parse-diff-as-document.ts(1 hunks)tests/unit/utils/parse-diff-as-document-test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/helpers/diff-to-document.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/utils/code-mirror-documents.ts
[warning] 5-5: app/utils/code-mirror-documents.ts#L5
Added line #L5 was not covered by tests
[warning] 28-28: app/utils/code-mirror-documents.ts#L28
Added line #L28 was not covered by tests
🪛 Biome
app/utils/code-mirror-documents.ts
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (4)
tests/unit/utils/parse-diff-as-document-test.ts (2)
1-4: LGTM! Clean and well-structured test setup.
The imports and module declaration follow standard QUnit testing practices.
1-16: Verify consistent line ending handling.
Consider adding test cases to verify that the utility function handles and maintains line endings consistently across different platforms (CRLF vs LF).
app/utils/code-mirror-documents.ts (2)
32-47: LGTM! Clean inheritance implementation.
The DiffBasedExampleDocument class properly extends the base class and handles the diff parsing logic well.
115-117: Be cautious with invisible characters in example documents.
The example contains invisible characters (tabs, spaces, and Unicode characters) which might cause unexpected behavior when used in tests or demos.
Let's verify the impact of these characters:
✅ Verification successful
Invisible characters in example documents are safe to use
The invisible characters are only used in the example documents file itself and are not referenced or used directly in any tests or other parts of the codebase. While the examples are imported in two controllers (file-contents-card.ts and code-mirror.ts), there's no evidence of the invisible characters causing any issues or being used in a way that could lead to unexpected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for invisible characters in test files that might use these examples
# Search for files that might import and use these examples
rg -l "code-mirror-documents"
# Search for tests using these examples
rg -l "test.js|test.html|test.py|test.sql|test.md|Dockerfile" tests/
Length of output: 224
Script:
#!/bin/bash
# Check how these examples are used in the identified files
rg -C 5 "code-mirror-documents" app/controllers/demo/file-contents-card.ts app/controllers/demo/code-mirror.ts
# Check if the test file uses these examples
rg -C 5 "code-mirror-documents" tests/acceptance/course-page/autofix-test.js
Length of output: 1807
Script:
#!/bin/bash
# Check how these examples are actually used in the controllers
ast-grep --pattern 'EXAMPLE_DOCUMENTS.$$$'
# Also check for any direct usage of the example strings
rg -l "\t\t ≪—— here|\t\t\t|" tests/ app/
Length of output: 127
Related to #1231
Brief
In preparation for adding
FileDiffCardCMcomponent (CodeMirror-based clone ofFileDiffCard) to the Demo Zone, this cleans up & unifies the Demo route pages.Details
This only involves the Demo page, nothing else. Changes to
FileContentsCardDemo page are inspired by the upcoming addition ofFileDiffCardCMto the Demo, and make all Demo pages behave similarly.More details
app/utils/code-mirror-documents, to reuse them across Demo pages instead of cloning:document&originalDocumentpairsFileDiffCard)FileContentsCardDemo page persistent via query paramsReset all optionslink toFileContentsCardDemo pageDarkModeTogglecomponent to appear last in the Demo Zone navigation bardiff-to-documenthelper into an utilparse-diff-as-document, to reuse it incode-mirror-documentsutilChecklist
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests
parseDiffAsDocumentfunction to ensure reliability and error handling.