Skip to content

Add actor generation form - #74

Open
sij411 wants to merge 4 commits into
mainfrom
feat/actor-page
Open

Add actor generation form#74
sij411 wants to merge 4 commits into
mainfrom
feat/actor-page

Conversation

@sij411

@sij411 sij411 commented Sep 8, 2026

Copy link
Copy Markdown
Member
  • Add an actor-generation form with validation and loading/error feedback.
  • Submit the selected instance global ID through Relay.

Closes #41

AI assistance

Used Codex (gpt-5.6-sol) to review the Relay/Formisch/Kobalte integration and make focused corrections to numeric input binding, an import suffix, a typo, and lint issues. I reviewed and retained the design and implementation decisions.

@sij411
sij411 marked this pull request as draft September 8, 2026 15:11
I implemented the actor-generation route with Formisch and Valibot validation, Kobalte controls, Relay global-ID mutation input, loading state, and distinct GraphQL, domain, and network error handling. Codex reviewed and explained the Relay and generated-type behavior, then made focused edits to route Kobalte numeric values into Formisch, correct a generated import suffix and typo, and resolve lint findings. I retained the design and implementation decisions.

Codex ran the full repository check successfully. I will manually verify the UI before submitting the pull request.

Assisted-by: Codex:gpt-5.6-sol
@sij411

sij411 commented Sep 8, 2026

Copy link
Copy Markdown
Member Author

Testing in progress

I pinned Seroval 1.5.4 in the web package so Solid Relay uses the same OpaqueReference implementation as Solid SSR, preventing serialization failures when entering preloaded dynamic routes. Codex diagnosed the upstream peer-version mismatch and applied the dependency and lockfile changes. I confirmed the fix in the development environment by entering the instance route and observing its GraphQL query execute normally.

Codex ran mise run check, mise run build, mise run test, and an SSR request to the affected dynamic route successfully.

Assisted-by: Codex:gpt-5.6-sol
@sij411

sij411 commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Currently, after creating actor, it stays on the actor creating page. Should i add some post actions?

image

@sij411
sij411 marked this pull request as ready for review September 9, 2026 05:06
dodok8
dodok8 previously approved these changes Sep 9, 2026
@dodok8

dodok8 commented Sep 9, 2026

Copy link
Copy Markdown
Member

How about add 'useNavigate'?

@sij411

sij411 commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

@dodok8 navigate to instance detail page?

@dodok8

dodok8 commented Sep 10, 2026

Copy link
Copy Markdown
Member

Yes.

const result = response.generateActors;
switch (result.resultType) {
case "CreateActorsSuccess": {
navigate(-1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about using navigate(-1), using more detatiled URL?

@sij411 sij411 Sep 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, do you mean explicit URL rather than navigate(-1)? If so, I have been working on navigating to instance detail page with explicit URL and popping up success toast.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Check this out: 49c3d6c

Navigate to the selected instance by slug after actor creation and refresh
its cached actor list. Add a persistent Kobalte toast region with a
dismissible success notification. Use the number field rawValue prop so
clearing the actor count leaves the input editable.

Codex generated these changes at my request. I verified that the toast
worked and reported the empty-input NaN bug, which guided
the subsequent input fix. The input fix has not been browser-verified.
Validation: mise run check and mise run test (including the build) passed;
lint reports a callback statement-count warning.

Assisted-by: Codex:gpt-6-astra
@2chanhaeng
2chanhaeng requested a review from dodok8 September 12, 2026 11:53
Comment on lines +169 to +172
case "%other": {
setErrorMessage("Unable to generate actors.");
return;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This branch is unnecessary because it's same with default branch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

without it, it violates lint rules. not satisfying the exhaustiveness check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it is a good rule... The default branch can take the role of the case "%other" branch. You can turn off the rule by adding "typescript/switch-exhaustiveness-check": ["error", { "considerDefaultExhaustiveForUnions": true }], in rules of .oxlintrc.json.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Turning off that lint error won't trigger a lint error for a new union members. if we keep that intentional, then we can turn it off, remove %other branch, and only leave a generic fallback (default). we can leave a comment above. hows that sound

Comment on lines +141 to +168
case "CreateActorsError": {
switch (result.type) {
case "InvalidSize": {
setErrorMessage("Enter at least one actor.");
return;
}
case "InstanceNotFound": {
setErrorMessage(
"The selected instance could not be found or is no longer available.",
);
return;
}
case "TooManyActors": {
setErrorMessage(result.message);
return;
}
case "%future added value": {
setErrorMessage(
"The server returned an unsupported actor-generation error.",
);
return;
}
default: {
setErrorMessage("Unable to generate actors.");
return;
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be OK for now with following:

case "CreateActorsError": {
  setErrorMessage(result.message);
  return;
}

Or,

case "CreateActorsError": {
  if (["InvalidSize", "InstanceNotFound", "TooManyActors"].includes(result.type)) {
    setErrorMessage(result.message);
  } else {
    setErrorMessage("Internal error.");
  }
  return;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They are meant to be shown to users. I don't think sending API error message directly to users is ideal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In fundamental, that's correct, but currently the error messages between both sides aren't that different and it seems like they'll be similar later on too. Also, the code is getting too long. If it still feels a bit off, how about something like this:

const ERROR_MESSAGES_BY_TYPE = {
  // [ERROR_TYPE]: ERROR_MESSAGE
}
// ...
if (result.type in ERROR_MESSAGES_BY_TYPE) {
  setErrorMessage(ERROR_MESSAGES_BY_TYPE[result.type]);
} else {
  setErrorMessage("Internal error.");
}

If the logic for each branch ends up being very different later, the current code might be better.

Comment thread packages/web/package.json
Comment on lines +169 to +172
case "%other": {
setErrorMessage("Unable to generate actors.");
return;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it is a good rule... The default branch can take the role of the case "%other" branch. You can turn off the rule by adding "typescript/switch-exhaustiveness-check": ["error", { "considerDefaultExhaustiveForUnions": true }], in rules of .oxlintrc.json.

store.get(instance)?.invalidateRecord();
}
},
onCompleted: (response, errors) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By the way, the max-statements lint warning shows on the onCompleted callback.

Comment on lines +141 to +168
case "CreateActorsError": {
switch (result.type) {
case "InvalidSize": {
setErrorMessage("Enter at least one actor.");
return;
}
case "InstanceNotFound": {
setErrorMessage(
"The selected instance could not be found or is no longer available.",
);
return;
}
case "TooManyActors": {
setErrorMessage(result.message);
return;
}
case "%future added value": {
setErrorMessage(
"The server returned an unsupported actor-generation error.",
);
return;
}
default: {
setErrorMessage("Unable to generate actors.");
return;
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In fundamental, that's correct, but currently the error messages between both sides aren't that different and it seems like they'll be similar later on too. Also, the code is getting too long. If it still feels a bit off, how about something like this:

const ERROR_MESSAGES_BY_TYPE = {
  // [ERROR_TYPE]: ERROR_MESSAGE
}
// ...
if (result.type in ERROR_MESSAGES_BY_TYPE) {
  setErrorMessage(ERROR_MESSAGES_BY_TYPE[result.type]);
} else {
  setErrorMessage("Internal error.");
}

If the logic for each branch ends up being very different later, the current code might be better.

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.

Add creating actor page for each instance

3 participants