Skip to content

fix: include simulation index in RTDB writes to fix empty SimulationGrid#87

Merged
TytaniumDev merged 2 commits intomainfrom
fix/simulation-grid-rtdb-index
Feb 27, 2026
Merged

fix: include simulation index in RTDB writes to fix empty SimulationGrid#87
TytaniumDev merged 2 commits intomainfrom
fix/simulation-grid-rtdb-index

Conversation

@TytaniumDev
Copy link
Owner

Summary

  • Root cause: API PATCH handler for simulation status updates built RTDB update objects from request body fields only — index was never included because it's a stored property, not sent by the worker. The frontend's SimulationGrid builds a Map<number, SimulationStatus> keyed by index, so undefined keys caused all lookups to miss, rendering zero cells.

  • API fix: Resolve sim index from getSimulationStatus() (already fetched for state-transition guard) or parse from simId (sim_0033), include in the RTDB write object.

  • Frontend defense-in-depth:

    • Extract parseRtdbSimulations() helper that falls back to parsing index from the simId key when RTDB data lacks it
    • Only cancel REST polling (markSimsReceived) when all sims have valid indices, ensuring the fallback continues until usable data arrives
    • Guard SimulationGrid against undefined index entries and NaN totalContainers

Test plan

  • npm run lint --prefix api — type-check passes
  • npm run lint --prefix frontend — ESLint passes (zero warnings)
  • npm run build --prefix frontend — production build succeeds
  • npx vitest run — all 78 tests pass (10 new)
  • Manual: Deploy and verify grid renders on a running GCP job

New tests

File Tests
frontend/src/hooks/useJobProgress.test.ts 7 tests: parseRtdbSimulations index fallback, preservation, sorting, filtering, edge cases
frontend/src/components/SimulationGrid.test.tsx 3 tests: undefined index → pending placeholders, totalSimulations=0, totalSimulations=undefined

🤖 Generated with Claude Code

The SimulationGrid was rendering zero cells in GCP mode because:
1. API PATCH handler built RTDB update objects from request body fields only,
   never including `index` (which lives in the stored simulation record)
2. Frontend RTDB listener received sims without `index`, called
   markSimsReceived(), cancelling REST polling (which had correct data)
3. SimulationGrid's simByIndex Map keyed by undefined index, causing all
   numeric get() lookups to miss

Root cause fix:
- Resolve sim index from getSimulationStatus() or parse from simId
  (e.g. "sim_003" → 3), include in RTDB write

Defense-in-depth:
- Extract parseRtdbSimulations() helper that falls back to parsing index
  from simId key when RTDB data lacks it
- Only cancel REST polling when all sims have valid indices
- Guard SimulationGrid against undefined index and NaN totalContainers

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@TytaniumDev
Copy link
Owner Author

@claude do a code review

@gemini-code-assist
Copy link

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug where the SimulationGrid in the frontend would appear empty due to missing simulation index data in Realtime Database (RTDB) updates. The fix involves a two-pronged approach: the API now explicitly includes the index when writing simulation status to RTDB, and the frontend has been enhanced with defensive parsing and rendering logic to ensure the grid displays correctly even if index data is initially absent or malformed. This significantly improves the reliability and user experience of monitoring simulation progress.

Highlights

  • API Enhancement: The API now explicitly includes the simulation index when writing simulation status updates to the Realtime Database (RTDB), resolving the root cause of the SimulationGrid rendering issue.
  • Frontend Resilience: The frontend's SimulationGrid component has been made more robust, gracefully handling cases where simulation index data might be missing or invalid, ensuring correct display.
  • New Utility Function: A new utility function, parseRtdbSimulations, was introduced to robustly extract simulation data from RTDB, including parsing the index from simId if not explicitly provided.
  • Improved Polling Logic: The logic for canceling REST polling in the frontend was refined to ensure it only occurs when all simulations have valid indices, preventing premature polling cessation.
Changelog
  • DATA_FLOW.md
    • Updated documentation to reflect that RTDB writes now include the simulation index for frontend grid construction.
    • Added a note about the frontend's defensive parsing of index from simId and continued REST polling.
    • Included new test files in the test coverage table.
  • api/app/api/jobs/[id]/simulations/[simId]/route.ts
    • Introduced logic to retrieve the simulation index from the job store or parse it from the simId.
    • Modified the RTDB update object to include the resolved simIndex.
  • frontend/src/components/SimulationGrid.test.tsx
    • Added new test cases to verify the SimulationGrid's behavior when simulations have undefined indices.
    • Included tests to ensure the SimulationGrid handles totalSimulations being 0 or undefined without crashing.
  • frontend/src/components/SimulationGrid.tsx
    • Modified the totalContainers calculation to guard against NaN or 0 values for totalSimulations.
    • Added a check to ensure s.index is a valid number before adding simulations to the simByIndex map.
  • frontend/src/hooks/useJobProgress.test.ts
    • Added a new test file with comprehensive tests for the parseRtdbSimulations utility function.
    • Tests cover index fallback from simId, preservation of existing index, sorting, filtering of invalid entries, and handling of edge cases.
  • frontend/src/hooks/useJobProgress.ts
    • Implemented the parseRtdbSimulations helper function to process raw RTDB simulation data.
    • Integrated parseRtdbSimulations into the useJobProgress hook to parse incoming RTDB snapshots.
    • Updated the markSimsReceived logic to only trigger when all parsed simulations have valid indices.
Activity
  • The pull request was created by TytaniumDev to address an issue with the SimulationGrid appearing empty.
  • Initial changes focused on including the simulation index in RTDB writes from the API and making the frontend more robust.
  • New tests were added for both the SimulationGrid component and the useJobProgress hook's new parsing logic.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude
Copy link

claude bot commented Feb 27, 2026

Claude finished @TytaniumDev's task in 1m 53s —— View job


Code Review

  • Gather context and read changed files
  • Review API changes (route.ts)
  • Review frontend hook changes (useJobProgress.ts)
  • Review frontend component changes (SimulationGrid.tsx)
  • Review test files
  • Review DATA_FLOW.md changes
  • Compile and post final review

Review Summary

The fix is correct and well-targeted. The root cause (RTDB writes missing index because it's a stored property, not in the request body) is properly addressed. The defense-in-depth approach on the frontend is a good pattern. Overall this is solid work — I found one potential bug and a few minor observations.


API — api/app/api/jobs/[id]/simulations/[simId]/route.ts

Looks good. The index resolution logic is clean:

simIndex from getSimulationStatus() (if state is being updated)
  → fallback to parsing simId ("sim_003" → 3)
  → if still undefined, omit index from RTDB write

One subtle edge case to be aware of: if state is defined but getSimulationStatus() returns null (e.g., the sim record doesn't exist yet in the store — possible on the first PATCH), simIndex stays undefined and falls through to the simId parser. That fallback will succeed for any sim_NNN id, so in practice this is fine. Just worth noting for future maintainers.

// route.ts:99-102
if (simIndex === undefined) {
  const match = simId.match(/^sim_(\d+)$/);
  if (match) simIndex = parseInt(match[1], 10);
}

The parsed index has no range validation, but this is a worker-internal API so the simId format is controlled. Not a real concern.


Frontend Hook — frontend/src/hooks/useJobProgress.ts

Potential Bug: simsRef listener missing length guard

The parent listener (line 152) guards setSimulations with if (sims.length > 0). The simsRef listener (line 184) does not:

// Line 175-187 (simsRef listener)
const data = snapshot.val();
if (!data) return;                                    // null guard ✓

const sims = parseRtdbSimulations(data as ...);

const hasValidIndices = sims.every(...);
if (hasValidIndices) markSimsReceived();
setSimulations(sims);  // ← called even when sims === [] !

If RTDB delivers a snapshot where all keys fail index parsing (e.g., all have non-sim_NNN keys and no index field), parseRtdbSimulations returns [] and this would call setSimulations([]), potentially wiping simulation data that had already been set by the parent listener. The inconsistency with the parent listener suggests this is unintentional. Fix this →

hasValidIndices is partially redundant

After parseRtdbSimulations runs, sims only contains entries with a resolved index. The hasValidIndices check:

const hasValidIndices = sims.every(s => typeof s.index === 'number' && s.index >= 0);

...will always be true unless RTDB delivers a negative index (e.g., { index: -1 }). The typeof === 'number' part is always true by this point. Not wrong, just slightly misleading — a comment explaining "guards against negative index values from malformed RTDB data" would help readers understand why this check exists post-parse.

parseRtdbSimulations — clean extraction

The extraction of this into a pure, exported function is the right call. It's well-typed, handles the spread-then-override pattern correctly ({ simId, ...simData, index } ensures index wins over any RTDB-provided value), and returns a properly sorted array.


Frontend Component — frontend/src/components/SimulationGrid.tsx

Both guards are correct:

// Guard NaN/0 for totalSimulations
const totalContainers = totalSimulations > 0 ? Math.ceil(totalSimulations / GAMES_PER_CONTAINER) : 0;

// Skip sims without valid index
if (typeof s.index === 'number' && !Number.isNaN(s.index)) {
  simByIndex.set(s.index, s);
}

The !Number.isNaN(s.index) check is technically redundant after the typeof === 'number' guard (since NaN is a number in JS), but makes the intent explicit. Fine to keep.


Tests

Good coverage across all new tests. A few observations:

  • useJobProgress.test.ts: The 7 tests cover all meaningful paths through parseRtdbSimulations. The "prefers RTDB index over parsed simId index" edge case (index=10 from a sim_005 key) is a nice catch.
  • SimulationGrid.test.tsx: Using as unknown as SimulationStatus to inject undefined index is the right approach for testing the defensive guard. The totalSimulations=undefined test correctly validates the NaN guard.
  • Missing: There's no test covering the potential issue above — i.e., what happens when the simsRef listener fires with data that parses to an empty array after previously receiving valid sims via the parent listener. If you add the length guard, a test for that behavior would round it out.

DATA_FLOW.md

Documentation is accurate and concise. The two additions (RTDB write includes index, frontend fallback parsing) correctly reflect the new behavior.


Overall Assessment

Area Status
Root cause fix ✅ Correct
API simIndex resolution ✅ Clean, correct fallback chain
parseRtdbSimulations extraction ✅ Well-designed, testable
markSimsReceived guard ✅ Correct
SimulationGrid NaN guard ✅ Correct
simsRef listener length guard ⚠️ Missing (potential stale-clear bug)
Test coverage ✅ Solid, one gap noted
DATA_FLOW.md ✅ Updated correctly

The only actionable item is the missing if (sims.length > 0) guard in the simsRef listener at useJobProgress.ts:184. Everything else is sound.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This is a great pull request that addresses a critical bug causing the SimulationGrid to be empty. The fix is comprehensive, with changes in the API to include the simulation index in RTDB writes and defensive coding on the frontend to handle cases where the index might be missing.

I appreciate the thoroughness, including:

  • Extracting parseRtdbSimulations for better code structure and testability.
  • Adding resilience to the SimulationGrid component for edge cases.
  • Writing extensive new tests to cover the new logic and prevent regressions.
  • Updating the DATA_FLOW.md documentation to reflect the changes.

I found one potential issue in the useJobProgress hook where the REST polling could be stopped prematurely, which is a valid code quality suggestion that should be addressed in this PR. My review comment includes a suggestion to fix it.

Overall, excellent work on this fix!


markSimsReceived();
const hasValidIndices = sims.every(s => typeof s.index === 'number' && s.index >= 0);
if (hasValidIndices) markSimsReceived();

Choose a reason for hiding this comment

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

high

The current logic for deciding when to stop REST polling has a subtle bug. Array.prototype.every() returns true for an empty array. If the simsRef listener receives an empty simulations object from RTDB (e.g., before any sims have been written), parseRtdbSimulations will return an empty array []. This will cause hasValidIndices to be true on line 182, and markSimsReceived() will be called here, prematurely stopping the REST polling fallback. This could lead to the SimulationGrid remaining empty if the initial RTDB data is empty.

To fix this, you should also check that the sims array is not empty before marking sims as received. The logic in the jobRef listener (lines 152-157) correctly handles this by checking sims.length > 0.

Suggested change
if (hasValidIndices) markSimsReceived();
if (sims.length > 0 && hasValidIndices) markSimsReceived();
References
  1. Address valid code quality suggestions immediately within the current pull request, even if it expands the scope. Do not defer them to a follow-up task.

Address review feedback from Gemini and Claude:
- Array.every() returns true for empty arrays, which could prematurely
  cancel REST polling if parseRtdbSimulations returns []
- simsRef listener was missing the length guard that the parent listener
  already had, risking setSimulations([]) clearing valid data

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@TytaniumDev TytaniumDev enabled auto-merge (squash) February 27, 2026 08:40
@TytaniumDev TytaniumDev merged commit 793cf4a into main Feb 27, 2026
3 checks passed
@TytaniumDev TytaniumDev deleted the fix/simulation-grid-rtdb-index branch February 27, 2026 08:40
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.

1 participant