Skip to content

update tests + issue 32#33

Open
jottenlips wants to merge 5 commits intomainfrom
fix/inverted-board-dimensions-in-tests
Open

update tests + issue 32#33
jottenlips wants to merge 5 commits intomainfrom
fix/inverted-board-dimensions-in-tests

Conversation

@jottenlips
Copy link
Copy Markdown
Contributor

No description provided.

@jottenlips jottenlips changed the title update tests update tests + issue 32 Mar 3, 2026
@jottenlips
Copy link
Copy Markdown
Contributor Author

#32

Comment on lines +22 to +28
// make sure we are in bounds, truncate the rest for now
if (rowIndex + top >= board.length) {
return;
}
if (bitIndex + left >= board[0].length) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test should probably be added that validates a component that is larger than the board size does not increase the returned board size.

Copy link
Copy Markdown
Contributor

@natekspencer natekspencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

📝 Walkthrough

Summary by CodeRabbit

Release Notes v1.3.4

  • Chores

    • Bumped version to 1.3.4
  • Bug Fixes

    • Fixed out-of-bounds writes when layout components exceed board dimensions
  • Tests

    • Added test coverage for layout behavior when components exceed board width
    • Updated existing layout tests with revised test parameters

Walkthrough

This PR introduces bounds checking to the layoutComponents function to prevent out-of-range writes when components exceed board boundaries, alongside a cross-platform test suite expansion that validates uniform row-length behavior when a component's width exceeds the board width.

Changes

Cohort / File(s) Summary
Test Suite Expansion (Cross-platform)
php/tests/VbmlTest.php, python/tests/test_vbml.py, src/__tests__/vbml.spec.ts
Swapped board dimensions in two existing layout tests and added a new test case (testShouldProduceUniformRowLengthsWhenComponentWiderThanBoard / test_uniform_row_lengths_when_component_wider_than_board) that validates uniform row-length enforcement when a component's width exceeds the board width; assertions verify exactly 22 rows of 6 columns each with proper truncation of the first row.
Bounds Checking Logic
src/layoutComponents.ts
Added explicit bounds checks to prevent out-of-range writes when placing component matrices onto the board during relative layout fill; early returns for cells where rowIndex + top exceeds board height or bitIndex + left exceeds board width.
Version Update
package.json
Bumped version from 1.3.3 to 1.3.4.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ❌ 2

❌ Failed checks (2 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'update tests + issue 32' is vague and generic, using non-descriptive language that doesn't convey the specific nature of the changes. Use a more descriptive title that highlights the main change, such as 'Fix inverted board dimensions in tests and add bounds checking for layout components'.
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess whether any description exists that relates to the changeset. Add a description that explains the fix for issue #32, the dimension swap rationale, and the bounds checking addition to help reviewers understand the intent.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/inverted-board-dimensions-in-tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/layoutComponents.ts`:
- Around line 22-28: The code assumes board[0] exists when checking
board[0].length; add a prior guard for an empty board (if board.length === 0)
and return early before accessing board[0].length so the truncation checks using
rowIndex, top, bitIndex, and left are safe; update the existing bounds checks
around rowIndex + top and bitIndex + left to run after the empty-board guard to
avoid accessing board[0].length when board is zero-height.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 11ea29f2-97ed-462c-ba88-405ddc0fbe48

📥 Commits

Reviewing files that changed from the base of the PR and between 1bdc16f and a04ceaf.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • package.json
  • php/tests/VbmlTest.php
  • python/tests/test_vbml.py
  • src/__tests__/vbml.spec.ts
  • src/layoutComponents.ts

Comment on lines +22 to +28
// make sure we are in bounds, truncate the rest for now
if (rowIndex + top >= board.length) {
return;
}
if (bitIndex + left >= board[0].length) {
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard empty-board access before using board[0].length.

Line 26 still assumes board[0] exists. With a zero-height board, this can throw before truncation logic helps.

🔧 Proposed fix
 export const layoutComponents = (
   board: number[][],
   components: number[][][],
   absoluteComponents: { characters: number[][]; x: number; y: number }[],
   calendarComponents?: { characters: number[][]; x: number }[]
 ) => {
+  const boardHeight = board.length;
+  const boardWidth = board[0]?.length ?? 0;
+  if (boardHeight === 0 || boardWidth === 0) {
+    return board;
+  }
+
   let position = {
     top: 0,
     left: 0,
     height: 0,
   };

   components.forEach((component) => {
     // If the component size plus the currently occupied size is larger than the board width, flow to the next line
-    const newLine = position.left + component[0].length > board[0].length;
+    const newLine = position.left + component[0].length > boardWidth;
     const left = newLine ? 0 : position.left;
     const top = newLine ? position.top + position.height : position.top;

     // Fill in the individual component bits over the empty board
     component.forEach((row, rowIndex) => {
       row.forEach((bit, bitIndex) => {
         // make sure we are in bounds, truncate the rest for now
-        if (rowIndex + top >= board.length) {
+        if (rowIndex + top >= boardHeight) {
           return;
         }
-        if (bitIndex + left >= board[0].length) {
+        if (bitIndex + left >= boardWidth) {
           return;
         }
         board[rowIndex + top][bitIndex + left] = bit;
       });
     });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/layoutComponents.ts` around lines 22 - 28, The code assumes board[0]
exists when checking board[0].length; add a prior guard for an empty board (if
board.length === 0) and return early before accessing board[0].length so the
truncation checks using rowIndex, top, bitIndex, and left are safe; update the
existing bounds checks around rowIndex + top and bitIndex + left to run after
the empty-board guard to avoid accessing board[0].length when board is
zero-height.

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.

3 participants