Skip to content

Syntax parsing to reference by cell order#1019

Merged
johbaxter merged 4 commits intodevfrom
1017-syntax-parsing-for-cell-numbers
May 1, 2025
Merged

Syntax parsing to reference by cell order#1019
johbaxter merged 4 commits intodevfrom
1017-syntax-parsing-for-cell-numbers

Conversation

@johbaxter
Copy link
Copy Markdown
Contributor

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

@johbaxter johbaxter requested a review from a team as a code owner April 30, 2025 19:46
@johbaxter johbaxter linked an issue Apr 30, 2025 that may be closed by this pull request
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

Syntax parsing to reference by cell order


User description

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Enhancement


Description

  • Support numeric cell references in renderer

  • Compute and display cell order badges in UI

  • Copy {{queryId.index}} syntax on badge click

  • Refactor styled components for badge styling


Changes walkthrough 📝

Relevant files
Enhancement
state.store.ts
Numeric cell reference parsing support                                     

libs/renderer/src/store/state/state.store.ts

  • Added numeric-based cell reference parsing for queries
  • Fallback to standard getVariable lookup otherwise
  • Wrapped getVariable call for query index logic
  • Updated TODO comment on nested loop handling
  • +37/-10 
    NotebookCell.tsx
    Clickable cell order badge UI                                                       

    packages/client/src/components/notebook/NotebookCell.tsx

  • Computed cell order number via useMemo hook
  • Displayed clickable order badge to copy reference
  • Enhanced badge hover styling and padding
  • Renamed and refactored styled components
  • +37/-6   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /review

    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memo Dependency Bug

    The dependency array in the useMemo hook for computing cellOrderNumber uses cell.query.cellList.length, but the logic reads from state.queries[cell.query.id].cellList. This mismatch can lead to stale values when the cell list changes. Update the dependencies to reflect the actual source.

    const cellOrderNumber = useMemo(() => {
        const nbCellList = state.queries[cell.query.id].cellList;
    
        let matchIndex = 0;
        nbCellList.forEach((c, i) => {
            if (c.id === cell.id) matchIndex = i;
        });
    
        return matchIndex + 1;
    }, [cell.id, cell.query.cellList.length]);
    Styling Override

    The newly added empty StyledName component has no styling, overriding the previous StyledName definition and potentially breaking the UI layout. Merge or restore necessary styles.

    const StyledName = styled(Typography)(({ theme }) => ({}));
    Numeric Path Handling

    The logic for parsing numeric indices in cell references uses parseFloat and subtracts one, but may not handle invalid or non-integer values robustly. Consider validation and using parseInt or proper error handling.

    const isNumber = !isNaN(parseFloat(path[1]))
    if(variable.type === "query" && isNumber) {
        try {
            const c = this._store.queries[variable.to].cellList[parseFloat(path[1]) - 1]
    
            const cellAlias = this.getAlias(variable.to, c.id)
            const p = path
            p.splice(0,2)
    
            value = this.getVariable(
                variable.to,
                "cell",
                [cellAlias, ...p],
                c.id,
                null
            ) 
        } catch {
            value = undefined
        }

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix undefined queryId reference

    The queryId variable is undefined in this scope and will cause a runtime error.
    Replace it with the correct cell.query.id reference when constructing the template
    string.

    packages/client/src/components/notebook/NotebookCell.tsx [503-506]

     copyTextToClipboard(
    -    `{{${queryId}.${cellOrderNumber}}}`,
    +    `{{${cell.query.id}.${cellOrderNumber}}}`,
         notification,
     );
    Suggestion importance[1-10]: 8

    __

    Why: The queryId variable isn’t defined in this scope; replacing it with cell.query.id prevents runtime errors.

    Medium
    Validate integer index and bounds

    Ensure the segment is an integer index and within the cells list bounds. Use
    parseInt and Number.isInteger to validate, then check it’s >= 1 and ≤ the
    cellList.length before indexing.

    libs/renderer/src/store/state/state.store.ts [616]

    -const isNumber = !isNaN(parseFloat(path[1]))
    +const index = parseInt(path[1], 10)
    +const isIndexValid = Number.isInteger(index) &&
    +                     index >= 1 &&
    +                     index <= this._store.queries[variable.to].cellList.length
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring the index is an integer and within cellList bounds prevents runtime errors when accessing out-of-range cells.

    Medium
    General
    Prevent mutating original array

    Avoid mutating the original array by using slice to create a new array of the
    remaining segments. This preserves path for other operations and prevents unintended
    side effects.

    libs/renderer/src/store/state/state.store.ts [622-623]

    -const p = path
    -p.splice(0,2)
    +const p = path.slice(2)
    Suggestion importance[1-10]: 6

    __

    Why: p.splice mutates the original path causing side-effects; using slice preserves immutability and avoids unintended bugs.

    Low
    Simplify lookup and fix dependencies

    Simplify the lookup using findIndex and include both cell.query.id and the actual
    nbCellList.length in the dependency array. This ensures the memo updates correctly
    when the list or query changes.

    packages/client/src/components/notebook/NotebookCell.tsx [316-325]

     const cellOrderNumber = useMemo(() => {
         const nbCellList = state.queries[cell.query.id].cellList;
    +    const index = nbCellList.findIndex(c => c.id === cell.id);
    +    return index >= 0 ? index + 1 : 0;
    +}, [cell.id, cell.query.id, state.queries[cell.query.id].cellList.length]);
     
    -    let matchIndex = 0;
    -    nbCellList.forEach((c, i) => {
    -        if (c.id === cell.id) matchIndex = i;
    -    });
    -
    -    return matchIndex + 1;
    -}, [cell.id, cell.query.cellList.length]);
    -
    Suggestion importance[1-10]: 6

    __

    Why: Using findIndex makes the code more concise and adding correct dependencies (cell.query.id and list length) ensures memo updates properly.

    Low

    @johbaxter johbaxter merged commit 801a613 into dev May 1, 2025
    3 checks passed
    @johbaxter johbaxter deleted the 1017-syntax-parsing-for-cell-numbers branch May 1, 2025 16:04
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 1, 2025

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-05-01 #1019

    • Added special syntax for referencing cells by order in expressions
    • Display and copy cell order numbers in notebook UI

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Syntax parsing for cell numbers

    2 participants