Skip to content
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.

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

fix: add unique id to app created page #257

Closed
wants to merge 1 commit into from
Closed

fix: add unique id to app created page #257

wants to merge 1 commit into from

Conversation

reneaaron
Copy link
Contributor

Fixes #252

State seems to be persisted in browsers history, we fix that by using a unique URI for every app that has been created.

Copy link
Member

@im-adithya im-adithya left a comment

Choose a reason for hiding this comment

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

Nice 🙌

@im-adithya
Copy link
Member

Did you try passing { state: null } after we go to /apps on having lastEventAt here? I can't reproduce even the actual issue

});
// Adding a unique id prevents https://github.com/getAlby/hub/issues/252
navigate(
`/apps/created?id=${createAppResponse.pairingPublicKey}${app ? `&app=${app.id}` : ""}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

It was already here, but why is there a conditional here? this seems wrong?

Also, the pairingPublicKey should be unique per app

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I completely misread this. I am wrong 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

but it's weird, the navigation state should work. And it seems like it's not

Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

something is wrong here (not sure what)

@rolznz
Copy link
Contributor

rolznz commented Jul 13, 2024

@im-adithya @reneaaron I found how to consistently reproduce the issue:

  1. Create a new app and do at least one NWC event for it
  2. Delete the app
  3. Create a new app

Step 3. Will lead to this bug.

The pubkey is different for each app, but SQLITE is reusing app IDs rather than properly auto-incrementing.

We fetch events by pubkey from the API, but internally we query by app ID.

And for some reason when we delete apps, the request_events are not deleted.

So this PR does actually not fix the issue, and there's nothing wrong with the routing (which passes different state each time)

@rolznz
Copy link
Contributor

rolznz commented Jul 13, 2024

For some reason the foreign key on delete constraint is not working

sqlite> .schema request_events
CREATE TABLE IF NOT EXISTS "request_events" (`id` integer,`app_id` integer null,`nostr_id` text UNIQUE,`state` text,`created_at` datetime,`updated_at` datetime, method TEXT, content_data TEXT,PRIMARY KEY (`id`),CONSTRAINT `fk_request_events_app` FOREIGN KEY (`app_id`) REFERENCES `apps`(`id`) ON DELETE CASCADE);
CREATE UNIQUE INDEX `idx_request_events_nostr_id` ON `request_events`(`nostr_id`);
CREATE INDEX `idx_request_events_app_id` ON `request_events`(`app_id`);
CREATE INDEX idx_request_events_app_id_and_id ON request_events(app_id, id);
CREATE INDEX `idx_request_events_method` ON `request_events`(`method`);

@rolznz rolznz closed this Jul 13, 2024
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.

Creating a new app instantly navigates
3 participants