fix(web): gate Finalize design package + trim handling reviewers asked for on #1649#1749
fix(web): gate Finalize design package + trim handling reviewers asked for on #1649#1749ecogsandbox wants to merge 1 commit into
Conversation
Carries upstream PR #1649 with the reviewer-requested trim handling that PR #1649 has not yet incorporated. Disables the Finalize button when config.apiKey or config.model is empty or whitespace-only, with a tooltip that points users to Settings. Defense-in-depth toast in handleFinalize for programmatic triggers. Trimmed values flow into finalize.trigger so the UI gate matches the daemon's apiKey.trim()/model.trim() validation. Bumps root package.json from 0.7.0 to 0.7.1. Fixes: local "Bad request: set up API key first" toast on Finalize click. References: nexu-io/open-design#1649, nexu-io/open-design#1348
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28cdaadd6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| baseUrl: config.baseUrl, | ||
| model: config.model, | ||
| model: config.model.trim(), | ||
| maxTokens: effectiveMaxTokens(config), |
There was a problem hiding this comment.
When the saved model has leading/trailing whitespace, this path now correctly sends model: config.model.trim(), but effectiveMaxTokens(config) still looks up the untrimmed value. In that scenario, known models like claude-sonnet-4-5 miss their model-specific default and fall back to 8192 tokens, so finalization can be unnecessarily truncated even though the request uses the trimmed model. Compute the token cap from the same trimmed model that is sent to the daemon.
Useful? React with 👍 / 👎.
lefarcen
added
size/S
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hey @ecogsandbox! 👋 The trim-contract analysis here is precise — the mismatch between the JS truthy check and the daemon's apiKey.trim() at import-export-routes.ts:551 is exactly the gap reviewers flagged on #1649, and your bug-fix verification steps (especially edge case 5) show it end-to-end.
Heads-up: PR #1649 by @xxiaoxiong is also open against this area — both PRs touch FinalizeDesignButton.tsx and address the Finalize gating for #1348. This PR adds the .trim() handling that #1649 hasn't yet incorporated. The maintainer team will decide which one lands; sharing so neither effort goes to waste.
@mrcfps has been assigned to review. ✨
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
Thanks for the #1348 follow-up, @ecogsandbox — the main gate looks good, but I found one trim-path mismatch that still needs a small follow-up before the behavior fully lines up with the daemon validation.
🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.| baseUrl: config.baseUrl, | ||
| model: config.model, | ||
| model: config.model.trim(), | ||
| maxTokens: effectiveMaxTokens(config), |
There was a problem hiding this comment.
apps/web/src/components/ProjectView.tsx:2316 now trims config.model before calling finalize.trigger, but maxTokens is still computed from the untrimmed config object. When a saved model has leading or trailing whitespace and there is no explicit max-token override, effectiveMaxTokens(config) falls through to 8192 because apps/web/src/state/maxTokens.ts:96 looks up the raw model string, while the request is sent with the trimmed model id. That means a padded value like claude-sonnet-4-5 can still finalize with the fallback token cap and get unnecessarily truncated even though this PR is trying to normalize whitespace-only config issues. Please derive the trimmed values once, pass the trimmed model into effectiveMaxTokens, and add a regression in apps/web/tests/state/maxTokens.test.ts for a whitespace-padded known model id.
|
@ecogsandbox friendly reminder: this PR has been waiting on an author response for more than 3 days after reviewer or maintainer feedback. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no author activity for more than 5 days after feedback may be closed automatically, but they can be reopened when work resumes. |
|
Closing this PR for now because it has been waiting on an author response for more than 5 days after reviewer or maintainer feedback. This is only a queue-management step, not a rejection of the work. If you would like to continue, please leave a comment or push an update and reopen the PR when ready. |
3 participants
Why
Use case: Mirror PR #1649's gating fix for #1348, but include the one outstanding piece of reviewer feedback that PR #1649 has not yet incorporated: trim handling on
apiKey/model.Pain being addressed: Without trim handling, whitespace-only values (
\" \") still pass the JS truthy check while the daemon route rejects them (!apiKey.trim()atapps/daemon/src/import-export-routes.ts:551). The user sees the same backendBad requesttoast this fix is meant to prevent. This is the exact.trim()contract mismatch that closed PR #1367 (per @PerishCode review feedback) and that @Siri-Ray, @lefarcen, and the Codex bot are blocking #1649 on:Happy to close this in favor of #1649 once @xxiaoxiong pushes the trim addition. Opening so reviewers can pull a verified branch in the meantime.
What users will see
Surface area
Screenshots
N/A — gating change. Disabled button + tooltip render via the native `disabled` attribute and `title`.
Bug fix verification
Validation
Fixes #1348
References #1649, #1367
Generated with Claude Code