Skip to content

Refactor of globals and State #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lsnow99 wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Refactor of globals and State #2

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03205c8
Added test
lsnow99 Jan 30, 2026
a879d41
...
lsnow99 Jan 30, 2026
5cda4e2
close image modal after loading
lsnow99 Jan 30, 2026
c004456
Merge branch 'master' into logan/const-globals
jstrieb Jan 30, 2026
b207c45
Merge branch 'master' into logan/const-globals
jstrieb Jan 30, 2026
15aac2d
switch to addSheet
lsnow99 Feb 2, 2026
ec72c46
add better Sheet.load test
lsnow99 Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
*.md
*.yml
coverage
Makefile
.prettierignore
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ test: node_modules
npx vitest run --coverage

watch-test: node_modules
npx vitest --coverage
npx vitest --coverage --bail 1

pre-commit-check: pre-commit-lint test

Expand Down
29 changes: 10 additions & 19 deletions src/App.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,8 @@
} from "./lib/helpers.js";

let { urlData } = $props();
let globals = $state(
load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]),
);
let globals;
globals = load(urlData) ?? new State([new Sheet("Sheet 1", 10, 10)]);
Copy link
Owner

@jstrieb jstrieb Jan 30, 2026

Choose a reason for hiding this comment

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

Change the constructor to not take Sheet instances, or else handle them specially. See comment below.

let table = $state();
let startHeight = $state(0);
let scrollArea = $state();
Expand Down Expand Up @@ -162,7 +161,7 @@
return undefined;
}
}
return State.load(data);
return State.loadNew(data);
}

let dontSave = $state(false);
Expand Down Expand Up @@ -309,15 +308,7 @@
return;
}
dontSave = true;
globals = Object.assign(State.load(e.state), {
currentSheetIndex: globals.currentSheetIndex,
mode: globals.mode,
helpOpen: globals.helpOpen,
editorOpen: globals.editorOpen,
imageOpen: globals.imageOpen,
elements: globals.elements,
pasteBuffer: globals.pasteBuffer,
});
globals.load(e.state);
}}
bind:innerHeight
/>
Expand All @@ -326,12 +317,12 @@
{#if navigator.maxTouchPoints <= 1}
<!-- Else float the bar above the virtual keyboard -->
<div>
<FormulaBar bind:globals bind:editor={globals.elements.formulaBar} />
<FormulaBar {globals} bind:editor={globals.elements.formulaBar} />
</div>
{/if}

<div class="tabs">
<Tabs bind:globals bind:value={globals.currentSheetIndex} />
<Tabs {globals} bind:value={globals.currentSheetIndex} />
</div>

<div
Expand All @@ -350,7 +341,7 @@
Set --width and --height default values because if Table is in a Dialog, it
will inherit the width and height of the dialog for table cells.
-->
<Table bind:globals bind:table --width="auto" --height="auto" />
<Table {globals} bind:table --width="auto" --height="auto" />
</div>

<Dialog
Expand Down Expand Up @@ -387,7 +378,7 @@
<p>Error {err}</p>
{/await}
</Details>
<SaveLoad bind:globals {imageData} />
<SaveLoad {globals} {imageData} />
</div>
</Dialog>

Expand Down Expand Up @@ -450,7 +441,7 @@
</Details>
<Details>
{#snippet summary()}Settings{/snippet}
<Settings bind:globals />
<Settings {globals} />
</Details>
</div>
</Dialog>
Expand Down Expand Up @@ -489,7 +480,7 @@
{#if navigator.maxTouchPoints > 1}
<!-- Must set top instead of bottom for correct placement on iOS -->
<div class="keyboardbar" style:top="calc({visualBottom}px - 2.5em * 2)">
<FormulaBar bind:globals bind:editor={globals.elements.formulaBar} />
<FormulaBar {globals} bind:editor={globals.elements.formulaBar} />
{#if showInputButtons}
<div class="buttonbar" style:z-index={nextZIndex()}>
{@render insertTextButton("=")}
Expand Down
2 changes: 1 addition & 1 deletion src/Cell.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@
</style>

<script>
let { cell, row, col, width, height, globals = $bindable() } = $props();
let { cell, row, col, width, height, globals } = $props();
let selected = $derived(globals.selected);
let value = $derived(cell.value);
let errorText = $derived(cell.errorText);
Expand Down
5 changes: 3 additions & 2 deletions src/SaveLoad.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

import { compressText, decompressText } from "./lib/compress.js";

let { imageData, globals = $bindable() } = $props();
let { imageData, globals } = $props();

let bottomText = $state("https://jstrieb.github.io/code-grid");
let dataUrl = $derived(
Expand All @@ -72,7 +72,8 @@
const data = reader.result.match(/,(.*)/)[1];
decompressText(data)
.then((result) => {
globals = State.load(JSON.parse(result));
globals.load(JSON.parse(result));
globals.imageOpen = false;
})
.catch((e) => {
console.error(e);
Expand Down
2 changes: 1 addition & 1 deletion src/Settings.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import Button from "./components/Button.svelte";
import NumericInput from "./components/NumericInput.svelte";

let { globals = $bindable() } = $props();
let { globals } = $props();

$effect(() => {
document
Expand Down
2 changes: 1 addition & 1 deletion src/Table.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@
import Button from "./components/Button.svelte";
import NumericInput from "./components/NumericInput.svelte";

let { globals = $bindable(), table = $bindable() } = $props();
let { globals, table = $bindable() } = $props();
let selected = $derived(globals.selected);
let sheet = $derived(globals.currentSheet);

Expand Down
2 changes: 1 addition & 1 deletion src/Tabs.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
<script>
import ContextMenu from "./components/ContextMenu.svelte";

let { globals = $bindable(), value = $bindable() } = $props();
let { globals, value = $bindable() } = $props();
let tabs = $derived(globals.sheets.map((s) => s.name));

let elements = $state(new Array(globals.sheets.length).fill());
Expand Down
25 changes: 13 additions & 12 deletions src/classes.svelte.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,29 +56,30 @@ functions.crypto = async (ticker) => {
});
forceSave = $state(0);

static load(data) {
let result = new State(
data.sheets.map((sheet) => {
constructor(sheets, formulaCode) {
this.load({ sheets, formulaCode });
Copy link
Owner

@jstrieb jstrieb Jan 30, 2026

Choose a reason for hiding this comment

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

I think this happens to work, but isn't quite correct. We can either change the constructor, or change what we pass to it. (I lean towards the latter.)

Right now, the constructor is called on Sheet instances. In a Sheet instance, the cell values are rederivable stores. In State.load, those rederivable stores are being passed as the values for each cell. Instead, the values are supposed to be raw values, not stores. Probably as soon as the formula is evaluated the raw values are thrown out in all of the test cases (except self-reference), which is why this wasn't caught by the tests. This is the problematic line:

https://github.com/lsnow99/code-grid/blob/5cda4e26ae3b0149ba7322496087d0807b03b647/src/classes.svelte.js#L75

I think the best fix is to never pass fully initialized Sheet instances to the constructor like we're currently doing elsewhere. But I am open to other ideas.

Copy link
Owner

@jstrieb jstrieb Jan 30, 2026

Choose a reason for hiding this comment

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

Fun fact: TypeScript would probably have caught this. Gonna have to set that up at some point...

Copy link
Collaborator Author

@lsnow99 lsnow99 Jan 30, 2026

Choose a reason for hiding this comment

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

Ah good catch, thank you.

Fun fact: TypeScript would probably have caught this. Gonna have to set that up at some point...

Yeah probably. Also consider jsdoc if you don't want to deal with the transpiling, but then again svelte has a compilation step anyway

}

static loadNew(data) {
return new State(data.sheets, data.formulaCode);
}

load({ sheets, formulaCode }) {
Object.assign(this, {
sheets: sheets.map((sheet) => {
let s = new Sheet(
sheet.name,
sheet.heights.length,
sheet.widths.length,
(i, j) => sheet.cells[i][j].formula,
(i, j) => sheet.cells[i][j].value,
);
s.globals = this;
s.widths = sheet.widths;
s.heights = sheet.heights;
return s;
}),
data.formulaCode,
);
return result;
}

constructor(sheets, formulaCode) {
this.sheets = sheets;
this.sheets.forEach((sheet) => {
sheet.globals = this;
formulaCode: formulaCode,
});
if (formulaCode != null) {
this.formulaCode = formulaCode;
Expand Down
26 changes: 24 additions & 2 deletions test/sheet-and-formula.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Sheet, State } from "../src/classes.svelte.js";
import { State } from "../src/classes.svelte.js";
import { test, expect, beforeEach } from "vitest";
import { evalCode, functions } from "../src/formula-functions.svelte.js";

function createSheet(cells, formulaCode = "") {
return State.load({
return State.loadNew({
sheets: [
{
name: "Sheet 1",
Expand Down Expand Up @@ -690,3 +690,25 @@ test("Binary literals", async () => {
[8, -8, -9, 7, 9],
]);
});

test("State.load preserves elements", async () => {
const state = createSheet([["1", "2", "3"]]);
await expectSheet(state.currentSheet, [[1, 2, 3]]);
state.elements.formulaBar = document.createElement("textarea");
state.elements.formulaBar.setAttribute("data-testid", "shouldPreserve");

const savedState = createSheet([["4", "5", "6"]]);
await expectSheet(savedState.currentSheet, [[4, 5, 6]]);

const testFormulaCode = "functions.testFormulaCode = () => {}";
state.load({
sheets: savedState.sheets,
formulaCode: testFormulaCode,
});

await expectSheet(state.currentSheet, [[4, 5, 6]]);
expect(state.formulaCode).toBe(testFormulaCode);
expect(state.elements.formulaBar.getAttribute("data-testid")).toBe(
"shouldPreserve",
);
});
Comment on lines +708 to +728
Copy link
Owner

@jstrieb jstrieb Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In my view, this is really two different tests: one tests that the bug you originally reported is fixed (i.e., the formula bar is not broken by loading a sheet), while the other tests that state.load works correctly for updating a sheet's data.

What might make sense instead is to add a much more comprehensive State.load test in addition and leave this one unchanged? (You could also break this one into two, but that seems extreme.) I'm thinking of one that has elements in cells and custom formulas and async formulas and stuff like that so we can verify that none of the tricky features are broken by the State.load implementation.