Fix copy paste when clicking on row #6020
Conversation
WalkthroughThe pull request involves updates to the Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/assets/javascripts/components/input_table.ts (1)
90-93: Consider using column name constants for better maintainabilityTo make the code more maintainable and less prone to errors when column positions change, consider:
- Defining column name constants
- Creating helper methods to get column indices
- Using these throughout the code instead of hard-coded indices
Example implementation:
// At class level private static readonly COLUMNS = { NAME: 0, DESCRIPTION: 1, MAXIMUM: 2, VISIBLE: 3, ID: 4 } as const; // Helper method private getColumnIndex(column: keyof typeof ScoreItemInputTable.COLUMNS): number { return ScoreItemInputTable.COLUMNS[column]; }This would make the code more maintainable and self-documenting.
Also applies to: 102-106, 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
app/assets/javascripts/components/input_table.ts(5 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (4)
app/assets/javascripts/components/input_table.ts (4)
3-9: LGTM: Clean import organization
The addition of JspreadsheetInstanceElement type import improves type safety.
168-168: LGTM: Column configuration aligns with data structure
The hidden id column is correctly positioned as the fifth column, consistent with the data structure changes and the copy-paste fix objective.
90-93: Confirm: Moving id column fixes copy-paste functionality
The reordering of the id field to the last position aligns with the PR objective to fix the copy-paste functionality. This change works around the known jspreadsheet-ce bug (#1344).
Let's verify there are no other instances where column order assumptions might need updating:
Also applies to: 102-106
✅ Verification successful
Let's examine the specific usage of these patterns in both files to ensure there are no column order dependencies.
Column reordering is safely implemented
The code inspection shows that column order dependencies are properly handled:
- In
input_table.ts, the array indices match the new column order (name, description, maximum, visible, id) - The usage in
institution.rbis unrelated as it deals with similarity matrix operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any code that might be making assumptions about column indices
rg -l "row\[\d\]|getColumn|setColumn" .
Length of output: 120
Script:
#!/bin/bash
# Check the actual usage of row indices and column operations
rg "row\[\d\]|getColumn|setColumn" -A 3 -B 3 ./app/models/institution.rb ./app/assets/javascripts/components/input_table.ts
Length of output: 1672
224-224: Verify: Description column index might be incorrect
The setWidth call uses index 1, which appears to be correct as the description column is the second column (index 1) in the configuration. However, to prevent future issues, consider using a named constant or computed index based on the column configuration.
Let's verify the column indices:
✅ Verification successful
Let me gather more context about the table structure and column configuration.
Let me search specifically for the table structure and column definitions.
Let me search for the column configuration definition to understand the column structure.
Based on the evidence found, I can now provide the final response:
The column index 1 for description column width is correct
Looking at the columnConfig definition, we can confirm that the description column is indeed at index 1 in the configuration array:
return [
{ type: "text", title: "name", width: 200 }, // index 0
{ type: "text", title: "description", width: ... }, // index 1
{ type: "numeric", title: "maximum", width: 75 }, // index 2
...
];The setWidth(1, this.descriptionColWidth) call is using the correct index for the description column.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for column index references to ensure consistency
rg "setWidth|getColumn|setColumn" .
Length of output: 150
Script:
#!/bin/bash
# Search for table initialization and column configuration
rg -A 5 "table\s*=\s*new" app/assets/javascripts/components/input_table.ts
# Search for column-related constants or configurations
ast-grep --pattern 'const $_ = $_' app/assets/javascripts/components/input_table.ts
# Look for any column index references or array definitions
rg "columns?\[|columns?\.push|addColumn" app/assets/javascripts/components/input_table.ts
Length of output: 5613
Script:
#!/bin/bash
# Search for table configuration and column definitions
rg -B 5 -A 10 "columns:" app/assets/javascripts/components/input_table.ts
# Look for table initialization code
rg -B 5 -A 10 "this\.table\s*=" app/assets/javascripts/components/input_table.ts
# Search for any array index references to columns
rg "row\[(\d+)\]" app/assets/javascripts/components/input_table.ts
Length of output: 1946
Script:
#!/bin/bash
# Search for columnConfig property definition
rg -B 5 -A 15 "columnConfig\s*=|get columnConfig" app/assets/javascripts/components/input_table.ts
# Search for any width-related configurations
rg -B 2 -A 2 "width:" app/assets/javascripts/components/input_table.ts
Length of output: 2537
This pull request fixes copy pasting a row in the editable score items table.
This is actually caused by an open bug in jspreadsheet-ce: jspreadsheet/ce#1344
I have circumvented the bug by moving the hidden column from first to fifth position.
Closes #6019
Also closes #6018
For some reason I had picked an old version of jsspreadsheet-ce. Now we are using the most recent version.