Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
56 changes: 16 additions & 40 deletions src/App.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,12 @@
} 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);
if (globals == null) {
globals = new State([]);
globals.addSheet("Sheet 1", 10, 10);
}
let table = $state();
let startHeight = $state(0);
let scrollArea = $state();
Expand Down Expand Up @@ -162,7 +165,7 @@
return undefined;
}
}
return State.load(data);
return State.loadNew(data);
}

let dontSave = $state(false);
Expand Down Expand Up @@ -191,26 +194,7 @@
// Allow cell changes with get or update to trigger save. Those updates
// change forceSave
globals.forceSave;
save({
sheets: [
// Spreads necessary for reactivity
...globals.sheets.map((sheet) => ({
name: sheet.name,
widths: [...sheet.widths],
heights: [...sheet.heights],
// TODO: Transpose for better compression
cells: [
...sheet.cells.map((row) =>
row.map((cell) => ({
formula: cell.formula,
value: cell.get(),
})),
),
],
})),
],
formulaCode: globals.formulaCode,
});
save(createSaveData(globals.sheets, globals.formulaCode));
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure this actually works? Last I tried to move this logic elsewhere, the $effect didn't deeply detect the reactive dependencies, and the save behavior changed: saving didn't happen often enough. I don't have behavior tests for saving, did you confirm in the UI that everything behaves correctly when you do this?

Even if it does work, I'm not sure it fits in this PR. As far as I understand, changing globals to const didn't actually break anything here, and therefore doesn't necessitate this change. Since I don't have tests, I'm hesitant to add this in with everything else.

We can talk more in-person.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're probably right about the deeply nested reactivity breaking

});
$effect(() => {
globals.currentSheetIndex;
Expand Down Expand Up @@ -309,15 +293,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 +302,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,14 +326,14 @@
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
bind:open={globals.editorOpen}
style="display: flex; flex-direction: column; align-items: stretch; overflow: hidden; gap: 0.25em;"
>
<CodeEditor numbers={true} bind:editor bind:code={globals.formulaCode} />
<CodeEditor numbers bind:editor bind:code={globals.formulaCode} />
{#if codeError}
<p style="white-space: pre; overflow-x: auto; flex-shrink: 0;">
{codeError}
Expand Down Expand Up @@ -387,7 +363,7 @@
<p>Error {err}</p>
{/await}
</Details>
<SaveLoad bind:globals {imageData} />
<SaveLoad {globals} {imageData} />
</div>
</Dialog>

Expand Down Expand Up @@ -450,7 +426,7 @@
</Details>
<Details>
{#snippet summary()}Settings{/snippet}
<Settings bind:globals />
<Settings {globals} />
</Details>
</div>
</Dialog>
Expand Down Expand Up @@ -489,7 +465,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
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
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
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
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
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
47 changes: 35 additions & 12 deletions src/classes.svelte.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,29 @@ function minmax(min, x, max) {
return Math.min(Math.max(x, min), max);
}

export function createSaveData(sheets, formulaCode) {
return {
sheets: [
// Spreads necessary for reactivity
...sheets.map((sheet) => ({
name: sheet.name,
widths: [...sheet.widths],
heights: [...sheet.heights],
// TODO: Transpose for better compression
cells: [
...sheet.cells.map((row) =>
row.map((cell) => ({
formula: cell.formula,
value: cell.get(),
})),
),
],
})),
],
formulaCode: formulaCode,
};
}

export class State {
sheets = $state([]);
currentSheetIndex = $state(0);
Expand Down Expand Up @@ -56,29 +79,29 @@ 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

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

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

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;
});
if (formulaCode != null) {
this.formulaCode = formulaCode;
Expand Down
62 changes: 60 additions & 2 deletions test/sheet-and-formula.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Sheet, State } from "../src/classes.svelte.js";
import { createSaveData, 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 @@ -31,6 +31,20 @@ function expectSheet(sheet, cells) {
);
}

function expectSheetElements(sheet, elements) {
return Promise.all(
elements
.map((row, i) =>
row.map((tagName, j) =>
expect
.poll(() => sheet.cells[i][j].element?.tagName)
.toEqual(tagName),
),
)
.flat(),
);
}

const originalFunctions = { ...functions };
beforeEach(() => {
// Restore the imported, destructively-modified, global functions object so
Expand Down Expand Up @@ -690,3 +704,47 @@ 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

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.


test("State.load correctness", async () => {
const testFormulaCode = `functions.timesTwo = (x) => 2 * x;
functions.H4 = function (x) { this.element = document.createElement("H4"); this.element.innerHTML = x; return x };
`;
const state = createSheet(
[["1", "2", "=H4(RC0+timesTwo(RC[-1]))"]],
testFormulaCode,
);
await expectSheet(state.currentSheet, [[1, 2, 5]]);
await expectSheetElements(state.currentSheet, [[undefined, undefined, "H4"]]);

const snapshot = createSaveData([state.currentSheet], testFormulaCode);

state.currentSheet.cells[0][2].formula = "=7";

state.load(snapshot);

expect(state.formulaCode).toBe(testFormulaCode);
await expectSheet(state.currentSheet, [[1, 2, 5]]);
await expectSheetElements(state.currentSheet, [[undefined, undefined, "H4"]]);
});